* [PATCH 0/2] Pinctrl fixes for rockchip @ 2014-11-18 23:49 Doug Anderson 2014-11-18 23:49 ` [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins Doug Anderson 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson 0 siblings, 2 replies; 10+ messages in thread From: Doug Anderson @ 2014-11-18 23:49 UTC (permalink / raw) To: Heiko Stuebner, Linus Walleij Cc: Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, linux-rockchip, Doug Anderson, linux-gpio, linux-kernel These two patches fix some pinctrl issues on rockchip. The first fixes a real issue where interrupts were being left on in suspend/resume and waking the system up when they shouldn't. The second fixes purely theoretical problems and could be considered more of an RFC patch. I've tested these patches more extensively on a non-mainline tree but have done basic compile and boot testing on mainline. I don't have suspend/resume working so well on mainline yet, so I haven't tested fully there. Doug Anderson (2): pinctrl: rockchip: Handle wakeup pins pinctrl: rockchip: Fix enable/disable/mask/unmask drivers/pinctrl/pinctrl-rockchip.c | 51 +++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-) -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins 2014-11-18 23:49 [PATCH 0/2] Pinctrl fixes for rockchip Doug Anderson @ 2014-11-18 23:49 ` Doug Anderson 2014-11-19 17:55 ` Dmitry Torokhov 2014-11-19 18:36 ` Heiko Stübner 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson 1 sibling, 2 replies; 10+ messages in thread From: Doug Anderson @ 2014-11-18 23:49 UTC (permalink / raw) To: Heiko Stuebner, Linus Walleij Cc: Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, linux-rockchip, Doug Anderson, linux-gpio, linux-kernel The rockchip pinctrl driver was using irq_gc_set_wake() as its implementation of irq_set_wake() but was totally ignoring everything that irq_gc_set_wake() did (which is to upkeep gc->wake_active). Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend time and restoring GPIO_INTEN at resume time. NOTE a few quirks when thinking about this patch: - Rockchip pinctrl hardware supports both "disable/enable" and "mask/unmask". Right now we only use "disable/enable" and present those to Linux as "mask/unmask". This should be OK because enable/disable is optional and Linux will implement it in terms of mask/unmask. At the moment we always tell hardware all interrupts are unmasked (the boot default). - At suspend time Linux tries to call "disable" on all interrupts and also enables wakeup on all wakeup interrupts. One would think that since "disable" is implemented as "mask" when "disable" isn't provided and that since we were ignoring gc->wake_active that nothing would have woken us up. That's not the case since Linux "optimizes" things and just leaves interrutps unmasked, assuming it could mask them later when they go off. That meant that at suspend time all interrupts were actually being left enabled. With this patch random non-wakeup interrupts no longer wake the system up. Wakeup interrupts still wake the system up. Signed-off-by: Doug Anderson <dianders@chromium.org> --- drivers/pinctrl/pinctrl-rockchip.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index ba74f0a..e91e845 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -89,6 +89,7 @@ struct rockchip_iomux { * @reg_pull: optional separate register for additional pull settings * @clk: clock of the gpio bank * @irq: interrupt of the gpio bank + * @saved_enables: Saved content of GPIO_INTEN at suspend time. * @pin_base: first pin number * @nr_pins: number of pins in this bank * @name: name of the bank @@ -107,6 +108,7 @@ struct rockchip_pin_bank { struct regmap *regmap_pull; struct clk *clk; int irq; + u32 saved_enables; u32 pin_base; u8 nr_pins; char *name; @@ -1543,6 +1545,23 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) return 0; } +static void rockchip_irq_suspend(struct irq_data *d) +{ + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct rockchip_pin_bank *bank = gc->private; + + bank->saved_enables = irq_reg_readl(gc, GPIO_INTEN); + irq_reg_writel(gc, gc->wake_active, GPIO_INTEN); +} + +static void rockchip_irq_resume(struct irq_data *d) +{ + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct rockchip_pin_bank *bank = gc->private; + + irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); +} + static int rockchip_interrupts_register(struct platform_device *pdev, struct rockchip_pinctrl *info) { @@ -1587,6 +1606,8 @@ static int rockchip_interrupts_register(struct platform_device *pdev, gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; + gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; + gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type; gc->wake_enabled = IRQ_MSK(bank->nr_pins); -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins 2014-11-18 23:49 ` [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins Doug Anderson @ 2014-11-19 17:55 ` Dmitry Torokhov 2014-11-19 18:36 ` Heiko Stübner 1 sibling, 0 replies; 10+ messages in thread From: Dmitry Torokhov @ 2014-11-19 17:55 UTC (permalink / raw) To: Doug Anderson Cc: Heiko Stuebner, Linus Walleij, Sonny Rao, Chris Zhong, linux-arm-kernel, linux-rockchip, linux-gpio, linux-kernel On Tue, Nov 18, 2014 at 03:49:55PM -0800, Doug Anderson wrote: > The rockchip pinctrl driver was using irq_gc_set_wake() as its > implementation of irq_set_wake() but was totally ignoring everything > that irq_gc_set_wake() did (which is to upkeep gc->wake_active). > > Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend > time and restoring GPIO_INTEN at resume time. > > NOTE a few quirks when thinking about this patch: > - Rockchip pinctrl hardware supports both "disable/enable" and > "mask/unmask". Right now we only use "disable/enable" and present > those to Linux as "mask/unmask". This should be OK because > enable/disable is optional and Linux will implement it in terms of > mask/unmask. At the moment we always tell hardware all interrupts > are unmasked (the boot default). > - At suspend time Linux tries to call "disable" on all interrupts and > also enables wakeup on all wakeup interrupts. One would think that > since "disable" is implemented as "mask" when "disable" isn't > provided and that since we were ignoring gc->wake_active that > nothing would have woken us up. That's not the case since Linux > "optimizes" things and just leaves interrutps unmasked, assuming it > could mask them later when they go off. That meant that at suspend > time all interrupts were actually being left enabled. > > With this patch random non-wakeup interrupts no longer wake the system > up. Wakeup interrupts still wake the system up. > > Signed-off-by: Doug Anderson <dianders@chromium.org> Seems reasonable to me. Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/pinctrl/pinctrl-rockchip.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > index ba74f0a..e91e845 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -89,6 +89,7 @@ struct rockchip_iomux { > * @reg_pull: optional separate register for additional pull settings > * @clk: clock of the gpio bank > * @irq: interrupt of the gpio bank > + * @saved_enables: Saved content of GPIO_INTEN at suspend time. > * @pin_base: first pin number > * @nr_pins: number of pins in this bank > * @name: name of the bank > @@ -107,6 +108,7 @@ struct rockchip_pin_bank { > struct regmap *regmap_pull; > struct clk *clk; > int irq; > + u32 saved_enables; > u32 pin_base; > u8 nr_pins; > char *name; > @@ -1543,6 +1545,23 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) > return 0; > } > > +static void rockchip_irq_suspend(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct rockchip_pin_bank *bank = gc->private; > + > + bank->saved_enables = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, gc->wake_active, GPIO_INTEN); > +} > + > +static void rockchip_irq_resume(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct rockchip_pin_bank *bank = gc->private; > + > + irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); > +} > + > static int rockchip_interrupts_register(struct platform_device *pdev, > struct rockchip_pinctrl *info) > { > @@ -1587,6 +1606,8 @@ static int rockchip_interrupts_register(struct platform_device *pdev, > gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; > gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; > gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; > + gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > + gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; > gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type; > gc->wake_enabled = IRQ_MSK(bank->nr_pins); > > -- > 2.1.0.rc2.206.gedb03e5 > -- Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins 2014-11-18 23:49 ` [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins Doug Anderson 2014-11-19 17:55 ` Dmitry Torokhov @ 2014-11-19 18:36 ` Heiko Stübner 1 sibling, 0 replies; 10+ messages in thread From: Heiko Stübner @ 2014-11-19 18:36 UTC (permalink / raw) To: Doug Anderson Cc: Linus Walleij, Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, linux-rockchip, linux-gpio, linux-kernel Am Dienstag, 18. November 2014, 15:49:55 schrieb Doug Anderson: > The rockchip pinctrl driver was using irq_gc_set_wake() as its > implementation of irq_set_wake() but was totally ignoring everything > that irq_gc_set_wake() did (which is to upkeep gc->wake_active). > > Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend > time and restoring GPIO_INTEN at resume time. > > NOTE a few quirks when thinking about this patch: > - Rockchip pinctrl hardware supports both "disable/enable" and > "mask/unmask". Right now we only use "disable/enable" and present > those to Linux as "mask/unmask". This should be OK because > enable/disable is optional and Linux will implement it in terms of > mask/unmask. At the moment we always tell hardware all interrupts > are unmasked (the boot default). > - At suspend time Linux tries to call "disable" on all interrupts and > also enables wakeup on all wakeup interrupts. One would think that > since "disable" is implemented as "mask" when "disable" isn't > provided and that since we were ignoring gc->wake_active that > nothing would have woken us up. That's not the case since Linux > "optimizes" things and just leaves interrutps unmasked, assuming it > could mask them later when they go off. That meant that at suspend > time all interrupts were actually being left enabled. > > With this patch random non-wakeup interrupts no longer wake the system > up. Wakeup interrupts still wake the system up. > > Signed-off-by: Doug Anderson <dianders@chromium.org> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/pinctrl/pinctrl-rockchip.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c > b/drivers/pinctrl/pinctrl-rockchip.c index ba74f0a..e91e845 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -89,6 +89,7 @@ struct rockchip_iomux { > * @reg_pull: optional separate register for additional pull settings > * @clk: clock of the gpio bank > * @irq: interrupt of the gpio bank > + * @saved_enables: Saved content of GPIO_INTEN at suspend time. > * @pin_base: first pin number > * @nr_pins: number of pins in this bank > * @name: name of the bank > @@ -107,6 +108,7 @@ struct rockchip_pin_bank { > struct regmap *regmap_pull; > struct clk *clk; > int irq; > + u32 saved_enables; > u32 pin_base; > u8 nr_pins; > char *name; > @@ -1543,6 +1545,23 @@ static int rockchip_irq_set_type(struct irq_data *d, > unsigned int type) return 0; > } > > +static void rockchip_irq_suspend(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct rockchip_pin_bank *bank = gc->private; > + > + bank->saved_enables = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, gc->wake_active, GPIO_INTEN); > +} > + > +static void rockchip_irq_resume(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct rockchip_pin_bank *bank = gc->private; > + > + irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); > +} > + > static int rockchip_interrupts_register(struct platform_device *pdev, > struct rockchip_pinctrl *info) > { > @@ -1587,6 +1606,8 @@ static int rockchip_interrupts_register(struct > platform_device *pdev, gc->chip_types[0].chip.irq_mask = > irq_gc_mask_clr_bit; > gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; > gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; > + gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > + gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; > gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type; > gc->wake_enabled = IRQ_MSK(bank->nr_pins); ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-18 23:49 [PATCH 0/2] Pinctrl fixes for rockchip Doug Anderson 2014-11-18 23:49 ` [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins Doug Anderson @ 2014-11-18 23:49 ` Doug Anderson 2014-11-19 17:54 ` Doug Anderson ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Doug Anderson @ 2014-11-18 23:49 UTC (permalink / raw) To: Heiko Stuebner, Linus Walleij Cc: Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, linux-rockchip, Doug Anderson, linux-gpio, linux-kernel The Rockchip pinctrl driver was only implementing the "mask" and "unmask" operations though the hardware actually has two distinct things: enable/disable and mask/unmask. It was implementing the "mask" operations as a hardware enable/disable and always leaving all interrupts unmasked. I believe that the old system had some downsides, specifically: - (Untested) if an interrupt went off while interrupts were "masked" it would be lost. Now it will be kept track of. - If someone wanted to change an interrupt back into a GPIO (is such a thing sensible?) by calling irq_disable() it wouldn't actually take effect. That's because Linux does some extra optimizations when there's no true "disable" function: it does a lazy mask. Let's actually implement enable/disable/mask/unmask properly. Signed-off-by: Doug Anderson <dianders@chromium.org> --- drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index e91e845..60d1a49 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -1562,6 +1562,28 @@ static void rockchip_irq_resume(struct irq_data *d) irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); } +static void rockchip_irq_disable(struct irq_data *d) +{ + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + u32 val; + + irq_gc_lock(gc); + val = irq_reg_readl(gc, GPIO_INTEN); + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); + irq_gc_unlock(gc); +} + +static void rockchip_irq_enable(struct irq_data *d) +{ + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + u32 val; + + irq_gc_lock(gc); + val = irq_reg_readl(gc, GPIO_INTEN); + irq_reg_writel(gc, val | d->mask, GPIO_INTEN); + irq_gc_unlock(gc); +} + static int rockchip_interrupts_register(struct platform_device *pdev, struct rockchip_pinctrl *info) { @@ -1600,11 +1622,13 @@ static int rockchip_interrupts_register(struct platform_device *pdev, gc = irq_get_domain_generic_chip(bank->domain, 0); gc->reg_base = bank->reg_base; gc->private = bank; - gc->chip_types[0].regs.mask = GPIO_INTEN; + gc->chip_types[0].regs.mask = GPIO_INTMASK; gc->chip_types[0].regs.ack = GPIO_PORTS_EOI; gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit; - gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; - gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; + gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; + gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; + gc->chip_types[0].chip.irq_enable = rockchip_irq_enable; + gc->chip_types[0].chip.irq_disable = rockchip_irq_disable; gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson @ 2014-11-19 17:54 ` Doug Anderson 2014-11-19 18:46 ` Heiko Stübner 2014-11-19 17:56 ` Dmitry Torokhov 2014-11-19 19:17 ` Heiko Stübner 2 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2014-11-19 17:54 UTC (permalink / raw) To: Heiko Stuebner, Linus Walleij Cc: Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, open list:ARM/Rockchip SoC..., Doug Anderson, linux-gpio, linux-kernel Hi, On Tue, Nov 18, 2014 at 3:49 PM, Doug Anderson <dianders@chromium.org> wrote: > +static void rockchip_irq_disable(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 val; > + > + irq_gc_lock(gc); > + val = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); > + irq_gc_unlock(gc); > +} Off list, Dmitry asked me why I didn't use irq_gc_mask_disable_reg() and irq_gc_unmask_enable_reg() (AKA why I coded up my own function here). Originally I tried to use irq_gc_mask_disable_reg() and irq_gc_unmask_enable_reg(). ..but they're really not designed to work in tandem with the irq_gc_mask_set_bit() and irq_gc_mask_clr_bit(). Specifically if you try to use one set of functions for your mask/unmask and the other for your disable/enable you'll find that they stomp on each other. Both functions upkeep the exact same "mask_cache" variable. Personally I'm totally baffled by how irq_gc_mask_disable_reg() and irq_gc_unmask_enable_reg() could actually be sane, but that's maybe a topic for another discussion. I say that they don't seem sane because (it seems to me) that if we have a separate "enable" and "disable" register in hardware that you'd want to write "1"s to both of them and also possibly not even have a cache. The current irq_gc_mask_disable_reg() doesn't do this. I'm imagining something like I think I've seen on STM32 where you're got a 3 registers for everything: the real register, a "write 1 to set", and a "write 1 to clear". -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-19 17:54 ` Doug Anderson @ 2014-11-19 18:46 ` Heiko Stübner 2014-11-19 18:48 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Heiko Stübner @ 2014-11-19 18:46 UTC (permalink / raw) To: Doug Anderson Cc: Linus Walleij, Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, open list:ARM/Rockchip SoC..., linux-gpio, linux-kernel Am Mittwoch, 19. November 2014, 09:54:13 schrieb Doug Anderson: > Hi, > > On Tue, Nov 18, 2014 at 3:49 PM, Doug Anderson <dianders@chromium.org> wrote: > > +static void rockchip_irq_disable(struct irq_data *d) > > +{ > > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > + u32 val; > > + > > + irq_gc_lock(gc); > > + val = irq_reg_readl(gc, GPIO_INTEN); > > + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); > > + irq_gc_unlock(gc); > > +} > > Off list, Dmitry asked me why I didn't use irq_gc_mask_disable_reg() > and irq_gc_unmask_enable_reg() (AKA why I coded up my own function > here). Originally I tried to use irq_gc_mask_disable_reg() and > irq_gc_unmask_enable_reg(). ..but they're really not designed to work > in tandem with the irq_gc_mask_set_bit() and irq_gc_mask_clr_bit(). > > Specifically if you try to use one set of functions for your > mask/unmask and the other for your disable/enable you'll find that > they stomp on each other. Both functions upkeep the exact same > "mask_cache" variable. > > Personally I'm totally baffled by how irq_gc_mask_disable_reg() and > irq_gc_unmask_enable_reg() could actually be sane, but that's maybe a > topic for another discussion. I don't think irq_gc_mask_disable_reg and irq_gc_unmask_enable_reg are meant as callbacks for irq_enable/irq_disable. As the name implies they are standardized callbacks for irq_mask/irq_unmask on machines using a different scheme for masking. So I would expected that they operate on the same mask_cache because both types of functions handle masking on different types of interrupt controllers. There don't seem to be any generalized callbacks for irq_enable/irq_disable themself, probably because machines do the most uncommon things there :-) Heiko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-19 18:46 ` Heiko Stübner @ 2014-11-19 18:48 ` Doug Anderson 0 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2014-11-19 18:48 UTC (permalink / raw) To: Heiko Stübner Cc: Linus Walleij, Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, open list:ARM/Rockchip SoC..., linux-gpio, linux-kernel Heiko, On Wed, Nov 19, 2014 at 10:46 AM, Heiko Stübner <heiko@sntech.de> wrote: > Am Mittwoch, 19. November 2014, 09:54:13 schrieb Doug Anderson: >> Hi, >> >> On Tue, Nov 18, 2014 at 3:49 PM, Doug Anderson <dianders@chromium.org> > wrote: >> > +static void rockchip_irq_disable(struct irq_data *d) >> > +{ >> > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); >> > + u32 val; >> > + >> > + irq_gc_lock(gc); >> > + val = irq_reg_readl(gc, GPIO_INTEN); >> > + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); >> > + irq_gc_unlock(gc); >> > +} >> >> Off list, Dmitry asked me why I didn't use irq_gc_mask_disable_reg() >> and irq_gc_unmask_enable_reg() (AKA why I coded up my own function >> here). Originally I tried to use irq_gc_mask_disable_reg() and >> irq_gc_unmask_enable_reg(). ..but they're really not designed to work >> in tandem with the irq_gc_mask_set_bit() and irq_gc_mask_clr_bit(). >> >> Specifically if you try to use one set of functions for your >> mask/unmask and the other for your disable/enable you'll find that >> they stomp on each other. Both functions upkeep the exact same >> "mask_cache" variable. >> >> Personally I'm totally baffled by how irq_gc_mask_disable_reg() and >> irq_gc_unmask_enable_reg() could actually be sane, but that's maybe a >> topic for another discussion. > > I don't think irq_gc_mask_disable_reg and irq_gc_unmask_enable_reg are meant > as callbacks for irq_enable/irq_disable. As the name implies they are > standardized callbacks for irq_mask/irq_unmask on machines using a different > scheme for masking. So I would expected that they operate on the same > mask_cache because both types of functions handle masking on different types of > interrupt controllers. Agreed that they aren't meant for irq_enable / irq_disable and that it's not a bug. It was just so tempting to use them and Dmitry wondered the same thing, so I wrote an email detailing this. Even though these aren't for use as enable/disable, I will point out that they don't seem sane (to me) for masking... > There don't seem to be any generalized callbacks for irq_enable/irq_disable > themself, probably because machines do the most uncommon things there :-) Fair enough. If it becomes common someone can move my functions somewhere common. ;) Do you think this patch is something that should land? Do I need to prove that it's useful before we actually land it? Right now I just posted it because it seemed better, not because it fixes anything that I know of. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson 2014-11-19 17:54 ` Doug Anderson @ 2014-11-19 17:56 ` Dmitry Torokhov 2014-11-19 19:17 ` Heiko Stübner 2 siblings, 0 replies; 10+ messages in thread From: Dmitry Torokhov @ 2014-11-19 17:56 UTC (permalink / raw) To: Doug Anderson Cc: Heiko Stuebner, Linus Walleij, Sonny Rao, Chris Zhong, linux-arm-kernel, linux-rockchip, linux-gpio, linux-kernel On Tue, Nov 18, 2014 at 03:49:56PM -0800, Doug Anderson wrote: > The Rockchip pinctrl driver was only implementing the "mask" and > "unmask" operations though the hardware actually has two distinct > things: enable/disable and mask/unmask. It was implementing the > "mask" operations as a hardware enable/disable and always leaving all > interrupts unmasked. > > I believe that the old system had some downsides, specifically: > - (Untested) if an interrupt went off while interrupts were "masked" > it would be lost. Now it will be kept track of. > - If someone wanted to change an interrupt back into a GPIO (is such a > thing sensible?) by calling irq_disable() it wouldn't actually take > effect. That's because Linux does some extra optimizations when > there's no true "disable" function: it does a lazy mask. > > Let's actually implement enable/disable/mask/unmask properly. > > Signed-off-by: Doug Anderson <dianders@chromium.org> After chatting a bit with Doug offline: Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > index e91e845..60d1a49 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -1562,6 +1562,28 @@ static void rockchip_irq_resume(struct irq_data *d) > irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); > } > > +static void rockchip_irq_disable(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 val; > + > + irq_gc_lock(gc); > + val = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); > + irq_gc_unlock(gc); > +} > + > +static void rockchip_irq_enable(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 val; > + > + irq_gc_lock(gc); > + val = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, val | d->mask, GPIO_INTEN); > + irq_gc_unlock(gc); > +} > + > static int rockchip_interrupts_register(struct platform_device *pdev, > struct rockchip_pinctrl *info) > { > @@ -1600,11 +1622,13 @@ static int rockchip_interrupts_register(struct platform_device *pdev, > gc = irq_get_domain_generic_chip(bank->domain, 0); > gc->reg_base = bank->reg_base; > gc->private = bank; > - gc->chip_types[0].regs.mask = GPIO_INTEN; > + gc->chip_types[0].regs.mask = GPIO_INTMASK; > gc->chip_types[0].regs.ack = GPIO_PORTS_EOI; > gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit; > - gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; > - gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; > + gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; > + gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; > + gc->chip_types[0].chip.irq_enable = rockchip_irq_enable; > + gc->chip_types[0].chip.irq_disable = rockchip_irq_disable; > gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; > gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; > -- > 2.1.0.rc2.206.gedb03e5 > -- Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson 2014-11-19 17:54 ` Doug Anderson 2014-11-19 17:56 ` Dmitry Torokhov @ 2014-11-19 19:17 ` Heiko Stübner 2 siblings, 0 replies; 10+ messages in thread From: Heiko Stübner @ 2014-11-19 19:17 UTC (permalink / raw) To: Doug Anderson Cc: Linus Walleij, Sonny Rao, Dmitry Torokhov, Chris Zhong, linux-arm-kernel, linux-rockchip, linux-gpio, linux-kernel Am Dienstag, 18. November 2014, 15:49:56 schrieb Doug Anderson: > The Rockchip pinctrl driver was only implementing the "mask" and > "unmask" operations though the hardware actually has two distinct > things: enable/disable and mask/unmask. It was implementing the > "mask" operations as a hardware enable/disable and always leaving all > interrupts unmasked. > > I believe that the old system had some downsides, specifically: > - (Untested) if an interrupt went off while interrupts were "masked" > it would be lost. Now it will be kept track of. > - If someone wanted to change an interrupt back into a GPIO (is such a > thing sensible?) by calling irq_disable() it wouldn't actually take > effect. That's because Linux does some extra optimizations when > there's no true "disable" function: it does a lazy mask. > > Let's actually implement enable/disable/mask/unmask properly. > > Signed-off-by: Doug Anderson <dianders@chromium.org> There is one small issue concerning a personal style-preference below. Otherwise Reviewed-by: Heiko Stuebner <heiko@sntech.de> @LinusW: any preference on how to handle these follow up changes - like another pull on top of the last, or do you simply want to apply these two yourself? > --- > drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c > b/drivers/pinctrl/pinctrl-rockchip.c index e91e845..60d1a49 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -1562,6 +1562,28 @@ static void rockchip_irq_resume(struct irq_data *d) > irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN); > } > > +static void rockchip_irq_disable(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 val; > + > + irq_gc_lock(gc); > + val = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, val & ~d->mask, GPIO_INTEN); personally I'd prefer this to be a bit more spread out, i.e. val = irq_reg_readl(gc, GPIO_INTEN); val &= ~d->mask; irq_reg_writel(gc, val, GPIO_INTEN); makes reading a bit easier > + irq_gc_unlock(gc); > +} > + > +static void rockchip_irq_enable(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 val; > + > + irq_gc_lock(gc); > + val = irq_reg_readl(gc, GPIO_INTEN); > + irq_reg_writel(gc, val | d->mask, GPIO_INTEN); same here > + irq_gc_unlock(gc); > +} > + > static int rockchip_interrupts_register(struct platform_device *pdev, > struct rockchip_pinctrl *info) > { > @@ -1600,11 +1622,13 @@ static int rockchip_interrupts_register(struct > platform_device *pdev, gc = irq_get_domain_generic_chip(bank->domain, 0); > gc->reg_base = bank->reg_base; > gc->private = bank; > - gc->chip_types[0].regs.mask = GPIO_INTEN; > + gc->chip_types[0].regs.mask = GPIO_INTMASK; > gc->chip_types[0].regs.ack = GPIO_PORTS_EOI; > gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit; > - gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; > - gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; > + gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; > + gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; > + gc->chip_types[0].chip.irq_enable = rockchip_irq_enable; > + gc->chip_types[0].chip.irq_disable = rockchip_irq_disable; > gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; > gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-11-19 19:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-18 23:49 [PATCH 0/2] Pinctrl fixes for rockchip Doug Anderson 2014-11-18 23:49 ` [PATCH 1/2] pinctrl: rockchip: Handle wakeup pins Doug Anderson 2014-11-19 17:55 ` Dmitry Torokhov 2014-11-19 18:36 ` Heiko Stübner 2014-11-18 23:49 ` [PATCH 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask Doug Anderson 2014-11-19 17:54 ` Doug Anderson 2014-11-19 18:46 ` Heiko Stübner 2014-11-19 18:48 ` Doug Anderson 2014-11-19 17:56 ` Dmitry Torokhov 2014-11-19 19:17 ` Heiko Stübner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).