Linux-Clk Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support
@ 2018-11-23  9:44 Miquel Raynal
  2018-11-23  9:44 ` [PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock Miquel Raynal
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Miquel Raynal @ 2018-11-23  9:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock
  2018-11-23  9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
@ 2018-11-23  9:44 ` Miquel Raynal
  2018-11-23  9:44 ` [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time Miquel Raynal
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Miquel Raynal @ 2018-11-23  9:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
  2018-11-23  9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
  2018-11-23  9:44 ` [PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock Miquel Raynal
@ 2018-11-23  9:44 ` Miquel Raynal
  2018-11-30  0:37   ` Stephen Boyd
  2018-11-23  9:44 ` [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name Miquel Raynal
  2018-11-23  9:44 ` [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock Miquel Raynal
  3 siblings, 1 reply; 11+ messages in thread
From: Miquel Raynal @ 2018-11-23  9:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name
  2018-11-23  9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
  2018-11-23  9:44 ` [PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock Miquel Raynal
  2018-11-23  9:44 ` [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time Miquel Raynal
@ 2018-11-23  9:44 ` Miquel Raynal
  2018-12-11 21:35   ` Rob Herring
  2018-11-23  9:44 ` [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock Miquel Raynal
  3 siblings, 1 reply; 11+ messages in thread
From: Miquel Raynal @ 2018-11-23  9:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock
  2018-11-23  9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
                   ` (2 preceding siblings ...)
  2018-11-23  9:44 ` [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name Miquel Raynal
@ 2018-11-23  9:44 ` Miquel Raynal
  2018-12-11 21:36   ` Rob Herring
  3 siblings, 1 reply; 11+ messages in thread
From: Miquel Raynal @ 2018-11-23  9:44 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
  2018-11-23  9:44 ` [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time Miquel Raynal
@ 2018-11-30  0:37   ` Stephen Boyd
  2018-11-30 10:58     ` Miquel Raynal
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2018-11-30  0:37 UTC (permalink / raw)
  To: Mark Rutland, Michael Turquette, Miquel Raynal, Rob Herring
  Cc: linux-clk, devicetree, Thomas Petazzoni, Antoine Tenart,
	Gregory Clement, Maxime Chevallier, Nadav Haklai, Miquel Raynal

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
  2018-11-30  0:37   ` Stephen Boyd
@ 2018-11-30 10:58     ` Miquel Raynal
  2018-12-12 21:59       ` Bjorn Helgaas
  2019-01-11 13:44       ` Miquel Raynal
  0 siblings, 2 replies; 11+ messages in thread
From: Miquel Raynal @ 2018-11-30 10:58 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Mark Rutland, Michael Turquette, Rob Herring, linux-clk,
	devicetree, Thomas Petazzoni, Antoine Tenart, Gregory Clement,
	Maxime Chevallier, Nadav Haklai, Bjorn Helgaas

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name
  2018-11-23  9:44 ` [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name Miquel Raynal
@ 2018-12-11 21:35   ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2018-12-11 21:35 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Michael Turquette, Stephen Boyd, Mark Rutland, linux-clk,
	devicetree, Thomas Petazzoni, Antoine Tenart, Gregory Clement,
	Maxime Chevallier, Nadav Haklai, Miquel Raynal

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock
  2018-11-23  9:44 ` [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock Miquel Raynal
@ 2018-12-11 21:36   ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2018-12-11 21:36 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Michael Turquette, Stephen Boyd, Mark Rutland, linux-clk,
	devicetree, Thomas Petazzoni, Antoine Tenart, Gregory Clement,
	Maxime Chevallier, Nadav Haklai, Miquel Raynal

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
  2018-11-30 10:58     ` Miquel Raynal
@ 2018-12-12 21:59       ` Bjorn Helgaas
  2019-01-11 13:44       ` Miquel Raynal
  1 sibling, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2018-12-12 21:59 UTC (permalink / raw)
  To: miquel.raynal
  Cc: sboyd, Mark Rutland, mturquette, Rob Herring, linux-clk,
	devicetree, thomas.petazzoni, antoine.tenart, gregory.clement,
	maxime.chevallier, Nadav Haklai, Rafael J. Wysocki,
	Linux PM list

[+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" :)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
  2018-11-30 10:58     ` Miquel Raynal
  2018-12-12 21:59       ` Bjorn Helgaas
@ 2019-01-11 13:44       ` Miquel Raynal
  1 sibling, 0 replies; 11+ messages in thread
From: Miquel Raynal @ 2019-01-11 13:44 UTC (permalink / raw)
  To: Stephen Boyd, Bjorn Helgaas
  Cc: Mark Rutland, Michael Turquette, Rob Herring, linux-clk,
	devicetree, Thomas Petazzoni, Antoine Tenart, Gregory Clement,
	Maxime Chevallier, Nadav Haklai

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23  9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
2018-11-23  9:44 ` [PATCH 1/4] clk: mvebu: armada-37xx-periph: add PCIe gated clock Miquel Raynal
2018-11-23  9:44 ` [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time Miquel Raynal
2018-11-30  0:37   ` Stephen Boyd
2018-11-30 10:58     ` Miquel Raynal
2018-12-12 21:59       ` Bjorn Helgaas
2019-01-11 13:44       ` Miquel Raynal
2018-11-23  9:44 ` [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name Miquel Raynal
2018-12-11 21:35   ` Rob Herring
2018-11-23  9:44 ` [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock Miquel Raynal
2018-12-11 21:36   ` Rob Herring

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox