> -----Original Message-----
> From: Ni, Ray <ray...@intel.com>
> Sent: Monday, February 10, 2020 2:16 PM
> To: Javeed, Ashraf <ashraf.jav...@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 02/12]
> MdeModulePkg/PciBusDxe: Setup PCI Express init phase
>
> My proposal defines four phases: Initialize, Scan, Program, Finalize. The four
> phases are very similar to your December's patch with just one phase opt out.
>
> As below:
> ---BEGIN----
> Each feature needs to provide function pointers for each phase and NULL means
> the feature doesn't need to do anything in the specific phase.
> With that, we can define a structure:
> Typedef struct {
> BOOLEAN Enable;
> PCIE_FEATURE_INITILAIZE Initialize;
> PCIE_FEATURE_SCAN Scan;
> PCIE_FEATURE_PROGRAM Program;
> PCIE_FEATURE_FINALIZE Finalize;
> } PCIE_FEATURE_ENTRY;
>
> With that, we can define a module level global variable:
> PCIE_FEATURE_ENTRY mPcieFeatures[] = {
> { TRUE, MaxPayloadInitialize, MaxPayloadScan, MaxPayloadProgram,
> MaxPayloadFinalize},
> { TRUE, MaxReadRequestInitialize, MaxReadRequestScan,
> MaxReadRequestProgram, MaxReadRequestFinalize},
> { TRUE, NULL, NULL, RelaxOrderProgram, NULL},
> { TRUE, NULL, CompletionTimeoutScan, CompletionTimeoutProgram, NULL },
> ...
> };
>
> PCIE_FEATURE_ENTRY.Enable can be set to FALSE according to the platform
> policy.
>
> The enable of PCIE features can be written as a feature agnostic for-loop.
> This can make the new feature enabling code easy to add and review.
> ---END----
>
I have seen this, in essence the concept is adopted, but it has been further
enhanced to achieve the objectives.
> While your today's patch defines a different 5-phase as below.
> typedef enum {
//
// preprocessing applicable only to few PCI Express features to bind all
devices
// under the common root bridge device (root port), that would be useful to
align
// all devices with a common value. This would be optional phase based on the
// type of the PCI Express feature to be programmed based on platform policy
//
> PciExpressFeaturePreProcessPhase,
//
// mandatory phase to setup the PCI Express feature to its applicable
attribute,
// based on its device-specific platform policies, matching with its device
capabilities
//
> PciExpressFeatureSetupPhase,
//
// optional phase primarily to align all devices, specially required when PCI
// switch is present in the hierarchy, applicable to certain few PCI Express
// features only
//
> PciExpressFeatureEntendedSetupPhase,
//
// mandatory programming phase to complete the configuration of the PCI
Express
// features
//
> PciExpressFeatureProgramPhase,
//
// optional phase to clean up temporary buffers, like those that were prepared
// during the preprocessing phase above
//
> PciExpressFeatureEndPhase
> } PCI_EXPRESS_FEATURE_CONFIGURATION_PHASE;
>
> I don't think they are very similar. And your new proposal is far different
> from
> what you proposed in December's patch.
> Did you meet any issues that triggered this new proposal?
>
The details are as follows:-
(1) Your proposal of data structure PCIE_FEATURE_ENTRY will add up 1 byte for
Boolean and 8 * 4 bytes for 4 different types of function pointers. For 10 PCIe
features, it is going to take 330 bytes fixed size.
(2) I wanted to optimized to avoid fixed size, added the phase, and feature Id
enum (4 bytes each), with just single pointer to function for the feature
initialization phase into the data structure
PCI_EXPRESS_FEATURE_INITIALIZATION_POINT, since I know that many don't need all
the phases for initialization. So, the total size turn outs to be 400 bytes,
which is not what I had desired. But, I can further optimized the data
structure PCI_EXPRESS_FEATURE_INITIALIZATION_POINT so that its size can be 10
bytes. With this, for 10 PCIe features initialization the routines defined in
the "mPciExpressFeatureInitializationList " shall be 250 bytes, shall be less
compare to using the PCIE_FEATURE_ENTRY; this can be in next version.
(3) The pointer-to-function name to be used as subphases appears fine for PCIe
feature specific action, but not for general action items, like free up of
temporary memory allocated resources. Explicit declarations has helped in
optimizing a code for both feature specific and for common action items, which
the subsequent patches reflect those.
(4) The phase names are matching as per their scope of actions, whose comments
in the code section are also captured (above).
(5) As per the nature of the PCIe feature to align common value among all the
PCI devices, every node in the tree has to be traversed twice to arrive at
final value for the initialization (especially required for the PCIe switch
multi-port device, to connect to other EndPoint devices), and the next phase
would be the actual write to the PCI device. The phases for traversing are the
- PciExpressFeatureSetupPhase, PciExpressFeatureEntendedSetupPhase; and write
operation phase is - PciExpressFeatureProgramPhase.
(6) The preprocessing phase is - PciExpressFeaturePreProcessPhase, where the
resources are allocated specific to PCIe feature, that needs to be bind to
every root bridge device.
(7) The final phase is - PciExpressFeatureEndPhase; this phase is use for 2
purpose: (i) to invoke the NotifyDeviceState() to indicate to platform about
its current device state after the PCIe feature programming is complete, and
(ii) free up the resource allocated in the preprocessing phase.
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Javeed, Ashraf <ashraf.jav...@intel.com>
> > Sent: Monday, February 10, 2020 4:33 PM
> > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > <hao.a...@intel.com>
> > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > 02/12] MdeModulePkg/PciBusDxe: Setup PCI Express init phase
> >
> > Thanks
> > Ashraf
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray...@intel.com>
> > > Sent: Monday, February 10, 2020 1:07 PM
> > > To: Javeed, Ashraf <ashraf.jav...@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> > > <hao.a...@intel.com>
> > > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH
> > > 02/12]
> > > MdeModulePkg/PciBusDxe: Setup PCI Express init phase
> > >
> > > > > + Status = EnumeratePciExpressFeatures (
> > > 1. "enumerate" means "visit". But I think this function is not just
> > > visiting the features but also
> > > programming them. So, How about "ProgramPciExpressFeatures"?
> > > (I gave a similar review comment in last time review in Dec.)
> > >
> > Actually I have already used the "ProgramPciExpressFeatures" in one of
> > the sub-phases of the PCI Express initialization code...do you have any
> > other
> name for this? How about "ConfigurePciExpressFeatures"?
> >
> > > 2. In mail https://edk2.groups.io/g/devel/message/52399 I proposed
> > > to simplify to 4 phases.
> > > Did you find any issue with my proposal?
> > I did simplify to 4 phases, please check. The fifth phase is to report
> > out the device state to the platform through the protocol interface
> "NotifyDeviceState".
> >
> > >
> > > Thanks,
> > > Ray
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54187): https://edk2.groups.io/g/devel/message/54187
Mute This Topic: https://groups.io/mt/71063383/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
