On 30 August 2016 at 14:32, Leif Lindholm leif.lindholm@linaro.org wrote:
On Tue, Aug 30, 2016 at 12:18:09PM +0100, Ard Biesheuvel wrote:
This implements a PCI driver for the Renesas XHCI PD720202 PCIe controller, which in some cases requires a device firmware to be uploaded by the system firmware to function properly.
By registering a UEFI driver with high dispatch priority, this driver should be able to access the device early, install the firmware, and
Do change the "should be" after we've tested it :)
Sure :-)
then decline the driver dispatch so that the device should be fully functional when the real XHCI driver gets to see it.
Good stuff! Minor comments below.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.c | 365 ++++++++++++++++++++ Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.inf | 43 +++ OpenPlatformPkg.dec | 3 + 3 files changed, 411 insertions(+)
diff --git a/Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.c b/Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.c new file mode 100644 index 000000000000..e1ff5d12c9bf --- /dev/null +++ b/Drivers/Xhci/RenesasFirmwarePD720202/RenesasFirmwarePD720202.c @@ -0,0 +1,365 @@ +/** @file
- Implementation of driver entry point and driver binding protocol.
+Copyright (c) 2016, Linaro Limited. All rights reserved.
+This program and the accompanying materials are licensed +and made available under the terms and conditions of the BSD License which +accompanies this distribution. The full text of the license may be found at +http://opensource.org/licenses/bsd-license.php
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#include <PiDxe.h>
+#include <Library/DebugLib.h> +#include <Library/DxeServicesLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/UefiLib.h>
+#include <IndustryStandard/Pci22.h>
+#include <Protocol/PciIo.h>
+#define PCI_VENDOR_ID_RENESAS 0x1912 +#define PCI_DEVICE_ID_PD720202 0x15
+#define PCI_FW_CTL_STAT_REG 0xF4 +#define PCI_FW_CTL_DATA_REG 0xF5 +#define PCI_FW_DATA0 0xF8 +#define PCI_FW_DATA1 0xFC
+#define PCI_FW_CTL_STAT_REG_FW_DL_ENABLE (1U << 0) +#define PCI_FW_CTL_STAT_REG_RESULT_CODE (7U << 4) +#define PCI_FW_CTL_STAT_REG_RESULT_OK (1U << 4) +#define PCI_FW_CTL_STAT_REG_SET_DATA0 (1U << 0) +#define PCI_FW_CTL_STAT_REG_SET_DATA1 (1U << 1)
+STATIC CONST UINT32 *mFirmwareImage; +STATIC UINTN mFirmwareImageSize;
+STATIC +UINT8 +ReadCtlStatVal (
- IN EFI_PCI_IO_PROTOCOL *PciIo,
- IN UINTN Offset
- )
+{
- UINT8 CtlStatVal;
- EFI_STATUS Status;
- Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, Offset, 1, &CtlStatVal);
- ASSERT_EFI_ERROR (Status);
- return CtlStatVal;
+}
+STATIC +VOID +WriteCtlStatVal (
- IN EFI_PCI_IO_PROTOCOL *PciIo,
- IN UINTN Offset,
- IN UINT8 CtlStatVal
- )
+{
- EFI_STATUS Status;
- Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, Offset, 1, &CtlStatVal);
- ASSERT_EFI_ERROR (Status);
+}
+STATIC +VOID +WriteDataVal (
- IN EFI_PCI_IO_PROTOCOL *PciIo,
- IN UINTN Offset,
- IN UINT32 DataVal
- )
+{
- EFI_STATUS Status;
- Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint32, Offset, 1, &DataVal);
- ASSERT_EFI_ERROR (Status);
+}
+STATIC +BOOLEAN +WaitReadCtlStatVal (
- IN EFI_PCI_IO_PROTOCOL *PciIo,
- IN UINTN Offset,
- IN UINT8 Mask,
- IN UINT8 Val
- )
+{
- UINTN Timeout;
- for (Timeout = 0; (ReadCtlStatVal (PciIo, Offset) & Mask) != Val; Timeout++) {
- if (Timeout > 1000) {
DEBUG ((EFI_D_ERROR,"%a: Timeout waiting for reg [+0x%x] & 0x%x to become 0x%x\n",__FUNCTION__, Offset, Mask, Val));return FALSE;- }
- gBS->Stall (10);
- }
- return TRUE;
+}
+STATIC +VOID +DownloadPD720202Firmware (
- IN EFI_PCI_IO_PROTOCOL *PciIo
- )
+{
- UINTN Idx;
- Idx = 0;
- // 1. Set "FW Download Enable" to '1b'.
- WriteCtlStatVal (PciIo, PCI_FW_CTL_STAT_REG,
- PCI_FW_CTL_STAT_REG_FW_DL_ENABLE);
- // 2. Read "Set DATA0" and confirm it is '0b'.
- if (!WaitReadCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG,
PCI_FW_CTL_STAT_REG_SET_DATA0, 0)) {- return;
- }
- // 3. Write FW data to "DATA0".
- WriteDataVal (PciIo, PCI_FW_DATA0, mFirmwareImage[Idx++]);
- // 4. Read "Set DATA1" and confirm it is '0b'.
- if (!WaitReadCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG,
PCI_FW_CTL_STAT_REG_SET_DATA1, 0)) {- return;
- }
- // 5. Write FW data to "DATA1".
- WriteDataVal (PciIo, PCI_FW_DATA1, mFirmwareImage[Idx++]);
- // 6. Set "Set DATA0" & "Set DATA1" to '1b'.
- WriteCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG,
- PCI_FW_CTL_STAT_REG_SET_DATA0 | PCI_FW_CTL_STAT_REG_SET_DATA1);
- while (Idx < mFirmwareImageSize / sizeof(UINT32)) {
- // 7. Read "Set DATA0" and confirm it is '0b'.
- if (!WaitReadCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG,
PCI_FW_CTL_STAT_REG_SET_DATA0, 0)) {return;- }
- // 8. Write FW data to"DATA0". Set "Set DATA0" to '1b'.
- WriteDataVal (PciIo, PCI_FW_DATA0, mFirmwareImage[Idx++]);
- WriteCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG, PCI_FW_CTL_STAT_REG_SET_DATA0);
- // 9. Read "Set DATA1" and confirm it is '0b'.
- if (!WaitReadCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG,
PCI_FW_CTL_STAT_REG_SET_DATA1, 0)) {return;- }
- // 10. Write FW data to"DATA1". Set "Set DATA1" to '1b'.
- WriteDataVal (PciIo, PCI_FW_DATA1, mFirmwareImage[Idx++]);
- WriteCtlStatVal (PciIo, PCI_FW_CTL_DATA_REG, PCI_FW_CTL_STAT_REG_SET_DATA1);
- // 11. Return to step 7 and repeat the sequence from step 7 to step 10.
- }
- // 12. After writing the last data of FW, the System Software must set "FW Download Enable" to '0b'.
- WriteCtlStatVal (PciIo, PCI_FW_CTL_STAT_REG, 0);
- // 13. Read "Result Code" and confirm it is '001b'.
- if (WaitReadCtlStatVal (PciIo, PCI_FW_CTL_STAT_REG,
PCI_FW_CTL_STAT_REG_RESULT_CODE, PCI_FW_CTL_STAT_REG_RESULT_OK)) {- DEBUG ((EFI_D_INFO, "%a: Renesas PD720202 firmware download successful\n",
__FUNCTION__));- } else {
- DEBUG ((EFI_D_ERROR, "%a: Renesas PD720202 firmware download FAILED\n",
__FUNCTION__));- }
+}
+/**
- Test to see if this driver supports ControllerHandle. This service
- is called by the EFI boot service ConnectController(). In
- order to make drivers as small as possible, there are a few calling
- restrictions for this service. ConnectController() must
- follow these calling restrictions. If any other agent wishes to call
- Supported() it must also follow these calling restrictions.
- @param This Protocol instance pointer.
- @param ControllerHandle Handle of device to test.
- @param RemainingDevicePath Optional parameter use to pick a specific child
device to start.- @retval EFI_SUCCESS This driver supports this device.
- @retval EFI_ALREADY_STARTED This driver is already running on this device.
- @retval other This driver does not support this device.
+**/ +STATIC +EFI_STATUS +EFIAPI +RenesasPD720202DriverSupported (
- IN EFI_DRIVER_BINDING_PROTOCOL *This,
- IN EFI_HANDLE Controller,
- IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
- )
+{
- EFI_STATUS Status;
- EFI_PCI_IO_PROTOCOL *PciIo;
- UINT32 PciID;
- UINT8 CtlStatVal;
- //
- // Check for the PCI IO Protocol
- //
- Status = gBS->OpenProtocol (Controller, &gEfiPciIoProtocolGuid,
(VOID **)&PciIo, This->DriverBindingHandle, Controller,EFI_OPEN_PROTOCOL_BY_DRIVER);- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, PCI_VENDOR_ID_OFFSET,
1, &PciID);- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR,
"%a: Pci->Pci.Read() of vendor/device id failed (Status == %r)\n",__FUNCTION__, Status));- goto CloseProtocol;
- }
- if ((PciID & 0xffff) != PCI_VENDOR_ID_RENESAS ||
(PciID >> 16) != PCI_DEVICE_ID_PD720202) {- DEBUG ((EFI_D_INFO, "%a: ignoring unsupported PCI device\n", __FUNCTION__));
- goto CloseProtocol;
- }
- //
- // We have found a device of the type we are looking for. Check if the
- // firmware needs to be loaded.
- //
- Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, PCI_FW_CTL_STAT_REG,
1, &CtlStatVal);- if (!EFI_ERROR (Status) &&
(CtlStatVal & PCI_FW_CTL_STAT_REG_RESULT_CODE) == 0) {Could we have a RESULT_INVALID define?
Will do
- //
- // Firmware download required
- //
- DEBUG ((EFI_D_INFO, "%a: downloading firmware\n", __FUNCTION__));
- DownloadPD720202Firmware (PciIo);
- }
+CloseProtocol:
- gBS->CloseProtocol (Controller, &gEfiPciIoProtocolGuid,
This->DriverBindingHandle, Controller);- //
- // Always return unsupported: we are not interested in driving the device,
- // only in having the opportunity to install the firmware before the real
- // driver attaches to it.
- //
- return EFI_UNSUPPORTED;
+}
+/**
- Start this driver on Controller by opening PciIo and DevicePath protocols.
- Initialize PXE structures, create a copy of the Controller Device Path with the
- NIC's MAC address appended to it, install the NetworkInterfaceIdentifier protocol
Ah, I see what you did there. Be nice to adjust the comment before commit though.
Ah, I thought i got rid of the boilerplate comments, but apparently not
- on the newly created Device Path.
- @param [in] This Protocol instance pointer.
- @param [in] Controller Handle of device to work with.
- @param [in] RemainingDevicePath Not used, always produce all possible children.
- @retval EFI_SUCCESS This driver is added to Controller.
- @retval other This driver does not support this device.
+**/ +STATIC +EFI_STATUS +EFIAPI +RenesasPD720202DriverStart (
- IN EFI_DRIVER_BINDING_PROTOCOL *This,
- IN EFI_HANDLE Controller,
- IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath
- )
+{
- //
- // We are not interested in driving the device, we only poke the firmware
- // in the .Supported() callback.
- //
- ASSERT (FALSE);
- return EFI_INVALID_PARAMETER;
+}
+/**
- Stop this driver on Controller
- @param [in] This Protocol instance pointer.
- @param [in] Controller Handle of device to stop driver on.
- @param [in] NumberOfChildren How many children need to be stopped.
- @param [in] ChildHandleBuffer Not used.
- @retval EFI_SUCCESS This driver is removed Controller.
- @retval EFI_DEVICE_ERROR The device could not be stopped due to a device error.
- @retval other This driver was not removed from this device.
+**/ +STATIC +EFI_STATUS +EFIAPI +RenesasPD720202DriverStop (
- IN EFI_DRIVER_BINDING_PROTOCOL *This,
- IN EFI_HANDLE Controller,
- IN UINTN NumberOfChildren,
- IN EFI_HANDLE *ChildHandleBuffer
- )
+{
So, I was going to point out that since we never Start the driver, it is invalid to call Stop (and we should put a comment to that effct in). However, I'm not convinced EFI_INVALID_PARAMETER is a valid thing to return here: "If ControllerHandle is stopped, then EFI_SUCCESS is returned."
(We may still want to assert, in order to find funny stuff going on.
Frankly, I don't think it makes a whole lot of difference what we return here, since if it is being called, weird stuff is going on anyway. So let's simply return EFI_SUCCESS instead.
- ASSERT (FALSE);
- return EFI_INVALID_PARAMETER;
+}
+// +// Simple Network Protocol Driver Global Variables
*cough*
+// +STATIC EFI_DRIVER_BINDING_PROTOCOL RenesasPD720202DriverBinding = {
- RenesasPD720202DriverSupported,
- RenesasPD720202DriverStart,
- RenesasPD720202DriverStop,
- 0xfffffff0,
Would it be worth adding a define for this one? If not, a comment pointing out the party trick might be in order: // Version values of 0xfffffff0-0xffffffff are reserved for // platform/OEM specific drivers. Protocol instances with higher // 'Version' drivers will be used before lower 'Version' ones. // XhciDxe uses version 0x30, so this driver will be called in // preference, and XhciDxe will be invoked after // RenesasPD720202DriverSupported returns EFI_UNSUPPORTED.
Yes, let me add that once we're a bit more confident that it actually works as expected
Thanks, Ard.