Hi Star, Thank you for your feedback. I will submit a new patch with the issues addressed. Kindly ignore the v2 patch.
Regards, Sami Mujawar -----Original Message----- From: Zeng, Star <star.z...@intel.com> Sent: 19 June 2018 03:53 AM To: Sami Mujawar <sami.muja...@arm.com>; edk2-devel@lists.01.org Cc: ruiyu...@intel.com; Stephanie Hughes-Fitt <stephanie.hughes-f...@arm.com>; eric.d...@intel.com; ard.biesheu...@linaro.org; leif.lindh...@linaro.org; nd <n...@arm.com>; star.z...@intel.com Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space Hi Sami, My feedback are inline. On 2018/6/15 21:51, Sami Mujawar wrote: > Hi Zeng, > > Please find my response marked [SAMI] below. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Zeng, Star <star.z...@intel.com> > Sent: 15 June 2018 10:42 AM > To: Sami Mujawar <sami.muja...@arm.com>; edk2-devel@lists.01.org > Cc: ruiyu...@intel.com; nd <n...@arm.com>; Stephanie Hughes-Fitt > <stephanie.hughes-f...@arm.com>; eric.d...@intel.com; > ard.biesheu...@linaro.org; leif.lindh...@linaro.org; > star.z...@intel.com > Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller > PCI mem space > > Generally, the patch is good to me. > Some comments below. > > On 2018/6/14 19:38, Sami Mujawar wrote: >> The SATA controller driver crashes while accessing the PCI memory, as >> the PCI memory space is not enabled. > > The code "accessing the PCI memory" you mentioned here is the AhciReadReg in > the following code block, right? > [SAMI] Yes. >> >> Enable the PCI memory space access to prevent the SATA Controller >> driver from crashing. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Sami Mujawar <sami.muja...@arm.com> >> --- >> The changes can be seen at >> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f >> i >> x_v1 >> >> Notes: >> v1: >> - Fix SATA Controller driver crash >> [SAMI] >> >> MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 >> +++++++++++++++++++- >> MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 7 ++ >> 2 files changed, 86 insertions(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c >> b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c >> index >> a6d55c15571728eb3fd572003f383ba7c86635ae..21cc101d693f5adfd9d43f0c21a >> 0 >> 96eb59ba73b1 100644 >> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c >> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c >> @@ -2,6 +2,7 @@ >> This driver module produces IDE_CONTROLLER_INIT protocol for Sata >> Controllers. >> >> Copyright (c) 2011 - 2016, Intel Corporation. All rights >> reserved.<BR> >> + Copyright (c) 2018, ARM Ltd. All rights reserved.<BR> >> 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 at @@ -364,6 +365,7 @@ SataControllerStart ( >> EFI_SATA_CONTROLLER_PRIVATE_DATA *Private; >> UINT32 Data32; >> UINTN TotalCount; >> + UINT64 PciAttributes; >> >> DEBUG ((EFI_D_INFO, "SataControllerStart start\n")); >> >> @@ -406,6 +408,61 @@ SataControllerStart ( >> Private->IdeInit.CalculateMode = IdeInitCalculateMode; >> Private->IdeInit.SetTiming = IdeInitSetTiming; >> Private->IdeInit.EnumAll = SATA_ENUMER_ALL; >> + Private->PciAttributesChanged = FALSE; >> + >> + // Save original PCI attributes >> + Status = PciIo->Attributes ( >> + PciIo, >> + EfiPciIoAttributeOperationGet, >> + 0, >> + &Private->OriginalPciAttributes >> + ); >> + if (EFI_ERROR (Status)) { >> + goto Done; >> + } > > Good to me. > >> + >> + DEBUG (( >> + EFI_D_INFO, >> + "PCI Attributes = 0x%llx\n", > > How about using "Original PCI Attributes = 0x%llx\n"? > [SAMI] I will submit a patch with this fixed. > >> + Private->OriginalPciAttributes >> + )); >> + >> + if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) { >> + Status = PciIo->Attributes ( >> + PciIo, >> + EfiPciIoAttributeOperationSupported, >> + 0, >> + &PciAttributes >> + ); >> + if (EFI_ERROR (Status)) { >> + goto Done; >> + } >> + >> + DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", >> + PciAttributes)); >> + >> + if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) { >> + DEBUG (( >> + EFI_D_ERROR, >> + "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n" >> + )); >> + Status = EFI_UNSUPPORTED; >> + goto Done; >> + } >> + >> + PciAttributes = Private->OriginalPciAttributes | >> + EFI_PCI_IO_ATTRIBUTE_MEMORY; >> + >> + DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes)); >> + Status = PciIo->Attributes ( >> + PciIo, >> + EfiPciIoAttributeOperationEnable, >> + PciAttributes, >> + NULL >> + ); >> + if (EFI_ERROR (Status)) { >> + goto Done; >> + } > > It is the case for enabling memory space, but there may be case to need IO > space enabling. I suggest to use the code block (same with other device > drivers). > > Status = PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationSupported, > 0, > &Supports > ); > if (!EFI_ERROR (Status)) { > Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > Status = PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationEnable, > Supports, > NULL > ); > } > > [SAMI] This SATA Controller driver only uses the PCI BAR5 register space > which is the AHCI Base Address (ABAR). According to the 'Serial ATA Advanced > Host Controller Interface (AHCI) 1.3.1' specification, section 2.1.11, 'This > register allocates space for the HBA memory registers'. > The section 2.1.10, allows provision for Optional BARs which may support > either memory or I/O spaces. However, in the context of the current SATA > controller driver, which only ever access the ABAR, enabling I/O memory space > is not required. Yes, I did know AHCI has ABAR with memory space. But this SATA Controller driver also supports IDE mode that uses IO space. So I suggested to handle them both. The attributes has been reflected in Supports, the code can naturally enable the supported attributes. > >> + Private->PciAttributesChanged = TRUE; } >> >> Status = PciIo->Pci.Read ( >> PciIo, >> @@ -414,7 +471,10 @@ SataControllerStart ( >> sizeof (PciData.Hdr.ClassCode), >> PciData.Hdr.ClassCode >> ); >> - ASSERT_EFI_ERROR (Status); >> + if (EFI_ERROR (Status)) { >> + ASSERT (FALSE); >> + goto Done; >> + } >> >> if (IS_PCI_IDE (&PciData)) { >> Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL; @@ -481,6 >> +541,15 @@ Done: >> if (Private->IdentifyValid != NULL) { >> FreePool (Private->IdentifyValid); >> } >> + if (Private->PciAttributesChanged) { >> + // Restore original PCI attributes > > Prefer to use > > // > // Restore original PCI attributes > // > > [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding > Standard Specification, revision 2.20, section 6.2.3. Before we have clear direction about this to align code and CCS spec. I prefer to align with the surrounding code to use below style. // // xxxxx // > >> + PciIo->Attributes ( >> + PciIo, >> + EfiPciIoAttributeOperationSet, >> + Private->OriginalPciAttributes, >> + NULL >> + ); >> + } >> FreePool (Private); >> } >> } >> @@ -556,6 +625,15 @@ SataControllerStop ( >> if (Private->IdentifyValid != NULL) { >> FreePool (Private->IdentifyValid); >> } >> + if (Private->PciAttributesChanged) { >> + // Restore original PCI attributes > Prefer to use > > // > // Restore original PCI attributes > // > > [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding > Standard Specification, revision 2.20, section 6.2.3. Same with above. > >> + Private->PciIo->Attributes ( >> + Private->PciIo, >> + EfiPciIoAttributeOperationSet, >> + Private->OriginalPciAttributes, >> + NULL >> + ); >> + } >> FreePool (Private); >> } >> >> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h >> b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h >> index >> f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..cb82b55763a077f5994c4a00ea4 >> 8 >> 93bfa2e07a79 100644 >> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h >> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h >> @@ -2,6 +2,7 @@ >> Header file for Sata Controller driver. >> >> Copyright (c) 2011 - 2016, Intel Corporation. All rights >> reserved.<BR> >> + Copyright (c) 2017, ARM Ltd. All rights reserved.<BR> >> 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 at @@ -104,6 +105,12 @@ typedef struct >> _EFI_SATA_CONTROLLER_PRIVATE_DATA { >> // >> EFI_IDENTIFY_DATA *IdentifyData; >> BOOLEAN *IdentifyValid; >> + >> + /// Track the state so that the PCI attributes that were modified >> + /// can be restored to the original value later > > Prefer to use > > // > // Track the state so that the PCI attributes that were modified // > can be restored to the original value later // > > to align with others. > [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding > Standard Specification, revision 2.20, section 6.2.3. > Also '///' is used for documenting structure members. See EDK2 > Coding Standard Specification, revision 2.20, section 6.9. > I will submit an updated patch with '.' at the end to indicate > where the comment block ends. I was not saying using '///' is wrong here. As I know, '///' are mainly used in *public* header files. For this case, I prefer to align with the surrounding code to use below style. // // xxxxx // > >> + BOOLEAN PciAttributesChanged; >> + /// Copy of the original PCI Attributes > > Prefer to use > > // > // Copy of the original PCI Attributes // > > to align with others. > [SAMI] This would violate the edk2 coding standard. See EDK2 Coding Standard > Specification, revision 2.20, section 6.2.3. Same with above. Thanks, Star > >> + UINT64 OriginalPciAttributes; >> } EFI_SATA_CONTROLLER_PRIVATE_DATA; >> >> #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, >> EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE) >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
