| Message ID | 1416354596-15013-3-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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;
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;
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(-)