STM32 ADC: support prescaler from RCC #97725
Conversation
zephyrbot
added
area: Boards/SoCs
area: ADC
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
anangl,
cfriedt,
djiatsaf-st,
erwango,
etienne-lms,
jhedberg,
kartben,
marwaiehm-st,
mathieuchopstm,
nashif and
nordic-krch
gautierg-st
force-pushed
the
adc_clock_names
branch
from
0a4bf0d to
261b108
Compare
| .pclken = {.bus = DT_INST_CLOCKS_CELL_BY_NAME(index, adcx, bus), \ | ||
| .enr = DT_INST_CLOCKS_CELL_BY_NAME(index, adcx, bits)}, \ |
There was a problem hiding this comment.
Nit: I would make a STM32-level macro for this. Basically all drivers are "wrong" today (div field left uninitialized) because they don't use the shared macro.
There was a problem hiding this comment.
This is a change for another PR. As you say, all drivers are impacted, so I'm not going to introduce such a change in this one. There are enough as it is.
There was a problem hiding this comment.
My 2 cents. div is not left uninitialized, It's 0 which is fine if division is not used (as per the various stm32_clock_control_get_subsys_rate() implemenations), no?
That said, I agree using an STM32 helper macro for these would be worth it.
There was a problem hiding this comment.
My 2 cents.
divis not left uninitialized, It's 0
By uninitialized, I meant that it doesn't take its value from DT 🙂
| (.pclken_ker = {.bus = DT_INST_CLOCKS_CELL_BY_NAME(index, adc_ker, bus), \ | ||
| .enr = DT_INST_CLOCKS_CELL_BY_NAME(index, adc_ker, bits)}, \ | ||
| .has_pclken_ker = true,), \ | ||
| (.has_pclken_ker = false,)) \ |
There was a problem hiding this comment.
Nit: could be IF_ENABLED() and leave has_pclken_ker not initialized => default to false.
But I don't mind the extra verbosity.
There was a problem hiding this comment.
Nits on commit include: zephyr: dt-bindings: clock: stm32: use width instead of bitmask:
- commit title is not very intuitive. prefer the simpler
rework STM32_DT_CLOCK_SELECT details? - Since we're breaking
STM32_DT_CLOCK_SELECT, I would suggest the new prototype to be insteadSTM32_DT_CLOCK_SELECT(val, high_bit, low_bit, reg)
[1] low_bit is merely a rename; high_bit would be the bitfield's MSB index - width would be computed internally as (high_bit - low_bit + 1). When copying RefMan, this would avoid the hurdle of computing bitfield width.
There was a problem hiding this comment.
Ok with changing the commit title, but disagree with your prototype proposition. The rework was needed to gain space, make the macro more compact so that N6 prescaler could fit into it. Width is only 3-bit long, using another bit_pos would be 5-bit long. Reg would have to be reduced (not currently blocking since no reg are that long, but still, it could be needed one day).
Besides, I don't think it's a hurdle to "calculate" the width of the bitfield from RefMan (most are under 4-bit long, that's pretty easy to read).
There was a problem hiding this comment.
Ok with changing the commit title, but disagree with your prototype proposition. The rework was needed to gain space, make the macro more compact so that N6 prescaler could fit into it. Width is only 3-bit long, using another bit_pos would be 5-bit long. Reg would have to be reduced (not currently blocking since no reg are that long, but still, it could be needed one day).
I am not suggesting to change the storage format but merely how the macro accepts parameters. When I said:
widthwould be computed internally as(high_bit - low_bit + 1)
I meant that it would be computed internally as part of the macro (storage format is unchanged and contains width)
There was a problem hiding this comment.
Oh, ok, my bad, I misunderstood your proposition.
I don't have strong opinions on either choice, but I'll wait for other opinions before making any potential changes. @etienne-lms, @erwango ?
There was a problem hiding this comment.
What would be the benefit of high_bit, low_bit ? Readability ?
There was a problem hiding this comment.
I think having the lowest and highest bit position as macro argument make sense, but I'm quite used the GENMASK() like macros. No strong opinion here.
There was a problem hiding this comment.
What would be the benefit of
high_bit, low_bit? Readability ?
Basically yes. Gives the information directly from reading RefMan:
==> ((val), 31, 30, CCIPR) instead of having to derive size (as @gautierg-st said, it is often 1/2 bits so not too much of a hurdle, but still more error-prone than copy-paste from RM, and we're starting to see more and more "large" fields)
I think having the lowest and highest bit position as macro argument make sense, but I'm quite used the
GENMASK()like macros. No strong opinion here.
GENMASK() takes a high/low argument as well so I don't understand your point 🤔
zephyr/include/zephyr/sys/util.h
Lines 76 to 80 in 169fd6a
There was a problem hiding this comment.
GENMASK() takes a high/low argument as well so I don't understand your point 🤔
I meant I'm fine with such API but there maybe other point of view hence no strong opinion of mine on which of high-bit/low-bit or bit-offset/bit-size couple is to be preferred as argument.
There was a problem hiding this comment.
The macro now uses MSB and LSB, all clock fields have been updated. Hopefully without error.
gautierg-st
force-pushed
the
adc_clock_names
branch
from
261b108 to
2745fb7
Compare
|
Rebased to fix conflicts, and comments taken into account. |
gautierg-st
force-pushed
the
adc_clock_names
branch
from
2745fb7 to
4222026
Compare
gautierg-st
force-pushed
the
adc_clock_names
branch
from
60e83a0 to
927fd02
Compare
Thanks, done. |
|
@kylebonnici Seems linter is wrong here: https://github.com/zephyrproject-rtos/zephyr/actions/runs/18772147196/job/53558655897?pr=97725#step:13:176:. It reports failure and proposes the following: dmas = <&gpdma1 0 7 (STM32_DMA_PERIPH_TX | STM32_DMA_16BITS |
- STM32_DMA_PRIORITY_HIGH)>,
+ STM32_DMA_PRIORITY_HIGH)>,
<&gpdma1 1 6 (STM32_DMA_PERIPH_RX | STM32_DMA_16BI |
I've fixed the other issues reported by linter, but I'm waiting for feedback on this one. |
is question s why it is aligning |
Yes, that's the question. |
Reworks the way the STM32_DT_CLOCK_SELECT builds its elements. Instead of taking a mask, it takes the MSB of the field. From the MSB and LSB, we calculate the width of the field, and this width is then stored (instead of the mask). This allows to gain space for higher values for the fields. This larger space is necessary to add the selection of the ADC prescaler on STM32N6 because it is an 8-bit long field. The allowed width is from 1 to 8 (and internally stored as 0-7 to fit on 3 bits). STM32_DT_CLKSEL_MASK_GET keeps the same name, since we still need the mask, and returns the bitmask from the width with the BIT_MASK macro. Other STM32_DT_CLKSEL_MASK_* macros are renamed with WIDTH. All call to STM32_DT_CLOCK_SELECT are updated to reflect the change and use a width instead of a mask. This also fixes a few issues like STM32H7 MCO macros, and adds MCO_PRE for STM32N6. Signed-off-by: Guillaume Gautier <[email protected]>
…and u3 This commit adds the RCC configurations for ADC prescaler for STM32F1, F3, N6 and U3. Signed-off-by: Guillaume Gautier <[email protected]>
To easily differentiate between the different clocks that can be configured in device tree, make their naming mandatory, and explicit what the expected names are. Add these names in all dtsi and dts files that need them. Signed-off-by: Guillaume Gautier <[email protected]>
Some series like F1, F3, N6 and U3 use an ADC prescaler defined in the RCC. Instead of adding specific properties in the RCC driver, use the secondary clock system to configure the prescaler. The ADC driver now configures the clocks depending on their presence and their name. Three clocks can be defined: - the register clock (mandatory for all series) - the kernel clock (depends on series) - the prescaler value (depends on series) Signed-off-by: Guillaume Gautier <[email protected]>
Now that the ADC prescaler are set within the driver using the clock system, the specific rcc compatibles for F1 and F3 are no longer useful. Replace them with the standard one (from which they were derived). Signed-off-by: Guillaume Gautier <[email protected]>
Now that the ADC prescaler are set within the driver using the clock system, remove the specific setting of the prescaler from the clock driver. Signed-off-by: Guillaume Gautier <[email protected]>
Update STM32 ADC binding description now that the STM32F3 ADC asynchronous prescaler is set through the clock property. Signed-off-by: Guillaume Gautier <[email protected]>
Update the 4.3 migration guide to include the change made on the STM32 ADC clocks. Signed-off-by: Guillaume Gautier <[email protected]>
gautierg-st
force-pushed
the
adc_clock_names
branch
from
907feb5 to
0258032
Compare
|
I've fixed the linter warnings since I'd like this PR to be merged for v4.3. |
Fix formatting errors reported by the dts linter. Signed-off-by: Guillaume Gautier <[email protected]>
gautierg-st
force-pushed
the
adc_clock_names
branch
from
0258032 to
ef0e8f9
Compare
This was discussed here: TLDR; the linter will always format with Solution for your use case. dmas = <&gpdma1 0 7
- STM32_DMA_PRIORITY_HIGH)>,
+ (STM32_DMA_PERIPH_TX | STM32_DMA_16BITS | STM32_DMA_PRIORITY_HIGH)>,
<&gpdma1 1 6 (STM32_DMA_PERIPH_RX | STM32_DMA_16BIin some cases this may still be too long .... in such case consider dmas = <&gpdma1 0 7
- STM32_DMA_PRIORITY_HIGH)>,
+ (STM32_DMA_PERIPH_TX | STM32_DMA_16BITS |
+ STM32_DMA_PRIORITY_HIGH)>,
<&gpdma1 1 6 (STM32_DMA_PERIPH_RX | STM32_DMA_16BIDo post an issue on the linter repo and a suggestion and I am happy to consider adding this. FYI this will come into play when we add auto splitting lines that exceed 100 chars. In such a case the linter would opt to move the whole groups to a new line to reduce need to split line again. |
|
8 participants
The goal of this PR is to add the support of the ADC prescaler located in RCC for some series (F1, F3, N6, U3).
Though already supported in F1 and F3, it was through a specific property of the RCC. To avoid increasing the complexity on this end, the prescaler is configured the same way as for a kernel clock, in the ADC
clocksproperty in device tree.This allows to remove the RCC specificities of F1 and F3.
This led to changes in the STM32 clock defines due to the fact that the N6 prescaler register field is 8-bit long, so the macros needed to be reworked to make it fit.
Lastly, since we can now have up to three different clocks in the
clocksproperties, instead of relying on ordering, theclock-namesproperty is mandatory for all ADC instances. Names have been added in existing dtsi, dts and overlay files, but it shall be redefined if a clock source or a prescaler is added.