[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The first patch fixup the mtk_pcie_find_port logical which will cause system
could not touch the EP's configuration space which was connected to PCIe slot 1.

The second patch fixup the enable msi logical, the operation to enable msi
should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

The third patch was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebase on the previous two patches.

The fourth patch add loadable kernel module support.

Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
so I just add the Reviewed-by tags in those patches.

[1] https://patchwork.kernel.org/patch/10479079

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.

Honghui Zhang (4):
  PCI: mediatek: fixup mtk_pcie_find_port logical
  PCI: mediatek: enable msi after clock enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
 2 files changed, 213 insertions(+), 78 deletions(-)

-- 
2.6.4

[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The first patch fixup the mtk_pcie_find_port logical which will cause system
could not touch the EP's configuration space which was connected to PCIe slot 1.

The second patch fixup the enable msi logical, the operation to enable msi
should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

The third patch was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebase on the previous two patches.

The fourth patch add loadable kernel module support.

Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
so I just add the Reviewed-by tags in those patches.

[1] https://patchwork.kernel.org/patch/10479079

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.

Honghui Zhang (4):
  PCI: mediatek: fixup mtk_pcie_find_port logical
  PCI: mediatek: enable msi after clock enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
 2 files changed, 213 insertions(+), 78 deletions(-)

-- 
2.6.4

[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The first patch fixup the mtk_pcie_find_port logical which will cause system
could not touch the EP's configuration space which was connected to PCIe slot 1.

The second patch fixup the enable msi logical, the operation to enable msi
should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

The third patch was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebase on the previous two patches.

The fourth patch add loadable kernel module support.

Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
so I just add the Reviewed-by tags in those patches.

[1] https://patchwork.kernel.org/patch/10479079

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.

Honghui Zhang (4):
  PCI: mediatek: fixup mtk_pcie_find_port logical
  PCI: mediatek: enable msi after clock enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
 2 files changed, 213 insertions(+), 78 deletions(-)

-- 
2.6.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The first patch fixup the mtk_pcie_find_port logical which will cause system
could not touch the EP's configuration space which was connected to PCIe slot 1.

The second patch fixup the enable msi logical, the operation to enable msi
should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

The third patch was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebase on the previous two patches.

The fourth patch add loadable kernel module support.

Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
so I just add the Reviewed-by tags in those patches.

[1] https://patchwork.kernel.org/patch/10479079

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.

Honghui Zhang (4):
  PCI: mediatek: fixup mtk_pcie_find_port logical
  PCI: mediatek: enable msi after clock enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
 2 files changed, 213 insertions(+), 78 deletions(-)

-- 
2.6.4

[PATCH v3 1/4] PCI: mediatek: fixup mtk_pcie_find_port logical

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logical by using the slot's
devfn for match.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..b43f41d 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,11 +337,26 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
 
-	list_for_each_entry(port, &pcie->ports, list)
-		if (port->slot == PCI_SLOT(devfn))
+	list_for_each_entry(port, &pcie->ports, list) {
+		if (!bus->number && port->slot == PCI_SLOT(devfn))
 			return port;
 
+		if (bus->number) {
+			pbus = bus;
+
+			while (pbus->number) {
+				dev = pbus->self;
+				pbus = dev->bus;
+			}
+
+			if (port->slot == PCI_SLOT(dev->devfn))
+				return port;
+		}
+	}
+
 	return NULL;
 }
 
-- 
2.6.4

[PATCH v3 1/4] PCI: mediatek: fixup mtk_pcie_find_port logical

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logical by using the slot's
devfn for match.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..b43f41d 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,11 +337,26 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
 
-	list_for_each_entry(port, &pcie->ports, list)
-		if (port->slot == PCI_SLOT(devfn))
+	list_for_each_entry(port, &pcie->ports, list) {
+		if (!bus->number && port->slot == PCI_SLOT(devfn))
 			return port;
 
+		if (bus->number) {
+			pbus = bus;
+
+			while (pbus->number) {
+				dev = pbus->self;
+				pbus = dev->bus;
+			}
+
+			if (port->slot == PCI_SLOT(dev->devfn))
+				return port;
+		}
+	}
+
 	return NULL;
 }
 
-- 
2.6.4

[PATCH v3 1/4] PCI: mediatek: fixup mtk_pcie_find_port logical

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logical by using the slot's
devfn for match.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..b43f41d 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,11 +337,26 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
 
-	list_for_each_entry(port, &pcie->ports, list)
-		if (port->slot == PCI_SLOT(devfn))
+	list_for_each_entry(port, &pcie->ports, list) {
+		if (!bus->number && port->slot == PCI_SLOT(devfn))
 			return port;
 
+		if (bus->number) {
+			pbus = bus;
+
+			while (pbus->number) {
+				dev = pbus->self;
+				pbus = dev->bus;
+			}
+
+			if (port->slot == PCI_SLOT(dev->devfn))
+				return port;
+		}
+	}
+
 	return NULL;
 }
 
-- 
2.6.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/4] PCI: mediatek: fixup mtk_pcie_find_port logical

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logical by using the slot's
devfn for match.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..b43f41d 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,11 +337,26 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
 
-	list_for_each_entry(port, &pcie->ports, list)
-		if (port->slot == PCI_SLOT(devfn))
+	list_for_each_entry(port, &pcie->ports, list) {
+		if (!bus->number && port->slot == PCI_SLOT(devfn))
 			return port;
 
+		if (bus->number) {
+			pbus = bus;
+
+			while (pbus->number) {
+				dev = pbus->self;
+				pbus = dev->bus;
+			}
+
+			if (port->slot == PCI_SLOT(dev->devfn))
+				return port;
+		}
+	}
+
 	return NULL;
 }
 
-- 
2.6.4

[PATCH v3 2/4] PCI: mediatek: enable msi after clock enabled

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The clocks was not enabled when enable MSI. This patch fix this
issue by calling mtk_pcie_enable_msi in mtk_pcie_startup_port_v2
since the clock was all enabled at that time.

The function of mtk_pcie_startup_port_v2's define location is
re-arranged to avoid mtk_pcie_enable_msi's forward declaration.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 143 +++++++++++++++++----------------
 1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index b43f41d..86918d4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -398,75 +398,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_HOST;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -643,8 +574,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -711,6 +640,78 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_HOST;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v3 2/4] PCI: mediatek: enable msi after clock enabled

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The clocks was not enabled when enable MSI. This patch fix this
issue by calling mtk_pcie_enable_msi in mtk_pcie_startup_port_v2
since the clock was all enabled at that time.

The function of mtk_pcie_startup_port_v2's define location is
re-arranged to avoid mtk_pcie_enable_msi's forward declaration.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 143 +++++++++++++++++----------------
 1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index b43f41d..86918d4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -398,75 +398,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_HOST;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -643,8 +574,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -711,6 +640,78 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_HOST;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v3 2/4] PCI: mediatek: enable msi after clock enabled

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The clocks was not enabled when enable MSI. This patch fix this
issue by calling mtk_pcie_enable_msi in mtk_pcie_startup_port_v2
since the clock was all enabled at that time.

The function of mtk_pcie_startup_port_v2's define location is
re-arranged to avoid mtk_pcie_enable_msi's forward declaration.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 143 +++++++++++++++++----------------
 1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index b43f41d..86918d4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -398,75 +398,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_HOST;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -643,8 +574,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -711,6 +640,78 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_HOST;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
suspend, and all the internal control register will be reset after system
resume. The PCIe link should be re-established and the related control
register values should be re-set after system resume.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 86918d4..175d7b6 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -134,12 +134,14 @@ struct mtk_pcie_port;
 /**
  * struct mtk_pcie_soc - differentiate between host generations
  * @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
  * @ops: pointer to configuration access functions
  * @startup: pointer to controller setting functions
  * @setup_irq: pointer to initialize IRQ functions
  */
 struct mtk_pcie_soc {
 	bool need_fix_class_id;
+	bool pm_support;
 	struct pci_ops *ops;
 	int (*startup)(struct mtk_pcie_port *port);
 	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port, *tmp;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	if (dev->pm_domain) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
 };
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
 	.need_fix_class_id = true,
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
suspend, and all the internal control register will be reset after system
resume. The PCIe link should be re-established and the related control
register values should be re-set after system resume.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 86918d4..175d7b6 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -134,12 +134,14 @@ struct mtk_pcie_port;
 /**
  * struct mtk_pcie_soc - differentiate between host generations
  * @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
  * @ops: pointer to configuration access functions
  * @startup: pointer to controller setting functions
  * @setup_irq: pointer to initialize IRQ functions
  */
 struct mtk_pcie_soc {
 	bool need_fix_class_id;
+	bool pm_support;
 	struct pci_ops *ops;
 	int (*startup)(struct mtk_pcie_port *port);
 	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port, *tmp;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	if (dev->pm_domain) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
 };
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
 	.need_fix_class_id = true,
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
suspend, and all the internal control register will be reset after system
resume. The PCIe link should be re-established and the related control
register values should be re-set after system resume.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 86918d4..175d7b6 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -134,12 +134,14 @@ struct mtk_pcie_port;
 /**
  * struct mtk_pcie_soc - differentiate between host generations
  * @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
  * @ops: pointer to configuration access functions
  * @startup: pointer to controller setting functions
  * @setup_irq: pointer to initialize IRQ functions
  */
 struct mtk_pcie_soc {
 	bool need_fix_class_id;
+	bool pm_support;
 	struct pci_ops *ops;
 	int (*startup)(struct mtk_pcie_port *port);
 	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port, *tmp;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	if (dev->pm_domain) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
 };
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
 	.need_fix_class_id = true,
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
suspend, and all the internal control register will be reset after system
resume. The PCIe link should be re-established and the related control
register values should be re-set after system resume.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 86918d4..175d7b6 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -134,12 +134,14 @@ struct mtk_pcie_port;
 /**
  * struct mtk_pcie_soc - differentiate between host generations
  * @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
  * @ops: pointer to configuration access functions
  * @startup: pointer to controller setting functions
  * @setup_irq: pointer to initialize IRQ functions
  */
 struct mtk_pcie_soc {
 	bool need_fix_class_id;
+	bool pm_support;
 	struct pci_ops *ops;
 	int (*startup)(struct mtk_pcie_port *port);
 	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port, *tmp;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	if (dev->pm_domain) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		mtk_pcie_subsys_powerdown(pcie);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
 };
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
 
 static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
 	.need_fix_class_id = true,
+	.pm_support = true,
 	.ops = &mtk_pcie_ops_v2,
 	.startup = mtk_pcie_startup_port_v2,
 	.setup_irq = mtk_pcie_setup_irq,
@@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v3 4/4] PCI: mediatek: Add loadable kernel module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 60 +++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b..6c61ac65 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -234,7 +234,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 175d7b6..28730b8 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -184,6 +185,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -538,6 +540,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -628,7 +651,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -636,8 +659,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
@@ -1199,6 +1223,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1291,6 +1341,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1298,4 +1349,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

[PATCH v3 4/4] PCI: mediatek: Add loadable kernel module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 60 +++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b..6c61ac65 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -234,7 +234,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 175d7b6..28730b8 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -184,6 +185,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -538,6 +540,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -628,7 +651,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -636,8 +659,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
@@ -1199,6 +1223,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1291,6 +1341,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1298,4 +1349,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

[PATCH v3 4/4] PCI: mediatek: Add loadable kernel module support

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 60 +++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b..6c61ac65 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -234,7 +234,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 175d7b6..28730b8 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -184,6 +185,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -538,6 +540,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -628,7 +651,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -636,8 +659,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
@@ -1199,6 +1223,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1291,6 +1341,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1298,4 +1349,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

Re: [PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

Hi, Bjorn, Lorenzo,

could you kindly take a look at this serial?

thanks.

On Mon, 2018-07-02 at 15:57 +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> This patchset includes misc patchs:
> 
> The first patch fixup the mtk_pcie_find_port logical which will cause system
> could not touch the EP's configuration space which was connected to PCIe slot 1.
> 
> The second patch fixup the enable msi logical, the operation to enable msi
> should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
> define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
> And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
> enabled at that time.
> 
> The third patch was rebased and refactor of the v4 patch[1], changes are:
>  -Add PM support for MT7622.
>  -Using mtk_pcie_enable_port to re-establish the link when resumed.
>  -Rebase on the previous two patches.
> 
> The fourth patch add loadable kernel module support.
> 
> Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
> so I just add the Reviewed-by tags in those patches.
> 
> [1] https://patchwork.kernel.org/patch/10479079
> 
> Change since v2:
>  - Fix the list_for_each_entry_safe parameter error.
>  - Add Ryder's Acked-by flag.
> 
> Change since v1:
>  - A bit of code refact of the first patch suggested by Andy Shevchenko, and
>    commit message updated.
>  - Using __maybe_unused.
>  - Remove the redundant list_empty check of the fourth patch.
> 
> Honghui Zhang (4):
>   PCI: mediatek: fixup mtk_pcie_find_port logical
>   PCI: mediatek: enable msi after clock enabled
>   PCI: mediatek: Add system pm support for MT2712 and MT7622
>   PCI: mediatek: Add loadable kernel module support
> 
>  drivers/pci/controller/Kconfig         |   2 +-
>  drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
>  2 files changed, 213 insertions(+), 78 deletions(-)
> 

Re: [PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

Hi, Bjorn, Lorenzo,

could you kindly take a look at this serial?

thanks.

On Mon, 2018-07-02 at 15:57 +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> This patchset includes misc patchs:
> 
> The first patch fixup the mtk_pcie_find_port logical which will cause system
> could not touch the EP's configuration space which was connected to PCIe slot 1.
> 
> The second patch fixup the enable msi logical, the operation to enable msi
> should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
> define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
> And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
> enabled at that time.
> 
> The third patch was rebased and refactor of the v4 patch[1], changes are:
>  -Add PM support for MT7622.
>  -Using mtk_pcie_enable_port to re-establish the link when resumed.
>  -Rebase on the previous two patches.
> 
> The fourth patch add loadable kernel module support.
> 
> Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
> so I just add the Reviewed-by tags in those patches.
> 
> [1] https://patchwork.kernel.org/patch/10479079
> 
> Change since v2:
>  - Fix the list_for_each_entry_safe parameter error.
>  - Add Ryder's Acked-by flag.
> 
> Change since v1:
>  - A bit of code refact of the first patch suggested by Andy Shevchenko, and
>    commit message updated.
>  - Using __maybe_unused.
>  - Remove the redundant list_empty check of the fourth patch.
> 
> Honghui Zhang (4):
>   PCI: mediatek: fixup mtk_pcie_find_port logical
>   PCI: mediatek: enable msi after clock enabled
>   PCI: mediatek: Add system pm support for MT2712 and MT7622
>   PCI: mediatek: Add loadable kernel module support
> 
>  drivers/pci/controller/Kconfig         |   2 +-
>  drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
>  2 files changed, 213 insertions(+), 78 deletions(-)
> 

Re: [PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

Hi, Bjorn, Lorenzo,

could you kindly take a look at this serial?

thanks.

On Mon, 2018-07-02 at 15:57 +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> This patchset includes misc patchs:
> 
> The first patch fixup the mtk_pcie_find_port logical which will cause system
> could not touch the EP's configuration space which was connected to PCIe slot 1.
> 
> The second patch fixup the enable msi logical, the operation to enable msi
> should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
> define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
> And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
> enabled at that time.
> 
> The third patch was rebased and refactor of the v4 patch[1], changes are:
>  -Add PM support for MT7622.
>  -Using mtk_pcie_enable_port to re-establish the link when resumed.
>  -Rebase on the previous two patches.
> 
> The fourth patch add loadable kernel module support.
> 
> Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
> so I just add the Reviewed-by tags in those patches.
> 
> [1] https://patchwork.kernel.org/patch/10479079
> 
> Change since v2:
>  - Fix the list_for_each_entry_safe parameter error.
>  - Add Ryder's Acked-by flag.
> 
> Change since v1:
>  - A bit of code refact of the first patch suggested by Andy Shevchenko, and
>    commit message updated.
>  - Using __maybe_unused.
>  - Remove the redundant list_empty check of the fourth patch.
> 
> Honghui Zhang (4):
>   PCI: mediatek: fixup mtk_pcie_find_port logical
>   PCI: mediatek: enable msi after clock enabled
>   PCI: mediatek: Add system pm support for MT2712 and MT7622
>   PCI: mediatek: Add loadable kernel module support
> 
>  drivers/pci/controller/Kconfig         |   2 +-
>  drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
>  2 files changed, 213 insertions(+), 78 deletions(-)
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 0/4] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

Hi, Bjorn, Lorenzo,

could you kindly take a look at this serial?

thanks.

On Mon, 2018-07-02 at 15:57 +0800, honghui.zhang at mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> This patchset includes misc patchs:
> 
> The first patch fixup the mtk_pcie_find_port logical which will cause system
> could not touch the EP's configuration space which was connected to PCIe slot 1.
> 
> The second patch fixup the enable msi logical, the operation to enable msi
> should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
> define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
> And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
> enabled at that time.
> 
> The third patch was rebased and refactor of the v4 patch[1], changes are:
>  -Add PM support for MT7622.
>  -Using mtk_pcie_enable_port to re-establish the link when resumed.
>  -Rebase on the previous two patches.
> 
> The fourth patch add loadable kernel module support.
> 
> Some of those patches was already reviewed-by Ryder Lee <ryder.lee@mediatek.com>,
> so I just add the Reviewed-by tags in those patches.
> 
> [1] https://patchwork.kernel.org/patch/10479079
> 
> Change since v2:
>  - Fix the list_for_each_entry_safe parameter error.
>  - Add Ryder's Acked-by flag.
> 
> Change since v1:
>  - A bit of code refact of the first patch suggested by Andy Shevchenko, and
>    commit message updated.
>  - Using __maybe_unused.
>  - Remove the redundant list_empty check of the fourth patch.
> 
> Honghui Zhang (4):
>   PCI: mediatek: fixup mtk_pcie_find_port logical
>   PCI: mediatek: enable msi after clock enabled
>   PCI: mediatek: Add system pm support for MT2712 and MT7622
>   PCI: mediatek: Add loadable kernel module support
> 
>  drivers/pci/controller/Kconfig         |   2 +-
>  drivers/pci/controller/pcie-mediatek.c | 289 ++++++++++++++++++++++++---------
>  2 files changed, 213 insertions(+), 78 deletions(-)
> 

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

[+Rafael, Kevin, Ulf]

On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> suspend, and all the internal control register will be reset after system
> resume. The PCIe link should be re-established and the related control
> register values should be re-set after system resume.
> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 86918d4..175d7b6 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>  	bool need_fix_class_id;
> +	bool pm_support;
>  	struct pci_ops *ops;
>  	int (*startup)(struct mtk_pcie_port *port);
>  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +	const struct mtk_pcie_soc *soc = pcie->soc;
> +	struct mtk_pcie_port *port;
> +
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	if (list_empty(&pcie->ports))
> +		return 0;
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		clk_disable_unprepare(port->pipe_ck);
> +		clk_disable_unprepare(port->obff_ck);
> +		clk_disable_unprepare(port->axi_ck);
> +		clk_disable_unprepare(port->aux_ck);
> +		clk_disable_unprepare(port->ahb_ck);
> +		clk_disable_unprepare(port->sys_ck);
> +		phy_power_off(port->phy);
> +		phy_exit(port->phy);
> +	}
> +
> +	mtk_pcie_subsys_powerdown(pcie);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> +{
> +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +	const struct mtk_pcie_soc *soc = pcie->soc;
> +	struct mtk_pcie_port *port, *tmp;
> +
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	if (list_empty(&pcie->ports))
> +		return 0;
> +
> +	if (dev->pm_domain) {
> +		pm_runtime_enable(dev);
> +		pm_runtime_get_sync(dev);
> +	}

Are these runtime PM calls needed/abused here ?

Mind explaining the logic ?

There is certainly an asymmetry with the suspend callback which made me
suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
that we can make progress with this patch.

Lorenzo

> +
> +	clk_prepare_enable(pcie->free_ck);
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		mtk_pcie_enable_port(port);
> +
> +	/* In case of EP was removed while system suspend. */
> +	if (list_empty(&pcie->ports))
> +		mtk_pcie_subsys_powerdown(pcie);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> +				      mtk_pcie_resume_noirq)
> +};
> +
>  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
>  	.ops = &mtk_pcie_ops,
>  	.startup = mtk_pcie_startup_port,
>  };
>  
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> +	.pm_support = true,
>  	.ops = &mtk_pcie_ops_v2,
>  	.startup = mtk_pcie_startup_port_v2,
>  	.setup_irq = mtk_pcie_setup_irq,
> @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>  
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
>  	.need_fix_class_id = true,
> +	.pm_support = true,
>  	.ops = &mtk_pcie_ops_v2,
>  	.startup = mtk_pcie_startup_port_v2,
>  	.setup_irq = mtk_pcie_setup_irq,
> @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
>  		.name = "mtk-pcie",
>  		.of_match_table = mtk_pcie_ids,
>  		.suppress_bind_attrs = true,
> +		.pm = &mtk_pcie_pm_ops,
>  	},
>  };
>  builtin_platform_driver(mtk_pcie_driver);
> -- 
> 2.6.4
> 

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

[+Rafael, Kevin, Ulf]

On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang at mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> suspend, and all the internal control register will be reset after system
> resume. The PCIe link should be re-established and the related control
> register values should be re-set after system resume.
> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 86918d4..175d7b6 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>  	bool need_fix_class_id;
> +	bool pm_support;
>  	struct pci_ops *ops;
>  	int (*startup)(struct mtk_pcie_port *port);
>  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +	const struct mtk_pcie_soc *soc = pcie->soc;
> +	struct mtk_pcie_port *port;
> +
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	if (list_empty(&pcie->ports))
> +		return 0;
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		clk_disable_unprepare(port->pipe_ck);
> +		clk_disable_unprepare(port->obff_ck);
> +		clk_disable_unprepare(port->axi_ck);
> +		clk_disable_unprepare(port->aux_ck);
> +		clk_disable_unprepare(port->ahb_ck);
> +		clk_disable_unprepare(port->sys_ck);
> +		phy_power_off(port->phy);
> +		phy_exit(port->phy);
> +	}
> +
> +	mtk_pcie_subsys_powerdown(pcie);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> +{
> +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +	const struct mtk_pcie_soc *soc = pcie->soc;
> +	struct mtk_pcie_port *port, *tmp;
> +
> +	if (!soc->pm_support)
> +		return 0;
> +
> +	if (list_empty(&pcie->ports))
> +		return 0;
> +
> +	if (dev->pm_domain) {
> +		pm_runtime_enable(dev);
> +		pm_runtime_get_sync(dev);
> +	}

Are these runtime PM calls needed/abused here ?

Mind explaining the logic ?

There is certainly an asymmetry with the suspend callback which made me
suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
that we can make progress with this patch.

Lorenzo

> +
> +	clk_prepare_enable(pcie->free_ck);
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		mtk_pcie_enable_port(port);
> +
> +	/* In case of EP was removed while system suspend. */
> +	if (list_empty(&pcie->ports))
> +		mtk_pcie_subsys_powerdown(pcie);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> +				      mtk_pcie_resume_noirq)
> +};
> +
>  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
>  	.ops = &mtk_pcie_ops,
>  	.startup = mtk_pcie_startup_port,
>  };
>  
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> +	.pm_support = true,
>  	.ops = &mtk_pcie_ops_v2,
>  	.startup = mtk_pcie_startup_port_v2,
>  	.setup_irq = mtk_pcie_setup_irq,
> @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>  
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
>  	.need_fix_class_id = true,
> +	.pm_support = true,
>  	.ops = &mtk_pcie_ops_v2,
>  	.startup = mtk_pcie_startup_port_v2,
>  	.setup_irq = mtk_pcie_setup_irq,
> @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
>  		.name = "mtk-pcie",
>  		.of_match_table = mtk_pcie_ids,
>  		.suppress_bind_attrs = true,
> +		.pm = &mtk_pcie_pm_ops,
>  	},
>  };
>  builtin_platform_driver(mtk_pcie_driver);
> -- 
> 2.6.4
> 

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Tue, 2018-07-17 at 18:15 +0100, Lorenzo Pieralisi wrote:
> [+Rafael, Kevin, Ulf]
> 
> On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->pipe_ck);
> > +		clk_disable_unprepare(port->obff_ck);
> > +		clk_disable_unprepare(port->axi_ck);
> > +		clk_disable_unprepare(port->aux_ck);
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +		phy_exit(port->phy);
> > +	}
> > +
> > +	mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port, *tmp;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	if (dev->pm_domain) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> > +	}
> 
> Are these runtime PM calls needed/abused here ?
> 
> Mind explaining the logic ?
> 
> There is certainly an asymmetry with the suspend callback which made me
> suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> that we can make progress with this patch.
> 
> Lorenzo
> 
Hi Lorenzo, thanks for your comments.
Sorry I don't get you.
I believe that in suspend callbacks the pm_runtime_put_sync and
pm_runtime_disable should be called to gated the CMOS for this module,
while the pm_rumtime_enable and pm_rumtime_get_sync should be called in
resume callback.

That's exactly this patch doing.
But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
in the mtk_pcie_subsys_powerdown.

I did not call mtk_pcie_subsys_powerup since it does not just wrapped
pm_rumtime related functions but also do the platform_resource_get,
devm_ioremap, and free_ck clock get which I do not needed in resume
callback.

Do you think it will be much clear if I abstract the
platform_resource_get, devm_ioremap functions from
mtk_pcie_subsys_powerup and put it to a new functions like
mtk_pcie_subsys_resource_get, and then we may call the
mtk_pcie_subsys_powerup in the resume function?

thanks
> > +
> > +	clk_prepare_enable(pcie->free_ck);
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +		mtk_pcie_enable_port(port);
> > +
> > +	/* In case of EP was removed while system suspend. */
> > +	if (list_empty(&pcie->ports))
> > +		mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +				      mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >  	.ops = &mtk_pcie_ops,
> >  	.startup = mtk_pcie_startup_port,
> >  };
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >  	.need_fix_class_id = true,
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >  		.name = "mtk-pcie",
> >  		.of_match_table = mtk_pcie_ids,
> >  		.suppress_bind_attrs = true,
> > +		.pm = &mtk_pcie_pm_ops,
> >  	},
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > -- 
> > 2.6.4
> > 

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Tue, 2018-07-17 at 18:15 +0100, Lorenzo Pieralisi wrote:
> [+Rafael, Kevin, Ulf]
> 
> On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->pipe_ck);
> > +		clk_disable_unprepare(port->obff_ck);
> > +		clk_disable_unprepare(port->axi_ck);
> > +		clk_disable_unprepare(port->aux_ck);
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +		phy_exit(port->phy);
> > +	}
> > +
> > +	mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port, *tmp;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	if (dev->pm_domain) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> > +	}
> 
> Are these runtime PM calls needed/abused here ?
> 
> Mind explaining the logic ?
> 
> There is certainly an asymmetry with the suspend callback which made me
> suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> that we can make progress with this patch.
> 
> Lorenzo
> 
Hi Lorenzo, thanks for your comments.
Sorry I don't get you.
I believe that in suspend callbacks the pm_runtime_put_sync and
pm_runtime_disable should be called to gated the CMOS for this module,
while the pm_rumtime_enable and pm_rumtime_get_sync should be called in
resume callback.

That's exactly this patch doing.
But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
in the mtk_pcie_subsys_powerdown.

I did not call mtk_pcie_subsys_powerup since it does not just wrapped
pm_rumtime related functions but also do the platform_resource_get,
devm_ioremap, and free_ck clock get which I do not needed in resume
callback.

Do you think it will be much clear if I abstract the
platform_resource_get, devm_ioremap functions from
mtk_pcie_subsys_powerup and put it to a new functions like
mtk_pcie_subsys_resource_get, and then we may call the
mtk_pcie_subsys_powerup in the resume function?

thanks
> > +
> > +	clk_prepare_enable(pcie->free_ck);
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +		mtk_pcie_enable_port(port);
> > +
> > +	/* In case of EP was removed while system suspend. */
> > +	if (list_empty(&pcie->ports))
> > +		mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +				      mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >  	.ops = &mtk_pcie_ops,
> >  	.startup = mtk_pcie_startup_port,
> >  };
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >  	.need_fix_class_id = true,
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >  		.name = "mtk-pcie",
> >  		.of_match_table = mtk_pcie_ids,
> >  		.suppress_bind_attrs = true,
> > +		.pm = &mtk_pcie_pm_ops,
> >  	},
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > -- 
> > 2.6.4
> > 

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Tue, 2018-07-17 at 18:15 +0100, Lorenzo Pieralisi wrote:
> [+Rafael, Kevin, Ulf]
> 
> On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->pipe_ck);
> > +		clk_disable_unprepare(port->obff_ck);
> > +		clk_disable_unprepare(port->axi_ck);
> > +		clk_disable_unprepare(port->aux_ck);
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +		phy_exit(port->phy);
> > +	}
> > +
> > +	mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port, *tmp;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	if (dev->pm_domain) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> > +	}
> 
> Are these runtime PM calls needed/abused here ?
> 
> Mind explaining the logic ?
> 
> There is certainly an asymmetry with the suspend callback which made me
> suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> that we can make progress with this patch.
> 
> Lorenzo
> 
Hi Lorenzo, thanks for your comments.
Sorry I don't get you.
I believe that in suspend callbacks the pm_runtime_put_sync and
pm_runtime_disable should be called to gated the CMOS for this module,
while the pm_rumtime_enable and pm_rumtime_get_sync should be called in
resume callback.

That's exactly this patch doing.
But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
in the mtk_pcie_subsys_powerdown.

I did not call mtk_pcie_subsys_powerup since it does not just wrapped
pm_rumtime related functions but also do the platform_resource_get,
devm_ioremap, and free_ck clock get which I do not needed in resume
callback.

Do you think it will be much clear if I abstract the
platform_resource_get, devm_ioremap functions from
mtk_pcie_subsys_powerup and put it to a new functions like
mtk_pcie_subsys_resource_get, and then we may call the
mtk_pcie_subsys_powerup in the resume function?

thanks
> > +
> > +	clk_prepare_enable(pcie->free_ck);
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +		mtk_pcie_enable_port(port);
> > +
> > +	/* In case of EP was removed while system suspend. */
> > +	if (list_empty(&pcie->ports))
> > +		mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +				      mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >  	.ops = &mtk_pcie_ops,
> >  	.startup = mtk_pcie_startup_port,
> >  };
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >  	.need_fix_class_id = true,
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >  		.name = "mtk-pcie",
> >  		.of_match_table = mtk_pcie_ids,
> >  		.suppress_bind_attrs = true,
> > +		.pm = &mtk_pcie_pm_ops,
> >  	},
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > -- 
> > 2.6.4
> > 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Tue, 2018-07-17 at 18:15 +0100, Lorenzo Pieralisi wrote:
> [+Rafael, Kevin, Ulf]
> 
> On Mon, Jul 02, 2018 at 03:57:43PM +0800, honghui.zhang at mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >  	bool need_fix_class_id;
> > +	bool pm_support;
> >  	struct pci_ops *ops;
> >  	int (*startup)(struct mtk_pcie_port *port);
> >  	int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >  	return err;
> >  }
> >  
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		clk_disable_unprepare(port->pipe_ck);
> > +		clk_disable_unprepare(port->obff_ck);
> > +		clk_disable_unprepare(port->axi_ck);
> > +		clk_disable_unprepare(port->aux_ck);
> > +		clk_disable_unprepare(port->ahb_ck);
> > +		clk_disable_unprepare(port->sys_ck);
> > +		phy_power_off(port->phy);
> > +		phy_exit(port->phy);
> > +	}
> > +
> > +	mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > +	struct mtk_pcie_port *port, *tmp;
> > +
> > +	if (!soc->pm_support)
> > +		return 0;
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return 0;
> > +
> > +	if (dev->pm_domain) {
> > +		pm_runtime_enable(dev);
> > +		pm_runtime_get_sync(dev);
> > +	}
> 
> Are these runtime PM calls needed/abused here ?
> 
> Mind explaining the logic ?
> 
> There is certainly an asymmetry with the suspend callback which made me
> suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> that we can make progress with this patch.
> 
> Lorenzo
> 
Hi Lorenzo, thanks for your comments.
Sorry I don't get you.
I believe that in suspend callbacks the pm_runtime_put_sync and
pm_runtime_disable should be called to gated the CMOS for this module,
while the pm_rumtime_enable and pm_rumtime_get_sync should be called in
resume callback.

That's exactly this patch doing.
But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
in the mtk_pcie_subsys_powerdown.

I did not call mtk_pcie_subsys_powerup since it does not just wrapped
pm_rumtime related functions but also do the platform_resource_get,
devm_ioremap, and free_ck clock get which I do not needed in resume
callback.

Do you think it will be much clear if I abstract the
platform_resource_get, devm_ioremap functions from
mtk_pcie_subsys_powerup and put it to a new functions like
mtk_pcie_subsys_resource_get, and then we may call the
mtk_pcie_subsys_powerup in the resume function?

thanks
> > +
> > +	clk_prepare_enable(pcie->free_ck);
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +		mtk_pcie_enable_port(port);
> > +
> > +	/* In case of EP was removed while system suspend. */
> > +	if (list_empty(&pcie->ports))
> > +		mtk_pcie_subsys_powerdown(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +				      mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >  	.ops = &mtk_pcie_ops,
> >  	.startup = mtk_pcie_startup_port,
> >  };
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >  
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >  	.need_fix_class_id = true,
> > +	.pm_support = true,
> >  	.ops = &mtk_pcie_ops_v2,
> >  	.startup = mtk_pcie_startup_port_v2,
> >  	.setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >  		.name = "mtk-pcie",
> >  		.of_match_table = mtk_pcie_ids,
> >  		.suppress_bind_attrs = true,
> > +		.pm = &mtk_pcie_pm_ops,
> >  	},
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > -- 
> > 2.6.4
> > 

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:

<snip>

> > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > +	struct mtk_pcie_port *port, *tmp;
> > > +
> > > +	if (!soc->pm_support)
> > > +		return 0;
> > > +
> > > +	if (list_empty(&pcie->ports))
> > > +		return 0;
> > > +
> > > +	if (dev->pm_domain) {
> > > +		pm_runtime_enable(dev);
> > > +		pm_runtime_get_sync(dev);
> > > +	}
> > 
> > Are these runtime PM calls needed/abused here ?
> > 
> > Mind explaining the logic ?
> > 
> > There is certainly an asymmetry with the suspend callback which made me
> > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > that we can make progress with this patch.
> > 
> > Lorenzo
> > 
> Hi Lorenzo, thanks for your comments.
> Sorry I don't get you.
> I believe that in suspend callbacks the pm_runtime_put_sync and
> pm_runtime_disable should be called to gated the CMOS for this module,
> while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> in resume callback.

That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
thoroughly, I am not sure it is needed and that's the right way
of doing it in system suspend callbacks.

> That's exactly this patch doing.
> But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> in the mtk_pcie_subsys_powerdown.

Ah, sorry, I missed that.

> I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> pm_rumtime related functions but also do the platform_resource_get,
> devm_ioremap, and free_ck clock get which I do not needed in resume
> callback.
> 
> Do you think it will be much clear if I abstract the
> platform_resource_get, devm_ioremap functions from
> mtk_pcie_subsys_powerup and put it to a new functions like
> mtk_pcie_subsys_resource_get, and then we may call the
> mtk_pcie_subsys_powerup in the resume function?

I think so but let's wait first for feedback on whether those
runtime PM calls are needed in the first place.

Lorenzo

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:

<snip>

> > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > +	struct mtk_pcie_port *port, *tmp;
> > > +
> > > +	if (!soc->pm_support)
> > > +		return 0;
> > > +
> > > +	if (list_empty(&pcie->ports))
> > > +		return 0;
> > > +
> > > +	if (dev->pm_domain) {
> > > +		pm_runtime_enable(dev);
> > > +		pm_runtime_get_sync(dev);
> > > +	}
> > 
> > Are these runtime PM calls needed/abused here ?
> > 
> > Mind explaining the logic ?
> > 
> > There is certainly an asymmetry with the suspend callback which made me
> > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > that we can make progress with this patch.
> > 
> > Lorenzo
> > 
> Hi Lorenzo, thanks for your comments.
> Sorry I don't get you.
> I believe that in suspend callbacks the pm_runtime_put_sync and
> pm_runtime_disable should be called to gated the CMOS for this module,
> while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> in resume callback.

That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
thoroughly, I am not sure it is needed and that's the right way
of doing it in system suspend callbacks.

> That's exactly this patch doing.
> But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> in the mtk_pcie_subsys_powerdown.

Ah, sorry, I missed that.

> I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> pm_rumtime related functions but also do the platform_resource_get,
> devm_ioremap, and free_ck clock get which I do not needed in resume
> callback.
> 
> Do you think it will be much clear if I abstract the
> platform_resource_get, devm_ioremap functions from
> mtk_pcie_subsys_powerup and put it to a new functions like
> mtk_pcie_subsys_resource_get, and then we may call the
> mtk_pcie_subsys_powerup in the resume function?

I think so but let's wait first for feedback on whether those
runtime PM calls are needed in the first place.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:

<snip>

> > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > +	struct mtk_pcie_port *port, *tmp;
> > > +
> > > +	if (!soc->pm_support)
> > > +		return 0;
> > > +
> > > +	if (list_empty(&pcie->ports))
> > > +		return 0;
> > > +
> > > +	if (dev->pm_domain) {
> > > +		pm_runtime_enable(dev);
> > > +		pm_runtime_get_sync(dev);
> > > +	}
> > 
> > Are these runtime PM calls needed/abused here ?
> > 
> > Mind explaining the logic ?
> > 
> > There is certainly an asymmetry with the suspend callback which made me
> > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > that we can make progress with this patch.
> > 
> > Lorenzo
> > 
> Hi Lorenzo, thanks for your comments.
> Sorry I don't get you.
> I believe that in suspend callbacks the pm_runtime_put_sync and
> pm_runtime_disable should be called to gated the CMOS for this module,
> while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> in resume callback.

That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
thoroughly, I am not sure it is needed and that's the right way
of doing it in system suspend callbacks.

> That's exactly this patch doing.
> But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> in the mtk_pcie_subsys_powerdown.

Ah, sorry, I missed that.

> I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> pm_rumtime related functions but also do the platform_resource_get,
> devm_ioremap, and free_ck clock get which I do not needed in resume
> callback.
> 
> Do you think it will be much clear if I abstract the
> platform_resource_get, devm_ioremap functions from
> mtk_pcie_subsys_powerup and put it to a new functions like
> mtk_pcie_subsys_resource_get, and then we may call the
> mtk_pcie_subsys_powerup in the resume function?

I think so but let's wait first for feedback on whether those
runtime PM calls are needed in the first place.

Lorenzo

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Wed, 2018-07-18 at 10:49 +0100, Lorenzo Pieralisi wrote:
> On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:
> 
> <snip>
> 
> > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > > +	struct mtk_pcie_port *port, *tmp;
> > > > +
> > > > +	if (!soc->pm_support)
> > > > +		return 0;
> > > > +
> > > > +	if (list_empty(&pcie->ports))
> > > > +		return 0;
> > > > +
> > > > +	if (dev->pm_domain) {
> > > > +		pm_runtime_enable(dev);
> > > > +		pm_runtime_get_sync(dev);
> > > > +	}
> > > 
> > > Are these runtime PM calls needed/abused here ?
> > > 
> > > Mind explaining the logic ?
> > > 
> > > There is certainly an asymmetry with the suspend callback which made me
> > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > > that we can make progress with this patch.
> > > 
> > > Lorenzo
> > > 
> > Hi Lorenzo, thanks for your comments.
> > Sorry I don't get you.
> > I believe that in suspend callbacks the pm_runtime_put_sync and
> > pm_runtime_disable should be called to gated the CMOS for this module,
> > while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> > in resume callback.

> That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
> thoroughly, I am not sure it is needed and that's the right way
> of doing it in system suspend callbacks.
> 

hi, Rafael, Kevin and Ulf,

after reading of the power related documents in Documents/power/, I'm
still confused whether the runtime_pm callbacks should be called in
system suspend callbacks.

I believe that system suspend does not care about the device's CMOS
status. And the device's CMOS status is controlled by runtime pm.
That why I gated the CMOS through runtime pm in the system suspend
callbacks.

But I checked all existing system suspend callbacks and found there's no
runtime pm was executed in system suspend callbacks. Does that means
when system suspend, the system suspend framework does not care about
the power consume? Or does it gated each device's CMOS in somewhere
else?

Or should I just remove the runtime pm callbacks in the system suspend
flow?

Could someone kindly give me some comments?

Thanks in advance.

> > That's exactly this patch doing.
> > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> > in the mtk_pcie_subsys_powerdown.
> 
> Ah, sorry, I missed that.
> 
> > I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> > pm_rumtime related functions but also do the platform_resource_get,
> > devm_ioremap, and free_ck clock get which I do not needed in resume
> > callback.
> > 
> > Do you think it will be much clear if I abstract the
> > platform_resource_get, devm_ioremap functions from
> > mtk_pcie_subsys_powerup and put it to a new functions like
> > mtk_pcie_subsys_resource_get, and then we may call the
> > mtk_pcie_subsys_powerup in the resume function?
> 
> I think so but let's wait first for feedback on whether those
> runtime PM calls are needed in the first place.
> 
> Lorenzo

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Wed, 2018-07-18 at 10:49 +0100, Lorenzo Pieralisi wrote:
> On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:
> 
> <snip>
> 
> > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > > +	struct mtk_pcie_port *port, *tmp;
> > > > +
> > > > +	if (!soc->pm_support)
> > > > +		return 0;
> > > > +
> > > > +	if (list_empty(&pcie->ports))
> > > > +		return 0;
> > > > +
> > > > +	if (dev->pm_domain) {
> > > > +		pm_runtime_enable(dev);
> > > > +		pm_runtime_get_sync(dev);
> > > > +	}
> > > 
> > > Are these runtime PM calls needed/abused here ?
> > > 
> > > Mind explaining the logic ?
> > > 
> > > There is certainly an asymmetry with the suspend callback which made me
> > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > > that we can make progress with this patch.
> > > 
> > > Lorenzo
> > > 
> > Hi Lorenzo, thanks for your comments.
> > Sorry I don't get you.
> > I believe that in suspend callbacks the pm_runtime_put_sync and
> > pm_runtime_disable should be called to gated the CMOS for this module,
> > while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> > in resume callback.

> That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
> thoroughly, I am not sure it is needed and that's the right way
> of doing it in system suspend callbacks.
> 

hi, Rafael, Kevin and Ulf,

after reading of the power related documents in Documents/power/, I'm
still confused whether the runtime_pm callbacks should be called in
system suspend callbacks.

I believe that system suspend does not care about the device's CMOS
status. And the device's CMOS status is controlled by runtime pm.
That why I gated the CMOS through runtime pm in the system suspend
callbacks.

But I checked all existing system suspend callbacks and found there's no
runtime pm was executed in system suspend callbacks. Does that means
when system suspend, the system suspend framework does not care about
the power consume? Or does it gated each device's CMOS in somewhere
else?

Or should I just remove the runtime pm callbacks in the system suspend
flow?

Could someone kindly give me some comments?

Thanks in advance.

> > That's exactly this patch doing.
> > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> > in the mtk_pcie_subsys_powerdown.
> 
> Ah, sorry, I missed that.
> 
> > I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> > pm_rumtime related functions but also do the platform_resource_get,
> > devm_ioremap, and free_ck clock get which I do not needed in resume
> > callback.
> > 
> > Do you think it will be much clear if I abstract the
> > platform_resource_get, devm_ioremap functions from
> > mtk_pcie_subsys_powerup and put it to a new functions like
> > mtk_pcie_subsys_resource_get, and then we may call the
> > mtk_pcie_subsys_powerup in the resume function?
> 
> I think so but let's wait first for feedback on whether those
> runtime PM calls are needed in the first place.
> 
> Lorenzo

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Wed, 2018-07-18 at 10:49 +0100, Lorenzo Pieralisi wrote:
> On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:
> 
> <snip>
> 
> > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > > +	struct mtk_pcie_port *port, *tmp;
> > > > +
> > > > +	if (!soc->pm_support)
> > > > +		return 0;
> > > > +
> > > > +	if (list_empty(&pcie->ports))
> > > > +		return 0;
> > > > +
> > > > +	if (dev->pm_domain) {
> > > > +		pm_runtime_enable(dev);
> > > > +		pm_runtime_get_sync(dev);
> > > > +	}
> > > 
> > > Are these runtime PM calls needed/abused here ?
> > > 
> > > Mind explaining the logic ?
> > > 
> > > There is certainly an asymmetry with the suspend callback which made me
> > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > > that we can make progress with this patch.
> > > 
> > > Lorenzo
> > > 
> > Hi Lorenzo, thanks for your comments.
> > Sorry I don't get you.
> > I believe that in suspend callbacks the pm_runtime_put_sync and
> > pm_runtime_disable should be called to gated the CMOS for this module,
> > while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> > in resume callback.

> That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
> thoroughly, I am not sure it is needed and that's the right way
> of doing it in system suspend callbacks.
> 

hi, Rafael, Kevin and Ulf,

after reading of the power related documents in Documents/power/, I'm
still confused whether the runtime_pm callbacks should be called in
system suspend callbacks.

I believe that system suspend does not care about the device's CMOS
status. And the device's CMOS status is controlled by runtime pm.
That why I gated the CMOS through runtime pm in the system suspend
callbacks.

But I checked all existing system suspend callbacks and found there's no
runtime pm was executed in system suspend callbacks. Does that means
when system suspend, the system suspend framework does not care about
the power consume? Or does it gated each device's CMOS in somewhere
else?

Or should I just remove the runtime pm callbacks in the system suspend
flow?

Could someone kindly give me some comments?

Thanks in advance.

> > That's exactly this patch doing.
> > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> > in the mtk_pcie_subsys_powerdown.
> 
> Ah, sorry, I missed that.
> 
> > I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> > pm_rumtime related functions but also do the platform_resource_get,
> > devm_ioremap, and free_ck clock get which I do not needed in resume
> > callback.
> > 
> > Do you think it will be much clear if I abstract the
> > platform_resource_get, devm_ioremap functions from
> > mtk_pcie_subsys_powerup and put it to a new functions like
> > mtk_pcie_subsys_resource_get, and then we may call the
> > mtk_pcie_subsys_powerup in the resume function?
> 
> I think so but let's wait first for feedback on whether those
> runtime PM calls are needed in the first place.
> 
> Lorenzo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Wed, 2018-07-18 at 10:49 +0100, Lorenzo Pieralisi wrote:
> On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:
> 
> <snip>
> 
> > > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > > +	const struct mtk_pcie_soc *soc = pcie->soc;
> > > > +	struct mtk_pcie_port *port, *tmp;
> > > > +
> > > > +	if (!soc->pm_support)
> > > > +		return 0;
> > > > +
> > > > +	if (list_empty(&pcie->ports))
> > > > +		return 0;
> > > > +
> > > > +	if (dev->pm_domain) {
> > > > +		pm_runtime_enable(dev);
> > > > +		pm_runtime_get_sync(dev);
> > > > +	}
> > > 
> > > Are these runtime PM calls needed/abused here ?
> > > 
> > > Mind explaining the logic ?
> > > 
> > > There is certainly an asymmetry with the suspend callback which made me
> > > suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
> > > that we can make progress with this patch.
> > > 
> > > Lorenzo
> > > 
> > Hi Lorenzo, thanks for your comments.
> > Sorry I don't get you.
> > I believe that in suspend callbacks the pm_runtime_put_sync and
> > pm_runtime_disable should be called to gated the CMOS for this module,
> > while the pm_rumtime_enable and pm_rumtime_get_sync should be called
> > in resume callback.

> That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
> thoroughly, I am not sure it is needed and that's the right way
> of doing it in system suspend callbacks.
> 

hi, Rafael, Kevin and Ulf,

after reading of the power related documents in Documents/power/, I'm
still confused whether the runtime_pm callbacks should be called in
system suspend callbacks.

I believe that system suspend does not care about the device's CMOS
status. And the device's CMOS status is controlled by runtime pm.
That why I gated the CMOS through runtime pm in the system suspend
callbacks.

But I checked all existing system suspend callbacks and found there's no
runtime pm was executed in system suspend callbacks. Does that means
when system suspend, the system suspend framework does not care about
the power consume? Or does it gated each device's CMOS in somewhere
else?

Or should I just remove the runtime pm callbacks in the system suspend
flow?

Could someone kindly give me some comments?

Thanks in advance.

> > That's exactly this patch doing.
> > But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
> > in the mtk_pcie_subsys_powerdown.
> 
> Ah, sorry, I missed that.
> 
> > I did not call mtk_pcie_subsys_powerup since it does not just wrapped
> > pm_rumtime related functions but also do the platform_resource_get,
> > devm_ioremap, and free_ck clock get which I do not needed in resume
> > callback.
> > 
> > Do you think it will be much clear if I abstract the
> > platform_resource_get, devm_ioremap functions from
> > mtk_pcie_subsys_powerup and put it to a new functions like
> > mtk_pcie_subsys_resource_get, and then we may call the
> > mtk_pcie_subsys_powerup in the resume function?
> 
> I think so but let's wait first for feedback on whether those
> runtime PM calls are needed in the first place.
> 
> Lorenzo

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Ulf Hansson]

-trimmed cc-list

On 2 July 2018 at 09:57,  <honghui.zhang@mediatek.com> wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
>
> The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> suspend, and all the internal control register will be reset after system
> resume. The PCIe link should be re-established and the related control
> register values should be re-set after system resume.

Sounds very familiar as what is implemented in
drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
get inspired to use the similar pattern.

A few comments below. However, please note, I am not an expert in PCIe
so I may overlook some obvious things. I am relying on Lorenzo or some
other PCIe expert, to fill in.

>
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 86918d4..175d7b6 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>         bool need_fix_class_id;
> +       bool pm_support;
>         struct pci_ops *ops;
>         int (*startup)(struct mtk_pcie_port *port);
>         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>         return err;
>  }
>
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

I am not sure I understand why you need to suspend the device in the
"noirq" phase. Isn't it fine to do that in the regular suspend phase?

> +{
> +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +       const struct mtk_pcie_soc *soc = pcie->soc;
> +       struct mtk_pcie_port *port;
> +
> +       if (!soc->pm_support)
> +               return 0;
> +
> +       if (list_empty(&pcie->ports))
> +               return 0;
> +
> +       list_for_each_entry(port, &pcie->ports, list) {
> +               clk_disable_unprepare(port->pipe_ck);
> +               clk_disable_unprepare(port->obff_ck);
> +               clk_disable_unprepare(port->axi_ck);
> +               clk_disable_unprepare(port->aux_ck);
> +               clk_disable_unprepare(port->ahb_ck);
> +               clk_disable_unprepare(port->sys_ck);
> +               phy_power_off(port->phy);
> +               phy_exit(port->phy);
> +       }
> +
> +       mtk_pcie_subsys_powerdown(pcie);

Why not gate clocks, unconditionally not depending on if pm_support is
set or not, during system suspend?

I understand that you for some SoCs needs also to restore registers
during system resume, but that's a different thing, isn't it?

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> +{
> +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +       const struct mtk_pcie_soc *soc = pcie->soc;
> +       struct mtk_pcie_port *port, *tmp;
> +
> +       if (!soc->pm_support)
> +               return 0;
> +
> +       if (list_empty(&pcie->ports))
> +               return 0;
> +
> +       if (dev->pm_domain) {
> +               pm_runtime_enable(dev);
> +               pm_runtime_get_sync(dev);

I noticed, Lorenzo wanted me to comment on this, so here it goes.

I guess the reason to why you make these pm_runtime_*() calls, is
because you want to restore the actions taken during
mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
pm_runtime_disable|put*())?

If that's the case, I would rather avoid calling pm_runtime_disable()
and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
isn't needed.

Of course, another option is to follow the pattern in
drivers/mmc/host/sdhci-xenon.c.

Overall, I am also wondering why only runtime PM is used when there is
a PM domain attached to the device? That seems to make the logic in
the driver, unnecessary complicated. Why not use runtime
unconditionally and thus enable it during ->probe()?

> +       }
> +
> +       clk_prepare_enable(pcie->free_ck);
> +
> +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +               mtk_pcie_enable_port(port);
> +
> +       /* In case of EP was removed while system suspend. */
> +       if (list_empty(&pcie->ports))
> +               mtk_pcie_subsys_powerdown(pcie);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> +                                     mtk_pcie_resume_noirq)
> +};
> +
>  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
>         .ops = &mtk_pcie_ops,
>         .startup = mtk_pcie_startup_port,
>  };
>
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> +       .pm_support = true,
>         .ops = &mtk_pcie_ops_v2,
>         .startup = mtk_pcie_startup_port_v2,
>         .setup_irq = mtk_pcie_setup_irq,
> @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
>         .need_fix_class_id = true,
> +       .pm_support = true,
>         .ops = &mtk_pcie_ops_v2,
>         .startup = mtk_pcie_startup_port_v2,
>         .setup_irq = mtk_pcie_setup_irq,
> @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
>                 .name = "mtk-pcie",
>                 .of_match_table = mtk_pcie_ids,
>                 .suppress_bind_attrs = true,
> +               .pm = &mtk_pcie_pm_ops,
>         },
>  };
>  builtin_platform_driver(mtk_pcie_driver);
> --
> 2.6.4
>

Kind regards
Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Ulf Hansson]

-trimmed cc-list

On 2 July 2018 at 09:57,  <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> From: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>
> The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> suspend, and all the internal control register will be reset after system
> resume. The PCIe link should be re-established and the related control
> register values should be re-set after system resume.

Sounds very familiar as what is implemented in
drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
get inspired to use the similar pattern.

A few comments below. However, please note, I am not an expert in PCIe
so I may overlook some obvious things. I am relying on Lorenzo or some
other PCIe expert, to fill in.

>
> Signed-off-by: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Acked-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 86918d4..175d7b6 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>  /**
>   * struct mtk_pcie_soc - differentiate between host generations
>   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> + * @pm_support: whether the host's MTCMOS will be off when suspend
>   * @ops: pointer to configuration access functions
>   * @startup: pointer to controller setting functions
>   * @setup_irq: pointer to initialize IRQ functions
>   */
>  struct mtk_pcie_soc {
>         bool need_fix_class_id;
> +       bool pm_support;
>         struct pci_ops *ops;
>         int (*startup)(struct mtk_pcie_port *port);
>         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>         return err;
>  }
>
> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

I am not sure I understand why you need to suspend the device in the
"noirq" phase. Isn't it fine to do that in the regular suspend phase?

> +{
> +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +       const struct mtk_pcie_soc *soc = pcie->soc;
> +       struct mtk_pcie_port *port;
> +
> +       if (!soc->pm_support)
> +               return 0;
> +
> +       if (list_empty(&pcie->ports))
> +               return 0;
> +
> +       list_for_each_entry(port, &pcie->ports, list) {
> +               clk_disable_unprepare(port->pipe_ck);
> +               clk_disable_unprepare(port->obff_ck);
> +               clk_disable_unprepare(port->axi_ck);
> +               clk_disable_unprepare(port->aux_ck);
> +               clk_disable_unprepare(port->ahb_ck);
> +               clk_disable_unprepare(port->sys_ck);
> +               phy_power_off(port->phy);
> +               phy_exit(port->phy);
> +       }
> +
> +       mtk_pcie_subsys_powerdown(pcie);

Why not gate clocks, unconditionally not depending on if pm_support is
set or not, during system suspend?

I understand that you for some SoCs needs also to restore registers
during system resume, but that's a different thing, isn't it?

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> +{
> +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> +       const struct mtk_pcie_soc *soc = pcie->soc;
> +       struct mtk_pcie_port *port, *tmp;
> +
> +       if (!soc->pm_support)
> +               return 0;
> +
> +       if (list_empty(&pcie->ports))
> +               return 0;
> +
> +       if (dev->pm_domain) {
> +               pm_runtime_enable(dev);
> +               pm_runtime_get_sync(dev);

I noticed, Lorenzo wanted me to comment on this, so here it goes.

I guess the reason to why you make these pm_runtime_*() calls, is
because you want to restore the actions taken during
mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
pm_runtime_disable|put*())?

If that's the case, I would rather avoid calling pm_runtime_disable()
and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
isn't needed.

Of course, another option is to follow the pattern in
drivers/mmc/host/sdhci-xenon.c.

Overall, I am also wondering why only runtime PM is used when there is
a PM domain attached to the device? That seems to make the logic in
the driver, unnecessary complicated. Why not use runtime
unconditionally and thus enable it during ->probe()?

> +       }
> +
> +       clk_prepare_enable(pcie->free_ck);
> +
> +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +               mtk_pcie_enable_port(port);
> +
> +       /* In case of EP was removed while system suspend. */
> +       if (list_empty(&pcie->ports))
> +               mtk_pcie_subsys_powerdown(pcie);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> +                                     mtk_pcie_resume_noirq)
> +};
> +
>  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
>         .ops = &mtk_pcie_ops,
>         .startup = mtk_pcie_startup_port,
>  };
>
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> +       .pm_support = true,
>         .ops = &mtk_pcie_ops_v2,
>         .startup = mtk_pcie_startup_port_v2,
>         .setup_irq = mtk_pcie_setup_irq,
> @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>
>  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
>         .need_fix_class_id = true,
> +       .pm_support = true,
>         .ops = &mtk_pcie_ops_v2,
>         .startup = mtk_pcie_startup_port_v2,
>         .setup_irq = mtk_pcie_setup_irq,
> @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
>                 .name = "mtk-pcie",
>                 .of_match_table = mtk_pcie_ids,
>                 .suppress_bind_attrs = true,
> +               .pm = &mtk_pcie_pm_ops,
>         },
>  };
>  builtin_platform_driver(mtk_pcie_driver);
> --
> 2.6.4
>

Kind regards
Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Fri, 2018-09-07 at 14:33 +0200, Ulf Hansson wrote:
> -trimmed cc-list
> 
> On 2 July 2018 at 09:57,  <honghui.zhang@mediatek.com> wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> >
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> 
> Sounds very familiar as what is implemented in
> drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
> get inspired to use the similar pattern.
> 
> A few comments below. However, please note, I am not an expert in PCIe
> so I may overlook some obvious things. I am relying on Lorenzo or some
> other PCIe expert, to fill in.
> 
Hi, Ulf, thanks very much for your comments, replied below.

> >
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >         bool need_fix_class_id;
> > +       bool pm_support;
> >         struct pci_ops *ops;
> >         int (*startup)(struct mtk_pcie_port *port);
> >         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >         return err;
> >  }
> >
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> 
> I am not sure I understand why you need to suspend the device in the
> "noirq" phase. Isn't it fine to do that in the regular suspend phase?

PCIe device's configuration space values should be saved/restored in
system suspend/resume flow, and PCIe framework will take care of that in
the suspend_noirq phase. Suspend device in the "noirq" phase will spare
device driver from that.

> 
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               clk_disable_unprepare(port->pipe_ck);
> > +               clk_disable_unprepare(port->obff_ck);
> > +               clk_disable_unprepare(port->axi_ck);
> > +               clk_disable_unprepare(port->aux_ck);
> > +               clk_disable_unprepare(port->ahb_ck);
> > +               clk_disable_unprepare(port->sys_ck);
> > +               phy_power_off(port->phy);
> > +               phy_exit(port->phy);
> > +       }
> > +
> > +       mtk_pcie_subsys_powerdown(pcie);
> 
> Why not gate clocks, unconditionally not depending on if pm_support is
> set or not, during system suspend?
> 
> I understand that you for some SoCs needs also to restore registers
> during system resume, but that's a different thing, isn't it?
> 

Well, current host driver support different SoCs like mt7623, mt7622 and
mt2712. mt7623 need quite special take care of when system suspend. This
patch just add the suspend/resume support for mt7622&mt2712, while
mt7623 was excluded. This "pm_support" maybe removed after we have a
solution for mt7623 SoC of the system suspend.

> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port, *tmp;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       if (dev->pm_domain) {
> > +               pm_runtime_enable(dev);
> > +               pm_runtime_get_sync(dev);
> 
> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> 
> I guess the reason to why you make these pm_runtime_*() calls, is
> because you want to restore the actions taken during
> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> pm_runtime_disable|put*())?

Yes, that's what am I want to do.

> 
> If that's the case, I would rather avoid calling pm_runtime_disable()
> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> isn't needed.

> Of course, another option is to follow the pattern in
> drivers/mmc/host/sdhci-xenon.c.
> 

Thanks.
I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
system suspend callbacks and set the device status to "PRM_SUSPENDED"
through the pm_runtime_force_suspend() interface.
I noticed that pm_runtime_put_sync will put the device status to
"PRM_SUSPENDED" status in some certain condition.
And I did not found the pci framework setting the status to
"PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
suspend_noirq phase.

According to my understanding of PM, the system suspend flow does not
care about the runtime PM status. The runtime pm is responsible for the
CMOS gate, if the device want to gated it's CMOS, it need to call the
runtime pm interface obviously.

But I found that the PCIe framework will still operate with the device
after device driver suspend_noirq executed like save the configuration
space values through pci_save_state, So I guess the cmos could not be
gated at the suspend_noirq phase. I will update the patchset to remove
the pm runtime operations in the suspend_noirq phase.

> Overall, I am also wondering why only runtime PM is used when there is
> a PM domain attached to the device? That seems to make the logic in
> the driver, unnecessary complicated. Why not use runtime
> unconditionally and thus enable it during ->probe()?

I'm not sure, I thought that if there's no "power-domains = " property
in device node, that means the device has no PM domain. Some of the SoCs
does not have independent CMOS domain. So does that means it will leave
the dev->pm_domain as NULL or assign it as the genpd->pm_domain?

I need to do some homework to figure this out and will send a new patch
to fix this if needed.

> 
> > +       }
> > +
> > +       clk_prepare_enable(pcie->free_ck);
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +               mtk_pcie_enable_port(port);
> > +
> > +       /* In case of EP was removed while system suspend. */
> > +       if (list_empty(&pcie->ports))
> > +               mtk_pcie_subsys_powerdown(pcie);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +                                     mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >         .ops = &mtk_pcie_ops,
> >         .startup = mtk_pcie_startup_port,
> >  };
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >         .need_fix_class_id = true,
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >                 .name = "mtk-pcie",
> >                 .of_match_table = mtk_pcie_ids,
> >                 .suppress_bind_attrs = true,
> > +               .pm = &mtk_pcie_pm_ops,
> >         },
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > --
> > 2.6.4
> >
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Fri, 2018-09-07 at 14:33 +0200, Ulf Hansson wrote:
> -trimmed cc-list
> 
> On 2 July 2018 at 09:57,  <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > From: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
> 
> Sounds very familiar as what is implemented in
> drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
> get inspired to use the similar pattern.
> 
> A few comments below. However, please note, I am not an expert in PCIe
> so I may overlook some obvious things. I am relying on Lorenzo or some
> other PCIe expert, to fill in.
> 
Hi, Ulf, thanks very much for your comments, replied below.

> >
> > Signed-off-by: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Acked-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 86918d4..175d7b6 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
> >  /**
> >   * struct mtk_pcie_soc - differentiate between host generations
> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > + * @pm_support: whether the host's MTCMOS will be off when suspend
> >   * @ops: pointer to configuration access functions
> >   * @startup: pointer to controller setting functions
> >   * @setup_irq: pointer to initialize IRQ functions
> >   */
> >  struct mtk_pcie_soc {
> >         bool need_fix_class_id;
> > +       bool pm_support;
> >         struct pci_ops *ops;
> >         int (*startup)(struct mtk_pcie_port *port);
> >         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> >         return err;
> >  }
> >
> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> 
> I am not sure I understand why you need to suspend the device in the
> "noirq" phase. Isn't it fine to do that in the regular suspend phase?

PCIe device's configuration space values should be saved/restored in
system suspend/resume flow, and PCIe framework will take care of that in
the suspend_noirq phase. Suspend device in the "noirq" phase will spare
device driver from that.

> 
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               clk_disable_unprepare(port->pipe_ck);
> > +               clk_disable_unprepare(port->obff_ck);
> > +               clk_disable_unprepare(port->axi_ck);
> > +               clk_disable_unprepare(port->aux_ck);
> > +               clk_disable_unprepare(port->ahb_ck);
> > +               clk_disable_unprepare(port->sys_ck);
> > +               phy_power_off(port->phy);
> > +               phy_exit(port->phy);
> > +       }
> > +
> > +       mtk_pcie_subsys_powerdown(pcie);
> 
> Why not gate clocks, unconditionally not depending on if pm_support is
> set or not, during system suspend?
> 
> I understand that you for some SoCs needs also to restore registers
> during system resume, but that's a different thing, isn't it?
> 

Well, current host driver support different SoCs like mt7623, mt7622 and
mt2712. mt7623 need quite special take care of when system suspend. This
patch just add the suspend/resume support for mt7622&mt2712, while
mt7623 was excluded. This "pm_support" maybe removed after we have a
solution for mt7623 SoC of the system suspend.

> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port, *tmp;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       if (dev->pm_domain) {
> > +               pm_runtime_enable(dev);
> > +               pm_runtime_get_sync(dev);
> 
> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> 
> I guess the reason to why you make these pm_runtime_*() calls, is
> because you want to restore the actions taken during
> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> pm_runtime_disable|put*())?

Yes, that's what am I want to do.

> 
> If that's the case, I would rather avoid calling pm_runtime_disable()
> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> isn't needed.

> Of course, another option is to follow the pattern in
> drivers/mmc/host/sdhci-xenon.c.
> 

Thanks.
I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
system suspend callbacks and set the device status to "PRM_SUSPENDED"
through the pm_runtime_force_suspend() interface.
I noticed that pm_runtime_put_sync will put the device status to
"PRM_SUSPENDED" status in some certain condition.
And I did not found the pci framework setting the status to
"PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
suspend_noirq phase.

According to my understanding of PM, the system suspend flow does not
care about the runtime PM status. The runtime pm is responsible for the
CMOS gate, if the device want to gated it's CMOS, it need to call the
runtime pm interface obviously.

But I found that the PCIe framework will still operate with the device
after device driver suspend_noirq executed like save the configuration
space values through pci_save_state, So I guess the cmos could not be
gated at the suspend_noirq phase. I will update the patchset to remove
the pm runtime operations in the suspend_noirq phase.

> Overall, I am also wondering why only runtime PM is used when there is
> a PM domain attached to the device? That seems to make the logic in
> the driver, unnecessary complicated. Why not use runtime
> unconditionally and thus enable it during ->probe()?

I'm not sure, I thought that if there's no "power-domains = " property
in device node, that means the device has no PM domain. Some of the SoCs
does not have independent CMOS domain. So does that means it will leave
the dev->pm_domain as NULL or assign it as the genpd->pm_domain?

I need to do some homework to figure this out and will send a new patch
to fix this if needed.

> 
> > +       }
> > +
> > +       clk_prepare_enable(pcie->free_ck);
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +               mtk_pcie_enable_port(port);
> > +
> > +       /* In case of EP was removed while system suspend. */
> > +       if (list_empty(&pcie->ports))
> > +               mtk_pcie_subsys_powerdown(pcie);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +                                     mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >         .ops = &mtk_pcie_ops,
> >         .startup = mtk_pcie_startup_port,
> >  };
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >         .need_fix_class_id = true,
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >                 .name = "mtk-pcie",
> >                 .of_match_table = mtk_pcie_ids,
> >                 .suppress_bind_attrs = true,
> > +               .pm = &mtk_pcie_pm_ops,
> >         },
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > --
> > 2.6.4
> >
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Ulf Hansson]

On 10 September 2018 at 11:42, Honghui Zhang <honghui.zhang@mediatek.com> wrote:
> On Fri, 2018-09-07 at 14:33 +0200, Ulf Hansson wrote:
>> -trimmed cc-list
>>
>> On 2 July 2018 at 09:57,  <honghui.zhang@mediatek.com> wrote:
>> > From: Honghui Zhang <honghui.zhang@mediatek.com>
>> >
>> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
>> > suspend, and all the internal control register will be reset after system
>> > resume. The PCIe link should be re-established and the related control
>> > register values should be re-set after system resume.
>>
>> Sounds very familiar as what is implemented in
>> drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
>> get inspired to use the similar pattern.
>>
>> A few comments below. However, please note, I am not an expert in PCIe
>> so I may overlook some obvious things. I am relying on Lorenzo or some
>> other PCIe expert, to fill in.
>>
> Hi, Ulf, thanks very much for your comments, replied below.
>
>> >
>> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
>> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
>> > ---
>> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 67 insertions(+)
>> >
>> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
>> > index 86918d4..175d7b6 100644
>> > --- a/drivers/pci/controller/pcie-mediatek.c
>> > +++ b/drivers/pci/controller/pcie-mediatek.c
>> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>> >  /**
>> >   * struct mtk_pcie_soc - differentiate between host generations
>> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
>> > + * @pm_support: whether the host's MTCMOS will be off when suspend
>> >   * @ops: pointer to configuration access functions
>> >   * @startup: pointer to controller setting functions
>> >   * @setup_irq: pointer to initialize IRQ functions
>> >   */
>> >  struct mtk_pcie_soc {
>> >         bool need_fix_class_id;
>> > +       bool pm_support;
>> >         struct pci_ops *ops;
>> >         int (*startup)(struct mtk_pcie_port *port);
>> >         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
>> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>> >         return err;
>> >  }
>> >
>> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
>>
>> I am not sure I understand why you need to suspend the device in the
>> "noirq" phase. Isn't it fine to do that in the regular suspend phase?
>
> PCIe device's configuration space values should be saved/restored in
> system suspend/resume flow, and PCIe framework will take care of that in
> the suspend_noirq phase. Suspend device in the "noirq" phase will spare
> device driver from that.

I see.

>
>>
>> > +{
>> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
>> > +       const struct mtk_pcie_soc *soc = pcie->soc;
>> > +       struct mtk_pcie_port *port;
>> > +
>> > +       if (!soc->pm_support)
>> > +               return 0;
>> > +
>> > +       if (list_empty(&pcie->ports))
>> > +               return 0;
>> > +
>> > +       list_for_each_entry(port, &pcie->ports, list) {
>> > +               clk_disable_unprepare(port->pipe_ck);
>> > +               clk_disable_unprepare(port->obff_ck);
>> > +               clk_disable_unprepare(port->axi_ck);
>> > +               clk_disable_unprepare(port->aux_ck);
>> > +               clk_disable_unprepare(port->ahb_ck);
>> > +               clk_disable_unprepare(port->sys_ck);
>> > +               phy_power_off(port->phy);
>> > +               phy_exit(port->phy);
>> > +       }
>> > +
>> > +       mtk_pcie_subsys_powerdown(pcie);
>>
>> Why not gate clocks, unconditionally not depending on if pm_support is
>> set or not, during system suspend?
>>
>> I understand that you for some SoCs needs also to restore registers
>> during system resume, but that's a different thing, isn't it?
>>
>
> Well, current host driver support different SoCs like mt7623, mt7622 and
> mt2712. mt7623 need quite special take care of when system suspend. This
> patch just add the suspend/resume support for mt7622&mt2712, while
> mt7623 was excluded. This "pm_support" maybe removed after we have a
> solution for mt7623 SoC of the system suspend.

Okay. So you simply want to take small steps, that works!

>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
>> > +{
>> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
>> > +       const struct mtk_pcie_soc *soc = pcie->soc;
>> > +       struct mtk_pcie_port *port, *tmp;
>> > +
>> > +       if (!soc->pm_support)
>> > +               return 0;
>> > +
>> > +       if (list_empty(&pcie->ports))
>> > +               return 0;
>> > +
>> > +       if (dev->pm_domain) {
>> > +               pm_runtime_enable(dev);
>> > +               pm_runtime_get_sync(dev);
>>
>> I noticed, Lorenzo wanted me to comment on this, so here it goes.
>>
>> I guess the reason to why you make these pm_runtime_*() calls, is
>> because you want to restore the actions taken during
>> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
>> pm_runtime_disable|put*())?
>
> Yes, that's what am I want to do.
>
>>
>> If that's the case, I would rather avoid calling pm_runtime_disable()
>> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
>> isn't needed.
>
>> Of course, another option is to follow the pattern in
>> drivers/mmc/host/sdhci-xenon.c.
>>
>
> Thanks.
> I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
> system suspend callbacks and set the device status to "PRM_SUSPENDED"
> through the pm_runtime_force_suspend() interface.
> I noticed that pm_runtime_put_sync will put the device status to
> "PRM_SUSPENDED" status in some certain condition.
> And I did not found the pci framework setting the status to
> "PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
> suspend_noirq phase.
>
> According to my understanding of PM, the system suspend flow does not
> care about the runtime PM status. The runtime pm is responsible for the
> CMOS gate, if the device want to gated it's CMOS, it need to call the
> runtime pm interface obviously.

Well, during system suspend, the PM core disables runtime PM for all
devices. See __device_suspend_late().

Additionally, the PM core prevents devices from being runtime
suspended. See device_prepare(), as it calls
pm_runtime_get_noresume().

In other words, drivers can not rely on calling pm_runtime_put_sync()
from a ->suspend_noirq() callback to put its device into low power
state.

>
> But I found that the PCIe framework will still operate with the device
> after device driver suspend_noirq executed like save the configuration
> space values through pci_save_state, So I guess the cmos could not be
> gated at the suspend_noirq phase. I will update the patchset to remove
> the pm runtime operations in the suspend_noirq phase.

That's doesn't sound correct to me. Although, I am not a PCI expert.

>
>> Overall, I am also wondering why only runtime PM is used when there is
>> a PM domain attached to the device? That seems to make the logic in
>> the driver, unnecessary complicated. Why not use runtime
>> unconditionally and thus enable it during ->probe()?
>
> I'm not sure, I thought that if there's no "power-domains = " property
> in device node, that means the device has no PM domain. Some of the SoCs
> does not have independent CMOS domain. So does that means it will leave
> the dev->pm_domain as NULL or assign it as the genpd->pm_domain?

My point is, that no matter if there is a PM domain assigned or not,
the PCIe driver can still enable/use runtime PM. It shouldn't hurt.

>
> I need to do some homework to figure this out and will send a new patch
> to fix this if needed.

I did some more re-search and maybe the easiest thing for you to do is
to follow the pattern in the tegra PCIe controller driver.
drivers/pci/controller/pci-tegra.c.

That should definitely work for your case as well.

[...]

Kind regards
Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Ulf Hansson]

On 10 September 2018 at 11:42, Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Fri, 2018-09-07 at 14:33 +0200, Ulf Hansson wrote:
>> -trimmed cc-list
>>
>> On 2 July 2018 at 09:57,  <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>> > From: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> >
>> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
>> > suspend, and all the internal control register will be reset after system
>> > resume. The PCIe link should be re-established and the related control
>> > register values should be re-set after system resume.
>>
>> Sounds very familiar as what is implemented in
>> drivers/mmc/host/sdhci-xenon.c. Please have a look, possibly you can
>> get inspired to use the similar pattern.
>>
>> A few comments below. However, please note, I am not an expert in PCIe
>> so I may overlook some obvious things. I am relying on Lorenzo or some
>> other PCIe expert, to fill in.
>>
> Hi, Ulf, thanks very much for your comments, replied below.
>
>> >
>> > Signed-off-by: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> > Acked-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> > ---
>> >  drivers/pci/controller/pcie-mediatek.c | 67 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 67 insertions(+)
>> >
>> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
>> > index 86918d4..175d7b6 100644
>> > --- a/drivers/pci/controller/pcie-mediatek.c
>> > +++ b/drivers/pci/controller/pcie-mediatek.c
>> > @@ -134,12 +134,14 @@ struct mtk_pcie_port;
>> >  /**
>> >   * struct mtk_pcie_soc - differentiate between host generations
>> >   * @need_fix_class_id: whether this host's class ID needed to be fixed or not
>> > + * @pm_support: whether the host's MTCMOS will be off when suspend
>> >   * @ops: pointer to configuration access functions
>> >   * @startup: pointer to controller setting functions
>> >   * @setup_irq: pointer to initialize IRQ functions
>> >   */
>> >  struct mtk_pcie_soc {
>> >         bool need_fix_class_id;
>> > +       bool pm_support;
>> >         struct pci_ops *ops;
>> >         int (*startup)(struct mtk_pcie_port *port);
>> >         int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
>> > @@ -1197,12 +1199,75 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>> >         return err;
>> >  }
>> >
>> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
>>
>> I am not sure I understand why you need to suspend the device in the
>> "noirq" phase. Isn't it fine to do that in the regular suspend phase?
>
> PCIe device's configuration space values should be saved/restored in
> system suspend/resume flow, and PCIe framework will take care of that in
> the suspend_noirq phase. Suspend device in the "noirq" phase will spare
> device driver from that.

I see.

>
>>
>> > +{
>> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
>> > +       const struct mtk_pcie_soc *soc = pcie->soc;
>> > +       struct mtk_pcie_port *port;
>> > +
>> > +       if (!soc->pm_support)
>> > +               return 0;
>> > +
>> > +       if (list_empty(&pcie->ports))
>> > +               return 0;
>> > +
>> > +       list_for_each_entry(port, &pcie->ports, list) {
>> > +               clk_disable_unprepare(port->pipe_ck);
>> > +               clk_disable_unprepare(port->obff_ck);
>> > +               clk_disable_unprepare(port->axi_ck);
>> > +               clk_disable_unprepare(port->aux_ck);
>> > +               clk_disable_unprepare(port->ahb_ck);
>> > +               clk_disable_unprepare(port->sys_ck);
>> > +               phy_power_off(port->phy);
>> > +               phy_exit(port->phy);
>> > +       }
>> > +
>> > +       mtk_pcie_subsys_powerdown(pcie);
>>
>> Why not gate clocks, unconditionally not depending on if pm_support is
>> set or not, during system suspend?
>>
>> I understand that you for some SoCs needs also to restore registers
>> during system resume, but that's a different thing, isn't it?
>>
>
> Well, current host driver support different SoCs like mt7623, mt7622 and
> mt2712. mt7623 need quite special take care of when system suspend. This
> patch just add the suspend/resume support for mt7622&mt2712, while
> mt7623 was excluded. This "pm_support" maybe removed after we have a
> solution for mt7623 SoC of the system suspend.

Okay. So you simply want to take small steps, that works!

>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
>> > +{
>> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
>> > +       const struct mtk_pcie_soc *soc = pcie->soc;
>> > +       struct mtk_pcie_port *port, *tmp;
>> > +
>> > +       if (!soc->pm_support)
>> > +               return 0;
>> > +
>> > +       if (list_empty(&pcie->ports))
>> > +               return 0;
>> > +
>> > +       if (dev->pm_domain) {
>> > +               pm_runtime_enable(dev);
>> > +               pm_runtime_get_sync(dev);
>>
>> I noticed, Lorenzo wanted me to comment on this, so here it goes.
>>
>> I guess the reason to why you make these pm_runtime_*() calls, is
>> because you want to restore the actions taken during
>> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
>> pm_runtime_disable|put*())?
>
> Yes, that's what am I want to do.
>
>>
>> If that's the case, I would rather avoid calling pm_runtime_disable()
>> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
>> isn't needed.
>
>> Of course, another option is to follow the pattern in
>> drivers/mmc/host/sdhci-xenon.c.
>>
>
> Thanks.
> I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
> system suspend callbacks and set the device status to "PRM_SUSPENDED"
> through the pm_runtime_force_suspend() interface.
> I noticed that pm_runtime_put_sync will put the device status to
> "PRM_SUSPENDED" status in some certain condition.
> And I did not found the pci framework setting the status to
> "PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
> suspend_noirq phase.
>
> According to my understanding of PM, the system suspend flow does not
> care about the runtime PM status. The runtime pm is responsible for the
> CMOS gate, if the device want to gated it's CMOS, it need to call the
> runtime pm interface obviously.

Well, during system suspend, the PM core disables runtime PM for all
devices. See __device_suspend_late().

Additionally, the PM core prevents devices from being runtime
suspended. See device_prepare(), as it calls
pm_runtime_get_noresume().

In other words, drivers can not rely on calling pm_runtime_put_sync()
from a ->suspend_noirq() callback to put its device into low power
state.

>
> But I found that the PCIe framework will still operate with the device
> after device driver suspend_noirq executed like save the configuration
> space values through pci_save_state, So I guess the cmos could not be
> gated at the suspend_noirq phase. I will update the patchset to remove
> the pm runtime operations in the suspend_noirq phase.

That's doesn't sound correct to me. Although, I am not a PCI expert.

>
>> Overall, I am also wondering why only runtime PM is used when there is
>> a PM domain attached to the device? That seems to make the logic in
>> the driver, unnecessary complicated. Why not use runtime
>> unconditionally and thus enable it during ->probe()?
>
> I'm not sure, I thought that if there's no "power-domains = " property
> in device node, that means the device has no PM domain. Some of the SoCs
> does not have independent CMOS domain. So does that means it will leave
> the dev->pm_domain as NULL or assign it as the genpd->pm_domain?

My point is, that no matter if there is a PM domain assigned or not,
the PCIe driver can still enable/use runtime PM. It shouldn't hurt.

>
> I need to do some homework to figure this out and will send a new patch
> to fix this if needed.

I did some more re-search and maybe the easiest thing for you to do is
to follow the pattern in the tegra PCIe controller driver.
drivers/pci/controller/pci-tegra.c.

That should definitely work for your case as well.

[...]

Kind regards
Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Mon, 2018-09-10 at 13:25 +0200, Ulf Hansson wrote:
> On 10 September 2018 at 11:42, Honghui Zhang <honghui.zhang@mediatek.com> wrote:
[...]

> >> > +               clk_disable_unprepare(port->sys_ck);
> >> > +               phy_power_off(port->phy);
> >> > +               phy_exit(port->phy);
> >> > +       }
> >> > +
> >> > +       mtk_pcie_subsys_powerdown(pcie);
> >>
> >> Why not gate clocks, unconditionally not depending on if pm_support is
> >> set or not, during system suspend?
> >>
> >> I understand that you for some SoCs needs also to restore registers
> >> during system resume, but that's a different thing, isn't it?
> >>
> >
> > Well, current host driver support different SoCs like mt7623, mt7622 and
> > mt2712. mt7623 need quite special take care of when system suspend. This
> > patch just add the suspend/resume support for mt7622&mt2712, while
> > mt7623 was excluded. This "pm_support" maybe removed after we have a
> > solution for mt7623 SoC of the system suspend.
> 
> Okay. So you simply want to take small steps, that works!
> 
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> >> > +{
> >> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> >> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> >> > +       struct mtk_pcie_port *port, *tmp;
> >> > +
> >> > +       if (!soc->pm_support)
> >> > +               return 0;
> >> > +
> >> > +       if (list_empty(&pcie->ports))
> >> > +               return 0;
> >> > +
> >> > +       if (dev->pm_domain) {
> >> > +               pm_runtime_enable(dev);
> >> > +               pm_runtime_get_sync(dev);
> >>
> >> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> >>
> >> I guess the reason to why you make these pm_runtime_*() calls, is
> >> because you want to restore the actions taken during
> >> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> >> pm_runtime_disable|put*())?
> >
> > Yes, that's what am I want to do.
> >
> >>
> >> If that's the case, I would rather avoid calling pm_runtime_disable()
> >> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> >> isn't needed.
> >
> >> Of course, another option is to follow the pattern in
> >> drivers/mmc/host/sdhci-xenon.c.
> >>
> >
> > Thanks.
> > I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
> > system suspend callbacks and set the device status to "PRM_SUSPENDED"
> > through the pm_runtime_force_suspend() interface.
> > I noticed that pm_runtime_put_sync will put the device status to
> > "PRM_SUSPENDED" status in some certain condition.
> > And I did not found the pci framework setting the status to
> > "PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
> > suspend_noirq phase.
> >
> > According to my understanding of PM, the system suspend flow does not
> > care about the runtime PM status. The runtime pm is responsible for the
> > CMOS gate, if the device want to gated it's CMOS, it need to call the
> > runtime pm interface obviously.
> 
> Well, during system suspend, the PM core disables runtime PM for all
> devices. See __device_suspend_late().
> 
> Additionally, the PM core prevents devices from being runtime
> suspended. See device_prepare(), as it calls
> pm_runtime_get_noresume().
> 
> In other words, drivers can not rely on calling pm_runtime_put_sync()
> from a ->suspend_noirq() callback to put its device into low power
> state.
> 
Thanks for your explain, that's a great help for me with the
understanding the PM logical.

> >
> > But I found that the PCIe framework will still operate with the device
> > after device driver suspend_noirq executed like save the configuration
> > space values through pci_save_state, So I guess the cmos could not be
> > gated at the suspend_noirq phase. I will update the patchset to remove
> > the pm runtime operations in the suspend_noirq phase.
> 
> That's doesn't sound correct to me. Although, I am not a PCI expert.
> 
Never mind, per my understanding, after the driver's suspend_noirq
callback executed, the CMOS should not be gated, since the PCI framework
still got work to do with the device.

> >
> >> Overall, I am also wondering why only runtime PM is used when there is
> >> a PM domain attached to the device? That seems to make the logic in
> >> the driver, unnecessary complicated. Why not use runtime
> >> unconditionally and thus enable it during ->probe()?
> >
> > I'm not sure, I thought that if there's no "power-domains = " property
> > in device node, that means the device has no PM domain. Some of the SoCs
> > does not have independent CMOS domain. So does that means it will leave
> > the dev->pm_domain as NULL or assign it as the genpd->pm_domain?
> 
> My point is, that no matter if there is a PM domain assigned or not,
> the PCIe driver can still enable/use runtime PM. It shouldn't hurt.
> 

Thanks, I will try and propose another patch for this.

> >
> > I need to do some homework to figure this out and will send a new patch
> > to fix this if needed.
> 
> I did some more re-search and maybe the easiest thing for you to do is
> to follow the pattern in the tegra PCIe controller driver.
> drivers/pci/controller/pci-tegra.c.
> 
> That should definitely work for your case as well.

Thanks, most of the PCIe host driver does not set the RUNTIME_PM_OPS for
suspend except pci-tegra.c. So my V4 version patch has followed majority
driver.

> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Mon, 2018-09-10 at 13:25 +0200, Ulf Hansson wrote:
> On 10 September 2018 at 11:42, Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
[...]

> >> > +               clk_disable_unprepare(port->sys_ck);
> >> > +               phy_power_off(port->phy);
> >> > +               phy_exit(port->phy);
> >> > +       }
> >> > +
> >> > +       mtk_pcie_subsys_powerdown(pcie);
> >>
> >> Why not gate clocks, unconditionally not depending on if pm_support is
> >> set or not, during system suspend?
> >>
> >> I understand that you for some SoCs needs also to restore registers
> >> during system resume, but that's a different thing, isn't it?
> >>
> >
> > Well, current host driver support different SoCs like mt7623, mt7622 and
> > mt2712. mt7623 need quite special take care of when system suspend. This
> > patch just add the suspend/resume support for mt7622&mt2712, while
> > mt7623 was excluded. This "pm_support" maybe removed after we have a
> > solution for mt7623 SoC of the system suspend.
> 
> Okay. So you simply want to take small steps, that works!
> 
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> >> > +{
> >> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> >> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> >> > +       struct mtk_pcie_port *port, *tmp;
> >> > +
> >> > +       if (!soc->pm_support)
> >> > +               return 0;
> >> > +
> >> > +       if (list_empty(&pcie->ports))
> >> > +               return 0;
> >> > +
> >> > +       if (dev->pm_domain) {
> >> > +               pm_runtime_enable(dev);
> >> > +               pm_runtime_get_sync(dev);
> >>
> >> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> >>
> >> I guess the reason to why you make these pm_runtime_*() calls, is
> >> because you want to restore the actions taken during
> >> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> >> pm_runtime_disable|put*())?
> >
> > Yes, that's what am I want to do.
> >
> >>
> >> If that's the case, I would rather avoid calling pm_runtime_disable()
> >> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> >> isn't needed.
> >
> >> Of course, another option is to follow the pattern in
> >> drivers/mmc/host/sdhci-xenon.c.
> >>
> >
> > Thanks.
> > I take a look at the drivers/mmc/sdhci-xenon.c's implement, it has add
> > system suspend callbacks and set the device status to "PRM_SUSPENDED"
> > through the pm_runtime_force_suspend() interface.
> > I noticed that pm_runtime_put_sync will put the device status to
> > "PRM_SUSPENDED" status in some certain condition.
> > And I did not found the pci framework setting the status to
> > "PRM_SUSPENDED". So I add the pm_runtime_put_sync/disable in
> > suspend_noirq phase.
> >
> > According to my understanding of PM, the system suspend flow does not
> > care about the runtime PM status. The runtime pm is responsible for the
> > CMOS gate, if the device want to gated it's CMOS, it need to call the
> > runtime pm interface obviously.
> 
> Well, during system suspend, the PM core disables runtime PM for all
> devices. See __device_suspend_late().
> 
> Additionally, the PM core prevents devices from being runtime
> suspended. See device_prepare(), as it calls
> pm_runtime_get_noresume().
> 
> In other words, drivers can not rely on calling pm_runtime_put_sync()
> from a ->suspend_noirq() callback to put its device into low power
> state.
> 
Thanks for your explain, that's a great help for me with the
understanding the PM logical.

> >
> > But I found that the PCIe framework will still operate with the device
> > after device driver suspend_noirq executed like save the configuration
> > space values through pci_save_state, So I guess the cmos could not be
> > gated at the suspend_noirq phase. I will update the patchset to remove
> > the pm runtime operations in the suspend_noirq phase.
> 
> That's doesn't sound correct to me. Although, I am not a PCI expert.
> 
Never mind, per my understanding, after the driver's suspend_noirq
callback executed, the CMOS should not be gated, since the PCI framework
still got work to do with the device.

> >
> >> Overall, I am also wondering why only runtime PM is used when there is
> >> a PM domain attached to the device? That seems to make the logic in
> >> the driver, unnecessary complicated. Why not use runtime
> >> unconditionally and thus enable it during ->probe()?
> >
> > I'm not sure, I thought that if there's no "power-domains = " property
> > in device node, that means the device has no PM domain. Some of the SoCs
> > does not have independent CMOS domain. So does that means it will leave
> > the dev->pm_domain as NULL or assign it as the genpd->pm_domain?
> 
> My point is, that no matter if there is a PM domain assigned or not,
> the PCIe driver can still enable/use runtime PM. It shouldn't hurt.
> 

Thanks, I will try and propose another patch for this.

> >
> > I need to do some homework to figure this out and will send a new patch
> > to fix this if needed.
> 
> I did some more re-search and maybe the easiest thing for you to do is
> to follow the pattern in the tegra PCIe controller driver.
> drivers/pci/controller/pci-tegra.c.
> 
> That should definitely work for your case as well.

Thanks, most of the PCIe host driver does not set the RUNTIME_PM_OPS for
suspend except pci-tegra.c. So my V4 version patch has followed majority
driver.

> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

On Fri, Sep 07, 2018 at 02:33:09PM +0200, Ulf Hansson wrote:

[...]

> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> 
> I am not sure I understand why you need to suspend the device in the
> "noirq" phase. Isn't it fine to do that in the regular suspend phase?
> 
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;

I have an additional comment on this. Isn't there a nicer and more
generic way to detect whether the soc supports pm rather than using
this utterly ugly pm_support static variable ?

Can't we sort out at core level (ie by not "registering" dev_pm_ops) ?

How is this situation handled in other drivers ?

Thanks,
Lorenzo

> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               clk_disable_unprepare(port->pipe_ck);
> > +               clk_disable_unprepare(port->obff_ck);
> > +               clk_disable_unprepare(port->axi_ck);
> > +               clk_disable_unprepare(port->aux_ck);
> > +               clk_disable_unprepare(port->ahb_ck);
> > +               clk_disable_unprepare(port->sys_ck);
> > +               phy_power_off(port->phy);
> > +               phy_exit(port->phy);
> > +       }
> > +
> > +       mtk_pcie_subsys_powerdown(pcie);
> 
> Why not gate clocks, unconditionally not depending on if pm_support is
> set or not, during system suspend?
> 
> I understand that you for some SoCs needs also to restore registers
> during system resume, but that's a different thing, isn't it?
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port, *tmp;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       if (dev->pm_domain) {
> > +               pm_runtime_enable(dev);
> > +               pm_runtime_get_sync(dev);
> 
> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> 
> I guess the reason to why you make these pm_runtime_*() calls, is
> because you want to restore the actions taken during
> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> pm_runtime_disable|put*())?
> 
> If that's the case, I would rather avoid calling pm_runtime_disable()
> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> isn't needed.
> 
> Of course, another option is to follow the pattern in
> drivers/mmc/host/sdhci-xenon.c.
> 
> Overall, I am also wondering why only runtime PM is used when there is
> a PM domain attached to the device? That seems to make the logic in
> the driver, unnecessary complicated. Why not use runtime
> unconditionally and thus enable it during ->probe()?
> 
> > +       }
> > +
> > +       clk_prepare_enable(pcie->free_ck);
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +               mtk_pcie_enable_port(port);
> > +
> > +       /* In case of EP was removed while system suspend. */
> > +       if (list_empty(&pcie->ports))
> > +               mtk_pcie_subsys_powerdown(pcie);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +                                     mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >         .ops = &mtk_pcie_ops,
> >         .startup = mtk_pcie_startup_port,
> >  };
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >         .need_fix_class_id = true,
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >                 .name = "mtk-pcie",
> >                 .of_match_table = mtk_pcie_ids,
> >                 .suppress_bind_attrs = true,
> > +               .pm = &mtk_pcie_pm_ops,
> >         },
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > --
> > 2.6.4
> >
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Lorenzo Pieralisi]

On Fri, Sep 07, 2018 at 02:33:09PM +0200, Ulf Hansson wrote:

[...]

> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> 
> I am not sure I understand why you need to suspend the device in the
> "noirq" phase. Isn't it fine to do that in the regular suspend phase?
> 
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;

I have an additional comment on this. Isn't there a nicer and more
generic way to detect whether the soc supports pm rather than using
this utterly ugly pm_support static variable ?

Can't we sort out at core level (ie by not "registering" dev_pm_ops) ?

How is this situation handled in other drivers ?

Thanks,
Lorenzo

> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               clk_disable_unprepare(port->pipe_ck);
> > +               clk_disable_unprepare(port->obff_ck);
> > +               clk_disable_unprepare(port->axi_ck);
> > +               clk_disable_unprepare(port->aux_ck);
> > +               clk_disable_unprepare(port->ahb_ck);
> > +               clk_disable_unprepare(port->sys_ck);
> > +               phy_power_off(port->phy);
> > +               phy_exit(port->phy);
> > +       }
> > +
> > +       mtk_pcie_subsys_powerdown(pcie);
> 
> Why not gate clocks, unconditionally not depending on if pm_support is
> set or not, during system suspend?
> 
> I understand that you for some SoCs needs also to restore registers
> during system resume, but that's a different thing, isn't it?
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > +       struct mtk_pcie_port *port, *tmp;
> > +
> > +       if (!soc->pm_support)
> > +               return 0;
> > +
> > +       if (list_empty(&pcie->ports))
> > +               return 0;
> > +
> > +       if (dev->pm_domain) {
> > +               pm_runtime_enable(dev);
> > +               pm_runtime_get_sync(dev);
> 
> I noticed, Lorenzo wanted me to comment on this, so here it goes.
> 
> I guess the reason to why you make these pm_runtime_*() calls, is
> because you want to restore the actions taken during
> mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> pm_runtime_disable|put*())?
> 
> If that's the case, I would rather avoid calling pm_runtime_disable()
> and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> isn't needed.
> 
> Of course, another option is to follow the pattern in
> drivers/mmc/host/sdhci-xenon.c.
> 
> Overall, I am also wondering why only runtime PM is used when there is
> a PM domain attached to the device? That seems to make the logic in
> the driver, unnecessary complicated. Why not use runtime
> unconditionally and thus enable it during ->probe()?
> 
> > +       }
> > +
> > +       clk_prepare_enable(pcie->free_ck);
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > +               mtk_pcie_enable_port(port);
> > +
> > +       /* In case of EP was removed while system suspend. */
> > +       if (list_empty(&pcie->ports))
> > +               mtk_pcie_subsys_powerdown(pcie);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > +                                     mtk_pcie_resume_noirq)
> > +};
> > +
> >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >         .ops = &mtk_pcie_ops,
> >         .startup = mtk_pcie_startup_port,
> >  };
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> >         .need_fix_class_id = true,
> > +       .pm_support = true,
> >         .ops = &mtk_pcie_ops_v2,
> >         .startup = mtk_pcie_startup_port_v2,
> >         .setup_irq = mtk_pcie_setup_irq,
> > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> >                 .name = "mtk-pcie",
> >                 .of_match_table = mtk_pcie_ids,
> >                 .suppress_bind_attrs = true,
> > +               .pm = &mtk_pcie_pm_ops,
> >         },
> >  };
> >  builtin_platform_driver(mtk_pcie_driver);
> > --
> > 2.6.4
> >
> 
> Kind regards
> Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Fri, 2018-09-21 at 18:10 +0100, Lorenzo Pieralisi wrote:
> On Fri, Sep 07, 2018 at 02:33:09PM +0200, Ulf Hansson wrote:
> 
> [...]
> 
> > > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > 
> > I am not sure I understand why you need to suspend the device in the
> > "noirq" phase. Isn't it fine to do that in the regular suspend phase?
> > 
> > > +{
> > > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > > +       struct mtk_pcie_port *port;
> > > +
> > > +       if (!soc->pm_support)
> > > +               return 0;
> 
> I have an additional comment on this. Isn't there a nicer and more
> generic way to detect whether the soc supports pm rather than using
> this utterly ugly pm_support static variable ?
> 
> Can't we sort out at core level (ie by not "registering" dev_pm_ops) ?
> 
> How is this situation handled in other drivers ?
> 

Hmm, yes, this pm_support is removable, I guess we add the system
suspend callbacks for MT7622 is fine, if MT7622 is not supported system
suspend, I guess these callbacks will never have a change to be
executed. Adding these callbacks will have no harm for MT7622.

Anyway, I will check other host driver, and try another approach to
distinguish those SoCs for system suspend callbacks.

Thanks very much for your comments.

> Thanks,
> Lorenzo
> 
> > > +       if (list_empty(&pcie->ports))
> > > +               return 0;
> > > +
> > > +       list_for_each_entry(port, &pcie->ports, list) {
> > > +               clk_disable_unprepare(port->pipe_ck);
> > > +               clk_disable_unprepare(port->obff_ck);
> > > +               clk_disable_unprepare(port->axi_ck);
> > > +               clk_disable_unprepare(port->aux_ck);
> > > +               clk_disable_unprepare(port->ahb_ck);
> > > +               clk_disable_unprepare(port->sys_ck);
> > > +               phy_power_off(port->phy);
> > > +               phy_exit(port->phy);
> > > +       }
> > > +
> > > +       mtk_pcie_subsys_powerdown(pcie);
> > 
> > Why not gate clocks, unconditionally not depending on if pm_support is
> > set or not, during system suspend?
> > 
> > I understand that you for some SoCs needs also to restore registers
> > during system resume, but that's a different thing, isn't it?
> > 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > > +       struct mtk_pcie_port *port, *tmp;
> > > +
> > > +       if (!soc->pm_support)
> > > +               return 0;
> > > +
> > > +       if (list_empty(&pcie->ports))
> > > +               return 0;
> > > +
> > > +       if (dev->pm_domain) {
> > > +               pm_runtime_enable(dev);
> > > +               pm_runtime_get_sync(dev);
> > 
> > I noticed, Lorenzo wanted me to comment on this, so here it goes.
> > 
> > I guess the reason to why you make these pm_runtime_*() calls, is
> > because you want to restore the actions taken during
> > mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> > pm_runtime_disable|put*())?
> > 
> > If that's the case, I would rather avoid calling pm_runtime_disable()
> > and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> > isn't needed.
> > 
> > Of course, another option is to follow the pattern in
> > drivers/mmc/host/sdhci-xenon.c.
> > 
> > Overall, I am also wondering why only runtime PM is used when there is
> > a PM domain attached to the device? That seems to make the logic in
> > the driver, unnecessary complicated. Why not use runtime
> > unconditionally and thus enable it during ->probe()?
> > 
> > > +       }
> > > +
> > > +       clk_prepare_enable(pcie->free_ck);
> > > +
> > > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > > +               mtk_pcie_enable_port(port);
> > > +
> > > +       /* In case of EP was removed while system suspend. */
> > > +       if (list_empty(&pcie->ports))
> > > +               mtk_pcie_subsys_powerdown(pcie);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > > +                                     mtk_pcie_resume_noirq)
> > > +};
> > > +
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> > >         .ops = &mtk_pcie_ops,
> > >         .startup = mtk_pcie_startup_port,
> > >  };
> > >
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > > +       .pm_support = true,
> > >         .ops = &mtk_pcie_ops_v2,
> > >         .startup = mtk_pcie_startup_port_v2,
> > >         .setup_irq = mtk_pcie_setup_irq,
> > > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > >
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> > >         .need_fix_class_id = true,
> > > +       .pm_support = true,
> > >         .ops = &mtk_pcie_ops_v2,
> > >         .startup = mtk_pcie_startup_port_v2,
> > >         .setup_irq = mtk_pcie_setup_irq,
> > > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> > >                 .name = "mtk-pcie",
> > >                 .of_match_table = mtk_pcie_ids,
> > >                 .suppress_bind_attrs = true,
> > > +               .pm = &mtk_pcie_pm_ops,
> > >         },
> > >  };
> > >  builtin_platform_driver(mtk_pcie_driver);
> > > --
> > > 2.6.4
> > >
> > 
> > Kind regards
> > Uffe

Re: [PATCH v3 3/4] PCI: mediatek: Add system pm support for MT2712 and MT7622

[Honghui Zhang]

On Fri, 2018-09-21 at 18:10 +0100, Lorenzo Pieralisi wrote:
> On Fri, Sep 07, 2018 at 02:33:09PM +0200, Ulf Hansson wrote:
> 
> [...]
> 
> > > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
> > 
> > I am not sure I understand why you need to suspend the device in the
> > "noirq" phase. Isn't it fine to do that in the regular suspend phase?
> > 
> > > +{
> > > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > > +       struct mtk_pcie_port *port;
> > > +
> > > +       if (!soc->pm_support)
> > > +               return 0;
> 
> I have an additional comment on this. Isn't there a nicer and more
> generic way to detect whether the soc supports pm rather than using
> this utterly ugly pm_support static variable ?
> 
> Can't we sort out at core level (ie by not "registering" dev_pm_ops) ?
> 
> How is this situation handled in other drivers ?
> 

Hmm, yes, this pm_support is removable, I guess we add the system
suspend callbacks for MT7622 is fine, if MT7622 is not supported system
suspend, I guess these callbacks will never have a change to be
executed. Adding these callbacks will have no harm for MT7622.

Anyway, I will check other host driver, and try another approach to
distinguish those SoCs for system suspend callbacks.

Thanks very much for your comments.

> Thanks,
> Lorenzo
> 
> > > +       if (list_empty(&pcie->ports))
> > > +               return 0;
> > > +
> > > +       list_for_each_entry(port, &pcie->ports, list) {
> > > +               clk_disable_unprepare(port->pipe_ck);
> > > +               clk_disable_unprepare(port->obff_ck);
> > > +               clk_disable_unprepare(port->axi_ck);
> > > +               clk_disable_unprepare(port->aux_ck);
> > > +               clk_disable_unprepare(port->ahb_ck);
> > > +               clk_disable_unprepare(port->sys_ck);
> > > +               phy_power_off(port->phy);
> > > +               phy_exit(port->phy);
> > > +       }
> > > +
> > > +       mtk_pcie_subsys_powerdown(pcie);
> > 
> > Why not gate clocks, unconditionally not depending on if pm_support is
> > set or not, during system suspend?
> > 
> > I understand that you for some SoCs needs also to restore registers
> > during system resume, but that's a different thing, isn't it?
> > 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +       struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > > +       const struct mtk_pcie_soc *soc = pcie->soc;
> > > +       struct mtk_pcie_port *port, *tmp;
> > > +
> > > +       if (!soc->pm_support)
> > > +               return 0;
> > > +
> > > +       if (list_empty(&pcie->ports))
> > > +               return 0;
> > > +
> > > +       if (dev->pm_domain) {
> > > +               pm_runtime_enable(dev);
> > > +               pm_runtime_get_sync(dev);
> > 
> > I noticed, Lorenzo wanted me to comment on this, so here it goes.
> > 
> > I guess the reason to why you make these pm_runtime_*() calls, is
> > because you want to restore the actions taken during
> > mtk_pcie_suspend_noirq() (which calls mtk_pcie_subsys_powerdown ->
> > pm_runtime_disable|put*())?
> > 
> > If that's the case, I would rather avoid calling pm_runtime_disable()
> > and pm_runtime_put() via mtk_pcie_suspend_noirq(), simply because it
> > isn't needed.
> > 
> > Of course, another option is to follow the pattern in
> > drivers/mmc/host/sdhci-xenon.c.
> > 
> > Overall, I am also wondering why only runtime PM is used when there is
> > a PM domain attached to the device? That seems to make the logic in
> > the driver, unnecessary complicated. Why not use runtime
> > unconditionally and thus enable it during ->probe()?
> > 
> > > +       }
> > > +
> > > +       clk_prepare_enable(pcie->free_ck);
> > > +
> > > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > > +               mtk_pcie_enable_port(port);
> > > +
> > > +       /* In case of EP was removed while system suspend. */
> > > +       if (list_empty(&pcie->ports))
> > > +               mtk_pcie_subsys_powerdown(pcie);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops mtk_pcie_pm_ops = {
> > > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
> > > +                                     mtk_pcie_resume_noirq)
> > > +};
> > > +
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> > >         .ops = &mtk_pcie_ops,
> > >         .startup = mtk_pcie_startup_port,
> > >  };
> > >
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > > +       .pm_support = true,
> > >         .ops = &mtk_pcie_ops_v2,
> > >         .startup = mtk_pcie_startup_port_v2,
> > >         .setup_irq = mtk_pcie_setup_irq,
> > > @@ -1210,6 +1275,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > >
> > >  static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> > >         .need_fix_class_id = true,
> > > +       .pm_support = true,
> > >         .ops = &mtk_pcie_ops_v2,
> > >         .startup = mtk_pcie_startup_port_v2,
> > >         .setup_irq = mtk_pcie_setup_irq,
> > > @@ -1229,6 +1295,7 @@ static struct platform_driver mtk_pcie_driver = {
> > >                 .name = "mtk-pcie",
> > >                 .of_match_table = mtk_pcie_ids,
> > >                 .suppress_bind_attrs = true,
> > > +               .pm = &mtk_pcie_pm_ops,
> > >         },
> > >  };
> > >  builtin_platform_driver(mtk_pcie_driver);
> > > --
> > > 2.6.4
> > >
> > 
> > Kind regards
> > Uffe