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

[Peng Fan <peng.fan@xxxxxxxxxxx>]

On Thu, Jan 02, 2025 at 05:06:57PM +0000, Cristian Marussi wrote:
>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.

ok, I will add "scmi_dev->dev.of_node = np" for cpufreq device.

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

fw_devlink is based on device tree node, so there is no way, unless
add subnodes for a protocol node, but this is not welcomed.

Thanks,
Peng

>
>Thanks,
>Cristian
>