* [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
2020-07-01 1:33 [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Drew Fustini
@ 2020-07-01 1:33 ` Drew Fustini
2020-09-08 23:52 ` Trent Piepho
2020-07-01 1:33 ` [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2 Drew Fustini
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Drew Fustini @ 2020-07-01 1:33 UTC (permalink / raw)
To: Tony Lindgren, Rob Herring, linux-omap, linux-kernel, linux-gpio,
linux-arm-kernel, Linus Walleij, Haojian Zhuang, devicetree,
bcousson, Jason Kridner, Robert Nelson
Cc: Drew Fustini
If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
get the value to store in the register.
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
drivers/pinctrl/pinctrl-single.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index f3a8a465d27e..a436ed7762cc 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1017,10 +1017,17 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
break;
}
- /* Index plus one value cell */
offset = pinctrl_spec.args[0];
vals[found].reg = pcs->base + offset;
- vals[found].val = pinctrl_spec.args[1];
+
+ switch (pinctrl_spec.args_count) {
+ case 2:
+ vals[found].val = pinctrl_spec.args[1];
+ break;
+ case 3:
+ vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
+ break;
+ }
dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
pinctrl_spec.np, offset, pinctrl_spec.args[1]);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
2020-07-01 1:33 ` [PATCH v4 1/2] pinctrl: single: parse " Drew Fustini
@ 2020-09-08 23:52 ` Trent Piepho
2020-09-13 19:42 ` Drew Fustini
0 siblings, 1 reply; 15+ messages in thread
From: Trent Piepho @ 2020-09-08 23:52 UTC (permalink / raw)
To: linux-arm-kernel
Cc: devicetree, linux-gpio, Drew Fustini, Linus Walleij,
linux-kernel, Tony Lindgren, Jason Kridner, Rob Herring,
Haojian Zhuang, bcousson, linux-omap, Robert Nelson
On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
> - vals[found].val = pinctrl_spec.args[1];
> +
> + switch (pinctrl_spec.args_count) {
> + case 2:
> + vals[found].val = pinctrl_spec.args[1];
> + break;
> + case 3:
> + vals[found].val = (pinctrl_spec.args[1] |
pinctrl_spec.args[2]);
> + break;
> + }
>
> dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> pinctrl_spec.np, offset,
pinctrl_spec.args[1]);
If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
anything other than zero (from when it's calloc'ed) and the pinctrl will
silently be programmed to zero.
The debug printout was not change to print vals[found].val, so it will
continue to print the value of the 2nd cell.
The result is that a #pinctrl-cells of 3 will produce no warning or error,
program the pinctrl to zero, whilst at the same time emit debug log messages
that it is programming the expected values.
The device tree documentation still states that #pinctrl-cells must be 1 when
using pinctrl-single,pins. This new special case of ORing two values is not
documented.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
2020-09-08 23:52 ` Trent Piepho
@ 2020-09-13 19:42 ` Drew Fustini
2020-09-13 23:27 ` Drew Fustini
0 siblings, 1 reply; 15+ messages in thread
From: Drew Fustini @ 2020-09-13 19:42 UTC (permalink / raw)
To: Trent Piepho
Cc: devicetree, Jason Kridner, Tony Lindgren, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, Haojian Zhuang, bcousson,
linux-omap, Robert Nelson, linux-arm-kernel
On Tue, Sep 08, 2020 at 04:52:58PM -0700, Trent Piepho wrote:
> On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > get the value to store in the register.
>
>
> > - vals[found].val = pinctrl_spec.args[1];
> > +
> > + switch (pinctrl_spec.args_count) {
> > + case 2:
> > + vals[found].val = pinctrl_spec.args[1];
> > + break;
> > + case 3:
> > + vals[found].val = (pinctrl_spec.args[1] |
> pinctrl_spec.args[2]);
> > + break;
> > + }
> >
> > dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> > pinctrl_spec.np, offset,
> pinctrl_spec.args[1]);
>
> If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
> anything other than zero (from when it's calloc'ed) and the pinctrl will
> silently be programmed to zero.
If #pinctrl-cells is 3, then it will be:
vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
Do you mean if #pinctrl-cells is great than 3 then it will just have a
default value of zero?
That does appear to be the case and is probably not the behavior we
want. Thank you for pointing this out. Earlier, there is a check to
make sure there are at least 2 arguments:
if (pinctrl_spec.args_count < 2) {
dev_err(pcs->dev, "invalid args_count for spec: %i\n",
pinctrl_spec.args_count);
break;
}
I'll submit a patch where the upper bound is also checked:
if (pinctrl_spec.args_count < 2 || pinctrl_spec.args_count > 3) {
dev_err(pcs->dev, "invalid args_count for spec: %i\n",
pinctrl_spec.args_count);
break;
}
> The debug printout was not change to print vals[found].val, so it will
> continue to print the value of the 2nd cell.
Thank you for pointing this out. Yes, this is an oversight and I will
submit a patch.
> The result is that a #pinctrl-cells of 3 will produce no warning or error,
> program the pinctrl to zero, whilst at the same time emit debug log messages
> that it is programming the expected values.
>
> The device tree documentation still states that #pinctrl-cells must be 1 when
> using pinctrl-single,pins. This new special case of ORing two values is not
> documented.
This is a good point, too. I will make a patch to update the
documentation.
-Drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
2020-09-13 19:42 ` Drew Fustini
@ 2020-09-13 23:27 ` Drew Fustini
0 siblings, 0 replies; 15+ messages in thread
From: Drew Fustini @ 2020-09-13 23:27 UTC (permalink / raw)
To: Trent Piepho
Cc: devicetree, Jason Kridner, Tony Lindgren, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, Haojian Zhuang, bcousson,
linux-omap, Robert Nelson, linux-arm-kernel
On Sun, Sep 13, 2020 at 09:42:33PM +0200, Drew Fustini wrote:
> On Tue, Sep 08, 2020 at 04:52:58PM -0700, Trent Piepho wrote:
> > On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> > > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> > > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > > get the value to store in the register.
> >
> >
> > > - vals[found].val = pinctrl_spec.args[1];
> > > +
> > > + switch (pinctrl_spec.args_count) {
> > > + case 2:
> > > + vals[found].val = pinctrl_spec.args[1];
> > > + break;
> > > + case 3:
> > > + vals[found].val = (pinctrl_spec.args[1] |
> > pinctrl_spec.args[2]);
> > > + break;
> > > + }
> > >
> > > dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> > > pinctrl_spec.np, offset,
> > pinctrl_spec.args[1]);
> >
> > If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
> > anything other than zero (from when it's calloc'ed) and the pinctrl will
> > silently be programmed to zero.
>
> If #pinctrl-cells is 3, then it will be:
>
> vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
>
> Do you mean if #pinctrl-cells is great than 3 then it will just have a
> default value of zero?
>
> That does appear to be the case and is probably not the behavior we
> want. Thank you for pointing this out. Earlier, there is a check to
> make sure there are at least 2 arguments:
>
> if (pinctrl_spec.args_count < 2) {
> dev_err(pcs->dev, "invalid args_count for spec: %i\n",
> pinctrl_spec.args_count);
> break;
> }
>
> I'll submit a patch where the upper bound is also checked:
>
> if (pinctrl_spec.args_count < 2 || pinctrl_spec.args_count > 3) {
> dev_err(pcs->dev, "invalid args_count for spec: %i\n",
> pinctrl_spec.args_count);
> break;
> }
>
I was mistaken when I wrote the above. I was using the term
#pinctrl-cells when I should have been writing pinctrl_spec.args_count.
pinctrl_spec.args_count is 2 when #pictrl-cells is 1.
pinctrl_spec.args_count is 3 when #pictrl-cells is 2.
I have submitted patches [1][2] with fixes for the bounds check and the
dev_dbg().
> > The debug printout was not change to print vals[found].val, so it will
> > continue to print the value of the 2nd cell.
>
> Thank you for pointing this out. Yes, this is an oversight and I will
> submit a patch.
>
> > The result is that a #pinctrl-cells of 3 will produce no warning or error,
> > program the pinctrl to zero, whilst at the same time emit debug log messages
> > that it is programming the expected values.
> >
> > The device tree documentation still states that #pinctrl-cells must be 1 when
> > using pinctrl-single,pins. This new special case of ORing two values is not
> > documented.
>
> This is a good point, too. I will make a patch to update the
> documentation.
>
>
> -Drew
[1] https://lore.kernel.org/linux-omap/20200913231557.2063071-1-drew@beagleboard.org/
[2] https://lore.kernel.org/linux-omap/20200913230306.2061645-1-drew@beagleboard.org/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
2020-07-01 1:33 [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Drew Fustini
2020-07-01 1:33 ` [PATCH v4 1/2] pinctrl: single: parse " Drew Fustini
@ 2020-07-01 1:33 ` Drew Fustini
2020-09-09 0:34 ` Trent Piepho
2021-01-15 18:02 ` Emmanuel Vadot
2020-07-02 17:36 ` [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Tony Lindgren
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Drew Fustini @ 2020-07-01 1:33 UTC (permalink / raw)
To: Tony Lindgren, Rob Herring, linux-omap, linux-kernel, linux-gpio,
linux-arm-kernel, Linus Walleij, Haojian Zhuang, devicetree,
bcousson, Jason Kridner, Robert Nelson
Cc: Drew Fustini
Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
conf and pin mux values separate.
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
include/dt-bindings/pinctrl/omap.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index a9cbefc80c0c..3141590e5889 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -278,7 +278,7 @@ scm: scm@0 {
am33xx_pinmux: pinmux@800 {
compatible = "pinctrl-single";
reg = <0x800 0x238>;
- #pinctrl-cells = <1>;
+ #pinctrl-cells = <2>;
pinctrl-single,register-width = <32>;
pinctrl-single,function-mask = <0x7f>;
};
diff --git a/include/dt-bindings/pinctrl/omap.h b/include/dt-bindings/pinctrl/omap.h
index 625718042413..2d2a8c737822 100644
--- a/include/dt-bindings/pinctrl/omap.h
+++ b/include/dt-bindings/pinctrl/omap.h
@@ -65,7 +65,7 @@
#define DM814X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
#define DM816X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
#define AM33XX_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
-#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
+#define AM33XX_PADCONF(pa, conf, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
/*
* Macros to allow using the offset from the padconf physical address
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
2020-07-01 1:33 ` [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2 Drew Fustini
@ 2020-09-09 0:34 ` Trent Piepho
2021-01-15 18:02 ` Emmanuel Vadot
1 sibling, 0 replies; 15+ messages in thread
From: Trent Piepho @ 2020-09-09 0:34 UTC (permalink / raw)
To: linux-arm-kernel
Cc: devicetree, linux-gpio, Drew Fustini, Linus Walleij,
linux-kernel, Tony Lindgren, Rob Herring, Haojian Zhuang,
Jason Kridner, linux-omap, Robert Nelson
On Tuesday, June 30, 2020 6:33:20 PM PDT Drew Fustini wrote:
> Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
> requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
> conf and pin mux values separate.
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -278,7 +278,7 @@ scm: scm@0 {
> am33xx_pinmux: pinmux@800 {
> compatible = "pinctrl-single";
> reg = <0x800 0x238>;
> - #pinctrl-cells = <1>;
> + #pinctrl-cells = <2>;
> #define AM33XX_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> -#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
> +#define AM33XX_PADCONF(pa, conf, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
If a dts file uses am33xx_pinmux from am33xx-l4.dtsi, but does not use the
AM33XX_PADCONF() macro for all pin settings, like say it uses AM33XX_IOPAD(),
then the dtb will be totally broken as pin addresses and values will all be
off.
Similarly, using AM33XX_PADCONF() with a different pinctrl defined elsewhere
would also break.
In the latest linux-next kernel, I found one case of the former problem, in
am335x-guardian.dts.
The barebox bootloader had all the am33xx boards broken when the dts change
was imported without adding the OR-two-values special case to the pinctrl
driver. Which I then tracked to here.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
2020-07-01 1:33 ` [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2 Drew Fustini
2020-09-09 0:34 ` Trent Piepho
@ 2021-01-15 18:02 ` Emmanuel Vadot
2021-01-15 21:40 ` Drew Fustini
1 sibling, 1 reply; 15+ messages in thread
From: Emmanuel Vadot @ 2021-01-15 18:02 UTC (permalink / raw)
To: Drew Fustini
Cc: devicetree, Jason Kridner, Tony Lindgren, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, Haojian Zhuang, bcousson,
linux-omap, Robert Nelson, linux-arm-kernel
Hello Drew,
On Wed, 1 Jul 2020 03:33:20 +0200
Drew Fustini <drew@beagleboard.org> wrote:
> Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
> requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
> conf and pin mux values separate.
>
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> ---
> arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
> include/dt-bindings/pinctrl/omap.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> index a9cbefc80c0c..3141590e5889 100644
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -278,7 +278,7 @@ scm: scm@0 {
> am33xx_pinmux: pinmux@800 {
> compatible = "pinctrl-single";
> reg = <0x800 0x238>;
> - #pinctrl-cells = <1>;
> + #pinctrl-cells = <2>;
> pinctrl-single,register-width = <32>;
> pinctrl-single,function-mask = <0x7f>;
> };
> diff --git a/include/dt-bindings/pinctrl/omap.h b/include/dt-bindings/pinctrl/omap.h
> index 625718042413..2d2a8c737822 100644
> --- a/include/dt-bindings/pinctrl/omap.h
> +++ b/include/dt-bindings/pinctrl/omap.h
> @@ -65,7 +65,7 @@
> #define DM814X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> #define DM816X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> #define AM33XX_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> -#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
> +#define AM33XX_PADCONF(pa, conf, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
>
> /*
> * Macros to allow using the offset from the padconf physical address
> --
> 2.25.1
Based on the bindings doc a value of 2 is only acceptable if one uses
pinctrl-single,bits but all the am33xx pins still uses
pinctrl-single,pins.
I noticed this because this breaks FreeBSD when I tried with 5.9 dts.
--
Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
2021-01-15 18:02 ` Emmanuel Vadot
@ 2021-01-15 21:40 ` Drew Fustini
2021-01-18 7:41 ` Tony Lindgren
0 siblings, 1 reply; 15+ messages in thread
From: Drew Fustini @ 2021-01-15 21:40 UTC (permalink / raw)
To: Emmanuel Vadot
Cc: devicetree, Jason Kridner, Tony Lindgren, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, Haojian Zhuang, bcousson,
linux-omap, Robert Nelson, linux-arm-kernel
On Fri, Jan 15, 2021 at 07:02:01PM +0100, Emmanuel Vadot wrote:
>
> Hello Drew,
>
> On Wed, 1 Jul 2020 03:33:20 +0200
> Drew Fustini <drew@beagleboard.org> wrote:
>
> > Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
> > requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
> > conf and pin mux values separate.
> >
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > ---
> > arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
> > include/dt-bindings/pinctrl/omap.h | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> > index a9cbefc80c0c..3141590e5889 100644
> > --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> > +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> > @@ -278,7 +278,7 @@ scm: scm@0 {
> > am33xx_pinmux: pinmux@800 {
> > compatible = "pinctrl-single";
> > reg = <0x800 0x238>;
> > - #pinctrl-cells = <1>;
> > + #pinctrl-cells = <2>;
> > pinctrl-single,register-width = <32>;
> > pinctrl-single,function-mask = <0x7f>;
> > };
> > diff --git a/include/dt-bindings/pinctrl/omap.h b/include/dt-bindings/pinctrl/omap.h
> > index 625718042413..2d2a8c737822 100644
> > --- a/include/dt-bindings/pinctrl/omap.h
> > +++ b/include/dt-bindings/pinctrl/omap.h
> > @@ -65,7 +65,7 @@
> > #define DM814X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > #define DM816X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > #define AM33XX_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > -#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
> > +#define AM33XX_PADCONF(pa, conf, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
> >
> > /*
> > * Macros to allow using the offset from the padconf physical address
> > --
> > 2.25.1
>
> Based on the bindings doc a value of 2 is only acceptable if one uses
> pinctrl-single,bits but all the am33xx pins still uses
> pinctrl-single,pins.
> I noticed this because this breaks FreeBSD when I tried with 5.9 dts.
>
> --
> Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>
Hello Emmanuel,
Sorry to hear about that. This change was made based on discussion with
Tony Lindgren this past July. Trent Piepho later pointed out issues wtih
the change including the binding documentation. I had tried to fix
the documentation in September [1]. However, I notice that it seems I
missed changing the lines near the top of pinctrl-single.txt [2]:
- #pinctrl-cells : number of cells in addition to the index, set to 1
for pinctrl-single,pins and 2 for pinctrl-single,bits
I am thinking that should be re-written as:
- #pinctrl-cells : number of cells in addition to the index, this value
can be 1 or 2 for pinctrl-single,pins and must be 2 for pinctrl-single,bits
Tony - what do you think?
Thanks,
Drew
[1] https://lore.kernel.org/linux-gpio/20200919200836.3218536-1-drew@beagleboard.org/
[2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
2021-01-15 21:40 ` Drew Fustini
@ 2021-01-18 7:41 ` Tony Lindgren
0 siblings, 0 replies; 15+ messages in thread
From: Tony Lindgren @ 2021-01-18 7:41 UTC (permalink / raw)
To: Drew Fustini
Cc: devicetree, Emmanuel Vadot, Jason Kridner, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, Haojian Zhuang, bcousson,
linux-omap, Robert Nelson, linux-arm-kernel
* Drew Fustini <drew@beagleboard.org> [210115 21:40]:
> On Fri, Jan 15, 2021 at 07:02:01PM +0100, Emmanuel Vadot wrote:
> >
> > Hello Drew,
> >
> > On Wed, 1 Jul 2020 03:33:20 +0200
> > Drew Fustini <drew@beagleboard.org> wrote:
> >
> > > Increase #pinctrl-cells to 2 so that mux and conf be kept separate. This
> > > requires the AM33XX_PADCONF macro in omap.h to also be modified to keep pin
> > > conf and pin mux values separate.
> > >
> > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > > ---
> > > arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
> > > include/dt-bindings/pinctrl/omap.h | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> > > index a9cbefc80c0c..3141590e5889 100644
> > > --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> > > +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> > > @@ -278,7 +278,7 @@ scm: scm@0 {
> > > am33xx_pinmux: pinmux@800 {
> > > compatible = "pinctrl-single";
> > > reg = <0x800 0x238>;
> > > - #pinctrl-cells = <1>;
> > > + #pinctrl-cells = <2>;
> > > pinctrl-single,register-width = <32>;
> > > pinctrl-single,function-mask = <0x7f>;
> > > };
> > > diff --git a/include/dt-bindings/pinctrl/omap.h b/include/dt-bindings/pinctrl/omap.h
> > > index 625718042413..2d2a8c737822 100644
> > > --- a/include/dt-bindings/pinctrl/omap.h
> > > +++ b/include/dt-bindings/pinctrl/omap.h
> > > @@ -65,7 +65,7 @@
> > > #define DM814X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > > #define DM816X_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > > #define AM33XX_IOPAD(pa, val) OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
> > > -#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
> > > +#define AM33XX_PADCONF(pa, conf, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
> > >
> > > /*
> > > * Macros to allow using the offset from the padconf physical address
> > > --
> > > 2.25.1
> >
> > Based on the bindings doc a value of 2 is only acceptable if one uses
> > pinctrl-single,bits but all the am33xx pins still uses
> > pinctrl-single,pins.
> > I noticed this because this breaks FreeBSD when I tried with 5.9 dts.
> >
> > --
> > Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org>
>
> Hello Emmanuel,
>
> Sorry to hear about that. This change was made based on discussion with
> Tony Lindgren this past July. Trent Piepho later pointed out issues wtih
> the change including the binding documentation. I had tried to fix
> the documentation in September [1]. However, I notice that it seems I
> missed changing the lines near the top of pinctrl-single.txt [2]:
>
> - #pinctrl-cells : number of cells in addition to the index, set to 1
> for pinctrl-single,pins and 2 for pinctrl-single,bits
>
> I am thinking that should be re-written as:
>
> - #pinctrl-cells : number of cells in addition to the index, this value
> can be 1 or 2 for pinctrl-single,pins and must be 2 for pinctrl-single,bits
>
> Tony - what do you think?
Sounds good to me.
Regards,
Tony
> [1] https://lore.kernel.org/linux-gpio/20200919200836.3218536-1-drew@beagleboard.org/
> [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2
2020-07-01 1:33 [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Drew Fustini
2020-07-01 1:33 ` [PATCH v4 1/2] pinctrl: single: parse " Drew Fustini
2020-07-01 1:33 ` [PATCH v4 2/2] ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2 Drew Fustini
@ 2020-07-02 17:36 ` Tony Lindgren
2020-07-05 9:01 ` Haojian Zhuang
2020-07-07 10:59 ` Linus Walleij
4 siblings, 0 replies; 15+ messages in thread
From: Tony Lindgren @ 2020-07-02 17:36 UTC (permalink / raw)
To: Drew Fustini
Cc: devicetree, Jason Kridner, Linus Walleij, linux-kernel,
linux-gpio, Rob Herring, Haojian Zhuang, bcousson, linux-omap,
Robert Nelson, linux-arm-kernel
* Drew Fustini <drew@beagleboard.org> [200701 01:34]:
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
Thanks for updating it. Best that Linus queues both patches via
the pinctrl tree when no more comments:
Acked-by: Tony Lindgren <tony@atomide.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2
2020-07-01 1:33 [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Drew Fustini
` (2 preceding siblings ...)
2020-07-02 17:36 ` [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Tony Lindgren
@ 2020-07-05 9:01 ` Haojian Zhuang
2020-07-07 10:59 ` Linus Walleij
4 siblings, 0 replies; 15+ messages in thread
From: Haojian Zhuang @ 2020-07-05 9:01 UTC (permalink / raw)
To: Drew Fustini
Cc: devicetree, Jason Kridner, Tony Lindgren, Linus Walleij,
linux-kernel, linux-gpio, Rob Herring, bcousson, linux-omap,
Robert Nelson, linux-arm-kernel
On Wed, 1 Jul 2020 at 09:33, Drew Fustini <drew@beagleboard.org> wrote:
>
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
>
> - v3: change order of patches to make sure the pinctrl-single.c patch
> does not break anything without the dts patches
>
> - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> compile error in dtc. I had added it per suggestion from checkpatch
> about having parentheses around complex values.
>
> Drew Fustini (2):
> pinctrl: single: parse #pinctrl-cells = 2
> ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
>
> arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
> drivers/pinctrl/pinctrl-single.c | 11 +++++++++--
> include/dt-bindings/pinctrl/omap.h | 2 +-
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> --
> 2.25.1
>
Acked-by: Haojian Zhuang <haojian.zhuang@linaro.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2
2020-07-01 1:33 [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2 Drew Fustini
` (3 preceding siblings ...)
2020-07-05 9:01 ` Haojian Zhuang
@ 2020-07-07 10:59 ` Linus Walleij
2020-07-07 11:02 ` Drew Fustini
4 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2020-07-07 10:59 UTC (permalink / raw)
To: Drew Fustini
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jason Kridner, Tony Lindgren, linux-kernel,
open list:GPIO SUBSYSTEM, Rob Herring, Haojian Zhuang,
Benoît Cousson, Linux-OMAP, Robert Nelson, Linux ARM
On Wed, Jul 1, 2020 at 3:33 AM Drew Fustini <drew@beagleboard.org> wrote:
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
>
> - v3: change order of patches to make sure the pinctrl-single.c patch
> does not break anything without the dts patches
>
> - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> compile error in dtc. I had added it per suggestion from checkpatch
> about having parentheses around complex values.
>
> Drew Fustini (2):
> pinctrl: single: parse #pinctrl-cells = 2
> ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
Both patches applied to the pinctrl devel branch for v5.9!
Please make sure not to create colliding patches in the DTS files merged
through ARM SoC this merge window.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2
2020-07-07 10:59 ` Linus Walleij
@ 2020-07-07 11:02 ` Drew Fustini
2020-07-11 21:12 ` Linus Walleij
0 siblings, 1 reply; 15+ messages in thread
From: Drew Fustini @ 2020-07-07 11:02 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM, Drew Fustini, linux-kernel,
Tony Lindgren, Jason Kridner, Rob Herring, Haojian Zhuang,
Benoît Cousson, Linux-OMAP, Robert Nelson, Linux ARM
On Tue, Jul 7, 2020 at 1:01 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jul 1, 2020 at 3:33 AM Drew Fustini <drew@beagleboard.org> wrote:
>
> > Currently, pinctrl-single only allows #pinctrl-cells = 1.
> >
> > This series will allow pinctrl-single to also support #pinctrl-cells = 2
> >
> > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > get the value to store in the register.
> >
> > To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> > omap.h is modified to keep pin conf and pin mux values separate.
> >
> > change log:
> > - v4: squash patches 2 and 3 together so that git biesct will not result
> > in a boot failure
> >
> > - v3: change order of patches to make sure the pinctrl-single.c patch
> > does not break anything without the dts patches
> >
> > - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> > compile error in dtc. I had added it per suggestion from checkpatch
> > about having parentheses around complex values.
> >
> > Drew Fustini (2):
> > pinctrl: single: parse #pinctrl-cells = 2
> > ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
>
> Both patches applied to the pinctrl devel branch for v5.9!
>
> Please make sure not to create colliding patches in the DTS files merged
> through ARM SoC this merge window.
>
> Yours,
> Linus Walleij
Thanks.
Which repo/branch is the best for me to use if I am going to be
posting any further dts patches?
-drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/2] pinctrl: single: support #pinctrl-cells = 2
2020-07-07 11:02 ` Drew Fustini
@ 2020-07-11 21:12 ` Linus Walleij
0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2020-07-11 21:12 UTC (permalink / raw)
To: Drew Fustini
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM, Drew Fustini, linux-kernel,
Tony Lindgren, Jason Kridner, Rob Herring, Haojian Zhuang,
Benoît Cousson, Linux-OMAP, Robert Nelson, Linux ARM
On Tue, Jul 7, 2020 at 1:02 PM Drew Fustini <pdp7pdp7@gmail.com> wrote:
> Which repo/branch is the best for me to use if I am going to be
> posting any further dts patches?
Mine, pinctrl devel branch during this (v5.9) cycle I suppose.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 15+ messages in thread