* [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq @ 2019-02-17 13:24 marek.vasut 2019-02-19 17:24 ` Wolfram Sang ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: marek.vasut @ 2019-02-17 13:24 UTC (permalink / raw) To: linux-pci Cc: Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> Reestablish the PCIe link very early in the resume process in case it went down to prevent PCI accesses from hanging the bus. Such accesses can happen early in the PCI resume process, in the resume_noirq, thus the link must be reestablished in the resume_noirq callback of the driver. Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Phil Edworthy <phil.edworthy@renesas.com> Cc: Simon Horman <horms+renesas@verge.net.au> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: linux-renesas-soc@vger.kernel.org --- V2: - Use BIT() macro for (1 << n) - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not add extra changes to this function anymore - Make resume_noirq return early and clean up parenthesis therein --- drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index c8febb009454..b8f8fb3bc640 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -46,6 +46,7 @@ /* Transfer control */ #define PCIETCTLR 0x02000 +#define DL_DOWN BIT(3) #define CFINIT 1 #define PCIETSTR 0x02004 #define DATA_LINK_ACTIVE 1 @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) pcie = pci_host_bridge_priv(bridge); pcie->dev = dev; + platform_set_drvdata(pdev, pcie); err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); if (err) @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) return err; } +static int rcar_pcie_resume_noirq(struct device *dev) +{ + struct rcar_pcie *pcie = dev_get_drvdata(dev); + + if (rcar_pci_read_reg(pcie, PMSR) && + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) + return 0; + + /* Re-establish the PCIe link */ + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); + return rcar_pcie_wait_for_dl(pcie); +} + +static const struct dev_pm_ops rcar_pcie_pm_ops = { + .resume_noirq = rcar_pcie_resume_noirq, +}; + static struct platform_driver rcar_pcie_driver = { .driver = { .name = "rcar-pcie", .of_match_table = rcar_pcie_of_match, + .pm = &rcar_pcie_pm_ops, .suppress_bind_attrs = true, }, .probe = rcar_pcie_probe, -- 2.19.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-02-17 13:24 [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq marek.vasut @ 2019-02-19 17:24 ` Wolfram Sang 2019-02-25 9:58 ` Simon Horman 2019-03-07 20:50 ` Bjorn Helgaas ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Wolfram Sang @ 2019-02-19 17:24 UTC (permalink / raw) To: marek.vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 890 bytes --] On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > Reestablish the PCIe link very early in the resume process in case it > went down to prevent PCI accesses from hanging the bus. Such accesses > can happen early in the PCI resume process, in the resume_noirq, thus > the link must be reestablished in the resume_noirq callback of the > driver. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> I am not a deep PCI expert. Still, the patch looks good to me and I double checked with the documentation. Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Minor nit would be 'CONFIG_PM' protection around the new code. But I have no strong opinion on that. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-02-19 17:24 ` Wolfram Sang @ 2019-02-25 9:58 ` Simon Horman 0 siblings, 0 replies; 18+ messages in thread From: Simon Horman @ 2019-02-25 9:58 UTC (permalink / raw) To: Wolfram Sang Cc: marek.vasut, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, linux-renesas-soc On Tue, Feb 19, 2019 at 06:24:20PM +0100, Wolfram Sang wrote: > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > > > Reestablish the PCIe link very early in the resume process in case it > > went down to prevent PCI accesses from hanging the bus. Such accesses > > can happen early in the PCI resume process, in the resume_noirq, thus > > the link must be reestablished in the resume_noirq callback of the > > driver. > > > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > I am not a deep PCI expert. Still, the patch looks good to me and I > double checked with the documentation. > > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Minor nit would be 'CONFIG_PM' protection around the new code. But I > have no strong opinion on that. I agree that would be a nice enhancement but I also do not feel strongly about it. Reviewed-by: Simon Horman <horms+renesas@verge.net.au> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-02-17 13:24 [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq marek.vasut 2019-02-19 17:24 ` Wolfram Sang @ 2019-03-07 20:50 ` Bjorn Helgaas 2019-03-07 22:49 ` Marek Vasut 2019-03-22 11:31 ` Lorenzo Pieralisi 2019-03-25 16:54 ` Lorenzo Pieralisi 3 siblings, 1 reply; 18+ messages in thread From: Bjorn Helgaas @ 2019-03-07 20:50 UTC (permalink / raw) To: marek.vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > Reestablish the PCIe link very early in the resume process in case it > went down to prevent PCI accesses from hanging the bus. Such accesses > can happen early in the PCI resume process, in the resume_noirq, thus > the link must be reestablished in the resume_noirq callback of the > driver. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Use BIT() macro for (1 << n) > - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not > add extra changes to this function anymore > - Make resume_noirq return early and clean up parenthesis therein > --- > drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index c8febb009454..b8f8fb3bc640 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -46,6 +46,7 @@ > > /* Transfer control */ > #define PCIETCTLR 0x02000 > +#define DL_DOWN BIT(3) > #define CFINIT 1 I saw discussion after the V1 patch about using BIT() and making similar constants also use BIT() for consistency. That makes sense to me, and I think the best way would be: 1) in *this* patch, use "#define DL_DOWN 8" 2) in a followup patch, convert them all to BIT() That way each revision of pcie-rcar.c is self-consistent. > #define PCIETSTR 0x02004 > #define DATA_LINK_ACTIVE 1 > @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > pcie = pci_host_bridge_priv(bridge); > > pcie->dev = dev; > + platform_set_drvdata(pdev, pcie); > > err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); > if (err) > @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume_noirq(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + > + if (rcar_pci_read_reg(pcie, PMSR) && > + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > + return 0; > + > + /* Re-establish the PCIe link */ > + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > + return rcar_pcie_wait_for_dl(pcie); > +} > + > +static const struct dev_pm_ops rcar_pcie_pm_ops = { > + .resume_noirq = rcar_pcie_resume_noirq, > +}; I think there's the beginning of a convention to use #ifdef CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. > static struct platform_driver rcar_pcie_driver = { > .driver = { > .name = "rcar-pcie", > .of_match_table = rcar_pcie_of_match, > + .pm = &rcar_pcie_pm_ops, > .suppress_bind_attrs = true, > }, > .probe = rcar_pcie_probe, > -- > 2.19.2 > [1] https://lore.kernel.org/linux-pci/20180531042020.GQ39853@bhelgaas-glaptop.roam.corp.google.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-07 20:50 ` Bjorn Helgaas @ 2019-03-07 22:49 ` Marek Vasut 2019-03-08 17:17 ` Bjorn Helgaas 0 siblings, 1 reply; 18+ messages in thread From: Marek Vasut @ 2019-03-07 22:49 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On 3/7/19 9:50 PM, Bjorn Helgaas wrote: > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> >> Reestablish the PCIe link very early in the resume process in case it >> went down to prevent PCI accesses from hanging the bus. Such accesses >> can happen early in the PCI resume process, in the resume_noirq, thus >> the link must be reestablished in the resume_noirq callback of the >> driver. >> >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: - Use BIT() macro for (1 << n) >> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not >> add extra changes to this function anymore >> - Make resume_noirq return early and clean up parenthesis therein >> --- >> drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c >> index c8febb009454..b8f8fb3bc640 100644 >> --- a/drivers/pci/controller/pcie-rcar.c >> +++ b/drivers/pci/controller/pcie-rcar.c >> @@ -46,6 +46,7 @@ >> >> /* Transfer control */ >> #define PCIETCTLR 0x02000 >> +#define DL_DOWN BIT(3) >> #define CFINIT 1 > > I saw discussion after the V1 patch about using BIT() and making > similar constants also use BIT() for consistency. That makes sense to > me, and I think the best way would be: > > 1) in *this* patch, use "#define DL_DOWN 8" > 2) in a followup patch, convert them all to BIT() > > That way each revision of pcie-rcar.c is self-consistent. But the BIT() macros are already cleaned , see commit 0ee40820989b330e24926d82953ffb9e1c7a8425 PCI: rcar: Clean up the macros >> #define PCIETSTR 0x02004 >> #define DATA_LINK_ACTIVE 1 >> @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> pcie = pci_host_bridge_priv(bridge); >> >> pcie->dev = dev; >> + platform_set_drvdata(pdev, pcie); >> >> err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); >> if (err) >> @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> return err; >> } >> >> +static int rcar_pcie_resume_noirq(struct device *dev) >> +{ >> + struct rcar_pcie *pcie = dev_get_drvdata(dev); >> + >> + if (rcar_pci_read_reg(pcie, PMSR) && >> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) >> + return 0; >> + >> + /* Re-establish the PCIe link */ >> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); >> + return rcar_pcie_wait_for_dl(pcie); >> +} >> + >> +static const struct dev_pm_ops rcar_pcie_pm_ops = { >> + .resume_noirq = rcar_pcie_resume_noirq, >> +}; > > I think there's the beginning of a convention to use #ifdef > CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think > we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the resume_noirq directly. >> static struct platform_driver rcar_pcie_driver = { >> .driver = { >> .name = "rcar-pcie", >> .of_match_table = rcar_pcie_of_match, >> + .pm = &rcar_pcie_pm_ops, >> .suppress_bind_attrs = true, >> }, >> .probe = rcar_pcie_probe, >> -- >> 2.19.2 >> > > [1] https://lore.kernel.org/linux-pci/20180531042020.GQ39853@bhelgaas-glaptop.roam.corp.google.com > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-07 22:49 ` Marek Vasut @ 2019-03-08 17:17 ` Bjorn Helgaas 2019-03-08 21:35 ` Marek Vasut 2019-03-19 16:18 ` Lorenzo Pieralisi 0 siblings, 2 replies; 18+ messages in thread From: Bjorn Helgaas @ 2019-03-08 17:17 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc, linux-pm, Rafael J. Wysocki [+cc linux-pm, Rafael for SET_NOIRQ_SYSTEM_SLEEP_PM_OPS question at the end] On Thu, Mar 07, 2019 at 11:49:34PM +0100, Marek Vasut wrote: > On 3/7/19 9:50 PM, Bjorn Helgaas wrote: > > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >> > >> Reestablish the PCIe link very early in the resume process in case it > >> went down to prevent PCI accesses from hanging the bus. Such accesses > >> can happen early in the PCI resume process, in the resume_noirq, thus > >> the link must be reestablished in the resume_noirq callback of the > >> driver. > >> > >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >> Cc: Simon Horman <horms+renesas@verge.net.au> > >> Cc: Wolfram Sang <wsa@the-dreams.de> > >> Cc: linux-renesas-soc@vger.kernel.org > >> --- > >> V2: - Use BIT() macro for (1 << n) > >> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not > >> add extra changes to this function anymore > >> - Make resume_noirq return early and clean up parenthesis therein > >> --- > >> drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > >> index c8febb009454..b8f8fb3bc640 100644 > >> --- a/drivers/pci/controller/pcie-rcar.c > >> +++ b/drivers/pci/controller/pcie-rcar.c > >> @@ -46,6 +46,7 @@ > >> > >> /* Transfer control */ > >> #define PCIETCTLR 0x02000 > >> +#define DL_DOWN BIT(3) > >> #define CFINIT 1 > > > > I saw discussion after the V1 patch about using BIT() and making > > similar constants also use BIT() for consistency. That makes sense to > > me, and I think the best way would be: > > > > 1) in *this* patch, use "#define DL_DOWN 8" > > 2) in a followup patch, convert them all to BIT() > > > > That way each revision of pcie-rcar.c is self-consistent. > > But the BIT() macros are already cleaned , see commit > 0ee40820989b330e24926d82953ffb9e1c7a8425 > > PCI: rcar: Clean up the macros Hmmm. Maybe I'm missing something, but it looks like 0ee40820989b didn't touch CFINIT, DATA_LINK_ACTIVE, or MSIFE. Arguably, LINK_SPEED_2_5GTS and LINK_SPEED_5_0GTS could use BIT() also. I guess I'm just old-school, but in my personal opinion, BIT() is more trouble than it's worth. I'd rather see a complete bitmask because I can easily match it with the typical pictures in a spec, multi-bit fields are easy (you don't have to mix BIT() and GENMASK()), it gives a hint about the register width, it's easy to match with a hexdump, etc, e.g., #define DL_DOWN 0x00000008 #define CFINIT 0x00000001 But I'm not suggesting that you get rid of BIT() in this driver. I'm fine with it as long as it's used consistently. BTW, while we're looking at it, I think rcar_pci_read_reg() and rcar_pci_write_reg() really should use "u32" instead of "unsigned long", since they deal with hardware registers that are explicitly 32 bits wide. > >> #define PCIETSTR 0x02004 > >> #define DATA_LINK_ACTIVE 1 > >> @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> pcie = pci_host_bridge_priv(bridge); > >> > >> pcie->dev = dev; > >> + platform_set_drvdata(pdev, pcie); > >> > >> err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); > >> if (err) > >> @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> return err; > >> } > >> > >> +static int rcar_pcie_resume_noirq(struct device *dev) > >> +{ > >> + struct rcar_pcie *pcie = dev_get_drvdata(dev); > >> + > >> + if (rcar_pci_read_reg(pcie, PMSR) && > >> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > >> + return 0; > >> + > >> + /* Re-establish the PCIe link */ > >> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > >> + return rcar_pcie_wait_for_dl(pcie); > >> +} > >> + > >> +static const struct dev_pm_ops rcar_pcie_pm_ops = { > >> + .resume_noirq = rcar_pcie_resume_noirq, > >> +}; > > > > I think there's the beginning of a convention to use #ifdef > > CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think > > we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. > > Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the > resume_noirq directly. Fair enough. I guess in this case if CONFIG_PM_SLEEP is unset, you set the pointer, which avoids the "unused function" warning, but we just never use that function pointer. My intent is to avoid needless differences when possible, so when I review things like this I look at how other drivers do things. It looks like all the other controllers use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() or similar: git grep -A3 "static.*dev_pm_ops" drivers/pci/controller In the rcar case you only need the resume_fn, not the suspend_fn, so SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() does a little more than you need and you'd have to pass NULL for suspend_fn. I didn't check them all (suspend_noirq, freeze_noirq, poweroff_noirq), but at least for suspend_noirq, all users check for NULL before calling through the .suspend_noirq() function pointer, so I think that should be safe. This *does* raise the question of whether you should be setting .thaw_noirq and .restore_noirq in addition to .resume_noirq, as SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() would. I'm not a PM person, but maybe Rafael or others will chime in. Bjorn ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-08 17:17 ` Bjorn Helgaas @ 2019-03-08 21:35 ` Marek Vasut 2019-03-11 10:10 ` Rafael J. Wysocki 2019-03-19 16:18 ` Lorenzo Pieralisi 1 sibling, 1 reply; 18+ messages in thread From: Marek Vasut @ 2019-03-08 21:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc, linux-pm, Rafael J. Wysocki On 3/8/19 6:17 PM, Bjorn Helgaas wrote: > [+cc linux-pm, Rafael for SET_NOIRQ_SYSTEM_SLEEP_PM_OPS question at the end] > > On Thu, Mar 07, 2019 at 11:49:34PM +0100, Marek Vasut wrote: >> On 3/7/19 9:50 PM, Bjorn Helgaas wrote: >>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >>>> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >>>> >>>> Reestablish the PCIe link very early in the resume process in case it >>>> went down to prevent PCI accesses from hanging the bus. Such accesses >>>> can happen early in the PCI resume process, in the resume_noirq, thus >>>> the link must be reestablished in the resume_noirq callback of the >>>> driver. >>>> >>>> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> >>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>> Cc: Wolfram Sang <wsa@the-dreams.de> >>>> Cc: linux-renesas-soc@vger.kernel.org >>>> --- >>>> V2: - Use BIT() macro for (1 << n) >>>> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not >>>> add extra changes to this function anymore >>>> - Make resume_noirq return early and clean up parenthesis therein >>>> --- >>>> drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c >>>> index c8febb009454..b8f8fb3bc640 100644 >>>> --- a/drivers/pci/controller/pcie-rcar.c >>>> +++ b/drivers/pci/controller/pcie-rcar.c >>>> @@ -46,6 +46,7 @@ >>>> >>>> /* Transfer control */ >>>> #define PCIETCTLR 0x02000 >>>> +#define DL_DOWN BIT(3) >>>> #define CFINIT 1 >>> >>> I saw discussion after the V1 patch about using BIT() and making >>> similar constants also use BIT() for consistency. That makes sense to >>> me, and I think the best way would be: >>> >>> 1) in *this* patch, use "#define DL_DOWN 8" >>> 2) in a followup patch, convert them all to BIT() >>> >>> That way each revision of pcie-rcar.c is self-consistent. >> >> But the BIT() macros are already cleaned , see commit >> 0ee40820989b330e24926d82953ffb9e1c7a8425 >> >> PCI: rcar: Clean up the macros > > Hmmm. Maybe I'm missing something, but it looks like 0ee40820989b > didn't touch CFINIT, DATA_LINK_ACTIVE, or MSIFE. Arguably, > LINK_SPEED_2_5GTS and LINK_SPEED_5_0GTS could use BIT() also. > > I guess I'm just old-school, but in my personal opinion, BIT() is more > trouble than it's worth. I'd rather see a complete bitmask because I > can easily match it with the typical pictures in a spec, multi-bit > fields are easy (you don't have to mix BIT() and GENMASK()), it gives > a hint about the register width, it's easy to match with a hexdump, > etc, e.g., > > #define DL_DOWN 0x00000008 > #define CFINIT 0x00000001 > > But I'm not suggesting that you get rid of BIT() in this driver. I'm > fine with it as long as it's used consistently. > > BTW, while we're looking at it, I think rcar_pci_read_reg() and > rcar_pci_write_reg() really should use "u32" instead of "unsigned > long", since they deal with hardware registers that are explicitly > 32 bits wide. OK, I can send those as separate patches. >>>> #define PCIETSTR 0x02004 >>>> #define DATA_LINK_ACTIVE 1 >>>> @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>> pcie = pci_host_bridge_priv(bridge); >>>> >>>> pcie->dev = dev; >>>> + platform_set_drvdata(pdev, pcie); >>>> >>>> err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); >>>> if (err) >>>> @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>> return err; >>>> } >>>> >>>> +static int rcar_pcie_resume_noirq(struct device *dev) >>>> +{ >>>> + struct rcar_pcie *pcie = dev_get_drvdata(dev); >>>> + >>>> + if (rcar_pci_read_reg(pcie, PMSR) && >>>> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) >>>> + return 0; >>>> + >>>> + /* Re-establish the PCIe link */ >>>> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); >>>> + return rcar_pcie_wait_for_dl(pcie); >>>> +} >>>> + >>>> +static const struct dev_pm_ops rcar_pcie_pm_ops = { >>>> + .resume_noirq = rcar_pcie_resume_noirq, >>>> +}; >>> >>> I think there's the beginning of a convention to use #ifdef >>> CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think >>> we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. >> >> Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the >> resume_noirq directly. > > Fair enough. I guess in this case if CONFIG_PM_SLEEP is unset, you > set the pointer, which avoids the "unused function" warning, but we > just never use that function pointer. > > My intent is to avoid needless differences when possible, so when I > review things like this I look at how other drivers do things. It > looks like all the other controllers use > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() or similar: > > git grep -A3 "static.*dev_pm_ops" drivers/pci/controller > > In the rcar case you only need the resume_fn, not the suspend_fn, so > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() does a little more than you need and > you'd have to pass NULL for suspend_fn. I didn't check them all > (suspend_noirq, freeze_noirq, poweroff_noirq), but at least for > suspend_noirq, all users check for NULL before calling through the > .suspend_noirq() function pointer, so I think that should be safe. > > This *does* raise the question of whether you should be setting > .thaw_noirq and .restore_noirq in addition to .resume_noirq, as > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() would. I'm not a PM person, but maybe > Rafael or others will chime in. OK, let's wait for the feedback ... -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-08 21:35 ` Marek Vasut @ 2019-03-11 10:10 ` Rafael J. Wysocki 0 siblings, 0 replies; 18+ messages in thread From: Rafael J. Wysocki @ 2019-03-11 10:10 UTC (permalink / raw) To: Marek Vasut Cc: Bjorn Helgaas, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc, linux-pm On Friday, March 8, 2019 10:35:49 PM CET Marek Vasut wrote: > On 3/8/19 6:17 PM, Bjorn Helgaas wrote: > > [+cc linux-pm, Rafael for SET_NOIRQ_SYSTEM_SLEEP_PM_OPS question at the end] > > > > On Thu, Mar 07, 2019 at 11:49:34PM +0100, Marek Vasut wrote: > >> On 3/7/19 9:50 PM, Bjorn Helgaas wrote: > >>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > >>>> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >>>> > >>>> Reestablish the PCIe link very early in the resume process in case it > >>>> went down to prevent PCI accesses from hanging the bus. Such accesses > >>>> can happen early in the PCI resume process, in the resume_noirq, thus > >>>> the link must be reestablished in the resume_noirq callback of the > >>>> driver. > >>>> > >>>> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >>>> Cc: Simon Horman <horms+renesas@verge.net.au> > >>>> Cc: Wolfram Sang <wsa@the-dreams.de> > >>>> Cc: linux-renesas-soc@vger.kernel.org > >>>> --- > >>>> V2: - Use BIT() macro for (1 << n) > >>>> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not > >>>> add extra changes to this function anymore > >>>> - Make resume_noirq return early and clean up parenthesis therein > >>>> --- > >>>> drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ > >>>> 1 file changed, 20 insertions(+) > >>>> > >>>> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > >>>> index c8febb009454..b8f8fb3bc640 100644 > >>>> --- a/drivers/pci/controller/pcie-rcar.c > >>>> +++ b/drivers/pci/controller/pcie-rcar.c > >>>> @@ -46,6 +46,7 @@ > >>>> > >>>> /* Transfer control */ > >>>> #define PCIETCTLR 0x02000 > >>>> +#define DL_DOWN BIT(3) > >>>> #define CFINIT 1 > >>> > >>> I saw discussion after the V1 patch about using BIT() and making > >>> similar constants also use BIT() for consistency. That makes sense to > >>> me, and I think the best way would be: > >>> > >>> 1) in *this* patch, use "#define DL_DOWN 8" > >>> 2) in a followup patch, convert them all to BIT() > >>> > >>> That way each revision of pcie-rcar.c is self-consistent. > >> > >> But the BIT() macros are already cleaned , see commit > >> 0ee40820989b330e24926d82953ffb9e1c7a8425 > >> > >> PCI: rcar: Clean up the macros > > > > Hmmm. Maybe I'm missing something, but it looks like 0ee40820989b > > didn't touch CFINIT, DATA_LINK_ACTIVE, or MSIFE. Arguably, > > LINK_SPEED_2_5GTS and LINK_SPEED_5_0GTS could use BIT() also. > > > > I guess I'm just old-school, but in my personal opinion, BIT() is more > > trouble than it's worth. I'd rather see a complete bitmask because I > > can easily match it with the typical pictures in a spec, multi-bit > > fields are easy (you don't have to mix BIT() and GENMASK()), it gives > > a hint about the register width, it's easy to match with a hexdump, > > etc, e.g., > > > > #define DL_DOWN 0x00000008 > > #define CFINIT 0x00000001 > > > > But I'm not suggesting that you get rid of BIT() in this driver. I'm > > fine with it as long as it's used consistently. > > > > BTW, while we're looking at it, I think rcar_pci_read_reg() and > > rcar_pci_write_reg() really should use "u32" instead of "unsigned > > long", since they deal with hardware registers that are explicitly > > 32 bits wide. > > OK, I can send those as separate patches. > > >>>> #define PCIETSTR 0x02004 > >>>> #define DATA_LINK_ACTIVE 1 > >>>> @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >>>> pcie = pci_host_bridge_priv(bridge); > >>>> > >>>> pcie->dev = dev; > >>>> + platform_set_drvdata(pdev, pcie); > >>>> > >>>> err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); > >>>> if (err) > >>>> @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >>>> return err; > >>>> } > >>>> > >>>> +static int rcar_pcie_resume_noirq(struct device *dev) > >>>> +{ > >>>> + struct rcar_pcie *pcie = dev_get_drvdata(dev); > >>>> + > >>>> + if (rcar_pci_read_reg(pcie, PMSR) && > >>>> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > >>>> + return 0; > >>>> + > >>>> + /* Re-establish the PCIe link */ > >>>> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > >>>> + return rcar_pcie_wait_for_dl(pcie); > >>>> +} > >>>> + > >>>> +static const struct dev_pm_ops rcar_pcie_pm_ops = { > >>>> + .resume_noirq = rcar_pcie_resume_noirq, > >>>> +}; > >>> > >>> I think there's the beginning of a convention to use #ifdef > >>> CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think > >>> we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. > >> > >> Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the > >> resume_noirq directly. > > > > Fair enough. I guess in this case if CONFIG_PM_SLEEP is unset, you > > set the pointer, which avoids the "unused function" warning, but we > > just never use that function pointer. > > > > My intent is to avoid needless differences when possible, so when I > > review things like this I look at how other drivers do things. It > > looks like all the other controllers use > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() or similar: > > > > git grep -A3 "static.*dev_pm_ops" drivers/pci/controller > > > > In the rcar case you only need the resume_fn, not the suspend_fn, so > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() does a little more than you need and > > you'd have to pass NULL for suspend_fn. I didn't check them all > > (suspend_noirq, freeze_noirq, poweroff_noirq), but at least for > > suspend_noirq, all users check for NULL before calling through the > > .suspend_noirq() function pointer, so I think that should be safe. Yes, this should be safe from the NULL pointer checks perspective. > > This *does* raise the question of whether you should be setting > > .thaw_noirq and .restore_noirq in addition to .resume_noirq, as > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() would. I'm not a PM person, but maybe > > Rafael or others will chime in. That depends on what happens during hibernation/restore. The driver may not need to do anything before the memory snapshot is taken or before the restore kernel jumps back to the image kernel. In that case you don't need freeze/thaw/restore_noirq at all. However, chances are that it needs to do something and in that case doing the same thing as during system-wide suspend/resume generally works, even though it may not be particularly energy-efficient. That's why it very ofther is done. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-08 17:17 ` Bjorn Helgaas 2019-03-08 21:35 ` Marek Vasut @ 2019-03-19 16:18 ` Lorenzo Pieralisi 1 sibling, 0 replies; 18+ messages in thread From: Lorenzo Pieralisi @ 2019-03-19 16:18 UTC (permalink / raw) To: Bjorn Helgaas Cc: Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc, linux-pm, Rafael J. Wysocki On Fri, Mar 08, 2019 at 11:17:38AM -0600, Bjorn Helgaas wrote: [...] > > >> +static int rcar_pcie_resume_noirq(struct device *dev) > > >> +{ > > >> + struct rcar_pcie *pcie = dev_get_drvdata(dev); > > >> + > > >> + if (rcar_pci_read_reg(pcie, PMSR) && > > >> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > > >> + return 0; > > >> + > > >> + /* Re-establish the PCIe link */ > > >> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > > >> + return rcar_pcie_wait_for_dl(pcie); > > >> +} > > >> + > > >> +static const struct dev_pm_ops rcar_pcie_pm_ops = { > > >> + .resume_noirq = rcar_pcie_resume_noirq, > > >> +}; > > > > > > I think there's the beginning of a convention to use #ifdef > > > CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think > > > we'll get a warning about unused code when CONFIG_PM_SLEEP is unset. > > > > Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the > > resume_noirq directly. > > Fair enough. I guess in this case if CONFIG_PM_SLEEP is unset, you > set the pointer, which avoids the "unused function" warning, but we > just never use that function pointer. > > My intent is to avoid needless differences when possible, so when I > review things like this I look at how other drivers do things. It > looks like all the other controllers use > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() or similar: > > git grep -A3 "static.*dev_pm_ops" drivers/pci/controller > > In the rcar case you only need the resume_fn, not the suspend_fn, so > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() does a little more than you need and > you'd have to pass NULL for suspend_fn. I didn't check them all > (suspend_noirq, freeze_noirq, poweroff_noirq), but at least for > suspend_noirq, all users check for NULL before calling through the > .suspend_noirq() function pointer, so I think that should be safe. It is a matter of consistency across drivers, yes, but that's something I can easily solve with a clean-up patch on top of Marek's one. I would merge this patch as-is and convert all the drivers to a uniform convention (which one I shall see). On a side note, this patch looks like a fix to me (even though it is not trivial for me to add a sensible Fixes: tag) and should be treated as such, so it should go in one of the upcoming -rc* (and I float the idea of sending it to stable kernels on which S2R is basically broken). Lorenzo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-02-17 13:24 [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq marek.vasut 2019-02-19 17:24 ` Wolfram Sang 2019-03-07 20:50 ` Bjorn Helgaas @ 2019-03-22 11:31 ` Lorenzo Pieralisi 2019-03-22 12:09 ` Marek Vasut 2019-03-25 16:54 ` Lorenzo Pieralisi 3 siblings, 1 reply; 18+ messages in thread From: Lorenzo Pieralisi @ 2019-03-22 11:31 UTC (permalink / raw) To: marek.vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > Reestablish the PCIe link very early in the resume process in case it > went down to prevent PCI accesses from hanging the bus. Such accesses Hi Marek, Kazufumi, Apologies for the delay. Just as a clarification, when you state "in case it went down" isn't this supposed to happen for every suspend cycle ? Let me know and I will add a comment to the patch commit log. > can happen early in the PCI resume process, in the resume_noirq, thus > the link must be reestablished in the resume_noirq callback of the > driver. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org This looks like a fix (most likely fixing initial S2R support, please help me chase the commit ID), should we consider it for stable kernels ? Without it I understand S2R is actually broken on platforms with this host bridge. Thanks, Lorenzo > --- > V2: - Use BIT() macro for (1 << n) > - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not > add extra changes to this function anymore > - Make resume_noirq return early and clean up parenthesis therein > --- > drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index c8febb009454..b8f8fb3bc640 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -46,6 +46,7 @@ > > /* Transfer control */ > #define PCIETCTLR 0x02000 > +#define DL_DOWN BIT(3) > #define CFINIT 1 > #define PCIETSTR 0x02004 > #define DATA_LINK_ACTIVE 1 > @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > pcie = pci_host_bridge_priv(bridge); > > pcie->dev = dev; > + platform_set_drvdata(pdev, pcie); > > err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); > if (err) > @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume_noirq(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + > + if (rcar_pci_read_reg(pcie, PMSR) && > + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > + return 0; > + > + /* Re-establish the PCIe link */ > + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > + return rcar_pcie_wait_for_dl(pcie); > +} > + > +static const struct dev_pm_ops rcar_pcie_pm_ops = { > + .resume_noirq = rcar_pcie_resume_noirq, > +}; > + > static struct platform_driver rcar_pcie_driver = { > .driver = { > .name = "rcar-pcie", > .of_match_table = rcar_pcie_of_match, > + .pm = &rcar_pcie_pm_ops, > .suppress_bind_attrs = true, > }, > .probe = rcar_pcie_probe, > -- > 2.19.2 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 11:31 ` Lorenzo Pieralisi @ 2019-03-22 12:09 ` Marek Vasut 2019-03-22 12:18 ` Lorenzo Pieralisi 0 siblings, 1 reply; 18+ messages in thread From: Marek Vasut @ 2019-03-22 12:09 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> >> Reestablish the PCIe link very early in the resume process in case it >> went down to prevent PCI accesses from hanging the bus. Such accesses > > Hi Marek, Kazufumi, Hi, > Apologies for the delay. > > Just as a clarification, when you state "in case it went down" isn't > this supposed to happen for every suspend cycle ? Let me know and I > will add a comment to the patch commit log. It does happen on every suspend/resume cycle and if you manually put a remote endpoint into non-L0 state. >> can happen early in the PCI resume process, in the resume_noirq, thus >> the link must be reestablished in the resume_noirq callback of the >> driver. >> >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: linux-renesas-soc@vger.kernel.org > > This looks like a fix (most likely fixing initial S2R support, please > help me chase the commit ID), should we consider it for stable kernels ? > > Without it I understand S2R is actually broken on platforms with this > host bridge. I don't think this ever worked, so it's hard to find a Fixes: commit for this. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 12:09 ` Marek Vasut @ 2019-03-22 12:18 ` Lorenzo Pieralisi 2019-03-22 12:29 ` Geert Uytterhoeven 0 siblings, 1 reply; 18+ messages in thread From: Lorenzo Pieralisi @ 2019-03-22 12:18 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On Fri, Mar 22, 2019 at 01:09:13PM +0100, Marek Vasut wrote: > On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: > > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >> > >> Reestablish the PCIe link very early in the resume process in case it > >> went down to prevent PCI accesses from hanging the bus. Such accesses > > > > Hi Marek, Kazufumi, > > Hi, > > > Apologies for the delay. > > > > Just as a clarification, when you state "in case it went down" isn't > > this supposed to happen for every suspend cycle ? Let me know and I > > will add a comment to the patch commit log. > > It does happen on every suspend/resume cycle and if you manually put a > remote endpoint into non-L0 state. Ok I will update the log then. > >> can happen early in the PCI resume process, in the resume_noirq, thus > >> the link must be reestablished in the resume_noirq callback of the > >> driver. > >> > >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >> Cc: Simon Horman <horms+renesas@verge.net.au> > >> Cc: Wolfram Sang <wsa@the-dreams.de> > >> Cc: linux-renesas-soc@vger.kernel.org > > > > This looks like a fix (most likely fixing initial S2R support, please > > help me chase the commit ID), should we consider it for stable kernels ? > > > > Without it I understand S2R is actually broken on platforms with this > > host bridge. > I don't think this ever worked, so it's hard to find a Fixes: commit for > this. If we want to send it to stable kernels we have to select which versions we are covering. I think the only options for a Fixes: tag are either the initial S2R support commit for the platforms this driver runs on or the initial driver commit that harks back to v3.16 AFAICS. I will queue it next week so there is some time to think about it. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 12:18 ` Lorenzo Pieralisi @ 2019-03-22 12:29 ` Geert Uytterhoeven 2019-03-22 12:33 ` Marek Vasut 0 siblings, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-22 12:29 UTC (permalink / raw) To: Lorenzo Pieralisi, Marek Vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, Linux-Renesas Hi Lorenzo, Marek, On Fri, Mar 22, 2019 at 1:18 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Mar 22, 2019 at 01:09:13PM +0100, Marek Vasut wrote: > > On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: > > > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > > >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > >> > > >> Reestablish the PCIe link very early in the resume process in case it > > >> went down to prevent PCI accesses from hanging the bus. Such accesses > > >> can happen early in the PCI resume process, in the resume_noirq, thus > > >> the link must be reestablished in the resume_noirq callback of the > > >> driver. > > > > > > This looks like a fix (most likely fixing initial S2R support, please > > > help me chase the commit ID), should we consider it for stable kernels ? > > > > > > Without it I understand S2R is actually broken on platforms with this > > > host bridge. > > I don't think this ever worked, so it's hard to find a Fixes: commit for > > this. > > If we want to send it to stable kernels we have to select which versions > we are covering. I think the only options for a Fixes: tag are either > the initial S2R support commit for the platforms this driver runs on > or the initial driver commit that harks back to v3.16 AFAICS. This only started to become an issue when support for arm64 platforms was added, where PSCI may power down the SoC, right? Hence: Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 12:29 ` Geert Uytterhoeven @ 2019-03-22 12:33 ` Marek Vasut 2019-03-22 12:42 ` Geert Uytterhoeven 0 siblings, 1 reply; 18+ messages in thread From: Marek Vasut @ 2019-03-22 12:33 UTC (permalink / raw) To: Geert Uytterhoeven, Lorenzo Pieralisi Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, Linux-Renesas On 3/22/19 1:29 PM, Geert Uytterhoeven wrote: > Hi Lorenzo, Marek, Hi, > On Fri, Mar 22, 2019 at 1:18 PM Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> On Fri, Mar 22, 2019 at 01:09:13PM +0100, Marek Vasut wrote: >>> On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: >>>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >>>>> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >>>>> >>>>> Reestablish the PCIe link very early in the resume process in case it >>>>> went down to prevent PCI accesses from hanging the bus. Such accesses >>>>> can happen early in the PCI resume process, in the resume_noirq, thus >>>>> the link must be reestablished in the resume_noirq callback of the >>>>> driver. >>>> >>>> This looks like a fix (most likely fixing initial S2R support, please >>>> help me chase the commit ID), should we consider it for stable kernels ? >>>> >>>> Without it I understand S2R is actually broken on platforms with this >>>> host bridge. >>> I don't think this ever worked, so it's hard to find a Fixes: commit for >>> this. >> >> If we want to send it to stable kernels we have to select which versions >> we are covering. I think the only options for a Fixes: tag are either >> the initial S2R support commit for the platforms this driver runs on >> or the initial driver commit that harks back to v3.16 AFAICS. > > This only started to become an issue when support for arm64 platforms > was added, where PSCI may power down the SoC, right? Wouldn't you also hit this on ARM32 LPAE ones ? > Hence: > Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") > > Gr{oetje,eeting}s, > > Geert > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 12:33 ` Marek Vasut @ 2019-03-22 12:42 ` Geert Uytterhoeven 2019-03-22 12:49 ` Marek Vasut 0 siblings, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-22 12:42 UTC (permalink / raw) To: Marek Vasut Cc: Lorenzo Pieralisi, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, Linux-Renesas Hi Marek, On Fri, Mar 22, 2019 at 1:33 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/22/19 1:29 PM, Geert Uytterhoeven wrote: > > On Fri, Mar 22, 2019 at 1:18 PM Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > >> On Fri, Mar 22, 2019 at 01:09:13PM +0100, Marek Vasut wrote: > >>> On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: > >>>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > >>>>> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > >>>>> > >>>>> Reestablish the PCIe link very early in the resume process in case it > >>>>> went down to prevent PCI accesses from hanging the bus. Such accesses > >>>>> can happen early in the PCI resume process, in the resume_noirq, thus > >>>>> the link must be reestablished in the resume_noirq callback of the > >>>>> driver. > >>>> > >>>> This looks like a fix (most likely fixing initial S2R support, please > >>>> help me chase the commit ID), should we consider it for stable kernels ? > >>>> > >>>> Without it I understand S2R is actually broken on platforms with this > >>>> host bridge. > >>> I don't think this ever worked, so it's hard to find a Fixes: commit for > >>> this. > >> > >> If we want to send it to stable kernels we have to select which versions > >> we are covering. I think the only options for a Fixes: tag are either > >> the initial S2R support commit for the platforms this driver runs on > >> or the initial driver commit that harks back to v3.16 AFAICS. > > > > This only started to become an issue when support for arm64 platforms > > was added, where PSCI may power down the SoC, right? > > Wouldn't you also hit this on ARM32 LPAE ones ? This is about the PCI link issue on system resume, right? > > Hence: > > Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-22 12:42 ` Geert Uytterhoeven @ 2019-03-22 12:49 ` Marek Vasut 0 siblings, 0 replies; 18+ messages in thread From: Marek Vasut @ 2019-03-22 12:49 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Lorenzo Pieralisi, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, Linux-Renesas On 3/22/19 1:42 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Fri, Mar 22, 2019 at 1:33 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 3/22/19 1:29 PM, Geert Uytterhoeven wrote: >>> On Fri, Mar 22, 2019 at 1:18 PM Lorenzo Pieralisi >>> <lorenzo.pieralisi@arm.com> wrote: >>>> On Fri, Mar 22, 2019 at 01:09:13PM +0100, Marek Vasut wrote: >>>>> On 3/22/19 12:31 PM, Lorenzo Pieralisi wrote: >>>>>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >>>>>>> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >>>>>>> >>>>>>> Reestablish the PCIe link very early in the resume process in case it >>>>>>> went down to prevent PCI accesses from hanging the bus. Such accesses >>>>>>> can happen early in the PCI resume process, in the resume_noirq, thus >>>>>>> the link must be reestablished in the resume_noirq callback of the >>>>>>> driver. >>>>>> >>>>>> This looks like a fix (most likely fixing initial S2R support, please >>>>>> help me chase the commit ID), should we consider it for stable kernels ? >>>>>> >>>>>> Without it I understand S2R is actually broken on platforms with this >>>>>> host bridge. >>>>> I don't think this ever worked, so it's hard to find a Fixes: commit for >>>>> this. >>>> >>>> If we want to send it to stable kernels we have to select which versions >>>> we are covering. I think the only options for a Fixes: tag are either >>>> the initial S2R support commit for the platforms this driver runs on >>>> or the initial driver commit that harks back to v3.16 AFAICS. >>> >>> This only started to become an issue when support for arm64 platforms >>> was added, where PSCI may power down the SoC, right? >> >> Wouldn't you also hit this on ARM32 LPAE ones ? > > This is about the PCI link issue on system resume, right? Forget what I said, yes, I mixed this with the MSI 64bit patch. >>> Hence: >>> Fixes: e015f88c368da1e6 ("PCI: rcar: Add support for R-Car H3 to pcie-rcar") > > Gr{oetje,eeting}s, > > Geert > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-02-17 13:24 [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq marek.vasut ` (2 preceding siblings ...) 2019-03-22 11:31 ` Lorenzo Pieralisi @ 2019-03-25 16:54 ` Lorenzo Pieralisi 2019-03-25 19:44 ` Marek Vasut 3 siblings, 1 reply; 18+ messages in thread From: Lorenzo Pieralisi @ 2019-03-25 16:54 UTC (permalink / raw) To: marek.vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > Reestablish the PCIe link very early in the resume process in case it > went down to prevent PCI accesses from hanging the bus. Such accesses > can happen early in the PCI resume process, in the resume_noirq, thus > the link must be reestablished in the resume_noirq callback of the > driver. > > Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Use BIT() macro for (1 << n) > - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not > add extra changes to this function anymore > - Make resume_noirq return early and clean up parenthesis therein > --- > drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) I applied it to pci/controller-fixes but the kbot barfed at it so we need to rework it. Thanks, Lorenzo > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index c8febb009454..b8f8fb3bc640 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -46,6 +46,7 @@ > > /* Transfer control */ > #define PCIETCTLR 0x02000 > +#define DL_DOWN BIT(3) > #define CFINIT 1 > #define PCIETSTR 0x02004 > #define DATA_LINK_ACTIVE 1 > @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > pcie = pci_host_bridge_priv(bridge); > > pcie->dev = dev; > + platform_set_drvdata(pdev, pcie); > > err = pci_parse_request_of_pci_ranges(dev, &pcie->resources, NULL); > if (err) > @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct platform_device *pdev) > return err; > } > > +static int rcar_pcie_resume_noirq(struct device *dev) > +{ > + struct rcar_pcie *pcie = dev_get_drvdata(dev); > + > + if (rcar_pci_read_reg(pcie, PMSR) && > + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) > + return 0; > + > + /* Re-establish the PCIe link */ > + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR); > + return rcar_pcie_wait_for_dl(pcie); > +} > + > +static const struct dev_pm_ops rcar_pcie_pm_ops = { > + .resume_noirq = rcar_pcie_resume_noirq, > +}; > + > static struct platform_driver rcar_pcie_driver = { > .driver = { > .name = "rcar-pcie", > .of_match_table = rcar_pcie_of_match, > + .pm = &rcar_pcie_pm_ops, > .suppress_bind_attrs = true, > }, > .probe = rcar_pcie_probe, > -- > 2.19.2 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2019-03-25 16:54 ` Lorenzo Pieralisi @ 2019-03-25 19:44 ` Marek Vasut 0 siblings, 0 replies; 18+ messages in thread From: Marek Vasut @ 2019-03-25 19:44 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc On 3/25/19 5:54 PM, Lorenzo Pieralisi wrote: > On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.vasut@gmail.com wrote: >> From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> >> Reestablish the PCIe link very early in the resume process in case it >> went down to prevent PCI accesses from hanging the bus. Such accesses >> can happen early in the PCI resume process, in the resume_noirq, thus >> the link must be reestablished in the resume_noirq callback of the >> driver. >> >> Signed-off-by: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: - Use BIT() macro for (1 << n) >> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not >> add extra changes to this function anymore >> - Make resume_noirq return early and clean up parenthesis therein >> --- >> drivers/pci/controller/pcie-rcar.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) > > I applied it to pci/controller-fixes but the kbot barfed at it so we > need to rework it. Right, PMSR definition added, updated commit message retained, patch resubmitted as V3. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-03-25 19:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-17 13:24 [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq marek.vasut 2019-02-19 17:24 ` Wolfram Sang 2019-02-25 9:58 ` Simon Horman 2019-03-07 20:50 ` Bjorn Helgaas 2019-03-07 22:49 ` Marek Vasut 2019-03-08 17:17 ` Bjorn Helgaas 2019-03-08 21:35 ` Marek Vasut 2019-03-11 10:10 ` Rafael J. Wysocki 2019-03-19 16:18 ` Lorenzo Pieralisi 2019-03-22 11:31 ` Lorenzo Pieralisi 2019-03-22 12:09 ` Marek Vasut 2019-03-22 12:18 ` Lorenzo Pieralisi 2019-03-22 12:29 ` Geert Uytterhoeven 2019-03-22 12:33 ` Marek Vasut 2019-03-22 12:42 ` Geert Uytterhoeven 2019-03-22 12:49 ` Marek Vasut 2019-03-25 16:54 ` Lorenzo Pieralisi 2019-03-25 19:44 ` Marek Vasut
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).