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

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

[Anson Huang]

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

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 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);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_mu_suspend_noirq,
+				      imx_mu_resume_noirq)
+};
+
 static struct platform_driver imx_mu_driver = {
 	.probe		= imx_mu_probe,
 	.remove		= imx_mu_remove,
 	.driver = {
 		.name	= "imx_mu",
 		.of_match_table = imx_mu_dt_ids,
+		.pm = &imx_mu_pm_ops,
 	},
 };
 module_platform_driver(imx_mu_driver);
-- 
2.7.4

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

[Aisheng Dong]

> From: Anson Huang <Anson.Huang@nxp.com>
> 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.
> 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.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

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

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

Regards
Aisheng

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops imx_mu_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_mu_suspend_noirq,
> +				      imx_mu_resume_noirq)
> +};
> +
>  static struct platform_driver imx_mu_driver = {
>  	.probe		= imx_mu_probe,
>  	.remove		= imx_mu_remove,
>  	.driver = {
>  		.name	= "imx_mu",
>  		.of_match_table = imx_mu_dt_ids,
> +		.pm = &imx_mu_pm_ops,
>  	},
>  };
>  module_platform_driver(imx_mu_driver);
> --
> 2.7.4

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

[Anson Huang]

> Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> suspend/resume
> 
> > From: Anson Huang <Anson.Huang@nxp.com>
> > 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.
> > 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.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> As mentioned before, we'd better keep the original author.
> 
> > ---
> >  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...

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.

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

[Aisheng Dong]

> From: Anson Huang <anson.huang@nxp.com>
> Sent: Friday, April 24, 2020 10:33 AM
> 
> > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > suspend/resume
> >
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > > 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.
> > > 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.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > As mentioned before, we'd better keep the original author.
> >
> > > ---
> > >  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.

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

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

Regards
Aisheng

> 

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

[Anson Huang]

> Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> suspend/resume
> 
> > From: Anson Huang <anson.huang@nxp.com>
> > Sent: Friday, April 24, 2020 10:33 AM
> >
> > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > suspend/resume
> > >
> > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > > 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.
> > > > 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.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > As mentioned before, we'd better keep the original author.
> > >
> > > > ---
> > > >  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.
> 
> > 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.
> >
> 
> 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.
> (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

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

[Anson Huang]

Gentle ping...


> Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> suspend/resume
> 
> 
> 
> > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > suspend/resume
> >
> > > From: Anson Huang <anson.huang@nxp.com>
> > > Sent: Friday, April 24, 2020 10:33 AM
> > >
> > > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > > suspend/resume
> > > >
> > > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > > > 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.
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > >
> > > > As mentioned before, we'd better keep the original author.
> > > >
> > > > > ---
> > > > >  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.
> >
> > > 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.
> > >
> >
> > 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.
> > (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

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

[Jassi Brar]

On Wed, May 27, 2020 at 8:55 PM Anson Huang <anson.huang@nxp.com> wrote:
>
> Gentle ping...
>
>
> > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > suspend/resume
> >
> >
> >
> > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > suspend/resume
> > >
> > > > From: Anson Huang <anson.huang@nxp.com>
> > > > Sent: Friday, April 24, 2020 10:33 AM
> > > >
> > > > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > > > suspend/resume
> > > > >
> > > > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > > > > 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.
> > > > > > 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.
> > > > > >
> > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > >
> > > > > As mentioned before, we'd better keep the original author.
> > > > >
> > > > > > ---
> > > > > >  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.
> > >
> > > > 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.
> > > >
> > >
> > > 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.
> > > (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.
> >
Not two patches, just add to the original patch and add a fix note in
commit as Anson suggested ... though I don't know what the original
patch was. But I am definitely in support of giving credit to the
original author.

thanks.