v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
If present, these regulators have the property names
"vpcie12v-supply" and "vpcie3v3-supply". The existence of these
regulators in the EP subnode are currently pending as a pullreq
in pci-bus.yaml at
https://github.com/devicetree-org/dt-schema/pull/54
(MarkB, RobH).
-- Check return of brcm_set_regulators() (Florian)
-- Specify one regulator string per line for easier update (Florian)
-- Author/Committer/Signoff email changed from that of V2 from
'[email protected]' to '[email protected]'.
v2 -- Use regulator bulk API rather than multiple calls (MarkB).
v1 -- Bindings are added for fixed regulators that may power the EP device.
-- The brcmstb RC driver is modified to control these regulators
during probe, suspend, and resume.
-- 7216 type SOCs have additional error reporting HW and a
panic handler is added to dump its info.
-- A missing return value check is added.
Jim Quinlan (6):
dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
PCI: brcmstb: Add control of EP voltage regulators
PCI: brcmstb: Do not turn off regulators if EP can wake up
PCI: brcmstb: Give 7216 SOCs their own config type
PCI: brcmstb: Add panic/die handler to RC driver
PCI: brcmstb: Check return value of clk_prepare_enable()
.../bindings/pci/brcm,stb-pcie.yaml | 6 +
drivers/pci/controller/pcie-brcmstb.c | 271 +++++++++++++++++-
2 files changed, 272 insertions(+), 5 deletions(-)
--
2.17.1
Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
1 file changed, 87 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..b76ec7d9af32 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
#include <linux/pci.h>
#include <linux/pci-ecam.h>
#include <linux/printk.h>
+#include <linux/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/sizes.h>
#include <linux/slab.h>
@@ -169,6 +170,7 @@
#define SSC_STATUS_SSC_MASK 0x400
#define SSC_STATUS_PLL_LOCK_MASK 0x800
#define PCIE_BRCM_MAX_MEMC 3
+#define PCIE_BRCM_MAX_EP_REGULATORS 4
#define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX])
#define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA])
@@ -295,8 +297,27 @@ struct brcm_pcie {
u32 hw_rev;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+ struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
+ unsigned int num_supplies;
};
+static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+ struct device *dev = pcie->dev;
+ int ret;
+
+ if (!pcie->num_supplies)
+ return 0;
+ if (on)
+ ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+ else
+ ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
+ if (ret)
+ dev_err(dev, "failed to %s EP regulators\n",
+ on ? "enable" : "disable");
+ return ret;
+}
+
/*
* This is to convert the size of the inbound "BAR" region to the
* non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
pcie->bridge_sw_init_set(pcie, 1);
}
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
+{
+ struct device_node *child, *parent = pcie->np;
+ const unsigned int max_name_len = 64 + 4;
+ struct property *pp;
+
+ /* Look for regulator supply property in the EP device subnodes */
+ for_each_available_child_of_node(parent, child) {
+ /*
+ * Do a santiy test to ensure that this is an EP node
+ * (e.g. node name: "pci-ep@0,0"). The slot number
+ * should always be 0 as our controller only has a single
+ * port.
+ */
+ const char *p = strstr(child->full_name, "@0");
+
+ if (!p || (p[2] && p[2] != ','))
+ continue;
+
+ /* Now look for regulator supply properties */
+ for_each_property_of_node(child, pp) {
+ int i, n = strnlen(pp->name, max_name_len);
+
+ if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
+ continue;
+
+ /* Make sure this is not a duplicate */
+ for (i = 0; i < pcie->num_supplies; i++)
+ if (strncmp(pcie->supplies[i].supply,
+ pp->name, max_name_len) == 0)
+ continue;
+
+ if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+ pcie->supplies[pcie->num_supplies++].supply = pp->name;
+ else
+ dev_warn(pcie->dev, "No room for EP supply %s\n",
+ pp->name);
+ }
+ }
+ /*
+ * Get the regulators that the EP devices require. We cannot use
+ * pcie->dev as the device argument in regulator_bulk_get() since
+ * it will not find the regulators. Instead, use NULL and the
+ * regulators are looked up by their name.
+ */
+ return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
+}
+
static int brcm_pcie_suspend(struct device *dev)
{
struct brcm_pcie *pcie = dev_get_drvdata(dev);
- int ret;
brcm_pcie_turn_off(pcie);
- ret = brcm_phy_stop(pcie);
+ brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
- return ret;
+ return brcm_set_regulators(pcie, false);
}
static int brcm_pcie_resume(struct device *dev)
@@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
+ ret = brcm_set_regulators(pcie, true);
+ if (ret)
+ return ret;
+
ret = brcm_phy_start(pcie);
if (ret)
goto err;
@@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
+ brcm_set_regulators(pcie, false);
+ regulator_bulk_free(pcie->num_supplies, pcie->supplies);
}
static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
+ ret = brcm_pcie_get_regulators(pcie);
+ if (ret) {
+ dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+ goto fail;
+ }
+
+ ret = brcm_set_regulators(pcie, true);
+ if (ret)
+ goto fail;
+
ret = brcm_pcie_setup(pcie);
if (ret)
goto fail;
--
2.17.1
If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index b76ec7d9af32..89745bb6ada6 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
enum {
RGR1_SW_INIT_1,
@@ -299,22 +300,65 @@ struct brcm_pcie {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned int num_supplies;
+ bool ep_wakeup_capable;
};
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
{
+ bool *ret = data;
+
+ if (device_may_wakeup(&dev->dev)) {
+ *ret = true;
+ dev_dbg(&dev->dev, "disable cancelled for wake-up device\n");
+ }
+ return (int) *ret;
+}
+
+enum {
+ TURN_OFF, /* Turn egulators off, unless an EP is wakeup-capable */
+ TURN_OFF_ALWAYS, /* Turn Regulators off, no exceptions */
+ TURN_ON, /* Turn regulators on, unless pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret;
if (!pcie->num_supplies)
return 0;
- if (on)
+ if (how == TURN_ON) {
+ if (pcie->ep_wakeup_capable) {
+ /*
+ * We are resuming from a suspend. In the
+ * previous suspend we did not disable the power
+ * supplies, so there is no need to enable them
+ * (and falsely increase their usage count).
+ */
+ pcie->ep_wakeup_capable = false;
+ return 0;
+ }
+ } else if (how == TURN_OFF) {
+ /*
+ * If at least one device on this bus is enabled as a
+ * wake-up source, do not turn off regulators.
+ */
+ pcie->ep_wakeup_capable = false;
+ if (bridge->bus && brcm_pcie_link_up(pcie)) {
+ pci_walk_bus(bridge->bus, pci_dev_may_wakeup, &pcie->ep_wakeup_capable);
+ if (pcie->ep_wakeup_capable)
+ return 0;
+ }
+ }
+
+ if (how == TURN_ON)
ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
else
ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
if (ret)
dev_err(dev, "failed to %s EP regulators\n",
- on ? "enable" : "disable");
+ how == TURN_ON ? "enable" : "disable");
return ret;
}
@@ -1218,7 +1262,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
- return brcm_set_regulators(pcie, false);
+ return brcm_set_regulators(pcie, TURN_OFF);
}
static int brcm_pcie_resume(struct device *dev)
@@ -1231,7 +1275,7 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
- ret = brcm_set_regulators(pcie, true);
+ ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
return ret;
@@ -1271,7 +1315,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
- brcm_set_regulators(pcie, false);
+ brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
regulator_bulk_free(pcie->num_supplies, pcie->supplies);
}
@@ -1369,7 +1413,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
goto fail;
}
- ret = brcm_set_regulators(pcie, true);
+ ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
goto fail;
--
2.17.1
This distinction is required for an imminent commit.
Signed-off-by: Jim Quinlan <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
---
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 89745bb6ada6..9c8b922a9c19 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -260,6 +260,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;
@@ -1336,7 +1343,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
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 <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
---
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 9c8b922a9c19..2d9288399014 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>
@@ -186,6 +188,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);
@@ -218,6 +253,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[] = {
@@ -265,6 +301,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 {
@@ -308,8 +345,87 @@ struct brcm_pcie {
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned int num_supplies;
bool ep_wakeup_capable;
+ bool has_err_report;
+ struct notifier_block die_notifier;
};
+/* Dump out PCIe errors on die or panic */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void *p)
+{
+ const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, die_notifier);
+ void __iomem *base = pcie->base;
+ int i, is_cfg_err, is_mem_err, lanes;
+ char *width_str, *direction_str, lanes_str[9];
+ u32 info;
+
+ if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+ return NOTIFY_DONE;
+ info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+ is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+ is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+ width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : "32bit";
+ direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK) ? "Write" : "Read";
+ lanes = FIELD_GET(PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK, info);
+ for (i = 0, lanes_str[8] = 0; i < 8; i++)
+ lanes_str[i] = (lanes & (1 << i)) ? '1' : '0';
+
+ if (is_cfg_err) {
+ u32 cfg_addr = readl(base + PCIE_OUTB_ERR_ACC_ADDR);
+ u32 cause = readl(base + PCIE_OUTB_ERR_CFG_CAUSE);
+ int bus = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK, cfg_addr);
+ int dev = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK, cfg_addr);
+ int func = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK, cfg_addr);
+ int reg = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_REG_MASK, cfg_addr);
+
+ dev_err(pcie->dev, "Error: CFG Acc, %s, %s, Bus=%d, Dev=%d, Fun=%d, Reg=0x%x, lanes=%s\n",
+ width_str, direction_str, bus, dev, func, reg, lanes_str);
+ dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccTO=%d AccDsbld=%d Acc64bit=%d\n",
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK),
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK),
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK),
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK),
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK),
+ !!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK));
+ }
+
+ if (is_mem_err) {
+ u32 cause = readl(base + PCIE_OUTB_ERR_MEM_CAUSE);
+ u32 lo = readl(base + PCIE_OUTB_ERR_MEM_ADDR_LO);
+ u32 hi = readl(base + PCIE_OUTB_ERR_MEM_ADDR_HI);
+ u64 addr = ((u64)hi << 32) | (u64)lo;
+
+ dev_err(pcie->dev, "Error: Mem Acc, %s, %s, @0x%llx, lanes=%s\n",
+ width_str, direction_str, addr, lanes_str);
+ dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccDsble=%d BadAddr=%d\n",
+ !!(cause & PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK),
+ !!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK),
+ !!(cause & PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK),
+ !!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK),
+ !!(cause & PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK));
+ }
+
+ /* Clear the error */
+ writel(1, base + PCIE_OUTB_ERR_CLEAR);
+
+ return NOTIFY_DONE;
+}
+
+static void brcm_register_die_notifiers(struct brcm_pcie *pcie)
+{
+ pcie->die_notifier.notifier_call = dump_pcie_error;
+ register_die_notifier(&pcie->die_notifier);
+ atomic_notifier_chain_register(&panic_notifier_list, &pcie->die_notifier);
+}
+
+static void brcm_unregister_die_notifiers(struct brcm_pcie *pcie)
+{
+ unregister_die_notifier(&pcie->die_notifier);
+ atomic_notifier_chain_unregister(&panic_notifier_list, &pcie->die_notifier);
+ pcie->die_notifier.notifier_call = NULL;
+}
+
static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
{
bool *ret = data;
@@ -1332,6 +1448,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);
@@ -1371,6 +1489,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;
@@ -1448,6 +1567,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
The check was missing on PCIe resume.
Signed-off-by: Jim Quinlan <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2d9288399014..f6d9d785b301 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1396,7 +1396,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_set_regulators(pcie, TURN_ON);
if (ret)
@@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
ret = brcm_pcie_get_regulators(pcie);
if (ret) {
- dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+ pcie->num_supplies = 0;
+ if (ret != -EPROBE_DEFER)
+ dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
goto fail;
}
--
2.17.1
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> Control of EP regulators by the RC is needed because of the chicken-and-egg
Can you expand "EP"? Not sure if this refers to "endpoint" or
something else.
If this refers to a device in a slot, I guess it isn't necessarily a
PCIe *endpoint*; it could also be a switch upstream port.
> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
> 1 file changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index e330e6811f0b..b76ec7d9af32 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
> #include <linux/pci.h>
> #include <linux/pci-ecam.h>
> #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> #include <linux/sizes.h>
> #include <linux/slab.h>
> @@ -169,6 +170,7 @@
> #define SSC_STATUS_SSC_MASK 0x400
> #define SSC_STATUS_PLL_LOCK_MASK 0x800
> #define PCIE_BRCM_MAX_MEMC 3
> +#define PCIE_BRCM_MAX_EP_REGULATORS 4
>
> #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX])
> #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA])
> @@ -295,8 +297,27 @@ struct brcm_pcie {
> u32 hw_rev;
> void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> + struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> + unsigned int num_supplies;
> };
>
> +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{
> + struct device *dev = pcie->dev;
> + int ret;
> +
> + if (!pcie->num_supplies)
> + return 0;
> + if (on)
> + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> + else
> + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> + if (ret)
> + dev_err(dev, "failed to %s EP regulators\n",
> + on ? "enable" : "disable");
> + return ret;
> +}
> +
> /*
> * This is to convert the size of the inbound "BAR" region to the
> * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> pcie->bridge_sw_init_set(pcie, 1);
> }
>
> +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> +{
> + struct device_node *child, *parent = pcie->np;
> + const unsigned int max_name_len = 64 + 4;
> + struct property *pp;
> +
> + /* Look for regulator supply property in the EP device subnodes */
> + for_each_available_child_of_node(parent, child) {
> + /*
> + * Do a santiy test to ensure that this is an EP node
s/santiy/sanity/
> + * (e.g. node name: "pci-ep@0,0"). The slot number
> + * should always be 0 as our controller only has a single
> + * port.
> + */
> + const char *p = strstr(child->full_name, "@0");
> +
> + if (!p || (p[2] && p[2] != ','))
> + continue;
> +
> + /* Now look for regulator supply properties */
> + for_each_property_of_node(child, pp) {
> + int i, n = strnlen(pp->name, max_name_len);
> +
> + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> + continue;
> +
> + /* Make sure this is not a duplicate */
> + for (i = 0; i < pcie->num_supplies; i++)
> + if (strncmp(pcie->supplies[i].supply,
> + pp->name, max_name_len) == 0)
> + continue;
> +
> + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> + pcie->supplies[pcie->num_supplies++].supply = pp->name;
> + else
> + dev_warn(pcie->dev, "No room for EP supply %s\n",
> + pp->name);
> + }
> + }
> + /*
> + * Get the regulators that the EP devices require. We cannot use
> + * pcie->dev as the device argument in regulator_bulk_get() since
> + * it will not find the regulators. Instead, use NULL and the
> + * regulators are looked up by their name.
The comment doesn't explain the interesting part of why you need NULL
instead of "pcie->dev". I assume it has something to do with the
platform topology and its DT description.
This appears to be the only instance in the whole kernel of a use of
regulator_bulk_get() or devm_regulator_bulk_get() with NULL. That
definitely warrants a comment, so I'm glad you've got something here.
The regulator_bulk_get() function comment doesn't mention the
possibility of "dev == NULL", although regulator_dev_lookup(),
create_regulator(), device_link_add() do check for it being NULL, so I
guess it's not a surprise. We may call dev_err(NULL), which I think
will *work* without crashing even though it will look like a mistake
on the output.
> + */
> + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
devm_regulator_bulk_get()?
> +}
> +
> static int brcm_pcie_suspend(struct device *dev)
> {
> struct brcm_pcie *pcie = dev_get_drvdata(dev);
> - int ret;
>
> brcm_pcie_turn_off(pcie);
> - ret = brcm_phy_stop(pcie);
> + brcm_phy_stop(pcie);
If we no longer care whether brcm_phy_stop() returns an error, nobody
looks at the return value and it could be void.
> clk_disable_unprepare(pcie->clk);
>
> - return ret;
> + return brcm_set_regulators(pcie, false);
> }
>
> static int brcm_pcie_resume(struct device *dev)
> @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
> base = pcie->base;
> clk_prepare_enable(pcie->clk);
>
> + ret = brcm_set_regulators(pcie, true);
> + if (ret)
> + return ret;
> +
> ret = brcm_phy_start(pcie);
> if (ret)
> goto err;
> @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
> brcm_phy_stop(pcie);
> reset_control_assert(pcie->rescal);
> clk_disable_unprepare(pcie->clk);
> + brcm_set_regulators(pcie, false);
> + regulator_bulk_free(pcie->num_supplies, pcie->supplies);
> }
>
> static int brcm_pcie_remove(struct platform_device *pdev)
> @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ret = brcm_pcie_get_regulators(pcie);
> + if (ret) {
> + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> + goto fail;
> + }
> +
> + ret = brcm_set_regulators(pcie, true);
> + if (ret)
> + goto fail;
> +
> ret = brcm_pcie_setup(pcie);
> if (ret)
> goto fail;
> --
> 2.17.1
>
On Fri, Mar 26, 2021 at 03:19:01PM -0400, Jim Quinlan wrote:
> If any downstream device may wake up during S2/S3 suspend, we do not want
> to turn off its power when suspending.
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 7 deletions(-)
> +enum {
> + TURN_OFF, /* Turn egulators off, unless an EP is wakeup-capable */
> + TURN_OFF_ALWAYS, /* Turn Regulators off, no exceptions */
> + TURN_ON, /* Turn regulators on, unless pcie->ep_wakeup_capable */
s/egulators/regulators/
s/Regulators/regulators/
On Fri, Mar 26, 2021 at 03:19:04PM -0400, Jim Quinlan wrote:
> The check was missing on PCIe resume.
"PCIe resume" isn't really a thing, per se. PCI/PCIe gives us device
power states (D0, D3hot, etc), and Linux power management builds
suspend/resume on top of those. Maybe:
Check for failure of clk_prepare_enable() on device resume.
> Signed-off-by: Jim Quinlan <[email protected]>
> Acked-by: Florian Fainelli <[email protected]>
> Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
> ---
> drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2d9288399014..f6d9d785b301 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1396,7 +1396,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;
This fix doesn't look like it depends on the EP regulator support.
Maybe it should be a preparatory patch before patch 1/6? It could
then easily be backported to kernels that contain 8195b7417018 but not
EP regulator support.
> ret = brcm_set_regulators(pcie, TURN_ON);
> if (ret)
> @@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>
> ret = brcm_pcie_get_regulators(pcie);
> if (ret) {
> - dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> + pcie->num_supplies = 0;
> + if (ret != -EPROBE_DEFER)
> + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
Looks like this hunk might belong somewhere else, e.g., in patch 2/6?
The "Fixes:" line suggests that this patch could/should be backported to
every kernel that contains 8195b7417018, but 8195b7417018 doesn't have
pcie->num_supplies.
> goto fail;
> }
>
> --
> 2.17.1
>
On Fri, Mar 26, 2021 at 4:11 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> > Control of EP regulators by the RC is needed because of the chicken-and-egg
>
> Can you expand "EP"? Not sure if this refers to "endpoint" or
> something else.
Yes I meant "endpoint" -- I will expand it.
>
> If this refers to a device in a slot, I guess it isn't necessarily aWe only support this feature for endpoint devices; it they hav
> PCIe *endpoint*; it could also be a switch upstream port.
True; to be precise I mean the device directly connected to the single RC port.
>
> > situation: although the regulator is "owned" by the EP and would be best
> > handled on its driver, the EP cannot be discovered and probed unless its
> > regulator is already turned on.
> >
> > Signed-off-by: Jim Quinlan <[email protected]>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
> > 1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index e330e6811f0b..b76ec7d9af32 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -24,6 +24,7 @@
> > #include <linux/pci.h>
> > #include <linux/pci-ecam.h>
> > #include <linux/printk.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/reset.h>
> > #include <linux/sizes.h>
> > #include <linux/slab.h>
> > @@ -169,6 +170,7 @@
> > #define SSC_STATUS_SSC_MASK 0x400
> > #define SSC_STATUS_PLL_LOCK_MASK 0x800
> > #define PCIE_BRCM_MAX_MEMC 3
> > +#define PCIE_BRCM_MAX_EP_REGULATORS 4
> >
> > #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX])
> > #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA])
> > @@ -295,8 +297,27 @@ struct brcm_pcie {
> > u32 hw_rev;
> > void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > + struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> > + unsigned int num_supplies;
> > };
> >
> > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
> > + struct device *dev = pcie->dev;
> > + int ret;
> > +
> > + if (!pcie->num_supplies)
> > + return 0;
> > + if (on)
> > + ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> > + else
> > + ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> > + if (ret)
> > + dev_err(dev, "failed to %s EP regulators\n",
> > + on ? "enable" : "disable");
> > + return ret;
> > +}
> > +
> > /*
> > * This is to convert the size of the inbound "BAR" region to the
> > * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> > @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > pcie->bridge_sw_init_set(pcie, 1);
> > }
> >
> > +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> > +{
> > + struct device_node *child, *parent = pcie->np;
> > + const unsigned int max_name_len = 64 + 4;
> > + struct property *pp;
> > +
> > + /* Look for regulator supply property in the EP device subnodes */
> > + for_each_available_child_of_node(parent, child) {
> > + /*
> > + * Do a santiy test to ensure that this is an EP node
>
> s/santiy/sanity/
>
> > + * (e.g. node name: "pci-ep@0,0"). The slot number
> > + * should always be 0 as our controller only has a single
> > + * port.
> > + */
> > + const char *p = strstr(child->full_name, "@0");
> > +
> > + if (!p || (p[2] && p[2] != ','))
> > + continue;
> > +
> > + /* Now look for regulator supply properties */
> > + for_each_property_of_node(child, pp) {
> > + int i, n = strnlen(pp->name, max_name_len);
> > +
> > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > + continue;
> > +
> > + /* Make sure this is not a duplicate */
> > + for (i = 0; i < pcie->num_supplies; i++)
> > + if (strncmp(pcie->supplies[i].supply,
> > + pp->name, max_name_len) == 0)
> > + continue;
> > +
> > + if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> > + pcie->supplies[pcie->num_supplies++].supply = pp->name;
> > + else
> > + dev_warn(pcie->dev, "No room for EP supply %s\n",
> > + pp->name);
> > + }
> > + }
> > + /*
> > + * Get the regulators that the EP devices require. We cannot use
> > + * pcie->dev as the device argument in regulator_bulk_get() since
> > + * it will not find the regulators. Instead, use NULL and the
> > + * regulators are looked up by their name.
>
> The comment doesn't explain the interesting part of why you need NULL
> instead of "pcie->dev". I assume it has something to do with the
> platform topology and its DT description.
>
> This appears to be the only instance in the whole kernel of a use of
> regulator_bulk_get() or devm_regulator_bulk_get() with NULL. That
> definitely warrants a comment, so I'm glad you've got something here.
>
> The regulator_bulk_get() function comment doesn't mention the
> possibility of "dev == NULL", although regulator_dev_lookup(),
> create_regulator(), device_link_add() do check for it being NULL, so I
> guess it's not a surprise. We may call dev_err(NULL), which I thinkWe only support this feature for endpoint devices; it they hav
> will *work* without crashing even though it will look like a mistake
> on the output.
Folks wanted me to put the "supply" in the endpoint subnode. After
looking at the regulator code I assumed that using the pcie->dev in
this call would not work as the supply property is not in its DT node.
Turns out it works fine; I will fix it.
>
> > + */
> > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> devm_regulator_bulk_get()?
Yep.
>
> > +}
> > +
> > static int brcm_pcie_suspend(struct device *dev)
> > {
> > struct brcm_pcie *pcie = dev_get_drvdata(dev);
> > - int ret;
> >
> > brcm_pcie_turn_off(pcie);
> > - ret = brcm_phy_stop(pcie);
> > + brcm_phy_stop(pcie);We only support this feature for endpoint devices; it they hav
>
> If we no longer care whether brcm_phy_stop() returns an error, nobody
> looks at the return value and it could be void.
Will fix.
Thanks,
Jim Quinlan
Broadcom STB
>
> > clk_disable_unprepare(pcie->clk);
> >
> > - return ret;
> > + return brcm_set_regulators(pcie, false);
> > }
> >
> > static int brcm_pcie_resume(struct device *dev)
> > @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
> > base = pcie->base;
> > clk_prepare_enable(pcie->clk);
> >
> > + ret = brcm_set_regulators(pcie, true);
> > + if (ret)
> > + return ret;
> > +
> > ret = brcm_phy_start(pcie);
> > if (ret)
> > goto err;
> > @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
> > brcm_phy_stop(pcie);
> > reset_control_assert(pcie->rescal);
> > clk_disable_unprepare(pcie->clk);
> > + brcm_set_regulators(pcie, false);
> > + regulator_bulk_free(pcie->num_supplies, pcie->supplies);
> > }
> >
> > static int brcm_pcie_remove(struct platform_device *pdev)
> > @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + ret = brcm_pcie_get_regulators(pcie);
> > + if (ret) {
> > + dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> > + goto fail;
> > + }
> > +
> > + ret = brcm_set_regulators(pcie, true);
> > + if (ret)
> > + goto fail;
> > +
> > ret = brcm_pcie_setup(pcie);
> > if (ret)
> > goto fail;
> > --
> > 2.17.1
> >
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> Control of EP regulators by the RC is needed because of the chicken-and-egg
> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.
Ideally the driver core would have a way for buses to register devices
pre physical enumeration and give drivers a callback that could be used
to do whatever is needed to trigger enumeration, letting the bus then
match newly physically enumerated devices with the software enumerated
struct devices. Soundwire has something in this area for a bus level
solution.
On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> + /* Now look for regulator supply properties */
> + for_each_property_of_node(child, pp) {
> + int i, n = strnlen(pp->name, max_name_len);
> +
> + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> + continue;
Here you are figuring out a device local supply name...
> + /*
> + * Get the regulators that the EP devices require. We cannot use
> + * pcie->dev as the device argument in regulator_bulk_get() since
> + * it will not find the regulators. Instead, use NULL and the
> + * regulators are looked up by their name.
> + */
> + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
...and here you are trying to look up that device local name in the
global namespace. That's not going to work well, the global names that
supplies are labelled with may be completely different to what the chip
designer called them and there could easily be naming collisions between
different chips.
On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <[email protected]>
w./lib/python3.6/site-packages/dtschema/schemasrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
>
> > + /* Now look for regulator supply properties */
> > + for_each_property_of_node(child, pp) {
> > + int i, n = strnlen(pp->name, max_name_len);
> > +
> > + if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > + continue;
>
> Here you are figuring out a device local supply name...
>
> > + /*
> > + * Get the regulators that the EP devianswerces require. We cannot use
> > + * pcie->dev as the device argument in regulator_bulk_get() since
> > + * it will not find the regulators. Instead, use NULL and the
> > + * regulators are looked up by their name.
> > + */
> > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> ...and here you are trying to look up that device local name in the
> global namespace. That's not going to work well, the global names that
> supplies are labelled with may be completely different to what the chip
> designer called them and there could easily be naming collisions between
> different chips.
Hello Mark,
I am re-submitting this pullreq using
"devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
NULL for the device and if so does this fix it? If not, what do you
suggest that I do?
Thanks,
Jim
On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <[email protected]>
> > Here you are figuring out a device local supply name...
> > > + /*
> > > + * Get the regulators that the EP devianswerces require. We cannot use
> > > + * pcie->dev as the device argument in regulator_bulk_get() since
> > > + * it will not find the regulators. Instead, use NULL and the
> > > + * regulators are looked up by their name.
> > > + */
> > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
> > ...and here you are trying to look up that device local name in the
> > global namespace. That's not going to work well, the global names that
> > supplies are labelled with may be completely different to what the chip
> > designer called them and there could easily be naming collisions between
> > different chips.
> "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> NULL for the device and if so does this fix it? If not, what do you
> suggest that I do?
If you use the struct device for the PCIe controller then that's going
to first try the PCIe controller then the global namespace so you still
have all the same problems. You really need to use the struct device
for the device that is being supplied not some random other struct
device you happened to find in the system.
As I said in my earlier reply I think either the driver core or PCI
needs something like Soundwire has which lets it create struct devices
for things that have been enumerated via software but not enumerated by
hardware and a callback or something which lets those devices take
whatever steps are needed to trigger probe.
/* Pmap_idx to avs pmap number */
const uint8_t pmap_idx_to_avs_id[20];
On Mon, Mar 29, 2021 at 1:16 PM Mark Brown <[email protected]> wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <[email protected]>
>
> > > Here you are figuring out a device local supply name...
>
> > > > + /*
> > > > + * Get the regulators that the EP devianswerces require. We cannot use
> > > > + * pcie->dev as the device argument in regulator_bulk_get() since
> > > > + * it will not find the regulators. Instead, use NULL and the
> > > > + * regulators are looked up by their name.
> > > > + */
> > > > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace. That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it? If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems. You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either. First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing. For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible. Our DT-generating
scripts also flag namespace issues. I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.
Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and
"vpcie3v3-supply".
Is the above enough to alleviate your concerns about global namespace collision?
>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.
Can you please elaborate this further and in detail? This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it. Do you really think
it is necessary to construct such a subsystem just to avoid the very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?
Regards,
Jim Quinlan
Broadcom STB
On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:
> I'm not concerned about a namespace collision and I don't think you
> should be concerned either. First, this driver is for Broadcom STB
> PCIe chips and boards, and we also deliver the DT to the customers.
> We typically do not have any other regulators in the DT besides the
> ones I am proposing. For example, the 7216 SOC DT has 0 other
You may not describe these regulators in the DT but you must have other
regulators in your system, most devices need power to operate. In any
case "this works for me with my DT on my system and nobody will ever
change our reference design" isn't really a great approach, frankly it's
not a claim I entirely believe and even if it turns out to be true for
your systems if we establish this as being how regulators work for
soldered down PCI devices everyone else is going to want to do the same
thing, most likely making the same claims for how much control they have
over the systems things will run on.
> regulators -- no namespace collision possible. Our DT-generating
> scripts also flag namespace issues. I admit that this driver is also
> used by RPi chips, but I can easily exclude this feature from the RPI
> if Nicolas has any objection.
That's certainly an issue, obviously the RPI is the sort of system where
I'd imagine this would be particularly useful.
> Further, if you want, I can restrict the search to the two regulators
> I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and
> "vpcie3v3-supply".
No, that doesn't help - what happens if someone uses separate regulators
for different PCI devices? The reason the supplies are device namespaced
is that each device can look up it's own supplies and label them how it
wants without reference to anything else on the board. Alternatively
what happens if some device has another supply it needs to power on (eg,
something that wants a clean LDO output for analogue use)?
> Is the above enough to alleviate your concerns about global namespace collision?
Not really. TBH it looks like this driver is keeping the regulators
enabled all the time except for suspend and resume anyway, if that's all
that's going on I'd have thought that you wouldn't need any explicit
management in the driver anyway? Just mark the regualtors as always on
and set up an appropriate suspend mode configuration and everything
should work without the drivers doing anything. Unless your PMIC isn't
able to provide separate suspend mode configuration for the regulators?
On 3/29/21 1:45 PM, Mark Brown wrote:
> On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:
>
>> I'm not concerned about a namespace collision and I don't think you
>> should be concerned either. First, this driver is for Broadcom STB
>> PCIe chips and boards, and we also deliver the DT to the customers.
>> We typically do not have any other regulators in the DT besides the
>> ones I am proposing. For example, the 7216 SOC DT has 0 other
>
> You may not describe these regulators in the DT but you must have other
> regulators in your system, most devices need power to operate. In any
> case "this works for me with my DT on my system and nobody will ever
> change our reference design" isn't really a great approach, frankly it's
> not a claim I entirely believe and even if it turns out to be true for
> your systems if we establish this as being how regulators work for
> soldered down PCI devices everyone else is going to want to do the same
> thing, most likely making the same claims for how much control they have
> over the systems things will run on.
>
>> regulators -- no namespace collision possible. Our DT-generating
>> scripts also flag namespace issues. I admit that this driver is also
>> used by RPi chips, but I can easily exclude this feature from the RPI
>> if Nicolas has any objection.
>
> That's certainly an issue, obviously the RPI is the sort of system where
> I'd imagine this would be particularly useful.
>
>> Further, if you want, I can restrict the search to the two regulators
>> I am proposing to add to pci-bus.yaml: "vpcie12v-supply" and
>> "vpcie3v3-supply".
>
> No, that doesn't help - what happens if someone uses separate regulators
> for different PCI devices? The reason the supplies are device namespaced
> is that each device can look up it's own supplies and label them how it
> wants without reference to anything else on the board. Alternatively
> what happens if some device has another supply it needs to power on (eg,
> something that wants a clean LDO output for analogue use)?
>
>> Is the above enough to alleviate your concerns about global namespace collision?
>
> Not really. TBH it looks like this driver is keeping the regulators
> enabled all the time except for suspend and resume anyway, if that's all
> that's going on I'd have thought that you wouldn't need any explicit
> management in the driver anyway? Just mark the regualtors as always on
> and set up an appropriate suspend mode configuration and everything
> should work without the drivers doing anything. Unless your PMIC isn't
> able to provide separate suspend mode configuration for the regulators?
>
We have typically GPIO-controlled and PMIC (via SCMI) controlled
regulators. During PCIe enumeration we need these regulators turned on
to be successful in training the PCIe link and discover the end-point
attached, so there an always on regulator would work.
When we enter a system suspend state however there are really two cases:
- the end-point supports Wake-on (typically wake-on-WLAN) and we need
its power supplied kept on to support that
- the end-point does not support or participate in any wake-up, there we
want to turn its supplies off to save power
How would we go about supporting such an use case with an always on
regulator?
--
Florian
On Mon, Mar 29, 2021 at 02:09:58PM -0700, Florian Fainelli wrote:
> On 3/29/21 1:45 PM, Mark Brown wrote:
> > management in the driver anyway? Just mark the regualtors as always on
> > and set up an appropriate suspend mode configuration and everything
> > should work without the drivers doing anything. Unless your PMIC isn't
> > able to provide separate suspend mode configuration for the regulators?
> We have typically GPIO-controlled and PMIC (via SCMI) controlled
> regulators. During PCIe enumeration we need these regulators turned on
> to be successful in training the PCIe link and discover the end-point
> attached, so there an always on regulator would work.
> When we enter a system suspend state however there are really two cases:
> - the end-point supports Wake-on (typically wake-on-WLAN) and we need
> its power supplied kept on to support that
> - the end-point does not support or participate in any wake-up, there we
> want to turn its supplies off to save power
> How would we go about supporting such an use case with an always on
> regulator?
With a PMIC most PMICs have a system suspend mode with separate
regulator configuration for that and there's seprate regulator API
control for those, including DT bindings. If that needs runtime
configuration for something hidden by SCMI I'd hope the SCMI regulator
stuff has facilities for that, if not then I guess a spec extension is
needed. If you want to dynamically select if something is on during
suspend there's not really a way around regulator API support.
For a GPIO regulator you probably need something that does a disable on
the way down, assuming that the GPIO/pin controller doesn't end up
having it's own suspend mode control that ends up powering things off
anyway. With GPIOs pinctrl on the pins rather than exposing as a
regulator might be enough.