From: Marc Zyngier <maz@kernel.org> To: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.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: Sun, 05 Jul 2020 21:45:36 +0100 [thread overview] Message-ID: <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> (raw) In-Reply-To: <CAMxfBF7pbH1LLE4fJnnCPnrqnQ-tdO+_xfoN1VerJcQ-ZyYM9Q@mail.gmail.com> 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. [...] >> >> > + 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. M. -- Jazz is not dead. It just smells funny...
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> Cc: devicetree@vger.kernel.org, Roger Quadros <rogerq@ti.com>, linux-omap@vger.kernel.org, jason@lakedaemon.net, linux-kernel@vger.kernel.org, "Andrew F . Davis" <afd@ti.com>, robh+dt@kernel.org, tglx@linutronix.de, Lee Jones <lee.jones@linaro.org>, "Mills, William" <wmills@ti.com>, linux-arm-kernel@lists.infradead.org, david@lechnology.com Subject: Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts Date: Sun, 05 Jul 2020 21:45:36 +0100 [thread overview] Message-ID: <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> (raw) In-Reply-To: <CAMxfBF7pbH1LLE4fJnnCPnrqnQ-tdO+_xfoN1VerJcQ-ZyYM9Q@mail.gmail.com> 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. [...] >> >> > + 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. M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-07-05 20:45 UTC|newest] Thread overview: 72+ 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 ` Grzegorz Jaszczyk 2020-07-02 14:17 ` [PATCHv3 1/6] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings Grzegorz Jaszczyk 2020-07-02 14:17 ` Grzegorz Jaszczyk 2020-07-13 21:25 ` Rob Herring 2020-07-13 21:25 ` Rob Herring 2020-07-16 9:25 ` Grzegorz Jaszczyk 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 14:17 ` Grzegorz Jaszczyk 2020-07-02 17:24 ` Marc Zyngier 2020-07-02 17:24 ` Marc Zyngier 2020-07-03 14:28 ` Grzegorz Jaszczyk 2020-07-03 14:28 ` Grzegorz Jaszczyk 2020-07-04 9:39 ` Marc Zyngier 2020-07-04 9:39 ` Marc Zyngier 2020-07-05 13:26 ` Grzegorz Jaszczyk 2020-07-05 13:26 ` Grzegorz Jaszczyk 2020-07-05 20:45 ` Marc Zyngier [this message] 2020-07-05 20:45 ` Marc Zyngier 2020-07-08 7:04 ` Grzegorz Jaszczyk 2020-07-08 7:04 ` Grzegorz Jaszczyk 2020-07-08 10:47 ` Marc Zyngier 2020-07-08 10:47 ` Marc Zyngier 2020-07-10 23:03 ` Suman Anna 2020-07-10 23:03 ` Suman Anna 2020-07-15 13:38 ` Grzegorz Jaszczyk 2020-07-15 13:38 ` Grzegorz Jaszczyk 2020-07-17 12:36 ` Marc Zyngier 2020-07-17 12:36 ` Marc Zyngier 2020-07-21 9:27 ` Grzegorz Jaszczyk 2020-07-21 9:27 ` Grzegorz Jaszczyk 2020-07-21 10:10 ` Marc Zyngier 2020-07-21 10:10 ` Marc Zyngier 2020-07-21 13:59 ` Grzegorz Jaszczyk 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 14:17 ` Grzegorz Jaszczyk 2020-07-02 17:44 ` Marc Zyngier 2020-07-02 17:44 ` Marc Zyngier 2020-07-10 20:59 ` Suman Anna 2020-07-10 20:59 ` Suman Anna 2020-07-17 11:02 ` Marc Zyngier 2020-07-17 11:02 ` Marc Zyngier 2020-07-25 15:57 ` Suman Anna 2020-07-25 15:57 ` Suman Anna 2020-07-25 16:27 ` Marc Zyngier 2020-07-25 16:27 ` Marc Zyngier 2020-07-25 16:39 ` Suman Anna 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 14:17 ` [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get, set}_irqchip_state ops Grzegorz Jaszczyk 2020-07-02 17:54 ` [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Marc Zyngier 2020-07-02 17:54 ` Marc Zyngier 2020-07-03 17:04 ` Grzegorz Jaszczyk 2020-07-03 17:04 ` Grzegorz Jaszczyk 2020-07-10 21:04 ` Suman Anna 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 14:17 ` Grzegorz Jaszczyk 2020-07-02 17:59 ` Marc Zyngier 2020-07-02 17:59 ` Marc Zyngier 2020-07-03 17:05 ` Grzegorz Jaszczyk 2020-07-03 17:05 ` Grzegorz Jaszczyk 2020-07-10 21:13 ` Suman Anna 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 14:17 ` Grzegorz Jaszczyk 2020-07-02 16:24 ` Suman Anna 2020-07-02 16:24 ` Suman Anna 2020-07-05 13:39 ` Grzegorz Jaszczyk 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=53d39d8fbd63c6638dbf0584c7016ee0@kernel.org \ --to=maz@kernel.org \ --cc=afd@ti.com \ --cc=david@lechnology.com \ --cc=devicetree@vger.kernel.org \ --cc=grzegorz.jaszczyk@linaro.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=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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.