From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.4395.1591699262110518622 for ; Tue, 09 Jun 2020 03:41:02 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id ABA0A1FB; Tue, 9 Jun 2020 03:41:01 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.184]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BA9BF3F73D; Tue, 9 Jun 2020 03:41:00 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Ensure USB controller init before console To: Pete Batard , devel@edk2.groups.io Cc: leif@nuviainc.com References: <20200609095850.3000-1-pete@akeo.ie> From: "Ard Biesheuvel" Message-ID: <9f6a8b88-02ef-91d6-54b8-45599347a63d@arm.com> Date: Tue, 9 Jun 2020 12:40:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 6/9/20 12:36 PM, Pete Batard wrote: > Hi Ard, >=20 > 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 >>> --- >>> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c | 31= =20 >>> ++++++++++++++++++++ >>> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManag= erLib.inf=20 >>> |=C2=A0 1 + >>> =C2=A0 2 files changed, 32 insertions(+) >>> >>> diff --git=20 >>> a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c=20 >>> 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 ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ReportText)); >>> =C2=A0 } >>> +/** >>> +=C2=A0 This CALLBACK_FUNCTION attempts to connect a handle=20 >>> non-recursively, asking >>> +=C2=A0 the matching driver to produce all first-level child handles. >>> +**/ >>> +STATIC >>> +VOID >>> +EFIAPI >>> +Connect ( >>> +=C2=A0 IN EFI_HANDLE=C2=A0=C2=A0 Handle, >>> +=C2=A0 IN CONST CHAR16 *ReportText >>> +=C2=A0 ) >>> +{ >>> +=C2=A0 EFI_STATUS Status; >>> + >>> +=C2=A0 Status =3D gBS->ConnectController ( >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Handle, // ControllerHandle >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 NULL,=C2=A0=C2=A0 // DriverImageHandle >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 NULL,=C2=A0=C2=A0 // RemainingDevicePat= h -- produce all=20 >>> children >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 FALSE=C2=A0=C2=A0 // Recursive >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>> +=C2=A0 DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, "%a= : %s:=20 >>> %r\n", >>> +=C2=A0=C2=A0=C2=A0 __FUNCTION__, ReportText, Status)); >>> +} >>> + >>> =C2=A0 STATIC >>> =C2=A0 INTN >>> =C2=A0 PlatformRegisterBootOption ( >>> @@ -617,6 +641,13 @@ PlatformBootManagerBeforeConsole ( >>> =C2=A0=C2=A0=C2=A0 // Dispatch deferred images after EndOfDxe event a= nd ReadyToLock=20 >>> installation. >>> =C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0 EfiBootManagerDispatchDeferredImages (); >>> + >>> +=C2=A0 // >>> +=C2=A0 // Ensure that USB is initialized by connecting the PCI root=20 >>> bridge so >>> +=C2=A0 // that the xHCI PCI controller gets enumerated (Pi 4) or by=20 >>> connecting >>> +=C2=A0 // to the DesignWare USB OTG controller directly. >>> +=C2=A0 FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Con= nect); >>> +=C2=A0 FilterAndProcess (&gEfiUsb2HcProtocolGuid, NULL, Connect); >> >> Are you sure this is necessary? >=20 > Not really. But I don't have any better options at the moment, and I=20 > consider that regression needs to be fixed as a matter of urgency=20 > because it makes the platform next to useless at the moment. >=20 >> Connecting the short-form USB device path (mUsbKeyboard) should=20 >> already trigger the code in the BDS that does the same. I.e., >> >> BmConnectUsbShortFormDevicePath ( >> ) >=20 > I tried that, before the call to EfiBootManagerUpdateConsoleVariable=20 > (...&mUsbKeyboard...) but it didn't seem to work... >=20 >> finds all PCI I/O protocol handles with the USB class code, and=20 >> connects them. >> >> Alternatively, we might use EfiBootManagerConnectAllDefaultConsoles()=20 >> instead of ConnectAll() if that is a cleaner way of fixing stuff >=20 > That would be my preferred choice if it worked, because someone may=20 > create their own console input device (e.g. using GPIO) and we'd want t= o=20 > have a way to connect everything that's relevant rather than just USB.=20 > Unfortunately, adding this call didn't seem to do anything either. >=20 > At this stage, because I validated that what's being proposed works on=20 > both Pi 3 and Pi 4 (both platforms are currently broken for keyboard=20 > input right now), and because there's only so much time I can devote to= =20 > testing alternatives right now, I'd push for this fix to be applied=20 > until we come up with a better solution... >=20 Fair enough Reviewed-by: Ard Biesheuvel Pushed as 678f6bff3c46..2d07a49e4532