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

From: Peng Fan <peng.fan@nxp.com>
Date: 2024-09-02 12:22:30
Also in: arm-scmi, lkml

Hi Saravana,

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

quoted

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

for

quoted

scmi cpufreq

On Wed, Aug 14, 2024 at 12:04 AM Peng Fan [off-list ref]
wrote:

quoted

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

fwnode

quoted

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

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

On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
From: Peng Fan <peng.fan@nxp.com>

Two drivers scmi_cpufreq.c and

scmi_perf_domain.c

quoted

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

name,

quoted

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

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

quoted

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

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
device not ready.

Because for cpufreq, no need use fw_devlink. So
bypass
setting
quoted
quoted
quoted
quoted
fwnode
quoted
for scmi cpufreq device.

Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set

fwnode

quoted

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

quoted

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.

quoted

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

quoted

quoted
dont
quoted
quoted
quoted
quoted
quoted
really
have
quoted
quoted
quoted
anything else to
use
quoted
quoted
as device_node, I rigth ?
I think, yes. And you remind me that with PINCTRL_SCMI

and

quoted

quoted
quoted
quoted
quoted
quoted
CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi

pinctrl

quoted

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

quoted

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

quoted

PINCTRL_IMX_SCMI both enabled, two scmi devices will be

created.

quoted

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

quoted

multiple devices getting instantiated from one DT node. The

problem

quoted

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

quoted

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.

quoted

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?

quoted

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

quoted

successfully.

Why?! Isn't this the whole point of compatible strings and being able
to list multiple compatible strings for a DT node?

The scmi devices are actually not populated from DT node, they are
create from driver according to a table if the reg(protocol_id) exists in
DT, see drivers/firmware/arm_scmi/bus.c "
                sdev = __scmi_device_create(np, parent,
                                            rdev->id_table->protocol_id,
                                            rdev->id_table->name); "

For compatible strings, I could not comment on the design. Cristian
seems replied in the other mail.

quoted

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

 If yes,

quoted

why are we not using a child node or a separate node for this

second

quoted

device? If it's always one or the other, why are we creating two

devices?

quoted

Sudeep and Cristian may comment on the design goal of scmi

drivers.

quoted

Can you please point to specific upstream DT examples for me to

get

quoted

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

Sure, I can see that file and these nodes, but what am I supposed to
make of it? Can you be more specific please?

Sorry for not being clear.

 I'm not familiar with the

quoted

protocol number mapping.

The ID is in include/linux/scmi_protocol.h, line 918.

 Which nodes are the consumers in this

quoted

example?

For " protocol@13", it is A55 cpu core with " power-domains =
<&scmi_perf IMX95_PERF_A55>;" and other devices(GPU/VPU) that
not in upstream tree as of now.

 Which node has more than one device created? Both of

quoted

these?

Yes, both.
drivers/firmware/arm_scmi/bus.c
__scmi_device_create will create device according to "static const
struct scmi_device_id scmi_id_table", so for protocol@19, It will create
two devices, one is "{ SCMI_PROTOCOL_PINCTRL, "pinctrl" }", The other
is " { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" }, ", both points to the
node protocol@19, see "device_set_node(&scmi_dev->dev,
of_fwnode_handle(np)); " in "__scmi_device_create".

quoted

Can you also point me to specific examples for the pinctrl thing you
mention above?

As above, two devices are created, both points to node protocol@19,
but fwnode protocol@19 will only have 1st device as supplier.

quoted

I'll take a closer look tomorrow.

Do you have a chance to look into the scmi stuff?

Thanks,
Peng.

Thanks,
Peng.

quoted

-Saravana

quoted

Btw, there is the deferred_probe_timeout command line option

that

quoted

can be used so that fw_devlink stops enforcing dependencies

where

quoted

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

quoted

there is more than one device instantiated for a given supplier DT

node.

quoted

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

Thanks,
Peng.

quoted

-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