A submission [1] was made to enable a PCIe root port to turn on regulators
for downstream devices. It was accepted. Months later, a regression was
discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
came too late in the release cycle. The regression in question is
triggered only when the PCIe RC DT node has no root port subnode, which is
a perfectly reasonsable configuration.
The original commits are now being resubmitted with some modifications to
fix the regression. The modifcations on the original commits are
described below (the SHA is that of the original commit):
[830aa6f29f07 PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
NOTE: In the originally submitted patchset, this commit introduced a
regression that was corrected by a subsequent commit in the same
patchset. Let's not do this again.
@@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
goto fail;
+ ret = brcm_pcie_linkup(pcie);
+ if (ret)
+ goto fail;
[67211aadcb4b PCI: brcmstb: Add mechanism to turn on subdev regulators]
NOTE: Not related to the regression, the regulators must be freed whenever
the PCIe tree is dismantled:
@@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
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;
[93e41f3fca3d PCI: brcmstb: Add control of subdevice voltage regulators]
NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
link-up was skipped. This is the regression. Fix it by attempting
link-up even if the Root Port DT subnode is missing.
@@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
static int brcm_pcie_add_bus(struct pci_bus *bus)
{
- struct device *dev = &bus->dev;
struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
int ret;
- if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+ if (!bus->parent || !pci_is_root_bus(bus->parent))
return 0;
ret = pci_subdev_regulators_add_bus(bus);
[1] https://lore.kernel.org/r/[email protected]
[2] https://bugzilla.kernel.org/show_bug.cgi?id=215925
[3] https://lore.kernel.org/linux-pci/[email protected]/
Jim Quinlan (4):
PCI: brcmstb: Split brcm_pcie_setup() into two funcs
PCI: brcmstb: Add mechanism to turn on subdev regulators
PCI: brcmstb: oAdd control of subdevice voltage regulators
PCI: brcmstb: Do not turn off WOL regulators on suspend
drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
1 file changed, 227 insertions(+), 30 deletions(-)
base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
--
2.17.1
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 DT node "RB" 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 node "RB".
-- 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 | 77 +++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2bf5cc399fd0..661d3834c6da 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>
@@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
};
+struct subdev_regulators {
+ unsigned int num_supplies;
+ struct regulator_bulk_data supplies[];
+};
+
+static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
+static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
+
struct brcm_msi {
struct device *dev;
void __iomem *base;
@@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
return ssc && pll ? 0 : -EIO;
}
+static void *alloc_subdev_regulators(struct device *dev)
+{
+ static const char * const supplies[] = {
+ "vpcie3v3",
+ "vpcie3v3aux",
+ "vpcie12v",
+ };
+ 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)
+ dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
+
+ sr = alloc_subdev_regulators(dev);
+ if (!sr)
+ return -ENOMEM;
+
+ dev->driver_data = sr;
+ ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+ if (ret)
+ return ret;
+
+ ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+ if (ret) {
+ dev_err(dev, "failed to enable regulators for downstream device\n");
+ return ret;
+ }
+
+ 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 (!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;
+}
+
/* Limits operation to a specific generation (1, 2, or 3) */
static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
{
@@ -779,6 +854,8 @@ 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 = {
--
2.17.1
We need to take some code in brcm_pcie_setup() and put it in a new function
brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
be called indirectly by pci_host_probe() as opposed to the host driver
invoking it directly.
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 e61058e13818..2bf5cc399fd0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -926,16 +926,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 */
@@ -1025,6 +1018,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_linkup(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);
@@ -1075,24 +1102,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)
@@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
goto err_reset;
+ ret = brcm_pcie_linkup(pcie);
+ if (ret)
+ goto err_reset;
+
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);
@@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
goto fail;
+ ret = brcm_pcie_linkup(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
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 | 53 ++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index a86bf502a265..d417dd366490 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -333,6 +333,7 @@ struct brcm_pcie {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
bool refusal_mode;
struct subdev_regulators *sr;
+ bool ep_wakeup_capable;
};
static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1350,9 +1351,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(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);
@@ -1371,11 +1384,22 @@ static int brcm_pcie_suspend(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);
@@ -1396,10 +1420,21 @@ static int brcm_pcie_resume(struct device *dev)
return ret;
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_disable_clk;
+ 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_disable_clk;
+ }
}
}
--
2.17.1
On 7/1/22 09:27, Jim Quinlan wrote:
> A submission [1] was made to enable a PCIe root port to turn on regulators
> for downstream devices. It was accepted. Months later, a regression was
> discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
> came too late in the release cycle. The regression in question is
> triggered only when the PCIe RC DT node has no root port subnode, which is
> a perfectly reasonsable configuration.
>
> The original commits are now being resubmitted with some modifications to
> fix the regression. The modifcations on the original commits are
> described below (the SHA is that of the original commit):
>
> [830aa6f29f07 PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
> NOTE: In the originally submitted patchset, this commit introduced a
> regression that was corrected by a subsequent commit in the same
> patchset. Let's not do this again.
>
> @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> if (ret)
> goto fail;
>
> + ret = brcm_pcie_linkup(pcie);
> + if (ret)
> + goto fail;
>
>
> [67211aadcb4b PCI: brcmstb: Add mechanism to turn on subdev regulators]
> NOTE: Not related to the regression, the regulators must be freed whenever
> the PCIe tree is dismantled:
>
> @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
>
> 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;
>
>
> [93e41f3fca3d PCI: brcmstb: Add control of subdevice voltage regulators]
> NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
> link-up was skipped. This is the regression. Fix it by attempting
> link-up even if the Root Port DT subnode is missing.
>
> @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
>
> static int brcm_pcie_add_bus(struct pci_bus *bus)
> {
> - struct device *dev = &bus->dev;
> struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> int ret;
>
> - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> + if (!bus->parent || !pci_is_root_bus(bus->parent))
> return 0;
>
> ret = pci_subdev_regulators_add_bus(bus);
>
> [1] https://lore.kernel.org/r/[email protected]
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=215925
> [3] https://lore.kernel.org/linux-pci/[email protected]/
On a Raspberry Pi 4B:
Tested-by: Florian Fainelli <[email protected]>
--
Florian
Florian Fainelli <[email protected]> (2022-07-01):
> On 7/1/22 09:27, Jim Quinlan wrote:
> > A submission [1] was made to enable a PCIe root port to turn on regulators
> > for downstream devices. It was accepted. Months later, a regression was
> > discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
> > came too late in the release cycle. The regression in question is
> > triggered only when the PCIe RC DT node has no root port subnode, which is
> > a perfectly reasonsable configuration.
> >
> > The original commits are now being resubmitted with some modifications to
> > fix the regression. The modifcations on the original commits are
> > described below (the SHA is that of the original commit):
> >
> > [830aa6f29f07 PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
> > NOTE: In the originally submitted patchset, this commit introduced a
> > regression that was corrected by a subsequent commit in the same
> > patchset. Let's not do this again.
> >
> > @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > goto fail;
> >
> > + ret = brcm_pcie_linkup(pcie);
> > + if (ret)
> > + goto fail;
> >
> >
> > [67211aadcb4b PCI: brcmstb: Add mechanism to turn on subdev regulators]
> > NOTE: Not related to the regression, the regulators must be freed whenever
> > the PCIe tree is dismantled:
> >
> > @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> >
> > 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;
> >
> >
> > [93e41f3fca3d PCI: brcmstb: Add control of subdevice voltage regulators]
> > NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
> > link-up was skipped. This is the regression. Fix it by attempting
> > link-up even if the Root Port DT subnode is missing.
> >
> > @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> >
> > static int brcm_pcie_add_bus(struct pci_bus *bus)
> > {
> > - struct device *dev = &bus->dev;
> > struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> > int ret;
> >
> > - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> > + if (!bus->parent || !pci_is_root_bus(bus->parent))
> > return 0;
> >
> > ret = pci_subdev_regulators_add_bus(bus);
> >
> > [1] https://lore.kernel.org/r/[email protected]
> > [2] https://bugzilla.kernel.org/show_bug.cgi?id=215925
> > [3] https://lore.kernel.org/linux-pci/[email protected]/
>
> On a Raspberry Pi 4B:
>
> Tested-by: Florian Fainelli <[email protected]>
As it stands, CM4 support in master is less than ideal: the mmc issues
I've mentioned in some earlier discussion are making it very hard to
draw any definitive conclusions. Soft reboots or cold boots don't seem
to make a difference: the storage might not show up at all, leading to
getting dropped into an initramfs shell, or it might show up but further
accesses can be delayed so much that the system proceeds to booting but
very slowly, and it might even lead to getting dropped into some
emergency/maintenance mode.
This affects both the CM4 Lite variant (no internal storage = SD card in
the CM4 IO slot) and some CM4 non-Lite variant (with internal storage),
with messages like this one getting repeated:
[ 310.105020] mmc0: Timeout waiting for hardware cmd interrupt.
[ 310.110864] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
[ 310.117390] mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00009902
[ 310.123918] mmc0: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000
[ 310.130445] mmc0: sdhci: Argument: 0x000001aa | Trn mode: 0x00000000
[ 310.136971] mmc0: sdhci: Present: 0x01ff0001 | Host ctl: 0x00000001
[ 310.143496] mmc0: sdhci: Power: 0x0000000f | Blk gap: 0x00000000
[ 310.150021] mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x00007187
[ 310.156548] mmc0: sdhci: Timeout: 0x00000000 | Int stat: 0x00018000
[ 310.163074] mmc0: sdhci: Int enab: 0x00ff0003 | Sig enab: 0x00ff0003
[ 310.169600] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000001
[ 310.176126] mmc0: sdhci: Caps: 0x00000000 | Caps_1: 0x00000000
[ 310.182652] mmc0: sdhci: Cmd: 0x0000081a | Max curr: 0x00000001
[ 310.189178] mmc0: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000
[ 310.195704] mmc0: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
[ 310.202230] mmc0: sdhci: Host ctl2: 0x00000000
[ 310.206728] mmc0: sdhci: ============================================
That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with or
without this patchset.
That being said, I'm not able to reproduce the showstopper regression
that I reported against the initial patchset (booting was breaking in
the very first few seconds), so I suppose it's fine to propose the
following even if that's somewhat tainted by those mmc issues.
With Raspberry Pi CM4 (Lite and non-Lite), mounted on a CM4 IO Board:
- with a PCIe to quad-USB board, USB storage and USB keyboard;
- without anything in the PCIe slot.
Tested-by: Cyril Brulebois <[email protected]>
Cheers,
--
Cyril Brulebois ([email protected]) <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant
On 7/5/22 13:55, Cyril Brulebois wrote:
> Florian Fainelli <[email protected]> (2022-07-01):
>> On 7/1/22 09:27, Jim Quinlan wrote:
>>> A submission [1] was made to enable a PCIe root port to turn on regulators
>>> for downstream devices. It was accepted. Months later, a regression was
>>> discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
>>> came too late in the release cycle. The regression in question is
>>> triggered only when the PCIe RC DT node has no root port subnode, which is
>>> a perfectly reasonsable configuration.
>>>
>>> The original commits are now being resubmitted with some modifications to
>>> fix the regression. The modifcations on the original commits are
>>> described below (the SHA is that of the original commit):
>>>
>>> [830aa6f29f07 PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
>>> NOTE: In the originally submitted patchset, this commit introduced a
>>> regression that was corrected by a subsequent commit in the same
>>> patchset. Let's not do this again.
>>>
>>> @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto fail;
>>>
>>> + ret = brcm_pcie_linkup(pcie);
>>> + if (ret)
>>> + goto fail;
>>>
>>>
>>> [67211aadcb4b PCI: brcmstb: Add mechanism to turn on subdev regulators]
>>> NOTE: Not related to the regression, the regulators must be freed whenever
>>> the PCIe tree is dismantled:
>>>
>>> @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
>>>
>>> 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;
>>>
>>>
>>> [93e41f3fca3d PCI: brcmstb: Add control of subdevice voltage regulators]
>>> NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
>>> link-up was skipped. This is the regression. Fix it by attempting
>>> link-up even if the Root Port DT subnode is missing.
>>>
>>> @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
>>>
>>> static int brcm_pcie_add_bus(struct pci_bus *bus)
>>> {
>>> - struct device *dev = &bus->dev;
>>> struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
>>> int ret;
>>>
>>> - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
>>> + if (!bus->parent || !pci_is_root_bus(bus->parent))
>>> return 0;
>>>
>>> ret = pci_subdev_regulators_add_bus(bus);
>>>
>>> [1] https://lore.kernel.org/r/[email protected]
>>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=215925
>>> [3] https://lore.kernel.org/linux-pci/[email protected]/
>>
>> On a Raspberry Pi 4B:
>>
>> Tested-by: Florian Fainelli <[email protected]>
>
> As it stands, CM4 support in master is less than ideal: the mmc issues
> I've mentioned in some earlier discussion are making it very hard to
> draw any definitive conclusions. Soft reboots or cold boots don't seem
> to make a difference: the storage might not show up at all, leading to
> getting dropped into an initramfs shell, or it might show up but further
> accesses can be delayed so much that the system proceeds to booting but
> very slowly, and it might even lead to getting dropped into some
> emergency/maintenance mode.
>
> This affects both the CM4 Lite variant (no internal storage = SD card in
> the CM4 IO slot) and some CM4 non-Lite variant (with internal storage),
> with messages like this one getting repeated:
>
> [ 310.105020] mmc0: Timeout waiting for hardware cmd interrupt.
> [ 310.110864] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> [ 310.117390] mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00009902
> [ 310.123918] mmc0: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000
> [ 310.130445] mmc0: sdhci: Argument: 0x000001aa | Trn mode: 0x00000000
> [ 310.136971] mmc0: sdhci: Present: 0x01ff0001 | Host ctl: 0x00000001
> [ 310.143496] mmc0: sdhci: Power: 0x0000000f | Blk gap: 0x00000000
> [ 310.150021] mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x00007187
> [ 310.156548] mmc0: sdhci: Timeout: 0x00000000 | Int stat: 0x00018000
> [ 310.163074] mmc0: sdhci: Int enab: 0x00ff0003 | Sig enab: 0x00ff0003
> [ 310.169600] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000001
> [ 310.176126] mmc0: sdhci: Caps: 0x00000000 | Caps_1: 0x00000000
> [ 310.182652] mmc0: sdhci: Cmd: 0x0000081a | Max curr: 0x00000001
> [ 310.189178] mmc0: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000
> [ 310.195704] mmc0: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
> [ 310.202230] mmc0: sdhci: Host ctl2: 0x00000000
> [ 310.206728] mmc0: sdhci: ============================================
>
> That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with or
> without this patchset.
>
> That being said, I'm not able to reproduce the showstopper regression
> that I reported against the initial patchset (booting was breaking in
> the very first few seconds), so I suppose it's fine to propose the
> following even if that's somewhat tainted by those mmc issues.
Any chance you can bisect the eMMC issues so we can investigate those
separately? Thanks!
>
>
> With Raspberry Pi CM4 (Lite and non-Lite), mounted on a CM4 IO Board:
> - with a PCIe to quad-USB board, USB storage and USB keyboard;
> - without anything in the PCIe slot.
>
> Tested-by: Cyril Brulebois <[email protected]>
Thanks!
--
Florian
Florian Fainelli <[email protected]> (2022-07-05):
> On 7/5/22 13:55, Cyril Brulebois wrote:
> > That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with
> > or without this patchset.
> >
> > That being said, I'm not able to reproduce the showstopper
> > regression that I reported against the initial patchset (booting was
> > breaking in the very first few seconds), so I suppose it's fine to
> > propose the following even if that's somewhat tainted by those mmc
> > issues.
>
> Any chance you can bisect the eMMC issues so we can investigate those
> separately? Thanks!
Definitely. I wanted to make sure I wouldn't delay the reintroduction of
this patchset (feeling partly responsible for the revert that happened
in the first place), by providing some feedback regarding a possible
come-back of the regression, as soon as possible.
Now that this is out of the way, I'll try and find time to investigate
those MMC issues. Ditto for DRM, I seem to have completely lost the HDMI
output (that's less of an issue thanks to the serial console that has
been rather reliable to gather kernel logs).
I think I started encountering both issues very early in the devel
cycle (when we were still trying to find some follow-up commits to fix
the regression instead of going for the full-on revert), but I couldn't
afford spending time chasing multiple issues at once. I haven't checked
whether reports exist already for those issues, but that's my next step.
Cheers,
--
Cyril Brulebois ([email protected]) <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant
On Tue, Jul 5, 2022 at 5:28 PM Cyril Brulebois <[email protected]> wrote:
>
> Florian Fainelli <[email protected]> (2022-07-05):
> > On 7/5/22 13:55, Cyril Brulebois wrote:
> > > That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with
> > > or without this patchset.
> > >
> > > That being said, I'm not able to reproduce the showstopper
> > > regression that I reported against the initial patchset (booting was
> > > breaking in the very first few seconds), so I suppose it's fine to
> > > propose the following even if that's somewhat tainted by those mmc
> > > issues.
> >
> > Any chance you can bisect the eMMC issues so we can investigate those
> > separately? Thanks!
Cyril,
Before you go to the trouble of a bisection, can you just post the
(or email me) the following:
o complete boot log
o output of "cat /proc/interrupts"
o output of "for i in $(find /sys/devices/platform/ -type f -name
state) ; do echo $i: $(cat $i) ; done"
Thanks,
Jim Quinlan
Broadcom STB
>
>
> Definitely. I wanted to make sure I wouldn't delay the reintroduction of
> this patchset (feeling partly responsible for the revert that happened
> in the first place), by providing some feedback regarding a possible
> come-back of the regression, as soon as possible.
>
> Now that this is out of the way, I'll try and find time to investigate
> those MMC issues. Ditto for DRM, I seem to have completely lost the HDMI
> output (that's less of an issue thanks to the serial console that has
> been rather reliable to gather kernel logs).
>
> I think I started encountering both issues very early in the devel
> cycle (when we were still trying to find some follow-up commits to fix
> the regression instead of going for the full-on revert), but I couldn't
> afford spending time chasing multiple issues at once. I haven't checked
> whether reports exist already for those issues, but that's my next step.
>
>
> Cheers,
> --
> Cyril Brulebois ([email protected]) <https://debamax.com/>
> D-I release manager -- Release team member -- Freelance Consultant
On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> We need to take some code in brcm_pcie_setup() and put it in a new function
> brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> be called indirectly by pci_host_probe() as opposed to the host driver
> invoking it directly.
>
> 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().
This says we need to move some code from brcm_pcie_setup() to
brcm_pcie_linkup(), but not *why* we need to do that.
In brcm_pcie_resume(), they're called together:
brcm_pcie_resume
brcm_pcie_setup
brcm_pcie_linkup
In the probe path, they're not called together, but they're in the
same order:
brcm_pcie_probe
brcm_pcie_setup
pci_host_probe
...
brcm_pcie_add_bus # bus->ops->add_bus
brcm_pcie_linkup
Is there something that must happen *between* them in the probe path?
> 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 e61058e13818..2bf5cc399fd0 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -926,16 +926,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 */
> @@ -1025,6 +1018,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_linkup(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);
>
> @@ -1075,24 +1102,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)
> @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> if (ret)
> goto err_reset;
>
> + ret = brcm_pcie_linkup(pcie);
> + if (ret)
> + goto err_reset;
> +
> if (pcie->msi)
> brcm_msi_set_regs(pcie->msi);
>
> @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> if (ret)
> goto fail;
>
> + ret = brcm_pcie_linkup(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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> 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 DT node "RB" under the host bridge DT node.
"RB" isn't mentioned in pcie-brcmstb.c. What's the connection to it?
Is it just an example, and the actual name doesn't matter?
> -- 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 node "RB".
> -- 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.
I guess this is all functionality that depends on new properties in
the DT? Prior to this patch, pcie-brcmstb.c didn't do anything at all
with regulators, although brcm,stb-pcie.yaml does mention
"vpcie3v3-supply" in an example.
> 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 | 77 +++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2bf5cc399fd0..661d3834c6da 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>
> @@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
> .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> };
>
> +struct subdev_regulators {
> + unsigned int num_supplies;
> + struct regulator_bulk_data supplies[];
> +};
> +
> +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
I think these forward declarations are unnecessary. I can remove them
if you agree.
> struct brcm_msi {
> struct device *dev;
> void __iomem *base;
> @@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> return ssc && pll ? 0 : -EIO;
> }
>
> +static void *alloc_subdev_regulators(struct device *dev)
> +{
> + static const char * const supplies[] = {
> + "vpcie3v3",
> + "vpcie3v3aux",
> + "vpcie12v",
> + };
> + 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)
> + dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
I guess you're using the pci_bus dev->driver_data. I don't know of
other users of it, but there's really no ownership model for it. If
it's non-NULL here, it means somebody else, e.g., the PCI core, is
already using it, and when you overwrite it below, you will break that
other user.
I think you should complain and return instead of breaking the other
user. That will mean the regulator won't get enabled and your
endpoint won't work, but I think that's a better way to fail than by
overwriting somebody else's pointer, which may lead to memory
corruption that's very hard to debug.
> + sr = alloc_subdev_regulators(dev);
> + if (!sr)
> + return -ENOMEM;
> +
> + dev->driver_data = sr;
> + ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> + if (ret)
> + return ret;
> +
> + ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
> + if (ret) {
> + dev_err(dev, "failed to enable regulators for downstream device\n");
> + return ret;
> + }
> +
> + 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 (!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;
> +}
> +
> /* Limits operation to a specific generation (1, 2, or 3) */
> static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> {
> @@ -779,6 +854,8 @@ 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 = {
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > We need to take some code in brcm_pcie_setup() and put it in a new function
> > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > be called indirectly by pci_host_probe() as opposed to the host driver
> > invoking it directly.
> >
> > 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().
>
> This says we need to move some code from brcm_pcie_setup() to
> brcm_pcie_linkup(), but not *why* we need to do that.
I will elaborate in the commit message.
>
> In brcm_pcie_resume(), they're called together:
>
> brcm_pcie_resume
> brcm_pcie_setup
> brcm_pcie_linkup
>
> In the probe path, they're not called together, but they're in the
> same order:
>
> brcm_pcie_probe
> brcm_pcie_setup
> pci_host_probe
> ...
> brcm_pcie_add_bus # bus->ops->add_bus
> brcm_pcie_linkup
>
> Is there something that must happen *between* them in the probe path?
Yes. In the probe() case, we must do things in this order:
1. brcm_pcie_setup()
2. Turn on regulators
3. brcm_pcie_linkup()
Since the voltage regulators are turned on during enumeration, pci_host_probe()
must be invoked prior to 3. Before regulators, we did not care.
In the resume case, there is no enumeration of course but our driver
has a handle to
the regulators and can turn them on/off w/o help.
Regards,
Jim Quinlan
Broradcom STB
>
> > 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 e61058e13818..2bf5cc399fd0 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -926,16 +926,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 */
> > @@ -1025,6 +1018,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_linkup(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);
> >
> > @@ -1075,24 +1102,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)
> > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> > if (ret)
> > goto err_reset;
> >
> > + ret = brcm_pcie_linkup(pcie);
> > + if (ret)
> > + goto err_reset;
> > +
> > if (pcie->msi)
> > brcm_msi_set_regs(pcie->msi);
> >
> > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > goto fail;
> >
> > + ret = brcm_pcie_linkup(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
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jul 6, 2022 at 7:12 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> > 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 DT node "RB" under the host bridge DT node.
>
> "RB" isn't mentioned in pcie-brcmstb.c. What's the connection to it?
> Is it just an example, and the actual name doesn't matter?
I will reword this to something like "a regulator with one of these names
... under a root port DT node. I will review/edit this entire commit msg.
>
> > -- 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 node "RB".
> > -- 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.
>
> I guess this is all functionality that depends on new properties in
> the DT? Prior to this patch, pcie-brcmstb.c didn't do anything at all
> with regulators, although brcm,stb-pcie.yaml does mention
> "vpcie3v3-supply" in an example.
What is new in the DT is the presence of a regulator under a root
port node. That is it. I submitted the regulator YAML allowance
to Rob's Github repo and I believe it was accepted.
>
> > 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 | 77 +++++++++++++++++++++++++++
> > 1 file changed, 77 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 2bf5cc399fd0..661d3834c6da 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>
> > @@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
> > .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> > };
> >
> > +struct subdev_regulators {
> > + unsigned int num_supplies;
> > + struct regulator_bulk_data supplies[];
> > +};
> > +
> > +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> > +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
>
> I think these forward declarations are unnecessary. I can remove them
> if you agree.
It is up to you. I have a commit-set ready that will make a number of
improvements to our driver.
One of them removes all forward declarations. Other commits concern
other suggestions you
have made, e.g. rename brcm_pcie_linkup() to brcm_pcie_start_link().
>
> > struct brcm_msi {
> > struct device *dev;
> > void __iomem *base;
> > @@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> > return ssc && pll ? 0 : -EIO;
> > }
> >
> > +static void *alloc_subdev_regulators(struct device *dev)
> > +{
> > + static const char * const supplies[] = {
> > + "vpcie3v3",
> > + "vpcie3v3aux",
> > + "vpcie12v",
> > + };
> > + 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)
> > + dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
>
> I guess you're using the pci_bus dev->driver_data. I don't know of
> other users of it, but there's really no ownership model for it. If
> it's non-NULL here, it means somebody else, e.g., the PCI core, is
> already using it, and when you overwrite it below, you will break that
> other user.
Yes, I'm not happy about this vulnerability.
>
> I think you should complain and return instead of breaking the other
> user. That will mean the regulator won't get enabled and your
> endpoint won't work, but I think that's a better way to fail than by
> overwriting somebody else's pointer, which may lead to memory
> corruption that's very hard to debug.
Agree, will do in v2.
Regards,
Jim Quinlan
Broadcom STB
>
> > + sr = alloc_subdev_regulators(dev);
> > + if (!sr)
> > + return -ENOMEM;
> > +
> > + dev->driver_data = sr;
> > + ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
> > + if (ret) {
> > + dev_err(dev, "failed to enable regulators for downstream device\n");
> > + return ret;
> > + }
> > +
> > + 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 (!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;
> > +}
> > +
> > /* Limits operation to a specific generation (1, 2, or 3) */
> > static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> > {
> > @@ -779,6 +854,8 @@ 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 = {
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > invoking it directly.
> > >
> > > 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().
> >
> > This says we need to move some code from brcm_pcie_setup() to
> > brcm_pcie_linkup(), but not *why* we need to do that.
> I will elaborate in the commit message.
> >
> > In brcm_pcie_resume(), they're called together:
> >
> > brcm_pcie_resume
> > brcm_pcie_setup
> > brcm_pcie_linkup
> >
> > In the probe path, they're not called together, but they're in the
> > same order:
> >
> > brcm_pcie_probe
> > brcm_pcie_setup
> > pci_host_probe
> > ...
> > brcm_pcie_add_bus # bus->ops->add_bus
> > brcm_pcie_linkup
> >
> > Is there something that must happen *between* them in the probe path?
>
> Yes. In the probe() case, we must do things in this order:
>
> 1. brcm_pcie_setup()
> 2. Turn on regulators
> 3. brcm_pcie_linkup()
Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
brcm_pcie_add_bus # bus->ops->add_bus
pci_subdev_regulators_add_bus
regulator_bulk_enable # turn on regulators
brcm_pcie_linkup
> Since the voltage regulators are turned on during enumeration,
> pci_host_probe() must be invoked prior to 3. Before regulators, we
> did not care.
I guess in the pre-regulator case, i.e., pcie->sr not set, the power
for downstream devices must always be on.
> In the resume case, there is no enumeration of course but our driver
> has a handle to the regulators and can turn them on/off w/o help.
And I guess we don't need brcm_pcie_setup() in the resume path because
suspend turns off power only for downstream devices, not for the root
port itself, so the programming done by brcm_pcie_setup() doesn't need
to be done again.
> > > 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 e61058e13818..2bf5cc399fd0 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -926,16 +926,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 */
> > > @@ -1025,6 +1018,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_linkup(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);
> > >
> > > @@ -1075,24 +1102,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)
> > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> > > if (ret)
> > > goto err_reset;
> > >
> > > + ret = brcm_pcie_linkup(pcie);
> > > + if (ret)
> > > + goto err_reset;
> > > +
> > > if (pcie->msi)
> > > brcm_msi_set_regs(pcie->msi);
> > >
> > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > if (ret)
> > > goto fail;
> > >
> > > + ret = brcm_pcie_linkup(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
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 08, 2022 at 10:14:11AM -0400, Jim Quinlan wrote:
> On Wed, Jul 6, 2022 at 7:12 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> > > 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().
> > > ...
> > > +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> > > +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
> >
> > I think these forward declarations are unnecessary. I can remove them
> > if you agree.
>
> It is up to you. I have a commit-set ready that will make a number of
> improvements to our driver.
> One of them removes all forward declarations. Other commits concern
> other suggestions you
> have made, e.g. rename brcm_pcie_linkup() to brcm_pcie_start_link().
If you're planning a v2, I'd drop the declarations there. No point in
adding them, only to remove them in a future patch (unless we need
them in the interim, of course, but in this case I don't think we do).
Bjorn
On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > invoking it directly.
> > > >
> > > > 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().
> > >
> > > This says we need to move some code from brcm_pcie_setup() to
> > > brcm_pcie_linkup(), but not *why* we need to do that.
> > I will elaborate in the commit message.
> > >
> > > In brcm_pcie_resume(), they're called together:
> > >
> > > brcm_pcie_resume
> > > brcm_pcie_setup
> > > brcm_pcie_linkup
> > >
> > > In the probe path, they're not called together, but they're in the
> > > same order:
> > >
> > > brcm_pcie_probe
> > > brcm_pcie_setup
> > > pci_host_probe
> > > ...
> > > brcm_pcie_add_bus # bus->ops->add_bus
> > > brcm_pcie_linkup
> > >
> > > Is there something that must happen *between* them in the probe path?
> >
> > Yes. In the probe() case, we must do things in this order:
> >
> > 1. brcm_pcie_setup()
> > 2. Turn on regulators
> > 3. brcm_pcie_linkup()
>
> Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
>
> brcm_pcie_add_bus # bus->ops->add_bus
> pci_subdev_regulators_add_bus
> regulator_bulk_enable # turn on regulators
> brcm_pcie_linkup
>
> > Since the voltage regulators are turned on during enumeration,
> > pci_host_probe() must be invoked prior to 3. Before regulators, we
> > did not care.
>
> I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> for downstream devices must always be on.
>
> > In the resume case, there is no enumeration of course but our driver
> > has a handle to the regulators and can turn them on/off w/o help.
>
> And I guess we don't need brcm_pcie_setup() in the resume path because
> suspend turns off power only for downstream devices, not for the root
> port itself, so the programming done by brcm_pcie_setup() doesn't need
> to be done again.
I'm not sure I understand what you are saying -- brcm_pcie_setup() is
called by brcm_pcie_resume()
because it is needed. brcm_pcie_setup() isn't concerned with power it
just does the preparation
required before attempting link-up.
Regards,
Jim Quinlan
Broadcom STB
>
> > > > 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 e61058e13818..2bf5cc399fd0 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -926,16 +926,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 */
> > > > @@ -1025,6 +1018,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_linkup(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);
> > > >
> > > > @@ -1075,24 +1102,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)
> > > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> > > > if (ret)
> > > > goto err_reset;
> > > >
> > > > + ret = brcm_pcie_linkup(pcie);
> > > > + if (ret)
> > > > + goto err_reset;
> > > > +
> > > > if (pcie->msi)
> > > > brcm_msi_set_regs(pcie->msi);
> > > >
> > > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > if (ret)
> > > > goto fail;
> > > >
> > > > + ret = brcm_pcie_linkup(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
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > [email protected]
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > invoking it directly.
> > > > >
> > > > > 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().
> > > >
> > > > This says we need to move some code from brcm_pcie_setup() to
> > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > I will elaborate in the commit message.
> > > >
> > > > In brcm_pcie_resume(), they're called together:
> > > >
> > > > brcm_pcie_resume
> > > > brcm_pcie_setup
> > > > brcm_pcie_linkup
> > > >
> > > > In the probe path, they're not called together, but they're in the
> > > > same order:
> > > >
> > > > brcm_pcie_probe
> > > > brcm_pcie_setup
> > > > pci_host_probe
> > > > ...
> > > > brcm_pcie_add_bus # bus->ops->add_bus
> > > > brcm_pcie_linkup
> > > >
> > > > Is there something that must happen *between* them in the probe path?
> > >
> > > Yes. In the probe() case, we must do things in this order:
> > >
> > > 1. brcm_pcie_setup()
> > > 2. Turn on regulators
> > > 3. brcm_pcie_linkup()
> >
> > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> >
> > brcm_pcie_add_bus # bus->ops->add_bus
> > pci_subdev_regulators_add_bus
> > regulator_bulk_enable # turn on regulators
> > brcm_pcie_linkup
> >
> > > Since the voltage regulators are turned on during enumeration,
> > > pci_host_probe() must be invoked prior to 3. Before regulators, we
> > > did not care.
> >
> > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > for downstream devices must always be on.
> >
> > > In the resume case, there is no enumeration of course but our driver
> > > has a handle to the regulators and can turn them on/off w/o help.
> >
> > And I guess we don't need brcm_pcie_setup() in the resume path because
> > suspend turns off power only for downstream devices, not for the root
> > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > to be done again.
>
> I'm not sure I understand what you are saying -- brcm_pcie_setup() is
> called by brcm_pcie_resume()
> because it is needed. brcm_pcie_setup() isn't concerned with power it
> just does the preparation
> required before attempting link-up.
Oh, sorry, I totally misread that.
But I wonder about the fact that probe and resume do these in
different orders:
brcm_pcie_probe
brcm_pcie_setup # setup
pci_host_probe
...
brcm_pcie_add_bus
pci_subdev_regulators_add_bus
regulator_bulk_enable # regulators on
brcm_pcie_linkup # linkup
brcm_pcie_resume
regulator_bulk_enable # regulators on
brcm_pcie_setup # setup
brcm_pcie_linkup # linkup
Maybe pci_subdev_regulators_add_bus() could be done directly from
brcm_pcie_probe() instead of in brcm_pcie_add_bus()? If the
regulators must be directly under the root port node in DT, it seems
like it would be reasonable to look for them in the probe path, which
seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
pcie-rockchip-host.c do.
Or maybe brcm_pcie_resume() should enable the regulators after
brcm_pcie_setup() so it's the same order as the probe path?
Bjorn
On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > > invoking it directly.
> > > > > >
> > > > > > 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().
> > > > >
> > > > > This says we need to move some code from brcm_pcie_setup() to
> > > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > > I will elaborate in the commit message.
> > > > >
> > > > > In brcm_pcie_resume(), they're called together:
> > > > >
> > > > > brcm_pcie_resume
> > > > > brcm_pcie_setup
> > > > > brcm_pcie_linkup
> > > > >
> > > > > In the probe path, they're not called together, but they're in the
> > > > > same order:
> > > > >
> > > > > brcm_pcie_probe
> > > > > brcm_pcie_setup
> > > > > pci_host_probe
> > > > > ...
> > > > > brcm_pcie_add_bus # bus->ops->add_bus
> > > > > brcm_pcie_linkup
> > > > >
> > > > > Is there something that must happen *between* them in the probe path?
> > > >
> > > > Yes. In the probe() case, we must do things in this order:
> > > >
> > > > 1. brcm_pcie_setup()
> > > > 2. Turn on regulators
> > > > 3. brcm_pcie_linkup()
> > >
> > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> > >
> > > brcm_pcie_add_bus # bus->ops->add_bus
> > > pci_subdev_regulators_add_bus
> > > regulator_bulk_enable # turn on regulators
> > > brcm_pcie_linkup
> > >
> > > > Since the voltage regulators are turned on during enumeration,
> > > > pci_host_probe() must be invoked prior to 3. Before regulators, we
> > > > did not care.
> > >
> > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > > for downstream devices must always be on.
> > >
> > > > In the resume case, there is no enumeration of course but our driver
> > > > has a handle to the regulators and can turn them on/off w/o help.
> > >
> > > And I guess we don't need brcm_pcie_setup() in the resume path because
> > > suspend turns off power only for downstream devices, not for the root
> > > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > > to be done again.
> >
> > I'm not sure I understand what you are saying -- brcm_pcie_setup() is
> > called by brcm_pcie_resume()
> > because it is needed. brcm_pcie_setup() isn't concerned with power it
> > just does the preparation
> > required before attempting link-up.
>
> Oh, sorry, I totally misread that.
>
> But I wonder about the fact that probe and resume do these in
> different orders:
>
> brcm_pcie_probe
> brcm_pcie_setup # setup
> pci_host_probe
> ...
> brcm_pcie_add_bus
> pci_subdev_regulators_add_bus
> regulator_bulk_enable # regulators on
> brcm_pcie_linkup # linkup
>
> brcm_pcie_resume
> regulator_bulk_enable # regulators on
> brcm_pcie_setup # setup
> brcm_pcie_linkup # linkup
>
brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(),
but your point is valid -- it is prudent to keep the orders
consistent. Let me think
about this.
> Maybe pci_subdev_regulators_add_bus() could be done directly from
> brcm_pcie_probe() instead of in brcm_pcie_add_bus()?
> regulators must be directly under the root port node in DT, it seems
> like it would be reasonable to look for them in the probe path, which
> seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
> pcie-rockchip-host.c do.
At some point in the original patchset -- IIRC -- the RC driver was
searching the DT
tree for regulators. However, doing a "get" on these regulators is pretty much
impossible if the "owning" device does not exist. I even had a version that
partially created the downstream device; this pullreq was a mess and
got feedback which put me on the current approach.
Reviews suggested that the best location for the regulators should be located
in the root port DT node(s). I agree with this. In addition, there
was a request to allow multiple regulators
to exist at each of the root ports in the downstream tree. So if the RC driver
has to potentially add multiple buses. I really don't know how it
would do that,
and then call the pci_host_probe() w/o it failing. Perhaps this is what ACPI
does before boot -- I'm guessing here -- but I would also guess that it is
a decent amount of code as it is not far from doing enumeration.
One thing I could do is to allow the port driver's suspend/resume to do the
turning off/on of the regulators. There are two issues with this: (1)
feedback suggested
to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable"
code would also have to live in the port driver and I'm not sure this
would be welcome.
>
> Or maybe brcm_pcie_resume() should enable the regulators after
> brcm_pcie_setup() so it's the same order as the probe path?
I think I'll do this.
Thanks,
Jim Quinlan
Broadcom STB
>
> Bjorn
On Fri, Jul 08, 2022 at 04:38:30PM -0400, Jim Quinlan wrote:
> On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> > > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will
> > > > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > > > invoking it directly.
> > > > > > >
> > > > > > > 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().
> > > > > >
> > > > > > This says we need to move some code from brcm_pcie_setup() to
> > > > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > > > I will elaborate in the commit message.
> > > > > >
> > > > > > In brcm_pcie_resume(), they're called together:
> > > > > >
> > > > > > brcm_pcie_resume
> > > > > > brcm_pcie_setup
> > > > > > brcm_pcie_linkup
> > > > > >
> > > > > > In the probe path, they're not called together, but they're in the
> > > > > > same order:
> > > > > >
> > > > > > brcm_pcie_probe
> > > > > > brcm_pcie_setup
> > > > > > pci_host_probe
> > > > > > ...
> > > > > > brcm_pcie_add_bus # bus->ops->add_bus
> > > > > > brcm_pcie_linkup
> > > > > >
> > > > > > Is there something that must happen *between* them in the probe path?
> > > > >
> > > > > Yes. In the probe() case, we must do things in this order:
> > > > >
> > > > > 1. brcm_pcie_setup()
> > > > > 2. Turn on regulators
> > > > > 3. brcm_pcie_linkup()
> > > >
> > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> > > >
> > > > brcm_pcie_add_bus # bus->ops->add_bus
> > > > pci_subdev_regulators_add_bus
> > > > regulator_bulk_enable # turn on regulators
> > > > brcm_pcie_linkup
> > > >
> > > > > Since the voltage regulators are turned on during enumeration,
> > > > > pci_host_probe() must be invoked prior to 3. Before regulators, we
> > > > > did not care.
> > > >
> > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > > > for downstream devices must always be on.
> > > >
> > > > > In the resume case, there is no enumeration of course but our driver
> > > > > has a handle to the regulators and can turn them on/off w/o help.
> > > >
> > > > And I guess we don't need brcm_pcie_setup() in the resume path because
> > > > suspend turns off power only for downstream devices, not for the root
> > > > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > > > to be done again.
> > >
> > > I'm not sure I understand what you are saying -- brcm_pcie_setup() is
> > > called by brcm_pcie_resume()
> > > because it is needed. brcm_pcie_setup() isn't concerned with power it
> > > just does the preparation
> > > required before attempting link-up.
> >
> > Oh, sorry, I totally misread that.
> >
> > But I wonder about the fact that probe and resume do these in
> > different orders:
> >
> > brcm_pcie_probe
> > brcm_pcie_setup # setup
> > pci_host_probe
> > ...
> > brcm_pcie_add_bus
> > pci_subdev_regulators_add_bus
> > regulator_bulk_enable # regulators on
> > brcm_pcie_linkup # linkup
> >
> > brcm_pcie_resume
> > regulator_bulk_enable # regulators on
> > brcm_pcie_setup # setup
> > brcm_pcie_linkup # linkup
> >
> brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(),
> but your point is valid -- it is prudent to keep the orders
> consistent. Let me think
> about this.
>
> > Maybe pci_subdev_regulators_add_bus() could be done directly from
> > brcm_pcie_probe() instead of in brcm_pcie_add_bus()?
> > regulators must be directly under the root port node in DT, it seems
> > like it would be reasonable to look for them in the probe path, which
> > seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
> > pcie-rockchip-host.c do.
> At some point in the original patchset -- IIRC -- the RC driver was
> searching the DT
> tree for regulators. However, doing a "get" on these regulators is pretty much
> impossible if the "owning" device does not exist. I even had a version that
> partially created the downstream device; this pullreq was a mess and
> got feedback which put me on the current approach.
Ah, I suppose because the regulators are not under the host bridge
itself, but under the *root port*, which is a PCI device that doesn't
exist until we enumerate it. Although I guess the root port is
described in the DT, and the regulators are connected with that DT
description, not directly with the pci_dev.
> Reviews suggested that the best location for the regulators should be located
> in the root port DT node(s). I agree with this. In addition, there
> was a request to allow multiple regulators
> to exist at each of the root ports in the downstream tree.
Makes sense.
> So if the RC driver
> has to potentially add multiple buses. I really don't know how it
> would do that,
> and then call the pci_host_probe() w/o it failing. Perhaps this is what ACPI
> does before boot -- I'm guessing here -- but I would also guess that it is
> a decent amount of code as it is not far from doing enumeration.
>
> One thing I could do is to allow the port driver's suspend/resume to do the
> turning off/on of the regulators. There are two issues with this: (1)
> feedback suggested
> to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable"
> code would also have to live in the port driver and I'm not sure this
> would be welcome.
>
> > Or maybe brcm_pcie_resume() should enable the regulators after
> > brcm_pcie_setup() so it's the same order as the probe path?
> I think I'll do this.
Yep, sounds like the right thing.
Bjorn
On Fri, Jul 15, 2022 at 2:27 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:21PM -0400, Jim Quinlan wrote:
> > A submission [1] was made to enable a PCIe root port to turn on regulators
> > for downstream devices. It was accepted. Months later, a regression was
> > discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
> > came too late in the release cycle. The regression in question is
> > triggered only when the PCIe RC DT node has no root port subnode, which is
> > a perfectly reasonsable configuration.
> > ...
>
> > Jim Quinlan (4):
> > PCI: brcmstb: Split brcm_pcie_setup() into two funcs
> > PCI: brcmstb: Add mechanism to turn on subdev regulators
> > PCI: brcmstb: oAdd control of subdevice voltage regulators
> > PCI: brcmstb: Do not turn off WOL regulators on suspend
> >
> > drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
> > 1 file changed, 227 insertions(+), 30 deletions(-)
>
> I'm assuming there's a v2 coming soonish? We should see -rc7 this
> weekend and likely a final v5.19 release on July 24, so v5.20 material
> should be tidied up by then.
Hi Bjorn,
Yes, it has been ready for a few days but I am bumping into unrelated
issues while
trying to do suspend/resume tests with the latest upstream. Hopefully
I will send
it out tonight or this WE.
Regards,
Jim Quinlan
Broadcom STB
>
> Bjorn
On Fri, Jul 01, 2022 at 12:27:21PM -0400, Jim Quinlan wrote:
> A submission [1] was made to enable a PCIe root port to turn on regulators
> for downstream devices. It was accepted. Months later, a regression was
> discovered on an RPi CM4 [2]. The patchset was reverted [3] as the fix
> came too late in the release cycle. The regression in question is
> triggered only when the PCIe RC DT node has no root port subnode, which is
> a perfectly reasonsable configuration.
> ...
> Jim Quinlan (4):
> PCI: brcmstb: Split brcm_pcie_setup() into two funcs
> PCI: brcmstb: Add mechanism to turn on subdev regulators
> PCI: brcmstb: oAdd control of subdevice voltage regulators
> PCI: brcmstb: Do not turn off WOL regulators on suspend
>
> drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
> 1 file changed, 227 insertions(+), 30 deletions(-)
I'm assuming there's a v2 coming soonish? We should see -rc7 this
weekend and likely a final v5.19 release on July 24, so v5.20 material
should be tidied up by then.
Bjorn