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

[Joel Stanley]

On Tue, 7 Apr 2020 at 10:09, Cameron Esfahani <address@hidden> wrote:
>
> 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);


Your reasoning is sound, thanks for writing it out. I would be happy
to see your patch land.

Reviewed-by: Joel Stanley <address@hidden>


>
>
> Cameron Esfahani
> address@hidden
>
> "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é <address@hidden> 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.
>