[edk2-platforms: PATCH v2 0/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[edk2-platforms: PATCH v2 0/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[Pete Batard]

(v2 as the previous patch had a FreePool call missing).

The following patch adds automatic the insertion of "bcm,bcm2835-usb" into the
"usb" Device Tree node for the Raspberry Pi 3, if not already present.

In terms of DTS structure, this patch basically alters:
  compatible = "brcm,bcm2708-usb";
to:
  compatible = "brcm,bcm2708-usb", "brcm,bcm2835-usb";

The reason we require this is because the official DT published by the Raspberry
Pi Foundation (https://github.com/raspberrypi/firmware/tree/master/boot) only
lists "brcm,bcm2708-usb" as a compatible property, and some Linux kernels, such
as the one used by Debian, require "bcm,bcm2835-usb" to also be listed in order
to handle USB devices.

For instance, if you don't have "bcm,bcm2835-usb", you may find that the USB
keyboards are unresponsive with the Debian installer.

Also, once this patch has been applied, we should be able to replace the Device
Tree blobs we currently have in edk2-non-osi for the RPi3 platform (that have
been manually modified to have "bcm,bcm2835-usb") with the untouched ones from
the Raspberry Pi Foundation. I will therefore send a patch to that effect once
these changes have been merged.

Pete Batard (1):
  Platform/Rpi3: Add compatible property to the "usb" Device Tree node

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

-- 
2.21.0.windows.1

[edk2-platforms: PATCH v2 1/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[Pete Batard]

Some Linux kernels (e.g. Debian) require "bcm,bcm2835-usb" to be present in
the list of compatible properties for the "usb" node, else they are unable
to handle some USB devices.

This patch ensures that the compatible property is added if not present.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index 45ffe2e394a2..eb8048930c30 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -135,6 +135,76 @@ UpdateMacAddress (
   return EFI_SUCCESS;
 }
 
+//
+// Add "bcm2835-usb" to the USB compatible property list, if not present.
+// Required because some Linux kernels can't handle USB devices otherwise.
+//
+STATIC
+EFI_STATUS
+AddUsbCompatibleProperty (
+  VOID
+  )
+{
+  CONST CHAR8   Prop[]    = "brcm,bcm2708-usb";
+  CONST CHAR8   NewProp[] = "brcm,bcm2835-usb";
+  CONST CHAR8   *List;
+  CHAR8         *NewList;
+  INT32         ListSize;
+  INTN          Node;
+  INTN          Retval;
+
+  // Locate the node that the 'usb' alias refers to
+  Node = fdt_path_offset (mFdtImage, "usb");
+  if (Node < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'usb' alias\n", __FUNCTION__));
+    return EFI_NOT_FOUND;
+  }
+
+  // Get the property list. This is a list of NUL terminated strings.
+  List = fdt_getprop (mFdtImage, Node, "compatible", &ListSize);
+  if (List == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate properties\n", __FUNCTION__));
+    return EFI_NOT_FOUND;
+  }
+
+  // Check if the compatible value we plan to add is already present
+  if (fdt_stringlist_contains (List, ListSize, NewProp)) {
+    DEBUG ((DEBUG_INFO, "%a: property '%a' is already set.\n",
+      __FUNCTION__, NewProp));
+    return EFI_SUCCESS;
+  }
+
+  // Make sure the compatible device is what we expect
+  if (!fdt_stringlist_contains (List, ListSize, Prop)) {
+    DEBUG ((DEBUG_ERROR, "%a: property '%a' is missing!\n",
+      __FUNCTION__, Prop));
+    return EFI_NOT_FOUND;
+  }
+
+  // Add the new NUL terminated entry to our list
+  DEBUG ((DEBUG_INFO, "%a: adding '%a' to the properties\n",
+    __FUNCTION__, NewProp));
+
+  NewList = AllocatePool (ListSize + sizeof (NewProp));
+  if (NewList == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
+    return EFI_OUT_OF_RESOURCES;;
+  }
+  CopyMem (NewList, List, ListSize);
+  CopyMem (&NewList[ListSize], NewProp, sizeof (NewProp));
+
+  Retval = fdt_setprop (mFdtImage, Node, "compatible", NewList,
+             ListSize + sizeof (NewProp));
+  FreePool (NewList);
+  if (Retval != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to update properties (%d)\n",
+      __FUNCTION__, Retval));
+    return EFI_NOT_FOUND;
+  }
+
+  return EFI_SUCCESS;
+}
+
 STATIC
 EFI_STATUS
 CleanMemoryNodes (
@@ -486,6 +556,11 @@ FdtDxeInitialize (
     Print (L"Failed to update MAC address: %r\n", Status);
   }
 
+  Status = AddUsbCompatibleProperty ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to update USB compatible properties: %r\n", Status);
+  }
+
   if (Internal) {
     /*
      * A GPU-provided DTB already has the full command line.
-- 
2.21.0.windows.1

Re: [edk2-platforms: PATCH v2 1/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[Leif Lindholm]

On Fri, Aug 23, 2019 at 01:20:50PM +0100, Pete Batard wrote:
> Some Linux kernels (e.g. Debian) require "bcm,bcm2835-usb" to be present in

Is the typo here or in the code? ('bcm,' vs. 'brcm,')
If here, I can fix up on committing.

/
    Leif

> the list of compatible properties for the "usb" node, else they are unable
> to handle some USB devices.
> 
> This patch ensures that the compatible property is added if not present.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index 45ffe2e394a2..eb8048930c30 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -135,6 +135,76 @@ UpdateMacAddress (
>    return EFI_SUCCESS;
>  }
>  
> +//
> +// Add "bcm2835-usb" to the USB compatible property list, if not present.
> +// Required because some Linux kernels can't handle USB devices otherwise.
> +//
> +STATIC
> +EFI_STATUS
> +AddUsbCompatibleProperty (
> +  VOID
> +  )
> +{
> +  CONST CHAR8   Prop[]    = "brcm,bcm2708-usb";
> +  CONST CHAR8   NewProp[] = "brcm,bcm2835-usb";
> +  CONST CHAR8   *List;
> +  CHAR8         *NewList;
> +  INT32         ListSize;
> +  INTN          Node;
> +  INTN          Retval;
> +
> +  // Locate the node that the 'usb' alias refers to
> +  Node = fdt_path_offset (mFdtImage, "usb");
> +  if (Node < 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'usb' alias\n", __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Get the property list. This is a list of NUL terminated strings.
> +  List = fdt_getprop (mFdtImage, Node, "compatible", &ListSize);
> +  if (List == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate properties\n", __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Check if the compatible value we plan to add is already present
> +  if (fdt_stringlist_contains (List, ListSize, NewProp)) {
> +    DEBUG ((DEBUG_INFO, "%a: property '%a' is already set.\n",
> +      __FUNCTION__, NewProp));
> +    return EFI_SUCCESS;
> +  }
> +
> +  // Make sure the compatible device is what we expect
> +  if (!fdt_stringlist_contains (List, ListSize, Prop)) {
> +    DEBUG ((DEBUG_ERROR, "%a: property '%a' is missing!\n",
> +      __FUNCTION__, Prop));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Add the new NUL terminated entry to our list
> +  DEBUG ((DEBUG_INFO, "%a: adding '%a' to the properties\n",
> +    __FUNCTION__, NewProp));
> +
> +  NewList = AllocatePool (ListSize + sizeof (NewProp));
> +  if (NewList == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;;
> +  }
> +  CopyMem (NewList, List, ListSize);
> +  CopyMem (&NewList[ListSize], NewProp, sizeof (NewProp));
> +
> +  Retval = fdt_setprop (mFdtImage, Node, "compatible", NewList,
> +             ListSize + sizeof (NewProp));
> +  FreePool (NewList);
> +  if (Retval != 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to update properties (%d)\n",
> +      __FUNCTION__, Retval));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  STATIC
>  EFI_STATUS
>  CleanMemoryNodes (
> @@ -486,6 +556,11 @@ FdtDxeInitialize (
>      Print (L"Failed to update MAC address: %r\n", Status);
>    }
>  
> +  Status = AddUsbCompatibleProperty ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to update USB compatible properties: %r\n", Status);
> +  }
> +
>    if (Internal) {
>      /*
>       * A GPU-provided DTB already has the full command line.
> -- 
> 2.21.0.windows.1
> 

Re: [edk2-platforms: PATCH v2 1/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[Pete Batard]

Hi Leif,

On 2019.08.29 14:54, Leif Lindholm wrote:
> On Fri, Aug 23, 2019 at 01:20:50PM +0100, Pete Batard wrote:
>> Some Linux kernels (e.g. Debian) require "bcm,bcm2835-usb" to be present in
> 
> Is the typo here or in the code? ('bcm,' vs. 'brcm,')

Sorry, should have been 'brcm,bcm2835-usb' above.

As mentioned in the cover letter, we're basically adding to the 
compatible string list so that:
   compatible = "brcm,bcm2708-usb";
becomes:
   compatible = "brcm,bcm2708-usb", "brcm,bcm2835-usb";

So the part before the comma should always be "brcm", and it's only the 
part after that should be "bcm" (which, alas, makes it easy to introduce 
typos).

In other words, the typo is in the commit message only.

I did validate the changes proposed by this patch on real hardware, so I 
can vouch for the code being correct, insofar as the compatible strings 
there are concerned.

> If here, I can fix up on committing.

If you can fix the typo in the commit message, that would be great.

Regards,

/Pete

> 
> /
>      Leif
> 
>> the list of compatible properties for the "usb" node, else they are unable
>> to handle some USB devices.
>>
>> This patch ensures that the compatible property is added if not present.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> index 45ffe2e394a2..eb8048930c30 100644
>> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> @@ -135,6 +135,76 @@ UpdateMacAddress (
>>     return EFI_SUCCESS;
>>   }
>>   
>> +//
>> +// Add "bcm2835-usb" to the USB compatible property list, if not present.
>> +// Required because some Linux kernels can't handle USB devices otherwise.
>> +//
>> +STATIC
>> +EFI_STATUS
>> +AddUsbCompatibleProperty (
>> +  VOID
>> +  )
>> +{
>> +  CONST CHAR8   Prop[]    = "brcm,bcm2708-usb";
>> +  CONST CHAR8   NewProp[] = "brcm,bcm2835-usb";
>> +  CONST CHAR8   *List;
>> +  CHAR8         *NewList;
>> +  INT32         ListSize;
>> +  INTN          Node;
>> +  INTN          Retval;
>> +
>> +  // Locate the node that the 'usb' alias refers to
>> +  Node = fdt_path_offset (mFdtImage, "usb");
>> +  if (Node < 0) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'usb' alias\n", __FUNCTION__));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  // Get the property list. This is a list of NUL terminated strings.
>> +  List = fdt_getprop (mFdtImage, Node, "compatible", &ListSize);
>> +  if (List == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate properties\n", __FUNCTION__));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  // Check if the compatible value we plan to add is already present
>> +  if (fdt_stringlist_contains (List, ListSize, NewProp)) {
>> +    DEBUG ((DEBUG_INFO, "%a: property '%a' is already set.\n",
>> +      __FUNCTION__, NewProp));
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  // Make sure the compatible device is what we expect
>> +  if (!fdt_stringlist_contains (List, ListSize, Prop)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: property '%a' is missing!\n",
>> +      __FUNCTION__, Prop));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  // Add the new NUL terminated entry to our list
>> +  DEBUG ((DEBUG_INFO, "%a: adding '%a' to the properties\n",
>> +    __FUNCTION__, NewProp));
>> +
>> +  NewList = AllocatePool (ListSize + sizeof (NewProp));
>> +  if (NewList == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
>> +    return EFI_OUT_OF_RESOURCES;;
>> +  }
>> +  CopyMem (NewList, List, ListSize);
>> +  CopyMem (&NewList[ListSize], NewProp, sizeof (NewProp));
>> +
>> +  Retval = fdt_setprop (mFdtImage, Node, "compatible", NewList,
>> +             ListSize + sizeof (NewProp));
>> +  FreePool (NewList);
>> +  if (Retval != 0) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to update properties (%d)\n",
>> +      __FUNCTION__, Retval));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>>   STATIC
>>   EFI_STATUS
>>   CleanMemoryNodes (
>> @@ -486,6 +556,11 @@ FdtDxeInitialize (
>>       Print (L"Failed to update MAC address: %r\n", Status);
>>     }
>>   
>> +  Status = AddUsbCompatibleProperty ();
>> +  if (EFI_ERROR (Status)) {
>> +    Print (L"Failed to update USB compatible properties: %r\n", Status);
>> +  }
>> +
>>     if (Internal) {
>>       /*
>>        * A GPU-provided DTB already has the full command line.
>> -- 
>> 2.21.0.windows.1
>>

Re: [edk2-platforms: PATCH v2 1/1] Platform/Rpi3: Add compatible property to the "usb" Device Tree node

[Leif Lindholm]

On Thu, Aug 29, 2019 at 04:26:58PM +0100, Pete Batard wrote:
> Hi Leif,
> 
> On 2019.08.29 14:54, Leif Lindholm wrote:
> > On Fri, Aug 23, 2019 at 01:20:50PM +0100, Pete Batard wrote:
> > > Some Linux kernels (e.g. Debian) require "bcm,bcm2835-usb" to be present in
> > 
> > Is the typo here or in the code? ('bcm,' vs. 'brcm,')
> 
> Sorry, should have been 'brcm,bcm2835-usb' above.
> 
> As mentioned in the cover letter, we're basically adding to the compatible
> string list so that:
>   compatible = "brcm,bcm2708-usb";
> becomes:
>   compatible = "brcm,bcm2708-usb", "brcm,bcm2835-usb";
> 
> So the part before the comma should always be "brcm", and it's only the part
> after that should be "bcm" (which, alas, makes it easy to introduce typos).
> 
> In other words, the typo is in the commit message only.
> 
> I did validate the changes proposed by this patch on real hardware, so I can
> vouch for the code being correct, insofar as the compatible strings there
> are concerned.

Thanks. I was 99% sure that was the case, but I prefer having these
things explicitly clarified before I commit.

> > If here, I can fix up on committing.
> 
> If you can fix the typo in the commit message, that would be great.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 5f003136c2bf.

> Regards,
> 
> /Pete
> 
> > 
> > /
> >      Leif
> > 
> > > the list of compatible properties for the "usb" node, else they are unable
> > > to handle some USB devices.
> > > 
> > > This patch ensures that the compatible property is added if not present.
> > > 
> > > Signed-off-by: Pete Batard <pete@akeo.ie>
> > > ---
> > >   Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++
> > >   1 file changed, 75 insertions(+)
> > > 
> > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> > > index 45ffe2e394a2..eb8048930c30 100644
> > > --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> > > +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> > > @@ -135,6 +135,76 @@ UpdateMacAddress (
> > >     return EFI_SUCCESS;
> > >   }
> > > +//
> > > +// Add "bcm2835-usb" to the USB compatible property list, if not present.
> > > +// Required because some Linux kernels can't handle USB devices otherwise.
> > > +//
> > > +STATIC
> > > +EFI_STATUS
> > > +AddUsbCompatibleProperty (
> > > +  VOID
> > > +  )
> > > +{
> > > +  CONST CHAR8   Prop[]    = "brcm,bcm2708-usb";
> > > +  CONST CHAR8   NewProp[] = "brcm,bcm2835-usb";
> > > +  CONST CHAR8   *List;
> > > +  CHAR8         *NewList;
> > > +  INT32         ListSize;
> > > +  INTN          Node;
> > > +  INTN          Retval;
> > > +
> > > +  // Locate the node that the 'usb' alias refers to
> > > +  Node = fdt_path_offset (mFdtImage, "usb");
> > > +  if (Node < 0) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'usb' alias\n", __FUNCTION__));
> > > +    return EFI_NOT_FOUND;
> > > +  }
> > > +
> > > +  // Get the property list. This is a list of NUL terminated strings.
> > > +  List = fdt_getprop (mFdtImage, Node, "compatible", &ListSize);
> > > +  if (List == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate properties\n", __FUNCTION__));
> > > +    return EFI_NOT_FOUND;
> > > +  }
> > > +
> > > +  // Check if the compatible value we plan to add is already present
> > > +  if (fdt_stringlist_contains (List, ListSize, NewProp)) {
> > > +    DEBUG ((DEBUG_INFO, "%a: property '%a' is already set.\n",
> > > +      __FUNCTION__, NewProp));
> > > +    return EFI_SUCCESS;
> > > +  }
> > > +
> > > +  // Make sure the compatible device is what we expect
> > > +  if (!fdt_stringlist_contains (List, ListSize, Prop)) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: property '%a' is missing!\n",
> > > +      __FUNCTION__, Prop));
> > > +    return EFI_NOT_FOUND;
> > > +  }
> > > +
> > > +  // Add the new NUL terminated entry to our list
> > > +  DEBUG ((DEBUG_INFO, "%a: adding '%a' to the properties\n",
> > > +    __FUNCTION__, NewProp));
> > > +
> > > +  NewList = AllocatePool (ListSize + sizeof (NewProp));
> > > +  if (NewList == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
> > > +    return EFI_OUT_OF_RESOURCES;;
> > > +  }
> > > +  CopyMem (NewList, List, ListSize);
> > > +  CopyMem (&NewList[ListSize], NewProp, sizeof (NewProp));
> > > +
> > > +  Retval = fdt_setprop (mFdtImage, Node, "compatible", NewList,
> > > +             ListSize + sizeof (NewProp));
> > > +  FreePool (NewList);
> > > +  if (Retval != 0) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: failed to update properties (%d)\n",
> > > +      __FUNCTION__, Retval));
> > > +    return EFI_NOT_FOUND;
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > >   STATIC
> > >   EFI_STATUS
> > >   CleanMemoryNodes (
> > > @@ -486,6 +556,11 @@ FdtDxeInitialize (
> > >       Print (L"Failed to update MAC address: %r\n", Status);
> > >     }
> > > +  Status = AddUsbCompatibleProperty ();
> > > +  if (EFI_ERROR (Status)) {
> > > +    Print (L"Failed to update USB compatible properties: %r\n", Status);
> > > +  }
> > > +
> > >     if (Internal) {
> > >       /*
> > >        * A GPU-provided DTB already has the full command line.
> > > -- 
> > > 2.21.0.windows.1
> > > 
>