On Thu, Jan 04, 2024 at 06:48:48PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.
>
Hi,
a few remarks.
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 524 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 537 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 487bff0d44c0..3fe790e90834 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21314,6 +21314,7 @@ F: drivers/clk/clk-sc[mp]i.c
> F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 8163a5983166..27b26e428f60 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -432,6 +432,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol interface"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + help
> + This driver provides support for pinctrl which is controlled
> + by firmware that implements the SCMI interface.
> + It uses SCMI Message Protocol to interact with the
> + firmware providing all the pinctrl controls.
> +
> config PINCTRL_SINGLE
> tristate "One-register-per-pin type device tree based pinctrl driver"
> depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 1071f301cc70..ba755ed2d46c 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o
> obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
> obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o
> obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o
> obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
> obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o
> obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> new file mode 100644
> index 000000000000..146308d27f54
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,524 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2023 EPAM
> + * Copyright 2023 NXP
> + */
2023-2024
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
[snip]
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> + .request = pinctrl_scmi_request,
> + .free = pinctrl_scmi_free,
> + .get_functions_count = pinctrl_scmi_get_functions_count,
> + .get_function_name = pinctrl_scmi_get_function_name,
> + .get_function_groups = pinctrl_scmi_get_function_groups,
> + .set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param, u8 *type)
> +{
Same as in the previous patch, is it reasonable to use the new enum here instead
of a generic u8 * ?
> + u8 arg = param;
> +
> + switch (arg) {
> + case PIN_CONFIG_BIAS_BUS_HOLD:
> + *type = SCMI_PIN_BIAS_BUS_HOLD;
> + break;
> + case PIN_CONFIG_BIAS_DISABLE:
> + *type = SCMI_PIN_BIAS_DISABLE;
> + break;
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + *type = SCMI_PIN_BIAS_PULL_DOWN;
> + break;
> + case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> + *type = SCMI_PIN_BIAS_PULL_DEFAULT;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + *type = SCMI_PIN_BIAS_PULL_UP;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + *type = SCMI_PIN_DRIVE_OPEN_DRAIN;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> + *type = SCMI_PIN_DRIVE_OPEN_SOURCE;
> + break;
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + *type = SCMI_PIN_DRIVE_PUSH_PULL;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + *type = SCMI_PIN_DRIVE_STRENGTH;
> + break;
> + case PIN_CONFIG_INPUT_DEBOUNCE:
> + *type = SCMI_PIN_INPUT_DEBOUNCE;
> + break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT:
> + *type = SCMI_PIN_INPUT_SCHMITT;
> + break;
> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> + *type = SCMI_PIN_INPUT_MODE;
> + break;
> + case PIN_CONFIG_MODE_LOW_POWER:
> + *type = SCMI_PIN_LOW_POWER_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + *type = SCMI_PIN_OUTPUT_MODE;
> + break;
> + case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
> + *type = SCMI_PIN_OUTPUT_VALUE;
> + break;
> + case PIN_CONFIG_POWER_SOURCE:
> + *type = SCMI_PIN_POWER_SOURCE;
> + break;
> + case PIN_CONFIG_SLEW_RATE:
> + *type = SCMI_PIN_SLEW_RATE;
> + break;
> + case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
> + *type = param;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + u32 config_value;
> + u8 type;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> +
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +
Issuing a cat /sys/kernel/debug/pictrl/<scmi_dev>/pinconf-pins
got a string of (adding some debug to print config_type):
[ 89.843868] scmi-pinctrl scmi_dev.12: Error map pinconf_type 22 - ret:-95
[ 89.846125] scmi-pinctrl scmi_dev.12: Error map pinconf_type 23 - ret:-95
but I suppose is due to some missing matching definitions in the SCMI
spec....just a heads up for the future.
> + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, type, &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> + unsigned int _pin,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
> + &pmx->config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> + pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
Are you sure that this pinctrl function cannot be called multiple times
concurrently ? Because pmx->config_type/value are globally defined for
the driver so any concurrent call will lead to corruption.
Moreover, you allocate those pmx->config_ arrays in probe as SZ_4K, but
effectively how many of these are effectively used in reality ?
I mean, we clearly have to address the worst case, which is 256 values by
the SCMI spec CONFIG_SET definitions, but typically how many configs are
going to be sent in a call ? because if it is a reasonable small values
you could just put in on the stack here with a check for overlflow and
a dynamic allocation to address the special case...this would also solve
the potential concurrent calls issue described above.
...but I could be missing something about Pinctrl so please explain.
> + ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs,
> + pmx->config_type, pmx->config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *configs,
> + unsigned int num_configs)
> +{
> + int i, ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!configs || !num_configs)
> + return -EINVAL;
> +
> + for (i = 0; i < num_configs; i++) {
> + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]),
> + &pmx->config_type[i]);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> + pmx->config_value[i] = pinconf_to_config_argument(configs[i]);
> + }
> +
> + ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs,
> + pmx->config_type, pmx->config_value);
> + if (ret)
> + dev_err(pmx->dev, "Error parsing config %d", ret);
> +
> + return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + u32 config_value;
> + u8 type;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
You dont check ret here ?
> +
> + ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, type, &config_value);
> + if (ret)
> + return ret;
> +
> + *config = pinconf_to_config_packed(config_type, config_value);
> +
> + return 0;
> +}
> +
Thanks,
Cristian
