Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
@ 2020-09-14 10:43 Drew Fustini
  2020-09-17  9:03 ` Trent Piepho
  2020-09-23  6:59 ` Tony Lindgren
  0 siblings, 2 replies; 21+ messages in thread
From: Drew Fustini @ 2020-09-14 10:43 UTC (permalink / raw)
  To: Tony Lindgren, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, Trent Piepho, linux-omap, linux-kernel,
	devicetree, linux-gpio
  Cc: Drew Fustini

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(-)

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-14 10:43 [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 Drew Fustini
@ 2020-09-17  9:03 ` Trent Piepho
  2020-09-17  9:20   ` Drew Fustini
  2020-09-23  6:59 ` Tony Lindgren
  1 sibling, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-17  9:03 UTC (permalink / raw)
  To: Drew Fustini
  Cc: Tony Lindgren, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-17  9:03 ` Trent Piepho
@ 2020-09-17  9:20   ` Drew Fustini
  2020-09-17 10:00     ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Drew Fustini @ 2020-09-17  9:20 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Tony Lindgren, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio

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/

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-17  9:20   ` Drew Fustini
@ 2020-09-17 10:00     ` Trent Piepho
  2020-09-17 10:39       ` Drew Fustini
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-17 10:00 UTC (permalink / raw)
  To: Drew Fustini
  Cc: Tony Lindgren, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-17 10:00     ` Trent Piepho
@ 2020-09-17 10:39       ` Drew Fustini
  2020-09-23  6:57         ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Drew Fustini @ 2020-09-17 10:39 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Tony Lindgren, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-17 10:39       ` Drew Fustini
@ 2020-09-23  6:57         ` Tony Lindgren
  2020-09-24  1:34           ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-23  6:57 UTC (permalink / raw)
  To: Drew Fustini
  Cc: Trent Piepho, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-14 10:43 [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 Drew Fustini
  2020-09-17  9:03 ` Trent Piepho
@ 2020-09-23  6:59 ` Tony Lindgren
  1 sibling, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-09-23  6:59 UTC (permalink / raw)
  To: Drew Fustini
  Cc: Rob Herring, Linus Walleij, Jason Kridner, Robert Nelson,
	Trent Piepho, linux-omap, linux-kernel, devicetree, linux-gpio

* 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-23  6:57         ` Tony Lindgren
@ 2020-09-24  1:34           ` Trent Piepho
  2020-09-24  5:43             ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-24  1:34 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  1:34           ` Trent Piepho
@ 2020-09-24  5:43             ` Tony Lindgren
  2020-09-24  5:49               ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-24  5:43 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  5:43             ` Tony Lindgren
@ 2020-09-24  5:49               ` Trent Piepho
  2020-09-24  6:06                 ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-24  5:49 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  5:49               ` Trent Piepho
@ 2020-09-24  6:06                 ` Tony Lindgren
  2020-09-24  6:31                   ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-24  6:06 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  6:06                 ` Tony Lindgren
@ 2020-09-24  6:31                   ` Trent Piepho
  2020-09-24  7:04                     ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-24  6:31 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  6:31                   ` Trent Piepho
@ 2020-09-24  7:04                     ` Tony Lindgren
  2020-09-29 20:15                       ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-24  7:04 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-24  7:04                     ` Tony Lindgren
@ 2020-09-29 20:15                       ` Trent Piepho
  2020-09-30  5:15                         ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-29 20:15 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-29 20:15                       ` Trent Piepho
@ 2020-09-30  5:15                         ` Tony Lindgren
  2020-09-30  8:34                           ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-30  5:15 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30  5:15                         ` Tony Lindgren
@ 2020-09-30  8:34                           ` Trent Piepho
  2020-09-30  9:15                             ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-30  8:34 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30  8:34                           ` Trent Piepho
@ 2020-09-30  9:15                             ` Tony Lindgren
  2020-09-30  9:34                               ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-30  9:15 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30  9:15                             ` Tony Lindgren
@ 2020-09-30  9:34                               ` Trent Piepho
  2020-09-30  9:47                                 ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-30  9:34 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30  9:34                               ` Trent Piepho
@ 2020-09-30  9:47                                 ` Tony Lindgren
  2020-09-30 18:50                                   ` Trent Piepho
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-09-30  9:47 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30  9:47                                 ` Tony Lindgren
@ 2020-09-30 18:50                                   ` Trent Piepho
  2020-10-01  7:00                                     ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Trent Piepho @ 2020-09-30 18:50 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2
  2020-09-30 18:50                                   ` Trent Piepho
@ 2020-10-01  7:00                                     ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-10-01  7:00 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Drew Fustini, Rob Herring, Linus Walleij, Jason Kridner,
	Robert Nelson, linux-omap, linux-kernel, devicetree, linux-gpio,
	Christina Quast

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 10:43 [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 Drew Fustini
2020-09-17  9:03 ` Trent Piepho
2020-09-17  9:20   ` Drew Fustini
2020-09-17 10:00     ` Trent Piepho
2020-09-17 10:39       ` Drew Fustini
2020-09-23  6:57         ` Tony Lindgren
2020-09-24  1:34           ` Trent Piepho
2020-09-24  5:43             ` Tony Lindgren
2020-09-24  5:49               ` Trent Piepho
2020-09-24  6:06                 ` Tony Lindgren
2020-09-24  6:31                   ` Trent Piepho
2020-09-24  7:04                     ` Tony Lindgren
2020-09-29 20:15                       ` Trent Piepho
2020-09-30  5:15                         ` Tony Lindgren
2020-09-30  8:34                           ` Trent Piepho
2020-09-30  9:15                             ` Tony Lindgren
2020-09-30  9:34                               ` Trent Piepho
2020-09-30  9:47                                 ` Tony Lindgren
2020-09-30 18:50                                   ` Trent Piepho
2020-10-01  7:00                                     ` Tony Lindgren
2020-09-23  6:59 ` Tony Lindgren

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git