[8/9] pinctrl: samsung: Add runtime PM support

Message ID 1482495889-6201-9-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Dec. 23, 2016, 12:24 p.m.
This patch adds runtime power management support to Samsung pin controller
driver. It uses recently introduced property to mark some pin states as
suitable for power off. When all pins for given controller are set to this
special state, the controller is able to enter runtime suspend state. This
in turn might allow to turn respective power domain off to reduce power
consumption.

This patch moves saving driver state to runtime pm callbacks and implements
system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume
helpers to keep the runtime pm state consistent with the hardware both
during runtime and system sleep transitions.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Krzysztof Kozlowski Dec. 25, 2016, 7:26 p.m. | #1
On Fri, Dec 23, 2016 at 01:24:48PM +0100, Marek Szyprowski wrote:
> This patch adds runtime power management support to Samsung pin controller

> driver. It uses recently introduced property to mark some pin states as

> suitable for power off. When all pins for given controller are set to this

> special state, the controller is able to enter runtime suspend state. This

> in turn might allow to turn respective power domain off to reduce power

> consumption.

> 

> This patch moves saving driver state to runtime pm callbacks and implements

> system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume

> helpers to keep the runtime pm state consistent with the hardware both

> during runtime and system sleep transitions.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--

>  drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +

>  2 files changed, 23 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c

> index 301169d2b6e1..c741e93d65b8 100644

> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c

> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c

> @@ -28,6 +28,7 @@

>  #include <linux/gpio.h>

>  #include <linux/irqdomain.h>

>  #include <linux/of_device.h>

> +#include <linux/pm_runtime.h>

>  #include <linux/spinlock.h>

>  

>  #include "../core.h"

> @@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,

>  		shift -= 32;

>  		reg += 4;

>  	}

> +	if (func->rpm_active) {

> +		if (!(bank->rpm_map & (1 << pin_offset))) {

> +			bank->rpm_map |= 1 << pin_offset;

> +			pm_runtime_get_sync(drvdata->dev);

> +		}

> +	} else {

> +		if ((bank->rpm_map & (1 << pin_offset))) {

> +			bank->rpm_map &= ~(1 << pin_offset);

> +			pm_runtime_put(drvdata->dev);

> +		}

> +	}

>  

>  	spin_lock_irqsave(&bank->slock, flags);

>  

> @@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,

>  	if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])

>  		return -EINVAL;

>  

> +	pm_runtime_get_sync(drvdata->dev);

> +

>  	width = type->fld_width[cfg_type];

>  	cfg_reg = type->reg_offset[cfg_type];

>  

> @@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,

>  

>  	spin_unlock_irqrestore(&bank->slock, flags);

>  

> +	pm_runtime_put(drvdata->dev);

> +

>  	return 0;

>  }

>  

> @@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)

>  		ctrl->retention_init(drvdata);

>  

>  	platform_set_drvdata(pdev, drvdata);

> +	pm_runtime_set_active(dev);

> +	pm_runtime_enable(dev);

>  

>  	return 0;

>  }

> @@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)

>  MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);

>  

>  static const struct dev_pm_ops samsung_pinctrl_pm_ops = {

> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,

> -				     samsung_pinctrl_resume)

> +	SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,

> +			   samsung_pinctrl_resume, NULL)

> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

> +				     pm_runtime_force_resume)

>  };

>  

>  static struct platform_driver samsung_pinctrl_driver = {

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h

> index edeafa00abd3..ccb24ec46796 100644

> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h

> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h

> @@ -172,6 +172,7 @@ struct samsung_pin_bank {


A nit:
Missing kernel doc update.

It took me a while to understand the logic behind the rpm_active (the
code is simple but logic was not that easy to see) but it looks good and
sensible. I didn't find any issues with that approach except the usage
of DT configuration as a "notification" mechanism.

For this itself:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof


>  	const char	*name;

>  

>  	u32		pin_base;

> +	u32		rpm_map;

>  	void		*soc_priv;

>  	struct device_node *of_node;

>  	struct samsung_pinctrl_drv_data *drvdata;

> -- 

> 1.9.1

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Dec. 26, 2016, 6:11 a.m. | #2
Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> This patch adds runtime power management support to Samsung pin controller

> driver. It uses recently introduced property to mark some pin states as

> suitable for power off. When all pins for given controller are set to this

> special state, the controller is able to enter runtime suspend state. This

> in turn might allow to turn respective power domain off to reduce power

> consumption.

>

> This patch moves saving driver state to runtime pm callbacks and implements

> system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume

> helpers to keep the runtime pm state consistent with the hardware both

> during runtime and system sleep transitions.

>

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--

>  drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +

>  2 files changed, 23 insertions(+), 2 deletions(-)

>

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c

> index 301169d2b6e1..c741e93d65b8 100644

> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c

> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c

> @@ -28,6 +28,7 @@

>  #include <linux/gpio.h>

>  #include <linux/irqdomain.h>

>  #include <linux/of_device.h>

> +#include <linux/pm_runtime.h>

>  #include <linux/spinlock.h>

>

>  #include "../core.h"

> @@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,

>                 shift -= 32;

>                 reg += 4;

>         }


nit: IMHO a blank line here would look better.

> +       if (func->rpm_active) {

> +               if (!(bank->rpm_map & (1 << pin_offset))) {


I think rpm_map could benefit from generic bitmap helpers.

> +                       bank->rpm_map |= 1 << pin_offset;

> +                       pm_runtime_get_sync(drvdata->dev);

> +               }

> +       } else {

> +               if ((bank->rpm_map & (1 << pin_offset))) {

> +                       bank->rpm_map &= ~(1 << pin_offset);

> +                       pm_runtime_put(drvdata->dev);

> +               }

> +       }


Then, the above could be simplified into:

        if (func->rpm_active && !test_bit(pin_offset, &bank->rpm_map)) {
                __set_bit(pin_offset, &bank->rpm_map);
                pm_runtime_get_sync(drvdata->dev);
        } else if (!func->rpm_active && test_bit(pin_offset, &bank->rpm_map) {
                __clear_bit(pin_offset, &bank->rpm_map);
                pm_runtime_put(drvdata->dev);
        }

>

>         spin_lock_irqsave(&bank->slock, flags);

>

> @@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,

>         if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])

>                 return -EINVAL;

>

> +       pm_runtime_get_sync(drvdata->dev);

> +

>         width = type->fld_width[cfg_type];

>         cfg_reg = type->reg_offset[cfg_type];

>

> @@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,

>

>         spin_unlock_irqrestore(&bank->slock, flags);

>

> +       pm_runtime_put(drvdata->dev);

> +

>         return 0;

>  }

>

> @@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)

>                 ctrl->retention_init(drvdata);

>

>         platform_set_drvdata(pdev, drvdata);

> +       pm_runtime_set_active(dev);

> +       pm_runtime_enable(dev);


Can we really enable this unconditionally on all platforms?

>

>         return 0;

>  }

> @@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)

>  MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);

>

>  static const struct dev_pm_ops samsung_pinctrl_pm_ops = {

> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,

> -                                    samsung_pinctrl_resume)

> +       SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,

> +                          samsung_pinctrl_resume, NULL)

> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

> +                                    pm_runtime_force_resume)

>  };

>

>  static struct platform_driver samsung_pinctrl_driver = {

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h

> index edeafa00abd3..ccb24ec46796 100644

> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h

> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h

> @@ -172,6 +172,7 @@ struct samsung_pin_bank {

>         const char      *name;

>

>         u32             pin_base;

> +       u32             rpm_map;


This could be an unsigned long instead and then could benefit from the
bitmap helpers.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 301169d2b6e1..c741e93d65b8 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -28,6 +28,7 @@ 
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 
 #include "../core.h"
@@ -378,6 +379,17 @@  static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 		shift -= 32;
 		reg += 4;
 	}
+	if (func->rpm_active) {
+		if (!(bank->rpm_map & (1 << pin_offset))) {
+			bank->rpm_map |= 1 << pin_offset;
+			pm_runtime_get_sync(drvdata->dev);
+		}
+	} else {
+		if ((bank->rpm_map & (1 << pin_offset))) {
+			bank->rpm_map &= ~(1 << pin_offset);
+			pm_runtime_put(drvdata->dev);
+		}
+	}
 
 	spin_lock_irqsave(&bank->slock, flags);
 
@@ -427,6 +439,8 @@  static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
 	if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
 		return -EINVAL;
 
+	pm_runtime_get_sync(drvdata->dev);
+
 	width = type->fld_width[cfg_type];
 	cfg_reg = type->reg_offset[cfg_type];
 
@@ -449,6 +463,8 @@  static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
+	pm_runtime_put(drvdata->dev);
+
 	return 0;
 }
 
@@ -1096,6 +1112,8 @@  static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->retention_init(drvdata);
 
 	platform_set_drvdata(pdev, drvdata);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	return 0;
 }
@@ -1242,8 +1260,10 @@  static int samsung_pinctrl_resume(struct device *dev)
 MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
 
 static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
-				     samsung_pinctrl_resume)
+	SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,
+			   samsung_pinctrl_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
 };
 
 static struct platform_driver samsung_pinctrl_driver = {
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index edeafa00abd3..ccb24ec46796 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -172,6 +172,7 @@  struct samsung_pin_bank {
 	const char	*name;
 
 	u32		pin_base;
+	u32		rpm_map;
 	void		*soc_priv;
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;