Attention is currently required from: Angel Pons, Varshit Pandya.
Máté Kukri 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:
(7 comments)
Patchset:
PS5: Left some quick comments, I'll fix the issues late tonight, or tomorrow.
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/144a5c80_8907d081 : PS5, Line 290: die("Unknown GPIO chassis type\n");
Why not return some fallback default value instead? There's nothing stopping coreboot from continuin […]
I am fine with turning these into warning.
https://review.coreboot.org/c/coreboot/+/81529/comment/8e6f8503_b594f8fd : PS5, Line 355: case 3:
Would it make sense to use an enum? Or are these "type" numbers magic?
These are the same chassis type values the vendor firmware uses. I kept them to make it easier to cross reference in case any bugs pop up on the chassis types i dont have.
https://review.coreboot.org/c/coreboot/+/81529/comment/666812be_a92c9be7 : PS5, Line 368: die("Unknown chassis type\n");
Same here, is there any harm in not programming any table?
No table results in the fan running at a low constant speed.
https://review.coreboot.org/c/coreboot/+/81529/comment/0630ca2b_a6ba66bf : PS5, Line 371: if (CONFIG_MAX_CPUS > 2) {
This is a compile-time check. […]
The vendor firmware bases this check on "max core address MSR" > 2 (i dont remember the id off the top of my head). on my system that returns 7, despite having a quad core cpu.
I didn't check with a dual core, but it might be fine to remove this check as I don't think any supported cpu would return < 2
File src/mainboard/dell/optiplex_9020/sch5555_ec.h:
https://review.coreboot.org/c/coreboot/+/81529/comment/9cdfb485_13d732a4 : PS5, Line 3: #pragma once
Hmmm, I thought we were supposed to use traditional include guards. […]
Indeed, I missed that as this file copy pasted from my user-space utility.
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/df098fcb_8088718b : 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?
Vendor firmware doesn't use any delay either. LPC traffic shows a couple dozen iterations always being enough, so i think this is fine here.