On 09/22/15 03:09, Tian, Feng wrote: > Sorry, I have totally forgotten this... > > Without this patch, could OvmfPkg Sata ControllerDxe driver runs correctly? > If yes, I agree to drop this patch.
Yes, it works. I've dropped the patch. > (I double checked the AHCI spec, looks like CAP register could be read before > setting AE bit) I agree; I had the same impression when I read the spec, while working on the patches. Thank you! Laszlo > > 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
