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=-4.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 72855C433E3 for ; Tue, 21 Jul 2020 14:00:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 43870206E9 for ; Tue, 21 Jul 2020 14:00:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="xUvWGSeh" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728674AbgGUOAA (ORCPT ); Tue, 21 Jul 2020 10:00:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49686 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728578AbgGUN76 (ORCPT ); Tue, 21 Jul 2020 09:59:58 -0400 Received: from mail-qk1-x741.google.com (mail-qk1-x741.google.com [IPv6:2607:f8b0:4864:20::741]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BBDFAC0619DB for ; Tue, 21 Jul 2020 06:59:58 -0700 (PDT) Received: by mail-qk1-x741.google.com with SMTP id h7so3424717qkk.7 for ; Tue, 21 Jul 2020 06:59:58 -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=B/ns5qRnWB+BBQRDL+fY5VzUrXMw4b6mfmFMfNKLujE=; b=xUvWGSehcqbhR8LcbHbPbIg4fk7c2gLg/V0Q1aiUY70FgLi772EkoUhgZjvqXma3Ue NmiWtv8DJcERQNuLdgKqZ1KV9eOqNPGvgZPLV+PAzUi/ZraJ0MpJv0CErxSirqBGiID+ nR4kodp2IcDwy5D7V1u6fxgOUT1a+c+19nJoGAYiotR/lxPNgjg0BvnWHRqTTALqB0It Enyx+DPYjAxOR98a6EfaN6gKbYa9R+A8azmCeQHvLu9cz6aR4jtuzjgGotZuFfX4reSZ G5YqK4vyjPFxQcNhABfDO5axfSxxjc3JYFcjMt1kViEwRaKLv1kjJlkXfZh8cv9hZqpx Lz4Q== 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=B/ns5qRnWB+BBQRDL+fY5VzUrXMw4b6mfmFMfNKLujE=; b=t2ZnYwt9vg6L/9kECt80AE8ILufH76sc+Xg+9/yttaGmi37RWMxUqQg5ms3AUkksoO hrcYhtd1obhl4td1X246KvH7z74Eg/BkAZxR96x9BrTpsUUiir+qZeDVgCdoV+0YjW/d yibilaPQyLDHmq+dYHWliVvXIfyP3yccBEp6TYEh2625mWiR4WTGn8eyUDbSO2mhP8uq 3k1EkiEUg70FYg0sL8oaEduYScIQIuEi9XtE+9x+pkBKq2LrfJo3D6Tnek4Do/Y8Pfb/ MLr2AoPrmB3+F4p0ywNMZij9IFXEPK7r73orOJtdAENvAmx/1SzZv+LP0OiVZv7OnN3z +baA== X-Gm-Message-State: AOAM530pVhizSmOuUldreWzsvomWD2UBc2S23zETf0YVicM/uV0SIush c3ZWGEdhCP2tj6NT9L5Zxh9YWkqghCUqfUiSAZh4yw== X-Google-Smtp-Source: ABdhPJxSzeUqungT6SEMM+LCGp1piUGd6ekWWDyqtfp5VTWLBtTe5Wlp4yTUXNpJXhYUcESRdjjROIwiY2m2VRnfgCo= X-Received: by 2002:a37:bec6:: with SMTP id o189mr10049210qkf.303.1595339997679; Tue, 21 Jul 2020 06:59:57 -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> <3501f3a6-0613-df1c-2c6d-5ac4610a226d@ti.com> <87ft9qxqqk.wl-maz@kernel.org> <0992af0ecec787a8453492ccdf063cbd@kernel.org> In-Reply-To: <0992af0ecec787a8453492ccdf063cbd@kernel.org> From: Grzegorz Jaszczyk Date: Tue, 21 Jul 2020 15:59:46 +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, 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 , Suman Anna 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 Hi Marc, Thank you again. On Tue, 21 Jul 2020 at 12:10, Marc Zyngier wrote: > > On 2020-07-21 10:27, Grzegorz Jaszczyk wrote: > > Hi Marc, > > > > First of all thank you very much for your review. I apologize in > > advance if the description below is too verbose or not detailed > > enough. > > > > On Fri, 17 Jul 2020 at 14:36, Marc Zyngier wrote: > >> > >> Suman, Grzegorz, > >> > >> On Wed, 15 Jul 2020 14:38:05 +0100, > >> Grzegorz Jaszczyk wrote: > >> > > >> > Hi Marc, > >> > > >> > > On 7/8/20 5:47 AM, Marc Zyngier wrote: > >> > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: > >> > > >> 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. > >> > > > >> > > Sorry for the typo in my comment causing this confusion. > >> > > > >> > > The h/w actually doesn't support the edge-interrupts. Likewise, the > >> > > polarity is always high. The individual register bit descriptions > >> > > mention what the bit values 0 and 1 mean, but there is additional > >> > > description in the TRMs on all the SoCs that says > >> > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and > >> > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x). > >> > > FWIW, these are also the reset values. > >> > > > >> > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73 > >> > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, > >> > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC > >> > > methodology. > >> > > > >> > > >> > >> > > >>> > >> > > >>> [...] > >> > > >>> > >> > > >>> >> >> > + 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. > >> > > > > >> > > > Then this whole driver is non-functional until the last patch that > >> > > > comes with the PRU-specific "value-add". > >> > > > >> > > It is all moot actually and the interrupts work only when the PRU > >> > > remoteproc/clients have invoked the irq_create_fwspec_mapping() > >> > > for all of the desired system events. It does not make much difference > >> > > if it was a separate patch or squashed in, patch #6 is a replacement for > >> > > the previous logic, and since it was complex, it was done in a separate > >> > > patch to better explain the usage (same reason on v1 and v2 as > >> > > well). > >> > >> It may make no difference to you, but it does for me, as I'm the lucky > >> idiot reviewing this code. So I am going to say it again: please keep > >> anything that only exists for the PRU subsystem benefit out of the > >> initial patches. > >> > >> I want to see something that works for Linux, and only for Linux. Once > >> we have that working, we'll see to add more stuff. But stop throwing > >> the PRU business into the early patches, as all you are achieving is > >> to delay the whole thing. > >> > >> > > > >> > > > > >> > > > [...] > >> > > > > >> > > >> 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. > >> > > > > >> > > > The two level routing has to be part of this (or another) irqchip > >> > > > driver (specially given that it appears to me like another set of > >> > > > crossbar). There should only be a *single* binding for all interrupts, > >> > > > including those targeting the PRU (you seem to have two). > >> > > > > >> > > > >> > > Yeah, there hasn't been a clean way of doing this. Our previous attempt > >> > > was to do this through custom exported functions so that the PRU > >> > > remoteproc driver can set these up correctly, but that was shot down and > >> > > this is the direction we are pointed to. > >> > > > >> > > We do want to leverage the "interrupts" property in the PRU user nodes > >> > > instead of inventing our own paradigm through a non-irqchip driver, and > >> > > at the same time, be able to configure this at the run time only when > >> > > that PRU driver is running, and remove the mappings once that driver is > >> > > removed allowing another PRU application/driver. We treat PRUs as an > >> > > exclusive resource, so everything needs to go along with an appropriate > >> > > client user. > >> > > >> > I will just add an explanation about interrupt binding. So actually > >> > there is one dt-binding defined in yaml (interrupt-cells = 1). The > >> > reason why you see xlate allowing to proceed with 1 or 3 parameters is > >> > because linux can change the PRU firmware at run-time (thorough linux > >> > remoteproc framework) and different firmware may require different > >> > kinds of interrupt mapping. Therefore during firmware load, the new > >> > mapping is created through irq_create_fwspec_mapping() and in this > >> > case 3 parameters are passed: system event, channel and host irq. > >> > Similarly the mapping is disposed during remoteproc stop by invoking > >> > irq_dispose_mapping. This allows to create new mapping, in the same > >> > way, for next firmware loaded through Linux remote-proc at runtime > >> > (depending on the needs of new remoteproc firmware). > >> > > >> > On the other hand dt-bindings defines interrupt-cells = 1, so when the > >> > interrupt is registered the xlate function (proceed with 1 parameter) > >> > checks if this event already has valid mapping - if yes we are fine, > >> > if not we return -EINVAL. > >> > >> It means that interrupts declared in DT get their two-level routing > >> via the kernel driver, while PRU interrupts get their routing via some > >> external blob that Linux is not in control of? > > > > Actually with the current approach all two-level routing goes through > > this linux driver. The interrupts that should be routed to PRU are > > described in remoteproc firmware resource table [1] and it is under > > Linux remoteproc driver control. In general, the resource table > > contains system resources that the remote processor requires before it > > should be powered on. We treat the interrupt mapping (described in the > > resource table, which is a dedicated elf section defined in [1]) as > > one of system resources that linux has to provide before we power on > > the PRU core. Therefore the remoteproce driver will parse the resource > > table and trigger irq_create_fwspec_mapping() after validating > > resource table content. > > Validating the resource table says nothing of a potential conflict > with previous configured interrupts. Yes, that's why we introduced the logic in pruss_intc_irq_domain_xlate and pruss_intc_map triggered by irq_create_fwspec_mapping, which will check potential conflicts with previous configured interrupts. I understand that you do not like how it is done but I do not know how to do it in a different way so it will cover all caveats, please see below. > > > > > [1] https://www.kernel.org/doc/Documentation/remoteproc.txt (Binary > > Firmware Structure) > > > >> > >> If so, this looks broken. What if you get a resource allocation > >> conflict because the kernel and the blob are stepping into each > >> other's toes? Why should an end-point client decide on the routing of > >> the interrupt? > > > > The code in the pruss_intc_map function checks if there are no > > allocation conflicts: e.g. if the sysevent is already assigned it will > > throw -EBUSY. Similarly when some channel was already assigned to > > host_irq and a different assignment is requested it will again throw > > -EBUSY. > > But why should it? The allocation should take place based on constraints > (source, target, and as you mentioned below, priority). Instead, you > seem to be relying on static allocation coming from binary blobs, > standardized or not. > > I claim that this static allocation is madness and should be eliminated. > Instead, the Linux driver should perform the routing based on allocation > requirements (the above constraints), and only fail if it cannot satisfy > these constraints. I am not sure if I understood. The allocation requirements are as you've described: source (system event), target (host interrupt) and priority (channel). E.g.: - routing system event 3 with priority (chanell) 2 to PRU core 0 will be described as bellow: (3, 2, 0) (0 corresponds to PRU0) - routing system event 10 with priority (chanell) 3 to PRU core 1: (10, 3, 1) - routing system event 15 with priority (5) to MCPU interrupt 0*: (15, 5, 2) * interrupts 2 through 9 (host_intr0 through host_intr7) I am not sure but you probably refer to changing it to loosely dynamic allocation but this will not work for any of them since: - different system event is just a different sources (e.g. some of them are tightly coupled with PRUSS industrial ethernet peripheral, other with PRUSS UART, other are general purpose ones and so on). - lower number channels have higher priority (10 different channels, each with different priority). - host interrupt 0 is for PRU core 0; host interrupt 1 is for PRU core 1; host interrupts 2 through 9 are for main CPU. So the logic in patch #6 prevents mapping system events if it was already assigned to a different channel or target (host interrupt) and only fails if it cannot be satisfied. Moreover I do not see a way to relax the static description since picking different numbers for each individual: system event, channel and host interrupt will result with something unintentional and wrong. Sorry if I misunderstood you, if so could you please elaborate? Thank you, 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=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 AEC92C433E5 for ; Tue, 21 Jul 2020 14:01:39 +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 6A87B2064B for ; Tue, 21 Jul 2020 14:01:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="x2CGkOlv"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="xUvWGSeh" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6A87B2064B 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=+kH0TdyagAJ1Gl5acaILXVbmVgOf749WOfPVLKqJtxE=; b=x2CGkOlv9abYnc0REaA4qlS4/ zfUqhnsQ9RW2xAPStqHMh1saawnjgxcOewzeN4VwracNQYyMQHBFFU0jpaNpgqtLlH5avgJt1WfvP aOmxFqwFo1r+g6Gt6h7pz07hvY1BZwUa29Th1NPAzbP1UIsLqO3WTtZoZppjOQERSdjAMTWbRrE9e ab0jVlpF+mDltDHwi1p4y9BficzO6mcHw9IJd/crGJf1erq20WiKHUt2EP61ysf8ltdgk4IHJSqlQ Y+q/lCaX5SAzf2rFlfMsQE64DE/XuK8ra+WO42cTkpY0H7v6XRTq1PV9MkJQxk711JGMVqJ/LTzgO b9Iy3CUVA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jxsoM-00078C-Lu; Tue, 21 Jul 2020 14:00:02 +0000 Received: from mail-qk1-x743.google.com ([2607:f8b0:4864:20::743]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jxsoJ-00077B-Gp for linux-arm-kernel@lists.infradead.org; Tue, 21 Jul 2020 14:00:00 +0000 Received: by mail-qk1-x743.google.com with SMTP id q2so8441882qkc.8 for ; Tue, 21 Jul 2020 06:59:59 -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=B/ns5qRnWB+BBQRDL+fY5VzUrXMw4b6mfmFMfNKLujE=; b=xUvWGSehcqbhR8LcbHbPbIg4fk7c2gLg/V0Q1aiUY70FgLi772EkoUhgZjvqXma3Ue NmiWtv8DJcERQNuLdgKqZ1KV9eOqNPGvgZPLV+PAzUi/ZraJ0MpJv0CErxSirqBGiID+ nR4kodp2IcDwy5D7V1u6fxgOUT1a+c+19nJoGAYiotR/lxPNgjg0BvnWHRqTTALqB0It Enyx+DPYjAxOR98a6EfaN6gKbYa9R+A8azmCeQHvLu9cz6aR4jtuzjgGotZuFfX4reSZ G5YqK4vyjPFxQcNhABfDO5axfSxxjc3JYFcjMt1kViEwRaKLv1kjJlkXfZh8cv9hZqpx Lz4Q== 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=B/ns5qRnWB+BBQRDL+fY5VzUrXMw4b6mfmFMfNKLujE=; b=BCP1GGMjxsrYSBVs6M2cupzErBLFjvBV5kxoBbufo53DchN/33jJ7BIYU2EEviPfaS pwwGPRaXL4umQO/N1yNNbsFQDngOHWPuIaxYHKk5cima7IT1MX6CPfgQ6QNJ21Myd/lX kIFDOitZPZewBVmjNy3ZyqIqokw8b8uCqm2eQWmt2+MY8eeMxqT5PWrzeXS7+YmTtLaL gINj/3KwHue5aj6/uD2y1h1cPDk9J2hZ+QRCdA9M5G2ALa/6K1nBtDN1CKPR9a8LtLUS rGxkASwCzRXlSP9PecKhnc9nnLlfveMSgYHxe0ll7V+MKr7Y3EvZS0qfe0nWdAWVVjmc a8hA== X-Gm-Message-State: AOAM53395USzb+LhgY4q7U3c++ze8Opncw2yyBb6t5QI34ywOFCGgGdl njCqaDB6L3LAwwaBgINcWSUfiEQKjV3sdLOakNgpWg== X-Google-Smtp-Source: ABdhPJxSzeUqungT6SEMM+LCGp1piUGd6ekWWDyqtfp5VTWLBtTe5Wlp4yTUXNpJXhYUcESRdjjROIwiY2m2VRnfgCo= X-Received: by 2002:a37:bec6:: with SMTP id o189mr10049210qkf.303.1595339997679; Tue, 21 Jul 2020 06:59:57 -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> <3501f3a6-0613-df1c-2c6d-5ac4610a226d@ti.com> <87ft9qxqqk.wl-maz@kernel.org> <0992af0ecec787a8453492ccdf063cbd@kernel.org> In-Reply-To: <0992af0ecec787a8453492ccdf063cbd@kernel.org> From: Grzegorz Jaszczyk Date: Tue, 21 Jul 2020 15:59:46 +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-20200721_095959_642623_98E5608A X-CRM114-Status: GOOD ( 66.76 ) 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 , jason@lakedaemon.net, linux-kernel@vger.kernel.org, "Andrew F . Davis" , robh+dt@kernel.org, tglx@linutronix.de, linux-omap@vger.kernel.org, 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 Hi Marc, Thank you again. On Tue, 21 Jul 2020 at 12:10, Marc Zyngier wrote: > > On 2020-07-21 10:27, Grzegorz Jaszczyk wrote: > > Hi Marc, > > > > First of all thank you very much for your review. I apologize in > > advance if the description below is too verbose or not detailed > > enough. > > > > On Fri, 17 Jul 2020 at 14:36, Marc Zyngier wrote: > >> > >> Suman, Grzegorz, > >> > >> On Wed, 15 Jul 2020 14:38:05 +0100, > >> Grzegorz Jaszczyk wrote: > >> > > >> > Hi Marc, > >> > > >> > > On 7/8/20 5:47 AM, Marc Zyngier wrote: > >> > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: > >> > > >> 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. > >> > > > >> > > Sorry for the typo in my comment causing this confusion. > >> > > > >> > > The h/w actually doesn't support the edge-interrupts. Likewise, the > >> > > polarity is always high. The individual register bit descriptions > >> > > mention what the bit values 0 and 1 mean, but there is additional > >> > > description in the TRMs on all the SoCs that says > >> > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and > >> > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x). > >> > > FWIW, these are also the reset values. > >> > > > >> > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73 > >> > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, > >> > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC > >> > > methodology. > >> > > > >> > > >> > >> > > >>> > >> > > >>> [...] > >> > > >>> > >> > > >>> >> >> > + 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. > >> > > > > >> > > > Then this whole driver is non-functional until the last patch that > >> > > > comes with the PRU-specific "value-add". > >> > > > >> > > It is all moot actually and the interrupts work only when the PRU > >> > > remoteproc/clients have invoked the irq_create_fwspec_mapping() > >> > > for all of the desired system events. It does not make much difference > >> > > if it was a separate patch or squashed in, patch #6 is a replacement for > >> > > the previous logic, and since it was complex, it was done in a separate > >> > > patch to better explain the usage (same reason on v1 and v2 as > >> > > well). > >> > >> It may make no difference to you, but it does for me, as I'm the lucky > >> idiot reviewing this code. So I am going to say it again: please keep > >> anything that only exists for the PRU subsystem benefit out of the > >> initial patches. > >> > >> I want to see something that works for Linux, and only for Linux. Once > >> we have that working, we'll see to add more stuff. But stop throwing > >> the PRU business into the early patches, as all you are achieving is > >> to delay the whole thing. > >> > >> > > > >> > > > > >> > > > [...] > >> > > > > >> > > >> 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. > >> > > > > >> > > > The two level routing has to be part of this (or another) irqchip > >> > > > driver (specially given that it appears to me like another set of > >> > > > crossbar). There should only be a *single* binding for all interrupts, > >> > > > including those targeting the PRU (you seem to have two). > >> > > > > >> > > > >> > > Yeah, there hasn't been a clean way of doing this. Our previous attempt > >> > > was to do this through custom exported functions so that the PRU > >> > > remoteproc driver can set these up correctly, but that was shot down and > >> > > this is the direction we are pointed to. > >> > > > >> > > We do want to leverage the "interrupts" property in the PRU user nodes > >> > > instead of inventing our own paradigm through a non-irqchip driver, and > >> > > at the same time, be able to configure this at the run time only when > >> > > that PRU driver is running, and remove the mappings once that driver is > >> > > removed allowing another PRU application/driver. We treat PRUs as an > >> > > exclusive resource, so everything needs to go along with an appropriate > >> > > client user. > >> > > >> > I will just add an explanation about interrupt binding. So actually > >> > there is one dt-binding defined in yaml (interrupt-cells = 1). The > >> > reason why you see xlate allowing to proceed with 1 or 3 parameters is > >> > because linux can change the PRU firmware at run-time (thorough linux > >> > remoteproc framework) and different firmware may require different > >> > kinds of interrupt mapping. Therefore during firmware load, the new > >> > mapping is created through irq_create_fwspec_mapping() and in this > >> > case 3 parameters are passed: system event, channel and host irq. > >> > Similarly the mapping is disposed during remoteproc stop by invoking > >> > irq_dispose_mapping. This allows to create new mapping, in the same > >> > way, for next firmware loaded through Linux remote-proc at runtime > >> > (depending on the needs of new remoteproc firmware). > >> > > >> > On the other hand dt-bindings defines interrupt-cells = 1, so when the > >> > interrupt is registered the xlate function (proceed with 1 parameter) > >> > checks if this event already has valid mapping - if yes we are fine, > >> > if not we return -EINVAL. > >> > >> It means that interrupts declared in DT get their two-level routing > >> via the kernel driver, while PRU interrupts get their routing via some > >> external blob that Linux is not in control of? > > > > Actually with the current approach all two-level routing goes through > > this linux driver. The interrupts that should be routed to PRU are > > described in remoteproc firmware resource table [1] and it is under > > Linux remoteproc driver control. In general, the resource table > > contains system resources that the remote processor requires before it > > should be powered on. We treat the interrupt mapping (described in the > > resource table, which is a dedicated elf section defined in [1]) as > > one of system resources that linux has to provide before we power on > > the PRU core. Therefore the remoteproce driver will parse the resource > > table and trigger irq_create_fwspec_mapping() after validating > > resource table content. > > Validating the resource table says nothing of a potential conflict > with previous configured interrupts. Yes, that's why we introduced the logic in pruss_intc_irq_domain_xlate and pruss_intc_map triggered by irq_create_fwspec_mapping, which will check potential conflicts with previous configured interrupts. I understand that you do not like how it is done but I do not know how to do it in a different way so it will cover all caveats, please see below. > > > > > [1] https://www.kernel.org/doc/Documentation/remoteproc.txt (Binary > > Firmware Structure) > > > >> > >> If so, this looks broken. What if you get a resource allocation > >> conflict because the kernel and the blob are stepping into each > >> other's toes? Why should an end-point client decide on the routing of > >> the interrupt? > > > > The code in the pruss_intc_map function checks if there are no > > allocation conflicts: e.g. if the sysevent is already assigned it will > > throw -EBUSY. Similarly when some channel was already assigned to > > host_irq and a different assignment is requested it will again throw > > -EBUSY. > > But why should it? The allocation should take place based on constraints > (source, target, and as you mentioned below, priority). Instead, you > seem to be relying on static allocation coming from binary blobs, > standardized or not. > > I claim that this static allocation is madness and should be eliminated. > Instead, the Linux driver should perform the routing based on allocation > requirements (the above constraints), and only fail if it cannot satisfy > these constraints. I am not sure if I understood. The allocation requirements are as you've described: source (system event), target (host interrupt) and priority (channel). E.g.: - routing system event 3 with priority (chanell) 2 to PRU core 0 will be described as bellow: (3, 2, 0) (0 corresponds to PRU0) - routing system event 10 with priority (chanell) 3 to PRU core 1: (10, 3, 1) - routing system event 15 with priority (5) to MCPU interrupt 0*: (15, 5, 2) * interrupts 2 through 9 (host_intr0 through host_intr7) I am not sure but you probably refer to changing it to loosely dynamic allocation but this will not work for any of them since: - different system event is just a different sources (e.g. some of them are tightly coupled with PRUSS industrial ethernet peripheral, other with PRUSS UART, other are general purpose ones and so on). - lower number channels have higher priority (10 different channels, each with different priority). - host interrupt 0 is for PRU core 0; host interrupt 1 is for PRU core 1; host interrupts 2 through 9 are for main CPU. So the logic in patch #6 prevents mapping system events if it was already assigned to a different channel or target (host interrupt) and only fails if it cannot be satisfied. Moreover I do not see a way to relax the static description since picking different numbers for each individual: system event, channel and host interrupt will result with something unintentional and wrong. Sorry if I misunderstood you, if so could you please elaborate? Thank you, Grzegorz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel