Messages in this thread | | | Date | Tue, 25 Apr 2023 10:13:56 +0800 | Subject | Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver | From | Peng Fan <> |
| |
On 4/24/2023 6:33 PM, Oleksii Moisieiev wrote: > > On 24.04.23 04:52, Peng Fan wrote: >> >> >> On 4/21/2023 4:40 PM, Oleksii Moisieiev wrote: >>> Hi Peng Fan, >>> >>> On 17.04.23 05:55, Peng Fan wrote: >>>> >>>> >>>> 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://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZHq8eC4g$ >>>>>> >>>>>> [developer[.]arm[.]com] >>>>>> >>>>> >>>>> 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. >>>> >>> IIUC you are expecting to have an ability to set some kind of array of >>> uint32_t config values to some specific ConfigType? >>> >>> I'm not sure if it's supported by pintctrl subsystem right now. I was >>> unable to find an example in the existing device-tree pinctrl bindings. >>> This makes me think that this kind of binding is OEM specific. >> >> Look at arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts >> >> Take lpuart for example: >> MX93_PAD_UART1_RXD__LPUART1_RX 0x31e >> >> The macro MX93_PAD_UART1_RXD__LPUART1_RX is an array. >> >> i.MX pinctrl driver use >> mux reg, mux value, select reg, select value, pad reg, pad value >> >> pinctrl driver will be handled by SCMI firmware for new i.MX SoC. >> >> With current spec, only one uint32 is low speed. So if the spec >> could extended to support more uint32 with pin config set, >> we could use one SCMI call to support i.MX pinctrl. >> >> Thanks >> Peng. >> > Hi Peng Fan, > > Thank you for the example. > > According to the comment for the function imx_pinctrl_parse_pin_mmio: > > /* > * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID > * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin. > * For generic_pinconf case, there's no extra u32 CONFIG. > * > * PIN_FUNC_ID format: > * Default: > * <mux_reg conf_reg input_reg mux_mode input_val> > * SHARE_MUX_CONF_REG: > * <mux_conf_reg input_reg mux_mode input_val> > * IMX_USE_SCU: > * <pin_id mux_mode> > */ > > IIUC this says that in scu format it is expected to use <pin_id > mux_mode>. This fit's protocol expectations.
The comments are misleading, it should be pin_id mux_mode config_setting See arch/arm64/boot/dts/freescale/imx8qm-mek.dts
> > The idea was to make all platform specific implementation, such as > <mux_reg conf_reg input_reg mux_mode input_val> > > on the SCP Firmware side and hide it under Pin ID and Config Type so > Client side can apply generic config in the device tree. > > Can't you use scu format here? Or execute > imx_pinconf_decode_generic_config on SCMI server side?
Technically with one config value is doable, but that means SCMI needs expose many pin ids, such as pin id SDHC_CD, it could be muxed as SDHC_CD, GPIO1_5, SAI_RX. Then scmi needs export: SDHC_CD_SDHC_CD SDHC_CD_GPIO1_5 SDHC_CD_SAI_RX
If with multiple uint32_value support by spec, scmi just needs exports SDHC_CD, with other values passed through uint32 value.
Regards, Peng.
> > Best regards, > > Oleksii. > >>> >>> Maybe it can be implemented by adding new IDs to OEM specific range >>> (192-255) which is reserved for OEM specific units (See Table 23 of >>> DEN0056E). >>> >>> Best regards, >>> >>> Oleksii >>> >>> >>>>> 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 >>>>> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZrZi7qlk$ >>>>> >>>>> [lists[.]infradead[.]org]
| |