No Clock When Using SPI2 on STM32MP157

[georgenyarangi-eaton]

I am seeing an issue where when using SPI2 on the STM32MP157 (actually using the OSD32MP157C SiP), there's no SPI clock signal (verified using logic analyzer), and the SPI driver hangs when calling ll_func_tx_is_empty() which ultimately calls into the STM32MP1 SPI driver.

I first observed this issuing using Zephyr release version 3.0.0 and it is present in v3.1. In v3.0.0, putting LL_RCC_SetSPIClockSource(LL_RCC_SPI23_CLKSOURCE_PER); in spi_stm32_init() seems to fix the issue as a workaround.

In my investigation, I came across PR #40998 which was a first attempt at fixing the the same issue but for the STM32H7.

What I noticed is that SPI works fine on the STM32MP157C-DK2 discovery kit; however, only SPI4 and SPI5 are pinned out to the connectors, and I used SPI4. Looking at the reference manual for the STM32MP157, the default clock source for SPI4 and 5 is pclk2 (peripheral clock) in the RCC_SPI45CKSELR (bits 2:0). However, for SPI2 and 3 the default clock source in the RCC_SPI2S23CKSELR (bits 2:0) is pll4_p_ck.

It looks like this issue #41650 which was addressed by PR #45053 added support for configuring alternate clocks; however, it doesn't seem like the STM32MP1 is supported but I could be wrong.

Any insights on how to best fix this issue would be appreciated.

FYI @erwango, @FRASTM, @scottbronkhorst-eaton

[erwango]

@georgenyarangi-eaton Indeed alternate clock support (#45053) didn't consider MP1.
It was left intentionally since too different from other platforms and I was waiting to see if there was interest before completing this work...

This would be great if you attempt to solve this. This being said you have some little work ahead.
Here are some quick insights, I hope these are correct and can help you to proceed.

Before describing what has to be done, here are the differences to be considered on MP1 vs other plaftorms
Main MP1 clock control driver specifics

• sysclock configuration is not performed on M4 side (stm32_clock_control_init is empty)
• As a consequence, there is no code, nor bindings to configure PLL, fixed clocks, ... the only clocks that are considered are bus clocks

The essence of supporting alternate clocks is to be able to:

• configure these clocks using new stm32_clock_control_configure API
• return clocks rates using stm32_clock_control_get_subsys_rate

Implementing stm32_clock_control_configure is not complex and relies on the implementation of a new clock binding include, such as dt-bindings/clock/stm32h7_clock.h.
This will allow to describe new clock as follows:

    clocks = <&rcc STM32_CLOCK_BUS_APB2 0x00001000>,  <- Existing, bus driven, "register" clock
             <&rcc STM32_SRC_PLL3_P SPI123_SEL(2)>;   <- Newly added alternate clock (PLL3_P output here)

With STM32_SRC_PLL3_P type definitions described as

zephyr/include/zephyr/dt-bindings/clock/stm32h7_clock.h

Lines 13 to 17 in 0456ddc

	/** PLL outputs */
	#define STM32_SRC_PLL1_P 0x001
	#define STM32_SRC_PLL1_Q 0x002
	#define STM32_SRC_PLL1_R 0x003
	/** PLL2 not yet supported */

and SPI123_SEL macros defined as

zephyr/include/zephyr/dt-bindings/clock/stm32h7_clock.h

Lines 95 to 99 in 0456ddc

	/** @brief Device clk sources selection helpers (RM0399.pdf) */
	/** D1CCIPR devices */
	#define FMC_SEL(val) STM32_CLOCK(val, 3, 0, D1CCIPR_REG)
	#define QSPI_SEL(val) STM32_CLOCK(val, 3, 4, D1CCIPR_REG)
	#define DSI_SEL(val) STM32_CLOCK(val, 1, 8, D1CCIPR_REG)

This part will be a bit tedious bit not complex.

For the second part, ideally thanks to use of LL APIs such as LL_RCC_GetUARTClockFreq, no change is requested to get correct freq from stm32_clock_control_get_subsys_rate as these API return the correct device freq, whatever the clock source. Unfortunately, since call to get_subsys_rate is implemented with parameters from the second cell of clocks as parameters (such as STM32_SRC_PLL3_P 0x00001000, instead of using parameters of second cell for other series (such as STM32_SRC_PLL3_P SPI123_SEL(2).
This would mean to add this MP1 specific case in every STM32 driver. Something like:

	/* Get clock rate */
	if (IS_ENABLED(STM32_UART_OPT_CLOCK_SUPPORT) && (config->pclk_len > 1) &&
            !IS_ENABLED(CONFIG_SOC_SERIES_STM32MP1)) {
		if (clock_control_get_rate(data->clock,
					   (clock_control_subsys_t)&config->pclken[1],
					   &clock_rate) < 0) {
			LOG_ERR("Failed call clock_control_get_rate(pclken[1])");
			return;
		}
	} else {
               /*  MP1: Using config->pclken[0] should be used to querry clock freq set using config->pclken[1] */
		if (clock_control_get_rate(data->clock,
					   (clock_control_subsys_t)&config->pclken[0],
					   &clock_rate) < 0) {
			LOG_ERR("Failed call clock_control_get_rate(pclken[0])");
			return;
		}
	}

This is a little bit crappy. So I'm open to any alternative.

[georgenyarangi-eaton]

@erwango Thank you for the prompt feedback, and for confirming that this issue was indeed not addressed for the MP1. This was a head scratcher until I figured out what was going on. I will forward your feedback to our platform team that supports Zephyr and go from there. Ultimately, we (Eaton Corp.) are interested in getting a fix in Zephyr so that we don't have to use patches.

Again, thanks a bunch!