[PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support

[PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support

[Miquel Raynal]

Hello,

As part of an effort to bring suspend to RAM support to the Armada
3700 SoC (main target: ESPRESSObin board), there are small things to
do in the Armada 3700 peripherals clock driver:

* On this SoC, the PCIe controller gets fed by a gated clock in the
  south bridge. This clock is missing in the current driver, patch 1
  adds it.

* Because of a constraint in the PCI core, the resume function of a
  PCIe controller driver must be run at an early stage
  (->suspend/resume_noirq()), before the core tries to ->read/write()
  in the PCIe registers to do more configuration. Hence, the PCIe
  clock must be resumed before. This is enforced thanks to two
  changes:
  1/ Add device links to the clock framework. This enforce order in
     the PM core: the clocks are resumed before the consumers. Series
     has been posted, see [1].
  2/ Even with the above feature, the clock's resume() callback is
     called after the PCI controller's resume_noirq() callback. The
     only way to fix this is to change the "priority" of the clock
     suspend/resume callbacks. This is done in patch 2.

* The bindings are updated with the PCI clock in patch 4 while patch 3
  is just a typo correction in the same file.

If there is anything unclear please feel free to ask.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html

Thanks,
Miquèl


Miquel Raynal (4):
  clk: mvebu: armada-37xx-periph: add PCIe gated clock
  clk: mvebu: armada-37xx-periph: change suspend/resume time
  dt-bindings: clk: armada3700: fix typo in SoC name
  dt-bindings: clk: armada3700: document the PCIe clock

 .../devicetree/bindings/clock/armada3700-periph-clock.txt   | 5 +++--
 drivers/clk/mvebu/armada-37xx-periph.c                      | 6 ++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.19.1

[PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock

[Miquel Raynal]

The PCIe clock is a gated clock which has the same source as GbE0
(both IPs share a set of registers). This source clock is called
'gbe_core' in the driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 1f1cff428d78..43198fb1efa4 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -302,6 +302,7 @@ PERIPH_CLK_GATE_DIV(gbe_bm, 12, DIV_SEL1, 0, clk_table1);
 PERIPH_CLK_FULL_DD(sdio, 11, 14, DIV_SEL0, DIV_SEL0, 3, 6);
 PERIPH_CLK_FULL_DD(usb32_usb2_sys, 16, 16, DIV_SEL0, DIV_SEL0, 9, 12);
 PERIPH_CLK_FULL_DD(usb32_ss_sys, 17, 18, DIV_SEL0, DIV_SEL0, 15, 18);
+static PERIPH_GATE(pcie, 14);
 
 static struct clk_periph_data data_sb[] = {
 	REF_CLK_MUX_DD(gbe_50),
@@ -317,6 +318,7 @@ static struct clk_periph_data data_sb[] = {
 	REF_CLK_FULL_DD(sdio),
 	REF_CLK_FULL_DD(usb32_usb2_sys),
 	REF_CLK_FULL_DD(usb32_ss_sys),
+	REF_CLK_GATE(pcie, "gbe_core"),
 	{ },
 };
 
-- 
2.19.1

[PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Miquel Raynal]

Armada 3700 PCIe IP relies on the PCIe clock managed by this
driver. For reasons related to the PCI core's organization when
suspending/resuming, PCI host controller drivers must reconfigure
their register at suspend_noirq()/resume_noirq() which happens after
suspend()/suspend_late() and before resume_early()/resume().

Device link support in the clock framework enforce that the clock
driver's resume() callback will be called before the PCIe
driver's. But, any resume_noirq() callback will be called before all
the registered resume() callbacks.

The solution to support PCIe resume operation is to change the
"priority" of this clock driver PM callbacks to "_noirq()".

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 43198fb1efa4..0b4be1ce6f8b 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -713,8 +713,8 @@ static int __maybe_unused armada_3700_periph_clock_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops armada_3700_periph_clock_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend,
-				armada_3700_periph_clock_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend,
+				      armada_3700_periph_clock_resume)
 };
 
 static int armada_3700_periph_clock_probe(struct platform_device *pdev)
-- 
2.19.1

Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Stephen Boyd]

Quoting Miquel Raynal (2018-11-23 01:44:41)
> Armada 3700 PCIe IP relies on the PCIe clock managed by this
> driver. For reasons related to the PCI core's organization when
> suspending/resuming, PCI host controller drivers must reconfigure
> their register at suspend_noirq()/resume_noirq() which happens after
> suspend()/suspend_late() and before resume_early()/resume().
> 
> Device link support in the clock framework enforce that the clock
> driver's resume() callback will be called before the PCIe
> driver's. But, any resume_noirq() callback will be called before all
> the registered resume() callbacks.

I thought any device driver that provides something to another device
driver will implicitly be probed before the driver that consumes said
resources. And we actually reorder the dpm list on probe defer so that
the order of devices is correct. Is it more that we want to parallelize
suspend/resume of the PCIe chip so we need to have device links so that
we know the dependency of the PCIe driver on the clock driver?

> 
> The solution to support PCIe resume operation is to change the
> "priority" of this clock driver PM callbacks to "_noirq()".

This seems sad that the PM core can't "priority boost" any
suspend/resume callbacks of a device that doesn't have noirq callbacks
when a device that depends on it from the device link perspective does
have noirq callbacks. And why does the PCIe device need to use noirq
callbacks in general?

I'm just saying this seems like a more fundamental problem with ordering
of provider and consumer suspend/resume functions that isn't being
solved in this patch. In fact, it's quite the opposite, this is working
around the problem.

Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Miquel Raynal]

Hi Stephen,

+ Bjorn to help us on PCI suspend/resume questions.

Stephen Boyd <sboyd@kernel.org> wrote on Thu, 29 Nov 2018 16:37:58
-0800:

> Quoting Miquel Raynal (2018-11-23 01:44:41)
> > Armada 3700 PCIe IP relies on the PCIe clock managed by this
> > driver. For reasons related to the PCI core's organization when
> > suspending/resuming, PCI host controller drivers must reconfigure
> > their register at suspend_noirq()/resume_noirq() which happens after
> > suspend()/suspend_late() and before resume_early()/resume().
> > 
> > Device link support in the clock framework enforce that the clock
> > driver's resume() callback will be called before the PCIe
> > driver's. But, any resume_noirq() callback will be called before all
> > the registered resume() callbacks.  
> 
> I thought any device driver that provides something to another device
> driver will implicitly be probed before the driver that consumes said
> resources. And we actually reorder the dpm list on probe defer so that
> the order of devices is correct. Is it more that we want to parallelize
> suspend/resume of the PCIe chip so we need to have device links so that
> we know the dependency of the PCIe driver on the clock driver?

I had the same idea of device links before testing. I hope I did
not make any mistake leading me to wrong observations, but indeed
this is what I think is happening:
* PM core call all suspend() callbacks
* then all suspend_late()
* then all suspend_noirq()
For me, the PM core does not care if a suspend_noirq() depends on the
suspend() of another driver.

I hope I did not miss anything.

> 
> > 
> > The solution to support PCIe resume operation is to change the
> > "priority" of this clock driver PM callbacks to "_noirq()".  
> 
> This seems sad that the PM core can't "priority boost" any
> suspend/resume callbacks of a device that doesn't have noirq callbacks
> when a device that depends on it from the device link perspective does
> have noirq callbacks.

I do agree on this but I'm not sure it would work. I suppose the
"noirq" state is a global state and thus code in regular suspend()
callbacks could probably fail to run in a "noirq" context?

> And why does the PCIe device need to use noirq callbacks in general?

I would like Bjorn to confirm this, but there is this commit that could
explain the situation:

commit ab14d45ea58eae67c739e4ba01871cae7b6c4586
Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date:   Tue Mar 17 15:55:45 2015 +0100

    PCI: mvebu: Add suspend/resume support
    
    Add suspend/resume support for the mvebu PCIe host driver.  Without this
    commit, the system will panic at resume time when PCIe devices are
    connected.
    
    Note that we have to use the ->suspend_noirq() and ->resume_noirq() hooks,
    because at resume time, the PCI fixups are done at ->resume_noirq() time,
    so the PCIe controller has to be ready at this point.
    
    Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Jason Cooper <jason@lakedaemon.net>

> 
> I'm just saying this seems like a more fundamental problem with ordering
> of provider and consumer suspend/resume functions that isn't being
> solved in this patch. In fact, it's quite the opposite, this is working
> around the problem.
> 

I do agree with your point, but I would not be confident tweaking the PM
core's scheduling "alone" :)


Thanks,
Miquèl

Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Bjorn Helgaas]

[+cc Rafael, linux-pm, since they know a lot more about PCI PM than I
do.  Sorry I didn't notice they weren't cc'd earlier]

On Fri, Nov 30, 2018 at 4:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Stephen Boyd <sboyd@kernel.org> wrote on Thu, 29 Nov 2018 16:37:58
> -0800:
> > Quoting Miquel Raynal (2018-11-23 01:44:41)
> > > Armada 3700 PCIe IP relies on the PCIe clock managed by this
> > > driver. For reasons related to the PCI core's organization when
> > > suspending/resuming, PCI host controller drivers must reconfigure
> > > their register at suspend_noirq()/resume_noirq() which happens after
> > > suspend()/suspend_late() and before resume_early()/resume().
> > >
> > > Device link support in the clock framework enforce that the clock
> > > driver's resume() callback will be called before the PCIe
> > > driver's. But, any resume_noirq() callback will be called before all
> > > the registered resume() callbacks.
> >
> > I thought any device driver that provides something to another device
> > driver will implicitly be probed before the driver that consumes said
> > resources. And we actually reorder the dpm list on probe defer so that
> > the order of devices is correct. Is it more that we want to parallelize
> > suspend/resume of the PCIe chip so we need to have device links so that
> > we know the dependency of the PCIe driver on the clock driver?
>
> I had the same idea of device links before testing. I hope I did
> not make any mistake leading me to wrong observations, but indeed
> this is what I think is happening:
> * PM core call all suspend() callbacks
> * then all suspend_late()
> * then all suspend_noirq()
> For me, the PM core does not care if a suspend_noirq() depends on the
> suspend() of another driver.
>
> I hope I did not miss anything.
>
> > > The solution to support PCIe resume operation is to change the
> > > "priority" of this clock driver PM callbacks to "_noirq()".
> >
> > This seems sad that the PM core can't "priority boost" any
> > suspend/resume callbacks of a device that doesn't have noirq callbacks
> > when a device that depends on it from the device link perspective does
> > have noirq callbacks.
>
> I do agree on this but I'm not sure it would work. I suppose the
> "noirq" state is a global state and thus code in regular suspend()
> callbacks could probably fail to run in a "noirq" context?
>
> > And why does the PCIe device need to use noirq callbacks in general?
>
> I would like Bjorn to confirm this, but there is this commit that could
> explain the situation:
>
> commit ab14d45ea58eae67c739e4ba01871cae7b6c4586
> Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Date:   Tue Mar 17 15:55:45 2015 +0100
>
>     PCI: mvebu: Add suspend/resume support
>
>     Add suspend/resume support for the mvebu PCIe host driver.  Without this
>     commit, the system will panic at resume time when PCIe devices are
>     connected.
>
>     Note that we have to use the ->suspend_noirq() and ->resume_noirq() hooks,
>     because at resume time, the PCI fixups are done at ->resume_noirq() time,
>     so the PCIe controller has to be ready at this point.
>
>     Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Acked-by: Jason Cooper <jason@lakedaemon.net>
>
> > I'm just saying this seems like a more fundamental problem with ordering
> > of provider and consumer suspend/resume functions that isn't being
> > solved in this patch. In fact, it's quite the opposite, this is working
> > around the problem.
>
> I do agree with your point, but I would not be confident tweaking the PM
> core's scheduling "alone" :)

Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Miquel Raynal]

Hi Bjorn and Stephen

Gentle ping about the below debate, I hope you will have enough
context to answer, please ask if something is unclear.

The question here about "shall we upgrade the clocks S2RAM callbacks to
the NOIRQ phase" to be handled before/after the PCIe callbacks (which
also are in the NOIRQ phase) also applies to the pinctrl driver so I am
really interested to know why this would not be a valid solution.

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Fri, 30
Nov 2018 11:58:21 +0100:

> Hi Stephen,
> 
> + Bjorn to help us on PCI suspend/resume questions.
> 
> Stephen Boyd <sboyd@kernel.org> wrote on Thu, 29 Nov 2018 16:37:58
> -0800:
> 
> > Quoting Miquel Raynal (2018-11-23 01:44:41)  
> > > Armada 3700 PCIe IP relies on the PCIe clock managed by this
> > > driver. For reasons related to the PCI core's organization when
> > > suspending/resuming, PCI host controller drivers must reconfigure
> > > their register at suspend_noirq()/resume_noirq() which happens after
> > > suspend()/suspend_late() and before resume_early()/resume().
> > > 
> > > Device link support in the clock framework enforce that the clock
> > > driver's resume() callback will be called before the PCIe
> > > driver's. But, any resume_noirq() callback will be called before all
> > > the registered resume() callbacks.    
> > 
> > I thought any device driver that provides something to another device
> > driver will implicitly be probed before the driver that consumes said
> > resources. And we actually reorder the dpm list on probe defer so that
> > the order of devices is correct. Is it more that we want to parallelize
> > suspend/resume of the PCIe chip so we need to have device links so that
> > we know the dependency of the PCIe driver on the clock driver?  
> 
> I had the same idea of device links before testing. I hope I did
> not make any mistake leading me to wrong observations, but indeed
> this is what I think is happening:
> * PM core call all suspend() callbacks
> * then all suspend_late()
> * then all suspend_noirq()
> For me, the PM core does not care if a suspend_noirq() depends on the
> suspend() of another driver.
> 
> I hope I did not miss anything.
> 
> >   
> > > 
> > > The solution to support PCIe resume operation is to change the
> > > "priority" of this clock driver PM callbacks to "_noirq()".    
> > 
> > This seems sad that the PM core can't "priority boost" any
> > suspend/resume callbacks of a device that doesn't have noirq callbacks
> > when a device that depends on it from the device link perspective does
> > have noirq callbacks.  
> 
> I do agree on this but I'm not sure it would work. I suppose the
> "noirq" state is a global state and thus code in regular suspend()
> callbacks could probably fail to run in a "noirq" context?
> 
> > And why does the PCIe device need to use noirq callbacks in general?  
> 
> I would like Bjorn to confirm this, but there is this commit that could
> explain the situation:
> 
> commit ab14d45ea58eae67c739e4ba01871cae7b6c4586
> Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Date:   Tue Mar 17 15:55:45 2015 +0100
> 
>     PCI: mvebu: Add suspend/resume support
>     
>     Add suspend/resume support for the mvebu PCIe host driver.  Without this
>     commit, the system will panic at resume time when PCIe devices are
>     connected.
>     
>     Note that we have to use the ->suspend_noirq() and ->resume_noirq() hooks,
>     because at resume time, the PCI fixups are done at ->resume_noirq() time,
>     so the PCIe controller has to be ready at this point.
>     
>     Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Acked-by: Jason Cooper <jason@lakedaemon.net>
> 
> > 
> > I'm just saying this seems like a more fundamental problem with ordering
> > of provider and consumer suspend/resume functions that isn't being
> > solved in this patch. In fact, it's quite the opposite, this is working
> > around the problem.
> >   
> 
> I do agree with your point, but I would not be confident tweaking the PM
> core's scheduling "alone" :)
> 

Thanks,
Miquèl

[PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name

[Miquel Raynal]

This documentation is about Armada 3700 SoCs.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/clock/armada3700-periph-clock.txt     | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
index 1e3370ba189f..85972715e593 100644
--- a/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
+++ b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
@@ -9,7 +9,7 @@ bridge.
 The peripheral clock consumer should specify the desired clock by
 having the clock ID in its "clocks" phandle cell.
 
-The following is a list of provided IDs for Armada 370 North bridge clocks:
+The following is a list of provided IDs for Armada 3700 North bridge clocks:
 ID	Clock name	Description
 -----------------------------------
 0	mmc		MMC controller
@@ -30,7 +30,7 @@ ID	Clock name	Description
 15	eip97		EIP 97
 16	cpu		CPU
 
-The following is a list of provided IDs for Armada 370 South bridge clocks:
+The following is a list of provided IDs for Armada 3700 South bridge clocks:
 ID	Clock name	Description
 -----------------------------------
 0	gbe-50		50 MHz parent clock for Gigabit Ethernet
-- 
2.19.1

Re: [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name

[Rob Herring]

On Fri, 23 Nov 2018 10:44:42 +0100, Miquel Raynal wrote:
> This documentation is about Armada 3700 SoCs.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/clock/armada3700-periph-clock.txt     | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock

[Miquel Raynal]

Add a reference to the missing PCIe clock managed by this IP. The
clock resides in the south bridge.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/clock/armada3700-periph-clock.txt        | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
index 85972715e593..fbf58c443c04 100644
--- a/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
+++ b/Documentation/devicetree/bindings/clock/armada3700-periph-clock.txt
@@ -46,6 +46,7 @@ ID	Clock name	Description
 10	sdio		SDIO
 11	usb32-sub2-sys	USB 2 clock
 12	usb32-ss-sys	USB 3 clock
+13	pcie		PCIe controller
 
 Required properties:
 
-- 
2.19.1

Re: [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock

[Rob Herring]

On Fri, 23 Nov 2018 10:44:43 +0100, Miquel Raynal wrote:
> Add a reference to the missing PCIe clock managed by this IP. The
> clock resides in the south bridge.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/clock/armada3700-periph-clock.txt        | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support

[Miquel Raynal]

Hi Stephen,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Fri, 23 Nov 2018
10:44:39 +0100:

> Hello,
> 
> As part of an effort to bring suspend to RAM support to the Armada
> 3700 SoC (main target: ESPRESSObin board), there are small things to
> do in the Armada 3700 peripherals clock driver:
> 
> * On this SoC, the PCIe controller gets fed by a gated clock in the
>   south bridge. This clock is missing in the current driver, patch 1
>   adds it.
> 
> * Because of a constraint in the PCI core, the resume function of a
>   PCIe controller driver must be run at an early stage
>   (->suspend/resume_noirq()), before the core tries to ->read/write()
>   in the PCIe registers to do more configuration. Hence, the PCIe
>   clock must be resumed before. This is enforced thanks to two
>   changes:
>   1/ Add device links to the clock framework. This enforce order in
>      the PM core: the clocks are resumed before the consumers. Series
>      has been posted, see [1].
>   2/ Even with the above feature, the clock's resume() callback is
>      called after the PCI controller's resume_noirq() callback. The
>      only way to fix this is to change the "priority" of the clock
>      suspend/resume callbacks. This is done in patch 2.
> 
> * The bindings are updated with the PCI clock in patch 4 while patch 3
>   is just a typo correction in the same file.
> 
> If there is anything unclear please feel free to ask.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html

This series does not depend on the clock device links work so I
wonder if you are still considering it?


Thanks,
Miquèl