| Message ID | 1401961994-18033-2-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 05, 2014 at 10:53:06AM +0100, Daniel Thompson wrote: > static int fiq_def_op(void *ref, int relinquish) > { > - if (!relinquish) > + if (!relinquish) { > + unsigned offset = FIQ_OFFSET; > + no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); > set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); > + } ... > void __init init_FIQ(int start) > { > - unsigned offset = FIQ_OFFSET; > - no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); > fiq_start = start; > } This is wrong - when the default handler is "reinstalled", this change has the effect that we read the first instruction of the existing handler, and then write that same instruction back, rather than replacing the first instruction with the value that was there at boot.
On 05/06/14 12:51, Russell King - ARM Linux wrote: > On Thu, Jun 05, 2014 at 10:53:06AM +0100, Daniel Thompson wrote: >> static int fiq_def_op(void *ref, int relinquish) >> { >> - if (!relinquish) >> + if (!relinquish) { >> + unsigned offset = FIQ_OFFSET; >> + no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); >> set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); >> + } > ... >> void __init init_FIQ(int start) >> { >> - unsigned offset = FIQ_OFFSET; >> - no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); >> fiq_start = start; >> } > > This is wrong - when the default handler is "reinstalled", this change has > the effect that we read the first instruction of the existing handler, and > then write that same instruction back, rather than replacing the first > instruction with the value that was there at boot. Thanks. I'll fix this.
On Thu, Jun 5, 2014 at 11:53 AM, Daniel Thompson <daniel.thompson@linaro.org> wrote: > Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ > virq into a FIQ virq. This is too inflexible for multi-platform kernels > and makes runtime error checking impossible. > > We solve this by introducing a flexible mapping that allows interrupt > controllers that support FIQ to register those mappings. This, in turn, > makes it much possible for drivers in DT kernels to gain access to > FIQ virqs. I always had a big problem with the term "virq" which I take to mean "virtual IRQ". The distinction is: - Hardware IRQ offsets/numbers - a number encoded in HW specs which we usually call hwirqs - Linux IRQ numbers - just some number Sometimes, just sometimes, the Linux IRQ number matches the HW numbers, and then I guess the IRQ is regarded "non-virtual". The only real effect being that the numbers shown in /proc/interrupts are the same in both columns... the leftmost column with the Linux IRQ number and the one right next to the IRQ controller name which is the hwirq local offset. But all Linux IRQ numbers are virtual by definition. It's just some number that the kernel use as a cookie to look up the irq_desc. And one day we may get rid of IRQ numbers altogether. I would prefer just to s/virq/irq/g on this patch or if that is disturbing something more to the point like s/virq/linux_irq/g. Maybe this is a pointless battle, but still... Yours, Linus Walleij
On 12/06/14 09:37, Linus Walleij wrote: > On Thu, Jun 5, 2014 at 11:53 AM, Daniel Thompson > <daniel.thompson@linaro.org> wrote: > >> Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ >> virq into a FIQ virq. This is too inflexible for multi-platform kernels >> and makes runtime error checking impossible. >> >> We solve this by introducing a flexible mapping that allows interrupt >> controllers that support FIQ to register those mappings. This, in turn, >> makes it much possible for drivers in DT kernels to gain access to >> FIQ virqs. > > I always had a big problem with the term "virq" which I take to mean > "virtual IRQ". So do I... That said, I think of it as the virtual as in virtual memory rather the virtual contained within virtualization. > The distinction is: > > - Hardware IRQ offsets/numbers - a number encoded in HW > specs which we usually call hwirqs > > - Linux IRQ numbers - just some number > > Sometimes, just sometimes, the Linux IRQ number matches the > HW numbers, and then I guess the IRQ is regarded "non-virtual". > The only real effect being that the numbers shown in > /proc/interrupts are the same in both columns... the leftmost > column with the Linux IRQ number and the one right next to > the IRQ controller name which is the hwirq local offset. > > But all Linux IRQ numbers are virtual by definition. It's just some > number that the kernel use as a cookie to look up the irq_desc. > And one day we may get rid of IRQ numbers altogether. > > I would prefer just to s/virq/irq/g on this patch or if that is > disturbing something more to the point like s/virq/linux_irq/g. > > Maybe this is a pointless battle, but still... I chose the term not because I have a personal attachement but because that is how they are spelt in the C symbols within irqdomain.[ch] which my patch relies upon heavily. The english documentation and comments is (very nearly) strict about using "Linux IRQ" so I'll happily change all the English text. However... I don't fancy s/virq/irq/ since this risks confusion between Linux irq and ARM IRQ and I don't fancy s/virq/linux_irq/ because in the kernel today virq is ~750x more frequenctly used than 'linux_irq' (git grep and wc suggests its 1507 versus 2). How strongly do you feel about this?
On Thu, Jun 5, 2014 at 4:53 AM, Daniel Thompson <daniel.thompson@linaro.org> wrote: > Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ > virq into a FIQ virq. This is too inflexible for multi-platform kernels > and makes runtime error checking impossible. > > We solve this by introducing a flexible mapping that allows interrupt > controllers that support FIQ to register those mappings. This, in turn, > makes it much possible for drivers in DT kernels to gain access to > FIQ virqs. I don't get why you need a separate linux irq numbers for FIQ. Isn't enabling FIQ simply a property of an irq like edge vs. level trigger? Also, given the constraints on FIQ, we can't really have more that 1 IRQ assigned to FIQ. Rob
On 13/06/14 15:29, Rob Herring wrote: > On Thu, Jun 5, 2014 at 4:53 AM, Daniel Thompson > <daniel.thompson@linaro.org> wrote: >> Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ >> virq into a FIQ virq. This is too inflexible for multi-platform kernels >> and makes runtime error checking impossible. >> >> We solve this by introducing a flexible mapping that allows interrupt >> controllers that support FIQ to register those mappings. This, in turn, >> makes it much possible for drivers in DT kernels to gain access to >> FIQ virqs. > > I don't get why you need a separate linux irq numbers for FIQ. Isn't > enabling FIQ simply a property of an irq like edge vs. level trigger? > Also, given the constraints on FIQ, we can't really have more that 1 > IRQ assigned to FIQ. No particular reason. I mostly went that way because it mimics the effect of fiq_start on enable_fiq/disable_fiq whilst supporting multi-platform. I'm tempted to keep the radix tree in the FIQ infrastructure but rather than messing about with shadow virqs use it to lookup a fiq_chip structure. I think this would keep a clean separation between the ARM centric (and slightly weird) FIQ from the generic irq code. Daniel.
diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h index d493d0b..75d98c6 100644 --- a/arch/arm/include/asm/fiq.h +++ b/arch/arm/include/asm/fiq.h @@ -36,8 +36,10 @@ struct fiq_handler { extern int claim_fiq(struct fiq_handler *f); extern void release_fiq(struct fiq_handler *f); extern void set_fiq_handler(void *start, unsigned int length); +extern struct irq_data *lookup_fiq_irq_data(int fiq); extern void enable_fiq(int fiq); extern void disable_fiq(int fiq); +extern void fiq_add_mapping(int irq, int fiq_virq, unsigned int length); /* helpers defined in fiqasm.S: */ extern void __set_fiq_regs(unsigned long const *regs); diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c index 918875d..177939c 100644 --- a/arch/arm/kernel/fiq.c +++ b/arch/arm/kernel/fiq.c @@ -40,6 +40,8 @@ #include <linux/init.h> #include <linux/interrupt.h> #include <linux/seq_file.h> +#include <linux/irq.h> +#include <linux/radix-tree.h> #include <asm/cacheflush.h> #include <asm/cp15.h> @@ -53,6 +55,9 @@ }) static unsigned long no_fiq_insn; +static int fiq_start = -1; +static RADIX_TREE(fiq_virq_tree, GFP_KERNEL); +static DEFINE_MUTEX(fiq_virq_mutex); /* Default reacquire function * - we always relinquish FIQ control @@ -60,8 +65,11 @@ static unsigned long no_fiq_insn; */ static int fiq_def_op(void *ref, int relinquish) { - if (!relinquish) + if (!relinquish) { + unsigned offset = FIQ_OFFSET; + no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); + } return 0; } @@ -127,16 +135,33 @@ void release_fiq(struct fiq_handler *f) while (current_fiq->fiq_op(current_fiq->dev_id, 0)); } -static int fiq_start; +struct irq_data *lookup_fiq_irq_data(int fiq) +{ + struct irq_data *d; + + if (fiq_start != -1) + return irq_get_irq_data(fiq + fiq_start); + + rcu_read_lock(); + d = radix_tree_lookup(&fiq_virq_tree, fiq); + rcu_read_unlock(); + return d; +} void enable_fiq(int fiq) { - enable_irq(fiq + fiq_start); + struct irq_data *d = lookup_fiq_irq_data(fiq); + if (WARN_ON(!d)) + return; + enable_irq(d->irq); } void disable_fiq(int fiq) { - disable_irq(fiq + fiq_start); + struct irq_data *d = lookup_fiq_irq_data(fiq); + if (WARN_ON(!d)) + return; + disable_irq(d->irq); } EXPORT_SYMBOL(set_fiq_handler); @@ -144,12 +169,32 @@ EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(claim_fiq); EXPORT_SYMBOL(release_fiq); +EXPORT_SYMBOL(lookup_fiq_irq_data); EXPORT_SYMBOL(enable_fiq); EXPORT_SYMBOL(disable_fiq); +/* + * Add a mapping between a normal IRQ and a FIQ shadow. + */ +void fiq_add_mapping(int irq, int fiq_virq, unsigned int length) +{ + int i; + + /* fiq_add_mapping can't be mixed with init_FIQ */ + BUG_ON(fiq_start != -1); + + mutex_lock(&fiq_virq_mutex); + for (i = 0; i < length; i++) { + int err = radix_tree_insert(&fiq_virq_tree, irq + i, + irq_get_irq_data(fiq_virq + i)); + if (err) + pr_err("fiq: Cannot map %d to %d\n", irq + i, + fiq_virq + i); + } + mutex_unlock(&fiq_virq_mutex); +} + void __init init_FIQ(int start) { - unsigned offset = FIQ_OFFSET; - no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); fiq_start = start; }
Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ virq into a FIQ virq. This is too inflexible for multi-platform kernels and makes runtime error checking impossible. We solve this by introducing a flexible mapping that allows interrupt controllers that support FIQ to register those mappings. This, in turn, makes it much possible for drivers in DT kernels to gain access to FIQ virqs. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Fabio Estevam <festevam@gmail.com> Cc: Nicolas Pitre <nico@linaro.org> --- arch/arm/include/asm/fiq.h | 2 ++ arch/arm/kernel/fiq.c | 57 +++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 53 insertions(+), 6 deletions(-)