[RFC v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[RFC v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Oleksii Moisieiev]

This RFC includes implementation of the new config_{get,set} according to the latest version of
DEN0056E document (v3.2 beta2). Current RFC series covers the implementation of the SCMI protocol
functions without integration to the pinctrl driver, which is under development.
Please review changes to start the review process before I'll be ready to post complete v5 version.

This Patch series is intended to introduce the generic driver for
pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0].

On ARM-based systems, a separate Cortex-M based System Control Processor (SCP)
provides control on pins, as well as with power, clocks, reset controllers. In this case,
kernel should use one of the possible transports, described in [0] to access SCP and
control clocks/power-domains etc. This driver is using SMC transport to communicate with SCP via
SCMI protocol and access to the Pin Control Subsystem.

The provided driver consists of 2 parts:
 - firmware/arm_scmi/pinctrl.c - the SCMI pinctrl protocol inmplementation
   responsible for the communication with SCP firmware.

 - drivers/pinctrl/pinctrl-scmi.c - pinctrl driver, which is using pinctrl
  protocol implementation to access all necessary data.

Configuration:
The scmi-pinctrl driver can be configured using DT bindings.
For example:
/ {
	cpu_scp_shm: scp-shmem@0x53FF0000 {
		compatible = "arm,scmi-shmem";
		reg = <0x0 0x53FF0000 0x0 0x1000>;
	};

	firmware {
		scmi {
			compatible = "arm,scmi-smc";
			arm,smc-id = <0x82000002>;
			shmem = <&cpu_scp_shm>;
			#address-cells = <1>;
			#size-cells = <0>;

			scmi_pinctrl: protocol@19 {
				reg = <0x18>;
				#pinctrl-cells = <0>;

				i2c2_pins: i2c2 {
					groups = "i2c2_a";
					function = "i2c2";
				};
			};
		};
	};
};

&pfc {
	/delete-node/i2c2;
};

So basically, it's enough to move pfc subnode, which configures pin group that should work through
SCMI protocol to scmi_pinctrl node. The current driver implementation is using generic pinctrl dt_node
format.

I've tested this driver on the Renesas H3ULCB Kingfisher board with pinctrl driver ported to the
Arm-trusted-firmware. Unfortunately, not all hardware was possible to test because the Renesas
pinctrl driver has gaps in pins and groups numeration, when Spec [0] requires pins, groups and
functions numerations to be 0..n without gaps.

Also, sharing link to the ATF pinctrl driver I used for testing:
https://github.com/oleksiimoisieiev/arm-trusted-firmware/tree/pinctrl_rcar_m3_up

[0] https://developer.arm.com/documentation/den0056/latest

---
Changes v4 -> v5
   - add new calls to scmi_protocol description for config_{get,set}
Changes v3 -> v4:
   - Fixed MAINTAINERS file description
   - adjusted pinctrl ops position and callback names
   - add trailing coma in scmi_protocol list
   - removed unneeded pi checks
   - corrected selector check
   - resource allocation refactoring
   - scmi_*_info swap params to generate better code
   - style, add trailing coma in definitions
   - reworked protocol@19 format in device-tree bindings
   - ordered config option and object file alphabetically
   - rephrased PINCTRL_SCMI config description
   - formatting fixes, removed blank lines after get_drvdata call
   - code style adjustments
   - add set_drvdata call
   - removed goto label
   - refactoring of the devm resource management
   - removed pctldev != NULL check
   - fix parameter name in pinconf-group-get
   - probe function refactoring
   - removed unneeded pmx checks

Changes v2 -> v3:
   - update get_name calls as suggested by Cristian Marussi
   - fixing comments
   - refactoring of the dt_bindings according to the comments
Changes v1 -> v2:
   - rebase patches to the latest kernel version
   - use protocol helpers in the pinctrl scmi protocol driver implementation
   - reworked pinctrl_ops. Removed similar calls to simplify the interface
   - implementation of the .instance_deinit callback to properly clean resources
   - add description of the pinctrl protocol to the device-tree schema

---
Cristian Marussi (1):
  firmware: arm_scmi: Add optional flags to extended names helper

Oleksii Moisieiev (4):
  drivers: firmware: scmi: Introduce scmi_get_max_msg_size function
  firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
  pinctrl: Implementation of the generic scmi-pinctrl driver
  dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

 .../bindings/firmware/arm,scmi.yaml           |  53 +
 MAINTAINERS                                   |   7 +
 drivers/firmware/arm_scmi/Makefile            |   2 +-
 drivers/firmware/arm_scmi/clock.c             |   2 +-
 drivers/firmware/arm_scmi/common.h            |   3 +
 drivers/firmware/arm_scmi/driver.c            |  25 +-
 drivers/firmware/arm_scmi/perf.c              |   3 +-
 drivers/firmware/arm_scmi/pinctrl.c           | 922 ++++++++++++++++++
 drivers/firmware/arm_scmi/power.c             |   2 +-
 drivers/firmware/arm_scmi/powercap.c          |   2 +-
 drivers/firmware/arm_scmi/protocols.h         |   4 +-
 drivers/firmware/arm_scmi/reset.c             |   3 +-
 drivers/firmware/arm_scmi/sensors.c           |   2 +-
 drivers/firmware/arm_scmi/voltage.c           |   2 +-
 drivers/pinctrl/Kconfig                       |  11 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-scmi.c                | 445 +++++++++
 include/linux/scmi_protocol.h                 |  47 +
 18 files changed, 1525 insertions(+), 11 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
 create mode 100644 drivers/pinctrl/pinctrl-scmi.c

-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC v5 1/5] firmware: arm_scmi: Add optional flags to extended names helper

[Oleksii Moisieiev]

From: Cristian Marussi <cristian.marussi@arm.com>

Some recently added SCMI protocols needs an additional flags parameter to
be able to properly configure the command used to query the extended name
of a resource.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     | 2 +-
 drivers/firmware/arm_scmi/driver.c    | 7 +++++--
 drivers/firmware/arm_scmi/perf.c      | 3 ++-
 drivers/firmware/arm_scmi/power.c     | 2 +-
 drivers/firmware/arm_scmi/powercap.c  | 2 +-
 drivers/firmware/arm_scmi/protocols.h | 3 ++-
 drivers/firmware/arm_scmi/reset.c     | 3 ++-
 drivers/firmware/arm_scmi/sensors.c   | 2 +-
 drivers/firmware/arm_scmi/voltage.c   | 2 +-
 9 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 96060bf90a24..e6e087686e8c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
 		if (SUPPORTS_EXTENDED_NAMES(attributes))
 			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
-						    clk->name,
+						    NULL, clk->name,
 						    SCMI_MAX_STR_SIZE);
 
 		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..729201d8f935 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
  * @ph: A protocol handle reference.
  * @cmd_id: The specific command ID to use.
  * @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
  * @name: A pointer to the preallocated area where the retrieved name will be
  *	  stored as a NULL terminated string.
  * @len: The len in bytes of the @name char array.
@@ -1445,8 +1446,8 @@ struct scmi_msg_resp_domain_name_get {
  * Return: 0 on Succcess
  */
 static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
-					 u8 cmd_id, u32 res_id, char *name,
-					 size_t len)
+					 u8 cmd_id, u32 res_id, u32 *flags,
+					 char *name, size_t len)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -1458,6 +1459,8 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
 		goto out;
 
 	put_unaligned_le32(res_id, t->tx.buf);
+	if (flags)
+		put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
 	resp = t->rx.buf;
 
 	ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ecf5c4de851b..d85d4a0e3605 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -237,7 +237,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 356e83631664..077767d6e902 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags)) {
 		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 	}
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 83b90bde755c..e7ea9210aae1 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -268,7 +268,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	 */
 	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 
 	return ret;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..b3c6314bb4b8 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -256,7 +256,8 @@ struct scmi_fc_info {
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
-				 u8 cmd_id, u32 res_id, char *name, size_t len);
+				 u8 cmd_id, u32 res_id, u32 *flags, char *name,
+				 size_t len);
 	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
 				    struct scmi_iterator_ops *ops,
 				    unsigned int max_resources, u8 msg_id,
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index e9afa8cab730..7217fd7c6afa 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attributes))
 		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0b5853fa9d87..9952a7bc6682 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
 	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attrl))
 		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
-					    s->name, SCMI_MAX_STR_SIZE);
+					    NULL, s->name, SCMI_MAX_STR_SIZE);
 
 	if (s->extended_scalar_attrs) {
 		s->sensor_power = le32_to_cpu(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index eaa8d944926a..36e2df77738c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 			if (SUPPORTS_EXTENDED_NAMES(attributes))
 				ph->hops->extended_name_get(ph,
 							VOLTAGE_DOMAIN_NAME_GET,
-							v->id, v->name,
+							v->id, NULL, v->name,
 							SCMI_MAX_STR_SIZE);
 			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
 				v->async_level_set = true;
-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 1/5] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

On Fri, Oct 27, 2023 at 06:28:09AM +0000, Oleksii Moisieiev wrote:
> From: Cristian Marussi <cristian.marussi@arm.com>
> 
> Some recently added SCMI protocols needs an additional flags parameter to
> be able to properly configure the command used to query the extended name
> of a resource.
> 

Hi,

this patch of mine contained a bug in v4, for which I sent you a fix
a while ago to include; since the patch is unchanged in v5 the bug is still
there: I'll reply to this mail thread with a new version of the whole patch
that indeed will be easier to include in your series instead of this current
buggy version.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

Some recently added SCMI protocols needs an additional flags parameter to
be able to properly configure the command used to query the extended name
of a resource.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     |  2 +-
 drivers/firmware/arm_scmi/driver.c    | 12 ++++++++----
 drivers/firmware/arm_scmi/perf.c      |  2 +-
 drivers/firmware/arm_scmi/power.c     |  2 +-
 drivers/firmware/arm_scmi/powercap.c  |  2 +-
 drivers/firmware/arm_scmi/protocols.h |  3 ++-
 drivers/firmware/arm_scmi/reset.c     |  3 ++-
 drivers/firmware/arm_scmi/sensors.c   |  2 +-
 drivers/firmware/arm_scmi/voltage.c   |  2 +-
 9 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 96060bf90a24..e6e087686e8c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
 		if (SUPPORTS_EXTENDED_NAMES(attributes))
 			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
-						    clk->name,
+						    NULL, clk->name,
 						    SCMI_MAX_STR_SIZE);
 
 		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 87383c05424b..7c7545dea8c6 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
  * @ph: A protocol handle reference.
  * @cmd_id: The specific command ID to use.
  * @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
  * @name: A pointer to the preallocated area where the retrieved name will be
  *	  stored as a NULL terminated string.
  * @len: The len in bytes of the @name char array.
@@ -1445,19 +1446,22 @@ struct scmi_msg_resp_domain_name_get {
  * Return: 0 on Succcess
  */
 static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
-					 u8 cmd_id, u32 res_id, char *name,
-					 size_t len)
+					 u8 cmd_id, u32 res_id, u32 *flags,
+					 char *name, size_t len)
 {
 	int ret;
+	size_t txlen;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_domain_name_get *resp;
 
-	ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id),
-				      sizeof(*resp), &t);
+	txlen = !flags ? sizeof(res_id) : sizeof(res_id) + sizeof(*flags);
+	ret = ph->xops->xfer_get_init(ph, cmd_id, txlen, sizeof(*resp), &t);
 	if (ret)
 		goto out;
 
 	put_unaligned_le32(res_id, t->tx.buf);
+	if (flags)
+		put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
 	resp = t->rx.buf;
 
 	ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index c0cd556fbaae..0902b32dcecd 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -288,7 +288,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET,
-					    dom_info->id, dom_info->name,
+					    dom_info->id, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 
 	if (dom_info->level_indexing_mode) {
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 356e83631664..077767d6e902 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags)) {
 		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 	}
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 244929cb4f3e..86c9a0b5a765 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -270,7 +270,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	 */
 	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 
 	return ret;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..b3c6314bb4b8 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -256,7 +256,8 @@ struct scmi_fc_info {
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
-				 u8 cmd_id, u32 res_id, char *name, size_t len);
+				 u8 cmd_id, u32 res_id, u32 *flags, char *name,
+				 size_t len);
 	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
 				    struct scmi_iterator_ops *ops,
 				    unsigned int max_resources, u8 msg_id,
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index e9afa8cab730..7217fd7c6afa 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attributes))
 		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0b5853fa9d87..9952a7bc6682 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
 	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attrl))
 		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
-					    s->name, SCMI_MAX_STR_SIZE);
+					    NULL, s->name, SCMI_MAX_STR_SIZE);
 
 	if (s->extended_scalar_attrs) {
 		s->sensor_power = le32_to_cpu(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index eaa8d944926a..36e2df77738c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 			if (SUPPORTS_EXTENDED_NAMES(attributes))
 				ph->hops->extended_name_get(ph,
 							VOLTAGE_DOMAIN_NAME_GET,
-							v->id, v->name,
+							v->id, NULL, v->name,
 							SCMI_MAX_STR_SIZE);
 			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
 				v->async_level_set = true;
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

On Thu, Nov 02, 2023 at 07:19:45AM +0000, Cristian Marussi wrote:
> Some recently added SCMI protocols needs an additional flags parameter to
> be able to properly configure the command used to query the extended name
> of a resource.
> 

Hi Oleksii,

I posted this independently from your series:

https://lore.kernel.org/lkml/20231114145449.3136412-1-cristian.marussi@arm.com

(and forgot to CC you :< ...)

Thanks,
Cristian

> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c     |  2 +-
>  drivers/firmware/arm_scmi/driver.c    | 12 ++++++++----
>  drivers/firmware/arm_scmi/perf.c      |  2 +-
>  drivers/firmware/arm_scmi/power.c     |  2 +-
>  drivers/firmware/arm_scmi/powercap.c  |  2 +-
>  drivers/firmware/arm_scmi/protocols.h |  3 ++-
>  drivers/firmware/arm_scmi/reset.c     |  3 ++-
>  drivers/firmware/arm_scmi/sensors.c   |  2 +-
>  drivers/firmware/arm_scmi/voltage.c   |  2 +-
>  9 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 96060bf90a24..e6e087686e8c 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>  	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
>  		if (SUPPORTS_EXTENDED_NAMES(attributes))
>  			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
> -						    clk->name,
> +						    NULL, clk->name,
>  						    SCMI_MAX_STR_SIZE);
>  
>  		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 87383c05424b..7c7545dea8c6 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
>   * @ph: A protocol handle reference.
>   * @cmd_id: The specific command ID to use.
>   * @res_id: The specific resource ID to use.
> + * @flags: A pointer to specific flags to use, if any.
>   * @name: A pointer to the preallocated area where the retrieved name will be
>   *	  stored as a NULL terminated string.
>   * @len: The len in bytes of the @name char array.
> @@ -1445,19 +1446,22 @@ struct scmi_msg_resp_domain_name_get {
>   * Return: 0 on Succcess
>   */
>  static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
> -					 u8 cmd_id, u32 res_id, char *name,
> -					 size_t len)
> +					 u8 cmd_id, u32 res_id, u32 *flags,
> +					 char *name, size_t len)
>  {
>  	int ret;
> +	size_t txlen;
>  	struct scmi_xfer *t;
>  	struct scmi_msg_resp_domain_name_get *resp;
>  
> -	ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id),
> -				      sizeof(*resp), &t);
> +	txlen = !flags ? sizeof(res_id) : sizeof(res_id) + sizeof(*flags);
> +	ret = ph->xops->xfer_get_init(ph, cmd_id, txlen, sizeof(*resp), &t);
>  	if (ret)
>  		goto out;
>  
>  	put_unaligned_le32(res_id, t->tx.buf);
> +	if (flags)
> +		put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
>  	resp = t->rx.buf;
>  
>  	ret = ph->xops->do_xfer(ph, t);
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index c0cd556fbaae..0902b32dcecd 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -288,7 +288,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
>  	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
>  	    SUPPORTS_EXTENDED_NAMES(flags))
>  		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET,
> -					    dom_info->id, dom_info->name,
> +					    dom_info->id, NULL, dom_info->name,
>  					    SCMI_MAX_STR_SIZE);
>  
>  	if (dom_info->level_indexing_mode) {
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 356e83631664..077767d6e902 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
>  	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
>  	    SUPPORTS_EXTENDED_NAMES(flags)) {
>  		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
> -					    domain, dom_info->name,
> +					    domain, NULL, dom_info->name,
>  					    SCMI_MAX_STR_SIZE);
>  	}
>  
> diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
> index 244929cb4f3e..86c9a0b5a765 100644
> --- a/drivers/firmware/arm_scmi/powercap.c
> +++ b/drivers/firmware/arm_scmi/powercap.c
> @@ -270,7 +270,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
>  	 */
>  	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
>  		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
> -					    domain, dom_info->name,
> +					    domain, NULL, dom_info->name,
>  					    SCMI_MAX_STR_SIZE);
>  
>  	return ret;
> diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> index 78e1a01eb656..b3c6314bb4b8 100644
> --- a/drivers/firmware/arm_scmi/protocols.h
> +++ b/drivers/firmware/arm_scmi/protocols.h
> @@ -256,7 +256,8 @@ struct scmi_fc_info {
>   */
>  struct scmi_proto_helpers_ops {
>  	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
> -				 u8 cmd_id, u32 res_id, char *name, size_t len);
> +				 u8 cmd_id, u32 res_id, u32 *flags, char *name,
> +				 size_t len);
>  	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
>  				    struct scmi_iterator_ops *ops,
>  				    unsigned int max_resources, u8 msg_id,
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index e9afa8cab730..7217fd7c6afa 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
>  	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
>  	    SUPPORTS_EXTENDED_NAMES(attributes))
>  		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
> -					    dom_info->name, SCMI_MAX_STR_SIZE);
> +					    NULL, dom_info->name,
> +					    SCMI_MAX_STR_SIZE);
>  
>  	return ret;
>  }
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 0b5853fa9d87..9952a7bc6682 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
>  	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
>  	    SUPPORTS_EXTENDED_NAMES(attrl))
>  		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
> -					    s->name, SCMI_MAX_STR_SIZE);
> +					    NULL, s->name, SCMI_MAX_STR_SIZE);
>  
>  	if (s->extended_scalar_attrs) {
>  		s->sensor_power = le32_to_cpu(sdesc->power);
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> index eaa8d944926a..36e2df77738c 100644
> --- a/drivers/firmware/arm_scmi/voltage.c
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
>  			if (SUPPORTS_EXTENDED_NAMES(attributes))
>  				ph->hops->extended_name_get(ph,
>  							VOLTAGE_DOMAIN_NAME_GET,
> -							v->id, v->name,
> +							v->id, NULL, v->name,
>  							SCMI_MAX_STR_SIZE);
>  			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
>  				v->async_level_set = true;
> -- 
> 2.42.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC v5 2/5] drivers: firmware: scmi: Introduce scmi_get_max_msg_size function

[Oleksii Moisieiev]

Current SCMI implementation supports only receiving arrays from the
SCMI server and provides helpers to process received data. It uses
msg_max_size value to determine maximum message size that can be
transmitted via selected protocol. When sending arrays to SCMI server
this value should be checked by the Client driver to prevent
overflowing protocol buffers.
That's why scmi_get_max_msg_size call was introduced.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---
 drivers/firmware/arm_scmi/common.h |  3 +++
 drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index c46dc5215af7..3db97f59bc59 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -286,6 +286,9 @@ int scmi_xfer_raw_inflight_register(const struct scmi_handle *handle,
 int scmi_xfer_raw_wait_for_message_response(struct scmi_chan_info *cinfo,
 					    struct scmi_xfer *xfer,
 					    unsigned int timeout_ms);
+
+int scmi_get_max_msg_size(const struct scmi_protocol_handle *ph);
+
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
 extern const struct scmi_desc scmi_mailbox_desc;
 #endif
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 729201d8f935..f15e9b2b21f3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1152,6 +1152,22 @@ int scmi_xfer_raw_wait_for_message_response(struct scmi_chan_info *cinfo,
 	return ret;
 }
 
+/**
+ * scmi_get_max_msg_size  - An helper to get currently configured
+ * maximum message size.
+ *
+ * @ph: SCMI protocol handle
+ *
+ * Return: Maximum message size for the current protocol.
+ */
+int scmi_get_max_msg_size(const struct scmi_protocol_handle *ph)
+{
+	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+	struct scmi_info *info = handle_to_scmi_info(pi->handle);
+
+	return info->desc->max_msg_size;
+}
+
 /**
  * do_xfer() - Do one transfer
  *
-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 2/5] drivers: firmware: scmi: Introduce scmi_get_max_msg_size function

[Cristian Marussi]

On Fri, Oct 27, 2023 at 06:28:09AM +0000, Oleksii Moisieiev wrote:
> Current SCMI implementation supports only receiving arrays from the
> SCMI server and provides helpers to process received data. It uses
> msg_max_size value to determine maximum message size that can be
> transmitted via selected protocol. When sending arrays to SCMI server
> this value should be checked by the Client driver to prevent
> overflowing protocol buffers.
> That's why scmi_get_max_msg_size call was introduced.
> 

Hi Oleksii,

indeed given the new variable sized v3.2 SCMI requests (instead of
responses) this common helper is now needed for the protocols to be
able to properly size and chunk their command requests, BUT this is
a new core helper that has potentially to be available to any future
protocol so it has to be exposed as a common helpers in helpers_ops
(like iterators or extended_name helpers), if NOT this common method
won't be available to protocols when compiled as distinct loadable
modules (vendor-modules can be defined and built as LKM)

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 2/5] drivers: firmware: scmi: Introduce scmi_get_max_msg_size function

[Oleksii Moisieiev]

Hi Cristian,

Just found an interesting note in the PINCTRL_CONFIG_SET command 
description:

The maximum value of this field is limited by
the transport used. The agent needs to specify
this field such that the entire command can be
accommodated within the transport chosen.

Furthermore, I observed the absence of a skip_configs parameter.

 From my understanding, this implies that the maximum number of 
configurations should not exceed the msg_max_size allowed by the 
protocol, enabling the transmission of only one message to the SCMI 
server at a time.

Given this constraint, it seems we might not require additional helper 
functions. We could potentially just verify against msg_max_size.

Best regards,
Oleksii

On 02.11.23 09:29, Cristian Marussi wrote:
> On Fri, Oct 27, 2023 at 06:28:09AM +0000, Oleksii Moisieiev wrote:
>> Current SCMI implementation supports only receiving arrays from the
>> SCMI server and provides helpers to process received data. It uses
>> msg_max_size value to determine maximum message size that can be
>> transmitted via selected protocol. When sending arrays to SCMI server
>> this value should be checked by the Client driver to prevent
>> overflowing protocol buffers.
>> That's why scmi_get_max_msg_size call was introduced.
>>
> 
> Hi Oleksii,
> 
> indeed given the new variable sized v3.2 SCMI requests (instead of
> responses) this common helper is now needed for the protocols to be
> able to properly size and chunk their command requests, BUT this is
> a new core helper that has potentially to be available to any future
> protocol so it has to be exposed as a common helpers in helpers_ops
> (like iterators or extended_name helpers), if NOT this common method
> won't be available to protocols when compiled as distinct loadable
> modules (vendor-modules can be defined and built as LKM)
> 
> Thanks,
> Cristian
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 2/5] drivers: firmware: scmi: Introduce scmi_get_max_msg_size function

[Cristian Marussi]

On Thu, Nov 02, 2023 at 01:57:24PM +0000, Oleksii Moisieiev wrote:
> Hi Cristian,
> 

Hi,

> Just found an interesting note in the PINCTRL_CONFIG_SET command 
> description:
> 
> The maximum value of this field is limited by
> the transport used. The agent needs to specify
> this field such that the entire command can be
> accommodated within the transport chosen.
> 

Yes I am aware of that.

> Furthermore, I observed the absence of a skip_configs parameter.
> 
>  From my understanding, this implies that the maximum number of 
> configurations should not exceed the msg_max_size allowed by the 
> protocol, enabling the transmission of only one message to the SCMI 
> server at a time.
>

Yes that is correct, my understanding is that the transmitter is in
charge of building a message whose payload can fit into the maximum
message size allowed by the underlying configured transport.

> Given this constraint, it seems we might not require additional helper 
> functions. We could potentially just verify against msg_max_size.
> 

Indeed for that reason the scmi_get_max_msg_size that you introduced is
just enough since it allows you to peek into the transport and get the
max_msg_size...the misunderstanding is around the fact that I was simply
meaning that you should plug it into some new helper_ops so that yo can
call it like:

  max_msg = ph->hops->get_max_msg_size(ph);

(like iterators or get_extended_name)

Because in this way you could use it also when the protocol is build as
a loadable module, thing that now it is possible only for vendor defined
protocols, but we could also easily switch all the base protocols to be
selectable via Kconfig and =m in the future (if ever)

Your helper is fine by itself it is just that it cannot be called by a
protocol defined to loaded as a module because the symbol is not
exported and, indeed, we introduced the ph->hops thing just for this
reason, i.e. to have a set of common protocol utilities that can be
called even from loadable modules protocols without the need to export
every single symbol.

The reference to iterators and extended_name was misleading
probably...my bad...or I am still missing something :D

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC v5 3/5] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Oleksii Moisieiev]

Add basic implementation of the SCMI v3.2 pincontrol protocol.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---
Changes v4 -> v5
  - add new calls to scmi_protocol description for config_{get,set}
Changes v3 -> v4
  - Fixed MAINTAINERS file description
  - adjusted pinctrl ops position and callback names
  - add trailing coma in scmi_protocol list
  - removed unneeded pi checks
  - corrected selector check
  - resource allocation refactoring
  - scmi_*_info swap params to generate better code
  - style, add trailing coma in definitions

---
 MAINTAINERS                           |   6 +
 drivers/firmware/arm_scmi/Makefile    |   2 +-
 drivers/firmware/arm_scmi/driver.c    |   2 +
 drivers/firmware/arm_scmi/pinctrl.c   | 922 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h |   1 +
 include/linux/scmi_protocol.h         |  47 ++
 6 files changed, 979 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/pinctrl.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0dab9737ec16..2d81d00e5f4f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20522,6 +20522,12 @@ F:	include/linux/sc[mp]i_protocol.h
 F:	include/trace/events/scmi.h
 F:	include/uapi/linux/virtio_scmi.h
 
+PINCTRL DRIVER FOR SYSTEM CONTROL MANAGEMENT INTERFACE (SCMI)
+M:	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
+L:	linux-arm-kernel@lists.infradead.org
+S:	Maintained
+F:	drivers/firmware/arm_scmi/pinctrl.c
+
 SYSTEM RESET/SHUTDOWN DRIVERS
 M:	Sebastian Reichel <sre@kernel.org>
 L:	linux-pm@vger.kernel.org
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index b31d78fa66cc..603430ec0bfe 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
 scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
 scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f15e9b2b21f3..31a0da37eeb3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3040,6 +3040,7 @@ static int __init scmi_driver_init(void)
 	scmi_voltage_register();
 	scmi_system_register();
 	scmi_powercap_register();
+	scmi_pinctrl_register();
 
 	return platform_driver_register(&scmi_driver);
 }
@@ -3057,6 +3058,7 @@ static void __exit scmi_driver_exit(void)
 	scmi_voltage_unregister();
 	scmi_system_unregister();
 	scmi_powercap_unregister();
+	scmi_pinctrl_unregister();
 
 	scmi_transports_exit();
 
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
new file mode 100644
index 000000000000..e5cbd976b6ae
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,922 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Pinctrl Protocol
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include "common.h"
+#include "protocols.h"
+
+#define REG_TYPE_BITS GENMASK(9, 8)
+#define REG_CONFIG GENMASK(7, 0)
+
+#define GET_GROUPS_NR(x)	le32_get_bits((x), GENMASK(31, 16))
+#define GET_PINS_NR(x)		le32_get_bits((x), GENMASK(15, 0))
+#define GET_FUNCTIONS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
+
+#define EXT_NAME_FLAG(x)	le32_get_bits((x), BIT(31))
+#define NUM_ELEMS(x)		le32_get_bits((x), GENMASK(15, 0))
+
+#define REMAINING(x)		le32_get_bits((x), GENMASK(31, 16))
+#define RETURNED(x)		le32_get_bits((x), GENMASK(11, 0))
+
+enum scmi_pinctrl_protocol_cmd {
+	PINCTRL_ATTRIBUTES = 0x3,
+	PINCTRL_LIST_ASSOCIATIONS = 0x4,
+	PINCTRL_CONFIG_GET = 0x5,
+	PINCTRL_CONFIG_SET = 0x6,
+	PINCTRL_FUNCTION_SELECT = 0x7,
+	PINCTRL_REQUEST = 0x8,
+	PINCTRL_RELEASE = 0x9,
+	PINCTRL_NAME_GET = 0xa,
+	PINCTRL_SET_PERMISSIONS = 0xb
+};
+
+struct scmi_msg_conf_set {
+	__le32 identifier;
+	__le32 attributes;
+	__le32 configs[];
+};
+
+struct scmi_msg_conf_get {
+	__le32 identifier;
+	__le32 attributes;
+};
+
+struct scmi_resp_conf_get {
+	__le32 num_configs;
+	__le32 configs[];
+};
+
+struct scmi_msg_pinctrl_protocol_attributes {
+	__le32 attributes_low;
+	__le32 attributes_high;
+};
+
+struct scmi_msg_pinctrl_attributes {
+	__le32 identifier;
+	__le32 flags;
+};
+
+struct scmi_resp_pinctrl_attributes {
+	__le32 attributes;
+	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
+};
+
+struct scmi_msg_pinctrl_list_assoc {
+	__le32 identifier;
+	__le32 flags;
+	__le32 index;
+};
+
+struct scmi_resp_pinctrl_list_assoc {
+	__le32 flags;
+	__le16 array[];
+};
+
+struct scmi_msg_func_set {
+	__le32 identifier;
+	__le32 function_id;
+	__le32 flags;
+};
+
+struct scmi_msg_request {
+	__le32 identifier;
+	__le32 flags;
+};
+
+struct scmi_group_info {
+	char name[SCMI_MAX_STR_SIZE];
+	bool present;
+	unsigned int *group_pins;
+	unsigned int nr_pins;
+};
+
+struct scmi_function_info {
+	char name[SCMI_MAX_STR_SIZE];
+	bool present;
+	unsigned int *groups;
+	unsigned int nr_groups;
+};
+
+struct scmi_pin_info {
+	char name[SCMI_MAX_STR_SIZE];
+	bool present;
+};
+
+struct scmi_pinctrl_info {
+	u32 version;
+	int nr_groups;
+	int nr_functions;
+	int nr_pins;
+	struct scmi_group_info *groups;
+	struct scmi_function_info *functions;
+	struct scmi_pin_info *pins;
+};
+
+static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
+				       struct scmi_pinctrl_info *pi)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_pinctrl_protocol_attributes *attr;
+
+	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t);
+	if (ret)
+		return ret;
+
+	attr = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
+		pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
+		pi->nr_pins = GET_PINS_NR(attr->attributes_low);
+	}
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
+				  enum scmi_pinctrl_selector_type type)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	switch (type) {
+	case PIN_TYPE:
+		return pi->nr_pins;
+	case GROUP_TYPE:
+		return pi->nr_groups;
+	case FUNCTION_TYPE:
+		return pi->nr_functions;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
+				    u32 identifier,
+				    enum scmi_pinctrl_selector_type type)
+{
+	int value;
+
+	value = scmi_pinctrl_count_get(ph, type);
+	if (value < 0)
+		return value;
+
+	if (identifier >= value)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
+				   enum scmi_pinctrl_selector_type type,
+				   u32 selector, char *name,
+				   unsigned int *n_elems)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_pinctrl_attributes *tx;
+	struct scmi_resp_pinctrl_attributes *rx;
+
+	if (!name)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, selector, type);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), sizeof(*rx), &t);
+	if (ret)
+		return ret;
+
+	tx = t->tx.buf;
+	rx = t->rx.buf;
+	tx->identifier = cpu_to_le32(selector);
+	tx->flags = cpu_to_le32(type);
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		if (n_elems)
+			*n_elems = NUM_ELEMS(rx->attributes);
+
+		strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && EXT_NAME_FLAG(rx->attributes))
+		ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
+					    (u32 *)&type, name,
+					    SCMI_MAX_STR_SIZE);
+	return ret;
+}
+
+struct scmi_pinctrl_ipriv {
+	u32 selector;
+	enum scmi_pinctrl_selector_type type;
+	unsigned int *array;
+};
+
+static void iter_pinctrl_assoc_prepare_message(void *message,
+					       unsigned int desc_index,
+					       const void *priv)
+{
+	struct scmi_msg_pinctrl_list_assoc *msg = message;
+	const struct scmi_pinctrl_ipriv *p = priv;
+
+	msg->identifier = cpu_to_le32(p->selector);
+	msg->flags = cpu_to_le32(p->type);
+	/* Set the number of OPPs to be skipped/already read */
+	msg->index = cpu_to_le32(desc_index);
+}
+
+static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
+					   const void *response, void *priv)
+{
+	const struct scmi_resp_pinctrl_list_assoc *r = response;
+
+	st->num_returned = RETURNED(r->flags);
+	st->num_remaining = REMAINING(r->flags);
+
+	return 0;
+}
+
+static int
+iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
+				    const void *response,
+				    struct scmi_iterator_state *st, void *priv)
+{
+	const struct scmi_resp_pinctrl_list_assoc *r = response;
+	struct scmi_pinctrl_ipriv *p = priv;
+
+	p->array[st->desc_index + st->loop_idx] =
+		le16_to_cpu(r->array[st->loop_idx]);
+
+	return 0;
+}
+
+static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
+					  u32 selector,
+					  enum scmi_pinctrl_selector_type type,
+					  u16 size, unsigned int *array)
+{
+	int ret;
+	void *iter;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_pinctrl_assoc_prepare_message,
+		.update_state = iter_pinctrl_assoc_update_state,
+		.process_response = iter_pinctrl_assoc_process_response,
+	};
+	struct scmi_pinctrl_ipriv ipriv = {
+		.selector = selector,
+		.type = type,
+		.array = array,
+	};
+
+	if (!array || !size || type == PIN_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, selector, type);
+	if (ret)
+		return ret;
+
+	iter = ph->hops->iter_response_init(ph, &ops, size,
+					    PINCTRL_LIST_ASSOCIATIONS,
+					    sizeof(struct scmi_msg_pinctrl_list_assoc),
+					    &ipriv);
+
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
+struct scmi_conf_get_ipriv {
+	u32 selector;
+	enum scmi_pinctrl_selector_type type;
+	u8 all;
+	u8 *config_types;
+	unsigned long *config_values;
+};
+
+static void iter_pinctrl_conf_get_prepare_message(void *message,
+						  unsigned int desc_index,
+						  const void *priv)
+{
+	struct scmi_msg_conf_get *msg = message;
+	const struct scmi_conf_get_ipriv *p = priv;
+	u32 attributes;
+
+	msg->identifier = cpu_to_le32(p->selector);
+	attributes = FIELD_PREP(BIT(18), p->all) |
+		     FIELD_PREP(GENMASK(17, 16), p->type);
+
+	if (p->all)
+		attributes |= FIELD_PREP(GENMASK(15, 8), desc_index);
+	else
+		attributes |= FIELD_PREP(GENMASK(7, 0), p->config_types[0]);
+
+	msg->attributes = cpu_to_le32(attributes);
+	msg->identifier = cpu_to_le32(p->selector);
+}
+
+static int iter_pinctrl_conf_get_update_state(struct scmi_iterator_state *st,
+					      const void *response, void *priv)
+{
+	const struct scmi_resp_conf_get *r = response;
+
+	st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
+	st->num_remaining = le32_get_bits(r->num_configs, GENMASK(31, 24));
+
+	return 0;
+}
+
+static int iter_pinctrl_conf_get_process_response(const struct scmi_protocol_handle *ph,
+						  const void *response,
+						  struct scmi_iterator_state *st, void *priv)
+{
+	const struct scmi_resp_conf_get *r = response;
+	struct scmi_conf_get_ipriv *p = priv;
+
+	if (!p->all) {
+		if (p->config_types[0] !=
+		    le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
+			return -EINVAL;
+	} else {
+		p->config_types[st->desc_index + st->loop_idx] =
+			le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0));
+	}
+
+	p->config_values[st->desc_index + st->loop_idx] =
+		le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
+
+	return 0;
+}
+
+static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
+				   u32 selector,
+				   enum scmi_pinctrl_selector_type type,
+				   u8 config_type, unsigned long *config_value)
+{
+	int ret;
+	void *iter;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_pinctrl_conf_get_prepare_message,
+		.update_state = iter_pinctrl_conf_get_update_state,
+		.process_response = iter_pinctrl_conf_get_process_response,
+	};
+	struct scmi_conf_get_ipriv ipriv = {
+		.selector = selector,
+		.type = type,
+		.all = 0,
+		.config_types = &config_type,
+		.config_values = config_value,
+	};
+
+	if (!config_value || type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, selector, type);
+	if (ret)
+		return ret;
+
+	iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_CONFIG_GET,
+					    sizeof(struct scmi_msg_conf_get),
+					    &ipriv);
+
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
+static int scmi_pinctrl_config_get_all(const struct scmi_protocol_handle *ph,
+				       u32 selector,
+				       enum scmi_pinctrl_selector_type type,
+				       u16 size, u8 *config_types,
+				       unsigned long *config_values)
+{
+	int ret;
+	void *iter;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_pinctrl_conf_get_prepare_message,
+		.update_state = iter_pinctrl_conf_get_update_state,
+		.process_response = iter_pinctrl_conf_get_process_response,
+	};
+	struct scmi_conf_get_ipriv ipriv = {
+		.selector = selector,
+		.type = type,
+		.all = 1,
+		.config_types = config_types,
+		.config_values = config_values,
+	};
+
+	if (!config_values || !config_types || type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, selector, type);
+	if (ret)
+		return ret;
+
+	iter = ph->hops->iter_response_init(ph, &ops, size, PINCTRL_CONFIG_GET,
+					    sizeof(struct scmi_msg_conf_get),
+					    &ipriv);
+
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
+static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph,
+				   u32 selector,
+				   enum scmi_pinctrl_selector_type type,
+				   unsigned int nr_configs, u8 *config_types,
+				   unsigned long *config_values)
+{
+	struct scmi_xfer *t;
+	struct scmi_msg_conf_set *tx;
+	u32 attributes;
+	int ret, i;
+	unsigned int configs_in_chunk, conf_num = 0;
+	unsigned int chunk;
+	int max_msg_size = scmi_get_max_msg_size(ph);
+
+	if (!config_types || !config_values || type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, selector, type);
+	if (ret)
+		return ret;
+
+	configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(unsigned long) * 2);
+	while (conf_num < nr_configs) {
+		chunk = (nr_configs - conf_num > configs_in_chunk) ? configs_in_chunk :
+			nr_configs - conf_num;
+
+		ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_SET,
+					      sizeof(*tx) + chunk * 2 * sizeof(unsigned long),
+					      0, &t);
+		if (ret)
+			return ret;
+
+		tx = t->tx.buf;
+		tx->identifier = cpu_to_le32(selector);
+		attributes = FIELD_PREP(GENMASK(1, 0), type) |
+			FIELD_PREP(GENMASK(9, 2), chunk);
+		tx->attributes = cpu_to_le32(attributes);
+
+		for (i = 0; i < chunk; i++) {
+			tx->configs[i * 2] = cpu_to_le32(config_types[i]);
+			tx->configs[i * 2 + 1] = cpu_to_le32(config_values[i]);
+		}
+
+		ret = ph->xops->do_xfer(ph, t);
+
+		ph->xops->xfer_put(ph, t);
+
+		if (ret)
+			break;
+
+		conf_num += chunk;
+	}
+
+	return ret;
+}
+
+static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
+					u32 identifier,
+					enum scmi_pinctrl_selector_type type,
+					u32 function_id)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_func_set *tx;
+
+	if (type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, identifier, type);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, PINCTRL_FUNCTION_SELECT, sizeof(*tx), 0, &t);
+	if (ret)
+		return ret;
+
+	tx = t->tx.buf;
+	tx->identifier = cpu_to_le32(identifier);
+	tx->function_id = cpu_to_le32(function_id);
+	tx->flags = cpu_to_le32(type);
+
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
+				u32 identifier,
+				enum scmi_pinctrl_selector_type type)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_request *tx;
+
+	if (type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, identifier, type);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0, &t);
+
+	tx = t->tx.buf;
+	tx->identifier = cpu_to_le32(identifier);
+	tx->flags = cpu_to_le32(type);
+
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
+				    u32 pin)
+{
+	return scmi_pinctrl_request(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
+			     u32 identifier,
+			     enum scmi_pinctrl_selector_type type)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_request *tx;
+
+	if (type == FUNCTION_TYPE)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_validate_id(ph, identifier, type);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
+
+	tx = t->tx.buf;
+	tx->identifier = cpu_to_le32(identifier);
+	tx->flags = cpu_to_le32(type);
+
+	ret = ph->xops->do_xfer(ph, t);
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
+{
+	return scmi_pinctrl_free(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
+				       u32 selector,
+				       struct scmi_group_info *group)
+{
+	int ret;
+
+	if (!group)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
+				      group->name,
+				      &group->nr_pins);
+	if (ret)
+		return ret;
+
+	if (!group->nr_pins) {
+		dev_err(ph->dev, "Group %d has 0 elements", selector);
+		return -ENODATA;
+	}
+
+	group->group_pins = kmalloc_array(group->nr_pins, sizeof(*group->group_pins), GFP_KERNEL);
+	if (!group->group_pins)
+		return -ENOMEM;
+
+	ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
+					     group->nr_pins, group->group_pins);
+	if (ret) {
+		kfree(group->group_pins);
+		return ret;
+	}
+
+	group->present = true;
+	return 0;
+}
+
+static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
+				       u32 selector, const char **name)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	if (!name)
+		return -EINVAL;
+
+	if (selector >= pi->nr_groups)
+		return -EINVAL;
+
+	if (!pi->groups[selector].present) {
+		int ret;
+
+		ret = scmi_pinctrl_get_group_info(ph, selector,
+						  &pi->groups[selector]);
+		if (ret)
+			return ret;
+	}
+
+	*name = pi->groups[selector].name;
+
+	return 0;
+}
+
+static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
+				       u32 selector, const unsigned int **pins,
+				       unsigned int *nr_pins)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	if (!pins || !nr_pins)
+		return -EINVAL;
+
+	if (selector >= pi->nr_groups)
+		return -EINVAL;
+
+	if (!pi->groups[selector].present) {
+		int ret;
+
+		ret = scmi_pinctrl_get_group_info(ph, selector,
+						  &pi->groups[selector]);
+		if (ret)
+			return ret;
+	}
+
+	*pins = pi->groups[selector].group_pins;
+	*nr_pins = pi->groups[selector].nr_pins;
+
+	return 0;
+}
+
+static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
+					  u32 selector,
+					  struct scmi_function_info *func)
+{
+	int ret;
+
+	if (!func)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
+				      func->name,
+				      &func->nr_groups);
+	if (ret)
+		return ret;
+
+	if (!func->nr_groups) {
+		dev_err(ph->dev, "Function %d has 0 elements", selector);
+		return -ENODATA;
+	}
+
+	func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups), GFP_KERNEL);
+	if (!func->groups)
+		return -ENOMEM;
+
+	ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
+					     func->nr_groups, func->groups);
+	if (ret) {
+		kfree(func->groups);
+		return ret;
+	}
+
+	func->present = true;
+	return 0;
+}
+
+static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
+					  u32 selector, const char **name)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	if (!name)
+		return -EINVAL;
+
+	if (selector >= pi->nr_functions)
+		return -EINVAL;
+
+	if (!pi->functions[selector].present) {
+		int ret;
+
+		ret = scmi_pinctrl_get_function_info(ph, selector,
+						     &pi->functions[selector]);
+		if (ret)
+			return ret;
+	}
+
+	*name = pi->functions[selector].name;
+	return 0;
+}
+
+static int scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
+					    u32 selector,
+					    unsigned int *nr_groups,
+					    const unsigned int **groups)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	if (!groups || !nr_groups)
+		return -EINVAL;
+
+	if (selector >= pi->nr_functions)
+		return -EINVAL;
+
+	if (!pi->functions[selector].present) {
+		int ret;
+
+		ret = scmi_pinctrl_get_function_info(ph, selector,
+						     &pi->functions[selector]);
+		if (ret)
+			return ret;
+	}
+
+	*groups = pi->functions[selector].groups;
+	*nr_groups = pi->functions[selector].nr_groups;
+
+	return 0;
+}
+
+static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
+				u32 selector, u32 group)
+{
+	return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
+					    selector);
+}
+
+static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
+				     u32 selector, struct scmi_pin_info *pin)
+{
+	int ret;
+
+	if (!pin)
+		return -EINVAL;
+
+	ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
+				      pin->name, NULL);
+	if (ret)
+		return ret;
+
+	pin->present = true;
+	return 0;
+}
+
+static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
+				     u32 selector, const char **name)
+{
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	if (!name)
+		return -EINVAL;
+
+	if (selector >= pi->nr_pins)
+		return -EINVAL;
+
+	if (!pi->pins[selector].present) {
+		int ret;
+
+		ret = scmi_pinctrl_get_pin_info(ph, selector,
+						&pi->pins[selector]);
+		if (ret)
+			return ret;
+	}
+
+	*name = pi->pins[selector].name;
+
+	return 0;
+}
+
+static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
+				 u32 selector,
+				 enum scmi_pinctrl_selector_type type,
+				 const char **name)
+{
+	switch (type) {
+	case PIN_TYPE:
+		return scmi_pinctrl_get_pin_name(ph, selector, name);
+	case GROUP_TYPE:
+		return scmi_pinctrl_get_group_name(ph, selector, name);
+	case FUNCTION_TYPE:
+		return scmi_pinctrl_get_function_name(ph, selector, name);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
+	.count_get = scmi_pinctrl_count_get,
+	.name_get = scmi_pinctrl_name_get,
+	.group_pins_get = scmi_pinctrl_group_pins_get,
+	.function_groups_get = scmi_pinctrl_function_groups_get,
+	.mux_set = scmi_pinctrl_mux_set,
+	.config_get = scmi_pinctrl_config_get,
+	.config_get_all = scmi_pinctrl_config_get_all,
+	.config_set = scmi_pinctrl_config_set,
+	.pin_request = scmi_pinctrl_pin_request,
+	.pin_free = scmi_pinctrl_pin_free,
+};
+
+static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
+{
+	int ret;
+	u32 version;
+	struct scmi_pinctrl_info *pinfo;
+
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
+
+	dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
+		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
+	if (!pinfo)
+		return -ENOMEM;
+
+	ret = scmi_pinctrl_attributes_get(ph, pinfo);
+	if (ret)
+		return ret;
+
+	pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
+				   sizeof(*pinfo->pins),
+				   GFP_KERNEL);
+	if (!pinfo->pins)
+		return -ENOMEM;
+
+	pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
+				     sizeof(*pinfo->groups),
+				     GFP_KERNEL);
+	if (!pinfo->groups)
+		return -ENOMEM;
+
+	pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
+					sizeof(*pinfo->functions),
+					GFP_KERNEL);
+	if (!pinfo->functions)
+		return -ENOMEM;
+
+	pinfo->version = version;
+
+	return ph->set_priv(ph, pinfo);
+}
+
+static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
+{
+	int i;
+	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+	for (i = 0; i < pi->nr_groups; i++) {
+		if (pi->groups[i].present) {
+			kfree(pi->groups[i].group_pins);
+			pi->groups[i].present = false;
+		}
+	}
+
+	for (i = 0; i < pi->nr_functions; i++) {
+		if (pi->functions[i].present) {
+			kfree(pi->functions[i].groups);
+			pi->functions[i].present = false;
+		}
+	}
+
+	return 0;
+}
+
+static const struct scmi_protocol scmi_pinctrl = {
+	.id = SCMI_PROTOCOL_PINCTRL,
+	.owner = THIS_MODULE,
+	.instance_init = &scmi_pinctrl_protocol_init,
+	.instance_deinit = &scmi_pinctrl_protocol_deinit,
+	.ops = &pinctrl_proto_ops,
+};
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index b3c6314bb4b8..674f949354f9 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -346,5 +346,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
 DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
 DECLARE_SCMI_REGISTER_UNREGISTER(system);
 DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
+DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
 
 #endif /* _SCMI_PROTOCOLS_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0ce5746a4470..a9f5398ea4cc 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -676,6 +676,52 @@ struct scmi_powercap_proto_ops {
 					  u32 *power_thresh_high);
 };
 
+enum scmi_pinctrl_selector_type {
+	PIN_TYPE = 0,
+	GROUP_TYPE,
+	FUNCTION_TYPE,
+};
+
+/**
+ * struct scmi_pinctrl_proto_ops - represents the various operations provided
+ * by SCMI Pinctrl Protocol
+ *
+ * @count_get: returns count of the registered elements in given type
+ * @name_get: returns name by index of given type
+ * @group_pins_get: returns the set of pins, assigned to the specified group
+ * @function_groups_get: returns the set of groups, assigned to the specified
+ *	function
+ * @mux_set: set muxing function for groups of pins
+ * @config_get: returns configuration parameter for pin or group
+ * @config_set: sets the configuration parameter for pin or group
+ * @pin_request: aquire pin before selecting mux setting
+ * @pin_free: frees pin, acquired by request_pin call
+ */
+struct scmi_pinctrl_proto_ops {
+	int (*count_get)(const struct scmi_protocol_handle *ph,
+			 enum scmi_pinctrl_selector_type type);
+	int (*name_get)(const struct scmi_protocol_handle *ph, u32 selector,
+			enum scmi_pinctrl_selector_type type, const char **name);
+	int (*group_pins_get)(const struct scmi_protocol_handle *ph, u32 selector,
+			      const unsigned int **pins, unsigned int *nr_pins);
+	int (*function_groups_get)(const struct scmi_protocol_handle *ph, u32 selector,
+				   unsigned int *nr_groups, const unsigned int **groups);
+	int (*mux_set)(const struct scmi_protocol_handle *ph, u32 selector, u32 group);
+	int (*config_get)(const struct scmi_protocol_handle *ph, u32 selector,
+			  enum scmi_pinctrl_selector_type type,
+			  u8 config_type, unsigned long *config_value);
+	int (*config_get_all)(const struct scmi_protocol_handle *ph,
+			      u32 selector,
+			      enum scmi_pinctrl_selector_type type, u16 size,
+			      u8 *config_types, unsigned long *config_values);
+	int (*config_set)(const struct scmi_protocol_handle *ph, u32 selector,
+			  enum scmi_pinctrl_selector_type type,
+			  unsigned int nr_configs, u8 *config_type,
+			  unsigned long *config_value);
+	int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
+	int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
+};
+
 /**
  * struct scmi_notify_ops  - represents notifications' operations provided by
  * SCMI core
@@ -783,6 +829,7 @@ enum scmi_std_protocol {
 	SCMI_PROTOCOL_RESET = 0x16,
 	SCMI_PROTOCOL_VOLTAGE = 0x17,
 	SCMI_PROTOCOL_POWERCAP = 0x18,
+	SCMI_PROTOCOL_PINCTRL = 0x19,
 };
 
 enum scmi_system_events {
-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 3/5] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Cristian Marussi]

On Fri, Oct 27, 2023 at 06:28:10AM +0000, Oleksii Moisieiev wrote:
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> ---

Hi Oleksii,

the new get/set v3.2 implementation seems finer to me at first sight.
I'll try to test this next days and give you more feedback.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 3/5] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[AKASHI Takahiro]

On Thu, Nov 02, 2023 at 08:06:41AM +0000, Cristian Marussi wrote:
> On Fri, Oct 27, 2023 at 06:28:10AM +0000, Oleksii Moisieiev wrote:
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> > 
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > ---
> 
> Hi Oleksii,
> 
> the new get/set v3.2 implementation seems finer to me at first sight.
> I'll try to test this next days and give you more feedback.

I don't think that this version addresses my comment yet:

https://lkml.iu.edu//hypermail/linux/kernel/2308.2/07483.html

I hope that it will be fixed in your *final* v5.

-Takahiro Akashi

> Thanks,
> Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 3/5] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[AKASHI Takahiro]

On Mon, Nov 06, 2023 at 11:26:11AM +0900, AKASHI Takahiro wrote:
> On Thu, Nov 02, 2023 at 08:06:41AM +0000, Cristian Marussi wrote:
> > On Fri, Oct 27, 2023 at 06:28:10AM +0000, Oleksii Moisieiev wrote:
> > > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> > > 
> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > > ---
> > 
> > Hi Oleksii,
> > 
> > the new get/set v3.2 implementation seems finer to me at first sight.
> > I'll try to test this next days and give you more feedback.
> 
> I don't think that this version addresses my comment yet:
> 
> https://lkml.iu.edu//hypermail/linux/kernel/2308.2/07483.html
> 
> I hope that it will be fixed in your *final* v5.

Oops, this comment should better go against patch#4/5.


> -Takahiro Akashi
> 
> > Thanks,
> > Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Oleksii Moisieiev]

Add new SCMI v3.2 pinctrl protocol bindings definitions and example.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>

---
Changes v3 -> v4
  - reworked protocol@19 format
---
 .../bindings/firmware/arm,scmi.yaml           | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..5318fe72354e 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -233,6 +233,39 @@ properties:
       reg:
         const: 0x18
 
+  protocol@19:
+    type: object
+    allOf:
+      - $ref: "#/$defs/protocol-node"
+      - $ref: "../pinctrl/pinctrl.yaml"
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        const: 0x19
+
+      '#pinctrl-cells':
+        const: 0
+
+    patternProperties:
+      '-pins$':
+        type: object
+        allOf:
+          - $ref: "../pinctrl/pincfg-node.yaml#"
+          - $ref: "../pinctrl/pinmux-node.yaml#"
+        unevaluatedProperties: false
+
+        description:
+          A pin multiplexing sub-node describe how to configure a
+          set of pins is some desired function.
+          A single sub-node may define several pin configurations.
+          This sub-node is using default pinctrl bindings to configure
+          pin multiplexing and using SCMI protocol to apply specified
+          configuration using SCMI protocol.
+
+    required:
+      - reg
+
 additionalProperties: false
 
 $defs:
@@ -384,6 +417,26 @@ examples:
             scmi_powercap: protocol@18 {
                 reg = <0x18>;
             };
+
+            scmi_pinctrl: protocol@19 {
+                reg = <0x19>;
+                #pinctrl-cells = <0>;
+
+                i2c2-pins {
+                    groups = "i2c2_a", "i2c2_b";
+                    function = "i2c2";
+                };
+
+                mdio-pins {
+                    groups = "avb_mdio";
+                    drive-strength = <24>;
+                };
+
+                keys_pins: keys-pins {
+                    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
+                    bias-pull-up;
+                };
+            };
         };
     };
 
-- 
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Krzysztof Kozlowski]

On 27/10/2023 08:28, Oleksii Moisieiev wrote:
> Add new SCMI v3.2 pinctrl protocol bindings definitions and example.
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> 
> ---
> Changes v3 -> v4
>   - reworked protocol@19 format
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..5318fe72354e 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -233,6 +233,39 @@ properties:
>        reg:
>          const: 0x18
>  
> +  protocol@19:
> +    type: object
> +    allOf:
> +      - $ref: "#/$defs/protocol-node"
> +      - $ref: "../pinctrl/pinctrl.yaml"

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.


It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Oleksii Moisieiev]

On 27.10.23 11:56, Krzysztof Kozlowski wrote:
> On 27/10/2023 08:28, Oleksii Moisieiev wrote:
>> Add new SCMI v3.2 pinctrl protocol bindings definitions and example.
>>
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>>
>> ---
>> Changes v3 -> v4
>>    - reworked protocol@19 format
>> ---
>>   .../bindings/firmware/arm,scmi.yaml           | 53 +++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..5318fe72354e 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -233,6 +233,39 @@ properties:
>>         reg:
>>           const: 0x18
>>   
>> +  protocol@19:
>> +    type: object
>> +    allOf:
>> +      - $ref: "#/$defs/protocol-node"
>> +      - $ref: "../pinctrl/pinctrl.yaml"
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.


Hi Krzysztof,

I'm sorry for the inconvenience.
The intention behind sharing this RFC was to initiate the review process 
for the changes I've been making to the pinctrl driver, aligning it with 
the updates in the DEN0056E beta2 document.

I am still actively working on these changes, and I fully intend to 
address and resolve the binding issues and all previously mentioned 
comments in the final v5 patch series.

Best regards,
Oleksii.

> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 
> 
> Best regards,
> Krzysztof
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Rob Herring]

On Fri, 27 Oct 2023 06:28:11 +0000, Oleksii Moisieiev wrote:
> Add new SCMI v3.2 pinctrl protocol bindings definitions and example.
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> 
> ---
> Changes v3 -> v4
>   - reworked protocol@19 format
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/firmware/arm,scmi.yaml:244:15: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/firmware/arm,scmi.yaml:258:19: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/firmware/arm,scmi.yaml:259:19: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/e9285b4377242e4d888391be987cbb99caf8c573.1698353854.git.oleksii_moisieiev@epam.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Linus Walleij]

On Fri, Oct 27, 2023 at 8:28 AM Oleksii Moisieiev
<Oleksii_Moisieiev@epam.com> wrote:

> +                keys_pins: keys-pins {
> +                    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
> +                    bias-pull-up;
> +                };

This is kind of interesting and relates to my question about naming groups and
functions of GPIO pins.

Here we see four pins suspiciously named "GP_*" which I read as
"generic purpose"
and they are not muxed to *any* function, yes pulled up.

I would have expected something like:

keys_pins: keys-pins {
  groups = "GP_5_17_grp", "GP_5_20_grp", "GP_5_22_grp", "GP_2_1_grp";
  function = "gpio";
  pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
  bias-pull-up;
};

I hope this illustrates what I see as a problem in not designing in
GPIO as an explicit
function, I get the impression that these pins are GPIO because it is hardware
default.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Takahiro Akashi]

Hi Arm folks,

Do you have any comment?
I expect that you have had some assumption when you defined
SCMI pinctrl protocol specification.

On Mon, Nov 06, 2023 at 02:12:36PM +0100, Linus Walleij wrote:
> On Fri, Oct 27, 2023 at 8:28???AM Oleksii Moisieiev
> <Oleksii_Moisieiev@epam.com> wrote:
> 
> > +                keys_pins: keys-pins {
> > +                    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
> > +                    bias-pull-up;
> > +                };
> 
> This is kind of interesting and relates to my question about naming groups and
> functions of GPIO pins.
> 
> Here we see four pins suspiciously named "GP_*" which I read as
> "generic purpose"
> and they are not muxed to *any* function, yes pulled up.
> 
> I would have expected something like:
> 
> keys_pins: keys-pins {
>   groups = "GP_5_17_grp", "GP_5_20_grp", "GP_5_22_grp", "GP_2_1_grp";
>   function = "gpio";
>   pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>   bias-pull-up;
> };
> 
> I hope this illustrates what I see as a problem in not designing in
> GPIO as an explicit
> function, I get the impression that these pins are GPIO because it is hardware
> default.

If you want to stick to "explicit", we may rather introduce a pre-defined
sub-node name, "gpio", in a device tree binding, i.e.

  protocol@19 { // pinctrl protocol
      ... // other pinmux nodes

      scmi_gpio: gpio { // "gpio" is a fixed name
          keys-pins {
              pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
              bias-pull-up;
              // possibly input or output
          };
          input-pins {
              groups = "some group"; // any name
              input-mode;
          }
          output-pins {
              pins = "foo1", "foo2"; // any name
              output-mode;
          }
      }
  }

It would indicate that all the succeeding nodes are for gpio definitions
and *virtual* gpio pin numbers will be assigned in the order of
appearances in "gpio" node. Then a client driver may refer to a gpio pin
(say, GP_2_1?) like in the current manner:

  foo_device {
       ...
       reset-gpios = <&scmi_gpio 3 ...>;
  }

-Takahiro Akashi

> 
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Cristian Marussi]

On Fri, Nov 10, 2023 at 09:58:39AM +0900, Takahiro Akashi wrote:
> Hi Arm folks,
> 

> Do you have any comment?
> I expect that you have had some assumption when you defined
> SCMI pinctrl protocol specification.
>

[CC Souvik]

@Souvik for context see:
https://lore.kernel.org/all/CACRpkdZ4GborirSpa3GK_PwMgCvY0ePEmZO+CwnLcP6nAdieow@mail.gmail.com/

Hi,

I am not sure what is the full story here, BUT the spec was mainly aimed
at supporting PINCTRL in SCMI with the idea to then, later on, base GPIO
on top of it, "easily" building on the PINCTRL spec features in the future
with a separate series from the one Oleksii is working on...but it like
seems the future is already here and maybe we have discovered something
to be clarified...

Souvik/Oleksii can tell you better what were (if any) further assumptions
related to GPIO on top on SCMI/PINCTRL, but the aim of this series was
always to be just the basic Generic Pinctrl support when dealing with an
SCMI server backend.

Regarding the current Pinctrl series by Oleksii, I would also notice that,
indeed, some "non-spec-dictated" naming assumptions are ALREADY present
somehow, because, currently, the spec and the pinctrl SCMI protocol layer
speak/refer about pins/groups/functions, as usual, only in terms of numeric
identifiers/IDs (with an associated name of course), while the pinctrl
driver (thanks to the Linux pictrl subsystem layer) describes and refers
anything in the DT in terms of names: so all of this really works only
because the names used in the DT happen to match the names reported by
the backend server.

My test DT uses just what Oleksii exemplified in the cover letter:

	pinctrl_i2c2: i2c2 {
		groups = "i2c2_a", "i2c2_b";
                function = "i2c2";
        };

	pinctrl_mdio: pins_mdio {
		groups = "avb_mdio";
                drive-strength = <24>;
        };

        keys_pins: keys {
		pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
                bias-pull-up;
        };


with a dummmy test driver referring to it, so as to trigger the drivers
core to initialize the pinctrl stuff.

But all of this works just because, in the example of my emulated setup,
my fake server exposes resources that are exactly named just as how the
above DT expects pins/functions/pins to be named, because this is how
the Generic Pinctrl subsystem in Linux is supposed to work, right ?

The difference is that the names, in the case of pinctrl-scmi, are not
hardcoded in the specific pin-controller driver BUT are provided dynamically
by the SCMI server at runtime.

And this is just a naming convention, between the Linux picntrl subsys AND
the SCMI server, that allows the Linux Pinctrl subsys to map, under-hood,
names to type/IDs as expected by the SCMI protocol layer (and by the spec):
so when you will define and describe a real platform with a DT, you will
will have to provide your name references, knowing that the shipped platform
SCMI fw will advertise exactly the same (or a superset of them)

As such, personally, I would find reasonable to use, equally, some
conventional function name like 'gpio' to advertise and configure groups
of pins as being used as GPIOs.

Maybe, though, both of these expected naming comventions should be
explicitly stated in the spec: indeed if you look at some Sensor protocol
extensions added in v3.0, in 4.7.2.5.1 "Sensor Axis Descriptors"
regarding naming we say:

"It is recommended that the name ends with ‘_’
followed by the axis of the sensor in uppercase. For
example, the name for the x-axis of a triaxial
accelerometer could be “acc_X” or “_X”."

...so maybe some similar remarks could be added here.

Souvik is really the one who can have a say about the opportunity (or
not) of these kind of explicit advised naming conventions on the spec,
so I have CCed him.
 
> On Mon, Nov 06, 2023 at 02:12:36PM +0100, Linus Walleij wrote:
> > On Fri, Oct 27, 2023 at 8:28???AM Oleksii Moisieiev
> > <Oleksii_Moisieiev@epam.com> wrote:
> > 
> > > +                keys_pins: keys-pins {
> > > +                    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
> > > +                    bias-pull-up;
> > > +                };
> > 
> > This is kind of interesting and relates to my question about naming groups and
> > functions of GPIO pins.
> > 
> > Here we see four pins suspiciously named "GP_*" which I read as
> > "generic purpose"
> > and they are not muxed to *any* function, yes pulled up.
> > 
> > I would have expected something like:
> > 
> > keys_pins: keys-pins {
> >   groups = "GP_5_17_grp", "GP_5_20_grp", "GP_5_22_grp", "GP_2_1_grp";
> >   function = "gpio";
> >   pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
> >   bias-pull-up;
> > };
> > 
> > I hope this illustrates what I see as a problem in not designing in
> > GPIO as an explicit
> > function, I get the impression that these pins are GPIO because it is hardware
> > default.
> 
> If you want to stick to "explicit", we may rather introduce a pre-defined
> sub-node name, "gpio", in a device tree binding, i.e.
> 
>   protocol@19 { // pinctrl protocol
>       ... // other pinmux nodes
> 
>       scmi_gpio: gpio { // "gpio" is a fixed name
>           keys-pins {
>               pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>               bias-pull-up;
>               // possibly input or output
>           };
>           input-pins {
>               groups = "some group"; // any name
>               input-mode;
>           }
>           output-pins {
>               pins = "foo1", "foo2"; // any name
>               output-mode;
>           }
>       }
>   }
>

I suppose your proposal of a specially named "gpio" node would be
another way, BUT it would also mean describing something in the DT that
could be discoverable dynamically querying the server (while making the
above assumptions about conventions).

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Souvik Chakravarty]

Hi,

On 10/11/2023 15:24, Cristian Marussi wrote:
> On Fri, Nov 10, 2023 at 09:58:39AM +0900, Takahiro Akashi wrote:
>> Hi Arm folks,
>>
> 
>> Do you have any comment?
>> I expect that you have had some assumption when you defined
>> SCMI pinctrl protocol specification.
>>
> 
> [CC Souvik]
> 
> @Souvik for context see:
> https://lore.kernel.org/all/CACRpkdZ4GborirSpa3GK_PwMgCvY0ePEmZO+CwnLcP6nAdieow@mail.gmail.com/
> 
> Hi,
> 
> I am not sure what is the full story here, BUT the spec was mainly aimed
> at supporting PINCTRL in SCMI with the idea to then, later on, base GPIO
> on top of it, "easily" building on the PINCTRL spec features in the future
> with a separate series from the one Oleksii is working on...but it like
> seems the future is already here and maybe we have discovered something
> to be clarified...
> 
> Souvik/Oleksii can tell you better what were (if any) further assumptions
> related to GPIO on top on SCMI/PINCTRL, but the aim of this series was
> always to be just the basic Generic Pinctrl support when dealing with an
> SCMI server backend.

The initial assumption always was that GPIOs can be considered as a 
specific function. Note that the spec does not define the types of 
function and leaves it to the DT binding (or driver) to figure out the 
function descriptions/names.

> 
> Regarding the current Pinctrl series by Oleksii, I would also notice that,
> indeed, some "non-spec-dictated" naming assumptions are ALREADY present
> somehow, because, currently, the spec and the pinctrl SCMI protocol layer
> speak/refer about pins/groups/functions, as usual, only in terms of numeric
> identifiers/IDs (with an associated name of course), while the pinctrl
> driver (thanks to the Linux pictrl subsystem layer) describes and refers
> anything in the DT in terms of names: so all of this really works only
> because the names used in the DT happen to match the names reported by
> the backend server.
> 
> My test DT uses just what Oleksii exemplified in the cover letter:
> 
> 	pinctrl_i2c2: i2c2 {
> 		groups = "i2c2_a", "i2c2_b";
>                  function = "i2c2";
>          };
> 
> 	pinctrl_mdio: pins_mdio {
> 		groups = "avb_mdio";
>                  drive-strength = <24>;
>          };
> 
>          keys_pins: keys {
> 		pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>                  bias-pull-up;
>          };
> 
> 
> with a dummmy test driver referring to it, so as to trigger the drivers
> core to initialize the pinctrl stuff.
> 
> But all of this works just because, in the example of my emulated setup,
> my fake server exposes resources that are exactly named just as how the
> above DT expects pins/functions/pins to be named, because this is how
> the Generic Pinctrl subsystem in Linux is supposed to work, right ?
> 
> The difference is that the names, in the case of pinctrl-scmi, are not
> hardcoded in the specific pin-controller driver BUT are provided dynamically
> by the SCMI server at runtime.
> 
> And this is just a naming convention, between the Linux picntrl subsys AND
> the SCMI server, that allows the Linux Pinctrl subsys to map, under-hood,
> names to type/IDs as expected by the SCMI protocol layer (and by the spec):
> so when you will define and describe a real platform with a DT, you will
> will have to provide your name references, knowing that the shipped platform
> SCMI fw will advertise exactly the same (or a superset of them)
> 
> As such, personally, I would find reasonable to use, equally, some
> conventional function name like 'gpio' to advertise and configure groups
> of pins as being used as GPIOs.

As a general principle, we dont try to put naming conventions in the 
spec if it can be easily resolved via DT. If this is proving to be a 
hassle then we can "recommend" in the spec that pins which can only be 
GPIOs are named starting "GPIO". Similar for functions.

However looking at Linus' comments below, I am not sure we are at that 
stage yet?

Regards,
Souvik

> 
> Maybe, though, both of these expected naming comventions should be
> explicitly stated in the spec: indeed if you look at some Sensor protocol
> extensions added in v3.0, in 4.7.2.5.1 "Sensor Axis Descriptors"
> regarding naming we say:
> 
> "It is recommended that the name ends with ‘_’
> followed by the axis of the sensor in uppercase. For
> example, the name for the x-axis of a triaxial
> accelerometer could be “acc_X” or “_X”."
> 
> ...so maybe some similar remarks could be added here.
> 
> Souvik is really the one who can have a say about the opportunity (or
> not) of these kind of explicit advised naming conventions on the spec,
> so I have CCed him.
>   
>> On Mon, Nov 06, 2023 at 02:12:36PM +0100, Linus Walleij wrote:
>>> On Fri, Oct 27, 2023 at 8:28???AM Oleksii Moisieiev
>>> <Oleksii_Moisieiev@epam.com> wrote:
>>>
>>>> +                keys_pins: keys-pins {
>>>> +                    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>>>> +                    bias-pull-up;
>>>> +                };
>>>
>>> This is kind of interesting and relates to my question about naming groups and
>>> functions of GPIO pins.
>>>
>>> Here we see four pins suspiciously named "GP_*" which I read as
>>> "generic purpose"
>>> and they are not muxed to *any* function, yes pulled up.
>>>
>>> I would have expected something like:
>>>
>>> keys_pins: keys-pins {
>>>    groups = "GP_5_17_grp", "GP_5_20_grp", "GP_5_22_grp", "GP_2_1_grp";
>>>    function = "gpio";
>>>    pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>>>    bias-pull-up;
>>> };
>>>
>>> I hope this illustrates what I see as a problem in not designing in
>>> GPIO as an explicit
>>> function, I get the impression that these pins are GPIO because it is hardware
>>> default.
>>
>> If you want to stick to "explicit", we may rather introduce a pre-defined
>> sub-node name, "gpio", in a device tree binding, i.e.
>>
>>    protocol@19 { // pinctrl protocol
>>        ... // other pinmux nodes
>>
>>        scmi_gpio: gpio { // "gpio" is a fixed name
>>            keys-pins {
>>                pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
>>                bias-pull-up;
>>                // possibly input or output
>>            };
>>            input-pins {
>>                groups = "some group"; // any name
>>                input-mode;
>>            }
>>            output-pins {
>>                pins = "foo1", "foo2"; // any name
>>                output-mode;
>>            }
>>        }
>>    }
>>
> 
> I suppose your proposal of a specially named "gpio" node would be
> another way, BUT it would also mean describing something in the DT that
> could be discoverable dynamically querying the server (while making the
> above assumptions about conventions).
> 
> Thanks,
> Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Linus Walleij]

Hi Souvik,

thanks for looking into this!

On Mon, Nov 13, 2023 at 1:56 PM Souvik Chakravarty
<souvik.chakravarty@arm.com> wrote:

> The initial assumption always was that GPIOs can be considered as a
> specific function. Note that the spec does not define the types of
> function and leaves it to the DT binding (or driver) to figure out the
> function descriptions/names.

Does this mean that each system using pinctrl-SCMI will need
to specify the available pins, groups and functions in a device tree
binding? For e.g. DT validation using schema?

This creates the problem of where to put it since
Documentation/devicetree/bindings/firmware/arm,scmi.yaml
is all we have, and for schemas to be applicable the implicit
assumption is that this is done per-compatible.

If we want to use device tree validation of the strings put into
the pinctrl node we need to allow for a per-soc compatible
under the pinctrl node like:

protocol@19 {
    compatible = "vendor,soc-scmi-pinctrl";
(...)

Then a DT schema can be made to match that and check it.

I'm uncertain about that because the SCMI binding has nothing
like this at the moment, all the protocol nodes are pretty
self-describing and don't seem to need any further configuration
to be used, but pin control may be the first instance where we
have to add some per-soc configuration into the protocol nodes :/

It's easy to do:

+  protocol@19:
+    type: object
+    allOf:
+      - $ref: "#/$defs/protocol-node"
+      - $ref: "../pinctrl/pinctrl.yaml"
+    unevaluatedProperties: false
+
+    properties:

        compatible:
            items:
              - enum:
                   - vendor1,soc1-scmi-pinctrl
                   - vendor2,soc2-scmi-pinctrl
                   - vendor3,soc3-scmi-pinctrl

This should be enough for just establishing the different
pin control configurations we can have in the device tree.

We are then able to put a more detailed schema for the
specific SoC pin control, such as a list of valid groups and
functions etc under the ordinary pinctrl bindings such as
Documentation/devicetree/bindings/pinctrl/vendor1,soc1-scmi-pinctrl.yaml
etc.

We should preferably put some pattern like this in place from
day 1 so developers know what is expected here. A mock
SoC is fine for the time being (we can delete it later when there
are some serious ones).

I'm uncertain because it feels like a first thing, but I can't really
think how it would work otherwise, part of me don't want to
pollute the SCMI binding with any per-soc compatibles, but
yet since these group and function strings will be per-soc I don't
see any other way, if they are supposed to be validated
with schema.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Souvik Chakravarty]

Hi Linus,

On 13/11/2023 13:32, Linus Walleij wrote:
> Hi Souvik,
> 
> thanks for looking into this!
> 
> On Mon, Nov 13, 2023 at 1:56 PM Souvik Chakravarty
> <souvik.chakravarty@arm.com> wrote:
> 
>> The initial assumption always was that GPIOs can be considered as a
>> specific function. Note that the spec does not define the types of
>> function and leaves it to the DT binding (or driver) to figure out the
>> function descriptions/names.
> 
> Does this mean that each system using pinctrl-SCMI will need
> to specify the available pins, groups and functions in a device tree
> binding? For e.g. DT validation using schema?

Sorry seems I made a typo above ("descriptions/names" should have been 
"description from names") which resulted in turning things on its head.

I really meant that the driver has to figure out the exact type or 
meaning of what the function does from its name. SCMI still continues to 
provide the list of pins/groups/functions and their names.

Regards,
Souvik

> 
> This creates the problem of where to put it since
> Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> is all we have, and for schemas to be applicable the implicit
> assumption is that this is done per-compatible.
> 
> If we want to use device tree validation of the strings put into
> the pinctrl node we need to allow for a per-soc compatible
> under the pinctrl node like:
> 
> protocol@19 {
>      compatible = "vendor,soc-scmi-pinctrl";
> (...)
> 
> Then a DT schema can be made to match that and check it.
> 
> I'm uncertain about that because the SCMI binding has nothing
> like this at the moment, all the protocol nodes are pretty
> self-describing and don't seem to need any further configuration
> to be used, but pin control may be the first instance where we
> have to add some per-soc configuration into the protocol nodes :/
> 
> It's easy to do:
> 
> +  protocol@19:
> +    type: object
> +    allOf:
> +      - $ref: "#/$defs/protocol-node"
> +      - $ref: "../pinctrl/pinctrl.yaml"
> +    unevaluatedProperties: false
> +
> +    properties:
> 
>          compatible:
>              items:
>                - enum:
>                     - vendor1,soc1-scmi-pinctrl
>                     - vendor2,soc2-scmi-pinctrl
>                     - vendor3,soc3-scmi-pinctrl
> 
> This should be enough for just establishing the different
> pin control configurations we can have in the device tree.
> 
> We are then able to put a more detailed schema for the
> specific SoC pin control, such as a list of valid groups and
> functions etc under the ordinary pinctrl bindings such as
> Documentation/devicetree/bindings/pinctrl/vendor1,soc1-scmi-pinctrl.yaml
> etc.
> 
> We should preferably put some pattern like this in place from
> day 1 so developers know what is expected here. A mock
> SoC is fine for the time being (we can delete it later when there
> are some serious ones).
> 
> I'm uncertain because it feels like a first thing, but I can't really
> think how it would work otherwise, part of me don't want to
> pollute the SCMI binding with any per-soc compatibles, but
> yet since these group and function strings will be per-soc I don't
> see any other way, if they are supposed to be validated
> with schema.
> 
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Linus Walleij]

On Mon, Nov 13, 2023 at 3:23 PM Souvik Chakravarty
<souvik.chakravarty@arm.com> wrote:
> On 13/11/2023 13:32, Linus Walleij wrote:
> > Hi Souvik,
> >
> > thanks for looking into this!
> >
> > On Mon, Nov 13, 2023 at 1:56 PM Souvik Chakravarty
> > <souvik.chakravarty@arm.com> wrote:
> >
> >> The initial assumption always was that GPIOs can be considered as a
> >> specific function. Note that the spec does not define the types of
> >> function and leaves it to the DT binding (or driver) to figure out the
> >> function descriptions/names.
> >
> > Does this mean that each system using pinctrl-SCMI will need
> > to specify the available pins, groups and functions in a device tree
> > binding? For e.g. DT validation using schema?
>
> Sorry seems I made a typo above ("descriptions/names" should have been
> "description from names") which resulted in turning things on its head.
>
> I really meant that the driver has to figure out the exact type or
> meaning of what the function does from its name. SCMI still continues to
> provide the list of pins/groups/functions and their names.

Indeed, that's what I imagined.

I think the rest of my question spurred by the phrase
"leaves it to the DT binding (or driver) to figure out" is actually
something Oleksii needs to look into more than a question to you.

It should probably come as a review comment to the patch 5/5 itself.

Oleksii, what is your take on my question about DT schema validation
for different SoCs?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Linus Walleij]

Hi Oleksii,

thanks for this patch, which still looks very good to me.

A question that was raised in discussion with Takahiro Akashi was how we
identify pins that can be used for GPIO and if the spec or implementation
has given any thought to that.

I can think of a few, such that:

1. Pins that can be used for GPIO all belong to some group - possibly even
  one group per pin such as "gpioA1", "gpioA2", "gpioA3" etc - that can be
  assigned a function named "gpio" or similar.

2. GPIO is seen as something external or "third usecase" that is handled
  by pin config, not by pin mux.

If it is 1 - which I find likely - it would be good to standardize the name of
the function to be "gpio" and somehow make it clear that all pins that are
desired to be used for GPIO need to have a (group, function) tuple pair
such as ("gpio001", "gpio") that will put the pin into GPIO mode.

If the assumption is anything goes, i.e. a vendor could say something
like ("io-group-99", "generic-io") to put a certain pin into GPIO mode,
that is maybe not so optimal, because it's nice for the GPIO driver
(which will come up) to be able to figure out by e.g. string name
conventions that a pin is in GPIO mode, and which group and function
that will put it into GPIO mode.

If this generality is not desired, having standard names for GPIO
functions and groups is still going to be an upside, if it can be achieved.
But maybe this isn't attainable at this point?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

Some recently added SCMI protocols needs an additional flags parameter to
be able to properly configure the command used to query the extended name
of a resource.

Modify extended_name_get helper accordingly.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     |  2 +-
 drivers/firmware/arm_scmi/driver.c    | 12 ++++++++----
 drivers/firmware/arm_scmi/perf.c      |  2 +-
 drivers/firmware/arm_scmi/power.c     |  2 +-
 drivers/firmware/arm_scmi/powercap.c  |  2 +-
 drivers/firmware/arm_scmi/protocols.h |  3 ++-
 drivers/firmware/arm_scmi/reset.c     |  3 ++-
 drivers/firmware/arm_scmi/sensors.c   |  2 +-
 drivers/firmware/arm_scmi/voltage.c   |  2 +-
 9 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 42b81c181d68..98511a3aa367 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -318,7 +318,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
 		if (SUPPORTS_EXTENDED_NAMES(attributes))
 			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
-						    clk->name,
+						    NULL, clk->name,
 						    SCMI_MAX_STR_SIZE);
 
 		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 09371f40d61f..3eb19ed6f148 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
  * @ph: A protocol handle reference.
  * @cmd_id: The specific command ID to use.
  * @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
  * @name: A pointer to the preallocated area where the retrieved name will be
  *	  stored as a NULL terminated string.
  * @len: The len in bytes of the @name char array.
@@ -1445,19 +1446,22 @@ struct scmi_msg_resp_domain_name_get {
  * Return: 0 on Succcess
  */
 static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
-					 u8 cmd_id, u32 res_id, char *name,
-					 size_t len)
+					 u8 cmd_id, u32 res_id, u32 *flags,
+					 char *name, size_t len)
 {
 	int ret;
+	size_t txlen;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_domain_name_get *resp;
 
-	ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id),
-				      sizeof(*resp), &t);
+	txlen = !flags ? sizeof(res_id) : sizeof(res_id) + sizeof(*flags);
+	ret = ph->xops->xfer_get_init(ph, cmd_id, txlen, sizeof(*resp), &t);
 	if (ret)
 		goto out;
 
 	put_unaligned_le32(res_id, t->tx.buf);
+	if (flags)
+		put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
 	resp = t->rx.buf;
 
 	ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index c2435be0ae1b..55db621a6fa0 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -288,7 +288,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET,
-					    dom_info->id, dom_info->info.name,
+					    dom_info->id, NULL, dom_info->info.name,
 					    SCMI_MAX_STR_SIZE);
 
 	if (dom_info->level_indexing_mode) {
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 356e83631664..077767d6e902 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags)) {
 		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 	}
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index cb5617443a14..62a7780fedbb 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -270,7 +270,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	 */
 	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 
 	return ret;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..b3c6314bb4b8 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -256,7 +256,8 @@ struct scmi_fc_info {
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
-				 u8 cmd_id, u32 res_id, char *name, size_t len);
+				 u8 cmd_id, u32 res_id, u32 *flags, char *name,
+				 size_t len);
 	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
 				    struct scmi_iterator_ops *ops,
 				    unsigned int max_resources, u8 msg_id,
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index e9afa8cab730..7217fd7c6afa 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attributes))
 		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0b5853fa9d87..9952a7bc6682 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
 	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attrl))
 		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
-					    s->name, SCMI_MAX_STR_SIZE);
+					    NULL, s->name, SCMI_MAX_STR_SIZE);
 
 	if (s->extended_scalar_attrs) {
 		s->sensor_power = le32_to_cpu(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index eaa8d944926a..36e2df77738c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 			if (SUPPORTS_EXTENDED_NAMES(attributes))
 				ph->hops->extended_name_get(ph,
 							VOLTAGE_DOMAIN_NAME_GET,
-							v->id, v->name,
+							v->id, NULL, v->name,
 							SCMI_MAX_STR_SIZE);
 			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
 				v->async_level_set = true;
-- 
2.42.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Add optional flags to extended names helper

[Sudeep Holla]

On Tue, 14 Nov 2023 14:54:49 +0000, Cristian Marussi wrote:
> Some recently added SCMI protocols needs an additional flags parameter to
> be able to properly configure the command used to query the extended name
> of a resource.
>
> Modify extended_name_get helper accordingly.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/1] firmware: arm_scmi: Add optional flags to extended names helper
      https://git.kernel.org/sudeep.holla/c/e4e6e8f1ad0f

--
Regards,
Sudeep


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/3] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Cristian Marussi]

On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> scmi: Introduce pinctrl SCMI protocol driver
> 
> Add basic implementation of the SCMI v3.2 pincontrol protocol
> excluding GPIO support. All pinctrl related callbacks and operations
> are exposed in the include/linux/scmi_protocol.h
> 

Hi Oleksii,

Thanks for this.

I tried out this in an emulated setup and found just a minor issue from
the spec/functional point of view...then I reworked the extended names
support using a modified hops->extended_name_get helper (as said the core
SCMI support needed a small modification to support PINCTRL): I'll reply
to this mail thread with such core SCMI modification patch, so you can
include this patch of mine in your next V3 and use it in your series.

Moreover, given that I wanted to test such rework of mine and a bunch
of other cleanups I did (as detailed down below), and it seemed silly
to throw all away just to then having to detail all to you, I'll also
include in another distinct reply the raw diff of what I changed in
your series to use the new extended_name support and a few other cleanups,
so that, if you want, you can just quickly merge that into your V3 patch
(of course if you like it and tests fine also for you...)...these are
small changes, if you take it, no need to bother with authorship and
attribution from my point of view.

> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> ---
>  MAINTAINERS                           |   6 +
>  drivers/firmware/arm_scmi/Makefile    |   3 +-
>  drivers/firmware/arm_scmi/driver.c    |   2 +
>  drivers/firmware/arm_scmi/pinctrl.c   | 932 ++++++++++++++++++++++++++
>  drivers/firmware/arm_scmi/protocols.h |   1 +
>  include/linux/scmi_protocol.h         |  47 ++
>  6 files changed, 990 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0e64787aace8..0d251ebac437 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20317,6 +20317,12 @@ F:	include/linux/sc[mp]i_protocol.h
>  F:	include/trace/events/scmi.h
>  F:	include/uapi/linux/virtio_scmi.h
>  
> +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
> +M:	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> +L:	linux-arm-kernel@lists.infradead.org
> +S:	Maintained
> +F:	drivers/firmware/arm_scmi/pinctrl.c
> +
>  SYSTEM RESET/SHUTDOWN DRIVERS
>  M:	Sebastian Reichel <sre@kernel.org>
>  L:	linux-pm@vger.kernel.org
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index b31d78fa66cc..071ac65f22b9 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
>  scmi-core-objs := $(scmi-bus-y)
>  
>  scmi-driver-y = driver.o notify.o
> +

Do not add spurios lines.

>  scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
>  scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
>  scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
>  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>  
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index dbc474ff62b7..fc7ba587ae6e 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3021,6 +3021,7 @@ static int __init scmi_driver_init(void)
>  	scmi_voltage_register();
>  	scmi_system_register();
>  	scmi_powercap_register();
> +	scmi_pinctrl_register();
>  
>  	return platform_driver_register(&scmi_driver);
>  }
> @@ -3038,6 +3039,7 @@ static void __exit scmi_driver_exit(void)
>  	scmi_voltage_unregister();
>  	scmi_system_unregister();
>  	scmi_powercap_unregister();
> +	scmi_pinctrl_unregister();
>  
>  	scmi_transports_exit();
>  
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> new file mode 100644
> index 000000000000..1c643d21390f
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,932 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "protocols.h"
> +
> +#define SET_TYPE(x) FIELD_PREP(GENMASK(1, 0), (x))
> +

This macro is really not needed given that you set just bit 0,1 on a
32bit flag field that is all 0-reserved otherwise...more on this down below.

> +#define REG_TYPE_BITS GENMASK(9, 8)
> +#define REG_CONFIG GENMASK(7, 0)
> +
> +#define GET_GROUPS_NR(x) FIELD_GET(GENMASK(31, 16), (x))
> +#define GET_PINS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
> +#define GET_FUNCTIONS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
> +
> +#define EXT_NAME_FLAG(x) FIELD_GET(BIT(31), (x))
> +#define NUM_ELEMS(x) FIELD_GET(GENMASK(15, 0), (x))
> +
> +#define REMAINING(x) FIELD_GET(GENMASK(31, 16), (x))
> +#define RETURNED(x) FIELD_GET(GENMASK(11, 0), (x))
> +
> +enum scmi_pinctrl_protocol_cmd {
> +	PINCTRL_ATTRIBUTES = 0x3,
> +	PINCTRL_LIST_ASSOCIATIONS = 0x4,
> +	PINCTRL_CONFIG_GET = 0x5,
> +	PINCTRL_CONFIG_SET = 0x6,
> +	PINCTRL_FUNCTION_SELECT = 0x7,
> +	PINCTRL_REQUEST = 0x8,
> +	PINCTRL_RELEASE = 0x9,
> +	PINCTRL_NAME_GET = 0xa,
> +	PINCTRL_SET_PERMISSIONS = 0xb
> +};
> +
> +struct scmi_msg_conf_set {
> +	__le32 identifier;
> +	__le32 attributes;
> +	__le32 config_value;
> +};
> +
> +struct scmi_msg_conf_get {
> +	__le32 identifier;
> +	__le32 attributes;
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> +	__le32 attributes_low;
> +	__le32 attributes_high;
> +};
> +
> +struct scmi_msg_ext_name {
> +	__le32 identifier;
> +	__le32 flags;
> +};
> +
> +struct scmi_resp_ext_name {
> +	__le32 flags;
> +	u8 name[SCMI_MAX_STR_SIZE];
> +};
> +

You could drop these structs once using the common extended_name_get
helper.

> +struct scmi_msg_pinctrl_attributes {
> +	__le32 identifier;
> +	__le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> +	__le32 attributes;
> +	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> +	__le32 identifier;
> +	__le32 flags;
> +	__le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> +	__le32 flags;
> +	__le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> +	__le32 identifier;
> +	__le32 function_id;
> +	__le32 flags;
> +};
> +
> +struct scmi_msg_request {
> +	__le32 identifier;
> +	__le32 flags;
> +};
> +
> +struct scmi_group_info {
> +	bool present;
> +	char *name;
> +	unsigned int *group_pins;
> +	unsigned int nr_pins;
> +};
> +
> +struct scmi_function_info {
> +	bool present;
> +	char *name;
> +	unsigned int *groups;
> +	unsigned int nr_groups;
> +};
> +
> +struct scmi_pin_info {
> +	bool present;
> +	char *name;
> +};
> +

I would re-define all of these 3 above *name pointers as
name[SCMI_MAX_STR_SIZE] instead and dont bother with dynalloc
and free...to avoid tricky resource tracking for a few bytes...and a
avoid a memleak that is now present :D ... more details later.

> +struct scmi_pinctrl_info {
> +	u32 version;
> +	int nr_groups;
> +	int nr_functions;
> +	int nr_pins;
> +	struct scmi_group_info *groups;
> +	struct scmi_function_info *functions;
> +	struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> +				       struct scmi_pinctrl_info *pi)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> +	if (!pi)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
> +				      0, sizeof(*attr), &t);
> +	if (ret)
> +		return ret;
> +
> +	attr = t->rx.buf;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		pi->nr_functions =
> +			le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
> +		pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
> +		pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));

I see a couple of issues here present in general all across this patch when
you use these macros;

You should take care of the endianity in the RX msg payload BEFORE and THEN
DISSECT the bitfields AND as a consequence use also an _le helper that fits
the size of the type that you are processing as in (being attributes 32 bit
little endian in the msg payload):

	pi->nr_pins = GET_PINS_NR(le32_to_cpu(attr->attributes_low));

Now all works just because everything is little endian really so nothing
is done by these macros....

> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph,
> +				  enum scmi_pinctrl_selector_type type)
> +{
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph)
> +		return -ENODEV;
> +

You can just drop this !ph check all-over this file...you just cannot be
here if ph was not initialized by the core...no need to check.

> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -ENODEV;
> +
> +	switch (type) {
> +	case PIN_TYPE:
> +		return pi->nr_pins;
> +	case GROUP_TYPE:
> +		return pi->nr_groups;
> +	case FUNCTION_TYPE:
> +		return pi->nr_functions;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> +				    u32 identifier,
> +				    enum scmi_pinctrl_selector_type type)
> +{
> +	int value;
> +
> +	if (!ph)
> +		return -ENODEV;

Ditto...I wont reapet all over.

> +
> +	value = scmi_pinctrl_get_count(ph, type);
> +	if (value < 0)
> +		return value;
> +
> +	if (identifier >= value)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_get_ext_name(const struct scmi_protocol_handle *ph,
> +				     u32 identifier,
> +				     enum scmi_pinctrl_selector_type type,
> +				     char **name)
> +{
> +	struct scmi_xfer *t;
> +	int ret = 0;
> +	struct scmi_msg_ext_name *tx;
> +	struct scmi_resp_ext_name *rx;
> +
> +	if (!ph || !name)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, identifier, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_NAME_GET, sizeof(*tx),
> +				      sizeof(*rx), &t);
> +
> +	tx = t->tx.buf;
> +	rx = t->rx.buf;
> +	tx->identifier = identifier;
> +	tx->flags = SET_TYPE(cpu_to_le32(type));
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (ret)
> +		goto out;
> +
> +	if (rx->flags) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	*name = kasprintf(GFP_KERNEL, "%s", rx->name);
> +	if (!*name)
> +		ret = -ENOMEM;
> + out:
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +

So I droppped completely the above function and used the helper with
some modifications as detailed below. (full details anyway in my
reference example patch as said above)

> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> +				   enum scmi_pinctrl_selector_type type,
> +				   u32 selector, char **name,
> +				   unsigned int *n_elems)
> +{
> +	int ret = 0;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_pinctrl_attributes *tx;
> +	struct scmi_resp_pinctrl_attributes *rx;
> +
> +	if (!ph || !name)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, selector, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> +				      sizeof(*rx), &t);
> +	if (ret)
> +		return ret;
> +
> +	tx = t->tx.buf;
> +	rx = t->rx.buf;
> +	tx->identifier = selector;

Missing cpu_to_le32

> +	tx->flags = SET_TYPE(cpu_to_le32(type));
> +

Here we are sending so we have to build an _le32 payload AT THE END
after having built our field like in:

	tx->flags = cpu_to_le32(SET_TYPE(type));

...BUT, as mentioned above, SET_TYPE really sets only bits 0,1 in a
32bit field so you can just: 

	tx->flags = cpu_to_le32(type);

> +	ret = ph->xops->do_xfer(ph, t);
> +	if (ret)
> +		goto out;

I would revert the logic here [if (!ret)] so as to avoid the goto
since, really, it is used only once in this function so we can easily
stick to non-goto flow.

> +
> +	*n_elems = NUM_ELEMS(rx->attributes);
> +

As explained below when this function is called for a PIN n_elems is
reported as zero and NOT needed really, so I made this param optional
adding here a:

	if (n_elems)
		*n_elems = ... 

> +	if (!EXT_NAME_FLAG(rx->attributes)) {

Missing le32_to_cpu on attributes.

> +		*name = kasprintf(GFP_KERNEL, "%s", rx->name);
> +		if (!*name)
> +			ret = -ENOMEM;
> +	} else {
> +		ret = scmi_pinctrl_get_ext_name(ph, selector, type, name);
> +	}

Ok, here there are a couple of things worth noting (beside the usage of
the common helper).

You dyn-allocate *name in both cases short ot extended name and, rightly
so, in the callers of this, you take care to free it on the error-path,
BUT you never take care to release it on Success and this is not even a
devres...so if protocol is unloaded you'll have a leak.

Moreover by the spec even if you have extended names supported you are
supposed to return anyway the first 16 bytes of the resource name in
rx->name anyway, and you have to consider that the following extended_name
GET (if supported) can fail for a number of reasons (including a faulty fw).

So usually, in the otehr proto we collect the short name anyway and then
overwrite it with the extended one if possible, issuing just a warning
on failure to get the extended one, since you have anyway the short.

But beside this I would not bother to track all this tiny dynalloc and
just declare a static name[SCMI_MAX_STR_SIZE] which can hold both.

IOW this could roughly be (with the helper):

	if (!ret) {
		attrs = le32_to_cpu(rx->attributes);
		if (n_elems)
			*n_elems = NUM_ELEMS(attrs);
		strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
 	}
 	ph->xops->xfer_put(ph, t);

	/*
	 * If supported overwrite short name with the extended one;
	 * on error just carry on and use already provided short name.
	 */
	if (!ret && EXT_NAME_FLAG(attrs))
		ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
					    (u32 *)&type, name,
					    SCMI_MAX_STR_SIZE);
 	return ret;

....probably more easy to grasp all of this looking at the proposed
fixes :D

> + out:
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +struct scmi_pinctrl_ipriv {
> +	u32 selector;
> +	enum scmi_pinctrl_selector_type type;
> +	unsigned int *array;
> +};
> +
> +static void iter_pinctrl_assoc_prepare_message(void *message,
> +					       unsigned int desc_index,
> +					       const void *priv)
> +{
> +	struct scmi_msg_pinctrl_list_assoc *msg = message;
> +	const struct scmi_pinctrl_ipriv *p = priv;
> +
> +	msg->identifier = cpu_to_le32(p->selector);
> +	msg->flags = SET_TYPE(cpu_to_le32(p->type));

Drop SET_TYPE as said above.

> +	/* Set the number of OPPs to be skipped/already read */
> +	msg->index = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> +					   const void *response, void *priv)
> +{
> +	const struct scmi_resp_pinctrl_list_assoc *r = response;
> +
> +	st->num_returned = le32_to_cpu(RETURNED(r->flags));
> +	st->num_remaining = le32_to_cpu(REMAINING(r->flags));
> +

Issue le32_to_cpu before applying the macro as said.

> +	return 0;
> +}
> +
> +static int
> +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
> +				    const void *response,
> +				    struct scmi_iterator_state *st, void *priv)
> +{
> +	const struct scmi_resp_pinctrl_list_assoc *r = response;
> +	struct scmi_pinctrl_ipriv *p = priv;
> +
> +	p->array[st->desc_index + st->loop_idx] =
> +		le16_to_cpu(r->array[st->loop_idx]);
> +
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
> +					  u32 selector,
> +					  enum scmi_pinctrl_selector_type type,
> +					  u16 size, unsigned int *array)
> +{
> +	int ret;
> +	void *iter;
> +	struct scmi_iterator_ops ops = {
> +		.prepare_message = iter_pinctrl_assoc_prepare_message,
> +		.update_state = iter_pinctrl_assoc_update_state,
> +		.process_response = iter_pinctrl_assoc_process_response,
> +	};
> +	struct scmi_pinctrl_ipriv ipriv = {
> +		.selector = selector,
> +		.type = type,
> +		.array = array,
> +	};
> +
> +	if (!ph || !array || !size)
> +		return -EINVAL;
> +
> +	if (type == PIN_TYPE)
> +		return -EINVAL;
> +

You can sqaush this 2 ifs into one and drop the !ph check.

> +	ret = scmi_pinctrl_validate_id(ph, selector, type);
> +	if (ret)
> +		return ret;
> +
> +	iter = ph->hops->iter_response_init(ph, &ops, size,
> +					    PINCTRL_LIST_ASSOCIATIONS,
> +					    sizeof(struct scmi_msg_pinctrl_list_assoc),
> +					    &ipriv);
> +
> +	if (IS_ERR(iter))
> +		return PTR_ERR(iter);
> +
> +	return ph->hops->iter_response_run(iter);
> +}
> +
> +static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
> +				   u32 selector,
> +				   enum scmi_pinctrl_selector_type type,
> +				   u8 config_type, u32 *config_value)
> +{
> +	struct scmi_xfer *t;
> +	struct scmi_msg_conf_get *tx;
> +	__le32 *le_config;

Safer if you use an existing unaligned helper for this...see below.

> +	u32 attributes = 0;
> +	int ret;
> +
> +	if (!ph || !config_value || type == FUNCTION_TYPE)
> +		return -EINVAL;
> +

Drop !ph

> +	ret = scmi_pinctrl_validate_id(ph, selector, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx),
> +				      sizeof(*le_config), &t);
				      sizeof(__le32), &t);
> +	if (ret)
> +		return ret;
> +
> +	tx = t->tx.buf;
> +	le_config = t->rx.buf;
> +	tx->identifier = cpu_to_le32(selector);
> +	attributes = FIELD_PREP(REG_TYPE_BITS, type) |
> +		FIELD_PREP(REG_CONFIG, config_type);
> +
> +	tx->attributes = cpu_to_le32(attributes);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	if (!ret)
> +		*config_value = le32_to_cpu(*le_config);

Use an helper:

		*config_value = get_unaligned_le32(t->rx.buf);

> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_set_config(const struct scmi_protocol_handle *ph,
> +				   u32 selector,
> +				   enum scmi_pinctrl_selector_type type,
> +				   u8 config_type, u32 config_value)
> +{
> +	struct scmi_xfer *t;
> +	struct scmi_msg_conf_set *tx;
> +	u32 attributes = 0;
> +	int ret;
> +
> +	if (!ph || type == FUNCTION_TYPE)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, selector, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_SET,
> +				      sizeof(*tx), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	tx = t->tx.buf;
> +	tx->identifier = cpu_to_le32(selector);
> +	attributes = FIELD_PREP(REG_TYPE_BITS, type) |
> +		FIELD_PREP(REG_CONFIG, config_type);
> +	tx->attributes = cpu_to_le32(attributes);
> +	tx->config_value = cpu_to_le32(config_value);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
> +					u32 identifier,
> +					enum scmi_pinctrl_selector_type type,
> +					u32 function_id)
> +{
> +	struct scmi_xfer *t;
> +	struct scmi_msg_func_set *tx;
> +	int ret;

Usually we place the shorter defs at start of the block...

> +
> +	if (!ph || type == FUNCTION_TYPE)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, identifier, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_FUNCTION_SELECT,
> +				      sizeof(*tx), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	tx = t->tx.buf;
> +	tx->identifier = cpu_to_le32(identifier);
> +	tx->function_id = cpu_to_le32(function_id);
> +	tx->flags = SET_TYPE(cpu_to_le32(type));
> +
Drop SET_TYPE

> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> +				u32 identifier,
> +				enum scmi_pinctrl_selector_type type)
> +{
> +	struct scmi_xfer *t;
> +	int ret;
This defs at start of the block..

> +	struct scmi_msg_request *tx;
> +
> +	if (!ph || type == FUNCTION_TYPE)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, identifier, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx),
> +				      0, &t);
> +
> +	tx = t->tx.buf;
> +	tx->identifier = identifier;
Missing cpu_to_le32

> +	tx->flags = SET_TYPE(cpu_to_le32(type));
Drop SET_TYPE

> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_request_pin(const struct scmi_protocol_handle *ph,
> +				    u32 pin)
> +{
> +	return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> +			     u32 identifier,
> +			     enum scmi_pinctrl_selector_type type)
> +{
> +	struct scmi_xfer *t;
> +	int ret;
> +	struct scmi_msg_request *tx;
> +
> +	if (!ph || type == FUNCTION_TYPE)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_validate_id(ph, identifier, type);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> +				      sizeof(*tx), 0, &t);
> +
> +	tx = t->tx.buf;
> +	tx->identifier = identifier;
Missing cpu_to_le32

> +	tx->flags = SET_TYPE(cpu_to_le32(type));
Drop SET_TYPE

> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_free_pin(const struct scmi_protocol_handle *ph, u32 pin)
> +{
> +	return scmi_pinctrl_free(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> +				       u32 selector,
> +				       struct scmi_group_info *group)
> +{
> +	int ret = 0;
> +
> +	if (!ph || !group)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> +				      &group->name,

Just a simple pointer in the above hyp of static allocation

				      group->name,

...same for all invoctions of course.

> +				      &group->nr_pins);
> +	if (ret)
> +		return ret;
> +
> +	if (!group->nr_pins) {
> +		dev_err(ph->dev, "Group %d has 0 elements", selector);
> +		return -ENODATA;
> +	}
> +
> +	group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins,
> +					       sizeof(*group->group_pins),
> +					       GFP_KERNEL);
> +	if (!group->group_pins) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> +					     group->nr_pins, group->group_pins);
> +	if (ret)
> +		goto err_groups;
> +
> +	group->present = true;
> +	return 0;
> +
> + err_groups:
> +	kfree(group->group_pins);
> + err:
> +	kfree(group->name);
> +	return ret;

You can rework all of the above to just drop both these goto exit
labels....take a look at my proposal fixes if you like it.

> +}
> +
> +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> +				       u32 selector, const char **name)
> +{
> +	int ret;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph || !name)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	if (selector > pi->nr_groups)
> +		return -EINVAL;
> +
> +	if (!pi->groups[selector].present) {
> +		ret = scmi_pinctrl_get_group_info(ph, selector,
> +						  &pi->groups[selector]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*name = pi->groups[selector].name;
> +
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
> +				       u32 selector, const unsigned int **pins,
> +				       unsigned int *nr_pins)
> +{
> +	int ret;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph || !pins || !nr_pins)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	if (selector > pi->nr_groups)
> +		return -EINVAL;
> +
> +	if (!pi->groups[selector].present) {
> +		ret = scmi_pinctrl_get_group_info(ph, selector,
> +						  &pi->groups[selector]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*pins = pi->groups[selector].group_pins;
> +	*nr_pins = pi->groups[selector].nr_pins;
> +
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> +					  u32 selector,
> +					  struct scmi_function_info *func)
> +{
> +	int ret = 0;
> +
> +	if (!ph || !func)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> +				      &func->name,
> +				      &func->nr_groups);
> +	if (ret)
> +		return ret;
> +
> +	if (!func->nr_groups) {
> +		dev_err(ph->dev, "Function %d has 0 elements", selector);
> +		return -ENODATA;
> +	}
> +
> +	func->groups = devm_kmalloc_array(ph->dev, func->nr_groups,
> +					  sizeof(*func->groups),
> +					  GFP_KERNEL);
> +	if (!func->groups) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> +					     func->nr_groups, func->groups);
> +	if (ret)
> +		goto err_funcs;
> +
> +	func->present = true;
> +	return 0;
> +
> + err_funcs:
> +	kfree(func->groups);
> + err:
> +	kfree(func->name);
> +	return ret;

Can be reworked and dropped as above.

> +}
> +
> +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> +					  u32 selector, const char **name)
> +{
> +	int ret;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph || !name)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	if (selector > pi->nr_functions)
> +		return -EINVAL;
> +
> +	if (!pi->functions[selector].present) {
> +		ret = scmi_pinctrl_get_function_info(ph, selector,
> +						     &pi->functions[selector]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*name = pi->functions[selector].name;
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *ph,
> +					    u32 selector,
> +					    unsigned int *nr_groups,
> +					    const unsigned int **groups)
> +{
> +	int ret;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph || !groups || !nr_groups)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	if (selector > pi->nr_functions)
> +		return -EINVAL;
> +
> +	if (!pi->functions[selector].present) {
> +		ret = scmi_pinctrl_get_function_info(ph, selector,
> +						     &pi->functions[selector]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*groups = pi->functions[selector].groups;
> +	*nr_groups = pi->functions[selector].nr_groups;
> +
> +	return ret;
> +}
> +
> +static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
> +				u32 selector, u32 group)
> +{
> +	return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> +					    selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> +				     u32 selector, struct scmi_pin_info *pin)
> +{
> +	int ret = 0;
> +	struct scmi_pinctrl_info *pi;
> +	unsigned int n_elems;
> +
> +	if (!ph || !pin)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> +				      &pin->name,
> +				      &n_elems);
> +	if (ret)
> +		return ret;
> +
> +	if (n_elems != pi->nr_pins) {
> +		dev_err(ph->dev, "Wrong pin count expected %d has %d",
> +			pi->nr_pins, n_elems);
> +		return -ENODATA;
> +	}

This is wrong because PINCTRL_ATTRIBUTES by the spec reports this
field of attributes as ZERO when the type queried is PIN_TYPE...so
you'll fail all the time here. (or you have to fix your server too :D)

So I would just drop this and this is the reason above I made n_elems
param to scmi_pinctrl_attributes() optional: here you dont need it.
[or you could ignore it...)

> +
> +	if (*pin->name == 0) {
> +		dev_err(ph->dev, "Pin name is empty");
> +		goto err;
> +	}

And I would just drop this check too...you'll have an empty string in
case you define name as name[SCMI_MAX_STR_SIZE]...maybe a warning
not an error...picntrl seems to survive with an empty string
> +
> +	pin->present = true;
> +	return 0;
> +
> + err:
> +	kfree(pin->name);
> +	return ret;
Can be dropped if static.

> +}
> +
> +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> +				     u32 selector, const char **name)
> +{
> +	int ret;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph || !name)
> +		return -EINVAL;
> +
> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	if (selector > pi->nr_pins)
> +		return -EINVAL;
> +
> +	if (!pi->pins[selector].present) {
> +		ret = scmi_pinctrl_get_pin_info(ph, selector,
> +						&pi->pins[selector]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*name = pi->pins[selector].name;
> +
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_get_name(const struct scmi_protocol_handle *ph,
> +				 u32 selector,
> +				 enum scmi_pinctrl_selector_type type,
> +				 const char **name)
> +{
> +	switch (type) {
> +	case PIN_TYPE:
> +		return scmi_pinctrl_get_pin_name(ph, selector, name);
> +	case GROUP_TYPE:
> +		return scmi_pinctrl_get_group_name(ph, selector, name);
> +	case FUNCTION_TYPE:
> +		return scmi_pinctrl_get_function_name(ph, selector, name);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> +	.get_count = scmi_pinctrl_get_count,
> +	.get_name = scmi_pinctrl_get_name,
> +	.get_group_pins = scmi_pinctrl_get_group_pins,
> +	.get_function_groups = scmi_pinctrl_get_function_groups,
> +	.set_mux = scmi_pinctrl_set_mux,
> +	.get_config = scmi_pinctrl_get_config,
> +	.set_config = scmi_pinctrl_set_config,
> +	.request_pin = scmi_pinctrl_request_pin,
> +	.free_pin = scmi_pinctrl_free_pin
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> +	u32 version;
> +	struct scmi_pinctrl_info *pinfo;
> +	int ret;
> +
> +	if (!ph)
> +		return -EINVAL;
Drop

> +
> +	ret = ph->xops->version_get(ph, &version);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> +		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> +	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> +	if (!pinfo)
> +		return -ENOMEM;
> +
> +	ret = scmi_pinctrl_attributes_get(ph, pinfo);
> +	if (ret)
> +		return ret;
> +
> +	pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> +				   sizeof(*pinfo->pins),
> +				   GFP_KERNEL);
> +	if (!pinfo->pins)
> +		return -ENOMEM;
> +
> +	pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> +				     sizeof(*pinfo->groups),
> +				     GFP_KERNEL);
> +	if (!pinfo->groups)
> +		return -ENOMEM;
> +
> +	pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> +					sizeof(*pinfo->functions),
> +					GFP_KERNEL);
> +	if (!pinfo->functions)
> +		return -ENOMEM;
> +
> +	pinfo->version = version;
> +
> +	return ph->set_priv(ph, pinfo);
> +}
> +
> +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> +{
> +	int i;
> +	struct scmi_pinctrl_info *pi;
> +
> +	if (!ph)
> +		return -EINVAL;
> +
Drop

> +	pi = ph->get_priv(ph);
> +	if (!pi)
> +		return -EINVAL;
> +
> +	for (i = 0; i < pi->nr_groups; i++)
> +		if (pi->groups[i].present) {
> +			devm_kfree(ph->dev, pi->groups[i].group_pins);
> +			pi->groups[i].present = false;
> +		}
> +
> +	for (i = 0; i < pi->nr_functions; i++)
> +		if (pi->functions[i].present) {
> +			devm_kfree(ph->dev, pi->functions[i].groups);
> +			pi->functions[i].present = false;
> +		}
> +
> +	return 0;
> +}
> +
> +static const struct scmi_protocol scmi_pinctrl = {
> +	.id = SCMI_PROTOCOL_PINCTRL,
> +	.owner = THIS_MODULE,
> +	.instance_init = &scmi_pinctrl_protocol_init,
> +	.instance_deinit = &scmi_pinctrl_protocol_deinit,
> +	.ops = &pinctrl_proto_ops,
> +};
> +
> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> index 78e1a01eb656..533b94c9a9a7 100644
> --- a/drivers/firmware/arm_scmi/protocols.h
> +++ b/drivers/firmware/arm_scmi/protocols.h
> @@ -345,5 +345,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
>  DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
>  DECLARE_SCMI_REGISTER_UNREGISTER(system);
>  DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
> +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
>  
>  #endif /* _SCMI_PROTOCOLS_H */
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 0ce5746a4470..8e15d2912045 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -735,6 +735,52 @@ struct scmi_notify_ops {
>  					 struct notifier_block *nb);
>  };
>  
> +enum scmi_pinctrl_selector_type {
> +	PIN_TYPE = 0,
> +	GROUP_TYPE,
> +	FUNCTION_TYPE
> +};
> +
> +/**
> + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
> + * by SCMI Pinctrl Protocol
> + *
> + * @get_count: returns count of the registered elements in given type
> + * @get_name: returns name by index of given type
> + * @get_group_pins: returns the set of pins, assigned to the specified group
> + * @get_function_groups: returns the set of groups, assigned to the specified
> + *	function
> + * @set_mux: set muxing function for groups of pins
> + * @get_config: returns configuration parameter for pin or group
> + * @set_config: sets the configuration parameter for pin or group
> + * @request_pin: aquire pin before selecting mux setting
> + * @free_pin: frees pin, acquired by request_pin call
> + */
> +struct scmi_pinctrl_proto_ops {
> +	int (*get_count)(const struct scmi_protocol_handle *ph,
> +			 enum scmi_pinctrl_selector_type type);
> +	int (*get_name)(const struct scmi_protocol_handle *ph,
> +			u32 selector,
> +			enum scmi_pinctrl_selector_type type,
> +			const char **name);
> +	int (*get_group_pins)(const struct scmi_protocol_handle *ph,
> +			      u32 selector,
> +			      const unsigned int **pins, unsigned int *nr_pins);
> +	int (*get_function_groups)(const struct scmi_protocol_handle *ph,
> +				   u32 selector, unsigned int *nr_groups,
> +				   const unsigned int **groups);
> +	int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
> +		       u32 group);
> +	int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
> +			  enum scmi_pinctrl_selector_type type,
> +			  u8 config_type, u32 *config_value);
> +	int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
> +			  enum scmi_pinctrl_selector_type type,
> +			  u8 config_type, u32 config_value);
> +	int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> +	int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> +};
> +

I know this could seem pedantic but all over the SCMI stack the protocol
operations, being external facing methods, are named as <something_ACTION>
as in:

	count_get
	name_get
 	config_get
	config_set

etc etc...please rename accordingly the pinctrl_ops.

>  /**
>   * struct scmi_handle - Handle returned to ARM SCMI clients for usage.
>   *
> @@ -783,6 +829,7 @@ enum scmi_std_protocol {
>  	SCMI_PROTOCOL_RESET = 0x16,
>  	SCMI_PROTOCOL_VOLTAGE = 0x17,
>  	SCMI_PROTOCOL_POWERCAP = 0x18,
> +	SCMI_PROTOCOL_PINCTRL = 0x19,
>  };
>  
>  enum scmi_system_events {
> -- 
> 2.25.1

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

Some recently added SCMI protocols needs an additional flags parameter to
be able to properly configure the command used to query the extended name
of a resource.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     | 2 +-
 drivers/firmware/arm_scmi/driver.c    | 8 ++++++--
 drivers/firmware/arm_scmi/perf.c      | 3 ++-
 drivers/firmware/arm_scmi/power.c     | 2 +-
 drivers/firmware/arm_scmi/powercap.c  | 2 +-
 drivers/firmware/arm_scmi/protocols.h | 3 ++-
 drivers/firmware/arm_scmi/reset.c     | 3 ++-
 drivers/firmware/arm_scmi/sensors.c   | 2 +-
 drivers/firmware/arm_scmi/voltage.c   | 2 +-
 9 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 96060bf90a24..e6e087686e8c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
 		if (SUPPORTS_EXTENDED_NAMES(attributes))
 			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
-						    clk->name,
+						    NULL, clk->name,
 						    SCMI_MAX_STR_SIZE);
 
 		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..5be931a07c84 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
  * @ph: A protocol handle reference.
  * @cmd_id: The specific command ID to use.
  * @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
  * @name: A pointer to the preallocated area where the retrieved name will be
  *	  stored as a NULL terminated string.
  * @len: The len in bytes of the @name char array.
@@ -1445,8 +1446,8 @@ struct scmi_msg_resp_domain_name_get {
  * Return: 0 on Succcess
  */
 static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
-					 u8 cmd_id, u32 res_id, char *name,
-					 size_t len)
+					 u8 cmd_id, u32 res_id, u32 *flags,
+					 char *name, size_t len)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -1458,6 +1459,9 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
 		goto out;
 
 	put_unaligned_le32(res_id, t->tx.buf);
+	if (flags)
+		put_unaligned_le32(*flags,
+				   (u8 *)t->tx.buf + sizeof(res_id));
 	resp = t->rx.buf;
 
 	ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ecf5c4de851b..d85d4a0e3605 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -237,7 +237,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 356e83631664..077767d6e902 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(flags)) {
 		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 	}
 
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 244929cb4f3e..86c9a0b5a765 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -270,7 +270,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	 */
 	if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
 		ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
-					    domain, dom_info->name,
+					    domain, NULL, dom_info->name,
 					    SCMI_MAX_STR_SIZE);
 
 	return ret;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..b3c6314bb4b8 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -256,7 +256,8 @@ struct scmi_fc_info {
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
-				 u8 cmd_id, u32 res_id, char *name, size_t len);
+				 u8 cmd_id, u32 res_id, u32 *flags, char *name,
+				 size_t len);
 	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
 				    struct scmi_iterator_ops *ops,
 				    unsigned int max_resources, u8 msg_id,
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index e9afa8cab730..7217fd7c6afa 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attributes))
 		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
-					    dom_info->name, SCMI_MAX_STR_SIZE);
+					    NULL, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
 
 	return ret;
 }
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0b5853fa9d87..9952a7bc6682 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
 	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
 	    SUPPORTS_EXTENDED_NAMES(attrl))
 		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
-					    s->name, SCMI_MAX_STR_SIZE);
+					    NULL, s->name, SCMI_MAX_STR_SIZE);
 
 	if (s->extended_scalar_attrs) {
 		s->sensor_power = le32_to_cpu(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index eaa8d944926a..36e2df77738c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 			if (SUPPORTS_EXTENDED_NAMES(attributes))
 				ph->hops->extended_name_get(ph,
 							VOLTAGE_DOMAIN_NAME_GET,
-							v->id, v->name,
+							v->id, NULL, v->name,
 							SCMI_MAX_STR_SIZE);
 			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
 				v->async_level_set = true;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel