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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 816E0C433EF for ; Fri, 8 Jul 2022 20:01:06 +0000 (UTC) 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:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=mYWfUwPJ4y+69iWxaDNsYomsYUueTsEz2ne8XgR349I=; b=VM9ImSBVo89dB+ 3qsYp/xpT8SLta7f7lOJg7YkM2HrNKEnl9M5ruMcsRrWlAdlaJCBomao6waV8thCcaUxC+65+oSmu jKHI3mNLMsQ0TG6hzFuSErnXfphHW+l+kH64nUvuylIbsk2zItuE9o4+CKDAz8ACH+qM+LLDmQ/Qq K3ByCnaS4Id3D2bAJ4Fq01Ogk1Vtywusq02bVTh/v50jBSVGiVeOm7GISAxPle5AeXoVJKKNXuJ5x Cxyi91cjUfpMpt8jlhDAgYhH59At8O4vz3btlphEmegs8f4HGRjZB8zuhGwmYtnssY9YheljDfWNH DiiHJXwKJn4wLc+eM38A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o9u8j-005ihF-3m; Fri, 08 Jul 2022 19:59:49 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o9u8f-005igc-Pd; Fri, 08 Jul 2022 19:59:47 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 47DB1B82915; Fri, 8 Jul 2022 19:59:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A9456C341C0; Fri, 8 Jul 2022 19:59:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657310383; bh=+AjKQiF4LiPg6oDcpkDj0Ni0dwsq4oPqmWlRHGeGSZ4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Vf43t8YXcF3M7+Fm1x/UndCN9D68h7a43YEztdFC5jWE8HfpGO5pyrmhDUXEXh7dA qrzG6nqr0eWq1RH/UPJqvzBeNVwKpNjR2Ou0yXZXEsmNAx7dpcFdLKZEef+vWU489s krfXv1B+WPRGqsFfggvICptBwv2rsxjV1WQXFp/Jhr9a3TlcsnRF7caTtFX0L++/ZV pkATJBeQV8reB39cUkPHd+5og7M+6w2VFSd8PYOyeHc1lmhAG/8EdNztKJc0Viyjh/ uUO09x/5Xo4oZbk4rVU3Q+pBpYGnPAt0jLFRXB/bZ3K/pJBzGmB8j5NEb72yFDbCg8 FI+aMvONTtfHA== Date: Fri, 8 Jul 2022 14:59:40 -0500 From: Bjorn Helgaas To: Jim Quinlan Cc: Jim Quinlan , "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" , Nicolas Saenz Julienne , Bjorn Helgaas , Lorenzo Pieralisi , Cyril Brulebois , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , Florian Fainelli , Lorenzo Pieralisi , Rob Herring , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , open list Subject: Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Message-ID: <20220708195940.GA369714@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220708_125946_131601_9DDC8362 X-CRM114-Status: GOOD ( 36.94 ) 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 On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote: > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas wrote: > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote: > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas wrote: > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote: > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function > > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will > > > > > be called indirectly by pci_host_probe() as opposed to the host driver > > > > > invoking it directly. > > > > > > > > > > Some code that was executed after the PCIe linkup is now placed so that it > > > > > executes prior to linkup, since this code has to run prior to the > > > > > invocation of pci_host_probe(). > > > > > > > > This says we need to move some code from brcm_pcie_setup() to > > > > brcm_pcie_linkup(), but not *why* we need to do that. > > > I will elaborate in the commit message. > > > > > > > > In brcm_pcie_resume(), they're called together: > > > > > > > > brcm_pcie_resume > > > > brcm_pcie_setup > > > > brcm_pcie_linkup > > > > > > > > In the probe path, they're not called together, but they're in the > > > > same order: > > > > > > > > brcm_pcie_probe > > > > brcm_pcie_setup > > > > pci_host_probe > > > > ... > > > > brcm_pcie_add_bus # bus->ops->add_bus > > > > brcm_pcie_linkup > > > > > > > > Is there something that must happen *between* them in the probe path? > > > > > > Yes. In the probe() case, we must do things in this order: > > > > > > 1. brcm_pcie_setup() > > > 2. Turn on regulators > > > 3. brcm_pcie_linkup() > > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus: > > > > brcm_pcie_add_bus # bus->ops->add_bus > > pci_subdev_regulators_add_bus > > regulator_bulk_enable # turn on regulators > > brcm_pcie_linkup > > > > > Since the voltage regulators are turned on during enumeration, > > > pci_host_probe() must be invoked prior to 3. Before regulators, we > > > did not care. > > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power > > for downstream devices must always be on. > > > > > In the resume case, there is no enumeration of course but our driver > > > has a handle to the regulators and can turn them on/off w/o help. > > > > And I guess we don't need brcm_pcie_setup() in the resume path because > > suspend turns off power only for downstream devices, not for the root > > port itself, so the programming done by brcm_pcie_setup() doesn't need > > to be done again. > > I'm not sure I understand what you are saying -- brcm_pcie_setup() is > called by brcm_pcie_resume() > because it is needed. brcm_pcie_setup() isn't concerned with power it > just does the preparation > required before attempting link-up. Oh, sorry, I totally misread that. But I wonder about the fact that probe and resume do these in different orders: brcm_pcie_probe brcm_pcie_setup # setup pci_host_probe ... brcm_pcie_add_bus pci_subdev_regulators_add_bus regulator_bulk_enable # regulators on brcm_pcie_linkup # linkup brcm_pcie_resume regulator_bulk_enable # regulators on brcm_pcie_setup # setup brcm_pcie_linkup # linkup Maybe pci_subdev_regulators_add_bus() could be done directly from brcm_pcie_probe() instead of in brcm_pcie_add_bus()? If the regulators must be directly under the root port node in DT, it seems like it would be reasonable to look for them in the probe path, which seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and pcie-rockchip-host.c do. Or maybe brcm_pcie_resume() should enable the regulators after brcm_pcie_setup() so it's the same order as the probe path? Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel