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

[Stefan Berger]

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