diff mbox

[v4] pinctrl: mediatek: Implement wake handler and suspend resume

Message ID 1439541486-22203-1-git-send-email-maoguang.meng@mediatek.com
State New
Headers show

Commit Message

maoguang.meng@mediatek.com Aug. 14, 2015, 8:38 a.m. UTC
From: Maoguang Meng <maoguang.meng@mediatek.com>

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>

---
changes since v3:
-add a comment in mtk_eint_chip_read_mask.
-delete ALIGN when allocate eint_offsets.ports.
-fix unrelated change.

changes since v2:
-modify irq_wake to handle irq wakeup source.
-allocate two buffers separately.
-fix some codestyle.

Changes since v1:
-implement irq_wake handler.
---

 drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
 drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
 drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
 3 files changed, 95 insertions(+), 1 deletion(-)

Comments

Daniel Kurtz Aug. 14, 2015, 9:57 a.m. UTC | #1
On Fri, Aug 14, 2015 at 4:38 PM, <maoguang.meng@mediatek.com> wrote:
>
> From: Maoguang Meng <maoguang.meng@mediatek.com>
>
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
>
> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>

Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

Thanks!
-Dan

>
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.
>
> changes since v2:
> -modify irq_wake to handle irq wakeup source.
> -allocate two buffers separately.
> -fix some codestyle.
>
> Changes since v1:
> -implement irq_wake handler.
> ---
>
>  drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>  3 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index d0c811d..ad27184 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>         .driver = {
>                 .name = "mediatek-mt8173-pinctrl",
>                 .of_match_table = mt8173_pctrl_match,
> +               .pm = &mtk_eint_pm_ops,
>         },
>  };
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index ad1ea16..fe34ce9 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -33,6 +33,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> +#include <linux/pm.h>
>  #include <dt-bindings/pinctrl/mt65xx.h>
>
>  #include "../core.h"
> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>         return 0;
>  }
>
> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +       struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +       int shift = d->hwirq & 0x1f;
> +       int reg = d->hwirq >> 5;
> +
> +       if (on)
> +               pctl->wake_mask[reg] |= BIT(shift);
> +       else
> +               pctl->wake_mask[reg] &= ~BIT(shift);
> +
> +       return 0;
> +}
> +
> +static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
> +               void __iomem *eint_reg_base, u32 *buf)
> +{
> +       int port;
> +       void __iomem *reg;
> +
> +       for (port = 0; port < chip->ports; port++) {
> +               reg = eint_reg_base + (port << 2);
> +               writel_relaxed(~buf[port], reg + chip->mask_set);
> +               writel_relaxed(buf[port], reg + chip->mask_clr);
> +       }
> +}
> +
> +static void mtk_eint_chip_read_mask(const struct mtk_eint_offsets *chip,
> +               void __iomem *eint_reg_base, u32 *buf)
> +{
> +       int port;
> +       void __iomem *reg;
> +
> +       for (port = 0; port < chip->ports; port++) {
> +               reg = eint_reg_base + chip->mask + (port << 2);
> +               buf[port] = ~readl_relaxed(reg);
> +               /* Mask is 0 when irq is enabled, and 1 when disabled. */
> +       }
> +}
> +
> +static int mtk_eint_suspend(struct device *device)
> +{
> +       void __iomem *reg;
> +       struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +       const struct mtk_eint_offsets *eint_offsets =
> +                       &pctl->devdata->eint_offsets;
> +
> +       reg = pctl->eint_reg_base;
> +       mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
> +       mtk_eint_chip_write_mask(eint_offsets, reg, pctl->wake_mask);
> +
> +       return 0;
> +}
> +
> +static int mtk_eint_resume(struct device *device)
> +{
> +       struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +       const struct mtk_eint_offsets *eint_offsets =
> +                       &pctl->devdata->eint_offsets;
> +
> +       mtk_eint_chip_write_mask(eint_offsets,
> +                       pctl->eint_reg_base, pctl->cur_mask);
> +
> +       return 0;
> +}
> +
> +const struct dev_pm_ops mtk_eint_pm_ops = {
> +       .suspend = mtk_eint_suspend,
> +       .resume = mtk_eint_resume,
> +};
> +
>  static void mtk_eint_ack(struct irq_data *d)
>  {
>         struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> @@ -1076,10 +1148,12 @@ static void mtk_eint_ack(struct irq_data *d)
>
>  static struct irq_chip mtk_pinctrl_irq_chip = {
>         .name = "mt-eint",
> +       .irq_disable = mtk_eint_mask,
>         .irq_mask = mtk_eint_mask,
>         .irq_unmask = mtk_eint_unmask,
>         .irq_ack = mtk_eint_ack,
>         .irq_set_type = mtk_eint_set_type,
> +       .irq_set_wake = mtk_eint_irq_set_wake,
>         .irq_request_resources = mtk_pinctrl_irq_request_resources,
>         .irq_release_resources = mtk_pinctrl_irq_release_resources,
>  };
> @@ -1217,7 +1291,7 @@ int mtk_pctrl_init(struct platform_device *pdev,
>         struct device_node *np = pdev->dev.of_node, *node;
>         struct property *prop;
>         struct resource *res;
> -       int i, ret, irq;
> +       int i, ret, irq, ports_buf;
>
>         pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
>         if (!pctl)
> @@ -1319,6 +1393,21 @@ int mtk_pctrl_init(struct platform_device *pdev,
>                 goto chip_error;
>         }
>
> +       ports_buf = pctl->devdata->eint_offsets.ports;
> +       pctl->wake_mask = devm_kcalloc(&pdev->dev, ports_buf,
> +                                       sizeof(*pctl->wake_mask), GFP_KERNEL);
> +       if (!pctl->wake_mask) {
> +               ret = -ENOMEM;
> +               goto chip_error;
> +       }
> +
> +       pctl->cur_mask = devm_kcalloc(&pdev->dev, ports_buf,
> +                                       sizeof(*pctl->cur_mask), GFP_KERNEL);
> +       if (!pctl->cur_mask) {
> +               ret = -ENOMEM;
> +               goto chip_error;
> +       }
> +
>         pctl->eint_dual_edges = devm_kcalloc(&pdev->dev, pctl->devdata->ap_num,
>                                              sizeof(int), GFP_KERNEL);
>         if (!pctl->eint_dual_edges) {
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
> index 30213e5..f1be8e8 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
> @@ -266,6 +266,8 @@ struct mtk_pinctrl {
>         void __iomem            *eint_reg_base;
>         struct irq_domain       *domain;
>         int                     *eint_dual_edges;
> +       u32 *wake_mask;
> +       u32 *cur_mask;
>  };
>
>  int mtk_pctrl_init(struct platform_device *pdev,
> @@ -281,4 +283,6 @@ int mtk_pconf_spec_set_ies_smt_range(struct regmap *regmap,
>                 const struct mtk_pin_ies_smt_set *ies_smt_infos, unsigned int info_num,
>                 unsigned int pin, unsigned char align, int value);
>
> +extern const struct dev_pm_ops mtk_eint_pm_ops;
> +
>  #endif /* __PINCTRL_MTK_COMMON_H */
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
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
Yingjoe Chen Aug. 17, 2015, 7:52 a.m. UTC | #2
On Fri, 2015-08-14 at 16:38 +0800, maoguang.meng@mediatek.com wrote:
> From: Maoguang Meng <maoguang.meng@mediatek.com>
> 
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
> 
> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
> 
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.
> 
> changes since v2:
> -modify irq_wake to handle irq wakeup source.
> -allocate two buffers separately.
> -fix some codestyle.
> 
> Changes since v1:
> -implement irq_wake handler.
> ---
> 
>  drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>  3 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index d0c811d..ad27184 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>  	.driver = {
>  		.name = "mediatek-mt8173-pinctrl",
>  		.of_match_table = mt8173_pctrl_match,
> +		.pm = &mtk_eint_pm_ops,
>  	},
>  };
>  
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index ad1ea16..fe34ce9 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -33,6 +33,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> +#include <linux/pm.h>
>  #include <dt-bindings/pinctrl/mt65xx.h>
>  
>  #include "../core.h"
> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>  	return 0;
>  }
>  
> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +	int shift = d->hwirq & 0x1f;
> +	int reg = d->hwirq >> 5;
> +
> +	if (on)
> +		pctl->wake_mask[reg] |= BIT(shift);
> +	else
> +		pctl->wake_mask[reg] &= ~BIT(shift);
> +
> +	return 0;
> +}

Hi Maoguang,

You changed from set_bit/clear_bit to this, but didn't add any locking.
Since this is basic read/modify/write, is it OK to do it without
locking?

Joe.C


--
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
Daniel Kurtz Aug. 17, 2015, 9:09 a.m. UTC | #3
On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> On Fri, 2015-08-14 at 16:38 +0800, maoguang.meng@mediatek.com wrote:
>> From: Maoguang Meng <maoguang.meng@mediatek.com>
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
>>
>> ---
>> changes since v3:
>> -add a comment in mtk_eint_chip_read_mask.
>> -delete ALIGN when allocate eint_offsets.ports.
>> -fix unrelated change.
>>
>> changes since v2:
>> -modify irq_wake to handle irq wakeup source.
>> -allocate two buffers separately.
>> -fix some codestyle.
>>
>> Changes since v1:
>> -implement irq_wake handler.
>> ---
>>
>>  drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
>>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
>>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>>  3 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> index d0c811d..ad27184 100644
>> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>>       .driver = {
>>               .name = "mediatek-mt8173-pinctrl",
>>               .of_match_table = mt8173_pctrl_match,
>> +             .pm = &mtk_eint_pm_ops,
>>       },
>>  };
>>
>> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> index ad1ea16..fe34ce9 100644
>> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/delay.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/pm.h>
>>  #include <dt-bindings/pinctrl/mt65xx.h>
>>
>>  #include "../core.h"
>> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>>       return 0;
>>  }
>>
>> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
>> +{
>> +     struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>> +     int shift = d->hwirq & 0x1f;
>> +     int reg = d->hwirq >> 5;
>> +
>> +     if (on)
>> +             pctl->wake_mask[reg] |= BIT(shift);
>> +     else
>> +             pctl->wake_mask[reg] &= ~BIT(shift);
>> +
>> +     return 0;
>> +}
>
> Hi Maoguang,
>
> You changed from set_bit/clear_bit to this, but didn't add any locking.
> Since this is basic read/modify/write, is it OK to do it without
> locking?

I believe calling .irq_set_wake() concurrently with itself is
protected by irq_get_desc_buslock():

 int irq_set_irq_wake(unsigned int irq, unsigned int on)
 {
         unsigned long flags;
         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
IRQ_GET_DESC_CHECK_GLOBAL);
         int ret = 0;
...
                         ret = set_irq_wake_real(irq, on);
...
         irq_put_desc_busunlock(desc, flags);
         return ret;
 }


I'm not 100% sure about the .suspend/.resume paths, but I don't think
they can occur during .irq_set_wake(), either.
Nor were they protected by the set_bit/clear_bit implementation.

-Dan


>
> Joe.C
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
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
Yingjoe Chen Aug. 17, 2015, 1:25 p.m. UTC | #4
On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
> On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> > On Fri, 2015-08-14 at 16:38 +0800, maoguang.meng@mediatek.com wrote:
> >> From: Maoguang Meng <maoguang.meng@mediatek.com>
> >>
> >> This patch implement irq_set_wake to get who is wakeup source and
> >> setup on suspend resume.
> >>
> >> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
> >>
> >> ---
> >> changes since v3:
> >> -add a comment in mtk_eint_chip_read_mask.
> >> -delete ALIGN when allocate eint_offsets.ports.
> >> -fix unrelated change.
> >>
> >> changes since v2:
> >> -modify irq_wake to handle irq wakeup source.
> >> -allocate two buffers separately.
> >> -fix some codestyle.
> >>
> >> Changes since v1:
> >> -implement irq_wake handler.
> >> ---
> >>
> >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
> >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
> >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
> >>  3 files changed, 95 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> index d0c811d..ad27184 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
> >>       .driver = {
> >>               .name = "mediatek-mt8173-pinctrl",
> >>               .of_match_table = mt8173_pctrl_match,
> >> +             .pm = &mtk_eint_pm_ops,
> >>       },
> >>  };
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> index ad1ea16..fe34ce9 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> @@ -33,6 +33,7 @@
> >>  #include <linux/mfd/syscon.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/interrupt.h>
> >> +#include <linux/pm.h>
> >>  #include <dt-bindings/pinctrl/mt65xx.h>
> >>
> >>  #include "../core.h"
> >> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
> >>       return 0;
> >>  }
> >>
> >> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> >> +{
> >> +     struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> >> +     int shift = d->hwirq & 0x1f;
> >> +     int reg = d->hwirq >> 5;
> >> +
> >> +     if (on)
> >> +             pctl->wake_mask[reg] |= BIT(shift);
> >> +     else
> >> +             pctl->wake_mask[reg] &= ~BIT(shift);
> >> +
> >> +     return 0;
> >> +}
> >
> > Hi Maoguang,
> >
> > You changed from set_bit/clear_bit to this, but didn't add any locking.
> > Since this is basic read/modify/write, is it OK to do it without
> > locking?
> 
> I believe calling .irq_set_wake() concurrently with itself is
> protected by irq_get_desc_buslock():
> 
>  int irq_set_irq_wake(unsigned int irq, unsigned int on)
>  {
>          unsigned long flags;
>          struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
> IRQ_GET_DESC_CHECK_GLOBAL);
>          int ret = 0;
> ...
>                          ret = set_irq_wake_real(irq, on);
> ...
>          irq_put_desc_busunlock(desc, flags);
>          return ret;
>  }

Hi Dan,

You are right, irq_get_desc_buslock will acquire desc lock even when
irq_chip didn't provide irq_bus_lock callback. So this should be OK.
Sorry for the noise.

For this patch:

Acked-by: Yingjoe Chen <yingjoe.chen@mediatek.com>

Joe.C



--
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
Hongzhou Yang Aug. 17, 2015, 9:45 p.m. UTC | #5
On Mon, 2015-08-17 at 21:25 +0800, Yingjoe Chen wrote:
> On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
> > On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> > > On Fri, 2015-08-14 at 16:38 +0800, maoguang.meng@mediatek.com wrote:
> > >> From: Maoguang Meng <maoguang.meng@mediatek.com>
> > >>
> > >> This patch implement irq_set_wake to get who is wakeup source and
> > >> setup on suspend resume.
> > >>
> > >> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
> > >>
> > >> ---
> > >> changes since v3:
> > >> -add a comment in mtk_eint_chip_read_mask.
> > >> -delete ALIGN when allocate eint_offsets.ports.
> > >> -fix unrelated change.
> > >>
> > >> changes since v2:
> > >> -modify irq_wake to handle irq wakeup source.
> > >> -allocate two buffers separately.
> > >> -fix some codestyle.
> > >>
> > >> Changes since v1:
> > >> -implement irq_wake handler.
> > >> ---
> > >>
> > >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
> > >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
> > >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
> > >>  3 files changed, 95 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> index d0c811d..ad27184 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
> > >>       .driver = {
> > >>               .name = "mediatek-mt8173-pinctrl",
> > >>               .of_match_table = mt8173_pctrl_match,
> > >> +             .pm = &mtk_eint_pm_ops,
> > >>       },
> > >>  };
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> index ad1ea16..fe34ce9 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> @@ -33,6 +33,7 @@
> > >>  #include <linux/mfd/syscon.h>
> > >>  #include <linux/delay.h>
> > >>  #include <linux/interrupt.h>
> > >> +#include <linux/pm.h>
> > >>  #include <dt-bindings/pinctrl/mt65xx.h>
> > >>
> > >>  #include "../core.h"
> > >> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
> > >>       return 0;
> > >>  }
> > >>
> > >> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> > >> +{
> > >> +     struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> > >> +     int shift = d->hwirq & 0x1f;
> > >> +     int reg = d->hwirq >> 5;
> > >> +
> > >> +     if (on)
> > >> +             pctl->wake_mask[reg] |= BIT(shift);
> > >> +     else
> > >> +             pctl->wake_mask[reg] &= ~BIT(shift);
> > >> +
> > >> +     return 0;
> > >> +}
> > >
> > > Hi Maoguang,
> > >
> > > You changed from set_bit/clear_bit to this, but didn't add any locking.
> > > Since this is basic read/modify/write, is it OK to do it without
> > > locking?
> > 
> > I believe calling .irq_set_wake() concurrently with itself is
> > protected by irq_get_desc_buslock():
> > 
> >  int irq_set_irq_wake(unsigned int irq, unsigned int on)
> >  {
> >          unsigned long flags;
> >          struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
> > IRQ_GET_DESC_CHECK_GLOBAL);
> >          int ret = 0;
> > ...
> >                          ret = set_irq_wake_real(irq, on);
> > ...
> >          irq_put_desc_busunlock(desc, flags);
> >          return ret;
> >  }
> 
> Hi Dan,
> 
> You are right, irq_get_desc_buslock will acquire desc lock even when
> irq_chip didn't provide irq_bus_lock callback. So this should be OK.
> Sorry for the noise.
> 
> For this patch:
> 
> Acked-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> 
> Joe.C
> 


Acked-by: Hongzhou Yang <hongzhou.yang@mediatek.com>

Thanks.
Hongzhou


--
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
Sudeep Holla Aug. 24, 2015, 4:27 p.m. UTC | #6
On 14/08/15 09:38, maoguang.meng@mediatek.com wrote:
> From: Maoguang Meng <maoguang.meng@mediatek.com>
>
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
>
> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
>
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.
>
> changes since v2:
> -modify irq_wake to handle irq wakeup source.
> -allocate two buffers separately.
> -fix some codestyle.
>
> Changes since v1:
> -implement irq_wake handler.
> ---
>
>   drivers/pinctrl/mediatek/pinctrl-mt8173.c     |  1 +
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++++++++++++++++++++++++++-
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>   3 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index d0c811d..ad27184 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>   	.driver = {
>   		.name = "mediatek-mt8173-pinctrl",
>   		.of_match_table = mt8173_pctrl_match,
> +		.pm = &mtk_eint_pm_ops,
>   	},
>   };
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index ad1ea16..fe34ce9 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -33,6 +33,7 @@
>   #include <linux/mfd/syscon.h>
>   #include <linux/delay.h>
>   #include <linux/interrupt.h>
> +#include <linux/pm.h>
>   #include <dt-bindings/pinctrl/mt65xx.h>
>
>   #include "../core.h"
> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>   	return 0;
>   }
>
> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +	int shift = d->hwirq & 0x1f;
> +	int reg = d->hwirq >> 5;
> +
> +	if (on)
> +		pctl->wake_mask[reg] |= BIT(shift);
> +	else
> +		pctl->wake_mask[reg] &= ~BIT(shift);
> +
> +	return 0;
> +}

Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
    IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
    mask_{set,clear} if the same registers are used for {en,dis}able

2. Is in always on domain ? If not, you need save/restore only to
    resume back the functionality. Generally we can set 	
    IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
    disabled during suspend and re-enabled in resume path. You just
    save/restore raw values without tracking the wake-up source.

Also I see that no care is taken to set the port irq as wake enable
source. It may work with current mainline, but won't with -next. Please
ensure the port irq to the parent interrupt controller remains
enabled(i.e set as wake).

Regards,
Sudeep
--
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
Linus Walleij Aug. 26, 2015, 12:41 p.m. UTC | #7
On Fri, Aug 14, 2015 at 10:38 AM,  <maoguang.meng@mediatek.com> wrote:

> From: Maoguang Meng <maoguang.meng@mediatek.com>
>
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
>
> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
>
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.

Patch applied with Daniel's, Yingjoe's and Hongzhu's tags.

Yours,
Linus Walleij
--
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
Daniel Kurtz Sept. 2, 2015, 6:02 a.m. UTC | #8
Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 14/08/15 09:38, maoguang.meng@mediatek.com wrote:
>>
>> From: Maoguang Meng <maoguang.meng@mediatek.com>
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
>>
[snip]

Can you please respond to Sudeep's questions:

> Does this pinmux controller:
>
> 1. Support wake-up configuration ? If not, you need to use
>    IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>    mask_{set,clear} if the same registers are used for {en,dis}able
>
> 2. Is in always on domain ? If not, you need save/restore only to
>    resume back the functionality. Generally we can set
>    IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>    disabled during suspend and re-enabled in resume path. You just
>    save/restore raw values without tracking the wake-up source.
>
> Also I see that no care is taken to set the port irq as wake enable
> source. It may work with current mainline, but won't with -next. Please
> ensure the port irq to the parent interrupt controller remains
> enabled(i.e set as wake).

>
> Regards,
> Sudeep
--
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
maoguang.meng@mediatek.com Sept. 6, 2015, 10:39 a.m. UTC | #9
On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
> Hi maoguang,
> 
> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >
> > On 14/08/15 09:38, maoguang.meng@mediatek.com wrote:
> >>
> >> From: Maoguang Meng <maoguang.meng@mediatek.com>
> >>
> >> This patch implement irq_set_wake to get who is wakeup source and
> >> setup on suspend resume.
> >>
> >> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
> >>
> [snip]
> 
> Can you please respond to Sudeep's questions:
> 
> > Does this pinmux controller:
> >
> > 1. Support wake-up configuration ? If not, you need to use
> >    IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
> >    mask_{set,clear} if the same registers are used for {en,dis}able

  YES.
  we can call enable_irq_wake(irq) to config this irq as a wake-up
source.

> >
> > 2. Is in always on domain ? If not, you need save/restore only to
> >    resume back the functionality. Generally we can set
> >    IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
> >    disabled during suspend and re-enabled in resume path. You just
> >    save/restore raw values without tracking the wake-up source.

  YES.

> >
> > Also I see that no care is taken to set the port irq as wake enable
> > source. It may work with current mainline, but won't with -next. Please
> > ensure the port irq to the parent interrupt controller remains
> > enabled(i.e set as wake).

As you know, we do not care the port irq as wake enbale source,
when enter suspend we think no matter what the port irq is
enabled/disabled which are not affected as a wake-up source,
Because eint has a line to receive SPM.

For example,after suspend,press power key(use eint5) will generate an
electrical signal which is send to spm by eint.
and then spm wake cpu.
  
> >
> > Regards,
> > Sudeep

Regards,
Maoguang

--
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
Sudeep Holla Sept. 8, 2015, 9:28 a.m. UTC | #10
On 06/09/15 11:39, maoguang meng wrote:
> On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
>> Hi maoguang,
>>
>> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 14/08/15 09:38, maoguang.meng@mediatek.com wrote:
>>>>
>>>> From: Maoguang Meng <maoguang.meng@mediatek.com>
>>>>
>>>> This patch implement irq_set_wake to get who is wakeup source and
>>>> setup on suspend resume.
>>>>
>>>> Signed-off-by: Maoguang Meng <maoguang.meng@mediatek.com>
>>>>
>> [snip]
>>
>> Can you please respond to Sudeep's questions:
>>
>>> Does this pinmux controller:
>>>
>>> 1. Support wake-up configuration ? If not, you need to use
>>>     IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>>>     mask_{set,clear} if the same registers are used for {en,dis}able
>
>    YES.
>    we can call enable_irq_wake(irq) to config this irq as a wake-up
> source.
>

No that doesn't answer my question. Yes you can always call
enable_irq_wake(irq) as along as you have irq_set_wake implemented.
But my question was does this pinmux controller support wake-up
configuration.

IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
So please stop copy-pasting the implementation from other drivers.
The reason is you operate on the same mask_{set,clear} which you use
to {en,dis}able the interrupts which means you don't have to do anything
to configure an interrupt as a wakeup source other than just keeping
them enabled. Hopefully this clarifies.

>>>
>>> 2. Is in always on domain ? If not, you need save/restore only to
>>>     resume back the functionality. Generally we can set
>>>     IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>>>     disabled during suspend and re-enabled in resume path. You just
>>>     save/restore raw values without tracking the wake-up source.
>
>    YES.
>

Again *YES* for what part of my question. If it's always-on, then you
don't need this suspend/resume callbacks at all, so I assume the answer
is *NO*.

>>>
>>> Also I see that no care is taken to set the port irq as wake enable
>>> source. It may work with current mainline, but won't with -next. Please
>>> ensure the port irq to the parent interrupt controller remains
>>> enabled(i.e set as wake).
>
> As you know, we do not care the port irq as wake enbale source,
> when enter suspend we think no matter what the port irq is
> enabled/disabled which are not affected as a wake-up source,
> Because eint has a line to receive SPM.
>

I understand that, but for proper wakeup functionality you need to do
configure the parent if you want to identify and handle that wakeup
interrupt. If you don't care then it should be fine.

> For example,after suspend,press power key(use eint5) will generate an
> electrical signal which is send to spm by eint.
> and then spm wake cpu.

Yes I got that, as I mentioned above, I think you don't need to identify
and handle the wakeup interrupt. E.g. if it's a keyboard key press, you
may need to identify the key pressed, so it needs to be handled
properly. In your case, I assume it's just power button whose main
function is to wake/boot and that's already achieved when you are woken up.

Regards,
Sudeep
--
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 mbox

Patch

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
index d0c811d..ad27184 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
@@ -385,6 +385,7 @@  static struct platform_driver mtk_pinctrl_driver = {
 	.driver = {
 		.name = "mediatek-mt8173-pinctrl",
 		.of_match_table = mt8173_pctrl_match,
+		.pm = &mtk_eint_pm_ops,
 	},
 };
 
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
index ad1ea16..fe34ce9 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -33,6 +33,7 @@ 
 #include <linux/mfd/syscon.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/pm.h>
 #include <dt-bindings/pinctrl/mt65xx.h>
 
 #include "../core.h"
@@ -1062,6 +1063,77 @@  static int mtk_eint_set_type(struct irq_data *d,
 	return 0;
 }
 
+static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+	int shift = d->hwirq & 0x1f;
+	int reg = d->hwirq >> 5;
+
+	if (on)
+		pctl->wake_mask[reg] |= BIT(shift);
+	else
+		pctl->wake_mask[reg] &= ~BIT(shift);
+
+	return 0;
+}
+
+static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
+		void __iomem *eint_reg_base, u32 *buf)
+{
+	int port;
+	void __iomem *reg;
+
+	for (port = 0; port < chip->ports; port++) {
+		reg = eint_reg_base + (port << 2);
+		writel_relaxed(~buf[port], reg + chip->mask_set);
+		writel_relaxed(buf[port], reg + chip->mask_clr);
+	}
+}
+
+static void mtk_eint_chip_read_mask(const struct mtk_eint_offsets *chip,
+		void __iomem *eint_reg_base, u32 *buf)
+{
+	int port;
+	void __iomem *reg;
+
+	for (port = 0; port < chip->ports; port++) {
+		reg = eint_reg_base + chip->mask + (port << 2);
+		buf[port] = ~readl_relaxed(reg);
+		/* Mask is 0 when irq is enabled, and 1 when disabled. */
+	}
+}
+
+static int mtk_eint_suspend(struct device *device)
+{
+	void __iomem *reg;
+	struct mtk_pinctrl *pctl = dev_get_drvdata(device);
+	const struct mtk_eint_offsets *eint_offsets =
+			&pctl->devdata->eint_offsets;
+
+	reg = pctl->eint_reg_base;
+	mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
+	mtk_eint_chip_write_mask(eint_offsets, reg, pctl->wake_mask);
+
+	return 0;
+}
+
+static int mtk_eint_resume(struct device *device)
+{
+	struct mtk_pinctrl *pctl = dev_get_drvdata(device);
+	const struct mtk_eint_offsets *eint_offsets =
+			&pctl->devdata->eint_offsets;
+
+	mtk_eint_chip_write_mask(eint_offsets,
+			pctl->eint_reg_base, pctl->cur_mask);
+
+	return 0;
+}
+
+const struct dev_pm_ops mtk_eint_pm_ops = {
+	.suspend = mtk_eint_suspend,
+	.resume = mtk_eint_resume,
+};
+
 static void mtk_eint_ack(struct irq_data *d)
 {
 	struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
@@ -1076,10 +1148,12 @@  static void mtk_eint_ack(struct irq_data *d)
 
 static struct irq_chip mtk_pinctrl_irq_chip = {
 	.name = "mt-eint",
+	.irq_disable = mtk_eint_mask,
 	.irq_mask = mtk_eint_mask,
 	.irq_unmask = mtk_eint_unmask,
 	.irq_ack = mtk_eint_ack,
 	.irq_set_type = mtk_eint_set_type,
+	.irq_set_wake = mtk_eint_irq_set_wake,
 	.irq_request_resources = mtk_pinctrl_irq_request_resources,
 	.irq_release_resources = mtk_pinctrl_irq_release_resources,
 };
@@ -1217,7 +1291,7 @@  int mtk_pctrl_init(struct platform_device *pdev,
 	struct device_node *np = pdev->dev.of_node, *node;
 	struct property *prop;
 	struct resource *res;
-	int i, ret, irq;
+	int i, ret, irq, ports_buf;
 
 	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
 	if (!pctl)
@@ -1319,6 +1393,21 @@  int mtk_pctrl_init(struct platform_device *pdev,
 		goto chip_error;
 	}
 
+	ports_buf = pctl->devdata->eint_offsets.ports;
+	pctl->wake_mask = devm_kcalloc(&pdev->dev, ports_buf,
+					sizeof(*pctl->wake_mask), GFP_KERNEL);
+	if (!pctl->wake_mask) {
+		ret = -ENOMEM;
+		goto chip_error;
+	}
+
+	pctl->cur_mask = devm_kcalloc(&pdev->dev, ports_buf,
+					sizeof(*pctl->cur_mask), GFP_KERNEL);
+	if (!pctl->cur_mask) {
+		ret = -ENOMEM;
+		goto chip_error;
+	}
+
 	pctl->eint_dual_edges = devm_kcalloc(&pdev->dev, pctl->devdata->ap_num,
 					     sizeof(int), GFP_KERNEL);
 	if (!pctl->eint_dual_edges) {
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
index 30213e5..f1be8e8 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.h
@@ -266,6 +266,8 @@  struct mtk_pinctrl {
 	void __iomem		*eint_reg_base;
 	struct irq_domain	*domain;
 	int			*eint_dual_edges;
+	u32 *wake_mask;
+	u32 *cur_mask;
 };
 
 int mtk_pctrl_init(struct platform_device *pdev,
@@ -281,4 +283,6 @@  int mtk_pconf_spec_set_ies_smt_range(struct regmap *regmap,
 		const struct mtk_pin_ies_smt_set *ies_smt_infos, unsigned int info_num,
 		unsigned int pin, unsigned char align, int value);
 
+extern const struct dev_pm_ops mtk_eint_pm_ops;
+
 #endif /* __PINCTRL_MTK_COMMON_H */