| Message ID | 1434130016-26574-1-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/12/2015 11:26 AM, Noralf Trønnes wrote: > Add a duplicate irq range with an offset on the hwirq's so the > driver can detect that enable_fiq() is used. > Tested with downstream dwc_otg USB controller driver. This basically looks OK, but a few comments/thoughts: a) Should the Kconfig change be a separate patch since it's a separate subsystem? b) Doesn't the driver need to refuse some operation (handler registration, IRQ setup, IRQ enable, ...?) for more than 1 IRQ in the FIQ range, since the FIQ control register only allows routing 1 IRQ to FIQ. c) The DT binding needs updating to describe the extra IRQs: > Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt d) I wonder how the FIQ handler actually gets routed to this controller and hooked to its handler etc. I assume there's a separate patch for that coming? BTW, I'll be on vacation for just over a couple weeks starting Saturday, so responses will certainly be delayed for a quite a while.
Den 18.06.2015 04:26, skrev Stephen Warren: > On 06/12/2015 11:26 AM, Noralf Trønnes wrote: >> Add a duplicate irq range with an offset on the hwirq's so the >> driver can detect that enable_fiq() is used. >> Tested with downstream dwc_otg USB controller driver. > This basically looks OK, but a few comments/thoughts: > > a) Should the Kconfig change be a separate patch since it's a separate > subsystem? Is this a yes since you haven't acked it?
Noralf Trønnes <noralf@tronnes.org> writes: > Add a duplicate irq range with an offset on the hwirq's so the > driver can detect that enable_fiq() is used. > Tested with downstream dwc_otg USB controller driver. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > arch/arm/mach-bcm/Kconfig | 1 + > drivers/irqchip/irq-bcm2835.c | 53 ++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig > index 8b11f44..7cfef7b 100644 > --- a/arch/arm/mach-bcm/Kconfig > +++ b/arch/arm/mach-bcm/Kconfig > @@ -114,6 +114,7 @@ config ARCH_BCM2835 > select ARM_ERRATA_411920 > select ARM_TIMER_SP804 > select CLKSRC_OF > + select FIQ > select PINCTRL > select PINCTRL_BCM2835 > help > diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c > index 5916d6c..db66246 100644 > --- a/drivers/irqchip/irq-bcm2835.c > +++ b/drivers/irqchip/irq-bcm2835.c > @@ -56,7 +56,7 @@ > #include "irqchip.h" > > /* Put the bank and irq (32 bits) into the hwirq */ > -#define MAKE_HWIRQ(b, n) ((b << 5) | (n)) > +#define MAKE_HWIRQ(b, n) (((b) << 5) | (n)) > #define HWIRQ_BANK(i) (i >> 5) > #define HWIRQ_BIT(i) BIT(i & 0x1f) > > @@ -72,9 +72,13 @@ > | SHORTCUT1_MASK | SHORTCUT2_MASK) > > #define REG_FIQ_CONTROL 0x0c > +#define REG_FIQ_ENABLE 0x80 > +#define REG_FIQ_DISABLE 0 > > #define NR_BANKS 3 > #define IRQS_PER_BANK 32 > +#define NUMBER_IRQS MAKE_HWIRQ(NR_BANKS, 0) > +#define FIQ_START (NR_IRQS_BANK0 + MAKE_HWIRQ(NR_BANKS - 1, 0)) > > static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 }; > static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 }; > @@ -98,14 +102,38 @@ static struct armctrl_ic intc __read_mostly; > static void __exception_irq_entry bcm2835_handle_irq( > struct pt_regs *regs); > > +static inline unsigned int hwirq_to_fiq(unsigned long hwirq) > +{ > + hwirq -= NUMBER_IRQS; > + /* > + * The hwirq numbering used in this driver is: > + * BASE (0-7) GPU1 (32-63) GPU2 (64-95). > + * This differ from the one used in the FIQ register: > + * GPU1 (0-31) GPU2 (32-63) BASE (64-71) > + */ > + if (hwirq >= 32) > + return hwirq - 32; > + > + return hwirq + 64; > +} > + > static void armctrl_mask_irq(struct irq_data *d) > { > - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]); > + if (d->hwirq >= NUMBER_IRQS) > + writel_relaxed(REG_FIQ_DISABLE, intc.base + REG_FIQ_CONTROL); > + else > + writel_relaxed(HWIRQ_BIT(d->hwirq), > + intc.disable[HWIRQ_BANK(d->hwirq)]); > } > > static void armctrl_unmask_irq(struct irq_data *d) > { > - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]); > + if (d->hwirq >= NUMBER_IRQS) > + writel_relaxed(REG_FIQ_ENABLE | hwirq_to_fiq(d->hwirq), > + intc.base + REG_FIQ_CONTROL); > + else > + writel_relaxed(HWIRQ_BIT(d->hwirq), > + intc.enable[HWIRQ_BANK(d->hwirq)]); > } I found it nice for the 2836 controller to declare a new irqchip when both the mask/unmask hooks needed to be changed for that class of interrupt. However, it looks like these functions aren't going to be called regularly, so it doesn't matter much. As far as interaction with my 2836 series, it looks like the only thing downstream does is make the FIQ get handled on CPU1 instead of CPU0. I think we'll be fine fixing this up later. Reviewed-by: Eric Anholt <eric@anholt.net>
On 07/22/2015 08:07 AM, Noralf Trønnes wrote: > > Den 18.06.2015 04:26, skrev Stephen Warren: >> On 06/12/2015 11:26 AM, Noralf Trønnes wrote: >>> Add a duplicate irq range with an offset on the hwirq's so the >>> driver can detect that enable_fiq() is used. >>> Tested with downstream dwc_otg USB controller driver. >> This basically looks OK, but a few comments/thoughts: >> >> a) Should the Kconfig change be a separate patch since it's a separate >> subsystem? > > Is this a yes since you haven't acked it? I didn't ack it because I thought there were still outstanding review comments. Looking back at the thread, everything is resolved. Acked-by: Stephen Warren <swarren@wwwdotorg.org> Whether the driver and Kconfig patch need to be in separate patches largely depends on who will merge it. If there's a desire to merge the two parts through separate subsystems, separate patches will be needed, else not. I'll leave that up to the IRQ subsystem maintainers and Lee (or Eric) since they're actually merging the bcm2835 patches.
Den 22.07.2015 23:32, skrev Eric Anholt: > Noralf Trønnes <noralf@tronnes.org> writes: > >> Add a duplicate irq range with an offset on the hwirq's so the >> driver can detect that enable_fiq() is used. >> Tested with downstream dwc_otg USB controller driver. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> arch/arm/mach-bcm/Kconfig | 1 + >> drivers/irqchip/irq-bcm2835.c | 53 ++++++++++++++++++++++++++++++++++++++----- >> 2 files changed, 48 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig >> index 8b11f44..7cfef7b 100644 >> --- a/arch/arm/mach-bcm/Kconfig >> +++ b/arch/arm/mach-bcm/Kconfig >> @@ -114,6 +114,7 @@ config ARCH_BCM2835 >> select ARM_ERRATA_411920 >> select ARM_TIMER_SP804 >> select CLKSRC_OF >> + select FIQ >> select PINCTRL >> select PINCTRL_BCM2835 >> help >> diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c >> index 5916d6c..db66246 100644 >> --- a/drivers/irqchip/irq-bcm2835.c >> +++ b/drivers/irqchip/irq-bcm2835.c >> @@ -56,7 +56,7 @@ >> #include "irqchip.h" >> >> /* Put the bank and irq (32 bits) into the hwirq */ >> -#define MAKE_HWIRQ(b, n) ((b << 5) | (n)) >> +#define MAKE_HWIRQ(b, n) (((b) << 5) | (n)) >> #define HWIRQ_BANK(i) (i >> 5) >> #define HWIRQ_BIT(i) BIT(i & 0x1f) >> >> @@ -72,9 +72,13 @@ >> | SHORTCUT1_MASK | SHORTCUT2_MASK) >> >> #define REG_FIQ_CONTROL 0x0c >> +#define REG_FIQ_ENABLE 0x80 >> +#define REG_FIQ_DISABLE 0 >> >> #define NR_BANKS 3 >> #define IRQS_PER_BANK 32 >> +#define NUMBER_IRQS MAKE_HWIRQ(NR_BANKS, 0) >> +#define FIQ_START (NR_IRQS_BANK0 + MAKE_HWIRQ(NR_BANKS - 1, 0)) >> >> static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 }; >> static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 }; >> @@ -98,14 +102,38 @@ static struct armctrl_ic intc __read_mostly; >> static void __exception_irq_entry bcm2835_handle_irq( >> struct pt_regs *regs); >> >> +static inline unsigned int hwirq_to_fiq(unsigned long hwirq) >> +{ >> + hwirq -= NUMBER_IRQS; >> + /* >> + * The hwirq numbering used in this driver is: >> + * BASE (0-7) GPU1 (32-63) GPU2 (64-95). >> + * This differ from the one used in the FIQ register: >> + * GPU1 (0-31) GPU2 (32-63) BASE (64-71) >> + */ >> + if (hwirq >= 32) >> + return hwirq - 32; >> + >> + return hwirq + 64; >> +} >> + >> static void armctrl_mask_irq(struct irq_data *d) >> { >> - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]); >> + if (d->hwirq >= NUMBER_IRQS) >> + writel_relaxed(REG_FIQ_DISABLE, intc.base + REG_FIQ_CONTROL); >> + else >> + writel_relaxed(HWIRQ_BIT(d->hwirq), >> + intc.disable[HWIRQ_BANK(d->hwirq)]); >> } >> >> static void armctrl_unmask_irq(struct irq_data *d) >> { >> - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]); >> + if (d->hwirq >= NUMBER_IRQS) >> + writel_relaxed(REG_FIQ_ENABLE | hwirq_to_fiq(d->hwirq), >> + intc.base + REG_FIQ_CONTROL); >> + else >> + writel_relaxed(HWIRQ_BIT(d->hwirq), >> + intc.enable[HWIRQ_BANK(d->hwirq)]); >> } > I found it nice for the 2836 controller to declare a new irqchip when > both the mask/unmask hooks needed to be changed for that class of > interrupt. However, it looks like these functions aren't going to be > called regularly, so it doesn't matter much. > > As far as interaction with my 2836 series, it looks like the only thing > downstream does is make the FIQ get handled on CPU1 instead of CPU0. I > think we'll be fine fixing this up later. > > Reviewed-by: Eric Anholt <eric@anholt.net> It seems that this has slipped through the cracks, at least it didn't enter in the 4.3 merge window. Noralf.
On Sun, Sep 13, 2015 at 09:24:48PM +0200, Noralf Trønnes wrote: > > Den 22.07.2015 23:32, skrev Eric Anholt: > >Noralf Trønnes <noralf@tronnes.org> writes: > > > >>Add a duplicate irq range with an offset on the hwirq's so the > >>driver can detect that enable_fiq() is used. > >>Tested with downstream dwc_otg USB controller driver. > >> > >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > >>--- > >> arch/arm/mach-bcm/Kconfig | 1 + > >> drivers/irqchip/irq-bcm2835.c | 53 ++++++++++++++++++++++++++++++++++++++----- > >> 2 files changed, 48 insertions(+), 6 deletions(-) > >> > >>diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig > >>index 8b11f44..7cfef7b 100644 > >>--- a/arch/arm/mach-bcm/Kconfig > >>+++ b/arch/arm/mach-bcm/Kconfig > >>@@ -114,6 +114,7 @@ config ARCH_BCM2835 > >> select ARM_ERRATA_411920 > >> select ARM_TIMER_SP804 > >> select CLKSRC_OF > >>+ select FIQ > >> select PINCTRL > >> select PINCTRL_BCM2835 > >> help > >>diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c > >>index 5916d6c..db66246 100644 > >>--- a/drivers/irqchip/irq-bcm2835.c > >>+++ b/drivers/irqchip/irq-bcm2835.c > >>@@ -56,7 +56,7 @@ > >> #include "irqchip.h" > >> /* Put the bank and irq (32 bits) into the hwirq */ > >>-#define MAKE_HWIRQ(b, n) ((b << 5) | (n)) > >>+#define MAKE_HWIRQ(b, n) (((b) << 5) | (n)) > >> #define HWIRQ_BANK(i) (i >> 5) > >> #define HWIRQ_BIT(i) BIT(i & 0x1f) > >>@@ -72,9 +72,13 @@ > >> | SHORTCUT1_MASK | SHORTCUT2_MASK) > >> #define REG_FIQ_CONTROL 0x0c > >>+#define REG_FIQ_ENABLE 0x80 > >>+#define REG_FIQ_DISABLE 0 > >> #define NR_BANKS 3 > >> #define IRQS_PER_BANK 32 > >>+#define NUMBER_IRQS MAKE_HWIRQ(NR_BANKS, 0) > >>+#define FIQ_START (NR_IRQS_BANK0 + MAKE_HWIRQ(NR_BANKS - 1, 0)) > >> static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 }; > >> static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 }; > >>@@ -98,14 +102,38 @@ static struct armctrl_ic intc __read_mostly; > >> static void __exception_irq_entry bcm2835_handle_irq( > >> struct pt_regs *regs); > >>+static inline unsigned int hwirq_to_fiq(unsigned long hwirq) > >>+{ > >>+ hwirq -= NUMBER_IRQS; > >>+ /* > >>+ * The hwirq numbering used in this driver is: > >>+ * BASE (0-7) GPU1 (32-63) GPU2 (64-95). > >>+ * This differ from the one used in the FIQ register: > >>+ * GPU1 (0-31) GPU2 (32-63) BASE (64-71) > >>+ */ > >>+ if (hwirq >= 32) > >>+ return hwirq - 32; > >>+ > >>+ return hwirq + 64; > >>+} > >>+ > >> static void armctrl_mask_irq(struct irq_data *d) > >> { > >>- writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]); > >>+ if (d->hwirq >= NUMBER_IRQS) > >>+ writel_relaxed(REG_FIQ_DISABLE, intc.base + REG_FIQ_CONTROL); > >>+ else > >>+ writel_relaxed(HWIRQ_BIT(d->hwirq), > >>+ intc.disable[HWIRQ_BANK(d->hwirq)]); > >> } > >> static void armctrl_unmask_irq(struct irq_data *d) > >> { > >>- writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]); > >>+ if (d->hwirq >= NUMBER_IRQS) > >>+ writel_relaxed(REG_FIQ_ENABLE | hwirq_to_fiq(d->hwirq), > >>+ intc.base + REG_FIQ_CONTROL); > >>+ else > >>+ writel_relaxed(HWIRQ_BIT(d->hwirq), > >>+ intc.enable[HWIRQ_BANK(d->hwirq)]); > >> } > >I found it nice for the 2836 controller to declare a new irqchip when > >both the mask/unmask hooks needed to be changed for that class of > >interrupt. However, it looks like these functions aren't going to be > >called regularly, so it doesn't matter much. > > > >As far as interaction with my 2836 series, it looks like the only thing > >downstream does is make the FIQ get handled on CPU1 instead of CPU0. I > >think we'll be fine fixing this up later. > > > >Reviewed-by: Eric Anholt <eric@anholt.net> > > It seems that this has slipped through the cracks, at least it didn't > enter in the 4.3 merge window. ... to which I'm glad. What's the use-case for FIQs on this SMP platform? Where's the code that makes use of the provided FIQs?
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Sun, Sep 13, 2015 at 09:24:48PM +0200, Noralf Trønnes wrote: >> >> Den 22.07.2015 23:32, skrev Eric Anholt: >> >Noralf Trønnes <noralf@tronnes.org> writes: >> > >> >>Add a duplicate irq range with an offset on the hwirq's so the >> >>driver can detect that enable_fiq() is used. >> >>Tested with downstream dwc_otg USB controller driver. >> >> >> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> >>--- >> >> arch/arm/mach-bcm/Kconfig | 1 + >> >> drivers/irqchip/irq-bcm2835.c | 53 ++++++++++++++++++++++++++++++++++++++----- >> >> 2 files changed, 48 insertions(+), 6 deletions(-) >> >> >> >>diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig >> >>index 8b11f44..7cfef7b 100644 >> >>--- a/arch/arm/mach-bcm/Kconfig >> >>+++ b/arch/arm/mach-bcm/Kconfig >> >>@@ -114,6 +114,7 @@ config ARCH_BCM2835 >> >> select ARM_ERRATA_411920 >> >> select ARM_TIMER_SP804 >> >> select CLKSRC_OF >> >>+ select FIQ >> >> select PINCTRL >> >> select PINCTRL_BCM2835 >> >> help >> >>diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c >> >>index 5916d6c..db66246 100644 >> >>--- a/drivers/irqchip/irq-bcm2835.c >> >>+++ b/drivers/irqchip/irq-bcm2835.c >> >>@@ -56,7 +56,7 @@ >> >> #include "irqchip.h" >> >> /* Put the bank and irq (32 bits) into the hwirq */ >> >>-#define MAKE_HWIRQ(b, n) ((b << 5) | (n)) >> >>+#define MAKE_HWIRQ(b, n) (((b) << 5) | (n)) >> >> #define HWIRQ_BANK(i) (i >> 5) >> >> #define HWIRQ_BIT(i) BIT(i & 0x1f) >> >>@@ -72,9 +72,13 @@ >> >> | SHORTCUT1_MASK | SHORTCUT2_MASK) >> >> #define REG_FIQ_CONTROL 0x0c >> >>+#define REG_FIQ_ENABLE 0x80 >> >>+#define REG_FIQ_DISABLE 0 >> >> #define NR_BANKS 3 >> >> #define IRQS_PER_BANK 32 >> >>+#define NUMBER_IRQS MAKE_HWIRQ(NR_BANKS, 0) >> >>+#define FIQ_START (NR_IRQS_BANK0 + MAKE_HWIRQ(NR_BANKS - 1, 0)) >> >> static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 }; >> >> static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 }; >> >>@@ -98,14 +102,38 @@ static struct armctrl_ic intc __read_mostly; >> >> static void __exception_irq_entry bcm2835_handle_irq( >> >> struct pt_regs *regs); >> >>+static inline unsigned int hwirq_to_fiq(unsigned long hwirq) >> >>+{ >> >>+ hwirq -= NUMBER_IRQS; >> >>+ /* >> >>+ * The hwirq numbering used in this driver is: >> >>+ * BASE (0-7) GPU1 (32-63) GPU2 (64-95). >> >>+ * This differ from the one used in the FIQ register: >> >>+ * GPU1 (0-31) GPU2 (32-63) BASE (64-71) >> >>+ */ >> >>+ if (hwirq >= 32) >> >>+ return hwirq - 32; >> >>+ >> >>+ return hwirq + 64; >> >>+} >> >>+ >> >> static void armctrl_mask_irq(struct irq_data *d) >> >> { >> >>- writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]); >> >>+ if (d->hwirq >= NUMBER_IRQS) >> >>+ writel_relaxed(REG_FIQ_DISABLE, intc.base + REG_FIQ_CONTROL); >> >>+ else >> >>+ writel_relaxed(HWIRQ_BIT(d->hwirq), >> >>+ intc.disable[HWIRQ_BANK(d->hwirq)]); >> >> } >> >> static void armctrl_unmask_irq(struct irq_data *d) >> >> { >> >>- writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]); >> >>+ if (d->hwirq >= NUMBER_IRQS) >> >>+ writel_relaxed(REG_FIQ_ENABLE | hwirq_to_fiq(d->hwirq), >> >>+ intc.base + REG_FIQ_CONTROL); >> >>+ else >> >>+ writel_relaxed(HWIRQ_BIT(d->hwirq), >> >>+ intc.enable[HWIRQ_BANK(d->hwirq)]); >> >> } >> >I found it nice for the 2836 controller to declare a new irqchip when >> >both the mask/unmask hooks needed to be changed for that class of >> >interrupt. However, it looks like these functions aren't going to be >> >called regularly, so it doesn't matter much. >> > >> >As far as interaction with my 2836 series, it looks like the only thing >> >downstream does is make the FIQ get handled on CPU1 instead of CPU0. I >> >think we'll be fine fixing this up later. >> > >> >Reviewed-by: Eric Anholt <eric@anholt.net> >> >> It seems that this has slipped through the cracks, at least it didn't >> enter in the 4.3 merge window. > > ... to which I'm glad. > > What's the use-case for FIQs on this SMP platform? Where's the code that > makes use of the provided FIQs? Downstream (what Raspberry Pi users are generally using) is using FIQ for the USB driver to reduce interrupt latency. Notably, the ethernet chip hangs off USB, and I've heard the USB chip is flaky when interrupt latency is too high. I've only seen dmesg warnings and not lockups myself, but if FIQs improve performance, it may be worthwhile for us. Right now downstream is using its own platform ports called 2708/2708. They're considering swapping over to using our platform ports instead, and being able to use upstream's interrut controller would be a big step on that front. If they can use our platform support in general, it means that future platforms based on it will be more mergeable upstream (It's been 7 months and we still don't have a Raspberry Pi 2 port in tree. It's kind of embarrassing). So, while FIQ isn't used in upstream, I think it's worthwhile to merge. It is another step to bringing the downstream developers into the fold.
On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote: > So, while FIQ isn't used in upstream, I think it's worthwhile to merge. > It is another step to bringing the downstream developers into the fold. I want to see the code _first_. Until then, I'm sorry, this patch can't go in.
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote: >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge. >> It is another step to bringing the downstream developers into the fold. > > I want to see the code _first_. Until then, I'm sorry, this patch can't > go in. If you just want to see "Yes, GPL-compatible code using this is available", then that is: https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c If not, I'm curious what exactly is the reason the patch can't go in.
On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote: > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > > > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote: > >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge. > >> It is another step to bringing the downstream developers into the fold. > > > > I want to see the code _first_. Until then, I'm sorry, this patch can't > > go in. > > If you just want to see "Yes, GPL-compatible code using this is > available", then that is: It's got nothing to do with "GPL-compatible code". I want to audit _all_ code that makes use of FIQ. It disgusts me that you're trying to dress this up as a licensing issue. It isn't. > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c > > If not, I'm curious what exactly is the reason the patch can't go in. In any case, I'm not going to say yes to code which provides a new internal kernel facility for mainline kernels, but with no mainline kernel users. Such code can only be to make external tree maintanence easlier, but it has the unintended effect of making mainline kernel maintanence harder. Such facilities _should_ always come with a user of that new facility. It's nothing personal, it's sane policy from experience. I've had stuff like this in the past, where people have sent new facilities without any users, years have gone by without that facility being used. The facility has eventually been removed from the mainline kernel _because_ it doesn't have any users. Moreover, there are people who audit the kernel looking for facilities which have no users and propose patches to remove them. So, if you want some new facility to be merged into mainline kernels, and for it to remain in the kernel, always accompany it with a user.
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote: >> Russell King - ARM Linux <linux@arm.linux.org.uk> writes: >> >> > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote: >> >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge. >> >> It is another step to bringing the downstream developers into the fold. >> > >> > I want to see the code _first_. Until then, I'm sorry, this patch can't >> > go in. >> >> If you just want to see "Yes, GPL-compatible code using this is >> available", then that is: > > It's got nothing to do with "GPL-compatible code". I want to audit > _all_ code that makes use of FIQ. It disgusts me that you're trying > to dress this up as a licensing issue. It isn't. > >> https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S >> https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c >> >> If not, I'm curious what exactly is the reason the patch can't go in. > > In any case, I'm not going to say yes to code which provides a new > internal kernel facility for mainline kernels, but with no mainline > kernel users. Such code can only be to make external tree maintanence > easlier, but it has the unintended effect of making mainline kernel > maintanence harder. > > Such facilities _should_ always come with a user of that new facility. > > It's nothing personal, it's sane policy from experience. I've had stuff > like this in the past, where people have sent new facilities without any > users, years have gone by without that facility being used. The facility > has eventually been removed from the mainline kernel _because_ it doesn't > have any users. > > Moreover, there are people who audit the kernel looking for facilities > which have no users and propose patches to remove them. > > So, if you want some new facility to be merged into mainline kernels, > and for it to remain in the kernel, always accompany it with a user. Thanks, this was all I needed. In the subsystem I work in, the rule has been "You need to have open userspace using the thing that you can show", but obviously you only get to merge the user once the core stuff is done.
On Wed, Sep 16, 2015 at 05:21:57PM +0100, Russell King - ARM Linux wrote: > On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote: > > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > > > > > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote: > > >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge. > > >> It is another step to bringing the downstream developers into the fold. > > > > > > I want to see the code _first_. Until then, I'm sorry, this patch can't > > > go in. > > > > If you just want to see "Yes, GPL-compatible code using this is > > available", then that is: > > It's got nothing to do with "GPL-compatible code". I want to audit > _all_ code that makes use of FIQ. It disgusts me that you're trying > to dress this up as a licensing issue. It isn't. > > > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S > > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c Some comments on the FIQ aspects of the code I find in https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/ as a whole: * For non-FIQ taking of your special FIQ lock, please add a couple of functions called something like fiq_fsm_spin_lock_disable() and fiq_fsm_spin_unlock_enable() which wraps up this detail. * Please remove the "local_fiq_enable();" at the end of hcd_init_fiq() - FIQs should be enabled already (please check, and if not, please say so, because that's a bug.) * You run the FIQ on CPU1 when there's two CPUs present - I hope you conditionalise this on CPU hotplug support being disabled, or have a means to deal with CPU1 being taken offline while the driver is operational. * It's good that you run set_fiq_regs() from a smp-called-function, where we're guaranteed to be non-preemptible; I've a couple of patches which cause set_fiq_regs() to complain if called in a preemptible context, and the good news is that this driver won't be affected by that change. * The comments in dwc_otg_fiq_fsm.c are incorrect; you now have locking so the bit about requiring FIQ and SGI on the same CPU seems to no longer apply - or does it? However, even if they're running on the same core, I'm completely unconvinced that the comment has ever been correct - the SGI can be interrupted by the FIQ at any moment. * AAPCS requires stacks to be aligned to 64-bits: regs.ARM_sp = (long) dwc_otg_hcd->fiq_stack + (sizeof(struct fiq_stack) - 4); where: struct fiq_stack { int magic1; uint8_t stack[2048]; int magic2; }; does not provide for that. It's also an incredibly inefficient size to allocate - do you absolutely need 2048 bytes or will 2036 bytes do? (Also note that C99 types are frowned upon in the kernel.) So, I'd suggest: struct fiq_stack { u32 magic1; u32 stack[2040 / sizeof(u32)]; u32 magic2; }; and: /* Get top of stack, which must be 64-bit aligned */ regs.ARM_sp = (long)&dwc_otg_hcd->fiq_stack->magic2; WARN_ON(regs.ARM_sp & 7); I think that's all the comments I have on the FIQ implementation there. What are the plans to get some of this USB code posted for review and potential merging?
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index 8b11f44..7cfef7b 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -114,6 +114,7 @@ config ARCH_BCM2835 select ARM_ERRATA_411920 select ARM_TIMER_SP804 select CLKSRC_OF + select FIQ select PINCTRL select PINCTRL_BCM2835 help diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c index 5916d6c..db66246 100644 --- a/drivers/irqchip/irq-bcm2835.c +++ b/drivers/irqchip/irq-bcm2835.c @@ -56,7 +56,7 @@ #include "irqchip.h" /* Put the bank and irq (32 bits) into the hwirq */ -#define MAKE_HWIRQ(b, n) ((b << 5) | (n)) +#define MAKE_HWIRQ(b, n) (((b) << 5) | (n)) #define HWIRQ_BANK(i) (i >> 5) #define HWIRQ_BIT(i) BIT(i & 0x1f) @@ -72,9 +72,13 @@ | SHORTCUT1_MASK | SHORTCUT2_MASK) #define REG_FIQ_CONTROL 0x0c +#define REG_FIQ_ENABLE 0x80 +#define REG_FIQ_DISABLE 0 #define NR_BANKS 3 #define IRQS_PER_BANK 32 +#define NUMBER_IRQS MAKE_HWIRQ(NR_BANKS, 0) +#define FIQ_START (NR_IRQS_BANK0 + MAKE_HWIRQ(NR_BANKS - 1, 0)) static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 }; static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 }; @@ -98,14 +102,38 @@ static struct armctrl_ic intc __read_mostly; static void __exception_irq_entry bcm2835_handle_irq( struct pt_regs *regs); +static inline unsigned int hwirq_to_fiq(unsigned long hwirq) +{ + hwirq -= NUMBER_IRQS; + /* + * The hwirq numbering used in this driver is: + * BASE (0-7) GPU1 (32-63) GPU2 (64-95). + * This differ from the one used in the FIQ register: + * GPU1 (0-31) GPU2 (32-63) BASE (64-71) + */ + if (hwirq >= 32) + return hwirq - 32; + + return hwirq + 64; +} + static void armctrl_mask_irq(struct irq_data *d) { - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]); + if (d->hwirq >= NUMBER_IRQS) + writel_relaxed(REG_FIQ_DISABLE, intc.base + REG_FIQ_CONTROL); + else + writel_relaxed(HWIRQ_BIT(d->hwirq), + intc.disable[HWIRQ_BANK(d->hwirq)]); } static void armctrl_unmask_irq(struct irq_data *d) { - writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]); + if (d->hwirq >= NUMBER_IRQS) + writel_relaxed(REG_FIQ_ENABLE | hwirq_to_fiq(d->hwirq), + intc.base + REG_FIQ_CONTROL); + else + writel_relaxed(HWIRQ_BIT(d->hwirq), + intc.enable[HWIRQ_BANK(d->hwirq)]); } static struct irq_chip armctrl_chip = { @@ -150,8 +178,9 @@ static int __init armctrl_of_init(struct device_node *node, panic("%s: unable to map IC registers\n", node->full_name); - intc.domain = irq_domain_add_linear(node, MAKE_HWIRQ(NR_BANKS, 0), - &armctrl_ops, NULL); + intc.base = base; + intc.domain = irq_domain_add_linear(node, NUMBER_IRQS * 2, + &armctrl_ops, NULL); if (!intc.domain) panic("%s: unable to create IRQ domain\n", node->full_name); @@ -168,8 +197,20 @@ static int __init armctrl_of_init(struct device_node *node, set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); } } - set_handle_irq(bcm2835_handle_irq); + + /* Make a duplicate irq range which is used to enable FIQ */ + for (b = 0; b < NR_BANKS; b++) { + for (i = 0; i < bank_irqs[b]; i++) { + irq = irq_create_mapping(intc.domain, + MAKE_HWIRQ(b, i) + NUMBER_IRQS); + BUG_ON(irq <= 0); + irq_set_chip(irq, &armctrl_chip); + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); + } + } + init_FIQ(FIQ_START); + return 0; }
Add a duplicate irq range with an offset on the hwirq's so the driver can detect that enable_fiq() is used. Tested with downstream dwc_otg USB controller driver. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- arch/arm/mach-bcm/Kconfig | 1 + drivers/irqchip/irq-bcm2835.c | 53 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 6 deletions(-)