From: Drew Fustini <drew@beagleboard.org>
To: Haojian Zhuang <haojian.zhuang@linaro.org>,
Linus Walleij <linus.walleij@linaro.org>
Cc: Tony Lindgren <tony@atomide.com>,
linux-omap@vger.kernel.org, linux-gpio@vger.kernel.org
Subject: Re: pinctrl-single: num_maps in generic pinconf support?
Date: Sun, 31 May 2020 02:17:09 +0200 [thread overview]
Message-ID: <20200531001709.GA168774@x1> (raw)
In-Reply-To: <20200529175544.GA3766151@x1>
On Fri, May 29, 2020 at 07:55:44PM +0200, Drew Fustini wrote:
> On Tue, May 26, 2020 at 02:21:33PM +0200, Drew Fustini wrote:
> > 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
>
> The BeagleBone fails to boot when I change the am33xx_pinmux compatible
> from "pinctrl,single" to "pinconf,single":
>
> diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> index 5ed7f3c58c0f..b5bedd776ee6 100644
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -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
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -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
>
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":
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 1e0614daee9b..18a02cd0c701 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -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
next prev parent reply other threads:[~2020-05-31 0:17 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-26 12:21 pinctrl-single: num_maps in generic pinconf support? Drew Fustini
2020-05-27 16:51 ` Tony Lindgren
2020-05-27 22:19 ` Drew Fustini
2020-05-27 22:41 ` Tony Lindgren
2020-05-28 12:53 ` Drew Fustini
2020-05-29 17:40 ` Tony Lindgren
2020-06-08 18:05 ` Drew Fustini
2020-06-08 20:22 ` Drew Fustini
2020-06-09 18:04 ` Tony Lindgren
2020-06-09 17:55 ` Tony Lindgren
2020-05-29 17:55 ` Drew Fustini
2020-05-31 0:17 ` Drew Fustini [this message]
2020-06-08 12:09 ` Linus Walleij
2020-06-08 12:40 ` Drew Fustini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200531001709.GA168774@x1 \
--to=drew@beagleboard.org \
--cc=haojian.zhuang@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.