Re: [PATCH] tests/qtest: Add qtest for NPCM8XX PSPI module

[Tim Lee]

Hi Peter,
Thanks for your suggestion. Those changes will be included in v2.

Peter Maydell <peter.maydell@linaro.org> 於 2025年5月6日 週二 下午8:52寫道：
>
> On Fri, 18 Apr 2025 at 10:12, Tim Lee <timlee660101@gmail.com> wrote:
> >
> > - Created qtest to check initialization of registers in PSPI Module
> > - Implemented test into Build File
> >
> > Tested:
> > ./build/tests/qtest/npcm8xx-pspi_test
> >
> > Signed-off-by: Tim Lee <timlee660101@gmail.com>
> > ---
> >  MAINTAINERS                     |   1 +
> >  tests/qtest/meson.build         |   3 +
> >  tests/qtest/npcm8xx_pspi-test.c | 104 ++++++++++++++++++++++++++++++++
> >  3 files changed, 108 insertions(+)
> >  create mode 100644 tests/qtest/npcm8xx_pspi-test.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d54b5578f8..0162f59bf7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -892,6 +892,7 @@ F: hw/sensor/adm1266.c
> >  F: include/hw/*/npcm*
> >  F: tests/qtest/npcm*
> >  F: tests/qtest/adm1266-test.c
> > +F: tests/qtest/npcm8xx_pspi-test.c
>
> This file is already matched as being in this section by the
> wildcard two lines earlier.

MAINTAINERS file keep no change.
>
> >  F: pc-bios/npcm7xx_bootrom.bin
> >  F: pc-bios/npcm8xx_bootrom.bin
> >  F: roms/vbootrom
>
> > diff --git a/tests/qtest/npcm8xx_pspi-test.c 
> > b/tests/qtest/npcm8xx_pspi-test.c
> > new file mode 100644
> > index 0000000000..107bce681f
> > --- /dev/null
> > +++ b/tests/qtest/npcm8xx_pspi-test.c
> > @@ -0,0 +1,104 @@
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qemu/module.h"
>
> Every source file needs to start with the usual brief
> comment giving its copyright/license information (and we
> like that to include an SPDX-license-Identifier these days
> for new source files).
>

Add comment for copyright/license information.
> > +
> > +#define DATA_OFFSET 0x00
> > +#define CTL_SPIEN   0x01
> > +#define CTL_OFFSET  0x02
> > +#define CTL_MOD     0x04
> > +
> > +typedef struct PSPI {
> > +    uint64_t base_addr;
> > +} PSPI;
> > +
> > +PSPI pspi_defs = {
> > +    .base_addr  = 0xf0201000
> > +};
> > +
> > +static uint16_t pspi_read_data(QTestState *qts, const PSPI *pspi)
> > +{
> > +    return qtest_readw(qts, pspi->base_addr + DATA_OFFSET);
> > +}
> > +
> > +static void pspi_write_data(QTestState *qts, const PSPI *pspi, uint16_t 
> > value)
> > +{
> > +    qtest_writew(qts, pspi->base_addr + DATA_OFFSET, value);
> > +}
> > +
> > +static uint32_t pspi_read_ctl(QTestState *qts, const PSPI *pspi)
> > +{
> > +    return qtest_readl(qts, pspi->base_addr + CTL_OFFSET);
> > +}
> > +
> > +static void pspi_write_ctl(QTestState *qts, const PSPI *pspi, uint32_t 
> > value)
> > +{
> > +    qtest_writel(qts, pspi->base_addr + CTL_OFFSET, value);
> > +}
>
> If I'm reading the implementation correctly, it makes both
> the DATA and CTL registers 16 bits, but this code has the
> data register 16 bits and the control register 32 bits.
> Which is correct ?
>

Yes, you are right! DATA and CLT registers both use 16 bits.
> > +/* Check PSPI can be reset to default value */
> > +static void test_init(gconstpointer pspi_p)
> > +{
> > +    const PSPI *pspi = pspi_p;
> > +
> > +    QTestState *qts = qtest_init("-machine npcm845-evb");
> > +    pspi_write_ctl(qts, pspi, CTL_SPIEN);
> > +    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, CTL_SPIEN);
> > +
> > +    qtest_quit(qts);
> > +}
> > +
> > +/* Check PSPI can be r/w data register */
> > +static void test_data(gconstpointer pspi_p)
> > +{
> > +    const PSPI *pspi = pspi_p;
> > +    uint16_t test = 0x1234;
> > +    uint16_t output;
> > +
> > +    QTestState *qts = qtest_init("-machine npcm845-evb");
> > +
> > +    /* Write to data register */
> > +    pspi_write_data(qts, pspi, test);
> > +    printf("Wrote 0x%x to data register\n", test);
>
> Don't put printf()s in test cases, please. The test
> output is supposed to be TAP test protocol format, and
> the printfs insert random junk into that.
>
> If you need to output some kind of message, you can use
> g_test_message(), but for simple stuff like this I don't think
> the printfs are really adding anything, because the test is
> so short.
>

Remove printf() in test cases.
> > +
> > +    /* Read from data register */
> > +    output = pspi_read_data(qts, pspi);
> > +    printf("Read 0x%x from data register\n", output);
>
> Can we assert something useful here about what we read
> (e.g. that it's the same as what we wrote) ?
>

Currently, I just write test data to data register then read data from
it and verify it.
> > +
> > +    qtest_quit(qts);
> > +}
>
> thanks
> -- PMM



-- 
Best regards,
Tim Lee