* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-06-12 17:26 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-12 17:26 UTC (permalink / raw)
To: tglx, jason
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel,
Noralf Trønnes
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)]);
}
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;
}
--
2.2.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-06-12 17:26 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-12 17:26 UTC (permalink / raw)
To: linux-arm-kernel
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)]);
}
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;
}
--
2.2.2
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-12 17:26 ` Noralf Trønnes
@ 2015-06-18 2:26 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-06-18 2:26 UTC (permalink / raw)
To: Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-06-18 2:26 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-06-18 2:26 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-18 2:26 ` Stephen Warren
@ 2015-06-18 13:32 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-18 13:32 UTC (permalink / raw)
To: Stephen Warren
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
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?
I can separate it out.
> 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.
claim_fiq() protects the FIQ. See d) answer below.
> c) The DT binding needs updating to describe the extra IRQs:
>
>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
Ok.
> 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?
set_fiq_handler() sets the handler and enable_fiq() enables it:
if (claim_fiq(&fh))
ERROR;
set_fiq_handler(...)
set_fiq_regs(®s);
enable_fiq(irq);
local_fiq_enable();
Downstream dwc_otg
------------------
FIQ handler:
https://github.com/raspberrypi/linux/blob/rpi-4.0.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S
FIQ is set up in hcd_init_fiq():
https://github.com/raspberrypi/linux/blob/rpi-4.0.y/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
This patch is also necessary:
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_driver.c
b/drivers/usb/host/dwc_otg/dwc_otg_driver.c
index 53307f0..95edadf 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_driver.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_driver.c
@@ -723,6 +723,7 @@ static int dwc_otg_driver_probe(
memset(dwc_otg_device, 0, sizeof(*dwc_otg_device));
dwc_otg_device->os_dep.reg_offset = 0xFFFFFFFF;
+ dwc_otg_device->os_dep.platformdev = _dev;
/*
* Map the DWC_otg Core memory into virtual address space.
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
index 8a31562..2961985 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
@@ -36,10 +36,8 @@
#include "dwc_otg_regs.h"
#include <linux/jiffies.h>
-#include <mach/hardware.h>
#include <asm/fiq.h>
-
extern bool microframe_schedule;
/** @file
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
index 6aad9c4..0440c66 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
@@ -445,7 +445,11 @@ static void hcd_init_fiq(void *cookie)
DWC_WARN("MPHI periph has NOT been enabled");
#endif
// Enable FIQ interrupt from USB peripheral
+#ifdef CONFIG_ARCH_BCM2835
+ enable_fiq(platform_get_irq(otg_dev->os_dep.platformdev, 1));
+#else
enable_fiq(INTERRUPT_VC_USB);
+#endif
local_fiq_enable();
}
DT node:
usb: usb@7e980000 {
compatible = "brcm,bcm2708-usb";
reg = <0x7e980000 0x10000>,
<0x7e006000 0x1000>;
interrupts = <2 0>,
<1 9>;
};
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-06-18 13:32 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-18 13:32 UTC (permalink / raw)
To: linux-arm-kernel
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?
I can separate it out.
> 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.
claim_fiq() protects the FIQ. See d) answer below.
> c) The DT binding needs updating to describe the extra IRQs:
>
>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
Ok.
> 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?
set_fiq_handler() sets the handler and enable_fiq() enables it:
if (claim_fiq(&fh))
ERROR;
set_fiq_handler(...)
set_fiq_regs(®s);
enable_fiq(irq);
local_fiq_enable();
Downstream dwc_otg
------------------
FIQ handler:
https://github.com/raspberrypi/linux/blob/rpi-4.0.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S
FIQ is set up in hcd_init_fiq():
https://github.com/raspberrypi/linux/blob/rpi-4.0.y/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
This patch is also necessary:
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_driver.c
b/drivers/usb/host/dwc_otg/dwc_otg_driver.c
index 53307f0..95edadf 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_driver.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_driver.c
@@ -723,6 +723,7 @@ static int dwc_otg_driver_probe(
memset(dwc_otg_device, 0, sizeof(*dwc_otg_device));
dwc_otg_device->os_dep.reg_offset = 0xFFFFFFFF;
+ dwc_otg_device->os_dep.platformdev = _dev;
/*
* Map the DWC_otg Core memory into virtual address space.
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
index 8a31562..2961985 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c
@@ -36,10 +36,8 @@
#include "dwc_otg_regs.h"
#include <linux/jiffies.h>
-#include <mach/hardware.h>
#include <asm/fiq.h>
-
extern bool microframe_schedule;
/** @file
diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
index 6aad9c4..0440c66 100644
--- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
+++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c
@@ -445,7 +445,11 @@ static void hcd_init_fiq(void *cookie)
DWC_WARN("MPHI periph has NOT been enabled");
#endif
// Enable FIQ interrupt from USB peripheral
+#ifdef CONFIG_ARCH_BCM2835
+ enable_fiq(platform_get_irq(otg_dev->os_dep.platformdev, 1));
+#else
enable_fiq(INTERRUPT_VC_USB);
+#endif
local_fiq_enable();
}
DT node:
usb: usb at 7e980000 {
compatible = "brcm,bcm2708-usb";
reg = <0x7e980000 0x10000>,
<0x7e006000 0x1000>;
interrupts = <2 0>,
<1 9>;
};
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-18 13:32 ` Noralf Trønnes
@ 2015-06-18 16:23 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-18 16:23 UTC (permalink / raw)
To: Stephen Warren
Cc: linux-kernel, tglx, jason, linux-arm-kernel, linux-rpi-kernel
Den 18.06.2015 15:32, skrev Noralf Trønnes:
>
> 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:
>>
>> c) The DT binding needs updating to describe the extra IRQs:
>>
>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>
>
> Ok.
>
I have seconds thoughts on this:
This patch does not change the DT bindings so I don't see what update
I should make. This patch only adds support for the Linux way of
handling FIQ's through enable_fiq(). It doesn't change how interrupts
are described in the DT.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-06-18 16:23 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-06-18 16:23 UTC (permalink / raw)
To: linux-arm-kernel
Den 18.06.2015 15:32, skrev Noralf Tr?nnes:
>
> 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:
>>
>> c) The DT binding needs updating to describe the extra IRQs:
>>
>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>
>
> Ok.
>
I have seconds thoughts on this:
This patch does not change the DT bindings so I don't see what update
I should make. This patch only adds support for the Linux way of
handling FIQ's through enable_fiq(). It doesn't change how interrupts
are described in the DT.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-18 13:32 ` Noralf Trønnes
@ 2015-07-11 4:09 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-11 4:09 UTC (permalink / raw)
To: Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
(Sorry for the slow reply; I was on vacation)
On 06/18/2015 07:32 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:
>> 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.
>
> claim_fiq() protects the FIQ. See d) answer below.
That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
doesn't do anything special to prevent clients from using IRQs n..2n-1
via the existing IRQ APIs, it's quite possible the a buggy client would.
(From another email):
>>> c) The DT binding needs updating to describe the extra IRQs:
>>>
>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>
>> Ok.
>
> I have seconds thoughts on this:
> This patch does not change the DT bindings so I don't see what update
> I should make. This patch only adds support for the Linux way of
> handling FIQ's through enable_fiq(). It doesn't change how interrupts
> are described in the DT.
The intention of the patch may not be to expand the set of IRQs
available via DT, but it does in practice. I think you need to add a
custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
be translated from DT, and not IRQs n..2n-1. If you do that, then I
agree that no DT binding update should be required.
Even with a custom of_xlate function, some code could hard-code an IRQ
number and hence end up registering a FIQ handler that way. However, I
guess that's a bug that the driver doesn't need to solve. We can just
fix that bug in the kernel code in that case. The same argument doesn't
apply to bad DTs; we need to more aggressively protect against that case.
>> 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?
>
> set_fiq_handler() sets the handler and enable_fiq() enables it:
>
> if (claim_fiq(&fh))
> ERROR;
> set_fiq_handler(...)
> set_fiq_regs(®s);
> enable_fiq(irq);
> local_fiq_enable();
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-11 4:09 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-11 4:09 UTC (permalink / raw)
To: linux-arm-kernel
(Sorry for the slow reply; I was on vacation)
On 06/18/2015 07:32 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:
>> 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.
>
> claim_fiq() protects the FIQ. See d) answer below.
That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
doesn't do anything special to prevent clients from using IRQs n..2n-1
via the existing IRQ APIs, it's quite possible the a buggy client would.
(From another email):
>>> c) The DT binding needs updating to describe the extra IRQs:
>>>
>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>
>> Ok.
>
> I have seconds thoughts on this:
> This patch does not change the DT bindings so I don't see what update
> I should make. This patch only adds support for the Linux way of
> handling FIQ's through enable_fiq(). It doesn't change how interrupts
> are described in the DT.
The intention of the patch may not be to expand the set of IRQs
available via DT, but it does in practice. I think you need to add a
custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
be translated from DT, and not IRQs n..2n-1. If you do that, then I
agree that no DT binding update should be required.
Even with a custom of_xlate function, some code could hard-code an IRQ
number and hence end up registering a FIQ handler that way. However, I
guess that's a bug that the driver doesn't need to solve. We can just
fix that bug in the kernel code in that case. The same argument doesn't
apply to bad DTs; we need to more aggressively protect against that case.
>> 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?
>
> set_fiq_handler() sets the handler and enable_fiq() enables it:
>
> if (claim_fiq(&fh))
> ERROR;
> set_fiq_handler(...)
> set_fiq_regs(®s);
> enable_fiq(irq);
> local_fiq_enable();
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-11 4:09 ` Stephen Warren
@ 2015-07-11 15:26 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-11 15:26 UTC (permalink / raw)
To: Stephen Warren
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
Den 11.07.2015 06:09, skrev Stephen Warren:
> (Sorry for the slow reply; I was on vacation)
>
> On 06/18/2015 07:32 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:
>>> 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.
>> claim_fiq() protects the FIQ. See d) answer below.
> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
> doesn't do anything special to prevent clients from using IRQs n..2n-1
> via the existing IRQ APIs, it's quite possible the a buggy client would.
Yes, but doesn't this apply to all irq use, using the wrong one doesn't
work.
If FIQ's where in more common use, we might have seen a FIQ IRQ flag instead
of special FIQ irqs.
> (From another email):
>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>
>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>> Ok.
>> I have seconds thoughts on this:
>> This patch does not change the DT bindings so I don't see what update
>> I should make. This patch only adds support for the Linux way of
>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>> are described in the DT.
> The intention of the patch may not be to expand the set of IRQs
> available via DT, but it does in practice. I think you need to add a
> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
> be translated from DT, and not IRQs n..2n-1. If you do that, then I
> agree that no DT binding update should be required.
armctrl_xlate() maps to the same hwirqs as before. This patch adds a
new range of hwirqs at the end of the "real" hwirq range.
It's not possible to get to these FIQ shadow hwirqs through DT.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-11 15:26 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-11 15:26 UTC (permalink / raw)
To: linux-arm-kernel
Den 11.07.2015 06:09, skrev Stephen Warren:
> (Sorry for the slow reply; I was on vacation)
>
> On 06/18/2015 07:32 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:
>>> 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.
>> claim_fiq() protects the FIQ. See d) answer below.
> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
> doesn't do anything special to prevent clients from using IRQs n..2n-1
> via the existing IRQ APIs, it's quite possible the a buggy client would.
Yes, but doesn't this apply to all irq use, using the wrong one doesn't
work.
If FIQ's where in more common use, we might have seen a FIQ IRQ flag instead
of special FIQ irqs.
> (From another email):
>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>
>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>> Ok.
>> I have seconds thoughts on this:
>> This patch does not change the DT bindings so I don't see what update
>> I should make. This patch only adds support for the Linux way of
>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>> are described in the DT.
> The intention of the patch may not be to expand the set of IRQs
> available via DT, but it does in practice. I think you need to add a
> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
> be translated from DT, and not IRQs n..2n-1. If you do that, then I
> agree that no DT binding update should be required.
armctrl_xlate() maps to the same hwirqs as before. This patch adds a
new range of hwirqs at the end of the "real" hwirq range.
It's not possible to get to these FIQ shadow hwirqs through DT.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-11 15:26 ` Noralf Trønnes
@ 2015-07-14 4:50 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-14 4:50 UTC (permalink / raw)
To: Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
On 07/11/2015 09:26 AM, Noralf Trønnes wrote:
>
> Den 11.07.2015 06:09, skrev Stephen Warren:
>> (Sorry for the slow reply; I was on vacation)
>>
>> On 06/18/2015 07:32 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:
>>>> 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.
>>> claim_fiq() protects the FIQ. See d) answer below.
>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>> via the existing IRQ APIs, it's quite possible the a buggy client would.
>
> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
> work.
> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
> instead
> of special FIQ irqs.
>
>> (From another email):
>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>
>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>
>>>> Ok.
>>> I have seconds thoughts on this:
>>> This patch does not change the DT bindings so I don't see what update
>>> I should make. This patch only adds support for the Linux way of
>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>> are described in the DT.
>> The intention of the patch may not be to expand the set of IRQs
>> available via DT, but it does in practice. I think you need to add a
>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>> agree that no DT binding update should be required.
>
> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
> new range of hwirqs at the end of the "real" hwirq range.
> It's not possible to get to these FIQ shadow hwirqs through DT.
What prevents a DT from (incorrectly) referencing the extra hwirqs?
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-14 4:50 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-14 4:50 UTC (permalink / raw)
To: linux-arm-kernel
On 07/11/2015 09:26 AM, Noralf Tr?nnes wrote:
>
> Den 11.07.2015 06:09, skrev Stephen Warren:
>> (Sorry for the slow reply; I was on vacation)
>>
>> On 06/18/2015 07:32 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:
>>>> 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.
>>> claim_fiq() protects the FIQ. See d) answer below.
>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>> via the existing IRQ APIs, it's quite possible the a buggy client would.
>
> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
> work.
> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
> instead
> of special FIQ irqs.
>
>> (From another email):
>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>
>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>
>>>> Ok.
>>> I have seconds thoughts on this:
>>> This patch does not change the DT bindings so I don't see what update
>>> I should make. This patch only adds support for the Linux way of
>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>> are described in the DT.
>> The intention of the patch may not be to expand the set of IRQs
>> available via DT, but it does in practice. I think you need to add a
>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>> agree that no DT binding update should be required.
>
> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
> new range of hwirqs at the end of the "real" hwirq range.
> It's not possible to get to these FIQ shadow hwirqs through DT.
What prevents a DT from (incorrectly) referencing the extra hwirqs?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-14 4:50 ` Stephen Warren
@ 2015-07-14 11:48 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-14 11:48 UTC (permalink / raw)
To: Stephen Warren
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
Den 14.07.2015 06:50, skrev Stephen Warren:
> On 07/11/2015 09:26 AM, Noralf Trønnes wrote:
>> Den 11.07.2015 06:09, skrev Stephen Warren:
>>> (Sorry for the slow reply; I was on vacation)
>>>
>>> On 06/18/2015 07:32 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:
>>>>> 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.
>>>> claim_fiq() protects the FIQ. See d) answer below.
>>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
>>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>>> via the existing IRQ APIs, it's quite possible the a buggy client would.
>> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
>> work.
>> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
>> instead
>> of special FIQ irqs.
>>
>>> (From another email):
>>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>>
>>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>>
>>>>> Ok.
>>>> I have seconds thoughts on this:
>>>> This patch does not change the DT bindings so I don't see what update
>>>> I should make. This patch only adds support for the Linux way of
>>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>>> are described in the DT.
>>> The intention of the patch may not be to expand the set of IRQs
>>> available via DT, but it does in practice. I think you need to add a
>>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>>> agree that no DT binding update should be required.
>> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
>> new range of hwirqs at the end of the "real" hwirq range.
>> It's not possible to get to these FIQ shadow hwirqs through DT.
> What prevents a DT from (incorrectly) referencing the extra hwirqs?
armctrl_xlate() has these limits:
if (WARN_ON(intspec[0] >= NR_BANKS))
if (WARN_ON(intspec[1] >= IRQS_PER_BANK))
if (WARN_ON(intspec[0] == 0 && intspec[1] >= NR_IRQS_BANK0))
Thus the maximum values allowed are:
intspec[0]: (NR_BANKS - 1) = 2
intspec[1]: (IRQS_PER_BANK - 1) = 31
This gives a maximum hwirq:
*out_hwirq = MAKE_HWIRQ(intspec[0], intspec[1]);
*out_hwirq = (2 << 5) | 31 = 95
The FIQ shadow hwirq range starts at 96:
irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i) + NUMBER_IRQS);
NUMBER_IRQS = MAKE_HWIRQ(NR_BANKS, 0) = 96
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-14 11:48 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-14 11:48 UTC (permalink / raw)
To: linux-arm-kernel
Den 14.07.2015 06:50, skrev Stephen Warren:
> On 07/11/2015 09:26 AM, Noralf Tr?nnes wrote:
>> Den 11.07.2015 06:09, skrev Stephen Warren:
>>> (Sorry for the slow reply; I was on vacation)
>>>
>>> On 06/18/2015 07:32 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:
>>>>> 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.
>>>> claim_fiq() protects the FIQ. See d) answer below.
>>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since this
>>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>>> via the existing IRQ APIs, it's quite possible the a buggy client would.
>> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
>> work.
>> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
>> instead
>> of special FIQ irqs.
>>
>>> (From another email):
>>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>>
>>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>>
>>>>> Ok.
>>>> I have seconds thoughts on this:
>>>> This patch does not change the DT bindings so I don't see what update
>>>> I should make. This patch only adds support for the Linux way of
>>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>>> are described in the DT.
>>> The intention of the patch may not be to expand the set of IRQs
>>> available via DT, but it does in practice. I think you need to add a
>>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>>> agree that no DT binding update should be required.
>> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
>> new range of hwirqs at the end of the "real" hwirq range.
>> It's not possible to get to these FIQ shadow hwirqs through DT.
> What prevents a DT from (incorrectly) referencing the extra hwirqs?
armctrl_xlate() has these limits:
if (WARN_ON(intspec[0] >= NR_BANKS))
if (WARN_ON(intspec[1] >= IRQS_PER_BANK))
if (WARN_ON(intspec[0] == 0 && intspec[1] >= NR_IRQS_BANK0))
Thus the maximum values allowed are:
intspec[0]: (NR_BANKS - 1) = 2
intspec[1]: (IRQS_PER_BANK - 1) = 31
This gives a maximum hwirq:
*out_hwirq = MAKE_HWIRQ(intspec[0], intspec[1]);
*out_hwirq = (2 << 5) | 31 = 95
The FIQ shadow hwirq range starts@96:
irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i) + NUMBER_IRQS);
NUMBER_IRQS = MAKE_HWIRQ(NR_BANKS, 0) = 96
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-14 11:48 ` Noralf Trønnes
@ 2015-07-22 1:50 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-22 1:50 UTC (permalink / raw)
To: Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
On 07/14/2015 05:48 AM, Noralf Trønnes wrote:
> Den 14.07.2015 06:50, skrev Stephen Warren:
>> On 07/11/2015 09:26 AM, Noralf Trønnes wrote:
>>> Den 11.07.2015 06:09, skrev Stephen Warren:
>>>> (Sorry for the slow reply; I was on vacation)
>>>>
>>>> On 06/18/2015 07:32 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:
>>>>>> 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.
>>>>> claim_fiq() protects the FIQ. See d) answer below.
>>>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since
>>>> this
>>>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>>>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>>>> via the existing IRQ APIs, it's quite possible the a buggy client
>>>> would.
>>> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
>>> work.
>>> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
>>> instead
>>> of special FIQ irqs.
>>>
>>>> (From another email):
>>>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>>>
>>>>>>>>
>>>>>> Ok.
>>>>> I have seconds thoughts on this:
>>>>> This patch does not change the DT bindings so I don't see what update
>>>>> I should make. This patch only adds support for the Linux way of
>>>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>>>> are described in the DT.
>>>> The intention of the patch may not be to expand the set of IRQs
>>>> available via DT, but it does in practice. I think you need to add a
>>>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>>>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>>>> agree that no DT binding update should be required.
>>> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
>>> new range of hwirqs at the end of the "real" hwirq range.
>>> It's not possible to get to these FIQ shadow hwirqs through DT.
>> What prevents a DT from (incorrectly) referencing the extra hwirqs?
>
> armctrl_xlate() has these limits:
>
> if (WARN_ON(intspec[0] >= NR_BANKS))
> if (WARN_ON(intspec[1] >= IRQS_PER_BANK))
> if (WARN_ON(intspec[0] == 0 && intspec[1] >= NR_IRQS_BANK0))
>
> Thus the maximum values allowed are:
> intspec[0]: (NR_BANKS - 1) = 2
> intspec[1]: (IRQS_PER_BANK - 1) = 31
>
> This gives a maximum hwirq:
> *out_hwirq = MAKE_HWIRQ(intspec[0], intspec[1]);
> *out_hwirq = (2 << 5) | 31 = 95
>
> The FIQ shadow hwirq range starts at 96:
> irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i) + NUMBER_IRQS);
>
> NUMBER_IRQS = MAKE_HWIRQ(NR_BANKS, 0) = 96
Great, thanks for the explanation. That should be fine then.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-22 1:50 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-22 1:50 UTC (permalink / raw)
To: linux-arm-kernel
On 07/14/2015 05:48 AM, Noralf Tr?nnes wrote:
> Den 14.07.2015 06:50, skrev Stephen Warren:
>> On 07/11/2015 09:26 AM, Noralf Tr?nnes wrote:
>>> Den 11.07.2015 06:09, skrev Stephen Warren:
>>>> (Sorry for the slow reply; I was on vacation)
>>>>
>>>> On 06/18/2015 07:32 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:
>>>>>> 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.
>>>>> claim_fiq() protects the FIQ. See d) answer below.
>>>> That assumes the IRQ is "accessed" via the fiq-specific APIs. Since
>>>> this
>>>> patch changes the IRQ domain from having n IRQs to having 2*n IRQs, and
>>>> doesn't do anything special to prevent clients from using IRQs n..2n-1
>>>> via the existing IRQ APIs, it's quite possible the a buggy client
>>>> would.
>>> Yes, but doesn't this apply to all irq use, using the wrong one doesn't
>>> work.
>>> If FIQ's where in more common use, we might have seen a FIQ IRQ flag
>>> instead
>>> of special FIQ irqs.
>>>
>>>> (From another email):
>>>>>>> c) The DT binding needs updating to describe the extra IRQs:
>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
>>>>>>>>
>>>>>>>>
>>>>>> Ok.
>>>>> I have seconds thoughts on this:
>>>>> This patch does not change the DT bindings so I don't see what update
>>>>> I should make. This patch only adds support for the Linux way of
>>>>> handling FIQ's through enable_fiq(). It doesn't change how interrupts
>>>>> are described in the DT.
>>>> The intention of the patch may not be to expand the set of IRQs
>>>> available via DT, but it does in practice. I think you need to add a
>>>> custom of_xlate for the IRQ domain to ensure that only IRQs 0..n-1 can
>>>> be translated from DT, and not IRQs n..2n-1. If you do that, then I
>>>> agree that no DT binding update should be required.
>>> armctrl_xlate() maps to the same hwirqs as before. This patch adds a
>>> new range of hwirqs at the end of the "real" hwirq range.
>>> It's not possible to get to these FIQ shadow hwirqs through DT.
>> What prevents a DT from (incorrectly) referencing the extra hwirqs?
>
> armctrl_xlate() has these limits:
>
> if (WARN_ON(intspec[0] >= NR_BANKS))
> if (WARN_ON(intspec[1] >= IRQS_PER_BANK))
> if (WARN_ON(intspec[0] == 0 && intspec[1] >= NR_IRQS_BANK0))
>
> Thus the maximum values allowed are:
> intspec[0]: (NR_BANKS - 1) = 2
> intspec[1]: (IRQS_PER_BANK - 1) = 31
>
> This gives a maximum hwirq:
> *out_hwirq = MAKE_HWIRQ(intspec[0], intspec[1]);
> *out_hwirq = (2 << 5) | 31 = 95
>
> The FIQ shadow hwirq range starts at 96:
> irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i) + NUMBER_IRQS);
>
> NUMBER_IRQS = MAKE_HWIRQ(NR_BANKS, 0) = 96
Great, thanks for the explanation. That should be fine then.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-18 2:26 ` Stephen Warren
@ 2015-07-22 14:07 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-22 14:07 UTC (permalink / raw)
To: Stephen Warren
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-22 14:07 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-07-22 14:07 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-06-12 17:26 ` Noralf Trønnes
@ 2015-07-22 21:32 ` Eric Anholt
-1 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-07-22 21:32 UTC (permalink / raw)
To: Noralf Trønnes, tglx, jason
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel,
Noralf Trønnes
[-- Attachment #1: Type: text/plain, Size: 3588 bytes --]
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>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-22 21:32 ` Eric Anholt
0 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-07-22 21:32 UTC (permalink / raw)
To: linux-arm-kernel
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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150722/b54c8391/attachment-0001.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-22 14:07 ` Noralf Trønnes
@ 2015-07-24 4:04 ` Stephen Warren
-1 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-24 4:04 UTC (permalink / raw)
To: Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-kernel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-07-24 4:04 ` Stephen Warren
0 siblings, 0 replies; 40+ messages in thread
From: Stephen Warren @ 2015-07-24 4:04 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-07-22 21:32 ` Eric Anholt
@ 2015-09-13 19:24 ` Noralf Trønnes
-1 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-09-13 19:24 UTC (permalink / raw)
To: Eric Anholt, tglx, jason; +Cc: linux-rpi-kernel, linux-arm-kernel, linux-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-13 19:24 ` Noralf Trønnes
0 siblings, 0 replies; 40+ messages in thread
From: Noralf Trønnes @ 2015-09-13 19:24 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-13 19:24 ` Noralf Trønnes
@ 2015-09-14 9:08 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-14 9:08 UTC (permalink / raw)
To: Noralf Trønnes
Cc: Eric Anholt, tglx, jason, linux-rpi-kernel, linux-arm-kernel,
linux-kernel
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?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-14 9:08 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-14 9:08 UTC (permalink / raw)
To: linux-arm-kernel
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?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-14 9:08 ` Russell King - ARM Linux
@ 2015-09-14 14:33 ` Eric Anholt
-1 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-14 14:33 UTC (permalink / raw)
To: Russell King - ARM Linux, Noralf Trønnes
Cc: tglx, jason, linux-rpi-kernel, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5282 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-14 14:33 ` Eric Anholt
0 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-14 14:33 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150914/c390c1b0/attachment-0001.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-14 14:33 ` Eric Anholt
@ 2015-09-14 14:34 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-14 14:34 UTC (permalink / raw)
To: Eric Anholt
Cc: Noralf Trønnes, tglx, jason, linux-rpi-kernel,
linux-arm-kernel, linux-kernel
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.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-14 14:34 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-14 14:34 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-14 14:34 ` Russell King - ARM Linux
@ 2015-09-16 14:02 ` Eric Anholt
-1 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-16 14:02 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Noralf Trønnes, tglx, jason, linux-rpi-kernel,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 712 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-16 14:02 ` Eric Anholt
0 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-16 14:02 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150916/db54ec0e/attachment-0001.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-16 14:02 ` Eric Anholt
@ 2015-09-16 16:21 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-16 16:21 UTC (permalink / raw)
To: Eric Anholt
Cc: Noralf Trønnes, tglx, jason, linux-rpi-kernel,
linux-arm-kernel, linux-kernel
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.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-16 16:21 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-16 16:21 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-16 16:21 ` Russell King - ARM Linux
@ 2015-09-16 18:48 ` Eric Anholt
-1 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-16 18:48 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Noralf Trønnes, tglx, jason, linux-rpi-kernel,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-16 18:48 ` Eric Anholt
0 siblings, 0 replies; 40+ messages in thread
From: Eric Anholt @ 2015-09-16 18:48 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150916/49a2a36d/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] irqchip: bcm2835: Add FIQ support
2015-09-16 16:21 ` Russell King - ARM Linux
@ 2015-09-16 19:13 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-16 19:13 UTC (permalink / raw)
To: Eric Anholt
Cc: jason, linux-kernel, Noralf Trønnes, linux-rpi-kernel, tglx,
linux-arm-kernel
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?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] irqchip: bcm2835: Add FIQ support
@ 2015-09-16 19:13 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2015-09-16 19:13 UTC (permalink / raw)
To: linux-arm-kernel
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?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2015-09-16 19:13 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-12 17:26 [PATCH] irqchip: bcm2835: Add FIQ support Noralf Trønnes
2015-06-12 17:26 ` Noralf Trønnes
2015-06-18 2:26 ` Stephen Warren
2015-06-18 2:26 ` Stephen Warren
2015-06-18 13:32 ` Noralf Trønnes
2015-06-18 13:32 ` Noralf Trønnes
2015-06-18 16:23 ` Noralf Trønnes
2015-06-18 16:23 ` Noralf Trønnes
2015-07-11 4:09 ` Stephen Warren
2015-07-11 4:09 ` Stephen Warren
2015-07-11 15:26 ` Noralf Trønnes
2015-07-11 15:26 ` Noralf Trønnes
2015-07-14 4:50 ` Stephen Warren
2015-07-14 4:50 ` Stephen Warren
2015-07-14 11:48 ` Noralf Trønnes
2015-07-14 11:48 ` Noralf Trønnes
2015-07-22 1:50 ` Stephen Warren
2015-07-22 1:50 ` Stephen Warren
2015-07-22 14:07 ` Noralf Trønnes
2015-07-22 14:07 ` Noralf Trønnes
2015-07-24 4:04 ` Stephen Warren
2015-07-24 4:04 ` Stephen Warren
2015-07-22 21:32 ` Eric Anholt
2015-07-22 21:32 ` Eric Anholt
2015-09-13 19:24 ` Noralf Trønnes
2015-09-13 19:24 ` Noralf Trønnes
2015-09-14 9:08 ` Russell King - ARM Linux
2015-09-14 9:08 ` Russell King - ARM Linux
2015-09-14 14:33 ` Eric Anholt
2015-09-14 14:33 ` Eric Anholt
2015-09-14 14:34 ` Russell King - ARM Linux
2015-09-14 14:34 ` Russell King - ARM Linux
2015-09-16 14:02 ` Eric Anholt
2015-09-16 14:02 ` Eric Anholt
2015-09-16 16:21 ` Russell King - ARM Linux
2015-09-16 16:21 ` Russell King - ARM Linux
2015-09-16 18:48 ` Eric Anholt
2015-09-16 18:48 ` Eric Anholt
2015-09-16 19:13 ` Russell King - ARM Linux
2015-09-16 19:13 ` Russell King - ARM Linux
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.