[PATCH 0/2] drm: atmel-hlcdc: PM support

[PATCH 0/2] drm: atmel-hlcdc: PM support

[Sylvain Rochet]

This series depends on Boris' "[PATCH v2] drm: atmel-hlcdc: Atomic 
mode-setting conversion"
  <1423236143-6494-1-git-send-email-boris.brezillon@free-electrons.com>
plus a few fixes which are going to be in v3 of Boris' patch.

This series adds basic PM support for Atmel HLCDC.

Sylvain Rochet (2):
  drm: atmel-hlcdc: Add PM suspend/resume support
  drm: atmel-hlcdc: Add pinctrl PM select sleep,default state in CRTC
    suspend/resume

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c |  3 ++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c   | 60 ++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

-- 
2.1.4

[PATCH 1/2] drm: atmel-hlcdc: Add PM suspend/resume support

[Sylvain Rochet]

On suspend: switch off CRTC if not already suspended with runtime PM

On resume: switch on CRTC if we were not already suspended from runtime
PM while suspending.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 60 ++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 22c3cca..4c89bd2 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -427,6 +427,36 @@ static void atmel_hlcdc_dc_lastclose(struct drm_device *dev)
 	drm_fbdev_cma_restore_mode(dc->fbdev);
 }
 
+static int atmel_hlcdc_dc_suspend(struct drm_device *dev, pm_message_t state)
+{
+	struct drm_crtc *crtc;
+
+	drm_modeset_lock_all(dev);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+		(*crtc_funcs->disable)(crtc);
+	}
+
+	drm_modeset_unlock_all(dev);
+	return 0;
+}
+
+static int atmel_hlcdc_dc_resume(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	drm_modeset_lock_all(dev);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+		(*crtc_funcs->enable)(crtc);
+	}
+
+	drm_modeset_unlock_all(dev);
+	return 0;
+}
+
 static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
@@ -488,6 +518,8 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
 	.driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
 	.preclose = atmel_hlcdc_dc_preclose,
 	.lastclose = atmel_hlcdc_dc_lastclose,
+	.suspend = atmel_hlcdc_dc_suspend,
+	.resume = atmel_hlcdc_dc_resume,
 	.irq_handler = atmel_hlcdc_dc_irq_handler,
 	.irq_preinstall = atmel_hlcdc_dc_irq_uninstall,
 	.irq_postinstall = atmel_hlcdc_dc_irq_postinstall,
@@ -559,6 +591,33 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	pm_message_t message;
+
+	if (pm_runtime_suspended(dev) || !drm_dev)
+		return 0;
+
+	message.event = PM_EVENT_SUSPEND;
+	return atmel_hlcdc_dc_suspend(drm_dev, message);
+}
+
+static int atmel_hlcdc_dc_drm_resume(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(dev) || !drm_dev)
+		return 0;
+
+	return atmel_hlcdc_dc_resume(drm_dev);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_hlcdc_dc_drm_pm_ops,
+		atmel_hlcdc_dc_drm_suspend, atmel_hlcdc_dc_drm_resume);
+
 static const struct of_device_id atmel_hlcdc_dc_of_match[] = {
 	{ .compatible = "atmel,hlcdc-display-controller" },
 	{ },
@@ -569,6 +628,7 @@ static struct platform_driver atmel_hlcdc_dc_platform_driver = {
 	.remove	= atmel_hlcdc_dc_drm_remove,
 	.driver	= {
 		.name	= "atmel-hlcdc-display-controller",
+		.pm	= &atmel_hlcdc_dc_drm_pm_ops,
 		.of_match_table = atmel_hlcdc_dc_of_match,
 	},
 };
-- 
2.1.4

[PATCH 2/2] drm: atmel-hlcdc: Add pinctrl PM select sleep, default state in CRTC suspend/resume

[Sylvain Rochet]

Some LCD panels have back-powering issue when un-powered, allows users
to use an alternate pinctrl "sleep" in order to clamp outputs to a
wanted state at suspend.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 79c9a19..decd9ea 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -21,6 +21,7 @@
 #include <linux/clk.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
@@ -156,6 +157,7 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
 		cpu_relax();
 
 	clk_disable_unprepare(crtc->dc->hlcdc->sys_clk);
+	pinctrl_pm_select_sleep_state(dev->dev);
 
 	pm_runtime_allow(dev->dev);
 
@@ -178,6 +180,7 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
 
 	pm_runtime_forbid(dev->dev);
 
+	pinctrl_pm_select_default_state(dev->dev);
 	clk_prepare_enable(crtc->dc->hlcdc->sys_clk);
 
 	regmap_write(regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PIXEL_CLK);
-- 
2.1.4

[PATCH 1/2] drm: atmel-hlcdc: Add PM suspend/resume support

[Boris Brezillon]

Hi Sylvain,

On Tue, 10 Feb 2015 14:40:45 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:

> On suspend: switch off CRTC if not already suspended with runtime PM
> 
> On resume: switch on CRTC if we were not already suspended from runtime
> PM while suspending.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 60 ++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 22c3cca..4c89bd2 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -427,6 +427,36 @@ static void atmel_hlcdc_dc_lastclose(struct drm_device *dev)
>  	drm_fbdev_cma_restore_mode(dc->fbdev);
>  }
>  
> +static int atmel_hlcdc_dc_suspend(struct drm_device *dev, pm_message_t state)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_modeset_lock_all(dev);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> +		(*crtc_funcs->disable)(crtc);

Please replace that line by:

		crtc_funcs->disable(crtc);
		

> +	}
> +
> +	drm_modeset_unlock_all(dev);
> +	return 0;
> +}
> +
> +static int atmel_hlcdc_dc_resume(struct drm_device *dev)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_modeset_lock_all(dev);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> +		(*crtc_funcs->enable)(crtc);

Ditto:
		crtc_funcs->enable(crtc);

> +	}
> +
> +	drm_modeset_unlock_all(dev);
> +	return 0;
> +}
> +
>  static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
>  {
>  	struct atmel_hlcdc_dc *dc = dev->dev_private;
> @@ -488,6 +518,8 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>  	.driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
>  	.preclose = atmel_hlcdc_dc_preclose,
>  	.lastclose = atmel_hlcdc_dc_lastclose,
> +	.suspend = atmel_hlcdc_dc_suspend,
> +	.resume = atmel_hlcdc_dc_resume,
>  	.irq_handler = atmel_hlcdc_dc_irq_handler,
>  	.irq_preinstall = atmel_hlcdc_dc_irq_uninstall,
>  	.irq_postinstall = atmel_hlcdc_dc_irq_postinstall,
> @@ -559,6 +591,33 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	pm_message_t message;
> +
> +	if (pm_runtime_suspended(dev) || !drm_dev)
> +		return 0;
> +
> +	message.event = PM_EVENT_SUSPEND;
> +	return atmel_hlcdc_dc_suspend(drm_dev, message);
> +}
> +
> +static int atmel_hlcdc_dc_drm_resume(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	if (pm_runtime_suspended(dev) || !drm_dev)
> +		return 0;
> +
> +	return atmel_hlcdc_dc_resume(drm_dev);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(atmel_hlcdc_dc_drm_pm_ops,
> +		atmel_hlcdc_dc_drm_suspend, atmel_hlcdc_dc_drm_resume);
> +

Do we really need to register both SIMPLE_DEV_PM_OPS and drm_driver
suspend/resume functions.
I thought the suspend/resume callbacks were called by DRM core code
as part of the PM class specific operations.
However, exynos driver seems to do the same, so this might well be
required.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/2] drm: atmel-hlcdc: Add pinctrl PM select sleep,default state in CRTC suspend/resume

[Boris Brezillon]

On Tue, 10 Feb 2015 14:40:46 +0100
Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:

> Some LCD panels have back-powering issue when un-powered, allows users
> to use an alternate pinctrl "sleep" in order to clamp outputs to a
> wanted state at suspend.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 79c9a19..decd9ea 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -21,6 +21,7 @@
>  #include <linux/clk.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -156,6 +157,7 @@ static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
>  		cpu_relax();
>  
>  	clk_disable_unprepare(crtc->dc->hlcdc->sys_clk);
> +	pinctrl_pm_select_sleep_state(dev->dev);
>  
>  	pm_runtime_allow(dev->dev);
>  
> @@ -178,6 +180,7 @@ static void atmel_hlcdc_crtc_enable(struct drm_crtc *c)
>  
>  	pm_runtime_forbid(dev->dev);
>  
> +	pinctrl_pm_select_default_state(dev->dev);
>  	clk_prepare_enable(crtc->dc->hlcdc->sys_clk);
>  
>  	regmap_write(regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PIXEL_CLK);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/2] drm: atmel-hlcdc: Add PM suspend/resume support

[Sylvain Rochet]

Hello Boris,

On Tue, Feb 10, 2015 at 03:05:57PM +0100, Boris Brezillon wrote:
> On Tue, 10 Feb 2015 14:40:45 +0100
> Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > +
> > +static SIMPLE_DEV_PM_OPS(atmel_hlcdc_dc_drm_pm_ops,
> > +		atmel_hlcdc_dc_drm_suspend, atmel_hlcdc_dc_drm_resume);
> > +
> 
> Do we really need to register both SIMPLE_DEV_PM_OPS and drm_driver
> suspend/resume functions.
> I thought the suspend/resume callbacks were called by DRM core code
> as part of the PM class specific operations.
> However, exynos driver seems to do the same, so this might well be
> required.

Looks like we need, if I comment out SIMPLE_DEV_PM_OPS ops registration 
we don't suspend or resume anymore. I guess drm_driver suspend/resume 
functions are only used for PM runtime support, which we don't support 
yet, at least we added the basic requirements to add PM runtime support 
at some point in the future.

Anyway, even without PM runtime support we need to suspend and resume 
this device, other DRM drivers do this as well.

Sylvain