v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
like to use this pullreq as a basis for future discussion.]
[Commit: Add bindings for ...]
-- Fix syntax error in YAML bindings example (RobH)
-- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
(I believe RobH said this was okay)
[Commit: Add control of ..]
-- Do not do global search for regulator; now we look specifically
for the property {vpcie12v,vpcie3v3}-supply in the root complex
DT node and then call devm_regulator_bulk_get() (MarkB)
-- Use devm_regulator_bulk_get() (Bjorn)
-- s/EP/slot0 device/ (Bjorn)
-- Spelling, capitalization (Bjorn)
-- Have brcm_phy_stop() return a void (Bjorn)
[Commit: Do not turn off ...]
-- Capitalization (Bjorn)
[Commit: Check return value ...]
-- Commit message content (Bjorn)
-- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
-- Move the rest of 6/6 before all other commits (Bjorn)
v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
If present, these regulators have the property names
"vpcie12v-supply" and "vpcie3v3-supply". The existence of these
regulators in the EP subnode are currently pending as a pullreq
in pci-bus.yaml at
https://github.com/devicetree-org/dt-schema/pull/54
(MarkB, RobH).
-- Check return of brcm_set_regulators() (Florian)
-- Specify one regulator string per line for easier update (Florian)
-- Author/Committer/Signoff email changed from that of V2 from
'[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):
PCI: brcmstb: Check return value of clk_prepare_enable()
dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage
regulators
PCI: brcmstb: Add control of slot0 device 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
.../bindings/pci/brcm,stb-pcie.yaml | 4 +
drivers/pci/controller/pcie-brcmstb.c | 262 +++++++++++++++++-
2 files changed, 259 insertions(+), 7 deletions(-)
--
2.17.1
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 | 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
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 4c79aff66de7..44128df33785 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,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;
@@ -1325,7 +1332,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 44128df33785..73a12c62b94e 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);
@@ -223,6 +258,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[] = {
@@ -270,6 +306,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 {
@@ -313,8 +350,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;
@@ -1321,6 +1437,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);
@@ -1360,6 +1478,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;
@@ -1439,6 +1558,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
Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
allows optional regulators to be attached and controlled by the PCIe RC
driver. That being said, this driver searches in the DT subnode (the EP
node, eg pci@0,0) for the regulator property.
The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at
https://github.com/devicetree-org/dt-schema/pull/54
Signed-off-by: Jim Quinlan <[email protected]>
---
Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..f2caa5b3b281 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,9 @@ properties:
aspm-no-l0s: true
+ vpcie12v-supply: true
+ vpcie3v3-supply: true
+
brcm,scb-sizes:
description: u64 giving the 64bit PCIe memory
viewport size of a memory controller. There may be up to
@@ -156,5 +159,6 @@ examples:
<0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
brcm,enable-ssc;
brcm,scb-sizes = <0x0000000080000000 0x0000000080000000>;
+ vpcie12v-supply = <&vreg12>;
};
};
--
2.17.1
This Broadcom STB has one port and connects directly to one device, be it a
switch or an endpoint. We want to be able to turn on/off any regulators
for that device. Control of regulators is needed because of the
chicken-and-egg situation: although the regulator is "owned" by the device
and would be best handled by its driver, the device cannot be discovered
and probed unless its regulator is already turned on.
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 83 +++++++++++++++++++++++++--
1 file changed, 78 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4ce1f3a60574..1b0de0c7da60 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])
@@ -192,6 +194,11 @@ 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 const char * const supplies[] = {
+ "vpcie12v-supply",
+ "vpcie3v3-supply",
+};
+
enum {
RGR1_SW_INIT_1,
EXT_CFG_INDEX,
@@ -295,8 +302,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
@@ -1112,9 +1138,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
}
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
{
- return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+ if (pcie->rescal)
+ brcm_phy_cntl(pcie, 0);
}
static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1141,16 +1168,45 @@ 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 *np = pcie->np;
+ struct property *pp;
+ const unsigned int ns = ARRAY_SIZE(supplies);
+ unsigned int i;
+ int ret = 0;
+
+ /* Look for specific pcie regulators in the RC DT node. */
+ for_each_property_of_node(np, pp) {
+ for (i = 0; i < ns; i++)
+ if (strcmp(supplies[i], pp->name) == 0)
+ break;
+ if (i >= ns)
+ continue;
+
+ if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+ pcie->supplies[pcie->num_supplies++].supply
+ = supplies[i];
+ else
+ dev_warn(pcie->dev, "No room for supply %s\n",
+ supplies[i]);
+ }
+
+ if (pcie->num_supplies)
+ ret = devm_regulator_bulk_get(pcie->dev, pcie->num_supplies,
+ pcie->supplies);
+ return ret;
+}
+
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)
@@ -1165,6 +1221,10 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
return ret;
+ ret = brcm_set_regulators(pcie, true);
+ if (ret)
+ return ret;
+
ret = brcm_phy_start(pcie);
if (ret)
goto err;
@@ -1201,6 +1261,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
+ brcm_set_regulators(pcie, false);
}
static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1291,6 +1352,18 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
+ ret = brcm_pcie_get_regulators(pcie);
+ if (ret) {
+ pcie->num_supplies = 0;
+ if (ret != -EPROBE_DEFER)
+ 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 Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote:
> > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> > allows optional regulators to be attached and controlled by the PCIe RC
> > driver. That being said, this driver searches in the DT subnode (the EP
> > node, eg pci@0,0) for the regulator property.
>
> > The use of a regulator property in the pcie EP subnode such as
> > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> > file at
> >
> > https://github.com/devicetree-org/dt-schema/pull/54
> >
> > Signed-off-by: Jim Quinlan <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >
> > aspm-no-l0s: true
> >
> > + vpcie12v-supply: true
> > + vpcie3v3-supply: true
> > +
>
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.
Hi Mark,
Can you explain what you think that would look like in the DT?
Thanks,
Jim Quinlan
Broadcom STB
On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
> > No great problem with having these in the controller node (assming it
> > accurately describes the hardware) but I do think we ought to also be
> > able to describe these per slot.
> Can you explain what you think that would look like in the DT?
I *think* that's just some properties on the nodes for the endpoints,
note that the driver could just ignore them for now. Not sure where or
if we document any extensions but child nodes are in section 4 of the
v2.1 PCI bus binding.
On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
> + /* Look for specific pcie regulators in the RC DT node. */
> + for_each_property_of_node(np, pp) {
> + for (i = 0; i < ns; i++)
> + if (strcmp(supplies[i], pp->name) == 0)
This is broken, the driver knows which supplies are expected, the device
can't function without these supplies so the driver should just
unconditionally request them like any other supply.
On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <[email protected]> wrote:
> > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
> > This is broken, the driver knows which supplies are expected, the device
> > can't function without these supplies so the driver should just
> > unconditionally request them like any other supply.
> Some boards require the regulators, some do not. So the driver is
No, some boards have the supplies described in firmware and some do not.
> only sure what the names may be if they are present. If I put these
> names in my struct regulator_bulk_data array and do a
> devm_regulator_bulk_get(), I will get the following for the boards
> that do not need the regulators (e.g. the RPi SOC):
>
> [ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> using dummy regulator
> [ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> using dummy regulator
Sure, those are just warnings.
> IIRC you consider this a debug feature? Be that as it may, these
> lines will confuse our customers and I'd like that they not be printed
> if possible.
You can stop the warnings by updating your firmware to more completely
describe the system - ideally all the supplies in the system would be
described for future proofing. Or if this is a custom software stack
just delete whatever error checking and warnings you like. The warnings
are there in case we've not got something mapped properly (eg, if there
were a typo in a property name) and things stop working, it's not great
to just ignore errors.
> So I ask you to allow the code as is. If you still insist, I will
> change and resubmit.
Remove it, conditional code like this is just as bad in this driver as
it is in every other one.
On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote:
> Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> allows optional regulators to be attached and controlled by the PCIe RC
> driver. That being said, this driver searches in the DT subnode (the EP
> node, eg pci@0,0) for the regulator property.
> The use of a regulator property in the pcie EP subnode such as
> "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> file at
>
> https://github.com/devicetree-org/dt-schema/pull/54
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index f90557f6deb8..f2caa5b3b281 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,9 @@ properties:
>
> aspm-no-l0s: true
>
> + vpcie12v-supply: true
> + vpcie3v3-supply: true
> +
No great problem with having these in the controller node (assming it
accurately describes the hardware) but I do think we ought to also be
able to describe these per slot.
On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <[email protected]> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > + /* Look for specific pcie regulators in the RC DT node. */
> > + for_each_property_of_node(np, pp) {
> > + for (i = 0; i < ns; i++)
> > + if (strcmp(supplies[i], pp->name) == 0)
>
> This is broken, the driver knows which supplies are expected, the device
> can't function without these supplies so the driver should just
> unconditionally request them like any other supply.
Hi Mark,
Some boards require the regulators, some do not. So the driver is
only sure what the names may be if they are present. If I put these
names in my struct regulator_bulk_data array and do a
devm_regulator_bulk_get(), I will get the following for the boards
that do not need the regulators (e.g. the RPi SOC):
[ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
using dummy regulator
[ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
using dummy regulator
IIRC you consider this a debug feature? Be that as it may, these
lines will confuse our customers and I'd like that they not be printed
if possible.
So I ask you to allow the code as is. If you still insist, I will
change and resubmit.
Regards,
Jim Quinlan
Broadcom STB
On Tue, Apr 6, 2021 at 1:23 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <[email protected]> wrote:
> > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > > This is broken, the driver knows which supplies are expected, the device
> > > can't function without these supplies so the driver should just
> > > unconditionally request them like any other supply.
>
> > Some boards require the regulators, some do not. So the driver is
>
> No, some boards have the supplies described in firmware and some do not.
True.
>
> > only sure what the names may be if they are present. If I put these
> > names in my struct regulator_bulk_data array and do a
> > devm_regulator_bulk_get(), I will get the following for the boards
> > that do not need the regulators (e.g. the RPi SOC):
> >
> > [ 6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> > using dummy regulator
> > [ 6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> > using dummy regulator
>
> Sure, those are just warnings.
>
> > IIRC you consider this a debug feature? Be that as it may, these
> > lines will confuse our customers and I'd like that they not be printed
> > if possible.
>
> You can stop the warnings by updating your firmware to more completely
> describe the system - ideally all the supplies in the system would be
> described for future proofing. Or if this is a custom software stack
> just delete whatever error checking and warnings you like. The warnings
> are there in case we've not got something mapped properly (eg, if there
> were a typo in a property name) and things stop working, it's not great
> to just ignore errors.
A lot of this is really not under our control.
>
> > So I ask you to allow the code as is. If you still insist, I will
> > change and resubmit.
>
> Remove it, conditional code like this is just as bad in this driver as
> it is in every other one.
I will remove this and resubmit.
Thanks,
Jim Quinlan
Broadcom STB
On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
>
> > > No great problem with having these in the controller node (assming it
> > > accurately describes the hardware) but I do think we ought to also be
> > > able to describe these per slot.
>
> > Can you explain what you think that would look like in the DT?
>
> I *think* that's just some properties on the nodes for the endpoints,
> note that the driver could just ignore them for now. Not sure where or
> if we document any extensions but child nodes are in section 4 of the
> v2.1 PCI bus binding.
Hi Mark,
I'm a little confused -- here is how I remember the chronology of the
"DT bindings" commit reviews, please correct me if I'm wrong:
o JimQ submitted a pullreq for using voltage regulators in the same
style as the existing "rockport" PCIe driver.
o After some deliberation, RobH preferred that the voltage regulators
should go into the PCIe subnode device's DT node.
o JimQ put the voltage regulators in the subnode device's DT node.
o MarkB didn't like the fact that the code did a global search for the
regulator since it could not provide the owning struct device* handle.
o RobH relented, and said that if it is just two specific and standard
voltage regulators, perhaps they can go in the parent DT node after
all.
o JimQ put the regulators back in the PCIe node.
o MarkB now wants the regulators to go back into the child node again?
Folks, please advise.
Regards,
Jim Quinlan
Broadcom STB
On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:
> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.
> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?
...having pointed out a couple of times now that there's no physical
requirement that the supplies be shared between slots never mind with
the controller. Also note that as I've said depending on what the
actual requirements of the controller node are you might want to have
the regulators in both places, and further note that the driver does not
have to actively use everything in the binding document (although if
it's not using something that turns out to be a requirement it's likely
to run into hardware where that causes bugs at some point).
Frankly I'm not clear why you're trying to handle powering on PCI slots
in a specific driver, surely PCI devices are PCI devices regardless of
the controller?
On 4/7/2021 4:27 AM, Mark Brown wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
>
>> I'm a little confused -- here is how I remember the chronology of the
>> "DT bindings" commit reviews, please correct me if I'm wrong:
>
>> o JimQ submitted a pullreq for using voltage regulators in the same
>> style as the existing "rockport" PCIe driver.
>> o After some deliberation, RobH preferred that the voltage regulators
>> should go into the PCIe subnode device's DT node.
>> o JimQ put the voltage regulators in the subnode device's DT node.
>> o MarkB didn't like the fact that the code did a global search for the
>> regulator since it could not provide the owning struct device* handle.
>> o RobH relented, and said that if it is just two specific and standard
>> voltage regulators, perhaps they can go in the parent DT node after
>> all.
>> o JimQ put the regulators back in the PCIe node.
>> o MarkB now wants the regulators to go back into the child node again?
>
> ...having pointed out a couple of times now that there's no physical
> requirement that the supplies be shared between slots never mind with
> the controller. Also note that as I've said depending on what the
> actual requirements of the controller node are you might want to have
> the regulators in both places, and further note that the driver does not
> have to actively use everything in the binding document (although if
> it's not using something that turns out to be a requirement it's likely
> to run into hardware where that causes bugs at some point).
>
> Frankly I'm not clear why you're trying to handle powering on PCI slots
> in a specific driver, surely PCI devices are PCI devices regardless of
> the controller?
There is no currently a way to deal with that situation since you have a
chicken and egg problem to solve: there is no pci_device created until
you enumerate the PCI bus, and you cannot enumerate the bus and create
those pci_devices unless you power on the slots/PCIe end-points attached
to the root complex. There are precedents like the rockchip PCIe RC
driver, and yes, we should not be cargo culting this, which is why we
are trying to understand what is it that should be done here and there
has been conflicting advice, or rather our interpretation has led to
perceiving it as a conflicting.
If the regulator had a variation where it supported passing a
device_node reference to look up regulator properties, we could solve
this generically for all devices, that does not exist, and you stated
you will not accept changes like those, fair enough.
When you suggested to look at soundwire for an example of "software
devices", we did that but it was not clear where the sdw_slave would be
created prior to sdw_slave_add(), but this does not matter too much.
Let us say we try to solve this generically using the same idea that we
would pre-populate pci_device prior to calling pci_scan_child_bus(). We
could do something along these lines:
- pci_scan_child_bus() would attempt to walk the list of device_node
children from the PCIe root complex's device_node and call
pci_alloc_dev() for each of these devices that it finds, along with
calling device_initialize() and ensuring that pci_dev::device::of_node
is set correctly by calling pci_set_of_node()/set_dev_node(). Finally we
call list_add_tail() with the pci_bus_sem semaphore held to add that
pci_device to bus->devices such that we can later find them
- from there on we try to get the regulators of those pci_device objects
we just allocated and we try to enable their regulators, either based on
a specific list of supplies or just trying to enable all supplied declared.
- now pci_scan_child_bus() will attempt to scan the bus for real by
reading the pci_device objects ID but we already have such objects, so
we need to update pci_scan_device() to search bus::devices and if
nothing is found we allocate one
Is that roughly what you have in mind as to what should be done?
--
Florian
On Wed, Apr 07, 2021 at 11:35:57AM -0700, Florian Fainelli wrote:
> On 4/7/2021 4:27 AM, Mark Brown wrote:
> > Frankly I'm not clear why you're trying to handle powering on PCI slots
> > in a specific driver, surely PCI devices are PCI devices regardless of
> > the controller?
> There is no currently a way to deal with that situation since you have a
> chicken and egg problem to solve: there is no pci_device created until
> you enumerate the PCI bus, and you cannot enumerate the bus and create
> those pci_devices unless you power on the slots/PCIe end-points attached
> to the root complex. There are precedents like the rockchip PCIe RC
> driver, and yes, we should not be cargo culting this, which is why we
> are trying to understand what is it that should be done here and there
> has been conflicting advice, or rather our interpretation has led to
> perceiving it as a conflicting.
As you note below I've pointed you at Slimbus which has a similar
problem and solves it at a bus level although it thought of this from
day one which makes life easier; I do think it'd be good to get this
stuff in the driver core since it's an issue that affects every
enumerable bus in the end but nobody's summoned up the enthusiasm for
that (including me).
> If the regulator had a variation where it supported passing a
> device_node reference to look up regulator properties, we could solve
> this generically for all devices, that does not exist, and you stated
> you will not accept changes like those, fair enough.
I'm certainly not enthusiastic about the idea and the likely abuse isn't
inspiring, and of course regulators aren't the only resource that might
be needed to get something up and running and would need to be extended
in the end. That said I've not seen any concrete proposals either.
> When you suggested to look at soundwire for an example of "software
> devices", we did that but it was not clear where the sdw_slave would be
> created prior to sdw_slave_add(), but this does not matter too much.
Looks like sdw_acpi_find_slaves() and sdw_of_find_slaves().
> Let us say we try to solve this generically using the same idea that we
> would pre-populate pci_device prior to calling pci_scan_child_bus(). We
> could do something along these lines:
...
> - from there on we try to get the regulators of those pci_device objects
> we just allocated and we try to enable their regulators, either based on
> a specific list of supplies or just trying to enable all supplied declared.
I'd suggest specfying the supplies that PCI provides to slots in the
spec with standard names and just using that list, at least as a start.
That'd probably cover most cases and allow the binding to be written at
the generic PCI level rather than having individual devices need to name
their supplies for the binding documentation and validation stuff which
seems easier. Devices with extra stuff can always extend the binding.
> - now pci_scan_child_bus() will attempt to scan the bus for real by
> reading the pci_device objects ID but we already have such objects, so
> we need to update pci_scan_device() to search bus::devices and if
> nothing is found we allocate one
> Is that roughly what you have in mind as to what should be done?
Yes, pretty much. Ideally there'd be some way for drivers to get a
callback prior to enumeration to handle any custom stuff for embedded
cases but unless someone actually has a use case for that you could just
punt.
On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <[email protected]> wrote:
> >
> > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
> >
> > > > No great problem with having these in the controller node (assming it
> > > > accurately describes the hardware) but I do think we ought to also be
> > > > able to describe these per slot.
PCIe is effectively point to point, so there's only 1 slot unless
there's a PCIe switch in the middle. If that's the case, then it's all
more complicated.
> > > Can you explain what you think that would look like in the DT?
> >
> > I *think* that's just some properties on the nodes for the endpoints,
> > note that the driver could just ignore them for now. Not sure where or
> > if we document any extensions but child nodes are in section 4 of the
> > v2.1 PCI bus binding.
>
> Hi Mark,
>
> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:
>
> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.
IIRC, that's because you said there isn't a standard slot.
> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?
>
> Folks, please advise.
>
> Regards,
> Jim Quinlan
> Broadcom STB
On Thu, Apr 08, 2021 at 11:20:16AM -0500, Rob Herring wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <[email protected]> wrote:
> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.
> PCIe is effectively point to point, so there's only 1 slot unless
> there's a PCIe switch in the middle. If that's the case, then it's all
> more complicated.
So even for PCIe that case exists, and it's not entirely clear to me
that this is all definitively PCIe specific.
> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.
> IIRC, that's because you said there isn't a standard slot.
I recall some message in the thread that said both cases exist even with
this specific system.
On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <[email protected]> wrote:
>
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <[email protected]> wrote:
> > >
> > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
> > >
> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.
>
> PCIe is effectively point to point, so there's only 1 slot unless
> there's a PCIe switch in the middle. If that's the case, then it's all
> more complicated.
>
> > > > Can you explain what you think that would look like in the DT?
> > >
> > > I *think* that's just some properties on the nodes for the endpoints,
> > > note that the driver could just ignore them for now. Not sure where or
> > > if we document any extensions but child nodes are in section 4 of the
> > > v2.1 PCI bus binding.
> >
> > Hi Mark,
> >
> > I'm a little confused -- here is how I remember the chronology of the
> > "DT bindings" commit reviews, please correct me if I'm wrong:
> >
> > o JimQ submitted a pullreq for using voltage regulators in the same
> > style as the existing "rockport" PCIe driver.
> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.
>
> IIRC, that's because you said there isn't a standard slot.
Admittedly, I'm not exactly sure what you mean by a "standard slot".
Our PCIIe HW does not support hotplug or have a presence bit or
anything like that. Our root complex has one port; it can only
directly connect to a single switch or endpoint. This connection shows
up as slot0. The voltage regulator(s) involved depend on a GPIO that
turns the power on/off for the connected device/chip. The gpio pin
can vary from board to board but this is easily handled in our DT.
Some boards have regulators that are always on and not associated with
a GPIO pin -- these have no representation in our DT.
Regards,
Jim
>
> > o JimQ put the voltage regulators in the subnode device's DT node.
> > o MarkB didn't like the fact that the code did a global search for the
> > regulator since it could not provide the owning struct device* handle.
> > o RobH relented, and said that if it is just two specific and standard
> > voltage regulators, perhaps they can go in the parent DT node after
> > all.
> > o JimQ put the regulators back in the PCIe node.
> > o MarkB now wants the regulators to go back into the child node again?
> >
> > Folks, please advise.
> >
> > Regards,
> > Jim Quinlan
> > Broadcom STB
On Tue, Apr 06, 2021 at 05:47:08PM +0100, Mark Brown wrote:
> On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote:
> > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> > allows optional regulators to be attached and controlled by the PCIe RC
> > driver. That being said, this driver searches in the DT subnode (the EP
> > node, eg pci@0,0) for the regulator property.
>
> > The use of a regulator property in the pcie EP subnode such as
> > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> > file at
> >
> > https://github.com/devicetree-org/dt-schema/pull/54
> >
> > Signed-off-by: Jim Quinlan <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >
> > aspm-no-l0s: true
> >
> > + vpcie12v-supply: true
> > + vpcie3v3-supply: true
> > +
>
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.
My hesistancy here is child nodes are already defined to be devices, not
slots. That's generally the same thing though. However, 'reg' is a bit
problematic as it includes the bus number which is dynamically
assigned. (This is a mismatch between true OpenFirmware and FDT as OF
was designed to populate the DT with what was discovered and that
includes some runtime config such as bus number assignments.) Maybe we
just say for FDT, the bus number is always 0 and ignored. In any case,
there needs to be some thought on this as well as the more complicated
case of devices needing device specific setup to be enumerated.
Rob
On Thu, Apr 08, 2021 at 12:58:05PM -0400, Jim Quinlan wrote:
> On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <[email protected]> wrote:
> >
> > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <[email protected]> wrote:
> > > >
> > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <[email protected]> wrote:
> > > >
> > > > > > No great problem with having these in the controller node (assming it
> > > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > > able to describe these per slot.
> >
> > PCIe is effectively point to point, so there's only 1 slot unless
> > there's a PCIe switch in the middle. If that's the case, then it's all
> > more complicated.
> >
> > > > > Can you explain what you think that would look like in the DT?
> > > >
> > > > I *think* that's just some properties on the nodes for the endpoints,
> > > > note that the driver could just ignore them for now. Not sure where or
> > > > if we document any extensions but child nodes are in section 4 of the
> > > > v2.1 PCI bus binding.
> > >
> > > Hi Mark,
> > >
> > > I'm a little confused -- here is how I remember the chronology of the
> > > "DT bindings" commit reviews, please correct me if I'm wrong:
> > >
> > > o JimQ submitted a pullreq for using voltage regulators in the same
> > > style as the existing "rockport" PCIe driver.
> > > o After some deliberation, RobH preferred that the voltage regulators
> > > should go into the PCIe subnode device's DT node.
> >
> > IIRC, that's because you said there isn't a standard slot.
> Admittedly, I'm not exactly sure what you mean by a "standard slot".
> Our PCIIe HW does not support hotplug or have a presence bit or
> anything like that. Our root complex has one port; it can only
> directly connect to a single switch or endpoint. This connection shows
> up as slot0. The voltage regulator(s) involved depend on a GPIO that
> turns the power on/off for the connected device/chip. The gpio pin
> can vary from board to board but this is easily handled in our DT.
> Some boards have regulators that are always on and not associated with
> a GPIO pin -- these have no representation in our DT.
By standard slot, I mean you have standard voltage rails 12V and 3.3V
(or 1.5 and 3.3 for mini PCIe) and PERST# signal, no other extra
things to make a device discoverable, and the timing for
those rails and PERST# follow what the spec defines.
There's also CLKREQ, WAKE, and hotplug detect signals, but I think those
are all optional and could be tied off. I think most PCI h/w is not
hotplug capable.
Rob