| Message ID | 1482495889-6201-9-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
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
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
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;
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