Skip to content

dts: wch: Enable using whole flash with CH32V208 And reorganize soc folder for future deduplication #87345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

dts: wch: Enable using whole flash with CH32V208 And reorganize soc folder for future deduplication #87345

wants to merge 2 commits into from
+29 −22

Conversation

VynDragon
Copy link
Collaborator

@VynDragon VynDragon commented Mar 19, 2025 •
edited
Loading

Enables using the whole flash on CH32V208
This needs #87141 This fixes it differently.
and #85395

@zephyrbot zephyrbot added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Mar 19, 2025
@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from 1d88336 to d4880f1 Compare March 19, 2025 12:57
@VynDragon VynDragon mentioned this pull request Mar 19, 2025
Open
@VynDragon VynDragon mentioned this pull request Mar 20, 2025
Closed
@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from d4880f1 to cbb1ae2 Compare March 20, 2025 13:34
fkokosinski
fkokosinski previously approved these changes Mar 21, 2025
View reviewed changes
@kartben kartben requested a review from fabiobaltieri March 21, 2025 20:40
@fabiobaltieri
Copy link
Member

cc @kholia (need to open a issue to add you to the collaborators)

@zephyrbot zephyrbot added the area: Timer Timer label Mar 24, 2025
@zephyrbot zephyrbot requested review from andyross and teburd March 24, 2025 19:35
@VynDragon VynDragon requested a review from fkokosinski March 24, 2025 19:37
@VynDragon
Copy link
Collaborator Author

@fkokosinski @miggazElquez I added the reorganization commit, please check and advise

@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from 9b716f6 to 734dc55 Compare March 24, 2025 19:44
miggazElquez
miggazElquez approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@miggazElquez miggazElquez left a comment

Choose a reason for hiding this comment

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

It looks good to me, I have rebased my code for the 303 on top of this and everything works.

modules/hal_wch/CMakeLists.txt Outdated
@@ -1,3 +1,3 @@
if(CONFIG_SOC_CH32V003 OR CONFIG_SOC_SERIES_QINGKE_V4C)
if(CONFIG_SOC_CH32V003 OR CONFIG_SOC_SERIES_QINGKE_V4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to put just CONFIG_SOC_FAMILY_CH32V ? I don't see why a ch32v soc wouldn't use the HAL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch 2 times, most recently from 4fbd942 to 19ee357 Compare March 25, 2025 14:55
fkokosinski
fkokosinski previously approved these changes Mar 26, 2025
View reviewed changes
fabiobaltieri
fabiobaltieri previously approved these changes Mar 26, 2025
View reviewed changes
@VynDragon VynDragon mentioned this pull request Apr 7, 2025
Open
@miggazElquez
Copy link
Contributor

@nzmichaelh As you are the assignee this can't be merged without your approval.

@VynDragon VynDragon mentioned this pull request Apr 25, 2025
Open
@VynDragon VynDragon dismissed stale reviews from fabiobaltieri and fkokosinski via ee20e07 April 25, 2025 13:26
@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from 19ee357 to ee20e07 Compare April 25, 2025 13:26
@VynDragon
Copy link
Collaborator Author

@nzmichaelh please take a look.

@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from ee20e07 to 015859c Compare April 25, 2025 13:28
@VynDragon
Copy link
Collaborator Author

Rebased on main due to CI fail.

nzmichaelh
nzmichaelh reviewed Apr 26, 2025
View reviewed changes
drivers/timer/Kconfig.wch_ch32v00x Outdated
@@ -3,6 +3,6 @@

config CH32V00X_SYSTICK
bool "CH32V00X systick timer"
depends on SOC_CH32V003 || SOC_SERIES_QINGKE_V4C
depends on SOC_CH32V003 || SOC_SERIES_QINGKE_V4
Copy link
Collaborator

Choose a reason for hiding this comment

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

No action required: I agree with the direction here, as the systick is part of the QingKe CPU

soc/wch/ch32v/qingke_v4/Kconfig.defconfig
@@ -1,7 +1,7 @@
# Copyright (c) 2025 MASSDRIVER EI (massdriver.space)
Copy link
Collaborator

@nzmichaelh nzmichaelh Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

TL;DR: I think we should rename this to ch32v20x_30x as microarchitecture != series, but doing this in a follow up PR is fine.

Long story: I've been thinking about the same thing at https://github.com/nzmichaelh/zephyr/tree/ch32v006

The issue is that QingKe VXY isn't really a series as per https://docs.zephyrproject.org/latest/hardware/porting/soc_porting.html The CH32V003 has a QingKe V2A, certain GPIO features, and AFIO mapping; while the CH32V006 and others in that series have a QingKe V2C and different GPIO, AFIO.

This is a coincidence, and as GPIO/AFIO configuration is not a feature of the microarchitecture, I don't think we should use the microarchitecture as the series.

I'd prefer keeping the directory as-is at the moment and revisiting in the future. Your choice though.

Copy link
Collaborator Author

@VynDragon VynDragon Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's only for sorting, there is no implication of having actual series behind it (at least for me): the support for Socs like CH592 was mentioned in the original discussion about this, and in the end, things are still set on soc/actual serie(ch32v30x, ch32v203, ch32v208, etc) level (and do note ch32v203 cant be shared with ch32v208 for example! the WCH 'series' dont actually mean much).

soc/wch/ch32v/qingke_v4/vector.S
@@ -18,12 +18,15 @@ GTEXT(__initialize)

SECTION_FUNC(vectors, ivt)
.option norvc
/* Jump to 0x08000008, into the main flash zone where j __start is */
lui x5, 0x8000
jr 0x8(x5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why doesn't the j __start work? Is the linker file correct?

Copy link
Collaborator Author

@VynDragon VynDragon Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

no, it jumps where the j start is, at 0x08000008. The actual issue it is needed with is because the aliased zone contains only the zero-wait flash zone, and when linking for running at 0x08000000, there is also the issue of the offset jumps and generally offset vs absolute adresses which wont always be correct.

In practice, to use non-zero wait flash or run at 0x08000000 instead of at 0x0 in the aliased copy, you need this or it will crash at some point in the code (due to the aforementioned issues).

fabiobaltieri
fabiobaltieri previously approved these changes Apr 26, 2025
View reviewed changes
@fabiobaltieri
Copy link
Member

@VynDragon can you rebase please?

@VynDragon
dts: wch: Enable using whole flash with CH32V208
5f09fa9 
Enables using the whole flash on CH32V208

Signed-off-by: Camille BAUD <[email protected]>
@VynDragon
soc: wch: Reorganize Qingke V4 Socs for future
a0682e9 
Reorganization for deduplication

Signed-off-by: Camille BAUD <[email protected]>
@VynDragon VynDragon force-pushed the ch32v208_fix_flash_addr branch from 015859c to a0682e9 Compare April 26, 2025 18:23
@VynDragon
Copy link
Collaborator Author

Done, rebased on main, hopefully I didnt mess it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miggazElquez miggazElquez miggazElquez approved these changes

@nzmichaelh nzmichaelh nzmichaelh approved these changes

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@carlocaione carlocaione Awaiting requested review from carlocaione

@edersondisouza edersondisouza Awaiting requested review from edersondisouza

@katsuster katsuster Awaiting requested review from katsuster

@kgugala kgugala Awaiting requested review from kgugala

@mgielda mgielda Awaiting requested review from mgielda

@npitre npitre Awaiting requested review from npitre

@tgorochowik tgorochowik Awaiting requested review from tgorochowik

@ycsin ycsin Awaiting requested review from ycsin

@andyross andyross Awaiting requested review from andyross

@teburd teburd Awaiting requested review from teburd

@fkokosinski fkokosinski Awaiting requested review from fkokosinski

At least 2 approving reviews are required to merge this pull request.

Assignees

@nzmichaelh nzmichaelh

Labels
area: RISCV RISCV Architecture (32-bit & 64-bit) area: Timer Timer platform: WinChipHead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@VynDragon @fabiobaltieri @miggazElquez @nzmichaelh @fkokosinski @kgugala @tgorochowik @zephyrbot