Re: [PATCH 04/17 v1] irqchip: Add driver for IXP4xx

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Feb 11, 2019 at 4:30 PM Marc Zyngier <marc.zyngier@arm.com> wrote:

> > +static int ixp4xx_set_irq_type(struct irq_data *d, unsigned int type)
> > +{
> > +     /* All are level active high (asserted) here */
>
> It'd be good to return an error if type isn't LEVEL_HIGH.

OK

> > +     if (ixi->is_356 && d->hwirq >= 32) {
> > +             val = __raw_readl(ixi->irqbase + IXP4XX_ICMR2);
> > +             val &= ~BIT(d->hwirq - 32);
> > +             __raw_writel(val, ixi->irqbase + IXP4XX_ICMR2);
> > +     } else {
> > +             val = __raw_readl(ixi->irqbase + IXP4XX_ICMR);
> > +             val &= ~BIT(d->hwirq);
> > +             __raw_writel(val, ixi->irqbase + IXP4XX_ICMR);
> > +     }
> > +}
>
> This probably comes from the original code, but I'd like to be able to
> use a LE kernel on this HW (full disclosure: I have some of this crap
> stashed somewhere in the basement... ;-).

It will work. One reason I am refactoring and preserving IXP4xx
is this stuff from the datasheet:

Ambient Air Temperature (Extended)  -40° C to 85° C
Ambient Air Temperature (Commercial) 0° C to 70° C
MTBF: 60 Years at 55°C

And I don't think there is any big difference between the "extended"
and "commercial" version of this SoC, they all come in the same
sturdy packaging too. Diamonds and IXP4xx are forever.

> How about using something that enforces the endianness of the accesses,
> as I suspect the bus is hardcoded to BE?

So, IIUC (Krzysztof can confirm) IXP4xx works like this that when the
CPU is brought up in LE mode, all bytes on the address bus are
swizzled so that the access can always use the CPU native
endianness.

Isn't it crazy :D

This means __raw_writel() will always do the right thing, because
that uses native endianness, and that is why it is used quite a lot
in IXP4xx drivers.

If you want full control you need to define local helpers for
everything that change behaviour at compile-time such as:

static void foo_writel(u32 val, __iomem void *addr)
{
#if defined(CONFIG_CPU_BIG_ENDIAN)
   iowrite32be(val, addr);
#else
    writel(val, addr);
#endif
}

Which will have the same effect as __raw_writel().

So that is why we use the (ugly) macro __raw_writel() a lot
in IXP4xx drivers.

> ioread32be/iowrite32be springs
> to mind, and I can see the current IXP4xx code provides such an
> implementation in its own io.h (which you may have to make private).

The stuf in <mach/io.h> is there for indirect PCI, which is
another unrelated oddity. PCI devices are always LE (as is
proper). It is there because the PCI host on the IXP4xx can
only access > 64MB PCI memory space with certain quirks that
no other PCI host controller needs.

I will get to this when I try to move that driver to drivers/pci ...
It's unrelated to this business however.

> > +     ixi->irqchip.name = "IXP4xx";
> > +     ixi->irqchip.irq_mask = ixp4xx_irq_mask;
> > +     ixi->irqchip.irq_unmask = ixp4xx_irq_unmask;
> > +     ixi->irqchip.irq_set_type = ixp4xx_set_irq_type;
>
> Aren't you guaranteed to only have one such irqchip? If so, this could
> become a static const object, instead of allocating it dynamically. Not
> a big deal though.

Force of habit. Comes from the GPIO side of things where we
never know how many instances there are of stuff, if it's OK
I'd like to keep it.

> > +     fwnode = irq_domain_alloc_fwnode(base);
>
> I assume this is a temporary solution until the SoC gains a DT port (and
> the irqchip a DT node)?

Yes, see the later DT patch that passes the parent as
fwnode, once all boards are converted we can drop this.

BTW here is how I then use the hierarchical parent in the
GPIO chip:
https://marc.info/?l=linux-arm-kernel&m=154923023907623&w=2
https://marc.info/?l=linux-arm-kernel&m=154923038707686&w=2

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel