[coreboot-gerrit] Change in coreboot[master]: soc/intel/alderlake: Refactor meminit code

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/alderlake: Refactor meminit code
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... 
File src/soc/intel/alderlake/meminit.c:

https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... 
PS2, Line 39: init_spd_upds
I have been thinking about this. What do you think about - probably flipping this around to handle the channels so that it can be used by any meminit* functions irrespective of the memory technology.

struct mem_channel_info {
       struct channel_to_spd_upd_masks {
            uint8_t dimm0_full_channel_mask;
            uint8_t dimm1_full_channel_mask;
            uint8_t dimm0_half_channel_mask;
            uint8_t dimm1_half_channel_mask;
       };
       struct mixed_topology_masks {
            uint8_t memory_down_mask;
            uint8_t sodimm_mask;
       }
} mem_channel_info[] = {
   [MEMTYPE_LP4X] = {
        .dimm0_full_channel_mask = 0xff, /* DIMM0 for all 8 channels are populated. */
        .dimm1_full_channel_mask = 0x0,  /* DIMM1 is not supported. */
        .dimm0_half_channel_mask = 0xf,  /* Lower half of the channels are populated. */
        .dimm1_half_channel_mask = 0x0,  /* DIMM1 is not supported. */
        .memory_down_mask = 0x0, /* Mixed topology is not supported. */
        .sodimm_mask = 0x0,      /* Mixed topology is not supported. */
   },
   [MEMTYPE_LP5X] = {
        ...
   },
   [MEMTYPE_DDR4] = {
        .dimm0_full_channel_mask = 0x11, /* Channel 0 <-> SPD index 0, Channel 1 <-> SPD index 4 */
        .dimm1_full_channel_mask = 0x11, /* Channel 0 <-> SPD index 0, Channel 1 <-> SPD index 4 */
        .dimm0_half_channel_mask = 0x1,  /* Only channel 0 is populated. */
        .dimm1_half_channel_mask = 0x1,  /* Only channel 0 is populated. */
        .memory_down_mask = 0x1, /* In mixed mode, channel 0 can only be memory down. */
        .sodimm_mask = 0x10,     /* In mixed mode, channel 1 can only be sodimm. */
   },
   [MEMTYPE_DDR5] = {
        ...
   },
};

and then we can have a init_spd() function:

static void init_spd(struct spd_info *spd_info, int max_channels, int mem_type, bool half_populated)
{
     struct mem_channel_info *channel_info = &mem_channel_info[mem_type];
     static uint32_t *dimm0_spd_ptrs = {
            &mem_cfg->MemorySpdPtr00,
            &mem_cfg->MemorySpdPtr02,
            ...
     };
     static uint32_t *dimm1_spd_ptrs = {
            &mem_cfg->MemorySpdPtr01,
            &mem_cfg->MemorySpdPtr03,
            ...
     };

if (MIXED OR MD topology)
        // Read MD SPD
     if (MIXED OR SODIMM topology)
        // Read SODIMM SPD

if (MIXED)
        // ensure lengths of md and sodimm spd are same

if (half_populated) {
        // use half channel masks
     } else {
        // use full channel masks
     }

for (i = 0; i < max_channels; i++) {
          dimm0_ptr = &dimm0_spd_ptrs[i];
          dimm1_ptr = &dimm1_spd_ptrs[i];
          disable_dimm_ptr = &disable_dimm_spd_ptrs[i];

dimm0_spd = get_spd(...);
          dimm1_spd = get_spd(...);

disable_dimm_mask = 0;

if (BIT(i) & dimm0_channel_spd_mask) {
                *dimm0_ptr = dimm0_spd;
          } else {
                *dimm0_ptr = 0;
                disable_dimm_mask |= DIMM0;
          }

if (BIT(i) & dimm1_channel_spd_mask) {
                *dimm1_ptr = dimm1_spd;
          } else {
                *dimm1_ptr = 0;
                disable_dimm_mask |= DIMM1;
          }

*disable_dimm_ptr = disable_dimm_mask;
     }
}

static uintptr_t get_spd(int channel, uint32_t md_spd, uint32_t sodimm_spd,
                         int topology, uint8_t md_mask, uint8_t sodimm_mask)
{
     if (topology == MIXED) {
         if (md_mask & BIT(channel))
             topology = MEMORY_DOWN;
         else if (sodimm_mask & BIT(channel))
             topology = SODIMM;
         else
             die(...);
     }

if (//MEMORY DOWN)
           return md_spd;
     if (//SODIMM)
           return sodimm_spd;
     die(...);
}

This probably doesn't reduce the overall code right now. But I am thinking of this change for 2 reasons:
1. Adding support for new memory types would be light weight. Adding a table for DDR5 for example instead of having multiple functions to format the inputs.

2. Long term, I am envisioning that this can be moved into common code for Intel SoCs so that we don't have to repeat this effort for every new generation. But, before we get there we need to arrive at an API that is flexible enough to work for any memory technology and FSP UPD expectations.