drivers/adc: stm32: workaround for L462xE errata 2.5.2 and stuck reads #34427
Conversation
github-actions
bot
added
area: ADC
|
It looks like buildkite had errors on tests that don't seems to be related to this PR changes (e.g. tests/kernel/workq/work/kernel.work.api). |
giansta
force-pushed
the
fix/adc-stm32-read-stuck
branch
from
7d08810 to
46ab454
Compare
Unit Test Results 10 files 41 suites 12m 16s ⏱️ Results for commit 46ab4548. |
|
I would like other ADC users could review |
There was a problem hiding this comment.
I am not an extensive adc user, but would like to raise the following two concerns:
- After this change the ADC has to be enabled twice for a single read on G0 series, because of the errata that the ADC_cfgr1 can't be changed while the ADEN bit is set(needed for LL_ADC_SetResolution in start_read()).
It would be great if this could be avoided. - Am I right that the issue is present for the async api?
Could moving the disabling and enabling of the adc into the start_read() function solve both these problems, or do you see any problem that is introduced with such a move.
|
@str4t0m, thanks for your suggestions. I tried to apply the changes you proposed. I could test them with some limitations on our L462VE platform, as we're using an older branch without G0 family support, I'll try to test it more when possible. Unfortunately I don't have any G0 family board to test it. |
According to the ST Errata sheet ES0389 - Rev 5, section 2.5.2, the workaround to the issue of "Wrong ADC result if conversion done late after calibration or previous conversion" is to perform two consecutive ADC conversions. We found also that the same objective is achieved enabling and disabling it at every read. Moreover, this approach solves another issue found on several STM32L462VE samples, where the reads were stuck, giving always the same wrong value, that was 0 or the last calibration value. Additional clearing of ADRDY flag has been added to avoid finding it already set before next enabling. Signed-off-by: Giancarlo Stasi <[email protected]>
Apply workaround for both L462xE errata 2.5.2 and G081xB errata 2.6.2, avoiding to have double enabling for a single read on G0 series. Make workaround for L462xE valid also for the async api. Signed-off-by: Giancarlo Stasi <[email protected]>
giansta
force-pushed
the
fix/adc-stm32-read-stuck
branch
from
4839cdc to
e346a72
Compare
|
@giansta - is this one moving forward still? |
|
@str4t0m - can you please re-review? |
|
@anangl - could you also please review? |
erwango
dismissed
their stale review
Dismiss my review as I missed the second commit. I'm gonna review again
str4t0m
left a comment
str4t0m
left a comment
There was a problem hiding this comment.
I'm not sure if disabling of the ADC after each read should be configurable, but as applications which continuously sample adc values likely use a custom dma based driver doing it always should be fine. (The time it takes until the ADC is ready is equal to 1-2 conversion cycles on series I am familiar with.)
Assuming the ADC is disabled unconditionally after each read, special handling of stm32g0 in lines 412-425 is no longer needed.
However I'd prefer to add a short note to the driver about the special cases on stm32g0 and stm32l462xe. Otherwise these requirements are likely to be overlooked in future driver changes.
Additionally the ADC should also be in disabled state after adc_stm32_init, otherwise the behavior will differ between the first and subsequent reads.
The same applies to start_read: In case calibration fails you should not return immediately, but also disable the ADC.
While the ADC is disabled unconditionally at the end of start_read it is never enabled for CONFIG_SOC_SERIES_STM32F1X and STM32F3X_ADC_V2_5. Could you please make sure that it is also enabled again on these series.
Could you eventually stash the two commits as the 2nd one reverts many changes of the first.
| * still not stabilized, this will wait for a short time to ensure ADC | ||
| * modules are properly enabled. | ||
| */ | ||
| uint32_t countTimeout = 0; |
There was a problem hiding this comment.
Nit: don't use camel case.
| if (LL_ADC_IsEnabled(adc) == 0) { | ||
| err = adc_stm32_enable_and_wait_stabilisation(adc); | ||
| if (err) { | ||
| return err; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you please move this after LL_ADC_SetResolution(), such that no special handling of STM32G0 is necessary.
I don't know of any Series which needs ADC to be enabled to set the resolution so this should be fine.
The HAL includes the following note regarding the LL_ADC_SetResolution macro:
ADC must be disabled or enabled without conversion on going...
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
7 participants
According to the ST Errata sheet ES0389 - Rev 5, section 2.5.2, the
workaround to the issue of "Wrong ADC result if conversion done late
after calibration or previous conversion" is to perform two consecutive
ADC conversions.
We found also that the same objective is achieved enabling and disabling
it at every read. Moreover, this approach solves another issue found on
several STM32L462VE samples, where the reads were stuck, giving always
the same wrong value, that was 0 or the last calibration value.
Additional clearing of ADRDY flag has been added to avoid finding it
already set before next enabling.
Signed-off-by: Giancarlo Stasi [email protected]