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

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

On Fri, Dec 27, 2024 at 03:13:06PM +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>

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

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

one device.

quoted

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

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

should

quoted

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 device not ready.

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

Hi,

quoted

quoted

Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the
scmi_device")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

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

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

157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb654845
43044b442

quoted

quoted

4fbe3b67245466 100644

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

@@ -345,6 +345,19 @@ static void __scmi_device_destroy(struct

scmi_device *scmi_dev)

quoted

quoted

 	device_unregister(&scmi_dev->dev);
 }

+static int
+__scmi_device_set_node(struct scmi_device *scmi_dev, struct

device_node *np,

quoted

quoted

+		       int protocol, const char *name) {
+	/* cpufreq device does not need to be supplier from devlink

perspective */

quoted

quoted

+	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,

"cpufreq"))

quoted

quoted

+		return 0;

This is just a assumption based on current implementation. What
happens if this is needed. Infact, it is used in the current
implementation to create a dummy clock provider, so for sure with

this

quoted

change that will break IMO.

I agree with Sudeep on this: if you want to exclude some SCMI device
from the fw_devlink handling to address the issues with multiple SCMI
devices created on the same protocol nodes, cant we just flag this
requirement here and avoid to call device_link_add in
driver:scmi_set_handle(), instead of killing completely any possibility of
referencing phandles (and having device_link_add failing as a
consequence of having a NULL supplier)

i.e. something like:

@bus.c
------
static int
__scmi_device_set_node(struct scmi_device *scmi_dev, struct
device_node *np,
		       int protocol, const char *name) {
	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
"cpufreq"))
		scmi_dev->avoid_devlink = true;

	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
	....


and @driver.c
-------------

static void scmi_set_handle(struct scmi_device *scmi_dev) {
	scmi_dev->handle = scmi_handle_get(&scmi_dev->dev);
	if (scmi_dev->handle && !scmi_dev->avoid_devlink)
		scmi_device_link_add(&scmi_dev->dev, scmi_dev-

quoted

handle->dev); }

.... so that you can avoid fw_devlink BUT keep the device_node NON-
null for the device.

This would mean also restoring the pre-existing explicit blacklisting in
pinctrl-imx to avoid issues when pinctrl subsystem searches by
device_node...

..or I am missing something ?

link_ret = device_links_check_suppliers(dev); to check fw_devlink
is before "ret = driver_sysfs_add(dev);" which
issue bus notify.

The link is fw_devlink, the devlink is created in 'device_add'
        if (dev->fwnode && !dev->fwnode->dev) {                                                     
                dev->fwnode->dev = dev;                                                             
                fw_devlink_link_device(dev);                                                        
        }
The check condition is fwnode.

I think scmi_dev->avoid_devlink not help here.

Thanks,
Peng 

Thanks,
Cristian