[PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver

[arnd@arndb.de (Arnd Bergmann)]

On Monday 05 August 2013, Alexander Shiyan wrote:
> On Mon, 5 Aug 2013 21:26:07 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Monday 05 August 2013, Alexander Shiyan wrote:
> 
> > > > Handling them in the irqchip driver probably seemed like a logical solution
> > > > when the code was initially written, but if you move that access into the
> > > > device drivers, you can probably remove the irqchip driver entirely and just
> > > > use the generic irqchip implementation.
> > > 
> > > ioremap for a whole set of CPU registers is called from map_io, so ioremap
> > > returns already mapped address, that is, it's just converting physical to
> > > virtual addresses.
> > 
> > I see, that does make things better at least.
> > 
> > It would be nice to change the DT binding in some way so the register
> > ranges are non-overlapping, but I'm not going to insist if the other
> > reviewers are ok with the small inconsistency here.
> > 
> > Do you have any reason for not using the syscon driver for these
> > registers?
> 
> I plan to use syscon for some registers to ensure the correct sequence
> of read-modify-write. For all other registers, the access to which is not
> needed from the different subsystems, it does not make sense and seriously
> degrade performance due to locks.

Have you measure this? MMIO accesses are typically really slow already, they
can be multiple orders of magnitude slower than the locks. Also, note that
spinlocks get optimized out on uniprocessor machines.

The main reason why syscon was introduced is to handle MMIO register sets
that span multiple devices within a small range, which is exactly what you
have here. You could simply have a single ioremap in the syscon driver
and refer to register offsets from drivers using the syscon binding. You
could then actually describe all the EIO registers in a way that 

> As alternative, I can completely remove clps711x_ioremap_one() and simply
> duplicate ioremap() for the entire range set of registers
> (without request_mem_region). The functionality of driver will remain the same.
> Will it be better?

That wouldn't change the binding in any way, so I don't see a reason to prefer
one or the other (ioremap or clps711x_ioremap_one), especially since you
explained that the registers already come with a static boot-time mapping.

	Arnd