[PATCH V4 00/16] Add generic PM domain support for Tegra

[PATCH V4 00/16] Add generic PM domain support for Tegra

[Jon Hunter]

Adds generic PM domain support for Tegra SoCs but so far it is only
enabled Tegra 64-bit devices. There is no reason why this cannot be
enabled for Tegra 32-bit devices, but to keep the patch series to a
minimum only 64-bit devices are enabled.

This series has been boot tested on Tegra210 as well as various 32-bit
Tegra platforms.

Summary of changes since V3 [0]:
- Dropped tegra124 support for now
- Removed MC flush support per feedback from Thierry
- Cleaned up the PMC changes per feedback from Thierry
- Added support for tegra210

Series Summary:
Patch 1:    Adds the new function of_reset_control_get_by_index().
Patch 2-9:  PMC clean-up and fixes
Patch 10:   Adds function to remove a generic PM domain
Patch 11:   Updates DT documentation for PMC w.r.t Tegra210
Patch 12:   Adds DT documentation for tegra generic PM domains
Patch 13:   Adds PMC generic DM domains support
Patch 14:   Adds audio clock for Tegra210 audio power-domain
Patch 15:   Adds DT bindings for Tegra210 audio power-domain
Patch 16:   Enable generic PM domains for tegra64

[0] http://comments.gmane.org/gmane.linux.ports.tegra/22944

Jon Hunter (15):
  soc: tegra: pmc: Add missing structure members to kernel-doc
  soc: tegra: pmc: Fix sparse warning for tegra_pmc_init_tsense_reset
  soc: tegra: pmc: Remove debugfs entry on probe failure
  soc: tegra: pmc: Avoid extra remapping of PMC registers
  soc: tegra: pmc: Wait for powergate state to change
  soc: tegra: pmc: Remove non-existing power partitions for T210
  soc: tegra: pmc: Fix checking of valid partitions
  soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC
  PM / Domains: Add function to remove a pm-domain
  Documentation: DT: bindings: Update NVIDIA PMC for Tegra210
  Documentation: DT: bindings: Add power domain info for NVIDIA PMC
  soc: tegra: pmc: Add generic PM domain support
  clk: tegra210: Add the APB2APE audio clock
  ARM64: tegra: Add audio PM domain device node for Tegra210
  ARM64: tegra: select PM_GENERIC_DOMAINS

Vince Hsu (1):
  reset: add of_reset_control_get_by_index()

 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      |  66 ++-
 arch/arm/boot/dts/tegra124.dtsi                    |   1 +
 arch/arm/mach-tegra/platsmp.c                      |  16 +-
 arch/arm64/Kconfig.platforms                       |   2 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi           |  11 +-
 drivers/base/power/domain.c                        |  26 +
 drivers/clk/tegra/clk-id.h                         |   1 +
 drivers/clk/tegra/clk-tegra-periph.c               |   1 +
 drivers/clk/tegra/clk-tegra210.c                   |   1 +
 drivers/reset/core.c                               |  40 +-
 drivers/soc/tegra/pmc.c                            | 586 +++++++++++++++++++--
 include/dt-bindings/clock/tegra210-car.h           |   2 +-
 include/dt-bindings/power/tegra-powergate.h        |  35 ++
 include/linux/pm_domain.h                          |   5 +
 include/linux/reset.h                              |   9 +
 include/soc/tegra/pmc.h                            |  38 +-
 16 files changed, 720 insertions(+), 120 deletions(-)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 01/16] reset: add of_reset_control_get_by_index()

[Jon Hunter]

From: Vince Hsu <vinceh@nvidia.com>

Add of_reset_control_get_by_index() to allow the drivers to get reset
device without knowing its name.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
[jonathanh@nvidia.com: Updated stub function to return -ENOTSUPP instead
 of -ENOSYS which should only be used for system calls.]
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

---
v3: addressed comments from Philipp
v2: minor changes according to Alex's comments
---
 drivers/reset/core.c  | 40 +++++++++++++++++++++++++++++-----------
 include/linux/reset.h |  9 +++++++++
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 7955e00d04d4..81ae17d15480 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -141,27 +141,24 @@ int reset_control_status(struct reset_control *rstc)
 EXPORT_SYMBOL_GPL(reset_control_status);
 
 /**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * of_reset_control_get_by_index - Lookup and obtain a reference to a reset
+ * controller by index.
  * @node: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
+ * @index: index of the reset controller
  *
- * Use of id names is optional.
+ * This is to be used to perform a list of resets for a device or power domain
+ * in whatever order. Returns a struct reset_control or IS_ERR() condition
+ * containing errno.
  */
-struct reset_control *of_reset_control_get(struct device_node *node,
-					   const char *id)
+struct reset_control *of_reset_control_get_by_index(struct device_node *node,
+					   int index)
 {
 	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
 	struct reset_controller_dev *r, *rcdev;
 	struct of_phandle_args args;
-	int index = 0;
 	int rstc_id;
 	int ret;
 
-	if (id)
-		index = of_property_match_string(node,
-						 "reset-names", id);
 	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
 					 index, &args);
 	if (ret)
@@ -202,6 +199,27 @@ struct reset_control *of_reset_control_get(struct device_node *node,
 
 	return rstc;
 }
+EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
+
+/**
+ * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @node: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ *
+ * Use of id names is optional.
+ */
+struct reset_control *of_reset_control_get(struct device_node *node,
+					   const char *id)
+{
+	int index = 0;
+
+	if (id)
+		index = of_property_match_string(node,
+						 "reset-names", id);
+	return of_reset_control_get_by_index(node, index);
+}
 EXPORT_SYMBOL_GPL(of_reset_control_get);
 
 /**
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 7f65f9cff951..6db74ad3dec7 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -38,6 +38,9 @@ static inline struct reset_control *devm_reset_control_get_optional(
 struct reset_control *of_reset_control_get(struct device_node *node,
 					   const char *id);
 
+struct reset_control *of_reset_control_get_by_index(
+					struct device_node *node, int index);
+
 #else
 
 static inline int reset_control_reset(struct reset_control *rstc)
@@ -106,6 +109,12 @@ static inline struct reset_control *of_reset_control_get(
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct reset_control *of_reset_control_get_by_index(
+				struct device_node *node, int index)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */
 
 #endif
-- 
2.1.4

[PATCH V4 02/16] soc: tegra: pmc: Add missing structure members to kernel-doc

[Jon Hunter]

Some members of the tegra_pmc structure are missing from the kernel-doc
comment for this structure. Add the missing members.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index bc34cf7482fb..99ca91f09929 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -113,8 +113,10 @@ struct tegra_pmc_soc {
 
 /**
  * struct tegra_pmc - NVIDIA Tegra PMC
+ * @dev: pointer to PMC device structure
  * @base: pointer to I/O remapped register region
  * @clk: pointer to pclk clock
+ * @soc: pointer to SoC data structure
  * @rate: currently configured rate of pclk
  * @suspend_mode: lowest suspend mode available
  * @cpu_good_time: CPU power good time (in microseconds)
-- 
2.1.4

Re: [PATCH V4 02/16] soc: tegra: pmc: Add missing structure members to kernel-doc

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Fri, Dec 04, 2015 at 02:57:03PM +0000, Jon Hunter wrote:
> Some members of the tegra_pmc structure are missing from the kernel-doc
> comment for this structure. Add the missing members.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH V4 03/16] soc: tegra: pmc: Fix sparse warning for tegra_pmc_init_tsense_reset

[Jon Hunter]

Sparse reports the following warning for tegra_pmc_init_tsense_reset:

drivers/soc/tegra/pmc.c:741:6: warning: symbol
'tegra_pmc_init_tsense_reset' was not declared. Should it be static?

This function is only used internally by the PMC driver and so fix this
by making it static.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 99ca91f09929..5b6125ecd69a 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -729,7 +729,7 @@ static void tegra_pmc_init(struct tegra_pmc *pmc)
 	tegra_pmc_writel(value, PMC_CNTRL);
 }
 
-void tegra_pmc_init_tsense_reset(struct tegra_pmc *pmc)
+static void tegra_pmc_init_tsense_reset(struct tegra_pmc *pmc)
 {
 	static const char disabled[] = "emergency thermal reset disabled";
 	u32 pmu_addr, ctrl_id, reg_addr, reg_data, pinmux;
-- 
2.1.4

Re: [PATCH V4 03/16] soc: tegra: pmc: Fix sparse warning for tegra_pmc_init_tsense_reset

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Fri, Dec 04, 2015 at 02:57:04PM +0000, Jon Hunter wrote:
> Sparse reports the following warning for tegra_pmc_init_tsense_reset:
> 
> drivers/soc/tegra/pmc.c:741:6: warning: symbol
> 'tegra_pmc_init_tsense_reset' was not declared. Should it be static?
> 
> This function is only used internally by the PMC driver and so fix this
> by making it static.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH V4 04/16] soc: tegra: pmc: Remove debugfs entry on probe failure

[Jon Hunter]

The debugfs entry for the PMC device will not be removed if the probe of
the device fails on registering the restart handler and so fix this so
that it is removed.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 5b6125ecd69a..e60fc2d33c94 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -117,6 +117,7 @@ struct tegra_pmc_soc {
  * @base: pointer to I/O remapped register region
  * @clk: pointer to pclk clock
  * @soc: pointer to SoC data structure
+ * @debugfs: pointer to debugfs entry
  * @rate: currently configured rate of pclk
  * @suspend_mode: lowest suspend mode available
  * @cpu_good_time: CPU power good time (in microseconds)
@@ -136,6 +137,7 @@ struct tegra_pmc {
 	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
+	struct dentry *debugfs;
 
 	const struct tegra_pmc_soc *soc;
 
@@ -447,11 +449,9 @@ static const struct file_operations powergate_fops = {
 
 static int tegra_powergate_debugfs_init(void)
 {
-	struct dentry *d;
-
-	d = debugfs_create_file("powergate", S_IRUGO, NULL, NULL,
-				&powergate_fops);
-	if (!d)
+	pmc->debugfs = debugfs_create_file("powergate", S_IRUGO, NULL, NULL,
+					   &powergate_fops);
+	if (!pmc->debugfs)
 		return -ENOMEM;
 
 	return 0;
@@ -844,6 +844,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	err = register_restart_handler(&tegra_pmc_restart_handler);
 	if (err) {
+		debugfs_remove(pmc->debugfs);
 		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
 			err);
 		return err;
-- 
2.1.4

Re: [PATCH V4 04/16] soc: tegra: pmc: Remove debugfs entry on probe failure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Fri, Dec 04, 2015 at 02:57:05PM +0000, Jon Hunter wrote:
> The debugfs entry for the PMC device will not be removed if the probe of
> the device fails on registering the restart handler and so fix this so
> that it is removed.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Applied, with a slightly rewritten commit message. Thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers

[Jon Hunter]

During early initialisation, the PMC registers are mapped and the PMC SoC
data is populated in the PMC data structure. This allows other drivers
access the PMC register space, via the public tegra PMC APIs, prior to
probing the PMC device.

When the PMC device is probed, the PMC registers are mapped again and if
successful the initial mapping is freed. If the probing of the PMC device
fails after the registers are remapped, then the registers will be
unmapped and hence the pointer to the PMC registers will be invalid. This
could lead to a potential crash, because once the PMC SoC data pointer is
populated, the driver assumes that the PMC register mapping is also valid
and a user calling any of the public tegra PMC APIs could trigger an
exception because these APIs don't check that the mapping is still valid.

Rather than adding a test to see if the PMC register mapping is valid,
fix this by removing the second mapping of the PMC registers and reserve
the memory region for the PMC registers during early initialisation where
the initial mapping is created. During the probing of the PMC simply check
that the PMC registers have been mapped.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e60fc2d33c94..fdd1a8d0940f 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -807,22 +807,17 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base = pmc->base;
-	struct resource *res;
 	int err;
 
+	if (!pmc->base) {
+		dev_err(&pdev->dev, "base address is not configured\n");
+		return -ENXIO;
+	}
+
 	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
 	if (err < 0)
 		return err;
 
-	/* take over the memory region from the early initialization */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pmc->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
-
-	iounmap(base);
-
 	pmc->clk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(pmc->clk)) {
 		err = PTR_ERR(pmc->clk);
@@ -1126,8 +1121,12 @@ static int __init tegra_pmc_early_init(void)
 		pmc->soc = match->data;
 	}
 
+	if (!request_mem_region(regs.start, resource_size(&regs), regs.name))
+		return -ENOMEM;
+
 	pmc->base = ioremap_nocache(regs.start, resource_size(&regs));
 	if (!pmc->base) {
+		release_mem_region(regs.start, resource_size(&regs));
 		pr_err("failed to map PMC registers\n");
 		return -ENXIO;
 	}
-- 
2.1.4

Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote:
> During early initialisation, the PMC registers are mapped and the PMC SoC
> data is populated in the PMC data structure. This allows other drivers
> access the PMC register space, via the public tegra PMC APIs, prior to
> probing the PMC device.
> 
> When the PMC device is probed, the PMC registers are mapped again and if
> successful the initial mapping is freed. If the probing of the PMC device
> fails after the registers are remapped, then the registers will be
> unmapped and hence the pointer to the PMC registers will be invalid. This
> could lead to a potential crash, because once the PMC SoC data pointer is
> populated, the driver assumes that the PMC register mapping is also valid
> and a user calling any of the public tegra PMC APIs could trigger an
> exception because these APIs don't check that the mapping is still valid.
> 
> Rather than adding a test to see if the PMC register mapping is valid,
> fix this by removing the second mapping of the PMC registers and reserve
> the memory region for the PMC registers during early initialisation where
> the initial mapping is created. During the probing of the PMC simply check
> that the PMC registers have been mapped.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

devm_ioremap_resource() was used on purpose to make sure we get a proper
name assigned to the memory region in /proc/iomem. As it is, there will
be no name associated with it, which I think is unfortunate. As such I'd
prefer keeping that call and instead fix the issue with the invalid
mapping by making sure that pmc->base is assigned only after nothing can
fail in probe anymore.

> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e60fc2d33c94..fdd1a8d0940f 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -807,22 +807,17 @@ out:
>  
>  static int tegra_pmc_probe(struct platform_device *pdev)
>  {
> -	void __iomem *base = pmc->base;

The alternative that I proposed above would entail not setting this...

> -	struct resource *res;
>  	int err;
>  
> +	if (!pmc->base) {
> +		dev_err(&pdev->dev, "base address is not configured\n");
> +		return -ENXIO;
> +	}
> +
>  	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
>  	if (err < 0)
>  		return err;
>  
> -	/* take over the memory region from the early initialization */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	pmc->base = devm_ioremap_resource(&pdev->dev, res);

... and storing the result of the mapping in "base" instead...

> -	if (IS_ERR(pmc->base))
> -		return PTR_ERR(pmc->base);
> -
> -	iounmap(base);

... and move the unmap to the very end of the probe function, which
would look something like:

	/* take over the memory region from the early initialization */
	iounmap(pmc->base);
	pmc->base = base;

That way the mapping in "base" will automatically be undone upon error
and the pmc->base will only be overridden when it's certain that the
probe will succeed.

What do you think?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers

[Jon Hunter]

On 14/01/16 13:45, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote:
>> During early initialisation, the PMC registers are mapped and the PMC SoC
>> data is populated in the PMC data structure. This allows other drivers
>> access the PMC register space, via the public tegra PMC APIs, prior to
>> probing the PMC device.
>>
>> When the PMC device is probed, the PMC registers are mapped again and if
>> successful the initial mapping is freed. If the probing of the PMC device
>> fails after the registers are remapped, then the registers will be
>> unmapped and hence the pointer to the PMC registers will be invalid. This
>> could lead to a potential crash, because once the PMC SoC data pointer is
>> populated, the driver assumes that the PMC register mapping is also valid
>> and a user calling any of the public tegra PMC APIs could trigger an
>> exception because these APIs don't check that the mapping is still valid.
>>
>> Rather than adding a test to see if the PMC register mapping is valid,
>> fix this by removing the second mapping of the PMC registers and reserve
>> the memory region for the PMC registers during early initialisation where
>> the initial mapping is created. During the probing of the PMC simply check
>> that the PMC registers have been mapped.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> devm_ioremap_resource() was used on purpose to make sure we get a proper
> name assigned to the memory region in /proc/iomem. As it is, there will
> be no name associated with it, which I think is unfortunate. As such I'd
> prefer keeping that call and instead fix the issue with the invalid
> mapping by making sure that pmc->base is assigned only after nothing can
> fail in probe anymore.

Yes, however, you still get a valid name in /proc/iomem with this
change. I made sure I tested that ...

/ # cat /proc/iomem
6000d000-6000dfff : /gpio@0,6000d000
60020000-600213ff : /dma@0,60020000
700008d4-70000b6f : /pinmux@0,700008d4
70003000-70003293 : /pinmux@0,700008d4
70006000-7000603f : serial
7000d100-7000d1ff : /i2c@0,7000d100
7000e400-7000e7ff : /pmc@0,7000e400
...

The only expection might be the non-DT case, but I am not sure we care
about that for most boards? Hmm ... I wonder if I need to set
"regs.name" for the non-DT case? I probably should ...

>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index e60fc2d33c94..fdd1a8d0940f 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -807,22 +807,17 @@ out:
>>  
>>  static int tegra_pmc_probe(struct platform_device *pdev)
>>  {
>> -	void __iomem *base = pmc->base;
> 
> The alternative that I proposed above would entail not setting this...
> 
>> -	struct resource *res;
>>  	int err;
>>  
>> +	if (!pmc->base) {
>> +		dev_err(&pdev->dev, "base address is not configured\n");
>> +		return -ENXIO;
>> +	}
>> +
>>  	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
>>  	if (err < 0)
>>  		return err;
>>  
>> -	/* take over the memory region from the early initialization */
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	pmc->base = devm_ioremap_resource(&pdev->dev, res);
> 
> ... and storing the result of the mapping in "base" instead...
> 
>> -	if (IS_ERR(pmc->base))
>> -		return PTR_ERR(pmc->base);
>> -
>> -	iounmap(base);
> 
> ... and move the unmap to the very end of the probe function, which
> would look something like:
> 
> 	/* take over the memory region from the early initialization */
> 	iounmap(pmc->base);
> 	pmc->base = base;
> 
> That way the mapping in "base" will automatically be undone upon error
> and the pmc->base will only be overridden when it's certain that the
> probe will succeed.
> 
> What do you think?

I thought about that, but it still seems racy. You probably want to
assign the new base before freeing the old and so you would need another
tmp variable. Even so, I was not sure if there could be a race here.
Ideally, you would lock, but then you need to lock everywhere that you
use base. Given that my patch still provides a /proc/iomem entry with a
valid name, it seems best to me.

Cheers
Jon

Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6728 bytes --]

On Thu, Jan 14, 2016 at 04:35:28PM +0000, Jon Hunter wrote:
> 
> On 14/01/16 13:45, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote:
> >> During early initialisation, the PMC registers are mapped and the PMC SoC
> >> data is populated in the PMC data structure. This allows other drivers
> >> access the PMC register space, via the public tegra PMC APIs, prior to
> >> probing the PMC device.
> >>
> >> When the PMC device is probed, the PMC registers are mapped again and if
> >> successful the initial mapping is freed. If the probing of the PMC device
> >> fails after the registers are remapped, then the registers will be
> >> unmapped and hence the pointer to the PMC registers will be invalid. This
> >> could lead to a potential crash, because once the PMC SoC data pointer is
> >> populated, the driver assumes that the PMC register mapping is also valid
> >> and a user calling any of the public tegra PMC APIs could trigger an
> >> exception because these APIs don't check that the mapping is still valid.
> >>
> >> Rather than adding a test to see if the PMC register mapping is valid,
> >> fix this by removing the second mapping of the PMC registers and reserve
> >> the memory region for the PMC registers during early initialisation where
> >> the initial mapping is created. During the probing of the PMC simply check
> >> that the PMC registers have been mapped.
> >>
> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >> ---
> >>  drivers/soc/tegra/pmc.c | 19 +++++++++----------
> >>  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > devm_ioremap_resource() was used on purpose to make sure we get a proper
> > name assigned to the memory region in /proc/iomem. As it is, there will
> > be no name associated with it, which I think is unfortunate. As such I'd
> > prefer keeping that call and instead fix the issue with the invalid
> > mapping by making sure that pmc->base is assigned only after nothing can
> > fail in probe anymore.
> 
> Yes, however, you still get a valid name in /proc/iomem with this
> change. I made sure I tested that ...
> 
> / # cat /proc/iomem
> 6000d000-6000dfff : /gpio@0,6000d000
> 60020000-600213ff : /dma@0,60020000
> 700008d4-70000b6f : /pinmux@0,700008d4
> 70003000-70003293 : /pinmux@0,700008d4
> 70006000-7000603f : serial
> 7000d100-7000d1ff : /i2c@0,7000d100
> 7000e400-7000e7ff : /pmc@0,7000e400
> ...
> 
> The only expection might be the non-DT case, but I am not sure we care
> about that for most boards? Hmm ... I wonder if I need to set
> "regs.name" for the non-DT case? I probably should ...

ARCH_TEGRA has depended on OF for a very long time, so I don't think we
care about the non-DT case.

> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> >> index e60fc2d33c94..fdd1a8d0940f 100644
> >> --- a/drivers/soc/tegra/pmc.c
> >> +++ b/drivers/soc/tegra/pmc.c
> >> @@ -807,22 +807,17 @@ out:
> >>  
> >>  static int tegra_pmc_probe(struct platform_device *pdev)
> >>  {
> >> -	void __iomem *base = pmc->base;
> > 
> > The alternative that I proposed above would entail not setting this...
> > 
> >> -	struct resource *res;
> >>  	int err;
> >>  
> >> +	if (!pmc->base) {
> >> +		dev_err(&pdev->dev, "base address is not configured\n");
> >> +		return -ENXIO;
> >> +	}
> >> +
> >>  	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
> >>  	if (err < 0)
> >>  		return err;
> >>  
> >> -	/* take over the memory region from the early initialization */
> >> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> -	pmc->base = devm_ioremap_resource(&pdev->dev, res);
> > 
> > ... and storing the result of the mapping in "base" instead...
> > 
> >> -	if (IS_ERR(pmc->base))
> >> -		return PTR_ERR(pmc->base);
> >> -
> >> -	iounmap(base);
> > 
> > ... and move the unmap to the very end of the probe function, which
> > would look something like:
> > 
> > 	/* take over the memory region from the early initialization */
> > 	iounmap(pmc->base);
> > 	pmc->base = base;
> > 
> > That way the mapping in "base" will automatically be undone upon error
> > and the pmc->base will only be overridden when it's certain that the
> > probe will succeed.
> > 
> > What do you think?
> 
> I thought about that, but it still seems racy. You probably want to
> assign the new base before freeing the old and so you would need another
> tmp variable.

Ah yes, of course. You could still do it with just the two pointers if
you keep the code as-is and revert back to the backed up value in case
of errors. Along this line:

	base = pmc->base;

	pmc->base = devm_ioremap_resource(&pdev->dev, res);

	/* on success */
	iounmap(base);

	/* on error */
	pmc->base = base;

These pointer assignments should be atomic, so no potential for races
there. I've attached a patch which should do the trick, though I have
not tested it.

>               Even so, I was not sure if there could be a race here.
> Ideally, you would lock, but then you need to lock everywhere that you
> use base. Given that my patch still provides a /proc/iomem entry with a
> valid name, it seems best to me.

The primary reason for the "takeover" is that except for the extra
iounmap() and pointer swapping the tegra_pmc_probe() function is really
a standard driver implementation. The idea behind this had always been
that it should be possible to easily convert this to a proper driver if
either we ended up with PSCI exclusively for SMP or defer SMP setup
until the PMC driver had been probed, so that we could get rid of the
early_initcall().

Thierry

--- >8 ---
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index d53030ac56f0..f2f6ddfd9399 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -925,7 +925,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (IS_ERR(pmc->clk)) {
 		err = PTR_ERR(pmc->clk);
 		dev_err(&pdev->dev, "failed to get pclk: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	pmc->dev = &pdev->dev;
@@ -937,17 +937,23 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			return err;
+			goto error;
 	}
 
 	err = register_restart_handler(&tegra_pmc_restart_handler);
 	if (err) {
 		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
 			err);
-		return err;
+		goto error;
 	}
 
+	iounmap(base);
+
 	return 0;
+
+error:
+	pmc->base = base;
+	return err;
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers

[Jon Hunter]

On 14/01/16 17:24, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jan 14, 2016 at 04:35:28PM +0000, Jon Hunter wrote:
>>
>> On 14/01/16 13:45, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote:
>>>> During early initialisation, the PMC registers are mapped and the PMC SoC
>>>> data is populated in the PMC data structure. This allows other drivers
>>>> access the PMC register space, via the public tegra PMC APIs, prior to
>>>> probing the PMC device.
>>>>
>>>> When the PMC device is probed, the PMC registers are mapped again and if
>>>> successful the initial mapping is freed. If the probing of the PMC device
>>>> fails after the registers are remapped, then the registers will be
>>>> unmapped and hence the pointer to the PMC registers will be invalid. This
>>>> could lead to a potential crash, because once the PMC SoC data pointer is
>>>> populated, the driver assumes that the PMC register mapping is also valid
>>>> and a user calling any of the public tegra PMC APIs could trigger an
>>>> exception because these APIs don't check that the mapping is still valid.
>>>>
>>>> Rather than adding a test to see if the PMC register mapping is valid,
>>>> fix this by removing the second mapping of the PMC registers and reserve
>>>> the memory region for the PMC registers during early initialisation where
>>>> the initial mapping is created. During the probing of the PMC simply check
>>>> that the PMC registers have been mapped.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/soc/tegra/pmc.c | 19 +++++++++----------
>>>>  1 file changed, 9 insertions(+), 10 deletions(-)

[snip]

> Ah yes, of course. You could still do it with just the two pointers if
> you keep the code as-is and revert back to the backed up value in case
> of errors. Along this line:
> 
> 	base = pmc->base;
> 
> 	pmc->base = devm_ioremap_resource(&pdev->dev, res);
> 
> 	/* on success */
> 	iounmap(base);
> 
> 	/* on error */
> 	pmc->base = base;
> 
> These pointer assignments should be atomic, so no potential for races
> there. I've attached a patch which should do the trick, though I have
> not tested it.

Right, but I am concerned about someone calling tegra_powergate_set()
(which with patch 6 of this series) will poll for the state to change. I
am not sure we can guarantee the pointer does not change while this is
happening.

>>               Even so, I was not sure if there could be a race here.
>> Ideally, you would lock, but then you need to lock everywhere that you
>> use base. Given that my patch still provides a /proc/iomem entry with a
>> valid name, it seems best to me.
> 
> The primary reason for the "takeover" is that except for the extra
> iounmap() and pointer swapping the tegra_pmc_probe() function is really
> a standard driver implementation. The idea behind this had always been
> that it should be possible to easily convert this to a proper driver if
> either we ended up with PSCI exclusively for SMP or defer SMP setup
> until the PMC driver had been probed, so that we could get rid of the
> early_initcall().

I agree it is cleaner, however, I am still concerned there could still
be a race.

Jon

[PATCH V4 06/16] soc: tegra: pmc: Wait for powergate state to change

[Jon Hunter]

Currently, the function tegra_powergate_set() simply sets the desired
powergate state but does not wait for the state to change. In most cases
we should wait for the state to change before proceeding. Currently, there
is a case for tegra114 and tegra124 devices where we do not wait when
starting the secondary CPU as this is not necessary. However, this is only
done at boot time and so waiting here will only have a small impact on boot
time. Therefore, update tegra_powergate_set() to wait when setting the
powergate.

By adding this feature, we can also eliminate the polling loop from
tegra30_boot_secondary().

A macro has also been adding for checking the status of the powergate and
so update the tegra_powergate_is_powered() to use this macro as well.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       | 21 +++++++++++++++------
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index b45086666648..40cb761e7c95 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * be un-gated by un-toggling the power gate register
 	 * manually.
 	 */
-	if (!tegra_pmc_cpu_is_powered(cpu)) {
-		ret = tegra_pmc_cpu_power_on(cpu);
-		if (ret)
-			return ret;
-
-		/* Wait for the power to come up. */
-		timeout = jiffies + msecs_to_jiffies(100);
-		while (!tegra_pmc_cpu_is_powered(cpu)) {
-			if (time_after(jiffies, timeout))
-				return -ETIMEDOUT;
-			udelay(10);
-		}
-	}
+	ret = tegra_pmc_cpu_power_on(cpu);
+	if (ret)
+		return ret;
 
 remove_clamps:
 	/* CPU partition is powered. Enable the CPU clock. */
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index fdd1a8d0940f..f94d970089ce 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -28,6 +28,7 @@
 #include <linux/export.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
@@ -101,6 +102,8 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+#define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  */
 static int tegra_powergate_set(int id, bool new_state)
 {
-	bool status;
+	u32 status;
+	int err = 0;
 
 	mutex_lock(&pmc->powergates_lock);
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	status = tegra_pmc_readl(PWRGATE_STATUS);
 
-	if (status == new_state) {
+	if (PMC_PWRGATE_STATE(status, id) == new_state) {
 		mutex_unlock(&pmc->powergates_lock);
 		return 0;
 	}
 
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
+	err = readx_poll_timeout(tegra_pmc_readl, PWRGATE_STATUS, status,
+				 PMC_PWRGATE_STATE(status, id) == new_state,
+				 10, 100000);
+
 	mutex_unlock(&pmc->powergates_lock);
 
-	return 0;
+	return err;
 }
 
 /**
@@ -235,8 +243,9 @@ int tegra_powergate_is_powered(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
-	return !!status;
+	status = tegra_pmc_readl(PWRGATE_STATUS);
+
+	return PMC_PWRGATE_STATE(status, id);
 }
 
 /**
-- 
2.1.4

Re: [PATCH V4 06/16] soc: tegra: pmc: Wait for powergate state to change

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3341 bytes --]

On Fri, Dec 04, 2015 at 02:57:07PM +0000, Jon Hunter wrote:
> Currently, the function tegra_powergate_set() simply sets the desired
> powergate state but does not wait for the state to change. In most cases
> we should wait for the state to change before proceeding. Currently, there
> is a case for tegra114 and tegra124 devices where we do not wait when
> starting the secondary CPU as this is not necessary. However, this is only
> done at boot time and so waiting here will only have a small impact on boot
> time. Therefore, update tegra_powergate_set() to wait when setting the
> powergate.
> 
> By adding this feature, we can also eliminate the polling loop from
> tegra30_boot_secondary().
> 
> A macro has also been adding for checking the status of the powergate and

"added"

> so update the tegra_powergate_is_powered() to use this macro as well.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>  drivers/soc/tegra/pmc.c       | 21 +++++++++++++++------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index b45086666648..40cb761e7c95 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	 * be un-gated by un-toggling the power gate register
>  	 * manually.
>  	 */
> -	if (!tegra_pmc_cpu_is_powered(cpu)) {
> -		ret = tegra_pmc_cpu_power_on(cpu);
> -		if (ret)
> -			return ret;
> -
> -		/* Wait for the power to come up. */
> -		timeout = jiffies + msecs_to_jiffies(100);
> -		while (!tegra_pmc_cpu_is_powered(cpu)) {
> -			if (time_after(jiffies, timeout))
> -				return -ETIMEDOUT;
> -			udelay(10);
> -		}
> -	}
> +	ret = tegra_pmc_cpu_power_on(cpu);
> +	if (ret)
> +		return ret;
>  
>  remove_clamps:
>  	/* CPU partition is powered. Enable the CPU clock. */

I vaguely remember this being very brittle. I assume you've tested this
extensively and made sure it doesn't regress?

> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index fdd1a8d0940f..f94d970089ce 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -28,6 +28,7 @@
>  #include <linux/export.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> @@ -101,6 +102,8 @@
>  
>  #define GPU_RG_CNTRL			0x2d4
>  
> +#define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
> +

This looks suspiciously like a register or register field definition.
Maybe turn this into a static inline function, such as:

	static inline bool tegra_powergate_state(u32 status, int id)
	{
		return (status & BIT(id)) != 0;
	}

?

>  struct tegra_pmc_soc {
>  	unsigned int num_powergates;
>  	const char *const *powergates;
> @@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>   */
>  static int tegra_powergate_set(int id, bool new_state)
>  {
> -	bool status;
> +	u32 status;
> +	int err = 0;

Initialization to 0 doesn't seem necessary here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 06/16] soc: tegra: pmc: Wait for powergate state to change

[Jon Hunter]

On 14/01/16 14:01, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:07PM +0000, Jon Hunter wrote:
>> Currently, the function tegra_powergate_set() simply sets the desired
>> powergate state but does not wait for the state to change. In most cases
>> we should wait for the state to change before proceeding. Currently, there
>> is a case for tegra114 and tegra124 devices where we do not wait when
>> starting the secondary CPU as this is not necessary. However, this is only
>> done at boot time and so waiting here will only have a small impact on boot
>> time. Therefore, update tegra_powergate_set() to wait when setting the
>> powergate.
>>
>> By adding this feature, we can also eliminate the polling loop from
>> tegra30_boot_secondary().
>>
>> A macro has also been adding for checking the status of the powergate and
> 
> "added"

Ok, thanks.

>> so update the tegra_powergate_is_powered() to use this macro as well.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>>  drivers/soc/tegra/pmc.c       | 21 +++++++++++++++------
>>  2 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
>> index b45086666648..40cb761e7c95 100644
>> --- a/arch/arm/mach-tegra/platsmp.c
>> +++ b/arch/arm/mach-tegra/platsmp.c
>> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>  	 * be un-gated by un-toggling the power gate register
>>  	 * manually.
>>  	 */
>> -	if (!tegra_pmc_cpu_is_powered(cpu)) {
>> -		ret = tegra_pmc_cpu_power_on(cpu);
>> -		if (ret)
>> -			return ret;
>> -
>> -		/* Wait for the power to come up. */
>> -		timeout = jiffies + msecs_to_jiffies(100);
>> -		while (!tegra_pmc_cpu_is_powered(cpu)) {
>> -			if (time_after(jiffies, timeout))
>> -				return -ETIMEDOUT;
>> -			udelay(10);
>> -		}
>> -	}
>> +	ret = tegra_pmc_cpu_power_on(cpu);
>> +	if (ret)
>> +		return ret;
>>  
>>  remove_clamps:
>>  	/* CPU partition is powered. Enable the CPU clock. */
> 
> I vaguely remember this being very brittle. I assume you've tested this
> extensively and made sure it doesn't regress?

I have boot tested several times, but if you are concerned about this
secondary boot failing specifically, I could setup a test over the
weekend to boot test say 1000 iterations for all boards.

>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index fdd1a8d0940f..f94d970089ce 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/export.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>> @@ -101,6 +102,8 @@
>>  
>>  #define GPU_RG_CNTRL			0x2d4
>>  
>> +#define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
>> +
> 
> This looks suspiciously like a register or register field definition.
> Maybe turn this into a static inline function, such as:
> 
> 	static inline bool tegra_powergate_state(u32 status, int id)
> 	{
> 		return (status & BIT(id)) != 0;
> 	}
> 
> ?

Ok.

>>  struct tegra_pmc_soc {
>>  	unsigned int num_powergates;
>>  	const char *const *powergates;
>> @@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>>   */
>>  static int tegra_powergate_set(int id, bool new_state)
>>  {
>> -	bool status;
>> +	u32 status;
>> +	int err = 0;
> 
> Initialization to 0 doesn't seem necessary here.

Yes, ok.

Cheers
Jon

[PATCH V4 07/16] soc: tegra: pmc: Remove non-existing power partitions for T210

[Jon Hunter]

The power partitions L2, HEG, CELP and C1NC do not exist for T210 but were
incorrectly documented in the TRM. These will be removed from the TRM and
so also remove their definitions for T210.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index f94d970089ce..28d3106d3add 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1013,17 +1013,13 @@ static const char * const tegra210_powergates[] = {
 	[TEGRA_POWERGATE_3D] = "3d",
 	[TEGRA_POWERGATE_VENC] = "venc",
 	[TEGRA_POWERGATE_PCIE] = "pcie",
-	[TEGRA_POWERGATE_L2] = "l2",
 	[TEGRA_POWERGATE_MPE] = "mpe",
-	[TEGRA_POWERGATE_HEG] = "heg",
 	[TEGRA_POWERGATE_SATA] = "sata",
 	[TEGRA_POWERGATE_CPU1] = "cpu1",
 	[TEGRA_POWERGATE_CPU2] = "cpu2",
 	[TEGRA_POWERGATE_CPU3] = "cpu3",
-	[TEGRA_POWERGATE_CELP] = "celp",
 	[TEGRA_POWERGATE_CPU0] = "cpu0",
 	[TEGRA_POWERGATE_C0NC] = "c0nc",
-	[TEGRA_POWERGATE_C1NC] = "c1nc",
 	[TEGRA_POWERGATE_SOR] = "sor",
 	[TEGRA_POWERGATE_DIS] = "dis",
 	[TEGRA_POWERGATE_DISB] = "disb",
-- 
2.1.4

Re: [PATCH V4 07/16] soc: tegra: pmc: Remove non-existing power partitions for T210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Fri, Dec 04, 2015 at 02:57:08PM +0000, Jon Hunter wrote:
> The power partitions L2, HEG, CELP and C1NC do not exist for T210 but were
> incorrectly documented in the TRM. These will be removed from the TRM and
> so also remove their definitions for T210.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 4 ----
>  1 file changed, 4 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions

[Jon Hunter]

The tegra power partitions are referenced by a numerical ID which are
the same values programmed into the PMC registers for controlling the
partition. For a given device, the valid partition IDs may not be
contiguous and so simply checking that an ID is not greater than the
maximum ID supported may not mean it is valid. Fix this by computing
a bit-mask of the valid partition IDs for a device and add a macro that
will test if the partition is valid based upon this mask.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 28d3106d3add..0967bba13947 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -103,6 +103,7 @@
 #define GPU_RG_CNTRL			0x2d4
 
 #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
+#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))
 
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
@@ -134,6 +135,7 @@ struct tegra_pmc_soc {
  * @cpu_pwr_good_en: CPU power good signal is enabled
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
+ * @powergates_mask: Bit mask of valid power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {
@@ -158,6 +160,7 @@ struct tegra_pmc {
 	bool cpu_pwr_good_en;
 	u32 lp0_vec_phys;
 	u32 lp0_vec_size;
+	u32 powergates_mask;
 
 	struct mutex powergates_lock;
 };
@@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
  */
 int tegra_powergate_power_on(int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, true);
@@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
  */
 int tegra_powergate_power_off(int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);
@@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
 {
 	u32 status;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	status = tegra_pmc_readl(PWRGATE_STATUS);
@@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	/*
@@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
 	struct device_node *np;
 	struct resource regs;
 	bool invert;
-	u32 value;
+	u32 value, i;
 
 	np = of_find_matching_node_and_match(NULL, tegra_pmc_match, &match);
 	if (!np) {
@@ -1136,6 +1139,11 @@ static int __init tegra_pmc_early_init(void)
 		return -ENXIO;
 	}
 
+	/* Create a bit-mask of the valid partitions */
+	for (i = 0; i < pmc->soc->num_powergates; i++)
+		if (pmc->soc->powergates[i])
+			pmc->powergates_mask |= BIT(i);
+
 	mutex_init(&pmc->powergates_lock);
 
 	/*
-- 
2.1.4

Re: [PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3948 bytes --]

On Fri, Dec 04, 2015 at 02:57:09PM +0000, Jon Hunter wrote:
> The tegra power partitions are referenced by a numerical ID which are
> the same values programmed into the PMC registers for controlling the
> partition. For a given device, the valid partition IDs may not be
> contiguous and so simply checking that an ID is not greater than the
> maximum ID supported may not mean it is valid. Fix this by computing
> a bit-mask of the valid partition IDs for a device and add a macro that
> will test if the partition is valid based upon this mask.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 28d3106d3add..0967bba13947 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -103,6 +103,7 @@
>  #define GPU_RG_CNTRL			0x2d4
>  
>  #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
> +#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))

I'd prefer a static inline function here, too. If you do that you will
also need to move the static inline below the struct tegra_pmc to avoid
build errors. Also don't forget to check for id < 0, because it
technically could be. It may also be worth checking for an upper bound
just in case anybody was going to pass in a value other that the ones
defined in include/soc/tegra/pmc.h.

>  struct tegra_pmc_soc {
>  	unsigned int num_powergates;
> @@ -134,6 +135,7 @@ struct tegra_pmc_soc {
>   * @cpu_pwr_good_en: CPU power good signal is enabled
>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>   * @lp0_vec_size: size of the LP0 warm boot code
> + * @powergates_mask: Bit mask of valid power gates
>   * @powergates_lock: mutex for power gate register access
>   */
>  struct tegra_pmc {
> @@ -158,6 +160,7 @@ struct tegra_pmc {
>  	bool cpu_pwr_good_en;
>  	u32 lp0_vec_phys;
>  	u32 lp0_vec_size;
> +	u32 powergates_mask;

This seems rather risky. The highest partition ID that we currently have
is 29, so there is potential that 32 bits will be exceeded in the near
future. It'd be more future-proof to turn this into a bitmap from the
start so that we never have to worry about it.

>  
>  	struct mutex powergates_lock;
>  };
> @@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
>   */
>  int tegra_powergate_power_on(int id)
>  {
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;

In a similar vein I've been meaning, for a long time, to make the ID an
unsigned integer. It might be worth doing that, even with this patch
applied, but it can be a separate patch.

>  	return tegra_powergate_set(id, true);
> @@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
>   */
>  int tegra_powergate_power_off(int id)
>  {
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	return tegra_powergate_set(id, false);
> @@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
>  {
>  	u32 status;
>  
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	status = tegra_pmc_readl(PWRGATE_STATUS);
> @@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
>  {
>  	u32 mask;
>  
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	/*
> @@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
>  	struct device_node *np;
>  	struct resource regs;
>  	bool invert;
> -	u32 value;
> +	u32 value, i;

I'd prefer an unsized type (unsigned int) for i because there is no
reason why it should be sized.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions

[Jon Hunter]

On 14/01/16 14:11, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:09PM +0000, Jon Hunter wrote:
>> The tegra power partitions are referenced by a numerical ID which are
>> the same values programmed into the PMC registers for controlling the
>> partition. For a given device, the valid partition IDs may not be
>> contiguous and so simply checking that an ID is not greater than the
>> maximum ID supported may not mean it is valid. Fix this by computing
>> a bit-mask of the valid partition IDs for a device and add a macro that
>> will test if the partition is valid based upon this mask.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 28d3106d3add..0967bba13947 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -103,6 +103,7 @@
>>  #define GPU_RG_CNTRL			0x2d4
>>  
>>  #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
>> +#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))
> 
> I'd prefer a static inline function here, too. If you do that you will
> also need to move the static inline below the struct tegra_pmc to avoid
> build errors. Also don't forget to check for id < 0, because it
> technically could be. It may also be worth checking for an upper bound
> just in case anybody was going to pass in a value other that the ones
> defined in include/soc/tegra/pmc.h.

Ok.

>>  struct tegra_pmc_soc {
>>  	unsigned int num_powergates;
>> @@ -134,6 +135,7 @@ struct tegra_pmc_soc {
>>   * @cpu_pwr_good_en: CPU power good signal is enabled
>>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>>   * @lp0_vec_size: size of the LP0 warm boot code
>> + * @powergates_mask: Bit mask of valid power gates
>>   * @powergates_lock: mutex for power gate register access
>>   */
>>  struct tegra_pmc {
>> @@ -158,6 +160,7 @@ struct tegra_pmc {
>>  	bool cpu_pwr_good_en;
>>  	u32 lp0_vec_phys;
>>  	u32 lp0_vec_size;
>> +	u32 powergates_mask;
> 
> This seems rather risky. The highest partition ID that we currently have
> is 29, so there is potential that 32 bits will be exceeded in the near
> future. It'd be more future-proof to turn this into a bitmap from the
> start so that we never have to worry about it.

Yes, I had thought about that, but figured we could handle later. Ok, I
will look at this now.

>>  
>>  	struct mutex powergates_lock;
>>  };
>> @@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
>>   */
>>  int tegra_powergate_power_on(int id)
>>  {
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
> 
> In a similar vein I've been meaning, for a long time, to make the ID an
> unsigned integer. It might be worth doing that, even with this patch
> applied, but it can be a separate patch.
> 
>>  	return tegra_powergate_set(id, true);
>> @@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
>>   */
>>  int tegra_powergate_power_off(int id)
>>  {
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	return tegra_powergate_set(id, false);
>> @@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
>>  {
>>  	u32 status;
>>  
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	status = tegra_pmc_readl(PWRGATE_STATUS);
>> @@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
>>  {
>>  	u32 mask;
>>  
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	/*
>> @@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
>>  	struct device_node *np;
>>  	struct resource regs;
>>  	bool invert;
>> -	u32 value;
>> +	u32 value, i;
> 
> I'd prefer an unsized type (unsigned int) for i because there is no
> reason why it should be sized.

Ok.

Jon

[PATCH V4 09/16] soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC

[Jon Hunter]

For Tegra124 and Tegra210, the GPU partition cannot be toggled on and off
via the APBDEV_PMC_PWRGATE_TOGGLE_0 register. For these devices, the
partition is simply powered up and down via an external regulator.
Describe in the PMC SoC data in which devices the GPU partition can be
controlled via the APBDEV_PMC_PWRGATE_TOGGLE_0 register and ensure that
no one can incorrectly try to toggle the GPU partition via the
APBDEV_PMC_PWRGATE_TOGGLE_0 register.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 0967bba13947..b412098b8197 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -113,6 +113,7 @@ struct tegra_pmc_soc {
 
 	bool has_tsense_reset;
 	bool has_gpu_clamps;
+	bool has_gpu_toggle;
 };
 
 /**
@@ -190,6 +191,9 @@ static int tegra_powergate_set(int id, bool new_state)
 	u32 status;
 	int err = 0;
 
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return -EINVAL;
+
 	mutex_lock(&pmc->powergates_lock);
 
 	status = tegra_pmc_readl(PWRGATE_STATUS);
@@ -246,6 +250,9 @@ int tegra_powergate_is_powered(int id)
 	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return -EINVAL;
+
 	status = tegra_pmc_readl(PWRGATE_STATUS);
 
 	return PMC_PWRGATE_STATE(status, id);
@@ -929,6 +936,7 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = {
 	.cpu_powergates = tegra30_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra114_powergates[] = {
@@ -966,6 +974,7 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = {
 	.cpu_powergates = tegra114_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra124_powergates[] = {
@@ -1009,6 +1018,7 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = {
 	.cpu_powergates = tegra124_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const char * const tegra210_powergates[] = {
@@ -1052,6 +1062,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.cpu_powergates = tegra210_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const struct of_device_id tegra_pmc_match[] = {
-- 
2.1.4

Re: [PATCH V4 09/16] soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Fri, Dec 04, 2015 at 02:57:10PM +0000, Jon Hunter wrote:
> For Tegra124 and Tegra210, the GPU partition cannot be toggled on and off
> via the APBDEV_PMC_PWRGATE_TOGGLE_0 register. For these devices, the
> partition is simply powered up and down via an external regulator.
> Describe in the PMC SoC data in which devices the GPU partition can be
> controlled via the APBDEV_PMC_PWRGATE_TOGGLE_0 register and ensure that
> no one can incorrectly try to toggle the GPU partition via the
> APBDEV_PMC_PWRGATE_TOGGLE_0 register.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

The TRM doesn't mention anything like this. Will this be updated in the
TRM as well?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 09/16] soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC

[Jon Hunter]

On 14/01/16 14:14, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:10PM +0000, Jon Hunter wrote:
>> For Tegra124 and Tegra210, the GPU partition cannot be toggled on and off
>> via the APBDEV_PMC_PWRGATE_TOGGLE_0 register. For these devices, the
>> partition is simply powered up and down via an external regulator.
>> Describe in the PMC SoC data in which devices the GPU partition can be
>> controlled via the APBDEV_PMC_PWRGATE_TOGGLE_0 register and ensure that
>> no one can incorrectly try to toggle the GPU partition via the
>> APBDEV_PMC_PWRGATE_TOGGLE_0 register.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/soc/tegra/pmc.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
> 
> The TRM doesn't mention anything like this. Will this be updated in the
> TRM as well?

For T210, I have requested that the partitions are updated. Looks like
the powergate sequencing information for T210 is still missing from the
TRM. I can request that this is added. I will make a note for T124 as
well as it is missing.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 10/16] PM / Domains: Add function to remove a pm-domain

[Jon Hunter]

The genpd framework allows users to add power-domains via the
pm_genpd_init() function, however, there is no corresponding function
to remove a power-domain. For most devices this may be fine as the power
domains are never removed, however, for devices that wish to populate
the power-domains from within a driver, having the ability to remove a
power domain if the probing of the device fails or the driver is unloaded
is necessary. Therefore, add a function to remove a power-domain. Please
note that the power domain can only be removed if there are no devices
using the power-domain and it is not linked to another domain.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e03b1ad25a90..137f29cde8a8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1509,6 +1509,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+/**
+ * pm_genpd_remove - Remove a generic I/O PM domain object.
+ * @genpd: PM domain object to remove.
+ */
+int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	if (IS_ERR_OR_NULL(genpd))
+		return -EINVAL;
+
+	mutex_lock(&genpd->lock);
+
+	if (!list_empty(&genpd->master_links)
+	    || !list_empty(&genpd->slave_links) || genpd->device_count) {
+		mutex_unlock(&genpd->lock);
+		return -EBUSY;
+	}
+
+	mutex_lock_nested(&gpd_list_lock, SINGLE_DEPTH_NESTING);
+	list_del(&genpd->gpd_list_node);
+	mutex_unlock(&gpd_list_lock);
+	mutex_unlock(&genpd->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_remove);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ba4ced38efae..637699ff5a23 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern void pm_genpd_init(struct generic_pm_domain *genpd,
 			  struct dev_power_governor *gov, bool is_off);
+extern int pm_genpd_remove(struct generic_pm_domain *genpd);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -161,6 +162,10 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
 				 struct dev_power_governor *gov, bool is_off)
 {
 }
+static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.1.4

[PATCH V4 11/16] Documentation: DT: bindings: Update NVIDIA PMC for Tegra210

[Jon Hunter]

---
 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 02c27004d4a8..838e1a69ec0a 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -9,8 +9,9 @@ Required properties:
 - compatible : For Tegra20, must contain "nvidia,tegra20-pmc".  For Tegra30,
   must contain "nvidia,tegra30-pmc".  For Tegra114, must contain
   "nvidia,tegra114-pmc".  For Tegra124, must contain "nvidia,tegra124-pmc".
-  Otherwise, must contain "nvidia,<chip>-pmc", plus at least one of the
-  above, where <chip> is tegra132.
+  For Tegra210, must contain "nvidia,tegra210-pmc". Otherwise, must contain
+  "nvidia,<chip>-pmc", plus at least one of the above, where <chip> is
+  tegra132.
 - reg : Offset and length of the register set for the device
 - clocks : Must contain an entry for each entry in clock-names.
   See ../clocks/clock-bindings.txt for details.
-- 
2.1.4

Re: [PATCH V4 11/16] Documentation: DT: bindings: Update NVIDIA PMC for Tegra210

[Rob Herring]

On Fri, Dec 04, 2015 at 02:57:12PM +0000, Jon Hunter wrote:
> ---
>  Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 02c27004d4a8..838e1a69ec0a 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -9,8 +9,9 @@ Required properties:
>  - compatible : For Tegra20, must contain "nvidia,tegra20-pmc".  For Tegra30,
>    must contain "nvidia,tegra30-pmc".  For Tegra114, must contain
>    "nvidia,tegra114-pmc".  For Tegra124, must contain "nvidia,tegra124-pmc".
> -  Otherwise, must contain "nvidia,<chip>-pmc", plus at least one of the
> -  above, where <chip> is tegra132.
> +  For Tegra210, must contain "nvidia,tegra210-pmc". Otherwise, must contain
> +  "nvidia,<chip>-pmc", plus at least one of the above, where <chip> is
> +  tegra132.

Can you reorg this some so the compatible strings are a list rather than 
a paragraph of text. I find the last sentence a bit confusing as well. 
Seems like <chip> should be a list rather than just tegra132.


>  - reg : Offset and length of the register set for the device
>  - clocks : Must contain an entry for each entry in clock-names.
>    See ../clocks/clock-bindings.txt for details.
> -- 
> 2.1.4
> 

Re: [PATCH V4 11/16] Documentation: DT: bindings: Update NVIDIA PMC for Tegra210

[Jon Hunter]

On 06/12/15 00:31, Rob Herring wrote:
> On Fri, Dec 04, 2015 at 02:57:12PM +0000, Jon Hunter wrote:
>> ---
>>  Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 02c27004d4a8..838e1a69ec0a 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -9,8 +9,9 @@ Required properties:
>>  - compatible : For Tegra20, must contain "nvidia,tegra20-pmc".  For Tegra30,
>>    must contain "nvidia,tegra30-pmc".  For Tegra114, must contain
>>    "nvidia,tegra114-pmc".  For Tegra124, must contain "nvidia,tegra124-pmc".
>> -  Otherwise, must contain "nvidia,<chip>-pmc", plus at least one of the
>> -  above, where <chip> is tegra132.
>> +  For Tegra210, must contain "nvidia,tegra210-pmc". Otherwise, must contain
>> +  "nvidia,<chip>-pmc", plus at least one of the above, where <chip> is
>> +  tegra132.
> 
> Can you reorg this some so the compatible strings are a list rather than 
> a paragraph of text. I find the last sentence a bit confusing as well. 
> Seems like <chip> should be a list rather than just tegra132.

Yes no problem.

Thanks
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

Add power-domain binding documentation for the NVIDIA PMC driver in
order to support generic power-domains.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

---

Please note that I have been debating whether I add this
"nvidia,powergate-clock-disable" property or just leave the clocks
disabled by default. Some downstream kernels leave the clocks enabled
for the audio power-domain because the clocks required for powering up
the power-domain are needed by all modules within the power-domain.
However are the same time there are other power-domains that may need
to be on, but not always clocked and so having the ability to specify if
the clocks should be disabled seems useful. However, I can also remove
this and just have the appropriate devices turn on the clocks as well.
---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 838e1a69ec0a..8e4641db51a9 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -1,5 +1,7 @@
 NVIDIA Tegra Power Management Controller (PMC)
 
+== Power Management Controller Node ==
+
 The PMC block interacts with an external Power Management Unit. The PMC
 mostly controls the entry and exit of the system from different sleep
 modes. It provides power-gating controllers for SoC and CPU power-islands.
@@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
                      Defaults to 0. Valid values are described in section 12.5.2
                      "Pinmux Support" of the Tegra4 Technical Reference Manual.
 
+Optional nodes:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file
@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
 	...
 };
+
+
+== PM Domain Nodes ==
+
+Each of the PM domain nodes represents a power-domain on the Tegra SoC
+that can be power-gated by the PMC and should be named appropriately.
+
+Required properties:
+  - clocks: Must contain an entry for each clock required by the PMC for
+    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
+  - resets: Must contain an entry for each reset required by the PMC for
+    controlling a power-gate. See ../reset/reset.txt for details.
+  - nvidia,powergate: Integer cell that contains an identifier for the PMC
+    power-gate that is associated with the power-domain. Please refer to
+    the Tegra TRM for more details.
+  - #power-domain-cells: Must be 0.
+
+Optional properties:
+  - nvidia,powergate-disable-clocks: Boolean property that if present
+    indicates that the clocks listed in the "clocks" property should be
+    disabled after turning on the power-domain. Otherwise the clocks will
+    be kept enabled.
+
+Example:
+
+	pmc: pmc@0,7000e400 {
+		compatible = "nvidia,tegra210-pmc";
+		reg = <0x0 0x7000e400 0x0 0x400>;
+		clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
+		clock-names = "pclk", "clk32k_in";
+
+		pm-domains {
+			pd_audio: aud {
+				clocks = <&tegra_car TEGRA210_CLK_APE>,
+					 <&tegra_car TEGRA210_CLK_APB2APE>;
+				resets = <&tegra_car 198>;
+				nvidia,powergate = <TEGRA_POWERGATE_AUD>;
+				#power-domain-cells = <0>;
+			};
+		};
+	};
+
+
+== PM Domain Consumers ==
+
+Hardware blocks belonging to a PM domain should contain a "power-domains"
+property that is a phandle pointing to the corresponding PM domain node.
+
+Example:
+
+	adma: adma@702e2000 {
+		...
+		power-domains = <&pd_audio>;
+		...
+	};
-- 
2.1.4

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Rob Herring]

On Fri, Dec 04, 2015 at 02:57:13PM +0000, Jon Hunter wrote:
> Add power-domain binding documentation for the NVIDIA PMC driver in
> order to support generic power-domains.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> 
> Please note that I have been debating whether I add this
> "nvidia,powergate-clock-disable" property or just leave the clocks
> disabled by default. Some downstream kernels leave the clocks enabled
> for the audio power-domain because the clocks required for powering up
> the power-domain are needed by all modules within the power-domain.
> However are the same time there are other power-domains that may need
> to be on, but not always clocked and so having the ability to specify if
> the clocks should be disabled seems useful. However, I can also remove
> this and just have the appropriate devices turn on the clocks as well.

Seems like that is just a failure of drivers to control all the clocks 
they need IMO.

> ---
>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 838e1a69ec0a..8e4641db51a9 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -1,5 +1,7 @@
>  NVIDIA Tegra Power Management Controller (PMC)
>  
> +== Power Management Controller Node ==
> +
>  The PMC block interacts with an external Power Management Unit. The PMC
>  mostly controls the entry and exit of the system from different sleep
>  modes. It provides power-gating controllers for SoC and CPU power-islands.
> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>                       Defaults to 0. Valid values are described in section 12.5.2
>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>  
> +Optional nodes:
> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
> +	       match the power-domains on the Tegra SoC.
> +
>  Example:
>  
>  / SoC dts including file
> @@ -114,3 +120,58 @@ pmc@7000f400 {
>  	};
>  	...
>  };
> +
> +
> +== PM Domain Nodes ==
> +
> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
> +that can be power-gated by the PMC and should be named appropriately.
> +
> +Required properties:
> +  - clocks: Must contain an entry for each clock required by the PMC for
> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
> +  - resets: Must contain an entry for each reset required by the PMC for
> +    controlling a power-gate. See ../reset/reset.txt for details.
> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
> +    power-gate that is associated with the power-domain. Please refer to
> +    the Tegra TRM for more details.

"reg" does not work here?

Rob

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 06/12/15 00:37, Rob Herring wrote:
> On Fri, Dec 04, 2015 at 02:57:13PM +0000, Jon Hunter wrote:
>> Add power-domain binding documentation for the NVIDIA PMC driver in
>> order to support generic power-domains.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> ---
>>
>> Please note that I have been debating whether I add this
>> "nvidia,powergate-clock-disable" property or just leave the clocks
>> disabled by default. Some downstream kernels leave the clocks enabled
>> for the audio power-domain because the clocks required for powering up
>> the power-domain are needed by all modules within the power-domain.
>> However are the same time there are other power-domains that may need
>> to be on, but not always clocked and so having the ability to specify if
>> the clocks should be disabled seems useful. However, I can also remove
>> this and just have the appropriate devices turn on the clocks as well.
> 
> Seems like that is just a failure of drivers to control all the clocks 
> they need IMO.

Right and that was why I was on the fence. However, I could see that
some domains may always need a bus clock on and so it did not seem
complete absurd. If the consensus is to drop it, I will.

>> ---
>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 838e1a69ec0a..8e4641db51a9 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -1,5 +1,7 @@
>>  NVIDIA Tegra Power Management Controller (PMC)
>>  
>> +== Power Management Controller Node ==
>> +
>>  The PMC block interacts with an external Power Management Unit. The PMC
>>  mostly controls the entry and exit of the system from different sleep
>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>                       Defaults to 0. Valid values are described in section 12.5.2
>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>  
>> +Optional nodes:
>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>> +	       match the power-domains on the Tegra SoC.
>> +
>>  Example:
>>  
>>  / SoC dts including file
>> @@ -114,3 +120,58 @@ pmc@7000f400 {
>>  	};
>>  	...
>>  };
>> +
>> +
>> +== PM Domain Nodes ==
>> +
>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>> +that can be power-gated by the PMC and should be named appropriately.
>> +
>> +Required properties:
>> +  - clocks: Must contain an entry for each clock required by the PMC for
>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>> +  - resets: Must contain an entry for each reset required by the PMC for
>> +    controlling a power-gate. See ../reset/reset.txt for details.
>> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
>> +    power-gate that is associated with the power-domain. Please refer to
>> +    the Tegra TRM for more details.
> 
> "reg" does not work here?

Not really. This value is used to program a register to power-up/down
the particular power-domain you are interested in.

Cheers
Jon

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]

On Fri, Dec 04, 2015 at 02:57:13PM +0000, Jon Hunter wrote:
> Add power-domain binding documentation for the NVIDIA PMC driver in
> order to support generic power-domains.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> 
> Please note that I have been debating whether I add this
> "nvidia,powergate-clock-disable" property or just leave the clocks
> disabled by default. Some downstream kernels leave the clocks enabled
> for the audio power-domain because the clocks required for powering up
> the power-domain are needed by all modules within the power-domain.
> However are the same time there are other power-domains that may need
> to be on, but not always clocked and so having the ability to specify if
> the clocks should be disabled seems useful. However, I can also remove
> this and just have the appropriate devices turn on the clocks as well.
> ---
>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 838e1a69ec0a..8e4641db51a9 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -1,5 +1,7 @@
>  NVIDIA Tegra Power Management Controller (PMC)
>  
> +== Power Management Controller Node ==
> +
>  The PMC block interacts with an external Power Management Unit. The PMC
>  mostly controls the entry and exit of the system from different sleep
>  modes. It provides power-gating controllers for SoC and CPU power-islands.
> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>                       Defaults to 0. Valid values are described in section 12.5.2
>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>  
> +Optional nodes:
> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
> +	       match the power-domains on the Tegra SoC.
> +

pm-domains is a linuxism. Perhaps name this after what Tegra calls this:
"powergates"?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 14/01/16 14:41, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:13PM +0000, Jon Hunter wrote:
>> Add power-domain binding documentation for the NVIDIA PMC driver in
>> order to support generic power-domains.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> ---
>>
>> Please note that I have been debating whether I add this
>> "nvidia,powergate-clock-disable" property or just leave the clocks
>> disabled by default. Some downstream kernels leave the clocks enabled
>> for the audio power-domain because the clocks required for powering up
>> the power-domain are needed by all modules within the power-domain.
>> However are the same time there are other power-domains that may need
>> to be on, but not always clocked and so having the ability to specify if
>> the clocks should be disabled seems useful. However, I can also remove
>> this and just have the appropriate devices turn on the clocks as well.
>> ---
>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 838e1a69ec0a..8e4641db51a9 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -1,5 +1,7 @@
>>  NVIDIA Tegra Power Management Controller (PMC)
>>  
>> +== Power Management Controller Node ==
>> +
>>  The PMC block interacts with an external Power Management Unit. The PMC
>>  mostly controls the entry and exit of the system from different sleep
>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>                       Defaults to 0. Valid values are described in section 12.5.2
>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>  
>> +Optional nodes:
>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>> +	       match the power-domains on the Tegra SoC.
>> +
> 
> pm-domains is a linuxism. Perhaps name this after what Tegra calls this:
> "powergates"?

Ok.

Jon

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Kevin Hilman]

Jon Hunter <jonathanh@nvidia.com> writes:

> Add power-domain binding documentation for the NVIDIA PMC driver in
> order to support generic power-domains.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>
> ---
>
> Please note that I have been debating whether I add this
> "nvidia,powergate-clock-disable" property or just leave the clocks
> disabled by default. Some downstream kernels leave the clocks enabled
> for the audio power-domain because the clocks required for powering up
> the power-domain are needed by all modules within the power-domain.
> However are the same time there are other power-domains that may need
> to be on, but not always clocked and so having the ability to specify if
> the clocks should be disabled seems useful. However, I can also remove
> this and just have the appropriate devices turn on the clocks as well.
> ---
>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 838e1a69ec0a..8e4641db51a9 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -1,5 +1,7 @@
>  NVIDIA Tegra Power Management Controller (PMC)
>  
> +== Power Management Controller Node ==
> +
>  The PMC block interacts with an external Power Management Unit. The PMC
>  mostly controls the entry and exit of the system from different sleep
>  modes. It provides power-gating controllers for SoC and CPU power-islands.
> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>                       Defaults to 0. Valid values are described in section 12.5.2
>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>  
> +Optional nodes:
> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
> +	       match the power-domains on the Tegra SoC.
> +
>  Example:
>  
>  / SoC dts including file
> @@ -114,3 +120,58 @@ pmc@7000f400 {
>  	};
>  	...
>  };
> +
> +
> +== PM Domain Nodes ==
> +
> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
> +that can be power-gated by the PMC and should be named appropriately.
> +
> +Required properties:
> +  - clocks: Must contain an entry for each clock required by the PMC for
> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.

We've had this discussiona for a couple of other SoCs, so I need to
ask...

Presumably these are not device clocks that a runtime PM enabled driver
should be managing for a device, right?  IOW, We want to make sure that the
PM domain isn't managing clocks for drivers that should be doing it.

I understand there are legitimate reasons for the PM domain to manage
clocks in addition to device drivers (e.g. for synchronous reset), but
just want to be sure it's not a shortcut for having a proper driver.

Kevin

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 08/12/15 19:07, Kevin Hilman wrote:
> Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:
> 
>> Add power-domain binding documentation for the NVIDIA PMC driver in
>> order to support generic power-domains.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>>
>> Please note that I have been debating whether I add this
>> "nvidia,powergate-clock-disable" property or just leave the clocks
>> disabled by default. Some downstream kernels leave the clocks enabled
>> for the audio power-domain because the clocks required for powering up
>> the power-domain are needed by all modules within the power-domain.
>> However are the same time there are other power-domains that may need
>> to be on, but not always clocked and so having the ability to specify if
>> the clocks should be disabled seems useful. However, I can also remove
>> this and just have the appropriate devices turn on the clocks as well.
>> ---
>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 838e1a69ec0a..8e4641db51a9 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -1,5 +1,7 @@
>>  NVIDIA Tegra Power Management Controller (PMC)
>>  
>> +== Power Management Controller Node ==
>> +
>>  The PMC block interacts with an external Power Management Unit. The PMC
>>  mostly controls the entry and exit of the system from different sleep
>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>                       Defaults to 0. Valid values are described in section 12.5.2
>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>  
>> +Optional nodes:
>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>> +	       match the power-domains on the Tegra SoC.
>> +
>>  Example:
>>  
>>  / SoC dts including file
>> @@ -114,3 +120,58 @@ pmc@7000f400 {
>>  	};
>>  	...
>>  };
>> +
>> +
>> +== PM Domain Nodes ==
>> +
>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>> +that can be power-gated by the PMC and should be named appropriately.
>> +
>> +Required properties:
>> +  - clocks: Must contain an entry for each clock required by the PMC for
>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
> 
> We've had this discussiona for a couple of other SoCs, so I need to
> ask...
> 
> Presumably these are not device clocks that a runtime PM enabled driver
> should be managing for a device, right?  IOW, We want to make sure that the
> PM domain isn't managing clocks for drivers that should be doing it.
> 
> I understand there are legitimate reasons for the PM domain to manage
> clocks in addition to device drivers (e.g. for synchronous reset), but
> just want to be sure it's not a shortcut for having a proper driver.

So some clocks may also be used by devices, but they are needed as part
of the power ungating/gating sequence. The general power-up sequence for
tegra is ...

1. Enable the power-domain
2. Enable the clock(s)
3. Remove signal clamps
4. De-assert reset(s)
5. Disable clocks (optional)

You may say we should only handle #1 above for the powering up sequence,
but we can't do this. The reason is that there is one bit for each
power-domain that controls the signaling clamps and so we need to turn
on all the clocks specified in the TRM before we do this. Once we have
done this and released the reset(s), we can then disable the clocks
again (shown above an optional as it is not mandatory from a design
perspective) and then the devices in the power-domain should enable the
clocks they need as and when they want them.

Please note that I 100% agree that all clocks required by a device are
handled by the device and we do not implement any short-cuts here. The
only question I had was if there are clocks that may be bus clocks in
the power-domain that are required by all device in the power-domain.
However, may be this should be represented as a bus driver and all the
devices are children of it?

Hope that helps.

Cheers
Jon

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 09/12/15 12:23, Jon Hunter wrote:
> 
> On 08/12/15 19:07, Kevin Hilman wrote:
>> Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:
>>
>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>> order to support generic power-domains.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> ---
>>>
>>> Please note that I have been debating whether I add this
>>> "nvidia,powergate-clock-disable" property or just leave the clocks
>>> disabled by default. Some downstream kernels leave the clocks enabled
>>> for the audio power-domain because the clocks required for powering up
>>> the power-domain are needed by all modules within the power-domain.
>>> However are the same time there are other power-domains that may need
>>> to be on, but not always clocked and so having the ability to specify if
>>> the clocks should be disabled seems useful. However, I can also remove
>>> this and just have the appropriate devices turn on the clocks as well.
>>> ---
>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>>  1 file changed, 61 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> index 838e1a69ec0a..8e4641db51a9 100644
>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> @@ -1,5 +1,7 @@
>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>  
>>> +== Power Management Controller Node ==
>>> +
>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>  mostly controls the entry and exit of the system from different sleep
>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>  
>>> +Optional nodes:
>>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>>> +	       match the power-domains on the Tegra SoC.
>>> +
>>>  Example:
>>>  
>>>  / SoC dts including file
>>> @@ -114,3 +120,58 @@ pmc@7000f400 {
>>>  	};
>>>  	...
>>>  };
>>> +
>>> +
>>> +== PM Domain Nodes ==
>>> +
>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>> +that can be power-gated by the PMC and should be named appropriately.
>>> +
>>> +Required properties:
>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>>
>> We've had this discussiona for a couple of other SoCs, so I need to
>> ask...
>>
>> Presumably these are not device clocks that a runtime PM enabled driver
>> should be managing for a device, right?  IOW, We want to make sure that the
>> PM domain isn't managing clocks for drivers that should be doing it.
>>
>> I understand there are legitimate reasons for the PM domain to manage
>> clocks in addition to device drivers (e.g. for synchronous reset), but
>> just want to be sure it's not a shortcut for having a proper driver.
> 
> So some clocks may also be used by devices, but they are needed as part
> of the power ungating/gating sequence. The general power-up sequence for
> tegra is ...
> 
> 1. Enable the power-domain
> 2. Enable the clock(s)
> 3. Remove signal clamps
> 4. De-assert reset(s)
> 5. Disable clocks (optional)
> 
> You may say we should only handle #1 above for the powering up sequence,
> but we can't do this. The reason is that there is one bit for each
> power-domain that controls the signaling clamps and so we need to turn
> on all the clocks specified in the TRM before we do this. Once we have
> done this and released the reset(s), we can then disable the clocks
> again (shown above an optional as it is not mandatory from a design
> perspective) and then the devices in the power-domain should enable the
> clocks they need as and when they want them.
> 
> Please note that I 100% agree that all clocks required by a device are
> handled by the device and we do not implement any short-cuts here. The
> only question I had was if there are clocks that may be bus clocks in
> the power-domain that are required by all device in the power-domain.
> However, may be this should be represented as a bus driver and all the
> devices are children of it?

Although the "nvidia,powergate-disable-clocks" optional property I had
proposed could be seen as a bit of a short-cut, it is true :-)

May be I should make the disabling of clocks again mandatory for the
power-up sequence.

Jon

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Kevin Hilman]

Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:

> On 09/12/15 12:23, Jon Hunter wrote:
>> 
>> On 08/12/15 19:07, Kevin Hilman wrote:
>>> Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:
>>>
>>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>>> order to support generic power-domains.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> ---
>>>>
>>>> Please note that I have been debating whether I add this
>>>> "nvidia,powergate-clock-disable" property or just leave the clocks
>>>> disabled by default. Some downstream kernels leave the clocks enabled
>>>> for the audio power-domain because the clocks required for powering up
>>>> the power-domain are needed by all modules within the power-domain.
>>>> However are the same time there are other power-domains that may need
>>>> to be on, but not always clocked and so having the ability to specify if
>>>> the clocks should be disabled seems useful. However, I can also remove
>>>> this and just have the appropriate devices turn on the clocks as well.
>>>> ---
>>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>>>  1 file changed, 61 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> index 838e1a69ec0a..8e4641db51a9 100644
>>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> @@ -1,5 +1,7 @@
>>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>>  
>>>> +== Power Management Controller Node ==
>>>> +
>>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>>  mostly controls the entry and exit of the system from different sleep
>>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>>  
>>>> +Optional nodes:
>>>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>>>> +	       match the power-domains on the Tegra SoC.
>>>> +
>>>>  Example:
>>>>  
>>>>  / SoC dts including file
>>>> @@ -114,3 +120,58 @@ pmc@7000f400 {
>>>>  	};
>>>>  	...
>>>>  };
>>>> +
>>>> +
>>>> +== PM Domain Nodes ==
>>>> +
>>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>>> +that can be power-gated by the PMC and should be named appropriately.
>>>> +
>>>> +Required properties:
>>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>>>
>>> We've had this discussiona for a couple of other SoCs, so I need to
>>> ask...
>>>
>>> Presumably these are not device clocks that a runtime PM enabled driver
>>> should be managing for a device, right?  IOW, We want to make sure that the
>>> PM domain isn't managing clocks for drivers that should be doing it.
>>>
>>> I understand there are legitimate reasons for the PM domain to manage
>>> clocks in addition to device drivers (e.g. for synchronous reset), but
>>> just want to be sure it's not a shortcut for having a proper driver.
>> 
>> So some clocks may also be used by devices, but they are needed as part
>> of the power ungating/gating sequence. The general power-up sequence for
>> tegra is ...
>> 
>> 1. Enable the power-domain
>> 2. Enable the clock(s)
>> 3. Remove signal clamps
>> 4. De-assert reset(s)
>> 5. Disable clocks (optional)
>> 
>> You may say we should only handle #1 above for the powering up sequence,
>> but we can't do this. The reason is that there is one bit for each
>> power-domain that controls the signaling clamps and so we need to turn
>> on all the clocks specified in the TRM before we do this. Once we have
>> done this and released the reset(s), we can then disable the clocks
>> again (shown above an optional as it is not mandatory from a design
>> perspective) and then the devices in the power-domain should enable the
>> clocks they need as and when they want them.
>> 
>> Please note that I 100% agree that all clocks required by a device are
>> handled by the device and we do not implement any short-cuts here. The
>> only question I had was if there are clocks that may be bus clocks in
>> the power-domain that are required by all device in the power-domain.
>> However, may be this should be represented as a bus driver and all the
>> devices are children of it?
>
> Although the "nvidia,powergate-disable-clocks" optional property I had
> proposed could be seen as a bit of a short-cut, it is true :-)
> May be I should make the disabling of clocks again mandatory for the
> power-up sequence.

IIUC, that looks like a flag that tells the power-domain to just leave
all the clocks enabled after the domain power up?  Is that right?

To me that looks like possibly useful bringup hack, but a short-cut
that's ripe for abuse and would likely be used so that drivers don't
ever need to do their own clock management.

Kevin

Re: [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Kevin Hilman]

Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:

> On 08/12/15 19:07, Kevin Hilman wrote:
>> Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:
>> 
>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>> order to support generic power-domains.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> ---
>>>
>>> Please note that I have been debating whether I add this
>>> "nvidia,powergate-clock-disable" property or just leave the clocks
>>> disabled by default. Some downstream kernels leave the clocks enabled
>>> for the audio power-domain because the clocks required for powering up
>>> the power-domain are needed by all modules within the power-domain.
>>> However are the same time there are other power-domains that may need
>>> to be on, but not always clocked and so having the ability to specify if
>>> the clocks should be disabled seems useful. However, I can also remove
>>> this and just have the appropriate devices turn on the clocks as well.
>>> ---
>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 61 ++++++++++++++++++++++
>>>  1 file changed, 61 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> index 838e1a69ec0a..8e4641db51a9 100644
>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> @@ -1,5 +1,7 @@
>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>  
>>> +== Power Management Controller Node ==
>>> +
>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>  mostly controls the entry and exit of the system from different sleep
>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>> @@ -69,6 +71,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>  
>>> +Optional nodes:
>>> +- pm-domains : This node contains a hierarchy of PM domain nodes, which should
>>> +	       match the power-domains on the Tegra SoC.
>>> +
>>>  Example:
>>>  
>>>  / SoC dts including file
>>> @@ -114,3 +120,58 @@ pmc@7000f400 {
>>>  	};
>>>  	...
>>>  };
>>> +
>>> +
>>> +== PM Domain Nodes ==
>>> +
>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>> +that can be power-gated by the PMC and should be named appropriately.
>>> +
>>> +Required properties:
>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>> 
>> We've had this discussiona for a couple of other SoCs, so I need to
>> ask...
>> 
>> Presumably these are not device clocks that a runtime PM enabled driver
>> should be managing for a device, right?  IOW, We want to make sure that the
>> PM domain isn't managing clocks for drivers that should be doing it.
>> 
>> I understand there are legitimate reasons for the PM domain to manage
>> clocks in addition to device drivers (e.g. for synchronous reset), but
>> just want to be sure it's not a shortcut for having a proper driver.
>
> So some clocks may also be used by devices, but they are needed as part
> of the power ungating/gating sequence. The general power-up sequence for
> tegra is ...
>
> 1. Enable the power-domain
> 2. Enable the clock(s)
> 3. Remove signal clamps
> 4. De-assert reset(s)
> 5. Disable clocks (optional)
>
> You may say we should only handle #1 above for the powering up sequence,
> but we can't do this. The reason is that there is one bit for each
> power-domain that controls the signaling clamps and so we need to turn
> on all the clocks specified in the TRM before we do this. Once we have
> done this and released the reset(s), we can then disable the clocks
> again (shown above an optional as it is not mandatory from a design
> perspective) and then the devices in the power-domain should enable the
> clocks they need as and when they want them.

Thanks for the clarification, that's what I expected and similar to what
I've seen on other SoCs.  I just wanted to be sure that the power-domain
clk managent was "in addition" to the device drivers, not "instead of."

Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

Adds generic PM support to the PMC driver where the PM domains are
populated from device-tree and the PM domain consumer devices are
bound to their relevant PM domains via device-tree as well.

Update the tegra_powergate_sequence_power_up() API so that internally
it calls the same tegra_powergate_xxx functions that are used by the
tegra generic power domain code for consistency.

This is based upon work by Thierry Reding <treding@nvidia.com>
and Vince Hsu <vinceh@nvidia.com>.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c                     | 504 ++++++++++++++++++++++++++--
 include/dt-bindings/power/tegra-powergate.h |  35 ++
 include/soc/tegra/pmc.h                     |  38 +--
 3 files changed, 513 insertions(+), 64 deletions(-)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index b412098b8197..76bee999c85b 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -19,6 +19,8 @@
 
 #define pr_fmt(fmt) "tegra-pmc: " fmt
 
+#include <dt-bindings/power/tegra-powergate.h>
+
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
@@ -31,7 +33,9 @@
 #include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/reboot.h>
 #include <linux/reset.h>
 #include <linux/seq_file.h>
@@ -105,6 +109,20 @@
 #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
 #define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))
 
+struct tegra_powergate {
+	struct generic_pm_domain genpd;
+	struct generic_pm_domain *parent;
+	struct tegra_pmc *pmc;
+	unsigned int id;
+	struct list_head node;
+	struct device_node *of_node;
+	struct clk **clks;
+	unsigned int num_clks;
+	struct reset_control **resets;
+	unsigned int num_resets;
+	bool disable_clocks;
+};
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -137,6 +155,7 @@ struct tegra_pmc_soc {
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
  * @powergates_mask: Bit mask of valid power gates
+ * @powergates_list: list of power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {
@@ -163,6 +182,7 @@ struct tegra_pmc {
 	u32 lp0_vec_size;
 	u32 powergates_mask;
 
+	struct list_head powergates_list;
 	struct mutex powergates_lock;
 };
 
@@ -171,6 +191,12 @@ static struct tegra_pmc *pmc = &(struct tegra_pmc) {
 	.suspend_mode = TEGRA_SUSPEND_NONE,
 };
 
+static inline struct tegra_powergate *
+to_powergate(struct generic_pm_domain *domain)
+{
+	return container_of(domain, struct tegra_powergate, genpd);
+}
+
 static u32 tegra_pmc_readl(unsigned long offset)
 {
 	return readl(pmc->base + offset);
@@ -214,6 +240,177 @@ static int tegra_powergate_set(int id, bool new_state)
 	return err;
 }
 
+static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
+{
+	unsigned int i;
+
+	for (i = 0; i < pg->num_clks; i++)
+		clk_disable_unprepare(pg->clks[i]);
+}
+
+static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_clks; i++) {
+		err = clk_prepare_enable(pg->clks[i]);
+		if (err)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	while (i--)
+		clk_disable_unprepare(pg->clks[i]);
+
+	return err;
+}
+
+static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_resets; i++) {
+		err = reset_control_assert(pg->resets[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_resets; i++) {
+		err = reset_control_deassert(pg->resets[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_powergate_power_up(struct tegra_powergate *pg,
+				    bool disable_clocks)
+{
+	int err;
+
+	err = tegra_powergate_reset_assert(pg);
+	if (err)
+		return err;
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_set(pg->id, true);
+	if (err < 0)
+		return err;
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_enable_clocks(pg);
+	if (err)
+		goto err_clks;
+
+	usleep_range(10, 20);
+
+	tegra_powergate_remove_clamping(pg->id);
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_reset_deassert(pg);
+	if (err)
+		goto err_reset;
+
+	usleep_range(10, 20);
+
+	if (disable_clocks)
+		tegra_powergate_disable_clocks(pg);
+
+	return 0;
+
+err_reset:
+	tegra_powergate_disable_clocks(pg);
+	usleep_range(10, 20);
+err_clks:
+	tegra_powergate_set(pg->id, false);
+
+	return err;
+}
+
+static int tegra_powergate_power_down(struct tegra_powergate *pg,
+				      bool enable_clocks)
+{
+	int err;
+
+	if (enable_clocks) {
+		err = tegra_powergate_enable_clocks(pg);
+		if (err)
+			return err;
+
+		usleep_range(10, 20);
+	}
+
+	err = tegra_powergate_reset_assert(pg);
+	if (err)
+		goto err_reset;
+
+	usleep_range(10, 20);
+
+	tegra_powergate_disable_clocks(pg);
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_set(pg->id, false);
+	if (err)
+		goto err_powergate;
+
+	return 0;
+
+err_powergate:
+	tegra_powergate_enable_clocks(pg);
+	usleep_range(10, 20);
+	tegra_powergate_reset_deassert(pg);
+	usleep_range(10, 20);
+err_reset:
+	tegra_powergate_disable_clocks(pg);
+
+	return err;
+}
+
+static int tegra_genpd_power_on(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *pg = to_powergate(domain);
+	struct tegra_pmc *pmc = pg->pmc;
+	int err;
+
+	err = tegra_powergate_power_up(pg, pg->disable_clocks);
+	if (err)
+		dev_err(pmc->dev, "failed to turn on PM domain %s: %d\n",
+			pg->of_node->name, err);
+
+	return err;
+}
+
+static int tegra_genpd_power_off(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *pg = to_powergate(domain);
+	struct tegra_pmc *pmc = pg->pmc;
+	int err;
+
+	err = tegra_powergate_power_down(pg, !pg->disable_clocks);
+	if (err)
+		dev_err(pmc->dev, "failed to turn off PM domain %s: %d\n",
+			pg->of_node->name, err);
+
+	return err;
+}
+
 /**
  * tegra_powergate_power_on() - power on partition
  * @id: partition ID
@@ -308,35 +505,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
 int tegra_powergate_sequence_power_up(int id, struct clk *clk,
 				      struct reset_control *rst)
 {
-	int ret;
-
-	reset_control_assert(rst);
-
-	ret = tegra_powergate_power_on(id);
-	if (ret)
-		goto err_power;
-
-	ret = clk_prepare_enable(clk);
-	if (ret)
-		goto err_clk;
-
-	usleep_range(10, 20);
-
-	ret = tegra_powergate_remove_clamping(id);
-	if (ret)
-		goto err_clamp;
+	struct tegra_powergate pg;
+	int err;
 
-	usleep_range(10, 20);
-	reset_control_deassert(rst);
+	pg.id = id;
+	pg.clks = &clk;
+	pg.num_clks = 1;
+	pg.resets = &rst;
+	pg.num_resets = 1;
 
-	return 0;
+	err = tegra_powergate_power_up(&pg, false);
+	if (err)
+		pr_err("failed to turn on partition %d: %d\n", id, err);
 
-err_clamp:
-	clk_disable_unprepare(clk);
-err_clk:
-	tegra_powergate_power_off(id);
-err_power:
-	return ret;
+	return err;
 }
 EXPORT_SYMBOL(tegra_powergate_sequence_power_up);
 
@@ -476,6 +658,260 @@ static int tegra_powergate_debugfs_init(void)
 	return 0;
 }
 
+static int tegra_powergate_of_get_clks(struct device *dev,
+				       struct tegra_powergate *pg)
+{
+	struct clk *clk;
+	unsigned int i, count;
+	int err;
+
+	/*
+	 * Determine number of clocks used by the powergate
+	 */
+	for (count = 0; ; count++) {
+		clk = of_clk_get(pg->of_node, count);
+		if (IS_ERR(clk))
+			break;
+
+		clk_put(clk);
+	}
+
+	if (PTR_ERR(clk) != -ENOENT)
+		return PTR_ERR(clk);
+
+	if (count == 0)
+		return -ENODEV;
+
+	pg->clks = devm_kcalloc(dev, count, sizeof(clk), GFP_KERNEL);
+	if (!pg->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		pg->clks[i] = of_clk_get(pg->of_node, i);
+		if (IS_ERR(pg->clks[i])) {
+			err = PTR_ERR(pg->clks[i]);
+			goto err;
+		}
+	}
+
+	pg->num_clks = count;
+
+	return 0;
+
+err:
+	while (i--)
+		clk_put(pg->clks[i]);
+
+	return err;
+}
+
+static int tegra_powergate_of_get_resets(struct device *dev,
+					 struct tegra_powergate *pg)
+{
+	struct reset_control *rst;
+	unsigned int i, count;
+	int err;
+
+	/*
+	 * Determine number of resets used by the powergate
+	 */
+	for (count = 0; ; count++) {
+		rst = of_reset_control_get_by_index(pg->of_node, count);
+		if (IS_ERR(rst))
+			break;
+
+		reset_control_put(rst);
+	}
+
+	if (PTR_ERR(rst) != -ENOENT)
+		return PTR_ERR(rst);
+
+	if (count == 0)
+		return -ENODEV;
+
+	pg->resets = devm_kcalloc(dev, count, sizeof(rst), GFP_KERNEL);
+	if (!pg->resets)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		pg->resets[i] = of_reset_control_get_by_index(pg->of_node, i);
+		if (IS_ERR(pg->resets[i])) {
+			err = PTR_ERR(pg->resets[i]);
+			goto err;
+		}
+	}
+
+	pg->num_resets = count;
+
+	return 0;
+
+err:
+	while (i--)
+		reset_control_put(pg->resets[i]);
+
+	return err;
+}
+
+static struct tegra_powergate *
+tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
+			struct generic_pm_domain *parent)
+{
+	struct tegra_powergate *pg;
+	unsigned int id;
+	bool off;
+	int err;
+
+	/*
+	 * If the powergate ID is missing or invalid then return NULL
+	 * to skip this one and allow any others to be created.
+	 */
+	err = of_property_read_u32(np, "nvidia,powergate", &id);
+	if (err) {
+		dev_WARN(pmc->dev, "no powergate ID for node %s\n", np->name);
+		return NULL;
+	}
+
+	if (!PMC_PWRGATE_IS_VALID(id)) {
+		dev_WARN(pmc->dev, "powergate ID invalid for %s\n", np->name);
+		return NULL;
+	}
+
+	pg = devm_kzalloc(pmc->dev, sizeof(*pg), GFP_KERNEL);
+	if (!pg) {
+		err = -ENOMEM;
+		goto err_mem;
+	}
+
+	pg->id = id;
+	pg->of_node = np;
+	pg->parent = parent;
+	pg->genpd.name = np->name;
+	pg->genpd.power_off = tegra_genpd_power_off;
+	pg->genpd.power_on = tegra_genpd_power_on;
+	pg->pmc = pmc;
+
+	pg->disable_clocks = of_property_read_bool(np,
+				"nvidia,powergate-disable-clocks");
+
+	err = tegra_powergate_of_get_clks(pmc->dev, pg);
+	if (err)
+		goto err_mem;
+
+	err = tegra_powergate_of_get_resets(pmc->dev, pg);
+	if (err)
+		goto err_reset;
+
+	off = !tegra_powergate_is_powered(pg->id);
+
+	pm_genpd_init(&pg->genpd, NULL, off);
+
+	if (pg->parent) {
+		err = pm_genpd_add_subdomain(pg->parent, &pg->genpd);
+		if (err)
+			goto err_subdomain;
+	}
+
+	err = of_genpd_add_provider_simple(pg->of_node, &pg->genpd);
+	if (err)
+		goto err_provider;
+
+	list_add_tail(&pg->node, &pmc->powergates_list);
+
+	dev_info(pmc->dev, "added power domain %s\n", pg->genpd.name);
+
+	return pg;
+
+err_provider:
+	WARN_ON(pm_genpd_remove_subdomain(pg->parent, &pg->genpd));
+err_subdomain:
+	WARN_ON(pm_genpd_remove(&pg->genpd));
+	while (pg->num_resets--)
+		reset_control_put(pg->resets[pg->num_resets]);
+err_reset:
+	while (pg->num_clks--)
+		clk_put(pg->clks[pg->num_clks]);
+err_mem:
+	dev_err(pmc->dev, "failed to create power domain for node %s\n",
+		np->name);
+
+	return ERR_PTR(err);
+}
+
+static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np,
+			       struct generic_pm_domain *parent)
+{
+	struct tegra_powergate *pg;
+	struct device_node *child;
+	int err = 0;
+
+	for_each_child_of_node(np, child) {
+		if (err)
+			goto err;
+
+		pg = tegra_powergate_add_one(pmc, child, parent);
+		if (IS_ERR(pg)) {
+			err = PTR_ERR(pg);
+			goto err;
+		}
+
+		if (pg)
+			err = tegra_powergate_add(pmc, child, pg->parent);
+
+err:
+		of_node_put(child);
+
+		if (err)
+			return err;
+	}
+
+	return err;
+}
+
+static void tegra_powergate_remove(struct tegra_pmc *pmc)
+{
+	struct tegra_powergate *pg, *n;
+
+	list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
+		of_genpd_del_provider(pg->of_node);
+		if (pg->parent)
+			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
+		pm_genpd_remove(&pg->genpd);
+
+		while (pg->num_clks--)
+			clk_put(pg->clks[pg->num_clks]);
+
+		while (pg->num_resets--)
+			reset_control_put(pg->resets[pg->num_resets]);
+
+		list_del(&pg->node);
+	}
+}
+
+static int tegra_powergate_init(struct tegra_pmc *pmc)
+{
+	struct device_node *np;
+	int err;
+
+	INIT_LIST_HEAD(&pmc->powergates_list);
+
+	np = of_get_child_by_name(pmc->dev->of_node, "pm-domains");
+	if (!np) {
+		dev_dbg(pmc->dev, "pm-domains node not found\n");
+		return 0;
+	}
+
+	err = tegra_powergate_add(pmc, np, NULL);
+	if (err) {
+		tegra_powergate_remove(pmc);
+		of_node_put(np);
+		return err;
+	}
+
+	of_node_put(np);
+
+	return 0;
+}
+
 static int tegra_io_rail_prepare(int id, unsigned long *request,
 				 unsigned long *status, unsigned int *bit)
 {
@@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_init_tsense_reset(pmc);
 
+	err = tegra_powergate_init(pmc);
+	if (err < 0)
+		return err;
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			return err;
+			goto err_debugfs;
 	}
 
 	err = register_restart_handler(&tegra_pmc_restart_handler);
 	if (err) {
-		debugfs_remove(pmc->debugfs);
 		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
 			err);
-		return err;
+		goto err_restart;
 	}
 
 	return 0;
+
+err_restart:
+	debugfs_remove(pmc->debugfs);
+err_debugfs:
+	tegra_powergate_remove(pmc);
+
+	return err;
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
diff --git a/include/dt-bindings/power/tegra-powergate.h b/include/dt-bindings/power/tegra-powergate.h
new file mode 100644
index 000000000000..12b72aa77d6a
--- /dev/null
+++ b/include/dt-bindings/power/tegra-powergate.h
@@ -0,0 +1,35 @@
+#ifndef _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+#define _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+
+#define TEGRA_POWERGATE_CPU	0
+#define TEGRA_POWERGATE_3D	1
+#define TEGRA_POWERGATE_VENC	2
+#define TEGRA_POWERGATE_PCIE	3
+#define TEGRA_POWERGATE_VDEC	4
+#define TEGRA_POWERGATE_L2	5
+#define TEGRA_POWERGATE_MPE	6
+#define TEGRA_POWERGATE_HEG	7
+#define TEGRA_POWERGATE_SATA	8
+#define TEGRA_POWERGATE_CPU1	9
+#define TEGRA_POWERGATE_CPU2	10
+#define TEGRA_POWERGATE_CPU3	11
+#define TEGRA_POWERGATE_CELP	12
+#define TEGRA_POWERGATE_3D1	13
+#define TEGRA_POWERGATE_CPU0	14
+#define TEGRA_POWERGATE_C0NC	15
+#define TEGRA_POWERGATE_C1NC	16
+#define TEGRA_POWERGATE_SOR	17
+#define TEGRA_POWERGATE_DIS	18
+#define TEGRA_POWERGATE_DISB	19
+#define TEGRA_POWERGATE_XUSBA	20
+#define TEGRA_POWERGATE_XUSBB	21
+#define TEGRA_POWERGATE_XUSBC	22
+#define TEGRA_POWERGATE_VIC	23
+#define TEGRA_POWERGATE_IRAM	24
+#define TEGRA_POWERGATE_NVDEC	25
+#define TEGRA_POWERGATE_NVJPG	26
+#define TEGRA_POWERGATE_AUD	27
+#define TEGRA_POWERGATE_DFD	28
+#define TEGRA_POWERGATE_VE2	29
+
+#endif
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index d18efe402ff1..61c9f847f2b2 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -19,6 +19,8 @@
 #ifndef __SOC_TEGRA_PMC_H__
 #define __SOC_TEGRA_PMC_H__
 
+#include <dt-bindings/power/tegra-powergate.h>
+
 #include <linux/reboot.h>
 
 #include <soc/tegra/pm.h>
@@ -39,42 +41,8 @@ int tegra_pmc_cpu_remove_clamping(int cpuid);
 #endif /* CONFIG_SMP */
 
 /*
- * powergate and I/O rail APIs
+ * I/O rail APIs
  */
-
-#define TEGRA_POWERGATE_CPU	0
-#define TEGRA_POWERGATE_3D	1
-#define TEGRA_POWERGATE_VENC	2
-#define TEGRA_POWERGATE_PCIE	3
-#define TEGRA_POWERGATE_VDEC	4
-#define TEGRA_POWERGATE_L2	5
-#define TEGRA_POWERGATE_MPE	6
-#define TEGRA_POWERGATE_HEG	7
-#define TEGRA_POWERGATE_SATA	8
-#define TEGRA_POWERGATE_CPU1	9
-#define TEGRA_POWERGATE_CPU2	10
-#define TEGRA_POWERGATE_CPU3	11
-#define TEGRA_POWERGATE_CELP	12
-#define TEGRA_POWERGATE_3D1	13
-#define TEGRA_POWERGATE_CPU0	14
-#define TEGRA_POWERGATE_C0NC	15
-#define TEGRA_POWERGATE_C1NC	16
-#define TEGRA_POWERGATE_SOR	17
-#define TEGRA_POWERGATE_DIS	18
-#define TEGRA_POWERGATE_DISB	19
-#define TEGRA_POWERGATE_XUSBA	20
-#define TEGRA_POWERGATE_XUSBB	21
-#define TEGRA_POWERGATE_XUSBC	22
-#define TEGRA_POWERGATE_VIC	23
-#define TEGRA_POWERGATE_IRAM	24
-#define TEGRA_POWERGATE_NVDEC	25
-#define TEGRA_POWERGATE_NVJPG	26
-#define TEGRA_POWERGATE_AUD	27
-#define TEGRA_POWERGATE_DFD	28
-#define TEGRA_POWERGATE_VE2	29
-
-#define TEGRA_POWERGATE_3D0	TEGRA_POWERGATE_3D
-
 #define TEGRA_IO_RAIL_CSIA	0
 #define TEGRA_IO_RAIL_CSIB	1
 #define TEGRA_IO_RAIL_DSI	2
-- 
2.1.4

Re: [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
[...]
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
[...]
> +static int tegra_powergate_power_down(struct tegra_powergate *pg,
> +				      bool enable_clocks)
> +{
> +	int err;
> +
> +	if (enable_clocks) {
> +		err = tegra_powergate_enable_clocks(pg);
> +		if (err)
> +			return err;
> +
> +		usleep_range(10, 20);
> +	}
> +
> +	err = tegra_powergate_reset_assert(pg);
> +	if (err)
> +		goto err_reset;
> +
> +	usleep_range(10, 20);
> +
> +	tegra_powergate_disable_clocks(pg);

There's no guarantee that all clocks are actually disabled at this
point. Will the power down and subsequent power up sequences still
work properly in such cases? If not perhaps we should add some way
of checking for that case here (WARN_ON?) to make sure we can fix
up all drivers to release their clock enable references.

> +static int tegra_powergate_of_get_clks(struct device *dev,
> +				       struct tegra_powergate *pg)
> +{
> +	struct clk *clk;
> +	unsigned int i, count;
> +	int err;
> +
> +	/*
> +	 * Determine number of clocks used by the powergate
> +	 */
> +	for (count = 0; ; count++) {
> +		clk = of_clk_get(pg->of_node, count);
> +		if (IS_ERR(clk))
> +			break;
> +
> +		clk_put(clk);
> +	}

of_count_phandle_with_args()?

> +static int tegra_powergate_of_get_resets(struct device *dev,
> +					 struct tegra_powergate *pg)
> +{
> +	struct reset_control *rst;
> +	unsigned int i, count;
> +	int err;
> +
> +	/*
> +	 * Determine number of resets used by the powergate
> +	 */
> +	for (count = 0; ; count++) {
> +		rst = of_reset_control_get_by_index(pg->of_node, count);
> +		if (IS_ERR(rst))
> +			break;
> +
> +		reset_control_put(rst);
> +	}

Same here.

> +static struct tegra_powergate *
> +tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
> +			struct generic_pm_domain *parent)
> +{
[...]
> +	dev_info(pmc->dev, "added power domain %s\n", pg->genpd.name);

That's a little chatty, isn't it? Perhaps dev_dbg()?

> +static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np,
> +			       struct generic_pm_domain *parent)
> +{
> +	struct tegra_powergate *pg;
> +	struct device_node *child;
> +	int err = 0;
> +
> +	for_each_child_of_node(np, child) {
> +		if (err)
> +			goto err;

This looks weird. Isn't the same check below good enough to catch all
cases?

> +
> +		pg = tegra_powergate_add_one(pmc, child, parent);
> +		if (IS_ERR(pg)) {
> +			err = PTR_ERR(pg);
> +			goto err;
> +		}
> +
> +		if (pg)
> +			err = tegra_powergate_add(pmc, child, pg->parent);
> +
> +err:
> +		of_node_put(child);
> +
> +		if (err)
> +			return err;

Perhaps break here instead of return?

> +	}
> +
> +	return err;
> +}
> +
> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
> +{
> +	struct tegra_powergate *pg, *n;
> +
> +	list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
> +		of_genpd_del_provider(pg->of_node);
> +		if (pg->parent)
> +			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
> +		pm_genpd_remove(&pg->genpd);
> +
> +		while (pg->num_clks--)
> +			clk_put(pg->clks[pg->num_clks]);
> +
> +		while (pg->num_resets--)
> +			reset_control_put(pg->resets[pg->num_resets]);
> +
> +		list_del(&pg->node);
> +	}
> +}

Are generic power domains reference counted? If not this will
potentially leave dangling pointers in user drivers, won't it?

That's a problem common to many subsystems, but maybe something to be
aware of.

> @@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>  
>  	tegra_pmc_init_tsense_reset(pmc);
>  
> +	err = tegra_powergate_init(pmc);
> +	if (err < 0)
> +		return err;
> +
>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>  		err = tegra_powergate_debugfs_init();
>  		if (err < 0)
> -			return err;
> +			goto err_debugfs;
>  	}
>  
>  	err = register_restart_handler(&tegra_pmc_restart_handler);
>  	if (err) {
> -		debugfs_remove(pmc->debugfs);
>  		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
>  			err);
> -		return err;
> +		goto err_restart;
>  	}
>  
>  	return 0;
> +
> +err_restart:
> +	debugfs_remove(pmc->debugfs);
> +err_debugfs:
> +	tegra_powergate_remove(pmc);

I prefer the labels to denote the action that is being taken rather than
the error that they respond to. remove_debugfs and remove_powergate
would therefore be better here, in my opinion. I think there were a
couple more in this and earlier patches.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 14/01/16 14:39, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
> [...]
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> [...]
>> +static int tegra_powergate_power_down(struct tegra_powergate *pg,
>> +				      bool enable_clocks)
>> +{
>> +	int err;
>> +
>> +	if (enable_clocks) {
>> +		err = tegra_powergate_enable_clocks(pg);
>> +		if (err)
>> +			return err;
>> +
>> +		usleep_range(10, 20);
>> +	}
>> +
>> +	err = tegra_powergate_reset_assert(pg);
>> +	if (err)
>> +		goto err_reset;
>> +
>> +	usleep_range(10, 20);
>> +
>> +	tegra_powergate_disable_clocks(pg);
> 
> There's no guarantee that all clocks are actually disabled at this
> point. Will the power down and subsequent power up sequences still
> work properly in such cases? If not perhaps we should add some way
> of checking for that case here (WARN_ON?) to make sure we can fix
> up all drivers to release their clock enable references.

The problem is that there is no easy way to check the status of a clock
and whether it is enabled. Yes clk-provider.h does provide a
__clk_is_enabled() API but I don't think that this is meant to be used
here. May be we need a clk API for disabling a clock that will WARN if
the clock is not disabled?

>> +static int tegra_powergate_of_get_clks(struct device *dev,
>> +				       struct tegra_powergate *pg)
>> +{
>> +	struct clk *clk;
>> +	unsigned int i, count;
>> +	int err;
>> +
>> +	/*
>> +	 * Determine number of clocks used by the powergate
>> +	 */
>> +	for (count = 0; ; count++) {
>> +		clk = of_clk_get(pg->of_node, count);
>> +		if (IS_ERR(clk))
>> +			break;
>> +
>> +		clk_put(clk);
>> +	}
> 
> of_count_phandle_with_args()?

Ok.

>> +static int tegra_powergate_of_get_resets(struct device *dev,
>> +					 struct tegra_powergate *pg)
>> +{
>> +	struct reset_control *rst;
>> +	unsigned int i, count;
>> +	int err;
>> +
>> +	/*
>> +	 * Determine number of resets used by the powergate
>> +	 */
>> +	for (count = 0; ; count++) {
>> +		rst = of_reset_control_get_by_index(pg->of_node, count);
>> +		if (IS_ERR(rst))
>> +			break;
>> +
>> +		reset_control_put(rst);
>> +	}
> 
> Same here.

Ok.

>> +static struct tegra_powergate *
>> +tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
>> +			struct generic_pm_domain *parent)
>> +{
> [...]
>> +	dev_info(pmc->dev, "added power domain %s\n", pg->genpd.name);
> 
> That's a little chatty, isn't it? Perhaps dev_dbg()?

Ok.

>> +static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np,
>> +			       struct generic_pm_domain *parent)
>> +{
>> +	struct tegra_powergate *pg;
>> +	struct device_node *child;
>> +	int err = 0;
>> +
>> +	for_each_child_of_node(np, child) {
>> +		if (err)
>> +			goto err;
> 
> This looks weird. Isn't the same check below good enough to catch all
> cases?

Yes, but this function is called recursively.

>> +
>> +		pg = tegra_powergate_add_one(pmc, child, parent);
>> +		if (IS_ERR(pg)) {
>> +			err = PTR_ERR(pg);
>> +			goto err;
>> +		}
>> +
>> +		if (pg)
>> +			err = tegra_powergate_add(pmc, child, pg->parent);
>> +
>> +err:
>> +		of_node_put(child);
>> +
>> +		if (err)
>> +			return err;
> 
> Perhaps break here instead of return?
> 
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
>> +{
>> +	struct tegra_powergate *pg, *n;
>> +
>> +	list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
>> +		of_genpd_del_provider(pg->of_node);
>> +		if (pg->parent)
>> +			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
>> +		pm_genpd_remove(&pg->genpd);
>> +
>> +		while (pg->num_clks--)
>> +			clk_put(pg->clks[pg->num_clks]);
>> +
>> +		while (pg->num_resets--)
>> +			reset_control_put(pg->resets[pg->num_resets]);
>> +
>> +		list_del(&pg->node);
>> +	}
>> +}
> 
> Are generic power domains reference counted? If not this will
> potentially leave dangling pointers in user drivers, won't it?
> 
> That's a problem common to many subsystems, but maybe something to be
> aware of.

pm_genpd_remove and pm_genpd_remove_subdomain can fail if they have
users and so I should check for this. The problem is what to do if one
fails? Just WARN and break? May be that is best even if some to do get
removed and we end up in a state with some populated and some removed.

>> @@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>>  
>>  	tegra_pmc_init_tsense_reset(pmc);
>>  
>> +	err = tegra_powergate_init(pmc);
>> +	if (err < 0)
>> +		return err;
>> +
>>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>>  		err = tegra_powergate_debugfs_init();
>>  		if (err < 0)
>> -			return err;
>> +			goto err_debugfs;
>>  	}
>>  
>>  	err = register_restart_handler(&tegra_pmc_restart_handler);
>>  	if (err) {
>> -		debugfs_remove(pmc->debugfs);
>>  		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
>>  			err);
>> -		return err;
>> +		goto err_restart;
>>  	}
>>  
>>  	return 0;
>> +
>> +err_restart:
>> +	debugfs_remove(pmc->debugfs);
>> +err_debugfs:
>> +	tegra_powergate_remove(pmc);
> 
> I prefer the labels to denote the action that is being taken rather than
> the error that they respond to. remove_debugfs and remove_powergate
> would therefore be better here, in my opinion. I think there were a
> couple more in this and earlier patches.

Ok.

Jon

Re: [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

[Lucas Stach]

Am Freitag, den 15.01.2016, 09:42 +0000 schrieb Jon Hunter:
> On 14/01/16 14:39, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
> > [...]
> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > [...]
> >> +static int tegra_powergate_power_down(struct tegra_powergate *pg,
> >> +				      bool enable_clocks)
> >> +{
> >> +	int err;
> >> +
> >> +	if (enable_clocks) {
> >> +		err = tegra_powergate_enable_clocks(pg);
> >> +		if (err)
> >> +			return err;
> >> +
> >> +		usleep_range(10, 20);
> >> +	}
> >> +
> >> +	err = tegra_powergate_reset_assert(pg);
> >> +	if (err)
> >> +		goto err_reset;
> >> +
> >> +	usleep_range(10, 20);
> >> +
> >> +	tegra_powergate_disable_clocks(pg);
> > 
> > There's no guarantee that all clocks are actually disabled at this
> > point. Will the power down and subsequent power up sequences still
> > work properly in such cases? If not perhaps we should add some way
> > of checking for that case here (WARN_ON?) to make sure we can fix
> > up all drivers to release their clock enable references.
> 
> The problem is that there is no easy way to check the status of a clock
> and whether it is enabled. Yes clk-provider.h does provide a
> __clk_is_enabled() API but I don't think that this is meant to be used
> here. May be we need a clk API for disabling a clock that will WARN if
> the clock is not disabled?
> 
I can't find any hint in the TRM that disabling the clocks is required
for clamping/powergating.

All occurrences of those clock requirements are in regard to a
synchronous reset propagation. So there is no requirement that the
clocks need to be _disabled_ at a certain time, but actually they just
need to be _enabled_ whenever the PMC is trying to drive the resets in
any way.

Obviously the power savings might be higher when the peripheral drivers
properly disable any unused clocks in suspend, but that can be solved on
a case by case basis I think.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 14/16] clk: tegra210: Add the APB2APE audio clock

[Jon Hunter]

The APB2APE clock for the audio subsystem is required for powering up the
audio power domain and accessing the various modules in this subsystem on
the tegra210 device. Add this clock for tegra210.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/clk/tegra/clk-id.h               | 1 +
 drivers/clk/tegra/clk-tegra-periph.c     | 1 +
 drivers/clk/tegra/clk-tegra210.c         | 1 +
 include/dt-bindings/clock/tegra210-car.h | 2 +-
 4 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
index 19ce0738ee76..62ea38187b71 100644
--- a/drivers/clk/tegra/clk-id.h
+++ b/drivers/clk/tegra/clk-id.h
@@ -11,6 +11,7 @@ enum clk_id {
 	tegra_clk_afi,
 	tegra_clk_amx,
 	tegra_clk_amx1,
+	tegra_clk_apb2ape,
 	tegra_clk_apbdma,
 	tegra_clk_apbif,
 	tegra_clk_ape,
diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
index 6ad381a888a6..f8360c850c69 100644
--- a/drivers/clk/tegra/clk-tegra-periph.c
+++ b/drivers/clk/tegra/clk-tegra-periph.c
@@ -829,6 +829,7 @@ static struct tegra_periph_init_data gate_clks[] = {
 	GATE("xusb_gate", "osc", 143, 0, tegra_clk_xusb_gate, 0),
 	GATE("pll_p_out_cpu", "pll_p", 223, 0, tegra_clk_pll_p_out_cpu, 0),
 	GATE("pll_p_out_adsp", "pll_p", 187, 0, tegra_clk_pll_p_out_adsp, 0),
+	GATE("apb2ape", "clk_m", 107, 0, tegra_clk_apb2ape, 0),
 };
 
 static struct tegra_periph_init_data div_clks[] = {
diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index 58514c44ea83..e2d44a6a6b96 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -2218,6 +2218,7 @@ static struct tegra_clk tegra210_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_pll_c4_out1] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT1, .present = true },
 	[tegra_clk_pll_c4_out2] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT2, .present = true },
 	[tegra_clk_pll_c4_out3] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT3, .present = true },
+	[tegra_clk_apb2ape] = { .dt_id = TEGRA210_CLK_APB2APE, .present = true },
 };
 
 static struct tegra_devclk devclks[] __initdata = {
diff --git a/include/dt-bindings/clock/tegra210-car.h b/include/dt-bindings/clock/tegra210-car.h
index 6f45aea49e4f..0a05b0d36ae7 100644
--- a/include/dt-bindings/clock/tegra210-car.h
+++ b/include/dt-bindings/clock/tegra210-car.h
@@ -126,7 +126,7 @@
 /* 104 */
 /* 105 */
 #define TEGRA210_CLK_D_AUDIO 106
-/* 107 ( affects abp -> ape) */
+#define TEGRA210_CLK_APB2APE 107
 /* 108 */
 /* 109 */
 /* 110 */
-- 
2.1.4

[PATCH V4 15/16] ARM64: tegra: Add audio PM domain device node for Tegra210

[Jon Hunter]

Add the audio power-domain for tegra210. Note that this also removes the
existing "#power-domain-cells" which was incorrectly included by
commit e53095857166 ("arm64: tegra: Add Tegra210 support").

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

---

So far I have only added the audio power-domain for tegra210 as this is
what I have been testing with to date. However, once this series is
accepted then we can begin to add more.
---
 arch/arm/boot/dts/tegra124.dtsi          |  1 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 68669f791c8b..d868f4452c4e 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -3,6 +3,7 @@
 #include <dt-bindings/memory/tegra124-mc.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
+#include <dt-bindings/power/tegra-powergate.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset/tegra124-car.h>
 #include <dt-bindings/thermal/tegra124-soctherm.h>
diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index bc23f4dea002..c5e9c320092b 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -2,6 +2,7 @@
 #include <dt-bindings/gpio/tegra-gpio.h>
 #include <dt-bindings/memory/tegra210-mc.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
+#include <dt-bindings/power/tegra-powergate.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
@@ -581,7 +582,15 @@
 		clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
 		clock-names = "pclk", "clk32k_in";
 
-		#power-domain-cells = <1>;
+		pm-domains {
+			pd_audio: aud {
+				clocks = <&tegra_car TEGRA210_CLK_APE>,
+					 <&tegra_car TEGRA210_CLK_APB2APE>;
+				resets = <&tegra_car 198>;
+				nvidia,powergate = <TEGRA_POWERGATE_AUD>;
+				#power-domain-cells = <0>;
+			};
+		};
 	};
 
 	fuse@0,7000f800 {
-- 
2.1.4

[PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
dependent upon a particular power-domain are only probed when that power
domain has been powered up, requires that PM is made mandatory for tegra
64-bit devices and so select this option for tegra as well.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 9806324fa215..e0b5bd0aff0f 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -93,6 +93,8 @@ config ARCH_TEGRA
 	select GENERIC_CLOCKEVENTS
 	select HAVE_CLK
 	select PINCTRL
+	select PM
+	select PM_GENERIC_DOMAINS
 	select RESET_CONTROLLER
 	help
 	  This enables support for the NVIDIA Tegra SoC family.
-- 
2.1.4

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Ulf Hansson]

On 4 December 2015 at 15:57, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
> dependent upon a particular power-domain are only probed when that power
> domain has been powered up, requires that PM is made mandatory for tegra
> 64-bit devices and so select this option for tegra as well.
>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm64/Kconfig.platforms | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 9806324fa215..e0b5bd0aff0f 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>         select GENERIC_CLOCKEVENTS
>         select HAVE_CLK
>         select PINCTRL
> +       select PM
> +       select PM_GENERIC_DOMAINS

If you still want to allow ARCH_TEGRA to run without PM, you should
probably change to:

select PM_GENERIC_DOMAINS if PM

>         select RESET_CONTROLLER
>         help
>           This enables support for the NVIDIA Tegra SoC family.
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

Hi Ulf,

On 15/12/15 19:54, Ulf Hansson wrote:
> On 4 December 2015 at 15:57, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>> dependent upon a particular power-domain are only probed when that power
>> domain has been powered up, requires that PM is made mandatory for tegra
>> 64-bit devices and so select this option for tegra as well.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm64/Kconfig.platforms | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 9806324fa215..e0b5bd0aff0f 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>         select GENERIC_CLOCKEVENTS
>>         select HAVE_CLK
>>         select PINCTRL
>> +       select PM
>> +       select PM_GENERIC_DOMAINS
> 
> If you still want to allow ARCH_TEGRA to run without PM, you should
> probably change to:
> 
> select PM_GENERIC_DOMAINS if PM

Per the changelog this is deliberate. If we allow !PM, then there is a
potential that you could probe a device when the power domain is not
powered on. I understand that some SoCs turn on all the power-domains
when !PM but this will not work for tegra because we don't register the
power domain until later in the boot and so we are relying upon probe
deferral to defer the probe of devices that use power-domains.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Ulf Hansson]

On 16 December 2015 at 10:40, Jon Hunter <jonathanh@nvidia.com> wrote:
> Hi Ulf,
>
> On 15/12/15 19:54, Ulf Hansson wrote:
>> On 4 December 2015 at 15:57, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>>> dependent upon a particular power-domain are only probed when that power
>>> domain has been powered up, requires that PM is made mandatory for tegra
>>> 64-bit devices and so select this option for tegra as well.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  arch/arm64/Kconfig.platforms | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>> index 9806324fa215..e0b5bd0aff0f 100644
>>> --- a/arch/arm64/Kconfig.platforms
>>> +++ b/arch/arm64/Kconfig.platforms
>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>>         select GENERIC_CLOCKEVENTS
>>>         select HAVE_CLK
>>>         select PINCTRL
>>> +       select PM
>>> +       select PM_GENERIC_DOMAINS
>>
>> If you still want to allow ARCH_TEGRA to run without PM, you should
>> probably change to:
>>
>> select PM_GENERIC_DOMAINS if PM
>
> Per the changelog this is deliberate. If we allow !PM, then there is a
> potential that you could probe a device when the power domain is not
> powered on. I understand that some SoCs turn on all the power-domains
> when !PM but this will not work for tegra because we don't register the
> power domain until later in the boot and so we are relying upon probe
> deferral to defer the probe of devices that use power-domains.

So what you are saying is that adding the PM domain support, will fix
some devices to become successfully probed as they were broken before?

If that's the case, this makes all good sense!

Kind regards
Uffe

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

On 16/12/15 09:47, Ulf Hansson wrote:
> On 16 December 2015 at 10:40, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Ulf,
>>
>> On 15/12/15 19:54, Ulf Hansson wrote:
>>> On 4 December 2015 at 15:57, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>>>> dependent upon a particular power-domain are only probed when that power
>>>> domain has been powered up, requires that PM is made mandatory for tegra
>>>> 64-bit devices and so select this option for tegra as well.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  arch/arm64/Kconfig.platforms | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>> index 9806324fa215..e0b5bd0aff0f 100644
>>>> --- a/arch/arm64/Kconfig.platforms
>>>> +++ b/arch/arm64/Kconfig.platforms
>>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>>>         select GENERIC_CLOCKEVENTS
>>>>         select HAVE_CLK
>>>>         select PINCTRL
>>>> +       select PM
>>>> +       select PM_GENERIC_DOMAINS
>>>
>>> If you still want to allow ARCH_TEGRA to run without PM, you should
>>> probably change to:
>>>
>>> select PM_GENERIC_DOMAINS if PM
>>
>> Per the changelog this is deliberate. If we allow !PM, then there is a
>> potential that you could probe a device when the power domain is not
>> powered on. I understand that some SoCs turn on all the power-domains
>> when !PM but this will not work for tegra because we don't register the
>> power domain until later in the boot and so we are relying upon probe
>> deferral to defer the probe of devices that use power-domains.
> 
> So what you are saying is that adding the PM domain support, will fix
> some devices to become successfully probed as they were broken before?

Not exactly. There is a legacy tegra_powergate_sequence_power_up() that
has been used to date to get around this. However, by migrating to GENPD
we really need to make PM mandatory, otherwise you could attempt to
probe a device in a power domain that is not powered. In other words,
you are probing blindly. I know some SoCs do this, but that does not
seem very robust.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Ulf Hansson]

On 16 December 2015 at 12:40, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 16/12/15 09:47, Ulf Hansson wrote:
>> On 16 December 2015 at 10:40, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Hi Ulf,
>>>
>>> On 15/12/15 19:54, Ulf Hansson wrote:
>>>> On 4 December 2015 at 15:57, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>>>>> dependent upon a particular power-domain are only probed when that power
>>>>> domain has been powered up, requires that PM is made mandatory for tegra
>>>>> 64-bit devices and so select this option for tegra as well.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> ---
>>>>>  arch/arm64/Kconfig.platforms | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>>> index 9806324fa215..e0b5bd0aff0f 100644
>>>>> --- a/arch/arm64/Kconfig.platforms
>>>>> +++ b/arch/arm64/Kconfig.platforms
>>>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>>>>         select GENERIC_CLOCKEVENTS
>>>>>         select HAVE_CLK
>>>>>         select PINCTRL
>>>>> +       select PM
>>>>> +       select PM_GENERIC_DOMAINS
>>>>
>>>> If you still want to allow ARCH_TEGRA to run without PM, you should
>>>> probably change to:
>>>>
>>>> select PM_GENERIC_DOMAINS if PM
>>>
>>> Per the changelog this is deliberate. If we allow !PM, then there is a
>>> potential that you could probe a device when the power domain is not
>>> powered on. I understand that some SoCs turn on all the power-domains
>>> when !PM but this will not work for tegra because we don't register the
>>> power domain until later in the boot and so we are relying upon probe
>>> deferral to defer the probe of devices that use power-domains.
>>
>> So what you are saying is that adding the PM domain support, will fix
>> some devices to become successfully probed as they were broken before?
>
> Not exactly. There is a legacy tegra_powergate_sequence_power_up() that
> has been used to date to get around this. However, by migrating to GENPD
> we really need to make PM mandatory, otherwise you could attempt to
> probe a device in a power domain that is not powered. In other words,
> you are probing blindly. I know some SoCs do this, but that does not
> seem very robust.

Thank for the clarification. I agree.

You may add my:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
> dependent upon a particular power-domain are only probed when that power
> domain has been powered up, requires that PM is made mandatory for tegra
> 64-bit devices and so select this option for tegra as well.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm64/Kconfig.platforms | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 9806324fa215..e0b5bd0aff0f 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>  	select GENERIC_CLOCKEVENTS
>  	select HAVE_CLK
>  	select PINCTRL
> +	select PM
> +	select PM_GENERIC_DOMAINS
>  	select RESET_CONTROLLER
>  	help
>  	  This enables support for the NVIDIA Tegra SoC family.

This has potential consequences for multi-platform builds, doesn't it?
All of a sudden any combination of builds that includes Tegra won't be
possible to build without PM support.

Adding linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org for visibility.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Arnd Bergmann]

On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
> > Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
> > dependent upon a particular power-domain are only probed when that power
> > domain has been powered up, requires that PM is made mandatory for tegra
> > 64-bit devices and so select this option for tegra as well.
> > 
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> >  arch/arm64/Kconfig.platforms | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> > index 9806324fa215..e0b5bd0aff0f 100644
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -93,6 +93,8 @@ config ARCH_TEGRA
> >       select GENERIC_CLOCKEVENTS
> >       select HAVE_CLK
> >       select PINCTRL
> > +     select PM
> > +     select PM_GENERIC_DOMAINS
> >       select RESET_CONTROLLER
> >       help
> >         This enables support for the NVIDIA Tegra SoC family.
> 
> This has potential consequences for multi-platform builds, doesn't it?
> All of a sudden any combination of builds that includes Tegra won't be
> possible to build without PM support.
> 
> Adding linux-arm-kernel@lists.infradead.org for visibility.
> 
> 

Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
dependencies in the drivers that require it.

	Arnd

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Ulf Hansson]

On 13 January 2016 at 21:43, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
>> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
>> > Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>> > dependent upon a particular power-domain are only probed when that power
>> > domain has been powered up, requires that PM is made mandatory for tegra
>> > 64-bit devices and so select this option for tegra as well.
>> >
>> > Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  arch/arm64/Kconfig.platforms | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> > index 9806324fa215..e0b5bd0aff0f 100644
>> > --- a/arch/arm64/Kconfig.platforms
>> > +++ b/arch/arm64/Kconfig.platforms
>> > @@ -93,6 +93,8 @@ config ARCH_TEGRA
>> >       select GENERIC_CLOCKEVENTS
>> >       select HAVE_CLK
>> >       select PINCTRL
>> > +     select PM
>> > +     select PM_GENERIC_DOMAINS
>> >       select RESET_CONTROLLER
>> >       help
>> >         This enables support for the NVIDIA Tegra SoC family.
>>
>> This has potential consequences for multi-platform builds, doesn't it?
>> All of a sudden any combination of builds that includes Tegra won't be
>> possible to build without PM support.
>>
>> Adding linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org for visibility.
>>
>>
>
> Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
> dependencies in the drivers that require it.
>

The problem with that approach is that if those drivers are cross SoC
drivers. In some cases PM isn't needed and it is.

Of course I don't have the in depth knowledge about the drivers being
used in Tegra which may need PM, perhaps it's not that many?

Anyway, to me it seems like ARCH_TEGRA should depend on PM instead.
Would that work?

Kind regards
Uffe

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Arnd Bergmann]

On Thursday 14 January 2016 09:57:14 Ulf Hansson wrote:
> On 13 January 2016 at 21:43, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
> >> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
> >> > Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
> >> > dependent upon a particular power-domain are only probed when that power
> >> > domain has been powered up, requires that PM is made mandatory for tegra
> >> > 64-bit devices and so select this option for tegra as well.
> >> >
> >> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >> > ---
> >> >  arch/arm64/Kconfig.platforms | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> >> > index 9806324fa215..e0b5bd0aff0f 100644
> >> > --- a/arch/arm64/Kconfig.platforms
> >> > +++ b/arch/arm64/Kconfig.platforms
> >> > @@ -93,6 +93,8 @@ config ARCH_TEGRA
> >> >       select GENERIC_CLOCKEVENTS
> >> >       select HAVE_CLK
> >> >       select PINCTRL
> >> > +     select PM
> >> > +     select PM_GENERIC_DOMAINS
> >> >       select RESET_CONTROLLER
> >> >       help
> >> >         This enables support for the NVIDIA Tegra SoC family.
> >>
> >> This has potential consequences for multi-platform builds, doesn't it?
> >> All of a sudden any combination of builds that includes Tegra won't be
> >> possible to build without PM support.
> >>
> >> Adding linux-arm-kernel@lists.infradead.org for visibility.
> >>
> >>
> >
> > Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
> > dependencies in the drivers that require it.
> >
> 
> The problem with that approach is that if those drivers are cross SoC
> drivers. In some cases PM isn't needed and it is.
> 
> Of course I don't have the in depth knowledge about the drivers being
> used in Tegra which may need PM, perhaps it's not that many?
> 
> Anyway, to me it seems like ARCH_TEGRA should depend on PM instead.
> Would that work?

That seems a little over-restrictive, as it prevents you from
building a tegra kernel even if none of the drivers that rely
on the pm domains are used, but it would work.

I've looked again at how other platforms (on arm32) do it, and
a lot of them use "select PM_GENERIC_DOMAINS if PM", so they don't
automatically enable PM, but they enable the pmdomain code if
PM is already set. No driver really "depends on PM_GENERIC_DOMAINS",
so we shouldn't really start that now or we end up with circular
dependencies in the long run.

	Arnd

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]

On Thu, Jan 14, 2016 at 10:21:06AM +0100, Arnd Bergmann wrote:
> On Thursday 14 January 2016 09:57:14 Ulf Hansson wrote:
> > On 13 January 2016 at 21:43, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > > On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
> > >> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
> > >> > Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
> > >> > dependent upon a particular power-domain are only probed when that power
> > >> > domain has been powered up, requires that PM is made mandatory for tegra
> > >> > 64-bit devices and so select this option for tegra as well.
> > >> >
> > >> > Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >> > ---
> > >> >  arch/arm64/Kconfig.platforms | 2 ++
> > >> >  1 file changed, 2 insertions(+)
> > >> >
> > >> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> > >> > index 9806324fa215..e0b5bd0aff0f 100644
> > >> > --- a/arch/arm64/Kconfig.platforms
> > >> > +++ b/arch/arm64/Kconfig.platforms
> > >> > @@ -93,6 +93,8 @@ config ARCH_TEGRA
> > >> >       select GENERIC_CLOCKEVENTS
> > >> >       select HAVE_CLK
> > >> >       select PINCTRL
> > >> > +     select PM
> > >> > +     select PM_GENERIC_DOMAINS
> > >> >       select RESET_CONTROLLER
> > >> >       help
> > >> >         This enables support for the NVIDIA Tegra SoC family.
> > >>
> > >> This has potential consequences for multi-platform builds, doesn't it?
> > >> All of a sudden any combination of builds that includes Tegra won't be
> > >> possible to build without PM support.
> > >>
> > >> Adding linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org for visibility.
> > >>
> > >>
> > >
> > > Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
> > > dependencies in the drivers that require it.
> > >
> > 
> > The problem with that approach is that if those drivers are cross SoC
> > drivers. In some cases PM isn't needed and it is.
> > 
> > Of course I don't have the in depth knowledge about the drivers being
> > used in Tegra which may need PM, perhaps it's not that many?
> > 
> > Anyway, to me it seems like ARCH_TEGRA should depend on PM instead.
> > Would that work?
> 
> That seems a little over-restrictive, as it prevents you from
> building a tegra kernel even if none of the drivers that rely
> on the pm domains are used, but it would work.
> 
> I've looked again at how other platforms (on arm32) do it, and
> a lot of them use "select PM_GENERIC_DOMAINS if PM", so they don't
> automatically enable PM, but they enable the pmdomain code if
> PM is already set. No driver really "depends on PM_GENERIC_DOMAINS",
> so we shouldn't really start that now or we end up with circular
> dependencies in the long run.

It just occurred to me that none of these options really make much of a
difference. As Jon mentioned once we merge this series a lot of features
on Tegra will start to rely on PM_GENERIC_DOMAINS and hence PM. So if we
do want to build a kernel with a maximum of Tegra features enabled (and
I think a multi_v7_defconfig should include that) we'll end up with a PM
dependency anyway, whether forced via select or implied via depends on.

I'm beginning to wonder if PM really should be an option these days. The
disadvantages of making it optional do outweigh the advantages in my
opinion. I'm not saying that, in general, it's totally useless to build
a kernel that has no PM support, but for the more specific case where
you would want to enable multi-platform support I don't think there's
much practical advantage in allowing !PM. One of the most common build
warnings are triggered because of this option. Also multi-platform
kernels are really big already, so much so that I doubt it would make a
significant difference if we unconditionally built PM support. Also the
chances are that we'll be seeing more and more SoCs support PM and rely
on it, much like Tegra would with the addition of this series.

I imagine that we could save ourselves a lot of headaches by simply
enabling PM by default, whether that be via the PM Kconfig option or by
selecting it from ARCH_TEGRA and any other architectures that may come
to rely on it. Doing so would also reduce the amount of test coverage
that we need to do, both at compile- and runtime.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Arnd Bergmann]

On Thursday 14 January 2016 11:29:24 Thierry Reding wrote:
> 
> It just occurred to me that none of these options really make much of a
> difference. As Jon mentioned once we merge this series a lot of features
> on Tegra will start to rely on PM_GENERIC_DOMAINS and hence PM. So if we
> do want to build a kernel with a maximum of Tegra features enabled (and
> I think a multi_v7_defconfig should include that) we'll end up with a PM
> dependency anyway, whether forced via select or implied via depends on.
> 
> I'm beginning to wonder if PM really should be an option these days. The
> disadvantages of making it optional do outweigh the advantages in my
> opinion. I'm not saying that, in general, it's totally useless to build
> a kernel that has no PM support, but for the more specific case where
> you would want to enable multi-platform support I don't think there's
> much practical advantage in allowing !PM. One of the most common build
> warnings are triggered because of this option. Also multi-platform
> kernels are really big already, so much so that I doubt it would make a
> significant difference if we unconditionally built PM support. Also the
> chances are that we'll be seeing more and more SoCs support PM and rely
> on it, much like Tegra would with the addition of this series.
> 
> I imagine that we could save ourselves a lot of headaches by simply
> enabling PM by default, whether that be via the PM Kconfig option or by
> selecting it from ARCH_TEGRA and any other architectures that may come
> to rely on it. Doing so would also reduce the amount of test coverage
> that we need to do, both at compile- and runtime.

I think this needs some investigation. As a general policy, we should
not grow the kernel image size when moving from a traditional ARM
platform to an ARCH_MULTIPLATFORM one.

This is somewhat contradicted by how we already require CONFIG_OF
to be set for multiplatform kernels, and that adds around 80kb
to the image size.

Looking at just the defconfig files, these are the ones that currently
do not set CONFIG_PM:

build/acs5k_defconfig/.config:# CONFIG_PM is not set
build/acs5k_tiny_defconfig/.config:# CONFIG_PM is not set
build/axm55xx_defconfig/.config:# CONFIG_PM is not set
build/bcm2835_defconfig/.config:# CONFIG_PM is not set
build/clps711x_defconfig/.config:# CONFIG_PM is not set
build/ebsa110_defconfig/.config:# CONFIG_PM is not set
build/footbridge_defconfig/.config:# CONFIG_PM is not set
build/ks8695_defconfig/.config:# CONFIG_PM is not set
build/netwinder_defconfig/.config:# CONFIG_PM is not set
build/rpc_defconfig/.config:# CONFIG_PM is not set
build/u300_defconfig/.config:# CONFIG_PM is not set
build/vf610m4_defconfig/.config:# CONFIG_PM is not set

The only ones among these are are actually multiplatform are axm55xx,
bcm2835, and u300. I see no downsides of force-enabling PM for
any of those, so we could decide to 'select PM' from
CONFIG_ARCH_MULTIPLATFORM.

The one usecase where we may want to have a modern machine without
CONFIG_PM is a minimal MACH_VIRT kernel for running in a virtual
machine or QEMU with minimal memory requirements, e.g. trying to
squeeze a large number of guests on a single host system.

	Arnd

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4598 bytes --]

On Thu, Jan 14, 2016 at 12:11:39PM +0100, Arnd Bergmann wrote:
> On Thursday 14 January 2016 11:29:24 Thierry Reding wrote:
> > 
> > It just occurred to me that none of these options really make much of a
> > difference. As Jon mentioned once we merge this series a lot of features
> > on Tegra will start to rely on PM_GENERIC_DOMAINS and hence PM. So if we
> > do want to build a kernel with a maximum of Tegra features enabled (and
> > I think a multi_v7_defconfig should include that) we'll end up with a PM
> > dependency anyway, whether forced via select or implied via depends on.
> > 
> > I'm beginning to wonder if PM really should be an option these days. The
> > disadvantages of making it optional do outweigh the advantages in my
> > opinion. I'm not saying that, in general, it's totally useless to build
> > a kernel that has no PM support, but for the more specific case where
> > you would want to enable multi-platform support I don't think there's
> > much practical advantage in allowing !PM. One of the most common build
> > warnings are triggered because of this option. Also multi-platform
> > kernels are really big already, so much so that I doubt it would make a
> > significant difference if we unconditionally built PM support. Also the
> > chances are that we'll be seeing more and more SoCs support PM and rely
> > on it, much like Tegra would with the addition of this series.
> > 
> > I imagine that we could save ourselves a lot of headaches by simply
> > enabling PM by default, whether that be via the PM Kconfig option or by
> > selecting it from ARCH_TEGRA and any other architectures that may come
> > to rely on it. Doing so would also reduce the amount of test coverage
> > that we need to do, both at compile- and runtime.
> 
> I think this needs some investigation. As a general policy, we should
> not grow the kernel image size when moving from a traditional ARM
> platform to an ARCH_MULTIPLATFORM one.

If we make ARCH_TEGRA select PM, then moving to a multi-platform kernel
isn't automatically going to increase the image size. The image size is
only going to increase if you select ARCH_TEGRA to be part of the multi
platform image.

> This is somewhat contradicted by how we already require CONFIG_OF
> to be set for multiplatform kernels, and that adds around 80kb
> to the image size.

Yeah, there's also a fair amount of per-SoC code that can't be built as
a module and which will be included in multi-platform images when the
corresponding ARCH_* symbol is enabled. But I think that's inevitable
given the purpose of multi-platform images.

> Looking at just the defconfig files, these are the ones that currently
> do not set CONFIG_PM:
> 
> build/acs5k_defconfig/.config:# CONFIG_PM is not set
> build/acs5k_tiny_defconfig/.config:# CONFIG_PM is not set
> build/axm55xx_defconfig/.config:# CONFIG_PM is not set
> build/bcm2835_defconfig/.config:# CONFIG_PM is not set
> build/clps711x_defconfig/.config:# CONFIG_PM is not set
> build/ebsa110_defconfig/.config:# CONFIG_PM is not set
> build/footbridge_defconfig/.config:# CONFIG_PM is not set
> build/ks8695_defconfig/.config:# CONFIG_PM is not set
> build/netwinder_defconfig/.config:# CONFIG_PM is not set
> build/rpc_defconfig/.config:# CONFIG_PM is not set
> build/u300_defconfig/.config:# CONFIG_PM is not set
> build/vf610m4_defconfig/.config:# CONFIG_PM is not set
> 
> The only ones among these are are actually multiplatform are axm55xx,
> bcm2835, and u300. I see no downsides of force-enabling PM for
> any of those, so we could decide to 'select PM' from
> CONFIG_ARCH_MULTIPLATFORM.

ARCH_MULTIPLATFORM selecting PM would include PM unconditionally, even
if none of the selected platforms require it. In my opinion an explicit
select from platforms that require PM would be cleaner. It could be that
once we start doing that for a single platform others might follow. When
this becomes common place it might be worth moving it up a level, but I
think explicit dependencies would be better for starters.

> The one usecase where we may want to have a modern machine without
> CONFIG_PM is a minimal MACH_VIRT kernel for running in a virtual
> machine or QEMU with minimal memory requirements, e.g. trying to
> squeeze a large number of guests on a single host system.

Right, but those won't be multi-platform kernels, right? If you know
exactly that the kernel will run in a virtual machine and that you want
to run lots of virtual machines, you probably want to build a custom
kernel.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Kevin Hilman]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Thu, Jan 14, 2016 at 12:11:39PM +0100, Arnd Bergmann wrote:
>> On Thursday 14 January 2016 11:29:24 Thierry Reding wrote:
>> > 
>> > It just occurred to me that none of these options really make much of a
>> > difference. As Jon mentioned once we merge this series a lot of features
>> > on Tegra will start to rely on PM_GENERIC_DOMAINS and hence PM. So if we
>> > do want to build a kernel with a maximum of Tegra features enabled (and
>> > I think a multi_v7_defconfig should include that) we'll end up with a PM
>> > dependency anyway, whether forced via select or implied via depends on.
>> > 
>> > I'm beginning to wonder if PM really should be an option these days. The
>> > disadvantages of making it optional do outweigh the advantages in my
>> > opinion. I'm not saying that, in general, it's totally useless to build
>> > a kernel that has no PM support, but for the more specific case where
>> > you would want to enable multi-platform support I don't think there's
>> > much practical advantage in allowing !PM. One of the most common build
>> > warnings are triggered because of this option. Also multi-platform
>> > kernels are really big already, so much so that I doubt it would make a
>> > significant difference if we unconditionally built PM support. Also the
>> > chances are that we'll be seeing more and more SoCs support PM and rely
>> > on it, much like Tegra would with the addition of this series.
>> > 
>> > I imagine that we could save ourselves a lot of headaches by simply
>> > enabling PM by default, whether that be via the PM Kconfig option or by
>> > selecting it from ARCH_TEGRA and any other architectures that may come
>> > to rely on it. Doing so would also reduce the amount of test coverage
>> > that we need to do, both at compile- and runtime.
>> 
>> I think this needs some investigation. As a general policy, we should
>> not grow the kernel image size when moving from a traditional ARM
>> platform to an ARCH_MULTIPLATFORM one.
>
> If we make ARCH_TEGRA select PM, then moving to a multi-platform kernel
> isn't automatically going to increase the image size. The image size is
> only going to increase if you select ARCH_TEGRA to be part of the multi
> platform image.
>
>> This is somewhat contradicted by how we already require CONFIG_OF
>> to be set for multiplatform kernels, and that adds around 80kb
>> to the image size.
>
> Yeah, there's also a fair amount of per-SoC code that can't be built as
> a module and which will be included in multi-platform images when the
> corresponding ARCH_* symbol is enabled. But I think that's inevitable
> given the purpose of multi-platform images.
>
>> Looking at just the defconfig files, these are the ones that currently
>> do not set CONFIG_PM:
>> 
>> build/acs5k_defconfig/.config:# CONFIG_PM is not set
>> build/acs5k_tiny_defconfig/.config:# CONFIG_PM is not set
>> build/axm55xx_defconfig/.config:# CONFIG_PM is not set
>> build/bcm2835_defconfig/.config:# CONFIG_PM is not set
>> build/clps711x_defconfig/.config:# CONFIG_PM is not set
>> build/ebsa110_defconfig/.config:# CONFIG_PM is not set
>> build/footbridge_defconfig/.config:# CONFIG_PM is not set
>> build/ks8695_defconfig/.config:# CONFIG_PM is not set
>> build/netwinder_defconfig/.config:# CONFIG_PM is not set
>> build/rpc_defconfig/.config:# CONFIG_PM is not set
>> build/u300_defconfig/.config:# CONFIG_PM is not set
>> build/vf610m4_defconfig/.config:# CONFIG_PM is not set
>> 
>> The only ones among these are are actually multiplatform are axm55xx,
>> bcm2835, and u300. I see no downsides of force-enabling PM for
>> any of those, so we could decide to 'select PM' from
>> CONFIG_ARCH_MULTIPLATFORM.
>
> ARCH_MULTIPLATFORM selecting PM would include PM unconditionally, even
> if none of the selected platforms require it. In my opinion an explicit
> select from platforms that require PM would be cleaner.

I agree.

Doing it this way also points you exactly at which arch(es) needs to be
disabled if you want to build a !PM multi-plaform kernel.

> It could be that once we start doing that for a single platform others
> might follow.

I suspect so as well.  The main reason we're not there already is that
full PM support for most platforms remains out of tree.

> When this becomes common place it might be worth moving it up a level,
> but I think explicit dependencies would be better for starters.

+1

Kevin

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

On 14/01/16 09:21, Arnd Bergmann wrote:
> On Thursday 14 January 2016 09:57:14 Ulf Hansson wrote:
>> On 13 January 2016 at 21:43, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
>>>> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
>>>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>>>>> dependent upon a particular power-domain are only probed when that power
>>>>> domain has been powered up, requires that PM is made mandatory for tegra
>>>>> 64-bit devices and so select this option for tegra as well.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> ---
>>>>>  arch/arm64/Kconfig.platforms | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>>> index 9806324fa215..e0b5bd0aff0f 100644
>>>>> --- a/arch/arm64/Kconfig.platforms
>>>>> +++ b/arch/arm64/Kconfig.platforms
>>>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>>>>       select GENERIC_CLOCKEVENTS
>>>>>       select HAVE_CLK
>>>>>       select PINCTRL
>>>>> +     select PM
>>>>> +     select PM_GENERIC_DOMAINS
>>>>>       select RESET_CONTROLLER
>>>>>       help
>>>>>         This enables support for the NVIDIA Tegra SoC family.
>>>>
>>>> This has potential consequences for multi-platform builds, doesn't it?
>>>> All of a sudden any combination of builds that includes Tegra won't be
>>>> possible to build without PM support.
>>>>
>>>> Adding linux-arm-kernel@lists.infradead.org for visibility.
>>>>
>>>>
>>>
>>> Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
>>> dependencies in the drivers that require it.
>>>
>>
>> The problem with that approach is that if those drivers are cross SoC
>> drivers. In some cases PM isn't needed and it is.
>>
>> Of course I don't have the in depth knowledge about the drivers being
>> used in Tegra which may need PM, perhaps it's not that many?
>>
>> Anyway, to me it seems like ARCH_TEGRA should depend on PM instead.
>> Would that work?
> 
> That seems a little over-restrictive, as it prevents you from
> building a tegra kernel even if none of the drivers that rely
> on the pm domains are used, but it would work.
> 
> I've looked again at how other platforms (on arm32) do it, and
> a lot of them use "select PM_GENERIC_DOMAINS if PM", so they don't
> automatically enable PM, but they enable the pmdomain code if
> PM is already set. No driver really "depends on PM_GENERIC_DOMAINS",
> so we shouldn't really start that now or we end up with circular
> dependencies in the long run.

What I am not a fan of in the current gen-pd implementation, is if we
have !PM but the platform has power-domains, then there is no way to
determine if a device within a power-domain can be probed safely. Some
arm platforms force all the power-domains on during early init in the
case of !PM. IMO this is still not ideal, because if a power-domain
failed to turn on during early init, then you should probably call
BUG(). Ideally the kernel should be able to boot and only probe the
devices you know that can be probed safely.

So for platforms have use PM_GENERIC_DOMAINS, I think really they should
select PM and not "select PM_GENERIC_DOMAINS if PM". IMO, "select
PM_GENERIC_DOMAINS if PM" seems fragile.

Cheers
Jon

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

Hi Arnd, Ulf,

On 14/01/16 17:16, Jon Hunter wrote:
> 
> On 14/01/16 09:21, Arnd Bergmann wrote:
>> On Thursday 14 January 2016 09:57:14 Ulf Hansson wrote:
>>> On 13 January 2016 at 21:43, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wednesday 13 January 2016 18:03:24 Thierry Reding wrote:
>>>>> On Fri, Dec 04, 2015 at 02:57:17PM +0000, Jon Hunter wrote:
>>>>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
>>>>>> dependent upon a particular power-domain are only probed when that power
>>>>>> domain has been powered up, requires that PM is made mandatory for tegra
>>>>>> 64-bit devices and so select this option for tegra as well.
>>>>>>
>>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>> ---
>>>>>>  arch/arm64/Kconfig.platforms | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>>>> index 9806324fa215..e0b5bd0aff0f 100644
>>>>>> --- a/arch/arm64/Kconfig.platforms
>>>>>> +++ b/arch/arm64/Kconfig.platforms
>>>>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA
>>>>>>       select GENERIC_CLOCKEVENTS
>>>>>>       select HAVE_CLK
>>>>>>       select PINCTRL
>>>>>> +     select PM
>>>>>> +     select PM_GENERIC_DOMAINS
>>>>>>       select RESET_CONTROLLER
>>>>>>       help
>>>>>>         This enables support for the NVIDIA Tegra SoC family.
>>>>>
>>>>> This has potential consequences for multi-platform builds, doesn't it?
>>>>> All of a sudden any combination of builds that includes Tegra won't be
>>>>> possible to build without PM support.
>>>>>
>>>>> Adding linux-arm-kernel@lists.infradead.org for visibility.
>>>>>
>>>>>
>>>>
>>>> Agreed, it would be better to add 'depends on PM_GENERIC_DOMAINS'
>>>> dependencies in the drivers that require it.
>>>>
>>>
>>> The problem with that approach is that if those drivers are cross SoC
>>> drivers. In some cases PM isn't needed and it is.
>>>
>>> Of course I don't have the in depth knowledge about the drivers being
>>> used in Tegra which may need PM, perhaps it's not that many?
>>>
>>> Anyway, to me it seems like ARCH_TEGRA should depend on PM instead.
>>> Would that work?
>>
>> That seems a little over-restrictive, as it prevents you from
>> building a tegra kernel even if none of the drivers that rely
>> on the pm domains are used, but it would work.
>>
>> I've looked again at how other platforms (on arm32) do it, and
>> a lot of them use "select PM_GENERIC_DOMAINS if PM", so they don't
>> automatically enable PM, but they enable the pmdomain code if
>> PM is already set. No driver really "depends on PM_GENERIC_DOMAINS",
>> so we shouldn't really start that now or we end up with circular
>> dependencies in the long run.
> 
> What I am not a fan of in the current gen-pd implementation, is if we
> have !PM but the platform has power-domains, then there is no way to
> determine if a device within a power-domain can be probed safely. Some
> arm platforms force all the power-domains on during early init in the
> case of !PM. IMO this is still not ideal, because if a power-domain
> failed to turn on during early init, then you should probably call
> BUG(). Ideally the kernel should be able to boot and only probe the
> devices you know that can be probed safely.
> 
> So for platforms have use PM_GENERIC_DOMAINS, I think really they should
> select PM and not "select PM_GENERIC_DOMAINS if PM". IMO, "select
> PM_GENERIC_DOMAINS if PM" seems fragile.

Any more thoughts on this?

I have been discussing with Thierry and we think that selecting PM for
tegra still makes the most sense. The question is, is this ok for
multi-configs?

The only other suggestion/thought I have is to allow
PM_GENERIC_DOMAINS_OF to be selected independently of PM so that we can
have minimal support for PM domains that allows you to register PM
domains with the kernel and their current state, but does not allow you
to control them, etc. This way tegra could always select
PM_GENERIC_DOMAINS_OF regardless of PM, and we would be able to
determine if we can probe a device safely.

I am not sure that Rafael is too keen on this approach but that is the
only alternative I have come up with.

I have a rough outline of a patch for this here [0] FWIW.

Cheers
Jon

[0] https://github.com/jonhunter/linux/commits/gpd

Re: [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS

[Ulf Hansson]

[...]

>
> Any more thoughts on this?
>
> I have been discussing with Thierry and we think that selecting PM for
> tegra still makes the most sense. The question is, is this ok for
> multi-configs?

Yes, this is too me the best option.

Additionally, for those tegra SoCs/platforms that needs
PM_GENERIC_DOMAINS, it can safely select that as well.

>
> The only other suggestion/thought I have is to allow
> PM_GENERIC_DOMAINS_OF to be selected independently of PM so that we can
> have minimal support for PM domains that allows you to register PM
> domains with the kernel and their current state, but does not allow you
> to control them, etc. This way tegra could always select
> PM_GENERIC_DOMAINS_OF regardless of PM, and we would be able to
> determine if we can probe a device safely.
>
> I am not sure that Rafael is too keen on this approach but that is the
> only alternative I have come up with.

I think Rafael already made his point around this approach, which is a
clear no. And I fully agree with it.

Kind regards
Uffe