| Message ID | 20241225-scmi-fwdevlink-v1-1-e9a3a5341362@nxp.com |
|---|---|
| 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. > 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.
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);