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=-28.6 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 AB24AC43381 for ; Thu, 7 Jan 2021 23:14:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8DE3D23602 for ; Thu, 7 Jan 2021 23:14:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728848AbhAGXO1 (ORCPT ); Thu, 7 Jan 2021 18:14:27 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54050 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725944AbhAGXO0 (ORCPT ); Thu, 7 Jan 2021 18:14:26 -0500 Received: from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com [IPv6:2607:f8b0:4864:20::b35]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4FFF8C0612F6 for ; Thu, 7 Jan 2021 15:13:46 -0800 (PST) Received: by mail-yb1-xb35.google.com with SMTP id k78so7652263ybf.12 for ; Thu, 07 Jan 2021 15:13:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Ma8dmHfcNup9f2l6KiI5aUKrS0zBSPZz4JeC2fHhEeg=; b=dDB7m+6M5qRLNvBOVXpbKeRt4krh9Ey+Ax1NsJ02ZAiFTN5zcObWAiHNQtoIDP3k0h QB+VsvGJr2/HqE7IEH7iBn9ES3r2EtncS3YhenGDA0RVepWqw2xylEt7wAAk+kQmnRsL oJfjsMzwHWXpnej/WSiW6gmf6vwckM5TbaTQSZh4U5V10U9hw+MXxzi8OgkAs50UCqjG 2oqBFTJX1h0EgPbXnHrsXK6F741vMlq2zjHxCUxSSO0F7ml8WqN9+SJqDpCvumb/nAOG 9FhVNULHc+XE8z9J8E27PBxbUR+x7OZdXcgCRSb+1qZdSBHWoDE1cs5zM8pGjCUQaLul 2p+Q== 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=Ma8dmHfcNup9f2l6KiI5aUKrS0zBSPZz4JeC2fHhEeg=; b=GrxnJqtpRtm6sRo2RaY0nQJVquLqWpYc8QpZk/175hYbmuSJ7UItk8ySI6I2/vm/op E3kXVRPmehjung/MUp+9k/Wwc+Rj3qgXKgR1nYluAswBScTNOs0alrJP1nJzfn4UvF7L 9+gtVyTdEKbuC2zjc9kbuOIFZpR52oE4YIvDI5N/c1maD+6mylrXqusr2aC5+i2STA76 6ffdHHD4w44WtUXi0+9zIJpg5W9td4CmEtsB7p6ZIhfLDo2P4L78Hl1OP6m7CxWS1+dE ItPhGUGWq/E6WC36eGhempjQW9KVRSJgdX3dgqhsX1OiXK21X5XAiZMlHQZ634/jrkSx /PDA== X-Gm-Message-State: AOAM530LhGyx8aTaLhVVB0LvCZICKDEgEzHZYczsLSOUa24AHdayjIEq Ra8vv/95Xa+UpJGS0eZCtUR/yatnWxobRiQFXxuSsg== X-Google-Smtp-Source: ABdhPJzQxkg9rjd5HmkGmFUmyyb8YFizrn/9J62upZOBfX65Ww5bdLnFf9DQbUWCgPIE5PG3FRJYii+nHiCyxKY07hA= X-Received: by 2002:a25:4f55:: with SMTP id d82mr1751139ybb.466.1610061225334; Thu, 07 Jan 2021 15:13:45 -0800 (PST) MIME-Version: 1.0 References: <20201218210750.3455872-1-saravanak@google.com> <2a6dbcc83d5aca7a3340e0cf4d751cdc@kernel.org> <20201231211240.GA2333246@robh.at.kernel.org> <877dovlgdl.wl-maz@kernel.org> In-Reply-To: From: Saravana Kannan Date: Thu, 7 Jan 2021 15:13:08 -0800 Message-ID: Subject: Re: [PATCH] of: property: Add device link support for interrupts To: Rob Herring Cc: Marc Zyngier , Greg Kroah-Hartman , Frank Rowand , Kevin Hilman , Android Kernel Team , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , LKML Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 7, 2021 at 11:33 AM Rob Herring wrote: > > On Thu, Jan 7, 2021 at 12:09 PM Saravana Kannan wrote: > > > > On Thu, Jan 7, 2021 at 10:39 AM Rob Herring wrote: > > > > > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan wrote: > > > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier wrote: > > > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > > Rob Herring wrote: > > > > > > > > > > > > On Mon, Dec 21, 2020 at 09:30:45AM +0000, Marc Zyngier wrote: > > > > > > > On 2020-12-18 21:07, Saravana Kannan wrote: > > > > > > > > Add support for creating device links out of interrupts property. > > > > > > > > > > > > > > > > Cc: Marc Zyngier > > > > > > > > Cc: Kevin Hilman > > > > > > > > Signed-off-by: Saravana Kannan > > > > > > > > --- > > > > > > > > Rob/Greg, > > > > > > > > > > > > > > > > This might need to go into driver-core to avoid conflict > > > > > > > > due to fw_devlink refactor series that merged there. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Saravana > > > > > > > > > > > > > > > > > > > > > > > > drivers/of/property.c | 17 +++++++++++++++++ > > > > > > > > 1 file changed, 17 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > > > > > > index 5f9eed79a8aa..e56a5eae0a0b 100644 > > > > > > > > --- a/drivers/of/property.c > > > > > > > > +++ b/drivers/of/property.c > > > > > > > > @@ -1271,6 +1271,22 @@ static struct device_node > > > > > > > > *parse_iommu_maps(struct device_node *np, > > > > > > > > return of_parse_phandle(np, prop_name, (index * 4) + 1); > > > > > > > > } > > > > > > > > > > > > > > > > +static struct device_node *parse_interrupts(struct device_node *np, > > > > > > > > + const char *prop_name, int index) > > > > > > > > +{ > > > > > > > > + struct device_node *sup; > > > > > > > > + > > > > > > > > + if (strcmp(prop_name, "interrupts") || index) > > > > > > > > + return NULL; > > > > > > > > + > > > > > > > > + of_node_get(np); > > > > > > > > + while (np && !(sup = of_parse_phandle(np, "interrupt-parent", 0))) > > > > > > > > + np = of_get_next_parent(np); > > > > > > > > + of_node_put(np); > > > > > > > > + > > > > > > > > + return sup; > > > > > > > > +} > > > > > > > > + > > > > > > > > static const struct supplier_bindings of_supplier_bindings[] = { > > > > > > > > { .parse_prop = parse_clocks, }, > > > > > > > > { .parse_prop = parse_interconnects, }, > > > > > > > > @@ -1296,6 +1312,7 @@ static const struct supplier_bindings > > > > > > > > of_supplier_bindings[] = { > > > > > > > > { .parse_prop = parse_pinctrl6, }, > > > > > > > > { .parse_prop = parse_pinctrl7, }, > > > > > > > > { .parse_prop = parse_pinctrl8, }, > > > > > > > > + { .parse_prop = parse_interrupts, }, > > > > > > > > { .parse_prop = parse_regulators, }, > > > > > > > > { .parse_prop = parse_gpio, }, > > > > > > > > { .parse_prop = parse_gpios, }, > > > > > > > > > > > > > > You don't really describe what this is for so I'm only guessing > > > > > > > from the context. If you want to follow the interrupt hierarchy, > > > > > > > "interrupt-parent" isn't enough. You also need to track > > > > > > > things like interrupt-map, or anything that carries a phandle > > > > > > > to an interrupt controller. > > > > > > > > > > > > We don't need to follow the hierarchy, we just need the immediate > > > > > > dependencies. > > > > > > > > > > Indeed. I also wonder why this isn't just a irq_find_parent() call, TBH. > > > > > > > > Thanks Rob for explaining it. > > > > > > > > Marc, I wasn't sure if Rob would be okay with including of_irq.h here. > > > > Also, I'm trying to keep of/property.c independent of the framework > > > > code for now. The long term goal is to see if I can move out most of > > > > this into the frameworks. But I want to do that after I sort of some > > > > of the larger problems (like getting fw_devlink=on to work on all > > > > devices first). Let me know if you have a strong preference for right > > > > now, if not, I'd rather keep property.c independent for now. > > > > > > > > I wasn't aware of interrupt-map until a few weeks ago and didn't know > > > > it carried phandles. I can add support for that too. There's no reason > > > > for all of them to go in one patch though. > > > > > > > > > > > > > > > But you are right that 'interrupt-map' also needs to be tracked. > > > > > > > > > > And 'interrupts-extended', while we're at it. > > > > > > > > This is already handled. > > > > > > > > > > > > > > > > I also noticed that we define 'interrupt-parent' as a dependency to > > > > > > parse, but that's wrong. The dependency is where 'interrupts' appears > > > > > > and where 'interrupt-parent' appears is irrelevant. > > > > > > > > No, the interrupt-parent parsing is correct and it's needed for > > > > interrupt controllers to probe in the right order. But > > > > interrupt-parent is also needs to be looked at for parsing > > > > "interrupts". > > > > > > If you parse 'interrupts' for interrupt controllers (which in turn > > > will use 'interrupt-parent'), then you aren't going to need to track > > > 'interrupt-parent' by itself. > > > > Do all interrupt controllers (that are not the root interrupt > > controller) need to have "interrupts" property? If yes, then yeah, > > that makes sense. But I vaguely remember that this wasn't the case for > > some DT I saw. > > There are some cases of stacked controllers where it's implicit. In that case, I think it's good to track interrupt-parent explicitly. Doesn't really hurt anything. We already protect for stuff like making sure a parent doesn't depend on its child, etc. > > > > Ah, here's one I found. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 > > Right, so this is one of several cases of custom interrupt mapping > properties (mediatek,ext-irq-range). Really, 'interrupts' or > 'interrupt-map' should have been used here, but 'interrupt-map' > doesn't really scale well if you have large ranges of interrupts. > > To handle the dependency with just 'interrupt-parent', you need to > find nodes that are themselves an 'interrupt-parent' and then find > their 'interrupt-parent'. Not sure I understand this. On a side note, if I'm adding device links between a device and the "interrupt-parent" it's pointing to, at worst, I'm having it depend on an interrupt controller its child devices would depend on. This combined with the fact that "weird" links like "parent depending on child", "non-device node having interrupt-parent", etc are already ignored, seems safe to leave in "interrupt-parent" to catch these cases where interrupt controllers don't specify "interrupts" or "interrupt-map"? I don't mind removing it, but maybe we can wait till we get fw_devlink=on and then remove it to see if anything breaks? If nothing breaks, we can remove explicit interrupt-parent parsing? Going back to interrupt-map, I understand the syntax now. I'm trying to see if I can break up of_irq_parse_raw() into smaller pieces and reuse (call into) some of that code. While doing that I see that when "address-cells" isn't present, the "addrsize" is initialized to 2 [1] but when "address-cells" isn't present, the "newaddrsize" is initialized to 0 [1]. Why is the default value of #address-cells two different numbers? Thanks, Saravana [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/irq.c#n141 [2] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/irq.c#n226