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

[Cristian Marussi <cristian.marussi@xxxxxxx>]

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...

Thanks,
Cristian