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=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 E366BC43381 for ; Fri, 8 Mar 2019 15:31:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BD41E208E4 for ; Fri, 8 Mar 2019 15:31:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726648AbfCHPbA (ORCPT ); Fri, 8 Mar 2019 10:31:00 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:60446 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726344AbfCHPa7 (ORCPT ); Fri, 8 Mar 2019 10:30:59 -0500 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 Cc: Thomas Gleixner , Jason Cooper , Maxime Coquelin , Alexandre TORGUE , "linux-kernel@vger.kernel.org" , "linux-stm32@st-md-mailman.stormreply.com" , "linux-arm-kernel@lists.infradead.org" , Benjamin GAIGNARD 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") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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.