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

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

[Jim Quinlan]

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

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         | 247 +++++++++++++++++-
 2 files changed, 257 insertions(+), 2 deletions(-)

-- 
2.17.1


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

[PATCH v1 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 --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3959 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 | 66 +++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..34d6bad07b66 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,18 @@ enum pcie_type {
 	BCM2711,
 };
 
+enum pcie_regulators {
+	VPCIE12V,
+	VPCIE3V3,
+	VPCIE1V8,
+	VPCIE0V9,
+	PCIE_REGULATORS_MAX,
+};
+
+static const char *ep_regulator_names[PCIE_REGULATORS_MAX] = {
+	"vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",
+};
+
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
@@ -287,8 +300,53 @@ 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	*regulators[PCIE_REGULATORS_MAX];
+	int			num_regulators;
 };
 
+static int brcm_parse_regulators(struct brcm_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct regulator *ep_reg;
+	int i;
+
+	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+		ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
+		if (IS_ERR(ep_reg)) {
+			if (PTR_ERR(ep_reg) == -ENODEV)
+				continue;
+			dev_err(dev, "failed to get regulator %s\n", ep_regulator_names[i]);
+			return PTR_ERR(ep_reg);
+		}
+		pcie->regulators[i] = ep_reg;
+		pcie->num_regulators++;
+	}
+	return 0;
+}
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+	struct device *dev = pcie->dev;
+	int ret, i;
+
+	if (pcie->num_regulators == 0)
+		return;
+
+	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+		if (!pcie->regulators[i])
+			continue;
+		if (on) {
+			ret = regulator_enable(pcie->regulators[i]);
+			dev_dbg(dev, "enable regulator %s (%s)\n",
+				ep_regulator_names[i], ret ? "fail" : "pass");
+		} else {
+			ret = regulator_disable(pcie->regulators[i]);
+			dev_dbg(dev, "disable regulator %s (%s)\n",
+				ep_regulator_names[i], ret ? "fail" : "pass");
+		}
+	}
+}
+
 /*
  * 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 +1197,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 +1210,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 +1249,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)
@@ -1238,6 +1299,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
+	ret = brcm_parse_regulators(pcie);
+	if (ret)
+		return ret;
+
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
@@ -1273,6 +1338,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 --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 4107 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 | 56 ++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 34d6bad07b66..9b888846f0bc 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,
@@ -302,6 +303,7 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator	*regulators[PCIE_REGULATORS_MAX];
 	int			num_regulators;
+	bool			ep_wakeup_capable;
 };
 
 static int brcm_parse_regulators(struct brcm_pcie *pcie)
@@ -324,18 +326,60 @@ static int brcm_parse_regulators(struct brcm_pcie *pcie)
 	return 0;
 }
 
-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, i;
 
 	if (pcie->num_regulators == 0)
 		return;
 
+	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;
+		}
+	}
+
 	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
 		if (!pcie->regulators[i])
 			continue;
-		if (on) {
+		if (how == TURN_ON) {
 			ret = regulator_enable(pcie->regulators[i]);
 			dev_dbg(dev, "enable regulator %s (%s)\n",
 				ep_regulator_names[i], ret ? "fail" : "pass");
@@ -1197,7 +1241,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;
 }
@@ -1210,7 +1254,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);
@@ -1249,7 +1293,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)
@@ -1338,7 +1382,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 --]

[PATCH v1 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 9b888846f0bc..e39bd93790d0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -264,6 +264,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;
@@ -1312,7 +1319,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 --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 7946 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 e39bd93790d0..469bbb0ebdd9 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);
@@ -229,6 +264,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[] = {
@@ -269,6 +305,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 {
@@ -311,8 +348,89 @@ struct brcm_pcie {
 	struct regulator	*regulators[PCIE_REGULATORS_MAX];
 	int			num_regulators;
 	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 brcm_parse_regulators(struct brcm_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -1309,6 +1427,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);
 
@@ -1347,6 +1467,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;
 
@@ -1410,6 +1531,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 --]

[PATCH v1 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 469bbb0ebdd9..56c88d2b4f87 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1380,7 +1380,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 v1 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Mark Brown]

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

On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote:

> +	for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
> +		ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
> +		if (IS_ERR(ep_reg)) {

Does PCI allow supplies to be physically absent?  If not then the driver
shouldn't be using regulator_get_optional() and much of the code here
can be deleted.

> +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{

This is open coding the regulator bulk APIs.

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

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

[Jim Quinlan]

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

On Thu, Nov 26, 2020 at 6:49 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote:
>
> > +     for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
> > +             ep_reg = devm_regulator_get_optional(dev, ep_regulator_names[i]);
> > +             if (IS_ERR(ep_reg)) {
>
> Does PCI allow supplies to be physically absent?  If not then the driver
> shouldn't be using regulator_get_optional() and much of the code here
> can be deleted.
Hi Mark,

First, as an aside, I'm  a little confused about the purpose of
devm_regulator_get_optional(...);  the other  xxx_get_optional() calls
I am familiar with (eg clock, reset, gpio) return NULL if the desired
item does not exist, and then NULL can be used as a valid pointer for
the rest of the API.  Not so here.

At any rate, our SOCs are placed in a variety of boards which
implement the PCIe RC-EP connection as they wish.  From the PCIe
driver's point of view, the type of power supply that needs to be
turned on is specified in the DT and they cannot be hard coded by the
driver.  I've listed all of the four possibilities; typically one,
maybe two will  be specified, but never all of them.  In addition,
sometimes a regulator is hard wired on and not even controllable by
the PCIe controller.

>
> > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
>
> This is open coding the regulator bulk APIs.
Except that a bulk regulator "get"  requires that all supplies are
present.  I would have to first scan the node's properties for the
"-supply" properties and fill in the bulk regulator structure.  I'm
fine with doing that.

However, a previous incarnation of this  commit was reviewed by RobH,
and if I understood him correctly he wanted the actual names of the
possible regulators to be used and specified in the bindings doc.   I
just followed the example of "pcie-rockchip-host.c" whose bindings doc
was reviewed by RobH.

Regards,
Jim Quinlan
Broadcom STB

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

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

[Mark Brown]

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

On Fri, Nov 27, 2020 at 03:26:53PM -0500, Jim Quinlan wrote:
> On Thu, Nov 26, 2020 at 6:49 AM Mark Brown <broonie@kernel.org> wrote:

> > Does PCI allow supplies to be physically absent?  If not then the driver
> > shouldn't be using regulator_get_optional() and much of the code here
> > can be deleted.

> First, as an aside, I'm  a little confused about the purpose of
> devm_regulator_get_optional(...);  the other  xxx_get_optional() calls
> I am familiar with (eg clock, reset, gpio) return NULL if the desired
> item does not exist, and then NULL can be used as a valid pointer for
> the rest of the API.  Not so here.

The other APIs that cloned the regulator API don't have the dummy
support that the regulator has and unfortunately changed the sense a bit
there.

> > > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > > +{

> > This is open coding the regulator bulk APIs.

> Except that a bulk regulator "get"  requires that all supplies are
> present.  I would have to first scan the node's properties for the
> "-supply" properties and fill in the bulk regulator structure.  I'm
> fine with doing that.

No, you should never do that.  If the supplies can be physically absent
then you should use regulator_get_optional() which allows you to do
whatever needs doing to configure the hardware for the missing supply.
If it's just that the supply may not be described in the DT but has to
be there for the device to operate then the code should use the normal
regualtor APIs - a dummy regulator will be provided if there's no supply
described.

> However, a previous incarnation of this  commit was reviewed by RobH,
> and if I understood him correctly he wanted the actual names of the
> possible regulators to be used and specified in the bindings doc.   I
> just followed the example of "pcie-rockchip-host.c" whose bindings doc
> was reviewed by RobH.

That is just plain bad code, the binding may well be fine but I can't
see any excuse for that driver to be using _optional() there.

Another subsystem I'm going to have to keep an eye on :(

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