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 ADC91C433FE for ; Tue, 2 Nov 2021 22:36:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8E06061058 for ; Tue, 2 Nov 2021 22:36:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230155AbhKBWi5 (ORCPT ); Tue, 2 Nov 2021 18:38:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41448 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229736AbhKBWi4 (ORCPT ); Tue, 2 Nov 2021 18:38:56 -0400 Received: from mail-wr1-x432.google.com (mail-wr1-x432.google.com [IPv6:2a00:1450:4864:20::432]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2CBC2C061203 for ; Tue, 2 Nov 2021 15:36:21 -0700 (PDT) Received: by mail-wr1-x432.google.com with SMTP id r8so722694wra.7 for ; Tue, 02 Nov 2021 15:36:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mlauy3vNd9KbBeQ/P3IhRFrTYk9NsjbV35NI/DoCFQQ=; b=LhJzMSC03GD6kwFoHkcv0WlZV8/LiMo3e6aUlmAyFmOZ03I0K/GJ7F40Hc1b+LDbys tYwF6MfkMGi4kGmcYAQrVZ0wm6bqC4JEDNJsKiWBlgYmK/SLRCAp/vSUag2Lgc2mUTbK FwoLgRPulKd9iQCBHERoM+IfA8GraokFzdvOU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mlauy3vNd9KbBeQ/P3IhRFrTYk9NsjbV35NI/DoCFQQ=; b=PUE+zgxxPPlefhYm0MXBBFvJCCZNR1zfr8D6WhcbuhTrlln0Jb+2vLof5RW/bKIhuf xidLq5jYZw8Hk2cI9nCGlodcw47FmLwSxYRi7arnBSzQqQDry3mbuwIjwiuFiXdDB5Fp e5rIiXv1EUOtZlxuTaiyWEMk3GGZLakB+qW9csO4yD0vWB9HWLtXJi3gf7xKuvjf3wMQ mcpnImkfiVYE8/Wx9IKKitUbogDcLQYjKmxhK0fhWQ5m6AeQmyNZS5Ja0KS3aQDRglnS 9DnDkl735OxwcxECqjVJhLEf8XX5wifCEN2cQ2Iej0BnorPjiEdMmruZJOIbCD8qxKoI Vh7A== X-Gm-Message-State: AOAM531YmmalYSy5/cbuN1crOFv6Rw+t7QccAGOO+paLRX4hROd5mSIx B4YKHtnN5cOuLoGe2FacP5Hv6W1jjT3ayx+GCcJZQg== X-Google-Smtp-Source: ABdhPJySLhbC5WzDuB67uzrOZ9QpcyPxMj+1yYafVt6ksGWZ/DZ7G9DR4tON+WdfbjWdcBceE+hKBMwT/h9K3GGxKII= X-Received: by 2002:a5d:59ab:: with SMTP id p11mr5870456wrr.340.1635892579719; Tue, 02 Nov 2021 15:36:19 -0700 (PDT) MIME-Version: 1.0 References: <20211029200319.23475-1-jim2101024@gmail.com> <20211029200319.23475-8-jim2101024@gmail.com> In-Reply-To: From: Jim Quinlan Date: Tue, 2 Nov 2021 18:36:08 -0400 Message-ID: Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators To: Rob Herring Cc: Jim Quinlan , linux-pci@vger.kernel.org, Nicolas Saenz Julienne , Mark Brown , bcm-kernel-feedback-list@broadcom.com, 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 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? > > 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()? The problem with this is that our host bridge needs to be the agent freeing the regulators. IIRC correctly, when the regulators were freed by the EP device -- or now the bridge in this case -- we got panics when doing unbinds. I will go back and get the details on this, but I'm wondering if our controller has arcane but necessary requirements outside of what a general mechanism could provide. Thanks, Jim > > 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 0537FC433F5 for ; Tue, 2 Nov 2021 22:37:40 +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 C0DE860240 for ; Tue, 2 Nov 2021 22:37:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C0DE860240 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=broadcom.com 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=rrjpTSlYdbzwswC9nhPnHkeFU8L92DYvKXO+K4M/5VQ=; b=XvefnUv6tU/adw 7Azm4X3u11k/RTPnVkG4EmyFML1ripICKVFOG+ldvE5+4ZbFiwd07rjqrCQ1XbdBkrvQZl0oy9PHd 9yzlSkFp9pv4NvdR3WImfHSS2rSs+/k9SItnw3pAZk9Nmu8k4jzCi4+Qi/QU2pO5cWcusQ1u2ozJI bR+XZWxnCe7Eu7+uRV5+QQeyBSS/lzqDNQVCw7rPeIwYdgZIb0Leb6OL5BjPQHELxxejjzJ5seYdX FPlTqIH0ErAwXVLVDxsIVjtDy5r2JXEscZiRsTJOw8fFAOTIE8yehSKUSBPej/P4Fk2cl6+Mg8wAW dvvS7KiUiLC32vsFF76g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mi2OJ-003Ae1-6Y; Tue, 02 Nov 2021 22:36:27 +0000 Received: from mail-wr1-x433.google.com ([2a00:1450:4864:20::433]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mi2OD-003Abj-PK for linux-arm-kernel@lists.infradead.org; Tue, 02 Nov 2021 22:36:23 +0000 Received: by mail-wr1-x433.google.com with SMTP id i5so761452wrb.2 for ; Tue, 02 Nov 2021 15:36:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mlauy3vNd9KbBeQ/P3IhRFrTYk9NsjbV35NI/DoCFQQ=; b=LhJzMSC03GD6kwFoHkcv0WlZV8/LiMo3e6aUlmAyFmOZ03I0K/GJ7F40Hc1b+LDbys tYwF6MfkMGi4kGmcYAQrVZ0wm6bqC4JEDNJsKiWBlgYmK/SLRCAp/vSUag2Lgc2mUTbK FwoLgRPulKd9iQCBHERoM+IfA8GraokFzdvOU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mlauy3vNd9KbBeQ/P3IhRFrTYk9NsjbV35NI/DoCFQQ=; b=IxrxBtZfcqzZYgWeIac37q5X2cwu/fkCpGcRggRs8AaHwaVTTPvpBv7ZeZlrTEYBcX u977ySv7FMV1cKNicOxrJSfxlulFt+JbCc4kdp7ZU1JmA6xNdipkDHqWNp/cdVyiT6Ti 2dc0Z5kUOGh5Ah+GhtoxJhaVhQz2BZ26maD7WrLqGUwlcJaqAybHeyEyUlreoIyI8UAk DQKeZK7TyUec3G7u4jNzyY/OVAZ+77QjHHPSnsBqQZ24oLa3G/UQfHTrvE82LjwyPeRS S5uFA0hiSZdQH/leCyuKhCIMu4i2J7+wvLG70VWwxeciWwSgmvqnXt1mt1RPMMFKHq/e 8VvA== X-Gm-Message-State: AOAM533CyJksNL5agVG/ZP5KwMU3ScUaaAJfRwxkk7pwz2xoAyAKcPrv 4XA2qKOr09aaoYoQzTXdayKEtbUnsx1DC9e58BjuwQ== X-Google-Smtp-Source: ABdhPJySLhbC5WzDuB67uzrOZ9QpcyPxMj+1yYafVt6ksGWZ/DZ7G9DR4tON+WdfbjWdcBceE+hKBMwT/h9K3GGxKII= X-Received: by 2002:a5d:59ab:: with SMTP id p11mr5870456wrr.340.1635892579719; Tue, 02 Nov 2021 15:36:19 -0700 (PDT) MIME-Version: 1.0 References: <20211029200319.23475-1-jim2101024@gmail.com> <20211029200319.23475-8-jim2101024@gmail.com> In-Reply-To: From: Jim Quinlan Date: Tue, 2 Nov 2021 18:36:08 -0400 Message-ID: Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators To: Rob Herring Cc: Jim Quinlan , linux-pci@vger.kernel.org, Nicolas Saenz Julienne , Mark Brown , bcm-kernel-feedback-list@broadcom.com, 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-20211102_153622_374824_A586797F X-CRM114-Status: GOOD ( 28.72 ) 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 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? > > 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()? The problem with this is that our host bridge needs to be the agent freeing the regulators. IIRC correctly, when the regulators were freed by the EP device -- or now the bridge in this case -- we got panics when doing unbinds. I will go back and get the details on this, but I'm wondering if our controller has arcane but necessary requirements outside of what a general mechanism could provide. Thanks, Jim > > Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel