Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
Uploaded patch set 7.
(35 comments)
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG Commit Message:
PS6:
would be good if you added the corresponding document id (332691-003EN)
Done
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG@9 PS6, Line 9: Kabylake
Kaby Lake
Done
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG@23 PS6, Line 23:
please add the Intel document number that you used
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/ahci.c File util/inteltool/ahci.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/ahci.c@124 PS6, Line 124: case 8: : printf("0x%04x: 0x%08x (%s)\n" : " 0x%08x\n", : ahci_cfg_registers[i].addr, : pci_read_long(ahci, ahci_cfg_registers[i].addr), : ahci_cfg_registers[i].name, : pci_read_long(ahci, ahci_cfg_registers[i].addr+4)); : break;
not needed
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/ahci.c@164 PS6, Line 164: 0xa0
add a define for this register?
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c File util/inteltool/apic.c:
PS6:
just using the read8/16/32 functions would also make this nicer
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@24 PS6, Line 24: #define IO_APIC_ADDR 0xfec00000
is this a really fixed location or only a typical location and needs to be detected?
I got this from src/arch/x86/include/arch/ioapic.h
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@33 PS6, Line 33: *((volatile u32 *)(ioapic_base)) = reg;
use read32 from arch/mmio. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@53 PS6, Line 53: rte_stepping
rte_step_size or rte_element_size maybe? i know "stepping" mainly as mask fix revision number of CPU […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@55 PS6, Line 55: : case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have smbus.\n"); : return 1;
this seems wrong (or at least unnecessary) to me
yup, forgot to remove after copypasta
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@60 PS6, Line 60: smbus on this southbridge is not (yet) supported.
APIC, not SMBus
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@69 PS6, Line 69: exit(1);
exit(1) or rather return 1? getting a null pointer back here is only fatal for the apic register dum […]
Well, that is done in multiple places in inteltool, but I fully aggree
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@83 PS6, Line 83: 0x10
replace this with maybe "rte_offset". Would IMHO improve readability. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c File util/inteltool/i2c.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@45 PS6, Line 45: sunrise_i2c_registers
yeah, these aren't the i2c mmio registers; haven't checked more than a few to be sure that those are […]
ouch. wtf did I do there?
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@60 PS6, Line 60: SPI_CS_CONTROL
SPI ???
...
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@138 PS6, Line 138: case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have I2C.\n"); : return 1;
not needed, remove
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@175 PS6, Line 175: }
default case for printing a warning instead of silently not doing something
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@199 PS6, Line 199: *(
since i2cbase is a u8 pointer, you'll only get 8 bits here. same below. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@221 PS6, Line 221: }
add a default case and print a warning?
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/io.c File util/inteltool/io.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/io.c@26 PS6, Line 26: {0x40, 1, "C0_ITSBFR"}, : {0x40, 1, "C0_CAPR"}, : {0x42, 1, "C2_ITSBFR"}, : {0x42, 1, "C2_CAPR"},
in combination with how those get read, this is likely wrong. […]
indeed, removed as this needs a specific implementation
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/io.c@32 PS6, Line 32: {0x21, 1, "MOCW1"}, : {0xA1, 1, "SOCW1"},
those registers also seem to have different meaning depending on some state; haven't looked closely […]
same
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/lpc.c File util/inteltool/lpc.c:
PS6:
as far as I've seen LPC and eSPI are the same PCI device D31:F0. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/lpc.c@71 PS6, Line 71: : case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have LPC.\n"); : return 1;
unneeded, remove
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/lpc.c@108 PS6, Line 108: }
default case with error message?
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/smbus.c File util/inteltool/smbus.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/smbus.c@113 PS6, Line 113: case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have smbus.\n"); : return 1;
remove
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/thermal.c File util/inteltool/thermal.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/thermal.c@10... PS6, Line 102: case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have Thermal.\n"); : return 1;
remove
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c File util/inteltool/usb.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@35 PS6, Line 35: 4
8?
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@36 PS6, Line 36: 4
2
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@120 PS6, Line 120: 0x80EF
0x80EC
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@180 PS6, Line 180: : case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have USB.\n"); : return 1;
remove
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@226 PS6, Line 226: u8
uint8_t
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@242 PS6, Line 242: *(usbbase+usb_registers[i].addr)
You need a pointer to uint32_t val to correctly read this value […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@244 PS6, Line 244: *(usbbase+usb_registers[i].addr+4));
*((uint32_t*) (usbbase + usb_registers[i]. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@249 PS6, Line 249: *(usbbase+usb_registers[i].addr)
Sorry for copy-paste […]
Done
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@255 PS6, Line 255: *(usbbase+usb_registers[i].addr)
i'd say that it would be a good idea to look into importing read8/read16/read32 functionality from c […]
Done