Sorry, I have totally forgotten this... Without this patch, could OvmfPkg Sata ControllerDxe driver runs correctly? If yes, I agree to drop this patch. (I double checked the AHCI spec, looks like CAP register could be read before setting AE bit)
Thanks Feng -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Monday, September 21, 2015 17:31 To: Tian, Feng; Justen, Jordan L; [email protected] Cc: Gabriel L. Somlo; Alexander Graf; Hannes Reinecke; Reza Jelveh; Kinney, Michael D Subject: Re: [edk2] [PATCH v4 06/10] OvmfPkg: SataControllerDxe: enable AHCI mode if IS_PCI_SATADPA() On 09/21/15 03:19, Tian, Feng wrote: > Hi, Laszlo > > Could you explain why the SataControllerDxe needs to touch > AHCI.GHC.AE? This bit would be set by > AtaAtapiPassThru.AhciModeInitialization(). That's right, it's done in AhciModeInitialization() -- I didn't miss that fact. The only reason I've included that setting in this patch series is because you mentioned it in: http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545/focus=10659 You wrote: "[...] enable BME/MSE/AE bit at SataControllerStart [...]" BME is "bus master enable", "MSE" is "memory space enable", and "AE" is "AHCI enable". I did think myself that setting AE on this level was redundant, but I wanted to follow your advice to the letter. If you think this setting is superfluous here, then: - I agree, and - I can simply drop this patch from the series (in fact, part of the incentive for keeping this change in a separate patch was exactly the possibility that it might prove redundant) Dropping the patch would be great, because it would also resolve Jordan's remark about the location of the R_AHCI_GHC macro & friends in the edk2 source tree. Thanks! Laszlo > > Thanks > Feng > > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Saturday, September 19, 2015 02:40 > To: Justen, Jordan L; [email protected] > Cc: Tian, Feng; Gabriel L. Somlo; Alexander Graf; Hannes Reinecke; > Reza Jelveh; Kinney, Michael D > Subject: Re: [edk2] [PATCH v4 06/10] OvmfPkg: SataControllerDxe: > enable AHCI mode if IS_PCI_SATADPA() > > On 09/18/15 20:30, Jordan Justen wrote: >> On 2015-09-15 19:57:26, Laszlo Ersek wrote: >>> The location of the Global HBA Control register, and the AHCI Enable >>> bit value come from the AHCI 1.3.1 spec. They are kept together with >>> the existing macros in the header file. >>> >>> This patch completes the implementation of the ideas that Feng >>> shared in >>> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545/focus=10659>. >>> >>> Cc: Alexander Graf <[email protected]> >>> Cc: Reza Jelveh <[email protected]> >>> Cc: Jordan Justen <[email protected]> >>> Cc: Hannes Reinecke <[email protected]> >>> Cc: Gabriel L. Somlo <[email protected]> >>> Cc: Feng Tian <[email protected]> >>> Suggested-by: Feng Tian <[email protected]> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <[email protected]> >>> --- >>> OvmfPkg/SataControllerDxe/SataController.h | 2 ++ >>> OvmfPkg/SataControllerDxe/SataController.c | 6 ++++++ >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/OvmfPkg/SataControllerDxe/SataController.h >>> b/OvmfPkg/SataControllerDxe/SataController.h >>> index e5b719e..96589ea 100644 >>> --- a/OvmfPkg/SataControllerDxe/SataController.h >>> +++ b/OvmfPkg/SataControllerDxe/SataController.h >>> @@ -40,6 +40,8 @@ extern EFI_COMPONENT_NAME2_PROTOCOL >>> gSataControllerComponentName2; #define R_AHCI_CAP 0x0 >>> #define B_AHCI_CAP_NPS (BIT4 | BIT3 | BIT2 | BIT1 | BIT0) // Number of >>> Ports >>> #define B_AHCI_CAP_SPM BIT17 // Supports Port Multiplier >>> +#define R_AHCI_GHC 0x4 >>> +#define B_AHCI_GHC_AE BIT31 >> >> Maybe use the same #define names as: >> >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h >> >> Or, why not create MdePkg/Include/IndustryStandard/Ahci.h? > > Sure -- Feng, can you please state what macros and types you'd like to see > moved from "MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h"? > > Mike, can you please state if you are okay with the MdePkg pathname suggested > by Jordan, and what timeframe would be possible for reviewing the new header > file after I submit it? > > Thanks > Laszlo > >> >> -Jordan >> >>> /// >>> /// AHCI each channel can have up to 1 device diff --git >>> a/OvmfPkg/SataControllerDxe/SataController.c >>> b/OvmfPkg/SataControllerDxe/SataController.c >>> index 275b527..6a4971d 100644 >>> --- a/OvmfPkg/SataControllerDxe/SataController.c >>> +++ b/OvmfPkg/SataControllerDxe/SataController.c >>> @@ -475,6 +475,12 @@ SataControllerStart ( >>> SataPrivateData->DeviceCount = IDE_MAX_DEVICES; >>> } else if (IS_PCI_SATADPA (&PciData)) { >>> // >>> + // Enable AHCI mode. >>> + // >>> + AhciWriteReg (PciIo, R_AHCI_GHC, >>> + AhciReadReg (PciIo, R_AHCI_GHC) | B_AHCI_GHC_AE); >>> + >>> + // >>> // Read Host Capability Register(CAP) to get Number of Ports(NPS) and >>> Supports Port Multiplier(SPM) >>> // NPS is 0's based value indicating the maximum number of ports >>> supported by the HBA silicon. >>> // A maximum of 32 ports can be supported. A value of '0h', >>> indicating one port, is the minimum requirement. >>> -- >>> 1.8.3.1 >>> >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel >> > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
