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.web10.6431.1591087428153855740 for ; Tue, 02 Jun 2020 01:43:48 -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 D12501FB; Tue, 2 Jun 2020 01:43:47 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.209]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6598B3F7D8; Tue, 2 Jun 2020 01:43:46 -0700 (PDT) Subject: Re: [PATCH edk2-platforms 1/2] Silicon/NXP: Add DWC3 USB controller initialization driver To: Meenakshi Aggarwal , leif@nuviainc.com, michael.d.kinney@intel.com, devel@edk2.groups.io Cc: v.sethi@nxp.com, Meenakshi Aggarwal References: <1591105661-29757-1-git-send-email-meenakshi.aggarwal@oss.nxp.com> <1591105661-29757-2-git-send-email-meenakshi.aggarwal@oss.nxp.com> From: "Ard Biesheuvel" Message-ID: <35c23340-8f60-f31a-6c40-09c348d9e76c@arm.com> Date: Tue, 2 Jun 2020 10:43:44 +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: <1591105661-29757-2-git-send-email-meenakshi.aggarwal@oss.nxp.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Meenakshi, On 6/2/20 3:47 PM, Meenakshi Aggarwal wrote: > From: Meenakshi Aggarwal > > Add support of DWC3 controller driver which performs > DWC3 controller initialization and register itself as > NonDiscoverableMmioDevice > > Signed-off-by: Meenakshi Aggarwal > --- > Silicon/NXP/NxpQoriqLs.dec | 5 + > Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.inf | 41 ++++ > Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.h | 138 +++++++++++++ > Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.c | 212 ++++++++++++++++++++ > 4 files changed, 396 insertions(+) > > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec > index d4d3057af509..72c1744fc934 100644 > --- a/Silicon/NXP/NxpQoriqLs.dec > +++ b/Silicon/NXP/NxpQoriqLs.dec > @@ -36,6 +36,11 @@ [PcdsFixedAtBuild.common] > gNxpQoriqLsTokenSpaceGuid.PcdPcieLutBase|0x0|UINT32|0x00000502 > gNxpQoriqLsTokenSpaceGuid.PcdPcieLutDbg|0x0|UINT32|0x00000503 > > + # Pcds for USB > + gNxpQoriqLsTokenSpaceGuid.PcdUsbBaseAddr|0x0|UINT64|0x00000510 > + gNxpQoriqLsTokenSpaceGuid.PcdUsbSize|0x0|UINT32|0x00000511 > + gNxpQoriqLsTokenSpaceGuid.PcdNumUsbController|0|UINT32|0x00000512 > + > [PcdsDynamic.common] > gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x00000600 > gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601 > diff --git a/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.inf b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.inf > new file mode 100644 > index 000000000000..b8c3be4bfb49 > --- /dev/null > +++ b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.inf > @@ -0,0 +1,41 @@ > +# UsbHcd.inf > +# > +# Copyright 2017, 2020 NXP > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > + > +[Defines] > + INF_VERSION = 0x0001001A 0x0001001A (or simply 1.27 works fine as well) > + BASE_NAME = UsbHcdDxe > + FILE_GUID = 196e7c2a-37b2-4b85-8683-718588952449 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = InitializeUsbHcd > + > +[Sources.common] > + UsbHcd.c > + Include the .h file here as well > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + Silicon/NXP/NxpQoriqLs.dec > + > +[LibraryClasses] > + BaseLib > + BaseMemoryLib > + DebugLib > + IoLib > + MemoryAllocationLib > + NonDiscoverableDeviceRegistrationLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + > +[FixedPcd] > + gNxpQoriqLsTokenSpaceGuid.PcdNumUsbController > + gNxpQoriqLsTokenSpaceGuid.PcdUsbBaseAddr > + gNxpQoriqLsTokenSpaceGuid.PcdUsbSize > + > +[Depex] > + TRUE > diff --git a/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.h b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.h > new file mode 100644 > index 000000000000..cd9f9ad80125 > --- /dev/null > +++ b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.h > @@ -0,0 +1,138 @@ > +/** @file > + > + Copyright 2017, 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef USB_HCD_H_ > +#define USB_HCD_H_ > + > +#include > + > +/* Global constants */ > +#define DWC3_GSNPSID_MASK 0xffff0000 > +#define DWC3_SYNOPSYS_ID 0x55330000 > +#define DWC3_RELEASE_MASK 0xffff > +#define DWC3_REG_OFFSET 0xC100 > +#define DWC3_RELEASE_190a 0x190a > + > +/* Global Configuration Register */ > +#define DWC3_GCTL_U2RSTECN BIT16 > +#define DWC3_GCTL_PRTCAPDIR(N) ((N) << 12) > +#define DWC3_GCTL_PRTCAP_HOST 1 > +#define DWC3_GCTL_PRTCAP_OTG 3 > +#define DWC3_GCTL_CORESOFTRESET BIT11 > +#define DWC3_GCTL_SCALEDOWN(N) ((N) << 4) > +#define DWC3_GCTL_SCALEDOWN_MASK DWC3_GCTL_SCALEDOWN(3) > +#define DWC3_GCTL_DISSCRAMBLE BIT3 > +#define DWC3_GCTL_DSBLCLKGTNG BIT0 > + > +/* Global HWPARAMS1 Register */ > +#define DWC3_GHWPARAMS1_EN_PWROPT(N) (((N) & (3 << 24)) >> 24) > +#define DWC3_GHWPARAMS1_EN_PWROPT_CLK 1 > + > +/* Global USB2 PHY Configuration Register */ > +#define DWC3_GUSB2PHYCFG_PHYSOFTRST BIT31 > + > +/* Global USB3 PIPE Control Register */ > +#define DWC3_GUSB3PIPECTL_PHYSOFTRST BIT31 > + > +/* Global Frame Length Adjustment Register */ > +#define GFLADJ_30MHZ_REG_SEL BIT7 > +#define GFLADJ_30MHZ(N) ((N) & 0x3f) > +#define GFLADJ_30MHZ_DEFAULT 0x20 > + > +/* Default to the FSL XHCI defines */ > +#define USB3_ENABLE_BEAT_BURST 0xF > +#define USB3_ENABLE_BEAT_BURST_MASK 0xFF > +#define USB3_SET_BEAT_BURST_LIMIT 0xF00 > + > +typedef struct { > + UINT32 GEvntAdrLo; > + UINT32 GEvntAdrHi; > + UINT32 GEvntSiz; > + UINT32 GEvntCount; > +} G_EVENT_BUFFER; > + > +typedef struct { > + UINT32 DDepCmdPar2; > + UINT32 DDepCmdPar1; > + UINT32 DDepCmdPar0; > + UINT32 DDepCmd; > +} D_PHYSICAL_EP; > + > +typedef struct { > + UINT32 GSBusCfg0; > + UINT32 GSBusCfg1; > + UINT32 GTxThrCfg; > + UINT32 GRxThrCfg; > + UINT32 GCtl; > + UINT32 Res1; > + UINT32 GSts; > + UINT32 Res2; > + UINT32 GSnpsId; > + UINT32 GGpio; > + UINT32 GUid; > + UINT32 GUctl; > + UINT64 GBusErrAddr; > + UINT64 GPrtbImap; > + UINT32 GHwParams0; > + UINT32 GHwParams1; > + UINT32 GHwParams2; > + UINT32 GHwParams3; > + UINT32 GHwParams4; > + UINT32 GHwParams5; > + UINT32 GHwParams6; > + UINT32 GHwParams7; > + UINT32 GDbgFifoSpace; > + UINT32 GDbgLtssm; > + UINT32 GDbgLnmcc; > + UINT32 GDbgBmu; > + UINT32 GDbgLspMux; > + UINT32 GDbgLsp; > + UINT32 GDbgEpInfo0; > + UINT32 GDbgEpInfo1; > + UINT64 GPrtbImapHs; > + UINT64 GPrtbImapFs; > + UINT32 Res3[28]; > + UINT32 GUsb2PhyCfg[16]; > + UINT32 GUsb2I2cCtl[16]; > + UINT32 GUsb2PhyAcc[16]; > + UINT32 GUsb3PipeCtl[16]; > + UINT32 GTxFifoSiz[32]; > + UINT32 GRxFifoSiz[32]; > + G_EVENT_BUFFER GEvntBuf[32]; > + UINT32 GHwParams8; > + UINT32 Res4[11]; > + UINT32 GFLAdj; > + UINT32 Res5[51]; > + UINT32 DCfg; > + UINT32 DCtl; > + UINT32 DEvten; > + UINT32 DSts; > + UINT32 DGCmdPar; > + UINT32 DGCmd; > + UINT32 Res6[2]; > + UINT32 DAlepena; > + UINT32 Res7[55]; > + D_PHYSICAL_EP DPhyEpCmd[32]; > + UINT32 Res8[128]; > + UINT32 OCfg; > + UINT32 OCtl; > + UINT32 OEvt; > + UINT32 OEvtEn; > + UINT32 OSts; > + UINT32 Res9[3]; > + UINT32 AdpCfg; > + UINT32 AdpCtl; > + UINT32 AdpEvt; > + UINT32 AdpEvten; > + UINT32 BcCfg; > + UINT32 Res10; > + UINT32 BcEvt; > + UINT32 BcEvten; > +} DWC3; > + > +#endif > diff --git a/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.c b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.c > new file mode 100644 > index 000000000000..40b027c639b1 > --- /dev/null > +++ b/Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.c > @@ -0,0 +1,212 @@ > +/** @file > + > + Copyright 2017, 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +#include "UsbHcd.h" > + > +STATIC > +VOID > +XhciSetBeatBurstLength ( > + IN UINTN UsbReg > + ) > +{ > + DWC3 *Dwc3Reg; > + > + Dwc3Reg = (VOID *)(UsbReg + DWC3_REG_OFFSET); > + > + MmioAndThenOr32 ((UINTN)&Dwc3Reg->GSBusCfg0, ~USB3_ENABLE_BEAT_BURST_MASK, > + USB3_ENABLE_BEAT_BURST); Indentation (align with the ( on the previous line, or with the 'i' in MmioAndThenOr32) > + MmioOr32 ((UINTN)&Dwc3Reg->GSBusCfg1, USB3_SET_BEAT_BURST_LIMIT); > + > + return; No return needed from functions returning VOID > +} > + > +STATIC > +VOID > +Dwc3SetFladj ( > + IN DWC3 *Dwc3Reg, > + IN UINT32 Val > + ) > +{ > + MmioOr32 ((UINTN)&Dwc3Reg->GFLAdj, GFLADJ_30MHZ_REG_SEL | > + GFLADJ_30MHZ(Val)); Indentation > +} > + STATIC? > +VOID > +Dwc3SetMode ( > + IN DWC3 *Dwc3Reg, > + IN UINT32 Mode > + ) > +{ > + MmioAndThenOr32 ((UINTN)&Dwc3Reg->GCtl, > + ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG)), > + DWC3_GCTL_PRTCAPDIR(Mode)); Indentation > +} > + > +STATIC > +VOID > +Dwc3CoreSoftReset ( > + IN DWC3 *Dwc3Reg > + ) > +{ > + MmioOr32 ((UINTN)&Dwc3Reg->GCtl, DWC3_GCTL_CORESOFTRESET); Are you sure you don't need a MemoryFence() here? > + MmioOr32 ((UINTN)&Dwc3Reg->GUsb3PipeCtl[0], DWC3_GUSB3PIPECTL_PHYSOFTRST); > + MmioOr32 ((UINTN)&Dwc3Reg->GUsb2PhyCfg, DWC3_GUSB2PHYCFG_PHYSOFTRST); .. or maybe here? > + MmioAnd32 ((UINTN)&Dwc3Reg->GUsb3PipeCtl[0], ~DWC3_GUSB3PIPECTL_PHYSOFTRST); > + MmioAnd32 ((UINTN)&Dwc3Reg->GUsb2PhyCfg, ~DWC3_GUSB2PHYCFG_PHYSOFTRST); > + MmioAnd32 ((UINTN)&Dwc3Reg->GCtl, ~DWC3_GCTL_CORESOFTRESET); > + > + return; Drop the return > +} > + > +STATIC > +EFI_STATUS > +Dwc3CoreInit ( > + IN DWC3 *Dwc3Reg > + ) > +{ > + UINT32 Revision; > + UINT32 Reg; > + UINTN Dwc3Hwparams1; > + > + Revision = MmioRead32 ((UINTN)&Dwc3Reg->GSnpsId); > + // > + // This should read as 0x5533, ascii of U3(DWC_usb3) followed by revision number > + // > + if ((Revision & DWC3_GSNPSID_MASK) != DWC3_SYNOPSYS_ID) { > + DEBUG ((DEBUG_ERROR,"This is not a DesignWare USB3 DRD Core.\n")); > + return EFI_NOT_FOUND; > + } > + > + Dwc3CoreSoftReset (Dwc3Reg); > + > + Reg = MmioRead32 ((UINTN)&Dwc3Reg->GCtl); > + Reg &= ~DWC3_GCTL_SCALEDOWN_MASK; > + Reg &= ~DWC3_GCTL_DISSCRAMBLE; > + > + Dwc3Hwparams1 = MmioRead32 ((UINTN)&Dwc3Reg->GHwParams1); > + > + if (DWC3_GHWPARAMS1_EN_PWROPT(Dwc3Hwparams1) == DWC3_GHWPARAMS1_EN_PWROPT_CLK) { > + Reg &= ~DWC3_GCTL_DSBLCLKGTNG; > + } else { > + DEBUG ((DEBUG_ERROR,"No power optimization available.\n")); If this 'error' condition requires no handling whatsoever, shouldn't it be a DEBUG_WARN instead? > + } > + > + if ((Revision & DWC3_RELEASE_MASK) < DWC3_RELEASE_190a) { > + Reg |= DWC3_GCTL_U2RSTECN; > + } > + > + MmioWrite32 ((UINTN)&Dwc3Reg->GCtl, Reg); > + > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > +XhciCoreInit ( > + IN UINTN UsbReg > + ) > +{ > + EFI_STATUS Status; > + DWC3 *Dwc3Reg; > + > + Dwc3Reg = (VOID *)(UsbReg + DWC3_REG_OFFSET); > + > + Status = Dwc3CoreInit (Dwc3Reg); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Dwc3CoreInit Failed for controller 0x%x (0x%x) \n", > + UsbReg, Status)); Indentation > + return Status; > + } > + > + Dwc3SetMode (Dwc3Reg, DWC3_GCTL_PRTCAP_HOST); > + > + Dwc3SetFladj (Dwc3Reg, GFLADJ_30MHZ_DEFAULT); > + > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +InitializeUsbController ( > + IN UINTN UsbReg > + ) > +{ > + EFI_STATUS Status; > + > + Status = XhciCoreInit (UsbReg); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Change beat burst and outstanding pipelined transfers requests > + // > + XhciSetBeatBurstLength (UsbReg); > + > + return Status; > +} > + > +/** > + The Entry Point of module. It follows the standard UEFI driver model. > + > + @param[in] ImageHandle The firmware allocated handle for the EFI image. > + @param[in] SystemTable A pointer to the EFI System Table. > + > + @retval EFI_SUCCESS The entry point is executed successfully. > + @retval other Some error occurs when executing this entry point. > + > +**/ > +EFI_STATUS > +EFIAPI > +InitializeUsbHcd ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + UINT32 NumUsbController; > + UINT32 ControllerAddr; > + UINT32 Index; > + > + Status = EFI_SUCCESS; > + NumUsbController = PcdGet32 (PcdNumUsbController); > + > + for (Index = 0; Index < NumUsbController; Index++) { > + ControllerAddr = PcdGet64 (PcdUsbBaseAddr) + > + (Index * PcdGet32 (PcdUsbSize)); > + > + Status = InitializeUsbController (ControllerAddr); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "USB Controller initialization Failed for %d (0x%x)\n", > + ControllerAddr, Status)); Indentation Also, use %r for status codes. > + continue; > + } > + > + Status = RegisterNonDiscoverableMmioDevice ( > + NonDiscoverableDeviceTypeXhci, > + NonDiscoverableDeviceDmaTypeNonCoherent, > + NULL, > + NULL, > + 1, > + ControllerAddr, PcdGet32 (PcdUsbSize) > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to register USB device (0x%x) with error 0x%x \n", > + ControllerAddr, Status)); Indentation Also, use %r for status codes. > + } > + } > + > + return Status; > +} > It would be better for this registration to occur in an EndOfDxe event notification callback - that way, the USB controller will not be connected recursively unless required for booting if the boot manager decides not to call EfiBootManagerConnectAll()