mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Peter A. Bigot]

While converting some old DTS bindings, I noticed that the Linux 
include/dt-bindings/pinctrl/am33xx.h and am43xx.h headers both define 
SLEWCTRL_FAST constants:

include/dt-bindings/pinctrl/am33xx.h:#define SLEWCTRL_FAST              
(1 << 6)
include/dt-bindings/pinctrl/am43xx.h:#define SLEWCTRL_FAST              
(1 << 19)

According to the TRM for these processors, the effect of setting that 
bit is to select slow slew; fast would be selected by leaving it 
cleared.  The constants should therefore be named SLEWCTRL_SLOW.

This is consistent with the value for the I2C binding constants I'm 
converting:

beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi: 0x158 
0x72      /* spi0_d1.i2c1_sda, SLEWCTRL_SLOW | INPUT_PULLUP | MODE2 */
beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi: 0x15c 
0x72      /* spi0_cs0.i2c1_scl, SLEWCTRL_SLOW | INPUT_PULLUP | MODE2 */

except that in the official files we have something like:

stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x188 
(PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /* i2c0_sda.i2c0_sda */
stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x18c 
(PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /* i2c0_scl.i2c0_scl */

which is either wrong or misleading.

Peter

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Peter A. Bigot]

Any comment on this?  I have a patch, but figured it'd be better for TI 
to provide an official solution.  Or is there another forum it should be 
raised in?

It's really misleading for the self-documenting device tree 
specifications to lie about what sort of slew is being configured.

Peter

On 09/06/2014 06:17 PM, Peter A. Bigot wrote:
> While converting some old DTS bindings, I noticed that the Linux 
> include/dt-bindings/pinctrl/am33xx.h and am43xx.h headers both define 
> SLEWCTRL_FAST constants:
>
> include/dt-bindings/pinctrl/am33xx.h:#define 
> SLEWCTRL_FAST              (1 << 6)
> include/dt-bindings/pinctrl/am43xx.h:#define 
> SLEWCTRL_FAST              (1 << 19)
>
> According to the TRM for these processors, the effect of setting that 
> bit is to select slow slew; fast would be selected by leaving it 
> cleared.  The constants should therefore be named SLEWCTRL_SLOW.
>
> This is consistent with the value for the I2C binding constants I'm 
> converting:
>
> beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi: 
> 0x158 0x72      /* spi0_d1.i2c1_sda, SLEWCTRL_SLOW | INPUT_PULLUP | 
> MODE2 */
> beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi: 
> 0x15c 0x72      /* spi0_cs0.i2c1_scl, SLEWCTRL_SLOW | INPUT_PULLUP | 
> MODE2 */
>
> except that in the official files we have something like:
>
> stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x188 
> (PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /* i2c0_sda.i2c0_sda */
> stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x18c 
> (PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /* i2c0_scl.i2c0_scl */
>
> which is either wrong or misleading.
>
> Peter

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Denys Dmytriyenko]

Wasn't this already fixed upstream?

On Wed, Sep 17, 2014 at 07:17:03AM -0500, Peter A. Bigot wrote:
> Any comment on this?  I have a patch, but figured it'd be better for
> TI to provide an official solution.  Or is there another forum it
> should be raised in?
> 
> It's really misleading for the self-documenting device tree
> specifications to lie about what sort of slew is being configured.
> 
> Peter
> 
> On 09/06/2014 06:17 PM, Peter A. Bigot wrote:
> >While converting some old DTS bindings, I noticed that the Linux
> >include/dt-bindings/pinctrl/am33xx.h and am43xx.h headers both
> >define SLEWCTRL_FAST constants:
> >
> >include/dt-bindings/pinctrl/am33xx.h:#define SLEWCTRL_FAST
> >(1 << 6)
> >include/dt-bindings/pinctrl/am43xx.h:#define SLEWCTRL_FAST
> >(1 << 19)
> >
> >According to the TRM for these processors, the effect of setting
> >that bit is to select slow slew; fast would be selected by leaving
> >it cleared.  The constants should therefore be named
> >SLEWCTRL_SLOW.
> >
> >This is consistent with the value for the I2C binding constants
> >I'm converting:
> >
> >beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi:
> >0x158 0x72      /* spi0_d1.i2c1_sda, SLEWCTRL_SLOW | INPUT_PULLUP
> >| MODE2 */
> >beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi:
> >0x15c 0x72      /* spi0_cs0.i2c1_scl, SLEWCTRL_SLOW | INPUT_PULLUP
> >| MODE2 */
> >
> >except that in the official files we have something like:
> >
> >stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x188
> >(PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /*
> >i2c0_sda.i2c0_sda */
> >stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x18c
> >(PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /*
> >i2c0_scl.i2c0_scl */
> >
> >which is either wrong or misleading.
> >
> >Peter
> 
> -- 
> _______________________________________________
> meta-ti mailing list
> meta-ti@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/meta-ti
> 

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Peter A. Bigot]

Depends what "upstream" means.  It's not in 3.17 released 05 Oct, nor in 
any other non-TI repository I can find.  (Might be hidden in a TI 
repository somewhere that I'm not looking.)

Peter

On 10/07/2014 03:58 PM, Denys Dmytriyenko wrote:
> Wasn't this already fixed upstream?
>
> On Wed, Sep 17, 2014 at 07:17:03AM -0500, Peter A. Bigot wrote:
>> Any comment on this?  I have a patch, but figured it'd be better for
>> TI to provide an official solution.  Or is there another forum it
>> should be raised in?
>>
>> It's really misleading for the self-documenting device tree
>> specifications to lie about what sort of slew is being configured.
>>
>> Peter
>>
>> On 09/06/2014 06:17 PM, Peter A. Bigot wrote:
>>> While converting some old DTS bindings, I noticed that the Linux
>>> include/dt-bindings/pinctrl/am33xx.h and am43xx.h headers both
>>> define SLEWCTRL_FAST constants:
>>>
>>> include/dt-bindings/pinctrl/am33xx.h:#define SLEWCTRL_FAST
>>> (1 << 6)
>>> include/dt-bindings/pinctrl/am43xx.h:#define SLEWCTRL_FAST
>>> (1 << 19)
>>>
>>> According to the TRM for these processors, the effect of setting
>>> that bit is to select slow slew; fast would be selected by leaving
>>> it cleared.  The constants should therefore be named
>>> SLEWCTRL_SLOW.
>>>
>>> This is consistent with the value for the I2C binding constants
>>> I'm converting:
>>>
>>> beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi:
>>> 0x158 0x72      /* spi0_d1.i2c1_sda, SLEWCTRL_SLOW | INPUT_PULLUP
>>> | MODE2 */
>>> beagleboard/3.14:arch/arm/boot/dts/am335x-bone-common-pinmux.dtsi:
>>> 0x15c 0x72      /* spi0_cs0.i2c1_scl, SLEWCTRL_SLOW | INPUT_PULLUP
>>> | MODE2 */
>>>
>>> except that in the official files we have something like:
>>>
>>> stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x188
>>> (PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /*
>>> i2c0_sda.i2c0_sda */
>>> stable/linux-3.14.y:arch/arm/boot/dts/am43x-epos-evm.dts: 0x18c
>>> (PIN_INPUT_PULLUP | SLEWCTRL_FAST | MUX_MODE0)    /*
>>> i2c0_scl.i2c0_scl */
>>>
>>> which is either wrong or misleading.
>>>
>>> Peter
>> -- 
>> _______________________________________________
>> meta-ti mailing list
>> meta-ti@yoctoproject.org
>> https://lists.yoctoproject.org/listinfo/meta-ti
>>

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Peter A. Bigot]

On 12/26/2014 09:49 AM, Christian d'Heureuse wrote:
> Hi Peter
>
> I stumbled on the same error in am33xx.h as you described in September:
> https://lists.yoctoproject.org/pipermail/meta-ti/2014-September/005211.html 
>
>
> Did you get any reactions?

Just the follow-up theorizing it'd been fixed upstream.  As of today, 
it's still wrong in both am33xx.h and am43xx.h when "upstream" means the 
torvalds master branch.

I keep hoping a TIer will step up, but if not I suppose somebody else 
has to do it.

Maybe E2E will yield results. 
http://e2e.ti.com/support/arm/sitara_arm/f/791/p/367445/1382229#1382229

Peter

>
> The error is still in the current Linux source:
>
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/dt-bindings/pinctrl/am33xx.h 
>
>
> #define SLEWCTRL_FAST           (1 << 6)
>
> In the old Rowboat kernel, the definition was correct:
>
> https://gitorious.org/rowboat/kernel/source/03f2a1e9ea7f8b11ff6809f5a75b614011f8adb6:arch/arm/mach-omap2/mux.h 
>
>
> #define AM33XX_SLEWCTRL_FAST            (0 << 6)
> #define AM33XX_SLEWCTRL_SLOW            (1 << 6)
>
> An in the AM335X Technical Reference Manual
> http://www.ti.com/lit/ug/spruh73k/spruh73k.pdf
> "slewctrl" bit:
> "9.2.2 Pad Control Registers"
> "9.3.1.50 conf_<module>_<pin> Register"
>
> According to 
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/log/include/dt-bindings/pinctrl/am33xx.h 
> the author is
> Florian Vaussard <florian.vaussard@epfl.ch> and the committer is 
> Benoit Cousson <benoit.cousson@linaro.org>. Maybe we could write one 
> of them?
>
> Christian
> http://www.source-code.biz  http://www.inventec.ch/chdh

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Denys Dmytriyenko]

On Fri, Dec 26, 2014 at 10:29:39AM -0600, Peter A. Bigot wrote:
> On 12/26/2014 09:49 AM, Christian d'Heureuse wrote:
> >Hi Peter
> >
> >I stumbled on the same error in am33xx.h as you described in September:
> >https://lists.yoctoproject.org/pipermail/meta-ti/2014-September/005211.html
> >
> >
> >Did you get any reactions?
> 
> Just the follow-up theorizing it'd been fixed upstream.  As of
> today, it's still wrong in both am33xx.h and am43xx.h when
> "upstream" means the torvalds master branch.
> 
> I keep hoping a TIer will step up, but if not I suppose somebody
> else has to do it.
> 
> Maybe E2E will yield results. http://e2e.ti.com/support/arm/sitara_arm/f/791/p/367445/1382229#1382229


Peter,

For the record, here are 2 replies from one of our kernel developers (Cc-ed) 
that I received for my inquiries on your question back in September, stating 
it's in upstream, after which I closed it:


> 1. Should rather be SLEWCONTROL like DRA7 rather than _FAST or _SLOW. that 
> avoids all kind of messed up translation.
> 
> OR should define both _FAST and SLOW for all SoCs equally..


> 2. Even better - if the change - like this one is already present in 
> upstream kernel(which in this case is actually in upstream) - ask the public 
> person to directly ask on linux-omap mailing list (public).


Since meta-ti is not specifically a kernel mailing list, there's less chance 
to get it resolved through here. Please use corresponding upstream mailing 
lists - linux-omap@vger.kernel.org is the closest one here, or either 
devicetree@vger.kernel.org or linux-soc@vger.kernel.org or even E2E forums. 

You wouldn't send this to Debian, Ubuntu or even Buildroot mailing lists, 
would you? Similarly, this list is for OpenEmbedded/Yocto layer - the best I 
can do is forward questions and answers back and forth, which is not very 
efficient. So, reporting this on one of the kernel lists would have yelded 
better results, I'd say, since that's where kernel devs are hanging out - 
they don't usually frequent Yocto lists, unfortunately.

Best regards,

-- 
Denys


> >The error is still in the current Linux source:
> >
> >https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/dt-bindings/pinctrl/am33xx.h
> >
> >
> >#define SLEWCTRL_FAST           (1 << 6)
> >
> >In the old Rowboat kernel, the definition was correct:
> >
> >https://gitorious.org/rowboat/kernel/source/03f2a1e9ea7f8b11ff6809f5a75b614011f8adb6:arch/arm/mach-omap2/mux.h
> >
> >
> >#define AM33XX_SLEWCTRL_FAST            (0 << 6)
> >#define AM33XX_SLEWCTRL_SLOW            (1 << 6)
> >
> >An in the AM335X Technical Reference Manual
> >http://www.ti.com/lit/ug/spruh73k/spruh73k.pdf
> >"slewctrl" bit:
> >"9.2.2 Pad Control Registers"
> >"9.3.1.50 conf_<module>_<pin> Register"
> >
> >According to https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/log/include/dt-bindings/pinctrl/am33xx.h
> >the author is
> >Florian Vaussard <florian.vaussard@epfl.ch> and the committer is
> >Benoit Cousson <benoit.cousson@linaro.org>. Maybe we could write
> >one of them?
> >
> >Christian
> >http://www.source-code.biz  http://www.inventec.ch/chdh
> 
> -- 
> _______________________________________________
> meta-ti mailing list
> meta-ti@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/meta-ti

Re: mis-definition of SLEWCTRL_FAST in dt-bindings amxxxx header

[Peter A. Bigot]

On 01/09/2015 03:46 PM, Denys Dmytriyenko wrote:
> On Fri, Dec 26, 2014 at 10:29:39AM -0600, Peter A. Bigot wrote:
>> On 12/26/2014 09:49 AM, Christian d'Heureuse wrote:
>>> Hi Peter
>>>
>>> I stumbled on the same error in am33xx.h as you described in September:
>>> https://lists.yoctoproject.org/pipermail/meta-ti/2014-September/005211.html
>>>
>>>
>>> Did you get any reactions?
>> Just the follow-up theorizing it'd been fixed upstream.  As of
>> today, it's still wrong in both am33xx.h and am43xx.h when
>> "upstream" means the torvalds master branch.
>>
>> I keep hoping a TIer will step up, but if not I suppose somebody
>> else has to do it.
>>
>> Maybe E2E will yield results. http://e2e.ti.com/support/arm/sitara_arm/f/791/p/367445/1382229#1382229
>
> Peter,
>
> For the record, here are 2 replies from one of our kernel developers (Cc-ed)
> that I received for my inquiries on your question back in September, stating
> it's in upstream, after which I closed it:
>
>
>> 1. Should rather be SLEWCONTROL like DRA7 rather than _FAST or _SLOW. that
>> avoids all kind of messed up translation.
>>
>> OR should define both _FAST and SLOW for all SoCs equally..
>
>> 2. Even better - if the change - like this one is already present in
>> upstream kernel(which in this case is actually in upstream) - ask the public
>> person to directly ask on linux-omap mailing list (public).
>
> Since meta-ti is not specifically a kernel mailing list, there's less chance
> to get it resolved through here. Please use corresponding upstream mailing
> lists - linux-omap@vger.kernel.org is the closest one here, or either
> devicetree@vger.kernel.org or linux-soc@vger.kernel.org or even E2E forums.
>
> You wouldn't send this to Debian, Ubuntu or even Buildroot mailing lists,
> would you? Similarly, this list is for OpenEmbedded/Yocto layer - the best I
> can do is forward questions and answers back and forth, which is not very
> efficient. So, reporting this on one of the kernel lists would have yelded
> better results, I'd say, since that's where kernel devs are hanging out -
> they don't usually frequent Yocto lists, unfortunately.

OK.  I understand it's out of scope for you and you're frustrated too; 
thanks for trying.  There are enough people aware of it now who should 
have some stake in getting it fixed that I'm not going to search out 
other places to report it.

Peter