2024-01-04 10:45:16

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH v2 0/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

This patchset is a rework from Oleksii's RFC v5 patchset
https://lore.kernel.org/all/[email protected]/

This patchset introduces some changes based on RFC v5:
- introduce helper get_max_msg_size
- support compatible string
- iterate the id_table
- Support multiple configs in one command
- Added i.MX support
- Patch 5 firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
is almost same as RFCv5 expect multiple configs support.
- Patch 4 the dt-bindings includes compatible string to support i.MX
- Rebased on 2023-12-15 linux-next/master

If any comments from RFC v5 are missed, I am sorry in advance.

This PINCTRL Protocol is following Version 3.2 SCMI Spec Beta release.

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. So implement the driver to support such cases.

The i.MX95 Example as below:

Configuration:
The scmi-pinctrl driver can be configured using DT bindings.
For example:
/ {
sram0: sram@445b1000 {
compatible = "mmio-sram";
reg = <0x0 0x445b1000 0x0 0x400>;

#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x0 0x445b1000 0x400>;

scmi_buf0: scmi-sram-section@0 {
compatible = "arm,scmi-shmem";
reg = <0x0 0x80>;
};

scmi_buf1: scmi-sram-section@80 {
compatible = "arm,scmi-shmem";
reg = <0x80 0x80>;
};
};

firmware {
scmi {
compatible = "arm,scmi";
mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
shmem = <&scmi_buf0>, <&scmi_buf1>;
#address-cells = <1>;
#size-cells = <0>;

scmi_iomuxc: protocol@19 {
compatible = "fsl,imx95-scmi-pinctrl";
reg = <0x19>;
};
};
};
};

&scmi_iomuxc {
pinctrl_tpm3: tpm3grp {
fsl,pins = <
IMX95_PAD_GPIO_IO12__TPM3_CH2 0x51e
>;
};
};

This patchset has been tested on i.MX95-19x19-EVK board.

Signed-off-by: Peng Fan <[email protected]>
---
Changes in v2:
Added comments, and added R-b for Patch 1
Moved the compatile string and i.MX patch to the end, marked NOT APPLY
Patchset based on lore.kernel.org/all/[email protected]/
Addressed the binding doc issue, dropped i.MX content.
For the firmware pinctrl scmi driver, addressed the comments from Cristian
For the pinctrl scmi driver, addressed comments from Cristian

For the i.MX95 OEM stuff, I not have good idea, expect using compatbile
string. Maybe the firmware public an protocol attribute to indicate it is
VENDOR stuff or NXP use a new protocol id, not 0x19. But I think
current pinctrl-scmi.c not able to support OEM config, should we extend
it with some method? Anyway if patch 1-4 is good enough, they could
be picked up first.

Since I am only able to test the patch on i.MX95 which not support
geneirc pinconf, only OEM configs are tested in my side.

---
Oleksii Moisieiev (1):
firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

Peng Fan (5):
firmware: arm_scmi: introduce helper get_max_msg_size
dt-bindings: firmware: arm,scmi: support pinctrl protocol
pinctrl: Implementation of the generic scmi-pinctrl driver
[NOT APPLY]firmware: scmi: support compatible string
[NOT APPLY] pinctrl: scmi: implement pinctrl_scmi_imx_dt_node_to_map

.../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++
MAINTAINERS | 7 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/bus.c | 39 +-
drivers/firmware/arm_scmi/common.h | 2 +-
drivers/firmware/arm_scmi/driver.c | 32 +-
drivers/firmware/arm_scmi/pinctrl.c | 930 +++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 3 +
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-scmi-imx.c | 117 +++
drivers/pinctrl/pinctrl-scmi.c | 540 ++++++++++++
drivers/pinctrl/pinctrl-scmi.h | 12 +
include/linux/scmi_protocol.h | 75 ++
14 files changed, 1806 insertions(+), 14 deletions(-)
---
base-commit: 8f266a167d1f0ca34668f05cd8c01bd245c8698b
change-id: 20231215-pinctrl-scmi-4c5b0374f4c6

Best regards,
--
Peng Fan <[email protected]>



2024-01-04 10:45:22

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH v2 1/6] firmware: arm_scmi: introduce helper get_max_msg_size

From: Peng Fan <[email protected]>

When Agent sending data to SCMI server, the Agent driver could check
the size to avoid protocol buffer overflow. So introduce the helper
get_max_msg_size.

Reviewed-by: Cristian Marussi <[email protected]>
Signed-off-by: Peng Fan <[email protected]>
---
drivers/firmware/arm_scmi/driver.c | 15 +++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 2 ++
2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3ea64b22cf0d..a12afc254afa 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1481,6 +1481,20 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
return ret;
}

+/**
+ * scmi_common_get_max_msg_size - Get maximum message size
+ * @ph: A protocol handle reference.
+ *
+ * Return: Maximum message size for the current protocol.
+ */
+static int scmi_common_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;
+}
+
/**
* struct scmi_iterator - Iterator descriptor
* @msg: A reference to the message TX buffer; filled by @prepare_message with
@@ -1756,6 +1770,7 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)

static const struct scmi_proto_helpers_ops helpers_ops = {
.extended_name_get = scmi_common_extended_name_get,
+ .get_max_msg_size = scmi_common_get_max_msg_size,
.iter_response_init = scmi_iterator_init,
.iter_response_run = scmi_iterator_run,
.fastchannel_init = scmi_common_fastchannel_init,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index e683c26f24eb..b5c65f26207b 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -254,6 +254,7 @@ struct scmi_fc_info {
* @fastchannel_init: A common helper used to initialize FC descriptors by
* gathering FC descriptions from the SCMI platform server.
* @fastchannel_db_ring: A common helper to ring a FC doorbell.
+ * @get_max_msg_size: A common helper to get the maximum message size.
*/
struct scmi_proto_helpers_ops {
int (*extended_name_get)(const struct scmi_protocol_handle *ph,
@@ -270,6 +271,7 @@ struct scmi_proto_helpers_ops {
void __iomem **p_addr,
struct scmi_fc_db_info **p_db);
void (*fastchannel_db_ring)(struct scmi_fc_db_info *db);
+ int (*get_max_msg_size)(const struct scmi_protocol_handle *ph);
};

/**

--
2.37.1


2024-01-04 10:45:45

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH v2 2/6] dt-bindings: firmware: arm,scmi: support pinctrl protocol

From: Peng Fan <[email protected]>

Add SCMI v3.2 pinctrl protocol bindings and example.

Signed-off-by: Peng Fan <[email protected]>
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 4591523b51a0..bba4a37de6b4 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -247,6 +247,37 @@ properties:
reg:
const: 0x18

+ protocol@19:
+ type: object
+ allOf:
+ - $ref: '#/$defs/protocol-node'
+ - $ref: /schemas/pinctrl/pinctrl.yaml
+
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ const: 0x19
+
+ patternProperties:
+ '-pins$':
+ type: object
+ allOf:
+ - $ref: /schemas/pinctrl/pincfg-node.yaml#
+ - $ref: /schemas/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:
@@ -401,6 +432,25 @@ examples:
scmi_powercap: protocol@18 {
reg = <0x18>;
};
+
+ scmi_pinctrl: protocol@19 {
+ reg = <0x19>;
+
+ i2c2-pins {
+ groups = "g_i2c2_a", "g_i2c2_b";
+ function = "f_i2c2";
+ };
+
+ mdio-pins {
+ groups = "g_avb_mdio";
+ drive-strength = <24>;
+ };
+
+ keys_pins: keys-pins {
+ pins = "gpio_5_17", "gpio_5_20", "gpio_5_22", "gpio_2_1";
+ bias-pull-up;
+ };
+ };
};
};


--
2.37.1


2024-01-04 10:46:08

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH v2 3/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

From: Oleksii Moisieiev <[email protected]>

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

Signed-off-by: Oleksii Moisieiev <[email protected]>
Co-developed-by: Peng Fan <[email protected]>
Signed-off-by: Peng Fan <[email protected]>
---
MAINTAINERS | 6 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 930 ++++++++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
include/linux/scmi_protocol.h | 73 +++
6 files changed, 1013 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0275a2c58f0f..487bff0d44c0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21322,6 +21322,12 @@ F: include/linux/sc[mp]i_protocol.h
F: include/trace/events/scmi.h
F: include/uapi/linux/virtio_scmi.h

+SYSTEM CONTROL MANAGEMENT INTERFACE (SCMI) PINCTRL DRIVER
+M: Oleksii Moisieiev <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/firmware/arm_scmi/pinctrl.c
+
SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel <[email protected]>
L: [email protected]
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..8e3874ff1544 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -11,6 +11,7 @@ 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 += 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 a12afc254afa..4c3752749105 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3050,6 +3050,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);
}
@@ -3067,6 +3068,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..aa6e698e7b7c
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,930 @@
+// 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"
+
+/* Updated only after ALL the mandatory features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x40000
+
+#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;
+ u32 *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;
+
+ 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;
+ struct scmi_conf_get_ipriv *p = priv;
+
+ if (!p->all) {
+ st->num_returned = 1;
+ st->num_remaining = 0;
+ } else {
+ 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, u32 *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,
+ u32 *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_type,
+ u32 *config_value)
+{
+ 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 = ph->hops->get_max_msg_size(ph);
+
+ if (!config_type || !config_value || 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(__le32) * 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(__le32),
+ 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_type[conf_num + i]);
+ tx->configs[i * 2 + 1] = cpu_to_le32(config_value[conf_num + 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, version);
+}
+
+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,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
+};
+
+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 b5c65f26207b..996410962b91 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -348,6 +348,7 @@ void __exit scmi_##name##_unregister(void) \
DECLARE_SCMI_REGISTER_UNREGISTER(base);
DECLARE_SCMI_REGISTER_UNREGISTER(clock);
DECLARE_SCMI_REGISTER_UNREGISTER(perf);
+DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
DECLARE_SCMI_REGISTER_UNREGISTER(power);
DECLARE_SCMI_REGISTER_UNREGISTER(reset);
DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..d8e2bb828ee7 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -717,6 +717,78 @@ 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, u32 *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, u32 *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, u32 *config_value);
+ int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
+ int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
+};
+
+enum scmi_pinctrl_conf_type {
+ SCMI_PIN_NONE = 0x0,
+ SCMI_PIN_BIAS_BUS_HOLD = 0x1,
+ SCMI_PIN_BIAS_DISABLE = 0x2,
+ SCMI_PIN_BIAS_HIGH_IMPEDANCE = 0x3,
+ SCMI_PIN_BIAS_PULL_UP = 0x4,
+ SCMI_PIN_BIAS_PULL_DEFAULT = 0x5,
+ SCMI_PIN_BIAS_PULL_DOWN = 0x6,
+ SCMI_PIN_DRIVE_OPEN_DRAIN = 0x7,
+ SCMI_PIN_DRIVE_OPEN_SOURCE = 0x8,
+ SCMI_PIN_DRIVE_PUSH_PULL = 0x9,
+ SCMI_PIN_DRIVE_STRENGTH = 0xA,
+ SCMI_PIN_INPUT_DEBOUNCE = 0xB,
+ SCMI_PIN_INPUT_MODE = 0xC,
+ SCMI_PIN_PULL_MODE = 0xD,
+ SCMI_PIN_INPUT_VALUE = 0xE,
+ SCMI_PIN_INPUT_SCHMITT = 0xF,
+ SCMI_PIN_LOW_POWER_MODE = 0x10,
+ SCMI_PIN_OUTPUT_MODE = 0x11,
+ SCMI_PIN_OUTPUT_VALUE = 0x12,
+ SCMI_PIN_POWER_SOURCE = 0x13,
+ SCMI_PIN_SLEW_RATE = 0x20,
+ SCMI_PIN_OEM_START = 0xC0,
+ SCMI_PIN_OEM_END = 0xFF,
+};
+
/**
* struct scmi_notify_ops - represents notifications' operations provided by
* SCMI core
@@ -824,6 +896,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.37.1


2024-01-04 10:46:26

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver

From: Peng Fan <[email protected]>

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.

Co-developed-by: Oleksii Moisieiev <[email protected]>
Signed-off-by: Oleksii Moisieiev <[email protected]>
Signed-off-by: Peng Fan <[email protected]>
---
MAINTAINERS | 1 +
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-scmi.c | 524 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 537 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 487bff0d44c0..3fe790e90834 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21314,6 +21314,7 @@ F: drivers/clk/clk-sc[mp]i.c
F: drivers/cpufreq/sc[mp]i-cpufreq.c
F: drivers/firmware/arm_scmi/
F: drivers/firmware/arm_scpi.c
+F: drivers/pinctrl/pinctrl-scmi.c
F: drivers/pmdomain/arm/
F: drivers/powercap/arm_scmi_powercap.c
F: drivers/regulator/scmi-regulator.c
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8163a5983166..27b26e428f60 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -432,6 +432,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 1071f301cc70..ba755ed2d46c 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..146308d27f54
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,524 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2023 EPAM
+ * Copyright 2023 NXP
+ */
+
+#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;
+ u8 *config_type;
+ u32 *config_value;
+};
+
+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);
+}
+
+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 = pinconf_generic_dt_node_to_map_all,
+ .dt_free_map = pinconf_generic_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_map_pinconf_type(enum pin_config_param param, u8 *type)
+{
+ u8 arg = param;
+
+ switch (arg) {
+ case PIN_CONFIG_BIAS_BUS_HOLD:
+ *type = SCMI_PIN_BIAS_BUS_HOLD;
+ break;
+ case PIN_CONFIG_BIAS_DISABLE:
+ *type = SCMI_PIN_BIAS_DISABLE;
+ break;
+ case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+ *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
+ break;
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ *type = SCMI_PIN_BIAS_PULL_DOWN;
+ break;
+ case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+ *type = SCMI_PIN_BIAS_PULL_DEFAULT;
+ break;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ *type = SCMI_PIN_BIAS_PULL_UP;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ *type = SCMI_PIN_DRIVE_OPEN_DRAIN;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ *type = SCMI_PIN_DRIVE_OPEN_SOURCE;
+ break;
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ *type = SCMI_PIN_DRIVE_PUSH_PULL;
+ break;
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ *type = SCMI_PIN_DRIVE_STRENGTH;
+ break;
+ case PIN_CONFIG_DRIVE_STRENGTH_UA:
+ *type = SCMI_PIN_DRIVE_STRENGTH;
+ break;
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ *type = SCMI_PIN_INPUT_DEBOUNCE;
+ break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ *type = SCMI_PIN_INPUT_MODE;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT:
+ *type = SCMI_PIN_INPUT_SCHMITT;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+ *type = SCMI_PIN_INPUT_MODE;
+ break;
+ case PIN_CONFIG_MODE_LOW_POWER:
+ *type = SCMI_PIN_LOW_POWER_MODE;
+ break;
+ case PIN_CONFIG_OUTPUT:
+ *type = SCMI_PIN_OUTPUT_VALUE;
+ break;
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ *type = SCMI_PIN_OUTPUT_MODE;
+ break;
+ case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
+ *type = SCMI_PIN_OUTPUT_VALUE;
+ break;
+ case PIN_CONFIG_POWER_SOURCE:
+ *type = SCMI_PIN_POWER_SOURCE;
+ break;
+ case PIN_CONFIG_SLEW_RATE:
+ *type = SCMI_PIN_SLEW_RATE;
+ break;
+ case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
+ *type = param;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+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;
+ u32 config_value;
+ u8 type;
+
+ if (!config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ return ret;
+ }
+
+ ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, 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);
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
+ &pmx->config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ return ret;
+ }
+ pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs,
+ pmx->config_type, pmx->config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d\n", ret);
+
+ 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);
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
+ &pmx->config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ return ret;
+ }
+ pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs,
+ pmx->config_type, pmx->config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d", ret);
+
+ 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;
+ u32 config_value;
+ u8 type;
+
+ if (!config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+ ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+
+ ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, 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->config_type = devm_kzalloc(dev, SZ_4K, GFP_KERNEL);
+ if (!pmx->config_type)
+ return -ENOMEM;
+
+ pmx->config_value = devm_kzalloc(dev, SZ_4K, GFP_KERNEL);
+ if (!pmx->config_value)
+ 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 <[email protected]>");
+MODULE_AUTHOR("Peng Fan <[email protected]>");
+MODULE_DESCRIPTION("ARM SCMI pin controller driver");
+MODULE_LICENSE("GPL");

--
2.37.1


2024-01-04 10:46:56

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH NOT APPLY v2 5/6] firmware: scmi: support compatible string

From: Peng Fan <[email protected]>

i.MX95 use SCMI PINCTRL OEM config type, so it could not reuse
pinctrl-scmi.c which use generic pinconf. Then need introduce
saying pinctrl-imx-scmi.c to support i.MX95, however both
pinctrl-scmi and pinctrl-imx-scmi driver will register a pinctrl device
with the same of_node(protocl@19). This will introduce an issue, because
the pinctrl subsystem will use of_node to find the exact pinctrl
device. But with two pinctrl devices registered by two drivers, it is
unknown which pinctrl device will be chosen.

So introduce compatible string to support cases that Vendor SCMI
driver which does almost same thing as generic SCMI driver, but
has some specific Vendor settings to do.

Signed-off-by: Peng Fan <[email protected]>
---
drivers/firmware/arm_scmi/bus.c | 39 ++++++++++++++++++++++++++++++--------
drivers/firmware/arm_scmi/common.h | 2 +-
drivers/firmware/arm_scmi/driver.c | 15 ++++++++++-----
include/linux/scmi_protocol.h | 2 ++
4 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 1f9735dbebec..9043f6366fa8 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -214,13 +214,21 @@ scmi_dev_match_id(struct scmi_device *scmi_dev, struct scmi_driver *scmi_drv)
if (!id)
return NULL;

- for (; id->protocol_id; id++)
- if (id->protocol_id == scmi_dev->protocol_id) {
- if (!id->name)
+ for (; id->protocol_id; id++) {
+ if (id->protocol_id != scmi_dev->protocol_id)
+ continue;
+ if (!id->name)
+ return id;
+
+ if (!strcmp(id->name, scmi_dev->name)) {
+ if (!id->compatible &&
+ device_property_read_string(&scmi_dev->dev, "compatible", NULL))
return id;
- else if (!strcmp(id->name, scmi_dev->name))
+
+ if (id->compatible && device_is_compatible(&scmi_dev->dev, id->compatible))
return id;
}
+ }

return NULL;
}
@@ -345,11 +353,14 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)

static struct scmi_device *
__scmi_device_create(struct device_node *np, struct device *parent,
- int protocol, const char *name)
+ int protocol, const char *name, const char *compatible)
{
int id, retval;
struct scmi_device *scmi_dev;

+ if (compatible && !of_device_is_compatible(np, compatible))
+ return NULL;
+
/*
* If the same protocol/name device already exist under the same parent
* (i.e. SCMI instance) just return the existent device.
@@ -425,6 +436,7 @@ __scmi_device_create(struct device_node *np, struct device *parent,
* @name: The requested-name of the device to be created; this is optional
* and if no @name is provided, all the devices currently known to
* be requested on the SCMI bus for @protocol will be created.
+ * @compatible: The compatible string
*
* This method can be invoked to create a single well-defined device (like
* a transport device or a device requested by an SCMI driver loaded after
@@ -441,14 +453,14 @@ __scmi_device_create(struct device_node *np, struct device *parent,
*/
struct scmi_device *scmi_device_create(struct device_node *np,
struct device *parent, int protocol,
- const char *name)
+ const char *name, const char *compatible)
{
struct list_head *phead;
struct scmi_requested_dev *rdev;
struct scmi_device *scmi_dev = NULL;

if (name)
- return __scmi_device_create(np, parent, protocol, name);
+ return __scmi_device_create(np, parent, protocol, name, compatible);

mutex_lock(&scmi_requested_devices_mtx);
phead = idr_find(&scmi_requested_devices, protocol);
@@ -462,9 +474,20 @@ struct scmi_device *scmi_device_create(struct device_node *np,
list_for_each_entry(rdev, phead, node) {
struct scmi_device *sdev;

+ if (compatible) {
+ if (!rdev->id_table->compatible)
+ continue;
+ if (strcmp(compatible, rdev->id_table->compatible))
+ continue;
+ } else {
+ if (rdev->id_table->compatible)
+ continue;
+ }
+
sdev = __scmi_device_create(np, parent,
rdev->id_table->protocol_id,
- rdev->id_table->name);
+ rdev->id_table->name,
+ rdev->id_table->compatible);
/* Report errors and carry on... */
if (sdev)
scmi_dev = sdev;
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 00b165d1f502..ac0d83ed5803 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -149,7 +149,7 @@ extern struct blocking_notifier_head scmi_requested_devices_nh;

struct scmi_device *scmi_device_create(struct device_node *np,
struct device *parent, int protocol,
- const char *name);
+ const char *name, const char *compatible);
void scmi_device_destroy(struct device *parent, int protocol, const char *name);

int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 4c3752749105..b78fce62a0de 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -261,15 +261,17 @@ EXPORT_SYMBOL_GPL(scmi_protocol_unregister);
* @name: The optional name of the device to be created: if not provided this
* call will lead to the creation of all the devices currently requested
* for the specified protocol.
+ * @compatible: optional, the compatible string
*/
static void scmi_create_protocol_devices(struct device_node *np,
struct scmi_info *info,
- int prot_id, const char *name)
+ int prot_id, const char *name,
+ const char *compatible)
{
struct scmi_device *sdev;

mutex_lock(&info->devreq_mtx);
- sdev = scmi_device_create(np, info->dev, prot_id, name);
+ sdev = scmi_device_create(np, info->dev, prot_id, name, compatible);
if (name && !sdev)
dev_err(info->dev,
"failed to create device for protocol 0x%X (%s)\n",
@@ -2354,7 +2356,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
snprintf(name, 32, "__scmi_transport_device_%s_%02X",
idx ? "rx" : "tx", prot_id);
/* Create a uniquely named, dedicated transport device for this chan */
- tdev = scmi_device_create(of_node, info->dev, prot_id, name);
+ tdev = scmi_device_create(of_node, info->dev, prot_id, name, NULL);
if (!tdev) {
dev_err(info->dev,
"failed to create transport device (%s)\n", name);
@@ -2550,7 +2552,7 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
switch (action) {
case SCMI_BUS_NOTIFY_DEVICE_REQUEST:
scmi_create_protocol_devices(np, info, id_table->protocol_id,
- id_table->name);
+ id_table->name, id_table->compatible);
break;
case SCMI_BUS_NOTIFY_DEVICE_UNREQUEST:
scmi_destroy_protocol_devices(info, id_table->protocol_id,
@@ -2802,10 +2804,13 @@ static int scmi_probe(struct platform_device *pdev)

for_each_available_child_of_node(np, child) {
u32 prot_id;
+ const char *s = NULL;

if (of_property_read_u32(child, "reg", &prot_id))
continue;

+ of_property_read_string(child, "compatible", &s);
+
if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id))
dev_err(dev, "Out of range protocol %d\n", prot_id);

@@ -2828,7 +2833,7 @@ static int scmi_probe(struct platform_device *pdev)
}

of_node_get(child);
- scmi_create_protocol_devices(child, info, prot_id, NULL);
+ scmi_create_protocol_devices(child, info, prot_id, NULL, s);
}

return 0;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index d8e2bb828ee7..ee7b77eb9466 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -912,6 +912,7 @@ struct scmi_device {
u32 id;
u8 protocol_id;
const char *name;
+ const char *compatible; /* Optional */
struct device dev;
struct scmi_handle *handle;
};
@@ -921,6 +922,7 @@ struct scmi_device {
struct scmi_device_id {
u8 protocol_id;
const char *name;
+ const char *compatible; /* Optional */
};

struct scmi_driver {

--
2.37.1


2024-01-04 10:47:18

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH NOT APPLY v2 6/6] pinctrl: scmi: implement pinctrl_scmi_imx_dt_node_to_map

From: Peng Fan <[email protected]>

i.MX95 System Manager FW supports SCMI PINCTRL protocol, but uses
OEM Pin Configuration type, so need i.MX specific dt_node_to_map.

Signed-off-by: Peng Fan <[email protected]>
---
drivers/pinctrl/Makefile | 2 +-
drivers/pinctrl/pinctrl-scmi-imx.c | 117 +++++++++++++++++++++++++++++++++++++
drivers/pinctrl/pinctrl-scmi.c | 16 +++++
drivers/pinctrl/pinctrl-scmi.h | 12 ++++
4 files changed, 146 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ba755ed2d46c..d96b7ede1355 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -44,7 +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_SCMI) += pinctrl-scmi.o pinctrl-scmi-imx.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-imx.c b/drivers/pinctrl/pinctrl-scmi-imx.c
new file mode 100644
index 000000000000..e9d02e4c2cc1
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi-imx.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2023 NXP
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinctrl-scmi.h"
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinconf.h"
+
+/* SCMI pin control types, aligned with SCMI firmware */
+#define IMX_SCMI_PIN_TYPE_MUX 192
+#define IMX_SCMI_PIN_TYPE_CONFIG 193
+#define IMX_SCMI_PIN_TYPE_DAISY_ID 194
+#define IMX_SCMI_PIN_TYPE_DAISY_CFG 195
+
+#define IMX_SCMI_NO_PAD_CTL BIT(31)
+#define IMX_SCMI_PAD_SION BIT(30)
+#define IMX_SCMI_IOMUXC_CONFIG_SION BIT(4)
+
+#define IMX_SCMI_NUM_CFG 4
+#define IMX_SCMI_PIN_SIZE 24
+
+int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned int *num_maps)
+{
+ struct pinctrl_map *new_map;
+ const __be32 *list;
+ unsigned long *configs = NULL;
+ unsigned long cfg[IMX_SCMI_NUM_CFG];
+ int map_num, size, pin_size, pin_id, num_pins;
+ int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val;
+ int i, j;
+ uint32_t ncfg;
+ static uint32_t daisy_off;
+
+ if (!daisy_off) {
+ if (of_machine_is_compatible("fsl,imx95"))
+ daisy_off = 0x408;
+ else
+ dev_err(pctldev->dev, "platform not support scmi pinctrl\n");
+ }
+
+ list = of_get_property(np, "fsl,pins", &size);
+ if (!list) {
+ dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ pin_size = IMX_SCMI_PIN_SIZE;
+
+ if (!size || size % pin_size) {
+ dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ num_pins = size / pin_size;
+ map_num = num_pins;
+
+ new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map),
+ GFP_KERNEL);
+ if (!new_map)
+ return -ENOMEM;
+
+ *map = new_map;
+ *num_maps = map_num;
+
+ /* create config map */
+ for (i = 0; i < num_pins; i++) {
+ j = 0;
+ ncfg = IMX_SCMI_NUM_CFG;
+ mux_reg = be32_to_cpu(*list++);
+ conf_reg = be32_to_cpu(*list++);
+ input_reg = be32_to_cpu(*list++);
+ mux_val = be32_to_cpu(*list++);
+ input_val = be32_to_cpu(*list++);
+ conf_val = be32_to_cpu(*list++);
+ if (conf_val & IMX_SCMI_PAD_SION)
+ mux_val |= IMX_SCMI_IOMUXC_CONFIG_SION;
+
+ pin_id = mux_reg / 4;
+
+ cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_MUX, mux_val);
+
+ if (!conf_reg || (conf_val & IMX_SCMI_NO_PAD_CTL))
+ ncfg--;
+ else
+ cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_CONFIG, conf_val);
+
+ if (!input_reg) {
+ ncfg -= 2;
+ } else {
+ cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_DAISY_ID,
+ (input_reg - daisy_off) / 4);
+ cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_DAISY_CFG, input_val);
+ }
+
+ configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
+
+ new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id);
+ new_map[i].data.configs.configs = configs;
+ new_map[i].data.configs.num_configs = ncfg;
+ }
+
+ return 0;
+}
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 146308d27f54..b49cbfbddbc9 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -9,6 +9,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/seq_file.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
@@ -19,6 +20,7 @@
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>

+#include "pinctrl-scmi.h"
#include "pinctrl-utils.h"
#include "core.h"
#include "pinconf.h"
@@ -89,6 +91,16 @@ static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
#endif
};

+static const struct pinctrl_ops pinctrl_scmi_imx_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_imx_dt_node_to_map,
+ .dt_free_map = pinconf_generic_dt_free_map,
+#endif
+};
+
static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
{
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
@@ -439,6 +451,7 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,

static const struct scmi_device_id scmi_id_table[] = {
{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+ { SCMI_PROTOCOL_PINCTRL, "pinctrl-scmi-imx", "fsl,imx95-scmi-pinctrl" },
{ }
};
MODULE_DEVICE_TABLE(scmi, scmi_id_table);
@@ -481,6 +494,9 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;

+ if (device_is_compatible(dev, "fsl,imx95-scmi-pinctrl"))
+ pmx->pctl_desc.pctlops = &pinctrl_scmi_imx_pinctrl_ops;
+
ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
&pmx->pctl_desc.pins);
if (ret)
diff --git a/drivers/pinctrl/pinctrl-scmi.h b/drivers/pinctrl/pinctrl-scmi.h
new file mode 100644
index 000000000000..25863b4428fe
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2023 NXP
+ */
+
+#ifndef __DRIVERS_PINCTRL_SCMI_H
+#define __DRIVERS_PINCTRL_SCMI_H
+
+int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned int *num_maps);
+
+#endif /* __DRIVERS_PINCTRL_SCMI_H */

--
2.37.1


2024-01-05 01:38:07

by AKASHI Takahiro

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Thu, Jan 04, 2024 at 06:48:44PM +0800, Peng Fan (OSS) wrote:
> This patchset is a rework from Oleksii's RFC v5 patchset
> https://lore.kernel.org/all/[email protected]/
>
> This patchset introduces some changes based on RFC v5:
> - introduce helper get_max_msg_size
> - support compatible string
> - iterate the id_table
> - Support multiple configs in one command
> - Added i.MX support
> - Patch 5 firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
> is almost same as RFCv5 expect multiple configs support.
> - Patch 4 the dt-bindings includes compatible string to support i.MX
> - Rebased on 2023-12-15 linux-next/master

For the record, the translation of pin config types,
pinctrl_scmi_map_pinconf_type(), is finally implemented in patch 4.

Thanks,
-Takahiro Akashi

> If any comments from RFC v5 are missed, I am sorry in advance.
>
> This PINCTRL Protocol is following Version 3.2 SCMI Spec Beta release.
>
> 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. So implement the driver to support such cases.
>
> The i.MX95 Example as below:
>
> Configuration:
> The scmi-pinctrl driver can be configured using DT bindings.
> For example:
> / {
> sram0: sram@445b1000 {
> compatible = "mmio-sram";
> reg = <0x0 0x445b1000 0x0 0x400>;
>
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x0 0x0 0x445b1000 0x400>;
>
> scmi_buf0: scmi-sram-section@0 {
> compatible = "arm,scmi-shmem";
> reg = <0x0 0x80>;
> };
>
> scmi_buf1: scmi-sram-section@80 {
> compatible = "arm,scmi-shmem";
> reg = <0x80 0x80>;
> };
> };
>
> firmware {
> scmi {
> compatible = "arm,scmi";
> mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> shmem = <&scmi_buf0>, <&scmi_buf1>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> scmi_iomuxc: protocol@19 {
> compatible = "fsl,imx95-scmi-pinctrl";
> reg = <0x19>;
> };
> };
> };
> };
>
> &scmi_iomuxc {
> pinctrl_tpm3: tpm3grp {
> fsl,pins = <
> IMX95_PAD_GPIO_IO12__TPM3_CH2 0x51e
> >;
> };
> };
>
> This patchset has been tested on i.MX95-19x19-EVK board.
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> Changes in v2:
> Added comments, and added R-b for Patch 1
> Moved the compatile string and i.MX patch to the end, marked NOT APPLY
> Patchset based on lore.kernel.org/all/[email protected]/
> Addressed the binding doc issue, dropped i.MX content.
> For the firmware pinctrl scmi driver, addressed the comments from Cristian
> For the pinctrl scmi driver, addressed comments from Cristian
>
> For the i.MX95 OEM stuff, I not have good idea, expect using compatbile
> string. Maybe the firmware public an protocol attribute to indicate it is
> VENDOR stuff or NXP use a new protocol id, not 0x19. But I think
> current pinctrl-scmi.c not able to support OEM config, should we extend
> it with some method? Anyway if patch 1-4 is good enough, they could
> be picked up first.
>
> Since I am only able to test the patch on i.MX95 which not support
> geneirc pinconf, only OEM configs are tested in my side.
>
> ---
> Oleksii Moisieiev (1):
> firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
>
> Peng Fan (5):
> firmware: arm_scmi: introduce helper get_max_msg_size
> dt-bindings: firmware: arm,scmi: support pinctrl protocol
> pinctrl: Implementation of the generic scmi-pinctrl driver
> [NOT APPLY]firmware: scmi: support compatible string
> [NOT APPLY] pinctrl: scmi: implement pinctrl_scmi_imx_dt_node_to_map
>
> .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++
> MAINTAINERS | 7 +
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/bus.c | 39 +-
> drivers/firmware/arm_scmi/common.h | 2 +-
> drivers/firmware/arm_scmi/driver.c | 32 +-
> drivers/firmware/arm_scmi/pinctrl.c | 930 +++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 3 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi-imx.c | 117 +++
> drivers/pinctrl/pinctrl-scmi.c | 540 ++++++++++++
> drivers/pinctrl/pinctrl-scmi.h | 12 +
> include/linux/scmi_protocol.h | 75 ++
> 14 files changed, 1806 insertions(+), 14 deletions(-)
> ---
> base-commit: 8f266a167d1f0ca34668f05cd8c01bd245c8698b
> change-id: 20231215-pinctrl-scmi-4c5b0374f4c6
>
> Best regards,
> --
> Peng Fan <[email protected]>
>

2024-01-09 21:18:12

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: arm,scmi: support pinctrl protocol


On Thu, 04 Jan 2024 18:48:46 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> Add SCMI v3.2 pinctrl protocol bindings and example.
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>


2024-01-10 14:23:49

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: arm,scmi: support pinctrl protocol

On Thu, Jan 4, 2024 at 11:44 AM Peng Fan (OSS) <[email protected]> wrote:

> From: Peng Fan <[email protected]>
>
> Add SCMI v3.2 pinctrl protocol bindings and example.
>
> Signed-off-by: Peng Fan <[email protected]>

This looks good to me.
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2024-01-17 19:39:01

by Cristian Marussi

[permalink] [raw]
Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Thu, Jan 04, 2024 at 06:48:47PM +0800, Peng Fan (OSS) wrote:
> From: Oleksii Moisieiev <[email protected]>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.

Hi Peng,

a few minor remarks below but this LGTM, and in my testing I could not
find any issue/misbehaviour at the protocol layer (i.e. stressing pinctrl_ops)

In general, can we stick to 80cols ? since it is how the SCMI stack is
currently formatted and this patch introduced just few minor lomng lines
that can be easily wrapped by moving a few params (or return types) on a
new line....

..but only when this does not render the thing unreadable of course, I
mean just in the obvious places where it is just a matter of hitting a
Return :D

>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Co-developed-by: Peng Fan <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>

[nitpick]: I have a silly spelling mistakes on email from checkpatch

CHECK: From:/Signed-off-by: email comments mismatch: 'From: Oleksii Moisieiev <[email protected]>' != 'Signed-off-by: Oleksii Moisieiev <[email protected]>

> ---
> MAINTAINERS | 6 +
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 930 ++++++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 73 +++
> 6 files changed, 1013 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0275a2c58f0f..487bff0d44c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21322,6 +21322,12 @@ F: include/linux/sc[mp]i_protocol.h
> F: include/trace/events/scmi.h
> F: include/uapi/linux/virtio_scmi.h
>
> +SYSTEM CONTROL MANAGEMENT INTERFACE (SCMI) PINCTRL DRIVER
> +M: Oleksii Moisieiev <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/firmware/arm_scmi/pinctrl.c
> +

I've got to check this with Sudeep since giving exclusive maintainership
of this bit could lead to troubles if the assigned maintainer becomes
unresponsive, and it seems this is already the case indeed even before
this thing is merged. Oleksii and EPAM will anyway maintain authorship
in any case; I'll check with Sudeep, as said.

> SYSTEM RESET/SHUTDOWN DRIVERS
> M: Sebastian Reichel <[email protected]>
> L: [email protected]
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..8e3874ff1544 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ 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 += 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 a12afc254afa..4c3752749105 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3050,6 +3050,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);
> }
> @@ -3067,6 +3068,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..aa6e698e7b7c
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,930 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2023 EPAM

2023-2024

> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +#include "protocols.h"
> +
> +/* Updated only after ALL the mandatory features for that version are merged */
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x40000

Why 0x40000, this should be 0x10000 I think looking at SCMI Pinctrl on v3.2-bet3,
BUT there are still other missing (minor) features as of now related to v3.2-bet3
(that impacts on all protocols like NEGOTIATE_PROTOCOL) and I will add those missing
feats in the SCMI core shortly in the upcoming weeks.

So, for the moment I would set this to 0x0, and I will bump it up to 0x10000
when the still missing v3.2-bet3 features are all in, because if not we would
be lying here saying that we now support Pinctrl 0x10000, while instead
something is still missing.

> +
> +#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))

[snip]

> +
> +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,
> + u32 *config_values)

This function is a bit odd, since you provide a pair of arrays and their
size to fetch all the results BUT there is no way, agfter the call, to know how
many valid results have been filled in the array, so it is a bit difficult
to use properly (given also that 0/0 is a valid config/val pair), BUT on the
other side I cannot see this being called from anywhere as of now, so I think
it can be left as it is and maybe will fix in the future when needed.

Another option could be to drop it, but it worked fine anyway when tested, and
needs only a minor fix (as said above) so I would keep even though unused.

Up to you. No strong opinion since it does not misbehave anyway, it is
just awkward a bit...

> +{
> + 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);
> +}

[snip]

> DECLARE_SCMI_REGISTER_UNREGISTER(reset);
> DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..d8e2bb828ee7 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -717,6 +717,78 @@ 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, u32 *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, u32 *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, u32 *config_value);
> + int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
> + int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
> +};
> +
> +enum scmi_pinctrl_conf_type {
> + SCMI_PIN_NONE = 0x0,
> + SCMI_PIN_BIAS_BUS_HOLD = 0x1,
> + SCMI_PIN_BIAS_DISABLE = 0x2,
> + SCMI_PIN_BIAS_HIGH_IMPEDANCE = 0x3,
> + SCMI_PIN_BIAS_PULL_UP = 0x4,
> + SCMI_PIN_BIAS_PULL_DEFAULT = 0x5,
> + SCMI_PIN_BIAS_PULL_DOWN = 0x6,
> + SCMI_PIN_DRIVE_OPEN_DRAIN = 0x7,
> + SCMI_PIN_DRIVE_OPEN_SOURCE = 0x8,
> + SCMI_PIN_DRIVE_PUSH_PULL = 0x9,
> + SCMI_PIN_DRIVE_STRENGTH = 0xA,
> + SCMI_PIN_INPUT_DEBOUNCE = 0xB,
> + SCMI_PIN_INPUT_MODE = 0xC,
> + SCMI_PIN_PULL_MODE = 0xD,
> + SCMI_PIN_INPUT_VALUE = 0xE,
> + SCMI_PIN_INPUT_SCHMITT = 0xF,
> + SCMI_PIN_LOW_POWER_MODE = 0x10,
> + SCMI_PIN_OUTPUT_MODE = 0x11,
> + SCMI_PIN_OUTPUT_VALUE = 0x12,
> + SCMI_PIN_POWER_SOURCE = 0x13,
> + SCMI_PIN_SLEW_RATE = 0x20,
> + SCMI_PIN_OEM_START = 0xC0,
> + SCMI_PIN_OEM_END = 0xFF,
> +};

Can you move this enum above the pinctrl_proto_ops for consistency?

Maybe use it in the ops prototype as the type of config_type param or
is there a reason to stick with u8 config_type even having the enum ?
We anyway place a u32 in the message at the end, so it does not seem
any gain in size.

Thanks,
Cristian

2024-01-17 20:43:29

by Cristian Marussi

[permalink] [raw]
Subject: Re: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver

On Thu, Jan 04, 2024 at 06:48:48PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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.
>

Hi,

a few remarks.

> Co-developed-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 524 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 537 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 487bff0d44c0..3fe790e90834 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21314,6 +21314,7 @@ F: drivers/clk/clk-sc[mp]i.c
> F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 8163a5983166..27b26e428f60 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -432,6 +432,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 1071f301cc70..ba755ed2d46c 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..146308d27f54
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,524 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2023 EPAM
> + * Copyright 2023 NXP
> + */

2023-2024

> +
> +#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"

[snip]

> +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_map_pinconf_type(enum pin_config_param param, u8 *type)
> +{

Same as in the previous patch, is it reasonable to use the new enum here instead
of a generic u8 * ?

> + u8 arg = param;
> +
> + switch (arg) {
> + case PIN_CONFIG_BIAS_BUS_HOLD:
> + *type = SCMI_PIN_BIAS_BUS_HOLD;
> + break;
> + case PIN_CONFIG_BIAS_DISABLE:
> + *type = SCMI_PIN_BIAS_DISABLE;
> + break;
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + *type = SCMI_PIN_BIAS_PULL_DOWN;
> + break;
> + case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> + *type = SCMI_PIN_BIAS_PULL_DEFAULT;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + *type = SCMI_PIN_BIAS_PULL_UP;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + *type = SCMI_PIN_DRIVE_OPEN_DRAIN;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> + *type = SCMI_PIN_DRIVE_OPEN_SOURCE;
> + break;
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + *type = SCMI_PIN_DRIVE_PUSH_PULL;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_INPUT_DEBOUNCE:
> + *type = SCMI_PIN_INPUT_DEBOUNCE;
> + break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT:
> + *type = SCMI_PIN_INPUT_SCHMITT;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_MODE_LOW_POWER:
> + *type = SCMI_PIN_LOW_POWER_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + *type = SCMI_PIN_OUTPUT_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_POWER_SOURCE:
> + *type = SCMI_PIN_POWER_SOURCE;
> + break;
> + case PIN_CONFIG_SLEW_RATE:
> + *type = SCMI_PIN_SLEW_RATE;
> + break;
> + case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
> + *type = param;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +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;
> + u32 config_value;
> + u8 type;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +

Issuing a cat /sys/kernel/debug/pictrl/<scmi_dev>/pinconf-pins
got a string of (adding some debug to print config_type):

[ 89.843868] scmi-pinctrl scmi_dev.12: Error map pinconf_type 22 - ret:-95
[ 89.846125] scmi-pinctrl scmi_dev.12: Error map pinconf_type 23 - ret:-95

but I suppose is due to some missing matching definitions in the SCMI
spec....just a heads up for the future.

> + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, 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);
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
> + &pmx->config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> + pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +

Are you sure that this pinctrl function cannot be called multiple times
concurrently ? Because pmx->config_type/value are globally defined for
the driver so any concurrent call will lead to corruption.

Moreover, you allocate those pmx->config_ arrays in probe as SZ_4K, but
effectively how many of these are effectively used in reality ?

I mean, we clearly have to address the worst case, which is 256 values by
the SCMI spec CONFIG_SET definitions, but typically how many configs are
going to be sent in a call ? because if it is a reasonable small values
you could just put in on the stack here with a check for overlflow and
a dynamic allocation to address the special case...this would also solve
the potential concurrent calls issue described above.

..but I could be missing something about Pinctrl so please explain.

> + ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs,
> + pmx->config_type, pmx->config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d\n", ret);
> +
> + 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);
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
> + &pmx->config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> + pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs,
> + pmx->config_type, pmx->config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d", ret);
> +
> + 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;
> + u32 config_value;
> + u8 type;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);

You dont check ret here ?

> +
> + ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, type, &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +

Thanks,
Cristian

2024-01-28 00:00:37

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Wed, Jan 17, 2024 at 8:38 PM Cristian Marussi
<[email protected]> wrote:

> > +SYSTEM CONTROL MANAGEMENT INTERFACE (SCMI) PINCTRL DRIVER
> > +M: Oleksii Moisieiev <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/firmware/arm_scmi/pinctrl.c
> > +
>
> I've got to check this with Sudeep since giving exclusive maintainership
> of this bit could lead to troubles if the assigned maintainer becomes
> unresponsive, and it seems this is already the case indeed even before
> this thing is merged. Oleksii and EPAM will anyway maintain authorship
> in any case; I'll check with Sudeep, as said.

As maintainer I regularly just lose patience and apply patches if the
driver maintainer becomes unresponsive so that's what is to be expected.

That, said: the more maintainers, the better. In this case some co-maintainer
@arm.com would be nice. Just list several.

Yours,
Linus Walleij

2024-01-28 00:06:49

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver

Hi Peng,

thanks for your patch!

On Thu, Jan 4, 2024 at 11:45 AM Peng Fan (OSS) <[email protected]> wrote:

> From: Peng Fan <[email protected]>
>
> 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.
>
> Co-developed-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
(...)
> +#ifdef CONFIG_OF
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
> +#endif

This looks like with !OF the driver becomes pretty unusable doesn't it?

Should we just depend on OF in Kconfig or are there already plans for
ACPI or similar?

Yours,
Linus Walleij

2024-01-28 00:28:58

by Peng Fan

[permalink] [raw]
Subject: RE: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver

Hi Linus,

> Subject: Re: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-
> pinctrl driver
>
> Hi Peng,
>
> thanks for your patch!
>
> On Thu, Jan 4, 2024 at 11:45 AM Peng Fan (OSS) <[email protected]>
> wrote:
>
> > From: Peng Fan <[email protected]>
> >
> > 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.
> >
> > Co-developed-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Peng Fan <[email protected]>
> (...)
> > +#ifdef CONFIG_OF
> > + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> > + .dt_free_map = pinconf_generic_dt_free_map, #endif
>
> This looks like with !OF the driver becomes pretty unusable doesn't it?
>
> Should we just depend on OF in Kconfig or are there already plans for ACPI or
> similar?

I not have platforms that has ACPI support. Cristian, Sudeep do you have
plans?

I could add depends on OF in V4, since current patch version is
https://lore.kernel.org/all/[email protected]/

or I could use a follow up patch after V4 patch 1-4 applied.

Would you ACK the current V3 patch or wait I add OF dependency?

Thanks,
Peng.

>
> Yours,
> Linus Walleij

2024-01-28 00:40:12

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver

On Sun, Jan 28, 2024 at 1:28 AM Peng Fan <[email protected]> wrote:

> Would you ACK the current V3 patch or wait I add OF dependency?

I ACKed it, don't worry it's just a nitpick.

Yours,
Linus Walleij