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 <[email protected]>
---
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
