JG Poxu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32800
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/1
diff --git a/src/soc/mediatek/mt8183/auxadc.c b/src/soc/mediatek/mt8183/auxadc.c old mode 100644 new mode 100755 index a167d2b..408f911 --- a/src/soc/mediatek/mt8183/auxadc.c +++ b/src/soc/mediatek/mt8183/auxadc.c @@ -18,11 +18,43 @@ #include <delay.h> #include <soc/addressmap.h> #include <soc/auxadc.h> +#include <soc/efuse.h> #include <soc/infracfg.h> #include <timer.h>
static struct mtk_auxadc_regs *const mtk_auxadc = (void *)AUXADC_BASE; +/* For calibration EFUSE: + * 1. ADC_GE_A[9:0] *(0x11F101B4)[19:10] Default:512 + * 2. ADC_OE_A[9:0] *(0x11F101B4)[9:0] Default:512 + * 3. ADC_CALI_EN_A(1b) *(0x11F101B4)[20] + */ +#define ADC_GE_A_SHIFT 10 +#define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT) +#define ADC_OE_A_SHIFT 0 +#define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT) +#define ADC_CALI_EN_A_SHIFT 20 +#define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT) +#define AUXADC_CALI_INIT ~((uint32_t)1)
+static int cali_oe = AUXADC_CALI_INIT; +static int cali_ge = AUXADC_CALI_INIT; + +static void mt_auxadc_update_cali(void) +{ + int cali_reg; + int cali_ge_a; + int cali_oe_a; + + cali_reg = read32(&mtk_efuse->adc_cali_reg); + + if ((cali_reg & ADC_CALI_EN_A_MASK) != 0) { + cali_oe_a = (cali_reg & ADC_OE_A_MASK) >> ADC_OE_A_SHIFT; + cali_ge_a = ((cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT); + /*In sw implement guide, ge should div 4096. But we don't do that now due to it will multi 4096 later*/ + cali_ge = cali_ge_a - 512; + cali_oe = cali_oe_a - 512; + } +} static uint32_t auxadc_get_rawdata(int channel) { setbits_le32(&mt8183_infracfg->module_sw_cg_1_clr, 1 << 10); @@ -47,5 +79,8 @@ assert(channel < 16);
/* 1.5V in 4096 steps */ - return (int)((int64_t)auxadc_get_rawdata(channel) * 1500000 / 4096); + rawvalue = auxadc_get_rawdata(channel); + + rawvalue = rawvalue - cali_oe; + return (int)((int64_t)rawvalue * 1500000 / (4096 + cali_ge)); } diff --git a/src/soc/mediatek/mt8183/include/soc/addressmap.h b/src/soc/mediatek/mt8183/include/soc/addressmap.h old mode 100644 new mode 100755 index d41b2b9..e9f80d1 --- a/src/soc/mediatek/mt8183/include/soc/addressmap.h +++ b/src/soc/mediatek/mt8183/include/soc/addressmap.h @@ -50,6 +50,7 @@ IOCFG_LB_BASE = IO_PHYS + 0x01E70000, IOCFG_LM_BASE = IO_PHYS + 0x01E80000, IOCFG_BL_BASE = IO_PHYS + 0x01E90000, + EFUSEC_BASE = IO_PHYS + 0x01F10000, IOCFG_LT_BASE = IO_PHYS + 0x01F20000, IOCFG_TL_BASE = IO_PHYS + 0x01F30000, SSUSB_SIF_BASE = IO_PHYS + 0x01F40300, diff --git a/src/soc/mediatek/mt8183/include/soc/efuse.h b/src/soc/mediatek/mt8183/include/soc/efuse.h new file mode 100644 index 0000000..2705497 --- /dev/null +++ b/src/soc/mediatek/mt8183/include/soc/efuse.h @@ -0,0 +1,29 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2018 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _MTK_EFUSE_H +#define _MTK_EFUSE_H + +#include <stdint.h> + +struct efuse_regs { + uint32_t rserved[109]; + uint32_t adc_cali_reg; +}; + +check_member(efuse_regs, adc_cali_reg, 0x1b4); +static struct efuse_regs *const mtk_efuse = (void *)EFUSEC_BASE; + +#endif
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32800/1/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/1/src/soc/mediatek/mt8183/auxadc.c@53 PS1, Line 53: /*In sw implement guide, ge should div 4096. But we don't do that now due to it will multi 4096 later*/ line over 80 characters
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#2).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 68 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/2
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#3).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 73 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/3
Hello Julius Werner, Tristan Hsieh, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#4).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 73 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/4
Hello Julius Werner, Tristan Hsieh, Ben Ho, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#5).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 5:
The build failure said auxadc.c should not have chmod +x set. Can you double check if the file permissions are correct?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1) (~(uint32_t)1)
I personally feel 0xffffffff also works
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@35 PS5, Line 35: int uint32_t, if the init itself was uint32_t type .
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@36 PS5, Line 36: int uint32_t
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int uint32_t (and the rest)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@48 PS5, Line 48: ((cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT) remove one level of quote just like above:
cali_ge_a = (cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT;
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@74 PS5, Line 74: rawvalue I'd prefer calling this raw_value
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali(); will it make more sense if we just put this if-check inside mt_auxadc_update_cali?
So we simply call mt_auxadc_update_cali() here.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t uint64_t ?
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@49 PS5, Line 49: cali_ge_a - 512 Hi Po, What is the possible range of cali_ge_a? Is it 0~1023? (since the mask is 0x3ff) I think this might help us figure out the appropriate data type (int or uint32_t).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
(~(uint32_t)1) […]
Be aware that ~1 is 0xfffffffe, not 0xffffffff (not sure which of the two you wanted here)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
Be aware that ~1 is 0xfffffffe, not 0xffffffff (not sure which of the two you wanted here)
need MTK's confirm why this is ~1 instead of -1 or how it was selected.
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, Ben Ho, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#6).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/6
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, Ben Ho, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#7).
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
mt8183: add efuse calibration in auxadc
due to not get efuse calibration, auxadc get value has deviated Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: jg_poxu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/7
JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
need MTK's confirm why this is ~1 instead of -1 or how it was selected.
I am going to change to ~0xffff
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
uint32_t (and the rest)
Not necessary, because the int type is completely sufficient The value of cali_reg only needs 20 bits lower
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@48 PS5, Line 48: ((cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT)
remove one level of quote just like above: […]
I will fix it
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@49 PS5, Line 49: cali_ge_a - 512
Hi Po, […]
yes
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@74 PS5, Line 74: rawvalue
I'd prefer calling this raw_value
I will fix it
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali();
will it make more sense if we just put this if-check inside mt_auxadc_update_cali? […]
Since auxadc_get_voltage is called frequently, using if-check here can reduce the number of mt_auxadc_update_cali() calls, which can reduce the running time.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
uint64_t ?
Because rawvalue * 1500000 operation results will exceed int32_t
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/#/c/32800/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32800/7//COMMIT_MSG@2 PS7, Line 2: jg_poxu Please use your full name.
$ git config --global user.name "…" $ git commit --amend -s --author="… jg_poxu@mediatek.com"
https://review.coreboot.org/#/c/32800/7//COMMIT_MSG@9 PS7, Line 9: due to not get efuse calibration, auxadc get value has deviated Please start sentences with capital letter and add a dot at the end.
https://review.coreboot.org/#/c/32800/7//COMMIT_MSG@10 PS7, Line 10: add adding
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c@26 PS7, Line 26: : #define ADC_GE_A_SHIFT 10 : #define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT) : #define ADC_OE_A_SHIFT 0 : #define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT) : #define ADC_CALI_EN_A_SHIFT 20 : #define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT) : #define AUXADC_CALI_INIT -0xffff Please use tabs or spaces consistently for alignment.
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c@35 PS7, Line 35: static int cali_oe = AUXADC_CALI_INIT; What does *oe* and *ge* stand for? Add a comment?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
Not necessary, because the int type is completely sufficient […]
The problem is there's no promise int here will be int32_t. And since you'll be reading as read32(), I think the returned value should match what read32 expects, i.e., uint32_t.
Unless if the calculation below may introduce negative values.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali();
Since auxadc_get_voltage is called frequently, using if-check here can reduce the number of mt_auxad […]
We'll probably only call this 4 times (board id, RAM ID, LCM ID, SKU). So I really doubt if that will make much difference.
Meanwhile, if performance is really a concern, I think we should do only single integer compare instead of relying on particular values. i.e.,
static int calibrated = 0; static int cali_oe, cali_ge;
...
if (!calibrated) { mt_auxadc_update_cali(); calibrated = 1; }
This should be more efficient and clear (and get rid of the init magic value thing).
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Because rawvalue * 1500000 operation results will exceed int32_t
Yes I know it can't be int32_t. My question is if this should be int64_t or uint64_t? (given rawvalue is uint).
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, Ben Ho, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#8).
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
mt8183: adding efuse calibration in auxadc
Due to not get efuse calibration, auxadc get value has deviated. Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/8
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, Ben Ho, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32800
to look at the new patch set (#9).
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
mt8183: adding efuse calibration in auxadc
Due to not get efuse calibration, auxadc get value has deviated. Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 67 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/9
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
The problem is there's no promise int here will be int32_t. […]
I think in general it's fine for SoC-specific code to rely on stuff like type sizes which they know will always be the same for that SoC, so where using an int would otherwise make sense I think this would be okay. But anything that holds a register value should always use a fixed-width type of the corresponding size, so yes these should be uint32_t (or possibly int32_t).
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali();
We'll probably only call this 4 times (board id, RAM ID, LCM ID, SKU). […]
Let's not overcomplicate things. Having this check either here or at the top of mt_auxadc_update_cali() both sounds fine. There's no need to optimize away a single function call or a single integer in memory comparison for something that will be called a handful of times per boot.
JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
I am going to change to ~0xffff
As long as it is not in the range of -512~512, it is ok.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@35 PS5, Line 35: int
uint32_t, if the init itself was uint32_t type .
Cali_oe & cali_ge may be negative
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@36 PS5, Line 36: int
uint32_t
Cali_oe & cali_ge may be negative
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
The problem is there's no promise int here will be int32_t. […]
I will fix it
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Yes I know it can't be int32_t. […]
Int64_t is completely enough， The maximum value of raw_valude is only 4095. There is no case of strong data loss. Do you think it should be changed to uint64_t?
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c@26 PS7, Line 26: : #define ADC_GE_A_SHIFT 10 : #define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT) : #define ADC_OE_A_SHIFT 0 : #define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT) : #define ADC_CALI_EN_A_SHIFT 20 : #define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT) : #define AUXADC_CALI_INIT -0xffff
Please use tabs or spaces consistently for alignment.
I will fix it
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
As long as it is not in the range of -512~512, it is ok.
well there's some problem here. ~((uint32_t)1) = -2.
If the calibration values may be negative, especially there's no where explaining there's a range of -512~512, I think it is better to have another variable to control init value instead of trying to determine an "unused value". i,e.,
static int has_calibrated, cali_oe, cali_ge;
And remove the AUXADC_CALI_INIT.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Int64_t is completely enough， […]
Hmmm. I think there are few things that can be improved.
1. If raw_value should 0~4096, I think there can be an assert to clarify.
2. After raw_value -= cali_oe, can it be negative? If yes, then raw_value should be int32_t, or even int64_t.
JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(9 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@33 PS5, Line 33: ~((uint32_t)1)
well there's some problem here. ~((uint32_t)1) = -2. […]
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@35 PS5, Line 35: int
Cali_oe & cali_ge may be negative
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@36 PS5, Line 36: int
Cali_oe & cali_ge may be negative
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
I will fix it
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@48 PS5, Line 48: ((cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT)
I will fix it
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@49 PS5, Line 49: cali_ge_a - 512
yes
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@74 PS5, Line 74: rawvalue
I will fix it
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali();
Let's not overcomplicate things. […]
Done
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Hmmm. I think there are few things that can be improved. […]
1. No need,because it read the value in the data register directly, and value=sample data & 0xfff,fully guaranteed value range from 0 to 4096 2. not negative
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
The buildbot found a failure: "File src/soc/mediatek/mt8183/auxadc.c has one or more executable bits set in the file permissions."
Can you do "chmod a-x auxadc.c", then "git commit --amend -a", and then re-upload?
Hung-Te Lin has uploaded a new patch set (#10) to the change originally created by JG Poxu. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
mt8183: adding efuse calibration in auxadc
Due to not get efuse calibration, auxadc get value has deviated. Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 67 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/10
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 10:
Re-uploaded to correct file permission bits.
@jwerner I think this is ready for review & merge?
If buildbot still fails then I think we can only push and ignore that error.
Hung-Te Lin has uploaded a new patch set (#11) to the change originally created by JG Poxu. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: adding efuse calibration in auxadc ......................................................................
mediatek/mt8183: adding efuse calibration in auxadc
Due to not get efuse calibration, auxadc get value has deviated. Before add efuse calibration code, adc value error range is about +/-50mv, after adding efuse code,the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 67 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/11
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/32800/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32800/11//COMMIT_MSG@7 PS11, Line 7: adding Present tense, imperative mood:
Add …
Hung-Te Lin has uploaded a new patch set (#12) to the change originally created by JG Poxu. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: Add efuse calibration in auxadc ......................................................................
mediatek/mt8183: Add efuse calibration in auxadc
The values from auxadc may be incorrect if not calibrated by efuse. Without calibration, the value error range is about +/-50mv, and after being calibrated the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild; boots on Kukui rev 2 units. BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 67 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/32800/12
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: Add efuse calibration in auxadc ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/32800/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32800/11//COMMIT_MSG@7 PS11, Line 7: adding
Present tense, imperative mood: […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: Add efuse calibration in auxadc ......................................................................
Patch Set 12: Code-Review+2
JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: Add efuse calibration in auxadc ......................................................................
Patch Set 12: Code-Review+1
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mediatek/mt8183: Add efuse calibration in auxadc ......................................................................
mediatek/mt8183: Add efuse calibration in auxadc
The values from auxadc may be incorrect if not calibrated by efuse. Without calibration, the value error range is about +/-50mv, and after being calibrated the error range is about +/-10mv.
BUG=b:131391176 TEST=make clean && make test-abuild; boots on Kukui rev 2 units. BRANCH=none
Change-Id: Iccd6ea0ad810c993f9b62c0974279c960f890e52 Signed-off-by: Po Xu jg_poxu@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32800 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: JG Poxu jg_poxu@mediatek.corp-partner.google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/mediatek/mt8183/auxadc.c M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/efuse.h 3 files changed, 67 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved JG Poxu: Looks good to me, but someone else must approve
diff --git a/src/soc/mediatek/mt8183/auxadc.c b/src/soc/mediatek/mt8183/auxadc.c index a167d2b..5460486 100644 --- a/src/soc/mediatek/mt8183/auxadc.c +++ b/src/soc/mediatek/mt8183/auxadc.c @@ -18,11 +18,37 @@ #include <delay.h> #include <soc/addressmap.h> #include <soc/auxadc.h> +#include <soc/efuse.h> #include <soc/infracfg.h> #include <timer.h>
static struct mtk_auxadc_regs *const mtk_auxadc = (void *)AUXADC_BASE;
+#define ADC_GE_A_SHIFT 10 +#define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT) +#define ADC_OE_A_SHIFT 0 +#define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT) +#define ADC_CALI_EN_A_SHIFT 20 +#define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT) + +static int cali_oe; +static int cali_ge; +static int calibrated = 0; +static void mt_auxadc_update_cali(void) +{ + uint32_t cali_reg; + int cali_ge_a; + int cali_oe_a; + + cali_reg = read32(&mtk_efuse->adc_cali_reg); + + if ((cali_reg & ADC_CALI_EN_A_MASK) != 0) { + cali_oe_a = (cali_reg & ADC_OE_A_MASK) >> ADC_OE_A_SHIFT; + cali_ge_a = (cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT; + cali_ge = cali_ge_a - 512; + cali_oe = cali_oe_a - 512; + } +} static uint32_t auxadc_get_rawdata(int channel) { setbits_le32(&mt8183_infracfg->module_sw_cg_1_clr, 1 << 10); @@ -44,8 +70,17 @@
int auxadc_get_voltage(unsigned int channel) { + uint32_t raw_value; assert(channel < 16);
+ if (!calibrated) { + mt_auxadc_update_cali(); + calibrated = 1; + } + /* 1.5V in 4096 steps */ - return (int)((int64_t)auxadc_get_rawdata(channel) * 1500000 / 4096); + raw_value = auxadc_get_rawdata(channel); + + raw_value = raw_value - cali_oe; + return (int)((int64_t)raw_value * 1500000 / (4096 + cali_ge)); } diff --git a/src/soc/mediatek/mt8183/include/soc/addressmap.h b/src/soc/mediatek/mt8183/include/soc/addressmap.h index d41b2b9..e9f80d1 100644 --- a/src/soc/mediatek/mt8183/include/soc/addressmap.h +++ b/src/soc/mediatek/mt8183/include/soc/addressmap.h @@ -50,6 +50,7 @@ IOCFG_LB_BASE = IO_PHYS + 0x01E70000, IOCFG_LM_BASE = IO_PHYS + 0x01E80000, IOCFG_BL_BASE = IO_PHYS + 0x01E90000, + EFUSEC_BASE = IO_PHYS + 0x01F10000, IOCFG_LT_BASE = IO_PHYS + 0x01F20000, IOCFG_TL_BASE = IO_PHYS + 0x01F30000, SSUSB_SIF_BASE = IO_PHYS + 0x01F40300, diff --git a/src/soc/mediatek/mt8183/include/soc/efuse.h b/src/soc/mediatek/mt8183/include/soc/efuse.h new file mode 100644 index 0000000..32126ab --- /dev/null +++ b/src/soc/mediatek/mt8183/include/soc/efuse.h @@ -0,0 +1,30 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2018 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _MTK_EFUSE_H +#define _MTK_EFUSE_H + +#include <soc/addressmap.h> +#include <types.h> + +struct efuse_regs { + uint32_t rserved[109]; + uint32_t adc_cali_reg; +}; + +check_member(efuse_regs, adc_cali_reg, 0x1b4); +static struct efuse_regs *const mtk_efuse = (void *)EFUSEC_BASE; + +#endif
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
JG Poxu (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tristan Hsieh (Code Review)