From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1B76C433E4 for ; Fri, 17 Jul 2020 11:02:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 74D2F20768 for ; Fri, 17 Jul 2020 11:02:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594983767; bh=bbLMObkNrniR80ZdzHzteBsWJ5zcpRCBrOI2j6GQ4W0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=Pyk79y48BJyb3/csv1+2AzkCfvtyauyxnP3PEnChYrNXs8Nkg1YXLuqKnKt8hO/4/ XWO4puRRgf+acwWath3SrK5lX6Ff4LSIJiAi42rYgoptyYpV/Zj/MeF53PhnZ7DqFf xgOqzoySb+/Q6winbq2WwF+3VnuYJrEhdjjtl2lI= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726200AbgGQLCn (ORCPT ); Fri, 17 Jul 2020 07:02:43 -0400 Received: from mail.kernel.org ([198.145.29.99]:55164 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725932AbgGQLCm (ORCPT ); Fri, 17 Jul 2020 07:02:42 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id CF84D20717; Fri, 17 Jul 2020 11:02:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594983761; bh=bbLMObkNrniR80ZdzHzteBsWJ5zcpRCBrOI2j6GQ4W0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=dqzm/OI6mZOxyuUKPJ7jFTK+TsDEjToHh/tLOy9xUBQ0ukZBFD8kQt8fFOxEk03fW BwU1AfbENm2NKpfpqLKq+ks8pBss9yT0+cwgjABMePaatnRza0S8ZMUDZPZknMAAlL 3TXECbw7eZtO0cuZCTRvKCVwNpJnABIlFHvSRfbE= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwO8V-00CbW3-DI; Fri, 17 Jul 2020 12:02:39 +0100 Date: Fri, 17 Jul 2020 12:02:38 +0100 Message-ID: <87imemxv3l.wl-maz@kernel.org> From: Marc Zyngier To: Suman Anna Cc: Grzegorz Jaszczyk , , , , , , , , , , Subject: Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts In-Reply-To: <3a73bb14-9f7b-970d-fbae-f9c7bb7bdf1e@ti.com> References: <1593699479-1445-1-git-send-email-grzegorz.jaszczyk@linaro.org> <1593699479-1445-4-git-send-email-grzegorz.jaszczyk@linaro.org> <2a6b0391f1395eb0aa15ffee6769184e@kernel.org> <3a73bb14-9f7b-970d-fbae-f9c7bb7bdf1e@ti.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: s-anna@ti.com, 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 X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org On Fri, 10 Jul 2020 21:59:17 +0100, Suman Anna wrote: Hi Suman, [...] > > Hi Marc, >=20 > On 7/2/20 12:44 PM, Marc Zyngier wrote: > > On 2020-07-02 15:17, Grzegorz Jaszczyk wrote: > >> From: Suman Anna > >>=20 > >> The PRUSS INTC has a fixed number of output interrupt lines that are > >> connected to a number of processors or other PRUSS instances or other > >> devices (like DMA) on the SoC. The output interrupt lines 2 through 9 > >> are usually connected to the main Arm host processor and are referred > >> to as host interrupts 0 through 7 from ARM/MPU perspective. > >>=20 > >> All of these 8 host interrupts are not always exclusively connected > >> to the Arm interrupt controller. Some SoCs have some interrupt lines > >> not connected to the Arm interrupt controller at all, while a few othe= rs > >> have the interrupt lines connected to multiple processors in which they > >> need to be partitioned as per SoC integration needs. For example, AM43= 7x > >> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrup= t 5 > >> connected to the other PRUSS, while AM335x has host interrupt 0 shared > >> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU a= nd > >> a DMA controller. > >>=20 > >> Add support to the PRUSS INTC driver to allow both these shared and > >> invalid interrupts by not returning a failure if any of these interrup= ts > >> are skipped from the corresponding INTC DT node. > >=20 > > That's not exactly "adding support", is it? It really is "ignore these > > interrupts because they are useless from the main CPU's perspective", > > right? >=20 > Correct. We can rephrase this to something like > "Add logic to the PRUSS INTC driver to ignore.." >=20 > >=20 > >>=20 > >> Signed-off-by: Suman Anna > >> Signed-off-by: Grzegorz Jaszczyk > >> --- > >> v2->v3: > >> - Extra checks for (intc->irqs[i]) in error/remove path was moved from > >> =C2=A0 "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS > >> =C2=A0 interrupts" to this patch > >> v1->v2: > >> - https://patchwork.kernel.org/patch/11069757/ > >> --- > >> =C2=A0drivers/irqchip/irq-pruss-intc.c | 73 > >> +++++++++++++++++++++++++++++++++++++--- > >> =C2=A01 file changed, 68 insertions(+), 5 deletions(-) > >>=20 > >> diff --git a/drivers/irqchip/irq-pruss-intc.c > >> b/drivers/irqchip/irq-pruss-intc.c > >> index fb3dda3..49c936f 100644 > >> --- a/drivers/irqchip/irq-pruss-intc.c > >> +++ b/drivers/irqchip/irq-pruss-intc.c > >> @@ -65,11 +65,15 @@ > >> =C2=A0 * @irqs: kernel irq numbers corresponding to PRUSS host interru= pts > >> =C2=A0 * @base: base virtual address of INTC register space > >> =C2=A0 * @domain: irq domain for this interrupt controller > >> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared > >=20 > > nit: bitmap >=20 > ok >=20 > >=20 > >> + * @invalid_intr: bit-map denoting if host interrupt is not > >> connected to MPU > >> =C2=A0 */ > >> =C2=A0struct pruss_intc { > >> =C2=A0=C2=A0=C2=A0=C2=A0 unsigned int irqs[MAX_NUM_HOST_IRQS]; > >> =C2=A0=C2=A0=C2=A0=C2=A0 void __iomem *base; > >> =C2=A0=C2=A0=C2=A0=C2=A0 struct irq_domain *domain; > >> +=C2=A0=C2=A0=C2=A0 u16 shared_intr; > >> +=C2=A0=C2=A0=C2=A0 u16 invalid_intr; > >=20 > > Please represent bitmaps as an unsigned long. >=20 > ok. We have atmost 8 interrupts coming in, but agree on the change > since we are using the BIT() macro below. >=20 > >=20 > >> =C2=A0}; > >>=20 > >> =C2=A0static inline u32 pruss_intc_read_reg(struct pruss_intc *intc, > >> unsigned int reg) > >> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct > >> platform_device *pdev) > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "host_intr4", "host_i= ntr5", "host_intr6", "host_intr7", }; > >> =C2=A0=C2=A0=C2=A0=C2=A0 struct device *dev =3D &pdev->dev; > >> =C2=A0=C2=A0=C2=A0=C2=A0 struct pruss_intc *intc; > >> -=C2=A0=C2=A0=C2=A0 int i, irq; > >> +=C2=A0=C2=A0=C2=A0 int i, irq, count; > >> +=C2=A0=C2=A0=C2=A0 u8 temp_intr[MAX_NUM_HOST_IRQS] =3D { 0 }; > >>=20 > >> =C2=A0=C2=A0=C2=A0=C2=A0 intc =3D devm_kzalloc(dev, sizeof(*intc), GFP= _KERNEL); > >> =C2=A0=C2=A0=C2=A0=C2=A0 if (!intc) > >> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct > >> platform_device *pdev) > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return PTR_ERR(intc->= base); > >> =C2=A0=C2=A0=C2=A0=C2=A0 } > >>=20 > >> +=C2=A0=C2=A0=C2=A0 count =3D of_property_read_variable_u8_array(dev->= of_node, > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 "ti,irqs-reserved", > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 temp_intr, 0, > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 MAX_NUM_HOST_IRQS); > >> +=C2=A0=C2=A0=C2=A0 /* > >> +=C2=A0=C2=A0=C2=A0=C2=A0 * The irqs-reserved is used only for some So= C's therefore not > >> having > >> +=C2=A0=C2=A0=C2=A0=C2=A0 * this property is still valid > >> +=C2=A0=C2=A0=C2=A0=C2=A0 */ > >> +=C2=A0=C2=A0=C2=A0 if (count =3D=3D -EINVAL) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count =3D 0; > >> +=C2=A0=C2=A0=C2=A0 if (count < 0) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return count; > >> + > >> +=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < count; i++) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (temp_intr[i] >=3D MAX_= NUM_HOST_IRQS) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 de= v_warn(dev, "ignoring invalid reserved irq %d\n", > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 temp_intr[i]); > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 co= ntinue; > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > >> + > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 intc->invalid_intr |=3D BI= T(temp_intr[i]); > >> +=C2=A0=C2=A0=C2=A0 } > >> + > >> +=C2=A0=C2=A0=C2=A0 count =3D of_property_read_variable_u8_array(dev->= of_node, > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 "ti,irqs-shared", > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 temp_intr, 0, > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 MAX_NUM_HOST_IRQS); > >> +=C2=A0=C2=A0=C2=A0 /* > >> +=C2=A0=C2=A0=C2=A0=C2=A0 * The irqs-shared is used only for some SoC'= s therefore not having > >> +=C2=A0=C2=A0=C2=A0=C2=A0 * this property is still valid > >> +=C2=A0=C2=A0=C2=A0=C2=A0 */ > >> +=C2=A0=C2=A0=C2=A0 if (count =3D=3D -EINVAL) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count =3D 0; > >> +=C2=A0=C2=A0=C2=A0 if (count < 0) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return count; > >> + > >> +=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < count; i++) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (temp_intr[i] >=3D MAX_= NUM_HOST_IRQS) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 de= v_warn(dev, "ignoring invalid shared irq %d\n", > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 temp_intr[i]); > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 co= ntinue; > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > >> + > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 intc->shared_intr |=3D BIT= (temp_intr[i]); > >> +=C2=A0=C2=A0=C2=A0 } > >> + > >=20 > > You probably want to move this in a separate function, since you popula= te a > > common structure. > >=20 > >> =C2=A0=C2=A0=C2=A0=C2=A0 pruss_intc_init(intc); > >>=20 > >> =C2=A0=C2=A0=C2=A0=C2=A0 /* always 64 events */ > >> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct > >> platform_device *pdev) > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -ENOMEM; > >>=20 > >> =C2=A0=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < MAX_NUM_HOST_IRQS; i++) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (intc->invalid_intr & B= IT(i)) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 co= ntinue; > >> + > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 irq =3D platform_get_= irq_byname(pdev, irq_names[i]); > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (irq <=3D 0) { > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if= (intc->shared_intr & BIT(i)) > >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 continue; > >=20 > > 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. >=20 > 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. Thanks, M. --=20 Without deviation from the norm, progress is not possible.