[PATCH 0/2] u-boot: pinctrl: scmi: Use standard pin muxing format

[PATCH 0/2] u-boot: pinctrl: scmi: Use standard pin muxing format

[Dan Carpenter]

In my original code I created a custom format for pin muxing but it's
better to use the standard format.  I also found a bug trying to use
set the function.  (That code wasn't used previously).

My GPIO driver is not affected by this change:
https://lore.kernel.org/all/ff0756d9a26c95540fb65a35765e9a43949a5814.1774350851.git.dan.carpenter@linaro.org/

Dan Carpenter (2):
  firmware: scmi: Fix setting the function
  pinctrl: scmi: Use standard device tree pin muxing format

 drivers/firmware/scmi/pinctrl.c |   2 +
 drivers/pinctrl/pinctrl-scmi.c  | 151 ++++++++++++--------------------
 2 files changed, 56 insertions(+), 97 deletions(-)

-- 
2.53.0

[PATCH 1/2] firmware: scmi: Fix setting the function

[Dan Carpenter]

Set BIT(10) when the function needs to be set, otherwise the setting is
ignored.

Fixes: 1048331f5d3c ("scmi: pinctrl: add pinctrl driver for SCMI")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/firmware/scmi/pinctrl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
index 47f7a8ad9b81..e670538c87f9 100644
--- a/drivers/firmware/scmi/pinctrl.c
+++ b/drivers/firmware/scmi/pinctrl.c
@@ -259,6 +259,8 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
 	in->attr = 0;
 	in->attr |= FIELD_PREP(GENMASK(9, 2), num_configs);
 	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
+	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
+		in->attr |= BIT(10);
 	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);
 
 	ret = devm_scmi_process_msg(dev, &msg);
-- 
2.53.0

Re: [PATCH 1/2] firmware: scmi: Fix setting the function

[Michal Simek]

On 3/26/26 13:08, Dan Carpenter wrote:
> Set BIT(10) when the function needs to be set, otherwise the setting is
> ignored.
> 
> Fixes: 1048331f5d3c ("scmi: pinctrl: add pinctrl driver for SCMI")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/firmware/scmi/pinctrl.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
> index 47f7a8ad9b81..e670538c87f9 100644
> --- a/drivers/firmware/scmi/pinctrl.c
> +++ b/drivers/firmware/scmi/pinctrl.c
> @@ -259,6 +259,8 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
>   	in->attr = 0;
>   	in->attr |= FIELD_PREP(GENMASK(9, 2), num_configs);
>   	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
> +	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
> +		in->attr |= BIT(10);

hm shouldn't be macros to use for holding that value instead?

As I see GENMASK above is the same case.

>   	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);

Another magic here.

M

Re: [PATCH 1/2] firmware: scmi: Fix setting the function

[Dan Carpenter]

On Fri, Mar 27, 2026 at 12:45:00PM +0100, Michal Simek wrote:
> 
> 
> On 3/26/26 13:08, Dan Carpenter wrote:
> > Set BIT(10) when the function needs to be set, otherwise the setting is
> > ignored.
> > 
> > Fixes: 1048331f5d3c ("scmi: pinctrl: add pinctrl driver for SCMI")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >   drivers/firmware/scmi/pinctrl.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
> > index 47f7a8ad9b81..e670538c87f9 100644
> > --- a/drivers/firmware/scmi/pinctrl.c
> > +++ b/drivers/firmware/scmi/pinctrl.c
> > @@ -259,6 +259,8 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
> >   	in->attr = 0;
> >   	in->attr |= FIELD_PREP(GENMASK(9, 2), num_configs);
> >   	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
> > +	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
> > +		in->attr |= BIT(10);
> 
> hm shouldn't be macros to use for holding that value instead?
> 
> As I see GENMASK above is the same case.
> 

I think adding macros here would make the code less readable...  If we
were going to re-use them then, sure.  But really, the code is shoving
the num_configs into bits 2-9, there isn't any special meaning beyond
that.

> >   	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);
> 
> Another magic here.

Sure, I'll send this patch on Monday.

regards,
dan carpenter

diff --git a/drivers/firmware/scmi/pinctrl.c b/drivers/firmware/scmi/pinctrl.c
index e670538c87f9..2ae02ffc81b8 100644
--- a/drivers/firmware/scmi/pinctrl.c
+++ b/drivers/firmware/scmi/pinctrl.c
@@ -245,7 +245,9 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
 		.out_msg = (u8 *)&out,
 		.out_msg_sz = sizeof(out),
 	};
-	size_t in_sz = sizeof(*in) + (num_configs * sizeof(u32) * 2);
+	/* configs are type/value pairs of size u32 */
+	size_t config_size = (num_configs * sizeof(u32) * 2);
+	size_t in_sz = sizeof(*in) + config_size;
 	int ret;
 
 	in = kzalloc(in_sz, GFP_KERNEL);
@@ -261,7 +263,7 @@ static int scmi_pinctrl_settings_configure_helper(struct udevice *dev,
 	in->attr |= FIELD_PREP(GENMASK(1, 0), select_type);
 	if (function_id != SCMI_PINCTRL_FUNCTION_NONE)
 		in->attr |= BIT(10);
-	memcpy(in->configs, configs, num_configs * sizeof(u32) * 2);
+	memcpy(in->configs, configs, config_size);
 
 	ret = devm_scmi_process_msg(dev, &msg);
 	if (ret)

[PATCH 2/2] pinctrl: scmi: Use standard device tree pin muxing format

[Dan Carpenter]

In the original code, I wrote a custom pin muxing parser but the
upstream device trees wouldn't accept something like that so it would
have complicated mergine the device tree files.

Use the standard device tree format with function and groups:

	pinmux1: pinmux1 {
		function = "f_gpio1";
		groups = "grp_1", "grp_3";
	};

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/pinctrl/pinctrl-scmi.c | 151 ++++++++++++---------------------
 1 file changed, 54 insertions(+), 97 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 63d4f8ffeb54..fa31b573f31e 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -33,20 +33,6 @@ static const struct pinconf_param pinctrl_scmi_conf_params[] = {
 	/* The SCMI spec also include "default", "pull-mode" and "input-value */
 };
 
-static bool valid_selector(struct udevice *dev, enum select_type select_type, u32 selector)
-{
-	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
-
-	if (select_type == SCMI_PIN)
-		return selector < priv->num_pins;
-	if (select_type == SCMI_GROUP)
-		return selector < priv->num_groups;
-	if (select_type == SCMI_FUNCTION)
-		return selector < priv->num_functions;
-
-	return false;
-}
-
 static int pinctrl_scmi_get_pins_count(struct udevice *dev)
 {
 	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
@@ -98,6 +84,38 @@ static const char *pinctrl_scmi_get_function_name(struct udevice *dev, unsigned
 	return (const char *)priv->function_info[selector].name;
 }
 
+static int pinctrl_scmi_get_function_id(struct udevice *dev, const char *function)
+{
+	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
+	int i;
+
+	if (!function)
+		return -EINVAL;
+
+	for (i = 0; i < priv->num_functions; i++) {
+		if (strcmp(priv->function_info[i].name, function) == 0)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int pinctrl_scmi_get_group_id(struct udevice *dev, const char *group)
+{
+	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
+	int i;
+
+	if (!group)
+		return -EINVAL;
+
+	for (i = 0; i < priv->num_groups; i++) {
+		if (strcmp(priv->group_info[i].name, group) == 0)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
 static int pinctrl_scmi_pinmux_set(struct udevice *dev, u32 pin, u32 function)
 {
 	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
@@ -120,96 +138,35 @@ static int pinctrl_scmi_pinmux_group_set(struct udevice *dev, u32 group, u32 fun
 
 static int pinctrl_scmi_set_state(struct udevice *dev, struct udevice *config)
 {
-	struct pinctrl_scmi_priv *priv = dev_get_priv(dev);
-	/* batch the setup into 20 lines at a go (there are 5 u32s in a config) */
-	const int batch_count = 20 * 5;
-	u32 prev_type = -1u;
-	u32 prev_selector;
-	u32 *configs;
-	const u32 *prop;
-	int offset, cnt, len;
-	int ret = 0;
-
-	prop = dev_read_prop(config, "pinmux", &len);
-	if (!prop)
-		return 0;
-
-	if (len % sizeof(u32) * 5) {
-		dev_err(dev, "invalid pin configuration: len=%d\n", len);
-		return -FDT_ERR_BADSTRUCTURE;
-	}
-
-	configs = kcalloc(batch_count, sizeof(u32), GFP_KERNEL);
-	if (!configs)
-		return -ENOMEM;
-
-	offset = 0;
-	cnt = 0;
-	while (offset + 4 < len / sizeof(u32)) {
-		u32 select_type  = fdt32_to_cpu(prop[offset]);
-		u32 selector     = fdt32_to_cpu(prop[offset + 1]);
-		u32 function     = fdt32_to_cpu(prop[offset + 2]);
-		u32 config_type  = fdt32_to_cpu(prop[offset + 3]);
-		u32 config_value = fdt32_to_cpu(prop[offset + 4]);
-
-		if (select_type > SCMI_GROUP ||
-		    !valid_selector(dev, select_type, selector) ||
-		    (function != SCMI_PINCTRL_FUNCTION_NONE &&
-		     function > priv->num_functions)) {
-			dev_err(dev, "invalid pinctrl data (%u %u %u %u %u)\n",
-				select_type, selector, function, config_type,
-				config_value);
-			ret = -EINVAL;
-			goto free;
-		}
+	int function_id, group_id;
+	const char *function;
+	const char **groups;
+	int group_count;
+	int ret;
+	int i;
 
-		if (function != SCMI_PINCTRL_FUNCTION_NONE) {
-			if (cnt) {
-				ret = scmi_pinctrl_settings_configure(dev,
-								      prev_type,
-								      prev_selector,
-								      cnt / 2, configs);
-				if (ret)
-					goto free;
-				prev_type = -1u;
-				cnt = 0;
-			}
-			scmi_pinctrl_set_function(dev, select_type, selector, function);
-			offset += 5;
-			continue;
-		}
+	ret = dev_read_string_index(config, "function", 0, &function);
+	if (ret)
+		return ret;
 
-		if (cnt == batch_count)
-			goto set;
+	function_id = pinctrl_scmi_get_function_id(dev, function);
+	if (function_id < 0)
+		return function_id;
 
-		if (prev_type == -1u)
-			goto store;
+	group_count = dev_read_string_list(config, "groups", &groups);
+	if (group_count < 0)
+		return group_count;
 
-		if (select_type == prev_type && selector == prev_selector)
-			goto store;
-set:
-		ret = scmi_pinctrl_settings_configure(dev, prev_type, prev_selector,
-						      cnt / 2, configs);
+	for (i = 0; i < group_count; i++) {
+		group_id = pinctrl_scmi_get_group_id(dev, groups[i]);
+		if (group_id < 0)
+			return group_id;
+		ret = pinctrl_scmi_pinmux_group_set(dev, group_id, function_id);
 		if (ret)
-			goto free;
-		cnt = 0;
-store:
-		prev_type = select_type;
-		prev_selector = selector;
-		configs[cnt++] = config_type;
-		configs[cnt++] = config_value;
-		offset += 5;
+			return ret;
 	}
 
-	if (cnt)
-		ret = scmi_pinctrl_settings_configure(dev, prev_type, prev_selector,
-						      cnt / 2, configs);
-free:
-	kfree(configs);
-	if (ret)
-		dev_err(dev, "set_state() failed: %d\n", ret);
-
-	return ret;
+	return 0;
 }
 
 static int get_pin_muxing(struct udevice *dev, unsigned int selector,
-- 
2.53.0

Re: [PATCH 2/2] pinctrl: scmi: Use standard device tree pin muxing format

[Linus Walleij]

On Thu, Mar 26, 2026 at 1:08 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:

> In the original code, I wrote a custom pin muxing parser but the
> upstream device trees wouldn't accept something like that so it would
> have complicated mergine the device tree files.
>
> Use the standard device tree format with function and groups:
>
>         pinmux1: pinmux1 {
>                 function = "f_gpio1";
>                 groups = "grp_1", "grp_3";
>         };
>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

I can't verify the code but the change I want is exactly this so:
Acked-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

Re: [PATCH 0/2] u-boot: pinctrl: scmi: Use standard pin muxing format

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>


On Thu, 26 Mar 2026 15:08:12 +0300, Dan Carpenter wrote:
> In my original code I created a custom format for pin muxing but it's
> better to use the standard format.  I also found a bug trying to use
> set the function.  (That code wasn't used previously).
> 
> My GPIO driver is not affected by this change:
> https://lore.kernel.org/all/ff0756d9a26c95540fb65a35765e9a43949a5814.1774350851.git.dan.carpenter@linaro.org/
> 
> [...]

Applied to fsl-qoriq-for-2026.07-rc1, thanks!

[1/2] firmware: scmi: Fix setting the function
      commit: 15594704f794245e14f5c8fa4109be42c88c1b6e
[2/2] pinctrl: scmi: Use standard device tree pin muxing format
      commit: 6aba443c41ae4774b6ec7298dca96c5ba08306cd

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>