Better clock selection

[maxgerhardt]

The original GD32 SPL code sadly does a static clock source selection in the system_xxx.c file.

gd32-pio-spl-package/gd32/cmsis/variants/gd32f30x/system_gd32f30x.c

Lines 42 to 55 in d8af9a4

	/* select a system clock by uncommenting the following line */
	/* use IRC8M */
	//#define __SYSTEM_CLOCK_IRC8M (uint32_t)(__IRC8M)
	//#define __SYSTEM_CLOCK_48M_PLL_IRC8M (uint32_t)(48000000)
	//#define __SYSTEM_CLOCK_72M_PLL_IRC8M (uint32_t)(72000000)
	//#define __SYSTEM_CLOCK_108M_PLL_IRC8M (uint32_t)(108000000)
	//#define __SYSTEM_CLOCK_120M_PLL_IRC8M (uint32_t)(120000000)
	/* use HXTAL(XD series CK_HXTAL = 8M, CL series CK_HXTAL = 25M) */
	//#define __SYSTEM_CLOCK_HXTAL (uint32_t)(__HXTAL)
	//#define __SYSTEM_CLOCK_48M_PLL_HXTAL (uint32_t)(48000000)
	//#define __SYSTEM_CLOCK_72M_PLL_HXTAL (uint32_t)(72000000)
	//#define __SYSTEM_CLOCK_108M_PLL_HXTAL (uint32_t)(108000000)
	#define __SYSTEM_CLOCK_120M_PLL_HXTAL (uint32_t)(120000000)

This means that users who want to change which clock their firmware uses need to edit SPL package files. This is not how it's supposed to be, the SPL files are supposed to be constant but externally user-configurable.

A solution must be engineered that allows, e.g. via global macro setting, to influence the clock selection logic of the SPL framework easily.

Idea: global CLOCK_SOURCE macro with numeric values selecting the source? Or better a more dynamic "target frequency" macro, HSE or HSI clock flag, PLL factors macros, ...?

It should also be a choice to not compile in the clock selection code, but have the user provide it via one of the project files (and the linker).

[PetteriAimonen]

As a workaround in the meanwhile, one can define e.g. __SYSTEM_CLOCK_24M_PLL_HXTAL define in platformio.ini build_flags.

The default define __SYSTEM_CLOCK_120M_PLL_HXTAL is last in the selection logic so any other definition will override it.

[maxgerhardt]

Hmm yes this could work.. the builder script in platform-gd32 could then be expanded to accept some board_build.clock_source = <setting name> value and converts it into the necessary define, say -D__SYSTEM_CLOCK_48M_PLL_HXTAL=48000000U.. Will do some experimenting.

[PetteriAimonen]

Looks like the default code in system_gd32xxx.c makes some assumptions about the external crystal frequency. Not perfect, but usable at least.

Also for some reason I had to call SystemCoreClockUpdate() in main() to have correct value in SystemCoreClock variable.

[maxgerhardt]

No, actually this can't be right. The variable has an initial value that should be correct according to its clock settings..

gd32-pio-spl-package/gd32/cmsis/variants/gd32f30x/system_gd32f30x.c

Lines 64 to 96 in d8af9a4

	/* set the system clock frequency and declare the system clock configuration function */
	#ifdef __SYSTEM_CLOCK_IRC8M
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_IRC8M;
	static void system_clock_8m_irc8m(void);
	#elif defined (__SYSTEM_CLOCK_48M_PLL_IRC8M)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_48M_PLL_IRC8M;
	static void system_clock_48m_irc8m(void);
	#elif defined (__SYSTEM_CLOCK_72M_PLL_IRC8M)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_72M_PLL_IRC8M;
	static void system_clock_72m_irc8m(void);
	#elif defined (__SYSTEM_CLOCK_108M_PLL_IRC8M)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_108M_PLL_IRC8M;
	static void system_clock_108m_irc8m(void);
	#elif defined (__SYSTEM_CLOCK_120M_PLL_IRC8M)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_120M_PLL_IRC8M;
	static void system_clock_120m_irc8m(void);
	#elif defined (__SYSTEM_CLOCK_HXTAL)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_HXTAL;
	static void system_clock_hxtal(void);
	#elif defined (__SYSTEM_CLOCK_48M_PLL_HXTAL)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_48M_PLL_HXTAL;
	static void system_clock_48m_hxtal(void);
	#elif defined (__SYSTEM_CLOCK_72M_PLL_HXTAL)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_72M_PLL_HXTAL;
	static void system_clock_72m_hxtal(void);
	#elif defined (__SYSTEM_CLOCK_108M_PLL_HXTAL)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_108M_PLL_HXTAL;
	static void system_clock_108m_hxtal(void);
	#elif defined (__SYSTEM_CLOCK_120M_PLL_HXTAL)
	uint32_t SystemCoreClock = __SYSTEM_CLOCK_120M_PLL_HXTAL;
	static void system_clock_120m_hxtal(void);
	#endif /* __SYSTEM_CLOCK_IRC8M */

something else must going on in your example.

You did define your clock macro __SYSTEM_CLOCK_24M_PLL_HXTAL to a value too, like 24000000U, instead of just defining it without a value (implicit 0), right?

[PetteriAimonen]

Oh, I missed that. Indeed I just defined it to '1' for enable.