This adds suspend to ram support for the PCIe (RC mode) on J7200 platform.
In this 5th iteration, the series was rebased on Linux 6.9-rc1.
The PHY patches were moved to a dedicated series.
The patch for the pinctrl-single driver was removed, as it was already
applied to the pinctrl tree.
Regards,
Thomas
Signed-off-by: Thomas Richard <[email protected]>
---
Changes in v5:
- all: series rebased on Linux 6.9-rc1
- pinctrl-single: patch removed (already applied to the pinctrl tree)
- phy: patches moved to a dedicated series.
- pci: add T_PERST_CLK_US macro.
- pci-j721e: update the comments about T_PERST_CLK_US.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- all: use SoB/Co-developed-by for patches initially developed by Théo
Lebrun.
- pinctrl-single: squash the two commits.
- i2c-omap: fix line lenghts of the comment in omap_i2c_suspend().
- mux: mux_chip_resume() return 0 or at the first error.
- phy-j721e-wiz: clean code around dev_err_probe().
- phy-j721e-wiz: use REF_CLK_100MHZ macros.
- pci: fix subject line for all PCI patches.
- pci-cadence: use fsleep() instead of usleep_range().
- pci-cadence: remove cdns_torrent_clk_cleanup() call in
cdns_torrent_phy_resume_noirq().
- pci-j721e: add a patch to use dev_err_probe() instead of dev_err() in the probe().
- pci-j721e: fix unordered header files.
- pci-j721e: remove some log spammers.
- pci-j721e: add a missing clock disable in j721e_pcie_resume_noirq().
- pci-j721e: simplify the patch "Add reset GPIO to struct j721e_pcie"
- Link to v3: https://lore.kernel.org/r/[email protected]
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/[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 (8):
gpio: pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()
i2c: omap: wakeup the controller during suspend() callback
mux: add mux_chip_resume() function
PCI: cadence: Extract link setup sequence from cdns_pcie_host_setup()
PCI: cadence: Set cdns_pcie_host_init() global
PCI: j721e: Use dev_err_probe() in the probe() function
PCI: Add T_PERST_CLK_US macro
PCI: j721e: Use T_PERST_CLK_US macro
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 | 29 +++++
drivers/mux/mmio.c | 12 ++
drivers/pci/controller/cadence/pci-j721e.c | 121 ++++++++++++++++++---
drivers/pci/controller/cadence/pcie-cadence-host.c | 44 +++++---
drivers/pci/controller/cadence/pcie-cadence.h | 12 ++
drivers/pci/pci.h | 3 +
include/linux/mux/driver.h | 1 +
9 files changed, 214 insertions(+), 37 deletions(-)
---
base-commit: 3d31103a742d8c94924848dc0fb5e2ce6b701932
change-id: 20240102-j7200-pcie-s2r-ecb1a979e357
Best regards,
--
Thomas Richard <[email protected]>
Some IOs can be needed during suspend_noirq()/resume_noirq().
So move suspend()/resume() to noirq.
Reviewed-by: Andi Shyti <[email protected]>
Acked-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
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
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.
Reviewed-by: Siddharth Vadapalli <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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 7a66a2f815dc..1d37d5f9f811 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -521,10 +521,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
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 | 29 +++++++++++++++++++++++++++++
include/linux/mux/driver.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..0742aa2a7c73 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -215,6 +215,35 @@ 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)
+ 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;
+}
+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
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.
Reviewed-by: Siddharth Vadapalli <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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 1d37d5f9f811..bb215aeebf20 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -522,6 +522,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);
@@ -531,6 +532,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
Use dev_err_probe() instead of dev_err() in the probe() function to
simplify the code and standardize the error output.
Reviewed-by: Francesco Dolcini <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/controller/cadence/pci-j721e.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 85718246016b..98484f001562 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -482,20 +482,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
- dev_err(dev, "pm_runtime_get_sync failed\n");
+ dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
goto err_get_sync;
}
ret = j721e_pcie_ctrl_init(pcie);
if (ret < 0) {
- dev_err(dev, "pm_runtime_get_sync failed\n");
+ dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
goto err_get_sync;
}
ret = devm_request_irq(dev, irq, j721e_pcie_link_irq_handler, 0,
"j721e-pcie-link-down-irq", pcie);
if (ret < 0) {
- dev_err(dev, "failed to request link state IRQ %d\n", irq);
+ dev_err_probe(dev, ret, "failed to request link state IRQ %d\n", irq);
goto err_get_sync;
}
@@ -505,28 +505,25 @@ static int j721e_pcie_probe(struct platform_device *pdev)
case PCI_MODE_RC:
gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpiod)) {
- ret = PTR_ERR(gpiod);
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "Failed to get reset GPIO\n");
+ ret = dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get reset GPIO\n");
goto err_get_sync;
}
ret = cdns_pcie_init_phy(dev, cdns_pcie);
if (ret) {
- dev_err(dev, "Failed to init phy\n");
+ dev_err_probe(dev, ret, "Failed to init phy\n");
goto err_get_sync;
}
clk = devm_clk_get_optional(dev, "pcie_refclk");
if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- dev_err(dev, "failed to get pcie_refclk\n");
+ ret = dev_err_probe(dev, PTR_ERR(clk), "failed to get pcie_refclk\n");
goto err_pcie_setup;
}
ret = clk_prepare_enable(clk);
if (ret) {
- dev_err(dev, "failed to enable pcie_refclk\n");
+ dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
goto err_pcie_setup;
}
pcie->refclk = clk;
@@ -554,7 +551,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
case PCI_MODE_EP:
ret = cdns_pcie_init_phy(dev, cdns_pcie);
if (ret) {
- dev_err(dev, "Failed to init phy\n");
+ dev_err_probe(dev, ret, "Failed to init phy\n");
goto err_get_sync;
}
--
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.
Reviewed-by: Andi Shyti <[email protected]>
Acked-by: Wolfram Sang <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
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..28417b2a18b0 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
"Power Sequencing and Reset Signal Timings" table (section 2.9.2) in PCI
EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 5.1 indicates PERST#
should be deasserted after minimum of 100us once REFCLK is stable (symbol
T_PERST-CLK).
Add a macro so that PCIe controller drivers can use it.
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/pci.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..c47b1d3b5887 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -16,6 +16,9 @@
/* Power stable to PERST# inactive from PCIe card Electromechanical Spec */
#define PCIE_T_PVPERL_MS 100
+/* REFCLK stable before PERST# inactive from PCIe card Electromechanical Spec */
+#define PCIE_T_PERST_CLK_US 100
+
/*
* PCIe r6.0, sec 5.3.3.2.1 <PME Synchronization>
* Recommends 1ms to 10ms timeout to check L2 ready.
--
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]>
Reviewed-by: Siddharth Vadapalli <[email protected]>
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/controller/cadence/pci-j721e.c | 98 ++++++++++++++++++++++++++++--
1 file changed, 92 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 967a5bf38e26..96316a79ab8a 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -7,6 +7,8 @@
*/
#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/container_of.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/io.h>
@@ -22,6 +24,8 @@
#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
@@ -531,12 +535,12 @@ static int j721e_pcie_probe(struct platform_device *pdev)
pcie->refclk = clk;
/*
- * "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.
+ * "Power Sequencing and Reset Signal Timings" table (section
+ * 2.9.2) in PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION,
+ * REV. 5.1 indicates PERST# should be deasserted after minimum
+ * of 100us once REFCLK is stable (symbol T_PERST-CLK).
+ * The REFCLK to the connector in RC mode is selected while
+ * enabling the PHY. So deassert PERST# after 100 us.
*/
if (gpiod) {
fsleep(PCIE_T_PERST_CLK_US);
@@ -588,6 +592,87 @@ 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)
+ 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)
+ return ret;
+
+ 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)
+ return ret;
+
+ /*
+ * "Power Sequencing and Reset Signal Timings" table (section
+ * 2.9.2) in PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION,
+ * REV. 5.1 indicates PERST# should be deasserted after minimum
+ * of 100us once REFCLK is stable (symbol T_PERST-CLK).
+ * The REFCLK to the connector in RC mode is selected while
+ * enabling the PHY. So deassert PERST# after 100 us.
+ */
+ if (pcie->reset_gpio) {
+ fsleep(PCIE_T_PERST_CLK_US);
+ 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) {
+ 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,
@@ -595,6 +680,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
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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 98484f001562..9af4fd64c1f9 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -52,6 +52,7 @@ struct j721e_pcie {
u32 mode;
u32 num_lanes;
u32 max_lanes;
+ struct gpio_desc *reset_gpio;
void __iomem *user_cfg_base;
void __iomem *intd_cfg_base;
u32 linkdown_irq_regfield;
@@ -508,6 +509,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
ret = dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get reset GPIO\n");
goto err_get_sync;
}
+ pcie->reset_gpio = gpiod;
ret = cdns_pcie_init_phy(dev, cdns_pcie);
if (ret) {
--
2.39.2
Use the T_PERST_CLK_US macro, and the fsleep() function instead of
usleep_range().
Signed-off-by: Thomas Richard <[email protected]>
---
drivers/pci/controller/cadence/pci-j721e.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 9af4fd64c1f9..967a5bf38e26 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -539,7 +539,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
* after 100 us.
*/
if (gpiod) {
- usleep_range(100, 200);
+ fsleep(PCIE_T_PERST_CLK_US);
gpiod_set_value_cansleep(gpiod, 1);
}
--
2.39.2
On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
> 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");
If we're going to ignore this error the message should be a dev_err()
at least.
> +
> + return 0;
> +}
> +
regards,
dan carpenter
On 4/16/24 16:16, Dan Carpenter wrote:
> On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
>> 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");
>
> If we're going to ignore this error the message should be a dev_err()
> at least.
Hello Dan,
In fact we already ignore this error [1]
I only moved the hardware configuration part of cdns_pcie_host_setup()
into a new function cdns_pcie_host_link_setup().
But I can use this patch to switch to dev_err() if needed.
[1]
https://elixir.bootlin.com/linux/v6.9-rc4/source/drivers/pci/controller/cadence/pcie-cadence-host.c#L549
Regards,
Thomas
Hi Thomas,
> +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)
If you don't have CONFIG_PM_SLEEP, though, this doesn't compile.
Andi
On 4/19/24 10:47, Andi Shyti wrote:
> Hi Thomas,
>
>> +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)
>
> If you don't have CONFIG_PM_SLEEP, though, this doesn't compile.
Hello Andi,
Yes indeed, the __maybe_unused attribute is missing for
omap_i2c_suspend() and omap_i2c_resume().
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, Apr 22, 2024 at 11:40:02AM +0200, Thomas Richard wrote:
> On 4/19/24 10:47, Andi Shyti wrote:
> > Hi Thomas,
> >
> >> +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)
> >
> > If you don't have CONFIG_PM_SLEEP, though, this doesn't compile.
>
> Hello Andi,
>
> Yes indeed, the __maybe_unused attribute is missing for
> omap_i2c_suspend() and omap_i2c_resume().
Isn't there a way to avoid having to use the __maybe_unused attribute?
E.g., use DEFINE_SIMPLE_DEV_PM_OPS() as is done by these:
82f9cefadac4 ("serial: 8250_exar: switch to DEFINE_SIMPLE_DEV_PM_OPS()")
f243df0a0be0 ("media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()")
6ccc22a5afcb ("net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()")
Bjorn
Hi Thomas,
On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
<[email protected]> wrote:
> Some IOs can be needed during suspend_noirq()/resume_noirq().
> So move suspend()/resume() to noirq.
>
> Reviewed-by: Andi Shyti <[email protected]>
> Acked-by: Bartosz Golaszewski <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Thomas Richard <[email protected]>
Thanks for your patch, which is now commit 86eb98127332748f ("gpio:
pca953x: move suspend()/resume() to suspend_noirq()/resume_noirq()")
in i2c-host/i2c/i2c-host.
This patch causes regressions on e.g. Salvator-XS.
s2idle:
Freezing user space processes
Freezing user space processes completed (elapsed 0.006 seconds)
OOM killer disabled.
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.003 seconds)
sd 0:0:0:0: [sda] Synchronizing SCSI cache
ata1.00: Entering standby power mode
+i2c-rcar e66d8000.i2c: error -16 : 10000005
+pca953x 4-0020: Failed to sync GPIO dir registers: -16
+pca953x 4-0020: Failed to restore register map: -16
+pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -16
+pca953x 4-0020: PM: failed to resume async noirq: error -16
s2ram:
Detected VIPT I-cache on CPU7
CPU7: Booted secondary processor 0x0000000103 [0x410fd034]
CPU7 is up
+i2c-rcar e66d8000.i2c: error -110 : 10000001
+pca953x 4-0020: Failed to sync GPIO dir registers: -110
+pca953x 4-0020: Failed to restore register map: -110
+pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
returns -110
+pca953x 4-0020: PM: failed to resume async noirq: error -110
usb usb1: root hub lost power or was reset
...
PM: suspend exit
ata1: link resume succeeded after 1 retries
-ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
-sd 0:0:0:0: [sda] Starting disk
-ata1.00: configured for UDMA/133
-ata1.00: Entering active power mode
+ata1: SATA link down (SStatus 0 SControl 300)
+ata1: link resume succeeded after 1 retries
+ata1: SATA link down (SStatus 0 SControl 300)
+ata1: limiting SATA link speed to <unknown>
+ata1: link resume succeeded after 1 retries
+ata1: SATA link down (SStatus 0 SControl 3F0)
+ata1.00: disable device
+ata1.00: detaching (SCSI 0:0:0:0)
+sd 0:0:0:0: [sda] Synchronizing SCSI cache
+sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK
When trying to read from /dev/sda afterwards:
ata1: link resume succeeded after 1 retries
ata1: SATA link down (SStatus 0 SControl 3F0)
ata1.00: disable device
ata1.00: detaching (SCSI 0:0:0:0)
device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x80700 phys_seg 4 prio class 0
device offline error, dev sda, sector 0 op 0x0:(READ) flags
0x0 phys_seg 1 prio class 0
Buffer I/O error on dev sda, logical block 0, async page read
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=DRIVER_OK
All issues above are fixed by reverting this commit.
> --- 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))
>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
<[email protected]> wrote:
> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
> <[email protected]> wrote:
..
> +i2c-rcar e66d8000.i2c: error -16 : 10000005
It probably means that I²C host controller is already in power off
mode and can't serve anymore.
> +pca953x 4-0020: Failed to sync GPIO dir registers: -16
> +pca953x 4-0020: Failed to restore register map: -16
> +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
> returns -16
> +pca953x 4-0020: PM: failed to resume async noirq: error -16
Yeah, with this it's kinda forcing _every_ I²C host controller PM to
be moved also to noirq() or alike.
..
> All issues above are fixed by reverting this commit.
Let's revert as we close to the end of the cycle and this is not
something that can be fixed ASAP in my opinion.
..
Thanks for the report, It's a pity that it wasn't tested before from
the mailing list...
--
With Best Regards,
Andy Shevchenko
On 4/23/24 12:34, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
> <[email protected]> wrote:
>> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
>> <[email protected]> wrote:
>
> ...
>
>> +i2c-rcar e66d8000.i2c: error -16 : 10000005
>
> It probably means that I²C host controller is already in power off
> mode and can't serve anymore.
Hello,
Yes the i2c controller is already off.
In fact it's the same issue I had with the i2c-omap driver.
In suspend-noirq, the runtime pm is disabled, so you can't wakeup a
device. More details available in this thread [1].
So the trick is to wakeup the device during suspend (like I did for the
i2c-omap driver [2].
[1]
https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/all/[email protected]/
I think the patch below should fix the issue.
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1232,7 +1232,7 @@ static void rcar_i2c_remove(struct platform_device
*pdev)
pm_runtime_disable(dev);
}
-static int rcar_i2c_suspend(struct device *dev)
+static int rcar_i2c_suspend_noirq(struct device *dev)
{
struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
@@ -1240,7 +1240,7 @@ static int rcar_i2c_suspend(struct device *dev)
return 0;
}
-static int rcar_i2c_resume(struct device *dev)
+static int rcar_i2c_resume_noirq(struct device *dev)
{
struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
@@ -1248,8 +1248,23 @@ static int rcar_i2c_resume(struct device *dev)
return 0;
}
+static int rcar_i2c_suspend(struct device *dev)
+{
+ pm_runtime_get_sync(dev);
+
+ return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+ pm_runtime_put(dev);
+
+ return 0;
+}
+
static const struct dev_pm_ops rcar_i2c_pm_ops = {
- NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend_noirq,
rcar_i2c_resume_noirq)
+ SYSTEM_SLEEP_PM_OPS(rcar_i2c_suspend, rcar_i2c_resume)
};
static struct platform_driver rcar_i2c_driver = {
>
>> +pca953x 4-0020: Failed to sync GPIO dir registers: -16
>> +pca953x 4-0020: Failed to restore register map: -16
>> +pca953x 4-0020: PM: dpm_run_callback(): pca953x_resume_noirq
>> returns -16
>> +pca953x 4-0020: PM: failed to resume async noirq: error -16
>
> Yeah, with this it's kinda forcing _every_ I²C host controller PM to
> be moved also to noirq() or alike.
Yes indeed.
But this controller is already in noirq().
So the issue was already there.
We never saw it because we never did i2c accesses in noirq().
Best Regards,
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Thomas,
On Tue, Apr 23, 2024 at 12:53 PM Thomas Richard
<[email protected]> wrote:
> On 4/23/24 12:34, Andy Shevchenko wrote:
> > On Tue, Apr 23, 2024 at 12:42 PM Geert Uytterhoeven
> > <[email protected]> wrote:
> >> On Tue, Apr 16, 2024 at 3:31 PM Thomas Richard
> >> <[email protected]> wrote:
> >
> > ...
> >
> >> +i2c-rcar e66d8000.i2c: error -16 : 10000005
> >
> > It probably means that I²C host controller is already in power off
> > mode and can't serve anymore.
>
> Yes the i2c controller is already off.
> In fact it's the same issue I had with the i2c-omap driver.
> In suspend-noirq, the runtime pm is disabled, so you can't wakeup a
> device. More details available in this thread [1].
> So the trick is to wakeup the device during suspend (like I did for the
> i2c-omap driver [2].
>
> [1]
> https://lore.kernel.org/all/[email protected]/
> [2]
> https://lore.kernel.org/all/[email protected]/
>
> I think the patch below should fix the issue.
Thanks, I gave that a try, but it doesn't make any difference.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 4/22/24 21:44, Bjorn Helgaas wrote:
> On Mon, Apr 22, 2024 at 11:40:02AM +0200, Thomas Richard wrote:
>> On 4/19/24 10:47, Andi Shyti wrote:
>>> Hi Thomas,
>>>
>>>> +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)
>>>
>>> If you don't have CONFIG_PM_SLEEP, though, this doesn't compile.
>>
>> Hello Andi,
>>
>> Yes indeed, the __maybe_unused attribute is missing for
>> omap_i2c_suspend() and omap_i2c_resume().
>
> Isn't there a way to avoid having to use the __maybe_unused attribute?
>
> E.g., use DEFINE_SIMPLE_DEV_PM_OPS() as is done by these:
>
> 82f9cefadac4 ("serial: 8250_exar: switch to DEFINE_SIMPLE_DEV_PM_OPS()")
> f243df0a0be0 ("media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()")
> 6ccc22a5afcb ("net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()")
Yes you're right, I don't need the __maybe_unused attribute if I use
NOIRQ_SYSTEM_SLEEP_PM_OPS().
By the way I can add a patch in the series to remove all the
__maybe_unused attributes of this driver.
Regards,
--
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com