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.gal...@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 <punitv...@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. _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel