Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
From: Suman Anna <s-anna@ti.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>,
	<tglx@linutronix.de>, <jason@lakedaemon.net>,
	<robh+dt@kernel.org>, <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>,
	<wmills@ti.com>
Subject: Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
Date: Sat, 25 Jul 2020 11:39:28 -0500
Message-ID: <0a5664c1-88af-072e-7ad2-6a0b28fce28f@ti.com> (raw)
In-Reply-To: <1cd0b6c9bfe2dc42e9c6b69baacf4635@kernel.org>

Hi Marc,

On 7/25/20 11:27 AM, Marc Zyngier wrote:
> On 2020-07-25 16:57, Suman Anna wrote:
> 
> Suman,
> 
>> Hi Marc,
> 
> [...]
> 
>>>>>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
>>>>>> platform_device *pdev)
>>>>>>           return -ENOMEM;
>>>>>>
>>>>>>       for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>>>>>> +        if (intc->invalid_intr & BIT(i))
>>>>>> +            continue;
>>>>>> +
>>>>>>           irq = platform_get_irq_byname(pdev, irq_names[i]);
>>>>>>           if (irq <= 0) {
>>>>>> +            if (intc->shared_intr & BIT(i))
>>>>>> +                continue;
>>>>>
>>>>> I don't really understand why you are treating these "shared" 
>>>>> interrupts
>>>>> differently from the invalid ones. In all cases, they shouldn't be 
>>>>> used.
>>>>
>>>> The behavior is the same in how we handle it, but the difference is
>>>> that an "invalid" one is never even connected to the ARM interrupt
>>>> controller, while the "shared" one is a choice. So, unless this
>>>> interrupt is being used/handled by a different processor/entity, you
>>>> would not see this skipped from the dts node.
>>>
>>> And I'm saying that all that matters is that you are discarding these
>>> interrupts. Whether they are flagged invalid or shared, they are not
>>> available to Linux. So the difference in handling is pointless and
>>> only makes it harder to understand what you are doing.
>>
>> The primary reason for using two properties and this logic was to
>> accurately describe the h/w and usage of these in the DT bindings to
>> distinguish the "never connected" vs the "optionally can be skipped"
>> interrupts rather than go by how these are handled in the driver. I
>> feel we will loose this description and make it confusing for SoC
>> product integration developers.
> 
> This logic makes zero difference to Linux, and I do not see what
> you gain by having two code paths with separate list of unusable
> interrupts. 

OK, I understand your stance on this.

And why on Earth would a "Soc product integration
> developer" have any business to mess with this driver code?
> They should very much stay away from it and deal with their
> precious value add.

It really depends on how they put together the system and exercise the 
PRUs and the number of processors interacting with them. We have had 
customers put together usecases where both the ARM core running Linux 
and a remote processor like an M4 or R5 talk to the PRU at the same 
time, or even inter PRUSS instances. They would have to adjust the DT in 
their board dts files in general.

> 
> If you want two properties or even twenty, go for it, and have fun.
> Just don't make this driver even more unreadable than it already is.
> Merge all these interrupts in *one* list of unusable interrupts,
> and be done with it.

Yes, we are merging this for the next version.

regards
Suman

  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
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 [this message]
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=0a5664c1-88af-072e-7ad2-6a0b28fce28f@ti.com \
    --to=s-anna@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=maz@kernel.org \
    --cc=robh+dt@kernel.org \
    --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