<div dir="auto">Hi Christian,<div dir="auto"><br></div><div dir="auto">Thanks very much for that, much appreciated. </div><div dir="auto"><br></div><div dir="auto">Cheers, </div><div dir="auto">James </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 14 Jul 2020, 03:52 Christian Mauderer, <<a href="mailto:oss@c-mauderer.de">oss@c-mauderer.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 13/07/2020 11:55, James Fitzsimons wrote:<br>
> Hi Christian and Chris,<br>
> <br>
> On Sun, 12 Jul 2020 at 06:27, Christian Mauderer <<a href="mailto:oss@c-mauderer.de" target="_blank" rel="noreferrer">oss@c-mauderer.de</a><br>
> <mailto:<a href="mailto:oss@c-mauderer.de" target="_blank" rel="noreferrer">oss@c-mauderer.de</a>>> wrote:<br>
> <br>
>     Hello Fitzsimons,<br>
> <br>
>     sorry for the late review and thanks Chris for poking me.<br>
> <br>
>     I have some troubles understanding the patch and the current code:<br>
> <br>
>     == Regarding your patch:<br>
> <br>
>     There is no pin GPMC_AD(18) or GPMC_AD(19). That offset will lead you to<br>
>     the pins GPMC_A8 and GPMC_A9. The correct macros to use would be<br>
>     AM335X_CONF_GPMC_A8 and AM335X_CONF_GPMC_A9. But that's only a style<br>
>     topic.<br>
> <br>
>     What's a bit odd is that GPMC_A8 seems to be connected to the LED USR3<br>
>     on the BBB. Reasonable output for a PWM. But GPMC_A9 is a HDMI_INT,<br>
>     which seems a bit odd<br>
> <br>
> <br>
> This is my fault for not really understanding the RTEMS header structure<br>
> for the beaglebone bsp properly prior to now. I've dug into it a lot<br>
> more over the last week or so while I've been working on implementing<br>
> the eQEP driver and now have a much better understanding of how all the<br>
> register definitions work.<br>
> <br>
> To be honest I found the following two macro definitions a bit<br>
> unnecessary but thought I was doing the right thing by following the<br>
> established pattern,<br>
> /**<br>
>  * @brief  BeagleBone Black PWM Macros.<br>
>  */<br>
> #define BBB_CONTROL_CONF_GPMC_AD(n)   (0x800 + (n * 4))<br>
> #define BBB_CONTROL_CONF_LCD_DATA(n)   (0x8a0 + (n * 4))<br>
> <br>
<br>
The BBB headers have grown a bit. It's a cheap platform and therefore it<br>
has been used during quite some GSoC and entry level projects. With that<br>
the files have been written by much more authors then most other BSPs.<br>
The headers most likely would need some clean up to get a consistent<br>
structure.<br>
<br>
>      <br>
> <br>
>     == Regarding the old pins<br>
> <br>
>     The old pins on the other hand (GPMC_AD2 and AD3) are connected to<br>
>     MMC1_DAT2 and 3 which is on P8 pin 5 and 6. That sounds a lot more<br>
>     reasonable. But the pins are definitively not what the pin_no suggests.<br>
> <br>
>     == What (maybe) should be the correct one<br>
> <br>
>     P9_14 and P9_16 are EHRPWM1A and EHRPWM1B on the beagle schematic. These<br>
>     are GPMC_AD8 and GPMC_AD9. These pins have a PWM function as an<br>
>     alternative function. So I would expect that you have to use these.<br>
> <br>
>     It's really hard to find a documentation from TI for the pinmux but I<br>
>     found a list:<br>
> <br>
>     <a href="https://git.ti.com/cgit/sitara-dss-files/am335x-dss-files/tree/padconf/am335x-pinmux.data" rel="noreferrer noreferrer" target="_blank">https://git.ti.com/cgit/sitara-dss-files/am335x-dss-files/tree/padconf/am335x-pinmux.data</a><br>
> <br>
> <br>
> A much better and easier to understand resource are the following two<br>
> documents from Derek Molloy's Exploring Beaglebone website:<br>
> <br>
> <a href="http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f008.png" rel="noreferrer noreferrer" target="_blank">http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f008.png</a><br>
> <a href="http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f009.png" rel="noreferrer noreferrer" target="_blank">http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f009.png</a> <br>
> <br>
> As you can see from the P9 header sheet, the Address we want for<br>
> EHRPWM1A  on pin P9_14 is 0x848, and EHRPWM1B on pin P9_16 is 0x84c. <br>
> <br>
> It seems like all the register definitions in the am335x.h header in<br>
> RTEMS are using the pin mode 1 name as the #define. So, for  EHRPWM1A <br>
> on pin P9_14 I should have used AM335X_CONF_GPMC_A2, and for  EHRPWM1B<br>
> on pin P9_16 I should have used AM335X_CONF_GPMC_A3.<br>
> I have just made this change and tested it and it works as expected.<br>
> I'll submit an updated patch for your review.<br>
> <br>
>     Can you please double check the registers used in your patch<br>
> <br>
> <br>
> BTW - the previous patch did actually calculate the correct register<br>
> offset - I confirmed this both by printing the register value in a debug<br>
> statement and by physically measuring a PWM output signal on the pins.<br>
<br>
You are right. I'm not sure where I got my offset wrong. Of course it<br>
should be GPMC_A2 and GPMC_A3.<br>
<br>
Your new patch is on top of the first one (replaces ...AD(18) with<br>
...A2). I'll squash both patches into one and push that one.<br>
<br>
Best regards<br>
<br>
Christian<br>
<br>
> <br>
> Cheers,<br>
> James <br>
>  <br>
> <br>
>     Best regards<br>
> <br>
>     Christian<br>
> <br>
> <br>
>     On 05/07/2020 12:39, James Fitzsimons wrote:<br>
>     > ---<br>
>     > Fixed incorrect register offset values for EHRPWM1A on P9_14<br>
>     >  and EHRPWM1B on P9_16<br>
>     ><br>
>     >  bsps/arm/beagle/pwm/pwm.c | 4 ++--<br>
>     >  1 file changed, 2 insertions(+), 2 deletions(-)<br>
>     ><br>
>     > diff --git a/bsps/arm/beagle/pwm/pwm.c b/bsps/arm/beagle/pwm/pwm.c<br>
>     > index 0bc5d125bf..9a346995aa 100644<br>
>     > --- a/bsps/arm/beagle/pwm/pwm.c<br>
>     > +++ b/bsps/arm/beagle/pwm/pwm.c<br>
>     > @@ -102,9 +102,9 @@ bool beagle_pwm_pinmux_setup(bbb_pwm_pin_t<br>
>     pin_no, BBB_PWMSS pwm_id)<br>
>     >       } else if (pin_no == BBB_P8_36_1A) {<br>
>     >         REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_LCD_DATA(10)) =<br>
>     BBB_MUXMODE(BBB_P8_36_MUX_PWM);<br>
>     >       } else if (pin_no == BBB_P9_14_1A) {<br>
>     > -       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(2)) =<br>
>     BBB_MUXMODE(BBB_P9_14_MUX_PWM);<br>
>     > +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(18)) =<br>
>     BBB_MUXMODE(BBB_P9_14_MUX_PWM);<br>
>     >       } else if (pin_no == BBB_P9_16_1B) {<br>
>     > -       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(3)) =<br>
>     BBB_MUXMODE(BBB_P9_16_MUX_PWM);<br>
>     > +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(19)) =<br>
>     BBB_MUXMODE(BBB_P9_16_MUX_PWM);<br>
>     >       } else {<br>
>     >         is_valid = false;<br>
>     >          }<br>
>     ><br>
> <br>
</blockquote></div>