| Message ID | 1479074375-2629-2-git-send-email-agustinv@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote: > This allows probe deferral to work properly when a dependent device > fails to get a valid IRQ because the IRQ domain was not registered > at the time the resources were added to the platform_device. > > Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> > --- > drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/base/platform.c | 9 +++++++- > include/linux/acpi.h | 7 ++++++ > 3 files changed, 74 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index 56241eb..4beda15 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares, > return (type & types) ? 0 : 1; > } > EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); > + > +struct acpi_irq_get_ctx { > + unsigned int index; > + struct resource *res; > +}; > + > +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context) > +{ > + struct acpi_irq_get_ctx *ctx = context; > + struct acpi_resource_irq *irq; > + struct acpi_resource_extended_irq *ext_irq; > + > + switch (ares->type) { > + case ACPI_RESOURCE_TYPE_IRQ: > + irq = &ares->data.irq; > + if (ctx->index < irq->interrupt_count) { > + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > + return AE_CTRL_TERMINATE; > + } > + ctx->index -= irq->interrupt_count; I do not understand this code, mind explaining what it is meant to do ? In particular I do not understand the logic behind the index decrement, I think I am missing something here. > + break; > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > + ext_irq = &ares->data.extended_irq; > + if (ctx->index < ext_irq->interrupt_count) { > + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > + return AE_CTRL_TERMINATE; > + } > + ctx->index -= ext_irq->interrupt_count; Ditto. Thanks, Lorenzo > + break; > + } > + > + return AE_OK; > +} > + > +/** > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and > + * use it to initialize the given Linux IRQ resource. > + * @handle ACPI device handle > + * @index ACPI IRQ resource index to lookup > + * @res Linux IRQ resource to initialize > + * > + * Return: 0 on success > + * -EINVAL if an error occurs > + * -EPROBE_DEFER if the IRQ lookup/conversion failed > + */ > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res) > +{ > + struct acpi_irq_get_ctx ctx = { index, res }; > + acpi_status status; > + > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > + acpi_irq_get_cb, &ctx); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + if (res->flags & IORESOURCE_DISABLED) > + return -EPROBE_DEFER; > + return 0; > +} > +EXPORT_SYMBOL_GPL(acpi_irq_get); > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index c4af003..61423d2 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > } > > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > + if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) { > + int ret; > + > + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); > + if (ret) > + return ret; > + } > + > /* > * The resources may pass trigger flags to the irqs that need > * to be set up. It so happens that the trigger flags for > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) > memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); > } > } > - > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 689a8b9..325bdb9 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, > unsigned int acpi_dev_get_irq_type(int triggering, int polarity); > bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > struct resource *res); > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res); > > void acpi_dev_free_resource_list(struct list_head *list); > int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list, > @@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb) > return -EINVAL; > } > > +static inline int acpi_irq_get(acpi_handle handle, unsigned int index, > + struct resource *res) > +{ > + return -EINVAL; > +} > + > #endif /* !CONFIG_ACPI */ > > #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC > -- > 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 Lorenzo, On 2016-11-15 10:48, Lorenzo Pieralisi wrote: > On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote: >> This allows probe deferral to work properly when a dependent device >> fails to get a valid IRQ because the IRQ domain was not registered >> at the time the resources were added to the platform_device. >> >> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> >> --- >> drivers/acpi/resource.c | 59 >> +++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/base/platform.c | 9 +++++++- >> include/linux/acpi.h | 7 ++++++ >> 3 files changed, 74 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c >> index 56241eb..4beda15 100644 >> --- a/drivers/acpi/resource.c >> +++ b/drivers/acpi/resource.c >> @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct >> acpi_resource *ares, >> return (type & types) ? 0 : 1; >> } >> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); >> + >> +struct acpi_irq_get_ctx { >> + unsigned int index; >> + struct resource *res; >> +}; >> + >> +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void >> *context) >> +{ >> + struct acpi_irq_get_ctx *ctx = context; >> + struct acpi_resource_irq *irq; >> + struct acpi_resource_extended_irq *ext_irq; >> + >> + switch (ares->type) { >> + case ACPI_RESOURCE_TYPE_IRQ: >> + irq = &ares->data.irq; >> + if (ctx->index < irq->interrupt_count) { >> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); >> + return AE_CTRL_TERMINATE; >> + } >> + ctx->index -= irq->interrupt_count; > > I do not understand this code, mind explaining what it is meant to do ? > > In particular I do not understand the logic behind the index decrement, > I think I am missing something here. > ACPI IRQ resources can be encoded into two types of structures: struct acpi_resource_irq, struct acpi_resource_extended_irq. In theory only the extended version can contain multiple IRQs, but the Linux ACPI core accommodates non-compliant DSDT tables that have regular IRQ resources contain multiple IRQs. To better explain, suppose you have a device that handles two GSIs and one other IRQ form a separate device: Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, ) { 130, 131 } Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, "\\_SB.TCS0.QIC0", ) { 4 } These are encoded into two separate structures with their own interrupts array: res0.interrupts[] = { 130, 131 } res1.interrupts[] = { 4 } However, from the perspective of a client driver these are indexed into a flat space: [0] -> 130 [1] -> 131 [2] -> 4 Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get retries to map it, the client code will pass index 2. acpi_walk_resources will call acpi_irq_get_cb with the first IRQ resource. If the index is less than the number of IRQs, we know this IRQ resource contains the IRQ we want so we call acpi_dev_resource_interrupt to do the actual mapping and return AE_CTRL_TERMINATE so acpi_walk_resources does not continue walking the resource buffer. On the other hand if the index is equal or larger it means we need to skip this IRQ resource and look at the next one, but we need to adjust the lookup index to that of the next IRQ resource. Makes sense? >> + break; >> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> + ext_irq = &ares->data.extended_irq; >> + if (ctx->index < ext_irq->interrupt_count) { >> + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); >> + return AE_CTRL_TERMINATE; >> + } >> + ctx->index -= ext_irq->interrupt_count; > > Ditto. The same logic is used for both types of resources because they are handled in the same way by the ACPI core when it comes to indexing. Thanks, Agustin > > Thanks, > Lorenzo > >> + break; >> + } >> + >> + return AE_OK; >> +} >> + >> +/** >> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index >> and >> + * use it to initialize the given Linux IRQ resource. >> + * @handle ACPI device handle >> + * @index ACPI IRQ resource index to lookup >> + * @res Linux IRQ resource to initialize >> + * >> + * Return: 0 on success >> + * -EINVAL if an error occurs >> + * -EPROBE_DEFER if the IRQ lookup/conversion failed >> + */ >> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct >> resource *res) >> +{ >> + struct acpi_irq_get_ctx ctx = { index, res }; >> + acpi_status status; >> + >> + status = acpi_walk_resources(handle, METHOD_NAME__CRS, >> + acpi_irq_get_cb, &ctx); >> + if (ACPI_FAILURE(status)) >> + return -EINVAL; >> + if (res->flags & IORESOURCE_DISABLED) >> + return -EPROBE_DEFER; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(acpi_irq_get); >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index c4af003..61423d2 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, >> unsigned int num) >> } >> >> r = platform_get_resource(dev, IORESOURCE_IRQ, num); >> + if (r && r->flags & IORESOURCE_DISABLED && >> ACPI_COMPANION(&dev->dev)) { >> + int ret; >> + >> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); >> + if (ret) >> + return ret; >> + } >> + >> /* >> * The resources may pass trigger flags to the irqs that need >> * to be set up. It so happens that the trigger flags for >> @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) >> memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); >> } >> } >> - >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 689a8b9..325bdb9 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct >> acpi_resource *ares, >> unsigned int acpi_dev_get_irq_type(int triggering, int polarity); >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int >> index, >> struct resource *res); >> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct >> resource *res); >> >> void acpi_dev_free_resource_list(struct list_head *list); >> int acpi_dev_get_resources(struct acpi_device *adev, struct list_head >> *list, >> @@ -763,6 +764,12 @@ static inline int >> acpi_reconfig_notifier_unregister(struct notifier_block *nb) >> return -EINVAL; >> } >> >> +static inline int acpi_irq_get(acpi_handle handle, unsigned int >> index, >> + struct resource *res) >> +{ >> + return -EINVAL; >> +} >> + >> #endif /* !CONFIG_ACPI */ >> >> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC >> -- >> 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 15, 2016 at 12:43:38PM -0500, Agustin Vega-Frias wrote: > Hi Lorenzo, > > On 2016-11-15 10:48, Lorenzo Pieralisi wrote: > >On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote: > >>This allows probe deferral to work properly when a dependent device > >>fails to get a valid IRQ because the IRQ domain was not registered > >>at the time the resources were added to the platform_device. > >> > >>Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> > >>--- > >> drivers/acpi/resource.c | 59 > >>+++++++++++++++++++++++++++++++++++++++++++++++++ > >> drivers/base/platform.c | 9 +++++++- > >> include/linux/acpi.h | 7 ++++++ > >> 3 files changed, 74 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > >>index 56241eb..4beda15 100644 > >>--- a/drivers/acpi/resource.c > >>+++ b/drivers/acpi/resource.c > >>@@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct > >>acpi_resource *ares, > >> return (type & types) ? 0 : 1; > >> } > >> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); > >>+ > >>+struct acpi_irq_get_ctx { > >>+ unsigned int index; > >>+ struct resource *res; > >>+}; > >>+ > >>+static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, > >>void *context) > >>+{ > >>+ struct acpi_irq_get_ctx *ctx = context; > >>+ struct acpi_resource_irq *irq; > >>+ struct acpi_resource_extended_irq *ext_irq; > >>+ > >>+ switch (ares->type) { > >>+ case ACPI_RESOURCE_TYPE_IRQ: > >>+ irq = &ares->data.irq; > >>+ if (ctx->index < irq->interrupt_count) { > >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > >>+ return AE_CTRL_TERMINATE; > >>+ } > >>+ ctx->index -= irq->interrupt_count; > > > >I do not understand this code, mind explaining what it is meant to do ? > > > >In particular I do not understand the logic behind the index decrement, > >I think I am missing something here. > > > > ACPI IRQ resources can be encoded into two types of structures: > > struct acpi_resource_irq, > struct acpi_resource_extended_irq. > > In theory only the extended version can contain multiple IRQs, but > the Linux > ACPI core accommodates non-compliant DSDT tables that have regular > IRQ resources > contain multiple IRQs. > > To better explain, suppose you have a device that handles two GSIs > and one > other IRQ form a separate device: > > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, ) > { 130, 131 } > > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, > "\\_SB.TCS0.QIC0", ) > { 4 } > > These are encoded into two separate structures with their own > interrupts array: > > res0.interrupts[] = { 130, 131 } > res1.interrupts[] = { 4 } > > However, from the perspective of a client driver these are indexed > into a flat space: > > [0] -> 130 > [1] -> 131 > [2] -> 4 > > Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get > retries to map it, the client code will pass index 2. > acpi_walk_resources will call acpi_irq_get_cb with the first IRQ > resource. If the index is less than the number of IRQs, we know this > IRQ resource contains the IRQ we want so we call > acpi_dev_resource_interrupt to do the actual mapping and return > AE_CTRL_TERMINATE so acpi_walk_resources does not continue walking the > resource buffer. On the other hand if the index is equal or larger it > means we need to skip this IRQ resource and look at the next one, but > we need to adjust the lookup index to that of the next IRQ resource. > > Makes sense? Yes, basically it is to create a linear index out of multiple resources, the DT case is simpler since you get the interrupt out of a single property that we can easily index (ie we have to know which firmware entry corresponds to the resource that we are retrying). That deserves a comment. Thanks for explaining. Lorenzo > >>+ break; > >>+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > >>+ ext_irq = &ares->data.extended_irq; > >>+ if (ctx->index < ext_irq->interrupt_count) { > >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > >>+ return AE_CTRL_TERMINATE; > >>+ } > >>+ ctx->index -= ext_irq->interrupt_count; > > > >Ditto. > > The same logic is used for both types of resources because they are > handled in > the same way by the ACPI core when it comes to indexing. > > Thanks, > Agustin > > > > >Thanks, > >Lorenzo > > > >>+ break; > >>+ } > >>+ > >>+ return AE_OK; > >>+} > >>+ > >>+/** > >>+ * acpi_irq_get - Look for the ACPI IRQ resource with the given > >>index and > >>+ * use it to initialize the given Linux IRQ resource. > >>+ * @handle ACPI device handle > >>+ * @index ACPI IRQ resource index to lookup > >>+ * @res Linux IRQ resource to initialize > >>+ * > >>+ * Return: 0 on success > >>+ * -EINVAL if an error occurs > >>+ * -EPROBE_DEFER if the IRQ lookup/conversion failed > >>+ */ > >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct > >>resource *res) > >>+{ > >>+ struct acpi_irq_get_ctx ctx = { index, res }; > >>+ acpi_status status; > >>+ > >>+ status = acpi_walk_resources(handle, METHOD_NAME__CRS, > >>+ acpi_irq_get_cb, &ctx); > >>+ if (ACPI_FAILURE(status)) > >>+ return -EINVAL; > >>+ if (res->flags & IORESOURCE_DISABLED) > >>+ return -EPROBE_DEFER; > >>+ return 0; > >>+} > >>+EXPORT_SYMBOL_GPL(acpi_irq_get); > >>diff --git a/drivers/base/platform.c b/drivers/base/platform.c > >>index c4af003..61423d2 100644 > >>--- a/drivers/base/platform.c > >>+++ b/drivers/base/platform.c > >>@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device > >>*dev, unsigned int num) > >> } > >> > >> r = platform_get_resource(dev, IORESOURCE_IRQ, num); > >>+ if (r && r->flags & IORESOURCE_DISABLED && > >>ACPI_COMPANION(&dev->dev)) { > >>+ int ret; > >>+ > >>+ ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); > >>+ if (ret) > >>+ return ret; > >>+ } > >>+ > >> /* > >> * The resources may pass trigger flags to the irqs that need > >> * to be set up. It so happens that the trigger flags for > >>@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) > >> memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); > >> } > >> } > >>- > >>diff --git a/include/linux/acpi.h b/include/linux/acpi.h > >>index 689a8b9..325bdb9 100644 > >>--- a/include/linux/acpi.h > >>+++ b/include/linux/acpi.h > >>@@ -406,6 +406,7 @@ bool > >>acpi_dev_resource_ext_address_space(struct acpi_resource *ares, > >> unsigned int acpi_dev_get_irq_type(int triggering, int polarity); > >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, > >>int index, > >> struct resource *res); > >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct > >>resource *res); > >> > >> void acpi_dev_free_resource_list(struct list_head *list); > >> int acpi_dev_get_resources(struct acpi_device *adev, struct > >>list_head *list, > >>@@ -763,6 +764,12 @@ static inline int > >>acpi_reconfig_notifier_unregister(struct notifier_block *nb) > >> return -EINVAL; > >> } > >> > >>+static inline int acpi_irq_get(acpi_handle handle, unsigned int > >>index, > >>+ struct resource *res) > >>+{ > >>+ return -EINVAL; > >>+} > >>+ > >> #endif /* !CONFIG_ACPI */ > >> > >> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC > >>-- > >>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 > > -- > 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 Lorenzo, On 2016-11-16 12:18, Lorenzo Pieralisi wrote: > On Tue, Nov 15, 2016 at 12:43:38PM -0500, Agustin Vega-Frias wrote: >> Hi Lorenzo, >> >> On 2016-11-15 10:48, Lorenzo Pieralisi wrote: >> >On Sun, Nov 13, 2016 at 04:59:33PM -0500, Agustin Vega-Frias wrote: >> >>This allows probe deferral to work properly when a dependent device >> >>fails to get a valid IRQ because the IRQ domain was not registered >> >>at the time the resources were added to the platform_device. >> >> >> >>Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> >> >>--- >> >> drivers/acpi/resource.c | 59 >> >>+++++++++++++++++++++++++++++++++++++++++++++++++ >> >> drivers/base/platform.c | 9 +++++++- >> >> include/linux/acpi.h | 7 ++++++ >> >> 3 files changed, 74 insertions(+), 1 deletion(-) >> >> >> >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c >> >>index 56241eb..4beda15 100644 >> >>--- a/drivers/acpi/resource.c >> >>+++ b/drivers/acpi/resource.c >> >>@@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct >> >>acpi_resource *ares, >> >> return (type & types) ? 0 : 1; >> >> } >> >> EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); >> >>+ >> >>+struct acpi_irq_get_ctx { >> >>+ unsigned int index; >> >>+ struct resource *res; >> >>+}; >> >>+ >> >>+static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, >> >>void *context) >> >>+{ >> >>+ struct acpi_irq_get_ctx *ctx = context; >> >>+ struct acpi_resource_irq *irq; >> >>+ struct acpi_resource_extended_irq *ext_irq; >> >>+ >> >>+ switch (ares->type) { >> >>+ case ACPI_RESOURCE_TYPE_IRQ: >> >>+ irq = &ares->data.irq; >> >>+ if (ctx->index < irq->interrupt_count) { >> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); >> >>+ return AE_CTRL_TERMINATE; >> >>+ } >> >>+ ctx->index -= irq->interrupt_count; >> > >> >I do not understand this code, mind explaining what it is meant to do ? >> > >> >In particular I do not understand the logic behind the index decrement, >> >I think I am missing something here. >> > >> >> ACPI IRQ resources can be encoded into two types of structures: >> >> struct acpi_resource_irq, >> struct acpi_resource_extended_irq. >> >> In theory only the extended version can contain multiple IRQs, but >> the Linux >> ACPI core accommodates non-compliant DSDT tables that have regular >> IRQ resources >> contain multiple IRQs. >> >> To better explain, suppose you have a device that handles two GSIs >> and one >> other IRQ form a separate device: >> >> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, ) >> { 130, 131 } >> >> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, 0x00, >> "\\_SB.TCS0.QIC0", ) >> { 4 } >> >> These are encoded into two separate structures with their own >> interrupts array: >> >> res0.interrupts[] = { 130, 131 } >> res1.interrupts[] = { 4 } >> >> However, from the perspective of a client driver these are indexed >> into a flat space: >> >> [0] -> 130 >> [1] -> 131 >> [2] -> 4 >> >> Now say mapping of IRQ 4 failed during bus scan. When acpi_irq_get >> retries to map it, the client code will pass index 2. >> acpi_walk_resources will call acpi_irq_get_cb with the first IRQ >> resource. If the index is less than the number of IRQs, we know this >> IRQ resource contains the IRQ we want so we call >> acpi_dev_resource_interrupt to do the actual mapping and return >> AE_CTRL_TERMINATE so acpi_walk_resources does not continue walking the >> resource buffer. On the other hand if the index is equal or larger it >> means we need to skip this IRQ resource and look at the next one, but >> we need to adjust the lookup index to that of the next IRQ resource. >> >> Makes sense? > > Yes, basically it is to create a linear index out of multiple > resources, > the DT case is simpler since you get the interrupt out of a single > property that we can easily index (ie we have to know which firmware > entry corresponds to the resource that we are retrying). That deserves > a comment. > Good point. I will add the comment on the next version of the series, however, I will hold a few days for more feedback before I submit that. Thanks, Agustin > Thanks for explaining. > > Lorenzo > >> >>+ break; >> >>+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> >>+ ext_irq = &ares->data.extended_irq; >> >>+ if (ctx->index < ext_irq->interrupt_count) { >> >>+ acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); >> >>+ return AE_CTRL_TERMINATE; >> >>+ } >> >>+ ctx->index -= ext_irq->interrupt_count; >> > >> >Ditto. >> >> The same logic is used for both types of resources because they are >> handled in >> the same way by the ACPI core when it comes to indexing. >> >> Thanks, >> Agustin >> >> > >> >Thanks, >> >Lorenzo >> > >> >>+ break; >> >>+ } >> >>+ >> >>+ return AE_OK; >> >>+} >> >>+ >> >>+/** >> >>+ * acpi_irq_get - Look for the ACPI IRQ resource with the given >> >>index and >> >>+ * use it to initialize the given Linux IRQ resource. >> >>+ * @handle ACPI device handle >> >>+ * @index ACPI IRQ resource index to lookup >> >>+ * @res Linux IRQ resource to initialize >> >>+ * >> >>+ * Return: 0 on success >> >>+ * -EINVAL if an error occurs >> >>+ * -EPROBE_DEFER if the IRQ lookup/conversion failed >> >>+ */ >> >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct >> >>resource *res) >> >>+{ >> >>+ struct acpi_irq_get_ctx ctx = { index, res }; >> >>+ acpi_status status; >> >>+ >> >>+ status = acpi_walk_resources(handle, METHOD_NAME__CRS, >> >>+ acpi_irq_get_cb, &ctx); >> >>+ if (ACPI_FAILURE(status)) >> >>+ return -EINVAL; >> >>+ if (res->flags & IORESOURCE_DISABLED) >> >>+ return -EPROBE_DEFER; >> >>+ return 0; >> >>+} >> >>+EXPORT_SYMBOL_GPL(acpi_irq_get); >> >>diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> >>index c4af003..61423d2 100644 >> >>--- a/drivers/base/platform.c >> >>+++ b/drivers/base/platform.c >> >>@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device >> >>*dev, unsigned int num) >> >> } >> >> >> >> r = platform_get_resource(dev, IORESOURCE_IRQ, num); >> >>+ if (r && r->flags & IORESOURCE_DISABLED && >> >>ACPI_COMPANION(&dev->dev)) { >> >>+ int ret; >> >>+ >> >>+ ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); >> >>+ if (ret) >> >>+ return ret; >> >>+ } >> >>+ >> >> /* >> >> * The resources may pass trigger flags to the irqs that need >> >> * to be set up. It so happens that the trigger flags for >> >>@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) >> >> memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); >> >> } >> >> } >> >>- >> >>diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> >>index 689a8b9..325bdb9 100644 >> >>--- a/include/linux/acpi.h >> >>+++ b/include/linux/acpi.h >> >>@@ -406,6 +406,7 @@ bool >> >>acpi_dev_resource_ext_address_space(struct acpi_resource *ares, >> >> unsigned int acpi_dev_get_irq_type(int triggering, int polarity); >> >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares, >> >>int index, >> >> struct resource *res); >> >>+int acpi_irq_get(acpi_handle handle, unsigned int index, struct >> >>resource *res); >> >> >> >> void acpi_dev_free_resource_list(struct list_head *list); >> >> int acpi_dev_get_resources(struct acpi_device *adev, struct >> >>list_head *list, >> >>@@ -763,6 +764,12 @@ static inline int >> >>acpi_reconfig_notifier_unregister(struct notifier_block *nb) >> >> return -EINVAL; >> >> } >> >> >> >>+static inline int acpi_irq_get(acpi_handle handle, unsigned int >> >>index, >> >>+ struct resource *res) >> >>+{ >> >>+ return -EINVAL; >> >>+} >> >>+ >> >> #endif /* !CONFIG_ACPI */ >> >> >> >> #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC >> >>-- >> >>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 >> >> -- >> 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.
This patch works fine on my D05 board. Tested-by: Majun <majun258@huawei.com> 在 2016/11/14 5:59, Agustin Vega-Frias 写道: > This allows probe deferral to work properly when a dependent device > fails to get a valid IRQ because the IRQ domain was not registered > at the time the resources were added to the platform_device. > > Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> > --- > drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/base/platform.c | 9 +++++++- > include/linux/acpi.h | 7 ++++++ > 3 files changed, 74 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c > index 56241eb..4beda15 100644 > --- a/drivers/acpi/resource.c > +++ b/drivers/acpi/resource.c > @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares, > return (type & types) ? 0 : 1; > } > EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); > + > +struct acpi_irq_get_ctx { > + unsigned int index; > + struct resource *res; > +}; > + > +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context) > +{ > + struct acpi_irq_get_ctx *ctx = context; > + struct acpi_resource_irq *irq; > + struct acpi_resource_extended_irq *ext_irq; > + > + switch (ares->type) { > + case ACPI_RESOURCE_TYPE_IRQ: > + irq = &ares->data.irq; > + if (ctx->index < irq->interrupt_count) { > + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > + return AE_CTRL_TERMINATE; > + } > + ctx->index -= irq->interrupt_count; > + break; > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > + ext_irq = &ares->data.extended_irq; > + if (ctx->index < ext_irq->interrupt_count) { > + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); > + return AE_CTRL_TERMINATE; > + } > + ctx->index -= ext_irq->interrupt_count; > + break; > + } > + > + return AE_OK; > +} > + > +/** > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and > + * use it to initialize the given Linux IRQ resource. > + * @handle ACPI device handle > + * @index ACPI IRQ resource index to lookup > + * @res Linux IRQ resource to initialize > + * > + * Return: 0 on success > + * -EINVAL if an error occurs > + * -EPROBE_DEFER if the IRQ lookup/conversion failed > + */ > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res) > +{ > + struct acpi_irq_get_ctx ctx = { index, res }; > + acpi_status status; > + > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > + acpi_irq_get_cb, &ctx); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + if (res->flags & IORESOURCE_DISABLED) > + return -EPROBE_DEFER; > + return 0; > +} > +EXPORT_SYMBOL_GPL(acpi_irq_get); > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index c4af003..61423d2 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > } > > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > + if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) { > + int ret; > + > + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); > + if (ret) > + return ret; > + } > + > /* > * The resources may pass trigger flags to the irqs that need > * to be set up. It so happens that the trigger flags for > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) > memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); > } > } > - > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 689a8b9..325bdb9 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, > unsigned int acpi_dev_get_irq_type(int triggering, int polarity); > bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, > struct resource *res); > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res); > > void acpi_dev_free_resource_list(struct list_head *list); > int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list, > @@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb) > return -EINVAL; > } > > +static inline int acpi_irq_get(acpi_handle handle, unsigned int index, > + struct resource *res) > +{ > + return -EINVAL; > +} > + > #endif /* !CONFIG_ACPI */ > > #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC >
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 56241eb..4beda15 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -664,3 +664,62 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares, return (type & types) ? 0 : 1; } EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type); + +struct acpi_irq_get_ctx { + unsigned int index; + struct resource *res; +}; + +static acpi_status acpi_irq_get_cb(struct acpi_resource *ares, void *context) +{ + struct acpi_irq_get_ctx *ctx = context; + struct acpi_resource_irq *irq; + struct acpi_resource_extended_irq *ext_irq; + + switch (ares->type) { + case ACPI_RESOURCE_TYPE_IRQ: + irq = &ares->data.irq; + if (ctx->index < irq->interrupt_count) { + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); + return AE_CTRL_TERMINATE; + } + ctx->index -= irq->interrupt_count; + break; + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: + ext_irq = &ares->data.extended_irq; + if (ctx->index < ext_irq->interrupt_count) { + acpi_dev_resource_interrupt(ares, ctx->index, ctx->res); + return AE_CTRL_TERMINATE; + } + ctx->index -= ext_irq->interrupt_count; + break; + } + + return AE_OK; +} + +/** + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and + * use it to initialize the given Linux IRQ resource. + * @handle ACPI device handle + * @index ACPI IRQ resource index to lookup + * @res Linux IRQ resource to initialize + * + * Return: 0 on success + * -EINVAL if an error occurs + * -EPROBE_DEFER if the IRQ lookup/conversion failed + */ +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res) +{ + struct acpi_irq_get_ctx ctx = { index, res }; + acpi_status status; + + status = acpi_walk_resources(handle, METHOD_NAME__CRS, + acpi_irq_get_cb, &ctx); + if (ACPI_FAILURE(status)) + return -EINVAL; + if (res->flags & IORESOURCE_DISABLED) + return -EPROBE_DEFER; + return 0; +} +EXPORT_SYMBOL_GPL(acpi_irq_get); diff --git a/drivers/base/platform.c b/drivers/base/platform.c index c4af003..61423d2 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) } r = platform_get_resource(dev, IORESOURCE_IRQ, num); + if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) { + int ret; + + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); + if (ret) + return ret; + } + /* * The resources may pass trigger flags to the irqs that need * to be set up. It so happens that the trigger flags for @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void) memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head)); } } - diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 689a8b9..325bdb9 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -406,6 +406,7 @@ bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, unsigned int acpi_dev_get_irq_type(int triggering, int polarity); bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index, struct resource *res); +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res); void acpi_dev_free_resource_list(struct list_head *list); int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list, @@ -763,6 +764,12 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb) return -EINVAL; } +static inline int acpi_irq_get(acpi_handle handle, unsigned int index, + struct resource *res) +{ + return -EINVAL; +} + #endif /* !CONFIG_ACPI */ #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
This allows probe deferral to work properly when a dependent device fails to get a valid IRQ because the IRQ domain was not registered at the time the resources were added to the platform_device. Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org> --- drivers/acpi/resource.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/base/platform.c | 9 +++++++- include/linux/acpi.h | 7 ++++++ 3 files changed, 74 insertions(+), 1 deletion(-)