CedarIsland wrong FSP UPD struct typedefs

[ArthurHeymans]

The SOC specific UPDs have consistently been packed in a struct with a typedef FSP_M_CONFIG. CedarIsland breaks this by calling it FSPM_CONFIG. The same holds true for FSP-T and FSP-S. Please rename those as it makes use of these headers more complicated in projects that expect the former name.

[zaolin]

@nate-desimone What's going on here?

[nate-desimone]

This change was intentional. All of the FSP specification defined UPD structures follow this convention. For example, consider the following structure definitions in section 6.1 of the FSP EAS:

• FSPT_UPD
• FSPT_ARCH_UPD
• FSPM_UPD
• FSPM_ARCH_UPD
• FSPS_UPD
• FSPS_ARCH_UPD

The name FSPM_CONFIG aligns more closely with this pattern than FSP_M_CONFIG. As such, this change is considered desirable and will not be reverted.

[ArthurHeymans]

I see. It would be a good idea to align other platforms too. Can it be concluded that the CedarIsland FSP was never validated with coreboot as coreboot currently does not compile with the headers as is?

[nate-desimone]

My understanding is that the coreboot port to the Delta Lake board uses the Cedar Island FSP posted here and was validated using this Cedar Island FSP. I find it a little surprising that coreboot does not compile with these UPD headers.

[nate-desimone]

Anyway, section 6.1.2 of the FSP EAS states that the FSPM_UPD structure has the following format:

typedef struct {
    FSP_UPD_HEADER UpdHeader;
    FSPM_ARCH_UPD  FspmArchUpd;

    /**
    Platform specific parameters
    **/
    ...
} FSPM_UPD;

typedef struct {
    UINT8             Revision;
    UINT8             Reserved[3];
    VOID              *NvsBufferPtr;
    VOID              *StackBase;
    UINT32            StackSize;
    UINT32            BootLoaderTolumSize;
    UINT32            BootMode;
    FSP_EVENT_HANDLER FspEventHandler;
    UINT8 Reserved1[4];
} FSPM_ARCH_UPD;

The spec places no requirements on the platform specific parameters being contained in a sub-struct, or the name of the sub-struct, etc. Therefore, coreboot assuming the existence of FSP_M_CONFIG is a spec violation.

[ArthurHeymans]

It's indeed not in the spec, there just happens to be a typedef FSP_M_CONFIG for all FSP releases except CedarIsland. This inconsistency is just inconvenient for firmware projects like coreboot that develop all supported platforms under one tree as opposed to in branches. The alternative is to achieve a pointer to platforms specific parameters is to use some ugly pointer arithmetics. Maybe you can improve upon the FSP spec to avoid that bad practice and introduce a consistent name for platform specific parameters struct typedef?

[ArthurHeymans]

I made a workaround this issue: https://review.coreboot.org/c/coreboot/+/57487/2