Re: [Linaro-uefi] [PATCH edk2-platforms v1 01/38] Silicon/Hisilicon: Modify the MRC interface for other module

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%27s-unkempt-git... 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