[coreboot-gerrit] Change in coreboot[master]: lib/spd_bin: Extend DDR4 spd information

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/6/src/lib/spd_bin.c 
File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/6/src/lib/spd_bin.c@37 
PS6, Line 37: 
            : 	const int spd_banks[8] = { 8, 16, 32, 64, -1, -1, -1, -1 };
            : 	const int ddr4_banks[10] = { 4, 8, -1, -1, 8, 16, -1, -1, 16, 32 };
            : 	const int spd_capmb[10] = { 1, 2, 4, 8, 16, 32, 64, 128, 48, 96 };
            : 	const int spd_rows[8]  = { 12, 13, 14, 15, 16, 17, 18, -1 };
            : 	const int spd_cols[8]  = { 9, 10, 11, 12, -1, -1, -1, -1 };
            : 	const int spd_ranks[8] = { 1, 2, 3, 4, 5, 6, 7, 8 };
            : 	const int spd_devw[8]  = { 4, 8, 16, 32, -1, -1, -1, -1 };
            : 	const int spd_busw[8]  = { 8, 16, 32, 64, -1, -1, -1, -1 };
            : 	char spd_name[DDR4_SPD_PART_LEN+1] = { 0 };
            : 
            : 	int banks = spd_banks[(spd[SPD_DENSITY_BANKS] >> 4) & 7];
            : 	int capmb = spd_capmb[spd[SPD_DENSITY_BANKS] & 7] * 256;
            : 	int rows  = spd_rows[(spd[SPD_ADDRESSING] >> 3) & 7];
            : 	int cols  = spd_cols[spd[SPD_ADDRESSING] & 7];
            : 	int ranks = spd_ranks[(spd[DDR3_ORGANIZATION] >> 3) & 7];
            : 	int devw  = spd_devw[spd[DDR3_ORGANIZATION] & 7];
            : 	int busw  = spd_busw[spd[DDR3_BUS_DEV_WIDTH] & 7];
            : 
I am wondering if we should reorganize this function to make it more clear. Right now, we read all these memory params assuming it is DDR3 and then re-read again if it turns out to be DDR4. How about having a function for each param that is required. Something like:

const char *spd_get_module_type_string(int dram_type)
{
     switch (dram_type) {
            case SPD_DRAM_DDR3:
                 return "DDR3";
            case SPD_DRAM_LPDDR3_INTEL:
            case SPD_DRAM_LPDDR3_JEDEC:
                 return "LPDDR3";
            case SPD_DRAM_DDR4:
                 return "DDR4";
     }
     return "UNKNOWN";
}

int spd_get_banks(uint8_t spd[], int dram_type)
{
    static const int spd_banks[8] = { ... };
    int spd_banks_idx = (spd[SPD_DENSITY_BANKS] >> 4) & 7;

if (spd_banks_idx >= ARRAY_SIZE(spd_banks))
         return -1;

return spd_banks[spd_banks_idx];
}

and so on. That way if a new memory type needs to be supported in the future, each function can be compared against the spec to decide what needs to be done for it. Thoughts?