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.
> 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).
> +
> +#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 ?
> +/* 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.
> +
> + /* 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) ?
> +
> + qtest_quit(qts);
> +}
thanks
-- PMM
