Message-ID: <CAMxfBF6Th+zKOmogA5phkh21tSUzutokCgU+pv0Eh-sDk=1Hbg@mail.gmail.com>
Date:   Wed, 8 Jul 2020 09:04:03 +0200
From:   Grzegorz Jaszczyk <grzegorz.jaszczyk@...aro.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     tglx@...utronix.de, jason@...edaemon.net,
        "Anna, Suman" <s-anna@...com>, robh+dt@...nel.org,
        Lee Jones <lee.jones@...aro.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, david@...hnology.com,
        "Mills, William" <wmills@...com>, "Andrew F . Davis" <afd@...com>,
        Roger Quadros <rogerq@...com>
Subject: Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver
 for PRUSS interrupts

On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@...nel.org> wrote:
>
> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:
> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@...nel.org> wrote:
> >>
> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote:
>
> [...]
>
> >> It still begs the question: if the HW can support both edge and level
> >> triggered interrupts, why isn't the driver supporting this diversity?
> >> I appreciate that your HW may only have level interrupts so far, but
> >> what guarantees that this will forever be true? It would imply a
> >> change
> >> in the DT binding, which isn't desirable.
> >
> > Ok, I've got your point. I will try to come up with something later
> > on. Probably extending interrupt-cells by one and passing interrupt
> > type will be enough for now. Extending this driver to actually support
> > it can be handled later if needed. Hope it works for you.
>
> Writing a set_type callback to deal with this should be pretty easy.
> Don't delay doing the right thing.

Ok.

>
> [...]
>
> >> >> > +             hwirq = hipir & GENMASK(9, 0);
> >> >> > +             virq = irq_linear_revmap(intc->domain, hwirq);
> >> >>
> >> >> And this is where I worry. You seems to have a single irqdomain
> >> >> for all the muxes. Are you guaranteed that you will have no
> >> >> overlap between muxes? And please use irq_find_mapping(), as
> >> >> I have top-secret plans to kill irq_linear_revmap().
> >> >
> >> > Regarding irq_find_mapping - sure.
> >> >
> >> > Regarding irqdomains:
> >> > It is a single irqdomain since the hwirq (system event) can be mapped
> >> > to different irq_host (muxes). Patch #6
> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how input
> >> > events can be mapped to some output host interrupts through 2 levels
> >> > of many-to-one mapping i.e. events to channel mapping and channels to
> >> > host interrupts. Mentioned implementation ensures that specific system
> >> > event (hwirq) can be mapped through PRUSS specific channel into a
> >> > single host interrupt.
> >>
> >> Patch #6 is a nightmare of its own, and I haven't fully groked it yet.
> >> Also, this driver seems to totally ignore the 2-level routing. Where
> >> is it set up? map/unmap in this driver do exactly *nothing*, so
> >> something somewhere must set it up.
> >
> > The map/unmap is updated in patch #6 and it deals with those 2-level
> > routing setup. Map is responsible for programming the Channel Map
> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on
> > provided configuration from the one parsed in the xlate function.
> > Unmap undo whatever was done on the map. More details can be found in
> > patch #6.
> >
> > Maybe it would be better to squash patch #6 with this one so it would
> > be less confusing. What is your advice?
>
> So am I right in understanding that without patch #6, this driver does
> exactly nothing? If so, it has been a waste of review time.
>
> Please split patch #6 so that this driver does something useful
> for Linux, without any of the PRU interrupt routing stuff. I want
> to see a Linux-only driver that works and doesn't rely on any other
> exotic feature.
>

Patch #6 provides PRU specific 2-level routing setup. This step is
required and it is part of the entire patch-set. Theoretically routing
setup could be done by other platform driver (not irq one) or e.g. by
PRU firmware. In such case this driver would be functional without
patch #6 but I do not think it would be proper. All this routing setup
is done via PRUSS INTC unit and uses PRUSS INTC register set,
therefore delegating it to another driver doesn't seem to be the best
option. Furthermore delegating this step to PRU firmware is also
problematic. First of all the PRU core tiny Instruction RAM space
makes it difficult to fit it together with the code that is required
for running a PRU specific application. Another issue that I see is
splitting management of the PRUSS INTC unit to Linux (main CPU) and
PRU firmware (PRU core).

I am also not sure if splitting patch #6 makes sense. Mentioned patch
allows to perform the entire 2-level routing setup. There is no
distinction between routing setup for main CPU and PRU core, both use
the same logic.The only difference between setting up the routing for
main CPU (Linux) and PRU core is choosing different, so called, "host
interrupt" in final level mapping.

Discussion about previous approach of handling this 2-level routing
setup can be found in v2 of this patch-set
(https://patchwork.kernel.org/patch/11069751/) - mentioned approach
wasn't good and was dropped but problem description made by Suman may
be useful.

I am open to any suggestion if there is a better way of handling
2-level routing. I will also appreciate if you could elaborate about
issues that you see with patch #6.

Best regards,
Grzegorz

Powered by blists - more mailing lists