| Message ID | 20200824193036.6033-5-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: enable PCIe for STB chips | expand |
On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: > From: Jim Quinlan <jquinlan@broadcom.com> > > Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend > and resume. Now the PCIe driver may do so as well. > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index c2b3d2946a36..3d588ab7a6dd 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) > brcm_pcie_bridge_sw_init_set(pcie, 1); > } > > +static int brcm_pcie_suspend(struct device *dev) > +{ > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > + > + brcm_pcie_turn_off(pcie); > + clk_disable_unprepare(pcie->clk); > + > + return 0; > +} > + > +static int brcm_pcie_resume(struct device *dev) > +{ > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > + void __iomem *base; > + u32 tmp; > + int ret; > + > + base = pcie->base; > + clk_prepare_enable(pcie->clk); > + > + /* Take bridge out of reset so we can access the SERDES reg */ > + brcm_pcie_bridge_sw_init_set(pcie, 0); > + > + /* SERDES_IDDQ = 0 */ > + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > + > + /* wait for serdes to be stable */ > + udelay(100); Really needs to be a spinloop? > + > + ret = brcm_pcie_setup(pcie); > + if (ret) > + return ret; > + > + if (pcie->msi) > + brcm_msi_set_regs(pcie->msi); > + > + return 0; > +} > + > static void __brcm_pcie_remove(struct brcm_pcie *pcie) > { > brcm_msi_remove(pcie); > @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, > + .resume_noirq = brcm_pcie_resume, Why do you need interrupts disabled? There's 39 cases of .suspend_noirq and 1352 of .suspend in the tree. Is doing a clk unprepare even safe in .suspend_noirq? IIRC, prepare/unprepare can sleep. > +}; > + > static struct platform_driver brcm_pcie_driver = { > .probe = brcm_pcie_probe, > .remove = brcm_pcie_remove, > .driver = { > .name = "brcm-pcie", > .of_match_table = brcm_pcie_match, > + .pm = &brcm_pcie_pm_ops, > }, > }; > module_platform_driver(brcm_pcie_driver); > -- > 2.17.1 >
On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: > > On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: > > From: Jim Quinlan <jquinlan@broadcom.com> > > > > Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend > > and resume. Now the PCIe driver may do so as well. > > > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index c2b3d2946a36..3d588ab7a6dd 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) > > brcm_pcie_bridge_sw_init_set(pcie, 1); > > } > > > > +static int brcm_pcie_suspend(struct device *dev) > > +{ > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > + > > + brcm_pcie_turn_off(pcie); > > + clk_disable_unprepare(pcie->clk); > > + > > + return 0; > > +} > > + > > +static int brcm_pcie_resume(struct device *dev) > > +{ > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > + void __iomem *base; > > + u32 tmp; > > + int ret; > > + > > + base = pcie->base; > > + clk_prepare_enable(pcie->clk); > > + > > + /* Take bridge out of reset so we can access the SERDES reg */ > > + brcm_pcie_bridge_sw_init_set(pcie, 0); > > + > > + /* SERDES_IDDQ = 0 */ > > + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > > + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > + > > + /* wait for serdes to be stable */ > > + udelay(100); > > Really needs to be a spinloop? > > > + > > + ret = brcm_pcie_setup(pcie); > > + if (ret) > > + return ret; > > + > > + if (pcie->msi) > > + brcm_msi_set_regs(pcie->msi); > > + > > + return 0; > > +} > > + > > static void __brcm_pcie_remove(struct brcm_pcie *pcie) > > { > > brcm_msi_remove(pcie); > > @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, > > + .resume_noirq = brcm_pcie_resume, > > Why do you need interrupts disabled? There's 39 cases of .suspend_noirq > and 1352 of .suspend in the tree. I will test switching this to suspend_late/resume_early. Thanks, Jim Quinlan Broadcom STB > > Is doing a clk unprepare even safe in .suspend_noirq? IIRC, > prepare/unprepare can sleep. > > > +}; > > + > > static struct platform_driver brcm_pcie_driver = { > > .probe = brcm_pcie_probe, > > .remove = brcm_pcie_remove, > > .driver = { > > .name = "brcm-pcie", > > .of_match_table = brcm_pcie_match, > > + .pm = &brcm_pcie_pm_ops, > > }, > > }; > > module_platform_driver(brcm_pcie_driver); > > -- > > 2.17.1 > >
On 9/10/2020 8:56 AM, Rob Herring wrote: > On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: >> From: Jim Quinlan <jquinlan@broadcom.com> >> >> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend >> and resume. Now the PCIe driver may do so as well. >> >> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >> index c2b3d2946a36..3d588ab7a6dd 100644 >> --- a/drivers/pci/controller/pcie-brcmstb.c >> +++ b/drivers/pci/controller/pcie-brcmstb.c >> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) >> brcm_pcie_bridge_sw_init_set(pcie, 1); >> } >> >> +static int brcm_pcie_suspend(struct device *dev) >> +{ >> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >> + >> + brcm_pcie_turn_off(pcie); >> + clk_disable_unprepare(pcie->clk); >> + >> + return 0; >> +} >> + >> +static int brcm_pcie_resume(struct device *dev) >> +{ >> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >> + void __iomem *base; >> + u32 tmp; >> + int ret; >> + >> + base = pcie->base; >> + clk_prepare_enable(pcie->clk); >> + >> + /* Take bridge out of reset so we can access the SERDES reg */ >> + brcm_pcie_bridge_sw_init_set(pcie, 0); >> + >> + /* SERDES_IDDQ = 0 */ >> + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >> + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); >> + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >> + >> + /* wait for serdes to be stable */ >> + udelay(100); > > Really needs to be a spinloop? > >> + >> + ret = brcm_pcie_setup(pcie); >> + if (ret) >> + return ret; >> + >> + if (pcie->msi) >> + brcm_msi_set_regs(pcie->msi); >> + >> + return 0; >> +} >> + >> static void __brcm_pcie_remove(struct brcm_pcie *pcie) >> { >> brcm_msi_remove(pcie); >> @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, >> + .resume_noirq = brcm_pcie_resume, > > Why do you need interrupts disabled? There's 39 cases of .suspend_noirq > and 1352 of .suspend in the tree. > > Is doing a clk unprepare even safe in .suspend_noirq? IIRC, > prepare/unprepare can sleep. Yes, it is safe, provided that your clock provider (clk-scmi.c in our case) supports it, too. In our case the underlying mailbox driver has its interrupts flagged with IRQF_NOSUSPEND such that they can still be processed at _noirq time. I think the rationale was to ensure that this would be done much later after other subsystem have been made quiescent, but given the Linux device driver model, the PCI bridge should be suspended after all pci_device child device, so it should be safe not to use _noirq.
On Thu, Sep 10, 2020 at 10:42 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: > > > > On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: > > > From: Jim Quinlan <jquinlan@broadcom.com> > > > > > > Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend > > > and resume. Now the PCIe driver may do so as well. > > > > > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > > drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ > > > 1 file changed, 47 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > > index c2b3d2946a36..3d588ab7a6dd 100644 > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) > > > brcm_pcie_bridge_sw_init_set(pcie, 1); > > > } > > > > > > +static int brcm_pcie_suspend(struct device *dev) > > > +{ > > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > > + > > > + brcm_pcie_turn_off(pcie); > > > + clk_disable_unprepare(pcie->clk); > > > + > > > + return 0; > > > +} > > > + > > > +static int brcm_pcie_resume(struct device *dev) > > > +{ > > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > > + void __iomem *base; > > > + u32 tmp; > > > + int ret; > > > + > > > + base = pcie->base; > > > + clk_prepare_enable(pcie->clk); > > > + > > > + /* Take bridge out of reset so we can access the SERDES reg */ > > > + brcm_pcie_bridge_sw_init_set(pcie, 0); > > > + > > > + /* SERDES_IDDQ = 0 */ > > > + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > > + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > > > + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > > + > > > + /* wait for serdes to be stable */ > > > + udelay(100); > > > > Really needs to be a spinloop? > > > > > + > > > + ret = brcm_pcie_setup(pcie); > > > + if (ret) > > > + return ret; > > > + > > > + if (pcie->msi) > > > + brcm_msi_set_regs(pcie->msi); > > > + > > > + return 0; > > > +} > > > + > > > static void __brcm_pcie_remove(struct brcm_pcie *pcie) > > > { > > > brcm_msi_remove(pcie); > > > @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, > > > + .resume_noirq = brcm_pcie_resume, > > > > Why do you need interrupts disabled? There's 39 cases of .suspend_noirq > > and 1352 of .suspend in the tree. > > I will test switching this to suspend_late/resume_early. Why not just the 'regular' flavor suspend/resume? Rob
On 9/10/2020 11:50 AM, Rob Herring wrote: > On Thu, Sep 10, 2020 at 10:42 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: >> >> On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: >>> >>> On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: >>>> From: Jim Quinlan <jquinlan@broadcom.com> >>>> >>>> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend >>>> and resume. Now the PCIe driver may do so as well. >>>> >>>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> >>>> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>>> drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ >>>> 1 file changed, 47 insertions(+) >>>> >>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>>> index c2b3d2946a36..3d588ab7a6dd 100644 >>>> --- a/drivers/pci/controller/pcie-brcmstb.c >>>> +++ b/drivers/pci/controller/pcie-brcmstb.c >>>> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) >>>> brcm_pcie_bridge_sw_init_set(pcie, 1); >>>> } >>>> >>>> +static int brcm_pcie_suspend(struct device *dev) >>>> +{ >>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >>>> + >>>> + brcm_pcie_turn_off(pcie); >>>> + clk_disable_unprepare(pcie->clk); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int brcm_pcie_resume(struct device *dev) >>>> +{ >>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >>>> + void __iomem *base; >>>> + u32 tmp; >>>> + int ret; >>>> + >>>> + base = pcie->base; >>>> + clk_prepare_enable(pcie->clk); >>>> + >>>> + /* Take bridge out of reset so we can access the SERDES reg */ >>>> + brcm_pcie_bridge_sw_init_set(pcie, 0); >>>> + >>>> + /* SERDES_IDDQ = 0 */ >>>> + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >>>> + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); >>>> + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >>>> + >>>> + /* wait for serdes to be stable */ >>>> + udelay(100); >>> >>> Really needs to be a spinloop? >>> >>>> + >>>> + ret = brcm_pcie_setup(pcie); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (pcie->msi) >>>> + brcm_msi_set_regs(pcie->msi); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static void __brcm_pcie_remove(struct brcm_pcie *pcie) >>>> { >>>> brcm_msi_remove(pcie); >>>> @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, >>>> + .resume_noirq = brcm_pcie_resume, >>> >>> Why do you need interrupts disabled? There's 39 cases of .suspend_noirq >>> and 1352 of .suspend in the tree. >> >> I will test switching this to suspend_late/resume_early. > > Why not just the 'regular' flavor suspend/resume? We must have inherited this from when the driver was not a platform_device back in our 3.14 downstream kernel and we used syscore_ops to do the system suspend/resume. Later on, we sort of mechanically made those _noirq() to preserve the semantics of syscore_ops, but in hindsight it should not be necessary, the regular suspsend/resume should work and the device driver model ordering between parent/child should take care of the bridge being suspended last within the PCI bus type that is.
On Thu, Sep 10, 2020 at 2:50 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Sep 10, 2020 at 10:42 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: > > > > From: Jim Quinlan <jquinlan@broadcom.com> > > > > > > > > Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend > > > > and resume. Now the PCIe driver may do so as well. > > > > > > > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > > > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > > > --- > > > > drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ > > > > 1 file changed, 47 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > > > index c2b3d2946a36..3d588ab7a6dd 100644 > > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > > @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) > > > > brcm_pcie_bridge_sw_init_set(pcie, 1); > > > > } > > > > > > > > +static int brcm_pcie_suspend(struct device *dev) > > > > +{ > > > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > > > + > > > > + brcm_pcie_turn_off(pcie); > > > > + clk_disable_unprepare(pcie->clk); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int brcm_pcie_resume(struct device *dev) > > > > +{ > > > > + struct brcm_pcie *pcie = dev_get_drvdata(dev); > > > > + void __iomem *base; > > > > + u32 tmp; > > > > + int ret; > > > > + > > > > + base = pcie->base; > > > > + clk_prepare_enable(pcie->clk); > > > > + > > > > + /* Take bridge out of reset so we can access the SERDES reg */ > > > > + brcm_pcie_bridge_sw_init_set(pcie, 0); > > > > + > > > > + /* SERDES_IDDQ = 0 */ > > > > + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > > > + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > > > > + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > > > > + > > > > + /* wait for serdes to be stable */ > > > > + udelay(100); > > > > > > Really needs to be a spinloop? > > > > > > > + > > > > + ret = brcm_pcie_setup(pcie); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (pcie->msi) > > > > + brcm_msi_set_regs(pcie->msi); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static void __brcm_pcie_remove(struct brcm_pcie *pcie) > > > > { > > > > brcm_msi_remove(pcie); > > > > @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, > > > > + .resume_noirq = brcm_pcie_resume, > > > > > > Why do you need interrupts disabled? There's 39 cases of .suspend_noirq > > > and 1352 of .suspend in the tree. > > > > I will test switching this to suspend_late/resume_early. > > Why not just the 'regular' flavor suspend/resume? > > Rob We must have our PCIe driver suspend last and resume first because our current driver turns off/on the power for the EPs. Note that this code isn't in the driver as we are still figuring out a way to make it upstreamable. Jim
On 9/10/2020 12:05 PM, Jim Quinlan wrote: > On Thu, Sep 10, 2020 at 2:50 PM Rob Herring <robh@kernel.org> wrote: >> >> On Thu, Sep 10, 2020 at 10:42 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: >>> >>> On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: >>>> >>>> On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: >>>>> From: Jim Quinlan <jquinlan@broadcom.com> >>>>> >>>>> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend >>>>> and resume. Now the PCIe driver may do so as well. >>>>> >>>>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> >>>>> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >>>>> --- >>>>> drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ >>>>> 1 file changed, 47 insertions(+) >>>>> >>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>>>> index c2b3d2946a36..3d588ab7a6dd 100644 >>>>> --- a/drivers/pci/controller/pcie-brcmstb.c >>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c >>>>> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) >>>>> brcm_pcie_bridge_sw_init_set(pcie, 1); >>>>> } >>>>> >>>>> +static int brcm_pcie_suspend(struct device *dev) >>>>> +{ >>>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >>>>> + >>>>> + brcm_pcie_turn_off(pcie); >>>>> + clk_disable_unprepare(pcie->clk); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int brcm_pcie_resume(struct device *dev) >>>>> +{ >>>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); >>>>> + void __iomem *base; >>>>> + u32 tmp; >>>>> + int ret; >>>>> + >>>>> + base = pcie->base; >>>>> + clk_prepare_enable(pcie->clk); >>>>> + >>>>> + /* Take bridge out of reset so we can access the SERDES reg */ >>>>> + brcm_pcie_bridge_sw_init_set(pcie, 0); >>>>> + >>>>> + /* SERDES_IDDQ = 0 */ >>>>> + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >>>>> + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); >>>>> + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); >>>>> + >>>>> + /* wait for serdes to be stable */ >>>>> + udelay(100); >>>> >>>> Really needs to be a spinloop? >>>> >>>>> + >>>>> + ret = brcm_pcie_setup(pcie); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + if (pcie->msi) >>>>> + brcm_msi_set_regs(pcie->msi); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static void __brcm_pcie_remove(struct brcm_pcie *pcie) >>>>> { >>>>> brcm_msi_remove(pcie); >>>>> @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, >>>>> + .resume_noirq = brcm_pcie_resume, >>>> >>>> Why do you need interrupts disabled? There's 39 cases of .suspend_noirq >>>> and 1352 of .suspend in the tree. >>> >>> I will test switching this to suspend_late/resume_early. >> >> Why not just the 'regular' flavor suspend/resume? >> >> Rob > We must have our PCIe driver suspend last and resume first because our > current driver turns off/on the power for the EPs. Note that this > code isn't in the driver as we are still figuring out a way to make it > upstreamable. The suspend/resume ordering should be guaranteed by the Linux device driver model though if not, this is a bug that ought to be fixed. The PCI bridge sits at the top of the pci_device list and all EPs should be child devices, so the suspend order should be from EPs down to the bridge, and the resume the converse.
On Thu, Sep 10, 2020 at 3:08 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 9/10/2020 12:05 PM, Jim Quinlan wrote: > > On Thu, Sep 10, 2020 at 2:50 PM Rob Herring <robh@kernel.org> wrote: > >> > >> On Thu, Sep 10, 2020 at 10:42 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > >>> > >>> On Thu, Sep 10, 2020 at 11:56 AM Rob Herring <robh@kernel.org> wrote: > >>>> > >>>> On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote: > >>>>> From: Jim Quinlan <jquinlan@broadcom.com> > >>>>> > >>>>> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend > >>>>> and resume. Now the PCIe driver may do so as well. > >>>>> > >>>>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > >>>>> Acked-by: Florian Fainelli <f.fainelli@gmail.com> > >>>>> --- > >>>>> drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++ > >>>>> 1 file changed, 47 insertions(+) > >>>>> > >>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > >>>>> index c2b3d2946a36..3d588ab7a6dd 100644 > >>>>> --- a/drivers/pci/controller/pcie-brcmstb.c > >>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c > >>>>> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) > >>>>> brcm_pcie_bridge_sw_init_set(pcie, 1); > >>>>> } > >>>>> > >>>>> +static int brcm_pcie_suspend(struct device *dev) > >>>>> +{ > >>>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); > >>>>> + > >>>>> + brcm_pcie_turn_off(pcie); > >>>>> + clk_disable_unprepare(pcie->clk); > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +static int brcm_pcie_resume(struct device *dev) > >>>>> +{ > >>>>> + struct brcm_pcie *pcie = dev_get_drvdata(dev); > >>>>> + void __iomem *base; > >>>>> + u32 tmp; > >>>>> + int ret; > >>>>> + > >>>>> + base = pcie->base; > >>>>> + clk_prepare_enable(pcie->clk); > >>>>> + > >>>>> + /* Take bridge out of reset so we can access the SERDES reg */ > >>>>> + brcm_pcie_bridge_sw_init_set(pcie, 0); > >>>>> + > >>>>> + /* SERDES_IDDQ = 0 */ > >>>>> + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > >>>>> + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > >>>>> + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); > >>>>> + > >>>>> + /* wait for serdes to be stable */ > >>>>> + udelay(100); > >>>> > >>>> Really needs to be a spinloop? > >>>> > >>>>> + > >>>>> + ret = brcm_pcie_setup(pcie); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (pcie->msi) > >>>>> + brcm_msi_set_regs(pcie->msi); > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> static void __brcm_pcie_remove(struct brcm_pcie *pcie) > >>>>> { > >>>>> brcm_msi_remove(pcie); > >>>>> @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, > >>>>> + .resume_noirq = brcm_pcie_resume, > >>>> > >>>> Why do you need interrupts disabled? There's 39 cases of .suspend_noirq > >>>> and 1352 of .suspend in the tree. > >>> > >>> I will test switching this to suspend_late/resume_early. > >> > >> Why not just the 'regular' flavor suspend/resume? > >> > >> Rob > > We must have our PCIe driver suspend last and resume first because our > > current driver turns off/on the power for the EPs. Note that this > > code isn't in the driver as we are still figuring out a way to make it > > upstreamable. > > The suspend/resume ordering should be guaranteed by the Linux device > driver model though if not, this is a bug that ought to be fixed. The > PCI bridge sits at the top of the pci_device list and all EPs should be > child devices, so the suspend order should be from EPs down to the > bridge, and the resume the converse. I remembered that after I hit send. Jim > -- > Florian
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index c2b3d2946a36..3d588ab7a6dd 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie) brcm_pcie_bridge_sw_init_set(pcie, 1); } +static int brcm_pcie_suspend(struct device *dev) +{ + struct brcm_pcie *pcie = dev_get_drvdata(dev); + + brcm_pcie_turn_off(pcie); + clk_disable_unprepare(pcie->clk); + + return 0; +} + +static int brcm_pcie_resume(struct device *dev) +{ + struct brcm_pcie *pcie = dev_get_drvdata(dev); + void __iomem *base; + u32 tmp; + int ret; + + base = pcie->base; + clk_prepare_enable(pcie->clk); + + /* Take bridge out of reset so we can access the SERDES reg */ + brcm_pcie_bridge_sw_init_set(pcie, 0); + + /* SERDES_IDDQ = 0 */ + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); + u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG); + + /* wait for serdes to be stable */ + udelay(100); + + ret = brcm_pcie_setup(pcie); + if (ret) + return ret; + + if (pcie->msi) + brcm_msi_set_regs(pcie->msi); + + return 0; +} + static void __brcm_pcie_remove(struct brcm_pcie *pcie) { brcm_msi_remove(pcie); @@ -1087,12 +1128,18 @@ 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_noirq = brcm_pcie_suspend, + .resume_noirq = brcm_pcie_resume, +}; + static struct platform_driver brcm_pcie_driver = { .probe = brcm_pcie_probe, .remove = brcm_pcie_remove, .driver = { .name = "brcm-pcie", .of_match_table = brcm_pcie_match, + .pm = &brcm_pcie_pm_ops, }, }; module_platform_driver(brcm_pcie_driver);