Subject: Add PWM driver for beagle bone black
punit vara
punitvara at gmail.com
Wed Jun 22 03:32:03 UTC 2016
Thank you very much for detailed review. I will do each changes you
suggested. Though I have some specific doubt I would like to ask as
following.
On Wed, Jun 22, 2016 at 1:07 AM, Martin Galvan
<martin.galvan at tallertechnologies.com> wrote:
> 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.
>
I would like to confirm it once again. Should I send a patch even if
it breaks the build right ?
> 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.
Sure I will do it.
>> 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.
Right now I am not using this BASE_CLOCK but IMHO we should keep it
here so that in future maintainer can know which system clock should
be used for BBB. Sure I will link it to TRM as well.
>
>> +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.
Ok I will do it.
> 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.
Yes you are absolutely right. Yet I have a doubt. Should we provide
access to individual pins or should we divide pins into groups like
pins for PWM0 , PWM1, PWM2 ?
>> +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.
Sure I will define Constants for these 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.
Yes I will document everything in detailed. All documentation should I
do in RTEMS wiki, or README ?
>> +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.
Does RTEMS has any debug mode like
https://github.com/VegetableAvenger/BBBIOlib/blob/master/BBBio_lib/BBBiolib_PWMSS.c#L112
so that printf can be only enabled in debug mode. IMHO printf can be
handy while debugging if there is any kind of debugging mode is
available.
>> + 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};
Commenting dividers as it is in TRM would be beneficial. What do you suggest ?
> 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.
This is to check for which module clock is enabled. This helped me a
lot during debugging. Debugging functions should not be included right
?
>> 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.
Where should I add declarations of functions that I defined in bbb-pwm.c ?
>> +/* 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