[PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Marek Vasut]

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: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 889603783f01..aa588a7d4811 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		(1 << 3)
 #define  CFINIT			1
 #define PCIETSTR		0x02004
 #define  DATA_LINK_ACTIVE	1
@@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
 	phy_wait_for_ack(pcie);
 }
 
-static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
+static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
 {
 	unsigned int timeout = 10;
 
@@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
 		if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
 			return 0;
 
-		msleep(5);
+		if (atomic)
+			mdelay(5);
+		else
+			msleep(5);
 	}
 
 	return -ETIMEDOUT;
@@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 
 	/* This will timeout if we don't have a link. */
-	err = rcar_pcie_wait_for_dl(pcie);
+	err = rcar_pcie_wait_for_dl(pcie, 0);
 	if (err)
 		return err;
 
@@ -1110,6 +1114,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);
 
@@ -1173,10 +1178,30 @@ 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);
+	u32 val = rcar_pci_read_reg(pcie, PMSR);
+	int ret = 0;
+
+	if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {
+		/* Re-establish the PCIe link */
+		rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
+		ret = rcar_pcie_wait_for_dl(pcie, 1);
+	}
+
+	return ret;
+}
+
+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

[PATCH 2/3] PCI: rcar: Support runtime PM, link state L1 handling

[Marek Vasut]

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: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
---
 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 aa588a7d4811..2b28292de93a 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		(1 << 24)
 #define  SCRAMBLE_DISABLE	(1 << 27)
+#define PMSR			0x01105c
+#define  L1FAEG			(1 << 31)
+#define  PM_ENTER_L1RX		(1 << 23)
+#define  PMSTATE		(7 << 16)
+#define  PMSTATE_L1		(3 << 16)
+#define PMCTLR			0x011060
+#define  L1_INIT		(1 << 31)
 #define MACS2R			0x011078
 #define MACCGSPSETR		0x011084
 #define  SPCNGRSN		(1 << 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 of 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

[PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver

[Marek Vasut]

From: Kazufumi Ikeda <kaz-ikeda@xc.jp.nec.com>

This adds the suspend/resume supports for pcie-rcar. The resume handler
reprogram 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: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/host/pcie-rcar.c | 86 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 2b28292de93a..7a9e30185c79 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
 		 (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
 }
 
+static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)
+{
+	struct resource_entry *win;
+	LIST_HEAD(res);
+	int i = 0;
+
+	/* Try setting 5 GT/s link speed */
+	rcar_pcie_force_speedup(pcie);
+
+	/* Setup PCI resources */
+	resource_list_for_each_entry(win, &pcie->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, pcie, res);
+			i++;
+			break;
+		default:
+			continue;
+		}
+	}
+
+	return 0;
+}
+
 static int rcar_pcie_enable(struct rcar_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -872,11 +902,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);
@@ -915,13 +959,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;
 
@@ -1202,6 +1240,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);
+	unsigned int data;
+	int err;
+	int (*hw_init_fn)(struct rcar_pcie *);
+
+	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);
@@ -1218,6 +1287,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

Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Sergei Shtylyov]

On 11/8/2017 12:28 PM, 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: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>   drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
>   1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 889603783f01..aa588a7d4811 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
[...]
> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>   	phy_wait_for_ack(pcie);
>   }
>   
> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)

    How about *bool* atomic?

[...]

MBR, Sergei

Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Simon Horman]

Hi Marek,

On Wed, Nov 08, 2017 at 10:28:04AM +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: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org

For patch-sets (with more than one patch) please provide a cover-letter.
The --cover-letter option to git format-patch can help.

> ---
>  drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 889603783f01..aa588a7d4811 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		(1 << 3)

Can you use the BIT() macro here?

>  #define  CFINIT			1
>  #define PCIETSTR		0x02004
>  #define  DATA_LINK_ACTIVE	1
> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>  	phy_wait_for_ack(pcie);
>  }
>  
> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)

As Sergei mentioned bool seems like a more appropriate type for atomic.

>  {
>  	unsigned int timeout = 10;
>  
> @@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
>  		if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
>  			return 0;
>  
> -		msleep(5);
> +		if (atomic)
> +			mdelay(5);
> +		else
> +			msleep(5);

If we must delay, then I suppose this is reasonable.

>  	}
>  
>  	return -ETIMEDOUT;
> @@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  
>  	/* This will timeout if we don't have a link. */
> -	err = rcar_pcie_wait_for_dl(pcie);
> +	err = rcar_pcie_wait_for_dl(pcie, 0);
>  	if (err)
>  		return err;
>  
> @@ -1110,6 +1114,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);
>  
> @@ -1173,10 +1178,30 @@ 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);
> +	u32 val = rcar_pci_read_reg(pcie, PMSR);
> +	int ret = 0;
> +
> +	if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {

Please remove the unnecessary parentheses from the line above.

Also, I would prefer if the function returned early.
Something like (completely untested!):

	if (rcar_pci_read_reg(pcie, PMSR) &&
	    !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
		return 0;

	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
	return rcar_pcie_wait_for_dl(pcie, 1);

> +		/* Re-establish the PCIe link */
> +		rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> +		ret = rcar_pcie_wait_for_dl(pcie, 1);
> +	}
> +
> +	return ret;
> +}
> +
> +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
> 

Re: [PATCH 2/3] PCI: rcar: Support runtime PM, link state L1 handling

[Simon Horman]

On Wed, Nov 08, 2017 at 10:28:05AM +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: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org

Hi Marek,

With my nit addressed please feel free to add:

Acked-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  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 aa588a7d4811..2b28292de93a 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		(1 << 24)
>  #define  SCRAMBLE_DISABLE	(1 << 27)
> +#define PMSR			0x01105c
> +#define  L1FAEG			(1 << 31)
> +#define  PM_ENTER_L1RX		(1 << 23)
> +#define  PMSTATE		(7 << 16)
> +#define  PMSTATE_L1		(3 << 16)
> +#define PMCTLR			0x011060
> +#define  L1_INIT		(1 << 31)

The BIT() and GENMASK() macros are nice in my opinion.
But I see what you have added follows the style of the existing code
in the driver.

>  #define MACS2R			0x011078
>  #define MACCGSPSETR		0x011084
>  #define  SPCNGRSN		(1 << 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 of of L1.
> +	 */
> +	val = rcar_pci_read_reg(pcie, PMSR);
> +	if ((val & PM_ENTER_L1RX) && ((val & PMSTATE) != PMSTATE_L1)) {

nit: please remove the unnecessary parentheses from the line above.

> +		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
> 

Re: [PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver

[Simon Horman]

On Wed, Nov 08, 2017 at 10:28:06AM +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
> reprogram 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: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/pci/host/pcie-rcar.c | 86 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 78 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 2b28292de93a..7a9e30185c79 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
>  		 (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
>  }
>  
> +static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)

This function always returns 0 and the value is not checked by the caller.
Can we change the return type to void?

> +{
> +	struct resource_entry *win;
> +	LIST_HEAD(res);
> +	int i = 0;
> +
> +	/* Try setting 5 GT/s link speed */

What if it fails?

> +	rcar_pcie_force_speedup(pcie);
> +
> +	/* Setup PCI resources */
> +	resource_list_for_each_entry(win, &pcie->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, pcie, res);
> +			i++;
> +			break;
> +		default:
> +			continue;

Can the default case be omitted?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -872,11 +902,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);

Why do you need to cast to void *?
I expect such casting can be done implicitly.

> +
> +	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);
> @@ -915,13 +959,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;
>  
> @@ -1202,6 +1240,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);
> +	unsigned int data;
> +	int err;
> +	int (*hw_init_fn)(struct rcar_pcie *);

Please sort local variables in reverse xmas tree order.

> +
> +	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);
> @@ -1218,6 +1287,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
> 

Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Marek Vasut]

On 11/08/2017 12:00 PM, Sergei Shtylyov wrote:
> On 11/8/2017 12:28 PM, 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: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>   drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
>>   1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
>> index 889603783f01..aa588a7d4811 100644
>> --- a/drivers/pci/host/pcie-rcar.c
>> +++ b/drivers/pci/host/pcie-rcar.c
> [...]
>> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>>       phy_wait_for_ack(pcie);
>>   }
>>   -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
>> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
> 
>    How about *bool* atomic?

Not a big fan of bool in C, but I think I can avoid this altogether.
If I poll more often, I can just use udelay(5) for such a short delay.

> [...]
> 
> MBR, Sergei


-- 
Best regards,
Marek Vasut

Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Marek Vasut]

On 11/10/2017 09:59 AM, Simon Horman wrote:
> Hi Marek,

Hi Simon,

> On Wed, Nov 08, 2017 at 10:28:04AM +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: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
> 
> For patch-sets (with more than one patch) please provide a cover-letter.
> The --cover-letter option to git format-patch can help.
> 
>> ---
>>  drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
>> index 889603783f01..aa588a7d4811 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		(1 << 3)
> 
> Can you use the BIT() macro here?

Yes, I'll also add a patch to the series to convert all the others to
the BIT() macro, to keep things consistent.

>>  #define  CFINIT			1
>>  #define PCIETSTR		0x02004
>>  #define  DATA_LINK_ACTIVE	1
>> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>>  	phy_wait_for_ack(pcie);
>>  }
>>  
>> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
>> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
> 
> As Sergei mentioned bool seems like a more appropriate type for atomic.

I can actually get rid of this altogether.

>>  {
>>  	unsigned int timeout = 10;
>>  
>> @@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
>>  		if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
>>  			return 0;
>>  
>> -		msleep(5);
>> +		if (atomic)
>> +			mdelay(5);
>> +		else
>> +			msleep(5);
> 
> If we must delay, then I suppose this is reasonable.

See above

>>  	}
>>  
>>  	return -ETIMEDOUT;
>> @@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
>>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>>  
>>  	/* This will timeout if we don't have a link. */
>> -	err = rcar_pcie_wait_for_dl(pcie);
>> +	err = rcar_pcie_wait_for_dl(pcie, 0);
>>  	if (err)
>>  		return err;
>>  
>> @@ -1110,6 +1114,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);
>>  
>> @@ -1173,10 +1178,30 @@ 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);
>> +	u32 val = rcar_pci_read_reg(pcie, PMSR);
>> +	int ret = 0;
>> +
>> +	if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {
> 
> Please remove the unnecessary parentheses from the line above.
> 
> Also, I would prefer if the function returned early.
> Something like (completely untested!):

This should work.

> 	if (rcar_pci_read_reg(pcie, PMSR) &&
> 	    !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> 		return 0;
> 
> 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> 	return rcar_pcie_wait_for_dl(pcie, 1);
> 
>> +		/* Re-establish the PCIe link */
>> +		rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>> +		ret = rcar_pcie_wait_for_dl(pcie, 1);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +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
>>


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] PCI: rcar: Support runtime PM, link state L1 handling

[Marek Vasut]

On 11/10/2017 10:03 AM, Simon Horman wrote:
> On Wed, Nov 08, 2017 at 10:28:05AM +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: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
> 
> Hi Marek,
> 
> With my nit addressed please feel free to add:
> 
> Acked-by: Simon Horman <horms+renesas@verge.net.au>
> 
>> ---
>>  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 aa588a7d4811..2b28292de93a 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		(1 << 24)
>>  #define  SCRAMBLE_DISABLE	(1 << 27)
>> +#define PMSR			0x01105c
>> +#define  L1FAEG			(1 << 31)
>> +#define  PM_ENTER_L1RX		(1 << 23)
>> +#define  PMSTATE		(7 << 16)
>> +#define  PMSTATE_L1		(3 << 16)
>> +#define PMCTLR			0x011060
>> +#define  L1_INIT		(1 << 31)
> 
> The BIT() and GENMASK() macros are nice in my opinion.
> But I see what you have added follows the style of the existing code
> in the driver.

I added a patch into the series to clean this up and will then use BIT()
and GENMASK() here too.

>>  #define MACS2R			0x011078
>>  #define MACCGSPSETR		0x011084
>>  #define  SPCNGRSN		(1 << 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 of of L1.
>> +	 */
>> +	val = rcar_pci_read_reg(pcie, PMSR);
>> +	if ((val & PM_ENTER_L1RX) && ((val & PMSTATE) != PMSTATE_L1)) {
> 
> nit: please remove the unnecessary parentheses from the line above.

Fixed

>> +		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
>>


-- 
Best regards,
Marek Vasut

Re: [PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver

[Marek Vasut]

On 11/10/2017 10:09 AM, Simon Horman wrote:
> On Wed, Nov 08, 2017 at 10:28:06AM +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
>> reprogram 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: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>  drivers/pci/host/pcie-rcar.c | 86 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 78 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
>> index 2b28292de93a..7a9e30185c79 100644
>> --- a/drivers/pci/host/pcie-rcar.c
>> +++ b/drivers/pci/host/pcie-rcar.c
>> @@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
>>  		 (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
>>  }
>>  
>> +static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)
> 
> This function always returns 0 and the value is not checked by the caller.
> Can we change the return type to void?

Yes, done

>> +{
>> +	struct resource_entry *win;
>> +	LIST_HEAD(res);
>> +	int i = 0;
>> +
>> +	/* Try setting 5 GT/s link speed */
> 
> What if it fails?

If it fails, we're back at 2.5 GT/s . The rcar_pcie_force_speedup()
first checks if the PCIe IP can do 5 GT/s at all. Only if so, tries to
initiate transition to 5 GT/s operation , checks whether that succeeded
and if it failed, falls back to 2.5 GT/s .

>> +	rcar_pcie_force_speedup(pcie);
>> +
>> +	/* Setup PCI resources */
>> +	resource_list_for_each_entry(win, &pcie->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, pcie, res);
>> +			i++;
>> +			break;
>> +		default:
>> +			continue;
> 
> Can the default case be omitted?

Sure

>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
>>  {
>>  	struct device *dev = pcie->dev;
>> @@ -872,11 +902,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);
> 
> Why do you need to cast to void *?
> I expect such casting can be done implicitly.

Because __get_free_pages() returns unsigned long and that's what's used
to assign msi->pages . And virt_to_phys() expects void * instead, thus
the cast.

>> +
>> +	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);
>> @@ -915,13 +959,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;
>>  
>> @@ -1202,6 +1240,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);
>> +	unsigned int data;
>> +	int err;
>> +	int (*hw_init_fn)(struct rcar_pcie *);
> 
> Please sort local variables in reverse xmas tree order.

OK

>> +
>> +	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);
>> @@ -1218,6 +1287,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
>>


-- 
Best regards,
Marek Vasut

Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq

[Simon Horman]

On Fri, Nov 10, 2017 at 10:14:33PM +0100, Marek Vasut wrote:
> On 11/10/2017 09:59 AM, Simon Horman wrote:
> > Hi Marek,
> 
> Hi Simon,
> 
> > On Wed, Nov 08, 2017 at 10:28:04AM +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: Simon Horman <horms+renesas@verge.net.au>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: linux-renesas-soc@vger.kernel.org
> > 
> > For patch-sets (with more than one patch) please provide a cover-letter.
> > The --cover-letter option to git format-patch can help.
> > 
> >> ---
> >>  drivers/pci/host/pcie-rcar.c | 31 ++++++++++++++++++++++++++++---
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 889603783f01..aa588a7d4811 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		(1 << 3)
> > 
> > Can you use the BIT() macro here?
> 
> Yes, I'll also add a patch to the series to convert all the others to
> the BIT() macro, to keep things consistent.

Great, thanks.

> >>  #define  CFINIT			1
> >>  #define PCIETSTR		0x02004
> >>  #define  DATA_LINK_ACTIVE	1
> >> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
> >>  	phy_wait_for_ack(pcie);
> >>  }
> >>  
> >> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> >> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
> > 
> > As Sergei mentioned bool seems like a more appropriate type for atomic.
> 
> I can actually get rid of this altogether.

Nice :)

> >>  {
> >>  	unsigned int timeout = 10;
> >>  
> >> @@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> >>  		if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
> >>  			return 0;
> >>  
> >> -		msleep(5);
> >> +		if (atomic)
> >> +			mdelay(5);
> >> +		else
> >> +			msleep(5);
> > 
> > If we must delay, then I suppose this is reasonable.
> 
> See above
> 
> >>  	}
> >>  
> >>  	return -ETIMEDOUT;
> >> @@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
> >>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >>  
> >>  	/* This will timeout if we don't have a link. */
> >> -	err = rcar_pcie_wait_for_dl(pcie);
> >> +	err = rcar_pcie_wait_for_dl(pcie, 0);
> >>  	if (err)
> >>  		return err;
> >>  
> >> @@ -1110,6 +1114,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);
> >>  
> >> @@ -1173,10 +1178,30 @@ 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);
> >> +	u32 val = rcar_pci_read_reg(pcie, PMSR);
> >> +	int ret = 0;
> >> +
> >> +	if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {
> > 
> > Please remove the unnecessary parentheses from the line above.
> > 
> > Also, I would prefer if the function returned early.
> > Something like (completely untested!):
> 
> This should work.
> 
> > 	if (rcar_pci_read_reg(pcie, PMSR) &&
> > 	    !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> > 		return 0;
> > 
> > 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> > 	return rcar_pcie_wait_for_dl(pcie, 1);
> > 
> >> +		/* Re-establish the PCIe link */
> >> +		rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >> +		ret = rcar_pcie_wait_for_dl(pcie, 1);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +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
> >>
> 
> 
> -- 
> Best regards,
> Marek Vasut
> 

Re: [PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver

[Simon Horman]

On Fri, Nov 10, 2017 at 10:53:07PM +0100, Marek Vasut wrote:
> On 11/10/2017 10:09 AM, Simon Horman wrote:
> > On Wed, Nov 08, 2017 at 10:28:06AM +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
> >> reprogram 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: Simon Horman <horms+renesas@verge.net.au>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >>  drivers/pci/host/pcie-rcar.c | 86 +++++++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 78 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 2b28292de93a..7a9e30185c79 100644
> >> --- a/drivers/pci/host/pcie-rcar.c
> >> +++ b/drivers/pci/host/pcie-rcar.c
> >> @@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
> >>  		 (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
> >>  }
> >>  
> >> +static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)
> > 
> > This function always returns 0 and the value is not checked by the caller.
> > Can we change the return type to void?
> 
> Yes, done
> 
> >> +{
> >> +	struct resource_entry *win;
> >> +	LIST_HEAD(res);
> >> +	int i = 0;
> >> +
> >> +	/* Try setting 5 GT/s link speed */
> > 
> > What if it fails?
> 
> If it fails, we're back at 2.5 GT/s . The rcar_pcie_force_speedup()
> first checks if the PCIe IP can do 5 GT/s at all. Only if so, tries to
> initiate transition to 5 GT/s operation , checks whether that succeeded
> and if it failed, falls back to 2.5 GT/s .

Thanks, got it.

> >> +	rcar_pcie_force_speedup(pcie);
> >> +
> >> +	/* Setup PCI resources */
> >> +	resource_list_for_each_entry(win, &pcie->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, pcie, res);
> >> +			i++;
> >> +			break;
> >> +		default:
> >> +			continue;
> > 
> > Can the default case be omitted?
> 
> Sure
> 
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
> >>  {
> >>  	struct device *dev = pcie->dev;
> >> @@ -872,11 +902,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);
> > 
> > Why do you need to cast to void *?
> > I expect such casting can be done implicitly.
> 
> Because __get_free_pages() returns unsigned long and that's what's used
> to assign msi->pages . And virt_to_phys() expects void * instead, thus
> the cast.

Right, but I don't think one should ever need to explicitly cast
to or from void *. What mean is, can you just remove "(void *)" without
changing any behaviour?

> 
> >> +
> >> +	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);
> >> @@ -915,13 +959,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;
> >>  
> >> @@ -1202,6 +1240,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);
> >> +	unsigned int data;
> >> +	int err;
> >> +	int (*hw_init_fn)(struct rcar_pcie *);
> > 
> > Please sort local variables in reverse xmas tree order.
> 
> OK
> 
> >> +
> >> +	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);
> >> @@ -1218,6 +1287,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
> >>
> 
> 
> -- 
> Best regards,
> Marek Vasut
> 

Re: [PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver

[Geert Uytterhoeven]

Hi Simon,

On Mon, Nov 13, 2017 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Nov 10, 2017 at 10:53:07PM +0100, Marek Vasut wrote:
>> On 11/10/2017 10:09 AM, Simon Horman wrote:
>> > On Wed, Nov 08, 2017 at 10:28:06AM +0100, Marek Vasut wrote:
>> >> @@ -872,11 +902,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);
>> >
>> > Why do you need to cast to void *?
>> > I expect such casting can be done implicitly.
>>
>> Because __get_free_pages() returns unsigned long and that's what's used
>> to assign msi->pages . And virt_to_phys() expects void * instead, thus
>> the cast.
>
> Right, but I don't think one should ever need to explicitly cast
> to or from void *. What mean is, can you just remove "(void *)" without

In general, I agree ("casts are evil").

> changing any behaviour?

In this particular case, there's no way around the cast, as __get_free_pages()
returns unsigned long because of historical reasons (if written today, it
would return a pointer for sure).

Other exceptions are storing integral types in of_device_id.data (which is
void *), and storing private data pointers in Scsi_Host.hostdata (which is
unsigned long).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds