On 25.04.23 05:13, Peng Fan wrote: > > > 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. > > Hi Peng, I think SCMI protocol can be modified to accept array of uint32_t of config_values on PINCTRL_CONFIG_SET and return array for config_values on PINCTRL_CONFIG_GET. Let's see what others think about that. Thanks, Oleksii. >> >> 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@xxxxxxxx> >>>>>>> --- >>>>>>> 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@xxxxxxxx> >>>>>>> +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >>>>>>> +S: Maintained >>>>>>> +F: drivers/firmware/arm_scmi/pinctrl.c >>>>>>> + >>>>>>> SYSTEM RESET/SHUTDOWN DRIVERS >>>>>>> M: Sebastian Reichel <sre@xxxxxxxxxx> >>>>>>> L: linux-pm@xxxxxxxxxxxxxxx >>>>>>> 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@xxxxxxxxxxxxxxxxxxx >>>>>> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!GF_29dbcQIUBPA!y2hR3PEGGxiPjVeXBcgGyV03DPDhzgUKR0uHvsTpiafKgBar8Egc6oOOs-IkFIquhSf-qBzltqEMyzRZrZi7qlk$ >>>>>> >>>>>> >>>>>> [lists[.]infradead[.]org]
Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>
- Date: Tue, 25 Apr 2023 07:22:11 +0000
- Accept-language: en-US
- In-reply-to: <f6d0c919-d6da-4361-79ed-1f7fca4d2121@oss.nxp.com>
- References: <cover.1680793130.git.oleksii_moisieiev@epam.com> <54119b2cb43e29f69c5858a5320d3a58f23fed21.1680793130.git.oleksii_moisieiev@epam.com> <ZDcqx9JVMvqr2WYu@e120937-lin> <6dc456ff-7fc6-3b73-3727-dd048e9a9629@oss.nxp.com> <f73f39e2-81dd-4204-a3be-c5e7f5e54c1b@epam.com> <b9eca9a4-f4b6-b98c-2861-eb14380d2d5f@oss.nxp.com> <3e696235-a772-f988-da3d-2166f827267b@epam.com> <f6d0c919-d6da-4361-79ed-1f7fca4d2121@oss.nxp.com>
- References:
- [RFC v1 0/2] Introducing generic SCMI pinctrl driver implementation
- From: Oleksii Moisieiev
- [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Oleksii Moisieiev
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Cristian Marussi
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Peng Fan
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Oleksii Moisieiev
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Peng Fan
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Oleksii Moisieiev
- Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- From: Peng Fan
- [RFC v1 0/2] Introducing generic SCMI pinctrl driver implementation
- Prev by Date: Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support
- Next by Date: Re: [PATCH v10 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs
- Previous by thread: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- Next by thread: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
- Index(es):
 |