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

Commit Message

Peng Fan (OSS) July 29, 2024, 7:03 a.m. UTC

From: Peng Fan <[e-mail]>

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 <[e-mail]>
---

V2:
 Use A!=B to replace !(A == B)
 Add fixes tag
 This might be a workaround, but since this is a fix, it is simple for
 backporting.

V1:
 https://lore.kernel.org/all/[e-mail]/

 drivers/firmware/arm_scmi/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Peng Fan Aug. 13, 2024, 12:26 a.m. UTC | #1

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

Any comments?

Thanks,
Peng.
> 
> From: Peng Fan <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
> V2:
>  Use A!=B to replace !(A == B)
>  Add fixes tag
>  This might be a workaround, but since this is a fix, it is simple for
> backporting.
> 
> V1:
>  https://lore.kernel.org/all/20240717093515.327647-1-
> [e-mail]/
> 
>  drivers/firmware/arm_scmi/bus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c
> b/drivers/firmware/arm_scmi/bus.c index
> 96b2e5f9a8ef..be91a82e0cda 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -395,7 +395,8 @@ __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));
> +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> "cpufreq"))
> +		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);
> --
> 2.37.1

Dhruva Gole Aug. 13, 2024, 8:57 a.m. UTC | #2

On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[e-mail]>
> 
> 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

The commit message itself seems very specific to some platform to me.
What about platforms that don't atall have a GPU? Why would they care
about this?

> 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 <[e-mail]>
> ---
> 
> V2:
>  Use A!=B to replace !(A == B)
>  Add fixes tag
>  This might be a workaround, but since this is a fix, it is simple for
>  backporting.

More than a workaround, it feels like a HACK to me.

> 
> V1:
>  https://lore.kernel.org/all/[e-mail]/
> 
>  drivers/firmware/arm_scmi/bus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 96b2e5f9a8ef..be91a82e0cda 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -395,7 +395,8 @@ __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));
> +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name, "cpufreq"))
> +		device_set_node(&scmi_dev->dev, of_fwnode_handle(np));

I kind of disagree with the idea here to be specific about the
PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver right?
Why bring in specific code into a bus driver? We will never fix the
actual root cause of the issue this way.

Peng Fan Aug. 13, 2024, 10:25 a.m. UTC | #3

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[e-mail]>
> >
> > 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
> 
> The commit message itself seems very specific to some platform to me.
> What about platforms that don't atall have a GPU? Why would they
> care about this?

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

> 
> > 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 <[e-mail]>
> > ---
> >
> > V2:
> >  Use A!=B to replace !(A == B)
> >  Add fixes tag
> >  This might be a workaround, but since this is a fix, it is simple for
> > backporting.
> 
> More than a workaround, it feels like a HACK to me.
> 
> >
> > V1:
> >
> >
> >
> >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/bus.c
> > b/drivers/firmware/arm_scmi/bus.c index
> 96b2e5f9a8ef..be91a82e0cda
> > 100644
> > --- a/drivers/firmware/arm_scmi/bus.c
> > +++ b/drivers/firmware/arm_scmi/bus.c
> > @@ -395,7 +395,8 @@ __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));
> > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> "cpufreq"))
> > +		device_set_node(&scmi_dev->dev,
> of_fwnode_handle(np));
> 
> I kind of disagree with the idea here to be specific about the
> PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver right?
> Why bring in specific code into a bus driver? We will never fix the
> actual root cause of the issue this way.

The root cause is fwnode devlink only supports one fwnode, one device.
1:1 match. But current arm scmi driver use one fwnode for two devices.

If your platform has scmi cpufreq and scmi device performance domain,
you might see that some devices are consumer of scmi cpufreq, but actually
they should be consumer of scmi device performance domain.

I not have a good idea that this is fw devlink design that only allows
1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
If not, fw devlink should be updated.

The current patch is the simplest method for stable tree fixes as I
could work out.

Thanks,
Peng.
.

> 
> --
> Best regards,
> Dhruva Gole <[e-mail]>

Cristian Marussi Aug. 13, 2024, 12:47 p.m. UTC | #4

On Tue, Aug 13, 2024 at 02:27:03PM +0530, Dhruva Gole wrote:
> On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[e-mail]>
> > 

Hi Peng, Dhruva

> > 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
> 
> The commit message itself seems very specific to some platform to me.
> What about platforms that don't atall have a GPU? Why would they care
> about this?
> 

+1

If you faced a problem on specific platform and a specific config the aim
here should be to solve it generally...not to just hack something around
to fix your specific case while potentially breaking future users
(evenm if now cpufreq does not need it...)

> > 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 <[e-mail]>
> > ---
> > 
> > V2:
> >  Use A!=B to replace !(A == B)
> >  Add fixes tag
> >  This might be a workaround, but since this is a fix, it is simple for
> >  backporting.
> 
> More than a workaround, it feels like a HACK to me.
> 

+1 ... exactly... I did not have time to dig into this but I would like
to understand better the implications of this, in order to avoid to
inadvertently fix one issue and create a few more :P

...as an example, for starters, killing devlink straigh away will most
probably kill also the auto-unbinding of the stack when a supplier is removed...

> > 
> > V1:
> >  https://lore.kernel.org/all/[e-mail]/
> > 
> >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > index 96b2e5f9a8ef..be91a82e0cda 100644
> > --- a/drivers/firmware/arm_scmi/bus.c
> > +++ b/drivers/firmware/arm_scmi/bus.c
> > @@ -395,7 +395,8 @@ __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));
> > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name, "cpufreq"))
> > +		device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> 
> I kind of disagree with the idea here to be specific about the
> PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver right?
> Why bring in specific code into a bus driver? We will never fix the
> actual root cause of the issue this way.
> 

+1 again

Whatevier solution we will adopt ahs to be general...like enabling SCMI
drivers to flag a particular condition/need so the core can take
countermeasures...beside being in the bus code, we cannot really think
of just keep blacklisting here all the combination we dont like...

Thanks,
Cristian

Cristian Marussi Aug. 13, 2024, 1:38 p.m. UTC | #5

On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > for scmi cpufreq
> > 
> > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[e-mail]>
> > >
> > > 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
> > 
> > The commit message itself seems very specific to some platform to me.
> > What about platforms that don't atall have a GPU? Why would they
> > care about this?
> 
> It is a generic issue if a platform has performance domain to serve
> scmi cpufreq and device performance level.
> 
> > 
> > > 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 <[e-mail]>
> > > ---
> > >
> > > V2:
> > >  Use A!=B to replace !(A == B)
> > >  Add fixes tag
> > >  This might be a workaround, but since this is a fix, it is simple for
> > > backporting.
> > 
> > More than a workaround, it feels like a HACK to me.
> > 
> > >
> > > V1:
> > >
> > >
> > >
> > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > b/drivers/firmware/arm_scmi/bus.c index
> > 96b2e5f9a8ef..be91a82e0cda
> > > 100644
> > > --- a/drivers/firmware/arm_scmi/bus.c
> > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > @@ -395,7 +395,8 @@ __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));
> > > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> > "cpufreq"))
> > > +		device_set_node(&scmi_dev->dev,
> > of_fwnode_handle(np));
> > 
> > I kind of disagree with the idea here to be specific about the
> > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver right?
> > Why bring in specific code into a bus driver? We will never fix the
> > actual root cause of the issue this way.
> 
> The root cause is fwnode devlink only supports one fwnode, one device.
> 1:1 match. But current arm scmi driver use one fwnode for two devices.
> 
> If your platform has scmi cpufreq and scmi device performance domain,
> you might see that some devices are consumer of scmi cpufreq, but actually
> they should be consumer of scmi device performance domain.
> 
> I not have a good idea that this is fw devlink design that only allows
> 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
> If not, fw devlink should be updated.
> 
> The current patch is the simplest method for stable tree fixes as I
> could work out.

So this is the same root cause at the end of the issues you had with IMX
pinctrl coexistence...i.e. the SCMI stack creates scmi devices that
embeds the protocol node, BUT since you can have multiple device/drivers
doing different things on different resources within the same protocol
you can end with 2 devices having the same embedded device_node, since
we dont really have anything else to use as device_node, I rigth ?

Thanks,
Cristian

Peng Fan Aug. 13, 2024, 1:52 p.m. UTC | #6

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for scmi cpufreq
> > >
> > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <[e-mail]>
> > > >
> > > > 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
> > >
> > > The commit message itself seems very specific to some platform to
> me.
> > > What about platforms that don't atall have a GPU? Why would
> they
> > > care about this?
> >
> > It is a generic issue if a platform has performance domain to serve
> > scmi cpufreq and device performance level.
> >
> > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >
> > > > V2:
> > > >  Use A!=B to replace !(A == B)
> > > >  Add fixes tag
> > > >  This might be a workaround, but since this is a fix, it is simple
> > > > for backporting.
> > >
> > > More than a workaround, it feels like a HACK to me.
> > >
> > > >
> > > > V1:
> > > >
> > > >
> > > >
> > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > b/drivers/firmware/arm_scmi/bus.c index
> > > 96b2e5f9a8ef..be91a82e0cda
> > > > 100644
> > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > @@ -395,7 +395,8 @@ __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));
> > > > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> > > "cpufreq"))
> > > > +		device_set_node(&scmi_dev->dev,
> > > of_fwnode_handle(np));
> > >
> > > I kind of disagree with the idea here to be specific about the
> > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver
> right?
> > > Why bring in specific code into a bus driver? We will never fix the
> > > actual root cause of the issue this way.
> >
> > The root cause is fwnode devlink only supports one fwnode, one
> device.
> > 1:1 match. But current arm scmi driver use one fwnode for two
> devices.
> >
> > If your platform has scmi cpufreq and scmi device performance
> domain,
> > you might see that some devices are consumer of scmi cpufreq, but
> > actually they should be consumer of scmi device performance
> domain.
> >
> > I not have a good idea that this is fw devlink design that only allows
> > 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
> > If not, fw devlink should be updated.
> >
> > The current patch is the simplest method for stable tree fixes as I
> > could work out.
> 
> So this is the same root cause at the end of the issues you had with
> IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices that
> embeds the protocol node, BUT since you can have multiple
> device/drivers doing different things on different resources within the
> same protocol you can end with 2 devices having the same embedded
> device_node, since we dont really have anything else to use as
> device_node, I rigth ?

I think, yes. And you remind me that with PINCTRL_SCMI and
CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
will only take one to set the fwnode device pointer depends on
the order to run __scmi_device_create.

So not only perf, pinctrl also has issue here, fwnode devlink will
not work properly for pinctrl/perf.

Regards,
Peng.

> 
> Thanks,
> Cristian

Cristian Marussi Aug. 13, 2024, 3:09 p.m. UTC | #7

On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > for scmi cpufreq
> > 
> > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > fwnode
> > > > for scmi cpufreq
> > > >
> > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <[e-mail]>
> > > > >
> > > > > 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
> > > >
> > > > The commit message itself seems very specific to some platform to
> > me.
> > > > What about platforms that don't atall have a GPU? Why would
> > they
> > > > care about this?
> > >
> > > It is a generic issue if a platform has performance domain to serve
> > > scmi cpufreq and device performance level.
> > >
> > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >  Use A!=B to replace !(A == B)
> > > > >  Add fixes tag
> > > > >  This might be a workaround, but since this is a fix, it is simple
> > > > > for backporting.
> > > >
> > > > More than a workaround, it feels like a HACK to me.
> > > >
> > > > >
> > > > > V1:
> > > > >
> > > > >
> > > > >
> > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > 100644
> > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > @@ -395,7 +395,8 @@ __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));
> > > > > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> > > > "cpufreq"))
> > > > > +		device_set_node(&scmi_dev->dev,
> > > > of_fwnode_handle(np));
> > > >
> > > > I kind of disagree with the idea here to be specific about the
> > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver
> > right?
> > > > Why bring in specific code into a bus driver? We will never fix the
> > > > actual root cause of the issue this way.
> > >
> > > The root cause is fwnode devlink only supports one fwnode, one
> > device.
> > > 1:1 match. But current arm scmi driver use one fwnode for two
> > devices.
> > >
> > > If your platform has scmi cpufreq and scmi device performance
> > domain,
> > > you might see that some devices are consumer of scmi cpufreq, but
> > > actually they should be consumer of scmi device performance
> > domain.
> > >
> > > I not have a good idea that this is fw devlink design that only allows
> > > 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
> > > If not, fw devlink should be updated.
> > >
> > > The current patch is the simplest method for stable tree fixes as I
> > > could work out.
> > 
> > So this is the same root cause at the end of the issues you had with
> > IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices that
> > embeds the protocol node, BUT since you can have multiple
> > device/drivers doing different things on different resources within the
> > same protocol you can end with 2 devices having the same embedded
> > device_node, since we dont really have anything else to use as
> > device_node, I rigth ?
> 
> I think, yes. And you remind me that with PINCTRL_SCMI and
> CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
> will only take one to set the fwnode device pointer depends on
> the order to run __scmi_device_create.
> 
> So not only perf, pinctrl also has issue here, fwnode devlink will
> not work properly for pinctrl/perf.

...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
mutually exclusive in the code (@probe time) itself as far as I can remember
about what you did, so why devlink should have that issue there ?
Have you seen any issue in this regards while having loaded pinctrl_scmi
and pinctrl_imx_scmi ?

I want to have a better look next days about this devlink issue that you
reported...it still not clear to me...from device_link_add() docs, it
seems indeed that it will return the old existing link if a link exist
already between that same supplier/consumer devices pair....but from the
code (at first sight) it seems that the check is made agains the devices
not their embeded device_nodes, but here (and in pinctrl/imx case) you will
have 2 distinct consumer devices (with same device_node)...I may have
missed something in your exaplanation....

Thanks,
Cristian

Peng Fan Aug. 13, 2024, 3:17 p.m. UTC | #8

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for scmi cpufreq
> > >
> > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > fwnode
> > > > > for scmi cpufreq
> > > > >
> > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > From: Peng Fan <[e-mail]>
> > > > > >
> > > > > > 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
> > > > >
> > > > > The commit message itself seems very specific to some platform
> > > > > to
> > > me.
> > > > > What about platforms that don't atall have a GPU? Why would
> > > they
> > > > > care about this?
> > > >
> > > > It is a generic issue if a platform has performance domain to
> > > > serve scmi cpufreq and device performance level.
> > > >
> > > > >
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > V2:
> > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This might be a
> > > > > > workaround, but since this is a fix, it is simple for
> > > > > > backporting.
> > > > >
> > > > > More than a workaround, it feels like a HACK to me.
> > > > >
> > > > > >
> > > > > > V1:
> > > > > >
> > > > > >
> > > > > >
> > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > 100644
> > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > +	if ((protocol != SCMI_PROTOCOL_PERF) ||
> strcmp(name,
> > > > > "cpufreq"))
> > > > > > +		device_set_node(&scmi_dev->dev,
> > > > > of_fwnode_handle(np));
> > > > >
> > > > > I kind of disagree with the idea here to be specific about the
> > > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus
> driver
> > > right?
> > > > > Why bring in specific code into a bus driver? We will never fix
> > > > > the actual root cause of the issue this way.
> > > >
> > > > The root cause is fwnode devlink only supports one fwnode, one
> > > device.
> > > > 1:1 match. But current arm scmi driver use one fwnode for two
> > > devices.
> > > >
> > > > If your platform has scmi cpufreq and scmi device performance
> > > domain,
> > > > you might see that some devices are consumer of scmi cpufreq,
> but
> > > > actually they should be consumer of scmi device performance
> > > domain.
> > > >
> > > > I not have a good idea that this is fw devlink design that only
> > > > allows
> > > > 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
> > > > If not, fw devlink should be updated.
> > > >
> > > > The current patch is the simplest method for stable tree fixes as
> > > > I could work out.
> > >
> > > So this is the same root cause at the end of the issues you had with
> > > IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices
> > > that embeds the protocol node, BUT since you can have multiple
> > > device/drivers doing different things on different resources within
> > > the same protocol you can end with 2 devices having the same
> > > embedded device_node, since we dont really have anything else to
> use
> > > as device_node, I rigth ?
> >
> > I think, yes. And you remind me that with PINCTRL_SCMI and
> > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node will
> only
> > take one to set the fwnode device pointer depends on the order to
> run
> > __scmi_device_create.
> >
> > So not only perf, pinctrl also has issue here, fwnode devlink will not
> > work properly for pinctrl/perf.
> 
> ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
> mutually exclusive in the code (@probe time) itself as far as I can
> remember about what you did, so why devlink should have that issue
> there ?
> Have you seen any issue in this regards while having loaded
> pinctrl_scmi and pinctrl_imx_scmi ?

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

> 
> I want to have a better look next days about this devlink issue that you
> reported...it still not clear to me...from device_link_add() docs, it seems
> indeed that it will return the old existing link if a link exist already
> between that same supplier/consumer devices pair....but from the code
> (at first sight) it seems that the check is made agains the devices not
> their embeded device_nodes, but here (and in pinctrl/imx case) you
> will have 2 distinct consumer devices (with same device_node)...I may
> have missed something in your exaplanation....

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

Regards,
Peng.

> 
> Thanks,
> Cristian

Peng Fan Aug. 14, 2024, 3:35 a.m. UTC | #9

> Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for
> > scmi cpufreq
> >
> > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > fwnode
> > > > for scmi cpufreq
> > > >
> > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > fwnode
> > > > > > for scmi cpufreq
> > > > > >
> > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > > From: Peng Fan <[e-mail]>
> > > > > > >
> > > > > > > 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
> > > > > >
> > > > > > The commit message itself seems very specific to some
> platform
> > > > > > to
> > > > me.
> > > > > > What about platforms that don't atall have a GPU? Why
> would
> > > > they
> > > > > > care about this?
> > > > >
> > > > > It is a generic issue if a platform has performance domain to
> > > > > serve scmi cpufreq and device performance level.
> > > > >
> > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >
> > > > > > > V2:
> > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This might be
> > > > > > > a workaround, but since this is a fix, it is simple for
> > > > > > > backporting.
> > > > > >
> > > > > > More than a workaround, it feels like a HACK to me.
> > > > > >
> > > > > > >
> > > > > > > V1:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > 100644
> > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > +	if ((protocol != SCMI_PROTOCOL_PERF) ||
> > strcmp(name,
> > > > > > "cpufreq"))
> > > > > > > +		device_set_node(&scmi_dev->dev,
> > > > > > of_fwnode_handle(np));
> > > > > >
> > > > > > I kind of disagree with the idea here to be specific about the
> > > > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus
> > driver
> > > > right?
> > > > > > Why bring in specific code into a bus driver? We will never
> > > > > > fix the actual root cause of the issue this way.
> > > > >
> > > > > The root cause is fwnode devlink only supports one fwnode, one
> > > > device.
> > > > > 1:1 match. But current arm scmi driver use one fwnode for two
> > > > devices.
> > > > >
> > > > > If your platform has scmi cpufreq and scmi device performance
> > > > domain,
> > > > > you might see that some devices are consumer of scmi cpufreq,
> > but
> > > > > actually they should be consumer of scmi device performance
> > > > domain.
> > > > >
> > > > > I not have a good idea that this is fw devlink design that only
> > > > > allows
> > > > > 1 fwnode has 1 device or not. If yes, that arm scmi should be
> fixed.
> > > > > If not, fw devlink should be updated.
> > > > >
> > > > > The current patch is the simplest method for stable tree fixes
> > > > > as I could work out.
> > > >
> > > > So this is the same root cause at the end of the issues you had
> > > > with IMX pinctrl coexistence...i.e. the SCMI stack creates scmi
> > > > devices that embeds the protocol node, BUT since you can have
> > > > multiple device/drivers doing different things on different
> > > > resources within the same protocol you can end with 2 devices
> > > > having the same embedded device_node, since we dont really
> have
> > > > anything else to
> > use
> > > > as device_node, I rigth ?
> > >
> > > I think, yes. And you remind me that with PINCTRL_SCMI and
> > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
> will
> > only
> > > take one to set the fwnode device pointer depends on the order to
> > run
> > > __scmi_device_create.
> > >
> > > So not only perf, pinctrl also has issue here, fwnode devlink will
> > > not work properly for pinctrl/perf.
> >
> > ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
> > mutually exclusive in the code (@probe time) itself as far as I can
> > remember about what you did, so why devlink should have that issue
> > there ?
> > Have you seen any issue in this regards while having loaded
> > pinctrl_scmi and pinctrl_imx_scmi ?
> 
> No. it works well in my setup. I am just worried that there might be
> issues because fwnode only has one dev pointer, see device_add.
> 
> >
> > I want to have a better look next days about this devlink issue that
> > you reported...it still not clear to me...from device_link_add() docs,
> > it seems indeed that it will return the old existing link if a link
> > exist already between that same supplier/consumer devices
> pair....but
> > from the code (at first sight) it seems that the check is made agains
> > the devices not their embeded device_nodes, but here (and in
> > pinctrl/imx case) you will have 2 distinct consumer devices (with
> same
> > device_node)...I may have missed something in your exaplanation....
> 
> It might be false alarm for pinctrl, but it is true issue for perf.

Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI
and PINCTRL_IMX_SCMI both enabled, two scmi devices
will be created. So it depends on device creation order, 1st
created device will be used as supplier.

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

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

Thanks,
Peng.

> 
> Regards,
> Peng.
> 
> >
> > Thanks,
> > Cristian

Saravana Kannan Aug. 14, 2024, 6:51 a.m. UTC | #10

On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]> wrote:
>
> > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > for scmi cpufreq
> >
> > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > for
> > > scmi cpufreq
> > >
> > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > fwnode
> > > > > for scmi cpufreq
> > > > >
> > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > > fwnode
> > > > > > > for scmi cpufreq
> > > > > > >
> > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > >
> > > > > > > > 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
> > > > > > >
> > > > > > > The commit message itself seems very specific to some
> > platform
> > > > > > > to
> > > > > me.
> > > > > > > What about platforms that don't atall have a GPU? Why
> > would
> > > > > they
> > > > > > > care about this?
> > > > > >
> > > > > > It is a generic issue if a platform has performance domain to
> > > > > > serve scmi cpufreq and device performance level.
> > > > > >
> > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > V2:
> > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This might be
> > > > > > > > a workaround, but since this is a fix, it is simple for
> > > > > > > > backporting.
> > > > > > >
> > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > >
> > > > > > > >
> > > > > > > > V1:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > 100644
> > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > strcmp(name,
> > > > > > > "cpufreq"))
> > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > of_fwnode_handle(np));
> > > > > > >
> > > > > > > I kind of disagree with the idea here to be specific about the
> > > > > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus
> > > driver
> > > > > right?
> > > > > > > Why bring in specific code into a bus driver? We will never
> > > > > > > fix the actual root cause of the issue this way.
> > > > > >
> > > > > > The root cause is fwnode devlink only supports one fwnode, one
> > > > > device.
> > > > > > 1:1 match. But current arm scmi driver use one fwnode for two
> > > > > devices.
> > > > > >
> > > > > > If your platform has scmi cpufreq and scmi device performance
> > > > > domain,
> > > > > > you might see that some devices are consumer of scmi cpufreq,
> > > but
> > > > > > actually they should be consumer of scmi device performance
> > > > > domain.
> > > > > >
> > > > > > I not have a good idea that this is fw devlink design that only
> > > > > > allows
> > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi should be
> > fixed.
> > > > > > If not, fw devlink should be updated.
> > > > > >
> > > > > > The current patch is the simplest method for stable tree fixes
> > > > > > as I could work out.
> > > > >
> > > > > So this is the same root cause at the end of the issues you had
> > > > > with IMX pinctrl coexistence...i.e. the SCMI stack creates scmi
> > > > > devices that embeds the protocol node, BUT since you can have
> > > > > multiple device/drivers doing different things on different
> > > > > resources within the same protocol you can end with 2 devices
> > > > > having the same embedded device_node, since we dont really
> > have
> > > > > anything else to
> > > use
> > > > > as device_node, I rigth ?
> > > >
> > > > I think, yes. And you remind me that with PINCTRL_SCMI and
> > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
> > will
> > > only
> > > > take one to set the fwnode device pointer depends on the order to
> > > run
> > > > __scmi_device_create.
> > > >
> > > > So not only perf, pinctrl also has issue here, fwnode devlink will
> > > > not work properly for pinctrl/perf.
> > >
> > > ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
> > > mutually exclusive in the code (@probe time) itself as far as I can
> > > remember about what you did, so why devlink should have that issue
> > > there ?
> > > Have you seen any issue in this regards while having loaded
> > > pinctrl_scmi and pinctrl_imx_scmi ?
> >
> > No. it works well in my setup. I am just worried that there might be
> > issues because fwnode only has one dev pointer, see device_add.
> >
> > >
> > > I want to have a better look next days about this devlink issue that
> > > you reported...it still not clear to me...from device_link_add() docs,
> > > it seems indeed that it will return the old existing link if a link
> > > exist already between that same supplier/consumer devices
> > pair....but
> > > from the code (at first sight) it seems that the check is made agains
> > > the devices not their embeded device_nodes, but here (and in
> > > pinctrl/imx case) you will have 2 distinct consumer devices (with
> > same
> > > device_node)...I may have missed something in your exaplanation....
> >
> > It might be false alarm for pinctrl, but it is true issue for perf.
>
> Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI
> and PINCTRL_IMX_SCMI both enabled, two scmi devices
> will be created. So it depends on device creation order, 1st
> created device will be used as supplier.
>
> On i.MX, there is no issue with both enabled, it is because
> the imx scmi device is created first. If the generic pinctrl scmi
> device created first, imx will have issue.
>
> In the end, this is generic fw_devlink limitation or arm scmi driver
> issue.

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

This patch itself is definitely a hack.

The problem isn't so much that fw_devlink doesn't want to support
multiple devices getting instantiated from one DT node. The problem is
that there's no way to know which of the multiple devices is the real
supplier just by looking at the information in devicetree/firmware
(the fw in fw_devlink). And keep in mind that one of the main
requirements of fw_devlink is to work before any driver is loaded and
not depend on drivers for correctness of the dependency information
because it needs to work on a fully modular kernel too. So, fw_devlink
just picks the first device that's instantiated from a DT node.

I really hate folks creating multiple devices from one DT node. One IP
block can support multiple things, there's no need to instantiate
multiple devices for it. The same driver could have just as easily
registered with multiple frameworks. So, ideally I'd want us to fix
this issue in the SCMI framework code. In the case where the same SCMI
node is creating two devices, can they both probe successfully? If
yes, why are we not using a child node or a separate node for this
second device? If it's always one or the other, why are we creating
two devices? Can you please point to specific upstream DT examples for
me to get a better handle on what's going on?

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

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

-Saravana

Peng Fan Aug. 14, 2024, 7:04 a.m. UTC | #11

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]>
> wrote:
> >
> > > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for scmi cpufreq
> > >
> > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for
> > > > scmi cpufreq
> > > >
> > > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > fwnode
> > > > > > for scmi cpufreq
> > > > > >
> > > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass
> > > > > > > > set
> > > > > > fwnode
> > > > > > > > for scmi cpufreq
> > > > > > > >
> > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > > >
> > > > > > > > > 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
> > > > > > > >
> > > > > > > > The commit message itself seems very specific to some
> > > platform
> > > > > > > > to
> > > > > > me.
> > > > > > > > What about platforms that don't atall have a GPU? Why
> > > would
> > > > > > they
> > > > > > > > care about this?
> > > > > > >
> > > > > > > It is a generic issue if a platform has performance domain
> > > > > > > to serve scmi cpufreq and device performance level.
> > > > > > >
> > > > > > > >
> > > > > > > > > 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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > V2:
> > > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This
> > > > > > > > > might be a workaround, but since this is a fix, it is
> > > > > > > > > simple for backporting.
> > > > > > > >
> > > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > V1:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > > strcmp(name,
> > > > > > > > "cpufreq"))
> > > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > > of_fwnode_handle(np));
> > > > > > > >
> > > > > > > > I kind of disagree with the idea here to be specific about
> > > > > > > > the PROTOCOL_PERF or cpufreq. This is a generic arm scmi
> > > > > > > > bus
> > > > driver
> > > > > > right?
> > > > > > > > Why bring in specific code into a bus driver? We will
> > > > > > > > never fix the actual root cause of the issue this way.
> > > > > > >
> > > > > > > The root cause is fwnode devlink only supports one fwnode,
> > > > > > > one
> > > > > > device.
> > > > > > > 1:1 match. But current arm scmi driver use one fwnode for
> > > > > > > two
> > > > > > devices.
> > > > > > >
> > > > > > > If your platform has scmi cpufreq and scmi device
> > > > > > > performance
> > > > > > domain,
> > > > > > > you might see that some devices are consumer of scmi
> > > > > > > cpufreq,
> > > > but
> > > > > > > actually they should be consumer of scmi device
> performance
> > > > > > domain.
> > > > > > >
> > > > > > > I not have a good idea that this is fw devlink design that
> > > > > > > only allows
> > > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi should
> > > > > > > be
> > > fixed.
> > > > > > > If not, fw devlink should be updated.
> > > > > > >
> > > > > > > The current patch is the simplest method for stable tree
> > > > > > > fixes as I could work out.
> > > > > >
> > > > > > So this is the same root cause at the end of the issues you
> > > > > > had with IMX pinctrl coexistence...i.e. the SCMI stack creates
> > > > > > scmi devices that embeds the protocol node, BUT since you
> can
> > > > > > have multiple device/drivers doing different things on
> > > > > > different resources within the same protocol you can end with
> > > > > > 2 devices having the same embedded device_node, since we
> dont
> > > > > > really
> > > have
> > > > > > anything else to
> > > > use
> > > > > > as device_node, I rigth ?
> > > > >
> > > > > I think, yes. And you remind me that with PINCTRL_SCMI and
> > > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl
> node
> > > will
> > > > only
> > > > > take one to set the fwnode device pointer depends on the order
> > > > > to
> > > > run
> > > > > __scmi_device_create.
> > > > >
> > > > > So not only perf, pinctrl also has issue here, fwnode devlink
> > > > > will not work properly for pinctrl/perf.
> > > >
> > > > ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl
> > > > are mutually exclusive in the code (@probe time) itself as far as
> > > > I can remember about what you did, so why devlink should have
> that
> > > > issue there ?
> > > > Have you seen any issue in this regards while having loaded
> > > > pinctrl_scmi and pinctrl_imx_scmi ?
> > >
> > > No. it works well in my setup. I am just worried that there might be
> > > issues because fwnode only has one dev pointer, see device_add.
> > >
> > > >
> > > > I want to have a better look next days about this devlink issue
> > > > that you reported...it still not clear to me...from
> > > > device_link_add() docs, it seems indeed that it will return the
> > > > old existing link if a link exist already between that same
> > > > supplier/consumer devices
> > > pair....but
> > > > from the code (at first sight) it seems that the check is made
> > > > agains the devices not their embeded device_nodes, but here
> (and
> > > > in pinctrl/imx case) you will have 2 distinct consumer devices
> > > > (with
> > > same
> > > > device_node)...I may have missed something in your
> exaplanation....
> > >
> > > It might be false alarm for pinctrl, but it is true issue for perf.
> >
> > Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI and
> > PINCTRL_IMX_SCMI both enabled, two scmi devices will be created.
> So it
> > depends on device creation order, 1st created device will be used as
> > supplier.
> >
> > On i.MX, there is no issue with both enabled, it is because the imx
> > scmi device is created first. If the generic pinctrl scmi device
> > created first, imx will have issue.
> >
> > In the end, this is generic fw_devlink limitation or arm scmi driver
> > issue.
> 
> +1 to what Cristian and Dhruva said in other parts of this thread.
> 
> This patch itself is definitely a hack.
> 
> The problem isn't so much that fw_devlink doesn't want to support
> multiple devices getting instantiated from one DT node. The problem is
> that there's no way to know which of the multiple devices is the real
> supplier just by looking at the information in devicetree/firmware (the
> fw in fw_devlink). 

Yes. I see.

And keep in mind that one of the main requirements
> of fw_devlink is to work before any driver is loaded and not depend on
> drivers for correctness of the dependency information because it needs
> to work on a fully modular kernel too. So, fw_devlink just picks the first
> device that's instantiated from a DT node.
> 
> I really hate folks creating multiple devices from one DT node. One IP
> block can support multiple things, there's no need to instantiate
> multiple devices for it. The same driver could have just as easily
> registered with multiple frameworks. So, ideally I'd want us to fix this
> issue in the SCMI framework code. In the case where the same SCMI
> node is creating two devices, can they both probe successfully?

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

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

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

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

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

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

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

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

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

> 
> Btw, there is the deferred_probe_timeout command line option that
> can be used so that fw_devlink stops enforcing dependencies where
> there are no supplier drivers for a device after a timeout. It's not ideal,
> but it's something to unblock you.
> 
> The best fw_devlink could do is just not enforce any dependencies if
> there is more than one device instantiated for a given supplier DT node.

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

Thanks,
Peng.

> 
> -Saravana

Saravana Kannan Aug. 14, 2024, 7:17 a.m. UTC | #12

On Wed, Aug 14, 2024 at 12:04 AM Peng Fan <[e-mail]> wrote:
>
> > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > for scmi cpufreq
> >
> > On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]>
> > wrote:
> > >
> > > > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > fwnode
> > > > for scmi cpufreq
> > > >
> > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > fwnode
> > > > for
> > > > > scmi cpufreq
> > > > >
> > > > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > > fwnode
> > > > > > > for scmi cpufreq
> > > > > > >
> > > > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass
> > > > > > > > > set
> > > > > > > fwnode
> > > > > > > > > for scmi cpufreq
> > > > > > > > >
> > > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > >
> > > > > > > > > The commit message itself seems very specific to some
> > > > platform
> > > > > > > > > to
> > > > > > > me.
> > > > > > > > > What about platforms that don't atall have a GPU? Why
> > > > would
> > > > > > > they
> > > > > > > > > care about this?
> > > > > > > >
> > > > > > > > It is a generic issue if a platform has performance domain
> > > > > > > > to serve scmi cpufreq and device performance level.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > 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 <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > V2:
> > > > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This
> > > > > > > > > > might be a workaround, but since this is a fix, it is
> > > > > > > > > > simple for backporting.
> > > > > > > > >
> > > > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > V1:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > > > 100644
> > > > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > > > strcmp(name,
> > > > > > > > > "cpufreq"))
> > > > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > > > of_fwnode_handle(np));
> > > > > > > > >
> > > > > > > > > I kind of disagree with the idea here to be specific about
> > > > > > > > > the PROTOCOL_PERF or cpufreq. This is a generic arm scmi
> > > > > > > > > bus
> > > > > driver
> > > > > > > right?
> > > > > > > > > Why bring in specific code into a bus driver? We will
> > > > > > > > > never fix the actual root cause of the issue this way.
> > > > > > > >
> > > > > > > > The root cause is fwnode devlink only supports one fwnode,
> > > > > > > > one
> > > > > > > device.
> > > > > > > > 1:1 match. But current arm scmi driver use one fwnode for
> > > > > > > > two
> > > > > > > devices.
> > > > > > > >
> > > > > > > > If your platform has scmi cpufreq and scmi device
> > > > > > > > performance
> > > > > > > domain,
> > > > > > > > you might see that some devices are consumer of scmi
> > > > > > > > cpufreq,
> > > > > but
> > > > > > > > actually they should be consumer of scmi device
> > performance
> > > > > > > domain.
> > > > > > > >
> > > > > > > > I not have a good idea that this is fw devlink design that
> > > > > > > > only allows
> > > > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi should
> > > > > > > > be
> > > > fixed.
> > > > > > > > If not, fw devlink should be updated.
> > > > > > > >
> > > > > > > > The current patch is the simplest method for stable tree
> > > > > > > > fixes as I could work out.
> > > > > > >
> > > > > > > So this is the same root cause at the end of the issues you
> > > > > > > had with IMX pinctrl coexistence...i.e. the SCMI stack creates
> > > > > > > scmi devices that embeds the protocol node, BUT since you
> > can
> > > > > > > have multiple device/drivers doing different things on
> > > > > > > different resources within the same protocol you can end with
> > > > > > > 2 devices having the same embedded device_node, since we
> > dont
> > > > > > > really
> > > > have
> > > > > > > anything else to
> > > > > use
> > > > > > > as device_node, I rigth ?
> > > > > >
> > > > > > I think, yes. And you remind me that with PINCTRL_SCMI and
> > > > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl
> > node
> > > > will
> > > > > only
> > > > > > take one to set the fwnode device pointer depends on the order
> > > > > > to
> > > > > run
> > > > > > __scmi_device_create.
> > > > > >
> > > > > > So not only perf, pinctrl also has issue here, fwnode devlink
> > > > > > will not work properly for pinctrl/perf.
> > > > >
> > > > > ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl
> > > > > are mutually exclusive in the code (@probe time) itself as far as
> > > > > I can remember about what you did, so why devlink should have
> > that
> > > > > issue there ?
> > > > > Have you seen any issue in this regards while having loaded
> > > > > pinctrl_scmi and pinctrl_imx_scmi ?
> > > >
> > > > No. it works well in my setup. I am just worried that there might be
> > > > issues because fwnode only has one dev pointer, see device_add.
> > > >
> > > > >
> > > > > I want to have a better look next days about this devlink issue
> > > > > that you reported...it still not clear to me...from
> > > > > device_link_add() docs, it seems indeed that it will return the
> > > > > old existing link if a link exist already between that same
> > > > > supplier/consumer devices
> > > > pair....but
> > > > > from the code (at first sight) it seems that the check is made
> > > > > agains the devices not their embeded device_nodes, but here
> > (and
> > > > > in pinctrl/imx case) you will have 2 distinct consumer devices
> > > > > (with
> > > > same
> > > > > device_node)...I may have missed something in your
> > exaplanation....
> > > >
> > > > It might be false alarm for pinctrl, but it is true issue for perf.
> > >
> > > Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI and
> > > PINCTRL_IMX_SCMI both enabled, two scmi devices will be created.
> > So it
> > > depends on device creation order, 1st created device will be used as
> > > supplier.
> > >
> > > On i.MX, there is no issue with both enabled, it is because the imx
> > > scmi device is created first. If the generic pinctrl scmi device
> > > created first, imx will have issue.
> > >
> > > In the end, this is generic fw_devlink limitation or arm scmi driver
> > > issue.
> >
> > +1 to what Cristian and Dhruva said in other parts of this thread.
> >
> > This patch itself is definitely a hack.
> >
> > The problem isn't so much that fw_devlink doesn't want to support
> > multiple devices getting instantiated from one DT node. The problem is
> > that there's no way to know which of the multiple devices is the real
> > supplier just by looking at the information in devicetree/firmware (the
> > fw in fw_devlink).
>
> Yes. I see.
>
> And keep in mind that one of the main requirements
> > of fw_devlink is to work before any driver is loaded and not depend on
> > drivers for correctness of the dependency information because it needs
> > to work on a fully modular kernel too. So, fw_devlink just picks the first
> > device that's instantiated from a DT node.
> >
> > I really hate folks creating multiple devices from one DT node. One IP
> > block can support multiple things, there's no need to instantiate
> > multiple devices for it. The same driver could have just as easily
> > registered with multiple frameworks. So, ideally I'd want us to fix this
> > issue in the SCMI framework code. In the case where the same SCMI
> > node is creating two devices, can they both probe successfully?
>
> In pinctrl case, only one could probe successfully.
> See
> drivers/pinctrl/pinctrl-scmi.c for generic pinctrl scmi driver
> drivers/pinctrl/freescale/pinctrl-imx-scmi.c for i.MX extension.
>
> There is a machine compatible string to make sure only one
> could probe successfully.

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

>
> In scmi performance case, both could probe successfully.
> drivers/cpufreq/scmi-cpufreq.c
> drivers/pmdomain/arm/scmi_perf_domain.c
>
>  If yes,
> > why are we not using a child node or a separate node for this second
> > device? If it's always one or the other, why are we creating two devices?
>
> Sudeep and Cristian may comment on the design goal of scmi drivers.
>
> > Can you please point to specific upstream DT examples for me to get a
> > better handle on what's going on?
>
> See linux-next tree:
> arch/arm64/boot/dts/freescale/imx95.dtsi
>
>                         scmi_perf: protocol@13 {
>                                 reg = <0x13>;
>                                 #power-domain-cells = <1>;
>                         };
>
>                         scmi_iomuxc: protocol@19 {
>                                 reg = <0x19>;
>                         };

Sure, I can see that file and these nodes, but what am I supposed to
make of it? Can you be more specific please? I'm not familiar with the
protocol number mapping. Which nodes are the consumers in this
example? Which node has more than one device created? Both of these?

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

I'll take a closer look tomorrow.

-Saravana

>
> >
> > Btw, there is the deferred_probe_timeout command line option that
> > can be used so that fw_devlink stops enforcing dependencies where
> > there are no supplier drivers for a device after a timeout. It's not ideal,
> > but it's something to unblock you.
> >
> > The best fw_devlink could do is just not enforce any dependencies if
> > there is more than one device instantiated for a given supplier DT node.
>
> You mean not enforce fw_devlink for all or this could only apply to
> specific nodes or devices?
>
> Thanks,
> Peng.
>
> >
> > -Saravana

Cristian Marussi Aug. 14, 2024, 11:27 a.m. UTC | #13

On Tue, Aug 13, 2024 at 11:51:24PM -0700, Saravana Kannan wrote:
> On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]> wrote:
> >
> > > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > > for scmi cpufreq
> > >

Hi,

> > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> > > for
> > > > scmi cpufreq
> > > >
> > > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > fwnode
> > > > > > for scmi cpufreq
> > > > > >
> > > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > > > fwnode
> > > > > > > > for scmi cpufreq
> > > > > > > >
> > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > > >
> > > > > > > > > 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
> > > > > > > >
> > > > > > > > The commit message itself seems very specific to some
> > > platform
> > > > > > > > to
> > > > > > me.
> > > > > > > > What about platforms that don't atall have a GPU? Why
> > > would
> > > > > > they
> > > > > > > > care about this?
> > > > > > >
> > > > > > > It is a generic issue if a platform has performance domain to
> > > > > > > serve scmi cpufreq and device performance level.
> > > > > > >
> > > > > > > >
> > > > > > > > > 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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > V2:
> > > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This might be
> > > > > > > > > a workaround, but since this is a fix, it is simple for
> > > > > > > > > backporting.
> > > > > > > >
> > > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > V1:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > > strcmp(name,
> > > > > > > > "cpufreq"))
> > > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > > of_fwnode_handle(np));
> > > > > > > >
> > > > > > > > I kind of disagree with the idea here to be specific about the
> > > > > > > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus
> > > > driver
> > > > > > right?
> > > > > > > > Why bring in specific code into a bus driver? We will never
> > > > > > > > fix the actual root cause of the issue this way.
> > > > > > >
> > > > > > > The root cause is fwnode devlink only supports one fwnode, one
> > > > > > device.
> > > > > > > 1:1 match. But current arm scmi driver use one fwnode for two
> > > > > > devices.
> > > > > > >
> > > > > > > If your platform has scmi cpufreq and scmi device performance
> > > > > > domain,
> > > > > > > you might see that some devices are consumer of scmi cpufreq,
> > > > but
> > > > > > > actually they should be consumer of scmi device performance
> > > > > > domain.
> > > > > > >
> > > > > > > I not have a good idea that this is fw devlink design that only
> > > > > > > allows
> > > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi should be
> > > fixed.
> > > > > > > If not, fw devlink should be updated.
> > > > > > >
> > > > > > > The current patch is the simplest method for stable tree fixes
> > > > > > > as I could work out.
> > > > > >
> > > > > > So this is the same root cause at the end of the issues you had
> > > > > > with IMX pinctrl coexistence...i.e. the SCMI stack creates scmi
> > > > > > devices that embeds the protocol node, BUT since you can have
> > > > > > multiple device/drivers doing different things on different
> > > > > > resources within the same protocol you can end with 2 devices
> > > > > > having the same embedded device_node, since we dont really
> > > have
> > > > > > anything else to
> > > > use
> > > > > > as device_node, I rigth ?
> > > > >
> > > > > I think, yes. And you remind me that with PINCTRL_SCMI and
> > > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
> > > will
> > > > only
> > > > > take one to set the fwnode device pointer depends on the order to
> > > > run
> > > > > __scmi_device_create.
> > > > >
> > > > > So not only perf, pinctrl also has issue here, fwnode devlink will
> > > > > not work properly for pinctrl/perf.
> > > >
> > > > ...mmm ... the standard generic Pinctrl driver and the IMX Pintrcl are
> > > > mutually exclusive in the code (@probe time) itself as far as I can
> > > > remember about what you did, so why devlink should have that issue
> > > > there ?
> > > > Have you seen any issue in this regards while having loaded
> > > > pinctrl_scmi and pinctrl_imx_scmi ?
> > >
> > > No. it works well in my setup. I am just worried that there might be
> > > issues because fwnode only has one dev pointer, see device_add.
> > >
> > > >
> > > > I want to have a better look next days about this devlink issue that
> > > > you reported...it still not clear to me...from device_link_add() docs,
> > > > it seems indeed that it will return the old existing link if a link
> > > > exist already between that same supplier/consumer devices
> > > pair....but
> > > > from the code (at first sight) it seems that the check is made agains
> > > > the devices not their embeded device_nodes, but here (and in
> > > > pinctrl/imx case) you will have 2 distinct consumer devices (with
> > > same
> > > > device_node)...I may have missed something in your exaplanation....
> > >
> > > It might be false alarm for pinctrl, but it is true issue for perf.
> >
> > Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI
> > and PINCTRL_IMX_SCMI both enabled, two scmi devices
> > will be created. So it depends on device creation order, 1st
> > created device will be used as supplier.
> >
> > On i.MX, there is no issue with both enabled, it is because
> > the imx scmi device is created first. If the generic pinctrl scmi
> > device created first, imx will have issue.
> >
> > In the end, this is generic fw_devlink limitation or arm scmi driver
> > issue.
> 
> +1 to what Cristian and Dhruva said in other parts of this thread.
> 
> This patch itself is definitely a hack.
> 
> The problem isn't so much that fw_devlink doesn't want to support
> multiple devices getting instantiated from one DT node. The problem is
> that there's no way to know which of the multiple devices is the real
> supplier just by looking at the information in devicetree/firmware
> (the fw in fw_devlink). And keep in mind that one of the main
> requirements of fw_devlink is to work before any driver is loaded and
> not depend on drivers for correctness of the dependency information
> because it needs to work on a fully modular kernel too. So, fw_devlink
> just picks the first device that's instantiated from a DT node.
> 
> I really hate folks creating multiple devices from one DT node. One IP
> block can support multiple things, there's no need to instantiate
> multiple devices for it. The same driver could have just as easily
> registered with multiple frameworks. So, ideally I'd want us to fix
> this issue in the SCMI framework code. In the case where the same SCMI
> node is creating two devices, can they both probe successfully? If
> yes, why are we not using a child node or a separate node for this
> second device? If it's always one or the other, why are we creating
> two devices? Can you please point to specific upstream DT examples for
> me to get a better handle on what's going on?
> 
> Btw, there is the deferred_probe_timeout command line option that can
> be used so that fw_devlink stops enforcing dependencies where there
> are no supplier drivers for a device after a timeout. It's not ideal,
> but it's something to unblock you.
> 
> The best fw_devlink could do is just not enforce any dependencies if
> there is more than one device instantiated for a given supplier DT
> node.
> 

So I'll try to describe in the following how the SCMI stack is
designed and behaves as it stands now....probably not exactly in a
concise way ... :P

The protocol-nodes in the DT are used to describe the presence and
sometime additional properties of an SCMI Protocol like Clock,
Peformance, Pinctrl etc, beside being used in phandles to refer to
specific per-protocol resources from other drivers.
(arch/arm64/boot/dts/arm/juno-scmi.dtsi to see how protocol nodes
 are referred )

Note also that the SCNI protocol is extensible and growing, we have 10
stanard protocols currently and potentially a number of additional vendor
protocols numbers that can be defined in the future.

The core SCMI stack knows how to build,send and track the proper SCMI
messages for all of these SCMI protocols and their well defined SCMI
commands, and exposes a common API (include/linux/scmi_protocol.h) in the
classic "file-operations" style for the driver-users that wants to use
such SCMI facilities.

As an example, clk-scmi is one of this SCMI drivers: it plugs on one side
on the standard CLK frameowrk and then registers also as an SCMI driver
with the SCMI stack, so that it can access and use the related specific
scmi_clk_ops to map common operations like clocks enable/disable into an
SCMI message exchange.

When the SCMI core stack comes up it queries the SCMI/fw upfront via the
dedicated Base protocol to discover, amongst other stuff, the list of
supported protocols by the current platorm.

An SCMI driver like clk-scmi needs some sort of device to drive: the
driver itself declares the protocol(CLK) that he wants to use and a name,
and the SCMI bus will create a device accordingly, using the related
protocol device_node and the name choosen (so that multiple devices
can exist with different names for different drivers but using the same
protocol)

Then, the clk-scmi driver at probe time, after a successufl match on
protocol/name, will obtain the related clk_scmi ops from the core
stack together with an opaque handle for that instance of the SCMI
stack: this first SCMI/clk user will trigger the specific core protocol
initialization code to be run.

The clk-scmi driver can then issue any CLK related ops he wants
during its lifetime.

Protocols are initialized on first use: when a SCMI driver appears that
wants to use protocol-X ops, protocol-X will be initialized...any
subsequent driver appearing that wants to use protocol-X will use the
same initialized protocol stack.

...BUT you can have possibly multiple drivers willing to use the same
protocol, probably on a disjoint set of resources in that protocol:
an example of this is the Sensor protocol which is used by scmi-hwmon
for a few classic sensors, BUT also by scmi-iio for another class of
distinct sensors (and plugging into a different subsystem): such drivers
will both use Sensor protocol and both will have a device created an
"hwmon"/sensor vs "iio"/sensor...

...the problem is, as it start to become apparent, I have ONLY one single
DT node to represent the protocol which I can use to embed in the
created devices...

In this scenario I would like to have multiple distinct DT nodes describing
the same protocol so that I can instantiate different devices using the same
protocol but using a distinct device_node...just I am not sure how to create
such a duplicate node out of thin-air in a non-dirty way...moreover I am not
even sure this is doable, since I fear that such a solution will even more
break the stack, since how the phandle references across the DT are even
supposed to work if I magically create a duplicate of a device_node
urelated to any real DT...

...sorry for my ignorance in this area, any advice will be very much welcome.

Thanks,
Cristian

Peng Fan Aug. 14, 2024, 12:08 p.m. UTC | #14

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Wed, Aug 14, 2024 at 12:04 AM Peng Fan <[e-mail]>
> wrote:
> >
> > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for scmi cpufreq
> > >
> > > On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]>
> > > wrote:
> > > >
> > > > > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > fwnode
> > > > > for scmi cpufreq
> > > > >
> > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > fwnode
> > > > > for
> > > > > > scmi cpufreq
> > > > > >
> > > > > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan wrote:
> > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass
> > > > > > > > set
> > > > > > fwnode
> > > > > > > > for scmi cpufreq
> > > > > > > >
> > > > > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan
> wrote:
> > > > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus:
> > > > > > > > > > bypass set
> > > > > > > > fwnode
> > > > > > > > > > for scmi cpufreq
> > > > > > > > > >
> > > > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS)
> wrote:
> > > > > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > > > > >
> > > > > > > > > > > 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
> > > > > > > > > >
> > > > > > > > > > The commit message itself seems very specific to some
> > > > > platform
> > > > > > > > > > to
> > > > > > > > me.
> > > > > > > > > > What about platforms that don't atall have a GPU?
> Why
> > > > > would
> > > > > > > > they
> > > > > > > > > > care about this?
> > > > > > > > >
> > > > > > > > > It is a generic issue if a platform has performance
> > > > > > > > > domain to serve scmi cpufreq and device performance
> level.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > 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 <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > V2:
> > > > > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag  This
> > > > > > > > > > > might be a workaround, but since this is a fix, it
> > > > > > > > > > > is simple for backporting.
> > > > > > > > > >
> > > > > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > V1:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > > > > 100644
> > > > > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > @@ -395,7 +395,8 @@ __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));
> > > > > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > > > > strcmp(name,
> > > > > > > > > > "cpufreq"))
> > > > > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > > > > of_fwnode_handle(np));
> > > > > > > > > >
> > > > > > > > > > I kind of disagree with the idea here to be specific
> > > > > > > > > > about the PROTOCOL_PERF or cpufreq. This is a generic
> > > > > > > > > > arm scmi bus
> > > > > > driver
> > > > > > > > right?
> > > > > > > > > > Why bring in specific code into a bus driver? We will
> > > > > > > > > > never fix the actual root cause of the issue this way.
> > > > > > > > >
> > > > > > > > > The root cause is fwnode devlink only supports one
> > > > > > > > > fwnode, one
> > > > > > > > device.
> > > > > > > > > 1:1 match. But current arm scmi driver use one fwnode
> > > > > > > > > for two
> > > > > > > > devices.
> > > > > > > > >
> > > > > > > > > If your platform has scmi cpufreq and scmi device
> > > > > > > > > performance
> > > > > > > > domain,
> > > > > > > > > you might see that some devices are consumer of scmi
> > > > > > > > > cpufreq,
> > > > > > but
> > > > > > > > > actually they should be consumer of scmi device
> > > performance
> > > > > > > > domain.
> > > > > > > > >
> > > > > > > > > I not have a good idea that this is fw devlink design
> > > > > > > > > that only allows
> > > > > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi
> > > > > > > > > should be
> > > > > fixed.
> > > > > > > > > If not, fw devlink should be updated.
> > > > > > > > >
> > > > > > > > > The current patch is the simplest method for stable tree
> > > > > > > > > fixes as I could work out.
> > > > > > > >
> > > > > > > > So this is the same root cause at the end of the issues
> > > > > > > > you had with IMX pinctrl coexistence...i.e. the SCMI stack
> > > > > > > > creates scmi devices that embeds the protocol node, BUT
> > > > > > > > since you
> > > can
> > > > > > > > have multiple device/drivers doing different things on
> > > > > > > > different resources within the same protocol you can end
> > > > > > > > with
> > > > > > > > 2 devices having the same embedded device_node, since
> we
> > > dont
> > > > > > > > really
> > > > > have
> > > > > > > > anything else to
> > > > > > use
> > > > > > > > as device_node, I rigth ?
> > > > > > >
> > > > > > > I think, yes. And you remind me that with PINCTRL_SCMI
> and
> > > > > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl
> > > node
> > > > > will
> > > > > > only
> > > > > > > take one to set the fwnode device pointer depends on the
> > > > > > > order to
> > > > > > run
> > > > > > > __scmi_device_create.
> > > > > > >
> > > > > > > So not only perf, pinctrl also has issue here, fwnode
> > > > > > > devlink will not work properly for pinctrl/perf.
> > > > > >
> > > > > > ...mmm ... the standard generic Pinctrl driver and the IMX
> > > > > > Pintrcl are mutually exclusive in the code (@probe time)
> > > > > > itself as far as I can remember about what you did, so why
> > > > > > devlink should have
> > > that
> > > > > > issue there ?
> > > > > > Have you seen any issue in this regards while having loaded
> > > > > > pinctrl_scmi and pinctrl_imx_scmi ?
> > > > >
> > > > > No. it works well in my setup. I am just worried that there
> > > > > might be issues because fwnode only has one dev pointer, see
> device_add.
> > > > >
> > > > > >
> > > > > > I want to have a better look next days about this devlink
> > > > > > issue that you reported...it still not clear to me...from
> > > > > > device_link_add() docs, it seems indeed that it will return
> > > > > > the old existing link if a link exist already between that
> > > > > > same supplier/consumer devices
> > > > > pair....but
> > > > > > from the code (at first sight) it seems that the check is made
> > > > > > agains the devices not their embeded device_nodes, but here
> > > (and
> > > > > > in pinctrl/imx case) you will have 2 distinct consumer devices
> > > > > > (with
> > > > > same
> > > > > > device_node)...I may have missed something in your
> > > exaplanation....
> > > > >
> > > > > It might be false alarm for pinctrl, but it is true issue for perf.
> > > >
> > > > Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI
> and
> > > > PINCTRL_IMX_SCMI both enabled, two scmi devices will be
> created.
> > > So it
> > > > depends on device creation order, 1st created device will be used
> > > > as supplier.
> > > >
> > > > On i.MX, there is no issue with both enabled, it is because the
> > > > imx scmi device is created first. If the generic pinctrl scmi
> > > > device created first, imx will have issue.
> > > >
> > > > In the end, this is generic fw_devlink limitation or arm scmi
> > > > driver issue.
> > >
> > > +1 to what Cristian and Dhruva said in other parts of this thread.
> > >
> > > This patch itself is definitely a hack.
> > >
> > > The problem isn't so much that fw_devlink doesn't want to support
> > > multiple devices getting instantiated from one DT node. The
> problem
> > > is that there's no way to know which of the multiple devices is the
> > > real supplier just by looking at the information in
> > > devicetree/firmware (the fw in fw_devlink).
> >
> > Yes. I see.
> >
> > And keep in mind that one of the main requirements
> > > of fw_devlink is to work before any driver is loaded and not depend
> > > on drivers for correctness of the dependency information because it
> > > needs to work on a fully modular kernel too. So, fw_devlink just
> > > picks the first device that's instantiated from a DT node.
> > >
> > > I really hate folks creating multiple devices from one DT node. One
> > > IP block can support multiple things, there's no need to instantiate
> > > multiple devices for it. The same driver could have just as easily
> > > registered with multiple frameworks. So, ideally I'd want us to fix
> > > this issue in the SCMI framework code. In the case where the same
> > > SCMI node is creating two devices, can they both probe successfully?
> >
> > In pinctrl case, only one could probe successfully.
> > See
> > drivers/pinctrl/pinctrl-scmi.c for generic pinctrl scmi driver
> > drivers/pinctrl/freescale/pinctrl-imx-scmi.c for i.MX extension.
> >
> > There is a machine compatible string to make sure only one could
> probe
> > successfully.
> 
> Why?! Isn't this the whole point of compatible strings and being able
> to list multiple compatible strings for a DT node?

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

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

> 
> >
> > In scmi performance case, both could probe successfully.
> > drivers/cpufreq/scmi-cpufreq.c
> > drivers/pmdomain/arm/scmi_perf_domain.c
> >
> >  If yes,
> > > why are we not using a child node or a separate node for this
> second
> > > device? If it's always one or the other, why are we creating two
> devices?
> >
> > Sudeep and Cristian may comment on the design goal of scmi drivers.
> >
> > > Can you please point to specific upstream DT examples for me to
> get
> > > a better handle on what's going on?
> >
> > See linux-next tree:
> > arch/arm64/boot/dts/freescale/imx95.dtsi
> >
> >                         scmi_perf: protocol@13 {
> >                                 reg = <0x13>;
> >                                 #power-domain-cells = <1>;
> >                         };
> >
> >                         scmi_iomuxc: protocol@19 {
> >                                 reg = <0x19>;
> >                         };
> 
> Sure, I can see that file and these nodes, but what am I supposed to
> make of it? Can you be more specific please?

Sorry for not being clear.

 I'm not familiar with the
> protocol number mapping.

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

 Which nodes are the consumers in this
> example?

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

 Which node has more than one device created? Both of
> these?

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

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

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

> 
> I'll take a closer look tomorrow.

Thanks,
Peng.

> 
> -Saravana
> 
> >
> > >
> > > Btw, there is the deferred_probe_timeout command line option
> that
> > > can be used so that fw_devlink stops enforcing dependencies where
> > > there are no supplier drivers for a device after a timeout. It's not
> > > ideal, but it's something to unblock you.
> > >
> > > The best fw_devlink could do is just not enforce any dependencies
> if
> > > there is more than one device instantiated for a given supplier DT
> node.
> >
> > You mean not enforce fw_devlink for all or this could only apply to
> > specific nodes or devices?
> >
> > Thanks,
> > Peng.
> >
> > >
> > > -Saravana

Peng Fan Sept. 2, 2024, 12:22 p.m. UTC | #15

Hi Saravana,

> Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for
> > scmi cpufreq
> >
> > On Wed, Aug 14, 2024 at 12:04 AM Peng Fan <[e-mail]>
> > wrote:
> > >
> > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > fwnode
> > > > for scmi cpufreq
> > > >
> > > > On Tue, Aug 13, 2024 at 8:36 PM Peng Fan <[e-mail]>
> > > > wrote:
> > > > >
> > > > > > Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > fwnode
> > > > > > for scmi cpufreq
> > > > > >
> > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> > > > fwnode
> > > > > > for
> > > > > > > scmi cpufreq
> > > > > > >
> > > > > > > On Tue, Aug 13, 2024 at 01:52:30PM +0000, Peng Fan
> wrote:
> > > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass
> > > > > > > > > set
> > > > > > > fwnode
> > > > > > > > > for scmi cpufreq
> > > > > > > > >
> > > > > > > > > On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan
> > wrote:
> > > > > > > > > > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus:
> > > > > > > > > > > bypass set
> > > > > > > > > fwnode
> > > > > > > > > > > for scmi cpufreq
> > > > > > > > > > >
> > > > > > > > > > > On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS)
> > wrote:
> > > > > > > > > > > > From: Peng Fan <[e-mail]>
> > > > > > > > > > > >
> > > > > > > > > > > > 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
> > > > > > > > > > >
> > > > > > > > > > > The commit message itself seems very specific to
> > > > > > > > > > > some
> > > > > > platform
> > > > > > > > > > > to
> > > > > > > > > me.
> > > > > > > > > > > What about platforms that don't atall have a GPU?
> > Why
> > > > > > would
> > > > > > > > > they
> > > > > > > > > > > care about this?
> > > > > > > > > >
> > > > > > > > > > It is a generic issue if a platform has performance
> > > > > > > > > > domain to serve scmi cpufreq and device performance
> > level.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > 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 <[e-mail]>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > V2:
> > > > > > > > > > > >  Use A!=B to replace !(A == B)  Add fixes tag
> > > > > > > > > > > > This might be a workaround, but since this is a
> > > > > > > > > > > > fix, it is simple for backporting.
> > > > > > > > > > >
> > > > > > > > > > > More than a workaround, it feels like a HACK to me.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > V1:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > > b/drivers/firmware/arm_scmi/bus.c index
> > > > > > > > > > > 96b2e5f9a8ef..be91a82e0cda
> > > > > > > > > > > > 100644
> > > > > > > > > > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > > > > > > > > > @@ -395,7 +395,8 @@
> __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));
> > > > > > > > > > > > + if ((protocol != SCMI_PROTOCOL_PERF) ||
> > > > > > > strcmp(name,
> > > > > > > > > > > "cpufreq"))
> > > > > > > > > > > > +         device_set_node(&scmi_dev->dev,
> > > > > > > > > > > of_fwnode_handle(np));
> > > > > > > > > > >
> > > > > > > > > > > I kind of disagree with the idea here to be specific
> > > > > > > > > > > about the PROTOCOL_PERF or cpufreq. This is a
> > > > > > > > > > > generic arm scmi bus
> > > > > > > driver
> > > > > > > > > right?
> > > > > > > > > > > Why bring in specific code into a bus driver? We
> > > > > > > > > > > will never fix the actual root cause of the issue this
> way.
> > > > > > > > > >
> > > > > > > > > > The root cause is fwnode devlink only supports one
> > > > > > > > > > fwnode, one
> > > > > > > > > device.
> > > > > > > > > > 1:1 match. But current arm scmi driver use one fwnode
> > > > > > > > > > for two
> > > > > > > > > devices.
> > > > > > > > > >
> > > > > > > > > > If your platform has scmi cpufreq and scmi device
> > > > > > > > > > performance
> > > > > > > > > domain,
> > > > > > > > > > you might see that some devices are consumer of scmi
> > > > > > > > > > cpufreq,
> > > > > > > but
> > > > > > > > > > actually they should be consumer of scmi device
> > > > performance
> > > > > > > > > domain.
> > > > > > > > > >
> > > > > > > > > > I not have a good idea that this is fw devlink design
> > > > > > > > > > that only allows
> > > > > > > > > > 1 fwnode has 1 device or not. If yes, that arm scmi
> > > > > > > > > > should be
> > > > > > fixed.
> > > > > > > > > > If not, fw devlink should be updated.
> > > > > > > > > >
> > > > > > > > > > The current patch is the simplest method for stable
> > > > > > > > > > tree fixes as I could work out.
> > > > > > > > >
> > > > > > > > > So this is the same root cause at the end of the issues
> > > > > > > > > you had with IMX pinctrl coexistence...i.e. the SCMI
> > > > > > > > > stack creates scmi devices that embeds the protocol
> > > > > > > > > node, BUT since you
> > > > can
> > > > > > > > > have multiple device/drivers doing different things on
> > > > > > > > > different resources within the same protocol you can end
> > > > > > > > > with
> > > > > > > > > 2 devices having the same embedded device_node, since
> > we
> > > > dont
> > > > > > > > > really
> > > > > > have
> > > > > > > > > anything else to
> > > > > > > use
> > > > > > > > > as device_node, I rigth ?
> > > > > > > >
> > > > > > > > I think, yes. And you remind me that with PINCTRL_SCMI
> > and
> > > > > > > > CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi
> pinctrl
> > > > node
> > > > > > will
> > > > > > > only
> > > > > > > > take one to set the fwnode device pointer depends on the
> > > > > > > > order to
> > > > > > > run
> > > > > > > > __scmi_device_create.
> > > > > > > >
> > > > > > > > So not only perf, pinctrl also has issue here, fwnode
> > > > > > > > devlink will not work properly for pinctrl/perf.
> > > > > > >
> > > > > > > ...mmm ... the standard generic Pinctrl driver and the IMX
> > > > > > > Pintrcl are mutually exclusive in the code (@probe time)
> > > > > > > itself as far as I can remember about what you did, so why
> > > > > > > devlink should have
> > > > that
> > > > > > > issue there ?
> > > > > > > Have you seen any issue in this regards while having loaded
> > > > > > > pinctrl_scmi and pinctrl_imx_scmi ?
> > > > > >
> > > > > > No. it works well in my setup. I am just worried that there
> > > > > > might be issues because fwnode only has one dev pointer, see
> > device_add.
> > > > > >
> > > > > > >
> > > > > > > I want to have a better look next days about this devlink
> > > > > > > issue that you reported...it still not clear to me...from
> > > > > > > device_link_add() docs, it seems indeed that it will return
> > > > > > > the old existing link if a link exist already between that
> > > > > > > same supplier/consumer devices
> > > > > > pair....but
> > > > > > > from the code (at first sight) it seems that the check is
> > > > > > > made agains the devices not their embeded device_nodes,
> but
> > > > > > > here
> > > > (and
> > > > > > > in pinctrl/imx case) you will have 2 distinct consumer
> > > > > > > devices (with
> > > > > > same
> > > > > > > device_node)...I may have missed something in your
> > > > exaplanation....
> > > > > >
> > > > > > It might be false alarm for pinctrl, but it is true issue for perf.
> > > > >
> > > > > Just give a recheck. Pinctrl has same issue. With PINCTRL_SCMI
> > and
> > > > > PINCTRL_IMX_SCMI both enabled, two scmi devices will be
> > created.
> > > > So it
> > > > > depends on device creation order, 1st created device will be
> > > > > used as supplier.
> > > > >
> > > > > On i.MX, there is no issue with both enabled, it is because the
> > > > > imx scmi device is created first. If the generic pinctrl scmi
> > > > > device created first, imx will have issue.
> > > > >
> > > > > In the end, this is generic fw_devlink limitation or arm scmi
> > > > > driver issue.
> > > >
> > > > +1 to what Cristian and Dhruva said in other parts of this thread.
> > > >
> > > > This patch itself is definitely a hack.
> > > >
> > > > The problem isn't so much that fw_devlink doesn't want to
> support
> > > > multiple devices getting instantiated from one DT node. The
> > problem
> > > > is that there's no way to know which of the multiple devices is
> > > > the real supplier just by looking at the information in
> > > > devicetree/firmware (the fw in fw_devlink).
> > >
> > > Yes. I see.
> > >
> > > And keep in mind that one of the main requirements
> > > > of fw_devlink is to work before any driver is loaded and not
> > > > depend on drivers for correctness of the dependency information
> > > > because it needs to work on a fully modular kernel too. So,
> > > > fw_devlink just picks the first device that's instantiated from a DT
> node.
> > > >
> > > > I really hate folks creating multiple devices from one DT node.
> > > > One IP block can support multiple things, there's no need to
> > > > instantiate multiple devices for it. The same driver could have
> > > > just as easily registered with multiple frameworks. So, ideally
> > > > I'd want us to fix this issue in the SCMI framework code. In the
> > > > case where the same SCMI node is creating two devices, can they
> both probe successfully?
> > >
> > > In pinctrl case, only one could probe successfully.
> > > See
> > > drivers/pinctrl/pinctrl-scmi.c for generic pinctrl scmi driver
> > > drivers/pinctrl/freescale/pinctrl-imx-scmi.c for i.MX extension.
> > >
> > > There is a machine compatible string to make sure only one could
> > probe
> > > successfully.
> >
> > Why?! Isn't this the whole point of compatible strings and being able
> > to list multiple compatible strings for a DT node?
> 
> The scmi devices are actually not populated from DT node, they are
> create from driver according to a table if the reg(protocol_id) exists in
> DT, see drivers/firmware/arm_scmi/bus.c "
>                 sdev = __scmi_device_create(np, parent,
>                                             rdev->id_table->protocol_id,
>                                             rdev->id_table->name); "
> 
> For compatible strings, I could not comment on the design. Cristian
> seems replied in the other mail.
> 
> >
> > >
> > > In scmi performance case, both could probe successfully.
> > > drivers/cpufreq/scmi-cpufreq.c
> > > drivers/pmdomain/arm/scmi_perf_domain.c
> > >
> > >  If yes,
> > > > why are we not using a child node or a separate node for this
> > second
> > > > device? If it's always one or the other, why are we creating two
> > devices?
> > >
> > > Sudeep and Cristian may comment on the design goal of scmi
> drivers.
> > >
> > > > Can you please point to specific upstream DT examples for me to
> > get
> > > > a better handle on what's going on?
> > >
> > > See linux-next tree:
> > > arch/arm64/boot/dts/freescale/imx95.dtsi
> > >
> > >                         scmi_perf: protocol@13 {
> > >                                 reg = <0x13>;
> > >                                 #power-domain-cells = <1>;
> > >                         };
> > >
> > >                         scmi_iomuxc: protocol@19 {
> > >                                 reg = <0x19>;
> > >                         };
> >
> > Sure, I can see that file and these nodes, but what am I supposed to
> > make of it? Can you be more specific please?
> 
> Sorry for not being clear.
> 
>  I'm not familiar with the
> > protocol number mapping.
> 
> The ID is in include/linux/scmi_protocol.h, line 918.
> 
>  Which nodes are the consumers in this
> > example?
> 
> For " protocol@13", it is A55 cpu core with " power-domains =
> <&scmi_perf IMX95_PERF_A55>;" and other devices(GPU/VPU) that
> not in upstream tree as of now.
> 
>  Which node has more than one device created? Both of
> > these?
> 
> Yes, both.
> drivers/firmware/arm_scmi/bus.c
> __scmi_device_create will create device according to "static const
> struct scmi_device_id scmi_id_table", so for protocol@19, It will create
> two devices, one is "{ SCMI_PROTOCOL_PINCTRL, "pinctrl" }", The other
> is " { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" }, ", both points to the
> node protocol@19, see "device_set_node(&scmi_dev->dev,
> of_fwnode_handle(np)); " in "__scmi_device_create".
> 
> >
> > Can you also point me to specific examples for the pinctrl thing you
> > mention above?
> 
> As above, two devices are created, both points to node protocol@19,
> but fwnode protocol@19 will only have 1st device as supplier.
> 
> >
> > I'll take a closer look tomorrow.

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

Thanks,
Peng.

> 
> Thanks,
> Peng.
> 
> >
> > -Saravana
> >
> > >
> > > >
> > > > Btw, there is the deferred_probe_timeout command line option
> > that
> > > > can be used so that fw_devlink stops enforcing dependencies
> where
> > > > there are no supplier drivers for a device after a timeout. It's not
> > > > ideal, but it's something to unblock you.
> > > >
> > > > The best fw_devlink could do is just not enforce any dependencies
> > if
> > > > there is more than one device instantiated for a given supplier DT
> > node.
> > >
> > > You mean not enforce fw_devlink for all or this could only apply to
> > > specific nodes or devices?
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > -Saravana

Patch

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 96b2e5f9a8ef..be91a82e0cda 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -395,7 +395,8 @@  __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));
+	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name, "cpufreq"))
+		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);