[PATCH v2 0/6] brcmstb: add EP regulators and panic handler

[PATCH v2 0/6] brcmstb: add EP regulators and panic handler

[Jim Quinlan]

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

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 (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulator(s)
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml           |  12 +
 drivers/pci/controller/pcie-brcmstb.c         | 219 +++++++++++++++++-
 2 files changed, 228 insertions(+), 3 deletions(-)

-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

[PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

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

Quite 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.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 807694b4f41f..baacc3d7ec87 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -85,6 +85,18 @@ properties:
       minItems: 1
       maxItems: 3
 
+  vpcie12v-supply:
+    description: 12v regulator phandle for the endpoint device
+
+  vpcie3v3-supply:
+    description: 3.3v regulator phandle for the endpoint device
+
+  vpcie1v8-supply:
+    description: 1.8v regulator phandle for the endpoint device
+
+  vpcie0v9-supply:
+    description: 0.9v regulator phandle for the endpoint device
+
 required:
   - reg
   - ranges
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> Quite 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.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 807694b4f41f..baacc3d7ec87 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -85,6 +85,18 @@ properties:
>        minItems: 1
>        maxItems: 3
>  
> +  vpcie12v-supply:
> +    description: 12v regulator phandle for the endpoint device
> +
> +  vpcie3v3-supply:
> +    description: 3.3v regulator phandle for the endpoint device

12V and 3.3V are standard slot supplies, can you add them to 
pci-bus.yaml. Then some day maybe we can have common slot handling code.
 
With that, here you just need:

vpcie3v3-supply: true

> +
> +  vpcie1v8-supply:
> +    description: 1.8v regulator phandle for the endpoint device
> +
> +  vpcie0v9-supply:
> +    description: 0.9v regulator phandle for the endpoint device

These are not standard. They go to a soldered down device or 
non-standard connector? For the former, the device should really be 
described in DT and the supplies added there.

Mini PCIe connector also has 1.5V supply.

Rob

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Wed, Dec 9, 2020 at 10:07 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> > Quite 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.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 807694b4f41f..baacc3d7ec87 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -85,6 +85,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  vpcie12v-supply:
> > +    description: 12v regulator phandle for the endpoint device
> > +
> > +  vpcie3v3-supply:
> > +    description: 3.3v regulator phandle for the endpoint device
>
> 12V and 3.3V are standard slot supplies, can you add them to
> pci-bus.yaml. Then some day maybe we can have common slot handling code.
>
> With that, here you just need:
>
> vpcie3v3-supply: true

Hi Rob,

Sorry for the delay in responding -- I just came back from vacation.

The problem we have is that these regulators are not "slot" supplies
-- our HW does not support PCI slots, so if and when general slot
power-handling code came along it would probably screw us up.   If you
don't think there is a problem then I will submit the two supply-names
you OKed, even though they may not match the voltages we are using for
the EPs.

For us, the supplies are for the EP chip's power.  We have the PCIe
controller turning them "on" for power-on/resume and "off" for
power-off/suspend.  We need the "xxx-supply" property in the
controller's DT node because of the chicken-and-egg situation: if the
property was in the EP's DT node, the RC  will never discover the EP
to see that there is a regulator to turn on.   We would be happy with
a single supply name, something like "ep-power".  We would be ecstatic
to have two (ep0-power, ep1-power).

I'm not sure if you remember but FlorianF talked to you about this
situation and concluded that something like the above was the way to
go forward.  For the latest pullreq I  just copied Rockchip's bindings
since you reviewed their bindings commit but it looks like you've
changed your mind.   Given the constraints I have described, what is
the best path forward?

Thanks,
Jim Quinlan
Broadcom STB
>
> > +
> > +  vpcie1v8-supply:
> > +    description: 1.8v regulator phandle for the endpoint device
> > +
> > +  vpcie0v9-supply:
> > +    description: 0.9v regulator phandle for the endpoint device
>
> These are not standard. They go to a soldered down device or
> non-standard connector? For the former, the device should really be
> described in DT and the supplies added there.
>
> Mini PCIe connector also has 1.5V supply.
>
> Rob

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

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

On Mon, Jan 04, 2021 at 05:12:11PM -0500, Jim Quinlan wrote:

> For us, the supplies are for the EP chip's power.  We have the PCIe
> controller turning them "on" for power-on/resume and "off" for
> power-off/suspend.  We need the "xxx-supply" property in the
> controller's DT node because of the chicken-and-egg situation: if the
> property was in the EP's DT node, the RC  will never discover the EP
> to see that there is a regulator to turn on.   We would be happy with
> a single supply name, something like "ep-power".  We would be ecstatic
> to have two (ep0-power, ep1-power).

Why can't the controller look at the nodes describing devices for
standard properties?

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

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Tue, Jan 5, 2021 at 9:01 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 04, 2021 at 05:12:11PM -0500, Jim Quinlan wrote:
>
> > For us, the supplies are for the EP chip's power.  We have the PCIe
> > controller turning them "on" for power-on/resume and "off" for
> > power-off/suspend.  We need the "xxx-supply" property in the
> > controller's DT node because of the chicken-and-egg situation: if the
> > property was in the EP's DT node, the RC  will never discover the EP
> > to see that there is a regulator to turn on.   We would be happy with
> > a single supply name, something like "ep-power".  We would be ecstatic
> > to have two (ep0-power, ep1-power).
>
> Why can't the controller look at the nodes describing devices for
> standard properties?
Hi Mark,

It just feels wrong for the driver (RC) of one DT node to be acting on
a property of another driver's (EP) node, even though it is a subnode.
There is also the possibility of the EP driver acting upon the
property simultaneously; we don't really have control of what EP
device and drivers are paired with our SOCs.
In addition, this just pushes the binding name issue down a level --
what should these power supplies be called?  They are not slot power
supplies.  Can the  Broadcom STB PCIe RC driver's binding document
specify and define the properties of EP sub-nodes?

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

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

On Tue, Jan 05, 2021 at 10:09:21AM -0500, Jim Quinlan wrote:
> On Tue, Jan 5, 2021 at 9:01 AM Mark Brown <broonie@kernel.org> wrote:

> > > For us, the supplies are for the EP chip's power.  We have the PCIe
> > > controller turning them "on" for power-on/resume and "off" for
> > > power-off/suspend.  We need the "xxx-supply" property in the
> > > controller's DT node because of the chicken-and-egg situation: if the
> > > property was in the EP's DT node, the RC  will never discover the EP
> > > to see that there is a regulator to turn on.   We would be happy with

> > Why can't the controller look at the nodes describing devices for
> > standard properties?

> It just feels wrong for the driver (RC) of one DT node to be acting on
> a property of another driver's (EP) node, even though it is a subnode.

This is something we do for other buses, for example where there's
device specific tuning that is actually implemented in the controller
hardware.

> There is also the possibility of the EP driver acting upon the
> property simultaneously; we don't really have control of what EP
> device and drivers are paired with our SOCs.

If the device is trying to do something with a supply that's a standard
part of the bus outside of the bus it seems like that's going to lead to
problems no matter what, due to the discovery issues the device must be
coordinating with the bus somehow.

> In addition, this just pushes the binding name issue down a level --
> what should these power supplies be called?  They are not slot power
> supplies.  Can the  Broadcom STB PCIe RC driver's binding document
> specify and define the properties of EP sub-nodes?

I assume the supplies have some name in the PCI specs, whatever names
are used there would probably be appropriate.

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

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Mon, Jan 4, 2021 at 3:12 PM Jim Quinlan <jim2101024@gmail.com> wrote:
>
> On Wed, Dec 9, 2020 at 10:07 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> > > Quite 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.
> > >
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > index 807694b4f41f..baacc3d7ec87 100644
> > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > @@ -85,6 +85,18 @@ properties:
> > >        minItems: 1
> > >        maxItems: 3
> > >
> > > +  vpcie12v-supply:
> > > +    description: 12v regulator phandle for the endpoint device
> > > +
> > > +  vpcie3v3-supply:
> > > +    description: 3.3v regulator phandle for the endpoint device
> >
> > 12V and 3.3V are standard slot supplies, can you add them to
> > pci-bus.yaml. Then some day maybe we can have common slot handling code.
> >
> > With that, here you just need:
> >
> > vpcie3v3-supply: true
>
> Hi Rob,
>
> Sorry for the delay in responding -- I just came back from vacation.

NP, me too.

> The problem we have is that these regulators are not "slot" supplies
> -- our HW does not support PCI slots, so if and when general slot
> power-handling code came along it would probably screw us up.   If you
> don't think there is a problem then I will submit the two supply-names
> you OKed, even though they may not match the voltages we are using for
> the EPs.

Maybe no slots, but you defined the voltages here and they look like
standard voltages. Given this is at least the 2nd usage of these
properties, it seemed like they should be common. Slot or no physical
slot.

> For us, the supplies are for the EP chip's power.  We have the PCIe
> controller turning them "on" for power-on/resume and "off" for
> power-off/suspend.  We need the "xxx-supply" property in the
> controller's DT node because of the chicken-and-egg situation: if the
> property was in the EP's DT node, the RC  will never discover the EP
> to see that there is a regulator to turn on.   We would be happy with
> a single supply name, something like "ep-power".  We would be ecstatic
> to have two (ep0-power, ep1-power).

The chicken-and-egg problem is nothing new. The same thing has come up
for USB, MDIO, MMC/SD to name a few. If devices on a discoverable bus
are not discoverable, then they need to be described in DT. I've given
suggestions many times how to fix the kernel side.

As Mark said, there's no reason you can't look at other nodes for your
data. The data a driver needs isn't always nicely packaged up into a
single node. The DT structure should match the h/w. The EP is a
different device from the PCI host and its supplies belong in its
node.

Not that if we really wanted to have complete slot support, we'd
probably end up having slot nodes in DT. That's generally where we've
ended up at for other cases.

Now there's a second problem here. If this is not standard PCIe rails
which have a defined power sequencing, then you really need to
describe the EP device in DT. Otherwise, we don't know what the power
sequencing is. I will reject any properties such as delays which try
to poorly describe power sequencing in DT.

>
> I'm not sure if you remember but FlorianF talked to you about this
> situation and concluded that something like the above was the way to
> go forward.

Unless it was last week, assume I don't remember.

>  For the latest pullreq I  just copied Rockchip's bindings
> since you reviewed their bindings commit but it looks like you've
> changed your mind.

Well, no. First, it takes more than one to see a pattern. So yes, how
we describe something might evolve. Second, I didn't ask for anything
different from Rockchip here. Just move what Rockchip had to a common
location to reuse. But your reply has convinced me you need an EP
node.

>   Given the constraints I have described, what is
> the best path forward?
>
> Thanks,
> Jim Quinlan
> Broadcom STB
> >
> > > +
> > > +  vpcie1v8-supply:
> > > +    description: 1.8v regulator phandle for the endpoint device
> > > +
> > > +  vpcie0v9-supply:
> > > +    description: 0.9v regulator phandle for the endpoint device
> >
> > These are not standard. They go to a soldered down device or
> > non-standard connector? For the former, the device should really be
> > described in DT and the supplies added there.
> >
> > Mini PCIe connector also has 1.5V supply.
> >
> > Rob

[PATCH v2 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Jim Quinlan]

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

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 38 ++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..9d4ac42b3bee 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/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -210,6 +211,10 @@ enum pcie_type {
 	BCM2711,
 };
 
+static const char * const ep_regulator_names[] = {
+	"vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",
+};
+
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
@@ -287,8 +292,25 @@ 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(ep_regulator_names)];
 };
 
+static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	if (on)
+		ret = regulator_bulk_enable(ARRAY_SIZE(ep_regulator_names),
+					    pcie->supplies);
+	else
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep_regulator_names),
+					     pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to %s EP regulators\n",
+			on ? "enable" : "disable");
+}
+
 /*
  * 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
@@ -1139,6 +1161,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	brcm_pcie_turn_off(pcie);
 	ret = brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
 
 	return ret;
 }
@@ -1151,6 +1174,7 @@ static int brcm_pcie_resume(struct device *dev)
 	int ret;
 
 	base = pcie->base;
+	brcm_set_regulators(pcie, true);
 	clk_prepare_enable(pcie->clk);
 
 	ret = brcm_phy_start(pcie);
@@ -1189,6 +1213,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1218,7 +1243,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	struct pci_host_bridge *bridge;
 	const struct pcie_cfg_data *data;
 	struct brcm_pcie *pcie;
-	int ret;
+	int ret, i;
 
 	bridge = devm_pci_alloc_host_bridge(&pdev->dev, sizeof(*pcie));
 	if (!bridge)
@@ -1246,6 +1271,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->clk))
 		return PTR_ERR(pcie->clk);
 
+	for (i = 0; i < ARRAY_SIZE(ep_regulator_names); i++)
+		pcie->supplies[i].supply = ep_regulator_names[i];
+
+	ret = devm_regulator_bulk_get(pcie->dev, ARRAY_SIZE(ep_regulator_names),
+				      pcie->supplies);
+	if (ret) {
+		dev_err(pcie->dev, "failed to get regulators\n");
+		return ret;
+	}
+
 	ret = of_pci_get_max_link_speed(np);
 	pcie->gen = (ret < 0) ? 0 : ret;
 
@@ -1273,6 +1308,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	brcm_set_regulators(pcie, true);
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Florian Fainelli]

On 11/30/2020 1:11 PM, Jim Quinlan wrote:
> Control of EP regulators by the RC is needed because of the chicken-and-egg
> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 38 ++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index bea86899bd5d..9d4ac42b3bee 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/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -210,6 +211,10 @@ enum pcie_type {
>  	BCM2711,
>  };
>  
> +static const char * const ep_regulator_names[] = {
> +	"vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",

Only if you need to re-spin this patch series, I would be keen on
putting each string on its own line, that way when adding a subsequent
regulator name, it is just a matter of an one line d

> +};
> +
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
> @@ -287,8 +292,25 @@ 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(ep_regulator_names)];
>  };
>  
> +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	if (on)
> +		ret = regulator_bulk_enable(ARRAY_SIZE(ep_regulator_names),
> +					    pcie->supplies);
> +	else
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep_regulator_names),
> +					     pcie->supplies);
> +	if (ret)
> +		dev_err(dev, "failed to %s EP regulators\n",
> +			on ? "enable" : "disable");

Should not you propagate the return value to the caller here?
-- 
Florian

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

[Jim Quinlan]

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

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 <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9d4ac42b3bee..cbdb315d4b2f 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32
 static inline void brcm_pcie_bridge_sw_init_set_generic(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 bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -293,14 +294,57 @@ struct brcm_pcie {
 	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(ep_regulator_names)];
+	bool			ep_wakeup_capable;
 };
 
-static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+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;
+}
+
+enum {
+	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */
+	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
+	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */
+};
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	struct device *dev = pcie->dev;
 	int ret;
 
-	if (on)
+	if (how == TURN_ON) {
+		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;
+		}
+	} else if (how == TURN_OFF) {
+		/*
+		 * If at least one device on this bus is enabled as a
+		 * wake-up source, do not turn off regulators.
+		 */
+		pcie->ep_wakeup_capable = false;
+		if (bridge->bus && brcm_pcie_link_up(pcie)) {
+			pci_walk_bus(bridge->bus, pci_dev_may_wakeup, &pcie->ep_wakeup_capable);
+			if (pcie->ep_wakeup_capable)
+				return;
+		}
+	}
+
+	if (how == TURN_ON)
 		ret = regulator_bulk_enable(ARRAY_SIZE(ep_regulator_names),
 					    pcie->supplies);
 	else
@@ -308,7 +352,7 @@ static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
 					     pcie->supplies);
 	if (ret)
 		dev_err(dev, "failed to %s EP regulators\n",
-			on ? "enable" : "disable");
+			how == TURN_ON ? "enable" : "disable");
 }
 
 /*
@@ -1161,7 +1205,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	brcm_pcie_turn_off(pcie);
 	ret = brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
-	brcm_set_regulators(pcie, false);
+	brcm_set_regulators(pcie, TURN_OFF);
 
 	return ret;
 }
@@ -1174,7 +1218,7 @@ static int brcm_pcie_resume(struct device *dev)
 	int ret;
 
 	base = pcie->base;
-	brcm_set_regulators(pcie, true);
+	brcm_set_regulators(pcie, TURN_ON);
 	clk_prepare_enable(pcie->clk);
 
 	ret = brcm_phy_start(pcie);
@@ -1213,7 +1257,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
-	brcm_set_regulators(pcie, false);
+	brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1308,7 +1352,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	brcm_set_regulators(pcie, true);
+	brcm_set_regulators(pcie, TURN_ON);
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Florian Fainelli]

On 11/30/2020 1:11 PM, Jim Quinlan wrote:
> 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 <james.quinlan@broadcom.com>

Looks good to me, just one minor typo below:

> +enum {
> +	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */

s/egulators/regulators/

> +	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
> +	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */

-- 
Florian

[PATCH v2 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

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

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index cbdb315d4b2f..989e4231d136 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -256,6 +256,13 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+	.offsets	= pcie_offset_bcm7278,
+	.type		= BCM7278,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -1276,7 +1283,7 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
-	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
+	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
 	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
 	{},
 };
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Florian Fainelli]

On 11/30/2020 1:11 PM, Jim Quinlan wrote:
> This distinction is required for an imminent commit.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

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

Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
  brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 124 ++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 989e4231d136..3983d6c80769 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include <linux/ioport.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/kdebug.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/notifier.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
@@ -187,6 +189,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK		0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT		0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT				0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK		0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK			0x2
+#define PCIE_OUTB_ERR_VALID				0x6004
+#define PCIE_OUTB_ERR_CLEAR				0x6008
+#define PCIE_OUTB_ERR_ACC_INFO				0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK		0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK		0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK		0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK		0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK		0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR				0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK			0xff00000
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK			0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK		0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK			0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE				0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK	0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK	0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO			0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI			0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE				0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK		0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val);
@@ -221,6 +256,7 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -261,6 +297,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.type		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.has_err_report = true,
 };
 
 struct brcm_msi {
@@ -302,8 +339,89 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator_bulk_data supplies[ARRAY_SIZE(ep_regulator_names)];
 	bool			ep_wakeup_capable;
+	bool			has_err_report;
+	struct notifier_block	die_notifier;
 };
 
+/*
+ * Dump out pcie errors on die or panic.
+ */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void *p)
+{
+	const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, die_notifier);
+	void __iomem *base = pcie->base;
+	int i, is_cfg_err, is_mem_err, lanes;
+	char *width_str, *direction_str, lanes_str[9];
+	u32 info;
+
+	if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+		return NOTIFY_DONE;
+	info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+	is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+	is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+	width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : "32bit";
+	direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK) ? "Write" : "Read";
+	lanes = FIELD_GET(PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK, info);
+	for (i = 0, lanes_str[8] = 0; i < 8; i++)
+		lanes_str[i] = (lanes & (1 << i)) ? '1' : '0';
+
+	if (is_cfg_err) {
+		u32 cfg_addr = readl(base + PCIE_OUTB_ERR_ACC_ADDR);
+		u32 cause = readl(base + PCIE_OUTB_ERR_CFG_CAUSE);
+		int bus = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK, cfg_addr);
+		int dev = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK, cfg_addr);
+		int func = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK, cfg_addr);
+		int reg = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_REG_MASK, cfg_addr);
+
+		dev_err(pcie->dev, "Error: CFG Acc, %s, %s, Bus=%d, Dev=%d, Fun=%d, Reg=0x%x, lanes=%s\n",
+			width_str, direction_str, bus, dev, func, reg, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccTO=%d AccDsbld=%d Acc64bit=%d\n",
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK));
+	}
+
+	if (is_mem_err) {
+		u32 cause = readl(base + PCIE_OUTB_ERR_MEM_CAUSE);
+		u32 lo = readl(base + PCIE_OUTB_ERR_MEM_ADDR_LO);
+		u32 hi = readl(base + PCIE_OUTB_ERR_MEM_ADDR_HI);
+		u64 addr = ((u64)hi << 32) | (u64)lo;
+
+		dev_err(pcie->dev, "Error: Mem Acc, %s, %s, @0x%llx, lanes=%s\n",
+			width_str, direction_str, addr, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccDsble=%d BadAddr=%d\n",
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK));
+	}
+
+	/* Clear the error */
+	writel(1, base + PCIE_OUTB_ERR_CLEAR);
+
+	return NOTIFY_DONE;
+}
+
+static void brcm_register_die_notifiers(struct brcm_pcie *pcie)
+{
+	pcie->die_notifier.notifier_call = dump_pcie_error;
+	register_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_register(&panic_notifier_list, &pcie->die_notifier);
+}
+
+static void brcm_unregister_die_notifiers(struct brcm_pcie *pcie)
+{
+	unregister_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &pcie->die_notifier);
+	pcie->die_notifier.notifier_call = NULL;
+}
+
 static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
 	bool *ret = data;
@@ -1273,6 +1391,8 @@ static int brcm_pcie_remove(struct platform_device *pdev)
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 
 	pci_stop_root_bus(bridge->bus);
+	if (pcie->has_err_report)
+		brcm_unregister_die_notifiers(pcie);
 	pci_remove_root_bus(bridge->bus);
 	__brcm_pcie_remove(pcie);
 
@@ -1311,6 +1431,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->np = np;
 	pcie->reg_offsets = data->offsets;
 	pcie->type = data->type;
+	pcie->has_err_report = data->has_err_report;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
@@ -1380,6 +1501,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	if (pcie->has_err_report)
+		brcm_register_die_notifiers(pcie);
+
 	return pci_host_probe(bridge);
 fail:
 	__brcm_pcie_remove(pcie);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Florian Fainelli]

On 11/30/2020 1:11 PM, Jim Quinlan wrote:
> Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> by default Broadcom's STB PCIe controller effects an abort.  This simple
> handler determines if the PCIe controller was the cause of the abort and if
> so, prints out diagnostic info.
> 
> Example output:
>   brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
>   brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Bjorn Helgaas]

On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> by default Broadcom's STB PCIe controller effects an abort.  This simple
> handler determines if the PCIe controller was the cause of the abort and if
> so, prints out diagnostic info.

What happens during enumeration?  pci_bus_generic_read_dev_vendor_id()
assumes a read of Vendor ID returns 0xffffffff if the device doesn't
exist.

I assume this case doesn't cause the abort you're referring to here,
or nothing would work.  I think this enumeration case results in PCIe
Unsupported Request errors (PCIe r5.0, sec 2.3.2 implementation note).

Bjorn

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

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

On Tue, Dec 1, 2020 at 1:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > handler determines if the PCIe controller was the cause of the abort and if
> > so, prints out diagnostic info.
>
> What happens during enumeration?  pci_bus_generic_read_dev_vendor_id()
> assumes a read of Vendor ID returns 0xffffffff if the device doesn't
> exist.
>
> I assume this case doesn't cause the abort you're referring to here,
> or nothing would work.  I think this enumeration case results in PCIe
> Unsupported Request errors (PCIe r5.0, sec 2.3.2 implementation note).
Hi Bjorn,

Yes, our controller makes a special case  to allow for config-space
accesses to the dev_id and vendor_id registers. even if the device is
missing.  That being said, it will abort on any access if the link is
down.

However, the 7216-type SOCs bring PCIe error-reporting HW but also
have a mode where 0xffffffff  is returned on improper accesses, just
like many other controllers.  We are debating whether we should turn
this on by default.

Regards,
Jim Quinlan
Broadcom STB
>
> Bjorn

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Bjorn Helgaas]

On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> by default Broadcom's STB PCIe controller effects an abort.  This simple
> handler determines if the PCIe controller was the cause of the abort and if
> so, prints out diagnostic info.
> 
> Example output:
>   brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
>   brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

What does this mean for all the other PCI core code that expects
0xffffffff data returns?  Does it work?  Does it break differently on
STB than on other platforms?

> +/*
> + * Dump out pcie errors on die or panic.

s/pcie/PCIe/
This could be a single-line comment.

> + */

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

On Wed, Jan 6, 2021 at 2:42 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> ---------- Forwarded message ---------
> From: Bjorn Helgaas <helgaas@kernel.org>
> Date: Wed, Jan 6, 2021 at 2:19 PM
> Subject: Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver
> To: Jim Quinlan <james.quinlan@broadcom.com>
> Cc: <linux-pci@vger.kernel.org>, Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de>, <broonie@kernel.org>,
> <bcm-kernel-feedback-list@broadcom.com>, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, Bjorn
> Helgaas <bhelgaas@google.com>, Florian Fainelli
> <f.fainelli@gmail.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>
>
>
> On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > handler determines if the PCIe controller was the cause of the abort and if
> > so, prints out diagnostic info.
> >
> > Example output:
> >   brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
> >   brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0
>
> What does this mean for all the other PCI core code that expects
> 0xffffffff data returns?  Does it work?  Does it break differently on
> STB than on other platforms?
Hi Bjorn,

Our PCIe HW causes a CPU abort when this happens.  Occasionally a
customer will have a fault handler try to fix up the abort and
continue on, but we recommend solving the root problem.  This commit
just gives us a chance to glean info about the problem.  Our newer
SOCs have a mode that doesn't abort and instead returns 0xffffffff.

BTW, can you point me to example files where "PCI core code that
expects  0xffffffff data returns" [on bad accesses]?

Regards,
Jim Quinlan
Broadcom STB

>
> > +/*
> > + * Dump out pcie errors on die or panic.
>
> s/pcie/PCIe/
> This could be a single-line comment.
>
> > + */
>

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Bjorn Helgaas]

On Wed, Jan 06, 2021 at 02:57:19PM -0500, Jim Quinlan wrote:
> On Wed, Jan 6, 2021 at 2:42 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > ---------- Forwarded message ---------
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Date: Wed, Jan 6, 2021 at 2:19 PM
> > Subject: Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver
> > To: Jim Quinlan <james.quinlan@broadcom.com>
> > Cc: <linux-pci@vger.kernel.org>, Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de>, <broonie@kernel.org>,
> > <bcm-kernel-feedback-list@broadcom.com>, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, Bjorn
> > Helgaas <bhelgaas@google.com>, Florian Fainelli
> > <f.fainelli@gmail.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>
> >
> >
> > On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > > Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> > > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > > handler determines if the PCIe controller was the cause of the abort and if
> > > so, prints out diagnostic info.
> > >
> > > Example output:
> > >   brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
> > >   brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0
> >
> > What does this mean for all the other PCI core code that expects
> > 0xffffffff data returns?  Does it work?  Does it break differently on
> > STB than on other platforms?
> Hi Bjorn,
> 
> Our PCIe HW causes a CPU abort when this happens.  Occasionally a
> customer will have a fault handler try to fix up the abort and
> continue on, but we recommend solving the root problem.  This commit
> just gives us a chance to glean info about the problem.  Our newer
> SOCs have a mode that doesn't abort and instead returns 0xffffffff.
> 
> BTW, can you point me to example files where "PCI core code that
> expects  0xffffffff data returns" [on bad accesses]?

The most important case is during enumeration.  A config read to a
device that doesn't exist normally terminates as an Unsupported
Request, and pci_bus_generic_read_dev_vendor_id() depends on reading
0xffffffff in that case.  I assume this particular case does work that
way for brcm-pcie, because I assume enumeration does work.

pci_cfg_space_size_ext() is similar.  I assume this also works for
brcm-pcie for the same reason.

pci_raw_set_power_state() looks for ~0, which it may see if it does a
config read to a device in D3cold.  pci_dev_wait(), dpc_irq(),
pcie_pme_work_fn(), pcie_pme_irq() are all similar.

Yes, this is ugly and we should check for these more consistently.

The above are all for config reads.  The PCI core doesn't do MMIO
accesses except for a few cases like MSI-X.  But drivers do, and if
they check for PCIe errors on MMIO reads, they do it by looking for
0xffffffff, e.g., pci_mmio_enabled() (in hfi1),
qib_pci_mmio_enabled(), bnx2x_get_hwinfo(), etc.

Bjorn

[PATCH v2 6/6] PCI: brcmstb: check return value of clk_prepare_enable()

[Jim Quinlan]

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

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 3983d6c80769..64cf534e44d0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1344,7 +1344,9 @@ static int brcm_pcie_resume(struct device *dev)
 
 	base = pcie->base;
 	brcm_set_regulators(pcie, TURN_ON);
-	clk_prepare_enable(pcie->clk);
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret)
+		return ret;
 
 	ret = brcm_phy_start(pcie);
 	if (ret)
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 6/6] PCI: brcmstb: check return value of clk_prepare_enable()

[Florian Fainelli]

On 11/30/2020 1:11 PM, Jim Quinlan wrote:
> The check was missing on PCIe resume.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
-- 
Florian

Re: [PATCH v2 0/6] brcmstb: add EP regulators and panic handler

[Bjorn Helgaas]

On Mon, Nov 30, 2020 at 04:11:37PM -0500, Jim Quinlan wrote:
> 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 (6):
>   dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
>   PCI: brcmstb: Add control of EP voltage regulator(s)
>   PCI: brcmstb: Do not turn off regulators if EP can wake up
>   PCI: brcmstb: Give 7216 SOCs their own config type
>   PCI: brcmstb: Add panic/die handler to RC driver
>   PCI: brcmstb: check return value of clk_prepare_enable()

If/when you repost this, capitalize the subjects consistently.

>  .../bindings/pci/brcm,stb-pcie.yaml           |  12 +
>  drivers/pci/controller/pcie-brcmstb.c         | 219 +++++++++++++++++-
>  2 files changed, 228 insertions(+), 3 deletions(-)
> 
> -- 
> 2.17.1
>