diff mbox series

[v6,3/7] tpm: Add the RNG child device

Message ID 20220704133444.1110715-4-sughosh.ganu@linaro.org
State Superseded
Headers show
Series tpm: rng: Move TPM RNG functionality to driver model | expand

Commit Message

Sughosh Ganu July 4, 2022, 1:34 p.m. UTC
The TPM device comes with the random number generator(RNG)
functionality which is built into the TPM device. Add logic to add the
RNG child device in the TPM uclass post probe callback.

The RNG device can then be used to pass a set of random bytes to the
linux kernel, need for address space randomisation through the
EFI_RNG_PROTOCOL interface.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5:
* Check if the TPM RNG device has already been added, through a call
  to device_find_first_child_by_uclass()

 drivers/tpm/tpm-uclass.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

Comments

Simon Glass July 5, 2022, 9:47 a.m. UTC | #1
Hi Sughosh,

On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device comes with the random number generator(RNG)
> functionality which is built into the TPM device. Add logic to add the
> RNG child device in the TPM uclass post probe callback.
>
> The RNG device can then be used to pass a set of random bytes to the
> linux kernel, need for address space randomisation through the
> EFI_RNG_PROTOCOL interface.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5:
> * Check if the TPM RNG device has already been added, through a call
>   to device_find_first_child_by_uclass()
>
>  drivers/tpm/tpm-uclass.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index f67fe1019b..e1f1ef01e1 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -11,10 +11,15 @@
>  #include <log.h>
>  #include <linux/delay.h>
>  #include <linux/unaligned/be_byteshift.h>
> +#include <tpm_api.h>
>  #include <tpm-v1.h>
>  #include <tpm-v2.h>
>  #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG_DRV_NAME       "tpm-rng"
> +
>  int tpm_open(struct udevice *dev)
>  {
>         struct tpm_ops *ops = tpm_get_ops(dev);
> @@ -136,12 +141,36 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>         return 0;
>  }
>
> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +       int ret;
> +       const char *drv = TPM_RNG_DRV_NAME;
> +       struct udevice *child;
> +
> +       if (CONFIG_IS_ENABLED(TPM_RNG)) {
> +               ret = device_find_first_child_by_uclass(dev, UCLASS_RNG,
> +                                                       &child);
> +
> +               if (ret != -ENODEV) {

This may return -EIO (for example) in which case your log message is incorrect.


> +                       log_debug("RNG child already added to the TPM device\n");
> +                       return ret;
> +               }
> +
> +               ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +               if (ret)
> +                       return log_msg_ret("bind", 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,
>         .per_device_auto        = sizeof(struct tpm_chip_priv),
>  };
> --
> 2.25.1
>

The driver needs a compatible string so it can be in the device tree.

Regards,
Simon
Ilias Apalodimas July 8, 2022, 8:23 a.m. UTC | #2
Hi Simon,

[...]

> > +
> >  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,
> >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> >  };
> > --
> > 2.25.1
> >
>
> The driver needs a compatible string so it can be in the device tree.

Why?  I've tried to hint this on the previous iteration of the patch.
The RNG here is not a *device*.  The TPM is the device and you are
guaranteed to have an RNG.  The way to get a random number is send a
special command to the TPM. So all that we should do here is leverage
the fact that the TPM is already in the device tree.

And fwiw we should stick to try to stick to what the DT spec defines
as much as possible.  I personally don't see this as a special usecase
were deviating from the spec is justified.

Regards
/Ilias
>
> Regards,
> Simon
Simon Glass July 12, 2022, 10:58 a.m. UTC | #3
Hi Ilias,

On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > +
> > >  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,
> > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > >  };
> > > --
> > > 2.25.1
> > >
> >
> > The driver needs a compatible string so it can be in the device tree.
>
> Why?  I've tried to hint this on the previous iteration of the patch.
> The RNG here is not a *device*.  The TPM is the device and you are
> guaranteed to have an RNG.  The way to get a random number is send a
> special command to the TPM. So all that we should do here is leverage
> the fact that the TPM is already in the device tree.
>
> And fwiw we should stick to try to stick to what the DT spec defines
> as much as possible.  I personally don't see this as a special usecase
> were deviating from the spec is justified.

This is not a deviation from a spec. What spec? Also, I don't want to
get into another discussion about what a device is. We can disagree on
that if you like.

One reason is that U-Boot generally requires compatible strings, e.g.
with of-platdata. But also we can refer to the rand device from
elsewhere in the tree. I know that Linux does lots of ad-hoc device
creation and doesn't really have the concept of a uclass consistently
applied, but this is U-Boot.

So please add a compatible string sow e can use it in the DT.

\Regards,
Simon
Rob Herring (Arm) July 12, 2022, 2:11 p.m. UTC | #4
On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > [...]
> >
> > > > +
> > > >  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,
> > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > >  };
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > The driver needs a compatible string so it can be in the device tree.
> >
> > Why?  I've tried to hint this on the previous iteration of the patch.
> > The RNG here is not a *device*.  The TPM is the device and you are
> > guaranteed to have an RNG.  The way to get a random number is send a
> > special command to the TPM. So all that we should do here is leverage
> > the fact that the TPM is already in the device tree.
> >
> > And fwiw we should stick to try to stick to what the DT spec defines
> > as much as possible.  I personally don't see this as a special usecase
> > were deviating from the spec is justified.
>
> This is not a deviation from a spec. What spec? Also, I don't want to
> get into another discussion about what a device is. We can disagree on
> that if you like.
>
> One reason is that U-Boot generally requires compatible strings, e.g.
> with of-platdata. But also we can refer to the rand device from
> elsewhere in the tree. I know that Linux does lots of ad-hoc device
> creation and doesn't really have the concept of a uclass consistently
> applied, but this is U-Boot.

You are letting client/OS details define your binding. Doing so
doesn't result in OS agnostic bindings. Sure, it would be nice if DT
nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
sometimes and DT is not an abstraction layer. The general guidance on
whether there are child nodes for sub-blocks is do they have their own
resources in DT or are the sub-blocks reusable on multiple devices.
Neither of those are the case here.

Besides, we already have TPM device bindings defined. Requiring
binding changes when adding a new client/OS feature is not good
practice either.

Rob
Simon Glass July 13, 2022, 3:28 p.m. UTC | #5
Hi Rob,

On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > +
> > > > >  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,
> > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > >  };
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> > > > The driver needs a compatible string so it can be in the device tree.
> > >
> > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > The RNG here is not a *device*.  The TPM is the device and you are
> > > guaranteed to have an RNG.  The way to get a random number is send a
> > > special command to the TPM. So all that we should do here is leverage
> > > the fact that the TPM is already in the device tree.
> > >
> > > And fwiw we should stick to try to stick to what the DT spec defines
> > > as much as possible.  I personally don't see this as a special usecase
> > > were deviating from the spec is justified.
> >
> > This is not a deviation from a spec. What spec? Also, I don't want to
> > get into another discussion about what a device is. We can disagree on
> > that if you like.
> >
> > One reason is that U-Boot generally requires compatible strings, e.g.
> > with of-platdata. But also we can refer to the rand device from
> > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > creation and doesn't really have the concept of a uclass consistently
> > applied, but this is U-Boot.
>
> You are letting client/OS details define your binding. Doing so
> doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> sometimes and DT is not an abstraction layer. The general guidance on
> whether there are child nodes for sub-blocks is do they have their own
> resources in DT or are the sub-blocks reusable on multiple devices.
> Neither of those are the case here.
>
> Besides, we already have TPM device bindings defined. Requiring
> binding changes when adding a new client/OS feature is not good
> practice either.

I'm not sure what to do with this, but in general the practice of
implied subnodes is not friendly to U-Boot. Yet it seems like a common
feature of the bindings at present, for example GPIOs.

The device tree is how U-Boot determines which random-number generator
to use for a particular function. For example, for VBE, if we need to
generate some random numbers we'd like to specify which device creates
them. It can't just be 'use the TPM if you find one'. I'm not sure how
that works in Linux?

Regards,
SImon
Tom Rini July 13, 2022, 6:09 p.m. UTC | #6
On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> Hi Rob,
> 
> On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > +
> > > > > >  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,
> > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > >  };
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > >
> > > > > The driver needs a compatible string so it can be in the device tree.
> > > >
> > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > special command to the TPM. So all that we should do here is leverage
> > > > the fact that the TPM is already in the device tree.
> > > >
> > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > as much as possible.  I personally don't see this as a special usecase
> > > > were deviating from the spec is justified.
> > >
> > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > get into another discussion about what a device is. We can disagree on
> > > that if you like.
> > >
> > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > with of-platdata. But also we can refer to the rand device from
> > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > creation and doesn't really have the concept of a uclass consistently
> > > applied, but this is U-Boot.
> >
> > You are letting client/OS details define your binding. Doing so
> > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > sometimes and DT is not an abstraction layer. The general guidance on
> > whether there are child nodes for sub-blocks is do they have their own
> > resources in DT or are the sub-blocks reusable on multiple devices.
> > Neither of those are the case here.
> >
> > Besides, we already have TPM device bindings defined. Requiring
> > binding changes when adding a new client/OS feature is not good
> > practice either.
> 
> I'm not sure what to do with this, but in general the practice of
> implied subnodes is not friendly to U-Boot. Yet it seems like a common
> feature of the bindings at present, for example GPIOs.
> 
> The device tree is how U-Boot determines which random-number generator
> to use for a particular function. For example, for VBE, if we need to
> generate some random numbers we'd like to specify which device creates
> them. It can't just be 'use the TPM if you find one'. I'm not sure how
> that works in Linux?

Why can't it be "use the TPM if you find one" since a TPM is a superset
of an RNG?
Simon Glass July 14, 2022, 10:21 a.m. UTC | #7
Hi Tom,

On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > Hi Rob,
> >
> > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > [...]
> > > > >
> > > > > > > +
> > > > > > >  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,
> > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > >  };
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > >
> > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > >
> > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > special command to the TPM. So all that we should do here is leverage
> > > > > the fact that the TPM is already in the device tree.
> > > > >
> > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > were deviating from the spec is justified.
> > > >
> > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > get into another discussion about what a device is. We can disagree on
> > > > that if you like.
> > > >
> > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > with of-platdata. But also we can refer to the rand device from
> > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > creation and doesn't really have the concept of a uclass consistently
> > > > applied, but this is U-Boot.
> > >
> > > You are letting client/OS details define your binding. Doing so
> > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > sometimes and DT is not an abstraction layer. The general guidance on
> > > whether there are child nodes for sub-blocks is do they have their own
> > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > Neither of those are the case here.
> > >
> > > Besides, we already have TPM device bindings defined. Requiring
> > > binding changes when adding a new client/OS feature is not good
> > > practice either.
> >
> > I'm not sure what to do with this, but in general the practice of
> > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > feature of the bindings at present, for example GPIOs.
> >
> > The device tree is how U-Boot determines which random-number generator
> > to use for a particular function. For example, for VBE, if we need to
> > generate some random numbers we'd like to specify which device creates
> > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > that works in Linux?
>
> Why can't it be "use the TPM if you find one" since a TPM is a superset
> of an RNG?

You mean look through the available rand devices and choose the one
whose parent is a TPM?

Sure. There are lots of things you can do. But device tree is supposed
to provide the tree of devices and allow such things to be configured
deterministically.

Regards,
Simon
Tom Rini July 14, 2022, 11:19 a.m. UTC | #8
On Thu, Jul 14, 2022 at 04:21:58AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > > Hi Rob,
> > >
> > > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > +
> > > > > > > >  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,
> > > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > > >  };
> > > > > > > > --
> > > > > > > > 2.25.1
> > > > > > > >
> > > > > > >
> > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > >
> > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > the fact that the TPM is already in the device tree.
> > > > > >
> > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > were deviating from the spec is justified.
> > > > >
> > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > get into another discussion about what a device is. We can disagree on
> > > > > that if you like.
> > > > >
> > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > with of-platdata. But also we can refer to the rand device from
> > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > applied, but this is U-Boot.
> > > >
> > > > You are letting client/OS details define your binding. Doing so
> > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > whether there are child nodes for sub-blocks is do they have their own
> > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > Neither of those are the case here.
> > > >
> > > > Besides, we already have TPM device bindings defined. Requiring
> > > > binding changes when adding a new client/OS feature is not good
> > > > practice either.
> > >
> > > I'm not sure what to do with this, but in general the practice of
> > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > feature of the bindings at present, for example GPIOs.
> > >
> > > The device tree is how U-Boot determines which random-number generator
> > > to use for a particular function. For example, for VBE, if we need to
> > > generate some random numbers we'd like to specify which device creates
> > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > that works in Linux?
> >
> > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > of an RNG?
> 
> You mean look through the available rand devices and choose the one
> whose parent is a TPM?
> 
> Sure. There are lots of things you can do. But device tree is supposed
> to provide the tree of devices and allow such things to be configured
> deterministically.

No, I mean pick the TPM device because by definition it is an RNG.
Simon Glass July 14, 2022, 2:51 p.m. UTC | #9
Hi Tom,

On Thu, 14 Jul 2022 at 05:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jul 14, 2022 at 04:21:58AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > > > Hi Rob,
> > > >
> > > > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > +
> > > > > > > > >  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,
> > > > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > > > >  };
> > > > > > > > > --
> > > > > > > > > 2.25.1
> > > > > > > > >
> > > > > > > >
> > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > >
> > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > the fact that the TPM is already in the device tree.
> > > > > > >
> > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > were deviating from the spec is justified.
> > > > > >
> > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > that if you like.
> > > > > >
> > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > applied, but this is U-Boot.
> > > > >
> > > > > You are letting client/OS details define your binding. Doing so
> > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > Neither of those are the case here.
> > > > >
> > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > binding changes when adding a new client/OS feature is not good
> > > > > practice either.
> > > >
> > > > I'm not sure what to do with this, but in general the practice of
> > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > feature of the bindings at present, for example GPIOs.
> > > >
> > > > The device tree is how U-Boot determines which random-number generator
> > > > to use for a particular function. For example, for VBE, if we need to
> > > > generate some random numbers we'd like to specify which device creates
> > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > that works in Linux?
> > >
> > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > of an RNG?
> >
> > You mean look through the available rand devices and choose the one
> > whose parent is a TPM?
> >
> > Sure. There are lots of things you can do. But device tree is supposed
> > to provide the tree of devices and allow such things to be configured
> > deterministically.
>
> No, I mean pick the TPM device because by definition it is an RNG.

I think that is what I said, or meant. The TPM is not a rand device,
it is the child of the TPM which might be. But for example, the Google
cr50 may not be.

Regards,
Simon
Ilias Apalodimas July 14, 2022, 3:47 p.m. UTC | #10
Hi Simon,

[...]
> > > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > > >
> > > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > > the fact that the TPM is already in the device tree.
> > > > > > > >
> > > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > > were deviating from the spec is justified.
> > > > > > >
> > > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > > that if you like.
> > > > > > >
> > > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > > applied, but this is U-Boot.
> > > > > >
> > > > > > You are letting client/OS details define your binding. Doing so
> > > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > > Neither of those are the case here.
> > > > > >
> > > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > > binding changes when adding a new client/OS feature is not good
> > > > > > practice either.
> > > > >
> > > > > I'm not sure what to do with this, but in general the practice of
> > > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > > feature of the bindings at present, for example GPIOs.
> > > > >
> > > > > The device tree is how U-Boot determines which random-number generator
> > > > > to use for a particular function. For example, for VBE, if we need to
> > > > > generate some random numbers we'd like to specify which device creates
> > > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > > that works in Linux?
> > > >
> > > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > > of an RNG?
> > >
> > > You mean look through the available rand devices and choose the one
> > > whose parent is a TPM?
> > >
> > > Sure. There are lots of things you can do. But device tree is supposed
> > > to provide the tree of devices and allow such things to be configured
> > > deterministically.
> >
> > No, I mean pick the TPM device because by definition it is an RNG.
>
> I think that is what I said, or meant. The TPM is not a rand device,
> it is the child of the TPM which might be.

The design principle of the tpm1.2 says that it must have an rng [1].
I think something similar is implied on 2.0 architecture as well [2].
Looking at the cr50 description is says "TPM-like" [3]  not TPM.  I
assume an RNG exists internally since the TPM requires and RNG for
nonces, key generation etc.

Any idea what happens if you run tpm_getrandom() against a cr50? If
you get back garbage then we got a problem since you can't really tell
if it's garbage or a random seed.  But if it returns something sane
along the lines of 'not supported' then I guess we can use that and
try the next RNG?

[1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-1-Design-Principles_v1.2_rev116_01032011.pdf
4.2.5 random number generator
[2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
11.4.10 RNG module
[3] https://www.kernel.org/doc/Documentation/devicetree/bindings/security/tpm/google%2Ccr50.txt

Regards
/Ilias

> But for example, the Google
> cr50 may not be.
>
> Regards,
> Simon
Tom Rini July 14, 2022, 4:04 p.m. UTC | #11
On Thu, Jul 14, 2022 at 06:47:28PM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> [...]
> > > > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > > > >
> > > > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > > > the fact that the TPM is already in the device tree.
> > > > > > > > >
> > > > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > > > were deviating from the spec is justified.
> > > > > > > >
> > > > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > > > that if you like.
> > > > > > > >
> > > > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > > > applied, but this is U-Boot.
> > > > > > >
> > > > > > > You are letting client/OS details define your binding. Doing so
> > > > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > > > Neither of those are the case here.
> > > > > > >
> > > > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > > > binding changes when adding a new client/OS feature is not good
> > > > > > > practice either.
> > > > > >
> > > > > > I'm not sure what to do with this, but in general the practice of
> > > > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > > > feature of the bindings at present, for example GPIOs.
> > > > > >
> > > > > > The device tree is how U-Boot determines which random-number generator
> > > > > > to use for a particular function. For example, for VBE, if we need to
> > > > > > generate some random numbers we'd like to specify which device creates
> > > > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > > > that works in Linux?
> > > > >
> > > > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > > > of an RNG?
> > > >
> > > > You mean look through the available rand devices and choose the one
> > > > whose parent is a TPM?
> > > >
> > > > Sure. There are lots of things you can do. But device tree is supposed
> > > > to provide the tree of devices and allow such things to be configured
> > > > deterministically.
> > >
> > > No, I mean pick the TPM device because by definition it is an RNG.
> >
> > I think that is what I said, or meant. The TPM is not a rand device,
> > it is the child of the TPM which might be.
> 
> The design principle of the tpm1.2 says that it must have an rng [1].
> I think something similar is implied on 2.0 architecture as well [2].
> Looking at the cr50 description is says "TPM-like" [3]  not TPM.  I
> assume an RNG exists internally since the TPM requires and RNG for
> nonces, key generation etc.
> 
> Any idea what happens if you run tpm_getrandom() against a cr50? If
> you get back garbage then we got a problem since you can't really tell
> if it's garbage or a random seed.  But if it returns something sane
> along the lines of 'not supported' then I guess we can use that and
> try the next RNG?
> 
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-1-Design-Principles_v1.2_rev116_01032011.pdf
> 4.2.5 random number generator
> [2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> 11.4.10 RNG module
> [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/security/tpm/google%2Ccr50.txt

What I'm saying is from what I understand a spec compliant TPM is an
RNG, so we should be able to match on that existing compatible.  What
the cr50 does or doesn't do (or is or is not compliant about) would
indeed make things more complex but I think also maybe helps the rest of
us understand where you've been coming from.
Rob Herring (Arm) July 14, 2022, 5:55 p.m. UTC | #12
On Wed, Jul 13, 2022 at 9:28 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rob,
>
> On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > +
> > > > > >  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,
> > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > >  };
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > >
> > > > > The driver needs a compatible string so it can be in the device tree.
> > > >
> > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > special command to the TPM. So all that we should do here is leverage
> > > > the fact that the TPM is already in the device tree.
> > > >
> > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > as much as possible.  I personally don't see this as a special usecase
> > > > were deviating from the spec is justified.
> > >
> > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > get into another discussion about what a device is. We can disagree on
> > > that if you like.
> > >
> > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > with of-platdata. But also we can refer to the rand device from
> > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > creation and doesn't really have the concept of a uclass consistently
> > > applied, but this is U-Boot.
> >
> > You are letting client/OS details define your binding. Doing so
> > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > sometimes and DT is not an abstraction layer. The general guidance on
> > whether there are child nodes for sub-blocks is do they have their own
> > resources in DT or are the sub-blocks reusable on multiple devices.
> > Neither of those are the case here.
> >
> > Besides, we already have TPM device bindings defined. Requiring
> > binding changes when adding a new client/OS feature is not good
> > practice either.
>
> I'm not sure what to do with this, but in general the practice of
> implied subnodes is not friendly to U-Boot. Yet it seems like a common
> feature of the bindings at present, for example GPIOs.

It's perfectly normal for a single device to be multiple providers. A
common example is clock AND reset blocks. They are both a clock and
reset provider. So you could have either:

crm {
  compatible = "foo,soc-crm";
  clock-controller {
   compatible = "foo,soc-crm-clocks";
   #clock-cells = <1>;
  };
  reset-controller {
    compatible = "foo,soc-crm-resets";
    #reset-cells = <1>;
  };
};

or:

crm {
  compatible = "foo,soc-crm";
  #clock-cells = <1>;
  #reset-cells = <1>;
};

Which one's right? Well, depends on the h/w really. If clock and reset
controls are interleaved bits or registers, then the first case really
doesn't make sense.

In any case, this ship has sailed. There are tons of the latter case
already, so either you have to figure out support 1 DT node to N
drivers or you're going to be carrying a bunch of your own u-boot
bindings.

> The device tree is how U-Boot determines which random-number generator
> to use for a particular function. For example, for VBE, if we need to
> generate some random numbers we'd like to specify which device creates
> them. It can't just be 'use the TPM if you find one'.

Why not?

> I'm not sure how that works in Linux?

Not sure. I think Linux's answer is use all of the RNG sources and
none of them directly.

Rob
Simon Glass July 15, 2022, 3:38 p.m. UTC | #13
Hi,

On Thu, 14 Jul 2022 at 11:55, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jul 13, 2022 at 9:28 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > [...]
> > > > >
> > > > > > > +
> > > > > > >  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,
> > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > >  };
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > >
> > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > >
> > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > special command to the TPM. So all that we should do here is leverage
> > > > > the fact that the TPM is already in the device tree.
> > > > >
> > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > were deviating from the spec is justified.
> > > >
> > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > get into another discussion about what a device is. We can disagree on
> > > > that if you like.
> > > >
> > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > with of-platdata. But also we can refer to the rand device from
> > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > creation and doesn't really have the concept of a uclass consistently
> > > > applied, but this is U-Boot.
> > >
> > > You are letting client/OS details define your binding. Doing so
> > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > sometimes and DT is not an abstraction layer. The general guidance on
> > > whether there are child nodes for sub-blocks is do they have their own
> > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > Neither of those are the case here.
> > >
> > > Besides, we already have TPM device bindings defined. Requiring
> > > binding changes when adding a new client/OS feature is not good
> > > practice either.
> >
> > I'm not sure what to do with this, but in general the practice of
> > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > feature of the bindings at present, for example GPIOs.
>
> It's perfectly normal for a single device to be multiple providers. A
> common example is clock AND reset blocks. They are both a clock and
> reset provider. So you could have either:
>
> crm {
>   compatible = "foo,soc-crm";
>   clock-controller {
>    compatible = "foo,soc-crm-clocks";
>    #clock-cells = <1>;
>   };
>   reset-controller {
>     compatible = "foo,soc-crm-resets";
>     #reset-cells = <1>;
>   };
> };
>
> or:
>
> crm {
>   compatible = "foo,soc-crm";
>   #clock-cells = <1>;
>   #reset-cells = <1>;
> };
>
> Which one's right? Well, depends on the h/w really. If clock and reset
> controls are interleaved bits or registers, then the first case really
> doesn't make sense.
>
> In any case, this ship has sailed. There are tons of the latter case
> already, so either you have to figure out support 1 DT node to N
> drivers or you're going to be carrying a bunch of your own u-boot
> bindings.

Thanks for the info. I'm really only talking about this TPM issue here.

>
> > The device tree is how U-Boot determines which random-number generator
> > to use for a particular function. For example, for VBE, if we need to
> > generate some random numbers we'd like to specify which device creates
> > them. It can't just be 'use the TPM if you find one'.
>
> Why not?

It isn't deterministic. This is exactly the sort of selection that
device tree is supposed to permit. It would be better to have a
phandle from the board-config node, VBE method node, or something else
that provides the devices to be used.

>
> > I'm not sure how that works in Linux?
>
> Not sure. I think Linux's answer is use all of the RNG sources and
> none of them directly.

OK.

Re Ilias' question about cr50, I don't know.

Re Tom's point about matching on compatible strings, IMO it is cleaner
for the driver to create a child device for the random-number device
if it can provide one, as is done with PMIC when providing GPIOS and
regulators, for example.

Since it seems that the TPM binding does not have this, my preference
would be to add this to the TPM binding.

If that is not acceptable, then we have the TPM_RNG option which we
can disable when we do have such a binding, perhaps only in U-Boot.

I think I have made my point here and people understand it, so I'll
drop my objection to this patch and we can worry about it later if it
causes problems.

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index f67fe1019b..e1f1ef01e1 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -11,10 +11,15 @@ 
 #include <log.h>
 #include <linux/delay.h>
 #include <linux/unaligned/be_byteshift.h>
+#include <tpm_api.h>
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG_DRV_NAME	"tpm-rng"
+
 int tpm_open(struct udevice *dev)
 {
 	struct tpm_ops *ops = tpm_get_ops(dev);
@@ -136,12 +141,36 @@  int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = TPM_RNG_DRV_NAME;
+	struct udevice *child;
+
+	if (CONFIG_IS_ENABLED(TPM_RNG)) {
+		ret = device_find_first_child_by_uclass(dev, UCLASS_RNG,
+							&child);
+
+		if (ret != -ENODEV) {
+			log_debug("RNG child already added to the TPM device\n");
+			return ret;
+		}
+
+		ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+		if (ret)
+			return log_msg_ret("bind", 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,
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };