Subject: Re: [PATCH] General CHRP/MPC5K2 platform support patch
Sent: Sat, 28 Oct 2006
From: "Benjamin Herrenschmidt" <[EMAIL PROTECTED]>
> On Fri, 2006-10-27 at 17:04 +0200, Nicolas DET wrote:
>
> > +static void mpc52xx_ic_mask(unsigned int virq)
> > +{
> > + u32 val;
> > + int irq;
> > + int l1irq;
> > + int l2irq;
> > +
> > + irq = irq_map[virq].hwirq;
> > + l1irq = (irq & MPC52xx_IRQ_L1_MASK) >> MPC52xx_IRQ_L1_OFFSET;
> > + l2irq = (irq & MPC52xx_IRQ_L2_MASK) >> MPC52xx_IRQ_L2_OFFSET;
> > +
> > + pr_debug("%s: irq=%x. l1=%d, l2=%dn", __func__, irq, l1irq, l2irq);
> > +
> > + switch (l1irq) {
> > + case MPC52xx_IRQ_L1_CRIT:
> > + if (l2irq != 0)
> > + BUG();
> > +
> > + val = in_be32(&intr->ctrl);
> > + val &= ~(1 << 11);
> > + out_be32(&intr->ctrl, val);
> > + break;
> > +
> > + case MPC52xx_IRQ_L1_MAIN:
> > + if ( (l2irq >= 1) && (l2irq <= 3) ) {
> > + val = in_be32(&intr->ctrl);
> > + val &= ~(1 << (11 - l2irq));
> > + out_be32(&intr->ctrl, val);
> > + } else {
> > + val = in_be32(&intr->main_mask);
> > + val |= 1 << (16 - l2irq);
> > + out_be32(&intr->main_mask, val);
> > + }
> > + break;
>
> Any reason why you do the above instead o defining two diferent levels
> instead. Also, L1_CRIT would fit in the L1_MAIN case too...
>
> I don't see the point of having also XXX-l2, just put a bit number in L2
> and be done with it.
>
> The idea is to streamling the code, such that you can index an array
> with l1irq to get the register, and build a mask. No test, no switch
> case, etc...
>
> Actually... the most performant way of doing all this is to have a
> different irq_chip (with a different set of ask,mask,unmask) for each
> "L1" so that they get right to the point. But I would settle for an
> array indexed by L1.
>
Ok. Let's go for the most performant then. So 4 irq_chip for the for type. Each
is very simple: just clear/set the approprite bit in the appropriate register.
> > +static void mpc52xx_ic_mask_and_ack(unsigned int irq)
> > +{
> > + mpc52xx_ic_mask(irq);
> > + mpc52xx_ic_ack(irq);
> > +}
>
> The above is unnuecessary. The core will call mask and ack.
Ok. Removed.
> > +static struct irq_chip mpc52xx_irqchip = {
> > + .typename = " MPC52xx ",
> > + .mask = mpc52xx_ic_mask,
> > + .unmask = mpc52xx_ic_unmask,
> > + .mask_ack = mpc52xx_ic_mask_and_ack,
> > +};
>
> In the above, you need to provide ack.
Done.
> > +static int mpc52xx_irqhost_match(struct irq_host *h, struct device_node
> *node)
> > +{
> > + pr_debug("%s: %p vs %pn", __func__, find_mpc52xx_picnode(), node);
> > + return find_mpc52xx_picnode() == node;
> > +}
>
> Don't redo the whole find all the time. Put the node in your
> irq_host->host_data at init time and compare it there. That way, also,
> find_mpc53xx_picnode() thingy can just stay static to chrp/setup.c. The
> way you did it would be a problem if we had more than one platform using
> 52xx built in the same kernel.
>
done.
> > +static int mpc52xx_islevel(int num)
> > +{
> > + u32 ictl_req;
> > + int ictl_type;
> > +
> > + ictl_req = in_be32(&intr->ctrl);
> > + ictl_type = (ictl_req >> 16) & 0x3;
> > +
> > + switch (ictl_type) {
> > + case 0:
> > + case 3:
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
> You only have level/edge settings, not polarity ? Also, you aren't
> giving the user or device-tree the option to set the type manually....
>
> Ideally, you should use the bits in IRQ_TYPE_SENSE_MASK to fully
> define the irq type/polarity and provide a set_irq_type() callback.
>
> Also, beware that you cannot call set_irq_chip_and_handler() from within
> set_irq_type() due to a spinlock recursion issue. There is a patch
> floating on the list for IPIC fixing an issue of that sort, you may want
> to have a look.
>
> In general, look at what others are doing, notably mpic and ipic.
I looked in i8258.c, let's look into others ;-)
> You can also keep IRQ_LEVEL in "sync" with the other polarity bits, it's
> really only useful to display the polarity in /proc/interrupts.
>
> Note that your map code would always OR the bit, never clear it. It
> should have probably cleared it before the switch/case.
> > +void mpc52xx_irqhost_unmap(struct irq_host *h, unsigned int virq)
> > +{
> > + pr_debug("%s: v=%xn", __func__, virq);
> > +
> > + mpc52xx_ic_mask(virq);
> > + set_irq_chip_and_handler(virq, NULL, NULL);
> > + synchronize_irq(virq);
> > +}
>
> The common code will do all of the above. Your unmap can be empty. (Or
> just don't do an unmap).
>
Ok.
> > +static struct irq_host_ops mpc52xx_irqhost_ops = {
> > + .match = mpc52xx_irqhost_match,
> > + .xlate = mpc52xx_irqhost_xlate,
> > + .map = mpc52xx_irqhost_map,
> > + .unmap = mpc52xx_irqhost_unmap,
> > +};
> > +
> > +void __init mpc52xx_init_irq(void)
> > +{
> > + int i;
> > + u32 intr_ctrl;
> > +
> > + /* Remap the necessary zones */
> > + intr = ioremap(MPC52xx_PA(MPC52xx_INTR_OFFSET), MPC52xx_INTR_SIZE);
> > + sdma = ioremap(MPC52xx_PA(MPC52xx_SDMA_OFFSET), MPC52xx_SDMA_SIZE);
> > +
> > + if ((intr == NULL) || (sdma == NULL))
> > + panic("Can't ioremap PIC/SDMA register or init_irq !");
> > +
> > + /* Disable all interrupt sources. */
> > + out_be32(&sdma->IntPend, 0xffffffff); /* 1 means clear pending */
> > + out_be32(&sdma->IntMask, 0xffffffff); /* 1 means disabled */
> > + out_be32(&intr->per_mask, 0x7ffffc00); /* 1 means disabled */
> > + out_be32(&intr->main_mask, 0x00010fff); /* 1 means disabled */
> > + intr_ctrl = in_be32(&intr->ctrl);
> > + intr_ctrl &= 0x00ff0000; /* Keeps IRQ[0-3] config */
> > + intr_ctrl |= 0x0f000000 | /* clear IRQ 0-3 */
> > + 0x00001000 | /* MEE master external enable */
> > + 0x00000000 | /* 0 means disable IRQ 0-3 */
> > + 0x00000001; /* CEb route critical normally */
> > + out_be32(&intr->ctrl, intr_ctrl);
> > +
> > + /* Zero a bunch of the priority settings. */
> > + out_be32(&intr->per_pri1, 0);
> > + out_be32(&intr->per_pri2, 0);
> > + out_be32(&intr->per_pri3, 0);
> > + out_be32(&intr->main_pri1, 0);
> > + out_be32(&intr->main_pri2, 0);
> > + /* Initialize irq_desc[i].handler's with mpc52xx_ic. */
> > + for (i = 0; i < NR_IRQS; i++) {
> > + irq_desc[i].chip = &mpc52xx_irqchip;
> > + irq_desc[i].status = IRQ_LEVEL;
> > +
> > + }
>
> The above is completely bogus and should be just removed. You should not
> touch the irq_desc array. You should only ever modify irq_desc's that
> have been assigned to you by your host->map callback.
>
Ok.
> > + /*
> > + * As last step, add an irq host to translate the real
> > + * hw irq information provided by the ofw to linux virq
> > + */
> > +
> > + mpc52xx_irqhost =
> > + irq_alloc_host(IRQ_HOST_MAP_LINEAR, NR_IRQS, &mpc52xx_irqhost_ops,
> > + -1);
> > +}
>
> As I said before, NR_IRQ is a poor choice for the size of the revmap.
> You should have a constant somewhere defining what is your max HW irq
> number. and use that +1, or a hw irq count.
Done: 64
Bye
_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded
