Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting fwnode for scmi cpufreq

[Peng Fan <peng.fan@xxxxxxxxxxx>]

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@xxxxxxx> 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@xxxxxxx>
>> > > >>
>
>[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