Gentle ping...

Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
suspend/resume

quoted

Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
suspend/resume

quoted

From: Anson Huang <redacted>
Sent: Friday, April 24, 2020 10:33 AM

quoted

Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
suspend/resume

quoted

From: Anson Huang <redacted>
Sent: Friday, April 24, 2020 7:01 AM

For "mem" mode suspend on i.MX8 SoCs, MU settings could be lost
because its power is off, so save/restore is needed for MU
settings during

suspend/resume.

quoted

However, the restore can ONLY be done when MU settings are
actually lost, for the scenario of settings NOT lost in "freeze"
mode suspend, since there could be still IPC going on multiple
CPUs, restoring the MU settings could overwrite the TIE by
mistake and cause system freeze, so need to make sure ONLY
restore the MU settings when it is

powered off.

quoted

Signed-off-by: Anson Huang <redacted>

As mentioned before, we'd better keep the original author.

quoted

---
 drivers/mailbox/imx-mailbox.c | 35
+++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mailbox/imx-mailbox.c

b/drivers/mailbox/imx-mailbox.c index 97bf0ac..b53cf63 100644

--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c

@@ -67,6 +67,8 @@ struct imx_mu_priv {
 	struct clk		*clk;
 	int			irq;

+	u32 xcr;
+
 	bool			side_b;
 };

@@ -583,12 +585,45 @@ static const struct of_device_id

imx_mu_dt_ids[] = {  };  MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);

+static int imx_mu_suspend_noirq(struct device *dev) {
+	struct imx_mu_priv *priv = dev_get_drvdata(dev);
+
+	priv->xcr = imx_mu_read(priv, priv->dcfg->xCR);
+
+	return 0;
+}
+
+static int imx_mu_resume_noirq(struct device *dev) {
+	struct imx_mu_priv *priv = dev_get_drvdata(dev);
+
+	/*
+	 * ONLY restore MU when context lost, the TIE could
+	 * be set during noirq resume as there is MU data
+	 * communication going on, and restore the saved
+	 * value will overwrite the TIE and cause MU data
+	 * send failed, may lead to system freeze. This issue
+	 * is observed by testing freeze mode suspend.
+	 */
+	if (!imx_mu_read(priv, priv->dcfg->xCR))
+		imx_mu_write(priv, priv->xcr, priv->dcfg->xCR);

This could be separate patch if it aims to fix a specific corner case.

This is NOT corner case, it can be reproduced with our imx_5.4.y
very easily, and this issue cause me many days to debug...Also cause
Clark's effort to help test it a lot for many days...

Is this issue only happen for non-state lost case (eg. Freeze mode)?
If yes, it's a specific case and worth a separate patch to highlight it IMHO.

BTW, it seems most drivers have this issue in current kernel because
they don't know whether the state are really lost, it seems like
kernel still doesn't support this well.

quoted

I do NOT think it makes sense to first send out your patch with bug
for review, And then add another patch to fix it. 1 patch is enough
for this

feature.

quoted

Anyway, if you really want to go with one patch, for this case, we
usually could keep original author and add a small fix note in commit

message.

quoted

(You could see many community guys do like this if you search kernel
commit
log)

Basically we try our best to keep origin author in order to respect
others' work for community work.

I am fine with whoever is the author, my focus is the issue fix and easy rebase.
If maintainer agrees that introduce a patch with bug and add another patch to
fix is OK, then I can rework the patch into 2 patches.

Anson

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel