Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 07 May 2015 11:42:46 Brian Norris wrote:
> On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote:
> > On Wednesday 06 May 2015 13:49:10 Brian Norris wrote:
> > > On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote:
> > > > It looks to me like this should be handled as a nested irqchip, so the node
> > > > you look up gets used as the "interrupt-parent" instead, making the behavior
> > > > of this SoC transparent to the nand driver.
> > > 
> > > You snipped the rest of the patch, which involves more than just IRQ
> > > handling. The same registers touch both interrupts and data bus endian
> > > configuration, so it can't possibly be done transparently to the NAND
> > > driver.
> > 
> > Anything else in there?
> 
> Looks like miscellaneous NAND-related control bits. AXI and APB endian
> configuration; several interrupt-enable bits (we only use one for now);
> a clock-enable; and some timing test mode bits.

I see. Describing that as an irqchip would indeed be too much of a stretch.

> > The bus configuration would just involve writing
> > a constant value in some register, right?
> 
> I'm not an expert on the Cygnus/iProc chips, but I believe the answer is
> no: we *must* reconfigure the bus before and after each data
> transaction, because it affects the rest of the core too. Others on the
> CC list can probably elaborate.

That would not be a problem I think: the irqchip driver would always
get initialized first, because the main driver would get an -EPROBE_DEFER
when requesting the interrupt line otherwise.

> > Doing that in the irqchip
> > is also a bit ugly, but may still be better overall than doing it the
> > way you have above.
> 
> Well, the Cygnus/iProc case is more complicated as I mention. But I
> agree that other cases could be nicer, like bcm63138 (which only has
> separate interrupt status/enable registers). Do you expect a new irqchip
> driver for every arrangement of registers like this? There are a few
> similar chips that have status/enable registers in different orders, and
> some that combine them into a single word. Do we really need a new
> irqchip driver for each one? I might have done that for bcm63138 and
> bcm3384, except that I thought I'd have to fall back to this extra
> per-SoC support driver for Cygnus anyway.

I assumed this one was the only odd one.

> > > > We recently merged nested irqdomain support as well, which might help here,
> > > > or might not be needed.
> > > 
> > > I'm not familiar with nested irqdomains. Do they address anything like
> > > the above problem?
> > 
> > The problem that nested irqdomains address is when an interrupt is handled
> > by two irqchips, in particular when one irqchip handles a virtual interrupt
> > number that was claimed by another irqchip already.
> 
> I'll do some reading on that, but it definitely doesn't address the main
> problem here.

Ok, back to the drawing board then: How about turning the probe order around
and splitting the SoC-specific part out into its own platform_driver:

Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a
bcm63138_nand_driver with its own probe() function that calls the
common probe function. That would make the soc specific parts
better contained and match how we normally do abstractions of
similar drivers.

	Arnd