Re: OMAP34xx

Tony Lindgren
2012-02-05T16:57:19Z

* 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