[PATCH 0/6] Add nRF51 DETECT signal with test

[PATCH 0/6] Add nRF51 DETECT signal with test

[Chris Laplante]

This patch series implements the nRF51 DETECT signal
in the GPIO peripheral. A qtest is added exercising the signal.

To implement the test, named out-GPIO IRQ interception had to be added
to the qtest framework. I also took the opportunity to improve IRQ
interception a bit by adding 'FAIL' responses when interception fails.
Otherwise, it is frustrating to troubleshoot why calls to
qtest_irq_intercept_out and friends appears to do nothing.

Chris Laplante (6):
  hw/gpio/nrf51: implement DETECT signal
  qtest: implement named interception of out-GPIO
  qtest: bail from irq_intercept_in if name is specified
  qtest: factor out qtest_install_gpio_out_intercepts
  qtest: irq_intercept_[out/in]: return FAIL if no intercepts are
    installed
  qtest: microbit-test: add tests for nRF51 DETECT

 hw/arm/nrf51_soc.c           |  1 +
 hw/gpio/nrf51_gpio.c         | 14 ++++++++-
 include/hw/gpio/nrf51_gpio.h |  1 +
 softmmu/qtest.c              | 56 ++++++++++++++++++++++++++----------
 tests/qtest/libqtest.c       |  6 ++++
 tests/qtest/libqtest.h       | 11 +++++++
 tests/qtest/microbit-test.c  | 42 +++++++++++++++++++++++++++
 7 files changed, 115 insertions(+), 16 deletions(-)

--
2.39.2

[PATCH 1/6] hw/gpio/nrf51: implement DETECT signal

[Chris Laplante]

Implement nRF51 DETECT signal in the GPIO peripheral.

The reference manual makes mention of a per-pin DETECT signal, but these
are not exposed to the user. See https://devzone.nordicsemi.com/f/nordic-q-a/39858/gpio-per-pin-detect-signal-available
for more information. Currently, I don't see a reason to model these.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 hw/arm/nrf51_soc.c           |  1 +
 hw/gpio/nrf51_gpio.c         | 14 +++++++++++++-
 include/hw/gpio/nrf51_gpio.h |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 34da0d62f0..7ae54e18be 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -150,6 +150,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     /* Pass all GPIOs to the SOC layer so they are available to the board */
     qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);
+    qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, "detect");
 
     /* TIMER */
     for (i = 0; i < NRF51_NUM_TIMERS; i++) {
diff --git a/hw/gpio/nrf51_gpio.c b/hw/gpio/nrf51_gpio.c
index b47fddf4ed..08396c69a4 100644
--- a/hw/gpio/nrf51_gpio.c
+++ b/hw/gpio/nrf51_gpio.c
@@ -78,6 +78,7 @@ static void update_state(NRF51GPIOState *s)
     int pull;
     size_t i;
     bool connected_out, dir, connected_in, out, in, input;
+    bool assert_detect = false;
 
     for (i = 0; i < NRF51_GPIO_PINS; i++) {
         pull = pull_value(s->cnf[i]);
@@ -99,7 +100,15 @@ static void update_state(NRF51GPIOState *s)
                 qemu_log_mask(LOG_GUEST_ERROR,
                               "GPIO pin %zu short circuited\n", i);
             }
-            if (!connected_in) {
+            if (connected_in) {
+                uint32_t detect_config = extract32(s->cnf[i], 16, 2);
+                if ((detect_config == 2) && (in == 1)) {
+                    assert_detect = true;
+                }
+                if ((detect_config == 3) && (in == 0)) {
+                    assert_detect = true;
+                }
+            } else {
                 /*
                  * Floating input: the output stimulates IN if connected,
                  * otherwise pull-up/pull-down resistors put a value on both
@@ -116,6 +125,8 @@ static void update_state(NRF51GPIOState *s)
         }
         update_output_irq(s, i, connected_out, out);
     }
+
+    qemu_set_irq(s->detect, assert_detect);
 }
 
 /*
@@ -291,6 +302,7 @@ static void nrf51_gpio_init(Object *obj)
 
     qdev_init_gpio_in(DEVICE(s), nrf51_gpio_set, NRF51_GPIO_PINS);
     qdev_init_gpio_out(DEVICE(s), s->output, NRF51_GPIO_PINS);
+    qdev_init_gpio_out_named(DEVICE(s), &s->detect, "detect", 1);
 }
 
 static void nrf51_gpio_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
index 8f9c2f86da..fcfa2bac17 100644
--- a/include/hw/gpio/nrf51_gpio.h
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -64,6 +64,7 @@ struct NRF51GPIOState {
     uint32_t old_out_connected;
 
     qemu_irq output[NRF51_GPIO_PINS];
+    qemu_irq detect;
 };
 
 
-- 
2.39.2

[PATCH 2/6] qtest: implement named interception of out-GPIO

[Chris Laplante]

Adds qtest_irq_intercept_out_named method, which utilizes a new optional
name parameter to the irq_intercept_out qtest command.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 softmmu/qtest.c        | 39 ++++++++++++++++++++++++++-------------
 tests/qtest/libqtest.c |  6 ++++++
 tests/qtest/libqtest.h | 11 +++++++++++
 3 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8d764b719..7c3dea5760 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -388,8 +388,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         || strcmp(words[0], "irq_intercept_in") == 0) {
         DeviceState *dev;
         NamedGPIOList *ngl;
+        bool is_named;
+        bool is_outbound;
 
         g_assert(words[1]);
+        is_named = words[2] != NULL;
+        is_outbound = words[0][14] == 'o';
         dev = DEVICE(object_resolve_path(words[1], NULL));
         if (!dev) {
             qtest_send_prefix(chr);
@@ -408,19 +412,28 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         }
 
         QLIST_FOREACH(ngl, &dev->gpios, node) {
-            /* We don't support intercept of named GPIOs yet */
-            if (ngl->name) {
-                continue;
-            }
-            if (words[0][14] == 'o') {
-                int i;
-                for (i = 0; i < ngl->num_out; ++i) {
-                    qemu_irq *disconnected = g_new0(qemu_irq, 1);
-                    qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
-                                                      disconnected, i);
-
-                    *disconnected = qdev_intercept_gpio_out(dev, icpt,
-                                                            ngl->name, i);
+            /* We don't support inbound interception of named GPIOs yet */
+            if (is_outbound) {
+                if (is_named) {
+                    if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
+                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
+                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+                                                          disconnected, 0);
+
+                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
+                                                                ngl->name, 0);
+                        break;
+                    }
+                } else if (!ngl->name) {
+                    int i;
+                    for (i = 0; i < ngl->num_out; ++i) {
+                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
+                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+                                                          disconnected, i);
+
+                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
+                                                                ngl->name, i);
+                    }
                 }
             } else {
                 qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index c22dfc30d3..471529e6cc 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -993,6 +993,12 @@ void qtest_irq_intercept_out(QTestState *s, const char *qom_path)
     qtest_rsp(s);
 }
 
+void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name)
+{
+    qtest_sendf(s, "irq_intercept_out %s %s\n", qom_path, name);
+    qtest_rsp(s);
+}
+
 void qtest_irq_intercept_in(QTestState *s, const char *qom_path)
 {
     qtest_sendf(s, "irq_intercept_in %s\n", qom_path);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 3a71bc45fc..e53e350e3a 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -371,6 +371,17 @@ void qtest_irq_intercept_in(QTestState *s, const char *string);
  */
 void qtest_irq_intercept_out(QTestState *s, const char *string);
 
+/**
+ * qtest_irq_intercept_out_named:
+ * @s: #QTestState instance to operate on.
+ * @qom_path: QOM path of a device.
+ * @name: Name of the GPIO out pin
+ *
+ * Associate a qtest irq with the named GPIO-out pin of the device
+ * whose path is specified by @string and whose name is @name.
+ */
+void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name);
+
 /**
  * qtest_set_irq_in:
  * @s: QTestState instance to operate on.
-- 
2.39.2

[PATCH 3/6] qtest: bail from irq_intercept_in if name is specified

[Chris Laplante]

Named interception of in-GPIOs is not supported yet.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 softmmu/qtest.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 7c3dea5760..74482ce3cd 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -401,6 +401,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
             return;
         }
 
+        if (is_named && !is_outbound) {
+            qtest_send_prefix(chr);
+            qtest_send(chr, "FAIL Interception of named in-GPIOs not yet supported\n");
+            return;
+        }
+
         if (irq_intercept_dev) {
             qtest_send_prefix(chr);
             if (irq_intercept_dev != dev) {
@@ -412,7 +418,6 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         }
 
         QLIST_FOREACH(ngl, &dev->gpios, node) {
-            /* We don't support inbound interception of named GPIOs yet */
             if (is_outbound) {
                 if (is_named) {
                     if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
-- 
2.39.2

[PATCH 4/6] qtest: factor out qtest_install_gpio_out_intercepts

[Chris Laplante]

Simplify the code a bit.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 softmmu/qtest.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 74482ce3cd..051bbf4177 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -365,6 +365,15 @@ void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words))
     process_command_cb = pc_cb;
 }
 
+static void qtest_install_gpio_out_intercepts(DeviceState *dev, const char *name, int n)
+{
+    qemu_irq *disconnected = g_new0(qemu_irq, 1);
+    qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+                                      disconnected, n);
+
+    *disconnected = qdev_intercept_gpio_out(dev, icpt,name, n);
+}
+
 static void qtest_process_command(CharBackend *chr, gchar **words)
 {
     const gchar *command;
@@ -421,23 +430,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
             if (is_outbound) {
                 if (is_named) {
                     if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
-                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
-                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
-                                                          disconnected, 0);
-
-                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
-                                                                ngl->name, 0);
+                        qtest_install_gpio_out_intercepts(dev, ngl->name, 0);
                         break;
                     }
                 } else if (!ngl->name) {
                     int i;
                     for (i = 0; i < ngl->num_out; ++i) {
-                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
-                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
-                                                          disconnected, i);
-
-                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
-                                                                ngl->name, i);
+                        qtest_install_gpio_out_intercepts(dev, ngl->name, i);
                     }
                 }
             } else {
-- 
2.39.2

[PATCH 5/6] qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed

[Chris Laplante]

This is much better than just silently failing with OK.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 softmmu/qtest.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 051bbf4177..e888acb319 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -399,6 +399,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         NamedGPIOList *ngl;
         bool is_named;
         bool is_outbound;
+        bool interception_succeeded = false;
 
         g_assert(words[1]);
         is_named = words[2] != NULL;
@@ -431,6 +432,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                 if (is_named) {
                     if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
                         qtest_install_gpio_out_intercepts(dev, ngl->name, 0);
+                        interception_succeeded = true;
                         break;
                     }
                 } else if (!ngl->name) {
@@ -438,15 +440,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                     for (i = 0; i < ngl->num_out; ++i) {
                         qtest_install_gpio_out_intercepts(dev, ngl->name, i);
                     }
+                    interception_succeeded = true;
                 }
             } else {
                 qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
                                       ngl->num_in);
+                interception_succeeded = true;
             }
         }
-        irq_intercept_dev = dev;
+
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK\n");
+        if (interception_succeeded) {
+            irq_intercept_dev = dev;
+            qtest_send(chr, "OK\n");
+        } else {
+            qtest_send(chr, "FAIL No intercepts installed\n");
+        }
     } else if (strcmp(words[0], "set_irq_in") == 0) {
         DeviceState *dev;
         qemu_irq irq;
-- 
2.39.2

[PATCH 6/6] qtest: microbit-test: add tests for nRF51 DETECT

[Chris Laplante]

Exercise the DETECT mechanism of the GPIO peripheral.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 tests/qtest/microbit-test.c | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
index 6022a92b6a..3c85adba37 100644
--- a/tests/qtest/microbit-test.c
+++ b/tests/qtest/microbit-test.c
@@ -393,6 +393,47 @@ static void test_nrf51_gpio(void)
     qtest_quit(qts);
 }
 
+static void test_nrf51_gpio_detect(void) {
+    QTestState *qts = qtest_init("-M microbit");
+    int i;
+
+    // Connect input buffer on pins 1-7, configure SENSE for high level
+    for (i = 1; i <= 7; i++) {
+        qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4, deposit32(0, 16, 2, 2));
+    }
+
+    qtest_irq_intercept_out_named(qts, "/machine/nrf51", "detect");
+
+    for (i = 1; i <= 7; i++) {
+        // Set pin high
+        qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1);
+        uint32_t actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN);
+        g_assert_cmpuint(actual, ==, 1 << i);
+
+        // Check that DETECT is high
+        g_assert_true(qtest_get_irq(qts, 0));
+
+        // Set pin low, check that DETECT goes low.
+        qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 0);
+        actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN);
+        g_assert_cmpuint(actual, ==, 0x0);
+        g_assert_false(qtest_get_irq(qts, 0));
+    }
+
+    // Set pin 0 high, check that DETECT doesn't fire
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
+    g_assert_false(qtest_get_irq(qts, 0));
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+
+    // Set pins 1, 2, and 3 high, then set 3 low. Check that DETECT is still high.
+    for (i = 1; i <= 3; i++) {
+        qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1);
+    }
+    g_assert_true(qtest_get_irq(qts, 0));
+    qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 3, 0);
+    g_assert_true(qtest_get_irq(qts, 0));
+}
+
 static void timer_task(QTestState *qts, hwaddr task)
 {
     qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
@@ -499,6 +540,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart);
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
+    qtest_add_func("/microbit/nrf51/gpio_detect", test_nrf51_gpio_detect);
     qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
     qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
     qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
-- 
2.39.2

Re: [PATCH 1/6] hw/gpio/nrf51: implement DETECT signal

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote:
>
> Implement nRF51 DETECT signal in the GPIO peripheral.
>
> The reference manual makes mention of a per-pin DETECT signal, but these
> are not exposed to the user. See https://devzone.nordicsemi.com/f/nordic-q-a/39858/gpio-per-pin-detect-signal-available
> for more information. Currently, I don't see a reason to model these.

I agree -- they seem to be internal to the GPIO module,
so we don't need to model them as qemu_irq lines.

> Signed-off-by: Chris Laplante <chris@laplante.io>
> ---
>  hw/arm/nrf51_soc.c           |  1 +
>  hw/gpio/nrf51_gpio.c         | 14 +++++++++++++-
>  include/hw/gpio/nrf51_gpio.h |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 34da0d62f0..7ae54e18be 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -150,6 +150,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>
>      /* Pass all GPIOs to the SOC layer so they are available to the board */
>      qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);
> +    qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, "detect");

Is the DETECT line really exposed external to the SoC?
I had a look at the nRF51822 datasheet and it suggests not.
For purposes of supporting the wake-up-on-gpio functionality
we don't need to expose it to the board -- the SoC layer
can just wire it up to the POWER device. (In fact, exposing
it to the board makes it harder, because you can't connect
one qemu_irq to two places, so if we let the board connect
it somewhere then the SoC can't conveniently connect it
to the POWER device without doing extra work to split it.)

The logic for calculating DETECT looks good to me.

thanks
-- PMM

Re: [PATCH 3/6] qtest: bail from irq_intercept_in if name is specified

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote:
>
> Named interception of in-GPIOs is not supported yet.
>
> Signed-off-by: Chris Laplante <chris@laplante.io>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 4/6] qtest: factor out qtest_install_gpio_out_intercepts

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote:
>
> Simplify the code a bit.
>
> Signed-off-by: Chris Laplante <chris@laplante.io>
> ---
>  softmmu/qtest.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
> index 74482ce3cd..051bbf4177 100644
> --- a/softmmu/qtest.c
> +++ b/softmmu/qtest.c
> @@ -365,6 +365,15 @@ void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words))
>      process_command_cb = pc_cb;
>  }
>
> +static void qtest_install_gpio_out_intercepts(DeviceState *dev, const char *name, int n)
> +{
> +    qemu_irq *disconnected = g_new0(qemu_irq, 1);
> +    qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
> +                                      disconnected, n);
> +
> +    *disconnected = qdev_intercept_gpio_out(dev, icpt,name, n);
> +}
> +
>  static void qtest_process_command(CharBackend *chr, gchar **words)
>  {
>      const gchar *command;
> @@ -421,23 +430,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>              if (is_outbound) {
>                  if (is_named) {
>                      if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
> -                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
> -                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
> -                                                          disconnected, 0);
> -
> -                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
> -                                                                ngl->name, 0);
> +                        qtest_install_gpio_out_intercepts(dev, ngl->name, 0);
>                          break;
>                      }
>                  } else if (!ngl->name) {
>                      int i;
>                      for (i = 0; i < ngl->num_out; ++i) {
> -                        qemu_irq *disconnected = g_new0(qemu_irq, 1);
> -                        qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
> -                                                          disconnected, i);
> -
> -                        *disconnected = qdev_intercept_gpio_out(dev, icpt,
> -                                                                ngl->name, i);
> +                        qtest_install_gpio_out_intercepts(dev, ngl->name, i);
>                      }

I think you should put this patch before patch 2 -- create the
new function first, and then you can directly use it,
rather than first creating the duplicate code and then
getting rid of it.

thanks
-- PMM

Re: [PATCH 5/6] qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote:
>
> This is much better than just silently failing with OK.
>
> Signed-off-by: Chris Laplante <chris@laplante.io>

Makes sense. Did you do a 'make check' on an
all-targets-enabled build just to confirm we haven't
accidentally let any bogus uses of the command in while
it was returning OK for these cases?

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 6/6] qtest: microbit-test: add tests for nRF51 DETECT

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:28, Chris Laplante <chris@laplante.io> wrote:
>
> Exercise the DETECT mechanism of the GPIO peripheral.
>
> Signed-off-by: Chris Laplante <chris@laplante.io>

I think you want to intercept the DETECT line on the
GPIO device itself, not on the SoC (see comments on
patch 1), but otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Peter Maydell]

On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote:
>
> This patch series implements the nRF51 DETECT signal
> in the GPIO peripheral. A qtest is added exercising the signal.
>
> To implement the test, named out-GPIO IRQ interception had to be added
> to the qtest framework. I also took the opportunity to improve IRQ
> interception a bit by adding 'FAIL' responses when interception fails.
> Otherwise, it is frustrating to troubleshoot why calls to
> qtest_irq_intercept_out and friends appears to do nothing.

Thanks for this patchset and especially for the work
improving the qtest infrastructure. I've given my
comments on the different patches, and in some cases
reviewed-by tags. (Where I've given one of those, you should
add it to your commit message for the relevant patch under
your Signed-off-by: line, so that when you send the version
2 of the patchset we know that those parts are already
reviewed and don't need re-examining. If I said "make
some change; otherwise Reviewed-by" that means "make
that minor change, and then you can add the tag, etc".)

Do you have the parts of this feature that use the DETECT
signal in the POWER device, or have you not written those
yet ? If you have them, you could send those too in v2.

-- PMM

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Chris Laplante]

Hi Peter,

> Thanks for this patchset and especially for the work
> improving the qtest infrastructure. I've given my
> comments on the different patches, and in some cases
> reviewed-by tags. (Where I've given one of those, you should
> add it to your commit message for the relevant patch under
> your Signed-off-by: line, so that when you send the version
> 2 of the patchset we know that those parts are already
> reviewed and don't need re-examining. If I said "make
> some change; otherwise Reviewed-by" that means "make
> that minor change, and then you can add the tag, etc".)

Thanks very much for the feedback and help!

> Do you have the parts of this feature that use the DETECT
> signal in the POWER device, or have you not written those
> yet ? If you have them, you could send those too in v2.

That part is halfway done, so I will work on finishing it before submitting v2. Two questions regarding that (to potentially save us a v3): 

1. The nRF51 POWER device overlaps with the memory maps of the CLOCK and MPU devices. So I have created a CPM (CLOCK, POWER, MPU) device in hw/misc. Does that sound reasonable naming-wise?
2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER.

Thanks,
Chris

Re: [PATCH 5/6] qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed

[Chris Laplante]

> Makes sense. Did you do a 'make check' on an
> all-targets-enabled build just to confirm we haven't
> accidentally let any bogus uses of the command in while
> it was returning OK for these cases?
> 
> Reviewed-by: Peter Maydell peter.maydell@linaro.org

Yes, I just did a 'make check' and got:

Ok:                 737 
Expected Fail:      0   
Fail:               0   
Unexpected Pass:    0   
Skipped:            71  
Timeout:            0   

Thanks,
Chris

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Peter Maydell]

On Tue, 25 Jul 2023 at 04:25, Chris Laplante <chris@laplante.io> wrote:
>
> Hi Peter,
>
> > Thanks for this patchset and especially for the work
> > improving the qtest infrastructure. I've given my
> > comments on the different patches, and in some cases
> > reviewed-by tags. (Where I've given one of those, you should
> > add it to your commit message for the relevant patch under
> > your Signed-off-by: line, so that when you send the version
> > 2 of the patchset we know that those parts are already
> > reviewed and don't need re-examining. If I said "make
> > some change; otherwise Reviewed-by" that means "make
> > that minor change, and then you can add the tag, etc".)
>
> Thanks very much for the feedback and help!
>
> > Do you have the parts of this feature that use the DETECT
> > signal in the POWER device, or have you not written those
> > yet ? If you have them, you could send those too in v2.
>
> That part is halfway done, so I will work on finishing it before submitting v2. Two questions regarding that (to potentially save us a v3):
>
> 1. The nRF51 POWER device overlaps with the memory maps of the CLOCK and MPU devices. So I have created a CPM (CLOCK, POWER, MPU) device in hw/misc. Does that sound reasonable naming-wise?

Yes, I think from QEMU's point of view the massive register
overlap makes them a single device. The name sounds OK
(give it the same kind of nrf51 prefix the rng device has).

> 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER.

If you think they're ready to go in, and it doesn't
make the series more than about 12-15 patches long,
you can put them on the end of the series. If the
patchset is starting to get a bit big then it might
be easier to get the POWER/DETECT parts reviewed
first.

thanks
-- PMM

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Chris Laplante]

> > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER.
> 
> 
> If you think they're ready to go in, and it doesn't
> make the series more than about 12-15 patches long,
> you can put them on the end of the series. If the
> patchset is starting to get a bit big then it might
> be easier to get the POWER/DETECT parts reviewed
> first.

I'm just going to send the POWER/DETECT bits first. There is quite a lot to emulate in CLOCK, POWER, and MPU, and I'd like to do a good job on it.

Thanks.
Chris

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Chris Laplante]

Hi Peter,

> > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER.
> 
> 
> If you think they're ready to go in, and it doesn't
> make the series more than about 12-15 patches long,
> you can put them on the end of the series. If the
> patchset is starting to get a bit big then it might
> be easier to get the POWER/DETECT parts reviewed
> first.

Unrelated question regarding the CLOCK module. Should I model the startup times for the various crystal oscillators? Or should I just assume they start instantly for simplicity? External xtal startup time is 750-800 us. Internal RC startup time is 4-5 us. I've already modeled the delay for the external xtal, but just wondering if its worth the extra code.

Thanks,
Chris

Re: [PATCH 0/6] Add nRF51 DETECT signal with test

[Peter Maydell]

On Thu, 27 Jul 2023 at 23:51, Chris Laplante <chris@laplante.io> wrote:
>
> Hi Peter,
>
> > > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER.
> >
> >
> > If you think they're ready to go in, and it doesn't
> > make the series more than about 12-15 patches long,
> > you can put them on the end of the series. If the
> > patchset is starting to get a bit big then it might
> > be easier to get the POWER/DETECT parts reviewed
> > first.
>
> Unrelated question regarding the CLOCK module. Should I model the startup times for the various crystal oscillators? Or should I just assume they start instantly for simplicity? External xtal startup time is 750-800 us. Internal RC startup time is 4-5 us. I've already modeled the delay for the external xtal, but just wondering if its worth the extra code.

We typically just have that sort of thing start instantly,
unless there's some specific guest workload that falls
over if you don't model the startup delay. Usually
modelling the delay is unnecessary complexity.

thanks
-- PMM