[PATCH] firmware: ti_sci: Scan all device instances when releasing exclusive devices

[Nishanth Menon]

On 11:57-20250402, Dhruva Gole wrote:
[...]
> > 
> > As a result of this, the cleanup we intent to do with
> 
> s/intent/intend

Thanks. will pick it up in a respin.

> 
[...]

> > To fix this, let us make ti_sci_cmd_release_exclusive_devices scan the
> > all registrations of tisci instances and cleanup all exclusive devices
> > that have ever been registered.
> 
> Firstly, it seems to me like this issue is mostly specific to the
> AM64/ DM+TIFS combined core arch K3 devices? So it would be good if the
> commit message can specify that for better context. People who are more
> used to the R5 core running DM architecture may find this description
> slightly confusing as there's no TI SCI calls that the R5 SPL can make
> to DM since DM isn't up yet on those devices...

This is not related to TI_SCI. As described in commit message, it is just
the multi_fit_dtb handling challenges. Combined DMSC or split DM arch
devices are probably both susceptible to this. That said, i have tested
this on am64x.

[...]
> > index 190a1e3f5fc3..54d6689ce783 100644
> > --- a/drivers/firmware/ti_sci.c
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -696,20 +696,25 @@ static int ti_sci_cmd_put_device(const struct ti_sci_handle *handle, u32 id)
> >  				       MSG_DEVICE_SW_STATE_AUTO_OFF);
> >  }
> >  
> > -static
> > -int ti_sci_cmd_release_exclusive_devices(const struct ti_sci_handle *handle)
> > +static int ti_sci_cmd_release_exclusive_devices(void)
> >  {
> >  	struct ti_sci_exclusive_dev *dev, *tmp;
> >  	struct ti_sci_info *info;
> >  	int i, cnt;
> >  
> > -	info = handle_to_ti_sci_info(handle);
> > -
> > -	list_for_each_entry_safe(dev, tmp, &info->dev_list, list) {
> > -		cnt = dev->count;
> > -		debug("%s: id = %d, cnt = %d\n", __func__, dev->id, cnt);
> > -		for (i = 0; i < cnt; i++)
> > -			ti_sci_cmd_put_device(handle, dev->id);
> > +	/*
> > +	 * Scan all ti_sci_list registrations, since with FIT images, we could
> > +	 * have started with one device tree registration and switched over
> > +	 * to a final version. This prevents exclusive devices identified
> > +	 * during the first probe to be left orphan.
> > +	 */
> > +	list_for_each_entry(info, &ti_sci_list, list) {
> > +		list_for_each_entry_safe(dev, tmp, &info->dev_list, list) {
> > +			cnt = dev->count;
> > +			debug("%s: id = %d, cnt = %d\n", __func__, dev->id, cnt);
> > +			for (i = 0; i < cnt; i++)
> > +				ti_sci_cmd_put_device(&info->handle, dev->id);
> > +		}
> 
> The other thing I am slightly not comfortable with is this.
> The release_exclusive call is going to be common to every K3 device.
> With your changes, will it result in any sort of increased boot time
> penalty? I feel like it might, unless I maybe mistaken.

It should not. for non-multi-dtb FIT, this logic just flows only 1 list
which is all that is present. for multi-dtb FIT images, it fixes a real
problem.

> 
> Also, to fix just AM64 style devices, we're touching a K3 common piece
> of code like TI SCI. The issue that you described about the TI SCI
> probing the second time causing issues seems more fundamental than the
> fix being offered here.

As described in commit message, calling probe twice is Normal since
there are two dtbs involved in the FIT image (one with which we
identify what is the board and use the information to pick the correct
dtb).

> Quoting the commit message,
> 
> > At this stage, the exclusive devices such as i2c instances used to
> > probe the board information is left in the old info->dev_list that is
> > no longer used actively by the system using the replaced dtb.
> 
> Can we instead try and fix the TI SCI driver behaviour from the first time?
> Like is it possible to implement a .remove for the TI SCI driver such
> that when the old dtb is now out of context, the driver gets cleanly
> removed, and with that calls the release_exclusive of all devices that
> it requested? 

Tried .remove and .unbind and enabling CONFIG_SPL_DM_DEVICE_REMOVE but
then we run into all kind of other dependency issues on dm_uninit

> That way the new instance of TI SCI driver doesn't need to worry about
> doing cleanups from any of it's previous instances right?
> 
> To me that sounds like the right root cause to try and fix. Let me know
> what are your thoughts on this.

I am open to any approach :) - just dont seem to find one that works
other than what I posted.

> 
> >  	}
> >  
> >  	return 0;
> > diff --git a/include/linux/soc/ti/ti_sci_protocol.h b/include/linux/soc/ti/ti_sci_protocol.h
> > index 8e4c43cef311..aa4d105ee988 100644
> > --- a/include/linux/soc/ti/ti_sci_protocol.h
> > +++ b/include/linux/soc/ti/ti_sci_protocol.h
> > @@ -143,7 +143,7 @@ struct ti_sci_dev_ops {
> >  				 u32 reset_state);
> >  	int (*get_device_resets)(const struct ti_sci_handle *handle, u32 id,
> >  				 u32 *reset_state);
> > -	int (*release_exclusive_devices)(const struct ti_sci_handle *handle);
> > +	int (*release_exclusive_devices)(void);
> 
> Nit: Just update the copyright dates of the files you're touching...

not for minor changes such as these.. not worth the diff.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D