Re: [PATCH v3 0/2] rng: Provide a RNG based on the RISC-V Zkr ISA extension

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

On Sat, Nov 04, 2023 at 05:12:12PM +0000, Andre Przywara wrote:
> On Fri, 3 Nov 2023 13:38:58 -0600
> Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Simon,
> 
> > Hi Heinrich,
> > 
> > On Wed, 1 Nov 2023 at 14:20, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > > On 11/1/23 19:05, Andre Przywara wrote:  
> > > > On Tue, 31 Oct 2023 14:55:50 +0200
> > > > Heinrich Schuchardt <heinrich.schuchardt@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.przywara@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 think we have a similar problem with how we're doing with DM_TIMER and
armv7-a/armv7-m/armv8[1]. We shouldn't need the drivers in drivers/timer/
to cover platforms where SYS_ARCH_TIMER is (or should be!) enabled. But
in turn, the code under arch/arm/cpu/*/*timer.c doesn't implement the
uclass side of things, only the regular API. This is because there's
nothing to probe even because we don't support the kind of multi-arch
binary where it'd be possible to not have the feature.

-- 
Tom

[1]: We do have the problem of armv7-r not having this feature so things
like say TI K3 platforms need the platform driver on the Cortex-R host.
A similar issue is the pre-ARMv7 i.MX platforms.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]