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:
(7 comments)
ok, I'll wait and fix everything together then
sounds like a plan. when I've reviewed a file and won't get around to review the next one right after that, i want to publish the comments, so i don't loose them accidentally. i've looked at the apic code now
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c File util/inteltool/apic.c:
PS6: i think the number of type casts on apicbase can be reduced when declaring it as volatile const u32 *const apicbase, cast the pointer returned by map_physical once and using volatile u32 *ioapic_base as parameter of io_apic_read
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?
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 CPUs
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
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
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 dumping, but not for the other things inteltool does
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. also below