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

[Neha Malcom Francis]

Hi Manorit

On 08/09/23 10:47, Manorit Chawdhry wrote:
> Hi Neha,
> 
> On 09:04-20230908, Neha Malcom Francis wrote:
>> 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.
>>
> 
> I still don't understand why we are coming to the TISCI driver to
> contact M4/DM for setting up the power domains and clocks, if this would
> have been the case then even the normal J721E wouldn't have been working
> and it shouldn't have been dependent on the DT Sync to uncover this
> issue. Could you elaborate more w.r.t this patch?
> 

We were using /delete-property/ to remove every instance of k3_clks and k3_pds 
before TIFS/SYSFW came up so it was never probing the TISCI driver.

>  From what I understand is that even if we end up coming here, we
> shouldn't be hitting the panic as the previous patch sets up the
> necessary clock (mcu_timer0) and at the worst - we would just end up
> polling to get a response and if we don't, we would fail and we
> shouldn't be getting a panic. I believe this is how the J721E would've

So earlier we never reached here because of above mentioned reason, now we do 
with the last patch. While probing ti_sci we invoke ti_sci_cmd_get_revision that 
returns -61 (ENODATA) and we reach panic like this:

get_timer --> get_ticks --> dm_timer_init (tries to get timer, fails for above 
reason) --> panic

> been working before the sync but am not sure what is breaking after the
> sync. Maybe this helps in uncovering the real issue that we still have,
> I believe we should still be looking at why the timer is failing and
> causing this probe to fail.
> 
> Regards,
> Manorit
> 
>>>
>>>> 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

-- 
Thanking You
Neha Malcom Francis