[PATCH] Fix for Beaglebone BSP PWM bug

[Christian Mauderer]

On 13/07/2020 11:55, James Fitzsimons wrote:
> Hi Christian and Chris,
> 
> On Sun, 12 Jul 2020 at 06:27, Christian Mauderer <oss at c-mauderer.de
> <mailto:oss at c-mauderer.de>> wrote:
> 
>     Hello Fitzsimons,
> 
>     sorry for the late review and thanks Chris for poking me.
> 
>     I have some troubles understanding the patch and the current code:
> 
>     == Regarding your patch:
> 
>     There is no pin GPMC_AD(18) or GPMC_AD(19). That offset will lead you to
>     the pins GPMC_A8 and GPMC_A9. The correct macros to use would be
>     AM335X_CONF_GPMC_A8 and AM335X_CONF_GPMC_A9. But that's only a style
>     topic.
> 
>     What's a bit odd is that GPMC_A8 seems to be connected to the LED USR3
>     on the BBB. Reasonable output for a PWM. But GPMC_A9 is a HDMI_INT,
>     which seems a bit odd
> 
> 
> This is my fault for not really understanding the RTEMS header structure
> for the beaglebone bsp properly prior to now. I've dug into it a lot
> more over the last week or so while I've been working on implementing
> the eQEP driver and now have a much better understanding of how all the
> register definitions work.
> 
> To be honest I found the following two macro definitions a bit
> unnecessary but thought I was doing the right thing by following the
> established pattern,
> /**
>  * @brief  BeagleBone Black PWM Macros.
>  */
> #define BBB_CONTROL_CONF_GPMC_AD(n)   (0x800 + (n * 4))
> #define BBB_CONTROL_CONF_LCD_DATA(n)   (0x8a0 + (n * 4))
> 

The BBB headers have grown a bit. It's a cheap platform and therefore it
has been used during quite some GSoC and entry level projects. With that
the files have been written by much more authors then most other BSPs.
The headers most likely would need some clean up to get a consistent
structure.

>      
> 
>     == Regarding the old pins
> 
>     The old pins on the other hand (GPMC_AD2 and AD3) are connected to
>     MMC1_DAT2 and 3 which is on P8 pin 5 and 6. That sounds a lot more
>     reasonable. But the pins are definitively not what the pin_no suggests.
> 
>     == What (maybe) should be the correct one
> 
>     P9_14 and P9_16 are EHRPWM1A and EHRPWM1B on the beagle schematic. These
>     are GPMC_AD8 and GPMC_AD9. These pins have a PWM function as an
>     alternative function. So I would expect that you have to use these.
> 
>     It's really hard to find a documentation from TI for the pinmux but I
>     found a list:
> 
>     https://git.ti.com/cgit/sitara-dss-files/am335x-dss-files/tree/padconf/am335x-pinmux.data
> 
> 
> A much better and easier to understand resource are the following two
> documents from Derek Molloy's Exploring Beaglebone website:
> 
> http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f008.png
> http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f009.png 
> 
> As you can see from the P9 header sheet, the Address we want for
> EHRPWM1A  on pin P9_14 is 0x848, and EHRPWM1B on pin P9_16 is 0x84c. 
> 
> It seems like all the register definitions in the am335x.h header in
> RTEMS are using the pin mode 1 name as the #define. So, for  EHRPWM1A 
> on pin P9_14 I should have used AM335X_CONF_GPMC_A2, and for  EHRPWM1B
> on pin P9_16 I should have used AM335X_CONF_GPMC_A3.
> I have just made this change and tested it and it works as expected.
> I'll submit an updated patch for your review.
> 
>     Can you please double check the registers used in your patch
> 
> 
> BTW - the previous patch did actually calculate the correct register
> offset - I confirmed this both by printing the register value in a debug
> statement and by physically measuring a PWM output signal on the pins.

You are right. I'm not sure where I got my offset wrong. Of course it
should be GPMC_A2 and GPMC_A3.

Your new patch is on top of the first one (replaces ...AD(18) with
...A2). I'll squash both patches into one and push that one.

Best regards

Christian

> 
> Cheers,
> James 
>  
> 
>     Best regards
> 
>     Christian
> 
> 
>     On 05/07/2020 12:39, James Fitzsimons wrote:
>     > ---
>     > Fixed incorrect register offset values for EHRPWM1A on P9_14
>     >  and EHRPWM1B on P9_16
>     >
>     >  bsps/arm/beagle/pwm/pwm.c | 4 ++--
>     >  1 file changed, 2 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/bsps/arm/beagle/pwm/pwm.c b/bsps/arm/beagle/pwm/pwm.c
>     > index 0bc5d125bf..9a346995aa 100644
>     > --- a/bsps/arm/beagle/pwm/pwm.c
>     > +++ b/bsps/arm/beagle/pwm/pwm.c
>     > @@ -102,9 +102,9 @@ bool beagle_pwm_pinmux_setup(bbb_pwm_pin_t
>     pin_no, BBB_PWMSS pwm_id)
>     >       } else if (pin_no == BBB_P8_36_1A) {
>     >         REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_LCD_DATA(10)) =
>     BBB_MUXMODE(BBB_P8_36_MUX_PWM);
>     >       } else if (pin_no == BBB_P9_14_1A) {
>     > -       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(2)) =
>     BBB_MUXMODE(BBB_P9_14_MUX_PWM);
>     > +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(18)) =
>     BBB_MUXMODE(BBB_P9_14_MUX_PWM);
>     >       } else if (pin_no == BBB_P9_16_1B) {
>     > -       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(3)) =
>     BBB_MUXMODE(BBB_P9_16_MUX_PWM);
>     > +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(19)) =
>     BBB_MUXMODE(BBB_P9_16_MUX_PWM);
>     >       } else {
>     >         is_valid = false;
>     >          }
>     >
>