From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22a; helo=mail-it0-x22a.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EFC902035D329 for ; Tue, 9 Jan 2018 00:21:17 -0800 (PST) Received: by mail-it0-x22a.google.com with SMTP id b5so10655119itc.3 for ; Tue, 09 Jan 2018 00:26:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Ct7muw1jSTsY+7YF2/G8z7qt37U/172qkubIFTHIMtQ=; b=NoeuYJpPz7vtYXnZH1H44t8eog/xZpEzSdn+g/Hp9o0m1LQ0U79Bsh66C+Uv+mFByJ qEdor/91ZDXyLO/+B3RRXX4A/6rVnOMJwQKky0dgCLelmRw2hqB4wI5djoNobu3CdWIK 5rVwt8ePGn6dR1A3FHZ5fhhpy1838earhpswU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Ct7muw1jSTsY+7YF2/G8z7qt37U/172qkubIFTHIMtQ=; b=D9t+Y3Jm4Ia8QLcDNLZ2io80OUndLu+ns8uq/y6BffFl6yfICL2P1Gspie0vLBcDce DFAUxBnSgiocFFbkbZAkszgGTziWicA0ZDrTzTQZNZ5DAtvuwAY/7ZTCR01DxO8McNnr GFigN/11tmqcMYkLJiMDrDhNOMobDXACi6CbJXAZMtdLjPcaCO/DglgfSMNSeVsE/mrD e7UtN4BJf5SVMYRHBOfzcNQjpKPPXqwQ7uiWlYgmSGx/6a1QoMQ07eD3RsHVzdc9KJs5 Mh5IwLUfP89wGbUnuc+GaFC1tpmHNDiZU8DiY134aaj1qfqAM8zywg+67GHSAQ4M0Fml l59g== X-Gm-Message-State: AKwxyteeigWicZYcOTEENz09HbrsT96EbEDnzO2adggPpEFv9sDMEZie JVaQrrfEaWoGheK74kchjfrhx8nUFkC3sDibahw7GQ== X-Google-Smtp-Source: ACJfBou05YZ5Jvxo9TnhRwkvnjw19N5ynFY9pvB5KHDIyKSlHcvBNU4xnTLuLAwOQkc2uil1GK1d72BedJIoIUpCYzg= X-Received: by 10.36.185.22 with SMTP id w22mr4563914ite.58.1515486387784; Tue, 09 Jan 2018 00:26:27 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.37.197 with HTTP; Tue, 9 Jan 2018 00:26:27 -0800 (PST) In-Reply-To: References: <1513945005-30002-1-git-send-email-meenakshi.aggarwal@nxp.com> <1515426912-13557-1-git-send-email-meenakshi.aggarwal@nxp.com> <1515426912-13557-2-git-send-email-meenakshi.aggarwal@nxp.com> From: Ard Biesheuvel Date: Tue, 9 Jan 2018 08:26:27 +0000 Message-ID: To: Meenakshi Aggarwal Cc: Leif Lindholm , "Kinney, Michael D" , "edk2-devel@lists.01.org" , Udit Kumar , Varun Sethi Subject: Re: [PATCH edk2-platforms v2 1/2] SATA : Added SATA controller driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jan 2018 08:21:18 -0000 Content-Type: text/plain; charset="UTF-8" On 9 January 2018 at 04:50, Meenakshi Aggarwal wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Monday, January 08, 2018 8:35 PM >> To: Meenakshi Aggarwal >> Cc: Leif Lindholm ; Kinney, Michael D >> ; edk2-devel@lists.01.org; Udit Kumar >> ; Varun Sethi >> Subject: Re: [PATCH edk2-platforms v2 1/2] SATA : Added SATA controller >> driver. >> >> Hi Meenakshi, >> >> This is looking much better - thanks for rewriting it. I do have some >> comments below >> >> On 8 January 2018 at 15:55, Meenakshi Aggarwal >> wrote: >> > This patch adds support of SATA controller, which >> > Initialize SATA controller, >> > apply platform specific errata and >> > Register itself as NonDiscoverableMmioDevice >> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Meenakshi Aggarwal >> > --- >> > Platform/NXP/Drivers/SataInitDxe/SataInit.c | 285 >> +++++++++++++++++++++++ >> > Platform/NXP/Drivers/SataInitDxe/SataInit.h | 36 +++ >> > Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf | 52 +++++ >> > Platform/NXP/NxpQoriqLs.dec | 14 +- >> > Platform/NXP/NxpQoriqLs.dsc | 13 ++ >> > 5 files changed, 398 insertions(+), 2 deletions(-) >> > create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInit.c >> > create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInit.h >> > create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf >> > .... >> > +/** >> > + 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 >> > +InitializeSataController ( >> > + IN EFI_HANDLE ImageHandle, >> > + IN EFI_SYSTEM_TABLE *SystemTable >> > + ) >> > +{ >> > + EFI_STATUS Status; >> > + UINT32 NumSataController; >> > + UINTN ControllerAddr; >> > + >> > + Status = EFI_SUCCESS; >> > + NumSataController = PcdGet32 (PcdNumSataController); >> > + >> > + // >> > + // Impact : The SATA controller does not detect some hard drives reliably >> with >> > + // the default SerDes register setting. >> > + // Workaround : write value 0x80104e20 to 0x1eb1300 (serdes 2) >> > + // >> > + if (PcdGetBool (PcdSataErratumA010554)) { >> > + BeMmioWrite32 ((UINTN)SERDES2_SATA_ERRATA, 0x80104e20); >> > + } >> > + >> > + // >> > + // Impact : Device may see false CRC errors causing unreliable SATA >> operation. >> > + // Workaround : write 0x80000000 to the address 0x20140520 (dcsr). >> > + // >> > + if (PcdGetBool (PcdSataErratumA010635)) { >> > + BeMmioWrite32 ((UINTN)DCSR_SATA_ERRATA, 0x80000000); >> > + } >> > + >> > + while (NumSataController) { >> > + NumSataController--; >> > + ControllerAddr = PcdGet32 (PcdSataBaseAddr) + >> > + (NumSataController * PcdGet32 (PcdSataSize)); >> > + >> > + Status = RegisterNonDiscoverableMmioDevice ( >> > + NonDiscoverableDeviceTypeAhci, >> > + NonDiscoverableDeviceDmaTypeNonCoherent, >> > + NULL, >> > + NULL, >> > + 1, >> > + ControllerAddr, PcdGet32 (PcdSataSize) >> > + ); >> > + >> > + if (EFI_ERROR (Status)) { >> > + DEBUG ((DEBUG_ERROR, "Failed to register SATA device (0x%x) with >> error 0x%x \n", >> > + ControllerAddr, Status)); >> >> Please don't use if/else for the expected path: instead, return here >> or goto the error/unwind code at the end of the function >> > In case of more than one controller, we cannot return/goto from here because there are chances that other controller might get register successfully. > So used if-else, please suggest the correct way. > Then use 'continue' In any case, all RegisterNonDiscoverableMmioDevice() does is install a protocol on a new handle, so it is unlikely to fail. You can just replace the error handling with ASSERT_EFI_ERROR(). >> > + } else { >> > + // >> > + // Register a protocol registration notification callback on the driver >> > + // binding protocol so we can attempt to connect to it as soon as it >> appears. >> > + // >> > + EfiCreateProtocolNotifyEvent ( >> > + &gEfiPciIoProtocolGuid, >> > + TPL_CALLBACK, >> > + PciIoRegistrationEvent, >> > + (VOID *)ControllerAddr, >> > + &mDriverEventRegistration); >> >> What is the point of this? AhciReadReg()/AhciWriteReg() can access >> ControllerAddr directly, so there is no reason to go through the PCI >> I/O protocol. >> > OK, i will check this. >> > + } >> > + } >> > + >> > + return Status; >> > +} >> > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.h >> b/Platform/NXP/Drivers/SataInitDxe/SataInit.h >> > new file mode 100644 >> > index 0000000..7fe6273 >> > --- /dev/null >> > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.h >> > @@ -0,0 +1,36 @@ >> > +/** @file >> > + Header file for Sata Controller initialization driver. >> > + >> > + Copyright 2017 NXP >> > + >> > + 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 >> > + >> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope >> nsource.org%2Flicenses%2Fbsd- >> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C1bf888 >> dbc6b34f8646fe08d556a93d5a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 >> %7C0%7C636510207254570536&sdata=hU2o5igZuy5SDt5emEUmAqhSn1gW9 >> H40OgvmH8gMn9k%3D&reserved=0 >> > + >> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" >> BASIS, >> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >> EXPRESS OR IMPLIED. >> > + >> > + **/ >> > + >> > +#ifndef _SATA_INIT_H_ >> > +#define _SATA_INIT_H_ >> > + >> > + >> > +#define AHCI_BAR_INDEX 0x05 >> > +// >> > +// Offset for AHCI base address in PCI Header >> > +// >> > +#define PCI_AHCI_BASE_ADDRESS 0x24 >> > + >> > +#define SATA_PPCFG 0xA8 >> > +#define SATA_PTC 0xC8 >> > + >> > +#define PORT_PHYSICAL 0xA003FFFE >> > +#define PORT_TRANSPORT 0x08000025 >> > +#define PORT_RXWM 0x08000029 >> > + >> > +#define DCSR_SATA_ERRATA 0x20140520 >> > +#define SERDES2_SATA_ERRATA 0x01eb1300 >> > + >> > +#endif >> > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf >> b/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf >> > new file mode 100644 >> > index 0000000..82535f4 >> > --- /dev/null >> > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf >> > @@ -0,0 +1,52 @@ >> > +## @file >> > +# Component description file for the Sata Controller initialization driver >> > +# >> > +# Copyright 2017 NXP >> > +# >> > +# 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 >> > +# >> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope >> nsource.org%2Flicenses%2Fbsd- >> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C1bf888 >> dbc6b34f8646fe08d556a93d5a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 >> %7C0%7C636510207254570536&sdata=hU2o5igZuy5SDt5emEUmAqhSn1gW9 >> H40OgvmH8gMn9k%3D&reserved=0 >> > +# >> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" >> BASIS, >> > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >> EXPRESS OR IMPLIED. >> > +# >> > +## >> > + >> > +[Defines] >> > + INF_VERSION = 0x0001000A >> > + BASE_NAME = SataInit >> > + FILE_GUID = 021722D8-522B-4079-852A-FE44C2C13F49 >> > + MODULE_TYPE = DXE_DRIVER >> > + VERSION_STRING = 1.0 >> > + ENTRY_POINT = InitializeSataController >> > + >> > +[Sources] >> > + SataInit.c >> > + >> > +[Packages] >> > + MdePkg/MdePkg.dec >> > + MdeModulePkg/MdeModulePkg.dec >> > + Platform/NXP/NxpQoriqLs.dec >> > + >> > +[LibraryClasses] >> > + BeIoLib >> > + DebugLib >> > + NonDiscoverableDeviceRegistrationLib >> > + UefiBootServicesTableLib >> > + UefiDriverEntryPoint >> > + UefiLib >> > + >> > +[FixedPcd] >> > + gNxpQoriqLsTokenSpaceGuid.PcdNumSataController >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataSize >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185 >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554 >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635 >> > + >> > +[Protocols] >> > + gEfiPciIoProtocolGuid >> > + >> > +[Depex] >> > + TRUE >> > diff --git a/Platform/NXP/NxpQoriqLs.dec b/Platform/NXP/NxpQoriqLs.dec >> > index bd4273f..65d659e 100644 >> > --- a/Platform/NXP/NxpQoriqLs.dec >> > +++ b/Platform/NXP/NxpQoriqLs.dec >> > @@ -52,8 +52,8 @@ >> > gNxpQoriqLsTokenSpaceGuid.PcdI2c1BaseAddr|0|UINT64|0x0000010E >> > gNxpQoriqLsTokenSpaceGuid.PcdI2c2BaseAddr|0|UINT64|0x0000010F >> > gNxpQoriqLsTokenSpaceGuid.PcdI2c3BaseAddr|0|UINT64|0x00000110 >> > - >> gNxpQoriqLsTokenSpaceGuid.PcdSataController1BaseAddress|0x0|UINT32| >> 0x00000111 >> > - >> gNxpQoriqLsTokenSpaceGuid.PcdSataController2BaseAddress|0x0|UINT32| >> 0x00000112 >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x0|UINT32|0x00000111 >> > + gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x0|UINT32|0x00000112 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0500000000|UINT6 >> 4|0x00000113 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0080000000|UINT64|0x0 >> 0000114 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0508000000|UINT64 >> |0x00000115 >> > @@ -83,6 +83,8 @@ >> > >> gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x01000000|UINT64|0x0000 >> 012D >> > >> gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0F000000|UINT64|0x0000012E >> > >> gNxpQoriqLsTokenSpaceGuid.PcdDramMemSize|0x0|UINT64|0x0000012F >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr|0x0|UINT64|0x00000130 >> > + gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize|0x0|UINT64|0x00000131 >> > >> > # >> > # DSPI Pcds >> > @@ -156,6 +158,9 @@ >> > >> gNxpQoriqLsTokenSpaceGuid.PcdErratumA008514|FALSE|BOOLEAN|0x0000 >> 0275 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdErratumA008336|FALSE|BOOLEAN|0x0000 >> 0276 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|FALSE|BOOLEAN|0x >> 00000277 >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554|FALSE|BOOLEAN|0x >> 00000278 >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635|FALSE|BOOLEAN|0x >> 00000279 >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA008402|FALSE|BOOLEAN|0x >> 0000027A >> > >> > # >> > # Test PCDs >> > @@ -249,3 +254,8 @@ >> > # >> > gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cBus|0|UINT32|0x0000330 >> > >> gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cAddress|0|UINT32|0x00003 >> 31 >> > + >> > + # >> > + # SATA Pcds >> > + # >> > + >> gNxpQoriqLsTokenSpaceGuid.PcdNumSataController|0x0|UINT32|0x000003 >> 40 >> > diff --git a/Platform/NXP/NxpQoriqLs.dsc b/Platform/NXP/NxpQoriqLs.dsc >> > index 10eff06..c3c0eb1 100644 >> > --- a/Platform/NXP/NxpQoriqLs.dsc >> > +++ b/Platform/NXP/NxpQoriqLs.dsc >> > @@ -99,6 +99,8 @@ >> > VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf >> > NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf >> > >> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.i >> nf >> > + UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf >> > + >> NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscove >> rableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf >> > >> > [LibraryClasses.common.SEC] >> > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf >> > @@ -144,6 +146,7 @@ >> > >> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL >> ib/DxeSecurityManagementLib.inf >> > >> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerforma >> nceLib.inf >> > MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf >> > + >> NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscove >> rableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf >> > >> >> Why do you need to add this twice? > My fault, will update. >> >> > [LibraryClasses.common.UEFI_APPLICATION] >> > PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> > @@ -334,6 +337,16 @@ >> > } >> > >> > # >> > + # AHCI Support >> > + # >> > + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> > + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> > + MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> > + MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> > + MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> > + >> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci >> DeviceDxe.inf >> > + >> > + # >> > # Architectural Protocols >> > # >> > ArmPkg/Drivers/CpuDxe/CpuDxe.inf >> > -- >> > 1.9.1 >> >