Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37137 )
Change subject: sb/intel/common: Add AHCI code ......................................................................
Patch Set 2:
(12 comments)
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h File src/include/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@22 PS2, Line 22: #define AHCI_CAP_NP 0x1f please name it _MASK or something similar
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@40 PS2, Line 40: odd whitespace
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@48 PS2, Line 48: #define AHCI_VS_MAJOR(x) ((x >> 16) & 0xf) ((((x) >> 24) & 0xf) * 10 + (((x) >> 16) & 0xf))
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@49 PS2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf) ((x) & 0xf ? ((((x) >> 8) & 0xf) * 10 + ((x) & 0xf)) : (((x) >> 8) & 0xf))
At least this seems how they write it, no trailing 0. OTOH, this makes it harder for comparisons, so maybe we should just print 1.10 as the hardware reports instead of 1.1.
(Intel seems to love to make it confusing. I guess 0.95 should have been 0.9.5, then their counting would make sense.)
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.h:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 27: const u32 clear_vnd); Qualifications of the parameter type like the `const` here are meaningless in an interface. For the caller, it's just noise. (Exception are pointers to qualified types ofc., where the qualification becomes part of the pointer type.)
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... File src/southbridge/intel/common/ahci.c:
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 55: * Spec: "No other ABAR registers should be accessed before this." I can't find this quote?
Spec talks about software and I'm sure it doesn't apply to Intel firmware, rather AHCI drivers. It also says that AE depends on SAM which is only configured later.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 67: BIOS_DEBUG I think BIOS_INFO would suit most of the messages better.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 73: ports + 1
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 75: : enable power management it's more than that
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 79: if (cccs && major >= 1 && minor >= 1) || major > 1
Versions are easier to compare in an encoded form. For instance with
#define AHCI_VERSION (mj, mn) ((mj) << 16 | (mn)))
this could become
if (read32(abar + AHCI_VS) >= AHCI_VERSION(1, 10))
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 80: reg32 |= AHCI_CAP_CCCS; Note, this wasn't explicitly set for bd82x6x.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 102: if (major >= 1 && minor >= 2) { Hmmm, it seems ICH9 advertises 1.20, but doesn't document this register.