Skip to content

cpu/fe310: add pwm peripheral driver#12953

Closed
aabadie wants to merge 2 commits intoRIOT-OS:masterfrom
aabadie:pr/cpu/fe310_pwm
Closed

cpu/fe310: add pwm peripheral driver#12953
aabadie wants to merge 2 commits intoRIOT-OS:masterfrom
aabadie:pr/cpu/fe310_pwm

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Dec 15, 2019

Contribution description

This PR implements the PWM driver for RISC-V FE310 CPU and adds a basic configuration for Sifive HiFive1 and HiFive1b boards.

Since a valid core clock value is required, this PR is based on #12519.

Testing procedure

  • Build and flash the tests/periph_pwm application on a Hifive1/b board:
$ make BOARD=hifive1b -C tests/periph_pwm flash term
  • Call the osci shell command
> osci

You should see the on-board leds oscillate synchronously (they are connected to an RGD led, so the led oscillates in white). You can also plug a led on the other configured pins (Arduino D4, D16, D17) and watch them oscillate as well.

Issues/PRs references

Based on #12934

@aabadie aabadie added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Area: cpu Area: CPU/MCU ports labels Dec 15, 2019
@aabadie aabadie requested a review from fjmolinas December 15, 2019 15:26
@aabadie aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 15, 2019
@aabadie aabadie changed the title cpu/fe310: add pwm driver cpu/fe310: add pwm peripheral driver Dec 15, 2019
@aabadie aabadie force-pushed the pr/cpu/fe310_pwm branch 2 times, most recently from f81822b to b8c1e78 Compare December 16, 2019 12:47
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first labels Jan 11, 2020
@aabadie
Copy link
Contributor Author

aabadie commented Jan 11, 2020

rebased

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Looks good, one minor comment. Will test on monday.

* @{
*/
#define PWM_NUMOF (3)
static const pwm_conf_t pwm_config[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like hifive1 and hhifive1b can share the config.

Comment on lines +38 to +42
uint8_t scale = 0;
while ((scale < (PWM_CMPWIDTH - 1)) &&
(freq * res) > (uint32_t)(1 << (scale + PWM_CMPWIDTH))) {
scale++;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This calculation seems very bad to me, how can you get a good approximation of freq without taking the cpu_freq into account? It results in very weird frequency settings that do not make much sense.

If you want full resolution for example that means you can only scale the register. But if you wan't only lets say 8 bits of resolution then you have 8 more to tune the desired frequency. For me the calculation should take into account:

  • bits left over by desired resolution: res/2^16
  • cpu_freq

We are basically trying to resolve:

min CPU_FREQ * (2^x)/(2^(16-scale)) - freq, where x = 1...2^rembits and scale = 0...(PWM_CMPWIDTH - 1). I don't know efficient algorithms to find this, would have to think about it more. But one way would be find a scale which gives you a PWM_FREQ higher than freq and then trim the value with the remaining resolution bits. Another option is for every scale find the best match and then compare them all.

For the first suggestion.

  1. In pseudo code: find scaling that fives you a PWM_FREQ > freq:
    uint8_t scale = (PWM_CMPWIDTH - 1);
    while ((scale > 0) &&
           cpu_freq() / (uint32_t)(1 << (scale + PWM_CMPWIDTH)) << freq) {
        scale--;
    }
  1. Use the leftover bits for the required resolution to trim PWM_FREQ as close to freq as possible

Then those leftover bits or rem_res give you more wiggle room to set the a freq closest to the desired one.

@aabadie aabadie removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 17, 2020
@aabadie
Copy link
Contributor Author

aabadie commented Jan 17, 2020

I removed the Murdock label to get this one out of the queue.

@stale
Copy link

stale bot commented Jul 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jul 20, 2020
@stale stale bot closed this Aug 21, 2020
@aabadie aabadie reopened this Aug 22, 2020
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 22, 2020
@benpicco benpicco requested a review from bergzand August 27, 2020 20:59
@benpicco
Copy link
Contributor

Needs a rebase

@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 19, 2021
@stale stale bot closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms State: stale State: The issue / PR has no activity for >185 days Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants