Hi,

On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
>
> Signed-off-by: Mikko Perttunen <[email protected]>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_tegra.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 396 insertions(+)
> create mode 100644 drivers/ata/ahci_tegra.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 7671dba..e65d400 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -141,6 +141,15 @@ config AHCI_SUNXI
>
> If unsure, say N.
>
> +config AHCI_TEGRA
> + tristate "NVIDIA Tegra124 AHCI SATA support"
> + depends on ARCH_TEGRA
> + help
> + This option enables support for the NVIDIA Tegra124 SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config AHCI_XGENE
> tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
> depends on PHY_XGENE
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 5a02aee..ae41107 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_ST) += ahci_st.o libahci.o libahci_platform.o
> +obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o
>
> # SFF w/ custom DMA
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> new file mode 100644
> index 0000000..a10aac0
> --- /dev/null
> +++ b/drivers/ata/ahci_tegra.c
> @@ -0,0 +1,386 @@
> +/*
> + * drivers/ata/ahci_tegra.c
> + *
> + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
> + *
> + * Author:
> + * Mikko Perttunen <[email protected]>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/ahci_platform.h>
> +#include <linux/reset.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/tegra-powergate.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/tegra-soc.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0 0x180
> +#define SATA_CONFIGURATION_EN_FPCI (1<<0)

minor nitpick: BIT() macro can be used here

> +#define SCFG_OFFSET 0x1000
> +
> +#define T_SATA0_CFG_1 0x04
> +#define T_SATA0_CFG_1_IO_SPACE (1<<0)
> +#define T_SATA0_CFG_1_MEMORY_SPACE (1<<1)
> +#define T_SATA0_CFG_1_BUS_MASTER (1<<2)
> +#define T_SATA0_CFG_1_SERR (1<<8)

ditto

> +#define T_SATA0_CFG_9 0x24
> +
> +#define SATA_FPCI_BAR5 0x94
> +
> +#define SATA_INTR_MASK 0x188
> +#define SATA_INTR_MASK_IP_INT_MASK (1<<16)

ditto

> +#define T_SATA0_AHCI_HBA_CAP_BKDR 0x300
> +
> +#define T_SATA0_BKDOOR_CC 0x4a4
> +
> +#define T_SATA0_CFG_SATA 0x54c
> +#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN (1<<12)

ditto

> +#define T_SATA0_CFG_MISC 0x550
> +
> +#define T_SATA0_INDEX 0x680
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1 0x690
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK 0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT 0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK (0xff<<8)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT 8
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2 0x694
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK 0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT 0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK (0xff<<12)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT 12
> +
> +#define T_SATA0_CHX_PHY_CTRL2 0x69c
> +#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1 0x23
> +
> +#define T_SATA0_CHX_PHY_CTRL11 0x6d0
> +#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ (0x2800<<16)
> +
> +struct sata_pad_calibration {
> + u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, gen2_tx_peak;
> +};
> +
> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> + {0x18, 0x04, 0x18, 0x0a},
> + {0x0e, 0x04, 0x14, 0x0a},
> + {0x0e, 0x07, 0x1a, 0x0e},
> + {0x14, 0x0e, 0x1a, 0x0e},
> +};
> +
> +struct tegra_ahci_priv {
> + struct platform_device *pdev;
> + void __iomem *sata_regs;
> + struct reset_control *sata_rst;
> + struct reset_control *sata_oob_rst;
> + struct reset_control *sata_cold_rst;
> + struct regulator_bulk_data supplies[3];
> +};
> +
> +static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
> + unsigned long offset)
> +{
> + writel(value, tegra->sata_regs + offset);
> +}
> +
> +static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
> + unsigned long offset)
> +{
> + return readl(tegra->sata_regs + offset);
> +}

sata_writel() and sata_read() static inlines don't seem to add any value.

Can they be removed?

> +static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> +{
> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
> + int ret;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> + tegra->supplies);
> + if (ret)
> + return ret;
> +
> + reset_control_assert(tegra->sata_rst);
> + reset_control_assert(tegra->sata_oob_rst);
> + reset_control_assert(tegra->sata_cold_rst);
> +
> + ret = tegra_powergate_power_on(TEGRA_POWERGATE_SATA);
> + if (ret)
> + goto reset_deassert;

Shouldn't the driver disable regulators on failure?

[ tegra_ahci_power_off() does it so it should also be done here. ]

> + /* Enable clocks */
> + ret = ahci_platform_enable_resources(hpriv);
> + if (ret)
> + goto power_off;
> +
> + udelay(10);
> +
> + ret = tegra_powergate_remove_clamping(TEGRA_POWERGATE_SATA);
> + if (ret)
> + goto disable_resources;
> +
> + udelay(10);
> +
> + reset_control_deassert(tegra->sata_cold_rst);
> + reset_control_deassert(tegra->sata_oob_rst);
> + reset_control_deassert(tegra->sata_rst);
> +
> + return 0;
> +
> +disable_resources:
> + ahci_platform_disable_resources(hpriv);
> +
> +power_off:
> + tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +reset_deassert:
> + reset_control_deassert(tegra->sata_cold_rst);
> + reset_control_deassert(tegra->sata_oob_rst);
> + reset_control_deassert(tegra->sata_rst);

reset_control_deassert() sequence is can be moved to separate static
inline helper to prevent code duplication.

> + return ret;
> +}
> +
> +static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> +{
> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +
> + reset_control_assert(tegra->sata_rst);
> + reset_control_assert(tegra->sata_oob_rst);
> + reset_control_assert(tegra->sata_cold_rst);

Same for reset_control_assert() sequence.

> + ahci_platform_disable_resources(hpriv);
> +
> + tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> + reset_control_deassert(tegra->sata_cold_rst);
> + reset_control_deassert(tegra->sata_oob_rst);
> + reset_control_deassert(tegra->sata_rst);
> +
> + regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +}
> +
> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> +{
> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
> + int ret;
> + unsigned int val;
> + struct sata_pad_calibration calib;
> +
> + ret = tegra_ahci_power_on(hpriv);
> + if (ret) {
> + dev_err(&tegra->pdev->dev,
> + "failed to power on ahci controller: %d\n", ret);
> + return ret;
> + }
> +
> + val = sata_readl(tegra, SATA_CONFIGURATION_0);
> + val |= SATA_CONFIGURATION_EN_FPCI;
> + sata_writel(tegra, val, SATA_CONFIGURATION_0);
> +
> + /* Pad calibration */
> +
> + ret = tegra_fuse_readl(0x224, &val);
> + if (ret) {
> + dev_err(&tegra->pdev->dev,
> + "failed to read sata calibration fuse: %d\n", ret);
> + return ret;
> + }
> +
> + calib = tegra124_pad_calibration[val];
> +
> + sata_writel(tegra, (1 << 0), SCFG_OFFSET + T_SATA0_INDEX);
> +
> + val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> + val |= calib.gen1_tx_amp <<
> + T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> + val |= calib.gen1_tx_peak <<
> + T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> + sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> + val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> + val |= calib.gen2_tx_amp <<
> + T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> + val |= calib.gen2_tx_peak <<
> + T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> + sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> + sata_writel(tegra, T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> + sata_writel(tegra, T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> + sata_writel(tegra, 0, SCFG_OFFSET + T_SATA0_INDEX);
> +
> + /* Program controller device ID */
> +
> + val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CFG_SATA);
> + val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> + sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> + sata_writel(tegra, 0x01060100, SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +
> + val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CFG_SATA);
> + val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> + sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> + /* Enable IO & memory access, bus master mode */
> +
> + val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CFG_1);
> + val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> + T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
> + sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_1);
> +
> + /* Program SATA MMIO */
> +
> + sata_writel(tegra, 0x10000 << 4, SATA_FPCI_BAR5);
> + sata_writel(tegra, 0x08000 << 13, SCFG_OFFSET + T_SATA0_CFG_9);
> +
> + /* Unmask SATA interrupts */
> +
> + val = sata_readl(tegra, SATA_INTR_MASK);
> + val |= SATA_INTR_MASK_IP_INT_MASK;
> + sata_writel(tegra, val, SATA_INTR_MASK);
> +
> + return 0;
> +}
> +
> +static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
> +{
> + tegra_ahci_power_off(hpriv);
> +}
> +
> +static void tegra_ahci_host_stop(struct ata_host *host)
> +{
> + struct ahci_host_priv *hpriv = host->private_data;
> +
> + tegra_ahci_controller_deinit(hpriv);
> +}
> +
> +static struct ata_port_operations ahci_tegra_port_ops = {
> + .inherits = &ahci_platform_ops,
> + .host_stop = tegra_ahci_host_stop,
> +};
> +
> +static const struct ata_port_info ahci_tegra_port_info = {
> + .flags = AHCI_FLAG_COMMON,
> + .pio_mask = ATA_PIO4,
> + .udma_mask = ATA_UDMA6,
> + .port_ops = &ahci_tegra_port_ops,
> +};
> +
> +static const struct of_device_id tegra_ahci_of_match[] = {
> + { .compatible = "nvidia,tegra124-ahci" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> +
> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct ahci_host_priv *hpriv;
> + struct tegra_ahci_priv *tegra;
> + int ret;
> +
> + match = of_match_device(tegra_ahci_of_match, &pdev->dev);
> + if (!match)
> + return -EINVAL;
> +
> + hpriv = ahci_platform_get_resources(pdev);
> + if (IS_ERR(hpriv))
> + return PTR_ERR(hpriv);
> +
> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> + if (!tegra)
> + return -ENOMEM;
> +
> + hpriv->plat_data = tegra;
> +
> + tegra->pdev = pdev;
> +
> + tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
> + platform_get_resource(pdev, IORESOURCE_MEM, 1));
> + if (IS_ERR(tegra->sata_regs)) {
> + dev_err(&pdev->dev, "Failed to get SATA IO memory");
> + return PTR_ERR(tegra->sata_regs);
> + }
> +
> + tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
> + if (IS_ERR(tegra->sata_rst)) {
> + dev_err(&pdev->dev, "Failed to get sata reset");
> + return PTR_ERR(tegra->sata_rst);
> + }
> +
> + tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
> + if (IS_ERR(tegra->sata_oob_rst)) {
> + dev_err(&pdev->dev, "Failed to get sata-oob reset");
> + return PTR_ERR(tegra->sata_oob_rst);
> + }
> +
> + tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
> + if (IS_ERR(tegra->sata_cold_rst)) {
> + dev_err(&pdev->dev, "Failed to get sata-cold reset");
> + return PTR_ERR(tegra->sata_cold_rst);
> + }
> +
> + tegra->supplies[0].supply = "avdd";
> + tegra->supplies[1].supply = "hvdd";
> + tegra->supplies[2].supply = "vddio";
> +
> + ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> + tegra->supplies);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to get regulators");
> + return ret;
> + }
> +
> + ret = tegra_ahci_controller_init(hpriv);
> + if (ret)
> + return ret;
> +
> + ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
> + 0, 0, 0);
> + if (ret) {
> + tegra_ahci_controller_deinit(hpriv);
> + return ret;
> + }
> +
> + return 0;
> +};
> +
> +static struct platform_driver tegra_ahci_driver = {
> + .probe = tegra_ahci_probe,
> + .remove = ata_platform_remove_one,
> + .driver = {
> + .name = "tegra-ahci",
> + .owner = THIS_MODULE,
> + .of_match_table = tegra_ahci_of_match,
> + },

This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.

> +};
> +module_platform_driver(tegra_ahci_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <[email protected]>");
> +MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_LICENSE("GPL v2");

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

2014-06-17 12:16:30

2014-07-08 13:08:34

On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > >
> > > Yes, but the problem is that you also need clocks and reset of other modules
> > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > someday which means we would need to control the domain and therefore would
> > > need to tell that driver about the ISPs clocks and resets even though the
> > > driver doesn't know anything about the ISP hw otherwise.
> >
> > Can't we make powergates reference counted so that they don't get
> > disabled as long as there are any users? Looking for example at the
>
> We could, but then why not switch to the powerdomain code and make powering
> off a domain a NOP until we sorted out the context save/restore or fixed
> the framework to allow for suspend without turning off the domains?

Well, one of the reasons why I'm not sure it's worth the effort at this
point is that we can't get rid of the tegra_powergate_*() API anyway
because of backwards compatibility. So we're going to add code (without
getting rid of old code) merely to support some generic framework. That
doesn't sound very useful to me.

> > display controller driver, modules don't seem to care overly much about
> > the powergate's state except that it needs to be turned on before they
> > touch some of the registers.
> >
> > From a bit of experimentation it also seems like the sequence encoded
> > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > couldn't find an authoritative reference for that either, so I'm tempted
> > to conclude that it was simply cargo-culted from the dark-ages.
> >
> > So I'm thinking that if we ever move to use power domains for this, we
> > may be able to just drop any extra handling (well, we'd need to keep it
> > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > and reset resources.
> >
> > On the other hand, given that we already need to keep the existing code
> > for backwards-compatibility, I'm not sure there's a real advantage in
> > turning them into power domains, since we'd be adding extra code without
> > an clear gains (especially since it seems like we'd need even more code
> > to properly handle suspend/resume in drivers that need powergates).
> >
>
> Unless we fix the framework to require context save/restore for suspend.
> There is a good reason to do that. context save/restore requires energy
> as well, so it's not a given that turning off domains in system suspend
> will save power.

I'm not sure I follow. "require context save/restore for suspend" is
what many drivers currently don't support, hence we can't use the
generic PM domains. Perhaps what you're saying is that the PM domain
core code should be enhanced so that domains can be marked so that they
stay on during a suspend/resume cycle.

Such functionality could be part of a specifier in a DT binding. It's
technically not a description of the hardware, but it encodes a device's
requirements regarding suspend/resume latency.

Thierry

Attachments:

(No filename) (3.13 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-09 08:33:21

by Peter De Schrijver

[permalink] [raw]

Subject: Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > >
> > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > someday which means we would need to control the domain and therefore would
> > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > driver doesn't know anything about the ISP hw otherwise.
> > >
> > > Can't we make powergates reference counted so that they don't get
> > > disabled as long as there are any users? Looking for example at the
> >
> > We could, but then why not switch to the powerdomain code and make powering
> > off a domain a NOP until we sorted out the context save/restore or fixed
> > the framework to allow for suspend without turning off the domains?
>
> Well, one of the reasons why I'm not sure it's worth the effort at this
> point is that we can't get rid of the tegra_powergate_*() API anyway
> because of backwards compatibility. So we're going to add code (without
> getting rid of old code) merely to support some generic framework. That
> doesn't sound very useful to me.
>

We can also convert the existing users to genpd. Today there are only 2 users
(gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
an impossible task.

> > > display controller driver, modules don't seem to care overly much about
> > > the powergate's state except that it needs to be turned on before they
> > > touch some of the registers.
> > >
> > > From a bit of experimentation it also seems like the sequence encoded
> > > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > > couldn't find an authoritative reference for that either, so I'm tempted
> > > to conclude that it was simply cargo-culted from the dark-ages.
> > >
> > > So I'm thinking that if we ever move to use power domains for this, we
> > > may be able to just drop any extra handling (well, we'd need to keep it
> > > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > > and reset resources.
> > >
> > > On the other hand, given that we already need to keep the existing code
> > > for backwards-compatibility, I'm not sure there's a real advantage in
> > > turning them into power domains, since we'd be adding extra code without
> > > an clear gains (especially since it seems like we'd need even more code
> > > to properly handle suspend/resume in drivers that need powergates).
> > >
> >
> > Unless we fix the framework to require context save/restore for suspend.
> > There is a good reason to do that. context save/restore requires energy
> > as well, so it's not a given that turning off domains in system suspend
> > will save power.
>
> I'm not sure I follow. "require context save/restore for suspend" is
> what many drivers currently don't support, hence we can't use the
> generic PM domains. Perhaps what you're saying is that the PM domain
> core code should be enhanced so that domains can be marked so that they
> stay on during a suspend/resume cycle.
>

Exactly.

Cheers,

Peter.

2014-07-09 10:26:00

On Wed, Jul 09, 2014 at 02:08:16PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 12:25:52PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > > > Old Signed by an unknown key
> > > >
> > > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > >
> > > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > > someday which means we would need to control the domain and therefore would
> > > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > >
> > > > > > Can't we make powergates reference counted so that they don't get
> > > > > > disabled as long as there are any users? Looking for example at the
> > > > >
> > > > > We could, but then why not switch to the powerdomain code and make powering
> > > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > > the framework to allow for suspend without turning off the domains?
> > > >
> > > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > > because of backwards compatibility. So we're going to add code (without
> > > > getting rid of old code) merely to support some generic framework. That
> > > > doesn't sound very useful to me.
> > > >
> > >
> > > We can also convert the existing users to genpd. Today there are only 2 users
> > > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > > an impossible task.
> >
> > We can certainly do that. What I'm trying to say is that since people
> > may be running newer versions of the kernel with a DTB that doesn't have
> > the necessary properties to hook up power domains, we have to keep calls
> > to tegra_powergate_*() functions as-is, lest we break those setups.
> >
>
> For those 2 domains we can find the necessary clocks and resets by parsing
> the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> even needed as we can always register some extra clkdev's to get them. There
> is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> but that's not too bad I think.

Even if we could really do this, at this point I don't see an advantage.
All that it would be doing is move to some subsystem that doesn't quite
match what we need just for the sake of moving to that subsystem. Having
a Tegra-specific API doesn't sound so bad anymore.

Thierry

Attachments:

(No filename) (2.94 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-09 12:44:07

by Peter De Schrijver

[permalink] [raw]

Subject: Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > For those 2 domains we can find the necessary clocks and resets by parsing
> > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > even needed as we can always register some extra clkdev's to get them. There
> > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > but that's not too bad I think.
>
> Even if we could really do this, at this point I don't see an advantage.
> All that it would be doing is move to some subsystem that doesn't quite
> match what we need just for the sake of moving to that subsystem. Having
> a Tegra-specific API doesn't sound so bad anymore.
>

The advantage would be that we can use LP0/SC7 as a cpuidle state. Also system
resume from LP0 can be faster as we potentially don't have to resume all
domains at once.

Cheers,

Peter.

2014-07-09 12:56:22

On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > but that's not too bad I think.
> > > >
> > > > Even if we could really do this, at this point I don't see an advantage.
> > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > match what we need just for the sake of moving to that subsystem. Having
> > > > a Tegra-specific API doesn't sound so bad anymore.
> > > >
> > >
> > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> >
> > How is that going to work? And why does it need powergates to be
>
> pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> when all devices in a domain are idle. It can then decide to turn off
> the domain (based on the decision of a per domain governor). If all
> domains are off (except for the non-powergateable domain), a special cpuidle
> state can be enabled by genpd which will initiate a transition to LP0 without
> actually doing a full system suspend.

Okay, I see.

> > implemented as power domains? If we switch off power gates, then we need
> > to restore context in the drivers anyway, therefore I assume .suspend()
> > and .resume() would need to be called, in which case powergate handling
> > can surely be done at that stage, can't it?
> >
>
> .suspend() and .resume() are not used for this. genpd uses other per device
> callbacks to save and restore the state which are invoked when the domain
> is turned off and on (.save_state and .restore_state). The major difference
> with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> prevent any new requests to the driver, finalize or cancel all outstanding
> requests and save the hw context. .save_state will only be called when the
> device is idle (based on the refcount controlled by pm_runtime_get() and
> pm_runtime_put()) which means it only has to handle saving the hw context.

With the above, would it be possible to make turning off the domain
conditional on whether or not all devices in the domain implement
.save_state() and .restore_state()? That would allow us to convert to
power domains and then stage in context save/restore in drivers (or even
leave it out if there's not enough to be gained from turning the
partition off).

> > > Also system
> > > resume from LP0 can be faster as we potentially don't have to resume all
> > > domains at once.
> >
> > I don't understand what that's got to do with anything. If we call into
> > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > code, then we have full control over suspend/resume in the drivers, and
> > therefore don't need to resume all at once either.
>
> But then we would be duplicating all the bookkeeping required for this? What's
> the point of that?

We're doing fine without any bookkeeping currently. I understand that
this may change eventually, but I'm hesitant to start any conversion
like this before we don't have a better understanding of how it should
work (and actual use-cases which we can test). Also we've seen in the
past that coding things up before we have enough use-cases we're bound
to fail at coming up with a proper binding and then we have to keep
carrying loads of code for compatibility.

So if you're willing to give this a shot, I'm not at all opposed to it
generally. But we need to make sure that both the binding is reasonably
future-proof and that we can actually test things like reference-counted
power domains.

Now in the meantime there are a bunch of other drivers that will need to
use the powergate API. DC is one of them. We haven't needed this before
because we assumed the partitions would be on by default. That's not
always the case apparently (ChromeOS does some funky things here). Both
the SATA and XUSB drivers that have been posted use them as well and the
nouveau driver that Alex has been working on uses at least parts of it.
I don't think it's fair to keep them from being merged while we're
trying to make the transition to power domains, but we should keep an
eye on what's happening there so it doesn't conflict with any of the
work we're planning for power domains.

Thierry

Attachments:

(No filename) (4.64 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-09 16:09:33

by Peter De Schrijver

[permalink] [raw]

Subject: Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

On Wed, Jul 09, 2014 at 04:42:18PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > >
> > > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > > but that's not too bad I think.
> > > > >
> > > > > Even if we could really do this, at this point I don't see an advantage.
> > > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > > match what we need just for the sake of moving to that subsystem. Having
> > > > > a Tegra-specific API doesn't sound so bad anymore.
> > > > >
> > > >
> > > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> > >
> > > How is that going to work? And why does it need powergates to be
> >
> > pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> > when all devices in a domain are idle. It can then decide to turn off
> > the domain (based on the decision of a per domain governor). If all
> > domains are off (except for the non-powergateable domain), a special cpuidle
> > state can be enabled by genpd which will initiate a transition to LP0 without
> > actually doing a full system suspend.
>
> Okay, I see.
>
> > > implemented as power domains? If we switch off power gates, then we need
> > > to restore context in the drivers anyway, therefore I assume .suspend()
> > > and .resume() would need to be called, in which case powergate handling
> > > can surely be done at that stage, can't it?
> > >
> >
> > .suspend() and .resume() are not used for this. genpd uses other per device
> > callbacks to save and restore the state which are invoked when the domain
> > is turned off and on (.save_state and .restore_state). The major difference
> > with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> > prevent any new requests to the driver, finalize or cancel all outstanding
> > requests and save the hw context. .save_state will only be called when the
> > device is idle (based on the refcount controlled by pm_runtime_get() and
> > pm_runtime_put()) which means it only has to handle saving the hw context.
>
> With the above, would it be possible to make turning off the domain
> conditional on whether or not all devices in the domain implement
> .save_state() and .restore_state()? That would allow us to convert to
> power domains and then stage in context save/restore in drivers (or even
> leave it out if there's not enough to be gained from turning the
> partition off).
>

Maybe. I would have to check that.

> > > > Also system
> > > > resume from LP0 can be faster as we potentially don't have to resume all
> > > > domains at once.
> > >
> > > I don't understand what that's got to do with anything. If we call into
> > > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > > code, then we have full control over suspend/resume in the drivers, and
> > > therefore don't need to resume all at once either.
> >
> > But then we would be duplicating all the bookkeeping required for this? What's
> > the point of that?
>
> We're doing fine without any bookkeeping currently. I understand that
> this may change eventually, but I'm hesitant to start any conversion
> like this before we don't have a better understanding of how it should
> work (and actual use-cases which we can test). Also we've seen in the
> past that coding things up before we have enough use-cases we're bound
> to fail at coming up with a proper binding and then we have to keep
> carrying loads of code for compatibility.
>
> So if you're willing to give this a shot, I'm not at all opposed to it
> generally. But we need to make sure that both the binding is reasonably
> future-proof and that we can actually test things like reference-counted
> power domains.
>
> Now in the meantime there are a bunch of other drivers that will need to
> use the powergate API. DC is one of them. We haven't needed this before
> because we assumed the partitions would be on by default. That's not
> always the case apparently (ChromeOS does some funky things here). Both
> the SATA and XUSB drivers that have been posted use them as well and the
> nouveau driver that Alex has been working on uses at least parts of it.
> I don't think it's fair to keep them from being merged while we're
> trying to make the transition to power domains, but we should keep an
> eye on what's happening there so it doesn't conflict with any of the
> work we're planning for power domains.

The problem with this is that moving to the genpd APIs will become much more
difficult I'm afraid. I think we should maybe just make the pmc driver turn
on all the domains which were turned off by the bootloader. That way the
drivers don't need to handle the powerdomains at all for the time being.

Cheers,

Peter.