Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: implement lpc config dump ......................................................................
util/inteltool: implement lpc config dump
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 16 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/1
diff --git a/util/inteltool/Makefile b/util/inteltool/Makefile index cd02fa8..680b81e 100644 --- a/util/inteltool/Makefile +++ b/util/inteltool/Makefile @@ -28,7 +28,7 @@ CPPFLAGS += -I$(top)/src/commonlib/include
OBJS = inteltool.o pcr.o cpu.o gpio.o gpio_groups.o rootcmplx.o powermgt.o \ - memory.o pcie.o amb.o ivy_memory.o spi.o gfx.o ahci.o \ + memory.o pcie.o amb.o ivy_memory.o spi.o gfx.o ahci.o lpc.o \
OS_ARCH = $(shell uname) ifeq ($(OS_ARCH), Darwin) diff --git a/util/inteltool/inteltool.c b/util/inteltool/inteltool.c index 76b1abe..1dfa443 100644 --- a/util/inteltool/inteltool.c +++ b/util/inteltool/inteltool.c @@ -459,7 +459,7 @@
static void print_usage(const char *name) { - printf("usage: %s [-vh?gGrpmedPMaAsfSRx]\n", name); + printf("usage: %s [-vh?gGrplmedPMaAsfSRx]\n", name); printf("\n" " -v | --version: print the version\n" " -h | --help: print this help\n\n" @@ -470,6 +470,7 @@ " -G | --gpio-diffs: show GPIO differences from defaults\n" " -r | --rcba: dump southbridge RCBA registers\n" " -p | --pmbase: dump southbridge Power Management registers\n\n" + " -l | --lpc: dump southbridge LPC Interface registers\n\n" " -m | --mchbar: dump northbridge Memory Controller registers\n" " -S FILE | --spd=FILE: create a file storing current timings (implies -m)\n" " -e | --epbar: dump northbridge EPBAR registers\n" @@ -499,6 +500,7 @@ int dump_pmbase = 0, dump_epbar = 0, dump_dmibar = 0; int dump_pciexbar = 0, dump_coremsrs = 0, dump_ambs = 0; int dump_spi = 0, dump_gfx = 0, dump_ahci = 0, dump_sgx = 0; + int dump_lpc = 0; int show_gpio_diffs = 0; size_t pcr_count = 0; uint8_t dump_pcr[MAX_PCR_PORTS]; @@ -511,6 +513,7 @@ {"mchbar", 0, 0, 'm'}, {"rcba", 0, 0, 'r'}, {"pmbase", 0, 0, 'p'}, + {"lpc", 0, 0, 'l'}, {"epbar", 0, 0, 'e'}, {"dmibar", 0, 0, 'd'}, {"pciexpress", 0, 0, 'P'}, @@ -526,7 +529,7 @@ {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "vh?gGrpmedPMaAsfRS:x", + while ((opt = getopt_long(argc, argv, "vh?gGrplmedPMaAsfRS:x", long_options, &option_index)) != EOF) { switch (opt) { case 'v': @@ -558,6 +561,9 @@ case 'p': dump_pmbase = 1; break; + case 'l': + dump_lpc = 1; + break; case 'e': dump_epbar = 1; break; @@ -576,6 +582,7 @@ dump_mchbar = 1; dump_rcba = 1; dump_pmbase = 1; + dump_lpc = 1; dump_epbar = 1; dump_dmibar = 1; dump_pciexbar = 1; @@ -775,6 +782,11 @@ printf("\n\n"); }
+ if (dump_lpc) { + print_lpc(sb, pacc); + printf("\n\n"); + } + if (dump_mchbar) { print_mchbar(nb, pacc, dump_spd_file); printf("\n\n"); diff --git a/util/inteltool/inteltool.h b/util/inteltool/inteltool.h index 483c930..a675177 100644 --- a/util/inteltool/inteltool.h +++ b/util/inteltool/inteltool.h @@ -372,6 +372,7 @@ int print_intel_core_msrs(void); int print_mchbar(struct pci_dev *nb, struct pci_access *pacc, const char *dump_spd_file); int print_pmbase(struct pci_dev *sb, struct pci_access *pacc); +int print_lpc(struct pci_dev *sb, struct pci_access *pacc); int print_rcba(struct pci_dev *sb); int print_gpios(struct pci_dev *sb, int show_all, int show_diffs); void print_gpio_groups(struct pci_dev *sb);
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: implement lpc config dump ......................................................................
Patch Set 1:
Please add lpc.c
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#2).
Change subject: util/inteltool: implement lpc config dump ......................................................................
util/inteltool: implement lpc config dump
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/lpc.c 4 files changed, 122 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: implement lpc config dump ......................................................................
Patch Set 2:
Patch Set 1:
Please add lpc.c
thanks, I missed that. done.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: implement lpc config dump ......................................................................
Patch Set 2:
Isn't this just dumping the PCI config space? Or am I missing something here?
Putting register names on known PCI IDs doesn't sound like a bad idea though
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: implement lpc config dump ......................................................................
Patch Set 2:
Patch Set 2:
Isn't this just dumping the PCI config space? Or am I missing something here?
yes, it's like ahci.c for PCI AHCI config registers
Putting register names on known PCI IDs doesn't sound like a bad idea though
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#3).
Change subject: util/inteltool: inteltool extensions ......................................................................
util/inteltool: inteltool extensions
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c A util/inteltool/thermal.c A util/inteltool/usb.c 12 files changed, 1,513 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: inteltool extensions ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c File util/inteltool/ahci.c:
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@77 PS3, Line 77: : static const io_register_t sunrise_ahci_sir_registers[] = { : {0x80, 4, "SQUELCH"}, : {0x90, 4, "SATA_MPHY_PG"}, : {0xa4, 4, "OOBRETR"}, : }; If these are part of ABAR, I do not see a reason to separate them.
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@121 PS3, Line 121: if space after the "if" please
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@126 PS3, Line 126: " 0x%08x\n", I'd say this fits on the previous line?
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@155 PS3, Line 155: SIR Registers I like how this literally means "SATA Initialization Register Registers"
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: inteltool extensions ......................................................................
Patch Set 3: Code-Review+1
Still, nice job there
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: inteltool extensions ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c File util/inteltool/ahci.c:
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@77 PS3, Line 77: : static const io_register_t sunrise_ahci_sir_registers[] = { : {0x80, 4, "SQUELCH"}, : {0x90, 4, "SATA_MPHY_PG"}, : {0xa4, 4, "OOBRETR"}, : };
If these are part of ABAR, I do not see a reason to separate them.
They aren't. The SIRs need to be accessed by index (via SIRI and SIRD)
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@126 PS3, Line 126: " 0x%08x\n",
I'd say this fits on the previous line?
hmm, I don't like to put text after a "\n". It's done this way anywhere in inteltool
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@155 PS3, Line 155: SIR Registers
I like how this literally means "SATA Initialization Register Registers"
\o/ indeed this needs a fix
Hello Angel Pons, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#4).
Change subject: util/inteltool: inteltool extensions ......................................................................
util/inteltool: inteltool extensions
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c A util/inteltool/thermal.c A util/inteltool/usb.c 13 files changed, 1,676 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/4
Hello Angel Pons, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#5).
Change subject: util/inteltool: inteltool extensions ......................................................................
util/inteltool: inteltool extensions
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c A util/inteltool/thermal.c A util/inteltool/usb.c 13 files changed, 1,676 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/5
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: inteltool extensions ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c File util/inteltool/ahci.c:
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@77 PS3, Line 77: : static const io_register_t sunrise_ahci_sir_registers[] = { : {0x80, 4, "SQUELCH"}, : {0x90, 4, "SATA_MPHY_PG"}, : {0xa4, 4, "OOBRETR"}, : };
They aren't. […]
Done
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@121 PS3, Line 121: if
space after the "if" please
Done
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@126 PS3, Line 126: " 0x%08x\n",
hmm, I don't like to put text after a "\n". […]
Done
https://review.coreboot.org/c/coreboot/+/35189/3/util/inteltool/ahci.c@155 PS3, Line 155: SIR Registers
\o/ indeed this needs a fix
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: inteltool extensions ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG@7 PS5, Line 7: util/inteltool: inteltool extensions Please make it a statement by adding a verb (in imperative mood):
Extend inteltool
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG@8 PS5, Line 8: So it’s mostly Intel Sunrise stuff? Could you elaborate a little?
Hello Angel Pons, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#6).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kabylake) that have not been added to inteltool, yet.
Completely new: - SMBus Interface - APIC - I2C Interface(s) - LPC Interface - Thermal Subsystem - USB (XHCI)
Additionally, inteltool is now able to show the PCR registers for most of the supported interfaces/subsystems when a register table is added for the appropriate chipset. For Sunrise Point a table is included now.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c A util/inteltool/thermal.c A util/inteltool/usb.c 13 files changed, 1,676 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/6
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 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG@7 PS5, Line 7: util/inteltool: inteltool extensions
Please make it a statement by adding a verb (in imperative mood): […]
Done
https://review.coreboot.org/c/coreboot/+/35189/5//COMMIT_MSG@8 PS5, Line 8:
So it’s mostly Intel Sunrise stuff? Could you elaborate a little?
Done
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:
(3 comments)
only looked at the ahci change for now; will look at the rest later
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)
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
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/ahci.c@164 PS6, Line 164: 0xa0 add a define for this register?
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 ......................................................................
Patch Set 6:
Patch Set 6:
(3 comments)
only looked at the ahci change for now; will look at the rest later
ok, I'll wait and fix everything together then
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
Maxim Polyakov 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: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG@9 PS6, Line 9: Kabylake Kaby Lake
https://review.coreboot.org/c/coreboot/+/35189/6//COMMIT_MSG@23 PS6, Line 23: please add the Intel document number that you used
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?
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@36 PS6, Line 36: 4 2
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@120 PS6, Line 120: 0x80EF 0x80EC
Maxim Polyakov 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:
(5 comments)
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@226 PS6, Line 226: u8 uint8_t
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 *((uint32_t*) (usbbase + usb_registers[i].addr))
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].addr + 4))
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@249 PS6, Line 249: *(usbbase+usb_registers[i].addr) *((uint32_t*) (usbbase + usb_registers[i].addr + 4))
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@255 PS6, Line 255: *(usbbase+usb_registers[i].addr) *((uint16_t*) (usbbase + usb_registers[i].addr + 4))
Maxim Polyakov 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:
(2 comments)
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@249 PS6, Line 249: *(usbbase+usb_registers[i].addr)
*((uint32_t*) (usbbase + usb_registers[i]. […]
Sorry for copy-paste *((uint32_t*) (usbbase + usb_registers[i].addr))
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/usb.c@255 PS6, Line 255: *(usbbase+usb_registers[i].addr)
*((uint16_t*) (usbbase + usb_registers[i]. […]
Sorry for copy-paste *((uint16_t*) (usbbase + usb_registers[i].addr))
Maxim Polyakov 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:
(4 comments)
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?
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@199 PS6, Line 199: *( See usb.c
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@16... PS6, Line 163: *( See usb.c
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
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:
(2 comments)
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 […]
just using the read8/16/32 functions would also make this nicer
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.h here instead to avoid having so much pointer type casts?
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#7).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,221 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/7
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
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 ......................................................................
Patch Set 7:
(1 comment)
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@16... PS6, Line 163: *(
See usb. […]
Done
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 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c File util/inteltool/apic.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@24 PS6, Line 24: #define IO_APIC_ADDR 0xfec00000
I got this from src/arch/x86/include/arch/ioapic. […]
According to doc#332691-003EN (at 23.2.2) and Intel MP Spec. v1.4 (at 3.1) this is a fixed address.
Nico Huber 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 7:
(4 comments)
It seems we don't have format-string warnings enabled?
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c File util/inteltool/apic.c:
https://review.coreboot.org/c/coreboot/+/35189/6/util/inteltool/apic.c@24 PS6, Line 24: #define IO_APIC_ADDR 0xfec00000
According to doc#332691-003EN (at 23.2.2) and Intel MP Spec. v1.4 (at 3.1) this is a fixed address.
Let's say: If there is a single I/O APIC, you can expect it there. And so far, FWIW, if there are multiple, the first uses this address, too.
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c File util/inteltool/apic.c:
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@110 PS7, Line 110: lx This won't work on x86_32.
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@110 PS7, Line 110: ld `i` is not a long, it's size_t?
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@111 PS7, Line 111: (uint16_t) Why the cast?
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#8).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,221 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/8
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 8.
(3 comments)
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c File util/inteltool/apic.c:
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@110 PS7, Line 110: lx
This won't work on x86_32.
Done
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@110 PS7, Line 110: ld
`i` is not a long, it's size_t?
Done
https://review.coreboot.org/c/coreboot/+/35189/7/util/inteltool/apic.c@111 PS7, Line 111: (uint16_t)
Why the cast?
Done
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#9).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,221 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/9
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 9.
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 10.
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#10).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,221 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/10
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#11).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,221 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/11
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 11.
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#12).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,231 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/12
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 12.
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#13).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,371 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/13
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 13.
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#14).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,371 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/14
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 14.
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#15).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,528 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/15
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 15.
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 15: Code-Review-1
(6 comments)
didn't check the named and offsets; found some other bugs though
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/gspi.c File util/inteltool/gspi.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/gspi.c@205 PS15, Line 205: read16 read32
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/i2c.c File util/inteltool/i2c.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/i2c.c@237 PS15, Line 237: read16 read32
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@16... PS6, Line 163: *(
Done
not done yet
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c File util/inteltool/thermal.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 176: *(thermalbase+thermal_registers[i].addr), use read32
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 182: *(thermalbase+thermal_registers[i].addr), use read16
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 188: *(thermalbase+thermal_registers[i].addr), use read8
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#16).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,528 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/16
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 16.
(6 comments)
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/gspi.c File util/inteltool/gspi.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/gspi.c@205 PS15, Line 205: read16
read32
Done
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/i2c.c File util/inteltool/i2c.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/i2c.c@237 PS15, Line 237: read16
read32
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@16... PS6, Line 163: *(
not done yet
oops... I'm sorry. now it's done
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c File util/inteltool/thermal.c:
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 176: *(thermalbase+thermal_registers[i].addr),
use read32
Done
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 182: *(thermalbase+thermal_registers[i].addr),
use read16
Done
https://review.coreboot.org/c/coreboot/+/35189/15/util/inteltool/thermal.c@1... PS15, Line 188: *(thermalbase+thermal_registers[i].addr),
use read8
Done
Hello Felix Held, Angel Pons, Maxim Polyakov, Stefan Reinauer, Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35189
to look at the new patch set (#17).
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
util/inteltool: add many missing registers for skl/kbl
This adds dumping of many registers for Sunrise Point (Skylake/Kaby Lake) that have not been added to inteltool, yet, from doc#332691-003EN and doc#334659-005.
Completely new: - SMBus - APIC - I2C - LPC/eSPI - Thermal Subsystem - USB (XHCI) - GSPI - SPI - PMC
The implementation for Sunrise Point-LP SPI interface could never work because the whole addressing scheme changed. The correct implementation has been added, which works for both Sunrise Point PCH-H and PCH-LP.
The same applies to PMC.
Additionally, inteltool is now able to show the PCI Configuration registers for most of the supported interfaces/subsystems when an appropriate register table is available for the chipset. For Sunrise Point those tables have been added.
Signed-off-by: Michael Niewöhner foss@mniewoehner.de Change-Id: I3bba1252635b3421051dd53f7d5adbd3d73ed1b6 --- M util/inteltool/Makefile M util/inteltool/ahci.c A util/inteltool/apic.c M util/inteltool/gpio_groups.c A util/inteltool/gspi.c A util/inteltool/i2c.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h A util/inteltool/io.c A util/inteltool/lpc.c M util/inteltool/powermgt.c A util/inteltool/smbus.c M util/inteltool/spi.c A util/inteltool/thermal.c A util/inteltool/usb.c 15 files changed, 2,529 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35189/17
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 17.
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 ......................................................................
Patch Set 17:
@Felix Held ping ;)
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 17: Code-Review+1
(2 comments)
looks good to me, but i currently don't have the time to check the register names and offsets, so I'm a bit hesitant to hit +2. if someone else wants to do that, please go ahead
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/lpc.c File util/inteltool/lpc.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/lpc.c@111 PS17, Line 111: 0xDC use a define for this? also coreboot uses mostly lower case for hex values
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c File util/inteltool/powermgt.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c@... PS17, Line 880: case 0x1234: // Dummy for non-existent functionality : printf("This southbridge does not have PMBASE.\n"); : return 1; this can be removed
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 ......................................................................
Patch Set 17:
Patch Set 17: Code-Review+1
(2 comments)
looks good to me, but i currently don't have the time to check the register names and offsets, so I'm a bit hesitant to hit +2. if someone else wants to do that, please go ahead
Since I copied the tables from the datasheet and just did some regex replace, most names *should* be correct. I just hope I didn't copy another wrong table like I did for I2C :D
HAOUAS Elyes 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 17: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c File util/inteltool/powermgt.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c@... PS17, Line 901: acpi here, acpi = NULL it gives me a "Segmentation fault" on "Southbridge: 8086:27b8 (ICH7)"
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 17: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c File util/inteltool/powermgt.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c@... PS17, Line 888: for (i = 0; i < pm_cfg_registers_size; i++) { : switch (pm_cfg_registers[i].size) { : case 8: : printf("0x%04x: 0x%08x (%s)\n" : " 0x%08x\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name, : pci_read_long(acpi, pm_cfg_registers[i].addr+4)); : break; : case 4: : printf("0x%04x: 0x%08x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 2: : printf("0x%04x: 0x%04x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_word(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 1: : printf("0x%04x: 0x%02x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_byte(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : } : } make this conditional, so that it only gets run for acpi != NULL
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 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c File util/inteltool/powermgt.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c@... PS17, Line 888: for (i = 0; i < pm_cfg_registers_size; i++) { : switch (pm_cfg_registers[i].size) { : case 8: : printf("0x%04x: 0x%08x (%s)\n" : " 0x%08x\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name, : pci_read_long(acpi, pm_cfg_registers[i].addr+4)); : break; : case 4: : printf("0x%04x: 0x%08x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 2: : printf("0x%04x: 0x%04x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_word(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 1: : printf("0x%04x: 0x%02x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_byte(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : } : }
make this conditional, so that it only gets run for acpi != NULL
from the irc: initializing pm_cfg_registers_size to 0 would also work around the issue. best solution would be doing both imho
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 ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c File util/inteltool/powermgt.c:
https://review.coreboot.org/c/coreboot/+/35189/17/util/inteltool/powermgt.c@... PS17, Line 888: for (i = 0; i < pm_cfg_registers_size; i++) { : switch (pm_cfg_registers[i].size) { : case 8: : printf("0x%04x: 0x%08x (%s)\n" : " 0x%08x\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name, : pci_read_long(acpi, pm_cfg_registers[i].addr+4)); : break; : case 4: : printf("0x%04x: 0x%08x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_long(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 2: : printf("0x%04x: 0x%04x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_word(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : case 1: : printf("0x%04x: 0x%02x (%s)\n", : pm_cfg_registers[i].addr, : pci_read_byte(acpi, pm_cfg_registers[i].addr), : pm_cfg_registers[i].name); : break; : } : }
from the irc: initializing pm_cfg_registers_size to 0 would also work around the issue. […]
oops. yep, will add that
Michael Niewöhner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35189 )
Change subject: util/inteltool: add many missing registers for skl/kbl ......................................................................
Abandoned
splitted into small parts
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
HAOUAS Elyes (Code Review)
-
Idwer Vollering (Code Review)
-
Maxim Polyakov (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)