an547: Correct typo that swaps ahb and apb peripherals

[PATCH v1] an547: Correct typo that swaps ahb and apb peripherals

Posted by Jimmy Brisson 2 years, 3 months ago

Turns out that this manifests in being unable to configure
the ethernet access permissions, as the IotKitPPC looks
these up by name.

With this fix, eth is configurable

Signed-off-by: Jimmy Brisson <jimmy.brisson@linaro.org>
---
 hw/arm/mps2-tz.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index f40e854dec..3c6456762a 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -1030,7 +1030,7 @@ static void mps2tz_common_init(MachineState *machine)
     };
 
     const PPCInfo an547_ppcs[] = { {
-            .name = "apb_ppcexp0",
+            .name = "ahb_ppcexp0",
             .ports = {
                 { "ssram-mpc", make_mpc, &mms->mpc[0], 0x57000000, 0x1000 },
                 { "qspi-mpc", make_mpc, &mms->mpc[1], 0x57001000, 0x1000 },
@@ -1072,7 +1072,7 @@ static void mps2tz_common_init(MachineState *machine)
                 { "rtc", make_rtc, &mms->rtc, 0x4930b000, 0x1000 },
             },
         }, {
-            .name = "ahb_ppcexp0",
+            .name = "apb_ppcexp0",
             .ports = {
                 { "gpio0", make_unimp_dev, &mms->gpio[0], 0x41100000, 0x1000 },
                 { "gpio1", make_unimp_dev, &mms->gpio[1], 0x41101000, 0x1000 },
-- 
2.33.1

Re: [PATCH v1] an547: Correct typo that swaps ahb and apb peripherals

Posted by Peter Maydell 2 years, 2 months ago

On Wed, 2 Feb 2022 at 15:23, Jimmy Brisson <jimmy.brisson@linaro.org> wrote:
>
> Turns out that this manifests in being unable to configure
> the ethernet access permissions, as the IotKitPPC looks
> these up by name.
>
> With this fix, eth is configurable
>
> Signed-off-by: Jimmy Brisson <jimmy.brisson@linaro.org>

Can you explain the issue here in more detail, and maybe
provide a repro case ? The AN547 document definitely thinks
that APB PPC EXP 0 has the Memory Protection Controllers and
AHB PPC EXP 0 has the GPIO, USB and Ethernet devices:
https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/DAI0547B_SSE300_PLUS_U55_FPGA_for_mps3.pdf
(tables 6-2 to 6-4 on pages 35, 36).

thanks
-- PMM

Re: [PATCH v1] an547: Correct typo that swaps ahb and apb peripherals

Posted by Jimmy Brisson 2 years, 2 months ago

On Fri, 4 Feb 2022 at 11:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Feb 2022 at 15:23, Jimmy Brisson <jimmy.brisson@linaro.org> wrote:
> >
> > Turns out that this manifests in being unable to configure
> > the ethernet access permissions, as the IotKitPPC looks
> > these up by name.
> >
> > With this fix, eth is configurable
> >
> > Signed-off-by: Jimmy Brisson <jimmy.brisson@linaro.org>
>
> Can you explain the issue here in more detail, and maybe
> provide a repro case ?

Sure. I documented how I found this on my blog here:
https://theotherjimmy.github.io/blog/blog/2022-01-31-debug-qemu-eth.html
It's long, so I'll provide a quick summary here.

My reproducer is the following zephyr example:

```
west build -p auto -b mps3_an547_ns zephyr/samples/net/dhcpv4_client \
  -d build/mps3_an547_ns/net/dhcpv4_client -- \
  -DOVERLAY_CONFIG=overlay-smsc911x.conf \
  -DCONFIG_NET_QEMU_USER=y \
  -DCONFIG_BUILD_WITH_TFM=y \
  -DCOONFIG_TFM_IPC=y
```

As part of my debugging, I was using gdb on both the guest (tfm &
zephyr and host qemu).
I was able to reproduce this quickly by dumping the associated
smsc911x struct in guest gdb, and
watching the breakpoints in the `tz_ppc_read` function. Digging
through, it seems that when setting
up the peripheral access control in `iotkit_secctl_update_ppc_ap`, it
wrote the lowest 3 bits of the ap
and ignored the rest. This matches the expected behavior from the APB PPC EXT0.

Let me know if you have further questions,
Jimmy

> The AN547 document definitely thinks
> that APB PPC EXP 0 has the Memory Protection Controllers and
> AHB PPC EXP 0 has the GPIO, USB and Ethernet devices:
> https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/DAI0547B_SSE300_PLUS_U55_FPGA_for_mps3.pdf
> (tables 6-2 to 6-4 on pages 35, 36).
>
> thanks
> -- PMM

Re: [PATCH v1] an547: Correct typo that swaps ahb and apb peripherals

Posted by Jimmy Brisson 2 years, 2 months ago

On Thu, 10 Feb 2022 at 09:13, Jimmy Brisson <jimmy.brisson@linaro.org> wrote:
>
> On Fri, 4 Feb 2022 at 11:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 2 Feb 2022 at 15:23, Jimmy Brisson <jimmy.brisson@linaro.org> wrote:
> > >
> > > Turns out that this manifests in being unable to configure
> > > the ethernet access permissions, as the IotKitPPC looks
> > > these up by name.
> > >
> > > With this fix, eth is configurable
> > >
> > > Signed-off-by: Jimmy Brisson <jimmy.brisson@linaro.org>
> >
> > Can you explain the issue here in more detail, and maybe
> > provide a repro case ?
>
> Sure. I documented how I found this on my blog here:
> https://theotherjimmy.github.io/blog/blog/2022-01-31-debug-qemu-eth.html
> It's long, so I'll provide a quick summary here.
>
> My reproducer is the following zephyr example:
>
> ```
> west build -p auto -b mps3_an547_ns zephyr/samples/net/dhcpv4_client \
>   -d build/mps3_an547_ns/net/dhcpv4_client -- \
>   -DOVERLAY_CONFIG=overlay-smsc911x.conf \
>   -DCONFIG_NET_QEMU_USER=y \
>   -DCONFIG_BUILD_WITH_TFM=y \
>   -DCOONFIG_TFM_IPC=y
> ```
>
> As part of my debugging, I was using gdb on both the guest (tfm &
> zephyr and host qemu).
> I was able to reproduce this quickly by dumping the associated
> smsc911x struct in guest gdb, and
> watching the breakpoints in the `tz_ppc_read` function. Digging
> through, it seems that when setting
> up the peripheral access control in `iotkit_secctl_update_ppc_ap`, it
> wrote the lowest 3 bits of the ap
> and ignored the rest. This matches the expected behavior from the APB PPC EXT0.
>
> Let me know if you have further questions,
> Jimmy
>
>
> > The AN547 document definitely thinks
> > that APB PPC EXP 0 has the Memory Protection Controllers and
> > AHB PPC EXP 0 has the GPIO, USB and Ethernet devices:
> > https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/DAI0547B_SSE300_PLUS_U55_FPGA_for_mps3.pdf
> > (tables 6-2 to 6-4 on pages 35, 36).

I looked that up, and funny story, the _actual_ problem is that we're
missing the User AHB interface 0-3 peripherals.
Without this change, and adding empty peripherals e.g. {},  as
placeholders it seems to work correctly.

> >
> > thanks
> > -- PMM