Re: [PATCH 01/11] irqchip: Allow irq_reg_{readl,writel} to use __raw_{readl_writel}

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Wednesday 29 October 2014 10:36:25 Florian Fainelli wrote:
> On 10/29/2014 12:43 AM, Arnd Bergmann wrote:
> > On Tuesday 28 October 2014 20:58:48 Kevin Cernekee wrote:
> >>
> >> +#ifdef CONFIG_RAW_IRQ_ACCESSORS
> >> +
> >> +#ifndef irq_reg_writel
> >> +# define irq_reg_writel(val, addr)     __raw_writel(val, addr)
> >> +#endif
> >> +#ifndef irq_reg_readl
> >> +# define irq_reg_readl(addr)           __raw_readl(addr)
> >> +#endif
> >> +
> >> +#else
> >> +
> > 
> > No, this is just wrong: registers almost always have a fixed endianess
> > indenpent of CPU endianess, so if you use __raw_writel, it will be
> > broken on one or the other.
> 
> Our brcmstb platforms had an endian strap settings for MIPS-based
> platforms, and for most peripherals this would be just completely
> transparent, as the HW always will do the internal swapping, such that
> host CPU does read/writes in its native endianess regardless of the
> actual strap settings.

That's irrelevant, it just makes matters worse because then you
might run into all combinations of big-endian and little-endian
kernels vs MMIO registers.

> AFAICT bcm3384, a MIPS-based Cable Modem platform has only one endianess
> setting: BE, and the HW only supports that specific endianess.

But they might have a secondary interrupt controller on a PCI card that
uses the same driver in the little-endian mode.

> > If you have a machine that uses big-endian registers in the interrupt
> > controller, you need to find a way to use the correct accessors
> > (e.g. iowrite32be) and use them independent of what endianess the CPU
> > is running.
> > 
> > As this code is being used on all sorts of platforms, you can't assume
> > that they all use the same endianess, which makes it rather tricky.
> 
> I think the more general problem with the use of readl_*() I/O accessors
> is that they just happen to work fine on most platforms out there: ARM
> Little-endian, because this nicely matches the endianess expected by the
> HW and that does not enforce an audit of whether your actual peripheral
> expects little-endian writes to be done.

Most peripherals have registers that are designed for PCI compatibility,
and PCI mandates little-endian registers. This has very little to do with
the architecture.

We also have a lot of peripherals that have big-endian registers, e.g
for things that are shared between ARM and PowerPC.

The only hardware that is causing problems is the kind where the hardware
developer tried to be helpful by making it possible to change endianess,
and this is what causes endless nightmares for kernel developers.

I think the easiest solution here is to make this irqchip not use
the irq-generic logic, because it clearly is not generic.

> The other problem is that readl() on ARM implies a barrier, which is not
> necesarily true/necessary for some other platforms such as some MIPS
> processors.

readl has to provide the semantics that PCI devices expect on x86.
On x86, it implies a barrier, so everything else has to do the same.

If you know that a driver does not need barriers, you can use
readl_relaxed(). It doesn't currently work on all architectures, but
it works on MIPS and I have a patch series from Will Deacon that I want
to push to make it work everywhere.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html