Re: [PATCH v2 17/17] ARM: dts: tegra30: cardhu-a04: Add CPU Operating Performance Points

[Jon Hunter]

On 15/11/2019 14:55, Dmitry Osipenko wrote:
> 15.11.2019 15:52, Jon Hunter ÐÐÑÐÑ:
>>
>> On 13/11/2019 13:57, Dmitry Osipenko wrote:
>>> Hello Jon,
>>>
>>> 13.11.2019 09:52, Jon Hunter ÐÐÑÐÑ:
>>>>
>>>> On 24/10/2019 23:14, Dmitry Osipenko wrote:
>>>>> Utilize common Tegra30 CPU OPP table. CPU DVFS is available now on
>>>>> cardhu-a04.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>> ---
>>>>>  arch/arm/boot/dts/tegra30-cardhu-a04.dts | 24 ++++++++++++++++++++++++
>>>>>  1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/tegra30-cardhu-a04.dts b/arch/arm/boot/dts/tegra30-cardhu-a04.dts
>>>>> index 0d71925d4f0b..9234988624ec 100644
>>>>> --- a/arch/arm/boot/dts/tegra30-cardhu-a04.dts
>>>>> +++ b/arch/arm/boot/dts/tegra30-cardhu-a04.dts
>>>>> @@ -2,6 +2,8 @@
>>>>>  /dts-v1/;
>>>>>  
>>>>>  #include "tegra30-cardhu.dtsi"
>>>>> +#include "tegra30-cpu-opp.dtsi"
>>>>> +#include "tegra30-cpu-opp-microvolt.dtsi"
>>>>>  
>>>>>  /* This dts file support the cardhu A04 and later versions of board */
>>>>>  
>>>>> @@ -127,4 +129,26 @@
>>>>>  			nvidia,tegra-core-regulator;
>>>>>  		};
>>>>>  	};
>>>>> +
>>>>> +	cpus {
>>>>> +		cpu0: cpu@0 {
>>>>> +			cpu-supply = <&vddctrl_reg>;
>>>>> +			operating-points-v2 = <&cpu0_opp_table>;
>>>>> +		};
>>>>> +
>>>>> +		cpu@1 {
>>>>> +			cpu-supply = <&vddctrl_reg>;
>>>>> +			operating-points-v2 = <&cpu0_opp_table>;
>>>>> +		};
>>>>> +
>>>>> +		cpu@2 {
>>>>> +			cpu-supply = <&vddctrl_reg>;
>>>>> +			operating-points-v2 = <&cpu0_opp_table>;
>>>>> +		};
>>>>> +
>>>>> +		cpu@3 {
>>>>> +			cpu-supply = <&vddctrl_reg>;
>>>>> +			operating-points-v2 = <&cpu0_opp_table>;
>>>>> +		};
>>>>> +	};
>>>>>  };
>>>>
>>>> Sorry for not testing this sooner, but this is generating the
>>>> following WARNING on boot ...
>>>>
>>>> [    2.916019] ------------[ cut here ]------------
>>>> [    2.920669] WARNING: CPU: 2 PID: 1 at /dvs/git/dirty/git-master_l4t-upstream/kernel/drivers/opp/of.c:688 _of_add_opp_table_v2.part.2+0x45c/0x4d4
>>>> [    2.933713] Modules linked in:
>>>> [    2.936785] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.4.0-rc7-next-20191112-gfc6d6db1df2c #1
>>>> [    2.945403] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>>> [    2.951706] [<c0112924>] (unwind_backtrace) from [<c010c9d0>] (show_stack+0x10/0x14)
>>>> [    2.959467] [<c010c9d0>] (show_stack) from [<c0aa4494>] (dump_stack+0xc0/0xd4)
>>>> [    2.966707] [<c0aa4494>] (dump_stack) from [<c0124750>] (__warn+0xe0/0xf8)
>>>> [    2.973593] [<c0124750>] (__warn) from [<c0124818>] (warn_slowpath_fmt+0xb0/0xb8)
>>>> [    2.981090] [<c0124818>] (warn_slowpath_fmt) from [<c0754be0>] (_of_add_opp_table_v2.part.2+0x45c/0x4d4)
>>>> [    2.990583] [<c0754be0>] (_of_add_opp_table_v2.part.2) from [<c0754c98>] (dev_pm_opp_of_add_table+0x40/0x15c)
>>>> [    3.000508] [<c0754c98>] (dev_pm_opp_of_add_table) from [<c0754de8>] (dev_pm_opp_of_cpumask_add_table+0x34/0xb4)
>>>> [    3.010704] [<c0754de8>] (dev_pm_opp_of_cpumask_add_table) from [<c075b058>] (cpufreq_init+0xf8/0x2cc)
>>>> [    3.020024] [<c075b058>] (cpufreq_init) from [<c0758758>] (cpufreq_online+0x260/0x824)
>>>> [    3.027953] [<c0758758>] (cpufreq_online) from [<c0758d98>] (cpufreq_add_dev+0x6c/0x78)
>>>> [    3.035976] [<c0758d98>] (cpufreq_add_dev) from [<c05b3188>] (subsys_interface_register+0xa0/0xec)
>>>> [    3.044951] [<c05b3188>] (subsys_interface_register) from [<c07574d4>] (cpufreq_register_driver+0x14c/0x20c)
>>>> [    3.054792] [<c07574d4>] (cpufreq_register_driver) from [<c075aee0>] (dt_cpufreq_probe+0x94/0x114)
>>>> [    3.063771] [<c075aee0>] (dt_cpufreq_probe) from [<c05b6a88>] (platform_drv_probe+0x48/0x98)
>>>> [    3.072225] [<c05b6a88>] (platform_drv_probe) from [<c05b4a38>] (really_probe+0x234/0x34c)
>>>> [    3.080502] [<c05b4a38>] (really_probe) from [<c05b4cc8>] (driver_probe_device+0x60/0x168)
>>>> [    3.088780] [<c05b4cc8>] (driver_probe_device) from [<c05b4f78>] (device_driver_attach+0x58/0x60)
>>>> [    3.097664] [<c05b4f78>] (device_driver_attach) from [<c05b5000>] (__driver_attach+0x80/0xbc)
>>>> [    3.106200] [<c05b5000>] (__driver_attach) from [<c05b2db0>] (bus_for_each_dev+0x74/0xb4)
>>>> [    3.114389] [<c05b2db0>] (bus_for_each_dev) from [<c05b3da4>] (bus_add_driver+0x164/0x1e8)
>>>> [    3.122666] [<c05b3da4>] (bus_add_driver) from [<c05b5b54>] (driver_register+0x7c/0x114)
>>>> [    3.130774] [<c05b5b54>] (driver_register) from [<c010306c>] (do_one_initcall+0x54/0x2a8)
>>>> [    3.138974] [<c010306c>] (do_one_initcall) from [<c0f01040>] (kernel_init_freeable+0x14c/0x1e8)
>>>> [    3.147695] [<c0f01040>] (kernel_init_freeable) from [<c0abbe88>] (kernel_init+0x8/0x10c)
>>>> [    3.155887] [<c0abbe88>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
>>>> [    3.163462] Exception stack(0xef0c9fb0 to 0xef0c9ff8)
>>>> [    3.168519] 9fa0:                                     00000000 00000000 00000000 00000000
>>>> [    3.176706] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> [    3.184893] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>>>> [    3.191695] ---[ end trace a7dc36f7a4ddbdb2 ]---
>>>> [    3.197855] ------------[ cut here ]------------
>>>>
>>>> Let me know if you can take a look at this.
>>>
>>> The warning happens because Cardhu now has CPU OPPs in the device-tree,
>>> but supported_hw isn't set for the OPPs and thus the count of available
>>> OPPs is 0.
>>>
>>> This is expected to happen because patch "cpufreq: tegra20: Use generic
>>> cpufreq-dt driver (Tegra30 supported now)" isn't applied yet.
>>>
>>> It is possible to factor out the blacklisting of Tegra SoCs in
>>> cpufreq_dt_platdev_init() into a separate patch and request backporting
>>> of that change in order to avoid the warning noise for older kernel
>>> versions + newer device-tree. Please let me know if you think that it's
>>> worth to do the separation.
>>
>> Unfortunately, I think we are going to need to drop this patch. Booting
>> Tegra30-cardhu-a04 with Thierry's for-5.5/arm/dt branch does not even
>> boot. There is no crash log but it hangs on boot. This patch appears to
>> be the culprit. What is odd is that Tegra30-cardhu-a04 boots fine with
>> Thierry's for-next branch which includes this. However, this is causing
>> lots of bisect problems. Updating the DT shouldn't break the boot.
> The diff between "for-5.5/arm/dt" and "for-next" is quite small, you
> only need to try to revert one or two patches. Seems the only major
> change which could be somehow related to CPUFreq is that
> "for-5.5/arm/dt" doesn't have regulators coupling support, but I don't
> see how it could cause any effect since CPUFreq just doesn't load due to
> a missing setup of supported_hw.
> 
> Unfortunately for now I can't reproduce the hang with a similar setup
> that has OPPs in DT, doesn't have Tegra CPUFreq driver, doesn't
> blacklist Tegra in cpufreq-dt-platdev.c and doesn't have regulators
> coupling. I see the warning splats on boot and everything appears to be
> working fine.
> 
> It's likely that the hang is caused by something else than this patch.

No, it is definitely this patch. 100% reproducible.

> It's also a bit odd that you're accusing this patch while apparently it
> worked a day before.

No, the behaviour on top of -next is different to the "for-5.5/arm/dt"
branch, which is what I am saying.

> Please give a try to the recent upstream linux-next. If it works, then
> probably there is nothing to worry about.

No, there is plenty to be concerned about, with the latest -next we are
getting these WARN splats on boot, which we need to avoid.

> I'll factor out the blacklisting change and add it as a separate patch
> into the next revision of the series, marking the patch as "fixes" and
> "stable" in order to get rid of the warning on stable kernels.
> 
> I'm also fine with reverting of the DT changes for now, if you prefer
> that. But again, this DT change shouldn't cause any critical problems.

No it should not, but it does.

> I need a detailed report or a way to reproduce the problem in order to
> solve it, please try to collect some more details. Try to compile the
> cpufreq-dt driver as a loadable module, enable tracing of clk and
> regulator events (tp_printk
> trace_event=regulator_set_voltage,clk_set_rate,clk_set_parent).

I add some debug prints and on boot with "for-5.5/arm/dt" is dies while
bringing CPU0 online for cpufreq ...

[    2.863369] cpufreq_dt: dt_cpufreq_probe-347
[    2.867917] cpufreq_dt: dt_cpufreq_probe-360
[    2.872195] cpufreq_dt: dt_cpufreq_probe-370
[    2.876509] cpufreq: cpufreq_register_driver-2632
[    2.881219] cpufreq: cpufreq_register_driver-2636
[    2.885989] cpufreq: trying to register driver cpufreq-dt
[    2.891395] cpufreq: cpufreq_register_driver-2651
[    2.896131] cpufreq: cpufreq_register_driver-2661
[    2.900839] cpufreq: cpufreq_register_driver-2665
[    2.905610] cpufreq: cpufreq_register_driver-2672
[    2.910329] cpu cpu0: cpufreq_add_dev: adding CPU0
[    2.915152] cpufreq: cpufreq_online: bringing CPU0 online

So I think it is best to revert this for now.

Jon

-- 
nvpublic