| Message ID | 20241225-scmi-fwdevlink-v1-0-e9a3a5341362@nxp.com |
|---|---|
| Headers | show |
| Series | scmi: Bypass set fwnode to address devlink issue | expand |
On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use > SCMI_PROTCOL_PERF protocol, but with different name, so two scmi devices > will be created. But the fwnode->dev could only point to one device. > > If scmi cpufreq device created earlier, the fwnode->dev will point to > the scmi cpufreq device. Then the fw_devlink will link performance > domain user device(consumer) to the scmi cpufreq device(supplier). > But actually the performance domain user device, such as GPU, should use > the scmi perf device as supplier. Also if 'cpufreq.off=1' in bootargs, > the GPU driver will defer probe always, because of the scmi cpufreq > device not ready. > > Because for cpufreq, no need use fw_devlink. So bypass setting fwnode > for scmi cpufreq device. > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the scmi_device") > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > index 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb65484543044b4424fbe3b67245466 100644 > --- a/drivers/firmware/arm_scmi/bus.c > +++ b/drivers/firmware/arm_scmi/bus.c > @@ -345,6 +345,19 @@ 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")) > + return 0; > This is just a assumption based on current implementation. What happens if this is needed. Infact, it is used in the current implementation to create a dummy clock provider, so for sure with this change that will break IMO. -- Regards, Sudeep
On Wed, Dec 25, 2024 at 04:20:46PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > For the platform that not compatible with scmi pinctrl device, the > fwnode will not be set, so checking fwnode will make code simpler > and easy to maintain. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pinctrl/pinctrl-scmi.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391741e77c0aac3f029991e4bb 100644 > --- a/drivers/pinctrl/pinctrl-scmi.c > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, > return 0; > } > > -static const char * const scmi_pinctrl_blocklist[] = { > - "fsl,imx95", > - NULL > -}; > - > static int scmi_pinctrl_probe(struct scmi_device *sdev) > { > int ret; > @@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev) > if (!sdev->handle) > return -EINVAL; > > - if (of_machine_compatible_match(scmi_pinctrl_blocklist)) > + if (!dev->fwnode) I would prefer to see the blocklist to be explicit here rather than implicitly hiding it away with this change set.
On Wed, Dec 25, 2024 at 9:21 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > 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. > > 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/ Please drive any devlink-related patches by Saravana Kannan, he's pretty much the only person I trust to know how to do devlinks right. Yours, Linus Walleij
On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote: >On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use >> SCMI_PROTCOL_PERF protocol, but with different name, so two scmi devices >> will be created. But the fwnode->dev could only point to one device. >> >> If scmi cpufreq device created earlier, the fwnode->dev will point to >> the scmi cpufreq device. Then the fw_devlink will link performance >> domain user device(consumer) to the scmi cpufreq device(supplier). >> But actually the performance domain user device, such as GPU, should use >> the scmi perf device as supplier. Also if 'cpufreq.off=1' in bootargs, >> the GPU driver will defer probe always, because of the scmi cpufreq >> device not ready. >> >> Because for cpufreq, no need use fw_devlink. So bypass setting fwnode >> for scmi cpufreq device. >> >> Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the scmi_device") >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c >> index 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb65484543044b4424fbe3b67245466 100644 >> --- a/drivers/firmware/arm_scmi/bus.c >> +++ b/drivers/firmware/arm_scmi/bus.c >> @@ -345,6 +345,19 @@ 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")) >> + return 0; >> > >This is just a assumption based on current implementation. What happens >if this is needed. Infact, it is used in the current implementation to >create a dummy clock provider, so for sure with this change that will >break IMO. If cpufreq needs the deivce_node, it will be parsed as a supplier from devlink view. Then come to the issue, multiple scmi devices match one of node, Saravana replied before in below thread https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/ So quote here " The best fw_devlink could do is just not enforce any dependencies if there is more than one device instantiated for a given supplier DT node. " Since we are here that fw_devlink not support multiple devices match one of node and will not support(per my understanding), what scmi part could do is: only set of node to the scmi device that needs to be supplier Or we introduce compatible string to scmi node, and subnodes if one protocol supports mutilple function, such as cpufreq and device performance using PERF protocol. But this needs big change to scmi framework. Thanks Peng > >-- >Regards, >Sudeep
On Fri, Dec 27, 2024 at 06:06:24PM +0100, Linus Walleij wrote: >On Wed, Dec 25, 2024 at 9:21 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > >> 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. >> >> 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/ > >Please drive any devlink-related patches by Saravana Kannan, he's pretty >much the only person I trust to know how to do devlinks right. Quote Saravana's conclution[1] here: "The best fw_devlink could do is just not enforce any dependencies if there is more than one device instantiated for a given supplier DT node." So I think for systems using scmi could not rely on devlink to build supplier/consumer to make driver probe in order. [1]https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/ Thanks, Peng > >Yours, >Linus Walleij
On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote: > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use > > SCMI_PROTCOL_PERF protocol, but with different name, so two scmi devices > > will be created. But the fwnode->dev could only point to one device. > > > > If scmi cpufreq device created earlier, the fwnode->dev will point to > > the scmi cpufreq device. Then the fw_devlink will link performance > > domain user device(consumer) to the scmi cpufreq device(supplier). > > But actually the performance domain user device, such as GPU, should use > > the scmi perf device as supplier. Also if 'cpufreq.off=1' in bootargs, > > the GPU driver will defer probe always, because of the scmi cpufreq > > device not ready. > > > > Because for cpufreq, no need use fw_devlink. So bypass setting fwnode > > for scmi cpufreq device. > > Hi, > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the scmi_device") > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > index 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb65484543044b4424fbe3b67245466 100644 > > --- a/drivers/firmware/arm_scmi/bus.c > > +++ b/drivers/firmware/arm_scmi/bus.c > > @@ -345,6 +345,19 @@ 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")) > > + return 0; > > > > This is just a assumption based on current implementation. What happens > if this is needed. Infact, it is used in the current implementation to > create a dummy clock provider, so for sure with this change that will > break IMO. I agree with Sudeep on this: if you want to exclude some SCMI device from the fw_devlink handling to address the issues with multiple SCMI devices created on the same protocol nodes, cant we just flag this requirement here and avoid to call device_link_add in driver:scmi_set_handle(), instead of killing completely any possibility of referencing phandles (and having device_link_add failing as a consequence of having a NULL supplier) i.e. something like: @bus.c ------ static int __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np, int protocol, const char *name) { if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) scmi_dev->avoid_devlink = true; device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); .... and @driver.c ------------- static void scmi_set_handle(struct scmi_device *scmi_dev) { scmi_dev->handle = scmi_handle_get(&scmi_dev->dev); if (scmi_dev->handle && !scmi_dev->avoid_devlink) scmi_device_link_add(&scmi_dev->dev, scmi_dev->handle->dev); } .... so that you can avoid fw_devlink BUT keep the device_node NON-null for the device. This would mean also restoring the pre-existing explicit blacklisting in pinctrl-imx to avoid issues when pinctrl subsystem searches by device_node... ..or I am missing something ? Thanks, Cristian
On Fri, Dec 27, 2024 at 03:30:20PM +0000, Sudeep Holla wrote: > On Wed, Dec 25, 2024 at 04:20:46PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > For the platform that not compatible with scmi pinctrl device, the > > fwnode will not be set, so checking fwnode will make code simpler > > and easy to maintain. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/pinctrl/pinctrl-scmi.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > > index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391741e77c0aac3f029991e4bb 100644 > > --- a/drivers/pinctrl/pinctrl-scmi.c > > +++ b/drivers/pinctrl/pinctrl-scmi.c > > @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, > > return 0; > > } > > > > -static const char * const scmi_pinctrl_blocklist[] = { > > - "fsl,imx95", > > - NULL > > -}; > > - > > static int scmi_pinctrl_probe(struct scmi_device *sdev) > > { > > int ret; > > @@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev) > > if (!sdev->handle) > > return -EINVAL; > > > > - if (of_machine_compatible_match(scmi_pinctrl_blocklist)) > > + if (!dev->fwnode) > > I would prefer to see the blocklist to be explicit here rather than > implicitly hiding it away with this change set. Using a flag to inhibit device_link_add as said early in the series this could be dropped and kept as is, I suppose. Thanks, Cristian
On Thu, Jan 02, 2025 at 05:06:57PM +0000, Cristian Marussi wrote: >On Thu, Jan 02, 2025 at 07:38:06AM +0000, Peng Fan wrote: >> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting >> > fwnode for scmi cpufreq >> > >> > On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote: >> > > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote: >> > > > From: Peng Fan <peng.fan@nxp.com> >> > > > >> > > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use >> > > > SCMI_PROTCOL_PERF protocol, but with different name, so two >> > scmi >> > > > devices will be created. But the fwnode->dev could only point to >> > one device. >> > > > >> > > > If scmi cpufreq device created earlier, the fwnode->dev will point >> > > > to the scmi cpufreq device. Then the fw_devlink will link >> > > > performance domain user device(consumer) to the scmi cpufreq >> > device(supplier). >> > > > But actually the performance domain user device, such as GPU, >> > should >> > > > use the scmi perf device as supplier. Also if 'cpufreq.off=1' in >> > > > bootargs, the GPU driver will defer probe always, because of the >> > > > scmi cpufreq device not ready. >> > > > >> > > > Because for cpufreq, no need use fw_devlink. So bypass setting >> > > > fwnode for scmi cpufreq device. >> > > > >> > >> > Hi, >> > >> > > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the >> > > > scmi_device") >> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> >> > > > --- >> > > > drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++- >> > > > 1 file changed, 14 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/drivers/firmware/arm_scmi/bus.c >> > > > b/drivers/firmware/arm_scmi/bus.c index >> > > > >> > 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb654845 >> > 43044b442 >> > > > 4fbe3b67245466 100644 >> > > > --- a/drivers/firmware/arm_scmi/bus.c >> > > > +++ b/drivers/firmware/arm_scmi/bus.c >> > > > @@ -345,6 +345,19 @@ 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")) >> > > > + return 0; >> > > > >> > > >> > > This is just a assumption based on current implementation. What >> > > happens if this is needed. Infact, it is used in the current >> > > implementation to create a dummy clock provider, so for sure with >> > this >> > > change that will break IMO. >> > >> > I agree with Sudeep on this: if you want to exclude some SCMI device >> > from the fw_devlink handling to address the issues with multiple SCMI >> > devices created on the same protocol nodes, cant we just flag this >> > requirement here and avoid to call device_link_add in >> > driver:scmi_set_handle(), instead of killing completely any possibility of >> > referencing phandles (and having device_link_add failing as a >> > consequence of having a NULL supplier) >> > >> > i.e. something like: >> > >> > @bus.c >> > ------ >> > static int >> > __scmi_device_set_node(struct scmi_device *scmi_dev, struct >> > device_node *np, >> > int protocol, const char *name) { >> > if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, >> > "cpufreq")) >> > scmi_dev->avoid_devlink = true; >> > >> > device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); >> > .... >> > >> > >> > and @driver.c >> > ------------- >> > >> > static void scmi_set_handle(struct scmi_device *scmi_dev) { >> > scmi_dev->handle = scmi_handle_get(&scmi_dev->dev); >> > if (scmi_dev->handle && !scmi_dev->avoid_devlink) >> > scmi_device_link_add(&scmi_dev->dev, scmi_dev- >> > >handle->dev); } >> > >> > .... so that you can avoid fw_devlink BUT keep the device_node NON- >> > null for the device. >> > >> > This would mean also restoring the pre-existing explicit blacklisting in >> > pinctrl-imx to avoid issues when pinctrl subsystem searches by >> > device_node... >> > >> > ..or I am missing something ? >> >> link_ret = device_links_check_suppliers(dev); to check fw_devlink >> is before "ret = driver_sysfs_add(dev);" which >> issue bus notify. >> >> The link is fw_devlink, the devlink is created in 'device_add' >> if (dev->fwnode && !dev->fwnode->dev) { >> dev->fwnode->dev = dev; >> fw_devlink_link_device(dev); >> } >> The check condition is fwnode. >> >> I think scmi_dev->avoid_devlink not help here. >> > >Ah right...my bad, the issue comes from the device_links created by >fw_devlink indirectly while walking the phandles backrefs...still... >...cant we keep the device_node reference while keep on dropping the >fw_node as you did: > > if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) { > scmi_dev->dev.of_node = np; > return 0; > } > > device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); > .... > >...so that the fw_devlink machinery is disabled but still we create a >device with an underlying related device_node that can be referred in a >phandle. ok, I will add "scmi_dev->dev.of_node = np" for cpufreq device. > >I wonder also if it was not even more clean to DO initialize fw_devlink >instead, BUT add some of the existent fw_devlink/devlink flags to inhibit >all the checks...but I am not familiar with fw_devlink so much and I >have not experimented in these regards...so I may have just said >something unfeasible. fw_devlink is based on device tree node, so there is no way, unless add subnodes for a protocol node, but this is not welcomed. Thanks, Peng > >Thanks, >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. 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/ Cc: Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Peng Fan (4): firmware: arm_scmi: bus: Bypass setting fwnode for scmi cpufreq firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl pinctrl: scmi: Check fwnode instead of machine compatible pinctrl: freescale: scmi: Check fwnode instead of machine compatible drivers/firmware/arm_scmi/bus.c | 29 +++++++++++++++++++++++++++- drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 7 +------ drivers/pinctrl/pinctrl-scmi.c | 7 +------ 3 files changed, 30 insertions(+), 13 deletions(-) --- base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2 change-id: 20241225-scmi-fwdevlink-afb5131f19ea Best regards,