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 <[email protected]> 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. _______________________________________________ devel mailing list [email protected] http://lists.rtems.org/mailman/listinfo/devel
