Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
From: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
To: Marc Zyngier <maz@kernel.org>
Cc: tglx@linutronix.de, jason@lakedaemon.net, "Anna,
	Suman" <s-anna@ti.com>,
	robh+dt@kernel.org, Lee Jones <lee.jones@linaro.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	david@lechnology.com, "Mills, William" <wmills@ti.com>,
	"Andrew F . Davis" <afd@ti.com>, Roger Quadros <rogerq@ti.com>
Subject: Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
Date: Wed, 8 Jul 2020 09:04:03 +0200
Message-ID: <CAMxfBF6Th+zKOmogA5phkh21tSUzutokCgU+pv0Eh-sDk=1Hbg@mail.gmail.com> (raw)
In-Reply-To: <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org>

On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote:
> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@kernel.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

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02 14:17 [PATCHv3 0/6] Add TI PRUSS Local Interrupt Controller IRQChip driver Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 1/6] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings Grzegorz Jaszczyk
2020-07-13 21:25   ` Rob Herring
2020-07-16  9:25     ` Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts Grzegorz Jaszczyk
2020-07-02 17:24   ` Marc Zyngier
2020-07-03 14:28     ` Grzegorz Jaszczyk
2020-07-04  9:39       ` Marc Zyngier
2020-07-05 13:26         ` Grzegorz Jaszczyk
2020-07-05 20:45           ` Marc Zyngier
2020-07-08  7:04             ` Grzegorz Jaszczyk [this message]
2020-07-08 10:47               ` Marc Zyngier
2020-07-10 23:03                 ` Suman Anna
2020-07-15 13:38                   ` Grzegorz Jaszczyk
2020-07-17 12:36                     ` Marc Zyngier
2020-07-21  9:27                       ` Grzegorz Jaszczyk
2020-07-21 10:10                         ` Marc Zyngier
2020-07-21 13:59                           ` Grzegorz Jaszczyk
2020-07-02 14:17 ` [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts Grzegorz Jaszczyk
2020-07-02 17:44   ` Marc Zyngier
2020-07-10 20:59     ` Suman Anna
2020-07-17 11:02       ` Marc Zyngier
2020-07-25 15:57         ` Suman Anna
2020-07-25 16:27           ` Marc Zyngier
2020-07-25 16:39             ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Grzegorz Jaszczyk
2020-07-02 17:54   ` Marc Zyngier
2020-07-03 17:04     ` Grzegorz Jaszczyk
2020-07-10 21:04       ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 5/6] irqchip/irq-pruss-intc: Add support for ICSSG INTC on K3 SoCs Grzegorz Jaszczyk
2020-07-02 17:59   ` Marc Zyngier
2020-07-03 17:05     ` Grzegorz Jaszczyk
2020-07-10 21:13       ` Suman Anna
2020-07-02 14:17 ` [PATCHv3 6/6] irqchip/irq-pruss-intc: Add event mapping support Grzegorz Jaszczyk
2020-07-02 16:24   ` Suman Anna
2020-07-05 13:39     ` Grzegorz Jaszczyk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMxfBF6Th+zKOmogA5phkh21tSUzutokCgU+pv0Eh-sDk=1Hbg@mail.gmail.com' \
    --to=grzegorz.jaszczyk@linaro.org \
    --cc=afd@ti.com \
    --cc=david@lechnology.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jason@lakedaemon.net \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=rogerq@ti.com \
    --cc=s-anna@ti.com \
    --cc=tglx@linutronix.de \
    --cc=wmills@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git