[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

[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

[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

[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

[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

[RFC v5 4/5] pinctrl: Implementation of the generic scmi-pinctrl driver

[Oleksii Moisieiev]

scmi-pinctrl driver implements pinctrl driver interface and using
SCMI protocol to redirect messages from pinctrl subsystem SDK to
SCMI platform firmware, which does the changes in HW.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---
Changes v3 -> v4
  - ordered config option alphabetically
  - ordered 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
---
 MAINTAINERS                    |   1 +
 drivers/pinctrl/Kconfig        |  11 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-scmi.c | 445 +++++++++++++++++++++++++++++++++
 4 files changed, 458 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-scmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d81d00e5f4f..c4e36f955e53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20527,6 +20527,7 @@ M:	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
 L:	linux-arm-kernel@lists.infradead.org
 S:	Maintained
 F:	drivers/firmware/arm_scmi/pinctrl.c
+F:	drivers/pinctrl/pinctrl-scmi.c
 
 SYSTEM RESET/SHUTDOWN DRIVERS
 M:	Sebastian Reichel <sre@kernel.org>
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 5787c579dcf6..956cfe76fbc6 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -428,6 +428,17 @@ config PINCTRL_ROCKCHIP
 	help
           This support pinctrl and GPIO driver for Rockchip SoCs.
 
+config PINCTRL_SCMI
+	tristate "Pinctrl driver using SCMI protocol interface"
+	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
+	select PINMUX
+	select GENERIC_PINCONF
+	help
+	  This driver provides support for pinctrl which is controlled
+	  by firmware that implements the SCMI interface.
+	  It uses SCMI Message Protocol to interact with the
+	  firmware providing all the pinctrl controls.
+
 config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e196c6e324ad..25d67bac9ee0 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
+obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
 obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
new file mode 100644
index 000000000000..4d16f3c10eef
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,445 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/seq_file.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinconf.h"
+
+#define DRV_NAME "scmi-pinctrl"
+
+static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
+
+struct scmi_pinctrl_funcs {
+	unsigned int num_groups;
+	const char **groups;
+};
+
+struct scmi_pinctrl {
+	struct device *dev;
+	struct scmi_protocol_handle *ph;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_desc pctl_desc;
+	struct scmi_pinctrl_funcs *functions;
+	unsigned int nr_functions;
+	char **groups;
+	unsigned int nr_groups;
+	struct pinctrl_pin_desc *pins;
+	unsigned int nr_pins;
+};
+
+static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE);
+}
+
+static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	int ret;
+	const char *name;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name);
+	if (ret) {
+		dev_err(pmx->dev, "get name failed with err %d", ret);
+		return NULL;
+	}
+
+	return name;
+}
+
+static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const unsigned int **pins,
+				       unsigned int *num_pins)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins);
+}
+
+#ifdef CONFIG_OF
+static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
+				       struct device_node *np_config,
+				       struct pinctrl_map **map,
+				       u32 *num_maps)
+{
+	return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
+					      PIN_MAP_TYPE_INVALID);
+}
+
+static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
+				     u32 num_maps)
+{
+	kfree(map);
+}
+
+#endif /* CONFIG_OF */
+
+static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
+	.get_groups_count = pinctrl_scmi_get_groups_count,
+	.get_group_name = pinctrl_scmi_get_group_name,
+	.get_group_pins = pinctrl_scmi_get_group_pins,
+#ifdef CONFIG_OF
+	.dt_node_to_map = pinctrl_scmi_dt_node_to_map,
+	.dt_free_map = pinctrl_scmi_dt_free_map,
+#endif
+};
+
+static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE);
+}
+
+static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
+						  unsigned int selector)
+{
+	int ret;
+	const char *name;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE, &name);
+	if (ret) {
+		dev_err(pmx->dev, "get name failed with err %d", ret);
+		return NULL;
+	}
+
+	return name;
+}
+
+static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
+					    unsigned int selector,
+					    const char * const **groups,
+					    unsigned int * const num_groups)
+{
+	const unsigned int *group_ids;
+	int ret, i;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!groups || !num_groups)
+		return -EINVAL;
+
+	if (selector < pmx->nr_functions &&
+	    pmx->functions[selector].num_groups) {
+		*groups = (const char * const *)pmx->functions[selector].groups;
+		*num_groups = pmx->functions[selector].num_groups;
+		return 0;
+	}
+
+	ret = pinctrl_ops->function_groups_get(pmx->ph, selector,
+					       &pmx->functions[selector].num_groups,
+					       &group_ids);
+	if (ret) {
+		dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
+		return ret;
+	}
+
+	*num_groups = pmx->functions[selector].num_groups;
+	if (!*num_groups)
+		return -EINVAL;
+
+	pmx->functions[selector].groups =
+		devm_kcalloc(pmx->dev, *num_groups, sizeof(*pmx->functions[selector].groups),
+			     GFP_KERNEL);
+	if (!pmx->functions[selector].groups)
+		return -ENOMEM;
+
+	for (i = 0; i < *num_groups; i++) {
+		pmx->functions[selector].groups[i] =
+			pinctrl_scmi_get_group_name(pmx->pctldev,
+						    group_ids[i]);
+		if (!pmx->functions[selector].groups[i]) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	*groups = (const char * const *)pmx->functions[selector].groups;
+
+	return 0;
+
+err_free:
+	devm_kfree(pmx->dev, pmx->functions[selector].groups);
+
+	return ret;
+}
+
+static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+				     unsigned int group)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->mux_set(pmx->ph, selector, group);
+}
+
+static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->pin_request(pmx->ph, offset);
+}
+
+static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl_ops->pin_free(pmx->ph, offset);
+}
+
+static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
+	.request = pinctrl_scmi_request,
+	.free = pinctrl_scmi_free,
+	.get_functions_count = pinctrl_scmi_get_functions_count,
+	.get_function_name = pinctrl_scmi_get_function_name,
+	.get_function_groups = pinctrl_scmi_get_function_groups,
+	.set_mux = pinctrl_scmi_func_set_mux,
+};
+
+static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin,
+				    unsigned long *config)
+{
+	int ret;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_type;
+	unsigned long config_value;
+
+	if (!config)
+		return -EINVAL;
+
+	config_type = pinconf_to_config_param(*config);
+
+	ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, config_type, &config_value);
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_packed(config_type, config_value);
+
+	return 0;
+}
+
+static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
+				    unsigned int _pin,
+				    unsigned long *configs,
+				    unsigned int num_configs)
+{
+	int i, ret;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_type;
+	unsigned long config_value;
+
+	if (!configs || num_configs == 0)
+		return -EINVAL;
+
+	for (i = 0; i < num_configs; i++) {
+		config_type = pinconf_to_config_param(configs[i]);
+		config_value = pinconf_to_config_argument(configs[i]);
+
+		ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE,
+					      num_configs, (u8 *)&config_type,
+					      &config_value);
+		if (ret) {
+			dev_err(pmx->dev, "Error parsing config %ld\n",
+				configs[i]);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
+					  unsigned int group,
+					  unsigned long *configs,
+					  unsigned int num_configs)
+{
+	int i, ret;
+	struct scmi_pinctrl *pmx =  pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_type;
+	unsigned long config_value;
+
+	if (!configs || num_configs == 0)
+		return -EINVAL;
+
+	for (i = 0; i < num_configs; i++) {
+		config_type = pinconf_to_config_param(configs[i]);
+		config_value = pinconf_to_config_argument(configs[i]);
+
+		ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE,
+					      num_configs, (u8 *)&config_type,
+					      &config_value);
+		if (ret) {
+			dev_err(pmx->dev, "Error parsing config = %ld",
+				configs[i]);
+			break;
+		}
+	}
+
+	return ret;
+};
+
+static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
+					  unsigned int group,
+					  unsigned long *config)
+{
+	int ret;
+	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_type;
+	unsigned long config_value;
+
+	if (!config)
+		return -EINVAL;
+
+	config_type = pinconf_to_config_param(*config);
+
+	ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, config_type, &config_value);
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_packed(config_type, config_value);
+
+	return 0;
+}
+
+static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = pinctrl_scmi_pinconf_get,
+	.pin_config_set = pinctrl_scmi_pinconf_set,
+	.pin_config_group_set = pinctrl_scmi_pinconf_group_set,
+	.pin_config_group_get = pinctrl_scmi_pinconf_group_get,
+	.pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
+				 unsigned int *nr_pins,
+				 const struct pinctrl_pin_desc **pins)
+{
+	int ret, i;
+
+	if (!pins || !nr_pins)
+		return -EINVAL;
+
+	if (pmx->nr_pins) {
+		*pins = pmx->pins;
+		*nr_pins = pmx->nr_pins;
+		return 0;
+	}
+
+	*nr_pins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
+
+	pmx->nr_pins = *nr_pins;
+	pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins), GFP_KERNEL);
+	if (!pmx->pins)
+		return -ENOMEM;
+
+	for (i = 0; i < *nr_pins; i++) {
+		pmx->pins[i].number = i;
+		ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pmx->pins[i].name);
+		if (ret) {
+			dev_err(pmx->dev, "Can't get name for pin %d: rc %d", i, ret);
+			pmx->nr_pins = 0;
+			return ret;
+		}
+	}
+
+	*pins = pmx->pins;
+	dev_dbg(pmx->dev, "got pins %d", *nr_pins);
+
+	return 0;
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+	{ }
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static int scmi_pinctrl_probe(struct scmi_device *sdev)
+{
+	int ret;
+	struct device *dev = &sdev->dev;
+	struct scmi_pinctrl *pmx;
+	const struct scmi_handle *handle;
+	struct scmi_protocol_handle *ph;
+
+	if (!sdev || !sdev->handle)
+		return -EINVAL;
+
+	handle = sdev->handle;
+
+	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
+	if (IS_ERR(pinctrl_ops))
+		return PTR_ERR(pinctrl_ops);
+
+	pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
+	if (!pmx)
+		return -ENOMEM;
+
+	pmx->ph = ph;
+
+	pmx->dev = dev;
+	pmx->pctl_desc.name = DRV_NAME;
+	pmx->pctl_desc.owner = THIS_MODULE;
+	pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
+	pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
+	pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
+
+	ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
+				    &pmx->pctl_desc.pins);
+	if (ret)
+		return ret;
+
+	ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx, &pmx->pctldev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
+
+	pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
+	pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
+
+	if (pmx->nr_functions) {
+		pmx->functions =
+			devm_kcalloc(dev, pmx->nr_functions, sizeof(*pmx->functions),
+				     GFP_KERNEL);
+		if (!pmx->functions)
+			return -ENOMEM;
+	}
+
+	if (pmx->nr_groups) {
+		pmx->groups =
+			devm_kcalloc(dev, pmx->nr_groups, sizeof(*pmx->groups), GFP_KERNEL);
+		if (!pmx->groups)
+			return -ENOMEM;
+	}
+
+	return pinctrl_enable(pmx->pctldev);
+}
+
+static struct scmi_driver scmi_pinctrl_driver = {
+	.name = DRV_NAME,
+	.probe = scmi_pinctrl_probe,
+	.id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_pinctrl_driver);
+
+MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>");
+MODULE_DESCRIPTION("ARM SCMI pin controller driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

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

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.

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
> 

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

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

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

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
> 

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

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

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

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

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

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

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

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

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

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

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