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

["Leif Lindholm" <leif.lindholm@linaro.org>]

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