Skip to content

drivers: pwm: add a CH32V00x General-prupose Timer Module (GPTM) driver #88388

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.

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

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

Conversation

nzmichaelh
Copy link
Collaborator

The GPTM is a general purpose module with a 16 bit prescaler, 16 bit
counter, and 4 compare units that can be used for PWM generation.

Add a driver and test via the samples.

Tested on the CH32V003.

@nzmichaelh
Copy link
Collaborator Author

@VynDragon @kholia @andrei-edward-popa FYI. I hope to send this for review this week.

@nzmichaelh nzmichaelh force-pushed the ch32v-pwm branch 2 times, most recently from afc7ec5 to 20e4f67 Compare April 10, 2025 19:03
@nzmichaelh nzmichaelh requested a review from VynDragon April 10, 2025 19:12
@nzmichaelh nzmichaelh marked this pull request as ready for review April 10, 2025 19:12
@github-actions github-actions bot added area: RISCV RISCV Architecture (32-bit & 64-bit) area: LED Label to identify LED subsystem platform: WinChipHead area: Samples Samples area: PWM Pulse Width Modulation labels Apr 10, 2025
@kartben kartben requested review from Copilot and removed request for mgielda April 10, 2025 19:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 2 comments.

Files not reviewed (10)
  • boards/wch/ch32v003evt/ch32v003evt-pinctrl.dtsi: Language not supported
  • boards/wch/ch32v003evt/ch32v003evt.dts: Language not supported
  • drivers/pwm/CMakeLists.txt: Language not supported
  • drivers/pwm/Kconfig: Language not supported
  • drivers/pwm/Kconfig.wch: Language not supported
  • dts/riscv/wch/ch32v0/ch32v003.dtsi: Language not supported
  • samples/basic/blinky_pwm/boards/ch32v003evt.conf: Language not supported
  • samples/basic/blinky_pwm/boards/ch32v003evt.overlay: Language not supported
  • samples/drivers/led/pwm/boards/ch32v003evt.conf: Language not supported
  • samples/drivers/led/pwm/boards/ch32v003evt.overlay: Language not supported

@paulwedeck
Copy link
Contributor

If I am not mistaken, the advanced timer (adtm; TIM1 in v003) is just a gptm with a few differences (see "11.2.2 Difference between General-purpose Timer and Advanced-control Timer" in the ch32v003rm).
Have you considered renaming the driver to make it more generic?

On a different note:
The STM32 timer/pwm also has a prescaler variable and they use the value range 0 to 0xffff instead of 1 to 0x10000 in the device tree binding.
I think it can be confusing to use a different (although arguably more reasonable) convention.

@nzmichaelh
Copy link
Collaborator Author

re: ADTM, OK if I do this in a follow up PR? This would need a new dts binding for wch,adtm, but I can add multiple compatible sections to the actual driver.

re: prescaler: agreed, will change. I looked through the dts bindings and the majority convention is that the prescaler field maps directly to hardware. It's a mix though - for example, with a power-of-2 prescaler it's sometimes an enum and sometimes the raw value.

Copy link
Collaborator

@VynDragon VynDragon left a comment

Choose a reason for hiding this comment

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

It's really not happy on linkw with necessary changes
image

But it 'works' on ch32v003, see attached video of blinky_pwm? Is this the expected behavior?

VID_20250412_185931.mp4

@paulwedeck
Copy link
Contributor

re: ADTM, OK if I do this in a follow up PR? This would need a new dts binding for wch,adtm, but I can add multiple compatible sections to the actual driver.

Sorry, I am not familiar enough with Zephyr development to answer that. My suggestion was based on the misconception that there can only be one compatible string per driver.

@nzmichaelh
Copy link
Collaborator Author

re: compatible driver, it's reasonably common - see serial/uart_gecko.c and dac/dac_ad569x.c.

re: samples/basic/blinky_pwm behaviour: the sample blinks faster and faster from 1 Hz to 128 Hz and then repeats, so the video looks about right.

Also, I've done some bench testing and fixed issues with invert, the prescaler in the samples, running all channels at once, and the channel numbering.

re: numbering, it seems that the standard is to number from 0, even if the channel is CH1 etc.

@VynDragon
Copy link
Collaborator

VynDragon commented Apr 13, 2025

Please, when do you do a rebase, push it as separately to a update of the code, it makes the whole thing unreadable, and impossible to check quickly what changed.

@VynDragon
Copy link
Collaborator

Still very broken on linkw, still working on ch32v003evt. Do you intend to support only ch32v003 or all wch with this PR?

@kartben kartben assigned nzmichaelh and unassigned kartben Apr 15, 2025
@nzmichaelh
Copy link
Collaborator Author

Please, when do you do a rebase, push it as separately to a update of the code, it makes the whole thing unreadable, and impossible to check quickly what changed.

Will do.

@nzmichaelh
Copy link
Collaborator Author

Still very broken on linkw, still working on ch32v003evt. Do you intend to support only ch32v003 or all wch with this PR?

I'd like to. I have a linkw and BluePill+ arriving soon and I'll test on that. If you have any insights, please let me know.

@VynDragon
Copy link
Collaborator

I'd like to. I have a linkw and BluePill+ arriving soon and I'll test on that. If you have any insights, please let me know.

Ah, I would've warned you against getting linkW specifically for CH32V208, the GBU6 package highly limits what is doable vs the other packages.

Sorry, no insights yet, just seems to look like some kind of clock issue? Possibly on linkW it conflicts with the usart2 clock?

Here is what i changed
patch.txt

@nzmichaelh
Copy link
Collaborator Author

Short story: this works on the CH32V203, good.

My BluePill Plus arrvied today and the samples/basic/blinky_pwm and samples/drivers/led/pwm work correctly.

To reproduce, add the BluePill support in #87490, patch in the CH32V203 timer definitions at nzmichaelh@ae5d6f2 and the samples patch in nzmichaelh@f00be74

Unfortunately the BluePill LED isn't on a PWM pin, so the sample uses PB9 instead.

Note that I leave the main clock unchanged at 120 MHz, which might be why the UART showed noise in your test.

Once the BluePill support is committed I'll move the CH32V203 support into this PR.

@VynDragon
Copy link
Collaborator

VynDragon commented Apr 19, 2025

Great! It means the issue is specific to ch32v208 and means there is no issue with the driver itself.

Note that I leave the main clock unchanged at 120 MHz, which might be why the UART showed noise in your test.

Yea might've been 144Mhz on the code version i used of ch32v208

@VynDragon
Copy link
Collaborator

VynDragon commented Apr 19, 2025

Note if you want to see CH32V203, the CH32V208 update, and the CH32V303 merged faster, this is blocking them: #85395

VynDragon
VynDragon previously approved these changes Apr 19, 2025
@nzmichaelh
Copy link
Collaborator Author

Rebased to fix the merge conflict.

@nzmichaelh nzmichaelh force-pushed the ch32v-pwm branch 2 times, most recently from d300091 to a4594de Compare May 1, 2025 13:13
nzmichaelh added 2 commits May 1, 2025 13:14
The GPTM is a general purpose module with a 16 bit prescaler, 16 bit
counter, and 4 compare units that can be used for PWM generation.

Use the same style as gd32 where the timer is a counter and the PWM
mode is a child node.

Signed-off-by: Michael Hope <[email protected]>
The CH32V003 has a single red LED that can be driven by GPIO or TIM2
CH1. Add to the samples.

Signed-off-by: Michael Hope <[email protected]>
@nzmichaelh
Copy link
Collaborator Author

I've rebased to fix a merge conflict and separately made some small changes based on my ongoing counter PR.

To recap: this works fine on the CH32V003EVT, the V203 based BluePill+, but there's corruption on the V208 based linkw. I've checked the remapping, clocks, and interrupts. I'd like to merge this and then fix the V208 issue while adding support.

@nzmichaelh nzmichaelh removed their assignment May 1, 2025
Copy link
Collaborator

@VynDragon VynDragon left a comment

Choose a reason for hiding this comment

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

It's a bit weird honestly. Anyway i wont be the one to stop doing it like this, i would do the same for simplicity.

@kartben
Copy link
Collaborator

kartben commented May 1, 2025

@nzmichaelh as maintainer of the platform, it's fine for you to be the assignee (even as the author) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: LED Label to identify LED subsystem area: Pinctrl area: PWM Pulse Width Modulation area: RISCV RISCV Architecture (32-bit & 64-bit) area: Samples Samples platform: WinChipHead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants