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 > > typedef union { > struct { > -- > 2.17.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
