* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-03-14 19:12 [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver marek.vasut
@ 2020-03-20 10:12 ` Lorenzo Pieralisi
2020-04-26 12:33 ` Marek Vasut
2020-04-24 11:54 ` Lorenzo Pieralisi
2020-04-24 19:57 ` Bjorn Helgaas
2 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Pieralisi @ 2020-03-20 10:12 UTC (permalink / raw)
To: marek.vasut
Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
linux-renesas-soc
On 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-03-20 10:12 ` Lorenzo Pieralisi
@ 2020-04-26 12:33 ` Marek Vasut
0 siblings, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2020-04-26 12:33 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
linux-renesas-soc
On 3/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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-03-14 19:12 [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver marek.vasut
2020-03-20 10:12 ` Lorenzo Pieralisi
@ 2020-04-24 11:54 ` Lorenzo Pieralisi
2020-04-24 19:57 ` Bjorn Helgaas
2 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2020-04-24 11:54 UTC (permalink / raw)
To: marek.vasut
Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
linux-renesas-soc
On 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-03-14 19:12 [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver marek.vasut
2020-03-20 10:12 ` Lorenzo Pieralisi
2020-04-24 11:54 ` Lorenzo Pieralisi
@ 2020-04-24 19:57 ` Bjorn Helgaas
2020-04-25 8:55 ` Geert Uytterhoeven
2020-04-26 12:32 ` Marek Vasut
2 siblings, 2 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2020-04-24 19:57 UTC (permalink / raw)
To: marek.vasut
Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
linux-renesas-soc, Vaibhav Gupta
[+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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-24 19:57 ` Bjorn Helgaas
@ 2020-04-25 8:55 ` Geert Uytterhoeven
2020-04-27 17:41 ` Bjorn Helgaas
2020-04-26 12:32 ` Marek Vasut
1 sibling, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2020-04-25 8:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
Linux-Renesas, Vaibhav Gupta
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-25 8:55 ` Geert Uytterhoeven
@ 2020-04-27 17:41 ` Bjorn Helgaas
2020-04-27 20:08 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2020-04-27 17:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
Linux-Renesas, Vaibhav Gupta, Lorenzo Pieralisi
[+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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-27 17:41 ` Bjorn Helgaas
@ 2020-04-27 20:08 ` Geert Uytterhoeven
2020-04-28 8:26 ` Lorenzo Pieralisi
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2020-04-27 20:08 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
Linux-Renesas, Vaibhav Gupta, Lorenzo Pieralisi
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-27 20:08 ` Geert Uytterhoeven
@ 2020-04-28 8:26 ` Lorenzo Pieralisi
2020-04-28 8:33 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Pieralisi @ 2020-04-28 8:26 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Bjorn Helgaas, Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami,
Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman,
Wolfram Sang, Linux-Renesas, Vaibhav Gupta
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-28 8:26 ` Lorenzo Pieralisi
@ 2020-04-28 8:33 ` Geert Uytterhoeven
0 siblings, 0 replies; 11+ messages in thread
From: Geert Uytterhoeven @ 2020-04-28 8:33 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Bjorn Helgaas, Marek Vasut, linux-pci, Kazufumi Ikeda, Gaku Inami,
Marek Vasut, Geert Uytterhoeven, Phil Edworthy, Simon Horman,
Wolfram Sang, Linux-Renesas, Vaibhav Gupta
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
2020-04-24 19:57 ` Bjorn Helgaas
2020-04-25 8:55 ` Geert Uytterhoeven
@ 2020-04-26 12:32 ` Marek Vasut
1 sibling, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2020-04-26 12:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Kazufumi Ikeda, Gaku Inami, Marek Vasut,
Geert Uytterhoeven, Phil Edworthy, Simon Horman, Wolfram Sang,
linux-renesas-soc, Vaibhav Gupta
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
^ permalink raw reply [flat|nested] 11+ messages in thread