| Message ID | 20240505-pinctrl-scmi-oem-v3-v4-0-7c99f989e9ba@nxp.com |
|---|---|
| Headers | show |
| Series | pinctrl: scmi: support i.MX95 OEM extensions | expand |
On Sun, May 05, 2024 at 11:47:18AM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > i.MX95 will have its own pinctrl scmi driver, so need block > pinctrl-scmi driver for i.MX95, otherwise there will be two pinctrl > devices for a single scmi protocol@19. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pinctrl/pinctrl-scmi.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > index 036bc1e3fc6c..331ca20ac68b 100644 > --- a/drivers/pinctrl/pinctrl-scmi.c > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -11,6 +11,7 @@ > #include <linux/errno.h> > #include <linux/module.h> > #include <linux/mod_devicetable.h> > +#include <linux/of.h> > #include <linux/scmi_protocol.h> > #include <linux/slab.h> > #include <linux/types.h> > @@ -504,6 +505,11 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, > return 0; > } > > +static const struct of_device_id scmi_pinctrl_blocklist[] = { > + { .compatible = "fsl,imx95", }, > + { } > +}; > + > static int scmi_pinctrl_probe(struct scmi_device *sdev) > { > int ret; > @@ -511,10 +517,14 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev) > struct scmi_pinctrl *pmx; > const struct scmi_handle *handle; > struct scmi_protocol_handle *ph; > + struct device_node *np __free(device_node) = of_find_node_by_path("/"); > > if (!sdev->handle) > return -EINVAL; > > + if (of_match_node(scmi_pinctrl_blocklist, np)) > + return -ENODEV; Use of_machine_compatible_match() instead. > + > handle = sdev->handle; > > pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph); > > -- > 2.37.1 >
Sun, May 05, 2024 at 11:47:19AM +0800, Peng Fan (OSS) kirjoitti: > From: Peng Fan <peng.fan@nxp.com> > > The generic pinctrl-scmi.c driver could not be used for i.MX95 because > i.MX95 SCMI firmware not supports functions, groups or generic > 'Pin Configuration Type and Enumerations' listed in SCMI Specification. > > i.MX95 System Control Management Interface(SCMI) firmware only supports > below pin configuration types which are OEM specific types: > 192: PIN MUX > 193: PIN CONF > 194: DAISY ID > 195: DAISY VAL > > To support Linux generic pinctrl properties(pinmux, bias-pull-[up, > down], and etc), need extract the value from the property and map > them to the format that i.MX95 SCMI pinctrl protocol understands, > so add this driver. ... > +struct imx_pin_group { > + struct pingroup data; > +}; I don't see the necessity of having this wrapper structure. Can't you simply use struct pingroup directly? ... > +static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) > +{ > + int ret; > + struct device *dev = &sdev->dev; > + struct scmi_pinctrl_imx *pmx; > + const struct scmi_handle *handle; > + struct scmi_protocol_handle *ph; > + struct device_node *np __free(device_node) = of_find_node_by_path("/"); > + const struct scmi_pinctrl_proto_ops *pinctrl_ops; > + if (!sdev->handle) > + return -EINVAL; When this conditional can be true? > + if (!of_match_node(scmi_pinctrl_imx_allowlist, np)) > + return -ENODEV; > + handle = sdev->handle; It's even better to assign first and then check if the above check is needed at all. > + pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph); > + if (IS_ERR(pinctrl_ops)) > + return PTR_ERR(pinctrl_ops); > + > + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL); > + if (!pmx) > + return -ENOMEM; > + > + pmx->ph = ph; > + pmx->ops = pinctrl_ops; > + > + pmx->dev = dev; > + pmx->pctl_desc.name = DRV_NAME; > + pmx->pctl_desc.owner = THIS_MODULE; > + pmx->pctl_desc.pctlops = &pinctrl_scmi_imx_pinctrl_ops; > + pmx->pctl_desc.pmxops = &pinctrl_scmi_imx_pinmux_ops; > + pmx->pctl_desc.confops = &pinctrl_scmi_imx_pinconf_ops; > + > + ret = scmi_pinctrl_imx_get_pins(pmx, &pmx->pctl_desc); > + if (ret) > + return ret; > + > + ret = scmi_pinctrl_imx_probe_dt(sdev, pmx); > + if (ret) > + return ret; > + > + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx, > + &pmx->pctldev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n"); > + > + return pinctrl_enable(pmx->pctldev); > +}
> Subject: Re: [PATCH v4 3/3] pinctrl: imx: support SCMI pinctrl protocol for > i.MX95 > > Sun, May 05, 2024 at 11:47:19AM +0800, Peng Fan (OSS) kirjoitti: > > From: Peng Fan <peng.fan@nxp.com> > > > > The generic pinctrl-scmi.c driver could not be used for i.MX95 because > > i.MX95 SCMI firmware not supports functions, groups or generic 'Pin > > Configuration Type and Enumerations' listed in SCMI Specification. > > > > i.MX95 System Control Management Interface(SCMI) firmware only > > supports below pin configuration types which are OEM specific types: > > 192: PIN MUX > > 193: PIN CONF > > 194: DAISY ID > > 195: DAISY VAL > > > > To support Linux generic pinctrl properties(pinmux, bias-pull-[up, > > down], and etc), need extract the value from the property and map them > > to the format that i.MX95 SCMI pinctrl protocol understands, so add > > this driver. > Thanks for refining this. > ... > > > +struct imx_pin_group { > > + struct pingroup data; > > +}; > > I don't see the necessity of having this wrapper structure. Can't you simply > use struct pingroup directly? Yeah. Let me drop the wrapper in v6. > > ... > > > +static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) { > > + int ret; > > + struct device *dev = &sdev->dev; > > + struct scmi_pinctrl_imx *pmx; > > + const struct scmi_handle *handle; > > + struct scmi_protocol_handle *ph; > > + struct device_node *np __free(device_node) = > of_find_node_by_path("/"); > > + const struct scmi_pinctrl_proto_ops *pinctrl_ops; > > > + if (!sdev->handle) > > + return -EINVAL; > > When this conditional can be true? Just follow what other SCMI drivers do. scmi_set_handle will get handles, per the code, there might be NULL. > > > + if (!of_match_node(scmi_pinctrl_imx_allowlist, np)) > > + return -ENODEV; > > > + handle = sdev->handle; > > It's even better to assign first and then check if the above check is needed at > all. Yeah. Update in v6. > > > + pinctrl_ops = handle->devm_protocol_get(sdev, > SCMI_PROTOCOL_PINCTRL, &ph); > > + if (IS_ERR(pinctrl_ops)) > > + return PTR_ERR(pinctrl_ops); > > + > > + pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL); > > + if (!pmx) > > + return -ENOMEM; > > + > > + pmx->ph = ph; > > + pmx->ops = pinctrl_ops; > > + > > + pmx->dev = dev; > > + pmx->pctl_desc.name = DRV_NAME; > > + pmx->pctl_desc.owner = THIS_MODULE; > > + pmx->pctl_desc.pctlops = &pinctrl_scmi_imx_pinctrl_ops; > > + pmx->pctl_desc.pmxops = &pinctrl_scmi_imx_pinmux_ops; > > + pmx->pctl_desc.confops = &pinctrl_scmi_imx_pinconf_ops; > > + > > + ret = scmi_pinctrl_imx_get_pins(pmx, &pmx->pctl_desc); > > + if (ret) > > + return ret; > > + > > + ret = scmi_pinctrl_imx_probe_dt(sdev, pmx); > > + if (ret) > > + return ret; > > + > > + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx, > > + &pmx->pctldev); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n"); > > + > > + return pinctrl_enable(pmx->pctldev); } Thanks, Peng. > > -- > With Best Regards, > Andy Shevchenko >
ARM SCMI v3.2 Table 24 Pin Configuration Type and Enumerations: '192 -255 OEM specific units'. i.MX95 System Manager FW supports SCMI PINCTRL protocol, but it has zero functions, groups. So pinctrl-scmi.c could not be reused for i.MX95. Because nxp,pin-func, nxp,pin-conf properties are rejected by dt maintainers, so use generic property 'pinmux' which requires a new driver pinctrl-imx-scmi.c The node will be as below: pinctrl_usdhc1: usdhc1-pins { sd1-grp0 { pinmux = <IMX95_PAD_SD1_CLK__USDHC1_CLK IMX95_PAD_SD1_STROBE__USDHC1_STROBE>; drive-strength = <0xe>; input-schmitt-enable; bias-pull-down; slew-rate = <0x3>; }; sd1-grp1 { pinmux = <IMX95_PAD_SD1_CMD__USDHC1_CMD IMX95_PAD_SD1_DATA0__USDHC1_DATA0 IMX95_PAD_SD1_DATA1__USDHC1_DATA1 IMX95_PAD_SD1_DATA2__USDHC1_DATA2 IMX95_PAD_SD1_DATA3__USDHC1_DATA3 IMX95_PAD_SD1_DATA4__USDHC1_DATA4 IMX95_PAD_SD1_DATA5__USDHC1_DATA5 IMX95_PAD_SD1_DATA6__USDHC1_DATA6 IMX95_PAD_SD1_DATA7__USDHC1_DATA7>; drive-strength = <0xe>; input-schmitt-enable; bias-pull-up; slew-rate = <0x3>; }; }; Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Changes in v4: - Rebase to next-20240503 - Add pinctrl-scmi-imx.c itself get pins and scmi pinctrl structure to decouple pinctrl-scmi.c and pinctrl-scmi-imx.c, so drop patch 3,4,5. - Link to v3: https://lore.kernel.org/r/20240428-pinctrl-scmi-oem-v3-v3-0-eda341eb47ed@nxp.com Changes in v3: - patch 2,3,4,5 are new. - Rewrite the binding, drop nxp,pin-x properties, use generic properties as Rob commented. - Switch to using pinmux means pinctrl-scmi.c could not be reused, so add a new driver in patch 6 for i.MX95. But pinctrl_scmi_get_pins and scmi_pinctrl are exported for i.MX95 usage. - Link to v2: https://lore.kernel.org/r/20240418-pinctrl-scmi-oem-v1-v2-0-3a555a3c58c3@nxp.com Changes in v2: - Rename nxp,imx95-pinctrl.yaml to nxp,imx95-scmi-pinctrl.yaml and move to firmware - Merged patch [1,2]/3 v1 into patch 1/2 v2. - nxp,imx95-scmi-pinctrl.yaml only has patterProperties for subnode The pinctrl will be as below for i.MX95. pinctrl_usdhc1: usdhc1-pins { sd1cmd { pins = "sd1cmd"; nxp,func-id = <0>; nxp,pin-conf = <0x138e>; }; sd1data { pins = "sd1data"; nxp,func-id = <0>; nxp,pin-conf = <0x138e>; }; }; - Add pins enum, correct description. - Link to v1: https://lore.kernel.org/r/20240412-pinctrl-scmi-oem-v1-v1-0-704f242544c1@nxp.com --- Peng Fan (3): dt-bindings: firmware: arm,scmi: Add properties for i.MX95 Pinctrl OEM extensions pinctrl: scmi: add blocklist pinctrl: imx: support SCMI pinctrl protocol for i.MX95 .../devicetree/bindings/firmware/arm,scmi.yaml | 9 +- .../bindings/firmware/nxp,imx95-scmi-pinctrl.yaml | 37 ++ drivers/pinctrl/freescale/Kconfig | 9 + drivers/pinctrl/freescale/Makefile | 1 + drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 586 +++++++++++++++++++++ drivers/pinctrl/pinctrl-scmi.c | 10 + 6 files changed, 649 insertions(+), 3 deletions(-) --- base-commit: 4db57327adc359a3f9a3481d60104be67c42964f change-id: 20240428-pinctrl-scmi-oem-v3-12130031a74d Best regards,