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=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 81BB1C433E1 for ; Wed, 8 Jul 2020 07:04:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5D47F20760 for ; Wed, 8 Jul 2020 07:04:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Omi9/Ujy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730057AbgGHHEQ (ORCPT ); Wed, 8 Jul 2020 03:04:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729889AbgGHHEP (ORCPT ); Wed, 8 Jul 2020 03:04:15 -0400 Received: from mail-qt1-x844.google.com (mail-qt1-x844.google.com [IPv6:2607:f8b0:4864:20::844]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C3437C08C5E1 for ; Wed, 8 Jul 2020 00:04:15 -0700 (PDT) Received: by mail-qt1-x844.google.com with SMTP id d27so33725226qtg.4 for ; Wed, 08 Jul 2020 00:04:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2iS8GNVus2oLt6BrJG806hhfOAQEc/axdhBS15zjWLo=; b=Omi9/UjyrDM94m6Hn/9tGzuCwejaXFZixzxkS4l0YPmtw6UPjYdnkDbOX5Q2rV/lFr u1TxIMxdxNTW4aT9NliJDUOzsqwMCfgBqv/GaB6/YcxP8D6m9dW9G1Q2A+k1AhXcnIcJ YDBmI2aKHBzbPIgwQ/GLoCx1rqtpSxv5WBgRsVTLGi4tu0OSrcxmQGlKWX7U++jKCkNF C7COudJTQnTpO/GUP94Li97jg42fApyQJxTH5GM9g+U2rK42Dat7lPNCxd5duQoADePp vx27GRxaYe2rU7Vecb4trNeAPCLI/J4z3NdkXw3WZKnEZ10krHbJ+zMVgBT/BokdEoOk LZWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2iS8GNVus2oLt6BrJG806hhfOAQEc/axdhBS15zjWLo=; b=bbO+zgAM/FdvFGQcFE+4cIEUnINWV8/uyppubV4/WhNMjwojTp4kfy//4m9oIHesOI 2a6YfGTliPHZdHxKrOASfip0K6a6CzyVLBqnzFAyg7P7w6+t0r8eGWD2ARSwVEhkglE1 FABArHQXbErSoTs1RRM9fj8wLUbmjAKjHGXlOGxdmv4VKLKT/bjFm3zDbCceW9EzzaKG wEF9RlI6eRjJVXuZWGrthHP0JTuy1sZwX1Xtajag7xlytvu5gLsBAk61Vkr4i6RJWSbp CIdwOgwqDUWDXYXsmGkeFt5GimBEuIDOzAKEd8xiLq2az4Y9Lqzrz7BTutqtyI6rjk// fUig== X-Gm-Message-State: AOAM530sUH2DqrE8TpQMX6HB3Aw8GlEGaxKp9r/qObHW2vZBzUsf+Vwb 4kTvJ6HsgZhzIBFZ4k6DVau0ai5CMI5SjxvK90t40Q== X-Google-Smtp-Source: ABdhPJyOpd9MCvJq83xKqOPDNKMak9gwDotLjA6QoKzbnEMOiGr9okynYLPyXCkNOJLEALP09uEV9N6WRvLQc4GN8+c= X-Received: by 2002:ac8:66d1:: with SMTP id m17mr58814731qtp.88.1594191854938; Wed, 08 Jul 2020 00:04:14 -0700 (PDT) MIME-Version: 1.0 References: <1593699479-1445-1-git-send-email-grzegorz.jaszczyk@linaro.org> <1593699479-1445-3-git-send-email-grzegorz.jaszczyk@linaro.org> <12db6d22c12369b6d64f410aa2434b03@kernel.org> <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> In-Reply-To: <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> From: Grzegorz Jaszczyk Date: Wed, 8 Jul 2020 09:04:03 +0200 Message-ID: Subject: Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts To: Marc Zyngier Cc: tglx@linutronix.de, jason@lakedaemon.net, "Anna, Suman" , robh+dt@kernel.org, Lee Jones , 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" , "Andrew F . Davis" , Roger Quadros Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 5 Jul 2020 at 22:45, Marc Zyngier wrote: > > On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier 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 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=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 B7404C433DF for ; Wed, 8 Jul 2020 07:07:28 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 86E0B20760 for ; Wed, 8 Jul 2020 07:07:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="IKgX5jxd"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Omi9/Ujy" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 86E0B20760 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=kEVXqinD1Wz5IlKsoE/6b9e8UDm7Ce4CeFeAVQYXD1g=; b=IKgX5jxderjN17jaVwnvrvNMH eAIQYcLNspITg3TeQPfQt6qpc6FmHTF7BEg3YjYn5gwhIFbzaA/Cak8j4dL2L9d8dtMzIz1E4X59u ygm8S5G+S9b4ycwURWrDfwUlgkyCOjMhokRZ1oC07S6VEBj4Or6+tboSUAfhYGtG4y5bxhyvACHYH /GrNA71QmltONhr07DGG97A1BTb89kvehYqnscD47R/LUYJ8BKBrgSDWsQAJMS7ril1kMV8RYNvGq pUr9l4T0K0CvEmYSnX2W/16IoMif6r5STVzpqoyAQZY83ti7RJxftoph31ecdv4Moo1jDbfMFhvRW XDFemnDow==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jt48L-0007xH-AL; Wed, 08 Jul 2020 07:04:45 +0000 Received: from mail-qt1-x843.google.com ([2607:f8b0:4864:20::843]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jt47u-0007vg-7U for linux-arm-kernel@lists.infradead.org; Wed, 08 Jul 2020 07:04:20 +0000 Received: by mail-qt1-x843.google.com with SMTP id d27so33725225qtg.4 for ; Wed, 08 Jul 2020 00:04:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2iS8GNVus2oLt6BrJG806hhfOAQEc/axdhBS15zjWLo=; b=Omi9/UjyrDM94m6Hn/9tGzuCwejaXFZixzxkS4l0YPmtw6UPjYdnkDbOX5Q2rV/lFr u1TxIMxdxNTW4aT9NliJDUOzsqwMCfgBqv/GaB6/YcxP8D6m9dW9G1Q2A+k1AhXcnIcJ YDBmI2aKHBzbPIgwQ/GLoCx1rqtpSxv5WBgRsVTLGi4tu0OSrcxmQGlKWX7U++jKCkNF C7COudJTQnTpO/GUP94Li97jg42fApyQJxTH5GM9g+U2rK42Dat7lPNCxd5duQoADePp vx27GRxaYe2rU7Vecb4trNeAPCLI/J4z3NdkXw3WZKnEZ10krHbJ+zMVgBT/BokdEoOk LZWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2iS8GNVus2oLt6BrJG806hhfOAQEc/axdhBS15zjWLo=; b=HUEZDlRiP6bCvZpAwl0dBT6vJAd4XM3CC5pxdC2JsRgLGImr3Abln7pIFRokQYbL68 tn6o5W2CdqPkRcoxK6f1GBQcPoyrKzTMPoYVzXjEguWhssvjdM6G4jTmiKAtV0D9aQeU Ss2KqKbkPpPJklfMe3ieO4wqN8wtY1jfU8bXr1UzYE0dixPnys9LT8/+2breVyR+IXfl ruYiay5/gvNDLsOZGlsPgMVXy7LjM4XkAvVoVZG1Dg57kXLQjenHbn8NEaevtnjVLnVP YcJnEOWK96TD/3qhCBmOb/F2hFM5npUf8YIck8zF88FOLbQIgwE3bvF21p6q32JxBW0k 7AUw== X-Gm-Message-State: AOAM532zoH4rj8U0n+wJ9ZI/4o2K99Q0yaCBTfuhj0njPtdsw8j8wN+R 9pIGjVeP96csuPtBGN8ey8UGGIIVgcyTAs2XcvJVYA== X-Google-Smtp-Source: ABdhPJyOpd9MCvJq83xKqOPDNKMak9gwDotLjA6QoKzbnEMOiGr9okynYLPyXCkNOJLEALP09uEV9N6WRvLQc4GN8+c= X-Received: by 2002:ac8:66d1:: with SMTP id m17mr58814731qtp.88.1594191854938; Wed, 08 Jul 2020 00:04:14 -0700 (PDT) MIME-Version: 1.0 References: <1593699479-1445-1-git-send-email-grzegorz.jaszczyk@linaro.org> <1593699479-1445-3-git-send-email-grzegorz.jaszczyk@linaro.org> <12db6d22c12369b6d64f410aa2434b03@kernel.org> <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> In-Reply-To: <53d39d8fbd63c6638dbf0584c7016ee0@kernel.org> From: Grzegorz Jaszczyk Date: Wed, 8 Jul 2020 09:04:03 +0200 Message-ID: Subject: Re: [PATCHv3 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts To: Marc Zyngier X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200708_030418_316976_D8A0EB85 X-CRM114-Status: GOOD ( 35.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Roger Quadros , linux-omap@vger.kernel.org, jason@lakedaemon.net, linux-kernel@vger.kernel.org, "Andrew F . Davis" , robh+dt@kernel.org, tglx@linutronix.de, Lee Jones , "Mills, William" , linux-arm-kernel@lists.infradead.org, david@lechnology.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, 5 Jul 2020 at 22:45, Marc Zyngier wrote: > > On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel