Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

Hi Tony,

> Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
> 
> This code is currently unable to find the dts opp tables as ti-cpufreq
> needs to set them up first based on speed binning.
> 
> We stopped initializing the opp tables with platform code years ago for
> device tree based booting with commit 92d51856d740 ("ARM: OMAP3+: do not
> register non-dt OPP tables for device tree boot"), and all of mach-omap2
> is now booting using device tree.
> 
> We currently get the following errors on init:
> 
> omap2_set_init_voltage: unable to find boot up OPP for vdd_mpu
> omap2_set_init_voltage: unable to set vdd_mpu
> omap2_set_init_voltage: unable to find boot up OPP for vdd_core
> omap2_set_init_voltage: unable to set vdd_core
> omap2_set_init_voltage: unable to find boot up OPP for vdd_iva
> omap2_set_init_voltage: unable to set vdd_iva
> 
> Let's just drop the unused code. Nowadays ti-cpufreq should be used to
> to initialize things properly.
> 
> Cc: Adam Ford <aford173@gmail.com>
> Cc: André Roth <neolynx@gmail.com>
> Cc: "H. Nikolaus Schaller" <hns@goldelico.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Guys, please check and ack if we can really do this to get rid of some
> pointless dmesg -l3 errors without affecting the ongoing cpufreq and
> voltage work.

unfortunately we did not yet test in combination with the 1GHz OPP
patches for omap3630 (queued for v5.5-rc1) and it appears that this
patch breaks the 1GHz OPP.

The symptom is that it works fine on a dm3730 with 800MHz rating
but results in spurious kernel panics, stack corruption, virtual memory
failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.

We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
the problem and its symptoms appear almost immediately.

After some scratching our heads we found that v5.3.7 is still good and
v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
point to this patch whichwas apparently already backported to v5.3.8 and
v5.4.

So I assume that the code removed here enabled or initialized something
we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
vdd regulator and initializes it earlier than without this code. Maybe
it also (pre-)initializes some clk which could now be left uninitialized
too long?

Note that seeing the log message indicates that voltdm_scale() and
dev_pm_opp_get_voltage() are not called, but all functions before could
be with side-effects.

v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
omap36xx) because it makes the combination of this patch and 1GHz OPP
public (linux-next should already fail but it appears that nobody has
tested).

Any ideas how to fix? Before I try to do a revert and then add goto exit;
after each function call and see which ones are essential for 1GHz.

BR,
Nikolaus


> 
> ---
> arch/arm/mach-omap2/pm.c | 100 ---------------------------------------
> 1 file changed, 100 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -74,83 +74,6 @@ int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused)
> 	return 0;
> }
> 
> -/*
> - * This API is to be called during init to set the various voltage
> - * domains to the voltage as per the opp table. Typically we boot up
> - * at the nominal voltage. So this function finds out the rate of
> - * the clock associated with the voltage domain, finds out the correct
> - * opp entry and sets the voltage domain to the voltage specified
> - * in the opp entry
> - */
> -static int __init omap2_set_init_voltage(char *vdd_name, char *clk_name,
> -					 const char *oh_name)
> -{
> -	struct voltagedomain *voltdm;
> -	struct clk *clk;
> -	struct dev_pm_opp *opp;
> -	unsigned long freq, bootup_volt;
> -	struct device *dev;
> -
> -	if (!vdd_name || !clk_name || !oh_name) {
> -		pr_err("%s: invalid parameters\n", __func__);
> -		goto exit;
> -	}
> -
> -	if (!strncmp(oh_name, "mpu", 3))
> -		/* 
> -		 * All current OMAPs share voltage rail and clock
> -		 * source, so CPU0 is used to represent the MPU-SS.
> -		 */
> -		dev = get_cpu_device(0);
> -	else
> -		dev = omap_device_get_by_hwmod_name(oh_name);
> -
> -	if (IS_ERR(dev)) {
> -		pr_err("%s: Unable to get dev pointer for hwmod %s\n",
> -			__func__, oh_name);
> -		goto exit;
> -	}
> -
> -	voltdm = voltdm_lookup(vdd_name);
> -	if (!voltdm) {
> -		pr_err("%s: unable to get vdd pointer for vdd_%s\n",
> -			__func__, vdd_name);
> -		goto exit;
> -	}
> -
> -	clk =  clk_get(NULL, clk_name);
> -	if (IS_ERR(clk)) {
> -		pr_err("%s: unable to get clk %s\n", __func__, clk_name);
> -		goto exit;
> -	}
> -
> -	freq = clk_get_rate(clk);
> -	clk_put(clk);
> -
> -	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -	if (IS_ERR(opp)) {
> -		pr_err("%s: unable to find boot up OPP for vdd_%s\n",
> -			__func__, vdd_name);
> -		goto exit;
> -	}
> -
> -	bootup_volt = dev_pm_opp_get_voltage(opp);
> -	dev_pm_opp_put(opp);
> -
> -	if (!bootup_volt) {
> -		pr_err("%s: unable to find voltage corresponding to the bootup OPP for vdd_%s\n",
> -		       __func__, vdd_name);
> -		goto exit;
> -	}
> -
> -	voltdm_scale(voltdm, bootup_volt);
> -	return 0;
> -
> -exit:
> -	pr_err("%s: unable to set vdd_%s\n", __func__, vdd_name);
> -	return -EINVAL;
> -}
> -
> #ifdef CONFIG_SUSPEND
> static int omap_pm_enter(suspend_state_t suspend_state)
> {
> @@ -208,25 +131,6 @@ void omap_common_suspend_init(void *pm_suspend)
> }
> #endif /* CONFIG_SUSPEND */
> 
> -static void __init omap3_init_voltages(void)
> -{
> -	if (!soc_is_omap34xx())
> -		return;
> -
> -	omap2_set_init_voltage("mpu_iva", "dpll1_ck", "mpu");
> -	omap2_set_init_voltage("core", "l3_ick", "l3_main");
> -}
> -
> -static void __init omap4_init_voltages(void)
> -{
> -	if (!soc_is_omap44xx())
> -		return;
> -
> -	omap2_set_init_voltage("mpu", "dpll_mpu_ck", "mpu");
> -	omap2_set_init_voltage("core", "l3_div_ck", "l3_main_1");
> -	omap2_set_init_voltage("iva", "dpll_iva_m5x2_ck", "iva");
> -}
> -
> int __maybe_unused omap_pm_nop_init(void)
> {
> 	return 0;
> @@ -246,10 +150,6 @@ int __init omap2_common_pm_late_init(void)
> 	omap4_twl_init();
> 	omap_voltage_late_init();
> 
> -	/* Initialize the voltages */
> -	omap3_init_voltages();
> -	omap4_init_voltages();
> -
> 	/* Smartreflex device init */
> 	omap_devinit_smartreflex();
> 
> -- 
> 2.23.0

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller <hns@goldelico.com> [191202 21:10]:
> > Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
> > Guys, please check and ack if we can really do this to get rid of some
> > pointless dmesg -l3 errors without affecting the ongoing cpufreq and
> > voltage work.
> 
> unfortunately we did not yet test in combination with the 1GHz OPP
> patches for omap3630 (queued for v5.5-rc1) and it appears that this
> patch breaks the 1GHz OPP.
> 
> The symptom is that it works fine on a dm3730 with 800MHz rating
> but results in spurious kernel panics, stack corruption, virtual memory
> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.

Hmm yeah OK, I was a bit worried about this breaking something.

> We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
> remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
> enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
> the problem and its symptoms appear almost immediately.
> 
> After some scratching our heads we found that v5.3.7 is still good and
> v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
> point to this patch whichwas apparently already backported to v5.3.8 and
> v5.4.
> 
> So I assume that the code removed here enabled or initialized something
> we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
> vdd regulator and initializes it earlier than without this code. Maybe
> it also (pre-)initializes some clk which could now be left uninitialized
> too long?

It was just doing voltdm_lookup() and clk_get_rate() and then failed
dev_pm_opp_find_freq_ceil(), but I don't see what these might affect..

> Note that seeing the log message indicates that voltdm_scale() and
> dev_pm_opp_get_voltage() are not called, but all functions before could
> be with side-effects.

Yes that is strange. There's no clk_prepare() before we proceed to
call clk_get_rate() either, not sure if that matter here though.

> v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
> omap36xx) because it makes the combination of this patch and 1GHz OPP
> public (linux-next should already fail but it appears that nobody has
> tested).

OK

> Any ideas how to fix? Before I try to do a revert and then add goto exit;
> after each function call and see which ones are essential for 1GHz.

If you have things reproducable, care to try to narrow the issue down
a bit by trying see which parts of the old omap2_set_init_voltage()
fix the issue?

The issue should be there somewhere in the few lines of code before
dev_pm_opp_find_freq_ceil(), right?

It would be good to understand what's going on before reverting or
fixing things condering that a revert would add back code that has
it's own errors and fails to init :)

Another thing to check is if the dev instance is actually the right
one we had in omap2_set_init_voltage() vs the dts dev instance as
we use that with dev_pm_opp_find_freq_ceil().

Regards,

Tony

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

HiTony,

> Am 02.12.2019 um 22:39 schrieb Tony Lindgren <tony@atomide.com>:
> 
> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191202 21:10]:
>>> Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
>>> Guys, please check and ack if we can really do this to get rid of some
>>> pointless dmesg -l3 errors without affecting the ongoing cpufreq and
>>> voltage work.
>> 
>> unfortunately we did not yet test in combination with the 1GHz OPP
>> patches for omap3630 (queued for v5.5-rc1) and it appears that this
>> patch breaks the 1GHz OPP.
>> 
>> The symptom is that it works fine on a dm3730 with 800MHz rating
>> but results in spurious kernel panics, stack corruption, virtual memory
>> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.
> 
> Hmm yeah OK, I was a bit worried about this breaking something.
> 
>> We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
>> remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
>> enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
>> the problem and its symptoms appear almost immediately.
>> 
>> After some scratching our heads we found that v5.3.7 is still good and
>> v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
>> point to this patch whichwas apparently already backported to v5.3.8 and
>> v5.4.
>> 
>> So I assume that the code removed here enabled or initialized something
>> we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
>> vdd regulator and initializes it earlier than without this code. Maybe
>> it also (pre-)initializes some clk which could now be left uninitialized
>> too long?
> 
> It was just doing voltdm_lookup() and clk_get_rate() and then failed
> dev_pm_opp_find_freq_ceil(), but I don't see what these might affect..
> 
>> Note that seeing the log message indicates that voltdm_scale() and
>> dev_pm_opp_get_voltage() are not called, but all functions before could
>> be with side-effects.
> 
> Yes that is strange. There's no clk_prepare() before we proceed to
> call clk_get_rate() either, not sure if that matter here though.
> 
>> v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
>> omap36xx) because it makes the combination of this patch and 1GHz OPP
>> public (linux-next should already fail but it appears that nobody has
>> tested).
> 
> OK

Well, it is not that urgent as I thought since I have not yet submitted
my patch to remove the turbo-mode tags for 1GHz OPP. Therefore even if this
code is deployed, no dm3730 will try to boot or run at 1GHz unless
manually enabled by echo 1 >/sys/devices/system/cpu/cpufreq/boost.

> 
>> Any ideas how to fix? Before I try to do a revert and then add goto exit;
>> after each function call and see which ones are essential for 1GHz.
> 
> If you have things reproducable, care to try to narrow the issue down
> a bit by trying see which parts of the old omap2_set_init_voltage()
> fix the issue?
> 
> The issue should be there somewhere in the few lines of code before
> dev_pm_opp_find_freq_ceil(), right?
> 
> It would be good to understand what's going on before reverting or
> fixing things condering that a revert would add back code that has
> it's own errors and fails to init :)

Indeed!

> 
> Another thing to check is if the dev instance is actually the right
> one we had in omap2_set_init_voltage() vs the dts dev instance as
> we use that with dev_pm_opp_find_freq_ceil().

As a first step I tried to comment out some steps but immediately
got failures.

What I then noticed is that there is only a message for

[    2.508392] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
[    2.517639] omap2_set_init_voltage: unable to set vdd_core

There is none for vdd_mpu_iva. This OPP initialization is successful
and does call voltdm_scale() once.

So it appears as if omap3_init_voltages() is not a complete no-op.

IMHO the reason for the message is that u-boot defines a frequency
and voltage that can not be found in the OPP table at all.

Maybe a better solution to get rid of the message would be to modify 
dev_pm_opp_find_freq_ceil() to interpolate between OPPs?

Hm. After looking into the code I start to wonder why it fails at
all. _find_freq_ceil() should return the highest available frequency
above the one passed in and u-boot should not pass more than 800 MHz...

That is IMHO a good next step to go into details.

BR,
Nikolaus
 

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

> Am 03.12.2019 um 10:53 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> HiTony,
> 
>> Am 02.12.2019 um 22:39 schrieb Tony Lindgren <tony@atomide.com>:
>> 
>> Hi,
>> 
>> * H. Nikolaus Schaller <hns@goldelico.com> [191202 21:10]:
>>>> Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
>>>> Guys, please check and ack if we can really do this to get rid of some
>>>> pointless dmesg -l3 errors without affecting the ongoing cpufreq and
>>>> voltage work.
>>> 
>>> unfortunately we did not yet test in combination with the 1GHz OPP
>>> patches for omap3630 (queued for v5.5-rc1) and it appears that this
>>> patch breaks the 1GHz OPP.
>>> 
>>> The symptom is that it works fine on a dm3730 with 800MHz rating
>>> but results in spurious kernel panics, stack corruption, virtual memory
>>> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.
>> 
>> Hmm yeah OK, I was a bit worried about this breaking something.
>> 
>>> We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
>>> remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
>>> enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
>>> the problem and its symptoms appear almost immediately.
>>> 
>>> After some scratching our heads we found that v5.3.7 is still good and
>>> v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
>>> point to this patch whichwas apparently already backported to v5.3.8 and
>>> v5.4.
>>> 
>>> So I assume that the code removed here enabled or initialized something
>>> we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
>>> vdd regulator and initializes it earlier than without this code. Maybe
>>> it also (pre-)initializes some clk which could now be left uninitialized
>>> too long?
>> 
>> It was just doing voltdm_lookup() and clk_get_rate() and then failed
>> dev_pm_opp_find_freq_ceil(), but I don't see what these might affect..
>> 
>>> Note that seeing the log message indicates that voltdm_scale() and
>>> dev_pm_opp_get_voltage() are not called, but all functions before could
>>> be with side-effects.
>> 
>> Yes that is strange. There's no clk_prepare() before we proceed to
>> call clk_get_rate() either, not sure if that matter here though.
>> 
>>> v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
>>> omap36xx) because it makes the combination of this patch and 1GHz OPP
>>> public (linux-next should already fail but it appears that nobody has
>>> tested).
>> 
>> OK
> 
> Well, it is not that urgent as I thought since I have not yet submitted
> my patch to remove the turbo-mode tags for 1GHz OPP. Therefore even if this
> code is deployed, no dm3730 will try to boot or run at 1GHz unless
> manually enabled by echo 1 >/sys/devices/system/cpu/cpufreq/boost.
> 
>> 
>>> Any ideas how to fix? Before I try to do a revert and then add goto exit;
>>> after each function call and see which ones are essential for 1GHz.
>> 
>> If you have things reproducable, care to try to narrow the issue down
>> a bit by trying see which parts of the old omap2_set_init_voltage()
>> fix the issue?
>> 
>> The issue should be there somewhere in the few lines of code before
>> dev_pm_opp_find_freq_ceil(), right?
>> 
>> It would be good to understand what's going on before reverting or
>> fixing things condering that a revert would add back code that has
>> it's own errors and fails to init :)
> 
> Indeed!
> 
>> 
>> Another thing to check is if the dev instance is actually the right
>> one we had in omap2_set_init_voltage() vs the dts dev instance as
>> we use that with dev_pm_opp_find_freq_ceil().
> 
> As a first step I tried to comment out some steps but immediately
> got failures.
> 
> What I then noticed is that there is only a message for
> 
> [    2.508392] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
> [    2.517639] omap2_set_init_voltage: unable to set vdd_core
> 
> There is none for vdd_mpu_iva. This OPP initialization is successful
> and does call voltdm_scale() once.
> 
> So it appears as if omap3_init_voltages() is not a complete no-op.
> 
> IMHO the reason for the message is that u-boot defines a frequency
> and voltage that can not be found in the OPP table at all.
> 
> Maybe a better solution to get rid of the message would be to modify 
> dev_pm_opp_find_freq_ceil() to interpolate between OPPs?
> 
> Hm. After looking into the code I start to wonder why it fails at
> all. _find_freq_ceil() should return the highest available frequency
> above the one passed in and u-boot should not pass more than 800 MHz...
> 
> That is IMHO a good next step to go into details.

Ok, dev_pm_opp_find_freq_ceil() is doing what it should do and it
returns the first OPP higher or equal than the frequency passed in.

The real reason for the warning is that the same OPP table is used
for vdd_mpu_iva and vdd_core and it appears as if "core" (l3_ick)
runs at 200 MHz which does not correspond to a valid OPP.

So to silcence the warning it suffices to remove

	omap2_set_init_voltage("core", "l3_ick", "l3_main");

The question is now what l3_ick has to do with the OPPs at all
and how it should interwork with OPPs and cpufreq.

Or does all this mean we may need a second OPP fable for vdd_core
and 200 MHz? But what would it be good for? I have not seen any
reference for "core-OPPs" in the TRM.

BR,
Nikolaus

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

> Am 03.12.2019 um 13:30 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
>> 
>> Am 03.12.2019 um 10:53 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>> 
>> HiTony,
>> 
>>> Am 02.12.2019 um 22:39 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>> Hi,
>>> 
>>> * H. Nikolaus Schaller <hns@goldelico.com> [191202 21:10]:
>>>>> Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
>>>>> Guys, please check and ack if we can really do this to get rid of some
>>>>> pointless dmesg -l3 errors without affecting the ongoing cpufreq and
>>>>> voltage work.
>>>> 
>>>> unfortunately we did not yet test in combination with the 1GHz OPP
>>>> patches for omap3630 (queued for v5.5-rc1) and it appears that this
>>>> patch breaks the 1GHz OPP.
>>>> 
>>>> The symptom is that it works fine on a dm3730 with 800MHz rating
>>>> but results in spurious kernel panics, stack corruption, virtual memory
>>>> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.
>>> 
>>> Hmm yeah OK, I was a bit worried about this breaking something.
>>> 
>>>> We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
>>>> remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
>>>> enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
>>>> the problem and its symptoms appear almost immediately.
>>>> 
>>>> After some scratching our heads we found that v5.3.7 is still good and
>>>> v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
>>>> point to this patch whichwas apparently already backported to v5.3.8 and
>>>> v5.4.
>>>> 
>>>> So I assume that the code removed here enabled or initialized something
>>>> we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
>>>> vdd regulator and initializes it earlier than without this code. Maybe
>>>> it also (pre-)initializes some clk which could now be left uninitialized
>>>> too long?
>>> 
>>> It was just doing voltdm_lookup() and clk_get_rate() and then failed
>>> dev_pm_opp_find_freq_ceil(), but I don't see what these might affect..
>>> 
>>>> Note that seeing the log message indicates that voltdm_scale() and
>>>> dev_pm_opp_get_voltage() are not called, but all functions before could
>>>> be with side-effects.
>>> 
>>> Yes that is strange. There's no clk_prepare() before we proceed to
>>> call clk_get_rate() either, not sure if that matter here though.
>>> 
>>>> v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
>>>> omap36xx) because it makes the combination of this patch and 1GHz OPP
>>>> public (linux-next should already fail but it appears that nobody has
>>>> tested).
>>> 
>>> OK
>> 
>> Well, it is not that urgent as I thought since I have not yet submitted
>> my patch to remove the turbo-mode tags for 1GHz OPP. Therefore even if this
>> code is deployed, no dm3730 will try to boot or run at 1GHz unless
>> manually enabled by echo 1 >/sys/devices/system/cpu/cpufreq/boost.
>> 
>>> 
>>>> Any ideas how to fix? Before I try to do a revert and then add goto exit;
>>>> after each function call and see which ones are essential for 1GHz.
>>> 
>>> If you have things reproducable, care to try to narrow the issue down
>>> a bit by trying see which parts of the old omap2_set_init_voltage()
>>> fix the issue?
>>> 
>>> The issue should be there somewhere in the few lines of code before
>>> dev_pm_opp_find_freq_ceil(), right?
>>> 
>>> It would be good to understand what's going on before reverting or
>>> fixing things condering that a revert would add back code that has
>>> it's own errors and fails to init :)
>> 
>> Indeed!
>> 
>>> 
>>> Another thing to check is if the dev instance is actually the right
>>> one we had in omap2_set_init_voltage() vs the dts dev instance as
>>> we use that with dev_pm_opp_find_freq_ceil().
>> 
>> As a first step I tried to comment out some steps but immediately
>> got failures.
>> 
>> What I then noticed is that there is only a message for
>> 
>> [    2.508392] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
>> [    2.517639] omap2_set_init_voltage: unable to set vdd_core
>> 
>> There is none for vdd_mpu_iva. This OPP initialization is successful
>> and does call voltdm_scale() once.
>> 
>> So it appears as if omap3_init_voltages() is not a complete no-op.
>> 
>> IMHO the reason for the message is that u-boot defines a frequency
>> and voltage that can not be found in the OPP table at all.
>> 
>> Maybe a better solution to get rid of the message would be to modify 
>> dev_pm_opp_find_freq_ceil() to interpolate between OPPs?
>> 
>> Hm. After looking into the code I start to wonder why it fails at
>> all. _find_freq_ceil() should return the highest available frequency
>> above the one passed in and u-boot should not pass more than 800 MHz...
>> 
>> That is IMHO a good next step to go into details.
> 
> Ok, dev_pm_opp_find_freq_ceil() is doing what it should do and it
> returns the first OPP higher or equal than the frequency passed in.
> 
> The real reason for the warning is that the same OPP table is used
> for vdd_mpu_iva and vdd_core and it appears as if "core" (l3_ick)
> runs at 200 MHz which does not correspond to a valid OPP.
> 
> So to silcence the warning it suffices to remove
> 
> 	omap2_set_init_voltage("core", "l3_ick", "l3_main");
> 
> The question is now what l3_ick has to do with the OPPs at all
> and how it should interwork with OPPs and cpufreq.
> 
> Or does all this mean we may need a second OPP fable for vdd_core
> and 200 MHz? But what would it be good for? I have not seen any
> reference for "core-OPPs" in the TRM.

One more, maybe important, finding:

cpufreq_init() is called ca. 0.4 seconds before omap2_set_init_voltage()
and dpll1_ck may already be 1 GHz at that point.

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [191203 12:31]:
> Ok, dev_pm_opp_find_freq_ceil() is doing what it should do and it
> returns the first OPP higher or equal than the frequency passed in.
>
> The real reason for the warning is that the same OPP table is used
> for vdd_mpu_iva and vdd_core and it appears as if "core" (l3_ick)
> runs at 200 MHz which does not correspond to a valid OPP.

OK

> So to silcence the warning it suffices to remove
> 
> 	omap2_set_init_voltage("core", "l3_ick", "l3_main");
> 
> The question is now what l3_ick has to do with the OPPs at all
> and how it should interwork with OPPs and cpufreq.

So what changed then for iva in your configuration then?

At least I'm getting errors for both for 34xx and dm3730 with
Linux next and reverted commit cf395f7ddb9e ("ARM: OMAP2+: Fix
warnings with broken omap2_set_init_voltage()"):

omap2_set_init_voltage: unable to find boot up OPP for vdd_mpu_iva
omap2_set_init_voltage: unable to set vdd_mpu_iva
omap2_set_init_voltage: unable to find boot up OPP for vdd_core
omap2_set_init_voltage: unable to set vdd_core

Then for fixing this code, seems like this can all happen from
a regular device driver init based on the dts data.. We've had
PM init completely ignore these errors already for years so
whatever dependency there might be seems non-critical :)

> Or does all this mean we may need a second OPP fable for vdd_core
> and 200 MHz? But what would it be good for? I have not seen any
> reference for "core-OPPs" in the TRM.

OK yeah sounds like all the domains need an opp table.

Also, I recall some SoCs having a dependency between having to
run DSP at a lower rate for higher MPU rates, not sure if omap3
has such dependencies though.

Regards,

Tony

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

> Am 03.12.2019 um 16:44 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191203 12:31]:
>> Ok, dev_pm_opp_find_freq_ceil() is doing what it should do and it
>> returns the first OPP higher or equal than the frequency passed in.
>> 
>> The real reason for the warning is that the same OPP table is used
>> for vdd_mpu_iva and vdd_core and it appears as if "core" (l3_ick)
>> runs at 200 MHz which does not correspond to a valid OPP.
> 
> OK
> 
>> So to silcence the warning it suffices to remove
>> 
>> 	omap2_set_init_voltage("core", "l3_ick", "l3_main");
>> 
>> The question is now what l3_ick has to do with the OPPs at all
>> and how it should interwork with OPPs and cpufreq.
> 
> So what changed then for iva in your configuration then?
> 
> At least I'm getting errors for both for 34xx and dm3730 with
> Linux next and reverted commit cf395f7ddb9e ("ARM: OMAP2+: Fix
> warnings with broken omap2_set_init_voltage()"):
> 
> omap2_set_init_voltage: unable to find boot up OPP for vdd_mpu_iva
> omap2_set_init_voltage: unable to set vdd_mpu_iva
> omap2_set_init_voltage: unable to find boot up OPP for vdd_core
> omap2_set_init_voltage: unable to set vdd_core

Hm... Is there maybe a dependency on u-boot?

We are using a quite old version which may boot with vdd_mpu_iva
as 300 MHz while yours may have a different clock.

What we could do is augment the printk (or dev_err) to tell
in these warnings what it is looking for...

	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
	if (IS_ERR(opp)) {
		pr_err("%s: unable to find boot up OPP for vdd_%s freq %ulHz\n",
		__func__, vdd_name, freq);
		goto exit;
	}

> Then for fixing this code, seems like this can all happen from
> a regular device driver init based on the dts data.. We've had
> PM init completely ignore these errors already for years so
> whatever dependency there might be seems non-critical :)
> 
>> Or does all this mean we may need a second OPP fable for vdd_core
>> and 200 MHz? But what would it be good for? I have not seen any
>> reference for "core-OPPs" in the TRM.
> 
> OK yeah sounds like all the domains need an opp table.
> 
> Also, I recall some SoCs having a dependency between having to
> run DSP at a lower rate for higher MPU rates, not sure if omap3
> has such dependencies though.

Well, I not aware of documentation of such dependencies and there
is also some confusion what vdd_mpu_iva exactly is and what vdd_core is.
twl4030 has vdd1 and vdd2 but their relationship isn't clear either.

Maybe Tero or Nisanth can clarify?

BR and thanks,
Nikolaus

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

> Am 03.12.2019 um 16:58 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
>> Am 03.12.2019 um 16:44 schrieb Tony Lindgren <tony@atomide.com>:
>> 
>> * H. Nikolaus Schaller <hns@goldelico.com> [191203 12:31]:
>>> Ok, dev_pm_opp_find_freq_ceil() is doing what it should do and it
>>> returns the first OPP higher or equal than the frequency passed in.
>>> 
>>> The real reason for the warning is that the same OPP table is used
>>> for vdd_mpu_iva and vdd_core and it appears as if "core" (l3_ick)
>>> runs at 200 MHz which does not correspond to a valid OPP.
>> 
>> OK
>> 
>>> So to silcence the warning it suffices to remove
>>> 
>>> 	omap2_set_init_voltage("core", "l3_ick", "l3_main");
>>> 
>>> The question is now what l3_ick has to do with the OPPs at all
>>> and how it should interwork with OPPs and cpufreq.
>> 
>> So what changed then for iva in your configuration then?
>> 
>> At least I'm getting errors for both for 34xx and dm3730 with
>> Linux next and reverted commit cf395f7ddb9e ("ARM: OMAP2+: Fix
>> warnings with broken omap2_set_init_voltage()"):
>> 
>> omap2_set_init_voltage: unable to find boot up OPP for vdd_mpu_iva
>> omap2_set_init_voltage: unable to set vdd_mpu_iva
>> omap2_set_init_voltage: unable to find boot up OPP for vdd_core
>> omap2_set_init_voltage: unable to set vdd_core
> 
> Hm... Is there maybe a dependency on u-boot?
> 
> We are using a quite old version which may boot with vdd_mpu_iva
> as 300 MHz while yours may have a different clock.
> 
> What we could do is augment the printk (or dev_err) to tell
> in these warnings what it is looking for...
> 
> 	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> 	if (IS_ERR(opp)) {
> 		pr_err("%s: unable to find boot up OPP for vdd_%s freq %ulHz\n",
> 		__func__, vdd_name, freq);
> 		goto exit;
> 	}

Easier and always prints info:

	freq = clk_get_rate(clk);
	clk_put(clk);

	pr_info("%s: vdd=%s clk=%s %luHz oh=%s\n", __func__, vdd_name, clk_name, freq, oh_name);

	opp = dev_pm_opp_find_freq_ceil(dev, &freq);

I get this:

[    2.908142] omap2_set_init_voltage: vdd=mpu_iva clk=dpll1_ck 1000000000Hz oh=mpu
[    2.930816] omap2_set_init_voltage: vdd=core clk=l3_ick 200000000Hz oh=l3_main
[    2.946228] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
[    2.953460] omap2_set_init_voltage: unable to set vdd_core

Which means that cpufreq already has increased dpll1_ck to 1 GHz
(I have removed the turbo-mode tags so that it already boots at
full speed) and l3_ick runs at initial 200 MHz.

> 
>> Then for fixing this code, seems like this can all happen from
>> a regular device driver init based on the dts data.. We've had
>> PM init completely ignore these errors already for years so
>> whatever dependency there might be seems non-critical :)
>> 
>>> Or does all this mean we may need a second OPP fable for vdd_core
>>> and 200 MHz? But what would it be good for? I have not seen any
>>> reference for "core-OPPs" in the TRM.
>> 
>> OK yeah sounds like all the domains need an opp table.
>> 
>> Also, I recall some SoCs having a dependency between having to
>> run DSP at a lower rate for higher MPU rates, not sure if omap3
>> has such dependencies though.
> 
> Well, I not aware of documentation of such dependencies and there
> is also some confusion what vdd_mpu_iva exactly is and what vdd_core is.
> twl4030 has vdd1 and vdd2 but their relationship isn't clear either.
> 
> Maybe Tero or Nisanth can clarify?
> 
> BR and thanks,
> Nikolaus
> 
> 
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [191203 16:55]:
> > What we could do is augment the printk (or dev_err) to tell
> > in these warnings what it is looking for...
> > 
> > 	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > 	if (IS_ERR(opp)) {
> > 		pr_err("%s: unable to find boot up OPP for vdd_%s freq %ulHz\n",
> > 		__func__, vdd_name, freq);
> > 		goto exit;
> > 	}
> 
> Easier and always prints info:
> 
> 	freq = clk_get_rate(clk);
> 	clk_put(clk);
> 
> 	pr_info("%s: vdd=%s clk=%s %luHz oh=%s\n", __func__, vdd_name, clk_name, freq, oh_name);
> 
> 	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> 
> I get this:
> 
> [    2.908142] omap2_set_init_voltage: vdd=mpu_iva clk=dpll1_ck 1000000000Hz oh=mpu
> [    2.930816] omap2_set_init_voltage: vdd=core clk=l3_ick 200000000Hz oh=l3_main
> [    2.946228] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
> [    2.953460] omap2_set_init_voltage: unable to set vdd_core

OK yeah that's more descriptive.

> Which means that cpufreq already has increased dpll1_ck to 1 GHz
> (I have removed the turbo-mode tags so that it already boots at
> full speed) and l3_ick runs at initial 200 MHz.

OK. I wonder where this initial code should live though..

Regards,

Tony

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[H. Nikolaus Schaller]

> Am 06.12.2019 um 19:20 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191203 16:55]:
>>> What we could do is augment the printk (or dev_err) to tell
>>> in these warnings what it is looking for...
>>> 
>>> 	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>>> 	if (IS_ERR(opp)) {
>>> 		pr_err("%s: unable to find boot up OPP for vdd_%s freq %ulHz\n",
>>> 		__func__, vdd_name, freq);
>>> 		goto exit;
>>> 	}
>> 
>> Easier and always prints info:
>> 
>> 	freq = clk_get_rate(clk);
>> 	clk_put(clk);
>> 
>> 	pr_info("%s: vdd=%s clk=%s %luHz oh=%s\n", __func__, vdd_name, clk_name, freq, oh_name);
>> 
>> 	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> 
>> I get this:
>> 
>> [    2.908142] omap2_set_init_voltage: vdd=mpu_iva clk=dpll1_ck 1000000000Hz oh=mpu
>> [    2.930816] omap2_set_init_voltage: vdd=core clk=l3_ick 200000000Hz oh=l3_main
>> [    2.946228] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
>> [    2.953460] omap2_set_init_voltage: unable to set vdd_core
> 
> OK yeah that's more descriptive.

So what does your board say as it is also unable to find the initial mpu_iva?
And/or does it show a different core clock?

> 
>> Which means that cpufreq already has increased dpll1_ck to 1 GHz
>> (I have removed the turbo-mode tags so that it already boots at
>> full speed) and l3_ick runs at initial 200 MHz.
> 
> OK. I wonder where this initial code should live though..

Well, we all agree that it should live in deserved retirement :)

But it has some positive effect and maybe "fixes" a bug in cpufreq-only setup.

BR,
Nikolaus

Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

[Andreas Kemnade]

On Mon, 2 Dec 2019 22:09:26 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi Tony,
> 
> > Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > This code is currently unable to find the dts opp tables as ti-cpufreq
> > needs to set them up first based on speed binning.
> > 
> > We stopped initializing the opp tables with platform code years ago for
> > device tree based booting with commit 92d51856d740 ("ARM: OMAP3+: do not
> > register non-dt OPP tables for device tree boot"), and all of mach-omap2
> > is now booting using device tree.
> > 
> > We currently get the following errors on init:
> > 
> > omap2_set_init_voltage: unable to find boot up OPP for vdd_mpu
> > omap2_set_init_voltage: unable to set vdd_mpu
> > omap2_set_init_voltage: unable to find boot up OPP for vdd_core
> > omap2_set_init_voltage: unable to set vdd_core
> > omap2_set_init_voltage: unable to find boot up OPP for vdd_iva
> > omap2_set_init_voltage: unable to set vdd_iva
> > 
> > Let's just drop the unused code. Nowadays ti-cpufreq should be used to
> > to initialize things properly.
> > 
> > Cc: Adam Ford <aford173@gmail.com>
> > Cc: André Roth <neolynx@gmail.com>
> > Cc: "H. Nikolaus Schaller" <hns@goldelico.com>
> > Cc: Nishanth Menon <nm@ti.com>
> > Cc: Tero Kristo <t-kristo@ti.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> > 
> > Guys, please check and ack if we can really do this to get rid of some
> > pointless dmesg -l3 errors without affecting the ongoing cpufreq and
> > voltage work.  
> 
> unfortunately we did not yet test in combination with the 1GHz OPP
> patches for omap3630 (queued for v5.5-rc1) and it appears that this
> patch breaks the 1GHz OPP.
> 
> The symptom is that it works fine on a dm3730 with 800MHz rating
> but results in spurious kernel panics, stack corruption, virtual memory
> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.
> 
I #if 0'ed the 1Ghz opp and found out that the OneNAND DMA timeouts
are independant of 1Ghz. But the result is interesting:
With this patch
xxd /dev/mtd0 shows only ff
without this patch gives content, it is slower.
In both cases I see
[  476.533477] omap2-onenand 4000000.onenand: timeout waiting for DMA

Regards,
Andreas