| Message ID | 20200914104352.2165818-1-drew@beagleboard.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 | expand |
On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote: > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > + > > +When #pinctrl-cells = 2, then setting a pin for a device could be done with: > > + > > + pinctrl-single,pins = <0xdc 0x30 0x07>; > > + > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value. > > +See the device example and static board pins example below for more information. > > Pin configuration and mux mode don't mean anything in pinctrl-single. > On another machine, mux mode might not be programmed this way or even > exist. Or the location of bits would probably be different, and this > would seem to imply the 0x07 would get shifted to the correct location > for where the pin mux setting was on that machine's pinctrl registers. > > It seems like it would be better to explain the values are ORed together. I descirbed it as seoerate values as I did not want to prescribe what the pcs driver would do with those values. But, yes, it is a just an OR operation, so I could change the language to reflect tat. > What is the purpose of this change anyway? It seems like in the end > it just does what it did before. The data is now split into two cells > in the device tree, but why? These changes were a result of desire to seperate pinconf and pinmux. Tony raised the idea in a thread at the end of May [1]. Tony wrote: > Only slightly related, but we should really eventually move omaps to use > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately > from the mux mode. We already treat them separately with the new > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to > use updated #pinctrl-cells. But I think pinctrl-single might need some > changes before we can do that. thanks, drew [1] https://lore.kernel.org/linux-omap/20200527165122.GL37466@atomide.com/
On Thu, Sep 17, 2020 at 03:00:36AM -0700, Trent Piepho wrote: > On Thu, Sep 17, 2020 at 2:20 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote: > > > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > > > > > + > > > > +When #pinctrl-cells = 2, then setting a pin for a device could be done with: > > > > + > > > > + pinctrl-single,pins = <0xdc 0x30 0x07>; > > > > + > > > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value. > > > > +See the device example and static board pins example below for more information. > > > > > > Pin configuration and mux mode don't mean anything in pinctrl-single. > > > On another machine, mux mode might not be programmed this way or even > > > exist. Or the location of bits would probably be different, and this > > > would seem to imply the 0x07 would get shifted to the correct location > > > for where the pin mux setting was on that machine's pinctrl registers. > > > > > > It seems like it would be better to explain the values are ORed together. > > > > I descirbed it as seoerate values as I did not want to prescribe what > > the pcs driver would do with those values. But, yes, it is a just an OR > > operation, so I could change the language to reflect tat. > > If you don't say what the pinctrl-single driver does with the values, > how would anyone know how to use it? > > > > What is the purpose of this change anyway? It seems like in the end > > > it just does what it did before. The data is now split into two cells > > > in the device tree, but why? > > > > These changes were a result of desire to seperate pinconf and pinmux. > > Tony raised the idea in a thread at the end of May [1]. > > > > Tony wrote: > > > Only slightly related, but we should really eventually move omaps to use > > > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately > > > from the mux mode. We already treat them separately with the new > > > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to > > > use updated #pinctrl-cells. But I think pinctrl-single might need some > > > changes before we can do that. > > I still don't see what the goal is here. Support generic pinconf? My interest is came out of my desire to turn on generic pinconf for AM3358 and I had to fix a bug that was breaking compatible "pinconf,single": f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") > Also note that while AM33XX_PADCONF() is changed, there is an in tree > board that doesn't use it, so it's broken now. I found this change > when it broke my out of tree board, due to the dtsi change not being > reflected in my board's pinctrl values. Thanks, that is a good point that arch/arm/boot/dts/am335x-guardian.dts needs to be converted from AM33XX_IOPAD to AM33XX_PADCONF. I'll submit a patch for that. Regarding AM33XX_PADCONF() restructuring, the change to have seperate arguments for direction and mux in AM33XX_PADCONF() predates my invovlement, so I've CC'd Christina Quast. commit f1ff9be7652b716c7eea67c9ca795027d911f148 Author: Christina Quast <cquast@hanoverdisplays.com> Date: Mon Apr 8 10:01:51 2019 -0700 ARM: dts: am33xx: Added AM33XX_PADCONF macro AM33XX_PADCONF takes three instead of two parameters, to make future changes to #pinctrl-cells easier. For old boards which are not mainlined, we left the AM33XX_IOPAD macro. Signed-off-by: Christina Quast <cquast@hanoverdisplays.com> Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Tony Lindgren <tony@atomide.com> Hopefully, Tony can also chime in. -Drew
* Drew Fustini <drew@beagleboard.org> [200917 10:39]: > On Thu, Sep 17, 2020 at 03:00:36AM -0700, Trent Piepho wrote: > > On Thu, Sep 17, 2020 at 2:20 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > > > On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote: > > > > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > > > > > > > + > > > > > +When #pinctrl-cells = 2, then setting a pin for a device could be done with: > > > > > + > > > > > + pinctrl-single,pins = <0xdc 0x30 0x07>; > > > > > + > > > > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value. > > > > > +See the device example and static board pins example below for more information. > > > > > > > > Pin configuration and mux mode don't mean anything in pinctrl-single. > > > > On another machine, mux mode might not be programmed this way or even > > > > exist. Or the location of bits would probably be different, and this > > > > would seem to imply the 0x07 would get shifted to the correct location > > > > for where the pin mux setting was on that machine's pinctrl registers. > > > > > > > > It seems like it would be better to explain the values are ORed together. > > > > > > I descirbed it as seoerate values as I did not want to prescribe what > > > the pcs driver would do with those values. But, yes, it is a just an OR > > > operation, so I could change the language to reflect tat. > > > > If you don't say what the pinctrl-single driver does with the values, > > how would anyone know how to use it? > > > > > > What is the purpose of this change anyway? It seems like in the end > > > > it just does what it did before. The data is now split into two cells > > > > in the device tree, but why? > > > > > > These changes were a result of desire to seperate pinconf and pinmux. > > > Tony raised the idea in a thread at the end of May [1]. > > > > > > Tony wrote: > > > > Only slightly related, but we should really eventually move omaps to use > > > > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately > > > > from the mux mode. We already treat them separately with the new > > > > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to > > > > use updated #pinctrl-cells. But I think pinctrl-single might need some > > > > changes before we can do that. > > > > I still don't see what the goal is here. Support generic pinconf? > > My interest is came out of my desire to turn on generic pinconf for AM3358 > and I had to fix a bug that was breaking compatible "pinconf,single": > f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value") > > > Also note that while AM33XX_PADCONF() is changed, there is an in tree > > board that doesn't use it, so it's broken now. I found this change > > when it broke my out of tree board, due to the dtsi change not being > > reflected in my board's pinctrl values. > > Thanks, that is a good point that arch/arm/boot/dts/am335x-guardian.dts > needs to be converted from AM33XX_IOPAD to AM33XX_PADCONF. I'll submit > a patch for that. > > Regarding AM33XX_PADCONF() restructuring, the change to have seperate > arguments for direction and mux in AM33XX_PADCONF() predates my > invovlement, so I've CC'd Christina Quast. > > commit f1ff9be7652b716c7eea67c9ca795027d911f148 > Author: Christina Quast <cquast@hanoverdisplays.com> > Date: Mon Apr 8 10:01:51 2019 -0700 > > ARM: dts: am33xx: Added AM33XX_PADCONF macro > > AM33XX_PADCONF takes three instead of two parameters, to make > future changes to #pinctrl-cells easier. > > For old boards which are not mainlined, we left the AM33XX_IOPAD > macro. > > Signed-off-by: Christina Quast <cquast@hanoverdisplays.com> > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > > Hopefully, Tony can also chime in. Also FYI, folks have also complained for a long time that the pinctrl-single binding mixes mux and conf values while they should be handled separately. Regards, Tony
On Wed, Sep 23, 2020 at 10:43 PM Tony Lindgren <tony@atomide.com> wrote: > > * Trent Piepho <tpiepho@gmail.com> [200924 01:34]: > > On Tue, Sep 22, 2020 at 11:57 PM Tony Lindgren <tony@atomide.com> wrote: > > > > > > Also FYI, folks have also complained for a long time that the pinctrl-single > > > binding mixes mux and conf values while they should be handled separately. > > > > > > > Instead of combining two fields when the dts is generated they are now > > combined when the pinctrl-single driver reads the dts. Other than > > this detail, the result is the same. The board dts source is the > > same. The value programmed into the pinctrl register is the same. > > There is no mechanism currently that can alter that value in any way. > > > > What does combining them later allow that is not possible now? > > It now allows further driver changes to manage conf and mux separately :) The pinctrl-single driver? How will that work with boards that are not am335x and don't use conf and mux fields in the same manner as am335x?
* Trent Piepho <tpiepho@gmail.com> [200930 09:34]: > On Wed, Sep 30, 2020 at 2:15 AM Tony Lindgren <tony@atomide.com> wrote: > > > > * Trent Piepho <tpiepho@gmail.com> [200930 08:35]: > > > The closest thing would be the generic pin config type bindings, which > > > go in the pinctrl driver's nodes, and look like this: > > > &am335x_pinmux { > > > pinctrl_yoyo_reset: yoyogrp { > > > pins = "foo"; > > > function = "gpio"; > > > bias-pull-up; > > > }; > > > }; > > > > There's a bit of a dtb size and boot time issue for adding properties > > for each pin where that needs to be done for several hundred pins :) > > pins is list, multiple pins can be specified at once. Otherwise the > property name would be "pin" and not "pins" There's also a groups > property to refer to multiple pins at once, e.g. > > arch/mips/boot/dts/ingenic/ci20.dts- pins_mmc1: mmc1 { > arch/mips/boot/dts/ingenic/ci20.dts- function = "mmc1"; > arch/mips/boot/dts/ingenic/ci20.dts: groups = > "mmc1-1bit-d", "mmc1-4bit-d"; > arch/mips/boot/dts/ingenic/ci20.dts- bias-disable; > arch/mips/boot/dts/ingenic/ci20.dts- }; > > arch/mips/boot/dts/pic32/pic32mzda_sk.dts- user_leds_s0: user_leds_s0 { > arch/mips/boot/dts/pic32/pic32mzda_sk.dts: pins = "H0", "H1", "H2"; > arch/mips/boot/dts/pic32/pic32mzda_sk.dts- output-low; > arch/mips/boot/dts/pic32/pic32mzda_sk.dts- microchip,digital; > arch/mips/boot/dts/pic32/pic32mzda_sk.dts- }; Right. > > > Is "some additional property for specifying generic conf flags" > > > different from the existing pinctrl-single,bias-pullup, etc. > > > properties? Because splitting the data cell into two parts doesn't > > > make any difference to those. > > > > So with an interrupt style binding with generic pinconf flags we can > > leave out the parsing of multiple properties for each pin. Sure the > > pin is only referenced by the controller like you pointed out but the > > pinconf flags could be generic. > > Where do these flags go? In pinctrl-single,pins? Like: > > pinctrl-single,pins = <AM335X_PIN_MDC MUX_MODE7 PIN_INPUT_PULLUP>; > > But PIN_INPUT_PULLUP is a generic flag? Which is translated into the > proper value by?? Yes that's what I was thinking, something like this with generic flags: pinctrl-single,pins = <AM335X_PIN_MDC (PIN_INPUT | PIN_PULLUP) MUX_MODE7>; > Or are you talking about replacing the existing pinctrl-0, > pinctrl-names properties with a totally different system that looks > more like gpio and interrupt handles? That would be even better :) Might be just too much to deal with.. In any case the parser code could already be generic if we had generic flags based on #pinctrl-cells. Regards, Tony
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt index ba428d345a56..ef560afdd52e 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt @@ -96,16 +96,22 @@ pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt document in this directory. The pin configuration nodes for pinctrl-single are specified as pinctrl -register offset and value pairs using pinctrl-single,pins. Only the bits -specified in pinctrl-single,function-mask are updated. For example, setting -a pin for a device could be done with: +register offset and values using pinctrl-single,pins. Only the bits specified +in pinctrl-single,function-mask are updated. + +When #pinctrl-cells = 1, then setting a pin for a device could be done with: pinctrl-single,pins = <0xdc 0x118>; -Where 0xdc is the offset from the pinctrl register base address for the -device pinctrl register, and 0x118 contains the desired value of the -pinctrl register. See the device example and static board pins example -below for more information. +Where 0xdc is the offset from the pinctrl register base address for the device +pinctrl register, and 0x118 contains the desired value of the pinctrl register. + +When #pinctrl-cells = 2, then setting a pin for a device could be done with: + + pinctrl-single,pins = <0xdc 0x30 0x07>; + +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value. +See the device example and static board pins example below for more information. In case when one register changes more than one pin's mux the pinctrl-single,bits need to be used which takes three parameters:
Document the values in pinctrl-single,pins when #pinctrl-cells = <2> Fixes: 27c90e5e48d0 ("ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2") Reported-by: Trent Piepho <tpiepho@gmail.com> Link: https://lore.kernel.org/linux-omap/3139716.CMS8C0sQ7x@zen.local/ Signed-off-by: Drew Fustini <drew@beagleboard.org> --- .../bindings/pinctrl/pinctrl-single.txt | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)