[PATCH 1/1] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Ilias,

On Fri, 2 Jul 2021 at 14:29, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Fri, Jul 02, 2021 at 07:54:13PM +0200, Heinrich Schuchardt wrote:
> > On 7/2/21 2:52 PM, Ilias Apalodimas wrote:
> > > Add a TPMv2 TIS MMIO compatible driver.
> > > This useful for a couple of reasons.  First of all we can support a TPMv2
> > > on devices that have this hardware (e.g the newly added SynQuacer box).
> > > We can also use the driver in our QEMU setups and test the EFI TCG2
> > > protocol, which cannot be tested with our sandbox.
> > >
> > > Ideally we should create an abstraction for all TIS compatible TPMs we
> > > support. Let's plug in the mmio driver first.
> >
> > This would match Linux' linux/drivers/char/tpm/tpm_tis_core.c. The
> > individual drivers provide the TIS level operations of struct
> > tpm_tis_phy_ops and call tpm_tis_core_init() to load the tpm_tis_core
> > module which supplies the TPM level operations of struct tpm_class_ops.
> >
> > Why don't you start with a separate tis_core module supporting the
> > tis_mmio driver? This would avoid pulling out the core functions out of
> > this driver and provide a template for refactoring the other drivers.
> >
>
> Yep, I am commenting more on this below.
> [...]
>
> > > +/*
> > > + * swtpm driver for TCG/TIS TPM (trusted platform module).
> >
> > swtpm is not easy to understand. I would prefer something like
> >
> > 'Driver for the TPM software emulation provided by the swtpm package
> > (https://github.com/stefanberger/swtpm)'.
>
> Sure
>
> >
> > > + * Specifications at www.trustedcomputinggroup.org
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <log.h>
> > > +#include <tpm-v2.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/compiler.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/types.h>
> > > +#include <linux/io.h>
> > > +#include <linux/unaligned/be_byteshift.h>
> > > +#include "tpm_tis.h"
> > > +#include "tpm_internal.h"
> > > +
> > > +enum tis_int_flags {
> > > +   TPM_GLOBAL_INT_ENABLE = 0x80000000,
> > > +   TPM_INTF_BURST_COUNT_STATIC = 0x100,
> > > +   TPM_INTF_CMD_READY_INT = 0x080,
> > > +   TPM_INTF_INT_EDGE_FALLING = 0x040,
> > > +   TPM_INTF_INT_EDGE_RISING = 0x020,
> > > +   TPM_INTF_INT_LEVEL_LOW = 0x010,
> > > +   TPM_INTF_INT_LEVEL_HIGH = 0x008,
> > > +   TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> > > +   TPM_INTF_STS_VALID_INT = 0x002,
> > > +   TPM_INTF_DATA_AVAIL_INT = 0x001,
> > > +};
> >
> > These definitions match Linux' tpm_tis_core.h.
> >
> > Should they be moved to an include of the same name in U-Boot?
>
> I got a tpm_tis.h in this series, so I might as well move those there
>
> >
>
> [...]
>
> > I would prefer if you would add these functions to a structure struct
> > tpm_tis_phy_ops which you can use to pull out the core driver.
>
> Me too :).  In fact that matches the design I proposed over IRC.  I just
> didn't have too much free time to fix it, so I went ahead and posted the
> driver matching what's already in U-Boot.  That being said, what's proposed
> here is the right was forward.  Basically if we do it like this then any
> future TPM TIS driver will only have to define the read/write ops for the
> underlyng bus.

I'm not sure about the irc side, but it is best to do things on the
mailing list so people can see it.

There seems to be quite a bit of duplicated code in this new driver. I
think it would be better to come up with the interface first, as
Heinrich suggests. Would something like regmap be good enough?

Regards,
Simon