diff mbox series

[v3,3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

Message ID 20181130080207.20505-4-anup@brainfault.org (mailing list archive)
State New, archived
Headers show
Series IRQ affinity support in PLIC driver | expand

Commit Message

Anup Patel Nov. 30, 2018, 8:02 a.m. UTC
We make plic_irq_toggle() more generic so that we can enable/disable
hwirq for given cpumask. This generic plic_irq_toggle() will be
eventually used to implement set_affinity for PLIC driver.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Christoph Hellwig Dec. 17, 2018, 6:27 p.m. UTC | #1
> -static inline void plic_toggle(struct plic_handler *handler,
> -				int hwirq, int enable)
> +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
>  {
>  	u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
>  	u32 hwirq_mask = 1 << (hwirq % 32);
> @@ -92,27 +91,27 @@ static inline void plic_toggle(struct plic_handler *handler,
>  	raw_spin_unlock(&handler->enable_lock);
>  }
>  
> -static inline void plic_irq_toggle(struct irq_data *d, int enable)
> +static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)

It also removes inline statements which seems rather unrelated to
the patch description.

Also the actual addintion of the single cpumask argument is simple
enough that it should probably go into the patch that makes use of it.
Anup Patel Dec. 18, 2018, 8:50 a.m. UTC | #2
On Mon, Dec 17, 2018 at 11:57 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > -static inline void plic_toggle(struct plic_handler *handler,
> > -                             int hwirq, int enable)
> > +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
> >  {
> >       u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
> >       u32 hwirq_mask = 1 << (hwirq % 32);
> > @@ -92,27 +91,27 @@ static inline void plic_toggle(struct plic_handler *handler,
> >       raw_spin_unlock(&handler->enable_lock);
> >  }
> >
> > -static inline void plic_irq_toggle(struct irq_data *d, int enable)
> > +static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
>
> It also removes inline statements which seems rather unrelated to
> the patch description.

Actually these functions should not be inline because plic_toggle() uses
raw_spin_lock() and plic_irq_toggle() uses for-loop.

>
> Also the actual addintion of the single cpumask argument is simple
> enough that it should probably go into the patch that makes use of it.

OK, I will have separate patch for removing "inline" and move addition
of cpumask argument to last patch.

Regards,
Anup
Christoph Hellwig Dec. 19, 2018, 4:28 p.m. UTC | #3
On Tue, Dec 18, 2018 at 02:20:10PM +0530, Anup Patel wrote:
> Actually these functions should not be inline because plic_toggle() uses
> raw_spin_lock() and plic_irq_toggle() uses for-loop.

So?  It still inlines the all of two instances into each caller
for slightly different but related work.  Not sure it is 100% worth
it, but probably more than the one to move the calculations to init
time..
Anup Patel Dec. 27, 2018, 5:27 a.m. UTC | #4
On Wed, Dec 19, 2018 at 9:58 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Dec 18, 2018 at 02:20:10PM +0530, Anup Patel wrote:
> > Actually these functions should not be inline because plic_toggle() uses
> > raw_spin_lock() and plic_irq_toggle() uses for-loop.
>
> So?  It still inlines the all of two instances into each caller
> for slightly different but related work.  Not sure it is 100% worth
> it, but probably more than the one to move the calculations to init
> time..

Not just at init time but these functions will also be used when
irq_affinity is changed by IRQ balancer at runtime.

Regards,
Anup
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 48bee877e0f1..d4433399eb89 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -78,8 +78,7 @@  struct plic_hw {
 
 static struct plic_hw plic;
 
-static inline void plic_toggle(struct plic_handler *handler,
-				int hwirq, int enable)
+static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
 {
 	u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
 	u32 hwirq_mask = 1 << (hwirq % 32);
@@ -92,27 +91,27 @@  static inline void plic_toggle(struct plic_handler *handler,
 	raw_spin_unlock(&handler->enable_lock);
 }
 
-static inline void plic_irq_toggle(struct irq_data *d, int enable)
+static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
 {
 	int cpu;
 
-	writel(enable, plic.regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
-	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
+	writel(enable, plic.regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
+	for_each_cpu(cpu, mask) {
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
 		if (handler->present)
-			plic_toggle(handler, d->hwirq, enable);
+			plic_toggle(handler, hwirq, enable);
 	}
 }
 
 static void plic_irq_enable(struct irq_data *d)
 {
-	plic_irq_toggle(d, 1);
+	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 1);
 }
 
 static void plic_irq_disable(struct irq_data *d)
 {
-	plic_irq_toggle(d, 0);
+	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 0);
 }
 
 static struct irq_chip plic_chip = {