* [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
@ 2021-04-27 17:51 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Florian Fainelli, Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Lorenzo Pieralisi, Rob Herring
v1 -- These commits were part of a previous pullreq but have
been split off because they are unrelated to said pullreq's
other more complex commits.
Jim Quinlan (4):
PCI: brcmstb: Check return value of clk_prepare_enable()
PCI: brcmstb: Give 7216 SOCs their own config type
PCI: brcmstb: Add panic/die handler to RC driver
PCI: brcmstb: add shutdown call to driver
drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
1 file changed, 143 insertions(+), 2 deletions(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH v1 1/4] PCI: brcmstb: Check return value of clk_prepare_enable()
2021-04-27 17:51 ` Jim Quinlan
@ 2021-04-27 17:51 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli, Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
Check for failure of clk_prepare_enable() on device resume.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
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 e330e6811f0b..4ce1f3a60574 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1161,7 +1161,9 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
base = pcie->base;
- 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v1 1/4] PCI: brcmstb: Check return value of clk_prepare_enable()
@ 2021-04-27 17:51 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli, Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
Check for failure of clk_prepare_enable() on device resume.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
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 e330e6811f0b..4ce1f3a60574 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1161,7 +1161,9 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
base = pcie->base;
- 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v1 2/4] PCI: brcmstb: Give 7216 SOCs their own config type
2021-04-27 17:51 ` Jim Quinlan
@ 2021-04-27 17:51 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
This distinction is required for an imminent commit.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.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 4ce1f3a60574..3b6a62dd2e72 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -257,6 +257,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;
@@ -1220,7 +1227,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v1 2/4] PCI: brcmstb: Give 7216 SOCs their own config type
@ 2021-04-27 17:51 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
This distinction is required for an imminent commit.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.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 4ce1f3a60574..3b6a62dd2e72 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -257,6 +257,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;
@@ -1220,7 +1227,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
2021-04-27 17:51 ` Jim Quinlan
@ 2021-04-27 17:51 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
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 <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
1 file changed, 122 insertions(+)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 3b6a62dd2e72..d3af8d84f0d6 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>
@@ -184,6 +186,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);
@@ -215,6 +250,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[] = {
@@ -262,6 +298,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,87 @@ 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);
+ 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;
+}
+
/*
* 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
@@ -1216,6 +1332,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);
@@ -1255,6 +1373,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;
@@ -1322,6 +1441,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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
@ 2021-04-27 17:51 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
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 <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
1 file changed, 122 insertions(+)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 3b6a62dd2e72..d3af8d84f0d6 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>
@@ -184,6 +186,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);
@@ -215,6 +250,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[] = {
@@ -262,6 +298,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,87 @@ 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);
+ 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;
+}
+
/*
* 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
@@ -1216,6 +1332,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);
@@ -1255,6 +1373,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;
@@ -1322,6 +1441,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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
2021-04-27 17:51 ` Jim Quinlan
@ 2021-05-25 20:40 ` Bjorn Helgaas
-1 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 20:40 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
Does this mean that errors that are recoverable on other hardware (by
noticing the 0xffffffff and checking for error) are fatal on the
Broadcom STB?
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
> 1 file changed, 122 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 3b6a62dd2e72..d3af8d84f0d6 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>
> @@ -184,6 +186,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);
> @@ -215,6 +250,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[] = {
> @@ -262,6 +298,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,87 @@ 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);
> + 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;
> +}
> +
> /*
> * 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
> @@ -1216,6 +1332,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);
>
> @@ -1255,6 +1373,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;
>
> @@ -1322,6 +1441,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
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
@ 2021-05-25 20:40 ` Bjorn Helgaas
0 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 20:40 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
Does this mean that errors that are recoverable on other hardware (by
noticing the 0xffffffff and checking for error) are fatal on the
Broadcom STB?
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
> 1 file changed, 122 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 3b6a62dd2e72..d3af8d84f0d6 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>
> @@ -184,6 +186,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);
> @@ -215,6 +250,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[] = {
> @@ -262,6 +298,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,87 @@ 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);
> + 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;
> +}
> +
> /*
> * 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
> @@ -1216,6 +1332,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);
>
> @@ -1255,6 +1373,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;
>
> @@ -1322,6 +1441,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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
2021-05-25 20:40 ` Bjorn Helgaas
@ 2021-05-25 21:05 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-05-25 21:05 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1: Type: text/plain, Size: 11252 bytes --]
On Tue, May 25, 2021 at 4:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
The entire system dies from the abort. Some customers elect to do a
fixup in the abort handler but we admonish them to fix the root cause.
With these patches we at least get immediate information about the
access that caused the abort.
>
>
> Does this mean that errors that are recoverable on other hardware (by
> noticing the 0xffffffff and checking for error) are fatal on the
> Broadcom STB?
Yes. For example, I have an old Rocketport RP2 card I sometimes use
for testing. On a Broadcom STB it dies when the rp2 probe does a
read after calling rp2_reset_asic(). On an x86, 0xffffffff is
returned on this read and all is well.
I don't think there is any PCIe spec that mandates an access error
returns 0xffffffff. Some of our SOCs have a new feature where we can
return the 0xffffffff instead of getting an abort. We will allow the
customer to turn this on if they ask for it, but for the time being we
prefer an abort as many drivers do not check for 0xffffffff.
Regards,
Jim Quinlan
Broadcom STB
>
>
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
> > 1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 3b6a62dd2e72..d3af8d84f0d6 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>
> > @@ -184,6 +186,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);
> > @@ -215,6 +250,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[] = {
> > @@ -262,6 +298,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,87 @@ 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);
> > + 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;
> > +}
> > +
> > /*
> > * 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
> > @@ -1216,6 +1332,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);
> >
> > @@ -1255,6 +1373,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;
> >
> > @@ -1322,6 +1441,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: 4210 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
@ 2021-05-25 21:05 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-05-25 21:05 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1.1: Type: text/plain, Size: 11252 bytes --]
On Tue, May 25, 2021 at 4:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
The entire system dies from the abort. Some customers elect to do a
fixup in the abort handler but we admonish them to fix the root cause.
With these patches we at least get immediate information about the
access that caused the abort.
>
>
> Does this mean that errors that are recoverable on other hardware (by
> noticing the 0xffffffff and checking for error) are fatal on the
> Broadcom STB?
Yes. For example, I have an old Rocketport RP2 card I sometimes use
for testing. On a Broadcom STB it dies when the rp2 probe does a
read after calling rp2_reset_asic(). On an x86, 0xffffffff is
returned on this read and all is well.
I don't think there is any PCIe spec that mandates an access error
returns 0xffffffff. Some of our SOCs have a new feature where we can
return the 0xffffffff instead of getting an abort. We will allow the
customer to turn this on if they ask for it, but for the time being we
prefer an abort as many drivers do not check for 0xffffffff.
Regards,
Jim Quinlan
Broadcom STB
>
>
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
> > 1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 3b6a62dd2e72..d3af8d84f0d6 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>
> > @@ -184,6 +186,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);
> > @@ -215,6 +250,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[] = {
> > @@ -262,6 +298,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,87 @@ 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);
> > + 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;
> > +}
> > +
> > /*
> > * 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
> > @@ -1216,6 +1332,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);
> >
> > @@ -1255,6 +1373,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;
> >
> > @@ -1322,6 +1441,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 #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
2021-05-25 21:05 ` Jim Quinlan
@ 2021-05-25 21:17 ` Bjorn Helgaas
-1 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 21:17 UTC (permalink / raw)
To: Jim Quinlan
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, May 25, 2021 at 05:05:51PM -0400, Jim Quinlan wrote:
> On Tue, May 25, 2021 at 4:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
>
> The entire system dies from the abort. Some customers elect to do a
> fixup in the abort handler but we admonish them to fix the root cause.
> With these patches we at least get immediate information about the
> access that caused the abort.
> >
> > Does this mean that errors that are recoverable on other hardware (by
> > noticing the 0xffffffff and checking for error) are fatal on the
> > Broadcom STB?
>
> Yes. For example, I have an old Rocketport RP2 card I sometimes use
> for testing. On a Broadcom STB it dies when the rp2 probe does a
> read after calling rp2_reset_asic(). On an x86, 0xffffffff is
> returned on this read and all is well.
>
> I don't think there is any PCIe spec that mandates an access error
> returns 0xffffffff. Some of our SOCs have a new feature where we can
> return the 0xffffffff instead of getting an abort. We will allow the
> customer to turn this on if they ask for it, but for the time being we
> prefer an abort as many drivers do not check for 0xffffffff.
Right, the mechanism of error reporting is an implementation choice.
Few drivers are actually prepared to deal with 0xffffffff data, but in
many systems, especially those with removable PCI devices, it is
important to be able to report errors in a way that doesn't crash the
system.
That may not be a concern in your system, so maybe just mention that
this is a fatal error for the system in the commit log.
Bjorn
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver
@ 2021-05-25 21:17 ` Bjorn Helgaas
0 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 21:17 UTC (permalink / raw)
To: Jim Quinlan
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, May 25, 2021 at 05:05:51PM -0400, Jim Quinlan wrote:
> On Tue, May 25, 2021 at 4:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Apr 27, 2021 at 01:51:38PM -0400, 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 happens to the driver that performed the illegal access?
>
> The entire system dies from the abort. Some customers elect to do a
> fixup in the abort handler but we admonish them to fix the root cause.
> With these patches we at least get immediate information about the
> access that caused the abort.
> >
> > Does this mean that errors that are recoverable on other hardware (by
> > noticing the 0xffffffff and checking for error) are fatal on the
> > Broadcom STB?
>
> Yes. For example, I have an old Rocketport RP2 card I sometimes use
> for testing. On a Broadcom STB it dies when the rp2 probe does a
> read after calling rp2_reset_asic(). On an x86, 0xffffffff is
> returned on this read and all is well.
>
> I don't think there is any PCIe spec that mandates an access error
> returns 0xffffffff. Some of our SOCs have a new feature where we can
> return the 0xffffffff instead of getting an abort. We will allow the
> customer to turn this on if they ask for it, but for the time being we
> prefer an abort as many drivers do not check for 0xffffffff.
Right, the mechanism of error reporting is an implementation choice.
Few drivers are actually prepared to deal with 0xffffffff data, but in
many systems, especially those with removable PCI devices, it is
important to be able to report errors in a way that doesn't crash the
system.
That may not be a concern in your system, so maybe just mention that
this is a fatal error for the system in the commit log.
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-04-27 17:51 ` Jim Quinlan
@ 2021-04-27 17:51 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
The shutdown() call is similar to the remove() call except the former does
not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
if it does.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d3af8d84f0d6..a1fe1a2ada48 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
return 0;
}
+static void brcm_pcie_shutdown(struct platform_device *pdev)
+{
+ struct brcm_pcie *pcie = platform_get_drvdata(pdev);
+
+ if (pcie->has_err_report)
+ brcm_unregister_die_notifiers(pcie);
+ __brcm_pcie_remove(pcie);
+}
+
static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
static struct platform_driver brcm_pcie_driver = {
.probe = brcm_pcie_probe,
.remove = brcm_pcie_remove,
+ .shutdown = brcm_pcie_shutdown,
.driver = {
.name = "brcm-pcie",
.of_match_table = brcm_pcie_match,
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-04-27 17:51 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-04-27 17:51 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
The shutdown() call is similar to the remove() call except the former does
not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
if it does.
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d3af8d84f0d6..a1fe1a2ada48 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
return 0;
}
+static void brcm_pcie_shutdown(struct platform_device *pdev)
+{
+ struct brcm_pcie *pcie = platform_get_drvdata(pdev);
+
+ if (pcie->has_err_report)
+ brcm_unregister_die_notifiers(pcie);
+ __brcm_pcie_remove(pcie);
+}
+
static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
static struct platform_driver brcm_pcie_driver = {
.probe = brcm_pcie_probe,
.remove = brcm_pcie_remove,
+ .shutdown = brcm_pcie_shutdown,
.driver = {
.name = "brcm-pcie",
.of_match_table = brcm_pcie_match,
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-04-27 17:51 ` Jim Quinlan
@ 2021-04-27 19:35 ` Florian Fainelli
-1 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-04-27 19:35 UTC (permalink / raw)
To: Jim Quinlan, linux-pci, Bjorn Helgaas, bcm-kernel-feedback-list,
james.quinlan
Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 4/27/2021 10:51 AM, Jim Quinlan wrote:
> The shutdown() call is similar to the remove() call except the former does
> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> if it does.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-04-27 19:35 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-04-27 19:35 UTC (permalink / raw)
To: Jim Quinlan, linux-pci, Bjorn Helgaas, bcm-kernel-feedback-list,
james.quinlan
Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 4/27/2021 10:51 AM, Jim Quinlan wrote:
> The shutdown() call is similar to the remove() call except the former does
> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> if it does.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-04-27 17:51 ` Jim Quinlan
@ 2021-05-25 21:18 ` Bjorn Helgaas
-1 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 21:18 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
Capitalize "Add" in the subject.
On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> The shutdown() call is similar to the remove() call except the former does
> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> if it does.
This doesn't explain why shutdown() is necessary. "errors occur"
might be a hint, except that AFAICT, many similar drivers do invoke
pci_stop_root_bus() and pci_remove_root_bus() (several of them while
holding pci_lock_rescan_remove()), without implementing .shutdown().
It is ... unfortunate that there's such a variety of implementations
here. I don't believe these driver differences are all necessary
consequences of hardware differences.
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d3af8d84f0d6..a1fe1a2ada48 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static void brcm_pcie_shutdown(struct platform_device *pdev)
> +{
> + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> +
> + if (pcie->has_err_report)
> + brcm_unregister_die_notifiers(pcie);
> + __brcm_pcie_remove(pcie);
> +}
> +
> static const struct of_device_id brcm_pcie_match[] = {
> { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
> static struct platform_driver brcm_pcie_driver = {
> .probe = brcm_pcie_probe,
> .remove = brcm_pcie_remove,
> + .shutdown = brcm_pcie_shutdown,
> .driver = {
> .name = "brcm-pcie",
> .of_match_table = brcm_pcie_match,
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-05-25 21:18 ` Bjorn Helgaas
0 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-05-25 21:18 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
Capitalize "Add" in the subject.
On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> The shutdown() call is similar to the remove() call except the former does
> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> if it does.
This doesn't explain why shutdown() is necessary. "errors occur"
might be a hint, except that AFAICT, many similar drivers do invoke
pci_stop_root_bus() and pci_remove_root_bus() (several of them while
holding pci_lock_rescan_remove()), without implementing .shutdown().
It is ... unfortunate that there's such a variety of implementations
here. I don't believe these driver differences are all necessary
consequences of hardware differences.
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d3af8d84f0d6..a1fe1a2ada48 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static void brcm_pcie_shutdown(struct platform_device *pdev)
> +{
> + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> +
> + if (pcie->has_err_report)
> + brcm_unregister_die_notifiers(pcie);
> + __brcm_pcie_remove(pcie);
> +}
> +
> static const struct of_device_id brcm_pcie_match[] = {
> { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
> static struct platform_driver brcm_pcie_driver = {
> .probe = brcm_pcie_probe,
> .remove = brcm_pcie_remove,
> + .shutdown = brcm_pcie_shutdown,
> .driver = {
> .name = "brcm-pcie",
> .of_match_table = brcm_pcie_match,
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-05-25 21:18 ` Bjorn Helgaas
@ 2021-05-25 21:40 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-05-25 21:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]
On Tue, May 25, 2021 at 5:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> > The shutdown() call is similar to the remove() call except the former does
> > not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> > if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
Fair enough, I'll dig into the error.
Regards,
Jim Quinlan
Broadcom STB
>
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index d3af8d84f0d6..a1fe1a2ada48 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static void brcm_pcie_shutdown(struct platform_device *pdev)
> > +{
> > + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> > +
> > + if (pcie->has_err_report)
> > + brcm_unregister_die_notifiers(pcie);
> > + __brcm_pcie_remove(pcie);
> > +}
> > +
> > static const struct of_device_id brcm_pcie_match[] = {
> > { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> > { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> > @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
> > static struct platform_driver brcm_pcie_driver = {
> > .probe = brcm_pcie_probe,
> > .remove = brcm_pcie_remove,
> > + .shutdown = brcm_pcie_shutdown,
> > .driver = {
> > .name = "brcm-pcie",
> > .of_match_table = brcm_pcie_match,
> > --
> > 2.17.1
> >
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-05-25 21:40 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-05-25 21:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1.1: Type: text/plain, Size: 2273 bytes --]
On Tue, May 25, 2021 at 5:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> > The shutdown() call is similar to the remove() call except the former does
> > not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> > if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
Fair enough, I'll dig into the error.
Regards,
Jim Quinlan
Broadcom STB
>
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index d3af8d84f0d6..a1fe1a2ada48 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static void brcm_pcie_shutdown(struct platform_device *pdev)
> > +{
> > + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> > +
> > + if (pcie->has_err_report)
> > + brcm_unregister_die_notifiers(pcie);
> > + __brcm_pcie_remove(pcie);
> > +}
> > +
> > static const struct of_device_id brcm_pcie_match[] = {
> > { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> > { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> > @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
> > static struct platform_driver brcm_pcie_driver = {
> > .probe = brcm_pcie_probe,
> > .remove = brcm_pcie_remove,
> > + .shutdown = brcm_pcie_shutdown,
> > .driver = {
> > .name = "brcm-pcie",
> > .of_match_table = brcm_pcie_match,
> > --
> > 2.17.1
> >
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-05-25 21:18 ` Bjorn Helgaas
@ 2021-05-26 16:11 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2021-05-26 16:11 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan, PCI, Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Jim Quinlan,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, May 25, 2021 at 4:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> > The shutdown() call is similar to the remove() call except the former does
> > not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> > if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
What's correct here?
This was on my radar and something we should get in front of. It's
only a matter of time until everyone is converting their drivers to
modules since that is now the Android WayTM. My plan here was to
register a devm hook to call pci_stop_root_bus() and
pci_remove_root_bus(). That way every driver doesn't have to implement
a remove() hook. Though maybe they all need to quiesce themselves
anyways.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-05-26 16:11 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2021-05-26 16:11 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan, PCI, Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Jim Quinlan,
Nicolas Saenz Julienne, Lorenzo Pieralisi, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Tue, May 25, 2021 at 4:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> > The shutdown() call is similar to the remove() call except the former does
> > not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> > if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
What's correct here?
This was on my radar and something we should get in front of. It's
only a matter of time until everyone is converting their drivers to
modules since that is now the Android WayTM. My plan here was to
register a devm hook to call pci_stop_root_bus() and
pci_remove_root_bus(). That way every driver doesn't have to implement
a remove() hook. Though maybe they all need to quiesce themselves
anyways.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-05-25 21:18 ` Bjorn Helgaas
@ 2021-05-26 17:03 ` Florian Fainelli
-1 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-05-26 17:03 UTC (permalink / raw)
To: Bjorn Helgaas, Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>> The shutdown() call is similar to the remove() call except the former does
>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>> if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
We have to implement .shutdown() in order to meet a certain power budget
while the chip is being put into S5 (soft off) state and still support
Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
power savings at the wall. We could probably add a word or two in a v2
that indicates this is done for power savings.
As far as the crash goes, if we call brcm_pcie_remove() directly from
brcm_pcie_shutdown() we see the following (this is in 5.4, but I have no
reason to believe mainline is different):
# lspci
0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7216 (rev 10)
0001:01:00.0 Network controller: Qualcomm Atheros AR9285 Wireless
Network Adapter (PCI-Express) (rev 01)
# reboot -f
[ 8.465111] ------------[ cut here ]------------
[ 8.469771] pcieport 0001:00:00.0: disabling already-disabled device
[ 8.469817] WARNING: CPU: 3 PID: 1656 at drivers/pci/pci.c:1945
pci_disable_device+0x78/0xd0
[ 8.484631] Modules linked in:
[ 8.487695] CPU: 3 PID: 1656 Comm: reboot Not tainted
5.4.120-1.1pre-g231f2cfa989a #4
[ 8.495537] Hardware name: BCX972160DV (DT)
[ 8.499728] pstate: 60000005 (nZCv daif -PAN -UAO)
[ 8.504527] pc : pci_disable_device+0x78/0xd0
[ 8.508891] lr : pci_disable_device+0x78/0xd0
[ 8.513254] sp : ffffffc014d33aa0
[ 8.516572] x29: ffffffc014d33aa0 x28: ffffff809deac800
[ 8.521894] x27: ffffff809e23a5b0 x26: ffffff809e23a490
[ 8.527214] x25: 0000000000000000 x24: ffffff809cf9f000
[ 8.532535] x23: ffffffc011d13090 x22: 0000000000000000
[ 8.537856] x21: 0000000000000000 x20: ffffff809cf9f0b0
[ 8.543175] x19: ffffff809cf9f000 x18: 000000000000000a
[ 8.548495] x17: 0000000000000000 x16: 0000000000000000
[ 8.553816] x15: 00000000000718d7 x14: 0720072007200720
[ 8.559138] x13: 0720072007200720 x12: 0720072007200720
[ 8.564458] x11: 0720072007200720 x10: 0720072007200720
[ 8.569777] x9 : 0720072007200720 x8 : 656c62617369642d
[ 8.575098] x7 : 79646165726c6120 x6 : ffffffc011dc3630
[ 8.580417] x5 : 0000000000000038 x4 : 0000000000000000
[ 8.585737] x3 : 0000000000000000 x2 : 0000000000000000
[ 8.591058] x1 : 73b2329426297100 x0 : 0000000000000000
[ 8.596379] Call trace:
[ 8.598832] pci_disable_device+0x78/0xd0
[ 8.602852] pcie_port_device_remove+0x3c/0x48
[ 8.607304] pcie_portdrv_remove+0x5c/0x8c
[ 8.611408] pci_device_remove+0x98/0xe8
[ 8.615341] device_release_driver_internal+0xb0/0x188
[ 8.620487] device_release_driver+0x28/0x34
[ 8.624765] pci_stop_bus_device+0x40/0xa8
[ 8.628867] pci_stop_root_bus+0x58/0x64
[ 8.632797] brcm_pcie_remove+0x24/0x70
[ 8.636640] brcm_pcie_shutdown+0x20/0x2c
[ 8.640655] platform_drv_shutdown+0x2c/0x38
[ 8.644934] device_shutdown+0x16c/0x1e0
[ 8.648865] kernel_restart_prepare+0x44/0x50
[ 8.653229] kernel_restart+0x20/0x68
[ 8.656896] __do_sys_reboot+0x158/0x200
[ 8.660816] __arm64_sys_reboot+0x2c/0x38
[ 8.664833] el0_svc_common.constprop.2+0xe8/0x168
[ 8.669631] el0_svc_handler+0x34/0x80
[ 8.673388] el0_svc+0x8/0x1fc
[ 8.676447] ---[ end trace 778d7966a5ef8213 ]---
[ 8.694539] pci_bus 0001:01: busn_res: [bus 01] is released
[ 8.700279] pci_bus 0001:00: busn_res: [bus 00-ff] is released
[ 8.710206] reboot: Restarting system
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
Yes, that is a bit unfortunate, Rob's idea to consolidate the
implementation sounds good and we can definitively test that.
>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>> drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index d3af8d84f0d6..a1fe1a2ada48 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static void brcm_pcie_shutdown(struct platform_device *pdev)
>> +{
>> + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> + if (pcie->has_err_report)
>> + brcm_unregister_die_notifiers(pcie);
>> + __brcm_pcie_remove(pcie);
>> +}
>> +
>> static const struct of_device_id brcm_pcie_match[] = {
>> { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>> { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
>> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
>> static struct platform_driver brcm_pcie_driver = {
>> .probe = brcm_pcie_probe,
>> .remove = brcm_pcie_remove,
>> + .shutdown = brcm_pcie_shutdown,
>> .driver = {
>> .name = "brcm-pcie",
>> .of_match_table = brcm_pcie_match,
>> --
>> 2.17.1
>>
--
Florian
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-05-26 17:03 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-05-26 17:03 UTC (permalink / raw)
To: Bjorn Helgaas, Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring, Florian Fainelli,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> Capitalize "Add" in the subject.
>
> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>> The shutdown() call is similar to the remove() call except the former does
>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>> if it does.
>
> This doesn't explain why shutdown() is necessary. "errors occur"
> might be a hint, except that AFAICT, many similar drivers do invoke
> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> holding pci_lock_rescan_remove()), without implementing .shutdown().
We have to implement .shutdown() in order to meet a certain power budget
while the chip is being put into S5 (soft off) state and still support
Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
power savings at the wall. We could probably add a word or two in a v2
that indicates this is done for power savings.
As far as the crash goes, if we call brcm_pcie_remove() directly from
brcm_pcie_shutdown() we see the following (this is in 5.4, but I have no
reason to believe mainline is different):
# lspci
0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7216 (rev 10)
0001:01:00.0 Network controller: Qualcomm Atheros AR9285 Wireless
Network Adapter (PCI-Express) (rev 01)
# reboot -f
[ 8.465111] ------------[ cut here ]------------
[ 8.469771] pcieport 0001:00:00.0: disabling already-disabled device
[ 8.469817] WARNING: CPU: 3 PID: 1656 at drivers/pci/pci.c:1945
pci_disable_device+0x78/0xd0
[ 8.484631] Modules linked in:
[ 8.487695] CPU: 3 PID: 1656 Comm: reboot Not tainted
5.4.120-1.1pre-g231f2cfa989a #4
[ 8.495537] Hardware name: BCX972160DV (DT)
[ 8.499728] pstate: 60000005 (nZCv daif -PAN -UAO)
[ 8.504527] pc : pci_disable_device+0x78/0xd0
[ 8.508891] lr : pci_disable_device+0x78/0xd0
[ 8.513254] sp : ffffffc014d33aa0
[ 8.516572] x29: ffffffc014d33aa0 x28: ffffff809deac800
[ 8.521894] x27: ffffff809e23a5b0 x26: ffffff809e23a490
[ 8.527214] x25: 0000000000000000 x24: ffffff809cf9f000
[ 8.532535] x23: ffffffc011d13090 x22: 0000000000000000
[ 8.537856] x21: 0000000000000000 x20: ffffff809cf9f0b0
[ 8.543175] x19: ffffff809cf9f000 x18: 000000000000000a
[ 8.548495] x17: 0000000000000000 x16: 0000000000000000
[ 8.553816] x15: 00000000000718d7 x14: 0720072007200720
[ 8.559138] x13: 0720072007200720 x12: 0720072007200720
[ 8.564458] x11: 0720072007200720 x10: 0720072007200720
[ 8.569777] x9 : 0720072007200720 x8 : 656c62617369642d
[ 8.575098] x7 : 79646165726c6120 x6 : ffffffc011dc3630
[ 8.580417] x5 : 0000000000000038 x4 : 0000000000000000
[ 8.585737] x3 : 0000000000000000 x2 : 0000000000000000
[ 8.591058] x1 : 73b2329426297100 x0 : 0000000000000000
[ 8.596379] Call trace:
[ 8.598832] pci_disable_device+0x78/0xd0
[ 8.602852] pcie_port_device_remove+0x3c/0x48
[ 8.607304] pcie_portdrv_remove+0x5c/0x8c
[ 8.611408] pci_device_remove+0x98/0xe8
[ 8.615341] device_release_driver_internal+0xb0/0x188
[ 8.620487] device_release_driver+0x28/0x34
[ 8.624765] pci_stop_bus_device+0x40/0xa8
[ 8.628867] pci_stop_root_bus+0x58/0x64
[ 8.632797] brcm_pcie_remove+0x24/0x70
[ 8.636640] brcm_pcie_shutdown+0x20/0x2c
[ 8.640655] platform_drv_shutdown+0x2c/0x38
[ 8.644934] device_shutdown+0x16c/0x1e0
[ 8.648865] kernel_restart_prepare+0x44/0x50
[ 8.653229] kernel_restart+0x20/0x68
[ 8.656896] __do_sys_reboot+0x158/0x200
[ 8.660816] __arm64_sys_reboot+0x2c/0x38
[ 8.664833] el0_svc_common.constprop.2+0xe8/0x168
[ 8.669631] el0_svc_handler+0x34/0x80
[ 8.673388] el0_svc+0x8/0x1fc
[ 8.676447] ---[ end trace 778d7966a5ef8213 ]---
[ 8.694539] pci_bus 0001:01: busn_res: [bus 01] is released
[ 8.700279] pci_bus 0001:00: busn_res: [bus 00-ff] is released
[ 8.710206] reboot: Restarting system
>
> It is ... unfortunate that there's such a variety of implementations
> here. I don't believe these driver differences are all necessary
> consequences of hardware differences.
Yes, that is a bit unfortunate, Rob's idea to consolidate the
implementation sounds good and we can definitively test that.
>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>> drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index d3af8d84f0d6..a1fe1a2ada48 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static void brcm_pcie_shutdown(struct platform_device *pdev)
>> +{
>> + struct brcm_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> + if (pcie->has_err_report)
>> + brcm_unregister_die_notifiers(pcie);
>> + __brcm_pcie_remove(pcie);
>> +}
>> +
>> static const struct of_device_id brcm_pcie_match[] = {
>> { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>> { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
>> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
>> static struct platform_driver brcm_pcie_driver = {
>> .probe = brcm_pcie_probe,
>> .remove = brcm_pcie_remove,
>> + .shutdown = brcm_pcie_shutdown,
>> .driver = {
>> .name = "brcm-pcie",
>> .of_match_table = brcm_pcie_match,
>> --
>> 2.17.1
>>
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-05-26 17:03 ` Florian Fainelli
@ 2021-06-03 17:23 ` Bjorn Helgaas
-1 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-06-03 17:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> > On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> >> The shutdown() call is similar to the remove() call except the former does
> >> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> >> if it does.
> >
> > This doesn't explain why shutdown() is necessary. "errors occur"
> > might be a hint, except that AFAICT, many similar drivers do invoke
> > pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> > holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> We have to implement .shutdown() in order to meet a certain power budget
> while the chip is being put into S5 (soft off) state and still support
> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
> power savings at the wall. We could probably add a word or two in a v2
> that indicates this is done for power savings.
"Saving power" is a great reason to do this. But we still need to
connect this to the driver model and the system-level behavior
somehow.
The pci_driver comment says @shutdown is to "stop idling DMA
operations" and it hooks into reboot_notifier_list in kernel/sys.c.
That's incorrect or at least incomplete because reboot_notifier_list
isn't mentioned at all in kernel/sys.c, and I don't see the connection
between @shutdown and reboot_notifier_list.
AFAICT, @shutdown is currently used in this path:
kernel_restart_prepare or kernel_shutdown_prepare
device_shutdown
dev->bus->shutdown
pci_device_shutdown # pci_bus_type.shutdown
drv->shutdown
so we're going to either reboot or halt/power-off the entire system,
and we're not going to use this device again until we're in a
brand-new kernel and we re-enumerate the device and re-register the
driver.
I'm not quite sure how either of those fits into the power-saving
reason. I guess going to S5 is probably via the kernel_power_off()
path and that by itself doesn't turn off as much power to the PCIe
controller as it could? And this new .shutdown() method will get
called in that path and will turn off more power, but will still leave
enough for wake-on-LAN to work? And when we *do* wake from S5,
obviously that means a complete boot with a new kernel.
Bjorn
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-06-03 17:23 ` Bjorn Helgaas
0 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-06-03 17:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> > On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> >> The shutdown() call is similar to the remove() call except the former does
> >> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> >> if it does.
> >
> > This doesn't explain why shutdown() is necessary. "errors occur"
> > might be a hint, except that AFAICT, many similar drivers do invoke
> > pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> > holding pci_lock_rescan_remove()), without implementing .shutdown().
>
> We have to implement .shutdown() in order to meet a certain power budget
> while the chip is being put into S5 (soft off) state and still support
> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
> power savings at the wall. We could probably add a word or two in a v2
> that indicates this is done for power savings.
"Saving power" is a great reason to do this. But we still need to
connect this to the driver model and the system-level behavior
somehow.
The pci_driver comment says @shutdown is to "stop idling DMA
operations" and it hooks into reboot_notifier_list in kernel/sys.c.
That's incorrect or at least incomplete because reboot_notifier_list
isn't mentioned at all in kernel/sys.c, and I don't see the connection
between @shutdown and reboot_notifier_list.
AFAICT, @shutdown is currently used in this path:
kernel_restart_prepare or kernel_shutdown_prepare
device_shutdown
dev->bus->shutdown
pci_device_shutdown # pci_bus_type.shutdown
drv->shutdown
so we're going to either reboot or halt/power-off the entire system,
and we're not going to use this device again until we're in a
brand-new kernel and we re-enumerate the device and re-register the
driver.
I'm not quite sure how either of those fits into the power-saving
reason. I guess going to S5 is probably via the kernel_power_off()
path and that by itself doesn't turn off as much power to the PCIe
controller as it could? And this new .shutdown() method will get
called in that path and will turn off more power, but will still leave
enough for wake-on-LAN to work? And when we *do* wake from S5,
obviously that means a complete boot with a new kernel.
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-06-03 17:23 ` Bjorn Helgaas
@ 2021-06-03 17:30 ` Florian Fainelli
-1 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-06-03 17:30 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
>> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
>>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>>>> The shutdown() call is similar to the remove() call except the former does
>>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>>>> if it does.
>>>
>>> This doesn't explain why shutdown() is necessary. "errors occur"
>>> might be a hint, except that AFAICT, many similar drivers do invoke
>>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
>>> holding pci_lock_rescan_remove()), without implementing .shutdown().
>>
>> We have to implement .shutdown() in order to meet a certain power budget
>> while the chip is being put into S5 (soft off) state and still support
>> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
>> power savings at the wall. We could probably add a word or two in a v2
>> that indicates this is done for power savings.
>
> "Saving power" is a great reason to do this. But we still need to
> connect this to the driver model and the system-level behavior
> somehow.
>
> The pci_driver comment says @shutdown is to "stop idling DMA
> operations" and it hooks into reboot_notifier_list in kernel/sys.c.
> That's incorrect or at least incomplete because reboot_notifier_list
> isn't mentioned at all in kernel/sys.c, and I don't see the connection
> between @shutdown and reboot_notifier_list.
>
> AFAICT, @shutdown is currently used in this path:
>
> kernel_restart_prepare or kernel_shutdown_prepare
> device_shutdown
> dev->bus->shutdown
> pci_device_shutdown # pci_bus_type.shutdown
> drv->shutdown
>
> so we're going to either reboot or halt/power-off the entire system,
> and we're not going to use this device again until we're in a
> brand-new kernel and we re-enumerate the device and re-register the
> driver.
>
> I'm not quite sure how either of those fits into the power-saving
> reason. I guess going to S5 is probably via the kernel_power_off()
> path and that by itself doesn't turn off as much power to the PCIe
> controller as it could? And this new .shutdown() method will get
> called in that path and will turn off more power, but will still leave
> enough for wake-on-LAN to work? And when we *do* wake from S5,
> obviously that means a complete boot with a new kernel.
Correct, the S5 shutdown is via kernel_power_off() and will turn off all
that we can in the PCIe root complex and its PHY, drop the PCIe link to
the end-point which signals that the end-point can enter its own suspend
logic, too. And yes, when we do wake-up from S5 it means booting a
completely new kernel. S5 is typically implemented in our chips by
keeping just a little bit of logic active to service wake-up events
(infrared remotes, GPIOs, RTC, etc.).
--
Florian
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-06-03 17:30 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-06-03 17:30 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
>> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
>>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>>>> The shutdown() call is similar to the remove() call except the former does
>>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>>>> if it does.
>>>
>>> This doesn't explain why shutdown() is necessary. "errors occur"
>>> might be a hint, except that AFAICT, many similar drivers do invoke
>>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
>>> holding pci_lock_rescan_remove()), without implementing .shutdown().
>>
>> We have to implement .shutdown() in order to meet a certain power budget
>> while the chip is being put into S5 (soft off) state and still support
>> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
>> power savings at the wall. We could probably add a word or two in a v2
>> that indicates this is done for power savings.
>
> "Saving power" is a great reason to do this. But we still need to
> connect this to the driver model and the system-level behavior
> somehow.
>
> The pci_driver comment says @shutdown is to "stop idling DMA
> operations" and it hooks into reboot_notifier_list in kernel/sys.c.
> That's incorrect or at least incomplete because reboot_notifier_list
> isn't mentioned at all in kernel/sys.c, and I don't see the connection
> between @shutdown and reboot_notifier_list.
>
> AFAICT, @shutdown is currently used in this path:
>
> kernel_restart_prepare or kernel_shutdown_prepare
> device_shutdown
> dev->bus->shutdown
> pci_device_shutdown # pci_bus_type.shutdown
> drv->shutdown
>
> so we're going to either reboot or halt/power-off the entire system,
> and we're not going to use this device again until we're in a
> brand-new kernel and we re-enumerate the device and re-register the
> driver.
>
> I'm not quite sure how either of those fits into the power-saving
> reason. I guess going to S5 is probably via the kernel_power_off()
> path and that by itself doesn't turn off as much power to the PCIe
> controller as it could? And this new .shutdown() method will get
> called in that path and will turn off more power, but will still leave
> enough for wake-on-LAN to work? And when we *do* wake from S5,
> obviously that means a complete boot with a new kernel.
Correct, the S5 shutdown is via kernel_power_off() and will turn off all
that we can in the PCIe root complex and its PHY, drop the PCIe link to
the end-point which signals that the end-point can enter its own suspend
logic, too. And yes, when we do wake-up from S5 it means booting a
completely new kernel. S5 is typically implemented in our chips by
keeping just a little bit of logic active to service wake-up events
(infrared remotes, GPIOs, RTC, etc.).
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-06-03 17:30 ` Florian Fainelli
@ 2021-06-03 20:58 ` Bjorn Helgaas
-1 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-06-03 20:58 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> > On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
> >> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> >>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> >>>> The shutdown() call is similar to the remove() call except the former does
> >>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> >>>> if it does.
> >>>
> >>> This doesn't explain why shutdown() is necessary. "errors occur"
> >>> might be a hint, except that AFAICT, many similar drivers do invoke
> >>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> >>> holding pci_lock_rescan_remove()), without implementing .shutdown().
> >>
> >> We have to implement .shutdown() in order to meet a certain power budget
> >> while the chip is being put into S5 (soft off) state and still support
> >> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
> >> power savings at the wall. We could probably add a word or two in a v2
> >> that indicates this is done for power savings.
> >
> > "Saving power" is a great reason to do this. But we still need to
> > connect this to the driver model and the system-level behavior
> > somehow.
> >
> > The pci_driver comment says @shutdown is to "stop idling DMA
> > operations" and it hooks into reboot_notifier_list in kernel/sys.c.
> > That's incorrect or at least incomplete because reboot_notifier_list
> > isn't mentioned at all in kernel/sys.c, and I don't see the connection
> > between @shutdown and reboot_notifier_list.
> >
> > AFAICT, @shutdown is currently used in this path:
> >
> > kernel_restart_prepare or kernel_shutdown_prepare
> > device_shutdown
> > dev->bus->shutdown
> > pci_device_shutdown # pci_bus_type.shutdown
> > drv->shutdown
> >
> > so we're going to either reboot or halt/power-off the entire system,
> > and we're not going to use this device again until we're in a
> > brand-new kernel and we re-enumerate the device and re-register the
> > driver.
> >
> > I'm not quite sure how either of those fits into the power-saving
> > reason. I guess going to S5 is probably via the kernel_power_off()
> > path and that by itself doesn't turn off as much power to the PCIe
> > controller as it could? And this new .shutdown() method will get
> > called in that path and will turn off more power, but will still leave
> > enough for wake-on-LAN to work? And when we *do* wake from S5,
> > obviously that means a complete boot with a new kernel.
>
> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
> that we can in the PCIe root complex and its PHY, drop the PCIe link to
> the end-point which signals that the end-point can enter its own suspend
> logic, too. And yes, when we do wake-up from S5 it means booting a
> completely new kernel. S5 is typically implemented in our chips by
> keeping just a little bit of logic active to service wake-up events
> (infrared remotes, GPIOs, RTC, etc.).
Which part of that does this patch change? Is it that the new
.shutdown() turns off more power than machine_power_off() does by
itself?
Bjorn
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-06-03 20:58 ` Bjorn Helgaas
0 siblings, 0 replies; 43+ messages in thread
From: Bjorn Helgaas @ 2021-06-03 20:58 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> > On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
> >> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> >>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> >>>> The shutdown() call is similar to the remove() call except the former does
> >>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> >>>> if it does.
> >>>
> >>> This doesn't explain why shutdown() is necessary. "errors occur"
> >>> might be a hint, except that AFAICT, many similar drivers do invoke
> >>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> >>> holding pci_lock_rescan_remove()), without implementing .shutdown().
> >>
> >> We have to implement .shutdown() in order to meet a certain power budget
> >> while the chip is being put into S5 (soft off) state and still support
> >> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
> >> power savings at the wall. We could probably add a word or two in a v2
> >> that indicates this is done for power savings.
> >
> > "Saving power" is a great reason to do this. But we still need to
> > connect this to the driver model and the system-level behavior
> > somehow.
> >
> > The pci_driver comment says @shutdown is to "stop idling DMA
> > operations" and it hooks into reboot_notifier_list in kernel/sys.c.
> > That's incorrect or at least incomplete because reboot_notifier_list
> > isn't mentioned at all in kernel/sys.c, and I don't see the connection
> > between @shutdown and reboot_notifier_list.
> >
> > AFAICT, @shutdown is currently used in this path:
> >
> > kernel_restart_prepare or kernel_shutdown_prepare
> > device_shutdown
> > dev->bus->shutdown
> > pci_device_shutdown # pci_bus_type.shutdown
> > drv->shutdown
> >
> > so we're going to either reboot or halt/power-off the entire system,
> > and we're not going to use this device again until we're in a
> > brand-new kernel and we re-enumerate the device and re-register the
> > driver.
> >
> > I'm not quite sure how either of those fits into the power-saving
> > reason. I guess going to S5 is probably via the kernel_power_off()
> > path and that by itself doesn't turn off as much power to the PCIe
> > controller as it could? And this new .shutdown() method will get
> > called in that path and will turn off more power, but will still leave
> > enough for wake-on-LAN to work? And when we *do* wake from S5,
> > obviously that means a complete boot with a new kernel.
>
> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
> that we can in the PCIe root complex and its PHY, drop the PCIe link to
> the end-point which signals that the end-point can enter its own suspend
> logic, too. And yes, when we do wake-up from S5 it means booting a
> completely new kernel. S5 is typically implemented in our chips by
> keeping just a little bit of logic active to service wake-up events
> (infrared remotes, GPIOs, RTC, etc.).
Which part of that does this patch change? Is it that the new
.shutdown() turns off more power than machine_power_off() does by
itself?
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
2021-06-03 20:58 ` Bjorn Helgaas
(?)
@ 2021-06-03 21:01 ` Florian Fainelli
-1 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-06-03 21:01 UTC (permalink / raw)
To: Bjorn Helgaas, Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]
On 6/3/21 1:58 PM, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
>> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
>>> On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
>>>> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
>>>>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>>>>>> The shutdown() call is similar to the remove() call except the former does
>>>>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>>>>>> if it does.
>>>>>
>>>>> This doesn't explain why shutdown() is necessary. "errors occur"
>>>>> might be a hint, except that AFAICT, many similar drivers do invoke
>>>>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
>>>>> holding pci_lock_rescan_remove()), without implementing .shutdown().
>>>>
>>>> We have to implement .shutdown() in order to meet a certain power budget
>>>> while the chip is being put into S5 (soft off) state and still support
>>>> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
>>>> power savings at the wall. We could probably add a word or two in a v2
>>>> that indicates this is done for power savings.
>>>
>>> "Saving power" is a great reason to do this. But we still need to
>>> connect this to the driver model and the system-level behavior
>>> somehow.
>>>
>>> The pci_driver comment says @shutdown is to "stop idling DMA
>>> operations" and it hooks into reboot_notifier_list in kernel/sys.c.
>>> That's incorrect or at least incomplete because reboot_notifier_list
>>> isn't mentioned at all in kernel/sys.c, and I don't see the connection
>>> between @shutdown and reboot_notifier_list.
>>>
>>> AFAICT, @shutdown is currently used in this path:
>>>
>>> kernel_restart_prepare or kernel_shutdown_prepare
>>> device_shutdown
>>> dev->bus->shutdown
>>> pci_device_shutdown # pci_bus_type.shutdown
>>> drv->shutdown
>>>
>>> so we're going to either reboot or halt/power-off the entire system,
>>> and we're not going to use this device again until we're in a
>>> brand-new kernel and we re-enumerate the device and re-register the
>>> driver.
>>>
>>> I'm not quite sure how either of those fits into the power-saving
>>> reason. I guess going to S5 is probably via the kernel_power_off()
>>> path and that by itself doesn't turn off as much power to the PCIe
>>> controller as it could? And this new .shutdown() method will get
>>> called in that path and will turn off more power, but will still leave
>>> enough for wake-on-LAN to work? And when we *do* wake from S5,
>>> obviously that means a complete boot with a new kernel.
>>
>> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
>> that we can in the PCIe root complex and its PHY, drop the PCIe link to
>> the end-point which signals that the end-point can enter its own suspend
>> logic, too. And yes, when we do wake-up from S5 it means booting a
>> completely new kernel. S5 is typically implemented in our chips by
>> keeping just a little bit of logic active to service wake-up events
>> (infrared remotes, GPIOs, RTC, etc.).
>
> Which part of that does this patch change? Is it that the new
> .shutdown() turns off more power than machine_power_off() does by
> itself?
Yes, with pcie-brcmstb.c providing a .shutdow() callback we have a
chance to turn off our PCIe PHY and the RC's digital clock which would
not be able to do otherwise.
--
Florian
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-06-03 21:01 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-06-03 21:01 UTC (permalink / raw)
To: Bjorn Helgaas, Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1.1: Type: text/plain, Size: 3477 bytes --]
On 6/3/21 1:58 PM, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
>> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
>>> On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
>>>> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
>>>>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>>>>>> The shutdown() call is similar to the remove() call except the former does
>>>>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>>>>>> if it does.
>>>>>
>>>>> This doesn't explain why shutdown() is necessary. "errors occur"
>>>>> might be a hint, except that AFAICT, many similar drivers do invoke
>>>>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
>>>>> holding pci_lock_rescan_remove()), without implementing .shutdown().
>>>>
>>>> We have to implement .shutdown() in order to meet a certain power budget
>>>> while the chip is being put into S5 (soft off) state and still support
>>>> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
>>>> power savings at the wall. We could probably add a word or two in a v2
>>>> that indicates this is done for power savings.
>>>
>>> "Saving power" is a great reason to do this. But we still need to
>>> connect this to the driver model and the system-level behavior
>>> somehow.
>>>
>>> The pci_driver comment says @shutdown is to "stop idling DMA
>>> operations" and it hooks into reboot_notifier_list in kernel/sys.c.
>>> That's incorrect or at least incomplete because reboot_notifier_list
>>> isn't mentioned at all in kernel/sys.c, and I don't see the connection
>>> between @shutdown and reboot_notifier_list.
>>>
>>> AFAICT, @shutdown is currently used in this path:
>>>
>>> kernel_restart_prepare or kernel_shutdown_prepare
>>> device_shutdown
>>> dev->bus->shutdown
>>> pci_device_shutdown # pci_bus_type.shutdown
>>> drv->shutdown
>>>
>>> so we're going to either reboot or halt/power-off the entire system,
>>> and we're not going to use this device again until we're in a
>>> brand-new kernel and we re-enumerate the device and re-register the
>>> driver.
>>>
>>> I'm not quite sure how either of those fits into the power-saving
>>> reason. I guess going to S5 is probably via the kernel_power_off()
>>> path and that by itself doesn't turn off as much power to the PCIe
>>> controller as it could? And this new .shutdown() method will get
>>> called in that path and will turn off more power, but will still leave
>>> enough for wake-on-LAN to work? And when we *do* wake from S5,
>>> obviously that means a complete boot with a new kernel.
>>
>> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
>> that we can in the PCIe root complex and its PHY, drop the PCIe link to
>> the end-point which signals that the end-point can enter its own suspend
>> logic, too. And yes, when we do wake-up from S5 it means booting a
>> completely new kernel. S5 is typically implemented in our chips by
>> keeping just a little bit of logic active to service wake-up events
>> (infrared remotes, GPIOs, RTC, etc.).
>
> Which part of that does this patch change? Is it that the new
> .shutdown() turns off more power than machine_power_off() does by
> itself?
Yes, with pcie-brcmstb.c providing a .shutdow() callback we have a
chance to turn off our PCIe PHY and the RC's digital clock which would
not be able to do otherwise.
--
Florian
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
@ 2021-06-03 21:01 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-06-03 21:01 UTC (permalink / raw)
To: Bjorn Helgaas, Florian Fainelli
Cc: Jim Quinlan, linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Nicolas Saenz Julienne,
Lorenzo Pieralisi, Rob Herring,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list
[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]
On 6/3/21 1:58 PM, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
>> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
>>> On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
>>>> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
>>>>> On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
>>>>>> The shutdown() call is similar to the remove() call except the former does
>>>>>> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
>>>>>> if it does.
>>>>>
>>>>> This doesn't explain why shutdown() is necessary. "errors occur"
>>>>> might be a hint, except that AFAICT, many similar drivers do invoke
>>>>> pci_stop_root_bus() and pci_remove_root_bus() (several of them while
>>>>> holding pci_lock_rescan_remove()), without implementing .shutdown().
>>>>
>>>> We have to implement .shutdown() in order to meet a certain power budget
>>>> while the chip is being put into S5 (soft off) state and still support
>>>> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
>>>> power savings at the wall. We could probably add a word or two in a v2
>>>> that indicates this is done for power savings.
>>>
>>> "Saving power" is a great reason to do this. But we still need to
>>> connect this to the driver model and the system-level behavior
>>> somehow.
>>>
>>> The pci_driver comment says @shutdown is to "stop idling DMA
>>> operations" and it hooks into reboot_notifier_list in kernel/sys.c.
>>> That's incorrect or at least incomplete because reboot_notifier_list
>>> isn't mentioned at all in kernel/sys.c, and I don't see the connection
>>> between @shutdown and reboot_notifier_list.
>>>
>>> AFAICT, @shutdown is currently used in this path:
>>>
>>> kernel_restart_prepare or kernel_shutdown_prepare
>>> device_shutdown
>>> dev->bus->shutdown
>>> pci_device_shutdown # pci_bus_type.shutdown
>>> drv->shutdown
>>>
>>> so we're going to either reboot or halt/power-off the entire system,
>>> and we're not going to use this device again until we're in a
>>> brand-new kernel and we re-enumerate the device and re-register the
>>> driver.
>>>
>>> I'm not quite sure how either of those fits into the power-saving
>>> reason. I guess going to S5 is probably via the kernel_power_off()
>>> path and that by itself doesn't turn off as much power to the PCIe
>>> controller as it could? And this new .shutdown() method will get
>>> called in that path and will turn off more power, but will still leave
>>> enough for wake-on-LAN to work? And when we *do* wake from S5,
>>> obviously that means a complete boot with a new kernel.
>>
>> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
>> that we can in the PCIe root complex and its PHY, drop the PCIe link to
>> the end-point which signals that the end-point can enter its own suspend
>> logic, too. And yes, when we do wake-up from S5 it means booting a
>> completely new kernel. S5 is typically implemented in our chips by
>> keeping just a little bit of logic active to service wake-up events
>> (infrared remotes, GPIOs, RTC, etc.).
>
> Which part of that does this patch change? Is it that the new
> .shutdown() turns off more power than machine_power_off() does by
> itself?
Yes, with pcie-brcmstb.c providing a .shutdow() callback we have a
chance to turn off our PCIe PHY and the RC's digital clock which would
not be able to do otherwise.
--
Florian
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
2021-04-27 17:51 ` Jim Quinlan
@ 2021-05-25 18:01 ` Florian Fainelli
-1 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-05-25 18:01 UTC (permalink / raw)
To: Jim Quinlan, linux-pci, Bjorn Helgaas, bcm-kernel-feedback-list,
james.quinlan, nsaenz
Cc: Florian Fainelli, Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Lorenzo Pieralisi, Rob Herring
On 4/27/21 10:51 AM, Jim Quinlan wrote:
> v1 -- These commits were part of a previous pullreq but have
> been split off because they are unrelated to said pullreq's
> other more complex commits.
>
> Jim Quinlan (4):
> PCI: brcmstb: Check return value of clk_prepare_enable()
> PCI: brcmstb: Give 7216 SOCs their own config type
> PCI: brcmstb: Add panic/die handler to RC driver
> PCI: brcmstb: add shutdown call to driver
Bjorn, Lorenzon, are you satisfied with these commits, any chance to get
get them reviewed and queued up?
Thanks!
--
Florian
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
@ 2021-05-25 18:01 ` Florian Fainelli
0 siblings, 0 replies; 43+ messages in thread
From: Florian Fainelli @ 2021-05-25 18:01 UTC (permalink / raw)
To: Jim Quinlan, linux-pci, Bjorn Helgaas, bcm-kernel-feedback-list,
james.quinlan, nsaenz
Cc: Florian Fainelli, Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Lorenzo Pieralisi, Rob Herring
On 4/27/21 10:51 AM, Jim Quinlan wrote:
> v1 -- These commits were part of a previous pullreq but have
> been split off because they are unrelated to said pullreq's
> other more complex commits.
>
> Jim Quinlan (4):
> PCI: brcmstb: Check return value of clk_prepare_enable()
> PCI: brcmstb: Give 7216 SOCs their own config type
> PCI: brcmstb: Add panic/die handler to RC driver
> PCI: brcmstb: add shutdown call to driver
Bjorn, Lorenzon, are you satisfied with these commits, any chance to get
get them reviewed and queued up?
Thanks!
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
2021-04-27 17:51 ` Jim Quinlan
@ 2021-06-03 16:32 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 43+ messages in thread
From: Lorenzo Pieralisi @ 2021-06-03 16:32 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> v1 -- These commits were part of a previous pullreq but have
> been split off because they are unrelated to said pullreq's
> other more complex commits.
Can I drop this series ?
https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
Thanks,
Lorenzo
> Jim Quinlan (4):
> PCI: brcmstb: Check return value of clk_prepare_enable()
> PCI: brcmstb: Give 7216 SOCs their own config type
> PCI: brcmstb: Add panic/die handler to RC driver
> PCI: brcmstb: add shutdown call to driver
>
> drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> 1 file changed, 143 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
@ 2021-06-03 16:32 ` Lorenzo Pieralisi
0 siblings, 0 replies; 43+ messages in thread
From: Lorenzo Pieralisi @ 2021-06-03 16:32 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Bjorn Helgaas, Nicolas Saenz Julienne,
bcm-kernel-feedback-list, james.quinlan, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> v1 -- These commits were part of a previous pullreq but have
> been split off because they are unrelated to said pullreq's
> other more complex commits.
Can I drop this series ?
https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
Thanks,
Lorenzo
> Jim Quinlan (4):
> PCI: brcmstb: Check return value of clk_prepare_enable()
> PCI: brcmstb: Give 7216 SOCs their own config type
> PCI: brcmstb: Add panic/die handler to RC driver
> PCI: brcmstb: add shutdown call to driver
>
> drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> 1 file changed, 143 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
2021-06-03 16:32 ` Lorenzo Pieralisi
@ 2021-06-03 17:31 ` Jim Quinlan
-1 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-06-03 17:31 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
On Thu, Jun 3, 2021 at 12:32 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> > v1 -- These commits were part of a previous pullreq but have
> > been split off because they are unrelated to said pullreq's
> > other more complex commits.
>
> Can I drop this series ?
>
> https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
I will be submitting the voltage regulator commits -- but not the
panic handler -- from the series above -- I just haven't had time to
get around it (sorry).
Regards,
Jim Quinlan
Broadcom STB
>
>
>
> Thanks,
> Lorenzo
>
> > Jim Quinlan (4):
> > PCI: brcmstb: Check return value of clk_prepare_enable()
> > PCI: brcmstb: Give 7216 SOCs their own config type
> > PCI: brcmstb: Add panic/die handler to RC driver
> > PCI: brcmstb: add shutdown call to driver
> >
> > drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> > 1 file changed, 143 insertions(+), 2 deletions(-)
> >
> > --
> > 2.17.1
> >
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
@ 2021-06-03 17:31 ` Jim Quinlan
0 siblings, 0 replies; 43+ messages in thread
From: Jim Quinlan @ 2021-06-03 17:31 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
[-- Attachment #1.1: Type: text/plain, Size: 1048 bytes --]
On Thu, Jun 3, 2021 at 12:32 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> > v1 -- These commits were part of a previous pullreq but have
> > been split off because they are unrelated to said pullreq's
> > other more complex commits.
>
> Can I drop this series ?
>
> https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
I will be submitting the voltage regulator commits -- but not the
panic handler -- from the series above -- I just haven't had time to
get around it (sorry).
Regards,
Jim Quinlan
Broadcom STB
>
>
>
> Thanks,
> Lorenzo
>
> > Jim Quinlan (4):
> > PCI: brcmstb: Check return value of clk_prepare_enable()
> > PCI: brcmstb: Give 7216 SOCs their own config type
> > PCI: brcmstb: Add panic/die handler to RC driver
> > PCI: brcmstb: add shutdown call to driver
> >
> > drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> > 1 file changed, 143 insertions(+), 2 deletions(-)
> >
> > --
> > 2.17.1
> >
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
2021-06-03 17:31 ` Jim Quinlan
@ 2021-06-04 9:22 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 43+ messages in thread
From: Lorenzo Pieralisi @ 2021-06-04 9:22 UTC (permalink / raw)
To: Jim Quinlan
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
On Thu, Jun 03, 2021 at 01:31:01PM -0400, Jim Quinlan wrote:
> On Thu, Jun 3, 2021 at 12:32 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> > > v1 -- These commits were part of a previous pullreq but have
> > > been split off because they are unrelated to said pullreq's
> > > other more complex commits.
> >
> > Can I drop this series ?
> >
> > https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
>
>
> I will be submitting the voltage regulator commits -- but not the
> panic handler -- from the series above -- I just haven't had time to
> get around it (sorry).
So the series above can be marked as superseded, that's what I
was asking.
Thanks,
Lorenzo
> Regards,
> Jim Quinlan
> Broadcom STB
> >
> >
> >
> > Thanks,
> > Lorenzo
> >
> > > Jim Quinlan (4):
> > > PCI: brcmstb: Check return value of clk_prepare_enable()
> > > PCI: brcmstb: Give 7216 SOCs their own config type
> > > PCI: brcmstb: Add panic/die handler to RC driver
> > > PCI: brcmstb: add shutdown call to driver
> > >
> > > drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> > > 1 file changed, 143 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func
@ 2021-06-04 9:22 ` Lorenzo Pieralisi
0 siblings, 0 replies; 43+ messages in thread
From: Lorenzo Pieralisi @ 2021-06-04 9:22 UTC (permalink / raw)
To: Jim Quinlan
Cc: Jim Quinlan,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
Bjorn Helgaas, Nicolas Saenz Julienne,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Florian Fainelli,
Jim Quinlan,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring
On Thu, Jun 03, 2021 at 01:31:01PM -0400, Jim Quinlan wrote:
> On Thu, Jun 3, 2021 at 12:32 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 01:51:35PM -0400, Jim Quinlan wrote:
> > > v1 -- These commits were part of a previous pullreq but have
> > > been split off because they are unrelated to said pullreq's
> > > other more complex commits.
> >
> > Can I drop this series ?
> >
> > https://patchwork.kernel.org/user/todo/linux-pci/?series=459871
>
>
> I will be submitting the voltage regulator commits -- but not the
> panic handler -- from the series above -- I just haven't had time to
> get around it (sorry).
So the series above can be marked as superseded, that's what I
was asking.
Thanks,
Lorenzo
> Regards,
> Jim Quinlan
> Broadcom STB
> >
> >
> >
> > Thanks,
> > Lorenzo
> >
> > > Jim Quinlan (4):
> > > PCI: brcmstb: Check return value of clk_prepare_enable()
> > > PCI: brcmstb: Give 7216 SOCs their own config type
> > > PCI: brcmstb: Add panic/die handler to RC driver
> > > PCI: brcmstb: add shutdown call to driver
> > >
> > > drivers/pci/controller/pcie-brcmstb.c | 145 +++++++++++++++++++++++++-
> > > 1 file changed, 143 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread