Re: [PATCH v4 09/14] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>, Andrew Lunn <andrew@lunn.ch>,
	Jason Cooper <jason@lakedaemon.net>,
	devicetree@vger.kernel.org,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Haim Boot <hayim@marvell.com>, Will Deacon <will.deacon@arm.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	Nadav Haklai <nadavh@marvell.com>,
	Rob Herring <robh+dt@kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Hanna Hawa <hannah@marvell.com>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH v4 09/14] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)
Date: Tue, 21 Aug 2018 12:28:19 +0200	[thread overview]
Message-ID: <20180821122819.726c434b@xps13> (raw)
In-Reply-To: <07e90507-1d51-6435-b006-c46f865a636c@arm.com>

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04
+0100:

> On 21/08/18 10:08, Miquel Raynal wrote:
> > Hi Marc,
> > 
> > I'm fine with the rest of the comments, please find just one last
> > question below.
> > 
> > [...]
> >   
> >>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> >>>  		return -EINVAL;
> >>>  	}
> >>>  
> >>> -	/* Mask the type to prevent wrong DT configuration */
> >>> -	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> >>> +	/*
> >>> +	 * The ICU receives level-interrupts. MSI SEI are
> >>> +	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
> >>> +	 * accordingly for the parent irqchip.
> >>> +	 */
> >>> +	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
> >>> +		*type = IRQ_TYPE_EDGE_RISING;    
> >>
> >> That's interesting. How is the resampling done here?  
> > 
> > I'm not sure to understand the question. What does 'resampling' means
> > in such context? MSI SEIs are of type "edge" and use the traditional
> > MSI signalling infrastructure. I'm asking to be sure not to ignore
> > something wrong in my code.  
> 
> You seems to be turning a level interrupt into an edge.

If is an SEI interrupt, it cannot be a level interrupt and the type
will be IRQ_TYPE_EDGE_RISING.

> You can do that,
> but only if you resample the level on EOI (and regenerate the
> corresponding edge if the level is still high).

What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory,
*type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But,
as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING.
I thought more clear to enforce it but if this unclear and useless,
let's drop it?

> 
> Or am I reading it the wrong way?

No, that's probably me not understanding correctly the purpose of the
above function.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel