[PATCH v3 00/18] Add suspend to ram support for PCIe on J7200

[PATCH v3 00/18] Add suspend to ram support for PCIe on J7200

[Thomas Richard]

This add suspend to ram support for the PCIe (RC mode) on J7200 platform.

In RC mode, the reset pin for endpoints is managed by a gpio expander on a
i2c bus. This pin shall be managed in suspend_noirq() and resume_noirq().
The suspend/resume has been moved to suspend_noirq()/resume_noirq() for
pca953x (expander) and pinctrl-single.

To do i2c accesses during suspend_noirq/resume_noirq, we need to force the
wakeup of the i2c controller (which is autosuspended) during suspend
callback. 
It's the only way to wakeup the controller if it's autosuspended, as
runtime pm is disabled in suspend_noirq and resume_noirq.

The main change in this v3 is the removal of the probe boolean for the
functions wiz_clock_probe() and cdns_pcie_host_setup().
Their contents were split in multiple functions which are called in the
resume_noirq() callbacks.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Changes in v3:
- pinctrl-single: split patch in two parts, a first patch to remove the
  dead code, a second to move suspend()/resume() callbacks to noirq.
- i2c-omap: add a comments above the suspend_noirq() callback.
- mux: now mux_chip_resume() try to restores all muxes, then return 0 if
  all is ok or the first failure.
- mmio: fix commit message.
- phy-j721e-wiz: add a patch to use dev_err_probe() instead of dev_err() in
  the wiz_clock_init() function.
- phy-j721e-wiz: remove probe boolean for the wiz_clock_init(), rename the
  function to wiz_clock_probe(), extract hardware configuration part in a
  new function wiz_clock_init().
- phy-cadence-torrent: use dev_err_probe() and fix commit messages
- pcie-cadence-host: remove probe boolean for the cdns_pcie_host_setup()
  function and extract the link setup part in a new function
  cdns_pcie_host_link_setup().
- pcie-cadence-host: make cdns_pcie_host_init() global.
- pci-j721e: use the cdns_pcie_host_link_setup() cdns_pcie_host_init()
  functions in the resume_noirq() callback.
- Link to v2: https://lore.kernel.org/r/20240102-j7200-pcie-s2r-v2-0-8e4f7d228ec2@bootlin.com

Changes in v2:
- all: fix commits messages.
- all: use DEFINE_NOIRQ_DEV_PM_OPS and pm_sleep_ptr macros.
- all: remove useless #ifdef CONFIG_PM.
- pinctrl-single: drop dead code
- mux: add mux_chip_resume() function in mux core.
- mmio: resume sequence is now a call to mux_chip_resume().
- phy-cadence-torrent: fix typo in resume sequence (reset_control_assert()
  instead of reset_control_put()).
- phy-cadence-torrent: use PHY instead of phy.
- pci-j721e: do not shadow cdns_pcie_host_setup return code in resume
  sequence.
- pci-j721e: drop dead code.
- Link to v1: https://lore.kernel.org/r/20240102-j7200-pcie-s2r-v1-0-84e55da52400@bootlin.com

---
Thomas Richard (15):
      gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()
      pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks
      pinctrl: pinctrl-single: move suspend()/resume() callbacks to noirq
      i2c: omap: wakeup the controller during suspend() callback
      mux: add mux_chip_resume() function
      phy: ti: phy-j721e-wiz: use dev_err_probe() instead of dev_err()
      phy: ti: phy-j721e-wiz: split wiz_clock_init() function
      phy: ti: phy-j721e-wiz: add resume support
      phy: cadence-torrent: extract calls to clk_get from cdns_torrent_clk
      phy: cadence-torrent: register resets even if the phy is already configured
      phy: cadence-torrent: add already_configured to struct cdns_torrent_phy
      phy: cadence-torrent: remove noop_ops phy operations
      phy: cadence-torrent: add suspend and resume support
      PCI: cadence: extract link setup sequence from cdns_pcie_host_setup()
      PCI: cadence: set cdns_pcie_host_init() global

Théo Lebrun (3):
      mux: mmio: add resume support
      PCI: j721e: add reset GPIO to struct j721e_pcie
      PCI: j721e: add suspend and resume support

 drivers/gpio/gpio-pca953x.c                        |   7 +-
 drivers/i2c/busses/i2c-omap.c                      |  22 ++++
 drivers/mux/core.c                                 |  30 +++++
 drivers/mux/mmio.c                                 |  12 ++
 drivers/pci/controller/cadence/pci-j721e.c         | 102 ++++++++++++++++-
 drivers/pci/controller/cadence/pcie-cadence-host.c |  44 +++++---
 drivers/pci/controller/cadence/pcie-cadence.h      |  12 ++
 drivers/phy/cadence/phy-cadence-torrent.c          | 121 +++++++++++++++------
 drivers/phy/ti/phy-j721e-wiz.c                     | 119 +++++++++++++-------
 drivers/pinctrl/pinctrl-single.c                   |  28 ++---
 include/linux/mux/driver.h                         |   1 +
 11 files changed, 380 insertions(+), 118 deletions(-)
---
base-commit: 00ff0f9ce40db8e64fe16c424a965fd7ab769c42
change-id: 20240102-j7200-pcie-s2r-ecb1a979e357

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH v3 01/18] gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()

[Thomas Richard]

Some IOs can be needed during suspend_noirq()/resume_noirq().
So move suspend()/resume() to noirq.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 00ffa168e405..6e495fc67a93 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1234,7 +1234,7 @@ static void pca953x_save_context(struct pca953x_chip *chip)
 	regcache_cache_only(chip->regmap, true);
 }
 
-static int pca953x_suspend(struct device *dev)
+static int pca953x_suspend_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 
@@ -1248,7 +1248,7 @@ static int pca953x_suspend(struct device *dev)
 	return 0;
 }
 
-static int pca953x_resume(struct device *dev)
+static int pca953x_resume_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 	int ret;
@@ -1268,7 +1268,8 @@ static int pca953x_resume(struct device *dev)
 	return ret;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
+static DEFINE_NOIRQ_DEV_PM_OPS(pca953x_pm_ops,
+			       pca953x_suspend_noirq, pca953x_resume_noirq);
 
 /* convenience to stop overlong match-table lines */
 #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))

-- 
2.39.2

[PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Thomas Richard]

No need to check the pointer returned by platform_get_drvdata(), as
platform_set_drvdata() is called during the probe.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/pinctrl-single.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 19cc0db771a5..02eabd28d46e 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1693,11 +1693,7 @@ static void pcs_restore_context(struct pcs_device *pcs)
 static int pinctrl_single_suspend(struct platform_device *pdev,
 					pm_message_t state)
 {
-	struct pcs_device *pcs;
-
-	pcs = platform_get_drvdata(pdev);
-	if (!pcs)
-		return -EINVAL;
+	struct pcs_device *pcs = platform_get_drvdata(pdev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
 		int ret;
@@ -1712,11 +1708,7 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
 
 static int pinctrl_single_resume(struct platform_device *pdev)
 {
-	struct pcs_device *pcs;
-
-	pcs = platform_get_drvdata(pdev);
-	if (!pcs)
-		return -EINVAL;
+	struct pcs_device *pcs = platform_get_drvdata(pdev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
 		pcs_restore_context(pcs);

-- 
2.39.2

[PATCH v3 03/18] pinctrl: pinctrl-single: move suspend()/resume() callbacks to noirq

[Thomas Richard]

The goal is to extend the active period of pinctrl.
Some devices may need active pinctrl after suspend() and/or before
resume().
So move suspend()/resume() to suspend_noirq()/resume_noirq() in order to
have active pinctrl until suspend_noirq() (included), and from
resume_noirq() (included).

The deprecated API has been removed to use the new one (dev_pm_ops struct).

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/pinctrl-single.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 02eabd28d46e..0dd4b0e11adf 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1625,7 +1625,6 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs,
 	return 0;
 }
 
-#ifdef CONFIG_PM
 static int pcs_save_context(struct pcs_device *pcs)
 {
 	int i, mux_bytes;
@@ -1690,10 +1689,9 @@ static void pcs_restore_context(struct pcs_device *pcs)
 	}
 }
 
-static int pinctrl_single_suspend(struct platform_device *pdev,
-					pm_message_t state)
+static int pinctrl_single_suspend_noirq(struct device *dev)
 {
-	struct pcs_device *pcs = platform_get_drvdata(pdev);
+	struct pcs_device *pcs = dev_get_drvdata(dev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
 		int ret;
@@ -1706,16 +1704,19 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
 	return pinctrl_force_sleep(pcs->pctl);
 }
 
-static int pinctrl_single_resume(struct platform_device *pdev)
+static int pinctrl_single_resume_noirq(struct device *dev)
 {
-	struct pcs_device *pcs = platform_get_drvdata(pdev);
+	struct pcs_device *pcs = dev_get_drvdata(dev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
 		pcs_restore_context(pcs);
 
 	return pinctrl_force_default(pcs->pctl);
 }
-#endif
+
+static DEFINE_NOIRQ_DEV_PM_OPS(pinctrl_single_pm_ops,
+			       pinctrl_single_suspend_noirq,
+			       pinctrl_single_resume_noirq);
 
 /**
  * pcs_quirk_missing_pinctrl_cells - handle legacy binding
@@ -1978,11 +1979,8 @@ static struct platform_driver pcs_driver = {
 	.driver = {
 		.name		= DRIVER_NAME,
 		.of_match_table	= pcs_of_match,
+		.pm = pm_sleep_ptr(&pinctrl_single_pm_ops),
 	},
-#ifdef CONFIG_PM
-	.suspend = pinctrl_single_suspend,
-	.resume = pinctrl_single_resume,
-#endif
 };
 
 module_platform_driver(pcs_driver);

-- 
2.39.2

[PATCH v3 04/18] i2c: omap: wakeup the controller during suspend() callback

[Thomas Richard]

A device may need the controller up during suspend_noirq() or
resume_noirq().
But if the controller is autosuspended, there is no way to wakeup it during
suspend_noirq() or resume_noirq() because runtime pm is disabled at this
time.

The suspend() callback wakes up the controller, so it is available until
its suspend_noirq() callback (pm_runtime_force_suspend()).
During the resume, it's restored by resume_noirq() callback
(pm_runtime_force_resume()). Then resume() callback enables autosuspend.

So the controller is up during a little time slot in suspend and resume
sequences even if it's not used.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/i2c/busses/i2c-omap.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 42165ef57946..c11deb285114 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1575,9 +1575,31 @@ static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int omap_i2c_suspend(struct device *dev)
+{
+	/*
+	 * If the controller is autosuspended, there is no way to wakeup it once
+	 * runtime pm is disabled (in suspend_late()).
+	 * But a device may need the controller up during suspend_noirq() or
+	 * resume_noirq().
+	 * Wakeup the controller while runtime pm is enabled, so it is available until
+	 * its suspend_noirq(), and from resume_noirq().
+	 */
+	return pm_runtime_resume_and_get(dev);
+}
+
+static int omap_i2c_resume(struct device *dev)
+{
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
 static const struct dev_pm_ops omap_i2c_pm_ops = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				      pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume)
 	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
 			   omap_i2c_runtime_resume, NULL)
 };

-- 
2.39.2

[PATCH v3 05/18] mux: add mux_chip_resume() function

[Thomas Richard]

The mux_chip_resume() function restores a mux_chip using the cached state
of each mux.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mux/core.c         | 30 ++++++++++++++++++++++++++++++
 include/linux/mux/driver.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..5db5a7698ad1 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -215,6 +215,36 @@ void mux_chip_free(struct mux_chip *mux_chip)
 }
 EXPORT_SYMBOL_GPL(mux_chip_free);
 
+/**
+ * mux_chip_resume() - restores the mux-chip state
+ * @mux_chip: The mux-chip to resume.
+ *
+ * Restores the mux-chip state.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int mux_chip_resume(struct mux_chip *mux_chip)
+{
+	int global_ret = 0;
+	int ret, i;
+
+	for (i = 0; i < mux_chip->controllers; ++i) {
+		struct mux_control *mux = &mux_chip->mux[i];
+
+		if (mux->cached_state == MUX_CACHE_UNKNOWN)
+			continue;
+
+		ret = mux_control_set(mux, mux->cached_state);
+		if (ret < 0) {
+			dev_err(&mux_chip->dev, "unable to restore state\n");
+			if (!global_ret)
+				global_ret = ret;
+		}
+	}
+	return global_ret;
+}
+EXPORT_SYMBOL_GPL(mux_chip_resume);
+
 static void devm_mux_chip_release(struct device *dev, void *res)
 {
 	struct mux_chip *mux_chip = *(struct mux_chip **)res;
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..2a7e5ec5d540 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -88,6 +88,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
 int mux_chip_register(struct mux_chip *mux_chip);
 void mux_chip_unregister(struct mux_chip *mux_chip);
 void mux_chip_free(struct mux_chip *mux_chip);
+int mux_chip_resume(struct mux_chip *mux_chip);
 
 struct mux_chip *devm_mux_chip_alloc(struct device *dev,
 				     unsigned int controllers,

-- 
2.39.2

[PATCH v3 06/18] mux: mmio: add resume support

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

No need to save something during the suspend stage, as the mux core has an
internal cache to store the state of muxes.

This cache is used by mux_chip_resume() to restore all muxes.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mux/mmio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index fd1d121a584b..892ff4714b68 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -125,13 +125,25 @@ static int mux_mmio_probe(struct platform_device *pdev)
 
 	mux_chip->ops = &mux_mmio_ops;
 
+	dev_set_drvdata(dev, mux_chip);
+
 	return devm_mux_chip_register(dev, mux_chip);
 }
 
+static int mux_mmio_resume_noirq(struct device *dev)
+{
+	struct mux_chip *mux_chip = dev_get_drvdata(dev);
+
+	return mux_chip_resume(mux_chip);
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(mux_mmio_pm_ops, NULL, mux_mmio_resume_noirq);
+
 static struct platform_driver mux_mmio_driver = {
 	.driver = {
 		.name = "mmio-mux",
 		.of_match_table	= mux_mmio_dt_ids,
+		.pm = pm_sleep_ptr(&mux_mmio_pm_ops),
 	},
 	.probe = mux_mmio_probe,
 };

-- 
2.39.2

[PATCH v3 07/18] phy: ti: phy-j721e-wiz: use dev_err_probe() instead of dev_err()

[Thomas Richard]

Use dev_err_probe() instead of dev_err() in wiz_clock_init() to simplify
the code and standardize the error output.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index fc3cd98c60ff..ce8a99801a4c 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1088,11 +1088,10 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 	int i;
 
 	clk = devm_clk_get(dev, "core_ref_clk");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "core_ref_clk clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
-	}
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "core_ref_clk clock not found\n");
+
 	wiz->input_clks[WIZ_CORE_REFCLK] = clk;
 
 	rate = clk_get_rate(clk);
@@ -1122,11 +1121,10 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 
 	if (wiz->data->pma_cmn_refclk1_int_mode) {
 		clk = devm_clk_get(dev, "core_ref1_clk");
-		if (IS_ERR(clk)) {
-			dev_err(dev, "core_ref1_clk clock not found\n");
-			ret = PTR_ERR(clk);
-			return ret;
-		}
+		if (IS_ERR(clk))
+			return dev_err_probe(dev, PTR_ERR(clk),
+					     "core_ref1_clk clock not found\n");
+
 		wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
 
 		rate = clk_get_rate(clk);
@@ -1137,11 +1135,10 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 	}
 
 	clk = devm_clk_get(dev, "ext_ref_clk");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "ext_ref_clk clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
-	}
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "ext_ref_clk clock not found\n");
+
 	wiz->input_clks[WIZ_EXT_REFCLK] = clk;
 
 	rate = clk_get_rate(clk);
@@ -1157,7 +1154,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 	case J721S2_WIZ_10G:
 		ret = wiz_clock_register(wiz);
 		if (ret)
-			dev_err(dev, "Failed to register wiz clocks\n");
+			dev_err_probe(dev, ret, "Failed to register wiz clocks\n");
 		return ret;
 	default:
 		break;
@@ -1175,8 +1172,8 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 		ret = wiz_mux_of_clk_register(wiz, clk_node, wiz->mux_sel_field[i],
 					      clk_mux_sel[i].table);
 		if (ret) {
-			dev_err(dev, "Failed to register %s clock\n",
-				node_name);
+			dev_err_probe(dev, ret, "Failed to register %s clock\n",
+				      node_name);
 			of_node_put(clk_node);
 			goto err;
 		}
@@ -1188,16 +1185,16 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 		node_name = clk_div_sel[i].node_name;
 		clk_node = of_get_child_by_name(node, node_name);
 		if (!clk_node) {
-			dev_err(dev, "Unable to get %s node\n", node_name);
 			ret = -EINVAL;
+			dev_err_probe(dev, ret, "Unable to get %s node\n", node_name);
 			goto err;
 		}
 
 		ret = wiz_div_clk_register(wiz, clk_node, wiz->div_sel_field[i],
 					   clk_div_sel[i].table);
 		if (ret) {
-			dev_err(dev, "Failed to register %s clock\n",
-				node_name);
+			dev_err_probe(dev, ret, "Failed to register %s clock\n",
+				      node_name);
 			of_node_put(clk_node);
 			goto err;
 		}

-- 
2.39.2

[PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Thomas Richard]

The wiz_clock_init() function mixes probe and hardware configuration.
Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
configuration part in a new function named wiz_clock_init().

This hardware configuration sequence must be called during the resume
stage of the driver.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 65 ++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index ce8a99801a4c..45c5a4e9cd12 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1076,25 +1076,11 @@ static int wiz_clock_register(struct wiz *wiz)
 	return ret;
 }
 
-static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
+static void wiz_clock_init(struct wiz *wiz)
 {
-	const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
-	struct device *dev = wiz->dev;
-	struct device_node *clk_node;
-	const char *node_name;
 	unsigned long rate;
-	struct clk *clk;
-	int ret;
-	int i;
-
-	clk = devm_clk_get(dev, "core_ref_clk");
-	if (IS_ERR(clk))
-		return dev_err_probe(dev, PTR_ERR(clk),
-				     "core_ref_clk clock not found\n");
-
-	wiz->input_clks[WIZ_CORE_REFCLK] = clk;
 
-	rate = clk_get_rate(clk);
+	rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK]);
 	if (rate >= 100000000)
 		regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
 	else
@@ -1119,6 +1105,39 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 		break;
 	}
 
+	if (wiz->input_clks[WIZ_CORE_REFCLK1]) {
+		rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK1]);
+		if (rate >= 100000000)
+			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
+		else
+			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
+
+	}
+
+	rate = clk_get_rate(wiz->input_clks[WIZ_EXT_REFCLK]);
+	if (rate >= 100000000)
+		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
+	else
+		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
+}
+
+static int wiz_clock_probe(struct wiz *wiz, struct device_node *node)
+{
+	const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
+	struct device *dev = wiz->dev;
+	struct device_node *clk_node;
+	const char *node_name;
+	struct clk *clk;
+	int ret;
+	int i;
+
+	clk = devm_clk_get(dev, "core_ref_clk");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "core_ref_clk clock not found\n");
+
+	wiz->input_clks[WIZ_CORE_REFCLK] = clk;
+
 	if (wiz->data->pma_cmn_refclk1_int_mode) {
 		clk = devm_clk_get(dev, "core_ref1_clk");
 		if (IS_ERR(clk))
@@ -1126,12 +1145,6 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 					     "core_ref1_clk clock not found\n");
 
 		wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
-
-		rate = clk_get_rate(clk);
-		if (rate >= 100000000)
-			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
-		else
-			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
 	}
 
 	clk = devm_clk_get(dev, "ext_ref_clk");
@@ -1141,11 +1154,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 
 	wiz->input_clks[WIZ_EXT_REFCLK] = clk;
 
-	rate = clk_get_rate(clk);
-	if (rate >= 100000000)
-		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
-	else
-		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
+	wiz_clock_init(wiz);
 
 	switch (wiz->type) {
 	case AM64_WIZ_10G:
@@ -1589,7 +1598,7 @@ static int wiz_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
-	ret = wiz_clock_init(wiz, node);
+	ret = wiz_clock_probe(wiz, node);
 	if (ret < 0) {
 		dev_warn(dev, "Failed to initialize clocks\n");
 		goto err_get_sync;

-- 
2.39.2

[PATCH v3 09/18] phy: ti: phy-j721e-wiz: add resume support

[Thomas Richard]

Add resume support.
It has been tested on J7200 SR1.0 and SR2.0.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index 45c5a4e9cd12..cde38182e34c 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1660,12 +1660,41 @@ static void wiz_remove(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 }
 
+static int wiz_resume_noirq(struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	struct wiz *wiz = dev_get_drvdata(dev);
+	int ret;
+
+	/* Enable supplemental Control override if available */
+	if (wiz->sup_legacy_clk_override)
+		regmap_field_write(wiz->sup_legacy_clk_override, 1);
+
+	wiz_clock_init(wiz);
+
+	ret = wiz_init(wiz);
+	if (ret) {
+		dev_err(dev, "WIZ initialization failed\n");
+		goto err_wiz_init;
+	}
+
+	return 0;
+
+err_wiz_init:
+	wiz_clock_cleanup(wiz, node);
+
+	return ret;
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(wiz_pm_ops, NULL, wiz_resume_noirq);
+
 static struct platform_driver wiz_driver = {
 	.probe		= wiz_probe,
 	.remove_new	= wiz_remove,
 	.driver		= {
 		.name	= "wiz",
 		.of_match_table = wiz_id_table,
+		.pm	= pm_sleep_ptr(&wiz_pm_ops),
 	},
 };
 module_platform_driver(wiz_driver);

-- 
2.39.2

[PATCH v3 10/18] phy: cadence-torrent: extract calls to clk_get from cdns_torrent_clk

[Thomas Richard]

Extract calls to clk_get from cdns_torrent_clk into a separate function.
It needs to call cdns_torrent_clk at resume without looking up the clock.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index a75c96385c57..803a76acf2fd 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -2681,18 +2681,21 @@ static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
 	return 0;
 }
 
+static int cdns_torrent_of_get_clk(struct cdns_torrent_phy *cdns_phy)
+{
+	cdns_phy->clk = devm_clk_get(cdns_phy->dev, "refclk");
+	if (IS_ERR(cdns_phy->clk))
+		return dev_err_probe(cdns_phy->dev, PTR_ERR(cdns_phy->clk),
+				     "phy ref clock not found\n");
+
+	return 0;
+}
+
 static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
 {
-	struct device *dev = cdns_phy->dev;
 	unsigned long ref_clk_rate;
 	int ret;
 
-	cdns_phy->clk = devm_clk_get(dev, "refclk");
-	if (IS_ERR(cdns_phy->clk)) {
-		dev_err(dev, "phy ref clock not found\n");
-		return PTR_ERR(cdns_phy->clk);
-	}
-
 	ret = clk_prepare_enable(cdns_phy->clk);
 	if (ret) {
 		dev_err(cdns_phy->dev, "Failed to prepare ref clock\n");
@@ -2776,6 +2779,10 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = cdns_torrent_of_get_clk(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
 	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
 
 	if (!already_configured) {

-- 
2.39.2

[PATCH v3 11/18] phy: cadence-torrent: register resets even if the phy is already configured

[Thomas Richard]

Resets are needed during suspend and resume stages.
So they shall be registered during the probe even the phy is already
initialized.

The function cdns_torrent_reset is renamed cdns_torrent_of_get_reset() to
make it clear.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 803a76acf2fd..bba10ca0bfdd 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -2660,7 +2660,7 @@ static int cdns_torrent_clk_register(struct cdns_torrent_phy *cdns_phy)
 	return 0;
 }
 
-static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
+static int cdns_torrent_of_get_reset(struct cdns_torrent_phy *cdns_phy)
 {
 	struct device *dev = cdns_phy->dev;
 
@@ -2779,6 +2779,10 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = cdns_torrent_of_get_reset(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
 	ret = cdns_torrent_of_get_clk(cdns_phy);
 	if (ret)
 		goto clk_cleanup;
@@ -2786,10 +2790,6 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
 
 	if (!already_configured) {
-		ret = cdns_torrent_reset(cdns_phy);
-		if (ret)
-			goto clk_cleanup;
-
 		ret = cdns_torrent_clk(cdns_phy);
 		if (ret)
 			goto clk_cleanup;

-- 
2.39.2

[PATCH v3 12/18] phy: cadence-torrent: add already_configured to struct cdns_torrent_phy

[Thomas Richard]

Add already_configured to struct cdns_torrent_phy, so it can be used at
differents stages.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index bba10ca0bfdd..b35fbc8a60e5 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -358,6 +358,7 @@ struct cdns_torrent_phy {
 	enum cdns_torrent_ref_clk ref_clk_rate;
 	struct cdns_torrent_inst phys[MAX_NUM_LANES];
 	int nsubnodes;
+	int already_configured;
 	const struct cdns_torrent_data *init_data;
 	struct regmap *regmap_common_cdb;
 	struct regmap *regmap_phy_pcs_common_cdb;
@@ -2740,7 +2741,6 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	struct device_node *child;
 	int ret, subnodes, node = 0, i;
 	u32 total_num_lanes = 0;
-	int already_configured;
 	u8 init_dp_regmap = 0;
 	u32 phy_type;
 
@@ -2787,9 +2787,9 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		goto clk_cleanup;
 
-	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
+	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &cdns_phy->already_configured);
 
-	if (!already_configured) {
+	if (!cdns_phy->already_configured) {
 		ret = cdns_torrent_clk(cdns_phy);
 		if (ret)
 			goto clk_cleanup;
@@ -2869,7 +2869,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		of_property_read_u32(child, "cdns,ssc-mode",
 				     &cdns_phy->phys[node].ssc_mode);
 
-		if (!already_configured)
+		if (!cdns_phy->already_configured)
 			gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
 		else
 			gphy = devm_phy_create(dev, child, &noop_ops);
@@ -2955,7 +2955,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		goto put_lnk_rst;
 	}
 
-	if (cdns_phy->nsubnodes > 1 && !already_configured) {
+	if (cdns_phy->nsubnodes > 1 && !cdns_phy->already_configured) {
 		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
 		if (ret)
 			goto put_lnk_rst;

-- 
2.39.2

[PATCH v3 13/18] phy: cadence-torrent: remove noop_ops phy operations

[Thomas Richard]

Even if a PHY is already configured, the PHY operations are needed during
resume stage, as the PHY is in reset state.
The noop_ops PHY operations is removed to always have PHY operations.
The already_configured flag is checked at the begening of init, configure
and poweron operations to keep the already_configured behaviour.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index b35fbc8a60e5..52cadca4c07b 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -1593,6 +1593,9 @@ static int cdns_torrent_dp_configure(struct phy *phy,
 	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(phy->dev.parent);
 	int ret;
 
+	if (cdns_phy->already_configured)
+		return 0;
+
 	ret = cdns_torrent_dp_verify_config(inst, &opts->dp);
 	if (ret) {
 		dev_err(&phy->dev, "invalid params for phy configure\n");
@@ -1628,6 +1631,12 @@ static int cdns_torrent_phy_on(struct phy *phy)
 	u32 read_val;
 	int ret;
 
+	if (cdns_phy->already_configured) {
+		/* Give 5ms to 10ms delay for the PIPE clock to be stable */
+		usleep_range(5000, 10000);
+		return 0;
+	}
+
 	if (cdns_phy->nsubnodes == 1) {
 		/* Take the PHY lane group out of reset */
 		reset_control_deassert(inst->lnk_rst);
@@ -2306,6 +2315,9 @@ static int cdns_torrent_phy_init(struct phy *phy)
 	u32 num_regs;
 	int i, j;
 
+	if (cdns_phy->already_configured)
+		return 0;
+
 	if (cdns_phy->nsubnodes > 1) {
 		if (phy_type == TYPE_DP)
 			return cdns_torrent_dp_multilink_init(cdns_phy, inst, phy);
@@ -2443,19 +2455,6 @@ static const struct phy_ops cdns_torrent_phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
-static int cdns_torrent_noop_phy_on(struct phy *phy)
-{
-	/* Give 5ms to 10ms delay for the PIPE clock to be stable */
-	usleep_range(5000, 10000);
-
-	return 0;
-}
-
-static const struct phy_ops noop_ops = {
-	.power_on	= cdns_torrent_noop_phy_on,
-	.owner		= THIS_MODULE,
-};
-
 static
 int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
 {
@@ -2869,10 +2868,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		of_property_read_u32(child, "cdns,ssc-mode",
 				     &cdns_phy->phys[node].ssc_mode);
 
-		if (!cdns_phy->already_configured)
-			gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
-		else
-			gphy = devm_phy_create(dev, child, &noop_ops);
+		gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
 		if (IS_ERR(gphy)) {
 			ret = PTR_ERR(gphy);
 			goto put_child;

-- 
2.39.2

[PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

Add suspend and resume support.

The already_configured flag is cleared during the suspend stage to force
the PHY initialization during the resume stage.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 52cadca4c07b..f8945a11e7ca 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
 	cdns_torrent_clk_cleanup(cdns_phy);
 }
 
+static int cdns_torrent_phy_suspend_noirq(struct device *dev)
+{
+	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
+	int i;
+
+	reset_control_assert(cdns_phy->phy_rst);
+	reset_control_assert(cdns_phy->apb_rst);
+	for (i = 0; i < cdns_phy->nsubnodes; i++)
+		reset_control_assert(cdns_phy->phys[i].lnk_rst);
+
+	if (cdns_phy->already_configured)
+		cdns_phy->already_configured = 0;
+	else
+		clk_disable_unprepare(cdns_phy->clk);
+
+	return 0;
+}
+
+static int cdns_torrent_phy_resume_noirq(struct device *dev)
+{
+	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
+	int node = cdns_phy->nsubnodes;
+	int ret, i;
+
+	ret = cdns_torrent_clk(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
+	/* Enable APB */
+	reset_control_deassert(cdns_phy->apb_rst);
+
+	if (cdns_phy->nsubnodes > 1) {
+		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
+		if (ret)
+			goto put_lnk_rst;
+	}
+
+	return 0;
+
+put_lnk_rst:
+	for (i = 0; i < node; i++)
+		reset_control_assert(cdns_phy->phys[i].lnk_rst);
+	reset_control_assert(cdns_phy->apb_rst);
+	clk_disable_unprepare(cdns_phy->clk);
+clk_cleanup:
+	cdns_torrent_clk_cleanup(cdns_phy);
+	return ret;
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(cdns_torrent_phy_pm_ops,
+			       cdns_torrent_phy_suspend_noirq,
+			       cdns_torrent_phy_resume_noirq);
+
 /* USB and DP link configuration */
 static struct cdns_reg_pairs usb_dp_link_cmn_regs[] = {
 	{0x0002, PHY_PLL_CFG},
@@ -4576,6 +4629,7 @@ static struct platform_driver cdns_torrent_phy_driver = {
 	.driver = {
 		.name	= "cdns-torrent-phy",
 		.of_match_table	= cdns_torrent_phy_of_match,
+		.pm	= pm_sleep_ptr(&cdns_torrent_phy_pm_ops),
 	}
 };
 module_platform_driver(cdns_torrent_phy_driver);

-- 
2.39.2

[PATCH v3 15/18] PCI: cadence: extract link setup sequence from cdns_pcie_host_setup()

[Thomas Richard]

The function cdns_pcie_host_setup() mixes probe structure and link setup.

The link setup must be done during the resume sequence. So extract it from
cdns_pcie_host_setup() and create a dedicated function.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 39 ++++++++++++++--------
 drivers/pci/controller/cadence/pcie-cadence.h      |  6 ++++
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 5b14f7ee3c79..93d9922730af 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
 	return cdns_pcie_host_init_address_translation(rc);
 }
 
+int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	struct device *dev = rc->pcie.dev;
+	int ret;
+
+	if (rc->quirk_detect_quiet_flag)
+		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
+
+	cdns_pcie_host_enable_ptm_response(pcie);
+
+	ret = cdns_pcie_start_link(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to start link\n");
+		return ret;
+	}
+
+	ret = cdns_pcie_host_start_link(rc);
+	if (ret)
+		dev_dbg(dev, "PCIe link never came up\n");
+
+	return 0;
+}
+
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	struct device *dev = rc->pcie.dev;
@@ -533,20 +557,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 		return PTR_ERR(rc->cfg_base);
 	rc->cfg_res = res;
 
-	if (rc->quirk_detect_quiet_flag)
-		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
-
-	cdns_pcie_host_enable_ptm_response(pcie);
-
-	ret = cdns_pcie_start_link(pcie);
-	if (ret) {
-		dev_err(dev, "Failed to start link\n");
-		return ret;
-	}
-
-	ret = cdns_pcie_host_start_link(rc);
+	ret = cdns_pcie_host_link_setup(rc);
 	if (ret)
-		dev_dbg(dev, "PCIe link never came up\n");
+		return ret;
 
 	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
 		rc->avail_ib_bar[bar] = true;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 373cb50fcd15..4c687aeb810e 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -515,10 +515,16 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 }
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
+int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
 #else
+static inline int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
+{
+	return 0;
+}
+
 static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	return 0;

-- 
2.39.2

[PATCH v3 16/18] PCI: cadence: set cdns_pcie_host_init() global

[Thomas Richard]

During the resume sequence of the host, cdns_pcie_host_init() needs to be
called, so set it global.

The dev function parameter is removed, as it isn't used.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 5 ++---
 drivers/pci/controller/cadence/pcie-cadence.h      | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 93d9922730af..8af95e9da7ce 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -485,8 +485,7 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 	return cdns_pcie_host_map_dma_ranges(rc);
 }
 
-static int cdns_pcie_host_init(struct device *dev,
-			       struct cdns_pcie_rc *rc)
+int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
 {
 	int err;
 
@@ -564,7 +563,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
 		rc->avail_ib_bar[bar] = true;
 
-	ret = cdns_pcie_host_init(dev, rc);
+	ret = cdns_pcie_host_init(rc);
 	if (ret)
 		return ret;
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 4c687aeb810e..d55dfd173f22 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -516,6 +516,7 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
 int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
+int cdns_pcie_host_init(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
@@ -525,6 +526,11 @@ static inline int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
 	return 0;
 }
 
+static inline int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
+{
+	return 0;
+}
+
 static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	return 0;

-- 
2.39.2

[PATCH v3 17/18] PCI: j721e: add reset GPIO to struct j721e_pcie

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

Add reset GPIO to struct j721e_pcie, so it can be used at suspend and
resume stages.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 2c87e7728a65..9c588e79f5ac 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -54,6 +54,7 @@ struct j721e_pcie {
 	struct clk		*refclk;
 	u32			mode;
 	u32			num_lanes;
+	struct gpio_desc	*reset_gpio;
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
@@ -359,7 +360,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	struct j721e_pcie *pcie;
 	struct cdns_pcie_rc *rc = NULL;
 	struct cdns_pcie_ep *ep = NULL;
-	struct gpio_desc *gpiod;
 	void __iomem *base;
 	struct clk *clk;
 	u32 num_lanes;
@@ -468,9 +468,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	switch (mode) {
 	case PCI_MODE_RC:
-		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-		if (IS_ERR(gpiod)) {
-			ret = PTR_ERR(gpiod);
+		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+		if (IS_ERR(pcie->reset_gpio)) {
+			ret = PTR_ERR(pcie->reset_gpio);
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "Failed to get reset GPIO\n");
 			goto err_get_sync;
@@ -504,9 +504,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 		 * mode is selected while enabling the PHY. So deassert PERST#
 		 * after 100 us.
 		 */
-		if (gpiod) {
+		if (pcie->reset_gpio) {
 			usleep_range(100, 200);
-			gpiod_set_value_cansleep(gpiod, 1);
+			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
 		}
 
 		ret = cdns_pcie_host_setup(rc);

-- 
2.39.2

[PATCH v3 18/18] PCI: j721e: add suspend and resume support

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

Add suspend and resume support. Only the rc mode is supported.

During the suspend stage PERST# is asserted, then deasserted during the
resume stage.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 90 ++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 9c588e79f5ac..cfa7ba237e1a 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -6,6 +6,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
+#include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -18,10 +19,13 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/container_of.h>
 
 #include "../../pci.h"
 #include "pcie-cadence.h"
 
+#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
+
 #define ENABLE_REG_SYS_2	0x108
 #define STATUS_REG_SYS_2	0x508
 #define STATUS_CLR_REG_SYS_2	0x708
@@ -554,6 +558,91 @@ static void j721e_pcie_remove(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 }
 
+static int j721e_pcie_suspend_noirq(struct device *dev)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(dev);
+
+	if (pcie->mode == PCI_MODE_RC) {
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+		clk_disable_unprepare(pcie->refclk);
+	}
+
+	cdns_pcie_disable_phy(pcie->cdns_pcie);
+
+	return 0;
+}
+
+static int j721e_pcie_resume_noirq(struct device *dev)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(dev);
+	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
+	int ret;
+
+	ret = j721e_pcie_ctrl_init(pcie);
+	if (ret < 0) {
+		dev_err(dev, "j721e_pcie_ctrl_init failed\n");
+		return ret;
+	}
+
+	j721e_pcie_config_link_irq(pcie);
+
+	/*
+	 * This is not called explicitly in the probe, it is called by
+	 * cdns_pcie_init_phy.
+	 */
+	ret = cdns_pcie_enable_phy(pcie->cdns_pcie);
+	if (ret < 0) {
+		dev_err(dev, "cdns_pcie_enable_phy failed\n");
+		return -ENODEV;
+	}
+
+	if (pcie->mode == PCI_MODE_RC) {
+		struct cdns_pcie_rc *rc = cdns_pcie_to_rc(cdns_pcie);
+
+		ret = clk_prepare_enable(pcie->refclk);
+		if (ret < 0) {
+			dev_err(dev, "clk_prepare_enable failed\n");
+			return -ENODEV;
+		}
+
+		/*
+		 * "Power Sequencing and Reset Signal Timings" table in
+		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
+		 * indicates PERST# should be deasserted after minimum of 100us
+		 * once REFCLK is stable. The REFCLK to the connector in RC
+		 * mode is selected while enabling the PHY. So deassert PERST#
+		 * after 100 us.
+		 */
+		if (pcie->reset_gpio) {
+			usleep_range(100, 200);
+			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+		}
+
+		ret = cdns_pcie_host_link_setup(rc);
+		if (ret < 0) {
+			clk_disable_unprepare(pcie->refclk);
+			return ret;
+		}
+
+		/*
+		 * Reset internal status of BARs to force reinitialization in
+		 * cdns_pcie_host_init().
+		 */
+		for (enum cdns_pcie_rp_bar bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
+			rc->avail_ib_bar[bar] = true;
+
+		ret = cdns_pcie_host_init(rc);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(j721e_pcie_pm_ops,
+			       j721e_pcie_suspend_noirq,
+			       j721e_pcie_resume_noirq);
+
 static struct platform_driver j721e_pcie_driver = {
 	.probe  = j721e_pcie_probe,
 	.remove_new = j721e_pcie_remove,
@@ -561,6 +650,7 @@ static struct platform_driver j721e_pcie_driver = {
 		.name	= "j721e-pcie",
 		.of_match_table = of_j721e_pcie_match,
 		.suppress_bind_attrs = true,
+		.pm	= pm_sleep_ptr(&j721e_pcie_pm_ops),
 	},
 };
 builtin_platform_driver(j721e_pcie_driver);

-- 
2.39.2

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
> No need to check the pointer returned by platform_get_drvdata(), as
> platform_set_drvdata() is called during the probe.

This patch should go _after_ the next one, otherwise the commit message doesn't
tell full story and the code change bring a potential regression.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/18] mux: add mux_chip_resume() function

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote:
> The mux_chip_resume() function restores a mux_chip using the cached state
> of each mux.

...

> +int mux_chip_resume(struct mux_chip *mux_chip)
> +{
> +	int global_ret = 0;
> +	int ret, i;
> +
> +	for (i = 0; i < mux_chip->controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		if (mux->cached_state == MUX_CACHE_UNKNOWN)
> +			continue;
> +
> +		ret = mux_control_set(mux, mux->cached_state);
> +		if (ret < 0) {
> +			dev_err(&mux_chip->dev, "unable to restore state\n");
> +			if (!global_ret)
> +				global_ret = ret;

Hmm... This will record the first error and continue.

> +		}
> +	}
> +	return global_ret;

So here, we actually will get stale data in case there are > 1 failures.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/18] i2c: omap: wakeup the controller during suspend() callback

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:49PM +0100, Thomas Richard wrote:
> A device may need the controller up during suspend_noirq() or
> resume_noirq().
> But if the controller is autosuspended, there is no way to wakeup it during
> suspend_noirq() or resume_noirq() because runtime pm is disabled at this
> time.
> 
> The suspend() callback wakes up the controller, so it is available until
> its suspend_noirq() callback (pm_runtime_force_suspend()).
> During the resume, it's restored by resume_noirq() callback
> (pm_runtime_force_resume()). Then resume() callback enables autosuspend.
> 
> So the controller is up during a little time slot in suspend and resume
> sequences even if it's not used.

...

> +	/*
> +	 * If the controller is autosuspended, there is no way to wakeup it once
> +	 * runtime pm is disabled (in suspend_late()).
> +	 * But a device may need the controller up during suspend_noirq() or
> +	 * resume_noirq().
> +	 * Wakeup the controller while runtime pm is enabled, so it is available until
> +	 * its suspend_noirq(), and from resume_noirq().
> +	 */

It's a bit random line lengths...
Can you repack this to be more condensed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/18] phy: ti: phy-j721e-wiz: use dev_err_probe() instead of dev_err()

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:52PM +0100, Thomas Richard wrote:
> Use dev_err_probe() instead of dev_err() in wiz_clock_init() to simplify
> the code and standardize the error output.

...

>  		ret = wiz_clock_register(wiz);
>  		if (ret)
> -			dev_err(dev, "Failed to register wiz clocks\n");
> +			dev_err_probe(dev, ret, "Failed to register wiz clocks\n");
>  		return ret;

Maybe

		if (ret)
			return dev_err_probe(dev, ret, "Failed to register wiz clocks\n");

		return 0;

?

...

>  		if (!clk_node) {
> -			dev_err(dev, "Unable to get %s node\n", node_name);
>  			ret = -EINVAL;
> +			dev_err_probe(dev, ret, "Unable to get %s node\n", node_name);
>  			goto err;

			ret = dev_err_probe(..., -EINVAL, ...);

>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/18] Add suspend to ram support for PCIe on J7200

[Philipp Stanner]

On Thu, 2024-02-15 at 16:17 +0100, Thomas Richard wrote:
> This add suspend to ram support for the PCIe (RC mode) on J7200
> platform.
> 
> In RC mode, the reset pin for endpoints is managed by a gpio expander
> on a
> i2c bus. This pin shall be managed in suspend_noirq() and
> resume_noirq().
> The suspend/resume has been moved to suspend_noirq()/resume_noirq()
> for
> pca953x (expander) and pinctrl-single.
> 
> To do i2c accesses during suspend_noirq/resume_noirq, we need to
> force the
> wakeup of the i2c controller (which is autosuspended) during suspend
> callback. 
> It's the only way to wakeup the controller if it's autosuspended, as
> runtime pm is disabled in suspend_noirq and resume_noirq.
> 
> The main change in this v3 is the removal of the probe boolean for
> the
> functions wiz_clock_probe() and cdns_pcie_host_setup().
> Their contents were split in multiple functions which are called in
> the
> resume_noirq() callbacks.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
> Changes in v3:
> - pinctrl-single: split patch in two parts, a first patch to remove
> the
>   dead code, a second to move suspend()/resume() callbacks to noirq.
> - i2c-omap: add a comments above the suspend_noirq() callback.
> - mux: now mux_chip_resume() try to restores all muxes, then return 0
> if
>   all is ok or the first failure.
> - mmio: fix commit message.
> - phy-j721e-wiz: add a patch to use dev_err_probe() instead of
> dev_err() in
>   the wiz_clock_init() function.
> - phy-j721e-wiz: remove probe boolean for the wiz_clock_init(),
> rename the
>   function to wiz_clock_probe(), extract hardware configuration part
> in a
>   new function wiz_clock_init().
> - phy-cadence-torrent: use dev_err_probe() and fix commit messages
> - pcie-cadence-host: remove probe boolean for the
> cdns_pcie_host_setup()
>   function and extract the link setup part in a new function
>   cdns_pcie_host_link_setup().
> - pcie-cadence-host: make cdns_pcie_host_init() global.
> - pci-j721e: use the cdns_pcie_host_link_setup()
> cdns_pcie_host_init()
>   functions in the resume_noirq() callback.
> - Link to v2:
> https://lore.kernel.org/r/20240102-j7200-pcie-s2r-v2-0-8e4f7d228ec2@bootlin.com
> 
> Changes in v2:
> - all: fix commits messages.
> - all: use DEFINE_NOIRQ_DEV_PM_OPS and pm_sleep_ptr macros.
> - all: remove useless #ifdef CONFIG_PM.
> - pinctrl-single: drop dead code
> - mux: add mux_chip_resume() function in mux core.
> - mmio: resume sequence is now a call to mux_chip_resume().
> - phy-cadence-torrent: fix typo in resume sequence
> (reset_control_assert()
>   instead of reset_control_put()).
> - phy-cadence-torrent: use PHY instead of phy.
> - pci-j721e: do not shadow cdns_pcie_host_setup return code in resume
>   sequence.
> - pci-j721e: drop dead code.
> - Link to v1:
> https://lore.kernel.org/r/20240102-j7200-pcie-s2r-v1-0-84e55da52400@bootlin.com
> 
> ---
> Thomas Richard (15):
>       gpio: pca953x: move suspend()/resume() to
> suspend_noirq()/resume_noirq()
>       pinctrl: pinctrl-single: remove dead code in suspend() and
> resume() callbacks
>       pinctrl: pinctrl-single: move suspend()/resume() callbacks to
> noirq
>       i2c: omap: wakeup the controller during suspend() callback
>       mux: add mux_chip_resume() function
>       phy: ti: phy-j721e-wiz: use dev_err_probe() instead of
> dev_err()
>       phy: ti: phy-j721e-wiz: split wiz_clock_init() function
>       phy: ti: phy-j721e-wiz: add resume support
>       phy: cadence-torrent: extract calls to clk_get from
> cdns_torrent_clk
>       phy: cadence-torrent: register resets even if the phy is
> already configured
>       phy: cadence-torrent: add already_configured to struct
> cdns_torrent_phy
>       phy: cadence-torrent: remove noop_ops phy operations
>       phy: cadence-torrent: add suspend and resume support
>       PCI: cadence: extract link setup sequence from
> cdns_pcie_host_setup()
>       PCI: cadence: set cdns_pcie_host_init() global
> 
> Théo Lebrun (3):
>       mux: mmio: add resume support
>       PCI: j721e: add reset GPIO to struct j721e_pcie
>       PCI: j721e: add suspend and resume support


For the PCI patches Bjorn is most likely going to ask you to adjust
them to PCI's common commit style; see here [1]

In particular, PCI (afaik) has no convention for naming subcomponents
such as j721e and the info following the : is written beginning with an
upper case, e.g.

PCI: Add suspend and resume support for j721e


Regards,
P.

[1] https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/


> 
>  drivers/gpio/gpio-pca953x.c                        |   7 +-
>  drivers/i2c/busses/i2c-omap.c                      |  22 ++++
>  drivers/mux/core.c                                 |  30 +++++
>  drivers/mux/mmio.c                                 |  12 ++
>  drivers/pci/controller/cadence/pci-j721e.c         | 102
> ++++++++++++++++-
>  drivers/pci/controller/cadence/pcie-cadence-host.c |  44 +++++---
>  drivers/pci/controller/cadence/pcie-cadence.h      |  12 ++
>  drivers/phy/cadence/phy-cadence-torrent.c          | 121
> +++++++++++++++------
>  drivers/phy/ti/phy-j721e-wiz.c                     | 119
> +++++++++++++-------
>  drivers/pinctrl/pinctrl-single.c                   |  28 ++---
>  include/linux/mux/driver.h                         |   1 +
>  11 files changed, 380 insertions(+), 118 deletions(-)
> ---
> base-commit: 00ff0f9ce40db8e64fe16c424a965fd7ab769c42
> change-id: 20240102-j7200-pcie-s2r-ecb1a979e357
> 
> Best regards,

Re: [PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> The wiz_clock_init() function mixes probe and hardware configuration.
> Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> configuration part in a new function named wiz_clock_init().
> 
> This hardware configuration sequence must be called during the resume
> stage of the driver.

...

(Side note, as this can be done later)

>  	if (rate >= 100000000)

> +		if (rate >= 100000000)

> +	if (rate >= 100000000)

I would make local definition and use it, we may get the global one as there
are users.

#define HZ_PER_GHZ	1000000000UL

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 13/18] phy: cadence-torrent: remove noop_ops phy operations

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:58PM +0100, Thomas Richard wrote:
> Even if a PHY is already configured, the PHY operations are needed during
> resume stage, as the PHY is in reset state.
> The noop_ops PHY operations is removed to always have PHY operations.
> The already_configured flag is checked at the begening of init, configure
> and poweron operations to keep the already_configured behaviour.

...

> +	if (cdns_phy->already_configured) {
> +		/* Give 5ms to 10ms delay for the PIPE clock to be stable */
> +		usleep_range(5000, 10000);

fsleep() ?
(Yes, I see this is the original code, perhaps later in a separate change)

> +		return 0;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:17:59PM +0100, Thomas Richard wrote:
> Add suspend and resume support.
> 
> The already_configured flag is cleared during the suspend stage to force
> the PHY initialization during the resume stage.

> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

SoB/Co-developed-by ?

...

> +static int cdns_torrent_phy_suspend_noirq(struct device *dev)
> +{
> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> +	int i;

Why signed?

> +	reset_control_assert(cdns_phy->phy_rst);
> +	reset_control_assert(cdns_phy->apb_rst);
> +	for (i = 0; i < cdns_phy->nsubnodes; i++)
> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> +
> +	if (cdns_phy->already_configured)
> +		cdns_phy->already_configured = 0;
> +	else
> +		clk_disable_unprepare(cdns_phy->clk);
> +
> +	return 0;
> +}
> +
> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
> +{
> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> +	int node = cdns_phy->nsubnodes;
> +	int ret, i;

Ditto.

> +	ret = cdns_torrent_clk(cdns_phy);
> +	if (ret)
> +		goto clk_cleanup;
> +
> +	/* Enable APB */
> +	reset_control_deassert(cdns_phy->apb_rst);
> +
> +	if (cdns_phy->nsubnodes > 1) {
> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
> +		if (ret)
> +			goto put_lnk_rst;
> +	}
> +
> +	return 0;
> +
> +put_lnk_rst:
> +	for (i = 0; i < node; i++)
> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> +	reset_control_assert(cdns_phy->apb_rst);
> +	clk_disable_unprepare(cdns_phy->clk);
> +clk_cleanup:
> +	cdns_torrent_clk_cleanup(cdns_phy);
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/18] PCI: j721e: add suspend and resume support

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:18:03PM +0100, Thomas Richard wrote:
> From: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Add suspend and resume support. Only the rc mode is supported.
> 
> During the suspend stage PERST# is asserted, then deasserted during the
> resume stage.

...

> +#include <linux/clk-provider.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> @@ -18,10 +19,13 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>

> +#include <linux/container_of.h>

Unordered.

...

> +	ret = j721e_pcie_ctrl_init(pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "j721e_pcie_ctrl_init failed\n");

Is there any guarantee this won't spam logs?

> +		return ret;
> +	}

...

> +	/*
> +	 * This is not called explicitly in the probe, it is called by
> +	 * cdns_pcie_init_phy.

cdns_pcie_init_phy()

> +	 */
> +	ret = cdns_pcie_enable_phy(pcie->cdns_pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "cdns_pcie_enable_phy failed\n");
> +		return -ENODEV;

A potential log spammer?

> +	}

> +	if (pcie->mode == PCI_MODE_RC) {
> +		struct cdns_pcie_rc *rc = cdns_pcie_to_rc(cdns_pcie);
> +
> +		ret = clk_prepare_enable(pcie->refclk);
> +		if (ret < 0) {
> +			dev_err(dev, "clk_prepare_enable failed\n");

Ditto.

> +			return -ENODEV;

Why is the error code shadowed?

> +		}

...

> +		if (pcie->reset_gpio) {
> +			usleep_range(100, 200);

fsleep()

> +			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> +		}

> +		ret = cdns_pcie_host_link_setup(rc);
> +		if (ret < 0) {
> +			clk_disable_unprepare(pcie->refclk);
> +			return ret;
> +		}
> +
> +		/*
> +		 * Reset internal status of BARs to force reinitialization in
> +		 * cdns_pcie_host_init().
> +		 */
> +		for (enum cdns_pcie_rp_bar bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
> +			rc->avail_ib_bar[bar] = true;
> +
> +		ret = cdns_pcie_host_init(rc);
> +		if (ret)

No clock disabling?

> +			return ret;
> +	}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 17/18] PCI: j721e: add reset GPIO to struct j721e_pcie

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 04:18:02PM +0100, Thomas Richard wrote:
> From: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Add reset GPIO to struct j721e_pcie, so it can be used at suspend and
> resume stages.

...

>  	case PCI_MODE_RC:
> -		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> -		if (IS_ERR(gpiod)) {
> -			ret = PTR_ERR(gpiod);
> +		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +		if (IS_ERR(pcie->reset_gpio)) {
> +			ret = PTR_ERR(pcie->reset_gpio);
>  			if (ret != -EPROBE_DEFER)
>  				dev_err(dev, "Failed to get reset GPIO\n");
>  			goto err_get_sync;
> @@ -504,9 +504,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  		 * mode is selected while enabling the PHY. So deassert PERST#
>  		 * after 100 us.
>  		 */
> -		if (gpiod) {
> +		if (pcie->reset_gpio) {
>  			usleep_range(100, 200);
> -			gpiod_set_value_cansleep(gpiod, 1);
> +			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
>  		}

Instead of all this, just add one line assignment. Moreover, before or after
this patch refactor the code to use ret = dev_err_probe(...); pattern that
eliminates those deferral probe checks.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/18] Add suspend to ram support for PCIe on J7200

[Bjorn Helgaas]

On Thu, Feb 15, 2024 at 04:17:45PM +0100, Thomas Richard wrote:
> This add suspend to ram support for the PCIe (RC mode) on J7200 platform.

>       PCI: cadence: extract link setup sequence from cdns_pcie_host_setup()
>       PCI: cadence: set cdns_pcie_host_init() global
>       PCI: j721e: add reset GPIO to struct j721e_pcie
>       PCI: j721e: add suspend and resume support

The drivers/pci/ subject line pattern is:

  PCI: <driver>: <Capitalized verb>

e.g.,

  PCI: cadence: Extract link setup sequence from cdns_pcie_host_setup()

Re: [PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Vinod Koul]

On 15-02-24, 17:43, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> > The wiz_clock_init() function mixes probe and hardware configuration.
> > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> > configuration part in a new function named wiz_clock_init().
> > 
> > This hardware configuration sequence must be called during the resume
> > stage of the driver.
> 
> ...
> 
> (Side note, as this can be done later)
> 
> >  	if (rate >= 100000000)
> 
> > +		if (rate >= 100000000)
> 
> > +	if (rate >= 100000000)
> 
> I would make local definition and use it, we may get the global one as there
> are users.
> 
> #define HZ_PER_GHZ	1000000000UL

Better to define as:
#define HZ_PER_GHZ 1 * GIGA

-- 
~Vinod

Re: [PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

On 2/15/24 16:46, Andy Shevchenko wrote:
>> +static int cdns_torrent_phy_suspend_noirq(struct device *dev)
>> +{
>> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
>> +	int i;
> 
> Why signed?

In the for loop below, the i variable is compared to
cdns_phy->nsubnodes, which is an int.

https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-torrent.c#L360

> 
>> +	reset_control_assert(cdns_phy->phy_rst);
>> +	reset_control_assert(cdns_phy->apb_rst);
>> +	for (i = 0; i < cdns_phy->nsubnodes; i++)
>> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
>> +
>> +	if (cdns_phy->already_configured)
>> +		cdns_phy->already_configured = 0;
>> +	else
>> +		clk_disable_unprepare(cdns_phy->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
>> +{
>> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
>> +	int node = cdns_phy->nsubnodes;
>> +	int ret, i;
> 
> Ditto>

Same reason

>> +	ret = cdns_torrent_clk(cdns_phy);
>> +	if (ret)
>> +		goto clk_cleanup;
>> +
>> +	/* Enable APB */
>> +	reset_control_deassert(cdns_phy->apb_rst);
>> +
>> +	if (cdns_phy->nsubnodes > 1) {
>> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
>> +		if (ret)
>> +			goto put_lnk_rst;
>> +	}
>> +
>> +	return 0;
>> +
>> +put_lnk_rst:
>> +	for (i = 0; i < node; i++)
>> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
>> +	reset_control_assert(cdns_phy->apb_rst);
>> +	clk_disable_unprepare(cdns_phy->clk);
>> +clk_cleanup:
>> +	cdns_torrent_clk_cleanup(cdns_phy);
>> +	return ret;
>> +}
> 
-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 05/18] mux: add mux_chip_resume() function

[Thomas Richard]

On 2/15/24 16:29, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote:
>> The mux_chip_resume() function restores a mux_chip using the cached state
>> of each mux.
> 
> ...
> 
>> +int mux_chip_resume(struct mux_chip *mux_chip)
>> +{
>> +	int global_ret = 0;
>> +	int ret, i;
>> +
>> +	for (i = 0; i < mux_chip->controllers; ++i) {
>> +		struct mux_control *mux = &mux_chip->mux[i];
>> +
>> +		if (mux->cached_state == MUX_CACHE_UNKNOWN)
>> +			continue;
>> +
>> +		ret = mux_control_set(mux, mux->cached_state);
>> +		if (ret < 0) {
>> +			dev_err(&mux_chip->dev, "unable to restore state\n");
>> +			if (!global_ret)
>> +				global_ret = ret;
> 
> Hmm... This will record the first error and continue.

In the v2 we talked about this with Peter Rosin.

In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was
done in the mmio driver) I had the same behavior: try to restore all
muxes and in case of error restore the first one.

I don't know what is the right solution. I just restored the behavior I
had in v1.

> 
>> +		}
>> +	}
>> +	return global_ret;
> 
> So here, we actually will get stale data in case there are > 1 failures.

Yes, indeed. But we will have an error message for each failure.

> 
>> +}
> 
-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Thomas Richard]

On 2/15/24 16:27, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
>> No need to check the pointer returned by platform_get_drvdata(), as
>> platform_set_drvdata() is called during the probe.
> 
> This patch should go _after_ the next one, otherwise the commit message doesn't
> tell full story and the code change bring a potential regression.
> 

Hello Andy,

I'm ok to move this patch after the next one.
But for my understanding, could you explain me why changing the order is
important in this case ?

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Siddharth Vadapalli]

On 24/02/16 11:32AM, Vinod Koul wrote:
> On 15-02-24, 17:43, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> > > The wiz_clock_init() function mixes probe and hardware configuration.
> > > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> > > configuration part in a new function named wiz_clock_init().
> > > 
> > > This hardware configuration sequence must be called during the resume
> > > stage of the driver.
> > 
> > ...
> > 
> > (Side note, as this can be done later)
> > 
> > >  	if (rate >= 100000000)
> > 
> > > +		if (rate >= 100000000)
> > 
> > > +	if (rate >= 100000000)
> > 
> > I would make local definition and use it, we may get the global one as there
> > are users.
> > 
> > #define HZ_PER_GHZ	1000000000UL
> 
> Better to define as:
> #define HZ_PER_GHZ 1 * GIGA

The variable "rate" is being compared against 100 MHz and not 1 GHz.
The driver already has the following macros defined:
#define REF_CLK_19_2MHZ         19200000
#define REF_CLK_25MHZ           25000000
#define REF_CLK_100MHZ          100000000
#define REF_CLK_156_25MHZ       156250000

So would it be acceptable to change it to:
	if (rate >= REF_CLK_100MHZ)
instead?

Regards,
Siddharth.

Re: [PATCH v3 15/18] PCI: cadence: extract link setup sequence from cdns_pcie_host_setup()

[Siddharth Vadapalli]

On 24/02/15 04:18PM, Thomas Richard wrote:
> The function cdns_pcie_host_setup() mixes probe structure and link setup.
> 
> The link setup must be done during the resume sequence. So extract it from
> cdns_pcie_host_setup() and create a dedicated function.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

LGTM.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.

> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 39 ++++++++++++++--------
>  drivers/pci/controller/cadence/pcie-cadence.h      |  6 ++++
>  2 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 5b14f7ee3c79..93d9922730af 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
>  	return cdns_pcie_host_init_address_translation(rc);
>  }
>  
> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
> +{
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	struct device *dev = rc->pcie.dev;
> +	int ret;
> +
> +	if (rc->quirk_detect_quiet_flag)
> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
> +
> +	cdns_pcie_host_enable_ptm_response(pcie);
> +
> +	ret = cdns_pcie_start_link(pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
> +	ret = cdns_pcie_host_start_link(rc);
> +	if (ret)
> +		dev_dbg(dev, "PCIe link never came up\n");
> +
> +	return 0;
> +}
> +
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	struct device *dev = rc->pcie.dev;
> @@ -533,20 +557,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  		return PTR_ERR(rc->cfg_base);
>  	rc->cfg_res = res;
>  
> -	if (rc->quirk_detect_quiet_flag)
> -		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
> -
> -	cdns_pcie_host_enable_ptm_response(pcie);
> -
> -	ret = cdns_pcie_start_link(pcie);
> -	if (ret) {
> -		dev_err(dev, "Failed to start link\n");
> -		return ret;
> -	}
> -
> -	ret = cdns_pcie_host_start_link(rc);
> +	ret = cdns_pcie_host_link_setup(rc);
>  	if (ret)
> -		dev_dbg(dev, "PCIe link never came up\n");
> +		return ret;
>  
>  	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
>  		rc->avail_ib_bar[bar] = true;
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 373cb50fcd15..4c687aeb810e 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -515,10 +515,16 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
>  }
>  
>  #ifdef CONFIG_PCIE_CADENCE_HOST
> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where);
>  #else
> +static inline int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
> +{
> +	return 0;
> +}
> +
>  static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	return 0;
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 16/18] PCI: cadence: set cdns_pcie_host_init() global

[Siddharth Vadapalli]

On 24/02/15 04:18PM, Thomas Richard wrote:
> During the resume sequence of the host, cdns_pcie_host_init() needs to be
> called, so set it global.
> 
> The dev function parameter is removed, as it isn't used.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.

> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 5 ++---
>  drivers/pci/controller/cadence/pcie-cadence.h      | 6 ++++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 93d9922730af..8af95e9da7ce 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -485,8 +485,7 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>  	return cdns_pcie_host_map_dma_ranges(rc);
>  }
>  
> -static int cdns_pcie_host_init(struct device *dev,
> -			       struct cdns_pcie_rc *rc)
> +int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
>  {
>  	int err;
>  
> @@ -564,7 +563,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
>  		rc->avail_ib_bar[bar] = true;
>  
> -	ret = cdns_pcie_host_init(dev, rc);
> +	ret = cdns_pcie_host_init(rc);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 4c687aeb810e..d55dfd173f22 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -516,6 +516,7 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
>  
>  #ifdef CONFIG_PCIE_CADENCE_HOST
>  int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
> +int cdns_pcie_host_init(struct cdns_pcie_rc *rc);
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
>  void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where);
> @@ -525,6 +526,11 @@ static inline int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
>  	return 0;
>  }
>  
> +static inline int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
> +{
> +	return 0;
> +}
> +
>  static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	return 0;
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 18/18] PCI: j721e: add suspend and resume support

[Siddharth Vadapalli]

On 24/02/15 04:18PM, Thomas Richard wrote:
> From: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Add suspend and resume support. Only the rc mode is supported.
> 
> During the suspend stage PERST# is asserted, then deasserted during the
> resume stage.

Wouldn't this imply that the Endpoint device will be reset and therefore
lose context? Or is it expected that the driver corresponding to the
Endpoint Function in Linux will restore the state on resume, post reset?

Regards,
Siddharth.

Re: [PATCH v3 18/18] PCI: j721e: add suspend and resume support

[Théo Lebrun]

Hello,

On Fri Feb 16, 2024 at 11:48 AM CET, Siddharth Vadapalli wrote:
> On 24/02/15 04:18PM, Thomas Richard wrote:
> > From: Théo Lebrun <theo.lebrun@bootlin.com>
> > 
> > Add suspend and resume support. Only the rc mode is supported.
> > 
> > During the suspend stage PERST# is asserted, then deasserted during the
> > resume stage.
>
> Wouldn't this imply that the Endpoint device will be reset and therefore
> lose context? Or is it expected that the driver corresponding to the
> Endpoint Function in Linux will restore the state on resume, post reset?

This does imply exactly that. Endpoint driver must be able to restore
context anyway, as system-wide suspend could mean lost power to PCI RC
controller (eg suspend-to-RAM) or PCI rails (dependent on hardware).

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 18/18] PCI: j721e: add suspend and resume support

[Siddharth Vadapalli]

On 24/02/16 12:09PM, Théo Lebrun wrote:
> Hello,
> 
> On Fri Feb 16, 2024 at 11:48 AM CET, Siddharth Vadapalli wrote:
> > On 24/02/15 04:18PM, Thomas Richard wrote:
> > > From: Théo Lebrun <theo.lebrun@bootlin.com>
> > > 
> > > Add suspend and resume support. Only the rc mode is supported.
> > > 
> > > During the suspend stage PERST# is asserted, then deasserted during the
> > > resume stage.
> >
> > Wouldn't this imply that the Endpoint device will be reset and therefore
> > lose context? Or is it expected that the driver corresponding to the
> > Endpoint Function in Linux will restore the state on resume, post reset?
> 
> This does imply exactly that. Endpoint driver must be able to restore
> context anyway, as system-wide suspend could mean lost power to PCI RC
> controller (eg suspend-to-RAM) or PCI rails (dependent on hardware).

Thank you for confirming.

Regards,
Siddharth.

Re: [PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 11:32:31AM +0530, Vinod Koul wrote:
> On 15-02-24, 17:43, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:

...

> > (Side note, as this can be done later)
> > 
> > >  	if (rate >= 100000000)
> > 
> > > +		if (rate >= 100000000)
> > 
> > > +	if (rate >= 100000000)
> > 
> > I would make local definition and use it, we may get the global one as there
> > are users.
> > 
> > #define HZ_PER_GHZ	1000000000UL
> 
> Better to define as:
> #define HZ_PER_GHZ 1 * GIGA

(with parentheses)

Maybe here, but when it appears in units.h it will be defined as I wrote
to be aligned with the rest of definitions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 02:34:39PM +0530, Siddharth Vadapalli wrote:
> On 24/02/16 11:32AM, Vinod Koul wrote:
> > On 15-02-24, 17:43, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:

...

> > > (Side note, as this can be done later)
> > > 
> > > >  	if (rate >= 100000000)
> > > 
> > > > +		if (rate >= 100000000)
> > > 
> > > > +	if (rate >= 100000000)
> > > 
> > > I would make local definition and use it, we may get the global one as there
> > > are users.
> > > 
> > > #define HZ_PER_GHZ	1000000000UL
> > 
> > Better to define as:
> > #define HZ_PER_GHZ 1 * GIGA
> 
> The variable "rate" is being compared against 100 MHz and not 1 GHz.

Extremely good point why constant definitions are better (to avoid missing
or extra 0, etc)!

> The driver already has the following macros defined:
> #define REF_CLK_19_2MHZ         19200000
> #define REF_CLK_25MHZ           25000000
> #define REF_CLK_100MHZ          100000000
> #define REF_CLK_156_25MHZ       156250000
> 
> So would it be acceptable to change it to:
> 	if (rate >= REF_CLK_100MHZ)
> instead?

Sounds like a good idea to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/18] mux: add mux_chip_resume() function

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 08:52:17AM +0100, Thomas Richard wrote:
> On 2/15/24 16:29, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote:

...

> >> +int mux_chip_resume(struct mux_chip *mux_chip)
> >> +{
> >> +	int global_ret = 0;
> >> +	int ret, i;
> >> +
> >> +	for (i = 0; i < mux_chip->controllers; ++i) {
> >> +		struct mux_control *mux = &mux_chip->mux[i];
> >> +
> >> +		if (mux->cached_state == MUX_CACHE_UNKNOWN)
> >> +			continue;
> >> +
> >> +		ret = mux_control_set(mux, mux->cached_state);
> >> +		if (ret < 0) {
> >> +			dev_err(&mux_chip->dev, "unable to restore state\n");
> >> +			if (!global_ret)
> >> +				global_ret = ret;
> > 
> > Hmm... This will record the first error and continue.
> 
> In the v2 we talked about this with Peter Rosin.
> 
> In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was
> done in the mmio driver) I had the same behavior: try to restore all
> muxes and in case of error restore the first one.
> 
> I don't know what is the right solution. I just restored the behavior I
> had in v1.

Okay, I believe you know what you are doing, folks. But to me this approach
sounds at bare minimum "unusual". Because the failures here are not fatal
and recording the first one may or may not make sense and it's so fragile
as it completely implementation-dependent.

> >> +		}
> >> +	}
> >> +	return global_ret;
> > 
> > So here, we actually will get stale data in case there are > 1 failures.
> 
> Yes, indeed. But we will have an error message for each failure.

Which is also problematic. PM calls may easily spam the logs and outshadow
really important messages (like oopses).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote:
> On 2/15/24 16:27, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
> >> No need to check the pointer returned by platform_get_drvdata(), as
> >> platform_set_drvdata() is called during the probe.
> > 
> > This patch should go _after_ the next one, otherwise the commit message doesn't
> > tell full story and the code change bring a potential regression.
> 
> Hello Andy,
> 
> I'm ok to move this patch after the next one.
> But for my understanding, could you explain me why changing the order is
> important in this case ?

Old PM calls obviously can be called in different circumstances and these
checks are important.

Just squash these two patches to avoid additional churn and we are done.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/18] mux: add mux_chip_resume() function

[Thomas Richard]

On 2/16/24 16:07, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 08:52:17AM +0100, Thomas Richard wrote:
>> On 2/15/24 16:29, Andy Shevchenko wrote:
>>> On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote:
> 
> ...
> 
>>>> +int mux_chip_resume(struct mux_chip *mux_chip)
>>>> +{
>>>> +	int global_ret = 0;
>>>> +	int ret, i;
>>>> +
>>>> +	for (i = 0; i < mux_chip->controllers; ++i) {
>>>> +		struct mux_control *mux = &mux_chip->mux[i];
>>>> +
>>>> +		if (mux->cached_state == MUX_CACHE_UNKNOWN)
>>>> +			continue;
>>>> +
>>>> +		ret = mux_control_set(mux, mux->cached_state);
>>>> +		if (ret < 0) {
>>>> +			dev_err(&mux_chip->dev, "unable to restore state\n");
>>>> +			if (!global_ret)
>>>> +				global_ret = ret;
>>>
>>> Hmm... This will record the first error and continue.
>>
>> In the v2 we talked about this with Peter Rosin.
>>
>> In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was
>> done in the mmio driver) I had the same behavior: try to restore all
>> muxes and in case of error restore the first one.
>>
>> I don't know what is the right solution. I just restored the behavior I
>> had in v1.
> 
> Okay, I believe you know what you are doing, folks. But to me this approach
> sounds at bare minimum "unusual". Because the failures here are not fatal
> and recording the first one may or may not make sense and it's so fragile
> as it completely implementation-dependent.

I guess if there is an error, the resume is completely dead so no need
to continue.
If it's okay for Peter I can return on first failure.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Thomas Richard]

On 2/16/24 16:08, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote:
>> On 2/15/24 16:27, Andy Shevchenko wrote:
>>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
>>>> No need to check the pointer returned by platform_get_drvdata(), as
>>>> platform_set_drvdata() is called during the probe.
>>>
>>> This patch should go _after_ the next one, otherwise the commit message doesn't
>>> tell full story and the code change bring a potential regression.
>>
>> Hello Andy,
>>
>> I'm ok to move this patch after the next one.
>> But for my understanding, could you explain me why changing the order is
>> important in this case ?
> 
> Old PM calls obviously can be called in different circumstances and these
> checks are important.
> 
> Just squash these two patches to avoid additional churn and we are done.

You mean invert the order instead of squash.

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Philipp Zabel]

On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote:
> Add suspend and resume support.
> 
> The already_configured flag is cleared during the suspend stage to force
> the PHY initialization during the resume stage.
> 
> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/phy/cadence/phy-cadence-torrent.c | 54 +++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> index 52cadca4c07b..f8945a11e7ca 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
>  	cdns_torrent_clk_cleanup(cdns_phy);
>  }
>  
> +static int cdns_torrent_phy_suspend_noirq(struct device *dev)
> +{
> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> +	int i;
> +
> +	reset_control_assert(cdns_phy->phy_rst);
> +	reset_control_assert(cdns_phy->apb_rst);
> +	for (i = 0; i < cdns_phy->nsubnodes; i++)
> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> +
> +	if (cdns_phy->already_configured)
> +		cdns_phy->already_configured = 0;
> +	else
> +		clk_disable_unprepare(cdns_phy->clk);
> +
> +	return 0;
> +}
> +
> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
> +{
> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> +	int node = cdns_phy->nsubnodes;
> +	int ret, i;
> +
> +	ret = cdns_torrent_clk(cdns_phy);
> +	if (ret)
> +		goto clk_cleanup;
> +
> +	/* Enable APB */
> +	reset_control_deassert(cdns_phy->apb_rst);
> +
> +	if (cdns_phy->nsubnodes > 1) {
> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
> +		if (ret)
> +			goto put_lnk_rst;
> +	}
> +
> +	return 0;
> +
> +put_lnk_rst:
> +	for (i = 0; i < node; i++)
> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);

The same cleanup is found in probe. Would it be cleaner to move this
into cdns_torrent_phy_configure_multilink() instead of duplicating it
here?

> +	reset_control_assert(cdns_phy->apb_rst);
> +	clk_disable_unprepare(cdns_phy->clk);
> +clk_cleanup:
> +	cdns_torrent_clk_cleanup(cdns_phy);

This calls of_clk_del_provider(), seems misplaced here.

regards
Philipp

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Andy Shevchenko]

On Wed, Feb 21, 2024 at 12:01:43PM +0100, Thomas Richard wrote:
> On 2/16/24 16:08, Andy Shevchenko wrote:
> > On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote:
> >> On 2/15/24 16:27, Andy Shevchenko wrote:
> >>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
> >>>> No need to check the pointer returned by platform_get_drvdata(), as
> >>>> platform_set_drvdata() is called during the probe.
> >>>
> >>> This patch should go _after_ the next one, otherwise the commit message doesn't
> >>> tell full story and the code change bring a potential regression.
> >>
> >> Hello Andy,
> >>
> >> I'm ok to move this patch after the next one.
> >> But for my understanding, could you explain me why changing the order is
> >> important in this case ?
> > 
> > Old PM calls obviously can be called in different circumstances and these
> > checks are important.
> > 
> > Just squash these two patches to avoid additional churn and we are done.
> 
> You mean invert the order instead of squash.

Either would work, but see how much churn in terms of changing just changed
lines it adds.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

On 2/21/24 14:09, Philipp Zabel wrote:
> On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote:
>> Add suspend and resume support.
>>
>> The already_configured flag is cleared during the suspend stage to force
>> the PHY initialization during the resume stage.
>>
>> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/phy/cadence/phy-cadence-torrent.c | 54 +++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
>> index 52cadca4c07b..f8945a11e7ca 100644
>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>> @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
>>  	cdns_torrent_clk_cleanup(cdns_phy);
>>  }
>>  
>> +static int cdns_torrent_phy_suspend_noirq(struct device *dev)
>> +{
>> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
>> +	int i;
>> +
>> +	reset_control_assert(cdns_phy->phy_rst);
>> +	reset_control_assert(cdns_phy->apb_rst);
>> +	for (i = 0; i < cdns_phy->nsubnodes; i++)
>> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
>> +
>> +	if (cdns_phy->already_configured)
>> +		cdns_phy->already_configured = 0;
>> +	else
>> +		clk_disable_unprepare(cdns_phy->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
>> +{
>> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
>> +	int node = cdns_phy->nsubnodes;
>> +	int ret, i;
>> +
>> +	ret = cdns_torrent_clk(cdns_phy);
>> +	if (ret)
>> +		goto clk_cleanup;
>> +
>> +	/* Enable APB */
>> +	reset_control_deassert(cdns_phy->apb_rst);
>> +
>> +	if (cdns_phy->nsubnodes > 1) {
>> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
>> +		if (ret)
>> +			goto put_lnk_rst;
>> +	}
>> +
>> +	return 0;
>> +
>> +put_lnk_rst:
>> +	for (i = 0; i < node; i++)
>> +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> 
> The same cleanup is found in probe. Would it be cleaner to move this
> into cdns_torrent_phy_configure_multilink() instead of duplicating it
> here?

Hello Philipp,

Yes I could, but from my point of view, it would not be cleaner.
This cleanup is called from many places in the probe:
-
https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2948
-
https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2954
-
https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2960

If I add this cleanup in cdns_torrent_phy_configure_multilink(), yes I
could remove it from cdns_torrent_phy_resume_noirq(), but I should keep
it in the probe. And I should modify the probe to jump to clk_cleanup if
cdns_torrent_phy_configure_multilink() fails.

> 
>> +	reset_control_assert(cdns_phy->apb_rst);
>> +	clk_disable_unprepare(cdns_phy->clk);
>> +clk_cleanup:
>> +	cdns_torrent_clk_cleanup(cdns_phy);
> 
> This calls of_clk_del_provider(), seems misplaced here.

Yes you're right, it's called in cdns_torrent_phy_remove().
So I should not call it in the resume callback, this will cause some
issues during the remove.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 14/18] phy: cadence-torrent: add suspend and resume support

[Philipp Zabel]

On Mi, 2024-02-21 at 14:41 +0100, Thomas Richard wrote:
> On 2/21/24 14:09, Philipp Zabel wrote:
> > On Do, 2024-02-15 at 16:17 +0100, Thomas Richard wrote:
> > > Add suspend and resume support.
> > > 
> > > The already_configured flag is cleared during the suspend stage to force
> > > the PHY initialization during the resume stage.
> > > 
> > > Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
> > > 
> > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> > > ---
> > >  drivers/phy/cadence/phy-cadence-torrent.c | 54 +++++++++++++++++++++++++++++++
> > >  1 file changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> > > index 52cadca4c07b..f8945a11e7ca 100644
> > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > @@ -3005,6 +3005,59 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
> > >  	cdns_torrent_clk_cleanup(cdns_phy);
> > >  }
> > >  
> > > +static int cdns_torrent_phy_suspend_noirq(struct device *dev)
> > > +{
> > > +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> > > +	int i;
> > > +
> > > +	reset_control_assert(cdns_phy->phy_rst);
> > > +	reset_control_assert(cdns_phy->apb_rst);
> > > +	for (i = 0; i < cdns_phy->nsubnodes; i++)
> > > +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> > > +
> > > +	if (cdns_phy->already_configured)
> > > +		cdns_phy->already_configured = 0;
> > > +	else
> > > +		clk_disable_unprepare(cdns_phy->clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int cdns_torrent_phy_resume_noirq(struct device *dev)
> > > +{
> > > +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> > > +	int node = cdns_phy->nsubnodes;
> > > +	int ret, i;
> > > +
> > > +	ret = cdns_torrent_clk(cdns_phy);
> > > +	if (ret)
> > > +		goto clk_cleanup;
> > > +
> > > +	/* Enable APB */
> > > +	reset_control_deassert(cdns_phy->apb_rst);
> > > +
> > > +	if (cdns_phy->nsubnodes > 1) {
> > > +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
> > > +		if (ret)
> > > +			goto put_lnk_rst;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +put_lnk_rst:
> > > +	for (i = 0; i < node; i++)
> > > +		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> > 
> > The same cleanup is found in probe. Would it be cleaner to move this
> > into cdns_torrent_phy_configure_multilink() instead of duplicating it
> > here?
> 
> Hello Philipp,
> 
> Yes I could, but from my point of view, it would not be cleaner.
> This cleanup is called from many places in the probe:
> -
> https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2948
> -
> https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2954
> -
> https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/phy/cadence/phy-cadence-torrent.c#L2960
>
> If I add this cleanup in cdns_torrent_phy_configure_multilink(), yes I
> could remove it from cdns_torrent_phy_resume_noirq(), but I should keep
> it in the probe. And I should modify the probe to jump to clk_cleanup if
> cdns_torrent_phy_configure_multilink() fails.

I see it now. If it can't be consolidated, it's not useful to move it
around.

> 
regards
Philipp

Re: [PATCH v3 02/18] pinctrl: pinctrl-single: remove dead code in suspend() and resume() callbacks

[Thomas Richard]

On 2/21/24 14:13, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 12:01:43PM +0100, Thomas Richard wrote:
>> On 2/16/24 16:08, Andy Shevchenko wrote:
>>> On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote:
>>>> On 2/15/24 16:27, Andy Shevchenko wrote:
>>>>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote:
>>>>>> No need to check the pointer returned by platform_get_drvdata(), as
>>>>>> platform_set_drvdata() is called during the probe.
>>>>>
>>>>> This patch should go _after_ the next one, otherwise the commit message doesn't
>>>>> tell full story and the code change bring a potential regression.
>>>>
>>>> Hello Andy,
>>>>
>>>> I'm ok to move this patch after the next one.
>>>> But for my understanding, could you explain me why changing the order is
>>>> important in this case ?
>>>
>>> Old PM calls obviously can be called in different circumstances and these
>>> checks are important.
>>>
>>> Just squash these two patches to avoid additional churn and we are done.
>>
>> You mean invert the order instead of squash.
> 
> Either would work, but see how much churn in terms of changing just changed
> lines it adds.

OK thanks.

I'll squash the two patches. And I'll add a comment which explains that
I dropped some dead code.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 01/18] gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()

[Linus Walleij]

On Thu, Feb 15, 2024 at 4:18 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 17/18] PCI: j721e: add reset GPIO to struct j721e_pcie

[Thomas Richard]

On 2/15/24 17:04, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 04:18:02PM +0100, Thomas Richard wrote:
>> From: Théo Lebrun <theo.lebrun@bootlin.com>
>>
>> Add reset GPIO to struct j721e_pcie, so it can be used at suspend and
>> resume stages.
> 
> ...
> 
>>  	case PCI_MODE_RC:
>> -		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> -		if (IS_ERR(gpiod)) {
>> -			ret = PTR_ERR(gpiod);
>> +		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +		if (IS_ERR(pcie->reset_gpio)) {
>> +			ret = PTR_ERR(pcie->reset_gpio);
>>  			if (ret != -EPROBE_DEFER)
>>  				dev_err(dev, "Failed to get reset GPIO\n");
>>  			goto err_get_sync;
>> @@ -504,9 +504,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>>  		 * mode is selected while enabling the PHY. So deassert PERST#
>>  		 * after 100 us.
>>  		 */
>> -		if (gpiod) {
>> +		if (pcie->reset_gpio) {
>>  			usleep_range(100, 200);
>> -			gpiod_set_value_cansleep(gpiod, 1);
>> +			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
>>  		}
> 
> Instead of all this, just add one line assignment. Moreover, before or after
> this patch refactor the code to use ret = dev_err_probe(...); pattern that
> eliminates those deferral probe checks.

Hi Andy,

I'm not sure what you exactly want when you write "just add one line
assignment".
For the dev_err_probe() it's okay, it will be fixed in the next iteration.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 17/18] PCI: j721e: add reset GPIO to struct j721e_pcie

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 06:05:16PM +0100, Thomas Richard wrote:
> On 2/15/24 17:04, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:18:02PM +0100, Thomas Richard wrote:
> >> From: Théo Lebrun <theo.lebrun@bootlin.com>

...

> >>  	case PCI_MODE_RC:
> >> -		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >> -		if (IS_ERR(gpiod)) {
> >> -			ret = PTR_ERR(gpiod);
> >> +		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >> +		if (IS_ERR(pcie->reset_gpio)) {
> >> +			ret = PTR_ERR(pcie->reset_gpio);
> >>  			if (ret != -EPROBE_DEFER)
> >>  				dev_err(dev, "Failed to get reset GPIO\n");
> >>  			goto err_get_sync;
> >> @@ -504,9 +504,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >>  		 * mode is selected while enabling the PHY. So deassert PERST#
> >>  		 * after 100 us.
> >>  		 */
> >> -		if (gpiod) {
> >> +		if (pcie->reset_gpio) {
> >>  			usleep_range(100, 200);
> >> -			gpiod_set_value_cansleep(gpiod, 1);
> >> +			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> >>  		}
> > 
> > Instead of all this, just add one line assignment. Moreover, before or after
> > this patch refactor the code to use ret = dev_err_probe(...); pattern that
> > eliminates those deferral probe checks.
> 
> Hi Andy,
> 
> I'm not sure what you exactly want when you write "just add one line
> assignment".

		pcie->reset_gpio = gpiod;

That's it. No additional code changes are needed (WRT the above context,
of course you want to have a new structure member).

-- 
With Best Regards,
Andy Shevchenko