Hi Heinrich, On Sat, 4 Nov 2023 at 20:36, Heinrich Schuchardt <heinrich.schucha...@canonical.com> wrote: > > 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.
Please just give up with the 'not a device' thing. Let's just say that a device is a software concept and how we model hardware. This 'not a device' thing is creating all sorts of problems, to no end. Nothing is changed until someone sends a patch. Regards, Simon > > 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/ > >> >