Subject: Add PWM driver for beagle bone black
Martin Galvan
martin.galvan at tallertechnologies.com
Tue Jun 21 19:37:33 UTC 2016
Hi Punit, thanks for sending this. If I understood correctly this is the BBBIO code
plus some changes of your own, right? If so, I think it would be best to send a patch
with the BBBIO code as is, and then another with your changes on top of it. I think
that was what we were going for with StarterWare, too.
For imported code we usually stick to the coding style of whatever we're importing.
However, I see that bbb-pwm.c isn't actually that much code. If you feel like it
you could reformat it to follow a more standard coding style. The core RTEMS files
follow https://devel.rtems.org/wiki/Developer/Coding/Conventions, though BSP code
can have a bit more leeway.
I prefer using stdint types such as uint32_t for driver code. Also, const-correctness
is always great to have.
Additional comments are below. I don't know for certain which code is yours and which
comes from BBBIO, so I'll give a quick look at everything, then when you send your
changes alone I'll take a deeper look at those.
On Tue, Jun 21, 2016 at 1:26 PM, Punit Vara <punitvara at gmail.com> wrote:
> This patch adds required definitions, registers definitions and testsuit to
> test pwm driver for beagle bone black.
Overall I think a slightly more detailed log message is required. Where did you
get the code from, what testing did you perform, why we used BBBIO instead of StarterWare
(license + bugs), and so on. Anything that a future maintainer might find useful.
> testsuites/samples/Makefile.am | 2 +-
> testsuites/samples/configure.ac | 1 +
> testsuites/samples/pwm/Makefile.am | 19 ++
> testsuites/samples/pwm/init.c | 70 ++++++
> testsuites/samples/pwm/pwm.doc | 9 +
> testsuites/samples/pwm/pwm.scn | 3 +
Unless I've missed something, right now RTEMS doesn't have BSP-specific tests. You *could*,
however, add some sort of README with an example of how to use the BBBIO API, as documentation.
> diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am b/c/src/lib/libbsp/arm/beagle/Makefile.am
> index 20d3092..68bdbd4 100644
> --- a/c/src/lib/libbsp/arm/beagle/Makefile.am
> +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
> @@ -117,6 +117,9 @@ libbsp_a_SOURCES += misc/i2c.c
> # GPIO
> libbsp_a_SOURCES += gpio/bbb-gpio.c
>
> +#pwm
> +libbsp_a_SOURCES += pwm/bbb-pwm.c
IIRC only the GPIO-related files have the name of the BSP prepended to them to help
the build system distinguish them from the ones belonging to the generic API. That is,
this could probably be named pwm.c.
> diff --git a/c/src/lib/libbsp/arm/beagle/pwm/bbb-pwm.c b/c/src/lib/libbsp/arm/beagle/pwm/bbb-pwm.c
> new file mode 100644
> index 0000000..a2f1107
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/beagle/pwm/bbb-pwm.c
> @@ -0,0 +1,345 @@
> +/* This file is based on following licence
> + * Copyright (c) 2015, Shabaz, VegetableAvenger
I'm not sure about this. I think it would be best if we didn't add such aliases to
copyright notices, and instead linked to the repository URL saying something like
"This code is based on the BBBIO sources, available at...". But maybe there's a better
way to go about this. What does the rest of you guys think?
> + * Added clock functions and improved pwm_enable function
I don't think these kind of messages should go here. If you want to make clear that you changed
imported code, do so in a more detailed way in the commit log or maybe atop the function
you changed.
> +#include<libcpu/am335x.h>
> +#include<stdio.h>
> +#include<bsp/gpio.h>
> +#include<bsp/bbb-gpio.h>
> +#include<bsp.h>
Add a space between #include and the file you're including.
> +#define BASE_CLOCK 100000000
We probably need a better name for this. We could name it SYSCLKOUT (since that's the name the
manual uses), and add a prominent comment explaining why we're using that value (remember that
the manual seldom mentions what the actual value is, save for a note below table 15-something).
It would also be nice to link to https://groups.google.com/forum/#!topic/beagleboard/Ed2J9Txe_E4
since there we can find a good explanation of why that value is safe to use.
> +void pwm_init(unsigned int baseAddr, unsigned int PWMSS_ID)
PWMSS_ID sounds like a macro name; regular variable names are lowercase.
I think this API could be improved. Correct me if I'm wrong, but it seems that a given PWMSS
instance must have its clock enabled twice: first from the Clock Module (through the CM_PER register)
and then from the PWM module itself (through its CLKCONFIG register). In any case, you'll always want
to enable the same instance, so having two separate arguments for it is confusing and error-prone.
Additionally, the user shouldn't have to know about hardware addresses; I think you could use an enum
or something similar here and according to its value use the appropriate addresses and offsets.
Of course, don't forget to check if the value you're getting is actually valid.
A comment explaining what the function does is always welcome, especially in driver code.
> +{
> + module_clk_config(PWMSS_ID);
> + EPWMPinMuxSetup();
We should always stick to either underscores or CamelCase, but not mix both. I like underscores better.
> +void pwmss_tbclk_enable(unsigned int instance)
> + switch(instance)
> + {
> +
Please remove unneeded whitespace.
> + case 0:
> + REG(AM335X_PADCONF_BASE + CONTROL_PWMSS_CTRL) |=
> + BBBIO_PWMSS_CTRL_PWMSS0_TBCLKEN;
> + break;
> +
> + case 1:
> + REG(AM335X_PADCONF_BASE + CONTROL_PWMSS_CTRL) |=
> + BBBIO_PWMSS_CTRL_PWMSS1_TBCLKEN;
> + break;
> +
> + case 2:
> + REG(AM335X_PADCONF_BASE + CONTROL_PWMSS_CTRL) |=
> + BBBIO_PWMSS_CTRL_PWMSS2_TBCLKEN;
> + break;
> +
This could be improved a bit:
uint32_t enable_bit;
bool is_valid = true;
if (instance == PWMSS0)
{
enable_bit = BBBIO_PWMSS_CTRL_PWMSS0_TBCLKEN;
}
else if (instance == PWMSS1)
{
enable_bit = BBBIO_PWMSS_CTRL_PWMSS1_TBCLKEN;
}
else if (instance == PWMSS2)
{
enable_bit = BBBIO_PWMSS_CTRL_PWMSS2_TBCLKEN;
}
else
{
is_valid = false;
}
if (is_valid)
{
REG(AM335X_PADCONF_BASE + CONTROL_PWMSS_CTRL) |= enable_bit;
}
return is_valid;
> + default:
> + break;
We should check if the instance we were passed is valid, as I did above.
> +void EPWMPinMuxSetup(void)
> +{
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_GPMC_AD(9)) = BBB_MUXMODE(4);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_GPMC_AD(8)) = BBB_MUXMODE(4);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_LCD_DATA(0)) = BBB_MUXMODE(3);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_LCD_DATA(1)) = BBB_MUXMODE(3);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_LCD_DATA(11)) = BBB_MUXMODE(2);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_LCD_DATA(10)) = BBB_MUXMODE(2);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_GPMC_AD(2)) = BBB_MUXMODE(6);
> +
> + REG(AM335X_PADCONF_BASE + CONTROL_CONF_GPMC_AD(3)) = BBB_MUXMODE(6);
> +
> + REG(AM335X_PADCONF_BASE + AM335X_CONF_SPI0_D0) = BBB_MUXMODE(3);
> +
> + REG(AM335X_PADCONF_BASE + AM335X_CONF_SPI0_SCLK) = BBB_MUXMODE(3);
> +
> + REG(AM335X_PADCONF_BASE + AM335X_CONF_MCASP0_FSX) = BBB_MUXMODE(1);
> +
> + REG(AM335X_PADCONF_BASE + AM335X_CONF_MCASP0_ACLKX) = BBB_MUXMODE(1);
> +}
Is this function changing all the muxed pins to PWM, or something like that?
That's wrong; we should only touch the pins that the user requests, otherwise we'll
be breaking any configurations he may have. Also, the arguments to BBB_MUXMODE shouldn't
be magic numbers.
> +void EPWM_clock_enable(unsigned int baseAdd)
See my comments for pwm_init.
> +
> +/**
> + * \brief This function configures the L3 and L4_PER system clocks.
> + * It also configures the system clocks for the specified ePWMSS
> + * instance.
This comment is inaccurate; we only configure the EPWM clocks here.
> +void module_clk_config(unsigned int instanceNum)
See my comments for pwm_init.
> +{
> + if(0 == instanceNum)
This is a confusing idiom; I'd rather avoid it. Also, space between the if and the opening
parentheses.
> + {
> + REG(BBBIO_CM_PER_ADDR + CM_PER_EPWMSS0_CLKCTRL) |=
> + CM_PER_EPWMSS0_CLKCTRL_MODULEMODE_ENABLE;
> +
> +
Trim unnecessary whitespace.
> + else
> + {
> +
> + }
Instead of an empty else, let the user know they tried to select an invalid instance.
> + * \brief This API enables the particular PWM module.
This is a bit vague. You should specify what initializations we're making.
> + * \param baseAddr Base Address of the PWM Module Registers.
Again, this is unsafe. We should check that this address is valid.
> +void ehrPWM_Enable(unsigned int baseAddr)
> +{
> + REG16(baseAddr + EHRPWM_AQCTLA) = 0x2 | (0x3 << 4);
> + REG16(baseAddr + EHRPWM_AQCTLB) = 0x2 | (0x3 << 8);
> + REG16(baseAddr + EHRPWM_TBCNT) = 0;
Please avoid magic numbers.
> +
> +/* PWMSS setting
> + * set pulse argument of epwm module
We need a way more detailed comment about this. Given that you're familiar with how these values work,
you should document this thoroughly.
> +int PWMSS_Setting(unsigned int baseAddr, float HZ, float dutyA, float dutyB)
> +{
> + unsigned int z,p,y;
> + int param_error =1;
> + if(HZ < 0)
> + param_error =0;
> + if(dutyA < 0.0f || dutyA > 100.0f || dutyB < 0.0f || dutyB > 100.0f)
> + param_error = 0;
> + if(param_error == 0) {
> + printf("ERROR in parameter \n");
> + }
This function definitely needs some formatting and refactoring. "HZ" isn't a good variable name; it should
be something like pwmFreq. Also, we should avoid using printf in driver code, especially if it's just debugging
messages.
> + dutyA /= 100.0f;
> + dutyB /= 100.0f;
> +
> + /*Compute necessary TBPRD*/
> + float Cyclens = 0.0f;
> + float Divisor =0;
> + int i,j;
> + 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};
These values should be documented.
> + int NearCLKDIV =7;
> + int NearHSPCLKDIV =7;
> + int NearTBPRD =0;
> +
> + Cyclens = 1000000000.0f / HZ; /** 10^9 /Hz compute time per cycle (ns)
> + */
> + Divisor = (Cyclens / 655350.0f); /** am335x provide (128* 14) divider,
> + * and per TBPRD means 10ns when divider
> + * and max TBPRD is 65535 so max cycle
> + * is 128 * 8 * 14 * 65535 * 10ns
> + */
This kind of commenting is ugly. Please move it to the line above.
> + if(Divisor > (128 * 14)) {
> + printf("Can't generate %f HZ",HZ);
> + return 0;
> + }
> + 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 awfully complex, and hard to read. Please refactor it.
> + // configure_tbclk(baseAddr, HZ);
Please remove commented code, if you won't need it.
> +
> +int PWMSS_TB_clock_check(unsigned int PWMSS_ID)
> +{
> + unsigned int reg_value,value;
> +
> + /*control module check*/
> + reg_value = REG(BBBIO_CONTROL_MODULE + BBBIO_PWMSS_CTRL);
> +
> + value = reg_value & (1 << PWMSS_ID);
> + printf("\n PWMSS_CTRL = %d and reg_value = %d \n",value,reg_value);
> + return (reg_value & (1 << PWMSS_ID));
> +}
This function could be removed altogether.
> diff --git a/c/src/lib/libbsp/shared/include/gpio.h b/c/src/lib/libbsp/shared/include/gpio.h
> index 7d8f67b..2a89f1d 100644
> --- a/c/src/lib/libbsp/shared/include/gpio.h
> +++ b/c/src/lib/libbsp/shared/include/gpio.h
> @@ -947,6 +947,17 @@ extern rtems_status_code rtems_gpio_bsp_disable_interrupt(
>
Please avoid adding BSP-specific code to shared headers.
> +/* Added by punit vara
No need for this comment. We can track it back using git.
Please make sure none of these defines come from StarterWare. Also, I'm seeing some repeated
defines (e.g. SOC_PWMSS0_REGS and PWMSS0_MMAP_ADDR). Please give this a sweep and remove anything
that's redundant.
> +//#define EHRPWM_SW_FORCED_SYNC 0x1
Please remove commented code.
More information about the devel
mailing list