Change in coreboot[master]: soc/intel/common/cse: Helper function to set D0I3 for all HECI devices

[Arthur Heymans (Code Review)]

Attention is currently required from:  Felix Singer, Subrata Banik, Tim Wawrzynczak, Angel Pons, Nick Vaccaro, Patrick Rudolph, EricR Lai.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60404 )

Change subject: soc/intel/common/cse: Helper function to set D0I3 for all HECI devices
......................................................................

Patch Set 2:

(1 comment)

File src/soc/intel/common/block/cse/cse_eop.c:

https://review.coreboot.org/c/coreboot/+/60404/comment/7f9cdc4a_f656c4d8 
PS2, Line 17: /* SoC callback to set D0I3 for all HECI devices */
            : __weak void soc_heci_set_d0i3(void) { /* no-op */ }
> > Is the weak empty function meaningful on most SoC's or should it be implemented by all and this is just a placeholder?
>
> Ideally should be implemented by other SoC and this is just a placeholder. For example: https://review.coreboot.org/c/coreboot/+/60406/2/src/soc/intel/alderlake/fin...
>
> I wished to make this mandatory for SoC that selects SOC_INTEL_COMMON_BLOCK_HECI_NOTIFY (skips FSP Notify APIs hence, coreboot need to put HECI devices into D0i3) but in that case we might need to guard function declaration as well. WDYT about this ?

I think it's best to avoid weak function as placeholders in this case. If you forgot about it (e.g. a future SoC) you'd have to find why it's not working at runtime. If there is a declaration without a definition, the compiler will pinpoint you towards the problem, before you even ran the image.

Why should the declaration be guarded? Only the function call needs to be.

if (CONFIG(SOC_INTEL_COMMON_BLOCK_HECI_NOTIFY))
        soc_heci_set_d0i3()

should work fine. Making a definition mandatory would be my preference.