| Message ID | 20241225-scmi-fwdevlink-v1-1-e9a3a5341362@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
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. > Not 100% sure if above is correct. See: Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider") Am I missing something ?
On Tue, Feb 11, 2025 at 05:13:21PM +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. >> > >Not 100% sure if above is correct. See: > >Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider") > >Am I missing something ? Could we update juno-scmi.dtsi to use ? &A53_0 { - clocks = <&scmi_dvfs 1>; + power-domains = <&scmi_perf x>; + power-domain-names = "perf"; }; Even for scmi-cpufreq.c that needs fw_devlink because of the clocks entry in juno-scmi.dtsi, there is no issue here. Because the dummy clock provider will always be ready before opp with your upper fix. So we could safetly ignore fw_devlink per my understanding. Regards, Peng > >-- >Regards, >Sudeep
On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote: > On Tue, Feb 11, 2025 at 05:13:21PM +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. > >> > > > >Not 100% sure if above is correct. See: > > > >Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider") > > > >Am I missing something ? > > Could we update juno-scmi.dtsi to use ? > > &A53_0 { > - clocks = <&scmi_dvfs 1>; > + power-domains = <&scmi_perf x>; > + power-domain-names = "perf"; > }; > We can, but I retained it so that the clocks property support can be still validated until it is removed. I think there are few downstream users of it. It is not just the DTS files you need to look at when dealing with such things. It is the bindings that matter. Until bindings are not deprecated and made obsolete, support must exist even if you modify the only user in the upstream DT. -- Regards, Sudeep
On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote: > > On Tue, Feb 11, 2025 at 05:13:21PM +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. > > >> > > > > > >Not 100% sure if above is correct. See: > > > > > >Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider") > > > > > >Am I missing something ? > > > > Could we update juno-scmi.dtsi to use ? > > > > &A53_0 { > > - clocks = <&scmi_dvfs 1>; > > + power-domains = <&scmi_perf x>; > > + power-domain-names = "perf"; > > }; > > > > We can, but I retained it so that the clocks property support can be still > validated until it is removed. I think there are few downstream users of > it. It is not just the DTS files you need to look at when dealing with > such things. It is the bindings that matter. Until bindings are not > deprecated and made obsolete, support must exist even if you modify the > only user in the upstream DT. Sorry, been caught up on other stuff and trying to get to some long pending emails. Sudeep, Do you know why commit dd461cd9183f ("opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER") was needed? I'd think fw_devlink would have caught those issues. I'd recommended reverting that/fixing it differently instead of creating commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider") Peng, Sudeep, If you make fwnode ignore the cpufreq device, then it'll also not enforce ordering between cpufreq and it's suppliers like clocks and power domains. Not sure if that's a real possibility for scmi (I'm guessing no?). Make sure that's not going to be a problem. Cristian, Thanks for taking the time to give a detailed description here[1]. I seem to have missed that email. [1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/ Peng/Cristian, Yes, we can have the driver core ignore this device for fw_devlink by looking at some flag on the device (and not on the fwnode). But that is just kicking the can down the road. We could easily end up with two SCMI devices needing a separate set of consumers. For example, something like below can have two SCMI devices A and B created where only A needs the mboxes and only B needs shmem and power-domains. This will get messy even for drivers if the driver for A optionally needs power-domains on some machines, but not this one. firmware { scmi { compatible = "arm,scmi"; scmi_dvfs: protocol@13 { reg = <0x13>; #clock-cells = <1>; mbox-names = "tx", "rx"; mboxes = <&mailbox 1 0 &mailbox 1 1>; shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>; power-domains = <&blah>; }; Wait a sec, looking around at the SCMI code, I just realized that you don't even really care about the node name to get the protocol number and you just look at "reg" for protocol number. Why not just have peng's device have two protocol@13 DT nodes? cpufreq@13 { reg = <0x13>; } whateverelse@13 { reg = <0x13>; } You can also probably throw in a compatible field if you need to help the drivers pick the right node (where they currently pick the same node). Or you can do whatever else would help make sure the cpufreq device is attached to the cpufreq node and the whateverelse device is attached to the whateverelse node. Looks like that'll first help clean up the "two devices for one node" issue. And then the rest should just work? Cristian, am I missing anything? Thanks, Saravana
On Thu, Feb 13, 2025 at 12:03:15AM -0800, Saravana Kannan wrote: > On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > Hi Saravana, > > On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote: > > > On Tue, Feb 11, 2025 at 05:13:21PM +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> > > > >> [snip] > > Cristian, > > Thanks for taking the time to give a detailed description here[1]. I > seem to have missed that email. > [1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/ > > Peng/Cristian, > > Yes, we can have the driver core ignore this device for fw_devlink by > looking at some flag on the device (and not on the fwnode). But that > is just kicking the can down the road. We could easily end up with two Oh yes this is definitely some sort of hack/workaround that just kicks the can down the road, I agree...just I cannot see any better solution from what Peng propose (beside maybe we can discuss his implementation details as we are doing...) > SCMI devices needing a separate set of consumers. For example, > something like below can have two SCMI devices A and B created where > only A needs the mboxes and only B needs shmem and power-domains. This ..not really...it is even worse :P ... the mbox/shmem props down below are really definition of a mailbox transport SCMI channel: some transports allow multiple channels to be defined and in such case you can dedicate one channel to a specific protocol... ...so, in this case, you will see there will be something similar defined in terms of mboxes/shmem at the top SCMI DT node to represent an SCMI channel used for all the protocols WHILE this additional definition inside the protocol node defines a dedicated channel...IOW these props mboxes/shmem are really parsed/consumed upfront by the core SCMI stack at probe to configure and allocare basic comms channel BEFORE any SCMI device is created ...then the protocol DT node is no more used by the core and is instead 'lent' to create SCMI devices for the drivers needing them...(possibly lending it to multiple users...that is the issue) > will get messy even for drivers if the driver for A optionally needs > power-domains on some machines, but not this one. > > firmware { > scmi { > compatible = "arm,scmi"; > scmi_dvfs: protocol@13 { > reg = <0x13>; > #clock-cells = <1>; > mbox-names = "tx", "rx"; > mboxes = <&mailbox 1 0 &mailbox 1 1>; > shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>; > power-domains = <&blah>; > }; > > Wait a sec, looking around at the SCMI code, I just realized that you > don't even really care about the node name to get the protocol number > and you just look at "reg" for protocol number. Why not just have > peng's device have two protocol@13 DT nodes? > > cpufreq@13 { > reg = <0x13>; > } > whateverelse@13 { > reg = <0x13>; > } > > You can also probably throw in a compatible field if you need to help > the drivers pick the right node (where they currently pick the same > node). Or you can do whatever else would help make sure the cpufreq > device is attached to the cpufreq node and the whateverelse device is > attached to the whateverelse node. ..well...my longer-than-ever explanation of the innner-workings was meant to explain where the problem comes from, and how would be difficult to address it WITHOUT changing the DT bindings, BECAUE I pretty much doubt that throwing into the mix also multiple nodes definitions and compatibles could fly with the DT maintainers, AND certainly it will go against the basic rules for 'reg-indexed' properties ...you cannot have 2 prop indexed with the same reg-value AFAIK...and the reg-value, here, is indeed the spec protocol number so you cannot change that either within the set of nodes sharing the same prop.... ...moreover the above additional construct of having possibly per-protocol channels would create even more a mess in this scenario of explicitly declared duplicated protocol-nodes: - should we duplicate the optional mbox/shmem too ? not possible...DT sanity would fail immediately also in this (I suppose due to duplicated entries) ...BUT - at the same time we should assume that ALL the duplicated protocols inherits the optional per-protocol dedicated channel that is defined in one of them...seems very dirty to me... ...moreover...explicitly allowing for such duplicate DT protocol definitions would open the door to create even more SCMI drivers like pinctrl-imx that uses the same PINCTRL protocol as the generic-pinctrl BUT really implements the SAME functionalities as the generic one (just slightly differently and using a complete distinct set of NXP pinctrl bindings for historical reasons AFAIU)....BUT pinctrl-imx is an *unfortunate* exception that we had to support for the historical reason I mentioned BUT should NOT be the rule NOR the advised way... ....while other drivers exists that share the usage of the same protocol (HWMON/IIO GENPD/CPUFREQ), they use the same protocol to achieve different things in different subsytems...and they are anyway impacted (even to a less degree) by this fw_devlink issue AFAIU so the problem indeed exist also out of pinctrl-imx > > Looks like that'll first help clean up the "two devices for one node" > issue. And then the rest should just work? Cristian, am I missing > anything? Yes that is the main issue...but still dont see how to solve it in a clean way... Thanks, Cristian
On Thu, Feb 13, 2025 at 08:23:53PM +0000, Cristian Marussi wrote: >On Thu, Feb 13, 2025 at 12:03:15AM -0800, Saravana Kannan wrote: >> On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote: >> > > >Hi Saravana, > >> > On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote: >> > > On Tue, Feb 11, 2025 at 05:13:21PM +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> >> > > >> > >[snip] > >> >> Cristian, >> >> Thanks for taking the time to give a detailed description here[1]. I >> seem to have missed that email. >> [1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/ >> >> Peng/Cristian, >> >> Yes, we can have the driver core ignore this device for fw_devlink by >> looking at some flag on the device (and not on the fwnode). But that >> is just kicking the can down the road. We could easily end up with two > >Oh yes this is definitely some sort of hack/workaround that just kicks >the can down the road, I agree...just I cannot see any better solution >from what Peng propose (beside maybe we can discuss his implementation >details as we are doing...) > >> SCMI devices needing a separate set of consumers. For example, >> something like below can have two SCMI devices A and B created where >> only A needs the mboxes and only B needs shmem and power-domains. This > >..not really...it is even worse :P ... the mbox/shmem props down below are >really definition of a mailbox transport SCMI channel: some transports >allow multiple channels to be defined and in such case you can dedicate >one channel to a specific protocol... > >...so, in this case, you will see there will be something similar defined >in terms of mboxes/shmem at the top SCMI DT node to represent an SCMI channel >used for all the protocols WHILE this additional definition inside the >protocol node defines a dedicated channel...IOW these props mboxes/shmem >are really parsed/consumed upfront by the core SCMI stack at probe to >configure and allocare basic comms channel BEFORE any SCMI device is created >...then the protocol DT node is no more used by the core and is instead 'lent' >to create SCMI devices for the drivers needing them...(possibly lending it to >multiple users...that is the issue) > >> will get messy even for drivers if the driver for A optionally needs >> power-domains on some machines, but not this one. >> >> firmware { >> scmi { >> compatible = "arm,scmi"; >> scmi_dvfs: protocol@13 { >> reg = <0x13>; >> #clock-cells = <1>; >> mbox-names = "tx", "rx"; >> mboxes = <&mailbox 1 0 &mailbox 1 1>; >> shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>; >> power-domains = <&blah>; >> }; >> >> Wait a sec, looking around at the SCMI code, I just realized that you >> don't even really care about the node name to get the protocol number >> and you just look at "reg" for protocol number. Why not just have >> peng's device have two protocol@13 DT nodes? >> >> cpufreq@13 { >> reg = <0x13>; >> } >> whateverelse@13 { >> reg = <0x13>; >> } >> >> You can also probably throw in a compatible field if you need to help >> the drivers pick the right node (where they currently pick the same >> node). Or you can do whatever else would help make sure the cpufreq >> device is attached to the cpufreq node and the whateverelse device is >> attached to the whateverelse node. > >..well...my longer-than-ever explanation of the innner-workings was >meant to explain where the problem comes from, and how would be difficult >to address it WITHOUT changing the DT bindings, BECAUE I pretty much doubt >that throwing into the mix also multiple nodes definitions and compatibles >could fly with the DT maintainers, AND certainly it will go against the basic >rules for 'reg-indexed' properties ...you cannot have 2 prop indexed with the >same reg-value AFAIK...and the reg-value, here, is indeed the spec protocol >number so you cannot change that either within the set of nodes sharing >the same prop.... > >...moreover the above additional construct of having possibly per-protocol >channels would create even more a mess in this scenario of explicitly >declared duplicated protocol-nodes: > >- should we duplicate the optional mbox/shmem too ? not possible...DT sanity > would fail immediately also in this (I suppose due to duplicated entries) > >...BUT > >- at the same time we should assume that ALL the duplicated protocols inherits >the optional per-protocol dedicated channel that is defined in one of >them...seems very dirty to me... > >...moreover...explicitly allowing for such duplicate DT protocol definitions >would open the door to create even more SCMI drivers like pinctrl-imx that >uses the same PINCTRL protocol as the generic-pinctrl BUT really implements >the SAME functionalities as the generic one (just slightly differently >and using a complete distinct set of NXP pinctrl bindings for historical >reasons AFAIU)....BUT pinctrl-imx is an *unfortunate* exception that we had >to support for the historical reason I mentioned BUT should NOT be the rule >NOR the advised way... > >....while other drivers exists that share the usage of the same protocol >(HWMON/IIO GENPD/CPUFREQ), they use the same protocol to achieve different >things in different subsytems...and they are anyway impacted (even to a less >degree) by this fw_devlink issue AFAIU so the problem indeed exist also >out of pinctrl-imx > >> >> Looks like that'll first help clean up the "two devices for one node" >> issue. And then the rest should just work? Cristian, am I missing >> anything? > >Yes that is the main issue...but still dont see how to solve it in a >clean way... A potential solution is not using reg in the protocol nodes. Define nodes as below: devperf { compatible ="arm,scmi-devperf"; } cpuperf { compatible ="arm,scmi-cpuperf"; } pinctrl { compatible ="arm,scmi-pinctrl"; } The reg is coded in driver. But the upper requires restruction of scmi framework. Put the above away, could we first purse a simple way first to address the current bug in kernel? Just as I prototyped here: https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2 Thanks, Peng. > >Thanks, >Cristian
On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote: > A potential solution is not using reg in the protocol nodes. Define nodes > as below: > devperf { > compatible ="arm,scmi-devperf"; > } > > cpuperf { > compatible ="arm,scmi-cpuperf"; > } > > pinctrl { > compatible ="arm,scmi-pinctrl"; > } > > The reg is coded in driver. > > But the upper requires restruction of scmi framework. > > Put the above away, could we first purse a simple way first to address > the current bug in kernel? Just as I prototyped here: > https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2 > Good luck getting these bindings merged. I don't like it as it is pushing software policy or issues into to the devicetree. What we have as SCMI binding is more than required for a firmware interface IMO. So, you are on your own to get these bindings approved as I am not on board with these but if you convince DT maintainers, I will have a look at it then to see if we can make that work really.
On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote: >On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote: >> A potential solution is not using reg in the protocol nodes. Define nodes >> as below: >> devperf { >> compatible ="arm,scmi-devperf"; >> } >> >> cpuperf { >> compatible ="arm,scmi-cpuperf"; >> } >> >> pinctrl { >> compatible ="arm,scmi-pinctrl"; >> } >> >> The reg is coded in driver. >> >> But the upper requires restruction of scmi framework. >> >> Put the above away, could we first purse a simple way first to address >> the current bug in kernel? Just as I prototyped here: >> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2 >> > >Good luck getting these bindings merged. I don't like it as it is pushing >software policy or issues into to the devicetree. What we have as SCMI >binding is more than required for a firmware interface IMO. So, you are Would you mind share more info on other cases that SCMI not as firmware interface? >on your own to get these bindings approved as I am not on board with >these but if you convince DT maintainers, I will have a look at it then >to see if we can make that work really. The issues are common to SCMI, not i.MX specific. I just propose potential solutions. You are the SCMI maintainer, there is no chance to get bindings approved without you. No more ideas from me. Leave this to you in case you have better solution. Regards, Peng > >-- >Regards, >Sudeep >
On Tue, Feb 18, 2025 at 09:36:19PM +0800, Peng Fan wrote: > On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote: > >On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote: > >> A potential solution is not using reg in the protocol nodes. Define nodes > >> as below: > >> devperf { > >> compatible ="arm,scmi-devperf"; > >> } > >> > >> cpuperf { > >> compatible ="arm,scmi-cpuperf"; > >> } > >> > >> pinctrl { > >> compatible ="arm,scmi-pinctrl"; > >> } > >> > >> The reg is coded in driver. > >> > >> But the upper requires restruction of scmi framework. > >> > >> Put the above away, could we first purse a simple way first to address > >> the current bug in kernel? Just as I prototyped here: > >> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2 > >> > > > >Good luck getting these bindings merged. I don't like it as it is pushing > >software policy or issues into to the devicetree. What we have as SCMI > >binding is more than required for a firmware interface IMO. So, you are > > Would you mind share more info on other cases that SCMI not as firmware > interface? > > >on your own to get these bindings approved as I am not on board with > >these but if you convince DT maintainers, I will have a look at it then > >to see if we can make that work really. > > The issues are common to SCMI, not i.MX specific. > I just propose potential solutions. You are the SCMI maintainer, there > is no chance to get bindings approved without you. > I am not blocking you. What I mentioned is I don't agree that DT can be used to resolve this issue, but I don't have time or alternate solution ATM. So if you propose DT based solution and the maintainers agree for the proposed bindings I will take a look and help you to make that work. But I will raise any objections I may have if the proposal has issues mainly around the compatibility and ease of maintenance. > No more ideas from me. Leave this to you in case you have better solution. > Unfortunately no, I don't have one. I haven't had time to sit and explore the issue and think of any solution yet. -- Regards, Sudeep
On Wed, Feb 19, 2025 at 10:17:46AM +0000, Sudeep Holla wrote: >On Tue, Feb 18, 2025 at 09:36:19PM +0800, Peng Fan wrote: >> On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote: >> >On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote: >> >> A potential solution is not using reg in the protocol nodes. Define nodes >> >> as below: >> >> devperf { >> >> compatible ="arm,scmi-devperf"; >> >> } >> >> >> >> cpuperf { >> >> compatible ="arm,scmi-cpuperf"; >> >> } >> >> >> >> pinctrl { >> >> compatible ="arm,scmi-pinctrl"; >> >> } >> >> >> >> The reg is coded in driver. >> >> >> >> But the upper requires restruction of scmi framework. >> >> >> >> Put the above away, could we first purse a simple way first to address >> >> the current bug in kernel? Just as I prototyped here: >> >> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2 >> >> >> > >> >Good luck getting these bindings merged. I don't like it as it is pushing >> >software policy or issues into to the devicetree. What we have as SCMI >> >binding is more than required for a firmware interface IMO. So, you are >> >> Would you mind share more info on other cases that SCMI not as firmware >> interface? >> >> >on your own to get these bindings approved as I am not on board with >> >these but if you convince DT maintainers, I will have a look at it then >> >to see if we can make that work really. >> >> The issues are common to SCMI, not i.MX specific. >> I just propose potential solutions. You are the SCMI maintainer, there >> is no chance to get bindings approved without you. >> > >I am not blocking you. What I mentioned is I don't agree that DT can be used >to resolve this issue, but I don't have time or alternate solution ATM. So >if you propose DT based solution and the maintainers agree for the proposed >bindings I will take a look and help you to make that work. But I will raise >any objections I may have if the proposal has issues mainly around the >compatibility and ease of maintenance. Sorry, if I misunderstood. I will give a look on this and propose a RFC. DT maintainers may ask for a patchset including binding change and driver changes to get a whole view on the compatible stuff. BTW, Cristian, Saravana if you have any objections/ideas or would take on this effort, please let me know. Thanks, Peng > >> No more ideas from me. Leave this to you in case you have better solution. >> > >Unfortunately no, I don't have one. I haven't had time to sit and explore >the issue and think of any solution yet. > >-- >Regards, >Sudeep
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);