LinuxLists.cc - [PATCH v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[permalink] [raw]

Subject: [PATCH v5 1/4] 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 2709598f3008..415e6f510057 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1488,6 +1488,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
@@ -1799,6 +1813,7 @@ static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,

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,
.protocol_msg_check = scmi_protocol_msg_check,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 317d3fb32676..3e91536a77a3 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -258,6 +258,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,
@@ -277,6 +278,7 @@ struct scmi_proto_helpers_ops {
struct scmi_fc_db_info **p_db,
u32 *rate_limit);
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-03-14 13:27:54

[permalink] [raw]

Subject: [PATCH v5 2/4] dt-bindings: firmware: arm,scmi: support pinctrl protocol

From: Peng Fan <[email protected]>

Add SCMI v3.2 pinctrl protocol bindings and example.

Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
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-03-14 13:28:10

[permalink] [raw]

Subject: [PATCH v5 3/4] 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.

Reviewed-by: Cristian Marussi <[email protected]>
Tested-by: Cristian Marussi <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Oleksii Moisieiev <[email protected]>
Co-developed-by: Peng Fan <[email protected]>
Signed-off-by: Peng Fan <[email protected]>
---
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 908 ++++++++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
include/linux/scmi_protocol.h | 75 +++
5 files changed, 987 insertions(+)

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 415e6f510057..ac2d4b19727c 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3142,6 +3142,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);
}
@@ -3159,6 +3160,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..0fcfa4269473
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,908 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Pinctrl Protocol
+ *
+ * Copyright (C) 2024 EPAM
+ * Copyright 2024 NXP
+ */
+
+#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 0x0
+
+#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;
+ enum scmi_pinctrl_conf_type *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,
+ enum scmi_pinctrl_conf_type 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_set(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ unsigned int nr_configs,
+ enum scmi_pinctrl_conf_type *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_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 3e91536a77a3..c02cbfd2bb03 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -355,6 +355,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 b807141acc14..fd647f805e5f 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -737,6 +737,80 @@ struct scmi_powercap_proto_ops {
u32 *power_thresh_high);
};

+enum scmi_pinctrl_selector_type {
+ PIN_TYPE = 0,
+ GROUP_TYPE,
+ FUNCTION_TYPE,
+};
+
+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_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,
+ enum scmi_pinctrl_conf_type config_type,
+ u32 *config_value);
+ int (*config_set)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ unsigned int nr_configs,
+ enum scmi_pinctrl_conf_type *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);
+};
+
/**
* struct scmi_notify_ops - represents notifications' operations provided by
* SCMI core
@@ -844,6 +918,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-03-14 13:29:36

[permalink] [raw]

Subject: [PATCH v5 4/4] 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.

Reviewed-by: Cristian Marussi <[email protected]>
Tested-by: Cristian Marussi <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
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 | 593 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 606 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b43102ca365d..aa9eb9476bbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21474,6 +21474,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
F: drivers/firmware/arm_scmi/
F: drivers/firmware/arm_scpi.c
F: drivers/hwmon/scmi-hwmon.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 d45657aa986a..4e6f65cf0e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -450,6 +450,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 2152539b53d5..cc809669405a 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -45,6 +45,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..f2fef3fb85ae
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,593 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2024 EPAM
+ * Copyright 2024 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"
+
+/* Define num configs, if not large than 4 use stack, else use kcalloc */
+#define SCMI_NUM_CONFIGS 4
+
+static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
+
+struct scmi_pinctrl_funcs {
+ unsigned int num_groups;
+ const char **groups;
+};
+
+struct scmi_pinctrl {
+ struct device *dev;
+ struct scmi_protocol_handle *ph;
+ struct pinctrl_dev *pctldev;
+ struct pinctrl_desc pctl_desc;
+ struct scmi_pinctrl_funcs *functions;
+ unsigned int nr_functions;
+ char **groups;
+ unsigned int nr_groups;
+ struct pinctrl_pin_desc *pins;
+ unsigned int nr_pins;
+};
+
+static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE);
+}
+
+static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ int ret;
+ const char *name;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name);
+ if (ret) {
+ dev_err(pmx->dev, "get name failed with err %d", ret);
+ return NULL;
+ }
+
+ return name;
+}
+
+static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins);
+}
+
+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,
+ enum scmi_pinctrl_conf_type *type)
+{
+ u32 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 = arg;
+ 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;
+ enum scmi_pinctrl_conf_type type;
+ u32 config_value;
+
+ 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_alloc_configs(struct pinctrl_dev *pctldev, u32 num_configs,
+ u32 **p_config_value,
+ enum scmi_pinctrl_conf_type **p_config_type)
+{
+ if (num_configs <= SCMI_NUM_CONFIGS)
+ return 0;
+
+ *p_config_value = kcalloc(num_configs, sizeof(u32), GFP_KERNEL);
+ *p_config_type = kcalloc(num_configs,
+ sizeof(enum scmi_pinctrl_conf_type),
+ GFP_KERNEL);
+
+ if (!*p_config_value || !*p_config_type) {
+ kfree(*p_config_value);
+ kfree(*p_config_type);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void
+pinctrl_scmi_free_configs(struct pinctrl_dev *pctldev, u32 num_configs,
+ u32 **p_config_value,
+ enum scmi_pinctrl_conf_type **p_config_type)
+{
+ if (num_configs <= SCMI_NUM_CONFIGS)
+ return;
+
+ kfree(*p_config_value);
+ kfree(*p_config_type);
+}
+
+static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
+ u32 config_value[SCMI_NUM_CONFIGS];
+ enum scmi_pinctrl_conf_type *p_config_type = config_type;
+ u32 *p_config_value = config_value;
+ enum pin_config_param param;
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ ret = pinctrl_scmi_map_pinconf_type(param, &p_config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ goto free_config;
+ }
+ p_config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs,
+ p_config_type, p_config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d\n", ret);
+
+free_config:
+ pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ return ret;
+}
+
+static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
+ unsigned int group,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
+ u32 config_value[SCMI_NUM_CONFIGS];
+ enum scmi_pinctrl_conf_type *p_config_type = config_type;
+ u32 *p_config_value = config_value;
+ enum pin_config_param param;
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ ret = pinctrl_scmi_map_pinconf_type(param,
+ &p_config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ goto free_config;
+ }
+
+ p_config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs,
+ p_config_type, p_config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d", ret);
+
+free_config:
+ pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ 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;
+ enum scmi_pinctrl_conf_type type;
+ u32 config_value;
+
+ 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, 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->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-03-14 15:32:05

[permalink] [raw]

Subject: Re: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver

On Thu, Mar 14, 2024 at 09:35:21PM +0800, Peng Fan (OSS) wrote:
> +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) {

If pmx->functions[selector].num_groups is set then we assume that
functions[selector].groups has been allocated.

> + *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);

However, pmx->functions[selector].num_groups is set here and not cleared
on the error paths. Or instead of clearing the .num_groups it would be
nice to pass a local variable and only do the
pmx->functions[selector].num_groups = local assignment right before the
success return.

regards,
dan carpenter

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

2024-03-14 15:37:17

[permalink] [raw]

Subject: Re: [PATCH v5 2/4] dt-bindings: firmware: arm,scmi: support pinctrl protocol

On Thu, Mar 14, 2024 at 09:35:19PM +0800, Peng Fan (OSS) wrote:
> + 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

describe[s]

> + set of pins is some desired function.

s/is/in/

> + 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.

This sub-node is using [the] default pinctrl bindings to configure
pin multiplexing and using SCMI protocol to apply [a] specified
configuration.

(Delete the duplicate "using SCMI protocol").

regards,
dan carpenter

2024-03-14 16:48:40

[permalink] [raw]

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

On Thu, Mar 14, 2024 at 09:35:17PM +0800, Peng Fan (OSS) wrote:
> Since SCMI 3.2 Spec is released, and this patchset has got R-b/T-b,
> is it ok to land this patchset?
>

I'll have a look at this last version and a spin on my test setup.

..but has this V5 change at all since the Reviewed-by tags due to the
latest spec changes ?

..IOW does this V5 include the latest small bits spec-changes or those
latest gpio-related spec-changes are just not needed at the level of the Linux
pinctrl support as of now and can be added later on when a Linux gpio
driver will be built on top of this ?

Thanks,
Cristian

2024-03-15 00:32:13

[permalink] [raw]

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

> Subject: Re: [PATCH v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Thu, Mar 14, 2024 at 09:35:17PM +0800, Peng Fan (OSS) wrote:
> > Since SCMI 3.2 Spec is released, and this patchset has got R-b/T-b, is
> > it ok to land this patchset?
> >
>
> I'll have a look at this last version and a spin on my test setup.
>
> ...but has this V5 change at all since the Reviewed-by tags due to the latest
> spec changes ?

The tags are same as V4. I only did a rebase, no more changes.
>
> ...IOW does this V5 include the latest small bits spec-changes or those latest
> gpio-related spec-changes are just not needed at the level of the Linux pinctrl
> support as of now and can be added later on when a Linux gpio driver will be
> built on top of this ?

In my current test, I no need the gpio related changes, so I would add that later
if you are ok.

Thanks,
Peng.

>
> Thanks,
> Cristian

2024-03-15 00:44:48

[permalink] [raw]

Subject: RE: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver

> Subject: Re: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-
> pinctrl driver
>
> On Thu, Mar 14, 2024 at 09:35:21PM +0800, Peng Fan (OSS) wrote:
> > +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) {
>
> If pmx->functions[selector].num_groups is set then we assume that
> functions[selector].groups has been allocated.
>
> > + *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);
>
> However, pmx->functions[selector].num_groups is set here and not cleared
> on the error paths. Or instead of clearing the .num_groups it would be nice
> to pass a local variable and only do the
> pmx->functions[selector].num_groups = local assignment right before the
> success return.

So you concern is I should clear the pmx->functions[selector].num_groups in
err path, right?

Thanks,
Peng.

>
> regards,
> dan carpenter
>
> > + 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;
> > +}

2024-03-15 06:28:29

[permalink] [raw]

Subject: Re: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver

On Fri, Mar 15, 2024 at 12:44:34AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-
> > pinctrl driver
> >
> > On Thu, Mar 14, 2024 at 09:35:21PM +0800, Peng Fan (OSS) wrote:
> > > +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) {
> >
> > If pmx->functions[selector].num_groups is set then we assume that
> > functions[selector].groups has been allocated.
> >
> > > + *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);
> >
> > However, pmx->functions[selector].num_groups is set here and not cleared
> > on the error paths. Or instead of clearing the .num_groups it would be nice
> > to pass a local variable and only do the
> > pmx->functions[selector].num_groups = local assignment right before the
> > success return.
>
> So you concern is I should clear the pmx->functions[selector].num_groups in
> err path, right?
>

Yes. If functions[selector].groups is invalid (NULL or freed) then the
pmx->functions[selector].num_groups variable must also be zero.

regards,
dan carpenter

2024-03-15 16:57:51

[permalink] [raw]

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

On Fri, Mar 15, 2024 at 12:31:51AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> > protocol basic support
> >
> > On Thu, Mar 14, 2024 at 09:35:17PM +0800, Peng Fan (OSS) wrote:
> > > Since SCMI 3.2 Spec is released, and this patchset has got R-b/T-b, is
> > > it ok to land this patchset?
> > >
> >
> > I'll have a look at this last version and a spin on my test setup.
> >
> > ...but has this V5 change at all since the Reviewed-by tags due to the latest
> > spec changes ?
>
> The tags are same as V4. I only did a rebase, no more changes.
> >

Ok.

> > ...IOW does this V5 include the latest small bits spec-changes or those latest
> > gpio-related spec-changes are just not needed at the level of the Linux pinctrl
> > support as of now and can be added later on when a Linux gpio driver will be
> > built on top of this ?
>
> In my current test, I no need the gpio related changes, so I would add that later
> if you are ok.
>

I COULD have agreed with this, since AFAIK there is currently an effort to
add support for GPIO on top of SCMI Pinctrl BUT not in Linux, so no reason to
block this series for gpio-related missing features, that should only be additions
not breaking backward compatibility...

...BUT, I've just wrapped my head again around the latest public release
of v3.2 spec (which has gone through so many changes and additions that
I had lost track O_o) AND beside the above mentioned GPIO changes there
are indeed also BREAKING changes around the commands PINCTRL_SETTINGS_GET and
PINCTRL_SETTINGS_CONFIGURE (which were the old PINCTRL_CONFIG_GET/SET),
that now also get/set the selected function: so that, at the end the payload
itself of those commands/replies has also changed IN SIZE, so the driver needs
definitely to be updated (and whatever you use to test on the backend server too,
if you want to test this...)

I think these changes (which I forgot being there) were in since last month, so
already V4 was broken in these regards (which I have not looked at)

I'll leave some comments along the series and test all of this again next week...
..since too many things has changed and I want to re-verify all on my side.

Thanks,
Cristian

2024-03-15 17:07:27

[permalink] [raw]

Subject: Re: [PATCH v5 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Thu, Mar 14, 2024 at 09:35:20PM +0800, Peng Fan (OSS) wrote:
> From: Oleksii Moisieiev <[email protected]>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
> Reviewed-by: Cristian Marussi <[email protected]>
> Tested-by: Cristian Marussi <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Co-developed-by: Peng Fan <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 908 ++++++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 75 +++
> 5 files changed, 987 insertions(+)
>
> 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 415e6f510057..ac2d4b19727c 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3142,6 +3142,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);
> }
> @@ -3159,6 +3160,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..0fcfa4269473
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,908 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#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 0x0
> +
> +#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,

These are now PINCTRL_SETTINGS_GET and PINCTRL_SETTINGS_CONFIGURE
with a bit of different payload and size....please also fix and rename
the related msg descriptos down below.

> + 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;

new field:

__le32 function_id;

> + __le32 attributes;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_conf_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_resp_conf_get {

new field

__le32 function_selected;

> + __le32 num_configs;

> + __le32 configs[];
> +};
> +

Thanks,
Cristian

2024-03-15 18:20:21

[permalink] [raw]

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

On Fri, Mar 15, 2024 at 04:53:10PM +0000, Cristian Marussi wrote:
> On Fri, Mar 15, 2024 at 12:31:51AM +0000, Peng Fan wrote:
> (and whatever you use to test on the backend server too, if you want
> to test this...)
>

What are people using to test this, btw?

regards,
dan carpenter

2024-03-17 19:08:48

[permalink] [raw]

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

On Fri, Mar 15, 2024 at 09:20:02PM +0300, Dan Carpenter wrote:
> On Fri, Mar 15, 2024 at 04:53:10PM +0000, Cristian Marussi wrote:
> > On Fri, Mar 15, 2024 at 12:31:51AM +0000, Peng Fan wrote:
> > (and whatever you use to test on the backend server too, if you want
> > to test this...)
> >
>
> What are people using to test this, btw?

Hi Dan,

I think NXP has their own SCMI server embedded in TF-A (Peng posted a
link at the public repo a while ago I think...) supporting Pinctrl;
additionally Oleksii/EPAM (the original author of this series) have their
own distinct proprietary SCMI server implementation with Pinctrl (not sure
where it run from in this case but it is a Xen based setup if I remember
right..).

Beside these, there are at least a couple more Vendor proprietary incarnations
of SCMI servers that I am aware of (that most probably did not support Pinctrl
anyway as of now...)

On top of this, on the other side there is, of course, the official SCP/SCMI
server reference implementation, that can live in a number of different places
thanks to the the virtualized environment built by Linaro, but this latter
SCP/SCMI server does not have any official Pinctrl support either as of now.

Thanks,
Cristian

2024-03-18 02:47:23

[permalink] [raw]

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

> Subject: Re: [PATCH v5 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Fri, Mar 15, 2024 at 12:31:51AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH v5 0/4] firmware: arm_scmi: Add SCMI v3.2
> > > pincontrol protocol basic support
> > >
> > > On Thu, Mar 14, 2024 at 09:35:17PM +0800, Peng Fan (OSS) wrote:
> > > > Since SCMI 3.2 Spec is released, and this patchset has got
> > > > R-b/T-b, is it ok to land this patchset?
> > > >
> > >
> > > I'll have a look at this last version and a spin on my test setup.
> > >
> > > ...but has this V5 change at all since the Reviewed-by tags due to
> > > the latest spec changes ?
> >
> > The tags are same as V4. I only did a rebase, no more changes.
> > >
>
> Ok.
>
> > > ...IOW does this V5 include the latest small bits spec-changes or
> > > those latest gpio-related spec-changes are just not needed at the
> > > level of the Linux pinctrl support as of now and can be added later
> > > on when a Linux gpio driver will be built on top of this ?
> >
> > In my current test, I no need the gpio related changes, so I would add
> > that later if you are ok.
> >
>
> I COULD have agreed with this, since AFAIK there is currently an effort to add
> support for GPIO on top of SCMI Pinctrl BUT not in Linux, so no reason to
> block this series for gpio-related missing features, that should only be
> additions not breaking backward compatibility...
>
> ....BUT, I've just wrapped my head again around the latest public release of
> v3.2 spec (which has gone through so many changes and additions that I had
> lost track O_o) AND beside the above mentioned GPIO changes there are
> indeed also BREAKING changes around the commands
> PINCTRL_SETTINGS_GET and PINCTRL_SETTINGS_CONFIGURE (which were
> the old PINCTRL_CONFIG_GET/SET), that now also get/set the selected
> function: so that, at the end the payload itself of those commands/replies has
> also changed IN SIZE, so the driver needs definitely to be updated (and
> whatever you use to test on the backend server too, if you want to test this...)

Ok, I see, there are indeed some changes, I will update the driver.

>
> I think these changes (which I forgot being there) were in since last month, so
> already V4 was broken in these regards (which I have not looked at)

I may need to drop the R-b/T-b?

>
> I'll leave some comments along the series and test all of this again next week...
> ...since too many things has changed and I want to re-verify all on my side.

Thanks,
Peng.
>
> Thanks,
> Cristian

2024-03-20 09:53:26

[permalink] [raw]

Subject: Re: [PATCH v5 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver

On Thu, Mar 14, 2024 at 09:35:21PM +0800, Peng Fan (OSS) wrote:
> +/* Define num configs, if not large than 4 use stack, else use kcalloc */
> +#define SCMI_NUM_CONFIGS 4
> +
> +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;

groups and nr_groups are set/allocated but not used anywhere. Delete
them.

> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;

These last two struct members duplicate the information in
pmx->pctl_desc.pins and pmx->pctl_desc.npins. They're not used outside
of the pinctrl_scmi_get_pins() function. Delete them.

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

-EINVAL would be a more appropriate error code than -ENOMEM. Nothing
is allocated here.

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

I re-wrote this function. I'm not totally sure it's better...

static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
unsigned int selector,
const char * const **p_groups,
unsigned int * const p_num_groups)
{
struct scmi_pinctrl_funcs *func;
const unsigned int *group_ids;
unsigned int num_groups;
const char **groups;
int ret, i;
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);

if (!p_groups || !p_num_groups)
return -EINVAL;

if (selector >= pmx->nr_functions)
return -EINVAL;

func = &pmx->functions[selector];
if (func->num_groups)
goto done;

ret = pinctrl_ops->function_groups_get(pmx->ph, selector, &num_groups,
&group_ids);
if (ret) {
dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
return ret;
}
if (!num_groups)
return -EINVAL;

groups = devm_kcalloc(pmx->dev, num_groups, sizeof(*groups), GFP_KERNEL);
if (!groups)
return -ENOMEM;

for (i = 0; i < num_groups; i++) {
groups[i] = pinctrl_scmi_get_group_name(pctldev, group_ids[i]);
if (!groups[i]) {
ret = -EINVAL;
goto err_free;
}
}

func->num_groups = num_groups;
func->groups = groups;
done:
*p_groups = (const char * const *)func->groups;
*p_num_groups = func->num_groups;

return 0;

err_free:
devm_kfree(pmx->dev, 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,
> + enum scmi_pinctrl_conf_type *type)
> +{
> + u32 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 = arg;
> + 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;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + 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_alloc_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> + u32 **p_config_value,
> + enum scmi_pinctrl_conf_type **p_config_type)
> +{
> + if (num_configs <= SCMI_NUM_CONFIGS)
> + return 0;
> +
> + *p_config_value = kcalloc(num_configs, sizeof(u32), GFP_KERNEL);
> + *p_config_type = kcalloc(num_configs,
> + sizeof(enum scmi_pinctrl_conf_type),
> + GFP_KERNEL);
> +
> + if (!*p_config_value || !*p_config_type) {
> + kfree(*p_config_value);
> + kfree(*p_config_type);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +pinctrl_scmi_free_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> + u32 **p_config_value,
> + enum scmi_pinctrl_conf_type **p_config_type)
> +{
> + if (num_configs <= SCMI_NUM_CONFIGS)
> + return;
> +
> + kfree(*p_config_value);
> + kfree(*p_config_type);
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> + u32 config_value[SCMI_NUM_CONFIGS];
> + enum scmi_pinctrl_conf_type *p_config_type = config_type;
> + u32 *p_config_value = config_value;
> + enum pin_config_param param;
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < num_configs; i++) {
> + param = pinconf_to_config_param(configs[i]);
> + ret = pinctrl_scmi_map_pinconf_type(param, &p_config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + goto free_config;
> + }
> + p_config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs,
> + p_config_type, p_config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d\n", ret);
> +
> +free_config:
> + pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> + u32 config_value[SCMI_NUM_CONFIGS];
> + enum scmi_pinctrl_conf_type *p_config_type = config_type;
> + u32 *p_config_value = config_value;
> + enum pin_config_param param;
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < num_configs; i++) {
> + param = pinconf_to_config_param(configs[i]);
> + ret = pinctrl_scmi_map_pinconf_type(param,
> + &p_config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + goto free_config;
> + }
> +
> + p_config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs,
> + p_config_type, p_config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d", ret);
> +
> +free_config:
> + pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> + &p_config_value);
> + 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;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + 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, 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) {

This condition is testing whether we can re-use the information from the
previous call. However, the function is only called once (from probe)
so there is no previous call and this condition can be removed.

> + *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;

If we were going to re-use the information then we'd need to set
pmx->nr_pins = 0; on this path. But we're not, so that's fine.

> +
> + 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;

No need to set ->nr_pins = 0;

> + return ret;
> + }
> + }
> +
> + *pins = pmx->pins;
> + dev_dbg(pmx->dev, "got pins %d", *nr_pins);
> +
> + return 0;
> +}

Ok, I put way too much thought into reviewing this function and I ended
up re-writing it slightly:

static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
struct pinctrl_desc *desc)
{
struct pinctrl_pin_desc *pins;
unsigned int npins;
int ret, i;

npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
if (!pins)
return -ENOMEM;

for (i = 0; i < npins; i++) {
pins[i].number = i;
ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
if (ret)
return dev_err_probe(pmx->dev, ret,
"Can't get name for pin %d", i);
}

desc->npins = npins;
desc->pins = pins;
dev_dbg(pmx->dev, "got pins %d", npins);

return 0;
}

> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
> +{
> + int ret;
> + struct device *dev = &sdev->dev;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev || !sdev->handle)
> + return -EINVAL;
> +
> + handle = sdev->handle;
> +
> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
> +
> + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = dev;
> + pmx->pctl_desc.name = DRV_NAME;
> + pmx->pctl_desc.owner = THIS_MODULE;
> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
> + &pmx->pctl_desc.pins);
> + if (ret)
> + return ret;
> +
> + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
> +
> + if (pmx->nr_functions) {

You don't need to have this condition. kcalloc() can allocate zero
element arrays.

> + pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
> + }
> +
> + if (pmx->nr_groups) {

Same.

> + pmx->groups = devm_kcalloc(dev, pmx->nr_groups,
> + sizeof(*pmx->groups), GFP_KERNEL);
> + if (!pmx->groups)
> + return -ENOMEM;
> + }
> +
> + return pinctrl_enable(pmx->pctldev);

There is a double free bug in pinctrl_enable() but it's not related to
your patch. I'll send a fix for that.

> +}

regards,
dan carpenter

2024-03-21 14:47:11

[permalink] [raw]

Subject: Re: [PATCH v5 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Thu, Mar 14, 2024 at 09:35:20PM +0800, Peng Fan (OSS) wrote:
> +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_FUNCTION_SELECT was removed from the spec so the other cmds were
renumbered. I'm still going through and reviewing this file. I'll
hopefully be done tomorrow.

> + PINCTRL_REQUEST = 0x8,
> + PINCTRL_RELEASE = 0x9,
> + PINCTRL_NAME_GET = 0xa,
> + PINCTRL_SET_PERMISSIONS = 0xb
> +};
> +

[ snip ]

> +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))
^^^^
Dereferencing "rx" after the ph->xops->xfer_put() is a use after free
(racy).

> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
> +}

[ snip ]

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

Missing error check.

if (ret)
return ret;

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

if (ret)
return ret;

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

[ snip ]

> +enum scmi_pinctrl_conf_type {
> + SCMI_PIN_NONE = 0x0,

This is SCMI_PIN_DEFAULT now.

> + 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,
^^^^
This is a decimal vs hex bug. It should 0x14. I think this enum would
be more readable in decimal anyway.

> + SCMI_PIN_OEM_START = 0xC0,
> + SCMI_PIN_OEM_END = 0xFF,
> +};

But I'm still trying to review the code so I'll probably respond more
tomorrow.

regards,
dan carpenter

2024-03-24 10:45:26

[permalink] [raw]

Subject: Re: [PATCH v5 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

On Thu, Mar 21, 2024 at 05:46:53PM +0300, Dan Carpenter wrote:
> On Thu, Mar 14, 2024 at 09:35:20PM +0800, Peng Fan (OSS) wrote:
> > +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_FUNCTION_SELECT was removed from the spec so the other cmds were
> renumbered. I'm still going through and reviewing this file. I'll
> hopefully be done tomorrow.
>

I think the rest is okay. It's just updating to the new version of the
spec. CONFIG_GET/SET need to be updated and FUNCTION_SELECT gets
deleted.

regards,
dan carpenter

2024-03-24 12:15:37