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

[Matti Vaittinen]

Hello Again Mark,

On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote:
> 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.

I am not sure if I have explained me well enough =) When we look at
regmap_add_irq_chip we see that it creates the IRQ domains. And we
further see:

		d->domain = irq_domain_add_linear(map->dev->of_node,
						  chip->num_irqs,
						  &regmap_domain_ops, d);

where map->dev->of_node points the node added to regmap. And as we
really want to use the i2c to access registers, we should have done the
regmap using:

devm_regmap_init_i2c(i2c, &regmap);


where the i2c is the struct i2c_client *. The dev in i2c_client is the
i2c device - which has of_node pointing at the "main i2c node" - not the
sub block nodes. Hence all irq chips created by regmap_add_irq_chip for
this physical i2c slave device will point to the same DT 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 )
> 
> I'm on the fence about the main controller idea

So am I. But I still would like to use regmap-irq no matter if I create
one or many irq chips. So please allow me to present the other idea:

Would it work if we add someting like

struct regmap_irq_chip {
	const char *name;

	unsigned int status_base;
	unsigned int mask_base;
	unsigned int unmask_base;
	unsigned int ack_base;
	unsigned int wake_base;
	unsigned int type_base;
	unsigned int irq_reg_stride;
	bool mask_writeonly:1;
+	bool no_of:1;

and in the regmap_add_irq_chip do:

		node = (chip->no_of) ? NULL : map->dev->of_node;
		d->domain = irq_domain_add_linear(node, chip->num_irqs,
						  &regmap_domain_ops, d);

Then we could have chip->no_of set for the 'main irq chip' and for those
chips we don't wan't to expose via DT. In my case I would leave no_of
unset only for the irq-chip which I created for the GPIO? Is this a
silly 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.

Yes. Question is whether we should support getting rid of extra irq
register reads if HW provides the 'main status' register. I would not
support main irq acking/masking (at least not for now).

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

Correct. But I guess the IP blocks often have limited set of registers for the
"sub interrupts". For such cases my idea would work, right. My RFC
handled case where there is many 'sub registers' to read for one 'main
bit.

> or several smaller IPs reporting through a
> single detailed status register.

I think that if we have more than two layers of irqs (main and sub) -
then we should do cascaded controllers.

>  I've also seen some interrupts
> represented directly in the main status if there's some need for a fast
> path.

Right. This is not covered by my 'RFC'. And here the flagging from
individual irq to main status bit would be usefull. But I am a bit
afraid of complexity supporting this adds - as well as the overhead of
describing 'main status bits' for all interrupts in cases where we don't
have such fast-path bits. Besides, adding the main status handling does
not force one to use it if existing mechanisms work well =)

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

I see your point now. But as I said, I am not sure we should add the
overhead of 'main irq bit description' for simple cases just to cover
the corner cases. Yet I can try seeing what I can come up with if you
think this is the way to go.

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

I am not sure this means I was able to convince you or if this is the
polite way to tell me not to bother =) I am Finnish guy who does not
understand small talk or polite hints ;)

Best Regards
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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