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

[Stefan Berger <stefanb@linux.ibm.com>]

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
>