Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Ensure USB controller init before console

["Pete Batard" <pete@akeo.ie>]

On 2020.06.09 11:40, Ard Biesheuvel wrote:
> On 6/9/20 12:36 PM, Pete Batard wrote:
>> Hi Ard,
>>
>> On 2020.06.09 11:06, Ard Biesheuvel wrote:
>>> Hi Pete,
>>>
>>> On 6/9/20 11:58 AM, Pete Batard wrote:
>>>> Due to the nature of USB init on the Pi platforms, commit
>>>> c8000ecccc83b728baf04ced2fedb870bc3bc1b3 introduced a regression
>>>> with regards to ensuring that USB devices are operational by the
>>>> time the console is up.
>>>>
>>>> This commit fixes this by adding explicit calls to connect USB
>>>> controllers before console initialization.
>>>>
>>>> Note that trying to connect a non existent device (e.g. PCI bus
>>>> on the Pi 3) is harmless so there's no need to guard these calls
>>>> according to the devices effectively supported by the platform.
>>>>
>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>> ---
>>>> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c | 
>>>> 31 ++++++++++++++++++++
>>>> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf 
>>>> |  1 +
>>>>   2 files changed, 32 insertions(+)
>>>>
>>>> diff --git 
>>>> a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c 
>>>> b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
>>>> index 253614a646c1..d347c733855d 100644
>>>> --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
>>>> +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
>>>> @@ -314,6 +314,30 @@ AddOutput (
>>>>       ReportText));
>>>>   }
>>>> +/**
>>>> +  This CALLBACK_FUNCTION attempts to connect a handle 
>>>> non-recursively, asking
>>>> +  the matching driver to produce all first-level child handles.
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +EFIAPI
>>>> +Connect (
>>>> +  IN EFI_HANDLE   Handle,
>>>> +  IN CONST CHAR16 *ReportText
>>>> +  )
>>>> +{
>>>> +  EFI_STATUS Status;
>>>> +
>>>> +  Status = gBS->ConnectController (
>>>> +                  Handle, // ControllerHandle
>>>> +                  NULL,   // DriverImageHandle
>>>> +                  NULL,   // RemainingDevicePath -- produce all 
>>>> children
>>>> +                  FALSE   // Recursive
>>>> +                  );
>>>> +  DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, "%a: 
>>>> %s: %r\n",
>>>> +    __FUNCTION__, ReportText, Status));
>>>> +}
>>>> +
>>>>   STATIC
>>>>   INTN
>>>>   PlatformRegisterBootOption (
>>>> @@ -617,6 +641,13 @@ PlatformBootManagerBeforeConsole (
>>>>     // Dispatch deferred images after EndOfDxe event and ReadyToLock 
>>>> installation.
>>>>     //
>>>>     EfiBootManagerDispatchDeferredImages ();
>>>> +
>>>> +  //
>>>> +  // Ensure that USB is initialized by connecting the PCI root 
>>>> bridge so
>>>> +  // that the xHCI PCI controller gets enumerated (Pi 4) or by 
>>>> connecting
>>>> +  // to the DesignWare USB OTG controller directly.
>>>> +  FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);
>>>> +  FilterAndProcess (&gEfiUsb2HcProtocolGuid, NULL, Connect);
>>>
>>> Are you sure this is necessary?
>>
>> Not really. But I don't have any better options at the moment, and I 
>> consider that regression needs to be fixed as a matter of urgency 
>> because it makes the platform next to useless at the moment.
>>
>>> Connecting the short-form USB device path (mUsbKeyboard) should 
>>> already trigger the code in the BDS that does the same. I.e.,
>>>
>>> BmConnectUsbShortFormDevicePath (
>>> )
>>
>> I tried that, before the call to EfiBootManagerUpdateConsoleVariable 
>> (...&mUsbKeyboard...) but it didn't seem to work...
>>
>>> finds all PCI I/O protocol handles with the USB class code, and 
>>> connects them.
>>>
>>> Alternatively, we might use EfiBootManagerConnectAllDefaultConsoles() 
>>> instead of ConnectAll() if that is a cleaner way of fixing stuff
>>
>> That would be my preferred choice if it worked, because someone may 
>> create their own console input device (e.g. using GPIO) and we'd want 
>> to have a way to connect everything that's relevant rather than just 
>> USB. Unfortunately, adding this call didn't seem to do anything either.
>>
>> At this stage, because I validated that what's being proposed works on 
>> both Pi 3 and Pi 4 (both platforms are currently broken for keyboard 
>> input right now), and because there's only so much time I can devote 
>> to testing alternatives right now, I'd push for this fix to be applied 
>> until we come up with a better solution...
>>
> 
> Fair enough
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Pushed as 678f6bff3c46..2d07a49e4532

Thanks!

/Pete