2018-11-24 08:36:45

by Miquel Raynal

Subject: [PATCH 00/12] Bring suspend to RAM support to PCIe Aardvark driver

Hello,

As part of an effort to bring suspend to RAM support to Armada 3700
SoCs (main target: ESPRESSObin), this series handles the work around
the PCIe IP.

First, more configuration is done in the 'setup' helper as inspired
from the U-Boot driver. This is needed to entirely initialize the IP
during future resume operation (patch 1).

Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As
current device trees do not provide the corresponding properties, not
finding one of these properties is not an error and just produces a
warning. However, if the property is present, an error during PHY
initialization will fail the probe of the driver.

Note: To be sure the clock will be resumed before this driver, a first
series adding links between clocks and consumers has been submitted,
see [1].

Patch 5 adds suspend/resume hooks, re-using all the above.

Finally, bindings and device trees are updated to reflect the hardware
(patch 6-12). While the clock depends on the SoC, the reset GPIO and
the PHY depends on the board so the clock is added in the
armada-37xx.dtsi file while the two other properties are added in
armada-3720-espressobin.dts.

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

Thanks,
Miquèl


Miquel Raynal (12):
PCI: aardvark: configure more registers in the configuration helper
PCI: aardvark: add reset GPIO support
PCI: aardvark: add PHY support
PCI: aardvark: add clock support
PCI: aardvark: add suspend to RAM support
dt-bindings: PCI: aardvark: describe the reset-gpios property
dt-bindings: PCI: aardvark: describe the clocks property
dt-bindings: PCI: aardvark: describe the PHY property
ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO
ARM64: dts: marvell: armada-37xx: declare PCIe clock
ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY

.../devicetree/bindings/pci/aardvark-pci.txt | 9 +
.../dts/marvell/armada-3720-espressobin.dts | 4 +
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 5 +
drivers/pci/controller/pci-aardvark.c | 214 ++++++++++++++++++
4 files changed, 232 insertions(+)

--
2.19.1

2018-11-24 08:36:50

by Miquel Raynal

Subject: [PATCH 01/12] PCI: aardvark: configure more registers in the configuration helper

Mimic U-Boot configuration to be sure all hardware registers are set
properly. This will be needed for future S2RAM operation.

Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 750081c1cb48..b95eb2aa00bb 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -100,6 +100,8 @@
#define PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE BIT(5)
#define PCIE_CORE_CTRL2_OB_WIN_ENABLE BIT(6)
#define PCIE_CORE_CTRL2_MSI_ENABLE BIT(10)
+#define PCIE_PHY_REFCLK (CONTROL_BASE_ADDR + 0x14)
+#define PCIE_PHY_REFCLK_BUF_CTRL 0x1342
#define PCIE_MSG_LOG_REG (CONTROL_BASE_ADDR + 0x30)
#define PCIE_ISR0_REG (CONTROL_BASE_ADDR + 0x40)
#define PCIE_MSG_PM_PME_MASK BIT(7)
@@ -243,6 +245,9 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
{
u32 reg;

+ /* Set HW Reference Clock Buffer Control */
+ advk_writel(pcie, PCIE_PHY_REFCLK_BUF_CTRL, PCIE_PHY_REFCLK);
+
/* Set to Direct mode */
reg = advk_readl(pcie, CTRL_CONFIG_REG);
reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
@@ -274,6 +279,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
PCIE_CORE_CTRL2_TD_ENABLE;
advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);

+ /* Set PCIe Device Control and Status 1 PF0 register */
+ reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
+ PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE;
+ advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
+
+ /* Program PCIe Control 2 to disable strict ordering */
+ reg = PCIE_CORE_CTRL2_RESERVED | PCIE_CORE_CTRL2_TD_ENABLE;
+ advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
+
/* Set GEN2 */
reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
reg &= ~PCIE_GEN_SEL_MSK;
--
2.19.1

2018-11-24 08:36:53

by Miquel Raynal

Subject: [PATCH 02/12] PCI: aardvark: add reset GPIO support

The IP supports a reset GPIO. When S2RAM will be added, we must ensure
the reset line (if any) is deasserted when resuming. Add support for
it.

Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 57 +++++++++++++++++++++++++++
1 file changed, 57 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index b95eb2aa00bb..1d31d74ddab7 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -9,6 +9,7 @@
*/

#include <linux/delay.h>
+#include <linux/gpio.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
@@ -17,6 +18,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/of_address.h>
+#include <linux/of_gpio.h>
#include <linux/of_pci.h>

#include "../pci.h"
@@ -201,6 +203,7 @@ struct advk_pcie {
u16 msi_msg;
int root_bus_nr;
struct pci_bridge_emul bridge;
+ struct gpio_desc *reset_gpio;
};

static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -973,6 +976,55 @@ static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
return err;
}

+static int advk_pcie_hard_reset(struct advk_pcie *pcie)
+{
+ if (!pcie->reset_gpio)
+ return -EINVAL;
+
+ gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+ msleep(1);
+ gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+
+ return 0;
+}
+
+static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ enum of_gpio_flags of_flags;
+ unsigned long gpio_flags;
+ int gpio_nb;
+ int ret;
+
+ gpio_nb = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
+ &of_flags);
+ if (gpio_nb == -EPROBE_DEFER)
+ return gpio_nb;
+
+ /* Old bindings miss the reset GPIO handle */
+ if (!gpio_is_valid(gpio_nb)) {
+ dev_warn(dev, "Reset GPIO unavailable\n");
+ return 0;
+ }
+
+ if (of_flags & OF_GPIO_ACTIVE_LOW)
+ gpio_flags = GPIOF_ACTIVE_LOW |
+ GPIOF_OUT_INIT_LOW;
+ else
+ gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+ ret = devm_gpio_request_one(dev, gpio_nb, gpio_flags,
+ "pcie-aardvark-reset");
+ if (ret) {
+ dev_err(dev, "Failed to retrieve reset GPIO (%d)\n", ret);
+ return ret;
+ }
+
+ pcie->reset_gpio = gpio_to_desc(gpio_nb);
+
+ return 0;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1008,6 +1060,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}

+ ret = advk_pcie_setup_reset_gpio(pcie);
+ if (ret)
+ return ret;
+
+ advk_pcie_hard_reset(pcie);
advk_pcie_setup_hw(pcie);

advk_sw_pci_bridge_init(pcie);
--
2.19.1

2018-11-24 08:36:55

by Miquel Raynal

Subject: [PATCH 04/12] PCI: aardvark: add clock support

The IP relies on a gated clock. When we will add S2RAM support, this
clock will need to be resumed before any PCIe registers are
accessed. Add support for this clock.

Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 29 +++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index da695572a2ed..108b3f15c410 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -8,6 +8,7 @@
* Author: Hezi Shahmoon <[email protected]>
*/

+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
@@ -190,6 +191,7 @@

struct advk_pcie {
struct platform_device *pdev;
+ struct clk *clk;
void __iomem *base;
struct list_head resources;
struct irq_domain *irq_domain;
@@ -1083,6 +1085,29 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
return ret;
}

+static int advk_pcie_setup_clk(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ int ret;
+
+ pcie->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pcie->clk) && (PTR_ERR(pcie->clk) == -EPROBE_DEFER))
+ return PTR_ERR(pcie->clk);
+
+ /* Old bindings miss the clock handle */
+ if (IS_ERR(pcie->clk)) {
+ dev_warn(dev, "Clock unavailable (%ld)\n", PTR_ERR(pcie->clk));
+ pcie->clk = NULL;
+ return 0;
+ }
+
+ ret = clk_prepare_enable(pcie->clk);
+ if (ret)
+ dev_err(dev, "Clock initialization failed (%d)\n", ret);
+
+ return ret;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1118,6 +1143,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}

+ ret = advk_pcie_setup_clk(pcie);
+ if (ret)
+ return ret;
+
ret = advk_pcie_setup_phy(pcie);
if (ret)
return ret;
--
2.19.1

2018-11-24 08:36:56

by Miquel Raynal

Subject: [PATCH 09/12] ARM64: dts: marvell: armada-37xx: declare PCIe reset pin

One GPIO can be muxed as PCIe reset.

Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 65bf774516ec..9f7e932c8144 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -295,6 +295,10 @@
function = "mii";
};

+ pcie_pins: pcie-pins {
+ groups = "pcie1";
+ function = "gpio";
+ };
};

eth0: ethernet@30000 {
--
2.19.1

2018-11-24 08:37:02

by Miquel Raynal

Subject: [PATCH 11/12] ARM64: dts: marvell: armada-37xx: declare PCIe clock

The PCIe IP is fed by a gated clock.

Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 9f7e932c8144..854b1d59b2ca 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -418,6 +418,7 @@
#address-cells = <3>;
#size-cells = <2>;
bus-range = <0x00 0xff>;
+ clocks = <&sb_periph_clk 13>;
interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <1>;
msi-parent = <&pcie0>;
--
2.19.1

2018-11-24 08:37:11

by Miquel Raynal

Subject: [PATCH 07/12] dt-bindings: PCI: aardvark: describe the clocks property

Describe the missing gated clock feeding the PCIe IP.

Signed-off-by: Miquel Raynal <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 252934237138..c275c3e39cde 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -12,6 +12,7 @@ contain the following properties:
- #size-cells: set to <2>
- device_type: set to "pci"
- ranges: ranges for the PCI memory and I/O regions
+ - clocks: the clock feeding the IP
- #interrupt-cells: set to <1>
- msi-controller: indicates that the PCIe controller can itself
handle MSI interrupts
@@ -41,6 +42,7 @@ Example:
#address-cells = <3>;
#size-cells = <2>;
bus-range = <0x00 0xff>;
+ clocks = <&sb_periph_clk 13>;
interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <1>;
msi-controller;
--
2.19.1

2018-11-24 08:37:12

by Miquel Raynal

Subject: [PATCH 06/12] dt-bindings: PCI: aardvark: describe the reset-gpios property

A GPIO might be used to reset the PCI IP. Describe the property needed
in this case.

Signed-off-by: Miquel Raynal <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 310ef7145c47..252934237138 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -20,6 +20,10 @@ contain the following properties:
define the mapping of the PCIe interface to interrupt numbers.
- bus-range: PCI bus numbers covered

+The following are optional properties:
+
+ - reset-gpios: GPIO to reset the device
+
In addition, the Device Tree describing an Aardvark PCIe controller
must include a sub-node that describes the legacy interrupt controller
built into the PCIe controller. This sub-node must have the following
@@ -48,6 +52,7 @@ Example:
<0 0 0 2 &pcie_intc 1>,
<0 0 0 3 &pcie_intc 2>,
<0 0 0 4 &pcie_intc 3>;
+ reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
pcie_intc: interrupt-controller {
interrupt-controller;
#interrupt-cells = <1>;
--
2.19.1

2018-11-24 08:37:12

by Miquel Raynal

Subject: [PATCH 08/12] dt-bindings: PCI: aardvark: describe the PHY property

Document the possibility to reference a PHY.

Signed-off-by: Miquel Raynal <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index c275c3e39cde..b41c69968e38 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -24,6 +24,7 @@ contain the following properties:
The following are optional properties:

- reset-gpios: GPIO to reset the device
+ - phys: the PCIe PHY handle

In addition, the Device Tree describing an Aardvark PCIe controller
must include a sub-node that describes the legacy interrupt controller
@@ -55,6 +56,7 @@ Example:
<0 0 0 3 &pcie_intc 2>,
<0 0 0 4 &pcie_intc 3>;
reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
+ phys = <&comphy1 0>;
pcie_intc: interrupt-controller {
interrupt-controller;
#interrupt-cells = <1>;
--
2.19.1

2018-11-24 08:37:21

by Miquel Raynal

Subject: [PATCH 03/12] PCI: aardvark: add PHY support

The IP needs its PHY to be properly configured to work. While the PHY
is usually already configured by the bootloader, we will need this
feature when adding S2RAM support. Take care of registering and
configuring the PHY from the driver itself.

Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 62 +++++++++++++++++++++++++++
1 file changed, 62 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 1d31d74ddab7..da695572a2ed 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -17,6 +17,7 @@
#include <linux/pci.h>
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
#include <linux/of_address.h>
#include <linux/of_gpio.h>
#include <linux/of_pci.h>
@@ -204,6 +205,7 @@ struct advk_pcie {
int root_bus_nr;
struct pci_bridge_emul bridge;
struct gpio_desc *reset_gpio;
+ struct phy *phy;
};

static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -1025,6 +1027,62 @@ static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
return 0;
}

+static void advk_pcie_disable_phy(struct advk_pcie *pcie)
+{
+ phy_power_off(pcie->phy);
+ phy_exit(pcie->phy);
+}
+
+static int advk_pcie_enable_phy(struct advk_pcie *pcie)
+{
+ int ret;
+
+ if (!pcie->phy)
+ return 0;
+
+ ret = phy_init(pcie->phy);
+ if (ret)
+ return ret;
+
+ ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
+ if (ret) {
+ phy_exit(pcie->phy);
+ return ret;
+ }
+
+ ret = phy_power_on(pcie->phy);
+ if (ret) {
+ phy_exit(pcie->phy);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int advk_pcie_setup_phy(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ struct device_node *node = dev->of_node;
+ int ret = 0;
+
+ pcie->phy = devm_of_phy_get(dev, node, NULL);
+ if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) == -EPROBE_DEFER))
+ return PTR_ERR(pcie->phy);
+
+ /* Old bindings miss the PHY handle */
+ if (IS_ERR(pcie->phy)) {
+ dev_warn(dev, "PHY unavailable (%ld)\n", PTR_ERR(pcie->phy));
+ pcie->phy = NULL;
+ return 0;
+ }
+
+ ret = advk_pcie_enable_phy(pcie);
+ if (ret)
+ dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
+
+ return ret;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1060,6 +1118,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}

+ ret = advk_pcie_setup_phy(pcie);
+ if (ret)
+ return ret;
+
ret = advk_pcie_setup_reset_gpio(pcie);
if (ret)
return ret;
--
2.19.1

2018-11-24 08:38:10

by Miquel Raynal

Subject: [PATCH 12/12] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY

The PCIe node is wired to the second PHY of the COMPHY IP.

Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 76a508da80b9..43e8e1edc467 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -49,6 +49,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pcie_pins>;
reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
+ phys = <&comphy1 0>;
};

/* J6 */
--
2.19.1

2018-11-24 08:38:18

by Miquel Raynal

Subject: [PATCH 10/12] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO

Add a reset-gpios property to the PCIe node.

Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 094994a9c68e..76a508da80b9 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -46,6 +46,9 @@
/* J9 */
&pcie0 {
status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie_pins>;
+ reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
};

/* J6 */
--
2.19.1

2018-11-24 08:38:29

by Miquel Raynal

Subject: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Add suspend and resume callbacks. The priority of these are
"_noirq()", to workaround early access to the registers done by the
PCI core through the ->read()/->write() callbacks at resume time.

Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
1 file changed, 52 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 108b3f15c410..7ecf1ac4036b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
return ret;
}

+static int __maybe_unused advk_pcie_suspend(struct device *dev)
+{
+ struct advk_pcie *pcie = dev_get_drvdata(dev);
+
+ advk_pcie_disable_phy(pcie);
+
+ clk_disable_unprepare(pcie->clk);
+
+ return 0;
+}
+
+static int __maybe_unused advk_pcie_resume(struct device *dev)
+{
+ struct advk_pcie *pcie = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(pcie->clk);
+ if (ret)
+ return ret;
+
+ /*
+ * Empirical delay needed after enabling the clock and before
+ * accessing any register.
+ */
+ msleep(10);
+
+ ret = advk_pcie_enable_phy(pcie);
+ if (ret)
+ return ret;
+
+ advk_pcie_hard_reset(pcie);
+
+ advk_pcie_setup_hw(pcie);
+
+ advk_sw_pci_bridge_init(pcie);
+
+ return 0;
+}
+
+/*
+ * The PCI core will try to reconfigure the bus quite early in the resume path.
+ * We must use the _noirq() alternatives to ensure the controller is ready when
+ * the core uses the ->read()/->write() callbacks.
+ */
+static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend,
+ advk_pcie_resume)
+};
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1188,6 +1237,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}

+ dev_set_drvdata(dev, pcie);
+
return 0;
}

@@ -1200,6 +1251,7 @@ static struct platform_driver advk_pcie_driver = {
.driver = {
.name = "advk-pcie",
.of_match_table = advk_pcie_of_match_table,
+ .pm = &advk_pcie_dev_pm_ops,
/* Driver unloading/unbinding currently not supported */
.suppress_bind_attrs = true,
},
--
2.19.1

2018-11-26 14:53:38

by Bjorn Helgaas

Subject: Re: [PATCH 00/12] Bring suspend to RAM support to PCIe Aardvark driver

On Fri, Nov 23, 2018 at 8:18 AM Miquel Raynal <[email protected]> wrote:
>
> Hello,
>
> As part of an effort to bring suspend to RAM support to Armada 3700
> SoCs (main target: ESPRESSObin), this series handles the work around
> the PCIe IP.
>
> First, more configuration is done in the 'setup' helper as inspired
> from the U-Boot driver. This is needed to entirely initialize the IP
> during future resume operation (patch 1).
>
> Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As
> current device trees do not provide the corresponding properties, not
> finding one of these properties is not an error and just produces a
> warning. However, if the property is present, an error during PHY
> initialization will fail the probe of the driver.
>
> Note: To be sure the clock will be resumed before this driver, a first
> series adding links between clocks and consumers has been submitted,
> see [1].
>
> Patch 5 adds suspend/resume hooks, re-using all the above.
>
> Finally, bindings and device trees are updated to reflect the hardware
> (patch 6-12). While the clock depends on the SoC, the reset GPIO and
> the PHY depends on the board so the clock is added in the
> armada-37xx.dtsi file while the two other properties are added in
> armada-3720-espressobin.dts.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html
>
> Thanks,
> Miquèl
>
>
> Miquel Raynal (12):
> PCI: aardvark: configure more registers in the configuration helper
> PCI: aardvark: add reset GPIO support
> PCI: aardvark: add PHY support
> PCI: aardvark: add clock support
> PCI: aardvark: add suspend to RAM support
> dt-bindings: PCI: aardvark: describe the reset-gpios property
> dt-bindings: PCI: aardvark: describe the clocks property
> dt-bindings: PCI: aardvark: describe the PHY property
> ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
> ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO
> ARM64: dts: marvell: armada-37xx: declare PCIe clock
> ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY

Hi Miquèl,

Thanks for your work! If/when you post a v2, please run "git log
--oneline" and adjust your subject lines to match the capitalization
conventions, i.e., for PCI, start the description with a capital
letter: "PCI: aardvark: Add suspend to RAM support".

BTW, I notice you closed your email with "Miquèl", but the patches
contain "Miquel". you *should* be able to use the correctly accented
version of your name in the Signed-off-by lines. I have tripped over
some tool issues, but if we pay attention, we should be able to get it
to work.

> .../devicetree/bindings/pci/aardvark-pci.txt | 9 +
> .../dts/marvell/armada-3720-espressobin.dts | 4 +
> arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 5 +
> drivers/pci/controller/pci-aardvark.c | 214 ++++++++++++++++++
> 4 files changed, 232 insertions(+)
>
> --
> 2.19.1
>

2018-11-30 13:14:52

by Miquel Raynal

Subject: Re: [PATCH 00/12] Bring suspend to RAM support to PCIe Aardvark driver

Hi Bjorn,

As you probably noticed already, I added you to the thread modifying
the A3700 periph clock driver, we have concerns about the PCIe
specificities.

Bjorn Helgaas <[email protected]> wrote on Mon, 26 Nov 2018 08:50:51
-0600:

> On Fri, Nov 23, 2018 at 8:18 AM Miquel Raynal <[email protected]> wrote:
> >
> > Hello,
> >
> > As part of an effort to bring suspend to RAM support to Armada 3700
> > SoCs (main target: ESPRESSObin), this series handles the work around
> > the PCIe IP.
> >
> > First, more configuration is done in the 'setup' helper as inspired
> > from the U-Boot driver. This is needed to entirely initialize the IP
> > during future resume operation (patch 1).
> >
> > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As
> > current device trees do not provide the corresponding properties, not
> > finding one of these properties is not an error and just produces a
> > warning. However, if the property is present, an error during PHY
> > initialization will fail the probe of the driver.
> >
> > Note: To be sure the clock will be resumed before this driver, a first
> > series adding links between clocks and consumers has been submitted,
> > see [1].
> >
> > Patch 5 adds suspend/resume hooks, re-using all the above.
> >
> > Finally, bindings and device trees are updated to reflect the hardware
> > (patch 6-12). While the clock depends on the SoC, the reset GPIO and
> > the PHY depends on the board so the clock is added in the
> > armada-37xx.dtsi file while the two other properties are added in
> > armada-3720-espressobin.dts.
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html
> >
> > Thanks,
> > Miquèl
> >
> >
> > Miquel Raynal (12):
> > PCI: aardvark: configure more registers in the configuration helper
> > PCI: aardvark: add reset GPIO support
> > PCI: aardvark: add PHY support
> > PCI: aardvark: add clock support
> > PCI: aardvark: add suspend to RAM support
> > dt-bindings: PCI: aardvark: describe the reset-gpios property
> > dt-bindings: PCI: aardvark: describe the clocks property
> > dt-bindings: PCI: aardvark: describe the PHY property
> > ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
> > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO
> > ARM64: dts: marvell: armada-37xx: declare PCIe clock
> > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY
>
> Hi Miquèl,
>
> Thanks for your work! If/when you post a v2, please run "git log
> --oneline" and adjust your subject lines to match the capitalization
> conventions, i.e., for PCI, start the description with a capital
> letter: "PCI: aardvark: Add suspend to RAM support".
>

Hum sorry, I noticed the upper case "PCI:" but I missed the other
capitalization. I had no review yet but I'll certainly send a v2 with
this fixed.

> BTW, I notice you closed your email with "Miquèl", but the patches
> contain "Miquel". you *should* be able to use the correctly accented
> version of your name in the Signed-off-by lines. I have tripped over
> some tool issues, but if we pay attention, we should be able to get it
> to work.

Indeed, I'm surprised that you noticed it! Even in France where éèàï
are quite common I very often get my first name screwed so I gave
up two years ago and decided to switch to ascii-only characters in my
'digital' life. Now it's harder to switch again, for example git log
--author would not recognize "Miquèl" and "Miquel" to be the same
person...

>
> > .../devicetree/bindings/pci/aardvark-pci.txt | 9 +
> > .../dts/marvell/armada-3720-espressobin.dts | 4 +
> > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 5 +
> > drivers/pci/controller/pci-aardvark.c | 214 ++++++++++++++++++
> > 4 files changed, 232 insertions(+)
> >
> > --
> > 2.19.1
> >


Thanks,
Miquèl

2018-12-03 10:28:07

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

[+Rafael, Sudeep]

On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> Add suspend and resume callbacks. The priority of these are
> "_noirq()", to workaround early access to the registers done by the
> PCI core through the ->read()/->write() callbacks at resume time.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 108b3f15c410..7ecf1ac4036b 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> return ret;
> }
>
> +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> +{
> + struct advk_pcie *pcie = dev_get_drvdata(dev);
> +
> + advk_pcie_disable_phy(pcie);
> +
> + clk_disable_unprepare(pcie->clk);

I have noticed it is common practice, still, I would like to check whether
it is allowed to call functions that may sleep in a NOIRQ suspend/resume
callback ?

Thanks,
Lorenzo

> +
> + return 0;
> +}
> +
> +static int __maybe_unused advk_pcie_resume(struct device *dev)
> +{
> + struct advk_pcie *pcie = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(pcie->clk);
> + if (ret)
> + return ret;
> +
> + /*
> + * Empirical delay needed after enabling the clock and before
> + * accessing any register.
> + */
> + msleep(10);
> +
> + ret = advk_pcie_enable_phy(pcie);
> + if (ret)
> + return ret;
> +
> + advk_pcie_hard_reset(pcie);
> +
> + advk_pcie_setup_hw(pcie);
> +
> + advk_sw_pci_bridge_init(pcie);
> +
> + return 0;
> +}
> +
> +/*
> + * The PCI core will try to reconfigure the bus quite early in the resume path.
> + * We must use the _noirq() alternatives to ensure the controller is ready when
> + * the core uses the ->read()/->write() callbacks.
> + */
> +static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend,
> + advk_pcie_resume)
> +};
> +
> static int advk_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1188,6 +1237,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> + dev_set_drvdata(dev, pcie);
> +
> return 0;
> }
>
> @@ -1200,6 +1251,7 @@ static struct platform_driver advk_pcie_driver = {
> .driver = {
> .name = "advk-pcie",
> .of_match_table = advk_pcie_of_match_table,
> + .pm = &advk_pcie_dev_pm_ops,
> /* Driver unloading/unbinding currently not supported */
> .suppress_bind_attrs = true,
> },
> --
> 2.19.1
>

2018-12-03 15:39:47

by Miquel Raynal

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Hi Lorenzo,

Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
10:27:08 +0000:

> [+Rafael, Sudeep]
>
> On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > Add suspend and resume callbacks. The priority of these are
> > "_noirq()", to workaround early access to the registers done by the
> > PCI core through the ->read()/->write() callbacks at resume time.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 108b3f15c410..7ecf1ac4036b 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> > return ret;
> > }
> >
> > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > +{
> > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > +
> > + advk_pcie_disable_phy(pcie);
> > +
> > + clk_disable_unprepare(pcie->clk);
>
> I have noticed it is common practice, still, I would like to check whether
> it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> callback ?

You are right this is weird. I double checked and for instance,
pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
probably other cases where drivers call functions that may sleep from a NOIRQ
context. I am interested to know if this is valid and if not, what is the
alternative?


Thanks,
Miquèl

2018-12-03 17:20:13

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

[+Stephen, Mike]

On Mon, Dec 03, 2018 at 04:38:46PM +0100, Miquel Raynal wrote:
> Hi Lorenzo,
>
> Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> 10:27:08 +0000:
>
> > [+Rafael, Sudeep]
> >
> > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > Add suspend and resume callbacks. The priority of these are
> > > "_noirq()", to workaround early access to the registers done by the
> > > PCI core through the ->read()/->write() callbacks at resume time.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > 1 file changed, 52 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 108b3f15c410..7ecf1ac4036b 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> > > return ret;
> > > }
> > >
> > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > +{
> > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > +
> > > + advk_pcie_disable_phy(pcie);
> > > +
> > > + clk_disable_unprepare(pcie->clk);
> >
> > I have noticed it is common practice, still, I would like to check whether
> > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > callback ?
>
> You are right this is weird. I double checked and for instance,
> pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing.
> There are probably other cases where drivers call functions that may
> sleep from a NOIRQ context. I am interested to know if this is valid
> and if not, what is the alternative?

I added Stephen and Mike, who along with Rafael can help us shed some
light into this, I do not have the necessary bits of info myself, I just
noticed.

Lorenzo

2018-12-03 19:20:18

by Stephen Boyd

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Quoting Lorenzo Pieralisi (2018-12-03 09:18:59)
> [+Stephen, Mike]
>
> On Mon, Dec 03, 2018 at 04:38:46PM +0100, Miquel Raynal wrote:
> > Hi Lorenzo,
> >
> > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > 10:27:08 +0000:
> >
> > > [+Rafael, Sudeep]
> > >
> > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > Add suspend and resume callbacks. The priority of these are
> > > > "_noirq()", to workaround early access to the registers done by the
> > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > 1 file changed, 52 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> > > > return ret;
> > > > }
> > > >
> > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > +{
> > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > +
> > > > + advk_pcie_disable_phy(pcie);
> > > > +
> > > > + clk_disable_unprepare(pcie->clk);
> > >
> > > I have noticed it is common practice, still, I would like to check whether
> > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > callback ?
> >
> > You are right this is weird. I double checked and for instance,
> > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing.
> > There are probably other cases where drivers call functions that may
> > sleep from a NOIRQ context. I am interested to know if this is valid
> > and if not, what is the alternative?
>
> I added Stephen and Mike, who along with Rafael can help us shed some
> light into this, I do not have the necessary bits of info myself, I just
> noticed.
>

Is the noirq phase of system suspend run with irqs disabled? Or just run
with the device irqs disabled? I thought it was the latter, which is
fine for this scenario because it's still running in a schedulable
context.

2018-12-03 22:01:25

by Rafael J. Wysocki

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> Hi Lorenzo,
>
> Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> 10:27:08 +0000:
>
> > [+Rafael, Sudeep]
> >
> > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > Add suspend and resume callbacks. The priority of these are
> > > "_noirq()", to workaround early access to the registers done by the
> > > PCI core through the ->read()/->write() callbacks at resume time.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > 1 file changed, 52 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 108b3f15c410..7ecf1ac4036b 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> > > return ret;
> > > }
> > >
> > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > +{
> > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > +
> > > + advk_pcie_disable_phy(pcie);
> > > +
> > > + clk_disable_unprepare(pcie->clk);
> >
> > I have noticed it is common practice, still, I would like to check whether
> > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > callback ?
>
> You are right this is weird. I double checked and for instance,
> pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> probably other cases where drivers call functions that may sleep from a NOIRQ
> context. I am interested to know if this is valid and if not, what is the
> alternative?
>

Yes, it is valid. _noirq means that the high-level action handlers will not be
invoked for interrupts occurring during that period, but that doesn't apply to
timer interrupts.

IOW, don't expect *your* IRQ handler to be invoked then (if this is not a timer
IRQ), but you can sleep.

Thanks,
Rafael

2018-12-03 22:21:03

by Miquel Raynal

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Hi Rafael, Stephen,

"Rafael J. Wysocki" <[email protected]> wrote on Mon, 03 Dec 2018
23:00:20 +0100:

> On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > Hi Lorenzo,
> >
> > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > 10:27:08 +0000:
> >
> > > [+Rafael, Sudeep]
> > >
> > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > Add suspend and resume callbacks. The priority of these are
> > > > "_noirq()", to workaround early access to the registers done by the
> > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > 1 file changed, 52 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
> > > > return ret;
> > > > }
> > > >
> > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > +{
> > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > +
> > > > + advk_pcie_disable_phy(pcie);
> > > > +
> > > > + clk_disable_unprepare(pcie->clk);
> > >
> > > I have noticed it is common practice, still, I would like to check whether
> > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > callback ?
> >
> > You are right this is weird. I double checked and for instance,
> > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> > probably other cases where drivers call functions that may sleep from a NOIRQ
> > context. I am interested to know if this is valid and if not, what is the
> > alternative?
> >
>
> Yes, it is valid. _noirq means that the high-level action handlers will not be
> invoked for interrupts occurring during that period, but that doesn't apply to
> timer interrupts.
>
> IOW, don't expect *your* IRQ handler to be invoked then (if this is not a timer
> IRQ), but you can sleep.
>
> Thanks,
> Rafael
>

Thank you both for the enlightenment.


Thanks,
Miquèl

2018-12-04 09:47:06

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > Hi Lorenzo,
> >
> > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > 10:27:08 +0000:
> >
> > > [+Rafael, Sudeep]
> > >
> > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > Add suspend and resume callbacks. The priority of these are
> > > > "_noirq()", to workaround early access to the registers done by the
> > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > 1 file changed, 52 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pci
> > > > return ret;
> > > > }
> > > >
> > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > +{
> > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > +
> > > > + advk_pcie_disable_phy(pcie);
> > > > +
> > > > + clk_disable_unprepare(pcie->clk);
> > >
> > > I have noticed it is common practice, still, I would like to check whether
> > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > callback ?
> >
> > You are right this is weird. I double checked and for instance,
> > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> > probably other cases where drivers call functions that may sleep from a NOIRQ
> > context. I am interested to know if this is valid and if not, what is the
> > alternative?
> >
>
> Yes, it is valid. _noirq means that the high-level action handlers
> will not be invoked for interrupts occurring during that period, but
> that doesn't apply to timer interrupts.
>
> IOW, don't expect *your* IRQ handler to be invoked then (if this is
> not a timer IRQ), but you can sleep.

Hi Rafael, all,

I did not ask my question (that may be silly) properly apologies. I know
that the S2R context allows sleeping the question is, in case
clk_disable_unprepare() (and resume counterparts) sleeps, what is going
to wake it up, given that we are in the S2R NOIRQ phase and as you said
the action handlers (that are possibly required to wake up the eg
clk_disable_unprepare() caller) are disabled (unless, AFAIK,
IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).

The clk API implementations back-ends are beyond my depth, I just wanted
to make sure I understand how the S2R flow is expected to work in this
specific case.

Thanks,
Lorenzo

2018-12-04 21:43:44

by Rafael J. Wysocki

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:
> On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> > On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > > Hi Lorenzo,
> > >
> > > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > > 10:27:08 +0000:
> > >
> > > > [+Rafael, Sudeep]
> > > >
> > > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > > Add suspend and resume callbacks. The priority of these are
> > > > > "_noirq()", to workaround early access to the registers done by the
> > > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > ---
> > > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > > 1 file changed, 52 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pci
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > > +{
> > > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > + advk_pcie_disable_phy(pcie);
> > > > > +
> > > > > + clk_disable_unprepare(pcie->clk);
> > > >
> > > > I have noticed it is common practice, still, I would like to check whether
> > > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > > callback ?
> > >
> > > You are right this is weird. I double checked and for instance,
> > > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> > > probably other cases where drivers call functions that may sleep from a NOIRQ
> > > context. I am interested to know if this is valid and if not, what is the
> > > alternative?
> > >
> >
> > Yes, it is valid. _noirq means that the high-level action handlers
> > will not be invoked for interrupts occurring during that period, but
> > that doesn't apply to timer interrupts.
> >
> > IOW, don't expect *your* IRQ handler to be invoked then (if this is
> > not a timer IRQ), but you can sleep.
>
> Hi Rafael, all,
>
> I did not ask my question (that may be silly) properly apologies. I know
> that the S2R context allows sleeping the question is, in case
> clk_disable_unprepare() (and resume counterparts) sleeps,

If it just sleeps, then this is not a problem, but if it actually *waits*
for something meaningful to happen (which I guess is what you really mean),
then things may go awry.

> what is going to wake it up, given that we are in the S2R NOIRQ phase and as
> you said the action handlers (that are possibly required to wake up the eg
> clk_disable_unprepare() caller) are disabled (unless, AFAIK,
> IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).

So if it waits for an action handler to do something and wake it up, it may
very well deadlock. I have no idea if that really happens, though.

> The clk API implementations back-ends are beyond my depth, I just wanted
> to make sure I understand how the S2R flow is expected to work in this
> specific case.

Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
(well, there are a few more complications I don't recall exactly, but
that's the basic rule). If anything depends on them to run, it will block.

Thanks,
Rafael

2018-12-05 11:01:39

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Tue, Dec 04, 2018 at 10:42:19PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:
> > On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > > > Hi Lorenzo,
> > > >
> > > > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > > > 10:27:08 +0000:
> > > >
> > > > > [+Rafael, Sudeep]
> > > > >
> > > > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > > > Add suspend and resume callbacks. The priority of these are
> > > > > > "_noirq()", to workaround early access to the registers done by the
> > > > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > > > >
> > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > ---
> > > > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > > > 1 file changed, 52 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pci
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > > > +{
> > > > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > > > +
> > > > > > + advk_pcie_disable_phy(pcie);
> > > > > > +
> > > > > > + clk_disable_unprepare(pcie->clk);
> > > > >
> > > > > I have noticed it is common practice, still, I would like to check whether
> > > > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > > > callback ?
> > > >
> > > > You are right this is weird. I double checked and for instance,
> > > > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> > > > probably other cases where drivers call functions that may sleep from a NOIRQ
> > > > context. I am interested to know if this is valid and if not, what is the
> > > > alternative?
> > > >
> > >
> > > Yes, it is valid. _noirq means that the high-level action handlers
> > > will not be invoked for interrupts occurring during that period, but
> > > that doesn't apply to timer interrupts.
> > >
> > > IOW, don't expect *your* IRQ handler to be invoked then (if this is
> > > not a timer IRQ), but you can sleep.
> >
> > Hi Rafael, all,
> >
> > I did not ask my question (that may be silly) properly apologies. I know
> > that the S2R context allows sleeping the question is, in case
> > clk_disable_unprepare() (and resume counterparts) sleeps,
>
> If it just sleeps, then this is not a problem, but if it actually *waits*
> for something meaningful to happen (which I guess is what you really mean),
> then things may go awry.

That's what I meant and I assume that's reason why the
*_prepare/unprepare() API are allowed to sleep, waiting for
an event to trigger.

> > what is going to wake it up, given that we are in the S2R NOIRQ phase and as
> > you said the action handlers (that are possibly required to wake up the eg
> > clk_disable_unprepare() caller) are disabled (unless, AFAIK,
> > IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).
>
> So if it waits for an action handler to do something and wake it up, it may
> very well deadlock. I have no idea if that really happens, though.

I have no idea either, that's why I asked the clock maintainers too,
the point is, the clk API allows this behaviour, I do not think it is safe
to rely on a non-blocking clk back-end, that's why I raised the question
in the first place.

> > The clk API implementations back-ends are beyond my depth, I just wanted
> > to make sure I understand how the S2R flow is expected to work in this
> > specific case.
>
> Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
> (well, there are a few more complications I don't recall exactly, but
> that's the basic rule). If anything depends on them to run, it will block.

It looks like this patch (and more code in the kernel) expects either
the clk calls not to block or the action handler implemented in the
clock back-ends to run in S2R-NOIRQ (possibly by using the
IRQF_NO_SUSPEND flag).

I would appreciate if the clock maintainers can shed some light on
this.

Thanks,
Lorenzo

2018-12-11 14:30:34

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Tue, Dec 04, 2018 at 10:42:19PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:
> > On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > > > Hi Lorenzo,
> > > >
> > > > Lorenzo Pieralisi <[email protected]> wrote on Mon, 3 Dec 2018
> > > > 10:27:08 +0000:
> > > >
> > > > > [+Rafael, Sudeep]
> > > > >
> > > > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > > > Add suspend and resume callbacks. The priority of these are
> > > > > > "_noirq()", to workaround early access to the registers done by the
> > > > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > > > >
> > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > ---
> > > > > > drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
> > > > > > 1 file changed, 52 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pci
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > > > +{
> > > > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > > > +
> > > > > > + advk_pcie_disable_phy(pcie);
> > > > > > +
> > > > > > + clk_disable_unprepare(pcie->clk);
> > > > >
> > > > > I have noticed it is common practice, still, I would like to check whether
> > > > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > > > callback ?
> > > >
> > > > You are right this is weird. I double checked and for instance,
> > > > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing. There are
> > > > probably other cases where drivers call functions that may sleep from a NOIRQ
> > > > context. I am interested to know if this is valid and if not, what is the
> > > > alternative?
> > > >
> > >
> > > Yes, it is valid. _noirq means that the high-level action handlers
> > > will not be invoked for interrupts occurring during that period, but
> > > that doesn't apply to timer interrupts.
> > >
> > > IOW, don't expect *your* IRQ handler to be invoked then (if this is
> > > not a timer IRQ), but you can sleep.
> >
> > Hi Rafael, all,
> >
> > I did not ask my question (that may be silly) properly apologies. I know
> > that the S2R context allows sleeping the question is, in case
> > clk_disable_unprepare() (and resume counterparts) sleeps,
>
> If it just sleeps, then this is not a problem, but if it actually *waits*
> for something meaningful to happen (which I guess is what you really mean),
> then things may go awry.
>
> > what is going to wake it up, given that we are in the S2R NOIRQ phase and as
> > you said the action handlers (that are possibly required to wake up the eg
> > clk_disable_unprepare() caller) are disabled (unless, AFAIK,
> > IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).
>
> So if it waits for an action handler to do something and wake it up, it may
> very well deadlock. I have no idea if that really happens, though.
>
> > The clk API implementations back-ends are beyond my depth, I just wanted
> > to make sure I understand how the S2R flow is expected to work in this
> > specific case.
>
> Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
> (well, there are a few more complications I don't recall exactly, but
> that's the basic rule). If anything depends on them to run, it will block.

Stephen, any comments on this ? I would like to understand if it is safe
to call a clk_*unprepare/prepare_* function (that may have a blocking
back-end waiting on a wake-up event triggered by an IRQ action) in the
suspend/resume NOIRQ phase.

It is not clear how the unprepare/prepare() callers can possibly know
whether it is safe to block at that stage given that IRQ actions are
suspended and the wake-up may never trigger.

Thanks,
Lorenzo

2018-12-11 21:46:00

by Rob Herring

Subject: Re: [PATCH 06/12] dt-bindings: PCI: aardvark: describe the reset-gpios property

On Fri, 23 Nov 2018 15:18:25 +0100, Miquel Raynal wrote:
> A GPIO might be used to reset the PCI IP. Describe the property needed
> in this case.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/aardvark-pci.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>

2018-12-11 21:46:05

by Rob Herring

Subject: Re: [PATCH 07/12] dt-bindings: PCI: aardvark: describe the clocks property

On Fri, 23 Nov 2018 15:18:26 +0100, Miquel Raynal wrote:
> Describe the missing gated clock feeding the PCIe IP.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
> 1 file changed, 2 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>

2018-12-11 21:46:21

by Rob Herring

Subject: Re: [PATCH 08/12] dt-bindings: PCI: aardvark: describe the PHY property

On Fri, 23 Nov 2018 15:18:27 +0100, Miquel Raynal wrote:
> Document the possibility to reference a PHY.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
> 1 file changed, 2 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>

2018-12-13 10:22:09

by Stephen Boyd

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Quoting Lorenzo Pieralisi (2018-12-11 06:16:27)
> On Tue, Dec 04, 2018 at 10:42:19PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:
> > > On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> > > > On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > >
> > > I did not ask my question (that may be silly) properly apologies. I know
> > > that the S2R context allows sleeping the question is, in case
> > > clk_disable_unprepare() (and resume counterparts) sleeps,
> >
> > If it just sleeps, then this is not a problem, but if it actually *waits*
> > for something meaningful to happen (which I guess is what you really mean),
> > then things may go awry.
> >
> > > what is going to wake it up, given that we are in the S2R NOIRQ phase and as
> > > you said the action handlers (that are possibly required to wake up the eg
> > > clk_disable_unprepare() caller) are disabled (unless, AFAIK,
> > > IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).
> >
> > So if it waits for an action handler to do something and wake it up, it may
> > very well deadlock. I have no idea if that really happens, though.
> >
> > > The clk API implementations back-ends are beyond my depth, I just wanted
> > > to make sure I understand how the S2R flow is expected to work in this
> > > specific case.
> >
> > Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
> > (well, there are a few more complications I don't recall exactly, but
> > that's the basic rule). If anything depends on them to run, it will block.
>
> Stephen, any comments on this ?

Sorry I seemed to miss this email. BTW, what is an "action handler"
here? The IRQ action handler?

> I would like to understand if it is safe
> to call a clk_*unprepare/prepare_* function (that may have a blocking
> back-end waiting on a wake-up event triggered by an IRQ action) in the
> suspend/resume NOIRQ phase.

Does this ever occur in practice? I imagine "blocking back-end waiting
on a wake-up event" would be some sort of i2c or SPI based "slow" clk
that is prepared/unprepared in the NOIRQ phase of suspend/resume? So
that function call into the clk API fails because the i2c or SPI
controller used to toggle the clk on/off state relies on the
controller's IRQ to manage the transaction over the bus but that IRQ is
disabled. I suppose this is possible but I've never heard of it
happening in practice. Do you have such a scenario?

>
> It is not clear how the unprepare/prepare() callers can possibly know
> whether it is safe to block at that stage given that IRQ actions are
> suspended and the wake-up may never trigger.
>

Is this solved in other situations somehow? I don't think clk consumers
have any idea that things are safe or not safe to use in the NOIRQ phase
of suspend, but I also don't see how clks are special here. Any provider
consumer pattern would fall into the same trap, but maybe clks are the
first ones to get here.

It seems like a larger problem with NOIRQ suspend in general and how it
is too coarse of a solution for suspend ordering of devices. It's not
like we need *all* device interrupts to be disabled to do something in
suspend with one particular device. Most likely, we just need the device
and all it's children to be suspended and this device to have it's IRQ
disabled for the NOIRQ suspend callback to work. (Maybe any devices it's
supplying with device links too?)

If that's really the case, then I can see how one device and it's
children are suspended and the irq for it is disabled but the providing
devices (clk, regulator, bus controller, etc.) are still fully active
and not suspended but in fact completely usable and able to service
interrupts. If that all makes sense, then I would answer the question
with a definitive "yes it's all fine" because the clk consumer could be
in the NOIRQ phase of its suspend but the clk provider wouldn't have
even started suspending yet when clk_disable_unprepare() is called.

2018-12-13 10:55:43

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Thu, Dec 13, 2018 at 01:00:26AM -0800, Stephen Boyd wrote:
> Quoting Lorenzo Pieralisi (2018-12-11 06:16:27)
> > On Tue, Dec 04, 2018 at 10:42:19PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, December 4, 2018 10:45:58 AM CET Lorenzo Pieralisi wrote:
> > > > On Mon, Dec 03, 2018 at 11:00:20PM +0100, Rafael J. Wysocki wrote:
> > > > > On Monday, December 3, 2018 4:38:46 PM CET Miquel Raynal wrote:
> > > >
> > > > I did not ask my question (that may be silly) properly apologies. I know
> > > > that the S2R context allows sleeping the question is, in case
> > > > clk_disable_unprepare() (and resume counterparts) sleeps,
> > >
> > > If it just sleeps, then this is not a problem, but if it actually *waits*
> > > for something meaningful to happen (which I guess is what you really mean),
> > > then things may go awry.
> > >
> > > > what is going to wake it up, given that we are in the S2R NOIRQ phase and as
> > > > you said the action handlers (that are possibly required to wake up the eg
> > > > clk_disable_unprepare() caller) are disabled (unless, AFAIK,
> > > > IRQF_NO_SUSPEND is passed at IRQ request time in the respective driver).
> > >
> > > So if it waits for an action handler to do something and wake it up, it may
> > > very well deadlock. I have no idea if that really happens, though.
> > >
> > > > The clk API implementations back-ends are beyond my depth, I just wanted
> > > > to make sure I understand how the S2R flow is expected to work in this
> > > > specific case.
> > >
> > > Action handlers won't run unless the IRQs are marked as IRQF_NO_SUSPEND
> > > (well, there are a few more complications I don't recall exactly, but
> > > that's the basic rule). If anything depends on them to run, it will block.
> >
> > Stephen, any comments on this ?
>
> Sorry I seemed to miss this email. BTW, what is an "action handler"
> here? The IRQ action handler?

Yes, that is.

> > I would like to understand if it is safe
> > to call a clk_*unprepare/prepare_* function (that may have a blocking
> > back-end waiting on a wake-up event triggered by an IRQ action) in the
> > suspend/resume NOIRQ phase.
>
> Does this ever occur in practice? I imagine "blocking back-end waiting
> on a wake-up event" would be some sort of i2c or SPI based "slow" clk
> that is prepared/unprepared in the NOIRQ phase of suspend/resume? So
> that function call into the clk API fails because the i2c or SPI
> controller used to toggle the clk on/off state relies on the
> controller's IRQ to manage the transaction over the bus but that IRQ is
> disabled. I suppose this is possible but I've never heard of it
> happening in practice. Do you have such a scenario?

No (because my knowledge of clock internals is poor) but I questioned
the code while reviewing it - I do not think it is a safe assumption
to make (otherwise what's the purpose of having a clk API -
*prepare/unprepare*() - that, AFAIK was implemented to allow back-ends to
block, waiting for an event).

There is clearly an implicit assumption there from clk API caller POW
"this call won't block or an IRQ action - IRQF_NO_SUSPEND - will wake
me up if it does. Or the call can time out, but that's an error path".

This seems fragile to me.

> > It is not clear how the unprepare/prepare() callers can possibly know
> > whether it is safe to block at that stage given that IRQ actions are
> > suspended and the wake-up may never trigger.
> >
>
> Is this solved in other situations somehow? I don't think clk consumers
> have any idea that things are safe or not safe to use in the NOIRQ phase
> of suspend, but I also don't see how clks are special here. Any provider
> consumer pattern would fall into the same trap, but maybe clks are the
> first ones to get here.

You have a good point, I do not think clk are specials, I am only saying
this patch code can run into significant issues.

> It seems like a larger problem with NOIRQ suspend in general and how it
> is too coarse of a solution for suspend ordering of devices. It's not
> like we need *all* device interrupts to be disabled to do something in
> suspend with one particular device. Most likely, we just need the device
> and all it's children to be suspended and this device to have it's IRQ
> disabled for the NOIRQ suspend callback to work. (Maybe any devices it's
> supplying with device links too?)
>
> If that's really the case, then I can see how one device and it's
> children are suspended and the irq for it is disabled but the providing
> devices (clk, regulator, bus controller, etc.) are still fully active
> and not suspended but in fact completely usable and able to service
> interrupts. If that all makes sense, then I would answer the question
> with a definitive "yes it's all fine" because the clk consumer could be
> in the NOIRQ phase of its suspend but the clk provider wouldn't have
> even started suspending yet when clk_disable_unprepare() is called.

That's a very good summary and address my concern, I still question this
patch correctness (and many others that carry out clk operations in S2R
NOIRQ phase), they may work but do not tell me they are rock solid given
your accurate summary above.

Thanks,
Lorenzo

2018-12-13 14:32:35

by Miquel Raynal

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Hi Lorenzo,

> > If that's really the case, then I can see how one device and it's
> > children are suspended and the irq for it is disabled but the providing
> > devices (clk, regulator, bus controller, etc.) are still fully active
> > and not suspended but in fact completely usable and able to service
> > interrupts. If that all makes sense, then I would answer the question
> > with a definitive "yes it's all fine" because the clk consumer could be
> > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > even started suspending yet when clk_disable_unprepare() is called.
>
> That's a very good summary and address my concern, I still question this
> patch correctness (and many others that carry out clk operations in S2R
> NOIRQ phase), they may work but do not tell me they are rock solid given
> your accurate summary above.

I understand your concern but I don't see any alternative right now
and a deep rework of the PM core to respect such dependency is not
something that can be done in a reasonable amount of time. With
regard to this constraint, do you think it is worth blocking the
series?


Thanks,
Miquèl

2018-12-13 14:53:26

by Lorenzo Pieralisi

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Thu, Dec 13, 2018 at 03:30:00PM +0100, Miquel Raynal wrote:
> Hi Lorenzo,
>
> > > If that's really the case, then I can see how one device and it's
> > > children are suspended and the irq for it is disabled but the providing
> > > devices (clk, regulator, bus controller, etc.) are still fully active
> > > and not suspended but in fact completely usable and able to service
> > > interrupts. If that all makes sense, then I would answer the question
> > > with a definitive "yes it's all fine" because the clk consumer could be
> > > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > > even started suspending yet when clk_disable_unprepare() is called.
> >
> > That's a very good summary and address my concern, I still question this
> > patch correctness (and many others that carry out clk operations in S2R
> > NOIRQ phase), they may work but do not tell me they are rock solid given
> > your accurate summary above.
>
> I understand your concern but I don't see any alternative right now
> and a deep rework of the PM core to respect such dependency is not
> something that can be done in a reasonable amount of time. With
> regard to this constraint, do you think it is worth blocking the
> series?

I think we agree that, depending on what HW/SW driver manage
this PCI controller clocks, this driver may well become broken,
the driver itself has no idea what's behind the clock API and
can end up waiting for an event forever.

This does not leave me in a comfortable position to merge code that I
know has flaws.

I won't merge it for v4.21, I need more time (and feedback) to
understand what can be done to make this driver (and many others)
more robust.

Thanks,
Lorenzo

2018-12-13 21:53:36

by Rafael J. Wysocki

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Thursday, December 13, 2018 3:30:00 PM CET Miquel Raynal wrote:
> Hi Lorenzo,
>
> > > If that's really the case, then I can see how one device and it's
> > > children are suspended and the irq for it is disabled but the providing
> > > devices (clk, regulator, bus controller, etc.) are still fully active
> > > and not suspended but in fact completely usable and able to service
> > > interrupts. If that all makes sense, then I would answer the question
> > > with a definitive "yes it's all fine" because the clk consumer could be
> > > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > > even started suspending yet when clk_disable_unprepare() is called.
> >
> > That's a very good summary and address my concern, I still question this
> > patch correctness (and many others that carry out clk operations in S2R
> > NOIRQ phase), they may work but do not tell me they are rock solid given
> > your accurate summary above.
>
> I understand your concern but I don't see any alternative right now
> and a deep rework of the PM core to respect such dependency is not
> something that can be done in a reasonable amount of time.

Maybe you don't need to rework anything. :-)

Have you considered using device links?

Thanks,
Rafael

2018-12-17 14:56:23

by Miquel Raynal

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Hi Rafael,

"Rafael J. Wysocki" <[email protected]> wrote on Thu, 13 Dec 2018
22:50:51 +0100:

> On Thursday, December 13, 2018 3:30:00 PM CET Miquel Raynal wrote:
> > Hi Lorenzo,
> >
> > > > If that's really the case, then I can see how one device and it's
> > > > children are suspended and the irq for it is disabled but the providing
> > > > devices (clk, regulator, bus controller, etc.) are still fully active
> > > > and not suspended but in fact completely usable and able to service
> > > > interrupts. If that all makes sense, then I would answer the question
> > > > with a definitive "yes it's all fine" because the clk consumer could be
> > > > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > > > even started suspending yet when clk_disable_unprepare() is called.
> > >
> > > That's a very good summary and address my concern, I still question this
> > > patch correctness (and many others that carry out clk operations in S2R
> > > NOIRQ phase), they may work but do not tell me they are rock solid given
> > > your accurate summary above.
> >
> > I understand your concern but I don't see any alternative right now
> > and a deep rework of the PM core to respect such dependency is not
> > something that can be done in a reasonable amount of time.
>
> Maybe you don't need to rework anything. :-)
>
> Have you considered using device links?

Absolutely, yes :) I am actively working on it in parallel, you can
check the third version there [1]. Stephen Boyd has a slightly
different idea of how it should be done, I will propose a v4 this week,
I can add you in copy if you are interested!

Anyway, there is one thing that is still missing:
* Let's have device A that requests clock B
* With the device link series, A is linked (as a child) to B.
* A suspend/resume hooks handle things in the NOIRQ phase.
* B suspend/resume hooks handle things in the default phase.

What I expected during a suspend:
1/ ->suspend_noirq(device A)
2/ ->suspend(clock B)

Unfortunately, device links do not seem to enforce any priority between
phases (default/late/noirq) and what happens is:
1/ ->suspend(B)
2/ ->suspend_noirq(A)
Which has no sense in my case. Hence, I had to request the clock
suspend/resume callbacks to be upgraded to the NOIRQ phase as well (I
don't have a better solution for now). This is still under discussion
in a thread you have been recently added to by Bjorn, see [2].

So when I told you I was not confident in "reworking the PM core to
respect such dependency", this is what I was referring to. I am
definitely ready to help, but I don't feel I can do it alone.

[1] https://www.spinics.net/lists/linux-clk/msg32824.html
[2] https://marc.info/?l=linux-pm&m=154465198510735&w=2


Thanks,
Miquèl

2018-12-18 10:56:19

by Rafael J. Wysocki

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

On Monday, December 17, 2018 3:54:26 PM CET Miquel Raynal wrote:
> Hi Rafael,
>
> "Rafael J. Wysocki" <[email protected]> wrote on Thu, 13 Dec 2018
> 22:50:51 +0100:
>
> > On Thursday, December 13, 2018 3:30:00 PM CET Miquel Raynal wrote:
> > > Hi Lorenzo,
> > >
> > > > > If that's really the case, then I can see how one device and it's
> > > > > children are suspended and the irq for it is disabled but the providing
> > > > > devices (clk, regulator, bus controller, etc.) are still fully active
> > > > > and not suspended but in fact completely usable and able to service
> > > > > interrupts. If that all makes sense, then I would answer the question
> > > > > with a definitive "yes it's all fine" because the clk consumer could be
> > > > > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > > > > even started suspending yet when clk_disable_unprepare() is called.
> > > >
> > > > That's a very good summary and address my concern, I still question this
> > > > patch correctness (and many others that carry out clk operations in S2R
> > > > NOIRQ phase), they may work but do not tell me they are rock solid given
> > > > your accurate summary above.
> > >
> > > I understand your concern but I don't see any alternative right now
> > > and a deep rework of the PM core to respect such dependency is not
> > > something that can be done in a reasonable amount of time.
> >
> > Maybe you don't need to rework anything. :-)
> >
> > Have you considered using device links?
>
> Absolutely, yes :) I am actively working on it in parallel, you can
> check the third version there [1]. Stephen Boyd has a slightly
> different idea of how it should be done, I will propose a v4 this week,
> I can add you in copy if you are interested!
>
> Anyway, there is one thing that is still missing:
> * Let's have device A that requests clock B
> * With the device link series, A is linked (as a child) to B.
> * A suspend/resume hooks handle things in the NOIRQ phase.

Why do you need them to run in the "noirq" phase in the first place?

> * B suspend/resume hooks handle things in the default phase.
>
> What I expected during a suspend:
> 1/ ->suspend_noirq(device A)
> 2/ ->suspend(clock B)

This expectation is not in agreement with the documented suspend code flow,
however.

Each phase of it is carried out for *all* devices completely before getting
to the next phase, "prepare" first, then "suspend", "suspend_late" and
"suspend_noirq", in this order.

> Unfortunately, device links do not seem to enforce any priority between
> phases (default/late/noirq) and what happens is:
> 1/ ->suspend(B)
> 2/ ->suspend_noirq(A)
> Which has no sense in my case. Hence, I had to request the clock
> suspend/resume callbacks to be upgraded to the NOIRQ phase as well (I
> don't have a better solution for now). This is still under discussion
> in a thread you have been recently added to by Bjorn, see [2].
>
> So when I told you I was not confident in "reworking the PM core to
> respect such dependency", this is what I was referring to. I am
> definitely ready to help, but I don't feel I can do it alone.
>
> [1] https://www.spinics.net/lists/linux-clk/msg32824.html
> [2] https://marc.info/?l=linux-pm&m=154465198510735&w=2

The rework you seem to be talking about is not possible, I'm afraid.

2018-12-18 14:15:52

by Miquel Raynal

Subject: Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

Hi Rafael, Stephen & Bjorn,

Glad to see you all in this thread that talks about:
* adding S2RAM support to a PCIe controller driver
* by taking into account that the PCI clock must be
{enabled before,disabled after} the PCI IP itself
* and that it requires some tweaking in the clock driver to
promote the suspend/resume() callbacks to the NOIRQ phase
(reference there [1]).

Stephen, Rafael answered here to your remark (in thread [1]) about the
NOIRQ promotion (see below).

Bjorn, there is a question for you below about the need for a PCI
controller driver to suspend/resume in the NOIRQ phase.

Rafael, thanks for the explanation of what the PM core sequences really
are, I would need you to confirm my approach that promotes the clock
suspend/resume() callbacks to the NOIRQ phase, or otherwise give me
pointers to an alternate solution (also below).


"Rafael J. Wysocki" <[email protected]> wrote on Tue, 18 Dec 2018
11:54:43 +0100:

> On Monday, December 17, 2018 3:54:26 PM CET Miquel Raynal wrote:
> > Hi Rafael,
> >
> > "Rafael J. Wysocki" <[email protected]> wrote on Thu, 13 Dec 2018
> > 22:50:51 +0100:
> >
> > > On Thursday, December 13, 2018 3:30:00 PM CET Miquel Raynal wrote:
> > > > Hi Lorenzo,
> > > >
> > > > > > If that's really the case, then I can see how one device and it's
> > > > > > children are suspended and the irq for it is disabled but the providing
> > > > > > devices (clk, regulator, bus controller, etc.) are still fully active
> > > > > > and not suspended but in fact completely usable and able to service
> > > > > > interrupts. If that all makes sense, then I would answer the question
> > > > > > with a definitive "yes it's all fine" because the clk consumer could be
> > > > > > in the NOIRQ phase of its suspend but the clk provider wouldn't have
> > > > > > even started suspending yet when clk_disable_unprepare() is called.
> > > > >
> > > > > That's a very good summary and address my concern, I still question this
> > > > > patch correctness (and many others that carry out clk operations in S2R
> > > > > NOIRQ phase), they may work but do not tell me they are rock solid given
> > > > > your accurate summary above.
> > > >
> > > > I understand your concern but I don't see any alternative right now
> > > > and a deep rework of the PM core to respect such dependency is not
> > > > something that can be done in a reasonable amount of time.
> > >
> > > Maybe you don't need to rework anything. :-)
> > >
> > > Have you considered using device links?
> >
> > Absolutely, yes :) I am actively working on it in parallel, you can
> > check the third version there [1]. Stephen Boyd has a slightly
> > different idea of how it should be done, I will propose a v4 this week,
> > I can add you in copy if you are interested!
> >
> > Anyway, there is one thing that is still missing:
> > * Let's have device A that requests clock B
> > * With the device link series, A is linked (as a child) to B.
> > * A suspend/resume hooks handle things in the NOIRQ phase.
>
> Why do you need them to run in the "noirq" phase in the first place?

I suppose (and I would like Bjorn to validate my thoughts) that this
is a limitation imposed by the PCI core, as described in this
commit:

commit ab14d45ea58eae67c739e4ba01871cae7b6c4586
Author: Thomas Petazzoni <[email protected]>
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 <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Jason Cooper <[email protected]>

>
> > * B suspend/resume hooks handle things in the default phase.
> >
> > What I expected during a suspend:
> > 1/ ->suspend_noirq(device A)
> > 2/ ->suspend(clock B)
>
> This expectation is not in agreement with the documented suspend code flow,
> however.
>
> Each phase of it is carried out for *all* devices completely before getting
> to the next phase, "prepare" first, then "suspend", "suspend_late" and
> "suspend_noirq", in this order.

Thanks for clarifying, now it is clear and it also answers Stephen
remark in the related thread [1]:

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

Stephen, said:

"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."

>
> > Unfortunately, device links do not seem to enforce any priority between
> > phases (default/late/noirq) and what happens is:
> > 1/ ->suspend(B)
> > 2/ ->suspend_noirq(A)
> > Which has no sense in my case. Hence, I had to request the clock
> > suspend/resume callbacks to be upgraded to the NOIRQ phase as well (I
> > don't have a better solution for now). This is still under discussion
> > in a thread you have been recently added to by Bjorn, see [2].
> >
> > So when I told you I was not confident in "reworking the PM core to
> > respect such dependency", this is what I was referring to. I am
> > definitely ready to help, but I don't feel I can do it alone.
> >
> > [1] https://www.spinics.net/lists/linux-clk/msg32824.html
> > [2] https://marc.info/?l=linux-pm&m=154465198510735&w=2
>
> The rework you seem to be talking about is not possible, I'm afraid.
>

Ok, then do you agree that the only solution in this case is what I
propose in thread [1], ie. promoting the clock suspend/resume callbacks
to the NOIRQ phase in order to ensure that they will run first (once
device links will be merged too) ?

[1] https://www.spinics.net/lists/linux-clk/msg32537.html


Thank you very much for helping,
Miquèl