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

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:

quoted

On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla [off-list ref] wrote:

quoted

Hi Saravana,

quoted

quoted

On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote:

quoted

On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:

quoted

On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:

quoted

From: Peng Fan <peng.fan@nxp.com>

[snip]

quoted

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/ (local)

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

quoted

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) 

quoted

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

quoted

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