Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAE3Pknn88NZNqiaHahM4HVvTU82hdMcbhGWWb4AyXXYMbcGuaQ@mail.gmail.com>
 <3ebbe879471898218f34918b1d009887@kernel.org> <CAE3PknnmUBPfCjJUxwLaTjAqU4tYXxakvH+qZTwYYyw4SS_DiA@mail.gmail.com>
 <20200403130327.6d961882@why> <CAE3Pknkq3+B=z32ZRsQS1aM1+EqGnysMY4pitFdpOKGmv4ax8Q@mail.gmail.com>
In-Reply-To: <CAE3Pknkq3+B=z32ZRsQS1aM1+EqGnysMY4pitFdpOKGmv4ax8Q@mail.gmail.com>
From:   =?UTF-8?Q?Lu=C3=ADs_Matallui?= <matallui@gmail.com>
Date:   Fri, 3 Apr 2020 15:46:23 -0600
Message-ID: <CAE3Pkn=e5FgvKAxMXWHSPDM7GruSq-udFsb0aAsCQvXs65o8Sg@mail.gmail.com>
Subject: Re: Help with IRQ-MSI-IRQ bridges
To:     Marc Zyngier <maz@kernel.org>
Cc:     linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hey Marc,

Just wanted to close this thread and thank you for the help.
Using that handle_fasteoi_ack_irq() saved my life.

I was just having trouble unrelated to this, but some poorly documented SoC=
.

Thanks again!
Luis

On Fri, 3 Apr 2020 at 07:26, Lu=C3=ADs Matallui <matallui@gmail.com> wrote:
>
> On Fri, 3 Apr 2020 at 06:03, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 3 Apr 2020 05:13:35 -0600
> > Lu=C3=ADs Matallui <matallui@gmail.com> wrote:
> >
> > > Hi Marc,
> > >
> > > On Fri, 3 Apr 2020 at 04:18, Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Hi Luis,
> > > >
> > > > On 2020-04-03 02:35, Lu=C3=ADs Matallui wrote:
> > > > > Hi,
> > > > >
> > > > > I've got this SoC which uses IRQ-MSI and MSI-IRQ bridges in order=
 to
> > > > > get interrupts from devices external to the ARM subsystem.
> > > > > I already got some pointers from Maz and have been able to create=
 the
> > > > > drivers with the stacked domains and can now see the mappings wor=
king
> > > > > fine across domains.
> > > > >
> > > > > Maz pointed me to the Marvell mvebu-gicp (for my MSI controller, =
a.k.a
> > > > > MSI-IRQ bridge) and to mvebu-icu for the MSI client (IRQ-MSI brid=
ge).
> > > > >
> > > > > I now have the interrupts working, but it seems like I'm missing =
a
> > > > > bunch of them. And therefore my device doesn't work properly.
> > > > > The main difference between my HW and Marvell's is that my IRQs a=
re
> > > > > not level-triggered and the MSIs don't support the two messages f=
or
> > > > > level-triggered interrupts.
> > > >
> > > > Which is probably a very good thing, as long as all your devices
> > > > generate
> > > > only edge-triggered interrupts.
> > > >
> > > > >
> > > > > To illustrate my system:
> > > > >
> > > > > DEV --line--> IRQ-MSI Bridge (MSIC) --msi--> MSI-IRQ Bridge (GICP=
)
> > > > > --line--> GICv2
> > > > >
> > > > > For MSIC, all I can do is configure the address and data for the =
MSI,
> > > > > and I believe on every rising edge of the Device IRQ, an MSI is s=
ent.
> > > > > For GICP, all I have is a doorbell and a way to enable/disable it=
, and
> > > > > whenever the doorbell is enabled and has a value !=3D 0, the IRQ =
line to
> > > > > GICv2 gets asserted.
> > > > >
> > > > > The first thing I noticed is that when I get an interrupt, the IR=
Q
> > > > > flow goes like:
> > > > >
> > > > >   handle_irq();
> > > > >   irq_eoi();
> > > > >
> > > > > So, I guess my first question here is, how can I guarantee that I
> > > > > don't get another MSI whilst in handle_irq()?
> > > >
> > > > At the GIC level, once the interrupt is Ack'd, anything that is sig=
ned
> > > > after this ack is a separate interrupt. It will be made pending and=
 will
> > > > fire once the GIC driver EOIs the first one.
> > >
> > > The thing here is, there is no Ack, or at least my irqchips are not g=
etting
> > > the irq_ack() callback, which is where I was expecting to clear the d=
oorbell.
> >
> > Not getting an irq_ack() here is expected, as the GIC uses the fast_eoi
> > flow, which doesn't use irq_ack() at all. If you really want irq_ack()
> > to be called, you'll need to change that flow for the specified
> > interrupts (handle_fasteoi_ack_irq is probably of interest).
> >
>
> I had tried level and trigger flow handlers and was always getting weird =
hangs,
> so never even attempted that, but with that handler I do get the Ack call=
s now.
>
> I'm still missing the interrupts, so I'm starting to believe I'm just
> doing something
> else wrong.
>
> > >
> > > >
> > > > > If I do, then I will clear the doorbell on irq_eoi() (because tha=
t's
> > > > > my only choice) and will lose the queued IRQs.
> > > >
> > > > Why do you need to do anything at the doorbell level? This is just =
a
> > > > write,
> > > > so there should be nothing to clear. If you do need to clear anythi=
ng,
> > > > then your MSI-IRQ bridge isn't stateless as it should, and you'll n=
eed
> > > > to
> > > > give much more details about the HW. Do you have a pointer to the T=
RM
> > > > for your HW?
> > >
> > > The hardware is really simple. On the MSI controller (GICP) side, eac=
h
> > > interrupt only has 3 registers: 1 status, 1 mask and 1 clear. When an
> > > MSI lands a write on the status register, it asserts the interrupt li=
ne.
> > > The interrupt stays asserted until we clear the status (using the cle=
ar
> > > register). The mask register is just to enable the interrupt basicall=
y.
> > > The MSI data is really irrelevant, as long as it's non-zero we always
> > > obtain the same result.
> >
> > Does it mean it asserts a level each time it gets an edge, and you need
> > to clear the MSI to allow another one? If so, that's a bit silly. it
> > would have made a lot more sense to leave it flowing to the GIC where
> > all the logic is already present.
>
> Yes! I totally agree this is silly, but I'm pretty sure that's how it
> works. I will
> try to reach out to the SoC team to double check, but from their document=
ation,
> they say the interrupt line will stay asserted as long as the doorbell st=
atus
> value is different than 0, so until we manually clear it, it stays assert=
ed.
>
> > >
> > > On the MSI client side, we only configure the MSI address and data fo=
r
> > > a certain device interrupt line, and for each rising edge, an MSI get=
s issued.
> > >
> > > >
> > > > > It also seems that I'm missing IRQs in the beginning after probin=
g the
> > > > > device, and before it was working for me when I was setting up al=
l
> > > > > these registers manually and simply using GICv2 as my only interr=
upt
> > > > > controller.
> > > >
> > > > Well, setting all of this in firmware is always the preferred optio=
n
> > > > if you don't expect things to change dynamically.
> > >
> > > Well, the solution I have now works perfectly for the configuration, =
because
> > > the MSIC gets configured by msi_compose_msg -> msi_write_msg at IRQ
> > > allocation time and never gets touched again.
> > >
> > > Then when the IRQ gets activated, the GICP is unmasking the interrupt=
 but
> > > enabling the doorbell (setting the mask register).
> > >
> > > The only thing I really need is to intercept every MSI before the han=
dler so
> > > I can Ack it by clearing the doorbell status register.
> >
> > See above.
> >
> > > > > I do see the unmask() ops being called for all my stacked irqchip=
s, so
> > > > > I don't understand how I'm missing so many interrupts.
> > > >
> > > > unmask is just a static configuration to enable the interrupt. Ther=
e
> > > > shouldn't
> > > > be that many calls to that later on unless an endpoint driver
> > > > disables/enables
> > > > interrupts by hand.
> > > >
> > > > Please give us a bit more details to understand the context, as the=
re is
> > > > only
> > > > so much I can do with so little HW information.
> > > >
> > > > Thanks,
> > > >
> > > >          M.
> > > > --
> > > > Jazz is not dead. It just smells funny...
> > >
> > > Let me know if that is good enough information. There's really not mu=
ch on
> > > the HW side.
> >
> > If I'm correct above, I'd say there is a bit too much there! ;-)
> >
> >         M.
> > --
> > Jazz is not dead. It just smells funny...
>
> Thanks again Marc!
> Let me try to reach out to the SoC guys and try to figure out what
> else I'm doing
> wrong. I will give you an update once I figure it out.
>
> -- Luis