pinctrl-single: num_maps in generic pinconf support?

DISCUSSION Drew Fustini 4 participants 14 messages May 26, 2020 – Jun 9, 2020

Subsystems (4)

Device tree bindings GPIO Pin control Pin controller - single

lore

Share & tools

lore

Drew Fustini

author

2020-05-26 12:21 UTC #0 lore raw

Hello Haojian and Linus,

For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:

1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);

git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"

Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?

thank you,
drew

Tony Lindgren

maintainer

2020-05-27 16:51 UTC #1 lore raw

Quoted 15 lines · Drew Fustini

Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);

Quoted 5 lines

git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?

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.

Regards,

Tony

Drew Fustini

author

2020-05-27 22:19 UTC #2 lore raw

Quoted 19 lines · Tony Lindgren

Quoted 18 lines · Drew Fustini
Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);
git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"

Quoted 6 lines

Quoted 2 lines
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?
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.

Thanks for the insight, Tony.

I was not considering the situation where pinctrl-cells would be more than 1.

I see now from pinctrl-single.txt bindings doc that:

- #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 now wondering if it wrong for me to expect compatible string of "pinconf-single" to work with pinctrl-cells of 1.

I see that arch/arm/boot/dts/da850.dtsi has:

154 pmx_core: pinmux@14120 {

155                         compatible = "pinctrl-single";
156                         reg = <0x14120 0x50>;
157                         #pinctrl-cells = <2>;
158                         pinctrl-single,bit-per-mux;

and arch/arm/boot/dts/keystone-k2l.dtsi has:

108 k2l_pmx: pinmux@2620690 {

109                         compatible = "pinctrl-single";
110                         reg = <0x02620690 0xc>;
111                         #address-cells = <1>;
112                         #size-cells = <0>;
113                         #pinctrl-cells = <2>;
114                         pinctrl-single,bit-per-mux;

Quoted 4 lines

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.

Do you mean that it would be possible to make the change just for AM335x to start with?

Do you think the changes would be limited to pinctrl-single.c and the associated device tree files like am33xx-l4.dtsi ?

thanks,
drew

Tony Lindgren

maintainer

2020-05-27 22:41 UTC #3 lore raw

Quoted 32 lines · Drew Fustini

Quoted 19 lines · Tony Lindgren
Quoted 18 lines · Drew Fustini
Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);
git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"
Quoted 6 lines
Quoted 2 lines
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?
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.
Thanks for the insight, Tony.
I was not considering the situation where pinctrl-cells would be more than 1.

Quoted 7 lines

I see now from pinctrl-single.txt bindings doc that:
- #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 now wondering if it wrong for me to expect compatible string of "pinconf-single" to work with pinctrl-cells of 1.

Ideally the #pinctrl-cells would be what makes sense for the hardware. However, I'm guessing pinctrl-single.c needs patching for that to happen.

Quoted 10 lines

I see that arch/arm/boot/dts/da850.dtsi has:
154 pmx_core: pinmux@14120 {
155                         compatible = "pinctrl-single";
156                         reg = <0x14120 0x50>;
157                         #pinctrl-cells = <2>;
158                         pinctrl-single,bit-per-mux;
and arch/arm/boot/dts/keystone-k2l.dtsi has:

Quoted 7 lines

108 k2l_pmx: pinmux@2620690 {

109                         compatible = "pinctrl-single";
110                         reg = <0x02620690 0xc>;
111                         #address-cells = <1>;
112                         #size-cells = <0>;
113                         #pinctrl-cells = <2>;
114                         pinctrl-single,bit-per-mux;

Yeah so there's also the "bit-per-mux" variant. That should not affect #pinctrl-cells use, we just need to make it more flexible.

Quoted 7 lines

Quoted 4 lines
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.
Do you mean that it would be possible to make the change just for AM335x to start with?

Yes. So ideally we'd just fix up whatever is needed in pinctrl-single.c, then just set #pinctrl-cells = <2> in am33xx-l4.dtsi, and update the AM33XX_PADCONF accordingly.

Quoted 2 lines

Do you think the changes would be limited to pinctrl-single.c and the associated device tree files like am33xx-l4.dtsi ?

Yes that should be the case. There should be no need to churn the board specific dts files now that we have AM33XX_PADCONF.

Regards,

Tony

Drew Fustini

author

2020-05-28 12:53 UTC #4 lore raw

Quoted 71 lines · Tony Lindgren

Quoted 32 lines · Drew Fustini
Quoted 19 lines · Tony Lindgren
Quoted 18 lines · Drew Fustini
Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);
git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"
Quoted 6 lines
Quoted 2 lines
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?
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.
Thanks for the insight, Tony.
I was not considering the situation where pinctrl-cells would be more than 1.
Quoted 7 lines
I see now from pinctrl-single.txt bindings doc that:
- #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 now wondering if it wrong for me to expect compatible string of "pinconf-single" to work with pinctrl-cells of 1.
Ideally the #pinctrl-cells would be what makes sense for the hardware. However, I'm guessing pinctrl-single.c needs patching for that to happen.
Quoted 10 lines
I see that arch/arm/boot/dts/da850.dtsi has:
154 pmx_core: pinmux@14120 {
155                         compatible = "pinctrl-single";
156                         reg = <0x14120 0x50>;
157                         #pinctrl-cells = <2>;
158                         pinctrl-single,bit-per-mux;
and arch/arm/boot/dts/keystone-k2l.dtsi has:
Quoted 7 lines
108 k2l_pmx: pinmux@2620690 {
109                         compatible = "pinctrl-single";
110                         reg = <0x02620690 0xc>;
111                         #address-cells = <1>;
112                         #size-cells = <0>;
113                         #pinctrl-cells = <2>;
114                         pinctrl-single,bit-per-mux;
Yeah so there's also the "bit-per-mux" variant. That should not affect #pinctrl-cells use, we just need to make it more flexible.
Quoted 5 lines
Quoted 4 lines
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.

Quoted 6 lines

Quoted 2 lines
Do you mean that it would be possible to make the change just for AM335x to start with?
Yes. So ideally we'd just fix up whatever is needed in pinctrl-single.c, then just set #pinctrl-cells = <2> in am33xx-l4.dtsi, and update the AM33XX_PADCONF accordingly.

Would you be able to describe what you think AM33XX_PADCONF would look like if the mux and conf are seperated?

Is there an example you know of for another SoC?

Quoted 5 lines

Quoted 2 lines
Do you think the changes would be limited to pinctrl-single.c and the associated device tree files like am33xx-l4.dtsi ?
Yes that should be the case. There should be no need to churn the board specific dts files now that we have AM33XX_PADCONF.

Currently, the macro takes dir and mux:

include/dt-bindings/pinctrl/omap.h:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))

For example, in arch/arm/boot/dts/am335x-bone-common.dtsi:
AM33XX_PADCONF(AM335X_PIN_I2C0_SDA, PIN_INPUT_PULLUP, MUX_MODE0)

I think it might be more accurate to rename 'dir' to 'conf'.

Table "9.3.1.50 conf_<module>_<pin> Register" in the AM335x TRM [0] defines bits 0-2 as "Pad functional signal mux select." Bits 3-6 are what I would consider pin configuration. Only bit 5 is related to
direction: "Input enable value for the PAD".

Quoted 4 lines

Regards,
Tony

thanks,
drew

Tony Lindgren

maintainer

2020-05-29 17:40 UTC #5 lore raw

Quoted 2 lines · Drew Fustini

Would you be able to describe what you think AM33XX_PADCONF would look like if the mux and conf are seperated?

Yes it would just slightly change, see your example below.

Quoted 1 line

Is there an example you know of for another SoC?

I think the other driver already keep the padconf and mux separate.

So not sure where all #pinctrl-cells could be used. It would make pinctrl-single.c a bit nicer though, and probably would make it easier to implement further features.

Some hardware may need it to have #pinctrl-cells = <3> if GPIO features are there too, ideally pinctrl-single would not even care but just work for what is configured for the hardware.

Quoted 4 lines

Currently, the macro takes dir and mux:
include/dt-bindings/pinctrl/omap.h:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))

So after fixing up pinctrl-single.c, and changing the SoC dts to have #pinctrl-cells = <2> instead of <1>, the macro would then need to be:

#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (dir), (mux))

Quoted 2 lines

For example, in arch/arm/boot/dts/am335x-bone-common.dtsi:
AM33XX_PADCONF(AM335X_PIN_I2C0_SDA, PIN_INPUT_PULLUP, MUX_MODE0)

Yeah so no change needed for the use.

Quoted 1 line

I think it might be more accurate to rename 'dir' to 'conf'.

Sure makes sense to rename it in the macro.

Regards,

Tony

Drew Fustini

author

2020-06-08 18:05 UTC #6 lore raw

Quoted 34 lines · Tony Lindgren

Quoted 2 lines · Drew Fustini
Would you be able to describe what you think AM33XX_PADCONF would look like if the mux and conf are seperated?
Yes it would just slightly change, see your example below.
Quoted 1 line
Is there an example you know of for another SoC?
I think the other driver already keep the padconf and mux separate.
So not sure where all #pinctrl-cells could be used. It would make pinctrl-single.c a bit nicer though, and probably would make it easier to implement further features.
Some hardware may need it to have #pinctrl-cells = <3> if GPIO features are there too, ideally pinctrl-single would not even care but just work for what is configured for the hardware.
Quoted 4 lines
Currently, the macro takes dir and mux:
include/dt-bindings/pinctrl/omap.h:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
So after fixing up pinctrl-single.c, and changing the SoC dts to have #pinctrl-cells = <2> instead of <1>, the macro would then need to be:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (dir), (mux))
Quoted 2 lines
For example, in arch/arm/boot/dts/am335x-bone-common.dtsi:
AM33XX_PADCONF(AM335X_PIN_I2C0_SDA, PIN_INPUT_PULLUP, MUX_MODE0)
Yeah so no change needed for the use.

Quoted 7 lines

Quoted 1 line
I think it might be more accurate to rename 'dir' to 'conf'.
Sure makes sense to rename it in the macro.
Regards,
Tony

Tony - would this be how the macro would need to be changed?

include/dt-bindings/pinctrl/omap.h +1 -1

index 625718042413..2d2a8c737822 100644
@@ -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

Drew Fustini

author

2020-06-08 20:22 UTC #7 lore raw

Quoted 50 lines · Drew Fustini

Quoted 34 lines · Tony Lindgren
Quoted 2 lines · Drew Fustini
Would you be able to describe what you think AM33XX_PADCONF would look like if the mux and conf are seperated?
Yes it would just slightly change, see your example below.
Quoted 1 line
Is there an example you know of for another SoC?
I think the other driver already keep the padconf and mux separate.
So not sure where all #pinctrl-cells could be used. It would make pinctrl-single.c a bit nicer though, and probably would make it easier to implement further features.
Some hardware may need it to have #pinctrl-cells = <3> if GPIO features are there too, ideally pinctrl-single would not even care but just work for what is configured for the hardware.
Quoted 4 lines
Currently, the macro takes dir and mux:
include/dt-bindings/pinctrl/omap.h:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
So after fixing up pinctrl-single.c, and changing the SoC dts to have #pinctrl-cells = <2> instead of <1>, the macro would then need to be:
#define AM33XX_PADCONF(pa, dir, mux) OMAP_IOPAD_OFFSET((pa), 0x0800) (dir), (mux))
Quoted 2 lines
For example, in arch/arm/boot/dts/am335x-bone-common.dtsi:
AM33XX_PADCONF(AM335X_PIN_I2C0_SDA, PIN_INPUT_PULLUP, MUX_MODE0)
Yeah so no change needed for the use.
Quoted 7 lines
Quoted 1 line
I think it might be more accurate to rename 'dir' to 'conf'.
Sure makes sense to rename it in the macro.
Regards,
Tony
Tony - would this be how the macro would need to be changed?
include/dt-bindings/pinctrl/omap.h +0 -0
index 625718042413..2d2a8c737822 100644

include/dt-bindings/pinctrl/omap.h line 71

Quoted 8 lines

include/dt-bindings/pinctrl/omap.h +1 -1

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

I've been looking at pinctrl-single.c further and now I am wondering if it makes sense to modify pcs_parse_one_pinctrl_entry() or create a new function. Currently it does:

1020                 /* Index plus one value cell */
1021                 offset = pinctrl_spec.args[0];
1022                 vals[found].reg = pcs->base + offset;
1023                 vals[found].val = pinctrl_spec.args[1];

I believe it will need something similar to what is done in pcs_parse_bits_in_pinctrl_entry():

1136                 /* Index plus two value cells */
1137                 offset = pinctrl_spec.args[0];
1138                 val = pinctrl_spec.args[1];
1139                 mask = pinctrl_spec.args[2];

So using #pinctrl-cells = <2> would require would require:

offset = pinctrl_spec.args[0];
vals[found].reg = pcs->base + offset;
vals[found].conf = pinctrl_spec.args[1];
vals[found].mux = pinctrl_spec.args[2];

However, the type of vals:

struct pcs_func_vals {
	void __iomem *reg;
	unsigned val;
	unsigned mask;
};

represents the combined mux + conf value in the register, so I think pcs_conf_vals would need to used:

struct pcs_conf_vals {
	enum pin_config_param param;
	unsigned val;
	unsigned enable;
	unsigned disable;
	unsigned mask;
};

Thoughts?

thanks,
drew

Tony Lindgren

maintainer

2020-06-09 18:04 UTC #8 lore raw

Quoted 43 lines · Drew Fustini

I've been looking at pinctrl-single.c further and now I am wondering if it makes sense to modify pcs_parse_one_pinctrl_entry() or create a new function. Currently it does:
1020                 /* Index plus one value cell */
1021                 offset = pinctrl_spec.args[0];
1022                 vals[found].reg = pcs->base + offset;
1023                 vals[found].val = pinctrl_spec.args[1];
I believe it will need something similar to what is done in pcs_parse_bits_in_pinctrl_entry():
1136                 /* Index plus two value cells */
1137                 offset = pinctrl_spec.args[0];
1138                 val = pinctrl_spec.args[1];
1139                 mask = pinctrl_spec.args[2];
So using #pinctrl-cells = <2> would require would require:
offset = pinctrl_spec.args[0];
vals[found].reg = pcs->base + offset;
vals[found].conf = pinctrl_spec.args[1];
vals[found].mux = pinctrl_spec.args[2];
However, the type of vals:
struct pcs_func_vals {
	void __iomem *reg;
	unsigned val;
	unsigned mask;
};
represents the combined mux + conf value in the register, so I think pcs_conf_vals would need to used:
struct pcs_conf_vals {
	enum pin_config_param param;
	unsigned val;
	unsigned enable;
	unsigned disable;
	unsigned mask;
};

Quoted 1 line

Thoughts?

Yes sounds about right to me. On write the driver would just write the register based on mux and conf and orr the values together using configured shifts.

Not sure if #pinctrl-cells would ever be more than 2 in this case, but having things generic where possible makes sense.

For example, the omap padconf wakeirq specific bits could potentially configured with #pinctrl-cells = <3>. We now handle them automatically as interrupts so there's really nothing to configure there.

I don't if other SoCs might need to configure other options beyond conf and mux in the dts. I guess it's safe to assume nothing else needs to be configured and have the folks needing more support add it as needed.

Regards,

Tony

Tony Lindgren

maintainer

2020-06-09 17:59 UTC #9 lore raw

In reply to Drew Fustini

include/dt-bindings/pinctrl/omap.h line 68

Quoted 12 lines · Drew Fustini

Tony - would this be how the macro would need to be changed?

include/dt-bindings/pinctrl/omap.h +1 -1

index 625718042413..2d2a8c737822 100644
@@ -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)

Yes looks right to me :)

Tony

Drew Fustini

author

2020-05-29 17:55 UTC #10 lore raw

In reply to Drew Fustini

Quoted 15 lines · Drew Fustini

Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);

Quoted 8 lines

git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?
thank you,
drew

The BeagleBone fails to boot when I change the am33xx_pinmux compatible from "pinctrl,single" to "pinconf,single":

arch/arm/boot/dts/am33xx-l4.dtsi +1 -1

index 5ed7f3c58c0f..b5bedd776ee6 100644
@@ -276,7 +276,7 @@ scm: scm@0 {
                                ranges = <0 0 0x2000>;
 
                                am33xx_pinmux: pinmux@800 {
-                                       compatible = "pinctrl-single";
+                                       compatible = "pinconf-single";
                                        reg = <0x800 0x238>;
                                        #pinctrl-cells = <1>;
                                        pinctrl-single,register-width = <32>;

From the full dmesg output [0], these lines seem the most relevant:

[    2.974958] pinctrl-single 44e10800.pinmux: 142 pins, size 568
[    3.847475] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x160 value: 0x2f
[    3.855556] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xfc value: 0x30
[    3.863520] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf8 value: 0x30
[    3.871483] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf4 value: 0x30
[    3.879444] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf0 value: 0x30
[    3.887404] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x104 value: 0x30
[    3.895455] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x100 value: 0x30
[    3.903505] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x1a0 value: 0x2c
[    3.911583] pinctrl core: add 2 pinctrl maps
[    3.915976] pinctrl core: failed to register map default (1): invalid type given
[    3.923594] omap_hsmmc: probe of 48060000.mmc failed with error -22
[    3.930403] omap_hsmmc 47810000.mmc: GPIO lookup for consumer cd
[    4.440389] Waiting for root device /dev/mmcblk0p1...

The error message:
"pinctrl core: failed to register map default (1): invalid type given"

comes from drivers/pinctrl/core.c:

1387 int pinctrl_register_mappings(const struct pinctrl_map *maps,
1388 unsigned num_maps)
1389 {
1390 int i, ret;
1391 struct pinctrl_maps *maps_node;
1392
1393 pr_debug("add %u pinctrl maps\n", num_maps);
1394
1395 /* First sanity check the new mapping */
1396 for (i = 0; i < num_maps; i++) {
<snip>
1416 switch (maps[i].type) {
1417 case PIN_MAP_TYPE_DUMMY_STATE:
1418 break;
1419 case PIN_MAP_TYPE_MUX_GROUP:
1420 ret = pinmux_validate_map(&maps[i], i);
1421 if (ret < 0)
1422 return ret;
1423 break;
1424 case PIN_MAP_TYPE_CONFIGS_PIN:
1425 case PIN_MAP_TYPE_CONFIGS_GROUP:
1426 ret = pinconf_validate_map(&maps[i], i);
1427 if (ret < 0)
1428 return ret;
1429 break;
1430 default:
1431 pr_err("failed to register map %s (%d): invalid type given\n",
1432 maps[i].name, i);
1433 return -EINVAL;
1434 }
1435 }

The invalid type error occurs when maps[i].type does not match any of the case statements.

I have determined the system will boot ok [1] when num_maps is forced to be 1. Here are the related dmesg lines:

[    3.920484] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x160 value: 0x2f
[    3.928420] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xfc value: 0x30
[    3.936224] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf8 value: 0x30
[    3.944026] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf4 value: 0x30
[    3.951826] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf0 value: 0x30
[    3.959627] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x104 value: 0x30
[    3.967515] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x100 value: 0x30
[    3.975403] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x1a0 value: 0x2c
[    3.983318] pinctrl core: add 1 pinctrl maps
[    3.987653] pinctrl-single 44e10800.pinmux: found group selector 4 for pinmux-mmc0-pins
[    3.995723] pinctrl-single 44e10800.pinmux: request pin 88 (PIN88) for 48060000.mmc
[    4.003434] pinctrl-single 44e10800.pinmux: request pin 63 (PIN63) for 48060000.mmc
[    4.011141] pinctrl-single 44e10800.pinmux: request pin 62 (PIN62) for 48060000.mmc
[    4.018847] pinctrl-single 44e10800.pinmux: request pin 61 (PIN61) for 48060000.mmc
[    4.026633] pinctrl-single 44e10800.pinmux: request pin 60 (PIN60) for 48060000.mmc
[    4.034351] pinctrl-single 44e10800.pinmux: request pin 65 (PIN65) for 48060000.mmc
[    4.042065] pinctrl-single 44e10800.pinmux: request pin 64 (PIN64) for 48060000.mmc
[    4.049774] pinctrl-single 44e10800.pinmux: request pin 104 (PIN104) for 48060000.mmc
[    4.057662] pinctrl-single 44e10800.pinmux: enabling (null) function4

Here is the patch for num_maps:

diiff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 1e0614daee9b..78a93336c711 100644

drivers/pinctrl/pinctrl-single.c +1 -1

@@ -1058,7 +1058,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
                res = pcs_parse_pinconf(pcs, np, function, map);
                if (res)
                        goto free_pingroups;
-               *num_maps = 2;
+               *num_maps = 1;
        } else {
                *num_maps = 1;
        }

I am trying to understand why num_maps is supposed to be 2 when PCS_HAS_PINCONF, and I would appreciate any comments.

Is there a bug somewhere in the code?

Or, is it supposed to be invalid to enable "pinconf,single" compatible for the am33xx_pinmux node?

thank you,
drew

[0] https://gist.github.com/pdp7/293716fe98d90f031bb75950803952a1
[1] https://gist.github.com/pdp7/fc5186f46e34c3acc1b1a169be85d3a9

Drew Fustini

author

2020-05-31 00:17 UTC #11 lore raw

Quoted 149 lines · Drew Fustini

Quoted 15 lines · Drew Fustini
Hello Haojian and Linus,
For pcs_parse_one_pinctrl_entry() in drivers/pinctrl/pinctrl-single.c, I see that num_maps is set to 2 if PCS_HAS_PINCONF is enabled:
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);
Quoted 8 lines
git blame shows me that came from 9dddb4df90d13:
"pinctrl: single: support generic pinconf"
Would you be able to provide any insight as to num_maps needs to be 2 when pinconf is enabled?
thank you,
drew
The BeagleBone fails to boot when I change the am33xx_pinmux compatible from "pinctrl,single" to "pinconf,single":
arch/arm/boot/dts/am33xx-l4.dtsi +1 -1
index 5ed7f3c58c0f..b5bedd776ee6 100644
@@ -276,7 +276,7 @@ scm: scm@0 {
                                ranges = <0 0 0x2000>;
 
                                am33xx_pinmux: pinmux@800 {
-                                       compatible = "pinctrl-single";
+                                       compatible = "pinconf-single";
                                        reg = <0x800 0x238>;
                                        #pinctrl-cells = <1>;
                                        pinctrl-single,register-width = <32>;
From the full dmesg output [0], these lines seem the most relevant:
[    2.974958] pinctrl-single 44e10800.pinmux: 142 pins, size 568
[    3.847475] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x160 value: 0x2f
[    3.855556] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xfc value: 0x30
[    3.863520] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf8 value: 0x30
[    3.871483] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf4 value: 0x30
[    3.879444] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf0 value: 0x30
[    3.887404] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x104 value: 0x30
[    3.895455] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x100 value: 0x30
[    3.903505] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x1a0 value: 0x2c
[    3.911583] pinctrl core: add 2 pinctrl maps
[    3.915976] pinctrl core: failed to register map default (1): invalid type given
[    3.923594] omap_hsmmc: probe of 48060000.mmc failed with error -22
[    3.930403] omap_hsmmc 47810000.mmc: GPIO lookup for consumer cd
[    4.440389] Waiting for root device /dev/mmcblk0p1...
The error message:
"pinctrl core: failed to register map default (1): invalid type given"
comes from drivers/pinctrl/core.c:
1387 int pinctrl_register_mappings(const struct pinctrl_map *maps,
1388 unsigned num_maps)
1389 {
1390 int i, ret;
1391 struct pinctrl_maps *maps_node;
1392
1393 pr_debug("add %u pinctrl maps\n", num_maps);
1394
1395 /* First sanity check the new mapping */
1396 for (i = 0; i < num_maps; i++) {
<snip>
1416 switch (maps[i].type) {
1417 case PIN_MAP_TYPE_DUMMY_STATE:
1418 break;
1419 case PIN_MAP_TYPE_MUX_GROUP:
1420 ret = pinmux_validate_map(&maps[i], i);
1421 if (ret < 0)
1422 return ret;
1423 break;
1424 case PIN_MAP_TYPE_CONFIGS_PIN:
1425 case PIN_MAP_TYPE_CONFIGS_GROUP:
1426 ret = pinconf_validate_map(&maps[i], i);
1427 if (ret < 0)
1428 return ret;
1429 break;
1430 default:
1431 pr_err("failed to register map %s (%d): invalid type given\n",
1432 maps[i].name, i);
1433 return -EINVAL;
1434 }
1435 }
The invalid type error occurs when maps[i].type does not match any of the case statements.
I have determined the system will boot ok [1] when num_maps is forced to be 1. Here are the related dmesg lines:
[    3.920484] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x160 value: 0x2f
[    3.928420] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xfc value: 0x30
[    3.936224] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf8 value: 0x30
[    3.944026] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf4 value: 0x30
[    3.951826] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0xf0 value: 0x30
[    3.959627] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x104 value: 0x30
[    3.967515] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x100 value: 0x30
[    3.975403] pinctrl-single 44e10800.pinmux: pinmux-mmc0-pins index: 0x1a0 value: 0x2c
[    3.983318] pinctrl core: add 1 pinctrl maps
[    3.987653] pinctrl-single 44e10800.pinmux: found group selector 4 for pinmux-mmc0-pins
[    3.995723] pinctrl-single 44e10800.pinmux: request pin 88 (PIN88) for 48060000.mmc
[    4.003434] pinctrl-single 44e10800.pinmux: request pin 63 (PIN63) for 48060000.mmc
[    4.011141] pinctrl-single 44e10800.pinmux: request pin 62 (PIN62) for 48060000.mmc
[    4.018847] pinctrl-single 44e10800.pinmux: request pin 61 (PIN61) for 48060000.mmc
[    4.026633] pinctrl-single 44e10800.pinmux: request pin 60 (PIN60) for 48060000.mmc
[    4.034351] pinctrl-single 44e10800.pinmux: request pin 65 (PIN65) for 48060000.mmc
[    4.042065] pinctrl-single 44e10800.pinmux: request pin 64 (PIN64) for 48060000.mmc
[    4.049774] pinctrl-single 44e10800.pinmux: request pin 104 (PIN104) for 48060000.mmc
[    4.057662] pinctrl-single 44e10800.pinmux: enabling (null) function4
Here is the patch for num_maps:
diiff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 1e0614daee9b..78a93336c711 100644
drivers/pinctrl/pinctrl-single.c +1 -1
@@ -1058,7 +1058,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
                res = pcs_parse_pinconf(pcs, np, function, map);
                if (res)
                        goto free_pingroups;
-               *num_maps = 2;
+               *num_maps = 1;
        } else {
                *num_maps = 1;
        }
I am trying to understand why num_maps is supposed to be 2 when PCS_HAS_PINCONF, and I would appreciate any comments.
Is there a bug somewhere in the code?
Or, is it supposed to be invalid to enable "pinconf,single" compatible for the am33xx_pinmux node?

Quoted 6 lines

thank you,
drew
[0] https://gist.github.com/pdp7/293716fe98d90f031bb75950803952a1
[1] https://gist.github.com/pdp7/fc5186f46e34c3acc1b1a169be85d3a9

I believe I may have found a bug in drivers/pinctrl/pinctrl-single.c

The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf() if PCS_HAS_PINCONF is enabled. The function pcs_parse_pinconf() returns 0 to indicate there was no error and num_maps is then set to 2:

980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,

 981                                                 struct device_node *np,
 982                                                 struct pinctrl_map **map,
 983                                                 unsigned *num_maps,
 984                                                 const char **pgnames)
 985 {
<snip>
1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
1054         (*map)->data.mux.group = np->name;
1055         (*map)->data.mux.function = np->name;

1056
1057 if (PCS_HAS_PINCONF && function) {
1058 res = pcs_parse_pinconf(pcs, np, function, map);
1059 if (res)
1060 goto free_pingroups;
1061 *num_maps = 2;
1062 } else {
1063 *num_maps = 1;
1064 }
1065 mutex_unlock(&pcs->mutex);
1066
1067 return 0;

However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or !nconfs. I believe these conditions should return non-zero. Otherwise pcs_parse_one_pinctrl_entry() will set num_maps = 2 even though no maps were successfully added, as it does not reach "m++" on line 944:

895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
896 struct pcs_function *func,
897 struct pinctrl_map **map)
898
899 {

 900         struct pinctrl_map *m = *map;
 901         int i = 0, nconfs = 0;
 902         unsigned long *settings = NULL, *s = NULL;
 903         struct pcs_conf_vals *conf = NULL;
 904         static const struct pcs_conf_type prop2[] = {
 905                 { "pinctrl-single,drive-strength", PIN_CONFIG_DRIVE_STRENGTH, },
 906                 { "pinctrl-single,slew-rate", PIN_CONFIG_SLEW_RATE, },
 907                 { "pinctrl-single,input-schmitt", PIN_CONFIG_INPUT_SCHMITT, },
 908                 { "pinctrl-single,low-power-mode", PIN_CONFIG_LOW_POWER_MODE, },
 909         };
 910         static const struct pcs_conf_type prop4[] = {
 911                 { "pinctrl-single,bias-pullup", PIN_CONFIG_BIAS_PULL_UP, },
 912                 { "pinctrl-single,bias-pulldown", PIN_CONFIG_BIAS_PULL_DOWN, },
 913                 { "pinctrl-single,input-schmitt-enable",
 914                         PIN_CONFIG_INPUT_SCHMITT_ENABLE, },
 915         };
 916
 917         /* If pinconf isn't supported, don't parse properties in below. */
 918         if (!PCS_HAS_PINCONF)
 919                 return 0;
 920
 921         /* cacluate how much properties are supported in current node */
 922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
 923                 if (of_find_property(np, prop2[i].name, NULL))
 924                         nconfs++;
 925         }
 926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
 927                 if (of_find_property(np, prop4[i].name, NULL))
 928                         nconfs++;
 929         }
 930         if (!nconfs)
 919                 return 0;
 932
 933         func->conf = devm_kcalloc(pcs->dev,
 934                                   nconfs, sizeof(struct pcs_conf_vals),
 935                                   GFP_KERNEL);
 936         if (!func->conf)
 937                 return -ENOMEM;
 938         func->nconfs = nconfs;
 939         conf = &(func->conf[0]);
 940         m++;
 941         settings = devm_kcalloc(pcs->dev, nconfs, sizeof(unsigned long),
 942                                 GFP_KERNEL);
 943         if (!settings)
 944                 return -ENOMEM;
 945         s = &settings[0];
 946
 947         for (i = 0; i < ARRAY_SIZE(prop2); i++)
 948                 pcs_add_conf2(pcs, np, prop2[i].name, prop2[i].param,
 949                               &conf, &s);
 950         for (i = 0; i < ARRAY_SIZE(prop4); i++)
 951                 pcs_add_conf4(pcs, np, prop4[i].name, prop4[i].param,
 952                               &conf, &s);
 953         m->type = PIN_MAP_TYPE_CONFIGS_GROUP;
 954         m->data.configs.group_or_pin = np->name;
 955         m->data.configs.configs = settings;
 956         m->data.configs.num_configs = nconfs;
 957         return 0;

958 }

This patch solves the boot failure on the BeagleBone Black when pinmux compatible is "pinconf,single":

drivers/pinctrl/pinctrl-single.c +2 -2

index 1e0614daee9b..18a02cd0c701 100644
@@ -916,7 +916,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
 
        /* If pinconf isn't supported, don't parse properties in below. */
        if (!PCS_HAS_PINCONF)
-               return 0;
+               return -ENOTSUPP; /* do not return 0 as no map added */
 
        /* cacluate how much properties are supported in current node */
        for (i = 0; i < ARRAY_SIZE(prop2); i++) {

@@ -928,7 +928,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
                        nconfs++;
        }
        if (!nconfs)
-               return 0;
+               return -ENOTSUPP; /* do not return 0 as no map added */
 
        func->conf = devm_kcalloc(pcs->dev,
                                  nconfs, sizeof(struct pcs_conf_vals),

Does this appear to be a reasonable solution?

I would appreciate any comments.

thank you,
drew

Linus Walleij

2020-06-08 12:09 UTC #12 lore raw

Quoted 3 lines · Drew Fustini

Does this appear to be a reasonable solution?
I would appreciate any comments.

Looks reasonable to me. I suggest you send a proper patch. Tony?

Yours,
Linus Walleij

Drew Fustini

2020-06-08 12:40 UTC #13 lore raw

Quoted 11 lines · Linus Walleij

Quoted 3 lines · Drew Fustini
Does this appear to be a reasonable solution?
I would appreciate any comments.
Looks reasonable to me. I suggest you send a proper patch. Tony?
Yours,
Linus Walleij

Thanks for reviewing, Linus.

I have posted a patch last week to which Tony replied that it looked okay but would like Haojian Zhuang to review as neither of us have the Hikey SoCs that enable pinconf for pinctrl-single [0].

On a related note, I'm about to send (from [email protected]) a new version that handles the case of when pincof is enabled and a group has no pinconf properties.

-Drew

[0] https://lore.kernel.org/linux-gpio/20200601174803.GD1371046@x1/