* [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
` (4 more replies)
0 siblings, 5 replies; 15+ 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] 15+ 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
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ 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 related [flat|nested] 15+ 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
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ 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 related [flat|nested] 15+ 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
2019-02-25 15:05 ` [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
4 siblings, 1 reply; 15+ 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 related [flat|nested] 15+ 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
2019-02-25 15:05 ` [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
4 siblings, 1 reply; 15+ 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 related [flat|nested] 15+ 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
0 siblings, 0 replies; 15+ 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
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] 15+ messages in thread
* Re: [PATCH 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time
@ 2018-11-30 0:37 ` Stephen Boyd
0 siblings, 0 replies; 15+ 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] 15+ 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
-1 siblings, 2 replies; 15+ 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] 15+ 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; 15+ 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
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] 15+ messages in thread
* Re: [PATCH 3/4] dt-bindings: clk: armada3700: fix typo in SoC name
@ 2018-12-11 21:35 ` Rob Herring
0 siblings, 0 replies; 15+ 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] 15+ 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; 15+ 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
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] 15+ messages in thread
* Re: [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock
@ 2018-12-11 21:36 ` Rob Herring
0 siblings, 0 replies; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread
* Re: [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support
2018-11-23 9:44 [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
` (3 preceding siblings ...)
2018-11-23 9:44 ` [PATCH 4/4] dt-bindings: clk: armada3700: document the PCIe clock Miquel Raynal
@ 2019-02-25 15:05 ` Miquel Raynal
4 siblings, 0 replies; 15+ messages in thread
From: Miquel Raynal @ 2019-02-25 15:05 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
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-02-25 15:05 UTC | newest]
Thread overview: 15+ 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 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-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
2018-12-11 21:36 ` Rob Herring
2019-02-25 15:05 ` [PATCH 0/4] Prepare Armada 3700 PCIe suspend to RAM support Miquel Raynal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.