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=-1.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED 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 A2BC7C43381 for ; Fri, 8 Mar 2019 15:31:13 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 6A9C6208E4 for ; Fri, 8 Mar 2019 15:31:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Ywy/gogW" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6A9C6208E4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ALy7+rb/kMMzjevBxqxcbmvyXEzAIigx7BId3TdKRxo=; b=Ywy/gogWH5/OUt /cSvALxYTNAgll53mlSi6w1WPWG4dlWgB1IEVSjWQ+ZOhMV8S31ABrJTzlL1mCrMvWl5uGGj61MOT UUVF1HHPDoJpItGx24cX0P8LEvvGiFwH6XAudymwt2vEoNhnqBAOwo7c11LNvo+7TxopQpnYPIsWn IRv2GYat3iNoAAaOGTLJbaTWvd4KxEAixElh/vQKyy1vCgowjicyNlyeqACkEqJG8ZcDbgEdbsGWL DawdF+KKmmxk1mUyuwW/A7jEYJDz+Yz0pyUJI0sk6BmjICjRyKTEGKTOqzBF7cd+rYb0UlBEnjgS4 Ty6lPLTaRUyxGAbNfz+g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h2HSj-0000uu-H0; Fri, 08 Mar 2019 15:31:05 +0000 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70] helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h2HSf-0000u7-I6 for linux-arm-kernel@lists.infradead.org; Fri, 08 Mar 2019 15:31:03 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CA574A78; Fri, 8 Mar 2019 07:30:58 -0800 (PST) Received: from big-swifty.misterjones.org (big-swifty.cambridge.arm.com [10.1.30.93]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 808E63F703; Fri, 8 Mar 2019 07:30:56 -0800 (PST) Date: Fri, 08 Mar 2019 15:30:53 +0000 Message-ID: <864l8d1iz6.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Fabien DESSENNE Subject: Re: [PATCH] irqchip: stm32: add a second level init to request hwspinlock In-Reply-To: References: <1551975829-16350-1-git-send-email-fabien.dessenne@st.com> <97040a1e-7a24-3d41-c3b7-43b551a70825@arm.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 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190308_073101_611182_61CE1647 X-CRM114-Status: GOOD ( 27.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Benjamin GAIGNARD , Jason Cooper , "linux-kernel@vger.kernel.org" , Maxime Coquelin , Thomas Gleixner , "linux-stm32@st-md-mailman.stormreply.com" , "linux-arm-kernel@lists.infradead.org" , Alexandre TORGUE Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 08 Mar 2019 14:03:55 +0000, Fabien DESSENNE wrote: Fabien, > > Hi Marc, > > Thank you for your feedback. Let me try to explain this patch, and the > reason of its unusual implementation choices. > > > Regarding the driver init mode: > As an important requirement, I want to keep this irq driver declared > with IRQCHIP_DECLARE(), so it is initialized early from > start_kernel()/init_IRQ(). > Most of the other irq drivers are implemented this way and I imagine > that this ensures the availability of the irq drivers, before the other > platform drivers get probed. Let me get this straight: - Either you don't have dependencies on anything, and you need to enable your irqchip early -> you use IRQCHIP_DECLARE. - Or you have dependencies on other subsystems -> You *do not* use IRQCHIP_DECLARE, and use the expected dependency system (deferred probing) There is no intermediate state. The other irqchip controllers that use IRQCHIP_DECLARE *do NOT* have dependencies on external subsystems. > Regarding the second init: > With the usage of the hwspinlock framework (used to protect against > coprocessor concurrent access to registers) we have a problem as the > hwspinlock driver is not available when the irq driver is being initialized. > In order to solve this, I added a second initialization where we get a > reference to hwspinlock. > You pointed that we are not supposed to use of_node_clear_flag (which > allows to get a second init call) : > I spent some time to find any information about it, but could not find > any reason to not use it. > Please, let me know if I missed something here. Yes, you missed the fact that each time someone tries to add some driver probing via an initcall, we push back. This is an internal kernel mechanism that is not to be used by random, non architectural drivers such as this interrupt controller. Furthermore, you're playing with stuff that is outside of the exported API of the DT framework. Clearing node flags is not something I really want to see, as you're messing with a state machine that isn't under your control. > Regarding the inits sequence and dependencies: > - The second init is guaranteed to be called after the first one, since > start_kernel()->init_IRQ() is called before platform drivers init. There is no such requirements that holds for secondary interrupt controllers. > - During the second init, the dependency with the hwspinlock driver is > implemented correctly : it makes use of defered probe when needed. Then do the right thing all the way: move your favourite toy irqchip to being a proper device driver, use the right abstraction, and stop piling ugly hacks on top of each other. Other irqchip drivers do that just fine (all GPIO irqchips, for example), and most drivers are already able to defer their probe routine. > I understand that this patch is 'surprising' but I hope that my > explanations justify its implementation. Surprising is not the word I'd have used. The explanations do not justify anything, as all you're saying is "it is the way it is because it is the way it is". So please fix your irqchip driver properly, in a way that is maintainable in the long term, using the abstractions that are available. If such abstractions are not good enough, please explain what you need and we'll work something out. Thanks, M. -- Jazz is not dead, it just smell funny. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel