Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57345 )
Change subject: driver/intel/pmc_mux/conn: Add type-c info to coreboot table ......................................................................
Patch Set 7:
(1 comment)
File src/drivers/intel/pmc_mux/conn/conn.c:
https://review.coreboot.org/c/coreboot/+/57345/comment/ae0e68b3_00f63b67 PS5, Line 115: mux = pmc->link_list->children;
How does that approach differ from incrementing the count in .final like it does now?
The functions are run in different boot phases in ramstage: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src.... So, .init() gets called before .final which gets called before .write_tables. The way this helps is:
.init --> Keeps incrementing total_port_count. By the end of .init phase, you will have the total number of type-c ports that are used by the mainboard.
.final --> There are two things required here: 1. If CBMEM does not already have an entry for `CBMEM_ID_TYPE_C_INFO`, then allocate space using total_port_count above. 2. Copy information about the current port into CBMEM and increment filled port count.
.write_tables --> This is not done as part of this driver but the write tables picks up the information in CBMEM and fills the coreboot table entry.
If you increment the count in .final, then you run into the problem where you do not know how much space to allocate in CBMEM. However, if you move the count increment to .init, then by the time you get to .final, you have the total port count available which can be used to allocate space in CBMEM. Does that make sense?
For my understanding, is the idea of writing to CBMEM to avoid the need for the callback, or to avoid the need for the space used by the new "static struct drivers_intel_pmc_mux_conn_config conn_info[MAX_TYPE_C_PORTS]" variable in the conn driver, or just because it's a more correct way to do it given the coreboot model?
What Tim said above.
BTW, I noticed you use hsl_orientation in your example implementation, but you suggested I change that field name away from hsl_orientation in an earlier CL, which is why it's now named data_orientation instead of hsl_orientation.
Sorry about the inconsistency. I was just copying parts of your code to prepare the above comment and I must have copied things from different revisions.