Attention is currently required from: Máté Kukri, Varshit Pandya.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization ......................................................................
Patch Set 5:
(6 comments)
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/93c6a2d1_db420594 : PS5, Line 290: die("Unknown GPIO chassis type\n"); Why not return some fallback default value instead? There's nothing stopping coreboot from continuing to boot, even if the fans don't cooperate.
https://review.coreboot.org/c/coreboot/+/81529/comment/152a91f0_b3ace8b6 : PS5, Line 355: case 3: Would it make sense to use an enum? Or are these "type" numbers magic?
https://review.coreboot.org/c/coreboot/+/81529/comment/2aefdcb5_bead46be : PS5, Line 368: die("Unknown chassis type\n"); Same here, is there any harm in not programming any table?
https://review.coreboot.org/c/coreboot/+/81529/comment/92684024_dba255a2 : PS5, Line 371: if (CONFIG_MAX_CPUS > 2) { This is a compile-time check. If this is about applying different values depending on the number of cores (or threads?) of the CPU, it would make more sense to query the information at runtime.
File src/mainboard/dell/optiplex_9020/sch5555_ec.h:
https://review.coreboot.org/c/coreboot/+/81529/comment/ccb820eb_39b954c1 : PS5, Line 3: #pragma once Hmmm, I thought we were supposed to use traditional include guards. Did the coding style change?
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/5add389b_b100fd0a : PS5, Line 51: for (size_t timeout = 0; timeout < 0xfff; ++timeout) : if (inb(SCH555x_EMI_IOBASE + 1) & 1) : break; No delay in the polling loop?