On 11/4/23 21:45, Simon Glass wrote:
Hi Andre,
On Sat, 4 Nov 2023 at 17:13, Andre Przywara <andre.przyw...@arm.com> wrote:
On Fri, 3 Nov 2023 13:38:58 -0600
Simon Glass <s...@chromium.org> wrote:
Hi Simon,
Hi Heinrich,
On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:
On 11/1/23 19:05, Andre Przywara wrote:
On Tue, 31 Oct 2023 14:55:50 +0200
Heinrich Schuchardt <heinrich.schucha...@canonical.com> wrote:
Hi Heinrich,
The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It
provides an interface to a physical entropy source.
A RNG driver based on the seed CSR is provided. It depends on
mseccfg.sseed being set in the SBI firmware.
As you might have seen, I added a similar driver for the respective Arm
functionality:
https://lore.kernel.org/u-boot/20230830113230.3925868-1-andre.przyw...@arm.com/
And I see that you seem to use the same mechanism to probe and init the
driver: U_BOOT_DRVINFO and fail in probe() if the feature is not
implemented.
One downside of this approach is that the driver is always loaded (and
visible in the DM tree), even with the feature not being available.
That doesn't seem too much of a problem on the first glance, but it
occupies a device number, and any subsequent other DM_RNG devices
(like virtio-rng) typically get higher device numbers. So without
the feature, but with virtio-rng, I get:
VExpress64# rng 0
No RNG device
Why do we get this? If the device is not there, the bind() function
can return -ENODEV
I see this in U-Boot:
U_BOOT_DRVINFO(cpu_arm_rndr) = {
We should not use this.
Agreed.
Use the devicetree.
No, this is definitely not something for the DT, at least not on ARM.
It's perfectly discoverable via the architected CPU ID registers.
Similar to PCI and USB devices, which we don't probe via the DT as well.
It's arguably not proper "driver" material per se, as I've argued before, but
it's the simplest solution and fits in nicely otherwise.
I was wondering if it might be something for UCLASS_CPU, something like
a "CPU feature bus": to let devices register on one on the many CPU
features (instead of compatible strings), then only bind() those
drivers it the respective bit is set.
Does that make sense? Would that be doable without boiling the ocean?
As I don't know if we see many users apart from this.
I have seen this so many times, where people want to avoid putting
things in the DT and then are surprised that everything is difficult,
broken and confusing. Why not just follow the rules? It is not just
about whether we can avoid it, etc. It is about how devices fit
together cohesively in the system, and how U-Boot operates.
The Zkr extension isn't a device. It is an instruction set architecture
extension. It is represented by the ISA extension strings of the
respective harts in the device-tree.
If the seed register of the hart can be accessed by U-Boot, is
determined by bit sseed in the mseccfg register.
I don't expect the encoding of RISC-V ISA extensions in device-trees to
be changed.
Best regards
Heinrich
VExpress64# rng 1
00000000: f3 88 b6 d4 24 da 49 ca 49 f7 9e 66 5f 12 07 b2 ....$.I.I..f_...
Essentially in any case were you have multiple drivers for the same
device using uclass_get_device(, 0, ) and uclass_find_first_device()
will only give you the first bound device and not the first successfully
probed device. Furthermore neither of this functions causes probing.
This is not restricted to the RNG drivers but could also happen with
multiple TPM drivers or multiple watchdogs.
This patch is related to the problem:
[PATCH v1] rng: add dm_rng_read_default() helper
https://lore.kernel.org/u-boot/4e28a388-f5b1-4cf7-b0e3-b12a876d0...@gmx.de/T/#me44263ec9141e3ea65ee232aa9a411fc6201bd95
We have weak function platform_get_rng_device() which should be moved to
drivers/rng/rng-uclass.c.
We could add a function to drivers/core/uclass.c to retrieve the first
successfully probed device. Another approach would be to implement
uclass_driver.post_probe() in the RNG uclass to take note of the first
successfully probed device.
@Simon:
What would make most sense from a DM design standpoint?
I am sure I provided feedback on this at the time, but I don't
remember. OK I just found it here [1]. So the problem is entirely
because my feedback was not addressed. Please just address it and
avoid this sort of mess.
Yeah, Tom just merged it, but that's not Heinrich's fault ;-)
So arm_rndr should have a devicetree compatible string and be bound
like anything else. If for some reason the device doesn't exist in the
hardware, it can return -ENODEV from its bind() method.
If you want to control which RNG device is used for booting, you could
add a property to /bootstd with a phandle to the device. We are trying
to provide a standard approach to booting in U-Boot, used by all
methods. Doing one-off things for particular cases is best avoided.
Picking the first usable device doesn't sound much like a one-off to me.
After all the caller (be it UEFI or the rng command) later detect that
this is not usable. So there might be some merit to cover this more
automatically, either in the caller, or by providing a suitable wrapper
function?
Or just follow the existing mechanisms which have been in U-Boot for
years. Please...!
[..]
Regards,
Simon
>
[1]
https://patchwork.ozlabs.org/project/uboot/patch/20230830113230.3925868-1-andre.przyw...@arm.com/
