STM32H7 SPI123 incorrect clock source used for prescaler calculation

heinwessels commented 2 years ago

Describe the bug

In the STM32H7 series the assumed clock source is incorrect when using SPI1, 2 or 3. This incorrect clock source is then used to calculate the clock prescaler (SPI_CFG:MBR), which can then potentially be calculated incorrectly. More specifically, it assumes the SCK clock source is, for example APB2, while it's by actually by default PLL1Q. This resulted that our custom STM32H743 implementation is outputting an incorrect SPI clock frequency.

This is because for the H7 series the SPI1, 2, and 3 has a seperate clock domain that governs the SCK speed. See the following snippet from RM0433 Figure 609.

Here spi_ker_ck is the SCLK source, not PCLK (f.i. APB2). The value of the spi_ker_ck is set by RCC_D2CCIP1R : SPI123SEL, which is pll1_q_ck by default.

To Reproduce

We can slightly adjust the nucleo_h743zi DTS and abuse the led_ws2812 app to easily see this behaviour.

I'm running this commit from the main branch e2588d6a42
Slightly adjust your code by either: a. Checking out this branch b. Applying the patch nucleo-configuration-patch supplied below.
- [Optional] Add print-clocks-output patch to print out debugging clock information
west build -p auto -b nucleo_h743zi samples/drivers/led_ws2812/
west flash onto a nucleo_h743zi
Probe CN7:10 (D13 or SPI_A_SCK or SPI_1_SCK) with an oscilloscope
Witness the SCK frequency incorrectly being 6MHz instead of the desired and calculated 3MHz.

It will also output the following with the optional logging patch

[00:00:00.005,000] <err> spi_ll_stm32: Is this SPI1? 1
[00:00:00.005,000] <err> spi_ll_stm32: spi clock bus: STM32_CLOCK_BUS_APB2
[00:00:00.005,000] <err> spi_ll_stm32: STM32_CLOCK_BUS_APB2 freq: 96000000
[00:00:00.005,000] <err> spi_ll_stm32: LL_RCC_SPI123_CLKSOURCE: LL_RCC_SPI123_CLKSOURCE_PLL1Q
[00:00:00.005,000] <err> spi_ll_stm32: pllq freq: 192000000
[00:00:00.005,000] <err> spi_ll_stm32: Desired frequency: 3000000
[00:00:00.005,000] <err> spi_ll_stm32: BR: 5 should create clock of freq: 3000000
[00:00:00.005,000] <err> spi_ll_stm32: Resulting actual clock clock with PLL: 6000000

Expected behavior

Expect the prescaler calculation to use the correct clock source to output the desired frequency.

Impact

Hard to notice issues with incorrect SPI frequencies meaning devices run slower/faster than designed for.

Environment (please complete the following information):

OS: Linux
Toolchain: Zephyr SDK 0.13.2
Commit SHA: e2588d6a42

Additional Context

I created #40998 to attempt to fix this issue, but ABOSTM found a showstopping flaw in my implementation. We then proposed a new sollution would be to:

Use the default spi_ker_ck value, since it's not possible to set two clocks the DTS. This means SCK source will always be PLL1Q.
When calculating the prescaler use PLL1Q clock value.
Add checks to ensure PPL1Q is enabled and #error if not.

I will try to create a fix for this implementation soon. However, I'm not sure yet what would be the best way to calculate the PLL1Q frequency, and would like some thoughts on that. The way I did it in the patch below is quite ugly.

It's coincidance that this has gone unnoticed during previous SPI driver testing. If I understand correctly then the Nucleo H743zi is popular for STM32H7 testing, and the PLL1 Q divider usually sets the clock up by chance that PLL1Q == APB2. Therefore I had to change the dts in the patch below to change pll1_q divisor from 4 to 1 to highlight this issue.

Patches

nucleo-configuration-patch

From 21c48a1f7ec70e773cf7639c3d0a3175ab390ff3 Mon Sep 17 00:00:00 2001
From: Hein Wessels <hein.wessels@nobleo.nl>
Date: Fri, 7 Jan 2022 14:19:30 +0100
Subject: [PATCH] nucleo config

---
 .../led_ws2812/boards/nucleo_h743.conf        |  2 +
 .../led_ws2812/boards/nucleo_h743zi.overlay   | 39 +++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100644 samples/drivers/led_ws2812/boards/nucleo_h743.conf
 create mode 100644 samples/drivers/led_ws2812/boards/nucleo_h743zi.overlay

diff --git a/samples/drivers/led_ws2812/boards/nucleo_h743.conf b/samples/drivers/led_ws2812/boards/nucleo_h743.conf
new file mode 100644
index 0000000000..319a42ce98
--- /dev/null
+++ b/samples/drivers/led_ws2812/boards/nucleo_h743.conf
@@ -0,0 +1,2 @@
+CONFIG_WS2812_STRIP_SPI=y
+CONFIG_SPI=y
diff --git a/samples/drivers/led_ws2812/boards/nucleo_h743zi.overlay b/samples/drivers/led_ws2812/boards/nucleo_h743zi.overlay
new file mode 100644
index 0000000000..0d1fba0ac4
--- /dev/null
+++ b/samples/drivers/led_ws2812/boards/nucleo_h743zi.overlay
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2021, STMicroelectronics
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <dt-bindings/led/led.h>
+
+arduino_spi: &spi1 {};
+
+
+&arduino_spi {
+   led_strip: ws2812@0 {
+       compatible = "worldsemi,ws2812-spi";
+       label = "WS2812";
+
+       /* SPI */
+       reg = <0>; /* ignored, but necessary for SPI bindings */
+       spi-max-frequency = <3000000>;
+
+       /* WS2812 */
+       chain-length = <16>; /* arbitrary; change at will */
+       spi-one-frame = <0x70>;
+       spi-zero-frame = <0x40>;
+       color-mapping = <LED_COLOR_ID_GREEN
+                LED_COLOR_ID_RED
+                LED_COLOR_ID_BLUE>;
+   };
+};
+
+/ {
+   aliases {
+       led-strip = &led_strip;
+   };
+};
+
+&pll{
+   div-q = <1>;
+};
\ No newline at end of file
-- 
2.25.1

print-clocks-output

From e376b740607da0f007265748cdae4defbb02bc1e Mon Sep 17 00:00:00 2001
From: Hein Wessels <hein.wessels@nobleo.nl>
Date: Fri, 7 Jan 2022 14:46:26 +0100
Subject: [PATCH] Add log output

---
 drivers/spi/spi_ll_stm32.c | 94 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/spi/spi_ll_stm32.c b/drivers/spi/spi_ll_stm32.c
index 47a5ae01d6..d779329ca0 100644
--- a/drivers/spi/spi_ll_stm32.c
+++ b/drivers/spi/spi_ll_stm32.c
@@ -451,6 +451,72 @@ static void spi_stm32_isr(const struct device *dev)
 }
 #endif

+#include <stm32_ll_rcc.h>
+
+#define PLLQ_FREQ(pllsrc_freq, divm, divn, divq)   (((pllsrc_freq)*\
+                           (divn))/((divm)*(divq)))
+
+static inline uint32_t get_pllsrc_frequency(void)
+{
+   switch (LL_RCC_PLL_GetSource()) {
+   case LL_RCC_PLLSOURCE_HSI:
+       return HSI_VALUE;
+   case LL_RCC_PLLSOURCE_CSI:
+       return CSI_VALUE;
+   case LL_RCC_PLLSOURCE_HSE:
+       return HSE_VALUE;
+   case LL_RCC_PLLSOURCE_NONE:
+   default:
+       return 0;
+   }
+}
+
+static uint32_t get_pllq_frequency(void)
+{
+   uint32_t sysclk = 0;
+   uint32_t hpre = 0;
+   uint32_t hsidiv = 0;
+
+   /* Get the current HSI divider */
+   switch (LL_RCC_HSI_GetDivider()) {
+   case LL_RCC_HSI_DIV2:
+       hsidiv = 2;
+       break;
+   case LL_RCC_HSI_DIV4:
+       hsidiv = 4;
+       break;
+   case LL_RCC_HSI_DIV8:
+       hsidiv = 8;
+       break;
+   case LL_RCC_HSI_DIV1:
+   default:
+       hsidiv = 1;
+       break;
+   }
+
+   /* Get the current system clock source */
+   switch (LL_RCC_GetSysClkSource()) {
+   case LL_RCC_SYS_CLKSOURCE_STATUS_HSI:
+       sysclk = HSI_VALUE/hsidiv;
+       break;
+   case LL_RCC_SYS_CLKSOURCE_STATUS_CSI:
+       sysclk = CSI_VALUE;
+       break;
+   case LL_RCC_SYS_CLKSOURCE_STATUS_HSE:
+       sysclk = HSE_VALUE;
+       break;
+   case LL_RCC_SYS_CLKSOURCE_STATUS_PLL1:
+       sysclk = PLLQ_FREQ(get_pllsrc_frequency(),
+                  LL_RCC_PLL1_GetM(),
+                  LL_RCC_PLL1_GetN(),
+                  LL_RCC_PLL1_GetQ());
+       break;
+   }
+
+   return sysclk;
+
+}
+
 static int spi_stm32_configure(const struct device *dev,
                   const struct spi_config *config)
 {
@@ -486,13 +552,41 @@ static int spi_stm32_configure(const struct device *dev,
        return -EIO;
    }

+   LOG_ERR("Is this SPI1? %d", spi == SPI1);
+   LOG_ERR("spi clock bus: %s", cfg->pclken.bus == 3 ? "STM32_CLOCK_BUS_APB2" : "something else" );
+   LOG_ERR("STM32_CLOCK_BUS_APB2 freq: %d", clock);
+
+   switch(LL_RCC_GetSPIClockSource(LL_RCC_SPI123_CLKSOURCE)){
+       case LL_RCC_SPI123_CLKSOURCE_PLL1Q:
+           LOG_ERR("LL_RCC_SPI123_CLKSOURCE: %s", "LL_RCC_SPI123_CLKSOURCE_PLL1Q");
+           break;
+       case LL_RCC_SPI123_CLKSOURCE_PLL2P:
+           LOG_ERR("LL_RCC_SPI123_CLKSOURCE: %s", "LL_RCC_SPI123_CLKSOURCE_PLL2P");
+           break;
+       case LL_RCC_SPI123_CLKSOURCE_PLL3P:
+           LOG_ERR("LL_RCC_SPI123_CLKSOURCE: %s", "LL_RCC_SPI123_CLKSOURCE_PLL3P");
+           break;
+       case LL_RCC_SPI123_CLKSOURCE_I2S_CKIN:
+           LOG_ERR("LL_RCC_SPI123_CLKSOURCE: %s", "LL_RCC_SPI123_CLKSOURCE_I2S_CKIN");
+           break;
+       case LL_RCC_SPI123_CLKSOURCE_CLKP:
+           LOG_ERR("LL_RCC_SPI123_CLKSOURCE: %s", "LL_RCC_SPI123_CLKSOURCE_CLKP");
+           break;          
+   }   
+   LOG_ERR("pllq freq: %d", get_pllq_frequency());
+
+   LOG_ERR("Desired frequency: %d", config->frequency);
+
    for (br = 1 ; br <= ARRAY_SIZE(scaler) ; ++br) {
        uint32_t clk = clock >> br;

        if (clk <= config->frequency) {
+           LOG_ERR("BR: %d should create clock of freq: %d", br, clk);
            break;
        }
    }
+   
+   LOG_ERR("Resulting actual clock clock with PLL: %d", get_pllq_frequency() >> br);   

    if (br > ARRAY_SIZE(scaler)) {
        LOG_ERR("Unsupported frequency %uHz, max %uHz, min %uHz",
-- 
2.25.1

erwango commented 2 years ago

Thanks for raising this @heinwessels.

I'm currently looking at supporting multiple/alternate peripheral clock source by STM32 clock_control driver. One of the advantage would be to centralize most of the work in one common driver, rather than splitting it in multiple peripheral drivers (which would also have to support similar problem for multiple series)

I still need to give it more thoughts before sharing more details but it looks like the case you mention would be a good starting point.

Btw, and this is a different topic but I see that the sample you're using is led_strip: ws2812. This is worrying me a little since we have identified a different issue with this driver, along with a different solution. Would you mind having a look to following points ?

https://github.com/zephyrproject-rtos/zephyr/issues/38683 (identified issue)
https://github.com/zephyrproject-rtos/zephyr/pull/40860 (proposed solution) I'd like to be sure this is orthogonal to the problem you're seeing on H7 SPI clocks.

heinwessels commented 2 years ago

Btw, and this is a different topic but I see that the sample you're using is led_strip: ws2812. This is worrying me a little since we have identified a different issue with this driver, along with a different solution. Would you mind having a look to following points ?

@erwango I do believe this not at all a framing issue, and therefore not related to the issue you mentioned. It's also not limited to the ws2812 at all. I picked up this issue when using the SPI for normal communiation (controlling a ili9341 LCD display). I simply used ws2812 sample as it seemed like the fastest way to get a dummy SPI peripheral running as minimal working example.

I'm currently looking at supporting multiple/alternate peripheral clock source by STM32 clock_control driver. One of the advantage would be to centralize most of the work in one common driver, rather than splitting it in multiple peripheral drivers (which would also have to support similar problem for multiple series)

Do you think I should still do an temporary fix for this SPI case in the mean time? A more comprehensive clock driver is the better solution, but it might still be some while off. There might be users running at different SPI frequencies without knowing it until then.

erwango commented 2 years ago

@erwango I do believe this not at all a framing issue, and therefore not related to the issue you mentioned. It's also not limited to the ws2812 at all. I picked up this issue when using the SPI for normal communiation (controlling a ili9341 LCD display). I simply used ws2812 sample as it seemed like the fastest way to get a dummy SPI peripheral running as minimal working example.

Ok. I didn't want to say this was the root cause of the clock issue you've identified, as it seems to be a well established issue. I just wanted to be sure we're not colliding 2 different issues here by picking up an already broken example. And if not the case then I wonder if #40860 is required if you're able to get the sample working w/o it. Though, anyway as mentioned, this is a totally different topic.

Do you think I should still do an temporary fix for this SPI case in the mean time?

One idea would be to push it as a PR and not getting it merged, so it is available to people that may face the issue. It may also be a good comparison point to the upcoming clock_control driver enhancement. I'm reluctant to merge this code if we already know it has to be removed rather soon, but in case of known delay, it is still an option.

heinwessels commented 2 years ago

I just wanted to be sure we're not colliding 2 different issues here by picking up an already broken example.

I don't think that's the case :) I made really sure that it's indeed a bug, because it's been giving me quite a few headaches.

I'm reluctant to merge this code if we already know it has to be removed rather soon, but in case of known delay, it is still an option.

I understand completely. Then it will be lower priority for me to create a temporary fix. In our implementation I will simply change the PLL Q divisor so that it matches the expected clock source frequency for now, since it's not used for anything else. Hacky, but should suffice until your refactoring at some point.

github-actions[bot] commented 2 years ago

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

zephyrproject-rtos / zephyr