diff mbox series

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

Message ID 20220701162726.31346-2-jim2101024@gmail.com (mailing list archive)
State Superseded
Headers show
Series PCI: brcmstb: Re-submit reverted patchset | expand

Commit Message

Jim Quinlan July 1, 2022, 4:27 p.m. UTC
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(-)

Comments

Bjorn Helgaas July 6, 2022, 9:56 p.m. UTC | #1
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
Jim Quinlan July 8, 2022, 1:29 p.m. UTC | #2
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
Bjorn Helgaas July 8, 2022, 7:04 p.m. UTC | #3
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
Jim Quinlan July 8, 2022, 7:40 p.m. UTC | #4
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
>
Bjorn Helgaas July 8, 2022, 7:59 p.m. UTC | #5
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
Jim Quinlan July 8, 2022, 8:38 p.m. UTC | #6
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
Bjorn Helgaas July 8, 2022, 10:27 p.m. UTC | #7
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
diff mbox series

Patch

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");