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

[Peng Fan <peng.fan@oss.nxp.com>]

On Thu, Apr 17, 2025 at 03:26:42PM +0100, Sudeep Holla wrote:
>On Wed, Apr 09, 2025 at 12:14:00PM +0100, Sudeep Holla wrote:
>> On Wed, Apr 09, 2025 at 11:50:29AM +0800, Peng Fan wrote:
>> > Hi Sudeep, Cristian
>> > 
>> > On Thu, Mar 13, 2025 at 01:23:27PM +0800, Peng Fan wrote:
>> > >On Wed, Mar 12, 2025 at 11:28:52AM +0000, Sudeep Holla wrote:
>> > >>On Wed, Mar 12, 2025 at 10:52:23AM +0000, Sudeep Holla wrote:
>> > >>> On Tue, Mar 11, 2025 at 11:23:12AM +0000, Sudeep Holla wrote:
>> > >>> > On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote:
>> > >>> > >
>> > >>> > > So it is clear that wrong fw_devlink is created, it is because scmi cpufreq device is
>> > >>> > > created earlier and when device_add, the below logic makes the fwnode pointer points
>> > >>> > > to scmi cpufreq device.
>> > >>> > >         if (dev->fwnode && !dev->fwnode->dev) {
>> > >>> > >                 dev->fwnode->dev = dev;
>> > >>> > >                 fw_devlink_link_device(dev);
>> > >>> > >         }
>> > >>> > >
>> > >>> >
>> > >>> > Thanks, looks like simple way to reproduce the issue. I will give it a try.
>> > >>> >
>> > >>> 
>> > >>> I could reproduce but none of my solution solved the problem completely
>> > >>> or properly. And I don't like the DT proposal you came up with. I am
>> > >>> not inclined to just drop this fwnode setting in the scmi devices and
>> > >>> just use of_node.
>> > >>>
>> > >>
>> > >>Sorry for the typo that changes the meaning: s/not/now
>> > >>
>> > >>I meant "I am now inclined ..", until we figure out a way to make this
>> > >>work with devlinks properly.
>> > >
>> > >when you have time, please give a look at
>> > >https://github.com/MrVan/linux/commit/b500c29cb7f6f32a38b1ed462e333db5a3e301e4
>> > >
>> > >The upper patch was to follow Cristian's and Dan's suggestion in V2[1] to use
>> > >a flag SCMI_DEVICE_NO_FWNODE for scmi device.
>> > >
>> > >I could post out the upper patch as V3 if it basically looks no design flaw.
>> > >I will drop the pinctrl patch in v3, considering we are first going
>> > >to resolve the fw_devlink issue for cpufreq/devfreq.
>> > >
>> > >[1] https://lore.kernel.org/all/Z6SgFGb4Z88v783c@pluto/
>> > 
>> > Not sure you gave a look on this or not. I am thinking to bring this V3
>> > out to mailing list later this week. Please raise if you have any concern.
>> > 
>> 
>> Yes I had some thoughts. I will take a look and refresh my memories first.
>> 
>
>OK, I will post it separately(may be next week) but I wanted this way.

Thanks.

>Revert to old behaviour and driver request fw_devlink dependencies to
>be created if they rely on them. I am not sure if that is better approach.

This requires to update various drivers(clk,power,perf,pinctrl,regulator) to
set the flag SCMI_DEV_SET_FWNODE.

Using SCMI_DEV_NO_FWNODE would avoid updating the various drivers.

Anyway, you decide which to go :)

Thanks,
Peng

>
>Regards,
>Sudeep
>
>
>-->8
>
>From: Sudeep Holla <sudeep.holla@arm.com>
>Date: Thu, 17 Apr 2025 10:59:10 +0100
>Subject: [PATCH] firmware: arm_scmi: Add flag to control setting of fwnode
> handle
>
>Currently, when multiple SCMI devices share the same protocol,
>their fwnode->dev all reference the same device tree node. Depending
>on the order of device creation, fwnode->dev ends up pointing to one
>of the SCMI devices, causing fw_devlink to incorrectly establish
>supplier-consumer relationships treating the first-created device as
>the supplier for all others.
>
>To address this, introduce a flag that enables explicit control over
>whether the fwnode handle should be set. This allows only those devices
>that require fw_devlink support to request it explicitly.
>
>By default, only the of_node is set, which is sufficient for most SCMI
>drivers.
>
>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>---
> drivers/firmware/arm_scmi/bus.c    | 19 ++++++++++++-------
> drivers/firmware/arm_scmi/common.h |  2 +-
> drivers/firmware/arm_scmi/driver.c | 12 ++++++------
> include/linux/scmi_protocol.h      |  4 ++++
> 4 files changed, 23 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
>index 1adef0389475..eeab5de03a1e 100644
>--- a/drivers/firmware/arm_scmi/bus.c
>+++ b/drivers/firmware/arm_scmi/bus.c
>@@ -389,7 +389,7 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> 
> static struct scmi_device *
> __scmi_device_create(struct device_node *np, struct device *parent,
>-		     int protocol, const char *name)
>+		     int protocol, const char *name, u32 flags)
> {
> 	int id, retval;
> 	struct scmi_device *scmi_dev;
>@@ -439,11 +439,15 @@ __scmi_device_create(struct device_node *np, struct device *parent,
> 	scmi_dev->id = id;
> 	scmi_dev->protocol_id = protocol;
> 	scmi_dev->dev.parent = parent;
>-	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> 	scmi_dev->dev.bus = &scmi_bus_type;
> 	scmi_dev->dev.release = scmi_device_release;
> 	dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);
> 
>+	if (flags & SCMI_DEV_SET_FWNODE)
>+		device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
>+	else
>+		scmi_dev->dev.of_node = np;
>+
> 	retval = device_register(&scmi_dev->dev);
> 	if (retval)
> 		goto put_dev;
>@@ -461,11 +465,11 @@ __scmi_device_create(struct device_node *np, struct device *parent,
> 
> static struct scmi_device *
> _scmi_device_create(struct device_node *np, struct device *parent,
>-		    int protocol, const char *name)
>+		    int protocol, const char *name, u32 flags)
> {
> 	struct scmi_device *sdev;
> 
>-	sdev = __scmi_device_create(np, parent, protocol, name);
>+	sdev = __scmi_device_create(np, parent, protocol, name, flags);
> 	if (!sdev)
> 		pr_err("(%s) Failed to create device for protocol 0x%x (%s)\n",
> 		       of_node_full_name(parent->of_node), protocol, name);
>@@ -498,14 +502,14 @@ _scmi_device_create(struct device_node *np, struct device *parent,
>  */
> struct scmi_device *scmi_device_create(struct device_node *np,
> 				       struct device *parent, int protocol,
>-				       const char *name)
>+				       const char *name, u32 flags)
> {
> 	struct list_head *phead;
> 	struct scmi_requested_dev *rdev;
> 	struct scmi_device *scmi_dev = NULL;
> 
> 	if (name)
>-		return _scmi_device_create(np, parent, protocol, name);
>+		return _scmi_device_create(np, parent, protocol, name, flags);
> 
> 	mutex_lock(&scmi_requested_devices_mtx);
> 	phead = idr_find(&scmi_requested_devices, protocol);
>@@ -521,7 +525,8 @@ struct scmi_device *scmi_device_create(struct device_node *np,
> 
> 		sdev = _scmi_device_create(np, parent,
> 					   rdev->id_table->protocol_id,
>-					   rdev->id_table->name);
>+					   rdev->id_table->name,
>+					   rdev->id_table->flags);
> 		if (sdev)
> 			scmi_dev = sdev;
> 	}
>diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
>index dab758c5fdea..c948c4d88332 100644
>--- a/drivers/firmware/arm_scmi/common.h
>+++ b/drivers/firmware/arm_scmi/common.h
>@@ -151,7 +151,7 @@ extern struct blocking_notifier_head scmi_requested_devices_nh;
> 
> struct scmi_device *scmi_device_create(struct device_node *np,
> 				       struct device *parent, int protocol,
>-				       const char *name);
>+				       const char *name, u32 flags);
> void scmi_device_destroy(struct device *parent, int protocol, const char *name);
> 
> int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id);
>diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>index f6c9e4491240..433b057ec0d9 100644
>--- a/drivers/firmware/arm_scmi/driver.c
>+++ b/drivers/firmware/arm_scmi/driver.c
>@@ -436,11 +436,11 @@ EXPORT_SYMBOL_GPL(scmi_protocol_unregister);
>  *	  for the specified protocol.
>  */
> static void scmi_create_protocol_devices(struct device_node *np,
>-					 struct scmi_info *info,
>-					 int prot_id, const char *name)
>+					 struct scmi_info *info, int prot_id,
>+					 const char *name, u32 flags)
> {
> 	mutex_lock(&info->devreq_mtx);
>-	scmi_device_create(np, info->dev, prot_id, name);
>+	scmi_device_create(np, info->dev, prot_id, name, flags);
> 	mutex_unlock(&info->devreq_mtx);
> }
> 
>@@ -2668,7 +2668,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
> 	snprintf(name, 32, "__scmi_transport_device_%s_%02X",
> 		 idx ? "rx" : "tx", prot_id);
> 	/* Create a uniquely named, dedicated transport device for this chan */
>-	tdev = scmi_device_create(of_node, info->dev, prot_id, name);
>+	tdev = scmi_device_create(of_node, info->dev, prot_id, name, 0);
> 	if (!tdev) {
> 		dev_err(info->dev,
> 			"failed to create transport device (%s)\n", name);
>@@ -2865,7 +2865,7 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
> 	switch (action) {
> 	case SCMI_BUS_NOTIFY_DEVICE_REQUEST:
> 		scmi_create_protocol_devices(np, info, id_table->protocol_id,
>-					     id_table->name);
>+					     id_table->name, id_table->flags);
> 		break;
> 	case SCMI_BUS_NOTIFY_DEVICE_UNREQUEST:
> 		scmi_destroy_protocol_devices(info, id_table->protocol_id,
>@@ -3244,7 +3244,7 @@ static int scmi_probe(struct platform_device *pdev)
> 		}
> 
> 		of_node_get(child);
>-		scmi_create_protocol_devices(child, info, prot_id, NULL);
>+		scmi_create_protocol_devices(child, info, prot_id, NULL, 0);
> 	}
> 
> 	return 0;
>diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
>index 688466a0e816..2546b7977fe3 100644
>--- a/include/linux/scmi_protocol.h
>+++ b/include/linux/scmi_protocol.h
>@@ -947,9 +947,13 @@ struct scmi_device {
> 
> #define to_scmi_dev(d) container_of_const(d, struct scmi_device, dev)
> 
>+/* The scmi device needs fwnode handle */
>+#define SCMI_DEV_SET_FWNODE		BIT(0)
>+
> struct scmi_device_id {
> 	u8 protocol_id;
> 	const char *name;
>+	u32 flags;
> };
> 
> struct scmi_driver {
>-- 
>2.34.1
>