Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683391 > unrolled thread

[PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions Doug Berger <opendmb@gmail.com> - 2017-07-07 21:30 +0200
    Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack  functions Thomas Gleixner <tglx@linutronix.de> - 2017-07-08 14:10 +0200
      Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack  functions Doug Berger <opendmb@gmail.com> - 2017-07-10 19:40 +0200
        Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack  functions Doug Berger <opendmb@gmail.com> - 2017-07-17 19:30 +0200
          Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack  functions Thomas Gleixner <tglx@linutronix.de> - 2017-07-17 23:00 +0200
    Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack  functions Doug Berger <opendmb@gmail.com> - 2017-07-12 21:30 +0200
      Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions Måns Rullgård <mans@mansr.com> - 2017-07-12 23:00 +0200

#1683391 — [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

The irq_gc_mask_disable_reg_and_ack() function name implies that it
provides the combined functions of irq_gc_mask_disable_reg() and
irq_gc_ack().  However, the implementation does not actually do
that since it writes the mask instead of the disable register. It
also does not maintain the mask cache which makes it inappropriate
to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
{set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
irq_gc_set_bit() so this function probably should have also been
renamed at that time.

Since this generic chip code provides three mask functions and two
ack functions, this commit provides generic implementations for all
six combinations of the mask and ack functions suitable for use
with the irq_mask_ack member of the struct irq_chip.

The '_reg' and '_bit' portions of the base function names were left
out of the new combined function names in an attempt to keep the
function name lengths manageable with the 80 character source code
line length while still capturing the distinct aspects of each
combination of functions.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 include/linux/irq.h       |   6 +++
 kernel/irq/generic-chip.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 00db35b61e9e..23b9617bb682 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1003,6 +1003,12 @@ void irq_gc_unmask_enable_reg(struct irq_data *d);
 void irq_gc_ack_set_bit(struct irq_data *d);
 void irq_gc_ack_clr_bit(struct irq_data *d);
 void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
+void irq_gc_mask_disable_and_ack_set(struct irq_data *d);
+void irq_gc_mask_disable_and_ack_clr(struct irq_data *d);
+void irq_gc_mask_set_and_ack_set(struct irq_data *d);
+void irq_gc_mask_set_and_ack_clr(struct irq_data *d);
+void irq_gc_mask_clr_and_ack_set(struct irq_data *d);
+void irq_gc_mask_clr_and_ack_clr(struct irq_data *d);
 void irq_gc_eoi(struct irq_data *d);
 int irq_gc_set_wake(struct irq_data *d, unsigned int on);
 
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index f7086b78ad6e..168887a81a29 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -151,6 +151,126 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 }
 
 /**
+ * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has separate enable/disable registers instead of a single mask
+ * register and pending interrupt is acknowledged by setting a bit.
+ */
+void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	irq_reg_writel(gc, mask, ct->regs.disable);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_disable_and_ack_clr - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has separate enable/disable registers instead of a single mask
+ * register and pending interrupt is acknowledged by clearing a bit.
+ */
+void irq_gc_mask_disable_and_ack_clr(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	irq_reg_writel(gc, mask, ct->regs.disable);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, ~mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_set_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where setting bits masks the interrupt
+ * and the pending interrupt is acknowledged by setting a bit.
+ */
+void irq_gc_mask_set_and_ack_set(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	*ct->mask_cache |= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_set_and_ack_clr - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where setting bits masks the interrupt
+ * and the pending interrupt is acknowledged by clearing a bit.
+ */
+void irq_gc_mask_set_and_ack_clr(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	*ct->mask_cache |= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, ~mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_clr_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where clearing bits masks the interrupt
+ * and the pending interrupt is acknowledged by setting a bit.
+ */
+void irq_gc_mask_clr_and_ack_set(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_clr_and_ack_clr - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where clearing bits masks the interrupt
+ * and the pending interrupt is acknowledged by clearing a bit.
+ */
+void irq_gc_mask_clr_and_ack_clr(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = ~d->mask;
+
+	irq_gc_lock(gc);
+	*ct->mask_cache &= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
  * irq_gc_eoi - EOI interrupt
  * @d: irq_data
  */
-- 
2.13.0

[toc] | [next] | [standalone]

#1683592 — Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

On Fri, 7 Jul 2017, Doug Berger wrote:

> The irq_gc_mask_disable_reg_and_ack() function name implies that it
> provides the combined functions of irq_gc_mask_disable_reg() and
> irq_gc_ack().  However, the implementation does not actually do
> that since it writes the mask instead of the disable register. It
> also does not maintain the mask cache which makes it inappropriate
> to use with other masking functions.
> 
> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
> irq_gc_set_bit() so this function probably should have also been
> renamed at that time.
> 
> Since this generic chip code provides three mask functions and two
> ack functions, this commit provides generic implementations for all
> six combinations of the mask and ack functions suitable for use
> with the irq_mask_ack member of the struct irq_chip.

We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that
needs exactly on function as replacement. Why do we need 6 variants of
that right now?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1684536 — Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

On 07/08/2017 05:08 AM, Thomas Gleixner wrote:
> On Fri, 7 Jul 2017, Doug Berger wrote:
> 
>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>> provides the combined functions of irq_gc_mask_disable_reg() and
>> irq_gc_ack().  However, the implementation does not actually do
>> that since it writes the mask instead of the disable register. It
>> also does not maintain the mask cache which makes it inappropriate
>> to use with other masking functions.
>>
>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>> irq_gc_set_bit() so this function probably should have also been
>> renamed at that time.
>>
>> Since this generic chip code provides three mask functions and two
>> ack functions, this commit provides generic implementations for all
>> six combinations of the mask and ack functions suitable for use
>> with the irq_mask_ack member of the struct irq_chip.
> 
> We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that
> needs exactly on function as replacement. Why do we need 6 variants of
> that right now?

This is merely a suggestion.

When I was originally adding support for the BCM7271 variant of the
irq-brcmstb-l2 interrupt controller I noticed that providing an
irq_mask_ack implementation would be slightly more efficient than only
providing irq_mask and irq_ack.  I assumed that it was philosophically
better to use a generic chip implementation than creating yet another
driver specific version of the method.

This task was originally done on a downstream development kernel derived
from the v4.1 kernel and I'm finally taking the opportunity to attempt
to upstream the change.  At that time, I was drawn to the
irq_gc_mask_disable_reg_and_ack() function based on the name, but I
discovered that it was actually using the mask register rather than the
disable register contrary to its name and hadn't been included in the
changes when mask caching was added and when some similar functions were
renamed.  I considered submitting a patch to correct what I perceived as
a bug, but after discovering there were no users of the function at that
time I decided that it should probably be removed and replaced with the
irq_gc_mask_disable_and_ack_set() function that I needed.

While preparing the upstream submission I discovered that the tango
interrupt controller driver had apparently started using the potentially
problematic function.  I'm not comfortable making changes to drivers for
devices that I'm not able to test (I'm still making mistakes with git
send-email --cc-cmd ;) so Florian accepted authorship of that change.

I had perhaps incorrectly assumed that the
irq_gc_mask_disable_reg_and_ack() function was originally included in
the generic chip implementation nearly 5 years before its first use was
to encourage driver developers to adopt generic chip implementations
rather than implementing unique versions in every driver.  To that end
I'm suggesting offering all currently possible generic chip
implementations of the irq_mask_ack method to encourage drivers to adopt
use of generic chip methods even though I only need one of them.

Perhaps someone bolder than I may be inspired to undertake converting
more irqchip drivers to use these methods.

If I am mistaken and this is an undesired change I am happy to implement
an irq-brcmstb-l2 driver specific implementation of the irq_mask_ack
method or just changing the single function.

> 
> Thanks,
> 
> 	tglx
> 

Thanks for your consideration and please let me know how you would like
me to proceed with the submission.
    Doug

[toc] | [prev] | [next] | [standalone]

#1689293 — Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

On 07/10/2017 10:39 AM, Doug Berger wrote:
> On 07/08/2017 05:08 AM, Thomas Gleixner wrote:
>> On Fri, 7 Jul 2017, Doug Berger wrote:
>>
>>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>>> provides the combined functions of irq_gc_mask_disable_reg() and
>>> irq_gc_ack().  However, the implementation does not actually do
>>> that since it writes the mask instead of the disable register. It
>>> also does not maintain the mask cache which makes it inappropriate
>>> to use with other masking functions.
>>>
>>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>>> irq_gc_set_bit() so this function probably should have also been
>>> renamed at that time.
>>>
>>> Since this generic chip code provides three mask functions and two
>>> ack functions, this commit provides generic implementations for all
>>> six combinations of the mask and ack functions suitable for use
>>> with the irq_mask_ack member of the struct irq_chip.
>>
>> We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that
>> needs exactly on function as replacement. Why do we need 6 variants of
>> that right now?
> 
> This is merely a suggestion.
> 
> When I was originally adding support for the BCM7271 variant of the
> irq-brcmstb-l2 interrupt controller I noticed that providing an
> irq_mask_ack implementation would be slightly more efficient than only
> providing irq_mask and irq_ack.  I assumed that it was philosophically
> better to use a generic chip implementation than creating yet another
> driver specific version of the method.
> 
> This task was originally done on a downstream development kernel derived
> from the v4.1 kernel and I'm finally taking the opportunity to attempt
> to upstream the change.  At that time, I was drawn to the
> irq_gc_mask_disable_reg_and_ack() function based on the name, but I
> discovered that it was actually using the mask register rather than the
> disable register contrary to its name and hadn't been included in the
> changes when mask caching was added and when some similar functions were
> renamed.  I considered submitting a patch to correct what I perceived as
> a bug, but after discovering there were no users of the function at that
> time I decided that it should probably be removed and replaced with the
> irq_gc_mask_disable_and_ack_set() function that I needed.
> 
> While preparing the upstream submission I discovered that the tango
> interrupt controller driver had apparently started using the potentially
> problematic function.  I'm not comfortable making changes to drivers for
> devices that I'm not able to test (I'm still making mistakes with git
> send-email --cc-cmd ;) so Florian accepted authorship of that change.
> 
> I had perhaps incorrectly assumed that the
> irq_gc_mask_disable_reg_and_ack() function was originally included in
> the generic chip implementation nearly 5 years before its first use was
> to encourage driver developers to adopt generic chip implementations
> rather than implementing unique versions in every driver.  To that end
> I'm suggesting offering all currently possible generic chip
> implementations of the irq_mask_ack method to encourage drivers to adopt
> use of generic chip methods even though I only need one of them.
> 
> Perhaps someone bolder than I may be inspired to undertake converting
> more irqchip drivers to use these methods.
> 
> If I am mistaken and this is an undesired change I am happy to implement
> an irq-brcmstb-l2 driver specific implementation of the irq_mask_ack
> method or just changing the single function.
> 
>>
>> Thanks,
>>
>> 	tglx
>>
> 
> Thanks for your consideration and please let me know how you would like
> me to proceed with the submission.
>     Doug
> 

Seeing as Mans has acked the change to his driver should I submit a V2
with just the function he and I are using and remove the other five
permutations, or are you willing to move forward with the patch as is?

Thanks,
    Doug

[toc] | [prev] | [next] | [standalone]

#1689467 — Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

On Mon, 17 Jul 2017, Doug Berger wrote:

> Seeing as Mans has acked the change to his driver should I submit a V2
> with just the function he and I are using and remove the other five
> permutations, or are you willing to move forward with the patch as is?

Please resubmit with the implementation which is actually used. We can add
the others when the need arises. You might mention that in the changelog
and the function comment, so people looking for a suitable alternative will
get the proper hint.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1685996 — Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

Mans, as the author of the only existing upstream user of this code,
should have received this as well.

-Doug

On 07/07/2017 12:20 PM, Doug Berger wrote:
> The irq_gc_mask_disable_reg_and_ack() function name implies that it
> provides the combined functions of irq_gc_mask_disable_reg() and
> irq_gc_ack().  However, the implementation does not actually do
> that since it writes the mask instead of the disable register. It
> also does not maintain the mask cache which makes it inappropriate
> to use with other masking functions.
> 
> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
> irq_gc_set_bit() so this function probably should have also been
> renamed at that time.
> 
> Since this generic chip code provides three mask functions and two
> ack functions, this commit provides generic implementations for all
> six combinations of the mask and ack functions suitable for use
> with the irq_mask_ack member of the struct irq_chip.
> 
> The '_reg' and '_bit' portions of the base function names were left
> out of the new combined function names in an attempt to keep the
> function name lengths manageable with the 80 character source code
> line length while still capturing the distinct aspects of each
> combination of functions.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  include/linux/irq.h       |   6 +++
>  kernel/irq/generic-chip.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 00db35b61e9e..23b9617bb682 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -1003,6 +1003,12 @@ void irq_gc_unmask_enable_reg(struct irq_data *d);
>  void irq_gc_ack_set_bit(struct irq_data *d);
>  void irq_gc_ack_clr_bit(struct irq_data *d);
>  void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
> +void irq_gc_mask_disable_and_ack_set(struct irq_data *d);
> +void irq_gc_mask_disable_and_ack_clr(struct irq_data *d);
> +void irq_gc_mask_set_and_ack_set(struct irq_data *d);
> +void irq_gc_mask_set_and_ack_clr(struct irq_data *d);
> +void irq_gc_mask_clr_and_ack_set(struct irq_data *d);
> +void irq_gc_mask_clr_and_ack_clr(struct irq_data *d);
>  void irq_gc_eoi(struct irq_data *d);
>  int irq_gc_set_wake(struct irq_data *d, unsigned int on);
>  
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index f7086b78ad6e..168887a81a29 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -151,6 +151,126 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
>  }
>  
>  /**
> + * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has separate enable/disable registers instead of a single mask
> + * register and pending interrupt is acknowledged by setting a bit.
> + */
> +void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	irq_reg_writel(gc, mask, ct->regs.disable);
> +	*ct->mask_cache &= ~mask;
> +	irq_reg_writel(gc, mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
> + * irq_gc_mask_disable_and_ack_clr - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has separate enable/disable registers instead of a single mask
> + * register and pending interrupt is acknowledged by clearing a bit.
> + */
> +void irq_gc_mask_disable_and_ack_clr(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	irq_reg_writel(gc, mask, ct->regs.disable);
> +	*ct->mask_cache &= ~mask;
> +	irq_reg_writel(gc, ~mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
> + * irq_gc_mask_set_and_ack_set - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has a single mask register where setting bits masks the interrupt
> + * and the pending interrupt is acknowledged by setting a bit.
> + */
> +void irq_gc_mask_set_and_ack_set(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	*ct->mask_cache |= mask;
> +	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
> +	irq_reg_writel(gc, mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
> + * irq_gc_mask_set_and_ack_clr - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has a single mask register where setting bits masks the interrupt
> + * and the pending interrupt is acknowledged by clearing a bit.
> + */
> +void irq_gc_mask_set_and_ack_clr(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	*ct->mask_cache |= mask;
> +	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
> +	irq_reg_writel(gc, ~mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
> + * irq_gc_mask_clr_and_ack_set - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has a single mask register where clearing bits masks the interrupt
> + * and the pending interrupt is acknowledged by setting a bit.
> + */
> +void irq_gc_mask_clr_and_ack_set(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	*ct->mask_cache &= ~mask;
> +	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
> +	irq_reg_writel(gc, mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
> + * irq_gc_mask_clr_and_ack_clr - Mask and ack pending interrupt
> + * @d: irq_data
> + *
> + * Chip has a single mask register where clearing bits masks the interrupt
> + * and the pending interrupt is acknowledged by clearing a bit.
> + */
> +void irq_gc_mask_clr_and_ack_clr(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = ~d->mask;
> +
> +	irq_gc_lock(gc);
> +	*ct->mask_cache &= mask;
> +	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
> +	irq_reg_writel(gc, mask, ct->regs.ack);
> +	irq_gc_unlock(gc);
> +}
> +
> +/**
>   * irq_gc_eoi - EOI interrupt
>   * @d: irq_data
>   */
> 

[toc] | [prev] | [next] | [standalone]

#1686029

Doug Berger <opendmb@gmail.com> writes:

> Mans, as the author of the only existing upstream user of this code,
> should have received this as well.
>
> -Doug
>
> On 07/07/2017 12:20 PM, Doug Berger wrote:
>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>> provides the combined functions of irq_gc_mask_disable_reg() and
>> irq_gc_ack().  However, the implementation does not actually do
>> that since it writes the mask instead of the disable register. It
>> also does not maintain the mask cache which makes it inappropriate
>> to use with other masking functions.
>> 
>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>> irq_gc_set_bit() so this function probably should have also been
>> renamed at that time.
>> 
>> Since this generic chip code provides three mask functions and two
>> ack functions, this commit provides generic implementations for all
>> six combinations of the mask and ack functions suitable for use
>> with the irq_mask_ack member of the struct irq_chip.
>> 
>> The '_reg' and '_bit' portions of the base function names were left
>> out of the new combined function names in an attempt to keep the
>> function name lengths manageable with the 80 character source code
>> line length while still capturing the distinct aspects of each
>> combination of functions.
>> 
>> Signed-off-by: Doug Berger <opendmb@gmail.com>

Hmm, something is wrong here.  The irq_gc_mask_disable_reg_and_ack()
function writes to regs.mask, but the irq-tango driver doesn't set this
field (there is no corresponding hardware register).  Either it is never
called, or the write ends up being harmless.  I don't remember why I set
irq_mask_ack that way.

>>  /**
>> + * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
>> + * @d: irq_data
>> + *
>> + * Chip has separate enable/disable registers instead of a single mask
>> + * register and pending interrupt is acknowledged by setting a bit.
>> + */
>> +void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
>> +{
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> +	u32 mask = d->mask;
>> +
>> +	irq_gc_lock(gc);
>> +	irq_reg_writel(gc, mask, ct->regs.disable);
>> +	*ct->mask_cache &= ~mask;
>> +	irq_reg_writel(gc, mask, ct->regs.ack);
>> +	irq_gc_unlock(gc);
>> +}

This function looks like it should probably be used instead.  I'll try
to remember to test it when I have time to fire up that hardware.

-- 
Måns Rullgård

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web