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

[Matti Vaittinen]

Hello Mark,

Thank you for taking the time and checking this! Highly appreciated!

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:
> 
> > IRQ handling is implemented so that each sub block has own
> > status/ack/mask registers and then there is one main status
> > register. When any of the sub blocks gets unmasked IRQ,
> > corresponding 'main status register' bit is set in main status
> > register.
> 
> 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.

> > Problem I faced is that I don't know how to create two regmap-irq chips for
> > the same i2c decvice - because the regmap-irq gets the DT node from regmnap
> > - and thus both of the irq chips would have same DT node. My assumption is
> > that the DT - xlate functionality for this setup would not be quite correct,
> > right? I guess the "main irq" interrupts would be hookable via DT? But
> > the devices are interested in "sub IRQs". So I thought that the correct
> > solution would be to model the interrupts from this PMIC as just one
> > irq-chip. So I wonder if addin this "main IRQ register" support for
> 
> 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
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.

>  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 )

> > configuring IRQ pins as level triggered. So I thought we could add
> > support for setting the type to level_low or level_high. This change
> > would require adding 'supported_types' information to all drivers which
> > are currently using regmap-irq for supporting type-setting - but quick
> > grep for drivers allow me to think that it is currently only the 
> > gpio-max77620 which uses the type setting in-tree.
> 
> That seems pluasible but definitely seems like a separate change.

All right. I will create a separate patch (set) out of this.

> 
> > + * @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.

> 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). 

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.

So what do you think, would changing the
> > + * @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, ...
to

@sub_reg_offsets: arrays of mappings from main register _value_ to to
		  sub irq registers ...

be the (simplest) way to go? And maybe changing the description for
main_status to:

 * @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?

Best Regards
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~