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

Groups > linux.kernel > #1261406 > unrolled thread

[PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain

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 10/14] irqchip/mips-gic: Add a IPI hierarchy domain Qais Yousef <qais.yousef@imgtec.com> - 2015-11-03 12:20 +0100
    Re: [PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain Thomas Gleixner <tglx@linutronix.de> - 2015-11-07 16:00 +0100
      Re: [PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain Qais Yousef <qais.yousef@imgtec.com> - 2015-11-09 12:20 +0100
      Re: [PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain Qais Yousef <qais.yousef@imgtec.com> - 2015-11-12 16:20 +0100

#1261406 — [PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain

Add a new ipi domain on top of the normal domain.

MIPS GIC now supports dynamic allocation of an IPI.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
---
 drivers/irqchip/Kconfig        |   1 +
 drivers/irqchip/irq-mips-gic.c | 142 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 136 insertions(+), 7 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e5d982..e1dcfdffd2c7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -168,6 +168,7 @@ config KEYSTONE_IRQ
 
 config MIPS_GIC
 	bool
+	select IRQ_DOMAIN_HIERARCHY
 	select MIPS_CM
 
 config INGENIC_IRQ
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index aeaa061f0dbf..9bcaa4e6f40c 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -33,11 +33,14 @@ static void __iomem *gic_base;
 static struct gic_pcpu_mask pcpu_masks[NR_CPUS];
 static DEFINE_SPINLOCK(gic_lock);
 static struct irq_domain *gic_irq_domain;
+static struct irq_domain *gic_ipi_domain;
 static int gic_shared_intrs;
 static int gic_vpes;
 static unsigned int gic_cpu_pin;
 static unsigned int timer_cpu_pin;
 static struct irq_chip gic_level_irq_controller, gic_edge_irq_controller;
+DECLARE_BITMAP(ipi_intrs, GIC_MAX_INTRS);
+DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS);
 
 static void __gic_irq_dispatch(void);
 
@@ -335,8 +338,14 @@ static void gic_handle_shared_int(bool chained)
 
 	intr = find_first_bit(pending, gic_shared_intrs);
 	while (intr != gic_shared_intrs) {
-		virq = irq_linear_revmap(gic_irq_domain,
-					 GIC_SHARED_TO_HWIRQ(intr));
+		if (test_bit(intr, ipi_intrs)) {
+			virq = irq_linear_revmap(gic_ipi_domain,
+					GIC_SHARED_TO_HWIRQ(intr));
+		} else {
+			virq = irq_linear_revmap(gic_irq_domain,
+					GIC_SHARED_TO_HWIRQ(intr));
+		}
+
 		if (chained)
 			generic_handle_irq(virq);
 		else
@@ -741,7 +750,7 @@ static int gic_local_irq_domain_map(struct irq_domain *d, unsigned int virq,
 }
 
 static int gic_shared_irq_domain_map(struct irq_domain *d, unsigned int virq,
-				     irq_hw_number_t hw)
+				     irq_hw_number_t hw, unsigned int vpe)
 {
 	int intr = GIC_HWIRQ_TO_SHARED(hw);
 	unsigned long flags;
@@ -751,9 +760,8 @@ static int gic_shared_irq_domain_map(struct irq_domain *d, unsigned int virq,
 
 	spin_lock_irqsave(&gic_lock, flags);
 	gic_map_to_pin(intr, gic_cpu_pin);
-	/* Map to VPE 0 by default */
-	gic_map_to_vpe(intr, 0);
-	set_bit(intr, pcpu_masks[0].pcpu_mask);
+	gic_map_to_vpe(intr, vpe);
+	set_bit(intr, pcpu_masks[vpe].pcpu_mask);
 	spin_unlock_irqrestore(&gic_lock, flags);
 
 	return 0;
@@ -764,7 +772,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq,
 {
 	if (GIC_HWIRQ_TO_LOCAL(hw) < GIC_NUM_LOCAL_INTRS)
 		return gic_local_irq_domain_map(d, virq, hw);
-	return gic_shared_irq_domain_map(d, virq, hw);
+	return gic_shared_irq_domain_map(d, virq, hw, 0);
 }
 
 static int gic_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
@@ -791,6 +799,115 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.xlate = gic_irq_domain_xlate,
 };
 
+static int gic_ipi_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+				const u32 *intspec, unsigned int intsize,
+				irq_hw_number_t *out_hwirq,
+				unsigned int *out_type)
+{
+	/*
+	 * There's nothing to translate here. hwirq is dynamically allocated and
+	 * the irq type is always edge triggered.
+	 * */
+	*out_hwirq = 0;
+	*out_type = IRQ_TYPE_EDGE_RISING;
+
+	return 0;
+}
+
+static int gic_ipi_domain_alloc(struct irq_domain *d, unsigned int virq,
+				unsigned int nr_irqs, void *arg)
+{
+	struct ipi_mask *ipimask = arg;
+	const unsigned long *bits;
+	struct ipi_mapping *map;
+	irq_hw_number_t hwirq;
+	int cpu, ret;
+
+	map = irq_alloc_ipi_mapping(gic_vpes);
+	if (!map)
+		return -ENOMEM;
+
+	bits = ipi_mask_bits(ipimask);
+
+	cpu = find_first_bit(bits, ipimask->nbits);
+	while (cpu != ipimask->nbits) {
+		hwirq = find_first_bit(ipi_resrv, gic_shared_intrs);
+		if (hwirq == gic_shared_intrs) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		bitmap_clear(ipi_resrv, hwirq, 1);
+		bitmap_set(ipi_intrs, hwirq, 1);
+
+		ret = irq_map_ipi(map, cpu, hwirq);
+		if (ret)
+			goto out;
+
+		hwirq = GIC_SHARED_TO_HWIRQ(hwirq);
+		ret = irq_domain_set_hwirq_and_chip(d, virq + cpu, hwirq,
+						&gic_edge_irq_controller, map);
+		if (ret)
+			goto out;
+
+		ret = gic_shared_irq_domain_map(d, virq + cpu, hwirq, cpu);
+		if (ret)
+			goto out;
+
+		ret = irq_set_irq_type(virq + cpu, IRQ_TYPE_EDGE_RISING);
+		if (ret)
+			goto out;
+
+		cpu = find_next_bit(bits, ipimask->nbits, cpu + 1);
+	}
+
+	return 0;
+out:
+	irq_free_ipi_mapping(map);
+	return ret;
+}
+
+void gic_ipi_domain_free(struct irq_domain *d, unsigned int virq,
+			 unsigned int nr_irqs)
+{
+	struct ipi_mapping *map = irq_get_chip_data(virq);
+	irq_hw_number_t hwirq;
+	int ret, cpu;
+
+	for (cpu = 0; cpu < map->nr_cpus; cpu++) {
+		ret = irq_unmap_ipi(map, cpu, &hwirq);
+		if (!ret) {
+			bitmap_set(ipi_resrv, hwirq, 1);
+			bitmap_clear(ipi_intrs, hwirq, 1);
+		}
+	}
+
+	irq_free_ipi_mapping(map);
+}
+
+int gic_ipi_domain_match(struct irq_domain *d, struct device_node *node,
+			 enum irq_domain_bus_token bus_token)
+{
+	bool is_ipi = true;
+
+	switch (bus_token) {
+	case DOMAIN_BUS_IPI:
+		is_ipi = d->bus_token == bus_token;
+	case DOMAIN_BUS_ANY:
+		return to_of_node(d->fwnode) == node && is_ipi;
+		break;
+	default:
+		return 0;
+	}
+}
+
+static struct irq_domain_ops gic_ipi_domain_ops = {
+	.xlate = gic_ipi_domain_xlate,
+	.alloc = gic_ipi_domain_alloc,
+	.free = gic_ipi_domain_free,
+	.match = gic_ipi_domain_match,
+};
+
 static void __init __gic_init(unsigned long gic_base_addr,
 			      unsigned long gic_addrspace_size,
 			      unsigned int cpu_vec, unsigned int irqbase,
@@ -850,6 +967,17 @@ static void __init __gic_init(unsigned long gic_base_addr,
 	if (!gic_irq_domain)
 		panic("Failed to add GIC IRQ domain");
 
+	gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain, IRQ_DOMAIN_FLAG_IPI,
+						  GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
+						  node, &gic_ipi_domain_ops, NULL);
+	if (!gic_ipi_domain)
+		panic("Failed to add GIC IPI domain");
+
+	gic_ipi_domain->bus_token = DOMAIN_BUS_IPI;
+
+	/* Make the last 2 * NR_CPUS available for IPIs */
+	bitmap_set(ipi_resrv, gic_shared_intrs - 2 * NR_CPUS, 2 * NR_CPUS);
+
 	gic_basic_init();
 
 	gic_ipi_init();
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1264856

On Tue, 3 Nov 2015, Qais Yousef wrote:

> Add a new ipi domain on top of the normal domain.
> 
> MIPS GIC now supports dynamic allocation of an IPI.

I don't think you make use of the power of hierarchical irq
domains. You just whacked the current code into submission.

Let me explain it to you how that should look like and how that's
going to make the code way simpler.

The root domain is the GIC itself. It provides an allocation mechanism
for all GIC interrupts, global, ipi and per cpu plus the basic
management of them.

So the GIC domain looks at the complete hardware irq space. Now that
irq space is first partioned into local and shared interrupts.

   ------------- hwirq MAX

   Shared interrupts   

   ------------- hwirq 6

   Local interrupts

   ------------- hwirq 0

So that shared interrupt space is where your device interrupts and the
ipi interrupts come from. That local interrupt space seems to be
hardwired, so it'd be overkill to handle that in an extra domain.

I assume that that shared interrupt space is partitioned as well
because the potential device interrupts on the SoC are hardwired. So
the picture looks like this:

   ------------- hwirq MAX

   Shared assignable interrupts

   ------------- hwirq X

   Shared device interrupts   

   ------------- hwirq 6

   Local interrupts

   ------------- hwirq 0


So if we look at the resulting hierarchy it looks like this:

                 |----- [IPI domain]
  [ GIC domain] -
                 |----- [device domain]

The GIC domain manages a bitmap of the full irq space. The IPI domain
and the device domain request N interrupts from the GIC domain at
allocation time.

So when you allocate from the device domain, you tell the parent
domain, that this is actually a device interrupt, and from the IPI
domain you tell it it's an IPI.

So the allocator in the root domain can decide from which space of the
bitmap to allocate.

       if (device) {
       	      hwirq = translate_from_dt(arg);
	      if (test_and_set_bit(&allocated_irqs, hwirq))
	      	     return -EBUSY;
       } else {
       	      start = first_ipi_irq;
	      end = last_ipi_irq + 1;
	      hwirq = bitmap_find_next_zero_area(allocated_irqs, start, end,
       	       				  	 nrirqs, 0);
       }
       ....

So that gives you a consecutive hw irq space for your IPI.

That makes a lot of things simpler. You don't have to keep a mapping
of the hwirq to the target cpu. You just can use the base hwirq and
calculate the destination hwirq from there when sending an IPI
(general Linux ones). The coprocessor one will just be a natural
fallout.

So if you have the following in the generic ipi code:

void ipi_send_single(unsigned int irq, unsigned int cpu)
{
	struct irq_desc *desc = irq_to_desc(irq);
	struct irq_data *data = irq_desc_get_irq_data(desc);
	struct irq_chip *chip = irq_data_get_irq_chip(data);
	
	if (chip->ipi_send_single)
		chip->ipi_send_single(data, cpu);
	else
		chip->ipi_send_mask(data, cpumask_of(cpu));
}

void ipi_send_mask(unsigned int irq, const struct cpumask *dest)
{
	struct irq_desc *desc = irq_to_desc(irq);
	struct irq_data *data = irq_desc_get_irq_data(desc);
	struct irq_chip *chip = irq_data_get_irq_chip(data);
	int cpu;

	if (!chip->ipi_send_mask) {
		for_each_cpu(cpu, dest)
			chip->ipi_send_single(data, cpu);
	} else {
		chip->ipi_send_mask(data, dest);
	}
}

void ipi_send_coproc_mask(unsigned int irq, const struct ipi_mask *dest)
{
        Fill in the obvious code.
}

And your ipi_send_single() callback just boils down to:
{
    	target = data->hw_irq + cpu;

	tweak_chip_regs(target);
}

Sanity checks omitted for brevity.

And that whole thing works for your case and for the case where we
only have a single per cpu interrupt descriptor allocated. The irq
descriptor based variants are exactly the same thing.

So now for the irq chips of your device and IPI domains. You can
either set the proper GIC chip variant or in case you need some extra
magic for one of the domains, you implement your own special chip
which can have some of its callback implemented by the existing
irq_***_parent() variants.

That gets rid of all your extra mappings, bitmaps and whatever add ons
you have duct taped into the existing GIC code.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1265571

On 11/07/2015 02:51 PM, Thomas Gleixner wrote:
> On Tue, 3 Nov 2015, Qais Yousef wrote:
>
>> Add a new ipi domain on top of the normal domain.
>>
>> MIPS GIC now supports dynamic allocation of an IPI.
> I don't think you make use of the power of hierarchical irq
> domains. You just whacked the current code into submission.
>
> Let me explain it to you how that should look like and how that's
> going to make the code way simpler.
>
> The root domain is the GIC itself. It provides an allocation mechanism
> for all GIC interrupts, global, ipi and per cpu plus the basic
> management of them.
>
> So the GIC domain looks at the complete hardware irq space. Now that
> irq space is first partioned into local and shared interrupts.
>
>     ------------- hwirq MAX
>
>     Shared interrupts
>
>     ------------- hwirq 6
>
>     Local interrupts
>
>     ------------- hwirq 0
>
> So that shared interrupt space is where your device interrupts and the
> ipi interrupts come from. That local interrupt space seems to be
> hardwired, so it'd be overkill to handle that in an extra domain.
>
> I assume that that shared interrupt space is partitioned as well
> because the potential device interrupts on the SoC are hardwired. So
> the picture looks like this:
>
>     ------------- hwirq MAX
>
>     Shared assignable interrupts
>
>     ------------- hwirq X
>
>     Shared device interrupts
>
>     ------------- hwirq 6
>
>     Local interrupts
>
>     ------------- hwirq 0
>
>
> So if we look at the resulting hierarchy it looks like this:
>
>                   |----- [IPI domain]
>    [ GIC domain] -
>                   |----- [device domain]
>
> The GIC domain manages a bitmap of the full irq space. The IPI domain
> and the device domain request N interrupts from the GIC domain at
> allocation time.
>
> So when you allocate from the device domain, you tell the parent
> domain, that this is actually a device interrupt, and from the IPI
> domain you tell it it's an IPI.
>
> So the allocator in the root domain can decide from which space of the
> bitmap to allocate.
>
>         if (device) {
>         	      hwirq = translate_from_dt(arg);
> 	      if (test_and_set_bit(&allocated_irqs, hwirq))
> 	      	     return -EBUSY;
>         } else {
>         	      start = first_ipi_irq;
> 	      end = last_ipi_irq + 1;
> 	      hwirq = bitmap_find_next_zero_area(allocated_irqs, start, end,
>         	       				  	 nrirqs, 0);
>         }
>         ....
>
> So that gives you a consecutive hw irq space for your IPI.


This is potentially dangerous. If a device allocation happens after an 
IPI allocation, and that device wants the hwirq at the region that IPI 
just allocated because it thought it's free, it'd fail.

Generally it's hard to know whether a real device is connected to a 
hwirq or not. I am saving a patch where we get a set of free hwirqs from 
DT as only the SoC designer knows what hwirq are actually free and safe 
to use for IPI. I'll send this patch with the DT IPI changes or the 
rproc driver that I will be send once these changes are merged.

The current code assumes that the last 2 * NR_CPUs hwirqs are always 
free to use for Linux SMP.

>
> That makes a lot of things simpler. You don't have to keep a mapping
> of the hwirq to the target cpu. You just can use the base hwirq and
> calculate the destination hwirq from there when sending an IPI
> (general Linux ones). The coprocessor one will just be a natural
> fallout.

Are you suggesting here to remove the whole new mapping API from the 
generic code or just that it's not necessary to use it in my case?

>
> So if you have the following in the generic ipi code:
>
> void ipi_send_single(unsigned int irq, unsigned int cpu)
> {
> 	struct irq_desc *desc = irq_to_desc(irq);
> 	struct irq_data *data = irq_desc_get_irq_data(desc);
> 	struct irq_chip *chip = irq_data_get_irq_chip(data);
> 	
> 	if (chip->ipi_send_single)
> 		chip->ipi_send_single(data, cpu);
> 	else
> 		chip->ipi_send_mask(data, cpumask_of(cpu));
> }
>
> void ipi_send_mask(unsigned int irq, const struct cpumask *dest)
> {
> 	struct irq_desc *desc = irq_to_desc(irq);
> 	struct irq_data *data = irq_desc_get_irq_data(desc);
> 	struct irq_chip *chip = irq_data_get_irq_chip(data);
> 	int cpu;
>
> 	if (!chip->ipi_send_mask) {
> 		for_each_cpu(cpu, dest)
> 			chip->ipi_send_single(data, cpu);
> 	} else {
> 		chip->ipi_send_mask(data, dest);
> 	}
> }
>
> void ipi_send_coproc_mask(unsigned int irq, const struct ipi_mask *dest)
> {
>          Fill in the obvious code.
> }
>
> And your ipi_send_single() callback just boils down to:
> {
>      	target = data->hw_irq + cpu;
>
> 	tweak_chip_regs(target);
> }
>
> Sanity checks omitted for brevity.

I'm confused here as well. Is this a complementary API or are you 
suggesting replacing the one this patch introduces?

>
> And that whole thing works for your case and for the case where we
> only have a single per cpu interrupt descriptor allocated. The irq
> descriptor based variants are exactly the same thing.
>
> So now for the irq chips of your device and IPI domains. You can
> either set the proper GIC chip variant or in case you need some extra
> magic for one of the domains, you implement your own special chip
> which can have some of its callback implemented by the existing
> irq_***_parent() variants.
>
> That gets rid of all your extra mappings, bitmaps and whatever add ons
> you have duct taped into the existing GIC code.

This is all new territory for me. I thought I did it the correct way and 
it seemed simple to me.

I'll follow your idea through and see what I come up with. I'm getting a 
bit confused about how the generic API should look like now though. I'll 
keep what I added and add any new needed stuff and hopefully once I 
start doing the work and when the next series is ready it'd be more 
obvious what's needed and what's not.

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1268024

Hi Thomas,

On 11/07/2015 02:51 PM, Thomas Gleixner wrote:
> On Tue, 3 Nov 2015, Qais Yousef wrote:
>
>> Add a new ipi domain on top of the normal domain.
>>
>> MIPS GIC now supports dynamic allocation of an IPI.
> I don't think you make use of the power of hierarchical irq
> domains. You just whacked the current code into submission.
>

This time I'm having problems digesting your suggestion. I can't see how 
it would make things simpler to be honest.

Issues I'm seeing:

     - Device domain would be identical to GIC domain and it would defer 
everything to the parent domain except for the extra level of 
indirection. No?

     - The race condition I mentioned in my earlier email where we must 
be told what hwirqs are available because we can't guarantee there's no 
real device connected to it which could interfere with the operation. We 
have always to work on a pre reserved set defined by the system. 
Currently GIC hard codes this set, but I'll be making it a DT property 
in the future.

     - If we remove the mapping, how can a coprocessor drivers find out 
the reverse mapping to pass the hwirq to the firmware so that it can 
send and listen on the correct hwirqs? I have to say my current patches 
missed dealing with this problem. Now I have something to test my rproc 
driver on I came to realise I haven't added the function to do the 
reverse mapping.

In summary. I can't see how adding the device domain would help in 
making things simpler and without having generic explicit cpu mapping I 
don't know how I can implement a generic reverse mapping function to get 
the hwirq to pass to the coprocessor firmware.

If I misunderstood your suggestion, mind rephrasing it please?

I can see though if I use irq_*_alloc_parent() I can probably get rid 
off the below since I'd be able to use gic_irq_domain all the time to do 
the revmap.

+		if (test_bit(intr, ipi_intrs)) {
+			virq = irq_linear_revmap(gic_ipi_domain,
+					GIC_SHARED_TO_HWIRQ(intr));
+		} else {
+			virq = irq_linear_revmap(gic_irq_domain,
+					GIC_SHARED_TO_HWIRQ(intr));
+		}
+



Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web