Patchwork arm/tegra: add support for tegra30 interrupts

login
register
mail settings
Submitter Peter De Schrijver
Date Nov. 2, 2011, 3:38 p.m.
Message ID <1320248292-22736-1-git-send-email-pdeschrijver@nvidia.com>
Download mbox | patch
Permalink /patch/123285/
State New, archived
Headers show

Comments

Peter De Schrijver - Nov. 2, 2011, 3:38 p.m.
Tegra30 has 1 extra legacy interrupt controller. Use the GIC ITLinesNumber
field to determine how many interrupt controllers we have and initialize
appropriately. Also make room for the extra tegra30 interrupts by moving
the GPIO IRQ base. This shouldn't affect existing code as it determines the
correct IRQ number for GPIOs using TEGRA_GPIO_TO_IRQ()

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 arch/arm/mach-tegra/include/mach/iomap.h |    3 +++
 arch/arm/mach-tegra/include/mach/irqs.h  |   14 +++++++-------
 arch/arm/mach-tegra/irq.c                |   15 ++++++++++-----
 3 files changed, 20 insertions(+), 12 deletions(-)
Stephen Warren - Nov. 2, 2011, 4:39 p.m.
Peter De Schrijver wrote by Wednesday, November 02, 2011 9:38 AM:
> Tegra30 has 1 extra legacy interrupt controller. Use the GIC ITLinesNumber
> field to determine how many interrupt controllers we have and initialize
> appropriately. Also make room for the extra tegra30 interrupts by moving
> the GPIO IRQ base. This shouldn't affect existing code as it determines the
> correct IRQ number for GPIOs using TEGRA_GPIO_TO_IRQ()
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>

After fixing one small issue I comment on below,
Acked-by: Stephen Warren <swarren@nvidia.com>

I was previously concerned that renumbering the interrupt IDs would cause
some kind of backwards compatibility problems, but thinking more, I don't
think there's an issue; for a DT-based system, all the IRQ and GPIO numbers
are specified relative to the controller, and the relative numbering isn't
changing here. For a non-DT-based system, any IRQ numbers for any GPIO
should indeed be generated using TEGRA_GPIO_TO_IRQ (or the run-time
equivalent) and hence should also work OK after this patch.

> diff --git a/arch/arm/mach-tegra/include/mach/irqs.h b/arch/arm/mach-tegra/include/mach/irqs.h
...
>  #define NR_BOARD_IRQS			32
> -
> -#define NR_IRQS				(INT_BOARD_BASE + NR_BOARD_IRQS)
> -#endif
> +#define NR_IRQS                         (INT_BOARD_BASE + NR_BOARD_IRQS)

The indentation change to NR_IRQS seems like a mistake; the existing code
looks OK.
Stephen Warren - Nov. 2, 2011, 6:06 p.m.
Stephen Warren wrote at Wednesday, November 02, 2011 10:39 AM:
> Peter De Schrijver wrote by Wednesday, November 02, 2011 9:38 AM:
> > Tegra30 has 1 extra legacy interrupt controller. Use the GIC ITLinesNumber
> > field to determine how many interrupt controllers we have and initialize
> > appropriately. Also make room for the extra tegra30 interrupts by moving
> > the GPIO IRQ base. This shouldn't affect existing code as it determines the
> > correct IRQ number for GPIOs using TEGRA_GPIO_TO_IRQ()
> >
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> 
> After fixing one small issue I comment on below,
> Acked-by: Stephen Warren <swarren@nvidia.com>

Also, both this and "arm/tegra: remove unused defines":

Tested-by: Stephen Warren <swarren@nvidia.com>

(on Tegra20 Seaboard/Springbank, not on Tegra30 Cardhu)
Colin Cross - Nov. 2, 2011, 6:13 p.m.
On Wed, Nov 2, 2011 at 8:38 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
> Tegra30 has 1 extra legacy interrupt controller. Use the GIC ITLinesNumber
> field to determine how many interrupt controllers we have and initialize
> appropriately. Also make room for the extra tegra30 interrupts by moving
> the GPIO IRQ base. This shouldn't affect existing code as it determines the
> correct IRQ number for GPIOs using TEGRA_GPIO_TO_IRQ()
>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  arch/arm/mach-tegra/include/mach/iomap.h |    3 +++
>  arch/arm/mach-tegra/include/mach/irqs.h  |   14 +++++++-------
>  arch/arm/mach-tegra/irq.c                |   15 ++++++++++-----
>  3 files changed, 20 insertions(+), 12 deletions(-)
>

<snip>

> diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
> index 8ad82af..aad335b 100644
> --- a/arch/arm/mach-tegra/irq.c
> +++ b/arch/arm/mach-tegra/irq.c
<snip>
> @@ -112,8 +114,12 @@ static int tegra_retrigger(struct irq_data *d)
>  void __init tegra_init_irq(void)
>  {
>        int i;
> +       void __iomem *distbase;
> +
> +       distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
> +       num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;

Check num_ictlrs against ARRAY_SIZE(ictlr_reg_base)

Other than that:
Acked-by: Colin Cross <ccross@android.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Nov. 2, 2011, 7:09 p.m.
Stephen Warren wrote at Wednesday, November 02, 2011 12:07 PM:
> Stephen Warren wrote at Wednesday, November 02, 2011 10:39 AM:
> > Peter De Schrijver wrote by Wednesday, November 02, 2011 9:38 AM:
> > > Tegra30 has 1 extra legacy interrupt controller. Use the GIC ITLinesNumber
> > > field to determine how many interrupt controllers we have and initialize
> > > appropriately. Also make room for the extra tegra30 interrupts by moving
> > > the GPIO IRQ base. This shouldn't affect existing code as it determines the
> > > correct IRQ number for GPIOs using TEGRA_GPIO_TO_IRQ()
> > >
> > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> >
> > After fixing one small issue I comment on below,
> > Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> Also, both this and "arm/tegra: remove unused defines":
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> (on Tegra20 Seaboard/Springbank, not on Tegra30 Cardhu)

Actually, I take that back. This patch breaks stuff: just GPIO stuff which
I didn't test, not IRQ stuff which I did.

This patch changes INT_GPIO_NR from 224 to 256, which in turn changes the
value of TEGRA_NR_GPIOS. This is the number of GPIOs that the Tegra GPIO
controller "assigns" to itself when registering with gpiolib. Now, Tegra
doesn't define ARCH_NR_GPIOs, so it defaults to 256. This means there is
no GPIO numbering space left for non-Tegra GPIO controllers. In particular,
the WM8903 audio codec registers with gpiolib, and one of the GPIOs is used
on most boards to enable/disable the speaker amplifier, which now doesn't
work since the GPIO ID to control it isn't registered.

To solve this, I recommend definining ARCH_NR_GPIOs for Tegra, to something
large; I see that arch/arm/mach-shmobile defines it to 1024. Failing any
disadvantage of using a number that large, I'd go for that...
Russell King - ARM Linux - Nov. 2, 2011, 7:21 p.m.
On Wed, Nov 02, 2011 at 12:09:36PM -0700, Stephen Warren wrote:
> To solve this, I recommend definining ARCH_NR_GPIOs for Tegra, to something
> large; I see that arch/arm/mach-shmobile defines it to 1024. Failing any
> disadvantage of using a number that large, I'd go for that...

I'd suggest that we immediately start solving the ARCH_NR_GPIO
definition a single zImage friendly way - rather than throwing a
definition into mach/gpio.h, add this into asm/gpio.h:

#if CONFIG_ARCH_NR_GPIO > 0
#define ARCH_NR_GPIO CONFIG_ARCH_NR_GPIO
#endif

and in arch/arm/Kconfig:

config ARCH_NR_GPIO
	int
	default 1024 if ARCH_SHMOBILE || ARCH_TEGRA
	... etc ...
	default 0

The list should be sorted in numeric order, largest first.

This then gives us a path to eliminate the ARCH_NR_GPIO definitions
from mach/gpio.h - and should allow more mach/gpio.h to become empty.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi - Nov. 2, 2011, 7:26 p.m.
hi,

On Wed, Nov 02, 2011 at 07:21:12PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 02, 2011 at 12:09:36PM -0700, Stephen Warren wrote:
> > To solve this, I recommend definining ARCH_NR_GPIOs for Tegra, to something
> > large; I see that arch/arm/mach-shmobile defines it to 1024. Failing any
> > disadvantage of using a number that large, I'd go for that...
> 
> I'd suggest that we immediately start solving the ARCH_NR_GPIO
> definition a single zImage friendly way - rather than throwing a
> definition into mach/gpio.h, add this into asm/gpio.h:
> 
> #if CONFIG_ARCH_NR_GPIO > 0
> #define ARCH_NR_GPIO CONFIG_ARCH_NR_GPIO
> #endif
> 
> and in arch/arm/Kconfig:
> 
> config ARCH_NR_GPIO
> 	int
> 	default 1024 if ARCH_SHMOBILE || ARCH_TEGRA
> 	... etc ...
> 	default 0
> 
> The list should be sorted in numeric order, largest first.
> 
> This then gives us a path to eliminate the ARCH_NR_GPIO definitions
> from mach/gpio.h - and should allow more mach/gpio.h to become empty.

would it be better to just change the default value in
arm-generic/gpio.h to something very large ?

I mean, ideally that wouldn't be gpio_desc wouldn't be an array anyway
right ?
Russell King - ARM Linux - Nov. 2, 2011, 7:39 p.m.
On Wed, Nov 02, 2011 at 09:26:26PM +0200, Felipe Balbi wrote:
> would it be better to just change the default value in
> arm-generic/gpio.h to something very large ?
> 
> I mean, ideally that wouldn't be gpio_desc wouldn't be an array anyway
> right ?

You'll excuse me if I take this slightly personally.

You really can't expect me to say that I'm fine with a 6K growth in
kernel size for something that not every platform needs if there's
been objections to maybe a 128 byte growth for including the V:P
patching code in the kernel by default.

Either we care about memory usage or we don't.  If we don't, lets get
rid of offering ARM_PATCH_PHYS_VIRT in any configuration and always
build with the dynamic V:P stuff enabled for the trivial cases.  I
mean:

 config ARM_PATCH_PHYS_VIRT
-	bool "Patch physical to virtual translations at runtime" if EMBEDDED
+	bool
	default y
	depends on !XIP_KERNEL && MMU
	depends on !ARCH_REALVIEW || !SPARSEMEM
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi - Nov. 2, 2011, 7:48 p.m.
Hi,

On Wed, Nov 02, 2011 at 07:39:47PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 02, 2011 at 09:26:26PM +0200, Felipe Balbi wrote:
> > would it be better to just change the default value in
> > arm-generic/gpio.h to something very large ?
> > 
> > I mean, ideally that wouldn't be gpio_desc wouldn't be an array anyway
> > right ?
> 
> You'll excuse me if I take this slightly personally.
> 
> You really can't expect me to say that I'm fine with a 6K growth in
> kernel size for something that not every platform needs if there's
> been objections to maybe a 128 byte growth for including the V:P
> patching code in the kernel by default.
> 
> Either we care about memory usage or we don't.  If we don't, lets get
> rid of offering ARM_PATCH_PHYS_VIRT in any configuration and always
> build with the dynamic V:P stuff enabled for the trivial cases.  I
> mean:
> 
>  config ARM_PATCH_PHYS_VIRT
> -	bool "Patch physical to virtual translations at runtime" if EMBEDDED
> +	bool
> 	default y
> 	depends on !XIP_KERNEL && MMU
> 	depends on !ARCH_REALVIEW || !SPARSEMEM

you forgot to comment on the fact that gpio_desc shouldn't be held in an
array. Any comments ?

What I mean is that, just like irq_descs, we should be able to allocate
them dynamically. Maybe, just like irq_descs, hold them in a radix tree
and maybe even have a matching API "gpio_alloc_descs()".

It's a pain, anyway, to keep track of GPIO base numbers, specially on
complex boards where you have gpio controllers which are internal to the
SoC and several others connected via e.g. I2C.

Most PHY chips for several I/O have some extra GPIOs which are generally
unused or hacked around (see tusb6010.c for example).
Russell King - ARM Linux - Nov. 2, 2011, 7:54 p.m.
On Wed, Nov 02, 2011 at 09:48:28PM +0200, Felipe Balbi wrote:
> Hi,
> 
> you forgot to comment on the fact that gpio_desc shouldn't be held in an
> array. Any comments ?

I did not comment on that because that's someone elses problem.

> What I mean is that, just like irq_descs, we should be able to allocate
> them dynamically. Maybe, just like irq_descs, hold them in a radix tree
> and maybe even have a matching API "gpio_alloc_descs()".

Probably - I expect Grant would really like to see some patches along
those lines.  As I say, someone elses problem.

But what is _our_ problem is what to do with all the ARCH_NR_GPIOs
that we have now, and stop them increasing.  There is a trivial solution
to that which I outlined which can be used until GPIO gets something
along your idea - and which doesn't involve me having to argue with
those who think that the kernel should remain as small as possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi - Nov. 2, 2011, 8:40 p.m.
Hi,

On Wed, Nov 02, 2011 at 07:54:04PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 02, 2011 at 09:48:28PM +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > you forgot to comment on the fact that gpio_desc shouldn't be held in an
> > array. Any comments ?
> 
> I did not comment on that because that's someone elses problem.

fair enough.

> > What I mean is that, just like irq_descs, we should be able to allocate
> > them dynamically. Maybe, just like irq_descs, hold them in a radix tree
> > and maybe even have a matching API "gpio_alloc_descs()".
> 
> Probably - I expect Grant would really like to see some patches along
> those lines.  As I say, someone elses problem.
> 
> But what is _our_ problem is what to do with all the ARCH_NR_GPIOs
> that we have now, and stop them increasing.  There is a trivial solution
> to that which I outlined which can be used until GPIO gets something
> along your idea - and which doesn't involve me having to argue with
> those who think that the kernel should remain as small as possible.

k, I'll see if I can cook something up.

Patch

diff --git a/arch/arm/mach-tegra/include/mach/iomap.h b/arch/arm/mach-tegra/include/mach/iomap.h
index 19dec3a..67644c9 100644
--- a/arch/arm/mach-tegra/include/mach/iomap.h
+++ b/arch/arm/mach-tegra/include/mach/iomap.h
@@ -74,6 +74,9 @@ 
 #define TEGRA_QUATERNARY_ICTLR_BASE	0x60004300
 #define TEGRA_QUATERNARY_ICTLR_SIZE	SZ_64
 
+#define TEGRA_QUINARY_ICTLR_BASE	0x60004400
+#define TEGRA_QUINARY_ICTLR_SIZE	SZ_64
+
 #define TEGRA_TMR1_BASE			0x60005000
 #define TEGRA_TMR1_SIZE			SZ_8
 
diff --git a/arch/arm/mach-tegra/include/mach/irqs.h b/arch/arm/mach-tegra/include/mach/irqs.h
index 73265af..b6ebb8e 100644
--- a/arch/arm/mach-tegra/include/mach/irqs.h
+++ b/arch/arm/mach-tegra/include/mach/irqs.h
@@ -25,7 +25,8 @@ 
 
 #define IRQ_LOCALTIMER                  29
 
-#ifdef CONFIG_ARCH_TEGRA_2x_SOC
+/* We only list the Tegra20 interrupts here as Tegra30 will always use FDT */
+
 /* Primary Interrupt Controller */
 #define INT_PRI_BASE			(INT_GIC_BASE + 32)
 #define INT_TMR1			(INT_PRI_BASE + 0)
@@ -166,18 +167,17 @@ 
 #define INT_QUAD_RES_30			(INT_QUAD_BASE + 30)
 #define INT_QUAD_RES_31			(INT_QUAD_BASE + 31)
 
-#define INT_MAIN_NR			(INT_QUAD_BASE + 32 - INT_PRI_BASE)
-
+/* Tegra30 has 5 banks of 32 IRQs */
+#define INT_MAIN_NR			(32 * 5)
 #define INT_GPIO_BASE			(INT_PRI_BASE + INT_MAIN_NR)
 
-#define INT_GPIO_NR			(28 * 8)
+/* Tegra30 has 8 banks of 32 GPIOs */
+#define INT_GPIO_NR			(32 * 8)
 
 #define TEGRA_NR_IRQS			(INT_GPIO_BASE + INT_GPIO_NR)
 
 #define INT_BOARD_BASE			TEGRA_NR_IRQS
 #define NR_BOARD_IRQS			32
-
-#define NR_IRQS				(INT_BOARD_BASE + NR_BOARD_IRQS)
-#endif
+#define NR_IRQS                         (INT_BOARD_BASE + NR_BOARD_IRQS)
 
 #endif
diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 8ad82af..aad335b 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -43,14 +43,16 @@ 
 #define ICTLR_COP_IER_CLR	0x38
 #define ICTLR_COP_IEP_CLASS	0x3c
 
-#define NUM_ICTLRS 4
 #define FIRST_LEGACY_IRQ 32
 
+static int num_ictlrs;
+
 static void __iomem *ictlr_reg_base[] = {
 	IO_ADDRESS(TEGRA_PRIMARY_ICTLR_BASE),
 	IO_ADDRESS(TEGRA_SECONDARY_ICTLR_BASE),
 	IO_ADDRESS(TEGRA_TERTIARY_ICTLR_BASE),
 	IO_ADDRESS(TEGRA_QUATERNARY_ICTLR_BASE),
+	IO_ADDRESS(TEGRA_QUINARY_ICTLR_BASE),
 };
 
 static inline void tegra_irq_write_mask(unsigned int irq, unsigned long reg)
@@ -59,7 +61,7 @@  static inline void tegra_irq_write_mask(unsigned int irq, unsigned long reg)
 	u32 mask;
 
 	BUG_ON(irq < FIRST_LEGACY_IRQ ||
-		irq >= FIRST_LEGACY_IRQ + NUM_ICTLRS * 32);
+		irq >= FIRST_LEGACY_IRQ + num_ictlrs * 32);
 
 	base = ictlr_reg_base[(irq - FIRST_LEGACY_IRQ) / 32];
 	mask = BIT((irq - FIRST_LEGACY_IRQ) % 32);
@@ -112,8 +114,12 @@  static int tegra_retrigger(struct irq_data *d)
 void __init tegra_init_irq(void)
 {
 	int i;
+	void __iomem *distbase;
+
+	distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
+	num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;
 
-	for (i = 0; i < NUM_ICTLRS; i++) {
+	for (i = 0; i < num_ictlrs; i++) {
 		void __iomem *ictlr = ictlr_reg_base[i];
 		writel(~0, ictlr + ICTLR_CPU_IER_CLR);
 		writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
@@ -125,6 +131,5 @@  void __init tegra_init_irq(void)
 	gic_arch_extn.irq_unmask = tegra_unmask;
 	gic_arch_extn.irq_retrigger = tegra_retrigger;
 
-	gic_init(0, 29, IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE),
-		 IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+	gic_init(0, 29, distbase, IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
 }