[PATCH v2 0/1] Fix DT boot on rpi4

[PATCH v2 0/1] Fix DT boot on rpi4

[Jeremy Linton]

Some rpi4's haven't booted cleanly using DT for a few months.  The primarly
problem is that the XHCI_NOTIFY_RESET mailbox call doesn't seem to be able
to correctly read the pcie translation state after we have set it up. So,
while linux appears to be able to adjust the PCIe subsystem to the demands
of it's DT, the XHCI won't function because its looking for an untranslated
DMA range to access(?) its firmware image in main RAM.

So, lets apply the same non-translated 3G limit to DT's we hand off, as
well as remove the XHCI reset controller info so that linux/etc doesn't try
to reload the firmware. This latter tweak is a bit non obvious, but seems
to cure the general problem.

V1->V2:
	Also remove the pci/usb node to keep Linux from
	trying to call the XHCI_NOTIFY_RESET mailbox.

Jeremy Linton (1):
  Platform/RaspberryPi: Always use non translating DMA in DT mode

 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

-- 
2.13.7

[PATCH v2 1/1] Platform/RaspberryPi: Always use non translating DMA in DT mode

[Jeremy Linton]

One of the many issues with the PCIe on this platform is
its inbound DMA is either constrained to the lower 3G, or
on later SoC's a translation can be used. That translation
was problematic with some of the OS's expected to boot
on this platform. So, across the board a 3G DMA limit is
enforced during boot. This itself causes problems because
the later boards removed the SPI EEPROM used by the onboard
XHCI controller, instead favoring using a block of RAM to
load its firmware. Hence it is the lower level firmware's
responsibility via a mailbox call, to read the bridge
translation/configuration before telling the XHCI controller
where it can find its firmware.

Everything is great, except that it appears that Linux
after reprogramming the bridge to match the DT (when using
a translation) can't actually get the XHCI/quirk/reset to
function. Apparently, because the firmware only reads the
bridge configuration the first time its called(?). Worse
with just the DMA ranges corrected, the XHCI/QUIRK itself
then causes the controller to start having what appear
to be DMA issues.

So again, simplify the situation and make all DT's
provided by this firmware have a 3G DMA limit on the
PCIe bus. Then remove the ability for linux/etc to
trigger the quirk by remove the DT node attaching the
reset controller to the XHCI. The latter seems somewhat
questionable, since the DT/PCIe host bridge driver is
doing what appears to be a PERST which in theory
might then require a firmware reload, but at the
moment seems to work without.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index 0472d8ecf6..1a8fc7a134 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -334,6 +334,76 @@ CleanSimpleFramebuffer (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+SyncPcie (
+  VOID
+  )
+{
+#if (RPI_MODEL == 4)
+  INTN          Node;
+  INTN          Retval;
+  UINT32        DmaRanges[7];
+
+  Node = fdt_path_offset (mFdtImage, "pcie0");
+  if (Node < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'pcie0' alias\n", __FUNCTION__));
+    return EFI_NOT_FOUND;
+  }
+
+  // non translated 32-bit DMA window with a limit of 0xc0000000
+  DmaRanges[0] = cpu_to_fdt32 (0x02000000); 
+  DmaRanges[1] = cpu_to_fdt32 (0x00000000); 
+  DmaRanges[2] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[3] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[4] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[5] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[6] = cpu_to_fdt32 (0xc0000000);
+
+  DEBUG ((DEBUG_INFO, "%a: Updating PCIe dma-ranges\n",  __FUNCTION__));
+
+  // Match dma-ranges with the edk2+ACPI setup we are using.
+  // This works around a failure in linux to reset the XHCI correctly
+  // when in DT mode following the linux kernel reprogramming the PCIe
+  // subsystem to match the DT supplied inbound PCIe/DMA translation.
+  // It appears the lower level firmware honors whatever it reads
+  // during the first PCI/XHCI quirk call and uses that value until
+  // rebooted rather than re-reading it on every mailbox command.
+  Retval = fdt_setprop (mFdtImage, Node, "dma-ranges",
+                        DmaRanges,  sizeof DmaRanges);
+  if (Retval != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to update 'dma-ranges' property (%d)\n",
+      __FUNCTION__, Retval));
+    return EFI_NOT_FOUND;
+  }
+
+  // Now that we are always running without DMA translation, and with
+  // a 3G DMA limit, there shouldn't be a need to reset/reload
+  // the XHCI for it to operate. The possible problem is that the PCIe root
+  // port is itself being reset (by linux+DT), The rpi foundation claims
+  // this is needed as a pre-req to reloading the XHCI firmware, which
+  // also implies that a PCI fundamental reset should cause the XHCI itself
+  // to reset. For sure isn't happening fully, otherwise reloading the
+  // firmware would be mandatory. As it is, recent kernels actually start to
+  // have problems following the XHCI reset notification mailbox!
+  // So lets just stop the kernel from triggering the mailbox by removing
+  // the node.
+  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@1,0");
+  if (Node < 0) {
+    // This can happen on CM4/etc which doesn't have an onboard XHCI
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate /scb/pcie@7d500000/pci@1/usb@1\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"));
+      return EFI_NOT_FOUND;
+    }
+  }
+
+#endif
+  return EFI_SUCCESS;
+}
+
 /**
   @param  ImageHandle   of the loaded driver
   @param  SystemTable   Pointer to the System Table
@@ -431,6 +501,11 @@ FdtDxeInitialize (
     Print (L"Failed to update USB compatible properties: %r\n", Status);
   }
 
+  SyncPcie ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to update PCIe address ranges: %r\n", Status);
+  }
+
   DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage));
   Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
   if (EFI_ERROR (Status)) {
-- 
2.13.7

Re: [PATCH v2 1/1] Platform/RaspberryPi: Always use non translating DMA in DT mode

[Ard Biesheuvel]

On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> One of the many issues with the PCIe on this platform is
> its inbound DMA is either constrained to the lower 3G, or
> on later SoC's a translation can be used. That translation
> was problematic with some of the OS's expected to boot
> on this platform. So, across the board a 3G DMA limit is
> enforced during boot. This itself causes problems because
> the later boards removed the SPI EEPROM used by the onboard
> XHCI controller, instead favoring using a block of RAM to
> load its firmware. Hence it is the lower level firmware's
> responsibility via a mailbox call, to read the bridge
> translation/configuration before telling the XHCI controller
> where it can find its firmware.
>
> Everything is great, except that it appears that Linux
> after reprogramming the bridge to match the DT (when using
> a translation) can't actually get the XHCI/quirk/reset to
> function. Apparently, because the firmware only reads the
> bridge configuration the first time its called(?). Worse
> with just the DMA ranges corrected, the XHCI/QUIRK itself
> then causes the controller to start having what appear
> to be DMA issues.
>
> So again, simplify the situation and make all DT's
> provided by this firmware have a 3G DMA limit on the
> PCIe bus. Then remove the ability for linux/etc to
> trigger the quirk by remove the DT node attaching the
> reset controller to the XHCI. The latter seems somewhat
> questionable, since the DT/PCIe host bridge driver is
> doing what appears to be a PERST which in theory
> might then require a firmware reload, but at the
> moment seems to work without.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

I am not opposed to this, but I'd like some other folks to chime in please?

> ---
>  Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> index 0472d8ecf6..1a8fc7a134 100644
> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> @@ -334,6 +334,76 @@ CleanSimpleFramebuffer (
>    return EFI_SUCCESS;
>  }
>
> +STATIC
> +EFI_STATUS
> +SyncPcie (
> +  VOID
> +  )
> +{
> +#if (RPI_MODEL == 4)
> +  INTN          Node;
> +  INTN          Retval;
> +  UINT32        DmaRanges[7];
> +
> +  Node = fdt_path_offset (mFdtImage, "pcie0");
> +  if (Node < 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'pcie0' alias\n", __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // non translated 32-bit DMA window with a limit of 0xc0000000
> +  DmaRanges[0] = cpu_to_fdt32 (0x02000000);
> +  DmaRanges[1] = cpu_to_fdt32 (0x00000000);
> +  DmaRanges[2] = cpu_to_fdt32 (0x00000000);
> +  DmaRanges[3] = cpu_to_fdt32 (0x00000000);
> +  DmaRanges[4] = cpu_to_fdt32 (0x00000000);
> +  DmaRanges[5] = cpu_to_fdt32 (0x00000000);
> +  DmaRanges[6] = cpu_to_fdt32 (0xc0000000);
> +
> +  DEBUG ((DEBUG_INFO, "%a: Updating PCIe dma-ranges\n",  __FUNCTION__));
> +
> +  // Match dma-ranges with the edk2+ACPI setup we are using.
> +  // This works around a failure in linux to reset the XHCI correctly
> +  // when in DT mode following the linux kernel reprogramming the PCIe
> +  // subsystem to match the DT supplied inbound PCIe/DMA translation.
> +  // It appears the lower level firmware honors whatever it reads
> +  // during the first PCI/XHCI quirk call and uses that value until
> +  // rebooted rather than re-reading it on every mailbox command.
> +  Retval = fdt_setprop (mFdtImage, Node, "dma-ranges",
> +                        DmaRanges,  sizeof DmaRanges);
> +  if (Retval != 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to update 'dma-ranges' property (%d)\n",
> +      __FUNCTION__, Retval));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Now that we are always running without DMA translation, and with
> +  // a 3G DMA limit, there shouldn't be a need to reset/reload
> +  // the XHCI for it to operate. The possible problem is that the PCIe root
> +  // port is itself being reset (by linux+DT), The rpi foundation claims
> +  // this is needed as a pre-req to reloading the XHCI firmware, which
> +  // also implies that a PCI fundamental reset should cause the XHCI itself
> +  // to reset. For sure isn't happening fully, otherwise reloading the
> +  // firmware would be mandatory. As it is, recent kernels actually start to
> +  // have problems following the XHCI reset notification mailbox!
> +  // So lets just stop the kernel from triggering the mailbox by removing
> +  // the node.
> +  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@1,0");
> +  if (Node < 0) {
> +    // This can happen on CM4/etc which doesn't have an onboard XHCI
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate /scb/pcie@7d500000/pci@1/usb@1\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"));
> +      return EFI_NOT_FOUND;
> +    }
> +  }
> +
> +#endif
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    @param  ImageHandle   of the loaded driver
>    @param  SystemTable   Pointer to the System Table
> @@ -431,6 +501,11 @@ FdtDxeInitialize (
>      Print (L"Failed to update USB compatible properties: %r\n", Status);
>    }
>
> +  SyncPcie ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to update PCIe address ranges: %r\n", Status);
> +  }
> +
>    DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage));
>    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
>    if (EFI_ERROR (Status)) {
> --
> 2.13.7
>