Re: [RFC] regmap-irq: add "main register" and level-irq support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7512 bytes --]

On Wed, Dec 05, 2018 at 10:22:51AM +0200, Matti Vaittinen wrote:
> On Tue, Dec 04, 2018 at 05:21:37PM +0000, Mark Brown wrote:
> > On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote:

> > This sounds exactly like the wm831x which uses cascaded irqchips for
> > this, though not regmap-irq.

> Yep. I guess I could do cascaded chips - but I would like to use the
> regmap-irq for this driver. Maybe we could change regmap-irq so that it
> would be possible to not give the DT node from regmap for this chip?
> This was actually my first thought - but then I felt that having two irq
> chips for one device is a bit hacky - and hence I decided to see how
> regmap-irq could be changed to support the 'main irq status' register
> usage. I think this 'main register' setup is pretty common design.

I'm not sure I see it as particularly hacky - it's using features that
the frameworks provide to do the fan out, that just seems like making
use of existing framework features.

> > I suspect the idiomatic way to handle this in DT is to make DT nodes
> > (and devices) for the subfunction interrupt controllers and expose the
> > internal structure of the chip to the DT.

> Yes. This would be one approach - but I am not sure how DT guys see
> this. I may be wrong but I have a feeling Rob prefers having only one DT
> node describing single device. But for me it would make sense to have
> own node for GPIO - especially because the GPIO is only IRQ controller

Well, it kind of depends if you see the sub-interrupt controllers as
independent devices or not (at the hardware level they will be).

> which really is visible outside the chip. But here we still hit the same
> problem if we want to use regmap-irq. Regmap-irq still knows only the
> i2c device DT node. Currently there is no way to tell regmap-irq to use
> the sub-node.

If they're full subdevices you'd be pointing at the relevant platform
device anyway.

> >  This does make some sense as
> > the individual interrupt controllers within the chip are clearly
> > reusable IP blocks though it means that the chip starts looking a bit
> > weird externally so perhaps that's not ideal and your suggestion makes
> > sense.

> So I take this as a suggestion to continue tinkering with the 'main irq
> register support'. Or how do you see my suggestion of making iot
> possible to instruct the regmap-irq to not use DT for 'main irq
> controller'? Then we could probably do cascaded controllers where only
> the controller handling the externally visible 'sub-irqs' would be
> bound to DT? The pitfall here is case where more than one sub-irq
> controllers needs to be exposed to other devices. In that sense the
> 'main irq thing' would have better 'case coverage' =) (Wow, what a fancy
> words, maybe I am turning into a manager here :p )

I'm on the fence about the main controller idea.

> > > + * @main_status: Base main status register address. For chips which have
> > > + *		 interrupts arranged in separate sub-irq blocks with own IRQ
> > > + *		 registers and which have a main IRQ registers indicating
> > > + *		 sub-irq blocks with unhandled interrupts. For such chips fill
> > > + *		 sub-irq register information in status_base, mask_base and
> > > + *		 ack_base.
> > > + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq
> > > + *		 registers. First item in array describes the registers
> > > + *		 for first main status bit. Second array for second bit etc.
> > > + *		 Offset is given as sub register status offset to status_base.
> > > + *		 Should contain num_regs arrays. Can be provided for chips with
> > > + *		 more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to
> > > + *		 2.nd sub-reg, ...

> > This interface feels a little awkward in that we define the sub
> > interrupts in this separate array and require them all to be in separate
> > single registers which probably won't be true in general (IIRC it wasn't
> > how the wm831x worked).

> So my implementation here was just the simplest solution to 'glue' the
> main status register on top of existing implementation. If we do it this
> way, then the 'main status register' is completely optional - chip would
> work just fine even without the 'main register' - main register only
> saves some reads as subregisters with no irqs can be left unread. But
> you are correct - this only supports limited amount of HWs. Many chips I
> have seen (prior to the one I am now working with) actually require
> acking also the main status register. They also may support masking the
> main status register. But I guess trying to support all such cases would
> make the regmap-irq really complex and hard to understand.

Right, it's that sort of thing that makes me want to just cascade the
interrupt controllers - at some point you just end up with as fully
featured an interrupt controller distributing to other fully featured
interrupt controllers.

> > How about instead we have the individual
> > interrupts mark which main status bits flag them, then build up a
> > mapping in the other direction during init of subregisters to read?

> I am not sure if I understand your idea correctly - but for me it feels
> that the 'main status register' is only benefical when we can do direct
> mapping from main register bit/value to sub-register(s). 

The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware
gets structured is that the central interrupt controller is saying which
IP block is flagging an interrupt rather than which register has an
interrupt set in it.  You can then have either more than one detailed
status register for that IP or several smaller IPs reporting through a
single detailed status register.  I've also seen some interrupts
represented directly in the main status if there's some need for a fast
path.

> But as we must really read whole sub register at once, it really does
> not matter which interrupts inside the sub register did activate the
> main status - we go through all bits in the sub register anyways. Hence
> it may not be needed to have mapping from individual interrupts to the
> main status register value? I guess looping through all the sub
> register bits is negligible source of latency compared to the time the
> register access takes. So identifying the sub register(s) based on main
> status is the thing - not mapping the individual interrupts in sub
> register(s), right? I think that having to specify the main status
> register value for each interrupt would be quite a overkill.

Right, it's about working out which subregister to read - the point is
you can do this by adding a link in either direction, I'm suggesting
doing it from the individual interrupt to the main register since we
already have individual data structures for those and it feels less
likely to run into hard to represent corner cases.

>  * @main_status: Optional base main status register address. Some chips have
>  *		 "main status register(s)" which indicate actual irq registers
>  *		 with unhandled interrupts. Provide main status register
>  *		 address here to avoid reading irq registers with no
>  *		 active interrupts. Mapping from main status value to
>  *		 interrupt register can be provided in sub_reg_offsets.

> Perhaps this would clarify that the status_base, mask_base, ack_base,
> etc. should still be used normally?

Probably.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]