[PATCH 0/4] scmi: Bypass set fwnode to address devlink issue

[PATCH 0/4] scmi: Bypass set fwnode to address devlink issue

[Peng Fan (OSS)]

Current scmi drivers not work well with devlink. This patchset is a
retry to address the issue in [1] which was a few months ago.

Current scmi devices are not created from device tree, they are created
from a scmi_device_id entry of each driver with the protocol matches
with the fwnode reg value, this means there could be multiple devices created
for one fwnode, but the fwnode only has one device pointer.

This patchset is to do more checking before setting the device fwnode.

This may looks like hack, but seems no better way to make scmi works
well with devlink.

[1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/

Cc: 

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (4):
      firmware: arm_scmi: bus: Bypass setting fwnode for scmi cpufreq
      firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl
      pinctrl: scmi: Check fwnode instead of machine compatible
      pinctrl: freescale: scmi: Check fwnode instead of machine compatible

 drivers/firmware/arm_scmi/bus.c              | 29 +++++++++++++++++++++++++++-
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c |  7 +------
 drivers/pinctrl/pinctrl-scmi.c               |  7 +------
 3 files changed, 30 insertions(+), 13 deletions(-)
---
base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2
change-id: 20241225-scmi-fwdevlink-afb5131f19ea

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

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

[Peng Fan (OSS)]

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 devices
will be created. But the fwnode->dev could only point to one device.

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).
But actually the performance domain user device, such as GPU, should 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.

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..12190d4dabb65484543044b4424fbe3b67245466 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)
 	device_unregister(&scmi_dev->dev);
 }
 
+static int
+__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
+		       int protocol, const char *name)
+{
+	/* cpufreq device does not need to be supplier from devlink perspective */
+	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
+		return 0;
+
+	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+
+	return 0;
+}
+
 static struct scmi_device *
 __scmi_device_create(struct device_node *np, struct device *parent,
 		     int protocol, const char *name)
@@ -397,7 +410,7 @@ __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_device_set_node(scmi_dev, np, protocol, name);
 	scmi_dev->dev.bus = &scmi_bus_type;
 	scmi_dev->dev.release = scmi_device_release;
 	dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);

-- 
2.37.1

[PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
If both drivers are built in, and the scmi device with name "pinctrl-imx"
is created earlier, and the fwnode device points to the scmi device,
non-i.MX platforms will never have the pinctrl supplier ready.

So bypass setting fwnode for scmi pinctrl devices that non
compatible with socs.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/bus.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 12190d4dabb65484543044b4424fbe3b67245466..87665b09c8ff492953c8300f80ed73eab6cce4fd 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -345,6 +345,11 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
 	device_unregister(&scmi_dev->dev);
 }
 
+static const char * const scmi_pinctrl_imx_lists[] = {
+	"fsl,imx95",
+	NULL
+};
+
 static int
 __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
 		       int protocol, const char *name)
@@ -353,6 +358,15 @@ __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
 	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
 		return 0;
 
+	if (protocol == SCMI_PROTOCOL_PINCTRL) {
+		if (!strcmp(name, "pinctrl") &&
+		    of_machine_compatible_match(scmi_pinctrl_imx_lists))
+			return 0;
+		if (!strcmp(name, "pinctrl-imx") &&
+		    !of_machine_compatible_match(scmi_pinctrl_imx_lists))
+			return 0;
+	}
+
 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
 
 	return 0;

-- 
2.37.1

[PATCH 3/4] pinctrl: scmi: Check fwnode instead of machine compatible

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

For the platform that not compatible with scmi pinctrl device, the
fwnode will not be set, so checking fwnode will make code simpler
and easy to maintain.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/pinctrl-scmi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391741e77c0aac3f029991e4bb 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
 	return 0;
 }
 
-static const char * const scmi_pinctrl_blocklist[] = {
-	"fsl,imx95",
-	NULL
-};
-
 static int scmi_pinctrl_probe(struct scmi_device *sdev)
 {
 	int ret;
@@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
 	if (!sdev->handle)
 		return -EINVAL;
 
-	if (of_machine_compatible_match(scmi_pinctrl_blocklist))
+	if (!dev->fwnode)
 		return -ENODEV;
 
 	handle = sdev->handle;

-- 
2.37.1

[PATCH 4/4] pinctrl: freescale: scmi: Check fwnode instead of machine compatible

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

For the platform that not compatible with scmi pinctrl device, the
fwnode will not be set, so checking fwnode will make code simpler
and easy to maintain.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
index 8f15c4c4dc4412dddb40505699fc3f459fdc0adc..5277d30af7084b9bbf83e3523f09c8136d41705b 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
@@ -287,11 +287,6 @@ scmi_pinctrl_imx_get_pins(struct scmi_pinctrl_imx *pmx, struct pinctrl_desc *des
 	return 0;
 }
 
-static const char * const scmi_pinctrl_imx_allowlist[] = {
-	"fsl,imx95",
-	NULL
-};
-
 static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 {
 	struct device *dev = &sdev->dev;
@@ -304,7 +299,7 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 	if (!handle)
 		return -EINVAL;
 
-	if (!of_machine_compatible_match(scmi_pinctrl_imx_allowlist))
+	if (!dev->fwnode)
 		return -ENODEV;
 
 	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);

-- 
2.37.1

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

[Sudeep Holla]

On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> 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 devices
> will be created. But the fwnode->dev could only point to one device.
>
> 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).
> But actually the performance domain user device, such as GPU, should 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.
>
> 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..12190d4dabb65484543044b4424fbe3b67245466 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)
>  	device_unregister(&scmi_dev->dev);
>  }
>
> +static int
> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> +		       int protocol, const char *name)
> +{
> +	/* cpufreq device does not need to be supplier from devlink perspective */
> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
> +		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 change that will
break IMO.

--
Regards,
Sudeep

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Sudeep Holla]

On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> If both drivers are built in, and the scmi device with name "pinctrl-imx"
> is created earlier, and the fwnode device points to the scmi device,
> non-i.MX platforms will never have the pinctrl supplier ready.
>

I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
i.MX platforms instead of this hack which IMO is little less hackier
(and little more cleaner as we don't create problem and then fix here)
than this change.

--
Regards,
Sudeep

Re: [PATCH 3/4] pinctrl: scmi: Check fwnode instead of machine compatible

[Sudeep Holla]

On Wed, Dec 25, 2024 at 04:20:46PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> For the platform that not compatible with scmi pinctrl device, the
> fwnode will not be set, so checking fwnode will make code simpler
> and easy to maintain.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/pinctrl/pinctrl-scmi.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391741e77c0aac3f029991e4bb 100644
> --- a/drivers/pinctrl/pinctrl-scmi.c
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
>  	return 0;
>  }
>  
> -static const char * const scmi_pinctrl_blocklist[] = {
> -	"fsl,imx95",
> -	NULL
> -};
> -
>  static int scmi_pinctrl_probe(struct scmi_device *sdev)
>  {
>  	int ret;
> @@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
>  	if (!sdev->handle)
>  		return -EINVAL;
>  
> -	if (of_machine_compatible_match(scmi_pinctrl_blocklist))
> +	if (!dev->fwnode)

I would prefer to see the blocklist to be explicit here rather than
implicitly hiding it away with this change set.

-- 
Regards,
Sudeep

Re: [PATCH 0/4] scmi: Bypass set fwnode to address devlink issue

[Linus Walleij]

On Wed, Dec 25, 2024 at 9:21 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> Current scmi drivers not work well with devlink. This patchset is a
> retry to address the issue in [1] which was a few months ago.
>
> Current scmi devices are not created from device tree, they are created
> from a scmi_device_id entry of each driver with the protocol matches
> with the fwnode reg value, this means there could be multiple devices created
> for one fwnode, but the fwnode only has one device pointer.
>
> This patchset is to do more checking before setting the device fwnode.
>
> This may looks like hack, but seems no better way to make scmi works
> well with devlink.
>
> [1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/

Please drive any devlink-related patches by Saravana Kannan, he's pretty
much the only person I trust to know how to do devlinks right.

Yours,
Linus Walleij

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

[Peng Fan]

On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote:
>On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
>> 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 devices
>> will be created. But the fwnode->dev could only point to one device.
>>
>> 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).
>> But actually the performance domain user device, such as GPU, should 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.
>>
>> 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..12190d4dabb65484543044b4424fbe3b67245466 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)
>>  	device_unregister(&scmi_dev->dev);
>>  }
>>
>> +static int
>> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>> +		       int protocol, const char *name)
>> +{
>> +	/* cpufreq device does not need to be supplier from devlink perspective */
>> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
>> +		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 change that will
>break IMO.

If cpufreq needs the deivce_node, it will be parsed as a supplier from
devlink view. Then come to the issue, multiple scmi devices match one
of node, Saravana replied before in below thread

https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/

So quote here
"
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.
"

Since we are here that fw_devlink not support multiple devices match one
of node and will not support(per my understanding), what scmi part could
do is: only set of node to the scmi device that needs to be supplier

Or we introduce compatible string to scmi node, and subnodes if
one protocol supports mutilple function, such as cpufreq and device performance
using PERF protocol. But this needs big change to scmi framework.

Thanks
Peng

>
>--
>Regards,
>Sudeep

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Peng Fan]

On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
>On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
>> If both drivers are built in, and the scmi device with name "pinctrl-imx"
>> is created earlier, and the fwnode device points to the scmi device,
>> non-i.MX platforms will never have the pinctrl supplier ready.
>>
>
>I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
>i.MX platforms instead of this hack which IMO is little less hackier
>(and little more cleaner as we don't create problem and then fix here)
>than this change.

I thought two ways that introduce new entries in scmi_device_id,
1. compatible string.
2. allowed machine string and blcoked machine string.

Thanks,
Peng
>
>--
>Regards,
>Sudeep

Re: [PATCH 0/4] scmi: Bypass set fwnode to address devlink issue

[Peng Fan]

On Fri, Dec 27, 2024 at 06:06:24PM +0100, Linus Walleij wrote:
>On Wed, Dec 25, 2024 at 9:21 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
>> Current scmi drivers not work well with devlink. This patchset is a
>> retry to address the issue in [1] which was a few months ago.
>>
>> Current scmi devices are not created from device tree, they are created
>> from a scmi_device_id entry of each driver with the protocol matches
>> with the fwnode reg value, this means there could be multiple devices created
>> for one fwnode, but the fwnode only has one device pointer.
>>
>> This patchset is to do more checking before setting the device fwnode.
>>
>> This may looks like hack, but seems no better way to make scmi works
>> well with devlink.
>>
>> [1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/
>
>Please drive any devlink-related patches by Saravana Kannan, he's pretty
>much the only person I trust to know how to do devlinks right.

Quote Saravana's conclution[1] here:
"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."

So I think for systems using scmi could not rely on devlink to build
supplier/consumer to make driver probe in order.

[1]https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/

Thanks,
Peng
>
>Yours,
>Linus Walleij

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

[Cristian Marussi]

On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote:
> On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > 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 devices
> > will be created. But the fwnode->dev could only point to one device.
> >
> > 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).
> > But actually the performance domain user device, such as GPU, should 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,

> > 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..12190d4dabb65484543044b4424fbe3b67245466 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)
> >  	device_unregister(&scmi_dev->dev);
> >  }
> >
> > +static int
> > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> > +		       int protocol, const char *name)
> > +{
> > +	/* cpufreq device does not need to be supplier from devlink perspective */
> > +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
> > +		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 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->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 ?

Thanks,
Cristian

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Cristian Marussi]

On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> If both drivers are built in, and the scmi device with name "pinctrl-imx"
> is created earlier, and the fwnode device points to the scmi device,
> non-i.MX platforms will never have the pinctrl supplier ready.
> 
> So bypass setting fwnode for scmi pinctrl devices that non
> compatible with socs.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 12190d4dabb65484543044b4424fbe3b67245466..87665b09c8ff492953c8300f80ed73eab6cce4fd 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -345,6 +345,11 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
>  	device_unregister(&scmi_dev->dev);
>  }
>  
> +static const char * const scmi_pinctrl_imx_lists[] = {
> +	"fsl,imx95",
> +	NULL
> +};
> +
>  static int
>  __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>  		       int protocol, const char *name)
> @@ -353,6 +358,15 @@ __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>  	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
>  		return 0;
>  
> +	if (protocol == SCMI_PROTOCOL_PINCTRL) {
> +		if (!strcmp(name, "pinctrl") &&
> +		    of_machine_compatible_match(scmi_pinctrl_imx_lists))
> +			return 0;
> +		if (!strcmp(name, "pinctrl-imx") &&
> +		    !of_machine_compatible_match(scmi_pinctrl_imx_lists))
> +			return 0;
> +	}

...and same here, you could set a flag scmi_dev->avoid_devlink and
just avoid calling device_link_add instead of killing the device_node...

Thanks,
Cristian

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Cristian Marussi]

On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
> On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> > If both drivers are built in, and the scmi device with name "pinctrl-imx"
> > is created earlier, and the fwnode device points to the scmi device,
> > non-i.MX platforms will never have the pinctrl supplier ready.
> >
> 
> I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
> i.MX platforms instead of this hack which IMO is little less hackier
> (and little more cleaner as we don't create problem and then fix here)
> than this change.

...or indeed this is another possibility

Thanks,
Cristian

Re: [PATCH 3/4] pinctrl: scmi: Check fwnode instead of machine compatible

[Cristian Marussi]

On Fri, Dec 27, 2024 at 03:30:20PM +0000, Sudeep Holla wrote:
> On Wed, Dec 25, 2024 at 04:20:46PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > For the platform that not compatible with scmi pinctrl device, the
> > fwnode will not be set, so checking fwnode will make code simpler
> > and easy to maintain.
> > 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/pinctrl/pinctrl-scmi.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> > index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391741e77c0aac3f029991e4bb 100644
> > --- a/drivers/pinctrl/pinctrl-scmi.c
> > +++ b/drivers/pinctrl/pinctrl-scmi.c
> > @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> >  	return 0;
> >  }
> >  
> > -static const char * const scmi_pinctrl_blocklist[] = {
> > -	"fsl,imx95",
> > -	NULL
> > -};
> > -
> >  static int scmi_pinctrl_probe(struct scmi_device *sdev)
> >  {
> >  	int ret;
> > @@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
> >  	if (!sdev->handle)
> >  		return -EINVAL;
> >  
> > -	if (of_machine_compatible_match(scmi_pinctrl_blocklist))
> > +	if (!dev->fwnode)
> 
> I would prefer to see the blocklist to be explicit here rather than
> implicitly hiding it away with this change set.

Using a flag to inhibit device_link_add as said early in the series this
could be dropped and kept as is, I suppose.

Thanks,
Cristian

RE: [PATCH 3/4] pinctrl: scmi: Check fwnode instead of machine compatible

[Peng Fan]

> Subject: Re: [PATCH 3/4] pinctrl: scmi: Check fwnode instead of
> machine compatible
> 
> On Fri, Dec 27, 2024 at 03:30:20PM +0000, Sudeep Holla wrote:
> > On Wed, Dec 25, 2024 at 04:20:46PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > For the platform that not compatible with scmi pinctrl device, the
> > > fwnode will not be set, so checking fwnode will make code simpler
> > > and easy to maintain.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/pinctrl/pinctrl-scmi.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/pinctrl-scmi.c
> > > b/drivers/pinctrl/pinctrl-scmi.c index
> > >
> df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..aade6df77dbb2c391
> 741e77c0a
> > > ac3f029991e4bb 100644
> > > --- a/drivers/pinctrl/pinctrl-scmi.c
> > > +++ b/drivers/pinctrl/pinctrl-scmi.c
> > > @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct
> scmi_pinctrl *pmx,
> > >  	return 0;
> > >  }
> > >
> > > -static const char * const scmi_pinctrl_blocklist[] = {
> > > -	"fsl,imx95",
> > > -	NULL
> > > -};
> > > -
> > >  static int scmi_pinctrl_probe(struct scmi_device *sdev)  {
> > >  	int ret;
> > > @@ -521,7 +516,7 @@ static int scmi_pinctrl_probe(struct
> scmi_device *sdev)
> > >  	if (!sdev->handle)
> > >  		return -EINVAL;
> > >
> > > -	if (of_machine_compatible_match(scmi_pinctrl_blocklist))
> > > +	if (!dev->fwnode)
> >
> > I would prefer to see the blocklist to be explicit here rather than
> > implicitly hiding it away with this change set.
> 
> Using a flag to inhibit device_link_add as said early in the series this
> could be dropped and kept as is, I suppose.

Since the list will expand to include more i.MX9[X] chips, I was
thinking to drop the list to avoid update the list every time
we add a new SoC. We may need to use a machine
saying "fsl,imx9-sm" or else.

I will give a check on a flag. Thanks for suggestion.

Thanks,
Peng.

> 
> Thanks,
> Cristian

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

[Peng Fan]

> 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:
> > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > > 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
> > > devices will be created. But the fwnode->dev could only point to
> one device.
> > >
> > > 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).
> > > But actually the performance domain user device, such as GPU,
> should
> > > 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,
> 
> > > 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
> > > 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)
> > >  	device_unregister(&scmi_dev->dev);
> > >  }
> > >
> > > +static int
> > > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct
> device_node *np,
> > > +		       int protocol, const char *name) {
> > > +	/* cpufreq device does not need to be supplier from devlink
> perspective */
> > > +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
> "cpufreq"))
> > > +		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
> > 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-
> >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

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

[Cristian Marussi]

On Thu, Jan 02, 2025 at 07:38:06AM +0000, Peng Fan wrote:
> > 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:
> > > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > > > 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
> > > > devices will be created. But the fwnode->dev could only point to
> > one device.
> > > >
> > > > 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).
> > > > But actually the performance domain user device, such as GPU,
> > should
> > > > 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,
> > 
> > > > 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
> > > > 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)
> > > >  	device_unregister(&scmi_dev->dev);
> > > >  }
> > > >
> > > > +static int
> > > > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct
> > device_node *np,
> > > > +		       int protocol, const char *name) {
> > > > +	/* cpufreq device does not need to be supplier from devlink
> > perspective */
> > > > +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
> > "cpufreq"))
> > > > +		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
> > > 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-
> > >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.
> 

Ah right...my bad, the issue comes from the device_links created by
fw_devlink indirectly while walking the phandles backrefs...still...
...cant we keep the device_node reference while keep on dropping the
fw_node as you did:

 	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
		scmi_dev->dev.of_node = np;
 		return 0;
	}
 
 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
 	....

...so that the fw_devlink machinery is disabled but still we create a
device with an underlying related device_node that can be referred in a
phandle.

I wonder also if it was not even more clean to DO initialize fw_devlink
instead, BUT add some of the existent fw_devlink/devlink flags to inhibit
all the checks...but I am not familiar with fw_devlink so much and I
have not experimented in these regards...so I may have just said
something unfeasible.

Thanks,
Cristian

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

[Peng Fan]

On Thu, Jan 02, 2025 at 05:06:57PM +0000, Cristian Marussi wrote:
>On Thu, Jan 02, 2025 at 07:38:06AM +0000, Peng Fan wrote:
>> > 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:
>> > > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
>> > > > 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
>> > > > devices will be created. But the fwnode->dev could only point to
>> > one device.
>> > > >
>> > > > 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).
>> > > > But actually the performance domain user device, such as GPU,
>> > should
>> > > > 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,
>> > 
>> > > > 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
>> > > > 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)
>> > > >  	device_unregister(&scmi_dev->dev);
>> > > >  }
>> > > >
>> > > > +static int
>> > > > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct
>> > device_node *np,
>> > > > +		       int protocol, const char *name) {
>> > > > +	/* cpufreq device does not need to be supplier from devlink
>> > perspective */
>> > > > +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
>> > "cpufreq"))
>> > > > +		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
>> > > 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-
>> > >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.
>> 
>
>Ah right...my bad, the issue comes from the device_links created by
>fw_devlink indirectly while walking the phandles backrefs...still...
>...cant we keep the device_node reference while keep on dropping the
>fw_node as you did:
>
> 	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
>		scmi_dev->dev.of_node = np;
> 		return 0;
>	}
> 
> 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> 	....
>
>...so that the fw_devlink machinery is disabled but still we create a
>device with an underlying related device_node that can be referred in a
>phandle.

ok, I will add "scmi_dev->dev.of_node = np" for cpufreq device.

>
>I wonder also if it was not even more clean to DO initialize fw_devlink
>instead, BUT add some of the existent fw_devlink/devlink flags to inhibit
>all the checks...but I am not familiar with fw_devlink so much and I
>have not experimented in these regards...so I may have just said
>something unfeasible.

fw_devlink is based on device tree node, so there is no way, unless
add subnodes for a protocol node, but this is not welcomed.

Thanks,
Peng

>
>Thanks,
>Cristian
>

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Peng Fan]

On Tue, Dec 31, 2024 at 06:16:12PM +0000, Cristian Marussi wrote:
>On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
>> On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
>> > From: Peng Fan <peng.fan@nxp.com>
>> >
>> > pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
>> > If both drivers are built in, and the scmi device with name "pinctrl-imx"
>> > is created earlier, and the fwnode device points to the scmi device,
>> > non-i.MX platforms will never have the pinctrl supplier ready.
>> >
>> 
>> I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
>> i.MX platforms instead of this hack which IMO is little less hackier
>> (and little more cleaner as we don't create problem and then fix here)
>> than this change.
>
>...or indeed this is another possibility

I am doing a patch as below, how to do you think?

With below patch, we could resolve the devlink issue and also support mutitple
vendor drivers built in, with each vendor driver has a machine_allowlist.

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 1d2aedfcfdb4..c1c45b545480 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
        unsigned int id = 0;
        struct list_head *head, *phead = NULL;
        struct scmi_requested_dev *rdev;
+       const char * const *allowlist = id_table->machine_allowlist;
+       const char * const *blocklist = id_table->machine_blocklist;
+
+       if (blocklist && of_machine_compatible_match(blocklist)) {
+               pr_debug("block SCMI device (%s) for protocol %x\n",
+                        id_table->name, id_table->protocol_id);
+               return 0;
+       }
+
+       if (allowlist && !of_machine_compatible_match(allowlist)) {
+               pr_debug("block SCMI device (%s) for protocol %x\n",
+                        id_table->name, id_table->protocol_id);
+               return 0;
+       }

        pr_debug("Requesting SCMI device (%s) for protocol %x\n",
                 id_table->name, id_table->protocol_id);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816..e1b822d3522f 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -950,6 +950,9 @@ struct scmi_device {
 struct scmi_device_id {
        u8 protocol_id;
        const char *name;
+       /* Optional */
+       const char * const *machine_blocklist;
+       const char * const *machine_allowlist;
 };

 struct scmi_driver {

Thanks,
Peng
>
>Thanks,
>Cristian

RE: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Peng Fan]

Hi Cristian, Sudeep

> Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for pinctrl
> 
[...]

> >> fix here) than this change.
> >
> >...or indeed this is another possibility
> 
> I am doing a patch as below, how to do you think?

Do you have any comments on below ideas?

I am thinking to send out new patchset based on
below ideas in this week.

> 
> With below patch, we could resolve the devlink issue and also support
> mutitple vendor drivers built in, with each vendor driver has a
> machine_allowlist.
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c
> b/drivers/firmware/arm_scmi/bus.c index
> 1d2aedfcfdb4..c1c45b545480 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const
> struct scmi_device_id *id_table)
>         unsigned int id = 0;
>         struct list_head *head, *phead = NULL;
>         struct scmi_requested_dev *rdev;
> +       const char * const *allowlist = id_table->machine_allowlist;
> +       const char * const *blocklist = id_table->machine_blocklist;
> +
> +       if (blocklist && of_machine_compatible_match(blocklist)) {
> +               pr_debug("block SCMI device (%s) for protocol %x\n",
> +                        id_table->name, id_table->protocol_id);
> +               return 0;
> +       }
> +
> +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> +               pr_debug("block SCMI device (%s) for protocol %x\n",
> +                        id_table->name, id_table->protocol_id);
> +               return 0;
> +       }
> 
>         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
>                  id_table->name, id_table->protocol_id); diff --git
> a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index
> 688466a0e816..e1b822d3522f 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id {
>         u8 protocol_id;
>         const char *name;
> +       /* Optional */
> +       const char * const *machine_blocklist;
> +       const char * const *machine_allowlist;
>  };

Thanks,
Peng.

> 
>  struct scmi_driver {
> 
> Thanks,
> Peng
> >
> >Thanks,
> >Cristian

Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Cristian Marussi]

On Tue, Jan 14, 2025 at 08:31:03AM +0000, Peng Fan wrote:
> Hi Cristian, Sudeep
> 
> > Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> > fwnode for pinctrl
> > 
> [...]
> 
> > >> fix here) than this change.
> > >
> > >...or indeed this is another possibility
> > 
> > I am doing a patch as below, how to do you think?
> 
> Do you have any comments on below ideas?
> 
> I am thinking to send out new patchset based on
> below ideas in this week.
> 

Hi Peng,

sorry for the delay.

Why both blacklist and allowlist ?

Cristian

> > 
> > With below patch, we could resolve the devlink issue and also support
> > mutitple vendor drivers built in, with each vendor driver has a
> > machine_allowlist.
> > 
> > diff --git a/drivers/firmware/arm_scmi/bus.c
> > b/drivers/firmware/arm_scmi/bus.c index
> > 1d2aedfcfdb4..c1c45b545480 100644
> > --- a/drivers/firmware/arm_scmi/bus.c
> > +++ b/drivers/firmware/arm_scmi/bus.c
> > @@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const
> > struct scmi_device_id *id_table)
> >         unsigned int id = 0;
> >         struct list_head *head, *phead = NULL;
> >         struct scmi_requested_dev *rdev;
> > +       const char * const *allowlist = id_table->machine_allowlist;
> > +       const char * const *blocklist = id_table->machine_blocklist;
> > +
> > +       if (blocklist && of_machine_compatible_match(blocklist)) {
> > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > +                        id_table->name, id_table->protocol_id);
> > +               return 0;
> > +       }
> > +
> > +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > +                        id_table->name, id_table->protocol_id);
> > +               return 0;
> > +       }
> > 
> >         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
> >                  id_table->name, id_table->protocol_id); diff --git
> > a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index
> > 688466a0e816..e1b822d3522f 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id {
> >         u8 protocol_id;
> >         const char *name;
> > +       /* Optional */
> > +       const char * const *machine_blocklist;
> > +       const char * const *machine_allowlist;
> >  };
> 
> Thanks,
> Peng.
> 
> > 
> >  struct scmi_driver {
> > 
> > Thanks,
> > Peng
> > >
> > >Thanks,
> > >Cristian

RE: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

[Peng Fan]

> Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for pinctrl
> 
> On Tue, Jan 14, 2025 at 08:31:03AM +0000, Peng Fan wrote:
> > Hi Cristian, Sudeep
> >
> > > Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> > > fwnode for pinctrl
> > >
> > [...]
> >
> > > >> fix here) than this change.
> > > >
> > > >...or indeed this is another possibility
> > >
> > > I am doing a patch as below, how to do you think?
> >
> > Do you have any comments on below ideas?
> >
> > I am thinking to send out new patchset based on below ideas in this
> > week.
> >
> 
> Hi Peng,
> 
> sorry for the delay.
> 
> Why both blacklist and allowlist ?

Because there is blocklist in pinctrl-scmi.c and allowlist in
pinctrl-imx-scmi.c.

we need to make sure only one pinctrl device are
created when both drivers are built in. So to pinctrl-scmi.c,
use blacklist, to pinctrl-imx-scmi.c use allowlist.

To vendor protocols, just need allowlist.

Regards,
Peng.

> 
> Cristian
> 
> > >
> > > With below patch, we could resolve the devlink issue and also
> > > support mutitple vendor drivers built in, with each vendor driver
> > > has a machine_allowlist.
> > >
> > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > b/drivers/firmware/arm_scmi/bus.c index
> > > 1d2aedfcfdb4..c1c45b545480 100644
> > > --- a/drivers/firmware/arm_scmi/bus.c
> > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > @@ -55,6 +55,20 @@ static int
> scmi_protocol_device_request(const
> > > struct scmi_device_id *id_table)
> > >         unsigned int id = 0;
> > >         struct list_head *head, *phead = NULL;
> > >         struct scmi_requested_dev *rdev;
> > > +       const char * const *allowlist = id_table->machine_allowlist;
> > > +       const char * const *blocklist = id_table->machine_blocklist;
> > > +
> > > +       if (blocklist && of_machine_compatible_match(blocklist)) {
> > > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > > +                        id_table->name, id_table->protocol_id);
> > > +               return 0;
> > > +       }
> > > +
> > > +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> > > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > > +                        id_table->name, id_table->protocol_id);
> > > +               return 0;
> > > +       }
> > >
> > >         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
> > >                  id_table->name, id_table->protocol_id); diff --git
> > > a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > > index 688466a0e816..e1b822d3522f 100644
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id
> {
> > >         u8 protocol_id;
> > >         const char *name;
> > > +       /* Optional */
> > > +       const char * const *machine_blocklist;
> > > +       const char * const *machine_allowlist;
> > >  };
> >
> > Thanks,
> > Peng.
> >
> > >
> > >  struct scmi_driver {
> > >
> > > Thanks,
> > > Peng
> > > >
> > > >Thanks,
> > > >Cristian

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

[Sudeep Holla]

On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> 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 devices
> will be created. But the fwnode->dev could only point to one device.
> 
> 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).
> But actually the performance domain user device, such as GPU, should 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.
>

Not 100% sure if above is correct. See:

Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider")

Am I missing something ?

-- 
Regards,
Sudeep

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

[Peng Fan]

On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:
>On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
>> 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 devices
>> will be created. But the fwnode->dev could only point to one device.
>> 
>> 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).
>> But actually the performance domain user device, such as GPU, should 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.
>>
>
>Not 100% sure if above is correct. See:
>
>Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider")
>
>Am I missing something ?

Could we update juno-scmi.dtsi to use ?

 &A53_0 {
-       clocks = <&scmi_dvfs 1>;
+       power-domains = <&scmi_perf x>;
+       power-domain-names = "perf";
 };

Even for scmi-cpufreq.c that needs fw_devlink because of the clocks entry in
juno-scmi.dtsi, there is no issue here.
Because the dummy clock provider will always be ready before opp with
your upper fix. So we could safetly ignore fw_devlink per my understanding.

Regards,
Peng

>
>-- 
>Regards,
>Sudeep

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

[Sudeep Holla]

On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote:
> On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:
> >On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> >> 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 devices
> >> will be created. But the fwnode->dev could only point to one device.
> >> 
> >> 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).
> >> But actually the performance domain user device, such as GPU, should 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.
> >>
> >
> >Not 100% sure if above is correct. See:
> >
> >Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider")
> >
> >Am I missing something ?
> 
> Could we update juno-scmi.dtsi to use ?
> 
>  &A53_0 {
> -       clocks = <&scmi_dvfs 1>;
> +       power-domains = <&scmi_perf x>;
> +       power-domain-names = "perf";
>  };
>

We can, but I retained it so that the clocks property support can be still
validated until it is removed. I think there are few downstream users of
it. It is not just the DTS files you need to look at when dealing with
such things. It is the bindings that matter. Until bindings are not
deprecated and made obsolete, support must exist even if you modify the
only user in the upstream DT.

--
Regards,
Sudeep

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

[Saravana Kannan]

On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote:
> > On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:
> > >On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > >> 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 devices
> > >> will be created. But the fwnode->dev could only point to one device.
> > >>
> > >> 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).
> > >> But actually the performance domain user device, such as GPU, should 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.
> > >>
> > >
> > >Not 100% sure if above is correct. See:
> > >
> > >Commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure with a dummy clock provider")
> > >
> > >Am I missing something ?
> >
> > Could we update juno-scmi.dtsi to use ?
> >
> >  &A53_0 {
> > -       clocks = <&scmi_dvfs 1>;
> > +       power-domains = <&scmi_perf x>;
> > +       power-domain-names = "perf";
> >  };
> >
>
> We can, but I retained it so that the clocks property support can be still
> validated until it is removed. I think there are few downstream users of
> it. It is not just the DTS files you need to look at when dealing with
> such things. It is the bindings that matter. Until bindings are not
> deprecated and made obsolete, support must exist even if you modify the
> only user in the upstream DT.

Sorry, been caught up on other stuff and trying to get to some long
pending emails.

Sudeep,

Do you know why commit dd461cd9183f ("opp: Allow
dev_pm_opp_get_opp_table() to return -EPROBE_DEFER") was needed? I'd
think fw_devlink would have caught those issues.
I'd recommended reverting that/fixing it differently instead of
creating commit 8410e7f3b31e ("cpufreq: scmi: Fix OPP addition failure
with a dummy clock provider")

Peng, Sudeep,

If you make fwnode ignore the cpufreq device, then it'll also not
enforce ordering between cpufreq and it's suppliers like clocks and
power domains. Not sure if that's a real possibility for scmi (I'm
guessing no?). Make sure that's not going to be a problem.

Cristian,

Thanks for taking the time to give a detailed description here[1]. I
seem to have missed that email.
[1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/

Peng/Cristian,

Yes, we can have the driver core ignore this device for fw_devlink by
looking at some flag on the device (and not on the fwnode). But that
is just kicking the can down the road. We could easily end up with two
SCMI devices needing a separate set of consumers. For example,
something like below can have two SCMI devices A and B created where
only A needs the mboxes and only B needs shmem and power-domains. This
will get messy even for drivers if the driver for A optionally needs
power-domains on some machines, but not this one.

        firmware {
                scmi {
                        compatible = "arm,scmi";
                        scmi_dvfs: protocol@13 {
                                reg = <0x13>;
                                #clock-cells = <1>;
                                mbox-names = "tx", "rx";
                                mboxes = <&mailbox 1 0 &mailbox 1 1>;
                                shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
                                power-domains = <&blah>;
                        };

Wait a sec, looking around at the SCMI code, I just realized that you
don't even really care about the node name to get the protocol number
and you just look at "reg" for protocol number. Why not just have
peng's device have two protocol@13 DT nodes?

cpufreq@13 {
    reg = <0x13>;
}
whateverelse@13 {
    reg = <0x13>;
}

You can also probably throw in a compatible field if you need to help
the drivers pick the right node (where they currently pick the same
node). Or you can do whatever else would help make sure the cpufreq
device is attached to the cpufreq node and the whateverelse device is
attached to the whateverelse node.

Looks like that'll first help clean up the "two devices for one node"
issue. And then the rest should just work? Cristian, am I missing
anything?

Thanks,
Saravana

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

[Cristian Marussi]

On Thu, Feb 13, 2025 at 12:03:15AM -0800, Saravana Kannan wrote:
> On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >

Hi Saravana,

> > On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote:
> > > On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:
> > > >On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > > >> From: Peng Fan <peng.fan@nxp.com>
> > > >>

[snip]

> 
> Cristian,
> 
> Thanks for taking the time to give a detailed description here[1]. I
> seem to have missed that email.
> [1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/
> 
> Peng/Cristian,
> 
> Yes, we can have the driver core ignore this device for fw_devlink by
> looking at some flag on the device (and not on the fwnode). But that
> is just kicking the can down the road. We could easily end up with two

Oh yes this is definitely some sort of hack/workaround that just kicks
the can down the road, I agree...just I cannot see any better solution
from what Peng propose (beside maybe we can discuss his implementation
details as we are doing...)

> SCMI devices needing a separate set of consumers. For example,
> something like below can have two SCMI devices A and B created where
> only A needs the mboxes and only B needs shmem and power-domains. This

..not really...it is even worse :P ... the mbox/shmem props down below are
really definition of a mailbox transport SCMI channel: some transports
allow multiple channels to be defined and in such case you can dedicate
one channel to a specific protocol...

...so, in this case, you will see there will be something similar defined
in terms of mboxes/shmem at the top SCMI DT node to represent an SCMI channel
used for all the protocols WHILE this additional definition inside the
protocol node defines a dedicated channel...IOW these props mboxes/shmem
are really parsed/consumed upfront by the core SCMI stack at probe to
configure and allocare basic comms channel BEFORE any SCMI device is created
...then the protocol DT node is no more used by the core and is instead 'lent'
to create SCMI devices for the drivers needing them...(possibly lending it to
multiple users...that is the issue) 

> will get messy even for drivers if the driver for A optionally needs
> power-domains on some machines, but not this one.
> 
>         firmware {
>                 scmi {
>                         compatible = "arm,scmi";
>                         scmi_dvfs: protocol@13 {
>                                 reg = <0x13>;
>                                 #clock-cells = <1>;
>                                 mbox-names = "tx", "rx";
>                                 mboxes = <&mailbox 1 0 &mailbox 1 1>;
>                                 shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
>                                 power-domains = <&blah>;
>                         };
> 
> Wait a sec, looking around at the SCMI code, I just realized that you
> don't even really care about the node name to get the protocol number
> and you just look at "reg" for protocol number. Why not just have
> peng's device have two protocol@13 DT nodes?
> 
> cpufreq@13 {
>     reg = <0x13>;
> }
> whateverelse@13 {
>     reg = <0x13>;
> }
> 
> You can also probably throw in a compatible field if you need to help
> the drivers pick the right node (where they currently pick the same
> node). Or you can do whatever else would help make sure the cpufreq
> device is attached to the cpufreq node and the whateverelse device is
> attached to the whateverelse node.

..well...my longer-than-ever explanation of the innner-workings was
meant to explain where the problem comes from, and how would be difficult
to address it WITHOUT changing the DT bindings, BECAUE I pretty much doubt
that throwing into the mix also multiple nodes definitions and compatibles
could fly with the DT maintainers, AND certainly it will go against the basic
rules for 'reg-indexed' properties ...you cannot have 2 prop indexed with the
same reg-value AFAIK...and the reg-value, here, is indeed the spec protocol
number so you cannot change that either within the set of nodes sharing
the same prop....

...moreover the above additional construct of having possibly per-protocol
channels would create even more a mess in this scenario of explicitly
declared duplicated protocol-nodes:
 
- should we duplicate the optional mbox/shmem too ? not possible...DT sanity
  would fail immediately also in this (I suppose due to duplicated entries)

...BUT

- at the same time we should assume that ALL the duplicated protocols inherits
the optional per-protocol dedicated channel that is defined in one of
them...seems very dirty to me...

...moreover...explicitly allowing for such duplicate DT protocol definitions
would open the door to create even more SCMI drivers like pinctrl-imx that
uses the same PINCTRL protocol as the generic-pinctrl BUT really implements
the SAME functionalities as the generic one (just slightly differently
and using a complete distinct set of NXP pinctrl bindings for historical
reasons AFAIU)....BUT pinctrl-imx is an *unfortunate* exception that we had
to support for the historical reason I mentioned BUT should NOT be the rule
NOR the advised way...

....while other drivers exists that share the usage of the same protocol
(HWMON/IIO GENPD/CPUFREQ), they use the same protocol to achieve different
things in different subsytems...and they are anyway impacted (even to a less
degree) by this fw_devlink issue AFAIU so the problem indeed exist also
out of pinctrl-imx

> 
> Looks like that'll first help clean up the "two devices for one node"
> issue. And then the rest should just work? Cristian, am I missing
> anything?

Yes that is the main issue...but still dont see how to solve it in a
clean way...

Thanks,
Cristian

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

[Peng Fan]

On Thu, Feb 13, 2025 at 08:23:53PM +0000, Cristian Marussi wrote:
>On Thu, Feb 13, 2025 at 12:03:15AM -0800, Saravana Kannan wrote:
>> On Wed, Feb 12, 2025 at 2:48 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>> >
>
>Hi Saravana,
>
>> > On Wed, Feb 12, 2025 at 03:01:20PM +0800, Peng Fan wrote:
>> > > On Tue, Feb 11, 2025 at 05:13:21PM +0000, Sudeep Holla wrote:
>> > > >On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
>> > > >> From: Peng Fan <peng.fan@nxp.com>
>> > > >>
>
>[snip]
>
>> 
>> Cristian,
>> 
>> Thanks for taking the time to give a detailed description here[1]. I
>> seem to have missed that email.
>> [1] - https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/
>> 
>> Peng/Cristian,
>> 
>> Yes, we can have the driver core ignore this device for fw_devlink by
>> looking at some flag on the device (and not on the fwnode). But that
>> is just kicking the can down the road. We could easily end up with two
>
>Oh yes this is definitely some sort of hack/workaround that just kicks
>the can down the road, I agree...just I cannot see any better solution
>from what Peng propose (beside maybe we can discuss his implementation
>details as we are doing...)
>
>> SCMI devices needing a separate set of consumers. For example,
>> something like below can have two SCMI devices A and B created where
>> only A needs the mboxes and only B needs shmem and power-domains. This
>
>..not really...it is even worse :P ... the mbox/shmem props down below are
>really definition of a mailbox transport SCMI channel: some transports
>allow multiple channels to be defined and in such case you can dedicate
>one channel to a specific protocol...
>
>...so, in this case, you will see there will be something similar defined
>in terms of mboxes/shmem at the top SCMI DT node to represent an SCMI channel
>used for all the protocols WHILE this additional definition inside the
>protocol node defines a dedicated channel...IOW these props mboxes/shmem
>are really parsed/consumed upfront by the core SCMI stack at probe to
>configure and allocare basic comms channel BEFORE any SCMI device is created
>...then the protocol DT node is no more used by the core and is instead 'lent'
>to create SCMI devices for the drivers needing them...(possibly lending it to
>multiple users...that is the issue) 
>
>> will get messy even for drivers if the driver for A optionally needs
>> power-domains on some machines, but not this one.
>> 
>>         firmware {
>>                 scmi {
>>                         compatible = "arm,scmi";
>>                         scmi_dvfs: protocol@13 {
>>                                 reg = <0x13>;
>>                                 #clock-cells = <1>;
>>                                 mbox-names = "tx", "rx";
>>                                 mboxes = <&mailbox 1 0 &mailbox 1 1>;
>>                                 shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
>>                                 power-domains = <&blah>;
>>                         };
>> 
>> Wait a sec, looking around at the SCMI code, I just realized that you
>> don't even really care about the node name to get the protocol number
>> and you just look at "reg" for protocol number. Why not just have
>> peng's device have two protocol@13 DT nodes?
>> 
>> cpufreq@13 {
>>     reg = <0x13>;
>> }
>> whateverelse@13 {
>>     reg = <0x13>;
>> }
>> 
>> You can also probably throw in a compatible field if you need to help
>> the drivers pick the right node (where they currently pick the same
>> node). Or you can do whatever else would help make sure the cpufreq
>> device is attached to the cpufreq node and the whateverelse device is
>> attached to the whateverelse node.
>
>..well...my longer-than-ever explanation of the innner-workings was
>meant to explain where the problem comes from, and how would be difficult
>to address it WITHOUT changing the DT bindings, BECAUE I pretty much doubt
>that throwing into the mix also multiple nodes definitions and compatibles
>could fly with the DT maintainers, AND certainly it will go against the basic
>rules for 'reg-indexed' properties ...you cannot have 2 prop indexed with the
>same reg-value AFAIK...and the reg-value, here, is indeed the spec protocol
>number so you cannot change that either within the set of nodes sharing
>the same prop....
>
>...moreover the above additional construct of having possibly per-protocol
>channels would create even more a mess in this scenario of explicitly
>declared duplicated protocol-nodes:
> 
>- should we duplicate the optional mbox/shmem too ? not possible...DT sanity
>  would fail immediately also in this (I suppose due to duplicated entries)
>
>...BUT
>
>- at the same time we should assume that ALL the duplicated protocols inherits
>the optional per-protocol dedicated channel that is defined in one of
>them...seems very dirty to me...
>
>...moreover...explicitly allowing for such duplicate DT protocol definitions
>would open the door to create even more SCMI drivers like pinctrl-imx that
>uses the same PINCTRL protocol as the generic-pinctrl BUT really implements
>the SAME functionalities as the generic one (just slightly differently
>and using a complete distinct set of NXP pinctrl bindings for historical
>reasons AFAIU)....BUT pinctrl-imx is an *unfortunate* exception that we had
>to support for the historical reason I mentioned BUT should NOT be the rule
>NOR the advised way...
>
>....while other drivers exists that share the usage of the same protocol
>(HWMON/IIO GENPD/CPUFREQ), they use the same protocol to achieve different
>things in different subsytems...and they are anyway impacted (even to a less
>degree) by this fw_devlink issue AFAIU so the problem indeed exist also
>out of pinctrl-imx
>
>> 
>> Looks like that'll first help clean up the "two devices for one node"
>> issue. And then the rest should just work? Cristian, am I missing
>> anything?
>
>Yes that is the main issue...but still dont see how to solve it in a
>clean way...

A potential solution is not using reg in the protocol nodes. Define nodes
as below:
devperf {
	compatible ="arm,scmi-devperf";
}

cpuperf {
	compatible ="arm,scmi-cpuperf";
}

pinctrl {
	compatible ="arm,scmi-pinctrl";
}

The reg is coded in driver.

But the upper requires restruction of scmi framework.

Put the above away, could we first purse a simple way first to address
the current bug in kernel? Just as I prototyped here:
https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2

Thanks,
Peng.

>
>Thanks,
>Cristian

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

[Sudeep Holla]

On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote:
> A potential solution is not using reg in the protocol nodes. Define nodes
> as below:
> devperf {
> 	compatible ="arm,scmi-devperf";
> }
> 
> cpuperf {
> 	compatible ="arm,scmi-cpuperf";
> }
> 
> pinctrl {
> 	compatible ="arm,scmi-pinctrl";
> }
> 
> The reg is coded in driver.
> 
> But the upper requires restruction of scmi framework.
> 
> Put the above away, could we first purse a simple way first to address
> the current bug in kernel? Just as I prototyped here:
> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2
> 

Good luck getting these bindings merged. I don't like it as it is pushing
software policy or issues into to the devicetree. What we have as SCMI
binding is more than required for a firmware interface IMO. So, you are
on your own to get these bindings approved as I am not on board with
these but if you convince DT maintainers, I will have a look at it then
to see if we can make that work really.

-- 
Regards,
Sudeep

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

[Peng Fan]

On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote:
>On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote:
>> A potential solution is not using reg in the protocol nodes. Define nodes
>> as below:
>> devperf {
>> 	compatible ="arm,scmi-devperf";
>> }
>> 
>> cpuperf {
>> 	compatible ="arm,scmi-cpuperf";
>> }
>> 
>> pinctrl {
>> 	compatible ="arm,scmi-pinctrl";
>> }
>> 
>> The reg is coded in driver.
>> 
>> But the upper requires restruction of scmi framework.
>> 
>> Put the above away, could we first purse a simple way first to address
>> the current bug in kernel? Just as I prototyped here:
>> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2
>> 
>
>Good luck getting these bindings merged. I don't like it as it is pushing
>software policy or issues into to the devicetree. What we have as SCMI
>binding is more than required for a firmware interface IMO. So, you are

Would you mind share more info on other cases that SCMI not as firmware
interface?

>on your own to get these bindings approved as I am not on board with
>these but if you convince DT maintainers, I will have a look at it then
>to see if we can make that work really.

The issues are common to SCMI, not i.MX specific.
I just propose potential solutions. You are the SCMI maintainer, there
is no chance to get bindings approved without you.

No more ideas from me. Leave this to you in case you have better solution.

Regards,
Peng

>
>-- 
>Regards,
>Sudeep
>

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

[Sudeep Holla]

On Tue, Feb 18, 2025 at 09:36:19PM +0800, Peng Fan wrote:
> On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote:
> >On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote:
> >> A potential solution is not using reg in the protocol nodes. Define nodes
> >> as below:
> >> devperf {
> >> 	compatible ="arm,scmi-devperf";
> >> }
> >>
> >> cpuperf {
> >> 	compatible ="arm,scmi-cpuperf";
> >> }
> >>
> >> pinctrl {
> >> 	compatible ="arm,scmi-pinctrl";
> >> }
> >>
> >> The reg is coded in driver.
> >>
> >> But the upper requires restruction of scmi framework.
> >>
> >> Put the above away, could we first purse a simple way first to address
> >> the current bug in kernel? Just as I prototyped here:
> >> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2
> >>
> >
> >Good luck getting these bindings merged. I don't like it as it is pushing
> >software policy or issues into to the devicetree. What we have as SCMI
> >binding is more than required for a firmware interface IMO. So, you are
>
> Would you mind share more info on other cases that SCMI not as firmware
> interface?
>
> >on your own to get these bindings approved as I am not on board with
> >these but if you convince DT maintainers, I will have a look at it then
> >to see if we can make that work really.
>
> The issues are common to SCMI, not i.MX specific.
> I just propose potential solutions. You are the SCMI maintainer, there
> is no chance to get bindings approved without you.
>

I am not blocking you. What I mentioned is I don't agree that DT can be used
to resolve this issue, but I don't have time or alternate solution ATM. So
if you propose DT based solution and the maintainers agree for the proposed
bindings I will take a look and help you to make that work. But I will raise
any objections I may have if the proposal has issues mainly around the
compatibility and ease of maintenance.

> No more ideas from me. Leave this to you in case you have better solution.
>

Unfortunately no, I don't have one. I haven't had time to sit and explore
the issue and think of any solution yet.

--
Regards,
Sudeep

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

[Peng Fan]

On Wed, Feb 19, 2025 at 10:17:46AM +0000, Sudeep Holla wrote:
>On Tue, Feb 18, 2025 at 09:36:19PM +0800, Peng Fan wrote:
>> On Tue, Feb 18, 2025 at 10:24:52AM +0000, Sudeep Holla wrote:
>> >On Tue, Feb 18, 2025 at 09:09:49AM +0800, Peng Fan wrote:
>> >> A potential solution is not using reg in the protocol nodes. Define nodes
>> >> as below:
>> >> devperf {
>> >> 	compatible ="arm,scmi-devperf";
>> >> }
>> >>
>> >> cpuperf {
>> >> 	compatible ="arm,scmi-cpuperf";
>> >> }
>> >>
>> >> pinctrl {
>> >> 	compatible ="arm,scmi-pinctrl";
>> >> }
>> >>
>> >> The reg is coded in driver.
>> >>
>> >> But the upper requires restruction of scmi framework.
>> >>
>> >> Put the above away, could we first purse a simple way first to address
>> >> the current bug in kernel? Just as I prototyped here:
>> >> https://github.com/MrVan/linux/tree/b4/scmi-fwdevlink-v2
>> >>
>> >
>> >Good luck getting these bindings merged. I don't like it as it is pushing
>> >software policy or issues into to the devicetree. What we have as SCMI
>> >binding is more than required for a firmware interface IMO. So, you are
>>
>> Would you mind share more info on other cases that SCMI not as firmware
>> interface?
>>
>> >on your own to get these bindings approved as I am not on board with
>> >these but if you convince DT maintainers, I will have a look at it then
>> >to see if we can make that work really.
>>
>> The issues are common to SCMI, not i.MX specific.
>> I just propose potential solutions. You are the SCMI maintainer, there
>> is no chance to get bindings approved without you.
>>
>
>I am not blocking you. What I mentioned is I don't agree that DT can be used
>to resolve this issue, but I don't have time or alternate solution ATM. So
>if you propose DT based solution and the maintainers agree for the proposed
>bindings I will take a look and help you to make that work. But I will raise
>any objections I may have if the proposal has issues mainly around the
>compatibility and ease of maintenance.

Sorry, if I misunderstood.

I will give a look on this and propose a RFC.

DT maintainers may ask for a patchset including binding change and
driver changes to get a whole view on the compatible stuff.

BTW, Cristian, Saravana if you have any objections/ideas or would take on this
effort, please let me know.

Thanks,
Peng

>
>> No more ideas from me. Leave this to you in case you have better solution.
>>
>
>Unfortunately no, I don't have one. I haven't had time to sit and explore
>the issue and think of any solution yet.
>
>--
>Regards,
>Sudeep

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

[Sudeep Holla]

On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
>
> Sorry, if I misunderstood.
>
> I will give a look on this and propose a RFC.
>
> DT maintainers may ask for a patchset including binding change and
> driver changes to get a whole view on the compatible stuff.
>
> BTW, Cristian, Saravana if you have any objections/ideas or would take on this
> effort, please let me know.
>

Can you point me to the DTS with which you are seeing this issue ?
I am trying to reproduce the issue but so far not successful. I did
move to power-domains for CPUFreq on Juno. IIUC all we need is both cpufreq
and performance genpd drivers in the kernel and then GPU using perf genpd
fails with probe deferral right ? I need pointers to reproduce the issue
so that I can check if what I have cooked up as a solution really works.

--
Regards,
Sudeep

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

[Peng Fan]

> Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for scmi cpufreq
> 
> On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
> >
> > Sorry, if I misunderstood.
> >
> > I will give a look on this and propose a RFC.
> >
> > DT maintainers may ask for a patchset including binding change and
> > driver changes to get a whole view on the compatible stuff.
> >
> > BTW, Cristian, Saravana if you have any objections/ideas or would
> take
> > on this effort, please let me know.
> >
> 
> Can you point me to the DTS with which you are seeing this issue ?
> I am trying to reproduce the issue but so far not successful. I did move
> to power-domains for CPUFreq on Juno. IIUC all we need is both
> cpufreq and performance genpd drivers in the kernel and then GPU
> using perf genpd fails with probe deferral right ? I need pointers to
> reproduce the issue so that I can check if what I have cooked up as a
> solution really works.

This is in downstream tree:
https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L2971
https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L3043
https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L80

we are using "power-domains" property for cpu perf and gpu/vpu perf.

If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer probe.

Regards,
Peng.

> 
> --
> Regards,
> Sudeep

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

[Sudeep Holla]

On Mon, Mar 10, 2025 at 10:45:44AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> > fwnode for scmi cpufreq
> > 
> > On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
> > >
> > > Sorry, if I misunderstood.
> > >
> > > I will give a look on this and propose a RFC.
> > >
> > > DT maintainers may ask for a patchset including binding change and
> > > driver changes to get a whole view on the compatible stuff.
> > >
> > > BTW, Cristian, Saravana if you have any objections/ideas or would
> > take
> > > on this effort, please let me know.
> > >
> > 
> > Can you point me to the DTS with which you are seeing this issue ?
> > I am trying to reproduce the issue but so far not successful. I did move
> > to power-domains for CPUFreq on Juno. IIUC all we need is both
> > cpufreq and performance genpd drivers in the kernel and then GPU
> > using perf genpd fails with probe deferral right ? I need pointers to
> > reproduce the issue so that I can check if what I have cooked up as a
> > solution really works.
>
> This is in downstream tree:
> https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L2971
> https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L3043
> https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L80
>
> we are using "power-domains" property for cpu perf and gpu/vpu perf.
>
> If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer probe.
>

OK, does the probe of these drivers get called or they don't as the driver
core doesn't allow that ? I just have a dummy driver for mali on Juno
which just does dev_pm_domain_attach_list() in the probe and it seem to
succeed even when cpufreq.off=1 is passed. I see scmi-cpufreq failing
with -ENODEV as expected.

I need to follow the code and check if I can somehow reproduce. Also are
you sure this is not with anything in the downstream code ? Also have you
tried this with v6.14-rc* ? Are you sure all the fw_devlink code is backported
in the tree you pointed me which is v6.6-stable ?

--
Regards,
Sudeep

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

[Sudeep Holla]

On Mon, Mar 10, 2025 at 11:59:33AM +0000, Sudeep Holla wrote:
> On Mon, Mar 10, 2025 at 10:45:44AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> > > fwnode for scmi cpufreq
> > > 
> > > On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
> > > >
> > > > Sorry, if I misunderstood.
> > > >
> > > > I will give a look on this and propose a RFC.
> > > >
> > > > DT maintainers may ask for a patchset including binding change and
> > > > driver changes to get a whole view on the compatible stuff.
> > > >
> > > > BTW, Cristian, Saravana if you have any objections/ideas or would
> > > take
> > > > on this effort, please let me know.
> > > >
> > > 
> > > Can you point me to the DTS with which you are seeing this issue ?
> > > I am trying to reproduce the issue but so far not successful. I did move
> > > to power-domains for CPUFreq on Juno. IIUC all we need is both
> > > cpufreq and performance genpd drivers in the kernel and then GPU
> > > using perf genpd fails with probe deferral right ? I need pointers to
> > > reproduce the issue so that I can check if what I have cooked up as a
> > > solution really works.
> >
> > This is in downstream tree:
> > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L2971
> > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L3043
> > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi#L80
> >
> > we are using "power-domains" property for cpu perf and gpu/vpu perf.
> >
> > If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer probe.
> >
> 
> OK, does the probe of these drivers get called or they don't as the driver
> core doesn't allow that ? I just have a dummy driver for mali on Juno
> which just does dev_pm_domain_attach_list() in the probe and it seem to
> succeed even when cpufreq.off=1 is passed. I see scmi-cpufreq failing
> with -ENODEV as expected.
> 
> I need to follow the code and check if I can somehow reproduce. Also are
> you sure this is not with anything in the downstream code ? Also have you
> tried this with v6.14-rc* ? Are you sure all the fw_devlink code is backported
> in the tree you pointed me which is v6.6-stable ?
> 

I even tried the above branch, but no luck. The above is neither latest
stable version nor pure stable. It has few extra patches backported
though IIUC. Anyways any pointers to enable me to reproduce the issue
would be much appreciated.

-- 
Regards,
Sudeep

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

[Peng Fan]

> Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for scmi cpufreq
> 
> On Mon, Mar 10, 2025 at 11:59:33AM +0000, Sudeep Holla wrote:
> > On Mon, Mar 10, 2025 at 10:45:44AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> > > > fwnode for scmi cpufreq
> > > >
> > > > On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
> > > > >
> > > > > Sorry, if I misunderstood.
> > > > >
> > > > > I will give a look on this and propose a RFC.
> > > > >
> > > > > DT maintainers may ask for a patchset including binding change
> > > > > and driver changes to get a whole view on the compatible stuff.
> > > > >
> > > > > BTW, Cristian, Saravana if you have any objections/ideas or
> > > > > would
> > > > take
> > > > > on this effort, please let me know.
> > > > >
> > > >
> > > > Can you point me to the DTS with which you are seeing this issue ?
> > > > I am trying to reproduce the issue but so far not successful. I
> > > > did move to power-domains for CPUFreq on Juno. IIUC all we
> need is
> > > > both cpufreq and performance genpd drivers in the kernel and
> then
> > > > GPU using perf genpd fails with probe deferral right ? I need
> > > > pointers to reproduce the issue so that I can check if what I have
> > > > cooked up as a solution really works.
> > >
> > > This is in downstream tree:
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> gi
> > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> 6.6.y%2Farch%2Farm64%2Fbo
> > >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L2971&data=05%7C02%7Cpe
> ng.fan%40
> > >
> nxp.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2
> b4c6fa92cd99
> > >
> c5c301635%7C0%7C0%7C638772109152491267%7CUnknown%7CT
> WFpbGZsb3d8eyJFb
> > >
> XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpb
> > >
> CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=nHFiE5qD7NpmdGmj
> SUL0mIdOq8P4W
> > > ErqVq8xE%2Fb3WM0%3D&reserved=0
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> gi
> > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> 6.6.y%2Farch%2Farm64%2Fbo
> > >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L3043&data=05%7C02%7Cpe
> ng.fan%40
> > >
> nxp.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2
> b4c6fa92cd99
> > >
> c5c301635%7C0%7C0%7C638772109152521215%7CUnknown%7CT
> WFpbGZsb3d8eyJFb
> > >
> XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpb
> > >
> CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=M4LJumL6y9bQ%2FL
> ocPvlNiMnCFtO
> > > vODYNrC0DGbbydxY%3D&reserved=0
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> gi
> > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> 6.6.y%2Farch%2Farm64%2Fbo
> > >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L80&data=05%7C02%7Cpeng
> .fan%40nx
> > >
> p.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2b4
> c6fa92cd99c5
> > >
> c301635%7C0%7C0%7C638772109152541725%7CUnknown%7CTWF
> pbGZsb3d8eyJFbXB
> > >
> 0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi
> TWFpbCI
> > >
> sIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=VpxcGrB6Dnr9yCO%2F
> wl8sEw1LYSlX5
> > > nPHqnlJ5mKm%2B7A%3D&reserved=0
> > >
> > > we are using "power-domains" property for cpu perf and gpu/vpu
> perf.
> > >
> > > If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer
> probe.
> > >
> >
> > OK, does the probe of these drivers get called or they don't as the
> > driver core doesn't allow that ? I just have a dummy driver for mali
> > on Juno which just does dev_pm_domain_attach_list() in the probe
> and
> > it seem to succeed even when cpufreq.off=1 is passed. I see
> > scmi-cpufreq failing with -ENODEV as expected.
> >
> > I need to follow the code and check if I can somehow reproduce. Also
> > are you sure this is not with anything in the downstream code ? Also
> > have you tried this with v6.14-rc* ? Are you sure all the fw_devlink
> > code is backported in the tree you pointed me which is v6.6-stable ?
> >
> 
> I even tried the above branch, but no luck. The above is neither latest
> stable version nor pure stable. It has few extra patches backported
> though IIUC. Anyways any pointers to enable me to reproduce the
> issue would be much appreciated.

I will setup test based latest linux-next and share results. Please wait.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

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

[Peng Fan]

> Subject: 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 Mon, Mar 10, 2025 at 11:59:33AM +0000, Sudeep Holla wrote:
> > > On Mon, Mar 10, 2025 at 10:45:44AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass
> setting
> > > > > fwnode for scmi cpufreq
> > > > >
> > > > > On Thu, Feb 20, 2025 at 08:59:18AM +0800, Peng Fan wrote:
> > > > > >
> > > > > > Sorry, if I misunderstood.
> > > > > >
> > > > > > I will give a look on this and propose a RFC.
> > > > > >
> > > > > > DT maintainers may ask for a patchset including binding
> change
> > > > > > and driver changes to get a whole view on the compatible
> stuff.
> > > > > >
> > > > > > BTW, Cristian, Saravana if you have any objections/ideas or
> > > > > > would
> > > > > take
> > > > > > on this effort, please let me know.
> > > > > >
> > > > >
> > > > > Can you point me to the DTS with which you are seeing this
> issue ?
> > > > > I am trying to reproduce the issue but so far not successful. I
> > > > > did move to power-domains for CPUFreq on Juno. IIUC all we
> > need is
> > > > > both cpufreq and performance genpd drivers in the kernel and
> > then
> > > > > GPU using perf genpd fails with probe deferral right ? I need
> > > > > pointers to reproduce the issue so that I can check if what I
> > > > > have cooked up as a solution really works.
> > > >
> > > > This is in downstream tree:
> > > >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gi
> > > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> > 6.6.y%2Farch%2Farm64%2Fbo
> > > >
> >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L2971&data=05%7C02%7Cpe
> > ng.fan%40
> > > >
> >
> nxp.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2
> > b4c6fa92cd99
> > > >
> > c5c301635%7C0%7C0%7C638772109152491267%7CUnknown%7CT
> > WFpbGZsb3d8eyJFb
> > > >
> >
> XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> > joiTWFpb
> > > >
> >
> CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=nHFiE5qD7NpmdGmj
> > SUL0mIdOq8P4W
> > > > ErqVq8xE%2Fb3WM0%3D&reserved=0
> > > >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gi
> > > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> > 6.6.y%2Farch%2Farm64%2Fbo
> > > >
> >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L3043&data=05%7C02%7Cpe
> > ng.fan%40
> > > >
> >
> nxp.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2
> > b4c6fa92cd99
> > > >
> > c5c301635%7C0%7C0%7C638772109152521215%7CUnknown%7CT
> > WFpbGZsb3d8eyJFb
> > > >
> >
> XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> > joiTWFpb
> > > >
> >
> CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=M4LJumL6y9bQ%2FL
> > ocPvlNiMnCFtO
> > > > vODYNrC0DGbbydxY%3D&reserved=0
> > > >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gi
> > > > thub.com%2Fnxp-imx%2Flinux-imx%2Fblob%2Flf-
> > 6.6.y%2Farch%2Farm64%2Fbo
> > > >
> >
> ot%2Fdts%2Ffreescale%2Fimx95.dtsi%23L80&data=05%7C02%7Cpeng
> > .fan%40nx
> > > >
> >
> p.com%7C72778d531e944c7214ca08dd5fd95012%7C686ea1d3bc2b4
> > c6fa92cd99c5
> > > >
> >
> c301635%7C0%7C0%7C638772109152541725%7CUnknown%7CTWF
> > pbGZsb3d8eyJFbXB
> > > >
> >
> 0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi
> > TWFpbCI
> > > >
> >
> sIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=VpxcGrB6Dnr9yCO%2F
> > wl8sEw1LYSlX5
> > > > nPHqnlJ5mKm%2B7A%3D&reserved=0
> > > >
> > > > we are using "power-domains" property for cpu perf and gpu/vpu
> > perf.
> > > >
> > > > If cpufreq.off=1 is set in bootargs, the vpu/gpu driver will defer
> > probe.
> > > >
> > >
> > > OK, does the probe of these drivers get called or they don't as the
> > > driver core doesn't allow that ? I just have a dummy driver for mali
> > > on Juno which just does dev_pm_domain_attach_list() in the probe
> > and
> > > it seem to succeed even when cpufreq.off=1 is passed. I see
> > > scmi-cpufreq failing with -ENODEV as expected.
> > >
> > > I need to follow the code and check if I can somehow reproduce.
> Also
> > > are you sure this is not with anything in the downstream code ?
> Also
> > > have you tried this with v6.14-rc* ? Are you sure all the fw_devlink
> > > code is backported in the tree you pointed me which is v6.6-stable ?
> > >
> >
> > I even tried the above branch, but no luck. The above is neither
> > latest stable version nor pure stable. It has few extra patches
> > backported though IIUC. Anyways any pointers to enable me to
> reproduce
> > the issue would be much appreciated.
> 
> I will setup test based latest linux-next and share results. Please wait.


Based on linux-next, I added below node:

+
+               test@4f000000 {
+                       compatible = "fsl,imx-test";
+                       power-domains = <&scmi_devpd IMX95_PD_VPU>, <&scmi_perf IMX95_PERF_VPU>;
+                       power-domain-names = "vpumix", "vpuperf";
+               };

I not write a driver for it, so just check devlink information from sysfs interface.

From below sys directory, this test device takes scmi_dev.4 and scmi_dev.3 as supplier.
root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# ls
driver_override  of_node  subsystem                          supplier:scmi_protocol:scmi_dev.4  waiting_for_supplier
modalias         power    supplier:scmi_protocol:scmi_dev.3  uevent

Checking scmi_dev.4 below, it is scmi cpufreq, not the scmi perf device.
scmi_dev.3 is correct, it is genpd.

root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.4/modalias
scmi_dev.4:13:cpufreq
root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.3/modalias
scmi_dev.3:11:genpd
root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000#


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

Hope this is clear.

Regards,
Peng.

> 
> Thanks,
> Peng.
> >
> > --
> > Regards,
> > Sudeep

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

[Sudeep Holla]

On Tue, Mar 11, 2025 at 11:12:45AM +0000, Peng Fan wrote:
> Based on linux-next, I added below node:
>
> +
> +               test@4f000000 {
> +                       compatible = "fsl,imx-test";
> +                       power-domains = <&scmi_devpd IMX95_PD_VPU>, <&scmi_perf IMX95_PERF_VPU>;
> +                       power-domain-names = "vpumix", "vpuperf";
> +               };
>
> I not write a driver for it, so just check devlink information from sysfs interface.
>
> From below sys directory, this test device takes scmi_dev.4 and scmi_dev.3 as supplier.
> root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# ls
> driver_override  of_node  subsystem                          supplier:scmi_protocol:scmi_dev.4  waiting_for_supplier
> modalias         power    supplier:scmi_protocol:scmi_dev.3  uevent
>
> Checking scmi_dev.4 below, it is scmi cpufreq, not the scmi perf device.
> scmi_dev.3 is correct, it is genpd.
>
> root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.4/modalias
> scmi_dev.4:13:cpufreq
> root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000# cat /sys/bus/scmi_protocol/devices/scmi_dev.3/modalias
> scmi_dev.3:11:genpd
> root@imx95evk:/sys/bus/platform/devices/soc:test@4f000000#
>
>
> 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.

--
Regards,
Sudeep

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Peng Fan]

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/

Thanks,
Peng.

>
>-- 
>Regards,
>Sudeep

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

[Peng Fan]

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.

Thanks,
Peng

>
>Thanks,
>Peng.
>
>>
>>-- 
>>Regards,
>>Sudeep

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

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

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

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

[Peng Fan]

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
>

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

[Sudeep Holla]

On Sun, Apr 20, 2025 at 10:09:44PM +0800, Peng Fan wrote:
> On Thu, Apr 17, 2025 at 03:26:42PM +0100, Sudeep Holla wrote:
> 
> >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.
> 

Agreed and good point.

> Anyway, you decide which to go :)

Still undecided 🙁

-- 
Regards,
Sudeep

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

[Peng Fan]

Hi Sudeep,
On Tue, Apr 22, 2025 at 11:16:44AM +0100, Sudeep Holla wrote:
>On Sun, Apr 20, 2025 at 10:09:44PM +0800, Peng Fan wrote:
>> On Thu, Apr 17, 2025 at 03:26:42PM +0100, Sudeep Holla wrote:
>> 
>> >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.
>> 
>
>Agreed and good point.
>
>> Anyway, you decide which to go :)
>
>Still undecided 🙁

Is this still in you list?

Thanks,
Peng.
>
>-- 
>Regards,
>Sudeep