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

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

[Jim Quinlan]

V2  -- As v1 included the minimal code to fix a regression.  v2 does
       the same but adds some improvements suggested by Bjorn.
    -- In the unlikely but possible case that some other driver
       starts using the port driver's dev_data field, do not overwrite
       it but issue an error and return.
    -- Functions probe() and resume() do similar operations but
       did them in a different order; make this order consistent
       for both.
    -- New commit to remove forward declarations.
    -- New commit for only the PCIe config-space access "refusal mode".
    -- brcm_pcie_linkup renamed to brcm_pcie_start_link
    -- Add '_noirq' to the brcm_pcie_{suspend,resume} function names
       to match the dev_pm_ops names.
    -- Changes to commit messages:
           o Explain why we are splitting a function in two parts.
           o s/RB/Root Port/

    NOTE for Bjorn: The two commits "add mechanism .." and "add control ..."
        would probably be more clear if they were squashed.  They are kept
        separate as the first one's code may someday be moved under the Port
        driver.  As such, there's some churn.

    NOTE for Bjorn: There is no hurry on Broadcom's side wrt which
        release cycle/phase this patchset is applied.  It goes in
        when you think it is ready.

V1 -- Resubmission of patchset after original was reverted for a
    regression.

    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/20220106160332.2143-1-jim2101024@gmail.com
    [2] https://bugzilla.kernel.org/show_bug.cgi?id=215925
    [3] https://lore.kernel.org/linux-pci/20220511201856.808690-1-helgaas@kernel.org/

Jim Quinlan (6):
  PCI: brcmstb: Remove unnecessary forward declarations
  PCI: brcmstb: Split brcm_pcie_setup() into two funcs
  PCI: brcmstb: Add "refusal mode" to preclude PCIe-induced CPU aborts
  PCI: brcmstb: Add mechanism to turn on subdev regulators
  PCI: brcmstb: Add control of subdevice voltage regulators
  PCI: brcmstb: Do not turn off WOL regulators on suspend

 drivers/pci/controller/pcie-brcmstb.c | 438 +++++++++++++++++++-------
 1 file changed, 332 insertions(+), 106 deletions(-)


base-commit: c5fe7a97f20c7f3070ac870144515c0fabc6b999
--
2.17.1

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

[Jim Quinlan]

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

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 149 +++++++++++++-------------
 1 file changed, 72 insertions(+), 77 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e61058e13818..bd88a0a46c63 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -190,11 +190,6 @@
 
 /* Forward declarations */
 struct brcm_pcie;
-static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val);
-static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val);
-static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
-static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
-static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -223,66 +218,6 @@ struct pcie_cfg_data {
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
 
-static const int pcie_offsets[] = {
-	[RGR1_SW_INIT_1] = 0x9210,
-	[EXT_CFG_INDEX]  = 0x9000,
-	[EXT_CFG_DATA]   = 0x9004,
-};
-
-static const int pcie_offsets_bmips_7425[] = {
-	[RGR1_SW_INIT_1] = 0x8010,
-	[EXT_CFG_INDEX]  = 0x8300,
-	[EXT_CFG_DATA]   = 0x8304,
-};
-
-static const struct pcie_cfg_data generic_cfg = {
-	.offsets	= pcie_offsets,
-	.type		= GENERIC,
-	.perst_set	= brcm_pcie_perst_set_generic,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm7425_cfg = {
-	.offsets	= pcie_offsets_bmips_7425,
-	.type		= BCM7425,
-	.perst_set	= brcm_pcie_perst_set_generic,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm7435_cfg = {
-	.offsets	= pcie_offsets,
-	.type		= BCM7435,
-	.perst_set	= brcm_pcie_perst_set_generic,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const struct pcie_cfg_data bcm4908_cfg = {
-	.offsets	= pcie_offsets,
-	.type		= BCM4908,
-	.perst_set	= brcm_pcie_perst_set_4908,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
-static const int pcie_offset_bcm7278[] = {
-	[RGR1_SW_INIT_1] = 0xc010,
-	[EXT_CFG_INDEX] = 0x9000,
-	[EXT_CFG_DATA] = 0x9004,
-};
-
-static const struct pcie_cfg_data bcm7278_cfg = {
-	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
-	.perst_set	= brcm_pcie_perst_set_7278,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
-};
-
-static const struct pcie_cfg_data bcm2711_cfg = {
-	.offsets	= pcie_offsets,
-	.type		= BCM2711,
-	.perst_set	= brcm_pcie_perst_set_generic,
-	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-};
-
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -775,18 +710,6 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
 	return base + DATA_ADDR(pcie);
 }
 
-static struct pci_ops brcm_pcie_ops = {
-	.map_bus = brcm_pcie_map_conf,
-	.read = pci_generic_config_read,
-	.write = pci_generic_config_write,
-};
-
-static struct pci_ops brcm_pcie_ops32 = {
-	.map_bus = brcm_pcie_map_conf32,
-	.read = pci_generic_config_read32,
-	.write = pci_generic_config_write32,
-};
-
 static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
@@ -1316,6 +1239,66 @@ static int brcm_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const int pcie_offsets[] = {
+	[RGR1_SW_INIT_1] = 0x9210,
+	[EXT_CFG_INDEX]  = 0x9000,
+	[EXT_CFG_DATA]   = 0x9004,
+};
+
+static const int pcie_offsets_bmips_7425[] = {
+	[RGR1_SW_INIT_1] = 0x8010,
+	[EXT_CFG_INDEX]  = 0x8300,
+	[EXT_CFG_DATA]   = 0x8304,
+};
+
+static const struct pcie_cfg_data generic_cfg = {
+	.offsets	= pcie_offsets,
+	.type		= GENERIC,
+	.perst_set	= brcm_pcie_perst_set_generic,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm7425_cfg = {
+	.offsets	= pcie_offsets_bmips_7425,
+	.type		= BCM7425,
+	.perst_set	= brcm_pcie_perst_set_generic,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm7435_cfg = {
+	.offsets	= pcie_offsets,
+	.type		= BCM7435,
+	.perst_set	= brcm_pcie_perst_set_generic,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const struct pcie_cfg_data bcm4908_cfg = {
+	.offsets	= pcie_offsets,
+	.type		= BCM4908,
+	.perst_set	= brcm_pcie_perst_set_4908,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
+static const int pcie_offset_bcm7278[] = {
+	[RGR1_SW_INIT_1] = 0xc010,
+	[EXT_CFG_INDEX] = 0x9000,
+	[EXT_CFG_DATA] = 0x9004,
+};
+
+static const struct pcie_cfg_data bcm7278_cfg = {
+	.offsets	= pcie_offset_bcm7278,
+	.type		= BCM7278,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
+static const struct pcie_cfg_data bcm2711_cfg = {
+	.offsets	= pcie_offsets,
+	.type		= BCM2711,
+	.perst_set	= brcm_pcie_perst_set_generic,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1328,6 +1311,18 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{},
 };
 
+static struct pci_ops brcm_pcie_ops = {
+	.map_bus = brcm_pcie_map_conf,
+	.read = pci_generic_config_read,
+	.write = pci_generic_config_write,
+};
+
+static struct pci_ops brcm_pcie_ops32 = {
+	.map_bus = brcm_pcie_map_conf32,
+	.read = pci_generic_config_read32,
+	.write = pci_generic_config_write32,
+};
+
 static int brcm_pcie_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node, *msi_np;
-- 
2.17.1

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

[Jim Quinlan]

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

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

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

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

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

Link: https://lore.kernel.org/r/20220106160332.2143-5-jim2101024@gmail.com
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 69 +++++++++++++++++----------
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bd88a0a46c63..c026446d5830 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -849,16 +849,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	struct device *dev = pcie->dev;
-	struct resource_entry *entry;
-	bool ssc_good = false;
-	struct resource *res;
-	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	int i, ret, memc;
+	int ret, memc;
 	u32 tmp, burst, aspm_support;
 
 	/* Reset the bridge */
@@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
 
+	/* Don't advertise L0s capability if 'aspm-no-l0s' */
+	aspm_support = PCIE_LINK_STATE_L1;
+	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+		aspm_support |= PCIE_LINK_STATE_L0S;
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	u32p_replace_bits(&tmp, aspm_support,
+		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+
+	/*
+	 * For config space accesses on the RC, show the right class for
+	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
+	 */
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+	u32p_replace_bits(&tmp, 0x060400,
+			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+
+	return 0;
+}
+
+static int brcm_pcie_start_link(struct brcm_pcie *pcie)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	struct device *dev = pcie->dev;
+	void __iomem *base = pcie->base;
+	struct resource_entry *entry;
+	struct resource *res;
+	int num_out_wins = 0;
+	u16 nlw, cls, lnksta;
+	bool ssc_good = false;
+	u32 tmp;
+	int ret, i;
+
 	/* Unassert the fundamental reset */
 	pcie->perst_set(pcie, 0);
 
@@ -998,24 +1025,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
-	/* Don't advertise L0s capability if 'aspm-no-l0s' */
-	aspm_support = PCIE_LINK_STATE_L1;
-	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
-		aspm_support |= PCIE_LINK_STATE_L0S;
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-	u32p_replace_bits(&tmp, aspm_support,
-		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-
-	/*
-	 * For config space accesses on the RC, show the right class for
-	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
-	 */
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-	u32p_replace_bits(&tmp, 0x060400,
-			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-
 	if (pcie->ssc) {
 		ret = brcm_pcie_set_ssc(pcie);
 		if (ret == 0)
@@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	ret = brcm_pcie_start_link(pcie);
+	if (ret)
+		goto err_reset;
+
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
@@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
+	ret = brcm_pcie_start_link(pcie);
+	if (ret)
+		goto fail;
+
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
 	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
-- 
2.17.1

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

[Jim Quinlan]

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

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

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c026446d5830..72219a4f3964 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -255,6 +255,7 @@ struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	bool			refusal_mode;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
 	if (pci_is_root_bus(bus))
 		return PCI_SLOT(devfn) ? NULL : base + where;
 
+	if (pcie->refusal_mode) {
+		/*
+		 * At this point we do not have PCIe link-up.  If there is
+		 * a config read or write access besides those targeting
+		 * the host bridge, our PCIe HW throws a CPU abort.  To
+		 * prevent this we return the NULL address.  The calling
+		 * functions -- pci_generic_config_*() -- will notice this
+		 * and not perform the access, and if it is a read access,
+		 * 0xffffffff is returned.
+		 */
+		return NULL;
+	}
+
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
 	writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
@@ -704,6 +718,11 @@ static void __iomem *brcm_pcie_map_conf32(struct pci_bus *bus, unsigned int devf
 	if (pci_is_root_bus(bus))
 		return PCI_SLOT(devfn) ? NULL : base + (where & ~0x3);
 
+	if (pcie->refusal_mode) {
+		/* See note above in brcm_pcie_map_conf() */
+		return NULL;
+	}
+
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, (where & ~3));
 	writel(idx, base + IDX_ADDR(pcie));
@@ -989,6 +1008,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 		dev_err(dev, "link down\n");
 		return -ENODEV;
 	}
+	pcie->refusal_mode = false;
 
 	if (!brcm_pcie_rc_mode(pcie)) {
 		dev_err(dev, "PCIe misconfigured; is in EP mode\n");
@@ -1134,6 +1154,8 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	void __iomem *base = pcie->base;
 	int tmp;
 
+	pcie->refusal_mode = true;
+
 	if (brcm_pcie_link_up(pcie))
 		brcm_pcie_enter_l23(pcie);
 	/* Assert fundamental reset */
@@ -1185,6 +1207,7 @@ static int brcm_pcie_resume(struct device *dev)
 	u32 tmp;
 	int ret;
 
+	pcie->refusal_mode = true;
 	base = pcie->base;
 	ret = clk_prepare_enable(pcie->clk);
 	if (ret)
@@ -1361,6 +1384,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->type = data->type;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
+	pcie->refusal_mode = true;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1

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

[Jim Quinlan]

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

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

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

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

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

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

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

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

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

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

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

Link: https://lore.kernel.org/r/20220106160332.2143-6-jim2101024@gmail.com
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 96 +++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 72219a4f3964..f98a5338fa8e 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -218,6 +219,11 @@ struct pcie_cfg_data {
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
 
+struct subdev_regulators {
+	unsigned int num_supplies;
+	struct regulator_bulk_data supplies[];
+};
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -1077,6 +1083,92 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	return 0;
 }
 
+static const char * const supplies[] = {
+	"vpcie3v3",
+	"vpcie3v3aux",
+	"vpcie12v",
+};
+
+static void *alloc_subdev_regulators(struct device *dev)
+{
+	const size_t size = sizeof(struct subdev_regulators)
+		+ sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
+	struct subdev_regulators *sr;
+	int i;
+
+	sr = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (sr) {
+		sr->num_supplies = ARRAY_SIZE(supplies);
+		for (i = 0; i < ARRAY_SIZE(supplies); i++)
+			sr->supplies[i].supply = supplies[i];
+	}
+
+	return sr;
+}
+
+static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct subdev_regulators *sr;
+	int ret;
+
+	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+		return 0;
+
+	if (dev->driver_data) {
+		/*
+		 * Oops, this is unfortunate.  We are using the port
+		 * driver's driver_data field to store our regulator info
+		 * and it appears that another driver started using it as
+		 * well.  If so, be a team player do not overwrite it.  We
+		 * may still be okay if there are no regulators.
+		 */
+		dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
+		return 0;
+	}
+
+	sr = alloc_subdev_regulators(dev);
+	if (!sr)
+		return -ENOMEM;
+
+	/*
+	 * There is not much of a point to return an error as currently it
+	 * will cause a WARNING() from pci_alloc_child_bus().  So only
+	 * return the error if it is -ENOMEM.  Note that we are always
+	 * doing a dev_err() for other erros.
+	 */
+	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators for downstream device\n");
+		return 0;
+	}
+
+	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators for downstream device\n");
+		regulator_bulk_free(sr->num_supplies, sr->supplies);
+		return 0;
+	}
+
+	dev->driver_data = sr;
+
+	return 0;
+}
+
+static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct subdev_regulators *sr = dev->driver_data;
+
+	if (!dev->of_node || !sr || !bus->parent || !pci_is_root_bus(bus->parent))
+		return;
+
+	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
+		dev_err(dev, "failed to disable regulators for downstream device\n");
+	regulator_bulk_free(sr->num_supplies, sr->supplies);
+	dev->driver_data = NULL;
+}
+
 /* L23 is a low-power PCIe link state */
 static void brcm_pcie_enter_l23(struct brcm_pcie *pcie)
 {
@@ -1351,12 +1443,16 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
+	.add_bus = pci_subdev_regulators_add_bus,
+	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
 static struct pci_ops brcm_pcie_ops32 = {
 	.map_bus = brcm_pcie_map_conf32,
 	.read = pci_generic_config_read32,
 	.write = pci_generic_config_write32,
+	.add_bus = pci_subdev_regulators_add_bus,
+	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
 static int brcm_pcie_probe(struct platform_device *pdev)
-- 
2.17.1

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

[Jim Quinlan]

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

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

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

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

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

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

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index f98a5338fa8e..88339fde0df7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -262,6 +262,8 @@ struct brcm_pcie {
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	bool			refusal_mode;
+	bool			regulator_oops;
+	struct subdev_regulators *sr;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1112,10 +1114,37 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	struct subdev_regulators *sr;
 	int ret;
 
-	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+	sr = alloc_subdev_regulators(dev);
+	if (!sr)
+		return -ENOMEM;
+
+	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators for downstream device\n");
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators for downstream device\n");
+		regulator_bulk_free(sr->num_supplies, sr->supplies);
+		return ret;
+	}
+	dev->driver_data = sr;
+
+	return 0;
+}
+
+static int brcm_pcie_add_bus(struct pci_bus *bus)
+{
+	struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
+	struct device *dev = &bus->dev;
+	int ret;
+
+	if (!bus->parent || !pci_is_root_bus(bus->parent) || !pcie)
 		return 0;
 
-	if (dev->driver_data) {
+	if (dev->of_node && dev->driver_data) {
 		/*
 		 * Oops, this is unfortunate.  We are using the port
 		 * driver's driver_data field to store our regulator info
@@ -1124,12 +1153,19 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 		 * may still be okay if there are no regulators.
 		 */
 		dev_err(dev, "root port dev.driver_data non-NULL; something wrong\n");
-		return 0;
+
+	} else if (dev->of_node) {
+		ret = pci_subdev_regulators_add_bus(bus);
+		/* Grab the regulators for suspend/resume */
+		pcie->sr = bus->dev.driver_data;
 	}
 
-	sr = alloc_subdev_regulators(dev);
-	if (!sr)
-		return -ENOMEM;
+	/*
+	 * We return 0 and turn on the "refusal_mode" so that any further
+	 * accesses to the pci_dev just get 0xffffffff
+	 */
+	if (brcm_pcie_start_link(pcie) != 0)
+		pcie->refusal_mode = true;
 
 	/*
 	 * There is not much of a point to return an error as currently it
@@ -1137,22 +1173,7 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	 * return the error if it is -ENOMEM.  Note that we are always
 	 * doing a dev_err() for other erros.
 	 */
-	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
-	if (ret) {
-		dev_err(dev, "failed to get regulators for downstream device\n");
-		return 0;
-	}
-
-	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
-	if (ret) {
-		dev_err(dev, "failed to enable regulators for downstream device\n");
-		regulator_bulk_free(sr->num_supplies, sr->supplies);
-		return 0;
-	}
-
-	dev->driver_data = sr;
-
-	return 0;
+	return ret == -ENOMEM ? ret : 0;
 }
 
 static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
@@ -1160,15 +1181,28 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
 	struct device *dev = &bus->dev;
 	struct subdev_regulators *sr = dev->driver_data;
 
-	if (!dev->of_node || !sr || !bus->parent || !pci_is_root_bus(bus->parent))
-		return;
-
 	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
 		dev_err(dev, "failed to disable regulators for downstream device\n");
 	regulator_bulk_free(sr->num_supplies, sr->supplies);
 	dev->driver_data = NULL;
 }
 
+static void brcm_pcie_remove_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct brcm_pcie *pcie;
+
+	if (!dev->of_node || !dev->driver_data || !bus->parent ||
+	    !pci_is_root_bus(bus->parent))
+		return;
+
+	pcie = (struct brcm_pcie *) bus->sysdata;
+	if (pcie && pcie->sr) {
+		pci_subdev_regulators_remove_bus(bus);
+		pcie->sr = NULL;
+	}
+}
+
 /* L23 is a low-power PCIe link state */
 static void brcm_pcie_enter_l23(struct brcm_pcie *pcie)
 {
@@ -1267,7 +1301,7 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
-static int brcm_pcie_suspend(struct device *dev)
+static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	int ret;
@@ -1287,12 +1321,20 @@ static int brcm_pcie_suspend(struct device *dev)
 		return ret;
 	}
 
+	if (pcie->sr) {
+		ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
+		if (ret) {
+			dev_err(dev, "Could not turn off regulators\n");
+			reset_control_reset(pcie->rescal);
+			return ret;
+		}
+	}
 	clk_disable_unprepare(pcie->clk);
 
 	return 0;
 }
 
-static int brcm_pcie_resume(struct device *dev)
+static int brcm_pcie_resume_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	void __iomem *base;
@@ -1328,15 +1370,27 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	if (pcie->sr) {
+		ret = regulator_bulk_enable(pcie->sr->num_supplies,
+					    pcie->sr->supplies);
+		if (ret) {
+			dev_err(dev, "Could not turn on regulators\n");
+			goto err_reset;
+		}
+	}
+
 	ret = brcm_pcie_start_link(pcie);
 	if (ret)
-		goto err_reset;
+		goto err_regulator;
 
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
 	return 0;
 
+err_regulator:
+	if (pcie->sr)
+		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_reset:
 	reset_control_rearm(pcie->rescal);
 err_disable_clk:
@@ -1443,16 +1497,16 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
-	.add_bus = pci_subdev_regulators_add_bus,
-	.remove_bus = pci_subdev_regulators_remove_bus,
+	.add_bus = brcm_pcie_add_bus,
+	.remove_bus = brcm_pcie_remove_bus
 };
 
 static struct pci_ops brcm_pcie_ops32 = {
 	.map_bus = brcm_pcie_map_conf32,
 	.read = pci_generic_config_read32,
 	.write = pci_generic_config_write32,
-	.add_bus = pci_subdev_regulators_add_bus,
-	.remove_bus = pci_subdev_regulators_remove_bus,
+	.add_bus = brcm_pcie_add_bus,
+	.remove_bus = brcm_pcie_remove_bus
 };
 
 static int brcm_pcie_probe(struct platform_device *pdev)
@@ -1526,10 +1580,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
-	ret = brcm_pcie_start_link(pcie);
-	if (ret)
-		goto fail;
-
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
 	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
@@ -1551,7 +1601,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
-	return pci_host_probe(bridge);
+	ret = pci_host_probe(bridge);
+	if (!ret && !brcm_pcie_link_up(pcie))
+		ret = -ENODEV;
+
+	if (ret) {
+		brcm_pcie_remove(pdev);
+		return ret;
+	}
+
+	return 0;
+
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
@@ -1560,8 +1620,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 MODULE_DEVICE_TABLE(of, brcm_pcie_match);
 
 static const struct dev_pm_ops brcm_pcie_pm_ops = {
-	.suspend = brcm_pcie_suspend,
-	.resume = brcm_pcie_resume,
+	.suspend_noirq = brcm_pcie_suspend_noirq,
+	.resume_noirq = brcm_pcie_resume_noirq,
 };
 
 static struct platform_driver brcm_pcie_driver = {
-- 
2.17.1

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

[Jim Quinlan]

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/20220106160332.2143-8-jim2101024@gmail.com
Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 54 ++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 88339fde0df7..570c98594d47 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -264,6 +264,7 @@ struct brcm_pcie {
 	bool			refusal_mode;
 	bool			regulator_oops;
 	struct subdev_regulators *sr;
+	bool			ep_wakeup_capable;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1301,9 +1302,21 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
+{
+	bool *ret = data;
+
+	if (device_may_wakeup(&dev->dev)) {
+		*ret = true;
+		dev_info(&dev->dev, "disable cancelled for wake-up device\n");
+	}
+	return (int) *ret;
+}
+
 static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	int ret;
 
 	brcm_pcie_turn_off(pcie);
@@ -1322,11 +1335,22 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
 	}
 
 	if (pcie->sr) {
-		ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
-		if (ret) {
-			dev_err(dev, "Could not turn off regulators\n");
-			reset_control_reset(pcie->rescal);
-			return ret;
+		/*
+		 * Now turn off the regulators, but if at least one
+		 * downstream device is enabled as a wake-up source, do not
+		 * turn off regulators.
+		 */
+		pcie->ep_wakeup_capable = false;
+		pci_walk_bus(bridge->bus, pci_dev_may_wakeup,
+			     &pcie->ep_wakeup_capable);
+		if (!pcie->ep_wakeup_capable) {
+			ret = regulator_bulk_disable(pcie->sr->num_supplies,
+						     pcie->sr->supplies);
+			if (ret) {
+				dev_err(dev, "Could not turn off regulators\n");
+				reset_control_reset(pcie->rescal);
+				return ret;
+			}
 		}
 	}
 	clk_disable_unprepare(pcie->clk);
@@ -1371,11 +1395,21 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 		goto err_reset;
 
 	if (pcie->sr) {
-		ret = regulator_bulk_enable(pcie->sr->num_supplies,
-					    pcie->sr->supplies);
-		if (ret) {
-			dev_err(dev, "Could not turn on regulators\n");
-			goto err_reset;
+		if (pcie->ep_wakeup_capable) {
+			/*
+			 * We are resuming from a suspend.  In the suspend we
+			 * did not disable the power supplies, so there is
+			 * no need to enable them (and falsely increase their
+			 * usage count).
+			 */
+			pcie->ep_wakeup_capable = false;
+		} else {
+			ret = regulator_bulk_enable(pcie->sr->num_supplies,
+						    pcie->sr->supplies);
+			if (ret) {
+				dev_err(dev, "Could not turn on regulators\n");
+				goto err_reset;
+			}
 		}
 	}
 
-- 
2.17.1

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

[Pali Rohár]

Hello!

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

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

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

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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

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

Will do, thanks.

Jim Quinlan
Broadcom STB

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Bjorn Helgaas]

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

I can fix that up locally.

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



> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Pali Rohár]

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

Great!

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

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

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

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

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

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

[Bjorn Helgaas]

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

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

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

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

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

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

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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3432 bytes --]

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

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

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

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

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

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

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

Jim Quinlan
Broadcom STB

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Bjorn Helgaas]

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

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

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

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

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bd88a0a46c63..70cad1cbcbb4 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -852,14 +852,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	struct device *dev = pcie->dev;
+	int ret, memc;
+	u32 tmp, burst, aspm_support;
 	struct resource_entry *entry;
-	bool ssc_good = false;
 	struct resource *res;
 	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	int i, ret, memc;
-	u32 tmp, burst, aspm_support;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -948,25 +945,23 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
 
-	/* Unassert the fundamental reset */
-	pcie->perst_set(pcie, 0);
+	/* Don't advertise L0s capability if 'aspm-no-l0s' */
+	aspm_support = PCIE_LINK_STATE_L1;
+	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+		aspm_support |= PCIE_LINK_STATE_L0S;
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	u32p_replace_bits(&tmp, aspm_support,
+		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
 
 	/*
-	 * Give the RC/EP time to wake up, before trying to configure RC.
-	 * Intermittently check status for link-up, up to a total of 100ms.
+	 * For config space accesses on the RC, show the right class for
+	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
 	 */
-	for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)
-		msleep(5);
-
-	if (!brcm_pcie_link_up(pcie)) {
-		dev_err(dev, "link down\n");
-		return -ENODEV;
-	}
-
-	if (!brcm_pcie_rc_mode(pcie)) {
-		dev_err(dev, "PCIe misconfigured; is in EP mode\n");
-		return -EINVAL;
-	}
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+	u32p_replace_bits(&tmp, PCI_CLASS_BRIDGE_PCI_NORMAL,
+			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
 
 	resource_list_for_each_entry(entry, &bridge->windows) {
 		res = entry->res;
@@ -998,23 +993,37 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
-	/* Don't advertise L0s capability if 'aspm-no-l0s' */
-	aspm_support = PCIE_LINK_STATE_L1;
-	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
-		aspm_support |= PCIE_LINK_STATE_L0S;
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-	u32p_replace_bits(&tmp, aspm_support,
-		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	return 0;
+}
+
+static int brcm_pcie_start_link(struct brcm_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	void __iomem *base = pcie->base;
+	u16 nlw, cls, lnksta;
+	bool ssc_good = false;
+	u32 tmp;
+	int ret, i;
+
+	/* Unassert the fundamental reset */
+	pcie->perst_set(pcie, 0);
 
 	/*
-	 * For config space accesses on the RC, show the right class for
-	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
+	 * Give the RC/EP time to wake up, before trying to configure RC.
+	 * Intermittently check status for link-up, up to a total of 100ms.
 	 */
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-	u32p_replace_bits(&tmp, 0x060400,
-			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+	for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)
+		msleep(5);
+
+	if (!brcm_pcie_link_up(pcie)) {
+		dev_err(dev, "link down\n");
+		return -ENODEV;
+	}
+
+	if (!brcm_pcie_rc_mode(pcie)) {
+		dev_err(dev, "PCIe misconfigured; is in EP mode\n");
+		return -EINVAL;
+	}
 
 	if (pcie->ssc) {
 		ret = brcm_pcie_set_ssc(pcie);
@@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	ret = brcm_pcie_start_link(pcie);
+	if (ret)
+		goto err_reset;
+
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
@@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
+	ret = brcm_pcie_start_link(pcie);
+	if (ret)
+		goto fail;
+
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
 	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");

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

[Bjorn Helgaas]

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

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

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

Bjorn

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

[Bjorn Helgaas]

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

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

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

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

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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3515 bytes --]

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


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

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

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

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


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

Regards,
Jim Quinlan
Broadcom STB

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


>
>
> Bjorn

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Bjorn Helgaas]

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

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

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

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

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

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

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

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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 5877 bytes --]

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

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

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

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

Regards,
Jim Quinlan
Broadcom STB

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Rob Herring]

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

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

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

Rob

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

[Bjorn Helgaas]

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

Can we close on this?  Splitting into

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

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

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

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

Bjorn

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

[Florian Fainelli]

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

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

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

[Bjorn Helgaas]

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

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

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

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

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

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

[Bjorn Helgaas]

On Sat, Jul 16, 2022 at 06:24:50PM -0400, Jim Quinlan wrote:
> Our PCIe RC HW has an atypical behavior: if it does not have PCIe link
> established between itself and downstream, any subsequent config space
> access causes a CPU abort.  This commit sets a "refusal mode" if the PCIe
> link-up fails, and this has our pci_ops map_bus function returning a NULL
> address, which in turn precludes the access from happening.

> @@ -687,6 +688,19 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,

Oh, and I forgot to mention that brcmstb is one of the few drivers
that doesn't name these functions ".*_map_bus()".  It's helpful when
they all match a simple grep pattern.  Maybe a patch at the end could
fix this.

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

[Rob Herring]

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

(Need to fix your mailer to wrap lines)

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

Rob

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 5327 bytes --]

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

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

Regards,
Jim Quinlan
Broadcom STB

[1] https://lore.kernel.org/linux-pci/20171215201434.GY30595@bhelgaas-glaptop.roam.corp.google.com/

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

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

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

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

Regards,
Jim Quinlan
Broadcom STB


>
> Bjorn

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

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

[Bjorn Helgaas]

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

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

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

  brcm_pcie_start_link
    pcie->refusal_mode = false

  <link goes down>

  brcm_pcie_map_conf
    if (pcie->refusal_mode)            # still false

  <config access causes abort>

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

Bjorn

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

[Bjorn Helgaas]

On Thu, Jul 21, 2022 at 10:56:53AM -0400, Jim Quinlan wrote:
> On Wed, Jul 20, 2022 at 4:37 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote:
> > > > ...
> > >
> > > > So I think brcm_pcie_setup() does initialization that doesn't depend
> > > > on the link or any downstream devices, and brcm_pcie_start_link() does
> > > > things that depend on the link being up.  Right?
> > > >
> > > > If so, "start_link" might be a slight misnomer since AFAICT
> > > > brcm_pcie_start_link() doesn't do anything to initiate link-up except
> > > > maybe deasserting fundamental reset.  Some drivers start the LTSSM or
> > > > explicitly enable link training, but brcm_pcie_start_link() doesn't
> > > > seem to do anything like that.
> > > >
> > > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win().  Does
> > > > that really depend on the link being up?  If that only affects the
> > > > Root Port, maybe it could be done before link-up?
> > >
> > > What about the /* PCIe->SCB endian mode for BAR */ thing?  Does that
> > > depend on the link being up?
> > >
> > > And the "Refclk from RC should be gated with CLKREQ#" part?  Does that
> > > depend on the link being up?
> > >
> > > It seems obvious that brcm_pcie_set_ssc() and reading the negotiated
> > > link speed and width depend on the link being up.
> >
> > Can we close on this?  Splitting into
>
> Absolutely.
> 
> >   (a) stuff that can be initialized before the link is available and
> >   (b) stuff that depends on the link
> >
> > makes good sense, but then (b) should only contain stuff that actually
> > depends on the link.
> >
> > The "PCIe->SCB endian mode for BAR" *sounds* like something related to
> > the primary side of the RP, not the link.
> >
> > Not sure about "Refclk from RC".  RC would certainly be primary side,
> > but ASPM has to do with secondary (link) side.
> 
> I get the feedback, submission coming soon -- I was waiting for the
> email thread to conclude.

I don't expect a new posting of the patches in response to every
question, but hopefully this is a conversation that goes both ways,
and there's no need to slow down the conversation.  It's more than
acceptable to simply ask and answer questions and post updated patches
later.  We're running out of runway and I want to make sure we get
this thing done this cycle.

Bjorn