| Message ID | 20181021205501.23943-2-digetx@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | CPUFREQ OPP's, DVFS and Tegra30 support by tegra20-cpufreq driver | expand |
On 21-10-18, 23:54, Dmitry Osipenko wrote: > Voltage regulators may be not available on some variations of HW, allow to > request stub voltage regulators by OPP core in a such case to reduce code > churning within drivers. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/cpufreq/cpufreq-dt.c | 2 +- > drivers/cpufreq/ti-cpufreq.c | 3 ++- > drivers/opp/core.c | 9 +++++++-- > include/linux/pm_opp.h | 4 ++-- > 4 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index e58bfcb1169e..6ebca472ec76 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -196,7 +196,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) > */ > name = find_supply_name(cpu_dev); > if (name) { > - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); > + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1, false); > if (IS_ERR(opp_table)) { > ret = PTR_ERR(opp_table); > dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", Have you actually tested this stuff ? The cpufreq-dt driver will probably fail to probe if the CPU node has a "-supply" property, but no regulator matching that.
On 10/22/18 8:36 AM, Viresh Kumar wrote: > On 21-10-18, 23:54, Dmitry Osipenko wrote: >> Voltage regulators may be not available on some variations of HW, allow to >> request stub voltage regulators by OPP core in a such case to reduce code >> churning within drivers. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/cpufreq/cpufreq-dt.c | 2 +- >> drivers/cpufreq/ti-cpufreq.c | 3 ++- >> drivers/opp/core.c | 9 +++++++-- >> include/linux/pm_opp.h | 4 ++-- >> 4 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >> index e58bfcb1169e..6ebca472ec76 100644 >> --- a/drivers/cpufreq/cpufreq-dt.c >> +++ b/drivers/cpufreq/cpufreq-dt.c >> @@ -196,7 +196,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) >> */ >> name = find_supply_name(cpu_dev); >> if (name) { >> - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); >> + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1, false); >> if (IS_ERR(opp_table)) { >> ret = PTR_ERR(opp_table); >> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", > > Have you actually tested this stuff ? The cpufreq-dt driver will > probably fail to probe if the CPU node has a "-supply" property, but > no regulator matching that. > Please notice that this patch doesn't change the original behaviour and I suppose that failing in a case of missing regulator is the expected behaviour for cpufreq-dt. Hence can't see any problem here.
On 22-10-18, 14:29, Dmitry Osipenko wrote: > On 10/22/18 8:36 AM, Viresh Kumar wrote: > > On 21-10-18, 23:54, Dmitry Osipenko wrote: > >> Voltage regulators may be not available on some variations of HW, allow to > >> request stub voltage regulators by OPP core in a such case to reduce code > >> churning within drivers. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/cpufreq/cpufreq-dt.c | 2 +- > >> drivers/cpufreq/ti-cpufreq.c | 3 ++- > >> drivers/opp/core.c | 9 +++++++-- > >> include/linux/pm_opp.h | 4 ++-- > >> 4 files changed, 12 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > >> index e58bfcb1169e..6ebca472ec76 100644 > >> --- a/drivers/cpufreq/cpufreq-dt.c > >> +++ b/drivers/cpufreq/cpufreq-dt.c > >> @@ -196,7 +196,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) > >> */ > >> name = find_supply_name(cpu_dev); > >> if (name) { > >> - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); > >> + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1, false); > >> if (IS_ERR(opp_table)) { > >> ret = PTR_ERR(opp_table); > >> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", > > > > Have you actually tested this stuff ? The cpufreq-dt driver will > > probably fail to probe if the CPU node has a "-supply" property, but > > no regulator matching that. > > > > Please notice that this patch doesn't change the original behaviour > and I suppose that failing in a case of missing regulator is the > expected behaviour for cpufreq-dt. Hence can't see any problem here. So why have this patch then ?
On 10/22/18 2:32 PM, Viresh Kumar wrote: > On 22-10-18, 14:29, Dmitry Osipenko wrote: >> On 10/22/18 8:36 AM, Viresh Kumar wrote: >>> On 21-10-18, 23:54, Dmitry Osipenko wrote: >>>> Voltage regulators may be not available on some variations of HW, allow to >>>> request stub voltage regulators by OPP core in a such case to reduce code >>>> churning within drivers. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/cpufreq/cpufreq-dt.c | 2 +- >>>> drivers/cpufreq/ti-cpufreq.c | 3 ++- >>>> drivers/opp/core.c | 9 +++++++-- >>>> include/linux/pm_opp.h | 4 ++-- >>>> 4 files changed, 12 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >>>> index e58bfcb1169e..6ebca472ec76 100644 >>>> --- a/drivers/cpufreq/cpufreq-dt.c >>>> +++ b/drivers/cpufreq/cpufreq-dt.c >>>> @@ -196,7 +196,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) >>>> */ >>>> name = find_supply_name(cpu_dev); >>>> if (name) { >>>> - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); >>>> + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1, false); >>>> if (IS_ERR(opp_table)) { >>>> ret = PTR_ERR(opp_table); >>>> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", >>> >>> Have you actually tested this stuff ? The cpufreq-dt driver will >>> probably fail to probe if the CPU node has a "-supply" property, but >>> no regulator matching that. >>> >> >> Please notice that this patch doesn't change the original behaviour >> and I suppose that failing in a case of missing regulator is the >> expected behaviour for cpufreq-dt. Hence can't see any problem here. > > So why have this patch then ? Because there is one Tegra20 board (tegra20-trimslice) that doesn't declare necessary regulators, but we want to have CPU frequency scaling. I couldn't find board schematics and so don't know if CPU / CORE voltages are fixed on Trim-Slice or it is just preferable not to have DVFS for that board, it is an outlet-powered device [0]. Hence tegra20-cpufreq driver will request a dummy regulators when appropriate. [0] http://www.compulab.co.il/utilite-computer/web/trim-slice
On 22-10-18, 15:12, Dmitry Osipenko wrote: > Because there is one Tegra20 board (tegra20-trimslice) that doesn't declare > necessary regulators, but we want to have CPU frequency scaling. I couldn't > find board schematics and so don't know if CPU / CORE voltages are fixed on > Trim-Slice or it is just preferable not to have DVFS for that board, it is an > outlet-powered device [0]. Hence tegra20-cpufreq driver will request a dummy > regulators when appropriate. We have been using the regulator_get_optional() variant until now in the OPP core to make sure that we don't do DVFS for the CPU without the mandatory regulators being present, as that may make things unstable and cause harm to the SoC if we try to take CPU to frequency range over the currently programmed regulator can support. Now coming back to tegra-20 SoC, which actually requires a regulator normally by design. On one of the boards (which is outlet powered), you aren't sure if there is a programmable regulator or not, or if DVFS should really be done or not. Isn't it worth checking the same from Tegra maintainers, or whomsoever has information on that board ? What would happen if there actually was a regulator and its default settings aren't good enough for high end frequencies ? On the other hand, the tegra20 cpufreq driver is common across a lot of boards. What will happen if the DT for some of the boards isn't correct and missed the necessary regulator node ? And because you are moving to regulator_get() API for the entire SoC (i.e. its cpufreq driver), people will never find the missing regulator. If we can do it safely for all tegra20 boards, why not migrate to using regulator_get() instead of regulator_get_optional() in the OPP core API itself for everyone ?
On 10/24/18 9:41 AM, Viresh Kumar wrote: > On 22-10-18, 15:12, Dmitry Osipenko wrote: >> Because there is one Tegra20 board (tegra20-trimslice) that doesn't declare >> necessary regulators, but we want to have CPU frequency scaling. I couldn't >> find board schematics and so don't know if CPU / CORE voltages are fixed on >> Trim-Slice or it is just preferable not to have DVFS for that board, it is an >> outlet-powered device [0]. Hence tegra20-cpufreq driver will request a dummy >> regulators when appropriate. > > We have been using the regulator_get_optional() variant until now in the OPP > core to make sure that we don't do DVFS for the CPU without the mandatory > regulators being present, as that may make things unstable and cause harm to the > SoC if we try to take CPU to frequency range over the currently programmed > regulator can support. > > Now coming back to tegra-20 SoC, which actually requires a regulator normally by > design. On one of the boards (which is outlet powered), you aren't sure if there > is a programmable regulator or not, or if DVFS should really be done or not. > Isn't it worth checking the same from Tegra maintainers, or whomsoever has > information on that board ? I'll try to find out more detailed information for the next revision of the patchset. What would happen if there actually was a regulator > and its default settings aren't good enough for high end frequencies ? Usually this causes kernel/applications crashes and/or machine hang. > On the other hand, the tegra20 cpufreq driver is common across a lot of boards. > What will happen if the DT for some of the boards isn't correct and missed the > necessary regulator node ? AFAIK, there is assumption that bootloader should setup regulators in a way that kernel could work properly at max clock rates. Otherwise things won't work. And because you are moving to regulator_get() API for > the entire SoC (i.e. its cpufreq driver), people will never find the missing > regulator. Regulators core prints info message when dummy regulator is being used. > If we can do it safely for all tegra20 boards, why not migrate to using > regulator_get() instead of regulator_get_optional() in the OPP core API itself > for everyone ? > This should be a platform-specific decision. For Tegra we know that regulators should be in a good state at kernel boot time, I don't think that this applies to other platforms.
Am Freitag, den 26.10.2018, 15:03 +0300 schrieb Dmitry Osipenko: [...] > > On the other hand, the tegra20 cpufreq driver is common across a lot of boards. > > What will happen if the DT for some of the boards isn't correct and missed the > > necessary regulator node ? > > AFAIK, there is assumption that bootloader should setup regulators in > a way that kernel could work properly at max clock rates. Otherwise > things won't work. This isn't true. The assumption is that the bootloader sets up the regulators such that stable operation at the CPU speed used by the bootloader is guaranteed. Often the bootloader doesn't know about specific SKUs, so drives things at a rate/voltage that is safe across all SKUs [1], in which case the bootloader is totally unaware of the voltage needed to run the CPU at highest possible clock frequency. Regards, Lucas [1] http://git.denx.de/?p=u-boot.git;a=commit;h=2364e151e432b4ccf32dc9e6147121253d4ff86d
On 10/26/18 6:37 PM, Lucas Stach wrote: > Am Freitag, den 26.10.2018, 15:03 +0300 schrieb Dmitry Osipenko: > [...] >>> On the other hand, the tegra20 cpufreq driver is common across a lot of boards. >>> What will happen if the DT for some of the boards isn't correct and missed the >>> necessary regulator node ? >> >> AFAIK, there is assumption that bootloader should setup regulators in >> a way that kernel could work properly at max clock rates. Otherwise >> things won't work. > > This isn't true. The assumption is that the bootloader sets up the > regulators such that stable operation at the CPU speed used by the > bootloader is guaranteed. > > Often the bootloader doesn't know about specific SKUs, so drives things > at a rate/voltage that is safe across all SKUs [1], in which case the > bootloader is totally unaware of the voltage needed to run the CPU at > highest possible clock frequency. > > Regards, > Lucas > > [1] http://git.denx.de/?p=u-boot.git;a=commit;h=2364e151e432b4ccf32dc9e6147121253d4ff86d > Oh, okay. Thank you for pointing at [1], though the assumption is true for T20. I agree that it's better not to rely on configuration left from bootloader in general.
On 26-10-18, 15:03, Dmitry Osipenko wrote: > On 10/24/18 9:41 AM, Viresh Kumar wrote: > > On 22-10-18, 15:12, Dmitry Osipenko wrote: > >> Because there is one Tegra20 board (tegra20-trimslice) that doesn't declare > >> necessary regulators, but we want to have CPU frequency scaling. I couldn't > >> find board schematics and so don't know if CPU / CORE voltages are fixed on > >> Trim-Slice or it is just preferable not to have DVFS for that board, it is an > >> outlet-powered device [0]. Hence tegra20-cpufreq driver will request a dummy > >> regulators when appropriate. > > > > We have been using the regulator_get_optional() variant until now in the OPP > > core to make sure that we don't do DVFS for the CPU without the mandatory > > regulators being present, as that may make things unstable and cause harm to the > > SoC if we try to take CPU to frequency range over the currently programmed > > regulator can support. > > > > Now coming back to tegra-20 SoC, which actually requires a regulator normally by > > design. On one of the boards (which is outlet powered), you aren't sure if there > > is a programmable regulator or not, or if DVFS should really be done or not. > > Isn't it worth checking the same from Tegra maintainers, or whomsoever has > > information on that board ? > > I'll try to find out more detailed information for the next revision of the patchset. Thanks Dmitry. > What would happen if there actually was a regulator Please preserve the '>' from previous replies at the beginning of the lines. Otherwise it looks as if you have written the above line. :) > > and its default settings aren't good enough for high end frequencies ? > > Usually this causes kernel/applications crashes and/or machine hang. Sure. I also do remember from some guys (maybe TI), where they mentioned that such scenarios can harm the hardware as well sometimes. Don't remember the details though. > And because you are moving to regulator_get() API for > > the entire SoC (i.e. its cpufreq driver), people will never find the missing > > regulator. > > Regulators core prints info message when dummy regulator is being used. Sure, but they are easy to miss and they are only seen by developers not regular users of a machine. > > If we can do it safely for all tegra20 boards, why not migrate to using > > regulator_get() instead of regulator_get_optional() in the OPP core API itself > > for everyone ? > > > > This should be a platform-specific decision. For Tegra we know that regulators should be in a good state at kernel boot time, I don't think that this applies to other platforms. Based on the other discussion with Lucas on this thread, I don't think this is correct any more ? Don't get me wrong, I am all good for changes and another API change doesn't matter much to me. I am just wondering if it would be the right approach to fix the issue. Why not rather call dev_pm_opp_set_regulators() conditionally and avoid calling it for the specific tegra20 board.
On 10/29/18 9:53 AM, Viresh Kumar wrote: > On 26-10-18, 15:03, Dmitry Osipenko wrote: >> On 10/24/18 9:41 AM, Viresh Kumar wrote: >>> On 22-10-18, 15:12, Dmitry Osipenko wrote: >>>> Because there is one Tegra20 board (tegra20-trimslice) that doesn't declare >>>> necessary regulators, but we want to have CPU frequency scaling. I couldn't >>>> find board schematics and so don't know if CPU / CORE voltages are fixed on >>>> Trim-Slice or it is just preferable not to have DVFS for that board, it is an >>>> outlet-powered device [0]. Hence tegra20-cpufreq driver will request a dummy >>>> regulators when appropriate. >>> >>> We have been using the regulator_get_optional() variant until now in the OPP >>> core to make sure that we don't do DVFS for the CPU without the mandatory >>> regulators being present, as that may make things unstable and cause harm to the >>> SoC if we try to take CPU to frequency range over the currently programmed >>> regulator can support. >>> >>> Now coming back to tegra-20 SoC, which actually requires a regulator normally by >>> design. On one of the boards (which is outlet powered), you aren't sure if there >>> is a programmable regulator or not, or if DVFS should really be done or not. >>> Isn't it worth checking the same from Tegra maintainers, or whomsoever has >>> information on that board ? >> >> I'll try to find out more detailed information for the next revision of the patchset. > > Thanks Dmitry. > >> What would happen if there actually was a regulator > > Please preserve the '>' from previous replies at the beginning of the > lines. Otherwise it looks as if you have written the above line. :) > >>> and its default settings aren't good enough for high end frequencies ? >> >> Usually this causes kernel/applications crashes and/or machine hang. > > Sure. I also do remember from some guys (maybe TI), where they > mentioned that such scenarios can harm the hardware as well sometimes. > Don't remember the details though. > >> And because you are moving to regulator_get() API for >>> the entire SoC (i.e. its cpufreq driver), people will never find the missing >>> regulator. >> >> Regulators core prints info message when dummy regulator is being used. > > Sure, but they are easy to miss and they are only seen by developers > not regular users of a machine. > >>> If we can do it safely for all tegra20 boards, why not migrate to using >>> regulator_get() instead of regulator_get_optional() in the OPP core API itself >>> for everyone ? >>> >> >> This should be a platform-specific decision. For Tegra we know that regulators should be in a good state at kernel boot time, I don't think that this applies to other platforms. > > Based on the other discussion with Lucas on this thread, I don't think > this is correct any more ? > > Don't get me wrong, I am all good for changes and another API change > doesn't matter much to me. I am just wondering if it would be the > right approach to fix the issue. Why not rather call > dev_pm_opp_set_regulators() conditionally and avoid calling it for the > specific tegra20 board. The right approach should be to specify device tree correctly. Probably we won't need this change at all with the correct DT, will see. Calling dev_pm_opp_set_regulators() conditionally should work, but that is extra churning in the driver that I tried to avoid.
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e58bfcb1169e..6ebca472ec76 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -196,7 +196,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) { - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1, false); if (IS_ERR(opp_table)) { ret = PTR_ERR(opp_table); dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c index 3f0e2a14895a..9099c8cdf447 100644 --- a/drivers/cpufreq/ti-cpufreq.c +++ b/drivers/cpufreq/ti-cpufreq.c @@ -268,7 +268,8 @@ static int ti_cpufreq_probe(struct platform_device *pdev) if (opp_data->soc_data->multi_regulator) { ti_opp_table = dev_pm_opp_set_regulators(opp_data->cpu_dev, reg_names, - ARRAY_SIZE(reg_names)); + ARRAY_SIZE(reg_names), + false); if (IS_ERR(ti_opp_table)) { dev_pm_opp_put_supported_hw(opp_data->opp_table); ret = PTR_ERR(ti_opp_table); diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 2c2df4e4fc14..fba1d7a1eb7c 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1365,6 +1365,7 @@ static void _free_set_opp_data(struct opp_table *opp_table) * @dev: Device for which regulator name is being set. * @names: Array of pointers to the names of the regulator. * @count: Number of regulators. + * @allow_stub_regulator: Some or all regulators can be missed. * * In order to support OPP switching, OPP layer needs to know the name of the * device's regulators, as the core would be required to switch voltages as @@ -1374,7 +1375,8 @@ static void _free_set_opp_data(struct opp_table *opp_table) */ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], - unsigned int count) + unsigned int count, + bool allow_stub_regulator) { struct opp_table *opp_table; struct regulator *reg; @@ -1403,7 +1405,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, } for (i = 0; i < count; i++) { - reg = regulator_get_optional(dev, names[i]); + if (allow_stub_regulator) + reg = regulator_get(dev, names[i]); + else + reg = regulator_get_optional(dev, names[i]); if (IS_ERR(reg)) { ret = PTR_ERR(reg); if (ret != -EPROBE_DEFER) diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 5d399eeef172..480666b0a008 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -120,7 +120,7 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, const u32 *ver void dev_pm_opp_put_supported_hw(struct opp_table *opp_table); struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name); void dev_pm_opp_put_prop_name(struct opp_table *opp_table); -struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count, bool allow_stub_regulator); void dev_pm_opp_put_regulators(struct opp_table *opp_table); struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name); void dev_pm_opp_put_clkname(struct opp_table *opp_table); @@ -258,7 +258,7 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {} -static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) +static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count, bool allow_stub_regulator) { return ERR_PTR(-ENOTSUPP); }
Voltage regulators may be not available on some variations of HW, allow to request stub voltage regulators by OPP core in a such case to reduce code churning within drivers. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/cpufreq/cpufreq-dt.c | 2 +- drivers/cpufreq/ti-cpufreq.c | 3 ++- drivers/opp/core.c | 9 +++++++-- include/linux/pm_opp.h | 4 ++-- 4 files changed, 12 insertions(+), 6 deletions(-)