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

[permalink] [raw]

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

scmi: Introduce pinctrl SCMI protocol driver

Add basic implementation of the SCMI v3.2 pincontrol protocol
excluding GPIO support. All pinctrl related callbacks and operations
are exposed in the include/linux/scmi_protocol.h

Signed-off-by: Oleksii Moisieiev <[email protected]>
---
MAINTAINERS | 6 +
drivers/firmware/arm_scmi/Makefile | 2 +-
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
include/linux/scmi_protocol.h | 47 ++
6 files changed, 893 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/arm_scmi/pinctrl.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0dab9737ec16..297b2512963d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h
F: include/trace/events/scmi.h
F: include/uapi/linux/virtio_scmi.h

+PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
+M: Oleksii Moisieiev <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/firmware/arm_scmi/pinctrl.c
+
SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel <[email protected]>
L: [email protected]
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index b31d78fa66cc..603430ec0bfe 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)

obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 5be931a07c84..a9fd337b9596 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3025,6 +3025,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);
}
@@ -3042,6 +3043,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..fc0fcc26dfb6
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,836 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Pinctrl Protocol
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include "protocols.h"
+
+#define 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 config_value;
+};
+
+struct scmi_msg_conf_get {
+ __le32 identifier;
+ __le32 attributes;
+};
+
+struct scmi_msg_pinctrl_protocol_attributes {
+ __le32 attributes_low;
+ __le32 attributes_high;
+};
+
+struct scmi_msg_pinctrl_attributes {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_resp_pinctrl_attributes {
+ __le32 attributes;
+ u8 name[SCMI_SHORT_NAME_MAX_SIZE];
+};
+
+struct scmi_msg_pinctrl_list_assoc {
+ __le32 identifier;
+ __le32 flags;
+ __le32 index;
+};
+
+struct scmi_resp_pinctrl_list_assoc {
+ __le32 flags;
+ __le16 array[];
+};
+
+struct scmi_msg_func_set {
+ __le32 identifier;
+ __le32 function_id;
+ __le32 flags;
+};
+
+struct scmi_msg_request {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_group_info {
+ bool present;
+ char name[SCMI_MAX_STR_SIZE];
+ unsigned int *group_pins;
+ unsigned int nr_pins;
+};
+
+struct scmi_function_info {
+ bool present;
+ char name[SCMI_MAX_STR_SIZE];
+ unsigned int *groups;
+ unsigned int nr_groups;
+};
+
+struct scmi_pin_info {
+ bool present;
+ char name[SCMI_MAX_STR_SIZE];
+};
+
+struct scmi_pinctrl_info {
+ u32 version;
+ int nr_groups;
+ int nr_functions;
+ int nr_pins;
+ struct scmi_group_info *groups;
+ struct scmi_function_info *functions;
+ struct scmi_pin_info *pins;
+};
+
+static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
+ struct scmi_pinctrl_info *pi)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_protocol_attributes *attr;
+
+ if (!pi)
+ return -EINVAL;
+
+ ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
+ 0, sizeof(*attr), &t);
+ if (ret)
+ return ret;
+
+ attr = t->rx.buf;
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ pi->nr_functions = 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_get_count(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type)
+{
+ struct scmi_pinctrl_info *pi;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -ENODEV;
+
+ switch (type) {
+ case PIN_TYPE:
+ return pi->nr_pins;
+ case GROUP_TYPE:
+ return pi->nr_groups;
+ case FUNCTION_TYPE:
+ return pi->nr_functions;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ int value;
+
+ value = scmi_pinctrl_get_count(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);
+}
+
+static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, unsigned long *config_value)
+{
+ int ret;
+ u32 attributes;
+ struct scmi_xfer *t;
+ struct scmi_msg_conf_get *tx;
+
+ if (!config_value || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx),
+ sizeof(__le32), &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ attributes = FIELD_PREP(REG_TYPE_BITS, type) |
+ FIELD_PREP(REG_CONFIG, config_type);
+ tx->attributes = cpu_to_le32(attributes);
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret)
+ *config_value = get_unaligned_le32(t->rx.buf);
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_set_config(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, unsigned long config_value)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_conf_set *tx;
+ u32 attributes = 0;
+ int ret;
+
+ if (type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_SET,
+ sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ attributes = FIELD_PREP(REG_TYPE_BITS, type) |
+ FIELD_PREP(REG_CONFIG, config_type);
+ tx->attributes = cpu_to_le32(attributes);
+ tx->config_value = cpu_to_le32(config_value);
+
+ ret = ph->xops->do_xfer(ph, t);
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type,
+ u32 function_id)
+{
+ 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_request_pin(const struct scmi_protocol_handle *ph,
+ u32 pin)
+{
+ return scmi_pinctrl_request(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ 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_free_pin(const struct scmi_protocol_handle *ph, u32 pin)
+{
+ return scmi_pinctrl_free(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_group_info *group)
+{
+ int ret;
+
+ 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 = devm_kmalloc_array(ph->dev, 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) {
+ devm_kfree(ph->dev, 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;
+
+ if (!name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ 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_get_group_pins(const struct scmi_protocol_handle *ph,
+ u32 selector, const unsigned int **pins,
+ unsigned int *nr_pins)
+{
+ struct scmi_pinctrl_info *pi;
+
+ if (!pins || !nr_pins)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_groups)
+ return -EINVAL;
+
+ if (!pi->groups[selector].present) {
+ 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 = devm_kmalloc_array(ph->dev, 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) {
+ devm_kfree(ph->dev, 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;
+
+ if (!name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ 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_get_function_groups(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ unsigned int *nr_groups,
+ const unsigned int **groups)
+{
+ struct scmi_pinctrl_info *pi;
+
+ if (!groups || !nr_groups)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_functions)
+ return -EINVAL;
+
+ if (!pi->functions[selector].present) {
+ 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_set_mux(const struct scmi_protocol_handle *ph,
+ u32 selector, u32 group)
+{
+ return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
+ selector);
+}
+
+static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
+ u32 selector, struct scmi_pin_info *pin)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!pin)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ 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;
+
+ if (!name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ 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_get_name(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name)
+{
+ switch (type) {
+ case PIN_TYPE:
+ return scmi_pinctrl_get_pin_name(ph, selector, name);
+ case GROUP_TYPE:
+ return scmi_pinctrl_get_group_name(ph, selector, name);
+ case FUNCTION_TYPE:
+ return scmi_pinctrl_get_function_name(ph, selector, name);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
+ .get_count = scmi_pinctrl_get_count,
+ .get_name = scmi_pinctrl_get_name,
+ .get_group_pins = scmi_pinctrl_get_group_pins,
+ .get_function_groups = scmi_pinctrl_get_function_groups,
+ .set_mux = scmi_pinctrl_set_mux,
+ .get_config = scmi_pinctrl_get_config,
+ .set_config = scmi_pinctrl_set_config,
+ .request_pin = scmi_pinctrl_request_pin,
+ .free_pin = scmi_pinctrl_free_pin
+};
+
+static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
+{
+ int ret;
+ u32 version;
+ struct scmi_pinctrl_info *pinfo;
+
+ ret = ph->xops->version_get(ph, &version);
+ if (ret)
+ return ret;
+
+ dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
+ PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+ pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
+ if (!pinfo)
+ return -ENOMEM;
+
+ ret = scmi_pinctrl_attributes_get(ph, pinfo);
+ if (ret)
+ return ret;
+
+ pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
+ sizeof(*pinfo->pins),
+ GFP_KERNEL);
+ if (!pinfo->pins)
+ return -ENOMEM;
+
+ pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
+ sizeof(*pinfo->groups),
+ GFP_KERNEL);
+ if (!pinfo->groups)
+ return -ENOMEM;
+
+ pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
+ sizeof(*pinfo->functions),
+ GFP_KERNEL);
+ if (!pinfo->functions)
+ return -ENOMEM;
+
+ pinfo->version = version;
+
+ return ph->set_priv(ph, pinfo);
+}
+
+static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
+{
+ int i;
+ struct scmi_pinctrl_info *pi;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ for (i = 0; i < pi->nr_groups; i++)
+ if (pi->groups[i].present) {
+ devm_kfree(ph->dev, pi->groups[i].group_pins);
+ pi->groups[i].present = false;
+ }
+
+ for (i = 0; i < pi->nr_functions; i++)
+ if (pi->functions[i].present) {
+ devm_kfree(ph->dev, pi->functions[i].groups);
+ pi->functions[i].present = false;
+ }
+
+ return 0;
+}
+
+static const struct scmi_protocol scmi_pinctrl = {
+ .id = SCMI_PROTOCOL_PINCTRL,
+ .owner = THIS_MODULE,
+ .instance_init = &scmi_pinctrl_protocol_init,
+ .instance_deinit = &scmi_pinctrl_protocol_deinit,
+ .ops = &pinctrl_proto_ops,
+};
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index b3c6314bb4b8..674f949354f9 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -346,5 +346,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
DECLARE_SCMI_REGISTER_UNREGISTER(system);
DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
+DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);

#endif /* _SCMI_PROTOCOLS_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0ce5746a4470..97631783a5a4 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -735,6 +735,52 @@ struct scmi_notify_ops {
struct notifier_block *nb);
};

+enum scmi_pinctrl_selector_type {
+ PIN_TYPE = 0,
+ GROUP_TYPE,
+ FUNCTION_TYPE
+};
+
+/**
+ * struct scmi_pinctrl_proto_ops - represents the various operations provided
+ * by SCMI Pinctrl Protocol
+ *
+ * @get_count: returns count of the registered elements in given type
+ * @get_name: returns name by index of given type
+ * @get_group_pins: returns the set of pins, assigned to the specified group
+ * @get_function_groups: returns the set of groups, assigned to the specified
+ * function
+ * @set_mux: set muxing function for groups of pins
+ * @get_config: returns configuration parameter for pin or group
+ * @set_config: sets the configuration parameter for pin or group
+ * @request_pin: aquire pin before selecting mux setting
+ * @free_pin: frees pin, acquired by request_pin call
+ */
+struct scmi_pinctrl_proto_ops {
+ int (*get_count)(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type);
+ int (*get_name)(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name);
+ int (*get_group_pins)(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ const unsigned int **pins, unsigned int *nr_pins);
+ int (*get_function_groups)(const struct scmi_protocol_handle *ph,
+ u32 selector, unsigned int *nr_groups,
+ const unsigned int **groups);
+ int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
+ u32 group);
+ int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, unsigned long *config_value);
+ int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, unsigned long config_value);
+ int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
+ int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
+};
+
/**
* struct scmi_handle - Handle returned to ARM SCMI clients for usage.
*
@@ -783,6 +829,7 @@ enum scmi_std_protocol {
SCMI_PROTOCOL_RESET = 0x16,
SCMI_PROTOCOL_VOLTAGE = 0x17,
SCMI_PROTOCOL_POWERCAP = 0x18,
+ SCMI_PROTOCOL_PINCTRL = 0x19,
};

enum scmi_system_events {
--
2.25.1

2023-06-06 16:47:10

[permalink] [raw]

Subject: [PATCH v3 3/4] pinctrl: Implementation of the generic scmi-pinctrl driver

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

This setup expects SCP firmware (or similar system, such as ATF)
to be installed on the platform, which implements pinctrl driver
for the specific platform.

SCMI-Pinctrl driver should be configured from the device-tree and uses
generic device-tree mappings for the configuration.

Signed-off-by: Oleksii Moisieiev <[email protected]>
---
MAINTAINERS | 1 +
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-scmi.c | 554 +++++++++++++++++++++++++++++++++
4 files changed, 567 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-scmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 297b2512963d..91883955fc1a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20527,6 +20527,7 @@ M: Oleksii Moisieiev <[email protected]>
L: [email protected]
S: Maintained
F: drivers/firmware/arm_scmi/pinctrl.c
+F: drivers/pinctrl/pinctrl-scmi.c

SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel <[email protected]>
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 5787c579dcf6..c4680a2c5e13 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
source "drivers/pinctrl/visconti/Kconfig"
source "drivers/pinctrl/vt8500/Kconfig"

+config PINCTRL_SCMI
+ tristate "Pinctrl driver controlled via SCMI 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.
+
endif
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e196c6e324ad..b932a116e6a0 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o
obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
+obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o

obj-y += actions/
obj-$(CONFIG_ARCH_ASPEED) += aspeed/
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
new file mode 100644
index 000000000000..e46dffa652c6
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,554 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/seq_file.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinconf.h"
+
+#define DRV_NAME "scmi-pinctrl"
+
+static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
+
+struct scmi_pinctrl_funcs {
+ unsigned int num_groups;
+ const char **groups;
+};
+
+struct scmi_pinctrl {
+ struct device *dev;
+ struct scmi_protocol_handle *ph;
+ struct pinctrl_dev *pctldev;
+ struct pinctrl_desc pctl_desc;
+ struct scmi_pinctrl_funcs *functions;
+ unsigned int nr_functions;
+ char **groups;
+ unsigned int nr_groups;
+ struct pinctrl_pin_desc *pins;
+ unsigned int nr_pins;
+};
+
+static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_count(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;
+
+ if (!pctldev)
+ return NULL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return NULL;
+
+ ret = pinctrl_ops->get_name(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;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_group_pins(pmx->ph, selector,
+ pins, num_pins);
+}
+
+#ifdef CONFIG_OF
+static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np_config,
+ struct pinctrl_map **map,
+ u32 *num_maps)
+{
+ return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
+ num_maps, PIN_MAP_TYPE_INVALID);
+}
+
+static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
+ struct pinctrl_map *map, u32 num_maps)
+{
+ kfree(map);
+}
+
+#endif /* CONFIG_OF */
+
+static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
+ .get_groups_count = pinctrl_scmi_get_groups_count,
+ .get_group_name = pinctrl_scmi_get_group_name,
+ .get_group_pins = pinctrl_scmi_get_group_pins,
+#ifdef CONFIG_OF
+ .dt_node_to_map = pinctrl_scmi_dt_node_to_map,
+ .dt_free_map = pinctrl_scmi_dt_free_map,
+#endif
+};
+
+static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_count(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;
+
+ if (!pctldev)
+ return NULL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return NULL;
+
+ ret = pinctrl_ops->get_name(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;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !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->get_function_groups(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 error;
+ }
+ }
+
+ *groups = (const char * const *)pmx->functions[selector].groups;
+
+ return 0;
+
+error:
+ 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;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->set_mux(pmx->ph, selector, group);
+}
+
+static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
+ unsigned int offset)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->request_pin(pmx->ph, offset);
+}
+
+static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->free_pin(pmx->ph, offset);
+}
+
+static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
+ .request = pinctrl_scmi_request,
+ .free = pinctrl_scmi_free,
+ .get_functions_count = pinctrl_scmi_get_functions_count,
+ .get_function_name = pinctrl_scmi_get_function_name,
+ .get_function_groups = pinctrl_scmi_get_function_groups,
+ .set_mux = pinctrl_scmi_func_set_mux,
+};
+
+static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
+ &config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !configs || num_configs == 0)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ config_type = pinconf_to_config_param(configs[i]);
+ config_value = pinconf_to_config_argument(configs[i]);
+
+ ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
+ config_value);
+ if (ret) {
+ dev_err(pmx->dev, "Error parsing config %ld\n",
+ configs[i]);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
+ unsigned int group,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !configs || num_configs == 0)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ config_type = pinconf_to_config_param(configs[i]);
+ config_value = pinconf_to_config_argument(configs[i]);
+
+ ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
+ config_type, config_value);
+ if (ret) {
+ dev_err(pmx->dev, "Error parsing config = %ld",
+ configs[i]);
+ break;
+ }
+ }
+
+ return ret;
+};
+
+static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
+ config_type, &config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
+ .is_generic = true,
+ .pin_config_get = pinctrl_scmi_pinconf_get,
+ .pin_config_set = pinctrl_scmi_pinconf_set,
+ .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
+ .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
+ .pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
+ unsigned int *nr_pins,
+ const struct pinctrl_pin_desc **pins)
+{
+ int ret, i;
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ if (!pins || !nr_pins)
+ return -EINVAL;
+
+ if (pmx->nr_pins) {
+ *pins = pmx->pins;
+ *nr_pins = pmx->nr_pins;
+ return 0;
+ }
+
+ *nr_pins = pinctrl_ops->get_count(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->get_name(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);
+ goto err_free;
+ }
+ }
+
+ *pins = pmx->pins;
+ dev_dbg(pmx->dev, "got pins %d", *nr_pins);
+
+ return 0;
+ err_free:
+ devm_kfree(pmx->dev, pmx->pins);
+ pmx->nr_pins = 0;
+
+ return ret;
+}
+
+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 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(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
+ if (!pmx)
+ return -ENOMEM;
+
+ pmx->ph = ph;
+
+ pmx->dev = &sdev->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(&sdev->dev, &pmx->pctl_desc, pmx,
+ &pmx->pctldev);
+ if (ret) {
+ dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
+ return ret;
+ }
+
+ 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(&sdev->dev, pmx->nr_functions,
+ sizeof(*pmx->functions),
+ GFP_KERNEL);
+ if (!pmx->functions)
+ return -ENOMEM;
+ }
+
+ if (pmx->nr_groups) {
+ pmx->groups =
+ devm_kcalloc(&sdev->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_DESCRIPTION("ARM SCMI pin controller driver");
+MODULE_LICENSE("GPL");
--
2.25.1

2023-06-06 16:48:12

[permalink] [raw]

Subject: [PATCH v3 4/4] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

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

Signed-off-by: Oleksii Moisieiev <[email protected]>
---
.../bindings/firmware/arm,scmi.yaml | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..a19aa184bbd1 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -233,6 +233,39 @@ properties:
reg:
const: 0x18

+ protocol@19:
+ $ref: '#/$defs/protocol-node'
+
+ properties:
+ reg:
+ const: 0x19
+
+ '#pinctrl-cells':
+ const: 0
+
+ allOf:
+ - $ref: /schemas/pinctrl/pinctrl.yaml#
+
+ required:
+ - reg
+
+ additionalProperties:
+ anyOf:
+ - type: object
+ allOf:
+ - $ref: /schemas/pinctrl/pincfg-node.yaml#
+ - $ref: /schemas/pinctrl/pinmux-node.yaml#
+
+ 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.
+
+ unevaluatedProperties: false
+
additionalProperties: false

$defs:
@@ -384,6 +417,26 @@ examples:
scmi_powercap: protocol@18 {
reg = <0x18>;
};
+
+ scmi_pinctrl: protocol@19 {
+ reg = <0x19>;
+ #pinctrl-cells = <0>;
+
+ i2c2 {
+ groups = "i2c2_a", "i2c2_b";
+ function = "i2c2";
+ };
+
+ pins_mdio {
+ groups = "avb_mdio";
+ drive-strength = <24>;
+ };
+
+ keys_pins: keys {
+ pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
+ bias-pull-up;
+ };
+ };
};
};

--
2.25.1

2023-06-07 07:23:05

[permalink] [raw]

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

Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> scmi: Introduce pinctrl SCMI protocol driver

Seems like you forgot to remove previous line(s) in the commit message and
the above has to be the Subject here.

> Add basic implementation of the SCMI v3.2 pincontrol protocol
> excluding GPIO support. All pinctrl related callbacks and operations
> are exposed in the include/linux/scmi_protocol.h

drop include/ part, everybody will understand that. Also mind the grammar
period.

...

> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o

Why not splitting it and make it ordered?

...

Missing headers:

bitfield.h
bits.h
byteorder/
types.h

> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>

Missing

asm/unaligned.h

...

> +struct scmi_group_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *group_pins;
> + unsigned int nr_pins;
> +};

So, why struct pingroup can't be embeded here?

> +struct scmi_function_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *groups;
> + unsigned int nr_groups;
> +};

So, why and struct pinfunction can't be embedded here (yes, you would need a
duplication of groups as here they are integers)?

As far as I understand these data structures are not part of any ABI (otherwise
the wrong type(s) / padding might be in use) and hence don't see the impediments
to use them, but would be nice to have a comment on top of each.

...

> +struct scmi_pin_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];

Swapping order might help compiler to generate a better code.
Also this applies to the _group_info and _function_info.

> +};

...

> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {

Can you rather follow the usual pattern, i.e. checking for the errors?

if (ret)
goto out_put_xfer;

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

out_put_xfer:

> + ph->xops->xfer_put(ph, t);
> + return ret;

...

> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {

Ditto.

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

if (ret)
return ret;

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

return 0;

> +}

...

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

if (ret)
goto out_put_xfer;

(but in this and similar, aka one line, cases the current wouldn't be bad, just
matter of the consistency with the rest of the code)

> + *config_value = get_unaligned_le32(t->rx.buf);
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;

...

> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> + sizeof(*tx), 0, &t);

This is perfectly one line. Please also check entire code for such an unneeded
wrap.

...

> +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 = devm_kmalloc_array(ph->dev, 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) {
> + devm_kfree(ph->dev, group->group_pins);

This is a red flag. Is this function is solely used at the ->probe() stage?
I do not think so. Why then the devm_*() is being used to begin with?

Also what are the object lifetimes for device here and the _group_info
instances?

> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}

...

> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{

As per above.

> +}

...

> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .get_count = scmi_pinctrl_get_count,
> + .get_name = scmi_pinctrl_get_name,
> + .get_group_pins = scmi_pinctrl_get_group_pins,
> + .get_function_groups = scmi_pinctrl_get_function_groups,
> + .set_mux = scmi_pinctrl_set_mux,
> + .get_config = scmi_pinctrl_get_config,
> + .set_config = scmi_pinctrl_set_config,
> + .request_pin = scmi_pinctrl_request_pin,
> + .free_pin = scmi_pinctrl_free_pin

It's not a terminator item, so leave trailing comma. It will reduce the burden
in case of update of this.

> +};

...

> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{

> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;

All the same, why devm_*() is in use and what are the object lifetimes?

> +}

...

> + for (i = 0; i < pi->nr_groups; i++)
> + if (pi->groups[i].present) {
> + devm_kfree(ph->dev, pi->groups[i].group_pins);
> + pi->groups[i].present = false;
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++)
> + if (pi->functions[i].present) {
> + devm_kfree(ph->dev, pi->functions[i].groups);
> + pi->functions[i].present = false;
> + }

Missing outer {}, but see above as well.

...

> +static const struct scmi_protocol scmi_pinctrl = {
> + .id = SCMI_PROTOCOL_PINCTRL,

> + .owner = THIS_MODULE,

This is not needed if you use a trick from ~15 years back...

> + .instance_init = &scmi_pinctrl_protocol_init,
> + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> + .ops = &pinctrl_proto_ops,
> +};
> +

Redundant blank line.

> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)

...i.e. initializing the owner by this macro.

It might require some update to the above macro and its users.

...

> +enum scmi_pinctrl_selector_type {
> + PIN_TYPE = 0,
> + GROUP_TYPE,
> + FUNCTION_TYPE

Leave trailing comma.

> +};

--
With Best Regards,
Andy Shevchenko

2023-06-07 07:52:26

[permalink] [raw]

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

Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.
>
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
>
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.

...

> +++ b/drivers/pinctrl/Kconfig
> @@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
> source "drivers/pinctrl/visconti/Kconfig"
> source "drivers/pinctrl/vt8500/Kconfig"
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver controlled via SCMI 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.

Sounds to me that u and v should be after S. Decrypting for your convenience,
the above is ordered and proposed change misses that.

> endif

Btw, what is this endif for and how does it affect your Kconfig option?

...

> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o
> obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o

Ditto.

> obj-y += actions/
> obj-$(CONFIG_ARCH_ASPEED) += aspeed/

...

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

> +struct scmi_pinctrl_funcs {
> + unsigned int num_groups;
> + const char **groups;
> +};

struct pinfunction

...

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

struct pingroup ?

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

...

> + pmx = pinctrl_dev_get_drvdata(pctldev);

> +

Redundant blank line.

> + if (!pmx || !pmx->ph)
> + return NULL;

...

> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

Ditto. And so on in a few more places.

...

> + pmx->functions[selector].groups[i] =

> + pinctrl_scmi_get_group_name(pmx->pctldev,
> + group_ids[i]);

It's okay to have this on a single line which takes only 81 character.

...

> +error:

Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto.

> + devm_kfree(pmx->dev, pmx->functions[selector].groups);

Red Flag. Please, elaborate.

> +
> + return ret;

...

> +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;
> + enum pin_config_param config_type;
> + unsigned long config_value;

> + if (!pctldev)
> + return -EINVAL;

Huh?! When this is not a dead code?

Ditto for other places.

> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config %ld\n",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +}

...

> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,

Why this strange parameter name?

> + unsigned long *config)

...

> + err_free:

This is better, but shows the inconsistency with the other goto label namings.

> + devm_kfree(pmx->dev, pmx->pins);

Red Flag. Please, elaborate.

> + pmx->nr_pins = 0;
> +
> + return ret;

...

> + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret) {
> + dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
> + return ret;

return dev_err_probe(...);

> + }

...

> + pmx->functions =
> + devm_kcalloc(&sdev->dev, pmx->nr_functions,

This is perfectly a signle line.

Also with

struct device *dev = &sdev->dev;

at the top you may make the entire ->probe() look neater.

> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;

--
With Best Regards,
Andy Shevchenko

2023-06-09 07:42:23

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

On Tue, Jun 6, 2023 at 6:22 PM Oleksii Moisieiev
<[email protected]> wrote:

> Add new SCMI v3.2 pinctrl protocol bindings definitions and example.
>
> Signed-off-by: Oleksii Moisieiev <[email protected]>

This looks very good to me! The DT is very readable which is exactly
according to my philosophy when I designed these abstractions and bindings.
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2023-06-14 23:10:20

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

On Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev wrote:
> Add new SCMI v3.2 pinctrl protocol bindings definitions and example.
>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> ---
> .../bindings/firmware/arm,scmi.yaml | 53 +++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..a19aa184bbd1 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -233,6 +233,39 @@ properties:
> reg:
> const: 0x18
>
> + protocol@19:
> + $ref: '#/$defs/protocol-node'

unevaluatedProperties: false

> +
> + properties:
> + reg:
> + const: 0x19
> +
> + '#pinctrl-cells':
> + const: 0
> +
> + allOf:
> + - $ref: /schemas/pinctrl/pinctrl.yaml#

Group this and the '#/$defs/protocol-node' $ref under allOf.

> +
> + required:
> + - reg
> +
> + additionalProperties:
> + anyOf:

Don't need anyOf with only 1 entry.

But the use of additionalProperties is usually for existing cases where
the pin config nodes had no naming convention. For new bindings, define
a node name pattern (under patternProperties). I'd suggest '-pins$' as
used elsewhere.

> + - type: object
> + allOf:
> + - $ref: /schemas/pinctrl/pincfg-node.yaml#
> + - $ref: /schemas/pinctrl/pinmux-node.yaml#
> +
> + 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.
> +
> + unevaluatedProperties: false
> +
> additionalProperties: false
>
> $defs:
> @@ -384,6 +417,26 @@ examples:
> scmi_powercap: protocol@18 {
> reg = <0x18>;
> };
> +
> + scmi_pinctrl: protocol@19 {
> + reg = <0x19>;
> + #pinctrl-cells = <0>;
> +
> + i2c2 {
> + groups = "i2c2_a", "i2c2_b";
> + function = "i2c2";
> + };
> +
> + pins_mdio {
> + groups = "avb_mdio";
> + drive-strength = <24>;
> + };
> +
> + keys_pins: keys {
> + pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1";
> + bias-pull-up;
> + };
> + };
> };
> };
>
> --
> 2.25.1

2023-06-30 10:54:25

[permalink] [raw]

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

On Tue, Jun 06, 2023 at 04:22:26PM +0000, Oleksii Moisieiev wrote:
> This Patch series is intended to introduce the potential generic driver for
> pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0].
>
> On ARM-based systems, a separate Cortex-M based System Control Processor (SCP)
> provides control on pins, as well as with power, clocks, reset controllers. In this case,
> kernel should use one of the possible transports, described in [0] to access SCP and
> control clocks/power-domains etc. This driver is using SMC transport to communicate with SCP via
> SCMI protocol and access to the Pin Control Subsystem.
>

Hi Oleksii,

sorry this review has been a bit delayed, I tested V3 in my setup and found no
practical issue. I'll post a few comments/remarks along the series.

Beside addressing pending comments, I suppose that the next step will be anyway
to address the upcoming changes in the new v3.2 BETA regarding support for
multiple type/value set/get operations as requested by Peng.

Thanks,
Cristian

2023-06-30 16:31:24

[permalink] [raw]

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

On Wed, Jun 07, 2023 at 10:10:44AM +0300, [email protected] wrote:
> Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:

Hi Andy,

I'll answer some of your remarks down below, since more generally
related to the SCMI stack.

> > scmi: Introduce pinctrl SCMI protocol driver
>
> Seems like you forgot to remove previous line(s) in the commit message and
> the above has to be the Subject here.
>
> > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > excluding GPIO support. All pinctrl related callbacks and operations
> > are exposed in the include/linux/scmi_protocol.h
>
> drop include/ part, everybody will understand that. Also mind the grammar
> period.
>
> ...
>
> > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
>
> Why not splitting it and make it ordered?
>

Maybe a good idea for a separate cleanup...not sure can fit this series
without causing churn with other in-flight SCMI series...I'll happily wait
for Sudeep to decide.

> ...
>
> Missing headers:
>
> bitfield.h
> bits.h
> byteorder/
> types.h
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> Missing
>
> asm/unaligned.h
>
> ...
>

Most if not all of these headers are already included by the

#include "protocols.h"

above that introduces a lot of common other stuff needed to implement
a new SCMI protocol.

> > +struct scmi_group_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *group_pins;
> > + unsigned int nr_pins;
> > +};
>
> So, why struct pingroup can't be embeded here?
>
> > +struct scmi_function_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *groups;
> > + unsigned int nr_groups;
> > +};
>
> So, why and struct pinfunction can't be embedded here (yes, you would need a
> duplication of groups as here they are integers)?
>
> As far as I understand these data structures are not part of any ABI (otherwise
> the wrong type(s) / padding might be in use) and hence don't see the impediments
> to use them, but would be nice to have a comment on top of each.
>
> ...
>
> > +struct scmi_pin_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
>
> Swapping order might help compiler to generate a better code.
> Also this applies to the _group_info and _function_info.
>
> > +};
>
> ...
>
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
>
> Can you rather follow the usual pattern, i.e. checking for the errors?
>

I think Oleksii here followed the (opinable maybe) pattern we have in
the SCMI stack where typically you get/build/send/put an scmi message
(xfer) while doing a few things only if the message was sent
successfully (if !ret ... most of the time): checking for success avoid
a lot of 'goto err:' all around.

[snip]

> > +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 = devm_kmalloc_array(ph->dev, 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) {
> > + devm_kfree(ph->dev, group->group_pins);
>
> This is a red flag. Is this function is solely used at the ->probe() stage?
> I do not think so. Why then the devm_*() is being used to begin with?
>
> Also what are the object lifetimes for device here and the _group_info
> instances?
>
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
>
> ...
>
> > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_function_info *func)
> > +{
>
> As per above.
>
> > +}
>
> ...
>
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .get_count = scmi_pinctrl_get_count,
> > + .get_name = scmi_pinctrl_get_name,
> > + .get_group_pins = scmi_pinctrl_get_group_pins,
> > + .get_function_groups = scmi_pinctrl_get_function_groups,
> > + .set_mux = scmi_pinctrl_set_mux,
> > + .get_config = scmi_pinctrl_get_config,
> > + .set_config = scmi_pinctrl_set_config,
> > + .request_pin = scmi_pinctrl_request_pin,
> > + .free_pin = scmi_pinctrl_free_pin
>
> It's not a terminator item, so leave trailing comma. It will reduce the burden
> in case of update of this.
>
> > +};
>
> ...
>
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
>
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
>
> All the same, why devm_*() is in use and what are the object lifetimes?
>
> > +}
>

This bit about alocation and devres deserves an explanation in the context
of the SCMI stack.

So, you can add support for a new SCMI protocol using the below macro

DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER

to register with the core SCMI stack a few things like an
initialization function and the protocol operations you wish this
protocol to expose.

At run-time, once the first user of your new protocol comes up (like
the pinctrl driver later in the series), the core SCMI will take care
to setup and initialize the protocol so that can be used by the SCMI
drivers (like pinctrl-scmi.c) via its exposed proto_operations.
(assuming the protocol has been also found as supported by the fw
serving as the SCMI server)

When the last user of a protocol is gone, similarly, the protocol
will be deinitialized (if anything is needed to be deinit really...)

Basically the core calls upfront the protocol_init function you provided
and passes to it a ph protocol_handle that embeds a number of things
useful for protocol implementations, like as example the xops-> needed
to build and send messages using the core facilities.

Another thing that is embedded in the ph, as you noticed, is the ph->dev
device reference to be optionally used for devres in your protocol: now,
we do NOT have per-protocol devices, so, that device lifetine is NOT bound
strictly to this protocol but to the whole stack... BUT the SCMI core
takes care to open/close a devres group around your protocol_init invocation,
so that you can use devres on your .protocol_init, and be assured that when
your protocol will be de-initialized (since no more used by anyone) all your
devres allocations will be freed.

For this see:

drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()

This clearly works ONLY for allocations descending directly from the
.protocol_init() call (when the devres group is open) and it was working
fine till today for all protocols, since all existing protocols
allocated all what they needed during protocol_init....

... Pinctrl is a differenet beast, though, since it could make sense indeed
(even though still under a bit of discussion..) to delay some allocations and
SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
some resources only later when the pinctrl subsystem will parse the DT and will
ask the pinctrl-scmi driver just for the strictly needed resources.
(so you avoid to query and allocate at boot time a bunch of pin stuff that you
will never use...)

These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
happen outside of the .protocol_init path so they HAVE TO to be explicitly
managed manually without devres; as a consequence the addition of a
dedicated .protocol_deinit() function and the frees on the err path: so
that anything non devres allocated in the protcol devres_group can be
freed properly when the core deinitializes the protocol.

What is WRONG, though, in this patch (and I missed it ... my bad) is that such
explicit manual alloc/dealloc need not and should not be of devres kind but just
plain simple kmalloc_ / kfree.
(even though it is not harmful in this context...the ph->dev never unbounds till
the end of the stack..it is certainkly not needed and confusing)

Hoping not to have bored you to death with all of this SCMI digression... :D

Thanks,
Cristian

2023-07-03 11:09:27

[permalink] [raw]

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

On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> scmi: Introduce pinctrl SCMI protocol driver
>

Hi Oleksii,

spurios line above.

> Add basic implementation of the SCMI v3.2 pincontrol protocol
> excluding GPIO support. All pinctrl related callbacks and operations
> are exposed in the include/linux/scmi_protocol.h
>

As Andy said already, you can drop the second sentence here, but I would
ALSO drop the GPIO part in the first sentence, since there is nothing
specific to GPIO in the SCMI spec and this patch is about the SCMI protocol
not the pinctrl driver.

> Signed-off-by: Oleksii Moisieiev <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/firmware/arm_scmi/Makefile | 2 +-
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 47 ++
> 6 files changed, 893 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0dab9737ec16..297b2512963d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h
> F: include/trace/events/scmi.h
> F: include/uapi/linux/virtio_scmi.h
>
> +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)

SCPI is a leftover here I suppose...

> +M: Oleksii Moisieiev <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/firmware/arm_scmi/pinctrl.c
> +
> SYSTEM RESET/SHUTDOWN DRIVERS
> M: Sebastian Reichel <[email protected]>
> L: [email protected]
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index b31d78fa66cc..603430ec0bfe 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 5be931a07c84..a9fd337b9596 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3025,6 +3025,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);
> }
> @@ -3042,6 +3043,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..fc0fcc26dfb6
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,836 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "protocols.h"
> +
> +#define 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 config_value;
> +};
> +
> +struct scmi_msg_conf_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> + __le32 attributes_low;
> + __le32 attributes_high;
> +};
> +
> +struct scmi_msg_pinctrl_attributes {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> + __le32 attributes;
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> + __le32 identifier;
> + __le32 flags;
> + __le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> + __le32 flags;
> + __le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};
> +
> +struct scmi_msg_request {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_group_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *group_pins;
> + unsigned int nr_pins;
> +};
> +
> +struct scmi_function_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> + unsigned int *groups;
> + unsigned int nr_groups;
> +};
> +

A small note related to Andy remarks about directly embedding here pinctrl
subsystem structures (like pingroup / pinfucntion) that I forgot to say
in my reply to him.

These structs above indeed are very similar to the Pinctrl ones but this is
the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl
subsystem which is, at the end the, one of the possible users of this layer
(via the SCMI pinctrl driver) but not necessarily the only one in the
future; moreover Pinctrl subsystem is not even needed at all if you think
about a testing scenario, so I would not build up a dependency here between
SCMI and Pinctrl by using Pinctrl structures...what if these latter change
in the future ?

All of this to just say this is fine for me as it is now :D

> +struct scmi_pin_info {
> + bool present;
> + char name[SCMI_MAX_STR_SIZE];
> +};
> +
> +struct scmi_pinctrl_info {
> + u32 version;
> + int nr_groups;
> + int nr_functions;
> + int nr_pins;
> + struct scmi_group_info *groups;
> + struct scmi_function_info *functions;
> + struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + if (!pi)
> + return -EINVAL;

You can drop this, cannot happen given the code paths.

> +
> + 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_get_count(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_pinctrl_info *pi;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -ENODEV;

You dont need to check for NULL here and nowhere else.
You set protocol private data with set_priv at the end of protocol init
which is called as soon as a user tries to use this protocol operations,
so it cannot ever be NULL in any of these following ops.

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

[snip]

> +
> +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 = devm_kmalloc_array(ph->dev, group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;

This is a lazy-allocation happening outside of the protocol init path so you
are rigthly tracking it manually here and in protocol_deinit, BUT you
should not use devm_ helpers, it is fuorviating even though harmless;
just plain kmalloc/kcalloc/kfree will do. (As I said oin the reply to
Andy I miss this previously, apologies)

> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + devm_kfree(ph->dev, group->group_pins);

kfree

> + 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;
> +
> + if (!name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)

Ditto.

> + return -EINVAL;
> +
> + if (selector > pi->nr_groups)
> + return -EINVAL;

selector >= ?

> +
> + 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_get_group_pins(const struct scmi_protocol_handle *ph,
> + u32 selector, const unsigned int **pins,
> + unsigned int *nr_pins)
> +{
> + struct scmi_pinctrl_info *pi;
> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto.

> +
> + if (selector > pi->nr_groups)
> + return -EINVAL;
> +

selector >= ?

> + 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 = devm_kmalloc_array(ph->dev, func->nr_groups,
> + sizeof(*func->groups),
> + GFP_KERNEL);
> + if (!func->groups)
> + return -ENOMEM;

Same as above...lazy allocation properly tracked BUT do not use devm_
variants.

> +
> + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> + func->nr_groups, func->groups);
> + if (ret) {
> + devm_kfree(ph->dev, func->groups);
> + return ret;

kfree

> + }
> +
> + 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;
> +
> + if (!name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto.

> +
> + if (selector > pi->nr_functions)
> + return -EINVAL;

selector >= ?

> +
> + 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_get_function_groups(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + unsigned int *nr_groups,
> + const unsigned int **groups)
> +{
> + struct scmi_pinctrl_info *pi;
> +
> + if (!groups || !nr_groups)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto.

> +
> + if (selector > pi->nr_functions)
> + return -EINVAL;

selector >= ?

> +
> + 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_set_mux(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 group)
> +{
> + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> + selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> + u32 selector, struct scmi_pin_info *pin)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto.

> +
> + 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;
> +
> + if (!name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto.

> +
> + if (selector > pi->nr_pins)
> + return -EINVAL;

selector >= ?

> +
> + 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_get_name(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name)
> +{
> + switch (type) {
> + case PIN_TYPE:
> + return scmi_pinctrl_get_pin_name(ph, selector, name);
> + case GROUP_TYPE:
> + return scmi_pinctrl_get_group_name(ph, selector, name);
> + case FUNCTION_TYPE:
> + return scmi_pinctrl_get_function_name(ph, selector, name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .get_count = scmi_pinctrl_get_count,
> + .get_name = scmi_pinctrl_get_name,
> + .get_group_pins = scmi_pinctrl_get_group_pins,
> + .get_function_groups = scmi_pinctrl_get_function_groups,
> + .set_mux = scmi_pinctrl_set_mux,
> + .get_config = scmi_pinctrl_get_config,
> + .set_config = scmi_pinctrl_set_config,
> + .request_pin = scmi_pinctrl_request_pin,
> + .free_pin = scmi_pinctrl_free_pin
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
> +
> + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> + sizeof(*pinfo->pins),
> + GFP_KERNEL);
> + if (!pinfo->pins)
> + return -ENOMEM;
> +
> + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> + sizeof(*pinfo->groups),
> + GFP_KERNEL);
> + if (!pinfo->groups)
> + return -ENOMEM;
> +
> + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> + sizeof(*pinfo->functions),
> + GFP_KERNEL);
> + if (!pinfo->functions)
> + return -ENOMEM;
> +
> + pinfo->version = version;
> +
> + return ph->set_priv(ph, pinfo);
> +}
> +
> +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> +{
> + int i;
> + struct scmi_pinctrl_info *pi;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;

Ditto. You never get even here if protocol init had not succesfully
completed.

> +
> + for (i = 0; i < pi->nr_groups; i++)
> + if (pi->groups[i].present) {
> + devm_kfree(ph->dev, pi->groups[i].group_pins);

kfree..you are managing these.

> + pi->groups[i].present = false;
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++)
> + if (pi->functions[i].present) {
> + devm_kfree(ph->dev, pi->functions[i].groups);

kfree..you are managing these.

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

Can you move all of these before .notify_ops where all others protocol
ops lives ? ... and rename all pinctrl_ops to match the ".<object>_<verb>"
pattern like other ops (i.e. count_get/name_get ... instead get_count/get_name)

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

Thanks,
Cristian

2023-07-03 11:27:28

[permalink] [raw]

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

On Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev wrote:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.

I would drop any reference to SCP, which is a reference implementation
BUT not neccesarily the only one.... "SCMI platform firware" should be
generic enough.

>
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
>
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.

This should be obvious, I would drop any reference to the fact that you
need an SCMI server somewhere replying to your requests and to the need
of a DT configuration thing which is anyeway standard.

>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 554 +++++++++++++++++++++++++++++++++
> 4 files changed, 567 insertions(+)
> create mode 100644 drivers/pinctrl/pinctrl-scmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 297b2512963d..91883955fc1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20527,6 +20527,7 @@ M: Oleksii Moisieiev <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/firmware/arm_scmi/pinctrl.c
> +F: drivers/pinctrl/pinctrl-scmi.c
>
> SYSTEM RESET/SHUTDOWN DRIVERS
> M: Sebastian Reichel <[email protected]>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 5787c579dcf6..c4680a2c5e13 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
> source "drivers/pinctrl/visconti/Kconfig"
> source "drivers/pinctrl/vt8500/Kconfig"
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver controlled via SCMI interface"

"using SCMI protocol interface" maybe is more fitting...it is the pin
that is controlled via SCMI not the driver, which is indeed the
controller of the pin via SCMI O_o

> + 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.
> +
> endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index e196c6e324ad..b932a116e6a0 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o
> obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
>

Probably needs to be alphabetically ordered.

> obj-y += actions/
> obj-$(CONFIG_ARCH_ASPEED) += aspeed/
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> new file mode 100644
> index 000000000000..e46dffa652c6
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl_funcs {
> + unsigned int num_groups;
> + const char **groups;
> +};
> +
> +struct scmi_pinctrl {
> + struct device *dev;
> + struct scmi_protocol_handle *ph;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_desc pctl_desc;
> + struct scmi_pinctrl_funcs *functions;
> + unsigned int nr_functions;
> + char **groups;
> + unsigned int nr_groups;
> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

This check is superfluos right ? If you registered successfully with the
Pinctrl subsys and you passed your pmx driver_data as properly
initialized in _probe you can never get a NULL here for pmx or pmx->ph.

> +
> + return pinctrl_ops->get_count(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;
> +
> + if (!pctldev)
> + return NULL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return NULL;
> +

Ditto.

> + ret = pinctrl_ops->get_name(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;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +

Ditto.

> + return pinctrl_ops->get_group_pins(pmx->ph, selector,
> + pins, num_pins);
> +}
> +
> +#ifdef CONFIG_OF
> +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
> + struct device_node *np_config,
> + struct pinctrl_map **map,
> + u32 *num_maps)
> +{
> + return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
> + num_maps, PIN_MAP_TYPE_INVALID);
> +}
> +
> +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
> + struct pinctrl_map *map, u32 num_maps)
> +{
> + kfree(map);
> +}
> +
> +#endif /* CONFIG_OF */
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> + .get_groups_count = pinctrl_scmi_get_groups_count,
> + .get_group_name = pinctrl_scmi_get_group_name,
> + .get_group_pins = pinctrl_scmi_get_group_pins,
> +#ifdef CONFIG_OF
> + .dt_node_to_map = pinctrl_scmi_dt_node_to_map,
> + .dt_free_map = pinctrl_scmi_dt_free_map,
> +#endif
> +};
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +

Ditto.

> + return pinctrl_ops->get_count(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;
> +
> + if (!pctldev)
> + return NULL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return NULL;
> +

Ditto.

> + ret = pinctrl_ops->get_name(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;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !groups || !num_groups)
> + return -EINVAL;
> +

Ditto for pmx/pmx->ph.

> + 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->get_function_groups(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 error;
> + }
> + }
> +
> + *groups = (const char * const *)pmx->functions[selector].groups;
> +
> + return 0;
> +
> +error:
> + 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;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

Ditto.

> +
> + return pinctrl_ops->set_mux(pmx->ph, selector, group);
> +}
> +
> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
> + unsigned int offset)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

Ditto.

> +
> + return pinctrl_ops->request_pin(pmx->ph, offset);
> +}
> +
> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

Ditto.

> +
> + return pinctrl_ops->free_pin(pmx->ph, offset);
> +}
> +
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> + .request = pinctrl_scmi_request,
> + .free = pinctrl_scmi_free,
> + .get_functions_count = pinctrl_scmi_get_functions_count,
> + .get_function_name = pinctrl_scmi_get_function_name,
> + .get_function_groups = pinctrl_scmi_get_function_groups,
> + .set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !config)
> + return -EINVAL;
> +

Ditto for pmx/pmx->ph.

> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config %ld\n",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
> + config_type, config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config = %ld",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !config)
> + return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
> + config_type, &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> + .is_generic = true,
> + .pin_config_get = pinctrl_scmi_pinconf_get,
> + .pin_config_set = pinctrl_scmi_pinconf_set,
> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> + .pin_config_config_dbg_show = pinconf_generic_dump_config,
> +};
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + unsigned int *nr_pins,
> + const struct pinctrl_pin_desc **pins)
> +{
> + int ret, i;
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;

Ditto.

> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + if (pmx->nr_pins) {
> + *pins = pmx->pins;
> + *nr_pins = pmx->nr_pins;
> + return 0;
> + }
> +
> + *nr_pins = pinctrl_ops->get_count(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->get_name(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);
> + goto err_free;
> + }
> + }
> +
> + *pins = pmx->pins;
> + dev_dbg(pmx->dev, "got pins %d", *nr_pins);
> +
> + return 0;
> + err_free:
> + devm_kfree(pmx->dev, pmx->pins);
> + pmx->nr_pins = 0;
> +
> + return ret;
> +}
> +
> +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 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(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = &sdev->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(&sdev->dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret) {
> + dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
> + return ret;
> + }
> +
> + 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(&sdev->dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
> + }
> +
> + if (pmx->nr_groups) {
> + pmx->groups =
> + devm_kcalloc(&sdev->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);
> +

Thanks,
Cristian

2023-07-03 13:29:15

[permalink] [raw]

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

Hi Cristian,

Thanks for the testing and for the review.
I will address all comments and v4 will be with v3.2 BETA changes.

Best regards,
Oleksii.

On Fri, Jun 30, 2023 at 11:38:47AM +0100, Cristian Marussi wrote:
> On Tue, Jun 06, 2023 at 04:22:26PM +0000, Oleksii Moisieiev wrote:
> > This Patch series is intended to introduce the potential generic driver for
> > pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0].
> >
> > On ARM-based systems, a separate Cortex-M based System Control Processor (SCP)
> > provides control on pins, as well as with power, clocks, reset controllers. In this case,
> > kernel should use one of the possible transports, described in [0] to access SCP and
> > control clocks/power-domains etc. This driver is using SMC transport to communicate with SCP via
> > SCMI protocol and access to the Pin Control Subsystem.
> >
>
> Hi Oleksii,
>
> sorry this review has been a bit delayed, I tested V3 in my setup and found no
> practical issue. I'll post a few comments/remarks along the series.
>
> Beside addressing pending comments, I suppose that the next step will be anyway
> to address the upcoming changes in the new v3.2 BETA regarding support for
> multiple type/value set/get operations as requested by Peng.
>
> Thanks,
> Cristian

2023-07-03 21:57:37

[permalink] [raw]

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

Fri, Jun 30, 2023 at 05:02:12PM +0100, Cristian Marussi kirjoitti:
> On Wed, Jun 07, 2023 at 10:10:44AM +0300, [email protected] wrote:
> > Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:

...

> > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> >
> > Why not splitting it and make it ordered?
>
> Maybe a good idea for a separate cleanup...not sure can fit this series
> without causing churn with other in-flight SCMI series...I'll happily wait
> for Sudeep to decide.

Sure.

...

> > Missing headers:
> >
> > bitfield.h
> > bits.h
> > byteorder/
> > types.h
> >
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> >
> > Missing
> >
> > asm/unaligned.h
>
> Most if not all of these headers are already included by the
>
> #include "protocols.h"
>
> above that introduces a lot of common other stuff needed to implement
> a new SCMI protocol.

OK!

...

> > > + ret = ph->xops->do_xfer(ph, t);
> > > + if (!ret) {
> >
> > Can you rather follow the usual pattern, i.e. checking for the errors?
>
> I think Oleksii here followed the (opinable maybe) pattern we have in
> the SCMI stack where typically you get/build/send/put an scmi message
> (xfer) while doing a few things only if the message was sent
> successfully (if !ret ... most of the time): checking for success avoid
> a lot of 'goto err:' all around.

If it's
ret = fpp();
if (!ret)
ret = bpp();
return ret;

I would agree with you, but in some cases it involves more core and that code
doesn't affect ret itself.

> > > + }

...

> > All the same, why devm_*() is in use and what are the object lifetimes?
>
> This bit about alocation and devres deserves an explanation in the context
> of the SCMI stack.
>
> So, you can add support for a new SCMI protocol using the below macro
>
> DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER
>
> to register with the core SCMI stack a few things like an
> initialization function and the protocol operations you wish this
> protocol to expose.
>
> At run-time, once the first user of your new protocol comes up (like
> the pinctrl driver later in the series), the core SCMI will take care
> to setup and initialize the protocol so that can be used by the SCMI
> drivers (like pinctrl-scmi.c) via its exposed proto_operations.
> (assuming the protocol has been also found as supported by the fw
> serving as the SCMI server)
>
> When the last user of a protocol is gone, similarly, the protocol
> will be deinitialized (if anything is needed to be deinit really...)
>
> Basically the core calls upfront the protocol_init function you provided
> and passes to it a ph protocol_handle that embeds a number of things
> useful for protocol implementations, like as example the xops-> needed
> to build and send messages using the core facilities.
>
> Another thing that is embedded in the ph, as you noticed, is the ph->dev
> device reference to be optionally used for devres in your protocol: now,
> we do NOT have per-protocol devices, so, that device lifetine is NOT bound
> strictly to this protocol but to the whole stack... BUT the SCMI core
> takes care to open/close a devres group around your protocol_init invocation,
> so that you can use devres on your .protocol_init, and be assured that when
> your protocol will be de-initialized (since no more used by anyone) all your
> devres allocations will be freed.
>
> For this see:
>
> drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()
>
> This clearly works ONLY for allocations descending directly from the
> .protocol_init() call (when the devres group is open) and it was working
> fine till today for all protocols, since all existing protocols
> allocated all what they needed during protocol_init....
>
> ... Pinctrl is a differenet beast, though, since it could make sense indeed
> (even though still under a bit of discussion..) to delay some allocations and
> SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
> some resources only later when the pinctrl subsystem will parse the DT and will
> ask the pinctrl-scmi driver just for the strictly needed resources.
> (so you avoid to query and allocate at boot time a bunch of pin stuff that you
> will never use...)
>
> These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
> happen outside of the .protocol_init path so they HAVE TO to be explicitly
> managed manually without devres; as a consequence the addition of a
> dedicated .protocol_deinit() function and the frees on the err path: so
> that anything non devres allocated in the protcol devres_group can be
> freed properly when the core deinitializes the protocol.
>
> What is WRONG, though, in this patch (and I missed it ... my bad) is that such
> explicit manual alloc/dealloc need not and should not be of devres kind but just
> plain simple kmalloc_ / kfree.
> (even though it is not harmful in this context...the ph->dev never unbounds till
> the end of the stack..it is certainkly not needed and confusing)
>
> Hoping not to have bored you to death with all of this SCMI digression... :D

Thank you for a dive into the implementation of the SCMI. Perhaps you can
summarize that into some kernel doc aroung thouse callbacks, so people can
clearly see when it's possible and when it's not to use devm_*() APIs.

--
With Best Regards,
Andy Shevchenko

2023-07-06 11:07:37

[permalink] [raw]

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

On Tue, Jul 04, 2023 at 12:27:40AM +0300, [email protected] wrote:
> Fri, Jun 30, 2023 at 05:02:12PM +0100, Cristian Marussi kirjoitti:
> > On Wed, Jun 07, 2023 at 10:10:44AM +0300, [email protected] wrote:
> > > Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
>
> ...
>

Hi Andy,

> > > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > >
> > > Why not splitting it and make it ordered?
> >
> > Maybe a good idea for a separate cleanup...not sure can fit this series
> > without causing churn with other in-flight SCMI series...I'll happily wait
> > for Sudeep to decide.

[snip]

> > > > + }
>
> ...
>
> > > All the same, why devm_*() is in use and what are the object lifetimes?
> >
> > This bit about alocation and devres deserves an explanation in the context
> > of the SCMI stack.
> >
> > So, you can add support for a new SCMI protocol using the below macro
> >
> > DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER
> >
> > to register with the core SCMI stack a few things like an
> > initialization function and the protocol operations you wish this
> > protocol to expose.
> >
> > At run-time, once the first user of your new protocol comes up (like
> > the pinctrl driver later in the series), the core SCMI will take care
> > to setup and initialize the protocol so that can be used by the SCMI
> > drivers (like pinctrl-scmi.c) via its exposed proto_operations.
> > (assuming the protocol has been also found as supported by the fw
> > serving as the SCMI server)
> >
> > When the last user of a protocol is gone, similarly, the protocol
> > will be deinitialized (if anything is needed to be deinit really...)
> >
> > Basically the core calls upfront the protocol_init function you provided
> > and passes to it a ph protocol_handle that embeds a number of things
> > useful for protocol implementations, like as example the xops-> needed
> > to build and send messages using the core facilities.
> >
> > Another thing that is embedded in the ph, as you noticed, is the ph->dev
> > device reference to be optionally used for devres in your protocol: now,
> > we do NOT have per-protocol devices, so, that device lifetine is NOT bound
> > strictly to this protocol but to the whole stack... BUT the SCMI core
> > takes care to open/close a devres group around your protocol_init invocation,
> > so that you can use devres on your .protocol_init, and be assured that when
> > your protocol will be de-initialized (since no more used by anyone) all your
> > devres allocations will be freed.
> >
> > For this see:
> >
> > drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()
> >
> > This clearly works ONLY for allocations descending directly from the
> > .protocol_init() call (when the devres group is open) and it was working
> > fine till today for all protocols, since all existing protocols
> > allocated all what they needed during protocol_init....
> >
> > ... Pinctrl is a differenet beast, though, since it could make sense indeed
> > (even though still under a bit of discussion..) to delay some allocations and
> > SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
> > some resources only later when the pinctrl subsystem will parse the DT and will
> > ask the pinctrl-scmi driver just for the strictly needed resources.
> > (so you avoid to query and allocate at boot time a bunch of pin stuff that you
> > will never use...)
> >
> > These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
> > happen outside of the .protocol_init path so they HAVE TO to be explicitly
> > managed manually without devres; as a consequence the addition of a
> > dedicated .protocol_deinit() function and the frees on the err path: so
> > that anything non devres allocated in the protcol devres_group can be
> > freed properly when the core deinitializes the protocol.
> >
> > What is WRONG, though, in this patch (and I missed it ... my bad) is that such
> > explicit manual alloc/dealloc need not and should not be of devres kind but just
> > plain simple kmalloc_ / kfree.
> > (even though it is not harmful in this context...the ph->dev never unbounds till
> > the end of the stack..it is certainkly not needed and confusing)
> >
> > Hoping not to have bored you to death with all of this SCMI digression... :D
>
> Thank you for a dive into the implementation of the SCMI. Perhaps you can
> summarize that into some kernel doc aroung thouse callbacks, so people can
> clearly see when it's possible and when it's not to use devm_*() APIs.
>

Absolutely, this is definitely missing, it's just that till now I was
the only dealing with this, so docs was overlooked ... and I am really
the first to be in need of this documentation :P

Thanks,
Cristian

P.S.: and apologies for late replies but our mail server seems to
constantly classify you as spam :D

2023-07-06 14:58:59

[permalink] [raw]

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

Hi Andy,

On Wed, Jun 07, 2023 at 10:10:44AM +0300, [email protected] wrote:
> Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> > scmi: Introduce pinctrl SCMI protocol driver
>
> Seems like you forgot to remove previous line(s) in the commit message and
> the above has to be the Subject here.
>
> > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > excluding GPIO support. All pinctrl related callbacks and operations
> > are exposed in the include/linux/scmi_protocol.h
>
> drop include/ part, everybody will understand that. Also mind the grammar
> period.
>
Fixed. Thanks,

> ...
>
> > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
>
> Why not splitting it and make it ordered?
>
> ...
>
> Missing headers:
>
> bitfield.h
> bits.h
> byteorder/
> types.h
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> Missing
>
> asm/unaligned.h
>
> ...
>
> > +struct scmi_group_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *group_pins;
> > + unsigned int nr_pins;
> > +};
>
> So, why struct pingroup can't be embeded here?
>
> > +struct scmi_function_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *groups;
> > + unsigned int nr_groups;
> > +};
>
> So, why and struct pinfunction can't be embedded here (yes, you would need a
> duplication of groups as here they are integers)?
>
> As far as I understand these data structures are not part of any ABI (otherwise
> the wrong type(s) / padding might be in use) and hence don't see the impediments
> to use them, but would be nice to have a comment on top of each.
>

My opinion is that we don't want to make SCMI Pinctrl protocol to be
dependent from Pinctrl subsystem. This will potentially require SCMI
protocol change in case of significant Pinctrl subsystem refactoring.
Another reason is that pincfunction has the following groups
definition:
const char * const *groups;

Which is meant to be constantly allocated.
So I when I try to gather list of groups in
pinctrl_scmi_get_function_groups I will receive compilation error.

Pinctrl subsystem structs are designed to be statically allocated and
can't be used for lazy allocations.

> ...
>
> > +struct scmi_pin_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
>
> Swapping order might help compiler to generate a better code.
> Also this applies to the _group_info and _function_info.
>
> > +};
>
> ...
>
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
>
> Can you rather follow the usual pattern, i.e. checking for the errors?
>
> if (ret)
> goto out_put_xfer;
>
> > + 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);
> > + }
>
> out_put_xfer:
>
> > + ph->xops->xfer_put(ph, t);
> > + return ret;
>
> ...
>
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
>
> Ditto.
>
> > + 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))
>
> if (ret)
> return ret;
>
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> > + (u32 *)&type, name,
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
>
> return 0;
>
> > +}
>
> ...
>
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret)
>
> if (ret)
> goto out_put_xfer;
>
> (but in this and similar, aka one line, cases the current wouldn't be bad, just
> matter of the consistency with the rest of the code)
>
> > + *config_value = get_unaligned_le32(t->rx.buf);
> > +
> > + ph->xops->xfer_put(ph, t);
> > + return ret;
>
> ...
>
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> > + sizeof(*tx), 0, &t);
>
> This is perfectly one line. Please also check entire code for such an unneeded
> wrap.
>
Thanks, I will go through the code and fix this.
> ...
>
> > +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 = devm_kmalloc_array(ph->dev, 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) {
> > + devm_kfree(ph->dev, group->group_pins);
>
> This is a red flag. Is this function is solely used at the ->probe() stage?
> I do not think so. Why then the devm_*() is being used to begin with?
>
> Also what are the object lifetimes for device here and the _group_info
> instances?
>
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
>
> ...
>
> > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_function_info *func)
> > +{
>
> As per above.
>
> > +}
>
> ...
>
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .get_count = scmi_pinctrl_get_count,
> > + .get_name = scmi_pinctrl_get_name,
> > + .get_group_pins = scmi_pinctrl_get_group_pins,
> > + .get_function_groups = scmi_pinctrl_get_function_groups,
> > + .set_mux = scmi_pinctrl_set_mux,
> > + .get_config = scmi_pinctrl_get_config,
> > + .set_config = scmi_pinctrl_set_config,
> > + .request_pin = scmi_pinctrl_request_pin,
> > + .free_pin = scmi_pinctrl_free_pin
>
> It's not a terminator item, so leave trailing comma. It will reduce the burden
> in case of update of this.
>
Ok, thanks.
> > +};
>
> ...
>
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
>
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
>
> All the same, why devm_*() is in use and what are the object lifetimes?
>
Ok, thanks.
> > +}
>
> ...
>
> > + for (i = 0; i < pi->nr_groups; i++)
> > + if (pi->groups[i].present) {
> > + devm_kfree(ph->dev, pi->groups[i].group_pins);
> > + pi->groups[i].present = false;
> > + }
> > +
> > + for (i = 0; i < pi->nr_functions; i++)
> > + if (pi->functions[i].present) {
> > + devm_kfree(ph->dev, pi->functions[i].groups);
> > + pi->functions[i].present = false;
> > + }
>
> Missing outer {}, but see above as well.
>

Checked linux kernel code with the following command:
pcregrep -lcrM "for $.*$[^{]$\n.*if" .

As I can see - this approach is taking place in kernel code. Although
not as widely as for with outer {}. I will add outer {}.

> ...
>
> > +static const struct scmi_protocol scmi_pinctrl = {
> > + .id = SCMI_PROTOCOL_PINCTRL,
>
> > + .owner = THIS_MODULE,
>
> This is not needed if you use a trick from ~15 years back...
>
> > + .instance_init = &scmi_pinctrl_protocol_init,
> > + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> > + .ops = &pinctrl_proto_ops,
> > +};
> > +
>
> Redundant blank line.
>
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
>
> ...i.e. initializing the owner by this macro.
>
> It might require some update to the above macro and its users.
>
> ...
>
>
> > +enum scmi_pinctrl_selector_type {
> > + PIN_TYPE = 0,
> > + GROUP_TYPE,
> > + FUNCTION_TYPE
>
> Leave trailing comma.
>
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thanks,
Oleksii.

2023-07-06 15:02:12

[permalink] [raw]

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

Hi Cristian,

Sorry for late answer.
Please see below.

On Mon, Jul 03, 2023 at 11:16:47AM +0100, Cristian Marussi wrote:
> On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> > scmi: Introduce pinctrl SCMI protocol driver
> >
>
> Hi Oleksii,
>
> spurios line above.

Yep thanks, I will remove.

>
> > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > excluding GPIO support. All pinctrl related callbacks and operations
> > are exposed in the include/linux/scmi_protocol.h
> >
>
> As Andy said already, you can drop the second sentence here, but I would
> ALSO drop the GPIO part in the first sentence, since there is nothing
> specific to GPIO in the SCMI spec and this patch is about the SCMI protocol
> not the pinctrl driver.
>

I've added few words about GPIO because in v2 series Michal Simek asked
about it: https://lore.kernel.org/linux-arm-kernel/[email protected]/
So I've decided to mention that there is still no GPIO support in the
commit message to avoid this type of questions in future. But I agree
that the commit message looks weird and will try to rephrase it.

> > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > ---
> > MAINTAINERS | 6 +
> > drivers/firmware/arm_scmi/Makefile | 2 +-
> > drivers/firmware/arm_scmi/driver.c | 2 +
> > drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++
> > drivers/firmware/arm_scmi/protocols.h | 1 +
> > include/linux/scmi_protocol.h | 47 ++
> > 6 files changed, 893 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0dab9737ec16..297b2512963d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h
> > F: include/trace/events/scmi.h
> > F: include/uapi/linux/virtio_scmi.h
> >
> > +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
>
> SCPI is a leftover here I suppose...
>

Thanks. I'll fix it.

> > +M: Oleksii Moisieiev <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/firmware/arm_scmi/pinctrl.c
> > +
> > SYSTEM RESET/SHUTDOWN DRIVERS
> > M: Sebastian Reichel <[email protected]>
> > L: [email protected]
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index b31d78fa66cc..603430ec0bfe 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 5be931a07c84..a9fd337b9596 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -3025,6 +3025,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);
> > }
> > @@ -3042,6 +3043,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..fc0fcc26dfb6
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/pinctrl.c
> > @@ -0,0 +1,836 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System Control and Management Interface (SCMI) Pinctrl Protocol
> > + *
> > + * Copyright (C) 2023 EPAM
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> > +
> > +#include "protocols.h"
> > +
> > +#define 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 config_value;
> > +};
> > +
> > +struct scmi_msg_conf_get {
> > + __le32 identifier;
> > + __le32 attributes;
> > +};
> > +
> > +struct scmi_msg_pinctrl_protocol_attributes {
> > + __le32 attributes_low;
> > + __le32 attributes_high;
> > +};
> > +
> > +struct scmi_msg_pinctrl_attributes {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_resp_pinctrl_attributes {
> > + __le32 attributes;
> > + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > +};
> > +
> > +struct scmi_msg_pinctrl_list_assoc {
> > + __le32 identifier;
> > + __le32 flags;
> > + __le32 index;
> > +};
> > +
> > +struct scmi_resp_pinctrl_list_assoc {
> > + __le32 flags;
> > + __le16 array[];
> > +};
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_msg_request {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_group_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *group_pins;
> > + unsigned int nr_pins;
> > +};
> > +
> > +struct scmi_function_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *groups;
> > + unsigned int nr_groups;
> > +};
> > +
>
> A small note related to Andy remarks about directly embedding here pinctrl
> subsystem structures (like pingroup / pinfucntion) that I forgot to say
> in my reply to him.
>
> These structs above indeed are very similar to the Pinctrl ones but this is
> the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl
> subsystem which is, at the end the, one of the possible users of this layer
> (via the SCMI pinctrl driver) but not necessarily the only one in the
> future; moreover Pinctrl subsystem is not even needed at all if you think
> about a testing scenario, so I would not build up a dependency here between
> SCMI and Pinctrl by using Pinctrl structures...what if these latter change
> in the future ?
>
> All of this to just say this is fine for me as it is now :D
>
I agree with you.
What we currently have is that scmi pinctrl protocol is not bound to
pinctrl-subsystem so in case of some changes in the pinctrl - no need to
change the protocol implementation.
Also, as I mentioned in v2: I can't use pincfunction it has the following groups
definition:
const char * const *groups;

Which is meant to be constantly allocated.
So I when I try to gather list of groups in
pinctrl_scmi_get_function_groups I will receive compilation error.

Pinctrl subsystem was designed to use statically defined
pins/groups/functions so we can't use those structures on lazy
allocations.

> > +struct scmi_pin_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > +};
> > +
> > +struct scmi_pinctrl_info {
> > + u32 version;
> > + int nr_groups;
> > + int nr_functions;
> > + int nr_pins;
> > + struct scmi_group_info *groups;
> > + struct scmi_function_info *functions;
> > + struct scmi_pin_info *pins;
> > +};
> > +
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> > + struct scmi_pinctrl_info *pi)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + if (!pi)
> > + return -EINVAL;
>
> You can drop this, cannot happen given the code paths.
>

Ok. thanks.

> > +
> > + 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_get_count(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -ENODEV;
>
> You dont need to check for NULL here and nowhere else.
> You set protocol private data with set_priv at the end of protocol init
> which is called as soon as a user tries to use this protocol operations,
> so it cannot ever be NULL in any of these following ops.
>

And what if I call set_priv(ph, NULL) on init stage?
As I can see there is no check for NULL in scmi_set_protocol_priv. So
theoretically I'm able to set ph->priv = NULL. Or did I missed some check in
SCMI driver? Or maybe priv = NULL is expected scenario and I shouldn't
return error here?
> > +
> > + 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_get_count(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;
> > +}
> > +
>
> [snip]
>
> > +
> > +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 = devm_kmalloc_array(ph->dev, group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
>
> This is a lazy-allocation happening outside of the protocol init path so you
> are rigthly tracking it manually here and in protocol_deinit, BUT you
> should not use devm_ helpers, it is fuorviating even though harmless;
> just plain kmalloc/kcalloc/kfree will do. (As I said oin the reply to
> Andy I miss this previously, apologies)
>

Thank you and Andy for the observations. I will refactor.

> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group->group_pins);
> > + if (ret) {
> > + devm_kfree(ph->dev, group->group_pins);
>
> kfree
>

Ok.

> > + 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;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
>
> Ditto.
>
> > + return -EINVAL;
> > +
> > + if (selector > pi->nr_groups)
> > + return -EINVAL;
>
> selector >= ?
>

Right. Thanks.

> > +
> > + 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_get_group_pins(const struct scmi_protocol_handle *ph,
> > + u32 selector, const unsigned int **pins,
> > + unsigned int *nr_pins)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!pins || !nr_pins)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_groups)
> > + return -EINVAL;
> > +
>
> selector >= ?
>

Yes, thanks.

> > + 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 = devm_kmalloc_array(ph->dev, func->nr_groups,
> > + sizeof(*func->groups),
> > + GFP_KERNEL);
> > + if (!func->groups)
> > + return -ENOMEM;
>
> Same as above...lazy allocation properly tracked BUT do not use devm_
> variants.
>
Ok.

> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> > + func->nr_groups, func->groups);
> > + if (ret) {
> > + devm_kfree(ph->dev, func->groups);
> > + return ret;
>
> kfree
>
> > + }
> > +
> > + 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;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_functions)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + 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_get_function_groups(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + unsigned int *nr_groups,
> > + const unsigned int **groups)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!groups || !nr_groups)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_functions)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + 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_set_mux(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 group)
> > +{
> > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> > + selector);
> > +}
> > +
> > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> > + u32 selector, struct scmi_pin_info *pin)
> > +{
> > + int ret;
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + 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;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_pins)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + 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_get_name(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name)
> > +{
> > + switch (type) {
> > + case PIN_TYPE:
> > + return scmi_pinctrl_get_pin_name(ph, selector, name);
> > + case GROUP_TYPE:
> > + return scmi_pinctrl_get_group_name(ph, selector, name);
> > + case FUNCTION_TYPE:
> > + return scmi_pinctrl_get_function_name(ph, selector, name);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .get_count = scmi_pinctrl_get_count,
> > + .get_name = scmi_pinctrl_get_name,
> > + .get_group_pins = scmi_pinctrl_get_group_pins,
> > + .get_function_groups = scmi_pinctrl_get_function_groups,
> > + .set_mux = scmi_pinctrl_set_mux,
> > + .get_config = scmi_pinctrl_get_config,
> > + .set_config = scmi_pinctrl_set_config,
> > + .request_pin = scmi_pinctrl_request_pin,
> > + .free_pin = scmi_pinctrl_free_pin
> > +};
> > +
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
> > +
> > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > + sizeof(*pinfo->pins),
> > + GFP_KERNEL);
> > + if (!pinfo->pins)
> > + return -ENOMEM;
> > +
> > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > + sizeof(*pinfo->groups),
> > + GFP_KERNEL);
> > + if (!pinfo->groups)
> > + return -ENOMEM;
> > +
> > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > + sizeof(*pinfo->functions),
> > + GFP_KERNEL);
> > + if (!pinfo->functions)
> > + return -ENOMEM;
> > +
> > + pinfo->version = version;
> > +
> > + return ph->set_priv(ph, pinfo);
> > +}
> > +
> > +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> > +{
> > + int i;
> > + struct scmi_pinctrl_info *pi;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto. You never get even here if protocol init had not succesfully
> completed.
>
> > +
> > + for (i = 0; i < pi->nr_groups; i++)
> > + if (pi->groups[i].present) {
> > + devm_kfree(ph->dev, pi->groups[i].group_pins);
>
> kfree..you are managing these.
>
> > + pi->groups[i].present = false;
> > + }
> > +
> > + for (i = 0; i < pi->nr_functions; i++)
> > + if (pi->functions[i].present) {
> > + devm_kfree(ph->dev, pi->functions[i].groups);
>
> kfree..you are managing these.
>
> > + pi->functions[i].present = false;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct scmi_protocol scmi_pinctrl = {
> > + .id = SCMI_PROTOCOL_PINCTRL,
> > + .owner = THIS_MODULE,
> > + .instance_init = &scmi_pinctrl_protocol_init,
> > + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> > + .ops = &pinctrl_proto_ops,
> > +};
> > +
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> > index b3c6314bb4b8..674f949354f9 100644
> > --- a/drivers/firmware/arm_scmi/protocols.h
> > +++ b/drivers/firmware/arm_scmi/protocols.h
> > @@ -346,5 +346,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> > DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
> > DECLARE_SCMI_REGISTER_UNREGISTER(system);
> > DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
> > +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
> >
> > #endif /* _SCMI_PROTOCOLS_H */
> > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > index 0ce5746a4470..97631783a5a4 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -735,6 +735,52 @@ struct scmi_notify_ops {
> > struct notifier_block *nb);
> > };
> >
> > +enum scmi_pinctrl_selector_type {
> > + PIN_TYPE = 0,
> > + GROUP_TYPE,
> > + FUNCTION_TYPE
> > +};
> > +
> > +/**
> > + * struct scmi_pinctrl_proto_ops - represents the various operations provided
> > + * by SCMI Pinctrl Protocol
> > + *
> > + * @get_count: returns count of the registered elements in given type
> > + * @get_name: returns name by index of given type
> > + * @get_group_pins: returns the set of pins, assigned to the specified group
> > + * @get_function_groups: returns the set of groups, assigned to the specified
> > + * function
> > + * @set_mux: set muxing function for groups of pins
> > + * @get_config: returns configuration parameter for pin or group
> > + * @set_config: sets the configuration parameter for pin or group
> > + * @request_pin: aquire pin before selecting mux setting
> > + * @free_pin: frees pin, acquired by request_pin call
> > + */
> > +struct scmi_pinctrl_proto_ops {
> > + int (*get_count)(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type);
> > + int (*get_name)(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name);
> > + int (*get_group_pins)(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + const unsigned int **pins, unsigned int *nr_pins);
> > + int (*get_function_groups)(const struct scmi_protocol_handle *ph,
> > + u32 selector, unsigned int *nr_groups,
> > + const unsigned int **groups);
> > + int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
> > + u32 group);
> > + int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u8 config_type, unsigned long *config_value);
> > + int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u8 config_type, unsigned long config_value);
> > + int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> > + int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> > +};
> > +
>
> Can you move all of these before .notify_ops where all others protocol
> ops lives ? ... and rename all pinctrl_ops to match the ".<object>_<verb>"
> pattern like other ops (i.e. count_get/name_get ... instead get_count/get_name)
>

I will do this.

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

Thanks,
Oleksii.

2023-07-06 15:12:13

[permalink] [raw]

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

On Thu, Jul 06, 2023 at 02:09:38PM +0000, Oleksii Moisieiev wrote:
> Hi Cristian,
>

Hi Oleksii,

> Sorry for late answer.
> Please see below.
>

No worries, not late at all.

> On Mon, Jul 03, 2023 at 11:16:47AM +0100, Cristian Marussi wrote:
> > On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> > > scmi: Introduce pinctrl SCMI protocol driver
> > >
> >
> > Hi Oleksii,
> >
> > spurios line above.
>
> Yep thanks, I will remove.
>
> >
> > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > excluding GPIO support. All pinctrl related callbacks and operations
> > > are exposed in the include/linux/scmi_protocol.h
> > >
> >
> > As Andy said already, you can drop the second sentence here, but I would
> > ALSO drop the GPIO part in the first sentence, since there is nothing
> > specific to GPIO in the SCMI spec and this patch is about the SCMI protocol
> > not the pinctrl driver.
> >
>
> I've added few words about GPIO because in v2 series Michal Simek asked
> about it: https://lore.kernel.org/linux-arm-kernel/[email protected]/
> So I've decided to mention that there is still no GPIO support in the
> commit message to avoid this type of questions in future. But I agree
> that the commit message looks weird and will try to rephrase it.
>

Yes I remember that and I understand why you want to mention this, what
I am saying is that anyway is NOT something related to the SCMI Pinctrl
spec AFAIU (I may be wrong): I mean GPIO support is something you can
build on top of Pinctrl SCMI spec and driver NOT something that has
still to be added to the spec right ? and this patch is about supporting
the new SCMI protocol, so I certainly agree that can be fine to point
out that GPIO support is missing, just maybe this is a comment more
appropriate to be added to the Pinctrl SCMI driver than to the Pinctrl
SCMI protocol layer...(but maybe the Pinctrl subsys maintainer will
disagree on this :P)

> > > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > > ---
> > > MAINTAINERS | 6 +
> > > drivers/firmware/arm_scmi/Makefile | 2 +-
> > > drivers/firmware/arm_scmi/driver.c | 2 +
> > > drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++
> > > drivers/firmware/arm_scmi/protocols.h | 1 +
> > > include/linux/scmi_protocol.h | 47 ++
> > > 6 files changed, 893 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 0dab9737ec16..297b2512963d 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h
> > > F: include/trace/events/scmi.h
> > > F: include/uapi/linux/virtio_scmi.h
> > >
> > > +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
> >
> > SCPI is a leftover here I suppose...
> >
>
> Thanks. I'll fix it.
>
> > > +M: Oleksii Moisieiev <[email protected]>
> > > +L: [email protected]
> > > +S: Maintained
> > > +F: drivers/firmware/arm_scmi/pinctrl.c
> > > +
> > > SYSTEM RESET/SHUTDOWN DRIVERS
> > > M: Sebastian Reichel <[email protected]>
> > > L: [email protected]
> > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > > index b31d78fa66cc..603430ec0bfe 100644
> > > --- a/drivers/firmware/arm_scmi/Makefile
> > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> > >
> > > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index 5be931a07c84..a9fd337b9596 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > @@ -3025,6 +3025,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);
> > > }
> > > @@ -3042,6 +3043,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..fc0fcc26dfb6
> > > --- /dev/null
> > > +++ b/drivers/firmware/arm_scmi/pinctrl.c
> > > @@ -0,0 +1,836 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * System Control and Management Interface (SCMI) Pinctrl Protocol
> > > + *
> > > + * Copyright (C) 2023 EPAM
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "protocols.h"
> > > +
> > > +#define 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 config_value;
> > > +};
> > > +
> > > +struct scmi_msg_conf_get {
> > > + __le32 identifier;
> > > + __le32 attributes;
> > > +};
> > > +
> > > +struct scmi_msg_pinctrl_protocol_attributes {
> > > + __le32 attributes_low;
> > > + __le32 attributes_high;
> > > +};
> > > +
> > > +struct scmi_msg_pinctrl_attributes {
> > > + __le32 identifier;
> > > + __le32 flags;
> > > +};
> > > +
> > > +struct scmi_resp_pinctrl_attributes {
> > > + __le32 attributes;
> > > + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > > +};
> > > +
> > > +struct scmi_msg_pinctrl_list_assoc {
> > > + __le32 identifier;
> > > + __le32 flags;
> > > + __le32 index;
> > > +};
> > > +
> > > +struct scmi_resp_pinctrl_list_assoc {
> > > + __le32 flags;
> > > + __le16 array[];
> > > +};
> > > +
> > > +struct scmi_msg_func_set {
> > > + __le32 identifier;
> > > + __le32 function_id;
> > > + __le32 flags;
> > > +};
> > > +
> > > +struct scmi_msg_request {
> > > + __le32 identifier;
> > > + __le32 flags;
> > > +};
> > > +
> > > +struct scmi_group_info {
> > > + bool present;
> > > + char name[SCMI_MAX_STR_SIZE];
> > > + unsigned int *group_pins;
> > > + unsigned int nr_pins;
> > > +};
> > > +
> > > +struct scmi_function_info {
> > > + bool present;
> > > + char name[SCMI_MAX_STR_SIZE];
> > > + unsigned int *groups;
> > > + unsigned int nr_groups;
> > > +};
> > > +
> >
> > A small note related to Andy remarks about directly embedding here pinctrl
> > subsystem structures (like pingroup / pinfucntion) that I forgot to say
> > in my reply to him.
> >
> > These structs above indeed are very similar to the Pinctrl ones but this is
> > the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl
> > subsystem which is, at the end the, one of the possible users of this layer
> > (via the SCMI pinctrl driver) but not necessarily the only one in the
> > future; moreover Pinctrl subsystem is not even needed at all if you think
> > about a testing scenario, so I would not build up a dependency here between
> > SCMI and Pinctrl by using Pinctrl structures...what if these latter change
> > in the future ?
> >
> > All of this to just say this is fine for me as it is now :D
> >
> I agree with you.
> What we currently have is that scmi pinctrl protocol is not bound to
> pinctrl-subsystem so in case of some changes in the pinctrl - no need to
> change the protocol implementation.
> Also, as I mentioned in v2: I can't use pincfunction it has the following groups
> definition:
> const char * const *groups;
>
> Which is meant to be constantly allocated.
> So I when I try to gather list of groups in
> pinctrl_scmi_get_function_groups I will receive compilation error.
>
> Pinctrl subsystem was designed to use statically defined
> pins/groups/functions so we can't use those structures on lazy
> allocations.
>

Indeed, I forgot that additional reason.

>
> > > +struct scmi_pin_info {
> > > + bool present;
> > > + char name[SCMI_MAX_STR_SIZE];
> > > +};
> > > +
> > > +struct scmi_pinctrl_info {
> > > + u32 version;
> > > + int nr_groups;
> > > + int nr_functions;
> > > + int nr_pins;
> > > + struct scmi_group_info *groups;
> > > + struct scmi_function_info *functions;
> > > + struct scmi_pin_info *pins;
> > > +};
> > > +
> > > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> > > + struct scmi_pinctrl_info *pi)
> > > +{
> > > + int ret;
> > > + struct scmi_xfer *t;
> > > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > > +
> > > + if (!pi)
> > > + return -EINVAL;
> >
> > You can drop this, cannot happen given the code paths.
> >
>
> Ok. thanks.
>
> > > +
> > > + 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_get_count(const struct scmi_protocol_handle *ph,
> > > + enum scmi_pinctrl_selector_type type)
> > > +{
> > > + struct scmi_pinctrl_info *pi;
> > > +
> > > + pi = ph->get_priv(ph);
> > > + if (!pi)
> > > + return -ENODEV;
> >
> > You dont need to check for NULL here and nowhere else.
> > You set protocol private data with set_priv at the end of protocol init
> > which is called as soon as a user tries to use this protocol operations,
> > so it cannot ever be NULL in any of these following ops.
> >
>
> And what if I call set_priv(ph, NULL) on init stage?
> As I can see there is no check for NULL in scmi_set_protocol_priv. So
> theoretically I'm able to set ph->priv = NULL. Or did I missed some check in
> SCMI driver? Or maybe priv = NULL is expected scenario and I shouldn't
> return error here?

Well, you are right that you could set periv to NULL, but the whole
point of set_priv/get_priv helpers are to help you protocol-writer to
store your private data at init for future usage while processing the
protocol operations that you, protocol-writer, are implementing; the
idea of all of this 'dancing' around protocol_handle was to ease the
developement of protocols by exposing a limited, common and well
controlled interface to use to build/send messages (ph->xops) while
hiding some internals related to protocol stack init that are handled
by the core for you.

The priv data are only set and get optionally by You depending on the
need of the protocol, so unless you can dynamically set, at runtime, priv
to NULL or not-NULL depending on the outcome of the init, you should very
well know at coding time if your priv could possibly be ever NULL or it
cannot be NULL at all (like in this case it seems to me): so the check
seemed to me redundant...

...clearly, beside trying to help the protocol devel, the SCMI core
protocol 'framework' cannot prevent you from shooting yourself in the
foot if you want :P

Thanks,
Cristian

2023-07-06 15:48:31

[permalink] [raw]

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

Hi Cristian,

On Thu, Jul 06, 2023 at 03:42:34PM +0100, Cristian Marussi wrote:
> On Thu, Jul 06, 2023 at 02:09:38PM +0000, Oleksii Moisieiev wrote:
> > Hi Cristian,
> >
>
> Hi Oleksii,
>
> > Sorry for late answer.
> > Please see below.
> >
>
> No worries, not late at all.
>
> > On Mon, Jul 03, 2023 at 11:16:47AM +0100, Cristian Marussi wrote:
> > > On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> > > > scmi: Introduce pinctrl SCMI protocol driver
> > > >
> > >
> > > Hi Oleksii,
> > >
> > > spurios line above.
> >
> > Yep thanks, I will remove.
> >
> > >
> > > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > > excluding GPIO support. All pinctrl related callbacks and operations
> > > > are exposed in the include/linux/scmi_protocol.h
> > > >
> > >
> > > As Andy said already, you can drop the second sentence here, but I would
> > > ALSO drop the GPIO part in the first sentence, since there is nothing
> > > specific to GPIO in the SCMI spec and this patch is about the SCMI protocol
> > > not the pinctrl driver.
> > >
> >
> > I've added few words about GPIO because in v2 series Michal Simek asked
> > about it: https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/[email protected]/__;!!GF_29dbcQIUBPA!1eit_iJDFpGMDrWcBk1hej0zgnDQilbjCvnU-4h-t8eL2GbpNrvXpdWEo7pttPI8rMae2gJAfCgRrkeiq5Qrr7OeqxXTiQ$ [lore[.]kernel[.]org]
> > So I've decided to mention that there is still no GPIO support in the
> > commit message to avoid this type of questions in future. But I agree
> > that the commit message looks weird and will try to rephrase it.
> >
>
> Yes I remember that and I understand why you want to mention this, what
> I am saying is that anyway is NOT something related to the SCMI Pinctrl
> spec AFAIU (I may be wrong): I mean GPIO support is something you can
> build on top of Pinctrl SCMI spec and driver NOT something that has
> still to be added to the spec right ? and this patch is about supporting
> the new SCMI protocol, so I certainly agree that can be fine to point
> out that GPIO support is missing, just maybe this is a comment more
> appropriate to be added to the Pinctrl SCMI driver than to the Pinctrl
> SCMI protocol layer...(but maybe the Pinctrl subsys maintainer will
> disagree on this :P)

Yeah, you're right. Just looked into the spec to ensure. I will rework this part.
Pinctrl maintainer will definitely disagree because GPIO is a separate
subsystem.

>
> > > > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > > > ---
> > > > MAINTAINERS | 6 +
> > > > drivers/firmware/arm_scmi/Makefile | 2 +-
> > > > drivers/firmware/arm_scmi/driver.c | 2 +
> > > > drivers/firmware/arm_scmi/pinctrl.c | 836 ++++++++++++++++++++++++++
> > > > drivers/firmware/arm_scmi/protocols.h | 1 +
> > > > include/linux/scmi_protocol.h | 47 ++
> > > > 6 files changed, 893 insertions(+), 1 deletion(-)
> > > > create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 0dab9737ec16..297b2512963d 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h
> > > > F: include/trace/events/scmi.h
> > > > F: include/uapi/linux/virtio_scmi.h
> > > >
> > > > +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
> > >
> > > SCPI is a leftover here I suppose...
> > >
> >
> > Thanks. I'll fix it.

[snip]

> > > > +
> > >
> > > A small note related to Andy remarks about directly embedding here pinctrl
> > > subsystem structures (like pingroup / pinfucntion) that I forgot to say
> > > in my reply to him.
> > >
> > > These structs above indeed are very similar to the Pinctrl ones but this is
> > > the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl
> > > subsystem which is, at the end the, one of the possible users of this layer
> > > (via the SCMI pinctrl driver) but not necessarily the only one in the
> > > future; moreover Pinctrl subsystem is not even needed at all if you think
> > > about a testing scenario, so I would not build up a dependency here between
> > > SCMI and Pinctrl by using Pinctrl structures...what if these latter change
> > > in the future ?
> > >
> > > All of this to just say this is fine for me as it is now :D
> > >
> > I agree with you.
> > What we currently have is that scmi pinctrl protocol is not bound to
> > pinctrl-subsystem so in case of some changes in the pinctrl - no need to
> > change the protocol implementation.
> > Also, as I mentioned in v2: I can't use pincfunction it has the following groups
> > definition:
> > const char * const *groups;
> >
> > Which is meant to be constantly allocated.
> > So I when I try to gather list of groups in
> > pinctrl_scmi_get_function_groups I will receive compilation error.
> >
> > Pinctrl subsystem was designed to use statically defined
> > pins/groups/functions so we can't use those structures on lazy
> > allocations.
> >
>
> Indeed, I forgot that additional reason.
>
> >
> > > > +struct scmi_pin_info {
> > > > + bool present;
> > > > + char name[SCMI_MAX_STR_SIZE];
> > > > +};
> > > > +
> > > > +struct scmi_pinctrl_info {
> > > > + u32 version;
> > > > + int nr_groups;
> > > > + int nr_functions;
> > > > + int nr_pins;
> > > > + struct scmi_group_info *groups;
> > > > + struct scmi_function_info *functions;
> > > > + struct scmi_pin_info *pins;
> > > > +};
> > > > +
> > > > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> > > > + struct scmi_pinctrl_info *pi)
> > > > +{
> > > > + int ret;
> > > > + struct scmi_xfer *t;
> > > > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > > > +
> > > > + if (!pi)
> > > > + return -EINVAL;
> > >
> > > You can drop this, cannot happen given the code paths.
> > >
> >
> > Ok. thanks.
> >
> > > > +
> > > > + 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_get_count(const struct scmi_protocol_handle *ph,
> > > > + enum scmi_pinctrl_selector_type type)
> > > > +{
> > > > + struct scmi_pinctrl_info *pi;
> > > > +
> > > > + pi = ph->get_priv(ph);
> > > > + if (!pi)
> > > > + return -ENODEV;
> > >
> > > You dont need to check for NULL here and nowhere else.
> > > You set protocol private data with set_priv at the end of protocol init
> > > which is called as soon as a user tries to use this protocol operations,
> > > so it cannot ever be NULL in any of these following ops.
> > >
> >
> > And what if I call set_priv(ph, NULL) on init stage?
> > As I can see there is no check for NULL in scmi_set_protocol_priv. So
> > theoretically I'm able to set ph->priv = NULL. Or did I missed some check in
> > SCMI driver? Or maybe priv = NULL is expected scenario and I shouldn't
> > return error here?
>
> Well, you are right that you could set periv to NULL, but the whole
> point of set_priv/get_priv helpers are to help you protocol-writer to
> store your private data at init for future usage while processing the
> protocol operations that you, protocol-writer, are implementing; the
> idea of all of this 'dancing' around protocol_handle was to ease the
> developement of protocols by exposing a limited, common and well
> controlled interface to use to build/send messages (ph->xops) while
> hiding some internals related to protocol stack init that are handled
> by the core for you.
>
> The priv data are only set and get optionally by You depending on the
> need of the protocol, so unless you can dynamically set, at runtime, priv
> to NULL or not-NULL depending on the outcome of the init, you should very
> well know at coding time if your priv could possibly be ever NULL or it
> cannot be NULL at all (like in this case it seems to me): so the check
> seemed to me redundant...
>
> ...clearly, beside trying to help the protocol devel, the SCMI core
> protocol 'framework' cannot prevent you from shooting yourself in the
> foot if you want :P
>
> Thanks,
> Cristian
>

That's why I was puzzled. Trying to shoot myself in the knee is what I've mostly
tried while written unit tests. Probably just need to write less tests
:).
I'll remove checks.

Best regards,
Oleksii.

2023-07-20 14:50:34

[permalink] [raw]

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

Hi Andy,

[email protected] writes:

> Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
>> scmi-pinctrl driver implements pinctrl driver interface and using
>> SCMI protocol to redirect messages from pinctrl subsystem SDK to
>> SCP firmware, which does the changes in HW.
>>
>> This setup expects SCP firmware (or similar system, such as ATF)
>> to be installed on the platform, which implements pinctrl driver
>> for the specific platform.
>>
>> SCMI-Pinctrl driver should be configured from the device-tree and uses
>> generic device-tree mappings for the configuration.

[snip]

> ...
>
>> +error:
>
> Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto.
>
>> + devm_kfree(pmx->dev, pmx->functions[selector].groups);
>
> Red Flag. Please, elaborate.
>

Thank you for the review.
I did some research regarding this and now I'm confused. Could you
please explain to me why it's a red flag?
IIUC devm_alloc/free functions are the calls to the resource-managed
alloc/free command, which is bound to the device.
pinctrl-scmi driver does devm_pinctrl_register_and_init which does
devres_alloc and doesn't open devres_group like
scmi_alloc_init_protocol_instance (thanks to Cristian detailed
explanation).

As was mentioned in Documentation/driver-api/driver-model/devres.rst:

```
No matter what, all devres entries are released on driver detach. On
release, the associated release function is invoked and then the
devres entry is freed.
```

Also there is devm_pinctrl_get call listed in the managed interfaces.

My understanding is that all resources, bound to the particular device
will be freed on driver detach.

Also I found some examples of using devm_alloc/free like from dt_node_to_map
call in pinctrl-simple.c driver.

I agree that I need to implement .remove callback with proper cleanup,
but why can't I use devm_* here?
Maybe I've misunderstood your point.

--
Thanks,
Oleksii

2023-07-20 16:34:53

[permalink] [raw]

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

On Thu, Jul 20, 2023 at 4:40 PM Oleksii Moisieiev
<[email protected]> wrote:
> [email protected] writes:
> > Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:

...

> >> + devm_kfree(pmx->dev, pmx->functions[selector].groups);
> >
> > Red Flag. Please, elaborate.
>
> Thank you for the review.
> I did some research regarding this and now I'm confused. Could you
> please explain to me why it's a red flag?
> IIUC devm_alloc/free functions are the calls to the resource-managed
> alloc/free command, which is bound to the device.
> pinctrl-scmi driver does devm_pinctrl_register_and_init which does
> devres_alloc and doesn't open devres_group like
> scmi_alloc_init_protocol_instance (thanks to Cristian detailed
> explanation).
>
> As was mentioned in Documentation/driver-api/driver-model/devres.rst:
>
> ```
> No matter what, all devres entries are released on driver detach. On
> release, the associated release function is invoked and then the
> devres entry is freed.
> ```

Precisely. So, why do you intervene in this?

> Also there is devm_pinctrl_get call listed in the managed interfaces.
>
> My understanding is that all resources, bound to the particular device
> will be freed on driver detach.
>
> Also I found some examples of using devm_alloc/free like from dt_node_to_map
> call in pinctrl-simple.c driver.
>
> I agree that I need to implement .remove callback with proper cleanup,
> but why can't I use devm_* here?

You can use devm_*(), but what's the point if you call release
yourself? That's quite a red flag usually shows a bigger issue
(misunderstanding of the objects lifetimes and their interaction).

> Maybe I've misunderstood your point.

--
With Best Regards,
Andy Shevchenko

2023-07-20 18:20:19