Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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  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)_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  writes:

> On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
>> Russell King - ARM Linux  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.


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
> Russell King - ARM Linux  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  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.


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  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.


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
> Russell King - ARM Linux  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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  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)_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  writes:

> On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
>> Russell King - ARM Linux  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.


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  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  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 
>> >>---
>> >>  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_ENABLE0x80
>> >>+#define REG_FIQ_DISABLE   0
>> >>  #define NR_BANKS 3
>> >>  #define IRQS_PER_BANK32
>> >>+#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 
>> 
>> 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 

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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  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 
> >>---
> >>  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_DISABLE0
> >>  #define NR_BANKS  3
> >>  #define IRQS_PER_BANK 32
> >>+#define NUMBER_IRQSMAKE_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 
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Russell King - ARM Linux  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  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 
>> >>---
>> >>  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_ENABLE0x80
>> >>+#define REG_FIQ_DISABLE   0
>> >>  #define NR_BANKS 3
>> >>  #define IRQS_PER_BANK32
>> >>+#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 
>> 
>> 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 

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Russell King - ARM Linux]

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  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 
> >>---
> >>  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_DISABLE0
> >>  #define NR_BANKS  3
> >>  #define IRQS_PER_BANK 32
> >>+#define NUMBER_IRQSMAKE_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 
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

Den 22.07.2015 23:32, skrev Eric Anholt:

Noralf Trønnes  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 
---
  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_DISABLE0
  
  #define NR_BANKS		3

  #define IRQS_PER_BANK 32
+#define NUMBER_IRQSMAKE_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 


It seems that this has slipped through the cracks, at least it didn't
enter in the 4.3 merge window.


Noralf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

Den 22.07.2015 23:32, skrev Eric Anholt:

Noralf Trønnes  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 
---
  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_DISABLE0
  
  #define NR_BANKS		3

  #define IRQS_PER_BANK 32
+#define NUMBER_IRQSMAKE_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 


It seems that this has slipped through the cracks, at least it didn't
enter in the 4.3 merge window.


Noralf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Stephen Warren]

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 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Stephen Warren]

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 swar...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Noralf Trønnes  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 
> ---
>  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_BANK32
> +#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 


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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:

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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Eric Anholt]

Noralf Trønnes nor...@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 nor...@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_BANK32
 +#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 e...@anholt.net


signature.asc
Description: PGP signature

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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:

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?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Stephen Warren]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Stephen Warren]

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.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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.

(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())
> ERROR;
> set_fiq_handler(...)
> set_fiq_regs();
> enable_fiq(irq);
> local_fiq_enable();

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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.

(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(regs);
 enable_fiq(irq);
 local_fiq_enable();

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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:

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())
ERROR;
set_fiq_handler(...)
set_fiq_regs();
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 = 0x;
+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 
-#include 
 #include 

-
 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@7e98 {
compatible = "brcm,bcm2708-usb";
reg = <0x7e98 0x1>,
  <0x7e006000 0x1000>;
interrupts = <2 0>,
 <1 9>;
};


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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:

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(regs);
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 = 0x;
+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@7e98 {
compatible = brcm,bcm2708-usb;
reg = 0x7e98 0x1,
  0x7e006000 0x1000;
interrupts = 2 0,
 1 9;
};


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[Noralf Trønnes]

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.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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?

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irqchip: bcm2835: Add FIQ support

[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?

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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/