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

[Cristian Marussi <cristian.marussi@xxxxxxx>]

On Thu, Jan 02, 2025 at 07:38:06AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
> > fwnode for scmi cpufreq
> > 
> > On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote:
> > > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > >
> > > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use
> > > > SCMI_PROTCOL_PERF protocol, but with different name, so two
> > scmi
> > > > devices will be created. But the fwnode->dev could only point to
> > one device.
> > > >
> > > > If scmi cpufreq device created earlier, the fwnode->dev will point
> > > > to the scmi cpufreq device. Then the fw_devlink will link
> > > > performance domain user device(consumer) to the scmi cpufreq
> > device(supplier).
> > > > But actually the performance domain user device, such as GPU,
> > should
> > > > use the scmi perf device as supplier. Also if 'cpufreq.off=1' in
> > > > bootargs, the GPU driver will defer probe always, because of the
> > > > scmi cpufreq device not ready.
> > > >
> > > > Because for cpufreq, no need use fw_devlink. So bypass setting
> > > > fwnode for scmi cpufreq device.
> > > >
> > 
> > Hi,
> > 
> > > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the
> > > > scmi_device")
> > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > ---
> > > >  drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++-
> > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > b/drivers/firmware/arm_scmi/bus.c index
> > > >
> > 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb654845
> > 43044b442
> > > > 4fbe3b67245466 100644
> > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > @@ -345,6 +345,19 @@ static void __scmi_device_destroy(struct
> > scmi_device *scmi_dev)
> > > >  	device_unregister(&scmi_dev->dev);
> > > >  }
> > > >
> > > > +static int
> > > > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct
> > device_node *np,
> > > > +		       int protocol, const char *name) {
> > > > +	/* cpufreq device does not need to be supplier from devlink
> > perspective */
> > > > +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
> > "cpufreq"))
> > > > +		return 0;
> > > >
> > >
> > > This is just a assumption based on current implementation. What
> > > happens if this is needed. Infact, it is used in the current
> > > implementation to create a dummy clock provider, so for sure with
> > this
> > > change that will break IMO.
> > 
> > I agree with Sudeep on this: if you want to exclude some SCMI device
> > from the fw_devlink handling to address the issues with multiple SCMI
> > devices created on the same protocol nodes, cant we just flag this
> > requirement here and avoid to call device_link_add in
> > driver:scmi_set_handle(), instead of killing completely any possibility of
> > referencing phandles (and having device_link_add failing as a
> > consequence of having a NULL supplier)
> > 
> > i.e. something like:
> > 
> > @bus.c
> > ------
> > static int
> > __scmi_device_set_node(struct scmi_device *scmi_dev, struct
> > device_node *np,
> > 		       int protocol, const char *name) {
> > 	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
> > "cpufreq"))
> > 		scmi_dev->avoid_devlink = true;
> > 
> > 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> > 	....
> > 
> > 
> > and @driver.c
> > -------------
> > 
> > static void scmi_set_handle(struct scmi_device *scmi_dev) {
> > 	scmi_dev->handle = scmi_handle_get(&scmi_dev->dev);
> > 	if (scmi_dev->handle && !scmi_dev->avoid_devlink)
> > 		scmi_device_link_add(&scmi_dev->dev, scmi_dev-
> > >handle->dev); }
> > 
> > .... so that you can avoid fw_devlink BUT keep the device_node NON-
> > null for the device.
> > 
> > This would mean also restoring the pre-existing explicit blacklisting in
> > pinctrl-imx to avoid issues when pinctrl subsystem searches by
> > device_node...
> > 
> > ..or I am missing something ?
> 
> link_ret = device_links_check_suppliers(dev); to check fw_devlink
> is before "ret = driver_sysfs_add(dev);" which
> issue bus notify.
> 
> The link is fw_devlink, the devlink is created in 'device_add'
>         if (dev->fwnode && !dev->fwnode->dev) {                                                     
>                 dev->fwnode->dev = dev;                                                             
>                 fw_devlink_link_device(dev);                                                        
>         }
> The check condition is fwnode.
> 
> I think scmi_dev->avoid_devlink not help here.
> 

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

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

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

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

Thanks,
Cristian