Re: OMAP34xx

[Tony Lindgren <tony@atomide.com>]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [120205 02:39]:
> On Sat, Feb 04, 2012 at 05:55:30PM -0800, Tony Lindgren wrote:
> > Hi,
> > 
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [120204 12:03]:
> > > 
> > > Another problem oopses the kernel at boot in the voltage domain code,
> > > which I suspect this has never been boot tested on OMAP34xx CPUs:
> > > 
> > > omap_vc_init_channel: PMIC info requried to configure vc forvdd_core not populated.Hence cannot initialize vc
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > ...
> > > Backtrace: 
> > > [<c03db824>] (omap_vp_init+0x0/0x15c) from [<c03db448>] (omap_voltage_late_init+
> > > 0xc4/0xfc)
> > > [<c03db384>] (omap_voltage_late_init+0x0/0xfc) from [<c03d9df8>] (omap2_common_p
> > > m_late_init+0x14/0x54)
> > >  r8:00000000 r7:00000013 r6:c0039988 r5:c03fe004 r4:c03fdfb8
> > > [<c03d9de4>] (omap2_common_pm_late_init+0x0/0x54) from [<c0008798>] (do_one_init
> > > call+0x9c/0x164)
> > > [<c00086fc>] (do_one_initcall+0x0/0x164) from [<c03d1284>] (kernel_init+0x7c/0x1
> > > 20)
> > > [<c03d1208>] (kernel_init+0x0/0x120) from [<c0039988>] (do_exit+0x0/0x2cc)
> > >  r5:c03d1208 r4:00000000
> > ...
> > 
> > > And OMAP4 doesn't build.  Fixing the build error in prm44xx.c (which is
> > > virtually the same as the OMAP3 build error in its prm file) gets us to
> > > a buildable state, but... it silently fails to boot.
> > > 
> > > arch/arm/mach-omap2/prm44xx.c:41: error: 'OMAP44XX_IRQ_PRCM' undeclared here (not in a function)
> > > 
> > > At least enabling DEBUG_LL gets this reason:
> > > 
> > > <1>Unable to handle kernel NULL pointer dereference at virtual address 00000000 
> > ...
> > > Backtrace:                                                                      
> > > [<c007bab4>] (irq_domain_add+0x0/0x134) from [<c029baac>] (twl_probe+0xd0/0x370)
> > >  r6:c03bef90 r5:00000014 r4:00000170                                            
> > > [<c029b9dc>] (twl_probe+0x0/0x370) from [<c01eee70>] (i2c_device_probe+0xb0/0xe4
> > > )                                                                               
> > > [<c01eedc0>] (i2c_device_probe+0x0/0xe4) from [<c01d1f34>] (really_probe+0xa0/0x
> > > 178)                                                                            
> > ...
> > 
> > Just to verify, it seems that both of these happen because of
> > IRQ_DOMAIN not being set?
> 
> No.  The OMAP3 boot oops is partly because of that rubbish dependency
> preventing the PMIC support code being build, but that is only part of
> the problem and therefore that patch is only _part_ of the solution.

OK
 
> It's clearly a legal configuration for the TWL support to be disabled,
> and the kernel should NOT oops on boot because of that.  So even though
> removing the IRQ_DOMAIN dependency _and_ enabling the driver makes the
> oops go away, that's _really_ not a fix by any shape or form.
> 
> Code should _never_ oops because something isn't built-in that's required.
> The vc layer of the voltage domain cope with the missing pmic, and so
> should the vp layer.

Agreed. The TWL support be optional. It is possible to have a some non-TWL/TPS
PMIC: For example Nokia n8x0 were using retu/tahvo instead of TPS PMIC.
 
> As for OMAP4, in no way does that patch fix the problem there, because
> OMAP4 has the GIC, and the GIC selects CONFIG_IRQ_DOMAIN.  The problem is
> much more fundamental, and of course the code reveals why:
> 
> drivers/mfd/twl-core.c:
>         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);
> 
> kernel/irq/irqdomain.c:
> void irq_domain_add(struct irq_domain *domain)
> {
>         irq_domain_for_each_irq(domain, hwirq, irq) {
> 
> include/linux/irqdomain.h:
> #define irq_domain_for_each_irq(d, hw, irq) \
>         for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \
>              hw < d->hwirq_base + d->nr_irq; \
>              hw++, irq = irq_domain_to_irq(d, hw))
> 
> static inline unsigned int irq_domain_to_irq(struct irq_domain *d,
>                                              unsigned long hwirq)
> {
>         if (d->ops->to_irq)
>                 return d->ops->to_irq(d, hwirq);
> 
> Basically, the irq domain ops pointer _must_ _always_ be a valid pointer
> before an irq domain is added.
> 
> At a guess, twl-core.c should be:
> 
> drivers/mfd/twl-core.c:
> #ifdef CONFIG_IRQ_DOMAIN
>         domain.irq_base = pdata->irq_base;
>         domain.nr_irq = nr_irqs;
> #ifdef CONFIG_OF_IRQ
>         domain.of_node = of_node_get(node);
> #endif
>         domain.ops = &irq_domain_simple_ops;
>         irq_domain_add(&domain);
> #endif

OK makes sense.

Regards,

Tony