| Message ID | 1320248292-22736-1-git-send-email-pdeschrijver@nvidia.com |
|---|---|
| State | New, archived |
| Headers | show |
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 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)
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 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...
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
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 ?
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
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).
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
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.
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)); }
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(-)