Hi Quentin,
On 10/16/2026 1:11 PM, Quentin Schulz wrote:
> Hi Jonas,
>
> On 10/4/26 11:13 PM, Jonas Karlman wrote:
>> The vendor TF-A blobs for RK3562 leave the TRNG and RK RNG clocks gated
>> when returning to U-Boot proper.
>>
>> Try to enable clocks while reading or otherwise accessing TRNG/RKRNG
>> regs to ensure random data can successfully be read on affected SoCs.
>>
>> This is not expected to have any impact on SoCs using a clock driver
>> with unimplemented enable/disable ops.
>>
>> Signed-off-by: Jonas Karlman <[email protected]>
>> ---
> [...]
>> @@ -306,6 +308,10 @@ static int rockchip_rng_read(struct udevice *dev, void
>> *data, size_t len)
>> if (!pdata->soc_data || !pdata->soc_data->rk_rng_read)
>> return -EINVAL;
>>
>> + ret = clk_enable_bulk(&pdata->clks);
>> + if (ret && ret != -ENOENT)
>
> Because I was confused that clk_enable_bulk() could return -ENOENT,
> maybe add a comment stating this is to catch clock drivers that
> implement a .enable() callback and return -ENOENT for unsupported
> clocks? RK3399 and PX30 seem to be the only one doing that on Rockchip.
> Same comment for rockchip_rng_probe().
Correct, this is because existing .enable() ops may return -ENOENT for
unsupported clocks, for pending SoC support in my rk3562 branch the
clock drivers instead may return -ENOSYS for unsupported clocks.
I could possible add a patch that changes existing clock drivers to
instead return -ENOSYS, then this check would be unnecessary. I will
otherwise add a small comment here for a v2.
> I'm also not entirely sure this is proper, shouldn't we rather implement
> support for the clocks instead of just saying "it's fine, don't worry"?
> I think only RK3399 would be impacted (as only RK3399 and PX30 clock
> drivers implement an .enable, and PX30 DTS doesn't have clocks
> specified), with missing support for HCLK_M_CRYPTO[01],
> HCLK_S_CRYPTO[01], and SCLK_CRYPTO[01].
We typically do not need to have enable/disable support in Rockchip
clock drivers, only for clocks that is disabled out of system reset, or
as in the mentioned case, because TF-A on RK3562 disables these clocks
before returning to U-Boot proper. Also we first should/need to have
proper clk enable_count handling before we try to add new enable/disable
ops for Rockchip clock drivers or we can very easily disable clocks that
are needed by multiple devices.
> While looking at it, I also saw that there are resets in the node, maybe
> something we'll need to control in the future too?
Out of all RK SoCs I have been testing 'rng' command on, only the RK3562
is having any kind of issue, and only because vendor TF-A blob
explicitly disable the clocks used for crypto/rng block.
A different hack/workaround would be to ungate the affected clocks in
RK3562 clock driver probe(), but that is something I rather avoid.
>
>> + return ret;
>> +
>> for (i = 0; i < len / RK_HW_RNG_MAX; i++, buf += RK_HW_RNG_MAX) {
>> ret = pdata->soc_data->rk_rng_read(dev, buf, RK_HW_RNG_MAX);
>> if (ret)
>> @@ -317,6 +323,7 @@ static int rockchip_rng_read(struct udevice *dev, void
>> *data, size_t len)
>> len % RK_HW_RNG_MAX);
>>
>> exit:
>> + clk_disable_bulk(&pdata->clks);
>> return ret;
>> }
>>
>> @@ -336,12 +343,22 @@ static int rockchip_rng_of_to_plat(struct udevice *dev)
>> static int rockchip_rng_probe(struct udevice *dev)
>> {
>> struct rk_rng_plat *pdata = dev_get_priv(dev);
>> - int ret = 0;
>> + int ret;
>> +
>> + ret = clk_get_bulk(dev, &pdata->clks);
>> + if (ret && ret != -ENOENT && ret != -ENOSYS)
>
> I think what you want is clk_bulk_get_optional() from the Linux kernel,
> so please implement that in U-Boot. We have it for reset, clk (not for
> bulk yet) and gpio, so this is the right way to go.
Hum, in U-Boot we have clk_get_bulk() and Linux have clk_bulk_get() with
different set of parameters, so adding one more non-conformant clock
function to U-Boot is not a can of worms I am willing to open ;-)
Regards,
Jonas
>
> Cheers,
> Quentin
