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

[Simon Glass <sjg@chromium.org>]

Hi Ilias,

On Tue, 13 Jul 2021 at 23:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Tue, Jul 13, 2021 at 08:49:21PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > >
> > > [...]
> > > > > > Should be a uclass interface.
> > > > > >
> > > > >
> > > > > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > > > > have a uclass for those.
> > > >
> > > > Who told you that a uclass is supposed to describe and abstract hardware? :-)
> > > >
> > >
> > > That's what I've mostly seen it used for, maybe i got the idea wrong.
> >
> > A uclass is basically a software construct. It is an interface between
> > clients and the driver, typically. Quite often the uclass is an
> > interface on top of the hardware (actually the driver). But quite
> > often it is not. For example, we use an GPIO uclass to access a pmic's
> > GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere
> > where it makes sense to have an abstraction, we use a uclass.
> >
> > >
> > > > The uclass is how driver model does APIs, so normally a uclass would
> > > > be used for any API. There are exceptions, but this one actually looks
> > > > like a useful interface we should have.
> > > >
> > >
> > > the point is we already have a uclass for tpm devices.  So why should the
> > > we add another one that just describes the TIS interface?
> >
> > You have already added another API, right? All we are discussing is
> > whether it should be a uclass or not. Unless there is a very good
> > reason, we should avoid creating custom interfaces that don't use
> > driver model. I actually think the interface you've created (MMIO)
> > will be very useful as a uclass.
> >
>
> So you are basically looking into adding something similar to
> dm_i2c_read/dm_i2c_write etc?
> I assume this is gong to be the default read method passed on the TIS API
> when we want to support i2c TPMs.

Well I'm not quite sure, but if MMIO can sit on top of I2C, then yes.

> For the MMIO case that would essentially mean, move the functions on a
> different file, add them on a header and define a UCLASS_DRIVER with only
> the .id and .name defined?

Yes. Something like:

- add a new uclass ID to dm/uciass-id.h
- add include/mmio.h (or whatever it is called) with operations (see
pch.h as an example)
- add drivers/misc/mmio-uclass.c with the UCLASS_DRIVER and stubs for the API
- add drivers/misc/sandbox_mmio.c with a sandbox driver that does very
little (perhaps just read canned data?)
- add test/dm/mmio.c sandbox test that calls each API function and
checks the result

As I said pch is a good example since it is fairly simple.

Regards,
Simon