Hi Ilias, On Fri, 2 Jul 2021 at 14:29, Ilias Apalodimas <[email protected]> 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
