[RFC PATCH 0/3] soc/tegra: Turn on XUSB partitions

[RFC PATCH 0/3] soc/tegra: Turn on XUSB partitions

[Jon Hunter]

The Tegra XHCI driver currently assumes the XUSB power partitions have
been enabled by the bootloader. This is not for the Tegra210 Smaug board
and so cause the kernel to hang when enabling XHCI support. Although the
XHCI driver itself needs to manage these partitions, for now enable the
partitions if the XHCI driver is enabled.

In order to do this I have made a fundamental change to the PMC driver
to initialise the power partitions during early init. A benefit of this
is that if CONFIG_PM_GENERIC_DOMAINS is not enabled, then we can simply
turn on the partitions early before any devices are probed.

This is based upon the PMC fixes series [0].

[0] http://marc.info/?l=linux-tegra&m=146711078013182&w=2

Jon Hunter (3):
  soc/tegra: pmc: Initialise power partitions early
  soc/tegra: pmc: Enable XUSB partitions on boot
  arm64: tegra210: Add XUSB powergates

 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 +++++++++++++++++
 drivers/soc/tegra/pmc.c                  | 45 ++++++++++++++++++++++++--------
 2 files changed, 58 insertions(+), 11 deletions(-)

-- 
2.1.4

[RFC PATCH 1/3] soc/tegra: pmc: Initialise power partitions early

[Jon Hunter]

If CONFIG_PM_GENERIC_DOMAINS is not enabled, then power partitions
associated with a device will not be enabled automatically by the PM
core when the device is in use. To avoid situations where a device in
a power partition is to be used but the partition is not enabled,
initialise the power partitions for Tegra early in the boot process and
if CONFIG_PM_GENERIC_DOMAINS is not enabled, then power on all
partitions defined in the device-tree blob.

Note that if CONFIG_PM_GENERIC_DOMAINS is not enabled, after the
partitions are turned on, the clocks and resets used as part of the
sequence for turning on the partition are released again as they are no
longer needed by the PMC driver. Another benefit of this is that this
avoids any issues of sharing resets between the PMC driver and other
device drivers that may wish to independently control a particular
reset.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 1f702538f8ec..64678ff2173e 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -788,7 +788,7 @@ error:
 static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 {
 	struct tegra_powergate *pg;
-	bool off;
+	bool off, err = true;
 	int id;
 
 	pg = kzalloc(sizeof(*pg), GFP_KERNEL);
@@ -819,6 +819,9 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 	if (tegra_powergate_of_get_resets(pg, np, off))
 		goto remove_clks;
 
+	if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
+		goto power_on_cleanup;
+
 	pm_genpd_init(&pg->genpd, NULL, off);
 
 	if (of_genpd_add_provider_simple(np, &pg->genpd))
@@ -828,6 +831,11 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 
 	return;
 
+power_on_cleanup:
+	if (off)
+		WARN_ON(tegra_powergate_power_up(pg, true));
+	err = false;
+
 remove_resets:
 	while (pg->num_resets--)
 		reset_control_put(pg->resets[pg->num_resets]);
@@ -845,14 +853,23 @@ free_mem:
 	kfree(pg);
 
 error:
-	dev_err(pmc->dev, "failed to create power domain for %s\n", np->name);
+	if (err)
+		dev_err(pmc->dev, "failed to configure partition %s\n",
+			np->name);
 }
 
-static void tegra_powergate_init(struct tegra_pmc *pmc)
+static void tegra_powergate_init(struct tegra_pmc *pmc,
+				 struct device_node *parent)
 {
 	struct device_node *np, *child;
+	unsigned int i;
 
-	np = of_get_child_by_name(pmc->dev->of_node, "powergates");
+	/* Create a bit-map of the available and valid partitions */
+	for (i = 0; i < pmc->soc->num_powergates; i++)
+		if (pmc->soc->powergates[i])
+			set_bit(i, pmc->powergates_available);
+
+	np = of_get_child_by_name(parent, "powergates");
 	if (!np)
 		return;
 
@@ -1273,8 +1290,6 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	tegra_powergate_init(pmc);
-
 	mutex_lock(&pmc->powergates_lock);
 	iounmap(pmc->base);
 	pmc->base = base;
@@ -1508,7 +1523,6 @@ static int __init tegra_pmc_early_init(void)
 	const struct of_device_id *match;
 	struct device_node *np;
 	struct resource regs;
-	unsigned int i;
 	bool invert;
 	u32 value;
 
@@ -1563,10 +1577,7 @@ static int __init tegra_pmc_early_init(void)
 	if (np) {
 		pmc->soc = match->data;
 
-		/* Create a bit-map of the available and valid partitions */
-		for (i = 0; i < pmc->soc->num_powergates; i++)
-			if (pmc->soc->powergates[i])
-				set_bit(i, pmc->powergates_available);
+		tegra_powergate_init(pmc, np);
 
 		/*
 		 * Invert the interrupt polarity if a PMC device tree node
-- 
2.1.4

[RFC PATCH 2/3] soc/tegra: pmc: Enable XUSB partitions on boot

[Jon Hunter]

The Tegra XHCI driver does not currently manage the Tegra XUSB power
partitions and so it these partitions have not been enabled by the
bootloader then the system will crash when probing the XHCI device.

While proper support for managing the power partitions is being
developed to the XHCI driver for Tegra, for now power on all the XUSB
partitions for USB host and super-speed on boot if the XHCI driver is
enabled.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 64678ff2173e..afaa5891c8f5 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -822,6 +822,18 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 	if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
 		goto power_on_cleanup;
 
+	/*
+	 * FIXME: If XHCI is enabled for Tegra, then power-up the XUSB
+	 * host and super-speed partitions. Once the XHCI driver
+	 * manages the partitions itself this code can be removed. Note
+	 * that we don't register these partitions with the genpd core
+	 * to avoid it from powering down the partitions as they appear
+	 * to be unused.
+	 */
+	if (IS_ENABLED(CONFIG_USB_XHCI_TEGRA) &&
+	    (id == TEGRA_POWERGATE_XUSBA || id == TEGRA_POWERGATE_XUSBC))
+		goto power_on_cleanup;
+
 	pm_genpd_init(&pg->genpd, NULL, off);
 
 	if (of_genpd_add_provider_simple(np, &pg->genpd))
-- 
2.1.4

[RFC PATCH 3/3] arm64: tegra210: Add XUSB powergates

[Jon Hunter]

The Tegra210 XUSB subsystem has 3 power partitions which are XUSBA
(super-speed logic), XUSBB (USB device logic) and XUSBC (USB host
logic). Populate the device-tree nodes for these XUSB partitions.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index 65b829b762bb..efb0fd98b789 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -670,6 +670,30 @@
 					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
 				#power-domain-cells = <0>;
 			};
+
+			pd_xusbss: xusba {
+				clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
+				clock-names = "xusb_ss";
+				resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
+				reset-names = "xusb_ss";
+				#power-domain-cells = <0>;
+			};
+
+			pd_xusbdev: xusbb {
+				clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>;
+				clock-names = "xusb_dev";
+				resets = <&tegra_car 95>;
+				reset-names = "xusb_dev";
+				#power-domain-cells = <0>;
+			};
+
+			pd_xusbhost: xusbc {
+				clocks = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
+				clock-names = "xusb_host";
+				resets = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
+				reset-names = "xusb_host";
+				#power-domain-cells = <0>;
+			};
 		};
 	};
 
-- 
2.1.4

Re: [RFC PATCH 3/3] arm64: tegra210: Add XUSB powergates

[Jon Hunter]

On 28/06/16 12:20, Jon Hunter wrote:
> The Tegra210 XUSB subsystem has 3 power partitions which are XUSBA
> (super-speed logic), XUSBB (USB device logic) and XUSBC (USB host
> logic). Populate the device-tree nodes for these XUSB partitions.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index 65b829b762bb..efb0fd98b789 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -670,6 +670,30 @@
>  					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
>  				#power-domain-cells = <0>;
>  			};
> +
> +			pd_xusbss: xusba {
> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> +				clock-names = "xusb_ss";
> +				resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> +				reset-names = "xusb_ss";
> +				#power-domain-cells = <0>;
> +			};
> +
> +			pd_xusbdev: xusbb {
> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>;
> +				clock-names = "xusb_dev";
> +				resets = <&tegra_car 95>;
> +				reset-names = "xusb_dev";
> +				#power-domain-cells = <0>;
> +			};
> +
> +			pd_xusbhost: xusbc {
> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> +				clock-names = "xusb_host";
> +				resets = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> +				reset-names = "xusb_host";
> +				#power-domain-cells = <0>;
> +			};
>  		};
>  	};

The 'clock-names' and 'reset-names' nodes are not used/required and so I
will remove these.

Jon

-- 
nvpublic

Re: [RFC PATCH 3/3] arm64: tegra210: Add XUSB powergates

[Thierry Reding]

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

On Wed, Jun 29, 2016 at 04:30:08PM +0100, Jon Hunter wrote:
> 
> On 28/06/16 12:20, Jon Hunter wrote:
> > The Tegra210 XUSB subsystem has 3 power partitions which are XUSBA
> > (super-speed logic), XUSBB (USB device logic) and XUSBC (USB host
> > logic). Populate the device-tree nodes for these XUSB partitions.
> > 
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> > index 65b829b762bb..efb0fd98b789 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> > +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> > @@ -670,6 +670,30 @@
> >  					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
> >  				#power-domain-cells = <0>;
> >  			};
> > +
> > +			pd_xusbss: xusba {
> > +				clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> > +				clock-names = "xusb_ss";
> > +				resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> > +				reset-names = "xusb_ss";
> > +				#power-domain-cells = <0>;
> > +			};
> > +
> > +			pd_xusbdev: xusbb {
> > +				clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>;
> > +				clock-names = "xusb_dev";
> > +				resets = <&tegra_car 95>;
> > +				reset-names = "xusb_dev";
> > +				#power-domain-cells = <0>;
> > +			};
> > +
> > +			pd_xusbhost: xusbc {
> > +				clocks = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> > +				clock-names = "xusb_host";
> > +				resets = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> > +				reset-names = "xusb_host";
> > +				#power-domain-cells = <0>;
> > +			};
> >  		};
> >  	};
> 
> The 'clock-names' and 'reset-names' nodes are not used/required and so I
> will remove these.

Please keep them and make use of the names. We used to not do this in
the past, and then things became tricky to describe in the DT bindings
in order to keep backwards-compatibility.

Though perhaps you're not using them because they are found by index? In
that case I think it might still be useful to have them for consistency.

If you keep them, you might want to turn the _ into -.

Thierry

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

Re: [RFC PATCH 3/3] arm64: tegra210: Add XUSB powergates

[Jon Hunter]

On 29/06/16 16:56, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jun 29, 2016 at 04:30:08PM +0100, Jon Hunter wrote:
>>
>> On 28/06/16 12:20, Jon Hunter wrote:
>>> The Tegra210 XUSB subsystem has 3 power partitions which are XUSBA
>>> (super-speed logic), XUSBB (USB device logic) and XUSBC (USB host
>>> logic). Populate the device-tree nodes for these XUSB partitions.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
>>> index 65b829b762bb..efb0fd98b789 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
>>> @@ -670,6 +670,30 @@
>>>  					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
>>>  				#power-domain-cells = <0>;
>>>  			};
>>> +
>>> +			pd_xusbss: xusba {
>>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
>>> +				clock-names = "xusb_ss";
>>> +				resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
>>> +				reset-names = "xusb_ss";
>>> +				#power-domain-cells = <0>;
>>> +			};
>>> +
>>> +			pd_xusbdev: xusbb {
>>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>;
>>> +				clock-names = "xusb_dev";
>>> +				resets = <&tegra_car 95>;
>>> +				reset-names = "xusb_dev";
>>> +				#power-domain-cells = <0>;
>>> +			};
>>> +
>>> +			pd_xusbhost: xusbc {
>>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
>>> +				clock-names = "xusb_host";
>>> +				resets = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
>>> +				reset-names = "xusb_host";
>>> +				#power-domain-cells = <0>;
>>> +			};
>>>  		};
>>>  	};
>>
>> The 'clock-names' and 'reset-names' nodes are not used/required and so I
>> will remove these.
> 
> Please keep them and make use of the names. We used to not do this in
> the past, and then things became tricky to describe in the DT bindings
> in order to keep backwards-compatibility.

Unfortunately, in order to use them, we would need to keep a list of all
the clock and reset names in the PMC driver which would be huge.

> Though perhaps you're not using them because they are found by index? In
> that case I think it might still be useful to have them for consistency.

Right, we just iterate over the number of the clocks and resets found
when we initialise the powergate.

> If you keep them, you might want to turn the _ into -.

I can keep them, however, in other patches I have sent out, for example
the SOR powergate (part of the DPAUX series) and Audio powergate, they
do not have them. So I thought I would remove them here to be
consistent. However, we could add them for these other powergates as well.

Cheers
Jon

-- 
nvpublic

Re: [RFC PATCH 1/3] soc/tegra: pmc: Initialise power partitions early

[Thierry Reding]

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

On Tue, Jun 28, 2016 at 12:20:42PM +0100, Jon Hunter wrote:
> If CONFIG_PM_GENERIC_DOMAINS is not enabled, then power partitions
> associated with a device will not be enabled automatically by the PM
> core when the device is in use. To avoid situations where a device in
> a power partition is to be used but the partition is not enabled,
> initialise the power partitions for Tegra early in the boot process and
> if CONFIG_PM_GENERIC_DOMAINS is not enabled, then power on all
> partitions defined in the device-tree blob.
> 
> Note that if CONFIG_PM_GENERIC_DOMAINS is not enabled, after the
> partitions are turned on, the clocks and resets used as part of the
> sequence for turning on the partition are released again as they are no
> longer needed by the PMC driver. Another benefit of this is that this
> avoids any issues of sharing resets between the PMC driver and other
> device drivers that may wish to independently control a particular
> reset.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 1f702538f8ec..64678ff2173e 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -788,7 +788,7 @@ error:
>  static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>  {
>  	struct tegra_powergate *pg;
> -	bool off;
> +	bool off, err = true;
>  	int id;
>  
>  	pg = kzalloc(sizeof(*pg), GFP_KERNEL);
> @@ -819,6 +819,9 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>  	if (tegra_powergate_of_get_resets(pg, np, off))
>  		goto remove_clks;
>  
> +	if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
> +		goto power_on_cleanup;
> +
>  	pm_genpd_init(&pg->genpd, NULL, off);
>  
>  	if (of_genpd_add_provider_simple(np, &pg->genpd))
> @@ -828,6 +831,11 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>  
>  	return;
>  
> +power_on_cleanup:
> +	if (off)
> +		WARN_ON(tegra_powergate_power_up(pg, true));
> +	err = false;
> +
>  remove_resets:
>  	while (pg->num_resets--)
>  		reset_control_put(pg->resets[pg->num_resets]);
> @@ -845,14 +853,23 @@ free_mem:
>  	kfree(pg);
>  
>  error:
> -	dev_err(pmc->dev, "failed to create power domain for %s\n", np->name);
> +	if (err)
> +		dev_err(pmc->dev, "failed to configure partition %s\n",
> +			np->name);

This is beginning to look very spaghetti-like. Do we really need the
error message here? Could we instead add more explicit error messages
before the gotos above?

Thierry

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

Re: [RFC PATCH 1/3] soc/tegra: pmc: Initialise power partitions early

[Jon Hunter]

On 30/06/16 11:17, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 28, 2016 at 12:20:42PM +0100, Jon Hunter wrote:
>> If CONFIG_PM_GENERIC_DOMAINS is not enabled, then power partitions
>> associated with a device will not be enabled automatically by the PM
>> core when the device is in use. To avoid situations where a device in
>> a power partition is to be used but the partition is not enabled,
>> initialise the power partitions for Tegra early in the boot process and
>> if CONFIG_PM_GENERIC_DOMAINS is not enabled, then power on all
>> partitions defined in the device-tree blob.
>>
>> Note that if CONFIG_PM_GENERIC_DOMAINS is not enabled, after the
>> partitions are turned on, the clocks and resets used as part of the
>> sequence for turning on the partition are released again as they are no
>> longer needed by the PMC driver. Another benefit of this is that this
>> avoids any issues of sharing resets between the PMC driver and other
>> device drivers that may wish to independently control a particular
>> reset.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 33 ++++++++++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 1f702538f8ec..64678ff2173e 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -788,7 +788,7 @@ error:
>>  static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>>  {
>>  	struct tegra_powergate *pg;
>> -	bool off;
>> +	bool off, err = true;
>>  	int id;
>>  
>>  	pg = kzalloc(sizeof(*pg), GFP_KERNEL);
>> @@ -819,6 +819,9 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>>  	if (tegra_powergate_of_get_resets(pg, np, off))
>>  		goto remove_clks;
>>  
>> +	if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
>> +		goto power_on_cleanup;
>> +
>>  	pm_genpd_init(&pg->genpd, NULL, off);
>>  
>>  	if (of_genpd_add_provider_simple(np, &pg->genpd))
>> @@ -828,6 +831,11 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>>  
>>  	return;
>>  
>> +power_on_cleanup:
>> +	if (off)
>> +		WARN_ON(tegra_powergate_power_up(pg, true));
>> +	err = false;
>> +
>>  remove_resets:
>>  	while (pg->num_resets--)
>>  		reset_control_put(pg->resets[pg->num_resets]);
>> @@ -845,14 +853,23 @@ free_mem:
>>  	kfree(pg);
>>  
>>  error:
>> -	dev_err(pmc->dev, "failed to create power domain for %s\n", np->name);
>> +	if (err)
>> +		dev_err(pmc->dev, "failed to configure partition %s\n",
>> +			np->name);
> 
> This is beginning to look very spaghetti-like. Do we really need the
> error message here? Could we instead add more explicit error messages
> before the gotos above?

Yes I can't say I loved it either. Ok, yes will add more specific error
messages above gotos instead.

Cheers
Jon

-- 
nvpublic

Re: [RFC PATCH 3/3] arm64: tegra210: Add XUSB powergates

[Thierry Reding]

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

On Wed, Jun 29, 2016 at 05:07:15PM +0100, Jon Hunter wrote:
> 
> On 29/06/16 16:56, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Jun 29, 2016 at 04:30:08PM +0100, Jon Hunter wrote:
> >>
> >> On 28/06/16 12:20, Jon Hunter wrote:
> >>> The Tegra210 XUSB subsystem has 3 power partitions which are XUSBA
> >>> (super-speed logic), XUSBB (USB device logic) and XUSBC (USB host
> >>> logic). Populate the device-tree nodes for these XUSB partitions.
> >>>
> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >>> ---
> >>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 24 ++++++++++++++++++++++++
> >>>  1 file changed, 24 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> >>> index 65b829b762bb..efb0fd98b789 100644
> >>> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> >>> @@ -670,6 +670,30 @@
> >>>  					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
> >>>  				#power-domain-cells = <0>;
> >>>  			};
> >>> +
> >>> +			pd_xusbss: xusba {
> >>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> >>> +				clock-names = "xusb_ss";
> >>> +				resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> >>> +				reset-names = "xusb_ss";
> >>> +				#power-domain-cells = <0>;
> >>> +			};
> >>> +
> >>> +			pd_xusbdev: xusbb {
> >>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>;
> >>> +				clock-names = "xusb_dev";
> >>> +				resets = <&tegra_car 95>;
> >>> +				reset-names = "xusb_dev";
> >>> +				#power-domain-cells = <0>;
> >>> +			};
> >>> +
> >>> +			pd_xusbhost: xusbc {
> >>> +				clocks = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> >>> +				clock-names = "xusb_host";
> >>> +				resets = <&tegra_car TEGRA210_CLK_XUSB_HOST>;
> >>> +				reset-names = "xusb_host";
> >>> +				#power-domain-cells = <0>;
> >>> +			};
> >>>  		};
> >>>  	};
> >>
> >> The 'clock-names' and 'reset-names' nodes are not used/required and so I
> >> will remove these.
> > 
> > Please keep them and make use of the names. We used to not do this in
> > the past, and then things became tricky to describe in the DT bindings
> > in order to keep backwards-compatibility.
> 
> Unfortunately, in order to use them, we would need to keep a list of all
> the clock and reset names in the PMC driver which would be huge.

That's a Linux driver implementation detail, so it shouldn't influence
the binding.

> > Though perhaps you're not using them because they are found by index? In
> > that case I think it might still be useful to have them for consistency.
> 
> Right, we just iterate over the number of the clocks and resets found
> when we initialise the powergate.

Again, that's a driver implementation detail.

> > If you keep them, you might want to turn the _ into -.
> 
> I can keep them, however, in other patches I have sent out, for example
> the SOR powergate (part of the DPAUX series) and Audio powergate, they
> do not have them. So I thought I would remove them here to be
> consistent. However, we could add them for these other powergates as well.

Yes, I'd prefer them to be listed in all nodes for documentation, if for
nothing else.

Thierry

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