Wrong PLL configuration on STM32H743

[isaev-d]

In file targets/TARGET_STM/TARGET_STM32H7/TARGET_STM32H743xI/TARGET_NUCLEO_H743ZI2/system_clock.c I see comment:

/**
  * This file configures the system clock as follows:
  *--------------------------------------------------------------------
  * System clock source   | 1- USE_PLL_HSE_EXTC (external 8 MHz clock)
  *                       | 2- USE_PLL_HSE_XTAL (external 8 MHz xtal)
  *                       | 3- USE_PLL_HSI (internal 64 MHz clock)
  *--------------------------------------------------------------------
  * SYSCLK(MHz)           |            480
  * AHBCLK (MHz)          |            240
  * APB1CLK (MHz)         |            120
  * APB2CLK (MHz)         |            120
  * APB3CLK (MHz)         |            120
  * APB4CLK (MHz)         |            120
  * USB capable (48 MHz)  |            YES
  *--------------------------------------------------------------------
**/

however function SetSysClock_PLL_HSI sets PLL controls to

RCC_OscInitStruct.PLL.PLLM = 8;
RCC_OscInitStruct.PLL.PLLN = 100;
RCC_OscInitStruct.PLL.PLLP = 2;
RCC_OscInitStruct.PLL.PLLQ = 2;
RCC_OscInitStruct.PLL.PLLR = 2;

( (64 /8) * 100 ) / 2 = 400, not a 480. So in case of using internal clock ant not external 8 MHz clock we have sysclock that 17% lower than expected.

My board does not have that external clock as Nucleo have so I'm using internal clock. I'v noticed that stdio UART baud rate is 8570, not 9600.

Another problem is ADC clock configuration as I reported in https://github.com/ARMmbed/mbed-os/issues/13535

If we look inside targets/TARGET_STM/TARGET_STM32H7/TARGET_STM32H743xI/analog_device.c :

    PeriphClkInitStruct.PLL2.PLL2M = 4;
    PeriphClkInitStruct.PLL2.PLL2N = 240;
    PeriphClkInitStruct.PLL2.PLL2P = 2;

PLL configured strictly for 8MHz external clock. In case of internal 64 MHz clock system get "analogin_init HAL_RCCEx_PeriphCLKConfig" error while enabling analog in.

I think the best solution is to divide internall 64 MHz clock to 8 MHz and keep PLL settings equal for whole code. In this case only HSIDiv register should be updated.

Board - DevEBox STM32H7XX (H743VI). Build with PlatformIO + VS Code on Linux system. framework-mbed 6.60200.200722 (6.2.0)

[isaev-d]

Also it would be nice if SetSysClock_PLL_HSE(uint8_t bypass) from system_clock.c were applying different PLL settings according bypass parameter.

Example: now

/* Enable HSE Oscillator and activate PLL with HSE as source */
    RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSE | RCC_OSCILLATORTYPE_HSI48;
    if (bypass) {
        RCC_OscInitStruct.HSEState = RCC_HSE_BYPASS;
    } else {
        RCC_OscInitStruct.HSEState = RCC_HSE_ON;
    }
    RCC_OscInitStruct.HSI48State = RCC_HSI48_ON;
    RCC_OscInitStruct.PLL.PLLState = RCC_PLL_ON;
    RCC_OscInitStruct.PLL.PLLSource = RCC_PLLSOURCE_HSE;
    RCC_OscInitStruct.PLL.PLLM = 4;   // 2 MHz
    RCC_OscInitStruct.PLL.PLLN = 480; // 960 MHz
    RCC_OscInitStruct.PLL.PLLP = 2;   // PLLCLK = SYSCLK = 480 MHz

more flexible code

    /* Enable HSE Oscillator and activate PLL with HSE as source */
    RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSE | RCC_OSCILLATORTYPE_HSI48;
    // external 8MHz clock
    if (bypass) {
        RCC_OscInitStruct.HSEState = RCC_HSE_BYPASS;
        RCC_OscInitStruct.PLL.PLLM = 4;   // 2 MHz
        RCC_OscInitStruct.PLL.PLLN = 480; // 960 MHz
    // external 25MHz XTAL
    } else {
        RCC_OscInitStruct.HSEState = RCC_HSE_ON;
        RCC_OscInitStruct.PLL.PLLM = 5;   // 5 MHz
        RCC_OscInitStruct.PLL.PLLN = 192; // 960 MHz
    }
    RCC_OscInitStruct.HSI48State = RCC_HSI48_ON;
    RCC_OscInitStruct.PLL.PLLState = RCC_PLL_ON;
    RCC_OscInitStruct.PLL.PLLSource = RCC_PLLSOURCE_HSE;

    RCC_OscInitStruct.PLL.PLLP = 2;   // PLLCLK = SYSCLK = 480 MHz

[jeromecoutant]

Hi

• about SetSysClock_PLL_HSE update, I agree. I will approve your coming PR :-)

• about ADC clock configuration, I propose to add a weak parameter for the new analogin_pll_configuration function. You could then override it in your application ?

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers. Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2822

[isaev-d]

I think that basic samples should work out of box. If someone will try to use ADC on nucleo H743 board and would switch to internal clock, error will occur with 100% probability. So weak parameters or functions are acceptable for XTAL as it is not preset on nucleo, but not for internal oscillator.

[jeromecoutant]

* about SetSysClock_PLL_HSE update, I agree. I will approve your coming PR :-)

Hi Maybe I could propose another way for coding? In fact the different needed configurations depend on HSE_VALUE, so code should be something like:

    if (bypass) {
        RCC_OscInitStruct.HSEState = RCC_HSE_BYPASS;
    } else {
        RCC_OscInitStruct.HSEState = RCC_HSE_ON;
    }
#if HSE_VALUE==8000000
        RCC_OscInitStruct.PLL.PLLM = 4;   // 2 MHz
        RCC_OscInitStruct.PLL.PLLN = 480; // 960 MHz
#elif HSE_VALUE==25000000
        RCC_OscInitStruct.PLL.PLLM = 5;   // 5 MHz
        RCC_OscInitStruct.PLL.PLLN = 192; // 960 MHz
#else
error("xxx")
#endif

[isaev-d]

OK, that is fine.

[jeromecoutant]

Good, I am waiting for PR review ! Thx

[0xc0170]

@isaev-d Could you please send a pull request? Thanks

[isaev-d]

Sorry, I was sick for a couple of days. I will try tomorrow.

[isaev-d]

What I know after all:

• PIO does not support mbed os 6.3 at the moment. Old build scripts somehow stopped working.
• My Linux distribution does not supported by Mbed Studio out-of-box. Even after couple evenings of hacking that stuff PyOCD doesn't work as it should.
• So, for me mbed cli + openocd + minicom (+ gdb?) is the only way to contribute fixes for latest releases.

I'm gonna walk a little, breathe oxygen.. then I will prepare PE for ADC.

[AGlass0fMilk]

I'm gonna walk a little, breathe oxygen..

I think we all need to do this a little more...

[isaev-d]

@jeromecoutant, help me please. I know it is simple, but I'm stuck. I would like to patch your code like this:

{
void analogin_pll_configuration(void)
{
#if defined(DUAL_CORE)
    while (LL_HSEM_1StepLock(HSEM, CFG_HW_RCC_SEMID)) {
    }
#endif /* DUAL_CORE */

    RCC_PeriphCLKInitTypeDef PeriphClkInitStruct = {0};
    PeriphClkInitStruct.PeriphClockSelection = RCC_PERIPHCLK_ADC;
#if ( ((CLOCK_SOURCE) & USE_PLL_HSE_EXTC) & HSE_VALUE==8000000 )
    RCC_OscInitStruct.PLL2.PLL2M = 4;   // 2 MHz
    RCC_OscInitStruct.PLL2.PLLN = 240; // 480 MHz
#elif ( ((CLOCK_SOURCE) & USE_PLL_HSE_XTAL) & HSE_VALUE==25000000 )
    RCC_OscInitStruct.PLL2.PLL2M = 5;   // 5 MHz
    RCC_OscInitStruct.PLL2.PLL2N = 96; // 480 MHz
#elif ((CLOCK_SOURCE) & USE_PLL_HSI)
    RCC_OscInitStruct.PLL2.PLL2M = 32;   // 2 MHz
    RCC_OscInitStruct.PLL2.PLLN = 240; // 480 MHz
#else
    #error ADC clock setup failed, check CLOCK_SOURCE and HSE_VALUE
#endif
    PeriphClkInitStruct.PLL2.PLL2P = 2;
    PeriphClkInitStruct.PLL2.PLL2Q = 2;
 ...

This way clock source will be chosen according "priority" in case of mask in target.json file:

config": {
            "clock_source": {
                "help": "Mask value : USE_PLL_HSE_EXTC | USE_PLL_HSE_XTAL (need HW patch) | USE_PLL_HSI",

But SetSysClock function works little different 1- Try to start with HSE and external clock (MCO from STLink PCB part) 2- If fail try to start with HSE and external xtal 3- If fail start with HSI clock

So, for example, if I set mask to _USE_PLL_HSE_XTAL | USE_PLLHSI system clock setup can initialize MCU internal clock (no XTAL, or HW failure, ets...), but ADC PLL init code wouldn't know about that, and we are at the beginning of the problem - wrong clock source selected.

Does exist any legal mechanism to tell from one driver to another something like competition status (successfully chosen source)? Simple way is not to allow system to try several clock sources. And it is dummy and not comfortable way imho.

[jeromecoutant]

Maybe don't use CLOCK_SOURCE macro ?

    pllsource = (RCC->PLLCKSELR & RCC_PLLCKSELR_PLLSRC);
    switch (pllsource)
    {
      case RCC_PLLSOURCE_HSI:  /* HSI used as PLL clock source */
        xxx
        break;

      case RCC_PLLSOURCE_HSE:  /* HSE used as PLL clock source */
#if HSE_VALUE==8000000 
           xxx
#elif HSE_VALUE==25000000
           xxx
        break;

      default:
           xxx
        break;
      }

[isaev-d]

Hm, maybe HAL_RCC_GetSysClockFreq or HAL_RCC_GetOscConfig would be more convenient?