| Message ID | 559C9BB4.90103@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 08/07/15 04:40, Hanjun Guo wrote: > Hi Marc, > > On 06/30/2015 08:17 PM, Marc Zyngier wrote: >> On 30/06/15 12:50, Hanjun Guo wrote: >>> Hi Marc, >>> >>> On 06/29/2015 04:39 PM, Marc Zyngier wrote: >>>> On 27/06/15 04:52, Hanjun Guo wrote: >>>>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: >>>>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: >>> [...] >>>>>> >>>>>>>>> for (i = 0; i < nr_irqs; i++) >>>>>>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>>>>> gic_irqs = 1020; >>>>>>>>> gic->gic_irqs = gic_irqs; >>>>>>>>> >>>>>>>>> - if (node) { /* DT case */ >>>>>>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>>>>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>>>>>> &gic_irq_domain_hierarchy_ops, >>>>>>>>> gic); >>>>>> >>>>>> I think this is a bit more worrying, I mean passing a NULL node pointer to >>>>>> the irqdomain layer which basically means you are booting out of ACPI >>>>> >>>>> I'm little confused here, would you mind explaining more for your >>>>> worrying? To me, node pointer is optional and it's ok for ACPI >>>>> case. >>>>> >>>>>> (for you, if that's true for the irq_domain_add_linear implementation >>>>>> that's another story), the node pointer should be optional but you >>>>>> need feedback from IRQ layer maintainers here. >>>>> >>>>> Sure. >>>> >>>> Frankly, I'd really like to see ACPI using the "node" parameter for >>>> something useful. This would save having to cache pointers all over the >>>> place, will make find_irq_host() work as expected... etc. >>>> >>>> See the comment at the top of linux/irqdomain.h : >>>> >>>> "... This code could thus be used on other architectures by replacing >>>> those two by some sort of arch-specific void * "token" used to identify >>>> interrupt controllers." > > I'm stuck here, I think the token for now is tied to device tree as > the device node pointer for now in the code if the pointer is not NULL, > just see the of_node_get(of_node) in __irq_domain_add(), if dynamic > dt is enabled: > > struct device_node *of_node_get(struct device_node *node) > { > if (node) > kobject_get(&node->kobj); > return node; > } > EXPORT_SYMBOL(of_node_get); > > so if the node is not NULL, it will be used to get the node's > kobject, that will be a problem for acpi case if we pass a token > as a parameter. Well, I'm sure this could be worked around, and we could have different handling for different type of objects. > > I rethink for quite a while and have a discussion with Graeme, > I come out another idea and we don't need the token at all > for ACPI case. >>> >>> To init GIC in ACPI, we can only use the table entry pointer as >>> the token, but the ACPI static tables are early mem/io remapped >>> memory at boot stage, and it will be not available after boot, >>> also we need muti types of MADT enties to init GIC (GICC and GICD >>> for GICv2, GICC or GICR and GICD for GICv3), not as DT, just >>> one single node to include all the information needed to init >>> the GIC. >> >> A single pointer would be enough, you don't need all of them. >> >>> We use ACPI handle for devices as node for DT when the namespace >>> is available, but that's pretty late in the boot stage which GIC, >>> SMP and timers were already initialized, so ACPI handle can not >>> use as the token too. >>> >>> I see multi places just pass NULL as the pointer directly for >>> irq_domain_add_linear() which works fine, and for ACPI, we tested >>> this patch and also it works. >> >> Yes it works. But you're reinventing the wheel by keeping references >> outside of the normal framework, which is simply going to make the code > > That's the framework for DT not suitable for ACPI, just see the comments > below. > >> more difficult to maintain in the long run. >> >> Putting NULL as the device_node parameter really means "this is a domain >> I don't need to look up later". In your case, you will have to lookup > > I agree that we need to look up the domain not just using the > global acpi_irq_domain pointer everywhere, which make the code not > scalable, but I don't think it's not OK to put NULL as the device_node > parameter in ACPI case. > > The reason is that we use different model of mapping interrupt with > device in ACPI, for ACPI device in DSDT, it's using another mapping of > device and hwirq number than DT, in DT, we using the interrupt parent > property to get the device node of irqchip, that's why we need the > matching function that match the device node with the one associated > with the irqdomain. But for ACPI, we only can get the gsi which the > device is using, no interrupt parent will be used, that because gsi is > a flat hwirq number which is unique in the system, then we can get its > associated irq domain by matching the gsi supported by this irqchip > (see drawings below), then we can be without the token pointer > matching the interrupt controller. But that's for only GSI. GSI are not the *real* problem. The real issue is when you have a system that composes domains. We're moving away from a model where you can store the various domains yourself. For example, have a look at this: https://lwn.net/Articles/650418/ and specially patch 13. See how the PCI/MSI domain looks up the core ITS domain? This is where we're going. Various *independent* entities building a hierarchy based on a common repository. So please stop saying that ACPI is different, because it really isn't. You create domains based on some global identifier, you look them up using this identifier. There is nothing more to it. The core code has some issues with that? Let's fix it. Look at Suravee's patches introducing ACPI support for GICv2m. He seems to be doing a rather good job reusing the existing framework, and adapting it to fit the ACPI model. It doesn't mean I agree with everything in his series (I need to review it in more details), but this is the direction I'd like to see things taken. Thanks, M.
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c index 02c8408..85dee53 100644 --- a/drivers/acpi/gsi.c +++ b/drivers/acpi/gsi.c @@ -15,11 +15,51 @@ enum acpi_irq_model_id acpi_irq_model; -static struct irq_domain *acpi_irq_domain __read_mostly; +struct gsi_cfg_data { + struct list_head list; + u32 gsi_base; + u32 gsi_end; + struct irq_domain *domain; +}; -void set_acpi_irq_domain(struct irq_domain *domain) +static LIST_HEAD(gsi_cfg_data_list); +static DEFINE_MUTEX(gsi_mutex); + +int gsi_cfg_data_add(struct irq_domain *domain, u32 gsi_base, u32 gsi_end) +{ + struct gsi_cfg_data *data; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->domain = domain; + data->gsi_base = gsi_base; + data->gsi_end = gsi_end; + + mutex_lock(&gsi_mutex); + list_add(&data->list, &gsi_cfg_data_list); + mutex_unlock(&gsi_mutex); + + return 0; +} + +/* Find irqdomain with GSI (hwirq number) */ +static struct irq_domain *acpi_find_irqdomain(u32 gsi) { - acpi_irq_domain = domain; + struct gsi_cfg_data *data; + struct irq_domain *domain = NULL; + + mutex_lock(&gsi_mutex); + list_for_each_entry(data, &gsi_cfg_data_list, list) { + if (gsi >= data->gsi_base && gsi <= data->gsi_end) { + domain = data->domain; + break; + } + } + mutex_unlock(&gsi_mutex); + + return domain; } static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) @@ -53,7 +93,9 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) */ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) { - *irq = irq_find_mapping(acpi_irq_domain, gsi); + struct irq_domain *domain = acpi_find_irqdomain(gsi); + + *irq = irq_find_mapping(domain, gsi); /* * *irq == 0 means no mapping, that should * be reported as a failure @@ -77,12 +119,13 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, { int irq; unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); + struct irq_domain *domain = acpi_find_irqdomain(gsi); - irq = irq_find_mapping(acpi_irq_domain, gsi); + irq = irq_find_mapping(domain, gsi); if (irq > 0) return irq; - irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), + irq = irq_domain_alloc_irqs(domain, 1, dev_to_node(dev), &gsi); if (irq <= 0) return -EINVAL; @@ -101,7 +144,8 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); */ void acpi_unregister_gsi(u32 gsi) { - int irq = irq_find_mapping(acpi_irq_domain, gsi); + struct irq_domain *domain = acpi_find_irqdomain(gsi); + int irq = irq_find_mapping(domain, gsi); irq_dispose_mapping(irq); } diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index a11c020..21a7404 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -947,6 +947,7 @@ static LIST_HEAD(redist_list); static struct redist_region *redist_regs __initdata; static u32 nr_redist_regions __initdata; static phys_addr_t dist_phy_base __initdata; +static u32 gsi_base __initdata; static int __init gic_acpi_register_redist(u64 phys_base, u64 size) @@ -1014,6 +1015,7 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, if (BAD_MADT_ENTRY(dist, end)) return -EINVAL; + gsi_base = dist->global_irq_base; dist_phy_base = dist->base_address; return 0; } @@ -1168,7 +1170,7 @@ init_base: if (err) goto out_release_redist; - set_acpi_irq_domain(gic_data.domain); + gsi_cfg_data_add(gic_data.domain, gsi_base, gsi_base + gic_data.irq_nr); return 0; out_release_redist: diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 7f943ef..15934fb 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1051,6 +1051,7 @@ IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); #ifdef CONFIG_ACPI static phys_addr_t dist_phy_base, cpu_phy_base __initdata; +static u32 gsi_base __initdata; static int __init gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, @@ -1089,6 +1090,7 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, if (BAD_MADT_ENTRY(dist, end)) return -EINVAL; + gsi_base = dist->global_irq_base; dist_phy_base = dist->base_address; return 0; } @@ -1139,7 +1141,10 @@ gic_v2_acpi_init(struct acpi_table_header *table) } gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); - set_acpi_irq_domain(gic_data[0].domain); + + /* since we have only one GICD, we can safely use gic_data[0] here */ + gsi_cfg_data_add(gic_data[0].domain, gsi_base, + gsi_base + gic_data[0].gic_irqs); acpi_irq_model = ACPI_IRQ_MODEL_GIC; return 0; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 6de4b0e..8154359 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -223,7 +223,7 @@ void acpi_unregister_gsi (u32 gsi); #ifdef CONFIG_ACPI_GENERIC_GSI struct irq_domain; -void set_acpi_irq_domain(struct irq_domain *domain); +int gsi_cfg_data_add(struct irq_domain *domain, u32 gsi_base, u32 gsi_end); #endif struct pci_dev;