[PATCH v3 2/2] Subject: Update PWM driver imported from BBBIO
Martin Galvan
martin.galvan at tallertechnologies.com
Fri Jul 1 21:12:32 UTC 2016
Hi Punit, thanks for the patch. As we spoke in the group chat with the other mentors, this seems to
be good for a first version. I'll be pointing out the required changes for the next version, but
unless somebody sees anything blocking this should be good to merge.
Besides improving the code itself, my intention here is for you to learn from having mistakes pointed out.
Everyone else is more than welcome to comment too.
As a general rule, I suggest you reformat your code following a sane coding style. Like I mentioned
before, the RTEMS core files usually follow https://devel.rtems.org/wiki/Developer/Coding/Conventions ,
but BSPs have a bit more leeway. I saw the BBB files use the 1TBS with 2-space indentations,
so you should follow that. I also recommend checking for const-correctness as much as possible; it helps
reading and sometimes the compiler might benefit from it too.
You might want to run a spell checker on your comments as well; especially on the ones for exported functions.
On Fri, Jul 1, 2016 at 12:28 PM, Punit Vara <punitvara at gmail.com> wrote:
c/src/lib/libbsp/arm/beagle/Makefile.am | 4 +
c/src/lib/libbsp/arm/beagle/include/bbb-pwm.h | 162 +++++-
I think we should follow a naming convention for pwm.c/h. I suggested renaming bbb-pwm.h to just
pwm.h, but I think someone else said to keep it this way?
--- a/c/src/lib/libbsp/arm/beagle/Makefile.am
+++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
@@ -41,6 +41,7 @@ include_bsp_HEADERS += include/irq.h
include_bsp_HEADERS += include/i2c.h
include_bsp_HEADERS += include/beagleboneblack.h
include_bsp_HEADERS += include/bbb-gpio.h
+include_bsp_HEADERS += include/bbb-pwm.h
include_libcpu_HEADERS =
include_libcpu_HEADERS += ../../../libcpu/arm/shared/include/arm-cp15.h
@@ -117,6 +118,9 @@ libbsp_a_SOURCES += misc/i2c.c
# GPIO
libbsp_a_SOURCES += gpio/bbb-gpio.c
+#pwm
Whenever possible, try to follow the style of the file you're editing. For example,
here you should add a blank line between the comment and the previous line. Also, "#pwm" -> "# PWM".
--- a/c/src/lib/libbsp/arm/beagle/include/bbb-pwm.h
+++ b/c/src/lib/libbsp/arm/beagle/include/bbb-pwm.h
@@ -1,3 +1,23 @@
+/**
+ * @file
+ *
+ * @ingroup arm_beagle
+ *
+ * @brief BeagleBone Black BSP definitions.
Try a more exact description, like "BeagleBone Black PWM support definitions".
#ifndef LIBBSP_ARM_BEAGLE_BBB_PWM_H
#define LIBBSP_ARM_BEAGLE_BBB_PWM_H
Don't forget to change this macro if we decide to use pwm.h instead of bbb-pwm.h.
+#define BBB_CONTROL_CONF_GPMC_AD(n) (0x800 + (n * 4))
+#define BBB_CONTROL_CONF_LCD_DATA(n) (0x8a0 + (n * 4))
As Ketul suggested, it would be good if you did a quick check for which macros are processor-specific,
and named them AM335X_* instead of BBB_*.
+#define BBB_PWMSS_COUNT 3
+#define BBB_PWMSS0 0
+#define BBB_PWMSS1 1
+#define BBB_PWMSS2 2
This could be an enum instead. Something like:
typedef enum {
BBB_PWMSS0,
BBB_PWMSS1,
BBB_PWMSS2,
BBB_PWMSS_COUNT
} BBB_PWMSS;
A description on what these macros/enums are is always nice to have. For example, here you
could mention these IDs represent the different PWM subsystems in the MCU.
+#define BBB_P8_13_2B 3
+#define BBB_P8_19_2A 4
+#define BBB_P8_45_2A 5
+#define BBB_P8_46_2B 6
+#define BBB_P8_34_1B 7
+#define BBB_P8_36_1A 8
+#define BBB_P9_14_1A 9
+#define BBB_P9_16_1B 10
+#define BBB_P9_21_0B 11
+#define BBB_P9_22_0A 12
+#define BBB_P9_29_0B 13
+#define BBB_P9_31_0A 14
Similarly, these should have a proper description. I take it P8/9 refers to the expansion
connector, the number that follows is the pin number, and the following characters are the
module and output signal IDs, respectively. So for instance, BBB_P9_31_0A refers to pin 31
of connector P9, which corresponds to the EPWM0A output signal.
In any case, beagleboneblack.h already has some of these listed, so perhaps it would be good
to add them there instead. Those defines omit the last two characters though, but you could
specify them in a comment.
On a related note, I noticed that in many places you usually test for PWMSS 2, 1, and 0 in that
order, which is a bit awkward. Reverse it so that we always read about PWMSS 0, then 1, and then 2.
+#define BBB_MUX0 0
+#define BBB_MUX1 1
+#define BBB_MUX2 2
+#define BBB_MUX3 3
+#define BBB_MUX4 4
+#define BBB_MUX5 5
+#define BBB_MUX6 6
+#define BBB_MUX7 7
These macros don't convey any special meaning to me. It's almost the same as #define ZERO 0.
Instead, I'd prefer it if you had something like #define BBB_P9_31_MUX_PWM 4. That way you're
saying that this macro translates to whatever mode corresponds to PWM for pin 31 of connector P9.
I'm not sure if everyone agrees with me on that one though; suggestions are more than welcome.
+#define BBB_EPWM1 1
+#define BBB_EPWM2 2
+#define BBB_EPWM0 0
These 3 are unused and unnecessary. Please remove them.
+ * @brief This function intilize clock and pinmuxing for pwm sub system.
This isn't accurate anymore, since it doesn't perform any pinmux configuration.
+bool beagle_pwm_init(uint32_t pwmss_id);
+
+/* PWMSS setting
+ * set pulse argument of epwm module
As we said before, there should be a prominent comment here explaining how this works,
and why the TI StarterWare code isn't as accurate as this. It would also be nice if
we referred to the manual and the Google groups link explaining where the value for
SYSCLKOUT comes from, as we discussed in the previous patch. You could even go further
and write a README in the pwm directory with all we've learned. That would allow for
a shorter comment here. The README could also include a sample code of how to use this API.
Additionally, I insist in that we should have a naming convention for the PWM functions.
Sometimes we use "pwm", others "pwmss", and others "ehrpwm". You should decide on which
one is better and follow that convention on the rest of the functions.
+ */
+int beagle_pwmss_setting(uint32_t pwm_id, float pwm_freq, float dutyA, float dutyB);
I'd change this to "beagle_pwm_configure".
+/**
+ * @brief This API enables the particular PWM module.
+ *
+ * @param pwmid It is the instance number of EPWM of pwm sub system.
+ *
+ * @return true if successful
+ * @return false if fail
+ *
+ **/
Please leave a blank line between a function's declaration and the following function's
doxycomment.
+bool beagle_ehrpwm_enable(uint32_t pwmid);
+/**
+ * @brief This API disables the HR sub-module.
While I know HR stands for High-Resolution, it might be a bit confusing for the user.
Use EHRPWM (or just PWM) instead.
--- a/c/src/lib/libbsp/arm/beagle/pwm/pwm.c
+++ b/c/src/lib/libbsp/arm/beagle/pwm/pwm.c
@@ -1,407 +1,419 @@
+
+/* Currently these definitions are for BeagleBone Black board only
+ * Later on Beagle-xM board support can be added in this code.
+ * After support gets added if condition should be removed
If you're referring to IS_AM335X/IS_DM3730, from what I've seen there are always two
versions of the code, one for each SoC. So I don't think this #if will be removed,
even after support is added for DM3730. Maybe Ben/Ketul know a bit more about this than
I do, though.
*/
+#if IS_AM335X
-int BBBIO_PWM_Init()
+/*
+ * @brief This function select PWM module to be enabled
+ *
+ * @param pwm_id It is the instance number of EPWM of pwm sub system.
+ *
+ * @return Base Address of respective pwm instant.
I don't think these doxycomments are required if we already have them in the headers.
+static uint32_t select_pwmss(uint32_t pwm_id)
{
+uint32_t baseAddr=0;
+ if (pwm_id == BBB_PWMSS0)
+ {
+ baseAddr = AM335X_EPWM_0_REGS;
+ return baseAddr;
+ }
+ else if (pwm_id == BBB_PWMSS1)
+ {
+ baseAddr = AM335X_EPWM_1_REGS;
+ return baseAddr;
+ }
+ else if (pwm_id == BBB_PWMSS2)
+ {
+ baseAddr = AM335X_EPWM_2_REGS;
+ return baseAddr;
+ }
+ else
+ {
+ printf("Invalid PWM Id\n");
As we said before, you should look into some way to mark these printfs as debug-only, or remove them
altogether.
If we use an enum for PWM IDs, pwm_id's type should be that instead of uint32_t.
I don't like multiple returns in a single function; you could return baseAddr in all cases here.
Finally, please avoid mixing camelCase with underscores-style naming.
+bool beagle_epwm_pinmux_setup(uint32_t pin_no, uint32_t pwm_id)
{
+ switch(pwm_id) {
+ case BBB_PWMSS2:
+ switch(pin_no) {
+ case BBB_P8_13_2B:
+ REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(9)) = BBB_MUXMODE(BBB_MUX4);
+ break;
+ case BBB_P8_19_2A:
+ REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(8)) = BBB_MUXMODE(BBB_MUX4);
+ break;
+ case BBB_P8_45_2A:
+ REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_LCD_DATA(0)) = BBB_MUXMODE(BBB_MUX3);
+ break;
+ case BBB_P8_46_2B:
+ REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_LCD_DATA(1)) = BBB_MUXMODE(BBB_MUX3);
+ break;
+ default :
+ printf("Invalid pin for module 2\n");
+ return false;
I don't like nested switches at all. They're awkward to read and error-prone. Try to simplify this using functions
instead.
Same comment as before for multiple returns in a single function.
+ **/
+static void epwm_clock_enable(uint32_t pwm_id)
+{
+ if((pwm_id <3) && (pwm_id >=0)) {
If we set pwm_id to be an enum, this could be if (pwm_id < BBB_PWMSS_COUNT). This applies elsewhere.
+ uint32_t baseAddr;
+ baseAddr = select_pwmss(pwm_id);
This could be const uint32_t base_addr = select_pwmss(pwm_id);
+ REG(baseAddr - AM335X_EPWM_REGS + AM335X_PWMSS_CLKCONFIG) |= AM335X_PWMSS_CLK_EN_ACK;
+ } else {
+ printf("Invalid pwm_id\n");
You could return true/false instead, so the user can check if something went wrong.
+ */
+static void module_clk_config(uint32_t pwmss_id)
{
+ if(pwmss_id == 0)
+ {
+ REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) |=
+ AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE_ENABLE;
+
+ while(AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE_ENABLE !=
+ (REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) &
+ AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE));
+
+ while((AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_FUNC <<
+ AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_SHIFT) !=
+ (REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) &
+ AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST));
This is all a bit hard to read. Perhaps you could use variables to ease reading? E.g.
const uint32_t is_idle = AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_FUNC <<
AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_SHIFT;
const uint32_t clkctrl = AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL;
const uint32_t idle_bits = AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST;
while ((REG(clkctrl) & idle_bits) != is_idle) {
/* Keep waiting */
}
and so on.
(Quiz: why did I need to wrap the bitwise & in parens? The answer will surprise you.
It certainly surprised me and a coworker back when we were working on the BBB interrupt
handler and the damn thing would keep hanging).
+ else
+ {
+ printf("Please enter valid pwm Id \n");
Return an error status here too.
+bool beagle_pwm_init(uint32_t pwmss_id)
+{
+ bool status = true;
+ if((pwmss_id <3) && (pwmss_id >=0))
+ {
+ module_clk_config(pwmss_id);
+ epwm_clock_enable(pwmss_id);
+ pwmss_tbclk_enable(pwmss_id);
+ return status;
+ }
+ else {
+ status =false;
+ return status;
+ }
+}
This could be simplified:
const bool id_is_valid = pwmss_id < BBB_PWMSS_COUNT;
if (id_is_valid) {
/* Do stuff here */
}
return id_is_valid;
This applies elsewhere.
+int beagle_pwmss_setting(uint32_t pwm_id, float pwm_freq, float dutyA, float dutyB)
This function needs a bit more work than the others. I'd start by specifying what dutyA and dutyB mean;
and perhaps renaming them to something more descriptive (if possible).
+ uint32_t baseAddr;
+ int status = 1;
+
+ if(pwm_freq <= 0.5) {
+ status =0;
+ return status;
Same comments as before for multiple returns. Also, that 0.5f could be defined as PWM_FREQ_THRESHOLD,
with an accompanying comment explaining that we'll use that to avoid near-zero values, and that
it's fine to use it since the PWMSS can't generate such a low frequency anyway.
+ /*Compute necessary TBPRD*/
+ float Cyclens = 0.0f;
"Cyclens" isn't a good name; consider renaming this.
+ float Divisor =0;
+ int i,j;
These can be unsigned.
+ const float CLKDIV_div[] = {1.0,2.0,4.0,8.0,16.0,32.0,64.0,128.0};
+ const float HSPCLKDIV_div[] = {1.0, 2.0, 4.0, 6.0, 8.0, 10.0,12.0, 14.0};
There really needs to be a description of how these work, either in a comment or the README.
+ int NearCLKDIV =7;
+ int NearHSPCLKDIV =7;
+ int NearTBPRD =0;
Same goes for these. In general I don't like magic numbers; the arrays above may be an
exception because of the nature of the values.
+ else {
+ for (i=0;i<8;i++) {
+ for(j=0 ; j<8; j++) {
+ if((CLKDIV_div[i] * HSPCLKDIV_div[j]) < (CLKDIV_div[NearCLKDIV]
+ * HSPCLKDIV_div[NearHSPCLKDIV]) && (CLKDIV_div[i] * HSPCLKDIV_div[j] > Divisor)) {
+ NearCLKDIV = i;
+ NearHSPCLKDIV = j;
This is quite complex. I suggest either using temporary variables or functions to simplify this code.
+ baseAddr = select_pwmss(pwm_id);
+ REG16(baseAddr + AM335X_EPWM_TBCTL) &= ~(AM335X_TBCTL_CLKDIV_MASK | AM335X_TBCTL_HSPCLKDIV_MASK);
+
+ REG16(baseAddr + AM335X_EPWM_TBCTL) = (REG16(baseAddr + AM335X_EPWM_TBCTL) &
+ (~AM335X_EPWM_TBCTL_CLKDIV)) | ((NearCLKDIV
+ << AM335X_EPWM_TBCTL_CLKDIV_SHIFT) & AM335X_EPWM_TBCTL_CLKDIV);
Same comments here for using variables to ease reading.
+ /*setting clock divider and freeze time base*/
+ REG16(baseAddr + AM335X_EPWM_CMPB) = (unsigned short)((float)(NearTBPRD) * dutyB);
+ REG16(baseAddr + AM335X_EPWM_CMPA) = (unsigned short)((float)(NearTBPRD) * dutyA);
+ REG16(baseAddr + AM335X_EPWM_TBPRD) = (unsigned short)NearTBPRD;
+ REG16(baseAddr + AM335X_EPWM_TBCNT) = 0;
These casts are quite ugly. Please remove them if possible.
+bool beagle_ehrpwm_enable(uint32_t pwmid)
{
+ bool status = true;
+ uint32_t baseAddr;
+ if((pwmid<3) && (pwmid >=0)) {
+ baseAddr = select_pwmss(pwmid);
+ REG16(baseAddr + AM335X_EPWM_AQCTLA) = AM335X_EPWM_AQCTLA_ZRO_XAHIGH | (AM335X_EPWM_AQCTLA_CAU_EPWMXATOGGLE << AM335X_EPWM_AQCTLA_CAU_SHIFT);
+ REG16(baseAddr + AM335X_EPWM_AQCTLB) = AM335X_EPWM_AQCTLB_ZRO_XBHIGH | (AM335X_EPWM_AQCTLB_CBU_EPWMXBTOGGLE << AM335X_EPWM_AQCTLB_CBU_SHIFT);
+ REG16(baseAddr + AM335X_EPWM_TBCNT) = 0;
+ REG16(baseAddr + AM335X_EPWM_TBCTL) |= AM335X_TBCTL_FREERUN | AM335X_TBCTL_CTRMODE_UP;
It would be nice to explain what these lines actually do. Driver code is usually pretty obscure
in that it tends to be just a number of assignments to registers, with little explanation of how
things work inside.
Mind you, I'm not asking to transcribe the SoC manual here, just a few quick one-liners will do.
--- a/c/src/lib/libcpu/arm/shared/include/am335x.h
+++ b/c/src/lib/libcpu/arm/shared/include/am335x.h
@@ -467,4 +467,99 @@
+#define AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE (0x00000003u)
+#define AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_FUNC (0x0u)
+#define AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_SHIFT (0x00000010u)
Maybe I'm the only one who thinks like this, but I find shift values a bit confusing if they're
in hex. If instead of 0x10 that was 16, we could instantly map it to table 8-65 from the manual.
I looked at it and immediately thought that something was wrong before noticing the "0x" at the beginning.
That's enough for now. I might've missed something, though, so make sure you check my previous review as well.
More information about the devel
mailing list