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

[Peng Fan <peng.fan@xxxxxxx>]

> 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.

Thanks,
Peng 
> 
> Thanks,
> Cristian