[PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Miquel Raynal]

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]. Anyway, having the clock series applied first is not needed.

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/2019-January/623885.html

Thanks,
Miquèl


Changes since v2:
=================
* Minor patches reordering.
* Added pinctrl patches from Gregory Clement fixing the PCIe pins. His
  changes implied modifications in the DT/bindings patches adding PCIe
  reset pin support.
* Added a new patch that enlarges the PIO timeout of the driver
  (explanations in the commit log).
* With the timeout changed, removed the "experimental delay" that was
  needed at resume time before accessing any register.

Changes since v1:
=================
* Change the capitalization in commit titles to follow the PCI
  subsystem rules.
* Added Suggested-by tag to the patch adding PHY support and to the
  patch adding the PHY property in the DT.
* Added Rob's Reviewed-by tags on bindings.
* I am following the discussion about calling functions that might
  sleep in a NOIRQ context. As there is no real problem yet (as per my
  understanding), I did not change anything on this regard.


Miquel Raynal (15):
  PCI: aardvark: Enlarge PIO timeout
  PCI: aardvark: Configure more registers in the configuration helper
  PCI: aardvark: Add clock support
  PCI: aardvark: Add PHY support
  PCI: aardvark: Add PCIe warm reset support
  PCI: aardvark: Add external reset GPIO support
  PCI: aardvark: Add suspend to RAM support
  dt-bindings: PCI: aardvark: Describe the clocks property
  dt-bindings: PCI: aardvark: Describe the PHY property
  dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins
  dt-bindings: PCI: aardvark: Describe the reset-gpios property
  ARM64: dts: marvell: armada-37xx: declare PCIe clock
  ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY
  ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
  ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset
    pin

 .../devicetree/bindings/pci/aardvark-pci.txt  |  14 ++
 .../dts/marvell/armada-3720-espressobin.dts   |   3 +
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  10 +
 drivers/pci/controller/pci-aardvark.c         | 217 +++++++++++++++++-
 4 files changed, 243 insertions(+), 1 deletion(-)

-- 
2.19.1

[PATCH v3 01/15] PCI: aardvark: Enlarge PIO timeout

[Miquel Raynal]

This delay has been proven to work until now, however, there is a
reproducible way to fail the driver probe because of it.

Adding the support for the PCIe gated peripheral clock that feeds the
Aardvark IP defers the Aardvark driver probe at boot time. The probe
functions calls pci_host_probe() at its end. This is where the timeout
(sometimes) resides, leading to a kernel panic the next time a
register is accessed. Here is the function call sequence:

    advk_pcie_probe()
    pci_host_probe()
    pci_scan_root_bus_bridge()
    pci_scan_child_bus_extend()
    pci_scan_bridge_extend()
    pci_scan_child_bus_extend()
    pci_scan_slot()
    pcie_aspm_init_link_state()
    pcie_aspm_cap_init()

pcie_aspm_cap_init() is in charge of doing the initial ASPM state
setup (PCIe Active-State Power Management). The logic at the top of
the function is:

    1/ Read upstream and downstream components' register state.
    2/ Configure the common clock before checking latencies.
    3/ Read again upstream and downstream component's register state.
    4/ ...

Experimentation shows that while 1/ always work, 3/ sometimes timeouts
when reading the downstream components' register state. Each read
operation is handled by the following sequence:

    pcie_capability_read_dword()
    pci_bus_read_config_dword()
    advk_pcie_rd_conf()

advk_pcie_rd_conf() just configures a few registers and polls on the
ISR bit before reading the value that has been retrieved through the
interconnect. The polling timeout of the ISR bit is set to 1ms. While
this value seems to fit most of the situations, in our case it is
sometimes crossed when reading PCI_EXP_LNKCAP (PCI link capacities)
from the downstream component. It has been measured that most of the
time that the read lasts longer than 1ms, it rises ~24ms. In my tests,
using twice this delay always works. The root cause and the reason why
the timeout only appearing when the probe is delayed is unknown
though.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 750081c1cb48..18120e312ae1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -175,7 +175,7 @@
 	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))	| \
 	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
 
-#define PIO_TIMEOUT_MS			1
+#define PIO_TIMEOUT_MS			50
 
 #define LINK_WAIT_MAX_RETRIES		10
 #define LINK_WAIT_USLEEP_MIN		90000
-- 
2.19.1

[PATCH v3 02/15] PCI: aardvark: Configure more registers in the configuration helper

[Miquel Raynal]

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 <miquel.raynal@bootlin.com>
---
 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 18120e312ae1..0f50163ae072 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

[PATCH v3 03/15] PCI: aardvark: Add clock support

[Miquel Raynal]

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 <miquel.raynal@bootlin.com>
---
 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 0f50163ae072..b1b8897d2d28 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -8,6 +8,7 @@
  * Author: Hezi Shahmoon <hezi.shahmoon@marvell.com>
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -187,6 +188,7 @@
 
 struct advk_pcie {
 	struct platform_device *pdev;
+	struct clk *clk;
 	void __iomem *base;
 	struct list_head resources;
 	struct irq_domain *irq_domain;
@@ -973,6 +975,29 @@ static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
 	return err;
 }
 
+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;
@@ -1008,6 +1033,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = advk_pcie_setup_clk(pcie);
+	if (ret)
+		return ret;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.19.1

[PATCH v3 04/15] PCI: aardvark: Add PHY support

[Miquel Raynal]

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.

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 b1b8897d2d28..27ec79defa57 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_pci.h>
 
@@ -203,6 +204,7 @@ struct advk_pcie {
 	u16 msi_msg;
 	int root_bus_nr;
 	struct pci_bridge_emul bridge;
+	struct phy *phy;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -998,6 +1000,62 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
 	return ret;
 }
 
+static void __maybe_unused 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;
@@ -1037,6 +1095,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = advk_pcie_setup_phy(pcie);
+	if (ret)
+		return ret;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.19.1

[PATCH v3 05/15] PCI: aardvark: Add PCIe warm reset support

[Miquel Raynal]

Make use of the 'warm reset' register to ensure every peace of
hardware (core, phy, endpoint card) are in a known state before doing
the hardware setup.

The Aardvark IP can trigger a reset signal upon hot reset or link
failure that will only reach the components on the board without
affecting the entire device (eg. only the endpoint card). This line is
multiplexed on MPPs so if it is not used as PCI reset and multiplexed
for instance as a GPIO, the signals produced by the PCIe IP during the
warm reset operation won't affect the state of the line.

As usual, hardware designers can implement a card reset wired to a
GPIO. Support for such reset GPIO will be added in another patch.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 27ec79defa57..cfe48e553bca 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -139,6 +139,10 @@
 #define     CTRL_MODE_MASK			0x1
 #define     PCIE_CORE_MODE_DIRECT		0x0
 #define     PCIE_CORE_MODE_COMMAND		0x1
+#define CTRL_WARM_RESET_REG			(CTRL_CORE_BASE_ADDR + 0x4)
+#define     CTRL_PCIE_CORE_WARM_RESET		BIT(0)
+#define     CTRL_PHY_CORE_WARM_RESET		BIT(1)
+#define     CTRL_PERSTN_GPIO_EN			BIT(3)
 
 /* PCIe Central Interrupts Registers */
 #define CENTRAL_INT_BASE_ADDR			0x1b000
@@ -249,6 +253,19 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	u32 reg;
 
+	/* Warm reset */
+	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
+	reg |= CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET |
+	       CTRL_PERSTN_GPIO_EN;
+	advk_writel(pcie, reg, CTRL_WARM_RESET_REG);
+	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
+	mdelay(1);
+	reg &= ~(CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET |
+		 CTRL_PERSTN_GPIO_EN);
+	advk_writel(pcie, reg, CTRL_WARM_RESET_REG);
+	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
+	mdelay(10);
+
 	/* Set HW Reference Clock Buffer Control */
 	advk_writel(pcie, PCIE_PHY_REFCLK_BUF_CTRL, PCIE_PHY_REFCLK);
 
-- 
2.19.1

[PATCH v3 06/15] PCI: aardvark: Add external reset GPIO support

[Miquel Raynal]

Add support for a possible external reset GPIO wired to the PCIe
endpoint card. Asserting/deasserting the reset line is done during the
warm reset because the warm reset operation already triggers the
internal reset line that may also reset the endpoint card (if muxed).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 50 +++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index cfe48e553bca..3fb14e37eb59 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -10,6 +10,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -19,6 +20,7 @@
 #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>
 
 #include "../pci.h"
@@ -209,6 +211,7 @@ struct advk_pcie {
 	int root_bus_nr;
 	struct pci_bridge_emul bridge;
 	struct phy *phy;
+	struct gpio_desc *reset_gpio;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -249,6 +252,12 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
 	return -ETIMEDOUT;
 }
 
+static void advk_pcie_card_reset_assert(struct advk_pcie *pcie, bool status)
+{
+	if (pcie->reset_gpio)
+		gpiod_set_value_cansleep(pcie->reset_gpio, status);
+}
+
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	u32 reg;
@@ -257,11 +266,13 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
 	reg |= CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET |
 	       CTRL_PERSTN_GPIO_EN;
+	advk_pcie_card_reset_assert(pcie, 1);
 	advk_writel(pcie, reg, CTRL_WARM_RESET_REG);
 	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
 	mdelay(1);
 	reg &= ~(CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET |
 		 CTRL_PERSTN_GPIO_EN);
+	advk_pcie_card_reset_assert(pcie, 0);
 	advk_writel(pcie, reg, CTRL_WARM_RESET_REG);
 	reg = advk_readl(pcie, CTRL_WARM_RESET_REG);
 	mdelay(10);
@@ -1073,6 +1084,41 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
 	return ret;
 }
 
+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;
+
+	/* No all boards use an external GPIO for card reset */
+	if (!gpio_is_valid(gpio_nb))
+		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-card-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;
@@ -1116,6 +1162,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = advk_pcie_setup_reset_gpio(pcie);
+	if (ret)
+		return ret;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.19.1

[PATCH v3 07/15] PCI: aardvark: Add suspend to RAM support

[Miquel Raynal]

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 <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 45 ++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 3fb14e37eb59..999bbe27761a 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1028,7 +1028,7 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
 	return ret;
 }
 
-static void __maybe_unused advk_pcie_disable_phy(struct advk_pcie *pcie)
+static void advk_pcie_disable_phy(struct advk_pcie *pcie)
 {
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
@@ -1119,6 +1119,47 @@ static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
 	return 0;
 }
 
+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;
+
+	ret = advk_pcie_enable_phy(pcie);
+	if (ret)
+		return ret;
+
+	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;
@@ -1133,6 +1174,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->pdev = pdev;
+	platform_set_drvdata(pdev, pcie);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pcie->base = devm_ioremap_resource(dev, res);
@@ -1210,6 +1252,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

[PATCH v3 08/15] dt-bindings: PCI: aardvark: Describe the clocks property

[Miquel Raynal]

Describe the missing gated clock feeding the PCIe IP.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 310ef7145c47..9d19ebfea189 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
@@ -37,6 +38,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

[PATCH v3 09/15] dt-bindings: PCI: aardvark: Describe the PHY property

[Miquel Raynal]

Document the possibility to reference a PHY.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 9d19ebfea189..a440f182ccf8 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -21,6 +21,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:
+
+ - 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
 built into the PCIe controller. This sub-node must have the following
@@ -50,6 +54,7 @@ Example:
 				<0 0 0 2 &pcie_intc 1>,
 				<0 0 0 3 &pcie_intc 2>,
 				<0 0 0 4 &pcie_intc 3>;
+		phys = <&comphy1 0>;
 		pcie_intc: interrupt-controller {
 			interrupt-controller;
 			#interrupt-cells = <1>;
-- 
2.19.1

[PATCH v3 10/15] dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins

[Miquel Raynal]

A line might be used by the PCIe IP to reset the endpoint card upon:
  - platform reset,
  - hot reset,
  - link failure.

Describe the properties needed in this case (optional).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 a440f182ccf8..8b7f048705ec 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -24,6 +24,9 @@ contain the following properties:
 The following are optional properties:
 
  - phys: the PCIe PHY handle
+ - pinctrl-names: must be "default".
+ - pinctrl-0: pin control group to be used to mux the PCIe endpoint card
+   reset line so that it will be automatically driven by the PCIe IP.
 
 In addition, the Device Tree describing an Aardvark PCIe controller
 must include a sub-node that describes the legacy interrupt controller
@@ -55,6 +58,8 @@ Example:
 				<0 0 0 3 &pcie_intc 2>,
 				<0 0 0 4 &pcie_intc 3>;
 		phys = <&comphy1 0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pcie_card_reset_pins &pcie_clkreq_pins>;
 		pcie_intc: interrupt-controller {
 			interrupt-controller;
 			#interrupt-cells = <1>;
-- 
2.19.1

[PATCH v3 11/15] dt-bindings: PCI: aardvark: Describe the reset-gpios property

[Miquel Raynal]

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

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 8b7f048705ec..b47d6d608ae1 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -27,6 +27,8 @@ The following are optional properties:
  - pinctrl-names: must be "default".
  - pinctrl-0: pin control group to be used to mux the PCIe endpoint card
    reset line so that it will be automatically driven by the PCIe IP.
+ - reset-gpios: GPIO to reset the device (alternative to the above
+   entries that represent a pin controlled by the PCI IP itself).
 
 In addition, the Device Tree describing an Aardvark PCIe controller
 must include a sub-node that describes the legacy interrupt controller
-- 
2.19.1

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

[Miquel Raynal]

The PCIe IP is fed by a gated clock.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 65bf774516ec..f6f8d2b3b2c1 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -414,6 +414,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

[PATCH v3 13/15] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY

[Miquel Raynal]

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

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 094994a9c68e..c5c72902c647 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -46,6 +46,7 @@
 /* J9 */
 &pcie0 {
 	status = "okay";
+	phys = <&comphy1 0>;
 };
 
 /* J6 */
-- 
2.19.1

[PATCH v3 14/15] ARM64: dts: marvell: armada-37xx: declare PCIe reset pin

[Miquel Raynal]

One pin can be muxed as PCIe endpoint card reset.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index f6f8d2b3b2c1..91c0ec9c382b 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -295,6 +295,15 @@
 					function = "mii";
 				};
 
+				pcie_reset_pins: pcie-reset-pins {
+					groups = "pcie1";
+					function = "pcie";
+				};
+
+				pcie_clkreq_pins: pcie-clkreq-pins {
+					groups = "pcie1_clkreq";
+					function = "pcie";
+				};
 			};
 
 			eth0: ethernet@30000 {
-- 
2.19.1

[PATCH v3 15/15] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset pin

[Miquel Raynal]

Ensure the PCIe endpoint card reset that is toggled by the PCIe
controller itself is muxed correctly on the EspressoBin.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index c5c72902c647..f19c1ecd5703 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -47,6 +47,8 @@
 &pcie0 {
 	status = "okay";
 	phys = <&comphy1 0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 };
 
 /* J6 */
-- 
2.19.1

Re: [PATCH v3 10/15] dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins

[Rob Herring]

On Tue,  8 Jan 2019 17:24:35 +0100, Miquel Raynal wrote:
> A line might be used by the PCIe IP to reset the endpoint card upon:
>   - platform reset,
>   - hot reset,
>   - link failure.
> 
> Describe the properties needed in this case (optional).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  Documentation/devicetree/bindings/pci/aardvark-pci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 

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

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Gregory CLEMENT]

Hi Miquel,
 
 On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> 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]. Anyway, having the clock series applied first is not needed.
>
> 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/2019-January/623885.html
>
> Thanks,
> Miquèl
>
>
> Changes since v2:
> =================
> * Minor patches reordering.
> * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His
>   changes implied modifications in the DT/bindings patches adding PCIe
>   reset pin support.

Actually these patches are not in this series. You propably meant that
this series is depend on these patches.

If needed, for peoaple who wanted to test this series, the pinctrl
changes are now in linux-next and also in pinctrl/for-next.

Gregpry

> * Added a new patch that enlarges the PIO timeout of the driver
>   (explanations in the commit log).
> * With the timeout changed, removed the "experimental delay" that was
>   needed at resume time before accessing any register.
>
> Changes since v1:
> =================
> * Change the capitalization in commit titles to follow the PCI
>   subsystem rules.
> * Added Suggested-by tag to the patch adding PHY support and to the
>   patch adding the PHY property in the DT.
> * Added Rob's Reviewed-by tags on bindings.
> * I am following the discussion about calling functions that might
>   sleep in a NOIRQ context. As there is no real problem yet (as per my
>   understanding), I did not change anything on this regard.
>
>
> Miquel Raynal (15):
>   PCI: aardvark: Enlarge PIO timeout
>   PCI: aardvark: Configure more registers in the configuration helper
>   PCI: aardvark: Add clock support
>   PCI: aardvark: Add PHY support
>   PCI: aardvark: Add PCIe warm reset support
>   PCI: aardvark: Add external reset GPIO support
>   PCI: aardvark: Add suspend to RAM support
>   dt-bindings: PCI: aardvark: Describe the clocks property
>   dt-bindings: PCI: aardvark: Describe the PHY property
>   dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins
>   dt-bindings: PCI: aardvark: Describe the reset-gpios property
>   ARM64: dts: marvell: armada-37xx: declare PCIe clock
>   ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY
>   ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
>   ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset
>     pin
>
>  .../devicetree/bindings/pci/aardvark-pci.txt  |  14 ++
>  .../dts/marvell/armada-3720-espressobin.dts   |   3 +
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  10 +
>  drivers/pci/controller/pci-aardvark.c         | 217 +++++++++++++++++-
>  4 files changed, 243 insertions(+), 1 deletion(-)
>
> -- 
> 2.19.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Miquel Raynal]

Hi Gregory,

Gregory CLEMENT <gregory.clement@bootlin.com> wrote on Fri, 18 Jan 2019
17:51:52 +0100:

> Hi Miquel,
>  
>  On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> 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]. Anyway, having the clock series applied first is not needed.
> >
> > 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/2019-January/623885.html
> >
> > Thanks,
> > Miquèl
> >
> >
> > Changes since v2:
> > =================
> > * Minor patches reordering.
> > * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His
> >   changes implied modifications in the DT/bindings patches adding PCIe
> >   reset pin support.  
> 
> Actually these patches are not in this series. You propably meant that
> this series is depend on these patches.
> 
> If needed, for peoaple who wanted to test this series, the pinctrl
> changes are now in linux-next and also in pinctrl/for-next.

Absolutely, this is what I meant, thanks for clarifying.

Thanks,
Miquèl

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Lorenzo Pieralisi]

On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal 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]. Anyway, having the clock series applied first is not needed.

I do not understand what this means, in particular in relation
to the blocking clock calls in the suspend/resume NOIRQ hooks.

Thanks,
Lorenzo

> 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/2019-January/623885.html
> 
> Thanks,
> Miqu??l
> 
> 
> Changes since v2:
> =================
> * Minor patches reordering.
> * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His
>   changes implied modifications in the DT/bindings patches adding PCIe
>   reset pin support.
> * Added a new patch that enlarges the PIO timeout of the driver
>   (explanations in the commit log).
> * With the timeout changed, removed the "experimental delay" that was
>   needed at resume time before accessing any register.
> 
> Changes since v1:
> =================
> * Change the capitalization in commit titles to follow the PCI
>   subsystem rules.
> * Added Suggested-by tag to the patch adding PHY support and to the
>   patch adding the PHY property in the DT.
> * Added Rob's Reviewed-by tags on bindings.
> * I am following the discussion about calling functions that might
>   sleep in a NOIRQ context. As there is no real problem yet (as per my
>   understanding), I did not change anything on this regard.
> 
> 
> Miquel Raynal (15):
>   PCI: aardvark: Enlarge PIO timeout
>   PCI: aardvark: Configure more registers in the configuration helper
>   PCI: aardvark: Add clock support
>   PCI: aardvark: Add PHY support
>   PCI: aardvark: Add PCIe warm reset support
>   PCI: aardvark: Add external reset GPIO support
>   PCI: aardvark: Add suspend to RAM support
>   dt-bindings: PCI: aardvark: Describe the clocks property
>   dt-bindings: PCI: aardvark: Describe the PHY property
>   dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins
>   dt-bindings: PCI: aardvark: Describe the reset-gpios property
>   ARM64: dts: marvell: armada-37xx: declare PCIe clock
>   ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY
>   ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
>   ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset
>     pin
> 
>  .../devicetree/bindings/pci/aardvark-pci.txt  |  14 ++
>  .../dts/marvell/armada-3720-espressobin.dts   |   3 +
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  10 +
>  drivers/pci/controller/pci-aardvark.c         | 217 +++++++++++++++++-
>  4 files changed, 243 insertions(+), 1 deletion(-)
> 
> -- 
> 2.19.1
> 

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Miquel Raynal]

Hi Lorenzo,

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019
17:05:09 +0000:

> On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal 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]. Anyway, having the clock series applied first is not needed.  
> 
> I do not understand what this means, in particular in relation
> to the blocking clock calls in the suspend/resume NOIRQ hooks.

I am not sure to understand your question.

As there are multiple points in this sentence I will detail each of
them so please comment on the one which is bothering you:
* I am working in parallel on a series adding device links to the clock
  framework. This way when a driver consumes a clock, the clock
  provider driver will be resumed first.
* If the clock series I am talking about is applied after this one,
  there is no build issue. Of course suspending the platform may
  not work but this is a new feature so nothing will be broken.
* Device links do not enforce any priority if the suspend/resume phase
  between two drivers is not the same. The PCIe driver suspends in the
  NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its
  suspend/resume callbacks must be promoted to the NOIRQ phase as well
  (and this is part of another series). As of today there is
  no alternative.


Thanks,
Miquèl

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Lorenzo Pieralisi]

On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote:
> Hi Lorenzo,
> 
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019
> 17:05:09 +0000:
> 
> > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal 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]. Anyway, having the clock series applied first is not needed.  
> > 
> > I do not understand what this means, in particular in relation
> > to the blocking clock calls in the suspend/resume NOIRQ hooks.
> 
> I am not sure to understand your question.
> 
> As there are multiple points in this sentence I will detail each of
> them so please comment on the one which is bothering you:
> * I am working in parallel on a series adding device links to the clock
>   framework. This way when a driver consumes a clock, the clock
>   provider driver will be resumed first.
> * If the clock series I am talking about is applied after this one,
>   there is no build issue. Of course suspending the platform may
>   not work but this is a new feature so nothing will be broken.

Suspend to RAM will be broken if the clock is suspended and no
notification will happen in the NOIRQ phase, it is a new-broken-feature.

> * Device links do not enforce any priority if the suspend/resume phase
>   between two drivers is not the same. The PCIe driver suspends in the
>   NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its
>   suspend/resume callbacks must be promoted to the NOIRQ phase as well
>   (and this is part of another series). As of today there is
>   no alternative.

I will merge this series when it works, I have no evidence that it does
given what you are writing above, if the series you mention are
*necessary* for suspend-to-RAM to work they ought to be merged first.

Thanks,
Lorenzo

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Miquel Raynal]

Hi Lorenzo,

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Fri, 25 Jan 2019
12:40:11 +0000:

> On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote:
> > Hi Lorenzo,
> > 
> > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019
> > 17:05:09 +0000:
> >   
> > > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal 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]. Anyway, having the clock series applied first is not needed.    
> > > 
> > > I do not understand what this means, in particular in relation
> > > to the blocking clock calls in the suspend/resume NOIRQ hooks.  
> > 
> > I am not sure to understand your question.
> > 
> > As there are multiple points in this sentence I will detail each of
> > them so please comment on the one which is bothering you:
> > * I am working in parallel on a series adding device links to the clock
> >   framework. This way when a driver consumes a clock, the clock
> >   provider driver will be resumed first.
> > * If the clock series I am talking about is applied after this one,
> >   there is no build issue. Of course suspending the platform may
> >   not work but this is a new feature so nothing will be broken.  
> 
> Suspend to RAM will be broken if the clock is suspended and no
> notification will happen in the NOIRQ phase, it is a new-broken-feature.
>
> 
> > * Device links do not enforce any priority if the suspend/resume phase
> >   between two drivers is not the same. The PCIe driver suspends in the
> >   NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its
> >   suspend/resume callbacks must be promoted to the NOIRQ phase as well
> >   (and this is part of another series). As of today there is
> >   no alternative.  
> 
> I will merge this series when it works, I have no evidence that it does
> given what you are writing above, if the series you mention are
> *necessary* for suspend-to-RAM to work they ought to be merged first.

I am working actively to bring A3700 SoC suspend to RAM support almost
from scratch. 

As of today I have contributed 65 patches spread in 8 series for the
PHY, clk, PCIe, SATA, USB, pinctrl and net subsystems. Some of them
have been merged, but the vast majority has not, yet.

I mentioned this run-time dependency because it exists for people who
would like to test just the PCIe IP. But S2RAM on A3700 will be a
new-broken-feature until all patches are merged. While there is
still one missing, the feature is broken. If everybody waits for
the other patches to be merged first, it is gonna be a long process :)

However, if you want to wait for the clock core series to be applied
first I respect this choice and I will update you when it will be the
case.


Thanks,
Miquèl

Re: [PATCH v3 00/15] Bring suspend to RAM support to PCIe Aardvark driver

[Lorenzo Pieralisi]

On Fri, Jan 25, 2019 at 01:57:57PM +0100, Miquel Raynal wrote:
> Hi Lorenzo,
> 
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Fri, 25 Jan 2019
> 12:40:11 +0000:
> 
> > On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote:
> > > Hi Lorenzo,
> > > 
> > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019
> > > 17:05:09 +0000:
> > >   
> > > > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal 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]. Anyway, having the clock series applied first is not needed.    
> > > > 
> > > > I do not understand what this means, in particular in relation
> > > > to the blocking clock calls in the suspend/resume NOIRQ hooks.  
> > > 
> > > I am not sure to understand your question.
> > > 
> > > As there are multiple points in this sentence I will detail each of
> > > them so please comment on the one which is bothering you:
> > > * I am working in parallel on a series adding device links to the clock
> > >   framework. This way when a driver consumes a clock, the clock
> > >   provider driver will be resumed first.
> > > * If the clock series I am talking about is applied after this one,
> > >   there is no build issue. Of course suspending the platform may
> > >   not work but this is a new feature so nothing will be broken.  
> > 
> > Suspend to RAM will be broken if the clock is suspended and no
> > notification will happen in the NOIRQ phase, it is a new-broken-feature.
> >
> > 
> > > * Device links do not enforce any priority if the suspend/resume phase
> > >   between two drivers is not the same. The PCIe driver suspends in the
> > >   NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its
> > >   suspend/resume callbacks must be promoted to the NOIRQ phase as well
> > >   (and this is part of another series). As of today there is
> > >   no alternative.  
> > 
> > I will merge this series when it works, I have no evidence that it does
> > given what you are writing above, if the series you mention are
> > *necessary* for suspend-to-RAM to work they ought to be merged first.
> 
> I am working actively to bring A3700 SoC suspend to RAM support almost
> from scratch. 
> 
> As of today I have contributed 65 patches spread in 8 series for the
> PHY, clk, PCIe, SATA, USB, pinctrl and net subsystems. Some of them
> have been merged, but the vast majority has not, yet.
> 
> I mentioned this run-time dependency because it exists for people who
> would like to test just the PCIe IP. But S2RAM on A3700 will be a
> new-broken-feature until all patches are merged. While there is
> still one missing, the feature is broken. If everybody waits for
> the other patches to be merged first, it is gonna be a long process :)
> 
> However, if you want to wait for the clock core series to be applied
> first I respect this choice and I will update you when it will be the
> case.

I do not want to merge new code with known issues, it makes no sense.

There are other drivers with a similar problem in the mainline, we
ought to fix those but certainly adding more won't help in that respect.

There is nothing I am saying here that goes against your work, I just
want to have working code in the mainlike kernel, we will merge this
series when it is ready, thanks for bearing with me.

Feel free to ping me/rebase/repost when dependencies are sorted.

Thanks,
Lorenzo

Re: [PATCH v3 14/15] ARM64: dts: marvell: armada-37xx: declare PCIe reset pin

[Gregory CLEMENT]

Hi Miquel,
 
 On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> One pin can be muxed as PCIe endpoint card reset.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to mvebu/dt64

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index f6f8d2b3b2c1..91c0ec9c382b 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -295,6 +295,15 @@
>  					function = "mii";
>  				};
>  
> +				pcie_reset_pins: pcie-reset-pins {
> +					groups = "pcie1";
> +					function = "pcie";
> +				};
> +
> +				pcie_clkreq_pins: pcie-clkreq-pins {
> +					groups = "pcie1_clkreq";
> +					function = "pcie";
> +				};
>  			};
>  
>  			eth0: ethernet@30000 {
> -- 
> 2.19.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v3 15/15] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset pin

[Gregory CLEMENT]

Hi Miquel,
 
 On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Ensure the PCIe endpoint card reset that is toggled by the PCIe
> controller itself is muxed correctly on the EspressoBin.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to mvebu/dt64

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> index c5c72902c647..f19c1ecd5703 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -47,6 +47,8 @@
>  &pcie0 {
>  	status = "okay";
>  	phys = <&comphy1 0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
>  };
>  
>  /* J6 */
> -- 
> 2.19.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v3 13/15] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY

[Gregory CLEMENT]

Hi Miquel,
 
 On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> The PCIe node is wired to the second PHY of the COMPHY IP.
>
> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to mvebu/dt64

Thanks,

Gregory

> ---
>  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 094994a9c68e..c5c72902c647 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -46,6 +46,7 @@
>  /* J9 */
>  &pcie0 {
>  	status = "okay";
> +	phys = <&comphy1 0>;
>  };
>  
>  /* J6 */
> -- 
> 2.19.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com