| 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.
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
On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote: > > 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. > Can you point me to the DTS with which you are seeing this issue ? I am trying to reproduce the issue but so far not successful. I did move to power-domains for CPUFreq on Juno. IIUC all we need is both cpufreq and performance genpd drivers in the kernel and then GPU using perf genpd fails with probe deferral right ? I need pointers to reproduce the issue so that I can check if what I have cooked up as a solution really works. -- Regards, Sudeep
On Mon, Mar 10, 2025 at 11:59:33AM +0000, Sudeep Holla wrote: > On Mon, Mar 10, 2025 at 10:45:44AM +0000, Peng Fan wrote: > > > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting > > > fwnode for scmi cpufreq > > > > > > On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote: > > > > > > > > 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. > > > > > > > > > > Can you point me to the DTS with which you are seeing this issue ? > > > I am trying to reproduce the issue but so far not successful. I did move > > > to power-domains for CPUFreq on Juno. IIUC all we need is both > > > cpufreq and performance genpd drivers in the kernel and then GPU > > > using perf genpd fails with probe deferral right ? I need pointers to > > > reproduce the issue so that I can check if what I have cooked up as a > > > solution really works. > > > > This is in downstream tree: > > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L2971 > > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L3043 > > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L80 > > > > we are using "power-domains" property for cpu perf and gpu/vpu perf. > > > > If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer probe. > > > > OK, does the probe of these drivers get called or they don't as the driver > core doesn't allow that ? I just have a dummy driver for mali on Juno > which just does dev_pm_domain_attach_list() in the probe and it seem to > succeed even when cpufreq.off=1 is passed. I see scmi-cpufreq failing > with -ENODEV as expected. > > I need to follow the code and check if I can somehow reproduce. Also are > you sure this is not with anything in the downstream code ? Also have you > tried this with v6.14-rc* ? Are you sure all the fw_devlink code is backported > in the tree you pointed me which is v6.6-stable ? > I even tried the above branch, but no luck. The above is neither latest stable version nor pure stable. It has few extra patches backported though IIUC. Anyways any pointers to enable me to reproduce the issue would be much appreciated.
On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote: > Based on linux-next, I added below node: > > + > + test@4f000000 { > + compatible = "fsl,imx-test"; > + power-domains = <&scmi_devpd IMX95_PD_VPU>, <&scmi_perf IMX95_PERF_VPU>; > + power-domain-names = "vpumix", "vpuperf"; > + }; > > I not write a driver for it, so just check devlink information from sysfs interface. > > From below sys directory, this test device takes scmi_dev.4 and scmi_dev.3 as supplier. > root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# ls > driver_override of_node subsystem supplier:scmi_protocol:scmi_dev.4 waiting_for_supplier > modalias power supplier:scmi_protocol:scmi_dev.3 uevent > > Checking scmi_dev.4 below, it is scmi cpufreq, not the scmi perf device. > scmi_dev.3 is correct, it is genpd. > > root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.4/modalias > scmi_dev.4:13:cpufreq > root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.3/modalias > scmi_dev.3:11:genpd > root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# > > > So it is clear that wrong fw_devlink is created, it is because scmi cpufreq device is > created earlier and when device_add, the below logic makes the fwnode pointer points > to scmi cpufreq device. > if (dev->fwnode && !dev->fwnode->dev) { > dev->fwnode->dev = dev; > fw_devlink_link_device(dev); > } > Thanks, looks like simple way to reproduce the issue. I will give it a try. -- Regards, Sudeep
On Tue, Mar 11, 2025 at 11:23:12AM +0000, Sudeep Holla wrote: > On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote: > > > > So it is clear that wrong fw_devlink is created, it is because scmi cpufreq device is > > created earlier and when device_add, the below logic makes the fwnode pointer points > > to scmi cpufreq device. > > if (dev->fwnode && !dev->fwnode->dev) { > > dev->fwnode->dev = dev; > > fw_devlink_link_device(dev); > > } > > > > Thanks, looks like simple way to reproduce the issue. I will give it a try. > I could reproduce but none of my solution solved the problem completely or properly. And I don't like the DT proposal you came up with. I am not inclined to just drop this fwnode setting in the scmi devices and just use of_node.
On Wed, Mar 12, 2025 at 10:52:23AM +0000, Sudeep Holla wrote: > On Tue, Mar 11, 2025 at 11:23:12AM +0000, Sudeep Holla wrote: > > On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote: > > > > > > So it is clear that wrong fw_devlink is created, it is because scmi cpufreq device is > > > created earlier and when device_add, the below logic makes the fwnode pointer points > > > to scmi cpufreq device. > > > if (dev->fwnode && !dev->fwnode->dev) { > > > dev->fwnode->dev = dev; > > > fw_devlink_link_device(dev); > > > } > > > > > > > Thanks, looks like simple way to reproduce the issue. I will give it a try. > > > > I could reproduce but none of my solution solved the problem completely > or properly. And I don't like the DT proposal you came up with. I am > not inclined to just drop this fwnode setting in the scmi devices and > just use of_node. > Sorry for the typo that changes the meaning: s/not/now I meant "I am now inclined ..", until we figure out a way to make this work with devlinks properly.
On Wed, Mar 12, 2025 at 11:28:52AM +0000, Sudeep Holla wrote: >On Wed, Mar 12, 2025 at 10:52:23AM +0000, Sudeep Holla wrote: >> On Tue, Mar 11, 2025 at 11:23:12AM +0000, Sudeep Holla wrote: >> > On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote: >> > > >> > > So it is clear that wrong fw_devlink is created, it is because scmi cpufreq device is >> > > created earlier and when device_add, the below logic makes the fwnode pointer points >> > > to scmi cpufreq device. >> > > if (dev->fwnode && !dev->fwnode->dev) { >> > > dev->fwnode->dev = dev; >> > > fw_devlink_link_device(dev); >> > > } >> > > >> > >> > Thanks, looks like simple way to reproduce the issue. I will give it a try. >> > >> >> I could reproduce but none of my solution solved the problem completely >> or properly. And I don't like the DT proposal you came up with. I am >> not inclined to just drop this fwnode setting in the scmi devices and >> just use of_node. >> > >Sorry for the typo that changes the meaning: s/not/now > >I meant "I am now inclined ..", until we figure out a way to make this >work with devlinks properly. when you have time, please give a look at https://github.com/MrVan/linux/commit/b500c29cb7f6f32a38b1ed462e333db5a3e301e4 The upper patch was to follow Cristian's and Dan's suggestion in V2[1] to use a flag SCMI_DEVICE_NO_FWNODE for scmi device. I could post out the upper patch as V3 if it basically looks no design flaw. I will drop the pinctrl patch in v3, considering we are first going to resolve the fw_devlink issue for cpufreq/devfreq. [1] https://lore.kernel.org/all/Z6SgFGb4Z88v783c@pluto/ Thanks, Peng. > >-- >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);