Message-ID:
 <PAXPR04MB8459AF9F24D0449A26B2646C88872@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Wed, 14 Aug 2024 03:35:56 +0000
From: Peng Fan <peng.fan@....com>
To: Peng Fan <peng.fan@....com>, Cristian Marussi <cristian.marussi@....com>
CC: Dhruva Gole <d-gole@...com>, "Peng Fan (OSS)" <peng.fan@....nxp.com>,
	"saravanak@...gle.com" <saravanak@...gle.com>, "sudeep.holla@....com"
	<sudeep.holla@....com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "arm-scmi@...r.kernel.org"
	<arm-scmi@...r.kernel.org>
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
> 
> > 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 <peng.fan@....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@....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 ?
> > >
> > > 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

Powered by blists - more mailing lists