| Message ID | 1465312354-27778-4-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Accepted |
| Delegated to: | Jon Hunter |
| Headers | show |
Hi.
I noticed my board would not work any more
when pulling recent updates.
I did "git-bisect" and I found the following commit is it.
commit 1e2a7d78499ec8859d2b469051b7b80bad3b08aa
Author: Jon Hunter <jonathanh@nvidia.com>
Date: Tue Jun 7 16:12:28 2016 +0100
irqdomain: Don't set type when mapping an IRQ
With reverting it, everything works fine for me.
My board is arch/arm64/boot/dts/socionext/uniphier-ph1-ld20-ref.dts
Is anything wrong with IRQ settings in my Device Tree?
If 1e2a7d784 is really a buggy commit,
could you do something please?
2016-06-08 0:12 GMT+09:00 Jon Hunter <jonathanh@nvidia.com>:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
>
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
>
> Add a stub function for irq_domain_free_irqs() to avoid any compilation
> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> include/linux/irqdomain.h | 3 +++
> kernel/irq/irqdomain.c | 23 ++++++++++++++++++-----
> 2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index f1f36e04d885..317503763314 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -452,6 +452,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
> return -1;
> }
>
> +static inline void irq_domain_free_irqs(unsigned int virq,
> + unsigned int nr_irqs) { }
> +
> static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
> {
> return false;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index f3ff1eb8dd09..caa6a63d26f0 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -567,6 +567,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> {
> struct irq_domain *domain;
> + struct irq_data *irq_data;
> irq_hw_number_t hwirq;
> unsigned int type = IRQ_TYPE_NONE;
> int virq;
> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> * it now and return the interrupt number.
> */
> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> - irq_set_irq_type(virq, type);
> + irq_data = irq_get_irq_data(virq);
> + if (!irq_data)
> + return 0;
> +
> + irqd_set_trigger_type(irq_data, type);
> return virq;
> }
>
> @@ -634,10 +639,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> return virq;
> }
>
> - /* Set type if specified and different than the current one */
> - if (type != IRQ_TYPE_NONE &&
> - type != irq_get_trigger_type(virq))
> - irq_set_irq_type(virq, type);
> + irq_data = irq_get_irq_data(virq);
> + if (!irq_data) {
> + if (irq_domain_is_hierarchy(domain))
> + irq_domain_free_irqs(virq, 1);
> + else
> + irq_dispose_mapping(virq);
> + return 0;
> + }
> +
> + /* Store trigger type */
> + irqd_set_trigger_type(irq_data, type);
> +
> return virq;
> }
> EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> --
> 2.1.4
>
On 29/07/16 04:53, Masahiro Yamada wrote: > Hi. > > > I noticed my board would not work any more > when pulling recent updates. > > > I did "git-bisect" and I found the following commit is it. It would help if you did post the log showing the failure. What if you apply the following patch: https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/diff/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi?h=timers/level-trigger&id=95e1fd920fcadce81626cfa9bd6af1a361f17e58 Thanks, M.
On 29/07/16 04:53, Masahiro Yamada wrote: > Hi. > > I noticed my board would not work any more > when pulling recent updates. > > I did "git-bisect" and I found the following commit is it. > > commit 1e2a7d78499ec8859d2b469051b7b80bad3b08aa > Author: Jon Hunter <jonathanh@nvidia.com> > Date: Tue Jun 7 16:12:28 2016 +0100 > > irqdomain: Don't set type when mapping an IRQ > > With reverting it, everything works fine for me. > > My board is arch/arm64/boot/dts/socionext/uniphier-ph1-ld20-ref.dts > > Is anything wrong with IRQ settings in my Device Tree? Most likely. > If 1e2a7d784 is really a buggy commit, > could you do something please? Before this commit bad IRQ type settings in device-tree were not getting reported and so that's why most likely it is bad IRQ type settings. As Marc mentioned without any more details (which IRQ for which device is failing) we cannot confirm. So I would look at the failing IRQ which is now failing when being requested and see if the type in device-tree is correct. Cheers Jon
2016-07-29 17:10 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>: > On 29/07/16 04:53, Masahiro Yamada wrote: >> Hi. >> >> >> I noticed my board would not work any more >> when pulling recent updates. >> >> >> I did "git-bisect" and I found the following commit is it. > > It would help if you did post the log showing the failure. > > What if you apply the following patch: > > https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/diff/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi?h=timers/level-trigger&id=95e1fd920fcadce81626cfa9bd6af1a361f17e58 > Hi Mark, Yes, it worked. But I did not understand why you changed the 3rd cell to 0xf08. The binding of arm,gic-v3.txt says as follows: The 3rd cell is the flags, encoded as follows: bits[3:0] trigger type and level flags. 1 = edge triggered 4 = level triggered Only 1 and 4 are defined for the bits[3:0]. 0xf04 worked, too. Which is correct? Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi guys, I hit an problem when testing a level triggered irq with: irq = platform_get_irq_byname(pdev, "wake"); ...<-- irq trigger type is correct irq_set_status_flags(irq, IRQ_NOAUTOEN); ...<-- irq trigger type become zero request_threaded_irq(irq, ...) the trigger type lost(irqd_get_trigger_type returns zero) after irq_set_status_flags. it looks like irq_set_status_flags would try to use irq_settings_is_level to get level trigger type, which would get zero since we removed set type here...could you help to check this, thanks :) more details in https://patchwork.kernel.org/patch/9903205/ On 06/07/2016 11:12 PM, Jon Hunter wrote: > Some IRQ chips, such as GPIO controllers or secondary level > interrupt controllers, may require require additional runtime power > management control to ensure they are accessible. For such IRQ chips, > it makes sense to enable the IRQ chip when interrupts are requested > and disabled them again once all interrupts have been freed. > > When mapping an IRQ, the IRQ type settings are read and then > programmed. The mapping of the IRQ happens before the IRQ is > requested and so the programming of the type settings occurs before > the IRQ is requested. This is a problem for IRQ chips that require > additional power management control because they may not be > accessible yet. Therefore, when mapping the IRQ, don't program the > type settings, just save them and then program these saved settings > when the IRQ is requested (so long as if they are not overridden via > the call to request the IRQ). > > Add a stub function for irq_domain_free_irqs() to avoid any > compilation errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> Reviewed-by: Marc > Zyngier <marc.zyngier@arm.com> --- include/linux/irqdomain.h | 3 > +++ kernel/irq/irqdomain.c | 23 ++++++++++++++++++----- 2 files > changed, 21 insertions(+), 5 deletions(-) > > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index f1f36e04d885..317503763314 100644 --- > a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -452,6 > +452,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain > *domain, return -1; } > > +static inline void irq_domain_free_irqs(unsigned int virq, + > unsigned int nr_irqs) { } + static inline bool > irq_domain_is_hierarchy(struct irq_domain *domain) { return false; > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index > f3ff1eb8dd09..caa6a63d26f0 100644 --- a/kernel/irq/irqdomain.c +++ > b/kernel/irq/irqdomain.c @@ -567,6 +567,7 @@ static void > of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, unsigned > int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) { struct > irq_domain *domain; + struct irq_data *irq_data; irq_hw_number_t > hwirq; unsigned int type = IRQ_TYPE_NONE; int virq; @@ -614,7 +615,11 > @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > * it now and return the interrupt number. */ if > (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) { - > irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); > + if (!irq_data) + return 0; + + irqd_set_trigger_type(irq_data, > type); return virq; } > > @@ -634,10 +639,18 @@ unsigned int irq_create_fwspec_mapping(struct > irq_fwspec *fwspec) return virq; } > > - /* Set type if specified and different than the current one */ - if > (type != IRQ_TYPE_NONE && - type != irq_get_trigger_type(virq)) - > irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); + > if (!irq_data) { + if (irq_domain_is_hierarchy(domain)) + > irq_domain_free_irqs(virq, 1); + else + irq_dispose_mapping(virq); + > return 0; + } + + /* Store trigger type */ + > irqd_set_trigger_type(irq_data, type); + return virq; } > EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping); > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/include/linux/irqdomain.h b/include/linux/irqdomain.h index f1f36e04d885..317503763314 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -452,6 +452,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, return -1; } +static inline void irq_domain_free_irqs(unsigned int virq, + unsigned int nr_irqs) { } + static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) { return false; diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index f3ff1eb8dd09..caa6a63d26f0 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -567,6 +567,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) { struct irq_domain *domain; + struct irq_data *irq_data; irq_hw_number_t hwirq; unsigned int type = IRQ_TYPE_NONE; int virq; @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) * it now and return the interrupt number. */ if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) { - irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); + if (!irq_data) + return 0; + + irqd_set_trigger_type(irq_data, type); return virq; } @@ -634,10 +639,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) return virq; } - /* Set type if specified and different than the current one */ - if (type != IRQ_TYPE_NONE && - type != irq_get_trigger_type(virq)) - irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); + if (!irq_data) { + if (irq_domain_is_hierarchy(domain)) + irq_domain_free_irqs(virq, 1); + else + irq_dispose_mapping(virq); + return 0; + } + + /* Store trigger type */ + irqd_set_trigger_type(irq_data, type); + return virq; } EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);