[PATCH v3 4/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 
Reviewed-by: Peter Maydell 
---
 softmmu/qtest.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 0f1d478bda..66757ba261 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -397,9 +397,11 @@ 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) {
@@ -408,6 +410,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) {
-- 
2.41.0

[PATCH v3 3/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 
---
 softmmu/qtest.c| 18 ++
 tests/qtest/libqtest.c |  6 ++
 tests/qtest/libqtest.h | 11 +++
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 1b86489162..0f1d478bda 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -397,8 +397,10 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 || strcmp(words[0], "irq_intercept_in") == 0) {
 DeviceState *dev;
 NamedGPIOList *ngl;
+bool is_outbound;
 
 g_assert(words[1]);
+is_outbound = words[0][14] == 'o';
 dev = DEVICE(object_resolve_path(words[1], NULL));
 if (!dev) {
 qtest_send_prefix(chr);
@@ -417,14 +419,14 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 }
 
 QLIST_FOREACH(ngl, >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) {
-qtest_install_gpio_out_intercept(dev, ngl->name, i);
+/* We don't support inbound interception of named GPIOs yet */
+if (is_outbound) {
+/* NULL is valid and matchable, for "unnamed GPIO" */
+if (g_strcmp0(ngl->name, words[2]) == 0) {
+int i;
+for (i = 0; i < ngl->num_out; ++i) {
+qtest_install_gpio_out_intercept(dev, 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.41.0

[PATCH v3 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 
---
 softmmu/qtest.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 66757ba261..35b643a274 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;
@@ -435,15 +436,22 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 for (i = 0; i < ngl->num_out; ++i) {
 qtest_install_gpio_out_intercept(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.41.0

[PATCH v3 2/6] qtest: factor out qtest_install_gpio_out_intercept

[Chris Laplante]

Signed-off-by: Chris Laplante 
Reviewed-by: Peter Maydell 
---
 softmmu/qtest.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8d764b719..1b86489162 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_intercept(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;
@@ -415,12 +424,7 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 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);
+qtest_install_gpio_out_intercept(dev, ngl->name, i);
 }
 } else {
 qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
-- 
2.41.0

[PATCH v3 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.

v2: https://patchwork.kernel.org/project/qemu-devel/list/?series=769532

Testing
===
Passes 'make check'

Changelog
=
v2: factor out qtest_install_gpio_out_intercept before usage (Peter)
renamed qtest_install_gpio_out_intercepts => 
qtest_install_gpio_out_intercept
don't pass DETECT to soc level (Peter)
change qtest to use DETECT at GPIO level (Peter)

v3: formatting fixup (Peter)
handle multiple named out-GPIOs, not just one (Peter)

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

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

--
2.41.0

[PATCH v3 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 
Reviewed-by: Peter Maydell 
---
 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..8f87810cd5 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/gpio", "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.41.0

[PATCH v3 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 
Reviewed-by: Peter Maydell 
---
 hw/gpio/nrf51_gpio.c | 14 +-
 include/hw/gpio/nrf51_gpio.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

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), >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.41.0

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 v2 3/6] qtest: implement named interception of out-GPIO

[Chris Laplante]

> (g_strcmp0() can handle the NULL case without having
> to special case it -- this is how qdev_get_named_gpio_list()
> finds entries in the ngl list.)
> 
> Apologies for not noticing that on the first round of review.

No worries - it makes the code much simpler anyway. Should we bother factoring 
out qtest_install_gpio_out_intercept still? It is only used in one place now, 
as before.

Thanks,
Chris

[PATCH v2 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 
Reviewed-by: Peter Maydell 
---
 softmmu/qtest.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 1719bbddc3..c9751f527f 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_intercept(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_intercept(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.41.0

[PATCH v2 4/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 
Reviewed-by: Peter Maydell 
---
 softmmu/qtest.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 7fd8546ed2..1719bbddc3 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -410,6 +410,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) {
@@ -421,7 +427,6 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 }
 
 QLIST_FOREACH(ngl, >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.41.0

[PATCH v2 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 
---
 hw/gpio/nrf51_gpio.c | 14 +-
 include/hw/gpio/nrf51_gpio.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

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), >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.41.0

[PATCH v2 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 
Reviewed-by: Peter Maydell 
---
 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..8f87810cd5 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/gpio", "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.41.0

[PATCH v2 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.

v1: https://patchwork.kernel.org/project/qemu-devel/list/?series=766078

Testing
===
Passes 'make check'

Changelog
=
v2: factor out qtest_install_gpio_out_intercept before usage (Peter)
renamed qtest_install_gpio_out_intercepts => 
qtest_install_gpio_out_intercept
don't pass DETECT to soc level (Peter)
change qtest to use DETECT at GPIO level (Peter)


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

 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 +++
 6 files changed, 114 insertions(+), 16 deletions(-)

--
2.41.0

[PATCH v2 2/6] qtest: factor out qtest_install_gpio_out_intercept

[Chris Laplante]

Signed-off-by: Chris Laplante 
---
 softmmu/qtest.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8d764b719..1c92e5a6a3 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_intercept(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;
@@ -415,12 +424,7 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 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);
+qtest_install_gpio_out_intercept(dev, ngl->name, i);
 }
 } else {
 qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
-- 
2.41.0

[PATCH v2 3/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 
---
 softmmu/qtest.c| 24 
 tests/qtest/libqtest.c |  6 ++
 tests/qtest/libqtest.h | 11 +++
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 1c92e5a6a3..7fd8546ed2 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -397,8 +397,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);
@@ -417,14 +421,18 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 }
 
 QLIST_FOREACH(ngl, >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) {
-qtest_install_gpio_out_intercept(dev, 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) {
+qtest_install_gpio_out_intercept(dev, ngl->name, 0);
+break;
+}
+} else if (!ngl->name) {
+int i;
+for (i = 0; i < ngl->num_out; ++i) {
+qtest_install_gpio_out_intercept(dev, 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.41.0

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 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.mayd...@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

[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

Thoughts on implementing SEGGER RTT

[Chris Laplante]

Hi all,

SEGGER RTT (https://wiki.segger.com/RTT) is a software debug mechanism which, 
among other things, exposes convenient virtual terminals. It is implemented as 
ring buffers in RAM. There is a control block with a fixed ID so it can be 
located by the RTT viewer (which runs on your PC, for example) at runtime.

I'm considering what it would take to implement an RTT viewer in qemu. Some 
small parts would be easy:

- Add a '-rtt' command line option
- Use address_space_read or flatview_* stuff to locate the fixed ID in the 
control block

But I don't yet know enough about QEMU to understand some bigger picture things:

- I assume the code for reading messages would need to live in a QemuThread? 
- Where would I output virtual terminal messages to?
- In the case of bidirectional communication, how would a user input messages 
to send to the emulated device?
- Is there a way to monitor memory ranges for changes, or would I need another 
way to detect when new messages are in the ring buffer?

Another possibility (which would avoid the last 3 issues, but of course create 
more complexity in other areas) is to have QEMU emulate a SEGGER J-LINK and 
provide a TCP/IP endpoint that a real RTT Viewer could connect to 
(https://www.segger.com/products/debug-probes/j-link/tools/rtt-viewer/). I'm 
not sure how feasible this is, though.

If anyone has any thoughts or guidance I'd appreciate it.

Thanks,
Chris

[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 
---
 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, >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 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 
---
 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 
---
 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

[PATCH 4/6] qtest: factor out qtest_install_gpio_out_intercepts

[Chris Laplante]

Simplify the code a bit.

Signed-off-by: Chris Laplante 
---
 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 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 
---
 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, >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 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 
---
 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(>gpio), dev_soc, NULL);
+qdev_pass_gpios(DEVICE(>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), >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

Re: Addition of qtest_irq_intercept_out_named, or modify qtest_irq_interrupt_out?

[Chris Laplante]

[sorry, resending because I forgot to reply-all]

> qtest_irq_intercept_out() takes a QOM path argument. Whether it is
> a named IRQ or not should be irrelevant at this layer.

I'm a little confused about the QOM path. Currently the code in 
qtest_process_command() is calling object_resolve_path, but (as I understand 
it) a named IRQ is not an object, it's a property. So I either need to find a 
flavor of object_resolve_path that can also handle properties, or I need to add 
a parameter to qtest_irq_intercept_out for specifying the name. Does that sound 
right?

Thanks,
Chris

Re: Emulation of 'System OFF' mode in ARM nRF51 SoCs

[Chris Laplante]

Hi Peter,

> > 
> > Working on adding this now. One question - if the CPU is off (via 
> > arm_set_cpu_off), will the 'DETECT' IRQ I add to nrf51_gpio.c still fire?
> 
> 
> Yes. The only thing that turning the CPU off affects is
> the CPU -- all the rest of the devices in the system
> continue to behave as normal.
> 

Excellent, thanks.

> > There is no power management IC or device in this system.
> 
> 
> The manual says there is: chapter 12 describes the
> power management and the registers involved, which are
> in the POWER peripheral part of the SoC, starting at
> 0x4000_. QEMU just doesn't model that yet.
>

Ah yes, my mistake. I am working on implementing the POWER peripheral (along 
with CLOCK and MPU, since they are overlapped at 0x4000_). It will be 
called nrf51_cpm.

I have modified the GPIO peripheral to support the DETECT mechanism, and so far 
it seems to work. I have also locally implemented a version of 
qtest_irq_intercept_out which supports named GPIO out interrupts, and I have a 
qtest that confirms that basic functionality of DETECT is working. 

Hoping to have patches to send in the next couple weeks.

Thanks,
Chris

Addition of qtest_irq_intercept_out_named, or modify qtest_irq_interrupt_out?

[Chris Laplante]

Hello all,

I have a test case that needs to intercept a named GPIO out interrupt. 
qtest_irq_intercept_out doesn't support this currently. I would like to send a 
patch to add this functionality. Does anyone have a preference if I implement 
it is a new function (qtest_irq_intercept_out_named), vs add the functionality 
to qtest_irq_intercept_out in the form of an optional additional parameter?

Thanks,
Chris

Re: Emulation of 'System OFF' mode in ARM nRF51 SoCs

[Chris Laplante]

Hi Peter,

> The reference manual is very unclear about what this "emulated
> system off" mode actually does. I think that implementing
> real "system off" is probably simpler. For that you should be able
> to implement it something like this:
> 
> (1) the power management device implements the SYSTEMOFF register
> to call arm_set_cpu_off() when a 1 is written
> (2) make sure the GPIO device implements DETECT as a GPIO output
> signal, ie an outbound qemu_irq (if we don't do this already
> the functionality will need to be added to the device model)

Working on adding this now. One question - if the CPU is off (via 
arm_set_cpu_off), will the 'DETECT' IRQ I add to nrf51_gpio.c still fire? 

> (3) similarly for ANADETECT from the LPCOMP device
> (4) Wire those qemu_irq GPIO outputs up to inputs on the
> power management device. When the power management device
> sees those signals go high and the CPU is in system off mode,
> it should trigger the reset of the CPU by calling
> arm_set_cpu_on_and_reset().
>

There is no power management IC or device in this system. Is it something I can 
implement in nrf51 directly, or will I need to do it in machine context? I 
guess what I'm asking is, if the CPU is off, can it still wake itself up?

Thanks,
Chris

Re: Emulation of 'System OFF' mode in ARM nRF51 SoCs

[Chris Laplante]

Hi Phil,

> What problem are you getting with a single CPU?
> The "arm/arm-powerctl.h" API should work well.
> If you scheduled a timer, I expect it to awake
> your CPU on expiration. You can also use a QMP
> command to toggle a GPIO and trigger an IRQ.
> 
> You can use the qtest API to test your code,
> see some tests in tests/qtest/ using:
> - qtest_set_irq_in()
> - qtest_qom_set_bool() for GPIO

Thanks for the tips - I am working on implementing this now!

Chris

Emulation of 'System OFF' mode in ARM nRF51 SoCs

[Chris Laplante]

Hi all,

I am working on improving nRF51 emulation. Specifically I want to implement the 
special "System OFF" mode. System OFF is a power saving mode. In this mode, the 
system can only be woken up by a reset or a handful of peripherals (most 
notably, GPIO via high/low level detection on configured pins). System reset is 
triggered upon wakeup.

I've been digging into the QEMU mailing list and source code and have come to 
the conclusion that deep sleep and low power modes are not implemented. There 
seems to be support for turning off ARM CPU cores, e.g. as used by imx6_src.c. 
But that doesn't apply here because I only have one CPU. 

So ultimately what I think I will try to implement is what the nRF51 reference 
manual calls "Emulated System OFF mode". From the reference manual:

If the device is in debug interface mode, System OFF will be emulated to 
secure 
that all required resources needed for debugging are available during 
System OFF... 
Since the CPU is kept on in emulated System OFF mode, it is recommended 
to add an infinite loop directly after entering System OFF, to prevent the 
CPU from 
executing code that normally should not be executed.

Does anyone have any guidance on how to implement this? I don't particularly 
care about fidelity. As long as a GPIO level trigger can break the CPU out of 
the infinite loop (which the reference manual tells users to add) and jump into 
the reset vector, it will be good enough for my use. I don't really care about 
masking out other interrupt sources, for example.

Thanks,
Chris