<div dir="ltr"><div dir="ltr">Hi Christian and Chris,<div><br></div><div>On Sun, 12 Jul 2020 at 06:27, Christian Mauderer <<a href="mailto:oss@c-mauderer.de">oss@c-mauderer.de</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 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</blockquote><div><br></div><div>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.</div><div><br></div><div>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,</div>/**<br> * @brief  BeagleBone Black PWM Macros.<br> */<br>#define BBB_CONTROL_CONF_GPMC_AD(n)   (0x800 + (n * 4))<br><div>#define BBB_CONTROL_CONF_LCD_DATA(n)   (0x8a0 + (n * 4))</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
== 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" target="_blank">https://git.ti.com/cgit/sitara-dss-files/am335x-dss-files/tree/padconf/am335x-pinmux.data</a></blockquote><div><br></div><div>A much better and easier to understand resource are the following two documents from Derek Molloy's Exploring Beaglebone website:</div><div><br></div><div><a href="http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f008.png">http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f008.png</a></div><div><a href="http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f009.png">http://exploringbeaglebone.com/wp-content/uploads/2019/01/533160-c06f009.png</a> </div><div><br></div><div>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. </div><div><br></div><div>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.</div><div>I have just made this change and tested it and it works as expected. I'll submit an updated patch for your review.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Can you please double check the registers used in your patch<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Cheers,</div><div>James </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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 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)) = 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)) = BBB_MUXMODE(BBB_P9_14_MUX_PWM);<br>
> +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(18)) = 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)) = BBB_MUXMODE(BBB_P9_16_MUX_PWM);<br>
> +       REG(AM335X_PADCONF_BASE + BBB_CONTROL_CONF_GPMC_AD(19)) = BBB_MUXMODE(BBB_P9_16_MUX_PWM);<br>
>       } else { <br>
>         is_valid = false;<br>
>          }<br>
> <br>
</blockquote></div></div>