How to change clock speed to 168MHz with direct register manipulation?

[JCorl.1]

I am following a Udemy course and we are learning to make a driver from scratch. I know, not practical, but it has taught me how it can be done and helped learn how to read the datasheet better. Anyway, I have written the below code to write to the RCC and FLASH registers to configure the clock. Though how can I test this? I tried to toggle an output pin on and off and it seemed terribly slow, around 1.4MHz. I am manipulating the register for the output directly as well. Any hints for this 32 bit beginner?

https://pastebin.com/9NkF34Hh

https://pastebin.com/QenbnU12

[waclawek.jan]

You can insert code here, use the </> icon on the bottom of the edit window:

void DkoSetClock168()
{
 // HSE = 8MHz
 // PLL_M = 8 *
 // PLL_N = 336 *
 // PLL_P = 2 *
 // PLL_Q = 7 *
 // PLLI2S_N = 192----
 // PLLI2S_R = 2------
 // AHB_PRE = 1 *
 // APB1_PRE = 4
 // APB2_PRE = 2
 // CORTEX PRE = 1
 // MAKES 168MHz
 // WAIT STATE MAX
 
 // PLL Configuration
 // * PLL-M: 8 *
 RCC->PLLCFGR &= (uint32_t)0xFFFFFFC0UL;
 RCC->PLLCFGR |= (uint32_t)0x00000008UL;
 
 // * PLL-N: 336 *
 RCC->PLLCFGR &= (uint32_t)0xFFFF803FUL;
 RCC->PLLCFGR |= (uint32_t)0x00001500UL;
 
 // * PLL-P: 2 *
 RCC->PLLCFGR &= (uint32_t)0xFFFCFFFFUL;
 RCC->PLLCFGR |= (uint32_t)0x00020000UL;
 
 // * PLL-Q: 7 *
 RCC->PLLCFGR &= (uint32_t)0xF0FFFFFFUL;
 RCC->PLLCFGR |= (uint32_t)0x07000000UL;
 
 // * Activate external clock to be 8 MHz) *
 RCC->CR |= (uint32_t)(1 << 16);
 
 // * Wait until HSE is ready *
 while ((RCC->CR & (uint32_t)(1 << 17)) == 0);
 
 // * Select HSE as PLL source *
 RCC->PLLCFGR |= (uint32_t)(1 << 22);
 
 // Enable PLL *
 RCC->CR |= (uint32_t)(1 << 24);
 
 // * Wait until PLL is ready *
 while ((RCC->CR & (1 << 25)) == 0);
 
 // * 7 wait state *
 FLASH->ACR &= (uint32_t)0xFFFFFFF8UL;
 FLASH->ACR |= (uint32_t)0x00000007UL;
 
 // * Enable pre-fetch buffer *
 FLASH->ACR |= (uint32_t)(1 << 8);
 
 // * Switching to PLL clock source
 RCC->CFGR &= (uint32_t)0xFFFFFFFCUL;
 RCC->CFGR |= (uint32_t)(1 << 1);
 
 // * Wait for PLL to be active clock source
 while ((RCC->CFGR & (1 << 3)) == 0);
 
 // Peripheral clock setup
 // AHB pre-scaler
 RCC->CFGR &= (uint32_t)0xFFFFFF0FUL;
 
 // APB1 pre-scaler
 RCC->CFGR &= (uint32_t)0xFFFFE3FFUL;
 RCC->CFGR |= (uint32_t)0x00001C00UL;
 
 // APB2 pre-scaler
 RCC->CFGR &= (uint32_t)0xFFFF1FFFUL;
 RCC->CFGR |= (uint32_t)0x00008000UL;
 
 // ADC and USB not needed on STM32F407?
}
 
[...]
 while(1)
 {
 GPIOC->ODR |= (1 << 8);
 GPIOC->ODR &= ~(1 << 8);
 GPIOC->ODR |= (1 << 8);
 GPIOC->ODR &= ~(1 << 8);
 GPIOC->ODR |= (1 << 8);
 GPIOC->ODR &= ~(1 << 8);
 }

Remarks:

1)

  RCC->PLLCFGR &= (uint32_t)0xFFFFFFC0UL;

  RCC->PLLCFGR |= (uint32_t)0x00000008UL;

Don't use multiple modifications to a register, and avoid RMW (read-modify-write) wherever possible.

The first is because you introduce intermediate values -  RCC->PLLCFGR may have an unintended value between those two lines; the second may have unwanted side effects at some kind of registers (where both hardware and software modifies the registers bits) - while this is not the case here, it's a bad practice nonetheless. And, both these are unnecessary, result in longer, slower code.

The whole sequence of setting PLLCFG can be replaced by one single write:

 RCC->PLLCFGR = ( HSE_CLOCK_MHZ / 2 * RCC_PLLCFGR_PLLM_0) // M = XTAL divider - the PLL needs 1MHz to 2MHz at its input, 2MHz reduces jitter
 OR ( /* 360 */ (2 * PLL_CLOCK_MHZ) / 2 * RCC_PLLCFGR_PLLN_0) // N = PLL multiplier (feedback divider) - the VCO's output needs to fall between 64MHz and 432MHz
 OR (( 2 / 2 - 1) * RCC_PLLCFGR_PLLP_0) // P = divider for SYSCLK - 0 = /2, 1 = /4, 2 = /6, 3 = /8
 OR ( 8 * RCC_PLLCFGR_PLLQ_0) // Q = divider for USB (and SDIO and RNG) -- can't achieve 48MHz with 180MHz system clock anyway, we would need to go down to 168MHz (which wouldn't need overdrive btw.), but we don't use USB here anyway
 OR (RCC_PLLCFGR_PLLSRC_HSE); // PLL source is HSE

(there are some macros used there I am not going to look up now, one notable but straigforward is #define OR | )

2)

// * PLL-N: 336 *  

RCC->PLLCFGR &= (uint32_t)0xFFFF803FUL;

  RCC->PLLCFGR |= (uint32_t)0x00001500UL;

This does not set N to 336 but to 84. Do your math again. Don't calculate manually where you can write a constant expression.

3)

   // * PLL-P: 2 *

   RCC->PLLCFGR &= (uint32_t)0xFFFCFFFFUL;

   RCC->PLLCFGR |= (uint32_t)0x00020000UL;

This does not set PLLP to 2 but to 6. Surprised? Read the manual again.

4)

Change the AHB/APB dividers *before* you change the clock. If you change them *after* as you do now, they run for a brief period of time beyond their designed speed which may have unwanted effects.

Again, this register can be written in one single write. And again, check the values you are writing against the manual, the values are not equal to the division rates.

5)

    GPIOC->ODR |= (1 << 8);

    GPIOC->ODR &= ~(1 << 8);

Don't use ODR, use BSRR. It does not matter in a trivial program such as this one, but in more complex programs you want as much atomic access as possible.

JW

[Tesla DeLorean]

You can export the internal clock via the MCO (PA8) and inspect it directly

[Piranha]

> ...learning to make a driver from scratch. I know, not practical...

Actually the exact opposite is true - for MCUs the manufacturer provided driver libraries are an absolute junk and writing your own driver libraries from scratch is the only way to get reliable drivers.

[JCorl.1]

Thanks everyone for their input! After looking through this, researching elsewhere, and after more tinkering, I finally got it working. Even though it was valuable learning how to make my own drivers from scratch (I understand way better now what other drivers do now), I decided I was comfortable enough using the CMSIS header definitions. Though I am still hesitant to use the ST HAL as what I am doing greatly benefits from direct register manipulation (emulating game controller protocols). Below is my final code for the STM32F407VG, to run at 180MHz, using 8MHz external crystal, so that someone else may benefit in the future.

static void configure_clock()
{
	/* Clock Settings for 168MHz
	 HCLK = 168
	 PLL: M = 8, N = 336, P = 2, Q = 7
	 AHB prescaler = 1
	 APB prescaler1 = 4, APB prescaler2 = 2
	 MCO1 prescaler = 2 */
 
	// Configures flash latency.
	// LATENCY: bits 2-0
	MODIFY_REG(FLASH->ACR,
		FLASH_ACR_LATENCY,
		_VAL2FLD(FLASH_ACR_LATENCY,
		FLASH_ACR_LATENCY_5WS) //FLASH_ACR_LATENCY_5WS << FLASH_ACR_LATENCY_Pos
	);
 
	// Enables HSE.
	// HSE_ON: bit 16
	SET_BIT(RCC->CR, RCC_CR_HSEON);
 
	// Waits until HSE is stable.
	// HSERDY: bit 17
	while (!READ_BIT(RCC->CR, RCC_CR_HSERDY));
 
	// Configures PLL: source = HSE, PLLCLK = 168MHz.
	// M: bits 5-0, N: bits 14-6, P: bits 17-16, PLLSRC: bit 22, Q: bits 27-24
	MODIFY_REG(RCC->PLLCFGR,RCC_PLLCFGR_PLLM | RCC_PLLCFGR_PLLN | RCC_PLLCFGR_PLLQ | RCC_PLLCFGR_PLLSRC
			| RCC_PLLCFGR_PLLP,_VAL2FLD(RCC_PLLCFGR_PLLM, 8) | _VAL2FLD(RCC_PLLCFGR_PLLN, 336)
			| _VAL2FLD(RCC_PLLCFGR_PLLQ, 7) | RCC_PLLCFGR_PLLSRC_HSE);
 
	// Enables PLL module.
	// PLLON: bit 24
	SET_BIT(RCC->CR, RCC_CR_PLLON);
 
	// Waits until PLL is stable.
	// PLLRDY: bit 25
	while (!READ_BIT(RCC->CR, RCC_CR_PLLRDY));
 
	// Switches system clock to PLL.
	// SW: bits 1-0
	MODIFY_REG(RCC->CFGR, RCC_CFGR_SW, _VAL2FLD(RCC_CFGR_SW, RCC_CFGR_SW_PLL));
 
	// Configures PPRE1 = 4
	MODIFY_REG(RCC->CFGR, RCC_CFGR_PPRE1, _VAL2FLD(RCC_CFGR_PPRE1, 5));
 
	// Configures PPRE2 = 2
	MODIFY_REG(RCC->CFGR, RCC_CFGR_PPRE1, _VAL2FLD(RCC_CFGR_PPRE1, 4));
 
	// Configures HPRE = 1
	MODIFY_REG(RCC->CFGR, RCC_CFGR_PPRE1, _VAL2FLD(RCC_CFGR_PPRE1, 1));
 
	// Waits until PLL is used.
	while(READ_BIT(RCC->CFGR, RCC_CFGR_SWS) != RCC_CFGR_SWS_PLL);
 
	// Disables HSI.
	CLEAR_BIT(RCC->CR, RCC_CR_HSION);
}
 
void configure_mco1()
{
	// Configures MCO1: source = PLLCLK, MCO1PRE = 2.
	MODIFY_REG(RCC->CFGR,
		RCC_CFGR_MCO1 | RCC_CFGR_MCO1PRE,
		_VAL2FLD(RCC_CFGR_MCO1, 3) | _VAL2FLD(RCC_CFGR_MCO1PRE, 7)
	);
 
	// Enables GPIOA (MCO1 is connected to PA8).
	SET_BIT(RCC->AHB1ENR, RCC_AHB1ENR_GPIOAEN);
 
	// Configures PA8 as medium speed.
	MODIFY_REG(GPIOA->OSPEEDR,
		GPIO_OSPEEDR_OSPEED8,
		_VAL2FLD(GPIO_OSPEEDR_OSPEED8, 1)
	);
 
	// Configures PA8 to work in alternate function mode.
	MODIFY_REG(GPIOA->MODER,
		GPIO_MODER_MODER8,
		_VAL2FLD(GPIO_MODER_MODER8, 2)
	);
}

[Piranha]

Using CMSIS headers is perfectly fine and recommended. Those SET and CLEAR macros are kind of useless, but MODIFY and some more advanced ones are nice. Anyway, the next level is to write some higher level software components, like USART or some other universal driver. :)

[waclawek.jan]

Looks OK. Style is an individual preference.

I would still recommend moving the switch of system clock to PLL after changing the buses prescalers.

> //FLASH_ACR_LATENCY_5WS << FLASH_ACR_LATENCY_Pos

Yes, the CMSIS headers are far from being perfect and consistent. ST doesn't care, it's all Cube for them.

ST doesn't care about serious examples either, that's why you have to reinvent them yourself.

JW