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

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

[Tim Lee]

- 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
 F: pc-bios/npcm7xx_bootrom.bin
 F: pc-bios/npcm8xx_bootrom.bin
 F: roms/vbootrom
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 3136d15e0f..88672a8b00 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -210,6 +210,8 @@ qtests_npcm7xx = \
    'npcm7xx_watchdog_timer-test',
    'npcm_gmac-test'] + \
    (slirp.found() ? ['npcm7xx_emc-test'] : [])
+qtests_npcm8xx = \
+  ['npcm8xx_pspi-test']
 qtests_aspeed = \
   ['aspeed_hace-test',
    'aspeed_smc-test',
@@ -257,6 +259,7 @@ qtests_aarch64 = \
   (config_all_accel.has_key('CONFIG_TCG') and                                            \
    config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
   (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
+  (config_all_devices.has_key('CONFIG_NPCM8XX') ? qtests_npcm8xx : []) + \
   ['arm-cpu-features',
    'numa-test',
    'boot-serial-test',
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"
+
+#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);
+}
+
+/* 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);
+
+    /* Read from data register */
+    output = pspi_read_data(qts, pspi);
+    printf("Read 0x%x from data register\n", output);
+
+    qtest_quit(qts);
+}
+
+/* Check PSPI can be r/w control register */
+static void test_ctl(gconstpointer pspi_p)
+{
+    const PSPI *pspi = pspi_p;
+    uint8_t control = CTL_MOD;
+
+    QTestState *qts = qtest_init("-machine npcm845-evb");
+
+    /* Write CTL_MOD value to control register for 16-bit interface mode */
+    qtest_memwrite(qts, pspi->base_addr + CTL_OFFSET,
+                   &control, sizeof(control));
+    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, control);
+    printf("Wrote CTL_MOD to control register\n");
+
+    qtest_quit(qts);
+}
+
+static void pspi_add_test(const char *name, const PSPI* wd,
+        GTestDataFunc fn)
+{
+    g_autofree char *full_name = g_strdup_printf("npcm8xx_pspi/%s",  name);
+    qtest_add_data_func(full_name, wd, fn);
+}
+
+#define add_test(name, td) pspi_add_test(#name, td, test_##name)
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    add_test(init, &pspi_defs);
+    add_test(ctl, &pspi_defs);
+    add_test(data, &pspi_defs);
+    return g_test_run();
+}
-- 
2.34.1

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

[Peter Maydell]

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

Hi -- I'm picking this patch as a random npcm one
to suggest that people in the Nuvoton MAINTAINERS file
should be doing code review on patches to the boards.
I can collect them up to go in to the tree, and I will do
review-of-last-resort, but these boards should not be "we
send the code upstream but we otherwise don't have any
interaction or do anything", please...

If people with an interest in the Nuvoton boards don't
collectively review each others' patches, this is likely
to mean that fixes get upstream rather slower than they
would otherwise do.

Other relevant recent patches:

https://patchew.org/QEMU/20250418091208.1888768-1-timlee660101@gmail.com/
https://patchew.org/QEMU/20250414020629.1867106-1-timlee660101@gmail.com/
https://patchew.org/QEMU/20250315142050.3642741-1-linux@roeck-us.net/
https://patchew.org/QEMU/20250401085903.224787-1-timlee660101@gmail.com/

thanks
-- PMM

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

[KFTING]

From: Tim Lee <timlee660101@gmail.com>
Sent: Friday, April 18, 2025 5:12 PM
To: farosas@suse.de; lvivier@redhat.com; pbonzini@redhat.com; wuhaotsh@google.com; CS20 KFTing <KFTING@nuvoton.com>; CS20 CHLi30 <CHLI30@nuvoton.com>
Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Tim Lee <timlee660101@gmail.com>
Subject: [PATCH] tests/qtest: Add qtest for NPCM8XX PSPI module


- 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
 F: pc-bios/npcm7xx_bootrom.bin
 F: pc-bios/npcm8xx_bootrom.bin
 F: roms/vbootrom
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 3136d15e0f..88672a8b00 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -210,6 +210,8 @@ qtests_npcm7xx = \
    'npcm7xx_watchdog_timer-test',
    'npcm_gmac-test'] + \
    (slirp.found() ? ['npcm7xx_emc-test'] : [])
+qtests_npcm8xx = \
+  ['npcm8xx_pspi-test']
 qtests_aspeed = \
   ['aspeed_hace-test',
    'aspeed_smc-test',
@@ -257,6 +259,7 @@ qtests_aarch64 = \
   (config_all_accel.has_key('CONFIG_TCG') and                                            \
    config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
   (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
+  (config_all_devices.has_key('CONFIG_NPCM8XX') ? qtests_npcm8xx : [])
+ + \
   ['arm-cpu-features',
    'numa-test',
    'boot-serial-test',
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"
+
+#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); }
+
+/* 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);
+
+    /* Read from data register */
+    output = pspi_read_data(qts, pspi);
+    printf("Read 0x%x from data register\n", output);
+
+    qtest_quit(qts);
+}
+
+/* Check PSPI can be r/w control register */ static void
+test_ctl(gconstpointer pspi_p) {
+    const PSPI *pspi = pspi_p;
+    uint8_t control = CTL_MOD;
+
+    QTestState *qts = qtest_init("-machine npcm845-evb");
+
+    /* Write CTL_MOD value to control register for 16-bit interface mode */
+    qtest_memwrite(qts, pspi->base_addr + CTL_OFFSET,
+                   &control, sizeof(control));
+    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, control);
+    printf("Wrote CTL_MOD to control register\n");
+
+    qtest_quit(qts);
+}
+
+static void pspi_add_test(const char *name, const PSPI* wd,
+        GTestDataFunc fn)
+{
+    g_autofree char *full_name = g_strdup_printf("npcm8xx_pspi/%s",  name);
+    qtest_add_data_func(full_name, wd, fn); }
+
+#define add_test(name, td) pspi_add_test(#name, td, test_##name)
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    add_test(init, &pspi_defs);
+    add_test(ctl, &pspi_defs);
+    add_test(data, &pspi_defs);
+    return g_test_run();
+}
--
2.34.1

Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.

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

[Fabiano Rosas]

Tim Lee <timlee660101@gmail.com> writes:

> - 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
>  F: pc-bios/npcm7xx_bootrom.bin
>  F: pc-bios/npcm8xx_bootrom.bin
>  F: roms/vbootrom
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 3136d15e0f..88672a8b00 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -210,6 +210,8 @@ qtests_npcm7xx = \
>     'npcm7xx_watchdog_timer-test',
>     'npcm_gmac-test'] + \
>     (slirp.found() ? ['npcm7xx_emc-test'] : [])
> +qtests_npcm8xx = \
> +  ['npcm8xx_pspi-test']
>  qtests_aspeed = \
>    ['aspeed_hace-test',
>     'aspeed_smc-test',
> @@ -257,6 +259,7 @@ qtests_aarch64 = \
>    (config_all_accel.has_key('CONFIG_TCG') and                                            \
>     config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
>    (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
> +  (config_all_devices.has_key('CONFIG_NPCM8XX') ? qtests_npcm8xx : []) + \
>    ['arm-cpu-features',
>     'numa-test',
>     'boot-serial-test',
> 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"
> +
> +#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);
> +}
> +
> +/* 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);
> +
> +    /* Read from data register */
> +    output = pspi_read_data(qts, pspi);
> +    printf("Read 0x%x from data register\n", output);
> +
> +    qtest_quit(qts);
> +}
> +
> +/* Check PSPI can be r/w control register */
> +static void test_ctl(gconstpointer pspi_p)
> +{
> +    const PSPI *pspi = pspi_p;
> +    uint8_t control = CTL_MOD;
> +
> +    QTestState *qts = qtest_init("-machine npcm845-evb");
> +
> +    /* Write CTL_MOD value to control register for 16-bit interface mode */
> +    qtest_memwrite(qts, pspi->base_addr + CTL_OFFSET,
> +                   &control, sizeof(control));
> +    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, control);
> +    printf("Wrote CTL_MOD to control register\n");
> +
> +    qtest_quit(qts);
> +}
> +
> +static void pspi_add_test(const char *name, const PSPI* wd,
> +        GTestDataFunc fn)
> +{
> +    g_autofree char *full_name = g_strdup_printf("npcm8xx_pspi/%s",  name);
> +    qtest_add_data_func(full_name, wd, fn);
> +}
> +
> +#define add_test(name, td) pspi_add_test(#name, td, test_##name)
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    add_test(init, &pspi_defs);
> +    add_test(ctl, &pspi_defs);
> +    add_test(data, &pspi_defs);
> +    return g_test_run();
> +}

This fails on top of current master, please take a look:

$ QTEST_LOG=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/npcm8xx_pspi-test
# random seed: R02S03f79fc48ba73b76c881f93f90b015e9
1..3
# Start of aarch64 tests
# Start of npcm8xx_pspi tests
# starting QEMU: exec ./qemu-system-aarch64 -qtest
  unix:/tmp/qtest-32530.sock -qtest-log /dev/fd/2 -chardev
  socket,path=/tmp/qtest-32530.qmp,id=char0 -mon
  chardev=char0,mode=control -display none -audio none -machine
  npcm845-evb -accel qtest
[I 0.000000] OPENED
[R +0.034918] endianness
[S +0.034944] OK little
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 10},
"package": "v10.0.0-530-g88d6459dae"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities"}
{"return": {}}
[R +0.037373] writel 0xf0201002 0x1
[S +0.037396] OK
[R +0.037417] readl 0xf0201002
[S +0.037426] OK 0x0000000000000000
**
ERROR:../tests/qtest/npcm8xx_pspi-test.c:45:test_init: assertion failed
(pspi_read_ctl(qts, pspi) == CTL_SPIEN): (0x00000000 == 0x00000001)
Bail out!
[I +0.037909] CLOSED
Aborted (core dumped)

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

[Tim Lee]

> This fails on top of current master, please take a look:
>
> $ QTEST_LOG=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/npcm8xx_pspi-test
> # random seed: R02S03f79fc48ba73b76c881f93f90b015e9
> 1..3
> # Start of aarch64 tests
> # Start of npcm8xx_pspi tests
> # starting QEMU: exec ./qemu-system-aarch64 -qtest
>   unix:/tmp/qtest-32530.sock -qtest-log /dev/fd/2 -chardev
>   socket,path=/tmp/qtest-32530.qmp,id=char0 -mon
>   chardev=char0,mode=control -display none -audio none -machine
>   npcm845-evb -accel qtest
> [I 0.000000] OPENED
> [R +0.034918] endianness
> [S +0.034944] OK little
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 10},
> "package": "v10.0.0-530-g88d6459dae"}, "capabilities": ["oob"]}}
> {"execute": "qmp_capabilities"}
> {"return": {}}
> [R +0.037373] writel 0xf0201002 0x1
> [S +0.037396] OK
> [R +0.037417] readl 0xf0201002
> [S +0.037426] OK 0x0000000000000000
> **
> ERROR:../tests/qtest/npcm8xx_pspi-test.c:45:test_init: assertion failed
> (pspi_read_ctl(qts, pspi) == CTL_SPIEN): (0x00000000 == 0x00000001)
> Bail out!
> [I +0.037909] CLOSED
> Aborted (core dumped)

Thank you for testing it. I think the failure seems to be related to
the following commit which, has not been merged yet.
https://patchew.org/QEMU/20250414020629.1867106-1-timlee660101@gmail.com/
Here is our test result for reference. Thanks.

tim@openbmc:~/qemu$ QTEST_LOG=1 ./build/tests/qtest/npcm8xx_pspi_test
# random seed: R02Sede04390f31d107799cc627dd5992309
1..3
# Start of aarch64 tests
# Start of npcm8xx_pspi tests
# starting QEMU: exec /home/tim/git/qemu/build/qemu-system-aarch64
-qtest unix:/tmp/qtest-974125.sock -qtest-log /dev/fd/2 -chardev
socket,path=/tmp/qtest-974125.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine
npcm845-evb -accel qtest
[I 0.000000] OPENED
[R +0.075439] endianness
[S +0.075466] OK little
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
"package": "v9.2.0-2138-g694a7d11fc"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities"}

{"return": {}}
[R +0.077905] writel 0xf0201002 0x1
[S +0.077915] OK
[R +0.077943] readl 0xf0201002
[S +0.077948] OK 0x0000000000000001
[I +0.078171] CLOSED
ok 1 /aarch64/npcm8xx_pspi/init
# starting QEMU: exec /home/tim/git/qemu/build/qemu-system-aarch64
-qtest unix:/tmp/qtest-974125.sock -qtest-log /dev/fd/2 -chardev
socket,path=/tmp/qtest-974125.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine
npcm845-evb -accel qtest
[I 0.000000] OPENED
[R +0.075965] endianness
[S +0.075991] OK little
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
"package": "v9.2.0-2138-g694a7d11fc"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities"}

{"return": {}}
[R +0.078505] write 0xf0201002 0x1 0x05
[S +0.078515] OK
[R +0.078546] readl 0xf0201002
[S +0.078551] OK 0x0000000000000005
Wrote 0x05 to control register
[I +0.078806] CLOSED
ok 2 /aarch64/npcm8xx_pspi/ctl
# starting QEMU: exec /home/tim/git/qemu/build/qemu-system-aarch64
-qtest unix:/tmp/qtest-974125.sock -qtest-log /dev/fd/2 -chardev
socket,path=/tmp/qtest-974125.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine
npcm845-evb -accel qtest
[I 0.000000] OPENED
[R +0.076480] endianness
[S +0.076506] OK little
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
"package": "v9.2.0-2138-g694a7d11fc"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities"}

{"return": {}}
[R +0.079280] writel 0xf0201002 0x5
[S +0.079288] OK
[R +0.079312] writew 0xf0201000 0x1234
[S +0.079316] OK
Wrote 0x1234 to data register
[R +0.079337] readw 0xf0201000
[S +0.079341] OK 0x0000000000001234
Read 0x1234 from data register
[I +0.079565] CLOSED
ok 3 /aarch64/npcm8xx_pspi/data
# End of npcm8xx_pspi tests
# End of aarch64 tests

-- 
Best regards,
Tim Lee

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

[Philippe Mathieu-Daudé]

Hi Tim,

On 6/5/25 04:14, Tim Lee wrote:
>> This fails on top of current master, please take a look:
>>
>> $ QTEST_LOG=1 QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/npcm8xx_pspi-test
>> # random seed: R02S03f79fc48ba73b76c881f93f90b015e9
>> 1..3
>> # Start of aarch64 tests
>> # Start of npcm8xx_pspi tests
>> # starting QEMU: exec ./qemu-system-aarch64 -qtest
>>    unix:/tmp/qtest-32530.sock -qtest-log /dev/fd/2 -chardev
>>    socket,path=/tmp/qtest-32530.qmp,id=char0 -mon
>>    chardev=char0,mode=control -display none -audio none -machine
>>    npcm845-evb -accel qtest
>> [I 0.000000] OPENED
>> [R +0.034918] endianness
>> [S +0.034944] OK little
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 10},
>> "package": "v10.0.0-530-g88d6459dae"}, "capabilities": ["oob"]}}
>> {"execute": "qmp_capabilities"}
>> {"return": {}}
>> [R +0.037373] writel 0xf0201002 0x1
>> [S +0.037396] OK
>> [R +0.037417] readl 0xf0201002
>> [S +0.037426] OK 0x0000000000000000
>> **
>> ERROR:../tests/qtest/npcm8xx_pspi-test.c:45:test_init: assertion failed
>> (pspi_read_ctl(qts, pspi) == CTL_SPIEN): (0x00000000 == 0x00000001)
>> Bail out!
>> [I +0.037909] CLOSED
>> Aborted (core dumped)
> 
> Thank you for testing it. I think the failure seems to be related to
> the following commit which, has not been merged yet.
> https://patchew.org/QEMU/20250414020629.1867106-1-timlee660101@gmail.com/
> Here is our test result for reference. Thanks.

It helps and saves maintainers' time if you group related / dependent
patches as a series, i.e.: patch introducing a feature with the patch
testing the feature.

Alternatively when big series are split, you can mention their
dependence using the 'Based-on' tag:

Based-on: 20250414020629.1867106-1-timlee660101@gmail.com

Regards,

Phil.

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

[Peter Maydell]

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.

>  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

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