On 10/19/2017 02:03 AM, Gregory Fong wrote:
> Hi Doug,
>
> Nice description of the problem with dealing with a pending disabled
> wake interrupt and the solution. A few remarks:
>
> On Fri, Sep 29, 2017 at 08:40:57PM -0700, Doug Berger wrote:
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index 752a46ce3589..c964ed71a68d 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> @@ -19,17 +19,29 @@
>> #include <linux/irqdomain.h>
>> #include <linux/irqchip/chained_irq.h>
>> #include <linux/interrupt.h>
>> -#include <linux/reboot.h>
>> -
>> -#define GIO_BANK_SIZE 0x20
>> -#define GIO_ODEN(bank) (((bank) * GIO_BANK_SIZE) + 0x00)
>> -#define GIO_DATA(bank) (((bank) * GIO_BANK_SIZE) + 0x04)
>> -#define GIO_IODIR(bank) (((bank) * GIO_BANK_SIZE) + 0x08)
>> -#define GIO_EC(bank) (((bank) * GIO_BANK_SIZE) + 0x0c)
>> -#define GIO_EI(bank) (((bank) * GIO_BANK_SIZE) + 0x10)
>> -#define GIO_MASK(bank) (((bank) * GIO_BANK_SIZE) + 0x14)
>> -#define GIO_LEVEL(bank) (((bank) * GIO_BANK_SIZE) + 0x18)
>> -#define GIO_STAT(bank) (((bank) * GIO_BANK_SIZE) + 0x1c)
>> +
>> +enum gio_reg_index {
>> + GIO_REG_ODEN = 0,
>> + GIO_REG_DATA,
>> + GIO_REG_IODIR,
>> + GIO_REG_EC,
>> + GIO_REG_EI,
>> + GIO_REG_MASK,
>> + GIO_REG_LEVEL,
>> + GIO_REG_STAT,
>> + GIO_REG_COUNT
>
> Please change the name or add a comment to make it clear that this is
> not for an actual register.
>
Will do.
>> +};
>> +
>> [snip]
>> @@ -37,6 +49,8 @@ struct brcmstb_gpio_bank {
>> struct gpio_chip gc;
>> struct brcmstb_gpio_priv *parent_priv;
>> u32 width;
>> + u32 wake_active;
>> + u32 regs[GIO_REG_STAT]; /* Don't save and restore GIO_REG_STAT */
>
> Consider naming to make it clearer that this is for save/restore, not
> some kind of shadow reg implementation or anything like that.
>
Will do.
>> };
>>
>> struct brcmstb_gpio_priv {
>> @@ -49,7 +63,6 @@ struct brcmstb_gpio_priv {
>> int gpio_base;
>> int num_gpios;
>> int parent_wake_irq;
>> - struct notifier_block reboot_notifier;
>> };
>>
>> #define MAX_GPIO_PER_BANK 32
>> @@ -65,15 +78,22 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
>> }
>>
>> static unsigned long
>> -brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>> +__brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>> {
>> void __iomem *reg_base = bank->parent_priv->reg_base;
>> +
>> + return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
>> + bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
>> +}
>> +
>> +static unsigned long
>> +brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>> +{
>> unsigned long status;
>> unsigned long flags;
>>
>> spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
>> - status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
>> - bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
>> + status = __brcmstb_gpio_get_active_irqs(bank);
>> spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
>>
>> return status;
>> @@ -209,11 +229,6 @@ static int brcmstb_gpio_priv_set_wake(struct
>> brcmstb_gpio_priv *priv,
>> {
>> int ret = 0;
>>
>> - /*
>> - * Only enable wake IRQ once for however many hwirqs can wake
>> - * since they all use the same wake IRQ. Mask will be set
>> - * up appropriately thanks to IRQCHIP_MASK_ON_SUSPEND flag.
>> - */
>> if (enable)
>> ret = enable_irq_wake(priv->parent_wake_irq);
>> else
>> @@ -227,7 +242,17 @@ static int brcmstb_gpio_priv_set_wake(struct
>> brcmstb_gpio_priv *priv,
>> static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int
>> enable)
>> {
>> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> - struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
>> + struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
>> + struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> + u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
>> +
>> + /* Do not do anything specific for now, suspend/resume callbacks will
>> + * configure the interrupt mask appropriately
>> + */
>
> Please fix comment format (this is the network subsystem style).
>
Thanks, force of habit ;).
>>
>> [snip]
>> @@ -508,6 +511,99 @@ static int brcmstb_gpio_irq_setup(struct
>> platform_device *pdev,
>> [...]
>> +static void brcmstb_gpio_shutdown(struct platform_device *pdev)
>> +{
>> + /* Enable GPIO for S5 cold boot */
>> + brcmstb_gpio_quiesce(&pdev->dev, 0);
>
> nit: use false instead of 0 (it's a bool).
>
Will do.
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> [snip]
>> +static int brcmstb_gpio_resume(struct device *dev)
>> +{
>> + struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
>> + struct brcmstb_gpio_bank *bank;
>> + u32 wake_mask = 0;
>
> This isn't really being used as a mask, contrary to appearances. It's
> just tracking whether any active IRQs were seen. Please change to use a
> bool instead and adjust the name accordingly.
>
I see your point, but I believe it is cleaner to use this to consolidate
the bit masks returned by each __brcmstb_gpio_get_active_irqs() call.
This allows a single test rather than a test per bank.
>> +
>> + list_for_each_entry(bank, &priv->bank_list, node) {
>> + wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
>> + brcmstb_gpio_bank_restore(priv, bank);
>> + }
>> +
>> + if (priv->parent_wake_irq && wake_mask)
>> + pm_wakeup_event(dev, 0);
>> +
>> + /* enable non-wake interrupt */
>> + if (priv->parent_irq >= 0)
>> + enable_irq(priv->parent_irq);
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +#define brcmstb_gpio_suspend NULL
>> +#define brcmstb_gpio_resume NULL
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
>> + .suspend_noirq = brcmstb_gpio_suspend,
>> + .resume_noirq = brcmstb_gpio_resume,
>> +};
>> +
>> static int brcmstb_gpio_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -517,7 +613,7 @@ static int brcmstb_gpio_probe(struct platform_device
>> *pdev)
>> struct resource *res;
>> struct property *prop;
>> const __be32 *p;
>> - u32 bank_width;
>> + u32 bank_width, wake_mask = 0;
>
> Same comment on wake_mask as for brcmstb_gpio_resume() above.
>
Same response.
>> int num_banks = 0;
>> int err;
>> static int gpio_base;
>> @@ -617,6 +713,7 @@ static int brcmstb_gpio_probe(struct platform_device
>> *pdev)
>> * Mask all interrupts by default, since wakeup interrupts may
>> * be retained from S5 cold boot
>> */
>> + wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
>> gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
>>
>> err = gpiochip_add_data(gc, bank);
>> @@ -646,6 +743,9 @@ static int brcmstb_gpio_probe(struct platform_device
>> *pdev)
>> dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
>> num_banks, priv->gpio_base, gpio_base - 1);
>>
>> + if (priv->parent_wake_irq && wake_mask)
>> + pm_wakeup_event(dev, 0);
>
> Why is this called in probe?
>
This allows proper wakeup accounting when waking from S5.
> Thanks,
> Gregory
>
Thanks again for the review. I will try to get a v2 out next week.
Doug
