On Tuesday 17 July 2007, Mark Zhan wrote:
> Hi Arnd,
>
> > > +/*
> > > + * For SBCPQ2 board, the interrupt of M48T59 RTC chip
> > > + * will generate a machine check exception. We use a
> > > + * fake irq to give the platform machine_check_exception() hook
> > > + * a chance to call the driver ISR. If IRQ_HANDLED is returned,
> > > + * then we will survive from the machine check exception.
> > > + */
> > > +static int sbcpq2_mach_check(struct pt_regs *regs)
> > > +{
> > > + int recover = 0;
> > > + struct irq_desc *desc = irq_desc + SBCPQ2_M48T59_IRQ;
> > > +
> > > + struct irqaction *action = desc->action;
> > > +
> > > + while (action && (action->dev_id != &m48t59_rtc))
> > > + action = action->next;
> > > +
> > > + /* Try to call m48t59 RTC driver ISR */
> > > + if (action && action->handler)
> > > + recover = action->handler(SBCPQ2_M48T59_IRQ, &m48t59_rtc);
> > > +
> > > + return recover;
> > > +}
> >
> > What you do here looks really scary, but maybe I'm just misunderstanding
> > it completely. Why don't you just register your rtc handler function
> > as the machine check handler instead of going through various indirections?
> >
>
> The rtc M48T59 driver is not specific to my board, it is probably used
> by other board. So I can't register the rtc intr handler as the mcheck
> exception handler. And in the other side, there are also other machine
> check sources, right?
>
> So here I add a platform mcheck hook for rtc intr handler. Yeah, it is
> really scary and confusing:-)
I think it would really be easier to just make the m48t59 irq handler
a global function, and keep it out of the regular interrupt logic.
Then your sbcpq2_mach_check() basically becomes trivial like
static int sbcpq2_mach_check(struct pt_regs *regs)
{
return m48t59_irq(NO_IRQ, NULL);
}
This has the advantage that you don't need to wait for the
m48t59 driver to be initialized first, instead you will just
get a link failure it that driver is not already built into the
kernel.
> > > +static void __init sbcpq2_init_IRQ(void)
> > > +{
> > > + struct device_node *np;
> > > + struct resource res;
> > > +
> > > + np = of_find_compatible_node(NULL, "cpm-pic", "CPM2");
> > > + if (np == NULL) {
> > > + printk(KERN_ERR "PIC init: can not find cpm-pic node\n");
> > > + return;
> > > + }
> >
> > This looks like your device tree is wrong. Shouldn't the interrupt
> > controller have device_type="interrupt-controller" and a specific
> > compatible property instead of having the name in the device_type?
> >
>
> Here, I just copy the codes from mpc82xx_ads, is there anything wrong?
I just checked the Recommended Practice document for interrupt mapping
and it seems that it's ok. The interrupt controller needs to have
an property named "interrupt-controller", but does not need a specific
device_type. So it appears to be correct here.
> > > + /* Boot Flash is the on-board flash */
> > > + mc->memc_br0 = (SBCPQ2_BOOT_FLASH_BASE & 0xFFFF8000) | 0x0801;
> > > + mc->memc_or0 = 0xFFE00896;
> >
> > consequently, this needs to use out_be32 or similar.
> > Where does SBCPQ2_BOOT_FLASH_BASE come from? Shouldn't that be set
> > up by the boot loader to match the device tree?
>
> Fixed. out_be32 is used.
btw, it would be good if you can run your code through the 'sparse'
checker. It will warn about this type of problem. I think I saw all
that you have added here, but I may have missed some, and sparse
can also find other problems. Just install the tool as it comes
with your distro and build the kernel with the 'C=1' make option.
> The reason why they are needed is because some
> legacy u-boot for this board probably was setting up the wrong memory
> map.
Hmm, will those legacy u-boot version be able to even boot this kernel?
The device tree looks like it needs to have some variables set by u-boot,
so I'd guess you don't need to worry about old versions that don't
set those either.
Arnd <><
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
