Hi Sughosh, On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > hi Simon, > > On Fri, 4 Mar 2022 at 08:07, Simon Glass <s...@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <s...@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.g...@linaro.org> > > > > wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None > > > > > > > > > > > > > > drivers/tpm/tpm-uclass.c | 58 > > > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c > > > > > > > index 8619da89d8..383cc7bc48 100644 > > > > > > > --- a/drivers/tpm/tpm-uclass.c > > > > > > > +++ b/drivers/tpm/tpm-uclass.c > > > > > > > @@ -16,6 +16,11 @@ > > > > > > > #include <tpm-v2.h> > > > > > > > #include "tpm_internal.h" > > > > > > > > > > > > > > +#include <dm/lists.h> > > > > > > > + > > > > > > > +#define TPM_RNG1_DRV_NAME "tpm1-rng" > > > > > > > +#define TPM_RNG2_DRV_NAME "tpm2-rng" > > > > > > > + > > > > > > > bool is_tpm1(struct udevice *dev) > > > > > > > { > > > > > > > return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) > > > > > > > == TPM_V1; > > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const > > > > > > > uint8_t *sendbuf, size_t send_size, > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_TPM) > > > > > > > > > > > > This should be in the Makefile so that we only build this file if > > > > > > TPM > > > > > > is enabled. > > > > > > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and > > > > > TPL stages as well. The addition of the RNG device is to be done only > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot > > > > > proper. Thanks. > > > > > > > > Well in that case, create a new SPL_TPM_RAND or similar to control > > > > enabling it in SPL. It should be explicit. > > > > > > I think it is easier to just protect the child addition functions > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We > > > don't have any requirement for generating random numbers in the SPL > > > and TPL stages. I feel that creating new symbols just for the sake of > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially > > > since we do not have any requirement for RNG devices in the SPL/TPL > > > stages. > > > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that > > option is enabled, the feature will be active in SPL and TPL too. > > Maybe I am not explaining it properly. We need the addition of the RNG > child device only in the u-boot proper stage, not in the SPL and TPL > stages. The TPM uclass driver can indeed be built for the SPL and TPL > stages, while the RNG uclass is needed only for u-boot proper. So, the > addition of the RNG child device done in the TPM uclass driver should > only happen in u-boot proper, and not in SPL/TPL stages. Which is the > reason for the CONFIG_TPM check.
Let me try one more time. If this doesn't work, please chat with a colleague. The way we do that is with Kconfig options. We don't use ad-hoc methods to enable things in U-Boot proper but not SPL. Boards that have CONFIG_TPM set have it set everywhere, including in SPL. So checking for CONFIG_TPM is going to return true in both U-Boot proper and SPL. If you want different behaviour in SPL, you need CONFIG_SPL_TPM. But even that is indirect, as I have explained. To enable your driver in SPL, you need a CONFIG_SPL_... Also, you should use devicetree to provide the random number generator, just a device_bind(). Add the node as a subnode of the TPM. Otherwise it cannot work with OF_PLATDATA. > > > > > Also I see another problem, on further examination. You cannot start > > up the TPM in the pre_probe() function. That needs to be done under > > board control. E.g. for coral it happens in the TPL (or soon VPL) > > phase so cannot be done again in U-Boot proper. > > I tested running the RNG command after the TPM device has already been > probed and tpm_startup has been called. Even if I call the tpm_startup > again, I do not see any issues. Does the TPM spec prohibit calling the > initialisation function multiple times. I believe that the TPM device > should be able to handle this scenario right? I hangs on coral, for example. > > > > > So perhaps we need to remember the state of the TPM (using SPL handoff > > perhaps). Also you probably need to move the startup stuff to the RNG > > itself. > > I can move the call to tpm_startup to the RNG driver's probe function. > But I thought it is better that the parent device(TPM) is initialised > before calling the probe of the child device. We use lazy init in U-Boot, so it should be possible to probe the TPM without automatically starting it. Otherwise you are going to break some of the tpm commands. > > > > > Perhaps we could add a new function to handle this, which can be > > called from your rand driver. > > > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode) > > Okay, I can add this, but the question is, does calling the > tpm_startup function cause issues? If not, maybe this is not needed? It does on coral. I don't know whether other TPMs are tolerant of it, but in any case it is not a good idea. Regards, Simon