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

[Evan Lloyd <Evan.Lloyd@arm.com>]

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.  

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)'
> >
> >