Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 15 June 2018 at 17:26, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Hi Ard, Zeng
>
>> -----Original Message-----
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Sent: 15 June 2018 15:17
>> To: Sami Mujawar <Sami.Mujawar@arm.com>
>> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric Dong
>> <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif Lindholm
>> <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>;
>> Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd
>> <Evan.Lloyd@arm.com>; Thomas Abraham <thomas.abraham@arm.com>;
>> nd <nd@arm.com>
>> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
>> space
>>
>> On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> 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 <sami.mujawar@arm.com>
>> > ---
>> > 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.