Re: [PATCH v2 2/2] pinctrl: zynqmp: Support muxing individual pins

[Sean Anderson <sean.anderson@linux.dev>]

Hi Sai Krishna,

On 6/10/24 08:16, Potthuri, Sai Krishna wrote:
> Hi Sean,
> 
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Monday, May 20, 2024 8:34 PM
>> To: Linus Walleij <linus.walleij@linaro.org>; Simek, Michal
>> <michal.simek@amd.com>; linux-gpio@vger.kernel.org
>> Cc: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; linux-
>> kernel@vger.kernel.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
>> linux-arm-kernel@lists.infradead.org; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH v2 2/2] pinctrl: zynqmp: Support muxing individual pins
>> 
>> While muxing groups of pins at once can be convenient for large interfaces,
>> it can also be rigid. This is because the group is set to all pins which support
>> a particular function, even though not all pins may be used. For example,
>> the sdhci0 function may be used with a 8-bit eMMC, 4-bit SD card, or even a
>> 1-bit SD card. In these cases, the extra pins may be repurposed for other
>> uses, but this is not currently allowed.
>> 
>> There is not too much point in pin "groups" when there are not actual pin
>> groups at the hardware level. The pins can all be muxed individually, so
>> there's no point in adding artificial groups on top.
>> Just mux the pins like the hardware allows.
>> 
>> To this effect, add a new group for each pin which can be muxed. These
>> groups are part of each function the pin can be muxed to. We treat group
>> selectors beyond the number of groups as "pin" groups. To set this up, we
>> initialize groups before functions, and then create a bitmap of used pins for
>> each function. These used pins are appended to the function's list of
>> groups.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v2:
>> - Use __set_bit instead of set_bit
>> - Use size_add when calculating the number of kcalloc members
>> - Expand commit message with some more motivation
>> 
>>  drivers/pinctrl/pinctrl-zynqmp.c | 61 ++++++++++++++++++++++----------
>>  1 file changed, 43 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/pinctrl/pinctrl-zynqmp.c b/drivers/pinctrl/pinctrl-
>> zynqmp.c
>> index 5c46b7d7ebcb..7cc1e43fb07c 100644
>> --- a/drivers/pinctrl/pinctrl-zynqmp.c
>> +++ b/drivers/pinctrl/pinctrl-zynqmp.c
>> @@ -10,6 +10,7 @@
>> 
>>  #include <dt-bindings/pinctrl/pinctrl-zynqmp.h>
>> 
>> +#include <linux/bitmap.h>
>>  #include <linux/init.h>
>>  #include <linux/module.h>
>>  #include <linux/of_address.h>
>> @@ -97,7 +98,7 @@ static int zynqmp_pctrl_get_groups_count(struct
>> pinctrl_dev *pctldev)  {
>>  	struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> 
>> -	return pctrl->ngroups;
>> +	return pctrl->ngroups + zynqmp_desc.npins;
>>  }
>> 
>>  static const char *zynqmp_pctrl_get_group_name(struct pinctrl_dev
>> *pctldev, @@ -105,7 +106,10 @@ static const char
>> *zynqmp_pctrl_get_group_name(struct pinctrl_dev *pctldev,  {
>>  	struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> 
>> -	return pctrl->groups[selector].name;
>> +	if (selector < pctrl->ngroups)
>> +		return pctrl->groups[selector].name;
>> +
>> +	return zynqmp_desc.pins[selector - pctrl->ngroups].name;
>>  }
>> 
>>  static int zynqmp_pctrl_get_group_pins(struct pinctrl_dev *pctldev, @@ -
>> 115,8 +119,13 @@ static int zynqmp_pctrl_get_group_pins(struct
>> pinctrl_dev *pctldev,  {
>>  	struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> 
>> -	*pins = pctrl->groups[selector].pins;
>> -	*npins = pctrl->groups[selector].npins;
>> +	if (selector < pctrl->ngroups) {
>> +		*pins = pctrl->groups[selector].pins;
>> +		*npins = pctrl->groups[selector].npins;
>> +	} else {
>> +		*pins = &zynqmp_desc.pins[selector - pctrl-
>> >ngroups].number;
>> +		*npins = 1;
>> +	}
>> 
>>  	return 0;
>>  }
>> @@ -560,10 +569,12 @@ static int
>> zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32 fid,  {
>>  	u16 resp[NUM_GROUPS_PER_RESP] = {0};
>>  	const char **fgroups;
>> -	int ret, index, i;
>> +	int ret, index, i, pin;
>> +	unsigned int npins;
>> +	unsigned long *used_pins __free(bitmap) =
>> +		bitmap_zalloc(zynqmp_desc.npins, GFP_KERNEL);
>> 
>> -	fgroups = devm_kcalloc(dev, func->ngroups, sizeof(*fgroups),
>> GFP_KERNEL);
>> -	if (!fgroups)
>> +	if (!used_pins)
>>  		return -ENOMEM;
>> 
>>  	for (index = 0; index < func->ngroups; index +=
>> NUM_GROUPS_PER_RESP) { @@ -578,23 +589,37 @@ static int
>> zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32 fid,
>>  			if (resp[i] == RESERVED_GROUP)
>>  				continue;
>> 
>> -			fgroups[index + i] = devm_kasprintf(dev,
>> GFP_KERNEL,
>> -							    "%s_%d_grp",
>> -							    func->name,
>> -							    index + i);
>> -			if (!fgroups[index + i])
>> -				return -ENOMEM;
>> -
>>  			groups[resp[i]].name = devm_kasprintf(dev,
>> GFP_KERNEL,
>>  							      "%s_%d_grp",
>>  							      func->name,
>>  							      index + i);
>>  			if (!groups[resp[i]].name)
>>  				return -ENOMEM;
>> +
>> +			for (pin = 0; pin < groups[resp[i]].npins; pin++)
>> +				__set_bit(groups[resp[i]].pins[pin],
>> used_pins);
>>  		}
>>  	}
>>  done:
>> +	npins = bitmap_weight(used_pins, zynqmp_desc.npins);
>> +	fgroups = devm_kcalloc(dev, size_add(func->ngroups, npins),
>> +			       sizeof(*fgroups), GFP_KERNEL);
>> +	if (!fgroups)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < func->ngroups; i++) {
>> +		fgroups[i] = devm_kasprintf(dev, GFP_KERNEL, "%s_%d_grp",
>> +					    func->name, i);
>> +		if (!fgroups[i])
>> +			return -ENOMEM;
>> +	}
>> +
>> +	pin = 0;
>> +	for_each_set_bit(pin, used_pins, zynqmp_desc.npins)
>> +		fgroups[i++] = zynqmp_desc.pins[pin].name;
>> +
>>  	func->groups = fgroups;
>> +	func->ngroups += npins;
>> 
>>  	return 0;
>>  }
>> @@ -772,6 +797,10 @@ static int
>> zynqmp_pinctrl_prepare_function_info(struct device *dev,
>>  	if (!groups)
>>  		return -ENOMEM;
>> 
>> +	ret = zynqmp_pinctrl_prepare_group_pins(dev, groups, pctrl-
>> >ngroups);
>> +	if (ret)
>> +		return ret;
>> +
>>  	for (i = 0; i < pctrl->nfuncs; i++) {
>>  		ret = zynqmp_pinctrl_prepare_func_groups(dev, i, &funcs[i],
>>  							 groups);
>> @@ -779,10 +808,6 @@ static int
>> zynqmp_pinctrl_prepare_function_info(struct device *dev,
>>  			return ret;
>>  	}
>> 
>> -	ret = zynqmp_pinctrl_prepare_group_pins(dev, groups, pctrl-
>> >ngroups);
>> -	if (ret)
>> -		return ret;
>> -
>>  	pctrl->funcs = funcs;
>>  	pctrl->groups = groups;
>> 
> While testing this patch, observed that some more changes required in the 
> other functions like set_mux, pin_config_group_set. Pasted the diff below.

Thanks for catching this. I will incorporate these changes into v3.

> diff --git a/drivers/pinctrl/pinctrl-zynqmp.c b/drivers/pinctrl/pinctrl-zynqmp.c
> index 636e56f7cd92..c82074819be3 100644
> --- a/drivers/pinctrl/pinctrl-zynqmp.c
> +++ b/drivers/pinctrl/pinctrl-zynqmp.c
> @@ -206,12 +206,18 @@ static int zynqmp_pinmux_set_mux(struct pinctrl_dev *pctldev,
>                                  unsigned int function,
>                                  unsigned int group)
>  {
> -       struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> -       const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[group];
> +       const unsigned *pins;
> +       unsigned npins;
>         int ret, i;
>  
> -       for (i = 0; i < pgrp->npins; i++) {
> -               unsigned int pin = pgrp->pins[i];
> +       ret = zynqmp_pctrl_get_group_pins(pctldev, group, &pins, &npins);
> +       if (ret) {

This function cannot fail, so we don't need to check the return.

> +               dev_err(pctldev->dev, "Get group pins failed for group %u\n", group);
> +               return ret;
> +       }
> +
> +       for (i = 0; i < npins; i++) {
> +               unsigned int pin = pins[i];
>  
>                 ret = zynqmp_pm_pinctrl_set_function(pin, function);
>                 if (ret) {
> @@ -476,13 +482,18 @@ static int zynqmp_pinconf_group_set(struct pinctrl_dev *pctldev,
>                                     unsigned long *configs,
>                                     unsigned int num_configs)
>  {
> -       int i, ret;
> -       struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> -       const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[selector];
> -
> -       for (i = 0; i < pgrp->npins; i++) {
> -               ret = zynqmp_pinconf_cfg_set(pctldev, pgrp->pins[i], configs,
> -                                            num_configs);
> +       const unsigned *pins;
> +        unsigned npins;
> +        int i, ret;
> +
> +        ret = zynqmp_pctrl_get_group_pins(pctldev, selector, &pins, &npins);
> +        if (ret) {

ditto

> +                dev_err(pctldev->dev, "Get group pins failed for group %u\n", selector);
> +                return ret;
> +        }
> +
> +       for (i = 0; i < npins; i++) {
> +               ret = zynqmp_pinconf_cfg_set(pctldev, pins[i], configs, num_configs);
>                 if (ret)
>                         return ret;
>         }
> 
> Regards
> Sai Krishna

--Sean