[PATCH] Fix for Beaglebone BSP PWM bug

[James Fitzsimons]

Hi Christian,

Thanks very much for that, much appreciated.

Cheers,
James

On Tue, 14 Jul 2020, 03:52 Christian Mauderer, <oss at c-mauderer.de> wrote:

>
>
> 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;
> >     >          }
> >     >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200714/aa3bdf9d/attachment-0001.html>