在 8/2/2018 10:42 PM, Leif Lindholm 写道:
> On Tue, Jul 24, 2018 at 03:08:45PM +0800, Ming Huang wrote:
>> This patch is to unify D0x. Add pGBL_INTERFACE struct define
>> and remove useless interfece. Replace DMRC pGblData with pGblInterface;
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Zhou You <zhouyo...@huawei.com>
>> Signed-off-by: Ming Huang <ming.hu...@linaro.org>
>> ---
>>  Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c     |   4 +-
>>  Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c |  22 +-
>>  Silicon/Hisilicon/Include/Library/HwMemInitLib.h                    | 351 
>> ++++----------------
> 
> Please add an orderfile as described in
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10
> in order to make api/stuct changes are visible before changes to their
> use.
> 
> You can set one permanently for your repository using
> $ git config diff.orderFile <full path to orderfile>
> 
>>  3 files changed, 74 insertions(+), 303 deletions(-)
>>
>> diff --git a/Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c 
>> b/Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
>> index 7d06fccc2b..f5869841dc 100644
>> --- a/Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
>> +++ b/Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c
>> @@ -56,7 +56,7 @@ UpdateSrat (
>>    UINT8               Skt = 0;
>>    UINTN               Index = 0;
>>    VOID                *HobList;
>> -  GBL_DATA            *Gbl_Data;
>> +  GBL_INTERFACE       *Gbl_Data;
>>    UINTN               Base;
>>    UINTN               Size;
>>    UINT8               NodeId;
>> @@ -69,7 +69,7 @@ UpdateSrat (
>>    if (HobList == NULL) {
>>      return EFI_UNSUPPORTED;
>>    }
>> -  Gbl_Data = (GBL_DATA*)GetNextGuidHob(&gHisiEfiMemoryMapGuid, HobList);
>> +  Gbl_Data = (GBL_INTERFACE*)GetNextGuidHob(&gHisiEfiMemoryMapGuid, 
>> HobList);
>>    if (Gbl_Data == NULL) {
>>      DEBUG((DEBUG_ERROR, "Get next Guid HOb fail.\n"));
>>      return EFI_NOT_FOUND;
>> diff --git 
>> a/Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c 
>> b/Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c
>> index da714c9e22..262b129419 100644
>> --- a/Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c
>> +++ b/Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c
>> @@ -45,7 +45,7 @@ SmbiosGetManufacturer (
>>  
>>  VOID
>>  SmbiosGetPartNumber (
>> -  IN pGBL_DATA          pGblData,
>> +  IN pGBL_INTERFACE     pGblData,
>>    IN UINT8              Skt,
>>    IN UINT8              Ch,
>>    IN UINT8              Dimm,
>> @@ -78,7 +78,7 @@ SmbiosGetPartNumber (
>>  
>>  VOID
>>  SmbiosGetSerialNumber (
>> -  IN pGBL_DATA          pGblData,
>> +  IN pGBL_INTERFACE     pGblData,
>>    IN UINT8              Skt,
>>    IN UINT8              Ch,
>>    IN UINT8              Dimm,
>> @@ -96,7 +96,7 @@ SmbiosGetSerialNumber (
>>  
>>  BOOLEAN
>>  IsDimmPresent (
>> -  IN  pGBL_DATA          pGblData,
>> +  IN  pGBL_INTERFACE     pGblData,
>>    IN  UINT8              Skt,
>>    IN  UINT8              Ch,
>>    IN  UINT8              Dimm
>> @@ -115,7 +115,7 @@ IsDimmPresent (
>>  
>>  UINT8
>>  SmbiosGetMemoryType (
>> -  IN  pGBL_DATA          pGblData,
>> +  IN  pGBL_INTERFACE     pGblData,
>>    IN  UINT8              Skt,
>>    IN  UINT8              Ch,
>>    IN  UINT8              Dimm
>> @@ -146,7 +146,7 @@ SmbiosGetMemoryType (
>>  
>>  VOID
>>  SmbiosGetTypeDetail (
>> -  IN  pGBL_DATA             pGblData,
>> +  IN  pGBL_INTERFACE        pGblData,
>>    IN  UINT8                 Skt,
>>    IN  UINT8                 Ch,
>>    IN  UINT8                 Dimm,
>> @@ -186,7 +186,7 @@ SmbiosGetTypeDetail (
>>  
>>  VOID
>>  SmbiosGetDimmVoltageInfo (
>> -  IN     pGBL_DATA             pGblData,
>> +  IN     pGBL_INTERFACE        pGblData,
>>    IN     UINT8                 Skt,
>>    IN     UINT8                 Ch,
>>    IN     UINT8                 Dimm,
>> @@ -281,7 +281,7 @@ SmbiosGetPartitionWidth (
>>  
>>  EFI_STATUS
>>  SmbiosAddType16Table (
>> -  IN  pGBL_DATA          pGblData,
>> +  IN  pGBL_INTERFACE     pGblData,
>>    OUT EFI_SMBIOS_HANDLE  *MemArraySmbiosHandle
>>    )
>>  {
>> @@ -345,7 +345,7 @@ SmbiosAddType16Table (
>>  
>>  EFI_STATUS
>>  SmbiosAddType19Table (
>> -  IN pGBL_DATA          pGblData,
>> +  IN pGBL_INTERFACE     pGblData,
>>    IN EFI_SMBIOS_HANDLE  MemArraySmbiosHandle
>>    )
>>  {
>> @@ -397,7 +397,7 @@ SmbiosAddType19Table (
>>  
>>  EFI_STATUS
>>  SmbiosAddType17Table (
>> -  IN pGBL_DATA          pGblData,
>> +  IN pGBL_INTERFACE     pGblData,
>>    IN UINT8              Skt,
>>    IN UINT8              Ch,
>>    IN UINT8              Dimm,
>> @@ -692,7 +692,7 @@ MemorySubClassEntryPoint(
>>      EFI_STATUS                      Status;
>>      EFI_SMBIOS_PROTOCOL             *Smbios;
>>      EFI_HOB_GUID_TYPE               *GuidHob;
>> -    pGBL_DATA                       pGblData;
>> +    pGBL_INTERFACE                  pGblData;
>>      EFI_SMBIOS_HANDLE               MemArraySmbiosHandle;
>>      UINT8                           Skt, Ch, Dimm;
>>  
>> @@ -702,7 +702,7 @@ MemorySubClassEntryPoint(
>>          DEBUG((EFI_D_ERROR, "Could not get MemoryMap Guid hob.  %r\n"));
>>          return EFI_NOT_FOUND;
>>      }
>> -    pGblData = (pGBL_DATA) GET_GUID_HOB_DATA(GuidHob);
>> +    pGblData = (pGBL_INTERFACE) GET_GUID_HOB_DATA(GuidHob);
>>  
>>      //
>>      // Locate dependent protocols
>> diff --git a/Silicon/Hisilicon/Include/Library/HwMemInitLib.h 
>> b/Silicon/Hisilicon/Include/Library/HwMemInitLib.h
>> index 2663cad836..2be90d35c7 100644
>> --- a/Silicon/Hisilicon/Include/Library/HwMemInitLib.h
>> +++ b/Silicon/Hisilicon/Include/Library/HwMemInitLib.h
>> @@ -50,48 +50,6 @@ typedef enum {
>>      DDR_FREQ_MAX
>>  } DDR_FREQUENCY_INDEX;
>>  
>> -typedef struct _DDR_FREQ_TCK
>> -{
>> -    UINT32      ddrFreq;
>> -    UINT32      ddrCk;
>> -}DDR_FREQ_TCK;
>> -
>> -typedef struct _GBL_CFG{
>> -
>> -
>> -}GBL_CFG;
>> -
>> -typedef struct _GBL_VAR{
>> -
>> -
>> -}GBL_VAR;
>> -
>> -typedef struct _GBL_NVDATA{
>> -
>> -
>> -}GBL_NVDATA;
>> -
>> -typedef struct _GOBAL {
>> -    const GBL_CFG Config;  // constant input data
>> -    GBL_VAR       Variable;    // variable, volatile data
>> -    GBL_NVDATA    NvData;  // variable, non-volatile data for S3, warm boot 
>> path
>> -    UINT32        PreBootFailed;
>> -}GOBAL, *PGOBAL;
>> -
>> -struct DDR_RANK {
>> -    BOOLEAN     Status;
>> -    UINT16      RttNom;
>> -    UINT16      RttPark;
>> -    UINT16      RttWr;
>> -    UINT16      MR0;
>> -    UINT16      MR1;
>> -    UINT16      MR2;
>> -    UINT16      MR3;
>> -    UINT16      MR4;
>> -    UINT16      MR5;
>> -    UINT16      MR6[9];
>> -};
>> -
>>  struct baseMargin {
>>    INT16 n;
>>    INT16 p;
>> @@ -101,171 +59,7 @@ struct rankMargin {
>>    struct baseMargin rank[MAX_CHANNEL][MAX_RANK_CH];
>>  };
>>  
>> -typedef struct _DDR_DIMM{
>> -    BOOLEAN     Status;
>> -    UINT8       mapout;
>> -    UINT8       DramType;           //Byte 2
>> -    UINT8       ModuleType;         //Byte 3
>> -    UINT8       ExtendModuleType;
>> -    UINT8       SDRAMCapacity;      //Byte 4
>> -    UINT8       BankNum;
>> -    UINT8       BGNum;              //Byte 4 For DDR4
>> -    UINT8       RowBits;            //Byte 5
>> -    UINT8       ColBits;            //Byte 5
>> -    UINT8       SpdVdd;             //Byte 6
>> -    UINT8       DramWidth;          //Byte 7
>> -    UINT8       RankNum;            //Byte 7
>> -    UINT8       PrimaryBusWidth;    //Byte 8
>> -    UINT8       ExtensionBusWidth;  //Byte 8
>> -    UINT32      Mtb;
>> -    UINT32      Ftb;
>> -    UINT32      minTck;
>> -    UINT8       MtbDividend;
>> -    UINT8       MtbDivsor;
>> -    UINT8       nCL;
>> -    UINT32      nRCD;
>> -    UINT32      nRP;
>> -    UINT8       SPDftb;
>> -    UINT8       SpdMinTCK;
>> -    UINT8       SpdMinTCKFtb;
>> -    UINT8       SpdMaxTCK;
>> -    UINT8       SpdMinTCL;
>> -    UINT8       SpdMinTCLFtb;
>> -    UINT8       SpdMinTWR;
>> -    UINT8       SpdMinTRCD;
>> -    UINT8       SpdMinTRCDFtb;
>> -    UINT8       SpdMinTRRD;
>> -    UINT8       SpdMinTRRDL;
>> -    UINT16      SpdMinTRAS;
>> -    UINT16      SpdMinTRC;
>> -    UINT16      SpdMinTRCFtb;
>> -    UINT16      SpdMinTRFC;
>> -    UINT8       SpdMinTWTR;
>> -    UINT8       SpdMinTRTP;
>> -    UINT8       SpdMinTAA;
>> -    UINT8       SpdMinTAAFtb;
>> -    UINT8       SpdMinTFAW;
>> -    UINT8       SpdMinTRP;
>> -    UINT8       SpdMinTRPFtb;
>> -    UINT8       SpdMinTCCDL;
>> -    UINT8       SpdMinTCCDLFtb;
>> -    UINT8       SpdAddrMap;
>> -    UINT8       SpdModuleAttr;
>> -
>> -    UINT8       SpdModPart[SPD_MODULE_PART];         // Module Part Number
>> -    UINT8       SpdModPartDDR4[SPD_MODULE_PART_DDR4];     // Module Part 
>> Number DDR4
>> -    UINT16      SpdMMfgId;              // Module Mfg Id from SPD
>> -    UINT16      SpdRMId;              // Register Manufacturer Id
>> -    UINT16      SpdMMDate;              // Module Manufacturing Date
>> -    UINT32      SpdSerialNum;
>> -    UINT16      DimmSize;
>> -    UINT16      DimmSpeed;
>> -    UINT32      RankSize;
>> -    UINT8       SpdMirror; //Denote the dram address mapping is standard 
>> mode or mirrored mode
>> -    struct DDR_RANK  Rank[MAX_RANK_DIMM];
>> -}DDR_DIMM;
>> -
>> -typedef struct {
>> -    UINT32      ddrcTiming0;
>> -    UINT32      ddrcTiming1;
>> -    UINT32      ddrcTiming2;
>> -    UINT32      ddrcTiming3;
>> -    UINT32      ddrcTiming4;
>> -    UINT32      ddrcTiming5;
>> -    UINT32      ddrcTiming6;
>> -    UINT32      ddrcTiming7;
>> -    UINT32      ddrcTiming8;
>> -}DDRC_TIMING;
>> -
>> -typedef struct _MARGIN_RESULT{
>> -    UINT32 OptimalDramVref[12];
>> -    UINT32 optimalPhyVref[18];
>> -}MARGIN_RESULT;
>> -
>> -typedef struct _DDR_Channel{
>> -    BOOLEAN     Status;
>> -    UINT8       CurrentDimmNum;
>> -    UINT8       CurrentRankNum;
>> -    UINT16      RankPresent;
>> -    UINT8       DramType;
>> -    UINT8       DramWidth;
>> -    UINT8       ModuleType;
>> -    UINT32      MemSize;
>> -    UINT32      tck;
>> -    UINT32      ratio;
>> -    UINT32      CLSupport;
>> -    UINT32      minTck;
>> -    UINT32      taref;
>> -    UINT32      nAA;
>> -    UINT32      nAOND;
>> -    UINT32      nCKE;
>> -    UINT32      nCL;
>> -    UINT32      nCCDL;
>> -    UINT32      nCKSRX;
>> -    UINT32      nCKSRE;
>> -    UINT32      nCCDNSW;
>> -    UINT32      nCCDNSR;
>> -    UINT32      nFAW;
>> -    UINT32      nMRD;
>> -    UINT32      nMOD;
>> -    UINT32      nRCD;
>> -    UINT32      nRRD;
>> -    UINT32      nRRDL;
>> -    UINT32      nRAS;
>> -    UINT32      nRC;
>> -    UINT32      nRFC;
>> -    UINT32      nRFCAB;
>> -    UINT32      nRTP;
>> -    UINT32      nRTW;
>> -    UINT32      nRP;
>> -    UINT32      nSRE;
>> -    UINT32      nWL;
>> -    UINT32      nWR;
>> -    UINT32      nWTR;
>> -    UINT32      nWTRL;
>> -    UINT32      nXARD;
>> -    UINT32      nZQPRD;
>> -    UINT32      nZQINIT;
>> -    UINT32      nZQCS;
>> -    UINT8       cwl;  //tWL?
>> -    UINT8       pl;     //parity latency
>> -    UINT8       wr_pre_2t_en;
>> -    UINT8       rd_pre_2t_en;
>> -    UINT8       cmd_2t_en;
>> -    UINT8       parity_en;
>> -    UINT8       wr_dbi_en;
>> -    UINT8       wr_dm_en;
>> -    UINT8       ddr4_crc_en;
>> -    UINT16      emrs0;
>> -    UINT16      emrs1;
>> -    UINT16      emrs1Wr;
>> -    UINT16      emrs2;
>> -    UINT16      emrs3;
>> -    UINT16      emrs4;
>> -    UINT16      emrs5;
>> -    UINT16      emrs5Wr;
>> -    UINT16      emrs6;
>> -    UINT16      emrs7;
>> -    UINT8       phy_rddata_set;
>> -    UINT8       phyif_tim_rdcs;
>> -    UINT8       phyif_tim_rden;
>> -    UINT8       phyif_tim_wden;
>> -    UINT8       phyif_tim_wdda;
>> -    UINT8       phyif_tim_wdcs;
>> -    UINT8       per_cs_training_en;
>> -    UINT32      phyRdDataEnIeDly;
>> -    UINT32      phyPadCalConfig;
>> -    UINT32      phyDqsFallRiseDelay;
>> -    UINT32      ddrcCfgDfiLat0;
>> -    UINT32      ddrcCfgDfiLat1;
>> -    UINT32      parityLatency;
>> -    UINT32      dimm_parity_en;
>> -    DDRC_TIMING ddrcTiming;
>> -    DDR_DIMM    Dimm[MAX_DIMM];
>> -    MARGIN_RESULT sMargin;
>> -}DDR_CHANNEL;
>> -
>> -typedef struct _NVRAM_RANK{
>> +typedef struct _NVRAM_RANK_DATA{
> 
> Space before {
> (Applies throughout, but please don't fix up structs otherwise
> untouched by this patch. Unless you do it as a separate patch
> preceding this one.)
> 
>>      UINT16      MR0;
>>      UINT16      MR1;
>>      UINT16      MR2;
>> @@ -273,15 +67,15 @@ typedef struct _NVRAM_RANK{
>>      UINT16      MR4;
>>      UINT16      MR5;
>>      UINT16      MR6[9];
>> -}NVRAM_RANK;
>> +}NVRAM_RANK_DATA;
> 
> Similarly, space after {.
> Also applies throughout.
> 
>>  
>> -typedef struct _NVRAM_DIMM{
>> -    NVRAM_RANK      Rank[MAX_RANK_DIMM];
>> -}NVRAM_DIMM;
>> +typedef struct _NVRAM_DIMM_DATA{
>> +    NVRAM_RANK_DATA      Rank[MAX_RANK_DIMM];
>> +}NVRAM_DIMM_DATA;
>>  
>>  
>> -typedef struct _NVRAM_CHANNEL{
>> -    NVRAM_DIMM      Dimm[MAX_DIMM];
>> +typedef struct _NVRAM_CHANNEL_DATA{
>> +    NVRAM_DIMM_DATA      Dimm[MAX_DIMM];
>>      UINT32          DDRC_CFG_ECC;
>>      UINT32          DDRC_CFG_WORKMODE;
>>      UINT32          DDRC_CFG_WORKMODE1;
>> @@ -325,94 +119,71 @@ typedef struct _NVRAM_CHANNEL{
>>      UINT32          DDRC_CFG_DDRPHY;
>>      UINT32          Config[24];
>>      BOOLEAN         Status;
>> -}NVRAM_CHANNEL;
>> +}NVRAM_CHANNEL_DATA;
>> +
>> +typedef struct _NVRAM_DATA{
>> +    UINT32              NvramCrc;
>> +    NVRAM_CHANNEL_DATA  Channel[MAX_SOCKET][MAX_CHANNEL];
>> +    UINT32              DdrFreqIdx;
>>  
>> -typedef struct _NVRAM{
>> -    UINT32          NvramCrc;
>> -    NVRAM_CHANNEL   Channel[MAX_SOCKET][MAX_CHANNEL];
>> -    UINT32          DdrFreqIdx;
>> +}NVRAM_DATA;
>>  
>> -}NVRAM;
>> +struct DDR_RANK_DATA {
>> +    BOOLEAN     Status;
> 
> Status is not a boolean thing.
> A boolean is for "active" or "enabled" or something suchlike, so you
> can write
>   if (entry->enabled) {
> .
> 
> Whereas
>   if (entry->status) {
> carries no meaning that can be discerned without fully reading through
> all code touching that variable.
> 
>> +};
>> +
>> +typedef struct _DDR_DIMM_DATA {
>> +    BOOLEAN         Status;
> 
> Status is not a boolean thing.
> 
>> +    UINT8           DramType;           //Byte 2
>> +    UINT8           ModuleType;         //Byte 3
>> +    UINT8           BankNum;            //Byte 
>> 4,??DDR4,?????BankGroup??Bank??
> 
> Some encoding issue in comment.
> 
>> +    UINT8           RowBits;            //Byte 5
>> +    UINT8           ColBits;            //Byte 5
>> +    UINT8           SpdVdd;             //Byte 6
>> +    UINT8           RankNum;            //Byte 7
>> +    UINT8           PrimaryBusWidth;    //Byte 8
>> +    UINT8           ExtensionBusWidth;  //Byte 8
>> +    UINT8           SpdModPart[SPD_MODULE_PART];                // Module 
>> Part Number
>> +    UINT8           SpdModPartDDR4[SPD_MODULE_PART_DDR4];       // Module 
>> Part Number DDR4
>> +    UINT16          SpdMMfgId;              // Module Mfg Id from SPD
>> +    UINT32          SpdSerialNum;
>> +    UINT32          RankSize;
>> +    UINT16          DimmSize;
>> +    UINT16          DimmSpeed;
>> +    UINT16          SpdMMDate;
>> +    struct DDR_RANK_DATA    Rank[MAX_RANK_DIMM];
>> +}DDR_DIMM_DATA;
>> +
>> +typedef struct _DDR_CHANNEL_DATA {
>> +    BOOLEAN         Status;
>> +    DDR_DIMM_DATA   Dimm[MAX_DIMM];
>> +    UINT8           CurrentDimmNum;
>> +}DDR_CHANNEL_DATA;
>>  
>> -typedef struct _MEMORY{
>> -    UINT8           Config0;
>> -    UINT8           marginTest;
>> -    UINT8           Config1[5];
>> -    UINT8           ErrorBypass; //register of spd mirror mode
>> -    UINT32          Config2;
>> -}MEMORY;
>> +typedef struct _MEMORY_DATA {
>> +    UINT8           rascBypass;
> 
> RascBypass.
> 
>> +}MEMORY_DATA;
>>  
>> -typedef struct _NUMAINFO{
>> +typedef struct _NUMAINFO_DATA {
>>      UINT8           NodeId;
>>      UINT64          Base;
>>      UINT64          Length;
>>      UINT32          ScclInterleaveEn;
>> -}NUMAINFO;
>> +}NUMAINFO_DATA;
>>  
>>  
>> -typedef struct _GBL_DATA
>> +typedef struct _GBL_DATA_INTERFACE
>>  {
>> -    DDR_CHANNEL Channel[MAX_SOCKET][MAX_CHANNEL];
>> -    UINT8       DramType;
>> -    UINT8       CurrentDimmNum;
>> -    UINT8       CurrentRankNum;
>> -    UINT8       MaxSPCNum;
>> -    UINT32      Freq;
>> -    UINT32      SpdTckMtb;
>> -    UINT32      SpdTckFtb;
>> -    UINT32      SpdTck;
>> -    UINT32      Tck;
>> -    UINT32      DdrFreqIdx;
>> -    UINT32      DevParaFreqIdx; //Maximum frequency of DDR device
>> -    UINT32      MemSize;
>> -    UINT32      EccEn;
>> -
>> -    BOOLEAN     SetupExist;
>> -    UINT8       warmReset;
>> -    UINT8       needColdReset;
>> -
>> -    UINT8       cl;
>> -    UINT8       cwl;
>> -    UINT8       pl;
>> -    UINT8       wr_pre_2t_en;
>> -    UINT8       rd_pre_2t_en;
>> -    UINT8       cmd_2t_en;
>> -    UINT8       ddr4_parity_en;
>> -    UINT8       wr_dbi_en;
>> -    UINT8       wr_dm_en;
>> -    UINT8       ddr4_crc_en;
>> -    UINT16      emrs0;
>> -    UINT16      emrs1;
>> -    UINT16      emrs2;
>> -    UINT16      emrs3;
>> -    UINT16      emrs4;
>> -    UINT16      emrs5;
>> -    UINT16      emrs6;
>> -    UINT16      emrs7;
>> -    UINT8       phy_rddata_set;
>> -    UINT8       phyif_tim_rdcs;
>> -    UINT8       phyif_tim_rden;
>> -    UINT8       phyif_tim_wden;
>> -    UINT8       phyif_tim_wdda;
>> -    UINT8       phyif_tim_wdcs;
>> -    UINT8       dimm_trtr;
>> -    UINT8       dimm_twtw;
>> -    UINT8       rnk_trtr;
>> -    UINT8       rnk_twtw;
>> -    UINT8       rnk_trtw;
>> -    UINT8       rnk_twtr;
>> -    UINT8       per_cs_training_en;
>> -    UINT8       scale;
>> -    UINT8       ddrFreq;
>> -    UINT8       debugNeed;
>> -    UINT8       ddr3OdtEnable;
>> -    double      fprd;
>> -    BOOLEAN     chipIsEc;
>> -    NVRAM       nvram;
>> -    MEMORY      mem;
>> -    NUMAINFO    NumaInfo[MAX_SOCKET][MAX_NUM_PER_TYPE];
>> -
>> -}GBL_DATA, *pGBL_DATA;
>> +    DDR_CHANNEL_DATA        Channel[MAX_SOCKET][MAX_CHANNEL];
>> +    UINT32                  DdrFreqIdx;
>> +    UINT32                  Freq;
>> +    UINT32                  EccEn;
>> +    UINT32                  MemSize;
>> +    BOOLEAN                 SetupExist;
>> +    NVRAM_DATA              nvram;
> 
> A bit too short a name. (Yes, I know it the code being removed already
> had this, but I was too stressed when I reviewed that.)
> 
> I think this should be NvRamInfo or NvRamData.
> 
>> +    MEMORY_DATA             mem;
> 
> As above, too short a name.
> Something descriptive please.
> 
>> +    NUMAINFO_DATA           NumaInfo[MAX_SOCKET][MAX_NUM_PER_TYPE];
>> +}GBL_INTERFACE, *pGBL_INTERFACE;
> 
> Please drop the *pGBL_INTERFACE typedef, it removes standardised
> semantic information provided by C and reduces readability.
> 
> /
>     Leif
> 

Thanks for review detailed. All comments will be applied in v2.

Ming

>>  
>>  typedef union {
>>      struct {
>> -- 
>> 2.17.0
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to