| Message ID | 20190124141147.8416-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/microbit-test: Add tests for nRF51 NVMC | expand |
On 2019-01-24 15:11, Stefan Hajnoczi wrote: > From: Steffen Görtz <contrib@steffen-goertz.de> > > Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > This resolves the conflict with Julia's UART test series due to > global_qtest removal. > > tests/microbit-test.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 108 insertions(+) > > diff --git a/tests/microbit-test.c b/tests/microbit-test.c > index 3bad947b6c..04e199ec33 100644 > --- a/tests/microbit-test.c > +++ b/tests/microbit-test.c > @@ -21,6 +21,7 @@ > #include "hw/arm/nrf51.h" > #include "hw/char/nrf51_uart.h" > #include "hw/gpio/nrf51_gpio.h" > +#include "hw/nvram/nrf51_nvm.h" > #include "hw/timer/nrf51_timer.h" > #include "hw/i2c/microbit_i2c.h" > > @@ -156,6 +157,112 @@ static void test_microbit_i2c(void) > qtest_quit(qts); > } > > +#define FLASH_SIZE (256 * NRF51_PAGE_SIZE) > + > +static void fill_and_erase(QTestState *qts, hwaddr base, hwaddr size, > + uint32_t address_reg) > +{ > + hwaddr i; > + > + /* Erase Page */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + address_reg, base); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + /* Check memory */ > + for (i = 0; i < size / 4; i++) { > + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, 0xFFFFFFFF); > + } > + > + /* Fill memory */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); > + for (i = 0; i < size / 4; i++) { > + qtest_writel(qts, base + i * 4, i); > + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, i); > + } > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > +} > + > +static void test_nrf51_nvmc(void) > +{ > + uint32_t value; > + hwaddr i; > + QTestState *qts = qtest_init("-M microbit"); > + > + /* Test always ready */ > + value = qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_READY); > + g_assert_cmpuint(value & 0x01, ==, 0x01); > + > + /* Test write-read config register */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x03); > + g_assert_cmpuint(qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), > + ==, 0x03); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + g_assert_cmpuint(qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), > + ==, 0x00); > + > + /* Test PCR0 */ > + fill_and_erase(qts, NRF51_FLASH_BASE, NRF51_PAGE_SIZE, > + NRF51_NVMC_ERASEPCR0); > + fill_and_erase(qts, NRF51_FLASH_BASE + NRF51_PAGE_SIZE, > + NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR0); > + > + /* Test PCR1 */ > + fill_and_erase(qts, NRF51_FLASH_BASE, NRF51_PAGE_SIZE, > + NRF51_NVMC_ERASEPCR1); > + fill_and_erase(qts, NRF51_FLASH_BASE + NRF51_PAGE_SIZE, > + NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR1); > + > + /* Erase all */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); > + for (i = 0; i < FLASH_SIZE / 4; i++) { > + qtest_writel(qts, NRF51_FLASH_BASE + i * 4, i); > + g_assert_cmpuint(qtest_readl(qts, NRF51_FLASH_BASE + i * 4), ==, i); > + } > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + for (i = 0; i < FLASH_SIZE / 4; i++) { > + g_assert_cmpuint(qtest_readl(qts, NRF51_FLASH_BASE + i * 4), > + ==, 0xFFFFFFFF); > + } > + > + /* Erase UICR */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { > + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), > + ==, 0xFFFFFFFF); > + } > + > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); > + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { > + qtest_writel(qts, NRF51_UICR_BASE + i * 4, i); > + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), ==, i); > + } > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { > + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), > + ==, 0xFFFFFFFF); > + } > + > + qtest_quit(qts); > +} > + > static void test_nrf51_gpio(void) > { > size_t i; > @@ -392,6 +499,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/nvmc", test_nrf51_nvmc); > qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); > qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c); Thank you for getting rid of global_qtest stuff also here! Acked-by: Thomas Huth <thuth@redhat.com>
On Thu, 24 Jan 2019 at 14:11, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > From: Steffen Görtz <contrib@steffen-goertz.de> > > Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > This resolves the conflict with Julia's UART test series due to > global_qtest removal. > > tests/microbit-test.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 108 insertions(+) Applied to target-arm.next, thanks. -- PMM
On Thu, 24 Jan 2019 at 14:11, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > From: Steffen Görtz <contrib@steffen-goertz.de> > > Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > This resolves the conflict with Julia's UART test series due to > global_qtest removal. > > tests/microbit-test.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 108 insertions(+) > > diff --git a/tests/microbit-test.c b/tests/microbit-test.c > index 3bad947b6c..04e199ec33 100644 > --- a/tests/microbit-test.c > +++ b/tests/microbit-test.c > @@ -21,6 +21,7 @@ > #include "hw/arm/nrf51.h" > #include "hw/char/nrf51_uart.h" > #include "hw/gpio/nrf51_gpio.h" > +#include "hw/nvram/nrf51_nvm.h" > #include "hw/timer/nrf51_timer.h" > #include "hw/i2c/microbit_i2c.h" > > @@ -156,6 +157,112 @@ static void test_microbit_i2c(void) > qtest_quit(qts); > } > > +#define FLASH_SIZE (256 * NRF51_PAGE_SIZE) > + > +static void fill_and_erase(QTestState *qts, hwaddr base, hwaddr size, > + uint32_t address_reg) > +{ > + hwaddr i; > + > + /* Erase Page */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); > + qtest_writel(qts, NRF51_NVMC_BASE + address_reg, base); > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); > + > + /* Check memory */ > + for (i = 0; i < size / 4; i++) { > + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, 0xFFFFFFFF); > + } > + > + /* Fill memory */ > + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); > + for (i = 0; i < size / 4; i++) { > + qtest_writel(qts, base + i * 4, i); > + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, i); This assertion fails on PPC64 hosts: ERROR:/home/pm215/qemu/tests/microbit-test.c:181:fill_and_erase: assertion failed (qtest_readl(qts, base + i * 4) == i): (16777216 == 1) I suspect an endianness issue, given that 16777216 is 0x0100_0000. The test looks OK, though -- a word write ought to come back the same on a word read -- so maybe it's in the implementation? thanks -- PMM
On Mon, 28 Jan 2019 at 19:01, Peter Maydell <peter.maydell@linaro.org> wrote: > This assertion fails on PPC64 hosts: > ERROR:/home/pm215/qemu/tests/microbit-test.c:181:fill_and_erase: > assertion failed (qtest_readl(qts, base + i * 4) == i): (16777216 == > 1) > > I suspect an endianness issue, given that 16777216 is 0x0100_0000. > The test looks OK, though -- a word write ought to come back > the same on a word read -- so maybe it's in the implementation? The problem here is that the read path goes directly to the underlying host memory, but the write path goes via the flash_write() function, and the two disagree about the representation of the underlying storage. I think that declaring s->storage to be a uint32_t* is a mistake -- the underlying RAM needs to be treated as a byte array, not as an array of host-endian-order 32-bit words (because the direct-to-RAM path effectively treats it as a byte array). Then, since the flash_write() function is part of a DEVICE_LITTLE_ENDIAN MemoryRegionOps, its 'value' argument is a 32-bit LE value, and it should do the modification of s->storage via something like oldval = ldl_le_p(s->storage + offset); oldval &= value; stl_le_p(s->storage + offset, oldval); Another reference to s->storage also simplifies when we fix its type: s->storage + value / 4 becomes just s->storage + value. NB: untested! thanks -- PMM
On Mon, 28 Jan 2019 at 19:01, Peter Maydell <peter.maydell@linaro.org> wrote: > This assertion fails on PPC64 hosts: > ERROR:/home/pm215/qemu/tests/microbit-test.c:181:fill_and_erase: > assertion failed (qtest_readl(qts, base + i * 4) == i): (16777216 == > 1) For the moment I have dropped patches hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories arm: Instantiate NRF51 special NVM's and NVMC tests/microbit-test: Add tests for nRF51 NVMC from target-arm.next (but have kept memory: add memory_region_flush_rom_device() from the nvmc series). thanks -- PMM
On Tue, Jan 29, 2019 at 7:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 28 Jan 2019 at 19:01, Peter Maydell <peter.maydell@linaro.org> wrote: > > This assertion fails on PPC64 hosts: > > ERROR:/home/pm215/qemu/tests/microbit-test.c:181:fill_and_erase: > > assertion failed (qtest_readl(qts, base + i * 4) == i): (16777216 == > > 1) > > > > I suspect an endianness issue, given that 16777216 is 0x0100_0000. > > The test looks OK, though -- a word write ought to come back > > the same on a word read -- so maybe it's in the implementation? > > The problem here is that the read path goes directly to the > underlying host memory, but the write path goes via the > flash_write() function, and the two disagree about the > representation of the underlying storage. > > I think that declaring s->storage to be a uint32_t* is a mistake -- > the underlying RAM needs to be treated as a byte array, not as an > array of host-endian-order 32-bit words (because the direct-to-RAM > path effectively treats it as a byte array). Then, since the > flash_write() function is part of a DEVICE_LITTLE_ENDIAN > MemoryRegionOps, its 'value' argument is a 32-bit LE value, and > it should do the modification of s->storage via something like > oldval = ldl_le_p(s->storage + offset); > oldval &= value; > stl_le_p(s->storage + offset, oldval); > > Another reference to s->storage also simplifies when we fix its > type: s->storage + value / 4 becomes just s->storage + value. > > NB: untested! Thanks for looking into this! I'll retest on a big-endian host and send a new revision. Stefan
diff --git a/tests/microbit-test.c b/tests/microbit-test.c index 3bad947b6c..04e199ec33 100644 --- a/tests/microbit-test.c +++ b/tests/microbit-test.c @@ -21,6 +21,7 @@ #include "hw/arm/nrf51.h" #include "hw/char/nrf51_uart.h" #include "hw/gpio/nrf51_gpio.h" +#include "hw/nvram/nrf51_nvm.h" #include "hw/timer/nrf51_timer.h" #include "hw/i2c/microbit_i2c.h" @@ -156,6 +157,112 @@ static void test_microbit_i2c(void) qtest_quit(qts); } +#define FLASH_SIZE (256 * NRF51_PAGE_SIZE) + +static void fill_and_erase(QTestState *qts, hwaddr base, hwaddr size, + uint32_t address_reg) +{ + hwaddr i; + + /* Erase Page */ + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); + qtest_writel(qts, NRF51_NVMC_BASE + address_reg, base); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + /* Check memory */ + for (i = 0; i < size / 4; i++) { + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, 0xFFFFFFFF); + } + + /* Fill memory */ + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); + for (i = 0; i < size / 4; i++) { + qtest_writel(qts, base + i * 4, i); + g_assert_cmpuint(qtest_readl(qts, base + i * 4), ==, i); + } + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); +} + +static void test_nrf51_nvmc(void) +{ + uint32_t value; + hwaddr i; + QTestState *qts = qtest_init("-M microbit"); + + /* Test always ready */ + value = qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_READY); + g_assert_cmpuint(value & 0x01, ==, 0x01); + + /* Test write-read config register */ + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x03); + g_assert_cmpuint(qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), + ==, 0x03); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + g_assert_cmpuint(qtest_readl(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), + ==, 0x00); + + /* Test PCR0 */ + fill_and_erase(qts, NRF51_FLASH_BASE, NRF51_PAGE_SIZE, + NRF51_NVMC_ERASEPCR0); + fill_and_erase(qts, NRF51_FLASH_BASE + NRF51_PAGE_SIZE, + NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR0); + + /* Test PCR1 */ + fill_and_erase(qts, NRF51_FLASH_BASE, NRF51_PAGE_SIZE, + NRF51_NVMC_ERASEPCR1); + fill_and_erase(qts, NRF51_FLASH_BASE + NRF51_PAGE_SIZE, + NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR1); + + /* Erase all */ + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); + for (i = 0; i < FLASH_SIZE / 4; i++) { + qtest_writel(qts, NRF51_FLASH_BASE + i * 4, i); + g_assert_cmpuint(qtest_readl(qts, NRF51_FLASH_BASE + i * 4), ==, i); + } + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + for (i = 0; i < FLASH_SIZE / 4; i++) { + g_assert_cmpuint(qtest_readl(qts, NRF51_FLASH_BASE + i * 4), + ==, 0xFFFFFFFF); + } + + /* Erase UICR */ + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), + ==, 0xFFFFFFFF); + } + + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01); + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { + qtest_writel(qts, NRF51_UICR_BASE + i * 4, i); + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), ==, i); + } + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01); + qtest_writel(qts, NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00); + + for (i = 0; i < NRF51_UICR_SIZE / 4; i++) { + g_assert_cmpuint(qtest_readl(qts, NRF51_UICR_BASE + i * 4), + ==, 0xFFFFFFFF); + } + + qtest_quit(qts); +} + static void test_nrf51_gpio(void) { size_t i; @@ -392,6 +499,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/nvmc", test_nrf51_nvmc); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);