Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

[removed DT people from CC, since this is all about clocks now]

Hello Ilya,

On Fri, Mar 08, 2024 at 10:23:31AM +0300, Ilya K wrote:
> This change seems to make my Orange Pi 5 (RK3588S) lock up on boot
> :( Any suggestions on how to debug this? It doesn't really log
> much...

I suppose with this change you explicitly mean the last patch, which
has not yet been applied? That patch effectively allows some clocks
to be disabled, that have previously been marked as critical (and
thus always-on).

If that results in a boot lockup, I expect you have a peripheral
driver, that does not enable a required clock (e.g. because of a
missing clock reference).

I assume you have a bunch of other patches applied on top of
upstream and not just this one? In that case it might be sensible to
first apply this patch, do a test boot (it hopefully boots). Then
step-by-step re-add the other patches and check which one breaks
when being combined with this patch.

P.S.: I plan to send a v9 of this patch once 6.9-rc1 has been tagged
and will split it, so that the removal of CLK_IS_CRITICAL is done in
a separate patch.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

Hi,

On Fri, Mar 08, 2024 at 04:30:26PM +0300, Ilya K wrote:
>>> This change seems to make my Orange Pi 5 (RK3588S) lock up on boot
>>> :( Any suggestions on how to debug this? It doesn't really log
>>> much...
>>>
>> I suppose with this change you explicitly mean the last patch, which
>> has not yet been applied? That patch effectively allows some clocks
>> to be disabled, that have previously been marked as critical (and
>> thus always-on).
>> 
>> If that results in a boot lockup, I expect you have a peripheral
>> driver, that does not enable a required clock (e.g. because of a
>> missing clock reference).
>> 
>> I assume you have a bunch of other patches applied on top of
>> upstream and not just this one? In that case it might be sensible to
>> first apply this patch, do a test boot (it hopefully boots). Then
>> step-by-step re-add the other patches and check which one breaks
>> when being combined with this patch.
>>
> Thanks! I think the email bounced from most of the mailing lists anyway... The
> exact tree I'm running is here: https://github.com/K900/linux/commits/
> rk3588-test/ and the last revert commit is what makes things boot again.

Please update your mailer to send plain text mails instead of HTML
when working with the kernel mailing lists. Also please do not
top-post.

The tree seems to be more or less what is working on Rock 5B and
EVB1. Please try a partial revert, which just undos the removal of
RK3588_LINKED_CLK (the define and the usage of the define). If that
works the next step would be to step-by-step remove usage of
RK3588_LINKED_CLK again until finding the clock that needs to be
critical.

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Chad LeClair]

On 3/8/24 08:27, Sebastian Reichel wrote:
> [removed DT people from CC, since this is all about clocks now]
> 
> Hello Ilya,
> 
> On Fri, Mar 08, 2024 at 10:23:31AM +0300, Ilya K wrote:
>> This change seems to make my Orange Pi 5 (RK3588S) lock up on boot
>> :( Any suggestions on how to debug this? It doesn't really log
>> much...
> 
> I suppose with this change you explicitly mean the last patch, which
> has not yet been applied? That patch effectively allows some clocks
> to be disabled, that have previously been marked as critical (and
> thus always-on).
> 
> If that results in a boot lockup, I expect you have a peripheral
> driver, that does not enable a required clock (e.g. because of a
> missing clock reference).

Sebastian,

Another Orange Pi 5 owner here...  I likewise ran into trouble with
this patch.  I have been playing with your rk3588 branch from the
Collabora gitlab and was also experiencing hangs on boot on my OPi5.  I
bisected your branch and found that the GATE_LINK support commit was the
problem for me.

Digging in, I found that the problem behind the hang was that I was not
getting the aclk_nvm_root which in turn caused DMA transactions to the
SFC driver to hang.

...
[   2.804519] rockchip-sfc fe2b0000.spi: DMA wait for transfer finish
timeout
[   2.805127] rockchip-sfc fe2b0000.spi: xfer data failed ret -110 dir 1
...

Setting aclk_nvm_root to critical allows the system to boot.  However
not all is well as I do get errors like the following which also seem to
indicate further clock problems:
[   6.296554] rockchip-pm-domain
fd8d8000.power-management:power-controller: failed to set idle on domain
'vo0', val=0


I assume that the con-ops of your patch is analogous to the downstream
"clock-link" driver.  (ie. you are looking for PM events to cause the
pm_clk_suspend()/pm_clk_resume() routines to enable/disable the linked
(second parent) clocks).  This wasn't happening for me.  Unlike in the
downstream implementation, the gate_link devices' dev->driver pointer
was null. So that when rpm_resume() was being called, it would make its
way to pm_generic_runtime_resume() which then would do nothing.  The end
result is that I would have a prepare count of 1 on aclk_nvm_root, but
an enable count of 0 so it would get disabled (orig patch w/o marking
this critical).

I did a quick hacky proof of concept where I set up the pm ops on the
clk-rk3588 driver to point to the pm_clk_suspend/pm_clk_resume routines
like downstream did.  I used device_bind_driver() to forceably bind the
clk-rk3588 driver to the gate link devices it was setting up.

This worked for me.  With those changes I then was able to boot without
needing to set aclk_nvm_root as critical.  Further, it cleaned up the
power-controller error messages on the sdio and vo0 domains.

The force bind I did doesn't feel like a particularly clean solution to
me.  However, I am not knowledgeable enough in the (platform) device
model to know what the right way to do this is without fully doing what
downstream did and having DT nodes for the various gate link devices.
But it seems to prove out that having the driver bound to the devices
with the relevant runtime pm ops is the missing piece in my use case on
the OPi5.

Hope this additional data point helps!

--
Chad LeClair

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4516 bytes --]

Hello Chad,

On Sun, Mar 17, 2024 at 07:34:41PM -0400, Chad LeClair wrote:
> On 3/8/24 08:27, Sebastian Reichel wrote:
> > [removed DT people from CC, since this is all about clocks now]
> > 
> > Hello Ilya,
> > 
> > On Fri, Mar 08, 2024 at 10:23:31AM +0300, Ilya K wrote:
> >> This change seems to make my Orange Pi 5 (RK3588S) lock up on boot
> >> :( Any suggestions on how to debug this? It doesn't really log
> >> much...
> > 
> > I suppose with this change you explicitly mean the last patch, which
> > has not yet been applied? That patch effectively allows some clocks
> > to be disabled, that have previously been marked as critical (and
> > thus always-on).
> > 
> > If that results in a boot lockup, I expect you have a peripheral
> > driver, that does not enable a required clock (e.g. because of a
> > missing clock reference).
> 
> Another Orange Pi 5 owner here...  I likewise ran into trouble with
> this patch.  I have been playing with your rk3588 branch from the
> Collabora gitlab and was also experiencing hangs on boot on my OPi5.  I
> bisected your branch and found that the GATE_LINK support commit was the
> problem for me.

Ok.

> Digging in, I found that the problem behind the hang was that I was not
> getting the aclk_nvm_root

That matches what Ilya found (there were some more off-list messages
because of HTML mail issues). I suspected, that HCLK_SD (which is
sourced from SCMI) might need HCLK_NVM.

> which in turn caused DMA transactions to the SFC driver to hang.
> 
> ...
> [   2.804519] rockchip-sfc fe2b0000.spi: DMA wait for transfer finish
> timeout
> [   2.805127] rockchip-sfc fe2b0000.spi: xfer data failed ret -110 dir 1
> ...
> 
> Setting aclk_nvm_root to critical allows the system to boot.  However
> not all is well as I do get errors like the following which also seem to
> indicate further clock problems:
> [   6.296554] rockchip-pm-domain
> fd8d8000.power-management:power-controller: failed to set idle on domain
> 'vo0', val=0

I see the same message for AV1 and I've also seen failed idle
message with the rockchip kernel. I have not yet debugged what is
going on with that.

> I assume that the con-ops of your patch is analogous to the downstream
> "clock-link" driver.  (ie. you are looking for PM events to cause the
> pm_clk_suspend()/pm_clk_resume() routines to enable/disable the linked
> (second parent) clocks).  This wasn't happening for me.  Unlike in the
> downstream implementation, the gate_link devices' dev->driver pointer
> was null. So that when rpm_resume() was being called, it would make its
> way to pm_generic_runtime_resume() which then would do nothing.  The end
> result is that I would have a prepare count of 1 on aclk_nvm_root, but
> an enable count of 0 so it would get disabled (orig patch w/o marking
> this critical).
> 
> I did a quick hacky proof of concept where I set up the pm ops on the
> clk-rk3588 driver to point to the pm_clk_suspend/pm_clk_resume routines
> like downstream did.  I used device_bind_driver() to forceably bind the
> clk-rk3588 driver to the gate link devices it was setting up.
> 
> This worked for me.  With those changes I then was able to boot without
> needing to set aclk_nvm_root as critical.  Further, it cleaned up the
> power-controller error messages on the sdio and vo0 domains.
> 
> The force bind I did doesn't feel like a particularly clean solution to
> me.  However, I am not knowledgeable enough in the (platform) device
> model to know what the right way to do this is without fully doing what
> downstream did and having DT nodes for the various gate link devices.
> But it seems to prove out that having the driver bound to the devices
> with the relevant runtime pm ops is the missing piece in my use case on
> the OPi5.

Can you test again using rk3588-test [0] (which is based on today's
master branch) from today as base? Ideally it should work with your
Orange Pi 5, since it contains "clk: rockchip: rk3588: mark hclk_nvm
critical".

I also worked on a cleaner solution for the issue you described and
integrated it in the patch adding proper gate clock support. So
please also test with HCLK_NVM not being marked as CRITICAL.

[0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-test

If everything is fine I will prepare a v9.

> Hope this additional data point helps!

Thanks for the detailed analysis.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Ilya K]

On 2024-03-20 19:36, Sebastian Reichel wrote:
> Can you test again using rk3588-test [0] (which is based on today's
> master branch) from today as base? Ideally it should work with your
> Orange Pi 5, since it contains "clk: rockchip: rk3588: mark hclk_nvm
> critical".
> 
> I also worked on a cleaner solution for the issue you described and
> integrated it in the patch adding proper gate clock support. So
> please also test with HCLK_NVM not being marked as CRITICAL.
> 
> [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-test
> 
> If everything is fine I will prepare a v9.
>

Tested this on my end, and I'm getting the same SFC issue, so there's probably another clock that's missing somewhere. Building with ROCKCHIP_SFC=y (instead of =m) removes the error message, but any access to the flash just locks up, so it's probably placebo, unless there's a second, unrelated issue.

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Chad LeClair]

Sebastian,

On 3/20/24 12:36, Sebastian Reichel wrote:

> I also worked on a cleaner solution for the issue you described and
> integrated it in the patch adding proper gate clock support. So
> please also test with HCLK_NVM not being marked as CRITICAL.
> 
> [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-test
> 
> If everything is fine I will prepare a v9.
> 
>> Hope this additional data point helps!
> 
> Thanks for the detailed analysis.
> 
> Greetings,
> 
> -- Sebastian


No luck unfortunately.  I still see the SFC dma timeouts.  It looks like
aclk_nvm_root is still getting disabled.  It now has both an enable count of 0
and a prepare count of 0.

Unlike your previous version, I _do_ see the driver bound to the device and
the rpm_resume() call finds its way to pm_clk_resume().  So it looks like
you resolved the original issue I was seeing.

However, when I reach __pm_clk_enable() it looks like the clock entry (ce)
is not in a good state:
  (gdb) print *ce
  $3 = {node = {next = 0xffff0001f0ce1930, prev = 0xffff0001f0ce1930}, con_id =
  0xffff0001f0a214f0 "aclk_nvm_root", clk = 0xfffffffffffffffe, status =
  PCE_STATUS_ERROR, enabled_when_prepared = false}

The immediate problem at hand is that ce->status is PCE_STATUS_ERROR so the 
switch statement will take the default case and return without doing anything. 
Also the ce->clk pointer looks like some sort of error pointer value so I'm
wondering if something went wrong in the setup you were doing in 
clk_gate_link_probe().

Note: I ran to that same __pm_clk_enable() breakpoint for a number of of 
GATE_LINK clocks.  They all looked to be in that same bad state.  I put the 
"aclk_nvm_root" one in the message here since that is the one that is most 
relevant to the discussion, but they all look to be broken in the same way.

Hopefully this gives you a hint as to what is going on.

BTW, In case it is of interest to you or Ilya, I have my "working" tree
available here: 
  https://github.com/sesca128/linux-rk3588/tree/v6.8-rk3588-test

My quick and dirty hack is at the tip of the branch. The rest is pretty 
much your rk3588-test branch from a few days ago. 

--
Chad LeClair

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

Hi Chad and Ilya,

On Wed, Mar 20, 2024 at 10:50:48PM -0400, Chad LeClair wrote:
> Sebastian,
> 
> On 3/20/24 12:36, Sebastian Reichel wrote:
> 
> > I also worked on a cleaner solution for the issue you described and
> > integrated it in the patch adding proper gate clock support. So
> > please also test with HCLK_NVM not being marked as CRITICAL.
> > 
> > [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-test
> > 
> > If everything is fine I will prepare a v9.
> > 
> >> Hope this additional data point helps!
> > 
> > Thanks for the detailed analysis.
> > 
> > Greetings,
> > 
> > -- Sebastian
> 
> 
> No luck unfortunately.  I still see the SFC dma timeouts.  It looks like
> aclk_nvm_root is still getting disabled.  It now has both an enable count of 0
> and a prepare count of 0.
>
> Unlike your previous version, I _do_ see the driver bound to the device and
> the rpm_resume() call finds its way to pm_clk_resume().  So it looks like
> you resolved the original issue I was seeing.
> 
> However, when I reach __pm_clk_enable() it looks like the clock entry (ce)
> is not in a good state:
>   (gdb) print *ce
>   $3 = {node = {next = 0xffff0001f0ce1930, prev = 0xffff0001f0ce1930}, con_id =
>   0xffff0001f0a214f0 "aclk_nvm_root", clk = 0xfffffffffffffffe, status =
>   PCE_STATUS_ERROR, enabled_when_prepared = false}
> 
> The immediate problem at hand is that ce->status is PCE_STATUS_ERROR so the 
> switch statement will take the default case and return without doing anything. 
> Also the ce->clk pointer looks like some sort of error pointer value so I'm
> wondering if something went wrong in the setup you were doing in 
> clk_gate_link_probe().
> 
> Note: I ran to that same __pm_clk_enable() breakpoint for a number of of 
> GATE_LINK clocks.  They all looked to be in that same bad state.  I put the 
> "aclk_nvm_root" one in the message here since that is the one that is most 
> relevant to the discussion, but they all look to be broken in the same way.
> 
> Hopefully this gives you a hint as to what is going on.

Ah, that was actually not setting up the clock links at all. Sorry
about that. I reworked everything again and moved all the GATE_LINK
code into the separate driver now. Please give it another try.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Ilya K]

On 2024-03-21 21:31, Sebastian Reichel wrote:
> 
> Ah, that was actually not setting up the clock links at all. Sorry
> about that. I reworked everything again and moved all the GATE_LINK
> code into the separate driver now. Please give it another try.
> 
> Greetings,
> 
> -- Sebastian

Applied this to my 6.8.1: https://github.com/K900/linux/tree/rk3588-test

As far as I can tell, literally everything works now - it boots, runs, and I can read and write the flash even with ROCKCHIP_SFC=m.

Thanks a lot for digging into this, y'all!

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

Hello Ilya,

On Thu, Mar 21, 2024 at 10:01:10PM +0300, Ilya K wrote:
> On 2024-03-21 21:31, Sebastian Reichel wrote:
> > 
> > Ah, that was actually not setting up the clock links at all. Sorry
> > about that. I reworked everything again and moved all the GATE_LINK
> > code into the separate driver now. Please give it another try.
> > 
> > Greetings,
> > 
> > -- Sebastian
> 
> Applied this to my 6.8.1: https://github.com/K900/linux/tree/rk3588-test
> 
> As far as I can tell, literally everything works now - it boots, runs, and I can read and write the flash even with ROCKCHIP_SFC=m.
> 
> Thanks a lot for digging into this, y'all!

Great, thanks for testing. Can you check if it still works with
ROCKCHIP_SFC=m when applying the following additional change on
top of your tree?

diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
index fea7e7fcc4a4..f0eb380b727c 100644
--- a/drivers/clk/rockchip/clk-rk3588.c
+++ b/drivers/clk/rockchip/clk-rk3588.c
@@ -2413,7 +2413,7 @@ static struct rockchip_clk_branch rk3588_early_clk_branches[] __initdata = {
 static struct rockchip_clk_branch rk3588_clk_branches[] = {
        GATE_LINK(ACLK_ISP1_PRE, "aclk_isp1_pre", "aclk_isp1_root", ACLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 6, GFLAGS),
        GATE_LINK(HCLK_ISP1_PRE, "hclk_isp1_pre", "hclk_isp1_root", HCLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 8, GFLAGS),
-       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, CLK_IS_CRITICAL, RK3588_CLKGATE_CON(31), 2, GFLAGS),
+       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, 0, RK3588_CLKGATE_CON(31), 2, GFLAGS),
        GATE_LINK(ACLK_USB, "aclk_usb", "aclk_usb_root", ACLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 2, GFLAGS),
        GATE_LINK(HCLK_USB, "hclk_usb", "hclk_usb_root", HCLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 3, GFLAGS),
        GATE_LINK(ACLK_JPEG_DECODER_PRE, "aclk_jpeg_decoder_pre", "aclk_jpeg_decoder_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(44), 7, GFLAGS),

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Chad LeClair]

Sebastian,

On 3/21/24 16:45, Sebastian Reichel wrote:
> Hello Ilya,
> 
> On Thu, Mar 21, 2024 at 10:01:10PM +0300, Ilya K wrote:
>> On 2024-03-21 21:31, Sebastian Reichel wrote:
>>>
>>> Ah, that was actually not setting up the clock links at all. Sorry
>>> about that. I reworked everything again and moved all the GATE_LINK
>>> code into the separate driver now. Please give it another try.
>>>
>>> Greetings,
>>>
>>> -- Sebastian
>>
>> Applied this to my 6.8.1: https://github.com/K900/linux/tree/rk3588-test
>>
>> As far as I can tell, literally everything works now - it boots, runs, and I can read and write the flash even with ROCKCHIP_SFC=m.
>>
>> Thanks a lot for digging into this, y'all!
> 
> Great, thanks for testing. Can you check if it still works with
> ROCKCHIP_SFC=m when applying the following additional change on
> top of your tree?
> 
> diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
> index fea7e7fcc4a4..f0eb380b727c 100644
> --- a/drivers/clk/rockchip/clk-rk3588.c
> +++ b/drivers/clk/rockchip/clk-rk3588.c
> @@ -2413,7 +2413,7 @@ static struct rockchip_clk_branch rk3588_early_clk_branches[] __initdata = {
>  static struct rockchip_clk_branch rk3588_clk_branches[] = {
>         GATE_LINK(ACLK_ISP1_PRE, "aclk_isp1_pre", "aclk_isp1_root", ACLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 6, GFLAGS),
>         GATE_LINK(HCLK_ISP1_PRE, "hclk_isp1_pre", "hclk_isp1_root", HCLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 8, GFLAGS),
> -       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, CLK_IS_CRITICAL, RK3588_CLKGATE_CON(31), 2, GFLAGS),
> +       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, 0, RK3588_CLKGATE_CON(31), 2, GFLAGS),
>         GATE_LINK(ACLK_USB, "aclk_usb", "aclk_usb_root", ACLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 2, GFLAGS),
>         GATE_LINK(HCLK_USB, "hclk_usb", "hclk_usb_root", HCLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 3, GFLAGS),
>         GATE_LINK(ACLK_JPEG_DECODER_PRE, "aclk_jpeg_decoder_pre", "aclk_jpeg_decoder_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(44), 7, GFLAGS),
> 

I can also report success with your latest tree.  It boots successfully and I
am able to interact with the SFC flash.  Looking at the clk debug info I now
see an enable value of 1 on aclk_nvm_root.

It also cleaned up the power-controller error message I had been seeing on vo0.
I just get the 'PM: genpd: Disabling unused power domains' message and no
further power errors/warnings.  So looks good.

I also tried the patch in your most recent message (removing CLK_IS_CRITICAL
from hclk_nvm).  My kernel is compiled ROCKCHIP_SFC=m. Everything still works
as expected.

So overall, looks great!  Thanks for all of your work on this!

--
Chad LeClair

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Ilya K]

On 2024-03-21 23:45, Sebastian Reichel wrote:
> Hello Ilya,
> 
> On Thu, Mar 21, 2024 at 10:01:10PM +0300, Ilya K wrote:
>> On 2024-03-21 21:31, Sebastian Reichel wrote:
>>>
>>> Ah, that was actually not setting up the clock links at all. Sorry
>>> about that. I reworked everything again and moved all the GATE_LINK
>>> code into the separate driver now. Please give it another try.
>>>
>>> Greetings,
>>>
>>> -- Sebastian
>>
>> Applied this to my 6.8.1: https://github.com/K900/linux/tree/rk3588-test
>>
>> As far as I can tell, literally everything works now - it boots, runs, and I can read and write the flash even with ROCKCHIP_SFC=m.
>>
>> Thanks a lot for digging into this, y'all!
> 
> Great, thanks for testing. Can you check if it still works with
> ROCKCHIP_SFC=m when applying the following additional change on
> top of your tree?
> 
> diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
> index fea7e7fcc4a4..f0eb380b727c 100644
> --- a/drivers/clk/rockchip/clk-rk3588.c
> +++ b/drivers/clk/rockchip/clk-rk3588.c
> @@ -2413,7 +2413,7 @@ static struct rockchip_clk_branch rk3588_early_clk_branches[] __initdata = {
>  static struct rockchip_clk_branch rk3588_clk_branches[] = {
>         GATE_LINK(ACLK_ISP1_PRE, "aclk_isp1_pre", "aclk_isp1_root", ACLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 6, GFLAGS),
>         GATE_LINK(HCLK_ISP1_PRE, "hclk_isp1_pre", "hclk_isp1_root", HCLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 8, GFLAGS),
> -       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, CLK_IS_CRITICAL, RK3588_CLKGATE_CON(31), 2, GFLAGS),
> +       GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, 0, RK3588_CLKGATE_CON(31), 2, GFLAGS),
>         GATE_LINK(ACLK_USB, "aclk_usb", "aclk_usb_root", ACLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 2, GFLAGS),
>         GATE_LINK(HCLK_USB, "hclk_usb", "hclk_usb_root", HCLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 3, GFLAGS),
>         GATE_LINK(ACLK_JPEG_DECODER_PRE, "aclk_jpeg_decoder_pre", "aclk_jpeg_decoder_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(44), 7, GFLAGS),
> 
> Greetings,
> 
> -- Sebastian

Can confirm everything works with this change too: https://github.com/K900/linux/commit/d11ccce948117aeef39d3a6dbedd17e04555a9cc

[PATCH v8 0/7] rockchip: clk: improve GATE_LINK support

[Sebastian Reichel]

Hi,

This is a follow-up for Elaine's series. These patches are written from
scratch, though. There are two parts:

part 1:
Elaine's series used to contain patches for the VO1GRF handling, but they were
dropped at some point because of the CLK_NR_CLKS feedback from the DT
maintainers. I added some code, that should hopefully fix everyones concerns by
figuring out the right number at runtime. I also moved the correct handling of
pclk_vo0grf/pclk_vo1grf before proper handling of GATE_LINK clocks, so that it
can be merged ASAP. These patches are needed for HDMI RX/TX support on RK3588.

part 2:
For proper GATE_LINK support I tried implementing the suggestion from Stephen
Boyd to use clk PM operations by creating MFD dynamically. This required some
restructuring, since CLK_OF_DECLARE() is called before devices are available.
All of this can be found in the last patch of this series.

Changes since PATCHv7:
 * https://lore.kernel.org/all/20231213185114.47565-1-sebastian.reichel@collabora.com/
 * rebased to v6.8-rc1
 * Collected Reviewed-by/Acked-by from Krzysztof Kozlowski for DT binding patches
 * support nr_clk=0 in rockchip_clk_find_max_clk_id() for smatch

Greetings,

-- Sebstian

Sebastian Reichel (7):
  clk: rockchip: rk3588: fix CLK_NR_CLKS usage
  dt-bindings: clock: rk3588: drop CLK_NR_CLKS
  dt-bindings: clock: rk3588: add missing PCLK_VO1GRF
  clk: rockchip: rk3588: fix pclk_vo0grf and pclk_vo1grf
  clk: rockchip: rk3588: fix indent
  clk: rockchip: rk3588: use linked clock ID for GATE_LINK
  clk: rockchip: implement proper GATE_LINK support

 drivers/clk/rockchip/clk-rk3588.c             | 163 ++++++++----------
 drivers/clk/rockchip/clk.c                    |  86 ++++++++-
 drivers/clk/rockchip/clk.h                    |  18 ++
 .../dt-bindings/clock/rockchip,rk3588-cru.h   |   3 +-
 4 files changed, 179 insertions(+), 91 deletions(-)

-- 
2.43.0

[PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

Recent Rockchip SoCs have a new hardware block called Native Interface
Unit (NIU), which gates clocks to devices behind them. These effectively
need two parent clocks.

GATE_LINK type clocks handle the second parent via 'linkedclk' by using
runtime PM clocks. To make that possible a new platform device is created
for every clock handled in this way.

Note, that before this patch clk_rk3588_probe() has never been called,
because CLK_OF_DECLARE marks the DT node as processed. This patch replaces
that with CLK_OF_DECLARE_DRIVER and thus the probe function is used now.
This is necessary to have 'struct device' available.

Also instead of builtin_platform_driver_probe, the driver has been
switched to use core_initcall, since it should be fully probed before
the Rockchip PM domain driver (and that is using postcore_initcall).

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/clk/rockchip/clk-rk3588.c | 122 +++++++++++++-----------------
 drivers/clk/rockchip/clk.c        |  69 ++++++++++++++++-
 drivers/clk/rockchip/clk.h        |  16 ++++
 3 files changed, 138 insertions(+), 69 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3588.c b/drivers/clk/rockchip/clk-rk3588.c
index b30279a96dc8..f0eb380b727c 100644
--- a/drivers/clk/rockchip/clk-rk3588.c
+++ b/drivers/clk/rockchip/clk-rk3588.c
@@ -12,28 +12,6 @@
 #include <dt-bindings/clock/rockchip,rk3588-cru.h>
 #include "clk.h"
 
-/*
- * Recent Rockchip SoCs have a new hardware block called Native Interface
- * Unit (NIU), which gates clocks to devices behind them. These effectively
- * need two parent clocks.
- *
- * Downstream enables the linked clock via runtime PM whenever the gate is
- * enabled. This implementation uses separate clock nodes for each of the
- * linked gate clocks, which leaks parts of the clock tree into DT.
- *
- * The GATE_LINK macro instead takes the second parent via 'linkname', but
- * ignores the information. Once the clock framework is ready to handle it, the
- * information should be passed on here. But since these clocks are required to
- * access multiple relevant IP blocks, such as PCIe or USB, we mark all linked
- * clocks critical until a better solution is available. This will waste some
- * power, but avoids leaking implementation details into DT or hanging the
- * system.
- */
-#define GATE_LINK(_id, cname, pname, linkedclk, f, o, b, gf) \
-	GATE(_id, cname, pname, f, o, b, gf)
-#define RK3588_LINKED_CLK		CLK_IS_CRITICAL
-
-
 #define RK3588_GRF_SOC_STATUS0		0x600
 #define RK3588_PHYREF_ALT_GATE		0xc38
 
@@ -266,6 +244,8 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = {
 	},							\
 }
 
+static struct rockchip_clk_provider *early_ctx;
+
 static struct rockchip_cpuclk_rate_table rk3588_cpub0clk_rates[] __initdata = {
 	RK3588_CPUB01CLK_RATE(2496000000, 1),
 	RK3588_CPUB01CLK_RATE(2400000000, 1),
@@ -694,7 +674,7 @@ static struct rockchip_pll_clock rk3588_pll_clks[] __initdata = {
 		     RK3588_MODE_CON0, 10, 15, 0, rk3588_pll_rates),
 };
 
-static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
+static struct rockchip_clk_branch rk3588_early_clk_branches[] __initdata = {
 	/*
 	 * CRU Clock-Architecture
 	 */
@@ -1456,7 +1436,7 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 	COMPOSITE_NODIV(HCLK_NVM_ROOT,  "hclk_nvm_root", mux_200m_100m_50m_24m_p, 0,
 			RK3588_CLKSEL_CON(77), 0, 2, MFLAGS,
 			RK3588_CLKGATE_CON(31), 0, GFLAGS),
-	COMPOSITE(ACLK_NVM_ROOT, "aclk_nvm_root", gpll_cpll_p, RK3588_LINKED_CLK,
+	COMPOSITE(ACLK_NVM_ROOT, "aclk_nvm_root", gpll_cpll_p, 0,
 			RK3588_CLKSEL_CON(77), 7, 1, MFLAGS, 2, 5, DFLAGS,
 			RK3588_CLKGATE_CON(31), 1, GFLAGS),
 	GATE(ACLK_EMMC, "aclk_emmc", "aclk_nvm_root", 0,
@@ -1685,13 +1665,13 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 			RK3588_CLKGATE_CON(42), 9, GFLAGS),
 
 	/* vdpu */
-	COMPOSITE(ACLK_VDPU_ROOT, "aclk_vdpu_root", gpll_cpll_aupll_p, RK3588_LINKED_CLK,
+	COMPOSITE(ACLK_VDPU_ROOT, "aclk_vdpu_root", gpll_cpll_aupll_p, 0,
 			RK3588_CLKSEL_CON(98), 5, 2, MFLAGS, 0, 5, DFLAGS,
 			RK3588_CLKGATE_CON(44), 0, GFLAGS),
 	COMPOSITE_NODIV(ACLK_VDPU_LOW_ROOT, "aclk_vdpu_low_root", mux_400m_200m_100m_24m_p, 0,
 			RK3588_CLKSEL_CON(98), 7, 2, MFLAGS,
 			RK3588_CLKGATE_CON(44), 1, GFLAGS),
-	COMPOSITE_NODIV(HCLK_VDPU_ROOT, "hclk_vdpu_root", mux_200m_100m_50m_24m_p, RK3588_LINKED_CLK,
+	COMPOSITE_NODIV(HCLK_VDPU_ROOT, "hclk_vdpu_root", mux_200m_100m_50m_24m_p, 0,
 			RK3588_CLKSEL_CON(98), 9, 2, MFLAGS,
 			RK3588_CLKGATE_CON(44), 2, GFLAGS),
 	COMPOSITE(ACLK_JPEG_DECODER_ROOT, "aclk_jpeg_decoder_root", gpll_cpll_aupll_spll_p, 0,
@@ -1742,9 +1722,9 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 	COMPOSITE(ACLK_RKVENC0_ROOT, "aclk_rkvenc0_root", gpll_cpll_npll_p, 0,
 			RK3588_CLKSEL_CON(102), 7, 2, MFLAGS, 2, 5, DFLAGS,
 			RK3588_CLKGATE_CON(47), 1, GFLAGS),
-	GATE(HCLK_RKVENC0, "hclk_rkvenc0", "hclk_rkvenc0_root", RK3588_LINKED_CLK,
+	GATE(HCLK_RKVENC0, "hclk_rkvenc0", "hclk_rkvenc0_root", 0,
 			RK3588_CLKGATE_CON(47), 4, GFLAGS),
-	GATE(ACLK_RKVENC0, "aclk_rkvenc0", "aclk_rkvenc0_root", RK3588_LINKED_CLK,
+	GATE(ACLK_RKVENC0, "aclk_rkvenc0", "aclk_rkvenc0_root", 0,
 			RK3588_CLKGATE_CON(47), 5, GFLAGS),
 	COMPOSITE(CLK_RKVENC0_CORE, "clk_rkvenc0_core", gpll_cpll_aupll_npll_p, 0,
 			RK3588_CLKSEL_CON(102), 14, 2, MFLAGS, 9, 5, DFLAGS,
@@ -1754,10 +1734,10 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 			RK3588_CLKGATE_CON(48), 6, GFLAGS),
 
 	/* vi */
-	COMPOSITE(ACLK_VI_ROOT, "aclk_vi_root", gpll_cpll_npll_aupll_spll_p, RK3588_LINKED_CLK,
+	COMPOSITE(ACLK_VI_ROOT, "aclk_vi_root", gpll_cpll_npll_aupll_spll_p, 0,
 			RK3588_CLKSEL_CON(106), 5, 3, MFLAGS, 0, 5, DFLAGS,
 			RK3588_CLKGATE_CON(49), 0, GFLAGS),
-	COMPOSITE_NODIV(HCLK_VI_ROOT, "hclk_vi_root", mux_200m_100m_50m_24m_p, RK3588_LINKED_CLK,
+	COMPOSITE_NODIV(HCLK_VI_ROOT, "hclk_vi_root", mux_200m_100m_50m_24m_p, 0,
 			RK3588_CLKSEL_CON(106), 8, 2, MFLAGS,
 			RK3588_CLKGATE_CON(49), 1, GFLAGS),
 	COMPOSITE_NODIV(PCLK_VI_ROOT, "pclk_vi_root", mux_100m_50m_24m_p, 0,
@@ -1927,10 +1907,10 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 	COMPOSITE(ACLK_VOP_ROOT, "aclk_vop_root", gpll_cpll_dmyaupll_npll_spll_p, 0,
 			RK3588_CLKSEL_CON(110), 5, 3, MFLAGS, 0, 5, DFLAGS,
 			RK3588_CLKGATE_CON(52), 0, GFLAGS),
-	COMPOSITE_NODIV(ACLK_VOP_LOW_ROOT, "aclk_vop_low_root", mux_400m_200m_100m_24m_p, RK3588_LINKED_CLK,
+	COMPOSITE_NODIV(ACLK_VOP_LOW_ROOT, "aclk_vop_low_root", mux_400m_200m_100m_24m_p, 0,
 			RK3588_CLKSEL_CON(110), 8, 2, MFLAGS,
 			RK3588_CLKGATE_CON(52), 1, GFLAGS),
-	COMPOSITE_NODIV(HCLK_VOP_ROOT, "hclk_vop_root", mux_200m_100m_50m_24m_p, RK3588_LINKED_CLK,
+	COMPOSITE_NODIV(HCLK_VOP_ROOT, "hclk_vop_root", mux_200m_100m_50m_24m_p, 0,
 			RK3588_CLKSEL_CON(110), 10, 2, MFLAGS,
 			RK3588_CLKGATE_CON(52), 2, GFLAGS),
 	COMPOSITE_NODIV(PCLK_VOP_ROOT, "pclk_vop_root", mux_100m_50m_24m_p, 0,
@@ -2428,10 +2408,12 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 			RK3588_CLKGATE_CON(68), 5, GFLAGS),
 	GATE(ACLK_AV1, "aclk_av1", "aclk_av1_pre", 0,
 			RK3588_CLKGATE_CON(68), 2, GFLAGS),
+};
 
+static struct rockchip_clk_branch rk3588_clk_branches[] = {
 	GATE_LINK(ACLK_ISP1_PRE, "aclk_isp1_pre", "aclk_isp1_root", ACLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 6, GFLAGS),
 	GATE_LINK(HCLK_ISP1_PRE, "hclk_isp1_pre", "hclk_isp1_root", HCLK_VI_ROOT, 0, RK3588_CLKGATE_CON(26), 8, GFLAGS),
-	GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, RK3588_LINKED_CLK, RK3588_CLKGATE_CON(31), 2, GFLAGS),
+	GATE_LINK(HCLK_NVM, "hclk_nvm", "hclk_nvm_root", ACLK_NVM_ROOT, 0, RK3588_CLKGATE_CON(31), 2, GFLAGS),
 	GATE_LINK(ACLK_USB, "aclk_usb", "aclk_usb_root", ACLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 2, GFLAGS),
 	GATE_LINK(HCLK_USB, "hclk_usb", "hclk_usb_root", HCLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(42), 3, GFLAGS),
 	GATE_LINK(ACLK_JPEG_DECODER_PRE, "aclk_jpeg_decoder_pre", "aclk_jpeg_decoder_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(44), 7, GFLAGS),
@@ -2443,9 +2425,9 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 	GATE_LINK(HCLK_RKVDEC1_PRE, "hclk_rkvdec1_pre", "hclk_rkvdec1_root", HCLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(41), 4, GFLAGS),
 	GATE_LINK(ACLK_RKVDEC1_PRE, "aclk_rkvdec1_pre", "aclk_rkvdec1_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(41), 5, GFLAGS),
 	GATE_LINK(ACLK_HDCP0_PRE, "aclk_hdcp0_pre", "aclk_vo0_root", ACLK_VOP_LOW_ROOT, 0, RK3588_CLKGATE_CON(55), 9, GFLAGS),
-	GATE_LINK(HCLK_VO0, "hclk_vo0", "hclk_vo0_root", HCLK_VOP_ROOT, RK3588_LINKED_CLK, RK3588_CLKGATE_CON(55), 5, GFLAGS),
+	GATE_LINK(HCLK_VO0, "hclk_vo0", "hclk_vo0_root", HCLK_VOP_ROOT, 0, RK3588_CLKGATE_CON(55), 5, GFLAGS),
 	GATE_LINK(ACLK_HDCP1_PRE, "aclk_hdcp1_pre", "aclk_hdcp1_root", ACLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(59), 6, GFLAGS),
-	GATE_LINK(HCLK_VO1, "hclk_vo1", "hclk_vo1_root", HCLK_VO1USB_TOP_ROOT, RK3588_LINKED_CLK, RK3588_CLKGATE_CON(59), 9, GFLAGS),
+	GATE_LINK(HCLK_VO1, "hclk_vo1", "hclk_vo1_root", HCLK_VO1USB_TOP_ROOT, 0, RK3588_CLKGATE_CON(59), 9, GFLAGS),
 	GATE_LINK(ACLK_AV1_PRE, "aclk_av1_pre", "aclk_av1_root", ACLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(68), 1, GFLAGS),
 	GATE_LINK(PCLK_AV1_PRE, "pclk_av1_pre", "pclk_av1_root", HCLK_VDPU_ROOT, 0, RK3588_CLKGATE_CON(68), 4, GFLAGS),
 	GATE_LINK(HCLK_SDIO_PRE, "hclk_sdio_pre", "hclk_sdio_root", HCLK_NVM, 0, RK3588_CLKGATE_CON(75), 1, GFLAGS),
@@ -2453,14 +2435,18 @@ static struct rockchip_clk_branch rk3588_clk_branches[] __initdata = {
 	GATE_LINK(PCLK_VO1GRF, "pclk_vo1grf", "pclk_vo1_root", HCLK_VO1, CLK_IGNORE_UNUSED, RK3588_CLKGATE_CON(59), 12, GFLAGS),
 };
 
-static void __init rk3588_clk_init(struct device_node *np)
+static void __init rk3588_clk_early_init(struct device_node *np)
 {
 	struct rockchip_clk_provider *ctx;
-	unsigned long clk_nr_clks;
+	unsigned long clk_nr_clks, max_clk_id1, max_clk_id2;
 	void __iomem *reg_base;
 
-	clk_nr_clks = rockchip_clk_find_max_clk_id(rk3588_clk_branches,
-					ARRAY_SIZE(rk3588_clk_branches)) + 1;
+	max_clk_id1 = rockchip_clk_find_max_clk_id(rk3588_clk_branches,
+					ARRAY_SIZE(rk3588_clk_branches));
+	max_clk_id2 = rockchip_clk_find_max_clk_id(rk3588_early_clk_branches,
+					ARRAY_SIZE(rk3588_early_clk_branches));
+	clk_nr_clks = max(max_clk_id1, max_clk_id2) + 1;
+
 	reg_base = of_iomap(np, 0);
 	if (!reg_base) {
 		pr_err("%s: could not map cru region\n", __func__);
@@ -2473,6 +2459,7 @@ static void __init rk3588_clk_init(struct device_node *np)
 		iounmap(reg_base);
 		return;
 	}
+	early_ctx = ctx;
 
 	rockchip_clk_register_plls(ctx, rk3588_pll_clks,
 				   ARRAY_SIZE(rk3588_pll_clks),
@@ -2491,54 +2478,53 @@ static void __init rk3588_clk_init(struct device_node *np)
 			&rk3588_cpub1clk_data, rk3588_cpub1clk_rates,
 			ARRAY_SIZE(rk3588_cpub1clk_rates));
 
+	rockchip_clk_register_branches(ctx, rk3588_early_clk_branches,
+				       ARRAY_SIZE(rk3588_early_clk_branches));
+
+	rockchip_clk_of_add_provider(np, ctx);
+}
+CLK_OF_DECLARE_DRIVER(rk3588_cru, "rockchip,rk3588-cru", rk3588_clk_early_init);
+
+static int clk_rk3588_probe(struct platform_device *pdev)
+{
+	struct rockchip_clk_provider *ctx = early_ctx;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
 	rockchip_clk_register_branches(ctx, rk3588_clk_branches,
 				       ARRAY_SIZE(rk3588_clk_branches));
 
-	rk3588_rst_init(np, reg_base);
-
+	rk3588_rst_init(np, ctx->reg_base);
 	rockchip_register_restart_notifier(ctx, RK3588_GLB_SRST_FST, NULL);
 
+	/*
+	 * Re-add clock provider, so that the newly added clocks are also
+	 * re-parented and get their defaults configured.
+	 */
+	of_clk_del_provider(np);
 	rockchip_clk_of_add_provider(np, ctx);
-}
 
-CLK_OF_DECLARE(rk3588_cru, "rockchip,rk3588-cru", rk3588_clk_init);
-
-struct clk_rk3588_inits {
-	void (*inits)(struct device_node *np);
-};
-
-static const struct clk_rk3588_inits clk_3588_cru_init = {
-	.inits = rk3588_clk_init,
-};
+	return 0;
+}
 
 static const struct of_device_id clk_rk3588_match_table[] = {
 	{
 		.compatible = "rockchip,rk3588-cru",
-		.data = &clk_3588_cru_init,
 	},
 	{ }
 };
 
-static int __init clk_rk3588_probe(struct platform_device *pdev)
-{
-	const struct clk_rk3588_inits *init_data;
-	struct device *dev = &pdev->dev;
-
-	init_data = device_get_match_data(dev);
-	if (!init_data)
-		return -EINVAL;
-
-	if (init_data->inits)
-		init_data->inits(dev->of_node);
-
-	return 0;
-}
-
 static struct platform_driver clk_rk3588_driver = {
+	.probe		= clk_rk3588_probe,
 	.driver		= {
 		.name	= "clk-rk3588",
 		.of_match_table = clk_rk3588_match_table,
 		.suppress_bind_attrs = true,
 	},
 };
-builtin_platform_driver_probe(clk_rk3588_driver, clk_rk3588_probe);
+
+static int __init rockchip_clk_rk3588_drv_register(void)
+{
+	return platform_driver_register(&clk_rk3588_driver);
+}
+core_initcall(rockchip_clk_rk3588_drv_register);
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 73d2cbdc716b..5807f1d820d6 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -19,8 +19,12 @@
 #include <linux/clk-provider.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
+#include <linux/of_platform.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reboot.h>
+#include <linux/platform_device.h>
 
 #include "../clk-fractional-divider.h"
 #include "clk.h"
@@ -376,7 +380,7 @@ struct rockchip_clk_provider *rockchip_clk_init(struct device_node *np,
 		goto err_free;
 
 	for (i = 0; i < nr_clks; ++i)
-		clk_table[i] = ERR_PTR(-ENOENT);
+		clk_table[i] = ERR_PTR(-EPROBE_DEFER);
 
 	ctx->reg_base = base;
 	ctx->clk_data.clks = clk_table;
@@ -446,6 +450,66 @@ unsigned long rockchip_clk_find_max_clk_id(struct rockchip_clk_branch *list,
 }
 EXPORT_SYMBOL_GPL(rockchip_clk_find_max_clk_id);
 
+static struct platform_device *rockchip_clk_register_pdev(
+		struct platform_device *parent,
+		const char *name,
+		struct device_node *np)
+{
+	struct platform_device_info pdevinfo = {
+		.parent = &parent->dev,
+		.name = name,
+		.fwnode = of_fwnode_handle(np),
+		.of_node_reused = true,
+	};
+
+	return platform_device_register_full(&pdevinfo);
+}
+
+static struct clk *rockchip_clk_register_linked_gate(
+	struct rockchip_clk_provider *ctx,
+	struct rockchip_clk_branch *clkbr)
+{
+	struct clk *linked_clk = ctx->clk_data.clks[clkbr->linked_clk_id];
+	unsigned long flags = clkbr->flags | CLK_SET_RATE_PARENT;
+	struct device_node *np = ctx->cru_node;
+	struct platform_device *parent, *pdev;
+	struct device *dev = NULL;
+	int ret;
+
+	parent = of_find_device_by_node(np);
+	if (!parent) {
+		pr_err("failed to find device for %pOF\n", np);
+		goto exit;
+	}
+
+	pdev = rockchip_clk_register_pdev(parent, clkbr->name, np);
+	put_device(&parent->dev);
+	if (!pdev) {
+		pr_err("failed to register device for clock %s\n", clkbr->name);
+		goto exit;
+	}
+
+	dev = &pdev->dev;
+	pm_runtime_enable(dev);
+	ret = pm_clk_create(dev);
+	if (ret) {
+		pr_err("failed to create PM clock list for %s\n", clkbr->name);
+		goto exit;
+	}
+
+	ret = pm_clk_add_clk(dev, linked_clk);
+	if (ret) {
+		pr_err("failed to setup linked clock for %s\n", clkbr->name);
+	}
+
+exit:
+	return clk_register_gate(dev, clkbr->name,
+				 clkbr->parent_names[0], flags,
+				 ctx->reg_base + clkbr->gate_offset,
+				 clkbr->gate_shift, clkbr->gate_flags,
+				 &ctx->lock);
+}
+
 void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
 				    struct rockchip_clk_branch *list,
 				    unsigned int nr_clk)
@@ -526,6 +590,9 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
 				ctx->reg_base + list->gate_offset,
 				list->gate_shift, list->gate_flags, &ctx->lock);
 			break;
+		case branch_linked_gate:
+			clk = rockchip_clk_register_linked_gate(ctx, list);
+			break;
 		case branch_composite:
 			clk = rockchip_clk_register_branch(list->name,
 				list->parent_names, list->num_parents,
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index fd3b476dedda..0d8e729fe332 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -517,6 +517,7 @@ enum rockchip_clk_branch_type {
 	branch_divider,
 	branch_fraction_divider,
 	branch_gate,
+	branch_linked_gate,
 	branch_mmc,
 	branch_inverter,
 	branch_factor,
@@ -544,6 +545,7 @@ struct rockchip_clk_branch {
 	int				gate_offset;
 	u8				gate_shift;
 	u8				gate_flags;
+	unsigned int			linked_clk_id;
 	struct rockchip_clk_branch	*child;
 };
 
@@ -842,6 +844,20 @@ struct rockchip_clk_branch {
 		.gate_flags	= gf,				\
 	}
 
+#define GATE_LINK(_id, cname, pname, linkedclk, f, o, b, gf)	\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_linked_gate,		\
+		.name		= cname,			\
+		.parent_names	= (const char *[]){ pname },	\
+		.linked_clk_id	= linkedclk,			\
+		.num_parents	= 1,				\
+		.flags		= f,				\
+		.gate_offset	= o,				\
+		.gate_shift	= b,				\
+		.gate_flags	= gf,				\
+	}
+
 #define MMC(_id, cname, pname, offset, shift)			\
 	{							\
 		.id		= _id,				\
-- 
2.43.0

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Dmitry Osipenko]

On 1/26/24 21:18, Sebastian Reichel wrote:
> Recent Rockchip SoCs have a new hardware block called Native Interface
> Unit (NIU), which gates clocks to devices behind them. These effectively
> need two parent clocks.
> 
> GATE_LINK type clocks handle the second parent via 'linkedclk' by using
> runtime PM clocks. To make that possible a new platform device is created
> for every clock handled in this way.
> 
> Note, that before this patch clk_rk3588_probe() has never been called,
> because CLK_OF_DECLARE marks the DT node as processed. This patch replaces
> that with CLK_OF_DECLARE_DRIVER and thus the probe function is used now.
> This is necessary to have 'struct device' available.
> 
> Also instead of builtin_platform_driver_probe, the driver has been
> switched to use core_initcall, since it should be fully probed before
> the Rockchip PM domain driver (and that is using postcore_initcall).

Why clk driver needs to be fully probed before PD? The PD driver
shouldn't probe until all clk providers that it uses are registered, and
then both clk and PD should be registered at the default level.

-- 
Best regards,
Dmitry

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

Hi Dmitry,

On Fri, Jan 26, 2024 at 10:36:13PM +0300, Dmitry Osipenko wrote:
> On 1/26/24 21:18, Sebastian Reichel wrote:
> > Recent Rockchip SoCs have a new hardware block called Native Interface
> > Unit (NIU), which gates clocks to devices behind them. These effectively
> > need two parent clocks.
> > 
> > GATE_LINK type clocks handle the second parent via 'linkedclk' by using
> > runtime PM clocks. To make that possible a new platform device is created
> > for every clock handled in this way.
> > 
> > Note, that before this patch clk_rk3588_probe() has never been called,
> > because CLK_OF_DECLARE marks the DT node as processed. This patch replaces
> > that with CLK_OF_DECLARE_DRIVER and thus the probe function is used now.
> > This is necessary to have 'struct device' available.
> > 
> > Also instead of builtin_platform_driver_probe, the driver has been
> > switched to use core_initcall, since it should be fully probed before
> > the Rockchip PM domain driver (and that is using postcore_initcall).
> 
> Why clk driver needs to be fully probed before PD? The PD driver
> shouldn't probe until all clk providers that it uses are registered, and
> then both clk and PD should be registered at the default level.

The error handling in the rockchip PD driver needs rework to
properly handle -EPROBE_DEFER, which I consider a separate series.

Note, that the driver currently has 'builtin_platform_driver_probe',
but does not actually probe anything. All clocks are registered via
CLK_OF_DECLARE, which happens even before core_initcall. So this
does not make things worse.

Also the OF node is marked as initialized by the early clocks
(CLK_OF_DECLARE_DRIVER) via the call to of_clk_add_provider(). This
is necessary, since otherwise the early clocks cannot be referenced
and we need the early clocks for the timer registration (so it's not
possible to move all the clocks to late init). This effectively
results in fw_devlink not working properly. It will tell PM domain
driver too early, that it may start probing (so a bunch of useless
-EPROBE_DEFER will happen).

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 7/7] clk: rockchip: implement proper GATE_LINK support

[Dmitry Osipenko]

On 1/30/24 17:47, Sebastian Reichel wrote:
> Hi Dmitry,
> 
> On Fri, Jan 26, 2024 at 10:36:13PM +0300, Dmitry Osipenko wrote:
>> On 1/26/24 21:18, Sebastian Reichel wrote:
>>> Recent Rockchip SoCs have a new hardware block called Native Interface
>>> Unit (NIU), which gates clocks to devices behind them. These effectively
>>> need two parent clocks.
>>>
>>> GATE_LINK type clocks handle the second parent via 'linkedclk' by using
>>> runtime PM clocks. To make that possible a new platform device is created
>>> for every clock handled in this way.
>>>
>>> Note, that before this patch clk_rk3588_probe() has never been called,
>>> because CLK_OF_DECLARE marks the DT node as processed. This patch replaces
>>> that with CLK_OF_DECLARE_DRIVER and thus the probe function is used now.
>>> This is necessary to have 'struct device' available.
>>>
>>> Also instead of builtin_platform_driver_probe, the driver has been
>>> switched to use core_initcall, since it should be fully probed before
>>> the Rockchip PM domain driver (and that is using postcore_initcall).
>>
>> Why clk driver needs to be fully probed before PD? The PD driver
>> shouldn't probe until all clk providers that it uses are registered, and
>> then both clk and PD should be registered at the default level.
> 
> The error handling in the rockchip PD driver needs rework to
> properly handle -EPROBE_DEFER, which I consider a separate series.
> 
> Note, that the driver currently has 'builtin_platform_driver_probe',
> but does not actually probe anything. All clocks are registered via
> CLK_OF_DECLARE, which happens even before core_initcall. So this
> does not make things worse.
> 
> Also the OF node is marked as initialized by the early clocks
> (CLK_OF_DECLARE_DRIVER) via the call to of_clk_add_provider(). This
> is necessary, since otherwise the early clocks cannot be referenced
> and we need the early clocks for the timer registration (so it's not
> possible to move all the clocks to late init). This effectively
> results in fw_devlink not working properly. It will tell PM domain
> driver too early, that it may start probing (so a bunch of useless
> -EPROBE_DEFER will happen).

Thanks for the clarification! Definitely will be good to improve the
probe defer handling in the future. And indeed, it can be done
separately from this patchset.

-- 
Best regards,
Dmitry