| Message ID | cover.1691518313.git.oleksii_moisieiev@epam.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support | expand |
On Tue, Aug 8, 2023 at 8:25 PM Oleksii Moisieiev <Oleksii_Moisieiev@epam.com> wrote: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCMI platform firmware, which does the changes in HW. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > Changes v3 -> v4 I'm certainly happy with this version. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote: > From: Cristian Marussi <cristian.marussi@arm.com> > > 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. > Hi Oleksii, > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- You need to apply also your Signed-off as last in V5 for this to be accepted AND beside this I spotted a bug while re-testing this. (my bad) See down below a possible fix. > drivers/firmware/arm_scmi/clock.c | 2 +- > drivers/firmware/arm_scmi/driver.c | 7 +++++-- > 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, 16 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..729201d8f935 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,8 @@ 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, t->tx.buf + sizeof(res_id)); > resp = t->rx.buf; > > ret = ph->xops->do_xfer(ph, t); Here my patch is buggy since when you provide the optional flags they are after the res_id parameter BUT the previous xfer_get_init still requires an xfer with a tx_len size of sizeof(res_id) ONLY, so while the flags will fit into tx.buf that second optional flags field won't be sent because the core SCMI stack will see t->tx.len == 4 :< (so this flags extension wont work when needed in Pinctrl) A possible fix that I tested consist in changing this snippet with: -->8--- diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index b5957cc12fee..06c101edba7f 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,19 +1446,22 @@ 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; + size_t txlen; struct scmi_xfer *t; struct scmi_msg_resp_domain_name_get *resp; - ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id), - sizeof(*resp), &t); + txlen = !flags ? sizeof(res_id) : sizeof(res_id) + sizeof(*flags); + ret = ph->xops->xfer_get_init(ph, cmd_id, txlen, sizeof(*resp), &t); if (ret) goto out; put_unaligned_le32(res_id, t->tx.buf); + if (flags) + put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id)); resp = t->rx.buf; ret = ph->xops->do_xfer(ph, t); ---8<--- My bad, Thanks, Cristian
On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote: > Add basic implementation of the SCMI v3.2 pincontrol protocol. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > Changes v3 -> v4 > - Fixed MAINTAINERS file description > - adjusted pinctrl ops position and callback names > - add trailing coma in scmi_protocol list > - removed unneeded pi checks > - corrected selector check > - resource allocation refactoring > - scmi_*_info swap params to generate better code > - style, add trailing coma in definitions > --- Hi Oleksii, thanks to have addressed all the previous reported issues. At this point my only residual blocker issue is that, after the recent changes to PINCTRL_CONFIG_GET/SET command formats [1], as requested by Peng (multiple config values per message), the support added by this patch around CONFIG_GET/SET is not out-of-spec, because the changes from the previous BETA were not backward compatible. So, even sending just one config/val request, as it is now, won't work because an out-of-spec message will be sent. I am NOT aware of any further change to this last BETA but you may want to check with Souvik. [1]: https://developer.arm.com/documentation/den0056/latest/ Some more comments on this down below. > MAINTAINERS | 6 + > drivers/firmware/arm_scmi/Makefile | 2 +- > drivers/firmware/arm_scmi/driver.c | 2 + > drivers/firmware/arm_scmi/pinctrl.c | 791 ++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/protocols.h | 1 + > include/linux/scmi_protocol.h | 42 ++ > 6 files changed, 843 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/arm_scmi/pinctrl.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0dab9737ec16..2d81d00e5f4f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20522,6 +20522,12 @@ F: include/linux/sc[mp]i_protocol.h > F: include/trace/events/scmi.h > F: include/uapi/linux/virtio_scmi.h > > +PINCTRL DRIVER FOR SYSTEM CONTROL MANAGEMENT INTERFACE (SCMI) > +M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > +L: linux-arm-kernel@lists.infradead.org > +S: Maintained > +F: drivers/firmware/arm_scmi/pinctrl.c > + > SYSTEM RESET/SHUTDOWN DRIVERS > M: Sebastian Reichel <sre@kernel.org> > L: linux-pm@vger.kernel.org > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index b31d78fa66cc..603430ec0bfe 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y) > > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 729201d8f935..03686bff000e 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -3024,6 +3024,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); > } > @@ -3041,6 +3042,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..868a2f9821be > --- /dev/null > +++ b/drivers/firmware/arm_scmi/pinctrl.c > @@ -0,0 +1,791 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Management Interface (SCMI) Pinctrl Protocol > + * > + * Copyright (C) 2023 EPAM > + */ > + > +#include <linux/module.h> > +#include <linux/scmi_protocol.h> > +#include <linux/slab.h> > + > +#include "protocols.h" > + > +#define REG_TYPE_BITS GENMASK(9, 8) > +#define REG_CONFIG GENMASK(7, 0) > + > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16)) > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > + > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31)) > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0)) > + > +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16)) > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0)) > + > +enum scmi_pinctrl_protocol_cmd { > + PINCTRL_ATTRIBUTES = 0x3, > + PINCTRL_LIST_ASSOCIATIONS = 0x4, > + PINCTRL_CONFIG_GET = 0x5, > + PINCTRL_CONFIG_SET = 0x6, > + PINCTRL_FUNCTION_SELECT = 0x7, > + PINCTRL_REQUEST = 0x8, > + PINCTRL_RELEASE = 0x9, > + PINCTRL_NAME_GET = 0xa, > + PINCTRL_SET_PERMISSIONS = 0xb > +}; > + > +struct scmi_msg_conf_set { > + __le32 identifier; > + __le32 attributes; > + __le32 config_value; > +}; > + > +struct scmi_msg_conf_get { > + __le32 identifier; > + __le32 attributes; > +}; > + > +struct scmi_msg_pinctrl_protocol_attributes { > + __le32 attributes_low; > + __le32 attributes_high; > +}; > + > +struct scmi_msg_pinctrl_attributes { > + __le32 identifier; > + __le32 flags; > +}; > + > +struct scmi_resp_pinctrl_attributes { > + __le32 attributes; > + u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > +}; > + > +struct scmi_msg_pinctrl_list_assoc { > + __le32 identifier; > + __le32 flags; > + __le32 index; > +}; > + > +struct scmi_resp_pinctrl_list_assoc { > + __le32 flags; > + __le16 array[]; > +}; > + > +struct scmi_msg_func_set { > + __le32 identifier; > + __le32 function_id; > + __le32 flags; > +}; > + > +struct scmi_msg_request { > + __le32 identifier; > + __le32 flags; > +}; > + > +struct scmi_group_info { > + char name[SCMI_MAX_STR_SIZE]; > + bool present; > + unsigned int *group_pins; > + unsigned int nr_pins; > +}; > + > +struct scmi_function_info { > + char name[SCMI_MAX_STR_SIZE]; > + bool present; > + unsigned int *groups; > + unsigned int nr_groups; > +}; > + > +struct scmi_pin_info { > + char name[SCMI_MAX_STR_SIZE]; > + bool present; > +}; > + > +struct scmi_pinctrl_info { > + u32 version; > + int nr_groups; > + int nr_functions; > + int nr_pins; > + struct scmi_group_info *groups; > + struct scmi_function_info *functions; > + struct scmi_pin_info *pins; > +}; > + > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph, > + struct scmi_pinctrl_info *pi) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_pinctrl_protocol_attributes *attr; > + > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high); > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low); > + pi->nr_pins = GET_PINS_NR(attr->attributes_low); > + } > + > + ph->xops->xfer_put(ph, t); > + return ret; > +} > + > +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph, > + enum scmi_pinctrl_selector_type type) > +{ > + struct scmi_pinctrl_info *pi = ph->get_priv(ph); > + > + switch (type) { > + case PIN_TYPE: > + return pi->nr_pins; > + case GROUP_TYPE: > + return pi->nr_groups; > + case FUNCTION_TYPE: > + return pi->nr_functions; > + default: > + return -EINVAL; > + } > +} > + > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph, > + u32 identifier, > + enum scmi_pinctrl_selector_type type) > +{ > + int value; > + > + value = scmi_pinctrl_count_get(ph, type); > + if (value < 0) > + return value; > + > + if (identifier >= value) > + return -EINVAL; > + > + return 0; > +} > + > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, > + enum scmi_pinctrl_selector_type type, > + u32 selector, char *name, > + unsigned int *n_elems) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_pinctrl_attributes *tx; > + struct scmi_resp_pinctrl_attributes *rx; > + > + if (!name) > + return -EINVAL; > + > + ret = scmi_pinctrl_validate_id(ph, selector, type); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx), sizeof(*rx), &t); > + if (ret) > + return ret; > + > + tx = t->tx.buf; > + rx = t->rx.buf; > + tx->identifier = cpu_to_le32(selector); > + tx->flags = cpu_to_le32(type); > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + if (n_elems) > + *n_elems = NUM_ELEMS(rx->attributes); > + > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); > + } > + > + ph->xops->xfer_put(ph, t); > + > + /* > + * If supported overwrite short name with the extended one; > + * on error just carry on and use already provided short name. > + */ > + if (!ret && EXT_NAME_FLAG(rx->attributes)) > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector, > + (u32 *)&type, name, > + SCMI_MAX_STR_SIZE); > + return ret; > +} > + > +struct scmi_pinctrl_ipriv { > + u32 selector; > + enum scmi_pinctrl_selector_type type; > + unsigned int *array; > +}; > + > +static void iter_pinctrl_assoc_prepare_message(void *message, > + unsigned int desc_index, > + const void *priv) > +{ > + struct scmi_msg_pinctrl_list_assoc *msg = message; > + const struct scmi_pinctrl_ipriv *p = priv; > + > + msg->identifier = cpu_to_le32(p->selector); > + msg->flags = cpu_to_le32(p->type); > + /* Set the number of OPPs to be skipped/already read */ > + msg->index = cpu_to_le32(desc_index); > +} > + > +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st, > + const void *response, void *priv) > +{ > + const struct scmi_resp_pinctrl_list_assoc *r = response; > + > + st->num_returned = RETURNED(r->flags); > + st->num_remaining = REMAINING(r->flags); > + > + return 0; > +} > + > +static int > +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle *ph, > + const void *response, > + struct scmi_iterator_state *st, void *priv) > +{ > + const struct scmi_resp_pinctrl_list_assoc *r = response; > + struct scmi_pinctrl_ipriv *p = priv; > + > + p->array[st->desc_index + st->loop_idx] = > + le16_to_cpu(r->array[st->loop_idx]); > + > + return 0; > +} > + > +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle *ph, > + u32 selector, > + enum scmi_pinctrl_selector_type type, > + u16 size, unsigned int *array) > +{ > + int ret; > + void *iter; > + struct scmi_iterator_ops ops = { > + .prepare_message = iter_pinctrl_assoc_prepare_message, > + .update_state = iter_pinctrl_assoc_update_state, > + .process_response = iter_pinctrl_assoc_process_response, > + }; > + struct scmi_pinctrl_ipriv ipriv = { > + .selector = selector, > + .type = type, > + .array = array, > + }; > + > + if (!array || !size || type == PIN_TYPE) > + return -EINVAL; > + > + ret = scmi_pinctrl_validate_id(ph, selector, type); > + if (ret) > + return ret; > + > + iter = ph->hops->iter_response_init(ph, &ops, size, > + PINCTRL_LIST_ASSOCIATIONS, > + sizeof(struct scmi_msg_pinctrl_list_assoc), > + &ipriv); > + > + if (IS_ERR(iter)) > + return PTR_ERR(iter); > + > + return ph->hops->iter_response_run(iter); > +} > + > +static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph, > + u32 selector, > + enum scmi_pinctrl_selector_type type, > + u8 config_type, unsigned long *config_value) > +{ > + int ret; > + u32 attributes; > + struct scmi_xfer *t; > + struct scmi_msg_conf_get *tx; > + When supporting new multiple config/values calls here you should be able to use the iterators helpers for the multipart reply. (beside reworking all the message field-bits ...) > + if (!config_value || type == FUNCTION_TYPE) > + return -EINVAL; > + > + ret = scmi_pinctrl_validate_id(ph, selector, type); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx), sizeof(__le32), &t); > + if (ret) > + return ret; > + > + tx = t->tx.buf; > + tx->identifier = cpu_to_le32(selector); > + attributes = FIELD_PREP(REG_TYPE_BITS, type) | > + FIELD_PREP(REG_CONFIG, config_type); > + tx->attributes = cpu_to_le32(attributes); > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) > + *config_value = get_unaligned_le32(t->rx.buf); > + > + ph->xops->xfer_put(ph, t); > + return ret; > +} > + > +static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph, > + u32 selector, > + enum scmi_pinctrl_selector_type type, > + u8 config_type, unsigned long config_value) > +{ > + struct scmi_xfer *t; > + struct scmi_msg_conf_set *tx; > + u32 attributes = 0; > + int ret; > + Here instead when suppoting setting multiple config/values at once you'll have anyway to split your request by hand into multiple messages based on the underlying transport size. Not sure if it is worth to add some support helper for multi-part request, probably NO, given that this protocol would be the only user of such multi-part request at the moment and this would add just more complexity for just one user. Anyway, per-transport max_msg_size is NOT exposed explicitly to the protocols as of now, but, off-the-top-of-my-head I think you should be able to obtain the current desc->max_msg_size reading t->rx.len after the xfer_get_init() since that rx.len is initialized to max_msg_size when you call xfer_get_init() with 0 as rx_size as you do. Thanks, Cristian
On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCMI platform firmware, which does the changes in HW. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > Changes v3 -> v4 > - ordered config option alphabetically > - ordered object file alphabetically > - rephrased PINCTRL_SCMI config description > - formatting fixes, removed blank lines after get_drvdata call > - code style adjustments > - add set_drvdata call > - removed goto label > - refactoring of the devm resource management > - removed pctldev != NULL check > - fix parameter name in pinconf-group-get > - probe function refactoring > - removed unneeded pmx checks > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-scmi.c | 442 +++++++++++++++++++++++++++++++++ > 4 files changed, 455 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-scmi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2d81d00e5f4f..c4e36f955e53 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20527,6 +20527,7 @@ M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > L: linux-arm-kernel@lists.infradead.org > S: Maintained > F: drivers/firmware/arm_scmi/pinctrl.c > +F: drivers/pinctrl/pinctrl-scmi.c > > SYSTEM RESET/SHUTDOWN DRIVERS > M: Sebastian Reichel <sre@kernel.org> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 5787c579dcf6..956cfe76fbc6 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -428,6 +428,17 @@ config PINCTRL_ROCKCHIP > help > This support pinctrl and GPIO driver for Rockchip SoCs. > > +config PINCTRL_SCMI > + tristate "Pinctrl driver using SCMI protocol interface" > + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST > + select PINMUX > + select GENERIC_PINCONF > + help > + This driver provides support for pinctrl which is controlled > + by firmware that implements the SCMI interface. > + It uses SCMI Message Protocol to interact with the > + firmware providing all the pinctrl controls. > + > config PINCTRL_SINGLE > tristate "One-register-per-pin type device tree based pinctrl driver" > depends on OF > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index e196c6e324ad..25d67bac9ee0 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > new file mode 100644 > index 000000000000..a9304402ddf1 > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -0,0 +1,442 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Power Interface (SCMI) Protocol based pinctrl driver > + * > + * Copyright (C) 2023 EPAM > + */ > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/seq_file.h> > +#include <linux/scmi_protocol.h> > +#include <linux/slab.h> > + > +#include <linux/pinctrl/machine.h> > +#include <linux/pinctrl/pinconf.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "pinctrl-utils.h" > +#include "core.h" > +#include "pinconf.h" > + > +#define DRV_NAME "scmi-pinctrl" > + > +static const struct scmi_pinctrl_proto_ops *pinctrl_ops; > + > +struct scmi_pinctrl_funcs { > + unsigned int num_groups; > + const char **groups; > +}; > + > +struct scmi_pinctrl { > + struct device *dev; > + struct scmi_protocol_handle *ph; > + struct pinctrl_dev *pctldev; > + struct pinctrl_desc pctl_desc; > + struct scmi_pinctrl_funcs *functions; > + unsigned int nr_functions; > + char **groups; > + unsigned int nr_groups; > + struct pinctrl_pin_desc *pins; > + unsigned int nr_pins; > +}; > + > +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE); > +} > + > +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins); > +} > + > +#ifdef CONFIG_OF > +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np_config, > + struct pinctrl_map **map, > + u32 *num_maps) > +{ > + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps, > + PIN_MAP_TYPE_INVALID); > +} > + > +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map, > + u32 num_maps) > +{ > + kfree(map); > +} > + > +#endif /* CONFIG_OF */ > + > +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { > + .get_groups_count = pinctrl_scmi_get_groups_count, > + .get_group_name = pinctrl_scmi_get_group_name, > + .get_group_pins = pinctrl_scmi_get_group_pins, > +#ifdef CONFIG_OF > + .dt_node_to_map = pinctrl_scmi_dt_node_to_map, > + .dt_free_map = pinctrl_scmi_dt_free_map, > +#endif > +}; > + > +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE); > +} > + > +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const char * const **groups, > + unsigned int * const num_groups) > +{ > + const unsigned int *group_ids; > + int ret, i; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!groups || !num_groups) > + return -EINVAL; > + > + if (selector < pmx->nr_functions && > + pmx->functions[selector].num_groups) { > + *groups = (const char * const *)pmx->functions[selector].groups; > + *num_groups = pmx->functions[selector].num_groups; > + return 0; > + } > + > + ret = pinctrl_ops->function_groups_get(pmx->ph, selector, > + &pmx->functions[selector].num_groups, > + &group_ids); > + if (ret) { > + dev_err(pmx->dev, "Unable to get function groups, err %d", ret); > + return ret; > + } > + > + *num_groups = pmx->functions[selector].num_groups; > + if (!*num_groups) > + return -EINVAL; > + > + pmx->functions[selector].groups = > + devm_kcalloc(pmx->dev, *num_groups, sizeof(*pmx->functions[selector].groups), > + GFP_KERNEL); > + if (!pmx->functions[selector].groups) > + return -ENOMEM; > + > + for (i = 0; i < *num_groups; i++) { > + pmx->functions[selector].groups[i] = > + pinctrl_scmi_get_group_name(pmx->pctldev, > + group_ids[i]); > + if (!pmx->functions[selector].groups[i]) { > + ret = -ENOMEM; > + goto err_free; > + } > + } > + > + *groups = (const char * const *)pmx->functions[selector].groups; > + > + return 0; > + > +err_free: > + devm_kfree(pmx->dev, pmx->functions[selector].groups); > + > + return ret; > +} > + > +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->mux_set(pmx->ph, selector, group); > +} > + > +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, unsigned int offset) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->pin_request(pmx->ph, offset); > +} > + > +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->pin_free(pmx->ph, offset); > +} > + > +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = { > + .request = pinctrl_scmi_request, > + .free = pinctrl_scmi_free, > + .get_functions_count = pinctrl_scmi_get_functions_count, > + .get_function_name = pinctrl_scmi_get_function_name, > + .get_function_groups = pinctrl_scmi_get_function_groups, > + .set_mux = pinctrl_scmi_func_set_mux, > +}; > + > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, config_type, &config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + > +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *configs, > + unsigned int num_configs) > +{ > + int i, ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!configs || num_configs == 0) > + return -EINVAL; > + Here if you re-implement the Pinctrl SCMI protocol with the latest spec you could pack this into one config_set() with multiple config/values pair (and let the SCMI Pinctrl protocol to chunk the request in messages appropriate for the underlying transport size. Thanks, Cristian
On Tue, Aug 08, 2023 at 06:25:34PM +0000, Oleksii Moisieiev wrote: > This Patch series is intended to introduce the generic driver for > pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0]. > Hi Oleksii, as said in general seems good to me now, beside the out-of-spec issue I mentioned elsewhere around PINCTRL_CONFIG_GET/SET. I'll review and test further with the above fixes in V5 during the next cycle. Thanks for all the work addressing the reported issues. Cristian
Hi Oleksii, On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCMI platform firmware, which does the changes in HW. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > Changes v3 -> v4 > - ordered config option alphabetically > - ordered object file alphabetically > - rephrased PINCTRL_SCMI config description > - formatting fixes, removed blank lines after get_drvdata call > - code style adjustments > - add set_drvdata call > - removed goto label > - refactoring of the devm resource management > - removed pctldev != NULL check > - fix parameter name in pinconf-group-get > - probe function refactoring > - removed unneeded pmx checks > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-scmi.c | 442 +++++++++++++++++++++++++++++++++ > 4 files changed, 455 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-scmi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2d81d00e5f4f..c4e36f955e53 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20527,6 +20527,7 @@ M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > L: linux-arm-kernel@lists.infradead.org > S: Maintained > F: drivers/firmware/arm_scmi/pinctrl.c > +F: drivers/pinctrl/pinctrl-scmi.c > > SYSTEM RESET/SHUTDOWN DRIVERS > M: Sebastian Reichel <sre@kernel.org> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 5787c579dcf6..956cfe76fbc6 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -428,6 +428,17 @@ config PINCTRL_ROCKCHIP > help > This support pinctrl and GPIO driver for Rockchip SoCs. > > +config PINCTRL_SCMI > + tristate "Pinctrl driver using SCMI protocol interface" > + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST > + select PINMUX > + select GENERIC_PINCONF > + help > + This driver provides support for pinctrl which is controlled > + by firmware that implements the SCMI interface. > + It uses SCMI Message Protocol to interact with the > + firmware providing all the pinctrl controls. > + > config PINCTRL_SINGLE > tristate "One-register-per-pin type device tree based pinctrl driver" > depends on OF > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index e196c6e324ad..25d67bac9ee0 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > new file mode 100644 > index 000000000000..a9304402ddf1 > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -0,0 +1,442 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Power Interface (SCMI) Protocol based pinctrl driver > + * > + * Copyright (C) 2023 EPAM > + */ > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/seq_file.h> > +#include <linux/scmi_protocol.h> > +#include <linux/slab.h> > + > +#include <linux/pinctrl/machine.h> > +#include <linux/pinctrl/pinconf.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "pinctrl-utils.h" > +#include "core.h" > +#include "pinconf.h" > + > +#define DRV_NAME "scmi-pinctrl" > + > +static const struct scmi_pinctrl_proto_ops *pinctrl_ops; > + > +struct scmi_pinctrl_funcs { > + unsigned int num_groups; > + const char **groups; > +}; > + > +struct scmi_pinctrl { > + struct device *dev; > + struct scmi_protocol_handle *ph; > + struct pinctrl_dev *pctldev; > + struct pinctrl_desc pctl_desc; > + struct scmi_pinctrl_funcs *functions; > + unsigned int nr_functions; > + char **groups; > + unsigned int nr_groups; > + struct pinctrl_pin_desc *pins; > + unsigned int nr_pins; > +}; > + > +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE); > +} > + > +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins); > +} > + > +#ifdef CONFIG_OF > +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np_config, > + struct pinctrl_map **map, > + u32 *num_maps) > +{ > + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps, > + PIN_MAP_TYPE_INVALID); > +} > + > +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map, > + u32 num_maps) > +{ > + kfree(map); > +} > + > +#endif /* CONFIG_OF */ > + > +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { > + .get_groups_count = pinctrl_scmi_get_groups_count, > + .get_group_name = pinctrl_scmi_get_group_name, > + .get_group_pins = pinctrl_scmi_get_group_pins, > +#ifdef CONFIG_OF > + .dt_node_to_map = pinctrl_scmi_dt_node_to_map, > + .dt_free_map = pinctrl_scmi_dt_free_map, > +#endif > +}; > + > +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE); > +} > + > +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const char * const **groups, > + unsigned int * const num_groups) > +{ > + const unsigned int *group_ids; > + int ret, i; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!groups || !num_groups) > + return -EINVAL; > + > + if (selector < pmx->nr_functions && > + pmx->functions[selector].num_groups) { > + *groups = (const char * const *)pmx->functions[selector].groups; > + *num_groups = pmx->functions[selector].num_groups; > + return 0; > + } > + > + ret = pinctrl_ops->function_groups_get(pmx->ph, selector, > + &pmx->functions[selector].num_groups, > + &group_ids); > + if (ret) { > + dev_err(pmx->dev, "Unable to get function groups, err %d", ret); > + return ret; > + } > + > + *num_groups = pmx->functions[selector].num_groups; > + if (!*num_groups) > + return -EINVAL; > + > + pmx->functions[selector].groups = > + devm_kcalloc(pmx->dev, *num_groups, sizeof(*pmx->functions[selector].groups), > + GFP_KERNEL); > + if (!pmx->functions[selector].groups) > + return -ENOMEM; > + > + for (i = 0; i < *num_groups; i++) { > + pmx->functions[selector].groups[i] = > + pinctrl_scmi_get_group_name(pmx->pctldev, > + group_ids[i]); > + if (!pmx->functions[selector].groups[i]) { > + ret = -ENOMEM; > + goto err_free; > + } > + } > + > + *groups = (const char * const *)pmx->functions[selector].groups; > + > + return 0; > + > +err_free: > + devm_kfree(pmx->dev, pmx->functions[selector].groups); > + > + return ret; > +} > + > +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->mux_set(pmx->ph, selector, group); > +} > + > +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, unsigned int offset) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->pin_request(pmx->ph, offset); > +} > + > +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset) > +{ > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + return pinctrl_ops->pin_free(pmx->ph, offset); > +} > + > +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = { > + .request = pinctrl_scmi_request, > + .free = pinctrl_scmi_free, > + .get_functions_count = pinctrl_scmi_get_functions_count, > + .get_function_name = pinctrl_scmi_get_function_name, > + .get_function_groups = pinctrl_scmi_get_function_groups, > + .set_mux = pinctrl_scmi_func_set_mux, > +}; > + > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, config_type, &config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + > +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *configs, > + unsigned int num_configs) > +{ > + int i, ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!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]); The generic pinconf parameters defined as "enum pin_config_param" do not exactly match with a set of SCMI's configuration types defined in Table 23 in the section 4.11.2.6 "PINCTRL_CONFIG_GET". pinconf_to_config_param() simply masks the lowest 8 bits of the input value, but doesn't convert anything. How does this function work as expected? Do I miss anything? Regards, -Takahiro Akashi > + > + ret = pinctrl_ops->config_set(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 = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!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->config_set(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 group, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, config_type, &config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + > +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = { > + .is_generic = true, > + .pin_config_get = pinctrl_scmi_pinconf_get, > + .pin_config_set = pinctrl_scmi_pinconf_set, > + .pin_config_group_set = pinctrl_scmi_pinconf_group_set, > + .pin_config_group_get = pinctrl_scmi_pinconf_group_get, > + .pin_config_config_dbg_show = pinconf_generic_dump_config, > +}; > + > +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, > + unsigned int *nr_pins, > + const struct pinctrl_pin_desc **pins) > +{ > + int ret, i; > + > + if (!pins || !nr_pins) > + return -EINVAL; > + > + if (pmx->nr_pins) { > + *pins = pmx->pins; > + *nr_pins = pmx->nr_pins; > + return 0; > + } > + > + *nr_pins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE); > + > + pmx->nr_pins = *nr_pins; > + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins), GFP_KERNEL); > + if (!pmx->pins) > + return -ENOMEM; > + > + for (i = 0; i < *nr_pins; i++) { > + pmx->pins[i].number = i; > + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pmx->pins[i].name); > + if (ret) { > + dev_err(pmx->dev, "Can't get name for pin %d: rc %d", i, ret); > + pmx->nr_pins = 0; > + return ret; > + } > + } > + > + *pins = pmx->pins; > + dev_dbg(pmx->dev, "got pins %d", *nr_pins); > + > + return 0; > +} > + > +static const struct scmi_device_id scmi_id_table[] = { > + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, > + { } > +}; > +MODULE_DEVICE_TABLE(scmi, scmi_id_table); > + > +static int scmi_pinctrl_probe(struct scmi_device *sdev) > +{ > + int ret; > + struct device *dev = &sdev->dev; > + struct scmi_pinctrl *pmx; > + const struct scmi_handle *handle; > + struct scmi_protocol_handle *ph; > + > + if (!sdev || !sdev->handle) > + return -EINVAL; > + > + handle = sdev->handle; > + > + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph); > + if (IS_ERR(pinctrl_ops)) > + return PTR_ERR(pinctrl_ops); > + > + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL); > + if (!pmx) > + return -ENOMEM; > + > + pmx->ph = ph; > + > + pmx->dev = dev; > + pmx->pctl_desc.name = DRV_NAME; > + pmx->pctl_desc.owner = THIS_MODULE; > + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops; > + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops; > + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops; > + > + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins, > + &pmx->pctl_desc.pins); > + if (ret) > + return ret; > + > + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx, &pmx->pctldev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n"); > + > + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev); > + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev); > + > + if (pmx->nr_functions) { > + pmx->functions = > + devm_kcalloc(dev, pmx->nr_functions, sizeof(*pmx->functions), > + GFP_KERNEL); > + if (!pmx->functions) > + return -ENOMEM; > + } > + > + if (pmx->nr_groups) { > + pmx->groups = > + devm_kcalloc(dev, pmx->nr_groups, sizeof(*pmx->groups), GFP_KERNEL); > + if (!pmx->groups) > + return -ENOMEM; > + } > + > + return pinctrl_enable(pmx->pctldev); > +} > + > +static struct scmi_driver scmi_pinctrl_driver = { > + .name = DRV_NAME, > + .probe = scmi_pinctrl_probe, > + .id_table = scmi_id_table, > +}; > +module_scmi_driver(scmi_pinctrl_driver); > + > +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>"); > +MODULE_DESCRIPTION("ARM SCMI pin controller driver"); > +MODULE_LICENSE("GPL"); > -- > 2.25.1
On Wed, Aug 23, 2023 at 01:13:12PM +0900, AKASHI Takahiro wrote: > Hi Oleksii, > Hi AKASHI, Oleksii, > On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote: > > scmi-pinctrl driver implements pinctrl driver interface and using > > SCMI protocol to redirect messages from pinctrl subsystem SDK to > > SCMI platform firmware, which does the changes in HW. > > > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> [snip] > > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin, > > + unsigned long *config) > > +{ > > + int ret; > > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > > + enum pin_config_param config_type; > > + unsigned long config_value; > > + > > + if (!config) > > + return -EINVAL; > > + > > + config_type = pinconf_to_config_param(*config); > > + > > + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, config_type, &config_value); > > + if (ret) > > + return ret; > > + > > + *config = pinconf_to_config_packed(config_type, config_value); > > + > > + return 0; > > +} > > + > > +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev, > > + unsigned int _pin, > > + unsigned long *configs, > > + unsigned int num_configs) > > +{ > > + int i, ret; > > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > > + enum pin_config_param config_type; > > + unsigned long config_value; > > + > > + if (!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]); > > The generic pinconf parameters defined as "enum pin_config_param" > do not exactly match with a set of SCMI's configuration types defined > in Table 23 in the section 4.11.2.6 "PINCTRL_CONFIG_GET". > pinconf_to_config_param() simply masks the lowest 8 bits of the input > value, but doesn't convert anything. Indeed the SCMI Types table and the Linux Pinctrl subsystem types are similar but not really the same; some kind of conversion/mapping will be needed. Some trivial conversion layer will be needed also in order to address here any of the possible future changes in the Linux pinctrl subsystem definitions without having to change the SCMI server side (that typically is fw...and that anyway is bound to the SCMI spec) Thanks, Cristian
This Patch series is intended to introduce the 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. Also, sharing link to the ATF pinctrl driver I used for testing: https://github.com/oleksiimoisieiev/arm-trusted-firmware/tree/pinctrl_rcar_m3_up [0] https://developer.arm.com/documentation/den0056/latest --- Changes v3 -> v4: - Fixed MAINTAINERS file description - adjusted pinctrl ops position and callback names - add trailing coma in scmi_protocol list - removed unneeded pi checks - corrected selector check - resource allocation refactoring - scmi_*_info swap params to generate better code - style, add trailing coma in definitions - reworked protocol@19 format in device-tree bindings - ordered config option and object file alphabetically - rephrased PINCTRL_SCMI config description - formatting fixes, removed blank lines after get_drvdata call - code style adjustments - add set_drvdata call - removed goto label - refactoring of the devm resource management - removed pctldev != NULL check - fix parameter name in pinconf-group-get - probe function refactoring - removed unneeded pmx checks Changes v2 -> v3: - update get_name calls as suggested by Cristian Marussi - fixing comments - refactoring of the dt_bindings according to the comments 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 --- Cristian Marussi (1): firmware: arm_scmi: Add optional flags to extended names helper 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 | 53 ++ MAINTAINERS | 7 + drivers/firmware/arm_scmi/Makefile | 2 +- drivers/firmware/arm_scmi/clock.c | 2 +- drivers/firmware/arm_scmi/driver.c | 9 +- drivers/firmware/arm_scmi/perf.c | 3 +- drivers/firmware/arm_scmi/pinctrl.c | 791 ++++++++++++++++++ drivers/firmware/arm_scmi/power.c | 2 +- drivers/firmware/arm_scmi/powercap.c | 2 +- drivers/firmware/arm_scmi/protocols.h | 4 +- drivers/firmware/arm_scmi/reset.c | 3 +- drivers/firmware/arm_scmi/sensors.c | 2 +- drivers/firmware/arm_scmi/voltage.c | 2 +- drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-scmi.c | 442 ++++++++++ include/linux/scmi_protocol.h | 42 + 17 files changed, 1367 insertions(+), 11 deletions(-) create mode 100644 drivers/firmware/arm_scmi/pinctrl.c create mode 100644 drivers/pinctrl/pinctrl-scmi.c