Re: [PATCH 3/3] Add support for TPM devices over I2C bus

[Stefan Berger]

On 3/22/23 07:28, Ninad Palsule wrote:
> On 3/21/23 8:30 PM, Stefan Berger wrote:
> > On 3/21/23 01:30, Ninad Palsule wrote:
> > > Qemu already supports devices attached to ISA and sysbus. This drop adds
> > > support for the I2C bus attached TPM devices. I2C model only supports
> > > TPM2 protocol.
> >
> > > +
> > > +/* Send data to TPM */
> > > +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
> > > +{
> > > +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
> > > +        uint16_t tis_reg;
> > > +        uint32_t data;
> > > +        int      i;
> > > +
> > > +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
> > > +
> > > +        /* Index 0 is always a register */
> > > +        for (i = 1; i < i2cst->offset; i++) {
> > > +            data = (i2cst->data[i] & 0xff);
> > > +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
> > > +        }
> >
> >
> > I think there should be tpm_tis_set_data_buffer function that you can call 
> > rather than transferring the data byte-by-byte.
> >
> > Thanks for the series!
> >
> >   Stefan
>
> I thought about it but the FIFO case performs multiple operations hence I did 
> not want to change it. Currently there is no function to set data buffer in the 
> common code.

It may not be correct to transfer it in one go, either. I just printed the I2C 
specs and I am going to look at them now.
When one writes TPM command data to the TIS the STS register has its 
TPM_TIS_STS_VALID bit set and TPM_TIS_STS_EXPECT bit reset once the command is 
complete. This would imply that you should not have a holding area for the 
command bytes but pass them on to the TIS immediately to get the effect of the 
STS register...

   Stefan


> Thanks for the review!
>
> Ninad Palsule