Maxim has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83196?usp=email )
Change subject: [RFC][WIP] util/superiotool: Add extra selectors control ......................................................................
[RFC][WIP] util/superiotool: Add extra selectors control
Some chips, have specific selectors (in addition to LDN-register) that affect the register space. At the same time, the utility doesn't provide a simple and convenient method for configuring such selectors (as in the case of LDN-selector) to create a dump, because this may be configured by several fields of the register, and the values of its other fields should not change after setting in the BIOS (fintek [1,2]).
To switch register banks using such an extra selector, it is necessary to define a table of its states (which includes the identifier, register index, register mask and selector value) and define a special EXT_SELECTOR macro in the superio_registers table instead of idx[i], and the corresponding def[i] should contain the id of the selector state table with the required value [1, 2]:
static extra_selector_state_t esel_table[] = { {.id = 1, .idx = 0x27, .mask = 0xf2, .val = 0x01}, {.id = 2, .idx = 0x27, .mask = 0xf2, .val = 0x00}, }
{... EXT_SELECTOR, 0x28,0x2c, EXT_SELECTOR, 0x2c, ...} {... 1, NANA,0x0f, 2, 0x00, ...}
The current solution with the esel state table seems to be the safest, since it limits the list of possible selector values.
Tested with Fintek F81966 on Asrock IMB-1222 [3]:
superiotool r24.05-302-g7d52d9d0f8 Found Fintek F81962/F81964/F81966/F81967 (vid=0x3419, id=0x0215) at 0x2e LDN 0xfe (Global) idx [27] 02 ... [27] 28 ... 2c [27] 28 ... 2c [27] 28 ... 2c [27] ... val [00] 00 ... [04] 00 ... 00 [08] 03 ... 01 [0c] 59 ... 18 [01] ... def [00] NA ... [04] 00 ... 00 [08] 03 ... 00 [0c] 5b ... 18 [01] ... ...
or with -a option:
superiotool r24.05-302-g7d52d9d0f8 Found Fintek F81962/F81964/F81966/F81967 (vid=0x3419, id=0x0215) at 0x2e LDN 0xfe (Global) idx def val ============================= [0x27] 0x00 0x00 ESEL[00] ============================= 0x02: (NA) 0x00 ... 0x2d: 0x28 [0x2e] ============================= [0x27] 0x04 0x04 ESEL[01] ============================= 0x28: 0x00 0x00 ... 0x2c: 0x00 0x00 ============================= [0x27] 0x08 0x08 ESEL[02] ============================= 0x28: 0x03 0x03 ... 0x2c: 0x00 [0x01] ============================= [0x27] 0x0c 0x0c ESEL[03] ============================= 0x28: 0x5b [0x59] ... 0x2c: 0x18 0x18 ============================= [0x27] 0x01 0x01 ESEL[04] ============================= 0x29: 0x03 0x03 ... 0x2c: 0x00 0x00 ============================= [0x27] 0x00 0x00 ESEL[00] ============================= ...
ESEL[x], where x is the id of the entry in the selector.
In print, idx contains the index of the selector register, val is the current value, and def is the value from the status table. In case of an error, if an entry in the table with the specified id does not exist, then [XX] will be printed instead of idx, def and var.
The status table looks better than the callback [4], because it standardizes the use of the selector and allows you to set all valid values at once + the code is cleaner, but I still don't really like this solution. In any case, I am limited to only one variable in def[i] from reg_table (I don't want to separate idx and def into different fields, it looks ugly and requires a lot of unnecessary macro constructs This will only create confusion)..
Another way is to embed the selector structure directly into 'superio_registers', but this is a lot of changes for all chips.
[1] CB:83004 [2] CB:83019 [3] CB:83107 [4] CB:83001
Change-Id: If56af9f977381e637245bdd26563f5ba7e6cbead Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/superiotool/superiotool.c M util/superiotool/superiotool.h 2 files changed, 107 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/83196/1
diff --git a/util/superiotool/superiotool.c b/util/superiotool/superiotool.c index aee5026..837e133 100644 --- a/util/superiotool/superiotool.c +++ b/util/superiotool/superiotool.c @@ -84,22 +84,47 @@ return "<unknown>"; }
+static uint8_t set_extra_selector(uint16_t port, const extra_selector_state_t *selector) { + + uint8_t val = regval(port, selector->idx); + val &= selector->mask; + val |= selector->val; + regwrite(port, selector->idx, val); + + return regval(port, selector->idx) & (~selector->mask); +} + +static const extra_selector_state_t* get_ext_sel_state_entry( + const extra_selector_state_t table[], uint8_t table_size, int16_t entry_num) { + if (table == NULL) { + return NULL; + } + for (int i = 0; i < table_size; i++) { + if (table[i].id == entry_num) + return &table[i]; + } + return NULL; +} + static void dump_regs(const struct superio_registers reg_table[], - int i, int j, uint16_t port, uint8_t ldn_sel) + int i, int j, uint16_t port, uint8_t ldn_sel, + const extra_selector_state_t esel_tbl[], int esel_tbl_size) { int k; const int16_t *idx, *def; + const uint8_t ldn = reg_table[i].ldn[j].ldn; + const char *ldn_name = reg_table[i].ldn[j].name;
- if (reg_table[i].ldn[j].ldn != NOLDN) { - printf("LDN 0x%02x", reg_table[i].ldn[j].ldn); - if (reg_table[i].ldn[j].name != NULL) - printf(" (%s)", reg_table[i].ldn[j].name); - regwrite(port, ldn_sel, reg_table[i].ldn[j].ldn); + if (ldn != NOLDN) { + printf("LDN 0x%02x", ldn); + if (ldn_name != NULL) + printf(" (%s)", ldn_name); + regwrite(port, ldn_sel, ldn); } else { - if (reg_table[i].ldn[j].name == NULL) + if (ldn_name == NULL) printf("Register dump:"); else - printf("(%s)", reg_table[i].ldn[j].name); + printf("(%s)", ldn_name); }
idx = reg_table[i].ldn[j].idx; @@ -118,6 +143,21 @@ continue; }
+ if (idx[k] == EXT_SELECTOR) { + const extra_selector_state_t *sel = get_ext_sel_state_entry(esel_tbl, + esel_tbl_size, def[k]); + + printf("=============================\n"); + if (sel == NULL) + printf("[XXXX] XXXX XXXX ESEL[%02d]\n", def[k]); + else + printf("[0x%02x] 0x%02x 0x%02x ESEL[%02d]\n", sel->idx, sel->val, + set_extra_selector(port, sel), def[k]); + printf("=============================\n"); + + continue; + } + printf("0x%02x: ", idx[k]); val = regval(port, idx[k]);
@@ -139,6 +179,19 @@ for (k = 0; idx[k] != EOT; k++) { if (k && !(k % 8)) putchar(' '); + + if (idx[k] == EXT_SELECTOR) { + const extra_selector_state_t *sel = get_ext_sel_state_entry(esel_tbl, + esel_tbl_size, def[k]); + + if (sel == NULL) + printf(" [XX]"); + else + printf(" [%02x]", sel->idx); + + continue; + } + printf(" %02x", idx[k]); }
@@ -146,6 +199,18 @@ for (k = 0; idx[k] != EOT; k++) { if (k && !(k % 8)) putchar(' '); + + if (idx[k] == EXT_SELECTOR) { + const extra_selector_state_t *sel = get_ext_sel_state_entry(esel_tbl, + esel_tbl_size, def[k]); + + if (sel == NULL) + printf(" [XX]"); + else + printf(" [%02x]", set_extra_selector(port, sel)); + + continue; + } printf(" %02x", regval(port, idx[k])); }
@@ -159,16 +224,24 @@ printf(" RR"); else if (def[k] == MISC) printf(" MM"); - else + else if (idx[k] == EXT_SELECTOR) { + const extra_selector_state_t *sel = get_ext_sel_state_entry( + esel_tbl, esel_tbl_size, def[k]); + if (sel == NULL) + printf(" [XX]"); + else + printf(" [%02x]", sel->val); + } else printf(" %02x", def[k]); } } printf("\n"); }
-void dump_superio(const char *vendor, +void dump_superio_with_extra_selector(const char *vendor, const struct superio_registers reg_table[], - uint16_t port, uint16_t id, uint8_t ldn_sel) + uint16_t port, uint16_t id, uint8_t ldn_sel, + const extra_selector_state_t esel_tbl[], int esel_tbl_size) { int i, j, no_dump_available = 1;
@@ -186,7 +259,7 @@ if (reg_table[i].ldn[j].ldn == EOT) break; no_dump_available = 0; - dump_regs(reg_table, i, j, port, ldn_sel); + dump_regs(reg_table, i, j, port, ldn_sel, esel_tbl, esel_tbl_size); }
if (no_dump_available) @@ -194,6 +267,13 @@ } }
+void dump_superio(const char *vendor, + const struct superio_registers reg_table[], + uint16_t port, uint16_t id, uint8_t ldn_sel) +{ + dump_superio_with_extra_selector(vendor, reg_table, port, id, ldn_sel, NULL, 0); +} + void dump_io(uint16_t iobase, uint16_t length) { uint16_t i; diff --git a/util/superiotool/superiotool.h b/util/superiotool/superiotool.h index 1409030..d60f0a9 100644 --- a/util/superiotool/superiotool.h +++ b/util/superiotool/superiotool.h @@ -121,6 +121,9 @@ Used for registers depending on external pin straps configuring static, but board-specific settings like SIO base address or AMD/Intel power seqencing type. */ +#define EXT_SELECTOR -6 /* Defines the operation of setting an extra + specific selector in the configuration register. + Used for fintek chips */ #define MAXLDN 0x14 /* Biggest LDN */ #define LDNSIZE (MAXLDN + 3) /* Biggest LDN + 0 + NOLDN + EOT */ #define MAXNUMIDX 170 /* Maximum number of indices */ @@ -131,6 +134,14 @@ #define LDN_SEL 0x07 /* LDN select register */ #define WINBOND_HWM_SEL 0x4e /* Hardware monitor bank select */
+/* Structure describing the extra selector and its stat (see fintek.c) */ +typedef struct { + int16_t id; + uint8_t idx; + uint8_t mask; + uint8_t val; +} extra_selector_state_t; + /* Command line parameters. */ extern int dump, verbose, extra_dump;
@@ -163,6 +174,10 @@ int superio_unknown(const struct superio_registers reg_table[], uint16_t id); const char *get_superio_name(const struct superio_registers reg_table[], uint16_t id); +void dump_superio_with_extra_selector(const char *vendor, + const struct superio_registers reg_table[], + uint16_t port, uint16_t id, uint8_t ldn_sel, + const extra_selector_state_t esel_tbl[], int esel_tbl_size); void dump_superio(const char *name, const struct superio_registers reg_table[], uint16_t port, uint16_t id, uint8_t ldn_sel); void dump_io(uint16_t iobase, uint16_t length);
-
Maxim (Code Review)