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

[Kevin Cernekee]

On Mon, Nov 24, 2014 at 6:31 AM, Jonas Gorski <jogo@xxxxxxxxxxx> wrote:
> On Mon, Nov 24, 2014 at 3:40 AM, Kevin Cernekee <cernekee@xxxxxxxxx> wrote:
>> To date, all supported controllers have had the IRQEN register at offset
>> 0x00 and the IRQSTAT register at 0x04.  So in DT we would typically see
>> something like:
>>
>>     reg = <0xf0406800 0x8>;
>>
>> We still want to support this format, but we also need to support cases
>> where IRQEN and IRQSTAT aren't adjacent.  So, we will amend the driver to
>> accept an alternate format that looks like this:
>>
>>     reg = <0xf0406800 0x4 0xf0406804 0x4>;
>>
>> i.e. if the first resource_size() == 4, assume that the first resource
>> points to IRQEN and that the next resource points to IRQSTAT.  If the
>> first resource_size() == 8, retain the current behavior.
>>
>> Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxx>
>
> Hmm ... the more I think about this, the less I like it.
>
> Using the amount and size of the reg-properties to infer a certain
> layout seems rather hackish and dirty to me. Maybe we should just use
> different compatible match ids for that? E.g. brcm,bm7120-l2-intc for
> the 32-bit en/stat pairs, and e.g. brcm,bcm6368-l2-intc for the 64-bit
> wide one. Or maybe make the bcm63xx one a separate driver and let it
> share code with the bcm7120-l2-intc driver.

In my current proposal, it is easy to specify an arbitrary number of
enable/status pairs located in any order and spread across different
parts of the register space.  Even register indices (0,2,4,...) are
enable registers, and odd register indices are status registers.

If I'm interpreting your post correctly, you don't agree that we need
this level of flexibility.  But looking over the register listings,
here are the cases we need to cover:


6828,6318: 4x mask(exthi,extlo,hi,lo),status(exthi,extlo,hi,lo)

TP0ExtIrqMaskHi
TP0ExtIrqMaskLo
TP0IrqMaskHi
TP0IrqMaskLo
TP0ExtIrqStatusHi
TP0ExtIrqStatusLo
TP0IrqStatusHi
TP0IrqStatusLo

TP1ExtIrqMaskHi
TP1ExtIrqMaskLo
...


6816,6362,6328: 2x extmask,mask,extstatus,status

ExtraChipIrqMask
ChipIrqMask
ExtraChipIrqStatus
ChipIrqStatus
ExtraChipIrqMask1 [1=TP1]
ChipIrqMask1
ExtraChipIrqStatus1
ChipIrqStatus1


6368: similar to above, but with yet another naming convention:

IRQMASK_MIPS0_Hi
IRQMASK_MIPS0_Lo
IRQSTATUS_MIPS0_Hi
IRQSTATUS_MIPS0_Lo
IRQMASK_MIPS1_Hi
IRQMASK_MIPS1_Lo
IRQSTATUS_MIPS1_Hi
IRQSTATUS_MIPS1_Lo



6838,3384: interleaved "mystery meat" mask/status (same IRQ source
names, with the output of each bcm7120-l2 routed to several different
processors/threads):

PeriphIRQMASK0
PeriphIRQSTATUS0
PeriphIRQMASK1 <- mine, if running on Zephyr
PeriphIRQSTATUS1 <- mine, if running on Zephyr
PeriphIRQMASK2
PeriphIRQSTATUS2
PeriphIRQMASK3 <- mine, if running on Viper
PeriphIRQSTATUS3 <- mine, if running on Viper

But wait, there's more!  There wasn't enough space in this block for
>32 IRQ bits, so the upper bits spilled over into a separate
"INT_EXT_PER" block that lives elsewhere in the register space:

PeriphIRQMASK0_2
PeriphIRQSTATUS0_2
PeriphIRQMASK1_2 <- mine, if running on Zephyr
PeriphIRQSTATUS1_2 <- mine, if running on Zephyr
PeriphIRQMASK2_2
PeriphIRQSTATUS2_2
PeriphIRQMASK3_2 <- mine, if running on Viper
PeriphIRQSTATUS3_2 <- mine, if running on Viper

The "INT_PER" and "INT_EXT_PER" outputs are ORed into the same MIPS
IRQ lines, so we need to treat them as two sides of a single
controller.  AFAICT, unlike a shared device IRQ, there is no way to
share a MIPS IRQ line between two controller instances.

Additionally, we have a few other random MASK/STATUS pairs scattered
around (ZMIPS, CMIPS blocks), and then we also have DQM IRQs with
multiple mask registers + single status register:

CMIPS_NOT_EMPTY_IRQ_MSK
CMIPS1_NOT_EMPTY_IRQ_MSK <- mine, if running on Viper
ZMIPS_NOT_EMPTY_IRQ_MSK <- mine, if running on Zephyr
PMC_NOT_EMPTY_IRQ_MSK
DFAP_NOT_EMPTY_IRQ_MSK
NOT_EMPTY_IRQ_STS

I suppose another alternative is to ioremap() a range large enough to
cover enable + status, and then specify the offset of each one in
another property.  This does run the risk of overlapping mappings.


> This would avoid having to specify a lot of regs (let's assume we also
> add support for affinity)

I concede that I have no idea how affinity should be handled here.
AFAICT it is completely off limits on BCM3384 because we just don't
have enough L2 outputs to offer proper masking for all of the
threads/CPUs in the system.

Perhaps we could write a simple, SMP-capable driver for the
saner/newer SoCs, and use the flexible-but-non-SMP version for the
complicated ones.


> and cause a lot of io(re)map calls

Is the ioremap() call really that big of a deal?

On MIPS it's basically computing CKSEG1ADDR(phys_addr).  On ARM, the
top level (with 64 to 128+ IRQs) goes through the GIC.  On both,
ioremap() is a one-time cost at startup.
--
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/