Il 07/12/21 13:10, Yong Wu ha scritto:
On Tue, 2021-12-07 at 09:56 +0100, AngeloGioacchino Del Regno wrote:
Il 07/12/21 07:24, Yong Wu ha scritto:
Hi AngeloGioacchino,
Thanks for your review.
On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno
wrote:
Il 03/12/21 07:40, Yong Wu ha scritto:
sleep control means that when the larb go to sleep, we should
wait
a bit
until all the current commands are finished. thus, when the
larb
runtime
suspend, we need enable this function to wait until all the
existed
command are finished. when the larb resume, just disable this
function.
This function only improve the safe of bus. Add a new flag for
this
function. Prepare for mt8186.
Signed-off-by: Anan Sun <anan.sun@xxxxxxxxxxxx>
Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
---
drivers/memory/mtk-smi.c | 39
+++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
[snip]
static int __maybe_unused mtk_smi_larb_suspend(struct device
*dev)
{
struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+ int ret = 0;
+
+ if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
MTK_SMI_FLAG_SLEEP_CTL))
+ ret = mtk_smi_larb_sleep_ctrl(dev, true);
Sorry but what happens if SLP_PROT_RDY is not getting set
properly?
From what I can understand in the commit description that you
wrote,
if we reach
the timeout, then the LARB transactions are not over....
I see that you are indeed returning a failure here, but you are
also
turning off
the clocks regardless of whether we get a failure or a success;
I'm
not sure that
this is right, as this may leave the hardware in an unpredictable
state (since
there were some more LARB transactions that didn't go through),
leading to crashes
at system resume (or when retyring to suspend).
Thanks for this question. In theory you are right. In this case,
the
bus already hang.
We only printed a fail log in this patch. If this fail happens, we
should request the master to check which case cause the larb hang.
If the master has a good reason or limitation, the hang is
expected, I
think we have to add larb reset in this fail case: Reset the larb
when
the larb runtime resume.
Think about the case in which the system gets resumed only partially
due to a
failure during resume of some driver, or due to a RTC or arch timer
resume and
suspend right after... or perhaps during runtime suspend/resume of
some devices.
In that case, we definitely want to avoid any kind of failure point
that would
lead to a system crash, or any kind of user noticeable (or UX
disrupting) "strange
behavior".
I think that we should make sure that the system suspends cleanly,
instead of
patching up any possible leftover issue at resume time: if this is
doable with
a LARB reset in suspend error case, that looks like being a good
option indeed.
As a side note, thinking about UX, losing a little more time during
suspend is
nothing really noticeable for the user... on the other hand, spending
more time
during resume may be something noticeable to the user.
For this reason, I think that guaranteeing that the system resumes as
fast as
possible is very important, which adds up to the need of suspending
cleanly.
Thanks for this comment. I will put it in the suspend when adding the
reset. But I have no plan to add it in this version since I don't see
the need for this right now. Maybe I should add a comment in the code
for this.
What I understand from your reply is that the reset is not trivial work
and needs quite some time to be done properly; in that case: yes, please
add a TODO comment that explains the situation and the discussed solution.
Also, since this SLP_PROT_RDY flag seems to be very nice, just a simple
question: is this a new feature in the SMI IP of MT8186, or is there anything
similar that we may use on other SoCs, like 8183, 8192, 8195, as a follow-up
of this series?
Fortunately, we have never got this issue. We could add this reset
when
necessary. Is this OK for you?
Thanks.
clk_bulk_disable_unprepare(larb->smi.clk_num, larb-
smi.clks);
- return 0;
+ return ret;
}
static const struct dev_pm_ops smi_larb_pm_ops = {
