| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d9e9eefa-191c-460f-aae1-3b328043c1e6@linux.dev>
Date: Mon, 10 Jun 2024 12:59:06 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: "Potthuri, Sai Krishna" <sai.krishna.potthuri@....com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
"Simek, Michal" <michal.simek@....com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andy Shevchenko <andy.shevchenko@...il.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"Goud, Srinivas" <srinivas.goud@....com>
Subject: Re: [PATCH v2 2/2] pinctrl: zynqmp: Support muxing individual pins
Hi Sai Krishna,
On 6/10/24 08:16, Potthuri, Sai Krishna wrote:
> Hi Sean,
>
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@...ux.dev>
>> Sent: Monday, May 20, 2024 8:34 PM
>> To: Linus Walleij <linus.walleij@...aro.org>; Simek, Michal
>> <michal.simek@....com>; linux-gpio@...r.kernel.org
>> Cc: Potthuri, Sai Krishna <sai.krishna.potthuri@....com>; linux-
>> kernel@...r.kernel.org; Andy Shevchenko <andy.shevchenko@...il.com>;
>> linux-arm-kernel@...ts.infradead.org; Sean Anderson
>> <sean.anderson@...ux.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@...ux.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
Powered by blists - more mailing lists