Hi Jonas,
On 10/16/26 11:03 PM, Jonas Karlman wrote:
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.
We probably should document the expected negative values for specific
errors so drivers know what to actually report instead of just picking
one. This is something we should fix at the clock subsystem level.
I'm honestly confused by clk_enable and clk_enable_bulk having a
different behavior for a clock that isn't supported by the clock driver
(or if the driver doesn't have an .enable ops). clk_enable will return
-ENOSYS on .enable ops being NULL, but clk_enable_bulk will happily eat
the return code and return 0 if that's the case. clk_enable will return
whatever the return code of the .enable ops is, even if it's -ENOSYS,
but clk_enable_bulk will happily eat -ENOSYS and return 0. Same for the
clk_disable, clk_disable_bulk.
But considering the above, yes, maybe returning ENOSYS in the Rockchip
CRU drivers is the better option and I would not have much to say
against the patch anymore (except that the clock subsystem seems really
inconsistent and confusing, to me).
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.
Yeah but this means we need to duplicate this work-around in all
consumers of Rockchip CRU clocks, so they know how to handle enable for
a clock that isn't supported by the driver. This doesn't scale.
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 ;-)
Instead, we duplicate this logic across all drivers. See
drivers/phy/qcom/phy-qcom-usb-hs-28nm.c
drivers/phy/qcom/phy-qcom-usb-ss.c
drivers/reset/reset-uniphier.c
drivers/usb/dwc3/dwc3-generic.c
drivers/usb/host/dwc2.c
drivers/usb/host/ehci-msm.c
drivers/usb/host/xhci-dwc3.c
drivers/core/simple-pm-bus.c
for examples of optional clk_get_bulk() and
drivers/serial/ns16550.c
drivers/serial/serial_esp32.c
drivers/serial/serial_mt7620.c
drivers/serial/serial_omap.c
for examples of optional clk_get_by_index(). I don't find this to be a
good idea. Especially as it makes reading the code quite difficult to
follow.But since other drivers already do this, I cannot reasonably
expect you to fix this now, so... fine :)
Please switch the existing .enable ops to return -ENOSYS on unsupported
clocks for Rockchip drivers and update clk_enable_bulk() in the
rockchip_rng driver to only check for a negative ret.
Cheers,
Quentin
