diff mbox series

[edk2,edk2-platforms,v1,01/38] Silicon/Hisilicon: Modify the MRC interface for other module

Message ID 20180724070922.63362-2-ming.huang@linaro.org
State New
Headers show
Series Upload for D06 platform | expand

Commit Message

Ming Huang July 24, 2018, 7:08 a.m. UTC
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 <zhouyou17@huawei.com>

Signed-off-by: Ming Huang <ming.huang@linaro.org>

---
 Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/UpdateAcpiTable.c     |   4 +-
 Silicon/Hisilicon/Drivers/Smbios/MemorySubClassDxe/MemorySubClass.c |  22 +-
 Silicon/Hisilicon/Include/Library/HwMemInitLib.h                    | 351 ++++----------------
 3 files changed, 74 insertions(+), 303 deletions(-)

-- 
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Aug. 2, 2018, 2:42 p.m. UTC | #1
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 <zhouyou17@huawei.com>

> Signed-off-by: Ming Huang <ming.huang@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

>  

>  typedef union {

>      struct {

> -- 

> 2.17.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ming Huang Aug. 5, 2018, 3:35 a.m. UTC | #2
在 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 <zhouyou17@huawei.com>
>> Signed-off-by: Ming Huang <ming.huang@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
>>
diff mbox series

Patch

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{
     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;
 
-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;
+};
+
+typedef struct _DDR_DIMM_DATA {
+    BOOLEAN         Status;
+    UINT8           DramType;           //Byte 2
+    UINT8           ModuleType;         //Byte 3
+    UINT8           BankNum;            //Byte 4,??DDR4,?????BankGroup??Bank??
+    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;
+}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;
+    MEMORY_DATA             mem;
+    NUMAINFO_DATA           NumaInfo[MAX_SOCKET][MAX_NUM_PER_TYPE];
+}GBL_INTERFACE, *pGBL_INTERFACE;
 
 typedef union {
     struct {