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

[edk2-platforms PATCH v2] 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 allows matching any address for the pci node, thus working
with both legacy and new device trees.

[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>
---

v1->v2: Match both pci@1,0 and pci@0,0

I chose to remove the address altogether, since there is only one pci
node for the Raspberry Pi 4 and thus no ambiguity. Let me know if you
prefer explicitly matching pci@1,0 and pci@0,0.

 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..cbbc2ad30d 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");
   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\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\n"));
       return EFI_NOT_FOUND;
     }
   }

base-commit: ae75c51f27e21036b6ee021a2d5b9f365f951413
-- 
2.37.3

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

[Jeremy Linton]

Hi,

On 10/6/22 12:46, Adrien Thierry wrote:
> 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 allows matching any address for the pci node, thus working
> with both legacy and new device trees.
> 

Thanks a lot for looking into this! This looks fine to me, the only 
thing that poped out was that the uefi checkpatch complained about the 
linux log in the commit message, but I think that should be ok.

so:

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

and I also did some basic testing on a rpi4's so:

Tested-by: Jeremy Linton <jeremy.linton@arm.com>

too.

Thanks again!

> [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>
> ---
> 
> v1->v2: Match both pci@1,0 and pci@0,0
> 
> I chose to remove the address altogether, since there is only one pci
> node for the Raspberry Pi 4 and thus no ambiguity. Let me know if you
> prefer explicitly matching pci@1,0 and pci@0,0.
> 
>   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..cbbc2ad30d 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");
>     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\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\n"));
>         return EFI_NOT_FOUND;
>       }
>     }
> 
> base-commit: ae75c51f27e21036b6ee021a2d5b9f365f951413

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

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 22:39, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 10/6/22 12:46, Adrien Thierry wrote:
> > 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 allows matching any address for the pci node, thus working
> > with both legacy and new device trees.
> >
>
> Thanks a lot for looking into this! This looks fine to me, the only
> thing that poped out was that the uefi checkpatch complained about the
> linux log in the commit message, but I think that should be ok.
>
> so:
>
> Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>
>
> and I also did some basic testing on a rpi4's so:
>
> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
>
> too.
>
> Thanks again!
>

Pushed as 3b889620a53c..ad00518399fc

Thanks all,

> > [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>
> > ---
> >
> > v1->v2: Match both pci@1,0 and pci@0,0
> >
> > I chose to remove the address altogether, since there is only one pci
> > node for the Raspberry Pi 4 and thus no ambiguity. Let me know if you
> > prefer explicitly matching pci@1,0 and pci@0,0.
> >
> >   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..cbbc2ad30d 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");
> >     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\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\n"));
> >         return EFI_NOT_FOUND;
> >       }
> >     }
> >
> > base-commit: ae75c51f27e21036b6ee021a2d5b9f365f951413
>