[PATCH 0/5] More SoCs for sdhci-omap to deprecate omap_hsmmc

[PATCH 0/5] More SoCs for sdhci-omap to deprecate omap_hsmmc

[Tony Lindgren]

Hi,

This series of changes adds support to sdhci-omap for earlier SoCs so we
can start deprecating the old omap_hsmmc driver.

For most part these changes can be tested just by changing the old hsmmc
devicetree compatible value for the SoC as described in the first binding
patch.

There are few old quirks I have not yet handled, like the wl1251 quirk
f6498b922e57 ("mmc: host: omap_hsmmc: add code for special init of wl1251
to get rid of pandora_wl1251_init_card"). But those can be added later
once we figure out how to best handle them :)

These patches are against v5.15-rc1, and depend on patch series
"[PATCH 0/5] Prepare sdhci-omap to support more SoCs".

Regards,

Tony


Tony Lindgren (5):
  dt-bindings: sdhci-omap: Update binding for legacy SoCs
  mmc: sdhci-omap: Handle voltages to add support omap4
  mmc: sdhci-omap: Add omap_offset to support omap3 and earlier
  mmc: sdhci-omap: Implement PM runtime functions
  mmc: sdhci-omap: Configure optional wakeirq

 .../devicetree/bindings/mmc/sdhci-omap.txt    |   6 +-
 drivers/mmc/host/sdhci-omap.c                 | 270 ++++++++++++++----
 2 files changed, 221 insertions(+), 55 deletions(-)

-- 
2.33.0

[PATCH 1/5] dt-bindings: sdhci-omap: Update binding for legacy SoCs

[Tony Lindgren]

Let's add compatible values for the legacy SoCs so we can continue
deprecating omap_hsmmc in favor of sdhci-omap driver.

For omap5, we want to have a separate compatible from omap4 for the
additional features available on omap5. AFAIK ti81 can just use the
omap4 compatible.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -5,7 +5,11 @@ Refer to mmc.txt for standard MMC bindings.
 For UHS devices which require tuning, the device tree should have a "cpu_thermal" node which maps to the appropriate thermal zone. This is used to get the temperature of the zone during tuning.
 
 Required properties:
-- compatible: Should be "ti,dra7-sdhci" for DRA7 and DRA72 controllers
+- compatible: Should be "ti,omap2430-sdhci" for omap2430 controllers
+	      Should be "ti,omap3-sdhci" for omap3 controllers
+	      Should be "ti,omap4-sdhci" for omap4 and ti81 controllers
+	      Should be "ti,omap5-sdhci" for omap5 controllers
+	      Should be "ti,dra7-sdhci" for DRA7 and DRA72 controllers
 	      Should be "ti,k2g-sdhci" for K2G
 	      Should be "ti,am335-sdhci" for am335x controllers
 	      Should be "ti,am437-sdhci" for am437x controllers
-- 
2.33.0

[PATCH 2/5] mmc: sdhci-omap: Handle voltages to add support omap4

[Tony Lindgren]

In order to start deprecating the custom omap_hsmmc.c in favor of the
generic sdhci-omap driver, we need to add support for voltages for earlier
SoCs.

The PBIAS regulator on omap4 and earlier only supports nominal values of
1.8V and 3.0V, while omap5 and later support nominal values of 1.8V and
3.3V IO voltage.

This gets omap4/5 working with sdhci-omap driver.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mmc/host/sdhci-omap.c | 124 ++++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -178,7 +178,7 @@ static int sdhci_omap_set_pbias(struct sdhci_omap_host *omap_host,
 }
 
 static int sdhci_omap_enable_iov(struct sdhci_omap_host *omap_host,
-				 unsigned int iov)
+				 unsigned int iov_pbias)
 {
 	int ret;
 	struct sdhci_host *host = omap_host->host;
@@ -189,14 +189,15 @@ static int sdhci_omap_enable_iov(struct sdhci_omap_host *omap_host,
 		return ret;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_set_voltage(mmc->supply.vqmmc, iov, iov);
-		if (ret) {
+		/* Pick the right voltage to allow 3.0V for 3.3V nominal PBIAS */
+		ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);
+		if (ret < 0) {
 			dev_err(mmc_dev(mmc), "vqmmc set voltage failed\n");
 			return ret;
 		}
 	}
 
-	ret = sdhci_omap_set_pbias(omap_host, true, iov);
+	ret = sdhci_omap_set_pbias(omap_host, true, iov_pbias);
 	if (ret)
 		return ret;
 
@@ -206,16 +207,28 @@ static int sdhci_omap_enable_iov(struct sdhci_omap_host *omap_host,
 static void sdhci_omap_conf_bus_power(struct sdhci_omap_host *omap_host,
 				      unsigned char signal_voltage)
 {
-	u32 reg;
+	u32 reg, capa;
 	ktime_t timeout;
 
 	reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_HCTL);
 	reg &= ~HCTL_SDVS_MASK;
 
-	if (signal_voltage == MMC_SIGNAL_VOLTAGE_330)
-		reg |= HCTL_SDVS_33;
-	else
+	switch (signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+		capa = sdhci_omap_readl(omap_host, SDHCI_OMAP_CAPA);
+		if (capa & CAPA_VS33)
+			reg |= HCTL_SDVS_33;
+		else if (capa & CAPA_VS30)
+			reg |= HCTL_SDVS_30;
+		else
+			dev_warn(omap_host->dev, "misconfigured CAPA: %08x\n",
+				 capa);
+		break;
+	case MMC_SIGNAL_VOLTAGE_180:
+	default:
 		reg |= HCTL_SDVS_18;
+		break;
+	}
 
 	sdhci_omap_writel(omap_host, SDHCI_OMAP_HCTL, reg);
 
@@ -533,28 +546,32 @@ static int sdhci_omap_start_signal_voltage_switch(struct mmc_host *mmc,
 
 	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
 		reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CAPA);
-		if (!(reg & CAPA_VS33))
+		if (!(reg & (CAPA_VS30 | CAPA_VS33)))
 			return -EOPNOTSUPP;
 
+		if (reg & CAPA_VS30)
+			iov = IOV_3V0;
+		else
+			iov = IOV_3V3;
+
 		sdhci_omap_conf_bus_power(omap_host, ios->signal_voltage);
 
 		reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_AC12);
 		reg &= ~AC12_V1V8_SIGEN;
 		sdhci_omap_writel(omap_host, SDHCI_OMAP_AC12, reg);
 
-		iov = IOV_3V3;
 	} else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
 		reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CAPA);
 		if (!(reg & CAPA_VS18))
 			return -EOPNOTSUPP;
 
+		iov = IOV_1V8;
+
 		sdhci_omap_conf_bus_power(omap_host, ios->signal_voltage);
 
 		reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_AC12);
 		reg |= AC12_V1V8_SIGEN;
 		sdhci_omap_writel(omap_host, SDHCI_OMAP_AC12, reg);
-
-		iov = IOV_1V8;
 	} else {
 		return -EOPNOTSUPP;
 	}
@@ -910,34 +927,73 @@ static struct sdhci_ops sdhci_omap_ops = {
 	.set_timeout = sdhci_omap_set_timeout,
 };
 
-static int sdhci_omap_set_capabilities(struct sdhci_omap_host *omap_host)
+static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
+						  const char *name)
 {
-	u32 reg;
-	int ret = 0;
+	struct regulator *reg;
+	unsigned int caps = 0;
+
+	reg = regulator_get(dev, name);
+	if (IS_ERR(reg))
+		return ~0UL;
+
+	if (regulator_is_supported_voltage(reg, 1700000, 1950000))
+		caps |= SDHCI_CAN_VDD_180;
+	if (regulator_is_supported_voltage(reg, 2700000, 3150000))
+		caps |= SDHCI_CAN_VDD_300;
+	if (regulator_is_supported_voltage(reg, 3150000, 3600000))
+		caps |= SDHCI_CAN_VDD_330;
+
+	regulator_put(reg);
+
+	return caps;
+}
+
+static int sdhci_omap_set_capabilities(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 	struct device *dev = omap_host->dev;
-	struct regulator *vqmmc;
+	const u32 mask = SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_300 | SDHCI_CAN_VDD_330;
+	unsigned int pbias, vqmmc, caps = 0;
+	u32 reg;
 
-	vqmmc = regulator_get(dev, "vqmmc");
-	if (IS_ERR(vqmmc)) {
-		ret = PTR_ERR(vqmmc);
-		goto reg_put;
-	}
+	pbias = sdhci_omap_regulator_get_caps(dev, "pbias");
+	vqmmc = sdhci_omap_regulator_get_caps(dev, "vqmmc");
+	caps = pbias & vqmmc;
+
+	if (pbias != ~0UL && vqmmc == ~0UL)
+		dev_warn(dev, "vqmmc regulator missing for pbias\n");
+	else if (caps == ~0UL)
+		return 0;
+
+	/*
+	 * Quirk handling to allow 3.0V vqmmc with a valid 3.3V PBIAS. This is
+	 * needed for 3.0V ldo9_reg on omap5 at least.
+	 */
+	if (pbias != ~0UL && (pbias & SDHCI_CAN_VDD_330) &&
+	    (vqmmc & SDHCI_CAN_VDD_300))
+		caps |= SDHCI_CAN_VDD_330;
 
 	/* voltage capabilities might be set by boot loader, clear it */
 	reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CAPA);
 	reg &= ~(CAPA_VS18 | CAPA_VS30 | CAPA_VS33);
 
-	if (regulator_is_supported_voltage(vqmmc, IOV_3V3, IOV_3V3))
-		reg |= CAPA_VS33;
-	if (regulator_is_supported_voltage(vqmmc, IOV_1V8, IOV_1V8))
+	if (caps & SDHCI_CAN_VDD_180)
 		reg |= CAPA_VS18;
 
+	if (caps & SDHCI_CAN_VDD_300)
+		reg |= CAPA_VS30;
+
+	if (caps & SDHCI_CAN_VDD_330)
+		reg |= CAPA_VS33;
+
 	sdhci_omap_writel(omap_host, SDHCI_OMAP_CAPA, reg);
 
-reg_put:
-	regulator_put(vqmmc);
+	host->caps &= ~mask;
+	host->caps |= caps;
 
-	return ret;
+	return 0;
 }
 
 static const struct sdhci_pltfm_data sdhci_omap_pdata = {
@@ -953,6 +1009,16 @@ static const struct sdhci_pltfm_data sdhci_omap_pdata = {
 	.ops = &sdhci_omap_ops,
 };
 
+static const struct sdhci_omap_data omap4_data = {
+	.offset = 0x200,
+	.flags = SDHCI_OMAP_SPECIAL_RESET,
+};
+
+static const struct sdhci_omap_data omap5_data = {
+	.offset = 0x200,
+	.flags = SDHCI_OMAP_SPECIAL_RESET,
+};
+
 static const struct sdhci_omap_data k2g_data = {
 	.offset = 0x200,
 };
@@ -973,6 +1039,8 @@ static const struct sdhci_omap_data dra7_data = {
 };
 
 static const struct of_device_id omap_sdhci_match[] = {
+	{ .compatible = "ti,omap4-sdhci", .data = &omap4_data },
+	{ .compatible = "ti,omap5-sdhci", .data = &omap5_data },
 	{ .compatible = "ti,dra7-sdhci", .data = &dra7_data },
 	{ .compatible = "ti,k2g-sdhci", .data = &k2g_data },
 	{ .compatible = "ti,am335-sdhci", .data = &am335_data },
@@ -1212,7 +1280,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 		goto err_rpm_disable;
 	}
 
-	ret = sdhci_omap_set_capabilities(omap_host);
+	ret = sdhci_omap_set_capabilities(host);
 	if (ret) {
 		dev_err(dev, "failed to set system capabilities\n");
 		goto err_put_sync;
-- 
2.33.0

[PATCH 3/5] mmc: sdhci-omap: Add omap_offset to support omap3 and earlier

[Tony Lindgren]

The omap specific registers are at offset 0x100 from base for omap4 and
later, and for omap3 and earlier they are at offset 0. Let's handle also
the earlier SoCs by adding omap_offset.

Note that eventually we should just move to using standard sdhci register
access for the sdhci range with new offsets starting at 0x100.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mmc/host/sdhci-omap.c | 61 ++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -21,9 +21,14 @@
 
 #include "sdhci-pltfm.h"
 
-#define SDHCI_OMAP_SYSCONFIG	0x110
+/*
+ * Note that the register offsets used here are from omap_regs
+ * base which is 0x100 for omap4 and later, and 0 for omap3 and
+ * earlier.
+ */
+#define SDHCI_OMAP_SYSCONFIG	0x10
 
-#define SDHCI_OMAP_CON		0x12c
+#define SDHCI_OMAP_CON		0x2c
 #define CON_DW8			BIT(5)
 #define CON_DMA_MASTER		BIT(20)
 #define CON_DDR			BIT(19)
@@ -33,20 +38,20 @@
 #define CON_INIT		BIT(1)
 #define CON_OD			BIT(0)
 
-#define SDHCI_OMAP_DLL		0x0134
+#define SDHCI_OMAP_DLL		0x34
 #define DLL_SWT			BIT(20)
 #define DLL_FORCE_SR_C_SHIFT	13
 #define DLL_FORCE_SR_C_MASK	(0x7f << DLL_FORCE_SR_C_SHIFT)
 #define DLL_FORCE_VALUE		BIT(12)
 #define DLL_CALIB		BIT(1)
 
-#define SDHCI_OMAP_CMD		0x20c
+#define SDHCI_OMAP_CMD		0x10c
 
-#define SDHCI_OMAP_PSTATE	0x0224
+#define SDHCI_OMAP_PSTATE	0x124
 #define PSTATE_DLEV_DAT0	BIT(20)
 #define PSTATE_DATI		BIT(1)
 
-#define SDHCI_OMAP_HCTL		0x228
+#define SDHCI_OMAP_HCTL		0x128
 #define HCTL_SDBP		BIT(8)
 #define HCTL_SDVS_SHIFT		9
 #define HCTL_SDVS_MASK		(0x7 << HCTL_SDVS_SHIFT)
@@ -54,28 +59,28 @@
 #define HCTL_SDVS_30		(0x6 << HCTL_SDVS_SHIFT)
 #define HCTL_SDVS_18		(0x5 << HCTL_SDVS_SHIFT)
 
-#define SDHCI_OMAP_SYSCTL	0x22c
+#define SDHCI_OMAP_SYSCTL	0x12c
 #define SYSCTL_CEN		BIT(2)
 #define SYSCTL_CLKD_SHIFT	6
 #define SYSCTL_CLKD_MASK	0x3ff
 
-#define SDHCI_OMAP_STAT		0x230
+#define SDHCI_OMAP_STAT		0x130
 
-#define SDHCI_OMAP_IE		0x234
+#define SDHCI_OMAP_IE		0x134
 #define INT_CC_EN		BIT(0)
 
-#define SDHCI_OMAP_ISE		0x238
+#define SDHCI_OMAP_ISE		0x138
 
-#define SDHCI_OMAP_AC12		0x23c
+#define SDHCI_OMAP_AC12		0x13c
 #define AC12_V1V8_SIGEN		BIT(19)
 #define AC12_SCLK_SEL		BIT(23)
 
-#define SDHCI_OMAP_CAPA		0x240
+#define SDHCI_OMAP_CAPA		0x140
 #define CAPA_VS33		BIT(24)
 #define CAPA_VS30		BIT(25)
 #define CAPA_VS18		BIT(26)
 
-#define SDHCI_OMAP_CAPA2	0x0244
+#define SDHCI_OMAP_CAPA2	0x144
 #define CAPA2_TSDR50		BIT(13)
 
 #define SDHCI_OMAP_TIMEOUT	1		/* 1 msec */
@@ -93,7 +98,8 @@
 #define SDHCI_OMAP_SPECIAL_RESET	BIT(1)
 
 struct sdhci_omap_data {
-	u32 offset;
+	int omap_offset;	/* Offset for omap regs from base */
+	u32 offset;		/* Offset for SDHCI regs from base */
 	u8 flags;
 };
 
@@ -112,6 +118,10 @@ struct sdhci_omap_host {
 	struct pinctrl		*pinctrl;
 	struct pinctrl_state	**pinctrl_state;
 	bool			is_tuning;
+
+	/* Offset for omap specific registers from base */
+	int			omap_offset;
+
 	/* Omap specific context save */
 	u32			con;
 	u32			hctl;
@@ -127,13 +137,13 @@ static void sdhci_omap_stop_clock(struct sdhci_omap_host *omap_host);
 static inline u32 sdhci_omap_readl(struct sdhci_omap_host *host,
 				   unsigned int offset)
 {
-	return readl(host->base + offset);
+	return readl(host->base + host->omap_offset + offset);
 }
 
 static inline void sdhci_omap_writel(struct sdhci_omap_host *host,
 				     unsigned int offset, u32 data)
 {
-	writel(data, host->base + offset);
+	writel(data, host->base + host->omap_offset + offset);
 }
 
 static int sdhci_omap_set_pbias(struct sdhci_omap_host *omap_host,
@@ -1009,36 +1019,54 @@ static const struct sdhci_pltfm_data sdhci_omap_pdata = {
 	.ops = &sdhci_omap_ops,
 };
 
+static const struct sdhci_omap_data omap2430_data = {
+	.omap_offset = 0,
+	.offset = 0x100,
+};
+
+static const struct sdhci_omap_data omap3_data = {
+	.omap_offset = 0,
+	.offset = 0x100,
+};
+
 static const struct sdhci_omap_data omap4_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 	.flags = SDHCI_OMAP_SPECIAL_RESET,
 };
 
 static const struct sdhci_omap_data omap5_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 	.flags = SDHCI_OMAP_SPECIAL_RESET,
 };
 
 static const struct sdhci_omap_data k2g_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 };
 
 static const struct sdhci_omap_data am335_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 	.flags = SDHCI_OMAP_SPECIAL_RESET,
 };
 
 static const struct sdhci_omap_data am437_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 	.flags = SDHCI_OMAP_SPECIAL_RESET,
 };
 
 static const struct sdhci_omap_data dra7_data = {
+	.omap_offset = 0x100,
 	.offset = 0x200,
 	.flags	= SDHCI_OMAP_REQUIRE_IODELAY,
 };
 
 static const struct of_device_id omap_sdhci_match[] = {
+	{ .compatible = "ti,omap2430-sdhci", .data = &omap2430_data },
+	{ .compatible = "ti,omap3-sdhci", .data = &omap3_data },
 	{ .compatible = "ti,omap4-sdhci", .data = &omap4_data },
 	{ .compatible = "ti,omap5-sdhci", .data = &omap5_data },
 	{ .compatible = "ti,dra7-sdhci", .data = &dra7_data },
@@ -1223,6 +1251,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	omap_host->power_mode = MMC_POWER_UNDEFINED;
 	omap_host->timing = MMC_TIMING_LEGACY;
 	omap_host->flags = data->flags;
+	omap_host->omap_offset = data->omap_offset;
 	host->ioaddr += offset;
 	host->mapbase = regs->start + offset;
 
-- 
2.33.0

[PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren]

Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.

Note that we save context in probe to avoid restoring invalid context
on the first resume. For system suspend, we have the new PM runtime
functions do most of the work.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mmc/host/sdhci-omap.c | 66 +++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -117,6 +117,9 @@ struct sdhci_omap_host {
 
 	struct pinctrl		*pinctrl;
 	struct pinctrl_state	**pinctrl_state;
+	unsigned long		context_valid:1;
+	unsigned long		is_runtime_suspended:1;
+	unsigned long		needs_resume:1;
 	bool			is_tuning;
 
 	/* Offset for omap specific registers from base */
@@ -1207,6 +1210,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = {
 	}
 };
 
+static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host);
+
 static int sdhci_omap_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -1338,6 +1343,8 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	/* R1B responses is required to properly manage HW busy detection. */
 	mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
 
+	mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
+
 	ret = sdhci_setup_host(host);
 	if (ret)
 		goto err_put_sync;
@@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup_host;
 
+	sdhci_omap_context_save(omap_host);
+	omap_host->context_valid = 1;
+
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_cleanup_host:
@@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(dev);
 	sdhci_remove_host(host, true);
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
@@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
 	sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
 }
 
-static int __maybe_unused sdhci_omap_suspend(struct device *dev)
+static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 
-	sdhci_suspend_host(host);
-
 	sdhci_omap_context_save(omap_host);
 
 	pinctrl_pm_select_idle_state(dev);
 
-	pm_runtime_force_suspend(dev);
+	omap_host->is_runtime_suspended = 1;
 
 	return 0;
 }
 
-static int __maybe_unused sdhci_omap_resume(struct device *dev)
+static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 
-	pm_runtime_force_resume(dev);
-
 	pinctrl_pm_select_default_state(dev);
 
-	sdhci_omap_context_restore(omap_host);
+	if (omap_host->context_valid)
+		sdhci_omap_context_restore(omap_host);
+
+	omap_host->is_runtime_suspended = 0;
+
+	return 0;
+}
+
+static int __maybe_unused sdhci_omap_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (omap_host->is_runtime_suspended)
+		return 0;
+
+	sdhci_suspend_host(host);
+	sdhci_omap_runtime_suspend(dev);
+	omap_host->needs_resume = 1;
 
+	return 0;
+}
+
+static int __maybe_unused sdhci_omap_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (!omap_host->needs_resume)
+		return 0;
+
+	sdhci_omap_runtime_resume(dev);
 	sdhci_resume_host(host);
+	omap_host->needs_resume = 0;
 
 	return 0;
 }
 #endif
-static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend,
-			 sdhci_omap_resume);
+
+static const struct dev_pm_ops sdhci_omap_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
+			   sdhci_omap_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume)
+};
 
 static struct platform_driver sdhci_omap_driver = {
 	.probe = sdhci_omap_probe,
-- 
2.33.0

[PATCH 5/5] mmc: sdhci-omap: Configure optional wakeirq

[Tony Lindgren]

Configure optional wakeirq. This may be optionally configured for SDIO
dat1 pin for wake-up events for SoCs that support deeper idle states.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mmc/host/sdhci-omap.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -12,8 +12,10 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/sys_soc.h>
@@ -117,6 +119,7 @@ struct sdhci_omap_host {
 
 	struct pinctrl		*pinctrl;
 	struct pinctrl_state	**pinctrl_state;
+	int			wakeirq;
 	unsigned long		context_valid:1;
 	unsigned long		is_runtime_suspended:1;
 	unsigned long		needs_resume:1;
@@ -1360,6 +1363,21 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	sdhci_omap_context_save(omap_host);
 	omap_host->context_valid = 1;
 
+	of_irq_get_byname(dev->of_node, "wakeup");
+	if (omap_host->wakeirq == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto err_cleanup_host;
+	}
+	if (omap_host->wakeirq > 0) {
+		device_init_wakeup(dev, true);
+		ret = dev_pm_set_dedicated_wake_irq(dev, omap_host->wakeirq);
+		if (ret) {
+			device_init_wakeup(dev, false);
+			goto err_cleanup_host;
+		}
+		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+	}
+
 	pm_runtime_put_sync(dev);
 
 	return 0;
@@ -1385,6 +1403,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(dev);
 	sdhci_remove_host(host, true);
+	dev_pm_clear_wake_irq(dev);
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 	sdhci_pltfm_free(pdev);
-- 
2.33.0

Re: [PATCH 5/5] mmc: sdhci-omap: Configure optional wakeirq

[Grygorii Strashko]

On 21/09/2021 14:16, Tony Lindgren wrote:
> Configure optional wakeirq. This may be optionally configured for SDIO
> dat1 pin for wake-up events for SoCs that support deeper idle states.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/mmc/host/sdhci-omap.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -12,8 +12,10 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/pinctrl/consumer.h>
>   #include <linux/sys_soc.h>
> @@ -117,6 +119,7 @@ struct sdhci_omap_host {
>   
>   	struct pinctrl		*pinctrl;
>   	struct pinctrl_state	**pinctrl_state;
> +	int			wakeirq;
>   	unsigned long		context_valid:1;
>   	unsigned long		is_runtime_suspended:1;
>   	unsigned long		needs_resume:1;
> @@ -1360,6 +1363,21 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>   	sdhci_omap_context_save(omap_host);
>   	omap_host->context_valid = 1;
>   
> +	of_irq_get_byname(dev->of_node, "wakeup");

No assign to omap_host->wakeirq

> +	if (omap_host->wakeirq == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto err_cleanup_host;
> +	}
> +	if (omap_host->wakeirq > 0) {
> +		device_init_wakeup(dev, true);
> +		ret = dev_pm_set_dedicated_wake_irq(dev, omap_host->wakeirq);
> +		if (ret) {
> +			device_init_wakeup(dev, false);
> +			goto err_cleanup_host;
> +		}
> +		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
> +	}
> +
>   	pm_runtime_put_sync(dev);
>   
>   	return 0;
> @@ -1385,6 +1403,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
>   
>   	pm_runtime_get_sync(dev);
>   	sdhci_remove_host(host, true);
> +	dev_pm_clear_wake_irq(dev);
>   	pm_runtime_put_sync(dev);
>   	pm_runtime_disable(dev);
>   	sdhci_pltfm_free(pdev);
> 

-- 
Best regards,
grygorii

Re: [PATCH 5/5] mmc: sdhci-omap: Configure optional wakeirq

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [210922 15:23]:
> > @@ -1360,6 +1363,21 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> >   	sdhci_omap_context_save(omap_host);
> >   	omap_host->context_valid = 1;
> > +	of_irq_get_byname(dev->of_node, "wakeup");
> 
> No assign to omap_host->wakeirq

Oops. Thanks for spotting it, will fix.

Regards,

Tony

Re: [PATCH 1/5] dt-bindings: sdhci-omap: Update binding for legacy SoCs

[Rob Herring]

On Tue, 21 Sep 2021 14:15:56 +0300, Tony Lindgren wrote:
> Let's add compatible values for the legacy SoCs so we can continue
> deprecating omap_hsmmc in favor of sdhci-omap driver.
> 
> For omap5, we want to have a separate compatible from omap4 for the
> additional features available on omap5. AFAIK ti81 can just use the
> omap4 compatible.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/5] mmc: sdhci-omap: Configure optional wakeirq

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [210923 05:34]:
> * Grygorii Strashko <grygorii.strashko@ti.com> [210922 15:23]:
> > > @@ -1360,6 +1363,21 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > >   	sdhci_omap_context_save(omap_host);
> > >   	omap_host->context_valid = 1;
> > > +	of_irq_get_byname(dev->of_node, "wakeup");
> > 
> > No assign to omap_host->wakeirq
> 
> Oops. Thanks for spotting it, will fix.

Ah I forgot that wlcore does not use the dat1 interrupt but has a separate
gpio interrupt for wake-up. And that's why grep wake /proc/interrupts was
increasing for me.. I'll ad a comment about that and will also test with
mwifiex_sdio device I have.

Regards,

Tony

Re: [PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [211012 09:07]:
> On Mon, 11 Oct 2021 at 07:23, Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Ulf Hansson <ulf.hansson@linaro.org> [211008 14:44]:
> > > On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@atomide.com> wrote:
> > > >
> > > > Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.
> > >
> > > I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
> > > is about enabling runtime PM management for the eMMC/SD card device,
> > > which is perfectly fine to use independently of whether runtime PM is
> > > supported for the host device.
> >
> > OK
> >
> > > > @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > > >         if (ret)
> > > >                 goto err_cleanup_host;
> > > >
> > > > +       sdhci_omap_context_save(omap_host);
> > > > +       omap_host->context_valid = 1;
> > >
> > > Looks like you can remove this flag, it's not being used.
> >
> > Hmm I think it is needed as otherwise we end up trying to restore
> > an invalid context on probe on the first pm_runtime_get(). Do you
> > have some nicer solution for that in mind?
> 
> Right, I didn't notice that, my apologies.
> 
> In any case, an option is to bring the device into full power, without
> calling pm_runtime_resume_and_get() from ->probe(). In principle,
> running the same operations as the ->runtime_resume() callback does,
> except for restoring the context then. When this is done, the
> following calls to runtime PM should do the trick (I extended it to
> support autosuspend as well):
> 
> pm_runtime_get_noresume()
> pm_runtime_set_active()
> pm_runtime_set_autosuspend_delay()
> pm_runtime_use_autosuspend()
> pm_runtime_enable()
> 
> Note that, this means that the omaps PM domain's ->runtime_resume()
> callback doesn't get invoked when powering on the device for the first
> time. Can this be a problem?

Yeah I think that would be a problem as the register access won't work :)

I changed things around to initialize omap_host->con = -ENODEV and then
check it on resume. Not an ideal solution but avoids the extra flag.

> > > > @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
> > > >         sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
> > > >  }
> > > >
> > > > -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > > > +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
> > > >  {
> > > >         struct sdhci_host *host = dev_get_drvdata(dev);
> > > >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > >         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> > > >
> > > > -       sdhci_suspend_host(host);
> > > > -
> > >
> > > Shouldn't you call sdhci_runtime_suspend_host() somewhere here?
> >
> > I'm pretty sure I tried, but runtime resume did not seem to work after
> > doing that.. I'll take a look again.

Confusion solved for this one and it's working now.. Maybe I tried
calling sdhci_suspend_host() instead of sdhci_runtime_suspend_host()
earlier who knows.

> > > It looks a bit odd that sdhci_suspend_host() is called only when the
> > > host is runtime resumed. Perhaps you can elaborate a bit more on why
> > > this is, so I can understand better what you want to achieve here.
> >
> > I guess I'm not clear on what's left for sdhci_suspend_host() to do if
> > the host is already runtime suspended :)
> 
> I think what boils down to that is that, sdhci_suspend|resume_host()
> adds some special treatment for system wakeups (for SDIO irqs). I am
> not sure whether you may need that.

OK, will check.

> Some host drivers doesn't use sdhci_suspend|resume_host, but sticks to
> the sdhci_runtime_suspend|resume()_host() functions. Like
> sdhci-sprd.c, for example.

Hmm so is there some helper to figure out if the mmc host is active
and has a card?

Seems we could skip sdhci_suspend/resume for the inactive mmc host
instances.

Regards,

Tony

Re: [PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Ulf Hansson]

On Mon, 11 Oct 2021 at 07:23, Tony Lindgren <tony@atomide.com> wrote:
>
> * Ulf Hansson <ulf.hansson@linaro.org> [211008 14:44]:
> > On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.
> >
> > I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
> > is about enabling runtime PM management for the eMMC/SD card device,
> > which is perfectly fine to use independently of whether runtime PM is
> > supported for the host device.
>
> OK
>
> > > @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 goto err_cleanup_host;
> > >
> > > +       sdhci_omap_context_save(omap_host);
> > > +       omap_host->context_valid = 1;
> >
> > Looks like you can remove this flag, it's not being used.
>
> Hmm I think it is needed as otherwise we end up trying to restore
> an invalid context on probe on the first pm_runtime_get(). Do you
> have some nicer solution for that in mind?

Right, I didn't notice that, my apologies.

In any case, an option is to bring the device into full power, without
calling pm_runtime_resume_and_get() from ->probe(). In principle,
running the same operations as the ->runtime_resume() callback does,
except for restoring the context then. When this is done, the
following calls to runtime PM should do the trick (I extended it to
support autosuspend as well):

pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_set_autosuspend_delay()
pm_runtime_use_autosuspend()
pm_runtime_enable()

Note that, this means that the omaps PM domain's ->runtime_resume()
callback doesn't get invoked when powering on the device for the first
time. Can this be a problem?

>
> > > +
> > > +       pm_runtime_put_sync(dev);
> >
> > I recommend to use the PM runtime autosuspend feature, as to avoid an
> > initial latency for every I/O request to the host driver. The mmc core
> > already supports that, see mmc_release_host().
> >
> > The typical default timeout value for autosuspend, is usually set
> > ~50-200ms, by host drivers (if I recall correctly).
>
> OK I have a patch to also enable autosuspend too, I'll add that
> too for the next revision.
>
> > > @@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
> > >         struct device *dev = &pdev->dev;
> > >         struct sdhci_host *host = platform_get_drvdata(pdev);
> > >
> > > +       pm_runtime_get_sync(dev);
> > >         sdhci_remove_host(host, true);
> > >         pm_runtime_put_sync(dev);
> >
> > There is no guarantee that this triggers a call to
> > ->sdhci_omap_runtime_suspend(), which I guess is what we want.
> > Userspace via sysfs may have increase the RPM usage count
> > (pm_runtime_forbid(), for example.
> >
> > To address this, I would call pm_runtime_disable() first and then
> > explicitly put the device into low power state, rather than relying on
> > runtime PM to do it. Another option could be to use
> > pm_runtime_force_suspend().
>
> OK I'll take a look.
>
> > > @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
> > >         sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
> > >  }
> > >
> > > -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > > +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
> > >  {
> > >         struct sdhci_host *host = dev_get_drvdata(dev);
> > >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > >         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> > >
> > > -       sdhci_suspend_host(host);
> > > -
> >
> > Shouldn't you call sdhci_runtime_suspend_host() somewhere here?
>
> I'm pretty sure I tried, but runtime resume did not seem to work after
> doing that.. I'll take a look again.
>
> > > +static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > > +{
> > > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +       struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> > > +
> > > +       if (omap_host->is_runtime_suspended)
> > > +               return 0;
> >
> > So if the host is already runtime suspended, it's okay to just leave it as is?
>
> Ideally yeah there should not be anything left to do for suspesnd at
> that point. But sounds like I may be missing something.
>
> > In a way that sounds like you could call pm_runtime_force_suspend()
> > instead, assuming the sdhci_omap_runtime_suspend() can be extended to
> > do the right thing for system suspend as well.
>
> OK I'll check.
>
> > It looks a bit odd that sdhci_suspend_host() is called only when the
> > host is runtime resumed. Perhaps you can elaborate a bit more on why
> > this is, so I can understand better what you want to achieve here.
>
> I guess I'm not clear on what's left for sdhci_suspend_host() to do if
> the host is already runtime suspended :)

I think what boils down to that is that, sdhci_suspend|resume_host()
adds some special treatment for system wakeups (for SDIO irqs). I am
not sure whether you may need that.

Some host drivers doesn't use sdhci_suspend|resume_host, but sticks to
the sdhci_runtime_suspend|resume()_host() functions. Like
sdhci-sprd.c, for example.

Kind regards
Uffe

Re: [PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [211008 14:44]:
> On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.
> 
> I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
> is about enabling runtime PM management for the eMMC/SD card device,
> which is perfectly fine to use independently of whether runtime PM is
> supported for the host device.

OK

> > @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto err_cleanup_host;
> >
> > +       sdhci_omap_context_save(omap_host);
> > +       omap_host->context_valid = 1;
> 
> Looks like you can remove this flag, it's not being used.

Hmm I think it is needed as otherwise we end up trying to restore
an invalid context on probe on the first pm_runtime_get(). Do you
have some nicer solution for that in mind?

> > +
> > +       pm_runtime_put_sync(dev);
> 
> I recommend to use the PM runtime autosuspend feature, as to avoid an
> initial latency for every I/O request to the host driver. The mmc core
> already supports that, see mmc_release_host().
> 
> The typical default timeout value for autosuspend, is usually set
> ~50-200ms, by host drivers (if I recall correctly).

OK I have a patch to also enable autosuspend too, I'll add that
too for the next revision.

> > @@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
> >         struct device *dev = &pdev->dev;
> >         struct sdhci_host *host = platform_get_drvdata(pdev);
> >
> > +       pm_runtime_get_sync(dev);
> >         sdhci_remove_host(host, true);
> >         pm_runtime_put_sync(dev);
> 
> There is no guarantee that this triggers a call to
> ->sdhci_omap_runtime_suspend(), which I guess is what we want.
> Userspace via sysfs may have increase the RPM usage count
> (pm_runtime_forbid(), for example.
> 
> To address this, I would call pm_runtime_disable() first and then
> explicitly put the device into low power state, rather than relying on
> runtime PM to do it. Another option could be to use
> pm_runtime_force_suspend().

OK I'll take a look.

> > @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
> >         sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
> >  }
> >
> > -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
> >  {
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> >
> > -       sdhci_suspend_host(host);
> > -
> 
> Shouldn't you call sdhci_runtime_suspend_host() somewhere here?

I'm pretty sure I tried, but runtime resume did not seem to work after
doing that.. I'll take a look again.

> > +static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +       struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +       if (omap_host->is_runtime_suspended)
> > +               return 0;
> 
> So if the host is already runtime suspended, it's okay to just leave it as is?

Ideally yeah there should not be anything left to do for suspesnd at
that point. But sounds like I may be missing something.

> In a way that sounds like you could call pm_runtime_force_suspend()
> instead, assuming the sdhci_omap_runtime_suspend() can be extended to
> do the right thing for system suspend as well.

OK I'll check.

> It looks a bit odd that sdhci_suspend_host() is called only when the
> host is runtime resumed. Perhaps you can elaborate a bit more on why
> this is, so I can understand better what you want to achieve here.

I guess I'm not clear on what's left for sdhci_suspend_host() to do if
the host is already runtime suspended :)

Regards,

Tony

Re: [PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Ulf Hansson]

On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@atomide.com> wrote:
>
> Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.

I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
is about enabling runtime PM management for the eMMC/SD card device,
which is perfectly fine to use independently of whether runtime PM is
supported for the host device.

>
> Note that we save context in probe to avoid restoring invalid context
> on the first resume. For system suspend, we have the new PM runtime
> functions do most of the work.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mmc/host/sdhci-omap.c | 66 +++++++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -117,6 +117,9 @@ struct sdhci_omap_host {
>
>         struct pinctrl          *pinctrl;
>         struct pinctrl_state    **pinctrl_state;
> +       unsigned long           context_valid:1;
> +       unsigned long           is_runtime_suspended:1;
> +       unsigned long           needs_resume:1;
>         bool                    is_tuning;
>
>         /* Offset for omap specific registers from base */
> @@ -1207,6 +1210,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = {
>         }
>  };
>
> +static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host);
> +
>  static int sdhci_omap_probe(struct platform_device *pdev)
>  {
>         int ret;
> @@ -1338,6 +1343,8 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>         /* R1B responses is required to properly manage HW busy detection. */
>         mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
>
> +       mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
> +
>         ret = sdhci_setup_host(host);
>         if (ret)
>                 goto err_put_sync;
> @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err_cleanup_host;
>
> +       sdhci_omap_context_save(omap_host);
> +       omap_host->context_valid = 1;

Looks like you can remove this flag, it's not being used.

> +
> +       pm_runtime_put_sync(dev);

I recommend to use the PM runtime autosuspend feature, as to avoid an
initial latency for every I/O request to the host driver. The mmc core
already supports that, see mmc_release_host().

The typical default timeout value for autosuspend, is usually set
~50-200ms, by host drivers (if I recall correctly).

> +
>         return 0;
>
>  err_cleanup_host:
> @@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct sdhci_host *host = platform_get_drvdata(pdev);
>
> +       pm_runtime_get_sync(dev);
>         sdhci_remove_host(host, true);
>         pm_runtime_put_sync(dev);

There is no guarantee that this triggers a call to
->sdhci_omap_runtime_suspend(), which I guess is what we want.
Userspace via sysfs may have increase the RPM usage count
(pm_runtime_forbid(), for example.

To address this, I would call pm_runtime_disable() first and then
explicitly put the device into low power state, rather than relying on
runtime PM to do it. Another option could be to use
pm_runtime_force_suspend().

>         pm_runtime_disable(dev);
> @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
>         sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
>  }
>
> -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
>  {
>         struct sdhci_host *host = dev_get_drvdata(dev);
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>
> -       sdhci_suspend_host(host);
> -

Shouldn't you call sdhci_runtime_suspend_host() somewhere here?

>         sdhci_omap_context_save(omap_host);
>
>         pinctrl_pm_select_idle_state(dev);
>
> -       pm_runtime_force_suspend(dev);
> +       omap_host->is_runtime_suspended = 1;
>
>         return 0;
>  }
>
> -static int __maybe_unused sdhci_omap_resume(struct device *dev)
> +static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev)
>  {
>         struct sdhci_host *host = dev_get_drvdata(dev);
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>
> -       pm_runtime_force_resume(dev);
> -
>         pinctrl_pm_select_default_state(dev);
>
> -       sdhci_omap_context_restore(omap_host);
> +       if (omap_host->context_valid)
> +               sdhci_omap_context_restore(omap_host);
> +
> +       omap_host->is_runtime_suspended = 0;

Shouldn't you call sdhci_runtime_resume_host() somewhere here?

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (omap_host->is_runtime_suspended)
> +               return 0;

So if the host is already runtime suspended, it's okay to just leave it as is?

In a way that sounds like you could call pm_runtime_force_suspend()
instead, assuming the sdhci_omap_runtime_suspend() can be extended to
do the right thing for system suspend as well.

It looks a bit odd that sdhci_suspend_host() is called only when the
host is runtime resumed. Perhaps you can elaborate a bit more on why
this is, so I can understand better what you want to achieve here.

> +
> +       sdhci_suspend_host(host);
> +       sdhci_omap_runtime_suspend(dev);
> +       omap_host->needs_resume = 1;
>
> +       return 0;
> +}
> +
> +static int __maybe_unused sdhci_omap_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (!omap_host->needs_resume)
> +               return 0;
> +
> +       sdhci_omap_runtime_resume(dev);
>         sdhci_resume_host(host);
> +       omap_host->needs_resume = 0;
>
>         return 0;
>  }
>  #endif
> -static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend,
> -                        sdhci_omap_resume);
> +
> +static const struct dev_pm_ops sdhci_omap_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
> +                          sdhci_omap_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume)
> +};
>
>  static struct platform_driver sdhci_omap_driver = {
>         .probe = sdhci_omap_probe,
> --
> 2.33.0

Kind regards
Uffe

[PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren]

Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.

Note that we save context in probe to avoid restoring invalid context
on the first resume. For system suspend, we have the new PM runtime
functions do most of the work.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mmc/host/sdhci-omap.c | 66 +++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -117,6 +117,9 @@ struct sdhci_omap_host {
 
 	struct pinctrl		*pinctrl;
 	struct pinctrl_state	**pinctrl_state;
+	unsigned long		context_valid:1;
+	unsigned long		is_runtime_suspended:1;
+	unsigned long		needs_resume:1;
 	bool			is_tuning;
 
 	/* Offset for omap specific registers from base */
@@ -1207,6 +1210,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = {
 	}
 };
 
+static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host);
+
 static int sdhci_omap_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -1338,6 +1343,8 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	/* R1B responses is required to properly manage HW busy detection. */
 	mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
 
+	mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
+
 	ret = sdhci_setup_host(host);
 	if (ret)
 		goto err_put_sync;
@@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup_host;
 
+	sdhci_omap_context_save(omap_host);
+	omap_host->context_valid = 1;
+
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_cleanup_host:
@@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(dev);
 	sdhci_remove_host(host, true);
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
@@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
 	sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
 }
 
-static int __maybe_unused sdhci_omap_suspend(struct device *dev)
+static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 
-	sdhci_suspend_host(host);
-
 	sdhci_omap_context_save(omap_host);
 
 	pinctrl_pm_select_idle_state(dev);
 
-	pm_runtime_force_suspend(dev);
+	omap_host->is_runtime_suspended = 1;
 
 	return 0;
 }
 
-static int __maybe_unused sdhci_omap_resume(struct device *dev)
+static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 
-	pm_runtime_force_resume(dev);
-
 	pinctrl_pm_select_default_state(dev);
 
-	sdhci_omap_context_restore(omap_host);
+	if (omap_host->context_valid)
+		sdhci_omap_context_restore(omap_host);
+
+	omap_host->is_runtime_suspended = 0;
+
+	return 0;
+}
+
+static int __maybe_unused sdhci_omap_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (omap_host->is_runtime_suspended)
+		return 0;
+
+	sdhci_suspend_host(host);
+	sdhci_omap_runtime_suspend(dev);
+	omap_host->needs_resume = 1;
 
+	return 0;
+}
+
+static int __maybe_unused sdhci_omap_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (!omap_host->needs_resume)
+		return 0;
+
+	sdhci_omap_runtime_resume(dev);
 	sdhci_resume_host(host);
+	omap_host->needs_resume = 0;
 
 	return 0;
 }
 #endif
-static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend,
-			 sdhci_omap_resume);
+
+static const struct dev_pm_ops sdhci_omap_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
+			   sdhci_omap_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume)
+};
 
 static struct platform_driver sdhci_omap_driver = {
 	.probe = sdhci_omap_probe,
-- 
2.33.0