[edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[Adrien Thierry]

This patch series does a few fixes in the SyncPcie() function, more
specifically in the logic that deletes the pci node to prevent Linux from
resetting the XHCI controller.

Adrien Thierry (2):
  Platform/RaspberryPi: fix pci DT node address in SyncPcie()
  Platform/RaspberryPi: delete usb node instead of pci in SyncPcie()

 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: e55f0527dde48a5f139c1b8f35acc4e6b59dd794
-- 
2.37.3

[edk2-platforms PATCH 1/2] Platform/RaspberryPi: fix pci DT node address in SyncPcie()

[Adrien Thierry]

To make sure the XHCI controller does not get reset by Linux in DT mode,
we remove its pci parent node from the device tree. However, the pci
node address has been updated in the Raspberry Pi 4 device tree [1] and
no longer matches the one we are trying to remove in SyncPcie(). This
results in the XHCI controller actually being reset by Linux, which
leads to errors during USB initialization:

[    3.563963] xhci_hcd 0000:01:00.0: xHCI Host Controller
[    3.569538] xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 1
[    3.577452] xhci_hcd 0000:01:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000040000000890
[    3.587725] xhci_hcd 0000:01:00.0: xHCI Host Controller
[    3.593115] xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 2
[    3.600693] xhci_hcd 0000:01:00.0: Host supports USB 3.0 SuperSpeed
[    3.608106] hub 1-0:1.0: USB hub found
[    3.612026] hub 1-0:1.0: 1 port detected
[    3.616819] hub 2-0:1.0: USB hub found
[    3.620726] hub 2-0:1.0: 4 ports detected
[    3.875902] usb 1-1: new high-speed USB device number 2 using xhci_hcd
[    4.008123] usb 1-1: device descriptor read/64, error -71
[    4.256088] usb 1-1: device descriptor read/64, error -71
[    4.495882] usb 1-1: new high-speed USB device number 3 using xhci_hcd
[    4.628111] usb 1-1: device descriptor read/64, error -71
[    4.872083] usb 1-1: device descriptor read/64, error -71
[    5.407888] usb 1-1: new high-speed USB device number 4 using xhci_hcd
[    6.023964] xhci_hcd 0000:01:00.0: Setup ERROR: setup address command for slot 1.
[    6.239977] xhci_hcd 0000:01:00.0: Setup ERROR: setup address command for slot 1.

This patch updates the pci node address (and usb node address in the
error messages) to match those found in the RPi4 device tree.

[1] https://lore.kernel.org/all/20210831125843.1233488-1-nsaenzju@redhat.com/

Fixes: efff29cdcdb7 ("Platform/RaspberryPi: Always use non translating DMA in DT mode")
Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index e72d132b18..55c9d185fc 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -388,14 +388,14 @@ SyncPcie (
    * triggering the mailbox by removing the node.
    */
 
-  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@1,0");
+  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@0,0");
   if (Node < 0) {
     // This can happen on CM4/etc which doesn't have an onboard XHCI
-    DEBUG ((DEBUG_INFO, "%a: failed to locate /scb/pcie@7d500000/pci@1/usb@1\n", __FUNCTION__));
+    DEBUG ((DEBUG_INFO, "%a: failed to locate /scb/pcie@7d500000/pci@0/usb@0\n", __FUNCTION__));
   } else {
     Retval = fdt_del_node (mFdtImage, Node);
     if (Retval != 0) {
-      DEBUG ((DEBUG_ERROR, "Failed to remove /scb/pcie@7d500000/pci@1/usb@1\n"));
+      DEBUG ((DEBUG_ERROR, "Failed to remove /scb/pcie@7d500000/pci@0/usb@0\n"));
       return EFI_NOT_FOUND;
     }
   }
-- 
2.37.3

[edk2-platforms PATCH 2/2] Platform/RaspberryPi: delete usb node instead of pci in SyncPcie()

[Adrien Thierry]

In SyncPcie(), the pci node is removed from the device tree to make sure
the XHCI controller is not reset by Linux in DT mode. However, we should
only remove the usb child node and not the whole pci node. Removing the
whole pci node prevents Linux to bypass XHCI handoff for the Raspberry
Pi 4 [1]. Moreover, removing the usb node seems to have been the
original intent according to the error messages shown if the node is not
detected.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/host/pci-quirks.c#L1258

Fixes: efff29cdcdb7 ("Platform/RaspberryPi: Always use non translating DMA in DT mode")
Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index 55c9d185fc..26334e50b8 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -388,7 +388,7 @@ SyncPcie (
    * triggering the mailbox by removing the node.
    */
 
-  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@0,0");
+  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@0,0/usb@0,0");
   if (Node < 0) {
     // This can happen on CM4/etc which doesn't have an onboard XHCI
     DEBUG ((DEBUG_INFO, "%a: failed to locate /scb/pcie@7d500000/pci@0/usb@0\n", __FUNCTION__));
-- 
2.37.3

Re: [edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[Jeremy Linton]

On 9/29/22 14:53, Adrien Thierry wrote:
> This patch series does a few fixes in the SyncPcie() function, more
> specifically in the logic that deletes the pci node to prevent Linux from
> resetting the XHCI controller.

Hmm, that code not being exactly right isn't surprising, I went through 
about a dozen revisions looking for the one that fixed it consistently 
and in the end this version worked with some older kernels (and likely 
dt) but doesn't work with any of the recent ones.

But... I think I found an actual fix a couple months ago while testing 
the DT->SMCCC pci config space code. Which is to update the ranges 
property as well. With that change the firmware can reset the XHCI 
controller in recent Linux's, so there isn't a need to remove the XHCI node.


There is a copy of the patch hiding on my github

https://github.com/jlinton/edk2-platforms/commit/50540bd24f93b633c3597b5dc58c1a1a3b49bf7f#diff-373e67aaa16dd9ac2428d5acc3d73ef218b2ed6d24f3350d5af558cba03cf5adR378

along with a change to update the compatible property to 
pci-host-smc-generic and remove the ranges property which should be 
ignored... :)

If you just add the range tweak, does that fix the XHCI config in your 
setup too?


I really need to start getting many of those old/stale patches cleaned 
up and merged, but its not been a high priority.


> 
> Adrien Thierry (2):
>    Platform/RaspberryPi: fix pci DT node address in SyncPcie()
>    Platform/RaspberryPi: delete usb node instead of pci in SyncPcie()
> 
>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> 
> base-commit: e55f0527dde48a5f139c1b8f35acc4e6b59dd794

Re: [edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[Adrien Thierry]

Hi Jeremy,

> If you just add the range tweak, does that fix the XHCI config in your setup
> too?

I tested applying the range tweak in your patch, unfortunately it doesn't
seem to work on my setup. I'm still getting "usb 1-1: device descriptor
read/64, error -71" errors.

Here's my SyncPcie function with the range tweak applied [1]

I'm running upstream linux 6.0-rc6 with the downstream device tree
provided in [2]

Thank you,
Adrien

[1] http://pastebin.test.redhat.com/1075875
[2] https://github.com/pftf/RPi4/releases/tag/v1.33

Re: [edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[Jeremy Linton]

Hi,

On 9/30/22 13:47, Adrien Thierry wrote:
> Hi Jeremy,
> 
>> If you just add the range tweak, does that fix the XHCI config in your setup
>> too?
> 
> I tested applying the range tweak in your patch, unfortunately it doesn't
> seem to work on my setup. I'm still getting "usb 1-1: device descriptor > read/64, error -71" errors.

That is unfortunate. Which revision/how much RAM? Can you paste the 
before/after kernel/pci output like:

] brcm-pcie fd500000.pcie: host bridge /scb/pcie@7d500000 ranges:
] brcm-pcie fd500000.pcie:   No bus range found for /scb/pcie@7d500000, 
using [bus 00-ff]
] brcm-pcie fd500000.pcie:      MEM 0x0600000000..0x0603ffffff -> 
0x00f8000000
] brcm-pcie fd500000.pcie:   IB MEM 0x0000000000..0x00bfffffff -> 
0x0000000000
] brcm-pcie fd500000.pcie: PCI host bridge to bus 0000:00
] pci_bus 0000:00: root bus resource [bus 00-ff]
] pci_bus 0000:00: root bus resource [mem 0x600000000-0x603ffffff] (bus 
address [0xf8000000-0xfbffffff])

I tested it on a C0 with 8G and a B0 with 4G, and it seems to work on 
both, although the C0 might have been misbehaving a bit (likely 
unrelated). In both cases I swapped in the latest Pi foundation DT 
(https://github.com/raspberrypi/firmware/blob/master/boot/bcm2711-rpi-4-b.dtb) 
which uses the address you suggest, and it still seems to work either 
way (with or without the reset). If we reroll the PFTF firmware it is 
usually with the latest pi foundation DTs.

So the first patch makes sense, but I think it should probably be 
checking both DT property names (pci0,0 and pci1,0) since we probably 
want maximum compatibility with differing DT's the user might swap in, 
and the upstream linux change which changes the node from 1,0 to 0,0 is 
from august of last year, so not old at all..

The second one, I'm less sure about, since the primary thing your 
changing (AFAIK) is whether the XHCI BIOS handoff is being checked, and 
since EDK2 supports the hand-off and Linux doesn't throw the XHCI bios 
failure message I think we actually want to leave that in place. If you 
have debugging turned on, the XHCI/LegacyBios ownership control is the 
message you see during exit boot services that says 
"XhciClearBiosOwnership: called to clear BIOS ownership". I suspect the 
kernel patch is yet another case of "UBOOT and/or the pi foundation 
firmware is broken so we fixed the kernel"

Do you see a difference with/without the second patch?




> 
> Here's my SyncPcie function with the range tweak applied [1]
> 
> I'm running upstream linux 6.0-rc6 with the downstream device tree
> provided in [2]
> 
> Thank you,
> Adrien
> 
> [1] http://pastebin.test.redhat.com/1075875
> [2] https://github.com/pftf/RPi4/releases/tag/v1.33
> 

Re: [edk2-platforms PATCH 0/2] Platform/RaspberryPi: SyncPcie() fixes

[Adrien Thierry]

Hi Jeremy,

> That is unfortunate. Which revision/how much RAM? Can you paste the
> before/after kernel/pci output like:

My RPi4 is a 8GB revision d03114.

Here's the pci output before applying your ranges tweak:
[    3.697773] brcm-pcie fd500000.pcie: host bridge /scb/pcie@7d500000 ranges:
[    3.706020] brcm-pcie fd500000.pcie:   No bus range found for /scb/pcie@7d500000, using [bus 00-ff]
[    3.716271] brcm-pcie fd500000.pcie:      MEM 0x0600000000..0x063fffffff -> 0x00c0000000
[    3.726229] brcm-pcie fd500000.pcie:   IB MEM 0x0000000000..0x00bfffffff -> 0x0000000000
[    3.737357] brcm-pcie fd500000.pcie: PCI host bridge to bus 0000:00
[    3.743891] pci_bus 0000:00: root bus resource [bus 00-ff]
[    3.749530] pci_bus 0000:00: root bus resource [mem 0x600000000-0x63fffffff] (bus address [0xc0000000-0xffffffff])

And after:
[    3.781758] brcm-pcie fd500000.pcie: host bridge /scb/pcie@7d500000 ranges:
[    3.789907] brcm-pcie fd500000.pcie:   No bus range found for /scb/pcie@7d500000, using [bus 00-ff]
[    3.800230] brcm-pcie fd500000.pcie:      MEM 0x0600000000..0x0603ffffff -> 0x00f8000000
[    3.809721] brcm-pcie fd500000.pcie:   IB MEM 0x0000000000..0x00bfffffff -> 0x0000000000
[    3.828359] brcm-pcie fd500000.pcie: PCI host bridge to bus 0000:00
[    3.835812] pci_bus 0000:00: root bus resource [bus 00-ff]
[    3.845651] pci_bus 0000:00: root bus resource [mem 0x600000000-0x603ffffff] (bus address [0xf8000000-0xfbffffff])

I tested with the latest binaries and DT from
https://github.com/raspberrypi/firmware (master), same issue with the USB.

Here's the firmware version I'm running (got that from Raspbian), are you
using a different/more recent one?

$ vcgencmd version
Aug 26 2022 14:03:16 
Copyright (c) 2012 Broadcom
version 102f1e848393c2112206fadffaaf86db04e98326 (clean) (release) (start)

> So the first patch makes sense, but I think it should probably be
> checking both DT property names (pci0,0 and pci1,0) since we probably
> want maximum compatibility with differing DT's the user might swap in,
> and the upstream linux change which changes the node from 1,0 to 0,0 is
> from august of last year, so not old at all..

Sounds good, I can submit a v2 handling both variants

> The second one, I'm less sure about, since the primary thing your
> changing (AFAIK) is whether the XHCI BIOS handoff is being checked, and
> since EDK2 supports the hand-off and Linux doesn't throw the XHCI bios
> failure message I think we actually want to leave that in place. If you
> have debugging turned on, the XHCI/LegacyBios ownership control is the
> message you see during exit boot services that says
> "XhciClearBiosOwnership: called to clear BIOS ownership". I suspect the
> kernel patch is yet another case of "UBOOT and/or the pi foundation
> firmware is broken so we fixed the kernel"
> 
> Do you see a difference with/without the second patch?

Thanks for explaining this. I can't see a difference with/without the
second patch. AFAICT the hand-off check seems to execute without issue on
Linux. It makes sense to me to drop this second patch since the hand-off
is supported by EDK2.

Adrien