drivers: adc: stm32: Implement boost settings for STM32H7. #78047

bas-archembedded · 2024-09-05T09:59:07Z

After boost is implemented and enabled it allows for higher sampling frequencies.

erwango · 2024-09-05T10:10:13Z

Please review commit title: drivers: adc: stm32: ...

decsny

do formatting changes in a separate commit

erwango

Naive question in order to help simplifying the driver a bit (if ever possible)

drivers/adc/adc_stm32.c

dismiss, vendor please review first

gautierg-st

Thanks for your PR, I wasn't aware of the boost mode for H7.
A few changes requested but otherwise looks good.

gautierg-st · 2024-09-11T11:08:52Z

drivers/adc/adc_stm32.c

+	/* For revision Y we have no prescaler of 2 */
+	if (LL_DBGMCU_GetRevisionID() <= 0x1003) {
+		presc = 1;
+	}


Suggested change

/* For revision Y we have no prescaler of 2 */

if (LL_DBGMCU_GetRevisionID() <= 0x1003) {

presc = 1;

}

#ifdef ADC_VER_V5_X

/* For revision Y we have no prescaler of 2 */

if (LL_DBGMCU_GetRevisionID() <= 0x1003) {

presc = 1;

}

#endif

This specificity is only present on STM32H742, H743, H750 and H753. So add an #ifdef to avoid conflict with other subseries (like H72x or H73x whose revisions are 0x1000 or 0x1001)

gautierg-st · 2024-09-11T11:13:30Z

drivers/adc/adc_stm32.c

+		.dma_cfg =                                                                         \
+			{                                                                          \


Nit: It looks strange to have the brace on a new line when just a few lines above, the brace after .dma is on the same line.
I think it would be best to keep the previous formatting (whatever clang-format may say)

gautierg-st · 2024-09-11T11:31:13Z

drivers/adc/adc_stm32.c

+#if defined(CONFIG_SOC_SERIES_STM32C0X) || defined(CONFIG_SOC_SERIES_STM32G0X) ||                  \
+	defined(CONFIG_SOC_SERIES_STM32L0X) ||                                                     \
+	(defined(CONFIG_SOC_SERIES_STM32WBX) && defined(ADC_SUPPORT_2_5_MSPS)) ||                  \
+	defined(CONFIG_SOC_SERIES_STM32WLX) || defined(CONFIG_SOC_SERIES_STM32H7X)
+static bool adc_stm32_is_clk_sync(const struct adc_stm32_cfg *config)


I don't really like to have the same #if defined duplicated in two places.
I would prefer to have something like

#if defined(CONFIG_SOC_SERIES_STM32C0X) || \ defined(CONFIG_SOC_SERIES_STM32G0X) || \ defined(CONFIG_SOC_SERIES_STM32L0X) || \ (defined(CONFIG_SOC_SERIES_STM32WBX) && defined(ADC_SUPPORT_2_5_MSPS)) || \ defined(CONFIG_SOC_SERIES_STM32WLX) define HAS_INDIVIDUAL_CLOCK_MODE #endif

And then use this define in the two places.
I'm not too sure about the define name though, don't hesitate to propose something else if you have ideas.
Note that keeping one SOC_SERIES per line is preferred for readability and adding potential future series.

gautierg-st · 2024-09-12T07:03:52Z

drivers/adc/adc_stm32.c

+
+	/* Get the input frequency */
+	clk_src = (clock_control_subsys_t)(adc_stm32_is_clk_sync(config) ? &config->pclken[0]
+									 : &config->pclken[1]);


Another thing I just noticed, config->pclken[1] does not necessarily exist. It is currently possible to be in ASYNC but not defining a secondary clock source. In that case, PLL2_P will be used (default one). But I agree it is not a good practice.

Maybe you could add a check (not limited to H7) to generate a compile-time error if ASYNC is defined without an explicit secondary clock.

But you still have to update this line for cases in SYNC that may not have the secondary clock.

@gautierg-st I did not understand the last line.. Why does it matter if pclken[1] is not set when running in SYNC mode ? AFAICS we only need to check pclken[1] when ASYNC mode is used ?

Well if you add global check, then yeah, your current code should be ok.

But thinking about it, I believe a simple check like the following could suffice:

clk_src = &config->pclken[0]; if (IS_ENABLED(STM32_ADC_DOMAIN_CLOCK_SUPPORT) && (config->pclk_len > 1) && !adc_stm32_is_clk_sync(config)) { clk_src = &config->pclken[1]; }

Well if you add global check, then yeah, your current code should be ok.

But thinking about it, I believe a simple check like the following could suffice:

clk_src = &config->pclken[0]; if (IS_ENABLED(STM32_ADC_DOMAIN_CLOCK_SUPPORT) && (config->pclk_len > 1) && !adc_stm32_is_clk_sync(config)) { clk_src = &config->pclken[1]; }

I have chosen for a compile error check as that code is running on H7 only anyway. Please check the current version ;)

gautierg-st · 2024-09-13T06:43:44Z

drivers/adc/adc_stm32.c

+#define ADC_STM32_CHECK_DT_CLOCK(x)                                                                \
+	BUILD_ASSERT(                                                                              \
+		IS_EQ(ADC_STM32_CLOCK(x), SYNC) ||                                                 \
+			(IS_ENABLED(STM32_ADC_DOMAIN_CLOCK_SUPPORT) && DT_INST_NUM_CLOCKS(x) > 1), \


I think this could be simplified with BUILD_ASSERT(IS_EQ(ADC_STM32_CLOCK(x), SYNC) || (DT_INST_NUM_CLOCKS(x) > 1)),
I don't think IS_ENABLED(STM32_ADC_DOMAIN_CLOCK_SUPPORT) brings more information than (DT_INST_NUM_CLOCKS(x) > 1)

@gautierg-st Isn't that define true if any of the instances has a second clock defined ? DT_INST_NUM_CLOCKS(x) is the same as config->pclk_len for a specific instance.

Exactly, and we care only about our specific instance, either we're in SYNC and we're good, or we have a domain clock and we're also good. It isn't important to check whether other instances have a domain clock here.

@gautierg-st Updated.

gautierg-st · 2024-09-13T06:49:28Z

drivers/adc/adc_stm32.c

+                                                                                                   \
+	static const struct adc_stm32_cfg adc_stm32_cfg_##index = {                                \
+		.base = (ADC_TypeDef *)DT_INST_REG_ADDR(index),                                    \
+		ADC_STM32_IRQ_FUNC(index).pclken = pclken_##index,                                 \


ADC_STM32_IRQ_FUNC(index) \ .pclken = pclken_##index, \

Besides, I just noticed that clang-format has replaced all tabs before the \ by spaces. Not sure of Zephyr guideline about that. @erwango?

I don't think it is blocking but tbh I'd drop the whole second commit as it doesn't add any value.
Those clang warnings are not blocking (and sometime even not good advices at all)

@gautierg-st It was done by clang-format, using the in tree .clang-format so I hope that reflects the standard :)

I don't think it is blocking but tbh I'd drop the whole second commit as it doesn't add any value. Those clang warnings are not blocking (and sometime even not good advices at all)

I've removed it.

@gautierg-st @erwango I assume this needs to be rebased now that the check is included in the new PR?

Yes this will need to be rebased after #78523 is merged. Hopefully soon!

After boost is implemented and enabled it allows for higher sampling frequencies. Signed-off-by: Bas van Loon <[email protected]>

gautierg-st

Looks good to me! Thank you for your patience :)

gautierg-st · 2024-09-13T09:25:39Z

Unsurprisingly, the check breaks things for several boards. I understand that it is out of scope of your initial PR, so I can take this point if you prefer. I can add the check and fix the issues in another PR. Let me know

bas-archembedded · 2024-09-14T08:23:12Z

Unsurprisingly, the check breaks things for several boards. I understand that it is out of scope of your initial PR, so I can take this point if you prefer. I can add the check and fix the issues in another PR. Let me know

Sure, go ahead. I have limited boards to check it with so if you can bring the broken boards along it would be great.

zephyrbot added platform: STM32 ST Micro STM32 area: ADC Analog-to-Digital Converter (ADC) labels Sep 5, 2024

zephyrbot requested review from anangl, decsny, erwango, FRASTM, gautierg-st, GeorgeCGV, marwaiehm-st and mathieuchopstm September 5, 2024 09:59

zephyrbot assigned erwango Sep 5, 2024

bas-archembedded force-pushed the dev/stm32_boost branch 2 times, most recently from adbec4a to df9a785 Compare September 5, 2024 10:22

bas-archembedded changed the title ~~STM32: Implement boost settings for STM32H7.~~ drivers: adc: stm32: Implement boost settings for STM32H7. Sep 5, 2024

bas-archembedded force-pushed the dev/stm32_boost branch from df9a785 to 4551c98 Compare September 5, 2024 10:28

decsny previously requested changes Sep 5, 2024

View reviewed changes

erwango requested changes Sep 5, 2024

View reviewed changes

drivers/adc/adc_stm32.c Outdated Show resolved Hide resolved

drivers/adc/adc_stm32.c Outdated Show resolved Hide resolved

drivers/adc/adc_stm32.c Outdated Show resolved Hide resolved

bas-archembedded force-pushed the dev/stm32_boost branch 2 times, most recently from adf7191 to 9ae95a1 Compare September 6, 2024 10:45

bas-archembedded requested a review from decsny September 6, 2024 14:23

gautierg-st requested changes Sep 11, 2024

View reviewed changes

decsny removed their request for review September 11, 2024 16:11

gautierg-st requested changes Sep 12, 2024

View reviewed changes

bas-archembedded force-pushed the dev/stm32_boost branch 4 times, most recently from fa9259f to aed6d67 Compare September 12, 2024 14:43

gautierg-st requested changes Sep 13, 2024

View reviewed changes

bas-archembedded force-pushed the dev/stm32_boost branch 2 times, most recently from f40e3ff to 40759e2 Compare September 13, 2024 07:23

drivers: adc: stm32: Implement boost settings for STM32H7.

e8048ae

After boost is implemented and enabled it allows for higher sampling frequencies. Signed-off-by: Bas van Loon <[email protected]>

bas-archembedded force-pushed the dev/stm32_boost branch from 40759e2 to e8048ae Compare September 13, 2024 07:27

gautierg-st approved these changes Sep 13, 2024

View reviewed changes

gautierg-st mentioned this pull request Sep 17, 2024

STM32 ADC asynchronous clock check #78523

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: adc: stm32: Implement boost settings for STM32H7. #78047

drivers: adc: stm32: Implement boost settings for STM32H7. #78047

bas-archembedded commented Sep 5, 2024

erwango commented Sep 5, 2024

decsny left a comment

erwango left a comment

gautierg-st left a comment

gautierg-st Sep 11, 2024

gautierg-st Sep 11, 2024

gautierg-st Sep 11, 2024

gautierg-st Sep 12, 2024

bas-archembedded Sep 12, 2024

gautierg-st Sep 12, 2024

bas-archembedded Sep 12, 2024

gautierg-st Sep 13, 2024

bas-archembedded Sep 13, 2024

gautierg-st Sep 13, 2024 •

edited

Loading

bas-archembedded Sep 13, 2024

gautierg-st Sep 13, 2024

erwango Sep 13, 2024

bas-archembedded Sep 13, 2024

bas-archembedded Sep 13, 2024

bas-archembedded Sep 19, 2024

gautierg-st Sep 19, 2024

gautierg-st left a comment

gautierg-st commented Sep 13, 2024

bas-archembedded commented Sep 14, 2024

drivers: adc: stm32: Implement boost settings for STM32H7. #78047

Are you sure you want to change the base?

drivers: adc: stm32: Implement boost settings for STM32H7. #78047

Conversation

bas-archembedded commented Sep 5, 2024

erwango commented Sep 5, 2024

decsny left a comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

gautierg-st left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gautierg-st Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gautierg-st left a comment

Choose a reason for hiding this comment

gautierg-st commented Sep 13, 2024

bas-archembedded commented Sep 14, 2024

gautierg-st Sep 13, 2024 •

edited

Loading