This RFC patch series is intended to introduce the potential generic driver for
pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0].
On ARM-based systems, a separate Cortex-M based System Control Processor (SCP)
provides control on pins, as well as with power, clocks, reset controllers. In this case,
kernel should use one of the possible transports, described in [0] to access SCP and
control clocks/power-domains etc. This driver is using SMC transport to communicate with SCP via
SCMI protocol and access to the Pin Control Subsystem.
The provided driver consists of 2 parts:
- firmware/arm_scmi/pinctrl.c - the SCMI pinctrl protocol inmplementation
responsible for the communication with SCP firmware.
- drivers/pinctrl/pinctrl-scmi.c - pinctrl driver, which is using pinctrl
protocol implementation to access all necessary data.
Configuration:
The scmi-pinctrl driver can be configured using DT bindings.
For example:
/ {
cpu_scp_shm: scp-shmem@0x53FF0000 {
compatible = "arm,scmi-shmem";
reg = <0x0 0x53FF0000 0x0 0x1000>;
};
firmware {
scmi {
compatible = "arm,scmi-smc";
arm,smc-id = <0x82000002>;
shmem = <&cpu_scp_shm>;
#address-cells = <1>;
#size-cells = <0>;
scmi_pinctrl: protocol@19 {
reg = <0x18>;
#pinctrl-cells = <0>;
i2c2_pins: i2c2 {
groups = "i2c2_a";
function = "i2c2";
};
};
};
};
};
&pfc {
/delete-node/i2c2;
};
So basically, it's enough to move pfc subnode, which configures pin group that should work through
SCMI protocol to scmi_pinctrl node. The current driver implementation is using generic pinctrl dt_node
format.
I've tested this driver on the Renesas H3ULCB Kingfisher board with pinctrl driver ported to the
Arm-trusted-firmware. Unfortunately, not all hardware was possible to test because the Renesas
pinctrl driver has gaps in pins and groups numeration, when Spec [0] requires pins, groups and
functions numerations to be 0..n without gaps.
This implementation still reqires some features, such as gpio support, but I've posted it as RFC to
start the discussion regarding the driver format.
[0] https://developer.arm.com/documentation/den0056/latest
---
Changes v1 -> v2:
- rebase patches to the latest kernel version
- use protocol helpers in the pinctrl scmi protocol driver implementation
- reworked pinctrl_ops. Removed similar calls to simplify the interface
- implementation of the .instance_deinit callback to properly clean resources
- add description of the pinctrl protocol to the device-tree schema
---
Oleksii Moisieiev (3):
firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
pinctrl: Implementation of the generic scmi-pinctrl driver
dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol
.../bindings/firmware/arm,scmi.yaml | 77 ++
MAINTAINERS | 7 +
drivers/firmware/arm_scmi/Makefile | 3 +-
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 932 ++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
drivers/pinctrl/Kconfig | 9 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++
include/linux/scmi_protocol.h | 47 +
10 files changed, 1656 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
create mode 100644 drivers/pinctrl/pinctrl-scmi.c
--
2.25.1
scmi: Introduce pinctrl SCMI protocol driver
Add basic implementation of the SCMI v3.2 pincontrol protocol
excluding GPIO support. All pinctrl related callbacks and operations
are exposed in the include/linux/scmi_protocol.h
Signed-off-by: Oleksii Moisieiev <[email protected]>
---
MAINTAINERS | 6 +
drivers/firmware/arm_scmi/Makefile | 3 +-
drivers/firmware/arm_scmi/driver.c | 2 +
drivers/firmware/arm_scmi/pinctrl.c | 932 ++++++++++++++++++++++++++
drivers/firmware/arm_scmi/protocols.h | 1 +
include/linux/scmi_protocol.h | 47 ++
6 files changed, 990 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e64787aace8..0d251ebac437 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20317,6 +20317,12 @@ F: include/linux/sc[mp]i_protocol.h
F: include/trace/events/scmi.h
F: include/uapi/linux/virtio_scmi.h
+PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
+M: Oleksii Moisieiev <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/firmware/arm_scmi/pinctrl.c
+
SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel <[email protected]>
L: [email protected]
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index b31d78fa66cc..071ac65f22b9 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -3,6 +3,7 @@ scmi-bus-y = bus.o
scmi-core-objs := $(scmi-bus-y)
scmi-driver-y = driver.o notify.o
+
scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
@@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
-scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index dbc474ff62b7..fc7ba587ae6e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3021,6 +3021,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);
}
@@ -3038,6 +3039,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..1c643d21390f
--- /dev/null
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -0,0 +1,932 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Pinctrl Protocol
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include "protocols.h"
+
+#define SET_TYPE(x) FIELD_PREP(GENMASK(1, 0), (x))
+
+#define REG_TYPE_BITS GENMASK(9, 8)
+#define REG_CONFIG GENMASK(7, 0)
+
+#define GET_GROUPS_NR(x) FIELD_GET(GENMASK(31, 16), (x))
+#define GET_PINS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
+#define GET_FUNCTIONS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
+
+#define EXT_NAME_FLAG(x) FIELD_GET(BIT(31), (x))
+#define NUM_ELEMS(x) FIELD_GET(GENMASK(15, 0), (x))
+
+#define REMAINING(x) FIELD_GET(GENMASK(31, 16), (x))
+#define RETURNED(x) FIELD_GET(GENMASK(11, 0), (x))
+
+enum scmi_pinctrl_protocol_cmd {
+ PINCTRL_ATTRIBUTES = 0x3,
+ PINCTRL_LIST_ASSOCIATIONS = 0x4,
+ PINCTRL_CONFIG_GET = 0x5,
+ PINCTRL_CONFIG_SET = 0x6,
+ PINCTRL_FUNCTION_SELECT = 0x7,
+ PINCTRL_REQUEST = 0x8,
+ PINCTRL_RELEASE = 0x9,
+ PINCTRL_NAME_GET = 0xa,
+ PINCTRL_SET_PERMISSIONS = 0xb
+};
+
+struct scmi_msg_conf_set {
+ __le32 identifier;
+ __le32 attributes;
+ __le32 config_value;
+};
+
+struct scmi_msg_conf_get {
+ __le32 identifier;
+ __le32 attributes;
+};
+
+struct scmi_msg_pinctrl_protocol_attributes {
+ __le32 attributes_low;
+ __le32 attributes_high;
+};
+
+struct scmi_msg_ext_name {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_resp_ext_name {
+ __le32 flags;
+ u8 name[SCMI_MAX_STR_SIZE];
+};
+
+struct scmi_msg_pinctrl_attributes {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_resp_pinctrl_attributes {
+ __le32 attributes;
+ u8 name[SCMI_SHORT_NAME_MAX_SIZE];
+};
+
+struct scmi_msg_pinctrl_list_assoc {
+ __le32 identifier;
+ __le32 flags;
+ __le32 index;
+};
+
+struct scmi_resp_pinctrl_list_assoc {
+ __le32 flags;
+ __le16 array[];
+};
+
+struct scmi_msg_func_set {
+ __le32 identifier;
+ __le32 function_id;
+ __le32 flags;
+};
+
+struct scmi_msg_request {
+ __le32 identifier;
+ __le32 flags;
+};
+
+struct scmi_group_info {
+ bool present;
+ char *name;
+ unsigned int *group_pins;
+ unsigned int nr_pins;
+};
+
+struct scmi_function_info {
+ bool present;
+ char *name;
+ unsigned int *groups;
+ unsigned int nr_groups;
+};
+
+struct scmi_pin_info {
+ bool present;
+ char *name;
+};
+
+struct scmi_pinctrl_info {
+ u32 version;
+ int nr_groups;
+ int nr_functions;
+ int nr_pins;
+ struct scmi_group_info *groups;
+ struct scmi_function_info *functions;
+ struct scmi_pin_info *pins;
+};
+
+static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
+ struct scmi_pinctrl_info *pi)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_protocol_attributes *attr;
+
+ if (!pi)
+ return -EINVAL;
+
+ ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
+ 0, sizeof(*attr), &t);
+ if (ret)
+ return ret;
+
+ attr = t->rx.buf;
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ pi->nr_functions =
+ le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
+ pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
+ pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));
+ }
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type)
+{
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph)
+ return -ENODEV;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -ENODEV;
+
+ switch (type) {
+ case PIN_TYPE:
+ return pi->nr_pins;
+ case GROUP_TYPE:
+ return pi->nr_groups;
+ case FUNCTION_TYPE:
+ return pi->nr_functions;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ int value;
+
+ if (!ph)
+ return -ENODEV;
+
+ value = scmi_pinctrl_get_count(ph, type);
+ if (value < 0)
+ return value;
+
+ if (identifier >= value)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int scmi_pinctrl_get_ext_name(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type,
+ char **name)
+{
+ struct scmi_xfer *t;
+ int ret = 0;
+ struct scmi_msg_ext_name *tx;
+ struct scmi_resp_ext_name *rx;
+
+ if (!ph || !name)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_NAME_GET, sizeof(*tx),
+ sizeof(*rx), &t);
+
+ tx = t->tx.buf;
+ rx = t->rx.buf;
+ tx->identifier = identifier;
+ tx->flags = SET_TYPE(cpu_to_le32(type));
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (ret)
+ goto out;
+
+ if (rx->flags) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ *name = kasprintf(GFP_KERNEL, "%s", rx->name);
+ if (!*name)
+ ret = -ENOMEM;
+ out:
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type,
+ u32 selector, char **name,
+ unsigned int *n_elems)
+{
+ int ret = 0;
+ struct scmi_xfer *t;
+ struct scmi_msg_pinctrl_attributes *tx;
+ struct scmi_resp_pinctrl_attributes *rx;
+
+ if (!ph || !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 = selector;
+ tx->flags = SET_TYPE(cpu_to_le32(type));
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (ret)
+ goto out;
+
+ *n_elems = NUM_ELEMS(rx->attributes);
+
+ if (!EXT_NAME_FLAG(rx->attributes)) {
+ *name = kasprintf(GFP_KERNEL, "%s", rx->name);
+ if (!*name)
+ ret = -ENOMEM;
+ } else {
+ ret = scmi_pinctrl_get_ext_name(ph, selector, type, name);
+ }
+ out:
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+struct scmi_pinctrl_ipriv {
+ u32 selector;
+ enum scmi_pinctrl_selector_type type;
+ unsigned int *array;
+};
+
+static void iter_pinctrl_assoc_prepare_message(void *message,
+ unsigned int desc_index,
+ const void *priv)
+{
+ struct scmi_msg_pinctrl_list_assoc *msg = message;
+ const struct scmi_pinctrl_ipriv *p = priv;
+
+ msg->identifier = cpu_to_le32(p->selector);
+ msg->flags = SET_TYPE(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 = le32_to_cpu(RETURNED(r->flags));
+ st->num_remaining = le32_to_cpu(REMAINING(r->flags));
+
+ return 0;
+}
+
+static int
+iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
+ const void *response,
+ struct scmi_iterator_state *st, void *priv)
+{
+ const struct scmi_resp_pinctrl_list_assoc *r = response;
+ struct scmi_pinctrl_ipriv *p = priv;
+
+ p->array[st->desc_index + st->loop_idx] =
+ le16_to_cpu(r->array[st->loop_idx]);
+
+ return 0;
+}
+
+static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u16 size, unsigned int *array)
+{
+ int ret;
+ void *iter;
+ struct scmi_iterator_ops ops = {
+ .prepare_message = iter_pinctrl_assoc_prepare_message,
+ .update_state = iter_pinctrl_assoc_update_state,
+ .process_response = iter_pinctrl_assoc_process_response,
+ };
+ struct scmi_pinctrl_ipriv ipriv = {
+ .selector = selector,
+ .type = type,
+ .array = array,
+ };
+
+ if (!ph || !array || !size)
+ return -EINVAL;
+
+ if (type == PIN_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ iter = ph->hops->iter_response_init(ph, &ops, size,
+ PINCTRL_LIST_ASSOCIATIONS,
+ sizeof(struct scmi_msg_pinctrl_list_assoc),
+ &ipriv);
+
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
+
+ return ph->hops->iter_response_run(iter);
+}
+
+static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, u32 *config_value)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_conf_get *tx;
+ __le32 *le_config;
+ u32 attributes = 0;
+ int ret;
+
+ if (!ph || !config_value || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx),
+ sizeof(*le_config), &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ le_config = t->rx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ attributes = FIELD_PREP(REG_TYPE_BITS, type) |
+ FIELD_PREP(REG_CONFIG, config_type);
+
+ tx->attributes = cpu_to_le32(attributes);
+
+ ret = ph->xops->do_xfer(ph, t);
+
+ if (!ret)
+ *config_value = le32_to_cpu(*le_config);
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_set_config(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, u32 config_value)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_conf_set *tx;
+ u32 attributes = 0;
+ int ret;
+
+ if (!ph || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, selector, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_SET,
+ sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(selector);
+ attributes = FIELD_PREP(REG_TYPE_BITS, type) |
+ FIELD_PREP(REG_CONFIG, config_type);
+ tx->attributes = cpu_to_le32(attributes);
+ tx->config_value = cpu_to_le32(config_value);
+
+ ret = ph->xops->do_xfer(ph, t);
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type,
+ u32 function_id)
+{
+ struct scmi_xfer *t;
+ struct scmi_msg_func_set *tx;
+ int ret;
+
+ if (!ph || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_FUNCTION_SELECT,
+ sizeof(*tx), 0, &t);
+ if (ret)
+ return ret;
+
+ tx = t->tx.buf;
+ tx->identifier = cpu_to_le32(identifier);
+ tx->function_id = cpu_to_le32(function_id);
+ tx->flags = SET_TYPE(cpu_to_le32(type));
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ struct scmi_xfer *t;
+ int ret;
+ struct scmi_msg_request *tx;
+
+ if (!ph || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx),
+ 0, &t);
+
+ tx = t->tx.buf;
+ tx->identifier = identifier;
+ tx->flags = SET_TYPE(cpu_to_le32(type));
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_request_pin(const struct scmi_protocol_handle *ph,
+ u32 pin)
+{
+ return scmi_pinctrl_request(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
+ u32 identifier,
+ enum scmi_pinctrl_selector_type type)
+{
+ struct scmi_xfer *t;
+ int ret;
+ struct scmi_msg_request *tx;
+
+ if (!ph || type == FUNCTION_TYPE)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_validate_id(ph, identifier, type);
+ if (ret)
+ return ret;
+
+ ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
+ sizeof(*tx), 0, &t);
+
+ tx = t->tx.buf;
+ tx->identifier = identifier;
+ tx->flags = SET_TYPE(cpu_to_le32(type));
+
+ ret = ph->xops->do_xfer(ph, t);
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_pinctrl_free_pin(const struct scmi_protocol_handle *ph, u32 pin)
+{
+ return scmi_pinctrl_free(ph, pin, PIN_TYPE);
+}
+
+static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_group_info *group)
+{
+ int ret = 0;
+
+ if (!ph || !group)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
+ &group->name,
+ &group->nr_pins);
+ if (ret)
+ return ret;
+
+ if (!group->nr_pins) {
+ dev_err(ph->dev, "Group %d has 0 elements", selector);
+ return -ENODATA;
+ }
+
+ group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins,
+ sizeof(*group->group_pins),
+ GFP_KERNEL);
+ if (!group->group_pins) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
+ group->nr_pins, group->group_pins);
+ if (ret)
+ goto err_groups;
+
+ group->present = true;
+ return 0;
+
+ err_groups:
+ kfree(group->group_pins);
+ err:
+ kfree(group->name);
+ return ret;
+}
+
+static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph || !name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_groups)
+ return -EINVAL;
+
+ if (!pi->groups[selector].present) {
+ ret = scmi_pinctrl_get_group_info(ph, selector,
+ &pi->groups[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->groups[selector].name;
+
+ return 0;
+}
+
+static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
+ u32 selector, const unsigned int **pins,
+ unsigned int *nr_pins)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph || !pins || !nr_pins)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_groups)
+ return -EINVAL;
+
+ if (!pi->groups[selector].present) {
+ 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 ret;
+}
+
+static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ struct scmi_function_info *func)
+{
+ int ret = 0;
+
+ if (!ph || !func)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
+ &func->name,
+ &func->nr_groups);
+ if (ret)
+ return ret;
+
+ if (!func->nr_groups) {
+ dev_err(ph->dev, "Function %d has 0 elements", selector);
+ return -ENODATA;
+ }
+
+ func->groups = devm_kmalloc_array(ph->dev, func->nr_groups,
+ sizeof(*func->groups),
+ GFP_KERNEL);
+ if (!func->groups) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
+ func->nr_groups, func->groups);
+ if (ret)
+ goto err_funcs;
+
+ func->present = true;
+ return 0;
+
+ err_funcs:
+ kfree(func->groups);
+ err:
+ kfree(func->name);
+ return ret;
+}
+
+static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph || !name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_functions)
+ return -EINVAL;
+
+ if (!pi->functions[selector].present) {
+ ret = scmi_pinctrl_get_function_info(ph, selector,
+ &pi->functions[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->functions[selector].name;
+ return 0;
+}
+
+static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ unsigned int *nr_groups,
+ const unsigned int **groups)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph || !groups || !nr_groups)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_functions)
+ return -EINVAL;
+
+ if (!pi->functions[selector].present) {
+ 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 ret;
+}
+
+static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
+ u32 selector, u32 group)
+{
+ return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
+ selector);
+}
+
+static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
+ u32 selector, struct scmi_pin_info *pin)
+{
+ int ret = 0;
+ struct scmi_pinctrl_info *pi;
+ unsigned int n_elems;
+
+ if (!ph || !pin)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
+ &pin->name,
+ &n_elems);
+ if (ret)
+ return ret;
+
+ if (n_elems != pi->nr_pins) {
+ dev_err(ph->dev, "Wrong pin count expected %d has %d",
+ pi->nr_pins, n_elems);
+ return -ENODATA;
+ }
+
+ if (*pin->name == 0) {
+ dev_err(ph->dev, "Pin name is empty");
+ goto err;
+ }
+
+ pin->present = true;
+ return 0;
+
+ err:
+ kfree(pin->name);
+ return ret;
+}
+
+static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
+ u32 selector, const char **name)
+{
+ int ret;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph || !name)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ if (selector > pi->nr_pins)
+ return -EINVAL;
+
+ if (!pi->pins[selector].present) {
+ ret = scmi_pinctrl_get_pin_info(ph, selector,
+ &pi->pins[selector]);
+ if (ret)
+ return ret;
+ }
+
+ *name = pi->pins[selector].name;
+
+ return 0;
+}
+
+static int scmi_pinctrl_get_name(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name)
+{
+ switch (type) {
+ case PIN_TYPE:
+ return scmi_pinctrl_get_pin_name(ph, selector, name);
+ case GROUP_TYPE:
+ return scmi_pinctrl_get_group_name(ph, selector, name);
+ case FUNCTION_TYPE:
+ return scmi_pinctrl_get_function_name(ph, selector, name);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
+ .get_count = scmi_pinctrl_get_count,
+ .get_name = scmi_pinctrl_get_name,
+ .get_group_pins = scmi_pinctrl_get_group_pins,
+ .get_function_groups = scmi_pinctrl_get_function_groups,
+ .set_mux = scmi_pinctrl_set_mux,
+ .get_config = scmi_pinctrl_get_config,
+ .set_config = scmi_pinctrl_set_config,
+ .request_pin = scmi_pinctrl_request_pin,
+ .free_pin = scmi_pinctrl_free_pin
+};
+
+static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
+{
+ u32 version;
+ struct scmi_pinctrl_info *pinfo;
+ int ret;
+
+ if (!ph)
+ return -EINVAL;
+
+ ret = ph->xops->version_get(ph, &version);
+ if (ret)
+ return ret;
+
+ dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
+ PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+ pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
+ if (!pinfo)
+ return -ENOMEM;
+
+ ret = scmi_pinctrl_attributes_get(ph, pinfo);
+ if (ret)
+ return ret;
+
+ pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
+ sizeof(*pinfo->pins),
+ GFP_KERNEL);
+ if (!pinfo->pins)
+ return -ENOMEM;
+
+ pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
+ sizeof(*pinfo->groups),
+ GFP_KERNEL);
+ if (!pinfo->groups)
+ return -ENOMEM;
+
+ pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
+ sizeof(*pinfo->functions),
+ GFP_KERNEL);
+ if (!pinfo->functions)
+ return -ENOMEM;
+
+ pinfo->version = version;
+
+ return ph->set_priv(ph, pinfo);
+}
+
+static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
+{
+ int i;
+ struct scmi_pinctrl_info *pi;
+
+ if (!ph)
+ return -EINVAL;
+
+ pi = ph->get_priv(ph);
+ if (!pi)
+ return -EINVAL;
+
+ for (i = 0; i < pi->nr_groups; i++)
+ if (pi->groups[i].present) {
+ devm_kfree(ph->dev, pi->groups[i].group_pins);
+ pi->groups[i].present = false;
+ }
+
+ for (i = 0; i < pi->nr_functions; i++)
+ if (pi->functions[i].present) {
+ devm_kfree(ph->dev, pi->functions[i].groups);
+ pi->functions[i].present = false;
+ }
+
+ return 0;
+}
+
+static const struct scmi_protocol scmi_pinctrl = {
+ .id = SCMI_PROTOCOL_PINCTRL,
+ .owner = THIS_MODULE,
+ .instance_init = &scmi_pinctrl_protocol_init,
+ .instance_deinit = &scmi_pinctrl_protocol_deinit,
+ .ops = &pinctrl_proto_ops,
+};
+
+DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..533b94c9a9a7 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -345,5 +345,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
DECLARE_SCMI_REGISTER_UNREGISTER(system);
DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
+DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
#endif /* _SCMI_PROTOCOLS_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0ce5746a4470..8e15d2912045 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -735,6 +735,52 @@ struct scmi_notify_ops {
struct notifier_block *nb);
};
+enum scmi_pinctrl_selector_type {
+ PIN_TYPE = 0,
+ GROUP_TYPE,
+ FUNCTION_TYPE
+};
+
+/**
+ * struct scmi_pinctrl_protocol_ops - represents the various operations provided
+ * by SCMI Pinctrl Protocol
+ *
+ * @get_count: returns count of the registered elements in given type
+ * @get_name: returns name by index of given type
+ * @get_group_pins: returns the set of pins, assigned to the specified group
+ * @get_function_groups: returns the set of groups, assigned to the specified
+ * function
+ * @set_mux: set muxing function for groups of pins
+ * @get_config: returns configuration parameter for pin or group
+ * @set_config: sets the configuration parameter for pin or group
+ * @request_pin: aquire pin before selecting mux setting
+ * @free_pin: frees pin, acquired by request_pin call
+ */
+struct scmi_pinctrl_proto_ops {
+ int (*get_count)(const struct scmi_protocol_handle *ph,
+ enum scmi_pinctrl_selector_type type);
+ int (*get_name)(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ const char **name);
+ int (*get_group_pins)(const struct scmi_protocol_handle *ph,
+ u32 selector,
+ const unsigned int **pins, unsigned int *nr_pins);
+ int (*get_function_groups)(const struct scmi_protocol_handle *ph,
+ u32 selector, unsigned int *nr_groups,
+ const unsigned int **groups);
+ int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
+ u32 group);
+ int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, u32 *config_value);
+ int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
+ enum scmi_pinctrl_selector_type type,
+ u8 config_type, u32 config_value);
+ int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
+ int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
+};
+
/**
* struct scmi_handle - Handle returned to ARM SCMI clients for usage.
*
@@ -783,6 +829,7 @@ enum scmi_std_protocol {
SCMI_PROTOCOL_RESET = 0x16,
SCMI_PROTOCOL_VOLTAGE = 0x17,
SCMI_PROTOCOL_POWERCAP = 0x18,
+ SCMI_PROTOCOL_PINCTRL = 0x19,
};
enum scmi_system_events {
--
2.25.1
scmi-pinctrl driver implements pinctrl driver interface and using
SCMI protocol to redirect messages from pinctrl subsystem SDK to
SCP firmware, which does the changes in HW.
This setup expects SCP firmware (or similar system, such as ATF)
to be installed on the platform, which implements pinctrl driver
for the specific platform.
SCMI-Pinctrl driver should be configured from the device-tree and uses
generic device-tree mappings for the configuration.
Signed-off-by: Oleksii Moisieiev <[email protected]>
---
MAINTAINERS | 1 +
drivers/pinctrl/Kconfig | 9 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++
4 files changed, 589 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-scmi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d251ebac437..ba9e3aea6176 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <[email protected]>
L: [email protected]
S: Maintained
F: drivers/firmware/arm_scmi/pinctrl.c
+F: drivers/pinctrl/pinctrl-scmi.c
SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel <[email protected]>
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index dcb53c4a9584..16bf2c67f095 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig"
source "drivers/pinctrl/visconti/Kconfig"
source "drivers/pinctrl/vt8500/Kconfig"
+config PINCTRL_SCMI
+ bool "Pinctrl driver controlled via SCMI interface"
+ depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
+ help
+ This driver provides support for pinctrl which is controlled
+ by firmware that implements the SCMI interface.
+ It uses SCMI Message Protocol to interact with the
+ firmware providing all the pinctrl controls.
+
endif
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..21366db4f4f4 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
+obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
obj-y += actions/
obj-$(CONFIG_ARCH_ASPEED) += aspeed/
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
new file mode 100644
index 000000000000..8401db1d030b
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Power Interface (SCMI) Protocol based pinctrl driver
+ *
+ * Copyright (C) 2023 EPAM
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/seq_file.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 <linux/scmi_protocol.h>
+#include <linux/slab.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinconf.h"
+
+#define DRV_NAME "scmi-pinctrl"
+
+static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
+
+struct scmi_pinctrl_funcs {
+ unsigned int num_groups;
+ const char **groups;
+};
+
+struct scmi_pinctrl {
+ struct device *dev;
+ struct scmi_protocol_handle *ph;
+ struct pinctrl_dev *pctldev;
+ struct pinctrl_desc pctl_desc;
+ struct scmi_pinctrl_funcs *functions;
+ unsigned int nr_functions;
+ char **groups;
+ unsigned int nr_groups;
+ struct pinctrl_pin_desc *pins;
+ unsigned int nr_pins;
+};
+
+static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE);
+}
+
+static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ int ret;
+ const char *name;
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return NULL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return NULL;
+
+ ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name);
+ if (ret) {
+ dev_err(pmx->dev, "get name failed with err %d", ret);
+ return NULL;
+ }
+
+ return name;
+}
+
+static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_group_pins(pmx->ph, selector,
+ pins, num_pins);
+}
+
+static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
+ struct seq_file *s,
+ unsigned int offset)
+{
+ seq_puts(s, DRV_NAME);
+}
+
+#ifdef CONFIG_OF
+static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np_config,
+ struct pinctrl_map **map,
+ u32 *num_maps)
+{
+ return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
+ num_maps, PIN_MAP_TYPE_INVALID);
+}
+
+static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
+ struct pinctrl_map *map, u32 num_maps)
+{
+ kfree(map);
+}
+
+#endif /* CONFIG_OF */
+
+static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
+ .get_groups_count = pinctrl_scmi_get_groups_count,
+ .get_group_name = pinctrl_scmi_get_group_name,
+ .get_group_pins = pinctrl_scmi_get_group_pins,
+ .pin_dbg_show = pinctrl_scmi_pin_dbg_show,
+#ifdef CONFIG_OF
+ .dt_node_to_map = pinctrl_scmi_dt_node_to_map,
+ .dt_free_map = pinctrl_scmi_dt_free_map,
+#endif
+};
+
+static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE);
+}
+
+static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ int ret;
+ const char *name;
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return NULL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return NULL;
+
+ ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name);
+ if (ret) {
+ dev_err(pmx->dev, "get name failed with err %d", ret);
+ return NULL;
+ }
+
+ return name;
+}
+
+static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const char * const **groups,
+ unsigned int * const num_groups)
+{
+ const unsigned int *group_ids;
+ int ret, i;
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !groups || !num_groups)
+ return -EINVAL;
+
+ if (selector < pmx->nr_functions &&
+ pmx->functions[selector].num_groups) {
+ *groups = (const char * const *)pmx->functions[selector].groups;
+ *num_groups = pmx->functions[selector].num_groups;
+ return 0;
+ }
+
+ ret = pinctrl_ops->get_function_groups(pmx->ph, selector,
+ &pmx->functions[selector].num_groups,
+ &group_ids);
+ if (ret) {
+ dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
+ return ret;
+ }
+
+ *num_groups = pmx->functions[selector].num_groups;
+ if (!*num_groups)
+ return -EINVAL;
+
+ pmx->functions[selector].groups =
+ devm_kcalloc(pmx->dev, *num_groups,
+ sizeof(*pmx->functions[selector].groups),
+ GFP_KERNEL);
+ if (!pmx->functions[selector].groups)
+ return -ENOMEM;
+
+ for (i = 0; i < *num_groups; i++) {
+ pmx->functions[selector].groups[i] =
+ pinctrl_scmi_get_group_name(pmx->pctldev,
+ group_ids[i]);
+ if (!pmx->functions[selector].groups[i]) {
+ ret = -ENOMEM;
+ goto error;
+ }
+ }
+
+ *groups = (const char * const *)pmx->functions[selector].groups;
+
+ return 0;
+
+error:
+ kfree(pmx->functions[selector].groups);
+
+ return ret;
+}
+
+static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
+ unsigned int selector, unsigned int group)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->set_mux(pmx->ph, selector, group);
+}
+
+static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
+ unsigned int offset)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->request_pin(pmx->ph, offset);
+}
+
+static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
+{
+ struct scmi_pinctrl *pmx;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ return pinctrl_ops->free_pin(pmx->ph, offset);
+}
+
+static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
+ .request = pinctrl_scmi_request,
+ .free = pinctrl_scmi_free,
+ .get_functions_count = pinctrl_scmi_get_functions_count,
+ .get_function_name = pinctrl_scmi_get_function_name,
+ .get_function_groups = pinctrl_scmi_get_function_groups,
+ .set_mux = pinctrl_scmi_func_set_mux,
+};
+
+static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
+ (u32 *)&config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !configs || num_configs == 0)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ config_type = pinconf_to_config_param(configs[i]);
+ config_value = pinconf_to_config_argument(configs[i]);
+
+ ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
+ config_value);
+ if (ret) {
+ dev_err(pmx->dev, "Error parsing config %ld\n",
+ configs[i]);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
+ unsigned int group,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !configs || num_configs == 0)
+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ config_type = pinconf_to_config_param(configs[i]);
+ config_value = pinconf_to_config_argument(configs[i]);
+
+ ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
+ config_type, config_value);
+ if (ret) {
+ dev_err(pmx->dev, "Error parsing config = %ld",
+ configs[i]);
+ break;
+ }
+ }
+
+ return ret;
+};
+
+static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
+ unsigned int _pin,
+ unsigned long *config)
+{
+ int ret;
+ struct scmi_pinctrl *pmx;
+ enum pin_config_param config_type;
+ unsigned long config_value;
+
+ if (!pctldev)
+ return -EINVAL;
+
+ pmx = pinctrl_dev_get_drvdata(pctldev);
+
+ if (!pmx || !pmx->ph || !config)
+ return -EINVAL;
+
+ config_type = pinconf_to_config_param(*config);
+
+ ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
+ config_type, (u32 *)&config_value);
+ if (ret)
+ return ret;
+
+ *config = pinconf_to_config_packed(config_type, config_value);
+
+ return 0;
+}
+
+static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
+ .is_generic = true,
+ .pin_config_get = pinctrl_scmi_pinconf_get,
+ .pin_config_set = pinctrl_scmi_pinconf_set,
+ .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
+ .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
+ .pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
+ unsigned int *nr_pins,
+ const struct pinctrl_pin_desc **pins)
+{
+ int ret, i;
+
+ if (!pmx || !pmx->ph)
+ return -EINVAL;
+
+ if (!pins || !nr_pins)
+ return -EINVAL;
+
+ if (pmx->nr_pins) {
+ *pins = pmx->pins;
+ *nr_pins = pmx->nr_pins;
+ return 0;
+ }
+
+ *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE);
+
+ pmx->nr_pins = *nr_pins;
+ pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins),
+ GFP_KERNEL);
+ if (!pmx->pins)
+ return -ENOMEM;
+
+ for (i = 0; i < *nr_pins; i++) {
+ pmx->pins[i].number = i;
+ ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE,
+ &pmx->pins[i].name);
+ if (ret) {
+ dev_err(pmx->dev, "Can't get name for pin %d: rc %d",
+ i, ret);
+ goto err;
+ }
+ }
+
+ *pins = pmx->pins;
+ dev_dbg(pmx->dev, "got pins %d", *nr_pins);
+
+ return 0;
+ err:
+ kfree(pmx->pins);
+ pmx->nr_pins = 0;
+
+ return ret;
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+ { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+ { },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static int scmi_pinctrl_probe(struct scmi_device *sdev)
+{
+ int ret;
+ struct scmi_pinctrl *pmx;
+ const struct scmi_handle *handle;
+ struct scmi_protocol_handle *ph;
+
+ if (!sdev || !sdev->handle)
+ return -EINVAL;
+
+ handle = sdev->handle;
+
+ pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
+ &ph);
+ if (IS_ERR(pinctrl_ops))
+ return PTR_ERR(pinctrl_ops);
+
+ pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
+ if (!pmx)
+ return -ENOMEM;
+
+ pmx->ph = ph;
+
+ pmx->dev = &sdev->dev;
+ pmx->pctl_desc.name = DRV_NAME;
+ pmx->pctl_desc.owner = THIS_MODULE;
+ pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
+ pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
+ pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
+
+ ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
+ &pmx->pctl_desc.pins);
+ if (ret)
+ goto clean;
+
+ ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
+ &pmx->pctldev);
+ if (ret) {
+ dev_err(&sdev->dev, "could not register: %i\n", ret);
+ goto clean;
+ }
+
+ pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
+ pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
+
+ if (pmx->nr_functions) {
+ pmx->functions =
+ devm_kcalloc(&sdev->dev, pmx->nr_functions,
+ sizeof(*pmx->functions),
+ GFP_KERNEL);
+ if (!pmx->functions) {
+ ret = -ENOMEM;
+ goto clean;
+ }
+ }
+
+ if (pmx->nr_groups) {
+ pmx->groups =
+ devm_kcalloc(&sdev->dev, pmx->nr_groups,
+ sizeof(*pmx->groups),
+ GFP_KERNEL);
+ if (!pmx->groups) {
+ ret = -ENOMEM;
+ goto clean;
+ }
+ }
+
+ return pinctrl_enable(pmx->pctldev);
+
+clean:
+ if (pmx) {
+ kfree(pmx->functions);
+ kfree(pmx->groups);
+ }
+
+ kfree(pmx);
+
+ return ret;
+}
+
+static struct scmi_driver scmi_pinctrl_driver = {
+ .name = DRV_NAME,
+ .probe = scmi_pinctrl_probe,
+ .id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_pinctrl_driver);
+
+MODULE_AUTHOR("Oleksii Moisieiev <[email protected]>");
+MODULE_DESCRIPTION("ARM SCMI pin controller driver");
+MODULE_LICENSE("GPL");
--
2.25.1
On Wed, Apr 26, 2023 at 3:26 PM Oleksii Moisieiev
<[email protected]> wrote:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.
>
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
>
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.
>
> Signed-off-by: Oleksii Moisieiev <[email protected]>
This looks as good as ever.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> scmi: Introduce pinctrl SCMI protocol driver
>
> Add basic implementation of the SCMI v3.2 pincontrol protocol
> excluding GPIO support. All pinctrl related callbacks and operations
> are exposed in the include/linux/scmi_protocol.h
>
Hi Oleksii,
Thanks for this.
I tried out this in an emulated setup and found just a minor issue from
the spec/functional point of view...then I reworked the extended names
support using a modified hops->extended_name_get helper (as said the core
SCMI support needed a small modification to support PINCTRL): I'll reply
to this mail thread with such core SCMI modification patch, so you can
include this patch of mine in your next V3 and use it in your series.
Moreover, given that I wanted to test such rework of mine and a bunch
of other cleanups I did (as detailed down below), and it seemed silly
to throw all away just to then having to detail all to you, I'll also
include in another distinct reply the raw diff of what I changed in
your series to use the new extended_name support and a few other cleanups,
so that, if you want, you can just quickly merge that into your V3 patch
(of course if you like it and tests fine also for you...)...these are
small changes, if you take it, no need to bother with authorship and
attribution from my point of view.
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/firmware/arm_scmi/Makefile | 3 +-
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/pinctrl.c | 932 ++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/protocols.h | 1 +
> include/linux/scmi_protocol.h | 47 ++
> 6 files changed, 990 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0e64787aace8..0d251ebac437 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20317,6 +20317,12 @@ F: include/linux/sc[mp]i_protocol.h
> F: include/trace/events/scmi.h
> F: include/uapi/linux/virtio_scmi.h
>
> +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
> +M: Oleksii Moisieiev <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/firmware/arm_scmi/pinctrl.c
> +
> SYSTEM RESET/SHUTDOWN DRIVERS
> M: Sebastian Reichel <[email protected]>
> L: [email protected]
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index b31d78fa66cc..071ac65f22b9 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> scmi-core-objs := $(scmi-bus-y)
>
> scmi-driver-y = driver.o notify.o
> +
Do not add spurios lines.
> scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index dbc474ff62b7..fc7ba587ae6e 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3021,6 +3021,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);
> }
> @@ -3038,6 +3039,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..1c643d21390f
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -0,0 +1,932 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Pinctrl Protocol
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "protocols.h"
> +
> +#define SET_TYPE(x) FIELD_PREP(GENMASK(1, 0), (x))
> +
This macro is really not needed given that you set just bit 0,1 on a
32bit flag field that is all 0-reserved otherwise...more on this down below.
> +#define REG_TYPE_BITS GENMASK(9, 8)
> +#define REG_CONFIG GENMASK(7, 0)
> +
> +#define GET_GROUPS_NR(x) FIELD_GET(GENMASK(31, 16), (x))
> +#define GET_PINS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
> +#define GET_FUNCTIONS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
> +
> +#define EXT_NAME_FLAG(x) FIELD_GET(BIT(31), (x))
> +#define NUM_ELEMS(x) FIELD_GET(GENMASK(15, 0), (x))
> +
> +#define REMAINING(x) FIELD_GET(GENMASK(31, 16), (x))
> +#define RETURNED(x) FIELD_GET(GENMASK(11, 0), (x))
> +
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_CONFIG_GET = 0x5,
> + PINCTRL_CONFIG_SET = 0x6,
> + PINCTRL_FUNCTION_SELECT = 0x7,
> + PINCTRL_REQUEST = 0x8,
> + PINCTRL_RELEASE = 0x9,
> + PINCTRL_NAME_GET = 0xa,
> + PINCTRL_SET_PERMISSIONS = 0xb
> +};
> +
> +struct scmi_msg_conf_set {
> + __le32 identifier;
> + __le32 attributes;
> + __le32 config_value;
> +};
> +
> +struct scmi_msg_conf_get {
> + __le32 identifier;
> + __le32 attributes;
> +};
> +
> +struct scmi_msg_pinctrl_protocol_attributes {
> + __le32 attributes_low;
> + __le32 attributes_high;
> +};
> +
> +struct scmi_msg_ext_name {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_ext_name {
> + __le32 flags;
> + u8 name[SCMI_MAX_STR_SIZE];
> +};
> +
You could drop these structs once using the common extended_name_get
helper.
> +struct scmi_msg_pinctrl_attributes {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_resp_pinctrl_attributes {
> + __le32 attributes;
> + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> +};
> +
> +struct scmi_msg_pinctrl_list_assoc {
> + __le32 identifier;
> + __le32 flags;
> + __le32 index;
> +};
> +
> +struct scmi_resp_pinctrl_list_assoc {
> + __le32 flags;
> + __le16 array[];
> +};
> +
> +struct scmi_msg_func_set {
> + __le32 identifier;
> + __le32 function_id;
> + __le32 flags;
> +};
> +
> +struct scmi_msg_request {
> + __le32 identifier;
> + __le32 flags;
> +};
> +
> +struct scmi_group_info {
> + bool present;
> + char *name;
> + unsigned int *group_pins;
> + unsigned int nr_pins;
> +};
> +
> +struct scmi_function_info {
> + bool present;
> + char *name;
> + unsigned int *groups;
> + unsigned int nr_groups;
> +};
> +
> +struct scmi_pin_info {
> + bool present;
> + char *name;
> +};
> +
I would re-define all of these 3 above *name pointers as
name[SCMI_MAX_STR_SIZE] instead and dont bother with dynalloc
and free...to avoid tricky resource tracking for a few bytes...and a
avoid a memleak that is now present :D ... more details later.
> +struct scmi_pinctrl_info {
> + u32 version;
> + int nr_groups;
> + int nr_functions;
> + int nr_pins;
> + struct scmi_group_info *groups;
> + struct scmi_function_info *functions;
> + struct scmi_pin_info *pins;
> +};
> +
> +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_pinctrl_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_protocol_attributes *attr;
> +
> + if (!pi)
> + return -EINVAL;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
> + 0, sizeof(*attr), &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_functions =
> + le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
> + pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
> + pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));
I see a couple of issues here present in general all across this patch when
you use these macros;
You should take care of the endianity in the RX msg payload BEFORE and THEN
DISSECT the bitfields AND as a consequence use also an _le helper that fits
the size of the type that you are processing as in (being attributes 32 bit
little endian in the msg payload):
pi->nr_pins = GET_PINS_NR(le32_to_cpu(attr->attributes_low));
Now all works just because everything is little endian really so nothing
is done by these macros....
> + }
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph)
> + return -ENODEV;
> +
You can just drop this !ph check all-over this file...you just cannot be
here if ph was not initialized by the core...no need to check.
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -ENODEV;
> +
> + switch (type) {
> + case PIN_TYPE:
> + return pi->nr_pins;
> + case GROUP_TYPE:
> + return pi->nr_groups;
> + case FUNCTION_TYPE:
> + return pi->nr_functions;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int value;
> +
> + if (!ph)
> + return -ENODEV;
Ditto...I wont reapet all over.
> +
> + value = scmi_pinctrl_get_count(ph, type);
> + if (value < 0)
> + return value;
> +
> + if (identifier >= value)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_ext_name(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type,
> + char **name)
> +{
> + struct scmi_xfer *t;
> + int ret = 0;
> + struct scmi_msg_ext_name *tx;
> + struct scmi_resp_ext_name *rx;
> +
> + if (!ph || !name)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_NAME_GET, sizeof(*tx),
> + sizeof(*rx), &t);
> +
> + tx = t->tx.buf;
> + rx = t->rx.buf;
> + tx->identifier = identifier;
> + tx->flags = SET_TYPE(cpu_to_le32(type));
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (ret)
> + goto out;
> +
> + if (rx->flags) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *name = kasprintf(GFP_KERNEL, "%s", rx->name);
> + if (!*name)
> + ret = -ENOMEM;
> + out:
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
So I droppped completely the above function and used the helper with
some modifications as detailed below. (full details anyway in my
reference example patch as said above)
> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type,
> + u32 selector, char **name,
> + unsigned int *n_elems)
> +{
> + int ret = 0;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_attributes *tx;
> + struct scmi_resp_pinctrl_attributes *rx;
> +
> + if (!ph || !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 = selector;
Missing cpu_to_le32
> + tx->flags = SET_TYPE(cpu_to_le32(type));
> +
Here we are sending so we have to build an _le32 payload AT THE END
after having built our field like in:
tx->flags = cpu_to_le32(SET_TYPE(type));
...BUT, as mentioned above, SET_TYPE really sets only bits 0,1 in a
32bit field so you can just:
tx->flags = cpu_to_le32(type);
> + ret = ph->xops->do_xfer(ph, t);
> + if (ret)
> + goto out;
I would revert the logic here [if (!ret)] so as to avoid the goto
since, really, it is used only once in this function so we can easily
stick to non-goto flow.
> +
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
As explained below when this function is called for a PIN n_elems is
reported as zero and NOT needed really, so I made this param optional
adding here a:
if (n_elems)
*n_elems = ...
> + if (!EXT_NAME_FLAG(rx->attributes)) {
Missing le32_to_cpu on attributes.
> + *name = kasprintf(GFP_KERNEL, "%s", rx->name);
> + if (!*name)
> + ret = -ENOMEM;
> + } else {
> + ret = scmi_pinctrl_get_ext_name(ph, selector, type, name);
> + }
Ok, here there are a couple of things worth noting (beside the usage of
the common helper).
You dyn-allocate *name in both cases short ot extended name and, rightly
so, in the callers of this, you take care to free it on the error-path,
BUT you never take care to release it on Success and this is not even a
devres...so if protocol is unloaded you'll have a leak.
Moreover by the spec even if you have extended names supported you are
supposed to return anyway the first 16 bytes of the resource name in
rx->name anyway, and you have to consider that the following extended_name
GET (if supported) can fail for a number of reasons (including a faulty fw).
So usually, in the otehr proto we collect the short name anyway and then
overwrite it with the extended one if possible, issuing just a warning
on failure to get the extended one, since you have anyway the short.
But beside this I would not bother to track all this tiny dynalloc and
just declare a static name[SCMI_MAX_STR_SIZE] which can hold both.
IOW this could roughly be (with the helper):
if (!ret) {
attrs = le32_to_cpu(rx->attributes);
if (n_elems)
*n_elems = NUM_ELEMS(attrs);
strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
}
ph->xops->xfer_put(ph, t);
/*
* If supported overwrite short name with the extended one;
* on error just carry on and use already provided short name.
*/
if (!ret && EXT_NAME_FLAG(attrs))
ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
(u32 *)&type, name,
SCMI_MAX_STR_SIZE);
return ret;
....probably more easy to grasp all of this looking at the proposed
fixes :D
> + out:
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +struct scmi_pinctrl_ipriv {
> + u32 selector;
> + enum scmi_pinctrl_selector_type type;
> + unsigned int *array;
> +};
> +
> +static void iter_pinctrl_assoc_prepare_message(void *message,
> + unsigned int desc_index,
> + const void *priv)
> +{
> + struct scmi_msg_pinctrl_list_assoc *msg = message;
> + const struct scmi_pinctrl_ipriv *p = priv;
> +
> + msg->identifier = cpu_to_le32(p->selector);
> + msg->flags = SET_TYPE(cpu_to_le32(p->type));
Drop SET_TYPE as said above.
> + /* 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 = le32_to_cpu(RETURNED(r->flags));
> + st->num_remaining = le32_to_cpu(REMAINING(r->flags));
> +
Issue le32_to_cpu before applying the macro as said.
> + return 0;
> +}
> +
> +static int
> +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph,
> + const void *response,
> + struct scmi_iterator_state *st, void *priv)
> +{
> + const struct scmi_resp_pinctrl_list_assoc *r = response;
> + struct scmi_pinctrl_ipriv *p = priv;
> +
> + p->array[st->desc_index + st->loop_idx] =
> + le16_to_cpu(r->array[st->loop_idx]);
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u16 size, unsigned int *array)
> +{
> + int ret;
> + void *iter;
> + struct scmi_iterator_ops ops = {
> + .prepare_message = iter_pinctrl_assoc_prepare_message,
> + .update_state = iter_pinctrl_assoc_update_state,
> + .process_response = iter_pinctrl_assoc_process_response,
> + };
> + struct scmi_pinctrl_ipriv ipriv = {
> + .selector = selector,
> + .type = type,
> + .array = array,
> + };
> +
> + if (!ph || !array || !size)
> + return -EINVAL;
> +
> + if (type == PIN_TYPE)
> + return -EINVAL;
> +
You can sqaush this 2 ifs into one and drop the !ph check.
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + iter = ph->hops->iter_response_init(ph, &ops, size,
> + PINCTRL_LIST_ASSOCIATIONS,
> + sizeof(struct scmi_msg_pinctrl_list_assoc),
> + &ipriv);
> +
> + if (IS_ERR(iter))
> + return PTR_ERR(iter);
> +
> + return ph->hops->iter_response_run(iter);
> +}
> +
> +static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u8 config_type, u32 *config_value)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_conf_get *tx;
> + __le32 *le_config;
Safer if you use an existing unaligned helper for this...see below.
> + u32 attributes = 0;
> + int ret;
> +
> + if (!ph || !config_value || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
Drop !ph
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx),
> + sizeof(*le_config), &t);
sizeof(__le32), &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + le_config = t->rx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + attributes = FIELD_PREP(REG_TYPE_BITS, type) |
> + FIELD_PREP(REG_CONFIG, config_type);
> +
> + tx->attributes = cpu_to_le32(attributes);
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + if (!ret)
> + *config_value = le32_to_cpu(*le_config);
Use an helper:
*config_value = get_unaligned_le32(t->rx.buf);
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_set_config(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u8 config_type, u32 config_value)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_conf_set *tx;
> + u32 attributes = 0;
> + int ret;
> +
> + if (!ph || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_SET,
> + sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + attributes = FIELD_PREP(REG_TYPE_BITS, type) |
> + FIELD_PREP(REG_CONFIG, config_type);
> + tx->attributes = cpu_to_le32(attributes);
> + tx->config_value = cpu_to_le32(config_value);
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type,
> + u32 function_id)
> +{
> + struct scmi_xfer *t;
> + struct scmi_msg_func_set *tx;
> + int ret;
Usually we place the shorter defs at start of the block...
> +
> + if (!ph || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_FUNCTION_SELECT,
> + sizeof(*tx), 0, &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->function_id = cpu_to_le32(function_id);
> + tx->flags = SET_TYPE(cpu_to_le32(type));
> +
Drop SET_TYPE
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_xfer *t;
> + int ret;
This defs at start of the block..
> + struct scmi_msg_request *tx;
> +
> + if (!ph || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx),
> + 0, &t);
> +
> + tx = t->tx.buf;
> + tx->identifier = identifier;
Missing cpu_to_le32
> + tx->flags = SET_TYPE(cpu_to_le32(type));
Drop SET_TYPE
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_request_pin(const struct scmi_protocol_handle *ph,
> + u32 pin)
> +{
> + return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + struct scmi_xfer *t;
> + int ret;
> + struct scmi_msg_request *tx;
> +
> + if (!ph || type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> + sizeof(*tx), 0, &t);
> +
> + tx = t->tx.buf;
> + tx->identifier = identifier;
Missing cpu_to_le32
> + tx->flags = SET_TYPE(cpu_to_le32(type));
Drop SET_TYPE
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_free_pin(const struct scmi_protocol_handle *ph, u32 pin)
> +{
> + return scmi_pinctrl_free(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_group_info *group)
> +{
> + int ret = 0;
> +
> + if (!ph || !group)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> + &group->name,
Just a simple pointer in the above hyp of static allocation
group->name,
...same for all invoctions of course.
> + &group->nr_pins);
> + if (ret)
> + return ret;
> +
> + if (!group->nr_pins) {
> + dev_err(ph->dev, "Group %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins,
> + sizeof(*group->group_pins),
> + GFP_KERNEL);
> + if (!group->group_pins) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> + group->nr_pins, group->group_pins);
> + if (ret)
> + goto err_groups;
> +
> + group->present = true;
> + return 0;
> +
> + err_groups:
> + kfree(group->group_pins);
> + err:
> + kfree(group->name);
> + return ret;
You can rework all of the above to just drop both these goto exit
labels....take a look at my proposal fixes if you like it.
> +}
> +
> +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph || !name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + if (selector > pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + ret = scmi_pinctrl_get_group_info(ph, selector,
> + &pi->groups[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->groups[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
> + u32 selector, const unsigned int **pins,
> + unsigned int *nr_pins)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph || !pins || !nr_pins)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + if (selector > pi->nr_groups)
> + return -EINVAL;
> +
> + if (!pi->groups[selector].present) {
> + 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 ret;
> +}
> +
> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + struct scmi_function_info *func)
> +{
> + int ret = 0;
> +
> + if (!ph || !func)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> + &func->name,
> + &func->nr_groups);
> + if (ret)
> + return ret;
> +
> + if (!func->nr_groups) {
> + dev_err(ph->dev, "Function %d has 0 elements", selector);
> + return -ENODATA;
> + }
> +
> + func->groups = devm_kmalloc_array(ph->dev, func->nr_groups,
> + sizeof(*func->groups),
> + GFP_KERNEL);
> + if (!func->groups) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> + func->nr_groups, func->groups);
> + if (ret)
> + goto err_funcs;
> +
> + func->present = true;
> + return 0;
> +
> + err_funcs:
> + kfree(func->groups);
> + err:
> + kfree(func->name);
> + return ret;
Can be reworked and dropped as above.
> +}
> +
> +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph || !name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + if (selector > pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + ret = scmi_pinctrl_get_function_info(ph, selector,
> + &pi->functions[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->functions[selector].name;
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + unsigned int *nr_groups,
> + const unsigned int **groups)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph || !groups || !nr_groups)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + if (selector > pi->nr_functions)
> + return -EINVAL;
> +
> + if (!pi->functions[selector].present) {
> + 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 ret;
> +}
> +
> +static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
> + u32 selector, u32 group)
> +{
> + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> + selector);
> +}
> +
> +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> + u32 selector, struct scmi_pin_info *pin)
> +{
> + int ret = 0;
> + struct scmi_pinctrl_info *pi;
> + unsigned int n_elems;
> +
> + if (!ph || !pin)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> + &pin->name,
> + &n_elems);
> + if (ret)
> + return ret;
> +
> + if (n_elems != pi->nr_pins) {
> + dev_err(ph->dev, "Wrong pin count expected %d has %d",
> + pi->nr_pins, n_elems);
> + return -ENODATA;
> + }
This is wrong because PINCTRL_ATTRIBUTES by the spec reports this
field of attributes as ZERO when the type queried is PIN_TYPE...so
you'll fail all the time here. (or you have to fix your server too :D)
So I would just drop this and this is the reason above I made n_elems
param to scmi_pinctrl_attributes() optional: here you dont need it.
[or you could ignore it...)
> +
> + if (*pin->name == 0) {
> + dev_err(ph->dev, "Pin name is empty");
> + goto err;
> + }
And I would just drop this check too...you'll have an empty string in
case you define name as name[SCMI_MAX_STR_SIZE]...maybe a warning
not an error...picntrl seems to survive with an empty string
> +
> + pin->present = true;
> + return 0;
> +
> + err:
> + kfree(pin->name);
> + return ret;
Can be dropped if static.
> +}
> +
> +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> + u32 selector, const char **name)
> +{
> + int ret;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph || !name)
> + return -EINVAL;
> +
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + if (selector > pi->nr_pins)
> + return -EINVAL;
> +
> + if (!pi->pins[selector].present) {
> + ret = scmi_pinctrl_get_pin_info(ph, selector,
> + &pi->pins[selector]);
> + if (ret)
> + return ret;
> + }
> +
> + *name = pi->pins[selector].name;
> +
> + return 0;
> +}
> +
> +static int scmi_pinctrl_get_name(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name)
> +{
> + switch (type) {
> + case PIN_TYPE:
> + return scmi_pinctrl_get_pin_name(ph, selector, name);
> + case GROUP_TYPE:
> + return scmi_pinctrl_get_group_name(ph, selector, name);
> + case FUNCTION_TYPE:
> + return scmi_pinctrl_get_function_name(ph, selector, name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> + .get_count = scmi_pinctrl_get_count,
> + .get_name = scmi_pinctrl_get_name,
> + .get_group_pins = scmi_pinctrl_get_group_pins,
> + .get_function_groups = scmi_pinctrl_get_function_groups,
> + .set_mux = scmi_pinctrl_set_mux,
> + .get_config = scmi_pinctrl_get_config,
> + .set_config = scmi_pinctrl_set_config,
> + .request_pin = scmi_pinctrl_request_pin,
> + .free_pin = scmi_pinctrl_free_pin
> +};
> +
> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> + struct scmi_pinctrl_info *pinfo;
> + int ret;
> +
> + if (!ph)
> + return -EINVAL;
Drop
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> + if (ret)
> + return ret;
> +
> + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> + sizeof(*pinfo->pins),
> + GFP_KERNEL);
> + if (!pinfo->pins)
> + return -ENOMEM;
> +
> + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> + sizeof(*pinfo->groups),
> + GFP_KERNEL);
> + if (!pinfo->groups)
> + return -ENOMEM;
> +
> + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> + sizeof(*pinfo->functions),
> + GFP_KERNEL);
> + if (!pinfo->functions)
> + return -ENOMEM;
> +
> + pinfo->version = version;
> +
> + return ph->set_priv(ph, pinfo);
> +}
> +
> +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> +{
> + int i;
> + struct scmi_pinctrl_info *pi;
> +
> + if (!ph)
> + return -EINVAL;
> +
Drop
> + pi = ph->get_priv(ph);
> + if (!pi)
> + return -EINVAL;
> +
> + for (i = 0; i < pi->nr_groups; i++)
> + if (pi->groups[i].present) {
> + devm_kfree(ph->dev, pi->groups[i].group_pins);
> + pi->groups[i].present = false;
> + }
> +
> + for (i = 0; i < pi->nr_functions; i++)
> + if (pi->functions[i].present) {
> + devm_kfree(ph->dev, pi->functions[i].groups);
> + pi->functions[i].present = false;
> + }
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol scmi_pinctrl = {
> + .id = SCMI_PROTOCOL_PINCTRL,
> + .owner = THIS_MODULE,
> + .instance_init = &scmi_pinctrl_protocol_init,
> + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> + .ops = &pinctrl_proto_ops,
> +};
> +
> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> index 78e1a01eb656..533b94c9a9a7 100644
> --- a/drivers/firmware/arm_scmi/protocols.h
> +++ b/drivers/firmware/arm_scmi/protocols.h
> @@ -345,5 +345,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
> DECLARE_SCMI_REGISTER_UNREGISTER(system);
> DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
> +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
>
> #endif /* _SCMI_PROTOCOLS_H */
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 0ce5746a4470..8e15d2912045 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -735,6 +735,52 @@ struct scmi_notify_ops {
> struct notifier_block *nb);
> };
>
> +enum scmi_pinctrl_selector_type {
> + PIN_TYPE = 0,
> + GROUP_TYPE,
> + FUNCTION_TYPE
> +};
> +
> +/**
> + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
> + * by SCMI Pinctrl Protocol
> + *
> + * @get_count: returns count of the registered elements in given type
> + * @get_name: returns name by index of given type
> + * @get_group_pins: returns the set of pins, assigned to the specified group
> + * @get_function_groups: returns the set of groups, assigned to the specified
> + * function
> + * @set_mux: set muxing function for groups of pins
> + * @get_config: returns configuration parameter for pin or group
> + * @set_config: sets the configuration parameter for pin or group
> + * @request_pin: aquire pin before selecting mux setting
> + * @free_pin: frees pin, acquired by request_pin call
> + */
> +struct scmi_pinctrl_proto_ops {
> + int (*get_count)(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type);
> + int (*get_name)(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + const char **name);
> + int (*get_group_pins)(const struct scmi_protocol_handle *ph,
> + u32 selector,
> + const unsigned int **pins, unsigned int *nr_pins);
> + int (*get_function_groups)(const struct scmi_protocol_handle *ph,
> + u32 selector, unsigned int *nr_groups,
> + const unsigned int **groups);
> + int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
> + u32 group);
> + int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u8 config_type, u32 *config_value);
> + int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
> + enum scmi_pinctrl_selector_type type,
> + u8 config_type, u32 config_value);
> + int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> + int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> +};
> +
I know this could seem pedantic but all over the SCMI stack the protocol
operations, being external facing methods, are named as <something_ACTION>
as in:
count_get
name_get
config_get
config_set
etc etc...please rename accordingly the pinctrl_ops.
> /**
> * struct scmi_handle - Handle returned to ARM SCMI clients for usage.
> *
> @@ -783,6 +829,7 @@ enum scmi_std_protocol {
> SCMI_PROTOCOL_RESET = 0x16,
> SCMI_PROTOCOL_VOLTAGE = 0x17,
> SCMI_PROTOCOL_POWERCAP = 0x18,
> + SCMI_PROTOCOL_PINCTRL = 0x19,
> };
>
> enum scmi_system_events {
> --
> 2.25.1
Thanks,
Cristian
On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.
>
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
>
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.
>
Hi Oleksii,
just a few remarks down below.
> Signed-off-by: Oleksii Moisieiev <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 9 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++
> 4 files changed, 589 insertions(+)
> create mode 100644 drivers/pinctrl/pinctrl-scmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d251ebac437..ba9e3aea6176 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/firmware/arm_scmi/pinctrl.c
> +F: drivers/pinctrl/pinctrl-scmi.c
>
> SYSTEM RESET/SHUTDOWN DRIVERS
> M: Sebastian Reichel <[email protected]>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index dcb53c4a9584..16bf2c67f095 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig"
> source "drivers/pinctrl/visconti/Kconfig"
> source "drivers/pinctrl/vt8500/Kconfig"
>
> +config PINCTRL_SCMI
> + bool "Pinctrl driver controlled via SCMI interface"
Why not a tristate ? The core SCMI stack can be compiled as module with
all the protocols...at least for testing is useful to have this driver
as M.
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +
> endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index d5939840bb2a..21366db4f4f4 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
> obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
>
> obj-y += actions/
> obj-$(CONFIG_ARCH_ASPEED) += aspeed/
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> new file mode 100644
> index 000000000000..8401db1d030b
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl_funcs {
> + unsigned int num_groups;
> + const char **groups;
> +};
> +
> +struct scmi_pinctrl {
> + struct device *dev;
> + struct scmi_protocol_handle *ph;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_desc pctl_desc;
> + struct scmi_pinctrl_funcs *functions;
> + unsigned int nr_functions;
> + char **groups;
> + unsigned int nr_groups;
> + struct pinctrl_pin_desc *pins;
> + unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return NULL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return NULL;
> +
> + ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->get_group_pins(pmx->ph, selector,
> + pins, num_pins);
> +}
> +
> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
> + struct seq_file *s,
> + unsigned int offset)
> +{
> + seq_puts(s, DRV_NAME);
> +}
> +
> +#ifdef CONFIG_OF
> +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
> + struct device_node *np_config,
> + struct pinctrl_map **map,
> + u32 *num_maps)
> +{
> + return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
> + num_maps, PIN_MAP_TYPE_INVALID);
> +}
> +
> +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
> + struct pinctrl_map *map, u32 num_maps)
> +{
> + kfree(map);
> +}
> +
> +#endif /* CONFIG_OF */
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> + .get_groups_count = pinctrl_scmi_get_groups_count,
> + .get_group_name = pinctrl_scmi_get_group_name,
> + .get_group_pins = pinctrl_scmi_get_group_pins,
> + .pin_dbg_show = pinctrl_scmi_pin_dbg_show,
> +#ifdef CONFIG_OF
> + .dt_node_to_map = pinctrl_scmi_dt_node_to_map,
> + .dt_free_map = pinctrl_scmi_dt_free_map,
> +#endif
> +};
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + int ret;
> + const char *name;
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return NULL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return NULL;
> +
> + ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name);
> + if (ret) {
> + dev_err(pmx->dev, "get name failed with err %d", ret);
> + return NULL;
> + }
> +
> + return name;
> +}
> +
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const char * const **groups,
> + unsigned int * const num_groups)
> +{
> + const unsigned int *group_ids;
> + int ret, i;
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !groups || !num_groups)
> + return -EINVAL;
> +
> + if (selector < pmx->nr_functions &&
> + pmx->functions[selector].num_groups) {
> + *groups = (const char * const *)pmx->functions[selector].groups;
> + *num_groups = pmx->functions[selector].num_groups;
> + return 0;
> + }
> +
> + ret = pinctrl_ops->get_function_groups(pmx->ph, selector,
> + &pmx->functions[selector].num_groups,
> + &group_ids);
> + if (ret) {
> + dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
> + return ret;
> + }
> +
> + *num_groups = pmx->functions[selector].num_groups;
> + if (!*num_groups)
> + return -EINVAL;
> +
> + pmx->functions[selector].groups =
> + devm_kcalloc(pmx->dev, *num_groups,
> + sizeof(*pmx->functions[selector].groups),
> + GFP_KERNEL);
> + if (!pmx->functions[selector].groups)
> + return -ENOMEM;
> +
> + for (i = 0; i < *num_groups; i++) {
> + pmx->functions[selector].groups[i] =
> + pinctrl_scmi_get_group_name(pmx->pctldev,
> + group_ids[i]);
> + if (!pmx->functions[selector].groups[i]) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + }
> +
> + *groups = (const char * const *)pmx->functions[selector].groups;
> +
> + return 0;
> +
> +error:
> + kfree(pmx->functions[selector].groups);
devm_kfree ?
> +
> + return ret;
> +}
> +
> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int selector, unsigned int group)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->set_mux(pmx->ph, selector, group);
> +}
> +
> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
> + unsigned int offset)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->request_pin(pmx->ph, offset);
> +}
> +
> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
> +{
> + struct scmi_pinctrl *pmx;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + return pinctrl_ops->free_pin(pmx->ph, offset);
> +}
> +
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> + .request = pinctrl_scmi_request,
> + .free = pinctrl_scmi_free,
> + .get_functions_count = pinctrl_scmi_get_functions_count,
> + .get_function_name = pinctrl_scmi_get_function_name,
> + .get_function_groups = pinctrl_scmi_get_function_groups,
> + .set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + (u32 *)&config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> + config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config %ld\n",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + config_type = pinconf_to_config_param(configs[i]);
> + config_value = pinconf_to_config_argument(configs[i]);
> +
> + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
> + config_type, config_value);
> + if (ret) {
> + dev_err(pmx->dev, "Error parsing config = %ld",
> + configs[i]);
> + break;
> + }
> + }
> +
> + return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
> + config_type, (u32 *)&config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> + .is_generic = true,
> + .pin_config_get = pinctrl_scmi_pinconf_get,
> + .pin_config_set = pinctrl_scmi_pinconf_set,
> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> + .pin_config_config_dbg_show = pinconf_generic_dump_config,
> +};
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> + unsigned int *nr_pins,
> + const struct pinctrl_pin_desc **pins)
> +{
> + int ret, i;
> +
> + if (!pmx || !pmx->ph)
> + return -EINVAL;
> +
> + if (!pins || !nr_pins)
> + return -EINVAL;
> +
> + if (pmx->nr_pins) {
> + *pins = pmx->pins;
> + *nr_pins = pmx->nr_pins;
> + return 0;
> + }
> +
> + *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE);
> +
> + pmx->nr_pins = *nr_pins;
> + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins),
> + GFP_KERNEL);
> + if (!pmx->pins)
> + return -ENOMEM;
> +
> + for (i = 0; i < *nr_pins; i++) {
> + pmx->pins[i].number = i;
> + ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE,
> + &pmx->pins[i].name);
> + if (ret) {
> + dev_err(pmx->dev, "Can't get name for pin %d: rc %d",
> + i, ret);
> + goto err;
> + }
> + }
> +
> + *pins = pmx->pins;
> + dev_dbg(pmx->dev, "got pins %d", *nr_pins);
> +
> + return 0;
> + err:
> + kfree(pmx->pins);
devm_kfree ? but anyway when this fails it will trigger the _probe to
fail so all devres will be released..so not needed probably at the end.
> + pmx->nr_pins = 0;
> +
> + return ret;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev || !sdev->handle)
> + return -EINVAL;
> +
> + handle = sdev->handle;
> +
> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
> +
> + pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = &sdev->dev;
> + pmx->pctl_desc.name = DRV_NAME;
> + pmx->pctl_desc.owner = THIS_MODULE;
> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
> + &pmx->pctl_desc.pins);
> + if (ret)
> + goto clean;
> +
> + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret) {
> + dev_err(&sdev->dev, "could not register: %i\n", ret);
> + goto clean;
> + }
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
> +
> + if (pmx->nr_functions) {
> + pmx->functions =
> + devm_kcalloc(&sdev->dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions) {
> + ret = -ENOMEM;
> + goto clean;
> + }
> + }
> +
> + if (pmx->nr_groups) {
> + pmx->groups =
> + devm_kcalloc(&sdev->dev, pmx->nr_groups,
> + sizeof(*pmx->groups),
> + GFP_KERNEL);
> + if (!pmx->groups) {
> + ret = -ENOMEM;
> + goto clean;
> + }
> + }
> +
> + return pinctrl_enable(pmx->pctldev);
> +
> +clean:
> + if (pmx) {
> + kfree(pmx->functions);
> + kfree(pmx->groups);
> + }
> +
> + kfree(pmx);
All of these are devres allocated...it does not seem to me that they
require explicit freeing on the _probe() failure path.
(indeed you dont need even a .remove function)
Thanks,
Cristian
A few TENTATIVE misc fixes and refactor as a reference
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/pinctrl.c | 271 +++++++++-------------------
1 file changed, 90 insertions(+), 181 deletions(-)
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 1c643d21390f..10c4cd1ae844 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -11,8 +11,6 @@
#include "protocols.h"
-#define SET_TYPE(x) FIELD_PREP(GENMASK(1, 0), (x))
-
#define REG_TYPE_BITS GENMASK(9, 8)
#define REG_CONFIG GENMASK(7, 0)
@@ -54,16 +52,6 @@ struct scmi_msg_pinctrl_protocol_attributes {
__le32 attributes_high;
};
-struct scmi_msg_ext_name {
- __le32 identifier;
- __le32 flags;
-};
-
-struct scmi_resp_ext_name {
- __le32 flags;
- u8 name[SCMI_MAX_STR_SIZE];
-};
-
struct scmi_msg_pinctrl_attributes {
__le32 identifier;
__le32 flags;
@@ -98,21 +86,21 @@ struct scmi_msg_request {
struct scmi_group_info {
bool present;
- char *name;
+ char name[SCMI_MAX_STR_SIZE];
unsigned int *group_pins;
unsigned int nr_pins;
};
struct scmi_function_info {
bool present;
- char *name;
+ char name[SCMI_MAX_STR_SIZE];
unsigned int *groups;
unsigned int nr_groups;
};
struct scmi_pin_info {
bool present;
- char *name;
+ char name[SCMI_MAX_STR_SIZE];
};
struct scmi_pinctrl_info {
@@ -145,9 +133,10 @@ static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
pi->nr_functions =
- le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
- pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
- pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));
+ GET_FUNCTIONS_NR(le32_to_cpu(attr->attributes_high));
+ pi->nr_groups =
+ GET_GROUPS_NR(le32_to_cpu(attr->attributes_low));
+ pi->nr_pins = GET_PINS_NR(le32_to_cpu(attr->attributes_low));
}
ph->xops->xfer_put(ph, t);
@@ -159,9 +148,6 @@ static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph,
{
struct scmi_pinctrl_info *pi;
- if (!ph)
- return -ENODEV;
-
pi = ph->get_priv(ph);
if (!pi)
return -ENODEV;
@@ -184,9 +170,6 @@ static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
{
int value;
- if (!ph)
- return -ENODEV;
-
value = scmi_pinctrl_get_count(ph, type);
if (value < 0)
return value;
@@ -197,60 +180,18 @@ static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
return 0;
}
-static int scmi_pinctrl_get_ext_name(const struct scmi_protocol_handle *ph,
- u32 identifier,
- enum scmi_pinctrl_selector_type type,
- char **name)
-{
- struct scmi_xfer *t;
- int ret = 0;
- struct scmi_msg_ext_name *tx;
- struct scmi_resp_ext_name *rx;
-
- if (!ph || !name)
- return -EINVAL;
-
- ret = scmi_pinctrl_validate_id(ph, identifier, type);
- if (ret)
- return ret;
-
- ret = ph->xops->xfer_get_init(ph, PINCTRL_NAME_GET, sizeof(*tx),
- sizeof(*rx), &t);
-
- tx = t->tx.buf;
- rx = t->rx.buf;
- tx->identifier = identifier;
- tx->flags = SET_TYPE(cpu_to_le32(type));
-
- ret = ph->xops->do_xfer(ph, t);
- if (ret)
- goto out;
-
- if (rx->flags) {
- ret = -EINVAL;
- goto out;
- }
-
- *name = kasprintf(GFP_KERNEL, "%s", rx->name);
- if (!*name)
- ret = -ENOMEM;
- out:
- ph->xops->xfer_put(ph, t);
-
- return ret;
-}
-
static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
enum scmi_pinctrl_selector_type type,
- u32 selector, char **name,
+ u32 selector, char *name,
unsigned int *n_elems)
{
- int ret = 0;
+ int ret;
+ u32 attrs;
struct scmi_xfer *t;
struct scmi_msg_pinctrl_attributes *tx;
struct scmi_resp_pinctrl_attributes *rx;
- if (!ph || !name)
+ if (!name)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, selector, type);
@@ -264,24 +205,27 @@ static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
rx = t->rx.buf;
- tx->identifier = selector;
- tx->flags = SET_TYPE(cpu_to_le32(type));
+ tx->identifier = cpu_to_le32(selector);
+ tx->flags = cpu_to_le32(type);
ret = ph->xops->do_xfer(ph, t);
- if (ret)
- goto out;
-
- *n_elems = NUM_ELEMS(rx->attributes);
-
- if (!EXT_NAME_FLAG(rx->attributes)) {
- *name = kasprintf(GFP_KERNEL, "%s", rx->name);
- if (!*name)
- ret = -ENOMEM;
- } else {
- ret = scmi_pinctrl_get_ext_name(ph, selector, type, name);
+ if (!ret) {
+ attrs = le32_to_cpu(rx->attributes);
+ if (n_elems)
+ *n_elems = NUM_ELEMS(attrs);
+ strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
}
- out:
+
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(attrs))
+ ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
+ (u32 *)&type, name,
+ SCMI_MAX_STR_SIZE);
return ret;
}
@@ -299,7 +243,7 @@ static void iter_pinctrl_assoc_prepare_message(void *message,
const struct scmi_pinctrl_ipriv *p = priv;
msg->identifier = cpu_to_le32(p->selector);
- msg->flags = SET_TYPE(cpu_to_le32(p->type));
+ msg->flags = cpu_to_le32(p->type);
/* Set the number of OPPs to be skipped/already read */
msg->index = cpu_to_le32(desc_index);
}
@@ -307,10 +251,12 @@ static void iter_pinctrl_assoc_prepare_message(void *message,
static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
const void *response, void *priv)
{
+ u32 flags;
const struct scmi_resp_pinctrl_list_assoc *r = response;
- st->num_returned = le32_to_cpu(RETURNED(r->flags));
- st->num_remaining = le32_to_cpu(REMAINING(r->flags));
+ flags = le32_to_cpu(r->flags);
+ st->num_returned = RETURNED(flags);
+ st->num_remaining = REMAINING(flags);
return 0;
}
@@ -347,10 +293,7 @@ static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph,
.array = array,
};
- if (!ph || !array || !size)
- return -EINVAL;
-
- if (type == PIN_TYPE)
+ if (!array || !size || type == PIN_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, selector, type);
@@ -373,13 +316,12 @@ static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
enum scmi_pinctrl_selector_type type,
u8 config_type, u32 *config_value)
{
+ int ret;
+ u32 attributes;
struct scmi_xfer *t;
struct scmi_msg_conf_get *tx;
- __le32 *le_config;
- u32 attributes = 0;
- int ret;
- if (!ph || !config_value || type == FUNCTION_TYPE)
+ if (!config_value || type == FUNCTION_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, selector, type);
@@ -387,22 +329,19 @@ static int scmi_pinctrl_get_config(const struct scmi_protocol_handle *ph,
return ret;
ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx),
- sizeof(*le_config), &t);
+ sizeof(__le32), &t);
if (ret)
return ret;
tx = t->tx.buf;
- le_config = t->rx.buf;
tx->identifier = cpu_to_le32(selector);
attributes = FIELD_PREP(REG_TYPE_BITS, type) |
FIELD_PREP(REG_CONFIG, config_type);
-
tx->attributes = cpu_to_le32(attributes);
ret = ph->xops->do_xfer(ph, t);
-
if (!ret)
- *config_value = le32_to_cpu(*le_config);
+ *config_value = get_unaligned_le32(t->rx.buf);
ph->xops->xfer_put(ph, t);
return ret;
@@ -418,7 +357,7 @@ static int scmi_pinctrl_set_config(const struct scmi_protocol_handle *ph,
u32 attributes = 0;
int ret;
- if (!ph || type == FUNCTION_TYPE)
+ if (type == FUNCTION_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, selector, type);
@@ -448,11 +387,11 @@ static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
enum scmi_pinctrl_selector_type type,
u32 function_id)
{
+ int ret;
struct scmi_xfer *t;
struct scmi_msg_func_set *tx;
- int ret;
- if (!ph || type == FUNCTION_TYPE)
+ if (type == FUNCTION_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, identifier, type);
@@ -467,7 +406,7 @@ static int scmi_pinctrl_function_select(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
tx->identifier = cpu_to_le32(identifier);
tx->function_id = cpu_to_le32(function_id);
- tx->flags = SET_TYPE(cpu_to_le32(type));
+ tx->flags = cpu_to_le32(type);
ret = ph->xops->do_xfer(ph, t);
ph->xops->xfer_put(ph, t);
@@ -479,11 +418,11 @@ static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
u32 identifier,
enum scmi_pinctrl_selector_type type)
{
- struct scmi_xfer *t;
int ret;
+ struct scmi_xfer *t;
struct scmi_msg_request *tx;
- if (!ph || type == FUNCTION_TYPE)
+ if (type == FUNCTION_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, identifier, type);
@@ -494,8 +433,8 @@ static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
0, &t);
tx = t->tx.buf;
- tx->identifier = identifier;
- tx->flags = SET_TYPE(cpu_to_le32(type));
+ 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);
@@ -513,11 +452,11 @@ static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
u32 identifier,
enum scmi_pinctrl_selector_type type)
{
- struct scmi_xfer *t;
int ret;
+ struct scmi_xfer *t;
struct scmi_msg_request *tx;
- if (!ph || type == FUNCTION_TYPE)
+ if (type == FUNCTION_TYPE)
return -EINVAL;
ret = scmi_pinctrl_validate_id(ph, identifier, type);
@@ -528,8 +467,8 @@ static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
sizeof(*tx), 0, &t);
tx = t->tx.buf;
- tx->identifier = identifier;
- tx->flags = SET_TYPE(cpu_to_le32(type));
+ 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);
@@ -546,13 +485,13 @@ static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
u32 selector,
struct scmi_group_info *group)
{
- int ret = 0;
+ int ret;
- if (!ph || !group)
+ if (!group)
return -EINVAL;
ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
- &group->name,
+ group->name,
&group->nr_pins);
if (ret)
return ret;
@@ -565,33 +504,26 @@ static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins,
sizeof(*group->group_pins),
GFP_KERNEL);
- if (!group->group_pins) {
- ret = -ENOMEM;
- goto err;
- }
+ if (!group->group_pins)
+ return -ENOMEM;
ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
group->nr_pins, group->group_pins);
- if (ret)
- goto err_groups;
+ if (ret) {
+ devm_kfree(ph->dev, group->group_pins);
+ return ret;
+ }
group->present = true;
return 0;
-
- err_groups:
- kfree(group->group_pins);
- err:
- kfree(group->name);
- return ret;
}
static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
u32 selector, const char **name)
{
- int ret;
struct scmi_pinctrl_info *pi;
- if (!ph || !name)
+ if (!name)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -602,6 +534,8 @@ static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
return -EINVAL;
if (!pi->groups[selector].present) {
+ int ret;
+
ret = scmi_pinctrl_get_group_info(ph, selector,
&pi->groups[selector]);
if (ret)
@@ -617,10 +551,9 @@ static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
u32 selector, const unsigned int **pins,
unsigned int *nr_pins)
{
- int ret;
struct scmi_pinctrl_info *pi;
- if (!ph || !pins || !nr_pins)
+ if (!pins || !nr_pins)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -631,6 +564,8 @@ static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
return -EINVAL;
if (!pi->groups[selector].present) {
+ int ret;
+
ret = scmi_pinctrl_get_group_info(ph, selector,
&pi->groups[selector]);
if (ret)
@@ -640,20 +575,20 @@ static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
*pins = pi->groups[selector].group_pins;
*nr_pins = pi->groups[selector].nr_pins;
- return ret;
+ return 0;
}
static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
u32 selector,
struct scmi_function_info *func)
{
- int ret = 0;
+ int ret;
- if (!ph || !func)
+ if (!func)
return -EINVAL;
ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
- &func->name,
+ func->name,
&func->nr_groups);
if (ret)
return ret;
@@ -666,33 +601,26 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
func->groups = devm_kmalloc_array(ph->dev, func->nr_groups,
sizeof(*func->groups),
GFP_KERNEL);
- if (!func->groups) {
- ret = -ENOMEM;
- goto err;
- }
+ if (!func->groups)
+ return -ENOMEM;
ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
func->nr_groups, func->groups);
- if (ret)
- goto err_funcs;
+ if (ret) {
+ devm_kfree(ph->dev, func->groups);
+ return ret;
+ }
func->present = true;
return 0;
-
- err_funcs:
- kfree(func->groups);
- err:
- kfree(func->name);
- return ret;
}
static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
u32 selector, const char **name)
{
- int ret;
struct scmi_pinctrl_info *pi;
- if (!ph || !name)
+ if (!name)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -703,6 +631,8 @@ static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
return -EINVAL;
if (!pi->functions[selector].present) {
+ int ret;
+
ret = scmi_pinctrl_get_function_info(ph, selector,
&pi->functions[selector]);
if (ret)
@@ -718,10 +648,9 @@ static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *p
unsigned int *nr_groups,
const unsigned int **groups)
{
- int ret;
struct scmi_pinctrl_info *pi;
- if (!ph || !groups || !nr_groups)
+ if (!groups || !nr_groups)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -732,6 +661,8 @@ static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *p
return -EINVAL;
if (!pi->functions[selector].present) {
+ int ret;
+
ret = scmi_pinctrl_get_function_info(ph, selector,
&pi->functions[selector]);
if (ret)
@@ -741,7 +672,7 @@ static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *p
*groups = pi->functions[selector].groups;
*nr_groups = pi->functions[selector].nr_groups;
- return ret;
+ return 0;
}
static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
@@ -754,11 +685,10 @@ static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
u32 selector, struct scmi_pin_info *pin)
{
- int ret = 0;
+ int ret;
struct scmi_pinctrl_info *pi;
- unsigned int n_elems;
- if (!ph || !pin)
+ if (!pin)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -766,37 +696,20 @@ static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
return -EINVAL;
ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
- &pin->name,
- &n_elems);
+ pin->name, NULL);
if (ret)
return ret;
- if (n_elems != pi->nr_pins) {
- dev_err(ph->dev, "Wrong pin count expected %d has %d",
- pi->nr_pins, n_elems);
- return -ENODATA;
- }
-
- if (*pin->name == 0) {
- dev_err(ph->dev, "Pin name is empty");
- goto err;
- }
-
pin->present = true;
return 0;
-
- err:
- kfree(pin->name);
- return ret;
}
static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
u32 selector, const char **name)
{
- int ret;
struct scmi_pinctrl_info *pi;
- if (!ph || !name)
+ if (!name)
return -EINVAL;
pi = ph->get_priv(ph);
@@ -807,6 +720,8 @@ static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
return -EINVAL;
if (!pi->pins[selector].present) {
+ int ret;
+
ret = scmi_pinctrl_get_pin_info(ph, selector,
&pi->pins[selector]);
if (ret)
@@ -849,12 +764,9 @@ static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
{
+ int ret;
u32 version;
struct scmi_pinctrl_info *pinfo;
- int ret;
-
- if (!ph)
- return -EINVAL;
ret = ph->xops->version_get(ph, &version);
if (ret)
@@ -899,9 +811,6 @@ static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
int i;
struct scmi_pinctrl_info *pi;
- if (!ph)
- return -EINVAL;
-
pi = ph->get_priv(ph);
if (!pi)
return -EINVAL;
--
2.34.1
Some recently added SCMI protocols needs an additional flags parameter to
be able to properly configure the command used to query the extended name
of a resource.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/clock.c | 2 +-
drivers/firmware/arm_scmi/driver.c | 8 ++++++--
drivers/firmware/arm_scmi/perf.c | 3 ++-
drivers/firmware/arm_scmi/power.c | 2 +-
drivers/firmware/arm_scmi/powercap.c | 2 +-
drivers/firmware/arm_scmi/protocols.h | 3 ++-
drivers/firmware/arm_scmi/reset.c | 3 ++-
drivers/firmware/arm_scmi/sensors.c | 2 +-
drivers/firmware/arm_scmi/voltage.c | 2 +-
9 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 96060bf90a24..e6e087686e8c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
if (SUPPORTS_EXTENDED_NAMES(attributes))
ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
- clk->name,
+ NULL, clk->name,
SCMI_MAX_STR_SIZE);
if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..5be931a07c84 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
* @ph: A protocol handle reference.
* @cmd_id: The specific command ID to use.
* @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
* @name: A pointer to the preallocated area where the retrieved name will be
* stored as a NULL terminated string.
* @len: The len in bytes of the @name char array.
@@ -1445,8 +1446,8 @@ struct scmi_msg_resp_domain_name_get {
* Return: 0 on Succcess
*/
static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
- u8 cmd_id, u32 res_id, char *name,
- size_t len)
+ u8 cmd_id, u32 res_id, u32 *flags,
+ char *name, size_t len)
{
int ret;
struct scmi_xfer *t;
@@ -1458,6 +1459,9 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
goto out;
put_unaligned_le32(res_id, t->tx.buf);
+ if (flags)
+ put_unaligned_le32(*flags,
+ (u8 *)t->tx.buf + sizeof(res_id));
resp = t->rx.buf;
ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ecf5c4de851b..d85d4a0e3605 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -237,7 +237,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
SUPPORTS_EXTENDED_NAMES(flags))
ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET, domain,
- dom_info->name, SCMI_MAX_STR_SIZE);
+ NULL, dom_info->name,
+ SCMI_MAX_STR_SIZE);
return ret;
}
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 356e83631664..077767d6e902 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -133,7 +133,7 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
SUPPORTS_EXTENDED_NAMES(flags)) {
ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
- domain, dom_info->name,
+ domain, NULL, dom_info->name,
SCMI_MAX_STR_SIZE);
}
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 244929cb4f3e..86c9a0b5a765 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -270,7 +270,7 @@ scmi_powercap_domain_attributes_get(const struct scmi_protocol_handle *ph,
*/
if (!ret && SUPPORTS_EXTENDED_NAMES(flags))
ph->hops->extended_name_get(ph, POWERCAP_DOMAIN_NAME_GET,
- domain, dom_info->name,
+ domain, NULL, dom_info->name,
SCMI_MAX_STR_SIZE);
return ret;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 78e1a01eb656..b3c6314bb4b8 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -256,7 +256,8 @@ struct scmi_fc_info {
*/
struct scmi_proto_helpers_ops {
int (*extended_name_get)(const struct scmi_protocol_handle *ph,
- u8 cmd_id, u32 res_id, char *name, size_t len);
+ u8 cmd_id, u32 res_id, u32 *flags, char *name,
+ size_t len);
void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
struct scmi_iterator_ops *ops,
unsigned int max_resources, u8 msg_id,
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index e9afa8cab730..7217fd7c6afa 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -128,7 +128,8 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
SUPPORTS_EXTENDED_NAMES(attributes))
ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
- dom_info->name, SCMI_MAX_STR_SIZE);
+ NULL, dom_info->name,
+ SCMI_MAX_STR_SIZE);
return ret;
}
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0b5853fa9d87..9952a7bc6682 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -644,7 +644,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
SUPPORTS_EXTENDED_NAMES(attrl))
ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
- s->name, SCMI_MAX_STR_SIZE);
+ NULL, s->name, SCMI_MAX_STR_SIZE);
if (s->extended_scalar_attrs) {
s->sensor_power = le32_to_cpu(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index eaa8d944926a..36e2df77738c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -242,7 +242,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
if (SUPPORTS_EXTENDED_NAMES(attributes))
ph->hops->extended_name_get(ph,
VOLTAGE_DOMAIN_NAME_GET,
- v->id, v->name,
+ v->id, NULL, v->name,
SCMI_MAX_STR_SIZE);
if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
v->async_level_set = true;
--
2.34.1
Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.
>
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
>
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.
...
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
I do not see any user of this header. Do you?
> +#include <linux/module.h>
> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> +#include <linux/slab.h>
Please, move these two to the upper group of the generic headers.
> +struct scmi_pinctrl_funcs {
> + unsigned int num_groups;
> + const char **groups;
> +};
Please, use struct pinfunction.
...
> +struct scmi_pinctrl {
> + struct scmi_pinctrl_funcs *functions;
> + unsigned int nr_functions;
> + char **groups;
> + unsigned int nr_groups;
I'm not sure what is the difference to what "functions" above represent.
> +};
...
> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
> + struct seq_file *s,
> + unsigned int offset)
> +{
> + seq_puts(s, DRV_NAME);
> +}
What is the usefulness of this method?
...
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx;
> + enum pin_config_param config_type;
> + unsigned long config_value;
> +
> + if (!pctldev)
> + return -EINVAL;
> +
> + pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!pmx || !pmx->ph || !config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
> + config_type, (u32 *)&config_value);
Endianess issue. This is, while likely working code, still ugly.
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
...
> + err:
err_free.
> + kfree(pmx->pins);
> + pmx->nr_pins = 0;
> +
> + return ret;
...
> +static const struct scmi_device_id scmi_id_table[] = {
> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> + { },
No comma for the terminator entry.
> +};
...
> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> + &ph);
Can be on one line.
> + if (IS_ERR(pinctrl_ops))
> + return PTR_ERR(pinctrl_ops);
...
> + if (pmx->nr_functions) {
> + pmx->functions =
> + devm_kcalloc(&sdev->dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions) {
> + ret = -ENOMEM;
> + goto clean;
Interleaving devm_*() with non-devm_*() in such order is not a good idea.
> + }
> + }
> +
> + if (pmx->nr_groups) {
> + pmx->groups =
> + devm_kcalloc(&sdev->dev, pmx->nr_groups,
> + sizeof(*pmx->groups),
> + GFP_KERNEL);
> + if (!pmx->groups) {
> + ret = -ENOMEM;
> + goto clean;
> + }
> + }
> +
> + return pinctrl_enable(pmx->pctldev);
> +
> +clean:
err_free:
> + if (pmx) {
> + kfree(pmx->functions);
> + kfree(pmx->groups);
Ah, this is simply wrong.
> + }
> +
> + kfree(pmx);
--
With Best Regards,
Andy Shevchenko
Hi Cristian,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20230506-041158/Oleksii-Moisieiev/pinctrl-Implementation-of-the-generic-scmi-pinctrl-driver/20230426-222739
base: the 1th patch of https://lore.kernel.org/r/b4d60f3408f8fe839933fa3938ecdc9bfceb75d7.1682513390.git.oleksii_moisieiev%40epam.com
patch link: https://lore.kernel.org/r/20230505201012.3171817-1-cristian.marussi%40arm.com
patch subject: [PATCH] [REVIEW][PINCTRL]: Misc Fixes and refactor
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230506/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c3e47504d3b8eac203b4ae3fc56c3791790dd88b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review UPDATE-20230506-041158/Oleksii-Moisieiev/pinctrl-Implementation-of-the-generic-scmi-pinctrl-driver/20230426-222739
git checkout c3e47504d3b8eac203b4ae3fc56c3791790dd88b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/firmware/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/firmware/arm_scmi/pinctrl.c: In function 'scmi_pinctrl_attributes':
drivers/firmware/arm_scmi/pinctrl.c:227:45: error: passing argument 4 of '((const struct scmi_proto_helpers_ops *)ph->hops)->extended_name_get' from incompatible pointer type [-Werror=incompatible-pointer-types]
227 | (u32 *)&type, name,
| ^~~~~~~~~~~~
| |
| u32 * {aka unsigned int *}
drivers/firmware/arm_scmi/pinctrl.c:227:45: note: expected 'char *' but argument is of type 'u32 *' {aka 'unsigned int *'}
>> drivers/firmware/arm_scmi/pinctrl.c:227:59: warning: passing argument 5 of '((const struct scmi_proto_helpers_ops *)ph->hops)->extended_name_get' makes integer from pointer without a cast [-Wint-conversion]
227 | (u32 *)&type, name,
| ^~~~
| |
| char *
drivers/firmware/arm_scmi/pinctrl.c:227:59: note: expected 'size_t' {aka 'unsigned int'} but argument is of type 'char *'
drivers/firmware/arm_scmi/pinctrl.c:226:17: error: too many arguments to function '((const struct scmi_proto_helpers_ops *)ph->hops)->extended_name_get'
226 | ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
| ^~
cc1: some warnings being treated as errors
vim +227 drivers/firmware/arm_scmi/pinctrl.c
182
183 static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
184 enum scmi_pinctrl_selector_type type,
185 u32 selector, char *name,
186 unsigned int *n_elems)
187 {
188 int ret;
189 u32 attrs;
190 struct scmi_xfer *t;
191 struct scmi_msg_pinctrl_attributes *tx;
192 struct scmi_resp_pinctrl_attributes *rx;
193
194 if (!name)
195 return -EINVAL;
196
197 ret = scmi_pinctrl_validate_id(ph, selector, type);
198 if (ret)
199 return ret;
200
201 ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
202 sizeof(*rx), &t);
203 if (ret)
204 return ret;
205
206 tx = t->tx.buf;
207 rx = t->rx.buf;
208 tx->identifier = cpu_to_le32(selector);
209 tx->flags = cpu_to_le32(type);
210
211 ret = ph->xops->do_xfer(ph, t);
212 if (!ret) {
213 attrs = le32_to_cpu(rx->attributes);
214 if (n_elems)
215 *n_elems = NUM_ELEMS(attrs);
216 strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
217 }
218
219 ph->xops->xfer_put(ph, t);
220
221 /*
222 * If supported overwrite short name with the extended one;
223 * on error just carry on and use already provided short name.
224 */
225 if (!ret && EXT_NAME_FLAG(attrs))
226 ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> 227 (u32 *)&type, name,
228 SCMI_MAX_STR_SIZE);
229 return ret;
230 }
231
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > scmi: Introduce pinctrl SCMI protocol driver
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > excluding GPIO support. All pinctrl related callbacks and operations
> > are exposed in the include/linux/scmi_protocol.h
> >
>
> Hi Oleksii,
Hi Oleksii,
mostly replying to myself here really (O_o) ... see below.
[snip]
> > +
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> > + struct scmi_pinctrl_info *pi)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + if (!pi)
> > + return -EINVAL;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
> > + 0, sizeof(*attr), &t);
> > + if (ret)
> > + return ret;
> > +
> > + attr = t->rx.buf;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + pi->nr_functions =
> > + le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
> > + pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
> > + pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));
>
> I see a couple of issues here present in general all across this patch when
> you use these macros;
>
> You should take care of the endianity in the RX msg payload BEFORE and THEN
> DISSECT the bitfields AND as a consequence use also an _le helper that fits
> the size of the type that you are processing as in (being attributes 32 bit
> little endian in the msg payload):
>
> pi->nr_pins = GET_PINS_NR(le32_to_cpu(attr->attributes_low));
>
> Now all works just because everything is little endian really so nothing
> is done by these macros....
>
Re-thinking about this, it turns out that the above advice of mine is
just plain wrong :< ... being a bitfield access you cannot do what I
badly advised you:
GET_PINS_NR(le32_to_cpu(attr->attributes_low));
the right way is indeed how you did it originally...my bad...
... BUT at the same time if you check your original solution:
le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high))
with a static analyzer like sparse, which dutifully checks that __le32
style labeled are accessed in a congruent manner, I get a lot of:
drivers/firmware/arm_scmi/pinctrl.c:136:25: warning: cast to restricted __le32
drivers/firmware/arm_scmi/pinctrl.c:136:25: warning: restricted __le32 degrades to integer
drivers/firmware/arm_scmi/pinctrl.c:136:25: warning: restricted __le32 degrades to integer
drivers/firmware/arm_scmi/pinctrl.c:136:25: warning: cast to restricted __le16
with your original solution and no errors with my broken proposal... :O
...this is due to the fact that indeed FIELD_GET & C. are not meant to
be used on such __le32 endian-styled vars, so sparse warns you about that
(but my bad advice remain broken even though fine for the static analyzer...)
Digging into bitfield.h, indeed, turns out that near the end, there are
defined some LE dedicated bitfield macros (https://lwn.net/Articles/741762/)
like:
u32 le32_get_bits(__le32 val, u32 field)
So you can just convert your macros to use le32_get_bits (insted of FIELD_GET) and
use those macros directly without the need for the additional le32_to_cpu conversion
as in the example below. (that indeed gives me no more sparse errors)
Hope not to have made to much noise :P
Thanks,
Cristian
P.S.: I would also drop the RFC on V3 so that maybe a few more public check-bots
will engage with your series
->8---
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 1392d15b3a58..66ea51606f46 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -14,15 +14,15 @@
#define REG_TYPE_BITS GENMASK(9, 8)
#define REG_CONFIG GENMASK(7, 0)
-#define GET_GROUPS_NR(x) FIELD_GET(GENMASK(31, 16), (x))
-#define GET_PINS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
-#define GET_FUNCTIONS_NR(x) FIELD_GET(GENMASK(15, 0), (x))
+#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) FIELD_GET(BIT(31), (x))
-#define NUM_ELEMS(x) FIELD_GET(GENMASK(15, 0), (x))
+#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) FIELD_GET(GENMASK(31, 16), (x))
-#define RETURNED(x) FIELD_GET(GENMASK(11, 0), (x))
+#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
+#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
enum scmi_pinctrl_protocol_cmd {
PINCTRL_ATTRIBUTES = 0x3,
@@ -132,10 +132,9 @@ static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
- pi->nr_functions =
- le16_to_cpu(GET_FUNCTIONS_NR(attr->attributes_high));
- pi->nr_groups = le16_to_cpu(GET_GROUPS_NR(attr->attributes_low));
- pi->nr_pins = le16_to_cpu(GET_PINS_NR(attr->attributes_low));
+ 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);
@@ -209,7 +208,7 @@ static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
ret = ph->xops->do_xfer(ph, t);
if (!ret) {
if (n_elems)
- *n_elems = le32_to_cpu(NUM_ELEMS(rx->attributes));
+ *n_elems = NUM_ELEMS(rx->attributes);
strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
}
@@ -253,8 +252,8 @@ static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
{
const struct scmi_resp_pinctrl_list_assoc *r = response;
- st->num_returned = le32_to_cpu(RETURNED(r->flags));
- st->num_remaining = le32_to_cpu(REMAINING(r->flags));
+ st->num_returned = RETURNED(r->flags);
+ st->num_remaining = REMAINING(r->flags);
return 0;
}
-8<---
Hi Cristian,
kernel test robot noticed the following build errors:
url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20230506-041158/Oleksii-Moisieiev/pinctrl-Implementation-of-the-generic-scmi-pinctrl-driver/20230426-222739
base: the 1th patch of https://lore.kernel.org/r/b4d60f3408f8fe839933fa3938ecdc9bfceb75d7.1682513390.git.oleksii_moisieiev%40epam.com
patch link: https://lore.kernel.org/r/20230505201012.3171817-1-cristian.marussi%40arm.com
patch subject: [PATCH] [REVIEW][PINCTRL]: Misc Fixes and refactor
config: arm-randconfig-r011-20230508 (https://download.01.org/0day-ci/archive/20230509/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/c3e47504d3b8eac203b4ae3fc56c3791790dd88b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review UPDATE-20230506-041158/Oleksii-Moisieiev/pinctrl-Implementation-of-the-generic-scmi-pinctrl-driver/20230426-222739
git checkout c3e47504d3b8eac203b4ae3fc56c3791790dd88b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/firmware/arm_scmi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/firmware/arm_scmi/pinctrl.c:228:10: error: too many arguments to function call, expected 5, have 6
SCMI_MAX_STR_SIZE);
^~~~~~~~~~~~~~~~~
include/linux/scmi_protocol.h:16:28: note: expanded from macro 'SCMI_MAX_STR_SIZE'
#define SCMI_MAX_STR_SIZE 64
^~
1 error generated.
vim +228 drivers/firmware/arm_scmi/pinctrl.c
182
183 static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
184 enum scmi_pinctrl_selector_type type,
185 u32 selector, char *name,
186 unsigned int *n_elems)
187 {
188 int ret;
189 u32 attrs;
190 struct scmi_xfer *t;
191 struct scmi_msg_pinctrl_attributes *tx;
192 struct scmi_resp_pinctrl_attributes *rx;
193
194 if (!name)
195 return -EINVAL;
196
197 ret = scmi_pinctrl_validate_id(ph, selector, type);
198 if (ret)
199 return ret;
200
201 ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
202 sizeof(*rx), &t);
203 if (ret)
204 return ret;
205
206 tx = t->tx.buf;
207 rx = t->rx.buf;
208 tx->identifier = cpu_to_le32(selector);
209 tx->flags = cpu_to_le32(type);
210
211 ret = ph->xops->do_xfer(ph, t);
212 if (!ret) {
213 attrs = le32_to_cpu(rx->attributes);
214 if (n_elems)
215 *n_elems = NUM_ELEMS(attrs);
216 strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
217 }
218
219 ph->xops->xfer_put(ph, t);
220
221 /*
222 * If supported overwrite short name with the extended one;
223 * on error just carry on and use already provided short name.
224 */
225 if (!ret && EXT_NAME_FLAG(attrs))
226 ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
227 (u32 *)&type, name,
> 228 SCMI_MAX_STR_SIZE);
229 return ret;
230 }
231
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Cristian,
Please see below.
On 05.05.23 23:01, Cristian Marussi wrote:
> On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
>> scmi-pinctrl driver implements pinctrl driver interface and using
>> SCMI protocol to redirect messages from pinctrl subsystem SDK to
>> SCP firmware, which does the changes in HW.
>>
>> This setup expects SCP firmware (or similar system, such as ATF)
>> to be installed on the platform, which implements pinctrl driver
>> for the specific platform.
>>
>> SCMI-Pinctrl driver should be configured from the device-tree and uses
>> generic device-tree mappings for the configuration.
>>
>
> Hi Oleksii,
>
> just a few remarks down below.
>
>> Signed-off-by: Oleksii Moisieiev <[email protected]>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pinctrl/Kconfig | 9 +
>> drivers/pinctrl/Makefile | 1 +
>> drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++
>> 4 files changed, 589 insertions(+)
>> create mode 100644 drivers/pinctrl/pinctrl-scmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0d251ebac437..ba9e3aea6176 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <[email protected]>
>> L: [email protected]
>> S: Maintained
>> F: drivers/firmware/arm_scmi/pinctrl.c
>> +F: drivers/pinctrl/pinctrl-scmi.c
>>
>> SYSTEM RESET/SHUTDOWN DRIVERS
>> M: Sebastian Reichel <[email protected]>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index dcb53c4a9584..16bf2c67f095 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig"
>> source "drivers/pinctrl/visconti/Kconfig"
>> source "drivers/pinctrl/vt8500/Kconfig"
>>
>> +config PINCTRL_SCMI
>> + bool "Pinctrl driver controlled via SCMI interface"
>
> Why not a tristate ? The core SCMI stack can be compiled as module with
> all the protocols...at least for testing is useful to have this driver
> as M.
>
Yes, thanks. Will fix in v3.
>> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
>> + help
>> + This driver provides support for pinctrl which is controlled
>> + by firmware that implements the SCMI interface.
>> + It uses SCMI Message Protocol to interact with the
>> + firmware providing all the pinctrl controls.
>> +
>> endif
>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>> index d5939840bb2a..21366db4f4f4 100644
>> --- a/drivers/pinctrl/Makefile
>> +++ b/drivers/pinctrl/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
>> obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
>> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
>> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
>> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
>>
>> obj-y += actions/
>> obj-$(CONFIG_ARCH_ASPEED) += aspeed/
>> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
>> new file mode 100644
>> index 000000000000..8401db1d030b
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-scmi.c
>> @@ -0,0 +1,578 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
>> + *
>> + * Copyright (C) 2023 EPAM
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
>> +#include <linux/slab.h>
>> +
>> +#include "pinctrl-utils.h"
>> +#include "core.h"
>> +#include "pinconf.h"
>> +
>> +#define DRV_NAME "scmi-pinctrl"
>> +
>> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
>> +
>> +struct scmi_pinctrl_funcs {
>> + unsigned int num_groups;
>> + const char **groups;
>> +};
>> +
>> +struct scmi_pinctrl {
>> + struct device *dev;
>> + struct scmi_protocol_handle *ph;
>> + struct pinctrl_dev *pctldev;
>> + struct pinctrl_desc pctl_desc;
>> + struct scmi_pinctrl_funcs *functions;
>> + unsigned int nr_functions;
>> + char **groups;
>> + unsigned int nr_groups;
>> + struct pinctrl_pin_desc *pins;
>> + unsigned int nr_pins;
>> +};
>> +
>> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE);
>> +}
>> +
>> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
>> + unsigned int selector)
>> +{
>> + int ret;
>> + const char *name;
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return NULL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return NULL;
>> +
>> + ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name);
>> + if (ret) {
>> + dev_err(pmx->dev, "get name failed with err %d", ret);
>> + return NULL;
>> + }
>> +
>> + return name;
>> +}
>> +
>> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
>> + unsigned int selector,
>> + const unsigned int **pins,
>> + unsigned int *num_pins)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->get_group_pins(pmx->ph, selector,
>> + pins, num_pins);
>> +}
>> +
>> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
>> + struct seq_file *s,
>> + unsigned int offset)
>> +{
>> + seq_puts(s, DRV_NAME);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
>> + struct device_node *np_config,
>> + struct pinctrl_map **map,
>> + u32 *num_maps)
>> +{
>> + return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
>> + num_maps, PIN_MAP_TYPE_INVALID);
>> +}
>> +
>> +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
>> + struct pinctrl_map *map, u32 num_maps)
>> +{
>> + kfree(map);
>> +}
>> +
>> +#endif /* CONFIG_OF */
>> +
>> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
>> + .get_groups_count = pinctrl_scmi_get_groups_count,
>> + .get_group_name = pinctrl_scmi_get_group_name,
>> + .get_group_pins = pinctrl_scmi_get_group_pins,
>> + .pin_dbg_show = pinctrl_scmi_pin_dbg_show,
>> +#ifdef CONFIG_OF
>> + .dt_node_to_map = pinctrl_scmi_dt_node_to_map,
>> + .dt_free_map = pinctrl_scmi_dt_free_map,
>> +#endif
>> +};
>> +
>> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE);
>> +}
>> +
>> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
>> + unsigned int selector)
>> +{
>> + int ret;
>> + const char *name;
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return NULL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return NULL;
>> +
>> + ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name);
>> + if (ret) {
>> + dev_err(pmx->dev, "get name failed with err %d", ret);
>> + return NULL;
>> + }
>> +
>> + return name;
>> +}
>> +
>> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
>> + unsigned int selector,
>> + const char * const **groups,
>> + unsigned int * const num_groups)
>> +{
>> + const unsigned int *group_ids;
>> + int ret, i;
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !groups || !num_groups)
>> + return -EINVAL;
>> +
>> + if (selector < pmx->nr_functions &&
>> + pmx->functions[selector].num_groups) {
>> + *groups = (const char * const *)pmx->functions[selector].groups;
>> + *num_groups = pmx->functions[selector].num_groups;
>> + return 0;
>> + }
>> +
>> + ret = pinctrl_ops->get_function_groups(pmx->ph, selector,
>> + &pmx->functions[selector].num_groups,
>> + &group_ids);
>> + if (ret) {
>> + dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
>> + return ret;
>> + }
>> +
>> + *num_groups = pmx->functions[selector].num_groups;
>> + if (!*num_groups)
>> + return -EINVAL;
>> +
>> + pmx->functions[selector].groups =
>> + devm_kcalloc(pmx->dev, *num_groups,
>> + sizeof(*pmx->functions[selector].groups),
>> + GFP_KERNEL);
>> + if (!pmx->functions[selector].groups)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < *num_groups; i++) {
>> + pmx->functions[selector].groups[i] =
>> + pinctrl_scmi_get_group_name(pmx->pctldev,
>> + group_ids[i]);
>> + if (!pmx->functions[selector].groups[i]) {
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>> + }
>> +
>> + *groups = (const char * const *)pmx->functions[selector].groups;
>> +
>> + return 0;
>> +
>> +error:
>> + kfree(pmx->functions[selector].groups);
>
> devm_kfree ?
>
Thanks, fixed.
>> +
>> + return ret;
>> +}
>> +
>> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
>> + unsigned int selector, unsigned int group)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->set_mux(pmx->ph, selector, group);
>> +}
>> +
>> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
>> + unsigned int offset)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->request_pin(pmx->ph, offset);
>> +}
>> +
>> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
>> +{
>> + struct scmi_pinctrl *pmx;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + return pinctrl_ops->free_pin(pmx->ph, offset);
>> +}
>> +
>> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
>> + .request = pinctrl_scmi_request,
>> + .free = pinctrl_scmi_free,
>> + .get_functions_count = pinctrl_scmi_get_functions_count,
>> + .get_function_name = pinctrl_scmi_get_function_name,
>> + .get_function_groups = pinctrl_scmi_get_function_groups,
>> + .set_mux = pinctrl_scmi_func_set_mux,
>> +};
>> +
>> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
>> + unsigned int _pin,
>> + unsigned long *config)
>> +{
>> + int ret;
>> + struct scmi_pinctrl *pmx;
>> + enum pin_config_param config_type;
>> + unsigned long config_value;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !config)
>> + return -EINVAL;
>> +
>> + config_type = pinconf_to_config_param(*config);
>> +
>> + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
>> + (u32 *)&config_value);
>> + if (ret)
>> + return ret;
>> +
>> + *config = pinconf_to_config_packed(config_type, config_value);
>> +
>> + return 0;
>> +}
>> +
>> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
>> + unsigned int _pin,
>> + unsigned long *configs,
>> + unsigned int num_configs)
>> +{
>> + int i, ret;
>> + struct scmi_pinctrl *pmx;
>> + enum pin_config_param config_type;
>> + unsigned long config_value;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < num_configs; i++) {
>> + config_type = pinconf_to_config_param(configs[i]);
>> + config_value = pinconf_to_config_argument(configs[i]);
>> +
>> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
>> + config_value);
>> + if (ret) {
>> + dev_err(pmx->dev, "Error parsing config %ld\n",
>> + configs[i]);
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
>> + unsigned int group,
>> + unsigned long *configs,
>> + unsigned int num_configs)
>> +{
>> + int i, ret;
>> + struct scmi_pinctrl *pmx;
>> + enum pin_config_param config_type;
>> + unsigned long config_value;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !configs || num_configs == 0)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < num_configs; i++) {
>> + config_type = pinconf_to_config_param(configs[i]);
>> + config_value = pinconf_to_config_argument(configs[i]);
>> +
>> + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
>> + config_type, config_value);
>> + if (ret) {
>> + dev_err(pmx->dev, "Error parsing config = %ld",
>> + configs[i]);
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +};
>> +
>> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
>> + unsigned int _pin,
>> + unsigned long *config)
>> +{
>> + int ret;
>> + struct scmi_pinctrl *pmx;
>> + enum pin_config_param config_type;
>> + unsigned long config_value;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !config)
>> + return -EINVAL;
>> +
>> + config_type = pinconf_to_config_param(*config);
>> +
>> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
>> + config_type, (u32 *)&config_value);
>> + if (ret)
>> + return ret;
>> +
>> + *config = pinconf_to_config_packed(config_type, config_value);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
>> + .is_generic = true,
>> + .pin_config_get = pinctrl_scmi_pinconf_get,
>> + .pin_config_set = pinctrl_scmi_pinconf_set,
>> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set,
>> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get,
>> + .pin_config_config_dbg_show = pinconf_generic_dump_config,
>> +};
>> +
>> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
>> + unsigned int *nr_pins,
>> + const struct pinctrl_pin_desc **pins)
>> +{
>> + int ret, i;
>> +
>> + if (!pmx || !pmx->ph)
>> + return -EINVAL;
>> +
>> + if (!pins || !nr_pins)
>> + return -EINVAL;
>> +
>> + if (pmx->nr_pins) {
>> + *pins = pmx->pins;
>> + *nr_pins = pmx->nr_pins;
>> + return 0;
>> + }
>> +
>> + *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE);
>> +
>> + pmx->nr_pins = *nr_pins;
>> + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins),
>> + GFP_KERNEL);
>> + if (!pmx->pins)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < *nr_pins; i++) {
>> + pmx->pins[i].number = i;
>> + ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE,
>> + &pmx->pins[i].name);
>> + if (ret) {
>> + dev_err(pmx->dev, "Can't get name for pin %d: rc %d",
>> + i, ret);
>> + goto err;
>> + }
>> + }
>> +
>> + *pins = pmx->pins;
>> + dev_dbg(pmx->dev, "got pins %d", *nr_pins);
>> +
>> + return 0;
>> + err:
>> + kfree(pmx->pins);
> devm_kfree ? but anyway when this fails it will trigger the _probe to
> fail so all devres will be released..so not needed probably at the end.
>
>> + pmx->nr_pins = 0;
>> +
>> + return ret;
>> +}
>> +
>> +static const struct scmi_device_id scmi_id_table[] = {
>> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>> +
>> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
>> +{
>> + int ret;
>> + struct scmi_pinctrl *pmx;
>> + const struct scmi_handle *handle;
>> + struct scmi_protocol_handle *ph;
>> +
>> + if (!sdev || !sdev->handle)
>> + return -EINVAL;
>> +
>> + handle = sdev->handle;
>> +
>> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
>> + &ph);
>> + if (IS_ERR(pinctrl_ops))
>> + return PTR_ERR(pinctrl_ops);
>> +
>> + pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
>> + if (!pmx)
>> + return -ENOMEM;
>> +
>> + pmx->ph = ph;
>> +
>> + pmx->dev = &sdev->dev;
>> + pmx->pctl_desc.name = DRV_NAME;
>> + pmx->pctl_desc.owner = THIS_MODULE;
>> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
>> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
>> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
>> +
>> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
>> + &pmx->pctl_desc.pins);
>> + if (ret)
>> + goto clean;
>> +
>> + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
>> + &pmx->pctldev);
>> + if (ret) {
>> + dev_err(&sdev->dev, "could not register: %i\n", ret);
>> + goto clean;
>> + }
>> +
>> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
>> + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
>> +
>> + if (pmx->nr_functions) {
>> + pmx->functions =
>> + devm_kcalloc(&sdev->dev, pmx->nr_functions,
>> + sizeof(*pmx->functions),
>> + GFP_KERNEL);
>> + if (!pmx->functions) {
>> + ret = -ENOMEM;
>> + goto clean;
>> + }
>> + }
>> +
>> + if (pmx->nr_groups) {
>> + pmx->groups =
>> + devm_kcalloc(&sdev->dev, pmx->nr_groups,
>> + sizeof(*pmx->groups),
>> + GFP_KERNEL);
>> + if (!pmx->groups) {
>> + ret = -ENOMEM;
>> + goto clean;
>> + }
>> + }
>> +
>> + return pinctrl_enable(pmx->pctldev);
>> +
>> +clean:
>> + if (pmx) {
>> + kfree(pmx->functions);
>> + kfree(pmx->groups);
>> + }
>> +
>> + kfree(pmx);
>
> All of these are devres allocated...it does not seem to me that they
> require explicit freeing on the _probe() failure path.
> (indeed you dont need even a .remove function)
>
Removed this block.
> Thanks,
> Cristian
Hello Andy,
On 05.05.23 23:35, [email protected] wrote:
> Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
>> scmi-pinctrl driver implements pinctrl driver interface and using
>> SCMI protocol to redirect messages from pinctrl subsystem SDK to
>> SCP firmware, which does the changes in HW.
>>
>> This setup expects SCP firmware (or similar system, such as ATF)
>> to be installed on the platform, which implements pinctrl driver
>> for the specific platform.
>>
>> SCMI-Pinctrl driver should be configured from the device-tree and uses
>> generic device-tree mappings for the configuration.
>
> ...
>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>
>> +#include <linux/of.h>
>
> I do not see any user of this header. Do you?
>
Yes, thanks. Removing
>> +#include <linux/module.h>
>> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
>> +#include <linux/slab.h>
>
> Please, move these two to the upper group of the generic headers.
>
Thanks, fixed.
>> +struct scmi_pinctrl_funcs {
>> + unsigned int num_groups;
>> + const char **groups;
>> +};
>
> Please, use struct pinfunction.
>
I can't use pincfunction here because it has the following groups
definition:
const char * const *groups;
Which is meant to be constantly allocated.
So I when I try to gather list of groups in
pinctrl_scmi_get_function_groups I will receive compilation error.
> ...
>
>> +struct scmi_pinctrl {
>
>> + struct scmi_pinctrl_funcs *functions;
>> + unsigned int nr_functions;
>
>> + char **groups;
>> + unsigned int nr_groups;
>
> I'm not sure what is the difference to what "functions" above represent.
>
The difference is that each function has a list of group names, so we
have to get name for each group in this function. I'm saving array to
avoid extra SCMI calls to gather groups in function.
>> +};
>
> ...
>
>> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
>> + struct seq_file *s,
>> + unsigned int offset)
>> +{
>> + seq_puts(s, DRV_NAME);
>> +}
>
> What is the usefulness of this method?
>
Removed
> ...
>
>> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
>> + unsigned int _pin,
>> + unsigned long *config)
>> +{
>> + int ret;
>> + struct scmi_pinctrl *pmx;
>> + enum pin_config_param config_type;
>> + unsigned long config_value;
>> +
>> + if (!pctldev)
>> + return -EINVAL;
>> +
>> + pmx = pinctrl_dev_get_drvdata(pctldev);
>> +
>> + if (!pmx || !pmx->ph || !config)
>> + return -EINVAL;
>> +
>> + config_type = pinconf_to_config_param(*config);
>> +
>> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
>> + config_type, (u32 *)&config_value);
>
> Endianess issue. This is, while likely working code, still ugly.
>
Fixed.
>> + if (ret)
>> + return ret;
>> +
>> + *config = pinconf_to_config_packed(config_type, config_value);
>> +
>> + return 0;
>> +}
>
> ...
>
>> + err:
>
> err_free.
>
Fixed
>> + kfree(pmx->pins);
>> + pmx->nr_pins = 0;
>> +
>> + return ret;
>
> ...
>
>> +static const struct scmi_device_id scmi_id_table[] = {
>> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
>
>> + { },
>
> No comma for the terminator entry.
>
Removed.
>> +};
>
> ...
>
>> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
>> + &ph);
> > Can be on one line.
Fixed
>
>> + if (IS_ERR(pinctrl_ops))
>> + return PTR_ERR(pinctrl_ops);
>
> ...
>
>> + if (pmx->nr_functions) {
>> + pmx->functions =
>> + devm_kcalloc(&sdev->dev, pmx->nr_functions,
>> + sizeof(*pmx->functions),
>> + GFP_KERNEL);
>> + if (!pmx->functions) {
>> + ret = -ENOMEM;
>> + goto clean;
>
> Interleaving devm_*() with non-devm_*() in such order is not a good idea.
>
Thanks, fixed.
>> + }
>> + }
>> +
>> + if (pmx->nr_groups) {
>> + pmx->groups =
>> + devm_kcalloc(&sdev->dev, pmx->nr_groups,
>> + sizeof(*pmx->groups),
>> + GFP_KERNEL);
>> + if (!pmx->groups) {
>> + ret = -ENOMEM;
>> + goto clean;
>> + }
>> + }
>> +
>> + return pinctrl_enable(pmx->pctldev);
>> +
>> +clean:
>
> err_free:
removed.
>
>> + if (pmx) {
>> + kfree(pmx->functions);
>> + kfree(pmx->groups);
>
> Ah, this is simply wrong.
>
Thanks, removed.
>> + }
>> +
>> + kfree(pmx);
>
On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > scmi: Introduce pinctrl SCMI protocol driver
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > excluding GPIO support. All pinctrl related callbacks and operations
> > are exposed in the include/linux/scmi_protocol.h
> >
>
> Hi Oleksii,
>
> Thanks for this.
>
> I tried out this in an emulated setup and found just a minor issue from
> the spec/functional point of view...then I reworked the extended names
> support using a modified hops->extended_name_get helper (as said the core
> SCMI support needed a small modification to support PINCTRL): I'll reply
> to this mail thread with such core SCMI modification patch, so you can
> include this patch of mine in your next V3 and use it in your series.
>
> Moreover, given that I wanted to test such rework of mine and a bunch
> of other cleanups I did (as detailed down below), and it seemed silly
> to throw all away just to then having to detail all to you, I'll also
> include in another distinct reply the raw diff of what I changed in
> your series to use the new extended_name support and a few other cleanups,
> so that, if you want, you can just quickly merge that into your V3 patch
> (of course if you like it and tests fine also for you...)...these are
> small changes, if you take it, no need to bother with authorship and
> attribution from my point of view.
>
Hi Cristian,
Thank you for the patches. I've applied them and tested with powerpc,
mx68 and clang environments (as test-robot complained about).
> > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > ---
> > MAINTAINERS | 6 +
[snip]
> > SYSTEM RESET/SHUTDOWN DRIVERS
> > M: Sebastian Reichel <[email protected]>
> > L: [email protected]
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index b31d78fa66cc..071ac65f22b9 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> > scmi-core-objs := $(scmi-bus-y)
> >
> > scmi-driver-y = driver.o notify.o
> > +
>
> Do not add spurios lines.
>
Thanks, removed
> > scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> >
I've applied patches you provided and made a small fixes. I'm going to
make patch:
"firmware: arm_scmi: Add optional flags to extended names helper"
as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
don't mind.
-- Oleksii
On Fri, May 12, 2023 at 08:38:06AM +0000, Oleksii Moisieiev wrote:
> On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> > On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > > scmi: Introduce pinctrl SCMI protocol driver
> > >
> > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > excluding GPIO support. All pinctrl related callbacks and operations
> > > are exposed in the include/linux/scmi_protocol.h
> > >
> >
> > Hi Oleksii,
> >
> > Thanks for this.
> >
> > I tried out this in an emulated setup and found just a minor issue from
> > the spec/functional point of view...then I reworked the extended names
> > support using a modified hops->extended_name_get helper (as said the core
> > SCMI support needed a small modification to support PINCTRL): I'll reply
> > to this mail thread with such core SCMI modification patch, so you can
> > include this patch of mine in your next V3 and use it in your series.
> >
> > Moreover, given that I wanted to test such rework of mine and a bunch
> > of other cleanups I did (as detailed down below), and it seemed silly
> > to throw all away just to then having to detail all to you, I'll also
> > include in another distinct reply the raw diff of what I changed in
> > your series to use the new extended_name support and a few other cleanups,
> > so that, if you want, you can just quickly merge that into your V3 patch
> > (of course if you like it and tests fine also for you...)...these are
> > small changes, if you take it, no need to bother with authorship and
> > attribution from my point of view.
> >
>
> Hi Cristian,
>
Hi,
> Thank you for the patches. I've applied them and tested with powerpc,
> mx68 and clang environments (as test-robot complained about).
>
Yes, sure, they were just tentative fixes, needed cleanup.
I forgot to add the RFC tag on my proposed fixes to avoid triggering the bots.
> > > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > > ---
> > > MAINTAINERS | 6 +
>
> [snip]
>
> > > SYSTEM RESET/SHUTDOWN DRIVERS
> > > M: Sebastian Reichel <[email protected]>
> > > L: [email protected]
> > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > > index b31d78fa66cc..071ac65f22b9 100644
> > > --- a/drivers/firmware/arm_scmi/Makefile
> > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> > > scmi-core-objs := $(scmi-bus-y)
> > >
> > > scmi-driver-y = driver.o notify.o
> > > +
> >
> > Do not add spurios lines.
> >
>
> Thanks, removed
>
> > > scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > > @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> > >
>
> I've applied patches you provided and made a small fixes. I'm going to
> make patch:
> "firmware: arm_scmi: Add optional flags to extended names helper"
> as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
> don't mind.
Sure, that's what I meant: include my general extended fixes at the
start of your series and just squash the misc_fixes (additionally fixed
by you :D) in your series.
A small nitpick I noticed later in scmi_protocol_ops Dox comment
+ * struct scmi_pinctrl_protocol_ops - represents the various operations provided
should be
* struct scmi_pinctrl_proto_ops
Thanks,
Cristian
On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote:
> Hello Andy,
>
> On 05.05.23 23:35, [email protected] wrote:
> > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
> >> scmi-pinctrl driver implements pinctrl driver interface and using
> >> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> >> SCP firmware, which does the changes in HW.
> >>
> >> This setup expects SCP firmware (or similar system, such as ATF)
> >> to be installed on the platform, which implements pinctrl driver
> >> for the specific platform.
> >>
> >> SCMI-Pinctrl driver should be configured from the device-tree and uses
> >> generic device-tree mappings for the configuration.
> >
> > ...
> >
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >
> >> +#include <linux/of.h>
> >
> > I do not see any user of this header. Do you?
> >
> Yes, thanks. Removing
>
> >> +#include <linux/module.h>
> >> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> >> +#include <linux/slab.h>
> >
> > Please, move these two to the upper group of the generic headers.
> >
> Thanks, fixed.
>
> >> +struct scmi_pinctrl_funcs {
> >> + unsigned int num_groups;
> >> + const char **groups;
> >> +};
> >
> > Please, use struct pinfunction.
> >
> I can't use pincfunction here because it has the following groups
> definition:
> const char * const *groups;
>
> Which is meant to be constantly allocated.
> So I when I try to gather list of groups in
> pinctrl_scmi_get_function_groups I will receive compilation error.
>
Maybe this is a further signal that we should re-evaluate the benefits of
the lazy allocations you now perform during protocol initialization
instead of querying and allocating statically all the info structs about
existing resources.
Not saying that is necessarily bad, I understood your points about reducing
the number of SCMI queries during boot and let pinctrl subsystem trigger only
the strictly needed one, just saying maybe good to reason a bit more about this
once V3 is posted. (i.e. I could bother you more :P ..)
Thanks,
Cristian
P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere
to include proper maintainers (and their bots)
Hello Cristian,
On Fri, May 12, 2023 at 10:04:41AM +0100, Cristian Marussi wrote:
> On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote:
> > Hello Andy,
> >
> > On 05.05.23 23:35, [email protected] wrote:
> > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
> > >> scmi-pinctrl driver implements pinctrl driver interface and using
> > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> > >> SCP firmware, which does the changes in HW.
> > >>
> > >> This setup expects SCP firmware (or similar system, such as ATF)
> > >> to be installed on the platform, which implements pinctrl driver
> > >> for the specific platform.
> > >>
> > >> SCMI-Pinctrl driver should be configured from the device-tree and uses
> > >> generic device-tree mappings for the configuration.
> > >
> > > ...
> > >
> > >> +#include <linux/device.h>
> > >> +#include <linux/err.h>
> > >
> > >> +#include <linux/of.h>
> > >
> > > I do not see any user of this header. Do you?
> > >
> > Yes, thanks. Removing
> >
> > >> +#include <linux/module.h>
> > >> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> > >> +#include <linux/slab.h>
> > >
> > > Please, move these two to the upper group of the generic headers.
> > >
> > Thanks, fixed.
> >
> > >> +struct scmi_pinctrl_funcs {
> > >> + unsigned int num_groups;
> > >> + const char **groups;
> > >> +};
> > >
> > > Please, use struct pinfunction.
> > >
> > I can't use pincfunction here because it has the following groups
> > definition:
> > const char * const *groups;
> >
> > Which is meant to be constantly allocated.
> > So I when I try to gather list of groups in
> > pinctrl_scmi_get_function_groups I will receive compilation error.
> >
>
> Maybe this is a further signal that we should re-evaluate the benefits of
> the lazy allocations you now perform during protocol initialization
> instead of querying and allocating statically all the info structs about
> existing resources.
>
> Not saying that is necessarily bad, I understood your points about reducing
> the number of SCMI queries during boot and let pinctrl subsystem trigger only
> the strictly needed one, just saying maybe good to reason a bit more about this
> once V3 is posted. (i.e. I could bother you more :P ..)
>
> Thanks,
> Cristian
>
> P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere
> to include proper maintainers (and their bots)
That's a good point to think about. Actually, functions are the only
thing that should be cached on pinctrl side. And we need it specifically
because groups in each function are presented by names, not selectors.
Maybe It's better to move this caching to pinctrl scmi driver. But, from
the other side - storing group names for each function is Linux Kernel
specific implementation and we probably don't want to add some specific
case to the Generic protocol driver.
I think I would leave it as in V3 so we can continue discussion.
Oleksii.
On 5/12/23 14:31, Oleksii Moisieiev wrote:
> On Fri, May 12, 2023 at 09:55:53AM +0100, Cristian Marussi wrote:
>> On Fri, May 12, 2023 at 08:38:06AM +0000, Oleksii Moisieiev wrote:
>>> On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
>>>> On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
>>>>> scmi: Introduce pinctrl SCMI protocol driver
>>>>>
>>>>> Add basic implementation of the SCMI v3.2 pincontrol protocol
>>>>> excluding GPIO support. All pinctrl related callbacks and operations
>>>>> are exposed in the include/linux/scmi_protocol.h
>>>>>
>>>>
>>>> Hi Oleksii,
>>>>
>>>> Thanks for this.
>>>>
>>>> I tried out this in an emulated setup and found just a minor issue from
>>>> the spec/functional point of view...then I reworked the extended names
>>>> support using a modified hops->extended_name_get helper (as said the core
>>>> SCMI support needed a small modification to support PINCTRL): I'll reply
>>>> to this mail thread with such core SCMI modification patch, so you can
>>>> include this patch of mine in your next V3 and use it in your series.
>>>>
>>>> Moreover, given that I wanted to test such rework of mine and a bunch
>>>> of other cleanups I did (as detailed down below), and it seemed silly
>>>> to throw all away just to then having to detail all to you, I'll also
>>>> include in another distinct reply the raw diff of what I changed in
>>>> your series to use the new extended_name support and a few other cleanups,
>>>> so that, if you want, you can just quickly merge that into your V3 patch
>>>> (of course if you like it and tests fine also for you...)...these are
>>>> small changes, if you take it, no need to bother with authorship and
>>>> attribution from my point of view.
>>>>
>>>
>>> Hi Cristian,
>>>
>>
>> Hi,
>>
>>> Thank you for the patches. I've applied them and tested with powerpc,
>>> mx68 and clang environments (as test-robot complained about).
>>>
>>
>> Yes, sure, they were just tentative fixes, needed cleanup.
>> I forgot to add the RFC tag on my proposed fixes to avoid triggering the bots.
>>
>>>>> Signed-off-by: Oleksii Moisieiev <[email protected]>
>>>>> ---
>>>>> MAINTAINERS | 6 +
>>>
>>> [snip]
>>>
>>>>> SYSTEM RESET/SHUTDOWN DRIVERS
>>>>> M: Sebastian Reichel <[email protected]>
>>>>> L: [email protected]
>>>>> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
>>>>> index b31d78fa66cc..071ac65f22b9 100644
>>>>> --- a/drivers/firmware/arm_scmi/Makefile
>>>>> +++ b/drivers/firmware/arm_scmi/Makefile
>>>>> @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
>>>>> scmi-core-objs := $(scmi-bus-y)
>>>>>
>>>>> scmi-driver-y = driver.o notify.o
>>>>> +
>>>>
>>>> Do not add spurios lines.
>>>>
>>>
>>> Thanks, removed
>>>
>>>>> scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
>>>>> scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
>>>>> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
>>>>> @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
>>>>> scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
>>>>> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
>>>>> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
>>>>> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
>>>>> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
>>>>> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>>>>>
>>>
>>> I've applied patches you provided and made a small fixes. I'm going to
>>> make patch:
>>> "firmware: arm_scmi: Add optional flags to extended names helper"
>>> as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
>>> don't mind.
>>
>> Sure, that's what I meant: include my general extended fixes at the
>> start of your series and just squash the misc_fixes (additionally fixed
>> by you :D) in your series.
>>
>> A small nitpick I noticed later in scmi_protocol_ops Dox comment
>>
>> + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
>>
>> should be
>> * struct scmi_pinctrl_proto_ops
>>
>> Thanks,
>> Cristian
>>
>
> Hi Cristian,
>
> Thank you very much for your help and your patches. I'm in the finishing
> line with V3. I'll send to you the unit tests (when I fix them to work with the
> latest changes) I'm using to test my changes and driver for ATF.
> Hope it will help you to test your environment.
Is that ATF driver available somewhere?
Thanks,
Michal
On Fri, May 12, 2023 at 09:55:53AM +0100, Cristian Marussi wrote:
> On Fri, May 12, 2023 at 08:38:06AM +0000, Oleksii Moisieiev wrote:
> > On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> > > On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > > > scmi: Introduce pinctrl SCMI protocol driver
> > > >
> > > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > > excluding GPIO support. All pinctrl related callbacks and operations
> > > > are exposed in the include/linux/scmi_protocol.h
> > > >
> > >
> > > Hi Oleksii,
> > >
> > > Thanks for this.
> > >
> > > I tried out this in an emulated setup and found just a minor issue from
> > > the spec/functional point of view...then I reworked the extended names
> > > support using a modified hops->extended_name_get helper (as said the core
> > > SCMI support needed a small modification to support PINCTRL): I'll reply
> > > to this mail thread with such core SCMI modification patch, so you can
> > > include this patch of mine in your next V3 and use it in your series.
> > >
> > > Moreover, given that I wanted to test such rework of mine and a bunch
> > > of other cleanups I did (as detailed down below), and it seemed silly
> > > to throw all away just to then having to detail all to you, I'll also
> > > include in another distinct reply the raw diff of what I changed in
> > > your series to use the new extended_name support and a few other cleanups,
> > > so that, if you want, you can just quickly merge that into your V3 patch
> > > (of course if you like it and tests fine also for you...)...these are
> > > small changes, if you take it, no need to bother with authorship and
> > > attribution from my point of view.
> > >
> >
> > Hi Cristian,
> >
>
> Hi,
>
> > Thank you for the patches. I've applied them and tested with powerpc,
> > mx68 and clang environments (as test-robot complained about).
> >
>
> Yes, sure, they were just tentative fixes, needed cleanup.
> I forgot to add the RFC tag on my proposed fixes to avoid triggering the bots.
>
> > > > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > > > ---
> > > > MAINTAINERS | 6 +
> >
> > [snip]
> >
> > > > SYSTEM RESET/SHUTDOWN DRIVERS
> > > > M: Sebastian Reichel <[email protected]>
> > > > L: [email protected]
> > > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > > > index b31d78fa66cc..071ac65f22b9 100644
> > > > --- a/drivers/firmware/arm_scmi/Makefile
> > > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > > @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> > > > scmi-core-objs := $(scmi-bus-y)
> > > >
> > > > scmi-driver-y = driver.o notify.o
> > > > +
> > >
> > > Do not add spurios lines.
> > >
> >
> > Thanks, removed
> >
> > > > scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > > > @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> > > >
> >
> > I've applied patches you provided and made a small fixes. I'm going to
> > make patch:
> > "firmware: arm_scmi: Add optional flags to extended names helper"
> > as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
> > don't mind.
>
> Sure, that's what I meant: include my general extended fixes at the
> start of your series and just squash the misc_fixes (additionally fixed
> by you :D) in your series.
>
> A small nitpick I noticed later in scmi_protocol_ops Dox comment
>
> + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
>
> should be
> * struct scmi_pinctrl_proto_ops
>
> Thanks,
> Cristian
>
Hi Cristian,
Thank you very much for your help and your patches. I'm in the finishing
line with V3. I'll send to you the unit tests (when I fix them to work with the
latest changes) I'm using to test my changes and driver for ATF.
Hope it will help you to test your environment.
Best regards,
Oleksii.
On Fri, May 12, 2023 at 12:18:03PM +0000, Oleksii Moisieiev wrote:
> Hello Cristian,
>
> On Fri, May 12, 2023 at 10:04:41AM +0100, Cristian Marussi wrote:
> > On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote:
> > > Hello Andy,
> > >
> > > On 05.05.23 23:35, [email protected] wrote:
> > > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
> > > >> scmi-pinctrl driver implements pinctrl driver interface and using
> > > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> > > >> SCP firmware, which does the changes in HW.
> > > >>
> > > >> This setup expects SCP firmware (or similar system, such as ATF)
> > > >> to be installed on the platform, which implements pinctrl driver
> > > >> for the specific platform.
> > > >>
> > > >> SCMI-Pinctrl driver should be configured from the device-tree and uses
> > > >> generic device-tree mappings for the configuration.
> > > >
> > > > ...
> > > >
> > > >> +#include <linux/device.h>
> > > >> +#include <linux/err.h>
> > > >
> > > >> +#include <linux/of.h>
> > > >
> > > > I do not see any user of this header. Do you?
> > > >
> > > Yes, thanks. Removing
> > >
> > > >> +#include <linux/module.h>
> > > >> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> > > >> +#include <linux/slab.h>
> > > >
> > > > Please, move these two to the upper group of the generic headers.
> > > >
> > > Thanks, fixed.
> > >
> > > >> +struct scmi_pinctrl_funcs {
> > > >> + unsigned int num_groups;
> > > >> + const char **groups;
> > > >> +};
> > > >
> > > > Please, use struct pinfunction.
> > > >
> > > I can't use pincfunction here because it has the following groups
> > > definition:
> > > const char * const *groups;
> > >
> > > Which is meant to be constantly allocated.
> > > So I when I try to gather list of groups in
> > > pinctrl_scmi_get_function_groups I will receive compilation error.
> > >
> >
> > Maybe this is a further signal that we should re-evaluate the benefits of
> > the lazy allocations you now perform during protocol initialization
> > instead of querying and allocating statically all the info structs about
> > existing resources.
> >
> > Not saying that is necessarily bad, I understood your points about reducing
> > the number of SCMI queries during boot and let pinctrl subsystem trigger only
> > the strictly needed one, just saying maybe good to reason a bit more about this
> > once V3 is posted. (i.e. I could bother you more :P ..)
> >
> > Thanks,
> > Cristian
> >
> > P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere
> > to include proper maintainers (and their bots)
>
> That's a good point to think about. Actually, functions are the only
> thing that should be cached on pinctrl side. And we need it specifically
> because groups in each function are presented by names, not selectors.
> Maybe It's better to move this caching to pinctrl scmi driver. But, from
> the other side - storing group names for each function is Linux Kernel
> specific implementation and we probably don't want to add some specific
> case to the Generic protocol driver.
>
> I think I would leave it as in V3 so we can continue discussion.
>
Sure, let's review/rediscuss this on top V3.
Thanks,
Cristian
On Fri, May 12, 2023 at 02:32:25PM +0200, Michal Simek wrote:
>
>
> On 5/12/23 14:31, Oleksii Moisieiev wrote:
> > On Fri, May 12, 2023 at 09:55:53AM +0100, Cristian Marussi wrote:
> > > On Fri, May 12, 2023 at 08:38:06AM +0000, Oleksii Moisieiev wrote:
> > > > On Fri, May 05, 2023 at 08:52:03PM +0100, Cristian Marussi wrote:
> > > > > On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote:
> > > > > > scmi: Introduce pinctrl SCMI protocol driver
> > > > > >
> > > > > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > > > > excluding GPIO support. All pinctrl related callbacks and operations
> > > > > > are exposed in the include/linux/scmi_protocol.h
> > > > > >
> > > > >
> > > > > Hi Oleksii,
> > > > >
> > > > > Thanks for this.
> > > > >
> > > > > I tried out this in an emulated setup and found just a minor issue from
> > > > > the spec/functional point of view...then I reworked the extended names
> > > > > support using a modified hops->extended_name_get helper (as said the core
> > > > > SCMI support needed a small modification to support PINCTRL): I'll reply
> > > > > to this mail thread with such core SCMI modification patch, so you can
> > > > > include this patch of mine in your next V3 and use it in your series.
> > > > >
> > > > > Moreover, given that I wanted to test such rework of mine and a bunch
> > > > > of other cleanups I did (as detailed down below), and it seemed silly
> > > > > to throw all away just to then having to detail all to you, I'll also
> > > > > include in another distinct reply the raw diff of what I changed in
> > > > > your series to use the new extended_name support and a few other cleanups,
> > > > > so that, if you want, you can just quickly merge that into your V3 patch
> > > > > (of course if you like it and tests fine also for you...)...these are
> > > > > small changes, if you take it, no need to bother with authorship and
> > > > > attribution from my point of view.
> > > > >
> > > >
> > > > Hi Cristian,
> > > >
> > >
> > > Hi,
> > >
> > > > Thank you for the patches. I've applied them and tested with powerpc,
> > > > mx68 and clang environments (as test-robot complained about).
> > > >
> > >
> > > Yes, sure, they were just tentative fixes, needed cleanup.
> > > I forgot to add the RFC tag on my proposed fixes to avoid triggering the bots.
> > >
> > > > > > Signed-off-by: Oleksii Moisieiev <[email protected]>
> > > > > > ---
> > > > > > MAINTAINERS | 6 +
> > > >
> > > > [snip]
> > > >
> > > > > > SYSTEM RESET/SHUTDOWN DRIVERS
> > > > > > M: Sebastian Reichel <[email protected]>
> > > > > > L: [email protected]
> > > > > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > > > > > index b31d78fa66cc..071ac65f22b9 100644
> > > > > > --- a/drivers/firmware/arm_scmi/Makefile
> > > > > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > > > > @@ -3,6 +3,7 @@ scmi-bus-y = bus.o
> > > > > > scmi-core-objs := $(scmi-bus-y)
> > > > > > scmi-driver-y = driver.o notify.o
> > > > > > +
> > > > >
> > > > > Do not add spurios lines.
> > > > >
> > > >
> > > > Thanks, removed
> > > >
> > > > > > scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > > > > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > > > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > > > > > @@ -10,7 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > > > > > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > > > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > > > > > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > > > > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > > > > > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > > > > > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> > > >
> > > > I've applied patches you provided and made a small fixes. I'm going to
> > > > make patch:
> > > > "firmware: arm_scmi: Add optional flags to extended names helper"
> > > > as a separate and squach "Misc Fixes and refactor" to my changes in V3 if you
> > > > don't mind.
> > >
> > > Sure, that's what I meant: include my general extended fixes at the
> > > start of your series and just squash the misc_fixes (additionally fixed
> > > by you :D) in your series.
> > >
> > > A small nitpick I noticed later in scmi_protocol_ops Dox comment
> > >
> > > + * struct scmi_pinctrl_protocol_ops - represents the various operations provided
> > >
> > > should be
> > > * struct scmi_pinctrl_proto_ops
> > >
> > > Thanks,
> > > Cristian
> > >
> >
> > Hi Cristian,
> >
> > Thank you very much for your help and your patches. I'm in the finishing
> > line with V3. I'll send to you the unit tests (when I fix them to work with the
> > latest changes) I'm using to test my changes and driver for ATF.
> > Hope it will help you to test your environment.
>
> Is that ATF driver available somewhere?
>
> Thanks,
> Michal
>
>
Hi Michal,
I've posted a link in the v3 series. But be aware it's not ready for
upstream so may be a bit ugly.
Oleksii.
On Tue, 14 Nov 2023 14:54:49 +0000, Cristian Marussi wrote:
> Some recently added SCMI protocols needs an additional flags parameter to
> be able to properly configure the command used to query the extended name
> of a resource.
>
> Modify extended_name_get helper accordingly.
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!
[1/1] firmware: arm_scmi: Add optional flags to extended names helper
https://git.kernel.org/sudeep.holla/c/e4e6e8f1ad0f
--
Regards,
Sudeep