mbox

[v3,00/25] irq_domain generalization and refinement

Message ID 1327700179-17454-1-git-send-email-grant.likely@secretlab.ca
State New
Headers show

Pull-request

git://git.secretlab.ca/git/linux-2.6 irqdomain/next

Message

Grant Likely Jan. 27, 2012, 9:35 p.m. UTC
Hey everyone,

This patch series is ready for much wider consumption now.  I'd like
to get it into linux-next ASAP because there will be ARM board support
depending on it.  I'll wait a few days before I ask Stephen to pull
this in.

Stephen/Milton/Ben, any testing you can help with here would be
appreciated since you've got access to a wider variety of Power
machines than I do.

Thomas, I think it makes sense to maintain this in a separate branch
from other irq changes until the next merge window.  If you prefer,
I'm happy to maintain this branch until then.

Cheers,
g.

The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f:

  Linux 3.3-rc1 (2012-01-19 15:04:48 -0800)

are available in the git repository at:
  git://git.secretlab.ca/git/linux-2.6 irqdomain/next

Grant Likely (24):
      irq_domain: add documentation and MAINTAINERS entry.
      dt: Make irqdomain less verbose
      irq_domain: Make irq_domain structure match powerpc's irq_host
      irq_domain: convert microblaze from irq_host to irq_domain
      irq_domain/powerpc: Use common irq_domain structure instead of irq_host
      irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
      irq_domain/powerpc: Eliminate virq_is_host()
      irq_domain: Move irq_domain code from powerpc to kernel/irq
      irqdomain: remove NO_IRQ from irq domain code
      irq_domain: Remove references to old irq_host names
      irq_domain: Replace irq_alloc_host() with revmap-specific initializers
      irq_domain: Add support for base irq and hwirq in legacy mappings
      irq_domain: Remove 'new' irq_domain in favour of the ppc one
      irq_domain: Remove irq_domain_add_simple()
      irq_domain: Create common xlate functions that device drivers can use
      irq_domain: constify irq_domain_ops
      irq_domain/c6x: constify irq_domain structures
      irq_domain/c6x: Use library of xlate functions
      irq_domain/powerpc: constify irq_domain_ops
      irqdomain/powerpc: Replace custom xlate functions with library functions
      irq_domain/x86: Convert x86 (embedded) to use common irq_domain
      irq_domain: Include hwirq number in /proc/interrupts
      irq_domain: remove "hint" when allocating irq numbers
      irq_domain: mostly eliminate slow-path revmap lookups

Mark Salter (1):
      irq_domain/c6x: Convert c6x to use generic irq_domain support.

 Documentation/IRQ-domain.txt                     |  113 +++
 MAINTAINERS                                      |    9 +
 arch/arm/common/gic.c                            |   95 ++--
 arch/arm/common/vic.c                            |   16 +-
 arch/arm/include/asm/hardware/gic.h              |    4 +-
 arch/arm/include/asm/hardware/vic.h              |    2 +
 arch/arm/mach-exynos/common.c                    |    2 +-
 arch/arm/mach-imx/mach-imx6q.c                   |    3 +-
 arch/arm/mach-msm/board-msm8x60.c                |    8 +-
 arch/arm/mach-mx5/imx51-dt.c                     |    4 +-
 arch/arm/mach-mx5/imx53-dt.c                     |    4 +-
 arch/arm/mach-omap2/board-generic.c              |    2 +-
 arch/arm/mach-prima2/irq.c                       |    2 +-
 arch/arm/mach-versatile/core.c                   |    5 +-
 arch/c6x/Kconfig                                 |    1 +
 arch/c6x/include/asm/irq.h                       |  245 +-------
 arch/c6x/kernel/irq.c                            |  612 +----------------
 arch/c6x/platforms/megamod-pic.c                 |   25 +-
 arch/microblaze/include/asm/irq.h                |    4 +-
 arch/microblaze/kernel/irq.c                     |    2 +-
 arch/microblaze/kernel/setup.c                   |    2 -
 arch/powerpc/Kconfig                             |    1 +
 arch/powerpc/include/asm/ehv_pic.h               |    2 +-
 arch/powerpc/include/asm/i8259.h                 |    2 +-
 arch/powerpc/include/asm/irq.h                   |  247 +-------
 arch/powerpc/include/asm/mpic.h                  |    2 +-
 arch/powerpc/include/asm/xics.h                  |    2 +-
 arch/powerpc/kernel/irq.c                        |  617 +----------------
 arch/powerpc/platforms/512x/mpc5121_ads_cpld.c   |   12 +-
 arch/powerpc/platforms/52xx/media5200.c          |   15 +-
 arch/powerpc/platforms/52xx/mpc52xx_gpt.c        |   16 +-
 arch/powerpc/platforms/52xx/mpc52xx_pic.c        |   12 +-
 arch/powerpc/platforms/82xx/pq2ads-pci-pic.c     |   14 +-
 arch/powerpc/platforms/85xx/socrates_fpga_pic.c  |   15 +-
 arch/powerpc/platforms/86xx/gef_pic.c            |   15 +-
 arch/powerpc/platforms/cell/axon_msi.c           |   29 +-
 arch/powerpc/platforms/cell/beat_interrupt.c     |   16 +-
 arch/powerpc/platforms/cell/interrupt.c          |   16 +-
 arch/powerpc/platforms/cell/spider-pic.c         |   14 +-
 arch/powerpc/platforms/embedded6xx/flipper-pic.c |   30 +-
 arch/powerpc/platforms/embedded6xx/hlwd-pic.c    |   35 +-
 arch/powerpc/platforms/iseries/irq.c             |   11 +-
 arch/powerpc/platforms/powermac/pic.c            |   26 +-
 arch/powerpc/platforms/powermac/smp.c            |    9 +-
 arch/powerpc/platforms/ps3/interrupt.c           |   11 +-
 arch/powerpc/platforms/wsp/opb_pic.c             |   26 +-
 arch/powerpc/sysdev/cpm1.c                       |    9 +-
 arch/powerpc/sysdev/cpm2_pic.c                   |   23 +-
 arch/powerpc/sysdev/ehv_pic.c                    |   14 +-
 arch/powerpc/sysdev/fsl_msi.c                    |   10 +-
 arch/powerpc/sysdev/fsl_msi.h                    |    2 +-
 arch/powerpc/sysdev/i8259.c                      |   15 +-
 arch/powerpc/sysdev/ipic.c                       |   31 +-
 arch/powerpc/sysdev/ipic.h                       |    2 +-
 arch/powerpc/sysdev/mpc8xx_pic.c                 |   11 +-
 arch/powerpc/sysdev/mpic.c                       |   17 +-
 arch/powerpc/sysdev/mpic_msi.c                   |    2 +-
 arch/powerpc/sysdev/mv64x60_pic.c                |   11 +-
 arch/powerpc/sysdev/qe_lib/qe_ic.c               |   26 +-
 arch/powerpc/sysdev/qe_lib/qe_ic.h               |    2 +-
 arch/powerpc/sysdev/tsi108_pci.c                 |   22 +-
 arch/powerpc/sysdev/uic.c                        |   26 +-
 arch/powerpc/sysdev/xics/xics-common.c           |   28 +-
 arch/powerpc/sysdev/xilinx_intc.c                |   19 +-
 arch/x86/Kconfig                                 |    2 +
 arch/x86/include/asm/irq_controller.h            |   12 -
 arch/x86/include/asm/prom.h                      |   10 -
 arch/x86/kernel/devicetree.c                     |  101 +--
 drivers/gpio/gpio-mpc8xxx.c                      |   30 +-
 drivers/mfd/twl-core.c                           |   12 +-
 drivers/net/phy/mdio-gpio.c                      |    4 +-
 include/linux/irqdomain.h                        |  190 ++++--
 kernel/irq/irqdomain.c                           |  816 +++++++++++++++++++---
 kernel/irq/proc.c                                |    3 +
 74 files changed, 1334 insertions(+), 2471 deletions(-)
 create mode 100644 Documentation/IRQ-domain.txt
 delete mode 100644 arch/x86/include/asm/irq_controller.h

Comments

Sebastian Andrzej Siewior Jan. 28, 2012, 4:44 p.m. UTC | #1
* Grant Likely | 2012-01-27 14:36:16 [-0700]:

>This patch removes the x86-specific definition of irq_domain and replaces
>it with the common implementation.

I pulled your devicetree/next tree. After this patch I get:

|Hierarchical RCU implementation.
|NR_IRQS:2304 nr_irqs:256 16
|------------[ cut here ]------------
|WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
|Modules linked in:
|Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
|Call Trace:
| [<c15044e0>] ? printk+0x18/0x1a
| [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
| [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
| [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
| [<c102ce0d>] warn_slowpath_null+0x1d/0x20
| [<c1095575>] irq_domain_add_legacy+0x75/0x150
| [<c1714824>] x86_add_irq_domains+0x96/0xd6
| [<c1708df2>] init_IRQ+0x8/0x33
| [<c170557f>] start_kernel+0x191/0x2e1
| [<c170517f>] ? loglevel+0x2b/0x2b
| [<c1705081>] i386_start_kernel+0x81/0x86
|---[ end trace 4eaa2a86a8e2da22 ]---
|------------[ cut here ]------------
|kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!

The warning is comming from this piece in irq_domain_add_legacy()
|for (i = 0; i < size; i++) {
|                 int irq = first_irq + i;
|                 struct irq_data *irq_data = irq_get_irq_data(irq);
| 
|                 if (WARN_ON(!irq_data || irq_data->domain)) {

irq_data is NULL here.

|                         mutex_unlock(&irq_domain_mutex);
|                         of_node_put(domain->of_node);
|                         kfree(domain);
|                         return NULL;
|                 }
|         }
| 

This is not always the case. arch_early_irq_init() in [0] sets up the
first 16 entries. The reminaing few (there is a toal of 24 irqs for
first ioapic and a second ioapic) are not initialized. This happens
later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.

[0] arch/x86/kernel/apic/io_apic.c

Sebastian
Randy.Dunlap Jan. 28, 2012, 6:23 p.m. UTC | #2
On 01/27/2012 01:35 PM, Grant Likely wrote:
> Documentation for irq_domain library which will be created in subsequent
> patches.

I posted a lot of comments to v2 on Jan. 24.
Looks like they were ignored.

Please see  http://marc.info/?l=linuxppc-embedded&m=132742951211808&w=2


> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Milton Miller <miltonm@bga.com>
> ---
>  Documentation/IRQ-domain.txt |  113 ++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                  |    9 +++
>  2 files changed, 122 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/IRQ-domain.txt
Rob Herring Jan. 28, 2012, 6:38 p.m. UTC | #3
On 01/27/2012 03:35 PM, Grant Likely wrote:
> Hey everyone,
> 
> This patch series is ready for much wider consumption now.  I'd like
> to get it into linux-next ASAP because there will be ARM board support
> depending on it.  I'll wait a few days before I ask Stephen to pull
> this in.
> 
> Stephen/Milton/Ben, any testing you can help with here would be
> appreciated since you've got access to a wider variety of Power
> machines than I do.
> 
> Thomas, I think it makes sense to maintain this in a separate branch
> from other irq changes until the next merge window.  If you prefer,
> I'm happy to maintain this branch until then.
> 

I picked up your irqdomain/next branch and it doesn't compile:

  CC      kernel/irq/irqdomain.o
kernel/irq/irqdomain.c: In function ‘irq_create_mapping’:
kernel/irq/irqdomain.c:403:47: error: ‘irq’ undeclared (first use in
this function)
kernel/irq/irqdomain.c:403:47: note: each undeclared identifier is
reported only once for each function it app

Rob

> Cheers,
> g.
> 
> The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f:
> 
>   Linux 3.3-rc1 (2012-01-19 15:04:48 -0800)
> 
> are available in the git repository at:
>   git://git.secretlab.ca/git/linux-2.6 irqdomain/next
> 
> Grant Likely (24):
>       irq_domain: add documentation and MAINTAINERS entry.
>       dt: Make irqdomain less verbose
>       irq_domain: Make irq_domain structure match powerpc's irq_host
>       irq_domain: convert microblaze from irq_host to irq_domain
>       irq_domain/powerpc: Use common irq_domain structure instead of irq_host
>       irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
>       irq_domain/powerpc: Eliminate virq_is_host()
>       irq_domain: Move irq_domain code from powerpc to kernel/irq
>       irqdomain: remove NO_IRQ from irq domain code
>       irq_domain: Remove references to old irq_host names
>       irq_domain: Replace irq_alloc_host() with revmap-specific initializers
>       irq_domain: Add support for base irq and hwirq in legacy mappings
>       irq_domain: Remove 'new' irq_domain in favour of the ppc one
>       irq_domain: Remove irq_domain_add_simple()
>       irq_domain: Create common xlate functions that device drivers can use
>       irq_domain: constify irq_domain_ops
>       irq_domain/c6x: constify irq_domain structures
>       irq_domain/c6x: Use library of xlate functions
>       irq_domain/powerpc: constify irq_domain_ops
>       irqdomain/powerpc: Replace custom xlate functions with library functions
>       irq_domain/x86: Convert x86 (embedded) to use common irq_domain
>       irq_domain: Include hwirq number in /proc/interrupts
>       irq_domain: remove "hint" when allocating irq numbers
>       irq_domain: mostly eliminate slow-path revmap lookups
> 
> Mark Salter (1):
>       irq_domain/c6x: Convert c6x to use generic irq_domain support.
> 
>  Documentation/IRQ-domain.txt                     |  113 +++
>  MAINTAINERS                                      |    9 +
>  arch/arm/common/gic.c                            |   95 ++--
>  arch/arm/common/vic.c                            |   16 +-
>  arch/arm/include/asm/hardware/gic.h              |    4 +-
>  arch/arm/include/asm/hardware/vic.h              |    2 +
>  arch/arm/mach-exynos/common.c                    |    2 +-
>  arch/arm/mach-imx/mach-imx6q.c                   |    3 +-
>  arch/arm/mach-msm/board-msm8x60.c                |    8 +-
>  arch/arm/mach-mx5/imx51-dt.c                     |    4 +-
>  arch/arm/mach-mx5/imx53-dt.c                     |    4 +-
>  arch/arm/mach-omap2/board-generic.c              |    2 +-
>  arch/arm/mach-prima2/irq.c                       |    2 +-
>  arch/arm/mach-versatile/core.c                   |    5 +-
>  arch/c6x/Kconfig                                 |    1 +
>  arch/c6x/include/asm/irq.h                       |  245 +-------
>  arch/c6x/kernel/irq.c                            |  612 +----------------
>  arch/c6x/platforms/megamod-pic.c                 |   25 +-
>  arch/microblaze/include/asm/irq.h                |    4 +-
>  arch/microblaze/kernel/irq.c                     |    2 +-
>  arch/microblaze/kernel/setup.c                   |    2 -
>  arch/powerpc/Kconfig                             |    1 +
>  arch/powerpc/include/asm/ehv_pic.h               |    2 +-
>  arch/powerpc/include/asm/i8259.h                 |    2 +-
>  arch/powerpc/include/asm/irq.h                   |  247 +-------
>  arch/powerpc/include/asm/mpic.h                  |    2 +-
>  arch/powerpc/include/asm/xics.h                  |    2 +-
>  arch/powerpc/kernel/irq.c                        |  617 +----------------
>  arch/powerpc/platforms/512x/mpc5121_ads_cpld.c   |   12 +-
>  arch/powerpc/platforms/52xx/media5200.c          |   15 +-
>  arch/powerpc/platforms/52xx/mpc52xx_gpt.c        |   16 +-
>  arch/powerpc/platforms/52xx/mpc52xx_pic.c        |   12 +-
>  arch/powerpc/platforms/82xx/pq2ads-pci-pic.c     |   14 +-
>  arch/powerpc/platforms/85xx/socrates_fpga_pic.c  |   15 +-
>  arch/powerpc/platforms/86xx/gef_pic.c            |   15 +-
>  arch/powerpc/platforms/cell/axon_msi.c           |   29 +-
>  arch/powerpc/platforms/cell/beat_interrupt.c     |   16 +-
>  arch/powerpc/platforms/cell/interrupt.c          |   16 +-
>  arch/powerpc/platforms/cell/spider-pic.c         |   14 +-
>  arch/powerpc/platforms/embedded6xx/flipper-pic.c |   30 +-
>  arch/powerpc/platforms/embedded6xx/hlwd-pic.c    |   35 +-
>  arch/powerpc/platforms/iseries/irq.c             |   11 +-
>  arch/powerpc/platforms/powermac/pic.c            |   26 +-
>  arch/powerpc/platforms/powermac/smp.c            |    9 +-
>  arch/powerpc/platforms/ps3/interrupt.c           |   11 +-
>  arch/powerpc/platforms/wsp/opb_pic.c             |   26 +-
>  arch/powerpc/sysdev/cpm1.c                       |    9 +-
>  arch/powerpc/sysdev/cpm2_pic.c                   |   23 +-
>  arch/powerpc/sysdev/ehv_pic.c                    |   14 +-
>  arch/powerpc/sysdev/fsl_msi.c                    |   10 +-
>  arch/powerpc/sysdev/fsl_msi.h                    |    2 +-
>  arch/powerpc/sysdev/i8259.c                      |   15 +-
>  arch/powerpc/sysdev/ipic.c                       |   31 +-
>  arch/powerpc/sysdev/ipic.h                       |    2 +-
>  arch/powerpc/sysdev/mpc8xx_pic.c                 |   11 +-
>  arch/powerpc/sysdev/mpic.c                       |   17 +-
>  arch/powerpc/sysdev/mpic_msi.c                   |    2 +-
>  arch/powerpc/sysdev/mv64x60_pic.c                |   11 +-
>  arch/powerpc/sysdev/qe_lib/qe_ic.c               |   26 +-
>  arch/powerpc/sysdev/qe_lib/qe_ic.h               |    2 +-
>  arch/powerpc/sysdev/tsi108_pci.c                 |   22 +-
>  arch/powerpc/sysdev/uic.c                        |   26 +-
>  arch/powerpc/sysdev/xics/xics-common.c           |   28 +-
>  arch/powerpc/sysdev/xilinx_intc.c                |   19 +-
>  arch/x86/Kconfig                                 |    2 +
>  arch/x86/include/asm/irq_controller.h            |   12 -
>  arch/x86/include/asm/prom.h                      |   10 -
>  arch/x86/kernel/devicetree.c                     |  101 +--
>  drivers/gpio/gpio-mpc8xxx.c                      |   30 +-
>  drivers/mfd/twl-core.c                           |   12 +-
>  drivers/net/phy/mdio-gpio.c                      |    4 +-
>  include/linux/irqdomain.h                        |  190 ++++--
>  kernel/irq/irqdomain.c                           |  816 +++++++++++++++++++---
>  kernel/irq/proc.c                                |    3 +
>  74 files changed, 1334 insertions(+), 2471 deletions(-)
>  create mode 100644 Documentation/IRQ-domain.txt
>  delete mode 100644 arch/x86/include/asm/irq_controller.h
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Grant Likely Jan. 29, 2012, 12:35 a.m. UTC | #4
On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
> 
> >This patch removes the x86-specific definition of irq_domain and replaces
> >it with the common implementation.
> 
> I pulled your devicetree/next tree. After this patch I get:
> 
> |Hierarchical RCU implementation.
> |NR_IRQS:2304 nr_irqs:256 16
> |------------[ cut here ]------------
> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
> |Modules linked in:
> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
> |Call Trace:
> | [<c15044e0>] ? printk+0x18/0x1a
> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
> | [<c1708df2>] init_IRQ+0x8/0x33
> | [<c170557f>] start_kernel+0x191/0x2e1
> | [<c170517f>] ? loglevel+0x2b/0x2b
> | [<c1705081>] i386_start_kernel+0x81/0x86
> |---[ end trace 4eaa2a86a8e2da22 ]---
> |------------[ cut here ]------------
> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
> 
> The warning is comming from this piece in irq_domain_add_legacy()
> |for (i = 0; i < size; i++) {
> |                 int irq = first_irq + i;
> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
> | 
> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
> 
> irq_data is NULL here.
> 
> |                         mutex_unlock(&irq_domain_mutex);
> |                         of_node_put(domain->of_node);
> |                         kfree(domain);
> |                         return NULL;
> |                 }
> |         }
> | 
> 
> This is not always the case. arch_early_irq_init() in [0] sets up the
> first 16 entries. The reminaing few (there is a toal of 24 irqs for
> first ioapic and a second ioapic) are not initialized. This happens
> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
> 
> [0] arch/x86/kernel/apic/io_apic.c

That's not going to work then.  The irqs must not be set up in ->xlate.
They need to be set up either before creating the irq_domain, or in the
.map hook.  Inside the map hook is preferred, but having some
configured and some not at initialization time does make it difficult

g.
Grant Likely Jan. 29, 2012, 12:39 a.m. UTC | #5
On Sat, Jan 28, 2012 at 5:35 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
>> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
>>
>> >This patch removes the x86-specific definition of irq_domain and replaces
>> >it with the common implementation.
>>
>> I pulled your devicetree/next tree. After this patch I get:
>>
>> |Hierarchical RCU implementation.
>> |NR_IRQS:2304 nr_irqs:256 16
>> |------------[ cut here ]------------
>> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
>> |Modules linked in:
>> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
>> |Call Trace:
>> | [<c15044e0>] ? printk+0x18/0x1a
>> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
>> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
>> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
>> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
>> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
>> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
>> | [<c1708df2>] init_IRQ+0x8/0x33
>> | [<c170557f>] start_kernel+0x191/0x2e1
>> | [<c170517f>] ? loglevel+0x2b/0x2b
>> | [<c1705081>] i386_start_kernel+0x81/0x86
>> |---[ end trace 4eaa2a86a8e2da22 ]---
>> |------------[ cut here ]------------
>> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
>>
>> The warning is comming from this piece in irq_domain_add_legacy()
>> |for (i = 0; i < size; i++) {
>> |                 int irq = first_irq + i;
>> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
>> |
>> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
>>
>> irq_data is NULL here.
>>
>> |                         mutex_unlock(&irq_domain_mutex);
>> |                         of_node_put(domain->of_node);
>> |                         kfree(domain);
>> |                         return NULL;
>> |                 }
>> |         }
>> |
>>
>> This is not always the case. arch_early_irq_init() in [0] sets up the
>> first 16 entries. The reminaing few (there is a toal of 24 irqs for
>> first ioapic and a second ioapic) are not initialized. This happens
>> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
>> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
>>
>> [0] arch/x86/kernel/apic/io_apic.c
>
> That's not going to work then.  The irqs must not be set up in ->xlate.
> They need to be set up either before creating the irq_domain, or in the
> .map hook.  Inside the map hook is preferred, but having some
> configured and some not at initialization time does make it difficult

Actually, that's still not right.  irq_set_chip_data() (sets
desc->irq_data.chip_data) has nothing to do with irq_get_irq_data()
(which returns desc->irq_data).  It's the   allocation of the irq_desc
that is at issue here.  Where does the allocation occur?  (I haven't
dug in to find it yet)

g.
Grant Likely Jan. 30, 2012, 7:58 p.m. UTC | #6
On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
> 
> >This patch removes the x86-specific definition of irq_domain and replaces
> >it with the common implementation.
> 
> I pulled your devicetree/next tree. After this patch I get:
> 
> |Hierarchical RCU implementation.
> |NR_IRQS:2304 nr_irqs:256 16
> |------------[ cut here ]------------
> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
> |Modules linked in:
> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
> |Call Trace:
> | [<c15044e0>] ? printk+0x18/0x1a
> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
> | [<c1708df2>] init_IRQ+0x8/0x33
> | [<c170557f>] start_kernel+0x191/0x2e1
> | [<c170517f>] ? loglevel+0x2b/0x2b
> | [<c1705081>] i386_start_kernel+0x81/0x86
> |---[ end trace 4eaa2a86a8e2da22 ]---
> |------------[ cut here ]------------
> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
> 
> The warning is comming from this piece in irq_domain_add_legacy()
> |for (i = 0; i < size; i++) {
> |                 int irq = first_irq + i;
> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
> | 
> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
> 
> irq_data is NULL here.
> 
> |                         mutex_unlock(&irq_domain_mutex);
> |                         of_node_put(domain->of_node);
> |                         kfree(domain);
> |                         return NULL;
> |                 }
> |         }
> | 
> 
> This is not always the case. arch_early_irq_init() in [0] sets up the
> first 16 entries. The reminaing few (there is a toal of 24 irqs for
> first ioapic and a second ioapic) are not initialized. This happens
> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
> 
> [0] arch/x86/kernel/apic/io_apic.c

Ugh.  This isn't easy.  The legacy mapping really needs all the
irq_desc structures to be allocated.  You could call irq_alloc_descs()
before calling irq_domain_add_legacy(), but that causes all the
irq_descs to be allocated (regardless of whether they are used), and
it will break io_apic_setup_irq_pin() which also wants to call
irq_alloc_desc().

Ideally irq_domain support would be rolled directly into ioapic.
That's more work though, and a greater change of breaking x86 on
non-embedded.  Looking at the ioapic code, it seems to me that it
could be simplified quite a bit by switching to irq_domain instead of
using the custom irq_cfg linked list.  Faster too since it could use
the irq_data->hwirq to go from linux irq to the hw irq number.  It
doesn't look like it would be even that hard, but of course the devil
is in the details and I don't have sufficient time right now to dig
into the guts of the ioapic.  Maybe after connect, but it would help
if you can find time to look at it.

If integrated into the ioapic code, then the irq_domain linear map is
probably the type of irq_domain to use.

g.
Olof Johansson Jan. 31, 2012, 4:53 a.m. UTC | #7
On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
> Hey everyone,
> 
> This patch series is ready for much wider consumption now.  I'd like
> to get it into linux-next ASAP because there will be ARM board support
> depending on it.  I'll wait a few days before I ask Stephen to pull
> this in.
> 
> Stephen/Milton/Ben, any testing you can help with here would be
> appreciated since you've got access to a wider variety of Power
> machines than I do.

This series has been:

Tested-by: Olof Johansson <olof@lixom.net>

On powerpc/pasemi (it's the only one I still have easy access to).


-Olof
Grant Likely Feb. 1, 2012, 12:07 a.m. UTC | #8
On Mon, Jan 30, 2012 at 08:53:44PM -0800, Olof Johansson wrote:
> On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
> > Hey everyone,
> > 
> > This patch series is ready for much wider consumption now.  I'd like
> > to get it into linux-next ASAP because there will be ARM board support
> > depending on it.  I'll wait a few days before I ask Stephen to pull
> > this in.
> > 
> > Stephen/Milton/Ben, any testing you can help with here would be
> > appreciated since you've got access to a wider variety of Power
> > machines than I do.
> 
> This series has been:
> 
> Tested-by: Olof Johansson <olof@lixom.net>
> 
> On powerpc/pasemi (it's the only one I still have easy access to).

Excellent, thanks Olof!

g.
Sebastian Andrzej Siewior Feb. 1, 2012, 2:17 p.m. UTC | #9
* Grant Likely | 2012-01-30 12:58:42 [-0700]:

>Ugh.  This isn't easy.  The legacy mapping really needs all the

Feel free to merge this patch. I don't have the time to look at this now
so I take a look at the ioapic later.

>g.

Sebastian
Grant Likely Feb. 1, 2012, 6:06 p.m. UTC | #10
On Wed, Feb 1, 2012 at 7:17 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> * Grant Likely | 2012-01-30 12:58:42 [-0700]:
>
>>Ugh.  This isn't easy.  The legacy mapping really needs all the
>
> Feel free to merge this patch. I don't have the time to look at this now
> so I take a look at the ioapic later.

There's no rush here.  I can leave it as-is with IRQ_DOMAIN turned off
for x86 for now.

g.
Grant Likely Feb. 3, 2012, 4:42 p.m. UTC | #11
On Fri, Feb 03, 2012 at 03:48:09PM +0100, Cousson, Benoit wrote:
> Hi Grant,
> 
> I finally had the time to rebase most of the OMAP3 and OMAP4 DT
> patches on your latest irq_domain series and found a couple of
> minors regressions that breaks OMAP3 boot.
> 
> On 1/27/2012 10:36 PM, Grant Likely wrote:
> 
> [...]
> 
> >diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> >index e04e04dd..aab236f 100644
> >--- a/drivers/mfd/twl-core.c
> >+++ b/drivers/mfd/twl-core.c
> >@@ -263,8 +263,6 @@ struct twl_client {
> >
> >  static struct twl_client twl_modules[TWL_NUM_SLAVES];
> >
> >-static struct irq_domain domain;
> >-
> >  /* mapping the module id to slave id and base address */
> >  struct twl_mapping {
> >  	unsigned char sid;	/* Slave ID */
> >@@ -1225,14 +1223,8 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >
> >  	pdata->irq_base = status;
> >  	pdata->irq_end = pdata->irq_base + nr_irqs;
> >-
> >-	domain.irq_base = pdata->irq_base;
> >-	domain.nr_irq = nr_irqs;
> >-#ifdef CONFIG_OF_IRQ
> >-	domain.of_node = of_node_get(node);
> >-	domain.ops =&irq_domain_simple_ops;
> >-#endif
> >-	irq_domain_add(&domain);
> >+	irq_domain_add_legacy(node, nr_irqs, pdata->irq_base, 0,
> >+			&irq_domain_simple_ops);
> 
> This commit cannot build due to the missing last parameter.
> 
> And in fact you fixed that in the next commit (#14), but the will
> break git bisect and anyway that fix does not really belong to this
> commit.
> 
> [PATCH v3 14/25] irq_domain: Remove irq_domain_add_simple()
> 
>  	irq_domain_add_legacy(node, nr_irqs, pdata->irq_base, 0,
> -			      &irq_domain_simple_ops);
> +			      &irq_domain_simple_ops, NULL);

Good catch, thanks.  I'll update the series to fix that.

> 
> 
> Moreover, it looks like this new irq_domain code is checking the
> number of hwirq and is not as lazy as the previous one :-)
> 
> Because of that and because of the wrong number of IRQs I put for
> the twl4030 :-(, it does not handle properly the children of the
> twl4030 now and print a big warning at boot time due to the
> following check.
> 
>   WARN_ON(hwirq < first_hwirq || hwirq >= first_hwirq + size)

Good! The warning is doing it's job.  :-)

> In fact 8 was just the number for the core functionality, but that
> chip does have some other interrupts for sub function like GPIOs and
> power events.

Okay, I'll add this patch to the series before patch 13.

> 
> With the following fix, it works fine.
> 
> Regards,
> Benoit
> 
> 
> ---
> From 12781619d2ab8d6d724acabc6873954f0f9f4347 Mon Sep 17 00:00:00 2001
> From: Benoit Cousson <b-cousson@ti.com>
> Date: Fri, 3 Feb 2012 14:58:17 +0100
> Subject: [PATCH] mfd: twl-core.c: Fix the number of interrupts
> managed by twl4030
> 
> TWL4030 does handle 3 different interrupts ranges: 8 for the core, 8
> for the power events and 18 for the GPIOs.
> 
> Change the total number of interrupts managed by TWL4030 from 8 to 34.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> ---
>  drivers/mfd/twl-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index e63b408..66f9bff 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -149,7 +149,7 @@
> 
>  #define TWL_MODULE_LAST TWL4030_MODULE_LAST
> 
> -#define TWL4030_NR_IRQS    8
> +#define TWL4030_NR_IRQS    34 /* core:8, power:8, gpio: 18 */
>  #define TWL6030_NR_IRQS    20
> 
>  /* Base Address defns for twl4030_map[] */
> -- 
> 1.7.0.4
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Russell King - ARM Linux Feb. 4, 2012, 10:17 p.m. UTC | #12
On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
> Hey everyone,
> 
> This patch series is ready for much wider consumption now.  I'd like
> to get it into linux-next ASAP because there will be ARM board support
> depending on it.  I'll wait a few days before I ask Stephen to pull
> this in.

Grant,

Can you answer me this: does this irqdomain support require DT?

The question comes up because OMAP has converted some of their support
to require irq domain support for their PMICs, and it seems irq domain
support requires DT.  This seems to have made the whole of OMAP
essentially become a DT-only platform.

Removing the dependency on IRQ_DOMAIN brings up these build errors
in the twl-core code (that being the PMIC for OMAP CPUs):

drivers/mfd/twl-core.c: In function 'twl_probe':
drivers/mfd/twl-core.c:1229: error: invalid use of undefined type 'struct irq_domain'
drivers/mfd/twl-core.c:1230: error: invalid use of undefined type 'struct irq_domain'
drivers/mfd/twl-core.c:1235: error: implicit declaration of function 'irq_domain_add'

That's a bit of a problem, because afaik there aren't the DT descriptions
for the boards I have yet, so it's causing me to see regressions when
building and booting kernels with CONFIG_OF=n.

The more core-code we end up with which requires DT, the worse this
problem is going to become - and obviously saying "everyone must now
convert to DT" is, even today, a mammoth task.

Now, here's the thing: I believe that IRQ domains - at least as far as
the hwirq stuff - should be available irrespective of whether we have
the rest of the IRQ domain support code in place, so that IRQ support
code doesn't have to keep playing games to decode from the global
space to the per-controller number space.

I believe that would certainly help the current OMAP problems, where
the current lack of CONFIG_IRQ_DOMAIN basically makes the kernel oops
on boot.

How we fix this regression for 3.4 I've no idea at present, I'm trying
to work out what the real dependencies are for OMAP on this stuff.

Finally, do we need asm/irq.h in our asm/prom.h ?  That's causing
fragility between DT and non-DT builds, because people are finding
that their DT builds work without their mach/irqs.h includes but
fail when built with non-DT.  The only thing which DT might need -
at the most - is NR_IRQS, but I'd hope with things like irq domains
it doesn't actually require it.
Benjamin Herrenschmidt Feb. 5, 2012, 12:01 a.m. UTC | #13
On Sat, 2012-02-04 at 22:17 +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
> > Hey everyone,
> > 
> > This patch series is ready for much wider consumption now.  I'd like
> > to get it into linux-next ASAP because there will be ARM board support
> > depending on it.  I'll wait a few days before I ask Stephen to pull
> > this in.
> 
> Grant,
> 
> Can you answer me this: does this irqdomain support require DT?

The original powerpc code this is based on didn't require it. It was an
explicit design decision and I remember insisting that Grant follows it,
but I haven't yet reviewed his last batch.

DT is orthogonal. You have "helpers" that use the DT to resolve the
domain of an interrupt source and do the mapping for you, but I made
sure that you call always still create domains and map interrupts using
explicit domain pointers & hw numbers.

(And I need them to deal with ancient broken device-tree's on some
platforms such as oldworld PowerMacs).

> The question comes up because OMAP has converted some of their support
> to require irq domain support for their PMICs, and it seems irq domain
> support requires DT.  This seems to have made the whole of OMAP
> essentially become a DT-only platform.

 .../...

> Now, here's the thing: I believe that IRQ domains - at least as far as
> the hwirq stuff - should be available irrespective of whether we have
> the rest of the IRQ domain support code in place, so that IRQ support
> code doesn't have to keep playing games to decode from the global
> space to the per-controller number space.
> 
> I believe that would certainly help the current OMAP problems, where
> the current lack of CONFIG_IRQ_DOMAIN basically makes the kernel oops
> on boot.
> 
> How we fix this regression for 3.4 I've no idea at present, I'm trying
> to work out what the real dependencies are for OMAP on this stuff.
> 
> Finally, do we need asm/irq.h in our asm/prom.h ?  That's causing
> fragility between DT and non-DT builds, because people are finding
> that their DT builds work without their mach/irqs.h includes but
> fail when built with non-DT.  The only thing which DT might need -
> at the most - is NR_IRQS, but I'd hope with things like irq domains
> it doesn't actually require it.

Cheers,
Ben.
Tony Lindgren Feb. 5, 2012, 1:38 a.m. UTC | #14
* Russell King - ARM Linux <linux@arm.linux.org.uk> [120204 14:00]:
> 
> Actually, it turns out to be not that hard, because twl doesn't actually
> make use of the IRQ domain stuff:
> 
> commit aeb5032b3f8b9ab69daa545777433fa94b3494c4
> Author:     Benoit Cousson <b-cousson@ti.com>
> AuthorDate: Mon Aug 29 16:20:23 2011 +0200
> Commit:     Samuel Ortiz <sameo@linux.intel.com>
> CommitDate: Mon Jan 9 00:37:40 2012 +0100
> 
>     mfd: twl-core: Add initial DT support for twl4030/twl6030
> 
>     [grant.likely@secretlab.ca: Fix IRQ_DOMAIN dependency in kconfig]
> 
> Adding any dependency - especially one which wouldn't be enabled - for
> a new feature which wasn't required before is going to break existing
> users, so this shouldn't have been done in the first place.
> 
> A better fix to preserve existing users would've been as below - yes
> it means more ifdefs, but if irq domain is to remain a DT only thing
> then we're going to end up with _lots_ of this stuff.
> 
> I'd much prefer to see irq domain become more widely available so it
> doesn't require these ifdefs everywhere.

Your patch below looks like a correct fix to me to the problem
you and Grazvydas are seeing:

Acked-by: Tony Lindgren <tony@atomide.com>
 
>  drivers/mfd/Kconfig    |    2 +-
>  drivers/mfd/twl-core.c |    4 ++++
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 28a301b..bd60ce0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -200,7 +200,7 @@ config MENELAUS
>  
>  config TWL4030_CORE
>  	bool "Texas Instruments TWL4030/TWL5030/TWL6030/TPS659x0 Support"
> -	depends on I2C=y && GENERIC_HARDIRQS && IRQ_DOMAIN
> +	depends on I2C=y && GENERIC_HARDIRQS
>  	help
>  	  Say yes here if you have TWL4030 / TWL6030 family chip on your board.
>  	  This core driver provides register access and IRQ handling
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index e04e04d..5913aaa 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -263,7 +263,9 @@ struct twl_client {
>  
>  static struct twl_client twl_modules[TWL_NUM_SLAVES];
>  
> +#ifdef CONFIG_IRQ_DOMAIN
>  static struct irq_domain domain;
> +#endif
>  
>  /* mapping the module id to slave id and base address */
>  struct twl_mapping {
> @@ -1226,6 +1228,7 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	pdata->irq_base = status;
>  	pdata->irq_end = pdata->irq_base + nr_irqs;
>  
> +#ifdef CONFIG_IRQ_DOMAIN
>  	domain.irq_base = pdata->irq_base;
>  	domain.nr_irq = nr_irqs;
>  #ifdef CONFIG_OF_IRQ
> @@ -1233,6 +1236,7 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	domain.ops = &irq_domain_simple_ops;
>  #endif
>  	irq_domain_add(&domain);
> +#endif
>  
>  	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) == 0) {
>  		dev_dbg(&client->dev, "can't talk I2C?\n");
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Russell King - ARM Linux Feb. 5, 2012, 4:13 p.m. UTC | #15
On Sat, Feb 04, 2012 at 05:38:53PM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [120204 14:00]:
> > 
> > Actually, it turns out to be not that hard, because twl doesn't actually
> > make use of the IRQ domain stuff:
> > 
> > commit aeb5032b3f8b9ab69daa545777433fa94b3494c4
> > Author:     Benoit Cousson <b-cousson@ti.com>
> > AuthorDate: Mon Aug 29 16:20:23 2011 +0200
> > Commit:     Samuel Ortiz <sameo@linux.intel.com>
> > CommitDate: Mon Jan 9 00:37:40 2012 +0100
> > 
> >     mfd: twl-core: Add initial DT support for twl4030/twl6030
> > 
> >     [grant.likely@secretlab.ca: Fix IRQ_DOMAIN dependency in kconfig]
> > 
> > Adding any dependency - especially one which wouldn't be enabled - for
> > a new feature which wasn't required before is going to break existing
> > users, so this shouldn't have been done in the first place.
> > 
> > A better fix to preserve existing users would've been as below - yes
> > it means more ifdefs, but if irq domain is to remain a DT only thing
> > then we're going to end up with _lots_ of this stuff.
> > 
> > I'd much prefer to see irq domain become more widely available so it
> > doesn't require these ifdefs everywhere.
> 
> Your patch below looks like a correct fix to me to the problem
> you and Grazvydas are seeing:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

It's not quite correct, because OMAP4 has issues in this area as well
(which does select IRQ_DOMAIN but can be without OF.)  The result is
an oops from irq_domain_add() because domain->ops is NULL.

The right solution is three fold:

1. Wrap the bits of code in CONFIG_IRQ_DOMAIN
2. Get rid of the #ifdef CONFIG_OF there, so the 'ops' member can be
   initialized.
3. Fix the OMAP vp code not to oops when voltdm->pmic is NULL

which I have in my combined patch for fixing OMAP so far.
Rob Herring Feb. 6, 2012, 12:51 a.m. UTC | #16
Russell,

On 02/04/2012 04:17 PM, Russell King - ARM Linux wrote:
> On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
>> Hey everyone,
>>
>> This patch series is ready for much wider consumption now.  I'd like
>> to get it into linux-next ASAP because there will be ARM board support
>> depending on it.  I'll wait a few days before I ask Stephen to pull
>> this in.
> 
> Grant,
> 
> Can you answer me this: does this irqdomain support require DT?
> 

No. It's the other way around, DT requires irqdomain. The GIC and VIC
code for example can be built with or w/o DT enabled, but both select
IRQ_DOMAIN.

FYI, I just submitted a patch that selects IRQ_DOMAIN for all of ARM:

http://www.gossamer-threads.com/lists/linux/kernel/1487231?page=last

Either we do that or we select IRQ_DOMAIN one irq_chip at a time. With
the "new" irq_domain code, we could probably do better to shrink the
code needed in the non-DT case.

> The question comes up because OMAP has converted some of their support
> to require irq domain support for their PMICs, and it seems irq domain
> support requires DT.  This seems to have made the whole of OMAP
> essentially become a DT-only platform.

I think we should select DT or not at the sub-arch level. Trying to
support both builds is a needless headache.

> 
> Removing the dependency on IRQ_DOMAIN brings up these build errors
> in the twl-core code (that being the PMIC for OMAP CPUs):
> 
> drivers/mfd/twl-core.c: In function 'twl_probe':
> drivers/mfd/twl-core.c:1229: error: invalid use of undefined type 'struct irq_domain'
> drivers/mfd/twl-core.c:1230: error: invalid use of undefined type 'struct irq_domain'
> drivers/mfd/twl-core.c:1235: error: implicit declaration of function 'irq_domain_add'
> 
> That's a bit of a problem, because afaik there aren't the DT descriptions
> for the boards I have yet, so it's causing me to see regressions when
> building and booting kernels with CONFIG_OF=n.
> 
> The more core-code we end up with which requires DT, the worse this
> problem is going to become - and obviously saying "everyone must now
> convert to DT" is, even today, a mammoth task.
> 
> Now, here's the thing: I believe that IRQ domains - at least as far as
> the hwirq stuff - should be available irrespective of whether we have
> the rest of the IRQ domain support code in place, so that IRQ support
> code doesn't have to keep playing games to decode from the global
> space to the per-controller number space.
> 
> I believe that would certainly help the current OMAP problems, where
> the current lack of CONFIG_IRQ_DOMAIN basically makes the kernel oops
> on boot.
> 
> How we fix this regression for 3.4 I've no idea at present, I'm trying
> to work out what the real dependencies are for OMAP on this stuff.
> 
> Finally, do we need asm/irq.h in our asm/prom.h ?  That's causing
> fragility between DT and non-DT builds, because people are finding
> that their DT builds work without their mach/irqs.h includes but
> fail when built with non-DT.  The only thing which DT might need -
> at the most - is NR_IRQS, but I'd hope with things like irq domains
> it doesn't actually require it.

Doesn't look like it is needed.

Rob

> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Grant Likely Feb. 6, 2012, 5:56 a.m. UTC | #17
On Sat, Feb 04, 2012 at 10:17:48PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote:
> > Hey everyone,
> > 
> > This patch series is ready for much wider consumption now.  I'd like
> > to get it into linux-next ASAP because there will be ARM board support
> > depending on it.  I'll wait a few days before I ask Stephen to pull
> > this in.
> 
> Grant,
> 
> Can you answer me this: does this irqdomain support require DT?

No, it should not.  Any situation where it does is a bug.

> Now, here's the thing: I believe that IRQ domains - at least as far as
> the hwirq stuff - should be available irrespective of whether we have
> the rest of the IRQ domain support code in place, so that IRQ support
> code doesn't have to keep playing games to decode from the global
> space to the per-controller number space.

Correct.  That's the intent.  My new series flushes out irq_domain quite a
bit better and gets all architectures doing the same thing if they use
irq_domains.  I've done some testing on both CONFIG_OF and !CONFIG_OF builds,
but I'm going to do some more to make sure I've not missed anything.

> I believe that would certainly help the current OMAP problems, where
> the current lack of CONFIG_IRQ_DOMAIN basically makes the kernel oops
> on boot.
> 
> How we fix this regression for 3.4 I've no idea at present, I'm trying
> to work out what the real dependencies are for OMAP on this stuff.
> 
> Finally, do we need asm/irq.h in our asm/prom.h ?  That's causing
> fragility between DT and non-DT builds, because people are finding
> that their DT builds work without their mach/irqs.h includes but
> fail when built with non-DT.  The only thing which DT might need -
> at the most - is NR_IRQS, but I'd hope with things like irq domains
> it doesn't actually require it.

I don't think so.  There may be a file or two that break because they're
not including everything they need, but I don't think anything in the
header requires it.

The irq_domain code is well isolated.  The header file doesn't need to
be including it.

g.
Grant Likely Feb. 6, 2012, 6 a.m. UTC | #18
On Mon, Feb 06, 2012 at 04:22:41PM +1100, Michael Neuling wrote:
> >  static const struct irq_domain_ops pmac_pic_host_ops = {
> >  	.match = pmac_pic_host_match,
> >  	.map = pmac_pic_host_map,
> > -	.xlate = pmac_pic_host_xlate,
> > +	.xlate = irq_domain_xlate_onecell;
> >  };
> >  
> 
> This causes a compile fail (in next-20120206):
>   arch/powerpc/platforms/powermac/pic.c:294: error: expected '}' before ';' token
> Trivial fix below:

fixed, thanks.

g.

> 
> 
> 
> irqdomain/powerpc: Fix compile error in powermac/pic.c
> 
> Error introduced in:
>   commit 72d6b477675374eafd206720d6c9146520df18ce
>   Author: Grant Likely <grant.likely@secretlab.ca>
>   irqdomain/powerpc: Replace custom xlate functions with library functions
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> 
> diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
> index 3af29d1..92afc38 100644
> --- a/arch/powerpc/platforms/powermac/pic.c
> +++ b/arch/powerpc/platforms/powermac/pic.c
> @@ -291,7 +291,7 @@ static int pmac_pic_host_map(struct irq_domain *h, unsigned int virq,
>  static const struct irq_domain_ops pmac_pic_host_ops = {
>  	.match = pmac_pic_host_match,
>  	.map = pmac_pic_host_map,
> -	.xlate = irq_domain_xlate_onecell;
> +	.xlate = irq_domain_xlate_onecell,
>  };
>  
>  static void __init pmac_pic_probe_oldstyle(void)
Mark Brown Feb. 7, 2012, 3:26 p.m. UTC | #19
On Sun, Feb 05, 2012 at 04:13:48PM +0000, Russell King - ARM Linux wrote:

> It's not quite correct, because OMAP4 has issues in this area as well
> (which does select IRQ_DOMAIN but can be without OF.)  The result is
> an oops from irq_domain_add() because domain->ops is NULL.

> The right solution is three fold:

> 1. Wrap the bits of code in CONFIG_IRQ_DOMAIN
> 2. Get rid of the #ifdef CONFIG_OF there, so the 'ops' member can be
>    initialized.
> 3. Fix the OMAP vp code not to oops when voltdm->pmic is NULL

> which I have in my combined patch for fixing OMAP so far.

It'd also help if we supported null ops, I sent patches for that a few
times over the 3.3 cycle since I was running into it on my systems but
apparently to /dev/null and further changes in this area have made the
patches not apply any more.
Nicolas Ferre Feb. 7, 2012, 6:07 p.m. UTC | #20
On 01/27/2012 10:36 PM, Grant Likely :
> The 'hint' used to try and line up irq numbers with hw irq numbers is
> rather a hack and not very useful.  Now that /proc/interrupts also outputs
> the hwirq number, it is even less useful to keep around the 'hint' heuristic.
> 
> This patch removes it.

Grant,

While trying your patch series in conjunction with Rob one, I do not
find this patch in your irqdomain/next branch (and a couple of others).
Can you tell me if this v3 series is available as a git tree?

Thanks, best regards,
Nicolas Ferre Feb. 15, 2012, 3:04 p.m. UTC | #21
On 02/07/2012 07:07 PM, Nicolas Ferre :
> On 01/27/2012 10:36 PM, Grant Likely :
>> The 'hint' used to try and line up irq numbers with hw irq numbers is
>> rather a hack and not very useful.  Now that /proc/interrupts also outputs
>> the hwirq number, it is even less useful to keep around the 'hint' heuristic.
>>
>> This patch removes it.
> 
> Grant,
> 
> While trying your patch series in conjunction with Rob one, I do not
> find this patch in your irqdomain/next branch (and a couple of others).
> Can you tell me if this v3 series is available as a git tree?

I am still interested by patch 24-25 of this series but still cannot
find them in your irqdomain/next branch:
Are they also expected to join the 3.4 merge window material?

Bye,
Nicolas Ferre Feb. 15, 2012, 4:36 p.m. UTC | #22
Grant,

I do not know if it is the latest revision but I have identified some
issues on error/slow paths...


On 01/27/2012 10:36 PM, Grant Likely :
> With the current state of irq_domain, the reverse map is always used when
> new IRQs get mapped.  This means that the irq_find_mapping() function
> can be simplified to always call out to the revmap-specific lookup function.
> 
> This patch adds lookup functions for the revmaps that don't yet have one
> and removes the slow path lookup from most of the code paths.  The only
> place where the slow path legitimately remains is when the linear map
> is used with a hwirq number larger than the revmap size.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Milton Miller <miltonm@bga.com>
> ---
>  arch/powerpc/sysdev/xics/xics-common.c |    3 -
>  include/linux/irqdomain.h              |    3 +-
>  kernel/irq/irqdomain.c                 |   94 +++++++++++++++++---------------
>  3 files changed, 51 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
> index ea5e204..1d7067d 100644
> --- a/arch/powerpc/sysdev/xics/xics-common.c
> +++ b/arch/powerpc/sysdev/xics/xics-common.c
> @@ -330,9 +330,6 @@ static int xics_host_map(struct irq_domain *h, unsigned int virq,
>  
>  	pr_devel("xics: map virq %d, hwirq 0x%lx\n", virq, hw);
>  
> -	/* Insert the interrupt mapping into the radix tree for fast lookup */
> -	irq_radix_revmap_insert(xics_host, virq, hw);
> -
>  	/* They aren't all level sensitive but we just don't really know */
>  	irq_set_status_flags(virq, IRQ_LEVEL);
>  
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 0b00f83..38314f2 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -93,6 +93,7 @@ struct irq_domain {
>  	struct list_head link;
>  
>  	/* type of reverse mapping_technique */
> +	unsigned int (*revmap)(struct irq_domain *host, irq_hw_number_t hwirq);
>  	unsigned int revmap_type;
>  	union {
>  		struct {
> @@ -155,8 +156,6 @@ extern void irq_dispose_mapping(unsigned int virq);
>  extern unsigned int irq_find_mapping(struct irq_domain *host,
>  				     irq_hw_number_t hwirq);
>  extern unsigned int irq_create_direct_mapping(struct irq_domain *host);
> -extern void irq_radix_revmap_insert(struct irq_domain *host, unsigned int virq,
> -				    irq_hw_number_t hwirq);
>  extern unsigned int irq_radix_revmap_lookup(struct irq_domain *host,
>  					    irq_hw_number_t hwirq);
>  extern unsigned int irq_linear_revmap(struct irq_domain *host,
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 5b4fc4d..91c1cb7 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -104,6 +104,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
>  	domain->revmap_data.legacy.first_irq = first_irq;
>  	domain->revmap_data.legacy.first_hwirq = first_hwirq;
>  	domain->revmap_data.legacy.size = size;
> +	domain->revmap = irq_domain_legacy_revmap;
>  
>  	mutex_lock(&irq_domain_mutex);
>  	/* Verify that all the irqs are available */
> @@ -174,18 +175,35 @@ struct irq_domain *irq_domain_add_linear(struct device_node *of_node,
>  	}
>  	domain->revmap_data.linear.size = size;
>  	domain->revmap_data.linear.revmap = revmap;
> +	domain->revmap = irq_linear_revmap;
>  	irq_domain_add(domain);
>  	return domain;
>  }
>  
> +static unsigned int irq_domain_nomap_revmap(struct irq_domain *domain,
> +					    irq_hw_number_t hwirq)
> +{
> +	struct irq_data *data = irq_get_irq_data(hwirq);
> +
> +	if (WARN_ON_ONCE(domain->revmap_type != IRQ_DOMAIN_MAP_NOMAP))
> +		return irq_find_mapping(domain, hwirq);

Should be:
		return irq_find_mapping_slow(domain, hwirq);

Recursion otherwise...


> +
> +	/* Verify that the map has actually been established */
> +	if (data && (data->domain == domain) && (data->hwirq == hwirq))
> +		return hwirq;
> +	return 0;
> +}
> +
>  struct irq_domain *irq_domain_add_nomap(struct device_node *of_node,
>  					 const struct irq_domain_ops *ops,
>  					 void *host_data)
>  {
>  	struct irq_domain *domain = irq_domain_alloc(of_node,
>  					IRQ_DOMAIN_MAP_NOMAP, ops, host_data);
> -	if (domain)
> +	if (domain) {
> +		domain->revmap = irq_domain_nomap_revmap;
>  		irq_domain_add(domain);
> +	}
>  	return domain;
>  }
>  
> @@ -205,6 +223,7 @@ struct irq_domain *irq_domain_add_tree(struct device_node *of_node,
>  					IRQ_DOMAIN_MAP_TREE, ops, host_data);
>  	if (domain) {
>  		INIT_RADIX_TREE(&domain->revmap_data.tree, GFP_KERNEL);
> +		domain->revmap = irq_radix_revmap_lookup;
>  		irq_domain_add(domain);
>  	}
>  	return domain;
> @@ -378,6 +397,19 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  		return 0;
>  	}
>  
> +	switch(domain->revmap_type) {
> +	case IRQ_DOMAIN_MAP_LINEAR:
> +		if (hwirq < domain->revmap_data.linear.size)
> +			domain->revmap_data.linear.revmap[hwirq] = irq;
> +		break;
> +	case IRQ_DOMAIN_MAP_TREE:
> +		mutex_lock(&revmap_trees_mutex);
> +		radix_tree_insert(&domain->revmap_data.tree, hwirq,
> +				  irq_get_irq_data(irq));
> +		mutex_unlock(&revmap_trees_mutex);
> +
> +		break;
> +	}
>  	pr_debug("irq: irq %lu on domain %s mapped to virtual irq %u\n",
>  		hwirq, domain->of_node ? domain->of_node->full_name : "null", virq);
>  
> @@ -478,25 +510,27 @@ EXPORT_SYMBOL_GPL(irq_dispose_mapping);
>   * irq_find_mapping() - Find a linux irq from an hw irq number.
>   * @domain: domain owning this hardware interrupt
>   * @hwirq: hardware irq number in that domain space
> - *
> - * This is a slow path, for use by generic code. It's expected that an
> - * irq controller implementation directly calls the appropriate low level
> - * mapping function.
>   */
>  unsigned int irq_find_mapping(struct irq_domain *domain,
>  			      irq_hw_number_t hwirq)
>  {
> -	unsigned int i;
> -
> -	/* Look for default domain if nececssary */
> -	if (domain == NULL)
> +	if (!domain)
>  		domain = irq_default_domain;
> -	if (domain == NULL)
> -		return 0;
> +	return domain ? domain->revmap(domain, hwirq) : 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_find_mapping);
>  
> -	/* legacy -> bail early */
> -	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
> -		return irq_domain_legacy_revmap(domain, hwirq);
> +/**
> + * irq_find_mapping_slow() - slow path for finding the irq mapped to a hwirq
> + *
> + * This is the failsafe slow path for finding an irq mapping.  The only time
> + * this will reasonably get called is when the linear map is used with a
> + * hwirq number larger than the size of the reverse map.
> + */
> +static unsigned int irq_find_mapping_slow(struct irq_domain *domain,
> +					  irq_hw_number_t hwirq)
> +{
> +	int i;
>  
>  	/* Slow path does a linear search of the map */
>  	for (i = 0; i < irq_virq_count; i++)  {
> @@ -506,7 +540,6 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>  	}
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(irq_find_mapping);
>  
>  /**
>   * irq_radix_revmap_lookup() - Find a linux irq from a hw irq number.
> @@ -537,31 +570,7 @@ unsigned int irq_radix_revmap_lookup(struct irq_domain *domain,
>  	 * Else fallback to linear lookup - this should not happen in practice
>  	 * as it means that we failed to insert the node in the radix tree.
>  	 */
> -	return irq_data ? irq_data->irq : irq_find_mapping(domain, hwirq);
> -}
> -
> -/**
> - * irq_radix_revmap_insert() - Insert a hw irq to linux irq number mapping.
> - * @domain: domain owning this hardware interrupt
> - * @virq: linux irq number
> - * @hwirq: hardware irq number in that domain space
> - *
> - * This is for use by irq controllers that use a radix tree reverse
> - * mapping for fast lookup.
> - */
> -void irq_radix_revmap_insert(struct irq_domain *domain, unsigned int virq,
> -			     irq_hw_number_t hwirq)
> -{
> -	struct irq_data *irq_data = irq_get_irq_data(virq);
> -
> -	if (WARN_ON(domain->revmap_type != IRQ_DOMAIN_MAP_TREE))
> -		return;
> -
> -	if (virq) {
> -		mutex_lock(&revmap_trees_mutex);
> -		radix_tree_insert(&domain->revmap_data.tree, hwirq, irq_data);
> -		mutex_unlock(&revmap_trees_mutex);
> -	}
> +	return irq_data ? irq_data->irq : irq_find_mapping_slow(domain, hwirq);
>  }
>  
>  /**
> @@ -585,14 +594,11 @@ unsigned int irq_linear_revmap(struct irq_domain *domain,
>  	if (unlikely(hwirq >= domain->revmap_data.linear.size))
>  		return irq_find_mapping(domain, hwirq);

Ditto here. And same whith previous one in same function. Check in
irq_radix_revmap_lookup() there is the same issue on the "WARN_ON_ONCE"
path...

>  
> -	/* Check if revmap was allocated */
>  	revmap = domain->revmap_data.linear.revmap;
> -	if (unlikely(revmap == NULL))
> -		return irq_find_mapping(domain, hwirq);
>  
>  	/* Fill up revmap with slow path if no mapping found */
>  	if (unlikely(!revmap[hwirq]))
> -		revmap[hwirq] = irq_find_mapping(domain, hwirq);
> +		revmap[hwirq] = irq_find_mapping_slow(domain, hwirq);
>  
>  	return revmap[hwirq];
>  }

Bye,
Grant Likely Feb. 15, 2012, 8:21 p.m. UTC | #23
On Wed, Feb 15, 2012 at 04:04:28PM +0100, Nicolas Ferre wrote:
> On 02/07/2012 07:07 PM, Nicolas Ferre :
> > On 01/27/2012 10:36 PM, Grant Likely :
> >> The 'hint' used to try and line up irq numbers with hw irq numbers is
> >> rather a hack and not very useful.  Now that /proc/interrupts also outputs
> >> the hwirq number, it is even less useful to keep around the 'hint' heuristic.
> >>
> >> This patch removes it.
> > 
> > Grant,
> > 
> > While trying your patch series in conjunction with Rob one, I do not
> > find this patch in your irqdomain/next branch (and a couple of others).
> > Can you tell me if this v3 series is available as a git tree?
> 
> I am still interested by patch 24-25 of this series but still cannot
> find them in your irqdomain/next branch:
> Are they also expected to join the 3.4 merge window material?

I've held off on putting them in irqdomain/next because they are a bit more
risky than the other patches, and I want an explicit ack from Ben for patches
24 & 25.  However, that shouldn't really cause any issues since the changes
in 24 & 25 don't impact the irq_domain functionality or API.  They are just
optimizations.

Also on 25, I'm not yet convinced that breaking out the revmap functions
into ops is the right thing to do.  It actually makes the .text size quite
a bit larger.  I may very well rewrite it.

g.
Grant Likely Feb. 15, 2012, 8:29 p.m. UTC | #24
On Wed, Feb 15, 2012 at 05:36:46PM +0100, Nicolas Ferre wrote:
> Grant,
> 
> I do not know if it is the latest revision but I have identified some
> issues on error/slow paths...
> 
> 
> On 01/27/2012 10:36 PM, Grant Likely :
> > With the current state of irq_domain, the reverse map is always used when
> > new IRQs get mapped.  This means that the irq_find_mapping() function
> > can be simplified to always call out to the revmap-specific lookup function.
> > 
> > This patch adds lookup functions for the revmaps that don't yet have one
> > and removes the slow path lookup from most of the code paths.  The only
> > place where the slow path legitimately remains is when the linear map
> > is used with a hwirq number larger than the revmap size.
> > 
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Milton Miller <miltonm@bga.com>
> > ---
> >  arch/powerpc/sysdev/xics/xics-common.c |    3 -
> >  include/linux/irqdomain.h              |    3 +-
> >  kernel/irq/irqdomain.c                 |   94 +++++++++++++++++---------------
> >  3 files changed, 51 insertions(+), 49 deletions(-)
> > 
> > diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
> > index ea5e204..1d7067d 100644
> > --- a/arch/powerpc/sysdev/xics/xics-common.c
> > +++ b/arch/powerpc/sysdev/xics/xics-common.c
> > @@ -330,9 +330,6 @@ static int xics_host_map(struct irq_domain *h, unsigned int virq,
> >  
> >  	pr_devel("xics: map virq %d, hwirq 0x%lx\n", virq, hw);
> >  
> > -	/* Insert the interrupt mapping into the radix tree for fast lookup */
> > -	irq_radix_revmap_insert(xics_host, virq, hw);
> > -
> >  	/* They aren't all level sensitive but we just don't really know */
> >  	irq_set_status_flags(virq, IRQ_LEVEL);
> >  
> > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> > index 0b00f83..38314f2 100644
> > --- a/include/linux/irqdomain.h
> > +++ b/include/linux/irqdomain.h
> > @@ -93,6 +93,7 @@ struct irq_domain {
> >  	struct list_head link;
> >  
> >  	/* type of reverse mapping_technique */
> > +	unsigned int (*revmap)(struct irq_domain *host, irq_hw_number_t hwirq);
> >  	unsigned int revmap_type;
> >  	union {
> >  		struct {
> > @@ -155,8 +156,6 @@ extern void irq_dispose_mapping(unsigned int virq);
> >  extern unsigned int irq_find_mapping(struct irq_domain *host,
> >  				     irq_hw_number_t hwirq);
> >  extern unsigned int irq_create_direct_mapping(struct irq_domain *host);
> > -extern void irq_radix_revmap_insert(struct irq_domain *host, unsigned int virq,
> > -				    irq_hw_number_t hwirq);
> >  extern unsigned int irq_radix_revmap_lookup(struct irq_domain *host,
> >  					    irq_hw_number_t hwirq);
> >  extern unsigned int irq_linear_revmap(struct irq_domain *host,
> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > index 5b4fc4d..91c1cb7 100644
> > --- a/kernel/irq/irqdomain.c
> > +++ b/kernel/irq/irqdomain.c
> > @@ -104,6 +104,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
> >  	domain->revmap_data.legacy.first_irq = first_irq;
> >  	domain->revmap_data.legacy.first_hwirq = first_hwirq;
> >  	domain->revmap_data.legacy.size = size;
> > +	domain->revmap = irq_domain_legacy_revmap;
> >  
> >  	mutex_lock(&irq_domain_mutex);
> >  	/* Verify that all the irqs are available */
> > @@ -174,18 +175,35 @@ struct irq_domain *irq_domain_add_linear(struct device_node *of_node,
> >  	}
> >  	domain->revmap_data.linear.size = size;
> >  	domain->revmap_data.linear.revmap = revmap;
> > +	domain->revmap = irq_linear_revmap;
> >  	irq_domain_add(domain);
> >  	return domain;
> >  }
> >  
> > +static unsigned int irq_domain_nomap_revmap(struct irq_domain *domain,
> > +					    irq_hw_number_t hwirq)
> > +{
> > +	struct irq_data *data = irq_get_irq_data(hwirq);
> > +
> > +	if (WARN_ON_ONCE(domain->revmap_type != IRQ_DOMAIN_MAP_NOMAP))
> > +		return irq_find_mapping(domain, hwirq);
> 
> Should be:
> 		return irq_find_mapping_slow(domain, hwirq);
> 
> Recursion otherwise...

Good catch; but it's not the whole story.  Part of the problem when looking
at the other revmap functions is that the revmaps can be called by external
code and there is the possibility that the wrong revmap got called.  In that
case, calling the normal path is the right thing to do.  However, it is also
possible (but unlikely) to have a ->revmap() and ->revmap_type mismatch where
calling the normal path would indeed cause a recursion.

So, the whole ->revmap thing is a weak design and needs to be rethought.

However, for the time being, I'll change all the revmaps to use the slow
path because it is safer.

g.
Grant Likely Feb. 15, 2012, 8:33 p.m. UTC | #25
On Tue, Feb 07, 2012 at 03:26:27PM +0000, Mark Brown wrote:
> On Sun, Feb 05, 2012 at 04:13:48PM +0000, Russell King - ARM Linux wrote:
> 
> > It's not quite correct, because OMAP4 has issues in this area as well
> > (which does select IRQ_DOMAIN but can be without OF.)  The result is
> > an oops from irq_domain_add() because domain->ops is NULL.
> 
> > The right solution is three fold:
> 
> > 1. Wrap the bits of code in CONFIG_IRQ_DOMAIN
> > 2. Get rid of the #ifdef CONFIG_OF there, so the 'ops' member can be
> >    initialized.
> > 3. Fix the OMAP vp code not to oops when voltdm->pmic is NULL
> 
> > which I have in my combined patch for fixing OMAP so far.
> 
> It'd also help if we supported null ops, I sent patches for that a few
> times over the 3.3 cycle since I was running into it on my systems but
> apparently to /dev/null and further changes in this area have made the
> patches not apply any more.

I've avoided allowing null ops so that the drivers are forced to be explicit
about what they want, and it makes for (IMHO) simpler to follow code in the
core.

Sorry I missed your patches though.

g.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Shawn Guo Feb. 15, 2012, 9:50 p.m. UTC | #26
On Wed, Feb 15, 2012 at 01:21:45PM -0700, Grant Likely wrote:
> On Wed, Feb 15, 2012 at 04:04:28PM +0100, Nicolas Ferre wrote:
> > On 02/07/2012 07:07 PM, Nicolas Ferre :
> > > On 01/27/2012 10:36 PM, Grant Likely :
> > >> The 'hint' used to try and line up irq numbers with hw irq numbers is
> > >> rather a hack and not very useful.  Now that /proc/interrupts also outputs
> > >> the hwirq number, it is even less useful to keep around the 'hint' heuristic.
> > >>
> > >> This patch removes it.
> > > 
> > > Grant,
> > > 
> > > While trying your patch series in conjunction with Rob one, I do not
> > > find this patch in your irqdomain/next branch (and a couple of others).
> > > Can you tell me if this v3 series is available as a git tree?
> > 
> > I am still interested by patch 24-25 of this series but still cannot
> > find them in your irqdomain/next branch:
> > Are they also expected to join the 3.4 merge window material?
> 
> I've held off on putting them in irqdomain/next because they are a bit more
> risky than the other patches, and I want an explicit ack from Ben for patches
> 24 & 25.  However, that shouldn't really cause any issues since the changes
> in 24 & 25 don't impact the irq_domain functionality or API.  They are just
> optimizations.
> 
I'm seeing that patch 24 does impact on irq_domain functionality
a little bit.  On next tree which has no this patch yet,
irq_create_mapping can reasonably create virq in range 1..15, while
irq_find_mapping will only try to find the virq from 16
(NUM_ISA_INTERRUPTS).  This will result in that any hwirq that is < 16
gets multiple entries in the mapping table with different virq numbers
mapped to the same one hwirq.

That's why I have to apply patch #24 (with one line change below) on
top of next tree to get my imx irqdomain series work properly.

@@ -371,7 +371,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
                return irq_domain_legacy_revmap(domain, hwirq);
 
        /* Allocate a virtual interrupt number */
-       virq = irq_alloc_desc(0);
+       virq = irq_alloc_desc_from(1, 0);
        if (!virq) {
                pr_debug("irq: -> virq allocation failed\n");
                return 0;

I need this line of change, because the first call on irq_alloc_desc
will always return 0 to virq and in turn irq_create_mapping fails.
On imx, that's the mapping for timer irq.  Hence, the system will hang
there due to irq mapping failure.
Shawn Guo Feb. 16, 2012, 6:03 a.m. UTC | #27
On Wed, Feb 15, 2012 at 10:32:43PM -0700, Grant Likely wrote:
...
> That's a bug then.  The implementation should work without patch 24.  Does
> this patch fix it?
> 
Yes, it fixes the problem for me.

Regards,
Shawn

> ---
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 2c1d6f8..2d3dfff 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -516,8 +516,8 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>  		return irq_domain_legacy_revmap(domain, hwirq);
>  
>  	/* Slow path does a linear search of the map */
> -	if (hint < NUM_ISA_INTERRUPTS)
> -		hint = NUM_ISA_INTERRUPTS;
> +	if (hint == 0)
> +		hint = 1;
>  	i = hint;
>  	do {
>  		struct irq_data *data = irq_get_irq_data(i);
> @@ -525,7 +525,7 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>  			return i;
>  		i++;
>  		if (i >= irq_virq_count)
> -			i = NUM_ISA_INTERRUPTS;
> +			i = 1
>  	} while(i != hint);
>  	return 0;
>  }
>
Sebastian Andrzej Siewior Feb. 23, 2012, 9:22 p.m. UTC | #28
On 02/23/2012 08:56 PM, Grant Likely wrote:
> On Wed, Feb 1, 2012 at 11:06 AM, Grant Likely<grant.likely@secretlab.ca>  wrote:
>> On Wed, Feb 1, 2012 at 7:17 AM, Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de>  wrote:
>>> * Grant Likely | 2012-01-30 12:58:42 [-0700]:
>>>
>>>> Ugh.  This isn't easy.  The legacy mapping really needs all the
>>>
>>> Feel free to merge this patch. I don't have the time to look at this now
>>> so I take a look at the ioapic later.
>>
>> There's no rush here.  I can leave it as-is with IRQ_DOMAIN turned off
>> for x86 for now.
>
> Turns out I have to enable IRQ_DOMAIN for x86 because the TI TWL4030
> driver needs it.  I do need to apply this patch.  Until something
> better can be implemented, can I change ioapic_add_ofnode() so that it
> allocates all irq_descs immediately.  It's not ideal, but every other
> approach I've looked at results in nasty hacks.
>
> Looking at the ioapic code, it appears to handle preallocated
> irq_descs gracefully.

Please merge your initial patch as-it.

> Does adding this loop help (apologies if it is whitespace damaged, I
> cut&paste it):
>
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 3ae2ced..89c1310 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -345,7 +345,7 @@ const struct irq_domain_ops ioapic_irq_domain_ops = {
>   static void __init ioapic_add_ofnode(struct device_node *np)
>   {
>   	struct resource r;
> -	int i, ret;
> +	int i, j, ret;
>
>   	ret = of_address_to_resource(np, 0,&r);
>   	if (ret) {
> @@ -361,6 +361,14 @@ static void __init ioapic_add_ofnode(struct
> device_node *np)
>
>   			gsi_cfg = mp_ioapic_gsi_routing(i);
>
> +			/*
> +			 * Preallocate irq_descs so that the legacy mapping
> +			 * works, but don't set them up.
> +			 * io_apic_setup_irq_pin_once() will finish the set up.
> +			 */
> +			for (j = 0; j<  32; j++)

It is not 32. If I remember correctly the first ioapic had 24 pins so
did the second. This is ioapic specifc.


>
> g.

Sebastian