| Message ID | 812ae71d017b115c55648dbf0a4c3502715b1955.1682513390.git.oleksii_moisieiev@epam.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introducing generic SCMI pinctrl driver implementation | expand |
On Wed, Apr 26, 2023 at 3:26 PM Oleksii Moisieiev <Oleksii_Moisieiev@epam.com> wrote: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCP firmware, which does the changes in HW. > > This setup expects SCP firmware (or similar system, such as ATF) > to be installed on the platform, which implements pinctrl driver > for the specific platform. > > SCMI-Pinctrl driver should be configured from the device-tree and uses > generic device-tree mappings for the configuration. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> This looks as good as ever. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCP firmware, which does the changes in HW. > > This setup expects SCP firmware (or similar system, such as ATF) > to be installed on the platform, which implements pinctrl driver > for the specific platform. > > SCMI-Pinctrl driver should be configured from the device-tree and uses > generic device-tree mappings for the configuration. > Hi Oleksii, just a few remarks down below. > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 9 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++ > 4 files changed, 589 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-scmi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0d251ebac437..ba9e3aea6176 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > L: linux-arm-kernel@lists.infradead.org > S: Maintained > F: drivers/firmware/arm_scmi/pinctrl.c > +F: drivers/pinctrl/pinctrl-scmi.c > > SYSTEM RESET/SHUTDOWN DRIVERS > M: Sebastian Reichel <sre@kernel.org> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index dcb53c4a9584..16bf2c67f095 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig" > source "drivers/pinctrl/visconti/Kconfig" > source "drivers/pinctrl/vt8500/Kconfig" > > +config PINCTRL_SCMI > + bool "Pinctrl driver controlled via SCMI interface" Why not a tristate ? The core SCMI stack can be compiled as module with all the protocols...at least for testing is useful to have this driver as M. > + 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. > + > endif > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index d5939840bb2a..21366db4f4f4 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o > obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o > obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o > obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o > +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > > obj-y += actions/ > obj-$(CONFIG_ARCH_ASPEED) += aspeed/ > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > new file mode 100644 > index 000000000000..8401db1d030b > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -0,0 +1,578 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Power Interface (SCMI) Protocol based pinctrl driver > + * > + * Copyright (C) 2023 EPAM > + */ > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/of.h> > +#include <linux/module.h> > +#include <linux/seq_file.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 <linux/scmi_protocol.h> > +#include <linux/slab.h> > + > +#include "pinctrl-utils.h" > +#include "core.h" > +#include "pinconf.h" > + > +#define DRV_NAME "scmi-pinctrl" > + > +static const struct scmi_pinctrl_proto_ops *pinctrl_ops; > + > +struct scmi_pinctrl_funcs { > + unsigned int num_groups; > + const char **groups; > +}; > + > +struct scmi_pinctrl { > + struct device *dev; > + struct scmi_protocol_handle *ph; > + struct pinctrl_dev *pctldev; > + struct pinctrl_desc pctl_desc; > + struct scmi_pinctrl_funcs *functions; > + unsigned int nr_functions; > + char **groups; > + unsigned int nr_groups; > + struct pinctrl_pin_desc *pins; > + unsigned int nr_pins; > +}; > + > +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE); > +} > + > +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return NULL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return NULL; > + > + ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->get_group_pins(pmx->ph, selector, > + pins, num_pins); > +} > + > +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, > + struct seq_file *s, > + unsigned int offset) > +{ > + seq_puts(s, DRV_NAME); > +} > + > +#ifdef CONFIG_OF > +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np_config, > + struct pinctrl_map **map, > + u32 *num_maps) > +{ > + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, > + num_maps, PIN_MAP_TYPE_INVALID); > +} > + > +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, > + struct pinctrl_map *map, u32 num_maps) > +{ > + kfree(map); > +} > + > +#endif /* CONFIG_OF */ > + > +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { > + .get_groups_count = pinctrl_scmi_get_groups_count, > + .get_group_name = pinctrl_scmi_get_group_name, > + .get_group_pins = pinctrl_scmi_get_group_pins, > + .pin_dbg_show = pinctrl_scmi_pin_dbg_show, > +#ifdef CONFIG_OF > + .dt_node_to_map = pinctrl_scmi_dt_node_to_map, > + .dt_free_map = pinctrl_scmi_dt_free_map, > +#endif > +}; > + > +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE); > +} > + > +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev, > + unsigned int selector) > +{ > + int ret; > + const char *name; > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return NULL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return NULL; > + > + ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name); > + if (ret) { > + dev_err(pmx->dev, "get name failed with err %d", ret); > + return NULL; > + } > + > + return name; > +} > + > +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const char * const **groups, > + unsigned int * const num_groups) > +{ > + const unsigned int *group_ids; > + int ret, i; > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !groups || !num_groups) > + return -EINVAL; > + > + if (selector < pmx->nr_functions && > + pmx->functions[selector].num_groups) { > + *groups = (const char * const *)pmx->functions[selector].groups; > + *num_groups = pmx->functions[selector].num_groups; > + return 0; > + } > + > + ret = pinctrl_ops->get_function_groups(pmx->ph, selector, > + &pmx->functions[selector].num_groups, > + &group_ids); > + if (ret) { > + dev_err(pmx->dev, "Unable to get function groups, err %d", ret); > + return ret; > + } > + > + *num_groups = pmx->functions[selector].num_groups; > + if (!*num_groups) > + return -EINVAL; > + > + pmx->functions[selector].groups = > + devm_kcalloc(pmx->dev, *num_groups, > + sizeof(*pmx->functions[selector].groups), > + GFP_KERNEL); > + if (!pmx->functions[selector].groups) > + return -ENOMEM; > + > + for (i = 0; i < *num_groups; i++) { > + pmx->functions[selector].groups[i] = > + pinctrl_scmi_get_group_name(pmx->pctldev, > + group_ids[i]); > + if (!pmx->functions[selector].groups[i]) { > + ret = -ENOMEM; > + goto error; > + } > + } > + > + *groups = (const char * const *)pmx->functions[selector].groups; > + > + return 0; > + > +error: > + kfree(pmx->functions[selector].groups); devm_kfree ? > + > + return ret; > +} > + > +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned int group) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->set_mux(pmx->ph, selector, group); > +} > + > +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, > + unsigned int offset) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->request_pin(pmx->ph, offset); > +} > + > +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset) > +{ > + struct scmi_pinctrl *pmx; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + return pinctrl_ops->free_pin(pmx->ph, offset); > +} > + > +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_pinconf_get(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx; > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type, > + (u32 *)&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; > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !configs || num_configs == 0) > + return -EINVAL; > + > + for (i = 0; i < num_configs; i++) { > + config_type = pinconf_to_config_param(configs[i]); > + config_value = pinconf_to_config_argument(configs[i]); > + > + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type, > + config_value); > + if (ret) { > + dev_err(pmx->dev, "Error parsing config %ld\n", > + configs[i]); > + break; > + } > + } > + > + 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; > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !configs || num_configs == 0) > + return -EINVAL; > + > + for (i = 0; i < num_configs; i++) { > + config_type = pinconf_to_config_param(configs[i]); > + config_value = pinconf_to_config_argument(configs[i]); > + > + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE, > + config_type, config_value); > + if (ret) { > + dev_err(pmx->dev, "Error parsing config = %ld", > + configs[i]); > + break; > + } > + } > + > + return ret; > +}; > + > +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx; > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, > + config_type, (u32 *)&config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + > +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = { > + .is_generic = true, > + .pin_config_get = pinctrl_scmi_pinconf_get, > + .pin_config_set = pinctrl_scmi_pinconf_set, > + .pin_config_group_set = pinctrl_scmi_pinconf_group_set, > + .pin_config_group_get = pinctrl_scmi_pinconf_group_get, > + .pin_config_config_dbg_show = pinconf_generic_dump_config, > +}; > + > +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, > + unsigned int *nr_pins, > + const struct pinctrl_pin_desc **pins) > +{ > + int ret, i; > + > + if (!pmx || !pmx->ph) > + return -EINVAL; > + > + if (!pins || !nr_pins) > + return -EINVAL; > + > + if (pmx->nr_pins) { > + *pins = pmx->pins; > + *nr_pins = pmx->nr_pins; > + return 0; > + } > + > + *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE); > + > + pmx->nr_pins = *nr_pins; > + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins), > + GFP_KERNEL); > + if (!pmx->pins) > + return -ENOMEM; > + > + for (i = 0; i < *nr_pins; i++) { > + pmx->pins[i].number = i; > + ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE, > + &pmx->pins[i].name); > + if (ret) { > + dev_err(pmx->dev, "Can't get name for pin %d: rc %d", > + i, ret); > + goto err; > + } > + } > + > + *pins = pmx->pins; > + dev_dbg(pmx->dev, "got pins %d", *nr_pins); > + > + return 0; > + err: > + kfree(pmx->pins); devm_kfree ? but anyway when this fails it will trigger the _probe to fail so all devres will be released..so not needed probably at the end. > + pmx->nr_pins = 0; > + > + return ret; > +} > + > +static const struct scmi_device_id scmi_id_table[] = { > + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(scmi, scmi_id_table); > + > +static int scmi_pinctrl_probe(struct scmi_device *sdev) > +{ > + int ret; > + struct scmi_pinctrl *pmx; > + const struct scmi_handle *handle; > + struct scmi_protocol_handle *ph; > + > + if (!sdev || !sdev->handle) > + return -EINVAL; > + > + handle = sdev->handle; > + > + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, > + &ph); > + if (IS_ERR(pinctrl_ops)) > + return PTR_ERR(pinctrl_ops); > + > + pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL); > + if (!pmx) > + return -ENOMEM; > + > + pmx->ph = ph; > + > + pmx->dev = &sdev->dev; > + pmx->pctl_desc.name = DRV_NAME; > + pmx->pctl_desc.owner = THIS_MODULE; > + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops; > + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops; > + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops; > + > + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins, > + &pmx->pctl_desc.pins); > + if (ret) > + goto clean; > + > + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx, > + &pmx->pctldev); > + if (ret) { > + dev_err(&sdev->dev, "could not register: %i\n", ret); > + goto clean; > + } > + > + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev); > + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev); > + > + if (pmx->nr_functions) { > + pmx->functions = > + devm_kcalloc(&sdev->dev, pmx->nr_functions, > + sizeof(*pmx->functions), > + GFP_KERNEL); > + if (!pmx->functions) { > + ret = -ENOMEM; > + goto clean; > + } > + } > + > + if (pmx->nr_groups) { > + pmx->groups = > + devm_kcalloc(&sdev->dev, pmx->nr_groups, > + sizeof(*pmx->groups), > + GFP_KERNEL); > + if (!pmx->groups) { > + ret = -ENOMEM; > + goto clean; > + } > + } > + > + return pinctrl_enable(pmx->pctldev); > + > +clean: > + if (pmx) { > + kfree(pmx->functions); > + kfree(pmx->groups); > + } > + > + kfree(pmx); All of these are devres allocated...it does not seem to me that they require explicit freeing on the _probe() failure path. (indeed you dont need even a .remove function) Thanks, Cristian
Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti: > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCP firmware, which does the changes in HW. > > This setup expects SCP firmware (or similar system, such as ATF) > to be installed on the platform, which implements pinctrl driver > for the specific platform. > > SCMI-Pinctrl driver should be configured from the device-tree and uses > generic device-tree mappings for the configuration. ... > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/of.h> I do not see any user of this header. Do you? > +#include <linux/module.h> > +#include <linux/seq_file.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 <linux/scmi_protocol.h> > +#include <linux/slab.h> Please, move these two to the upper group of the generic headers. > +struct scmi_pinctrl_funcs { > + unsigned int num_groups; > + const char **groups; > +}; Please, use struct pinfunction. ... > +struct scmi_pinctrl { > + struct scmi_pinctrl_funcs *functions; > + unsigned int nr_functions; > + char **groups; > + unsigned int nr_groups; I'm not sure what is the difference to what "functions" above represent. > +}; ... > +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, > + struct seq_file *s, > + unsigned int offset) > +{ > + seq_puts(s, DRV_NAME); > +} What is the usefulness of this method? ... > +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx; > + enum pin_config_param config_type; > + unsigned long config_value; > + > + if (!pctldev) > + return -EINVAL; > + > + pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!pmx || !pmx->ph || !config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, > + config_type, (u32 *)&config_value); Endianess issue. This is, while likely working code, still ugly. > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} ... > + err: err_free. > + kfree(pmx->pins); > + pmx->nr_pins = 0; > + > + return ret; ... > +static const struct scmi_device_id scmi_id_table[] = { > + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, > + { }, No comma for the terminator entry. > +}; ... > + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, > + &ph); Can be on one line. > + if (IS_ERR(pinctrl_ops)) > + return PTR_ERR(pinctrl_ops); ... > + if (pmx->nr_functions) { > + pmx->functions = > + devm_kcalloc(&sdev->dev, pmx->nr_functions, > + sizeof(*pmx->functions), > + GFP_KERNEL); > + if (!pmx->functions) { > + ret = -ENOMEM; > + goto clean; Interleaving devm_*() with non-devm_*() in such order is not a good idea. > + } > + } > + > + if (pmx->nr_groups) { > + pmx->groups = > + devm_kcalloc(&sdev->dev, pmx->nr_groups, > + sizeof(*pmx->groups), > + GFP_KERNEL); > + if (!pmx->groups) { > + ret = -ENOMEM; > + goto clean; > + } > + } > + > + return pinctrl_enable(pmx->pctldev); > + > +clean: err_free: > + if (pmx) { > + kfree(pmx->functions); > + kfree(pmx->groups); Ah, this is simply wrong. > + } > + > + kfree(pmx);
Hi Cristian, Please see below. On 05.05.23 23:01, Cristian Marussi wrote: > On Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev wrote: >> scmi-pinctrl driver implements pinctrl driver interface and using >> SCMI protocol to redirect messages from pinctrl subsystem SDK to >> SCP firmware, which does the changes in HW. >> >> This setup expects SCP firmware (or similar system, such as ATF) >> to be installed on the platform, which implements pinctrl driver >> for the specific platform. >> >> SCMI-Pinctrl driver should be configured from the device-tree and uses >> generic device-tree mappings for the configuration. >> > > Hi Oleksii, > > just a few remarks down below. > >> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> >> --- >> MAINTAINERS | 1 + >> drivers/pinctrl/Kconfig | 9 + >> drivers/pinctrl/Makefile | 1 + >> drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++ >> 4 files changed, 589 insertions(+) >> create mode 100644 drivers/pinctrl/pinctrl-scmi.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 0d251ebac437..ba9e3aea6176 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> >> L: linux-arm-kernel@lists.infradead.org >> S: Maintained >> F: drivers/firmware/arm_scmi/pinctrl.c >> +F: drivers/pinctrl/pinctrl-scmi.c >> >> SYSTEM RESET/SHUTDOWN DRIVERS >> M: Sebastian Reichel <sre@kernel.org> >> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >> index dcb53c4a9584..16bf2c67f095 100644 >> --- a/drivers/pinctrl/Kconfig >> +++ b/drivers/pinctrl/Kconfig >> @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig" >> source "drivers/pinctrl/visconti/Kconfig" >> source "drivers/pinctrl/vt8500/Kconfig" >> >> +config PINCTRL_SCMI >> + bool "Pinctrl driver controlled via SCMI interface" > > Why not a tristate ? The core SCMI stack can be compiled as module with > all the protocols...at least for testing is useful to have this driver > as M. > Yes, thanks. Will fix in v3. >> + 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. >> + >> endif >> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile >> index d5939840bb2a..21366db4f4f4 100644 >> --- a/drivers/pinctrl/Makefile >> +++ b/drivers/pinctrl/Makefile >> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o >> obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o >> obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o >> obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o >> +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o >> >> obj-y += actions/ >> obj-$(CONFIG_ARCH_ASPEED) += aspeed/ >> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c >> new file mode 100644 >> index 000000000000..8401db1d030b >> --- /dev/null >> +++ b/drivers/pinctrl/pinctrl-scmi.c >> @@ -0,0 +1,578 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver >> + * >> + * Copyright (C) 2023 EPAM >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/of.h> >> +#include <linux/module.h> >> +#include <linux/seq_file.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 <linux/scmi_protocol.h> >> +#include <linux/slab.h> >> + >> +#include "pinctrl-utils.h" >> +#include "core.h" >> +#include "pinconf.h" >> + >> +#define DRV_NAME "scmi-pinctrl" >> + >> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops; >> + >> +struct scmi_pinctrl_funcs { >> + unsigned int num_groups; >> + const char **groups; >> +}; >> + >> +struct scmi_pinctrl { >> + struct device *dev; >> + struct scmi_protocol_handle *ph; >> + struct pinctrl_dev *pctldev; >> + struct pinctrl_desc pctl_desc; >> + struct scmi_pinctrl_funcs *functions; >> + unsigned int nr_functions; >> + char **groups; >> + unsigned int nr_groups; >> + struct pinctrl_pin_desc *pins; >> + unsigned int nr_pins; >> +}; >> + >> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE); >> +} >> + >> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, >> + unsigned int selector) >> +{ >> + int ret; >> + const char *name; >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return NULL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return NULL; >> + >> + ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name); >> + if (ret) { >> + dev_err(pmx->dev, "get name failed with err %d", ret); >> + return NULL; >> + } >> + >> + return name; >> +} >> + >> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, >> + unsigned int selector, >> + const unsigned int **pins, >> + unsigned int *num_pins) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->get_group_pins(pmx->ph, selector, >> + pins, num_pins); >> +} >> + >> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, >> + struct seq_file *s, >> + unsigned int offset) >> +{ >> + seq_puts(s, DRV_NAME); >> +} >> + >> +#ifdef CONFIG_OF >> +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, >> + struct device_node *np_config, >> + struct pinctrl_map **map, >> + u32 *num_maps) >> +{ >> + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, >> + num_maps, PIN_MAP_TYPE_INVALID); >> +} >> + >> +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, >> + struct pinctrl_map *map, u32 num_maps) >> +{ >> + kfree(map); >> +} >> + >> +#endif /* CONFIG_OF */ >> + >> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { >> + .get_groups_count = pinctrl_scmi_get_groups_count, >> + .get_group_name = pinctrl_scmi_get_group_name, >> + .get_group_pins = pinctrl_scmi_get_group_pins, >> + .pin_dbg_show = pinctrl_scmi_pin_dbg_show, >> +#ifdef CONFIG_OF >> + .dt_node_to_map = pinctrl_scmi_dt_node_to_map, >> + .dt_free_map = pinctrl_scmi_dt_free_map, >> +#endif >> +}; >> + >> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE); >> +} >> + >> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev, >> + unsigned int selector) >> +{ >> + int ret; >> + const char *name; >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return NULL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return NULL; >> + >> + ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name); >> + if (ret) { >> + dev_err(pmx->dev, "get name failed with err %d", ret); >> + return NULL; >> + } >> + >> + return name; >> +} >> + >> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev, >> + unsigned int selector, >> + const char * const **groups, >> + unsigned int * const num_groups) >> +{ >> + const unsigned int *group_ids; >> + int ret, i; >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !groups || !num_groups) >> + return -EINVAL; >> + >> + if (selector < pmx->nr_functions && >> + pmx->functions[selector].num_groups) { >> + *groups = (const char * const *)pmx->functions[selector].groups; >> + *num_groups = pmx->functions[selector].num_groups; >> + return 0; >> + } >> + >> + ret = pinctrl_ops->get_function_groups(pmx->ph, selector, >> + &pmx->functions[selector].num_groups, >> + &group_ids); >> + if (ret) { >> + dev_err(pmx->dev, "Unable to get function groups, err %d", ret); >> + return ret; >> + } >> + >> + *num_groups = pmx->functions[selector].num_groups; >> + if (!*num_groups) >> + return -EINVAL; >> + >> + pmx->functions[selector].groups = >> + devm_kcalloc(pmx->dev, *num_groups, >> + sizeof(*pmx->functions[selector].groups), >> + GFP_KERNEL); >> + if (!pmx->functions[selector].groups) >> + return -ENOMEM; >> + >> + for (i = 0; i < *num_groups; i++) { >> + pmx->functions[selector].groups[i] = >> + pinctrl_scmi_get_group_name(pmx->pctldev, >> + group_ids[i]); >> + if (!pmx->functions[selector].groups[i]) { >> + ret = -ENOMEM; >> + goto error; >> + } >> + } >> + >> + *groups = (const char * const *)pmx->functions[selector].groups; >> + >> + return 0; >> + >> +error: >> + kfree(pmx->functions[selector].groups); > > devm_kfree ? > Thanks, fixed. >> + >> + return ret; >> +} >> + >> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, >> + unsigned int selector, unsigned int group) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->set_mux(pmx->ph, selector, group); >> +} >> + >> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, >> + unsigned int offset) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->request_pin(pmx->ph, offset); >> +} >> + >> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset) >> +{ >> + struct scmi_pinctrl *pmx; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + return pinctrl_ops->free_pin(pmx->ph, offset); >> +} >> + >> +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_pinconf_get(struct pinctrl_dev *pctldev, >> + unsigned int _pin, >> + unsigned long *config) >> +{ >> + int ret; >> + struct scmi_pinctrl *pmx; >> + enum pin_config_param config_type; >> + unsigned long config_value; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !config) >> + return -EINVAL; >> + >> + config_type = pinconf_to_config_param(*config); >> + >> + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type, >> + (u32 *)&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; >> + enum pin_config_param config_type; >> + unsigned long config_value; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !configs || num_configs == 0) >> + return -EINVAL; >> + >> + for (i = 0; i < num_configs; i++) { >> + config_type = pinconf_to_config_param(configs[i]); >> + config_value = pinconf_to_config_argument(configs[i]); >> + >> + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type, >> + config_value); >> + if (ret) { >> + dev_err(pmx->dev, "Error parsing config %ld\n", >> + configs[i]); >> + break; >> + } >> + } >> + >> + 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; >> + enum pin_config_param config_type; >> + unsigned long config_value; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !configs || num_configs == 0) >> + return -EINVAL; >> + >> + for (i = 0; i < num_configs; i++) { >> + config_type = pinconf_to_config_param(configs[i]); >> + config_value = pinconf_to_config_argument(configs[i]); >> + >> + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE, >> + config_type, config_value); >> + if (ret) { >> + dev_err(pmx->dev, "Error parsing config = %ld", >> + configs[i]); >> + break; >> + } >> + } >> + >> + return ret; >> +}; >> + >> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, >> + unsigned int _pin, >> + unsigned long *config) >> +{ >> + int ret; >> + struct scmi_pinctrl *pmx; >> + enum pin_config_param config_type; >> + unsigned long config_value; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !config) >> + return -EINVAL; >> + >> + config_type = pinconf_to_config_param(*config); >> + >> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, >> + config_type, (u32 *)&config_value); >> + if (ret) >> + return ret; >> + >> + *config = pinconf_to_config_packed(config_type, config_value); >> + >> + return 0; >> +} >> + >> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = { >> + .is_generic = true, >> + .pin_config_get = pinctrl_scmi_pinconf_get, >> + .pin_config_set = pinctrl_scmi_pinconf_set, >> + .pin_config_group_set = pinctrl_scmi_pinconf_group_set, >> + .pin_config_group_get = pinctrl_scmi_pinconf_group_get, >> + .pin_config_config_dbg_show = pinconf_generic_dump_config, >> +}; >> + >> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, >> + unsigned int *nr_pins, >> + const struct pinctrl_pin_desc **pins) >> +{ >> + int ret, i; >> + >> + if (!pmx || !pmx->ph) >> + return -EINVAL; >> + >> + if (!pins || !nr_pins) >> + return -EINVAL; >> + >> + if (pmx->nr_pins) { >> + *pins = pmx->pins; >> + *nr_pins = pmx->nr_pins; >> + return 0; >> + } >> + >> + *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE); >> + >> + pmx->nr_pins = *nr_pins; >> + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins), >> + GFP_KERNEL); >> + if (!pmx->pins) >> + return -ENOMEM; >> + >> + for (i = 0; i < *nr_pins; i++) { >> + pmx->pins[i].number = i; >> + ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE, >> + &pmx->pins[i].name); >> + if (ret) { >> + dev_err(pmx->dev, "Can't get name for pin %d: rc %d", >> + i, ret); >> + goto err; >> + } >> + } >> + >> + *pins = pmx->pins; >> + dev_dbg(pmx->dev, "got pins %d", *nr_pins); >> + >> + return 0; >> + err: >> + kfree(pmx->pins); > devm_kfree ? but anyway when this fails it will trigger the _probe to > fail so all devres will be released..so not needed probably at the end. > >> + pmx->nr_pins = 0; >> + >> + return ret; >> +} >> + >> +static const struct scmi_device_id scmi_id_table[] = { >> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(scmi, scmi_id_table); >> + >> +static int scmi_pinctrl_probe(struct scmi_device *sdev) >> +{ >> + int ret; >> + struct scmi_pinctrl *pmx; >> + const struct scmi_handle *handle; >> + struct scmi_protocol_handle *ph; >> + >> + if (!sdev || !sdev->handle) >> + return -EINVAL; >> + >> + handle = sdev->handle; >> + >> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, >> + &ph); >> + if (IS_ERR(pinctrl_ops)) >> + return PTR_ERR(pinctrl_ops); >> + >> + pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL); >> + if (!pmx) >> + return -ENOMEM; >> + >> + pmx->ph = ph; >> + >> + pmx->dev = &sdev->dev; >> + pmx->pctl_desc.name = DRV_NAME; >> + pmx->pctl_desc.owner = THIS_MODULE; >> + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops; >> + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops; >> + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops; >> + >> + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins, >> + &pmx->pctl_desc.pins); >> + if (ret) >> + goto clean; >> + >> + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx, >> + &pmx->pctldev); >> + if (ret) { >> + dev_err(&sdev->dev, "could not register: %i\n", ret); >> + goto clean; >> + } >> + >> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev); >> + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev); >> + >> + if (pmx->nr_functions) { >> + pmx->functions = >> + devm_kcalloc(&sdev->dev, pmx->nr_functions, >> + sizeof(*pmx->functions), >> + GFP_KERNEL); >> + if (!pmx->functions) { >> + ret = -ENOMEM; >> + goto clean; >> + } >> + } >> + >> + if (pmx->nr_groups) { >> + pmx->groups = >> + devm_kcalloc(&sdev->dev, pmx->nr_groups, >> + sizeof(*pmx->groups), >> + GFP_KERNEL); >> + if (!pmx->groups) { >> + ret = -ENOMEM; >> + goto clean; >> + } >> + } >> + >> + return pinctrl_enable(pmx->pctldev); >> + >> +clean: >> + if (pmx) { >> + kfree(pmx->functions); >> + kfree(pmx->groups); >> + } >> + >> + kfree(pmx); > > All of these are devres allocated...it does not seem to me that they > require explicit freeing on the _probe() failure path. > (indeed you dont need even a .remove function) > Removed this block. > Thanks, > Cristian
Hello Andy, On 05.05.23 23:35, andy.shevchenko@gmail.com wrote: > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti: >> scmi-pinctrl driver implements pinctrl driver interface and using >> SCMI protocol to redirect messages from pinctrl subsystem SDK to >> SCP firmware, which does the changes in HW. >> >> This setup expects SCP firmware (or similar system, such as ATF) >> to be installed on the platform, which implements pinctrl driver >> for the specific platform. >> >> SCMI-Pinctrl driver should be configured from the device-tree and uses >> generic device-tree mappings for the configuration. > > ... > >> +#include <linux/device.h> >> +#include <linux/err.h> > >> +#include <linux/of.h> > > I do not see any user of this header. Do you? > Yes, thanks. Removing >> +#include <linux/module.h> >> +#include <linux/seq_file.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 <linux/scmi_protocol.h> >> +#include <linux/slab.h> > > Please, move these two to the upper group of the generic headers. > Thanks, fixed. >> +struct scmi_pinctrl_funcs { >> + unsigned int num_groups; >> + const char **groups; >> +}; > > Please, use struct pinfunction. > I can't use pincfunction here because it has the following groups definition: const char * const *groups; Which is meant to be constantly allocated. So I when I try to gather list of groups in pinctrl_scmi_get_function_groups I will receive compilation error. > ... > >> +struct scmi_pinctrl { > >> + struct scmi_pinctrl_funcs *functions; >> + unsigned int nr_functions; > >> + char **groups; >> + unsigned int nr_groups; > > I'm not sure what is the difference to what "functions" above represent. > The difference is that each function has a list of group names, so we have to get name for each group in this function. I'm saving array to avoid extra SCMI calls to gather groups in function. >> +}; > > ... > >> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, >> + struct seq_file *s, >> + unsigned int offset) >> +{ >> + seq_puts(s, DRV_NAME); >> +} > > What is the usefulness of this method? > Removed > ... > >> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, >> + unsigned int _pin, >> + unsigned long *config) >> +{ >> + int ret; >> + struct scmi_pinctrl *pmx; >> + enum pin_config_param config_type; >> + unsigned long config_value; >> + >> + if (!pctldev) >> + return -EINVAL; >> + >> + pmx = pinctrl_dev_get_drvdata(pctldev); >> + >> + if (!pmx || !pmx->ph || !config) >> + return -EINVAL; >> + >> + config_type = pinconf_to_config_param(*config); >> + >> + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, >> + config_type, (u32 *)&config_value); > > Endianess issue. This is, while likely working code, still ugly. > Fixed. >> + if (ret) >> + return ret; >> + >> + *config = pinconf_to_config_packed(config_type, config_value); >> + >> + return 0; >> +} > > ... > >> + err: > > err_free. > Fixed >> + kfree(pmx->pins); >> + pmx->nr_pins = 0; >> + >> + return ret; > > ... > >> +static const struct scmi_device_id scmi_id_table[] = { >> + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, > >> + { }, > > No comma for the terminator entry. > Removed. >> +}; > > ... > >> + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, >> + &ph); > > Can be on one line. Fixed > >> + if (IS_ERR(pinctrl_ops)) >> + return PTR_ERR(pinctrl_ops); > > ... > >> + if (pmx->nr_functions) { >> + pmx->functions = >> + devm_kcalloc(&sdev->dev, pmx->nr_functions, >> + sizeof(*pmx->functions), >> + GFP_KERNEL); >> + if (!pmx->functions) { >> + ret = -ENOMEM; >> + goto clean; > > Interleaving devm_*() with non-devm_*() in such order is not a good idea. > Thanks, fixed. >> + } >> + } >> + >> + if (pmx->nr_groups) { >> + pmx->groups = >> + devm_kcalloc(&sdev->dev, pmx->nr_groups, >> + sizeof(*pmx->groups), >> + GFP_KERNEL); >> + if (!pmx->groups) { >> + ret = -ENOMEM; >> + goto clean; >> + } >> + } >> + >> + return pinctrl_enable(pmx->pctldev); >> + >> +clean: > > err_free: removed. > >> + if (pmx) { >> + kfree(pmx->functions); >> + kfree(pmx->groups); > > Ah, this is simply wrong. > Thanks, removed. >> + } >> + >> + kfree(pmx); >
On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote: > Hello Andy, > > On 05.05.23 23:35, andy.shevchenko@gmail.com wrote: > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti: > >> scmi-pinctrl driver implements pinctrl driver interface and using > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to > >> SCP firmware, which does the changes in HW. > >> > >> This setup expects SCP firmware (or similar system, such as ATF) > >> to be installed on the platform, which implements pinctrl driver > >> for the specific platform. > >> > >> SCMI-Pinctrl driver should be configured from the device-tree and uses > >> generic device-tree mappings for the configuration. > > > > ... > > > >> +#include <linux/device.h> > >> +#include <linux/err.h> > > > >> +#include <linux/of.h> > > > > I do not see any user of this header. Do you? > > > Yes, thanks. Removing > > >> +#include <linux/module.h> > >> +#include <linux/seq_file.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 <linux/scmi_protocol.h> > >> +#include <linux/slab.h> > > > > Please, move these two to the upper group of the generic headers. > > > Thanks, fixed. > > >> +struct scmi_pinctrl_funcs { > >> + unsigned int num_groups; > >> + const char **groups; > >> +}; > > > > Please, use struct pinfunction. > > > I can't use pincfunction here because it has the following groups > definition: > const char * const *groups; > > Which is meant to be constantly allocated. > So I when I try to gather list of groups in > pinctrl_scmi_get_function_groups I will receive compilation error. > Maybe this is a further signal that we should re-evaluate the benefits of the lazy allocations you now perform during protocol initialization instead of querying and allocating statically all the info structs about existing resources. Not saying that is necessarily bad, I understood your points about reducing the number of SCMI queries during boot and let pinctrl subsystem trigger only the strictly needed one, just saying maybe good to reason a bit more about this once V3 is posted. (i.e. I could bother you more :P ..) Thanks, Cristian P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere to include proper maintainers (and their bots)
Hello Cristian, On Fri, May 12, 2023 at 10:04:41AM +0100, Cristian Marussi wrote: > On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote: > > Hello Andy, > > > > On 05.05.23 23:35, andy.shevchenko@gmail.com wrote: > > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti: > > >> scmi-pinctrl driver implements pinctrl driver interface and using > > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to > > >> SCP firmware, which does the changes in HW. > > >> > > >> This setup expects SCP firmware (or similar system, such as ATF) > > >> to be installed on the platform, which implements pinctrl driver > > >> for the specific platform. > > >> > > >> SCMI-Pinctrl driver should be configured from the device-tree and uses > > >> generic device-tree mappings for the configuration. > > > > > > ... > > > > > >> +#include <linux/device.h> > > >> +#include <linux/err.h> > > > > > >> +#include <linux/of.h> > > > > > > I do not see any user of this header. Do you? > > > > > Yes, thanks. Removing > > > > >> +#include <linux/module.h> > > >> +#include <linux/seq_file.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 <linux/scmi_protocol.h> > > >> +#include <linux/slab.h> > > > > > > Please, move these two to the upper group of the generic headers. > > > > > Thanks, fixed. > > > > >> +struct scmi_pinctrl_funcs { > > >> + unsigned int num_groups; > > >> + const char **groups; > > >> +}; > > > > > > Please, use struct pinfunction. > > > > > I can't use pincfunction here because it has the following groups > > definition: > > const char * const *groups; > > > > Which is meant to be constantly allocated. > > So I when I try to gather list of groups in > > pinctrl_scmi_get_function_groups I will receive compilation error. > > > > Maybe this is a further signal that we should re-evaluate the benefits of > the lazy allocations you now perform during protocol initialization > instead of querying and allocating statically all the info structs about > existing resources. > > Not saying that is necessarily bad, I understood your points about reducing > the number of SCMI queries during boot and let pinctrl subsystem trigger only > the strictly needed one, just saying maybe good to reason a bit more about this > once V3 is posted. (i.e. I could bother you more :P ..) > > Thanks, > Cristian > > P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere > to include proper maintainers (and their bots) That's a good point to think about. Actually, functions are the only thing that should be cached on pinctrl side. And we need it specifically because groups in each function are presented by names, not selectors. Maybe It's better to move this caching to pinctrl scmi driver. But, from the other side - storing group names for each function is Linux Kernel specific implementation and we probably don't want to add some specific case to the Generic protocol driver. I think I would leave it as in V3 so we can continue discussion. Oleksii.
On Fri, May 12, 2023 at 12:18:03PM +0000, Oleksii Moisieiev wrote: > Hello Cristian, > > On Fri, May 12, 2023 at 10:04:41AM +0100, Cristian Marussi wrote: > > On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote: > > > Hello Andy, > > > > > > On 05.05.23 23:35, andy.shevchenko@gmail.com wrote: > > > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti: > > > >> scmi-pinctrl driver implements pinctrl driver interface and using > > > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to > > > >> SCP firmware, which does the changes in HW. > > > >> > > > >> This setup expects SCP firmware (or similar system, such as ATF) > > > >> to be installed on the platform, which implements pinctrl driver > > > >> for the specific platform. > > > >> > > > >> SCMI-Pinctrl driver should be configured from the device-tree and uses > > > >> generic device-tree mappings for the configuration. > > > > > > > > ... > > > > > > > >> +#include <linux/device.h> > > > >> +#include <linux/err.h> > > > > > > > >> +#include <linux/of.h> > > > > > > > > I do not see any user of this header. Do you? > > > > > > > Yes, thanks. Removing > > > > > > >> +#include <linux/module.h> > > > >> +#include <linux/seq_file.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 <linux/scmi_protocol.h> > > > >> +#include <linux/slab.h> > > > > > > > > Please, move these two to the upper group of the generic headers. > > > > > > > Thanks, fixed. > > > > > > >> +struct scmi_pinctrl_funcs { > > > >> + unsigned int num_groups; > > > >> + const char **groups; > > > >> +}; > > > > > > > > Please, use struct pinfunction. > > > > > > > I can't use pincfunction here because it has the following groups > > > definition: > > > const char * const *groups; > > > > > > Which is meant to be constantly allocated. > > > So I when I try to gather list of groups in > > > pinctrl_scmi_get_function_groups I will receive compilation error. > > > > > > > Maybe this is a further signal that we should re-evaluate the benefits of > > the lazy allocations you now perform during protocol initialization > > instead of querying and allocating statically all the info structs about > > existing resources. > > > > Not saying that is necessarily bad, I understood your points about reducing > > the number of SCMI queries during boot and let pinctrl subsystem trigger only > > the strictly needed one, just saying maybe good to reason a bit more about this > > once V3 is posted. (i.e. I could bother you more :P ..) > > > > Thanks, > > Cristian > > > > P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere > > to include proper maintainers (and their bots) > > That's a good point to think about. Actually, functions are the only > thing that should be cached on pinctrl side. And we need it specifically > because groups in each function are presented by names, not selectors. > Maybe It's better to move this caching to pinctrl scmi driver. But, from > the other side - storing group names for each function is Linux Kernel > specific implementation and we probably don't want to add some specific > case to the Generic protocol driver. > > I think I would leave it as in V3 so we can continue discussion. > Sure, let's review/rediscuss this on top V3. Thanks, Cristian
diff --git a/MAINTAINERS b/MAINTAINERS index 0d251ebac437..ba9e3aea6176 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20322,6 +20322,7 @@ M: Oleksii Moisieiev <oleksii_moisieiev@epam.com> L: linux-arm-kernel@lists.infradead.org S: Maintained F: drivers/firmware/arm_scmi/pinctrl.c +F: drivers/pinctrl/pinctrl-scmi.c SYSTEM RESET/SHUTDOWN DRIVERS M: Sebastian Reichel <sre@kernel.org> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index dcb53c4a9584..16bf2c67f095 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig" source "drivers/pinctrl/visconti/Kconfig" source "drivers/pinctrl/vt8500/Kconfig" +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. + endif diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index d5939840bb2a..21366db4f4f4 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o obj-y += actions/ obj-$(CONFIG_ARCH_ASPEED) += aspeed/ diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c new file mode 100644 index 000000000000..8401db1d030b --- /dev/null +++ b/drivers/pinctrl/pinctrl-scmi.c @@ -0,0 +1,578 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * System Control and Power Interface (SCMI) Protocol based pinctrl driver + * + * Copyright (C) 2023 EPAM + */ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/module.h> +#include <linux/seq_file.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 <linux/scmi_protocol.h> +#include <linux/slab.h> + +#include "pinctrl-utils.h" +#include "core.h" +#include "pinconf.h" + +#define DRV_NAME "scmi-pinctrl" + +static const struct scmi_pinctrl_proto_ops *pinctrl_ops; + +struct scmi_pinctrl_funcs { + unsigned int num_groups; + const char **groups; +}; + +struct scmi_pinctrl { + struct device *dev; + struct scmi_protocol_handle *ph; + struct pinctrl_dev *pctldev; + struct pinctrl_desc pctl_desc; + struct scmi_pinctrl_funcs *functions; + unsigned int nr_functions; + char **groups; + unsigned int nr_groups; + struct pinctrl_pin_desc *pins; + unsigned int nr_pins; +}; + +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE); +} + +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + int ret; + const char *name; + struct scmi_pinctrl *pmx; + + if (!pctldev) + return NULL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return NULL; + + ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name); + if (ret) { + dev_err(pmx->dev, "get name failed with err %d", ret); + return NULL; + } + + return name; +} + +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, + unsigned int selector, + const unsigned int **pins, + unsigned int *num_pins) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->get_group_pins(pmx->ph, selector, + pins, num_pins); +} + +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev, + struct seq_file *s, + unsigned int offset) +{ + seq_puts(s, DRV_NAME); +} + +#ifdef CONFIG_OF +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np_config, + struct pinctrl_map **map, + u32 *num_maps) +{ + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, + num_maps, PIN_MAP_TYPE_INVALID); +} + +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, + struct pinctrl_map *map, u32 num_maps) +{ + kfree(map); +} + +#endif /* CONFIG_OF */ + +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { + .get_groups_count = pinctrl_scmi_get_groups_count, + .get_group_name = pinctrl_scmi_get_group_name, + .get_group_pins = pinctrl_scmi_get_group_pins, + .pin_dbg_show = pinctrl_scmi_pin_dbg_show, +#ifdef CONFIG_OF + .dt_node_to_map = pinctrl_scmi_dt_node_to_map, + .dt_free_map = pinctrl_scmi_dt_free_map, +#endif +}; + +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE); +} + +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + int ret; + const char *name; + struct scmi_pinctrl *pmx; + + if (!pctldev) + return NULL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return NULL; + + ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name); + if (ret) { + dev_err(pmx->dev, "get name failed with err %d", ret); + return NULL; + } + + return name; +} + +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev, + unsigned int selector, + const char * const **groups, + unsigned int * const num_groups) +{ + const unsigned int *group_ids; + int ret, i; + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !groups || !num_groups) + return -EINVAL; + + if (selector < pmx->nr_functions && + pmx->functions[selector].num_groups) { + *groups = (const char * const *)pmx->functions[selector].groups; + *num_groups = pmx->functions[selector].num_groups; + return 0; + } + + ret = pinctrl_ops->get_function_groups(pmx->ph, selector, + &pmx->functions[selector].num_groups, + &group_ids); + if (ret) { + dev_err(pmx->dev, "Unable to get function groups, err %d", ret); + return ret; + } + + *num_groups = pmx->functions[selector].num_groups; + if (!*num_groups) + return -EINVAL; + + pmx->functions[selector].groups = + devm_kcalloc(pmx->dev, *num_groups, + sizeof(*pmx->functions[selector].groups), + GFP_KERNEL); + if (!pmx->functions[selector].groups) + return -ENOMEM; + + for (i = 0; i < *num_groups; i++) { + pmx->functions[selector].groups[i] = + pinctrl_scmi_get_group_name(pmx->pctldev, + group_ids[i]); + if (!pmx->functions[selector].groups[i]) { + ret = -ENOMEM; + goto error; + } + } + + *groups = (const char * const *)pmx->functions[selector].groups; + + return 0; + +error: + kfree(pmx->functions[selector].groups); + + return ret; +} + +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev, + unsigned int selector, unsigned int group) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->set_mux(pmx->ph, selector, group); +} + +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev, + unsigned int offset) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->request_pin(pmx->ph, offset); +} + +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset) +{ + struct scmi_pinctrl *pmx; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph) + return -EINVAL; + + return pinctrl_ops->free_pin(pmx->ph, offset); +} + +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_pinconf_get(struct pinctrl_dev *pctldev, + unsigned int _pin, + unsigned long *config) +{ + int ret; + struct scmi_pinctrl *pmx; + enum pin_config_param config_type; + unsigned long config_value; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !config) + return -EINVAL; + + config_type = pinconf_to_config_param(*config); + + ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type, + (u32 *)&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; + enum pin_config_param config_type; + unsigned long config_value; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !configs || num_configs == 0) + return -EINVAL; + + for (i = 0; i < num_configs; i++) { + config_type = pinconf_to_config_param(configs[i]); + config_value = pinconf_to_config_argument(configs[i]); + + ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type, + config_value); + if (ret) { + dev_err(pmx->dev, "Error parsing config %ld\n", + configs[i]); + break; + } + } + + 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; + enum pin_config_param config_type; + unsigned long config_value; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !configs || num_configs == 0) + return -EINVAL; + + for (i = 0; i < num_configs; i++) { + config_type = pinconf_to_config_param(configs[i]); + config_value = pinconf_to_config_argument(configs[i]); + + ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE, + config_type, config_value); + if (ret) { + dev_err(pmx->dev, "Error parsing config = %ld", + configs[i]); + break; + } + } + + return ret; +}; + +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, + unsigned int _pin, + unsigned long *config) +{ + int ret; + struct scmi_pinctrl *pmx; + enum pin_config_param config_type; + unsigned long config_value; + + if (!pctldev) + return -EINVAL; + + pmx = pinctrl_dev_get_drvdata(pctldev); + + if (!pmx || !pmx->ph || !config) + return -EINVAL; + + config_type = pinconf_to_config_param(*config); + + ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE, + config_type, (u32 *)&config_value); + if (ret) + return ret; + + *config = pinconf_to_config_packed(config_type, config_value); + + return 0; +} + +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = { + .is_generic = true, + .pin_config_get = pinctrl_scmi_pinconf_get, + .pin_config_set = pinctrl_scmi_pinconf_set, + .pin_config_group_set = pinctrl_scmi_pinconf_group_set, + .pin_config_group_get = pinctrl_scmi_pinconf_group_get, + .pin_config_config_dbg_show = pinconf_generic_dump_config, +}; + +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, + unsigned int *nr_pins, + const struct pinctrl_pin_desc **pins) +{ + int ret, i; + + if (!pmx || !pmx->ph) + return -EINVAL; + + if (!pins || !nr_pins) + return -EINVAL; + + if (pmx->nr_pins) { + *pins = pmx->pins; + *nr_pins = pmx->nr_pins; + return 0; + } + + *nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE); + + pmx->nr_pins = *nr_pins; + pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins), + GFP_KERNEL); + if (!pmx->pins) + return -ENOMEM; + + for (i = 0; i < *nr_pins; i++) { + pmx->pins[i].number = i; + ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE, + &pmx->pins[i].name); + if (ret) { + dev_err(pmx->dev, "Can't get name for pin %d: rc %d", + i, ret); + goto err; + } + } + + *pins = pmx->pins; + dev_dbg(pmx->dev, "got pins %d", *nr_pins); + + return 0; + err: + kfree(pmx->pins); + pmx->nr_pins = 0; + + return ret; +} + +static const struct scmi_device_id scmi_id_table[] = { + { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, + { }, +}; +MODULE_DEVICE_TABLE(scmi, scmi_id_table); + +static int scmi_pinctrl_probe(struct scmi_device *sdev) +{ + int ret; + struct scmi_pinctrl *pmx; + const struct scmi_handle *handle; + struct scmi_protocol_handle *ph; + + if (!sdev || !sdev->handle) + return -EINVAL; + + handle = sdev->handle; + + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, + &ph); + if (IS_ERR(pinctrl_ops)) + return PTR_ERR(pinctrl_ops); + + pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL); + if (!pmx) + return -ENOMEM; + + pmx->ph = ph; + + pmx->dev = &sdev->dev; + pmx->pctl_desc.name = DRV_NAME; + pmx->pctl_desc.owner = THIS_MODULE; + pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops; + pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops; + pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops; + + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins, + &pmx->pctl_desc.pins); + if (ret) + goto clean; + + ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx, + &pmx->pctldev); + if (ret) { + dev_err(&sdev->dev, "could not register: %i\n", ret); + goto clean; + } + + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev); + pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev); + + if (pmx->nr_functions) { + pmx->functions = + devm_kcalloc(&sdev->dev, pmx->nr_functions, + sizeof(*pmx->functions), + GFP_KERNEL); + if (!pmx->functions) { + ret = -ENOMEM; + goto clean; + } + } + + if (pmx->nr_groups) { + pmx->groups = + devm_kcalloc(&sdev->dev, pmx->nr_groups, + sizeof(*pmx->groups), + GFP_KERNEL); + if (!pmx->groups) { + ret = -ENOMEM; + goto clean; + } + } + + return pinctrl_enable(pmx->pctldev); + +clean: + if (pmx) { + kfree(pmx->functions); + kfree(pmx->groups); + } + + kfree(pmx); + + return ret; +} + +static struct scmi_driver scmi_pinctrl_driver = { + .name = DRV_NAME, + .probe = scmi_pinctrl_probe, + .id_table = scmi_id_table, +}; +module_scmi_driver(scmi_pinctrl_driver); + +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>"); +MODULE_DESCRIPTION("ARM SCMI pin controller driver"); +MODULE_LICENSE("GPL");
scmi-pinctrl driver implements pinctrl driver interface and using SCMI protocol to redirect messages from pinctrl subsystem SDK to SCP firmware, which does the changes in HW. This setup expects SCP firmware (or similar system, such as ATF) to be installed on the platform, which implements pinctrl driver for the specific platform. SCMI-Pinctrl driver should be configured from the device-tree and uses generic device-tree mappings for the configuration. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> --- MAINTAINERS | 1 + drivers/pinctrl/Kconfig | 9 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++ 4 files changed, 589 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-scmi.c