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

[v2] 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>
---
Changes since v1:
- MAINTAINERS file not need to change
- Add comment for copyright/license information
- Correct CTL registers to use 16 bits
- Remove printf() in test cases

 tests/qtest/meson.build         |   3 +
 tests/qtest/npcm8xx_pspi-test.c | 118 ++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 tests/qtest/npcm8xx_pspi-test.c

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..13b8a8229c
--- /dev/null
+++ b/tests/qtest/npcm8xx_pspi-test.c
@@ -0,0 +1,118 @@
+/*
+ * QTests for the Nuvoton NPCM8XX PSPI Controller
+ *
+ * Copyright (c) 2025 Nuvoton Technology Corporation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qemu/module.h"
+
+/* Register offsets */
+#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 uint16_t pspi_read_ctl(QTestState *qts, const PSPI *pspi)
+{
+    return qtest_readw(qts, pspi->base_addr + CTL_OFFSET);
+}
+
+static void pspi_write_ctl(QTestState *qts, const PSPI *pspi, uint16_t value)
+{
+    qtest_writew(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");
+
+    /* Write CTL_SPIEN value to control register for enable PSPI module */
+    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");
+
+    /* Enable 16-bit data interface mode */
+    pspi_write_ctl(qts, pspi, CTL_MOD);
+    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, CTL_MOD);
+
+    /* Write to data register */
+    pspi_write_data(qts, pspi, test);
+
+    /* Read from data register */
+    output = pspi_read_data(qts, pspi);
+    g_assert_cmphex(output, ==, test);
+
+    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);
+
+    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: [v2] tests/qtest: Add qtest for NPCM8XX PSPI module

[KFTING]

From: Tim Lee <timlee660101@gmail.com>
Sent: Wednesday, May 7, 2025 5:19 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: [v2] 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>
---
Changes since v1:
- MAINTAINERS file not need to change
- Add comment for copyright/license information
- Correct CTL registers to use 16 bits
- Remove printf() in test cases

 tests/qtest/meson.build         |   3 +
 tests/qtest/npcm8xx_pspi-test.c | 118 ++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 tests/qtest/npcm8xx_pspi-test.c

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..13b8a8229c
--- /dev/null
+++ b/tests/qtest/npcm8xx_pspi-test.c
@@ -0,0 +1,118 @@
+/*
+ * QTests for the Nuvoton NPCM8XX PSPI Controller
+ *
+ * Copyright (c) 2025 Nuvoton Technology Corporation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qemu/module.h"
+
+/* Register offsets */
+#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 uint16_t pspi_read_ctl(QTestState *qts, const PSPI *pspi) {
+    return qtest_readw(qts, pspi->base_addr + CTL_OFFSET); }
+
+static void pspi_write_ctl(QTestState *qts, const PSPI *pspi, uint16_t
+value) {
+    qtest_writew(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");
+
+    /* Write CTL_SPIEN value to control register for enable PSPI module */
+    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");
+
+    /* Enable 16-bit data interface mode */
+    pspi_write_ctl(qts, pspi, CTL_MOD);
+    g_assert_cmphex(pspi_read_ctl(qts, pspi), ==, CTL_MOD);
+
+    /* Write to data register */
+    pspi_write_data(qts, pspi, test);
+
+    /* Read from data register */
+    output = pspi_read_data(qts, pspi);
+    g_assert_cmphex(output, ==, test);
+
+    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);
+
+    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: [v2] tests/qtest: Add qtest for NPCM8XX PSPI module

[Peter Maydell]

On Wed, 7 May 2025 at 10:19, 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>
> ---
> Changes since v1:
> - MAINTAINERS file not need to change
> - Add comment for copyright/license information
> - Correct CTL registers to use 16 bits
> - Remove printf() in test cases



Applied to target-arm.next, thanks.

-- PMM

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

[Peter Maydell]

On Sun, 11 May 2025 at 14:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 7 May 2025 at 10:19, 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>
> > ---
> > Changes since v1:
> > - MAINTAINERS file not need to change
> > - Add comment for copyright/license information
> > - Correct CTL registers to use 16 bits
> > - Remove printf() in test cases
>
>
>
> Applied to target-arm.next, thanks.

...but it fails "make check", so I've dropped it:

not ok /aarch64/npcm8xx_pspi/data - ERROR:../../tests/qtest/npcm8xx_pspi-test.c:
80:test_data: assertion failed (output == test): (0x00000000 == 0x00001234)
Bail out!
----------------------------------- stderr -----------------------------------
**
ERROR:../../tests/qtest/npcm8xx_pspi-test.c:80:test_data: assertion failed (outp
ut == test): (0x00000000 == 0x00001234)

(test program exited with status code -6)

thanks
-- PMM

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

[Tim Lee]

Hi Peter,
Sorry about that. When I ran this qtest and I found an error then I
tried to modify npcm_pspi driver to make data register read/write test
pass.

[R +0.080118] writew 0xf0201002 0x4
[S +0.080126] OK
[R +0.080148] readw 0xf0201002
[S +0.080153] OK 0x0000000000000004
[R +0.080168] writew 0xf0201000 0x1234
[S +0.080171] OK
[R +0.080191] readw 0xf0201000
[S +0.080194] OK 0x0000000000001234
[I +0.080445] CLOSED
ok 3 /aarch64/npcm8xx_pspi/data
# End of npcm8xx_pspi tests
# End of aarch64 tests

Here is the change diff of what I modified in npcm_pspi_write_data()
Should I submit this patch for npcm_pspi driver? I'm not sure if I
modified it correctly. Thanks for your time.

 static void npcm_pspi_write_data(NPCMPSPIState *s, uint16_t data)
 {
-    uint16_t value = 0;
+    uint16_t data_l, data_h = 0;

     if (FIELD_EX16(s->regs[R_PSPI_CTL1], PSPI_CTL1, MOD)) {
-        value = ssi_transfer(s->spi, extract16(data, 8, 8)) << 8;
+        data_h = (extract16(data, 8, 8) << 8);
+        ssi_transfer(s->spi, data_h);
     }
-    value |= ssi_transfer(s->spi, extract16(data, 0, 8));
-    s->regs[R_PSPI_DATA] = value;
+    data_l = extract16(data, 0, 8);
+    ssi_transfer(s->spi, data_l);
+    s->regs[R_PSPI_DATA] = (data_h | data_l);

Peter Maydell <peter.maydell@linaro.org> 於 2025年5月11日 週日 下午9:58寫道：
>
> On Sun, 11 May 2025 at 14:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 7 May 2025 at 10:19, 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>
> > > ---
> > > Changes since v1:
> > > - MAINTAINERS file not need to change
> > > - Add comment for copyright/license information
> > > - Correct CTL registers to use 16 bits
> > > - Remove printf() in test cases
> >
> >
> >
> > Applied to target-arm.next, thanks.
>
> ...but it fails "make check", so I've dropped it:
>
> not ok /aarch64/npcm8xx_pspi/data - ERROR:../../tests/qtest/npcm8xx_pspi-test.c:
> 80:test_data: assertion failed (output == test): (0x00000000 == 0x00001234)
> Bail out!
> ----------------------------------- stderr -----------------------------------
> **
> ERROR:../../tests/qtest/npcm8xx_pspi-test.c:80:test_data: assertion failed (outp
> ut == test): (0x00000000 == 0x00001234)
>
> (test program exited with status code -6)
>
> thanks
> -- PMM

-- 
Best regards,
Tim Lee

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

[Peter Maydell]

On Mon, 12 May 2025 at 02:25, Tim Lee <timlee660101@gmail.com> wrote:
>
> Hi Peter,
> Sorry about that. When I ran this qtest and I found an error then I
> tried to modify npcm_pspi driver to make data register read/write test
> pass.
>
> [R +0.080118] writew 0xf0201002 0x4
> [S +0.080126] OK
> [R +0.080148] readw 0xf0201002
> [S +0.080153] OK 0x0000000000000004
> [R +0.080168] writew 0xf0201000 0x1234
> [S +0.080171] OK
> [R +0.080191] readw 0xf0201000
> [S +0.080194] OK 0x0000000000001234
> [I +0.080445] CLOSED
> ok 3 /aarch64/npcm8xx_pspi/data
> # End of npcm8xx_pspi tests
> # End of aarch64 tests
>
> Here is the change diff of what I modified in npcm_pspi_write_data()
> Should I submit this patch for npcm_pspi driver? I'm not sure if I
> modified it correctly. Thanks for your time.
>
>  static void npcm_pspi_write_data(NPCMPSPIState *s, uint16_t data)
>  {
> -    uint16_t value = 0;
> +    uint16_t data_l, data_h = 0;
>
>      if (FIELD_EX16(s->regs[R_PSPI_CTL1], PSPI_CTL1, MOD)) {
> -        value = ssi_transfer(s->spi, extract16(data, 8, 8)) << 8;
> +        data_h = (extract16(data, 8, 8) << 8);
> +        ssi_transfer(s->spi, data_h);
>      }
> -    value |= ssi_transfer(s->spi, extract16(data, 0, 8));
> -    s->regs[R_PSPI_DATA] = value;
> +    data_l = extract16(data, 0, 8);
> +    ssi_transfer(s->spi, data_l);
> +    s->regs[R_PSPI_DATA] = (data_h | data_l);

If you think there's a bug in the PSPI device then the
best thing is to submit what you think is the fix as
its own patch, with a commit message that details what
the incorrect behaviour is (and why you think it's incorrect,
e.g. with reference to a data sheet). Then the NPCM folks can
review it for whether it's correct or not.

You can send a 2-patch series with the bug fix as patch 1,
followed by the new test case as patch 2.

thanks
-- PMM