[PATCH v7 0/7] PCI: brcmstb: have host-bridge turn on sub-device power

[PATCH v7 0/7] PCI: brcmstb: have host-bridge turn on sub-device power

[Jim Quinlan]

v7  -- RobH suggested putting the "vpcixxx-supply" property under the
       bridge-node rather than the endpoint device node.  Also, he said to
       use the pci-ops add_bus/remove methods.  Doing so simplifies the
       code greatly and three commits were dropped.  Thanks!

    -- Rob also suggested (I think) having this patchset be a general
       feature which is activated by an OF property under the bridge node.
       I tried to do that but realized that our root complex driver
       controls the regulators with its dev_pm_ops and there is no way to
       transfer this control when using general mechanism.  Note that
       although the regulator core deals with suspend, our RC driver wants
       the right to sometimes to preclude this for WOL scenarios.

    -- One commit was added to change the response to the return value of
       the pci_ops add_bus() method.  Currently, an error causes a WARNING,
       a dev_err(...), and continues to return the child bus.  The
       modification was, for returning -ENOLINK only, to skip WARNING &
       dev_err() and return NULL.  This is necessary for our RC HW, as if
       the code continues on it will do a pci_read_config_dword() for the
       vendor/id, and our HW flags a CPU abort (instead of returning
       0xffffffff) when the is no pcie-link established.

       [NOTE: MarkB, I did not add one of your two "Reviewed-by"s
              because the commit had a decent amount of change.]

v6   -- Dropped the idea of a placeholder regulator
        property (brcm-ep-a-supply). (MarkB)
     -- device_initialize() now called once.  Two
        commits were added for this.  (GKH)
     -- In two cases, separated a single function 
        into two or more functions (MarkB)
     -- "(void)foo();" => "foo()".  Note that although
        foo() returns an int, in this instance it is being
	invoked within a function returning void, and foo()
	already executes a dev_err() on error. (MarkB)
     -- Added a commit to correct PCIe interrupts in YAML.
     -- Removed "device_type = "pci";" for the EP node
        in the YAML example.
     -- Updated the URL related to the voltage regulator
        names on GitHub.  Note that I added vpciev3v3aux.

v5 [NOTE: It has been a while since v4.  Sorry]
     -- See "PCI: allow for callback to prepare nascent subdev"
        commit message for the cornerstone of this patchset
        and the reasons behind it.  This is a new commit.
     -- The RC driver now looks into its DT children and
        turns on regulators for a sub-device, and this occurs
	prior to PCIe link as it must.
     -- Dropped commits not related to the focus of this patchset.

v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
          like to use this pullreq as a basis for future discussion.]
   [Commit: Add bindings for ...]
     -- Fix syntax error in YAML bindings example (RobH)
     -- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
        (I believe RobH said this was okay)
   [Commit: Add control of ..]
     -- Do not do global search for regulator; now we look specifically
        for the property {vpcie12v,vpcie3v3}-supply in the root complex
	DT node and then call devm_regulator_bulk_get() (MarkB)
     -- Use devm_regulator_bulk_get() (Bjorn)
     -- s/EP/slot0 device/ (Bjorn)
     -- Spelling, capitalization (Bjorn)
     -- Have brcm_phy_stop() return a void (Bjorn)
   [Commit: Do not turn off ...]
     -- Capitalization (Bjorn)
   [Commit: Check return value ...]
     -- Commit message content (Bjorn)
     -- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
     -- Move the rest of 6/6 before all other commits (Bjorn)

v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
      If present, these regulators have the property names
      "vpcie12v-supply" and "vpcie3v3-supply".  The existence of these
      regulators in the EP subnode are currently pending as a pullreq
      in pci-bus.yaml at
      https://github.com/devicetree-org/dt-schema/pull/54
      (MarkB, RobH).
   -- Check return of brcm_set_regulators() (Florian)
   -- Specify one regulator string per line for easier update (Florian)
   -- Author/Committer/Signoff email changed from that of V2 from
      'james.quinlan@broadcom.com' to 'jim2101024@gmail.com'.

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
      during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
      panic handler is added to dump its info.
   -- A missing return value check is added.

Jim Quinlan (7):
  dt-bindings: PCI: Correct brcmstb interrupts, interrupt-map.
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Split brcm_pcie_setup() into two funcs
  PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK
  PCI: brcmstb: Add control of subdevice voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Change brcm_phy_stop() to return void

 .../bindings/pci/brcm,stb-pcie.yaml           |  31 ++-
 drivers/pci/controller/pcie-brcmstb.c         | 233 ++++++++++++++++--
 drivers/pci/probe.c                           |   3 +
 3 files changed, 250 insertions(+), 17 deletions(-)


base-commit: 8e37395c3a5dceff62a5010ebbbc107f4145935c
-- 
2.17.1

[PATCH v7 1/7] dt-bindings: PCI: Correct brcmstb interrupts, interrupt-map.

[Jim Quinlan]

The "pcie" and "msi" interrupts were given the same interrupt when they are
actually different.  Interrupt-map only had the INTA entry; the INTB, INTC,
and INTD entries are added.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index b9589a0daa5c..508e5dce1282 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -142,11 +142,15 @@ examples:
                     #address-cells = <3>;
                     #size-cells = <2>;
                     #interrupt-cells = <1>;
-                    interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
+                    interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>,
                                  <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
                     interrupt-names = "pcie", "msi";
                     interrupt-map-mask = <0x0 0x0 0x0 0x7>;
-                    interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
+                    interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH
+                                     0 0 0 2 &gicv2 GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH
+                                     0 0 0 3 &gicv2 GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH
+                                     0 0 0 4 &gicv2 GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
+
                     msi-parent = <&pcie0>;
                     msi-controller;
                     ranges = <0x02000000 0x0 0xf8000000 0x6 0x00000000 0x0 0x04000000>;
-- 
2.17.1

[PATCH v7 2/7] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

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.  That being said, this driver searches in the DT subnode (the EP
node, eg pci-ep@0,0) for the regulator property.

The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at

https://github.com/devicetree-org/dt-schema/pull/63

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 508e5dce1282..ef2427320b7d 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -158,5 +158,28 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+
+                    /* PCIe bridge */
+                    pci@0,0 {
+                            #address-cells = <3>;
+                            #size-cells = <2>;
+                            reg = <0x0 0x0 0x0 0x0 0x0>;
+                            compatible = "pciclass,0604";
+                            device_type = "pci";
+                            vpcie3v3-supply = <&vreg7>;
+                            ranges;
+
+                            /* PCIe endpoint */
+                            pci-ep@0,0 {
+                                    assigned-addresses =
+                                        <0x82010000 0x0 0xf8000000 0x6 0x00000000 0x0 0x2000>;
+                                    reg = <0x0 0x0 0x0 0x0 0x0>;
+                                    compatible = "pci14e4,1688";
+                                    #address-cells = <3>;
+                                    #size-cells = <2>;
+
+                                    ranges;
+                            };
+                    };
             };
     };
-- 
2.17.1

[PATCH v7 3/7] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

We need to split a function in two so that the driver can take advantage of
the recently added function pci_subdev_prepare() which is a member
of struct pci_host_bridge.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 30 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index cc30215f5a43..ba4d6daf312c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -863,17 +863,10 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	struct device *dev = pcie->dev;
-	struct resource_entry *entry;
-	bool ssc_good = false;
-	struct resource *res;
-	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	int i, ret, memc;
-	u32 tmp, burst, aspm_support;
+	int ret, memc;
+	u32 tmp, burst;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -956,6 +949,21 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
+	return 0;
+}
+
+static int brcm_pcie_linkup(struct brcm_pcie *pcie)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	struct device *dev = pcie->dev;
+	void __iomem *base = pcie->base;
+	struct resource_entry *entry;
+	struct resource *res;
+	int num_out_wins = 0;
+	u16 nlw, cls, lnksta;
+	bool ssc_good = false;
+	u32 aspm_support, tmp;
+	int ret, i;
 
 	/* Unassert the fundamental reset */
 	pcie->perst_set(pcie, 0);
@@ -1186,6 +1194,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto err_reset;
+
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
-- 
2.17.1

[PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK

[Jim Quinlan]

Currently, if the call to the pci_ops add_bus() method returns an error, a
WARNING and dev_err() occurs.  We keep this behavior for all errors except
-ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
NULL.  The argument for this case is that one does not want to continue
enumerating if pcie-link has not been established.  The real reason is that
without doing this the pcie-brcmstb.c driver panics when the dev/id is
read, as this controller panics on such accesses rather than returning
0xffffffff.

It appears that there are only a few uses of the pci_ops add_bus() method
in the kernel and none of them currently return -ENOLINK so it should be
safe to do this.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/probe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..fdc3f42634b7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 
 	if (child->ops->add_bus) {
 		ret = child->ops->add_bus(child);
+		/* Don't return the child if w/o pcie link-up */
+		if (ret == -ENOLINK)
+			return NULL;
 		if (WARN_ON(ret < 0))
 			dev_err(&child->dev, "failed to add bus: %d\n", ret);
 	}
-- 
2.17.1

[PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

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.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 156 +++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index ba4d6daf312c..aaf6a4cbeb78 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -191,6 +192,15 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static int brcm_pcie_add_bus(struct pci_bus *bus);
+static void brcm_pcie_remove_bus(struct pci_bus *bus);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
+
+static const char * const supplies[] = {
+	"vpcie3v3",
+	"vpcie3v3aux",
+	"vpcie12v",
+};
 
 enum {
 	RGR1_SW_INIT_1,
@@ -295,8 +305,38 @@ struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
+	unsigned int		num_supplies;
 };
 
+static int brcm_regulators_on(struct brcm_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	if (!pcie->num_supplies)
+		return 0;
+	ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to enable EP regulators\n");
+
+	return ret;
+}
+
+static int brcm_regulators_off(struct brcm_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	if (!pcie->num_supplies)
+		return 0;
+	ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to disable EP regulators\n");
+
+	return ret;
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -722,6 +762,8 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
+	.add_bus = brcm_pcie_add_bus,
+	.remove_bus = brcm_pcie_remove_bus,
 };
 
 static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
@@ -1148,6 +1190,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct device *dev)
+{
+	const unsigned int ns = ARRAY_SIZE(supplies);
+	struct device_node *dn;
+	struct property *pp;
+	unsigned int i;
+	int ret;
+
+	/* This is for Broadcom STB/CM chips only */
+	if (pcie->type == BCM2711)
+		return 0;
+
+	dn = dev->of_node;
+	if (!dn)
+		return 0;
+
+	for_each_property_of_node(dn, pp) {
+		for (i = 0; i < ns; i++) {
+			char prop_name[64]; /* 64 is max size of property name */
+
+			snprintf(prop_name, 64, "%s-supply", supplies[i]);
+			if (strcmp(prop_name, pp->name) == 0)
+				break;
+		}
+		if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
+			continue;
+
+		pcie->supplies[pcie->num_supplies++].supply = supplies[i];
+	}
+
+	if (pcie->num_supplies == 0)
+		return 0;
+
+	ret = devm_regulator_bulk_get(dev, pcie->num_supplies,
+				      pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to get EP regulators\n");
+
+	return ret;
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
@@ -1158,7 +1241,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 
-	return ret;
+	return brcm_regulators_off(pcie);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1174,6 +1257,9 @@ static int brcm_pcie_resume(struct device *dev)
 	ret = reset_control_reset(pcie->rescal);
 	if (ret)
 		goto err_disable_clk;
+	ret = brcm_regulators_on(pcie);
+	if (ret)
+		goto err_reset;
 
 	ret = brcm_phy_start(pcie);
 	if (ret)
@@ -1241,6 +1327,62 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{},
 };
 
+static int brcm_pcie_add_bus(struct pci_bus *bus)
+{
+	struct pci_host_bridge *hb = pci_find_host_bridge(bus);
+	struct brcm_pcie *pcie = (struct brcm_pcie *) hb->sysdata;
+	struct device *dev = &bus->dev;
+	int ret;
+
+	/*
+	 * We only care about a device that is directly connected
+	 * to the root complex, ie bus == 1 and slot == 0.
+	 */
+	if (bus->number != 1)
+		return 0;
+
+	ret = brcm_pcie_get_regulators(pcie, dev);
+	if (ret)
+		goto err_out;
+
+	ret = brcm_regulators_on(pcie);
+	if (ret)
+		goto err_out;
+
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	/*
+	 * If we have failed there is no point to return the exact reason,
+	 * as currently it will cause a WARNING() from
+	 * pci_alloc_child_bus().  We return -ENOLINK to the caller, which
+	 * unlike any other errors, will stop the PCIe tree exploration.
+	 */
+	return -ENOLINK;
+}
+
+static void brcm_pcie_remove_bus(struct pci_bus *bus)
+{
+	struct pci_host_bridge *hb = pci_find_host_bridge(bus);
+	struct brcm_pcie *pcie = (struct brcm_pcie *) hb->sysdata;
+	struct device *dev = &bus->dev;
+	int ret;
+
+	if (bus->number != 1)
+		return;
+
+	if (pcie->num_supplies > 0) {
+		ret = brcm_regulators_off(pcie, true);
+		if (ret)
+			dev_err(dev, "Could not turn of regulators\n");
+		pcie->num_supplies = 0;
+	}
+}
+
 static int brcm_pcie_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node, *msi_np;
@@ -1332,7 +1474,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
-	return pci_host_probe(bridge);
+	ret = pci_host_probe(bridge);
+	if (!ret && !brcm_pcie_link_up(pcie))
+		ret = -ENODEV;
+
+	if (ret) {
+		brcm_pcie_remove(pdev);
+		return ret;
+	}
+
+	return 0;
+
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
-- 
2.17.1

[PATCH v7 6/7] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/pci/controller/pcie-brcmstb.c | 43 +++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index aaf6a4cbeb78..9aadb0eccfff 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -307,8 +307,20 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
 	unsigned int		num_supplies;
+	bool			ep_wakeup_capable;
 };
 
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
+{
+	bool *ret = data;
+
+	if (device_may_wakeup(&dev->dev)) {
+		*ret = true;
+		dev_dbg(&dev->dev, "disable cancelled for wake-up device\n");
+	}
+	return (int) *ret;
+}
+
 static int brcm_regulators_on(struct brcm_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -316,6 +328,18 @@ static int brcm_regulators_on(struct brcm_pcie *pcie)
 
 	if (!pcie->num_supplies)
 		return 0;
+
+	if (pcie->ep_wakeup_capable) {
+		/*
+		 * We are resuming from a suspend.  In the previous suspend
+		 * we did not disable the power supplies, so there is no
+		 * need to enable them (and falsely increase their usage
+		 * count).
+		 */
+		pcie->ep_wakeup_capable = false;
+		return 0;
+	}
+
 	ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	if (ret)
 		dev_err(dev, "failed to enable EP regulators\n");
@@ -323,13 +347,28 @@ static int brcm_regulators_on(struct brcm_pcie *pcie)
 	return ret;
 }
 
-static int brcm_regulators_off(struct brcm_pcie *pcie)
+static int brcm_regulators_off(struct brcm_pcie *pcie, bool force)
 {
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	struct device *dev = pcie->dev;
 	int ret;
 
 	if (!pcie->num_supplies)
 		return 0;
+
+	pcie->ep_wakeup_capable = false;
+
+	if (!force && bridge->bus && brcm_pcie_link_up(pcie)) {
+		/*
+		 * If at least one device on this bus is enabled as a
+		 * wake-up source, do not turn off regulators.
+		 */
+		pci_walk_bus(bridge->bus, pci_dev_may_wakeup,
+			     &pcie->ep_wakeup_capable);
+		if (pcie->ep_wakeup_capable)
+			return 0;
+	}
+
 	ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (ret)
 		dev_err(dev, "failed to disable EP regulators\n");
@@ -1241,7 +1280,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 
-	return brcm_regulators_off(pcie);
+	return brcm_regulators_off(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
-- 
2.17.1

[PATCH v7 7/7] PCI: brcmstb: Change brcm_phy_stop() to return void

[Jim Quinlan]

We do not use the result of this function so make it void.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9aadb0eccfff..5eabbd72e5b8 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1200,9 +1200,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
 	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+	if (pcie->rescal)
+		brcm_phy_cntl(pcie, 0);
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1273,10 +1274,9 @@ static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct device *dev)
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
-	int ret;
 
 	brcm_pcie_turn_off(pcie);
-	ret = brcm_phy_stop(pcie);
+	brcm_phy_stop(pcie);
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 
-- 
2.17.1

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:

> +	for_each_property_of_node(dn, pp) {
> +		for (i = 0; i < ns; i++) {
> +			char prop_name[64]; /* 64 is max size of property name */
> +
> +			snprintf(prop_name, 64, "%s-supply", supplies[i]);
> +			if (strcmp(prop_name, pp->name) == 0)
> +				break;
> +		}
> +		if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> +			continue;
> +
> +		pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> +	}

Why are we doing this?  If the DT omits the supplies the framework will
provide dummy supplies so there is no need to open code handling for
supplies not being present at all in client drivers.  Just
unconditionally ask for all the supplies.

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

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:
>
> > +     for_each_property_of_node(dn, pp) {
> > +             for (i = 0; i < ns; i++) {
> > +                     char prop_name[64]; /* 64 is max size of property name */
> > +
> > +                     snprintf(prop_name, 64, "%s-supply", supplies[i]);
> > +                     if (strcmp(prop_name, pp->name) == 0)
> > +                             break;
> > +             }
> > +             if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> > +                     continue;
> > +
> > +             pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> > +     }
>
> Why are we doing this?  If the DT omits the supplies the framework will
> provide dummy supplies so there is no need to open code handling for
> supplies not being present at all in client drivers.  Just
> unconditionally ask for all the supplies.

I did it to squelch the "supply xxxxx not found, using dummy
regulator" output.  I'll change it.
Regards, Jim

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:
> >
> > > +     for_each_property_of_node(dn, pp) {
> > > +             for (i = 0; i < ns; i++) {
> > > +                     char prop_name[64]; /* 64 is max size of property name */
> > > +
> > > +                     snprintf(prop_name, 64, "%s-supply", supplies[i]);
> > > +                     if (strcmp(prop_name, pp->name) == 0)
> > > +                             break;
> > > +             }
> > > +             if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> > > +                     continue;
> > > +
> > > +             pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> > > +     }
> >
> > Why are we doing this?  If the DT omits the supplies the framework will
> > provide dummy supplies so there is no need to open code handling for
> > supplies not being present at all in client drivers.  Just
> > unconditionally ask for all the supplies.
>
> I did it to squelch the "supply xxxxx not found, using dummy
> regulator" output.  I'll change it.
Now I remember: if I know there are no vpciexxx-supplly props in the
DT, I can skip executing all of the buik regulator calls entirely, as
well as walking the PCI bus as in brcm_regulators_off().

Do you consider this a valid reason?

Jim

Jim

Jim

> Regards, Jim

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Wed, Nov 03, 2021 at 04:34:34PM -0400, Jim Quinlan wrote:
> On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:

> > I did it to squelch the "supply xxxxx not found, using dummy
> > regulator" output.  I'll change it.

> Now I remember: if I know there are no vpciexxx-supplly props in the
> DT, I can skip executing all of the buik regulator calls entirely, as
> well as walking the PCI bus as in brcm_regulators_off().

> Do you consider this a valid reason?

No, the whole point in the core code providing dummy supplies is that it
removes the complexity introduced by client drivers trying to guess if
there's supplies available or not.  If they do that then we end up with
a bunch of code duplication and issues if there's any changes or
extensions to the generic bindings.

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

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Wed, Nov 3, 2021 at 5:52 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 03, 2021 at 04:34:34PM -0400, Jim Quinlan wrote:
> > On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> > > I did it to squelch the "supply xxxxx not found, using dummy
> > > regulator" output.  I'll change it.
>
> > Now I remember: if I know there are no vpciexxx-supplly props in the
> > DT, I can skip executing all of the buik regulator calls entirely, as
> > well as walking the PCI bus as in brcm_regulators_off().
>
> > Do you consider this a valid reason?
>
> No, the whole point in the core code providing dummy supplies is that it
> removes the complexity introduced by client drivers trying to guess if
> there's supplies available or not.  If they do that then we end up with
> a bunch of code duplication and issues if there's any changes or
> extensions to the generic bindings.
Ok, will change it.
Thanks, Jim

Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK

[Pali Rohár]

On Wednesday 03 November 2021 14:49:34 Jim Quinlan wrote:
> Currently, if the call to the pci_ops add_bus() method returns an error, a
> WARNING and dev_err() occurs.  We keep this behavior for all errors except
> -ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
> NULL.  The argument for this case is that one does not want to continue
> enumerating if pcie-link has not been established.  The real reason is that
> without doing this the pcie-brcmstb.c driver panics when the dev/id is
> read, as this controller panics on such accesses rather than returning
> 0xffffffff.

I think that this is something which should be fixed in the driver, not
in the pci core code. Check in driver code that you can touch HW and if
not return fabricated value 0xffffffff.

> It appears that there are only a few uses of the pci_ops add_bus() method
> in the kernel and none of them currently return -ENOLINK so it should be
> safe to do this.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/probe.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index d9fc02a71baa..fdc3f42634b7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  
>  	if (child->ops->add_bus) {
>  		ret = child->ops->add_bus(child);
> +		/* Don't return the child if w/o pcie link-up */
> +		if (ret == -ENOLINK)

In my opinion ENOLINK is not the correct errno code for signaling
"no link-up" error. IIRC ENOLINK was defined for file/inode links. For
network connections there is ENETDOWN errno code which is more similar
to "no link-up" than inode link.

Anyway, I still do not think if it is a good idea to have this check in
core pci code.

(This is just my opinion... wait for Bjorn with maintainer's hat what
will say that is the best way to handle above issue)

> +			return NULL;
>  		if (WARN_ON(ret < 0))
>  			dev_err(&child->dev, "failed to add bus: %d\n", ret);
>  	}
> -- 
> 2.17.1
> 

Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK

[Jim Quinlan]

On Wed, Nov 3, 2021 at 7:30 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 03 November 2021 14:49:34 Jim Quinlan wrote:
> > Currently, if the call to the pci_ops add_bus() method returns an error, a
> > WARNING and dev_err() occurs.  We keep this behavior for all errors except
> > -ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
> > NULL.  The argument for this case is that one does not want to continue
> > enumerating if pcie-link has not been established.  The real reason is that
> > without doing this the pcie-brcmstb.c driver panics when the dev/id is
> > read, as this controller panics on such accesses rather than returning
> > 0xffffffff.
>
> I think that this is something which should be fixed in the driver, not
> in the pci core code. Check in driver code that you can touch HW and if
> not return fabricated value 0xffffffff.
Yes --  I don't have control of the config-space data but I do have control
of the address, and I can hijack the address so that it points to an accessible
register that holds 0xffffffff.

>
> > It appears that there are only a few uses of the pci_ops add_bus() method
> > in the kernel and none of them currently return -ENOLINK so it should be
> > safe to do this.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/probe.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index d9fc02a71baa..fdc3f42634b7 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >
> >       if (child->ops->add_bus) {
> >               ret = child->ops->add_bus(child);
> > +             /* Don't return the child if w/o pcie link-up */
> > +             if (ret == -ENOLINK)
>
> In my opinion ENOLINK is not the correct errno code for signaling
> "no link-up" error. IIRC ENOLINK was defined for file/inode links. For
> network connections there is ENETDOWN errno code which is more similar
> to "no link-up" than inode link.
>
> Anyway, I still do not think if it is a good idea to have this check in
> core pci code.
This commit is no longer needed per your suggestion of having the host-bridge
driver force a return of 0xffffffff.

Thanks,
Jim

>
> (This is just my opinion... wait for Bjorn with maintainer's hat what
> will say that is the best way to handle above issue)
>
> > +                     return NULL;
> >               if (WARN_ON(ret < 0))
> >                       dev_err(&child->dev, "failed to add bus: %d\n", ret);
> >       }
> > --
> > 2.17.1
> >

Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Rob Herring]

On Wed, Nov 3, 2021 at 1:49 PM Jim Quinlan <jim2101024@gmail.com> 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.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 156 +++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index ba4d6daf312c..aaf6a4cbeb78 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -191,6 +192,15 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
>  static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
> +static int brcm_pcie_add_bus(struct pci_bus *bus);
> +static void brcm_pcie_remove_bus(struct pci_bus *bus);
> +static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
> +
> +static const char * const supplies[] = {
> +       "vpcie3v3",
> +       "vpcie3v3aux",
> +       "vpcie12v",

Common DT properties, so they should be in common DT code.

> +};
>
>  enum {
>         RGR1_SW_INIT_1,
> @@ -295,8 +305,38 @@ struct brcm_pcie {
>         u32                     hw_rev;
>         void                    (*perst_set)(struct brcm_pcie *pcie, u32 val);
>         void                    (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> +       struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
> +       unsigned int            num_supplies;

Humm, this will need to be stored somewhere, but the host bridge is
not the right place. That doesn't scale to more than 1 bridge/bus. I'm
not exactly sure where though. pci_bus.self->sys_data,
pci_bus.self->dev.driver_data, pci_bus->sysdata,
pci_bus->dev.driver_data are possible options. Bjorn?

However, given suspend/resume hooks are also needed, maybe
portdrv_pci.c driver is the better spot for all this? The host bridge
wouldn't be in control of opting in, but presence of a DT node ptr for
the port device may be sufficient.

Rob