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

[Manorit Chawdhry]

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? 

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