Re: pinctrl-single: num_maps in generic pinconf support?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux