[PATCH v5 4/9] tpm: Add the RNG child device

[Simon Glass]

Hi Ilias,

On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, 14 Mar 2022 at 20:24, Simon Glass <sjg at chromium.org> wrote:
> >
>
> [...]
>
> > > > > +       }
> > > >
> > > > This really should be in the device tree so what you are doing here is
> > > > quite strange.
> > >
> > > Like I had mentioned in my earlier emails, the TPM device has a
> > > builtin RNG functionality, which is non-optional. So I don't
> > > understand why we need to use a device tree subnode here. Whether the
> > > device is being bound to the parent is being controlled by the TPM_RNG
> > > config that you asked me to put in my previous version, which I am
> > > doing.
> >
> > See how PMICs work, for example. We have GPIOs, regulators and
> > suchlike in the PMIC and we add subnodes for them in the DT. It is
> > just how it is done.
> >
> > Driver model is designed to automatically bind devices based on the
> > device tree. There are cases where it is necessary to manually bind
> > things, but we mustn't prevent people from doing it 'properly'.
>
> There's a small difference here though.  The RNG is not a device.  The
> TPM is the device and an encoded command to that device returns a
> random number.  There's no binding initiating etc.

A device is just something with a struct udevice, so I don't see the
random number generator as anything different from another device. We
might have a white-noise generator which produces random numbers. Just
because the feature is packaged inside a single chip doesn't make it
any less a device. Just like the PMIC.

>
> >
> > Finally, I know you keep saying that random numbers are only needed in
> > U-Boot proper, but if I want a random number in SPL, it may not work,
> > since device_bind() is often not available, for code-size reasons.
>
> And the entire tpm framework will fit?

Yes. For verified boot it has to, since you cannot init RAM until you
have selected your A/B/recovery image.

>
> >
> > So that is why I say that what you are doing is quite strange. Perhaps
> > you are coming from a different project, which does things
> > differently.
>
> I don't personally find it strange.  The device is already described
> in the DTS and I don't see a strong reason to deviate for the upstream
> version again.

Linux tends to rely a lot more on manually adding devices. It can have
a pretty dramatic bloating effect on code size in U-Boot.

Anyway, so long as we can detect an existing device, as I explained
below, it is fine to manually add it when it is missing.


>
> Regards
> /Ilias
> >
> > >
> > >  If you want to manually bind it, please call
> > > > device_find_first_child_by_uclass() first to make sure it isn't
> > > > already there.
> > >
> > > Okay
> > >
> > > >
> > > > Also you should bind it in the bind() method, not in probe().
> > >
> > > Okay
> > >
> > > >
> > > > This is the code used for the same thing in the bootstd series:
> > > >
> > > > struct udevice *bdev;
> > > > int ret;
> > > >
> > > > ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev);
> > > > if (ret) {
> > > >    if (ret != -ENODEV) {
> > > >    log_debug("Cannot access bootdev device\n");
> > > >    return ret;
> > > >    }
> > > >
> > > >    ret = bootdev_bind(parent, drv_name, "bootdev", &bdev);
> > > >    if (ret) {
> > > >       log_debug("Cannot create bootdev device\n");
> > > >       return ret;
> > > >    }
> > > > }
> > > >
> > > >
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > >  UCLASS_DRIVER(tpm) = {
> > > > > -       .id             = UCLASS_TPM,
> > > > > -       .name           = "tpm",
> > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > +       .id                     = UCLASS_TPM,
> > > > > +       .name                   = "tpm",
> > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > >  #endif
> > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > >
> > > > Should be post_bind.
> > >
> > > Okay
> >
> > [..]

Regards,
Simon