I added SataControllerDxe in DuetPkg to support AHCI for Duet that has chipset and platform specific initialization done before the Duet boot. It was thought to be hardly shared and used by real hardware platforms as the platforms (at least all I touched) have their own SataControllerDxe driver with some chipset policies or platform configurations.
Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Thursday, August 07, 2014 1:10 AM To: Ni, Ruiyu; Reza Jelveh Cc: [email protected]; [email protected] Subject: Re: [edk2] [PATCH 1/2] OvmfPkg: enable SATA controller (Ray, I'll ask a question below about SataControllerDxe, thanks for your attention.) On 08/06/14 18:16, [email protected] wrote: > From: Reza Jelveh <[email protected]> > > Replace the IdeController with a SataController that supports both in the > Ovmf firmware. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Reza Jelveh <[email protected]> > --- > OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++---- OvmfPkg/OvmfPkgX64.fdf | > 11 +++++++---- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index > 66459c2..b629a5e 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -78,6 +78,7 @@ > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf > DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf > > DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTabl > eLib.inf > + > + ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/Dx > + eExtractGuidedSectionLib.inf Reza, Which driver that you are introducing in this patch requires this? >From "MdePkg/Include/Library/ExtractGuidedSectionLib.h": This library provides common functions to process the different guided section data. This library provides functions to process GUIDed sections of FFS files. Handlers may be registered to decode GUIDed sections of FFS files. Services are provided to determine the set of supported section GUIDs, collection information about a specific GUIDed section, and decode a specific GUIDed section. A library instance that produces this library class may be used to produce a EFI_PEI_GUIDED_SECTION_EXTRACTION_PPI or a EFI_GUIDED_SECTION_EXTRACTION_PROTOCOL providing a simple method to extend the number of GUIDed sections types a platform supports. Actually, I can find this exact same library class --> library instance resolution in the DSC file elsewhere. The section that you are augmenting is [LibraryClasses] and the same class->instance resolution is already present under [LibraryClasses.common.DXE_CORE] The library makes perfect sense for the DXE core, but I can't see how anything related to a SATA driver would need it. If some other module type (ie. one of the drivers you are introducing) nonetheless needs the library class resolved, please state in the commit mesage which one. In addition, in that case you might want to add the resolution for only that one module type (eg. [LibraryClasses.common.DXE_DRIVER] or [LibraryClasses.common.UEFI_DRIVER]). Since the library instance "MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf" is not suitable for all module types, it probably shouldn't be under [LibraryClasses]. > > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf > PciCf8Lib|MdePkg/Library/BasePciCf8Lib/BasePciCf8Lib.inf > PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf > @@ -455,10 +456,6 @@ > MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf > MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf > > MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf > - MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf > - MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf > - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf > - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf > MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf > MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf > MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf > @@ -470,6 +467,15 @@ > } > > # > + # AHCI Support > + # > + DuetPkg/SataControllerDxe/SataControllerDxe.inf > + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf > + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf > + MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf > + MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf > + > + # > # ISA Support > # > PcAtChipsetPkg/IsaAcpiDxe/IsaAcpi.inf Moved around / preserved: - MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf - MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf Removed: - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf (provides gEfiDiskInfoProtocolGuid, gEfiBlockIoProtocolGuid) - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf (provides gEfiIdeControllerInitProtocolGuid) Added: - DuetPkg/SataControllerDxe/SataControllerDxe.inf (provides gEfiIdeControllerInitProtocolGuid) - MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf (provides gEfiAtaPassThruProtocolGuid, gEfiExtScsiPassThruProtocolGuid) - MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf (provides gEfiDiskInfoProtocolGuid, gEfiBlockIoProtocolGuid, gEfiBlockIo2ProtocolGuid, gEfiStorageSecurityCommandProtocolGuid) Quantity-wise, the available protocols don't seem to regress. But: - Please describe with some kind of tree diagram or indentation in the commit message how the new stuff stacks up. Reviewers shouldn't have to gather that information for you. - I strongly dislike the fact that you need to pull in a DuetPkg module (for gEfiIdeControllerInitProtocolGuid); that's a first for OVMF. If that module is universally usable, it should belong to MdeModulePkg. Ray, can you please enlighten me why SataControllerDxe is under DuetPkg and not MdeModulePkg? Then, > @@ -567,3 +573,4 @@ > !endif > > OvmfPkg/PlatformDxe/Platform.inf > + IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf No clue what this is good for. It seems to produce gEfiDataHubProtocolGuid, which is consumed by... what? Based on your recent patches, please be *much* more verbose in commit messages. Both the pre-patch and the post-patch protocol dependency tree should be described, with the relevant drivers identified. You can't avoid knowing all this information, because you fixed the dependencies one by one. You added the one driver that you cared about, and added the rest to resolve dependencies. Please capture that process in the commit message with the tree I mentioned. Next week please ping Jordan too for a review. Thanks, Laszlo ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
