Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Hi Star, My sincere apologies for getting your name wrong. For some reason my email client displays your name as 'surname, name'. However, it is still my fault. I will ensure I address you correctly in the future. Hi Lazlo, Thanks for letting me know. Regards, Sami Mujawar -Original Message- From: Laszlo Ersek Sent: 18 June 2018 04:52 PM To: Sami Mujawar ; edk2-devel@lists.01.org Cc: ruiyu...@intel.com; nd ; Stephanie Hughes-Fitt ; eric.d...@intel.com; ard.biesheu...@linaro.org; leif.lindh...@linaro.org; star.z...@intel.com Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space off-topic: On 06/15/18 16:13, Sami Mujawar wrote: > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG] My understanding is that "Star" is Star's given name, and "Zeng" is his surname. Let's not start calling each other by surname, shall we? (Email addresses under @intel.com seem to follow the "given-name.surname" or "given-name.middle-initial.surname" pattern.) Thanks, Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Hi Sami,
I will have feedback in the V1 patch thread based on your response there.
Thanks,
Star
-Original Message-
From: Sami Mujawar [mailto:sami.muja...@arm.com]
Sent: Friday, June 15, 2018 10:14 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric ; Ni,
Ruiyu ; ard.biesheu...@linaro.org;
leif.lindh...@linaro.org; matteo.carl...@arm.com;
stephanie.hughes-f...@arm.com; evan.ll...@arm.com; thomas.abra...@arm.com;
n...@arm.com
Subject: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
The SATA controller driver crashes while accessing the PCI memory [AHCI Base
Registers (ABAR)], as the PCI memory space is not enabled.
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
---
The changes can be viewed at
https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2
Notes:
v2:
- Improved log message and code documentation based on feedback [SAMI]
- Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG]
- 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.[SAMI]
- Prefer to use // surrounding comments [ZENG]
- Doing this would violate the edk2 coding standard. See EDK2
Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI]
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..faf1b62e81000449e43e5298bb8ff885e48e4318
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.
+ Copyright (c) 2018, ARM Ltd. All rights reserved.
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;
UINT32Data32;
UINTN TotalCount;
+ UINT64PciAttributes;
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,
+>OriginalPciAttributes
+);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ DEBUG ((
+EFI_D_INFO,
+"Original PCI Attributes = 0x%llx\n",
+Private->OriginalPciAttributes
+));
+
+ if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationSupported,
+ 0,
+
+ );
+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
+
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Never mind. :) Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Evan Lloyd Sent: Tuesday, June 19, 2018 1:19 AM To: Laszlo Ersek ; Sami Mujawar ; edk2-devel@lists.01.org Cc: Ni, Ruiyu ; Stephanie Hughes-Fitt ; Dong, Eric ; ard.biesheu...@linaro.org; leif.lindh...@linaro.org; nd ; Zeng, Star Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space Hi Laszlo, Star. Looking at the e-mail thread, I think I also made the mistake of getting Star's name inverted. I'm sorry, especially as in this case it is not as unclear as some Chinese names can be for foreigners. I can only put this one down to approaching senility. Sorry, Star - I'll try not to do it again. Evan > -Original Message- > From: edk2-devel On Behalf Of Laszlo > Ersek > Sent: 18 June 2018 16:52 > To: Sami Mujawar ; edk2-devel@lists.01.org > Cc: ruiyu...@intel.com; Stephanie Hughes-Fitt f...@arm.com>; eric.d...@intel.com; ard.biesheu...@linaro.org; > leif.lindh...@linaro.org; nd ; star.z...@intel.com > Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller > PCI mem space > > off-topic: > > On 06/15/18 16:13, Sami Mujawar wrote: > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG] > > My understanding is that "Star" is Star's given name, and "Zeng" is > his surname. Let's not start calling each other by surname, shall we? > > (Email addresses under @intel.com seem to follow the "given- > name.surname" or "given-name.middle-initial.surname" pattern.) > > Thanks, > Laszlo > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Leif, I totally agree with you. Before we have clear direction about this to align code and CCS spec. I prefer to align with the immediately surrounding code. Thanks, Star -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif Lindholm Sent: Saturday, June 16, 2018 12:13 AM To: Evan Lloyd Cc: Ni, Ruiyu ; nd ; Stephanie Hughes-Fitt ; Dong, Eric ; Ard Biesheuvel ; edk2-devel@lists.01.org; Zeng, Star Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space On Fri, Jun 15, 2018 at 03:26:54PM +, Evan Lloyd wrote: > Hi Ard, Zeng > > > -Original Message- > > From: Ard Biesheuvel > > Sent: 15 June 2018 15:17 > > To: Sami Mujawar > > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric > > Dong ; Ruiyu Ni ; Leif > > Lindholm ; Matteo Carlini > > ; Stephanie Hughes-Fitt > > ; Evan Lloyd ; > > Thomas Abraham ; nd > > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem > > space > > > > On 15 June 2018 at 16:13, Sami Mujawar wrote: > > > The SATA controller driver crashes while accessing the PCI memory > > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. > > > > > > 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 > > > --- > > > The changes can be viewed at > > > > > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_ > > f > > i > > > x_v2 > > > > > > Notes: > > > v2: > > > - Improved log message and code documentation based on > > > feedback > > [SAMI] > > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE > > > [ZENG] > > > - 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. > > > [SAMI] > > > - Prefer to use // surrounding comments > > > [ZENG] > > > - Doing this would violate the edk2 coding standard. See EDK2 > > > Coding Standard Specification, revision 2.20, section 6.2.3. > > > [SAMI] > > > > > > > Please stop obsessing about the coding standard. The maintainer was > > quite clear what he wanted, and in the past, I have also indicated > > that for the ARM related packages, I prefer readability and > > consistency over adherence to a dubious standard. > > [[Evan Lloyd]] I'd like to make some points here: > 1.It is perfectly reasonable for Sami to explain why he has done > something a certain way - Zeng is then at liberty to respond > with his preference, but we do not (yet) know what that might > be. Yes we do. Zeng responded with that in the first instance. Sami then disputed that explicitly stated preference, referring to the coding standard. There was no lack of clarity in what Zeng wanted - so I remain unclear on what this is intending to achieve? > 2."readability and consistency" is the very purpose of any > coding standard. If consistency is valuable, doesn't that > apply across modules? I understand that it may be > particularly valuable for maintainers within their modules, > but the rest of us benefit from a consistent style - > especially when looking outside our normal demesne. At which point the rule of thumb is: aligning with the immediately surrounding code always trumps adhering to the specified style. Which is in itself trumped by the maintainer explicitly stating another preference. (Like here.) > 3.Applying a consistent Coding Standard across the whole project > is a necessary pre-condition for automated CI checks on new > submissions. I'd like to aim e.g. for an improved > patchcheck.py, but that requires consistency across modules, > at least for new code. Such a system will _never_ be fully automatable. I will _always_ ta
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Hi Laszlo, Star. Looking at the e-mail thread, I think I also made the mistake of getting Star's name inverted. I'm sorry, especially as in this case it is not as unclear as some Chinese names can be for foreigners. I can only put this one down to approaching senility. Sorry, Star - I'll try not to do it again. Evan > -Original Message- > From: edk2-devel On Behalf Of Laszlo > Ersek > Sent: 18 June 2018 16:52 > To: Sami Mujawar ; edk2-devel@lists.01.org > Cc: ruiyu...@intel.com; Stephanie Hughes-Fitt f...@arm.com>; eric.d...@intel.com; ard.biesheu...@linaro.org; > leif.lindh...@linaro.org; nd ; star.z...@intel.com > Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI > mem space > > off-topic: > > On 06/15/18 16:13, Sami Mujawar wrote: > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG] > > My understanding is that "Star" is Star's given name, and "Zeng" is his > surname. Let's not start calling each other by surname, shall we? > > (Email addresses under @intel.com seem to follow the "given- > name.surname" or "given-name.middle-initial.surname" pattern.) > > Thanks, > Laszlo > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
off-topic: On 06/15/18 16:13, Sami Mujawar wrote: > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG] My understanding is that "Star" is Star's given name, and "Zeng" is his surname. Let's not start calling each other by surname, shall we? (Email addresses under @intel.com seem to follow the "given-name.surname" or "given-name.middle-initial.surname" pattern.) Thanks, Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
On Fri, Jun 15, 2018 at 03:26:54PM +, Evan Lloyd wrote: > Hi Ard, Zeng > > > -Original Message- > > From: Ard Biesheuvel > > Sent: 15 June 2018 15:17 > > To: Sami Mujawar > > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong > > ; Ruiyu Ni ; Leif Lindholm > > ; Matteo Carlini ; > > Stephanie Hughes-Fitt ; Evan Lloyd > > ; Thomas Abraham ; > > nd > > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem > > space > > > > On 15 June 2018 at 16:13, Sami Mujawar wrote: > > > The SATA controller driver crashes while accessing the PCI memory > > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. > > > > > > 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 > > > --- > > > The changes can be viewed at > > > > > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f > > i > > > x_v2 > > > > > > Notes: > > > v2: > > > - Improved log message and code documentation based on feedback > > [SAMI] > > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE > > > [ZENG] > > > - 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. > > > [SAMI] > > > - Prefer to use // surrounding comments > > > [ZENG] > > > - Doing this would violate the edk2 coding standard. See EDK2 > > > Coding Standard Specification, revision 2.20, section 6.2.3. > > > [SAMI] > > > > > > > Please stop obsessing about the coding standard. The maintainer was quite > > clear what he wanted, and in the past, I have also indicated that for the > > ARM > > related packages, I prefer readability and consistency over adherence to a > > dubious standard. > > [[Evan Lloyd]] I'd like to make some points here: > 1.It is perfectly reasonable for Sami to explain why he has done > something a certain way - Zeng is then at liberty to respond > with his preference, but we do not (yet) know what that might > be. Yes we do. Zeng responded with that in the first instance. Sami then disputed that explicitly stated preference, referring to the coding standard. There was no lack of clarity in what Zeng wanted - so I remain unclear on what this is intending to achieve? > 2."readability and consistency" is the very purpose of any > coding standard. If consistency is valuable, doesn't that > apply across modules? I understand that it may be > particularly valuable for maintainers within their modules, > but the rest of us benefit from a consistent style - > especially when looking outside our normal demesne. At which point the rule of thumb is: aligning with the immediately surrounding code always trumps adhering to the specified style. Which is in itself trumped by the maintainer explicitly stating another preference. (Like here.) > 3.Applying a consistent Coding Standard across the whole project > is a necessary pre-condition for automated CI checks on new > submissions. I'd like to aim e.g. for an improved > patchcheck.py, but that requires consistency across modules, > at least for new code. Such a system will _never_ be fully automatable. I will _always_ take a 80 < x < 90 line over one that breaks up an output string or one that reduces readability more than having to side-scroll does. That doesn't make it worthless, far from it. > Note: I am not disputing the dubiosity of the CCS. It could be > improved (a lot). However it is all we have right now. Then a more constructive approach is to recognise that not a single one of the maintainers are appearing to be respecting the horror vacui rule and submit a patch against https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification to have section 6.2.3 deleted. (Much like I should at some point get around to have 5.4.2.2.2 deleted, or at least conditionalised to only apply for !ARM.) Regards, Leif > Regards, > Evan > > > > > > > > v1: > > > - Fix SATA Controller driver crash > > > [SAMI] > ... > > > -- > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
On 15 June 2018 at 17:26, Evan Lloyd wrote: > Hi Ard, Zeng > >> -Original Message- >> From: Ard Biesheuvel >> Sent: 15 June 2018 15:17 >> To: Sami Mujawar >> Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong >> ; Ruiyu Ni ; Leif Lindholm >> ; Matteo Carlini ; >> Stephanie Hughes-Fitt ; Evan Lloyd >> ; Thomas Abraham ; >> nd >> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem >> space >> >> On 15 June 2018 at 16:13, Sami Mujawar wrote: >> > The SATA controller driver crashes while accessing the PCI memory >> > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. >> > >> > 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 >> > --- >> > The changes can be viewed at >> > >> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f >> i >> > x_v2 >> > >> > Notes: >> > v2: >> > - Improved log message and code documentation based on feedback >> [SAMI] >> > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE >> > [ZENG] >> > - 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. >> > [SAMI] >> > - Prefer to use // surrounding comments >> > [ZENG] >> > - Doing this would violate the edk2 coding standard. See EDK2 >> > Coding Standard Specification, revision 2.20, section 6.2.3. >> > [SAMI] >> > >> >> Please stop obsessing about the coding standard. The maintainer was quite >> clear what he wanted, and in the past, I have also indicated that for the ARM >> related packages, I prefer readability and consistency over adherence to a >> dubious standard. > > [[Evan Lloyd]] I'd like to make some points here: > 1. It is perfectly reasonable for Sami to explain why he has done > something a certain way - Zeng is then at liberty to respond with his > preference, but we do not (yet) know what that might be. > Yes, we do. He already told us his preference. > 2. "readability and consistency" is the very purpose of any coding > standard. If consistency is valuable, doesn't that apply across modules? I > understand that it may be particularly valuable for maintainers within their > modules, but the rest of us benefit from a consistent style - especially when > looking outside our normal demesne. > > 3. Applying a consistent Coding Standard across the whole project is a > necessary pre-condition for automated CI checks on new submissions. I'd like > to aim e.g. for an improved patchcheck.py, but that requires consistency > across modules, at least for new code. > > Note: I am not disputing the dubiosity of the CCS. It could be improved (a > lot). However it is all we have right now. > OK, so you are arguing that we should adhere to the letter of the CCS even though there is consensus about its shortcomings and its detachment from reality? I think that is a waste of everybody's time. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
Hi Ard, Zeng > -Original Message- > From: Ard Biesheuvel > Sent: 15 June 2018 15:17 > To: Sami Mujawar > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong > ; Ruiyu Ni ; Leif Lindholm > ; Matteo Carlini ; > Stephanie Hughes-Fitt ; Evan Lloyd > ; Thomas Abraham ; > nd > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem > space > > On 15 June 2018 at 16:13, Sami Mujawar wrote: > > The SATA controller driver crashes while accessing the PCI memory > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. > > > > 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 > > --- > > The changes can be viewed at > > > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f > i > > x_v2 > > > > Notes: > > v2: > > - Improved log message and code documentation based on feedback > [SAMI] > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG] > > - 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.[SAMI] > > - Prefer to use // surrounding comments [ZENG] > > - Doing this would violate the edk2 coding standard. See EDK2 > > Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI] > > > > Please stop obsessing about the coding standard. The maintainer was quite > clear what he wanted, and in the past, I have also indicated that for the ARM > related packages, I prefer readability and consistency over adherence to a > dubious standard. [[Evan Lloyd]] I'd like to make some points here: 1. It is perfectly reasonable for Sami to explain why he has done something a certain way - Zeng is then at liberty to respond with his preference, but we do not (yet) know what that might be. 2. "readability and consistency" is the very purpose of any coding standard. If consistency is valuable, doesn't that apply across modules? I understand that it may be particularly valuable for maintainers within their modules, but the rest of us benefit from a consistent style - especially when looking outside our normal demesne. 3. Applying a consistent Coding Standard across the whole project is a necessary pre-condition for automated CI checks on new submissions. I'd like to aim e.g. for an improved patchcheck.py, but that requires consistency across modules, at least for new code. Note: I am not disputing the dubiosity of the CCS. It could be improved (a lot). However it is all we have right now. Regards, Evan > > > > v1: > > - Fix SATA Controller driver crash[SAMI] ... > > -- > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
On 15 June 2018 at 16:13, Sami Mujawar wrote:
> The SATA controller driver crashes while accessing the
> PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
> space is not enabled.
>
> 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
> ---
> The changes can be viewed at
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2
>
> Notes:
> v2:
> - Improved log message and code documentation based on feedback [SAMI]
> - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG]
> - 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.[SAMI]
> - Prefer to use // surrounding comments [ZENG]
> - Doing this would violate the edk2 coding standard. See EDK2
> Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI]
>
Please stop obsessing about the coding standard. The maintainer was
quite clear what he wanted, and in the past, I have also indicated
that for the ARM related packages, I prefer readability and
consistency over adherence to a dubious standard.
> 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..faf1b62e81000449e43e5298bb8ff885e48e4318
> 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.
> + Copyright (c) 2018, ARM Ltd. All rights reserved.
>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;
>UINT32Data32;
>UINTN TotalCount;
> + UINT64PciAttributes;
>
>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,
> +>OriginalPciAttributes
> +);
> + if (EFI_ERROR (Status)) {
> + goto Done;
> + }
> +
> + DEBUG ((
> +EFI_D_INFO,
> +"Original PCI Attributes = 0x%llx\n",
> +Private->OriginalPciAttributes
> +));
> +
> + if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +Status = PciIo->Attributes (
> + PciIo,
> + EfiPciIoAttributeOperationSupported,
> + 0,
> +
> + );
> +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
> +
