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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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