Skip to content
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

arch/risc-v/bl602 : add pwm onshot watchdog driver. #2655

Merged
merged 1 commit into from Jan 7, 2021

Conversation

liangzhanggb
Copy link
Contributor

Summary

arch/risc-v/bl602 : add pwm onshot watchdog driver

Impact

N/A

Testing

config:

  1. → System Type → BL602 Peripheral Support → TIMER1
  2. → Device Drivers → Timer Driver Support → PWM Driver Support → (5)Number of Output Channels Per Timer
  3. → Device Drivers → Timer Driver Support → Oneshot timer drive
  4. → Device Drivers → Timer Driver Support → Watchdog Timer Support
  5. → RTOS Features → Performance Monitoring → Enable CPU load monitoring → Use external clock
  6. → Application Configuration → Examples → Watchdog Timer example
  7. → Application Configuration → Examples → Pulse width modulation (PWM) example
    result:
    out

@btashton
Copy link
Contributor

btashton commented Jan 5, 2021

@liangzhanggb @xiaoxiang781216 I will review and test this evening. First glace there are some things that need to be moved. The hardware/.h file should only have register information. I see some structs defined there that should be moved to the driver header in the next level up.

@liangzhanggb liangzhanggb force-pushed the merge_bl602_drv branch 3 times, most recently from e6ae845 to 583fa5e Compare January 6, 2021 01:59
@liangzhanggb
Copy link
Contributor Author

@liangzhanggb @xiaoxiang781216 I will review and test this evening. First glace there are some things that need to be moved. The hardware/.h file should only have register information. I see some structs defined there that should be moved to the driver header in the next level up.

I have fixed it, Thank you very much

Copy link
Contributor

@btashton btashton left a comment

Choose a reason for hiding this comment

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

@liangzhanggb I commented on some issues, but otherwise this looks very good.

I tested (after a couple fixes I called out) the wdt and pwm with two channels enabled and the timing looks good.

nsh> pwm -f 20000 -c 1 -c 2 -d 20 -d 80
nsh: pwm: too many arguments
pwm_main: starting output with frequency: 20000 channel: 1 duty: 00003333 channel: 2 duty: 0000cccc
pwm_main: stopping output
nsh> 

image

I would also recommend that you add a new config for the board to highlight the timer features. This is the one I generated if you want to use it. This also provides a little more code coverage of the drivers to the CI system.

defconfig.txt

arch/risc-v/src/bl602/bl602_pwm_lowerhalf.c Outdated Show resolved Hide resolved
arch/risc-v/src/bl602/bl602_pwm_lowerhalf.c Outdated Show resolved Hide resolved
arch/risc-v/src/bl602/bl602_tim.c Show resolved Hide resolved
arch/risc-v/src/bl602/Kconfig Outdated Show resolved Hide resolved
arch/risc-v/src/bl602/Make.defs Outdated Show resolved Hide resolved
arch/risc-v/src/bl602/bl602_pwm_lowerhalf.c Outdated Show resolved Hide resolved
arch/risc-v/src/bl602/bl602_start.c Outdated Show resolved Hide resolved
@btashton
Copy link
Contributor

btashton commented Jan 6, 2021

@liangzhanggb One more minor fix that I think will also get the build to pass. I verified your latest changes and they all look good. Thanks.

diff --git a/arch/risc-v/src/bl602/Kconfig b/arch/risc-v/src/bl602/Kconfig
index 9542d0be9c..0c819f95c8 100644
--- a/arch/risc-v/src/bl602/Kconfig
+++ b/arch/risc-v/src/bl602/Kconfig
@@ -13,6 +13,7 @@ config BL602_HAVE_UART0
        select ARCH_HAVE_UART0
        select UART0_SERIALDRIVER
        select ARCH_HAVE_SERIAL_TERMIOS
+       select ARCH_HAVE_PWM_MULTICHAN
 
 config BL602_UART0
        bool
@@ -37,6 +38,5 @@ config BL602_TIMER1
 
 config BL602_PWM0
        bool "PWM0"
-       select ARCH_HAVE_PWM_MULTICHAN
 
 endmenu
diff --git a/boards/risc-v/bl602/bl602evb/configs/timer/defconfig b/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
index 63ef88f96a..94d63251ff 100644
--- a/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
+++ b/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
@@ -55,6 +55,7 @@ CONFIG_ONESHOT=y
 CONFIG_PREALLOC_TIMERS=0
 CONFIG_PTHREAD_STACK_DEFAULT=8192
 CONFIG_PWM=y
+CONFIG_PWM_MULTICHAN=y
 CONFIG_PWM_NCHANNELS=2
 CONFIG_RAM_SIZE=134217728
 CONFIG_RAM_START=0xc0800000

@liangzhanggb
Copy link
Contributor Author

@liangzhanggb One more minor fix that I think will also get the build to pass. I verified your latest changes and they all look good. Thanks.

diff --git a/arch/risc-v/src/bl602/Kconfig b/arch/risc-v/src/bl602/Kconfig
index 9542d0be9c..0c819f95c8 100644
--- a/arch/risc-v/src/bl602/Kconfig
+++ b/arch/risc-v/src/bl602/Kconfig
@@ -13,6 +13,7 @@ config BL602_HAVE_UART0
        select ARCH_HAVE_UART0
        select UART0_SERIALDRIVER
        select ARCH_HAVE_SERIAL_TERMIOS
+       select ARCH_HAVE_PWM_MULTICHAN
 
 config BL602_UART0
        bool
@@ -37,6 +38,5 @@ config BL602_TIMER1
 
 config BL602_PWM0
        bool "PWM0"
-       select ARCH_HAVE_PWM_MULTICHAN
 
 endmenu
diff --git a/boards/risc-v/bl602/bl602evb/configs/timer/defconfig b/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
index 63ef88f96a..94d63251ff 100644
--- a/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
+++ b/boards/risc-v/bl602/bl602evb/configs/timer/defconfig
@@ -55,6 +55,7 @@ CONFIG_ONESHOT=y
 CONFIG_PREALLOC_TIMERS=0
 CONFIG_PTHREAD_STACK_DEFAULT=8192
 CONFIG_PWM=y
+CONFIG_PWM_MULTICHAN=y
 CONFIG_PWM_NCHANNELS=2
 CONFIG_RAM_SIZE=134217728
 CONFIG_RAM_START=0xc0800000

I have fixed it, Thank you very much

@btashton
Copy link
Contributor

btashton commented Jan 7, 2021

LGTM I see a couple other unrelated things for this port in the Kconfigs that I can clean in another PR.

@btashton btashton merged commit 2889315 into apache:master Jan 7, 2021
@btashton btashton added this to To-Add in Release Notes - 10.1.0 Apr 12, 2021
@jerpelea jerpelea moved this from To-Add to arch in Release Notes - 10.1.0 Apr 13, 2021
@jerpelea jerpelea moved this from arch to Added in Release Notes - 10.1.0 Apr 16, 2021
Comment on lines +375 to +376
bl602_pwm_freq(priv, i, info->frequency);
bl602_pwm_duty(priv, i, info->channels[i].duty);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the second argument have been info->channels[i].channel? Otherwise there's no way to change the order of channels.

Also PR #4227 added an option to break these loops when channel == -1, it looks like it was missed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR #5091 to add the early exit from PR #4227.

Copy link
Contributor

Choose a reason for hiding this comment

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

and PR #5093 to use chan instead of i.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants