[RFC PATCH] staging: imx-drm: add suspend / resume

[RFC PATCH] staging: imx-drm: add suspend / resume

[Martin Fuzzey]

Currently i.MX53 boards with the imx-drm display driver active
fail an intensive suspend to ram / resume test.

After around 5 - 50 cycles it is no longer possible to resume
the board.

The culprit is the imx-drm driver which does not stop DMA
before suspending. Removing the driver "fixes" the problem.

This patch provides a minimal suspend / resume implementation
enabling the intensive test to work (500 cycles ok).

I am only sending this as RFC for the moment since I don't
really know the hardware or driver code well enough to be
sure this is the "right" way of doing it.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/staging/imx-drm/ipu-v3/ipu-common.c |   47 +++++++++++++++++++++++++++
 drivers/staging/imx-drm/ipu-v3/ipu-prv.h    |    1 +
 2 files changed, 48 insertions(+)

diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
index 97ca692..484a90a 100644
--- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
+++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
@@ -692,6 +692,8 @@ int ipu_idmac_enable_channel(struct ipuv3_channel *channel)
 	val |= idma_mask(channel->num);
 	ipu_idmac_write(ipu, val, IDMAC_CHA_EN(channel->num));
 
+	channel->enabled = true;
+
 	spin_unlock_irqrestore(&ipu->lock, flags);
 
 	return 0;
@@ -750,6 +752,8 @@ int ipu_idmac_disable_channel(struct ipuv3_channel *channel)
 	val &= ~idma_mask(channel->num);
 	ipu_cm_write(ipu, val, IPU_CHA_DB_MODE_SEL(channel->num));
 
+	channel->enabled = false;
+
 	spin_unlock_irqrestore(&ipu->lock, flags);
 
 	return 0;
@@ -1245,10 +1249,53 @@ static int ipu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+
+static int ipu_suspend(struct device *dev)
+{
+	struct ipu_soc *ipu = dev_get_drvdata(dev);
+	struct ipuv3_channel *channel;
+	int i;
+
+	channel = ipu->channel;
+	for (i = 0; i < ARRAY_SIZE(ipu->channel); i++, channel++) {
+		channel->suspended = false;
+		if (channel->enabled) {
+			if (ipu_idmac_wait_busy(channel, 50))
+				dev_warn(dev,
+					"%s: Timeout channel %d idle\n",
+					__func__, i);
+			ipu_idmac_disable_channel(channel);
+			channel->suspended = true;
+		}
+	}
+	return 0;
+}
+
+static int ipu_resume(struct device *dev)
+{
+	struct ipu_soc *ipu = dev_get_drvdata(dev);
+	struct ipuv3_channel *channel;
+	int i;
+
+	channel = ipu->channel;
+	for (i = 0; i < ARRAY_SIZE(ipu->channel); i++, channel++) {
+		if (channel->suspended) {
+			ipu_idmac_enable_channel(channel);
+			channel->suspended = false;
+		}
+	}
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ipu_pm_ops, ipu_suspend, ipu_resume);
+
 static struct platform_driver imx_ipu_driver = {
 	.driver = {
 		.name = "imx-ipuv3",
 		.of_match_table = imx_ipu_dt_ids,
+		.pm = &ipu_pm_ops,
 	},
 	.probe = ipu_probe,
 	.remove = ipu_remove,
diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-prv.h b/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
index 4df0050..233749a 100644
--- a/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
+++ b/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
@@ -144,6 +144,7 @@ struct ipuv3_channel {
 
 	bool enabled;
 	bool busy;
+	bool suspended;
 
 	struct ipu_soc *ipu;
 };

[RFC PATCH] staging: imx-drm: add suspend / resume

[Fabio Estevam]

Hi Martin,

On Thu, Mar 20, 2014 at 12:17 PM, Martin Fuzzey <mfuzzey@parkeon.com> wrote:
> Currently i.MX53 boards with the imx-drm display driver active
> fail an intensive suspend to ram / resume test.
>
> After around 5 - 50 cycles it is no longer possible to resume
> the board.
>
> The culprit is the imx-drm driver which does not stop DMA
> before suspending. Removing the driver "fixes" the problem.
>
> This patch provides a minimal suspend / resume implementation
> enabling the intensive test to work (500 cycles ok).
>
> I am only sending this as RFC for the moment since I don't
> really know the hardware or driver code well enough to be
> sure this is the "right" way of doing it.

Does this patch still cause the visual artifacts you mentioned earlier?

Regards,

Fabio Estevam

>
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/staging/imx-drm/ipu-v3/ipu-common.c |   47 +++++++++++++++++++++++++++
>  drivers/staging/imx-drm/ipu-v3/ipu-prv.h    |    1 +
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> index 97ca692..484a90a 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> @@ -692,6 +692,8 @@ int ipu_idmac_enable_channel(struct ipuv3_channel *channel)
>         val |= idma_mask(channel->num);
>         ipu_idmac_write(ipu, val, IDMAC_CHA_EN(channel->num));
>
> +       channel->enabled = true;
> +
>         spin_unlock_irqrestore(&ipu->lock, flags);
>
>         return 0;
> @@ -750,6 +752,8 @@ int ipu_idmac_disable_channel(struct ipuv3_channel *channel)
>         val &= ~idma_mask(channel->num);
>         ipu_cm_write(ipu, val, IPU_CHA_DB_MODE_SEL(channel->num));
>
> +       channel->enabled = false;
> +
>         spin_unlock_irqrestore(&ipu->lock, flags);
>
>         return 0;
> @@ -1245,10 +1249,53 @@ static int ipu_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int ipu_suspend(struct device *dev)
> +{
> +       struct ipu_soc *ipu = dev_get_drvdata(dev);
> +       struct ipuv3_channel *channel;
> +       int i;
> +
> +       channel = ipu->channel;
> +       for (i = 0; i < ARRAY_SIZE(ipu->channel); i++, channel++) {
> +               channel->suspended = false;
> +               if (channel->enabled) {
> +                       if (ipu_idmac_wait_busy(channel, 50))
> +                               dev_warn(dev,
> +                                       "%s: Timeout channel %d idle\n",
> +                                       __func__, i);
> +                       ipu_idmac_disable_channel(channel);
> +                       channel->suspended = true;
> +               }
> +       }
> +       return 0;
> +}
> +
> +static int ipu_resume(struct device *dev)
> +{
> +       struct ipu_soc *ipu = dev_get_drvdata(dev);
> +       struct ipuv3_channel *channel;
> +       int i;
> +
> +       channel = ipu->channel;
> +       for (i = 0; i < ARRAY_SIZE(ipu->channel); i++, channel++) {
> +               if (channel->suspended) {
> +                       ipu_idmac_enable_channel(channel);
> +                       channel->suspended = false;
> +               }
> +       }
> +       return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(ipu_pm_ops, ipu_suspend, ipu_resume);
> +
>  static struct platform_driver imx_ipu_driver = {
>         .driver = {
>                 .name = "imx-ipuv3",
>                 .of_match_table = imx_ipu_dt_ids,
> +               .pm = &ipu_pm_ops,
>         },
>         .probe = ipu_probe,
>         .remove = ipu_remove,
> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-prv.h b/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
> index 4df0050..233749a 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-prv.h
> @@ -144,6 +144,7 @@ struct ipuv3_channel {
>
>         bool enabled;
>         bool busy;
> +       bool suspended;
>
>         struct ipu_soc *ipu;
>  };
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH] staging: imx-drm: add suspend / resume

[Martin Fuzzey]

Hi Fabio,
On 20/03/14 16:31, Fabio Estevam wrote:
> Does this patch still cause the visual artifacts you mentioned 
> earlier? Regards, Fabio Estevam 

No.

Since I switched from poking the registers to using the existing 
wrappers I have not observed any artifcats.

Regards,

Martin

[RFC PATCH] staging: imx-drm: add suspend / resume

[Bill Pringlemeir]

On 20 Mar 2014, mfuzzey at parkeon.com wrote:

> Currently i.MX53 boards with the imx-drm display driver active
> fail an intensive suspend to ram / resume test.

> After around 5 - 50 cycles it is no longer possible to resume
> the board.

> The culprit is the imx-drm driver which does not stop DMA
> before suspending. Removing the driver "fixes" the problem.

> This patch provides a minimal suspend / resume implementation
> enabling the intensive test to work (500 cycles ok).

> I am only sending this as RFC for the moment since I don't
> really know the hardware or driver code well enough to be
> sure this is the "right" way of doing it.

Sorry, I do not known that much about the imx53.  However, the imx25
needs to have DMA controllers/modules clocked and working when trying to
suspend the DDRam.  They may currently be bursting to the RAM
independent of the main CPU, so some deep sleep (self-refresh) need all
DMA controllers to ACK the DDR controller so that it may go to
self-refresh.

The controllers (FB, USB, ENET, etc on imx25) themselves seem to have a
'sleep' mode.  However, the sleep code maybe trying to disable some
clocks during sleep which need to remain active.  If the DRM DMA is
active, but the module clock is turned off, it will not be able to ACK
the DDR-controller.  Similar issues may exist with USB, ENET, etc.

Fwiw,
Bill Pringlemeir.