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

cpu/sam3x8e: Add pwm implementation #3170

Merged
merged 1 commit into from
Jun 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions boards/arduino-due/Makefile.features
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
FEATURES_PROVIDED += cpp
FEATURES_PROVIDED += periph_uart
FEATURES_PROVIDED += periph_gpio
FEATURES_PROVIDED += periph_pwm
FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_random
FEATURES_MCU_GROUP = cortex_m3_1
32 changes: 32 additions & 0 deletions boards/arduino-due/include/periph_conf.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*
* @author Hauke Petersen <hauke.petersen@fu-berlin.de>
* @author Peter Kietzmann <peter.kietzmann@haw-hamburg.de>
* @author Andreas "Paul" Pauli <andreas.pauli@haw-hamburg.de>
*/

#ifndef __PERIPH_CONF_H
Expand Down Expand Up @@ -342,6 +343,37 @@ extern "C" {
#define GPIO_B21_MAP 31
/** @} */

/**
* @name PWM configuration
* @{
*/
#define PWM_NUMOF (1U)
#define PWM_0_EN (1)
#define PWM_MAX_VALUE (0xffff)
#define PWM_MAX_CHANNELS (4U)

/* PWM_0 configuration */
#define PWM_0_DEV PWM
Copy link
Contributor

Choose a reason for hiding this comment

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

please implement using structs and calculated offsets.

#define PWM_0_PID ID_PWM
#define PWM_0_CHANNELS (4U)
#define PWM_0_DEV_CH0 (&(PWM_0_DEV->PWM_CH_NUM[4]))
#define PWM_0_ENA_CH0 PWM_ENA_CHID4
#define PWM_0_PORT_CH0 PIOC
#define PWM_0_PIN_CH0 PIO_PC21B_PWML4
#define PWM_0_DEV_CH1 (&(PWM_0_DEV->PWM_CH_NUM[5]))
#define PWM_0_ENA_CH1 PWM_ENA_CHID5
#define PWM_0_PORT_CH1 PIOC
#define PWM_0_PIN_CH1 PIO_PC22B_PWML5
#define PWM_0_DEV_CH2 (&(PWM_0_DEV->PWM_CH_NUM[6]))
#define PWM_0_ENA_CH2 PWM_ENA_CHID6
#define PWM_0_PORT_CH2 PIOC
#define PWM_0_PIN_CH2 PIO_PC23B_PWML6
#define PWM_0_DEV_CH3 (&(PWM_0_DEV->PWM_CH_NUM[7]))
#define PWM_0_ENA_CH3 PWM_ENA_CHID7
#define PWM_0_PORT_CH3 PIOC
#define PWM_0_PIN_CH3 PIO_PC24B_PWML7
/** @} */

#ifdef __cplusplus
}
#endif
Expand Down
291 changes: 291 additions & 0 deletions cpu/sam3x8e/periph/pwm.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,291 @@
/*
* Copyright (C) 2015 Hamburg University of Applied Sciences
*
* This file is subject to the terms and conditions of the GNU Lesser General
* Public License v2.1. See the file LICENSE in the top level directory for more
* details.
*/

/**
* @ingroup cpu_sam3x8e
* @{
*
* @file
* @brief CPU specific low-level PWM driver implementation for the SAM3X8E
*
* @author Andreas "Paul" Pauli <andreas.pauli@haw-hamburg.de>
*
* @}
*/
#include <stdint.h>
#include "board.h"
#include "periph/pwm.h"

/*
* guard file in case no PWM device is defined,
*/
#if PWM_NUMOF && PWM_0_EN

#define ERR_INIT_MODE (-1)
#define ERR_INIT_BWTH (-2)
#define ERR_SET_CHAN (-1)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if these macros are necessary. The API defines the error values. Anyway, if you want it this way I guess it's ok

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are already constants or macros defined, please let me know where to find them. More general, I prefer to write something like:

    return ERR_SET_CHAN;

instead of

    return -1;

Copy link
Member

Choose a reason for hiding this comment

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

The second version was what I meant, but I'm fine with your solution. One could have a look at errno.h if there are useful macros which match our API, but I guess the answer would be "no" :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

In RIOT I just found one errno.h. "cpu/atmega_common/avr-libc-extra/errno.h". Wrong cpu. ;-)
Before defining my own, I somehow expected to find them in "pwm.h".

Copy link
Contributor

Choose a reason for hiding this comment

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

errno.h is supplied by the C-library, so check the newlib includes...

#define MCK_DIV_LB_MAX (10U)

int pwm_init(pwm_t dev, pwm_mode_t mode,
unsigned int frequency,
unsigned int resolution)
{

Copy link
Member

Choose a reason for hiding this comment

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

Where do you use this macro?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's related to #3170 (diff). Will be removed.

int32_t retval = ERR_INIT_MODE; /* Worst/First case */
uint32_t pwm_clk = 0; /* Desired/real pwm_clock */
uint32_t diva = 1; /* Candidate for 8bit divider */
uint32_t prea = 0; /* Candidate for clock select */

switch (dev) {
#if PWM_0_EN
Copy link
Contributor

Choose a reason for hiding this comment

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

if #if PWM_0_EN shows up more than once in this file, the implementation is broken. (Don't copy the bad coding style of existing implementations.)


case PWM_0:
break;
#endif

default:
return ERR_INIT_MODE;
}

/*
* Mode check.
* Only PW_LEFT -which is hw default- supported for now.
*/
switch (mode) {
case PWM_LEFT:
break;

case PWM_RIGHT:
case PWM_CENTER:
default:
return ERR_INIT_MODE;
}
Copy link
Member

Choose a reason for hiding this comment

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

What happened to PWM_RIGHT?

Copy link
Member Author

Choose a reason for hiding this comment

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

The same what should have happend to PWM_CENTER at the moment. default-case (which has the wrong error code also).
As it say in the comment, the block is useless for the moment and just does no harm.

I will implement the center align a few commits ahead, when I fixed other issues. Shall I:

  • [] Cut it out and put it back in later?
  • [] Let it stay (in shame) meanwhile?

Copy link
Member

Choose a reason for hiding this comment

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

I tend to have a "completely useless" switch-block to (i) be prepared for further implementations and (ii) highlight the lack so one immediately sees what's missing.


/* Should check if "|log_2 frequency|+|log_2 resolution| <= 32" */
pwm_clk = frequency * resolution;

/*
* The pwm provides 11 prescaled clocks with (MCK/2^prea | prea=[0,10])
* and a divider (diva) with a denominator range [1,255] in line.
*/
if (F_CPU < pwm_clk) { /* Have to cut down resulting frequency. */
frequency = F_CPU / resolution;
}
else { /* Estimate prescaler and divider. */
diva = F_CPU / pwm_clk;

while ((prea < MCK_DIV_LB_MAX) && (~0xff & diva)) {
prea = prea + 1;
diva = diva >> 1;
}

frequency = F_CPU / ((resolution * diva) << prea);
}
Copy link
Member

Choose a reason for hiding this comment

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

I didn't go into details here but did you compare other drivers? Your solution looks a bit complicated to me. Also I think the while-loop can be avoided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please clarify what is considered as complicated.
The distiction for a case where the frequency should be cut in favour to provide the desired resolution is part of the interface description.
For the reason of estimating two scaler values please have a short look at the block diagram "38.6.1 PWM Clock Generator" in the Reference Manual

Copy link
Member

Choose a reason for hiding this comment

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

Ok, will do hopefully soon! Also don't take all my comments too serious. Sometimes I just want to point to things upon which I stumbled and hope to trigger an optimization :-) . This is mostly the fact when I write in a questioning way. But also a constitutive response is a good result. As I said, I will look into the section you mentioned.


retval = frequency;

/* Activate PWM block by enabling it's clock. */
PMC->PMC_PCER1 = PMC_PCER1_PID36;

/* Unlock User Interface */
PWM_0_DEV->PWM_WPCR = PWM_ENA_CHID0;
Copy link
Member

Choose a reason for hiding this comment

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

What I wanted to say in some previous comments: You don't use the input parameter pwm_t dev. That means if an application calls the init function for PWM_2 you don't even return with an error but just initialize PWM_0

Copy link
Member Author

Choose a reason for hiding this comment

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

O.K., got it. I focused to much on pwm_t dev as a selector, in case there are more than one. So I lost the aspect of whitelisting.
I will (re)introduce the switches in all functions.

Anyhow, your above example wouldn't compile, as long as nobody places #define PWM_2_EN 1 in arduino-due/include/periph_conf.h. And no one would be so insane to do that. ;)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there you're right. It was just an example and now you got what I meant :-) . Let's talk about the unclear things tomorrow. BTW: nice to hear that your drivers is working now!


/* Disable all channels to allow CPRD updates. */
PWM_0_DEV->PWM_DIS = 0xff;

/* Step 2. Configure clock generator */
PWM_0_DEV->PWM_CLK = PWM_CLK_PREA(prea) | PWM_CLK_DIVA(diva) |
(~(PWM_CLK_PREA_Msk | PWM_CLK_DIVA_Msk) &
PWM_0_DEV->PWM_CLK);

/* +++++++++++ Configure and init channels +++++++++++++++ */

/* Set clock source, resolution, duty-cycle and enable */
#if PWM_0_CHANNELS > 0
PWM_0_DEV_CH0->PWM_CMR = PWM_CMR_CPRE_CLKA;
PWM_0_DEV_CH0->PWM_CPRD = resolution - 1;
PWM_0_DEV_CH0->PWM_CDTY = 0;
PWM_0_DEV->PWM_ENA = PWM_0_ENA_CH0;
#endif
#if PWM_0_CHANNELS > 1
PWM_0_DEV_CH1->PWM_CMR = PWM_CMR_CPRE_CLKA;
PWM_0_DEV_CH1->PWM_CPRD = resolution - 1;
PWM_0_DEV_CH1->PWM_CDTY = 0;
PWM_0_DEV->PWM_ENA = PWM_0_ENA_CH1;
#endif
#if PWM_0_CHANNELS > 2
PWM_0_DEV_CH2->PWM_CMR = PWM_CMR_CPRE_CLKA;
PWM_0_DEV_CH2->PWM_CPRD = resolution - 1;
PWM_0_DEV_CH2->PWM_CDTY = 0;
PWM_0_DEV->PWM_ENA = PWM_0_ENA_CH2;
#endif
#if PWM_0_CHANNELS > 3
PWM_0_DEV_CH3->PWM_CMR = PWM_CMR_CPRE_CLKA;
PWM_0_DEV_CH3->PWM_CPRD = resolution - 1;
PWM_0_DEV_CH3->PWM_CDTY = 0;
PWM_0_DEV->PWM_ENA = PWM_0_ENA_CH3;
#endif

/* +++++++++++ Configure and init channels ready++++++++++ */


/*
* Disable GPIO and set peripheral A/B ("0/1") for pwm channel pins.
*/
#if PWM_0_CHANNELS > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

same here!

PWM_0_PORT_CH0->PIO_PDR |= PWM_0_PIN_CH0;
PWM_0_PORT_CH0->PIO_ABSR |= PWM_0_PIN_CH0;
#endif
#if PWM_0_CHANNELS > 1
PWM_0_PORT_CH1->PIO_PDR |= PWM_0_PIN_CH1;
PWM_0_PORT_CH1->PIO_ABSR |= PWM_0_PIN_CH1;
#endif
#if PWM_0_CHANNELS > 2
PWM_0_PORT_CH2->PIO_PDR |= PWM_0_PIN_CH2;
PWM_0_PORT_CH2->PIO_ABSR |= PWM_0_PIN_CH2;
#endif
#if PWM_0_CHANNELS > 3
PWM_0_PORT_CH3->PIO_PDR |= PWM_0_PIN_CH3;
PWM_0_PORT_CH3->PIO_ABSR |= PWM_0_PIN_CH3;
#endif

return retval;
Copy link
Member

Choose a reason for hiding this comment

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

same as above..

}

/*
* Update duty-cycle in channel with value.
* If value is larger than resolution set by pwm_init() it is cropped.
*/
int pwm_set(pwm_t dev, int channel, unsigned int value)
{

int retval = ERR_SET_CHAN; /* Worst case */
uint32_t period = 0; /* Store pwm period */
PwmCh_num *chan = (void *)0; /* Addressed channel. */

switch (dev) {
#if PWM_0_EN

case PWM_0:
break;
#endif

default:
return ERR_SET_CHAN;
}


switch (channel) {
#if PWM_0_CHANNELS > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

implement without #if and duplicate code!


case 0:
chan = PWM_0_DEV_CH0;
break;
#endif
#if PWM_0_CHANNELS > 1

case 1:
chan = PWM_0_DEV_CH1;
break;
#endif
#if PWM_0_CHANNELS > 2

case 2:
chan = PWM_0_DEV_CH2;
break;
#endif
#if PWM_0_CHANNELS > 3

case 3:
chan = PWM_0_DEV_CH3;
break;
#endif

default:
retval = ERR_SET_CHAN;
}

if (chan) {
period = chan->PWM_CPRD & PWM_CPRD_CPRD_Msk;


if (value < period) {
chan->PWM_CDTYUPD = value;
}
else { /* Value Out of range. Clip silent as required by interface. */
chan->PWM_CDTYUPD = period;
}

retval = 0;
}

return retval;
}

/*
* Continue operation.
*/
void pwm_start(pwm_t dev)
{
switch (dev) {
#if PWM_0_EN

case PWM_0:
PMC->PMC_PCER1 |= PMC_PCER1_PID36;
break;
#endif
}
}

/*
* Stop operation and set output to 0.
*/
void pwm_stop(pwm_t dev)
{
switch (dev) {
#if PWM_0_EN

case PWM_0:
PMC->PMC_PCDR1 |= PMC_PCDR1_PID36;
break;
#endif
}
}

/*
* The device is reactivated by by clocking the device block.
* Operation continues where it has been stopped by poweroff.
*/
void pwm_poweron(pwm_t dev)
{
switch (dev) {
#if PWM_0_EN

case PWM_0:
PMC->PMC_PCER1 |= PMC_PCER1_PID36;
break;
#endif
}
}

/*
* The device is set to power saving mode by disabling the clock.
*/
void pwm_poweroff(pwm_t dev)
{
switch (dev) {
#if PWM_0_EN

case PWM_0:
PMC->PMC_PCDR1 |= PMC_PCDR1_PID36;
break;
#endif
}
}

#endif /* PWM_NUMOF */