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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7F9B6C433F5 for ; Wed, 29 Sep 2021 03:57:38 +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 4D39F613D0 for ; Wed, 29 Sep 2021 03:57:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 4D39F613D0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sholland.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:Subject:From:References:Cc:To:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=6wtO5lwL2v67QJuly/V8KiVIeuw67MNKyJiz5PMuKKA=; b=2FtFPbcmQzb5kD3NXJ2osYGN3T ucoTt/wf0nGnrBCgxQRkpFwzjYcwsEZaTrn3S/UK+qlK7WumL6AkhkYqxYj5wRkJEK07nUk8AdhwZ jlhuloBG3ygMR7pCsTS3bTEUhZ7wi3Y2cGlliNkPlmxQ9Xsts1TTkAJabiFmCOAb//UFcr3BMBVXT VqWiYwLvUrVs0pUJxCepF5z50SBLI/AnFUtrYxUNqJ9Xh493vrisfDml1zJuQWNXB4yrMNXq1vZvA CoSgQgLnOu7zrZ3hXuFEJM1949quWSu/NLrS7proWrh0+P/ZlcL0FwF0qA3qBimojf124wJBtMdT8 TXcIzZIg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mVQfz-009r8K-Gj; Wed, 29 Sep 2021 03:54:35 +0000 Received: from new2-smtp.messagingengine.com ([66.111.4.224]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mVQfw-009r7r-13 for linux-arm-kernel@lists.infradead.org; Wed, 29 Sep 2021 03:54:33 +0000 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailnew.nyi.internal (Postfix) with ESMTP id 30E56580E57; Tue, 28 Sep 2021 23:54:29 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Tue, 28 Sep 2021 23:54:29 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sholland.org; h= to:cc:references:from:subject:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s=fm3; bh=/ fz2xDmGvnVOv6nmGoPaDrmEwWr82udufb63/AmiMe8=; b=YfDdo97RhokBFNvMz ImWhirnQy0nIl1TaEPKmE1HRb9R1rMZu8yR2Q93McjWprNkNNr3EzfXcV5yOoI76 zNk02KygJKgfr9JtpsUnaVoPCZ974tSBbS8YfzyeN6Ph6CIwN53eLaJlcnVMrauj hy2uvDNglliwdAkZ9154x4UfvhQTpg8QCDJEw0cmzGkxunCnYQEUDUv/MBEHLmNB z1VX8+J0TqD5DdRmqOn4PhSPD886e8/tnDMKzEG3b51ln3hxrlQMmCNdraSnlSFE XFO0dfo9epTcMG3jHtFh5gVGUbJ/J1VukF8RL+aHYNGSVPPwpeuHNq/CJ2XU8HfD l5n/Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=/fz2xDmGvnVOv6nmGoPaDrmEwWr82udufb63/AmiM e8=; b=oup3cklh5OZrUL36ojjnMb5SRVL1GVBtY+Fx30n9bu7dVWrCwDZNwUz2R BVz4huTwpafnlFN3Iqul/vUPVemG7hQ6MCKDtXHFeEUx+vM504FDd6JZ+OXcFZ56 pnVkBohD07XWG4vzquXtQzkpS3nPFWxRvO36V7iOeKNib39UrSqqkXJJdxrFrCCi X14H3KM9MuNeVBPc8ok4QpNZtIqME6gnS8p/Ea3EFkaCmVeSYbc2VameVKfdElxF EP7LnjW5x2EvSLmuslnAyw4DLKq8zScUwTvoymWY9HXCisme/NF8OeM/FI53VM+e Jtwg77n5Zdl19Yz4+Z756r4M+CmVg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudekuddgjeegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepvfhfhffukffffgggjggtgfesthekredttdefjeenucfhrhhomhepufgrmhhu vghlucfjohhllhgrnhguuceoshgrmhhuvghlsehshhholhhlrghnugdrohhrgheqnecugg ftrfgrthhtvghrnhepvddttdejieduudfgffevteekffegffeguddtgfefkeduvedukeff hedtfeevuedvnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepshgrmhhuvghlsehshhholhhlrghnugdrohhrgh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 28 Sep 2021 23:54:27 -0400 (EDT) To: Maxime Ripard Cc: Chen-Yu Tsai , Jernej Skrabec , Rob Herring , Michael Turquette , Stephen Boyd , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org References: <20210901053951.60952-1-samuel@sholland.org> <20210903145013.hn6dv7lfyvfys374@gilmour> <4a187add-462b-dfe4-868a-fdab85258b8d@sholland.org> <20210909084538.jeqltc7b3rtqvu4h@gilmour> <20210928090625.rq3atiaejaq5kcbx@gilmour> From: Samuel Holland Subject: Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver Message-ID: Date: Tue, 28 Sep 2021 22:54:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux ppc64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <20210928090625.rq3atiaejaq5kcbx@gilmour> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210928_205432_279985_82D033AD X-CRM114-Status: GOOD ( 49.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Maxime, Thanks for your reply. On 9/28/21 4:06 AM, Maxime Ripard wrote: > On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote: >> On 9/9/21 3:45 AM, Maxime Ripard wrote: >>> On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote: >>>> On 9/3/21 9:50 AM, Maxime Ripard wrote: >>>>> And since we can register all those clocks at device probe time, we >>>>> don't really need to split the driver in two (and especially in two >>>>> different places). The only obstacle to this after your previous series >>>>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe >>>>> functions public, but that can easily be fixed by moving their >>>>> definition to include/linux/clk/sunxi-ng.h >>>> >>>> Where are you thinking the clock definitions would go? We don't export >>>> any of those structures (ccu_mux, ccu_common) or macros >>>> (SUNXI_CCU_GATE_DATA) in a public header either. >>> >>> Ah, right... >>> >>>> Would you want to export those? That seems like a lot of churn. Or would >>>> we put the CCU descriptions in drivers/clk/sunxi-ng and export a >>>> function that the RTC driver can call? (Or some other idea?) >>> >>> I guess we could export it. There's some fairly big headers in >>> include/linux/clk already (tegra and ti), it's not uAPI and we do have >>> reasons to do so, so I guess it's fine. >>> >>> I'd like to avoid having two drivers for the same device if possible, >>> especially in two separate places. This creates some confusion since the >>> general expectation is that there's only one driver per device. There's >>> also the fact that this could lead to subtle bugs since the probe order >>> is the link order (or module loading). >> >> I don't think there can be two "struct device"s for a single OF node. > > That's not what I meant, there's indeed a single of_node for a single > struct device. If we dig a bit into the core framework, the most likely > scenario is that we would register both the RTC and clock driver at > module_init, and with the device already created with its of_node set > during the initial DT parsing. > > We register our platform driver using module_platform_driver, which > expands to calling driver_register() at module_init(), setting the > driver bus to the platform_bus in the process (in > __platform_driver_register()). > > After some sanity check, driver_register() calls bus_add_driver(), which > will call driver_attach() if drivers_autoprobe is set (which is the > default, set into bus_register()). > > driver_attach() will, for each device on the platform bus, call > __driver_attach(). If there's a match between that device and our driver > (which is evaluated by platform_match() in our case), we'll call our > driver probe with that device through driver_probe_device(), > __driver_probe_device() and finally really_probe(). > > However, at no point in time there's any check about whether that device > has already been bound to a driver, nor does it create a new device for > each driver. I would expect this to hit the: if (dev->driver) return -EBUSY; in __driver_probe_device(), or fail the "if (!dev->driver)" check in __driver_attach() for the async case, once the first driver is bound. > So this means that, if you have two drivers that match the > same device (like our clock and RTC drivers), you'll have both probe > being called with the same device, and the probe order will be defined > by the link order. Worse, they would share the same driver_data, with > each driver not being aware of the other. This is incredibly fragile, > and hard to notice since it goes against the usual expectations. > >> So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe" >> function would have to be called from the RTC driver. > > No, it would be called by the core directly if there's a compatible to > match. > >> Since there has to be cooperation anyway, I don't think there would be >> any ordering problems. > > My initial point was that, with a direct function call, it's both > deterministic and obvious. I believe I did what you are suggesting for v2. From patch 7: --- a/drivers/rtc/rtc-sun6i.c +++ b/drivers/rtc/rtc-sun6i.c @@ -683,6 +684,10 @@ static int sun6i_rtc_probe(struct platform_device *pdev) chip->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(chip->base)) return PTR_ERR(chip->base); + + ret = sun6i_rtc_ccu_probe(&pdev->dev, chip->base); + if (ret) + return ret; } platform_set_drvdata(pdev, chip); >>> And synchronizing access to registers between those two drivers will be >>> hard, while we could just share the same spin lock between the RTC and >>> clock drivers if they are instanciated in the same place. >> >> While the RTC driver currently shares a spinlock between the clock part >> and the RTC part, there isn't actually any overlap in register usage >> between the two. So there doesn't need to be any synchronization. > > I know, but this was more of a social problem than a technical one. Each > contributor and reviewer in the future will have to know or remember > that it's there, and make sure that it's still the case after any change > they make or review. > > This is again a fairly fragile assumption. Yeah, I agree that having a lock that is only sometimes safe to use with certain registers is quite fragile. Would splitting the spinlock in rtc-sun6i.c into "losc_lock" (for the clock provider) and "alarm_lock" (for the RTC driver) make this distinction clear enough? Eventually, I want to split up the struct between the clock provider and RTC driver so it's clear which members belong to whom, and there's no ugly global pointer use. Maybe I should do this first? Regards, Samuel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel