| Message ID | 20200914104352.2165818-1-drew@beagleboard.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 | expand |
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. 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?
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 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? 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.
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
* 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 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?
* 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 :) 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> [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 Wed, Sep 23, 2020 at 11:06 PM Tony Lindgren <tony@atomide.com> wrote: > > * 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>. If pincntrl-single is going to be am335x specific, then shouldn't it be a different compatible string? Are the driver changes something that can be not be done with the pinconf-single properties? They all include a mask.
* Trent Piepho <tpiepho@gmail.com> [200924 06:31]: > On Wed, Sep 23, 2020 at 11:06 PM Tony Lindgren <tony@atomide.com> wrote: > > > > * 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>. > > 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: $ git grep pinctrl-single,function-mask arch/*/boot/dts/ | wc -l 41 > 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. 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.
* Trent Piepho <tpiepho@gmail.com> [200929 20:16]: > On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren <tony@atomide.com> wrote: > > 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. Treating conf and mux separately is not am335x specific. Linux treats them separately because the conf options typically can be described in a generic way while the mux is just signal routing. Sure the conf values are currently not generic, but that could be done if wanted to and added some property to specify that the controller uses generic conf values. > 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 For SoCs using #pinctrl-cells = <2> we now have conf and mux values separated in the dtb. Certainly that's a better place to be compared to earlier for any further pinconf changes. > 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. With interrupts the IRQ_TYPE flags are generic and separate from the hardware specific cells. If we wanted to, we could have something similar for pinctrl framework. > 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. I don't see issues with backward compabilty. If we specify that the controller uses #pinctrl-cells = <2>, and some additional property for specifying generic conf flags, we'd have a similar generic binding to the interrupt binding. Regards, Tony
On Tue, Sep 29, 2020 at 10:15 PM Tony Lindgren <tony@atomide.com> wrote: > * Trent Piepho <tpiepho@gmail.com> [200929 20:16]: > > On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren <tony@atomide.com> wrote: > > > 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. > > Treating conf and mux separately is not am335x specific. Linux treats > them separately because the conf options typically can be described > in a generic way while the mux is just signal routing. There's already a generic pinconf feature for pinctrl-single. It seems like this could be used with am335x now without any changes, e.g. by adding "pinctrl-single,drive-strength" to define the bits that control drive strength. It doesn't require added cells to use this. Pin mux and configuration fields all have masks defined. Example, add this: #define PULL_MASK (PULL_UP|PULL_DISABLE) pinctrl-single,bias-pullup = <PULL_DISABLE PULL_UP PULL_DISABLE PULL_MASK>; pinctrl-single,bias-pulldown = <PULL_DISABLE 0 PULL_DISABLE PULL_MASK>; If I read the driver right (the bindings doc is far from clear), then I think that configures the pin with pad pull up/down disabled and allows the generic pinconf system to enable the pullup or pulldown. Combining the definition of the fields with the value to initialize it in the same property doesn't make that much sense to me. > With interrupts the IRQ_TYPE flags are generic and separate from the > hardware specific cells. If we wanted to, we could have something > similar for pinctrl framework. pinctrl-cells is really pretty different from gpio-cells and interrupt-cells. The latter two are used in handles that allow an external node to reference something in the node that defines the gpio or interrupt cells. The generic flags are used by an *unrelated driver* to tell an platform specific interrupt controller driver it should configure an edge triggered interrupt. It makes it easier to use the binding in the unrelated driver that needs an interrupt since the flags are always the same. But mainly it works because the gpio and interrupt framework in the kernel already support these concepts, so the flags can get passed as is to existing kernel functions. But pinctrl-single,pins is only used inside pinctrl-single itself. There's no handle anywhere like: yoyodyne,reset = <&am335x_pinmux AM335X_PIN_FOO MUX_MODE7 GENERIC_PULLUP_ENABLED_FLAG>; I don't see how one could get made either, since there's already a pinctrl system and it just doesn't work that way. 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; }; }; That would work on some other boards. To use pinctrl-single, you'd need to have a binding that mapped pin and function names to numbers (why couldn't the pincfg binding use numbers!) and the bits/mask for pull up. Which could be done. But at that point pinctrl-single,pins is replaced, it wouldn't even get used, so adding more cells to it hasn't done anything for you. > > 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. > > I don't see issues with backward compabilty. If we specify that the > controller uses #pinctrl-cells = <2>, and some additional property > for specifying generic conf flags, we'd have a similar generic binding > to the interrupt binding. 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.
* 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 :) > 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. Regards, Tony
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- }; > > 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?? 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?
* 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
On Wed, Sep 30, 2020 at 2:47 AM Tony Lindgren <tony@atomide.com> wrote: > > * Trent Piepho <tpiepho@gmail.com> [200930 09:34]: > > > > 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>; It doesn't seem like these patches help achieve that, since they create device tree binaries with a property that has the same name and number of cells, but the cells have a different meaning than above, and must be handled differently by the driver. So you either drop compatibility or need to forever deal with supporting an interim format that is being created by these patches. The conf and max are already split in (all but one) of the device tree SOURCE files via the macro with multiple fields. That does seem like a good step if you wanted to transition into something like the above. But splitting it in the binary too doesn't help. Is there a way the dtb now being created can turn into the above via a driver change? Absolutely not. So what's the point of an interim binary format? All that matters is what the source looks like, and since that doesn't change, nothing is accomplished. I'll also point out that the current generic pinconf, done not via flags but with multiple properties for each configurable parameter, has a drive strength properties with strength in mA or µA as a parameter. How would you do that with generic bit flags? I don't think you can fit everything in pinconf-generic.h into one 32 bit cell. > > 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. But the pinctrl-single,pins isn't parsed. It just uses the values as they are. You'd have to write something to parse the cells and add more data to the dts that told the parser how to turn them into device specific values. It seems like that could eventually achieve basically what is happening already with the dts preprocessor converting symbolic constants into device specific values.
* Trent Piepho <tpiepho@gmail.com> [200930 18:50]: > On Wed, Sep 30, 2020 at 2:47 AM Tony Lindgren <tony@atomide.com> wrote: > > > > * Trent Piepho <tpiepho@gmail.com> [200930 09:34]: > > > > > > > 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>; > > It doesn't seem like these patches help achieve that, since they > create device tree binaries with a property that has the same name and > number of cells, but the cells have a different meaning than above, > and must be handled differently by the driver. So you either drop > compatibility or need to forever deal with supporting an interim > format that is being created by these patches. > > The conf and max are already split in (all but one) of the device tree > SOURCE files via the macro with multiple fields. That does seem like > a good step if you wanted to transition into something like the above. > But splitting it in the binary too doesn't help. Is there a way the > dtb now being created can turn into the above via a driver change? > Absolutely not. So what's the point of an interim binary format? All > that matters is what the source looks like, and since that doesn't > change, nothing is accomplished. We do get the conf and mux data separated though. This fixes the long term complaint that the pinctrl single binding mixes conf and mux data. And the driver can take the dtb conf values and convert them into something generic and use more generic pinctrl functions. So I would not call it interim binary format, it's something we'll be using in the long term. I would also like to get to the next step that we've been discussing, but who knows if we ever get there. Feel free to get the ball rolling on the new generic binding, we'll be discussing it for years as has happened with the earlier attempts :) BTW, for more complicated use cases, we can already quite easily create hardware specific drivers with the use of GENERIC_PINCTRL_GROUPS and GENERIC_PINMUX_FUNCTIONS, see for example the iodelay driver at drivers/pinctrl/ti/pinctrl-ti-iodelay.c. It uses #pinctrl-cells = 2 and passes the timing values in two cells. > I'll also point out that the current generic pinconf, done not via > flags but with multiple properties for each configurable parameter, > has a drive strength properties with strength in mA or µA as a > parameter. How would you do that with generic bit flags? I don't > think you can fit everything in pinconf-generic.h into one 32 bit > cell. Good point. The values could be passed by increasing the value for #pinctrl-cells and then the generic format would need to look like: pinctrl-single,pins = <AM335X_PIN_MDC (PIN_INPUT | PIN_PULLUP) 1234 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. > > But the pinctrl-single,pins isn't parsed. It just uses the values as > they are. You'd have to write something to parse the cells and add > more data to the dts that told the parser how to turn them into device > specific values. It seems like that could eventually achieve > basically what is happening already with the dts preprocessor > converting symbolic constants into device specific values. Except we'd be setting the conf in a generic way and would avoid potentially lots of pin specific properties. 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(-)