hi Simon, On Mon, 14 Mar 2022 at 23:54, Simon Glass <s...@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 14 Mar 2022 at 05:43, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > hi Simon, > > > > On Mon, 14 Mar 2022 at 03:53, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.g...@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.g...@linaro.org> > > > > --- > > > > > > > > Changes since V4: > > > > > > > > * Put a check for CONFIG_TPM_RNG for binding the RNG device with it's > > > > driver in the post_probe callback instead of putting > > > > CONFIG_{SPL,TPL}_BUILD guards > > > > > > > > drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- > > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > > > > > > > This looks a lot better, please see below. > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c > > > > index f67fe1019b..e1c61d26f0 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,28 @@ 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_bind_driver(dev, drv, "tpm-rng0", &child); > > > > + if (ret) > > > > + return log_msg_ret("bind", ret); > > > > + } > > > > > > 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'. > > 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. > > 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.
Well, FWIW I actually found usage of this kind of device binding in this very project. There are quite a few drivers which are using the API in the same way that is being done in this patch. And I have already mentioned the reason that I am using this method as against a device tree. Thanks. -sughosh > > > > > 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