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

[PATCH v3 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

Changes in v3:
==============
* Fixed one typo: s/their register/their registers/.

Changes in v2:
==============
* Rebased on top of v5.2-rc1.
* Added Rob's R-by tags.
* No change on the "change suspend/resume time" patch as, despite my
  pings, I got no answer and IMHO the proposed approach is entirely
  valid.


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 v3 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 4c20093a42fb..1e18c5a875bd 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -304,6 +304,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),
@@ -319,6 +320,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 v3 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 registers 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 1e18c5a875bd..bee45e43a85f 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -715,8 +715,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

[PATCH v3 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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

[PATCH v3 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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 v3 0/4] Prepare Armada 3700 PCIe suspend to RAM support

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Tue, 17 Sep 2019 10:31:53
-0700:

> Quoting Miquel Raynal (2019-06-27 05:52:41)
> > 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.
> >   
> 
> Should I drop this patch series?
> 

No, if it is right for you I would really prefer to have it merged
(sorry for the delay in answering though) because it will be still
needed, no matter how clock dependencies are handled.


Thanks,
Miquèl

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

[Stephen Boyd]

Quoting Miquel Raynal (2019-09-20 01:03:01)
> Hi Stephen,
> 
> Stephen Boyd <sboyd@kernel.org> wrote on Tue, 17 Sep 2019 10:31:53
> -0700:
> 
> > Quoting Miquel Raynal (2019-06-27 05:52:41)
> > > 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.
> > >   
> > 
> > Should I drop this patch series?
> > 
> 
> No, if it is right for you I would really prefer to have it merged
> (sorry for the delay in answering though) because it will be still
> needed, no matter how clock dependencies are handled.
> 
> 

Ok. I'll apply it after the merge window. Let me know if it's more
urgent than that.

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

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@kernel.org> wrote on Fri, 20 Sep 2019 09:54:01
-0700:

> Quoting Miquel Raynal (2019-09-20 01:03:01)
> > Hi Stephen,
> > 
> > Stephen Boyd <sboyd@kernel.org> wrote on Tue, 17 Sep 2019 10:31:53
> > -0700:
> >   
> > > Quoting Miquel Raynal (2019-06-27 05:52:41)  
> > > > 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.
> > > >     
> > > 
> > > Should I drop this patch series?
> > >   
> > 
> > No, if it is right for you I would really prefer to have it merged
> > (sorry for the delay in answering though) because it will be still
> > needed, no matter how clock dependencies are handled.
> > 
> >   
> 
> Ok. I'll apply it after the merge window. Let me know if it's more
> urgent than that.
> 

No it's not, 5.5 is fine.


Thanks,
Miquèl