[PATCH V2 00/13] OPP: Add support for multiple clocks*

[PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

Hello,

This patchset adds support for devices with multiple clocks. None of the clocks
is considered primary in this case and all are handled equally.

The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
to set the specific OPP. Though how they find the target OPP is left for the
users to handle. For some, we may have another unique OPP property, like level,
which can be used to find the OPP. While in case of others, we may want to
implement freq-based OPP finder APIs for multiple clock rates. I have decided
not to implement them in advance, and add them only someone wants to use them.

This is rebased over a lot of other OPP changes and is pushed here:

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

V1->V2:

- Fix broken git bisect for:
  OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

- Include binding changes written by Krzysztof earlier.

- Check config_clks before calling it, it isn't always set.

- Add config_clks for Tegra30's devfreq to handle its corner case.

- _opp_compare_key() supports multi-clk case now, earlier it skipped freq
  comparison for such a case.

- New patch to compare all bandwidth values as well in _opp_compare_key().

- New patch to remove *_noclk() interface.

- Various other minor fixes.

--
Viresh

Krzysztof Kozlowski (1):
  dt-bindings: opp: accept array of frequencies

Viresh Kumar (12):
  OPP: Use consistent names for OPP table instances
  OPP: Remove rate_not_available parameter to _opp_add()
  OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
  OPP: Make dev_pm_opp_set_opp() independent of frequency
  OPP: Allow multiple clocks for a device
  OPP: Compare bandwidths for all paths in _opp_compare_key()
  OPP: Add key specific assert() method to key finding helpers
  OPP: Assert clk_count == 1 for single clk helpers
  OPP: Provide a simple implementation to configure multiple clocks
  OPP: Allow config_clks helper for single clk case
  PM / devfreq: tegra30: Register config_clks helper
  OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

 .../devicetree/bindings/opp/opp-v2-base.yaml  |  10 +
 drivers/devfreq/tegra30-devfreq.c             |  22 +-
 drivers/opp/core.c                            | 404 +++++++++++++-----
 drivers/opp/cpu.c                             |  12 +-
 drivers/opp/debugfs.c                         |  27 +-
 drivers/opp/of.c                              | 139 +++---
 drivers/opp/opp.h                             |  24 +-
 include/linux/pm_opp.h                        |  29 +-
 8 files changed, 466 insertions(+), 201 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 05/13] dt-bindings: opp: accept array of frequencies

[Viresh Kumar]

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Devices might need to control several clocks when scaling the frequency
and voltage.  Allow passing array of clock frequencies, similarly to the
voltages.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 76c8acd981b3..66d0ec763f0b 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -50,6 +50,16 @@ select: false
           property to uniquely identify the OPP nodes exists. Devices like power
           domains must have another (implementation dependent) property.
 
+          Entries for multiple clocks shall be provided in the same field, as
+          array of frequencies.  The OPP binding doesn't provide any provisions
+          to relate the values to their clocks or the order in which the clocks
+          need to be configured and that is left for the implementation
+          specific binding.
+        minItems: 1
+        maxItems: 16
+        items:
+          maxItems: 1
+
       opp-microvolt:
         description: |
           Voltage for the OPP
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/5/22 10:00, Viresh Kumar wrote:
> Hello,
> 
> This patchset adds support for devices with multiple clocks. None of the clocks
> is considered primary in this case and all are handled equally.
> 
> The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
> to set the specific OPP. Though how they find the target OPP is left for the
> users to handle. For some, we may have another unique OPP property, like level,
> which can be used to find the OPP. While in case of others, we may want to
> implement freq-based OPP finder APIs for multiple clock rates. I have decided
> not to implement them in advance, and add them only someone wants to use them.
> 
> This is rebased over a lot of other OPP changes and is pushed here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next
> 
> V1->V2:
> 
> - Fix broken git bisect for:
>   OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
> 
> - Include binding changes written by Krzysztof earlier.
> 
> - Check config_clks before calling it, it isn't always set.
> 
> - Add config_clks for Tegra30's devfreq to handle its corner case.
> 
> - _opp_compare_key() supports multi-clk case now, earlier it skipped freq
>   comparison for such a case.
> 
> - New patch to compare all bandwidth values as well in _opp_compare_key().
> 
> - New patch to remove *_noclk() interface.
> 
> - Various other minor fixes.
> 
> --
> Viresh
> 
> Krzysztof Kozlowski (1):
>   dt-bindings: opp: accept array of frequencies
> 
> Viresh Kumar (12):
>   OPP: Use consistent names for OPP table instances
>   OPP: Remove rate_not_available parameter to _opp_add()
>   OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
>   OPP: Make dev_pm_opp_set_opp() independent of frequency
>   OPP: Allow multiple clocks for a device
>   OPP: Compare bandwidths for all paths in _opp_compare_key()
>   OPP: Add key specific assert() method to key finding helpers
>   OPP: Assert clk_count == 1 for single clk helpers
>   OPP: Provide a simple implementation to configure multiple clocks
>   OPP: Allow config_clks helper for single clk case
>   PM / devfreq: tegra30: Register config_clks helper

Hello Viresh,

This patch breaks Tegra again, please take a look:

   OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

 8<--- cut here ---
 Unable to handle kernel paging request at virtual address ffffffff
 [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
 Internal error: Oops: 37 [#1] PREEMPT SMP ARM
 Modules linked in:
 CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
5.19.0-rc1-00040-g30b62d123f4f #82
 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
 Workqueue: events_unbound deferred_probe_work_func
 PC is at _opp_compare_key+0x40/0xc4
 LR is at 0xfffffffb
 pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
 sp : df831b08  ip : c33cd4d0  fp : df831b24
 r10: c2586078  r9 : c258606c  r8 : 00000000
 r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
 r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
 Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
 Control: 10c5387d  Table: 8000404a  DAC: 00000051
...
 Backtrace:
  _opp_compare_key from _set_opp+0x80/0x408
  r7:00000000 r6:c27c0010 r5:c33cd480 r4:c2586000
  _set_opp from dev_pm_opp_set_opp+0x74/0xdc
  r10:00000001 r9:000f4240 r8:c33b6840 r7:c33b6840 r6:c33cd480 r5:c27c0010
  r4:c2586000
  dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x54/0xbc
  r6:c33b6840 r5:c1a21760 r4:c33cd480
  tegra_pmc_core_pd_set_performance_state from
_genpd_set_performance_state+0x1f0/0x280
  r6:c33b6b58 r5:c33b6b48 r4:000f4240
  _genpd_set_performance_state from _genpd_set_performance_state+0xb4/0x280
  r10:00000001 r9:000f4240 r8:c33b9800 r7:c33b6840 r6:c33b9b18 r5:c33d0040
  r4:000f4240
  _genpd_set_performance_state from genpd_set_performance_state+0xb8/0xd4
  r10:c33b9a98 r9:c33d0400 r8:00000000 r7:00000000 r6:c33b9800 r5:00000000
  r4:c33d0400
  genpd_set_performance_state from genpd_runtime_resume+0x22c/0x240
  r5:00000000 r4:c27c0810
  genpd_runtime_resume from __rpm_callback+0x4c/0x1ac
  r10:c27ba8bc r9:00000000 r8:c27c0960 r7:c27c08cc r6:c27ba800 r5:c09611b8
  r4:c27c0810
  __rpm_callback from rpm_callback+0x60/0x64
  r9:df831ce4 r8:c27c0960 r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810
  rpm_callback from rpm_resume+0x480/0x7e0
  r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810
  rpm_resume from __pm_runtime_resume+0x58/0xb0
  r10:00000000 r9:c2587194 r8:c2587210 r7:c27c0810 r6:c27c08cc r5:60000113
  r4:c27c0810
  __pm_runtime_resume from host1x_probe+0x3d4/0x6d4
  r7:c27c0810 r6:c27c0800 r5:00000000 r4:c2587040
  host1x_probe from platform_probe+0x6c/0xc0
  r10:c191438c r9:c1aa8e20 r8:0000000d r7:c27c0810 r6:c1a2d354 r5:c27c0810
  r4:00000000
  platform_probe from really_probe.part.0+0xac/0x2c0
  r7:c27c0810 r6:c1a2d354 r5:c27c0810 r4:00000000
  really_probe.part.0 from __driver_probe_device+0xb8/0x14c
  r7:c27c0810 r6:c1a2d354 r5:00000000 r4:c27c0810
  __driver_probe_device from driver_probe_device+0x44/0x11c
  r7:c27c0810 r6:c27c0810 r5:c2137c58 r4:c2137c54
  driver_probe_device from __device_attach_driver+0xc8/0x10c
  r9:c1aa8e20 r8:c242c000 r7:00000000 r6:c27c0810 r5:df831e6c r4:c1a2d354
  __device_attach_driver from bus_for_each_drv+0x90/0xdc
  r7:00000000 r6:c09440f8 r5:df831e6c r4:00000000
  bus_for_each_drv from __device_attach+0xbc/0x1d4
  r6:c27c0854 r5:00000001 r4:c27c0810
  __device_attach from device_initial_probe+0x1c/0x20
  r6:c1a30df8 r5:c27c0810 r4:c27c0810
  device_initial_probe from bus_probe_device+0x98/0xa0
  bus_probe_device from deferred_probe_work_func+0x8c/0xbc
  r7:00000000 r6:c1a309e8 r5:c1a3099c r4:c27c0810
  deferred_probe_work_func from process_one_work+0x2b8/0x774
  r7:c25c8000 r6:c2407000 r5:c2557480 r4:c1a309f8
  process_one_work from worker_thread+0x17c/0x56c
  r10:00000088 r9:c25c8000 r8:c1905d40 r7:c240703c r6:c2557498 r5:c2407000
  r4:c2557480
  worker_thread from kthread+0x108/0x13c
  r10:00000000 r9:df815e2c r8:c2557500 r7:c2557480 r6:c0151924 r5:c25c8000
  r4:c2554540
  kthread from ret_from_fork+0x14/0x28
 Exception stack(0xdf831fb0 to 0xdf831ff8)
 1fa0:                                     00000000 00000000 00000000
00000000
 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c015ba68 r4:c2554540
 Code: e24cc004 ea000001 e1530006 0a000007 (e5be5004)
 ---[ end trace 0000000000000000 ]---


-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 07-07-22, 22:43, Dmitry Osipenko wrote:
> This patch breaks Tegra again, please take a look:

Damn, not again :(

>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

Why did you mention this patch ? This just removed an unused API,
Tegra should have broke because of something else, isn't it ?

>  8<--- cut here ---
>  Unable to handle kernel paging request at virtual address ffffffff
>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>  Modules linked in:
>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
> 5.19.0-rc1-00040-g30b62d123f4f #82
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  Workqueue: events_unbound deferred_probe_work_func
>  PC is at _opp_compare_key+0x40/0xc4
>  LR is at 0xfffffffb

How is LR set to such an address ?

>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>  r10: c2586078  r9 : c258606c  r8 : 00000000
>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
> ...
>  Backtrace:
>   _opp_compare_key from _set_opp+0x80/0x408

Whatever happened, happened from _opp_compare_key() and I tried to
look at it many times, couldn't figure out what's wrong there.

For the device in question, pmc I think, we don't have any "opp-hz"
property in the DT, but still the OPP core will fetch its clock and
set clk_count to 1. But this was working earlier too, we were
comparing the rate anyways. I think one of _opp_compare_rate() or
_opp_compare_bw() is broken here, but I just couldn't figure out. The
rate one should run one loop and bw one should just return. I don't
see why a crash should come out eventually.

Can you help debug this a bit ? Also see what are the values of
opp_table->path_count and opp_table->clk_count, should be 0 and 1
AFAICT.

Sorry about this Dmitry, I think we are all settled and again went
into crap.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 10:19, Viresh Kumar wrote:
> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>> This patch breaks Tegra again, please take a look:
> 
> Damn, not again :(
> 
>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
> 
> Why did you mention this patch ? This just removed an unused API,
> Tegra should have broke because of something else, isn't it ?

This patch is the cause.

>>  8<--- cut here ---
>>  Unable to handle kernel paging request at virtual address ffffffff
>>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>>  Modules linked in:
>>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
>> 5.19.0-rc1-00040-g30b62d123f4f #82
>>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>  Workqueue: events_unbound deferred_probe_work_func
>>  PC is at _opp_compare_key+0x40/0xc4
>>  LR is at 0xfffffffb
> 
> How is LR set to such an address ?
> 
>>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>>  r10: c2586078  r9 : c258606c  r8 : 00000000
>>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
>> ...
>>  Backtrace:
>>   _opp_compare_key from _set_opp+0x80/0x408
> 
> Whatever happened, happened from _opp_compare_key() and I tried to
> look at it many times, couldn't figure out what's wrong there.
> 
> For the device in question, pmc I think, we don't have any "opp-hz"
> property in the DT, but still the OPP core will fetch its clock and
> set clk_count to 1. But this was working earlier too, we were
> comparing the rate anyways. I think one of _opp_compare_rate() or
> _opp_compare_bw() is broken here, but I just couldn't figure out. The
> rate one should run one loop and bw one should just return. I don't
> see why a crash should come out eventually.
> 
> Can you help debug this a bit ? Also see what are the values of
> opp_table->path_count and opp_table->clk_count, should be 0 and 1
> AFAICT.

I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
false)", now it's "_add_opp_table(dev, true)".

Will take a closer look later on.

> Sorry about this Dmitry, I think we are all settled and again went
> into crap.

No problems :)

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 10:26, Dmitry Osipenko wrote:
> On 7/8/22 10:19, Viresh Kumar wrote:
>> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>>> This patch breaks Tegra again, please take a look:
>>
>> Damn, not again :(
>>
>>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
>>
>> Why did you mention this patch ? This just removed an unused API,
>> Tegra should have broke because of something else, isn't it ?
> 
> This patch is the cause.
> 
>>>  8<--- cut here ---
>>>  Unable to handle kernel paging request at virtual address ffffffff
>>>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>>>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>>>  Modules linked in:
>>>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
>>> 5.19.0-rc1-00040-g30b62d123f4f #82
>>>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>>  Workqueue: events_unbound deferred_probe_work_func
>>>  PC is at _opp_compare_key+0x40/0xc4
>>>  LR is at 0xfffffffb
>>
>> How is LR set to such an address ?
>>
>>>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>>>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>>>  r10: c2586078  r9 : c258606c  r8 : 00000000
>>>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>>>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>>>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
>>> ...
>>>  Backtrace:
>>>   _opp_compare_key from _set_opp+0x80/0x408
>>
>> Whatever happened, happened from _opp_compare_key() and I tried to
>> look at it many times, couldn't figure out what's wrong there.
>>
>> For the device in question, pmc I think, we don't have any "opp-hz"
>> property in the DT, but still the OPP core will fetch its clock and
>> set clk_count to 1. But this was working earlier too, we were
>> comparing the rate anyways. I think one of _opp_compare_rate() or
>> _opp_compare_bw() is broken here, but I just couldn't figure out. The
>> rate one should run one loop and bw one should just return. I don't
>> see why a crash should come out eventually.
>>
>> Can you help debug this a bit ? Also see what are the values of
>> opp_table->path_count and opp_table->clk_count, should be 0 and 1
>> AFAICT.
> 
> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
> false)", now it's "_add_opp_table(dev, true)".
> 
> Will take a closer look later on.
> 
>> Sorry about this Dmitry, I think we are all settled and again went
>> into crap.
> 
> No problems :)

BTW, maybe we should consider to start adding kselftests for OPP, like
clk framework did. That will be handy to have given that it's not easy
to test the whole OPP core without having specific devices.

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 08-07-22, 10:26, Dmitry Osipenko wrote:
> On 7/8/22 10:19, Viresh Kumar wrote:
> > On 07-07-22, 22:43, Dmitry Osipenko wrote:
> >> This patch breaks Tegra again, please take a look:
> > 
> > Damn, not again :(
> > 
> >>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
> > 
> > Why did you mention this patch ? This just removed an unused API,
> > Tegra should have broke because of something else, isn't it ?
> 
> This patch is the cause.

I was tracking the crash too closely it seems. :(

> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
> false)", now it's "_add_opp_table(dev, true)".

That's definitely a mistake, I still don't understand though how it
can lead to the crash we got.

I have fixed this in my tree now, can you check again please.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 08-07-22, 10:30, Dmitry Osipenko wrote:
> BTW, maybe we should consider to start adding kselftests for OPP, like
> clk framework did. That will be handy to have given that it's not easy
> to test the whole OPP core without having specific devices.

After being regularly bitten by such issues, I added some for cpufreq
earlier. Its time that I invest some time for OPP core too I think :)

I don't know though when I will be able to find time for that :(

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 11:12, Viresh Kumar wrote:
> On 08-07-22, 10:26, Dmitry Osipenko wrote:
>> On 7/8/22 10:19, Viresh Kumar wrote:
>>> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>>>> This patch breaks Tegra again, please take a look:
>>>
>>> Damn, not again :(
>>>
>>>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
>>>
>>> Why did you mention this patch ? This just removed an unused API,
>>> Tegra should have broke because of something else, isn't it ?
>>
>> This patch is the cause.
> 
> I was tracking the crash too closely it seems. :(
> 
>> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
>> false)", now it's "_add_opp_table(dev, true)".
> 
> That's definitely a mistake, I still don't understand though how it
> can lead to the crash we got.

I'll investigate it.

> I have fixed this in my tree now, can you check again please.
> 

Yours tree works, thank you.

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 05, 2022 at 12:30:03PM +0530, Viresh Kumar wrote:
> Hello,
> 
> This patchset adds support for devices with multiple clocks. None of the clocks
> is considered primary in this case and all are handled equally.
> 
> The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
> to set the specific OPP. Though how they find the target OPP is left for the
> users to handle. For some, we may have another unique OPP property, like level,
> which can be used to find the OPP. While in case of others, we may want to
> implement freq-based OPP finder APIs for multiple clock rates. I have decided
> not to implement them in advance, and add them only someone wants to use them.

This break OPP parsing on SC8280XP and hence cpufreq and other things:

[  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
[  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
[  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
[  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
...

I just did a rebase on next-20220708 and hit this.

I've narrowed it down to _read_rate() now returning -ENODEV since
opp_table->clk_count is zero.

Similar to what was reported for tegra for v1:

	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/

I don't have time to look at this any more today, but it would we nice
if you could unbreak linux-next.

Perhaps Bjorn or Mani can help with further details, but this doesn't
look like something that is specific to SC8280XP.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 11-07-22, 18:40, Johan Hovold wrote:
> This break OPP parsing on SC8280XP and hence cpufreq and other things:
> 
> [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> ...
> 
> I just did a rebase on next-20220708 and hit this.
> 
> I've narrowed it down to _read_rate() now returning -ENODEV since
> opp_table->clk_count is zero.
> 
> Similar to what was reported for tegra for v1:
> 
> 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> 
> I don't have time to look at this any more today, but it would we nice
> if you could unbreak linux-next.
> 
> Perhaps Bjorn or Mani can help with further details, but this doesn't
> look like something that is specific to SC8280XP.

It is actually. This is yet another corner case, Tegra had one as
well.

I have tried to understand the Qcom code / setup to best of my
abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
provide a clk to the OPP core, which breaks it after the new updates
to the OPP core. I believe following will solve it. Can someone please
try this ? I will then merge it with the right commit.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 666e1ebf91d1..4f4a285886fa 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
        }

        if (ret == -ENOENT) {
+               /*
+                * There are few platforms which don't want the OPP core to
+                * manage device's clock settings. In such cases neither the
+                * platform provides the clks explicitly to us, nor the DT
+                * contains a valid clk entry. The OPP nodes in DT may still
+                * contain "opp-hz" property though, which we need to parse and
+                * allow the platform to find an OPP based on freq later on.
+                *
+                * This is a simple solution to take care of such corner cases,
+                * i.e. make the clk_count 1, which lets us allocate space for
+                * frequency in opp->rates and also parse the entries in DT.
+                */
+               opp_table->clk_count = 1;
+
                dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
                return opp_table;
        }

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Manivannan Sadhasivam]

On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> On 11-07-22, 18:40, Johan Hovold wrote:
> > This break OPP parsing on SC8280XP and hence cpufreq and other things:
> > 
> > [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> > [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> > [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> > [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> > ...
> > 
> > I just did a rebase on next-20220708 and hit this.
> > 
> > I've narrowed it down to _read_rate() now returning -ENODEV since
> > opp_table->clk_count is zero.
> > 
> > Similar to what was reported for tegra for v1:
> > 
> > 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> > 
> > I don't have time to look at this any more today, but it would we nice
> > if you could unbreak linux-next.
> > 
> > Perhaps Bjorn or Mani can help with further details, but this doesn't
> > look like something that is specific to SC8280XP.
> 
> It is actually. This is yet another corner case, Tegra had one as
> well.
> 
> I have tried to understand the Qcom code / setup to best of my
> abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
> provide a clk to the OPP core, which breaks it after the new updates
> to the OPP core. I believe following will solve it. Can someone please
> try this ? I will then merge it with the right commit.
> 

I gave it a shot on Lenovo X13s based on SC8280 SoC and it fixes the OPP
issue. So you can add,

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 666e1ebf91d1..4f4a285886fa 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>         }
> 
>         if (ret == -ENOENT) {
> +               /*
> +                * There are few platforms which don't want the OPP core to
> +                * manage device's clock settings. In such cases neither the
> +                * platform provides the clks explicitly to us, nor the DT
> +                * contains a valid clk entry. The OPP nodes in DT may still
> +                * contain "opp-hz" property though, which we need to parse and
> +                * allow the platform to find an OPP based on freq later on.
> +                *
> +                * This is a simple solution to take care of such corner cases,
> +                * i.e. make the clk_count 1, which lets us allocate space for
> +                * frequency in opp->rates and also parse the entries in DT.
> +                */
> +               opp_table->clk_count = 1;
> +
>                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>                 return opp_table;
>         }
> 
> -- 
> viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> On 11-07-22, 18:40, Johan Hovold wrote:
> > This break OPP parsing on SC8280XP and hence cpufreq and other things:
> > 
> > [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> > [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> > [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> > [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> > ...
> > 
> > I just did a rebase on next-20220708 and hit this.
> > 
> > I've narrowed it down to _read_rate() now returning -ENODEV since
> > opp_table->clk_count is zero.
> > 
> > Similar to what was reported for tegra for v1:
> > 
> > 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> > 
> > I don't have time to look at this any more today, but it would we nice
> > if you could unbreak linux-next.
> > 
> > Perhaps Bjorn or Mani can help with further details, but this doesn't
> > look like something that is specific to SC8280XP.
> 
> It is actually. This is yet another corner case, Tegra had one as
> well.

I literally meant that it does not appear to be SC8280XP specific. Bjorn
reported seeing similar problems on multiple Qualcomm SoCs.

> I have tried to understand the Qcom code / setup to best of my
> abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
> provide a clk to the OPP core, which breaks it after the new updates
> to the OPP core. I believe following will solve it. Can someone please
> try this ? I will then merge it with the right commit.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 666e1ebf91d1..4f4a285886fa 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>         }
> 
>         if (ret == -ENOENT) {
> +               /*
> +                * There are few platforms which don't want the OPP core to
> +                * manage device's clock settings. In such cases neither the
> +                * platform provides the clks explicitly to us, nor the DT
> +                * contains a valid clk entry. The OPP nodes in DT may still
> +                * contain "opp-hz" property though, which we need to parse and
> +                * allow the platform to find an OPP based on freq later on.
> +                *
> +                * This is a simple solution to take care of such corner cases,
> +                * i.e. make the clk_count 1, which lets us allocate space for
> +                * frequency in opp->rates and also parse the entries in DT.
> +                */
> +               opp_table->clk_count = 1;
> +
>                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>                 return opp_table;
>         }

This looks like a hack. And it also triggers a bunch of new warning when
opp is trying to create debugfs entries for an entirely different table
which now gets clk_count set to 1:

[  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
[  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!

This is for the rpmhpd whose opp table does not have either opp-hz or
clocks (just opp-level).

The above unbreaks cpufreq though.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 12-07-22, 16:29, Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 666e1ebf91d1..4f4a285886fa 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> >         }
> > 
> >         if (ret == -ENOENT) {
> > +               /*
> > +                * There are few platforms which don't want the OPP core to
> > +                * manage device's clock settings. In such cases neither the
> > +                * platform provides the clks explicitly to us, nor the DT
> > +                * contains a valid clk entry. The OPP nodes in DT may still
> > +                * contain "opp-hz" property though, which we need to parse and
> > +                * allow the platform to find an OPP based on freq later on.
> > +                *
> > +                * This is a simple solution to take care of such corner cases,
> > +                * i.e. make the clk_count 1, which lets us allocate space for
> > +                * frequency in opp->rates and also parse the entries in DT.
> > +                */
> > +               opp_table->clk_count = 1;
> > +
> >                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> >                 return opp_table;
> >         }
> 
> This looks like a hack.

Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it
is done for Tegra, where you will pass the right clock name to the OPP
core, so it can verify that the clk is there and parse the table. And
then tell the OPP core not to configure the clk from
dev_pm_opp_set_opp(), which is possible now. This would have done the
things in the right way.

The problem with Qcom's DT is that the CPU node have the OPP table but
doesn't contain the clocks, which are available with the
qcom,cpufreq-hw node instead. Because of this, I couldn't pass the
real clocks name to the OPP core, "xo", for the CPU device.

I really tried to avoid adding the above code for Tegra and found a
better and cleaner way out. But I couldn't do the same here and
figured it may be more generic of a problem, which is fine as well.

The OPP core does two things currently:

1) Parse the DT and provide helpers to find the right OPP, etc.

2) Provide generic helper to configure all resources related to the
   OPP.

It is fine if some platforms only want to have the first and not the
second. To have the second though, you need to have the first as well.

The clk is required only for the second case, and the OPP core should
parse the DT anyways, irrespective of the availability of the clock.
Because of this reason, making the above change looked reasonable
(this is what was happening before my new patches came in anyway). The
clock isn't there, but there is "opp-hz" present in the DT, which
needs to be parsed.

> And it also triggers a bunch of new warning when
> opp is trying to create debugfs entries for an entirely different table
> which now gets clk_count set to 1:
> 
> [  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
> [  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> 
> This is for the rpmhpd whose opp table does not have either opp-hz or
> clocks (just opp-level).

Ahh, I missed switching back to the earlier code here. i.e. not use
the frequency for OPP directory's name, when it is 0.

This will fix it.

diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 402c507edac7..96a30a032c5f 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
         * - For some devices rate isn't available or there are multiple, use
         *   index instead for them.
         */
-       if (likely(opp_table->clk_count == 1))
+       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
                id = opp->rates[0];
        else
                id = _get_opp_count(opp_table);

I have merged this into:

commit 341df9889277 ("OPP: Allow multiple clocks for a device")

and pushed out for linux-next.


Bjorn, Mani,

It would be really good if we can find a way to make following work on
Qcom:

        clk_get(cpu_dev, NULL or "xo")

If that happens, we can handle the special case just at the consumer
driver (qcom-cpufreq-hw) and not in the core.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 12, 2022 at 08:40:45PM +0530, Viresh Kumar wrote:
> On 12-07-22, 16:29, Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index 666e1ebf91d1..4f4a285886fa 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> > >         }
> > > 
> > >         if (ret == -ENOENT) {
> > > +               /*
> > > +                * There are few platforms which don't want the OPP core to
> > > +                * manage device's clock settings. In such cases neither the
> > > +                * platform provides the clks explicitly to us, nor the DT
> > > +                * contains a valid clk entry. The OPP nodes in DT may still
> > > +                * contain "opp-hz" property though, which we need to parse and
> > > +                * allow the platform to find an OPP based on freq later on.
> > > +                *
> > > +                * This is a simple solution to take care of such corner cases,
> > > +                * i.e. make the clk_count 1, which lets us allocate space for
> > > +                * frequency in opp->rates and also parse the entries in DT.
> > > +                */
> > > +               opp_table->clk_count = 1;
> > > +
> > >                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> > >                 return opp_table;
> > >         }
> > 
> > This looks like a hack.
> 
> Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it
> is done for Tegra, where you will pass the right clock name to the OPP
> core, so it can verify that the clk is there and parse the table. And
> then tell the OPP core not to configure the clk from
> dev_pm_opp_set_opp(), which is possible now. This would have done the
> things in the right way.
> 
> The problem with Qcom's DT is that the CPU node have the OPP table but
> doesn't contain the clocks, which are available with the
> qcom,cpufreq-hw node instead. Because of this, I couldn't pass the
> real clocks name to the OPP core, "xo", for the CPU device.
> 
> I really tried to avoid adding the above code for Tegra and found a
> better and cleaner way out. But I couldn't do the same here and
> figured it may be more generic of a problem, which is fine as well.
> 
> The OPP core does two things currently:
> 
> 1) Parse the DT and provide helpers to find the right OPP, etc.
> 
> 2) Provide generic helper to configure all resources related to the
>    OPP.
> 
> It is fine if some platforms only want to have the first and not the
> second. To have the second though, you need to have the first as well.
> 
> The clk is required only for the second case, and the OPP core should
> parse the DT anyways, irrespective of the availability of the clock.
> Because of this reason, making the above change looked reasonable
> (this is what was happening before my new patches came in anyway). The
> clock isn't there, but there is "opp-hz" present in the DT, which
> needs to be parsed.

Ok, thanks for the details. I'd still look into if there's some way to
avoid setting clk_count when there are no clocks as it sounds like an
anti-pattern that will just make the code harder to understand and
maintain.

> > And it also triggers a bunch of new warning when
> > opp is trying to create debugfs entries for an entirely different table
> > which now gets clk_count set to 1:
> > 
> > [  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
> > [  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!

> > This is for the rpmhpd whose opp table does not have either opp-hz or
> > clocks (just opp-level).
> 
> Ahh, I missed switching back to the earlier code here. i.e. not use
> the frequency for OPP directory's name, when it is 0.
> 
> This will fix it.
> 
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 402c507edac7..96a30a032c5f 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
>          * - For some devices rate isn't available or there are multiple, use
>          *   index instead for them.
>          */
> -       if (likely(opp_table->clk_count == 1))
> +       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
>                 id = opp->rates[0];
>         else
>                 id = _get_opp_count(opp_table);

Indeed it does, thanks.

> I have merged this into:
> 
> commit 341df9889277 ("OPP: Allow multiple clocks for a device")
> 
> and pushed out for linux-next.

Thanks for addressing this quickly. With the two patches above applied,
the issues I noticed are gone.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 12-07-22, 17:55, Johan Hovold wrote:
> Ok, thanks for the details. I'd still look into if there's some way to
> avoid setting clk_count when there are no clocks as it sounds like an
> anti-pattern that will just make the code harder to understand and
> maintain.

Here is an attempt from me :)

https://lore.kernel.org/lkml/cover.1657695140.git.viresh.kumar@linaro.org/

-- 
viresh