Change in ...coreboot[master]: board/kukui: Print ADC values on failure in getting board IDs
Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32353
Change subject: board/kukui: Print ADC values on failure in getting board IDs ......................................................................
board/kukui: Print ADC values on failure in getting board IDs
WIP: Need a better way to report ADC failures.
BUG=None TEST=None BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/1
diff --git a/src/mainboard/google/kukui/boardid.c b/src/mainboard/google/kukui/boardid.c index b9f1ba2..953ae33 100644 --- a/src/mainboard/google/kukui/boardid.c +++ b/src/mainboard/google/kukui/boardid.c @@ -15,6 +15,7 @@
#include <assert.h> #include <boardid.h> +#include <console/console.h> #include <soc/auxadc.h> #include <ec/google/chromeec/ec.h> #include <stddef.h> @@ -83,7 +84,11 @@ break;
const int tolerance = 10000; /* 10,000 uV */ - assert(ABS(value - voltages[id]) < tolerance); + if (ABS(value - voltages[id]) < tolerance) { + printk(BIOS_ERR, "ADC channel %u value out of range: %d\n" + channel, value); + die("Invalid ADC value"); + }
return id; }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32353
to look at the new patch set (#2).
Change subject: board/kukui: Print ADC values on failure in getting board IDs ......................................................................
board/kukui: Print ADC values on failure in getting board IDs
WIP: Need a better way to report ADC failures.
BUG=None TEST=None BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32353
to look at the new patch set (#3).
Change subject: board/kukui: Support ADC value for NC ......................................................................
board/kukui: Support ADC value for NC
When the components like LCM ID are not installed (i.e., NC), ADC will return some value with much larger variation from standard value (out of the tolerance we set). To support that, we should check tolerance only on non-NC voltages.
Also improved the error messages so we can see the ADC raw values instead of simple assertion error (which makes debugging more difficult since we have to build another firmware image just to print the values).
BUG=None TEST=None BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/3
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32353
to look at the new patch set (#4).
Change subject: board/kukui: Support ADC value for NC ......................................................................
board/kukui: Support ADC value for NC
When the components like LCM ID are not installed (i.e., NC), ADC will return some value with much larger variation from standard value (out of the tolerance we set). To support that, we should check tolerance only on non-NC voltages.
Also improved the error messages so we can see the ADC raw values instead of simple assertion error (which makes debugging more difficult since we have to build another firmware image just to print the values).
BUG=None TEST=Boots on Kukui and saw correct SKU ID for NC LCMID. BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/4
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32353
to look at the new patch set (#5).
Change subject: board/kukui: Support ADC value for NC ......................................................................
board/kukui: Support ADC value for NC
When the components like LCM ID are not installed (i.e., NC), ADC will return some value with much larger variation from standard value (out of the tolerance we set). To support that, we should check tolerance only on non-NC voltages.
Also improved the error messages so we can see the ADC raw values instead of simple assertion error (which makes debugging more difficult since we have to build another firmware image just to print the values).
BUG=None TEST=Booted on Kukui and got correct SKU ID for NC LCMID. BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/5
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32353/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32353/4//COMMIT_MSG@14 PS4, Line 14: improved improve
https://review.coreboot.org/#/c/32353/4/src/mainboard/google/kukui/boardid.c File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/#/c/32353/4/src/mainboard/google/kukui/boardid.c... PS4, Line 91: assert(0); Why is the assert still needed?
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
Patch Set 5: Code-Review+1
Hello Julius Werner, You-Cheng Syu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32353
to look at the new patch set (#6).
Change subject: board/kukui: Support ADC value for NC ......................................................................
board/kukui: Support ADC value for NC
When the components like LCM ID are not installed (i.e., NC), ADC will return some value with much larger variation from standard value (out of the tolerance we set). To support that, we should check tolerance only on non-NC voltages.
Also improve the error messages so we can see the ADC raw values instead of simple assertion error (which makes debugging more difficult since we have to build another firmware image just to print the values).
BUG=None TEST=Booted on Kukui and got correct SKU ID for NC LCMID. BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/32353/6
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32353/4/src/mainboard/google/kukui/boardid.c File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/#/c/32353/4/src/mainboard/google/kukui/boardid.c... PS4, Line 91: assert(0);
Why is the assert still needed?
I think we still want the device to hang and fail for early/debug builds (to catch real build failure), but probably not failing in release products. So an assert is added.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32353/6/src/mainboard/google/kukui/boardid.c File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/#/c/32353/6/src/mainboard/google/kukui/boardid.c... PS6, Line 86: /* The last level is NC and may be larger than standard tolerance. */ I don't really understand how we get here when the pin is not connected. Wouldn't that make it float (meaning you could have any possible value)? Or do you have it strapped high somewhere?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32353/6/src/mainboard/google/kukui/boardid.c File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/#/c/32353/6/src/mainboard/google/kukui/boardid.c... PS6, Line 86: /* The last level is NC and may be larger than standard tolerance. */
I don't really understand how we get here when the pin is not connected. […]
MTK is still doing final confirm, but from what we understand for now is:
- The ADC may read from 0~1.5V+-50mV, so the max value from spec was set to 1.45V. - However, if not calibrated, the ADC may still see >1.45V. - So in our case, the NC caused ADC seeing higher voltage and returns something over standard tolerance.
For now both teams agreed to not restricting (or make it larger) tolerance for NC values.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32353 )
Change subject: board/kukui: Support ADC value for NC ......................................................................
board/kukui: Support ADC value for NC
When the components like LCM ID are not installed (i.e., NC), ADC will return some value with much larger variation from standard value (out of the tolerance we set). To support that, we should check tolerance only on non-NC voltages.
Also improve the error messages so we can see the ADC raw values instead of simple assertion error (which makes debugging more difficult since we have to build another firmware image just to print the values).
BUG=None TEST=Booted on Kukui and got correct SKU ID for NC LCMID. BRANCH=None
Change-Id: I8d00956e0e3b48ddbcaa505dd3ade24720c3b4ad Signed-off-by: Hung-Te Lin hungte@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32353 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: You-Cheng Syu youcheng@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/kukui/boardid.c 1 file changed, 7 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved You-Cheng Syu: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/kukui/boardid.c b/src/mainboard/google/kukui/boardid.c index b9f1ba2..71f051f 100644 --- a/src/mainboard/google/kukui/boardid.c +++ b/src/mainboard/google/kukui/boardid.c @@ -15,6 +15,7 @@
#include <assert.h> #include <boardid.h> +#include <console/console.h> #include <soc/auxadc.h> #include <ec/google/chromeec/ec.h> #include <stddef.h> @@ -82,8 +83,13 @@ if (value < (voltages[id] + voltages[id + 1]) / 2) break;
+ /* The last level is NC and may be larger than standard tolerance. */ const int tolerance = 10000; /* 10,000 uV */ - assert(ABS(value - voltages[id]) < tolerance); + if (id < ADC_LEVELS - 1 && ABS(value - voltages[id]) > tolerance) { + printk(BIOS_ERR, "ADC channel %u value out of range: %d\n", + channel, value); + assert(0); + }
return id; }
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
You-Cheng Syu (Code Review)