diff mbox series

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

Message ID 20200314191232.3122290-1-marek.vasut@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Lorenzo Pieralisi
Headers show
Series [V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver | expand

Commit Message

Marek Vasut March 14, 2020, 7:12 p.m. UTC
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()
V3: - Update on top of next-20200313
---
 drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 12 deletions(-)

Comments

Lorenzo Pieralisi March 20, 2020, 10:12 a.m. UTC | #1
On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com 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
> ---
> 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()
> V3: - Update on top of next-20200313
> ---
>  drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++-----
>  1 file changed, 74 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 759c6542c5c8..f86513638b8a 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -452,6 +452,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, win);
> +			i++;
> +			break;
> +		}
> +	}
> +}
> +
>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie)
>  	irq_domain_remove(msi->domain);
>  }
>  
> +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, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> +	rcar_pci_write_reg(pcie, upper_32_bits(base), 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;
> -	phys_addr_t base;
>  	int err, i;
>  
>  	mutex_init(&msi->lock);
> @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
>  
>  	/* setup MSI data target */
>  	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> -	if (!msi->pages) {
> -		err = -ENOMEM;
> -		goto err;
> -	}
> -	base = virt_to_phys((void *)msi->pages);
> -
> -	rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> -	rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR);
> -
> -	/* enable all MSI interrupts */
> -	rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
> +	rcar_pcie_hw_enable_msi(pcie);
>  
>  	return 0;
>  
> @@ -1219,6 +1249,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);
> +	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);

Hi Marek,

happy to apply it as is, I was wondering if it is work taking this
look-up out of the resume path, it is not supposed to change anyway,
you can even save the function pointer at probe.

Again, happy to apply it as-is, just let me know please.

Thanks,
Lorenzo

> +	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);
> @@ -1234,6 +1295,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.25.0
>
Lorenzo Pieralisi April 24, 2020, 11:54 a.m. UTC | #2
On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com 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
> ---
> 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()
> V3: - Update on top of next-20200313
> ---
>  drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++-----
>  1 file changed, 74 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 759c6542c5c8..f86513638b8a 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c


Applied to pci/rcar for v5.18, thanks.

Lorenzo

> @@ -452,6 +452,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, win);
> +			i++;
> +			break;
> +		}
> +	}
> +}
> +
>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie)
>  	irq_domain_remove(msi->domain);
>  }
>  
> +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, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> +	rcar_pci_write_reg(pcie, upper_32_bits(base), 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;
> -	phys_addr_t base;
>  	int err, i;
>  
>  	mutex_init(&msi->lock);
> @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
>  
>  	/* setup MSI data target */
>  	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> -	if (!msi->pages) {
> -		err = -ENOMEM;
> -		goto err;
> -	}
> -	base = virt_to_phys((void *)msi->pages);
> -
> -	rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> -	rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR);
> -
> -	/* enable all MSI interrupts */
> -	rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
> +	rcar_pcie_hw_enable_msi(pcie);
>  
>  	return 0;
>  
> @@ -1219,6 +1249,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);
> +	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);
> @@ -1234,6 +1295,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.25.0
>
Bjorn Helgaas April 24, 2020, 7:57 p.m. UTC | #3
[+cc Vaibhav]

Alternate less redundant subject:

  PCI: rcar: Add suspend/resume support

On Sat, Mar 14, 2020 at 08:12:32PM +0100, marek.vasut@gmail.com 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.

s/This adds the/Add/
s/supports/support/

> 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()
> V3: - Update on top of next-20200313
> ---
>  drivers/pci/controller/pcie-rcar.c | 86 +++++++++++++++++++++++++-----
>  1 file changed, 74 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 759c6542c5c8..f86513638b8a 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -452,6 +452,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, win);
> +			i++;
> +			break;
> +		}
> +	}
> +}
> +
>  static int rcar_pcie_enable(struct rcar_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -891,11 +917,25 @@ static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie)
>  	irq_domain_remove(msi->domain);
>  }
>  
> +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, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> +	rcar_pci_write_reg(pcie, upper_32_bits(base), 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;
> -	phys_addr_t base;
>  	int err, i;
>  
>  	mutex_init(&msi->lock);
> @@ -934,17 +974,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
>  
>  	/* setup MSI data target */
>  	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> -	if (!msi->pages) {
> -		err = -ENOMEM;
> -		goto err;
> -	}
> -	base = virt_to_phys((void *)msi->pages);
> -
> -	rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR);
> -	rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR);
> -
> -	/* enable all MSI interrupts */
> -	rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
> +	rcar_pcie_hw_enable_msi(pcie);
>  
>  	return 0;
>  
> @@ -1219,6 +1249,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);
> +	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);
> @@ -1234,6 +1295,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)

This causes the following warning when CONFIG_PM_SLEEP is not set:

  drivers/pci/controller/pcie-rcar.c:1253:12: warning: ‘rcar_pcie_resume’ defined but not used [-Wunused-function]
   1253 | static int rcar_pcie_resume(struct device *dev)
	|            ^~~~~~~~~~~~~~~~

Most people seem to be using __maybe_unused on the suspend/resume
functions to avoid this, e.g., 226e6b866d74 ("gpio: pch: Convert to
dev_pm_ops").

>  	.resume_noirq = rcar_pcie_resume_noirq,
>  };
>  
> -- 
> 2.25.0
>
Geert Uytterhoeven April 25, 2020, 8:55 a.m. UTC | #4
Hi Bjorn,

On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Vaibhav]
>
> Alternate less redundant subject:
>
>   PCI: rcar: Add suspend/resume support

Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3
PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI).
People tend to use the prefix "PCI: rcar: " for both :-(

Gr{oetje,eeting}s,

                        Geert
Marek Vasut April 26, 2020, 12:32 p.m. UTC | #5
On 4/24/20 9:57 PM, Bjorn Helgaas wrote:
Hi,

[...]

>> @@ -1234,6 +1295,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)
> 
> This causes the following warning when CONFIG_PM_SLEEP is not set:
> 
>   drivers/pci/controller/pcie-rcar.c:1253:12: warning: ‘rcar_pcie_resume’ defined but not used [-Wunused-function]
>    1253 | static int rcar_pcie_resume(struct device *dev)
> 	|            ^~~~~~~~~~~~~~~~
> 
> Most people seem to be using __maybe_unused on the suspend/resume
> functions to avoid this, e.g., 226e6b866d74 ("gpio: pch: Convert to
> dev_pm_ops").

Should be fixed by:
PCI: pcie-rcar: Mark rcar_pcie_resume() with __maybe_unused
Marek Vasut April 26, 2020, 12:33 p.m. UTC | #6
On 3/20/20 11:12 AM, Lorenzo Pieralisi wrote:
[...]

>> +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);
>> +	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);
> 
> Hi Marek,

Hi,

> happy to apply it as is, I was wondering if it is work taking this
> look-up out of the resume path, it is not supposed to change anyway,
> you can even save the function pointer at probe.
> 
> Again, happy to apply it as-is, just let me know please.

I just sent subsequent patch to address this:
 PCI: pcie-rcar: Cache PHY init function pointer
Bjorn Helgaas April 27, 2020, 5:41 p.m. UTC | #7
[+cc Lorenzo]

On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote:
> On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc Vaibhav]
> >
> > Alternate less redundant subject:
> >
> >   PCI: rcar: Add suspend/resume support
> 
> Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3
> PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI).
> People tend to use the prefix "PCI: rcar: " for both :-(

Yeah, that's pretty broken, thanks for pointing this out!

For most drivers we use a chipset name ("keystone", "imx6", "tegra",
etc) as the changlog tag.  That's nice because it gives space for
multiple drivers from the same vendor, but I don't know anything
similarly specific for the R-Car drivers.

pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI
bus?  The "gen2" is confusing because "Gen 2" is more commonly used
for PCIe than for Conventional PCI.

I would propose keeping "rcar" for the PCIe driver and using
"rcar-pci" for the Conventional PCI one, but the Conventional PCI one
(pci-rcar-gen2.c) seems pretty inactive.  The most recent commits are
from 2018, and they're trivial cleanups.  So I'm doubtful that anybody
will remember when the next change comes in.

Bjorn
Geert Uytterhoeven April 27, 2020, 8:08 p.m. UTC | #8
Hi Bjorn,

On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > [+cc Vaibhav]
> > >
> > > Alternate less redundant subject:
> > >
> > >   PCI: rcar: Add suspend/resume support
> >
> > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3
> > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI).
> > People tend to use the prefix "PCI: rcar: " for both :-(
>
> Yeah, that's pretty broken, thanks for pointing this out!
>
> For most drivers we use a chipset name ("keystone", "imx6", "tegra",
> etc) as the changlog tag.  That's nice because it gives space for
> multiple drivers from the same vendor, but I don't know anything
> similarly specific for the R-Car drivers.
>
> pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI

AFAIUI it's some internal PCI glue to the *HCI USB controller.

> bus?  The "gen2" is confusing because "Gen 2" is more commonly used
> for PCIe than for Conventional PCI.

The "Gen2" applies to "R-Car", not to "PCI".

> I would propose keeping "rcar" for the PCIe driver and using
> "rcar-pci" for the Conventional PCI one, but the Conventional PCI one

(/me resists against bike-shedding)

> (pci-rcar-gen2.c) seems pretty inactive.  The most recent commits are
> from 2018, and they're trivial cleanups.  So I'm doubtful that anybody
> will remember when the next change comes in.

I guess pci-rcar-gen2.c is simpler and more mature ;-)
R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers
can be used together on the same hardware.

Gr{oetje,eeting}s,

                        Geert
Lorenzo Pieralisi April 28, 2020, 8:26 a.m. UTC | #9
On Mon, Apr 27, 2020 at 10:08:52PM +0200, Geert Uytterhoeven wrote:
> Hi Bjorn,
> 
> On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > [+cc Vaibhav]
> > > >
> > > > Alternate less redundant subject:
> > > >
> > > >   PCI: rcar: Add suspend/resume support
> > >
> > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3
> > > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI).
> > > People tend to use the prefix "PCI: rcar: " for both :-(
> >
> > Yeah, that's pretty broken, thanks for pointing this out!
> >
> > For most drivers we use a chipset name ("keystone", "imx6", "tegra",
> > etc) as the changlog tag.  That's nice because it gives space for
> > multiple drivers from the same vendor, but I don't know anything
> > similarly specific for the R-Car drivers.
> >
> > pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI
> 
> AFAIUI it's some internal PCI glue to the *HCI USB controller.
> 
> > bus?  The "gen2" is confusing because "Gen 2" is more commonly used
> > for PCIe than for Conventional PCI.
> 
> The "Gen2" applies to "R-Car", not to "PCI".

Wicked :) !

> > I would propose keeping "rcar" for the PCIe driver and using
> > "rcar-pci" for the Conventional PCI one, but the Conventional PCI one
> 
> (/me resists against bike-shedding)

I'd agree with Bjorn - I don't know, internal vs external seems
artificial. Certainly gen2 is misleading, it does not take much
to improve it.

> > (pci-rcar-gen2.c) seems pretty inactive.  The most recent commits are
> > from 2018, and they're trivial cleanups.  So I'm doubtful that anybody
> > will remember when the next change comes in.
> 
> I guess pci-rcar-gen2.c is simpler and more mature ;-)
> R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers
> can be used together on the same hardware.

I'd remove gen2 to start with, you are better placed to know the
internals to come up with something significant.

Lorenzo
Geert Uytterhoeven April 28, 2020, 8:33 a.m. UTC | #10
Hi Lorenzo,

On Tue, Apr 28, 2020 at 10:26 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Mon, Apr 27, 2020 at 10:08:52PM +0200, Geert Uytterhoeven wrote:
> > On Mon, Apr 27, 2020 at 7:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Sat, Apr 25, 2020 at 10:55:21AM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, Apr 24, 2020 at 9:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > [+cc Vaibhav]
> > > > >
> > > > > Alternate less redundant subject:
> > > > >
> > > > >   PCI: rcar: Add suspend/resume support
> > > >
> > > > Note that there's both pcie-rcar.c (this driver, for R-Car Gen2 and Gen3
> > > > PCIe) and pci-rcar-gen2.c (for R-Car Gen2 PCI).
> > > > People tend to use the prefix "PCI: rcar: " for both :-(
> > >
> > > Yeah, that's pretty broken, thanks for pointing this out!
> > >
> > > For most drivers we use a chipset name ("keystone", "imx6", "tegra",
> > > etc) as the changlog tag.  That's nice because it gives space for
> > > multiple drivers from the same vendor, but I don't know anything
> > > similarly specific for the R-Car drivers.
> > >
> > > pci-rcar-gen2.c seems to be for some sort of internal Conventional PCI
> >
> > AFAIUI it's some internal PCI glue to the *HCI USB controller.
> >
> > > bus?  The "gen2" is confusing because "Gen 2" is more commonly used
> > > for PCIe than for Conventional PCI.
> >
> > The "Gen2" applies to "R-Car", not to "PCI".
>
> Wicked :) !

pcie-rcar.c supports R-Car Gen1, Gen2, and Gen3.

> > > I would propose keeping "rcar" for the PCIe driver and using
> > > "rcar-pci" for the Conventional PCI one, but the Conventional PCI one
> >
> > (/me resists against bike-shedding)
>
> I'd agree with Bjorn - I don't know, internal vs external seems
> artificial. Certainly gen2 is misleading, it does not take much
> to improve it.

We have lots of drivers in other subsystems with "rcar-gen2" or
"rcar-gen3" as part of their names.

> > > (pci-rcar-gen2.c) seems pretty inactive.  The most recent commits are
> > > from 2018, and they're trivial cleanups.  So I'm doubtful that anybody
> > > will remember when the next change comes in.
> >
> > I guess pci-rcar-gen2.c is simpler and more mature ;-)
> > R-Car Gen2 SoCs have both (internal) PCI and PCIe, so the two drivers
> > can be used together on the same hardware.
>
> I'd remove gen2 to start with, you are better placed to know the
> internals to come up with something significant.

So we're back at "PCI: rcar: ...", for both ;-)

I'd say the main difference between the two drivers is PCI vs. PCIe.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 759c6542c5c8..f86513638b8a 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -452,6 +452,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, win);
+			i++;
+			break;
+		}
+	}
+}
+
 static int rcar_pcie_enable(struct rcar_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -891,11 +917,25 @@  static void rcar_pcie_unmap_msi(struct rcar_pcie *pcie)
 	irq_domain_remove(msi->domain);
 }
 
+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, lower_32_bits(base) | MSIFE, PCIEMSIALR);
+	rcar_pci_write_reg(pcie, upper_32_bits(base), 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;
-	phys_addr_t base;
 	int err, i;
 
 	mutex_init(&msi->lock);
@@ -934,17 +974,7 @@  static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
 
 	/* setup MSI data target */
 	msi->pages = __get_free_pages(GFP_KERNEL, 0);
-	if (!msi->pages) {
-		err = -ENOMEM;
-		goto err;
-	}
-	base = virt_to_phys((void *)msi->pages);
-
-	rcar_pci_write_reg(pcie, lower_32_bits(base) | MSIFE, PCIEMSIALR);
-	rcar_pci_write_reg(pcie, upper_32_bits(base), PCIEMSIAUR);
-
-	/* enable all MSI interrupts */
-	rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
+	rcar_pcie_hw_enable_msi(pcie);
 
 	return 0;
 
@@ -1219,6 +1249,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);
+	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);
@@ -1234,6 +1295,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,
 };