From: Peng Fan <[email protected]>
Add basic implementation of the SCMI v3.2 pincontrol protocol.
Co-developed-by: Oleksii Moisieiev <[email protected]>
Signed-off-by: Oleksii Moisieiev <[email protected]>
Signed-off-by: Peng Fan <[email protected]>
---
drivers/firmware/arm_scmi/Makefile | 3 +-
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 897 ++++++++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
include/linux/scmi_protocol.h | 83 ++++
5 files changed, 985 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..fd59f58ce8a2 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -10,7 +10,8 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-y := base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+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..503cd1b55561
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,897 @@
+// 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 0x10000
+
+#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))
+
+#define CONFIG_FLAG_MASK GENMASK(19, 18)
+#define SELECTOR_MASK GENMASK(17, 16)
+#define SKIP_CONFIGS_MASK GENMASK(15, 8)
+#define CONFIG_TYPE_MASK GENMASK(7, 0)
+
+enum scmi_pinctrl_protocol_cmd {
+ PINCTRL_ATTRIBUTES = 0x3,
+ PINCTRL_LIST_ASSOCIATIONS = 0x4,
+ PINCTRL_SETTINGS_GET = 0x5,
+ PINCTRL_SETTINGS_CONFIGURE = 0x6,
+ PINCTRL_REQUEST = 0x7,
+ PINCTRL_RELEASE = 0x8,
+ PINCTRL_NAME_GET = 0x9,
+ PINCTRL_SET_PERMISSIONS = 0xa,
+};
+
+struct scmi_msg_settings_conf {
+ __le32 identifier;
+ __le32 function_id;
+ __le32 attributes;
+ __le32 configs[];
+};
+
+struct scmi_msg_settings_get {
+ __le32 identifier;
+ __le32 attributes;
+};
+
+struct scmi_resp_settings_get {
+ __le32 function_selected;
+ __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_request {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_group_info {
+ char name[SCMI_MAX_STR_SIZE];
+ bool present;
+ u32 *group_pins;
+ u32 nr_pins;
+};
+
+struct scmi_function_info {
+ char name[SCMI_MAX_STR_SIZE];
+ bool present;
+ u32 *groups;
+ u32 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);
+ if (pi->nr_pins == 0) {
+ dev_warn(ph->dev, "returned zero pins\n");
+ ret = -EINVAL;
+ }
+ }
+
+ 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 selector,
+ enum scmi_pinctrl_selector_type type)
+{
+ int value;
+
+ value = scmi_pinctrl_count_get(ph, type);
+ if (value < 0)
+ return value;
+
+ if (selector >= value || value == 0)
+ 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,
+ u32 *n_elems)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_attributes *tx;
+ struct scmi_resp_pinctrl_attributes *rx;
+ bool ext_name_flag;
+
+ 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);
+
+ ext_name_flag = !!EXT_NAME_FLAG(rx->attributes);
+ }
+
+ ph->xops->xfer_put(ph, t);
+
+ if (ret)
+ return ret;
+ /*
+ * If supported overwrite short name with the extended one;
+ * on error just carry on and use already provided short name.
+ */
+ if (ext_name_flag)
+ ret = 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;
+ u32 *array;
+};
+
+static void iter_pinctrl_assoc_prepare_message(void *message,
+ u32 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);
+ 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, u32 *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_settings_get_ipriv {
+ u32 selector;
+ enum scmi_pinctrl_selector_type type;
+ bool get_all;
+ enum scmi_pinctrl_conf_type *config_types;
+ u32 *config_values;
+};
+
+static void
+iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
+ const void *priv)
+{
+ struct scmi_msg_settings_get *msg = message;
+ const struct scmi_settings_get_ipriv *p = priv;
+ u32 attributes;
+
+ attributes = FIELD_PREP(SELECTOR_MASK, p->type);
+
+ if (p->get_all) {
+ attributes |= FIELD_PREP(CONFIG_FLAG_MASK, 1) |
+ FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
+ } else {
+ attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
+ }
+
+ msg->attributes = cpu_to_le32(attributes);
+ msg->identifier = cpu_to_le32(p->selector);
+}
+
+static int
+iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
+ const void *response, void *priv)
+{
+ const struct scmi_resp_settings_get *r = response;
+ struct scmi_settings_get_ipriv *p = priv;
+
+ if (p->get_all) {
+ st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
+ st->num_remaining = le32_get_bits(r->num_configs, GENMASK(31, 24));
+ } else {
+ st->num_returned = 1;
+ st->num_remaining = 0;
+ }
+
+ return 0;
+}
+
+static int
+iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
+ const void *response,
+ struct scmi_iterator_state *st,
+ void *priv)
+{
+ const struct scmi_resp_settings_get *r = response;
+ struct scmi_settings_get_ipriv *p = priv;
+ u32 type = le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0));
+ u32 val = le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
+
+ if (p->get_all) {
+ p->config_types[st->desc_index + st->loop_idx] = type;
+ } else {
+ if (p->config_types[0] != type)
+ return -EINVAL;
+ }
+
+ p->config_values[st->desc_index + st->loop_idx] = val;
+
+ return 0;
+}
+
+static int
+scmi_pinctrl_settings_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, bool get_all)
+{
+ int ret;
+ void *iter;
+ struct scmi_iterator_ops ops = {
+ .prepare_message = iter_pinctrl_settings_get_prepare_message,
+ .update_state = iter_pinctrl_settings_get_update_state,
+ .process_response = iter_pinctrl_settings_get_process_response,
+ };
+ struct scmi_settings_get_ipriv ipriv = {
+ .selector = selector,
+ .type = type,
+ .get_all = get_all,
+ .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, SCMI_PIN_OEM_END,
+ PINCTRL_SETTINGS_GET,
+ sizeof(struct scmi_msg_settings_get),
+ &ipriv);
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
+
+ return ph->hops->iter_response_run(iter);
+}
+
+static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ enum scmi_pinctrl_conf_type config_type,
+ u32 *config_value)
+{
+ return scmi_pinctrl_settings_get(ph, selector, type, config_type,
+ config_value, false);
+}
+
+static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ enum scmi_pinctrl_conf_type config_type,
+ u32 *config_value)
+{
+ return scmi_pinctrl_settings_get(ph, selector, type, config_type,
+ config_value, true);
+}
+
+static int
+scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u32 nr_configs,
+ enum scmi_pinctrl_conf_type *config_type,
+ u32 *config_value)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_settings_conf *tx;
+ u32 attributes;
+ int ret, i;
+ u32 configs_in_chunk, conf_num = 0;
+ u32 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_SETTINGS_CONFIGURE,
+ sizeof(*tx) +
+ chunk * 2 * sizeof(__le32), 0, &t);
+ if (ret)
+ break;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ tx->function_id = cpu_to_le32(0xFFFFFFFF);
+ 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 group,
+ enum scmi_pinctrl_selector_type type,
+ u32 function_id)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_settings_conf *tx;
+ u32 attributes;
+
+ ret = scmi_pinctrl_validate_id(ph, group, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
+ sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(group);
+ tx->function_id = cpu_to_le32(function_id);
+ attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
+ tx->attributes = cpu_to_le32(attributes);
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_request_free(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type,
+ enum scmi_pinctrl_protocol_cmd cmd)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_request *tx;
+
+ if (type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ if (cmd != PINCTRL_REQUEST && cmd != PINCTRL_RELEASE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, cmd, 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;
+}
+
+static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
+ u32 pin)
+{
+ return scmi_pinctrl_request_free(ph, pin, PIN_TYPE, PINCTRL_REQUEST);
+}
+
+static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
+{
+ return scmi_pinctrl_request_free(ph, pin, PIN_TYPE, PINCTRL_RELEASE);
+}
+
+static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_group_info *group)
+{
+ int ret;
+
+ 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 || pi->nr_groups == 0)
+ 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 u32 **pins,
+ u32 *nr_pins)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!pins || !nr_pins)
+ return -EINVAL;
+
+ if (selector >= pi->nr_groups || pi->nr_groups == 0)
+ 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;
+
+ 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 || pi->nr_functions == 0)
+ 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, u32 *nr_groups,
+ const u32 **groups)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!groups || !nr_groups)
+ return -EINVAL;
+
+ if (selector >= pi->nr_functions || pi->nr_functions == 0)
+ 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,
+ .settings_get_one = scmi_pinctrl_settings_get_one,
+ .settings_get_all = scmi_pinctrl_settings_get_all,
+ .settings_conf = scmi_pinctrl_settings_conf,
+ .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..e59c48c1aed6 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -737,6 +737,88 @@ 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_DEFAULT = 0,
+ SCMI_PIN_BIAS_BUS_HOLD = 1,
+ SCMI_PIN_BIAS_DISABLE = 2,
+ SCMI_PIN_BIAS_HIGH_IMPEDANCE = 3,
+ SCMI_PIN_BIAS_PULL_UP = 4,
+ SCMI_PIN_BIAS_PULL_DEFAULT = 5,
+ SCMI_PIN_BIAS_PULL_DOWN = 6,
+ SCMI_PIN_DRIVE_OPEN_DRAIN = 7,
+ SCMI_PIN_DRIVE_OPEN_SOURCE = 8,
+ SCMI_PIN_DRIVE_PUSH_PULL = 9,
+ SCMI_PIN_DRIVE_STRENGTH = 10,
+ SCMI_PIN_INPUT_DEBOUNCE = 11,
+ SCMI_PIN_INPUT_MODE = 12,
+ SCMI_PIN_PULL_MODE = 13,
+ SCMI_PIN_INPUT_VALUE = 14,
+ SCMI_PIN_INPUT_SCHMITT = 15,
+ SCMI_PIN_LOW_POWER_MODE = 16,
+ SCMI_PIN_OUTPUT_MODE = 17,
+ SCMI_PIN_OUTPUT_VALUE = 18,
+ SCMI_PIN_POWER_SOURCE = 19,
+ SCMI_PIN_SLEW_RATE = 20,
+ SCMI_PIN_OEM_START = 192,
+ SCMI_PIN_OEM_END = 255,
+};
+
+/**
+ * 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
+ * @settings_get_one: returns one configuration parameter for pin or group
+ * specified by config_type
+ * @settings_get_all: returns all configuration parameters for pin or group
+ * @settings_conf: 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 (*settings_get_one)(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 (*settings_get_all)(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 (*settings_conf)(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 +926,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
On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
Hi,
> Co-developed-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
> ---
[snip]
> +struct scmi_settings_get_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + bool get_all;
> + enum scmi_pinctrl_conf_type *config_types;
> + u32 *config_values;
> +};
> +
> +static void
> +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_settings_get *msg = message;
> + const struct scmi_settings_get_ipriv *p = priv;
> + u32 attributes;
> +
> + attributes = FIELD_PREP(SELECTOR_MASK, p->type);
> +
> + if (p->get_all) {
> + attributes |= FIELD_PREP(CONFIG_FLAG_MASK, 1) |
> + FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + } else {
> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> + }
> +
> + msg->attributes = cpu_to_le32(attributes);
> + msg->identifier = cpu_to_le32(p->selector);
> +}
> +
> +static int
> +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (p->get_all) {
> + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> + st->num_remaining = le32_get_bits(r->num_configs, GENMASK(31, 24));
> + } else {
> + st->num_returned = 1;
> + st->num_remaining = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> + u32 type = le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0));
> + u32 val = le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> +
> + if (p->get_all) {
> + p->config_types[st->desc_index + st->loop_idx] = type;
> + } else {
> + if (p->config_types[0] != type)
> + return -EINVAL;
> + }
> +
> + p->config_values[st->desc_index + st->loop_idx] = val;
> +
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_settings_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, bool get_all)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> + .update_state = iter_pinctrl_settings_get_update_state,
> + .process_response = iter_pinctrl_settings_get_process_response,
> + };
> + struct scmi_settings_get_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .get_all = get_all,
> + .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, SCMI_PIN_OEM_END,
> + PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> + config_value, false);
> +}
> +
> +static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> + config_value, true);
> +}
> +
If you generalize the scmi_pinctrl_settings_get() and reintroduce a
settings_get_all() ops (even though unused by pinctrl driver, I am fine
with this..), you should take care to pass as an input parameter NOT only
the array of config_values BUT also an array of config_types since you could
get back up to 256 OEM types: for this reason you will need also to pass to
scmi_pinctrl_settings_get() an input param that specifies the sizes of the
2 array input params (in order to avoid oveflows) AND use that same inout
param also as an output param to report at the end how many OEM types were
effectively found and returned....
IOW, I did this on top of your V7 to make the settings_get_all work:
---8<---
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index b75af1dd75fa..f4937af66c4d 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -317,6 +317,7 @@ struct scmi_settings_get_ipriv {
u32 selector;
enum scmi_pinctrl_selector_type type;
bool get_all;
+ unsigned int *nr_configs;
enum scmi_pinctrl_conf_type *config_types;
u32 *config_values;
};
@@ -379,6 +380,7 @@ iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph
}
p->config_values[st->desc_index + st->loop_idx] = val;
+ ++*p->nr_configs;
return 0;
}
@@ -386,11 +388,13 @@ iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph
static int
scmi_pinctrl_settings_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, bool get_all)
+ unsigned int *nr_configs,
+ enum scmi_pinctrl_conf_type *config_types,
+ u32 *config_values)
{
int ret;
void *iter;
+ unsigned int max_configs = *nr_configs;
struct scmi_iterator_ops ops = {
.prepare_message = iter_pinctrl_settings_get_prepare_message,
.update_state = iter_pinctrl_settings_get_update_state,
@@ -399,19 +403,22 @@ scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
struct scmi_settings_get_ipriv ipriv = {
.selector = selector,
.type = type,
- .get_all = get_all,
- .config_types = &config_type,
- .config_values = config_value,
+ .get_all = (max_configs > 1),
+ .nr_configs = nr_configs,
+ .config_types = config_types,
+ .config_values = config_values,
};
- if (!config_value || type == FUNCTION_TYPE)
+ if (!config_types || !config_values || 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, SCMI_PIN_OEM_END,
+ /* Prepare to count returned configs */
+ *nr_configs = 0;
+ iter = ph->hops->iter_response_init(ph, &ops, max_configs,
PINCTRL_SETTINGS_GET,
sizeof(struct scmi_msg_settings_get),
&ipriv);
@@ -427,18 +434,24 @@ static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_handle *ph,
enum scmi_pinctrl_conf_type config_type,
u32 *config_value)
{
- return scmi_pinctrl_settings_get(ph, selector, type, config_type,
- config_value, false);
+ unsigned int nr_configs = 1;
+
+ return scmi_pinctrl_settings_get(ph, selector, type, &nr_configs,
+ &config_type, config_value);
}
static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle *ph,
u32 selector,
enum scmi_pinctrl_selector_type type,
- enum scmi_pinctrl_conf_type config_type,
- u32 *config_value)
+ unsigned int *nr_configs,
+ enum scmi_pinctrl_conf_type *config_types,
+ u32 *config_values)
{
- return scmi_pinctrl_settings_get(ph, selector, type, config_type,
- config_value, true);
+ if (!nr_configs || *nr_configs == 0)
+ return -EINVAL;
+
+ return scmi_pinctrl_settings_get(ph, selector, type, nr_configs,
+ config_types, config_values);
}
static int
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index abaf6122ea37..7915792efd81 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -882,8 +882,9 @@ struct scmi_pinctrl_proto_ops {
int (*settings_get_all)(const struct scmi_protocol_handle *ph,
u32 selector,
enum scmi_pinctrl_selector_type type,
- enum scmi_pinctrl_conf_type config_type,
- u32 *config_value);
+ unsigned int *nr_configs,
+ enum scmi_pinctrl_conf_type *config_types,
+ u32 *config_values);
int (*settings_conf)(const struct scmi_protocol_handle *ph,
u32 selector, enum scmi_pinctrl_selector_type type,
unsigned int nr_configs,
--->8-----
Please check if this addition sounds good to you and integrate into v8
eventually...
Thanks,
Cristian
Hi Andy
> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> Please, follow IWYU principle, a lot of headers are missed.
ok. I will try to figure out. BTW, is there an easy way to filter
out what is missed?
>
> > +#include "common.h"
> > +#include "protocols.h"
>
> ...
>
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
>
> It's netter as a single line.
I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian
is ok, I could use a single line.
>
> > + if (ret)
> > + return ret;
> > + }
>
> ...
>
> > +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);
>
> Same comments as per previous version. devm_ here is simply wrong.
> It breaks the order of freeing resources.
>
> I.o.w. I see *no guarantee* that these init-deinit functions will be properly
> called from the respective probe-remove. Moreover the latter one may also
> have its own devm allocations (which are rightfully placed) and you get
> completely out of control the resource management.
I see an old thread.
https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
The free in deinit is not to free the ones alloced in init, it is to free the ones
alloced such as in scmi_pinctrl_get_function_info
Thanks,
Peng.
>
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
..
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
Please, follow IWYU principle, a lot of headers are missed.
> +#include "common.h"
> +#include "protocols.h"
..
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
It's netter as a single line.
> + if (ret)
> + return ret;
> + }
..
> +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);
Same comments as per previous version. devm_ here is simply wrong.
It breaks the order of freeing resources.
I.o.w. I see *no guarantee* that these init-deinit functions will be properly
called from the respective probe-remove. Moreover the latter one may also have
its own devm allocations (which are rightfully placed) and you get completely
out of control the resource management.
> +}
--
With Best Regards,
Andy Shevchenko
> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
>
> Hi,
>
>
> > Co-developed-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
>
> [snip]
>
>
> > +struct scmi_settings_get_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + bool get_all;
> > + enum scmi_pinctrl_conf_type *config_types;
> > + u32 *config_values;
> > +};
> > +
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->get_all) {
> > + attributes |= FIELD_PREP(CONFIG_FLAG_MASK, 1) |
> > + FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + } else {
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > + }
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->get_all) {
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > + u32 type = le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7,
> 0));
> > + u32 val = le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + if (p->get_all) {
> > + p->config_types[st->desc_index + st->loop_idx] = type;
> > + } else {
> > + if (p->config_types[0] != type)
> > + return -EINVAL;
> > + }
> > +
> > + p->config_values[st->desc_index + st->loop_idx] = val;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_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, bool get_all) {
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .get_all = get_all,
> > + .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, SCMI_PIN_OEM_END,
> > + PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_one(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + enum scmi_pinctrl_conf_type
> config_type,
> > + u32 *config_value)
> > +{
> > + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > + config_value, false);
> > +}
> > +
> > +static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + enum scmi_pinctrl_conf_type
> config_type,
> > + u32 *config_value)
> > +{
> > + return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> > + config_value, true);
> > +}
> > +
>
> If you generalize the scmi_pinctrl_settings_get() and reintroduce a
> .settings_get_all() ops (even though unused by pinctrl driver, I am fine with
> this..), you should take care to pass as an input parameter NOT only the array
> of config_values BUT also an array of config_types since you could get back
> up to 256 OEM types: for this reason you will need also to pass to
> scmi_pinctrl_settings_get() an input param that specifies the sizes of the
> 2 array input params (in order to avoid oveflows) AND use that same inout
> param also as an output param to report at the end how many OEM types
> were effectively found and returned....
>
> IOW, I did this on top of your V7 to make the settings_get_all work:
>
> ---8<---
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c
> b/drivers/firmware/arm_scmi/pinctrl.c
> index b75af1dd75fa..f4937af66c4d 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -317,6 +317,7 @@ struct scmi_settings_get_ipriv {
> u32 selector;
> enum scmi_pinctrl_selector_type type;
> bool get_all;
> + unsigned int *nr_configs;
> enum scmi_pinctrl_conf_type *config_types;
> u32 *config_values;
> };
> @@ -379,6 +380,7 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph
> }
>
> p->config_values[st->desc_index + st->loop_idx] = val;
> + ++*p->nr_configs;
>
> return 0;
> }
> @@ -386,11 +388,13 @@ iter_pinctrl_settings_get_process_response(const
> struct scmi_protocol_handle *ph static int scmi_pinctrl_settings_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, bool get_all)
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type *config_types,
> + u32 *config_values)
> {
> int ret;
> void *iter;
> + unsigned int max_configs = *nr_configs;
> struct scmi_iterator_ops ops = {
> .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> .update_state = iter_pinctrl_settings_get_update_state,
> @@ -399,19 +403,22 @@ scmi_pinctrl_settings_get(const struct
> scmi_protocol_handle *ph, u32 selector,
> struct scmi_settings_get_ipriv ipriv = {
> .selector = selector,
> .type = type,
> - .get_all = get_all,
> - .config_types = &config_type,
> - .config_values = config_value,
> + .get_all = (max_configs > 1),
> + .nr_configs = nr_configs,
> + .config_types = config_types,
> + .config_values = config_values,
> };
>
> - if (!config_value || type == FUNCTION_TYPE)
> + if (!config_types || !config_values || 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, SCMI_PIN_OEM_END,
> + /* Prepare to count returned configs */
> + *nr_configs = 0;
> + iter = ph->hops->iter_response_init(ph, &ops, max_configs,
> PINCTRL_SETTINGS_GET,
> sizeof(struct
> scmi_msg_settings_get),
> &ipriv);
> @@ -427,18 +434,24 @@ static int scmi_pinctrl_settings_get_one(const
> struct scmi_protocol_handle *ph,
> enum scmi_pinctrl_conf_type
> config_type,
> u32 *config_value)
> {
> - return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> - config_value, false);
> + unsigned int nr_configs = 1;
> +
> + return scmi_pinctrl_settings_get(ph, selector, type, &nr_configs,
> + &config_type, config_value);
> }
>
> static int scmi_pinctrl_settings_get_all(const struct scmi_protocol_handle
> *ph,
> u32 selector,
> enum scmi_pinctrl_selector_type
> type,
> - enum scmi_pinctrl_conf_type
> config_type,
> - u32 *config_value)
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type
> *config_types,
> + u32 *config_values)
> {
> - return scmi_pinctrl_settings_get(ph, selector, type, config_type,
> - config_value, true);
> + if (!nr_configs || *nr_configs == 0)
> + return -EINVAL;
> +
> + return scmi_pinctrl_settings_get(ph, selector, type, nr_configs,
> + config_types, config_values);
> }
>
> static int
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index abaf6122ea37..7915792efd81 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -882,8 +882,9 @@ struct scmi_pinctrl_proto_ops {
> int (*settings_get_all)(const struct scmi_protocol_handle *ph,
> u32 selector,
> enum scmi_pinctrl_selector_type type,
> - enum scmi_pinctrl_conf_type config_type,
> - u32 *config_value);
> + unsigned int *nr_configs,
> + enum scmi_pinctrl_conf_type *config_types,
> + u32 *config_values);
> int (*settings_conf)(const struct scmi_protocol_handle *ph,
> u32 selector, enum scmi_pinctrl_selector_type
> type,
> unsigned int nr_configs,
> --->8-----
>
> Please check if this addition sounds good to you and integrate into v8
> eventually...
Thanks for helping on this, I will included your changes, and your
Co-developed-by tag if you not mind.
Thanks,
Peng.
>
> Thanks,
> Cristian
On Tue, Apr 02, 2024 at 01:27:19PM +0000, Peng Fan wrote:
> > On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
..
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> >
> > Please, follow IWYU principle, a lot of headers are missed.
>
> ok. I will try to figure out. BTW, is there an easy way to filter
> out what is missed?
For you is much easier than to me as I'm not familiar with the code.
Basically you should know what you wrote :-)
But if you are asking about tooling, we would appreciate when somebody comes
with a such.
> > > +#include "common.h"
> > > +#include "protocols.h"
..
> > > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > > + &pi->pins[selector]);
> >
> > It's netter as a single line.
>
> I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian
> is ok, I could use a single line.
It's minor, but even before relaxation of 80 limit it was and is mentioned
in the documentation that you may go over if it increases readability.
> > > + if (ret)
> > > + return ret;
> > > + }
..
> > > +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);
> >
> > Same comments as per previous version. devm_ here is simply wrong.
> > It breaks the order of freeing resources.
> >
> > I.o.w. I see *no guarantee* that these init-deinit functions will be properly
> > called from the respective probe-remove. Moreover the latter one may also
> > have its own devm allocations (which are rightfully placed) and you get
> > completely out of control the resource management.
>
> I see an old thread.
> https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
>
> The free in deinit is not to free the ones alloced in init, it is to free the ones
> alloced such as in scmi_pinctrl_get_function_info
Even messier than I thought. For bare minimum these two should be documented
and renamed accordingly that no-one will think that deinit is a counter part
of init.
--
With Best Regards,
Andy Shevchenko