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

[permalink] [raw]

Subject: [PATCH v6 1/4] firmware: arm_scmi: introduce helper get_max_msg_size

From: Peng Fan <[email protected]>

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

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

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 2709598f3008..415e6f510057 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1488,6 +1488,20 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
return ret;
}

+/**
+ * scmi_common_get_max_msg_size - Get maximum message size
+ * @ph: A protocol handle reference.
+ *
+ * Return: Maximum message size for the current protocol.
+ */
+static int scmi_common_get_max_msg_size(const struct scmi_protocol_handle *ph)
+{
+ const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+ struct scmi_info *info = handle_to_scmi_info(pi->handle);
+
+ return info->desc->max_msg_size;
+}
+
/**
* struct scmi_iterator - Iterator descriptor
* @msg: A reference to the message TX buffer; filled by @prepare_message with
@@ -1799,6 +1813,7 @@ static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,

static const struct scmi_proto_helpers_ops helpers_ops = {
.extended_name_get = scmi_common_extended_name_get,
+ .get_max_msg_size = scmi_common_get_max_msg_size,
.iter_response_init = scmi_iterator_init,
.iter_response_run = scmi_iterator_run,
.protocol_msg_check = scmi_protocol_msg_check,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 317d3fb32676..3e91536a77a3 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -258,6 +258,7 @@ struct scmi_fc_info {
* @fastchannel_init: A common helper used to initialize FC descriptors by
* gathering FC descriptions from the SCMI platform server.
* @fastchannel_db_ring: A common helper to ring a FC doorbell.
+ * @get_max_msg_size: A common helper to get the maximum message size.
*/
struct scmi_proto_helpers_ops {
int (*extended_name_get)(const struct scmi_protocol_handle *ph,
@@ -277,6 +278,7 @@ struct scmi_proto_helpers_ops {
struct scmi_fc_db_info **p_db,
u32 *rate_limit);
void (*fastchannel_db_ring)(struct scmi_fc_db_info *db);
+ int (*get_max_msg_size)(const struct scmi_protocol_handle *ph);
};

/**

--
2.37.1

2024-03-23 12:08:11

[permalink] [raw]

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

From: Peng Fan <[email protected]>

Add SCMI v3.2 pinctrl protocol bindings and example.

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

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

+ protocol@19:
+ type: object
+ allOf:
+ - $ref: '#/$defs/protocol-node'
+ - $ref: /schemas/pinctrl/pinctrl.yaml
+
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ const: 0x19
+
+ patternProperties:
+ '-pins$':
+ type: object
+ allOf:
+ - $ref: /schemas/pinctrl/pincfg-node.yaml#
+ - $ref: /schemas/pinctrl/pinmux-node.yaml#
+ unevaluatedProperties: false
+
+ description:
+ A pin multiplexing sub-node describes how to configure a
+ set of pins in some desired function.
+ A single sub-node may define several pin configurations.
+ This sub-node is using the default pinctrl bindings to configure
+ pin multiplexing and using SCMI protocol to apply a specified
+ configuration.
+
+ required:
+ - reg
+
additionalProperties: false

$defs:
@@ -401,6 +432,25 @@ examples:
scmi_powercap: protocol@18 {
reg = <0x18>;
};
+
+ scmi_pinctrl: protocol@19 {
+ reg = <0x19>;
+
+ i2c2-pins {
+ groups = "g_i2c2_a", "g_i2c2_b";
+ function = "f_i2c2";
+ };
+
+ mdio-pins {
+ groups = "g_avb_mdio";
+ drive-strength = <24>;
+ };
+
+ keys_pins: keys-pins {
+ pins = "gpio_5_17", "gpio_5_20", "gpio_5_22", "gpio_2_1";
+ bias-pull-up;
+ };
+ };
};
};

--
2.37.1

2024-03-23 12:08:30

[permalink] [raw]

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

From: Peng Fan <[email protected]>

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

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

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..8e3874ff1544 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-y += pinctrl.o
scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)

obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 415e6f510057..ac2d4b19727c 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
scmi_voltage_register();
scmi_system_register();
scmi_powercap_register();
+ scmi_pinctrl_register();

return platform_driver_register(&scmi_driver);
}
@@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
scmi_voltage_unregister();
scmi_system_unregister();
scmi_powercap_unregister();
+ scmi_pinctrl_unregister();

scmi_transports_exit();

diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
new file mode 100644
index 000000000000..87d9b89cab13
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,921 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Pinctrl Protocol
+ *
+ * Copyright (C) 2024 EPAM
+ * Copyright 2024 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include "common.h"
+#include "protocols.h"
+
+/* Updated only after ALL the mandatory features for that version are merged */
+#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
+
+#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
+#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
+#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
+
+#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
+#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
+
+#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
+#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
+
+#define CONFIG_FLAG_MASK GENMASK(19, 18)
+#define SELECTOR_MASK GENMASK(17, 16)
+#define SKIP_CONFIGS_MASK GENMASK(15, 8)
+#define CONFIG_TYPE_MASK GENMASK(7, 0)
+
+enum scmi_pinctrl_protocol_cmd {
+ PINCTRL_ATTRIBUTES = 0x3,
+ PINCTRL_LIST_ASSOCIATIONS = 0x4,
+ PINCTRL_SETTINGS_GET = 0x5,
+ PINCTRL_SETTINGS_CONFIGURE = 0x6,
+ PINCTRL_REQUEST = 0x7,
+ PINCTRL_RELEASE = 0x8,
+ PINCTRL_NAME_GET = 0x9,
+ PINCTRL_SET_PERMISSIONS = 0xa
+};
+
+struct scmi_msg_settings_conf {
+ __le32 identifier;
+ __le32 function_id;
+ __le32 attributes;
+ __le32 configs[];
+};
+
+struct scmi_msg_settings_get {
+ __le32 identifier;
+ __le32 attributes;
+};
+
+struct scmi_resp_settings_get {
+ __le32 function_selected;
+ __le32 num_configs;
+ __le32 configs[];
+};
+
+struct scmi_msg_pinctrl_protocol_attributes {
+ __le32 attributes_low;
+ __le32 attributes_high;
+};
+
+struct scmi_msg_pinctrl_attributes {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_resp_pinctrl_attributes {
+ __le32 attributes;
+ u8 name[SCMI_SHORT_NAME_MAX_SIZE];
+};
+
+struct scmi_msg_pinctrl_list_assoc {
+ __le32 identifier;
+ __le32 flags;
+ __le32 index;
+};
+
+struct scmi_resp_pinctrl_list_assoc {
+ __le32 flags;
+ __le16 array[];
+};
+
+struct scmi_msg_func_set {
+ __le32 identifier;
+ __le32 function_id;
+ __le32 flags;
+};
+
+struct scmi_msg_request {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_group_info {
+ char name[SCMI_MAX_STR_SIZE];
+ bool present;
+ u32 *group_pins;
+ u32 nr_pins;
+};
+
+struct scmi_function_info {
+ char name[SCMI_MAX_STR_SIZE];
+ bool present;
+ u32 *groups;
+ u32 nr_groups;
+};
+
+struct scmi_pin_info {
+ char name[SCMI_MAX_STR_SIZE];
+ bool present;
+};
+
+struct scmi_pinctrl_info {
+ u32 version;
+ int nr_groups;
+ int nr_functions;
+ int nr_pins;
+ struct scmi_group_info *groups;
+ struct scmi_function_info *functions;
+ struct scmi_pin_info *pins;
+};
+
+static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
+ struct scmi_pinctrl_info *pi)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_protocol_attributes *attr;
+
+ ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
+ &t);
+ if (ret)
+ return ret;
+
+ attr = t->rx.buf;
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
+ pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
+ pi->nr_pins = GET_PINS_NR(attr->attributes_low);
+ }
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ switch (type) {
+ case PIN_TYPE:
+ return pi->nr_pins;
+ case GROUP_TYPE:
+ return pi->nr_groups;
+ case FUNCTION_TYPE:
+ return pi->nr_functions;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ int value;
+
+ value = scmi_pinctrl_count_get(ph, type);
+ if (value < 0)
+ return value;
+
+ if (identifier >= value)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type,
+ u32 selector, char *name,
+ u32 *n_elems)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_attributes *tx;
+ struct scmi_resp_pinctrl_attributes *rx;
+ u32 ext_name_flag;
+
+ if (!name)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
+ sizeof(*rx), &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ rx = t->rx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ tx->flags = cpu_to_le32(type);
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ if (n_elems)
+ *n_elems = NUM_ELEMS(rx->attributes);
+
+ strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
+
+ ext_name_flag = EXT_NAME_FLAG(rx->attributes);
+ } else
+ ext_name_flag = 0;
+
+ 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)
+ ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
+ (u32 *)&type, name,
+ SCMI_MAX_STR_SIZE);
+ return ret;
+}
+
+struct scmi_pinctrl_ipriv {
+ u32 selector;
+ enum scmi_pinctrl_selector_type type;
+ u32 *array;
+};
+
+static void iter_pinctrl_assoc_prepare_message(void *message,
+ u32 desc_index,
+ const void *priv)
+{
+ struct scmi_msg_pinctrl_list_assoc *msg = message;
+ const struct scmi_pinctrl_ipriv *p = priv;
+
+ msg->identifier = cpu_to_le32(p->selector);
+ msg->flags = cpu_to_le32(p->type);
+ /* 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, u32 *array)
+{
+ int ret;
+ void *iter;
+ struct scmi_iterator_ops ops = {
+ .prepare_message = iter_pinctrl_assoc_prepare_message,
+ .update_state = iter_pinctrl_assoc_update_state,
+ .process_response = iter_pinctrl_assoc_process_response,
+ };
+ struct scmi_pinctrl_ipriv ipriv = {
+ .selector = selector,
+ .type = type,
+ .array = array,
+ };
+
+ if (!array || !size || type == PIN_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ iter = ph->hops->iter_response_init(ph, &ops, size,
+ PINCTRL_LIST_ASSOCIATIONS,
+ sizeof(struct scmi_msg_pinctrl_list_assoc),
+ &ipriv);
+
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
+
+ return ph->hops->iter_response_run(iter);
+}
+
+struct scmi_settings_get_ipriv {
+ u32 selector;
+ enum scmi_pinctrl_selector_type type;
+ u32 flag;
+ enum scmi_pinctrl_conf_type *config_types;
+ u32 *config_values;
+};
+
+static void
+iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
+ const void *priv)
+{
+ struct scmi_msg_settings_get *msg = message;
+ const struct scmi_settings_get_ipriv *p = priv;
+ u32 attributes;
+
+ attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
+ FIELD_PREP(SELECTOR_MASK, p->type);
+
+ if (p->flag == 1)
+ attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
+ else if (!p->flag)
+ attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
+
+ msg->attributes = cpu_to_le32(attributes);
+ msg->identifier = cpu_to_le32(p->selector);
+}
+
+static int
+iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
+ const void *response, void *priv)
+{
+ const struct scmi_resp_settings_get *r = response;
+ struct scmi_settings_get_ipriv *p = priv;
+
+ if (p->flag == 1) {
+ st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
+ st->num_remaining = le32_get_bits(r->num_configs,
+ GENMASK(31, 24));
+ } else {
+ st->num_returned = 1;
+ st->num_remaining = 0;
+ }
+
+ return 0;
+}
+
+static int
+iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
+ const void *response,
+ struct scmi_iterator_state *st,
+ void *priv)
+{
+ const struct scmi_resp_settings_get *r = response;
+ struct scmi_settings_get_ipriv *p = priv;
+
+ if (!p->flag) {
+ if (p->config_types[0] !=
+ le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
+ return -EINVAL;
+ } else if (p->flag == 1) {
+ p->config_types[st->desc_index + st->loop_idx] =
+ le32_get_bits(r->configs[st->loop_idx * 2],
+ GENMASK(7, 0));
+ } else if (p->flag == 2) {
+ return 0;
+ }
+
+ p->config_values[st->desc_index + st->loop_idx] =
+ le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
+
+ return 0;
+}
+
+static int
+scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ enum scmi_pinctrl_conf_type config_type,
+ u32 *config_value)
+{
+ int ret;
+ void *iter;
+ struct scmi_iterator_ops ops = {
+ .prepare_message = iter_pinctrl_settings_get_prepare_message,
+ .update_state = iter_pinctrl_settings_get_update_state,
+ .process_response = iter_pinctrl_settings_get_process_response,
+ };
+ struct scmi_settings_get_ipriv ipriv = {
+ .selector = selector,
+ .type = type,
+ .flag = 0,
+ .config_types = &config_type,
+ .config_values = config_value,
+ };
+
+ if (!config_value || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
+ sizeof(struct scmi_msg_settings_get),
+ &ipriv);
+
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
+
+ return ph->hops->iter_response_run(iter);
+}
+
+static int
+scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u32 nr_configs,
+ enum scmi_pinctrl_conf_type *config_type,
+ u32 *config_value)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_settings_conf *tx;
+ u32 attributes;
+ int ret, i;
+ u32 configs_in_chunk, conf_num = 0;
+ u32 chunk;
+ int max_msg_size = ph->hops->get_max_msg_size(ph);
+
+ if (!config_type || !config_value || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
+ while (conf_num < nr_configs) {
+ chunk = (nr_configs - conf_num > configs_in_chunk) ?
+ configs_in_chunk : nr_configs - conf_num;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
+ sizeof(*tx) +
+ chunk * 2 * sizeof(__le32),
+ 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ attributes = FIELD_PREP(GENMASK(1, 0), type) |
+ FIELD_PREP(GENMASK(9, 2), chunk);
+ tx->attributes = cpu_to_le32(attributes);
+
+ for (i = 0; i < chunk; i++) {
+ tx->configs[i * 2] =
+ cpu_to_le32(config_type[conf_num + i]);
+ tx->configs[i * 2 + 1] =
+ cpu_to_le32(config_value[conf_num + i]);
+ }
+
+ ret = ph->xops->do_xfer(ph, t);
+
+ ph->xops->xfer_put(ph, t);
+
+ if (ret)
+ break;
+
+ conf_num += chunk;
+ }
+
+ return ret;
+}
+
+static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
+ u32 group,
+ enum scmi_pinctrl_selector_type type,
+ u32 function_id)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_settings_conf *tx;
+ u32 attributes;
+
+ ret = scmi_pinctrl_validate_id(ph, group, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
+ sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(group);
+ tx->function_id = cpu_to_le32(function_id);
+ attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
+ tx->attributes = cpu_to_le32(attributes);
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_request(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);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(identifier);
+ tx->flags = cpu_to_le32(type);
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
+ u32 pin)
+{
+ return scmi_pinctrl_request(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_request *tx;
+
+ if (type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(identifier);
+ tx->flags = cpu_to_le32(type);
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
+{
+ return scmi_pinctrl_free(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_group_info *group)
+{
+ int ret;
+
+ if (!group)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
+ group->name,
+ &group->nr_pins);
+ if (ret)
+ return ret;
+
+ if (!group->nr_pins) {
+ dev_err(ph->dev, "Group %d has 0 elements", selector);
+ return -ENODATA;
+ }
+
+ group->group_pins = kmalloc_array(group->nr_pins,
+ sizeof(*group->group_pins),
+ GFP_KERNEL);
+ if (!group->group_pins)
+ return -ENOMEM;
+
+ ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
+ group->nr_pins, group->group_pins);
+ if (ret) {
+ kfree(group->group_pins);
+ return ret;
+ }
+
+ group->present = true;
+ return 0;
+}
+
+static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!name)
+ return -EINVAL;
+
+ if (selector >= pi->nr_groups)
+ return -EINVAL;
+
+ if (!pi->groups[selector].present) {
+ int ret;
+
+ ret = scmi_pinctrl_get_group_info(ph, selector,
+ &pi->groups[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->groups[selector].name;
+
+ return 0;
+}
+
+static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
+ u32 selector, const u32 **pins,
+ u32 *nr_pins)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!pins || !nr_pins)
+ return -EINVAL;
+
+ if (selector >= pi->nr_groups)
+ return -EINVAL;
+
+ if (!pi->groups[selector].present) {
+ int ret;
+
+ ret = scmi_pinctrl_get_group_info(ph, selector,
+ &pi->groups[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *pins = pi->groups[selector].group_pins;
+ *nr_pins = pi->groups[selector].nr_pins;
+
+ return 0;
+}
+
+static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_function_info *func)
+{
+ int ret;
+
+ if (!func)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
+ func->name,
+ &func->nr_groups);
+ if (ret)
+ return ret;
+
+ if (!func->nr_groups) {
+ dev_err(ph->dev, "Function %d has 0 elements", selector);
+ return -ENODATA;
+ }
+
+ func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
+ GFP_KERNEL);
+ if (!func->groups)
+ return -ENOMEM;
+
+ ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
+ func->nr_groups, func->groups);
+ if (ret) {
+ kfree(func->groups);
+ return ret;
+ }
+
+ func->present = true;
+ return 0;
+}
+
+static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!name)
+ return -EINVAL;
+
+ if (selector >= pi->nr_functions)
+ return -EINVAL;
+
+ if (!pi->functions[selector].present) {
+ int ret;
+
+ ret = scmi_pinctrl_get_function_info(ph, selector,
+ &pi->functions[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->functions[selector].name;
+ return 0;
+}
+
+static int
+scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
+ u32 selector, u32 *nr_groups,
+ const u32 **groups)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!groups || !nr_groups)
+ return -EINVAL;
+
+ if (selector >= pi->nr_functions)
+ return -EINVAL;
+
+ if (!pi->functions[selector].present) {
+ int ret;
+
+ ret = scmi_pinctrl_get_function_info(ph, selector,
+ &pi->functions[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *groups = pi->functions[selector].groups;
+ *nr_groups = pi->functions[selector].nr_groups;
+
+ return 0;
+}
+
+static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
+ u32 selector, u32 group)
+{
+ return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, selector);
+}
+
+static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
+ u32 selector, struct scmi_pin_info *pin)
+{
+ int ret;
+
+ if (!pin)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
+ pin->name, NULL);
+ if (ret)
+ return ret;
+
+ pin->present = true;
+ return 0;
+}
+
+static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ if (!name)
+ return -EINVAL;
+
+ if (selector >= pi->nr_pins)
+ return -EINVAL;
+
+ if (!pi->pins[selector].present) {
+ int ret;
+
+ ret = scmi_pinctrl_get_pin_info(ph, selector,
+ &pi->pins[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->pins[selector].name;
+
+ return 0;
+}
+
+static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name)
+{
+ switch (type) {
+ case PIN_TYPE:
+ return scmi_pinctrl_get_pin_name(ph, selector, name);
+ case GROUP_TYPE:
+ return scmi_pinctrl_get_group_name(ph, selector, name);
+ case FUNCTION_TYPE:
+ return scmi_pinctrl_get_function_name(ph, selector, name);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
+ .count_get = scmi_pinctrl_count_get,
+ .name_get = scmi_pinctrl_name_get,
+ .group_pins_get = scmi_pinctrl_group_pins_get,
+ .function_groups_get = scmi_pinctrl_function_groups_get,
+ .mux_set = scmi_pinctrl_mux_set,
+ .settings_get = scmi_pinctrl_settings_get,
+ .settings_conf = scmi_pinctrl_settings_conf,
+ .pin_request = scmi_pinctrl_pin_request,
+ .pin_free = scmi_pinctrl_pin_free,
+};
+
+static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
+{
+ int ret;
+ u32 version;
+ struct scmi_pinctrl_info *pinfo;
+
+ ret = ph->xops->version_get(ph, &version);
+ if (ret)
+ return ret;
+
+ dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
+ PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+ pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
+ if (!pinfo)
+ return -ENOMEM;
+
+ ret = scmi_pinctrl_attributes_get(ph, pinfo);
+ if (ret)
+ return ret;
+
+ pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
+ sizeof(*pinfo->pins),
+ GFP_KERNEL);
+ if (!pinfo->pins)
+ return -ENOMEM;
+
+ pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
+ sizeof(*pinfo->groups),
+ GFP_KERNEL);
+ if (!pinfo->groups)
+ return -ENOMEM;
+
+ pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
+ sizeof(*pinfo->functions),
+ GFP_KERNEL);
+ if (!pinfo->functions)
+ return -ENOMEM;
+
+ pinfo->version = version;
+
+ return ph->set_priv(ph, pinfo, version);
+}
+
+static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
+{
+ int i;
+ struct scmi_pinctrl_info *pi = ph->get_priv(ph);
+
+ for (i = 0; i < pi->nr_groups; i++) {
+ if (pi->groups[i].present) {
+ kfree(pi->groups[i].group_pins);
+ pi->groups[i].present = false;
+ }
+ }
+
+ for (i = 0; i < pi->nr_functions; i++) {
+ if (pi->functions[i].present) {
+ kfree(pi->functions[i].groups);
+ pi->functions[i].present = false;
+ }
+ }
+
+ return 0;
+}
+
+static const struct scmi_protocol scmi_pinctrl = {
+ .id = SCMI_PROTOCOL_PINCTRL,
+ .owner = THIS_MODULE,
+ .instance_init = &scmi_pinctrl_protocol_init,
+ .instance_deinit = &scmi_pinctrl_protocol_deinit,
+ .ops = &pinctrl_proto_ops,
+ .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
+};
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 3e91536a77a3..c02cbfd2bb03 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -355,6 +355,7 @@ void __exit scmi_##name##_unregister(void) \
DECLARE_SCMI_REGISTER_UNREGISTER(base);
DECLARE_SCMI_REGISTER_UNREGISTER(clock);
DECLARE_SCMI_REGISTER_UNREGISTER(perf);
+DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
DECLARE_SCMI_REGISTER_UNREGISTER(power);
DECLARE_SCMI_REGISTER_UNREGISTER(reset);
DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b807141acc14..64043e269701 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -737,6 +737,80 @@ struct scmi_powercap_proto_ops {
u32 *power_thresh_high);
};

+enum scmi_pinctrl_selector_type {
+ PIN_TYPE = 0,
+ GROUP_TYPE,
+ FUNCTION_TYPE,
+};
+
+enum scmi_pinctrl_conf_type {
+ SCMI_PIN_DEFAULT = 0,
+ SCMI_PIN_BIAS_BUS_HOLD = 1,
+ SCMI_PIN_BIAS_DISABLE = 2,
+ SCMI_PIN_BIAS_HIGH_IMPEDANCE = 3,
+ SCMI_PIN_BIAS_PULL_UP = 4,
+ SCMI_PIN_BIAS_PULL_DEFAULT = 5,
+ SCMI_PIN_BIAS_PULL_DOWN = 6,
+ SCMI_PIN_DRIVE_OPEN_DRAIN = 7,
+ SCMI_PIN_DRIVE_OPEN_SOURCE = 8,
+ SCMI_PIN_DRIVE_PUSH_PULL = 9,
+ SCMI_PIN_DRIVE_STRENGTH = 10,
+ SCMI_PIN_INPUT_DEBOUNCE = 11,
+ SCMI_PIN_INPUT_MODE = 12,
+ SCMI_PIN_PULL_MODE = 13,
+ SCMI_PIN_INPUT_VALUE = 14,
+ SCMI_PIN_INPUT_SCHMITT = 15,
+ SCMI_PIN_LOW_POWER_MODE = 16,
+ SCMI_PIN_OUTPUT_MODE = 17,
+ SCMI_PIN_OUTPUT_VALUE = 18,
+ SCMI_PIN_POWER_SOURCE = 19,
+ SCMI_PIN_SLEW_RATE = 20,
+ SCMI_PIN_OEM_START = 192,
+ SCMI_PIN_OEM_END = 255,
+};
+
+/**
+ * struct scmi_pinctrl_proto_ops - represents the various operations provided
+ * by SCMI Pinctrl Protocol
+ *
+ * @count_get: returns count of the registered elements in given type
+ * @name_get: returns name by index of given type
+ * @group_pins_get: returns the set of pins, assigned to the specified group
+ * @function_groups_get: returns the set of groups, assigned to the specified
+ * function
+ * @mux_set: set muxing function for groups of pins
+ * @config_get: returns configuration parameter for pin or group
+ * @config_set: sets the configuration parameter for pin or group
+ * @pin_request: aquire pin before selecting mux setting
+ * @pin_free: frees pin, acquired by request_pin call
+ */
+struct scmi_pinctrl_proto_ops {
+ int (*count_get)(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type);
+ int (*name_get)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name);
+ int (*group_pins_get)(const struct scmi_protocol_handle *ph,
+ u32 selector, const unsigned int **pins,
+ unsigned int *nr_pins);
+ int (*function_groups_get)(const struct scmi_protocol_handle *ph,
+ u32 selector, unsigned int *nr_groups,
+ const unsigned int **groups);
+ int (*mux_set)(const struct scmi_protocol_handle *ph, u32 selector,
+ u32 group);
+ int (*settings_get)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ enum scmi_pinctrl_conf_type config_type,
+ u32 *config_value);
+ int (*settings_conf)(const struct scmi_protocol_handle *ph,
+ u32 selector, enum scmi_pinctrl_selector_type type,
+ unsigned int nr_configs,
+ enum scmi_pinctrl_conf_type *config_type,
+ u32 *config_value);
+ int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
+ int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
+};
+
/**
* struct scmi_notify_ops - represents notifications' operations provided by
* SCMI core
@@ -844,6 +918,7 @@ enum scmi_std_protocol {
SCMI_PROTOCOL_RESET = 0x16,
SCMI_PROTOCOL_VOLTAGE = 0x17,
SCMI_PROTOCOL_POWERCAP = 0x18,
+ SCMI_PROTOCOL_PINCTRL = 0x19,
};

enum scmi_system_events {

--
2.37.1

2024-03-23 12:08:39

[permalink] [raw]

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

From: Peng Fan <[email protected]>

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 4b511a55101c..d8270ac6651a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
F: drivers/firmware/arm_scmi/
F: drivers/firmware/arm_scpi.c
F: drivers/hwmon/scmi-hwmon.c
+F: drivers/pinctrl/pinctrl-scmi.c
F: drivers/pmdomain/arm/
F: drivers/powercap/arm_scmi_powercap.c
F: drivers/regulator/scmi-regulator.c
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 87d9b89cab13..0ecefe855432 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -465,6 +465,7 @@ scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,

tx = t->tx.buf;
tx->identifier = cpu_to_le32(selector);
+ tx->function_id = cpu_to_le32(0xFFFFFFFF);
attributes = FIELD_PREP(GENMASK(1, 0), type) |
FIELD_PREP(GENMASK(9, 2), chunk);
tx->attributes = cpu_to_le32(attributes);
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index d45657aa986a..4e6f65cf0e76 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
help
This support pinctrl and GPIO driver for Rockchip SoCs.

+config PINCTRL_SCMI
+ tristate "Pinctrl driver using SCMI protocol interface"
+ depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
+ select PINMUX
+ select GENERIC_PINCONF
+ help
+ This driver provides support for pinctrl which is controlled
+ by firmware that implements the SCMI interface.
+ It uses SCMI Message Protocol to interact with the
+ firmware providing all the pinctrl controls.
+
config PINCTRL_SINGLE
tristate "One-register-per-pin type device tree based pinctrl driver"
depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 2152539b53d5..cc809669405a 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o
obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o
obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
+obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o
obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
new file mode 100644
index 000000000000..b23533bff63a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,564 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2024 EPAM
+ * Copyright 2024 NXP
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/seq_file.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinconf.h"
+
+#define DRV_NAME "scmi-pinctrl"
+
+/* Define num configs, if not large than 4 use stack, else use kcalloc */
+#define SCMI_NUM_CONFIGS 4
+
+static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
+
+struct scmi_pinctrl_funcs {
+ unsigned int num_groups;
+ const char **groups;
+};
+
+struct scmi_pinctrl {
+ struct device *dev;
+ struct scmi_protocol_handle *ph;
+ struct pinctrl_dev *pctldev;
+ struct pinctrl_desc pctl_desc;
+ struct scmi_pinctrl_funcs *functions;
+ unsigned int nr_functions;
+ struct pinctrl_pin_desc *pins;
+ unsigned int nr_pins;
+};
+
+static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE);
+}
+
+static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ int ret;
+ const char *name;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name);
+ if (ret) {
+ dev_err(pmx->dev, "get name failed with err %d", ret);
+ return NULL;
+ }
+
+ return name;
+}
+
+static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins);
+}
+
+static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
+ .get_groups_count = pinctrl_scmi_get_groups_count,
+ .get_group_name = pinctrl_scmi_get_group_name,
+ .get_group_pins = pinctrl_scmi_get_group_pins,
+#ifdef CONFIG_OF
+ .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+ .dt_free_map = pinconf_generic_dt_free_map,
+#endif
+};
+
+static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE);
+}
+
+static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ int ret;
+ const char *name;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE, &name);
+ if (ret) {
+ dev_err(pmx->dev, "get name failed with err %d", ret);
+ return NULL;
+ }
+
+ return name;
+}
+
+static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const char * const **p_groups,
+ unsigned int * const p_num_groups)
+{
+ struct scmi_pinctrl_funcs *func;
+ const unsigned int *group_ids;
+ unsigned int num_groups;
+ const char **groups;
+ int ret, i;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!p_groups || !p_num_groups)
+ return -EINVAL;
+
+ if (selector >= pmx->nr_functions)
+ return -EINVAL;
+
+ func = &pmx->functions[selector];
+ if (func->num_groups)
+ goto done;
+
+ ret = pinctrl_ops->function_groups_get(pmx->ph, selector, &num_groups,
+ &group_ids);
+ if (ret) {
+ dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
+ return ret;
+ }
+ if (!num_groups)
+ return -EINVAL;
+
+ groups = devm_kcalloc(pmx->dev, num_groups, sizeof(*groups), GFP_KERNEL);
+ if (!groups)
+ return -ENOMEM;
+
+ for (i = 0; i < num_groups; i++) {
+ groups[i] = pinctrl_scmi_get_group_name(pctldev, group_ids[i]);
+ if (!groups[i]) {
+ ret = -EINVAL;
+ goto err_free;
+ }
+ }
+
+ func->num_groups = num_groups;
+ func->groups = groups;
+done:
+ *p_groups = (const char * const *)func->groups;
+ *p_num_groups = func->num_groups;
+
+ return 0;
+
+err_free:
+ devm_kfree(pmx->dev, groups);
+
+ return ret;
+}
+
+static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
+ unsigned int selector, unsigned int group)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->mux_set(pmx->ph, selector, group);
+}
+
+static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
+ unsigned int offset)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->pin_request(pmx->ph, offset);
+}
+
+static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ return pinctrl_ops->pin_free(pmx->ph, offset);
+}
+
+static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
+ .request = pinctrl_scmi_request,
+ .free = pinctrl_scmi_free,
+ .get_functions_count = pinctrl_scmi_get_functions_count,
+ .get_function_name = pinctrl_scmi_get_function_name,
+ .get_function_groups = pinctrl_scmi_get_function_groups,
+ .set_mux = pinctrl_scmi_func_set_mux,
+};
+
+static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
+ enum scmi_pinctrl_conf_type *type)
+{
+ u32 arg = param;
+
+ switch (arg) {
+ case PIN_CONFIG_BIAS_BUS_HOLD:
+ *type = SCMI_PIN_BIAS_BUS_HOLD;
+ break;
+ case PIN_CONFIG_BIAS_DISABLE:
+ *type = SCMI_PIN_BIAS_DISABLE;
+ break;
+ case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+ *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
+ break;
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ *type = SCMI_PIN_BIAS_PULL_DOWN;
+ break;
+ case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+ *type = SCMI_PIN_BIAS_PULL_DEFAULT;
+ break;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ *type = SCMI_PIN_BIAS_PULL_UP;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ *type = SCMI_PIN_DRIVE_OPEN_DRAIN;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ *type = SCMI_PIN_DRIVE_OPEN_SOURCE;
+ break;
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ *type = SCMI_PIN_DRIVE_PUSH_PULL;
+ break;
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ *type = SCMI_PIN_DRIVE_STRENGTH;
+ break;
+ case PIN_CONFIG_DRIVE_STRENGTH_UA:
+ *type = SCMI_PIN_DRIVE_STRENGTH;
+ break;
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ *type = SCMI_PIN_INPUT_DEBOUNCE;
+ break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ *type = SCMI_PIN_INPUT_MODE;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT:
+ *type = SCMI_PIN_INPUT_SCHMITT;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+ *type = SCMI_PIN_INPUT_MODE;
+ break;
+ case PIN_CONFIG_MODE_LOW_POWER:
+ *type = SCMI_PIN_LOW_POWER_MODE;
+ break;
+ case PIN_CONFIG_OUTPUT:
+ *type = SCMI_PIN_OUTPUT_VALUE;
+ break;
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ *type = SCMI_PIN_OUTPUT_MODE;
+ break;
+ case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
+ *type = SCMI_PIN_OUTPUT_VALUE;
+ break;
+ case PIN_CONFIG_POWER_SOURCE:
+ *type = SCMI_PIN_POWER_SOURCE;
+ break;
+ case PIN_CONFIG_SLEW_RATE:
+ *type = SCMI_PIN_SLEW_RATE;
+ break;
+ case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
+ *type = arg;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
+ unsigned int _pin, unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param config_type;
+ enum scmi_pinctrl_conf_type type;
+ u32 config_value;
+
+ if (!config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+ if (ret)
+ return ret;
+
+ ret = pinctrl_ops->settings_get(pmx->ph, _pin, PIN_TYPE, type,
+ &config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static int
+pinctrl_scmi_alloc_configs(struct pinctrl_dev *pctldev, u32 num_configs,
+ u32 **p_config_value,
+ enum scmi_pinctrl_conf_type **p_config_type)
+{
+ if (num_configs <= SCMI_NUM_CONFIGS)
+ return 0;
+
+ *p_config_value = kcalloc(num_configs, sizeof(u32), GFP_KERNEL);
+ *p_config_type = kcalloc(num_configs,
+ sizeof(enum scmi_pinctrl_conf_type),
+ GFP_KERNEL);
+
+ if (!*p_config_value || !*p_config_type) {
+ kfree(*p_config_value);
+ kfree(*p_config_type);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void
+pinctrl_scmi_free_configs(struct pinctrl_dev *pctldev, u32 num_configs,
+ u32 **p_config_value,
+ enum scmi_pinctrl_conf_type **p_config_type)
+{
+ if (num_configs <= SCMI_NUM_CONFIGS)
+ return;
+
+ kfree(*p_config_value);
+ kfree(*p_config_type);
+}
+
+static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
+ u32 config_value[SCMI_NUM_CONFIGS];
+ enum scmi_pinctrl_conf_type *p_config_type = config_type;
+ u32 *p_config_value = config_value;
+ enum pin_config_param param;
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ ret = pinctrl_scmi_map_pinconf_type(param, &p_config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ goto free_config;
+ }
+ p_config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->settings_conf(pmx->ph, _pin, PIN_TYPE, num_configs,
+ p_config_type, p_config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d\n", ret);
+
+free_config:
+ pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ return ret;
+}
+
+static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
+ unsigned int group,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
+ u32 config_value[SCMI_NUM_CONFIGS];
+ enum scmi_pinctrl_conf_type *p_config_type = config_type;
+ u32 *p_config_value = config_value;
+ enum pin_config_param param;
+
+ if (!configs || !num_configs)
+ return -EINVAL;
+
+ ret = pinctrl_scmi_alloc_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ ret = pinctrl_scmi_map_pinconf_type(param,
+ &p_config_type[i]);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ goto free_config;
+ }
+
+ p_config_value[i] = pinconf_to_config_argument(configs[i]);
+ }
+
+ ret = pinctrl_ops->settings_conf(pmx->ph, group, GROUP_TYPE,
+ num_configs, p_config_type,
+ p_config_value);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d", ret);
+
+free_config:
+ pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
+ &p_config_value);
+ return ret;
+};
+
+static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
+ unsigned int group,
+ unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param config_type;
+ enum scmi_pinctrl_conf_type type;
+ u32 config_value;
+
+ if (!config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+ ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
+ if (ret) {
+ dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
+ return ret;
+ }
+
+ ret = pinctrl_ops->settings_get(pmx->ph, group, GROUP_TYPE, type,
+ &config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
+ .is_generic = true,
+ .pin_config_get = pinctrl_scmi_pinconf_get,
+ .pin_config_set = pinctrl_scmi_pinconf_set,
+ .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
+ .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
+ .pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
+ struct pinctrl_desc *desc)
+{
+ struct pinctrl_pin_desc *pins;
+ unsigned int npins;
+ int ret, i;
+
+ npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
+ pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
+ if (!pins)
+ return -ENOMEM;
+
+ for (i = 0; i < npins; i++) {
+ pins[i].number = i;
+ ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
+ if (ret)
+ return dev_err_probe(pmx->dev, ret,
+ "Can't get name for pin %d", i);
+ }
+
+ desc->npins = npins;
+ desc->pins = pins;
+ dev_dbg(pmx->dev, "got pins %d", npins);
+
+ return 0;
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+ { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+ { }
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static int scmi_pinctrl_probe(struct scmi_device *sdev)
+{
+ int ret;
+ struct device *dev = &sdev->dev;
+ struct scmi_pinctrl *pmx;
+ const struct scmi_handle *handle;
+ struct scmi_protocol_handle *ph;
+
+ if (!sdev || !sdev->handle)
+ return -EINVAL;
+
+ handle = sdev->handle;
+
+ pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
+ &ph);
+ if (IS_ERR(pinctrl_ops))
+ return PTR_ERR(pinctrl_ops);
+
+ pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
+ if (!pmx)
+ return -ENOMEM;
+
+ pmx->ph = ph;
+
+ pmx->dev = dev;
+ pmx->pctl_desc.name = DRV_NAME;
+ pmx->pctl_desc.owner = THIS_MODULE;
+ pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
+ pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
+ pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
+
+ ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc);
+ if (ret)
+ return ret;
+
+ ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
+ &pmx->pctldev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
+
+ pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
+ pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
+ sizeof(*pmx->functions),
+ GFP_KERNEL);
+ if (!pmx->functions)
+ return -ENOMEM;
+
+ return pinctrl_enable(pmx->pctldev);
+}
+
+static struct scmi_driver scmi_pinctrl_driver = {
+ .name = DRV_NAME,
+ .probe = scmi_pinctrl_probe,
+ .id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_pinctrl_driver);
+
+MODULE_AUTHOR("Oleksii Moisieiev <[email protected]>");
+MODULE_AUTHOR("Peng Fan <[email protected]>");
+MODULE_DESCRIPTION("ARM SCMI pin controller driver");
+MODULE_LICENSE("GPL");

--
2.37.1

2024-03-27 10:50:08

by Dan Carpenter

[permalink] [raw]

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

Looks really nice. Just a few small comments below.

On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};

This scmi_msg_func_set struct is unused. Delete.

> +static void
> +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_settings_get *msg = message;
> + const struct scmi_settings_get_ipriv *p = priv;
> + u32 attributes;
> +
> + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> + FIELD_PREP(SELECTOR_MASK, p->type);
> +
> + if (p->flag == 1)
> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)

This is a nit-pick but could you change these !p->flag conditions to
p->flag == 0? It's a number zero, not a bool.

> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> +
> + msg->attributes = cpu_to_le32(attributes);
> + msg->identifier = cpu_to_le32(p->selector);
> +}
> +
> +static int
> +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (p->flag == 1) {
> + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));
> + } else {
> + st->num_returned = 1;
> + st->num_remaining = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (!p->flag) {

if (p->flag == 0) {

> + if (p->config_types[0] !=
> + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> + return -EINVAL;
> + } else if (p->flag == 1) {
> + p->config_types[st->desc_index + st->loop_idx] =
> + le32_get_bits(r->configs[st->loop_idx * 2],
> + GENMASK(7, 0));
> + } else if (p->flag == 2) {
> + return 0;
> + }
> +
> + p->config_values[st->desc_index + st->loop_idx] =
> + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> +
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> + .update_state = iter_pinctrl_settings_get_update_state,
> + .process_response = iter_pinctrl_settings_get_process_response,
> + };
> + struct scmi_settings_get_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .flag = 0,

->flag should be 0-2.

> + .config_types = &config_type,
> + .config_values = config_value,
> + };
> +
> + if (!config_value || type == FUNCTION_TYPE)
^^^^^^^^^^^^
config_value should be optional for flag == 2.

regards,
dan carpenter

> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +

2024-03-27 11:23:00

by Dan Carpenter

[permalink] [raw]

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

On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote:
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> index 87d9b89cab13..0ecefe855432 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -465,6 +465,7 @@ scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
>
> tx = t->tx.buf;
> tx->identifier = cpu_to_le32(selector);
> + tx->function_id = cpu_to_le32(0xFFFFFFFF);
> attributes = FIELD_PREP(GENMASK(1, 0), type) |
> FIELD_PREP(GENMASK(9, 2), chunk);
> tx->attributes = cpu_to_le32(attributes);

This should have been done in patch 3.

regards,
dan carpenter

> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP

2024-03-27 13:22:25

[permalink] [raw]

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

On Wed, Mar 27, 2024 at 01:46:11PM +0300, Dan Carpenter wrote:
> Looks really nice. Just a few small comments below.
>

Hi,

I aa having a look at this today (and try to retest it), just a quick remark
down below...

> On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
>
> This scmi_msg_func_set struct is unused. Delete.
>
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> > + FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->flag == 1)
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
>
> This is a nit-pick but could you change these !p->flag conditions to
> p->flag == 0? It's a number zero, not a bool.
>
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector);
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->flag == 1) {
> > + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (!p->flag) {
>
>
> if (p->flag == 0) {
>
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
> > +
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response = iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .flag = 0,
>
> ->flag should be 0-2.
>

The .flag in this priv structure is for 'configuring' the iterators in
the SCMI core to parse a multi-part response, so (looking at how the
iterators functs are implemented above) setting it here to zero
means issuing a SETTINGS_GET with attributes.config_flag[19:18] == 0,
that in turn means requesting just a single config_value to be read...

..if you want to support the other 'flavours' of SETTINGS_GET
(multiple configs & selected_func) you will have to extend the signature
of this function to optionally select to readback multiple configs (and to
allow the return of such multiple config_values) and/or optionally request
to return the selected function....

..or maybe add distinct wrapper protocol_ops just for these flavours...

Anyway, till now in this series it was avoided to add such 'flavours'
support (e.g. for multiple configs) since there are no users for the
multi-config and function selected in the pinctrl driver...so no way to test..

Will the Linux GPIO driver need these ? Should we delay anyway the
addition of the support of such variants of SETTING_GET for when a real
user like GPIO driveer appears ?

..anyway @Peng please add a comment somewhere explaining how p->flag is
used to configure the type of SETTINGS_GET

Thanks,
Cristian

2024-03-28 15:54:35

[permalink] [raw]

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

On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.
>

Hi,

a few more comments down below...

> Co-developed-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 921 ++++++++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 75 +++
> 5 files changed, 1000 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..8e3874ff1544 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y += pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 415e6f510057..ac2d4b19727c 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
> scmi_voltage_register();
> scmi_system_register();
> scmi_powercap_register();
> + scmi_pinctrl_register();
>
> return platform_driver_register(&scmi_driver);
> }
> @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
> scmi_voltage_unregister();
> scmi_system_unregister();
> scmi_powercap_unregister();
> + scmi_pinctrl_unregister();
>
> scmi_transports_exit();
>
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> new file mode 100644
> index 000000000000..87d9b89cab13
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,921 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +#include "protocols.h"
> +
> +/* Updated only after ALL the mandatory features for that version are merged */
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
> +

AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional command)
and the multiple-configs on SETTINGS_GET, but this latter is something really
that we have to ask for in the request AND we did not as of now since we dont
need it...so I would say to bump the version to 0x10000 just to avoid needless
warning as soon as a server supporting Pinctrl is met.

> +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
> +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> +
> +#define CONFIG_FLAG_MASK GENMASK(19, 18)
> +#define SELECTOR_MASK GENMASK(17, 16)
> +#define SKIP_CONFIGS_MASK GENMASK(15, 8)
> +#define CONFIG_TYPE_MASK GENMASK(7, 0)
> +
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_SETTINGS_GET = 0x5,
> + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> + PINCTRL_REQUEST = 0x7,
> + PINCTRL_RELEASE = 0x8,
> + PINCTRL_NAME_GET = 0x9,
> + PINCTRL_SET_PERMISSIONS = 0xa
> +};
> +
> +struct scmi_msg_settings_conf {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 attributes;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_settings_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_resp_settings_get {
> + __le32 function_selected;
> + __le32 num_configs;
> + __le32 configs[];
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> + __le32 attributes_low;
> + __le32 attributes_high;
> +};
> +
> +struct scmi_msg_pinctrl_attributes {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> + __le32 attributes;
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> + __le32 identifier;
> + __le32 flags;
> + __le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> + __le32 flags;
> + __le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};
> +

As said by Dan...drop this.

> +struct scmi_msg_request {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_group_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *group_pins;
> + u32 nr_pins;
> +};
> +
> +struct scmi_function_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> + u32 *groups;
> + u32 nr_groups;
> +};
> +
> +struct scmi_pin_info {
> + char name[SCMI_MAX_STR_SIZE];
> + bool present;
> +};
> +
> +struct scmi_pinctrl_info {
> + u32 version;
> + int nr_groups;
> + int nr_functions;
> + int nr_pins;
> + struct scmi_group_info *groups;
> + struct scmi_function_info *functions;
> + struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
> + &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
> + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> + pi->nr_pins = GET_PINS_NR(attr->attributes_low);

I was thinking, does make sense to allow a nr_pins == 0 setup to probe
successfully ? Becasuse is legit for the platform to return zero groups or
zero functions BUT zero pins is just useless (spec does not say
anything)

Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail
out with -EINVAL...

On the other side looking at the zero groups/function case, that is
plausible and handled properly by the driver since a 0-bytes
devm_kcalloc will return ZERO_SIZE_PTR (not NULL) and all the remaining
references to pinfo->groups and pinfo->functions are guarded by a check
on selector >= nr_groups (or >= nr_functions), and by scmi_pinctrl_validate_id()
so the zero grouyps/fuctions scenarios should be safely handled.

> + }
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + switch (type) {
> + case PIN_TYPE:
> + return pi->nr_pins;
> + case GROUP_TYPE:
> + return pi->nr_groups;
> + case FUNCTION_TYPE:
> + return pi->nr_functions;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int value;
> +
> + value = scmi_pinctrl_count_get(ph, type);
> + if (value < 0)
> + return value;
> +
> + if (identifier >= value)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type,
> + u32 selector, char *name,
> + u32 *n_elems)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_attributes *tx;
> + struct scmi_resp_pinctrl_attributes *rx;
> + u32 ext_name_flag;

what about a bool

> +
> + if (!name)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> + sizeof(*rx), &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + rx = t->rx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + if (n_elems)
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
> + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> +
> + ext_name_flag = EXT_NAME_FLAG(rx->attributes);
> + } else
> + ext_name_flag = 0;

and you dont need this else branch to set ext_name_flag to false, since down
below you will check ext_flag ONLY if !ret, so it will have surely been
set if the do_xfer did not fail.

> +
> + 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)
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
> +}
> +
> +struct scmi_pinctrl_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 *array;
> +};
> +
> +static void iter_pinctrl_assoc_prepare_message(void *message,
> + u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_pinctrl_list_assoc *msg = message;
> + const struct scmi_pinctrl_ipriv *p = priv;
> +
> + msg->identifier = cpu_to_le32(p->selector);
> + msg->flags = cpu_to_le32(p->type);
> + /* Set the number of OPPs to be skipped/already read */

OPP ? .. maybe drop this comment that was cut/pasted from somewhere else :D

> + msg->index = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> +
> + st->num_returned = RETURNED(r->flags);
> + st->num_remaining = REMAINING(r->flags);
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> + struct scmi_pinctrl_ipriv *p = priv;
> +
> + p->array[st->desc_index + st->loop_idx] =
> + le16_to_cpu(r->array[st->loop_idx]);
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u16 size, u32 *array)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_assoc_prepare_message,
> + .update_state = iter_pinctrl_assoc_update_state,
> + .process_response = iter_pinctrl_assoc_process_response,
> + };
> + struct scmi_pinctrl_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .array = array,
> + };
> +
> + if (!array || !size || type == PIN_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +struct scmi_settings_get_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + u32 flag;
> + enum scmi_pinctrl_conf_type *config_types;
> + u32 *config_values;
> +};
> +
> +static void
> +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_settings_get *msg = message;
> + const struct scmi_settings_get_ipriv *p = priv;
> + u32 attributes;
> +
> + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> + FIELD_PREP(SELECTOR_MASK, p->type);
> +
> + if (p->flag == 1)

A boolean like .get_all would be more clear..see down below why you dont need
a flag 0|1|2

> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)
> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);
> +
> + msg->attributes = cpu_to_le32(attributes);
> + msg->identifier = cpu_to_le32(p->selector);
> +}
> +
> +static int
> +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> + const void *response, void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (p->flag == 1) {

Ditto... see below the explanation

> + st->num_returned = le32_get_bits(r->num_configs, GENMASK(7, 0));
> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));
> + } else {
> + st->num_returned = 1;
> + st->num_remaining = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_settings_get_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st,
> + void *priv)
> +{
> + const struct scmi_resp_settings_get *r = response;
> + struct scmi_settings_get_ipriv *p = priv;
> +
> + if (!p->flag) {
> + if (p->config_types[0] !=
> + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> + return -EINVAL;
> + } else if (p->flag == 1) {
> + p->config_types[st->desc_index + st->loop_idx] =
> + le32_get_bits(r->configs[st->loop_idx * 2],
> + GENMASK(7, 0));
> + } else if (p->flag == 2) {
> + return 0;
> + }

Unneeded...see down below for explanation

> +
> + p->config_values[st->desc_index + st->loop_idx] =
> + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> +
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + enum scmi_pinctrl_conf_type config_type,
> + u32 *config_value)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_settings_get_prepare_message,
> + .update_state = iter_pinctrl_settings_get_update_state,
> + .process_response = iter_pinctrl_settings_get_process_response,
> + };
> + struct scmi_settings_get_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .flag = 0,
> + .config_types = &config_type,
> + .config_values = config_value,
> + };
> +

So this function is used to retrieve configs; as of now, just one, then
it could be extended to fetch all the configs, and for this it uses the
iterators helpers, BUT it is not and will not be used to just fetch the
selected_function with flag_2 (even though is always provided), since in
that case you wont get back a multi-part SCMI response and so there is no
need to use iterators...

IOW... no need here to handle flag_2 scenario and as a consequence I would
change the ipriv flag to be be a boolean .get_all, like it was, since it is
more readable (and so you wont need to add any comment..)

In the future could make sense to add here also a *selected_function output
param to this function since you will always get it back for free when
retrieving configs ... BUT for now is just not needed really...no users
for this case till now...

..when the time will come that we will need a function_selected_get to
be issued without retrieveing also the configs I would add a distinct
routine that crafts properly a SETTINGS_GET with flag_2 without worrying
about multi-part responses (and with no need for iterators support)

Trying to handle all in here just complicates stuff...

> + if (!config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +static int
> +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u32 nr_configs,
> + enum scmi_pinctrl_conf_type *config_type,
> + u32 *config_value)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> + int ret, i;
> + u32 configs_in_chunk, conf_num = 0;
> + u32 chunk;
> + int max_msg_size = ph->hops->get_max_msg_size(ph);
> +
> + if (!config_type || !config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
> + while (conf_num < nr_configs) {
> + chunk = (nr_configs - conf_num > configs_in_chunk) ?
> + configs_in_chunk : nr_configs - conf_num;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx) +
> + chunk * 2 * sizeof(__le32),
> + 0, &t);
> + if (ret)
> + return ret;
for consistency I would
break;

like below and you will exit always from the last return ret;

> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) |
> + FIELD_PREP(GENMASK(9, 2), chunk);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + for (i = 0; i < chunk; i++) {
> + tx->configs[i * 2] =
> + cpu_to_le32(config_type[conf_num + i]);
> + tx->configs[i * 2 + 1] =
> + cpu_to_le32(config_value[conf_num + i]);
> + }
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> +
> + if (ret)
> + break;
> +
> + conf_num += chunk;
> + }
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
> + u32 group,
> + enum scmi_pinctrl_selector_type type,
> + u32 function_id)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_settings_conf *tx;
> + u32 attributes;
> +
> + ret = scmi_pinctrl_validate_id(ph, group, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> + sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(group);
> + tx->function_id = cpu_to_le32(function_id);
> + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
> + tx->attributes = cpu_to_le32(attributes);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_request(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);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +

.this function ...

> +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> + u32 pin)
> +{
> + return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +

..and this are completely identical, beside the used command msg_id...please make
it a common workhorse function by adding a param for the command...

> +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle *ph, u32 pin)
> +{
> + return scmi_pinctrl_free(ph, pin, PIN_TYPE);
> +}
> +

..and convert these _request/_free functions into a pair odf simple wrapper invoking
the common workhorse...

> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_group_info *group)
> +{
> + int ret;
> +
> + if (!group)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + group->name,
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = kmalloc_array(group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + kfree(group->group_pins);
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->groups[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle *ph,
> + u32 selector, const u32 **pins,
> + u32 *nr_pins)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *pins = pi->groups[selector].group_pins;
> + *nr_pins = pi->groups[selector].nr_pins;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{
> + int ret;
> +
> + if (!func)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + func->name,
> + &func->nr_groups);
> + if (ret)
> + return ret;
> +
> + if (!func->nr_groups) {
> + dev_err(ph->dev, "Function %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
> + GFP_KERNEL);
> + if (!func->groups)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> + func->nr_groups, func->groups);
> + if (ret) {
> + kfree(func->groups);
> + return ret;
> + }
> +
> + func->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->functions[selector].name;
> + return 0;
> +}
> +
> +static int
> +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 *nr_groups,
> + const u32 **groups)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!groups || !nr_groups)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *groups = pi->functions[selector].groups;
> + *nr_groups = pi->functions[selector].nr_groups;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 group)
> +{
> + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE, selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> + u32 selector, struct scmi_pin_info *pin)
> +{
> + int ret;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + pin->name, NULL);
> + if (ret)
> + return ret;
> +
> + pin->present = true;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + if (!name)
> + return -EINVAL;
> +
> + if (selector >= pi->nr_pins)
> + return -EINVAL;
> +
> + if (!pi->pins[selector].present) {
> + int ret;
> +
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->pins[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name)
> +{
> + switch (type) {
> + case PIN_TYPE:
> + return scmi_pinctrl_get_pin_name(ph, selector, name);
> + case GROUP_TYPE:
> + return scmi_pinctrl_get_group_name(ph, selector, name);
> + case FUNCTION_TYPE:
> + return scmi_pinctrl_get_function_name(ph, selector, name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .count_get = scmi_pinctrl_count_get,
> + .name_get = scmi_pinctrl_name_get,
> + .group_pins_get = scmi_pinctrl_group_pins_get,
> + .function_groups_get = scmi_pinctrl_function_groups_get,
> + .mux_set = scmi_pinctrl_mux_set,
> + .settings_get = scmi_pinctrl_settings_get,
> + .settings_conf = scmi_pinctrl_settings_conf,
> + .pin_request = scmi_pinctrl_pin_request,
> + .pin_free = scmi_pinctrl_pin_free,
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;

.as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get
could return -EINVAL here and bail out....not sure that a running setup
with zero pins has any values (even for testing...) BUT, as said above,
I wuld certainly add a dev_warn in scmi_pinctrl_attributes_get() when
nr_pins == 0

Thanks,
Cristian

2024-03-28 16:50:15

[permalink] [raw]

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

On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.
>

Hi,

> Co-developed-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/firmware/arm_scmi/pinctrl.c | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 564 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 578 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> F: drivers/hwmon/scmi-hwmon.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> index 87d9b89cab13..0ecefe855432 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -465,6 +465,7 @@ scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
>
> tx = t->tx.buf;
> tx->identifier = cpu_to_le32(selector);
> + tx->function_id = cpu_to_le32(0xFFFFFFFF);

As already said....does not belong to this patch

> attributes = FIELD_PREP(GENMASK(1, 0), type) |
> FIELD_PREP(GENMASK(9, 2), chunk);
> tx->attributes = cpu_to_le32(attributes);
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +

[snip]

> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +
> + ret = pinctrl_ops->settings_get(pmx->ph, group, GROUP_TYPE, type,
> + &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> + .is_generic = true,
> + .pin_config_get = pinctrl_scmi_pinconf_get,
> + .pin_config_set = pinctrl_scmi_pinconf_set,
> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> + .pin_config_config_dbg_show = pinconf_generic_dump_config,
> +};
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + struct pinctrl_desc *desc)
> +{
> + struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + int ret, i;
> +
> + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +

This is fine only if npins != 0, because on zero npins
devm_kmalloc_array will return ZERO_SIZE_PTR which is NOT-NULL so I
would add a check for !npins and bail out..or use ZERO_OR_NULL_PTR()
to check the return value...if from the previous pinctrl patch you
had decided to bail out at the protocol layer when (nr_pins == 0) and so
you will never get here, please add a comment above that npins cannot be
zero...

> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %d", npins);
> +
> + return 0;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
> +{
> + int ret;
> + struct device *dev = &sdev->dev;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev || !sdev->handle)

if (!sdev->handle) is enough...

> + return -EINVAL;
> +
> + handle = sdev->handle;
> +
> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
> +
> + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = dev;
> + pmx->pctl_desc.name = DRV_NAME;
> + pmx->pctl_desc.owner = THIS_MODULE;
> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc);
> + if (ret)
> + return ret;
> +
> + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> + pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + return -ENOMEM;
> +
> + return pinctrl_enable(pmx->pctldev);
> +}
> +
> +static struct scmi_driver scmi_pinctrl_driver = {
> + .name = DRV_NAME,
> + .probe = scmi_pinctrl_probe,
> + .id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_pinctrl_driver);
> +
> +MODULE_AUTHOR("Oleksii Moisieiev <[email protected]>");
> +MODULE_AUTHOR("Peng Fan <[email protected]>");
> +MODULE_DESCRIPTION("ARM SCMI pin controller driver");
> +MODULE_LICENSE("GPL");
>

Thanks,
Cristian

2024-03-28 16:51:11

[permalink] [raw]

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

On Sat, Mar 23, 2024 at 08:15:13PM +0800, Peng Fan (OSS) wrote:
> This patchset is a rework from Oleksii's RFC v5 patchset
> https://lore.kernel.org/all/[email protected]/
>

Hi,

beside a few more remarks on the series, I tested this again on my setup
with the latest v3.2 changes and it works fine.

Thanks,
Cristian

2024-03-29 19:06:00

[permalink] [raw]

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

Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> From: Peng Fan <[email protected]>
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol.

..

> 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

Actually you want to have := here.

> +scmi-protocols-y += pinctrl.o

> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)

Side note: The -objs has to be -y

..

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

This is semi-random list of headers. Please, follow IWYU principle
(include what you use). There are a lot of inclusions I see missing
(just in the context of this page I see bits.h, types.h, and
asm/byteorder.h).

..

> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_SETTINGS_GET = 0x5,
> + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> + PINCTRL_REQUEST = 0x7,
> + PINCTRL_RELEASE = 0x8,
> + PINCTRL_NAME_GET = 0x9,
> + PINCTRL_SET_PERMISSIONS = 0xa

Leave trailing comma as it's not a termination.

> +};

..

> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr),
> + &t);

This looks much better on a single line.

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

..

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

Possible to have on a single line (if you use relaxed 100 limit).
Or (re)split it more logically:

ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES,
sizeof(*tx), sizeof(*rx), &t);

> + if (ret)
> + return ret;

..

> + /*
> + * If supported overwrite short name with the extended one;
> + * on error just carry on and use already provided short name.
> + */
> + if (!ret && ext_name_flag)

Please, use standard pattern, i.e.

if (ret)
return ret;

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

Why is an explicit casting needed?

> + SCMI_MAX_STR_SIZE);
> + return ret;

..

> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);

> +

Redundant blank line.

> + if (IS_ERR(iter))
> + return PTR_ERR(iter);

..

> + if (p->flag == 1)
> + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> + else if (!p->flag)

Be consistent, i.e. if (p->flag == 0)

> + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p->config_types[0]);

..

> + st->num_remaining = le32_get_bits(r->num_configs,
> + GENMASK(31, 24));

One line?

..

> + if (!p->flag) {
> + if (p->config_types[0] !=
> + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> + return -EINVAL;
> + } else if (p->flag == 1) {
> + p->config_types[st->desc_index + st->loop_idx] =
> + le32_get_bits(r->configs[st->loop_idx * 2],
> + GENMASK(7, 0));

With a temporary variable for r->configs[st->loop_idx * 2] the above can be
written in much better way.

> + } else if (p->flag == 2) {
> + return 0;
> + }

> + p->config_values[st->desc_index + st->loop_idx] =
> + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);

For the sake of consistency with the above suggestion also temporary for next
config value.

..

> + iter = ph->hops->iter_response_init(ph, &ops, 1, PINCTRL_SETTINGS_GET,
> + sizeof(struct scmi_msg_settings_get),
> + &ipriv);

> +

Redundant blank line.

> + if (IS_ERR(iter))
> + return PTR_ERR(iter);

..

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

When the above is not a dead code?

> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + group->name,
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = kmalloc_array(group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret) {
> + kfree(group->group_pins);
> + return ret;
> + }
> +
> + group->present = true;
> + return 0;
> +}

..

> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);

One line?

> + if (ret)
> + return ret;

..

> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + func->name,
> + &func->nr_groups);

At least last two lines can be joined.

> + if (ret)
> + return ret;

..

> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + pin->name, NULL);

It's pleany of room on the previous line.

> + if (ret)
> + return ret;

..

> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);

Ditto.

> + if (ret)
> + return ret;

..

> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + int ret;
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);

Huh?!

Please, get yourself familiar with the scope of devm APIs.

> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
> +
> + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> + sizeof(*pinfo->pins),
> + GFP_KERNEL);
> + if (!pinfo->pins)
> + return -ENOMEM;
> +
> + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> + sizeof(*pinfo->groups),
> + GFP_KERNEL);
> + if (!pinfo->groups)
> + return -ENOMEM;
> +
> + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> + sizeof(*pinfo->functions),
> + GFP_KERNEL);
> + if (!pinfo->functions)
> + return -ENOMEM;
> +
> + pinfo->version = version;
> +
> + return ph->set_priv(ph, pinfo, version);
> +}
> +
> +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> +{
> + int i;
> + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> +
> + for (i = 0; i < pi->nr_groups; i++) {
> + if (pi->groups[i].present) {
> + kfree(pi->groups[i].group_pins);
> + pi->groups[i].present = false;
> + }
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++) {
> + if (pi->functions[i].present) {
> + kfree(pi->functions[i].groups);

This is wrong in conjunction with the above.

> + pi->functions[i].present = false;
> + }
> + }
> +
> + return 0;
> +}

..

> +static const struct scmi_protocol scmi_pinctrl = {
> + .id = SCMI_PROTOCOL_PINCTRL,
> + .owner = THIS_MODULE,
> + .instance_init = &scmi_pinctrl_protocol_init,
> + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> + .ops = &pinctrl_proto_ops,
> + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
> +};

> +

Redundant blank line.

> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)

--
With Best Regards,
Andy Shevchenko

2024-03-29 19:10:37

[permalink] [raw]

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

Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) kirjoitti:
> From: Peng Fan <[email protected]>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.

..

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

Use IWYU principle. There are missing header inclusions and/or forward
declarations.

..

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

This is repeating struct pinfunction. Why can't the latter be used?

..

> +err_free:
> + devm_kfree(pmx->dev, groups);

Why?!

This is 99.9% that the initial allocation must not be devm.

..

> + *p_config_value = kcalloc(num_configs, sizeof(u32), GFP_KERNEL);

sizeof(**p_config_value)?

> + *p_config_type = kcalloc(num_configs,
> + sizeof(enum scmi_pinctrl_conf_type),

sizeof(**p_config_type)?

> + GFP_KERNEL);
> +
> + if (!*p_config_value || !*p_config_type) {
> + kfree(*p_config_value);
> + kfree(*p_config_type);
> + return -ENOMEM;

Why to allocate / free memory in the case when you know that the first one
failed?

> + }

..

> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + struct pinctrl_desc *desc)
> +{
> + struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + int ret, i;
> +
> + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %d", npins);

%u

> + return 0;
> +}

..

> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);

One line.

> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);

--
With Best Regards,
Andy Shevchenko

2024-03-31 12:47:24

[permalink] [raw]

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

Hi Dan,

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> Looks really nice. Just a few small comments below.
>
> On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
>
> This scmi_msg_func_set struct is unused. Delete.
>
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> > + FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->flag == 1)
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
>
> This is a nit-pick but could you change these !p->flag conditions to
> p->flag == 0? It's a number zero, not a bool.
>
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->flag == 1) {
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (!p->flag) {
>
>
> if (p->flag == 0) {
>
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
> > +
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .flag = 0,
>
> ->flag should be 0-2.
>
> > + .config_types = &config_type,
> > + .config_values = config_value,
> > + };
> > +
> > + if (!config_value || type == FUNCTION_TYPE)
> ^^^^^^^^^^^^
> config_value should be optional for flag == 2.

As Cristian replied, I would keep it as is until we have a case in
linux that need flag == 2.

Thanks,
Peng

>
> regards,
> dan carpenter
>
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +

2024-03-31 13:28:51

[permalink] [raw]

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

Hi Cristian,

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
>
> Hi,
>
> a few more comments down below...
>
> > Co-developed-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/Makefile | 1 +
> > drivers/firmware/arm_scmi/driver.c | 2 +
> > drivers/firmware/arm_scmi/pinctrl.c | 921
> ++++++++++++++++++++++++++++++++++
> > drivers/firmware/arm_scmi/protocols.h | 1 +
> > include/linux/scmi_protocol.h | 75 +++
> > 5 files changed, 1000 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index a7bc4796519c..8e3874ff1544 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) +=
> msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
> > +scmi-protocols-y += pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > a/drivers/firmware/arm_scmi/driver.c
> > b/drivers/firmware/arm_scmi/driver.c
> > index 415e6f510057..ac2d4b19727c 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
> > scmi_voltage_register();
> > scmi_system_register();
> > scmi_powercap_register();
> > + scmi_pinctrl_register();
> >
> > return platform_driver_register(&scmi_driver);
> > }
> > @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
> > scmi_voltage_unregister();
> > scmi_system_unregister();
> > scmi_powercap_unregister();
> > + scmi_pinctrl_unregister();
> >
> > scmi_transports_exit();
> >
> > diff --git a/drivers/firmware/arm_scmi/pinctrl.c
> > b/drivers/firmware/arm_scmi/pinctrl.c
> > new file mode 100644
> > index 000000000000..87d9b89cab13
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/pinctrl.c
> > @@ -0,0 +1,921 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System Control and Management Interface (SCMI) Pinctrl Protocol
> > + *
> > + * Copyright (C) 2024 EPAM
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> > +
> > +#include "common.h"
> > +#include "protocols.h"
> > +
> > +/* Updated only after ALL the mandatory features for that version are
> merged */
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
> > +
>
> AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional
> command)

I not see users as of now, could we add it until we need it?

and the multiple-configs on SETTINGS_GET, but this latter is
> something really that we have to ask for in the request AND we did not as of
> now since we dont need it...so I would say to bump the version to 0x10000

ok.

> just to avoid needless warning as soon as a server supporting Pinctrl is met.
>
> > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define REMAINING(x) le32_get_bits((x), GENMASK(31,
> 16))
> > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> > +
> > +#define CONFIG_FLAG_MASK GENMASK(19, 18)
> > +#define SELECTOR_MASK GENMASK(17, 16)
> > +#define SKIP_CONFIGS_MASK GENMASK(15, 8)
> > +#define CONFIG_TYPE_MASK GENMASK(7, 0)
> > +
> > +enum scmi_pinctrl_protocol_cmd {
> > + PINCTRL_ATTRIBUTES = 0x3,
> > + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> > + PINCTRL_SETTINGS_GET = 0x5,
> > + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> > + PINCTRL_REQUEST = 0x7,
> > + PINCTRL_RELEASE = 0x8,
> > + PINCTRL_NAME_GET = 0x9,
> > + PINCTRL_SET_PERMISSIONS = 0xa
> > +};
> > +
> > +struct scmi_msg_settings_conf {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 attributes;
> > + __le32 configs[];
> > +};
> > +
> > +struct scmi_msg_settings_get {
> > + __le32 identifier;
> > + __le32 attributes;
> > +};
> > +
> > +struct scmi_resp_settings_get {
> > + __le32 function_selected;
> > + __le32 num_configs;
> > + __le32 configs[];
> > +};
> > +
> > +struct scmi_msg_pinctrl_protocol_attributes {
> > + __le32 attributes_low;
> > + __le32 attributes_high;
> > +};
> > +
> > +struct scmi_msg_pinctrl_attributes {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_resp_pinctrl_attributes {
> > + __le32 attributes;
> > + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > +};
> > +
> > +struct scmi_msg_pinctrl_list_assoc {
> > + __le32 identifier;
> > + __le32 flags;
> > + __le32 index;
> > +};
> > +
> > +struct scmi_resp_pinctrl_list_assoc {
> > + __le32 flags;
> > + __le16 array[];
> > +};
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
> > +
>
> As said by Dan...drop this.
>
> > +struct scmi_msg_request {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_group_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > + u32 *group_pins;
> > + u32 nr_pins;
> > +};
> > +
> > +struct scmi_function_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > + u32 *groups;
> > + u32 nr_groups;
> > +};
> > +
> > +struct scmi_pin_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > +};
> > +
> > +struct scmi_pinctrl_info {
> > + u32 version;
> > + int nr_groups;
> > + int nr_functions;
> > + int nr_pins;
> > + struct scmi_group_info *groups;
> > + struct scmi_function_info *functions;
> > + struct scmi_pin_info *pins;
> > +};
> > +
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle
> *ph,
> > + struct scmi_pinctrl_info *pi) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> sizeof(*attr),
> > + &t);
> > + if (ret)
> > + return ret;
> > +
> > + attr = t->rx.buf;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + pi->nr_functions = GET_FUNCTIONS_NR(attr-
> >attributes_high);
> > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> > + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
>
> I was thinking, does make sense to allow a nr_pins == 0 setup to probe
> successfully ? Becasuse is legit for the platform to return zero groups or zero
> functions BUT zero pins is just useless (spec does not say
> anything)
>
> Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail out
> with -EINVAL...

ok, fix in v7.

>
> On the other side looking at the zero groups/function case, that is plausible
> and handled properly by the driver since a 0-bytes devm_kcalloc will return
> ZERO_SIZE_PTR (not NULL) and all the remaining references to pinfo->groups
> and pinfo->functions are guarded by a check on selector >= nr_groups (or >=
> nr_functions), and by scmi_pinctrl_validate_id() so the zero grouyps/fuctions
> scenarios should be safely handled.
>
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + switch (type) {
> > + case PIN_TYPE:
> > + return pi->nr_pins;
> > + case GROUP_TYPE:
> > + return pi->nr_groups;
> > + case FUNCTION_TYPE:
> > + return pi->nr_functions;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type) {
> > + int value;
> > +
> > + value = scmi_pinctrl_count_get(ph, type);
> > + if (value < 0)
> > + return value;
> > +
> > + if (identifier >= value)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type,
> > + u32 selector, char *name,
> > + u32 *n_elems)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_attributes *tx;
> > + struct scmi_resp_pinctrl_attributes *rx;
> > + u32 ext_name_flag;
>
> what about a bool
>
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > + sizeof(*rx), &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + rx = t->rx.buf;
> > + tx->identifier = cpu_to_le32(selector);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + if (n_elems)
> > + *n_elems = NUM_ELEMS(rx->attributes);
> > +
> > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> > +
> > + ext_name_flag = EXT_NAME_FLAG(rx->attributes);
> > + } else
> > + ext_name_flag = 0;
>
> and you dont need this else branch to set ext_name_flag to false, since down
> below you will check ext_flag ONLY if !ret, so it will have surely been set if the
> do_xfer did not fail.
>
> > +
> > + 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)
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET,
> selector,
> > + (u32 *)&type, name,
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
> > +}
> > +
> > +struct scmi_pinctrl_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + u32 *array;
> > +};
> > +
> > +static void iter_pinctrl_assoc_prepare_message(void *message,
> > + u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_pinctrl_list_assoc *msg = message;
> > + const struct scmi_pinctrl_ipriv *p = priv;
> > +
> > + msg->identifier = cpu_to_le32(p->selector);
> > + msg->flags = cpu_to_le32(p->type);
> > + /* Set the number of OPPs to be skipped/already read */
>
> OPP ? .. maybe drop this comment that was cut/pasted from somewhere
> else :D
>
> > + msg->index = cpu_to_le32(desc_index); }
> > +
> > +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv)
> {
> > + const struct scmi_resp_pinctrl_list_assoc *r = response;
> > +
> > + st->num_returned = RETURNED(r->flags);
> > + st->num_remaining = REMAINING(r->flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle
> *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st, void *priv)
> {
> > + const struct scmi_resp_pinctrl_list_assoc *r = response;
> > + struct scmi_pinctrl_ipriv *p = priv;
> > +
> > + p->array[st->desc_index + st->loop_idx] =
> > + le16_to_cpu(r->array[st->loop_idx]);
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + u16 size, u32 *array)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message = iter_pinctrl_assoc_prepare_message,
> > + .update_state = iter_pinctrl_assoc_update_state,
> > + .process_response = iter_pinctrl_assoc_process_response,
> > + };
> > + struct scmi_pinctrl_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .array = array,
> > + };
> > +
> > + if (!array || !size || type == PIN_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, size,
> > + PINCTRL_LIST_ASSOCIATIONS,
> > + sizeof(struct
> scmi_msg_pinctrl_list_assoc),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +struct scmi_settings_get_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + u32 flag;
> > + enum scmi_pinctrl_conf_type *config_types;
> > + u32 *config_values;
> > +};
> > +
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> > + FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->flag == 1)
>
> A boolean like .get_all would be more clear..see down below why you dont
> need a flag 0|1|2
>
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->flag == 1) {
>
> Ditto... see below the explanation
>
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (!p->flag) {
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
>
> Unneeded...see down below for explanation
>
> > +
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .flag = 0,
> > + .config_types = &config_type,
> > + .config_values = config_value,
> > + };
> > +
>
> So this function is used to retrieve configs; as of now, just one, then it could
> be extended to fetch all the configs, and for this it uses the iterators helpers,
> BUT it is not and will not be used to just fetch the selected_function with
> flag_2 (even though is always provided), since in that case you wont get back
> a multi-part SCMI response and so there is no need to use iterators...
>
> IOW... no need here to handle flag_2 scenario and as a consequence I would
> change the ipriv flag to be be a boolean .get_all, like it was, since it is more
> readable (and so you wont need to add any comment..)

ok, so your suggestion is drop the iterators, and only support one config,
right?

Or keep iterators with get_all be passed as a function parameter?

>
> In the future could make sense to add here also a *selected_function output
> param to this function since you will always get it back for free when
> retrieving configs ... BUT for now is just not needed really...no users for this
> case till now...
>
> ...when the time will come that we will need a function_selected_get to be
> issued without retrieveing also the configs I would add a distinct routine that
> crafts properly a SETTINGS_GET with flag_2 without worrying about multi-
> part responses (and with no need for iterators support)
>
> Trying to handle all in here just complicates stuff...
>
> > + if (!config_value || type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u32 nr_configs,
> > + enum scmi_pinctrl_conf_type *config_type,
> > + u32 *config_value)
> > +{
> > + struct scmi_xfer *t;
> > + struct scmi_msg_settings_conf *tx;
> > + u32 attributes;
> > + int ret, i;
> > + u32 configs_in_chunk, conf_num = 0;
> > + u32 chunk;
> > + int max_msg_size = ph->hops->get_max_msg_size(ph);
> > +
> > + if (!config_type || !config_value || type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
> > + while (conf_num < nr_configs) {
> > + chunk = (nr_configs - conf_num > configs_in_chunk) ?
> > + configs_in_chunk : nr_configs - conf_num;
> > +
> > + ret = ph->xops->xfer_get_init(ph,
> PINCTRL_SETTINGS_CONFIGURE,
> > + sizeof(*tx) +
> > + chunk * 2 * sizeof(__le32),
> > + 0, &t);
> > + if (ret)
> > + return ret;
> for consistency I would
> break;
>
> like below and you will exit always from the last return ret;
>
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(selector);
> > + attributes = FIELD_PREP(GENMASK(1, 0), type) |
> > + FIELD_PREP(GENMASK(9, 2), chunk);
> > + tx->attributes = cpu_to_le32(attributes);
> > +
> > + for (i = 0; i < chunk; i++) {
> > + tx->configs[i * 2] =
> > + cpu_to_le32(config_type[conf_num + i]);
> > + tx->configs[i * 2 + 1] =
> > + cpu_to_le32(config_value[conf_num + i]);
> > + }
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + if (ret)
> > + break;
> > +
> > + conf_num += chunk;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle
> *ph,
> > + u32 group,
> > + enum scmi_pinctrl_selector_type
> type,
> > + u32 function_id)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_settings_conf *tx;
> > + u32 attributes;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, group, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> > + sizeof(*tx), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(group);
> > + tx->function_id = cpu_to_le32(function_id);
> > + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
> > + tx->attributes = cpu_to_le32(attributes);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_request(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);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(identifier);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
>
> ..this function ...
>
> > +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> > + u32 pin)
> > +{
> > + return scmi_pinctrl_request(ph, pin, PIN_TYPE); }
> > +
> > +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_request *tx;
> > +
> > + if (type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(identifier);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
>
> ...and this are completely identical, beside the used command msg_id...please
> make it a common workhorse function by adding a param for the command...
>
> > +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle
> > +*ph, u32 pin) {
> > + return scmi_pinctrl_free(ph, pin, PIN_TYPE); }
> > +
>
> ...and convert these _request/_free functions into a pair odf simple wrapper
> invoking the common workhorse...
>
> > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + struct scmi_group_info *group) {
> > + int ret;
> > +
> > + if (!group)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> > + group->name,
> > + &group->nr_pins);
> > + if (ret)
> > + return ret;
> > +
> > + if (!group->nr_pins) {
> > + dev_err(ph->dev, "Group %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + group->group_pins = kmalloc_array(group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group-
> >group_pins);
> > + if (ret) {
> > + kfree(group->group_pins);
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_groups)
> > + return -EINVAL;
> > +
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->groups[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const u32 **pins,
> > + u32 *nr_pins)
> > +{
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!pins || !nr_pins)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_groups)
> > + return -EINVAL;
> > +
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *pins = pi->groups[selector].group_pins;
> > + *nr_pins = pi->groups[selector].nr_pins;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_info(const struct
> scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_function_info *func) {
> > + int ret;
> > +
> > + if (!func)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > + func->name,
> > + &func->nr_groups);
> > + if (ret)
> > + return ret;
> > +
> > + if (!func->nr_groups) {
> > + dev_err(ph->dev, "Function %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
> > + GFP_KERNEL);
> > + if (!func->groups)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> > + func->nr_groups, func->groups);
> > + if (ret) {
> > + kfree(func->groups);
> > + return ret;
> > + }
> > +
> > + func->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_name(const struct
> scmi_protocol_handle *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_functions)
> > + return -EINVAL;
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi-
> >functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->functions[selector].name;
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 *nr_groups,
> > + const u32 **groups)
> > +{
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!groups || !nr_groups)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_functions)
> > + return -EINVAL;
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi-
> >functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *groups = pi->functions[selector].groups;
> > + *nr_groups = pi->functions[selector].nr_groups;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 group)
> > +{
> > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> > +selector); }
> > +
> > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> > + u32 selector, struct scmi_pin_info *pin) {
> > + int ret;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > + pin->name, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + pin->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_pins)
> > + return -EINVAL;
> > +
> > + if (!pi->pins[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->pins[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name)
> > +{
> > + switch (type) {
> > + case PIN_TYPE:
> > + return scmi_pinctrl_get_pin_name(ph, selector, name);
> > + case GROUP_TYPE:
> > + return scmi_pinctrl_get_group_name(ph, selector, name);
> > + case FUNCTION_TYPE:
> > + return scmi_pinctrl_get_function_name(ph, selector, name);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .count_get = scmi_pinctrl_count_get,
> > + .name_get = scmi_pinctrl_name_get,
> > + .group_pins_get = scmi_pinctrl_group_pins_get,
> > + .function_groups_get = scmi_pinctrl_function_groups_get,
> > + .mux_set = scmi_pinctrl_mux_set,
> > + .settings_get = scmi_pinctrl_settings_get,
> > + .settings_conf = scmi_pinctrl_settings_conf,
> > + .pin_request = scmi_pinctrl_pin_request,
> > + .pin_free = scmi_pinctrl_pin_free,
> > +};
> > +
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
>
> ..as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get could return
> -EINVAL here and bail out....not sure that a running setup with zero pins has
> any values (even for testing...) BUT, as said above, I wuld certainly add a
> dev_warn in scmi_pinctrl_attributes_get() when nr_pins == 0

Fix it in v7.

Thanks,
Peng.
>
> Thanks,
> Cristian

2024-03-31 13:44:43

[permalink] [raw]

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

Hi Andy,

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> > From: Peng Fan <[email protected]>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
> ...
>
> > 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
>
> Actually you want to have := here.
>
> > +scmi-protocols-y += pinctrl.o
>
>
>
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
>
> Side note: The -objs has to be -y
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> This is semi-random list of headers. Please, follow IWYU principle (include
> what you use). There are a lot of inclusions I see missing (just in the context of
> this page I see bits.h, types.h, and asm/byteorder.h).

Is there any documentation about this requirement?
Some headers are already included by others.

>
> ...
>
> > +enum scmi_pinctrl_protocol_cmd {
> > + PINCTRL_ATTRIBUTES = 0x3,
> > + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> > + PINCTRL_SETTINGS_GET = 0x5,
> > + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> > + PINCTRL_REQUEST = 0x7,
> > + PINCTRL_RELEASE = 0x8,
> > + PINCTRL_NAME_GET = 0x9,
> > + PINCTRL_SET_PERMISSIONS = 0xa
>
> Leave trailing comma as it's not a termination.
>
> > +};
>
> ...
>
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle
> *ph,
> > + struct scmi_pinctrl_info *pi) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> sizeof(*attr),
> > + &t);
>
> This looks much better on a single line.

Per Cristian, scmi drivers keep 80 max chars.

>
> > + 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;
> > +}
>
> ...
>
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > + sizeof(*rx), &t);
>
> Possible to have on a single line (if you use relaxed 100 limit).
> Or (re)split it more logically:
>
> ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES,
> sizeof(*tx), sizeof(*rx), &t);
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + /*
> > + * If supported overwrite short name with the extended one;
> > + * on error just carry on and use already provided short name.
> > + */
> > + if (!ret && ext_name_flag)
>
> Please, use standard pattern, i.e.
>
> if (ret)
> return ret;
>
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET,
> selector,
> > + (u32 *)&type, name,
>
> Why is an explicit casting needed?

The type is enum, not u32.

>
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
>
> ...
>
> > + iter = ph->hops->iter_response_init(ph, &ops, size,
> > + PINCTRL_LIST_ASSOCIATIONS,
> > + sizeof(struct
> scmi_msg_pinctrl_list_assoc),
> > + &ipriv);
>
> > +
>
> Redundant blank line.
>
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
>
> ...
>
> > + if (p->flag == 1)
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
>
> Be consistent, i.e. if (p->flag == 0)
>
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
>
> ...
>
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
>
> One line?

Scmi drivers use 80 max drivers.
>
> ...
>
> > + if (!p->flag) {
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
>
> With a temporary variable for r->configs[st->loop_idx * 2] the above can be
> written in much better way.

ok. Fix in v7.
>
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
>
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
>
> For the sake of consistency with the above suggestion also temporary for next
> config value.
>
> ...
>
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
>
> > +
>
> Redundant blank line.
>
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
>
> ...
>
> > +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;
>
> When the above is not a dead code?

It could be removed.

>
> > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> > + group->name,
> > + &group->nr_pins);
> > + if (ret)
> > + return ret;
> > +
> > + if (!group->nr_pins) {
> > + dev_err(ph->dev, "Group %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + group->group_pins = kmalloc_array(group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group-
> >group_pins);
> > + if (ret) {
> > + kfree(group->group_pins);
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
>
> ...
>
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
>
> One line?
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > + func->name,
> > + &func->nr_groups);
>
> At least last two lines can be joined.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > + pin->name, NULL);
>
> It's pleany of room on the previous line.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
>
> Ditto.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
>
> Huh?!
>
> Please, get yourself familiar with the scope of devm APIs.

Please teach me if this is wrong.

>
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
> > +
> > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > + sizeof(*pinfo->pins),
> > + GFP_KERNEL);
> > + if (!pinfo->pins)
> > + return -ENOMEM;
> > +
> > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > + sizeof(*pinfo->groups),
> > + GFP_KERNEL);
> > + if (!pinfo->groups)
> > + return -ENOMEM;
> > +
> > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > + sizeof(*pinfo->functions),
> > + GFP_KERNEL);
> > + if (!pinfo->functions)
> > + return -ENOMEM;
> > +
> > + pinfo->version = version;
> > +
> > + return ph->set_priv(ph, pinfo, version); }
> > +
> > +static int scmi_pinctrl_protocol_deinit(const struct
> > +scmi_protocol_handle *ph) {
> > + int i;
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + for (i = 0; i < pi->nr_groups; i++) {
> > + if (pi->groups[i].present) {
> > + kfree(pi->groups[i].group_pins);
> > + pi->groups[i].present = false;
> > + }
> > + }
> > +
> > + for (i = 0; i < pi->nr_functions; i++) {
> > + if (pi->functions[i].present) {
> > + kfree(pi->functions[i].groups);
>
> This is wrong in conjunction with the above.

Yeah.

>
> > + pi->functions[i].present = false;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static const struct scmi_protocol scmi_pinctrl = {
> > + .id = SCMI_PROTOCOL_PINCTRL,
> > + .owner = THIS_MODULE,
> > + .instance_init = &scmi_pinctrl_protocol_init,
> > + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> > + .ops = &pinctrl_proto_ops,
> > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
>
> > +
>
> Redundant blank line.

Fix in v7

Thanks,
Peng.
>
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
>
>
> --
> With Best Regards,
> Andy Shevchenko
>

2024-04-02 07:49:08

[permalink] [raw]

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

On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> Hi Andy,
>

Hi Peng,

> > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> > protocol basic support
> >
> > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> > > From: Peng Fan <[email protected]>
> > >
> > > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
> > ...
> >
> > > 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
> >
> > Actually you want to have := here.
> >
> > > +scmi-protocols-y += pinctrl.o
> >
> >
> >
> > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > > $(scmi-transport-y)
> >
> > Side note: The -objs has to be -y
> >
> > ...
> >
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> >
> > This is semi-random list of headers. Please, follow IWYU principle (include
> > what you use). There are a lot of inclusions I see missing (just in the context of
> > this page I see bits.h, types.h, and asm/byteorder.h).
>
> Is there any documentation about this requirement?
> Some headers are already included by others.
>

Andy made (mostly) the same remarks on this same patch ~1-year ago on
this same patch while it was posted by Oleksii.

And I told that time that most of the remarks around devm_ usage were
wrong due to how the SCMI core handles protocol initialization (using a
devres group transparently).

This is what I answered that time.

https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t

I wont repeat myself, but, in a nutshell the memory allocation like it
is now is fine: a bit happens via devm_ at protocol initialization, the
other is doe via explicit kmalloc at runtime and freed via kfree at
remove time (if needed...i.e. checking the present flag of some structs)

I'll made further remarks on v7 that you just posted.

Thanks,
Cristian

2024-04-02 13:07:04

[permalink] [raw]

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

On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
<[email protected]> wrote:
> On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:

..

> > > > +#include <linux/module.h>
> > > > +#include <linux/scmi_protocol.h>
> > > > +#include <linux/slab.h>
> > >
> > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > this page I see bits.h, types.h, and asm/byteorder.h).
> >
> > Is there any documentation about this requirement?
> > Some headers are already included by others.

The documentation here is called "a common sense".
The C language is built like this and we expect that nobody will
invest into the dependency hell that we have already, that's why IWYU
principle, please follow it.

> Andy made (mostly) the same remarks on this same patch ~1-year ago on
> this same patch while it was posted by Oleksii.
>
> And I told that time that most of the remarks around devm_ usage were
> wrong due to how the SCMI core handles protocol initialization (using a
> devres group transparently).
>
> This is what I answered that time.
>
> https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
>
> I wont repeat myself, but, in a nutshell the memory allocation like it
> is now is fine: a bit happens via devm_ at protocol initialization, the
> other is doe via explicit kmalloc at runtime and freed via kfree at
> remove time (if needed...i.e. checking the present flag of some structs)

This sounds like a mess. devm_ is expected to be used only for the
->probe() stage, otherwise you may consider cleanup.h (__free() macro)
to have automatic free at the paths where memory is not needed.

And the function naming doesn't suggest that you have a probe-remove
pair. Moreover, if the init-deinit part is called in the probe-remove,
the devm_ must not be mixed with non-devm ones, as it breaks the order
and leads to subtle mistakes.

> I'll made further remarks on v7 that you just posted.

--
With Best Regards,
Andy Shevchenko

2024-04-02 14:26:55

[permalink] [raw]

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

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> <[email protected]> wrote:
> > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
>
> ...
>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/scmi_protocol.h> #include <linux/slab.h>
> > > >
> > > > This is semi-random list of headers. Please, follow IWYU principle
> > > > (include what you use). There are a lot of inclusions I see
> > > > missing (just in the context of this page I see bits.h, types.h, and
> asm/byteorder.h).
> > >
> > > Is there any documentation about this requirement?
> > > Some headers are already included by others.
>
> The documentation here is called "a common sense".
> The C language is built like this and we expect that nobody will invest into the
> dependency hell that we have already, that's why IWYU principle, please
> follow it.
>
> > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > this same patch while it was posted by Oleksii.
> >
> > And I told that time that most of the remarks around devm_ usage were
> > wrong due to how the SCMI core handles protocol initialization (using
> > a devres group transparently).
> >
> > This is what I answered that time.
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937-
> lin%2F%2
> >
> 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d
> c5315bed
> >
> 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583
> 40430%7CUn
> >
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1haW
> >
> wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6
> K3HF%2FofD
> > Yu7j0Lrm8dN5k%3D&reserved=0
> >
> > I wont repeat myself, but, in a nutshell the memory allocation like it
> > is now is fine: a bit happens via devm_ at protocol initialization,
> > the other is doe via explicit kmalloc at runtime and freed via kfree
> > at remove time (if needed...i.e. checking the present flag of some
> > structs)
>
> This sounds like a mess. devm_ is expected to be used only for the
> ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> to have automatic free at the paths where memory is not needed.
>
> And the function naming doesn't suggest that you have a probe-remove pair.
> Moreover, if the init-deinit part is called in the probe-remove, the devm_
> must not be mixed with non-devm ones, as it breaks the order and leads to
> subtle mistakes.

I am new to __free() honestly. I'll let Cristian and Sudeep to comment on
what should I do for v8.

Thanks,
Peng.

>
> > I'll made further remarks on v7 that you just posted.
>
> --
> With Best Regards,
> Andy Shevchenko

2024-04-02 15:14:07

[permalink] [raw]

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

Hi Andy,

> Subject: RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2
> > pincontrol protocol basic support
> >
> > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > <[email protected]> wrote:
> > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> >
> > ...
> >
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/scmi_protocol.h> #include <linux/slab.h>
> > > > >
> > > > > This is semi-random list of headers. Please, follow IWYU
> > > > > principle (include what you use). There are a lot of inclusions
> > > > > I see missing (just in the context of this page I see bits.h,
> > > > > types.h, and
> > asm/byteorder.h).
> > > >
> > > > Is there any documentation about this requirement?
> > > > Some headers are already included by others.
> >
> > The documentation here is called "a common sense".
> > The C language is built like this and we expect that nobody will
> > invest into the dependency hell that we have already, that's why IWYU
> > principle, please follow it.
> >
> > > Andy made (mostly) the same remarks on this same patch ~1-year ago
> > > on this same patch while it was posted by Oleksii.
> > >
> > > And I told that time that most of the remarks around devm_ usage
> > > were wrong due to how the SCMI core handles protocol initialization
> > > (using a devres group transparently).
> > >
> > > This is what I answered that time.
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > re
> > > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937-
> > lin%2F%2
> > >
> >
> 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d
> > c5315bed
> > >
> >
> 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583
> > 40430%7CUn
> > >
> >
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> > k1haW
> > >
> >
> wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6
> > K3HF%2FofD
> > > Yu7j0Lrm8dN5k%3D&reserved=0
> > >
> > > I wont repeat myself, but, in a nutshell the memory allocation like
> > > it is now is fine: a bit happens via devm_ at protocol
> > > initialization, the other is doe via explicit kmalloc at runtime and
> > > freed via kfree at remove time (if needed...i.e. checking the
> > > present flag of some
> > > structs)
> >
> > This sounds like a mess. devm_ is expected to be used only for the
> > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > to have automatic free at the paths where memory is not needed.
> >
> > And the function naming doesn't suggest that you have a probe-remove
> pair.
> > Moreover, if the init-deinit part is called in the probe-remove, the
> > devm_ must not be mixed with non-devm ones, as it breaks the order and
> > leads to subtle mistakes.
>
> I am new to __free() honestly. I'll let Cristian and Sudeep to comment on
> what should I do for v8.

Just give a look. But since most scmi firmware drivers are using
devm_x APIs in protocol init. I would follow the style to use
devm_x as of now.

And for pinctrl protocol deinit phase, I will add a comment on why
use kfree and what it is to free.

For the __free macro, people drop all the scmi firmware drivers
using devm_x APIs in init phase in a future patch.

Is this ok?

Thanks,
Peng.

>
> Thanks,
> Peng.
>
> >
> > > I'll made further remarks on v7 that you just posted.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

2024-04-02 15:59:01

[permalink] [raw]

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

On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> <[email protected]> wrote:
> > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
>
> ...
>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/scmi_protocol.h>
> > > > > +#include <linux/slab.h>
> > > >
> > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > >
> > > Is there any documentation about this requirement?
> > > Some headers are already included by others.
>
> The documentation here is called "a common sense".
> The C language is built like this and we expect that nobody will
> invest into the dependency hell that we have already, that's why IWYU
> principle, please follow it.
>

Yes, but given that we have a growing number of SCMI protocols there is a
common local protocols.h header to group all includes needed by any
protocols: the idea behind this (and the devm_ saga down below) was to ease
development of protocols, since there are lots of them and growing, given
the SCMI spec is extensible.

> > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > this same patch while it was posted by Oleksii.
> >
> > And I told that time that most of the remarks around devm_ usage were
> > wrong due to how the SCMI core handles protocol initialization (using a
> > devres group transparently).
> >
> > This is what I answered that time.
> >
> > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> >
> > I wont repeat myself, but, in a nutshell the memory allocation like it
> > is now is fine: a bit happens via devm_ at protocol initialization, the
> > other is doe via explicit kmalloc at runtime and freed via kfree at
> > remove time (if needed...i.e. checking the present flag of some structs)
>
> This sounds like a mess. devm_ is expected to be used only for the
> ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> to have automatic free at the paths where memory is not needed.
>

Indeed, this protocol_init code is called by the SCMI core once for all when
an SCMI driver tries at first to use this specific protocol by 'getting' its
protocol_ops, so it is indeed called inside the probe chain of the driver:
at this point you *can* decide to use devres to allocate memory and be assured
that if the init fails, or when the driver cease to use this protocol (calling
its remove()) and no other driver is using it, all the stuff that have been
allocated related to this protocol will be released by the core for you.
(using an internal devres group)

Without this you should handle manually all the deallocation manually on
the init error-paths AND also provide all the cleanup explicitly when
the protocol is no more used by any driver (multiple users of the same
protocol instance are possible)...for all protocols.

This is/was handy since, till now, all the SCMI querying and resources
allocation happened anyway all at once at init time...

..the mess, as you kindly called it, derives from the fact that this specific
protocol is the first and only one that does NOT allocate all that it needs
during the initialization (to minimize needless allocs for a lot of possibly
unused resources) and this lazy-initialization phase, done after init at runtime,
must be handled manually since it cannot be managed by the devres group that is
open/clsoed around init by the SCMI core.

I dont like particularly this split allocation but it has a reason and any
other solution seems more messy to me at the moment.

And I dont feel like changing all the SCMI protocol initialziation core code
(that address a lot more under the hood) is a desirable solution to address a
non-existent problem really.

> And the function naming doesn't suggest that you have a probe-remove
> pair. Moreover, if the init-deinit part is called in the probe-remove,
> the devm_ must not be mixed with non-devm ones, as it breaks the order
> and leads to subtle mistakes.
>

Initialization order is enforced by SCMI core like this:

@driver_probe->get_protocol_ops()
@core/get_protocol_ops
-> devres_group_open()
-> protocol_init->devm_*()
-> devres_group_close()
-> driver_probing

@runtime optional explicit_lazy_kmallocs inside the protocol

@driver_remove->put_protocol_ops()
@core/put_protocol_ops()
-> protocol_denit->optional_explicit_kfree_of_the_above
-> devres_group_release()
-> driver_removing

.. dont think there's an ordering problem.

..note that the ph->dev provided in the protocol_init and used by devm_
is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
it is an internal SCMI device associated with the core SCMI stack probing and
allocations, within which a devres group for the specific protocol is created
when that specific protocol is initialized...protocols are not fully
fledged drivers are just bits of the SCMI stack that are initialized when needed
(and possibly also loaded when needed for vendor protocols) and
de-initialzed when no more SCMI driver users exist for that protocol.

Thanks,
Cristian

2024-04-02 16:40:42

[permalink] [raw]

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

On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
<[email protected]> wrote:
> On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > <[email protected]> wrote:
> > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:

..

> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/scmi_protocol.h>
> > > > > > +#include <linux/slab.h>
> > > > >
> > > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > > >
> > > > Is there any documentation about this requirement?
> > > > Some headers are already included by others.
> >
> > The documentation here is called "a common sense".
> > The C language is built like this and we expect that nobody will
> > invest into the dependency hell that we have already, that's why IWYU
> > principle, please follow it.
>
> Yes, but given that we have a growing number of SCMI protocols there is a
> common local protocols.h header to group all includes needed by any
> protocols: the idea behind this (and the devm_ saga down below) was to ease
> development of protocols, since there are lots of them and growing, given
> the SCMI spec is extensible.

Yes, and what you are effectively suggesting is: "Technical debt? Oh,
fine, we do not care!" This is not good. I'm in a long term of
cleaning up the dependency hell in the kernel (my main focus is
kernel.h for now) and I am talking from my experience. I do not like
what people are doing in 95% of the code, that's why I really want to
stop the bad practices as soon as possible.

Last to add, but not least is that your code may be used as an example
for others, hence we really have to do our best in order to avoid bad
design, practices, and cargo cults. If this requires more refactoring
of the existing code, then do it sooner than later.

..

> > > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > > this same patch while it was posted by Oleksii.
> > >
> > > And I told that time that most of the remarks around devm_ usage were
> > > wrong due to how the SCMI core handles protocol initialization (using a
> > > devres group transparently).
> > >
> > > This is what I answered that time.
> > >
> > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> > >
> > > I wont repeat myself, but, in a nutshell the memory allocation like it
> > > is now is fine: a bit happens via devm_ at protocol initialization, the
> > > other is doe via explicit kmalloc at runtime and freed via kfree at
> > > remove time (if needed...i.e. checking the present flag of some structs)
> >
> > This sounds like a mess. devm_ is expected to be used only for the
> > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > to have automatic free at the paths where memory is not needed.
>
> Indeed, this protocol_init code is called by the SCMI core once for all when
> an SCMI driver tries at first to use this specific protocol by 'getting' its
> protocol_ops, so it is indeed called inside the probe chain of the driver:
> at this point you *can* decide to use devres to allocate memory and be assured
> that if the init fails, or when the driver cease to use this protocol (calling
> its remove()) and no other driver is using it, all the stuff that have been
> allocated related to this protocol will be released by the core for you.
> (using an internal devres group)
>
> Without this you should handle manually all the deallocation manually on
> the init error-paths AND also provide all the cleanup explicitly when
> the protocol is no more used by any driver (multiple users of the same
> protocol instance are possible)...for all protocols.

Yes. Is it a problem?

> This is/was handy since, till now, all the SCMI querying and resources
> allocation happened anyway all at once at init time...
>
> ...the mess, as you kindly called it, derives from the fact that this specific
> protocol is the first and only one that does NOT allocate all that it needs
> during the initialization (to minimize needless allocs for a lot of possibly
> unused resources) and this lazy-initialization phase, done after init at runtime,
> must be handled manually since it cannot be managed by the devres group that is
> open/clsoed around init by the SCMI core.
>
> I dont like particularly this split allocation but it has a reason and any
> other solution seems more messy to me at the moment.
>
> And I dont feel like changing all the SCMI protocol initialziation core code
> (that address a lot more under the hood) is a desirable solution to address a
> non-existent problem really.
>
> > And the function naming doesn't suggest that you have a probe-remove
> > pair. Moreover, if the init-deinit part is called in the probe-remove,
> > the devm_ must not be mixed with non-devm ones, as it breaks the order
> > and leads to subtle mistakes.
>
> Initialization order is enforced by SCMI core like this:
>
> @driver_probe->get_protocol_ops()
> @core/get_protocol_ops
> -> devres_group_open()
> -> protocol_init->devm_*()
> -> devres_group_close()
> -> driver_probing
>
> @runtime optional explicit_lazy_kmallocs inside the protocol
>
> @driver_remove->put_protocol_ops()
> @core/put_protocol_ops()
> -> protocol_denit->optional_explicit_kfree_of_the_above
> -> devres_group_release()
> -> driver_removing
>
> ... dont think there's an ordering problem.

The mess with devm_ vs. non-devm is quite easy to achieve. You are
probably out of the control of what the protocol driver wants to do in
the init. Is the usage of devm (which APIs and in which order can be
used) WRT SCMI documented somewhere?

Misuse of devm is a common issue, I'm not surprised it will hit your
subsystem one day with such an approach.

> ...note that the ph->dev provided in the protocol_init and used by devm_
> is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
> it is an internal SCMI device associated with the core SCMI stack probing and
> allocations, within which a devres group for the specific protocol is created
> when that specific protocol is initialized...protocols are not fully
> fledged drivers are just bits of the SCMI stack that are initialized when needed
> (and possibly also loaded when needed for vendor protocols) and
> de-initialzed when no more SCMI driver users exist for that protocol.

P.S. I guess from now on it's your call, but this code and in case
other drivers use similar, is badly written. I hope some documentation
exists to at least justify all this mess and explaining why there no
and (what's really important) will never be a problem.

--
With Best Regards,
Andy Shevchenko

2024-04-03 08:16:40

[permalink] [raw]

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

On Tue, Apr 02, 2024 at 07:39:44PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
> <[email protected]> wrote:
> > On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > > <[email protected]> wrote:
> > > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
>
> ...
>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/scmi_protocol.h>
> > > > > > > +#include <linux/slab.h>
> > > > > >
> > > > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > > > >
> > > > > Is there any documentation about this requirement?
> > > > > Some headers are already included by others.
> > >
> > > The documentation here is called "a common sense".
> > > The C language is built like this and we expect that nobody will
> > > invest into the dependency hell that we have already, that's why IWYU
> > > principle, please follow it.
> >
> > Yes, but given that we have a growing number of SCMI protocols there is a
> > common local protocols.h header to group all includes needed by any
> > protocols: the idea behind this (and the devm_ saga down below) was to ease
> > development of protocols, since there are lots of them and growing, given
> > the SCMI spec is extensible.
>
> Yes, and what you are effectively suggesting is: "Technical debt? Oh,
> fine, we do not care!" This is not good. I'm in a long term of
> cleaning up the dependency hell in the kernel (my main focus is
> kernel.h for now) and I am talking from my experience. I do not like
> what people are doing in 95% of the code, that's why I really want to
> stop the bad practices as soon as possible.
>

Not at all, the aim was exactly the opposite, avoiding that some protocol
could have been written without all the needed includes: since a basic set
of headers is definitely common to any protocol you may think to write,
grouping all there was meant to avoid this...I thought that by moving the
problem away in one single internal common header was easier to monitor.

I certainly maybe wrong, but I dont see how you can deduce I dont care...

..and maybe, only maybe, what that 95% of people is trying to do in their
horrible code is to deliver the best reasonably possible thing within their
timeline while you are barking at them in chase of never to be released utter
perfection.

> Last to add, but not least is that your code may be used as an example
> for others, hence we really have to do our best in order to avoid bad
> design, practices, and cargo cults. If this requires more refactoring
> of the existing code, then do it sooner than later.
>
> ...
>
> > > > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > > > this same patch while it was posted by Oleksii.
> > > >
> > > > And I told that time that most of the remarks around devm_ usage were
> > > > wrong due to how the SCMI core handles protocol initialization (using a
> > > > devres group transparently).
> > > >
> > > > This is what I answered that time.
> > > >
> > > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> > > >
> > > > I wont repeat myself, but, in a nutshell the memory allocation like it
> > > > is now is fine: a bit happens via devm_ at protocol initialization, the
> > > > other is doe via explicit kmalloc at runtime and freed via kfree at
> > > > remove time (if needed...i.e. checking the present flag of some structs)
> > >
> > > This sounds like a mess. devm_ is expected to be used only for the
> > > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > > to have automatic free at the paths where memory is not needed.
> >
> > Indeed, this protocol_init code is called by the SCMI core once for all when
> > an SCMI driver tries at first to use this specific protocol by 'getting' its
> > protocol_ops, so it is indeed called inside the probe chain of the driver:
> > at this point you *can* decide to use devres to allocate memory and be assured
> > that if the init fails, or when the driver cease to use this protocol (calling
> > its remove()) and no other driver is using it, all the stuff that have been
> > allocated related to this protocol will be released by the core for you.
> > (using an internal devres group)
> >
> > Without this you should handle manually all the deallocation manually on
> > the init error-paths AND also provide all the cleanup explicitly when
> > the protocol is no more used by any driver (multiple users of the same
> > protocol instance are possible)...for all protocols.
>
> Yes. Is it a problem?
>

Well, no, but is it not a repetitive and error-prone process ?
Is it not the exact reason why devres management exist in first place, to avoid
repetitive manual alloc/free of resources and related pitfalls ? (even though
certainly it is normally used in a more conventional and straightforward way)

The idea was to give some sort of aid in the SCMI stack for writing protocols,
so regarding mem_mgmt, I just built on top of devres facilities, not invented
anything, to try to avoid repetitions and let the core handle mem allocs/free
during the probe phases as much as possible: in pinctrl case would be
particularly trivial to instead manually allocate stuff at init (due to many
lazy delayed allocations) but other protocols need a lot more to be done at init,
frequently in a loop to allocate multiple resources descriptors, and manually
undoing all of that on each error-path and on cleanup is definitely error-prone
and a pain.

Last but not least, this whole thing was designed to address the needs of the
protocols that existed at that time....it is only now with pinctrl lazy-allocations
at runtime that the ugly cohexistence of devm_ and non-devm allocations became a
thing....so clearly the thing needs to be improved/rethinked...even dropped if no
more fitting...

.. or alternatively since devres allocations are anyway optional, you could just
use regular kmalloc/kfree for this protocol and avoid this dual handling...

..this was just to put things in context...and I'll happily let Sudeep decide
what he prefers in the immediate for pinctrl or more in general about all the
scmi devres, that I've got enough of these pleasant interactions for now...

Thanks,
Cristian

2024-04-03 09:20:00