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 3C106C43334 for ; Wed, 20 Jul 2022 21:35:32 +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:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OXdSVdd6FHk1aCvZy7bkVIZuDV9ohmryElfrdemn8YE=; b=ZxDlrx1u3psgEO guJnzd+yyQD/nsLYFr/gD2s7dniamyHlEPdAZICtrLVCrDbjHTbUpy+agN2SkUPt9k0dIHBOb90q8 uddT38Oth8nGTS3xb9H0lXquII8xZZSrZ+e2/9h5SaiObs0SmuQMPYlfEq4VCWXlkt5azUQnjjUMD njcZ65rrZZnpJLzxvYLuAu/rMpSdmGE+8K0RDEuzkdiBoCsQFG7neDDbyWM9x2aqgjyLNyIjdjoDw kyDNb6LtemOebEwCa0VYLHP5TwcsvHJt86rW6blThIVnKkpj/LxAJyDbd93YI60WEH9Jt/0zlICKR fmiLKw38mzY3ajx3qc3Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEHKo-00BXjE-Md; Wed, 20 Jul 2022 21:34:24 +0000 Received: from mail-pg1-x535.google.com ([2607:f8b0:4864:20::535]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEHKh-00BXYI-0y; Wed, 20 Jul 2022 21:34:17 +0000 Received: by mail-pg1-x535.google.com with SMTP id 23so17570877pgc.8; Wed, 20 Jul 2022 14:34:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=uMfaOA0dCEc0Y+KHogJz6qUXZ7HQhYeRsFa6pE/6Lrg=; b=I4S2BEsq+qf/F8h6DXcChhNMrWg2GnRu4T21dhs2/9eKsdDDg4ILkGPrKSi6C/QHOp R5Jf4nK3KkRiw9ZAwZOWU3GaXvNpJdD50gb6RAqEx97hf87X3wncNfNZYyVDLyoG8Wd7 JV8ppRJ1na+MyCMA1khbMIOFJwpt7bbTzriz5noYklCdLdRqToyAwKrJP1/9mli8Sm3M Q7OLuu7ZrDOTDm0A2NxZEWLHL5qGtRmlPpWycmSoiCW4rDJAbS26dN42vfnXv4/nzRtI n1Vd0sc+lTN45FkCLqUELvXK7aNwQcSrn/O005csGodfGCAs3+r253sZl2/ObBZ2JT+j IYxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=uMfaOA0dCEc0Y+KHogJz6qUXZ7HQhYeRsFa6pE/6Lrg=; b=1L0aXN277+JW9fFTO7F1wEwOZit49ndw1XxDZ9Hc4VwwDBci89TB1ucfOQCOWf1pi/ 2mQhm66KUdq181zkqNSh0wnDvQuL/eppdX2O3NbOJVpHUiiivfQ0GkvQWxI5r1kh7kg8 eXLgIEnMCu8Wh0UBvK1a0rdkFyUFbfjSmeG/zAYz21acVKojijAXLdasubcXsrzBrdwt cUvya46NjYK9/qkjd0vZw1OJyEu/ndAFVAyIFgIGmXiWwXwYo3pji9O1Y3R0BBl3giiQ vRtCWAtiv2wAS4MzGYZGfCMTGLb/G6790k0wH5RcQ+Rp3K3EppPPxUqekDcw05nvET5N 8LzQ== X-Gm-Message-State: AJIora97DKWt/8ywTiaG4gCMMW4O2IngHraOsfnfSWmsqxfKTDzLaaVS OtcoDn8NCJYRRbwiH/gdCMXml3WtA9U= X-Google-Smtp-Source: AGRyM1skqVnD1rvn6wErTlZ7qIMObRpv1ckGwpRu3TdyNjEQr+sq4mOpzbxayS902JpSkLbBS9YNjQ== X-Received: by 2002:a63:8743:0:b0:41a:6f6b:db7b with SMTP id i64-20020a638743000000b0041a6f6bdb7bmr3116562pge.594.1658352851261; Wed, 20 Jul 2022 14:34:11 -0700 (PDT) Received: from [10.67.48.245] ([192.19.223.252]) by smtp.googlemail.com with ESMTPSA id r3-20020a63a543000000b0040d1eb90d67sm12157419pgu.93.2022.07.20.14.34.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 Jul 2022 14:34:10 -0700 (PDT) Message-ID: <3cf519d2-9ffa-1337-935e-4a617a0eab16@gmail.com> Date: Wed, 20 Jul 2022 14:34:06 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Content-Language: en-US To: Rob Herring , Jim Quinlan Cc: Bjorn Helgaas , 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 , =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , open list References: <20220719200332.GA1552587@bhelgaas> From: Florian Fainelli In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220720_143415_150303_A8DFFF3B X-CRM114-Status: GOOD ( 40.51 ) 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 7/20/22 09:18, Rob Herring wrote: > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan wrote: >> >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas wrote: >>> >>> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote: >>>> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas wrote: >>>>> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote: >>>>>> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas wrote: >>>>>>> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote: >>>>>>>> Currently, the function does the setup for establishing PCIe link-up >>>>>>>> with the downstream device, and it does the actual link-up as well. >>>>>>>> The calling sequence is (roughly) the following in the probe: >>>>>>>> >>>>>>>> -> brcm_pcie_probe() >>>>>>>> -> brcm_pcie_setup(); /* Set-up and link-up */ >>>>>>>> -> pci_host_probe(bridge); >>>>>>>> >>>>>>>> This commit splits the setup function in two: brcm_pcie_setup(), which only >>>>>>>> does the set-up, and brcm_pcie_start_link(), which only does the link-up. >>>>>>>> The reason why we are doing this is to lay a foundation for subsequent >>>>>>>> commits so that we can turn on any power regulators, as described in the >>>>>>>> root port's DT node, prior to doing link-up. >>>>>>> >>>>>>> All drivers that care about power regulators turn them on before >>>>>>> link-up, but typically those regulators are described directly under >>>>>>> the host bridge itself. >>>>>> >>>>>> Actually, what you describe is what I proposed with my v1 back in Nov 2020. >>>>>> The binding commit message said, >>>>>> >>>>>> "Quite similar to the regulator bindings found in >>>>>> "rockchip-pcie-host.txt", this allows optional regulators to be >>>>>> attached and controlled by the PCIe RC driver." >>>>>> >>>>>>> IIUC the difference here is that you have regulators described under >>>>>>> Root Ports (not the host bridge/Root Complex itself), so you don't >>>>>>> know about them until you've enumerated the Root Ports. >>>>>>> brcm_pcie_probe() can't turn them on directly because it doesn't know >>>>>>> what Root Ports are present and doesn't know about regulators below >>>>>>> them. >>>>>> >>>>>> The reviewer's requested me to move the regulator node(s) >>>>>> elsewhere, and at some point later it was requested to be placed >>>>>> under the Root Port driver. I would love to return them under the >>>>>> host bridge, just say the word! >>>>> >>>>> Actually, I think my understanding is wrong. Even though the PCI core >>>>> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows >>>>> about it and should be able to look up the regulators and turn them >>>>> on. >>>> >>>> One can do this with >>>> regulator_bulk_get(NULL, ...); >>>> >>>> However, MarkB did not like the idea of a driver getting the >>>> regulator from the global DT namespace [1]. >>>> >>>> For the RC driver, one cannot invoke regulator_bulk_get(dev, ...) >>>> if there is not a direct child regulator node. One needs to use the >>>> Port driver device. The Port driver device does not exist at this >>>> point unless one tries to prematurely create it; I tried this and it >>>> was a mess to say the least. >>>> >>>>> Can you dig up the previous discussion about why the regulators need >>>>> to be under the Root Port and why they can't be turned on before >>>>> calling pci_host_probe()? >>>> >>>> RobH did not want the regulators to be under the RC as he said their >>>> DT location should resemble the HW [2]. The consensus evolved to >>>> place it under the port driver, which can provide a general >>>> mechanism for turning on regulators anywhere in the PCIe tree. >>> >>> I don't want to redesign this whole thing. I just want a crisp >>> rationale for the commit log. >>> >>> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194, >>> tegra, rockchip-host) have regulators for downstream PCIe power >>> directly under the RC. If putting the regulators under an RP instead >>> is the direction of the future, I guess that might be OK, and I guess >>> the reasons are: >>> >>> 1) Slot or device power regulators that are logically below the RP >>> should be described that way in the DT. >>> >>> 2) Associating regulators with a RP allows the possibility of >>> selectively controlling power to slots/devices below the RP, >>> e.g., to power down devices below RP A when suspending while >>> leaving wakeup devices below RP B powered up. >>> >>> I think in your case the motivating reason is 2). >>> >>> Your commit log for "Add mechanism to turn on subdev regulators" >>> suggests that you want some user control of endpoint power, e.g., via >>> sysfs, but I don't see that implemented yet except possibly via a >>> "remove" file that would unbind the driver and remove the entire >>> device. >> Hi Bjorn, >> >> Initially we wanted to (a) turn on any regulator found under the RC >> node and (b) allow the possibility of the regulator to control the >> EP's power. From the feedback, we realized early on that neither of >> these were going to fly, so we conceded both requests and just wanted >> to turn on standard PCIe regulators. Upon reading the aforementioned >> commit message I realize that there are a couple of leftover sentences >> from these early days that must be removed. >> >> I think when I submitted v1 of the original series that only the >> rockchip driver had regulators under the RC. And my recollection was >> that this was accepted but there was apprehension of this turning into >> the "standard" way of turning on such regulators, as the location of >> the regulator nodes was in question. >> >> In short, I would be quite content to follow the existing examples. > > The existing examples are, in general, incomplete and only work for > the simplest cases. > > There's 2 cases to consider here. The first is standard slots with > standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is > either non-standard slots or just soldered down devices which could > have any number of device specific resources. In the latter case, > those resources need to go into the node for the device. For the > former case (which we are discussing here), putting the resources in > the upstream (side of the link) node is fine. That's the root port > node(s) or switch port nodes. However, since most host bridges are a > single RP and don't put the RP node in DT, we've ended up with these > properties in host bridge nodes. That's fine as long as it's a single > RP and device. When it is not, we need to do something different. The > only way this scales is putting resources in the port nodes as those > are what have a 1:1 relationship to slots. If that's supported, then > the simple cases are also easily supported with if the resources are > not found in the port node/device, then look for them in the parent > node. That's also the path for how we get the handling of PERST out of > every host bridge driver. This has me confused now, are you suggesting that we pursue what Jim has put together here as a series which describes the regulators in the PCIe end-point device DT node, or that given that we have a single RC single RP configuration it is somewhat acceptable to describe regulators in the PCIe RC node? -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel