| Message ID | 1396557727-19102-14-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: > For now, ARM uses different IRQ functions to setup an interrupt handler. This > is a bit annoying for common driver because we have to add idefery when > an IRQ is setup (see ns16550_init_postirq for an example). > > To avoid to completely fork the IRQ management code, we can introduce a field > to store the IRQ type (e.g level/edge ...). > > This patch also adds platform_get_irq which will retrieve the IRQ from the > device tree and setup correctly the IRQ type. > > In order to use this solution, we have to move init_IRQ earlier for the boot > CPU. It's fine because the code only depends on percpu. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Patch added > --- > xen/arch/arm/gic.c | 21 +++++++----- > xen/arch/arm/irq.c | 80 ++++++++++++++++++++++++++++++++++++++++----- > xen/arch/arm/setup.c | 3 +- > xen/include/asm-arm/gic.h | 5 ++- > xen/include/asm-arm/irq.h | 3 ++ > 5 files changed, 91 insertions(+), 21 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 9127ecf..ec2994e 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -223,15 +223,20 @@ static hw_irq_controller gic_guest_irq_type = { > > /* > * - needs to be called with a valid cpu_mask, ie each cpu in the mask has > + * - desc.lock must be held > * already called gic_cpu_init I think you've injected that line in the middle of a sentence ;-) > */ > -static void gic_set_irq_properties(unsigned int irq, bool_t level, > +static void gic_set_irq_properties(struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority) > { > volatile unsigned char *bytereg; > uint32_t cfg, edgebit; > unsigned int mask; > + unsigned int irq = desc->irq; > + unsigned int type = desc->arch.type; > + > + ASSERT(spin_is_locked(&desc->lock)); > > spin_lock(&gic.lock); > > @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > /* Set edge / level */ > cfg = GICD[GICD_ICFGR + irq / 16]; > edgebit = 2u << (2 * (irq % 16)); > - if ( level ) > + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) Is getting DT_IRQ_TYPE_NONE here not an error? Oh, I see this is the innards of dt_irq_is_level_triggered. Could that be refactored e.g. into dt_irq_type_is_level_triggered(const something type)? > cfg &= ~edgebit; > else > cfg |= edgebit; > @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void) > init_one_irq_desc(desc); > desc->irq = irq; > desc->action = NULL; > + > + /* PPIs are include in local_irqs, we have to copy the IRQ type from > + * CPU0 otherwise we may miss the type if the IRQ type has been > + * set early. > + */ > + desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type; I thought we had a boot_cpu(foo) accessor, but I guess not (or at least I can't find it right now). That might be nicer to add than adding a hardcoded 0 (I suppose it isn't the only one though). > @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) > BUG(); > } > > +static inline int irq_set_type(struct irq_desc *desc, unsigned int type) > +{ > + unsigned int flags; > + int ret = -EBUSY; > + > + if ( type == DT_IRQ_TYPE_NONE ) > + return 0; > + > + spin_lock_irqsave(&desc->lock, flags); > + > + if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type ) > + goto err; > + > + desc->arch.type = type; There was an open coded assignment in the guest path which unfortunately I already trimmed. Shouldn't that have all these checks too? > + > + ret = 0; > + > +err: > + spin_unlock_irqrestore(&desc->lock, flags); > + return ret; > +} > + > +unsigned int platform_get_irq(const struct dt_device_node *device, > + int index) > +{ > + struct dt_irq dt_irq; > + struct irq_desc *desc; > + unsigned int type, irq; > + int res; > + > + res = dt_device_get_irq(device, index, &dt_irq); > + if ( res ) > + return 0; Not an error? Do we take precautions against IRQ0 being actually used somewhere? We should have an explicit #define for an invalid IRQ number. > + irq = dt_irq.irq; > + type = dt_irq.type; > + > + /* Setup the IRQ type */ > + > + if ( irq < NR_LOCAL_IRQS ) > + { > + unsigned int cpu; > + /* For PPIs, we need to set IRQ type on every online CPUs */ > + for_each_cpu( cpu, &cpu_online_map ) > + { > + desc = &per_cpu(local_irq_desc, cpu)[irq]; > + res = irq_set_type(desc, type); > + if ( res ) > + return 0; Error? Also no need to undo any partial work? I haven't seen the caller yet, but for PPIs do we not get called for each CPU as it binds to the PPI anyway? > + } > + } > + else > + { > + res = irq_set_type(irq_to_desc(irq), type); > + if ( res ) > + return 0; > + } > + > + return irq; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index b52c26f..107c13a 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -16,6 +16,7 @@ struct arch_pirq > > struct arch_irq_desc { > int eoi_cpu; > + unsigned int type; I was wondering through the above if this ought to be a "bool_t level" or not. Thoughts? > }; > > #define NR_LOCAL_IRQS 32 > @@ -46,6 +47,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); > > int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq, > const char *devname); > +unsigned int platform_get_irq(const struct dt_device_node *device, > + int index); > > #endif /* _ASM_HW_IRQ_H */ > /*
On 04/07/2014 04:03 PM, Ian Campbell wrote: >> /* >> * - needs to be called with a valid cpu_mask, ie each cpu in the mask has >> + * - desc.lock must be held >> * already called gic_cpu_init > > I think you've injected that line in the middle of a sentence ;-) Oops right. >> */ >> -static void gic_set_irq_properties(unsigned int irq, bool_t level, >> +static void gic_set_irq_properties(struct irq_desc *desc, >> const cpumask_t *cpu_mask, >> unsigned int priority) >> { >> volatile unsigned char *bytereg; >> uint32_t cfg, edgebit; >> unsigned int mask; >> + unsigned int irq = desc->irq; >> + unsigned int type = desc->arch.type; >> + >> + ASSERT(spin_is_locked(&desc->lock)); >> >> spin_lock(&gic.lock); >> >> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, >> /* Set edge / level */ >> cfg = GICD[GICD_ICFGR + irq / 16]; >> edgebit = 2u << (2 * (irq % 16)); >> - if ( level ) >> + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) > > Is getting DT_IRQ_TYPE_NONE here not an error? No, there is some DT like the exynos one which is using 0 (i.e DT_IRQ_TYPE_NONE) for the IRQ type. I guess we have to skip setting level/edge property in this case. > Oh, I see this is the innards of dt_irq_is_level_triggered. Could that > be refactored e.g. into dt_irq_type_is_level_triggered(const something > type)? I was wondering something like that instead: if ( (type & DT_IRQ_TYPE_LEVEL_MASK) ) ... else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) ) ... So we skip the DT_IRQ_TYPE_NONE. > >> cfg &= ~edgebit; >> else >> cfg |= edgebit; > >> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void) >> init_one_irq_desc(desc); >> desc->irq = irq; >> desc->action = NULL; >> + >> + /* PPIs are include in local_irqs, we have to copy the IRQ type from >> + * CPU0 otherwise we may miss the type if the IRQ type has been >> + * set early. >> + */ >> + desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type; > > I thought we had a boot_cpu(foo) accessor, but I guess not (or at least > I can't find it right now). > > That might be nicer to add than adding a hardcoded 0 (I suppose it isn't > the only one though). It's also used in mm.c when page tables is setup. I will introduce boot_cpu macro. >> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) >> BUG(); >> } >> >> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type) >> +{ >> + unsigned int flags; >> + int ret = -EBUSY; >> + >> + if ( type == DT_IRQ_TYPE_NONE ) >> + return 0; >> + >> + spin_lock_irqsave(&desc->lock, flags); >> + >> + if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type ) >> + goto err; >> + >> + desc->arch.type = type; > > There was an open coded assignment in the guest path which unfortunately > I already trimmed. Shouldn't that have all these checks too? No, because with patch #11 the desc->arch.type is only set once by IRQ. >> + >> + ret = 0; >> + >> +err: >> + spin_unlock_irqrestore(&desc->lock, flags); >> + return ret; >> +} >> + >> +unsigned int platform_get_irq(const struct dt_device_node *device, >> + int index) >> +{ >> + struct dt_irq dt_irq; >> + struct irq_desc *desc; >> + unsigned int type, irq; >> + int res; >> + >> + res = dt_device_get_irq(device, index, &dt_irq); >> + if ( res ) >> + return 0; > > Not an error? Do we take precautions against IRQ0 being actually used > somewhere? Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI. > We should have an explicit #define for an invalid IRQ number. I don't think it's useful because the device tree can't provide an IRQ smaller than 16. >> + irq = dt_irq.irq; >> + type = dt_irq.type; >> + >> + /* Setup the IRQ type */ >> + >> + if ( irq < NR_LOCAL_IRQS ) >> + { >> + unsigned int cpu; >> + /* For PPIs, we need to set IRQ type on every online CPUs */ >> + for_each_cpu( cpu, &cpu_online_map ) >> + { >> + desc = &per_cpu(local_irq_desc, cpu)[irq]; >> + res = irq_set_type(desc, type); >> + if ( res ) >> + return 0; > > Error? > > Also no need to undo any partial work? desc->arch.type should be sync on every CPU. It would be crazy to have a different IRQ type on every CPU. > I haven't seen the caller yet, but for PPIs do we not get called for > each CPU as it binds to the PPI anyway? No, this function is only called once, when the DT is parsed at startup. Basically, this function will replace dt_device_get_irq call. >> + } >> + } >> + else >> + { >> + res = irq_set_type(irq_to_desc(irq), type); >> + if ( res ) >> + return 0; >> + } >> + >> + return irq; >> +} >> + >> /* >> * Local variables: >> * mode: C > >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h >> index b52c26f..107c13a 100644 >> --- a/xen/include/asm-arm/irq.h >> +++ b/xen/include/asm-arm/irq.h >> @@ -16,6 +16,7 @@ struct arch_pirq >> >> struct arch_irq_desc { >> int eoi_cpu; >> + unsigned int type; > > I was wondering through the above if this ought to be a "bool_t level" > or not. Thoughts? We have 5 possibles types: - default : we don't have to set the edge bit - level high/low - edge rising/falling Even if GICv2 is only handling 2 types, it can change for the next version of GIC. Regards,
On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote: > >> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > >> /* Set edge / level */ > >> cfg = GICD[GICD_ICFGR + irq / 16]; > >> edgebit = 2u << (2 * (irq % 16)); > >> - if ( level ) > >> + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) > > > > Is getting DT_IRQ_TYPE_NONE here not an error? > > No, there is some DT like the exynos one which is using 0 (i.e > DT_IRQ_TYPE_NONE) for the IRQ type. The underlying physical interrupt must be one or the other though, surely? So either there is some implicit (or perhaps documented?) assumption that NONE==something or the DT is buggy. > > I guess we have to skip setting level/edge property in this case. > > > Oh, I see this is the innards of dt_irq_is_level_triggered. Could that > > be refactored e.g. into dt_irq_type_is_level_triggered(const something > > type)? > > I was wondering something like that instead: > > if ( (type & DT_IRQ_TYPE_LEVEL_MASK) ) > ... > else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) ) > ... > > So we skip the DT_IRQ_TYPE_NONE. Well, it seems the existing code treats NONE as == level, I don't know if that is deliberate or not. > >> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) > >> BUG(); > >> } > >> > >> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type) > >> +{ > >> + unsigned int flags; > >> + int ret = -EBUSY; > >> + > >> + if ( type == DT_IRQ_TYPE_NONE ) > >> + return 0; > >> + > >> + spin_lock_irqsave(&desc->lock, flags); > >> + > >> + if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type ) > >> + goto err; > >> + > >> + desc->arch.type = type; > > > > There was an open coded assignment in the guest path which unfortunately > > I already trimmed. Shouldn't that have all these checks too? > > No, because with patch #11 the desc->arch.type is only set once by IRQ. I don't follow. What is all this stuff above for if that is the case? Was I misremembering the other instance of desc->arch.type = type? > > >> + > >> + ret = 0; > >> + > >> +err: > >> + spin_unlock_irqrestore(&desc->lock, flags); > >> + return ret; > >> +} > >> + > >> +unsigned int platform_get_irq(const struct dt_device_node *device, > >> + int index) > >> +{ > >> + struct dt_irq dt_irq; > >> + struct irq_desc *desc; > >> + unsigned int type, irq; > >> + int res; > >> + > >> + res = dt_device_get_irq(device, index, &dt_irq); > >> + if ( res ) > >> + return 0; > > > > Not an error? Do we take precautions against IRQ0 being actually used > > somewhere? > > Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI. Ah yes. > > We should have an explicit #define for an invalid IRQ number. > > I don't think it's useful because the device tree can't provide an IRQ > smaller than 16. It would also potentially serve to make the code more self-documenting. "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok and more normal though) > > >> + irq = dt_irq.irq; > >> + type = dt_irq.type; > >> + > >> + /* Setup the IRQ type */ > >> + > >> + if ( irq < NR_LOCAL_IRQS ) > >> + { > >> + unsigned int cpu; > >> + /* For PPIs, we need to set IRQ type on every online CPUs */ > >> + for_each_cpu( cpu, &cpu_online_map ) > >> + { > >> + desc = &per_cpu(local_irq_desc, cpu)[irq]; > >> + res = irq_set_type(desc, type); > >> + if ( res ) > >> + return 0; > > > > Error? > > > > Also no need to undo any partial work? > > desc->arch.type should be sync on every CPU. It would be crazy to have a > different IRQ type on every CPU. Well, the code as it stands appears to make a partial attempt at handling just that. If that weren't the case irq_set_type wouldn't be able to fail for cpu > 0. > >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > >> index b52c26f..107c13a 100644 > >> --- a/xen/include/asm-arm/irq.h > >> +++ b/xen/include/asm-arm/irq.h > >> @@ -16,6 +16,7 @@ struct arch_pirq > >> > >> struct arch_irq_desc { > >> int eoi_cpu; > >> + unsigned int type; > > > > I was wondering through the above if this ought to be a "bool_t level" > > or not. Thoughts? > > We have 5 possibles types: > - default : we don't have to set the edge bit > - level high/low > - edge rising/falling OK. > Even if GICv2 is only handling 2 types, it can change for the next > version of GIC. Well, there aren't all that many more ways you can frob a physical line. I suppose MSIs will account for one more though. Ian.
On 04/07/2014 05:26 PM, Ian Campbell wrote: > On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote: >>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, >>>> /* Set edge / level */ >>>> cfg = GICD[GICD_ICFGR + irq / 16]; >>>> edgebit = 2u << (2 * (irq % 16)); >>>> - if ( level ) >>>> + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) >>> >>> Is getting DT_IRQ_TYPE_NONE here not an error? >> >> No, there is some DT like the exynos one which is using 0 (i.e >> DT_IRQ_TYPE_NONE) for the IRQ type. > > The underlying physical interrupt must be one or the other though, > surely? > > So either there is some implicit (or perhaps documented?) assumption > that NONE==something or the DT is buggy. By default Linux is setting every interrupt to be level triggered, active low. I've just noticed we do the same thing in gic_dist_init. >> >> I guess we have to skip setting level/edge property in this case. >> >>> Oh, I see this is the innards of dt_irq_is_level_triggered. Could that >>> be refactored e.g. into dt_irq_type_is_level_triggered(const something >>> type)? >> >> I was wondering something like that instead: >> >> if ( (type & DT_IRQ_TYPE_LEVEL_MASK) ) >> ... >> else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) ) >> ... >> >> So we skip the DT_IRQ_TYPE_NONE. > > Well, it seems the existing code treats NONE as == level, I don't know > if that is deliberate or not. I wrote this code, until now I had forgotten why I was using NONE :). >>>> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) >>>> BUG(); >>>> } >>>> >>>> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type) >>>> +{ >>>> + unsigned int flags; >>>> + int ret = -EBUSY; >>>> + >>>> + if ( type == DT_IRQ_TYPE_NONE ) >>>> + return 0; >>>> + >>>> + spin_lock_irqsave(&desc->lock, flags); >>>> + >>>> + if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type ) >>>> + goto err; >>>> + >>>> + desc->arch.type = type; >>> >>> There was an open coded assignment in the guest path which unfortunately >>> I already trimmed. Shouldn't that have all these checks too? >> >> No, because with patch #11 the desc->arch.type is only set once by IRQ. > > I don't follow. What is all this stuff above for if that is the case? > Was I misremembering the other instance of desc->arch.type = type? Sorry, I was talking about desc->arch.type = type in route_dt_irq_to_guest. >> >>>> + >>>> + ret = 0; >>>> + >>>> +err: >>>> + spin_unlock_irqrestore(&desc->lock, flags); >>>> + return ret; >>>> +} >>>> + >>>> +unsigned int platform_get_irq(const struct dt_device_node *device, >>>> + int index) >>>> +{ >>>> + struct dt_irq dt_irq; >>>> + struct irq_desc *desc; >>>> + unsigned int type, irq; >>>> + int res; >>>> + >>>> + res = dt_device_get_irq(device, index, &dt_irq); >>>> + if ( res ) >>>> + return 0; >>> >>> Not an error? Do we take precautions against IRQ0 being actually used >>> somewhere? >> >> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI. > > Ah yes. > >>> We should have an explicit #define for an invalid IRQ number. >> >> I don't think it's useful because the device tree can't provide an IRQ >> smaller than 16. > > It would also potentially serve to make the code more self-documenting. > "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more > obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok > and more normal though) I would prefer to use either both either nothing. It's confusing to return INVALID_IRQ and assuming after it's always 0... If you prefer I can add a common above the function to say 0 is used when an error is occured. >> >>>> + irq = dt_irq.irq; >>>> + type = dt_irq.type; >>>> + >>>> + /* Setup the IRQ type */ >>>> + >>>> + if ( irq < NR_LOCAL_IRQS ) >>>> + { >>>> + unsigned int cpu; >>>> + /* For PPIs, we need to set IRQ type on every online CPUs */ >>>> + for_each_cpu( cpu, &cpu_online_map ) >>>> + { >>>> + desc = &per_cpu(local_irq_desc, cpu)[irq]; >>>> + res = irq_set_type(desc, type); >>>> + if ( res ) >>>> + return 0; >>> >>> Error? >>> >>> Also no need to undo any partial work? >> >> desc->arch.type should be sync on every CPU. It would be crazy to have a >> different IRQ type on every CPU. > > Well, the code as it stands appears to make a partial attempt at > handling just that. If that weren't the case irq_set_type wouldn't be > able to fail for cpu > 0. I just use the irq_set_type handler for more convenience. If you want I can add an ASSERT(cpu > 0 && !res); Regards,
On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote: > On 04/07/2014 05:26 PM, Ian Campbell wrote: > > On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote: > >>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > >>>> /* Set edge / level */ > >>>> cfg = GICD[GICD_ICFGR + irq / 16]; > >>>> edgebit = 2u << (2 * (irq % 16)); > >>>> - if ( level ) > >>>> + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) > >>> > >>> Is getting DT_IRQ_TYPE_NONE here not an error? > >> > >> No, there is some DT like the exynos one which is using 0 (i.e > >> DT_IRQ_TYPE_NONE) for the IRQ type. > > > > The underlying physical interrupt must be one or the other though, > > surely? > > > > So either there is some implicit (or perhaps documented?) assumption > > that NONE==something or the DT is buggy. > > By default Linux is setting every interrupt to be level triggered, > active low. Do we know if this is because ePAPR or some other standard say this is the default or just a random choice by Linux? > I've just noticed we do the same thing in gic_dist_init. Whatever the reason for them doing it it probably make sense to do the same, otherwise we are just making pain for ourselves. I'm not convinced that the exynos DT doesn't also need to be fixed though. > >> > >>>> + > >>>> + ret = 0; > >>>> + > >>>> +err: > >>>> + spin_unlock_irqrestore(&desc->lock, flags); > >>>> + return ret; > >>>> +} > >>>> + > >>>> +unsigned int platform_get_irq(const struct dt_device_node *device, > >>>> + int index) > >>>> +{ > >>>> + struct dt_irq dt_irq; > >>>> + struct irq_desc *desc; > >>>> + unsigned int type, irq; > >>>> + int res; > >>>> + > >>>> + res = dt_device_get_irq(device, index, &dt_irq); > >>>> + if ( res ) > >>>> + return 0; > >>> > >>> Not an error? Do we take precautions against IRQ0 being actually used > >>> somewhere? > >> > >> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI. > > > > Ah yes. > > > >>> We should have an explicit #define for an invalid IRQ number. > >> > >> I don't think it's useful because the device tree can't provide an IRQ > >> smaller than 16. > > > > It would also potentially serve to make the code more self-documenting. > > "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more > > obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok > > and more normal though) > > I would prefer to use either both either nothing. It's confusing to > return INVALID_IRQ and assuming after it's always 0... Sure, if INVALID_IRQ is used then it should be used, and we should probably make it ~0 to avoid people mistreating it. > If you prefer I can add a common above the function to say 0 is used > when an error is occured. Well, this is a more global thing than this one function really. > >>>> + irq = dt_irq.irq; > >>>> + type = dt_irq.type; > >>>> + > >>>> + /* Setup the IRQ type */ > >>>> + > >>>> + if ( irq < NR_LOCAL_IRQS ) > >>>> + { > >>>> + unsigned int cpu; > >>>> + /* For PPIs, we need to set IRQ type on every online CPUs */ > >>>> + for_each_cpu( cpu, &cpu_online_map ) > >>>> + { > >>>> + desc = &per_cpu(local_irq_desc, cpu)[irq]; > >>>> + res = irq_set_type(desc, type); > >>>> + if ( res ) > >>>> + return 0; > >>> > >>> Error? > >>> > >>> Also no need to undo any partial work? > >> > >> desc->arch.type should be sync on every CPU. It would be crazy to have a > >> different IRQ type on every CPU. > > > > Well, the code as it stands appears to make a partial attempt at > > handling just that. If that weren't the case irq_set_type wouldn't be > > able to fail for cpu > 0. > > I just use the irq_set_type handler for more convenience. If you want I > can add an ASSERT(cpu > 0 && !res); If you like.
Hi Ian, I have sent a V3 few minutes ago. I will take into account of your remarks in V4. On 04/08/2014 04:30 PM, Ian Campbell wrote: > On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote: >> On 04/07/2014 05:26 PM, Ian Campbell wrote: >>> On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote: >>>>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, >>>>>> /* Set edge / level */ >>>>>> cfg = GICD[GICD_ICFGR + irq / 16]; >>>>>> edgebit = 2u << (2 * (irq % 16)); >>>>>> - if ( level ) >>>>>> + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) >>>>> >>>>> Is getting DT_IRQ_TYPE_NONE here not an error? >>>> >>>> No, there is some DT like the exynos one which is using 0 (i.e >>>> DT_IRQ_TYPE_NONE) for the IRQ type. >>> >>> The underlying physical interrupt must be one or the other though, >>> surely? >>> >>> So either there is some implicit (or perhaps documented?) assumption >>> that NONE==something or the DT is buggy. >> >> By default Linux is setting every interrupt to be level triggered, >> active low. > > Do we know if this is because ePAPR or some other standard say this is > the default or just a random choice by Linux? ePAPR only describes interrupts with 2 cells. I don't find anything about the default value. I don't know how Linux chose this value. I guess it's because most of interrupt level-sensitive on ARM. >> I've just noticed we do the same thing in gic_dist_init. > > Whatever the reason for them doing it it probably make sense to do the > same, otherwise we are just making pain for ourselves. > > I'm not convinced that the exynos DT doesn't also need to be fixed > though. In any case we have to be able to boot any valid device tree for Linux no matter are the values. It seems that Linux is ignoring NONE and doesn't update the ICFGR. >> If you prefer I can add a common above the function to say 0 is used >> when an error is occured. > > Well, this is a more global thing than this one function really. I can use the same solution as serial_irq callbacks, i.e returning -1 in case of an error. Regards,
On Tue, 2014-04-08 at 16:50 +0100, Julien Grall wrote: > >> I've just noticed we do the same thing in gic_dist_init. > > > > Whatever the reason for them doing it it probably make sense to do the > > same, otherwise we are just making pain for ourselves. > > > > I'm not convinced that the exynos DT doesn't also need to be fixed > > though. > > In any case we have to be able to boot any valid device tree for Linux > no matter are the values. Right. Those two statements are not contradictory though. > It seems that Linux is ignoring NONE and doesn't update the ICFGR. That makes sense actually: i.e. if DT says none then you have assume that someone (e.g. the bootloader) has done the right thing already and not touch it. > >> If you prefer I can add a common above the function to say 0 is used > >> when an error is occured. > > > > Well, this is a more global thing than this one function really. > > I can use the same solution as serial_irq callbacks, i.e returning -1 in > case of an error. OK.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 9127ecf..ec2994e 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -223,15 +223,20 @@ static hw_irq_controller gic_guest_irq_type = { /* * - needs to be called with a valid cpu_mask, ie each cpu in the mask has + * - desc.lock must be held * already called gic_cpu_init */ -static void gic_set_irq_properties(unsigned int irq, bool_t level, +static void gic_set_irq_properties(struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority) { volatile unsigned char *bytereg; uint32_t cfg, edgebit; unsigned int mask; + unsigned int irq = desc->irq; + unsigned int type = desc->arch.type; + + ASSERT(spin_is_locked(&desc->lock)); spin_lock(&gic.lock); @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, /* Set edge / level */ cfg = GICD[GICD_ICFGR + irq / 16]; edgebit = 2u << (2 * (irq % 16)); - if ( level ) + if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) ) cfg &= ~edgebit; else cfg |= edgebit; @@ -260,8 +265,8 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, /* Program the GIC to route an interrupt to the host (eg Xen) * - needs to be called with desc.lock held */ -void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level, - const cpumask_t *cpu_mask, unsigned int priority) +void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, + unsigned int priority) { ASSERT(priority <= 0xff); /* Only 8 bits of priority */ ASSERT(desc->irq < gic.lines);/* Can't route interrupts that don't exist */ @@ -270,15 +275,14 @@ void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level, desc->handler = &gic_host_irq_type; - gic_set_irq_properties(desc->irq, level, cpu_mask, priority); + gic_set_irq_properties(desc, cpu_mask, priority); } /* Program the GIC to route an interrupt to a guest * - desc.lock must be held */ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, - bool_t level, const cpumask_t *cpu_mask, - unsigned int priority) + const cpumask_t *cpu_mask, unsigned int priority) { struct pending_irq *p; ASSERT(spin_is_locked(&desc->lock)); @@ -286,8 +290,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, desc->handler = &gic_guest_irq_type; desc->status |= IRQ_GUEST; - gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()), - GIC_PRI_IRQ); + gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); /* TODO: do not assume delivery to vcpu0 */ p = irq_to_pending(d->vcpu[0], desc->irq); diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 2bf6618..a56ccf2 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -55,6 +55,7 @@ irq_desc_t *__irq_to_desc(int irq) int __init arch_init_one_irq_desc(struct irq_desc *desc) { + desc->arch.type = DT_IRQ_TYPE_NONE; return 0; } @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void) init_one_irq_desc(desc); desc->irq = irq; desc->action = NULL; + + /* PPIs are include in local_irqs, we have to copy the IRQ type from + * CPU0 otherwise we may miss the type if the IRQ type has been + * set early. + */ + desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type; } return 0; @@ -272,9 +279,6 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) /* First time the IRQ is setup */ if ( disabled ) { - bool_t level; - - level = dt_irq_is_level_triggered(irq); /* It's fine to use smp_processor_id() because: * For PPI: irq_desc is banked * For SPI: we don't care for now which CPU will receive the @@ -282,7 +286,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) * TODO: Handle case where SPI is setup on different CPU than * the targeted CPU and the priority. */ - gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()), + desc->arch.type = irq->type; + gic_route_irq_to_xen(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); desc->handler->startup(desc); } @@ -300,7 +305,6 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq, struct irq_desc *desc = irq_to_desc(irq->irq); unsigned long flags; int retval = 0; - bool_t level; action = xmalloc(struct irqaction); if (!action) @@ -341,10 +345,9 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq, goto out; } - level = dt_irq_is_level_triggered(irq); - gic_route_irq_to_guest(d, desc, level, cpumask_of(smp_processor_id()), + desc->arch.type = irq->type; + gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); - out: spin_unlock_irqrestore(&desc->lock, flags); return retval; @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) BUG(); } +static inline int irq_set_type(struct irq_desc *desc, unsigned int type) +{ + unsigned int flags; + int ret = -EBUSY; + + if ( type == DT_IRQ_TYPE_NONE ) + return 0; + + spin_lock_irqsave(&desc->lock, flags); + + if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type ) + goto err; + + desc->arch.type = type; + + ret = 0; + +err: + spin_unlock_irqrestore(&desc->lock, flags); + return ret; +} + +unsigned int platform_get_irq(const struct dt_device_node *device, + int index) +{ + struct dt_irq dt_irq; + struct irq_desc *desc; + unsigned int type, irq; + int res; + + res = dt_device_get_irq(device, index, &dt_irq); + if ( res ) + return 0; + + irq = dt_irq.irq; + type = dt_irq.type; + + /* Setup the IRQ type */ + + if ( irq < NR_LOCAL_IRQS ) + { + unsigned int cpu; + /* For PPIs, we need to set IRQ type on every online CPUs */ + for_each_cpu( cpu, &cpu_online_map ) + { + desc = &per_cpu(local_irq_desc, cpu)[irq]; + res = irq_set_type(desc, type); + if ( res ) + return 0; + } + } + else + { + res = irq_set_type(irq_to_desc(irq), type); + if ( res ) + return 0; + } + + return irq; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 7b02282..b755964 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -687,6 +687,8 @@ void __init start_xen(unsigned long boot_phys_offset, dt_unflatten_host_device_tree(); dt_irq_xlate = gic_irq_xlate; + init_IRQ(); + dt_uart_init(); console_init_preirq(); @@ -716,7 +718,6 @@ void __init start_xen(unsigned long boot_phys_offset, tasklet_subsys_init(); - init_IRQ(); xsm_dt_init(); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index b750b17..80f8dd2 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -168,11 +168,10 @@ extern void vgic_clear_pending_irqs(struct vcpu *v); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq); /* Program the GIC to route an interrupt */ -extern void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level, - const cpumask_t *cpu_mask, +extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority); extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc, - bool_t level, const cpumask_t *cpu_mask, + const cpumask_t *cpu_mask, unsigned int priority); extern void gic_inject(void); diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index b52c26f..107c13a 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -16,6 +16,7 @@ struct arch_pirq struct arch_irq_desc { int eoi_cpu; + unsigned int type; }; #define NR_LOCAL_IRQS 32 @@ -46,6 +47,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new); int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq, const char *devname); +unsigned int platform_get_irq(const struct dt_device_node *device, + int index); #endif /* _ASM_HW_IRQ_H */ /*
For now, ARM uses different IRQ functions to setup an interrupt handler. This is a bit annoying for common driver because we have to add idefery when an IRQ is setup (see ns16550_init_postirq for an example). To avoid to completely fork the IRQ management code, we can introduce a field to store the IRQ type (e.g level/edge ...). This patch also adds platform_get_irq which will retrieve the IRQ from the device tree and setup correctly the IRQ type. In order to use this solution, we have to move init_IRQ earlier for the boot CPU. It's fine because the code only depends on percpu. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Patch added --- xen/arch/arm/gic.c | 21 +++++++----- xen/arch/arm/irq.c | 80 ++++++++++++++++++++++++++++++++++++++++----- xen/arch/arm/setup.c | 3 +- xen/include/asm-arm/gic.h | 5 ++- xen/include/asm-arm/irq.h | 3 ++ 5 files changed, 91 insertions(+), 21 deletions(-)