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 <[email protected]>
---
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
On 06/12/2015 11:26 AM, Noralf Trønnes wrote:
> Add a duplicate irq range with an offset on the hwirq's so the
> driver can detect that enable_fiq() is used.
> Tested with downstream dwc_otg USB controller driver.
This basically looks OK, but a few comments/thoughts:
a) Should the Kconfig change be a separate patch since it's a separate
subsystem?
b) Doesn't the driver need to refuse some operation (handler
registration, IRQ setup, IRQ enable, ...?) for more than 1 IRQ in the
FIQ range, since the FIQ control register only allows routing 1 IRQ to FIQ.
c) The DT binding needs updating to describe the extra IRQs:
> Documentation/devicetree/bindings/interrupt-controller/brcm,bcm28armctrl-ic.txt
d) I wonder how the FIQ handler actually gets routed to this controller
and hooked to its handler etc. I assume there's a separate patch for
that coming?
BTW, I'll be on vacation for just over a couple weeks starting Saturday,
so responses will certainly be delayed for a quite a while.
Den 18.06.2015 04:26, skrev Stephen Warren:
> On 06/12/2015 11:26 AM, Noralf Trønnes wrote:
>> Add a duplicate irq range with an offset on the hwirq's so the
>> driver can detect that enable_fiq() is used.
>> Tested with downstream dwc_otg USB controller driver.
> This basically looks OK, but a few comments/thoughts:
>
> a) Should the Kconfig change be a separate patch since it's a separate
> subsystem?
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>;
};
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.
(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();
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.
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?
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
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.
Den 18.06.2015 04:26, skrev Stephen Warren:
> On 06/12/2015 11:26 AM, Noralf Trønnes wrote:
>> Add a duplicate irq range with an offset on the hwirq's so the
>> driver can detect that enable_fiq() is used.
>> Tested with downstream dwc_otg USB controller driver.
> This basically looks OK, but a few comments/thoughts:
>
> a) Should the Kconfig change be a separate patch since it's a separate
> subsystem?
Is this a yes since you haven't acked it?
Noralf Trønnes <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
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.