Skip to content

Conversation

@feilongfl
Copy link
Contributor

  1. add pinconfig yaml for gd32e103xx
  2. gen files by gd32pinctrl.py script.

I noticed that the serial name of Gigadevice has changed unusually a few months ago.
They removed the CAN-FD module of the gd32e103 chip,
and the old gd32e103 with CAN-FD was renamed as gd32c103.
This PR for the new gd32e103 only.

@feilongfl feilongfl changed the title add gd32e103xx pin config [wip]add gd32e103xx pin config Nov 24, 2021
Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @feilongfl ,
Thank you for your PR. Please, link this with your board at Zephyr main repo.
We need build a board there using this newer changes to accept PR.

@nandojve nandojve added the DNM Do Not Merge label Nov 30, 2021
@feilongfl
Copy link
Contributor Author

Hi @feilongfl , Thank you for your PR. Please, link this with your board at Zephyr main repo. We need build a board there using this newer changes to accept PR.

Sorry for waste your guys time to review this in-process PR.
When I use this one to compile soc, I found I miss something must be done here. I'll push this PR when I fix error.

@nandojve
Copy link
Member

nandojve commented Dec 1, 2021

Hi @feilongfl , Thank you for your PR. Please, link this with your board at Zephyr main repo. We need build a board there using this newer changes to accept PR.

Sorry for waste your guys time to review this in-process PR. When I use this one to compile soc, I found I miss something must be done here. I'll push this PR when I fix error.

Don't worry about it @feilongfl everyone is learning here.
Please, update your PRs at Zephyr main repo with missing info to be possible move forward.

You can look zephyrproject-rtos/zephyr#40283 and use it as guidance.

Tips:

  • No need to add wip, when you open PR, simple select Draft when you create it. This tell us that PR is still in progress. If you need assistance, you can ask and someone will, eventually, help you.
  • After people let you reviews, you can ask to them look again by clicking at circle with arrows, left side people names. That will inform reviewers that you already update everything or need that person check again PR.

@feilongfl feilongfl force-pushed the main branch 2 times, most recently from 8e80fac to 4c671fc Compare December 5, 2021 02:37
@feilongfl feilongfl changed the title [wip]add gd32e103xx pin config add gd32e103xx pin config Dec 5, 2021
@feilongfl
Copy link
Contributor Author

Hi @nandojve @gmarull
I've update this PR, and tested sample/hello_world with zephyrproject-rtos/zephyr#36833

  • resolved problem which raised by @gmarull
  • add gd32e10x_libopt.h from GD32E10x_Firmware_Library_V1.2.0/Examples/GPIO/Running_led/gd32e10x_libopt.h
  • Some patches in HAL
    • force use external 8M oscillator
      I think we will use value from devicetree later, so there only fix for gd32e103v_eval board.
    • remove nvic config
      I think nvic is config by zephyr, don't need to config here.
    • enable MPU flag.
      I found that all of header file in hal_gigadevice module is include only when MPU is enable.
      When the MPU is not available, I don't know how to include the files.
      so I think temporary enable __MPU_PRESENT marco is a good idea.

Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @feilongfl ,

Nice work!

I think you only need to drop all CAN related at gd32e103.

Comment on lines 52 to 56
#if !defined HXTAL_VALUE
#ifdef GD32E103V_EVAL
#ifdef CONFIG_BOARD_GD32E103V_EVAL
#define HXTAL_VALUE ((uint32_t)8000000) /*!< value of the external oscillator in Hz */
#define HXTAL_VALUE_8M HXTAL_VALUE
#elif defined(GD32E103R_START)
#elif defined(CONFIG_BOARD_GD32E103R_START)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HAL headers can't depend on Kconfig variables. Instead, Zephyr build system should insert the HXTAL_VALUE based on a Kconfig or DT value. See zephyrproject-rtos/zephyr@a939085

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use HXTAL_VALUE, but I'm a bit unsure if it's the same as your idea.

@feilongfl feilongfl force-pushed the main branch 2 times, most recently from 8d07746 to 41d9fbd Compare December 11, 2021 00:09
Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop hal:gd32e10x: temporary enable MPU flag commit. You need remove MPU at dts and soc definition.
Drop standard_peripheral/include/gd32e10x_fmc.c file.

@feilongfl
Copy link
Contributor Author

Drop hal:gd32e10x: temporary enable MPU flag commit. You need remove MPU at dts and soc definition.

If I Drop this commit, I'll get a compile error that header file in hal_gigadevice is not included.

I try to analyse why these file not included, I write them here.
#12 (comment)

Currently I have found no way to solve this problem other than turning on MPU.

@nandojve
Copy link
Member

Please, check zephyrproject-rtos/zephyr#36833 (review) and let me know result.

@feilongfl
Copy link
Contributor Author

Please, check zephyrproject-rtos/zephyr#36833 (review) and let me know result.

You're right, I forget that I can also modify the driver code.

Comment on lines 60 to 68
/* get external oscillator from KCONFIG */
#define HXTAL_VALUE CONFIG_GD32_HXTAL_VALUE
#if HXTAL_VALUE == 8000000
#define HXTAL_VALUE_8M HXTAL_VALUE
#elif HXTAL_VALUE == 25000000
#define HXTAL_VALUE_25M HXTAL_VALUE
#else
#error "GD32E10X lib only support 8M and 25M oscillator (HXTAL)"
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to do that, just drop this commit and do the same as zephyrproject-rtos/zephyr@a939085 (cherry-pick it in your PR while it's not merged, even though it's now approved so it'll go in soon)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh,I see HXTAL_VALUE was defined in cmake script, I'll remove it.
Because of code in https://github.com/zephyrproject-rtos/hal_gigadevice/blob/main/gd32e10x/cmsis/gd/gd32e10x/source/system_gd32e10x.c#L745-L751 ,
I need HXTAL_VALUE_8M or HXTAL_VALUE_25M marco to make clock config success.

@feilongfl feilongfl force-pushed the main branch 3 times, most recently from 4167d62 to d36cb81 Compare December 15, 2021 10:54
Add pin configurations for the GD32E103XX SoCs.

Signed-off-by: YuLong Yao <[email protected]>
Add remap definitions for GD32E103XX SoCs.

Signed-off-by: YuLong Yao <[email protected]>
Files autogenerated using the gd32pinctrl.py script.

Signed-off-by: YuLong Yao <[email protected]>
file from `GD32E10x_Demo_Suites_V1.2.0`


Signed-off-by: YuLong Yao <[email protected]>
drop `nvic_vector_table_set`call in `SystemInit` function.

Signed-off-by: YuLong Yao <[email protected]>
drop standard_peripheral/include/gd32e10x_fmc.c
add HXTAL_VALUE_8M and HXTAL_VALUE_25M marco by HXTAL_VALUE.


Signed-off-by: YuLong Yao <[email protected]>
@gmarull gmarull requested a review from nandojve February 21, 2022 08:55
@nandojve nandojve merged commit cc85acb into zephyrproject-rtos:main Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DNM Do Not Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants