Hi Simon, Simon Glass <[email protected]> wrote on Wed, 18 Jul 2018 19:31:41 -0600:
> Hi Miquel, > > On 14 July 2018 at 06:16, Miquel Raynal <[email protected]> wrote: > > While there is probably no reason to do so in a real life situation, it > > will allow to compile test both stacks with the same sandbox defconfig. > > > > As we cannot define two 'tpm' commands at the same time, the command for > > TPM v1 is still called 'tpm' and the one for TPM v2 'tpm2'. While this > > is the exact command name that must be written into eg. test files, any > > user already using the TPM v2 stack can continue using just 'tpm' as > > command as long as TPM v1 support is not compiled (because U-Boot prompt > > will search for the closest command named after 'tpm'.b) > > > > The command set can also be changed at runtime (not supported yet, but > > ready to be), but as one can compile only either one stack or the other, > > there is still one spot in the code where conditionals are used: to > > retrieve the v1 or v2 command set. > > > > Signed-off-by: Miquel Raynal <[email protected]> > > --- > > cmd/tpm-common.c | 24 +++++++++++++++++++++++- > > cmd/tpm-v1.c | 2 +- > > cmd/tpm-v2.c | 4 ++-- > > drivers/tpm/Kconfig | 7 ++----- > > drivers/tpm/tpm-uclass.c | 13 ++++++++++--- > > drivers/tpm/tpm2_tis_sandbox.c | 11 +++++++++++ > > drivers/tpm/tpm2_tis_spi.c | 15 +++++++++++++++ > > include/tpm-common.h | 36 ++++++++++++++++++++++++++++++------ > > lib/tpm-common.c | 4 ++++ > > lib/tpm-utils.h | 9 +++++++++ > > 10 files changed, 107 insertions(+), 18 deletions(-) > > > > Reviewed-by: Simon Glass <[email protected]> > > Some comments below. > > > diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c > > index 6cf9fcc9ac..69cc02b599 100644 > > --- a/cmd/tpm-common.c > > +++ b/cmd/tpm-common.c > > @@ -273,12 +273,34 @@ int do_tpm_init(cmd_tbl_t *cmdtp, int flag, int argc, > > char * const argv[]) > > int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > { > > cmd_tbl_t *tpm_commands, *cmd; > > + struct tpm_chip_priv *priv; > > + struct udevice *dev; > > unsigned int size; > > + int rc; > > Can we use 'ret'? That is more common. Of course. > > > > > if (argc < 2) > > return CMD_RET_USAGE; > > > > - tpm_commands = get_tpm_commands(&size); > > + rc = get_tpm(&dev); > > + if (rc) > > + return rc; > > + > > + priv = dev_get_uclass_priv(dev); > > + > > + switch (priv->version) { > > +#if defined(CONFIG_TPM_V1) > > + case TPM_V1: > > if (IS_ENABLED(CONFIG_TPM_V1)) > > (I think #ifdef is worse) Actually, in the present state, get_tpmx_commands symbol is only available if the file if TPM_Vx is selected in Kconfig. I created two dummy functions in tpm-common.h which return NULL for each symbol if their respective TPM version is not compiled. This way it removes any conditionals in the code. > > > + tpm_commands = get_tpm1_commands(&size); > > + break; > > +#endif > > +#if defined(CONFIG_TPM_V2) > > + case TPM_V2: > > + tpm_commands = get_tpm2_commands(&size); > > + break; > > +#endif > > + default: > > + return CMD_RET_USAGE; > > + } [...] > > > > +static int sandbox_tpm2_set_version(struct udevice *dev) > > +{ > > + struct tpm_chip_priv *priv = dev_get_uclass_priv(dev); > > + > > + /* Use the TPM v2 stack */ > > + priv->version = TPM_V2; > > Don't you think this could be done in the probe() method? When I first wrote this I faced some issues. As I did not remembered why it would fail I tried again and indeed, moving this selection in the probe works. > > > + > > + return 0; > > +} > > + [...] > > @@ -65,6 +78,16 @@ struct tpm_chip_priv { > > * to bytes which are sent and received. > > */ > > struct tpm_ops { > > + /** > > + * set_version() - Set the right TPM stack version to use > > + * > > + * Default is TPM_V1. TPM of newer versions can implement this > > + * optional hook to set another value. > > + * > > + * @dev: TPM device > > + */ > > + int (*set_version)(struct udevice *dev); > > I think this could just be a helper function provided by the uclass, > rather than a device method. Actually there is no need for this hook or any helper anymore, I removed them all as the version selection is done in the driver ->probe(). I'll let other people that would need to change the version at runtime adding support for that if they want, it should not be too tricky anyway. Thanks, Miquèl _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
