Attention is currently required from: Angel Pons, Arthur Heymans, Evgeny Zinoviev, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49997 )
Change subject: sb/intel/bd82x6x: Clean up native DID message code ......................................................................
Patch Set 6: Code-Review+2
(6 comments)
File src/southbridge/intel/bd82x6x/early_me.c:
https://review.coreboot.org/c/coreboot/+/49997/comment/7394b5a6_346a3c5b PS6, Line 175: meDID = did.uma_base | (1 << 28);// | (1 << 23); Note to myself: this was just (brain)dead code. .init_done is already initialized.
https://review.coreboot.org/c/coreboot/+/49997/comment/65e7981e_8e609193 PS6, Line 138: BIOS_NOTICE BIOS_WARNING or _ERR would be better here, imho. If you want to unify the files, just leave it for now.
https://review.coreboot.org/c/coreboot/+/49997/comment/bbc3dbaa_9dbf026e PS6, Line 171: if (stopwatch_expired(&timer)) NB. Spec says to ignore the ack data in this case and continue. Overriding `hfs.ack_data` with `ME_HFS_ACK_CONTINUE` here would achieve that.
https://review.coreboot.org/c/coreboot/+/49997/comment/c60efc1d_55a137f9 PS6, Line 172: break; Please keep a delay here. Even if this works, it's possible that this loops permanently triggers an interrupt handler which may slow things down. Unlikely for this register interface, but generally good practice. The original 1ms seems excessive, MRC path uses 10us.
Spec says to time out after 5s btw.
https://review.coreboot.org/c/coreboot/+/49997/comment/c242edc2_e251cc74 PS6, Line 181: me_ack_values[hfs.ack_data]); Nit, does this fit one line?
https://review.coreboot.org/c/coreboot/+/49997/comment/9f05c81b_682e21da PS6, Line 194: reset = 0x06; Unless you want to keep it for now for unification, you could directly call system_reset() / full_reset() here (maybe even global_reset() if we have that yet).