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

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

[Jimmy Brisson]

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

[Peter Maydell]

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

[Jimmy Brisson]

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

[Jimmy Brisson]

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