| Message ID | 1368144651-11250-4-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear John Stultz, On Thu, 9 May 2013 17:10:51 -0700, John Stultz wrote: > +static void kgdb_fiq_select(bool on) > +{ > + void __iomem *sel = kgdb_irq_base + VIC_INT_SELECT; > + u32 msk = 1 << kgdb_irq; > + u32 val; > + > + pr_debug("rerouting VIC vector %d to %s\n", kgdb_irq, > + on ? "FIQ" : "IRQ"); > + > + val = readl(sel); > + val &= ~msk; > + if (on) > + val |= msk; > + writel(val, sel); > +} > + > +static bool kgdb_is_fiq_rised(void) Shouldn't this function be kgdb_is_fiq_raised() ? > +{ > + return readl(kgdb_irq_base + VIC_FIQ_STATUS) & (1 << kgdb_irq); > +} > + > +static int __init kgdb_fiq_init(void) > +{ > + kgdb_irq_base = __io_address(VERSATILE_VIC_BASE); > + kgdb_irq += INT_UARTINT0; > + WARN_ON(kgdb_irq > INT_UARTINT2); > + > + return kgdb_register_fiq(kgdb_fiq_select, kgdb_is_fiq_rised); > +} > +console_initcall(kgdb_fiq_init); Also, this code that uses hardcoded addresses and IRQ numbers doesn't seem to play really well with the DT. I was considering toying around with this thing on the mach-mvebu platform, but don't have those hardcoded addresses and IRQs, and the register offsets of the IRQ controller driver are not exposed in an header file. Shouldn't IRQ controller driver be exposing a the fiq_select() functionality instead? Like a new flag for irq_chip->irq_set_type(), or a completely new hook irq_chip->irq_set_mode() or something like that? Best regards, Thomas
Thanks a lot for the review, Thomas! On Fri, May 10, 2013 at 04:17:10PM +0200, Thomas Petazzoni wrote: [...] > > +{ > > + return readl(kgdb_irq_base + VIC_FIQ_STATUS) & (1 << kgdb_irq); > > +} > > + > > +static int __init kgdb_fiq_init(void) > > +{ > > + kgdb_irq_base = __io_address(VERSATILE_VIC_BASE); > > + kgdb_irq += INT_UARTINT0; > > + WARN_ON(kgdb_irq > INT_UARTINT2); > > + > > + return kgdb_register_fiq(kgdb_fiq_select, kgdb_is_fiq_rised); > > +} > > +console_initcall(kgdb_fiq_init); > > Also, this code that uses hardcoded addresses and IRQ numbers doesn't > seem to play really well with the DT. I was considering toying around > with this thing on the mach-mvebu platform, but don't have those > hardcoded addresses and IRQs, and the register offsets of the IRQ > controller driver are not exposed in an header file. Shouldn't IRQ > controller driver be exposing a the fiq_select() functionality instead? > Like a new flag for irq_chip->irq_set_type(), or a completely new hook > irq_chip->irq_set_mode() or something like that? FIQs are very specific to the platform, and I doubt that anyone will be happy in exposing this kind of knowledge to the generic subsystem. Moreover, Russell King once said that we should better detach IRQ stuff from FIQs altogether. Thing is, FIQs are not going through the generic IRQ code flow, so there is actually very little point in managing them thru the generic IRQ subsystem. What seems more plausible is making a "FIQ subsystem", and register each IRQ controller that can redirect its sources to FIQ with that new subsystem. But the "subsystem" isn't there, and even doing some cleanups in this core stuff takes forever to review/make decision. :) Plus, writing it for this small thing seems like a bit of overhead. And if you take a closer look at this patch, it actually does not use IRQ numbers with the IRQ subsystem. Instead, it bypasses the whole thing and writes to the VIC registers directly. So, for your case (if you want to use FIQ debugger on your board), I would suggest doing the same. At least until we figure out how to do these ten lines of board-specific code better (ideas/hints are welcome! :) Thanks! Anton
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 66a9076..9909af3 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -364,6 +364,7 @@ config ARCH_VERSATILE select PLAT_VERSATILE_CLCD select PLAT_VERSATILE_CLOCK select VERSATILE_FPGA_IRQ + select ARCH_MIGHT_HAVE_KGDB_FIQ help This enables support for ARM Ltd Versatile board. diff --git a/arch/arm/mach-versatile/Makefile b/arch/arm/mach-versatile/Makefile index 81fa3fe..bfd761f 100644 --- a/arch/arm/mach-versatile/Makefile +++ b/arch/arm/mach-versatile/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_VERSATILE_PB) += versatile_pb.o obj-$(CONFIG_MACH_VERSATILE_AB) += versatile_ab.o obj-$(CONFIG_MACH_VERSATILE_DT) += versatile_dt.o obj-$(CONFIG_PCI) += pci.o +obj-$(CONFIG_KGDB_FIQ) += kgdb_fiq.o diff --git a/arch/arm/mach-versatile/kgdb_fiq.c b/arch/arm/mach-versatile/kgdb_fiq.c new file mode 100644 index 0000000..dd92e40 --- /dev/null +++ b/arch/arm/mach-versatile/kgdb_fiq.c @@ -0,0 +1,55 @@ +/* + * KGDB FIQ board support + * + * Copyright 2012 Linaro Ltd. + * Anton Vorontsov <anton.vorontsov@linaro.org> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kgdb.h> +#include <mach/hardware.h> +#include <mach/platform.h> +#include <asm/hardware/vic.h> + +static void *kgdb_irq_base; + +static int kgdb_irq; +module_param_named(uart_num, kgdb_irq, int, 0600); +MODULE_PARM_DESC(uart_num, "UART<number> port to use for KGDB FIQ"); + +static void kgdb_fiq_select(bool on) +{ + void __iomem *sel = kgdb_irq_base + VIC_INT_SELECT; + u32 msk = 1 << kgdb_irq; + u32 val; + + pr_debug("rerouting VIC vector %d to %s\n", kgdb_irq, + on ? "FIQ" : "IRQ"); + + val = readl(sel); + val &= ~msk; + if (on) + val |= msk; + writel(val, sel); +} + +static bool kgdb_is_fiq_rised(void) +{ + return readl(kgdb_irq_base + VIC_FIQ_STATUS) & (1 << kgdb_irq); +} + +static int __init kgdb_fiq_init(void) +{ + kgdb_irq_base = __io_address(VERSATILE_VIC_BASE); + kgdb_irq += INT_UARTINT0; + WARN_ON(kgdb_irq > INT_UARTINT2); + + return kgdb_register_fiq(kgdb_fiq_select, kgdb_is_fiq_rised); +} +console_initcall(kgdb_fiq_init);