Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12] MdeModulePkg/PciBusDxe: Setup for PCI Express features

["Javeed, Ashraf" <ashraf.javeed@intel.com>]

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, February 10, 2020 2:08 PM
> To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12]
> MdeModulePkg/PciBusDxe: Setup for PCI Express features
> 
> 
> 
> > -----Original Message-----
> > From: Javeed, Ashraf <ashraf.javeed@intel.com>
> > Sent: Monday, February 10, 2020 4:26 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > 01/12] MdeModulePkg/PciBusDxe: Setup for PCI Express features
> >
> > My comments below.
> >
> > Thanks
> > Ashraf
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Monday, February 10, 2020 12:50 PM
> > > To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>
> > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > > 01/12]
> > > MdeModulePkg/PciBusDxe: Setup for PCI Express features
> > >
> > > > > +  PCI_CAPABILITY_PCIEXP                     PciExpressCapabilityStructure;
> > > 1. To Align with existing field "Pci", how about rename it to
> > >    "PciExpressCapability" (no "Structure" suffix)?
> > >
> > As per PCI Express Base Specification, the feature name is "PCI
> > Express Capability Structure" and I have used the same type although
> > the data structure defined for the same has not used it. Is this really important
> that I have to remove the "Structure" from its name?
> I prefer to use shorter name if possible and it also aligns to the existing code
> style.
> Is it really that important that you keep the "Structure" to align to the PCIE spec?
> 😊
I think it is better to have a right name here.
> 
> > >
> > > 2. I see that only GetPciExpressProtocol() in PciPlatformSupport.c
> > > is used in this patch.
> > >     All other functions in PciFeatureSupport.c
> > > andPciPlatformSupport.c are not used.
> > >     It makes the reviewers confused about how those unused functions
> > > can be used.
> > >     You should remove these unused functions in this patch and add
> > > them in later patches
> > >     when the code logic calls them.
> > >
> > All the other main routines of the PciPlatformSupport.c are called from the
> code of PciFeatureSupport.c.
> > All the routines of the PciFeatureSupport.c are called from within
> > itself, and its main routine that defines the integration of PCI Express feature
> initialization is defined in the following second patch.
> > If required, I can combine both the patches into one patch, which
> > gives the basic structure of the PCI Express feature initialization in the
> PciBusDxe.
> Are all the routines in the two C files called from the main routine in PciBus
> driver?
> The answer with this patch is NO.
> The guideline is one patch serves one purpose and it's better to be as small as
> possible so that the change can be easier to review.
> With this guideline, combining both patches to one big one is not preferred.
> 
The purpose of the patch was to set up general code infrastructure, so that the PCI Express feature initialization routines can be added in the following patches.
If I have to break this code infrastructure into smaller patches than not all the routines are expected to be called from the main routine of the PciBusDxe. From this development, only 2 routines are called from the main routine of the PciBusDxe, hence the first 2 patches. If I have to break down further than don’t expect that all the routines shall be called from main routine.
From the developer perspective this task of breaking the code into smaller patches is very difficult. This actually delays the main goal of the changes.

> > > Thanks,
> > > Ray