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

[Chad LeClair <leclair@gmail.com>]

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