| Message ID | 20190213164648.26579-6-krzk@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
| Series | arm: exynos: Fix reboot on Odroid HC1 | expand |
On Wed, 13 Feb 2019 17:46:44 +0100 Krzysztof Kozlowski <krzk@kernel.org> wrote: > Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel > disabled the LDO4/VDD_ADC regulator. > > The LDO4 supplies both ADC block and the ADC input AIN9. Voltage on > AIN9 will rise slowly, so be patient and wait for it to stabilize. > > First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308 > (reference value is 1309). > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > board/samsung/common/exynos5-dt-types.c | 38 > ++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 > deletion(-) > > diff --git a/board/samsung/common/exynos5-dt-types.c > b/board/samsung/common/exynos5-dt-types.c index > af711e727a78..8aed64183837 100644 --- > a/board/samsung/common/exynos5-dt-types.c +++ > b/board/samsung/common/exynos5-dt-types.c @@ -57,12 +57,48 @@ static > unsigned int odroid_get_rev(void) return 0; > } > > +/* > + * Read ADC at least twice and check the resuls. If regulator > providing voltage > + * on to measured point was just turned on, first reads might > require time > + * to stabilize. > + */ > +static int odroid_get_adc_val(unsigned int *adcval) > +{ > + unsigned int adcval_prev = 0; > + int ret, retries = 20; > + > + ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, > + &adcval_prev); > + if (ret) > + return ret; > + > + while (retries--) { > + mdelay(5); > + > + ret = adc_channel_single_shot("adc", > CONFIG_ODROID_REV_AIN, > + adcval); > + if (ret) > + return ret; > + > + /* > + * If difference between ADC reads is less than 3%, > + * accept the result > + */ > + if ((100 * abs(*adcval - adcval_prev) / adcval_prev) > < 3) > + return ret; > + > + adcval_prev = *adcval; > + } Is there in the documentation any required time to wait before reading the ADC value? If yes then maybe get_timer() based approach shall be used (if get_timer() is available in this context)? Please see for example drivers/net/fec_mxc.c for how timeouts are handled there. I will test this patch series on XU3 during the weekend. Thanks for this fix :-) > + > + return ret; > +} > + > static int odroid_get_board_type(void) > { > unsigned int adcval; > int ret, i; > > - ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, > &adcval); > + ret = odroid_get_adc_val(&adcval); > if (ret) > goto rev_default; > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, 15 Feb 2019 at 08:16, Lukasz Majewski <lukma@denx.de> wrote: > > On Wed, 13 Feb 2019 17:46:44 +0100 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel > > disabled the LDO4/VDD_ADC regulator. > > > > The LDO4 supplies both ADC block and the ADC input AIN9. Voltage on > > AIN9 will rise slowly, so be patient and wait for it to stabilize. > > > > First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308 > > (reference value is 1309). > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > board/samsung/common/exynos5-dt-types.c | 38 > > ++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 > > deletion(-) > > > > diff --git a/board/samsung/common/exynos5-dt-types.c > > b/board/samsung/common/exynos5-dt-types.c index > > af711e727a78..8aed64183837 100644 --- > > a/board/samsung/common/exynos5-dt-types.c +++ > > b/board/samsung/common/exynos5-dt-types.c @@ -57,12 +57,48 @@ static > > unsigned int odroid_get_rev(void) return 0; > > } > > > > +/* > > + * Read ADC at least twice and check the resuls. If regulator > > providing voltage > > + * on to measured point was just turned on, first reads might > > require time > > + * to stabilize. > > + */ > > +static int odroid_get_adc_val(unsigned int *adcval) > > +{ > > + unsigned int adcval_prev = 0; > > + int ret, retries = 20; > > + > > + ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, > > + &adcval_prev); > > + if (ret) > > + return ret; > > + > > + while (retries--) { > > + mdelay(5); > > + > > + ret = adc_channel_single_shot("adc", > > CONFIG_ODROID_REV_AIN, > > + adcval); > > + if (ret) > > + return ret; > > + > > + /* > > + * If difference between ADC reads is less than 3%, > > + * accept the result > > + */ > > + if ((100 * abs(*adcval - adcval_prev) / adcval_prev) > > < 3) > > + return ret; > > + > > + adcval_prev = *adcval; > > + } > > Is there in the documentation any required time to wait before reading > the ADC value? No, I think this delay is not SoC specific. The ADC already has proper delay/conversion rounds. The only thing it misses is to wait for 25 cycles of ADC PCLK after SWRESET but I found that adding it does not affect anything. To my understanding this is delay is purely some charging or slow ramp rate (although measuring point is on simple voltage divider...). > If yes then maybe get_timer() based approach shall be used (if > get_timer() is available in this context)? > > Please see for example drivers/net/fec_mxc.c for how timeouts are > handled there. I can take a look. First read of ADC might be very early so maybe before times... but I will check if these could be used. Best regards, Krzysztof
On Fri, Feb 15, 2019 at 08:15:45AM +0100, Lukasz Majewski wrote: > On Wed, 13 Feb 2019 17:46:44 +0100 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel > > disabled the LDO4/VDD_ADC regulator. > > > > The LDO4 supplies both ADC block and the ADC input AIN9. Voltage on > > AIN9 will rise slowly, so be patient and wait for it to stabilize. > > > > First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308 > > (reference value is 1309). > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > board/samsung/common/exynos5-dt-types.c | 38 > > ++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 > > deletion(-) > > > > diff --git a/board/samsung/common/exynos5-dt-types.c > > b/board/samsung/common/exynos5-dt-types.c index > > af711e727a78..8aed64183837 100644 --- > > a/board/samsung/common/exynos5-dt-types.c +++ > > b/board/samsung/common/exynos5-dt-types.c @@ -57,12 +57,48 @@ static > > unsigned int odroid_get_rev(void) return 0; > > } > > > > +/* > > + * Read ADC at least twice and check the resuls. If regulator > > providing voltage > > + * on to measured point was just turned on, first reads might > > require time > > + * to stabilize. > > + */ > > +static int odroid_get_adc_val(unsigned int *adcval) > > +{ > > + unsigned int adcval_prev = 0; > > + int ret, retries = 20; > > + > > + ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, > > + &adcval_prev); > > + if (ret) > > + return ret; > > + > > + while (retries--) { > > + mdelay(5); > > + > > + ret = adc_channel_single_shot("adc", > > CONFIG_ODROID_REV_AIN, > > + adcval); > > + if (ret) > > + return ret; > > + > > + /* > > + * If difference between ADC reads is less than 3%, > > + * accept the result > > + */ > > + if ((100 * abs(*adcval - adcval_prev) / adcval_prev) > > < 3) > > + return ret; > > + > > + adcval_prev = *adcval; > > + } > > Is there in the documentation any required time to wait before reading > the ADC value? > > If yes then maybe get_timer() based approach shall be used (if > get_timer() is available in this context)? > > Please see for example drivers/net/fec_mxc.c for how timeouts are > handled there. I must admit that I do not see benefit of timers... 1. Make code slightly more complicated (instead of simple retries and mdelay()). 2. Introduce no delay by itself so the ADC reads happen one after another. Probing ADC value fast does not work with my approach of waiting till the values get closer to each other... With timer-based approach, without delay I got: ADC = 660 ADC = 887 ADC = 1031 ADC = 1125 ADC = 1186 ADC = 1226 ADC = 1253 return - because the difference is too small (<3 %). I could narrow my threshold to 1%... but then: ADC = 651 ADC = 881 ADC = 1027 ADC = 1122 ADC = 1184 ADC = 1225 ADC = 1253 ADC = 1271 ADC = 1284 ADC = 1292 I prefer simpler method with delay. Best regards, Krzysztof
Hi Krzysztof, > On Fri, Feb 15, 2019 at 08:15:45AM +0100, Lukasz Majewski wrote: > > On Wed, 13 Feb 2019 17:46:44 +0100 > > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel > > > disabled the LDO4/VDD_ADC regulator. > > > > > > The LDO4 supplies both ADC block and the ADC input AIN9. Voltage > > > on AIN9 will rise slowly, so be patient and wait for it to > > > stabilize. > > > > > > First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308 > > > (reference value is 1309). > > > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > > --- > > > board/samsung/common/exynos5-dt-types.c | 38 > > > ++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 > > > deletion(-) > > > > > > diff --git a/board/samsung/common/exynos5-dt-types.c > > > b/board/samsung/common/exynos5-dt-types.c index > > > af711e727a78..8aed64183837 100644 --- > > > a/board/samsung/common/exynos5-dt-types.c +++ > > > b/board/samsung/common/exynos5-dt-types.c @@ -57,12 +57,48 @@ > > > static unsigned int odroid_get_rev(void) return 0; > > > } > > > > > > +/* > > > + * Read ADC at least twice and check the resuls. If regulator > > > providing voltage > > > + * on to measured point was just turned on, first reads might > > > require time > > > + * to stabilize. > > > + */ > > > +static int odroid_get_adc_val(unsigned int *adcval) > > > +{ > > > + unsigned int adcval_prev = 0; > > > + int ret, retries = 20; > > > + > > > + ret = adc_channel_single_shot("adc", > > > CONFIG_ODROID_REV_AIN, > > > + &adcval_prev); > > > + if (ret) > > > + return ret; > > > + > > > + while (retries--) { > > > + mdelay(5); > > > + > > > + ret = adc_channel_single_shot("adc", > > > CONFIG_ODROID_REV_AIN, > > > + adcval); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * If difference between ADC reads is less than > > > 3%, > > > + * accept the result > > > + */ > > > + if ((100 * abs(*adcval - adcval_prev) / > > > adcval_prev) < 3) > > > + return ret; > > > + > > > + adcval_prev = *adcval; > > > + } > > > > Is there in the documentation any required time to wait before > > reading the ADC value? > > > > If yes then maybe get_timer() based approach shall be used (if > > get_timer() is available in this context)? > > > > Please see for example drivers/net/fec_mxc.c for how timeouts are > > handled there. > > I must admit that I do not see benefit of timers... > 1. Make code slightly more complicated (instead of simple retries and > mdelay()). > 2. Introduce no delay by itself so the ADC reads happen one after > another. Probing ADC value fast does not work with my approach > of waiting till the values get closer to each other... > > With timer-based approach, without delay I got: > ADC = 660 > ADC = 887 > ADC = 1031 > ADC = 1125 > ADC = 1186 > ADC = 1226 > ADC = 1253 > return - because the difference is too small (<3 %). > > I could narrow my threshold to 1%... but then: > ADC = 651 > ADC = 881 > ADC = 1027 > ADC = 1122 > ADC = 1184 > ADC = 1225 > ADC = 1253 > ADC = 1271 > ADC = 1284 > ADC = 1292 > > I prefer simpler method with delay. Ok. I see. Just please add proper comment /or patch description. Thanks for your effort :-) > > Best regards, > Krzysztof > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/board/samsung/common/exynos5-dt-types.c b/board/samsung/common/exynos5-dt-types.c index af711e727a78..8aed64183837 100644 --- a/board/samsung/common/exynos5-dt-types.c +++ b/board/samsung/common/exynos5-dt-types.c @@ -57,12 +57,48 @@ static unsigned int odroid_get_rev(void) return 0; } +/* + * Read ADC at least twice and check the resuls. If regulator providing voltage + * on to measured point was just turned on, first reads might require time + * to stabilize. + */ +static int odroid_get_adc_val(unsigned int *adcval) +{ + unsigned int adcval_prev = 0; + int ret, retries = 20; + + ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, + &adcval_prev); + if (ret) + return ret; + + while (retries--) { + mdelay(5); + + ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, + adcval); + if (ret) + return ret; + + /* + * If difference between ADC reads is less than 3%, + * accept the result + */ + if ((100 * abs(*adcval - adcval_prev) / adcval_prev) < 3) + return ret; + + adcval_prev = *adcval; + } + + return ret; +} + static int odroid_get_board_type(void) { unsigned int adcval; int ret, i; - ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, &adcval); + ret = odroid_get_adc_val(&adcval); if (ret) goto rev_default;
Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel disabled the LDO4/VDD_ADC regulator. The LDO4 supplies both ADC block and the ADC input AIN9. Voltage on AIN9 will rise slowly, so be patient and wait for it to stabilize. First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308 (reference value is 1309). Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- board/samsung/common/exynos5-dt-types.c | 38 ++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-)