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 v2 is the add of mux_chip_resume() function in the
mux core.
This function restores the state of mux-chip using cached state. It's used
by mmio driver in the resume_noirq() callback.
Signed-off-by: Thomas Richard <[email protected]>
---
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/[email protected]
---
Thomas Richard (11):
gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()
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: make wiz_clock_init callable multiple times
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
Théo Lebrun (4):
mux: mmio: add resume support
PCI: cadence: add resume support to cdns_pcie_host_setup()
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 | 14 +++
drivers/mux/core.c | 27 +++++
drivers/mux/mmio.c | 12 ++
drivers/pci/controller/cadence/pci-j721e.c | 93 ++++++++++++++--
drivers/pci/controller/cadence/pcie-cadence-host.c | 49 +++++----
drivers/pci/controller/cadence/pcie-cadence-plat.c | 2 +-
drivers/pci/controller/cadence/pcie-cadence.h | 4 +-
drivers/phy/cadence/phy-cadence-torrent.c | 122 +++++++++++++++------
drivers/phy/ti/phy-j721e-wiz.c | 95 ++++++++++++----
drivers/pinctrl/pinctrl-single.c | 28 ++---
include/linux/mux/driver.h | 1 +
12 files changed, 343 insertions(+), 111 deletions(-)
---
base-commit: 00ff0f9ce40db8e64fe16c424a965fd7ab769c42
change-id: 20240102-j7200-pcie-s2r-ecb1a979e357
Best regards,
--
Thomas Richard <[email protected]>
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).
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pinctrl/pinctrl-single.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 19cc0db771a5..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,14 +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;
-
- pcs = platform_get_drvdata(pdev);
- if (!pcs)
- return -EINVAL;
+ struct pcs_device *pcs = dev_get_drvdata(dev);
if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
int ret;
@@ -1710,20 +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;
-
- pcs = platform_get_drvdata(pdev);
- if (!pcs)
- return -EINVAL;
+ 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
@@ -1986,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
Some IOs can be needed during suspend_noirq()/resume_noirq().
So move suspend()/resume() to noirq.
Signed-off-by: Thomas Richard <[email protected]>
---
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
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 <[email protected]>
---
drivers/i2c/busses/i2c-omap.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 42165ef57946..cc9841cb08f1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1575,9 +1575,23 @@ static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
return 0;
}
+static int omap_i2c_suspend(struct device *dev)
+{
+ 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
The mux_chip_resume() function restores a mux_chip using the cached state
of each mux.
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/mux/core.c | 27 +++++++++++++++++++++++++++
include/linux/mux/driver.h | 1 +
2 files changed, 28 insertions(+)
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..896f74b34eb8 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -215,6 +215,33 @@ 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 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) {
+ ret = mux_control_set(mux, mux->cached_state);
+ if (ret < 0) {
+ dev_err(&mux_chip->dev, "unable to restore state\n");
+ return ret;
+ }
+ }
+ }
+ return 0;
+}
+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
From: Théo Lebrun <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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
For suspend and resume support, wiz_clock_init needs to be called multiple
times.
Add a parameter to wiz_clock_init to be able to skip clocks registration.
Based on the work of Théo Lebrun <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index fc3cd98c60ff..09f7edf16562 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz)
return ret;
}
-static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
+static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
{
const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
struct device *dev = wiz->dev;
@@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
int ret;
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 (probe) {
+ 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;
+ }
+ 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)) {
+ dev_err(dev, "core_ref1_clk clock not found\n");
+ ret = PTR_ERR(clk);
+ return ret;
+ }
+ wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
+ }
+
+ 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;
+ }
+ wiz->input_clks[WIZ_EXT_REFCLK] = clk;
}
- wiz->input_clks[WIZ_CORE_REFCLK] = clk;
+
+ clk = wiz->input_clks[WIZ_CORE_REFCLK];
rate = clk_get_rate(clk);
if (rate >= 100000000)
regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
@@ -1121,14 +1143,7 @@ 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;
- }
- wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
-
+ clk = wiz->input_clks[WIZ_CORE_REFCLK1];
rate = clk_get_rate(clk);
if (rate >= 100000000)
regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
@@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
}
- 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;
- }
- wiz->input_clks[WIZ_EXT_REFCLK] = clk;
-
+ clk = wiz->input_clks[WIZ_EXT_REFCLK];
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);
+ /* What follows is about registering clocks. */
+ if (!probe)
+ return 0;
+
switch (wiz->type) {
case AM64_WIZ_10G:
case J7200_WIZ_10G:
@@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev)
goto err_get_sync;
}
- ret = wiz_clock_init(wiz, node);
+ ret = wiz_clock_init(wiz, node, true);
if (ret < 0) {
dev_warn(dev, "Failed to initialize clocks\n");
goto err_get_sync;
--
2.39.2
Add resume support.
It has been tested on J7200 SR1.0 and SR2.0.
Based on the work of Théo Lebrun <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/phy/ti/phy-j721e-wiz.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index 09f7edf16562..163ac3e95ddb 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1666,12 +1666,47 @@ 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);
+
+ ret = wiz_clock_init(wiz, node, false);
+ if (ret < 0) {
+ dev_warn(dev, "Failed to initialize clocks\n");
+ goto err_get_sync;
+ }
+
+ 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);
+
+err_get_sync:
+
+ 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
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 <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/phy/cadence/phy-cadence-torrent.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index a75c96385c57..94298ad9f875 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -2681,18 +2681,22 @@ static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
return 0;
}
-static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
+static int cdns_torrent_of_get_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");
+ cdns_phy->clk = devm_clk_get(cdns_phy->dev, "refclk");
if (IS_ERR(cdns_phy->clk)) {
- dev_err(dev, "phy ref clock not found\n");
+ dev_err(cdns_phy->dev, "phy ref clock not found\n");
return PTR_ERR(cdns_phy->clk);
}
+ return 0;
+}
+
+static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
+{
+ unsigned long ref_clk_rate;
+ int ret;
+
ret = clk_prepare_enable(cdns_phy->clk);
if (ret) {
dev_err(cdns_phy->dev, "Failed to prepare ref clock\n");
@@ -2776,6 +2780,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
Add already_configured to struct cdns_torrent_phy, so it can be used at
differents stages.
Signed-off-by: Thomas Richard <[email protected]>
---
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 100b58965558..a1e9d06453e6 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;
@@ -2741,7 +2742,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;
@@ -2788,9 +2788,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;
@@ -2870,7 +2870,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);
@@ -2956,7 +2956,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
Add suspend and resume support.
The already_configured flag is cleared during suspend stage to force the
PHY initialization during the resume stage.
Based on the work of Théo Lebrun <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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 70413fca5776..5f4ecf371030 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -3006,6 +3006,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)
+ clk_disable_unprepare(cdns_phy->clk);
+ else
+ cdns_phy->already_configured = 0;
+
+ 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},
@@ -4577,6 +4630,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
From: Théo Lebrun <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/controller/cadence/pci-j721e.c | 79 ++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 477275d72257..668d98422c4d 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 <[email protected]>
*/
+#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,80 @@ 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_setup(rc, false);
+ if (ret < 0) {
+ clk_disable_unprepare(pcie->refclk);
+ 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 +639,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
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 <[email protected]>
---
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 a1e9d06453e6..70413fca5776 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)
{
@@ -2870,10 +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 (!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
From: Théo Lebrun <[email protected]>
That function mixes probe structure init and hardware config.
The whole hardware config part must be done at resume after a suspend to
ram.
We therefore pass it a boolean flag determining if we are at probe or at
resume.
Signed-off-by: Théo Lebrun <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/controller/cadence/pci-j721e.c | 2 +-
drivers/pci/controller/cadence/pcie-cadence-host.c | 49 ++++++++++++----------
drivers/pci/controller/cadence/pcie-cadence-plat.c | 2 +-
drivers/pci/controller/cadence/pcie-cadence.h | 4 +-
4 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 2c87e7728a65..9b343a46da11 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -509,7 +509,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
gpiod_set_value_cansleep(gpiod, 1);
}
- ret = cdns_pcie_host_setup(rc);
+ ret = cdns_pcie_host_setup(rc, true);
if (ret < 0) {
clk_disable_unprepare(pcie->refclk);
goto err_pcie_setup;
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 5b14f7ee3c79..dd4d876a9138 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -497,14 +497,14 @@ static int cdns_pcie_host_init(struct device *dev,
return cdns_pcie_host_init_address_translation(rc);
}
-int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
+int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
{
struct device *dev = rc->pcie.dev;
struct platform_device *pdev = to_platform_device(dev);
struct device_node *np = dev->of_node;
struct pci_host_bridge *bridge;
enum cdns_pcie_rp_bar bar;
- struct cdns_pcie *pcie;
+ struct cdns_pcie *pcie = &rc->pcie;
struct resource *res;
int ret;
@@ -512,26 +512,27 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
if (!bridge)
return -ENOMEM;
- pcie = &rc->pcie;
- pcie->is_rc = true;
+ if (probe) {
+ pcie->is_rc = true;
- rc->vendor_id = 0xffff;
- of_property_read_u32(np, "vendor-id", &rc->vendor_id);
+ rc->vendor_id = 0xffff;
+ of_property_read_u32(np, "vendor-id", &rc->vendor_id);
- rc->device_id = 0xffff;
- of_property_read_u32(np, "device-id", &rc->device_id);
+ rc->device_id = 0xffff;
+ of_property_read_u32(np, "device-id", &rc->device_id);
- pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
- if (IS_ERR(pcie->reg_base)) {
- dev_err(dev, "missing \"reg\"\n");
- return PTR_ERR(pcie->reg_base);
- }
+ pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
+ if (IS_ERR(pcie->reg_base)) {
+ dev_err(dev, "missing \"reg\"\n");
+ return PTR_ERR(pcie->reg_base);
+ }
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
- rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
- if (IS_ERR(rc->cfg_base))
- return PTR_ERR(rc->cfg_base);
- rc->cfg_res = res;
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+ rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(rc->cfg_base))
+ 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);
@@ -555,12 +556,14 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
if (ret)
return ret;
- if (!bridge->ops)
- bridge->ops = &cdns_pcie_host_ops;
+ if (probe) {
+ if (!bridge->ops)
+ bridge->ops = &cdns_pcie_host_ops;
- ret = pci_host_probe(bridge);
- if (ret < 0)
- goto err_init;
+ ret = pci_host_probe(bridge);
+ if (ret < 0)
+ goto err_init;
+ }
return 0;
diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
index 0456845dabb9..071423091668 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
@@ -86,7 +86,7 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
goto err_get_sync;
}
- ret = cdns_pcie_host_setup(rc);
+ ret = cdns_pcie_host_setup(rc, true);
if (ret)
goto err_init;
} else {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 373cb50fcd15..3b0da889ed64 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -515,11 +515,11 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
}
#ifdef CONFIG_PCIE_CADENCE_HOST
-int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
+int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe);
void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
int where);
#else
-static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
+static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
{
return 0;
}
--
2.39.2
From: Théo Lebrun <[email protected]>
Add reset GPIO to struct j721e_pcie, so it can be used at suspend and
resume stages.
Signed-off-by: Théo Lebrun <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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 9b343a46da11..477275d72257 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, true);
--
2.39.2
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 <[email protected]>
---
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 94298ad9f875..100b58965558 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;
@@ -2780,6 +2780,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;
@@ -2787,10 +2791,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
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> 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 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) {
if (_state == ...)
continue;
?
> + ret = mux_control_set(mux, mux->cached_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to restore state\n");
> + return ret;
> + }
> + }
> + }
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
On the first glance the code is okay, but I haven't tested it on the
platforms I have that use this chip.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> 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).
LGTM, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> From: Théo Lebrun <[email protected]>
>
> 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.
mux_chip_resume()
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> For suspend and resume support, wiz_clock_init needs to be called multiple
wiz_clock_init()
> times.
>
> Add a parameter to wiz_clock_init to be able to skip clocks registration.
Ditto.
..to skip the registration of the clocks.
..
> -static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
So, why not refactor this to two functions first?
..
> + 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;
This is wrong and has to be fixed in the original code.
The correct approach is to use `return dev_err_probe(...)` or open
code it, but since we have a helper, open coding is highly
discouraged.
> + }
..
> + 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;
Ditto.
> + }
..
> + 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;
Ditto.
> + }
..
> + /* What follows is about registering clocks. */
> + if (!probe)
> + return 0;
Just refactor properly, no need to have this ugly mix of devm_*() for
probe stage, parameter and HW related things.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> Add resume support.
> It has been tested on J7200 SR1.0 and SR2.0.
..
> +err_get_sync:
> +
An useless label. Just return directly.
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
In subject, capitalize "Add ..." to follow historical convention.
Also the other driver/pci/ patches.
On Fri, Jan 26, 2024 at 03:36:55PM +0100, Thomas Richard wrote:
> From: Théo Lebrun <[email protected]>
>
> That function mixes probe structure init and hardware config.
> The whole hardware config part must be done at resume after a suspend to
> ram.
> We therefore pass it a boolean flag determining if we are at probe or at
> resume.
s/That function/cdns_pcie_host_setup()/ so the commit log stands
alone even without the subject line.
s/ram/RAM/
Add blank line between paragraphs (or rewrap into a single paragraph).
This is pretty similar but slightly different from the DWC pattern:
imx6_pcie_probe
... do structure init ...
if (RC)
dw_pcie_host_init
pp->ops->init
imx6_pcie_host_init
imx6_pcie_resume_noirq
imx6_pcie_host_init
j721e_pcie_probe
j721e_pcie_ctrl_init
if (RC)
cdns_pcie_host_setup(true)
j721e_pcie_resume_noirq
j721e_pcie_ctrl_init
if (RC)
cdns_pcie_host_setup(false)
It'd be super nice to have them the same. Passing in a "probe" flag
works but seems a little harder to read in cdns_pcie_host_setup() and
you have to keep track of what it means in the callers.
> Signed-off-by: Théo Lebrun <[email protected]>
> Signed-off-by: Thomas Richard <[email protected]>
> ---
> drivers/pci/controller/cadence/pci-j721e.c | 2 +-
> drivers/pci/controller/cadence/pcie-cadence-host.c | 49 ++++++++++++----------
> drivers/pci/controller/cadence/pcie-cadence-plat.c | 2 +-
> drivers/pci/controller/cadence/pcie-cadence.h | 4 +-
> 4 files changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 2c87e7728a65..9b343a46da11 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -509,7 +509,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> gpiod_set_value_cansleep(gpiod, 1);
> }
>
> - ret = cdns_pcie_host_setup(rc);
> + ret = cdns_pcie_host_setup(rc, true);
> if (ret < 0) {
> clk_disable_unprepare(pcie->refclk);
> goto err_pcie_setup;
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 5b14f7ee3c79..dd4d876a9138 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -497,14 +497,14 @@ static int cdns_pcie_host_init(struct device *dev,
> return cdns_pcie_host_init_address_translation(rc);
> }
>
> -int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> +int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
> {
> struct device *dev = rc->pcie.dev;
> struct platform_device *pdev = to_platform_device(dev);
> struct device_node *np = dev->of_node;
> struct pci_host_bridge *bridge;
> enum cdns_pcie_rp_bar bar;
> - struct cdns_pcie *pcie;
> + struct cdns_pcie *pcie = &rc->pcie;
> struct resource *res;
> int ret;
>
> @@ -512,26 +512,27 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> if (!bridge)
> return -ENOMEM;
>
> - pcie = &rc->pcie;
> - pcie->is_rc = true;
> + if (probe) {
> + pcie->is_rc = true;
>
> - rc->vendor_id = 0xffff;
> - of_property_read_u32(np, "vendor-id", &rc->vendor_id);
> + rc->vendor_id = 0xffff;
> + of_property_read_u32(np, "vendor-id", &rc->vendor_id);
>
> - rc->device_id = 0xffff;
> - of_property_read_u32(np, "device-id", &rc->device_id);
> + rc->device_id = 0xffff;
> + of_property_read_u32(np, "device-id", &rc->device_id);
>
> - pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
> - if (IS_ERR(pcie->reg_base)) {
> - dev_err(dev, "missing \"reg\"\n");
> - return PTR_ERR(pcie->reg_base);
> - }
> + pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
> + if (IS_ERR(pcie->reg_base)) {
> + dev_err(dev, "missing \"reg\"\n");
> + return PTR_ERR(pcie->reg_base);
> + }
>
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> - rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
> - if (IS_ERR(rc->cfg_base))
> - return PTR_ERR(rc->cfg_base);
> - rc->cfg_res = res;
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> + rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(rc->cfg_base))
> + 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);
> @@ -555,12 +556,14 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> if (ret)
> return ret;
>
> - if (!bridge->ops)
> - bridge->ops = &cdns_pcie_host_ops;
> + if (probe) {
> + if (!bridge->ops)
> + bridge->ops = &cdns_pcie_host_ops;
>
> - ret = pci_host_probe(bridge);
> - if (ret < 0)
> - goto err_init;
> + ret = pci_host_probe(bridge);
> + if (ret < 0)
> + goto err_init;
> + }
>
> return 0;
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> index 0456845dabb9..071423091668 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> @@ -86,7 +86,7 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
> goto err_get_sync;
> }
>
> - ret = cdns_pcie_host_setup(rc);
> + ret = cdns_pcie_host_setup(rc, true);
> if (ret)
> goto err_init;
> } else {
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 373cb50fcd15..3b0da889ed64 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -515,11 +515,11 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
> }
>
> #ifdef CONFIG_PCIE_CADENCE_HOST
> -int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
> +int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe);
> void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> int where);
> #else
> -static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> +static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
> {
> return 0;
> }
>
> --
> 2.39.2
>
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<[email protected]> wrote:
>
> 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 <[email protected]>
(Just a side question: Have you used --histogram diff algo when
preparing the series? Please use, if not)
..
> - cdns_phy->clk = devm_clk_get(dev, "refclk");
> + cdns_phy->clk = devm_clk_get(cdns_phy->dev, "refclk");
> if (IS_ERR(cdns_phy->clk)) {
> - dev_err(dev, "phy ref clock not found\n");
> + dev_err(cdns_phy->dev, "phy ref clock not found\n");
> return PTR_ERR(cdns_phy->clk);
> }
Same issue as I noted in a previous email.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:39 PM Thomas Richard
<[email protected]> wrote:
>
> 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.
Functions are referred to as func().
It is also applicable to other commit messages in this series.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:38 PM Thomas Richard
<[email protected]> wrote:
>
> Add suspend and resume support.
>
> The already_configured flag is cleared during suspend stage to force the
the suspend
> PHY initialization during the resume stage.
..
> + if (!cdns_phy->already_configured)
> + clk_disable_unprepare(cdns_phy->clk);
> + else
> + cdns_phy->already_configured = 0;
What's wrong with a positive check?
Why do you need unprepare clock?
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 11:43 PM Bjorn Helgaas <[email protected]> wrote:
> On Fri, Jan 26, 2024 at 03:36:55PM +0100, Thomas Richard wrote:
..
> It'd be super nice to have them the same. Passing in a "probe" flag
> works but seems a little harder to read in cdns_pcie_host_setup() and
> you have to keep track of what it means in the callers.
FWIW, in another patch with a similar approach I found it (the
approach) a bit ugly and unmaintainable. I would suggest properly
refactoring to create 2+ functions and use as needed.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:38 PM Thomas Richard
<[email protected]> wrote:
>
> From: Théo Lebrun <[email protected]>
>
> Add reset GPIO to struct j721e_pcie, so it can be used at suspend and
> resume stages.
..
> + 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");
A side note: At some point would be nice to see this being changed to simply
ret = dev_err_probe(...);
goto ...;
> goto err_get_sync;
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 26, 2024 at 4:38 PM Thomas Richard
<[email protected]> wrote:
>
> From: Théo Lebrun <[email protected]>
>
> Add suspend and resume support. Only the rc mode is supported.
>
> During the suspend stage PERST# is asserted, then deasserted during the
> resume stage.
..
> +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);
Same Q as in a few mails before: Do you need unprepare? What will be
the benefit from a PM perspective?
> + }
> +
> + cdns_pcie_disable_phy(pcie->cdns_pcie);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Hi Thomas,
On Fri, Jan 26, 2024 at 03:36:43PM +0100, Thomas Richard wrote:
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Signed-off-by: Thomas Richard <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Thanks,
Andi
Hi Thomas,
..
> - struct pcs_device *pcs;
> -
> - pcs = platform_get_drvdata(pdev);
> - if (!pcs)
> - return -EINVAL;
> + struct pcs_device *pcs = dev_get_drvdata(dev);
I think this cleanup can be placed in a different patch. Besides,
it's not mentioned in the commit log.
Otherwise looks good.
Andi
On Fri, Jan 26, 2024 at 3:37 PM Thomas Richard
<[email protected]> wrote:
> 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).
>
> Signed-off-by: Thomas Richard <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Do you want to merge this as a series or is this something I
should just apply?
Yours,
Linus Walleij
On Fri, Jan 26, 2024 at 3:37 PM Thomas Richard
<[email protected]> wrote:
>
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Signed-off-by: Thomas Richard <[email protected]>
> ---
> 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
>
Acked-by: Bartosz Golaszewski <[email protected]>
On Fri, Jan 26, 2024 at 03:43:39PM -0600, Bjorn Helgaas wrote:
> In subject, capitalize "Add ..." to follow historical convention.
> Also the other driver/pci/ patches.
>
> On Fri, Jan 26, 2024 at 03:36:55PM +0100, Thomas Richard wrote:
> > From: Théo Lebrun <[email protected]>
> >
> > That function mixes probe structure init and hardware config.
> > The whole hardware config part must be done at resume after a suspend to
> > ram.
> > We therefore pass it a boolean flag determining if we are at probe or at
> > resume.
> ...
> This is pretty similar but slightly different from the DWC pattern:
>
> imx6_pcie_probe
> ... do structure init ...
> if (RC)
> dw_pcie_host_init
> pp->ops->init
> imx6_pcie_host_init
>
> imx6_pcie_resume_noirq
> imx6_pcie_host_init
>
> j721e_pcie_probe
> j721e_pcie_ctrl_init
> if (RC)
> cdns_pcie_host_setup(true)
>
> j721e_pcie_resume_noirq
> j721e_pcie_ctrl_init
> if (RC)
> cdns_pcie_host_setup(false)
>
> It'd be super nice to have them the same. Passing in a "probe" flag
> works but seems a little harder to read in cdns_pcie_host_setup() and
> you have to keep track of what it means in the callers.
Maybe a better way to say this is that this patch uses the "probe"
flag to select the behavior of cdns_pcie_host_setup(), and I think it
would be nicer to split those two behaviors into separate functions.
Bjorn
On Mon, Jan 29, 2024 at 04:16:49PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 26, 2024 at 03:43:39PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 26, 2024 at 03:36:55PM +0100, Thomas Richard wrote:
> > > From: Théo Lebrun <[email protected]>
> > >
> > > That function mixes probe structure init and hardware config.
> > > The whole hardware config part must be done at resume after a suspend to
> > > ram.
> > > We therefore pass it a boolean flag determining if we are at probe or at
> > > resume.
> > ...
> > It'd be super nice to have them the same. Passing in a "probe" flag
> > works but seems a little harder to read in cdns_pcie_host_setup() and
> > you have to keep track of what it means in the callers.
>
> Maybe a better way to say this is that this patch uses the "probe"
> flag to select the behavior of cdns_pcie_host_setup(), and I think it
> would be nicer to split those two behaviors into separate functions.
Oops, sorry, Andy, I wrote this before I saw your similar response
from last Friday. Didn't mean to repeat what you already said!
Bjorn
Hi Thomas,
On Fri, Jan 26, 2024 at 03:36:45PM +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.
Looks good, can I ask you to put this in a comment below? It's
not completely straight forward.
Thanks,
Andi
> Signed-off-by: Thomas Richard <[email protected]>
> ---
> drivers/i2c/busses/i2c-omap.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 42165ef57946..cc9841cb08f1 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1575,9 +1575,23 @@ static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
> return 0;
> }
>
> +static int omap_i2c_suspend(struct device *dev)
> +{
> + 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
>
Hi Linus,
On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
> On Fri, Jan 26, 2024 at 3:37 PM Thomas Richard
> <[email protected]> wrote:
>
> > 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).
> >
> > Signed-off-by: Thomas Richard <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
>
> Do you want to merge this as a series or is this something I
> should just apply?
there is still a comment from me pending.
Thanks,
Andi
> Yours,
> Linus Walleij
Hi Thomas,
..
> +/**
> + * 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 ret, i;
you could put this 'ret' in the innermost indentation part, which
here is inside the 'if (...)' (or inside the 'for (...)' if you
follow Andy's suggestion).
Andi
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->cached_state != MUX_CACHE_UNKNOWN) {
> + ret = mux_control_set(mux, mux->cached_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to restore state\n");
> + return ret;
> + }
> + }
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_resume);
Hi Thomas,
On Fri, Jan 26, 2024 at 03:36:48PM +0100, Thomas Richard wrote:
> For suspend and resume support, wiz_clock_init needs to be called multiple
> times.
>
> Add a parameter to wiz_clock_init to be able to skip clocks registration.
>
> Based on the work of Th?o Lebrun <[email protected]>
>
> Signed-off-by: Thomas Richard <[email protected]>
> ---
> drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++-----------------
> 1 file changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index fc3cd98c60ff..09f7edf16562 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz)
> return ret;
> }
>
> -static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
the 'bool probe' is ugly. When someone reads something like
"ret = wiz_clock_init(wiz, node, true);" wonders what the
true/false is and needs to follow through the function to
understand.
Please, split this function (and below) into two parts:
wiz_clock_probe() and wiz_clock_init(). The former calls the
latter. Later on you can just call wiz_clock_init().
Andi
> {
> const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
> struct device *dev = wiz->dev;
> @@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> int ret;
> 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 (probe) {
> + 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;
> + }
> + 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)) {
> + dev_err(dev, "core_ref1_clk clock not found\n");
> + ret = PTR_ERR(clk);
> + return ret;
> + }
> + wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> + }
> +
> + 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;
> + }
> + wiz->input_clks[WIZ_EXT_REFCLK] = clk;
> }
> - wiz->input_clks[WIZ_CORE_REFCLK] = clk;
>
> +
> + clk = wiz->input_clks[WIZ_CORE_REFCLK];
> rate = clk_get_rate(clk);
> if (rate >= 100000000)
> regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
> @@ -1121,14 +1143,7 @@ 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;
> - }
> - wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> -
> + clk = wiz->input_clks[WIZ_CORE_REFCLK1];
> rate = clk_get_rate(clk);
> if (rate >= 100000000)
> regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
> @@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
> }
>
> - 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;
> - }
> - wiz->input_clks[WIZ_EXT_REFCLK] = clk;
> -
> + clk = wiz->input_clks[WIZ_EXT_REFCLK];
> 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);
>
> + /* What follows is about registering clocks. */
> + if (!probe)
> + return 0;
> +
> switch (wiz->type) {
> case AM64_WIZ_10G:
> case J7200_WIZ_10G:
> @@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev)
> goto err_get_sync;
> }
>
> - ret = wiz_clock_init(wiz, node);
> + ret = wiz_clock_init(wiz, node, true);
> if (ret < 0) {
> dev_warn(dev, "Failed to initialize clocks\n");
> goto err_get_sync;
>
> --
> 2.39.2
>
Hi!
2024-01-26 at 15:36, Thomas Richard wrote:
> The mux_chip_resume() function restores a mux_chip using the cached state
> of each mux.
>
> Signed-off-by: Thomas Richard <[email protected]>
> ---
> drivers/mux/core.c | 27 +++++++++++++++++++++++++++
> include/linux/mux/driver.h | 1 +
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 775816112932..896f74b34eb8 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -215,6 +215,33 @@ 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 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) {
> + ret = mux_control_set(mux, mux->cached_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to restore state\n");
> + return ret;
I'm don't know what is expected of the core resume code on error,
but is it ok to return on first failure? Is it not better to try
to restore all muxes and return zero if all is well or the first
failure when something is up?
But maybe the resume is completely dead anyway if there is any
failure? In that case the above early return is fine, I guess...
Cheers,
Peter
> + }
> + }
> + }
> + return 0;
> +}
> +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,
>
On 1/30/24 09:25, Peter Rosin wrote:
> Hi!
>
> 2024-01-26 at 15:36, Thomas Richard wrote:
>> The mux_chip_resume() function restores a mux_chip using the cached state
>> of each mux.
>>
>> Signed-off-by: Thomas Richard <[email protected]>
>> ---
>> drivers/mux/core.c | 27 +++++++++++++++++++++++++++
>> include/linux/mux/driver.h | 1 +
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 775816112932..896f74b34eb8 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -215,6 +215,33 @@ 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 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) {
>> + ret = mux_control_set(mux, mux->cached_state);
>> + if (ret < 0) {
>> + dev_err(&mux_chip->dev, "unable to restore state\n");
>> + return ret;
>
> I'm don't know what is expected of the core resume code on error,
> but is it ok to return on first failure? Is it not better to try
> to restore all muxes and return zero if all is well or the first
> failure when something is up?
>
> But maybe the resume is completely dead anyway if there is any
> failure? In that case the above early return is fine, I guess...
>
In the first iteration of this series (when it was done in mmio driver),
it restored all muxes and returned zero or the first failure.
I don't know why I changed the behaviour.
For me it's better to try to restores all muxes.
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 1/26/24 22:28, Andy Shevchenko wrote:
> On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
> <[email protected]> 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 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) {
>
> if (_state == ...)
> continue;
>
> ?
Yes it makes the code easier to read.
Fixed in next iteration.
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
* Andi Shyti <[email protected]> [240129 22:49]:
> On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
> > On Fri, Jan 26, 2024 at 3:37 PM Thomas Richard
> > <[email protected]> wrote:
> >
> > > 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).
> > >
> > > Signed-off-by: Thomas Richard <[email protected]>
> >
> > Reviewed-by: Linus Walleij <[email protected]>
> >
> > Do you want to merge this as a series or is this something I
> > should just apply?
>
> there is still a comment from me pending.
FYI I gave this a brief test and things seem to work fine for me. Sounds
like there will be another revision though so I'll test again then.
Regards,
Tony
* Thomas Richard <[email protected]> [240126 14:37]:
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
Hmm so what happened to the earlier i2c transfer at noirq level comments
from me and Linus W here? It seems the cover letter mentions it but I
don't see the related changes in this patch?
Regards,
Tony
On 1/29/24 23:49, Andi Shyti wrote:
> Hi Linus,
>
> On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
>> On Fri, Jan 26, 2024 at 3:37 PM Thomas Richard
>> <[email protected]> wrote:
>>
>>> 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).
>>>
>>> Signed-off-by: Thomas Richard <[email protected]>
>>
>> Reviewed-by: Linus Walleij <[email protected]>
>>
>> Do you want to merge this as a series or is this something I
>> should just apply?
>
> there is still a comment from me pending.
Hi Andi,
Based on your comment, for the next iteration, I will move the cleanup
in a dedicated patch.
@Linus, you can apply pinctrl patches once everything is ok for you.
Regards,
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com