From: Jim Quinlan <james.quinlan@broadcom.com> To: Rob Herring <robh@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, linux-pci@vger.kernel.org, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Mark Brown" <broonie@kernel.org>, bcm-kernel-feedback-list@broadcom.com, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Krzysztof Wilczyński" <kw@linux.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators Date: Tue, 2 Nov 2021 18:36:08 -0400 [thread overview] Message-ID: <CA+-6iNwdLt96U26eW-GDJFD3XB9unKX-ucF3gZ754ux78yMRCw@mail.gmail.com> (raw) In-Reply-To: <YYFgmxMCnKtTlaqL@robh.at.kernel.org> On Tue, Nov 2, 2021 at 12:00 PM Rob Herring <robh@kernel.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Jim Quinlan <james.quinlan@broadcom.com> To: Rob Herring <robh@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, linux-pci@vger.kernel.org, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Mark Brown" <broonie@kernel.org>, bcm-kernel-feedback-list@broadcom.com, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Krzysztof Wilczyński" <kw@linux.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators Date: Tue, 2 Nov 2021 18:36:08 -0400 [thread overview] Message-ID: <CA+-6iNwdLt96U26eW-GDJFD3XB9unKX-ucF3gZ754ux78yMRCw@mail.gmail.com> (raw) In-Reply-To: <YYFgmxMCnKtTlaqL@robh.at.kernel.org> On Tue, Nov 2, 2021 at 12:00 PM Rob Herring <robh@kernel.org> 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
next prev parent reply other threads:[~2021-11-02 22:36 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-29 20:03 [PATCH v6 0/9] PCI: brcmstb: have host-bridge turn on sub-device power Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-10-29 20:03 ` [PATCH v6 1/9] dt-bindings: PCI: correct brcmstb interrupts, interrupt-map Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-10-29 21:06 ` Florian Fainelli 2021-10-29 21:06 ` Florian Fainelli 2021-11-02 14:27 ` Rob Herring 2021-11-02 14:27 ` Rob Herring 2021-10-29 20:03 ` [PATCH v6 2/9] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-11-02 15:18 ` Rob Herring 2021-11-02 15:18 ` Rob Herring 2021-10-29 20:03 ` [PATCH v6 3/9] PCI: move pci_device_add() call Jim Quinlan 2021-10-29 20:03 ` [PATCH v6 4/9] PCI: separate device_initialize() from pci_device_add() Jim Quinlan 2021-10-29 20:03 ` [PATCH v6 5/9] PCI: allow for callback to prepare nascent subdev Jim Quinlan 2021-10-29 20:03 ` [PATCH v6 6/9] PCI: brcmstb: split brcm_pcie_setup() into two funcs Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-10-29 20:03 ` [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-10-31 19:49 ` kernel test robot 2021-10-31 19:49 ` kernel test robot 2021-11-01 15:24 ` Mark Brown 2021-11-01 15:24 ` Mark Brown 2021-11-02 16:00 ` Rob Herring 2021-11-02 16:00 ` Rob Herring 2021-11-02 22:36 ` Jim Quinlan [this message] 2021-11-02 22:36 ` Jim Quinlan 2021-11-04 14:42 ` Rob Herring 2021-11-04 14:42 ` Rob Herring 2021-10-29 20:03 ` [PATCH v6 8/9] PCI: brcmstb: Do not turn off regulators if EP can wake up Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan 2021-11-01 15:26 ` Mark Brown 2021-11-01 15:26 ` Mark Brown 2021-10-29 20:03 ` [PATCH v6 9/9] PCI: brcmstb: change brcm_phy_stop() to return void Jim Quinlan 2021-10-29 20:03 ` Jim Quinlan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CA+-6iNwdLt96U26eW-GDJFD3XB9unKX-ucF3gZ754ux78yMRCw@mail.gmail.com \ --to=james.quinlan@broadcom.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=bhelgaas@google.com \ --cc=broonie@kernel.org \ --cc=f.fainelli@gmail.com \ --cc=jim2101024@gmail.com \ --cc=kw@linux.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=nsaenz@kernel.org \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.