Hi Peter,
Thanks for your suggestion. Those changes will be included in v2.
Peter Maydell <peter.mayd...@linaro.org> 於 2025年5月6日 週二 下午8:52寫道：
>
> On Fri, 18 Apr 2025 at 10:12, Tim Lee <timlee660...@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 <timlee660...@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
