[PATCH v2 4/7] drivers: firmware: ti_sci: Get SCI revision only if TIFS/SYSFW is up

[Neha Malcom Francis]

Hi Nishanth

On 07/09/23 20:35, Nishanth Menon wrote:
> On 19:44-20230907, Neha Malcom Francis wrote:
>> While setting up necessary dependent clocks and power domains, we end up
>> probing the ti_sci driver before TIFS/SYSFW has been loaded in the case
>> of legacy boot flow devices. This leads to panic when getting the SCI
>> revision. Return the revision only if it is combined boot flow. Also
> 
> Not clear description of the problem.

Hmm okay I will modify this and the earlier commit message to give a clearer 
picture.

> 
>> ensure that after TIFS/SYSFW comes up we run ti_sci_cmd_get_revision
>> again so we print the revision right.
>>
>> Signed-off-by: Neha Malcom Francis <n-francis at ti.com>
>> ---
>>   drivers/firmware/ti_sci.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
>> index 72f572d824..6a85d202f2 100644
>> --- a/drivers/firmware/ti_sci.c
>> +++ b/drivers/firmware/ti_sci.c
>> @@ -2703,6 +2703,9 @@ struct ti_sci_handle *ti_sci_get_handle_from_sysfw(struct udevice *sci_dev)
>>   	if (!handle)
>>   		return ERR_PTR(-EINVAL);
>>   
>> +	/* May have been skipped in case TIFS/SYSFW had come up later */
> Very confusing comment.
> 

Understood, will change that.

>> +	ti_sci_cmd_get_revision(handle);
> 
> should this be protected with if (IS_ENABLED(CONFIG_K3_DM_FW) ?
> 
> Since getting the handle is where the entire flow happens, why not do it
> here always?
> 

This is a valid point, I am in the process of making sure there's no other 
consequences to getting the revision this late for platforms with other boot 
devices, if this gets an all clear, I will push it here for the next version, 
else I will explain why I couldn't.

> what if ti_sci_cmd_get_revision fails? there is no error handling here.

Yes that would be a consequence since ti_sci_probe, the logic it was written; 
used reading the version as an "all okay return 0" for the TIFS/SYSFW binary.

But now after moving, ti_sci_get_handle_from_sysfw happens after k3_sysfw_loader 
so we're not checking binary sanity early on in the case of combined boot

I'm hesitant that we are checking version way later after k3_sysfw_loader for 
combined platforms and if that is something we want? But there is an 
advantage(?) that it will run every time someone tries to get a handle for doing 
anything with TISCI and that would be a kind of check (?) if the binary is good? 
Correct me if I'm wrong in thinking that.

>> +
>>   	return handle;
>>   }
>>   
>> @@ -2825,7 +2828,11 @@ static int ti_sci_probe(struct udevice *dev)
>>   	list_add_tail(&info->list, &ti_sci_list);
>>   	ti_sci_setup_ops(info);
>>   
>> -	ret = ti_sci_cmd_get_revision(&info->handle);
>> +	/* Get SCI revision ONLY if we are real */
>> +	if (!IS_ENABLED(CONFIG_K3_DM_FW))
>> +		ret = ti_sci_cmd_get_revision(&info->handle);
>> +	else
>> +		ret = 0;
>>   
>>   	INIT_LIST_HEAD(&info->dev_list);
>>   
>> -- 
>> 2.34.1
>>
> 

-- 
Thanking You
Neha Malcom Francis