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

[Javeed, Ashraf]

> -----Original Message-----
> From: Ni, Ray <ray.ni at intel.com>
> Sent: Monday, February 10, 2020 2:08 PM
> To: Javeed, Ashraf <ashraf.javeed at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at 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 at intel.com>
> > Sent: Monday, February 10, 2020 4:26 PM
> > To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> > <hao.a.wu at 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 at intel.com>
> > > Sent: Monday, February 10, 2020 12:50 PM
> > > To: Javeed, Ashraf <ashraf.javeed at intel.com>; devel at edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> > > <hao.a.wu at 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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54176): https://edk2.groups.io/g/devel/message/54176
Mute This Topic: https://groups.io/mt/71063360/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-