Re: [PATCH 2/2] qtest: Add a test case for TPM TIS I2C connected to Aspeed I2C controller

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

On 3/27/23 03:49, Cédric Le Goater wrote:
> On 3/27/23 02:37, Stefan Berger wrote:
>> Add a test case for the TPM TIS I2C device exercising most of its
>> functionality, including localities.
>>
>> Add library functions for being able to read from and write to registers
>> of the TPM TIS I2C device connected to the Aspeed i2c controller.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Thanks for doing the I2C qtest driver. This gives the opportunity to write
> more unit tests.
> 
>> ---
>>   tests/qtest/meson.build        |   3 +
>>   tests/qtest/qtest_aspeed.c     | 117 ++++++
>>   tests/qtest/qtest_aspeed.h     |  27 ++
>>   tests/qtest/tpm-tis-i2c-test.c | 628 +++++++++++++++++++++++++++++++++
>>   4 files changed, 775 insertions(+)
>>   create mode 100644 tests/qtest/qtest_aspeed.c
>>   create mode 100644 tests/qtest/qtest_aspeed.h
>>   create mode 100644 tests/qtest/tpm-tis-i2c-test.c
>>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 29a4efb4c2..065a00d34d 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -200,6 +200,7 @@ qtests_arm = \
>>     (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed : []) + \
>>     (config_all_devices.has_key('CONFIG_NPCM7XX') ? qtests_npcm7xx : []) + \
>>     (config_all_devices.has_key('CONFIG_GENERIC_LOADER') ? ['hexloader-test'] : []) + \
>> +  (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
>>     ['arm-cpu-features',
>>      'microbit-test',
>>      'test-arm-mptimer',
>> @@ -212,6 +213,7 @@ qtests_aarch64 = \
>>       ['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +                                         \
>>     (config_all_devices.has_key('CONFIG_XLNX_ZYNQMP_ARM') ? ['xlnx-can-test', 'fuzz-xlnx-dp-test'] : []) + \
>>     (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test'] : []) +  \
>> +  (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
>>     ['arm-cpu-features',
>>      'numa-test',
>>      'boot-serial-test',
>> @@ -303,6 +305,7 @@ qtests = {
>>     'tpm-crb-test': [io, tpmemu_files],
>>     'tpm-tis-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>>     'tpm-tis-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>> +  'tpm-tis-i2c-test': [io, tpmemu_files, 'qtest_aspeed.c'],
>>     'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>>     'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>>     'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
>> diff --git a/tests/qtest/qtest_aspeed.c b/tests/qtest/qtest_aspeed.c
>> new file mode 100644
>> index 0000000000..2b316178e4
>> --- /dev/null
>> +++ b/tests/qtest/qtest_aspeed.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Aspeed i2c bus interface to reading and writing to i2c device registers
>> + *
>> + * Copyright (c) 2023 IBM Corporation
>> + *
>> + * Authors:
>> + *   Stefan Berger <stefanb@linux.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "qtest_aspeed.h"
>> +
>> +#include "hw/i2c/aspeed_i2c.h"
>> +#include "libqtest-single.h"
>> +
>> +#define A_I2CD_M_STOP_CMD       BIT(5)
>> +#define A_I2CD_M_RX_CMD         BIT(3)
>> +#define A_I2CD_M_TX_CMD         BIT(1)
>> +#define A_I2CD_M_START_CMD      BIT(0)
>> +
>> +#define A_I2CD_MASTER_EN        BIT(0)
> 
> Why do you need to include the aspeed_i2c.h file and add some more
> definitions ? Couldn't we gather all of them under the same file ?

I moved them now.



>> +
>> +#define I2C_SLAVE_ADDR   0x2e
>> +#define I2C_DEV_BUS_NUM  10
>> +
>> +static const uint8_t TPM_CMD[12] =
>> +    "\x80\x01\x00\x00\x00\x0c\x00\x00\x01\x44\x00\x00";
>> +
>> +uint32_t aspeed_dev_addr = 0X1e78a000 + 0x80 + I2C_DEV_BUS_NUM * 0x80;
> 
> 0X1e78a000 could be a define

Is it suitable for a public header file or limited to the board we are using it with?
Where should the define go? Into the qtest_aspeed.h file under this name?

#define AST2600_ASPEED_I2C_BASE_ADDR 0x1e78a0000

> > The resulting address should be calculated with an helper defined in
> qtest_aspeed.h, with an ast2600_ prefix in the name since the calculation
> is SoC dependent.  See aspeed_i2c_realize()

static inline uint32_t ast2600_aspeed_i2c_calc_dev_addr(uint8_t bus_num)
{
     return AST2600_ASPEED_I2C_BASE_ADDR + 0x80 + bus_num * 0x80;
}
Like this?

> 
> My knowledge on TPM is too limited to comment. Could you please extract
> the I2C driver in its own patch ?

Will do.

    Stefan