[PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Jim Quinlan]

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 (4):
  PCI: brcmstb: Split brcm_pcie_setup() into two funcs
  PCI: brcmstb: Add mechanism to turn on subdev regulators
  PCI: brcmstb: oAdd control of subdevice voltage regulators
  PCI: brcmstb: Do not turn off WOL regulators on suspend

 drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
 1 file changed, 227 insertions(+), 30 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.17.1

[PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

We need to take some code in brcm_pcie_setup() and put it in a new function
brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
be called indirectly by pci_host_probe() as opposed to the host driver
invoking it directly.

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

Link: https://lore.kernel.org/r/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 e61058e13818..2bf5cc399fd0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -926,16 +926,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	struct device *dev = pcie->dev;
-	struct resource_entry *entry;
-	bool ssc_good = false;
-	struct resource *res;
-	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	int i, ret, memc;
+	int ret, memc;
 	u32 tmp, burst, aspm_support;
 
 	/* Reset the bridge */
@@ -1025,6 +1018,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
 
+	/* Don't advertise L0s capability if 'aspm-no-l0s' */
+	aspm_support = PCIE_LINK_STATE_L1;
+	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+		aspm_support |= PCIE_LINK_STATE_L0S;
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	u32p_replace_bits(&tmp, aspm_support,
+		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+
+	/*
+	 * For config space accesses on the RC, show the right class for
+	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
+	 */
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+	u32p_replace_bits(&tmp, 0x060400,
+			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+
+	return 0;
+}
+
+static int brcm_pcie_linkup(struct brcm_pcie *pcie)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	struct device *dev = pcie->dev;
+	void __iomem *base = pcie->base;
+	struct resource_entry *entry;
+	struct resource *res;
+	int num_out_wins = 0;
+	u16 nlw, cls, lnksta;
+	bool ssc_good = false;
+	u32 tmp;
+	int ret, i;
+
 	/* Unassert the fundamental reset */
 	pcie->perst_set(pcie, 0);
 
@@ -1075,24 +1102,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
-	/* Don't advertise L0s capability if 'aspm-no-l0s' */
-	aspm_support = PCIE_LINK_STATE_L1;
-	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
-		aspm_support |= PCIE_LINK_STATE_L0S;
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-	u32p_replace_bits(&tmp, aspm_support,
-		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-
-	/*
-	 * For config space accesses on the RC, show the right class for
-	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
-	 */
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-	u32p_replace_bits(&tmp, 0x060400,
-			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-
 	if (pcie->ssc) {
 		ret = brcm_pcie_set_ssc(pcie);
 		if (ret == 0)
@@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto err_reset;
+
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
@@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto fail;
+
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
 	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
-- 
2.17.1

Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Bjorn Helgaas]

On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> We need to take some code in brcm_pcie_setup() and put it in a new function
> brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> be called indirectly by pci_host_probe() as opposed to the host driver
> invoking it directly.
>
> Some code that was executed after the PCIe linkup is now placed so that it
> executes prior to linkup, since this code has to run prior to the
> invocation of pci_host_probe().

This says we need to move some code from brcm_pcie_setup() to 
brcm_pcie_linkup(), but not *why* we need to do that.

In brcm_pcie_resume(), they're called together:

  brcm_pcie_resume
    brcm_pcie_setup
    brcm_pcie_linkup

In the probe path, they're not called together, but they're in the
same order:

  brcm_pcie_probe
    brcm_pcie_setup
    pci_host_probe
      ...
        brcm_pcie_add_bus		# bus->ops->add_bus
	  brcm_pcie_linkup

Is there something that must happen *between* them in the probe path?

> Link: https://lore.kernel.org/r/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 e61058e13818..2bf5cc399fd0 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -926,16 +926,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  
>  static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  {
> -	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
>  	u64 rc_bar2_offset, rc_bar2_size;
>  	void __iomem *base = pcie->base;
> -	struct device *dev = pcie->dev;
> -	struct resource_entry *entry;
> -	bool ssc_good = false;
> -	struct resource *res;
> -	int num_out_wins = 0;
> -	u16 nlw, cls, lnksta;
> -	int i, ret, memc;
> +	int ret, memc;
>  	u32 tmp, burst, aspm_support;
>  
>  	/* Reset the bridge */
> @@ -1025,6 +1018,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	if (pcie->gen)
>  		brcm_pcie_set_gen(pcie, pcie->gen);
>  
> +	/* Don't advertise L0s capability if 'aspm-no-l0s' */
> +	aspm_support = PCIE_LINK_STATE_L1;
> +	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> +		aspm_support |= PCIE_LINK_STATE_L0S;
> +	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> +	u32p_replace_bits(&tmp, aspm_support,
> +		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> +	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> +
> +	/*
> +	 * For config space accesses on the RC, show the right class for
> +	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
> +	 */
> +	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> +	u32p_replace_bits(&tmp, 0x060400,
> +			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> +	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> +
> +	return 0;
> +}
> +
> +static int brcm_pcie_linkup(struct brcm_pcie *pcie)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +	struct device *dev = pcie->dev;
> +	void __iomem *base = pcie->base;
> +	struct resource_entry *entry;
> +	struct resource *res;
> +	int num_out_wins = 0;
> +	u16 nlw, cls, lnksta;
> +	bool ssc_good = false;
> +	u32 tmp;
> +	int ret, i;
> +
>  	/* Unassert the fundamental reset */
>  	pcie->perst_set(pcie, 0);
>  
> @@ -1075,24 +1102,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  		num_out_wins++;
>  	}
>  
> -	/* Don't advertise L0s capability if 'aspm-no-l0s' */
> -	aspm_support = PCIE_LINK_STATE_L1;
> -	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> -		aspm_support |= PCIE_LINK_STATE_L0S;
> -	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> -	u32p_replace_bits(&tmp, aspm_support,
> -		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> -	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> -
> -	/*
> -	 * For config space accesses on the RC, show the right class for
> -	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
> -	 */
> -	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> -	u32p_replace_bits(&tmp, 0x060400,
> -			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> -	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> -
>  	if (pcie->ssc) {
>  		ret = brcm_pcie_set_ssc(pcie);
>  		if (ret == 0)
> @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
>  	if (ret)
>  		goto err_reset;
>  
> +	ret = brcm_pcie_linkup(pcie);
> +	if (ret)
> +		goto err_reset;
> +
>  	if (pcie->msi)
>  		brcm_msi_set_regs(pcie->msi);
>  
> @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail;
>  
> +	ret = brcm_pcie_linkup(pcie);
> +	if (ret)
> +		goto fail;
> +
>  	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
>  	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
>  		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

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

On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > We need to take some code in brcm_pcie_setup() and put it in a new function
> > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > be called indirectly by pci_host_probe() as opposed to the host driver
> > invoking it directly.
> >
> > Some code that was executed after the PCIe linkup is now placed so that it
> > executes prior to linkup, since this code has to run prior to the
> > invocation of pci_host_probe().
>
> This says we need to move some code from brcm_pcie_setup() to
> brcm_pcie_linkup(), but not *why* we need to do that.
I will elaborate in the commit message.
>
> In brcm_pcie_resume(), they're called together:
>
>   brcm_pcie_resume
>     brcm_pcie_setup
>     brcm_pcie_linkup
>
> In the probe path, they're not called together, but they're in the
> same order:
>
>   brcm_pcie_probe
>     brcm_pcie_setup
>     pci_host_probe
>       ...
>         brcm_pcie_add_bus               # bus->ops->add_bus
>           brcm_pcie_linkup
>
> Is there something that must happen *between* them in the probe path?

Yes.  In the probe() case, we must do things in this order:

1. brcm_pcie_setup()
2. Turn on regulators
3. brcm_pcie_linkup()

Since the voltage regulators are turned on during enumeration, pci_host_probe()
must be invoked prior to 3.  Before regulators, we did not care.

In the resume case, there is no enumeration of course but our driver
has a handle to
the regulators and can turn them on/off w/o help.

Regards,
Jim  Quinlan
Broradcom STB

>
> > Link: https://lore.kernel.org/r/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 e61058e13818..2bf5cc399fd0 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -926,16 +926,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> >
> >  static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >  {
> > -     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> >       u64 rc_bar2_offset, rc_bar2_size;
> >       void __iomem *base = pcie->base;
> > -     struct device *dev = pcie->dev;
> > -     struct resource_entry *entry;
> > -     bool ssc_good = false;
> > -     struct resource *res;
> > -     int num_out_wins = 0;
> > -     u16 nlw, cls, lnksta;
> > -     int i, ret, memc;
> > +     int ret, memc;
> >       u32 tmp, burst, aspm_support;
> >
> >       /* Reset the bridge */
> > @@ -1025,6 +1018,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >       if (pcie->gen)
> >               brcm_pcie_set_gen(pcie, pcie->gen);
> >
> > +     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > +     aspm_support = PCIE_LINK_STATE_L1;
> > +     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > +             aspm_support |= PCIE_LINK_STATE_L0S;
> > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > +     u32p_replace_bits(&tmp, aspm_support,
> > +             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > +
> > +     /*
> > +      * For config space accesses on the RC, show the right class for
> > +      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > +      */
> > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > +     u32p_replace_bits(&tmp, 0x060400,
> > +                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > +
> > +     return 0;
> > +}
> > +
> > +static int brcm_pcie_linkup(struct brcm_pcie *pcie)
> > +{
> > +     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > +     struct device *dev = pcie->dev;
> > +     void __iomem *base = pcie->base;
> > +     struct resource_entry *entry;
> > +     struct resource *res;
> > +     int num_out_wins = 0;
> > +     u16 nlw, cls, lnksta;
> > +     bool ssc_good = false;
> > +     u32 tmp;
> > +     int ret, i;
> > +
> >       /* Unassert the fundamental reset */
> >       pcie->perst_set(pcie, 0);
> >
> > @@ -1075,24 +1102,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >               num_out_wins++;
> >       }
> >
> > -     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > -     aspm_support = PCIE_LINK_STATE_L1;
> > -     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > -             aspm_support |= PCIE_LINK_STATE_L0S;
> > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > -     u32p_replace_bits(&tmp, aspm_support,
> > -             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > -
> > -     /*
> > -      * For config space accesses on the RC, show the right class for
> > -      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > -      */
> > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > -     u32p_replace_bits(&tmp, 0x060400,
> > -                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > -
> >       if (pcie->ssc) {
> >               ret = brcm_pcie_set_ssc(pcie);
> >               if (ret == 0)
> > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> >       if (ret)
> >               goto err_reset;
> >
> > +     ret = brcm_pcie_linkup(pcie);
> > +     if (ret)
> > +             goto err_reset;
> > +
> >       if (pcie->msi)
> >               brcm_msi_set_regs(pcie->msi);
> >
> > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > +     ret = brcm_pcie_linkup(pcie);
> > +     if (ret)
> > +             goto fail;
> > +
> >       pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> >       if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> >               dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > 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 v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > invoking it directly.
> > >
> > > Some code that was executed after the PCIe linkup is now placed so that it
> > > executes prior to linkup, since this code has to run prior to the
> > > invocation of pci_host_probe().
> >
> > This says we need to move some code from brcm_pcie_setup() to
> > brcm_pcie_linkup(), but not *why* we need to do that.
> I will elaborate in the commit message.
> >
> > In brcm_pcie_resume(), they're called together:
> >
> >   brcm_pcie_resume
> >     brcm_pcie_setup
> >     brcm_pcie_linkup
> >
> > In the probe path, they're not called together, but they're in the
> > same order:
> >
> >   brcm_pcie_probe
> >     brcm_pcie_setup
> >     pci_host_probe
> >       ...
> >         brcm_pcie_add_bus               # bus->ops->add_bus
> >           brcm_pcie_linkup
> >
> > Is there something that must happen *between* them in the probe path?
> 
> Yes.  In the probe() case, we must do things in this order:
> 
> 1. brcm_pcie_setup()
> 2. Turn on regulators
> 3. brcm_pcie_linkup()

Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:

  brcm_pcie_add_bus                    # bus->ops->add_bus
    pci_subdev_regulators_add_bus
      regulator_bulk_enable            # turn on regulators
    brcm_pcie_linkup

> Since the voltage regulators are turned on during enumeration,
> pci_host_probe() must be invoked prior to 3.  Before regulators, we
> did not care.

I guess in the pre-regulator case, i.e., pcie->sr not set, the power
for downstream devices must always be on.

> In the resume case, there is no enumeration of course but our driver
> has a handle to the regulators and can turn them on/off w/o help.

And I guess we don't need brcm_pcie_setup() in the resume path because
suspend turns off power only for downstream devices, not for the root
port itself, so the programming done by brcm_pcie_setup() doesn't need
to be done again.

> > > Link: https://lore.kernel.org/r/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 e61058e13818..2bf5cc399fd0 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -926,16 +926,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> > >
> > >  static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > >  {
> > > -     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > >       u64 rc_bar2_offset, rc_bar2_size;
> > >       void __iomem *base = pcie->base;
> > > -     struct device *dev = pcie->dev;
> > > -     struct resource_entry *entry;
> > > -     bool ssc_good = false;
> > > -     struct resource *res;
> > > -     int num_out_wins = 0;
> > > -     u16 nlw, cls, lnksta;
> > > -     int i, ret, memc;
> > > +     int ret, memc;
> > >       u32 tmp, burst, aspm_support;
> > >
> > >       /* Reset the bridge */
> > > @@ -1025,6 +1018,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > >       if (pcie->gen)
> > >               brcm_pcie_set_gen(pcie, pcie->gen);
> > >
> > > +     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > +     aspm_support = PCIE_LINK_STATE_L1;
> > > +     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > +             aspm_support |= PCIE_LINK_STATE_L0S;
> > > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > +     u32p_replace_bits(&tmp, aspm_support,
> > > +             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > +
> > > +     /*
> > > +      * For config space accesses on the RC, show the right class for
> > > +      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > +      */
> > > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > +     u32p_replace_bits(&tmp, 0x060400,
> > > +                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int brcm_pcie_linkup(struct brcm_pcie *pcie)
> > > +{
> > > +     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > +     struct device *dev = pcie->dev;
> > > +     void __iomem *base = pcie->base;
> > > +     struct resource_entry *entry;
> > > +     struct resource *res;
> > > +     int num_out_wins = 0;
> > > +     u16 nlw, cls, lnksta;
> > > +     bool ssc_good = false;
> > > +     u32 tmp;
> > > +     int ret, i;
> > > +
> > >       /* Unassert the fundamental reset */
> > >       pcie->perst_set(pcie, 0);
> > >
> > > @@ -1075,24 +1102,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > >               num_out_wins++;
> > >       }
> > >
> > > -     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > -     aspm_support = PCIE_LINK_STATE_L1;
> > > -     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > -             aspm_support |= PCIE_LINK_STATE_L0S;
> > > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > -     u32p_replace_bits(&tmp, aspm_support,
> > > -             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > -
> > > -     /*
> > > -      * For config space accesses on the RC, show the right class for
> > > -      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > -      */
> > > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > -     u32p_replace_bits(&tmp, 0x060400,
> > > -                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > -
> > >       if (pcie->ssc) {
> > >               ret = brcm_pcie_set_ssc(pcie);
> > >               if (ret == 0)
> > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> > >       if (ret)
> > >               goto err_reset;
> > >
> > > +     ret = brcm_pcie_linkup(pcie);
> > > +     if (ret)
> > > +             goto err_reset;
> > > +
> > >       if (pcie->msi)
> > >               brcm_msi_set_regs(pcie->msi);
> > >
> > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >       if (ret)
> > >               goto fail;
> > >
> > > +     ret = brcm_pcie_linkup(pcie);
> > > +     if (ret)
> > > +             goto fail;
> > > +
> > >       pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> > >       if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> > >               dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> > > --
> > > 2.17.1
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

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

On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > invoking it directly.
> > > >
> > > > Some code that was executed after the PCIe linkup is now placed so that it
> > > > executes prior to linkup, since this code has to run prior to the
> > > > invocation of pci_host_probe().
> > >
> > > This says we need to move some code from brcm_pcie_setup() to
> > > brcm_pcie_linkup(), but not *why* we need to do that.
> > I will elaborate in the commit message.
> > >
> > > In brcm_pcie_resume(), they're called together:
> > >
> > >   brcm_pcie_resume
> > >     brcm_pcie_setup
> > >     brcm_pcie_linkup
> > >
> > > In the probe path, they're not called together, but they're in the
> > > same order:
> > >
> > >   brcm_pcie_probe
> > >     brcm_pcie_setup
> > >     pci_host_probe
> > >       ...
> > >         brcm_pcie_add_bus               # bus->ops->add_bus
> > >           brcm_pcie_linkup
> > >
> > > Is there something that must happen *between* them in the probe path?
> >
> > Yes.  In the probe() case, we must do things in this order:
> >
> > 1. brcm_pcie_setup()
> > 2. Turn on regulators
> > 3. brcm_pcie_linkup()
>
> Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
>
>   brcm_pcie_add_bus                    # bus->ops->add_bus
>     pci_subdev_regulators_add_bus
>       regulator_bulk_enable            # turn on regulators
>     brcm_pcie_linkup
>
> > Since the voltage regulators are turned on during enumeration,
> > pci_host_probe() must be invoked prior to 3.  Before regulators, we
> > did not care.
>
> I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> for downstream devices must always be on.
>
> > In the resume case, there is no enumeration of course but our driver
> > has a handle to the regulators and can turn them on/off w/o help.
>
> And I guess we don't need brcm_pcie_setup() in the resume path because
> suspend turns off power only for downstream devices, not for the root
> port itself, so the programming done by brcm_pcie_setup() doesn't need
> to be done again.

I'm not sure I understand what you are saying -- brcm_pcie_setup()  is
called by brcm_pcie_resume()
because it is needed.  brcm_pcie_setup() isn't concerned with power it
 just does the preparation
required before attempting link-up.

Regards,
Jim Quinlan
Broadcom STB


>
> > > > Link: https://lore.kernel.org/r/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 e61058e13818..2bf5cc399fd0 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -926,16 +926,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> > > >
> > > >  static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > >  {
> > > > -     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > >       u64 rc_bar2_offset, rc_bar2_size;
> > > >       void __iomem *base = pcie->base;
> > > > -     struct device *dev = pcie->dev;
> > > > -     struct resource_entry *entry;
> > > > -     bool ssc_good = false;
> > > > -     struct resource *res;
> > > > -     int num_out_wins = 0;
> > > > -     u16 nlw, cls, lnksta;
> > > > -     int i, ret, memc;
> > > > +     int ret, memc;
> > > >       u32 tmp, burst, aspm_support;
> > > >
> > > >       /* Reset the bridge */
> > > > @@ -1025,6 +1018,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > >       if (pcie->gen)
> > > >               brcm_pcie_set_gen(pcie, pcie->gen);
> > > >
> > > > +     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > > +     aspm_support = PCIE_LINK_STATE_L1;
> > > > +     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > > +             aspm_support |= PCIE_LINK_STATE_L0S;
> > > > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > +     u32p_replace_bits(&tmp, aspm_support,
> > > > +             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > +
> > > > +     /*
> > > > +      * For config space accesses on the RC, show the right class for
> > > > +      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > > +      */
> > > > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > +     u32p_replace_bits(&tmp, 0x060400,
> > > > +                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int brcm_pcie_linkup(struct brcm_pcie *pcie)
> > > > +{
> > > > +     struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > > > +     struct device *dev = pcie->dev;
> > > > +     void __iomem *base = pcie->base;
> > > > +     struct resource_entry *entry;
> > > > +     struct resource *res;
> > > > +     int num_out_wins = 0;
> > > > +     u16 nlw, cls, lnksta;
> > > > +     bool ssc_good = false;
> > > > +     u32 tmp;
> > > > +     int ret, i;
> > > > +
> > > >       /* Unassert the fundamental reset */
> > > >       pcie->perst_set(pcie, 0);
> > > >
> > > > @@ -1075,24 +1102,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > >               num_out_wins++;
> > > >       }
> > > >
> > > > -     /* Don't advertise L0s capability if 'aspm-no-l0s' */
> > > > -     aspm_support = PCIE_LINK_STATE_L1;
> > > > -     if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> > > > -             aspm_support |= PCIE_LINK_STATE_L0S;
> > > > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > -     u32p_replace_bits(&tmp, aspm_support,
> > > > -             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > > > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > > > -
> > > > -     /*
> > > > -      * For config space accesses on the RC, show the right class for
> > > > -      * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > > > -      */
> > > > -     tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > -     u32p_replace_bits(&tmp, 0x060400,
> > > > -                       PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> > > > -     writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> > > > -
> > > >       if (pcie->ssc) {
> > > >               ret = brcm_pcie_set_ssc(pcie);
> > > >               if (ret == 0)
> > > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev)
> > > >       if (ret)
> > > >               goto err_reset;
> > > >
> > > > +     ret = brcm_pcie_linkup(pcie);
> > > > +     if (ret)
> > > > +             goto err_reset;
> > > > +
> > > >       if (pcie->msi)
> > > >               brcm_msi_set_regs(pcie->msi);
> > > >
> > > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > >       if (ret)
> > > >               goto fail;
> > > >
> > > > +     ret = brcm_pcie_linkup(pcie);
> > > > +     if (ret)
> > > > +             goto fail;
> > > > +
> > > >       pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> > > >       if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> > > >               dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> > > > --
> > > > 2.17.1
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> > _______________________________________________
> > 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 v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > invoking it directly.
> > > > >
> > > > > Some code that was executed after the PCIe linkup is now placed so that it
> > > > > executes prior to linkup, since this code has to run prior to the
> > > > > invocation of pci_host_probe().
> > > >
> > > > This says we need to move some code from brcm_pcie_setup() to
> > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > I will elaborate in the commit message.
> > > >
> > > > In brcm_pcie_resume(), they're called together:
> > > >
> > > >   brcm_pcie_resume
> > > >     brcm_pcie_setup
> > > >     brcm_pcie_linkup
> > > >
> > > > In the probe path, they're not called together, but they're in the
> > > > same order:
> > > >
> > > >   brcm_pcie_probe
> > > >     brcm_pcie_setup
> > > >     pci_host_probe
> > > >       ...
> > > >         brcm_pcie_add_bus               # bus->ops->add_bus
> > > >           brcm_pcie_linkup
> > > >
> > > > Is there something that must happen *between* them in the probe path?
> > >
> > > Yes.  In the probe() case, we must do things in this order:
> > >
> > > 1. brcm_pcie_setup()
> > > 2. Turn on regulators
> > > 3. brcm_pcie_linkup()
> >
> > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> >
> >   brcm_pcie_add_bus                    # bus->ops->add_bus
> >     pci_subdev_regulators_add_bus
> >       regulator_bulk_enable            # turn on regulators
> >     brcm_pcie_linkup
> >
> > > Since the voltage regulators are turned on during enumeration,
> > > pci_host_probe() must be invoked prior to 3.  Before regulators, we
> > > did not care.
> >
> > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > for downstream devices must always be on.
> >
> > > In the resume case, there is no enumeration of course but our driver
> > > has a handle to the regulators and can turn them on/off w/o help.
> >
> > And I guess we don't need brcm_pcie_setup() in the resume path because
> > suspend turns off power only for downstream devices, not for the root
> > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > to be done again.
> 
> I'm not sure I understand what you are saying -- brcm_pcie_setup()  is
> called by brcm_pcie_resume()
> because it is needed.  brcm_pcie_setup() isn't concerned with power it
>  just does the preparation
> required before attempting link-up.

Oh, sorry, I totally misread that.

But I wonder about the fact that probe and resume do these in
different orders:

  brcm_pcie_probe
    brcm_pcie_setup                          # setup
    pci_host_probe
      ...
        brcm_pcie_add_bus
	  pci_subdev_regulators_add_bus
	    regulator_bulk_enable            # regulators on
	  brcm_pcie_linkup                   # linkup

  brcm_pcie_resume
    regulator_bulk_enable                    # regulators on
    brcm_pcie_setup                          # setup
    brcm_pcie_linkup                         # linkup

Maybe pci_subdev_regulators_add_bus() could be done directly from
brcm_pcie_probe() instead of in brcm_pcie_add_bus()?  If the
regulators must be directly under the root port node in DT, it seems
like it would be reasonable to look for them in the probe path, which
seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
pcie-rockchip-host.c do.

Or maybe brcm_pcie_resume() should enable the regulators after
brcm_pcie_setup() so it's the same order as the probe path?

Bjorn

Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Jim Quinlan]

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

On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > > invoking it directly.
> > > > > >
> > > > > > Some code that was executed after the PCIe linkup is now placed so that it
> > > > > > executes prior to linkup, since this code has to run prior to the
> > > > > > invocation of pci_host_probe().
> > > > >
> > > > > This says we need to move some code from brcm_pcie_setup() to
> > > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > > I will elaborate in the commit message.
> > > > >
> > > > > In brcm_pcie_resume(), they're called together:
> > > > >
> > > > >   brcm_pcie_resume
> > > > >     brcm_pcie_setup
> > > > >     brcm_pcie_linkup
> > > > >
> > > > > In the probe path, they're not called together, but they're in the
> > > > > same order:
> > > > >
> > > > >   brcm_pcie_probe
> > > > >     brcm_pcie_setup
> > > > >     pci_host_probe
> > > > >       ...
> > > > >         brcm_pcie_add_bus               # bus->ops->add_bus
> > > > >           brcm_pcie_linkup
> > > > >
> > > > > Is there something that must happen *between* them in the probe path?
> > > >
> > > > Yes.  In the probe() case, we must do things in this order:
> > > >
> > > > 1. brcm_pcie_setup()
> > > > 2. Turn on regulators
> > > > 3. brcm_pcie_linkup()
> > >
> > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> > >
> > >   brcm_pcie_add_bus                    # bus->ops->add_bus
> > >     pci_subdev_regulators_add_bus
> > >       regulator_bulk_enable            # turn on regulators
> > >     brcm_pcie_linkup
> > >
> > > > Since the voltage regulators are turned on during enumeration,
> > > > pci_host_probe() must be invoked prior to 3.  Before regulators, we
> > > > did not care.
> > >
> > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > > for downstream devices must always be on.
> > >
> > > > In the resume case, there is no enumeration of course but our driver
> > > > has a handle to the regulators and can turn them on/off w/o help.
> > >
> > > And I guess we don't need brcm_pcie_setup() in the resume path because
> > > suspend turns off power only for downstream devices, not for the root
> > > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > > to be done again.
> >
> > I'm not sure I understand what you are saying -- brcm_pcie_setup()  is
> > called by brcm_pcie_resume()
> > because it is needed.  brcm_pcie_setup() isn't concerned with power it
> >  just does the preparation
> > required before attempting link-up.
>
> Oh, sorry, I totally misread that.
>
> But I wonder about the fact that probe and resume do these in
> different orders:
>
>   brcm_pcie_probe
>     brcm_pcie_setup                          # setup
>     pci_host_probe
>       ...
>         brcm_pcie_add_bus
>           pci_subdev_regulators_add_bus
>             regulator_bulk_enable            # regulators on
>           brcm_pcie_linkup                   # linkup
>
>   brcm_pcie_resume
>     regulator_bulk_enable                    # regulators on
>     brcm_pcie_setup                          # setup
>     brcm_pcie_linkup                         # linkup
>
brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(),
but your point is valid -- it is prudent to keep the orders
consistent. Let me think
about this.

> Maybe pci_subdev_regulators_add_bus() could be done directly from
> brcm_pcie_probe() instead of in brcm_pcie_add_bus()?
> regulators must be directly under the root port node in DT, it seems
> like it would be reasonable to look for them in the probe path, which
> seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
> pcie-rockchip-host.c do.
At some point in the original patchset -- IIRC -- the RC driver was
searching the DT
tree for regulators.  However, doing a "get" on these regulators is pretty much
impossible if the "owning" device does not exist.  I even had a version that
partially created the downstream device;  this pullreq was a mess and
got feedback which  put me on the current approach.

Reviews suggested  that the best location for the regulators should be located
in the root port DT node(s).  I agree with this. In addition, there
was a request to allow multiple regulators
to exist at each of the root ports in the downstream tree.  So if the RC driver
has to  potentially add multiple buses.  I really don't know how it
would do that,
and then call the pci_host_probe() w/o it failing.  Perhaps this is what ACPI
does before boot  -- I'm guessing here -- but I would also guess that it is
a decent amount of code as it is not far from doing enumeration.

One thing I could do is to allow the port driver's suspend/resume to do the
turning off/on of the regulators.  There are two issues with this: (1)
feedback suggested
to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable"
code would also have to live in the port driver and I'm not sure this
would be welcome.

>
> Or maybe brcm_pcie_resume() should enable the regulators after
> brcm_pcie_setup() so it's the same order as the probe path?
I  think I'll do this.

Thanks,
Jim Quinlan
Broadcom STB
>
> Bjorn

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

Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 04:38:30PM -0400, Jim Quinlan wrote:
> On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote:
> > > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote:
> > > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote:
> > > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function
> > > > > > > brcm_pcie_linkup().  In future commits the brcm_pcie_linkup() function will
> > > > > > > be called indirectly by pci_host_probe() as opposed to the host driver
> > > > > > > invoking it directly.
> > > > > > >
> > > > > > > Some code that was executed after the PCIe linkup is now placed so that it
> > > > > > > executes prior to linkup, since this code has to run prior to the
> > > > > > > invocation of pci_host_probe().
> > > > > >
> > > > > > This says we need to move some code from brcm_pcie_setup() to
> > > > > > brcm_pcie_linkup(), but not *why* we need to do that.
> > > > > I will elaborate in the commit message.
> > > > > >
> > > > > > In brcm_pcie_resume(), they're called together:
> > > > > >
> > > > > >   brcm_pcie_resume
> > > > > >     brcm_pcie_setup
> > > > > >     brcm_pcie_linkup
> > > > > >
> > > > > > In the probe path, they're not called together, but they're in the
> > > > > > same order:
> > > > > >
> > > > > >   brcm_pcie_probe
> > > > > >     brcm_pcie_setup
> > > > > >     pci_host_probe
> > > > > >       ...
> > > > > >         brcm_pcie_add_bus               # bus->ops->add_bus
> > > > > >           brcm_pcie_linkup
> > > > > >
> > > > > > Is there something that must happen *between* them in the probe path?
> > > > >
> > > > > Yes.  In the probe() case, we must do things in this order:
> > > > >
> > > > > 1. brcm_pcie_setup()
> > > > > 2. Turn on regulators
> > > > > 3. brcm_pcie_linkup()
> > > >
> > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus:
> > > >
> > > >   brcm_pcie_add_bus                    # bus->ops->add_bus
> > > >     pci_subdev_regulators_add_bus
> > > >       regulator_bulk_enable            # turn on regulators
> > > >     brcm_pcie_linkup
> > > >
> > > > > Since the voltage regulators are turned on during enumeration,
> > > > > pci_host_probe() must be invoked prior to 3.  Before regulators, we
> > > > > did not care.
> > > >
> > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power
> > > > for downstream devices must always be on.
> > > >
> > > > > In the resume case, there is no enumeration of course but our driver
> > > > > has a handle to the regulators and can turn them on/off w/o help.
> > > >
> > > > And I guess we don't need brcm_pcie_setup() in the resume path because
> > > > suspend turns off power only for downstream devices, not for the root
> > > > port itself, so the programming done by brcm_pcie_setup() doesn't need
> > > > to be done again.
> > >
> > > I'm not sure I understand what you are saying -- brcm_pcie_setup()  is
> > > called by brcm_pcie_resume()
> > > because it is needed.  brcm_pcie_setup() isn't concerned with power it
> > >  just does the preparation
> > > required before attempting link-up.
> >
> > Oh, sorry, I totally misread that.
> >
> > But I wonder about the fact that probe and resume do these in
> > different orders:
> >
> >   brcm_pcie_probe
> >     brcm_pcie_setup                          # setup
> >     pci_host_probe
> >       ...
> >         brcm_pcie_add_bus
> >           pci_subdev_regulators_add_bus
> >             regulator_bulk_enable            # regulators on
> >           brcm_pcie_linkup                   # linkup
> >
> >   brcm_pcie_resume
> >     regulator_bulk_enable                    # regulators on
> >     brcm_pcie_setup                          # setup
> >     brcm_pcie_linkup                         # linkup
> >
> brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(),
> but your point is valid -- it is prudent to keep the orders
> consistent. Let me think
> about this.
> 
> > Maybe pci_subdev_regulators_add_bus() could be done directly from
> > brcm_pcie_probe() instead of in brcm_pcie_add_bus()?
> > regulators must be directly under the root port node in DT, it seems
> > like it would be reasonable to look for them in the probe path, which
> > seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and
> > pcie-rockchip-host.c do.
> At some point in the original patchset -- IIRC -- the RC driver was
> searching the DT
> tree for regulators.  However, doing a "get" on these regulators is pretty much
> impossible if the "owning" device does not exist.  I even had a version that
> partially created the downstream device;  this pullreq was a mess and
> got feedback which  put me on the current approach.

Ah, I suppose because the regulators are not under the host bridge
itself, but under the *root port*, which is a PCI device that doesn't
exist until we enumerate it.  Although I guess the root port is
described in the DT, and the regulators are connected with that DT
description, not directly with the pci_dev.

> Reviews suggested  that the best location for the regulators should be located
> in the root port DT node(s).  I agree with this. In addition, there
> was a request to allow multiple regulators
> to exist at each of the root ports in the downstream tree.

Makes sense.

> So if the RC driver
> has to  potentially add multiple buses.  I really don't know how it
> would do that,
> and then call the pci_host_probe() w/o it failing.  Perhaps this is what ACPI
> does before boot  -- I'm guessing here -- but I would also guess that it is
> a decent amount of code as it is not far from doing enumeration.
> 
> One thing I could do is to allow the port driver's suspend/resume to do the
> turning off/on of the regulators.  There are two issues with this: (1)
> feedback suggested
> to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable"
> code would also have to live in the port driver and I'm not sure this
> would be welcome.
> 
> > Or maybe brcm_pcie_resume() should enable the regulators after
> > brcm_pcie_setup() so it's the same order as the probe path?
> I  think I'll do this.

Yep, sounds like the right thing.

Bjorn

[PATCH v1 2/4] 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 DT node "RB" under the host bridge DT node.
    -- During the RC driver's pci_host_probe() the add_bus callback
       is invoked where (bus->parent && pci_is_root_bus(bus->parent)
       is true

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

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

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

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

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

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

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

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

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2bf5cc399fd0..661d3834c6da 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+struct subdev_regulators {
+	unsigned int num_supplies;
+	struct regulator_bulk_data supplies[];
+};
+
+static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
+static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
 	return ssc && pll ? 0 : -EIO;
 }
 
+static void *alloc_subdev_regulators(struct device *dev)
+{
+	static const char * const supplies[] = {
+		"vpcie3v3",
+		"vpcie3v3aux",
+		"vpcie12v",
+	};
+	const size_t size = sizeof(struct subdev_regulators)
+		+ sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
+	struct subdev_regulators *sr;
+	int i;
+
+	sr = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (sr) {
+		sr->num_supplies = ARRAY_SIZE(supplies);
+		for (i = 0; i < ARRAY_SIZE(supplies); i++)
+			sr->supplies[i].supply = supplies[i];
+	}
+
+	return sr;
+}
+
+static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct subdev_regulators *sr;
+	int ret;
+
+	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
+		return 0;
+
+	if (dev->driver_data)
+		dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
+
+	sr = alloc_subdev_regulators(dev);
+	if (!sr)
+		return -ENOMEM;
+
+	dev->driver_data = sr;
+	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators for downstream device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
+{
+	struct device *dev = &bus->dev;
+	struct subdev_regulators *sr = dev->driver_data;
+
+	if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
+		return;
+
+	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
+		dev_err(dev, "failed to disable regulators for downstream device\n");
+	regulator_bulk_free(sr->num_supplies, sr->supplies);
+	dev->driver_data = NULL;
+}
+
 /* Limits operation to a specific generation (1, 2, or 3) */
 static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
 {
@@ -779,6 +854,8 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
+	.add_bus = pci_subdev_regulators_add_bus,
+	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
 static struct pci_ops brcm_pcie_ops32 = {
-- 
2.17.1

Re: [PATCH v1 2/4] PCI: brcmstb: Add mechanism to turn on subdev regulators

[Bjorn Helgaas]

On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> Add a mechanism to identify standard PCIe regulators in the DT, allocate
> them, and turn them on before the rest of the bus is scanned during
> pci_host_probe().
> 
> The allocated structure that contains the regulators is stored in the port
> driver dev.driver_data field.  Here is a point-by-point of how and when
> this mechanism is activated:
> 
> If:
>     -- PCIe RC driver sets pci_ops {add,remove)_bus to
>        pci_subdev_regulators_{add,remove}_bus during its probe.
>     -- There is a DT node "RB" under the host bridge DT node.

"RB" isn't mentioned in pcie-brcmstb.c.  What's the connection to it?
Is it just an example, and the actual name doesn't matter?

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

I guess this is all functionality that depends on new properties in
the DT?  Prior to this patch, pcie-brcmstb.c didn't do anything at all
with regulators, although brcm,stb-pcie.yaml does mention
"vpcie3v3-supply" in an example.

> The unabridged reason for doing this is as follows.  We would like the
> Broadcom STB PCIe root complex driver (and others) to be able to turn
> off/on regulators[1] that provide power to endpoint[2] devices.  Typically,
> the drivers of these endpoint devices are stock Linux drivers that are not
> aware that these regulator(s) exist and must be turned on for the driver to
> be probed.  The simple solution of course is to turn these regulators on at
> boot and keep them on.  However, this solution does not satisfy at least
> three of our usage modes:
> 
>   1. For example, one customer uses multiple PCIe controllers, but wants
>      the ability to, by script invoking and unbind, turn any or all of them
>      and their subdevices off to save power, e.g. when in battery mode.
> 
>   2. Another example is when a watchdog script discovers that an endpoint
>      device is in an unresponsive state and would like to unbind, power
>      toggle, and re-bind just the PCIe endpoint and controller.
> 
>   3. Of course we also want power turned off during suspend mode.  However,
>      some endpoint devices may be able to "wake" during suspend and we need
>      to recognise this case and veto the nominal act of turning off its
>      regulator.  Such is the case with Wake-on-LAN and Wake-on-WLAN support
>      where the PCIe endpoint device needs to be kept powered on in order to
>      receive network packets and wake the system.
> 
> In all of these cases it is advantageous for the PCIe controller to govern
> the turning off/on the regulators needed by the endpoint device.  The first
> two cases can be done by simply unbinding and binding the PCIe controller,
> if the controller has control of these regulators.
> 
> [1] These regulators typically govern the actual power supply to the
>     endpoint chip.  Sometimes they may be the official PCIe socket
>     power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
>     the regulator(s) that supply power to the EP chip.
> 
> [2] The 99% configuration of our boards is a single endpoint device
>     attached to the PCIe controller.  I use the term endpoint but it could
>     possibly mean a switch as well.
> 
> Link: https://lore.kernel.org/r/20220106160332.2143-6-jim2101024@gmail.com
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 77 +++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2bf5cc399fd0..661d3834c6da 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  };
>  
> +struct subdev_regulators {
> +	unsigned int num_supplies;
> +	struct regulator_bulk_data supplies[];
> +};
> +
> +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);

I think these forward declarations are unnecessary.  I can remove them
if you agree.

>  struct brcm_msi {
>  	struct device		*dev;
>  	void __iomem		*base;
> @@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
>  	return ssc && pll ? 0 : -EIO;
>  }
>  
> +static void *alloc_subdev_regulators(struct device *dev)
> +{
> +	static const char * const supplies[] = {
> +		"vpcie3v3",
> +		"vpcie3v3aux",
> +		"vpcie12v",
> +	};
> +	const size_t size = sizeof(struct subdev_regulators)
> +		+ sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
> +	struct subdev_regulators *sr;
> +	int i;
> +
> +	sr = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (sr) {
> +		sr->num_supplies = ARRAY_SIZE(supplies);
> +		for (i = 0; i < ARRAY_SIZE(supplies); i++)
> +			sr->supplies[i].supply = supplies[i];
> +	}
> +
> +	return sr;
> +}
> +
> +static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> +{
> +	struct device *dev = &bus->dev;
> +	struct subdev_regulators *sr;
> +	int ret;
> +
> +	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> +		return 0;
> +
> +	if (dev->driver_data)
> +		dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");

I guess you're using the pci_bus dev->driver_data.  I don't know of
other users of it, but there's really no ownership model for it.  If
it's non-NULL here, it means somebody else, e.g., the PCI core, is
already using it, and when you overwrite it below, you will break that
other user.

I think you should complain and return instead of breaking the other
user.  That will mean the regulator won't get enabled and your
endpoint won't work, but I think that's a better way to fail than by
overwriting somebody else's pointer, which may lead to memory
corruption that's very hard to debug.

> +	sr = alloc_subdev_regulators(dev);
> +	if (!sr)
> +		return -ENOMEM;
> +
> +	dev->driver_data = sr;
> +	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators for downstream device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> +{
> +	struct device *dev = &bus->dev;
> +	struct subdev_regulators *sr = dev->driver_data;
> +
> +	if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
> +		return;
> +
> +	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> +		dev_err(dev, "failed to disable regulators for downstream device\n");
> +	regulator_bulk_free(sr->num_supplies, sr->supplies);
> +	dev->driver_data = NULL;
> +}
> +
>  /* Limits operation to a specific generation (1, 2, or 3) */
>  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
>  {
> @@ -779,6 +854,8 @@ static struct pci_ops brcm_pcie_ops = {
>  	.map_bus = brcm_pcie_map_conf,
>  	.read = pci_generic_config_read,
>  	.write = pci_generic_config_write,
> +	.add_bus = pci_subdev_regulators_add_bus,
> +	.remove_bus = pci_subdev_regulators_remove_bus,
>  };
>  
>  static struct pci_ops brcm_pcie_ops32 = {
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] PCI: brcmstb: Add mechanism to turn on subdev regulators

[Jim Quinlan]

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

On Wed, Jul 6, 2022 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> > Add a mechanism to identify standard PCIe regulators in the DT, allocate
> > them, and turn them on before the rest of the bus is scanned during
> > pci_host_probe().
> >
> > The allocated structure that contains the regulators is stored in the port
> > driver dev.driver_data field.  Here is a point-by-point of how and when
> > this mechanism is activated:
> >
> > If:
> >     -- PCIe RC driver sets pci_ops {add,remove)_bus to
> >        pci_subdev_regulators_{add,remove}_bus during its probe.
> >     -- There is a DT node "RB" under the host bridge DT node.
>
> "RB" isn't mentioned in pcie-brcmstb.c.  What's the connection to it?
> Is it just an example, and the actual name doesn't matter?

I will reword this to something like "a regulator with one of these names
... under a root port DT node.  I will review/edit this entire commit msg.

>
> >     -- During the RC driver's pci_host_probe() the add_bus callback
> >        is invoked where (bus->parent && pci_is_root_bus(bus->parent)
> >        is true
> >
> > Then:
> >     -- A struct subdev_regulators structure will be allocated and
> >        assigned to bus->dev.driver_data.
> >     -- regulator_bulk_{get,enable} will be invoked on &bus->dev
> >        and the former will search for and process any
> >        vpcie{12v,3v3,3v3aux}-supply properties that reside in node "RB".
> >     -- The regulators will be turned off/on for any unbind/bind operations.
> >     -- The regulators will be turned off/on for any suspend/resumes, but
> >        only if the RC driver handles this on its own.  This will appear
> >        in a later commit for the pcie-brcmstb.c driver.
>
> I guess this is all functionality that depends on new properties in
> the DT?  Prior to this patch, pcie-brcmstb.c didn't do anything at all
> with regulators, although brcm,stb-pcie.yaml does mention
> "vpcie3v3-supply" in an example.

What is new in the DT is the presence of a regulator under a root
port node.  That is it.  I submitted the regulator YAML allowance
to Rob's Github repo  and I  believe it was accepted.

>
> > The unabridged reason for doing this is as follows.  We would like the
> > Broadcom STB PCIe root complex driver (and others) to be able to turn
> > off/on regulators[1] that provide power to endpoint[2] devices.  Typically,
> > the drivers of these endpoint devices are stock Linux drivers that are not
> > aware that these regulator(s) exist and must be turned on for the driver to
> > be probed.  The simple solution of course is to turn these regulators on at
> > boot and keep them on.  However, this solution does not satisfy at least
> > three of our usage modes:
> >
> >   1. For example, one customer uses multiple PCIe controllers, but wants
> >      the ability to, by script invoking and unbind, turn any or all of them
> >      and their subdevices off to save power, e.g. when in battery mode.
> >
> >   2. Another example is when a watchdog script discovers that an endpoint
> >      device is in an unresponsive state and would like to unbind, power
> >      toggle, and re-bind just the PCIe endpoint and controller.
> >
> >   3. Of course we also want power turned off during suspend mode.  However,
> >      some endpoint devices may be able to "wake" during suspend and we need
> >      to recognise this case and veto the nominal act of turning off its
> >      regulator.  Such is the case with Wake-on-LAN and Wake-on-WLAN support
> >      where the PCIe endpoint device needs to be kept powered on in order to
> >      receive network packets and wake the system.
> >
> > In all of these cases it is advantageous for the PCIe controller to govern
> > the turning off/on the regulators needed by the endpoint device.  The first
> > two cases can be done by simply unbinding and binding the PCIe controller,
> > if the controller has control of these regulators.
> >
> > [1] These regulators typically govern the actual power supply to the
> >     endpoint chip.  Sometimes they may be the official PCIe socket
> >     power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
> >     the regulator(s) that supply power to the EP chip.
> >
> > [2] The 99% configuration of our boards is a single endpoint device
> >     attached to the PCIe controller.  I use the term endpoint but it could
> >     possibly mean a switch as well.
> >
> > Link: https://lore.kernel.org/r/20220106160332.2143-6-jim2101024@gmail.com
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 77 +++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 2bf5cc399fd0..661d3834c6da 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/printk.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> >  #include <linux/sizes.h>
> >  #include <linux/slab.h>
> > @@ -283,6 +284,14 @@ static const struct pcie_cfg_data bcm2711_cfg = {
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >  };
> >
> > +struct subdev_regulators {
> > +     unsigned int num_supplies;
> > +     struct regulator_bulk_data supplies[];
> > +};
> > +
> > +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> > +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
>
> I think these forward declarations are unnecessary.  I can remove them
> if you agree.

It is up to you.  I have a commit-set ready that will make a number of
improvements to our driver.
One of them removes all forward declarations.  Other commits concern
other suggestions you
have made, e.g. rename brcm_pcie_linkup() to brcm_pcie_start_link().

>
> >  struct brcm_msi {
> >       struct device           *dev;
> >       void __iomem            *base;
> > @@ -436,6 +445,72 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> >       return ssc && pll ? 0 : -EIO;
> >  }
> >
> > +static void *alloc_subdev_regulators(struct device *dev)
> > +{
> > +     static const char * const supplies[] = {
> > +             "vpcie3v3",
> > +             "vpcie3v3aux",
> > +             "vpcie12v",
> > +     };
> > +     const size_t size = sizeof(struct subdev_regulators)
> > +             + sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
> > +     struct subdev_regulators *sr;
> > +     int i;
> > +
> > +     sr = devm_kzalloc(dev, size, GFP_KERNEL);
> > +     if (sr) {
> > +             sr->num_supplies = ARRAY_SIZE(supplies);
> > +             for (i = 0; i < ARRAY_SIZE(supplies); i++)
> > +                     sr->supplies[i].supply = supplies[i];
> > +     }
> > +
> > +     return sr;
> > +}
> > +
> > +static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> > +{
> > +     struct device *dev = &bus->dev;
> > +     struct subdev_regulators *sr;
> > +     int ret;
> > +
> > +     if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> > +             return 0;
> > +
> > +     if (dev->driver_data)
> > +             dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
>
> I guess you're using the pci_bus dev->driver_data.  I don't know of
> other users of it, but there's really no ownership model for it.  If
> it's non-NULL here, it means somebody else, e.g., the PCI core, is
> already using it, and when you overwrite it below, you will break that
> other user.

Yes, I'm not happy about this vulnerability.

>
> I think you should complain and return instead of breaking the other
> user.  That will mean the regulator won't get enabled and your
> endpoint won't work, but I think that's a better way to fail than by
> overwriting somebody else's pointer, which may lead to memory
> corruption that's very hard to debug.

Agree, will do in v2.

Regards,
Jim Quinlan
Broadcom STB
>
> > +     sr = alloc_subdev_regulators(dev);
> > +     if (!sr)
> > +             return -ENOMEM;
> > +
> > +     dev->driver_data = sr;
> > +     ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
> > +     if (ret) {
> > +             dev_err(dev, "failed to enable regulators for downstream device\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> > +{
> > +     struct device *dev = &bus->dev;
> > +     struct subdev_regulators *sr = dev->driver_data;
> > +
> > +     if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
> > +             return;
> > +
> > +     if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> > +             dev_err(dev, "failed to disable regulators for downstream device\n");
> > +     regulator_bulk_free(sr->num_supplies, sr->supplies);
> > +     dev->driver_data = NULL;
> > +}
> > +
> >  /* Limits operation to a specific generation (1, 2, or 3) */
> >  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> >  {
> > @@ -779,6 +854,8 @@ static struct pci_ops brcm_pcie_ops = {
> >       .map_bus = brcm_pcie_map_conf,
> >       .read = pci_generic_config_read,
> >       .write = pci_generic_config_write,
> > +     .add_bus = pci_subdev_regulators_add_bus,
> > +     .remove_bus = pci_subdev_regulators_remove_bus,
> >  };
> >
> >  static struct pci_ops brcm_pcie_ops32 = {
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > 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 v1 2/4] PCI: brcmstb: Add mechanism to turn on subdev regulators

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 10:14:11AM -0400, Jim Quinlan wrote:
> On Wed, Jul 6, 2022 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 01, 2022 at 12:27:23PM -0400, Jim Quinlan wrote:
> > > Add a mechanism to identify standard PCIe regulators in the DT, allocate
> > > them, and turn them on before the rest of the bus is scanned during
> > > pci_host_probe().
> > > ...

> > > +static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
> > > +static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
> >
> > I think these forward declarations are unnecessary.  I can remove them
> > if you agree.
> 
> It is up to you.  I have a commit-set ready that will make a number of
> improvements to our driver.
> One of them removes all forward declarations.  Other commits concern
> other suggestions you
> have made, e.g. rename brcm_pcie_linkup() to brcm_pcie_start_link().

If you're planning a v2, I'd drop the declarations there.  No point in
adding them, only to remove them in a future patch (unless we need
them in the interim, of course, but in this case I don't think we do).

Bjorn

[PATCH v1 3/4] 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.

[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 | 86 ++++++++++++++++++++++++---
 1 file changed, 77 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 661d3834c6da..a86bf502a265 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static int brcm_pcie_linkup(struct brcm_pcie *pcie);
+static int brcm_pcie_add_bus(struct pci_bus *bus);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -329,6 +331,8 @@ 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;
+	struct subdev_regulators *sr;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	return 0;
 }
 
+static int brcm_pcie_add_bus(struct pci_bus *bus)
+{
+	struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
+	int ret;
+
+	if (!bus->parent || !pci_is_root_bus(bus->parent))
+		return 0;
+
+	ret = pci_subdev_regulators_add_bus(bus);
+	if (ret)
+		return ret;
+
+	/* Grab the regulators for suspend/resume */
+	pcie->sr = bus->dev.driver_data;
+
+	/*
+	 * If we have failed linkup there is no point to return an error as
+	 * currently it will cause a WARNING() from pci_alloc_child_bus().
+	 * We return 0 and turn on the "refusal_mode" so that any further
+	 * accesses to the pci_dev just get 0xffffffff
+	 */
+	if (brcm_pcie_linkup(pcie) != 0)
+		pcie->refusal_mode = true;
+
+	return 0;
+}
+
 static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
 {
 	struct device *dev = &bus->dev;
@@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
 	/* Accesses to the RC go right to the RC registers if slot==0 */
 	if (pci_is_root_bus(bus))
 		return PCI_SLOT(devfn) ? NULL : base + where;
+	if (pcie->refusal_mode) {
+		/*
+		 * At this point we do not have link.  There will be a CPU
+		 * abort -- a quirk with this controller --if Linux tries
+		 * to read any config-space registers besides those
+		 * targeting the host bridge.  To prevent this we hijack
+		 * the address to point to a safe access that will return
+		 * 0xffffffff.
+		 */
+		writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI);
+		return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3);
+	}
 
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
@@ -854,7 +897,7 @@ 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,
+	.add_bus = brcm_pcie_add_bus,
 	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
@@ -1327,6 +1370,14 @@ 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;
@@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (pcie->sr) {
+		ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies);
+		if (ret) {
+			dev_err(dev, "Could not turn on regulators\n");
+			goto err_disable_clk;
+		}
+	}
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret)
-		goto err_disable_clk;
+		goto err_regulator;
 
 	ret = brcm_phy_start(pcie);
 	if (ret)
@@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev)
 
 err_reset:
 	reset_control_rearm(pcie->rescal);
+err_regulator:
+	if (pcie->sr)
+		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_disable_clk:
 	clk_disable_unprepare(pcie->clk);
 	return ret;
@@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
-	ret = brcm_pcie_linkup(pcie);
-	if (ret)
-		goto fail;
-
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
 	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
@@ -1513,7 +1571,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;
@@ -1522,8 +1590,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,
+	.resume_noirq = brcm_pcie_resume,
 };
 
 static struct platform_driver brcm_pcie_driver = {
-- 
2.17.1

Re: [PATCH v1 3/4] PCI: brcmstb: Add control of subdevice voltage regulators

[Bjorn Helgaas]

On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote:
> 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.
> 
> [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 | 86 ++++++++++++++++++++++++---
>  1 file changed, 77 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 661d3834c6da..a86bf502a265 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
>  static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
> +static int brcm_pcie_linkup(struct brcm_pcie *pcie);
> +static int brcm_pcie_add_bus(struct pci_bus *bus);

I think the brcm_pcie_add_bus() declaration is unnecessary.

The brcm_pcie_linkup() one is probably unnecessary, too, but would
require a lot of reordering that I don't think we should do in this
series.

>  enum {
>  	RGR1_SW_INIT_1,
> @@ -329,6 +331,8 @@ 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;
> +	struct subdev_regulators *sr;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
>  	return 0;
>  }
>  
> +static int brcm_pcie_add_bus(struct pci_bus *bus)
> +{
> +	struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> +	int ret;
> +
> +	if (!bus->parent || !pci_is_root_bus(bus->parent))
> +		return 0;
> +
> +	ret = pci_subdev_regulators_add_bus(bus);
> +	if (ret)
> +		return ret;
> +
> +	/* Grab the regulators for suspend/resume */
> +	pcie->sr = bus->dev.driver_data;
> +
> +	/*
> +	 * If we have failed linkup there is no point to return an error as
> +	 * currently it will cause a WARNING() from pci_alloc_child_bus().
> +	 * We return 0 and turn on the "refusal_mode" so that any further
> +	 * accesses to the pci_dev just get 0xffffffff
> +	 */
> +	if (brcm_pcie_linkup(pcie) != 0)
> +		pcie->refusal_mode = true;

Is there a bisection hole between the previous patch and this one?
The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(),
so we'll turn on the regulators, but we don't know whether the link
came up.  If it didn't come up, it looks like we might get a CPU abort
when enumerating?

I think we should split out the refusal_mode patch:

  - Add refusal mode
  - Add subdev regulator mechanism
  - This patch (which would then be clearly about managing regulators
    in suspend/resume, IIUC)

> +	return 0;
> +}
> +
>  static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
>  {
>  	struct device *dev = &bus->dev;
> @@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
>  	/* Accesses to the RC go right to the RC registers if slot==0 */
>  	if (pci_is_root_bus(bus))
>  		return PCI_SLOT(devfn) ? NULL : base + where;
> +	if (pcie->refusal_mode) {
> +		/*
> +		 * At this point we do not have link.  There will be a CPU
> +		 * abort -- a quirk with this controller --if Linux tries
> +		 * to read any config-space registers besides those
> +		 * targeting the host bridge.  To prevent this we hijack
> +		 * the address to point to a safe access that will return
> +		 * 0xffffffff.
> +		 */
> +		writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI);
> +		return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3);
> +	}
>  
>  	/* For devices, write to the config space index register */
>  	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
> @@ -854,7 +897,7 @@ 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,
> +	.add_bus = brcm_pcie_add_bus,
>  	.remove_bus = pci_subdev_regulators_remove_bus,
>  };
>  
> @@ -1327,6 +1370,14 @@ 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;
> @@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	if (pcie->sr) {
> +		ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies);
> +		if (ret) {
> +			dev_err(dev, "Could not turn on regulators\n");
> +			goto err_disable_clk;
> +		}
> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret)
> -		goto err_disable_clk;
> +		goto err_regulator;
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret)
> @@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev)
>  
>  err_reset:
>  	reset_control_rearm(pcie->rescal);
> +err_regulator:
> +	if (pcie->sr)
> +		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
>  err_disable_clk:
>  	clk_disable_unprepare(pcie->clk);
>  	return ret;
> @@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail;
>  
> -	ret = brcm_pcie_linkup(pcie);
> -	if (ret)
> -		goto fail;
> -
>  	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
>  	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
>  		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> @@ -1513,7 +1571,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;
> @@ -1522,8 +1590,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,
> +	.resume_noirq = brcm_pcie_resume,

Can you name these brcm_pcie_suspend_noirq() and
brcm_pcie_resume_noirq() to match the hook names?

>  };
>  
>  static struct platform_driver brcm_pcie_driver = {
> -- 
> 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 v1 3/4] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

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

On Wed, Jul 6, 2022 at 7:13 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote:
> > 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.
> >
> > [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 | 86 ++++++++++++++++++++++++---
> >  1 file changed, 77 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 661d3834c6da..a86bf502a265 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -196,6 +196,8 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
> >  static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
> >  static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
> >  static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
> > +static int brcm_pcie_linkup(struct brcm_pcie *pcie);
> > +static int brcm_pcie_add_bus(struct pci_bus *bus);
>
> I think the brcm_pcie_add_bus() declaration is unnecessary.
Will remove.

>
> The brcm_pcie_linkup() one is probably unnecessary, too, but would
> require a lot of reordering that I don't think we should do in this
> series.

I have a future commit that will remove all forward declarations.  I just
wanted to keep this the un-revert patchset simple.

>
> >  enum {
> >       RGR1_SW_INIT_1,
> > @@ -329,6 +331,8 @@ 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;
> > +     struct subdev_regulators *sr;
> >  };
> >
> >  static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -497,6 +501,33 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> >       return 0;
> >  }
> >
> > +static int brcm_pcie_add_bus(struct pci_bus *bus)
> > +{
> > +     struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> > +     int ret;
> > +
> > +     if (!bus->parent || !pci_is_root_bus(bus->parent))
> > +             return 0;
> > +
> > +     ret = pci_subdev_regulators_add_bus(bus);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Grab the regulators for suspend/resume */
> > +     pcie->sr = bus->dev.driver_data;
> > +
> > +     /*
> > +      * If we have failed linkup there is no point to return an error as
> > +      * currently it will cause a WARNING() from pci_alloc_child_bus().
> > +      * We return 0 and turn on the "refusal_mode" so that any further
> > +      * accesses to the pci_dev just get 0xffffffff
> > +      */
> > +     if (brcm_pcie_linkup(pcie) != 0)
> > +             pcie->refusal_mode = true;
>
> Is there a bisection hole between the previous patch and this one?
> The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(),
> so we'll turn on the regulators, but we don't know whether the link
> came up.  If it didn't come up, it looks like we might get a CPU abort
> when enumerating?

I don't think so.  At this commit, attempting the link-up is still
done before the call
to pci_host_probe().  Since there is no power there will be no link,
the probe will
fail and pci_host_probe() will never be invoked.

>
> I think we should split out the refusal_mode patch:
>
>   - Add refusal mode
>   - Add subdev regulator mechanism
>   - This patch (which would then be clearly about managing regulators
>     in suspend/resume, IIUC)

Will do.

>
> > +     return 0;
> > +}
> > +
> >  static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> >  {
> >       struct device *dev = &bus->dev;
> > @@ -826,6 +857,18 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
> >       /* Accesses to the RC go right to the RC registers if slot==0 */
> >       if (pci_is_root_bus(bus))
> >               return PCI_SLOT(devfn) ? NULL : base + where;
> > +     if (pcie->refusal_mode) {
> > +             /*
> > +              * At this point we do not have link.  There will be a CPU
> > +              * abort -- a quirk with this controller --if Linux tries
> > +              * to read any config-space registers besides those
> > +              * targeting the host bridge.  To prevent this we hijack
> > +              * the address to point to a safe access that will return
> > +              * 0xffffffff.
> > +              */
> > +             writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI);
> > +             return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3);
> > +     }
> >
> >       /* For devices, write to the config space index register */
> >       idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
> > @@ -854,7 +897,7 @@ 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,
> > +     .add_bus = brcm_pcie_add_bus,
> >       .remove_bus = pci_subdev_regulators_remove_bus,
> >  };
> >
> > @@ -1327,6 +1370,14 @@ 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;
> > @@ -1344,9 +1395,17 @@ static int brcm_pcie_resume(struct device *dev)
> >       if (ret)
> >               return ret;
> >
> > +     if (pcie->sr) {
> > +             ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies);
> > +             if (ret) {
> > +                     dev_err(dev, "Could not turn on regulators\n");
> > +                     goto err_disable_clk;
> > +             }
> > +     }
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (ret)
> > -             goto err_disable_clk;
> > +             goto err_regulator;
> >
> >       ret = brcm_phy_start(pcie);
> >       if (ret)
> > @@ -1378,6 +1437,9 @@ static int brcm_pcie_resume(struct device *dev)
> >
> >  err_reset:
> >       reset_control_rearm(pcie->rescal);
> > +err_regulator:
> > +     if (pcie->sr)
> > +             regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
> >  err_disable_clk:
> >       clk_disable_unprepare(pcie->clk);
> >       return ret;
> > @@ -1488,10 +1550,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > -     ret = brcm_pcie_linkup(pcie);
> > -     if (ret)
> > -             goto fail;
> > -
> >       pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> >       if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> >               dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> > @@ -1513,7 +1571,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;
> > @@ -1522,8 +1590,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,
> > +     .resume_noirq = brcm_pcie_resume,
>
> Can you name these brcm_pcie_suspend_noirq() and
> brcm_pcie_resume_noirq() to match the hook names?
Yes.

Jim Quintlan
Broadcom STB

>
> >  };
> >
> >  static struct platform_driver brcm_pcie_driver = {
> > --
> > 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 v1 3/4] PCI: brcmstb: Add control of subdevice voltage regulators

[Bjorn Helgaas]

On Fri, Jul 08, 2022 at 11:16:08AM -0400, Jim Quinlan wrote:
> On Wed, Jul 6, 2022 at 7:13 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 01, 2022 at 12:27:24PM -0400, Jim Quinlan wrote:
> > > 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.
> > > ...

> > > +      * If we have failed linkup there is no point to return an error as
> > > +      * currently it will cause a WARNING() from pci_alloc_child_bus().
> > > +      * We return 0 and turn on the "refusal_mode" so that any further
> > > +      * accesses to the pci_dev just get 0xffffffff
> > > +      */
> > > +     if (brcm_pcie_linkup(pcie) != 0)
> > > +             pcie->refusal_mode = true;
> >
> > Is there a bisection hole between the previous patch and this one?
> > The previous patch sets .add_bus() to pci_subdev_regulators_add_bus(),
> > so we'll turn on the regulators, but we don't know whether the link
> > came up.  If it didn't come up, it looks like we might get a CPU abort
> > when enumerating?
> 
> I don't think so.  At this commit, attempting the link-up is still
> done before the call
> to pci_host_probe().  Since there is no power there will be no link,
> the probe will
> fail and pci_host_probe() will never be invoked.

Ah, OK, thanks for the explanation.

> > I think we should split out the refusal_mode patch:
> >
> >   - Add refusal mode
> >   - Add subdev regulator mechanism
> >   - This patch (which would then be clearly about managing regulators
> >     in suspend/resume, IIUC)
> 
> Will do.

If it's not too hard to do, I think splitting it will make the patches
easier to read.

Bjorn

[PATCH v1 4/4] 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 | 53 ++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 9 deletions(-)

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

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Florian Fainelli]

On 7/1/22 09:27, Jim Quinlan wrote:
> A submission [1] was made to enable a PCIe root port to turn on regulators
> for downstream devices.  It was accepted.  Months later, a regression was
> discovered on an RPi CM4 [2].  The patchset was reverted [3] as the fix
> came too late in the release cycle.  The regression in question is
> triggered only when the PCIe RC DT node has no root port subnode, which is
> a perfectly reasonsable configuration.
> 
> The original commits are now being resubmitted with some modifications to
> fix the regression.  The modifcations on the original commits are
> described below (the SHA is that of the original commit):
> 
> [830aa6f29f07  PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
>      NOTE: In the originally submitted patchset, this commit introduced a
>      regression that was corrected by a subsequent commit in the same
>      patchset.  Let's not do this again.
> 
>      @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> 	    if (ret)
> 		    goto fail;
> 
>      +       ret = brcm_pcie_linkup(pcie);
>      +       if (ret)
>      +               goto fail;
> 
> 
> [67211aadcb4b  PCI: brcmstb: Add mechanism to turn on subdev regulators]
>      NOTE: Not related to the regression, the regulators must be freed whenever
>      the PCIe tree is dismantled:
> 
>      @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> 
>      if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> 		    dev_err(dev, "failed to disable regulators for downstream device\n");
>      +       regulator_bulk_free(sr->num_supplies, sr->supplies);
> 	    dev->driver_data = NULL;
> 
> 
> [93e41f3fca3d  PCI: brcmstb: Add control of subdevice voltage regulators]
>      NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
>      link-up was skipped.  This is the regression.  Fix it by attempting
>      link-up even if the Root Port DT subnode is missing.
> 
>      @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> 
>       static int brcm_pcie_add_bus(struct pci_bus *bus)
>       {
>      -       struct device *dev = &bus->dev;
> 	    struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> 	    int ret;
> 
>      -       if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
>      +       if (!bus->parent || !pci_is_root_bus(bus->parent))
> 		    return 0;
> 
> 	    ret = pci_subdev_regulators_add_bus(bus);
> 
> [1] https://lore.kernel.org/r/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/

On a Raspberry Pi 4B:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Cyril Brulebois]

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

Florian Fainelli <f.fainelli@gmail.com> (2022-07-01):
> On 7/1/22 09:27, Jim Quinlan wrote:
> > A submission [1] was made to enable a PCIe root port to turn on regulators
> > for downstream devices.  It was accepted.  Months later, a regression was
> > discovered on an RPi CM4 [2].  The patchset was reverted [3] as the fix
> > came too late in the release cycle.  The regression in question is
> > triggered only when the PCIe RC DT node has no root port subnode, which is
> > a perfectly reasonsable configuration.
> > 
> > The original commits are now being resubmitted with some modifications to
> > fix the regression.  The modifcations on the original commits are
> > described below (the SHA is that of the original commit):
> > 
> > [830aa6f29f07  PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
> >      NOTE: In the originally submitted patchset, this commit introduced a
> >      regression that was corrected by a subsequent commit in the same
> >      patchset.  Let's not do this again.
> > 
> >      @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > 	    if (ret)
> > 		    goto fail;
> > 
> >      +       ret = brcm_pcie_linkup(pcie);
> >      +       if (ret)
> >      +               goto fail;
> > 
> > 
> > [67211aadcb4b  PCI: brcmstb: Add mechanism to turn on subdev regulators]
> >      NOTE: Not related to the regression, the regulators must be freed whenever
> >      the PCIe tree is dismantled:
> > 
> >      @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
> > 
> >      if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> > 		    dev_err(dev, "failed to disable regulators for downstream device\n");
> >      +       regulator_bulk_free(sr->num_supplies, sr->supplies);
> > 	    dev->driver_data = NULL;
> > 
> > 
> > [93e41f3fca3d  PCI: brcmstb: Add control of subdevice voltage regulators]
> >      NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
> >      link-up was skipped.  This is the regression.  Fix it by attempting
> >      link-up even if the Root Port DT subnode is missing.
> > 
> >      @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> > 
> >       static int brcm_pcie_add_bus(struct pci_bus *bus)
> >       {
> >      -       struct device *dev = &bus->dev;
> > 	    struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> > 	    int ret;
> > 
> >      -       if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> >      +       if (!bus->parent || !pci_is_root_bus(bus->parent))
> > 		    return 0;
> > 
> > 	    ret = pci_subdev_regulators_add_bus(bus);
> > 
> > [1] https://lore.kernel.org/r/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/
> 
> On a Raspberry Pi 4B:
> 
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>

As it stands, CM4 support in master is less than ideal: the mmc issues
I've mentioned in some earlier discussion are making it very hard to
draw any definitive conclusions. Soft reboots or cold boots don't seem
to make a difference: the storage might not show up at all, leading to
getting dropped into an initramfs shell, or it might show up but further
accesses can be delayed so much that the system proceeds to booting but
very slowly, and it might even lead to getting dropped into some
emergency/maintenance mode.

This affects both the CM4 Lite variant (no internal storage = SD card in
the CM4 IO slot) and some CM4 non-Lite variant (with internal storage),
with messages like this one getting repeated:

    [  310.105020] mmc0: Timeout waiting for hardware cmd interrupt.
    [  310.110864] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
    [  310.117390] mmc0: sdhci: Sys addr:  0x00000000 | Version:  0x00009902
    [  310.123918] mmc0: sdhci: Blk size:  0x00000000 | Blk cnt:  0x00000000
    [  310.130445] mmc0: sdhci: Argument:  0x000001aa | Trn mode: 0x00000000
    [  310.136971] mmc0: sdhci: Present:   0x01ff0001 | Host ctl: 0x00000001
    [  310.143496] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
    [  310.150021] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00007187
    [  310.156548] mmc0: sdhci: Timeout:   0x00000000 | Int stat: 0x00018000
    [  310.163074] mmc0: sdhci: Int enab:  0x00ff0003 | Sig enab: 0x00ff0003
    [  310.169600] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000001
    [  310.176126] mmc0: sdhci: Caps:      0x00000000 | Caps_1:   0x00000000
    [  310.182652] mmc0: sdhci: Cmd:       0x0000081a | Max curr: 0x00000001
    [  310.189178] mmc0: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
    [  310.195704] mmc0: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
    [  310.202230] mmc0: sdhci: Host ctl2: 0x00000000
    [  310.206728] mmc0: sdhci: ============================================

That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with or
without this patchset.

That being said, I'm not able to reproduce the showstopper regression
that I reported against the initial patchset (booting was breaking in
the very first few seconds), so I suppose it's fine to propose the
following even if that's somewhat tainted by those mmc issues.


With Raspberry Pi CM4 (Lite and non-Lite), mounted on a CM4 IO Board:
 - with a PCIe to quad-USB board, USB storage and USB keyboard;
 - without anything in the PCIe slot.

Tested-by: Cyril Brulebois <cyril@debamax.com>


Cheers,
-- 
Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Florian Fainelli]

On 7/5/22 13:55, Cyril Brulebois wrote:
> Florian Fainelli <f.fainelli@gmail.com> (2022-07-01):
>> On 7/1/22 09:27, Jim Quinlan wrote:
>>> A submission [1] was made to enable a PCIe root port to turn on regulators
>>> for downstream devices.  It was accepted.  Months later, a regression was
>>> discovered on an RPi CM4 [2].  The patchset was reverted [3] as the fix
>>> came too late in the release cycle.  The regression in question is
>>> triggered only when the PCIe RC DT node has no root port subnode, which is
>>> a perfectly reasonsable configuration.
>>>
>>> The original commits are now being resubmitted with some modifications to
>>> fix the regression.  The modifcations on the original commits are
>>> described below (the SHA is that of the original commit):
>>>
>>> [830aa6f29f07  PCI: brcmstb: Split brcm_pcie_setup() into two funcs]
>>>       NOTE: In the originally submitted patchset, this commit introduced a
>>>       regression that was corrected by a subsequent commit in the same
>>>       patchset.  Let's not do this again.
>>>
>>>       @@ -1411,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>> 	    if (ret)
>>> 		    goto fail;
>>>
>>>       +       ret = brcm_pcie_linkup(pcie);
>>>       +       if (ret)
>>>       +               goto fail;
>>>
>>>
>>> [67211aadcb4b  PCI: brcmstb: Add mechanism to turn on subdev regulators]
>>>       NOTE: Not related to the regression, the regulators must be freed whenever
>>>       the PCIe tree is dismantled:
>>>
>>>       @@ -507,6 +507,7 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
>>>
>>>       if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
>>> 		    dev_err(dev, "failed to disable regulators for downstream device\n");
>>>       +       regulator_bulk_free(sr->num_supplies, sr->supplies);
>>> 	    dev->driver_data = NULL;
>>>
>>>
>>> [93e41f3fca3d  PCI: brcmstb: Add control of subdevice voltage regulators]
>>>       NOTE: If the PCIe RC DT node was missing a Root Port subnode, the PCIe
>>>       link-up was skipped.  This is the regression.  Fix it by attempting
>>>       link-up even if the Root Port DT subnode is missing.
>>>
>>>       @@ -503,11 +503,10 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
>>>
>>>        static int brcm_pcie_add_bus(struct pci_bus *bus)
>>>        {
>>>       -       struct device *dev = &bus->dev;
>>> 	    struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
>>> 	    int ret;
>>>
>>>       -       if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
>>>       +       if (!bus->parent || !pci_is_root_bus(bus->parent))
>>> 		    return 0;
>>>
>>> 	    ret = pci_subdev_regulators_add_bus(bus);
>>>
>>> [1] https://lore.kernel.org/r/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/
>>
>> On a Raspberry Pi 4B:
>>
>> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> As it stands, CM4 support in master is less than ideal: the mmc issues
> I've mentioned in some earlier discussion are making it very hard to
> draw any definitive conclusions. Soft reboots or cold boots don't seem
> to make a difference: the storage might not show up at all, leading to
> getting dropped into an initramfs shell, or it might show up but further
> accesses can be delayed so much that the system proceeds to booting but
> very slowly, and it might even lead to getting dropped into some
> emergency/maintenance mode.
> 
> This affects both the CM4 Lite variant (no internal storage = SD card in
> the CM4 IO slot) and some CM4 non-Lite variant (with internal storage),
> with messages like this one getting repeated:
> 
>      [  310.105020] mmc0: Timeout waiting for hardware cmd interrupt.
>      [  310.110864] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
>      [  310.117390] mmc0: sdhci: Sys addr:  0x00000000 | Version:  0x00009902
>      [  310.123918] mmc0: sdhci: Blk size:  0x00000000 | Blk cnt:  0x00000000
>      [  310.130445] mmc0: sdhci: Argument:  0x000001aa | Trn mode: 0x00000000
>      [  310.136971] mmc0: sdhci: Present:   0x01ff0001 | Host ctl: 0x00000001
>      [  310.143496] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
>      [  310.150021] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00007187
>      [  310.156548] mmc0: sdhci: Timeout:   0x00000000 | Int stat: 0x00018000
>      [  310.163074] mmc0: sdhci: Int enab:  0x00ff0003 | Sig enab: 0x00ff0003
>      [  310.169600] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000001
>      [  310.176126] mmc0: sdhci: Caps:      0x00000000 | Caps_1:   0x00000000
>      [  310.182652] mmc0: sdhci: Cmd:       0x0000081a | Max curr: 0x00000001
>      [  310.189178] mmc0: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
>      [  310.195704] mmc0: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
>      [  310.202230] mmc0: sdhci: Host ctl2: 0x00000000
>      [  310.206728] mmc0: sdhci: ============================================
> 
> That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with or
> without this patchset.
> 
> That being said, I'm not able to reproduce the showstopper regression
> that I reported against the initial patchset (booting was breaking in
> the very first few seconds), so I suppose it's fine to propose the
> following even if that's somewhat tainted by those mmc issues.

Any chance you can bisect the eMMC issues so we can investigate those 
separately? Thanks!

> 
> 
> With Raspberry Pi CM4 (Lite and non-Lite), mounted on a CM4 IO Board:
>   - with a PCIe to quad-USB board, USB storage and USB keyboard;
>   - without anything in the PCIe slot.
> 
> Tested-by: Cyril Brulebois <cyril@debamax.com>

Thanks!
-- 
Florian

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Cyril Brulebois]

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

Florian Fainelli <f.fainelli@gmail.com> (2022-07-05):
> On 7/5/22 13:55, Cyril Brulebois wrote:
> > That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with
> > or without this patchset.
> > 
> > That being said, I'm not able to reproduce the showstopper
> > regression that I reported against the initial patchset (booting was
> > breaking in the very first few seconds), so I suppose it's fine to
> > propose the following even if that's somewhat tainted by those mmc
> > issues.
> 
> Any chance you can bisect the eMMC issues so we can investigate those
> separately? Thanks!

Definitely. I wanted to make sure I wouldn't delay the reintroduction of
this patchset (feeling partly responsible for the revert that happened
in the first place), by providing some feedback regarding a possible
come-back of the regression, as soon as possible.

Now that this is out of the way, I'll try and find time to investigate
those MMC issues. Ditto for DRM, I seem to have completely lost the HDMI
output (that's less of an issue thanks to the serial console that has
been rather reliable to gather kernel logs).

I think I started encountering both issues very early in the devel
cycle (when we were still trying to find some follow-up commits to fix
the regression instead of going for the full-on revert), but I couldn't
afford spending time chasing multiple issues at once. I haven't checked
whether reports exist already for those issues, but that's my next step.


Cheers,
-- 
Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Jim Quinlan]

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

On Tue, Jul 5, 2022 at 5:28 PM Cyril Brulebois <kibi@debian.org> wrote:
>
> Florian Fainelli <f.fainelli@gmail.com> (2022-07-05):
> > On 7/5/22 13:55, Cyril Brulebois wrote:
> > > That happens with current master (v5.19-rc5-56-ge35e5b6f695d2), with
> > > or without this patchset.
> > >
> > > That being said, I'm not able to reproduce the showstopper
> > > regression that I reported against the initial patchset (booting was
> > > breaking in the very first few seconds), so I suppose it's fine to
> > > propose the following even if that's somewhat tainted by those mmc
> > > issues.
> >
> > Any chance you can bisect the eMMC issues so we can investigate those
> > separately? Thanks!
Cyril,

Before you go to the trouble of a bisection, can you just post the
(or email me) the following:

o complete boot log
o output of "cat /proc/interrupts"
o output of "for i in $(find /sys/devices/platform/ -type f -name
state) ; do echo $i: $(cat $i) ; done"

Thanks,
Jim Quinlan
Broadcom STB

>
>
> Definitely. I wanted to make sure I wouldn't delay the reintroduction of
> this patchset (feeling partly responsible for the revert that happened
> in the first place), by providing some feedback regarding a possible
> come-back of the regression, as soon as possible.
>
> Now that this is out of the way, I'll try and find time to investigate
> those MMC issues. Ditto for DRM, I seem to have completely lost the HDMI
> output (that's less of an issue thanks to the serial console that has
> been rather reliable to gather kernel logs).
>
> I think I started encountering both issues very early in the devel
> cycle (when we were still trying to find some follow-up commits to fix
> the regression instead of going for the full-on revert), but I couldn't
> afford spending time chasing multiple issues at once. I haven't checked
> whether reports exist already for those issues, but that's my next step.
>
>
> Cheers,
> --
> Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
> D-I release manager -- Release team member -- Freelance Consultant

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

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Bjorn Helgaas]

On Fri, Jul 01, 2022 at 12:27:21PM -0400, Jim Quinlan wrote:
> A submission [1] was made to enable a PCIe root port to turn on regulators
> for downstream devices.  It was accepted.  Months later, a regression was
> discovered on an RPi CM4 [2].  The patchset was reverted [3] as the fix
> came too late in the release cycle.  The regression in question is
> triggered only when the PCIe RC DT node has no root port subnode, which is
> a perfectly reasonsable configuration.
> ...

> Jim Quinlan (4):
>   PCI: brcmstb: Split brcm_pcie_setup() into two funcs
>   PCI: brcmstb: Add mechanism to turn on subdev regulators
>   PCI: brcmstb: oAdd control of subdevice voltage regulators
>   PCI: brcmstb: Do not turn off WOL regulators on suspend
> 
>  drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
>  1 file changed, 227 insertions(+), 30 deletions(-)

I'm assuming there's a v2 coming soonish?  We should see -rc7 this
weekend and likely a final v5.19 release on July 24, so v5.20 material
should be tidied up by then.

Bjorn

Re: [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset

[Jim Quinlan]

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

On Fri, Jul 15, 2022 at 2:27 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 12:27:21PM -0400, Jim Quinlan wrote:
> > A submission [1] was made to enable a PCIe root port to turn on regulators
> > for downstream devices.  It was accepted.  Months later, a regression was
> > discovered on an RPi CM4 [2].  The patchset was reverted [3] as the fix
> > came too late in the release cycle.  The regression in question is
> > triggered only when the PCIe RC DT node has no root port subnode, which is
> > a perfectly reasonsable configuration.
> > ...
>
> > Jim Quinlan (4):
> >   PCI: brcmstb: Split brcm_pcie_setup() into two funcs
> >   PCI: brcmstb: Add mechanism to turn on subdev regulators
> >   PCI: brcmstb: oAdd control of subdevice voltage regulators
> >   PCI: brcmstb: Do not turn off WOL regulators on suspend
> >
> >  drivers/pci/controller/pcie-brcmstb.c | 257 +++++++++++++++++++++++---
> >  1 file changed, 227 insertions(+), 30 deletions(-)
>
> I'm assuming there's a v2 coming soonish?  We should see -rc7 this
> weekend and likely a final v5.19 release on July 24, so v5.20 material
> should be tidied up by then.
Hi Bjorn,

Yes, it has been ready for a few days but I am bumping into unrelated
issues while
trying to do suspend/resume tests with the latest upstream.  Hopefully
I will send
it out tonight or this WE.

Regards,
Jim Quinlan
Broadcom STB
>
> Bjorn

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