| Message ID | 20200529191522.27938-5-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: bcm2835: Support for wake-up interrupts | expand |
Hi Florian, Am 29.05.20 um 21:15 schrieb Florian Fainelli: > Leverage the IRQCHIP_MASK_ON_SUSPEND flag in order to avoid having to > specifically treat the GPIO interrupts during suspend and resume, and > simply implement an irq_set_wake() callback that is responsible for > enabling the parent wake-up interrupt as a wake-up interrupt. > > To avoid allocating unnecessary resources for other chips, the wake-up > interrupts are only initialized if we have a brcm,bcm7211-gpio > compatibility string. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 76 ++++++++++++++++++++++++++- > 1 file changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index 1b00d93aa66e..1fbf067a3eed 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -19,6 +19,7 @@ > #include <linux/irq.h> > #include <linux/irqdesc.h> > #include <linux/init.h> > +#include <linux/interrupt.h> > #include <linux/of_address.h> > #include <linux/of.h> > #include <linux/of_irq.h> > @@ -76,6 +77,7 @@ > struct bcm2835_pinctrl { > struct device *dev; > void __iomem *base; > + int *wake_irq; > > /* note: locking assumes each bank will have its own unsigned long */ > unsigned long enabled_irq_map[BCM2835_NUM_BANKS]; > @@ -435,6 +437,11 @@ static void bcm2835_gpio_irq_handler(struct irq_desc *desc) > chained_irq_exit(host_chip, desc); > } > > +static irqreturn_t bcm2835_gpio_wake_irq_handler(int irq, void *dev_id) > +{ > + return IRQ_HANDLED; > +} > + > static inline void __bcm2835_gpio_irq_config(struct bcm2835_pinctrl *pc, > unsigned reg, unsigned offset, bool enable) > { > @@ -634,6 +641,34 @@ static void bcm2835_gpio_irq_ack(struct irq_data *data) > bcm2835_gpio_set_bit(pc, GPEDS0, gpio); > } > > +static int bcm2835_gpio_irq_set_wake(struct irq_data *data, unsigned int on) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > + struct bcm2835_pinctrl *pc = gpiochip_get_data(chip); > + unsigned gpio = irqd_to_hwirq(data); > + unsigned int irqgroup; > + int ret = -EINVAL; > + > + if (!pc->wake_irq) > + return ret; > + > + if (gpio <= 27) > + irqgroup = 0; > + else if (gpio >= 28 && gpio <= 45) > + irqgroup = 1; > + else if (gpio >= 46 && gpio <= 53) > + irqgroup = 2; in case the BCM7211 has 58 GPIOs, but the wake up interrupts are only available for the first 54 this should deserve a comment. > + else > + return ret; > + > + if (on) > + ret = enable_irq_wake(pc->wake_irq[irqgroup]); > + else > + ret = disable_irq_wake(pc->wake_irq[irqgroup]); > + > + return ret; > +} > + > static struct irq_chip bcm2835_gpio_irq_chip = { > .name = MODULE_NAME, > .irq_enable = bcm2835_gpio_irq_enable, > @@ -642,6 +677,8 @@ static struct irq_chip bcm2835_gpio_irq_chip = { > .irq_ack = bcm2835_gpio_irq_ack, > .irq_mask = bcm2835_gpio_irq_disable, > .irq_unmask = bcm2835_gpio_irq_enable, > + .irq_set_wake = bcm2835_gpio_irq_set_wake, > + .flags = IRQCHIP_MASK_ON_SUSPEND, > }; > > static int bcm2835_pctl_get_groups_count(struct pinctrl_dev *pctldev) > @@ -1154,6 +1191,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) > struct resource iomem; > int err, i; > const struct of_device_id *match; > + int is_7211 = 0; > > BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_pins) != BCM2711_NUM_GPIOS); > BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_groups) != BCM2711_NUM_GPIOS); > @@ -1180,6 +1218,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) > return -EINVAL; > > pdata = match->data; > + is_7211 = of_device_is_compatible(np, "brcm,bcm7211-gpio"); > > pc->gpio_chip = *pdata->gpio_chip; > pc->gpio_chip.parent = dev; > @@ -1214,6 +1253,15 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) > GFP_KERNEL); > if (!girq->parents) > return -ENOMEM; > + > + if (is_7211) { > + pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, > + sizeof(*pc->wake_irq), > + GFP_KERNEL); > + if (!pc->wake_irq) > + return -ENOMEM; > + } > + > /* > * Use the same handler for all groups: this is necessary > * since we use one gpiochip to cover all lines - the > @@ -1221,8 +1269,34 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) > * bank that was firing the IRQ and look up the per-group > * and bank data. > */ > - for (i = 0; i < BCM2835_NUM_IRQS; i++) > + for (i = 0; i < BCM2835_NUM_IRQS; i++) { > + int len; > + char *name; > + > girq->parents[i] = irq_of_parse_and_map(np, i); > + if (!is_7211) > + continue; > + > + /* Skip over the all banks interrupts */ > + pc->wake_irq[i] = irq_of_parse_and_map(np, i + > + BCM2835_NUM_IRQS + 1); > + > + len = strlen(dev_name(pc->dev)) + 16; > + name = devm_kzalloc(pc->dev, len, GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + snprintf(name, len, "%s:bank%d", dev_name(pc->dev), i); > + > + /* These are optional interrupts */ > + err = devm_request_irq(dev, pc->wake_irq[i], > + bcm2835_gpio_wake_irq_handler, > + IRQF_SHARED, name, pc); > + if (err) > + dev_warn(dev, "unable to request wake IRQ %d\n", > + pc->wake_irq[i]); > + } > + > girq->default_type = IRQ_TYPE_NONE; > girq->handler = handle_level_irq; >
On 5/30/2020 12:49 AM, Stefan Wahren wrote: > Hi Florian, > > Am 29.05.20 um 21:15 schrieb Florian Fainelli: >> Leverage the IRQCHIP_MASK_ON_SUSPEND flag in order to avoid having to >> specifically treat the GPIO interrupts during suspend and resume, and >> simply implement an irq_set_wake() callback that is responsible for >> enabling the parent wake-up interrupt as a wake-up interrupt. >> >> To avoid allocating unnecessary resources for other chips, the wake-up >> interrupts are only initialized if we have a brcm,bcm7211-gpio >> compatibility string. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/pinctrl/bcm/pinctrl-bcm2835.c | 76 ++++++++++++++++++++++++++- >> 1 file changed, 75 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> index 1b00d93aa66e..1fbf067a3eed 100644 >> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> @@ -19,6 +19,7 @@ >> #include <linux/irq.h> >> #include <linux/irqdesc.h> >> #include <linux/init.h> >> +#include <linux/interrupt.h> >> #include <linux/of_address.h> >> #include <linux/of.h> >> #include <linux/of_irq.h> >> @@ -76,6 +77,7 @@ >> struct bcm2835_pinctrl { >> struct device *dev; >> void __iomem *base; >> + int *wake_irq; >> >> /* note: locking assumes each bank will have its own unsigned long */ >> unsigned long enabled_irq_map[BCM2835_NUM_BANKS]; >> @@ -435,6 +437,11 @@ static void bcm2835_gpio_irq_handler(struct irq_desc *desc) >> chained_irq_exit(host_chip, desc); >> } >> >> +static irqreturn_t bcm2835_gpio_wake_irq_handler(int irq, void *dev_id) >> +{ >> + return IRQ_HANDLED; >> +} >> + >> static inline void __bcm2835_gpio_irq_config(struct bcm2835_pinctrl *pc, >> unsigned reg, unsigned offset, bool enable) >> { >> @@ -634,6 +641,34 @@ static void bcm2835_gpio_irq_ack(struct irq_data *data) >> bcm2835_gpio_set_bit(pc, GPEDS0, gpio); >> } >> >> +static int bcm2835_gpio_irq_set_wake(struct irq_data *data, unsigned int on) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); >> + struct bcm2835_pinctrl *pc = gpiochip_get_data(chip); >> + unsigned gpio = irqd_to_hwirq(data); >> + unsigned int irqgroup; >> + int ret = -EINVAL; >> + >> + if (!pc->wake_irq) >> + return ret; >> + >> + if (gpio <= 27) >> + irqgroup = 0; >> + else if (gpio >= 28 && gpio <= 45) >> + irqgroup = 1; >> + else if (gpio >= 46 && gpio <= 53) >> + irqgroup = 2; > in case the BCM7211 has 58 GPIOs, but the wake up interrupts are only > available for the first 54 this should deserve a comment. irqgroup 2 covers GPIOs 46 through 57, thanks for noticing. Do you have more comments before I spin a v3? Thank you for reviewing.
Hi Florian, Am 30.05.20 um 23:19 schrieb Florian Fainelli: > > On 5/30/2020 12:49 AM, Stefan Wahren wrote: >> Hi Florian, >> >> Am 29.05.20 um 21:15 schrieb Florian Fainelli: >>> } >>> >>> +static int bcm2835_gpio_irq_set_wake(struct irq_data *data, unsigned int on) >>> +{ >>> + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); >>> + struct bcm2835_pinctrl *pc = gpiochip_get_data(chip); >>> + unsigned gpio = irqd_to_hwirq(data); >>> + unsigned int irqgroup; >>> + int ret = -EINVAL; >>> + >>> + if (!pc->wake_irq) >>> + return ret; >>> + >>> + if (gpio <= 27) >>> + irqgroup = 0; >>> + else if (gpio >= 28 && gpio <= 45) >>> + irqgroup = 1; >>> + else if (gpio >= 46 && gpio <= 53) >>> + irqgroup = 2; >> in case the BCM7211 has 58 GPIOs, but the wake up interrupts are only >> available for the first 54 this should deserve a comment. > irqgroup 2 covers GPIOs 46 through 57, thanks for noticing. Do you have > more comments before I spin a v3? Thank you for reviewing. no, i don't. Regards Stefan
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index 1b00d93aa66e..1fbf067a3eed 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -19,6 +19,7 @@ #include <linux/irq.h> #include <linux/irqdesc.h> #include <linux/init.h> +#include <linux/interrupt.h> #include <linux/of_address.h> #include <linux/of.h> #include <linux/of_irq.h> @@ -76,6 +77,7 @@ struct bcm2835_pinctrl { struct device *dev; void __iomem *base; + int *wake_irq; /* note: locking assumes each bank will have its own unsigned long */ unsigned long enabled_irq_map[BCM2835_NUM_BANKS]; @@ -435,6 +437,11 @@ static void bcm2835_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(host_chip, desc); } +static irqreturn_t bcm2835_gpio_wake_irq_handler(int irq, void *dev_id) +{ + return IRQ_HANDLED; +} + static inline void __bcm2835_gpio_irq_config(struct bcm2835_pinctrl *pc, unsigned reg, unsigned offset, bool enable) { @@ -634,6 +641,34 @@ static void bcm2835_gpio_irq_ack(struct irq_data *data) bcm2835_gpio_set_bit(pc, GPEDS0, gpio); } +static int bcm2835_gpio_irq_set_wake(struct irq_data *data, unsigned int on) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(data); + struct bcm2835_pinctrl *pc = gpiochip_get_data(chip); + unsigned gpio = irqd_to_hwirq(data); + unsigned int irqgroup; + int ret = -EINVAL; + + if (!pc->wake_irq) + return ret; + + if (gpio <= 27) + irqgroup = 0; + else if (gpio >= 28 && gpio <= 45) + irqgroup = 1; + else if (gpio >= 46 && gpio <= 53) + irqgroup = 2; + else + return ret; + + if (on) + ret = enable_irq_wake(pc->wake_irq[irqgroup]); + else + ret = disable_irq_wake(pc->wake_irq[irqgroup]); + + return ret; +} + static struct irq_chip bcm2835_gpio_irq_chip = { .name = MODULE_NAME, .irq_enable = bcm2835_gpio_irq_enable, @@ -642,6 +677,8 @@ static struct irq_chip bcm2835_gpio_irq_chip = { .irq_ack = bcm2835_gpio_irq_ack, .irq_mask = bcm2835_gpio_irq_disable, .irq_unmask = bcm2835_gpio_irq_enable, + .irq_set_wake = bcm2835_gpio_irq_set_wake, + .flags = IRQCHIP_MASK_ON_SUSPEND, }; static int bcm2835_pctl_get_groups_count(struct pinctrl_dev *pctldev) @@ -1154,6 +1191,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) struct resource iomem; int err, i; const struct of_device_id *match; + int is_7211 = 0; BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_pins) != BCM2711_NUM_GPIOS); BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_groups) != BCM2711_NUM_GPIOS); @@ -1180,6 +1218,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) return -EINVAL; pdata = match->data; + is_7211 = of_device_is_compatible(np, "brcm,bcm7211-gpio"); pc->gpio_chip = *pdata->gpio_chip; pc->gpio_chip.parent = dev; @@ -1214,6 +1253,15 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) GFP_KERNEL); if (!girq->parents) return -ENOMEM; + + if (is_7211) { + pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, + sizeof(*pc->wake_irq), + GFP_KERNEL); + if (!pc->wake_irq) + return -ENOMEM; + } + /* * Use the same handler for all groups: this is necessary * since we use one gpiochip to cover all lines - the @@ -1221,8 +1269,34 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) * bank that was firing the IRQ and look up the per-group * and bank data. */ - for (i = 0; i < BCM2835_NUM_IRQS; i++) + for (i = 0; i < BCM2835_NUM_IRQS; i++) { + int len; + char *name; + girq->parents[i] = irq_of_parse_and_map(np, i); + if (!is_7211) + continue; + + /* Skip over the all banks interrupts */ + pc->wake_irq[i] = irq_of_parse_and_map(np, i + + BCM2835_NUM_IRQS + 1); + + len = strlen(dev_name(pc->dev)) + 16; + name = devm_kzalloc(pc->dev, len, GFP_KERNEL); + if (!name) + return -ENOMEM; + + snprintf(name, len, "%s:bank%d", dev_name(pc->dev), i); + + /* These are optional interrupts */ + err = devm_request_irq(dev, pc->wake_irq[i], + bcm2835_gpio_wake_irq_handler, + IRQF_SHARED, name, pc); + if (err) + dev_warn(dev, "unable to request wake IRQ %d\n", + pc->wake_irq[i]); + } + girq->default_type = IRQ_TYPE_NONE; girq->handler = handle_level_irq;
Leverage the IRQCHIP_MASK_ON_SUSPEND flag in order to avoid having to specifically treat the GPIO interrupts during suspend and resume, and simply implement an irq_set_wake() callback that is responsible for enabling the parent wake-up interrupt as a wake-up interrupt. To avoid allocating unnecessary resources for other chips, the wake-up interrupts are only initialized if we have a brcm,bcm7211-gpio compatibility string. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 76 ++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-)