| Message ID | 20250120-scmi-fwdevlink-v2-0-3af2fa37dbac@nxp.com |
|---|---|
| Headers | show |
| Series | scmi: Bypass set fwnode and introduce allow/block list to address devlink issue | expand |
On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > --- a/drivers/firmware/arm_scmi/bus.c > +++ b/drivers/firmware/arm_scmi/bus.c > @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > device_unregister(&scmi_dev->dev); > } > > +static int > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > + int protocol, const char *name) > +{ > + /* cpufreq device does not need to be supplier from devlink perspective */ > + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { I don't love this... It seems like an hack. Could we put a flag somewhere instead? Perhaps in scmi_device? (I'm just saying that because that's what we're passing to this function). regards, dan carpenter
On Tue, Feb 4, 2025 at 4:31 AM Peng Fan <peng.fan@nxp.com> wrote: > > Subject: [PATCH v2 0/4] scmi: Bypass set fwnode and introduce > > allow/block list to address devlink issue > > Any comments on this patchset? The pinctrl changes look OK to me so for those 2 patches: Acked-by: Linus Walleij <linus.walleij@linaro.org> in case you want to merge it through some other tree. Yours, Linus Walleij
On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 >> --- a/drivers/firmware/arm_scmi/bus.c >> +++ b/drivers/firmware/arm_scmi/bus.c >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) >> device_unregister(&scmi_dev->dev); >> } >> >> +static int >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, >> + int protocol, const char *name) >> +{ >> + /* cpufreq device does not need to be supplier from devlink perspective */ >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > >I don't love this... It seems like an hack. Could we put a flag >somewhere instead? Perhaps in scmi_device? (I'm just saying that >because that's what we're passing to this function). This means when creating scmi_device, a flag needs to be set which requires to extend scmi_device_id to include a flag entry or else. As below in scmi-cpufreq.c { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } I am not sure Sudeep or Cristian are happy with the idea or not. But back to the patch here, we are in the path creating the scmi_device and cpufreq scmi device seems the only one that cause issue. So it should be fine using this patch? Thanks, Peng > >regards, >dan carpenter >
On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote: > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > >> --- a/drivers/firmware/arm_scmi/bus.c > >> +++ b/drivers/firmware/arm_scmi/bus.c > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > >> device_unregister(&scmi_dev->dev); > >> } > >> > >> +static int > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > >> + int protocol, const char *name) > >> +{ > >> + /* cpufreq device does not need to be supplier from devlink perspective */ > >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > >I don't love this... It seems like an hack. Could we put a flag > >somewhere instead? Perhaps in scmi_device? (I'm just saying that > >because that's what we're passing to this function). > > This means when creating scmi_device, a flag needs to be set which requires > to extend scmi_device_id to include a flag entry or else. > > As below in scmi-cpufreq.c > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } > Yeah, I like that. - if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { + if (scmi_dev->flags & SCMI_FWNODE_NO) { Or we could do something like "if (scmi_dev->no_fwnode) {" regards, dan carpenter
On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote: > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote: > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > > >> --- a/drivers/firmware/arm_scmi/bus.c > > >> +++ b/drivers/firmware/arm_scmi/bus.c > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > > >> device_unregister(&scmi_dev->dev); > > >> } > > >> > > >> +static int > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > > >> + int protocol, const char *name) > > >> +{ > > >> + /* cpufreq device does not need to be supplier from devlink perspective */ > > >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > > > >I don't love this... It seems like an hack. Could we put a flag > > >somewhere instead? Perhaps in scmi_device? (I'm just saying that > > >because that's what we're passing to this function). > > > > This means when creating scmi_device, a flag needs to be set which requires > > to extend scmi_device_id to include a flag entry or else. > > > > As below in scmi-cpufreq.c > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } > > > > Yeah, I like that. > > - if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > + if (scmi_dev->flags & SCMI_FWNODE_NO) { > > Or we could do something like "if (scmi_dev->no_fwnode) {" I proposed a flag a few review ago about this, it shoule come somehow from the device_table above like Peng was proposing, so that a driver can just declare that does NOT need fw_devlink. Thanks, Cristian
On Thu, Feb 06, 2025 at 11:42:12AM +0000, Cristian Marussi wrote: > On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote: > > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote: > > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: > > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > > > >> --- a/drivers/firmware/arm_scmi/bus.c > > > >> +++ b/drivers/firmware/arm_scmi/bus.c > > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > > > >> device_unregister(&scmi_dev->dev); > > > >> } > > > >> > > > >> +static int > > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > > > >> + int protocol, const char *name) > > > >> +{ > > > >> + /* cpufreq device does not need to be supplier from devlink perspective */ > > > >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > > > > > >I don't love this... It seems like an hack. Could we put a flag > > > >somewhere instead? Perhaps in scmi_device? (I'm just saying that > > > >because that's what we're passing to this function). > > > > > > This means when creating scmi_device, a flag needs to be set which requires > > > to extend scmi_device_id to include a flag entry or else. > > > > > > As below in scmi-cpufreq.c > > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } > > > > > > > Yeah, I like that. > > > > - if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > + if (scmi_dev->flags & SCMI_FWNODE_NO) { > > > > Or we could do something like "if (scmi_dev->no_fwnode) {" > > I proposed a flag a few review ago about this, it shoule come somehow > from the device_table above like Peng was proposing, so that a driver > can just declare that does NOT need fw_devlink. Great. I think we're on the same page then. regards, dan carpenter
On Sun, Jan 19, 2025 at 11:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > With machine_allowlist, only allowed machines have pinctrl imx scmi > devices created. The fw_devlink will link consumer and supplier > correctly. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c > index 8f15c4c4dc4412dddb40505699fc3f459fdc0adc..058b4f0477039d57ddae06f385ad809cbb4784d6 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c > @@ -287,11 +287,6 @@ scmi_pinctrl_imx_get_pins(struct scmi_pinctrl_imx *pmx, struct pinctrl_desc *des > return 0; > } > > -static const char * const scmi_pinctrl_imx_allowlist[] = { > - "fsl,imx95", > - NULL > -}; > - > static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) > { > struct device *dev = &sdev->dev; > @@ -304,9 +299,6 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) > if (!handle) > return -EINVAL; > > - if (!of_machine_compatible_match(scmi_pinctrl_imx_allowlist)) > - return -ENODEV; > - > pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph); > if (IS_ERR(pinctrl_ops)) > return PTR_ERR(pinctrl_ops); > @@ -339,8 +331,13 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev) > return pinctrl_enable(pmx->pctldev); > } > > +static const char * const scmi_pinctrl_imx_allowlist[] = { > + "fsl,imx95", > + NULL > +}; > + > static const struct scmi_device_id scmi_id_table[] = { > - { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" }, > + { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx", NULL, scmi_pinctrl_imx_allowlist }, > { } > }; > MODULE_DEVICE_TABLE(scmi, scmi_id_table); > Definite NACK to this. Please don't depend on indirect conditions/flags. There's no guarantee that this check will hold true in the future. -Saravana
On Thu, Feb 6, 2025 at 3:42 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > > On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote: > > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote: > > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: > > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > > > >> --- a/drivers/firmware/arm_scmi/bus.c > > > >> +++ b/drivers/firmware/arm_scmi/bus.c > > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > > > >> device_unregister(&scmi_dev->dev); > > > >> } > > > >> > > > >> +static int > > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > > > >> + int protocol, const char *name) > > > >> +{ > > > >> + /* cpufreq device does not need to be supplier from devlink perspective */ > > > >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > > > > > >I don't love this... It seems like an hack. Could we put a flag > > > >somewhere instead? Perhaps in scmi_device? (I'm just saying that > > > >because that's what we're passing to this function). > > > > > > This means when creating scmi_device, a flag needs to be set which requires > > > to extend scmi_device_id to include a flag entry or else. > > > > > > As below in scmi-cpufreq.c > > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } > > > > > > > Yeah, I like that. > > > > - if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > + if (scmi_dev->flags & SCMI_FWNODE_NO) { > > > > Or we could do something like "if (scmi_dev->no_fwnode) {" > > I proposed a flag a few review ago about this, it shoule come somehow > from the device_table above like Peng was proposing, so that a driver > can just declare that does NOT need fw_devlink. Sorry, looks I replied to v1 series. Can you take a look at that response please? https://lore.kernel.org/lkml/CAGETcx87Stfkru9gJrc1sf=PtFGLY7=jrfFaCzK5Z4hq+2TCzg@mail.gmail.com/ If that suggestion I gave there would work, then that's the cleanest approach. This patch series is just kicking the can down the road (or down an inch). -Saravana
On Thu, Feb 13, 2025 at 12:17:06AM -0800, Saravana Kannan wrote: > On Thu, Feb 6, 2025 at 3:42 AM Cristian Marussi > <cristian.marussi@arm.com> wrote: > > > > On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote: > > > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote: > > > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote: > > > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote: > > > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644 > > > > >> --- a/drivers/firmware/arm_scmi/bus.c > > > > >> +++ b/drivers/firmware/arm_scmi/bus.c > > > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev) > > > > >> device_unregister(&scmi_dev->dev); > > > > >> } > > > > >> > > > > >> +static int > > > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, > > > > >> + int protocol, const char *name) > > > > >> +{ > > > > >> + /* cpufreq device does not need to be supplier from devlink perspective */ > > > > >> + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > > > > > > > >I don't love this... It seems like an hack. Could we put a flag > > > > >somewhere instead? Perhaps in scmi_device? (I'm just saying that > > > > >because that's what we're passing to this function). > > > > > > > > This means when creating scmi_device, a flag needs to be set which requires > > > > to extend scmi_device_id to include a flag entry or else. > > > > > > > > As below in scmi-cpufreq.c > > > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO } > > > > > > > > > > Yeah, I like that. > > > > > > - if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > > > + if (scmi_dev->flags & SCMI_FWNODE_NO) { > > > > > > Or we could do something like "if (scmi_dev->no_fwnode) {" > > > > I proposed a flag a few review ago about this, it shoule come somehow > > from the device_table above like Peng was proposing, so that a driver > > can just declare that does NOT need fw_devlink. > > Sorry, looks I replied to v1 series. Can you take a look at that > response please? > https://lore.kernel.org/lkml/CAGETcx87Stfkru9gJrc1sf=PtFGLY7=jrfFaCzK5Z4hq+2TCzg@mail.gmail.com/ > > If that suggestion I gave there would work, then that's the cleanest > approach. This patch series is just kicking the can down the road (or > down an inch). Thanks for the reply, I will answer on that other thread. Cristian
Current scmi drivers not work well with devlink. This patchset is a retry to address the issue in [1] which was a few months ago. Current scmi devices are not created from device tree, they are created from a scmi_device_id entry of each driver with the protocol matches with the fwnode reg value, this means there could be multiple devices created for one fwnode, but the fwnode only has one device pointer. This patchset is to do more checking before setting the device fwnode. And Introduce machine_allowlist and machine_blocklist. The reason to introduce machine_blocklist is for case that if pinctrl-scmi.c probes before pinctrl-imx-scmi.c probes on i.MX platform. Need to block pinctrl-scmi.c on i.MX platform. This may looks like hack, but seems no better way to make scmi works well with devlink. [1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/ Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Changes in v2: - Introduce machine_allowlist and machine_blocklist - Keep of_node for cpufreq device per Cristian - Patch 2 is an optimization patch when fixing the devlink issue - Link to v1: https://lore.kernel.org/r/20241225-scmi-fwdevlink-v1-0-e9a3a5341362@nxp.com --- Peng Fan (4): firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq firmware: arm_scmi: Add machine_allowlist and machine_blocklist pinctrl: freescale: scmi: Switch to use machine_allowlist pinctrl: scmi: Switch to use machine_blocklist drivers/firmware/arm_scmi/bus.c | 31 +++++++++++++++++++++++++++- drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++-------- drivers/pinctrl/pinctrl-scmi.c | 15 ++++++-------- include/linux/scmi_protocol.h | 3 +++ 4 files changed, 45 insertions(+), 19 deletions(-) --- base-commit: 9dff7bbdd359c73f1b44ab592bbb17e1c174fe43 change-id: 20241225-scmi-fwdevlink-afb5131f19ea Best regards,