| 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> [200914 10:44]: > 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> Acked-by: Tony Lindgren <tony@atomide.com> > --- > .../bindings/pinctrl/pinctrl-single.txt | 20 ++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > 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: > -- > 2.25.1 >
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> [200924 05:49]: > 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? For those cases we still have #pinctrl-cells = <1>. Regards, Tony
On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren <tony@atomide.com> wrote: > > * Trent Piepho <tpiepho@gmail.com> [200924 06:31]: > > > > > > > > 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? > > > > > > For those cases we still have #pinctrl-cells = <1>. > > > > If pincntrl-single is going to be am335x specific, then shouldn't it > > be a different compatible string? > > Certainly different compatible strings can be used as needed. > But pinctrl-single is not going to be am335x specific though :) > We have quite a few SoCs using it: So what doesn't make sense to me, is to put something am335x specific like two cells for conf and mux, into a generic driver like pinctrl single. This series adds two cells ORed into one. Ok, that's generic, other platforms could use it. But it also accomplishes nothing, so what's the point? You've hinted there is more to come, which will accomplish something, but what is it? That can be: Used by platforms other than am335x Can't already be done with the pinctrl single pinconf features Needs more than one data cell per pin > > Are the driver changes something that can be not be done with the > > pinconf-single properties? They all include a mask. > > Sure but in the long term we're better off with using #pinctrl-cells > along the lines what we have for example for #interrupt-cells and > #gpio-cells. So if you look at gpio-cells, virtually every driver uses 2, where one cell is the gpio index and the other is a common set of flags. It's the standard layout of a gpio handle that almost all bindings use. Only a handful of platform specific gpio drivers have another cell to indicate bank (probably better to have made each bank its own device node). Interrupt controllers have different numbers of cells, but they are all platform specific, and the cells have defined platform specific meanings. pci-host-cam-generic is a somewhat generic interrupt controller and it uses 1 cell, since it lacks device specific fields to put into additional cells. So I don't see an example of multiple cells which do not have a defined meaning that applies to all devices using the bindings. Consider also that any future changes to the pinctrl-single bindings would need to be backward compatible with a device tree binary where two cells get combined. So if the bindings being added here aren't done, then adding them now creates an unnecessary additional version to deal with for backward compatibility.
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(-)