Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Oshri Alkobi]

On Fri, Aug 16, 2019 at 7:12 PM Alexander Steffen
<Alexander.Steffen@xxxxxxxxxxxx> wrote:
>
> On 15.08.2019 19:03, Oshri Alkobi wrote:
> > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
> > <Alexander.Steffen@xxxxxxxxxxxx> wrote:
> >>
> >> On 18.07.2019 14:51, Eyal.Cohen@xxxxxxxxxxx wrote:
> >>> Hi Jarkko and Alexander,
> >>>
> >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
> >>
> >> Great :) In the meantime, I've done some experiments creating an I2C
> >> driver based on tpm_tis_core, see
> >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
> >> and provide your feedback (and/or use it as a basis for further
> >> implementations).
> >>
> >
> > Sorry for the late response.
> >
> > Thanks Alexander, indeed it looks much simpler.
> > I've checked it with Nuvoton's TPM - basic TPM commands work
>
> Nice :)
>
> > I only
> > had to remove the first msg from the read/write I2C transmitting
> > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
> > a sequence.
> > Actually it is more efficient to set TPM_LOC_SEL only before locality
> > check/request/relinquish - it is sticky.
>
> There is one problem though: Do we assume that only the kernel driver
> will communicate with the TPM or might there be something else that also
> talks to the TPM?
>
> If it is only the kernel driver, we could probably skip setting
> TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use
> anything else. If something else does its own I2C communication with the
> TPM, it might write different values to TPM_LOC_SEL at any time, causing
> the kernel driver to use a different locality than intended. This was
> the reason I always set TPM_LOC_SEL within the same transaction.
>
> > I still didn't manage to work with interrupts, will debug it.
>
> Interrupt support might be broken in general at the moment, see this
> thread: https://www.spinics.net/lists/linux-integrity/msg08663.html
>
> > We weren't aware to the implementation of Christophe/ST which looks
> > good and can be complement to yours.
> > If no one is currently working on that, we can prepare a new patch
> > that is based on both.
> > Please let us know.
>
> I won't have the time to do anything, at least for the next two weeks,
> so feel free to pick it up.
>

Great, we will start working on it.

> >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> >>> At a minimum we thought of:
> >>> 1. Handling TPM Localities in I2C is different
> >>
> >> It turned out not to be that different in the end, see the code
> >> mentioned above and my comment here:
> >> https://patchwork.kernel.org/cover/11049365/
> >>
> >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
> >>
> >> That is completely optional, so there is no need to implement it in the
> >> beginning. Also, do you expect a huge benefit from that functionality?
> >> Are bit flips that much more likely on I2C compared to SPI, which has no
> >> CRC at all, but still works fine?
> >>
> >
> > I2C is noisy bus with potentially more devices with larger variety
> > than SPI. I2C may have more than one master and may have collisions
> > and/or arbitration.
>
> If multi-master usage is a concern, there are probably a lot more places
> in the driver that need to be adapted to deal with concurrent
> access/data corruption. For now, I'd assume a single master, similar to SPI.
>
> Alexander

Oshri