Messages in this thread | | | Date | Mon, 17 Apr 2023 10:55:17 +0800 | Subject | Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver | From | Peng Fan <> |
| |
On 4/13/2023 6:04 AM, Cristian Marussi wrote: > On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote: >> Implementation of the SCMI client driver, which implements >> PINCTRL_PROTOCOL. This protocol has ID 19 and is described >> in the latest DEN0056 document. > > Hi, > >> This protocol is part of the feature that was designed to >> separate the pinctrl subsystem from the SCP firmware. >> The idea is to separate communication of the pin control >> subsystem with the hardware to SCP firmware >> (or a similar system, such as ATF), which provides an interface >> to give the OS ability to control the hardware through SCMI protocol. >> This is a generic driver that implements SCMI protocol, >> independent of the platform type. >> >> DEN0056 document: >> https://developer.arm.com/documentation/den0056/latest >> > > No need to specify all of this in the commit message, just a note that > you are adding a new SCMIv3.2 Pincontrol protocol, highlighting anything > that has been left out in this patch (if any) will be enough.
Is it possible to extend the spec to support multilple uint32_t for PIN CONFIG SET?
With only one uint32_t could not satisfy i.MX requirement.
Thanks, Peng.
> You can look at the very first commit logs of existing protos as an > example like: drivers/firmware/arm_scmi/powercap.c > > Some more comments down below, I'll mostly skip anything related to the > SCMI API change I mentioned before... > > I'll also wont comment on more trivial stuff related to style, BUT there > are lots of them: you should run > > ./scripts/checkpatch.pl --strict <your-git-format-patch-file> > > for each patch in the series. (and fix accordingly..spacing, brackets...etc) > >> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> >> --- >> MAINTAINERS | 6 + >> drivers/firmware/arm_scmi/Makefile | 2 +- >> drivers/firmware/arm_scmi/common.h | 1 + >> drivers/firmware/arm_scmi/driver.c | 3 + >> drivers/firmware/arm_scmi/pinctrl.c | 905 ++++++++++++++++++++++++++++ >> drivers/pinctrl/Kconfig | 9 + >> include/linux/scmi_protocol.h | 58 +- >> 7 files changed, 982 insertions(+), 2 deletions(-) >> create mode 100644 drivers/firmware/arm_scmi/pinctrl.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 281de213ef47..abc543fd7544 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16961,6 +16961,12 @@ F: drivers/reset/reset-scmi.c >> F: include/linux/sc[mp]i_protocol.h >> F: include/trace/events/scmi.h >> >> +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/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 bc0d54f8e861..5cec357fbfa8 100644 >> --- a/drivers/firmware/arm_scmi/Makefile >> +++ b/drivers/firmware/arm_scmi/Makefile >> @@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o >> scmi-transport-y = shmem.o >> scmi-transport-$(CONFIG_MAILBOX) += mailbox.o >> scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o >> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o >> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o pinctrl.o >> scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \ >> $(scmi-transport-y) >> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o >> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h >> index 65063fa948d4..8bbb404abe8d 100644 >> --- a/drivers/firmware/arm_scmi/common.h >> +++ b/drivers/firmware/arm_scmi/common.h >> @@ -170,6 +170,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(power); >> DECLARE_SCMI_REGISTER_UNREGISTER(reset); >> DECLARE_SCMI_REGISTER_UNREGISTER(sensors); >> DECLARE_SCMI_REGISTER_UNREGISTER(system); >> +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl); >> >> #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \ >> int __init scmi_##name##_register(void) \ >> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c >> index 3dfd8b6a0ebf..fb9525fb3c24 100644 >> --- a/drivers/firmware/arm_scmi/driver.c >> +++ b/drivers/firmware/arm_scmi/driver.c >> @@ -743,6 +743,7 @@ static struct scmi_prot_devnames devnames[] = { >> { SCMI_PROTOCOL_CLOCK, { "clocks" },}, >> { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, >> { SCMI_PROTOCOL_RESET, { "reset" },}, >> + { SCMI_PROTOCOL_PINCTRL, { "pinctrl" },}, >> }; >> >> static inline void >> @@ -947,6 +948,7 @@ static int __init scmi_driver_init(void) >> scmi_reset_register(); >> scmi_sensors_register(); >> scmi_system_register(); >> + scmi_pinctrl_register(); >> >> return platform_driver_register(&scmi_driver); >> } >> @@ -962,6 +964,7 @@ static void __exit scmi_driver_exit(void) >> scmi_reset_unregister(); >> scmi_sensors_unregister(); >> scmi_system_unregister(); >> + scmi_pinctrl_unregister(); >> >> platform_driver_unregister(&scmi_driver); >> } >> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c >> new file mode 100644 >> index 000000000000..037270d7f39b >> --- /dev/null >> +++ b/drivers/firmware/arm_scmi/pinctrl.c >> @@ -0,0 +1,905 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * System Control and Management Interface (SCMI) Pinctrl Protocol >> + * >> + * Copyright (C) 2021 EPAM. > > nitpick: update (C) years > >> + */ >> + >> +#define pr_fmt(fmt) "SCMI Notifications PINCTRL - " fmt >> + > > This is not needed, no notifs in this proto. > >> +#include <linux/scmi_protocol.h> >> +#include <linux/slab.h> >> + >> +#include "common.h" >> +#include "notify.h" > > Notifs not needed, and in the new API world you'll just need a: > > #include "protocols.h" > >> + >> +#define SET_TYPE(x) ((x) & 0x3) >> + > > Even if trivial better to use std bitfield.h macros like > FIELD_GET() BIT() ... etc > >> +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 >> +}; >> + >> +enum scmi_pinctrl_selector_type { >> + PIN_TYPE = 0, >> + GROUP_TYPE, >> + FUNCTION_TYPE >> +}; >> + >> +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; >> + u16 nr_groups; >> + u16 nr_functions; >> + u16 nr_pins; > > Since these vars are not related to stricly spaced message fields (even though > derived from such messages) do not use sized types, you can just stick with > unsigned int. (it is also better not to mix sized and unsized types in the same > struct). This also could come handy if these will be exposed to the user > in scmi_protocol.h in the future (more on this down below) > >> + struct scmi_group_info *groups; >> + struct scmi_function_info *functions; >> + struct scmi_pin_info *pins; >> +}; >> + >> +struct scmi_conf_tx { >> + __le32 identifier; >> +#define SET_TYPE_BITS(attr, x) (((attr) & 0xFFFFFCFF) | (x & 0x3) << 8) >> +#define SET_CONFIG(attr, x) (((attr) & 0xFF) | (x & 0xFF)) > > Use bitfield.h like FIELD_SET / GENMASK etc > >> + __le32 attributes; >> +}; >> + >> +static int scmi_pinctrl_attributes_get(const struct scmi_handle *handle, >> + struct scmi_pinctrl_info *pi) >> +{ >> + int ret; >> + struct scmi_xfer *t; >> + struct scmi_msg_pinctrl_protocol_attributes { >> +#define GROUPS_NR(x) ((x) >> 16) >> +#define PINS_NR(x) ((x) & 0xffff) >> + __le32 attributes_low; >> +#define FUNCTIONS_NR(x) ((x) & 0xffff) >> + __le32 attributes_high; >> + } *attr; > > For consistency with the rest of the stack (mostly :D), please move this struct > definition and related macros outside in the global scope right after command > enum. (and use bitfield macros ...) > >> + >> + if (!pi) >> + return -EINVAL; >> + >> + ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES, >> + SCMI_PROTOCOL_PINCTRL, 0, sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + attr = t->rx.buf; >> + >> + ret = scmi_do_xfer(handle, t); >> + if (!ret) { >> + pi->nr_functions = >> + le16_to_cpu(FUNCTIONS_NR(attr->attributes_high)); >> + pi->nr_groups = le16_to_cpu(GROUPS_NR(attr->attributes_low)); >> + pi->nr_pins = le16_to_cpu(PINS_NR(attr->attributes_low)); >> + } >> + >> + scmi_xfer_put(handle, t); >> + return ret; >> +} >> + >> +static int scmi_pinctrl_get_groups_count(const struct scmi_handle *handle) >> +{ >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv) >> + return -ENODEV; >> + >> + pi = handle->pinctrl_priv; >> + >> + return pi->nr_groups; >> +} >> + >> +static int scmi_pinctrl_get_pins_count(const struct scmi_handle *handle) >> +{ >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv) >> + return -ENODEV; >> + >> + pi = handle->pinctrl_priv; >> + >> + return pi->nr_pins; >> +} >> + >> +static int scmi_pinctrl_get_functions_count(const struct scmi_handle *handle) >> +{ >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv) >> + return -ENODEV; >> + >> + pi = handle->pinctrl_priv; >> + >> + return pi->nr_functions; >> +} >> + >> +static int scmi_pinctrl_validate_id(const struct scmi_handle *handle, >> + u32 identifier, >> + enum scmi_pinctrl_selector_type type) >> +{ >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv) >> + return -ENODEV; >> + >> + switch (type) { >> + case PIN_TYPE: >> + pi = handle->pinctrl_priv; >> + >> + return (identifier < pi->nr_pins) ? 0 : -EINVAL; >> + case GROUP_TYPE: >> + return (identifier < >> + scmi_pinctrl_get_groups_count(handle)) ? >> + 0 : -EINVAL; >> + case FUNCTION_TYPE: >> + return (identifier < >> + scmi_pinctrl_get_functions_count(handle)) ? >> + 0 : -EINVAL; >> + default: >> + return -EINVAL; >> + } > > Here I would just pick the right value to compare, break and then > compare and exit, something aroundf the lines of: > > case PIN_TYPE: > ... > val = pi->nr_pins; > break; > ... > case GROUP_TYPE: > val = scmi_pinctrl_get_groups_count()); > break; > > .... > .... > default: > return -EINVAL; > } > > if (identifier >= val) > return -EINVAL; > > return 0; > } > > ... it's easier to read. What do you think ? > > >> +} >> + >> +static int scmi_pinctrl_get_name(const struct scmi_handle *handle, >> + u32 identifier, >> + enum scmi_pinctrl_selector_type type, >> + char **name) >> +{ > > As said, there is common helper for this, but it will need some small > adaptation in the SCMI core to work here so keep it as it is, and I'll take > care of this later, if it sounds fine for you. > >> + struct scmi_xfer *t; >> + int ret = 0; >> + struct scmi_name_tx { >> + __le32 identifier; >> + __le32 flags; >> + } *tx; >> + struct scmi_name_rx { >> + __le32 flags; >> + u8 name[64]; >> + } *rx; >> + >> + if (!handle || !name) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, identifier, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_NAME_GET, >> + SCMI_PROTOCOL_PINCTRL, >> + 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 = scmi_do_xfer(handle, t); >> + if (ret) >> + goto out; >> + >> + if (rx->flags) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + *name = kasprintf(GFP_KERNEL, "%s", rx->name); >> + if (!*name) >> + ret = -ENOMEM; >> + out: >> + scmi_xfer_put(handle, t); >> + >> + return ret; >> +} >> + >> +static int scmi_pinctrl_attributes(const struct scmi_handle *handle, >> + enum scmi_pinctrl_selector_type type, >> + u32 selector, char **name, >> + unsigned int *n_elems) >> +{ >> + int ret = 0; >> + struct scmi_xfer *t; >> + struct scmi_pinctrl_attributes_tx { >> + __le32 identifier; >> + __le32 flags; >> + } *tx; >> + struct scmi_pinctrl_attributes_rx { >> +#define EXT_NAME_FLAG(x) ((x) & BIT(31)) >> +#define NUM_ELEMS(x) ((x) & 0xffff) >> + __le32 attributes; >> + u8 name[16]; >> + } *rx; > > Ditto. Move these defs outside, bitfield.h for macros and try to use the > same naming style for message structs as in other protos, i.e. > > for commands: struct scmi_msg_pinctrl_attributes > for replies: struct scmi_resp_pinctrl_attributes > > (or some variations around that... > scmi_msg_cmd_* scmi_msg_resp_* > > we have not been fully consistent really, so I dont want to be > pedantic here, but we never used tx/rx in message context since it is > already (mis)-used in SCMI channel context...) > >> + >> + if (!handle || !name) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, selector, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_ATTRIBUTES, >> + SCMI_PROTOCOL_PINCTRL, 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 = scmi_do_xfer(handle, 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_name(handle, selector, type, name); >> + out: >> + scmi_xfer_put(handle, t); >> + return ret; >> +} >> + >> +static int scmi_pinctrl_list_associations(const struct scmi_handle *handle, >> + u32 selector, >> + enum scmi_pinctrl_selector_type type, >> + uint16_t size, unsigned int *array) >> +{ > > This is the other functionalities you could implement straight away using > ph->hops helpers (iterators) but just leave it this way, and I'll port it later > (once we retested all of this as working with the new API but without any > ph->hops usage..I think it is safer to change one bit at time... :P) > >> + struct scmi_xfer *t; >> + struct scmi_pinctrl_list_assoc_tx { >> + __le32 identifier; >> + __le32 flags; >> + __le32 index; >> + } *tx; >> + struct scmi_pinctrl_list_assoc_rx { >> +#define RETURNED(x) ((x) & 0xFFF) >> +#define REMAINING(x) ((x) >> 16) >> + __le32 flags; >> + __le16 array[]; >> + } *rx; > > Ditto, about struct naming and macros. > >> + u16 tot_num_ret = 0, loop_num_ret; >> + u16 remaining_num_ret; >> + int ret, loop; >> + >> + if (!handle || !array || !size) >> + return -EINVAL; >> + >> + if (type == PIN_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, selector, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_LIST_ASSOCIATIONS, >> + SCMI_PROTOCOL_PINCTRL, sizeof(*tx), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + tx = t->tx.buf; >> + rx = t->rx.buf; >> + >> + do { >> + tx->identifier = cpu_to_le32(selector); >> + tx->flags = SET_TYPE(cpu_to_le32(type)); >> + tx->index = cpu_to_le32(tot_num_ret); >> + >> + ret = scmi_do_xfer(handle, t); >> + if (ret) >> + break; >> + >> + loop_num_ret = le32_to_cpu(RETURNED(rx->flags)); >> + remaining_num_ret = le32_to_cpu(REMAINING(rx->flags)); >> + >> + for (loop = 0; loop < loop_num_ret; loop++) { >> + if (tot_num_ret + loop >= size) { >> + ret = -EMSGSIZE; >> + goto out; >> + } >> + >> + array[tot_num_ret + loop] = >> + le16_to_cpu(rx->array[loop]); >> + } >> + >> + tot_num_ret += loop_num_ret; >> + >> + scmi_reset_rx_to_maxsz(handle, t); >> + } while (remaining_num_ret > 0); >> +out: >> + scmi_xfer_put(handle, t); >> + return ret; >> +} >> + >> +static int scmi_pinctrl_request_config(const struct scmi_handle *handle, >> + u32 selector, >> + enum scmi_pinctrl_selector_type type, >> + u32 *config) >> +{ >> + struct scmi_xfer *t; >> + struct scmi_conf_tx *tx; >> + __le32 *packed_config; >> + u32 attributes = 0; >> + int ret; >> + >> + if (!handle || !config || type == FUNCTION_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, selector, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_GET, >> + SCMI_PROTOCOL_PINCTRL, >> + sizeof(*tx), sizeof(*packed_config), &t); >> + if (ret) >> + return ret; >> + >> + tx = t->tx.buf; >> + packed_config = t->rx.buf; >> + tx->identifier = cpu_to_le32(selector); >> + attributes = SET_TYPE_BITS(attributes, type); >> + attributes = SET_CONFIG(attributes, *config); >> + >> + tx->attributes = cpu_to_le32(attributes); >> + >> + ret = scmi_do_xfer(handle, t); >> + >> + if (!ret) >> + *config = le32_to_cpu(*packed_config); >> + >> + scmi_xfer_put(handle, t); >> + return ret; >> +} >> + >> +static int scmi_pinctrl_get_config(const struct scmi_handle *handle, u32 pin, >> + u32 *config) >> +{ >> + return scmi_pinctrl_request_config(handle, pin, PIN_TYPE, config); >> +} >> + >> +static int scmi_pinctrl_apply_config(const struct scmi_handle *handle, >> + u32 selector, >> + enum scmi_pinctrl_selector_type type, >> + u32 config) >> +{ >> + struct scmi_xfer *t; >> + struct scmi_conf_tx *tx; >> + u32 attributes = 0; >> + int ret; >> + >> + if (!handle || type == FUNCTION_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, selector, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_SET, >> + SCMI_PROTOCOL_PINCTRL, >> + sizeof(*tx), 0, &t); >> + if (ret) >> + return ret; >> + >> + tx = t->tx.buf; >> + tx->identifier = cpu_to_le32(selector); >> + attributes = SET_TYPE_BITS(attributes, type); >> + attributes = SET_CONFIG(attributes, config); >> + tx->attributes = cpu_to_le32(attributes); >> + >> + ret = scmi_do_xfer(handle, t); >> + >> + scmi_xfer_put(handle, t); >> + return ret; >> +} >> + >> +static int scmi_pinctrl_set_config(const struct scmi_handle *handle, u32 pin, >> + u32 config) >> +{ >> + return scmi_pinctrl_apply_config(handle, pin, PIN_TYPE, config); >> +} >> + >> +static int scmi_pinctrl_get_config_group(const struct scmi_handle *handle, >> + u32 group, u32 *config) >> +{ >> + return scmi_pinctrl_request_config(handle, group, GROUP_TYPE, config); >> +} >> + >> +static int scmi_pinctrl_set_config_group(const struct scmi_handle *handle, >> + u32 group, u32 config) >> +{ >> + return scmi_pinctrl_apply_config(handle, group, GROUP_TYPE, config); >> +} >> + >> +static int scmi_pinctrl_function_select(const struct scmi_handle *handle, >> + u32 identifier, >> + enum scmi_pinctrl_selector_type type, >> + u32 function_id) >> +{ >> + struct scmi_xfer *t; >> + struct scmi_func_set_tx { >> + __le32 identifier; >> + __le32 function_id; >> + __le32 flags; >> + } *tx; > > Ditto. > >> + int ret; >> + >> + if (!handle || type == FUNCTION_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, identifier, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_FUNCTION_SELECT, >> + SCMI_PROTOCOL_PINCTRL, >> + 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 = scmi_do_xfer(handle, t); >> + scmi_xfer_put(handle, t); >> + >> + return ret; >> +} >> + >> +static int scmi_pinctrl_request(const struct scmi_handle *handle, >> + u32 identifier, >> + enum scmi_pinctrl_selector_type type) >> +{ >> + struct scmi_xfer *t; >> + int ret; >> + struct scmi_request_tx { >> + __le32 identifier; >> + __le32 flags; >> + } *tx; >> + > > Ditto. > >> + if (!handle || type == FUNCTION_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, identifier, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_REQUEST, SCMI_PROTOCOL_PINCTRL, >> + sizeof(*tx), 0, &t); >> + >> + tx = t->tx.buf; >> + tx->identifier = identifier; >> + tx->flags = SET_TYPE(cpu_to_le32(type)); >> + >> + ret = scmi_do_xfer(handle, t); >> + scmi_xfer_put(handle, t); >> + >> + return ret; >> +} >> + >> +static int scmi_pinctrl_request_pin(const struct scmi_handle *handle, u32 pin) >> +{ >> + return scmi_pinctrl_request(handle, pin, PIN_TYPE); >> +} >> + >> +static int scmi_pinctrl_free(const struct scmi_handle *handle, u32 identifier, >> + enum scmi_pinctrl_selector_type type) >> +{ >> + struct scmi_xfer *t; >> + int ret; >> + struct scmi_request_tx { >> + __le32 identifier; >> + __le32 flags; >> + } *tx; >> + > Ditto. > >> + if (!handle || type == FUNCTION_TYPE) >> + return -EINVAL; >> + >> + ret = scmi_pinctrl_validate_id(handle, identifier, type); >> + if (ret) >> + return ret; >> + >> + ret = scmi_xfer_get_init(handle, PINCTRL_RELEASE, SCMI_PROTOCOL_PINCTRL, >> + sizeof(*tx), 0, &t); >> + >> + tx = t->tx.buf; >> + tx->identifier = identifier; >> + tx->flags = SET_TYPE(cpu_to_le32(type)); >> + >> + ret = scmi_do_xfer(handle, t); >> + scmi_xfer_put(handle, t); >> + >> + return ret; >> +} >> + >> +static int scmi_pinctrl_free_pin(const struct scmi_handle *handle, u32 pin) >> +{ >> + return scmi_pinctrl_free(handle, pin, PIN_TYPE); >> +} >> + >> + >> +static int scmi_pinctrl_get_group_info(const struct scmi_handle *handle, >> + u32 selector, >> + struct scmi_group_info *group) >> +{ >> + int ret = 0; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !group) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + ret = scmi_pinctrl_attributes(handle, GROUP_TYPE, selector, >> + &group->name, >> + &group->nr_pins); >> + if (ret) >> + return ret; >> + >> + if (!group->nr_pins) { >> + dev_err(handle->dev, "Group %d has 0 elements", selector); >> + return -ENODATA; >> + } >> + >> + group->group_pins = devm_kmalloc_array(handle->dev, group->nr_pins, >> + sizeof(*group->group_pins), >> + GFP_KERNEL); > > I think you can just use for the array allocation > > devm_kcalloc(dev, n, size, flags) > > and it will add also __GFP_ZERO internally to clear it. > (indeed it calls in turn devm_kmalloc_array) > > ...BUT I think there is a further tricky issue here related to memory allocation... > > You call this and others function of this kind from some scmi_pinctrl_ops, > like in scmi_pinctrl_get_group_pins (scmi_pinctrl_ops->get_group_pins), > and then this is in turn called by the SCMI Pinctrl driver via > pinctrl_ops->get_group_pins AND you set a present flag so that you issue a > PINCTRL_LIST_ASSOCIATIONS and allocate here a new group_pins array just > the first time: but these are never released anywhere, since, even though > lazily dynamically allocated when asked for, these are static data that > you pass to the caller/user of this protocol and so you cannot release > them anytime soon, indeed. > > The core SCMI stack usually takes care to track and release all the devm_ > resources allocated by the protocol ONLY if they were allocated with devres > while inside scmi_pinctrl_protocol_init() function. > (see drivers/firmware/arm-scmi/driver.c:scmi_alloc_init_protocol_instance() > and scmi_protocol_release) > > BUT you do not allocate these arrays inside the protocol-init function, > you allocate them the first time these ops are called at runtime. > > If you unbind/unload all the drivers using this protocol and then reload > them, all the devm_ allocations in protocol_init will be freed and > reallocated BUT these arrays will never be freed (they are boudn to handle->dev) > and instead they will be reallocated multiple times (present flag will be cleared > on unload), remained unused and freed finally only when the whole SCMI stack is > unbind/unloaded. > > You use a present flag to avoid reissuing the same query and > reallocating all the arrays each time a driver calls these > protocol_ops one, but really all these data is available early on at > protocol init time and they are not supposed to change at runtime, dont they ? > > Even in a virtualized environment, you boot an agent and the SCMI > platform server provides to the agent the list of associations when > queried but then this does not change until the next reboot right ? > (indeed you do not query more than once...) > > The agent can only change the PIN status with CONFIG_SET or > FUNCTION_SELECT or REQUEST the exclusive use of a pin/group, but it is > not that the platform can change the pin/groups associaion for the same > agent at run time, this are static data for the whole life of the agent. > > Am I right ? > > IOW I think there is some potential memory leak on unbind/bind and it would > be better to query and allocate all of these resources at init time and keep > them ready to be retrieved by subsequent operations, since the lifetime > of these resources is pretty long and they are basically representing > static data that does not change after the init/probe phases. > > Indeed, all the other protocols usually allocate all the needed > resources and query all the available SCMI resources once for all during > the protocol_init, storing all the retrieved info in some struct *_info > exposed in scmi_protocol.h and then provide some related protocol_ops to > get the number of resources and to retrieve specific domain info descriptors. > (voltage.c is an example and more on this down below...) > > This way, any dynamic allocation is done during protocol_init, so > it can be automatically freed by the SCMI core once there are no more > users of that protocol, and all of this static info data is queried > and retrieved once for all at protocol initialization time, avoiding > unneeded message exchanges to retrieve always the same data. > (which you avoid anyway with the present flag) > > If you have a good reason to instead perform this sort of lazy > allocation/query performed only at the last minute when someone ask for > that specific resource, you will have to provide also a .instance_deinit > function to clean anything you allocated out of the .instance_init > routine; but this would seem strange to me since any resource that is > discovered at init will be eventually immediately queried by a driver > which uses this protocol...am I missing something ? > > >> + if (!group->group_pins) { >> + ret = -ENOMEM; >> + goto err; >> + } >> + >> + ret = scmi_pinctrl_list_associations(handle, 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_handle *handle, >> + u32 selector, const char **name) >> +{ >> + int ret; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !name) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + if (selector > pi->nr_groups) >> + return -EINVAL; >> + >> + if (!pi->groups[selector].present) { >> + ret = scmi_pinctrl_get_group_info(handle, 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_handle *handle, >> + u32 selector, const unsigned int **pins, >> + unsigned int *nr_pins) >> +{ >> + int ret; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !pins || !nr_pins) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + if (selector > pi->nr_groups) >> + return -EINVAL; >> + >> + if (!pi->groups[selector].present) { >> + ret = scmi_pinctrl_get_group_info(handle, 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_handle *handle, >> + u32 selector, >> + struct scmi_function_info *func) >> +{ >> + int ret = 0; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !func) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + ret = scmi_pinctrl_attributes(handle, FUNCTION_TYPE, selector, >> + &func->name, >> + &func->nr_groups); >> + if (ret) >> + return ret; >> + >> + if (!func->nr_groups) { >> + dev_err(handle->dev, "Function %d has 0 elements", selector); >> + return -ENODATA; >> + } >> + >> + func->groups = devm_kmalloc_array(handle->dev, func->nr_groups, >> + sizeof(*func->groups), >> + GFP_KERNEL); >> + if (!func->groups) { >> + ret = -ENOMEM; >> + goto err; >> + } >> + >> + ret = scmi_pinctrl_list_associations(handle, 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_handle *handle, >> + u32 selector, const char **name) >> +{ >> + int ret; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !name) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + if (selector > pi->nr_functions) >> + return -EINVAL; >> + >> + if (!pi->functions[selector].present) { >> + ret = scmi_pinctrl_get_function_info(handle, 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_handle *handle, >> + u32 selector, >> + unsigned int *nr_groups, >> + const unsigned int **groups) >> +{ >> + int ret; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !groups || !nr_groups) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + if (selector > pi->nr_functions) >> + return -EINVAL; >> + >> + if (!pi->functions[selector].present) { >> + ret = scmi_pinctrl_get_function_info(handle, 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_handle *handle, u32 selector, >> + u32 group) >> +{ >> + return scmi_pinctrl_function_select(handle, group, GROUP_TYPE, >> + selector); >> +} >> + >> +static int scmi_pinctrl_get_pin_info(const struct scmi_handle *handle, >> + u32 selector, struct scmi_pin_info *pin) >> +{ >> + int ret = 0; >> + struct scmi_pinctrl_info *pi; >> + unsigned int n_elems; >> + >> + if (!handle || !handle->pinctrl_priv || !pin) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + ret = scmi_pinctrl_attributes(handle, PIN_TYPE, selector, >> + &pin->name, >> + &n_elems); >> + if (ret) >> + return ret; >> + >> + if (n_elems != pi->nr_pins) { >> + dev_err(handle->dev, "Wrong pin count expected %d has %d", >> + pi->nr_pins, n_elems); >> + return -ENODATA; >> + } >> + >> + if (*(pin->name) == 0) { >> + dev_err(handle->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_handle *handle, u32 selector, >> + const char **name) >> +{ >> + >> + int ret; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !name) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + if (selector > pi->nr_pins) >> + return -EINVAL; >> + >> + if (!pi->pins[selector].present) { >> + ret = scmi_pinctrl_get_pin_info(handle, selector, >> + &pi->pins[selector]); >> + if (ret) >> + return ret; >> + } >> + >> + *name = pi->pins[selector].name; >> + >> + return 0; >> +} >> + >> + >> +static const struct scmi_pinctrl_ops pinctrl_ops = { >> + .get_groups_count = scmi_pinctrl_get_groups_count, >> + .get_group_name = scmi_pinctrl_get_group_name, >> + .get_group_pins = scmi_pinctrl_get_group_pins, >> + .get_functions_count = scmi_pinctrl_get_functions_count, >> + .get_function_name = scmi_pinctrl_get_function_name, >> + .get_function_groups = scmi_pinctrl_get_function_groups, >> + .set_mux = scmi_pinctrl_set_mux, >> + .get_pin_name = scmi_pinctrl_get_pin_name, >> + .get_pins_count = scmi_pinctrl_get_pins_count, >> + .get_config = scmi_pinctrl_get_config, >> + .set_config = scmi_pinctrl_set_config, >> + .get_config_group = scmi_pinctrl_get_config_group, >> + .set_config_group = scmi_pinctrl_set_config_group, >> + .request_pin = scmi_pinctrl_request_pin, >> + .free_pin = scmi_pinctrl_free_pin >> +}; >> + >> +static int scmi_pinctrl_protocol_init(struct scmi_handle *handle) >> +{ >> + u32 version; >> + struct scmi_pinctrl_info *pinfo; >> + int ret; >> + >> + if (!handle) >> + return -EINVAL; >> + >> + scmi_version_get(handle, SCMI_PROTOCOL_PINCTRL, &version); >> + >> + dev_dbg(handle->dev, "Pinctrl Version %d.%d\n", >> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); >> + >> + pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL); >> + if (!pinfo) >> + return -ENOMEM; >> + >> + ret = scmi_pinctrl_attributes_get(handle, pinfo); >> + if (ret) >> + goto free; >> + >> + pinfo->pins = devm_kmalloc_array(handle->dev, pinfo->nr_pins, >> + sizeof(*pinfo->pins), >> + GFP_KERNEL | __GFP_ZERO); > > devm_kcalloc() zeroes on its own > >> + if (!pinfo->pins) { >> + ret = -ENOMEM; >> + goto free; >> + } >> + >> + pinfo->groups = devm_kmalloc_array(handle->dev, pinfo->nr_groups, >> + sizeof(*pinfo->groups), >> + GFP_KERNEL | __GFP_ZERO); > > Ditto. >> + if (!pinfo->groups) { >> + ret = -ENOMEM; >> + goto free; >> + } >> + >> + pinfo->functions = devm_kmalloc_array(handle->dev, pinfo->nr_functions, >> + sizeof(*pinfo->functions), >> + GFP_KERNEL | __GFP_ZERO); >> + if (!pinfo->functions) { >> + ret = -ENOMEM; >> + goto free; >> + } >> + >> + pinfo->version = version; >> + handle->pinctrl_ops = &pinctrl_ops; >> + handle->pinctrl_priv = pinfo; >> + >> + return 0; >> +free: >> + if (pinfo) { >> + devm_kfree(handle->dev, pinfo->pins); >> + devm_kfree(handle->dev, pinfo->functions); >> + devm_kfree(handle->dev, pinfo->groups); >> + } > > These frees are really not needed...if this function return failure any > devres allocated in it is freed by the SCMI core. (as said above...in a > recent kernel with the new API of course) > >> + >> + devm_kfree(handle->dev, pinfo); >> + >> + return ret; >> +} >> + >> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(SCMI_PROTOCOL_PINCTRL, pinctrl) >> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >> index 815095326e2d..68add4d06e8c 100644 >> --- a/drivers/pinctrl/Kconfig >> +++ b/drivers/pinctrl/Kconfig >> @@ -431,4 +431,13 @@ config PINCTRL_EQUILIBRIUM >> pin functions, configure GPIO attributes for LGM SoC pins. Pinmux and >> pinconf settings are retrieved from device tree. >> >> +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. >> + > > This does NOT belong to this patch, but to the next right ? > >> endif >> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h >> index 9cd312a1ff92..6a909ef3bf51 100644 >> --- a/include/linux/scmi_protocol.h >> +++ b/include/linux/scmi_protocol.h >> @@ -12,7 +12,8 @@ >> #include <linux/notifier.h> >> #include <linux/types.h> >> >> -#define SCMI_MAX_STR_SIZE 16 >> +#define SCMI_MAX_STR_SIZE 16 >> +#define SCMI_MAX_STR_EXT_SIZE 64 > > This is handled as part of how the extended names are handled with ph->hops > in a common way, as I was saying, so please move this if you need it in > the protocol code, then I'll port to the ph->hops interface and clean > up. > >> #define SCMI_MAX_NUM_RATES 16 >> >> /** >> @@ -252,6 +253,55 @@ struct scmi_notify_ops { >> struct notifier_block *nb); >> }; >> >> +/** >> + * struct scmi_pinctrl_ops - represents the various operations provided >> + * by SCMI Pinctrl Protocol >> + * >> + * @get_groups_count: returns count of the registered groups >> + * @get_group_name: returns group name by index >> + * @get_group_pins: returns the set of pins, assigned to the specified group >> + * @get_functions_count: returns count of the registered fucntions >> + * @get_function_name: returns function name by indes >> + * @get_function_groups: returns the set of groups, assigned to the specified >> + * function >> + * @set_mux: set muxing function for groups of pins >> + * @get_pins: returns the set of pins, registered in driver >> + * @get_config: returns configuration parameter for pin >> + * @set_config: sets the configuration parameter for pin >> + * @get_config_group: returns the configuration parameter for a group of pins >> + * @set_config_group: sets the configuration parameter for a groups of pins >> + * @request_pin: aquire pin before selecting mux setting >> + * @free_pin: frees pin, acquired by request_pin call >> + */ >> +struct scmi_pinctrl_ops { >> + int (*get_groups_count)(const struct scmi_handle *handle); >> + int (*get_group_name)(const struct scmi_handle *handles, u32 selector, >> + const char **name); >> + int (*get_group_pins)(const struct scmi_handle *handle, u32 selector, >> + const unsigned int **pins, unsigned int *nr_pins); >> + int (*get_functions_count)(const struct scmi_handle *handle); >> + int (*get_function_name)(const struct scmi_handle *handle, u32 selector, >> + const char **name); >> + int (*get_function_groups)(const struct scmi_handle *handle, >> + u32 selector, unsigned int *nr_groups, >> + const unsigned int **groups); >> + int (*set_mux)(const struct scmi_handle *handle, u32 selector, >> + u32 group); >> + int (*get_pin_name)(const struct scmi_handle *handle, u32 selector, >> + const char **name); >> + int (*get_pins_count)(const struct scmi_handle *handle); >> + int (*get_config)(const struct scmi_handle *handle, u32 pin, >> + u32 *config); >> + int (*set_config)(const struct scmi_handle *handle, u32 pin, >> + u32 config); >> + int (*get_config_group)(const struct scmi_handle *handle, u32 pin, >> + u32 *config); >> + int (*set_config_group)(const struct scmi_handle *handle, u32 pin, >> + u32 config); >> + int (*request_pin)(const struct scmi_handle *handle, u32 pin); >> + int (*free_pin)(const struct scmi_handle *handle, u32 pin); >> +}; >> + > > As mentioned above, here you could drop a lot of this get_X_count/name/pins > and instead expose a few of the internal proocol struct scmi__X_info and then > provide just a mean to query how many resource are there and then get the info > descriptor you want for the specific domain_id, i.e.: > > int (*num_domains_get)(ph, type) > void *(*info_get)(ph, type, domain_id); > > Thanks, > Cristian > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
| |