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:c06::231; helo=mail-io0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (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 25D1E22280C3E for ; Mon, 8 Jan 2018 07:00:11 -0800 (PST) Received: by mail-io0-x231.google.com with SMTP id 14so14219997iou.2 for ; Mon, 08 Jan 2018 07:05:21 -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=CPSc1ecoaDVnL83crbHQrxBJcQE72shSOE9v9X8I+xY=; b=jg9S2JaYIeizhmqgZBrcnljwmE/R1frHELWu0IgrrWTwcfsKGjEtsARsq9OfWvAchs PptgHK0iW192DHWBbrDJZ/xp9J2xVxdAJhv0eVyDT/thD5vqcyLNWi4rBdXA78T9qj5h teEGOzLXRxuK04m1nuuN7VrwnC6Zm2cidfd9c= 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=CPSc1ecoaDVnL83crbHQrxBJcQE72shSOE9v9X8I+xY=; b=VDniC3IzUCcQVmaq2trnffpi46j7R7LfAGVqOa/WQ4U6k8XfqJ7RfuRshAFJONjZ7Y 3pGgRVcO/QB2OJpuMjZ76/ay+5OzXvhVfNZxieQ7JesTbJYj5wKJe+B1IgugJ+/tYn6I mZgU2KuiodUwg48Pc/fvc0qx7SWePEDBqr81soDp6Yn9xSAjBbzwZ8LkNo7397+hxfqT Sj7TP7GPXCypTH1CLdoP8qjvmB+Ub18TW+Io+iP+GQPqsarXbNuWABEpsgJsMUaB1TTm IzF50++cj//1q0+/Jnfvw3cQdQ1HAxSHFftkt/tfi7+Qw4EkcZX5eAvAMBYznwKId/HR 2STw== X-Gm-Message-State: AKwxytetbpQcGTc67Lk7oo7wrPjhEOsI/zBZcjPC//Dj7AfGWhUNhy1W TW/vSr1cBbe7ZZDTbLZwnZKiwkZi0JTaHoVWYtuvTg== X-Google-Smtp-Source: ACJfBouMdrFzzMYsnt7PraJuyoH4SFn6LEdYYeJvOVuSrZg8BS3JRTLNpLgGYoWZ/ipHXm4qKywdWpjpmJtHMC6TbV0= X-Received: by 10.36.185.22 with SMTP id w22mr2081229ite.58.1515423920763; Mon, 08 Jan 2018 07:05:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.37.197 with HTTP; Mon, 8 Jan 2018 07:05:20 -0800 (PST) In-Reply-To: <1515426912-13557-2-git-send-email-meenakshi.aggarwal@nxp.com> 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: Mon, 8 Jan 2018 15:05:20 +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: Mon, 08 Jan 2018 15:00:12 -0000 Content-Type: text/plain; charset="UTF-8" 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 > > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.c b/Platform/NXP/Drivers/SataInitDxe/SataInit.c > new file mode 100644 > index 0000000..bac390b > --- /dev/null > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.c > @@ -0,0 +1,285 @@ > +/** @file > + This driver module adds SATA controller support. > + > + 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 > + 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "SataInit.h" > + > +STATIC VOID *mDriverEventRegistration; > + > +/** > + Read AHCI Operation register. > + > + @param PciIo The PCI IO protocol instance. > + @param Offset The operation register offset. > + > + @return The register content read. > +**/ > + > +UINT32 > +EFIAPI > +AhciReadReg ( > + IN EFI_PCI_IO_PROTOCOL *PciIo, > + IN UINT32 Offset > + ) > +{ > + UINT32 Data; > + > + ASSERT (PciIo != NULL); > + > + Data = 0; > + > + PciIo->Mem.Read ( > + PciIo, > + EfiPciIoWidthUint32, > + AHCI_BAR_INDEX, > + (UINT64) Offset, > + 1, > + &Data > + ); > + > + return Data; > +} > + > +/** > + Write AHCI Operation register. > + > + @param PciIo The PCI IO protocol instance. > + @param Offset The operation register offset. > + @param Data The data used to write down. > + > +**/ > +VOID > +EFIAPI > +AhciWriteReg ( > + IN EFI_PCI_IO_PROTOCOL *PciIo, > + IN UINT32 Offset, > + IN UINT32 Data > + ) > +{ > + ASSERT (PciIo != NULL); > + > + PciIo->Mem.Write ( > + PciIo, > + EfiPciIoWidthUint32, > + AHCI_BAR_INDEX, > + (UINT64) Offset, > + 1, > + &Data > + ); > + > + return; > +} > + > +STATIC > +VOID > +PciIoRegistrationEvent ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + EFI_STATUS Status; > + UINTN HandleCount; > + UINTN Address; > + UINT32 Count; > + UINT32 Data; > + UINT8 PciClass; > + UINT8 PciSubClass; > + EFI_PCI_IO_PROTOCOL *PciIo; > + EFI_HANDLE *HandleBuf; > + > + PciIo = NULL; > + > + Status = gBS->LocateHandleBuffer ( > + ByProtocol, > + &gEfiPciIoProtocolGuid, > + NULL, > + &HandleCount, > + &HandleBuf); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Sata controller is not able to locate gEfiPciIoProtocolGuid 0x%x\n", > + Status)); > + return; > + } > + > + for (Count = 0; Count < HandleCount; Count++) { > + Status = gBS->OpenProtocol ( > + HandleBuf[Count], > + &gEfiPciIoProtocolGuid, > + (VOID **) &PciIo, > + NULL, > + NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + if (EFI_ERROR (Status)) { > + continue; > + } > + > + // > + // Now further check the PCI header: Base class (offset 0x0B) and > + // Sub Class (offset 0x0A). This controller should be an Ide controller > + // > + Status = PciIo->Pci.Read ( > + PciIo, > + EfiPciIoWidthUint8, > + PCI_CLASSCODE_OFFSET + 2, > + 1, > + &PciClass > + ); > + if (EFI_ERROR (Status)) { > + continue; > + } > + > + Status = PciIo->Pci.Read ( > + PciIo, > + EfiPciIoWidthUint8, > + PCI_CLASSCODE_OFFSET + 1, > + 1, > + &PciSubClass > + ); > + if (EFI_ERROR (Status)) { > + continue; > + } > + > + // > + // Examine Ide PCI Configuration table fields > + // > + if ((PciClass != PCI_CLASS_MASS_STORAGE) || > + (PciSubClass != PCI_CLASS_MASS_STORAGE_SATADPA)) { > + continue; > + } > + > + Status = PciIo->Pci.Read ( > + PciIo, > + EfiPciIoWidthUint32, > + PCI_AHCI_BASE_ADDRESS, > + 1, > + &Address > + ); > + if (EFI_ERROR (Status)) { > + continue; > + } else if (Address == (UINTN)Context) { > + gBS->CloseEvent (Event); > + > + // > + // configuring Physical Control Layer parameters for Port 0 > + // > + AhciWriteReg (PciIo, SATA_PPCFG, PORT_PHYSICAL); > + > + // > + // This register controls the configuration of the > + // Transport Layer for Port 0 > + // Errata Description : The default Rx watermark value may be insufficient for some > + // hard drives and result in a false CRC or internal errors. > + // Workaround: Change PTC[RXWM] field at offset 0xC8 to 0x29. Do not change > + // the other reserved fields of the register. > + // > + > + Data = AhciReadReg (PciIo, SATA_PTC); > + if (PcdGetBool (PcdSataErratumA009185)) { > + Data |= PORT_RXWM; > + } else { > + Data |= PORT_TRANSPORT; > + } > + AhciWriteReg (PciIo, SATA_PTC, Data); > + > + break; > + } > + } > + > + gBS->FreePool (HandleBuf); > + > + return; > +} > + > +/** > + 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 > + } 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. > + } > + } > + > + 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 > + 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. > + > + **/ > + > +#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 > +# 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. > +# > +## > + > +[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|UINT64|0x00000113 > gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0080000000|UINT64|0x00000114 > gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0508000000|UINT64|0x00000115 > @@ -83,6 +83,8 @@ > gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x01000000|UINT64|0x0000012D > 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|0x00000275 > gNxpQoriqLsTokenSpaceGuid.PcdErratumA008336|FALSE|BOOLEAN|0x00000276 > gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|FALSE|BOOLEAN|0x00000277 > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554|FALSE|BOOLEAN|0x00000278 > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635|FALSE|BOOLEAN|0x00000279 > + gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA008402|FALSE|BOOLEAN|0x0000027A > > # > # Test PCDs > @@ -249,3 +254,8 @@ > # > gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cBus|0|UINT32|0x0000330 > gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cAddress|0|UINT32|0x0000331 > + > + # > + # SATA Pcds > + # > + gNxpQoriqLsTokenSpaceGuid.PcdNumSataController|0x0|UINT32|0x00000340 > 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.inf > + UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf > + NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf > > [LibraryClasses.common.SEC] > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > @@ -144,6 +146,7 @@ > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf > PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf > MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf > + NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf > Why do you need to add this twice? > [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/NonDiscoverablePciDeviceDxe.inf > + > + # > # Architectural Protocols > # > ArmPkg/Drivers/CpuDxe/CpuDxe.inf > -- > 1.9.1 >