LinuxLists.cc - [PATCH v2 0/6] PCI: brcmstb: Re-submit reverted patchset

[permalink] [raw]

Subject: [PATCH v2 1/6] PCI: brcmstb: Remove unnecessary forward declarations

The forward function declarations in this driver are removed. Besides
that, some constant structure definitions are moved towards a lower
position in the file. There are no changes to the code that has been
moved.

Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 149 +++++++++++++-------------
1 file changed, 72 insertions(+), 77 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e61058e13818..bd88a0a46c63 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -190,11 +190,6 @@

/* Forward declarations */
struct brcm_pcie;
-static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val);
-static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val);
-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);

enum {
RGR1_SW_INIT_1,
@@ -223,66 +218,6 @@ struct pcie_cfg_data {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
};

-static const int pcie_offsets[] = {
- [RGR1_SW_INIT_1] = 0x9210,
- [EXT_CFG_INDEX] = 0x9000,
- [EXT_CFG_DATA] = 0x9004,
-};
-
-static const int pcie_offsets_bmips_7425[] = {
- [RGR1_SW_INIT_1] = 0x8010,
- [EXT_CFG_INDEX] = 0x8300,
- [EXT_CFG_DATA] = 0x8304,
-};
-
-static const struct pcie_cfg_data generic_cfg = {
- .offsets = pcie_offsets,
- .type = GENERIC,
- .perst_set = brcm_pcie_perst_set_generic,
- .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm7425_cfg = {
- .offsets = pcie_offsets_bmips_7425,
- .type = BCM7425,
- .perst_set = brcm_pcie_perst_set_generic,
- .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm7435_cfg = {
- .offsets = pcie_offsets,
- .type = BCM7435,
- .perst_set = brcm_pcie_perst_set_generic,
- .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm4908_cfg = {
- .offsets = pcie_offsets,
- .type = BCM4908,
- .perst_set = brcm_pcie_perst_set_4908,
- .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const int pcie_offset_bcm7278[] = {
- [RGR1_SW_INIT_1] = 0xc010,
- [EXT_CFG_INDEX] = 0x9000,
- [EXT_CFG_DATA] = 0x9004,
-};
-
-static const struct pcie_cfg_data bcm7278_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,
-};
-
-static const struct pcie_cfg_data bcm2711_cfg = {
- .offsets = pcie_offsets,
- .type = BCM2711,
- .perst_set = brcm_pcie_perst_set_generic,
- .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
struct brcm_msi {
struct device *dev;
void __iomem *base;
@@ -775,18 +710,6 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
return base + DATA_ADDR(pcie);
}

-static struct pci_ops brcm_pcie_ops = {
- .map_bus = brcm_pcie_map_conf,
- .read = pci_generic_config_read,
- .write = pci_generic_config_write,
-};
-
-static struct pci_ops brcm_pcie_ops32 = {
- .map_bus = brcm_pcie_map_conf32,
- .read = pci_generic_config_read32,
- .write = pci_generic_config_write32,
-};
-
static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
{
u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK;
@@ -1316,6 +1239,66 @@ static int brcm_pcie_remove(struct platform_device *pdev)
return 0;
}

+static const int pcie_offsets[] = {
+ [RGR1_SW_INIT_1] = 0x9210,
+ [EXT_CFG_INDEX] = 0x9000,
+ [EXT_CFG_DATA] = 0x9004,
+};
+
+static const int pcie_offsets_bmips_7425[] = {
+ [RGR1_SW_INIT_1] = 0x8010,
+ [EXT_CFG_INDEX] = 0x8300,
+ [EXT_CFG_DATA] = 0x8304,
+};
+
+static const struct pcie_cfg_data generic_cfg = {
+ .offsets = pcie_offsets,
+ .type = GENERIC,
+ .perst_set = brcm_pcie_perst_set_generic,
+ .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm7425_cfg = {
+ .offsets = pcie_offsets_bmips_7425,
+ .type = BCM7425,
+ .perst_set = brcm_pcie_perst_set_generic,
+ .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm7435_cfg = {
+ .offsets = pcie_offsets,
+ .type = BCM7435,
+ .perst_set = brcm_pcie_perst_set_generic,
+ .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm4908_cfg = {
+ .offsets = pcie_offsets,
+ .type = BCM4908,
+ .perst_set = brcm_pcie_perst_set_4908,
+ .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const int pcie_offset_bcm7278[] = {
+ [RGR1_SW_INIT_1] = 0xc010,
+ [EXT_CFG_INDEX] = 0x9000,
+ [EXT_CFG_DATA] = 0x9004,
+};
+
+static const struct pcie_cfg_data bcm7278_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,
+};
+
+static const struct pcie_cfg_data bcm2711_cfg = {
+ .offsets = pcie_offsets,
+ .type = BCM2711,
+ .perst_set = brcm_pcie_perst_set_generic,
+ .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1328,6 +1311,18 @@ static const struct of_device_id brcm_pcie_match[] = {
{},
};

+static struct pci_ops brcm_pcie_ops = {
+ .map_bus = brcm_pcie_map_conf,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+};
+
+static struct pci_ops brcm_pcie_ops32 = {
+ .map_bus = brcm_pcie_map_conf32,
+ .read = pci_generic_config_read32,
+ .write = pci_generic_config_write32,
+};
+
static int brcm_pcie_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node, *msi_np;
--
2.17.1

2022-07-16 22:28:30

[permalink] [raw]

Subject: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts

Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
established between itself and downstream, any subsequent config space
access causes a CPU abort. This commit sets a "refusal mode" if the PCIe
link-up fails, and this has our pci_ops map_bus function returning a NULL
address, which in turn precludes the access from happening.

Right now, "refusal mode" is window dressing. It will become relevant
in a future commit when brcm_pcie_start_link() is invoked during
enumeration instead of before it.

Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c026446d5830..72219a4f3964 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -255,6 +255,7 @@ struct brcm_pcie {
u32 hw_rev;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+ bool refusal_mode;
};

static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
if (pci_is_root_bus(bus))
return PCI_SLOT(devfn) ? NULL : base + where;

+ if (pcie->refusal_mode) {
+ /*
+ * At this point we do not have PCIe link-up. If there is
+ * a config read or write access besides those targeting
+ * the host bridge, our PCIe HW throws a CPU abort. To
+ * prevent this we return the NULL address. The calling
+ * functions -- pci_generic_config_*() -- will notice this
+ * and not perform the access, and if it is a read access,
+ * 0xffffffff is returned.
+ */
+ return NULL;
+ }
+
/* For devices, write to the config space index register */
idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
@@ -704,6 +718,11 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
if (pci_is_root_bus(bus))
return PCI_SLOT(devfn) ? NULL : base + (where & ~0x3);

+ if (pcie->refusal_mode) {
+ /* See note above in brcm_pcie_map_conf() */
+ return NULL;
+ }
+
/* For devices, write to the config space index register */
idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & ~3));
writel(idx, base + IDX_ADDR(pcie));
@@ -989,6 +1008,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
dev_err(dev, "link down\n");
return -ENODEV;
}
+ pcie->refusal_mode = false;

if (!brcm_pcie_rc_mode(pcie)) {
dev_err(dev, "PCIe misconfigured; is in EP mode\n");
@@ -1134,6 +1154,8 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
void __iomem *base = pcie->base;
int tmp;

+ pcie->refusal_mode = true;
+
if (brcm_pcie_link_up(pcie))
brcm_pcie_enter_l23(pcie);
/* Assert fundamental reset */
@@ -1185,6 +1207,7 @@ static int brcm_pcie_resume(struct device *dev)
u32 tmp;
int ret;

+ pcie->refusal_mode = true;
base = pcie->base;
ret = clk_prepare_enable(pcie->clk);
if (ret)
@@ -1361,6 +1384,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
pcie->type = data->type;
pcie->perst_set = data->perst_set;
pcie->bridge_sw_init_set = data->bridge_sw_init_set;
+ pcie->refusal_mode = true;

pcie->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(pcie->base))
--
2.17.1

2022-07-16 22:41:14

[permalink] [raw]

Subject: [PATCH v2 6/6] PCI: brcmstb: Do not turn off WOL regulators on suspend

If any downstream device can be a wakeup device, do not turn off the
regulators as the device will need them on.

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 54 ++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 88339fde0df7..570c98594d47 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -264,6 +264,7 @@ struct brcm_pcie {
bool refusal_mode;
bool regulator_oops;
struct subdev_regulators *sr;
+ bool ep_wakeup_capable;
};

static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1301,9 +1302,21 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
pcie->bridge_sw_init_set(pcie, 1);
}

+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
+{
+ bool *ret = data;
+
+ if (device_may_wakeup(&dev->dev)) {
+ *ret = true;
+ dev_info(&dev->dev, "disable cancelled for wake-up device\n");
+ }
+ return (int) *ret;
+}
+
static int brcm_pcie_suspend_noirq(struct device *dev)
{
struct brcm_pcie *pcie = dev_get_drvdata(dev);
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
int ret;

brcm_pcie_turn_off(pcie);
@@ -1322,11 +1335,22 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
}

if (pcie->sr) {
- ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
- if (ret) {
- dev_err(dev, "Could not turn off regulators\n");
- reset_control_reset(pcie->rescal);
- return ret;
+ /*
+ * Now turn off the regulators, but if at least one
+ * downstream device is enabled as a wake-up source, do not
+ * turn off regulators.
+ */
+ pcie->ep_wakeup_capable = false;
+ pci_walk_bus(bridge->bus, pci_dev_may_wakeup,
+ &pcie->ep_wakeup_capable);
+ if (!pcie->ep_wakeup_capable) {
+ ret = regulator_bulk_disable(pcie->sr->num_supplies,
+ pcie->sr->supplies);
+ if (ret) {
+ dev_err(dev, "Could not turn off regulators\n");
+ reset_control_reset(pcie->rescal);
+ return ret;
+ }
}
}
clk_disable_unprepare(pcie->clk);
@@ -1371,11 +1395,21 @@ static int brcm_pcie_resume_noirq(struct device *dev)
goto err_reset;

if (pcie->sr) {
- ret = regulator_bulk_enable(pcie->sr->num_supplies,
- pcie->sr->supplies);
- if (ret) {
- dev_err(dev, "Could not turn on regulators\n");
- goto err_reset;
+ if (pcie->ep_wakeup_capable) {
+ /*
+ * We are resuming from a suspend. In the 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;
+ } else {
+ ret = regulator_bulk_enable(pcie->sr->num_supplies,
+ pcie->sr->supplies);
+ if (ret) {
+ dev_err(dev, "Could not turn on regulators\n");
+ goto err_reset;
+ }
}
}

--
2.17.1

2022-07-16 22:41:14

[permalink] [raw]

Subject: [PATCH v2 4/6] PCI: brcmstb: Add mechanism to turn on subdev regulators

Add a mechanism to identify standard PCIe regulators in the DT, allocate
them, and turn them on before the rest of the bus is scanned during
pci_host_probe().

The allocated structure that contains the regulators is stored in the port
driver dev.driver_data field. Here is a point-by-point of how and when
this mechanism is activated:

If:
-- PCIe RC driver sets pci_ops {add,remove)_bus to
pci_subdev_regulators_{add,remove}_bus during its probe.
-- There is a root port DT node under the host bridge DT node.
-- During the RC driver's pci_host_probe() the add_bus callback
is invoked where (bus->parent && pci_is_root_bus(bus->parent)
is true

Then:
-- A struct subdev_regulators structure will be allocated and
assigned to bus->dev.driver_data.
-- regulator_bulk_{get,enable} will be invoked on &bus->dev
and the former will search for and process any
vpcie{12v,3v3,3v3aux}-supply properties that reside in the
root port DT node.
-- The regulators will be turned off/on for any unbind/bind operations.
-- The regulators will be turned off/on for any suspend/resumes, but
only if the RC driver handles this on its own. This will appear
in a later commit for the pcie-brcmstb.c driver.

The unabridged reason for doing this is as follows. We would like the
Broadcom STB PCIe root complex driver (and others) to be able to turn
off/on regulators[1] that provide power to endpoint[2] devices. Typically,
the drivers of these endpoint devices are stock Linux drivers that are not
aware that these regulator(s) exist and must be turned on for the driver to
be probed. The simple solution of course is to turn these regulators on at
boot and keep them on. However, this solution does not satisfy at least
three of our usage modes:

1. For example, one customer uses multiple PCIe controllers, but wants
the ability to, by script invoking and unbind, turn any or all of them
and their subdevices off to save power, e.g. when in battery mode.

2. Another example is when a watchdog script discovers that an endpoint
device is in an unresponsive state and would like to unbind, power
toggle, and re-bind just the PCIe endpoint and controller.

3. Of course we also want power turned off during suspend mode. However,
some endpoint devices may be able to "wake" during suspend and we need
to recognise this case and veto the nominal act of turning off its
regulator. Such is the case with Wake-on-LAN and Wake-on-WLAN support
where the PCIe endpoint device needs to be kept powered on in order to
receive network packets and wake the system.

In all of these cases it is advantageous for the PCIe controller to govern
the turning off/on the regulators needed by the endpoint device. The first
two cases can be done by simply unbinding and binding the PCIe controller,
if the controller has control of these regulators.

[1] These regulators typically govern the actual power supply to the
endpoint chip. Sometimes they may be the official PCIe socket
power -- such as 3.3v or aux-3.3v. Sometimes they are truly
the regulator(s) that supply power to the EP chip.

[2] The 99% configuration of our boards is a single endpoint device
attached to the PCIe controller. I use the term endpoint but it could
possibly mean a switch as well.

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 96 +++++++++++++++++++++++++++
1 file changed, 96 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 72219a4f3964..f98a5338fa8e 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>
@@ -218,6 +219,11 @@ struct pcie_cfg_data {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
};

+struct subdev_regulators {
+ unsigned int num_supplies;
+ struct regulator_bulk_data supplies[];
+};
+
struct brcm_msi {
struct device *dev;
void __iomem *base;
@@ -1077,6 +1083,92 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
return 0;
}

+static const char * const supplies[] = {
+ "vpcie3v3",
+ "vpcie3v3aux",
+ "vpcie12v",
+};
+
+static void *alloc_subdev_regulators(struct device *dev)
+{
+ const size_t size = sizeof(struct subdev_regulators)
+ + sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
+ struct subdev_regulators *sr;
+ int i;
+
+ sr = devm_kzalloc(dev, size, GFP_KERNEL);
+ if (sr) {
+ sr->num_supplies = ARRAY_SIZE(supplies);
+ for (i = 0; i < ARRAY_SIZE(supplies); i++)
+ sr->supplies[i].supply = supplies[i];
+ }
+
+ return sr;
+}
+
+static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
+{
+ struct device *dev = &bus->dev;
+ struct subdev_regulators *sr;
+ int ret;
+
+ if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+ return 0;
+
+ if (dev->driver_data) {
+ /*
+ * Oops, this is unfortunate. We are using the port
+ * driver's driver_data field to store our regulator info
+ * and it appears that another driver started using it as
+ * well. If so, be a team player do not overwrite it. We
+ * may still be okay if there are no regulators.
+ */
+ dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
+ return 0;
+ }
+
+ sr = alloc_subdev_regulators(dev);
+ if (!sr)
+ return -ENOMEM;
+
+ /*
+ * There is not much of a point to return an error as currently it
+ * will cause a WARNING() from pci_alloc_child_bus(). So only
+ * return the error if it is -ENOMEM. Note that we are always
+ * doing a dev_err() for other erros.
+ */
+ ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+ if (ret) {
+ dev_err(dev, "failed to get regulators for downstream device\n");
+ return 0;
+ }
+
+ ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+ if (ret) {
+ dev_err(dev, "failed to enable regulators for downstream device\n");
+ regulator_bulk_free(sr->num_supplies, sr->supplies);
+ return 0;
+ }
+
+ dev->driver_data = sr;
+
+ return 0;
+}
+
+static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
+{
+ struct device *dev = &bus->dev;
+ struct subdev_regulators *sr = dev->driver_data;
+
+ if (!dev->of_node || !sr || !bus->parent || !pci_is_root_bus(bus->parent))
+ return;
+
+ if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
+ dev_err(dev, "failed to disable regulators for downstream device\n");
+ regulator_bulk_free(sr->num_supplies, sr->supplies);
+ dev->driver_data = NULL;
+}
+
/* L23 is a low-power PCIe link state */
static void brcm_pcie_enter_l23(struct brcm_pcie *pcie)
{
@@ -1351,12 +1443,16 @@ static struct pci_ops brcm_pcie_ops = {
.map_bus = brcm_pcie_map_conf,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
+ .add_bus = pci_subdev_regulators_add_bus,
+ .remove_bus = pci_subdev_regulators_remove_bus,
};

static struct pci_ops brcm_pcie_ops32 = {
.map_bus = brcm_pcie_map_conf32,
.read = pci_generic_config_read32,
.write = pci_generic_config_write32,
+ .add_bus = pci_subdev_regulators_add_bus,
+ .remove_bus = pci_subdev_regulators_remove_bus,
};

static int brcm_pcie_probe(struct platform_device *pdev)
--
2.17.1

2022-07-16 22:42:26

[permalink] [raw]

Subject: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

Currently, the function does the setup for establishing PCIe link-up
with the downstream device, and it does the actual link-up as well.
The calling sequence is (roughly) the following in the probe:

-> brcm_pcie_probe()
-> brcm_pcie_setup(); /* Set-up and link-up */
-> pci_host_probe(bridge);

This commit splits the setup function in two: brcm_pcie_setup(), which only
does the set-up, and brcm_pcie_start_link(), which only does the link-up.
The reason why we are doing this is to lay a foundation for subsequent
commits so that we can turn on any power regulators, as described in the
root port's DT node, prior to doing link-up. We do this by defining an
add_bus() callback which is invoked during enumeraion. At the end of this
patchset the probe function trace will look something like this:

-> brcm_pcie_probe()
-> brcm_pcie_setup(); /* Set-up only */
-> pci_host_probe(bridge);
-> [enumeration]
-> pci_alloc_child_bus()
-> bus->ops->add_bus(bus); /* We've set this op */
-> brcm_pcie_add_bus() /* Our callback */
-> [turn on regulators] /* Main objective! */
-> brcm_pcie_start_link() /* Link-up */

One final note: some code that was executed after the PCIe linkup is now
placed so that it executes prior to linkup, since this code has to run
prior to the invocation of pci_host_probe().

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 69 +++++++++++++++++----------
1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bd88a0a46c63..c026446d5830 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -849,16 +849,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,

static int brcm_pcie_setup(struct brcm_pcie *pcie)
{
- struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
u64 rc_bar2_offset, rc_bar2_size;
void __iomem *base = pcie->base;
- struct device *dev = pcie->dev;
- struct resource_entry *entry;
- bool ssc_good = false;
- struct resource *res;
- int num_out_wins = 0;
- u16 nlw, cls, lnksta;
- int i, ret, memc;
+ int ret, memc;
u32 tmp, burst, aspm_support;

/* Reset the bridge */
@@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
if (pcie->gen)
brcm_pcie_set_gen(pcie, pcie->gen);

+ /* Don't advertise L0s capability if 'aspm-no-l0s' */
+ aspm_support = PCIE_LINK_STATE_L1;
+ if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+ aspm_support |= PCIE_LINK_STATE_L0S;
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+ u32p_replace_bits(&tmp, aspm_support,
+ PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+
+ /*
+ * For config space accesses on the RC, show the right class for
+ * a PCIe-PCIe bridge (the default setting is to be EP mode).
+ */
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+ u32p_replace_bits(&tmp, 0x060400,
+ PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+
+ return 0;
+}
+
+static int brcm_pcie_start_link(struct brcm_pcie *pcie)
+{
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+ struct device *dev = pcie->dev;
+ void __iomem *base = pcie->base;
+ struct resource_entry *entry;
+ struct resource *res;
+ int num_out_wins = 0;
+ u16 nlw, cls, lnksta;
+ bool ssc_good = false;
+ u32 tmp;
+ int ret, i;
+
/* Unassert the fundamental reset */
pcie->perst_set(pcie, 0);

@@ -998,24 +1025,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
num_out_wins++;
}

- /* Don't advertise L0s capability if 'aspm-no-l0s' */
- aspm_support = PCIE_LINK_STATE_L1;
- if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
- aspm_support |= PCIE_LINK_STATE_L0S;
- tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
- u32p_replace_bits(&tmp, aspm_support,
- PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
- writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-
- /*
- * For config space accesses on the RC, show the right class for
- * a PCIe-PCIe bridge (the default setting is to be EP mode).
- */
- tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
- u32p_replace_bits(&tmp, 0x060400,
- PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
- writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-
if (pcie->ssc) {
ret = brcm_pcie_set_ssc(pcie);
if (ret == 0)
@@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
goto err_reset;

+ ret = brcm_pcie_start_link(pcie);
+ if (ret)
+ goto err_reset;
+
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);

@@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
goto fail;

+ ret = brcm_pcie_start_link(pcie);
+ if (ret)
+ goto fail;
+
pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
--
2.17.1

2022-07-16 22:56:14

[permalink] [raw]

Subject: [PATCH v2 5/6] PCI: brcmstb: Add control of subdevice voltage regulators

This Broadcom STB PCIe RC driver has one port and connects directly to one
device, be it a switch or an endpoint. We want to be able to leverage the
recently added mechanism that allocates and turns on/off subdevice
regulators.

All that needs to be done is to put the regulator DT nodes in the bridge
below host and to set the pci_ops methods add_bus and remove_bus.

Note that the pci_subdev_regulators_add_bus() method is wrapped for two
reasons:

1. To achieve link up after the voltage regulators are turned on.

2. If, in the case of an unsuccessful link up, to redirect any PCIe
accesses to subdevices, e.g. the scan for DEV/ID. This redirection
is needed because the Broadcom PCIe HW will issue a CPU abort if such
an access is made when the link is down.

The pci_subdev_regulators_remove_bus() is wrapped as well as
we must avoid invoking it if we see that there is a collision
in the use of dev->driver_data.

[bhelgaas: fold in
https://lore.kernel.org/r/[email protected]]
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 138 ++++++++++++++++++--------
1 file changed, 99 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index f98a5338fa8e..88339fde0df7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -262,6 +262,8 @@ struct brcm_pcie {
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
bool refusal_mode;
+ bool regulator_oops;
+ struct subdev_regulators *sr;
};

static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1112,10 +1114,37 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
struct subdev_regulators *sr;
int ret;

- if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+ sr = alloc_subdev_regulators(dev);
+ if (!sr)
+ return -ENOMEM;
+
+ ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+ if (ret) {
+ dev_err(dev, "failed to get regulators for downstream device\n");
+ return ret;
+ }
+
+ ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+ if (ret) {
+ dev_err(dev, "failed to enable regulators for downstream device\n");
+ regulator_bulk_free(sr->num_supplies, sr->supplies);
+ return ret;
+ }
+ dev->driver_data = sr;
+
+ return 0;
+}
+
+static int brcm_pcie_add_bus(struct pci_bus *bus)
+{
+ struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
+ struct device *dev = &bus->dev;
+ int ret;
+
+ if (!bus->parent || !pci_is_root_bus(bus->parent) || !pcie)
return 0;

- if (dev->driver_data) {
+ if (dev->of_node && dev->driver_data) {
/*
* Oops, this is unfortunate. We are using the port
* driver's driver_data field to store our regulator info
@@ -1124,12 +1153,19 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
* may still be okay if there are no regulators.
*/
dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
- return 0;
+
+ } else if (dev->of_node) {
+ ret = pci_subdev_regulators_add_bus(bus);
+ /* Grab the regulators for suspend/resume */
+ pcie->sr = bus->dev.driver_data;
}

- sr = alloc_subdev_regulators(dev);
- if (!sr)
- return -ENOMEM;
+ /*
+ * We return 0 and turn on the "refusal_mode" so that any further
+ * accesses to the pci_dev just get 0xffffffff
+ */
+ if (brcm_pcie_start_link(pcie) != 0)
+ pcie->refusal_mode = true;

/*
* There is not much of a point to return an error as currently it
@@ -1137,22 +1173,7 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
* return the error if it is -ENOMEM. Note that we are always
* doing a dev_err() for other erros.
*/
- ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
- if (ret) {
- dev_err(dev, "failed to get regulators for downstream device\n");
- return 0;
- }
-
- ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
- if (ret) {
- dev_err(dev, "failed to enable regulators for downstream device\n");
- regulator_bulk_free(sr->num_supplies, sr->supplies);
- return 0;
- }
-
- dev->driver_data = sr;
-
- return 0;
+ return ret == -ENOMEM ? ret : 0;
}

static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
@@ -1160,15 +1181,28 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
struct device *dev = &bus->dev;
struct subdev_regulators *sr = dev->driver_data;

- if (!dev->of_node || !sr || !bus->parent || !pci_is_root_bus(bus->parent))
- return;
-
if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
dev_err(dev, "failed to disable regulators for downstream device\n");
regulator_bulk_free(sr->num_supplies, sr->supplies);
dev->driver_data = NULL;
}

+static void brcm_pcie_remove_bus(struct pci_bus *bus)
+{
+ struct device *dev = &bus->dev;
+ struct brcm_pcie *pcie;
+
+ if (!dev->of_node || !dev->driver_data || !bus->parent ||
+ !pci_is_root_bus(bus->parent))
+ return;
+
+ pcie = (struct brcm_pcie *) bus->sysdata;
+ if (pcie && pcie->sr) {
+ pci_subdev_regulators_remove_bus(bus);
+ pcie->sr = NULL;
+ }
+}
+
/* L23 is a low-power PCIe link state */
static void brcm_pcie_enter_l23(struct brcm_pcie *pcie)
{
@@ -1267,7 +1301,7 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
pcie->bridge_sw_init_set(pcie, 1);
}

-static int brcm_pcie_suspend(struct device *dev)
+static int brcm_pcie_suspend_noirq(struct device *dev)
{
struct brcm_pcie *pcie = dev_get_drvdata(dev);
int ret;
@@ -1287,12 +1321,20 @@ static int brcm_pcie_suspend(struct device *dev)
return ret;
}

+ if (pcie->sr) {
+ ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
+ if (ret) {
+ dev_err(dev, "Could not turn off regulators\n");
+ reset_control_reset(pcie->rescal);
+ return ret;
+ }
+ }
clk_disable_unprepare(pcie->clk);

return 0;
}

-static int brcm_pcie_resume(struct device *dev)
+static int brcm_pcie_resume_noirq(struct device *dev)
{
struct brcm_pcie *pcie = dev_get_drvdata(dev);
void __iomem *base;
@@ -1328,15 +1370,27 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
goto err_reset;

+ if (pcie->sr) {
+ ret = regulator_bulk_enable(pcie->sr->num_supplies,
+ pcie->sr->supplies);
+ if (ret) {
+ dev_err(dev, "Could not turn on regulators\n");
+ goto err_reset;
+ }
+ }
+
ret = brcm_pcie_start_link(pcie);
if (ret)
- goto err_reset;
+ goto err_regulator;

if (pcie->msi)
brcm_msi_set_regs(pcie->msi);

return 0;

+err_regulator:
+ if (pcie->sr)
+ regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
err_reset:
reset_control_rearm(pcie->rescal);
err_disable_clk:
@@ -1443,16 +1497,16 @@ static struct pci_ops brcm_pcie_ops = {
.map_bus = brcm_pcie_map_conf,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
- .add_bus = pci_subdev_regulators_add_bus,
- .remove_bus = pci_subdev_regulators_remove_bus,
+ .add_bus = brcm_pcie_add_bus,
+ .remove_bus = brcm_pcie_remove_bus
};

static struct pci_ops brcm_pcie_ops32 = {
.map_bus = brcm_pcie_map_conf32,
.read = pci_generic_config_read32,
.write = pci_generic_config_write32,
- .add_bus = pci_subdev_regulators_add_bus,
- .remove_bus = pci_subdev_regulators_remove_bus,
+ .add_bus = brcm_pcie_add_bus,
+ .remove_bus = brcm_pcie_remove_bus
};

static int brcm_pcie_probe(struct platform_device *pdev)
@@ -1526,10 +1580,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
goto fail;

- ret = brcm_pcie_start_link(pcie);
- if (ret)
- goto fail;
-
pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
@@ -1551,7 +1601,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, pcie);

- return pci_host_probe(bridge);
+ ret = pci_host_probe(bridge);
+ if (!ret && !brcm_pcie_link_up(pcie))
+ ret = -ENODEV;
+
+ if (ret) {
+ brcm_pcie_remove(pdev);
+ return ret;
+ }
+
+ return 0;
+
fail:
__brcm_pcie_remove(pcie);
return ret;
@@ -1560,8 +1620,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
MODULE_DEVICE_TABLE(of, brcm_pcie_match);

static const struct dev_pm_ops brcm_pcie_pm_ops = {
- .suspend = brcm_pcie_suspend,
- .resume = brcm_pcie_resume,
+ .suspend_noirq = brcm_pcie_suspend_noirq,
+ .resume_noirq = brcm_pcie_resume_noirq,
};

static struct platform_driver brcm_pcie_driver = {
--
2.17.1

2022-07-18 13:21:22

by Pali Rohár

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

Hello!

On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote:
> @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> if (pcie->gen)
> brcm_pcie_set_gen(pcie, pcie->gen);
>
> + /* Don't advertise L0s capability if 'aspm-no-l0s' */
> + aspm_support = PCIE_LINK_STATE_L1;
> + if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> + aspm_support |= PCIE_LINK_STATE_L0S;
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> + u32p_replace_bits(&tmp, aspm_support,
> + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> +
> + /*
> + * For config space accesses on the RC, show the right class for
> + * a PCIe-PCIe bridge (the default setting is to be EP mode).
> + */
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> + u32p_replace_bits(&tmp, 0x060400,

There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it
instead of magic constant.

I introduced it recently in commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004

> + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> +
> + return 0;
> +}

2022-07-18 14:33:27

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 9:11 AM Pali Rohár <[email protected]> wrote:
>
> Hello!
>
> On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote:
> > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > if (pcie->gen)
> > brcm_pcie_set_gen(pcie, pcie->gen);
> >
> > + /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > + aspm_support = PCIE_LINK_STATE_L1;
> > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > + aspm_support |= PCIE_LINK_STATE_L0S;
> > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > + u32p_replace_bits(&tmp, aspm_support,
> > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > +
> > + /*
> > + * For config space accesses on the RC, show the right class for
> > + * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > + */
> > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > + u32p_replace_bits(&tmp, 0x060400,
>
> There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it
> instead of magic constant.

Will do, thanks.

Jim Quinlan
Broadcom STB

>
> I introduced it recently in commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004
>
> > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > +
> > + return 0;
> > +}

Attachments:

2022-07-18 17:24:25

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 09:37:08AM -0400, Jim Quinlan wrote:
> On Mon, Jul 18, 2022 at 9:11 AM Pali Roh?r <[email protected]> wrote:
> >
> > Hello!
> >
> > On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote:
> > > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > if (pcie->gen)
> > > brcm_pcie_set_gen(pcie, pcie->gen);
> > >
> > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > + aspm_support = PCIE_LINK_STATE_L1;
> > > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > + aspm_support |= PCIE_LINK_STATE_L0S;
> > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > + u32p_replace_bits(&tmp, aspm_support,
> > > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > +
> > > + /*
> > > + * For config space accesses on the RC, show the right class for
> > > + * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > + */
> > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > + u32p_replace_bits(&tmp, 0x060400,
> >
> > There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it
> > instead of magic constant.
>
> Will do, thanks.

I can fix that up locally.

> > I introduced it recently in commit:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004
> >
> > > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > +
> > > + return 0;
> > > +}

> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2022-07-18 18:09:32

by Pali Rohár

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Monday 18 July 2022 12:05:28 Bjorn Helgaas wrote:
> On Mon, Jul 18, 2022 at 09:37:08AM -0400, Jim Quinlan wrote:
> > On Mon, Jul 18, 2022 at 9:11 AM Pali Rohár <[email protected]> wrote:
> > >
> > > Hello!
> > >
> > > On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote:
> > > > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > > if (pcie->gen)
> > > > brcm_pcie_set_gen(pcie, pcie->gen);
> > > >
> > > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > > + aspm_support = PCIE_LINK_STATE_L1;
> > > > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > > + aspm_support |= PCIE_LINK_STATE_L0S;
> > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > + u32p_replace_bits(&tmp, aspm_support,
> > > > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > +
> > > > + /*
> > > > + * For config space accesses on the RC, show the right class for
> > > > + * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > > + */
> > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > + u32p_replace_bits(&tmp, 0x060400,
> > >
> > > There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it
> > > instead of magic constant.
> >
> > Will do, thanks.
>
> I can fix that up locally.

Great!

I did git grep on recent master branch and found another candidates with
magic numbers which refers to PCI_CLASS_BRIDGE_PCI_NORMAL:

arch/mips/pci/pci-mt7620.c: pcie_w32(0x06040001, RALINK_PCI0_CLASS);
arch/mips/pci/pci-rt3883.c: rt3883_pci_w32(rpc, 0x06040001, RT3883_PCI_REG_CLASS(1));
arch/powerpc/platforms/4xx/pci.c: out_le32(mbase + 0x208, 0x06040001);
drivers/pci/controller/pcie-brcmstb.c: u32p_replace_bits(&tmp, 0x060400,

(class code is stored in upper 24 bits of 32-bit register, so it makes
sense that on lowest 8 bits is something more - 0x01)

What do you think? Does it make sense to send patch which replace above
hex numbers by macros?

> > > I introduced it recently in commit:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004
> > >
> > > > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > +
> > > > + return 0;
> > > > +}
>
>
>
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2022-07-18 19:02:34

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > Currently, the function does the setup for establishing PCIe link-up
> > with the downstream device, and it does the actual link-up as well.
> > The calling sequence is (roughly) the following in the probe:
> >
> > -> brcm_pcie_probe()
> > -> brcm_pcie_setup(); /* Set-up and link-up */
> > -> pci_host_probe(bridge);
> >
> > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > The reason why we are doing this is to lay a foundation for subsequent
> > commits so that we can turn on any power regulators, as described in the
> > root port's DT node, prior to doing link-up.
>
> All drivers that care about power regulators turn them on before
> link-up, but typically those regulators are described directly under
> the host bridge itself.
Hi Bjorn,

Actually, what you describe is what I proposed with my v1 back in Nov 2020.
The binding commit message said,

"Quite similar to the regulator bindings found in "rockchip-pcie-host.txt",
this allows optional regulators to be attached and controlled by the
PCIe RC driver."

>
> IIUC the difference here is that you have regulators described under
> Root Ports (not the host bridge/Root Complex itself), so you don't
> know about them until you've enumerated the Root Ports.
> brcm_pcie_probe() can't turn them on directly because it doesn't know
> what Root Ports are present and doesn't know about regulators below
> them.

The reviewer's requested me to move the regulator node(s) elsewhere,
and at some point later it was requested to be placed under the Root Port
driver. I would love to return them under the host bridge, just say the word!

>
> So I think brcm_pcie_setup() does initialization that doesn't depend
> on the link or any downstream devices, and brcm_pcie_start_link() does
> things that depend on the link being up. Right?
Yes.

>
> If so, "start_link" might be a slight misnomer since AFAICT
> brcm_pcie_start_link() doesn't do anything to initiate link-up except
> maybe deasserting fundamental reset. Some drivers start the LTSSM or
> explicitly enable link training, but brcm_pcie_start_link() doesn't
> seem to do anything like that.
>
> brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
> that really depend on the link being up? If that only affects the
> Root Port, maybe it could be done before link-up?
Some of the registers cannot be accessed until after linkup but these do
not have that issue. I will change this.

Jim Quinlan
Broadcom STB

>
> > We do this by defining an
> > add_bus() callback which is invoked during enumeraion. At the end of this
> > patchset the probe function trace will look something like this:
> >
> > -> brcm_pcie_probe()
> > -> brcm_pcie_setup(); /* Set-up only */
> > -> pci_host_probe(bridge);
> > -> [enumeration]
> > -> pci_alloc_child_bus()
> > -> bus->ops->add_bus(bus); /* We've set this op */
> > -> brcm_pcie_add_bus() /* Our callback */
> > -> [turn on regulators] /* Main objective! */
> > -> brcm_pcie_start_link() /* Link-up */

Attachments:

2022-07-18 19:16:57

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> Currently, the function does the setup for establishing PCIe link-up
> with the downstream device, and it does the actual link-up as well.
> The calling sequence is (roughly) the following in the probe:
>
> -> brcm_pcie_probe()
> -> brcm_pcie_setup(); /* Set-up and link-up */
> -> pci_host_probe(bridge);
>
> This commit splits the setup function in two: brcm_pcie_setup(), which only
> does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> The reason why we are doing this is to lay a foundation for subsequent
> commits so that we can turn on any power regulators, as described in the
> root port's DT node, prior to doing link-up.

All drivers that care about power regulators turn them on before
link-up, but typically those regulators are described directly under
the host bridge itself.

IIUC the difference here is that you have regulators described under
Root Ports (not the host bridge/Root Complex itself), so you don't
know about them until you've enumerated the Root Ports.
brcm_pcie_probe() can't turn them on directly because it doesn't know
what Root Ports are present and doesn't know about regulators below
them.

So I think brcm_pcie_setup() does initialization that doesn't depend
on the link or any downstream devices, and brcm_pcie_start_link() does
things that depend on the link being up. Right?

If so, "start_link" might be a slight misnomer since AFAICT
brcm_pcie_start_link() doesn't do anything to initiate link-up except
maybe deasserting fundamental reset. Some drivers start the LTSSM or
explicitly enable link training, but brcm_pcie_start_link() doesn't
seem to do anything like that.

brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
that really depend on the link being up? If that only affects the
Root Port, maybe it could be done before link-up?

> We do this by defining an
> add_bus() callback which is invoked during enumeraion. At the end of this
> patchset the probe function trace will look something like this:
>
> -> brcm_pcie_probe()
> -> brcm_pcie_setup(); /* Set-up only */
> -> pci_host_probe(bridge);
> -> [enumeration]
> -> pci_alloc_child_bus()
> -> bus->ops->add_bus(bus); /* We've set this op */
> -> brcm_pcie_add_bus() /* Our callback */
> -> [turn on regulators] /* Main objective! */
> -> brcm_pcie_start_link() /* Link-up */

2022-07-18 19:29:26

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> >
> > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > Currently, the function does the setup for establishing PCIe link-up
> > > with the downstream device, and it does the actual link-up as well.
> > > The calling sequence is (roughly) the following in the probe:
> > >
> > > -> brcm_pcie_probe()
> > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > -> pci_host_probe(bridge);
> > >
> > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > The reason why we are doing this is to lay a foundation for subsequent
> > > commits so that we can turn on any power regulators, as described in the
> > > root port's DT node, prior to doing link-up.
> >
> > All drivers that care about power regulators turn them on before
> > link-up, but typically those regulators are described directly under
> > the host bridge itself.
>
> Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> The binding commit message said,
>
> "Quite similar to the regulator bindings found in
> "rockchip-pcie-host.txt", this allows optional regulators to be
> attached and controlled by the PCIe RC driver."
>
> > IIUC the difference here is that you have regulators described under
> > Root Ports (not the host bridge/Root Complex itself), so you don't
> > know about them until you've enumerated the Root Ports.
> > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > what Root Ports are present and doesn't know about regulators below
> > them.
>
> The reviewer's requested me to move the regulator node(s)
> elsewhere, and at some point later it was requested to be placed
> under the Root Port driver. I would love to return them under the
> host bridge, just say the word!

I'm not suggesting a change in that design; I'm only trying to
understand and clarify the commit log.

I looked briefly for the suggestion to put the regulators under the
Root Port instead of the host bridge, but didn't find it. I don't
know enough to have an opinion yet.

> > So I think brcm_pcie_setup() does initialization that doesn't depend
> > on the link or any downstream devices, and brcm_pcie_start_link() does
> > things that depend on the link being up. Right?
>
> Yes.
>
> > If so, "start_link" might be a slight misnomer since AFAICT
> > brcm_pcie_start_link() doesn't do anything to initiate link-up except
> > maybe deasserting fundamental reset. Some drivers start the LTSSM or
> > explicitly enable link training, but brcm_pcie_start_link() doesn't
> > seem to do anything like that.
> >
> > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
> > that really depend on the link being up? If that only affects the
> > Root Port, maybe it could be done before link-up?
>
> Some of the registers cannot be accessed until after linkup but these do
> not have that issue. I will change this.

Here's my attempt (assuming we don't change the DT regulator design):

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bd88a0a46c63..70cad1cbcbb4 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -852,14 +852,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
u64 rc_bar2_offset, rc_bar2_size;
void __iomem *base = pcie->base;
- struct device *dev = pcie->dev;
+ int ret, memc;
+ u32 tmp, burst, aspm_support;
struct resource_entry *entry;
- bool ssc_good = false;
struct resource *res;
int num_out_wins = 0;
- u16 nlw, cls, lnksta;
- int i, ret, memc;
- u32 tmp, burst, aspm_support;

/* Reset the bridge */
pcie->bridge_sw_init_set(pcie, 1);
@@ -948,25 +945,23 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
if (pcie->gen)
brcm_pcie_set_gen(pcie, pcie->gen);

- /* Unassert the fundamental reset */
- pcie->perst_set(pcie, 0);
+ /* Don't advertise L0s capability if 'aspm-no-l0s' */
+ aspm_support = PCIE_LINK_STATE_L1;
+ if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+ aspm_support |= PCIE_LINK_STATE_L0S;
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+ u32p_replace_bits(&tmp, aspm_support,
+ PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);

/*
- * Give the RC/EP time to wake up, before trying to configure RC.
- * Intermittently check status for link-up, up to a total of 100ms.
+ * For config space accesses on the RC, show the right class for
+ * a PCIe-PCIe bridge (the default setting is to be EP mode).
*/
- for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)
- msleep(5);
-
- if (!brcm_pcie_link_up(pcie)) {
- dev_err(dev, "link down\n");
- return -ENODEV;
- }
-
- if (!brcm_pcie_rc_mode(pcie)) {
- dev_err(dev, "PCIe misconfigured; is in EP mode\n");
- return -EINVAL;
- }
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+ u32p_replace_bits(&tmp, PCI_CLASS_BRIDGE_PCI_NORMAL,
+ PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);

resource_list_for_each_entry(entry, &bridge->windows) {
res = entry->res;
@@ -998,23 +993,37 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
num_out_wins++;
}

- /* Don't advertise L0s capability if 'aspm-no-l0s' */
- aspm_support = PCIE_LINK_STATE_L1;
- if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
- aspm_support |= PCIE_LINK_STATE_L0S;
- tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
- u32p_replace_bits(&tmp, aspm_support,
- PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
- writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+ return 0;
+}
+
+static int brcm_pcie_start_link(struct brcm_pcie *pcie)
+{
+ struct device *dev = pcie->dev;
+ void __iomem *base = pcie->base;
+ u16 nlw, cls, lnksta;
+ bool ssc_good = false;
+ u32 tmp;
+ int ret, i;
+
+ /* Unassert the fundamental reset */
+ pcie->perst_set(pcie, 0);

/*
- * For config space accesses on the RC, show the right class for
- * a PCIe-PCIe bridge (the default setting is to be EP mode).
+ * Give the RC/EP time to wake up, before trying to configure RC.
+ * Intermittently check status for link-up, up to a total of 100ms.
*/
- tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
- u32p_replace_bits(&tmp, 0x060400,
- PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
- writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+ for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)
+ msleep(5);
+
+ if (!brcm_pcie_link_up(pcie)) {
+ dev_err(dev, "link down\n");
+ return -ENODEV;
+ }
+
+ if (!brcm_pcie_rc_mode(pcie)) {
+ dev_err(dev, "PCIe misconfigured; is in EP mode\n");
+ return -EINVAL;
+ }

if (pcie->ssc) {
ret = brcm_pcie_set_ssc(pcie);
@@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
goto err_reset;

+ ret = brcm_pcie_start_link(pcie);
+ if (ret)
+ goto err_reset;
+
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);

@@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
goto fail;

+ ret = brcm_pcie_start_link(pcie);
+ if (ret)
+ goto fail;
+
pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");

2022-07-18 22:43:01

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > Currently, the function does the setup for establishing PCIe link-up
> > > with the downstream device, and it does the actual link-up as well.
> > > The calling sequence is (roughly) the following in the probe:
> > >
> > > -> brcm_pcie_probe()
> > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > -> pci_host_probe(bridge);
> > >
> > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > The reason why we are doing this is to lay a foundation for subsequent
> > > commits so that we can turn on any power regulators, as described in the
> > > root port's DT node, prior to doing link-up.
> >
> > All drivers that care about power regulators turn them on before
> > link-up, but typically those regulators are described directly under
> > the host bridge itself.
>
> Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> The binding commit message said,
>
> "Quite similar to the regulator bindings found in
> "rockchip-pcie-host.txt", this allows optional regulators to be
> attached and controlled by the PCIe RC driver."
>
> > IIUC the difference here is that you have regulators described under
> > Root Ports (not the host bridge/Root Complex itself), so you don't
> > know about them until you've enumerated the Root Ports.
> > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > what Root Ports are present and doesn't know about regulators below
> > them.
>
> The reviewer's requested me to move the regulator node(s)
> elsewhere, and at some point later it was requested to be placed
> under the Root Port driver. I would love to return them under the
> host bridge, just say the word!

Actually, I think my understanding is wrong. Even though the PCI core
hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
about it and should be able to look up the regulators and turn them
on.

Can you dig up the previous discussion about why the regulators need
to be under the Root Port and why they can't be turned on before
calling pci_host_probe()?

Bjorn

2022-07-18 22:43:25

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote:
> ...

> So I think brcm_pcie_setup() does initialization that doesn't depend
> on the link or any downstream devices, and brcm_pcie_start_link() does
> things that depend on the link being up. Right?
>
> If so, "start_link" might be a slight misnomer since AFAICT
> brcm_pcie_start_link() doesn't do anything to initiate link-up except
> maybe deasserting fundamental reset. Some drivers start the LTSSM or
> explicitly enable link training, but brcm_pcie_start_link() doesn't
> seem to do anything like that.
>
> brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
> that really depend on the link being up? If that only affects the
> Root Port, maybe it could be done before link-up?

What about the /* PCIe->SCB endian mode for BAR */ thing? Does that
depend on the link being up?

And the "Refclk from RC should be gated with CLKREQ#" part? Does that
depend on the link being up?

It seems obvious that brcm_pcie_set_ssc() and reading the negotiated
link speed and width depend on the link being up.

2022-07-19 13:59:25

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> > On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > > Currently, the function does the setup for establishing PCIe link-up
> > > > with the downstream device, and it does the actual link-up as well.
> > > > The calling sequence is (roughly) the following in the probe:
> > > >
> > > > -> brcm_pcie_probe()
> > > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > > -> pci_host_probe(bridge);
> > > >
> > > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > > The reason why we are doing this is to lay a foundation for subsequent
> > > > commits so that we can turn on any power regulators, as described in the
> > > > root port's DT node, prior to doing link-up.
> > >
> > > All drivers that care about power regulators turn them on before
> > > link-up, but typically those regulators are described directly under
> > > the host bridge itself.
> >
> > Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> > The binding commit message said,
> >
> > "Quite similar to the regulator bindings found in
> > "rockchip-pcie-host.txt", this allows optional regulators to be
> > attached and controlled by the PCIe RC driver."
> >
> > > IIUC the difference here is that you have regulators described under
> > > Root Ports (not the host bridge/Root Complex itself), so you don't
> > > know about them until you've enumerated the Root Ports.
> > > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > > what Root Ports are present and doesn't know about regulators below
> > > them.
> >
> > The reviewer's requested me to move the regulator node(s)
> > elsewhere, and at some point later it was requested to be placed
> > under the Root Port driver. I would love to return them under the
> > host bridge, just say the word!
>
> Actually, I think my understanding is wrong. Even though the PCI core
> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
> about it and should be able to look up the regulators and turn them
> on.

One can do this with
regulator_bulk_get(NULL, ...);

However, MarkB did not like the idea of a driver getting the regulator from
the global DT namespace [1].

For the RC driver, one cannot invoke regulator_bulk_get(dev, ...) if
there is not a
direct child regulator node. One needs to use the Port driver device.
The Port driver
device does not exist at this point unless one tries to prematurely create it;
I tried this and it was a mess to say the least.

>
> Can you dig up the previous discussion about why the regulators need
> to be under the Root Port and why they can't be turned on before
> calling pci_host_probe()?

RobH did not want the regulators to be under the RC as he said their DT
location should resemble the HW [2]. The consensus evolved
to place it under the port driver, which can provide a general
mechanism for turning
on regulators anywhere in the PCIe tree.

Regards,
Jim Quinlan
Broadcom STB

[1] https://lore.kernel.org/linux-pci/[email protected]/
[2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/

>
>
> Bjorn

Attachments:

2022-07-19 20:10:17

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
> > On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> > > On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > > > Currently, the function does the setup for establishing PCIe link-up
> > > > > with the downstream device, and it does the actual link-up as well.
> > > > > The calling sequence is (roughly) the following in the probe:
> > > > >
> > > > > -> brcm_pcie_probe()
> > > > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > > > -> pci_host_probe(bridge);
> > > > >
> > > > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > > > The reason why we are doing this is to lay a foundation for subsequent
> > > > > commits so that we can turn on any power regulators, as described in the
> > > > > root port's DT node, prior to doing link-up.
> > > >
> > > > All drivers that care about power regulators turn them on before
> > > > link-up, but typically those regulators are described directly under
> > > > the host bridge itself.
> > >
> > > Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> > > The binding commit message said,
> > >
> > > "Quite similar to the regulator bindings found in
> > > "rockchip-pcie-host.txt", this allows optional regulators to be
> > > attached and controlled by the PCIe RC driver."
> > >
> > > > IIUC the difference here is that you have regulators described under
> > > > Root Ports (not the host bridge/Root Complex itself), so you don't
> > > > know about them until you've enumerated the Root Ports.
> > > > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > > > what Root Ports are present and doesn't know about regulators below
> > > > them.
> > >
> > > The reviewer's requested me to move the regulator node(s)
> > > elsewhere, and at some point later it was requested to be placed
> > > under the Root Port driver. I would love to return them under the
> > > host bridge, just say the word!
> >
> > Actually, I think my understanding is wrong. Even though the PCI core
> > hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
> > about it and should be able to look up the regulators and turn them
> > on.
>
> One can do this with
> regulator_bulk_get(NULL, ...);
>
> However, MarkB did not like the idea of a driver getting the
> regulator from the global DT namespace [1].
>
> For the RC driver, one cannot invoke regulator_bulk_get(dev, ...)
> if there is not a direct child regulator node. One needs to use the
> Port driver device. The Port driver device does not exist at this
> point unless one tries to prematurely create it; I tried this and it
> was a mess to say the least.
>
> > Can you dig up the previous discussion about why the regulators need
> > to be under the Root Port and why they can't be turned on before
> > calling pci_host_probe()?
>
> RobH did not want the regulators to be under the RC as he said their
> DT location should resemble the HW [2]. The consensus evolved to
> place it under the port driver, which can provide a general
> mechanism for turning on regulators anywhere in the PCIe tree.

I don't want to redesign this whole thing. I just want a crisp
rationale for the commit log.

All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
tegra, rockchip-host) have regulators for downstream PCIe power
directly under the RC. If putting the regulators under an RP instead
is the direction of the future, I guess that might be OK, and I guess
the reasons are:

1) Slot or device power regulators that are logically below the RP
should be described that way in the DT.

2) Associating regulators with a RP allows the possibility of
selectively controlling power to slots/devices below the RP,
e.g., to power down devices below RP A when suspending while
leaving wakeup devices below RP B powered up.

I think in your case the motivating reason is 2).

Your commit log for "Add mechanism to turn on subdev regulators"
suggests that you want some user control of endpoint power, e.g., via
sysfs, but I don't see that implemented yet except possibly via a
"remove" file that would unbind the driver and remove the entire
device.

> [1] https://lore.kernel.org/linux-pci/[email protected]/
> [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/

2022-07-20 15:00:43

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
> > On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> > > > On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > > > > Currently, the function does the setup for establishing PCIe link-up
> > > > > > with the downstream device, and it does the actual link-up as well.
> > > > > > The calling sequence is (roughly) the following in the probe:
> > > > > >
> > > > > > -> brcm_pcie_probe()
> > > > > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > > > > -> pci_host_probe(bridge);
> > > > > >
> > > > > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > > > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > > > > The reason why we are doing this is to lay a foundation for subsequent
> > > > > > commits so that we can turn on any power regulators, as described in the
> > > > > > root port's DT node, prior to doing link-up.
> > > > >
> > > > > All drivers that care about power regulators turn them on before
> > > > > link-up, but typically those regulators are described directly under
> > > > > the host bridge itself.
> > > >
> > > > Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> > > > The binding commit message said,
> > > >
> > > > "Quite similar to the regulator bindings found in
> > > > "rockchip-pcie-host.txt", this allows optional regulators to be
> > > > attached and controlled by the PCIe RC driver."
> > > >
> > > > > IIUC the difference here is that you have regulators described under
> > > > > Root Ports (not the host bridge/Root Complex itself), so you don't
> > > > > know about them until you've enumerated the Root Ports.
> > > > > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > > > > what Root Ports are present and doesn't know about regulators below
> > > > > them.
> > > >
> > > > The reviewer's requested me to move the regulator node(s)
> > > > elsewhere, and at some point later it was requested to be placed
> > > > under the Root Port driver. I would love to return them under the
> > > > host bridge, just say the word!
> > >
> > > Actually, I think my understanding is wrong. Even though the PCI core
> > > hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
> > > about it and should be able to look up the regulators and turn them
> > > on.
> >
> > One can do this with
> > regulator_bulk_get(NULL, ...);
> >
> > However, MarkB did not like the idea of a driver getting the
> > regulator from the global DT namespace [1].
> >
> > For the RC driver, one cannot invoke regulator_bulk_get(dev, ...)
> > if there is not a direct child regulator node. One needs to use the
> > Port driver device. The Port driver device does not exist at this
> > point unless one tries to prematurely create it; I tried this and it
> > was a mess to say the least.
> >
> > > Can you dig up the previous discussion about why the regulators need
> > > to be under the Root Port and why they can't be turned on before
> > > calling pci_host_probe()?
> >
> > RobH did not want the regulators to be under the RC as he said their
> > DT location should resemble the HW [2]. The consensus evolved to
> > place it under the port driver, which can provide a general
> > mechanism for turning on regulators anywhere in the PCIe tree.
>
> I don't want to redesign this whole thing. I just want a crisp
> rationale for the commit log.
>
> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
> tegra, rockchip-host) have regulators for downstream PCIe power
> directly under the RC. If putting the regulators under an RP instead
> is the direction of the future, I guess that might be OK, and I guess
> the reasons are:
>
> 1) Slot or device power regulators that are logically below the RP
> should be described that way in the DT.
>
> 2) Associating regulators with a RP allows the possibility of
> selectively controlling power to slots/devices below the RP,
> e.g., to power down devices below RP A when suspending while
> leaving wakeup devices below RP B powered up.
>
> I think in your case the motivating reason is 2).
>
> Your commit log for "Add mechanism to turn on subdev regulators"
> suggests that you want some user control of endpoint power, e.g., via
> sysfs, but I don't see that implemented yet except possibly via a
> "remove" file that would unbind the driver and remove the entire
> device.
Hi Bjorn,

Initially we wanted to (a) turn on any regulator found under the RC
node and (b) allow the possibility of the regulator to control the
EP's power. From the feedback, we realized early on that neither of
these were going to fly, so we conceded both requests and just wanted
to turn on standard PCIe regulators. Upon reading the aforementioned
commit message I realize that there are a couple of leftover sentences
from these early days that must be removed.

I think when I submitted v1 of the original series that only the
rockchip driver had regulators under the RC. And my recollection was
that this was accepted but there was apprehension of this turning into
the "standard" way of turning on such regulators, as the location of
the regulator nodes was in question.

In short, I would be quite content to follow the existing examples.

Regards,
Jim Quinlan
Broadcom STB

>
> > [1] https://lore.kernel.org/linux-pci/[email protected]/
> > [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/

Attachments:

2022-07-20 16:25:18

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <[email protected]> wrote:
>
> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <[email protected]> wrote:
> >
> > On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
> > > On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> > > > > On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > > On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> > > > > > > Currently, the function does the setup for establishing PCIe link-up
> > > > > > > with the downstream device, and it does the actual link-up as well.
> > > > > > > The calling sequence is (roughly) the following in the probe:
> > > > > > >
> > > > > > > -> brcm_pcie_probe()
> > > > > > > -> brcm_pcie_setup(); /* Set-up and link-up */
> > > > > > > -> pci_host_probe(bridge);
> > > > > > >
> > > > > > > This commit splits the setup function in two: brcm_pcie_setup(), which only
> > > > > > > does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> > > > > > > The reason why we are doing this is to lay a foundation for subsequent
> > > > > > > commits so that we can turn on any power regulators, as described in the
> > > > > > > root port's DT node, prior to doing link-up.
> > > > > >
> > > > > > All drivers that care about power regulators turn them on before
> > > > > > link-up, but typically those regulators are described directly under
> > > > > > the host bridge itself.
> > > > >
> > > > > Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> > > > > The binding commit message said,
> > > > >
> > > > > "Quite similar to the regulator bindings found in
> > > > > "rockchip-pcie-host.txt", this allows optional regulators to be
> > > > > attached and controlled by the PCIe RC driver."
> > > > >
> > > > > > IIUC the difference here is that you have regulators described under
> > > > > > Root Ports (not the host bridge/Root Complex itself), so you don't
> > > > > > know about them until you've enumerated the Root Ports.
> > > > > > brcm_pcie_probe() can't turn them on directly because it doesn't know
> > > > > > what Root Ports are present and doesn't know about regulators below
> > > > > > them.
> > > > >
> > > > > The reviewer's requested me to move the regulator node(s)
> > > > > elsewhere, and at some point later it was requested to be placed
> > > > > under the Root Port driver. I would love to return them under the
> > > > > host bridge, just say the word!
> > > >
> > > > Actually, I think my understanding is wrong. Even though the PCI core
> > > > hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
> > > > about it and should be able to look up the regulators and turn them
> > > > on.
> > >
> > > One can do this with
> > > regulator_bulk_get(NULL, ...);
> > >
> > > However, MarkB did not like the idea of a driver getting the
> > > regulator from the global DT namespace [1].
> > >
> > > For the RC driver, one cannot invoke regulator_bulk_get(dev, ...)
> > > if there is not a direct child regulator node. One needs to use the
> > > Port driver device. The Port driver device does not exist at this
> > > point unless one tries to prematurely create it; I tried this and it
> > > was a mess to say the least.
> > >
> > > > Can you dig up the previous discussion about why the regulators need
> > > > to be under the Root Port and why they can't be turned on before
> > > > calling pci_host_probe()?
> > >
> > > RobH did not want the regulators to be under the RC as he said their
> > > DT location should resemble the HW [2]. The consensus evolved to
> > > place it under the port driver, which can provide a general
> > > mechanism for turning on regulators anywhere in the PCIe tree.
> >
> > I don't want to redesign this whole thing. I just want a crisp
> > rationale for the commit log.
> >
> > All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
> > tegra, rockchip-host) have regulators for downstream PCIe power
> > directly under the RC. If putting the regulators under an RP instead
> > is the direction of the future, I guess that might be OK, and I guess
> > the reasons are:
> >
> > 1) Slot or device power regulators that are logically below the RP
> > should be described that way in the DT.
> >
> > 2) Associating regulators with a RP allows the possibility of
> > selectively controlling power to slots/devices below the RP,
> > e.g., to power down devices below RP A when suspending while
> > leaving wakeup devices below RP B powered up.
> >
> > I think in your case the motivating reason is 2).
> >
> > Your commit log for "Add mechanism to turn on subdev regulators"
> > suggests that you want some user control of endpoint power, e.g., via
> > sysfs, but I don't see that implemented yet except possibly via a
> > "remove" file that would unbind the driver and remove the entire
> > device.
> Hi Bjorn,
>
> Initially we wanted to (a) turn on any regulator found under the RC
> node and (b) allow the possibility of the regulator to control the
> EP's power. From the feedback, we realized early on that neither of
> these were going to fly, so we conceded both requests and just wanted
> to turn on standard PCIe regulators. Upon reading the aforementioned
> commit message I realize that there are a couple of leftover sentences
> from these early days that must be removed.
>
> I think when I submitted v1 of the original series that only the
> rockchip driver had regulators under the RC. And my recollection was
> that this was accepted but there was apprehension of this turning into
> the "standard" way of turning on such regulators, as the location of
> the regulator nodes was in question.
>
> In short, I would be quite content to follow the existing examples.

The existing examples are, in general, incomplete and only work for
the simplest cases.

There's 2 cases to consider here. The first is standard slots with
standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is
either non-standard slots or just soldered down devices which could
have any number of device specific resources. In the latter case,
those resources need to go into the node for the device. For the
former case (which we are discussing here), putting the resources in
the upstream (side of the link) node is fine. That's the root port
node(s) or switch port nodes. However, since most host bridges are a
single RP and don't put the RP node in DT, we've ended up with these
properties in host bridge nodes. That's fine as long as it's a single
RP and device. When it is not, we need to do something different. The
only way this scales is putting resources in the port nodes as those
are what have a 1:1 relationship to slots. If that's supported, then
the simple cases are also easily supported with if the resources are
not found in the port node/device, then look for them in the parent
node. That's also the path for how we get the handling of PERST out of
every host bridge driver.

Rob

2022-07-20 20:40:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote:
> > ...
>
> > So I think brcm_pcie_setup() does initialization that doesn't depend
> > on the link or any downstream devices, and brcm_pcie_start_link() does
> > things that depend on the link being up. Right?
> >
> > If so, "start_link" might be a slight misnomer since AFAICT
> > brcm_pcie_start_link() doesn't do anything to initiate link-up except
> > maybe deasserting fundamental reset. Some drivers start the LTSSM or
> > explicitly enable link training, but brcm_pcie_start_link() doesn't
> > seem to do anything like that.
> >
> > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
> > that really depend on the link being up? If that only affects the
> > Root Port, maybe it could be done before link-up?
>
> What about the /* PCIe->SCB endian mode for BAR */ thing? Does that
> depend on the link being up?
>
> And the "Refclk from RC should be gated with CLKREQ#" part? Does that
> depend on the link being up?
>
> It seems obvious that brcm_pcie_set_ssc() and reading the negotiated
> link speed and width depend on the link being up.

Can we close on this? Splitting into

(a) stuff that can be initialized before the link is available and
(b) stuff that depends on the link

makes good sense, but then (b) should only contain stuff that actually
depends on the link.

The "PCIe->SCB endian mode for BAR" *sounds* like something related to
the primary side of the RP, not the link.

Not sure about "Refclk from RC". RC would certainly be primary side,
but ASPM has to do with secondary (link) side.

Bjorn

2022-07-20 21:44:45

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On 7/20/22 09:18, Rob Herring wrote:
> On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <[email protected]> wrote:
>>
>> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <[email protected]> wrote:
>>>
>>> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
>>>> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
>>>>> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
>>>>>> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
>>>>>>> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
>>>>>>>> Currently, the function does the setup for establishing PCIe link-up
>>>>>>>> with the downstream device, and it does the actual link-up as well.
>>>>>>>> The calling sequence is (roughly) the following in the probe:
>>>>>>>>
>>>>>>>> -> brcm_pcie_probe()
>>>>>>>> -> brcm_pcie_setup(); /* Set-up and link-up */
>>>>>>>> -> pci_host_probe(bridge);
>>>>>>>>
>>>>>>>> This commit splits the setup function in two: brcm_pcie_setup(), which only
>>>>>>>> does the set-up, and brcm_pcie_start_link(), which only does the link-up.
>>>>>>>> The reason why we are doing this is to lay a foundation for subsequent
>>>>>>>> commits so that we can turn on any power regulators, as described in the
>>>>>>>> root port's DT node, prior to doing link-up.
>>>>>>>
>>>>>>> All drivers that care about power regulators turn them on before
>>>>>>> link-up, but typically those regulators are described directly under
>>>>>>> the host bridge itself.
>>>>>>
>>>>>> Actually, what you describe is what I proposed with my v1 back in Nov 2020.
>>>>>> The binding commit message said,
>>>>>>
>>>>>> "Quite similar to the regulator bindings found in
>>>>>> "rockchip-pcie-host.txt", this allows optional regulators to be
>>>>>> attached and controlled by the PCIe RC driver."
>>>>>>
>>>>>>> IIUC the difference here is that you have regulators described under
>>>>>>> Root Ports (not the host bridge/Root Complex itself), so you don't
>>>>>>> know about them until you've enumerated the Root Ports.
>>>>>>> brcm_pcie_probe() can't turn them on directly because it doesn't know
>>>>>>> what Root Ports are present and doesn't know about regulators below
>>>>>>> them.
>>>>>>
>>>>>> The reviewer's requested me to move the regulator node(s)
>>>>>> elsewhere, and at some point later it was requested to be placed
>>>>>> under the Root Port driver. I would love to return them under the
>>>>>> host bridge, just say the word!
>>>>>
>>>>> Actually, I think my understanding is wrong. Even though the PCI core
>>>>> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
>>>>> about it and should be able to look up the regulators and turn them
>>>>> on.
>>>>
>>>> One can do this with
>>>> regulator_bulk_get(NULL, ...);
>>>>
>>>> However, MarkB did not like the idea of a driver getting the
>>>> regulator from the global DT namespace [1].
>>>>
>>>> For the RC driver, one cannot invoke regulator_bulk_get(dev, ...)
>>>> if there is not a direct child regulator node. One needs to use the
>>>> Port driver device. The Port driver device does not exist at this
>>>> point unless one tries to prematurely create it; I tried this and it
>>>> was a mess to say the least.
>>>>
>>>>> Can you dig up the previous discussion about why the regulators need
>>>>> to be under the Root Port and why they can't be turned on before
>>>>> calling pci_host_probe()?
>>>>
>>>> RobH did not want the regulators to be under the RC as he said their
>>>> DT location should resemble the HW [2]. The consensus evolved to
>>>> place it under the port driver, which can provide a general
>>>> mechanism for turning on regulators anywhere in the PCIe tree.
>>>
>>> I don't want to redesign this whole thing. I just want a crisp
>>> rationale for the commit log.
>>>
>>> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
>>> tegra, rockchip-host) have regulators for downstream PCIe power
>>> directly under the RC. If putting the regulators under an RP instead
>>> is the direction of the future, I guess that might be OK, and I guess
>>> the reasons are:
>>>
>>> 1) Slot or device power regulators that are logically below the RP
>>> should be described that way in the DT.
>>>
>>> 2) Associating regulators with a RP allows the possibility of
>>> selectively controlling power to slots/devices below the RP,
>>> e.g., to power down devices below RP A when suspending while
>>> leaving wakeup devices below RP B powered up.
>>>
>>> I think in your case the motivating reason is 2).
>>>
>>> Your commit log for "Add mechanism to turn on subdev regulators"
>>> suggests that you want some user control of endpoint power, e.g., via
>>> sysfs, but I don't see that implemented yet except possibly via a
>>> "remove" file that would unbind the driver and remove the entire
>>> device.
>> Hi Bjorn,
>>
>> Initially we wanted to (a) turn on any regulator found under the RC
>> node and (b) allow the possibility of the regulator to control the
>> EP's power. From the feedback, we realized early on that neither of
>> these were going to fly, so we conceded both requests and just wanted
>> to turn on standard PCIe regulators. Upon reading the aforementioned
>> commit message I realize that there are a couple of leftover sentences
>> from these early days that must be removed.
>>
>> I think when I submitted v1 of the original series that only the
>> rockchip driver had regulators under the RC. And my recollection was
>> that this was accepted but there was apprehension of this turning into
>> the "standard" way of turning on such regulators, as the location of
>> the regulator nodes was in question.
>>
>> In short, I would be quite content to follow the existing examples.
>
> The existing examples are, in general, incomplete and only work for
> the simplest cases.
>
> There's 2 cases to consider here. The first is standard slots with
> standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is
> either non-standard slots or just soldered down devices which could
> have any number of device specific resources. In the latter case,
> those resources need to go into the node for the device. For the
> former case (which we are discussing here), putting the resources in
> the upstream (side of the link) node is fine. That's the root port
> node(s) or switch port nodes. However, since most host bridges are a
> single RP and don't put the RP node in DT, we've ended up with these
> properties in host bridge nodes. That's fine as long as it's a single
> RP and device. When it is not, we need to do something different. The
> only way this scales is putting resources in the port nodes as those
> are what have a 1:1 relationship to slots. If that's supported, then
> the simple cases are also easily supported with if the resources are
> not found in the port node/device, then look for them in the parent
> node. That's also the path for how we get the handling of PERST out of
> every host bridge driver.

This has me confused now, are you suggesting that we pursue what Jim has put together here as a series which describes the regulators in the PCIe end-point device DT node, or that given that we have a single RC single RP configuration it is somewhat acceptable to describe regulators in the PCIe RC node?
--
Florian

2022-07-20 22:28:15

[permalink] [raw]

Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts

On Sat, Jul 16, 2022 at 06:24:50PM -0400, Jim Quinlan wrote:
> Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
> established between itself and downstream, any subsequent config space
> access causes a CPU abort. This commit sets a "refusal mode" if the PCIe
> link-up fails, and this has our pci_ops map_bus function returning a NULL
> address, which in turn precludes the access from happening.
>
> Right now, "refusal mode" is window dressing. It will become relevant
> in a future commit when brcm_pcie_start_link() is invoked during
> enumeration instead of before it.
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c026446d5830..72219a4f3964 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -255,6 +255,7 @@ struct brcm_pcie {
> u32 hw_rev;
> void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> + bool refusal_mode;
> };
>
> static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
> if (pci_is_root_bus(bus))
> return PCI_SLOT(devfn) ? NULL : base + where;
>
> + if (pcie->refusal_mode) {
> + /*
> + * At this point we do not have PCIe link-up. If there is
> + * a config read or write access besides those targeting
> + * the host bridge, our PCIe HW throws a CPU abort. To
> + * prevent this we return the NULL address. The calling
> + * functions -- pci_generic_config_*() -- will notice this
> + * and not perform the access, and if it is a read access,
> + * 0xffffffff is returned.
> + */
> + return NULL;
> + }

Is this any different from all the other .map_bus() implementations
that return NULL when the link is down?

cdns_pci_map_bus()
dw_pcie_other_conf_map_bus()
nwl_pcie_map_bus() (see nwl_pcie_valid_device())
xilinx_pcie_map_bus() (see xilinx_pcie_valid_device())

If you can implement this the same way, i.e., using
brcm_pcie_link_up(), it would be nice.

> /* For devices, write to the config space index register */
> idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
> writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
> @@ -704,6 +718,11 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
> if (pci_is_root_bus(bus))
> return PCI_SLOT(devfn) ? NULL : base + (where & ~0x3);
>
> + if (pcie->refusal_mode) {
> + /* See note above in brcm_pcie_map_conf() */
> + return NULL;
> + }
> +
> /* For devices, write to the config space index register */
> idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & ~3));
> writel(idx, base + IDX_ADDR(pcie));
> @@ -989,6 +1008,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> dev_err(dev, "link down\n");
> return -ENODEV;
> }
> + pcie->refusal_mode = false;
>
> if (!brcm_pcie_rc_mode(pcie)) {
> dev_err(dev, "PCIe misconfigured; is in EP mode\n");
> @@ -1134,6 +1154,8 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> void __iomem *base = pcie->base;
> int tmp;
>
> + pcie->refusal_mode = true;
> +
> if (brcm_pcie_link_up(pcie))
> brcm_pcie_enter_l23(pcie);
> /* Assert fundamental reset */
> @@ -1185,6 +1207,7 @@ static int brcm_pcie_resume(struct device *dev)
> u32 tmp;
> int ret;
>
> + pcie->refusal_mode = true;
> base = pcie->base;
> ret = clk_prepare_enable(pcie->clk);
> if (ret)
> @@ -1361,6 +1384,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> pcie->type = data->type;
> pcie->perst_set = data->perst_set;
> pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> + pcie->refusal_mode = true;
>
> pcie->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(pcie->base))
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2022-07-20 22:29:24

[permalink] [raw]

Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts

2022-07-21 15:09:55

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Wed, Jul 20, 2022 at 4:37 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote:
> > > ...
> >
> > > So I think brcm_pcie_setup() does initialization that doesn't depend
> > > on the link or any downstream devices, and brcm_pcie_start_link() does
> > > things that depend on the link being up. Right?
> > >
> > > If so, "start_link" might be a slight misnomer since AFAICT
> > > brcm_pcie_start_link() doesn't do anything to initiate link-up except
> > > maybe deasserting fundamental reset. Some drivers start the LTSSM or
> > > explicitly enable link training, but brcm_pcie_start_link() doesn't
> > > seem to do anything like that.
> > >
> > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does
> > > that really depend on the link being up? If that only affects the
> > > Root Port, maybe it could be done before link-up?
> >
> > What about the /* PCIe->SCB endian mode for BAR */ thing? Does that
> > depend on the link being up?
> >
> > And the "Refclk from RC should be gated with CLKREQ#" part? Does that
> > depend on the link being up?
> >
> > It seems obvious that brcm_pcie_set_ssc() and reading the negotiated
> > link speed and width depend on the link being up.
>
> Can we close on this? Splitting into
Absolutely.

>
> (a) stuff that can be initialized before the link is available and
> (b) stuff that depends on the link
>
> makes good sense, but then (b) should only contain stuff that actually
> depends on the link.
>
> The "PCIe->SCB endian mode for BAR" *sounds* like something related to
> the primary side of the RP, not the link.
>
> Not sure about "Refclk from RC". RC would certainly be primary side,
> but ASPM has to do with secondary (link) side.

I get the feedback, submission coming soon -- I was waiting for the
email thread to conclude.

Regards,
Jim Quinlan
Broadcom STB

>
> Bjorn

Attachments:

2022-07-21 15:27:10

[permalink] [raw]

Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts

https://lore.kernel.org/linux-pci/[email protected]/
On Wed, Jul 20, 2022 at 6:06 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Sat, Jul 16, 2022 at 06:24:50PM -0400, Jim Quinlan wrote:
> > Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
> > established between itself and downstream, any subsequent config space
> > access causes a CPU abort. This commit sets a "refusal mode" if the PCIe
> > link-up fails, and this has our pci_ops map_bus function returning a NULL
> > address, which in turn precludes the access from happening.
> >
> > Right now, "refusal mode" is window dressing. It will become relevant
> > in a future commit when brcm_pcie_start_link() is invoked during
> > enumeration instead of before it.
> >
> > Signed-off-by: Jim Quinlan <[email protected]>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index c026446d5830..72219a4f3964 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -255,6 +255,7 @@ struct brcm_pcie {
> > u32 hw_rev;
> > void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > + bool refusal_mode;
> > };
> >
> > static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
> > if (pci_is_root_bus(bus))
> > return PCI_SLOT(devfn) ? NULL : base + where;
> >
> > + if (pcie->refusal_mode) {
> > + /*
> > + * At this point we do not have PCIe link-up. If there is
> > + * a config read or write access besides those targeting
> > + * the host bridge, our PCIe HW throws a CPU abort. To
> > + * prevent this we return the NULL address. The calling
> > + * functions -- pci_generic_config_*() -- will notice this
> > + * and not perform the access, and if it is a read access,
> > + * 0xffffffff is returned.
> > + */
> > + return NULL;
> > + }
>
> Is this any different from all the other .map_bus() implementations
> that return NULL when the link is down?

Not really,,but long ago I submitted code that gated the config spec
access based on link status and was advised not to do it [1].
I'll be happy to make it look like the others.

Regards,
Jim Quinlan
Broadcom STB

[1] https://lore.kernel.org/linux-pci/[email protected]/

>
> cdns_pci_map_bus()
> dw_pcie_other_conf_map_bus()
> nwl_pcie_map_bus() (see nwl_pcie_valid_device())
> xilinx_pcie_map_bus() (see xilinx_pcie_valid_device())
>
> If you can implement this the same way, i.e., using
> brcm_pcie_link_up(), it would be nice.
>
> > /* For devices, write to the config space index register */
> > idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
> > writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
> > @@ -704,6 +718,11 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
> > if (pci_is_root_bus(bus))
> > return PCI_SLOT(devfn) ? NULL : base + (where & ~0x3);
> >
> > + if (pcie->refusal_mode) {
> > + /* See note above in brcm_pcie_map_conf() */
> > + return NULL;
> > + }
> > +
> > /* For devices, write to the config space index register */
> > idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & ~3));
> > writel(idx, base + IDX_ADDR(pcie));
> > @@ -989,6 +1008,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> > dev_err(dev, "link down\n");
> > return -ENODEV;
> > }
> > + pcie->refusal_mode = false;
> >
> > if (!brcm_pcie_rc_mode(pcie)) {
> > dev_err(dev, "PCIe misconfigured; is in EP mode\n");
> > @@ -1134,6 +1154,8 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > void __iomem *base = pcie->base;
> > int tmp;
> >
> > + pcie->refusal_mode = true;
> > +
> > if (brcm_pcie_link_up(pcie))
> > brcm_pcie_enter_l23(pcie);
> > /* Assert fundamental reset */
> > @@ -1185,6 +1207,7 @@ static int brcm_pcie_resume(struct device *dev)
> > u32 tmp;
> > int ret;
> >
> > + pcie->refusal_mode = true;
> > base = pcie->base;
> > ret = clk_prepare_enable(pcie->clk);
> > if (ret)
> > @@ -1361,6 +1384,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > pcie->type = data->type;
> > pcie->perst_set = data->perst_set;
> > pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> > + pcie->refusal_mode = true;
> >
> > pcie->base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(pcie->base))
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Attachments:

2022-07-21 15:30:49

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

On Wed, Jul 20, 2022 at 02:34:06PM -0700, Florian Fainelli wrote:
> On 7/20/22 09:18, Rob Herring wrote:
> > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <[email protected]> wrote:
> >>
> >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <[email protected]> wrote:
> >>>
> >>> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
> >>>> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <[email protected]> wrote:
> >>>>> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
> >>>>>> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <[email protected]> wrote:
> >>>>>>> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
> >>>>>>>> Currently, the function does the setup for establishing PCIe link-up
> >>>>>>>> with the downstream device, and it does the actual link-up as well.
> >>>>>>>> The calling sequence is (roughly) the following in the probe:
> >>>>>>>>
> >>>>>>>> -> brcm_pcie_probe()
> >>>>>>>> -> brcm_pcie_setup(); /* Set-up and link-up */
> >>>>>>>> -> pci_host_probe(bridge);
> >>>>>>>>
> >>>>>>>> This commit splits the setup function in two: brcm_pcie_setup(), which only
> >>>>>>>> does the set-up, and brcm_pcie_start_link(), which only does the link-up.
> >>>>>>>> The reason why we are doing this is to lay a foundation for subsequent
> >>>>>>>> commits so that we can turn on any power regulators, as described in the
> >>>>>>>> root port's DT node, prior to doing link-up.
> >>>>>>>
> >>>>>>> All drivers that care about power regulators turn them on before
> >>>>>>> link-up, but typically those regulators are described directly under
> >>>>>>> the host bridge itself.
> >>>>>>
> >>>>>> Actually, what you describe is what I proposed with my v1 back in Nov 2020.
> >>>>>> The binding commit message said,
> >>>>>>
> >>>>>> "Quite similar to the regulator bindings found in
> >>>>>> "rockchip-pcie-host.txt", this allows optional regulators to be
> >>>>>> attached and controlled by the PCIe RC driver."
> >>>>>>
> >>>>>>> IIUC the difference here is that you have regulators described under
> >>>>>>> Root Ports (not the host bridge/Root Complex itself), so you don't
> >>>>>>> know about them until you've enumerated the Root Ports.
> >>>>>>> brcm_pcie_probe() can't turn them on directly because it doesn't know
> >>>>>>> what Root Ports are present and doesn't know about regulators below
> >>>>>>> them.
> >>>>>>
> >>>>>> The reviewer's requested me to move the regulator node(s)
> >>>>>> elsewhere, and at some point later it was requested to be placed
> >>>>>> under the Root Port driver. I would love to return them under the
> >>>>>> host bridge, just say the word!
> >>>>>
> >>>>> Actually, I think my understanding is wrong. Even though the PCI core
> >>>>> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
> >>>>> about it and should be able to look up the regulators and turn them
> >>>>> on.
> >>>>
> >>>> One can do this with
> >>>> regulator_bulk_get(NULL, ...);
> >>>>
> >>>> However, MarkB did not like the idea of a driver getting the
> >>>> regulator from the global DT namespace [1].
> >>>>
> >>>> For the RC driver, one cannot invoke regulator_bulk_get(dev, ...)
> >>>> if there is not a direct child regulator node. One needs to use the
> >>>> Port driver device. The Port driver device does not exist at this
> >>>> point unless one tries to prematurely create it; I tried this and it
> >>>> was a mess to say the least.
> >>>>
> >>>>> Can you dig up the previous discussion about why the regulators need
> >>>>> to be under the Root Port and why they can't be turned on before
> >>>>> calling pci_host_probe()?
> >>>>
> >>>> RobH did not want the regulators to be under the RC as he said their
> >>>> DT location should resemble the HW [2]. The consensus evolved to
> >>>> place it under the port driver, which can provide a general
> >>>> mechanism for turning on regulators anywhere in the PCIe tree.
> >>>
> >>> I don't want to redesign this whole thing. I just want a crisp
> >>> rationale for the commit log.
> >>>
> >>> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
> >>> tegra, rockchip-host) have regulators for downstream PCIe power
> >>> directly under the RC. If putting the regulators under an RP instead
> >>> is the direction of the future, I guess that might be OK, and I guess
> >>> the reasons are:
> >>>
> >>> 1) Slot or device power regulators that are logically below the RP
> >>> should be described that way in the DT.
> >>>
> >>> 2) Associating regulators with a RP allows the possibility of
> >>> selectively controlling power to slots/devices below the RP,
> >>> e.g., to power down devices below RP A when suspending while
> >>> leaving wakeup devices below RP B powered up.
> >>>
> >>> I think in your case the motivating reason is 2).
> >>>
> >>> Your commit log for "Add mechanism to turn on subdev regulators"
> >>> suggests that you want some user control of endpoint power, e.g., via
> >>> sysfs, but I don't see that implemented yet except possibly via a
> >>> "remove" file that would unbind the driver and remove the entire
> >>> device.
> >> Hi Bjorn,
> >>
> >> Initially we wanted to (a) turn on any regulator found under the RC
> >> node and (b) allow the possibility of the regulator to control the
> >> EP's power. From the feedback, we realized early on that neither of
> >> these were going to fly, so we conceded both requests and just wanted
> >> to turn on standard PCIe regulators. Upon reading the aforementioned
> >> commit message I realize that there are a couple of leftover sentences
> >> from these early days that must be removed.
> >>
> >> I think when I submitted v1 of the original series that only the
> >> rockchip driver had regulators under the RC. And my recollection was
> >> that this was accepted but there was apprehension of this turning into
> >> the "standard" way of turning on such regulators, as the location of
> >> the regulator nodes was in question.
> >>
> >> In short, I would be quite content to follow the existing examples.
> >
> > The existing examples are, in general, incomplete and only work for
> > the simplest cases.
> >
> > There's 2 cases to consider here. The first is standard slots with
> > standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is
> > either non-standard slots or just soldered down devices which could
> > have any number of device specific resources. In the latter case,
> > those resources need to go into the node for the device. For the
> > former case (which we are discussing here), putting the resources in
> > the upstream (side of the link) node is fine. That's the root port
> > node(s) or switch port nodes. However, since most host bridges are a
> > single RP and don't put the RP node in DT, we've ended up with these
> > properties in host bridge nodes. That's fine as long as it's a single
> > RP and device. When it is not, we need to do something different. The
> > only way this scales is putting resources in the port nodes as those
> > are what have a 1:1 relationship to slots. If that's supported, then
> > the simple cases are also easily supported with if the resources are
> > not found in the port node/device, then look for them in the parent
> > node. That's also the path for how we get the handling of PERST out of
> > every host bridge driver.
>
> This has me confused now, are you suggesting that we pursue what Jim
> has put together here as a series which describes the regulators in
> the PCIe end-point device DT node, or that given that we have a
> single RC single RP configuration it is somewhat acceptable to
> describe regulators in the PCIe RC node?

(Need to fix your mailer to wrap lines)

We should not continue with the same mistake of putting per slot
properties in the RC node when they belong in the RP node. I was just
pointing out that we could still handle those existing cases by checking
the parent node.

Rob

2022-07-21 16:05:41

[permalink] [raw]

Subject: Re: [PATCH v2 3/6] PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts

On Thu, Jul 21, 2022 at 10:53:54AM -0400, Jim Quinlan wrote:
> https://lore.kernel.org/linux-pci/[email protected]/
> On Wed, Jul 20, 2022 at 6:06 PM Bjorn Helgaas <[email protected]> wrote:
> > On Sat, Jul 16, 2022 at 06:24:50PM -0400, Jim Quinlan wrote:
> > > Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
> > > established between itself and downstream, any subsequent config space
> > > access causes a CPU abort. This commit sets a "refusal mode" if the PCIe
> > > link-up fails, and this has our pci_ops map_bus function returning a NULL
> > > address, which in turn precludes the access from happening.
> > >
> > > Right now, "refusal mode" is window dressing. It will become relevant
> > > in a future commit when brcm_pcie_start_link() is invoked during
> > > enumeration instead of before it.
> > >
> > > Signed-off-by: Jim Quinlan <[email protected]>
> > > ---
> > > drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
> > > 1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index c026446d5830..72219a4f3964 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -255,6 +255,7 @@ struct brcm_pcie {
> > > u32 hw_rev;
> > > void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > > + bool refusal_mode;
> > > };
> > >
> > > static inline bool is_bmips(const struct brcm_pcie *pcie)
> > > @@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
> > > if (pci_is_root_bus(bus))
> > > return PCI_SLOT(devfn) ? NULL : base + where;
> > >
> > > + if (pcie->refusal_mode) {
> > > + /*
> > > + * At this point we do not have PCIe link-up. If there is
> > > + * a config read or write access besides those targeting
> > > + * the host bridge, our PCIe HW throws a CPU abort. To
> > > + * prevent this we return the NULL address. The calling
> > > + * functions -- pci_generic_config_*() -- will notice this
> > > + * and not perform the access, and if it is a read access,
> > > + * 0xffffffff is returned.
> > > + */
> > > + return NULL;
> > > + }
> >
> > Is this any different from all the other .map_bus() implementations
> > that return NULL when the link is down?
>
> Not really, but long ago I submitted code that gated the config spec
> access based on link status and was advised not to do it [1].
> I'll be happy to make it look like the others.
>
> [1] https://lore.kernel.org/linux-pci/[email protected]/

My point there was that if you can deal with the abort cleanly, that's
the best approach. Apparently brcmstb can't recover cleanly, so you
have to settle for the 99% solution.

The refusal_mode approach has the same race as checking
*_pcie_link_up(), since the link may go down between the time
brcm_pcie_start_link() sees that it is up and the time somebody does a
config access:

brcm_pcie_start_link
pcie->refusal_mode = false

<link goes down>

brcm_pcie_map_conf
if (pcie->refusal_mode) # still false

<config access causes abort>

So there's no advantage in making the code look different. Checking
for link-up in the config access path can never completely remove the
window, but it does make it smaller than using refusal_mode.

Bjorn

2022-07-21 16:12:27