GD32F130xxxx clock support

[robcazzaro]

Hello, I'm working on the SimpleFOC port together with @Candas1 (#95)

The split boards used in many hoverboards use a GD32F130 (C8T6, C6T6 or K6), running at 48MHz and without a crystal oscillator. The SPL files in this repository don't seem to have the proper initialization code for that clock, e.g.

ArduinoCore-GD32/system/GD32F1x0_firmware/CMSIS/GD/GD32F1x0/Source/system_gd32f1x0.c

Line 49 in 2cb8305

#define __SYSTEM_CLOCK_72M_PLL_IRC8M_DIV2 (uint32_t)(72000000)

I downloaded the latest SPL for the GD32F1x0 processors, version 3.3.4, and the system_gd32f1x0.c file is different and includes the proper #defines and code to initialize the clock at 48MHz using the internal oscillator

/* system frequency define */
#define __IRC8M           (IRC8M_VALUE)            /* internal 8 MHz RC oscillator frequency */
#define __HXTAL           (HXTAL_VALUE)            /* high speed crystal oscillator frequency */
#define __SYS_OSC_CLK     (__IRC8M)                /* main oscillator frequency */

#define VECT_TAB_OFFSET  (uint32_t)0x00            /* vector table base offset */

/* select a system clock by uncommenting the following line */
//#define __SYSTEM_CLOCK_8M_HXTAL              (__HXTAL)
//#define __SYSTEM_CLOCK_8M_IRC8M              (__IRC8M)
//#define __SYSTEM_CLOCK_48M_PLL_HXTAL         (uint32_t)(48000000)
//#define __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2    (uint32_t)(48000000)
#define __SYSTEM_CLOCK_72M_PLL_HXTAL         (uint32_t)(72000000)
//#define __SYSTEM_CLOCK_72M_PLL_IRC8M_DIV2    (uint32_t)(72000000)

Do you plan to update the SPL? if not, what is the best way to use a custom system_gd32f1x0.c file?

Latest SPL for GD32F1x0 is here https://www.gd32mcu.com/download/down/document_id/288/path_type/1

[maxgerhardt]

Sure, we can update the SPL. The way these macros are structured in the code, the first activated macro is effect. So in the above example, let's say

//#define __SYSTEM_CLOCK_8M_HXTAL              (__HXTAL)
//#define __SYSTEM_CLOCK_8M_IRC8M              (__IRC8M)
//#define __SYSTEM_CLOCK_48M_PLL_HXTAL         (uint32_t)(48000000)
#define __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2    (uint32_t)(48000000)
#define __SYSTEM_CLOCK_72M_PLL_HXTAL         (uint32_t)(72000000)
//#define __SYSTEM_CLOCK_72M_PLL_IRC8M_DIV2    (uint32_t)(72000000)

Then 48M_PLL_IRC8M_DIV2 will actually take effect. That was already discovered in some other issue. Meaning that, after the update, you should be able to use

build_flags = __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2=48000000

in the platformio.ini to override the clock selection to the wanted one, even if we have the default of 72MHz HXTAL again.

Note that getting easier better clock selection is tracked in #19.

[robcazzaro]

Thanks for the super-fast reply!

The issue is much more than a macro, though. If you follow the new code (attached below as a ZIP), you will see that if that #define in turns executes the following code, which is not present in the SPL in your repository

#elif defined (__SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2)
uint32_t SystemCoreClock = __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2;
static void system_clock_48m_irc8m(void);

and

#elif defined (__SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2)
/*!
    \brief      configure the system clock to 72M by PLL which selects IRC8M/2 as its clock source
    \param[in]  none
    \param[out] none
    \retval     none
*/
static void system_clock_48m_irc8m(void)
{
    /* AHB = SYSCLK */
    RCU_CFG0 |= RCU_AHB_CKSYS_DIV1;
    /* APB2 = AHB */
    RCU_CFG0 |= RCU_APB2_CKAHB_DIV1;
    /* APB1 = AHB */
    RCU_CFG0 |= RCU_APB1_CKAHB_DIV1;
    /* PLL = (IRC8M/2) * 12 = 48 MHz */
    RCU_CFG0 &= ~(RCU_CFG0_PLLSEL | RCU_CFG0_PLLMF);
    RCU_CFG0 |= (RCU_PLLSRC_IRC8M_DIV2 | RCU_PLL_MUL12);
    
    /* enable PLL */
    RCU_CTL0 |= RCU_CTL0_PLLEN;

    /* wait until PLL is stable */
    while(0 == (RCU_CTL0 & RCU_CTL0_PLLSTB));

    /* select PLL as system clock */
    RCU_CFG0 &= ~RCU_CFG0_SCS;
    RCU_CFG0 |= RCU_CKSYSSRC_PLL;

    /* wait until PLL is selected as system clock */
    while(RCU_SCSS_PLL != (RCU_CFG0 & RCU_CFG0_SCSS)){
    }
}

system_gd32f1x0.zip

[maxgerhardt]

Yeah the above with "previous definition ovverides last one" is actually wrong here due to the way it's structured differently than what I was working on previously (CommunityGD32Cores/gd32-pio-spl-package#4).

The SPL is updated now in 86e0ac7.

The default clock source is unmodified with 72MHz PLL from HXTAL.

However, I added a __PIO_DONT_SET_CLOCK_SOURCE__ option that disables this default option. So, you can do

build_flags = 
  -D __PIO_DONT_SET_CLOCK_SOURCE__
  -D __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2=48000000

in the platformio.ini and that'll set the clock source to 48MHz from internal RC-oscillator and PLL-

I've tested that to be working on my GD32F130C8.

To update, use a PlatformIO core CLI and execute

pio pkg update -g -p gd32

Per

[robcazzaro]

Oh, wow, this is great!

I really like the PIO_DONT_SET_CLOCK_SOURCE override, thanks again for the amazing turnaround and addressing this.

Should I close the issue or will you? Not sure about your project preferences...

[maxgerhardt]

Have you tested this to be working already on your board?

Well something is still wrong.. Updating the F1x0 SPL to the newest version..

..killed all CAN SPL code? F170 and F190 series chips have "CAN" written in their datasheet in the versions I'm looking at. What in the world is going there, I need to investigate this for a moment.

[maxgerhardt]

ARE THEY SERIOUS? They just deleted the datasheets for F170 and F190 that we have a backup copy of..

https://github.com/CommunityGD32Cores/gigadevice-firmware-and-docs/tree/main/GD32F1x0

Okay, so I guess these chips are not made anymore, noone ever bought them, and they just kill support for it, no backwards compatibility? Crazy, I'll have to write an email about this to Gigadevice.

In any case, the underlying problem in this issue should be solved per above. I'll wait for you to test on real hardware to confirm the workings.

[Candas1]

I think we were also confused about inserted adc desappearing from some of the documents but still being in some of the spl examples. Maybe it's a mistake?

[robcazzaro]

What @Candas1 is referring to, is that in previous GD32F130 reference manuals there was documentation about the inserted ADC functionality (same as the STM32F103 injected mode), but it has been removed completely in newer doc revisions. It's still present in the SPL, but the latest SPL is much older than the latest reference manual. My guess is that GigaDevice discovered issues with the inserted ADC functionality and removed support for it, but didn't update the SPL yet. Or didn't want to guarantee functionality, but it's still in the SPL for people who used it

@maxgerhardt if you have a contact in GigaDevice, would be good to ask them about the inserted ADC functionality (registers below have been removed from the documentation V 3.6 https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20230209/GD32F1x0_User_Manual_EN_Rev3.6.pdf)

#define ADC_ISQ REG32(ADC + 0x00000038U) /*!< ADC inserted sequence register /
#define ADC_IDATA0 REG32(ADC + 0x0000003CU) /!< ADC inserted data register 0 /
#define ADC_IDATA1 REG32(ADC + 0x00000040U) /!< ADC inserted data register 1 /
#define ADC_IDATA2 REG32(ADC + 0x00000044U) /!< ADC inserted data register 2 /
#define ADC_IDATA3 REG32(ADC + 0x00000048U) /!< ADC inserted data register 3 /
#define ADC_RDATA REG32(ADC + 0x0000004CU) /!< ADC regular data register */

In the new SPL readme, it's interesting to see the supported chip versions

\version 2014-12-26, V1.0.0, platform GD32F1x0(x=3,5)
\version 2016-01-15, V2.0.0, platform GD32F1x0(x=3,5,7,9)
\version 2016-04-30, V3.0.0, firmware update for GD32F1x0(x=3,5,7,9)
\version 2017-06-19, V3.1.0, firmware update for GD32F1x0(x=3,5,7,9)
\version 2019-11-20, V3.2.0, firmware update for GD32F1x0(x=3,5,7,9)
\version 2020-09-21, V3.3.0, firmware update for GD32F1x0(x=3,5,7,9)
\version 2022-08-15, V3.4.0, firmware update for GD32F1x0(x=3,5)

7 and 9 appeared in 2016, deprecated in the SPL in 2022

[maxgerhardt]

I didn't immediately find a technical support address but [email protected] seems to be valid, I'm waiting for a response there.

[maxgerhardt]

Dear Maximilian,

Thank you for your interest in GigaDevice GD32 MCU’s. The GD32F170 and GD32F190 series are old legacy products and currently not in production.

Please feel free to contact me if you should have any further questions.

Best Regards, Reuben

Wow.

[Candas1]

Hi,

My code is running only if I add this in platformio.ini now

build_flags = 
  -D __PIO_DONT_SET_CLOCK_SOURCE__
  -D __SYSTEM_CLOCK_48M_PLL_IRC8M_DIV2=48000000

Is it the expected behavior ?
Could this belong to a custom variant.h file later ?
Is there anything I should test to close this issue ?

Thanks in advance.

[maxgerhardt]

@Candas1 so it was running fine before the update (which was using external crystal + PLL) but now it won't run in the default settings?

[Candas1]

@Candas1 so it was running fine before the update (which was using external crystal + PLL) but now it won't run in the default settings?

Yes. So now if I run the same program while commenting the build flags, the program is running slower, I am receiving data properly, but the firmware on the other side doesn't like the data I am sending. (SystemCoreClock = 72000000)
If I uncomment the build flags, it's running fine.(SystemCoreClock = 48000000)
And it was running fine before.

[maxgerhardt]

Woops, in the previous version, the clock was actually set from 72MHz from INTERNAL RC + PLL.

ArduinoCore-GD32/system/GD32F1x0_firmware/CMSIS/GD/GD32F1x0/Source/system_gd32f1x0.c

Lines 45 to 49 in b03bd51

	/* select a system clock by uncommenting the following line */
	//#define __SYSTEM_CLOCK_8M_HXTAL (__HXTAL)
	//#define __SYSTEM_CLOCK_8M_IRC8M (__IRC8M)
	//#define __SYSTEM_CLOCK_72M_PLL_HXTAL (uint32_t)(72000000)
	#define __SYSTEM_CLOCK_72M_PLL_IRC8M_DIV2 (uint32_t)(72000000)

Now the default (no build_flags set) is 72MHz from external crystal. Meaning if your board doesn't have one, the startup of the clock will fail and it'll likely keep its reset clock of 8MHz.

Can you quickly manually go into the file (in PlatformIO: C:\Users\<user>\.platformio\packages\framework-arduinogd32\system\GD32F1x0_firmware\CMSIS\GD\GD32F1x0\Source\system_gd32f1x0.c and comment+uncomment stuff so that it does 72 MHz from IRC8M again?

We will fix this shortly.

[maxgerhardt]

Or rather, if you don't want to hack around in files, it's commited in 8465b99 now. The defaults should work for internal clock + 72MHz now.

[Candas1]

Oh wow so fast.
Yes now it works even without the build flags.

[maxgerhardt]

Great, then with the new default being the old default and the possibility of changing the clock source arbitrarily per build flags, I'm considering this done :)

[Candas1]

Thanks a lot