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

[Cristian Marussi <cristian.marussi@arm.com>]

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 <peng.fan@nxp.com>
> > >
> > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use
> > > SCMI_PROTCOL_PERF protocol, but with different name, so two scmi
> > > devices will be created. But the fwnode->dev could only point to one
> > device.
> > >
> > > If scmi cpufreq device created earlier, the fwnode->dev will point to
> > > the scmi cpufreq device. Then the fw_devlink will link performance
> > > domain user device(consumer) to the scmi cpufreq device(supplier).
> > > But actually the performance domain user device, such as GPU,
> > should
> > > use the scmi perf device as supplier. Also if 'cpufreq.off=1' in
> > > bootargs, the GPU driver will defer probe always, because of the scmi
> > > cpufreq
> > 
> > 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 <peng.fan@nxp.com>
> > > ---
> > >
> > > V2:
> > >  Use A!=B to replace !(A == B)
> > >  Add fixes tag
> > >  This might be a workaround, but since this is a fix, it is simple for
> > > backporting.
> > 
> > More than a workaround, it feels like a HACK to me.
> > 
> > >
> > > 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