[v3,09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

Message ID 1517934852-23255-10-git-send-email-pdeschrijver@nvidia.com
State New
Headers show
Series
  • Tegra210 DFLL implementation
Related show

Commit Message

Peter De Schrijver Feb. 6, 2018, 4:34 p.m.
Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
support in this driver. Also allow for the case where the CPU voltage is
controlled directly by the DFLL rather than by a separate regulator object.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Jon Hunter March 8, 2018, 11:25 p.m. | #1
On 06/02/18 16:34, Peter De Schrijver wrote:
> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> support in this driver. Also allow for the case where the CPU voltage is
> controlled directly by the DFLL rather than by a separate regulator object.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> index 4353025..f8e01a8 100644
> --- a/drivers/cpufreq/tegra124-cpufreq.c
> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
>  {
>  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
>  	clk_disable_unprepare(priv->dfll_clk);
> -	regulator_sync_voltage(priv->vdd_cpu_reg);
> +	if (priv->vdd_cpu_reg)
> +		regulator_sync_voltage(priv->vdd_cpu_reg);
>  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
>  }
>  
> @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  
>  	priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> -	if (IS_ERR(priv->vdd_cpu_reg)) {
> -		ret = PTR_ERR(priv->vdd_cpu_reg);
> -		goto out_put_np;
> -	}
> +	if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> +		priv->vdd_cpu_reg = NULL;
> +	else
> +		return -EPROBE_DEFER;

I am still not sure that we should rely on the fact that the regulator
is not present in DT to imply that we do not need it. I think that we
should be checking if we are using I2C mode here.

Cheers
Jon
Peter De Schrijver March 9, 2018, 8:14 a.m. | #2
On Thu, Mar 08, 2018 at 11:25:04PM +0000, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
> >  {
> >  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> >  	clk_disable_unprepare(priv->dfll_clk);
> > -	regulator_sync_voltage(priv->vdd_cpu_reg);
> > +	if (priv->vdd_cpu_reg)
> > +		regulator_sync_voltage(priv->vdd_cpu_reg);
> >  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> >  
> > @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  
> >  	priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> > -	if (IS_ERR(priv->vdd_cpu_reg)) {
> > -		ret = PTR_ERR(priv->vdd_cpu_reg);
> > -		goto out_put_np;
> > -	}
> > +	if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> > +		priv->vdd_cpu_reg = NULL;
> > +	else
> > +		return -EPROBE_DEFER;
> 
> I am still not sure that we should rely on the fact that the regulator
> is not present in DT to imply that we do not need it. I think that we
> should be checking if we are using I2C mode here.
> 

The cpufreq driver doesn't know this however. Also the current approach of
setting the same voltage when switching to pll_x is incorrect. The CVB
tables when using pll_x include more margin than when using the DFLL.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar March 9, 2018, 9:11 a.m. | #3
On Tue, Feb 6, 2018 at 10:04 PM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> support in this driver. Also allow for the case where the CPU voltage is
> controlled directly by the DFLL rather than by a separate regulator object.
>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

You forgot to Cc maintainers and pm-list from the cpufreq world :(
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter March 12, 2018, 10:14 a.m. | #4
On 09/03/18 08:14, Peter De Schrijver wrote:
> On Thu, Mar 08, 2018 at 11:25:04PM +0000, Jon Hunter wrote:
>>
>> On 06/02/18 16:34, Peter De Schrijver wrote:
>>> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
>>> support in this driver. Also allow for the case where the CPU voltage is
>>> controlled directly by the DFLL rather than by a separate regulator object.
>>>
>>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
>>> ---
>>>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
>>> index 4353025..f8e01a8 100644
>>> --- a/drivers/cpufreq/tegra124-cpufreq.c
>>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
>>> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
>>>  {
>>>  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
>>>  	clk_disable_unprepare(priv->dfll_clk);
>>> -	regulator_sync_voltage(priv->vdd_cpu_reg);
>>> +	if (priv->vdd_cpu_reg)
>>> +		regulator_sync_voltage(priv->vdd_cpu_reg);
>>>  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
>>>  }
>>>  
>>> @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct platform_device *pdev)
>>>  		return -ENODEV;
>>>  
>>>  	priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
>>> -	if (IS_ERR(priv->vdd_cpu_reg)) {
>>> -		ret = PTR_ERR(priv->vdd_cpu_reg);
>>> -		goto out_put_np;
>>> -	}
>>> +	if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
>>> +		priv->vdd_cpu_reg = NULL;
>>> +	else
>>> +		return -EPROBE_DEFER;
>>
>> I am still not sure that we should rely on the fact that the regulator
>> is not present in DT to imply that we do not need it. I think that we
>> should be checking if we are using I2C mode here.
>>
> 
> The cpufreq driver doesn't know this however. Also the current approach of
> setting the same voltage when switching to pll_x is incorrect. The CVB
> tables when using pll_x include more margin than when using the DFLL.

Ah yes I see now. However, we are going to need to update the DT doc,
because 'vdd-cpu-supply' is listed as required.

Cheers
Jon
Jon Hunter March 12, 2018, 12:15 p.m. | #5
On 06/02/18 16:34, Peter De Schrijver wrote:
> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> support in this driver. Also allow for the case where the CPU voltage is
> controlled directly by the DFLL rather than by a separate regulator object.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> index 4353025..f8e01a8 100644
> --- a/drivers/cpufreq/tegra124-cpufreq.c
> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
>  {
>  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
>  	clk_disable_unprepare(priv->dfll_clk);
> -	regulator_sync_voltage(priv->vdd_cpu_reg);
> +	if (priv->vdd_cpu_reg)
> +		regulator_sync_voltage(priv->vdd_cpu_reg);
>  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
>  }

OK, so this bit does not make sense to me. In the above we are switching
from the DFLL to the PLL (ie. disabling the DFLL) and so to ensure we
are operating at the correct voltage after disabling the DFLL we need to
sync the voltage. Seems we would need to do this for all devices, no?
How is the different between Tegra124 and Tegra210?

Cheers
Jon
Peter De Schrijver March 13, 2018, 9:28 a.m. | #6
On Mon, Mar 12, 2018 at 10:14:17AM +0000, Jon Hunter wrote:
> 
> On 09/03/18 08:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 11:25:04PM +0000, Jon Hunter wrote:
> >>
> >> On 06/02/18 16:34, Peter De Schrijver wrote:
> >>> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> >>> support in this driver. Also allow for the case where the CPU voltage is
> >>> controlled directly by the DFLL rather than by a separate regulator object.
> >>>
> >>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> >>> ---
> >>>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
> >>>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> >>> index 4353025..f8e01a8 100644
> >>> --- a/drivers/cpufreq/tegra124-cpufreq.c
> >>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> >>> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
> >>>  {
> >>>  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> >>>  	clk_disable_unprepare(priv->dfll_clk);
> >>> -	regulator_sync_voltage(priv->vdd_cpu_reg);
> >>> +	if (priv->vdd_cpu_reg)
> >>> +		regulator_sync_voltage(priv->vdd_cpu_reg);
> >>>  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >>>  }
> >>>  
> >>> @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct platform_device *pdev)
> >>>  		return -ENODEV;
> >>>  
> >>>  	priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> >>> -	if (IS_ERR(priv->vdd_cpu_reg)) {
> >>> -		ret = PTR_ERR(priv->vdd_cpu_reg);
> >>> -		goto out_put_np;
> >>> -	}
> >>> +	if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> >>> +		priv->vdd_cpu_reg = NULL;
> >>> +	else
> >>> +		return -EPROBE_DEFER;
> >>
> >> I am still not sure that we should rely on the fact that the regulator
> >> is not present in DT to imply that we do not need it. I think that we
> >> should be checking if we are using I2C mode here.
> >>
> > 
> > The cpufreq driver doesn't know this however. Also the current approach of
> > setting the same voltage when switching to pll_x is incorrect. The CVB
> > tables when using pll_x include more margin than when using the DFLL.
> 
> Ah yes I see now. However, we are going to need to update the DT doc,
> because 'vdd-cpu-supply' is listed as required.
> 

Ok.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter De Schrijver March 13, 2018, 9:51 a.m. | #7
On Mon, Mar 12, 2018 at 12:15:22PM +0000, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
> >  {
> >  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> >  	clk_disable_unprepare(priv->dfll_clk);
> > -	regulator_sync_voltage(priv->vdd_cpu_reg);
> > +	if (priv->vdd_cpu_reg)
> > +		regulator_sync_voltage(priv->vdd_cpu_reg);
> >  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> 
> OK, so this bit does not make sense to me. In the above we are switching
> from the DFLL to the PLL (ie. disabling the DFLL) and so to ensure we
> are operating at the correct voltage after disabling the DFLL we need to
> sync the voltage. Seems we would need to do this for all devices, no?
> How is the different between Tegra124 and Tegra210?

Yes. So in case of i2c the regulator framework will reapply the voltage it
knows which in our case is the boot voltage for VDD_CPU because noone else
from a regulator framework pov has ever changed the voltage. In case of PWM
putting the PWM output pad in tri state will cause the OVR regulator to output
a hardware defined voltage. This is done as part of the dfll_clk_disable()
function. To summarize:

switch from pll_x to DFLL for i2c regulator:

entry: voltage is boot voltage set bootloader
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will switch to closed loop mode and start controlling
   the voltage via i2c, however because we are below Fmax@Vmin there's no
   V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for i2c regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the voltage to be the last value programmed
   by the DFLL.
3) go back to the voltage as programmed by the boot loader using
   regulator_sync_voltage().
4) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

switch from pll_x to DFLL for PWM regulator:

entry: voltage is boot voltage set by hardware because PWM pin is in tristate
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will set the PWM pad to output, switch to closed loop mode
   and start controlling the voltage via PWM, however because we are below
   Fmax@Vmin there's no V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for PWM regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the PWM pad to be programmed to tristate
   and the voltage to change back to the hardware defined voltage.
3) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Hunter March 13, 2018, 10:20 a.m. | #8
On 13/03/18 09:51, Peter De Schrijver wrote:
> On Mon, Mar 12, 2018 at 12:15:22PM +0000, Jon Hunter wrote:
>>
>> On 06/02/18 16:34, Peter De Schrijver wrote:
>>> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
>>> support in this driver. Also allow for the case where the CPU voltage is
>>> controlled directly by the DFLL rather than by a separate regulator object.
>>>
>>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
>>> ---
>>>  drivers/cpufreq/tegra124-cpufreq.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
>>> index 4353025..f8e01a8 100644
>>> --- a/drivers/cpufreq/tegra124-cpufreq.c
>>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
>>> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
>>>  {
>>>  	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
>>>  	clk_disable_unprepare(priv->dfll_clk);
>>> -	regulator_sync_voltage(priv->vdd_cpu_reg);
>>> +	if (priv->vdd_cpu_reg)
>>> +		regulator_sync_voltage(priv->vdd_cpu_reg);
>>>  	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
>>>  }
>>
>> OK, so this bit does not make sense to me. In the above we are switching
>> from the DFLL to the PLL (ie. disabling the DFLL) and so to ensure we
>> are operating at the correct voltage after disabling the DFLL we need to
>> sync the voltage. Seems we would need to do this for all devices, no?
>> How is the different between Tegra124 and Tegra210?
> 
> Yes. So in case of i2c the regulator framework will reapply the voltage it
> knows which in our case is the boot voltage for VDD_CPU because noone else
> from a regulator framework pov has ever changed the voltage. In case of PWM
> putting the PWM output pad in tri state will cause the OVR regulator to output
> a hardware defined voltage. This is done as part of the dfll_clk_disable()
> function. To summarize:

So this is the piece of information I was missing. Maybe add this to the
changelog so it is clear why we do not need to handle the cpu rail in
the case of PWM.

Cheers
Jon

Patch

diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
index 4353025..f8e01a8 100644
--- a/drivers/cpufreq/tegra124-cpufreq.c
+++ b/drivers/cpufreq/tegra124-cpufreq.c
@@ -64,7 +64,8 @@  static void tegra124_cpu_switch_to_pllx(struct tegra124_cpufreq_priv *priv)
 {
 	clk_set_parent(priv->cpu_clk, priv->pllp_clk);
 	clk_disable_unprepare(priv->dfll_clk);
-	regulator_sync_voltage(priv->vdd_cpu_reg);
+	if (priv->vdd_cpu_reg)
+		regulator_sync_voltage(priv->vdd_cpu_reg);
 	clk_set_parent(priv->cpu_clk, priv->pllx_clk);
 }
 
@@ -89,10 +90,10 @@  static int tegra124_cpufreq_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
-	if (IS_ERR(priv->vdd_cpu_reg)) {
-		ret = PTR_ERR(priv->vdd_cpu_reg);
-		goto out_put_np;
-	}
+	if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
+		priv->vdd_cpu_reg = NULL;
+	else
+		return -EPROBE_DEFER;
 
 	priv->cpu_clk = of_clk_get_by_name(np, "cpu_g");
 	if (IS_ERR(priv->cpu_clk)) {
@@ -148,7 +149,6 @@  static int tegra124_cpufreq_probe(struct platform_device *pdev)
 	clk_put(priv->cpu_clk);
 out_put_vdd_cpu_reg:
 	regulator_put(priv->vdd_cpu_reg);
-out_put_np:
 	of_node_put(np);
 
 	return ret;
@@ -181,7 +181,8 @@  static int __init tegra_cpufreq_init(void)
 	int ret;
 	struct platform_device *pdev;
 
-	if (!of_machine_is_compatible("nvidia,tegra124"))
+	if (!(of_machine_is_compatible("nvidia,tegra124")
+		|| of_machine_is_compatible("nvidia,tegra210")))
 		return -ENODEV;
 
 	/*