On Tue, 7 Apr 2020 at 10:09, Cameron Esfahani <di...@apple.com> 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 <j...@jms.id.au> > > > Cameron Esfahani > di...@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é <phi...@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. >
