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 3:
(8 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
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@40 PS2, Line 40:
odd whitespace
Done
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))
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/include/ahci.h@49 PS2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf)
when you compare chapter 3.1.5.6 and 3.1.5.1 it's clear that 0.95 is actually 0.9.5 as 1. […]
Done
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 67: BIOS_DEBUG
I think BIOS_INFO would suit most of the messages better.
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 73: ports
- 1
What I meant here is that the register value is 0-based, hence we need `ports + 1` here.
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 75: : enable power management
it's more than that
Done
https://review.coreboot.org/c/coreboot/+/37137/2/src/southbridge/intel/commo... PS2, Line 79: if (cccs && major >= 1 && minor >= 1)
|| major > 1 […]
Done