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

2018-03-13 Thread Jon Hunter 

On 13/03/18 09:51, Peter De Schrijver wrote:
> On Mon, Mar 12, 2018 at 12:15:22PM +, 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 
>>> ---
>>>  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

-- 
nvpublic

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

2018-03-13 Thread Jon Hunter 

On 13/03/18 09:51, Peter De Schrijver wrote:
> On Mon, Mar 12, 2018 at 12:15:22PM +, 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 
>>> ---
>>>  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

-- 
nvpublic

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

2018-03-13 Thread Peter De Schrijver 
On Mon, Mar 12, 2018 at 12:15:22PM +, 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 
> > ---
> >  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.

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

2018-03-13 Thread Peter De Schrijver 
On Mon, Mar 12, 2018 at 12:15:22PM +, 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 
> > ---
> >  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.

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

2018-03-13 Thread Peter De Schrijver 
On Mon, Mar 12, 2018 at 10:14:17AM +, Jon Hunter wrote:
> 
> On 09/03/18 08:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
> >>> ---
> >>>  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.

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

2018-03-13 Thread Peter De Schrijver 
On Mon, Mar 12, 2018 at 10:14:17AM +, Jon Hunter wrote:
> 
> On 09/03/18 08:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
> >>> ---
> >>>  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.

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

2018-03-12 Thread Jon Hunter 

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 
> ---
>  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

-- 
nvpublic

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

2018-03-12 Thread Jon Hunter 

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 
> ---
>  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

-- 
nvpublic

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

2018-03-12 Thread Jon Hunter 

On 09/03/18 08:14, Peter De Schrijver wrote:
> On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
>>> ---
>>>  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

-- 
nvpublic

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

2018-03-12 Thread Jon Hunter 

On 09/03/18 08:14, Peter De Schrijver wrote:
> On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
>>> ---
>>>  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

-- 
nvpublic

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

2018-03-09 Thread Viresh Kumar 
On Tue, Feb 6, 2018 at 10:04 PM, 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 
> ---
>  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 :(

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

2018-03-09 Thread Viresh Kumar 
On Tue, Feb 6, 2018 at 10:04 PM, 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 
> ---
>  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 :(

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

2018-03-09 Thread Peter De Schrijver 
On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
> > ---
> >  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.

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

2018-03-09 Thread Peter De Schrijver 
On Thu, Mar 08, 2018 at 11:25:04PM +, 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 
> > ---
> >  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.

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

2018-03-08 Thread Jon Hunter 

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 
> ---
>  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

-- 
nvpublic

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

2018-03-08 Thread Jon Hunter 

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 
> ---
>  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

-- 
nvpublic