In a future commit, a new handler will be introduced that in part does
reads and writes to some of the PCIe registers. When this handler is
invoked, it is paramount that it does not do these register accesses when
the PCIe bridge is inactive, as this will cause CPU abort errors.
To solve this we keep a spinlock that guards a variable which indicates
whether the bridge is on or off. When the bridge is on, access of the PCIe
HW registers may proceed.
Since there are multiple ways to reset the bridge, we introduce a general
function to obtain the spinlock, call the specific function that is used
for the specific SoC, sets the bridge active indicator variable, and
releases the spinlock.
Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
drivers/pci/controller/pcie-brcmstb.c | 40 +++++++++++++++++++++++----
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 92887b394eb4..400854c893d8 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -29,6 +29,7 @@
#include <linux/reset.h>
#include <linux/sizes.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/string.h>
#include <linux/types.h>
@@ -254,6 +255,7 @@ struct pcie_cfg_data {
int (*perst_set)(struct brcm_pcie *pcie, u32 val);
int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
int (*post_setup)(struct brcm_pcie *pcie);
+ bool has_err_report;
};
struct subdev_regulators {
@@ -299,6 +301,8 @@ struct brcm_pcie {
struct subdev_regulators *sr;
bool ep_wakeup_capable;
const struct pcie_cfg_data *cfg;
+ bool bridge_on;
+ spinlock_t bridge_lock;
};
static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -306,6 +310,24 @@ static inline bool is_bmips(const struct brcm_pcie *pcie)
return pcie->cfg->soc_base == BCM7435 || pcie->cfg->soc_base == BCM7425;
}
+static inline int brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
+{
+ unsigned long flags;
+ int ret;
+
+ if (pcie->cfg->has_err_report)
+ spin_lock_irqsave(&pcie->bridge_lock, flags);
+
+ ret = pcie->cfg->bridge_sw_init_set(pcie, val);
+ if (ret)
+ pcie->bridge_on = !val;
+
+ if (pcie->cfg->has_err_report)
+ spin_unlock_irqrestore(&pcie->bridge_lock, flags);
+
+ return ret;
+}
+
/*
* This is to convert the size of the inbound "BAR" region to the
* non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1078,7 +1100,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
int memc, ret;
/* Reset the bridge */
- ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
+ ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
if (ret)
return ret;
@@ -1094,7 +1116,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
usleep_range(100, 200);
/* Take the bridge out of reset */
- ret = pcie->cfg->bridge_sw_init_set(pcie, 0);
+ ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
if (ret)
return ret;
@@ -1545,7 +1567,7 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
if (!(pcie->cfg->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN))
/* Shutdown PCIe bridge */
- ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
+ ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
return ret;
}
@@ -1633,7 +1655,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
goto err_reset;
/* Take bridge out of reset so we can access the SERDES reg */
- pcie->cfg->bridge_sw_init_set(pcie, 0);
+ ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
+ if (ret)
+ goto err_reset;
/* SERDES_IDDQ = 0 */
tmp = readl(base + HARD_DEBUG(pcie));
@@ -1901,7 +1925,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (ret)
return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
- pcie->cfg->bridge_sw_init_set(pcie, 0);
+ ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "could not un-reset the bridge\n");
if (pcie->swinit_reset) {
ret = reset_control_assert(pcie->swinit_reset);
@@ -1976,6 +2003,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
+ if (pcie->cfg->has_err_report)
+ spin_lock_init(&pcie->bridge_lock);
+
return 0;
fail:
--
2.34.1On Fri, Jun 13, 2025 at 06:08:42PM -0400, Jim Quinlan wrote:
> In a future commit, a new handler will be introduced that in part does
> reads and writes to some of the PCIe registers. When this handler is
> invoked, it is paramount that it does not do these register accesses when
> the PCIe bridge is inactive, as this will cause CPU abort errors.
>
> To solve this we keep a spinlock that guards a variable which indicates
> whether the bridge is on or off. When the bridge is on, access of the PCIe
> HW registers may proceed.
>
> Since there are multiple ways to reset the bridge, we introduce a general
> function to obtain the spinlock, call the specific function that is used
> for the specific SoC, sets the bridge active indicator variable, and
> releases the spinlock.
>
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 40 +++++++++++++++++++++++----
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 92887b394eb4..400854c893d8 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -29,6 +29,7 @@
> #include <linux/reset.h>
> #include <linux/sizes.h>
> #include <linux/slab.h>
> +#include <linux/spinlock.h>
> #include <linux/string.h>
> #include <linux/types.h>
>
> @@ -254,6 +255,7 @@ struct pcie_cfg_data {
> int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> int (*post_setup)(struct brcm_pcie *pcie);
> + bool has_err_report;
It doesn't look worth it to me to add this. It only avoids locking in
a non-performance path.
> };
>
> struct subdev_regulators {
> @@ -299,6 +301,8 @@ struct brcm_pcie {
> struct subdev_regulators *sr;
> bool ep_wakeup_capable;
> const struct pcie_cfg_data *cfg;
> + bool bridge_on;
> + spinlock_t bridge_lock;
> };
>
> static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -306,6 +310,24 @@ static inline bool is_bmips(const struct brcm_pcie *pcie)
> return pcie->cfg->soc_base == BCM7435 || pcie->cfg->soc_base == BCM7425;
> }
>
> +static inline int brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
> +{
> + unsigned long flags;
> + int ret;
> +
> + if (pcie->cfg->has_err_report)
> + spin_lock_irqsave(&pcie->bridge_lock, flags);
> +
> + ret = pcie->cfg->bridge_sw_init_set(pcie, val);
> + if (ret)
> + pcie->bridge_on = !val;
AFAICT, .bridge_sw_init_set(1) asserts reset, .bridge_sw_init_set(0)
deasserts reset, and it returns 0 for success, so I'm confused about
this. If either assert or deassert failed (ret != 0), I guess we
don't know the state of the bridge and can't assume it's active, so I
would have expected something like:
ret = pcie->cfg->bridge_sw_init_set(pcie, val);
if (ret)
pcie->bridge_on = false;
else
pcie->bridge_on = !val;
Tangent: the last "return ret" in brcm_pcie_bridge_sw_init_set_generic()
should be "return 0" and drop the unnecessary initialization of "ret".
And the code there would be vastly improved by using FIELD_PREP() or
u32p_replace_bits() and getting rid of the shifting.
> + if (pcie->cfg->has_err_report)
> + spin_unlock_irqrestore(&pcie->bridge_lock, flags);
> +
> + return ret;
> +}
> +
> /*
> * This is to convert the size of the inbound "BAR" region to the
> * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> @@ -1078,7 +1100,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> int memc, ret;
>
> /* Reset the bridge */
> - ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
> + ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
> if (ret)
> return ret;
>
> @@ -1094,7 +1116,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> usleep_range(100, 200);
>
> /* Take the bridge out of reset */
> - ret = pcie->cfg->bridge_sw_init_set(pcie, 0);
> + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> if (ret)
> return ret;
>
> @@ -1545,7 +1567,7 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
>
> if (!(pcie->cfg->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN))
> /* Shutdown PCIe bridge */
> - ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
> + ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
>
> return ret;
> }
> @@ -1633,7 +1655,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
> goto err_reset;
>
> /* Take bridge out of reset so we can access the SERDES reg */
> - pcie->cfg->bridge_sw_init_set(pcie, 0);
> + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> + if (ret)
> + goto err_reset;
>
> /* SERDES_IDDQ = 0 */
> tmp = readl(base + HARD_DEBUG(pcie));
> @@ -1901,7 +1925,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>
> - pcie->cfg->bridge_sw_init_set(pcie, 0);
> + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "could not un-reset the bridge\n");
"un-reset" doesn't mean anything to me. Is this the same as "could
not take the bridge out of reset"? Or maybe "could not deassert
bridge reset"?
> if (pcie->swinit_reset) {
> ret = reset_control_assert(pcie->swinit_reset);
> @@ -1976,6 +2003,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> + if (pcie->cfg->has_err_report)
> + spin_lock_init(&pcie->bridge_lock);
> +
> return 0;
>
> fail:
> --
> 2.34.1
>
On Wed, Aug 6, 2025 at 3:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 13, 2025 at 06:08:42PM -0400, Jim Quinlan wrote:
> > In a future commit, a new handler will be introduced that in part does
> > reads and writes to some of the PCIe registers. When this handler is
> > invoked, it is paramount that it does not do these register accesses when
> > the PCIe bridge is inactive, as this will cause CPU abort errors.
> >
> > To solve this we keep a spinlock that guards a variable which indicates
> > whether the bridge is on or off. When the bridge is on, access of the PCIe
> > HW registers may proceed.
> >
> > Since there are multiple ways to reset the bridge, we introduce a general
> > function to obtain the spinlock, call the specific function that is used
> > for the specific SoC, sets the bridge active indicator variable, and
> > releases the spinlock.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 40 +++++++++++++++++++++++----
> > 1 file changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 92887b394eb4..400854c893d8 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -29,6 +29,7 @@
> > #include <linux/reset.h>
> > #include <linux/sizes.h>
> > #include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > #include <linux/string.h>
> > #include <linux/types.h>
> >
> > @@ -254,6 +255,7 @@ struct pcie_cfg_data {
> > int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > int (*post_setup)(struct brcm_pcie *pcie);
> > + bool has_err_report;
>
> It doesn't look worth it to me to add this. It only avoids locking in
> a non-performance path.
>
> > };
> >
> > struct subdev_regulators {
> > @@ -299,6 +301,8 @@ struct brcm_pcie {
> > struct subdev_regulators *sr;
> > bool ep_wakeup_capable;
> > const struct pcie_cfg_data *cfg;
> > + bool bridge_on;
> > + spinlock_t bridge_lock;
> > };
> >
> > static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -306,6 +310,24 @@ static inline bool is_bmips(const struct brcm_pcie *pcie)
> > return pcie->cfg->soc_base == BCM7435 || pcie->cfg->soc_base == BCM7425;
> > }
> >
> > +static inline int brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
> > +{
> > + unsigned long flags;
> > + int ret;
> > +
> > + if (pcie->cfg->has_err_report)
> > + spin_lock_irqsave(&pcie->bridge_lock, flags);
> > +
> > + ret = pcie->cfg->bridge_sw_init_set(pcie, val);
> > + if (ret)
> > + pcie->bridge_on = !val;
>
> AFAICT, .bridge_sw_init_set(1) asserts reset, .bridge_sw_init_set(0)
> deasserts reset, and it returns 0 for success, so I'm confused about
> this. If either assert or deassert failed (ret != 0), I guess we
> don't know the state of the bridge and can't assume it's active, so I
> would have expected something like:
>
> ret = pcie->cfg->bridge_sw_init_set(pcie, val);
> if (ret)
> pcie->bridge_on = false;
> else
> pcie->bridge_on = !val;
Ack
>
> Tangent: the last "return ret" in brcm_pcie_bridge_sw_init_set_generic()
> should be "return 0" and drop the unnecessary initialization of "ret".
Ack
>
> And the code there would be vastly improved by using FIELD_PREP() or
> u32p_replace_bits() and getting rid of the shifting.
Ack
>
> > + if (pcie->cfg->has_err_report)
> > + spin_unlock_irqrestore(&pcie->bridge_lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * This is to convert the size of the inbound "BAR" region to the
> > * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> > @@ -1078,7 +1100,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > int memc, ret;
> >
> > /* Reset the bridge */
> > - ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
> > + ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
> > if (ret)
> > return ret;
> >
> > @@ -1094,7 +1116,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > usleep_range(100, 200);
> >
> > /* Take the bridge out of reset */
> > - ret = pcie->cfg->bridge_sw_init_set(pcie, 0);
> > + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> > if (ret)
> > return ret;
> >
> > @@ -1545,7 +1567,7 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
> >
> > if (!(pcie->cfg->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN))
> > /* Shutdown PCIe bridge */
> > - ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
> > + ret = brcm_pcie_bridge_sw_init_set(pcie, 1);
> >
> > return ret;
> > }
> > @@ -1633,7 +1655,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
> > goto err_reset;
> >
> > /* Take bridge out of reset so we can access the SERDES reg */
> > - pcie->cfg->bridge_sw_init_set(pcie, 0);
> > + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> > + if (ret)
> > + goto err_reset;
> >
> > /* SERDES_IDDQ = 0 */
> > tmp = readl(base + HARD_DEBUG(pcie));
> > @@ -1901,7 +1925,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> > - pcie->cfg->bridge_sw_init_set(pcie, 0);
> > + ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "could not un-reset the bridge\n");
>
> "un-reset" doesn't mean anything to me. Is this the same as "could
> not take the bridge out of reset"? Or maybe "could not deassert
> bridge reset"?
Ack
Thanks,
Jim Quinlan
Broadcom STB/CM
>
> > if (pcie->swinit_reset) {
> > ret = reset_control_assert(pcie->swinit_reset);
> > @@ -1976,6 +2003,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + if (pcie->cfg->has_err_report)
> > + spin_lock_init(&pcie->bridge_lock);
> > +
> > return 0;
> >
> > fail:
> > --
> > 2.34.1
> >
On 6/13/25 15:08, Jim Quinlan wrote: > In a future commit, a new handler will be introduced that in part does > reads and writes to some of the PCIe registers. When this handler is > invoked, it is paramount that it does not do these register accesses when > the PCIe bridge is inactive, as this will cause CPU abort errors. > > To solve this we keep a spinlock that guards a variable which indicates > whether the bridge is on or off. When the bridge is on, access of the PCIe > HW registers may proceed. > > Since there are multiple ways to reset the bridge, we introduce a general > function to obtain the spinlock, call the specific function that is used > for the specific SoC, sets the bridge active indicator variable, and > releases the spinlock. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> -- Florian
© 2016 - 2025 Red Hat, Inc.