STM32L4: PLL MSI and LSE Configuration Question

[mattbrown015]

Description

I've been asked to check the operation of the 32 kHz LSE crystal on our board. What we've done is similar/identical to the NUCLEO_L433RC_P.

I believe our LSE is functioning correctly but I'm trying to understand if the device configuration would fail if it wasn't.

SetSysClock_PLL_MSI in targets\TARGET_STM\TARGET_STM32L4\TARGET_STM32L433xC\TARGET_NUCLEO_L433RC_P\system_clock.c enables the LSE and configures the PLL.

AFAICT, no error will occur if the LSE does not become ready:

// Enable LSE Oscillator to automatically calibrate the MSI clock
RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_LSE;
RCC_OscInitStruct.PLL.PLLState   = RCC_PLL_NONE; // No PLL update
RCC_OscInitStruct.LSEState       = RCC_LSE_ON; // External 32.768 kHz clock on OSC_IN/OSC_OUT
if (HAL_RCC_OscConfig(&RCC_OscInitStruct) == HAL_OK) {
    RCC->CR |= RCC_CR_MSIPLLEN; // Enable MSI PLL-mode
}

I'm wondering why it doesn't fail if HAL_RCC_OscConfig fails as happens with the other calls to HAL_RCC_OscConfig?

Also, I don't understand why RCC->CR |= RCC_CR_MSIPLLEN is effectively repeated with a call to HAL_RCCEx_EnableMSIPLLMode after the configuration of the PLL? The first enable is dependant on the successful configuration of LSE and the second is unconditional.

Issue request type

[X] Question
[ ] Enhancement
[ ] Bug

[0xc0170]

@ARMmbed/team-st-mcd Please review

[ciarmcom]

ARM Internal Ref: MBOTRIAGE-1475

[bcostm]

no error will occur if the LSE does not become ready

I suppose this has been done on purpose? If the LSE does not start then we continue the MSI configuration but without calibration. This was maybe the idea behind this.

But I agree that in case the LSE does not start then the call to HAL_RCCEx_EnableMSIPLLMode();must be avoided.

We should add maybe a flag when the LSE has started correctly and use this flag for the rest of the code.

Or simply do this:

if (HAL_RCC_OscConfig(&RCC_OscInitStruct) == HAL_OK) {
    /* Enable MSI Auto-calibration through LSE */
    HAL_RCCEx_EnableMSIPLLMode();
}

and remove the second call to HAL_RCCEx_EnableMSIPLLMode();

cc @helmut64 what do you think ? (as this change is impacting also your own board)

[helmut64]

I don't have the complete overview on this, at present we use the LSE crystal on our board.

[mattbrown015]

I suppose this has been done on purpose? If the LSE does not start then we continue the MSI configuration but without calibration. This was maybe the idea behind this.

@bcostm Possibly, this sounds like a good idea for a prototyping environment that is trying to get people started quickly and easily. But, I'm concerned/interested because we've moved beyond prototyping. We're about to build a number of production boards and I think it would be bad if some boards silently behaved differently to others because of a manufacturing fault i.e. some problem with the crystal.

As it happens though, this won't be the case for me partly because I've added my own test of LSERDY and partly because rtc_init fails if the LSE initialisation fails. (Which I think is inconsistent with SetSysClock_PLL_MSI.)

at present we use the LSE crystal on our board

@helmut64 As do we, but I removed the crystal off 'just to see what happens'. I did this mainly because we're thinking about what could possible go wrong with our boards and how to test to them.

@bcostm's proposed change is what I had in mind but I wondered if there were any subtle ordering requirements in the clock configuration. In other words, should RCC_CR_MSIPLLEN be selected before or after enabling the MSI (or doesn't it matter). It would be easy enough for me to try but I've got something more important to do this morning.

[jeromecoutant]

Hi

Can you review this commit : 29c527d

Thx

[mattbrown015]

Hi @jeromecoutant,

29c527d looks good to me. Although I wouldn't claim to have any expertise. :-}

I tried it on our custom board, with the LSE connected, and it seemed to continue working as before. I haven't tried disconnecting the LSE.

Are you going to apply this change to all STM32L4 targets? Our custom board is actually based on NUCLEO_L432KC so I changed TARGET_STM32L432xC/TARGET_NUCLEO_L432KC/system_clock.c.

While testing your change I noticed something else. Do you think line 151 is meant to say MBED_ASSERT(0) rather than MBED_ASSERT(1), which never asserts?

[jeromecoutant]

Hi

Are you going to apply this change to all STM32L4 targets?

Yes

MBED_ASSERT(0) rather than MBED_ASSERT(1), which never asserts?

Good finding! I will make a patch as well!

[bcostm]

Can you please close this issue if everything is ok now ? Thanks.

[mattbrown015]

I suppose my question was answered by the creation of the PR. At the time of writing the PR hasn't merged but that's a different issue. Hence closing.