[PATCH 0/2] PCI: meson: Add system suspend/resume support

[PATCH 0/2] PCI: meson: Add system suspend/resume support

[Igor Velkov via B4 Relay]

On SM1 the PCIe controller loses power in system suspend and the driver
has no PM callbacks, so the first config access after resume hangs the
CPU; on ODROID-HC4 s2idle never returns.

Patch 1 moves the PHY power on and the resets into the host .init and
.deinit callbacks.  Patch 2 adds noirq suspend/resume through the DWC
helpers, for SM1 only.

Tested on ODROID-HC4 (SM1, ASM1061 SATA with two disks behind the port)
on an Armbian v7.4-rc2 kernel with this series, which also carries
Hans Zhang's pending MPS series (v9): 20 s2idle and 20 suspend-to-RAM
cycles, the link back at Gen2 x1 and the disk readable after each.
Suspend-to-RAM woke on the VRTC alarm via a patched BL301; the stock
firmware has no timer wake on this board.

Not tested on G12A, G12B or AXG.  There patch 1 changes probe and
remove, and patch 2 leaves suspend alone.

After a failed resume, dw_pcie_suspend_noirq() calls .deinit a second
time; pci-imx6 and pcie-rcar-gen4 have no guard against it.  Patch 2
guards with a flag; I can fix it in the DWC core instead if preferred.

Signed-off-by: Igor Velkov <iav@iav.lv>
---
Igor Velkov (2):
      PCI: meson: Power the PHY on and reset the controller in host .init
      PCI: meson: Add system suspend/resume support

 drivers/pci/controller/dwc/pci-meson.c | 108 +++++++++++++++++++++++++++------
 1 file changed, 89 insertions(+), 19 deletions(-)
---
base-commit: 35f800669ce835aa77364a2ab5b61be19b778d3c
change-id: 20261114-b4-pci-meson-suspend-20798a7b9956

Best regards,
-- 
Igor Velkov <iav@iav.lv>



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

[PATCH 1/2] PCI: meson: Power the PHY on and reset the controller in host .init

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

Move the PHY power on and the PHY and controller reset from probe into
the host .init callback, and add a .deinit that asserts PERST# and
powers the PHY off, and a .stop_link that disables the LTSSM.
dw_pcie_host_init() calls .deinit on a later probe error and
dw_pcie_host_deinit() calls it at remove, so probe and remove do not
power the PHY off themselves.

This prepares for system suspend, where dw_pcie_suspend_noirq() and
dw_pcie_resume_noirq() run the same callbacks.

It also changes the order in probe: the clocks are enabled before the
PHY is powered on and the controller is reset.  Resume runs .init with
the clocks still on, so both take the same order.

Signed-off-by: Igor Velkov <iav@iav.lv>
---
 drivers/pci/controller/dwc/pci-meson.c | 55 ++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 03dd1282f3f3..627470c83bb5 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -315,6 +315,16 @@ static int meson_pcie_start_link(struct dw_pcie *pci)
 	return 0;
 }
 
+static void meson_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct meson_pcie *mp = to_meson_pcie(pci);
+	u32 val;
+
+	val = meson_cfg_readl(mp, PCIE_CFG0);
+	val &= ~APP_LTSSM_ENABLE;
+	meson_cfg_writel(mp, val, PCIE_CFG0);
+}
+
 static int meson_pcie_rd_own_conf(struct pci_bus *bus, u32 devfn,
 				  int where, int size, u32 *val)
 {
@@ -360,6 +370,20 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct meson_pcie *mp = to_meson_pcie(pci);
+	int ret;
+
+	ret = meson_pcie_power_on(mp);
+	if (ret) {
+		dev_err(pci->dev, "phy power on failed, %d\n", ret);
+		return ret;
+	}
+
+	ret = meson_pcie_reset(mp);
+	if (ret) {
+		dev_err(pci->dev, "reset failed, %d\n", ret);
+		meson_pcie_power_off(mp);
+		return ret;
+	}
 
 	pp->bridge->ops = &meson_pci_ops;
 
@@ -369,13 +393,23 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static void meson_pcie_host_deinit(struct dw_pcie_rp *pp)
+{
+	struct meson_pcie *mp = to_meson_pcie(to_dw_pcie_from_pp(pp));
+
+	gpiod_set_value_cansleep(mp->reset_gpio, 1);
+	meson_pcie_power_off(mp);
+}
+
 static const struct dw_pcie_host_ops meson_pcie_host_ops = {
 	.init = meson_pcie_host_init,
+	.deinit = meson_pcie_host_deinit,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = meson_pcie_link_up,
 	.start_link = meson_pcie_start_link,
+	.stop_link = meson_pcie_stop_link,
 };
 
 static int meson_pcie_probe(struct platform_device *pdev)
@@ -419,22 +453,10 @@ static int meson_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = meson_pcie_power_on(mp);
-	if (ret) {
-		dev_err(dev, "phy power on failed, %d\n", ret);
-		return ret;
-	}
-
-	ret = meson_pcie_reset(mp);
-	if (ret) {
-		dev_err(dev, "reset failed, %d\n", ret);
-		goto err_phy;
-	}
-
 	ret = meson_pcie_probe_clocks(mp);
 	if (ret) {
 		dev_err(dev, "init clock resources failed, %d\n", ret);
-		goto err_phy;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, mp);
@@ -442,14 +464,10 @@ static int meson_pcie_probe(struct platform_device *pdev)
 	ret = dw_pcie_host_init(&pci->pp);
 	if (ret < 0) {
 		dev_err(dev, "Add PCIe port failed, %d\n", ret);
-		goto err_phy;
+		return ret;
 	}
 
 	return 0;
-
-err_phy:
-	meson_pcie_power_off(mp);
-	return ret;
 }
 
 static void meson_pcie_remove(struct platform_device *pdev)
@@ -457,7 +475,6 @@ static void meson_pcie_remove(struct platform_device *pdev)
 	struct meson_pcie *mp = platform_get_drvdata(pdev);
 
 	dw_pcie_host_deinit(&mp->pci.pp);
-	meson_pcie_power_off(mp);
 }
 
 static const struct of_device_id meson_pcie_of_match[] = {

-- 
2.43.0



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

[PATCH 2/2] PCI: meson: Add system suspend/resume support

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

On SM1 the controller sits in the PCIE power domain, which genpd
switches off in the noirq suspend phase.  The driver has no PM
callbacks, so the controller comes back unprogrammed and the first
config access to the Root Port after resume hangs the CPU.  On
ODROID-HC4 (ASM1061 SATA behind the port) s2idle never returns.

Add noirq PM callbacks that call dw_pcie_suspend_noirq() and
dw_pcie_resume_noirq(): suspend broadcasts PME_Turn_Off and waits for
L2 if the link is up, then calls .deinit; resume calls .init, programs
the Root Complex, MSI and iATU registers and restarts the link.  Set
suspend_poweroff, so that suspend enters L2 even when D3cold is not
possible.

Send PME_Turn_Off through the apps_pm_xmt_turnoff bit of PCIE_CFG0
in a .pme_turn_off callback, so that no outbound iATU window is needed
for the Msg TLP.  Bit 12 is apps_pm_xmt_turnoff in the description of
this wrapper in the Amlogic A311D (G12B) datasheet, where bit 7 is the
app_ltssm_enable the driver already uses.

Do this only when the controller has a power domain, as on SM1.  G12A,
G12B and AXG have none; entering L2 and powering the PHY off in
suspend is untested there, so the callbacks return at once.

A resume that fails to start the link or times out waiting for it
calls .deinit itself, and the next suspend calls it again.  A second
phy_power_off() and phy_exit() would unbalance the PHY counts, and the
next .init would not power the PHY on, so .deinit leaves a controller
that is already off alone.

Signed-off-by: Igor Velkov <iav@iav.lv>
---
 drivers/pci/controller/dwc/pci-meson.c | 53 ++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 627470c83bb5..5ef7369bb21b 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -9,6 +9,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
@@ -27,6 +28,7 @@
 /* PCIe specific config registers */
 #define PCIE_CFG0			0x0
 #define APP_LTSSM_ENABLE		BIT(7)
+#define APPS_PM_XMT_TURNOFF		BIT(12)
 
 #define PCIE_CFG_STATUS12		0x30
 #define IS_SMLH_LINK_UP(x)		((x) & (1 << 6))
@@ -68,6 +70,8 @@ struct meson_pcie {
 	struct meson_pcie_rc_reset mrst;
 	struct gpio_desc *reset_gpio;
 	struct phy *phy;
+	bool power_domain;
+	bool powered;
 };
 
 static struct reset_control *meson_pcie_get_reset(struct meson_pcie *mp,
@@ -385,6 +389,8 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 		return ret;
 	}
 
+	mp->powered = true;
+
 	pp->bridge->ops = &meson_pci_ops;
 
 	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
@@ -397,13 +403,29 @@ static void meson_pcie_host_deinit(struct dw_pcie_rp *pp)
 {
 	struct meson_pcie *mp = to_meson_pcie(to_dw_pcie_from_pp(pp));
 
+	/* After a failed resume the next suspend calls .deinit once more */
+	if (!mp->powered)
+		return;
+
 	gpiod_set_value_cansleep(mp->reset_gpio, 1);
 	meson_pcie_power_off(mp);
+	mp->powered = false;
+}
+
+static void meson_pcie_pme_turn_off(struct dw_pcie_rp *pp)
+{
+	struct meson_pcie *mp = to_meson_pcie(to_dw_pcie_from_pp(pp));
+	u32 val;
+
+	val = meson_cfg_readl(mp, PCIE_CFG0);
+	meson_cfg_writel(mp, val | APPS_PM_XMT_TURNOFF, PCIE_CFG0);
+	meson_cfg_writel(mp, val, PCIE_CFG0);
 }
 
 static const struct dw_pcie_host_ops meson_pcie_host_ops = {
 	.init = meson_pcie_host_init,
 	.deinit = meson_pcie_host_deinit,
+	.pme_turn_off = meson_pcie_pme_turn_off,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -429,6 +451,13 @@ static int meson_pcie_probe(struct platform_device *pdev)
 	pci->pp.ops = &meson_pcie_host_ops;
 	pci->num_lanes = 1;
 
+	/*
+	 * Genpd switches the controller's power domain off in suspend.  Only
+	 * SM1 has one, and only SM1 gets suspend support.
+	 */
+	mp->power_domain = of_property_present(dev->of_node, "power-domains");
+	pci->pp.suspend_poweroff = mp->power_domain;
+
 	mp->phy = devm_phy_get(dev, "pcie");
 	if (IS_ERR(mp->phy)) {
 		dev_err(dev, "get phy failed, %pe\n", mp->phy);
@@ -477,6 +506,29 @@ static void meson_pcie_remove(struct platform_device *pdev)
 	dw_pcie_host_deinit(&mp->pci.pp);
 }
 
+static int meson_pcie_suspend_noirq(struct device *dev)
+{
+	struct meson_pcie *mp = dev_get_drvdata(dev);
+
+	if (!mp->power_domain)
+		return 0;
+
+	return dw_pcie_suspend_noirq(&mp->pci);
+}
+
+static int meson_pcie_resume_noirq(struct device *dev)
+{
+	struct meson_pcie *mp = dev_get_drvdata(dev);
+
+	if (!mp->power_domain)
+		return 0;
+
+	return dw_pcie_resume_noirq(&mp->pci);
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(meson_pcie_pm_ops, meson_pcie_suspend_noirq,
+			       meson_pcie_resume_noirq);
+
 static const struct of_device_id meson_pcie_of_match[] = {
 	{
 		.compatible = "amlogic,axg-pcie",
@@ -494,6 +546,7 @@ static struct platform_driver meson_pcie_driver = {
 	.driver = {
 		.name = "meson-pcie",
 		.of_match_table = meson_pcie_of_match,
+		.pm = pm_sleep_ptr(&meson_pcie_pm_ops),
 	},
 };
 

-- 
2.43.0



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

Re: [PATCH 2/2] PCI: meson: Add system suspend/resume support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] PCI: meson: accidental clearing of W1C bits in Device Status
  register [1]
--

commit f7a5cff83ce20dd936f701817faa8230379ab3ad
Author: Igor Velkov <iav@iav.lv>

PCI: meson: Add system suspend/resume support

This commit adds noirq suspend and resume callbacks for the Amlogic Meson
PCIe controller when attached to a power domain, along with a
.pme_turn_off callback and power state tracking across host init and
deinit.

> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 627470c83bb5d..5ef7369bb21bf 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c

[ ... ]

> @@ -385,6 +389,8 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
>  		return ret;
>  	}
>  
> +	mp->powered = true;
> +
>  	pp->bridge->ops = &meson_pci_ops;
>  
>  	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);

[Severity: Medium]
This problem wasn't introduced by this patch, but can
meson_set_max_payload() and meson_set_max_rd_req_size() inadvertently
clear write-1-to-clear status bits in PCI_EXP_DEVSTA [1] when
meson_pcie_host_init() runs on probe and resume?

drivers/pci/controller/dwc/pci-meson.c:meson_set_max_payload() {
    ...
	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);

	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
}

Both helpers perform 32-bit read-modify-write accesses at offset +
PCI_EXP_DEVCTL, which includes the 16-bit PCI_EXP_DEVSTA register in the
upper half of the word.  Writing back 1s read from PCI_EXP_DEVSTA clears
pending correctable or non-fatal error status bits, and using
dw_pcie_readw_dbi() and dw_pcie_writew_dbi() would avoid touching
PCI_EXP_DEVSTA.

[1] https://sashiko.dev/#/bug/linux-607d4e29-7318-49b1-a7c3-c91a4c4f26a0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-pci-meson-suspend-v1-0-be3d04a13304@iav.lv?part=2

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