| Message ID | 1479074375-2629-3-git-send-email-agustinv@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi Agustin, On Sun, Nov 13, 2016 at 04:59:34PM -0500, Agustin Vega-Frias wrote: > When an Extended IRQ Resource contains a valid ResourceSource > use it to map the IRQ on the domain associated with the ACPI > device referenced. > > With this in place an irqchip driver can create its domain using > irq_domain_create_linear and pass the device fwnode to create > the domain mapping. When dependent devices are probed these > changes allow the ACPI core find the domain and map the IRQ. > > Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> > --- > drivers/acpi/Makefile | 2 +- > drivers/acpi/{gsi.c => irq.c} | 98 +++++++++++++++++++++++++++++++++++++------ > drivers/acpi/resource.c | 29 +++++++------ > include/linux/acpi.h | 19 +++++++++ > 4 files changed, 121 insertions(+), 27 deletions(-) > rename drivers/acpi/{gsi.c => irq.c} (53%) It looks to me the direction is the right one but I have a question for you and others below. > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 9ed0878..a391bbc 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS) += debugfs.o > acpi-$(CONFIG_ACPI_NUMA) += numa.o > acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o > acpi-y += acpi_lpat.o > -acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o > +acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o > acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o > > # These are (potentially) separate modules > diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c > similarity index 53% > rename from drivers/acpi/gsi.c > rename to drivers/acpi/irq.c > index ee9e0f2..c6ecaab 100644 > --- a/drivers/acpi/gsi.c > +++ b/drivers/acpi/irq.c > @@ -18,6 +18,45 @@ > static struct fwnode_handle *acpi_gsi_domain_id; > > /** > + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given > + * acpi_resource_source which is used > + * to be used as an IRQ domain id > + * @source: acpi_resource_source to use for the lookup > + * > + * Returns: The appropriate IRQ fwhandle domain id > + * NULL on failure > + */ > +struct fwnode_handle * > +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source) > +{ > + struct fwnode_handle *result; > + struct acpi_device *device; > + acpi_handle handle; > + acpi_status status; > + > + if (!source->string_length) > + return acpi_gsi_domain_id; > + > + status = acpi_get_handle(NULL, source->string_ptr, &handle); > + if (ACPI_FAILURE(status)) { > + pr_warn("Could not find handle for %s\n", source->string_ptr); > + return NULL; > + } > + > + device = acpi_bus_get_acpi_device(handle); > + if (!device) { > + pr_warn("Could not get device for %s\n", source->string_ptr); > + return NULL; > + } > + > + result = &device->fwnode; > + acpi_bus_put_acpi_device(device); > + > + return result; > +} > +EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle); > + > +/** > * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI > * @gsi: GSI IRQ number to map > * @irq: pointer where linux IRQ number is stored > @@ -42,6 +81,50 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); > > /** > + * acpi_register_irq() - Map a hardware to a linux IRQ number > + * @source: IRQ source > + * @hwirq: Hardware IRQ number > + * @trigger: trigger type of the IRQ number to be mapped > + * @polarity: polarity of the IRQ to be mapped > + * > + * Returns: a valid linux IRQ number on success > + * -EINVAL on failure Nit: You need to update the return values list. > + */ > +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger, > + int polarity) > +{ > + struct irq_fwspec fwspec; > + > + if (!source) > + return -EINVAL; > + > + if (irq_find_matching_fwnode(source, DOMAIN_BUS_ANY) == NULL) > + return -EPROBE_DEFER; > + > + fwspec.fwnode = source; > + fwspec.param[0] = hwirq; > + fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity); > + fwspec.param_count = 2; > + > + return irq_create_fwspec_mapping(&fwspec); > +} > +EXPORT_SYMBOL_GPL(acpi_register_irq); > + > +/** > + * acpi_unregister_irq() - Free a Hardware IRQ<->linux IRQ number mapping > + * @hwirq: Hardware IRQ number > + */ > +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq) > +{ > + struct irq_domain *d = irq_find_matching_fwnode(source, > + DOMAIN_BUS_ANY); > + int irq = irq_find_mapping(d, hwirq); > + > + irq_dispose_mapping(irq); > +} > +EXPORT_SYMBOL_GPL(acpi_unregister_irq); > + > +/** > * acpi_register_gsi() - Map a GSI to a linux IRQ number > * @dev: device for which IRQ has to be mapped > * @gsi: GSI IRQ number > @@ -54,19 +137,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, > int polarity) > { > - struct irq_fwspec fwspec; > - > if (WARN_ON(!acpi_gsi_domain_id)) { > pr_warn("GSI: No registered irqchip, giving up\n"); > return -EINVAL; > } > > - fwspec.fwnode = acpi_gsi_domain_id; > - fwspec.param[0] = gsi; > - fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity); > - fwspec.param_count = 2; > - > - return irq_create_fwspec_mapping(&fwspec); > + return acpi_register_irq(acpi_gsi_domain_id, gsi, trigger, polarity); > } > EXPORT_SYMBOL_GPL(acpi_register_gsi); > > @@ -76,11 +152,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, > */ > void acpi_unregister_gsi(u32 gsi) > { > - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id, > - DOMAIN_BUS_ANY); > - int irq = irq_find_mapping(d, gsi); > - > - irq_dispose_mapping(irq); > + acpi_unregister_irq(acpi_gsi_domain_id, gsi); > } > EXPORT_SYMBOL_GPL(acpi_unregister_gsi); > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index 4beda15..83cff00 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -374,21 +374,22 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity) > } > EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type); > > -static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi) > +static void acpi_dev_irqresource_disabled(struct resource *res, u32 hwirq) > { > - res->start = gsi; > - res->end = gsi; > + res->start = hwirq; > + res->end = hwirq; > res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET; > } > > -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, > +static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq, > + struct fwnode_handle *source, > u8 triggering, u8 polarity, u8 shareable, > bool legacy) > { > int irq, p, t; > > - if (!valid_IRQ(gsi)) { > - acpi_dev_irqresource_disabled(res, gsi); > + if (!source && !valid_IRQ(hwirq)) { > + acpi_dev_irqresource_disabled(res, hwirq); > return; > } > > @@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, > * using extended IRQ descriptors we take the IRQ configuration > * from _CRS directly. > */ > - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) { > + if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) { > u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; > u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; > > if (triggering != trig || polarity != pol) { > - pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi, > - t ? "level" : "edge", p ? "low" : "high"); > + pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq, > + t ? "level" : "edge", p ? "low" : "high"); > triggering = trig; > polarity = pol; > } > } > > res->flags = acpi_dev_irq_flags(triggering, polarity, shareable); > - irq = acpi_register_gsi(NULL, gsi, triggering, polarity); > + irq = acpi_register_irq(source, hwirq, triggering, polarity); > if (irq >= 0) { > res->start = irq; > res->end = irq; > } else { > - acpi_dev_irqresource_disabled(res, gsi); > + acpi_dev_irqresource_disabled(res, hwirq); > } > } > > @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > { > struct acpi_resource_irq *irq; > struct acpi_resource_extended_irq *ext_irq; > + struct fwnode_handle *src; > > switch (ares->type) { > case ACPI_RESOURCE_TYPE_IRQ: > @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > acpi_dev_irqresource_disabled(res, 0); > return false; > } > - acpi_dev_get_irqresource(res, irq->interrupts[index], > + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, > irq->triggering, irq->polarity, > irq->sharable, true); > break; > @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > acpi_dev_irqresource_disabled(res, 0); > return false; > } > - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], > + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); Is there a reason why we need to do the domain look-up here ? I would like to understand if, by reshuffling the code (and by returning the resource_source to the calling code - somehow), it would be possible to just mirror what the OF code does in of_irq_get(), namely: (1) parse the irq entry -> of_irq_parse_one() (2) look the domain up -> irq_find_host() (3) create the mapping -> irq_create_of_mapping() You wrote the code already, I think it is just a matter of shuffling it around (well, minus returning the resource_source to the caller which is phandle equivalent in DT). You abstracted away (2) and (3) behind acpi_register_irq(), that on anything than does not use ACPI_GENERIC_GSI is just glue code to acpi_register_gsi(). Also, it is not a question on this patch but I ask it here because it is related. On ACPI you are doing the reverse of what is done in DT in platform_get_irq(): - get the resources already parsed -> platform_get_resource() - if they are disabled -> acpi_irq_get() and I think the ordering is tied to my question above because you carry out the domain look up in acpi_dev_resource_interrupt() so that if for any reason it fails the corresponding resource is disabled so that we try to get it again through acpi_irq_get(). I suspect you did it this way to make sure: a) keep the current ACPI IRQ parsing interface changes to a mininum b) avoid changing the behaviour on x86/ia64; in particular, calling acpi_register_gsi() for the _same_ mapping (an IRQ that was already registered at device creation resource parsing) multiple times can trigger issues on x86/ia64 I think that's a reasonable approach but I wanted to get these clarifications, I do not think you are far from getting this done but since it is a significant change I think it is worth discussing the points I raised above because I think the DT code sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ layer perspective (instead of having the domain look-up buried inside the ACPI IRQ resource parsing API). Thanks ! Lorenzo > + acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src, > ext_irq->triggering, ext_irq->polarity, > ext_irq->sharable, false); > break; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 325bdb9..1099b51 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model, > */ > void acpi_unregister_gsi (u32 gsi); > > +#ifdef CONFIG_ACPI_GENERIC_GSI > +struct fwnode_handle * > +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source); > +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger, > + int polarity); > +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq); > +#else > +#define acpi_get_irq_source_fwhandle(source) (NULL) > +static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, > + int trigger, int polarity) > +{ > + return acpi_register_gsi(NULL, hwirq, trigger, polarity); > +} > +static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq) > +{ > + acpi_unregister_gsi(hwirq); > +} > +#endif > + > struct pci_dev; > > int acpi_pci_irq_enable (struct pci_dev *dev); > -- > Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Agustin, On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: [...] > > @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > > { > > struct acpi_resource_irq *irq; > > struct acpi_resource_extended_irq *ext_irq; > > + struct fwnode_handle *src; > > > > switch (ares->type) { > > case ACPI_RESOURCE_TYPE_IRQ: > > @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > > acpi_dev_irqresource_disabled(res, 0); > > return false; > > } > > - acpi_dev_get_irqresource(res, irq->interrupts[index], > > + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, > > irq->triggering, irq->polarity, > > irq->sharable, true); > > break; > > @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > > acpi_dev_irqresource_disabled(res, 0); > > return false; > > } > > - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], > > + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); > > Is there a reason why we need to do the domain look-up here ? > > I would like to understand if, by reshuffling the code (and by returning > the resource_source to the calling code - somehow), it would be possible > to just mirror what the OF code does in of_irq_get(), namely: > > (1) parse the irq entry -> of_irq_parse_one() > (2) look the domain up -> irq_find_host() > (3) create the mapping -> irq_create_of_mapping() > > You wrote the code already, I think it is just a matter of shuffling > it around (well, minus returning the resource_source to the caller > which is phandle equivalent in DT). > > You abstracted away (2) and (3) behind acpi_register_irq(), that > on anything than does not use ACPI_GENERIC_GSI is just glue code > to acpi_register_gsi(). > > Also, it is not a question on this patch but I ask it here because it > is related. On ACPI you are doing the reverse of what is done in > DT in platform_get_irq(): > > - get the resources already parsed -> platform_get_resource() > - if they are disabled -> acpi_irq_get() > > and I think the ordering is tied to my question above because > you carry out the domain look up in acpi_dev_resource_interrupt() > so that if for any reason it fails the corresponding resource > is disabled so that we try to get it again through acpi_irq_get(). > > I suspect you did it this way to make sure: > > a) keep the current ACPI IRQ parsing interface changes to a mininum > b) avoid changing the behaviour on x86/ia64; in particular, calling > acpi_register_gsi() for the _same_ mapping (an IRQ that was already > registered at device creation resource parsing) multiple times can > trigger issues on x86/ia64 > > I think that's a reasonable approach but I wanted to get these > clarifications, I do not think you are far from getting this > done but since it is a significant change I think it is worth > discussing the points I raised above because I think the DT code > sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ > layer perspective (instead of having the domain look-up buried > inside the ACPI IRQ resource parsing API). I had another look and to achieve the above one way of doing that is to implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for !ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI we would fall back to current solution for ACPI). Within acpi_irq_get() you can easily carry out the same steps (1->2->3) above in ACPI you have the code already there I think it is easy to change the acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and the interface would become identical to of_irq_get() that is an advantage to maintain it from an IRQ maintainership perspective I think, that's my opinion. There is still a nagging snag though. When platform devices are created, core ACPI code parse the resources through: acpi_dev_get_resources() and we _have_ to have way to avoid initializing IRQ resources that have a dependency (ie there is a resource_source pointer that is valid in their descriptors) that's easy to do if we think that's the right thing to do and can hardly break current code (which ignores the resource_source altogether). It is an important difference with DT probing, where the IRQ resources are only created if the domain reference (ie interrupt controller phandle) is satisfied at of_device_alloc() time (see of_device_alloc()). Thoughts ? Please let me know, the code to implement what I say is already in these patches, it is just a matter of reshuffling it. Thanks ! Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, Can you chime in on Lorenzo's feedback and the discussion below? It would be great if you can comment on the reason ACPI does things in a certain way. Hi Lorenzo, On 2016-11-25 06:40, Lorenzo Pieralisi wrote: > Hi Agustin, > > On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: > > [...] > >> > @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> > { >> > struct acpi_resource_irq *irq; >> > struct acpi_resource_extended_irq *ext_irq; >> > + struct fwnode_handle *src; >> > >> > switch (ares->type) { >> > case ACPI_RESOURCE_TYPE_IRQ: >> > @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> > acpi_dev_irqresource_disabled(res, 0); >> > return false; >> > } >> > - acpi_dev_get_irqresource(res, irq->interrupts[index], >> > + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, >> > irq->triggering, irq->polarity, >> > irq->sharable, true); >> > break; >> > @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> > acpi_dev_irqresource_disabled(res, 0); >> > return false; >> > } >> > - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >> > + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >> >> Is there a reason why we need to do the domain look-up here ? Because we need to pass the resource down to acpi_dev_get_irqresource which does the mapping through acpi_register_irq/acpi_register_gsi. >> >> I would like to understand if, by reshuffling the code (and by >> returning >> the resource_source to the calling code - somehow), it would be >> possible >> to just mirror what the OF code does in of_irq_get(), namely: >> >> (1) parse the irq entry -> of_irq_parse_one() >> (2) look the domain up -> irq_find_host() >> (3) create the mapping -> irq_create_of_mapping() >> >> You wrote the code already, I think it is just a matter of shuffling >> it around (well, minus returning the resource_source to the caller >> which is phandle equivalent in DT). This is one area in which DT and ACPI are fundamentally different. In DT once the flattened blob is expanded the data is fixed. In ACPI the data returned by a method can change. In reality most methods like CRS return constants, but given that per-spec they are methods the interpreter has to be involved, which makes it an expensive operation. I believe that is the reason the resource parsing code in ACPI attempts all mappings during the bus scan. Rafael can you comment on this? One way to do what you suggest would be to defer IRQ mapping by, e.g., populating res->start with the HW IRQ number and res->end with the fwnode. That way we can avoid having to walk the resource buffer when a mapping is needed. I don't think that approach would deviate much more from the spec from what the current ahead-of-time mapping does, but it would require more changes in the core code. An alternative would be to do that only for resources that fail to map. >> >> You abstracted away (2) and (3) behind acpi_register_irq(), that >> on anything than does not use ACPI_GENERIC_GSI is just glue code >> to acpi_register_gsi(). >> >> Also, it is not a question on this patch but I ask it here because it >> is related. On ACPI you are doing the reverse of what is done in >> DT in platform_get_irq(): >> >> - get the resources already parsed -> platform_get_resource() >> - if they are disabled -> acpi_irq_get() >> >> and I think the ordering is tied to my question above because >> you carry out the domain look up in acpi_dev_resource_interrupt() >> so that if for any reason it fails the corresponding resource >> is disabled so that we try to get it again through acpi_irq_get(). >> >> I suspect you did it this way to make sure: >> >> a) keep the current ACPI IRQ parsing interface changes to a mininum >> b) avoid changing the behaviour on x86/ia64; in particular, calling >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >> registered at device creation resource parsing) multiple times can >> trigger issues on x86/ia64 You are correct about my reasons. I wanted to keep ACPI core code changes to a minimum, and I also needed to work within the current implementation which uses the pre-converted IRQ resources. >> >> I think that's a reasonable approach but I wanted to get these >> clarifications, I do not think you are far from getting this >> done but since it is a significant change I think it is worth >> discussing the points I raised above because I think the DT code >> sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >> layer perspective (instead of having the domain look-up buried >> inside the ACPI IRQ resource parsing API). > > I had another look and to achieve the above one way of doing that is to > implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for > !ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI > we would fall back to current solution for ACPI). Within acpi_irq_get() > you can easily carry out the same steps (1->2->3) above in ACPI you > have > the code already there I think it is easy to change the > acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and > the interface would become identical to of_irq_get() that is an > advantage to maintain it from an IRQ maintainership perspective I > think, > that's my opinion. I think I get what you mean. I'll take a stab at implementing acpi_irq_get() in the way you suggest. > > There is still a nagging snag though. When platform devices are > created, core ACPI code parse the resources through: > > acpi_dev_get_resources() > > and we _have_ to have way to avoid initializing IRQ resources that > have a dependency (ie there is a resource_source pointer that is valid > in their descriptors) that's easy to do if we think that's the right > thing to do and can hardly break current code (which ignores the > resource_source altogether). I'd rather keep the core code as-is with regard to the ahead-of-time conversion. Whether a resource source is available at the time of the bus scan should be transparent to the code in drivers/acpi/resource.c, and we need the initialization as a disabled resource to signal the need to retry anyway. Rafael, do you have any other suggestions/feedback on how to go about doing this? Thanks, Agustin > > It is an important difference with DT probing, where the IRQ > resources are only created if the domain reference (ie interrupt > controller phandle) is satisfied at of_device_alloc() time > (see of_device_alloc()). > > Thoughts ? Please let me know, the code to implement what I say > is already in these patches, it is just a matter of reshuffling it. > > Thanks ! > Lorenzo
Hi Agustin, On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: > Hi Rafael, > > Can you chime in on Lorenzo's feedback and the discussion below? > It would be great if you can comment on the reason ACPI does things > in a certain way. > > Hi Lorenzo, > > On 2016-11-25 06:40, Lorenzo Pieralisi wrote: > >Hi Agustin, > > > >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: > > > >[...] > > > >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > >>> { > >>> struct acpi_resource_irq *irq; > >>> struct acpi_resource_extended_irq *ext_irq; > >>> + struct fwnode_handle *src; > >>> > >>> switch (ares->type) { > >>> case ACPI_RESOURCE_TYPE_IRQ: > >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > >>> acpi_dev_irqresource_disabled(res, 0); > >>> return false; > >>> } > >>> - acpi_dev_get_irqresource(res, irq->interrupts[index], > >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, > >>> irq->triggering, irq->polarity, > >>> irq->sharable, true); > >>> break; > >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > >>> acpi_dev_irqresource_disabled(res, 0); > >>> return false; > >>> } > >>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], > >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); > >> > >>Is there a reason why we need to do the domain look-up here ? > > Because we need to pass the resource down to acpi_dev_get_irqresource > which does the mapping through acpi_register_irq/acpi_register_gsi. > > >> > >>I would like to understand if, by reshuffling the code (and by > >>returning > >>the resource_source to the calling code - somehow), it would be > >>possible > >>to just mirror what the OF code does in of_irq_get(), namely: > >> > >>(1) parse the irq entry -> of_irq_parse_one() > >>(2) look the domain up -> irq_find_host() > >>(3) create the mapping -> irq_create_of_mapping() > >> > >>You wrote the code already, I think it is just a matter of shuffling > >>it around (well, minus returning the resource_source to the caller > >>which is phandle equivalent in DT). > > This is one area in which DT and ACPI are fundamentally different. In DT > once the flattened blob is expanded the data is fixed. In ACPI the data > returned by a method can change. In reality most methods like CRS return > constants, but given that per-spec they are methods the interpreter has > to be involved, which makes it an expensive operation. I believe that is > the reason the resource parsing code in ACPI attempts all mappings > during > the bus scan. Rafael can you comment on this? > > One way to do what you suggest would be to defer IRQ mapping by, e.g., > populating res->start with the HW IRQ number and res->end with the > fwnode. > That way we can avoid having to walk the resource buffer when a mapping > is needed. I don't think that approach would deviate much more from > the spec from what the current ahead-of-time mapping does, but it would > require more changes in the core code. An alternative would be to do > that only for resources that fail to map. > > >> > >>You abstracted away (2) and (3) behind acpi_register_irq(), that > >>on anything than does not use ACPI_GENERIC_GSI is just glue code > >>to acpi_register_gsi(). > >> > >>Also, it is not a question on this patch but I ask it here because it > >>is related. On ACPI you are doing the reverse of what is done in > >>DT in platform_get_irq(): > >> > >>- get the resources already parsed -> platform_get_resource() > >>- if they are disabled -> acpi_irq_get() > >> > >>and I think the ordering is tied to my question above because > >>you carry out the domain look up in acpi_dev_resource_interrupt() > >>so that if for any reason it fails the corresponding resource > >>is disabled so that we try to get it again through acpi_irq_get(). > >> > >>I suspect you did it this way to make sure: > >> > >>a) keep the current ACPI IRQ parsing interface changes to a mininum > >>b) avoid changing the behaviour on x86/ia64; in particular, calling > >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already > >> registered at device creation resource parsing) multiple times can > >> trigger issues on x86/ia64 > > You are correct about my reasons. I wanted to keep ACPI core code > changes > to a minimum, and I also needed to work within the current > implementation > which uses the pre-converted IRQ resources. > > >> > >>I think that's a reasonable approach but I wanted to get these > >>clarifications, I do not think you are far from getting this > >>done but since it is a significant change I think it is worth > >>discussing the points I raised above because I think the DT code > >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ > >>layer perspective (instead of having the domain look-up buried > >>inside the ACPI IRQ resource parsing API). > > > >I had another look and to achieve the above one way of doing that is to > >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for > >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI > >we would fall back to current solution for ACPI). Within acpi_irq_get() > >you can easily carry out the same steps (1->2->3) above in ACPI > >you have > >the code already there I think it is easy to change the > >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and > >the interface would become identical to of_irq_get() that is an > >advantage to maintain it from an IRQ maintainership perspective I > >think, > >that's my opinion. > > I think I get what you mean. I'll take a stab at implementing > acpi_irq_get() > in the way you suggest. > > > > >There is still a nagging snag though. When platform devices are > >created, core ACPI code parse the resources through: > > > >acpi_dev_get_resources() > > > >and we _have_ to have way to avoid initializing IRQ resources that > >have a dependency (ie there is a resource_source pointer that is valid > >in their descriptors) that's easy to do if we think that's the right > >thing to do and can hardly break current code (which ignores the > >resource_source altogether). > > I'd rather keep the core code as-is with regard to the ahead-of-time > conversion. Whether a resource source is available at the time of > the bus > scan should be transparent to the code in drivers/acpi/resource.c, and > we need the initialization as a disabled resource to signal the need > to retry anyway. Yes, exactly that's the nub. Your current code works, I am trying to make it more modular and similar to the DT/irqdomain IRQ look-up path, which has its advantages. There are two options IMHO: - always disable the resource if it has a resource_source dependency and defer its parsing to acpi_irq_get() (where you can easily implement steps 1-2-3 above). What I wanted to say is that, by disabling the resource if it has a resource_source dependency you can't break x86/ia64 (it is ignored at present - hopefully there is nothing that we are not aware of behind that choice). On x86/ia64 acpi_irq_get() would be an empty stub. This way you would keep the irqdomain look-up out of the ACPI resource parsing API, correct ? - keep code as-is Your point on _CRS being _current_ resource setting is perfectly valid so platform_get_resource() in platform_get_irq() must always take precedence over acpi_irq_get() (which should just apply to disabled resources), I am not sure that doing it the other way around is safe. > Rafael, do you have any other suggestions/feedback on how to go about > doing this? Yes, comments very appreciated, these changes are not trivial and need agreement. Thanks, Lorenzo > > Thanks, > Agustin > > > > >It is an important difference with DT probing, where the IRQ > >resources are only created if the domain reference (ie interrupt > >controller phandle) is satisfied at of_device_alloc() time > >(see of_device_alloc()). > > > >Thoughts ? Please let me know, the code to implement what I say > >is already in these patches, it is just a matter of reshuffling it. > > > >Thanks ! > >Lorenzo > > -- > Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm > Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a > Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > Hi Agustin, > > On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: >> Hi Rafael, >> >> Can you chime in on Lorenzo's feedback and the discussion below? >> It would be great if you can comment on the reason ACPI does things >> in a certain way. >> >> Hi Lorenzo, >> >> On 2016-11-25 06:40, Lorenzo Pieralisi wrote: >> >Hi Agustin, >> > >> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: >> > >> >[...] >> > >> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> { >> >>> struct acpi_resource_irq *irq; >> >>> struct acpi_resource_extended_irq *ext_irq; >> >>> + struct fwnode_handle *src; >> >>> >> >>> switch (ares->type) { >> >>> case ACPI_RESOURCE_TYPE_IRQ: >> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> acpi_dev_irqresource_disabled(res, 0); >> >>> return false; >> >>> } >> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index], >> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, >> >>> irq->triggering, irq->polarity, >> >>> irq->sharable, true); >> >>> break; >> >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> acpi_dev_irqresource_disabled(res, 0); >> >>> return false; >> >>> } >> >>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >> >> >> >>Is there a reason why we need to do the domain look-up here ? >> >> Because we need to pass the resource down to acpi_dev_get_irqresource >> which does the mapping through acpi_register_irq/acpi_register_gsi. >> >> >> >> >>I would like to understand if, by reshuffling the code (and by >> >>returning >> >>the resource_source to the calling code - somehow), it would be >> >>possible >> >>to just mirror what the OF code does in of_irq_get(), namely: >> >> >> >>(1) parse the irq entry -> of_irq_parse_one() >> >>(2) look the domain up -> irq_find_host() >> >>(3) create the mapping -> irq_create_of_mapping() >> >> >> >>You wrote the code already, I think it is just a matter of shuffling >> >>it around (well, minus returning the resource_source to the caller >> >>which is phandle equivalent in DT). >> >> This is one area in which DT and ACPI are fundamentally different. In DT >> once the flattened blob is expanded the data is fixed. In ACPI the data >> returned by a method can change. In reality most methods like CRS return >> constants, but given that per-spec they are methods the interpreter has >> to be involved, which makes it an expensive operation. I believe that is >> the reason the resource parsing code in ACPI attempts all mappings >> during >> the bus scan. Rafael can you comment on this? >> >> One way to do what you suggest would be to defer IRQ mapping by, e.g., >> populating res->start with the HW IRQ number and res->end with the >> fwnode. >> That way we can avoid having to walk the resource buffer when a mapping >> is needed. I don't think that approach would deviate much more from >> the spec from what the current ahead-of-time mapping does, but it would >> require more changes in the core code. An alternative would be to do >> that only for resources that fail to map. >> >> >> >> >>You abstracted away (2) and (3) behind acpi_register_irq(), that >> >>on anything than does not use ACPI_GENERIC_GSI is just glue code >> >>to acpi_register_gsi(). >> >> >> >>Also, it is not a question on this patch but I ask it here because it >> >>is related. On ACPI you are doing the reverse of what is done in >> >>DT in platform_get_irq(): >> >> >> >>- get the resources already parsed -> platform_get_resource() >> >>- if they are disabled -> acpi_irq_get() >> >> >> >>and I think the ordering is tied to my question above because >> >>you carry out the domain look up in acpi_dev_resource_interrupt() >> >>so that if for any reason it fails the corresponding resource >> >>is disabled so that we try to get it again through acpi_irq_get(). >> >> >> >>I suspect you did it this way to make sure: >> >> >> >>a) keep the current ACPI IRQ parsing interface changes to a mininum >> >>b) avoid changing the behaviour on x86/ia64; in particular, calling >> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >> >> registered at device creation resource parsing) multiple times can >> >> trigger issues on x86/ia64 >> >> You are correct about my reasons. I wanted to keep ACPI core code >> changes >> to a minimum, and I also needed to work within the current >> implementation >> which uses the pre-converted IRQ resources. >> >> >> >> >>I think that's a reasonable approach but I wanted to get these >> >>clarifications, I do not think you are far from getting this >> >>done but since it is a significant change I think it is worth >> >>discussing the points I raised above because I think the DT code >> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >> >>layer perspective (instead of having the domain look-up buried >> >>inside the ACPI IRQ resource parsing API). >> > >> >I had another look and to achieve the above one way of doing that is to >> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for >> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI >> >we would fall back to current solution for ACPI). Within acpi_irq_get() >> >you can easily carry out the same steps (1->2->3) above in ACPI >> >you have >> >the code already there I think it is easy to change the >> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and >> >the interface would become identical to of_irq_get() that is an >> >advantage to maintain it from an IRQ maintainership perspective I >> >think, >> >that's my opinion. >> >> I think I get what you mean. I'll take a stab at implementing >> acpi_irq_get() >> in the way you suggest. >> >> > >> >There is still a nagging snag though. When platform devices are >> >created, core ACPI code parse the resources through: >> > >> >acpi_dev_get_resources() >> > >> >and we _have_ to have way to avoid initializing IRQ resources that >> >have a dependency (ie there is a resource_source pointer that is valid >> >in their descriptors) that's easy to do if we think that's the right >> >thing to do and can hardly break current code (which ignores the >> >resource_source altogether). >> >> I'd rather keep the core code as-is with regard to the ahead-of-time >> conversion. Whether a resource source is available at the time of >> the bus >> scan should be transparent to the code in drivers/acpi/resource.c, and >> we need the initialization as a disabled resource to signal the need >> to retry anyway. > > Yes, exactly that's the nub. Your current code works, I am trying to > make it more modular and similar to the DT/irqdomain IRQ look-up path, > which has its advantages. > > There are two options IMHO: > > - always disable the resource if it has a resource_source dependency and defer > its parsing to acpi_irq_get() (where you can easily implement steps 1-2-3 above). > What I wanted to say is that, by disabling the resource if it has a > resource_source dependency you can't break x86/ia64 (it is ignored at > present - hopefully there is nothing that we are not aware of behind > that choice). On x86/ia64 acpi_irq_get() would be an empty stub. > This way you would keep the irqdomain look-up out of the ACPI resource > parsing API, correct ? > - keep code as-is > > Your point on _CRS being _current_ resource setting is perfectly valid > so platform_get_resource() in platform_get_irq() must always take > precedence over acpi_irq_get() (which should just apply to disabled > resources), I am not sure that doing it the other way around is safe. > >> Rafael, do you have any other suggestions/feedback on how to go about >> doing this? > > Yes, comments very appreciated, these changes are not trivial and need > agreement. So I need more time. But basically, _CRS can't really change on the fly AFAICS. I'm not even sure it is valid for it to change at all after the first evaluation if _SRS/_PRS are not present. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lorenzo, On 2016-11-29 07:11, Lorenzo Pieralisi wrote: > Hi Agustin, > > On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: >> Hi Rafael, >> >> Can you chime in on Lorenzo's feedback and the discussion below? >> It would be great if you can comment on the reason ACPI does things >> in a certain way. >> >> Hi Lorenzo, >> >> On 2016-11-25 06:40, Lorenzo Pieralisi wrote: >> >Hi Agustin, >> > >> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: >> > >> >[...] >> > >> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> { >> >>> struct acpi_resource_irq *irq; >> >>> struct acpi_resource_extended_irq *ext_irq; >> >>> + struct fwnode_handle *src; >> >>> >> >>> switch (ares->type) { >> >>> case ACPI_RESOURCE_TYPE_IRQ: >> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> acpi_dev_irqresource_disabled(res, 0); >> >>> return false; >> >>> } >> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index], >> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, >> >>> irq->triggering, irq->polarity, >> >>> irq->sharable, true); >> >>> break; >> >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >> >>> acpi_dev_irqresource_disabled(res, 0); >> >>> return false; >> >>> } >> >>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >> >> >> >>Is there a reason why we need to do the domain look-up here ? >> >> Because we need to pass the resource down to acpi_dev_get_irqresource >> which does the mapping through acpi_register_irq/acpi_register_gsi. >> >> >> >> >>I would like to understand if, by reshuffling the code (and by >> >>returning >> >>the resource_source to the calling code - somehow), it would be >> >>possible >> >>to just mirror what the OF code does in of_irq_get(), namely: >> >> >> >>(1) parse the irq entry -> of_irq_parse_one() >> >>(2) look the domain up -> irq_find_host() >> >>(3) create the mapping -> irq_create_of_mapping() >> >> >> >>You wrote the code already, I think it is just a matter of shuffling >> >>it around (well, minus returning the resource_source to the caller >> >>which is phandle equivalent in DT). >> >> This is one area in which DT and ACPI are fundamentally different. In >> DT >> once the flattened blob is expanded the data is fixed. In ACPI the >> data >> returned by a method can change. In reality most methods like CRS >> return >> constants, but given that per-spec they are methods the interpreter >> has >> to be involved, which makes it an expensive operation. I believe that >> is >> the reason the resource parsing code in ACPI attempts all mappings >> during >> the bus scan. Rafael can you comment on this? >> >> One way to do what you suggest would be to defer IRQ mapping by, e.g., >> populating res->start with the HW IRQ number and res->end with the >> fwnode. >> That way we can avoid having to walk the resource buffer when a >> mapping >> is needed. I don't think that approach would deviate much more from >> the spec from what the current ahead-of-time mapping does, but it >> would >> require more changes in the core code. An alternative would be to do >> that only for resources that fail to map. >> >> >> >> >>You abstracted away (2) and (3) behind acpi_register_irq(), that >> >>on anything than does not use ACPI_GENERIC_GSI is just glue code >> >>to acpi_register_gsi(). >> >> >> >>Also, it is not a question on this patch but I ask it here because it >> >>is related. On ACPI you are doing the reverse of what is done in >> >>DT in platform_get_irq(): >> >> >> >>- get the resources already parsed -> platform_get_resource() >> >>- if they are disabled -> acpi_irq_get() >> >> >> >>and I think the ordering is tied to my question above because >> >>you carry out the domain look up in acpi_dev_resource_interrupt() >> >>so that if for any reason it fails the corresponding resource >> >>is disabled so that we try to get it again through acpi_irq_get(). >> >> >> >>I suspect you did it this way to make sure: >> >> >> >>a) keep the current ACPI IRQ parsing interface changes to a mininum >> >>b) avoid changing the behaviour on x86/ia64; in particular, calling >> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >> >> registered at device creation resource parsing) multiple times can >> >> trigger issues on x86/ia64 >> >> You are correct about my reasons. I wanted to keep ACPI core code >> changes >> to a minimum, and I also needed to work within the current >> implementation >> which uses the pre-converted IRQ resources. >> >> >> >> >>I think that's a reasonable approach but I wanted to get these >> >>clarifications, I do not think you are far from getting this >> >>done but since it is a significant change I think it is worth >> >>discussing the points I raised above because I think the DT code >> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >> >>layer perspective (instead of having the domain look-up buried >> >>inside the ACPI IRQ resource parsing API). >> > >> >I had another look and to achieve the above one way of doing that is to >> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for >> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI >> >we would fall back to current solution for ACPI). Within acpi_irq_get() >> >you can easily carry out the same steps (1->2->3) above in ACPI >> >you have >> >the code already there I think it is easy to change the >> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and >> >the interface would become identical to of_irq_get() that is an >> >advantage to maintain it from an IRQ maintainership perspective I >> >think, >> >that's my opinion. >> >> I think I get what you mean. I'll take a stab at implementing >> acpi_irq_get() >> in the way you suggest. >> >> > >> >There is still a nagging snag though. When platform devices are >> >created, core ACPI code parse the resources through: >> > >> >acpi_dev_get_resources() >> > >> >and we _have_ to have way to avoid initializing IRQ resources that >> >have a dependency (ie there is a resource_source pointer that is valid >> >in their descriptors) that's easy to do if we think that's the right >> >thing to do and can hardly break current code (which ignores the >> >resource_source altogether). >> >> I'd rather keep the core code as-is with regard to the ahead-of-time >> conversion. Whether a resource source is available at the time of >> the bus >> scan should be transparent to the code in drivers/acpi/resource.c, and >> we need the initialization as a disabled resource to signal the need >> to retry anyway. > > Yes, exactly that's the nub. Your current code works, I am trying to > make it more modular and similar to the DT/irqdomain IRQ look-up path, > which has its advantages. > > There are two options IMHO: > > - always disable the resource if it has a resource_source dependency > and defer > its parsing to acpi_irq_get() (where you can easily implement steps > 1-2-3 above). > What I wanted to say is that, by disabling the resource if it has a > resource_source dependency you can't break x86/ia64 (it is ignored at > present - hopefully there is nothing that we are not aware of behind > that choice). On x86/ia64 acpi_irq_get() would be an empty stub. > This way you would keep the irqdomain look-up out of the ACPI > resource > parsing API, correct ? > - keep code as-is I plan to work on V8 today to address the point you raised w.r.t. making it look more like DT for maintainability. I will leave other things as-is with an understanding that we might revisit based on Rafael's feedback Thanks, Agustin > > Your point on _CRS being _current_ resource setting is perfectly valid > so platform_get_resource() in platform_get_irq() must always take > precedence over acpi_irq_get() (which should just apply to disabled > resources), I am not sure that doing it the other way around is safe. > >> Rafael, do you have any other suggestions/feedback on how to go about >> doing this? > > Yes, comments very appreciated, these changes are not trivial and need > agreement. > > Thanks, > Lorenzo > >> >> Thanks, >> Agustin >> >> > >> >It is an important difference with DT probing, where the IRQ >> >resources are only created if the domain reference (ie interrupt >> >controller phandle) is satisfied at of_device_alloc() time >> >(see of_device_alloc()). >> > >> >Thoughts ? Please let me know, the code to implement what I say >> >is already in these patches, it is just a matter of reshuffling it. >> > >> >Thanks ! >> >Lorenzo >> >> -- >> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm >> Technologies, Inc. >> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a >> Linux Foundation Collaborative Project.
Hi Rafael, On 2016-11-29 07:43, Rafael J. Wysocki wrote: > On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> Hi Agustin, >> >> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: >>> Hi Rafael, >>> >>> Can you chime in on Lorenzo's feedback and the discussion below? >>> It would be great if you can comment on the reason ACPI does things >>> in a certain way. >>> >>> Hi Lorenzo, >>> >>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote: >>> >Hi Agustin, >>> > >>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: >>> > >>> >[...] >>> > >>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >>> >>> { >>> >>> struct acpi_resource_irq *irq; >>> >>> struct acpi_resource_extended_irq *ext_irq; >>> >>> + struct fwnode_handle *src; >>> >>> >>> >>> switch (ares->type) { >>> >>> case ACPI_RESOURCE_TYPE_IRQ: >>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >>> >>> acpi_dev_irqresource_disabled(res, 0); >>> >>> return false; >>> >>> } >>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index], >>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, >>> >>> irq->triggering, irq->polarity, >>> >>> irq->sharable, true); >>> >>> break; >>> >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, >>> >>> acpi_dev_irqresource_disabled(res, 0); >>> >>> return false; >>> >>> } >>> >>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >>> >>> + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >>> >> >>> >>Is there a reason why we need to do the domain look-up here ? >>> >>> Because we need to pass the resource down to acpi_dev_get_irqresource >>> which does the mapping through acpi_register_irq/acpi_register_gsi. >>> >>> >> >>> >>I would like to understand if, by reshuffling the code (and by >>> >>returning >>> >>the resource_source to the calling code - somehow), it would be >>> >>possible >>> >>to just mirror what the OF code does in of_irq_get(), namely: >>> >> >>> >>(1) parse the irq entry -> of_irq_parse_one() >>> >>(2) look the domain up -> irq_find_host() >>> >>(3) create the mapping -> irq_create_of_mapping() >>> >> >>> >>You wrote the code already, I think it is just a matter of shuffling >>> >>it around (well, minus returning the resource_source to the caller >>> >>which is phandle equivalent in DT). >>> >>> This is one area in which DT and ACPI are fundamentally different. In >>> DT >>> once the flattened blob is expanded the data is fixed. In ACPI the >>> data >>> returned by a method can change. In reality most methods like CRS >>> return >>> constants, but given that per-spec they are methods the interpreter >>> has >>> to be involved, which makes it an expensive operation. I believe that >>> is >>> the reason the resource parsing code in ACPI attempts all mappings >>> during >>> the bus scan. Rafael can you comment on this? >>> >>> One way to do what you suggest would be to defer IRQ mapping by, >>> e.g., >>> populating res->start with the HW IRQ number and res->end with the >>> fwnode. >>> That way we can avoid having to walk the resource buffer when a >>> mapping >>> is needed. I don't think that approach would deviate much more from >>> the spec from what the current ahead-of-time mapping does, but it >>> would >>> require more changes in the core code. An alternative would be to do >>> that only for resources that fail to map. >>> >>> >> >>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that >>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code >>> >>to acpi_register_gsi(). >>> >> >>> >>Also, it is not a question on this patch but I ask it here because it >>> >>is related. On ACPI you are doing the reverse of what is done in >>> >>DT in platform_get_irq(): >>> >> >>> >>- get the resources already parsed -> platform_get_resource() >>> >>- if they are disabled -> acpi_irq_get() >>> >> >>> >>and I think the ordering is tied to my question above because >>> >>you carry out the domain look up in acpi_dev_resource_interrupt() >>> >>so that if for any reason it fails the corresponding resource >>> >>is disabled so that we try to get it again through acpi_irq_get(). >>> >> >>> >>I suspect you did it this way to make sure: >>> >> >>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum >>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling >>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >>> >> registered at device creation resource parsing) multiple times can >>> >> trigger issues on x86/ia64 >>> >>> You are correct about my reasons. I wanted to keep ACPI core code >>> changes >>> to a minimum, and I also needed to work within the current >>> implementation >>> which uses the pre-converted IRQ resources. >>> >>> >> >>> >>I think that's a reasonable approach but I wanted to get these >>> >>clarifications, I do not think you are far from getting this >>> >>done but since it is a significant change I think it is worth >>> >>discussing the points I raised above because I think the DT code >>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >>> >>layer perspective (instead of having the domain look-up buried >>> >>inside the ACPI IRQ resource parsing API). >>> > >>> >I had another look and to achieve the above one way of doing that is to >>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for >>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI >>> >we would fall back to current solution for ACPI). Within acpi_irq_get() >>> >you can easily carry out the same steps (1->2->3) above in ACPI >>> >you have >>> >the code already there I think it is easy to change the >>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and >>> >the interface would become identical to of_irq_get() that is an >>> >advantage to maintain it from an IRQ maintainership perspective I >>> >think, >>> >that's my opinion. >>> >>> I think I get what you mean. I'll take a stab at implementing >>> acpi_irq_get() >>> in the way you suggest. >>> >>> > >>> >There is still a nagging snag though. When platform devices are >>> >created, core ACPI code parse the resources through: >>> > >>> >acpi_dev_get_resources() >>> > >>> >and we _have_ to have way to avoid initializing IRQ resources that >>> >have a dependency (ie there is a resource_source pointer that is valid >>> >in their descriptors) that's easy to do if we think that's the right >>> >thing to do and can hardly break current code (which ignores the >>> >resource_source altogether). >>> >>> I'd rather keep the core code as-is with regard to the ahead-of-time >>> conversion. Whether a resource source is available at the time of >>> the bus >>> scan should be transparent to the code in drivers/acpi/resource.c, >>> and >>> we need the initialization as a disabled resource to signal the need >>> to retry anyway. >> >> Yes, exactly that's the nub. Your current code works, I am trying to >> make it more modular and similar to the DT/irqdomain IRQ look-up path, >> which has its advantages. >> >> There are two options IMHO: >> >> - always disable the resource if it has a resource_source dependency >> and defer >> its parsing to acpi_irq_get() (where you can easily implement steps >> 1-2-3 above). >> What I wanted to say is that, by disabling the resource if it has a >> resource_source dependency you can't break x86/ia64 (it is ignored >> at >> present - hopefully there is nothing that we are not aware of behind >> that choice). On x86/ia64 acpi_irq_get() would be an empty stub. >> This way you would keep the irqdomain look-up out of the ACPI >> resource >> parsing API, correct ? >> - keep code as-is >> >> Your point on _CRS being _current_ resource setting is perfectly valid >> so platform_get_resource() in platform_get_irq() must always take >> precedence over acpi_irq_get() (which should just apply to disabled >> resources), I am not sure that doing it the other way around is safe. >> >>> Rafael, do you have any other suggestions/feedback on how to go about >>> doing this? >> >> Yes, comments very appreciated, these changes are not trivial and need >> agreement. > > So I need more time. Please wait for V8 which will address some issues raised by Lorenzo. > > But basically, _CRS can't really change on the fly AFAICS. I'm not > even sure it is valid for it to change at all after the first > evaluation if _SRS/_PRS are not present. That's good to know and it opens more possibilities. Thanks, Agustin > > Thanks, > Rafael
On Tue, Nov 29, 2016 at 4:20 PM, Agustin Vega-Frias <agustinv@codeaurora.org> wrote: > Hi Rafael, > > > On 2016-11-29 07:43, Rafael J. Wysocki wrote: >> >> On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >>> >>> Hi Agustin, >>> >>> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: >>>> >>>> Hi Rafael, >>>> >>>> Can you chime in on Lorenzo's feedback and the discussion below? >>>> It would be great if you can comment on the reason ACPI does things >>>> in a certain way. >>>> >>>> Hi Lorenzo, >>>> >>>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote: >>>> >Hi Agustin, >>>> > >>>> >On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: >>>> > >>>> >[...] >>>> > >>>> >>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct >>>> >>> acpi_resource *ares, int index, >>>> >>> { >>>> >>> struct acpi_resource_irq *irq; >>>> >>> struct acpi_resource_extended_irq *ext_irq; >>>> >>> + struct fwnode_handle *src; >>>> >>> >>>> >>> switch (ares->type) { >>>> >>> case ACPI_RESOURCE_TYPE_IRQ: >>>> >>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct >>>> >>> acpi_resource *ares, int index, >>>> >>> acpi_dev_irqresource_disabled(res, 0); >>>> >>> return false; >>>> >>> } >>>> >>> - acpi_dev_get_irqresource(res, irq->interrupts[index], >>>> >>> + acpi_dev_get_irqresource(res, irq->interrupts[index], >>>> >>> NULL, >>>> >>> irq->triggering, irq->polarity, >>>> >>> irq->sharable, true); >>>> >>> break; >>>> >>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct >>>> >>> acpi_resource *ares, int index, >>>> >>> acpi_dev_irqresource_disabled(res, 0); >>>> >>> return false; >>>> >>> } >>>> >>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >>>> >>> + src = >>>> >>> acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >>>> >> >>>> >>Is there a reason why we need to do the domain look-up here ? >>>> >>>> Because we need to pass the resource down to acpi_dev_get_irqresource >>>> which does the mapping through acpi_register_irq/acpi_register_gsi. >>>> >>>> >> >>>> >>I would like to understand if, by reshuffling the code (and by >>>> >>returning >>>> >>the resource_source to the calling code - somehow), it would be >>>> >>possible >>>> >>to just mirror what the OF code does in of_irq_get(), namely: >>>> >> >>>> >>(1) parse the irq entry -> of_irq_parse_one() >>>> >>(2) look the domain up -> irq_find_host() >>>> >>(3) create the mapping -> irq_create_of_mapping() >>>> >> >>>> >>You wrote the code already, I think it is just a matter of shuffling >>>> >>it around (well, minus returning the resource_source to the caller >>>> >>which is phandle equivalent in DT). >>>> >>>> This is one area in which DT and ACPI are fundamentally different. In DT >>>> once the flattened blob is expanded the data is fixed. In ACPI the data >>>> returned by a method can change. In reality most methods like CRS return >>>> constants, but given that per-spec they are methods the interpreter has >>>> to be involved, which makes it an expensive operation. I believe that is >>>> the reason the resource parsing code in ACPI attempts all mappings >>>> during >>>> the bus scan. Rafael can you comment on this? >>>> >>>> One way to do what you suggest would be to defer IRQ mapping by, e.g., >>>> populating res->start with the HW IRQ number and res->end with the >>>> fwnode. >>>> That way we can avoid having to walk the resource buffer when a mapping >>>> is needed. I don't think that approach would deviate much more from >>>> the spec from what the current ahead-of-time mapping does, but it would >>>> require more changes in the core code. An alternative would be to do >>>> that only for resources that fail to map. >>>> >>>> >> >>>> >>You abstracted away (2) and (3) behind acpi_register_irq(), that >>>> >>on anything than does not use ACPI_GENERIC_GSI is just glue code >>>> >>to acpi_register_gsi(). >>>> >> >>>> >>Also, it is not a question on this patch but I ask it here because it >>>> >>is related. On ACPI you are doing the reverse of what is done in >>>> >>DT in platform_get_irq(): >>>> >> >>>> >>- get the resources already parsed -> platform_get_resource() >>>> >>- if they are disabled -> acpi_irq_get() >>>> >> >>>> >>and I think the ordering is tied to my question above because >>>> >>you carry out the domain look up in acpi_dev_resource_interrupt() >>>> >>so that if for any reason it fails the corresponding resource >>>> >>is disabled so that we try to get it again through acpi_irq_get(). >>>> >> >>>> >>I suspect you did it this way to make sure: >>>> >> >>>> >>a) keep the current ACPI IRQ parsing interface changes to a mininum >>>> >>b) avoid changing the behaviour on x86/ia64; in particular, calling >>>> >> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >>>> >> registered at device creation resource parsing) multiple times can >>>> >> trigger issues on x86/ia64 >>>> >>>> You are correct about my reasons. I wanted to keep ACPI core code >>>> changes >>>> to a minimum, and I also needed to work within the current >>>> implementation >>>> which uses the pre-converted IRQ resources. >>>> >>>> >> >>>> >>I think that's a reasonable approach but I wanted to get these >>>> >>clarifications, I do not think you are far from getting this >>>> >>done but since it is a significant change I think it is worth >>>> >>discussing the points I raised above because I think the DT code >>>> >>sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >>>> >>layer perspective (instead of having the domain look-up buried >>>> >>inside the ACPI IRQ resource parsing API). >>>> > >>>> >I had another look and to achieve the above one way of doing that is to >>>> >implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for >>>> >!ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI >>>> >we would fall back to current solution for ACPI). Within acpi_irq_get() >>>> >you can easily carry out the same steps (1->2->3) above in ACPI >>>> >you have >>>> >the code already there I think it is easy to change the >>>> >acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and >>>> >the interface would become identical to of_irq_get() that is an >>>> >advantage to maintain it from an IRQ maintainership perspective I >>>> >think, >>>> >that's my opinion. >>>> >>>> I think I get what you mean. I'll take a stab at implementing >>>> acpi_irq_get() >>>> in the way you suggest. >>>> >>>> > >>>> >There is still a nagging snag though. When platform devices are >>>> >created, core ACPI code parse the resources through: >>>> > >>>> >acpi_dev_get_resources() >>>> > >>>> >and we _have_ to have way to avoid initializing IRQ resources that >>>> >have a dependency (ie there is a resource_source pointer that is valid >>>> >in their descriptors) that's easy to do if we think that's the right >>>> >thing to do and can hardly break current code (which ignores the >>>> >resource_source altogether). >>>> >>>> I'd rather keep the core code as-is with regard to the ahead-of-time >>>> conversion. Whether a resource source is available at the time of >>>> the bus >>>> scan should be transparent to the code in drivers/acpi/resource.c, and >>>> we need the initialization as a disabled resource to signal the need >>>> to retry anyway. >>> >>> >>> Yes, exactly that's the nub. Your current code works, I am trying to >>> make it more modular and similar to the DT/irqdomain IRQ look-up path, >>> which has its advantages. >>> >>> There are two options IMHO: >>> >>> - always disable the resource if it has a resource_source dependency and >>> defer >>> its parsing to acpi_irq_get() (where you can easily implement steps >>> 1-2-3 above). >>> What I wanted to say is that, by disabling the resource if it has a >>> resource_source dependency you can't break x86/ia64 (it is ignored at >>> present - hopefully there is nothing that we are not aware of behind >>> that choice). On x86/ia64 acpi_irq_get() would be an empty stub. >>> This way you would keep the irqdomain look-up out of the ACPI resource >>> parsing API, correct ? >>> - keep code as-is >>> >>> Your point on _CRS being _current_ resource setting is perfectly valid >>> so platform_get_resource() in platform_get_irq() must always take >>> precedence over acpi_irq_get() (which should just apply to disabled >>> resources), I am not sure that doing it the other way around is safe. >>> >>>> Rafael, do you have any other suggestions/feedback on how to go about >>>> doing this? >>> >>> >>> Yes, comments very appreciated, these changes are not trivial and need >>> agreement. >> >> >> So I need more time. > > > Please wait for V8 which will address some issues raised by Lorenzo. > >> >> But basically, _CRS can't really change on the fly AFAICS. I'm not >> even sure it is valid for it to change at all after the first >> evaluation if _SRS/_PRS are not present. > > > That's good to know and it opens more possibilities. Actually, to me it follows from the very purpose of _CRS that, as long as the device is enabled, it should be expected to return the same data every time it is evaluated, unless _SRS is invoked in the meantime. Otherwise, it would be possible for the device's resources to change unexpectedly under a driver using it. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/11/30 0:26, Rafael J. Wysocki wrote: > On Tue, Nov 29, 2016 at 4:20 PM, Agustin Vega-Frias > <agustinv@codeaurora.org> wrote: >> Hi Rafael, >> >> >> On 2016-11-29 07:43, Rafael J. Wysocki wrote: >>> On Tue, Nov 29, 2016 at 1:11 PM, Lorenzo Pieralisi >>> <lorenzo.pieralisi@arm.com> wrote: >>>> Hi Agustin, >>>> >>>> On Mon, Nov 28, 2016 at 05:40:24PM -0500, Agustin Vega-Frias wrote: >>>>> Hi Rafael, >>>>> >>>>> Can you chime in on Lorenzo's feedback and the discussion below? >>>>> It would be great if you can comment on the reason ACPI does things >>>>> in a certain way. >>>>> >>>>> Hi Lorenzo, >>>>> >>>>> On 2016-11-25 06:40, Lorenzo Pieralisi wrote: >>>>>> Hi Agustin, >>>>>> >>>>>> On Thu, Nov 24, 2016 at 04:15:48PM +0000, Lorenzo Pieralisi wrote: >>>>>> >>>>>> [...] >>>>>> >>>>>>>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct >>>>>>>> acpi_resource *ares, int index, >>>>>>>> { >>>>>>>> struct acpi_resource_irq *irq; >>>>>>>> struct acpi_resource_extended_irq *ext_irq; >>>>>>>> + struct fwnode_handle *src; >>>>>>>> >>>>>>>> switch (ares->type) { >>>>>>>> case ACPI_RESOURCE_TYPE_IRQ: >>>>>>>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct >>>>>>>> acpi_resource *ares, int index, >>>>>>>> acpi_dev_irqresource_disabled(res, 0); >>>>>>>> return false; >>>>>>>> } >>>>>>>> - acpi_dev_get_irqresource(res, irq->interrupts[index], >>>>>>>> + acpi_dev_get_irqresource(res, irq->interrupts[index], >>>>>>>> NULL, >>>>>>>> irq->triggering, irq->polarity, >>>>>>>> irq->sharable, true); >>>>>>>> break; >>>>>>>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct >>>>>>>> acpi_resource *ares, int index, >>>>>>>> acpi_dev_irqresource_disabled(res, 0); >>>>>>>> return false; >>>>>>>> } >>>>>>>> - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], >>>>>>>> + src = >>>>>>>> acpi_get_irq_source_fwhandle(&ext_irq->resource_source); >>>>>>> Is there a reason why we need to do the domain look-up here ? >>>>> Because we need to pass the resource down to acpi_dev_get_irqresource >>>>> which does the mapping through acpi_register_irq/acpi_register_gsi. >>>>> >>>>>>> I would like to understand if, by reshuffling the code (and by >>>>>>> returning >>>>>>> the resource_source to the calling code - somehow), it would be >>>>>>> possible >>>>>>> to just mirror what the OF code does in of_irq_get(), namely: >>>>>>> >>>>>>> (1) parse the irq entry -> of_irq_parse_one() >>>>>>> (2) look the domain up -> irq_find_host() >>>>>>> (3) create the mapping -> irq_create_of_mapping() >>>>>>> >>>>>>> You wrote the code already, I think it is just a matter of shuffling >>>>>>> it around (well, minus returning the resource_source to the caller >>>>>>> which is phandle equivalent in DT). >>>>> This is one area in which DT and ACPI are fundamentally different. In DT >>>>> once the flattened blob is expanded the data is fixed. In ACPI the data >>>>> returned by a method can change. In reality most methods like CRS return >>>>> constants, but given that per-spec they are methods the interpreter has >>>>> to be involved, which makes it an expensive operation. I believe that is >>>>> the reason the resource parsing code in ACPI attempts all mappings >>>>> during >>>>> the bus scan. Rafael can you comment on this? >>>>> >>>>> One way to do what you suggest would be to defer IRQ mapping by, e.g., >>>>> populating res->start with the HW IRQ number and res->end with the >>>>> fwnode. >>>>> That way we can avoid having to walk the resource buffer when a mapping >>>>> is needed. I don't think that approach would deviate much more from >>>>> the spec from what the current ahead-of-time mapping does, but it would >>>>> require more changes in the core code. An alternative would be to do >>>>> that only for resources that fail to map. >>>>> >>>>>>> You abstracted away (2) and (3) behind acpi_register_irq(), that >>>>>>> on anything than does not use ACPI_GENERIC_GSI is just glue code >>>>>>> to acpi_register_gsi(). >>>>>>> >>>>>>> Also, it is not a question on this patch but I ask it here because it >>>>>>> is related. On ACPI you are doing the reverse of what is done in >>>>>>> DT in platform_get_irq(): >>>>>>> >>>>>>> - get the resources already parsed -> platform_get_resource() >>>>>>> - if they are disabled -> acpi_irq_get() >>>>>>> >>>>>>> and I think the ordering is tied to my question above because >>>>>>> you carry out the domain look up in acpi_dev_resource_interrupt() >>>>>>> so that if for any reason it fails the corresponding resource >>>>>>> is disabled so that we try to get it again through acpi_irq_get(). >>>>>>> >>>>>>> I suspect you did it this way to make sure: >>>>>>> >>>>>>> a) keep the current ACPI IRQ parsing interface changes to a mininum >>>>>>> b) avoid changing the behaviour on x86/ia64; in particular, calling >>>>>>> acpi_register_gsi() for the _same_ mapping (an IRQ that was already >>>>>>> registered at device creation resource parsing) multiple times can >>>>>>> trigger issues on x86/ia64 >>>>> You are correct about my reasons. I wanted to keep ACPI core code >>>>> changes >>>>> to a minimum, and I also needed to work within the current >>>>> implementation >>>>> which uses the pre-converted IRQ resources. >>>>> >>>>>>> I think that's a reasonable approach but I wanted to get these >>>>>>> clarifications, I do not think you are far from getting this >>>>>>> done but since it is a significant change I think it is worth >>>>>>> discussing the points I raised above because I think the DT code >>>>>>> sequence in of_irq_get() (1-2-3 above) is cleaner from an IRQ >>>>>>> layer perspective (instead of having the domain look-up buried >>>>>>> inside the ACPI IRQ resource parsing API). >>>>>> I had another look and to achieve the above one way of doing that is to >>>>>> implement acpi_irq_get() only for ACPI_GENERIC_GSI and stub it out for >>>>>> !ACPI_GENERIC_GSI (ie return an error code so that on !ACPI_GENERIC_GSI >>>>>> we would fall back to current solution for ACPI). Within acpi_irq_get() >>>>>> you can easily carry out the same steps (1->2->3) above in ACPI >>>>>> you have >>>>>> the code already there I think it is easy to change the >>>>>> acpi_irq_get_cb() interface to return a filled in struct irq_fwspec and >>>>>> the interface would become identical to of_irq_get() that is an >>>>>> advantage to maintain it from an IRQ maintainership perspective I >>>>>> think, >>>>>> that's my opinion. >>>>> I think I get what you mean. I'll take a stab at implementing >>>>> acpi_irq_get() >>>>> in the way you suggest. >>>>> >>>>>> There is still a nagging snag though. When platform devices are >>>>>> created, core ACPI code parse the resources through: >>>>>> >>>>>> acpi_dev_get_resources() >>>>>> >>>>>> and we _have_ to have way to avoid initializing IRQ resources that >>>>>> have a dependency (ie there is a resource_source pointer that is valid >>>>>> in their descriptors) that's easy to do if we think that's the right >>>>>> thing to do and can hardly break current code (which ignores the >>>>>> resource_source altogether). >>>>> I'd rather keep the core code as-is with regard to the ahead-of-time >>>>> conversion. Whether a resource source is available at the time of >>>>> the bus >>>>> scan should be transparent to the code in drivers/acpi/resource.c, and >>>>> we need the initialization as a disabled resource to signal the need >>>>> to retry anyway. >>>> >>>> Yes, exactly that's the nub. Your current code works, I am trying to >>>> make it more modular and similar to the DT/irqdomain IRQ look-up path, >>>> which has its advantages. >>>> >>>> There are two options IMHO: >>>> >>>> - always disable the resource if it has a resource_source dependency and >>>> defer >>>> its parsing to acpi_irq_get() (where you can easily implement steps >>>> 1-2-3 above). >>>> What I wanted to say is that, by disabling the resource if it has a >>>> resource_source dependency you can't break x86/ia64 (it is ignored at >>>> present - hopefully there is nothing that we are not aware of behind >>>> that choice). On x86/ia64 acpi_irq_get() would be an empty stub. >>>> This way you would keep the irqdomain look-up out of the ACPI resource >>>> parsing API, correct ? >>>> - keep code as-is >>>> >>>> Your point on _CRS being _current_ resource setting is perfectly valid >>>> so platform_get_resource() in platform_get_irq() must always take >>>> precedence over acpi_irq_get() (which should just apply to disabled >>>> resources), I am not sure that doing it the other way around is safe. >>>> >>>>> Rafael, do you have any other suggestions/feedback on how to go about >>>>> doing this? >>>> >>>> Yes, comments very appreciated, these changes are not trivial and need >>>> agreement. >>> >>> So I need more time. >> >> Please wait for V8 which will address some issues raised by Lorenzo. >> >>> But basically, _CRS can't really change on the fly AFAICS. I'm not >>> even sure it is valid for it to change at all after the first >>> evaluation if _SRS/_PRS are not present. >> >> That's good to know and it opens more possibilities. > Actually, to me it follows from the very purpose of _CRS that, as long > as the device is enabled, it should be expected to return the same > data every time it is evaluated, unless _SRS is invoked in the > meantime. Otherwise, it would be possible for the device's resources > to change unexpectedly under a driver using it. Agreed, here is the hotplug case: For hotplug cases, _CRS is updated before the device is enabled. for example for a memory hot add, BIOS will update the _CRS to collect memory address ranges before notify OS, but once the BIOS notified OS to add the memory in (BIOS will enable the device to set the _STA to functional), _CRS can't be updated. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 9ed0878..a391bbc 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS) += debugfs.o acpi-$(CONFIG_ACPI_NUMA) += numa.o acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o acpi-y += acpi_lpat.o -acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o +acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o # These are (potentially) separate modules diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c similarity index 53% rename from drivers/acpi/gsi.c rename to drivers/acpi/irq.c index ee9e0f2..c6ecaab 100644 --- a/drivers/acpi/gsi.c +++ b/drivers/acpi/irq.c @@ -18,6 +18,45 @@ static struct fwnode_handle *acpi_gsi_domain_id; /** + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given + * acpi_resource_source which is used + * to be used as an IRQ domain id + * @source: acpi_resource_source to use for the lookup + * + * Returns: The appropriate IRQ fwhandle domain id + * NULL on failure + */ +struct fwnode_handle * +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source) +{ + struct fwnode_handle *result; + struct acpi_device *device; + acpi_handle handle; + acpi_status status; + + if (!source->string_length) + return acpi_gsi_domain_id; + + status = acpi_get_handle(NULL, source->string_ptr, &handle); + if (ACPI_FAILURE(status)) { + pr_warn("Could not find handle for %s\n", source->string_ptr); + return NULL; + } + + device = acpi_bus_get_acpi_device(handle); + if (!device) { + pr_warn("Could not get device for %s\n", source->string_ptr); + return NULL; + } + + result = &device->fwnode; + acpi_bus_put_acpi_device(device); + + return result; +} +EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle); + +/** * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI * @gsi: GSI IRQ number to map * @irq: pointer where linux IRQ number is stored @@ -42,6 +81,50 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); /** + * acpi_register_irq() - Map a hardware to a linux IRQ number + * @source: IRQ source + * @hwirq: Hardware IRQ number + * @trigger: trigger type of the IRQ number to be mapped + * @polarity: polarity of the IRQ to be mapped + * + * Returns: a valid linux IRQ number on success + * -EINVAL on failure + */ +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger, + int polarity) +{ + struct irq_fwspec fwspec; + + if (!source) + return -EINVAL; + + if (irq_find_matching_fwnode(source, DOMAIN_BUS_ANY) == NULL) + return -EPROBE_DEFER; + + fwspec.fwnode = source; + fwspec.param[0] = hwirq; + fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity); + fwspec.param_count = 2; + + return irq_create_fwspec_mapping(&fwspec); +} +EXPORT_SYMBOL_GPL(acpi_register_irq); + +/** + * acpi_unregister_irq() - Free a Hardware IRQ<->linux IRQ number mapping + * @hwirq: Hardware IRQ number + */ +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq) +{ + struct irq_domain *d = irq_find_matching_fwnode(source, + DOMAIN_BUS_ANY); + int irq = irq_find_mapping(d, hwirq); + + irq_dispose_mapping(irq); +} +EXPORT_SYMBOL_GPL(acpi_unregister_irq); + +/** * acpi_register_gsi() - Map a GSI to a linux IRQ number * @dev: device for which IRQ has to be mapped * @gsi: GSI IRQ number @@ -54,19 +137,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) { - struct irq_fwspec fwspec; - if (WARN_ON(!acpi_gsi_domain_id)) { pr_warn("GSI: No registered irqchip, giving up\n"); return -EINVAL; } - fwspec.fwnode = acpi_gsi_domain_id; - fwspec.param[0] = gsi; - fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity); - fwspec.param_count = 2; - - return irq_create_fwspec_mapping(&fwspec); + return acpi_register_irq(acpi_gsi_domain_id, gsi, trigger, polarity); } EXPORT_SYMBOL_GPL(acpi_register_gsi); @@ -76,11 +152,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, */ void acpi_unregister_gsi(u32 gsi) { - struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id, - DOMAIN_BUS_ANY); - int irq = irq_find_mapping(d, gsi); - - irq_dispose_mapping(irq); + acpi_unregister_irq(acpi_gsi_domain_id, gsi); } EXPORT_SYMBOL_GPL(acpi_unregister_gsi); diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 4beda15..83cff00 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -374,21 +374,22 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity) } EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type); -static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi) +static void acpi_dev_irqresource_disabled(struct resource *res, u32 hwirq) { - res->start = gsi; - res->end = gsi; + res->start = hwirq; + res->end = hwirq; res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET; } -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, +static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq, + struct fwnode_handle *source, u8 triggering, u8 polarity, u8 shareable, bool legacy) { int irq, p, t; - if (!valid_IRQ(gsi)) { - acpi_dev_irqresource_disabled(res, gsi); + if (!source && !valid_IRQ(hwirq)) { + acpi_dev_irqresource_disabled(res, hwirq); return; } @@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi, * using extended IRQ descriptors we take the IRQ configuration * from _CRS directly. */ - if (legacy && !acpi_get_override_irq(gsi, &t, &p)) { + if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) { u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE; u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH; if (triggering != trig || polarity != pol) { - pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi, - t ? "level" : "edge", p ? "low" : "high"); + pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq, + t ? "level" : "edge", p ? "low" : "high"); triggering = trig; polarity = pol; } } res->flags = acpi_dev_irq_flags(triggering, polarity, shareable); - irq = acpi_register_gsi(NULL, gsi, triggering, polarity); + irq = acpi_register_irq(source, hwirq, triggering, polarity); if (irq >= 0) { res->start = irq; res->end = irq; } else { - acpi_dev_irqresource_disabled(res, gsi); + acpi_dev_irqresource_disabled(res, hwirq); } } @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, { struct acpi_resource_irq *irq; struct acpi_resource_extended_irq *ext_irq; + struct fwnode_handle *src; switch (ares->type) { case ACPI_RESOURCE_TYPE_IRQ: @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, acpi_dev_irqresource_disabled(res, 0); return false; } - acpi_dev_get_irqresource(res, irq->interrupts[index], + acpi_dev_get_irqresource(res, irq->interrupts[index], NULL, irq->triggering, irq->polarity, irq->sharable, true); break; @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, acpi_dev_irqresource_disabled(res, 0); return false; } - acpi_dev_get_irqresource(res, ext_irq->interrupts[index], + src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source); + acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src, ext_irq->triggering, ext_irq->polarity, ext_irq->sharable, false); break; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 325bdb9..1099b51 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model, */ void acpi_unregister_gsi (u32 gsi); +#ifdef CONFIG_ACPI_GENERIC_GSI +struct fwnode_handle * +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source); +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger, + int polarity); +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq); +#else +#define acpi_get_irq_source_fwhandle(source) (NULL) +static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, + int trigger, int polarity) +{ + return acpi_register_gsi(NULL, hwirq, trigger, polarity); +} +static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq) +{ + acpi_unregister_gsi(hwirq); +} +#endif + struct pci_dev; int acpi_pci_irq_enable (struct pci_dev *dev);
When an Extended IRQ Resource contains a valid ResourceSource use it to map the IRQ on the domain associated with the ACPI device referenced. With this in place an irqchip driver can create its domain using irq_domain_create_linear and pass the device fwnode to create the domain mapping. When dependent devices are probed these changes allow the ACPI core find the domain and map the IRQ. Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> --- drivers/acpi/Makefile | 2 +- drivers/acpi/{gsi.c => irq.c} | 98 +++++++++++++++++++++++++++++++++++++------ drivers/acpi/resource.c | 29 +++++++------ include/linux/acpi.h | 19 +++++++++ 4 files changed, 121 insertions(+), 27 deletions(-) rename drivers/acpi/{gsi.c => irq.c} (53%)