| Message ID | 20241225-scmi-fwdevlink-v1-1-e9a3a5341362@nxp.com (mailing list archive) |
|---|---|
| State | New |
| 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 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 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
> 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. Thanks, Peng > > Thanks, > Cristian
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. 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. 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 >
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; + + device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); + + return 0; +} + static struct scmi_device * __scmi_device_create(struct device_node *np, struct device *parent, int protocol, const char *name) @@ -397,7 +410,7 @@ __scmi_device_create(struct device_node *np, struct device *parent, scmi_dev->id = id; scmi_dev->protocol_id = protocol; scmi_dev->dev.parent = parent; - device_set_node(&scmi_dev->dev, of_fwnode_handle(np)); + __scmi_device_set_node(scmi_dev, np, protocol, name); scmi_dev->dev.bus = &scmi_bus_type; scmi_dev->dev.release = scmi_device_release; dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);