[PATCH v2 0/2] PCI: brcmstb: Add Broadcom STB support

[PATCH v2 0/2] PCI: brcmstb: Add Broadcom STB support

[Jaedon Shin]

This series supports GPIO based regulator supplies for its power supplies.
and this has an improvement on devm_ APIs.

Changes v2:
- Remove incomplete 7445 based Broadcom STB SoC enable codes.
- Add devicetree description for supply-names.
- Use -EPROBE_DEFER if the GPIO system is not available.
- Remove unnecessary #ifdef CONFIG_REGULATOR
- Use devm_regulator_bulk_* instead of devm_regulator_*

Jaedon Shin (2):
  PCI: brcmstb: Add regulator support
  PCI: brcmstb: Drop clk_put when probe fails and remove

 .../bindings/pci/brcm,stb-pcie.yaml           |  3 ++
 drivers/pci/controller/pcie-brcmstb.c         | 37 ++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

-- 
2.21.0

[PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Jaedon Shin]

ARM-based Broadcom STB SoCs have GPIO-based voltage regulator for PCIe
turning off/on power supplies.

Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
---
 .../bindings/pci/brcm,stb-pcie.yaml           |  3 ++
 drivers/pci/controller/pcie-brcmstb.c         | 36 +++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 77d3e81a437b..efa5c885724b 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -56,6 +56,9 @@ properties:
     description: Indicates usage of spread-spectrum clocking.
     type: boolean
 
+  supply-names:
+    description: List of regulator supplies to use for PCIe
+
 required:
   - reg
   - dma-ranges
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d20aabc26273..8968ef7fa55d 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -23,6 +23,7 @@
 #include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -173,6 +174,8 @@ struct brcm_pcie {
 	int			gen;
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
+	struct regulator_bulk_data *vreg_bulk;
+	int			num_vregs;
 };
 
 /*
@@ -898,6 +901,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 {
 	brcm_msi_remove(pcie);
 	brcm_pcie_turn_off(pcie);
+	regulator_bulk_disable(pcie->num_vregs, pcie->vreg_bulk);
 	clk_disable_unprepare(pcie->clk);
 	clk_put(pcie->clk);
 }
@@ -920,6 +924,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	struct brcm_pcie *pcie;
 	struct pci_bus *child;
 	struct resource *res;
+	struct regulator_bulk_data *bulk;
+	int i;
 	int ret;
 
 	bridge = devm_pci_alloc_host_bridge(&pdev->dev, sizeof(*pcie));
@@ -955,6 +961,36 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = of_property_count_strings(np, "supply-names");
+	pcie->num_vregs = (ret < 0) ? 0 : ret;
+
+	if (pcie->num_vregs) {
+		bulk = devm_kcalloc(pcie->dev, pcie->num_vregs, sizeof(*bulk),
+				    GFP_KERNEL);
+		if (!bulk) {
+			clk_disable_unprepare(pcie->clk);
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < pcie->num_vregs; i++)
+			of_property_read_string_index(np, "supply-names", i,
+						      &bulk[i].supply);
+
+		ret = devm_regulator_bulk_get(pcie->dev, pcie->num_vregs, bulk);
+		if (ret < 0) {
+			clk_disable_unprepare(pcie->clk);
+			return ret;
+		}
+
+		pcie->vreg_bulk = bulk;
+	}
+
+	ret = regulator_bulk_enable(pcie->num_vregs, pcie->vreg_bulk);
+	if (ret) {
+		clk_disable_unprepare(pcie->clk);
+		return ret;
+	}
+
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;
-- 
2.21.0

[PATCH v2 2/2] PCI: brcmstb: Drop clk_put when probe fails and remove

[Jaedon Shin]

devm_clk_get* APIs are device managed and get freed automatically when
the device detaches. so there is no reason to explicitly call clk_put()
in probe or remove functions.

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host
controller driver")
Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/pci/controller/pcie-brcmstb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 8968ef7fa55d..cbb587eb8d39 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -903,7 +903,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_pcie_turn_off(pcie);
 	regulator_bulk_disable(pcie->num_vregs, pcie->vreg_bulk);
 	clk_disable_unprepare(pcie->clk);
-	clk_put(pcie->clk);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
-- 
2.21.0

Re: [PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

On Fri, Feb 21, 2020 at 12:36:39PM +0900, Jaedon Shin wrote:

> +  supply-names:
> +    description: List of regulator supplies to use for PCIe
> +

No, this isn't a good idea - the set of supplies the device has is
fixed when the silicon is produced, it's not something that needs to
vary per board.  This means that the binding should have a fixed list of
supplies, per SoC if that's needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Florian Fainelli]

On 2/21/2020 4:12 AM, Mark Brown wrote:
> On Fri, Feb 21, 2020 at 12:36:39PM +0900, Jaedon Shin wrote:
> 
>> +  supply-names:
>> +    description: List of regulator supplies to use for PCIe
>> +
> 
> No, this isn't a good idea - the set of supplies the device has is
> fixed when the silicon is produced, it's not something that needs to
> vary per board.  This means that the binding should have a fixed list of
> supplies, per SoC if that's needed.

These are not the supplies for the PCIe I/Os on the SoCs side, but the
supplies for the PCIe end-point connected to the SoCs. More on that below.

What I suspect is going on is that Jaedon wants to use the canonical
bootloader from Broadcom which provides the supplies in way that they
are properties of the PCIe RC node as opposed to properties of the PCIe
EP node. This has been done historically because there is a chicken and
egg problem and we did not know or could solve this at the time.

If you describe the regulators as properties of the PCIe EP nodes which
are child nodes of the PCIe RC node (as we should), you will not be able
to manage all of them within your pci_driver instance, because if there
is no power to the EP you just do not enumerate it, therefore no
pci_device is created.

It seems to me that what we are missing is some logic within the PCI
subsystem that is able to parse regulator/resets properties and ensure
that they are enabled in order for a PCI bus scan to succeed. Jaedon, do
you feel like taking on this effort?

From these existing bindings:

Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt

it seems to me that they are all describing the supplies from the SoC
side, but not sure, there may have been a precedent for a similar
simplification.
-- 
Florian

Re: [PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Fri, Feb 21, 2020 at 08:33:36AM -0800, Florian Fainelli wrote:
> On 2/21/2020 4:12 AM, Mark Brown wrote:

> > No, this isn't a good idea - the set of supplies the device has is
> > fixed when the silicon is produced, it's not something that needs to
> > vary per board.  This means that the binding should have a fixed list of
> > supplies, per SoC if that's needed.

> These are not the supplies for the PCIe I/Os on the SoCs side, but the
> supplies for the PCIe end-point connected to the SoCs. More on that below.

Then the "slots" (obviously at least some of these are soldered down
rather than on actual slots) should be represented in DT and the
controller or bus should know that it needs to iterate over them to
bring up the chips.  I would also expect that there are standard names
in the PCI specs for the standard supplies that go into devices as part
of the bus spec which would mean that there should still be no need to
encode names like this.

> If you describe the regulators as properties of the PCIe EP nodes which
> are child nodes of the PCIe RC node (as we should), you will not be able
> to manage all of them within your pci_driver instance, because if there
> is no power to the EP you just do not enumerate it, therefore no
> pci_device is created.

The framework and/or driver can enumerate firmware information without
actually powering up the devices of course.

I would not be surprised to learn that most systems just mark the device
supplies always on, it's not like the devices will be able to use them
normally anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Florian Fainelli]

On 2/21/20 9:11 AM, Mark Brown wrote:
> On Fri, Feb 21, 2020 at 08:33:36AM -0800, Florian Fainelli wrote:
>> On 2/21/2020 4:12 AM, Mark Brown wrote:
> 
>>> No, this isn't a good idea - the set of supplies the device has is
>>> fixed when the silicon is produced, it's not something that needs to
>>> vary per board.  This means that the binding should have a fixed list of
>>> supplies, per SoC if that's needed.
> 
>> These are not the supplies for the PCIe I/Os on the SoCs side, but the
>> supplies for the PCIe end-point connected to the SoCs. More on that below.
> 
> Then the "slots" (obviously at least some of these are soldered down
> rather than on actual slots) should be represented in DT and the
> controller or bus should know that it needs to iterate over them to
> bring up the chips.  I would also expect that there are standard names
> in the PCI specs for the standard supplies that go into devices as part
> of the bus spec which would mean that there should still be no need to
> encode names like this.

Agree.

> 
>> If you describe the regulators as properties of the PCIe EP nodes which
>> are child nodes of the PCIe RC node (as we should), you will not be able
>> to manage all of them within your pci_driver instance, because if there
>> is no power to the EP you just do not enumerate it, therefore no
>> pci_device is created.
> 
> The framework and/or driver can enumerate firmware information without
> actually powering up the devices of course.

The issue is not enumeration, it is ensuring that you will be able to
establish the PCIe link with the EP. If there is no pci_device created
because the bus scanning returned a link down, there is not much that
can be done. Also the question is whether this logic belongs in the PCI
bus layer or the driver.

> 
> I would not be surprised to learn that most systems just mark the device
> supplies always on, it's not like the devices will be able to use them
> normally anyway.

In the downstream PCIe driver which is this one is just a subset of
until we close the gap, we have some additional logical to determine
whether the EP device is wakeup enabled in order to leave its regulators
turned on during system sleep so as to permit Wake-on-WLAN for instance.
-- 
Florian

Re: [PATCH v2 1/2] PCI: brcmstb: Add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

On Fri, Feb 21, 2020 at 09:50:42AM -0800, Florian Fainelli wrote:
> On 2/21/20 9:11 AM, Mark Brown wrote:

> > The framework and/or driver can enumerate firmware information without
> > actually powering up the devices of course.

> The issue is not enumeration, it is ensuring that you will be able to
> establish the PCIe link with the EP. If there is no pci_device created
> because the bus scanning returned a link down, there is not much that
> can be done. Also the question is whether this logic belongs in the PCI
> bus layer or the driver.

Given that the interface with the devices is all standardized I'd have
expected it to be in the bus code as a first pass.

> > I would not be surprised to learn that most systems just mark the device
> > supplies always on, it's not like the devices will be able to use them
> > normally anyway.

> In the downstream PCIe driver which is this one is just a subset of
> until we close the gap, we have some additional logical to determine
> whether the EP device is wakeup enabled in order to leave its regulators
> turned on during system sleep so as to permit Wake-on-WLAN for instance.

Is that just using standard PCI stuff or is it custom for embedded
applications?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] PCI: brcmstb: Drop clk_put when probe fails and remove

[Rob Herring]

On Fri, 21 Feb 2020 12:36:40 +0900, Jaedon Shin wrote:
> devm_clk_get* APIs are device managed and get freed automatically when
> the device detaches. so there is no reason to explicitly call clk_put()
> in probe or remove functions.
> 
> Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host
> controller driver")
> Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>