Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Jon Hunter]

On 10/06/2022 09:20, Viresh Kumar wrote:
> This patch adds support to allow multiple clocks for a device.
> 
> The design is pretty much similar to how this is done for regulators,
> and platforms can supply their own version of the config_clks() callback
> if they have multiple clocks for their device. The core manages the
> calls via opp_table->config_clks() eventually.
> 
> We have kept both "clk" and "clks" fields in the OPP table structure and
> the reason is provided as a comment in _opp_set_clknames(). The same
> isn't done for "rates" though and we use rates[0] at most of the places
> now.
> 
> Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/opp/core.c     | 165 ++++++++++++++++++++++++++++-------------
>   drivers/opp/debugfs.c  |  27 ++++++-
>   drivers/opp/of.c       |  67 +++++++++++++----
>   drivers/opp/opp.h      |  16 ++--
>   include/linux/pm_opp.h |   7 +-
>   5 files changed, 208 insertions(+), 74 deletions(-)


I am seeing the following panic on -next and bisect is point to
this commit ...

[    2.145604] 8<--- cut here ---
[    2.145615] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    2.145625] [00000000] *pgd=00000000
[    2.145647] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
[    2.145660] Modules linked in:
[    2.145671] CPU: 1 PID: 35 Comm: kworker/u8:1 Not tainted 5.19.0-rc3-next-20220622-gf739bc2a47f6 #1
[    2.145688] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    2.145697] Workqueue: events_unbound deferred_probe_work_func
[    2.145740] PC is at 0x0
[    2.145750] LR is at _set_opp+0x194/0x380
[    2.145779] pc : [<00000000>]    lr : [<c086b578>]    psr: 20000013
[    2.145789] sp : f08f1c80  ip : c152cb40  fp : c264c040
[    2.145798] r10: 00000000  r9 : c152cb40  r8 : c16f3010
[    2.145806] r7 : c264c034  r6 : 00000000  r5 : c265d800  r4 : c264c000
[    2.145815] r3 : 00000000  r2 : c265d800  r1 : c264c000  r0 : c16f3010
[    2.145825] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    2.145840] Control: 10c5387d  Table: 8000404a  DAC: 00000051
[    2.145847] Register r0 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
[    2.145883] Register r1 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
[    2.145914] Register r2 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
[    2.145942] Register r3 information: NULL pointer
[    2.145955] Register r4 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
[    2.145983] Register r5 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
[    2.146012] Register r6 information: NULL pointer
[    2.146023] Register r7 information: slab kmalloc-256 start c264c000 pointer offset 52 size 256
[    2.146052] Register r8 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
[    2.146081] Register r9 information: slab task_struct start c152cb40 pointer offset 0
[    2.146105] Register r10 information: NULL pointer
[    2.146116] Register r11 information: slab kmalloc-256 start c264c000 pointer offset 64 size 256
[    2.146146] Register r12 information: slab task_struct start c152cb40 pointer offset 0
[    2.348527] Process kworker/u8:1 (pid: 35, stack limit = 0x(ptrval))
[    2.354896] Stack: (0xf08f1c80 to 0xf08f2000)
[    2.359273] 1c80: 00000001 c0868db0 00000000 7a13d783 c152cb40 c264c000 c16f3010 c265d800
[    2.367471] 1ca0: c2661c40 00000001 c2661c40 00000001 c2661c40 c086b8e8 00000000 7a13d783
[    2.375666] 1cc0: c12ea5a0 c265d800 000f4240 c058cfcc ef7dddec 000f4240 00000000 c2661e88
[    2.383861] 1ce0: 000f4240 000f4240 c2661e98 c068d238 00000000 c2665e00 000f4240 000f4240
[    2.392056] 1d00: c2666258 00000000 c2666000 00000001 c2661c40 c068d15c 00000000 c2666000
[    2.400253] 1d20: c16fd140 00000000 c16fd140 00000000 00000000 c16b78b8 c16b5e00 c068d2d8
[    2.408450] 1d40: c16b7810 c26661d8 c2666000 c068ed98 c2663d00 c2663d00 00000000 c086914c
[    2.416647] 1d60: c2663d00 c16b7810 c068ebec c16b7894 00000004 c0174868 00000000 c16b78b8
[    2.424843] 1d80: c16b5e00 c0684630 c16b7810 c068ebec 00000000 00000004 c0174868 c152cb40
[    2.433041] 1da0: c16b78b8 c0684794 c16b7810 c068ebec 00000000 c068506c 00000a00 c265e040
[    2.441237] 1dc0: c0f5bdd4 00000004 00000000 7a13d783 00000000 c16b7810 c16b7894 00000004
[    2.449434] 1de0: 60000013 00000000 c265e10c c265e154 00000000 c06854c4 c265e040 c16b7800
[    2.457629] 1e00: c16b7810 c152cb40 00000000 c05e42b0 00000001 00000000 ffffffff 00000000
[    2.465824] 1e20: c16b7810 ff8067a4 01000000 7a13d783 c16b7810 c16b7810 00000000 c12f7700
[    2.474020] 1e40: 00000000 00000001 c1367c20 c140f00d c1405800 c067a668 c16b7810 00000000
[    2.482214] 1e60: c12f7700 c0678280 c16b7810 c12f7700 c16b7810 00000017 00000001 c06784e4
[    2.490411] 1e80: c13b6754 f08f1ee4 c16b7810 c0678574 c12f7700 f08f1ee4 c16b7810 c152cb40
[    2.498609] 1ea0: 00000001 c06788bc 00000000 f08f1ee4 c0678834 c067675c c1367c20 c14b4e70
[    2.506804] 1ec0: c1ea60b8 7a13d783 c16b7810 c16b7810 c152cb40 c16b7854 c12fa050 c067810c
[    2.515001] 1ee0: c1400000 c16b7810 00000001 7a13d783 c16b7810 c16b7810 c12fa2f0 c12fa050
[    2.523197] 1f00: 00000000 c0677410 c16b7810 c12fa038 c12fa038 c06778d0 c12fa06c c176a180
[    2.531394] 1f20: c1405800 c140f000 00000000 c013dd2c c152cb40 c1405800 c1203d40 c176a180
[    2.539592] 1f40: c1405800 c140581c c1203d40 c176a198 00000088 c1367177 c1405800 c013e2a4
[    2.547790] 1f60: c0f07434 00000000 f08f1f7c c176f7c0 c152cb40 c013e064 c176a180 c176f640
[    2.555984] 1f80: f0831eb4 00000000 00000000 c01459b4 c176f7c0 c01458ac 00000000 00000000
[    2.564180] 1fa0: 00000000 00000000 00000000 c01001a8 00000000 00000000 00000000 00000000
[    2.572373] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.580569] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    2.588768]  _set_opp from dev_pm_opp_set_opp+0x38/0x78
[    2.594038]  dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x44/0xb0
[    2.602010]  tegra_pmc_core_pd_set_performance_state from _genpd_set_performance_state+0x180/0x1d0
[    2.611018]  _genpd_set_performance_state from _genpd_set_performance_state+0xa4/0x1d0
[    2.618972]  _genpd_set_performance_state from genpd_set_performance_state+0x50/0x7c
[    2.626752]  genpd_set_performance_state from genpd_runtime_resume+0x1ac/0x25c
[    2.634016]  genpd_runtime_resume from __rpm_callback+0x38/0x14c
[    2.640073]  __rpm_callback from rpm_callback+0x50/0x54
[    2.645335]  rpm_callback from rpm_resume+0x394/0x7a0
[    2.650424]  rpm_resume from __pm_runtime_resume+0x4c/0x64
[    2.655947]  __pm_runtime_resume from host1x_probe+0x29c/0x654
[    2.661824]  host1x_probe from platform_probe+0x5c/0xb8
[    2.667080]  platform_probe from really_probe+0xc8/0x2a8
[    2.672433]  really_probe from __driver_probe_device+0x84/0xe4
[    2.678308]  __driver_probe_device from driver_probe_device+0x30/0xd0
[    2.684789]  driver_probe_device from __device_attach_driver+0x88/0xb4
[    2.691350]  __device_attach_driver from bus_for_each_drv+0x54/0xb4
[    2.697647]  bus_for_each_drv from __device_attach+0xf0/0x194
[    2.703430]  __device_attach from bus_probe_device+0x84/0x8c
[    2.709129]  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
[    2.715608]  deferred_probe_work_func from process_one_work+0x214/0x54c
[    2.722269]  process_one_work from worker_thread+0x240/0x508
[    2.727960]  worker_thread from kthread+0x108/0x124
[    2.732872]  kthread from ret_from_fork+0x14/0x2c
[    2.737602] Exception stack(0xf08f1fb0 to 0xf08f1ff8)
[    2.742669] 1fa0:                                     00000000 00000000 00000000 00000000
[    2.750865] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.759061] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.765690] Code: bad PC value
[    2.768990] ---[ end trace 0000000000000000 ]---


Let me know if you have any thoughts.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 3/8] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

[Jon Hunter]

On 10/06/2022 09:20, Viresh Kumar wrote:
> Reuse _opp_compare_key() in _opp_add_static_v2() instead of just
> comparing frequency while finding suspend frequency. Also add a comment
> over _opp_compare_key() explaining its return values.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/opp/core.c | 6 ++++++
>   drivers/opp/of.c   | 4 ++--
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index fe447f41c99e..9f284dc0d9d7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1618,6 +1618,12 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
>   	return true;
>   }
>   
> +/*
> + * Returns
> + * 0: opp1 == opp2
> + * 1: opp1 > opp2
> + * -1: opp1 < opp2
> + */
>   int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
>   {
>   	if (opp1->rate != opp2->rate)
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index bec9644a7260..843923ab9d66 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -929,8 +929,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>   	/* OPP to select on device suspend */
>   	if (of_property_read_bool(np, "opp-suspend")) {
>   		if (opp_table->suspend_opp) {
> -			/* Pick the OPP with higher rate as suspend OPP */
> -			if (new_opp->rate > opp_table->suspend_opp->rate) {
> +			/* Pick the OPP with higher rate/bw/level as suspend OPP */
> +			if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
>   				opp_table->suspend_opp->suspend = false;
>   				new_opp->suspend = true;
>   				opp_table->suspend_opp = new_opp;


FYI ... if I checkout commit 00d776d33da9 ("OPP: Reuse
_opp_compare_key() in _opp_add_static_v2()") from next-20220622
it does not compile ...

drivers/opp/of.c: In function ‘_opp_add_static_v2’:
drivers/opp/of.c:933:25: error: passing argument 1 of ‘_opp_compare_key’ from incompatible pointer type [-Werror=incompatible-pointer-types]
     if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
                          ^~~~~~~~~
In file included from drivers/opp/of.c:22:0:
drivers/opp/opp.h:228:5: note: expected ‘struct dev_pm_opp *’ but argument is of type ‘struct opp_table *’
  int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
      ^~~~~~~~~~~~~~~~
drivers/opp/of.c:933:8: error: too many arguments to function ‘_opp_compare_key’
     if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
         ^~~~~~~~~~~~~~~~
In file included from drivers/opp/of.c:22:0:
drivers/opp/opp.h:228:5: note: declared here
  int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
      ^~~~~~~~~~~~~~~~

This breaks bisecting -next and so would be good to fix this.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 3/8] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

[Viresh Kumar]

On 22-06-22, 14:58, Jon Hunter wrote:
> FYI ... if I checkout commit 00d776d33da9 ("OPP: Reuse
> _opp_compare_key() in _opp_add_static_v2()") from next-20220622
> it does not compile ...
> 
> drivers/opp/of.c: In function ‘_opp_add_static_v2’:
> drivers/opp/of.c:933:25: error: passing argument 1 of ‘_opp_compare_key’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>     if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
>                          ^~~~~~~~~
> In file included from drivers/opp/of.c:22:0:
> drivers/opp/opp.h:228:5: note: expected ‘struct dev_pm_opp *’ but argument is of type ‘struct opp_table *’
>  int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
>      ^~~~~~~~~~~~~~~~
> drivers/opp/of.c:933:8: error: too many arguments to function ‘_opp_compare_key’
>     if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
>         ^~~~~~~~~~~~~~~~
> In file included from drivers/opp/of.c:22:0:
> drivers/opp/opp.h:228:5: note: declared here
>  int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
>      ^~~~~~~~~~~~~~~~
> 
> This breaks bisecting -next and so would be good to fix this.

Yeah, this was reported yesterday and is already fixed in my branch, along with
few more fixes.

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

It hasn't landed into linux-next/master yet though.

-- 
viresh

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 22-06-22, 14:47, Jon Hunter wrote:
> I am seeing the following panic on -next and bisect is point to
> this commit ...
> 
> [    2.145604] 8<--- cut here ---
> [    2.145615] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [    2.145625] [00000000] *pgd=00000000
> [    2.145647] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
> [    2.145660] Modules linked in:
> [    2.145671] CPU: 1 PID: 35 Comm: kworker/u8:1 Not tainted 5.19.0-rc3-next-20220622-gf739bc2a47f6 #1
> [    2.145688] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    2.145697] Workqueue: events_unbound deferred_probe_work_func
> [    2.145740] PC is at 0x0
> [    2.145750] LR is at _set_opp+0x194/0x380
> [    2.145779] pc : [<00000000>]    lr : [<c086b578>]    psr: 20000013
> [    2.145789] sp : f08f1c80  ip : c152cb40  fp : c264c040
> [    2.145798] r10: 00000000  r9 : c152cb40  r8 : c16f3010
> [    2.145806] r7 : c264c034  r6 : 00000000  r5 : c265d800  r4 : c264c000
> [    2.145815] r3 : 00000000  r2 : c265d800  r1 : c264c000  r0 : c16f3010
> [    2.145825] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    2.145840] Control: 10c5387d  Table: 8000404a  DAC: 00000051
> [    2.145847] Register r0 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
> [    2.145883] Register r1 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
> [    2.145914] Register r2 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
> [    2.145942] Register r3 information: NULL pointer
> [    2.145955] Register r4 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
> [    2.145983] Register r5 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
> [    2.146012] Register r6 information: NULL pointer
> [    2.146023] Register r7 information: slab kmalloc-256 start c264c000 pointer offset 52 size 256
> [    2.146052] Register r8 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
> [    2.146081] Register r9 information: slab task_struct start c152cb40 pointer offset 0
> [    2.146105] Register r10 information: NULL pointer
> [    2.146116] Register r11 information: slab kmalloc-256 start c264c000 pointer offset 64 size 256
> [    2.146146] Register r12 information: slab task_struct start c152cb40 pointer offset 0
> [    2.348527] Process kworker/u8:1 (pid: 35, stack limit = 0x(ptrval))
> [    2.354896] Stack: (0xf08f1c80 to 0xf08f2000)
> [    2.359273] 1c80: 00000001 c0868db0 00000000 7a13d783 c152cb40 c264c000 c16f3010 c265d800
> [    2.367471] 1ca0: c2661c40 00000001 c2661c40 00000001 c2661c40 c086b8e8 00000000 7a13d783
> [    2.375666] 1cc0: c12ea5a0 c265d800 000f4240 c058cfcc ef7dddec 000f4240 00000000 c2661e88
> [    2.383861] 1ce0: 000f4240 000f4240 c2661e98 c068d238 00000000 c2665e00 000f4240 000f4240
> [    2.392056] 1d00: c2666258 00000000 c2666000 00000001 c2661c40 c068d15c 00000000 c2666000
> [    2.400253] 1d20: c16fd140 00000000 c16fd140 00000000 00000000 c16b78b8 c16b5e00 c068d2d8
> [    2.408450] 1d40: c16b7810 c26661d8 c2666000 c068ed98 c2663d00 c2663d00 00000000 c086914c
> [    2.416647] 1d60: c2663d00 c16b7810 c068ebec c16b7894 00000004 c0174868 00000000 c16b78b8
> [    2.424843] 1d80: c16b5e00 c0684630 c16b7810 c068ebec 00000000 00000004 c0174868 c152cb40
> [    2.433041] 1da0: c16b78b8 c0684794 c16b7810 c068ebec 00000000 c068506c 00000a00 c265e040
> [    2.441237] 1dc0: c0f5bdd4 00000004 00000000 7a13d783 00000000 c16b7810 c16b7894 00000004
> [    2.449434] 1de0: 60000013 00000000 c265e10c c265e154 00000000 c06854c4 c265e040 c16b7800
> [    2.457629] 1e00: c16b7810 c152cb40 00000000 c05e42b0 00000001 00000000 ffffffff 00000000
> [    2.465824] 1e20: c16b7810 ff8067a4 01000000 7a13d783 c16b7810 c16b7810 00000000 c12f7700
> [    2.474020] 1e40: 00000000 00000001 c1367c20 c140f00d c1405800 c067a668 c16b7810 00000000
> [    2.482214] 1e60: c12f7700 c0678280 c16b7810 c12f7700 c16b7810 00000017 00000001 c06784e4
> [    2.490411] 1e80: c13b6754 f08f1ee4 c16b7810 c0678574 c12f7700 f08f1ee4 c16b7810 c152cb40
> [    2.498609] 1ea0: 00000001 c06788bc 00000000 f08f1ee4 c0678834 c067675c c1367c20 c14b4e70
> [    2.506804] 1ec0: c1ea60b8 7a13d783 c16b7810 c16b7810 c152cb40 c16b7854 c12fa050 c067810c
> [    2.515001] 1ee0: c1400000 c16b7810 00000001 7a13d783 c16b7810 c16b7810 c12fa2f0 c12fa050
> [    2.523197] 1f00: 00000000 c0677410 c16b7810 c12fa038 c12fa038 c06778d0 c12fa06c c176a180
> [    2.531394] 1f20: c1405800 c140f000 00000000 c013dd2c c152cb40 c1405800 c1203d40 c176a180
> [    2.539592] 1f40: c1405800 c140581c c1203d40 c176a198 00000088 c1367177 c1405800 c013e2a4
> [    2.547790] 1f60: c0f07434 00000000 f08f1f7c c176f7c0 c152cb40 c013e064 c176a180 c176f640
> [    2.555984] 1f80: f0831eb4 00000000 00000000 c01459b4 c176f7c0 c01458ac 00000000 00000000
> [    2.564180] 1fa0: 00000000 00000000 00000000 c01001a8 00000000 00000000 00000000 00000000
> [    2.572373] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    2.580569] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [    2.588768]  _set_opp from dev_pm_opp_set_opp+0x38/0x78
> [    2.594038]  dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x44/0xb0
> [    2.602010]  tegra_pmc_core_pd_set_performance_state from _genpd_set_performance_state+0x180/0x1d0
> [    2.611018]  _genpd_set_performance_state from _genpd_set_performance_state+0xa4/0x1d0
> [    2.618972]  _genpd_set_performance_state from genpd_set_performance_state+0x50/0x7c
> [    2.626752]  genpd_set_performance_state from genpd_runtime_resume+0x1ac/0x25c
> [    2.634016]  genpd_runtime_resume from __rpm_callback+0x38/0x14c
> [    2.640073]  __rpm_callback from rpm_callback+0x50/0x54
> [    2.645335]  rpm_callback from rpm_resume+0x394/0x7a0
> [    2.650424]  rpm_resume from __pm_runtime_resume+0x4c/0x64
> [    2.655947]  __pm_runtime_resume from host1x_probe+0x29c/0x654
> [    2.661824]  host1x_probe from platform_probe+0x5c/0xb8
> [    2.667080]  platform_probe from really_probe+0xc8/0x2a8
> [    2.672433]  really_probe from __driver_probe_device+0x84/0xe4
> [    2.678308]  __driver_probe_device from driver_probe_device+0x30/0xd0
> [    2.684789]  driver_probe_device from __device_attach_driver+0x88/0xb4
> [    2.691350]  __device_attach_driver from bus_for_each_drv+0x54/0xb4
> [    2.697647]  bus_for_each_drv from __device_attach+0xf0/0x194
> [    2.703430]  __device_attach from bus_probe_device+0x84/0x8c
> [    2.709129]  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
> [    2.715608]  deferred_probe_work_func from process_one_work+0x214/0x54c
> [    2.722269]  process_one_work from worker_thread+0x240/0x508
> [    2.727960]  worker_thread from kthread+0x108/0x124
> [    2.732872]  kthread from ret_from_fork+0x14/0x2c
> [    2.737602] Exception stack(0xf08f1fb0 to 0xf08f1ff8)
> [    2.742669] 1fa0:                                     00000000 00000000 00000000 00000000
> [    2.750865] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    2.759061] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    2.765690] Code: bad PC value
> [    2.768990] ---[ end trace 0000000000000000 ]---
> 
> 
> Let me know if you have any thoughts.

Maybe I understand what's going on here now, Dmitry too reported this yesterday,
cc'd. Does below fix it for you ?

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2c1ae1286376..a7c7f6f40a7e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1120,9 +1120,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
                }
        }

-       ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
-       if (ret)
-               return ret;
+       if (opp_table->config_clks) {
+               ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
+               if (ret)
+                       return ret;
+       }

        /* Scaling down? Configure required OPPs after frequency */
        if (scaling_down) {

-- 
viresh

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Jon Hunter]

On 22/06/2022 15:15, Viresh Kumar wrote:
> On 22-06-22, 14:47, Jon Hunter wrote:
>> I am seeing the following panic on -next and bisect is point to
>> this commit ...
>>
>> [    2.145604] 8<--- cut here ---
>> [    2.145615] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> [    2.145625] [00000000] *pgd=00000000
>> [    2.145647] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
>> [    2.145660] Modules linked in:
>> [    2.145671] CPU: 1 PID: 35 Comm: kworker/u8:1 Not tainted 5.19.0-rc3-next-20220622-gf739bc2a47f6 #1
>> [    2.145688] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>> [    2.145697] Workqueue: events_unbound deferred_probe_work_func
>> [    2.145740] PC is at 0x0
>> [    2.145750] LR is at _set_opp+0x194/0x380
>> [    2.145779] pc : [<00000000>]    lr : [<c086b578>]    psr: 20000013
>> [    2.145789] sp : f08f1c80  ip : c152cb40  fp : c264c040
>> [    2.145798] r10: 00000000  r9 : c152cb40  r8 : c16f3010
>> [    2.145806] r7 : c264c034  r6 : 00000000  r5 : c265d800  r4 : c264c000
>> [    2.145815] r3 : 00000000  r2 : c265d800  r1 : c264c000  r0 : c16f3010
>> [    2.145825] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> [    2.145840] Control: 10c5387d  Table: 8000404a  DAC: 00000051
>> [    2.145847] Register r0 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
>> [    2.145883] Register r1 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
>> [    2.145914] Register r2 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
>> [    2.145942] Register r3 information: NULL pointer
>> [    2.145955] Register r4 information: slab kmalloc-256 start c264c000 pointer offset 0 size 256
>> [    2.145983] Register r5 information: slab kmalloc-128 start c265d800 pointer offset 0 size 128
>> [    2.146012] Register r6 information: NULL pointer
>> [    2.146023] Register r7 information: slab kmalloc-256 start c264c000 pointer offset 52 size 256
>> [    2.146052] Register r8 information: slab kmalloc-1k start c16f3000 pointer offset 16 size 1024
>> [    2.146081] Register r9 information: slab task_struct start c152cb40 pointer offset 0
>> [    2.146105] Register r10 information: NULL pointer
>> [    2.146116] Register r11 information: slab kmalloc-256 start c264c000 pointer offset 64 size 256
>> [    2.146146] Register r12 information: slab task_struct start c152cb40 pointer offset 0
>> [    2.348527] Process kworker/u8:1 (pid: 35, stack limit = 0x(ptrval))
>> [    2.354896] Stack: (0xf08f1c80 to 0xf08f2000)
>> [    2.359273] 1c80: 00000001 c0868db0 00000000 7a13d783 c152cb40 c264c000 c16f3010 c265d800
>> [    2.367471] 1ca0: c2661c40 00000001 c2661c40 00000001 c2661c40 c086b8e8 00000000 7a13d783
>> [    2.375666] 1cc0: c12ea5a0 c265d800 000f4240 c058cfcc ef7dddec 000f4240 00000000 c2661e88
>> [    2.383861] 1ce0: 000f4240 000f4240 c2661e98 c068d238 00000000 c2665e00 000f4240 000f4240
>> [    2.392056] 1d00: c2666258 00000000 c2666000 00000001 c2661c40 c068d15c 00000000 c2666000
>> [    2.400253] 1d20: c16fd140 00000000 c16fd140 00000000 00000000 c16b78b8 c16b5e00 c068d2d8
>> [    2.408450] 1d40: c16b7810 c26661d8 c2666000 c068ed98 c2663d00 c2663d00 00000000 c086914c
>> [    2.416647] 1d60: c2663d00 c16b7810 c068ebec c16b7894 00000004 c0174868 00000000 c16b78b8
>> [    2.424843] 1d80: c16b5e00 c0684630 c16b7810 c068ebec 00000000 00000004 c0174868 c152cb40
>> [    2.433041] 1da0: c16b78b8 c0684794 c16b7810 c068ebec 00000000 c068506c 00000a00 c265e040
>> [    2.441237] 1dc0: c0f5bdd4 00000004 00000000 7a13d783 00000000 c16b7810 c16b7894 00000004
>> [    2.449434] 1de0: 60000013 00000000 c265e10c c265e154 00000000 c06854c4 c265e040 c16b7800
>> [    2.457629] 1e00: c16b7810 c152cb40 00000000 c05e42b0 00000001 00000000 ffffffff 00000000
>> [    2.465824] 1e20: c16b7810 ff8067a4 01000000 7a13d783 c16b7810 c16b7810 00000000 c12f7700
>> [    2.474020] 1e40: 00000000 00000001 c1367c20 c140f00d c1405800 c067a668 c16b7810 00000000
>> [    2.482214] 1e60: c12f7700 c0678280 c16b7810 c12f7700 c16b7810 00000017 00000001 c06784e4
>> [    2.490411] 1e80: c13b6754 f08f1ee4 c16b7810 c0678574 c12f7700 f08f1ee4 c16b7810 c152cb40
>> [    2.498609] 1ea0: 00000001 c06788bc 00000000 f08f1ee4 c0678834 c067675c c1367c20 c14b4e70
>> [    2.506804] 1ec0: c1ea60b8 7a13d783 c16b7810 c16b7810 c152cb40 c16b7854 c12fa050 c067810c
>> [    2.515001] 1ee0: c1400000 c16b7810 00000001 7a13d783 c16b7810 c16b7810 c12fa2f0 c12fa050
>> [    2.523197] 1f00: 00000000 c0677410 c16b7810 c12fa038 c12fa038 c06778d0 c12fa06c c176a180
>> [    2.531394] 1f20: c1405800 c140f000 00000000 c013dd2c c152cb40 c1405800 c1203d40 c176a180
>> [    2.539592] 1f40: c1405800 c140581c c1203d40 c176a198 00000088 c1367177 c1405800 c013e2a4
>> [    2.547790] 1f60: c0f07434 00000000 f08f1f7c c176f7c0 c152cb40 c013e064 c176a180 c176f640
>> [    2.555984] 1f80: f0831eb4 00000000 00000000 c01459b4 c176f7c0 c01458ac 00000000 00000000
>> [    2.564180] 1fa0: 00000000 00000000 00000000 c01001a8 00000000 00000000 00000000 00000000
>> [    2.572373] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    2.580569] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
>> [    2.588768]  _set_opp from dev_pm_opp_set_opp+0x38/0x78
>> [    2.594038]  dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x44/0xb0
>> [    2.602010]  tegra_pmc_core_pd_set_performance_state from _genpd_set_performance_state+0x180/0x1d0
>> [    2.611018]  _genpd_set_performance_state from _genpd_set_performance_state+0xa4/0x1d0
>> [    2.618972]  _genpd_set_performance_state from genpd_set_performance_state+0x50/0x7c
>> [    2.626752]  genpd_set_performance_state from genpd_runtime_resume+0x1ac/0x25c
>> [    2.634016]  genpd_runtime_resume from __rpm_callback+0x38/0x14c
>> [    2.640073]  __rpm_callback from rpm_callback+0x50/0x54
>> [    2.645335]  rpm_callback from rpm_resume+0x394/0x7a0
>> [    2.650424]  rpm_resume from __pm_runtime_resume+0x4c/0x64
>> [    2.655947]  __pm_runtime_resume from host1x_probe+0x29c/0x654
>> [    2.661824]  host1x_probe from platform_probe+0x5c/0xb8
>> [    2.667080]  platform_probe from really_probe+0xc8/0x2a8
>> [    2.672433]  really_probe from __driver_probe_device+0x84/0xe4
>> [    2.678308]  __driver_probe_device from driver_probe_device+0x30/0xd0
>> [    2.684789]  driver_probe_device from __device_attach_driver+0x88/0xb4
>> [    2.691350]  __device_attach_driver from bus_for_each_drv+0x54/0xb4
>> [    2.697647]  bus_for_each_drv from __device_attach+0xf0/0x194
>> [    2.703430]  __device_attach from bus_probe_device+0x84/0x8c
>> [    2.709129]  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
>> [    2.715608]  deferred_probe_work_func from process_one_work+0x214/0x54c
>> [    2.722269]  process_one_work from worker_thread+0x240/0x508
>> [    2.727960]  worker_thread from kthread+0x108/0x124
>> [    2.732872]  kthread from ret_from_fork+0x14/0x2c
>> [    2.737602] Exception stack(0xf08f1fb0 to 0xf08f1ff8)
>> [    2.742669] 1fa0:                                     00000000 00000000 00000000 00000000
>> [    2.750865] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    2.759061] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [    2.765690] Code: bad PC value
>> [    2.768990] ---[ end trace 0000000000000000 ]---
>>
>>
>> Let me know if you have any thoughts.
> 
> Maybe I understand what's going on here now, Dmitry too reported this yesterday,
> cc'd. Does below fix it for you ?
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2c1ae1286376..a7c7f6f40a7e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1120,9 +1120,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>                  }
>          }
> 
> -       ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
> -       if (ret)
> -               return ret;
> +       if (opp_table->config_clks) {
> +               ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
> +               if (ret)
> +                       return ret;
> +       }
> 
>          /* Scaling down? Configure required OPPs after frequency */
>          if (scaling_down) {
> 


Yes that fixes it thanks!

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Jon

-- 
nvpublic

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Dmitry Osipenko]

On 6/22/22 18:27, Jon Hunter wrote:
> 
> 
> On 22/06/2022 15:15, Viresh Kumar wrote:
>> On 22-06-22, 14:47, Jon Hunter wrote:
>>> I am seeing the following panic on -next and bisect is point to
>>> this commit ...
>>>
>>> [    2.145604] 8<--- cut here ---
>>> [    2.145615] Unable to handle kernel NULL pointer dereference at
>>> virtual address 00000000
>>> [    2.145625] [00000000] *pgd=00000000
>>> [    2.145647] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
>>> [    2.145660] Modules linked in:
>>> [    2.145671] CPU: 1 PID: 35 Comm: kworker/u8:1 Not tainted
>>> 5.19.0-rc3-next-20220622-gf739bc2a47f6 #1
>>> [    2.145688] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>> [    2.145697] Workqueue: events_unbound deferred_probe_work_func
>>> [    2.145740] PC is at 0x0
>>> [    2.145750] LR is at _set_opp+0x194/0x380
>>> [    2.145779] pc : [<00000000>]    lr : [<c086b578>]    psr: 20000013
>>> [    2.145789] sp : f08f1c80  ip : c152cb40  fp : c264c040
>>> [    2.145798] r10: 00000000  r9 : c152cb40  r8 : c16f3010
>>> [    2.145806] r7 : c264c034  r6 : 00000000  r5 : c265d800  r4 :
>>> c264c000
>>> [    2.145815] r3 : 00000000  r2 : c265d800  r1 : c264c000  r0 :
>>> c16f3010
>>> [    2.145825] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM 
>>> Segment none
>>> [    2.145840] Control: 10c5387d  Table: 8000404a  DAC: 00000051
>>> [    2.145847] Register r0 information: slab kmalloc-1k start
>>> c16f3000 pointer offset 16 size 1024
>>> [    2.145883] Register r1 information: slab kmalloc-256 start
>>> c264c000 pointer offset 0 size 256
>>> [    2.145914] Register r2 information: slab kmalloc-128 start
>>> c265d800 pointer offset 0 size 128
>>> [    2.145942] Register r3 information: NULL pointer
>>> [    2.145955] Register r4 information: slab kmalloc-256 start
>>> c264c000 pointer offset 0 size 256
>>> [    2.145983] Register r5 information: slab kmalloc-128 start
>>> c265d800 pointer offset 0 size 128
>>> [    2.146012] Register r6 information: NULL pointer
>>> [    2.146023] Register r7 information: slab kmalloc-256 start
>>> c264c000 pointer offset 52 size 256
>>> [    2.146052] Register r8 information: slab kmalloc-1k start
>>> c16f3000 pointer offset 16 size 1024
>>> [    2.146081] Register r9 information: slab task_struct start
>>> c152cb40 pointer offset 0
>>> [    2.146105] Register r10 information: NULL pointer
>>> [    2.146116] Register r11 information: slab kmalloc-256 start
>>> c264c000 pointer offset 64 size 256
>>> [    2.146146] Register r12 information: slab task_struct start
>>> c152cb40 pointer offset 0
>>> [    2.348527] Process kworker/u8:1 (pid: 35, stack limit = 0x(ptrval))
>>> [    2.354896] Stack: (0xf08f1c80 to 0xf08f2000)
>>> [    2.359273] 1c80: 00000001 c0868db0 00000000 7a13d783 c152cb40
>>> c264c000 c16f3010 c265d800
>>> [    2.367471] 1ca0: c2661c40 00000001 c2661c40 00000001 c2661c40
>>> c086b8e8 00000000 7a13d783
>>> [    2.375666] 1cc0: c12ea5a0 c265d800 000f4240 c058cfcc ef7dddec
>>> 000f4240 00000000 c2661e88
>>> [    2.383861] 1ce0: 000f4240 000f4240 c2661e98 c068d238 00000000
>>> c2665e00 000f4240 000f4240
>>> [    2.392056] 1d00: c2666258 00000000 c2666000 00000001 c2661c40
>>> c068d15c 00000000 c2666000
>>> [    2.400253] 1d20: c16fd140 00000000 c16fd140 00000000 00000000
>>> c16b78b8 c16b5e00 c068d2d8
>>> [    2.408450] 1d40: c16b7810 c26661d8 c2666000 c068ed98 c2663d00
>>> c2663d00 00000000 c086914c
>>> [    2.416647] 1d60: c2663d00 c16b7810 c068ebec c16b7894 00000004
>>> c0174868 00000000 c16b78b8
>>> [    2.424843] 1d80: c16b5e00 c0684630 c16b7810 c068ebec 00000000
>>> 00000004 c0174868 c152cb40
>>> [    2.433041] 1da0: c16b78b8 c0684794 c16b7810 c068ebec 00000000
>>> c068506c 00000a00 c265e040
>>> [    2.441237] 1dc0: c0f5bdd4 00000004 00000000 7a13d783 00000000
>>> c16b7810 c16b7894 00000004
>>> [    2.449434] 1de0: 60000013 00000000 c265e10c c265e154 00000000
>>> c06854c4 c265e040 c16b7800
>>> [    2.457629] 1e00: c16b7810 c152cb40 00000000 c05e42b0 00000001
>>> 00000000 ffffffff 00000000
>>> [    2.465824] 1e20: c16b7810 ff8067a4 01000000 7a13d783 c16b7810
>>> c16b7810 00000000 c12f7700
>>> [    2.474020] 1e40: 00000000 00000001 c1367c20 c140f00d c1405800
>>> c067a668 c16b7810 00000000
>>> [    2.482214] 1e60: c12f7700 c0678280 c16b7810 c12f7700 c16b7810
>>> 00000017 00000001 c06784e4
>>> [    2.490411] 1e80: c13b6754 f08f1ee4 c16b7810 c0678574 c12f7700
>>> f08f1ee4 c16b7810 c152cb40
>>> [    2.498609] 1ea0: 00000001 c06788bc 00000000 f08f1ee4 c0678834
>>> c067675c c1367c20 c14b4e70
>>> [    2.506804] 1ec0: c1ea60b8 7a13d783 c16b7810 c16b7810 c152cb40
>>> c16b7854 c12fa050 c067810c
>>> [    2.515001] 1ee0: c1400000 c16b7810 00000001 7a13d783 c16b7810
>>> c16b7810 c12fa2f0 c12fa050
>>> [    2.523197] 1f00: 00000000 c0677410 c16b7810 c12fa038 c12fa038
>>> c06778d0 c12fa06c c176a180
>>> [    2.531394] 1f20: c1405800 c140f000 00000000 c013dd2c c152cb40
>>> c1405800 c1203d40 c176a180
>>> [    2.539592] 1f40: c1405800 c140581c c1203d40 c176a198 00000088
>>> c1367177 c1405800 c013e2a4
>>> [    2.547790] 1f60: c0f07434 00000000 f08f1f7c c176f7c0 c152cb40
>>> c013e064 c176a180 c176f640
>>> [    2.555984] 1f80: f0831eb4 00000000 00000000 c01459b4 c176f7c0
>>> c01458ac 00000000 00000000
>>> [    2.564180] 1fa0: 00000000 00000000 00000000 c01001a8 00000000
>>> 00000000 00000000 00000000
>>> [    2.572373] 1fc0: 00000000 00000000 00000000 00000000 00000000
>>> 00000000 00000000 00000000
>>> [    2.580569] 1fe0: 00000000 00000000 00000000 00000000 00000013
>>> 00000000 00000000 00000000
>>> [    2.588768]  _set_opp from dev_pm_opp_set_opp+0x38/0x78
>>> [    2.594038]  dev_pm_opp_set_opp from
>>> tegra_pmc_core_pd_set_performance_state+0x44/0xb0
>>> [    2.602010]  tegra_pmc_core_pd_set_performance_state from
>>> _genpd_set_performance_state+0x180/0x1d0
>>> [    2.611018]  _genpd_set_performance_state from
>>> _genpd_set_performance_state+0xa4/0x1d0
>>> [    2.618972]  _genpd_set_performance_state from
>>> genpd_set_performance_state+0x50/0x7c
>>> [    2.626752]  genpd_set_performance_state from
>>> genpd_runtime_resume+0x1ac/0x25c
>>> [    2.634016]  genpd_runtime_resume from __rpm_callback+0x38/0x14c
>>> [    2.640073]  __rpm_callback from rpm_callback+0x50/0x54
>>> [    2.645335]  rpm_callback from rpm_resume+0x394/0x7a0
>>> [    2.650424]  rpm_resume from __pm_runtime_resume+0x4c/0x64
>>> [    2.655947]  __pm_runtime_resume from host1x_probe+0x29c/0x654
>>> [    2.661824]  host1x_probe from platform_probe+0x5c/0xb8
>>> [    2.667080]  platform_probe from really_probe+0xc8/0x2a8
>>> [    2.672433]  really_probe from __driver_probe_device+0x84/0xe4
>>> [    2.678308]  __driver_probe_device from driver_probe_device+0x30/0xd0
>>> [    2.684789]  driver_probe_device from
>>> __device_attach_driver+0x88/0xb4
>>> [    2.691350]  __device_attach_driver from bus_for_each_drv+0x54/0xb4
>>> [    2.697647]  bus_for_each_drv from __device_attach+0xf0/0x194
>>> [    2.703430]  __device_attach from bus_probe_device+0x84/0x8c
>>> [    2.709129]  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
>>> [    2.715608]  deferred_probe_work_func from
>>> process_one_work+0x214/0x54c
>>> [    2.722269]  process_one_work from worker_thread+0x240/0x508
>>> [    2.727960]  worker_thread from kthread+0x108/0x124
>>> [    2.732872]  kthread from ret_from_fork+0x14/0x2c
>>> [    2.737602] Exception stack(0xf08f1fb0 to 0xf08f1ff8)
>>> [    2.742669] 1fa0:                                     00000000
>>> 00000000 00000000 00000000
>>> [    2.750865] 1fc0: 00000000 00000000 00000000 00000000 00000000
>>> 00000000 00000000 00000000
>>> [    2.759061] 1fe0: 00000000 00000000 00000000 00000000 00000013
>>> 00000000
>>> [    2.765690] Code: bad PC value
>>> [    2.768990] ---[ end trace 0000000000000000 ]---
>>>
>>>
>>> Let me know if you have any thoughts.
>>
>> Maybe I understand what's going on here now, Dmitry too reported this
>> yesterday,
>> cc'd. Does below fix it for you ?
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 2c1ae1286376..a7c7f6f40a7e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1120,9 +1120,11 @@ static int _set_opp(struct device *dev, struct
>> opp_table *opp_table,
>>                  }
>>          }
>>
>> -       ret = opp_table->config_clks(dev, opp_table, opp, clk_data,
>> scaling_down);
>> -       if (ret)
>> -               return ret;
>> +       if (opp_table->config_clks) {
>> +               ret = opp_table->config_clks(dev, opp_table, opp,
>> clk_data, scaling_down);
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>          /* Scaling down? Configure required OPPs after frequency */
>>          if (scaling_down) {
>>
> 
> 
> Yes that fixes it thanks!
> 
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Hi Viresh,

Today I noticed that tegra30-devfreq driver now fails to probe because
dev_pm_opp_find_freq_ceil() fails with -ERANGE. This patch is guilty for
that. Could you please take a look?

-- 
Best regards,
Dmitry

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 29-06-22, 21:33, Dmitry Osipenko wrote:
> Today I noticed that tegra30-devfreq driver now fails to probe because
> dev_pm_opp_find_freq_ceil() fails with -ERANGE. This patch is guilty for
> that. Could you please take a look?

I remember this corner case now [1] and it was easy to miss this. So
you want the OPP core to still parse the DT to read opp-hz, but don't
want dev_pm_opp_set_opp() to update the clock rate for it.

What was the reason for this again ?

I have a couple of solutions in mind, but one may be other than second
and so want to know the real issue at hand first.

-- 
viresh

[1] https://lore.kernel.org/lkml/71451eb2-46b2-1ea0-efcc-0811568159a4@gmail.com/

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Dmitry Osipenko]

On 6/30/22 03:50, Viresh Kumar wrote:
> On 29-06-22, 21:33, Dmitry Osipenko wrote:
>> Today I noticed that tegra30-devfreq driver now fails to probe because
>> dev_pm_opp_find_freq_ceil() fails with -ERANGE. This patch is guilty for
>> that. Could you please take a look?
> 
> I remember this corner case now [1] and it was easy to miss this. So
> you want the OPP core to still parse the DT to read opp-hz, but don't
> want dev_pm_opp_set_opp() to update the clock rate for it.
> 
> What was the reason for this again ?
> 
> I have a couple of solutions in mind, but one may be other than second
> and so want to know the real issue at hand first.
> 

We added memory interconnect support to Tegra and since that time only
the memory controller can drive the clock rate. All other drivers,
including the devfreq, now issue memory bandwidth requests using ICC.

In case of the devfreq driver, it's the OPP core that makes the bw
request using ICC.

But it's the set_freq_table() that fails [2], I see
dev_pm_opp_get_opp_count() returns 17, which is correct, and then
dev_pm_opp_find_freq_ceil(freq=0) returns freq=1, which shall be
freq=12750000.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/?id=16e8b2a7cb886bcc3dd89ad28948d374a2319bbc

[2]
https://elixir.bootlin.com/linux/v5.19-rc4/source/drivers/devfreq/devfreq.c#L179

-- 
Best regards,
Dmitry

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 30-06-22, 12:13, Dmitry Osipenko wrote:
> On 6/30/22 03:50, Viresh Kumar wrote:
> > On 29-06-22, 21:33, Dmitry Osipenko wrote:
> >> Today I noticed that tegra30-devfreq driver now fails to probe because
> >> dev_pm_opp_find_freq_ceil() fails with -ERANGE. This patch is guilty for
> >> that. Could you please take a look?
 
> We added memory interconnect support to Tegra and since that time only
> the memory controller can drive the clock rate. All other drivers,
> including the devfreq, now issue memory bandwidth requests using ICC.
> 
> In case of the devfreq driver, it's the OPP core that makes the bw
> request using ICC.
> 
> But it's the set_freq_table() that fails [2], I see
> dev_pm_opp_get_opp_count() returns 17, which is correct, and then
> dev_pm_opp_find_freq_ceil(freq=0) returns freq=1, which shall be
> freq=12750000.

I am confused, you said earlier that it is failing with -ERANGE, but
now it is a bad freq value ?

Which one of these it is ?

The problem I see is here though, because of which I was asking you
the question earlier:

- tegra30-devfreq driver calls devm_pm_opp_of_add_table_noclk(), i.e.
  clk_count == 0.

- _read_rate() (in drivers/opp/of.c) skips reading any opp-hz
  properties if clk_count is 0.

- And so you can get -ERANGE or some other error.

Can you please see where we are failing. Also I don't see how freq can
get set to 1 currently.

-- 
viresh

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Dmitry Osipenko]

On 6/30/22 12:52, Viresh Kumar wrote:
> On 30-06-22, 12:13, Dmitry Osipenko wrote:
>> On 6/30/22 03:50, Viresh Kumar wrote:
>>> On 29-06-22, 21:33, Dmitry Osipenko wrote:
>>>> Today I noticed that tegra30-devfreq driver now fails to probe because
>>>> dev_pm_opp_find_freq_ceil() fails with -ERANGE. This patch is guilty for
>>>> that. Could you please take a look?
>  
>> We added memory interconnect support to Tegra and since that time only
>> the memory controller can drive the clock rate. All other drivers,
>> including the devfreq, now issue memory bandwidth requests using ICC.
>>
>> In case of the devfreq driver, it's the OPP core that makes the bw
>> request using ICC.
>>
>> But it's the set_freq_table() that fails [2], I see
>> dev_pm_opp_get_opp_count() returns 17, which is correct, and then
>> dev_pm_opp_find_freq_ceil(freq=0) returns freq=1, which shall be
>> freq=12750000.
> 
> I am confused, you said earlier that it is failing with -ERANGE, but
> now it is a bad freq value ?
> 
> Which one of these it is ?
> 
> The problem I see is here though, because of which I was asking you
> the question earlier:
> 
> - tegra30-devfreq driver calls devm_pm_opp_of_add_table_noclk(), i.e.
>   clk_count == 0.
> 
> - _read_rate() (in drivers/opp/of.c) skips reading any opp-hz
>   properties if clk_count is 0.
> 
> - And so you can get -ERANGE or some other error.
> 
> Can you please see where we are failing. Also I don't see how freq can
> get set to 1 currently.
> 

The set_freq_table() gets available freqs using
dev_pm_opp_find_freq_ceil() iteration.

The first dev_pm_opp_find_freq_ceil(freq=0) succeeds and returns ceil
freq=1.

The second dev_pm_opp_find_freq_ceil(freq=1) fails with -ERANGE.

I haven't looked yet at why freq is set to 1.

-- 
Best regards,
Dmitry

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 30-06-22, 12:57, Dmitry Osipenko wrote:
> The set_freq_table() gets available freqs using
> dev_pm_opp_find_freq_ceil() iteration.
> 
> The first dev_pm_opp_find_freq_ceil(freq=0) succeeds and returns ceil
> freq=1.

I don't see how this can possibly happen. One possibility is that freq
is set to 0 and one the next loop you do freq++, which can make it 1.

> The second dev_pm_opp_find_freq_ceil(freq=1) fails with -ERANGE.

Even if we send freq = 1, I don't see how we can get ERANGE if the OPP
table is properly initialized.

> I haven't looked yet at why freq is set to 1.

Thanks, but I would need some help to get it debugged.

-- 
viresh

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 30-06-22, 15:45, Viresh Kumar wrote:
> On 30-06-22, 12:57, Dmitry Osipenko wrote:
> > The set_freq_table() gets available freqs using
> > dev_pm_opp_find_freq_ceil() iteration.
> > 
> > The first dev_pm_opp_find_freq_ceil(freq=0) succeeds and returns ceil
> > freq=1.
> 
> I don't see how this can possibly happen. One possibility is that freq
> is set to 0 and one the next loop you do freq++, which can make it 1.
> 
> > The second dev_pm_opp_find_freq_ceil(freq=1) fails with -ERANGE.
> 
> Even if we send freq = 1, I don't see how we can get ERANGE if the OPP
> table is properly initialized.
> 
> > I haven't looked yet at why freq is set to 1.
> 
> Thanks, but I would need some help to get it debugged.

Hi Dmitry,

I am looking to send another version of this now and soon merge this
in for 5.20-rc1. Can you please help figure out what's going on here ?

Thanks.

-- 
viresh

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Dmitry Osipenko]

On 7/4/22 15:09, Viresh Kumar wrote:
> On 30-06-22, 15:45, Viresh Kumar wrote:
>> On 30-06-22, 12:57, Dmitry Osipenko wrote:
>>> The set_freq_table() gets available freqs using
>>> dev_pm_opp_find_freq_ceil() iteration.
>>>
>>> The first dev_pm_opp_find_freq_ceil(freq=0) succeeds and returns ceil
>>> freq=1.
>>
>> I don't see how this can possibly happen. One possibility is that freq
>> is set to 0 and one the next loop you do freq++, which can make it 1.
>>
>>> The second dev_pm_opp_find_freq_ceil(freq=1) fails with -ERANGE.
>>
>> Even if we send freq = 1, I don't see how we can get ERANGE if the OPP
>> table is properly initialized.
>>
>>> I haven't looked yet at why freq is set to 1.
Actually the freq was 0 and it was 1 on the next loop like you suggested.

>> Thanks, but I would need some help to get it debugged.
> 
> Hi Dmitry,
> 
> I am looking to send another version of this now and soon merge this
> in for 5.20-rc1. Can you please help figure out what's going on here ?

Previously, the _read_opp_key() was always reading the opp-hz. Now it
skips reading the rates in _read_rate() because opp_table->clk_count=0
for the tegra30-devfreq driver the uses devm_pm_opp_of_add_table_noclk().

-- 
Best regards,
Dmitry

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 04-07-22, 16:17, Dmitry Osipenko wrote:
> Actually the freq was 0 and it was 1 on the next loop like you suggested.
> 
> Previously, the _read_opp_key() was always reading the opp-hz. Now it
> skips reading the rates in _read_rate() because opp_table->clk_count=0
> for the tegra30-devfreq driver the uses devm_pm_opp_of_add_table_noclk().

This is exactly what I wrote in an earlier email :)

Anyway, I have pushed two patches on top of my opp/linux-next branch
and they should fix it in a good way now I suppose. Can you please
give that a try.

This is how the diff looks like:

PM / devfreq: tegra30: Register config_clks helper

There is a corner case with Tegra30, where we want to skip clk
configuration via dev_pm_opp_set_opp(), but still want the OPP core to
read the "opp-hz" property so we can find the right OPP via freq finding
helpers.

The OPP core provides support for the platforms to provide config_clks
helpers now, lets use them instead of devm_pm_opp_of_add_table_noclk()
to achieve the same result, as the OPP core won't parse the DT's
"opp-hz" property if the clock isn't provided.

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 65ecf17a36f4..0e0a4058f45c 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -821,6 +821,15 @@ static int devm_tegra_devfreq_init_hw(struct device *dev,
 	return err;
 }
 
+static int tegra_devfreq_config_clks_nop(struct device *dev,
+					 struct opp_table *opp_table,
+					 struct dev_pm_opp *opp, void *data,
+					 bool scaling_down)
+{
+	/* We want to skip clk configuration via dev_pm_opp_set_opp() */
+	return 0;
+}
+
 static int tegra_devfreq_probe(struct platform_device *pdev)
 {
 	u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
@@ -830,6 +839,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	unsigned int i;
 	long rate;
 	int err;
+	const char *clk_names[] = { "actmon", NULL };
+	struct dev_pm_opp_config config = {
+		.supported_hw = &hw_version,
+		.supported_hw_count = 1,
+		.clk_names = clk_names,
+		.config_clks = tegra_devfreq_config_clks_nop,
+	};
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
 	if (!tegra)
@@ -874,13 +890,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = devm_pm_opp_set_supported_hw(&pdev->dev, &hw_version, 1);
+	err = devm_pm_opp_set_config(&pdev->dev, &config);
 	if (err) {
-		dev_err(&pdev->dev, "Failed to set supported HW: %d\n", err);
+		dev_err(&pdev->dev, "Failed to set OPP config: %d\n", err);
 		return err;
 	}
 
-	err = devm_pm_opp_of_add_table_noclk(&pdev->dev, 0);
+	err = devm_pm_opp_of_add_table_indexed(&pdev->dev, 0);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
 		return err;
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 94c19e9b8cbf..03283ada3341 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2142,7 +2142,7 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 		count = 1;
 
 	/* Fail early for invalid configurations */
-	if (!count || (config_clks && count == 1) || (!config_clks && count > 1))
+	if (!count || (!config_clks && count > 1))
 		return -EINVAL;
 
 	/* Another CPU that shares the OPP table has set the clkname ? */
@@ -2168,10 +2168,12 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 	}
 
 	opp_table->clk_count = count;
+	opp_table->config_clks = config_clks;
 
 	/* Set generic single clk set here */
 	if (count == 1) {
-		opp_table->config_clks = _opp_config_clk_single;
+		if (!opp_table->config_clks)
+			opp_table->config_clks = _opp_config_clk_single;
 
 		/*
 		 * We could have just dropped the "clk" field and used "clks"
@@ -2186,8 +2188,6 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 		 * too.
 		 */
 		opp_table->clk = opp_table->clks[0];
-	} else {
-		opp_table->config_clks = config_clks;
 	}
 
 	return 0;

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Dmitry Osipenko]

On 7/4/22 18:52, Viresh Kumar wrote:
> On 04-07-22, 16:17, Dmitry Osipenko wrote:
>> Actually the freq was 0 and it was 1 on the next loop like you suggested.
>>
>> Previously, the _read_opp_key() was always reading the opp-hz. Now it
>> skips reading the rates in _read_rate() because opp_table->clk_count=0
>> for the tegra30-devfreq driver the uses devm_pm_opp_of_add_table_noclk().
> 
> This is exactly what I wrote in an earlier email :)
> 
> Anyway, I have pushed two patches on top of my opp/linux-next branch
> and they should fix it in a good way now I suppose. Can you please
> give that a try.
> 
> This is how the diff looks like:
> 
> PM / devfreq: tegra30: Register config_clks helper
> 
> There is a corner case with Tegra30, where we want to skip clk
> configuration via dev_pm_opp_set_opp(), but still want the OPP core to
> read the "opp-hz" property so we can find the right OPP via freq finding
> helpers.
> 
> The OPP core provides support for the platforms to provide config_clks
> helpers now, lets use them instead of devm_pm_opp_of_add_table_noclk()
> to achieve the same result, as the OPP core won't parse the DT's
> "opp-hz" property if the clock isn't provided.

Works, thanks you!

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH 5/8] OPP: Allow multiple clocks for a device

[Viresh Kumar]

On 04-07-22, 21:04, Dmitry Osipenko wrote:
> On 7/4/22 18:52, Viresh Kumar wrote:
> > On 04-07-22, 16:17, Dmitry Osipenko wrote:
> >> Actually the freq was 0 and it was 1 on the next loop like you suggested.
> >>
> >> Previously, the _read_opp_key() was always reading the opp-hz. Now it
> >> skips reading the rates in _read_rate() because opp_table->clk_count=0
> >> for the tegra30-devfreq driver the uses devm_pm_opp_of_add_table_noclk().
> > 
> > This is exactly what I wrote in an earlier email :)
> > 
> > Anyway, I have pushed two patches on top of my opp/linux-next branch
> > and they should fix it in a good way now I suppose. Can you please
> > give that a try.
> > 
> > This is how the diff looks like:
> > 
> > PM / devfreq: tegra30: Register config_clks helper
> > 
> > There is a corner case with Tegra30, where we want to skip clk
> > configuration via dev_pm_opp_set_opp(), but still want the OPP core to
> > read the "opp-hz" property so we can find the right OPP via freq finding
> > helpers.
> > 
> > The OPP core provides support for the platforms to provide config_clks
> > helpers now, lets use them instead of devm_pm_opp_of_add_table_noclk()
> > to achieve the same result, as the OPP core won't parse the DT's
> > "opp-hz" property if the clock isn't provided.
> 
> Works, thanks you!
> 
> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Finally, thanks a lot Dmitry :)

-- 
viresh