[2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask
diff mbox

Message ID 1416354596-15013-3-git-send-email-dianders@chromium.org
State New, archived
Headers show

Commit Message

Doug Anderson Nov. 18, 2014, 11:49 p.m. UTC
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(-)

Comments

Doug Anderson Nov. 19, 2014, 5:54 p.m. UTC | #1
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
'Dmitry Torokhov' Nov. 19, 2014, 5:56 p.m. UTC | #2
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
>
Heiko Stuebner Nov. 19, 2014, 6:46 p.m. UTC | #3
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
Doug Anderson Nov. 19, 2014, 6:48 p.m. UTC | #4
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
Heiko Stuebner Nov. 19, 2014, 7:17 p.m. UTC | #5
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;

Patch
diff mbox

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;