Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
Patch Set 6:
(14 comments)
haven't checked all struct arrays, but i think i've looked at most of the rest now. big improvement would probably be using the mmio access functions from coreboot
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
Maybe these registers for GSPI device?
yeah, these aren't the i2c mmio registers; haven't checked more than a few to be sure that those aren't from the i2c device
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
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
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@199 PS6, Line 199: *(
See usb. […]
since i2cbase is a u8 pointer, you'll only get 8 bits here. same below.
also since all registers are 32 bits wide, I'd only implement that and have a default case to catch problems in future extensions. but yeah, i don't like implementing unneeded functionality too much, since those code paths won't get tested if they're not used at all. as long as it's apparently bug-free, I don't have much objection to keep that though
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/i2c.c@221 PS6, Line 221: } add a default case and print a warning?
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. io register 0x43 influences what you get back when reading 0x40 or 0x42
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 into that though
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. is it needed to detect which one we're dealing with? detecting and printing this might also be useful?! or just print all registers that might not be used for the other type?
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
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/lpc.c@108 PS6, Line 108: } default case with error message?
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
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
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@180 PS6, Line 180: : case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have USB.\n"); : return 1; remove
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@255 PS6, Line 255: *(usbbase+usb_registers[i].addr)
Sorry for copy-paste […]
i'd say that it would be a good idea to look into importing read8/read16/read32 functionality from coreboot's arch/mmio.h into inteltool to avoid some of the problems of typecasting pointers