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 7F3E5C433F5 for ; Thu, 4 Nov 2021 14:42:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6786261106 for ; Thu, 4 Nov 2021 14:42:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231168AbhKDOpe (ORCPT ); Thu, 4 Nov 2021 10:45:34 -0400 Received: from mail.kernel.org ([198.145.29.99]:57030 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230409AbhKDOp1 (ORCPT ); Thu, 4 Nov 2021 10:45:27 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 78512611EE; Thu, 4 Nov 2021 14:42:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1636036969; bh=n0CzbhBUbgevxQWjjtb4wBTU1nHZxIHCQLQyJbqJ9So=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=j+WEmNhZNbzEI0icdK6TZFy0yBqKjsQ9BHQtFlWfWhbEhe7nD79OmcPWML2fi2KKF 7znem49g6JuW+JiTJtg2673yZhArNI8kpVmKmRAGuJlFWm4kHX8eyk2UwQCDBUxrSK RBL531RVUoFOiu8k3q9Hf26RIhJlCoGi52CoMEfo/g6RdG+76Kuk5JsYJhdLhVqPjR HJ/RfxYX51nbosuSDGEgqq+RcruTcAzpyXHcI8RTe6KySjYIK1fcPfpaI25vFgws+j ORLh2dJUnHtnYTBF0vlgHD6zO4Fs1RiiAlyzZwYiMxJD/0X2fuk+3ciDXAELkthKSg vxEM7DW+efsTA== Received: by mail-ed1-f52.google.com with SMTP id b15so2961836edd.7; Thu, 04 Nov 2021 07:42:49 -0700 (PDT) X-Gm-Message-State: AOAM533UN0CSsXc/07VNBk9yCJqmKir73d3eBzYe1IQma1s6F61cuDqi BUMNAQxa2AQaWMWe4Ya5Jv9CWaW3xvBZADJRuQ== X-Google-Smtp-Source: ABdhPJy6P/8T2QkdFCza1adxwbd7SLzZ9Y6/ekNEpVkworwq423dcdxpGyT6AJAuyvFvRLeBH+tBIMAGgd/eVM0CKSc= X-Received: by 2002:a17:907:a411:: with SMTP id sg17mr38835733ejc.84.1636036967479; Thu, 04 Nov 2021 07:42:47 -0700 (PDT) MIME-Version: 1.0 References: <20211029200319.23475-1-jim2101024@gmail.com> <20211029200319.23475-8-jim2101024@gmail.com> In-Reply-To: From: Rob Herring Date: Thu, 4 Nov 2021 09:42:34 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators To: Jim Quinlan Cc: Jim Quinlan , PCI , Nicolas Saenz Julienne , Mark Brown , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , Florian Fainelli , Lorenzo Pieralisi , =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= , Bjorn Helgaas , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 2, 2021 at 5:36 PM Jim Quinlan wrote: > > On Tue, Nov 2, 2021 at 12:00 PM Rob Herring wrote: > > > > On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > > device, be it a switch or an endpoint. We want to be able to turn on/off > > > any regulators for that device. Control of regulators is needed because of > > > the chicken-and-egg situation: although the regulator is "owned" by the > > > device and would be best handled by its driver, the device cannot be > > > discovered and probed unless its regulator is already turned on. > > > > I think this can be done in a much more simple way that avoids the > > prior patches using the pci_ops.add_bus() (and remove_bus()) hook. > > add_bus is called before the core scans a child bus. In the handler, you > > just need to get the bridge device, then the bridge DT node, and then > > get the regulators and enable. > Hi Rob, > In reply to my bindings commit you wanted to put the "xxx-supply" > property(s) under the > bridge node rather than under the pci-ep node. This not only makes > sense but also removes > the burden of prematurely creating the struct device *ptr as the > bridge device has > already been created. > > However, there is still an issue: if the pcie-link is not > successful, we want the bus enumeration > to stop and not read the vendor/dev id of the EP. Our controller has > the disadvantage of causing > an abort when accessing config space when the link is not established. Other > controllers kindly return 0xffffffff as the data. > > Doing something like this gets around the issue: > > static struct pci_bus *pci_alloc_child_bus(...) > { > /* ... */ > add_dev: > /* ... */ > if (child->ops->add_bus) { > ret = child->ops->add_bus(child); > + if (ret == -ENOLINK) > + return NULL; > if (WARN_ON(ret < 0)) > dev_err(&child->dev, "failed to add bus: %d\n", ret); > } > > Is this acceptable? Other suggestions? Acceptable yes once we agree on error code to return. I'd just do -ENODEV. > > Given we're talking about standard properties in a standard (bridge) > > node, I think the implementation for .add_bus should be common > > (drivers/pci/of.c). It doesn't scale to be doing this in every host > > bridge driver. > Are you saying that the bridge DT node should have a property such as > "get-and-turn-on-subdev-regulators;" which would invoke what I'm now > calling brcm_pcie_add_bus()? No! Define a common function that host drivers can opt in to by setting their .add_bus() hook to or calling from their own add_bus function. Ideally, it would work on Rockchip too as it's the same supplies. However, that would require some reworking of the link initialization and PERST handling. As Bjorn has mentioned, all that should be per RP, not per host bridge anyways. I'm taking it one step further and saying it should be per PCI bridge. Hikey for example needs PERST handling on bridges behind a switch. Rob 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 AA74AC433EF for ; Thu, 4 Nov 2021 14:44:49 +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 7864E611C4 for ; Thu, 4 Nov 2021 14:44:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 7864E611C4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=f4OCjgt+/yggTjQxOUTNsi9+MTJ7O0vKJjfpegZ94AA=; b=39rtdz7PVOZ7W6 9E+8SXX3FscUV4+ch17RKZAGTk6e1HRjvHVRWX9hRygMCKO6zZXZ8+feW++UtUXCVuBfTobhg32jD BraLCCpFPNIoNVbs0k9CSG1z7ZflYaKFkjuK6/1SplC+CAwGG/r0S9YunlHnOBDwmITclBfLObiyl IxaELaedymGdyUMb/t2dHInd3NKhFHLcxwZ6fWLCIVi0zEHHJqRAcCdJWFqnlV30JKRVKtPfniS4U hjPc4FmO7oUNfkhAkc0iyII5zMks12p1c2ggrVytDI1uHNxHiwNr1IFlX3WvfyC72a6dGjCeH7KPK niu0wl2LHhoD0qpz5D1A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1midx9-0097gw-Az; Thu, 04 Nov 2021 14:42:55 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1midx6-0097g4-0R; Thu, 04 Nov 2021 14:42:53 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 87A056056B; Thu, 4 Nov 2021 14:42:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1636036970; bh=n0CzbhBUbgevxQWjjtb4wBTU1nHZxIHCQLQyJbqJ9So=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=rgS8QsbIqVww1+pDy+UIYfu+y4m4zQF/n7Ee9iCF+9l1HxkYR/wvyEtSF+vDkqRIx Upm4Q3PfUmoDBBdaoOLYDpnxWdIgvGLmP66r5bERr1/MDplQbIU3Kdn8nySVcZheRI JjluZv/QQmBx1fu6tJ5HngLGp+L1CkECzwQUcmvx+XoBMsV6iOL5v0c0OTpj9phxfP AiCpS8QOGS4yfaCGMM6B92AIp+QfHzH2SHVOtJYhV3X4jKtICsiPU0b5BGtEsapPPK tf2YGy2L4lN1BuCu6AeLq3V1roM4w5WtK4W4cEXnKjigzYtIesWAhPvSRUxETGaT+k ZeswK0g53E1EA== Received: by mail-ed1-f49.google.com with SMTP id g10so22099635edj.1; Thu, 04 Nov 2021 07:42:50 -0700 (PDT) X-Gm-Message-State: AOAM531f/Ts7IEHEZWVmKAtTKcSgZOc6pdHNpK8NGx3OQ/xY8j+zXFDm XSfCE8sjbXlwUrve3pcOzaozP9xfy5tLFH1yLQ== X-Google-Smtp-Source: ABdhPJy6P/8T2QkdFCza1adxwbd7SLzZ9Y6/ekNEpVkworwq423dcdxpGyT6AJAuyvFvRLeBH+tBIMAGgd/eVM0CKSc= X-Received: by 2002:a17:907:a411:: with SMTP id sg17mr38835733ejc.84.1636036967479; Thu, 04 Nov 2021 07:42:47 -0700 (PDT) MIME-Version: 1.0 References: <20211029200319.23475-1-jim2101024@gmail.com> <20211029200319.23475-8-jim2101024@gmail.com> In-Reply-To: From: Rob Herring Date: Thu, 4 Nov 2021 09:42:34 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators To: Jim Quinlan Cc: Jim Quinlan , PCI , Nicolas Saenz Julienne , Mark Brown , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , Florian Fainelli , Lorenzo Pieralisi , =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= , Bjorn Helgaas , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , open list X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211104_074252_114698_20577F79 X-CRM114-Status: GOOD ( 31.86 ) 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 Tue, Nov 2, 2021 at 5:36 PM Jim Quinlan wrote: > > On Tue, Nov 2, 2021 at 12:00 PM Rob Herring wrote: > > > > On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote: > > > This Broadcom STB PCIe RC driver has one port and connects directly to one > > > device, be it a switch or an endpoint. We want to be able to turn on/off > > > any regulators for that device. Control of regulators is needed because of > > > the chicken-and-egg situation: although the regulator is "owned" by the > > > device and would be best handled by its driver, the device cannot be > > > discovered and probed unless its regulator is already turned on. > > > > I think this can be done in a much more simple way that avoids the > > prior patches using the pci_ops.add_bus() (and remove_bus()) hook. > > add_bus is called before the core scans a child bus. In the handler, you > > just need to get the bridge device, then the bridge DT node, and then > > get the regulators and enable. > Hi Rob, > In reply to my bindings commit you wanted to put the "xxx-supply" > property(s) under the > bridge node rather than under the pci-ep node. This not only makes > sense but also removes > the burden of prematurely creating the struct device *ptr as the > bridge device has > already been created. > > However, there is still an issue: if the pcie-link is not > successful, we want the bus enumeration > to stop and not read the vendor/dev id of the EP. Our controller has > the disadvantage of causing > an abort when accessing config space when the link is not established. Other > controllers kindly return 0xffffffff as the data. > > Doing something like this gets around the issue: > > static struct pci_bus *pci_alloc_child_bus(...) > { > /* ... */ > add_dev: > /* ... */ > if (child->ops->add_bus) { > ret = child->ops->add_bus(child); > + if (ret == -ENOLINK) > + return NULL; > if (WARN_ON(ret < 0)) > dev_err(&child->dev, "failed to add bus: %d\n", ret); > } > > Is this acceptable? Other suggestions? Acceptable yes once we agree on error code to return. I'd just do -ENODEV. > > Given we're talking about standard properties in a standard (bridge) > > node, I think the implementation for .add_bus should be common > > (drivers/pci/of.c). It doesn't scale to be doing this in every host > > bridge driver. > Are you saying that the bridge DT node should have a property such as > "get-and-turn-on-subdev-regulators;" which would invoke what I'm now > calling brcm_pcie_add_bus()? No! Define a common function that host drivers can opt in to by setting their .add_bus() hook to or calling from their own add_bus function. Ideally, it would work on Rockchip too as it's the same supplies. However, that would require some reworking of the link initialization and PERST handling. As Bjorn has mentioned, all that should be per RP, not per host bridge anyways. I'm taking it one step further and saying it should be per PCI bridge. Hikey for example needs PERST handling on bridges behind a switch. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel