Re: [PATCH V3 06/11] irqchip: bcm7120-l2: Change DT binding to allow non-contiguous IRQEN/IRQSTAT

[Jonas Gorski]

On Wed, Nov 26, 2014 at 10:45 AM, Jonas Gorski <jogo@xxxxxxxxxxx> wrote:
> As far as I can see, we have three distinct layouts here:
>
> a) An arbitrary number of 32-bit Mask/Status-pairs (3384/6838). No per
> thread support (well, not sure about 60333).
>
> b) An arbitrary length (32 to 128 bit) Mask register, followed by a
> same length Status register (63xx except 63381, 6818, 6828); repeated
> for each thread.
>
> c) A single arbitrary length (currently only 128 bit) Status register,
> followed by per thread same length Mask registers (63381).
>
> On a first glance this could translate to three distinct
> drivers/compatible properties, where each expects different reg = <>;
> contents.
>
> For a), it should be enough to expand the current 7120-l2 driver to
> accept/use more than one 0x8 length register, which should simplify
> the register map setup.
>
> For b) we could add a a new compatible name (maybe bcm6358-l2, because
> that was AFAICT the first one with blocks) that will use the 8 to 32
> byte length regs (one for each block). For now we could ignore the SMP
> capability of it and make it a variant of the 7120-l2 driver, and when
> we add SMP support, split it into a second different driver if we want
> to avoid having all the spinlock for register accesses even for a).
>
> We could then even easily document/add the extra block registers /
> interrrupts in documentation / the dtsi files before actually
> supporting them, because we only have a fixed amount of regs/irqs to
> expect in the !SMP case and can easily ignore the extra
> registers/interrupts.
>
> For c) we could add a a third compatible name (bcm63381-l2), also with
> its own setup routine. I would guess it doesn't matter if both
> thread's irqstatus register pointers point to the same region.

This split-up is especially tempting to me after I had a closer look
at the current 7120-l2 driver, which already accepts more than one
interrupt, but uses it in a different way. So even if we try to make
it very flexible with only one compatible property,

   reg = <irqstatus0 irqmask0 irqstatus1 irqmask1>;
   interrupts = <irq0>, <irq1>;

Could then mean either irq0 is for interrupts 0..31 (mask/status0) and
irq1 for interrupts 32 .. 64 (mask/status1), or irq0 is for interrupts
0..31 on cpu0, and irq1 is for interrupts 0..31 on cpu1, and then
would require an additional property to tell them apart, for which we
then also could just use a different compatible name, and have (IMHO)
a lot less headache.  (I wonder why we couldn't just have had more
than one instance of 7120-l2 in the dts for the first case)


Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/