[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/meteorlake: Update Touch Controller UPD params

[Cliff Huang (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans, Dinesh Gehlot, Eran Mitrani, Eric Lai, Jakub Czapiga, Jingyuan Liang, Kapil Porwal, Kyoung Il Kim, Paul Menzel, Subrata Banik, Tarun.

Cliff Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81332?usp=email )

Change subject: soc/intel/meteorlake: Update Touch Controller UPD params
......................................................................

Patch Set 5:

(2 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/81332/comment/397551a7_ff124d18 :
PS4, Line 7: UDP
> typo? UPD

yes. typo. thx

File src/soc/intel/meteorlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/81332/comment/5fadc123_51cf43b0 :
PS2, Line 639: 	s_cfg->ThcAssignment[0] = THC_NONE;
> Using `#if` and `#endif` should be a last-resort measure as it prevents code from being build-tested […]

We have run into several cases that the design/support is added to new SOC, and some existing ones are removed. With if (CONFIG(build flag))/else statement, it requires both conditional code block to be present to compile. For instance, one of MTL example is GPMR. there is no PCH in MTL and GPMR moved to IOC. And there is no longer DMI interface. Therefore, the existing code for DMI such as PID_DMI no longer valid.

Intel SOC common code src/soc/intel/common/block/gpmr/gpmr.c

Initially:

uint32_t gpmr_read32(uint16_t offset)
{
        return pcr_read32(PID_DMI, offset);
}

Currently:
uint32_t gpmr_read32(uint16_t offset)
{
    if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC))       -> used in MTL and later.
    	return ioc_reg_write32(offset, val);      -> no such function in SOC prior to MTL.
    else
        return pcr_write32(PID_DMI, offset, val); -> No such PCR access and DMI PID in MTL and later.
}

But need to be changed to:

uint32_t gpmr_read32(uint16_t offset)
{
#if CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)
    return ioc_reg_write32(offset, val);
#else
    return pcr_write32(PID_DMI, offset, val);
#endif
}

The same thing applies to Intel driver code and the SOC code but have difference in various SKUs. One example I can think of is MTL UFS support.

Another hesitation to me of using if (CONFIG(build flag))/else statement is that it produces always true or false conditional logic. It does not make sense to me from the code maintenance aspect even though the complier will optimal it. I understand that using #if/#else/#endif makes code not structural but it does job of adding and removing design changes going forward, while leaving the older projects more intact. But, I do agree that #if/#else/#end should be avoided in other cases.

Going back to the initial comment on these ThCAssignment assigned to THC_NONE. These is merely initialized to not use and have thc_configuration to determine the value, along with is_devfn_enabled().