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

From: Peng Fan <peng.fan@nxp.com>
Date: 2024-08-14 07:04:29
Also in: arm-scmi, lkml

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

On Tue, Aug 13, 2024 at 8:36 PM Peng Fan [off-list ref]
wrote:

quoted

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

fwnode

quoted

for scmi cpufreq

quoted

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

fwnode

quoted

for

quoted

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

quoted

for scmi cpufreq

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

quoted

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

fwnode

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

quoted

quoted
use
quoted
quoted
quoted
quoted
quoted
quoted
SCMI_PROTCOL_PERF protocol, but with different name,

so

quoted

two

quoted

scmi

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

But actually the performance domain user device, such

as

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
GPU,
should
quoted
use the scmi perf device as supplier. Also if

'cpufreq.off=1'

quoted

in bootargs, the GPU driver will defer probe always,
because of the scmi cpufreq

The commit message itself seems very specific to some

platform

quoted

quoted
quoted
quoted
quoted
to
me.
quoted
quoted
What about platforms that don't atall have a GPU? Why

would

quoted

they

quoted

care about this?

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

quoted

device not ready.

Because for cpufreq, no need use fw_devlink. So bypass

setting

quoted

fwnode

quoted

for scmi cpufreq device.

Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode

for

quoted

the

quoted

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

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

*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));

quoted

quoted
quoted
quoted
quoted
+ if ((protocol != SCMI_PROTOCOL_PERF) ||

strcmp(name,

quoted

"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

quoted

right?

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

quoted

actually they should be consumer of scmi device

performance

quoted

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.

quoted

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

quoted

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

quoted

quoted
quoted
really

have

quoted

anything else to

use

quoted

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

quoted

will

quoted

only

quoted

take one to set the fwnode device pointer depends on the order
to

run

quoted

__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

quoted

issue there ?
Have you seen any issue in this regards while having loaded
pinctrl_scmi and pinctrl_imx_scmi ?

No. it works well in my setup. I am just worried that there might be
issues because fwnode only has one dev pointer, see device_add.

quoted

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

quoted

from the code (at first sight) it seems that the check is made
agains the devices not their embeded device_nodes, but here

(and

quoted

in pinctrl/imx case) you will have 2 distinct consumer devices
(with

same

quoted

device_node)...I may have missed something in your

exaplanation....

quoted

It might be false alarm for pinctrl, but it is true issue for perf.

Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI and
PINCTRL_IMX_SCMI both enabled, two scmi devices will be created.

So it

quoted

depends on device creation order, 1st created device will be used as
supplier.

On i.MX, there is no issue with both enabled, it is because the imx
scmi device is created first. If the generic pinctrl scmi device
created first, imx will have issue.

In the end, this is generic fw_devlink limitation or arm scmi driver
issue.

+1 to what Cristian and Dhruva said in other parts of this thread.

This patch itself is definitely a hack.

The problem isn't so much that fw_devlink doesn't want to support
multiple devices getting instantiated from one DT node. The problem is
that there's no way to know which of the multiple devices is the real
supplier just by looking at the information in devicetree/firmware (the
fw in fw_devlink).

Yes. I see.

And keep in mind that one of the main requirements

of fw_devlink is to work before any driver is loaded and not depend on
drivers for correctness of the dependency information because it needs
to work on a fully modular kernel too. So, fw_devlink just picks the first
device that's instantiated from a DT node.

I really hate folks creating multiple devices from one DT node. One IP
block can support multiple things, there's no need to instantiate
multiple devices for it. The same driver could have just as easily
registered with multiple frameworks. So, ideally I'd want us to fix this
issue in the SCMI framework code. In the case where the same SCMI
node is creating two devices, can they both probe successfully?

In pinctrl case, only one could probe successfully.
See
drivers/pinctrl/pinctrl-scmi.c for generic pinctrl scmi driver
drivers/pinctrl/freescale/pinctrl-imx-scmi.c for i.MX extension.

There is a machine compatible string to make sure only one
could probe successfully.

In scmi performance case, both could probe successfully.
drivers/cpufreq/scmi-cpufreq.c
drivers/pmdomain/arm/scmi_perf_domain.c

 If yes,

why are we not using a child node or a separate node for this second
device? If it's always one or the other, why are we creating two devices?

Sudeep and Cristian may comment on the design goal of scmi drivers.

Can you please point to specific upstream DT examples for me to get a
better handle on what's going on?

See linux-next tree:
arch/arm64/boot/dts/freescale/imx95.dtsi

                        scmi_perf: protocol@13 {                                                    
                                reg = <0x13>;                                                       
                                #power-domain-cells = <1>;                                          
                        };

                        scmi_iomuxc: protocol@19 {                                                  
                                reg = <0x19>;                                                       
                        };

Btw, there is the deferred_probe_timeout command line option that
can be used so that fw_devlink stops enforcing dependencies where
there are no supplier drivers for a device after a timeout. It's not ideal,
but it's something to unblock you.

The best fw_devlink could do is just not enforce any dependencies if
there is more than one device instantiated for a given supplier DT node.

You mean not enforce fw_devlink for all or this could only apply to
specific nodes or devices?

Thanks,
Peng.

-Saravana

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help