Re: [PATCH v7 2/2] irqchip: Add Qualcomm MPM controller driver

[Shawn Guo]

On Fri, Mar 04, 2022 at 07:59:15AM +0000, Marc Zyngier wrote:
> On Thu, 03 Mar 2022 04:02:29 +0000,
> Shawn Guo <shawn.guo@xxxxxxxxxx> wrote:
> > 
> > On Wed, Mar 02, 2022 at 01:57:27PM +0000, Marc Zyngier wrote:
> > > This code actually makes me ask more questions. Why is it programming
> > > 2 'pins' for each IRQ?
> > 
> > The mapping between MPM pin and GIC IRQ is not strictly 1-1.  There are
> > some rare case that up to 2 MPM pins map to a single GIC IRQ, for
> > example the last two in QC2290 'qcom,mpm-pin-map' below.
> > 
> > 	qcom,mpm-pin-map = <2 275>,     /* tsens0_tsens_upper_lower_int */
> > 			   <5 296>,     /* lpass_irq_out_sdc */
> > 			   <12 422>,    /* b3_lfps_rxterm_irq */
> > 			   <24 79>,     /* bi_px_lpi_1_aoss_mx */
> > 			   <86 183>,    /* mpm_wake,spmi_m */
> > 			   <90 260>,    /* eud_p0_dpse_int_mx */
> > 			   <91 260>;    /* eud_p0_dmse_int_mx */
> > 
> > 
> > The downstream uses a DT bindings that specifies GIC hwirq number in
> > client device nodes.  In that case, d->hwirq in the driver is GIC IRQ
> > number, and the driver will need to query mapping table, find out the
> > possible 2 MPM pins, and set them up.
> > 
> > The patches I'm posting here use a different bindings that specifies MPM
> > pin instead in client device nodes.  Thus the driver can simply get the
> > MPM pin from d->hwirq, so that the whole look-up procedure can be saved.
> 
> It still remains that there is no 1:1 mapping between input and
> output, which is the rule #1 to be able to use a hierarchical setup.

For direction of MPM pin -> GIC interrupt, it's a 1:1 mapping, i.e. for
given MPM pin, there is only one GIC interrupt.  And that's the
mapping MPM driver relies on.  For GIC interrupt -> MPM pin, it's not
a strict 1:1 mapping.  For given GIC interrupt, there could be up to 2
MPM pin mapped to it.  MPM driver doesn't use this direction of mapping
though.

> 
> /me puzzled.
> 
> >
> > > 
> > > > 
> > > > It seems MPM_REG_POLARITY is only meant for level interrupts, since edge
> > > > interrupts already have separate registers for rising and falling.
> > > 
> > > Then level interrupts must clear both the edge registers at all times.
> > 
> > The downstream logic already covers that, right?  The edge register bits
> > will be cleared as long as 'flowtype' is not EDGE.
> 
> I am talking about *your* code, not the Qualcomm stuff.

OK. If you do not see anything wrong on the vendor code logic, I plan to
replace my broken code with it.

Shawn