Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver
From: Cristian Marussi
Date: Wed Apr 12 2023 - 18:04:37 EST
On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote:
> Implementation of the SCMI client driver, which implements
> PINCTRL_PROTOCOL. This protocol has ID 19 and is described
> in the latest DEN0056 document.
Hi,
> This protocol is part of the feature that was designed to
> separate the pinctrl subsystem from the SCP firmware.
> The idea is to separate communication of the pin control
> subsystem with the hardware to SCP firmware
> (or a similar system, such as ATF), which provides an interface
> to give the OS ability to control the hardware through SCMI protocol.
> This is a generic driver that implements SCMI protocol,
> independent of the platform type.
>
> DEN0056 document:
> https://developer.arm.com/documentation/den0056/latest
>
No need to specify all of this in the commit message, just a note that
you are adding a new SCMIv3.2 Pincontrol protocol, highlighting anything
that has been left out in this patch (if any) will be enough.
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