Message-ID:
 <PAXPR04MB8459D35BF6FD55B38350E67388862@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Tue, 13 Aug 2024 15:17:01 +0000
From: Peng Fan <peng.fan@....com>
To: 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
> 
> 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.

Regards,
Peng.

> 
> Thanks,
> Cristian

Powered by blists - more mailing lists