Change in coreboot[master]: util/superiotool: default to using the first discovered chip and exit...
Matt Parnell has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34890 )
Change subject: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. ......................................................................
util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives.
Signed-off-by: Matt Parnell mparnell@gmail.com Change-Id: I220b87a449a9efbaa4a77ad5ae837ffcdced9605 --- M util/superiotool/superiotool.c M util/superiotool/superiotool.h 2 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/34890/1
diff --git a/util/superiotool/superiotool.c b/util/superiotool/superiotool.c index 3e21b0c..be56b68 100644 --- a/util/superiotool/superiotool.c +++ b/util/superiotool/superiotool.c @@ -294,6 +294,7 @@ static const struct option long_options[] = { {"dump", no_argument, NULL, 'd'}, {"extra-dump", no_argument, NULL, 'e'}, + {"dump-all", no_argument, NULL, 'A'}, {"alternate-dump", no_argument, NULL, 'a'}, {"list-supported", no_argument, NULL, 'l'}, {"verbose", no_argument, NULL, 'V'}, @@ -302,7 +303,7 @@ {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "dealVvh", + while ((opt = getopt_long(argc, argv, "deAalVvh", long_options, &option_index)) != EOF) { switch (opt) { case 'd': @@ -330,6 +331,9 @@ printf(USAGE_INFO); exit(0); break; + case 'A': + dump_all = 1; + break; default: /* Unknown option. */ exit(1); @@ -358,9 +362,15 @@ #endif
for (i = 0; i < ARRAY_SIZE(superio_ports_table); i++) { - for (j = 0; superio_ports_table[i].ports[j] != EOT; j++) + for (j = 0; superio_ports_table[i].ports[j] != EOT; j++) { superio_ports_table[i].probe_idregs( superio_ports_table[i].ports[j]); + + /* please, please, fix your false positives + * and make the below unnecessary */ + if (!dump_all && chip_found) + return(0); + } }
if (!chip_found) diff --git a/util/superiotool/superiotool.h b/util/superiotool/superiotool.h index d3b9fd0..db1c08b 100644 --- a/util/superiotool/superiotool.h +++ b/util/superiotool/superiotool.h @@ -149,6 +149,7 @@ /* Command line parameters. */ extern int dump, verbose, extra_dump;
+int dump_all; extern int chip_found;
struct superio_registers { @@ -247,7 +248,6 @@ int ports[MAXNUMPORTS]; /* Signed, as we need EOT. */ } superio_ports_table[] = { {probe_idregs_ali, {0x3f0, 0x370, EOT}}, - {probe_idregs_aspeed, {0x2e, 0x4e, EOT}}, {probe_idregs_exar, {0x2e, 0x4e, EOT}}, {probe_idregs_fintek, {0x2e, 0x4e, EOT}}, {probe_idregs_fintek_alternative, {0x2e, 0x4e, EOT}}, @@ -263,6 +263,7 @@ {probe_idregs_via, {0x2e, 0x4e, 0x3f0, EOT}}, /* in fact read the BASE from HW */ {probe_idregs_amd, {0xaa, EOT}}, + {probe_idregs_aspeed, {0x2e, 0x4e, EOT}}, /* this one gives lots of false positives */ #endif {probe_idregs_serverengines, {0x2e, EOT}}, {probe_idregs_infineon, {0x2e, 0x4e, EOT}},
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34890 )
Change subject: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. ......................................................................
Patch Set 1: Code-Review-1
there are systems with more than one super IO chip. I'd strongly prefer fixing the underlying issue than implementing some workaround to hide it, that causes other issues
Matt Parnell has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34890 )
Change subject: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
there are systems with more than one super IO chip. I'd strongly prefer fixing the underlying issue than implementing some workaround to hide it, that causes other issues
In that case, currently for the IT8987 I'm getting false positives for these: Found SMSC SCH5027 (id=0x89, rev=0x87) at 0x2e Found Aspeed AST2400 (id=0x00) at 0x2e
And previously a Winbond...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34890 )
Change subject: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. ......................................................................
Patch Set 1:
(1 comment)
I agree with Felix: while multiple SIOs on a board is rare, it can happen. And breaking superiotool on those cases doesn't sound alright.
Maybe make it like flashrom, that has a commandline switch to specify the flash chip definition to use? Though for superiotool, maybe a simpler "vendor mask" would do: if you know your board only has ITE and Nuvoton chips, just probe for those vendors.
https://review.coreboot.org/c/coreboot/+/34890/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34890/1//COMMIT_MSG@7 PS1, Line 7: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. I think this line miiight be bit too long for a commit summary
Matt Parnell has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34890 )
Change subject: util/superiotool: default to using the first discovered chip and exit (let's fix false positives so this isn't necessary); put aspeed in the appropriate execution order to prevent false positives. ......................................................................
Abandoned
abandoned in favor of improving false positives, which i can't do... and also perhaps add a manufacturer filter mask later on
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Matt Parnell (Code Review)