Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode for scmi cpufreq

On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:

quoted

Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
for scmi cpufreq

On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:

quoted

quoted

Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set

fwnode

quoted

quoted

for scmi cpufreq

On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:

quoted

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

quoted

quoted

quoted

devices will be created. But the fwnode->dev could only point to
one

device.

quoted

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

quoted

quoted

quoted

But actually the performance domain user device, such as GPU,

should

quoted

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

The commit message itself seems very specific to some platform to

me.

quoted

quoted

What about platforms that don't atall have a GPU? Why would

they

quoted

quoted

care about this?

It is a generic issue if a platform has performance domain to serve
scmi cpufreq and device performance level.

quoted

quoted

device not ready.

Because for cpufreq, no need use fw_devlink. So bypass setting

fwnode

quoted

for scmi cpufreq device.

Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the
scmi_device")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Use A!=B to replace !(A == B)
 Add fixes tag
 This might be a workaround, but since this is a fix, it is simple
for backporting.

More than a workaround, it feels like a HACK to me.

quoted

V1:



 drivers/firmware/arm_scmi/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/bus.c

b/drivers/firmware/arm_scmi/bus.c index

96b2e5f9a8ef..be91a82e0cda

quoted

100644

--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c

@@ -395,7 +395,8 @@ __scmi_device_create(struct

device_node

quoted

quoted

*np, struct device *parent,

quoted

 	scmi_dev->id = id;
 	scmi_dev->protocol_id = protocol;
 	scmi_dev->dev.parent = parent;
-	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,

"cpufreq"))

quoted

+		device_set_node(&scmi_dev->dev,

of_fwnode_handle(np));

I kind of disagree with the idea here to be specific about the
PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver

right?

quoted

quoted

Why bring in specific code into a bus driver? We will never fix the
actual root cause of the issue this way.

The root cause is fwnode devlink only supports one fwnode, one

device.

quoted

1:1 match. But current arm scmi driver use one fwnode for two

devices.

quoted

If your platform has scmi cpufreq and scmi device performance

domain,

quoted

you might see that some devices are consumer of scmi cpufreq, but
actually they should be consumer of scmi device performance

domain.

quoted

I not have a good idea that this is fw devlink design that only allows
1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
If not, fw devlink should be updated.

The current patch is the simplest method for stable tree fixes as I
could work out.

So this is the same root cause at the end of the issues you had with
IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices that
embeds the protocol node, BUT since you can have multiple
device/drivers doing different things on different resources within the
same protocol you can end with 2 devices having the same embedded
device_node, since we dont really have anything else to use as
device_node, I rigth ?

I think, yes. And you remind me that with PINCTRL_SCMI and
CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
will only take one to set the fwnode device pointer depends on
the order to run __scmi_device_create.

So not only perf, pinctrl also has issue here, fwnode devlink will
not work properly for pinctrl/perf.

...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
mutually exclusive in the code (@probe time) itself as far as I can remember
about what you did, so why devlink should have that issue there ?
Have you seen any issue in this regards while having loaded pinctrl_scmi
and pinctrl_imx_scmi ?

I want to have a better look next days about this devlink issue that you
reported...it still not clear to me...from device_link_add() docs, it
seems indeed that it will return the old existing link if a link exist
already between that same supplier/consumer devices pair....but from the
code (at first sight) it seems that the check is made agains the devices
not their embeded device_nodes, but here (and in pinctrl/imx case) you will
have 2 distinct consumer devices (with same device_node)...I may have
missed something in your exaplanation....

Thanks,
Cristian