| Message ID | 1517934852-23255-10-git-send-email-pdeschrijver@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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; /*
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(-)