Re: [PATCH v1] nrf51: Fix last GPIO CNF address

[Cameron Esfahani via <qemu-devel@nongnu.org>]

I'm not burying anything.  This patch is stand alone and all the tests do work.  They work with or without Cedric's nee Andrew's patch.  But, if some derivative of that patch is eventually implemented, something needs to be done for this NRF51 gpio qtest to work.

There are two possibilities for the following qtest in microbit-test.c:

>     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END) & 0x01;
>     g_assert_cmpuint(actual, ==, 0x01);


1 - The code is purposefully reading 32-bits from an unaligned address which only partially refers to a documented register.  And the only reason this code works is because that 32-bit value is turned into a 8-bit read.  And if that's the case, then someone should update a comment in the test to indicate that this is being done purposefully.
2 - NRF51_GPIO_REG_CNF_END is incorrectly set to be 0x77F.  Looking at the documentation for this chip, the last defined CNF register starts at 0x77C.

The NRF51 doesn't support unaligned accesses, so I assume that possibility 1 isn't true.

So, is NRF51_GPIO_REG_CNF_END supposed to refer to a valid register, or the end of the implementation space?

If it's the former, then it should be adjusted to 0x77c and possibly update the below code in nrf51_gpio.c (line 156):

>     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END:


to become

>     case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END + 3:

if unaligned access are expected to work.  But, considering the NRF51 doesn't support unaligned accesses, I don't think this appropriate.

If it's the latter, then the test cases in microbit-test.c should be updated to something like the following:

>     actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_END - 3) & 0x01;
>     g_assert_cmpuint(actual, ==, 0x01);


Cameron Esfahani
dirty@apple.com

"Americans are very skilled at creating a custom meaning from something that's mass-produced."

Ann Powers


> On Apr 7, 2020, at 1:44 AM, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Considering NRF51 doesn't support unaligned accesses, the simplest fix
>> is to actually set NRF51_GPIO_REG_CNF_END to the start of the last valid
>> CNF register: 0x77c.
> 
> NAck. You are burying bugs deeper. This test has to work.
> 
> What would be helpful is qtests with unaligned accesses (expected to work) such your USB XHCI VERSION example.