Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31485 )
Change subject: soc/amd/common: Introduce module_dispatch() ......................................................................
Patch Set 5: Code-Review+2
(3 comments)
Looks good, but I'd prefer to get rid of the `void *` casting.
I'm a bit concerned by one of the calls to amd_late_run_ap_task(). I would expect agesa_RunFcnOnAllAps() to have its `Func` parameter set to `AGESA_RUNFUNC_ON_ALL_APS` but callout_ap_entry() explicitly checks for `AGESA_RUNFUNC_ONAP`. (beside my usual concern if it's possible to comply with the GPL when distributing this in compiled form along with a calling blob) With the limited scope of the global `agesadata` there, one consumer two providers, I don't think the `Func` field is useful at all.
https://review.coreboot.org/#/c/31485/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31485/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 45: StdHeader I would prefer not to add any more CamelCase.
https://review.coreboot.org/#/c/31485/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 62: AGESA_STATUS amd_late_run_ap_task(AP_EXE_PARAMS *ApExeParams) So this seems to be the exception why you set `StdHeader->Func` in module_dispatch(). This wasn't easy to grasp. Maybe make this more explicit:
static AGESA_STATUS amd_dispatch(void *Params) { MODULE_ENTRY dispatcher = agesa_get_dispatcher(); if (!dispatcher) return AGESA_UNSUPPORTED; return dispatcher(Params); }
AGESA_STATUS amd_late_run_ap_task(AP_EXE_PARAMS *ApExeParams) { ApExeParams->StdHeader.Func = AMD_LATE_RUN_AP_TASK; return amd_dispatch(ApExeParams); }
If I didn't miss anything, it should do the same with the excep- tion that `Func` gets also set when agesa_get_dispatcher() fails. But that was the old behavior anyway. It also avoids the implicit cast from `void *`.
https://review.coreboot.org/#/c/31485/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 65: StdHeader Simply `&ApExeParams->StdHeader` would avoid the casting.