| Message ID | 20231215-pinctrl-scmi-v1-7-0fe35e4611f7@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support | expand |
On Fri, Dec 15, 2023 at 07:56:35PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > i.MX95 System Manager FW supports SCMI PINCTRL protocol, but uses > OEM Pin Configuration type, so need i.MX specific dt_node_to_map. > This does not even compile for me, as of now, when configuring the Pinctrl SCMI driver as a module with your IMX custom additions. (I think the Makefile with the additional pinctrl-imx is wrong in how describes the objects composing the pinctrl-scmi module with IMX addons...) ERROR: modpost: "pinctrl_scmi_imx_dt_node_to_map" [drivers/pinctrl/pinctrl-scmi.ko] undefined! make[3]: *** [dev/src/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1 make[2]: *** [dev/src/linux/Makefile:1863: modpost] Error 2 make[1]: *** [dev/src/linux/Makefile:234: __sub-make] Error 2 make[1]: Leaving directory dev/out_linux make: *** [Makefile:234: __sub-make] Error 2 More in general, I think that this NXP OEM specific additions, which are in general welcome (and indeed as you know part of the spec was modified to allow for OEM specific needs), do NOT belong to this generic SCMI Pinctrl driver, because the driver from Oleksii/EPAM was born as a generic SCMI driver and it fits perfectly with the Generic Pinctrl Linux subsystem and related generic bindings parsing: now with this you are trying to stick a custom OEM slight varied behaviour (and related binding) on top of a generic thing. And this choice leads to a number of additional changes in the SCMI core to support an even more complex handling of SCMI devices, which is already too complex IMO.. IOW...I dont think that the whole idea of the per-protocol optional compatible to be able to select slightly different behaviours/parsing would have a great chance to fly sincerely... I know there is an issue with having a completely distinct SCMI IMX pinctrl driver that uses the same protocol node @19 (without the need for the compatible trick) due to the way in which the Pinctrl subsystem searches for devices (by of_node)...I'll think about an alternative way to allow this but I am not sure (as you saw) that would be so easily doable... Also, I am wondering if this is really a problem in reality since I would NOT expect you to load/ship both the OEM/NXP custom specific SCMI pinctrl driver AND the generic one on the same platform (after having made them distinct I mean...) am I wrong ? (so you could even made them exclude each other at compile time...far from being the best option I agree...) Thanks, Cristian
> Subject: Re: [PATCH 7/7] pinctrl: scmi: implement > pinctrl_scmi_imx_dt_node_to_map > > On Fri, Dec 15, 2023 at 07:56:35PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > i.MX95 System Manager FW supports SCMI PINCTRL protocol, but uses > OEM > > Pin Configuration type, so need i.MX specific dt_node_to_map. > > > > This does not even compile for me, as of now, when configuring the Pinctrl > SCMI driver as a module with your IMX custom additions. (I think the > Makefile with the additional pinctrl-imx is wrong in how describes the objects > composing the pinctrl-scmi module with IMX addons...) > > ERROR: modpost: "pinctrl_scmi_imx_dt_node_to_map" > [drivers/pinctrl/pinctrl-scmi.ko] undefined! > make[3]: *** [dev/src/linux/scripts/Makefile.modpost:145: Module.symvers] > Error 1 > make[2]: *** [dev/src/linux/Makefile:1863: modpost] Error 2 > make[1]: *** [dev/src/linux/Makefile:234: __sub-make] Error 2 > make[1]: Leaving directory dev/out_linux > make: *** [Makefile:234: __sub-make] Error 2 Oh, sorry for this. I could post a new version if you require. But before that we may better align on the approach on how to support i.MX. > > More in general, I think that this NXP OEM specific additions, which are in > general welcome (and indeed as you know part of the spec was modified to > allow for OEM specific needs), do NOT belong to this generic SCMI Pinctrl > driver, because the driver from Oleksii/EPAM was born as a generic SCMI > driver and it fits perfectly with the Generic Pinctrl Linux subsystem and > related generic bindings parsing: now with this you are trying to stick a > custom OEM slight varied behaviour (and related binding) on top of a generic > thing. > > And this choice leads to a number of additional changes in the SCMI core to > support an even more complex handling of SCMI devices, which is already too > complex IMO.. > > IOW...I dont think that the whole idea of the per-protocol optional > compatible to be able to select slightly different behaviours/parsing would > have a great chance to fly sincerely... > > I know there is an issue with having a completely distinct SCMI IMX pinctrl > driver that uses the same protocol node @19 (without the need for the > compatible trick) due to the way in which the Pinctrl subsystem searches for > devices (by of_node)...I'll think about an alternative way to allow this but I am > not sure (as you saw) that would be so easily doable... For all protocols supports VENDOR extension attributes, we need a way to handle I think. As Linus wrote in https://lore.kernel.org/all/CACRpkdaRY+rU+md-r5gVyFH5ATt3Pqp9=M4=+WArYkfVLAFdpw@mail.gmail.com/: We may need: protocol@19 { compatible = "vendor,soc-scmi-pinctrl"; (...) > > Also, I am wondering if this is really a problem in reality since I would NOT > expect you to load/ship both the OEM/NXP custom specific SCMI pinctrl > driver AND the generic one on the same platform (after having made them > distinct I mean...) am I wrong ? You are right, but that means the upstream ARM64 defconfig will not able to support both i.MX9 and others. Thanks, Peng. > (so you could even made them exclude each other at compile time...far from > being the best option I agree...) > > Thanks, > Cristian
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index ba755ed2d46c..d96b7ede1355 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -44,7 +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_SCMI) += pinctrl-scmi.o pinctrl-scmi-imx.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-imx.c b/drivers/pinctrl/pinctrl-scmi-imx.c new file mode 100644 index 000000000000..e9d02e4c2cc1 --- /dev/null +++ b/drivers/pinctrl/pinctrl-scmi-imx.c @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2023 NXP + */ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/slab.h> + +#include <linux/pinctrl/pinconf.h> +#include <linux/pinctrl/pinconf-generic.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h> + +#include "pinctrl-scmi.h" +#include "pinctrl-utils.h" +#include "core.h" +#include "pinconf.h" + +/* SCMI pin control types, aligned with SCMI firmware */ +#define IMX_SCMI_PIN_TYPE_MUX 192 +#define IMX_SCMI_PIN_TYPE_CONFIG 193 +#define IMX_SCMI_PIN_TYPE_DAISY_ID 194 +#define IMX_SCMI_PIN_TYPE_DAISY_CFG 195 + +#define IMX_SCMI_NO_PAD_CTL BIT(31) +#define IMX_SCMI_PAD_SION BIT(30) +#define IMX_SCMI_IOMUXC_CONFIG_SION BIT(4) + +#define IMX_SCMI_NUM_CFG 4 +#define IMX_SCMI_PIN_SIZE 24 + +int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np, + struct pinctrl_map **map, unsigned int *num_maps) +{ + struct pinctrl_map *new_map; + const __be32 *list; + unsigned long *configs = NULL; + unsigned long cfg[IMX_SCMI_NUM_CFG]; + int map_num, size, pin_size, pin_id, num_pins; + int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val; + int i, j; + uint32_t ncfg; + static uint32_t daisy_off; + + if (!daisy_off) { + if (of_machine_is_compatible("fsl,imx95")) + daisy_off = 0x408; + else + dev_err(pctldev->dev, "platform not support scmi pinctrl\n"); + } + + list = of_get_property(np, "fsl,pins", &size); + if (!list) { + dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np); + return -EINVAL; + } + + pin_size = IMX_SCMI_PIN_SIZE; + + if (!size || size % pin_size) { + dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np); + return -EINVAL; + } + + num_pins = size / pin_size; + map_num = num_pins; + + new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map), + GFP_KERNEL); + if (!new_map) + return -ENOMEM; + + *map = new_map; + *num_maps = map_num; + + /* create config map */ + for (i = 0; i < num_pins; i++) { + j = 0; + ncfg = IMX_SCMI_NUM_CFG; + mux_reg = be32_to_cpu(*list++); + conf_reg = be32_to_cpu(*list++); + input_reg = be32_to_cpu(*list++); + mux_val = be32_to_cpu(*list++); + input_val = be32_to_cpu(*list++); + conf_val = be32_to_cpu(*list++); + if (conf_val & IMX_SCMI_PAD_SION) + mux_val |= IMX_SCMI_IOMUXC_CONFIG_SION; + + pin_id = mux_reg / 4; + + cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_MUX, mux_val); + + if (!conf_reg || (conf_val & IMX_SCMI_NO_PAD_CTL)) + ncfg--; + else + cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_CONFIG, conf_val); + + if (!input_reg) { + ncfg -= 2; + } else { + cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_DAISY_ID, + (input_reg - daisy_off) / 4); + cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_TYPE_DAISY_CFG, input_val); + } + + configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL); + + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; + new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id); + new_map[i].data.configs.configs = configs; + new_map[i].data.configs.num_configs = ncfg; + } + + return 0; +} diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c index 0710ce9a1b42..e677e2064ba7 100644 --- a/drivers/pinctrl/pinctrl-scmi.c +++ b/drivers/pinctrl/pinctrl-scmi.c @@ -8,6 +8,7 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/seq_file.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> @@ -18,6 +19,7 @@ #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> +#include "pinctrl-scmi.h" #include "pinctrl-utils.h" #include "core.h" #include "pinconf.h" @@ -86,6 +88,16 @@ static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = { #endif }; +static const struct pinctrl_ops pinctrl_scmi_imx_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, +#ifdef CONFIG_OF + .dt_node_to_map = pinctrl_scmi_imx_dt_node_to_map, + .dt_free_map = pinconf_generic_dt_free_map, +#endif +}; + static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev) { struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); @@ -327,6 +339,7 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, static const struct scmi_device_id scmi_id_table[] = { { SCMI_PROTOCOL_PINCTRL, "pinctrl" }, + { SCMI_PROTOCOL_PINCTRL, "pinctrl-scmi-imx", "fsl,imx95-scmi-pinctrl" }, { } }; MODULE_DEVICE_TABLE(scmi, scmi_id_table); @@ -361,6 +374,9 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev) pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops; pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops; + if (device_is_compatible(dev, "fsl,imx95-scmi-pinctrl")) + pmx->pctl_desc.pctlops = &pinctrl_scmi_imx_pinctrl_ops; + ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins, &pmx->pctl_desc.pins); if (ret) diff --git a/drivers/pinctrl/pinctrl-scmi.h b/drivers/pinctrl/pinctrl-scmi.h new file mode 100644 index 000000000000..25863b4428fe --- /dev/null +++ b/drivers/pinctrl/pinctrl-scmi.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright 2023 NXP + */ + +#ifndef __DRIVERS_PINCTRL_SCMI_H +#define __DRIVERS_PINCTRL_SCMI_H + +int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np, + struct pinctrl_map **map, unsigned int *num_maps); + +#endif /* __DRIVERS_PINCTRL_SCMI_H */