* [PATCH V2 0/5] PCI: rcar: Add suspend/resume support @ 2017-11-10 21:58 Marek Vasut 2017-11-10 21:58 ` [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() Marek Vasut ` (4 more replies) 0 siblings, 5 replies; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 UTC (permalink / raw) To: linux-pci Cc: Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc This patchset adds support for suspend/resume on the Renesas RCar PCIe controller. First two patches clean the driver up a little, while the remaining three add the required suspend/resume functionality. Kazufumi Ikeda (2): PCI: rcar: Add the initialization of PCIe link in resume_noirq PCI: rcar: Add the suspend/resume for pcie-rcar driver Marek Vasut (2): PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() PCI: rcar: Clean up the macros Phil Edworthy (1): PCI: rcar: Support runtime PM, link state L1 handling drivers/pci/host/pcie-rcar.c | 182 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 146 insertions(+), 36 deletions(-) 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 -- 2.11.0 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut @ 2017-11-10 21:58 ` Marek Vasut 2017-11-13 7:03 ` Simon Horman 2017-11-10 21:58 ` [PATCH V2 2/5] PCI: rcar: Clean up the macros Marek Vasut ` (3 subsequent siblings) 4 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 UTC (permalink / raw) To: linux-pci Cc: Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc The data link active signal usually takes ~20 uSec to be asserted, poll the bit more often to avoid useless delays in this function. 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: New patch --- drivers/pci/host/pcie-rcar.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index e00f865952d5..351e1276b90a 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -531,13 +531,13 @@ static void phy_write_reg(struct rcar_pcie *pcie, static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie) { - unsigned int timeout = 10; + unsigned int timeout = 10000; while (timeout--) { if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE)) return 0; - msleep(5); + udelay(5); } return -ETIMEDOUT; -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() 2017-11-10 21:58 ` [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() Marek Vasut @ 2017-11-13 7:03 ` Simon Horman 0 siblings, 0 replies; 33+ messages in thread From: Simon Horman @ 2017-11-13 7:03 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On Fri, Nov 10, 2017 at 10:58:39PM +0100, Marek Vasut wrote: > The data link active signal usually takes ~20 uSec to be asserted, > poll the bit more often to avoid useless delays in this function. > > 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 Acked-by: Simon Horman <horms+renesas@verge.net.au> > --- > V2: New patch > --- > drivers/pci/host/pcie-rcar.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index e00f865952d5..351e1276b90a 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -531,13 +531,13 @@ static void phy_write_reg(struct rcar_pcie *pcie, > > static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie) > { > - unsigned int timeout = 10; > + unsigned int timeout = 10000; > > while (timeout--) { > if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE)) > return 0; > > - msleep(5); > + udelay(5); > } > > return -ETIMEDOUT; > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V2 2/5] PCI: rcar: Clean up the macros 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut 2017-11-10 21:58 ` [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() Marek Vasut @ 2017-11-10 21:58 ` Marek Vasut 2017-11-13 7:03 ` Simon Horman 2017-11-10 21:58 ` [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq Marek Vasut ` (2 subsequent siblings) 4 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 UTC (permalink / raw) To: linux-pci Cc: Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang, linux-renesas-soc Just clean up the macros in the RCar PCI driver, no functional change. 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: New patch --- drivers/pci/host/pcie-rcar.c | 52 ++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 351e1276b90a..811e8194ef74 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -33,9 +33,9 @@ #define PCIECAR 0x000010 #define PCIECCTLR 0x000018 -#define CONFIG_SEND_ENABLE (1 << 31) +#define CONFIG_SEND_ENABLE BIT(31) #define TYPE0 (0 << 8) -#define TYPE1 (1 << 8) +#define TYPE1 BIT(8) #define PCIECDR 0x000020 #define PCIEMSR 0x000028 #define PCIEINTXR 0x000400 @@ -47,7 +47,7 @@ #define PCIETSTR 0x02004 #define DATA_LINK_ACTIVE 1 #define PCIEERRFR 0x02020 -#define UNSUPPORTED_REQUEST (1 << 4) +#define UNSUPPORTED_REQUEST BIT(4) #define PCIEMSIFR 0x02044 #define PCIEMSIALR 0x02048 #define MSIFE 1 @@ -60,17 +60,17 @@ /* local address reg & mask */ #define PCIELAR(x) (0x02200 + ((x) * 0x20)) #define PCIELAMR(x) (0x02208 + ((x) * 0x20)) -#define LAM_PREFETCH (1 << 3) -#define LAM_64BIT (1 << 2) -#define LAR_ENABLE (1 << 1) +#define LAM_PREFETCH BIT(3) +#define LAM_64BIT BIT(2) +#define LAR_ENABLE BIT(1) /* PCIe address reg & mask */ #define PCIEPALR(x) (0x03400 + ((x) * 0x20)) #define PCIEPAUR(x) (0x03404 + ((x) * 0x20)) #define PCIEPAMR(x) (0x03408 + ((x) * 0x20)) #define PCIEPTCTLR(x) (0x0340c + ((x) * 0x20)) -#define PAR_ENABLE (1 << 31) -#define IO_SPACE (1 << 8) +#define PAR_ENABLE BIT(31) +#define IO_SPACE BIT(8) /* Configuration */ #define PCICONF(x) (0x010000 + ((x) * 0x4)) @@ -82,23 +82,23 @@ #define IDSETR1 0x011004 #define TLCTLR 0x011048 #define MACSR 0x011054 -#define SPCHGFIN (1 << 4) -#define SPCHGFAIL (1 << 6) -#define SPCHGSUC (1 << 7) +#define SPCHGFIN BIT(4) +#define SPCHGFAIL BIT(6) +#define SPCHGSUC BIT(7) #define LINK_SPEED (0xf << 16) #define LINK_SPEED_2_5GTS (1 << 16) #define LINK_SPEED_5_0GTS (2 << 16) #define MACCTLR 0x011058 -#define SPEED_CHANGE (1 << 24) -#define SCRAMBLE_DISABLE (1 << 27) +#define SPEED_CHANGE BIT(24) +#define SCRAMBLE_DISABLE BIT(27) #define MACS2R 0x011078 #define MACCGSPSETR 0x011084 -#define SPCNGRSN (1 << 31) +#define SPCNGRSN BIT(31) /* R-Car H1 PHY */ #define H1_PCIEPHYADRR 0x04000c -#define WRITE_CMD (1 << 16) -#define PHY_ACK (1 << 24) +#define WRITE_CMD BIT(16) +#define PHY_ACK BIT(24) #define RATE_POS 12 #define LANE_POS 8 #define ADR_POS 0 @@ -110,19 +110,19 @@ #define GEN2_PCIEPHYDATA 0x784 #define GEN2_PCIEPHYCTRL 0x78c -#define INT_PCI_MSI_NR 32 +#define INT_PCI_MSI_NR 32 -#define RCONF(x) (PCICONF(0)+(x)) -#define RPMCAP(x) (PMCAP(0)+(x)) -#define REXPCAP(x) (EXPCAP(0)+(x)) -#define RVCCAP(x) (VCCAP(0)+(x)) +#define RCONF(x) (PCICONF(0) + (x)) +#define RPMCAP(x) (PMCAP(0) + (x)) +#define REXPCAP(x) (EXPCAP(0) + (x)) +#define RVCCAP(x) (VCCAP(0) + (x)) -#define PCIE_CONF_BUS(b) (((b) & 0xff) << 24) -#define PCIE_CONF_DEV(d) (((d) & 0x1f) << 19) -#define PCIE_CONF_FUNC(f) (((f) & 0x7) << 16) +#define PCIE_CONF_BUS(b) (((b) & 0xff) << 24) +#define PCIE_CONF_DEV(d) (((d) & 0x1f) << 19) +#define PCIE_CONF_FUNC(f) (((f) & 0x7) << 16) -#define RCAR_PCI_MAX_RESOURCES 4 -#define MAX_NR_INBOUND_MAPS 6 +#define RCAR_PCI_MAX_RESOURCES 4 +#define MAX_NR_INBOUND_MAPS 6 struct rcar_msi { DECLARE_BITMAP(used, INT_PCI_MSI_NR); -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH V2 2/5] PCI: rcar: Clean up the macros 2017-11-10 21:58 ` [PATCH V2 2/5] PCI: rcar: Clean up the macros Marek Vasut @ 2017-11-13 7:03 ` Simon Horman 2017-11-13 18:11 ` Marek Vasut 0 siblings, 1 reply; 33+ messages in thread From: Simon Horman @ 2017-11-13 7:03 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On Fri, Nov 10, 2017 at 10:58:40PM +0100, Marek Vasut wrote: > Just clean up the macros in the RCar PCI driver, no functional change. Could you describe the cleanup in slightly more detail? My reading is 1. Use BIT() macro 2. tidy up whitespace. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 2/5] PCI: rcar: Clean up the macros 2017-11-13 7:03 ` Simon Horman @ 2017-11-13 18:11 ` Marek Vasut 2017-11-15 13:28 ` Simon Horman 0 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2017-11-13 18:11 UTC (permalink / raw) To: Simon Horman Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On 11/13/2017 08:03 AM, Simon Horman wrote: > On Fri, Nov 10, 2017 at 10:58:40PM +0100, Marek Vasut wrote: >> Just clean up the macros in the RCar PCI driver, no functional change. > > Could you describe the cleanup in slightly more detail? > My reading is 1. Use BIT() macro 2. tidy up whitespace. > That's all there is, indeed. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 2/5] PCI: rcar: Clean up the macros 2017-11-13 18:11 ` Marek Vasut @ 2017-11-15 13:28 ` Simon Horman 2017-11-22 11:20 ` Marek Vasut 0 siblings, 1 reply; 33+ messages in thread From: Simon Horman @ 2017-11-15 13:28 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On Mon, Nov 13, 2017 at 07:11:54PM +0100, Marek Vasut wrote: > On 11/13/2017 08:03 AM, Simon Horman wrote: > > On Fri, Nov 10, 2017 at 10:58:40PM +0100, Marek Vasut wrote: > >> Just clean up the macros in the RCar PCI driver, no functional change. > > > > Could you describe the cleanup in slightly more detail? > > My reading is 1. Use BIT() macro 2. tidy up whitespace. > > > That's all there is, indeed. Right, but I'd rather that the changelog be expanded to include that information. With that fixed feel free to add: Acked-by: Simon Horman <horms+renesas@verge.net.au> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 2/5] PCI: rcar: Clean up the macros 2017-11-15 13:28 ` Simon Horman @ 2017-11-22 11:20 ` Marek Vasut 0 siblings, 0 replies; 33+ messages in thread From: Marek Vasut @ 2017-11-22 11:20 UTC (permalink / raw) To: Simon Horman Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On 11/15/2017 02:28 PM, Simon Horman wrote: > On Mon, Nov 13, 2017 at 07:11:54PM +0100, Marek Vasut wrote: >> On 11/13/2017 08:03 AM, Simon Horman wrote: >>> On Fri, Nov 10, 2017 at 10:58:40PM +0100, Marek Vasut wrote: >>>> Just clean up the macros in the RCar PCI driver, no functional change. >>> >>> Could you describe the cleanup in slightly more detail? >>> My reading is 1. Use BIT() macro 2. tidy up whitespace. >>> >> That's all there is, indeed. > > Right, but I'd rather that the changelog be expanded to include that > information. With that fixed feel free to add: Fixed > Acked-by: Simon Horman <horms+renesas@verge.net.au> > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut 2017-11-10 21:58 ` [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() Marek Vasut 2017-11-10 21:58 ` [PATCH V2 2/5] PCI: rcar: Clean up the macros Marek Vasut @ 2017-11-10 21:58 ` Marek Vasut 2017-11-13 7:05 ` Simon Horman 2017-11-10 21:58 ` [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling Marek Vasut 2017-11-10 21:58 ` [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver Marek Vasut 4 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 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/host/pcie-rcar.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 811e8194ef74..ab61829db389 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -43,6 +43,7 @@ /* Transfer control */ #define PCIETCTLR 0x02000 +#define DL_DOWN BIT(3) #define CFINIT 1 #define PCIETSTR 0x02004 #define DATA_LINK_ACTIVE 1 @@ -1107,6 +1108,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) pcie = pci_host_bridge_priv(bridge); pcie->dev = dev; + platform_set_drvdata(pdev, pcie); INIT_LIST_HEAD(&pcie->resources); @@ -1167,10 +1169,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.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq 2017-11-10 21:58 ` [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq Marek Vasut @ 2017-11-13 7:05 ` Simon Horman 0 siblings, 0 replies; 33+ messages in thread From: Simon Horman @ 2017-11-13 7:05 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On Fri, Nov 10, 2017 at 10:58:41PM +0100, Marek Vasut 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 Acked-by: Simon Horman <horms+renesas@verge.net.au> > --- > 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/host/pcie-rcar.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index 811e8194ef74..ab61829db389 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -43,6 +43,7 @@ > > /* Transfer control */ > #define PCIETCTLR 0x02000 > +#define DL_DOWN BIT(3) > #define CFINIT 1 > #define PCIETSTR 0x02004 > #define DATA_LINK_ACTIVE 1 > @@ -1107,6 +1108,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > pcie = pci_host_bridge_priv(bridge); > > pcie->dev = dev; > + platform_set_drvdata(pdev, pcie); > > INIT_LIST_HEAD(&pcie->resources); > > @@ -1167,10 +1169,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.11.0 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut ` (2 preceding siblings ...) 2017-11-10 21:58 ` [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq Marek Vasut @ 2017-11-10 21:58 ` Marek Vasut 2017-11-13 7:05 ` Simon Horman 2017-11-17 17:49 ` Lorenzo Pieralisi 2017-11-10 21:58 ` [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver Marek Vasut 4 siblings, 2 replies; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 UTC (permalink / raw) To: linux-pci Cc: Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc From: Phil Edworthy <phil.edworthy@renesas.com> Most PCIe host controllers support L0s and L1 power states via ASPM. The R-Car hardware only supports L0s, so when the system suspends and resumes we have to manually handle L1. When the system suspends, cards can put themselves into L1 and send a PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer access the card's config registers. The R-Car host controller will handle taking cards out of L1 as long as the host controller has also been transitioned to L1 link state. Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and transition the host to L1 immediately. However, this patch just ensures that we can talk to cards after they have gone into L1. When attempting a config access, it checks to see if the card has gone into L1, and if so, does the same for the host controller. This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis - Use GENMASK() - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ --- drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index ab61829db389..068bf9067ec1 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -92,6 +92,13 @@ #define MACCTLR 0x011058 #define SPEED_CHANGE BIT(24) #define SCRAMBLE_DISABLE BIT(27) +#define PMSR 0x01105c +#define L1FAEG BIT(31) +#define PM_ENTER_L1RX BIT(23) +#define PMSTATE GENMASK(18, 16) +#define PMSTATE_L1 GENMASK(17, 16) +#define PMCTLR 0x011060 +#define L1_INIT BIT(31) #define MACS2R 0x011078 #define MACCGSPSETR 0x011084 #define SPCNGRSN BIT(31) @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, unsigned int devfn, int where, u32 *data) { int dev, func, reg, index; + u32 val; dev = PCI_SLOT(devfn); func = PCI_FUNC(devfn); @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, if (pcie->root_bus_nr < 0) return PCIBIOS_DEVICE_NOT_FOUND; + /* + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, + * transition to L1 link state. The HW will handle coming out of L1. + */ + val = rcar_pci_read_reg(pcie, PMSR); + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); + + /* Wait until we are in L1 */ + while (!(val & L1FAEG)) + val = rcar_pci_read_reg(pcie, PMSR); + + /* Clear flags indicating link has transitioned to L1 */ + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); + } + /* Clear errors */ rcar_pci_write_reg(pcie, rcar_pci_read_reg(pcie, PCIEERRFR), PCIEERRFR); -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2017-11-10 21:58 ` [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling Marek Vasut @ 2017-11-13 7:05 ` Simon Horman 2017-11-17 17:49 ` Lorenzo Pieralisi 1 sibling, 0 replies; 33+ messages in thread From: Simon Horman @ 2017-11-13 7:05 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > Most PCIe host controllers support L0s and L1 power states via ASPM. > The R-Car hardware only supports L0s, so when the system suspends and > resumes we have to manually handle L1. > > When the system suspends, cards can put themselves into L1 and send a > PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > access the card's config registers. > > The R-Car host controller will handle taking cards out of L1 as long as > the host controller has also been transitioned to L1 link state. > > Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > transition the host to L1 immediately. However, this patch just ensures > that we can talk to cards after they have gone into L1. > When attempting a config access, it checks to see if the card has gone > into L1, and if so, does the same for the host controller. > > This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > Signed-off-by: Phil Edworthy <phil.edworthy@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 Acked-by: Simon Horman <horms+renesas@verge.net.au> > --- > V2: - Drop extra parenthesis > - Use GENMASK() > - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > --- > drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index ab61829db389..068bf9067ec1 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -92,6 +92,13 @@ > #define MACCTLR 0x011058 > #define SPEED_CHANGE BIT(24) > #define SCRAMBLE_DISABLE BIT(27) > +#define PMSR 0x01105c > +#define L1FAEG BIT(31) > +#define PM_ENTER_L1RX BIT(23) > +#define PMSTATE GENMASK(18, 16) > +#define PMSTATE_L1 GENMASK(17, 16) > +#define PMCTLR 0x011060 > +#define L1_INIT BIT(31) > #define MACS2R 0x011078 > #define MACCGSPSETR 0x011084 > #define SPCNGRSN BIT(31) > @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > unsigned int devfn, int where, u32 *data) > { > int dev, func, reg, index; > + u32 val; > > dev = PCI_SLOT(devfn); > func = PCI_FUNC(devfn); > @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > if (pcie->root_bus_nr < 0) > return PCIBIOS_DEVICE_NOT_FOUND; > > + /* > + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > + * transition to L1 link state. The HW will handle coming out of L1. > + */ > + val = rcar_pci_read_reg(pcie, PMSR); > + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > + > + /* Wait until we are in L1 */ > + while (!(val & L1FAEG)) > + val = rcar_pci_read_reg(pcie, PMSR); > + > + /* Clear flags indicating link has transitioned to L1 */ > + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > + } > + > /* Clear errors */ > rcar_pci_write_reg(pcie, rcar_pci_read_reg(pcie, PCIEERRFR), PCIEERRFR); > > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2017-11-10 21:58 ` [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling Marek Vasut 2017-11-13 7:05 ` Simon Horman @ 2017-11-17 17:49 ` Lorenzo Pieralisi 2018-06-10 13:57 ` Marek Vasut 1 sibling, 1 reply; 33+ messages in thread From: Lorenzo Pieralisi @ 2017-11-17 17:49 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc Hi Marek, On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > Most PCIe host controllers support L0s and L1 power states via ASPM. > The R-Car hardware only supports L0s, so when the system suspends and > resumes we have to manually handle L1. > When the system suspends, cards can put themselves into L1 and send a I assumed L1 entry has to be negotiated depending upon the PCIe hierarchy capabilities, I would appreciate if you can explain to me what's the root cause of the issue please. > PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > access the card's config registers. > > The R-Car host controller will handle taking cards out of L1 as long as > the host controller has also been transitioned to L1 link state. I wonder why this can't be done in a PM restore hook but that's not really where my question is. > Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > transition the host to L1 immediately. However, this patch just ensures > that we can talk to cards after they have gone into L1. > When attempting a config access, it checks to see if the card has gone > into L1, and if so, does the same for the host controller. > > This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > - Use GENMASK() > - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > --- > drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index ab61829db389..068bf9067ec1 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -92,6 +92,13 @@ > #define MACCTLR 0x011058 > #define SPEED_CHANGE BIT(24) > #define SCRAMBLE_DISABLE BIT(27) > +#define PMSR 0x01105c > +#define L1FAEG BIT(31) > +#define PM_ENTER_L1RX BIT(23) > +#define PMSTATE GENMASK(18, 16) > +#define PMSTATE_L1 GENMASK(17, 16) > +#define PMCTLR 0x011060 > +#define L1_INIT BIT(31) > #define MACS2R 0x011078 > #define MACCGSPSETR 0x011084 > #define SPCNGRSN BIT(31) > @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > unsigned int devfn, int where, u32 *data) > { > int dev, func, reg, index; > + u32 val; > > dev = PCI_SLOT(devfn); > func = PCI_FUNC(devfn); > @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > if (pcie->root_bus_nr < 0) > return PCIBIOS_DEVICE_NOT_FOUND; > > + /* > + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > + * transition to L1 link state. The HW will handle coming out of L1. > + */ > + val = rcar_pci_read_reg(pcie, PMSR); > + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > + > + /* Wait until we are in L1 */ > + while (!(val & L1FAEG)) > + val = rcar_pci_read_reg(pcie, PMSR); > + > + /* Clear flags indicating link has transitioned to L1 */ > + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > + } I do not get why you need to add the DLLP check for _every_ given config access and how/why it is just related to suspend/resume and not eg cold boot (I supposed it is because devices can enter L1 upon suspend(?)), I would ask you please to provide a thorough explanation so that I can actually review this patch (the commit log must be rewritten nonetheless, I do not think it is clear, at least it is not for me). Thanks, Lorenzo > + > /* Clear errors */ > rcar_pci_write_reg(pcie, rcar_pci_read_reg(pcie, PCIEERRFR), PCIEERRFR); > > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2017-11-17 17:49 ` Lorenzo Pieralisi @ 2018-06-10 13:57 ` Marek Vasut 2018-06-11 13:59 ` Bjorn Helgaas 0 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2018-06-10 13:57 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > Hi Marek, Hi, > On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: >> From: Phil Edworthy <phil.edworthy@renesas.com> >> >> Most PCIe host controllers support L0s and L1 power states via ASPM. >> The R-Car hardware only supports L0s, so when the system suspends and >> resumes we have to manually handle L1. >> When the system suspends, cards can put themselves into L1 and send a > > I assumed L1 entry has to be negotiated depending upon the PCIe > hierarchy capabilities, I would appreciate if you can explain to > me what's the root cause of the issue please. You should probably ignore the suspend/resume part altogether. The issue here is that the cards can enter L1 state, while the controller won't do that automatically, it can only detect that the link went into L1 state. If that happens,the driver must manually put the controller to L1 state. The controller can transition out of L1 state automatically though. >> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer >> access the card's config registers. >> >> The R-Car host controller will handle taking cards out of L1 as long as >> the host controller has also been transitioned to L1 link state. > > I wonder why this can't be done in a PM restore hook but that's not > really where my question is. I suspect because the link can be in L1 during startup too? >> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and >> transition the host to L1 immediately. However, this patch just ensures >> that we can talk to cards after they have gone into L1. > >> When attempting a config access, it checks to see if the card has gone >> into L1, and if so, does the same for the host controller. >> >> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> >> >> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis >> - Use GENMASK() >> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ >> --- >> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >> index ab61829db389..068bf9067ec1 100644 >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c >> @@ -92,6 +92,13 @@ >> #define MACCTLR 0x011058 >> #define SPEED_CHANGE BIT(24) >> #define SCRAMBLE_DISABLE BIT(27) >> +#define PMSR 0x01105c >> +#define L1FAEG BIT(31) >> +#define PM_ENTER_L1RX BIT(23) >> +#define PMSTATE GENMASK(18, 16) >> +#define PMSTATE_L1 GENMASK(17, 16) >> +#define PMCTLR 0x011060 >> +#define L1_INIT BIT(31) >> #define MACS2R 0x011078 >> #define MACCGSPSETR 0x011084 >> #define SPCNGRSN BIT(31) >> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, >> unsigned int devfn, int where, u32 *data) >> { >> int dev, func, reg, index; >> + u32 val; >> >> dev = PCI_SLOT(devfn); >> func = PCI_FUNC(devfn); >> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, >> if (pcie->root_bus_nr < 0) >> return PCIBIOS_DEVICE_NOT_FOUND; >> >> + /* >> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, >> + * transition to L1 link state. The HW will handle coming out of L1. >> + */ >> + val = rcar_pci_read_reg(pcie, PMSR); >> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { >> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); >> + >> + /* Wait until we are in L1 */ >> + while (!(val & L1FAEG)) >> + val = rcar_pci_read_reg(pcie, PMSR); >> + >> + /* Clear flags indicating link has transitioned to L1 */ >> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); >> + } > > I do not get why you need to add the DLLP check for _every_ given config > access and how/why it is just related to suspend/resume and not eg cold > boot (I supposed it is because devices can enter L1 upon suspend(?)), I > would ask you please to provide a thorough explanation so that I can > actually review this patch (the commit log must be rewritten nonetheless, > I do not think it is clear, at least it is not for me). See above -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-10 13:57 ` Marek Vasut @ 2018-06-11 13:59 ` Bjorn Helgaas 2018-06-12 23:54 ` Marek Vasut 0 siblings, 1 reply; 33+ messages in thread From: Bjorn Helgaas @ 2018-06-11 13:59 UTC (permalink / raw) To: Marek Vasut Cc: Lorenzo Pieralisi, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > > On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > >> From: Phil Edworthy <phil.edworthy@renesas.com> > >> > >> Most PCIe host controllers support L0s and L1 power states via ASPM. > >> The R-Car hardware only supports L0s, so when the system suspends and > >> resumes we have to manually handle L1. > >> When the system suspends, cards can put themselves into L1 and send a > > > > I assumed L1 entry has to be negotiated depending upon the PCIe > > hierarchy capabilities, I would appreciate if you can explain to > > me what's the root cause of the issue please. > > You should probably ignore the suspend/resume part altogether. The issue > here is that the cards can enter L1 state, while the controller won't do > that automatically, it can only detect that the link went into L1 state. > If that happens,the driver must manually put the controller to L1 state. > The controller can transition out of L1 state automatically though. >From earlier discussion I thought the R-Car root port did not advertise L1 support. If that's the case, we shouldn't enable L1 entry on a card. Is the core ASPM code doing something wrong here? > >> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > >> access the card's config registers. > >> > >> The R-Car host controller will handle taking cards out of L1 as long as > >> the host controller has also been transitioned to L1 link state. > > > > I wonder why this can't be done in a PM restore hook but that's not > > really where my question is. > > I suspect because the link can be in L1 during startup too? > > >> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > >> transition the host to L1 immediately. However, this patch just ensures > >> that we can talk to cards after they have gone into L1. > > > >> When attempting a config access, it checks to see if the card has gone > >> into L1, and if so, does the same for the host controller. > >> > >> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > >> > >> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > >> - Use GENMASK() > >> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > >> --- > >> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > >> index ab61829db389..068bf9067ec1 100644 > >> --- a/drivers/pci/host/pcie-rcar.c > >> +++ b/drivers/pci/host/pcie-rcar.c > >> @@ -92,6 +92,13 @@ > >> #define MACCTLR 0x011058 > >> #define SPEED_CHANGE BIT(24) > >> #define SCRAMBLE_DISABLE BIT(27) > >> +#define PMSR 0x01105c > >> +#define L1FAEG BIT(31) > >> +#define PM_ENTER_L1RX BIT(23) > >> +#define PMSTATE GENMASK(18, 16) > >> +#define PMSTATE_L1 GENMASK(17, 16) > >> +#define PMCTLR 0x011060 > >> +#define L1_INIT BIT(31) > >> #define MACS2R 0x011078 > >> #define MACCGSPSETR 0x011084 > >> #define SPCNGRSN BIT(31) > >> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > >> unsigned int devfn, int where, u32 *data) > >> { > >> int dev, func, reg, index; > >> + u32 val; > >> > >> dev = PCI_SLOT(devfn); > >> func = PCI_FUNC(devfn); > >> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > >> if (pcie->root_bus_nr < 0) > >> return PCIBIOS_DEVICE_NOT_FOUND; > >> > >> + /* > >> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > >> + * transition to L1 link state. The HW will handle coming out of L1. > >> + */ > >> + val = rcar_pci_read_reg(pcie, PMSR); > >> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > >> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > >> + > >> + /* Wait until we are in L1 */ > >> + while (!(val & L1FAEG)) > >> + val = rcar_pci_read_reg(pcie, PMSR); > >> + > >> + /* Clear flags indicating link has transitioned to L1 */ > >> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > >> + } > > > > I do not get why you need to add the DLLP check for _every_ given config > > access and how/why it is just related to suspend/resume and not eg cold > > boot (I supposed it is because devices can enter L1 upon suspend(?)), I > > would ask you please to provide a thorough explanation so that I can > > actually review this patch (the commit log must be rewritten nonetheless, > > I do not think it is clear, at least it is not for me). > > See above > > -- > Best regards, > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-11 13:59 ` Bjorn Helgaas @ 2018-06-12 23:54 ` Marek Vasut 2018-06-13 13:53 ` Bjorn Helgaas 0 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2018-06-12 23:54 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: >> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: >>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: >>>> From: Phil Edworthy <phil.edworthy@renesas.com> >>>> >>>> Most PCIe host controllers support L0s and L1 power states via ASPM. >>>> The R-Car hardware only supports L0s, so when the system suspends and >>>> resumes we have to manually handle L1. >>>> When the system suspends, cards can put themselves into L1 and send a >>> >>> I assumed L1 entry has to be negotiated depending upon the PCIe >>> hierarchy capabilities, I would appreciate if you can explain to >>> me what's the root cause of the issue please. >> >> You should probably ignore the suspend/resume part altogether. The issue >> here is that the cards can enter L1 state, while the controller won't do >> that automatically, it can only detect that the link went into L1 state. >> If that happens,the driver must manually put the controller to L1 state. >> The controller can transition out of L1 state automatically though. > > From earlier discussion I thought the R-Car root port did not > advertise L1 support. Which discussion ? This one or somewhere else ? > If that's the case, we shouldn't enable L1 > entry on a card. Is the core ASPM code doing something wrong here? I can double-check, am I looking for some particular register in the PCIe space ? >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer >>>> access the card's config registers. >>>> >>>> The R-Car host controller will handle taking cards out of L1 as long as >>>> the host controller has also been transitioned to L1 link state. >>> >>> I wonder why this can't be done in a PM restore hook but that's not >>> really where my question is. >> >> I suspect because the link can be in L1 during startup too? >> >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and >>>> transition the host to L1 immediately. However, this patch just ensures >>>> that we can talk to cards after they have gone into L1. >>> >>>> When attempting a config access, it checks to see if the card has gone >>>> into L1, and if so, does the same for the host controller. >>>> >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> >>>> >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis >>>> - Use GENMASK() >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ >>>> --- >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >>>> index ab61829db389..068bf9067ec1 100644 >>>> --- a/drivers/pci/host/pcie-rcar.c >>>> +++ b/drivers/pci/host/pcie-rcar.c >>>> @@ -92,6 +92,13 @@ >>>> #define MACCTLR 0x011058 >>>> #define SPEED_CHANGE BIT(24) >>>> #define SCRAMBLE_DISABLE BIT(27) >>>> +#define PMSR 0x01105c >>>> +#define L1FAEG BIT(31) >>>> +#define PM_ENTER_L1RX BIT(23) >>>> +#define PMSTATE GENMASK(18, 16) >>>> +#define PMSTATE_L1 GENMASK(17, 16) >>>> +#define PMCTLR 0x011060 >>>> +#define L1_INIT BIT(31) >>>> #define MACS2R 0x011078 >>>> #define MACCGSPSETR 0x011084 >>>> #define SPCNGRSN BIT(31) >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, >>>> unsigned int devfn, int where, u32 *data) >>>> { >>>> int dev, func, reg, index; >>>> + u32 val; >>>> >>>> dev = PCI_SLOT(devfn); >>>> func = PCI_FUNC(devfn); >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, >>>> if (pcie->root_bus_nr < 0) >>>> return PCIBIOS_DEVICE_NOT_FOUND; >>>> >>>> + /* >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, >>>> + * transition to L1 link state. The HW will handle coming out of L1. >>>> + */ >>>> + val = rcar_pci_read_reg(pcie, PMSR); >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); >>>> + >>>> + /* Wait until we are in L1 */ >>>> + while (!(val & L1FAEG)) >>>> + val = rcar_pci_read_reg(pcie, PMSR); >>>> + >>>> + /* Clear flags indicating link has transitioned to L1 */ >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); >>>> + } >>> >>> I do not get why you need to add the DLLP check for _every_ given config >>> access and how/why it is just related to suspend/resume and not eg cold >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I >>> would ask you please to provide a thorough explanation so that I can >>> actually review this patch (the commit log must be rewritten nonetheless, >>> I do not think it is clear, at least it is not for me). >> >> See above >> >> -- >> Best regards, >> Marek Vasut -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-12 23:54 ` Marek Vasut @ 2018-06-13 13:53 ` Bjorn Helgaas 2018-06-13 15:52 ` Lorenzo Pieralisi 0 siblings, 1 reply; 33+ messages in thread From: Bjorn Helgaas @ 2018-06-13 13:53 UTC (permalink / raw) To: Marek Vasut Cc: Lorenzo Pieralisi, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > > On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > >> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > >>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > >>>> From: Phil Edworthy <phil.edworthy@renesas.com> > >>>> > >>>> Most PCIe host controllers support L0s and L1 power states via ASPM. > >>>> The R-Car hardware only supports L0s, so when the system suspends and > >>>> resumes we have to manually handle L1. > >>>> When the system suspends, cards can put themselves into L1 and send a > >>> > >>> I assumed L1 entry has to be negotiated depending upon the PCIe > >>> hierarchy capabilities, I would appreciate if you can explain to > >>> me what's the root cause of the issue please. > >> > >> You should probably ignore the suspend/resume part altogether. The issue > >> here is that the cards can enter L1 state, while the controller won't do > >> that automatically, it can only detect that the link went into L1 state. > >> If that happens,the driver must manually put the controller to L1 state. > >> The controller can transition out of L1 state automatically though. > > > > From earlier discussion I thought the R-Car root port did not > > advertise L1 support. > > Which discussion ? This one or somewhere else ? https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com Re-reading that, I think I see my misunderstanding. I was only considering L1 in the ASPM context. I didn't realize the L1 implications of devices being in states other than D0. Obviously L1 support for ASPM is optional and advertised via Link Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for PCI-PM compatible power management, and is entered "whenever all Functions ... are programmed to a D-state other than D0." So I guess this means *every* device is supposed to support L1 when it is in a non-D0 power state. I think *this* is the case you're solving. A little more of this detail, e.g., that this issue has nothing to do with ASPM, it's probably an R-Car erratum that the RC can't transition from L1 to L0, etc., in the changelog would really help clear things up for me. > > If that's the case, we shouldn't enable L1 > > entry on a card. Is the core ASPM code doing something wrong here? > > I can double-check, am I looking for some particular register in the > PCIe space ? > > >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > >>>> access the card's config registers. > >>>> > >>>> The R-Car host controller will handle taking cards out of L1 as long as > >>>> the host controller has also been transitioned to L1 link state. > >>> > >>> I wonder why this can't be done in a PM restore hook but that's not > >>> really where my question is. > >> > >> I suspect because the link can be in L1 during startup too? > >> > >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > >>>> transition the host to L1 immediately. However, this patch just ensures > >>>> that we can talk to cards after they have gone into L1. > >>> > >>>> When attempting a config access, it checks to see if the card has gone > >>>> into L1, and if so, does the same for the host controller. > >>>> > >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > >>>> > >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > >>>> - Use GENMASK() > >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > >>>> --- > >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > >>>> 1 file changed, 24 insertions(+) > >>>> > >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > >>>> index ab61829db389..068bf9067ec1 100644 > >>>> --- a/drivers/pci/host/pcie-rcar.c > >>>> +++ b/drivers/pci/host/pcie-rcar.c > >>>> @@ -92,6 +92,13 @@ > >>>> #define MACCTLR 0x011058 > >>>> #define SPEED_CHANGE BIT(24) > >>>> #define SCRAMBLE_DISABLE BIT(27) > >>>> +#define PMSR 0x01105c > >>>> +#define L1FAEG BIT(31) > >>>> +#define PM_ENTER_L1RX BIT(23) > >>>> +#define PMSTATE GENMASK(18, 16) > >>>> +#define PMSTATE_L1 GENMASK(17, 16) > >>>> +#define PMCTLR 0x011060 > >>>> +#define L1_INIT BIT(31) > >>>> #define MACS2R 0x011078 > >>>> #define MACCGSPSETR 0x011084 > >>>> #define SPCNGRSN BIT(31) > >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > >>>> unsigned int devfn, int where, u32 *data) > >>>> { > >>>> int dev, func, reg, index; > >>>> + u32 val; > >>>> > >>>> dev = PCI_SLOT(devfn); > >>>> func = PCI_FUNC(devfn); > >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > >>>> if (pcie->root_bus_nr < 0) > >>>> return PCIBIOS_DEVICE_NOT_FOUND; > >>>> > >>>> + /* > >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > >>>> + * transition to L1 link state. The HW will handle coming out of L1. > >>>> + */ > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > >>>> + > >>>> + /* Wait until we are in L1 */ > >>>> + while (!(val & L1FAEG)) > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > >>>> + > >>>> + /* Clear flags indicating link has transitioned to L1 */ > >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > >>>> + } > >>> > >>> I do not get why you need to add the DLLP check for _every_ given config > >>> access and how/why it is just related to suspend/resume and not eg cold > >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I > >>> would ask you please to provide a thorough explanation so that I can > >>> actually review this patch (the commit log must be rewritten nonetheless, > >>> I do not think it is clear, at least it is not for me). > >> > >> See above > >> > >> -- > >> Best regards, > >> Marek Vasut > > > -- > Best regards, > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-13 13:53 ` Bjorn Helgaas @ 2018-06-13 15:52 ` Lorenzo Pieralisi 2018-06-13 17:25 ` Bjorn Helgaas 0 siblings, 1 reply; 33+ messages in thread From: Lorenzo Pieralisi @ 2018-06-13 15:52 UTC (permalink / raw) To: Bjorn Helgaas Cc: Marek Vasut, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: > On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > > On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > > > On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > > >> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > > >>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > > >>>> From: Phil Edworthy <phil.edworthy@renesas.com> > > >>>> > > >>>> Most PCIe host controllers support L0s and L1 power states via ASPM. > > >>>> The R-Car hardware only supports L0s, so when the system suspends and > > >>>> resumes we have to manually handle L1. > > >>>> When the system suspends, cards can put themselves into L1 and send a > > >>> > > >>> I assumed L1 entry has to be negotiated depending upon the PCIe > > >>> hierarchy capabilities, I would appreciate if you can explain to > > >>> me what's the root cause of the issue please. > > >> > > >> You should probably ignore the suspend/resume part altogether. The issue > > >> here is that the cards can enter L1 state, while the controller won't do > > >> that automatically, it can only detect that the link went into L1 state. > > >> If that happens,the driver must manually put the controller to L1 state. > > >> The controller can transition out of L1 state automatically though. > > > > > > From earlier discussion I thought the R-Car root port did not > > > advertise L1 support. > > > > Which discussion ? This one or somewhere else ? > > https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com > > Re-reading that, I think I see my misunderstanding. I was only > considering L1 in the ASPM context. I didn't realize the L1 > implications of devices being in states other than D0. > > Obviously L1 support for ASPM is optional and advertised via Link > Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for > PCI-PM compatible power management, and is entered "whenever all > Functions ... are programmed to a D-state other than D0." > > So I guess this means *every* device is supposed to support L1 when it > is in a non-D0 power state. I think *this* is the case you're > solving. > > A little more of this detail, e.g., that this issue has nothing to do > with ASPM, it's probably an R-Car erratum that the RC can't transition > from L1 to L0, etc., in the changelog would really help clear things > up for me. I think that the issue is related to the L0->L1 transition upon system suspend (ie the kernel must force the controller into L1 when all devices are in a sleep state) and for this specific reason I still think that checking for a PM_Enter_L1 DLLP reception and doing the L0->L1 transition within a config access is wrong and prone to error (what's the rationale behind that ?), this ought to be done using PM methods in the host controller driver. And yes, adding more details to the commit log would help everybody understand where the problem lies. Thanks, Lorenzo > > > If that's the case, we shouldn't enable L1 > > > entry on a card. Is the core ASPM code doing something wrong here? > > > > I can double-check, am I looking for some particular register in the > > PCIe space ? > > > > >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > > >>>> access the card's config registers. > > >>>> > > >>>> The R-Car host controller will handle taking cards out of L1 as long as > > >>>> the host controller has also been transitioned to L1 link state. > > >>> > > >>> I wonder why this can't be done in a PM restore hook but that's not > > >>> really where my question is. > > >> > > >> I suspect because the link can be in L1 during startup too? > > >> > > >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > > >>>> transition the host to L1 immediately. However, this patch just ensures > > >>>> that we can talk to cards after they have gone into L1. > > >>> > > >>>> When attempting a config access, it checks to see if the card has gone > > >>>> into L1, and if so, does the same for the host controller. > > >>>> > > >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > >>>> > > >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > > >>>> - Use GENMASK() > > >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > > >>>> --- > > >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > > >>>> 1 file changed, 24 insertions(+) > > >>>> > > >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > > >>>> index ab61829db389..068bf9067ec1 100644 > > >>>> --- a/drivers/pci/host/pcie-rcar.c > > >>>> +++ b/drivers/pci/host/pcie-rcar.c > > >>>> @@ -92,6 +92,13 @@ > > >>>> #define MACCTLR 0x011058 > > >>>> #define SPEED_CHANGE BIT(24) > > >>>> #define SCRAMBLE_DISABLE BIT(27) > > >>>> +#define PMSR 0x01105c > > >>>> +#define L1FAEG BIT(31) > > >>>> +#define PM_ENTER_L1RX BIT(23) > > >>>> +#define PMSTATE GENMASK(18, 16) > > >>>> +#define PMSTATE_L1 GENMASK(17, 16) > > >>>> +#define PMCTLR 0x011060 > > >>>> +#define L1_INIT BIT(31) > > >>>> #define MACS2R 0x011078 > > >>>> #define MACCGSPSETR 0x011084 > > >>>> #define SPCNGRSN BIT(31) > > >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > >>>> unsigned int devfn, int where, u32 *data) > > >>>> { > > >>>> int dev, func, reg, index; > > >>>> + u32 val; > > >>>> > > >>>> dev = PCI_SLOT(devfn); > > >>>> func = PCI_FUNC(devfn); > > >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > >>>> if (pcie->root_bus_nr < 0) > > >>>> return PCIBIOS_DEVICE_NOT_FOUND; > > >>>> > > >>>> + /* > > >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > > >>>> + * transition to L1 link state. The HW will handle coming out of L1. > > >>>> + */ > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > > >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > > >>>> + > > >>>> + /* Wait until we are in L1 */ > > >>>> + while (!(val & L1FAEG)) > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > >>>> + > > >>>> + /* Clear flags indicating link has transitioned to L1 */ > > >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > > >>>> + } > > >>> > > >>> I do not get why you need to add the DLLP check for _every_ given config > > >>> access and how/why it is just related to suspend/resume and not eg cold > > >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I > > >>> would ask you please to provide a thorough explanation so that I can > > >>> actually review this patch (the commit log must be rewritten nonetheless, > > >>> I do not think it is clear, at least it is not for me). > > >> > > >> See above > > >> > > >> -- > > >> Best regards, > > >> Marek Vasut > > > > > > -- > > Best regards, > > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-13 15:52 ` Lorenzo Pieralisi @ 2018-06-13 17:25 ` Bjorn Helgaas 2018-06-14 11:43 ` Lorenzo Pieralisi ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Bjorn Helgaas @ 2018-06-13 17:25 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: > On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: > > On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > > > On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > > > > On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > > > >> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > > > >>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > > > >>>> From: Phil Edworthy <phil.edworthy@renesas.com> > > > >>>> > > > >>>> Most PCIe host controllers support L0s and L1 power states via ASPM. > > > >>>> The R-Car hardware only supports L0s, so when the system suspends and > > > >>>> resumes we have to manually handle L1. > > > >>>> When the system suspends, cards can put themselves into L1 and send a > > > >>> > > > >>> I assumed L1 entry has to be negotiated depending upon the PCIe > > > >>> hierarchy capabilities, I would appreciate if you can explain to > > > >>> me what's the root cause of the issue please. > > > >> > > > >> You should probably ignore the suspend/resume part altogether. The issue > > > >> here is that the cards can enter L1 state, while the controller won't do > > > >> that automatically, it can only detect that the link went into L1 state. > > > >> If that happens,the driver must manually put the controller to L1 state. > > > >> The controller can transition out of L1 state automatically though. > > > > > > > > From earlier discussion I thought the R-Car root port did not > > > > advertise L1 support. > > > > > > Which discussion ? This one or somewhere else ? > > > > https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com > > > > Re-reading that, I think I see my misunderstanding. I was only > > considering L1 in the ASPM context. I didn't realize the L1 > > implications of devices being in states other than D0. > > > > Obviously L1 support for ASPM is optional and advertised via Link > > Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for > > PCI-PM compatible power management, and is entered "whenever all > > Functions ... are programmed to a D-state other than D0." > > > > So I guess this means *every* device is supposed to support L1 when it > > is in a non-D0 power state. I think *this* is the case you're > > solving. > > > > A little more of this detail, e.g., that this issue has nothing to do > > with ASPM, it's probably an R-Car erratum that the RC can't transition > > from L1 to L0, etc., in the changelog would really help clear things > > up for me. > > I think that the issue is related to the L0->L1 transition upon system > suspend (ie the kernel must force the controller into L1 when all > devices are in a sleep state) and for this specific reason I still think > that checking for a PM_Enter_L1 DLLP reception and doing the L0->L1 > transition within a config access is wrong and prone to error (what's > the rationale behind that ?), this ought to be done using PM methods in > the host controller driver. But doesn't the problem happen whenever the link goes to L1, for any reason? E.g., runtime power management might put an endpoint in D3 even if we're not doing a whole system suspend. A user could even force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If that's the case, I don't think the host controller PM methods will be enough to work around the issue. The comment in the patch ("If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, transition to L1 link state") suggests that the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 correctly, i.e., it doesn't complete the transition of the link to L1. Putting this workaround in the config accessor makes sense to me because in this situation the endpoint thinks it's in L1 and it won't receive TLPs for config accesses. Apparently forcing the RP to L1 completes the L1 entry, and the RP correctly handles the "Exit from L1 State" (sec 5.3.2.2) that's required when the RP needs to send a TLP to the endpoint. I think there's still a potential issue if the endpoint goes to a non-D0 state, the link is stuck in this transitional state (endpoint thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit L1, e.g., so it can send a PME message for a wakeup. I don't know what happens then. If there were a real erratum writeup for this, it would probably discuss this situation. > > > > If that's the case, we shouldn't enable L1 > > > > entry on a card. Is the core ASPM code doing something wrong here? > > > > > > I can double-check, am I looking for some particular register in the > > > PCIe space ? > > > > > > >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > > > >>>> access the card's config registers. > > > >>>> > > > >>>> The R-Car host controller will handle taking cards out of L1 as long as > > > >>>> the host controller has also been transitioned to L1 link state. > > > >>> > > > >>> I wonder why this can't be done in a PM restore hook but that's not > > > >>> really where my question is. > > > >> > > > >> I suspect because the link can be in L1 during startup too? > > > >> > > > >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > > > >>>> transition the host to L1 immediately. However, this patch just ensures > > > >>>> that we can talk to cards after they have gone into L1. > > > >>> > > > >>>> When attempting a config access, it checks to see if the card has gone > > > >>>> into L1, and if so, does the same for the host controller. > > > >>>> > > > >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > > >>>> > > > >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > > > >>>> - Use GENMASK() > > > >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > > > >>>> --- > > > >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > > > >>>> 1 file changed, 24 insertions(+) > > > >>>> > > > >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > > > >>>> index ab61829db389..068bf9067ec1 100644 > > > >>>> --- a/drivers/pci/host/pcie-rcar.c > > > >>>> +++ b/drivers/pci/host/pcie-rcar.c > > > >>>> @@ -92,6 +92,13 @@ > > > >>>> #define MACCTLR 0x011058 > > > >>>> #define SPEED_CHANGE BIT(24) > > > >>>> #define SCRAMBLE_DISABLE BIT(27) > > > >>>> +#define PMSR 0x01105c > > > >>>> +#define L1FAEG BIT(31) > > > >>>> +#define PM_ENTER_L1RX BIT(23) > > > >>>> +#define PMSTATE GENMASK(18, 16) > > > >>>> +#define PMSTATE_L1 GENMASK(17, 16) > > > >>>> +#define PMCTLR 0x011060 > > > >>>> +#define L1_INIT BIT(31) > > > >>>> #define MACS2R 0x011078 > > > >>>> #define MACCGSPSETR 0x011084 > > > >>>> #define SPCNGRSN BIT(31) > > > >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > >>>> unsigned int devfn, int where, u32 *data) > > > >>>> { > > > >>>> int dev, func, reg, index; > > > >>>> + u32 val; > > > >>>> > > > >>>> dev = PCI_SLOT(devfn); > > > >>>> func = PCI_FUNC(devfn); > > > >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > >>>> if (pcie->root_bus_nr < 0) > > > >>>> return PCIBIOS_DEVICE_NOT_FOUND; > > > >>>> > > > >>>> + /* > > > >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > > > >>>> + * transition to L1 link state. The HW will handle coming out of L1. > > > >>>> + */ > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > > > >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > > > >>>> + > > > >>>> + /* Wait until we are in L1 */ > > > >>>> + while (!(val & L1FAEG)) > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > >>>> + > > > >>>> + /* Clear flags indicating link has transitioned to L1 */ > > > >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > > > >>>> + } > > > >>> > > > >>> I do not get why you need to add the DLLP check for _every_ given config > > > >>> access and how/why it is just related to suspend/resume and not eg cold > > > >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I > > > >>> would ask you please to provide a thorough explanation so that I can > > > >>> actually review this patch (the commit log must be rewritten nonetheless, > > > >>> I do not think it is clear, at least it is not for me). > > > >> > > > >> See above > > > >> > > > >> -- > > > >> Best regards, > > > >> Marek Vasut > > > > > > > > > -- > > > Best regards, > > > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-13 17:25 ` Bjorn Helgaas @ 2018-06-14 11:43 ` Lorenzo Pieralisi 2018-07-25 21:08 ` Marek Vasut 2018-08-14 16:25 ` Lorenzo Pieralisi 2 siblings, 0 replies; 33+ messages in thread From: Lorenzo Pieralisi @ 2018-06-14 11:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: Marek Vasut, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Wed, Jun 13, 2018 at 12:25:59PM -0500, Bjorn Helgaas wrote: > On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: > > > On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > > > > On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > > > > > On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > > > > >> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > > > > >>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > > > > >>>> From: Phil Edworthy <phil.edworthy@renesas.com> > > > > >>>> > > > > >>>> Most PCIe host controllers support L0s and L1 power states via ASPM. > > > > >>>> The R-Car hardware only supports L0s, so when the system suspends and > > > > >>>> resumes we have to manually handle L1. > > > > >>>> When the system suspends, cards can put themselves into L1 and send a > > > > >>> > > > > >>> I assumed L1 entry has to be negotiated depending upon the PCIe > > > > >>> hierarchy capabilities, I would appreciate if you can explain to > > > > >>> me what's the root cause of the issue please. > > > > >> > > > > >> You should probably ignore the suspend/resume part altogether. The issue > > > > >> here is that the cards can enter L1 state, while the controller won't do > > > > >> that automatically, it can only detect that the link went into L1 state. > > > > >> If that happens,the driver must manually put the controller to L1 state. > > > > >> The controller can transition out of L1 state automatically though. > > > > > > > > > > From earlier discussion I thought the R-Car root port did not > > > > > advertise L1 support. > > > > > > > > Which discussion ? This one or somewhere else ? > > > > > > https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com > > > > > > Re-reading that, I think I see my misunderstanding. I was only > > > considering L1 in the ASPM context. I didn't realize the L1 > > > implications of devices being in states other than D0. > > > > > > Obviously L1 support for ASPM is optional and advertised via Link > > > Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for > > > PCI-PM compatible power management, and is entered "whenever all > > > Functions ... are programmed to a D-state other than D0." > > > > > > So I guess this means *every* device is supposed to support L1 when it > > > is in a non-D0 power state. I think *this* is the case you're > > > solving. > > > > > > A little more of this detail, e.g., that this issue has nothing to do > > > with ASPM, it's probably an R-Car erratum that the RC can't transition > > > from L1 to L0, etc., in the changelog would really help clear things > > > up for me. > > > > I think that the issue is related to the L0->L1 transition upon system > > suspend (ie the kernel must force the controller into L1 when all > > devices are in a sleep state) and for this specific reason I still think > > that checking for a PM_Enter_L1 DLLP reception and doing the L0->L1 > > transition within a config access is wrong and prone to error (what's > > the rationale behind that ?), this ought to be done using PM methods in > > the host controller driver. > > But doesn't the problem happen whenever the link goes to L1, for any > reason? E.g., runtime power management might put an endpoint in D3 > even if we're not doing a whole system suspend. A user could even > force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If > that's the case, I don't think the host controller PM methods will be > enough to work around the issue. By PM methods I included runtime PM (and related device dependencies) but you are right there, I missed some use cases (which are not necessarily the most common but we have to cope with them anyway). > The comment in the patch ("If we are not in L1 link state and we have > received PM_ENTER_L1 DLLP, transition to L1 link state") suggests that > the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 > correctly, i.e., it doesn't complete the transition of the link to L1. > > Putting this workaround in the config accessor makes sense to me > because in this situation the endpoint thinks it's in L1 and it won't > receive TLPs for config accesses. Apparently forcing the RP to L1 > completes the L1 entry, and the RP correctly handles the "Exit from L1 > State" (sec 5.3.2.2) that's required when the RP needs to send a TLP > to the endpoint. Yep, see above, I do not like it but I do not see how we can solve it in another way either. > I think there's still a potential issue if the endpoint goes to a > non-D0 state, the link is stuck in this transitional state (endpoint > thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > L1, e.g., so it can send a PME message for a wakeup. I don't know > what happens then. That's for Marek to explain and the explanation has to go along with this discussion in the resulting commit log. Lorenzo > If there were a real erratum writeup for this, it would probably > discuss this situation. > > > > > > If that's the case, we shouldn't enable L1 > > > > > entry on a card. Is the core ASPM code doing something wrong here? > > > > > > > > I can double-check, am I looking for some particular register in the > > > > PCIe space ? > > > > > > > > >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > > > > >>>> access the card's config registers. > > > > >>>> > > > > >>>> The R-Car host controller will handle taking cards out of L1 as long as > > > > >>>> the host controller has also been transitioned to L1 link state. > > > > >>> > > > > >>> I wonder why this can't be done in a PM restore hook but that's not > > > > >>> really where my question is. > > > > >> > > > > >> I suspect because the link can be in L1 during startup too? > > > > >> > > > > >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > > > > >>>> transition the host to L1 immediately. However, this patch just ensures > > > > >>>> that we can talk to cards after they have gone into L1. > > > > >>> > > > > >>>> When attempting a config access, it checks to see if the card has gone > > > > >>>> into L1, and if so, does the same for the host controller. > > > > >>>> > > > > >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > > > >>>> > > > > >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > > > > >>>> - Use GENMASK() > > > > >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > > > > >>>> --- > > > > >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > > > > >>>> 1 file changed, 24 insertions(+) > > > > >>>> > > > > >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > > > > >>>> index ab61829db389..068bf9067ec1 100644 > > > > >>>> --- a/drivers/pci/host/pcie-rcar.c > > > > >>>> +++ b/drivers/pci/host/pcie-rcar.c > > > > >>>> @@ -92,6 +92,13 @@ > > > > >>>> #define MACCTLR 0x011058 > > > > >>>> #define SPEED_CHANGE BIT(24) > > > > >>>> #define SCRAMBLE_DISABLE BIT(27) > > > > >>>> +#define PMSR 0x01105c > > > > >>>> +#define L1FAEG BIT(31) > > > > >>>> +#define PM_ENTER_L1RX BIT(23) > > > > >>>> +#define PMSTATE GENMASK(18, 16) > > > > >>>> +#define PMSTATE_L1 GENMASK(17, 16) > > > > >>>> +#define PMCTLR 0x011060 > > > > >>>> +#define L1_INIT BIT(31) > > > > >>>> #define MACS2R 0x011078 > > > > >>>> #define MACCGSPSETR 0x011084 > > > > >>>> #define SPCNGRSN BIT(31) > > > > >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > > >>>> unsigned int devfn, int where, u32 *data) > > > > >>>> { > > > > >>>> int dev, func, reg, index; > > > > >>>> + u32 val; > > > > >>>> > > > > >>>> dev = PCI_SLOT(devfn); > > > > >>>> func = PCI_FUNC(devfn); > > > > >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > > >>>> if (pcie->root_bus_nr < 0) > > > > >>>> return PCIBIOS_DEVICE_NOT_FOUND; > > > > >>>> > > > > >>>> + /* > > > > >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > > > > >>>> + * transition to L1 link state. The HW will handle coming out of L1. > > > > >>>> + */ > > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > > >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > > > > >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > > > > >>>> + > > > > >>>> + /* Wait until we are in L1 */ > > > > >>>> + while (!(val & L1FAEG)) > > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > > >>>> + > > > > >>>> + /* Clear flags indicating link has transitioned to L1 */ > > > > >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > > > > >>>> + } > > > > >>> > > > > >>> I do not get why you need to add the DLLP check for _every_ given config > > > > >>> access and how/why it is just related to suspend/resume and not eg cold > > > > >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I > > > > >>> would ask you please to provide a thorough explanation so that I can > > > > >>> actually review this patch (the commit log must be rewritten nonetheless, > > > > >>> I do not think it is clear, at least it is not for me). > > > > >> > > > > >> See above > > > > >> > > > > >> -- > > > > >> Best regards, > > > > >> Marek Vasut > > > > > > > > > > > > -- > > > > Best regards, > > > > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-13 17:25 ` Bjorn Helgaas 2018-06-14 11:43 ` Lorenzo Pieralisi @ 2018-07-25 21:08 ` Marek Vasut 2018-08-08 13:29 ` Marek Vasut 2018-08-14 16:25 ` Lorenzo Pieralisi 2 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2018-07-25 21:08 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On 06/13/2018 07:25 PM, Bjorn Helgaas wrote: > On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: >> On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: >>> On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: >>>> On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: >>>>> On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: >>>>>> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: >>>>>>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: >>>>>>>> From: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>>> >>>>>>>> Most PCIe host controllers support L0s and L1 power states via ASPM. >>>>>>>> The R-Car hardware only supports L0s, so when the system suspends and >>>>>>>> resumes we have to manually handle L1. >>>>>>>> When the system suspends, cards can put themselves into L1 and send a >>>>>>> >>>>>>> I assumed L1 entry has to be negotiated depending upon the PCIe >>>>>>> hierarchy capabilities, I would appreciate if you can explain to >>>>>>> me what's the root cause of the issue please. >>>>>> >>>>>> You should probably ignore the suspend/resume part altogether. The issue >>>>>> here is that the cards can enter L1 state, while the controller won't do >>>>>> that automatically, it can only detect that the link went into L1 state. >>>>>> If that happens,the driver must manually put the controller to L1 state. >>>>>> The controller can transition out of L1 state automatically though. >>>>> >>>>> From earlier discussion I thought the R-Car root port did not >>>>> advertise L1 support. >>>> >>>> Which discussion ? This one or somewhere else ? >>> >>> https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com >>> >>> Re-reading that, I think I see my misunderstanding. I was only >>> considering L1 in the ASPM context. I didn't realize the L1 >>> implications of devices being in states other than D0. >>> >>> Obviously L1 support for ASPM is optional and advertised via Link >>> Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for >>> PCI-PM compatible power management, and is entered "whenever all >>> Functions ... are programmed to a D-state other than D0." >>> >>> So I guess this means *every* device is supposed to support L1 when it >>> is in a non-D0 power state. I think *this* is the case you're >>> solving. >>> >>> A little more of this detail, e.g., that this issue has nothing to do >>> with ASPM, it's probably an R-Car erratum that the RC can't transition >>> from L1 to L0, etc., in the changelog would really help clear things >>> up for me. >> >> I think that the issue is related to the L0->L1 transition upon system >> suspend (ie the kernel must force the controller into L1 when all >> devices are in a sleep state) and for this specific reason I still think >> that checking for a PM_Enter_L1 DLLP reception and doing the L0->L1 >> transition within a config access is wrong and prone to error (what's >> the rationale behind that ?), this ought to be done using PM methods in >> the host controller driver. > > But doesn't the problem happen whenever the link goes to L1, for any > reason? E.g., runtime power management might put an endpoint in D3 > even if we're not doing a whole system suspend. A user could even > force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If > that's the case, I don't think the host controller PM methods will be > enough to work around the issue. I think so, it's the link that goes into L1 state and this can happen without any action from the controller side. > The comment in the patch ("If we are not in L1 link state and we have > received PM_ENTER_L1 DLLP, transition to L1 link state") suggests that > the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 > correctly, i.e., it doesn't complete the transition of the link to L1. > > Putting this workaround in the config accessor makes sense to me > because in this situation the endpoint thinks it's in L1 and it won't > receive TLPs for config accesses. Apparently forcing the RP to L1 > completes the L1 entry, and the RP correctly handles the "Exit from L1 > State" (sec 5.3.2.2) that's required when the RP needs to send a TLP > to the endpoint. > > I think there's still a potential issue if the endpoint goes to a > non-D0 state, the link is stuck in this transitional state (endpoint > thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > L1, e.g., so it can send a PME message for a wakeup. I don't know > what happens then. Is there some hardware which I can use to simulate this situation ? > If there were a real erratum writeup for this, it would probably > discuss this situation. I went through the latest errata sheet and don't see anything. The datasheet only mentions that L0/L0s/L1 is supported and L2 is not supported. Maybe Phil can comment on this too ? [...] -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-07-25 21:08 ` Marek Vasut @ 2018-08-08 13:29 ` Marek Vasut 2018-08-20 13:44 ` Phil Edworthy 0 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2018-08-08 13:29 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On 07/25/2018 11:08 PM, Marek Vasut wrote: > On 06/13/2018 07:25 PM, Bjorn Helgaas wrote: >> On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: >>> On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: >>>> On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: >>>>> On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: >>>>>> On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: >>>>>>> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: >>>>>>>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: >>>>>>>>> From: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>>>> >>>>>>>>> Most PCIe host controllers support L0s and L1 power states via ASPM. >>>>>>>>> The R-Car hardware only supports L0s, so when the system suspends and >>>>>>>>> resumes we have to manually handle L1. >>>>>>>>> When the system suspends, cards can put themselves into L1 and send a >>>>>>>> >>>>>>>> I assumed L1 entry has to be negotiated depending upon the PCIe >>>>>>>> hierarchy capabilities, I would appreciate if you can explain to >>>>>>>> me what's the root cause of the issue please. >>>>>>> >>>>>>> You should probably ignore the suspend/resume part altogether. The issue >>>>>>> here is that the cards can enter L1 state, while the controller won't do >>>>>>> that automatically, it can only detect that the link went into L1 state. >>>>>>> If that happens,the driver must manually put the controller to L1 state. >>>>>>> The controller can transition out of L1 state automatically though. >>>>>> >>>>>> From earlier discussion I thought the R-Car root port did not >>>>>> advertise L1 support. >>>>> >>>>> Which discussion ? This one or somewhere else ? >>>> >>>> https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0@HK2PR0601MB1393.apcprd06.prod.outlook.com >>>> >>>> Re-reading that, I think I see my misunderstanding. I was only >>>> considering L1 in the ASPM context. I didn't realize the L1 >>>> implications of devices being in states other than D0. >>>> >>>> Obviously L1 support for ASPM is optional and advertised via Link >>>> Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required for >>>> PCI-PM compatible power management, and is entered "whenever all >>>> Functions ... are programmed to a D-state other than D0." >>>> >>>> So I guess this means *every* device is supposed to support L1 when it >>>> is in a non-D0 power state. I think *this* is the case you're >>>> solving. >>>> >>>> A little more of this detail, e.g., that this issue has nothing to do >>>> with ASPM, it's probably an R-Car erratum that the RC can't transition >>>> from L1 to L0, etc., in the changelog would really help clear things >>>> up for me. >>> >>> I think that the issue is related to the L0->L1 transition upon system >>> suspend (ie the kernel must force the controller into L1 when all >>> devices are in a sleep state) and for this specific reason I still think >>> that checking for a PM_Enter_L1 DLLP reception and doing the L0->L1 >>> transition within a config access is wrong and prone to error (what's >>> the rationale behind that ?), this ought to be done using PM methods in >>> the host controller driver. >> >> But doesn't the problem happen whenever the link goes to L1, for any >> reason? E.g., runtime power management might put an endpoint in D3 >> even if we're not doing a whole system suspend. A user could even >> force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If >> that's the case, I don't think the host controller PM methods will be >> enough to work around the issue. > > I think so, it's the link that goes into L1 state and this can happen > without any action from the controller side. > >> The comment in the patch ("If we are not in L1 link state and we have >> received PM_ENTER_L1 DLLP, transition to L1 link state") suggests that >> the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 >> correctly, i.e., it doesn't complete the transition of the link to L1. >> >> Putting this workaround in the config accessor makes sense to me >> because in this situation the endpoint thinks it's in L1 and it won't >> receive TLPs for config accesses. Apparently forcing the RP to L1 >> completes the L1 entry, and the RP correctly handles the "Exit from L1 >> State" (sec 5.3.2.2) that's required when the RP needs to send a TLP >> to the endpoint. >> >> I think there's still a potential issue if the endpoint goes to a >> non-D0 state, the link is stuck in this transitional state (endpoint >> thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit >> L1, e.g., so it can send a PME message for a wakeup. I don't know >> what happens then. > > Is there some hardware which I can use to simulate this situation ? > >> If there were a real erratum writeup for this, it would probably >> discuss this situation. > > I went through the latest errata sheet and don't see anything. The > datasheet only mentions that L0/L0s/L1 is supported and L2 is not supported. > > Maybe Phil can comment on this too ? Bump ? > [...] > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-08-08 13:29 ` Marek Vasut @ 2018-08-20 13:44 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-20 13:44 UTC (permalink / raw) To: Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc Hello, Sorry for the delay. On 08 August 2018 14:30, Marek Vasut wrote: > On 07/25/2018 11:08 PM, Marek Vasut wrote: > > On 06/13/2018 07:25 PM, Bjorn Helgaas wrote: > >> On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: > >>> On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: > >>>> On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > >>>>> On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > >>>>>> On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > >>>>>>> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > >>>>>>>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > >>>>>>>>> From: Phil Edworthy <phil.edworthy@renesas.com> > >>>>>>>>> > >>>>>>>>> Most PCIe host controllers support L0s and L1 power states via > ASPM. > >>>>>>>>> The R-Car hardware only supports L0s, so when the system > >>>>>>>>> suspends and resumes we have to manually handle L1. > >>>>>>>>> When the system suspends, cards can put themselves into L1 > and > >>>>>>>>> send a > >>>>>>>> > >>>>>>>> I assumed L1 entry has to be negotiated depending upon the PCIe > >>>>>>>> hierarchy capabilities, I would appreciate if you can explain > >>>>>>>> to me what's the root cause of the issue please. > >>>>>>> > >>>>>>> You should probably ignore the suspend/resume part altogether. > >>>>>>> The issue here is that the cards can enter L1 state, while the > >>>>>>> controller won't do that automatically, it can only detect that the > link went into L1 state. > >>>>>>> If that happens,the driver must manually put the controller to L1 > state. > >>>>>>> The controller can transition out of L1 state automatically though. > >>>>>> > >>>>>> From earlier discussion I thought the R-Car root port did not > >>>>>> advertise L1 support. > >>>>> > >>>>> Which discussion ? This one or somewhere else ? > >>>> > >>>> > https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0 > @ > >>>> HK2PR0601MB1393.apcprd06.prod.outlook.com > >>>> > >>>> Re-reading that, I think I see my misunderstanding. I was only > >>>> considering L1 in the ASPM context. I didn't realize the L1 > >>>> implications of devices being in states other than D0. > >>>> > >>>> Obviously L1 support for ASPM is optional and advertised via Link > >>>> Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required > >>>> for PCI-PM compatible power management, and is entered "whenever > >>>> all Functions ... are programmed to a D-state other than D0." > >>>> > >>>> So I guess this means *every* device is supposed to support L1 when > >>>> it is in a non-D0 power state. I think *this* is the case you're > >>>> solving. > >>>> > >>>> A little more of this detail, e.g., that this issue has nothing to > >>>> do with ASPM, it's probably an R-Car erratum that the RC can't > >>>> transition from L1 to L0, etc., in the changelog would really help > >>>> clear things up for me. > >>> > >>> I think that the issue is related to the L0->L1 transition upon > >>> system suspend (ie the kernel must force the controller into L1 when > >>> all devices are in a sleep state) and for this specific reason I > >>> still think that checking for a PM_Enter_L1 DLLP reception and doing > >>> the L0->L1 transition within a config access is wrong and prone to > >>> error (what's the rationale behind that ?), this ought to be done > >>> using PM methods in the host controller driver. > >> > >> But doesn't the problem happen whenever the link goes to L1, for any > >> reason? E.g., runtime power management might put an endpoint in D3 > >> even if we're not doing a whole system suspend. A user could even > >> force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If > >> that's the case, I don't think the host controller PM methods will be > >> enough to work around the issue. > > > > I think so, it's the link that goes into L1 state and this can happen > > without any action from the controller side. > > > >> The comment in the patch ("If we are not in L1 link state and we have > >> received PM_ENTER_L1 DLLP, transition to L1 link state") suggests > >> that the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 > >> correctly, i.e., it doesn't complete the transition of the link to L1. > >> > >> Putting this workaround in the config accessor makes sense to me > >> because in this situation the endpoint thinks it's in L1 and it won't > >> receive TLPs for config accesses. Apparently forcing the RP to L1 > >> completes the L1 entry, and the RP correctly handles the "Exit from > >> L1 State" (sec 5.3.2.2) that's required when the RP needs to send a > >> TLP to the endpoint. That my understanding. > >> I think there's still a potential issue if the endpoint goes to a > >> non-D0 state, the link is stuck in this transitional state (endpoint > >> thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > >> L1, e.g., so it can send a PME message for a wakeup. I don't know > >> what happens then. > > > > Is there some hardware which I can use to simulate this situation ? > > > >> If there were a real erratum writeup for this, it would probably > >> discuss this situation. > > > > I went through the latest errata sheet and don't see anything. The > > datasheet only mentions that L0/L0s/L1 is supported and L2 is not > supported. > > > Maybe Phil can comment on this too ? There is no errata for this that I know of. The rcar RP supports L0/L0s/L1 in hardware, but only supports L0s via ASPM, i.e. you need software to poke some registers to make the RP transition to L1. However, both before and after this patch, the RP does not transition L1 when the endpoints change to L1. This patch only transitions the RP to L1 during accessing a card's config registers, if the RP is not in L1 link state and has received PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle the transition out of L1. The relevant part of the rcar manual says: "After a recovery to L0, if the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted from the downstream device, software should confirm that hardware is in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence is: L0 → L1 → L0 recovery → L1 again." I don’t think the potential issue that Bjorn talked about can happen because the RP does go into L1. I could be wrong though... The driver should also have a runtime-PM hook to transition to L1 on suspend in order to save power. However, that is somewhat separate to the problem the patch fixes. Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling @ 2018-08-20 13:44 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-20 13:44 UTC (permalink / raw) To: Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc SGVsbG8sDQoNClNvcnJ5IGZvciB0aGUgZGVsYXkuDQoNCk9uIDA4IEF1Z3VzdCAyMDE4IDE0OjMw LCBNYXJlayBWYXN1dCB3cm90ZToNCj4gT24gMDcvMjUvMjAxOCAxMTowOCBQTSwgTWFyZWsgVmFz dXQgd3JvdGU6DQo+ID4gT24gMDYvMTMvMjAxOCAwNzoyNSBQTSwgQmpvcm4gSGVsZ2FhcyB3cm90 ZToNCj4gPj4gT24gV2VkLCBKdW4gMTMsIDIwMTggYXQgMDQ6NTI6NTJQTSArMDEwMCwgTG9yZW56 byBQaWVyYWxpc2kgd3JvdGU6DQo+ID4+PiBPbiBXZWQsIEp1biAxMywgMjAxOCBhdCAwODo1Mzow OEFNIC0wNTAwLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0KPiA+Pj4+IE9uIFdlZCwgSnVuIDEzLCAy MDE4IGF0IDAxOjU0OjUxQU0gKzAyMDAsIE1hcmVrIFZhc3V0IHdyb3RlOg0KPiA+Pj4+PiBPbiAw Ni8xMS8yMDE4IDAzOjU5IFBNLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0KPiA+Pj4+Pj4gT24gU3Vu LCBKdW4gMTAsIDIwMTggYXQgMDM6NTc6MTBQTSArMDIwMCwgTWFyZWsgVmFzdXQgd3JvdGU6DQo+ ID4+Pj4+Pj4gT24gMTEvMTcvMjAxNyAwNjo0OSBQTSwgTG9yZW56byBQaWVyYWxpc2kgd3JvdGU6 DQo+ID4+Pj4+Pj4+IE9uIEZyaSwgTm92IDEwLCAyMDE3IGF0IDEwOjU4OjQyUE0gKzAxMDAsIE1h cmVrIFZhc3V0IHdyb3RlOg0KPiA+Pj4+Pj4+Pj4gRnJvbTogUGhpbCBFZHdvcnRoeSA8cGhpbC5l ZHdvcnRoeUByZW5lc2FzLmNvbT4NCj4gPj4+Pj4+Pj4+DQo+ID4+Pj4+Pj4+PiBNb3N0IFBDSWUg aG9zdCBjb250cm9sbGVycyBzdXBwb3J0IEwwcyBhbmQgTDEgcG93ZXIgc3RhdGVzIHZpYQ0KPiBB U1BNLg0KPiA+Pj4+Pj4+Pj4gVGhlIFItQ2FyIGhhcmR3YXJlIG9ubHkgc3VwcG9ydHMgTDBzLCBz byB3aGVuIHRoZSBzeXN0ZW0NCj4gPj4+Pj4+Pj4+IHN1c3BlbmRzIGFuZCByZXN1bWVzIHdlIGhh dmUgdG8gbWFudWFsbHkgaGFuZGxlIEwxLg0KPiA+Pj4+Pj4+Pj4gV2hlbiB0aGUgc3lzdGVtIHN1 c3BlbmRzLCBjYXJkcyBjYW4gcHV0IHRoZW1zZWx2ZXMgaW50byBMMQ0KPiBhbmQNCj4gPj4+Pj4+ Pj4+IHNlbmQgYQ0KPiA+Pj4+Pj4+Pg0KPiA+Pj4+Pj4+PiBJIGFzc3VtZWQgTDEgZW50cnkgaGFz IHRvIGJlIG5lZ290aWF0ZWQgZGVwZW5kaW5nIHVwb24gdGhlIFBDSWUNCj4gPj4+Pj4+Pj4gaGll cmFyY2h5IGNhcGFiaWxpdGllcywgSSB3b3VsZCBhcHByZWNpYXRlIGlmIHlvdSBjYW4gZXhwbGFp bg0KPiA+Pj4+Pj4+PiB0byBtZSB3aGF0J3MgdGhlIHJvb3QgY2F1c2Ugb2YgdGhlIGlzc3VlIHBs ZWFzZS4NCj4gPj4+Pj4+Pg0KPiA+Pj4+Pj4+IFlvdSBzaG91bGQgcHJvYmFibHkgaWdub3JlIHRo ZSBzdXNwZW5kL3Jlc3VtZSBwYXJ0IGFsdG9nZXRoZXIuDQo+ID4+Pj4+Pj4gVGhlIGlzc3VlIGhl cmUgaXMgdGhhdCB0aGUgY2FyZHMgY2FuIGVudGVyIEwxIHN0YXRlLCB3aGlsZSB0aGUNCj4gPj4+ Pj4+PiBjb250cm9sbGVyIHdvbid0IGRvIHRoYXQgYXV0b21hdGljYWxseSwgaXQgY2FuIG9ubHkg ZGV0ZWN0IHRoYXQgdGhlDQo+IGxpbmsgd2VudCBpbnRvIEwxIHN0YXRlLg0KPiA+Pj4+Pj4+IElm IHRoYXQgaGFwcGVucyx0aGUgZHJpdmVyIG11c3QgbWFudWFsbHkgcHV0IHRoZSBjb250cm9sbGVy IHRvIEwxDQo+IHN0YXRlLg0KPiA+Pj4+Pj4+IFRoZSBjb250cm9sbGVyIGNhbiB0cmFuc2l0aW9u IG91dCBvZiBMMSBzdGF0ZSBhdXRvbWF0aWNhbGx5IHRob3VnaC4NCj4gPj4+Pj4+DQo+ID4+Pj4+ PiBGcm9tIGVhcmxpZXIgZGlzY3Vzc2lvbiBJIHRob3VnaHQgdGhlIFItQ2FyIHJvb3QgcG9ydCBk aWQgbm90DQo+ID4+Pj4+PiBhZHZlcnRpc2UgTDEgc3VwcG9ydC4NCj4gPj4+Pj4NCj4gPj4+Pj4g V2hpY2ggZGlzY3Vzc2lvbiA/IFRoaXMgb25lIG9yIHNvbWV3aGVyZSBlbHNlID8NCj4gPj4+Pg0K PiA+Pj4+DQo+IGh0dHBzOi8vbGttbC5rZXJuZWwub3JnL3IvSEsyUFIwNjAxTUIxMzkzRDkxN0Qz NDNFNjM2MzQ4NENBNjhGNUNCMA0KPiBADQo+ID4+Pj4gSEsyUFIwNjAxTUIxMzkzLmFwY3ByZDA2 LnByb2Qub3V0bG9vay5jb20NCj4gPj4+Pg0KPiA+Pj4+IFJlLXJlYWRpbmcgdGhhdCwgSSB0aGlu ayBJIHNlZSBteSBtaXN1bmRlcnN0YW5kaW5nLiAgSSB3YXMgb25seQ0KPiA+Pj4+IGNvbnNpZGVy aW5nIEwxIGluIHRoZSBBU1BNIGNvbnRleHQuICBJIGRpZG4ndCByZWFsaXplIHRoZSBMMQ0KPiA+ Pj4+IGltcGxpY2F0aW9ucyBvZiBkZXZpY2VzIGJlaW5nIGluIHN0YXRlcyBvdGhlciB0aGFuIEQw Lg0KPiA+Pj4+DQo+ID4+Pj4gT2J2aW91c2x5IEwxIHN1cHBvcnQgZm9yIEFTUE0gaXMgb3B0aW9u YWwgYW5kIGFkdmVydGlzZWQgdmlhIExpbmsNCj4gPj4+PiBDYXBhYmlsaXRpZXMuICBCdXQgcGVy IFBDSWUgcjQuMCwgc2VjIDUuMiwgTDEgc3VwcG9ydCBpcyByZXF1aXJlZA0KPiA+Pj4+IGZvciBQ Q0ktUE0gY29tcGF0aWJsZSBwb3dlciBtYW5hZ2VtZW50LCBhbmQgaXMgZW50ZXJlZCAid2hlbmV2 ZXINCj4gPj4+PiBhbGwgRnVuY3Rpb25zIC4uLiBhcmUgcHJvZ3JhbW1lZCB0byBhIEQtc3RhdGUg b3RoZXIgdGhhbiBEMC4iDQo+ID4+Pj4NCj4gPj4+PiBTbyBJIGd1ZXNzIHRoaXMgbWVhbnMgKmV2 ZXJ5KiBkZXZpY2UgaXMgc3VwcG9zZWQgdG8gc3VwcG9ydCBMMSB3aGVuDQo+ID4+Pj4gaXQgaXMg aW4gYSBub24tRDAgcG93ZXIgc3RhdGUuICBJIHRoaW5rICp0aGlzKiBpcyB0aGUgY2FzZSB5b3Un cmUNCj4gPj4+PiBzb2x2aW5nLg0KPiA+Pj4+DQo+ID4+Pj4gQSBsaXR0bGUgbW9yZSBvZiB0aGlz IGRldGFpbCwgZS5nLiwgdGhhdCB0aGlzIGlzc3VlIGhhcyBub3RoaW5nIHRvDQo+ID4+Pj4gZG8g d2l0aCBBU1BNLCBpdCdzIHByb2JhYmx5IGFuIFItQ2FyIGVycmF0dW0gdGhhdCB0aGUgUkMgY2Fu J3QNCj4gPj4+PiB0cmFuc2l0aW9uIGZyb20gTDEgdG8gTDAsIGV0Yy4sIGluIHRoZSBjaGFuZ2Vs b2cgd291bGQgcmVhbGx5IGhlbHANCj4gPj4+PiBjbGVhciB0aGluZ3MgdXAgZm9yIG1lLg0KPiA+ Pj4NCj4gPj4+IEkgdGhpbmsgdGhhdCB0aGUgaXNzdWUgaXMgcmVsYXRlZCB0byB0aGUgTDAtPkwx IHRyYW5zaXRpb24gdXBvbg0KPiA+Pj4gc3lzdGVtIHN1c3BlbmQgKGllIHRoZSBrZXJuZWwgbXVz dCBmb3JjZSB0aGUgY29udHJvbGxlciBpbnRvIEwxIHdoZW4NCj4gPj4+IGFsbCBkZXZpY2VzIGFy ZSBpbiBhIHNsZWVwIHN0YXRlKSBhbmQgZm9yIHRoaXMgc3BlY2lmaWMgcmVhc29uIEkNCj4gPj4+ IHN0aWxsIHRoaW5rIHRoYXQgY2hlY2tpbmcgZm9yIGEgUE1fRW50ZXJfTDEgRExMUCByZWNlcHRp b24gYW5kIGRvaW5nDQo+ID4+PiB0aGUgTDAtPkwxIHRyYW5zaXRpb24gd2l0aGluIGEgY29uZmln IGFjY2VzcyBpcyB3cm9uZyBhbmQgcHJvbmUgdG8NCj4gPj4+IGVycm9yICh3aGF0J3MgdGhlIHJh dGlvbmFsZSBiZWhpbmQgdGhhdCA/KSwgdGhpcyBvdWdodCB0byBiZSBkb25lDQo+ID4+PiB1c2lu ZyBQTSBtZXRob2RzIGluIHRoZSBob3N0IGNvbnRyb2xsZXIgZHJpdmVyLg0KPiA+Pg0KPiA+PiBC dXQgZG9lc24ndCB0aGUgcHJvYmxlbSBoYXBwZW4gd2hlbmV2ZXIgdGhlIGxpbmsgZ29lcyB0byBM MSwgZm9yIGFueQ0KPiA+PiByZWFzb24/ICBFLmcuLCBydW50aW1lIHBvd2VyIG1hbmFnZW1lbnQg bWlnaHQgcHV0IGFuIGVuZHBvaW50IGluIEQzDQo+ID4+IGV2ZW4gaWYgd2UncmUgbm90IGRvaW5n IGEgd2hvbGUgc3lzdGVtIHN1c3BlbmQuICBBIHVzZXIgY291bGQgZXZlbg0KPiA+PiBmb3JjZSB0 aGUgZW5kcG9pbnQgdG8gRDMgYnkgd3JpdGluZyB0byBQQ0lfUE1fQ1RSTCB3aXRoICJzZXRwY2ki LiAgSWYNCj4gPj4gdGhhdCdzIHRoZSBjYXNlLCBJIGRvbid0IHRoaW5rIHRoZSBob3N0IGNvbnRy b2xsZXIgUE0gbWV0aG9kcyB3aWxsIGJlDQo+ID4+IGVub3VnaCB0byB3b3JrIGFyb3VuZCB0aGUg aXNzdWUuDQo+ID4NCj4gPiBJIHRoaW5rIHNvLCBpdCdzIHRoZSBsaW5rIHRoYXQgZ29lcyBpbnRv IEwxIHN0YXRlIGFuZCB0aGlzIGNhbiBoYXBwZW4NCj4gPiB3aXRob3V0IGFueSBhY3Rpb24gZnJv bSB0aGUgY29udHJvbGxlciBzaWRlLg0KPiA+DQo+ID4+IFRoZSBjb21tZW50IGluIHRoZSBwYXRj aCAoIklmIHdlIGFyZSBub3QgaW4gTDEgbGluayBzdGF0ZSBhbmQgd2UgaGF2ZQ0KPiA+PiByZWNl aXZlZCBQTV9FTlRFUl9MMSBETExQLCB0cmFuc2l0aW9uIHRvIEwxIGxpbmsgc3RhdGUiKSBzdWdn ZXN0cw0KPiA+PiB0aGF0IHRoZSBSLUNhciBob3N0IGRvZXNuJ3QgaGFuZGxlIHN0ZXAgMTAgaW4g UENJZSByNC4wLCBzZWMgNS4zLjIuMQ0KPiA+PiBjb3JyZWN0bHksIGkuZS4sIGl0IGRvZXNuJ3Qg Y29tcGxldGUgdGhlIHRyYW5zaXRpb24gb2YgdGhlIGxpbmsgdG8gTDEuDQo+ID4+DQo+ID4+IFB1 dHRpbmcgdGhpcyB3b3JrYXJvdW5kIGluIHRoZSBjb25maWcgYWNjZXNzb3IgbWFrZXMgc2Vuc2Ug dG8gbWUNCj4gPj4gYmVjYXVzZSBpbiB0aGlzIHNpdHVhdGlvbiB0aGUgZW5kcG9pbnQgdGhpbmtz IGl0J3MgaW4gTDEgYW5kIGl0IHdvbid0DQo+ID4+IHJlY2VpdmUgVExQcyBmb3IgY29uZmlnIGFj Y2Vzc2VzLiAgQXBwYXJlbnRseSBmb3JjaW5nIHRoZSBSUCB0byBMMQ0KPiA+PiBjb21wbGV0ZXMg dGhlIEwxIGVudHJ5LCBhbmQgdGhlIFJQIGNvcnJlY3RseSBoYW5kbGVzIHRoZSAiRXhpdCBmcm9t DQo+ID4+IEwxIFN0YXRlIiAoc2VjIDUuMy4yLjIpIHRoYXQncyByZXF1aXJlZCB3aGVuIHRoZSBS UCBuZWVkcyB0byBzZW5kIGENCj4gPj4gVExQIHRvIHRoZSBlbmRwb2ludC4NClRoYXQgbXkgdW5k ZXJzdGFuZGluZy4NCg0KPiA+PiBJIHRoaW5rIHRoZXJlJ3Mgc3RpbGwgYSBwb3RlbnRpYWwgaXNz dWUgaWYgdGhlIGVuZHBvaW50IGdvZXMgdG8gYQ0KPiA+PiBub24tRDAgc3RhdGUsIHRoZSBsaW5r IGlzIHN0dWNrIGluIHRoaXMgdHJhbnNpdGlvbmFsIHN0YXRlIChlbmRwb2ludA0KPiA+PiB0aGlu a3MgaXQncyBMMSwgUlAgdGhpbmtzIGl0J3MgTDApLCBhbmQgdGhlICplbmRwb2ludCogd2FudHMg dG8gZXhpdA0KPiA+PiBMMSwgZS5nLiwgc28gaXQgY2FuIHNlbmQgYSBQTUUgbWVzc2FnZSBmb3Ig YSB3YWtldXAuICBJIGRvbid0IGtub3cNCj4gPj4gd2hhdCBoYXBwZW5zIHRoZW4uDQo+ID4NCj4g PiBJcyB0aGVyZSBzb21lIGhhcmR3YXJlIHdoaWNoIEkgY2FuIHVzZSB0byBzaW11bGF0ZSB0aGlz IHNpdHVhdGlvbiA/DQo+ID4NCj4gPj4gSWYgdGhlcmUgd2VyZSBhIHJlYWwgZXJyYXR1bSB3cml0 ZXVwIGZvciB0aGlzLCBpdCB3b3VsZCBwcm9iYWJseQ0KPiA+PiBkaXNjdXNzIHRoaXMgc2l0dWF0 aW9uLg0KPiA+DQo+ID4gSSB3ZW50IHRocm91Z2ggdGhlIGxhdGVzdCBlcnJhdGEgc2hlZXQgYW5k IGRvbid0IHNlZSBhbnl0aGluZy4gVGhlDQo+ID4gZGF0YXNoZWV0IG9ubHkgbWVudGlvbnMgdGhh dCBMMC9MMHMvTDEgaXMgc3VwcG9ydGVkIGFuZCBMMiBpcyBub3QNCj4gc3VwcG9ydGVkLg0KPg0K PiA+IE1heWJlIFBoaWwgY2FuIGNvbW1lbnQgb24gdGhpcyB0b28gPw0KVGhlcmUgaXMgbm8gZXJy YXRhIGZvciB0aGlzIHRoYXQgSSBrbm93IG9mLg0KDQpUaGUgcmNhciBSUCBzdXBwb3J0cyBMMC9M MHMvTDEgaW4gaGFyZHdhcmUsIGJ1dCBvbmx5IHN1cHBvcnRzIEwwcyB2aWEgQVNQTSwNCmkuZS4g eW91IG5lZWQgc29mdHdhcmUgdG8gcG9rZSBzb21lIHJlZ2lzdGVycyB0byBtYWtlIHRoZSBSUCB0 cmFuc2l0aW9uIHRvIEwxLg0KDQpIb3dldmVyLCBib3RoIGJlZm9yZSBhbmQgYWZ0ZXIgdGhpcyBw YXRjaCwgdGhlIFJQIGRvZXMgbm90IHRyYW5zaXRpb24gTDENCndoZW4gdGhlIGVuZHBvaW50cyBj aGFuZ2UgdG8gTDEuDQpUaGlzIHBhdGNoIG9ubHkgdHJhbnNpdGlvbnMgdGhlIFJQIHRvIEwxIGR1 cmluZyBhY2Nlc3NpbmcgYSBjYXJkJ3MgY29uZmlnDQpyZWdpc3RlcnMsIGlmIHRoZSBSUCBpcyBu b3QgaW4gTDEgbGluayBzdGF0ZSBhbmQgaGFzIHJlY2VpdmVkIFBNX0VOVEVSX0wxIERMTFANCihl LmcuIHJlc3VtZSkuIEFmdGVyIHRoaXMsIHRoZSBoYXJkd2FyZSB3aWxsIGhhbmRsZSB0aGUgdHJh bnNpdGlvbiBvdXQgb2YgTDEuDQoNClRoZSByZWxldmFudCBwYXJ0IG9mIHRoZSByY2FyIG1hbnVh bCBzYXlzOg0KIkFmdGVyIGEgcmVjb3ZlcnkgdG8gTDAsIGlmIHRoZSBkZXZpY2UgaXMgaW4gdGhl IE5vbi1EMCBzdGF0ZSBhbmQgUE1fRW50ZXJfTDEgRExMUCBpcyB0cmFuc21pdHRlZCBmcm9tIHRo ZSBkb3duc3RyZWFtIGRldmljZSwgc29mdHdhcmUgc2hvdWxkIGNvbmZpcm0gdGhhdCBoYXJkd2Fy ZSBpcyBpbiB0aGUgTDAgc3RhdGUgKFBNU1IuUE1TVEFURSA9IEwwKSBhbmQgaW5pdGlhdGUgdGhl IEwxIHRyYW5zaXRpb24gc2VxdWVuY2UgYWdhaW4gKHdyaXRlIDEgdG8gUE1DVExSLkwxSUFUTiku IEluIHRoaXMgY2FzZSwgdGhlIHNlcXVlbmNlIGlzOiBMMCDihpIgTDEg4oaSIEwwIHJlY292ZXJ5 IOKGkiBMMSBhZ2Fpbi4iDQoNCkkgZG9u4oCZdCB0aGluayB0aGUgcG90ZW50aWFsIGlzc3VlIHRo YXQgQmpvcm4gdGFsa2VkIGFib3V0IGNhbiBoYXBwZW4NCmJlY2F1c2UgdGhlIFJQIGRvZXMgZ28g aW50byBMMS4gSSBjb3VsZCBiZSB3cm9uZyB0aG91Z2guLi4NCg0KVGhlIGRyaXZlciBzaG91bGQg YWxzbyBoYXZlIGEgcnVudGltZS1QTSBob29rIHRvIHRyYW5zaXRpb24gdG8gTDEgb24gDQpzdXNw ZW5kIGluIG9yZGVyIHRvIHNhdmUgcG93ZXIuIEhvd2V2ZXIsIHRoYXQgaXMgc29tZXdoYXQgc2Vw YXJhdGUNCnRvIHRoZSBwcm9ibGVtIHRoZSBwYXRjaCBmaXhlcy4NCg0KUGhpbA0K ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-08-20 13:44 ` Phil Edworthy (?) @ 2018-08-20 14:47 ` Lorenzo Pieralisi 2018-08-21 8:58 ` Phil Edworthy -1 siblings, 1 reply; 33+ messages in thread From: Lorenzo Pieralisi @ 2018-08-20 14:47 UTC (permalink / raw) To: Phil Edworthy Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote: [...] > However, both before and after this patch, the RP does not transition L1 > when the endpoints change to L1. > This patch only transitions the RP to L1 during accessing a card's > config registers, if the RP is not in L1 link state and has received > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle > the transition out of L1. > > The relevant part of the rcar manual says: "After a recovery to L0, if > the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted > from the downstream device, software should confirm that hardware is > in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition > sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence > is: L0 → L1 → L0 recovery → L1 again." Can you map these FSM steps to this patch code please ? I would like to understand what Link state maps to which command written and when. > I don’t think the potential issue that Bjorn talked about can happen > because the RP does go into L1. I could be wrong though... I do not understand this paragraph, mind elaborating on it ? > The driver should also have a runtime-PM hook to transition to L1 on > suspend in order to save power. However, that is somewhat separate > to the problem the patch fixes. Yes that's a separate patch. Thanks for chiming in, let's try to get to the bottom of this thread. Lorenzo ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-08-20 14:47 ` Lorenzo Pieralisi @ 2018-08-21 8:58 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-21 8:58 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc Hi Lorenzo, On 20 August 2018 15:48 Lorenzo Pieralisi wrote: > On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote: > > [...] > > > However, both before and after this patch, the RP does not transition > > L1 when the endpoints change to L1. > > This patch only transitions the RP to L1 during accessing a card's > > config registers, if the RP is not in L1 link state and has received > > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle > > the transition out of L1. > > > > The relevant part of the rcar manual says: "After a recovery to L0, if > > the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted > > from the downstream device, software should confirm that hardware is > > in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition > > sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence > > is: L0 → L1 → L0 recovery → L1 again." > > Can you map these FSM steps to this patch code please ? I would like to > understand what Link state maps to which command written and when. I don’t think I can because we are not initially entering L1. Looking at this again, I think this section of the manual only helps in indicating how to detect we should have gone into L1 and how to poke the HW to initiate the transition to L1. On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state. The rcar RP cannot enter L1 by HW alone, so is still in L0. The only way out of this from the PCIe spec FSM is for both EP and RP to enter the Recovery state. The patch simply detects that we should have gone into L1, and so initiates that state change, and the HW will then handle the transition from L1 to Recovery and then on to L0. > > I don’t think the potential issue that Bjorn talked about can happen > > because the RP does go into L1. I could be wrong though... > > I do not understand this paragraph, mind elaborating on it ? As rcar RP only supports D0 and D3hot/cold, (the manual says it supports D3cold, but I cannot see how if it doesn’t support L2 or L3 states), if you force the link to D3, we can only be in L1 state. > > The driver should also have a runtime-PM hook to transition to L1 on > > suspend in order to save power. However, that is somewhat separate to > > the problem the patch fixes. > > Yes that's a separate patch. > > Thanks for chiming in, let's try to get to the bottom of this thread. > > Lorenzo Thanks Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling @ 2018-08-21 8:58 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-21 8:58 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc SGkgTG9yZW56bywNCg0KT24gMjAgQXVndXN0IDIwMTggMTU6NDggTG9yZW56byBQaWVyYWxpc2kg d3JvdGU6DQo+IE9uIE1vbiwgQXVnIDIwLCAyMDE4IGF0IDAxOjQ0OjQ4UE0gKzAwMDAsIFBoaWwg RWR3b3J0aHkgd3JvdGU6DQo+IA0KPiBbLi4uXQ0KPiANCj4gPiBIb3dldmVyLCBib3RoIGJlZm9y ZSBhbmQgYWZ0ZXIgdGhpcyBwYXRjaCwgdGhlIFJQIGRvZXMgbm90IHRyYW5zaXRpb24NCj4gPiBM MSB3aGVuIHRoZSBlbmRwb2ludHMgY2hhbmdlIHRvIEwxLg0KPiA+IFRoaXMgcGF0Y2ggb25seSB0 cmFuc2l0aW9ucyB0aGUgUlAgdG8gTDEgZHVyaW5nIGFjY2Vzc2luZyBhIGNhcmQncw0KPiA+IGNv bmZpZyByZWdpc3RlcnMsIGlmIHRoZSBSUCBpcyBub3QgaW4gTDEgbGluayBzdGF0ZSBhbmQgaGFz IHJlY2VpdmVkDQo+ID4gUE1fRU5URVJfTDEgRExMUCAoZS5nLiByZXN1bWUpLiBBZnRlciB0aGlz LCB0aGUgaGFyZHdhcmUgd2lsbCBoYW5kbGUNCj4gPiB0aGUgdHJhbnNpdGlvbiBvdXQgb2YgTDEu DQo+ID4NCj4gPiBUaGUgcmVsZXZhbnQgcGFydCBvZiB0aGUgcmNhciBtYW51YWwgc2F5czogIkFm dGVyIGEgcmVjb3ZlcnkgdG8gTDAsIGlmDQo+ID4gdGhlIGRldmljZSBpcyBpbiB0aGUgTm9uLUQw IHN0YXRlIGFuZCBQTV9FbnRlcl9MMSBETExQIGlzIHRyYW5zbWl0dGVkDQo+ID4gZnJvbSB0aGUg ZG93bnN0cmVhbSBkZXZpY2UsIHNvZnR3YXJlIHNob3VsZCBjb25maXJtIHRoYXQgaGFyZHdhcmUg aXMNCj4gPiBpbiB0aGUgTDAgc3RhdGUgKFBNU1IuUE1TVEFURSA9IEwwKSBhbmQgaW5pdGlhdGUg dGhlIEwxIHRyYW5zaXRpb24NCj4gPiBzZXF1ZW5jZSBhZ2FpbiAod3JpdGUgMSB0byBQTUNUTFIu TDFJQVROKS4gSW4gdGhpcyBjYXNlLCB0aGUgc2VxdWVuY2UNCj4gPiBpczogTDAg4oaSIEwxIOKG kiBMMCByZWNvdmVyeSDihpIgTDEgYWdhaW4uIg0KPiANCj4gQ2FuIHlvdSBtYXAgdGhlc2UgRlNN IHN0ZXBzIHRvIHRoaXMgcGF0Y2ggY29kZSBwbGVhc2UgPyBJIHdvdWxkIGxpa2UgdG8NCj4gdW5k ZXJzdGFuZCB3aGF0IExpbmsgc3RhdGUgbWFwcyB0byB3aGljaCBjb21tYW5kIHdyaXR0ZW4gYW5k IHdoZW4uDQpJIGRvbuKAmXQgdGhpbmsgSSBjYW4gYmVjYXVzZSB3ZSBhcmUgbm90IGluaXRpYWxs eSBlbnRlcmluZyBMMS4gTG9va2luZyBhdCB0aGlzDQphZ2FpbiwgSSB0aGluayB0aGlzIHNlY3Rp b24gb2YgdGhlIG1hbnVhbCBvbmx5IGhlbHBzIGluIGluZGljYXRpbmcgaG93IHRvIA0KZGV0ZWN0 IHdlIHNob3VsZCBoYXZlIGdvbmUgaW50byBMMSBhbmQgaG93IHRvIHBva2UgdGhlIEhXIHRvIGlu aXRpYXRlIHRoZQ0KdHJhbnNpdGlvbiB0byBMMS4NCg0KT24gc3lzdGVtIHN1c3BlbmQsIHRoZSBF UCBzZW5kcyBQTV9FbnRlcl9MMSBETExQIGFuZCBlbnRlcnMgTDEgc3RhdGUuDQpUaGUgcmNhciBS UCBjYW5ub3QgZW50ZXIgTDEgYnkgSFcgYWxvbmUsIHNvIGlzIHN0aWxsIGluIEwwLiBUaGUgb25s eSB3YXkgb3V0DQpvZiB0aGlzIGZyb20gdGhlIFBDSWUgc3BlYyBGU00gaXMgZm9yIGJvdGggRVAg YW5kIFJQIHRvIGVudGVyIHRoZSBSZWNvdmVyeQ0Kc3RhdGUuDQpUaGUgcGF0Y2ggc2ltcGx5IGRl dGVjdHMgdGhhdCB3ZSBzaG91bGQgaGF2ZSBnb25lIGludG8gTDEsIGFuZCBzbyBpbml0aWF0ZXMN CnRoYXQgc3RhdGUgY2hhbmdlLCBhbmQgdGhlIEhXIHdpbGwgdGhlbiBoYW5kbGUgdGhlIHRyYW5z aXRpb24gZnJvbSBMMSB0bw0KUmVjb3ZlcnkgYW5kIHRoZW4gb24gdG8gTDAuDQoNCg0KPiA+IEkg ZG9u4oCZdCB0aGluayB0aGUgcG90ZW50aWFsIGlzc3VlIHRoYXQgQmpvcm4gdGFsa2VkIGFib3V0 IGNhbiBoYXBwZW4NCj4gPiBiZWNhdXNlIHRoZSBSUCBkb2VzIGdvIGludG8gTDEuIEkgY291bGQg YmUgd3JvbmcgdGhvdWdoLi4uDQo+IA0KPiBJIGRvIG5vdCB1bmRlcnN0YW5kIHRoaXMgcGFyYWdy YXBoLCBtaW5kIGVsYWJvcmF0aW5nIG9uIGl0ID8NCkFzIHJjYXIgUlAgb25seSBzdXBwb3J0cyBE MCBhbmQgRDNob3QvY29sZCwgKHRoZSBtYW51YWwgc2F5cyBpdCBzdXBwb3J0cw0KRDNjb2xkLCBi dXQgSSBjYW5ub3Qgc2VlIGhvdyBpZiBpdCBkb2VzbuKAmXQgc3VwcG9ydCBMMiBvciBMMyBzdGF0 ZXMpLCBpZiB5b3UNCmZvcmNlIHRoZSBsaW5rIHRvIEQzLCB3ZSBjYW4gb25seSBiZSBpbiBMMSBz dGF0ZS4NCg0KDQo+ID4gVGhlIGRyaXZlciBzaG91bGQgYWxzbyBoYXZlIGEgcnVudGltZS1QTSBo b29rIHRvIHRyYW5zaXRpb24gdG8gTDEgb24NCj4gPiBzdXNwZW5kIGluIG9yZGVyIHRvIHNhdmUg cG93ZXIuIEhvd2V2ZXIsIHRoYXQgaXMgc29tZXdoYXQgc2VwYXJhdGUgdG8NCj4gPiB0aGUgcHJv YmxlbSB0aGUgcGF0Y2ggZml4ZXMuDQo+IA0KPiBZZXMgdGhhdCdzIGEgc2VwYXJhdGUgcGF0Y2gu DQo+IA0KPiBUaGFua3MgZm9yIGNoaW1pbmcgaW4sIGxldCdzIHRyeSB0byBnZXQgdG8gdGhlIGJv dHRvbSBvZiB0aGlzIHRocmVhZC4NCj4gDQo+IExvcmVuem8NCg0KVGhhbmtzDQpQaGlsDQo= ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-08-21 8:58 ` Phil Edworthy (?) @ 2018-08-21 15:32 ` Lorenzo Pieralisi 2018-08-22 9:20 ` Phil Edworthy -1 siblings, 1 reply; 33+ messages in thread From: Lorenzo Pieralisi @ 2018-08-21 15:32 UTC (permalink / raw) To: Phil Edworthy Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Tue, Aug 21, 2018 at 08:58:38AM +0000, Phil Edworthy wrote: > Hi Lorenzo, > > On 20 August 2018 15:48 Lorenzo Pieralisi wrote: > > On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote: > > > > [...] > > > > > However, both before and after this patch, the RP does not transition > > > L1 when the endpoints change to L1. > > > This patch only transitions the RP to L1 during accessing a card's > > > config registers, if the RP is not in L1 link state and has received > > > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle > > > the transition out of L1. > > > > > > The relevant part of the rcar manual says: "After a recovery to L0, if > > > the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted > > > from the downstream device, software should confirm that hardware is > > > in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition > > > sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence > > > is: L0 ??? L1 ??? L0 recovery ??? L1 again." > > > > Can you map these FSM steps to this patch code please ? I would like to > > understand what Link state maps to which command written and when. > I don't think I can because we are not initially entering L1. Looking at this > again, I think this section of the manual only helps in indicating how to > detect we should have gone into L1 and how to poke the HW to initiate the > transition to L1. > > On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state. I am still struggling to understand what "EP enters L1 state" means. A link in L1 means both ends of the link are in electrical idle, it is a two-way handshake, see PCI express specifications 5.3.2.1 "Entry into the L1 state". > The rcar RP cannot enter L1 by HW alone, so is still in L0. See above. > The only way out of this from the PCIe spec FSM is for both EP and RP > to enter the Recovery state. > The patch simply detects that we should have gone into L1, and so initiates > that state change, and the HW will then handle the transition from L1 to > Recovery and then on to L0. That I can understand, I reckon it is to "reset" the RP link state machine to a "sane" state. > > > I don't think the potential issue that Bjorn talked about can happen > > > because the RP does go into L1. I could be wrong though... > > > > I do not understand this paragraph, mind elaborating on it ? > As rcar RP only supports D0 and D3hot/cold, (the manual says it supports > D3cold, but I cannot see how if it doesn't support L2 or L3 states), if you > force the link to D3, we can only be in L1 state. D3 is a device state, not a link state. I still do not understand this statement. The link between RP and EP can enter L1 when all functions in the EP are in a device state != D0 but, as I mentioned above, it is still unclear what happens in this platform since I do not get what state in the PCI spec 5.3.2.1 state machine the RP Link state machine is in. If we programme the device into any D-state and the device wants to send a PME message _before_ we reset the RP state machine with the procedure described in this thread, what happens ? Or, more explicitly, what are in _HW_ the states of upstream and downstream link state machines when the EP is put in, say, D1 ? That's in short our question. I would be happy to get to the bottom of this since it is an interesting issue we are facing, we need HW details, I can apply Marek's patch but I would be happier if I get the whole picture first. Thanks, Lorenzo > > > > > The driver should also have a runtime-PM hook to transition to L1 on > > > suspend in order to save power. However, that is somewhat separate to > > > the problem the patch fixes. > > > > Yes that's a separate patch. > > > > Thanks for chiming in, let's try to get to the bottom of this thread. > > > > Lorenzo > > Thanks > Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-08-21 15:32 ` Lorenzo Pieralisi @ 2018-08-22 9:20 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-22 9:20 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc Hi Lorenzo, On 21 August 2018 16:32, Lorenzo Pieralisi wrote: > On Tue, Aug 21, 2018 at 08:58:38AM +0000, Phil Edworthy wrote: > > On 20 August 2018 15:48 Lorenzo Pieralisi wrote: > > > On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote: > > > > > > [...] > > > > > > > However, both before and after this patch, the RP does not > > > > transition > > > > L1 when the endpoints change to L1. > > > > This patch only transitions the RP to L1 during accessing a card's > > > > config registers, if the RP is not in L1 link state and has > > > > received > > > > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will > > > > handle the transition out of L1. > > > > > > > > The relevant part of the rcar manual says: "After a recovery to > > > > L0, if the device is in the Non-D0 state and PM_Enter_L1 DLLP is > > > > transmitted from the downstream device, software should confirm > > > > that hardware is in the L0 state (PMSR.PMSTATE = L0) and initiate > > > > the L1 transition sequence again (write 1 to PMCTLR.L1IATN). In > > > > this case, the sequence > > > > is: L0 ??? L1 ??? L0 recovery ??? L1 again." > > > > > > Can you map these FSM steps to this patch code please ? I would like > > > to understand what Link state maps to which command written and > when. > > I don't think I can because we are not initially entering L1. Looking > > at this again, I think this section of the manual only helps in > > indicating how to detect we should have gone into L1 and how to poke > > the HW to initiate the transition to L1. > > > > On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state. > > I am still struggling to understand what "EP enters L1 state" means. A link in > L1 means both ends of the link are in electrical idle, it is a two-way > handshake, see PCI express specifications 5.3.2.1 "Entry into the L1 state". Sorry, I'm no PCIe expert and the rcar HW documentation doesn't provide enough detail. I guess (that's all I can do with this) the following: a) the EP sends the PM_Enter_L1 DLLP, b) the RP sends a PM_Request_Ack DLLP. c) The EP physical layer is then inactive. However, the rcar RP doesn't complete L1 transition, so the RP physical layer is still active. Hence the EP thinks it is in L1, but the RP is not. > > The rcar RP cannot enter L1 by HW alone, so is still in L0. > > See above. > > > The only way out of this from the PCIe spec FSM is for both EP and RP > > to enter the Recovery state. > > The patch simply detects that we should have gone into L1, and so > > initiates that state change, and the HW will then handle the > > transition from L1 to Recovery and then on to L0. > > That I can understand, I reckon it is to "reset" the RP link state machine to a > "sane" state. Bjorn's comment added back: > I think there's still a potential issue if the endpoint goes to a > non-D0 state, the link is stuck in this transitional state (endpoint > thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > L1, e.g., so it can send a PME message for a wakeup. I don't know > what happens then. > > > > I don't think the potential issue that Bjorn talked about can > > > > happen because the RP does go into L1. I could be wrong though... > > > > > > I do not understand this paragraph, mind elaborating on it ? > > As rcar RP only supports D0 and D3hot/cold, (the manual says it > > supports D3cold, but I cannot see how if it doesn't support L2 or L3 > > states), if you force the link to D3, we can only be in L1 state. > > D3 is a device state, not a link state. I still do not understand this statement. > > The link between RP and EP can enter L1 when all functions in the EP are in a > device state != D0 but, as I mentioned above, it is still unclear what happens > in this platform since I do not get what state in the PCI spec 5.3.2.1 state > machine the RP Link state machine is in. > > If we programme the device into any D-state and the device wants to send a > PME message _before_ we reset the RP state machine with the procedure > described in this thread, what happens ? Or, more explicitly, what are in > _HW_ the states of upstream and downstream link state machines when the > EP is put in, say, D1 ? rcar only supports D0 and D3, and L0/L0s/L1 so it's a bit simpler (I assume devices can only be put into D states that are supported by the RP). If I read the PCIe spec 5.3.2 correctly, for rcar, if the device is put into D3, the interconnect state must be L1. Hence my comment... Re-reading Bjorn's comment, I believe he is discussing a different case. I really don't know what will happen in this case. Can you suggest a test to get a device to go from D3 to D0? Would suspend using a NIC with WOL be enough? Or something simpler? > That's in short our question. I would be happy to get to the bottom of this > since it is an interesting issue we are facing, we need HW details, I can apply > Marek's patch but I would be happier if I get the whole picture first. Sure, I'll help if I can. Thanks for your help Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling @ 2018-08-22 9:20 ` Phil Edworthy 0 siblings, 0 replies; 33+ messages in thread From: Phil Edworthy @ 2018-08-22 9:20 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, Bjorn Helgaas, linux-pci, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc Hi Lorenzo, On 21 August 2018 16:32, Lorenzo Pieralisi wrote: > On Tue, Aug 21, 2018 at 08:58:38AM +0000, Phil Edworthy wrote: > > On 20 August 2018 15:48 Lorenzo Pieralisi wrote: > > > On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote: > > > > > > [...] > > > > > > > However, both before and after this patch, the RP does not > > > > transition > > > > L1 when the endpoints change to L1. > > > > This patch only transitions the RP to L1 during accessing a card's > > > > config registers, if the RP is not in L1 link state and has > > > > received > > > > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will > > > > handle the transition out of L1. > > > > > > > > The relevant part of the rcar manual says: "After a recovery to > > > > L0, if the device is in the Non-D0 state and PM_Enter_L1 DLLP is > > > > transmitted from the downstream device, software should confirm > > > > that hardware is in the L0 state (PMSR.PMSTATE =3D L0) and initiate > > > > the L1 transition sequence again (write 1 to PMCTLR.L1IATN). In > > > > this case, the sequence > > > > is: L0 ??? L1 ??? L0 recovery ??? L1 again." > > > > > > Can you map these FSM steps to this patch code please ? I would like > > > to understand what Link state maps to which command written and > when. > > I don't think I can because we are not initially entering L1. Looking > > at this again, I think this section of the manual only helps in > > indicating how to detect we should have gone into L1 and how to poke > > the HW to initiate the transition to L1. > > > > On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state. >=20 > I am still struggling to understand what "EP enters L1 state" means. A li= nk in > L1 means both ends of the link are in electrical idle, it is a two-way > handshake, see PCI express specifications 5.3.2.1 "Entry into the L1 stat= e". Sorry, I'm no PCIe expert and the rcar HW documentation doesn't provide enough detail. I guess (that's all I can do with this) the following: a) the EP sends the PM_Enter_L1 DLLP, b) the RP sends a PM_Request_Ack DLLP. c) The EP physical layer is then inactive. However, the rcar RP doesn't complete L1 transition, so the RP physical lay= er is still active. Hence the EP thinks it is in L1, but the RP is not. > > The rcar RP cannot enter L1 by HW alone, so is still in L0. >=20 > See above. >=20 > > The only way out of this from the PCIe spec FSM is for both EP and RP > > to enter the Recovery state. > > The patch simply detects that we should have gone into L1, and so > > initiates that state change, and the HW will then handle the > > transition from L1 to Recovery and then on to L0. >=20 > That I can understand, I reckon it is to "reset" the RP link state machin= e to a > "sane" state. Bjorn's comment added back: > I think there's still a potential issue if the endpoint goes to a > non-D0 state, the link is stuck in this transitional state (endpoint=20 > thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit=20 > L1, e.g., so it can send a PME message for a wakeup. I don't know=20 > what happens then. > > > > I don't think the potential issue that Bjorn talked about can > > > > happen because the RP does go into L1. I could be wrong though... > > > > > > I do not understand this paragraph, mind elaborating on it ? > > As rcar RP only supports D0 and D3hot/cold, (the manual says it > > supports D3cold, but I cannot see how if it doesn't support L2 or L3 > > states), if you force the link to D3, we can only be in L1 state. >=20 > D3 is a device state, not a link state. I still do not understand this st= atement. >=20 > The link between RP and EP can enter L1 when all functions in the EP are = in a > device state !=3D D0 but, as I mentioned above, it is still unclear what = happens > in this platform since I do not get what state in the PCI spec 5.3.2.1 st= ate > machine the RP Link state machine is in. >=20 > If we programme the device into any D-state and the device wants to send = a > PME message _before_ we reset the RP state machine with the procedure > described in this thread, what happens ? Or, more explicitly, what are in > _HW_ the states of upstream and downstream link state machines when the > EP is put in, say, D1 ? rcar only supports D0 and D3, and L0/L0s/L1 so it's a bit simpler (I assume devices can only be put into D states that are supported by the RP).=20 If I read the PCIe spec 5.3.2 correctly, for rcar, if the device is put int= o D3, the interconnect state must be L1. Hence my comment... Re-reading Bjorn's comment, I believe he is discussing a different case. I really don't know what will happen in this case. Can you suggest a test to get a device to go from D3 to D0? Would suspend using a NIC with WOL be enough? Or something simpler? > That's in short our question. I would be happy to get to the bottom of th= is > since it is an interesting issue we are facing, we need HW details, I can= apply > Marek's patch but I would be happier if I get the whole picture first. Sure, I'll help if I can. Thanks for your help Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling 2018-06-13 17:25 ` Bjorn Helgaas 2018-06-14 11:43 ` Lorenzo Pieralisi 2018-07-25 21:08 ` Marek Vasut @ 2018-08-14 16:25 ` Lorenzo Pieralisi 2 siblings, 0 replies; 33+ messages in thread From: Lorenzo Pieralisi @ 2018-08-14 16:25 UTC (permalink / raw) To: Bjorn Helgaas Cc: Marek Vasut, linux-pci, Phil Edworthy, Marek Vasut, Geert Uytterhoeven, Simon Horman, Wolfram Sang, linux-renesas-soc On Wed, Jun 13, 2018 at 12:25:59PM -0500, Bjorn Helgaas wrote: <snip> > Putting this workaround in the config accessor makes sense to me > because in this situation the endpoint thinks it's in L1 and it won't > receive TLPs for config accesses. Apparently forcing the RP to L1 > completes the L1 entry, and the RP correctly handles the "Exit from L1 > State" (sec 5.3.2.2) that's required when the RP needs to send a TLP > to the endpoint. > > I think there's still a potential issue if the endpoint goes to a > non-D0 state, the link is stuck in this transitional state (endpoint > thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > L1, e.g., so it can send a PME message for a wakeup. I don't know > what happens then. What is not clear to me is whether the endpoint link state can really be in electrical idle (so ready for L1) if it has not received a PM_Request_Ack DLLP from the host controller. It is not clear whether the host controller sends it upon PM_Enter_L1 DLLP reception (ie does it actually carry out step 9 in PCIe specs 5.3.2.1 "Entry into L1 state") or it has to be coerced into L1 to send it. I agree with Bjorn that this is not clear at all and it boils down to how HW is designed. We need to understand what "Endpoint in L1, RP in L0" means in this context and for that we need an errata document otherwise that's impossible to untangle. Lorenzo > If there were a real erratum writeup for this, it would probably > discuss this situation. > > > > > > If that's the case, we shouldn't enable L1 > > > > > entry on a card. Is the core ASPM code doing something wrong here? > > > > > > > > I can double-check, am I looking for some particular register in the > > > > PCIe space ? > > > > > > > > >>>> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer > > > > >>>> access the card's config registers. > > > > >>>> > > > > >>>> The R-Car host controller will handle taking cards out of L1 as long as > > > > >>>> the host controller has also been transitioned to L1 link state. > > > > >>> > > > > >>> I wonder why this can't be done in a PM restore hook but that's not > > > > >>> really where my question is. > > > > >> > > > > >> I suspect because the link can be in L1 during startup too? > > > > >> > > > > >>>> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and > > > > >>>> transition the host to L1 immediately. However, this patch just ensures > > > > >>>> that we can talk to cards after they have gone into L1. > > > > >>> > > > > >>>> When attempting a config access, it checks to see if the card has gone > > > > >>>> into L1, and if so, does the same for the host controller. > > > > >>>> > > > > >>>> This is based on a patch by Hien Dang <hien.dang.eb@rvc.renesas.com> > > > > >>>> > > > > >>>> Signed-off-by: Phil Edworthy <phil.edworthy@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: - Drop extra parenthesis > > > > >>>> - Use GENMASK() > > > > >>>> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/ > > > > >>>> --- > > > > >>>> drivers/pci/host/pcie-rcar.c | 24 ++++++++++++++++++++++++ > > > > >>>> 1 file changed, 24 insertions(+) > > > > >>>> > > > > >>>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > > > > >>>> index ab61829db389..068bf9067ec1 100644 > > > > >>>> --- a/drivers/pci/host/pcie-rcar.c > > > > >>>> +++ b/drivers/pci/host/pcie-rcar.c > > > > >>>> @@ -92,6 +92,13 @@ > > > > >>>> #define MACCTLR 0x011058 > > > > >>>> #define SPEED_CHANGE BIT(24) > > > > >>>> #define SCRAMBLE_DISABLE BIT(27) > > > > >>>> +#define PMSR 0x01105c > > > > >>>> +#define L1FAEG BIT(31) > > > > >>>> +#define PM_ENTER_L1RX BIT(23) > > > > >>>> +#define PMSTATE GENMASK(18, 16) > > > > >>>> +#define PMSTATE_L1 GENMASK(17, 16) > > > > >>>> +#define PMCTLR 0x011060 > > > > >>>> +#define L1_INIT BIT(31) > > > > >>>> #define MACS2R 0x011078 > > > > >>>> #define MACCGSPSETR 0x011084 > > > > >>>> #define SPCNGRSN BIT(31) > > > > >>>> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > > >>>> unsigned int devfn, int where, u32 *data) > > > > >>>> { > > > > >>>> int dev, func, reg, index; > > > > >>>> + u32 val; > > > > >>>> > > > > >>>> dev = PCI_SLOT(devfn); > > > > >>>> func = PCI_FUNC(devfn); > > > > >>>> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie, > > > > >>>> if (pcie->root_bus_nr < 0) > > > > >>>> return PCIBIOS_DEVICE_NOT_FOUND; > > > > >>>> > > > > >>>> + /* > > > > >>>> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP, > > > > >>>> + * transition to L1 link state. The HW will handle coming out of L1. > > > > >>>> + */ > > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > > >>>> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) { > > > > >>>> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR); > > > > >>>> + > > > > >>>> + /* Wait until we are in L1 */ > > > > >>>> + while (!(val & L1FAEG)) > > > > >>>> + val = rcar_pci_read_reg(pcie, PMSR); > > > > >>>> + > > > > >>>> + /* Clear flags indicating link has transitioned to L1 */ > > > > >>>> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR); > > > > >>>> + } > > > > >>> > > > > >>> I do not get why you need to add the DLLP check for _every_ given config > > > > >>> access and how/why it is just related to suspend/resume and not eg cold > > > > >>> boot (I supposed it is because devices can enter L1 upon suspend(?)), I > > > > >>> would ask you please to provide a thorough explanation so that I can > > > > >>> actually review this patch (the commit log must be rewritten nonetheless, > > > > >>> I do not think it is clear, at least it is not for me). > > > > >> > > > > >> See above > > > > >> > > > > >> -- > > > > >> Best regards, > > > > >> Marek Vasut > > > > > > > > > > > > -- > > > > Best regards, > > > > Marek Vasut ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut ` (3 preceding siblings ...) 2017-11-10 21:58 ` [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling Marek Vasut @ 2017-11-10 21:58 ` Marek Vasut 2017-11-15 13:27 ` Simon Horman 4 siblings, 1 reply; 33+ messages in thread From: Marek Vasut @ 2017-11-10 21:58 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> This adds the suspend/resume supports for pcie-rcar. The resume handler reprograms the hardware based on the software state kept in specific device structures. Also it doesn't need to save any registers. 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: - Change return type of rcar_pcie_hw_enable() to void - Drop default: case in switch statement in rcar_pcie_hw_enable() - Sort variables in rcar_pcie_resume() --- drivers/pci/host/pcie-rcar.c | 82 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 8 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 068bf9067ec1..f65ad226335d 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -471,6 +471,32 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5"); } +static void rcar_pcie_hw_enable(struct rcar_pcie *pci) +{ + struct resource_entry *win; + LIST_HEAD(res); + int i = 0; + + /* Try setting 5 GT/s link speed */ + rcar_pcie_force_speedup(pci); + + /* Setup PCI resources */ + resource_list_for_each_entry(win, &pci->resources) { + struct resource *res = win->res; + + if (!res->flags) + continue; + + switch (resource_type(res)) { + case IORESOURCE_IO: + case IORESOURCE_MEM: + rcar_pcie_setup_window(i, pci, res); + i++; + break; + } + } +} + static int rcar_pcie_enable(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; @@ -869,11 +895,25 @@ static const struct irq_domain_ops msi_domain_ops = { .map = rcar_msi_map, }; +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie) +{ + struct rcar_msi *msi = &pcie->msi; + unsigned long base; + + /* setup MSI data target */ + base = virt_to_phys((void *)msi->pages); + + rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR); + rcar_pci_write_reg(pcie, 0, PCIEMSIAUR); + + /* enable all MSI interrupts */ + rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); +} + static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; struct rcar_msi *msi = &pcie->msi; - unsigned long base; int err, i; mutex_init(&msi->lock); @@ -912,13 +952,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) /* setup MSI data target */ msi->pages = __get_free_pages(GFP_KERNEL, 0); - base = virt_to_phys((void *)msi->pages); - - rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR); - rcar_pci_write_reg(pcie, 0, PCIEMSIAUR); - - /* enable all MSI interrupts */ - rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER); + rcar_pcie_hw_enable_msi(pcie); return 0; @@ -1193,6 +1227,37 @@ static int rcar_pcie_probe(struct platform_device *pdev) return err; } +static int rcar_pcie_resume(struct device *dev) +{ + struct rcar_pcie *pcie = dev_get_drvdata(dev); + int (*hw_init_fn)(struct rcar_pcie *); + unsigned int data; + int err; + + err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); + if (err) + return 0; + + /* Failure to get a link might just be that no cards are inserted */ + hw_init_fn = of_device_get_match_data(dev); + err = hw_init_fn(pcie); + if (err) { + dev_info(dev, "PCIe link down\n"); + return 0; + } + + data = rcar_pci_read_reg(pcie, MACSR); + dev_info(dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f); + + /* Enable MSI */ + if (IS_ENABLED(CONFIG_PCI_MSI)) + rcar_pcie_hw_enable_msi(pcie); + + rcar_pcie_hw_enable(pcie); + + return 0; +} + static int rcar_pcie_resume_noirq(struct device *dev) { struct rcar_pcie *pcie = dev_get_drvdata(dev); @@ -1207,6 +1272,7 @@ static int rcar_pcie_resume_noirq(struct device *dev) } static const struct dev_pm_ops rcar_pcie_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(NULL, rcar_pcie_resume) .resume_noirq = rcar_pcie_resume_noirq, }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver 2017-11-10 21:58 ` [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver Marek Vasut @ 2017-11-15 13:27 ` Simon Horman 0 siblings, 0 replies; 33+ messages in thread From: Simon Horman @ 2017-11-15 13:27 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Wolfram Sang, linux-renesas-soc On Fri, Nov 10, 2017 at 10:58:43PM +0100, Marek Vasut wrote: > From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com> > > This adds the suspend/resume supports for pcie-rcar. The resume handler > reprograms the hardware based on the software state kept in specific > device structures. Also it doesn't need to save any registers. > > 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 Acked-by: Simon Horman <horms+renesas@verge.net.au> ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-08-22 12:44 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-10 21:58 [PATCH V2 0/5] PCI: rcar: Add suspend/resume support Marek Vasut 2017-11-10 21:58 ` [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl() Marek Vasut 2017-11-13 7:03 ` Simon Horman 2017-11-10 21:58 ` [PATCH V2 2/5] PCI: rcar: Clean up the macros Marek Vasut 2017-11-13 7:03 ` Simon Horman 2017-11-13 18:11 ` Marek Vasut 2017-11-15 13:28 ` Simon Horman 2017-11-22 11:20 ` Marek Vasut 2017-11-10 21:58 ` [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq Marek Vasut 2017-11-13 7:05 ` Simon Horman 2017-11-10 21:58 ` [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling Marek Vasut 2017-11-13 7:05 ` Simon Horman 2017-11-17 17:49 ` Lorenzo Pieralisi 2018-06-10 13:57 ` Marek Vasut 2018-06-11 13:59 ` Bjorn Helgaas 2018-06-12 23:54 ` Marek Vasut 2018-06-13 13:53 ` Bjorn Helgaas 2018-06-13 15:52 ` Lorenzo Pieralisi 2018-06-13 17:25 ` Bjorn Helgaas 2018-06-14 11:43 ` Lorenzo Pieralisi 2018-07-25 21:08 ` Marek Vasut 2018-08-08 13:29 ` Marek Vasut 2018-08-20 13:44 ` Phil Edworthy 2018-08-20 13:44 ` Phil Edworthy 2018-08-20 14:47 ` Lorenzo Pieralisi 2018-08-21 8:58 ` Phil Edworthy 2018-08-21 8:58 ` Phil Edworthy 2018-08-21 15:32 ` Lorenzo Pieralisi 2018-08-22 9:20 ` Phil Edworthy 2018-08-22 9:20 ` Phil Edworthy 2018-08-14 16:25 ` Lorenzo Pieralisi 2017-11-10 21:58 ` [PATCH V2 5/5] PCI: rcar: Add the suspend/resume for pcie-rcar driver Marek Vasut 2017-11-15 13:27 ` Simon Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.