| Message ID | 20200529191522.27938-1-f.fainelli@gmail.com |
|---|---|
| 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
Hi Linus, This patch series updates the bcm2835 pinctrl driver to support the BCM7211 SoC which is quite similar to 2711 (Raspberry Pi 4) except that it also supports wake-up interrupts. Thanks! Changes in v2: - fixed patch #3 to reference the correct data structure (Stefan) - fixed patch #4 to use conditional initialization and fetching of interrupt resources to limit the memory overhead for non-7211 chips Florian Fainelli (4): dt-bindings: pinctrl: Document 7211 compatible for brcm,bcm2835-gpio.txt dt-bindings: pinctrl: Document optional BCM7211 wake-up interrupts pinctrl: bcm2835: Match BCM7211 compatible string pinctrl: bcm2835: Add support for wake-up interrupts .../bindings/pinctrl/brcm,bcm2835-gpio.txt | 5 +- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 80 ++++++++++++++++++- 2 files changed, 83 insertions(+), 2 deletions(-)