* [PATCH v3 0/3] Some features and fix for sdhci-of-dwcmshc
@ 2023-02-02 0:35 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
This patchset has 3 patches. The first one is a signal improvement
actually. Then we add runtime time support and hsq support which have
been tested in vendor kernel for a long time.
Changes in v2:
- fix Kconfig error
Shawn Lin (3):
mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip
platform
mmc: sdhci-of-dwcmshc: Add runtime PM support
mmc: sdhci-of-dwcmshc: Add host software queue support
drivers/mmc/host/Kconfig | 1 +
drivers/mmc/host/sdhci-of-dwcmshc.c | 93 ++++++++++++++++++++++++++++++++++---
2 files changed, 88 insertions(+), 6 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 0/3] Some features and fix for sdhci-of-dwcmshc
@ 2023-02-02 0:35 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
This patchset has 3 patches. The first one is a signal improvement
actually. Then we add runtime time support and hsq support which have
been tested in vendor kernel for a long time.
Changes in v2:
- fix Kconfig error
Shawn Lin (3):
mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip
platform
mmc: sdhci-of-dwcmshc: Add runtime PM support
mmc: sdhci-of-dwcmshc: Add host software queue support
drivers/mmc/host/Kconfig | 1 +
drivers/mmc/host/sdhci-of-dwcmshc.c | 93 ++++++++++++++++++++++++++++++++++---
2 files changed, 88 insertions(+), 6 deletions(-)
--
2.7.4
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/3] mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip platform
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-02 0:35 ` Shawn Lin
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
For Rockchip platform, DLL bypass bit and start bit need to be set if
DLL is not locked. And adjust pre-change delay to 0x3 for better signal
test result.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/sdhci-of-dwcmshc.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 4933867..46b1ce7 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -48,6 +48,7 @@
#define DWCMSHC_EMMC_DLL_RXCLK_SRCSEL 29
#define DWCMSHC_EMMC_DLL_START_POINT 16
#define DWCMSHC_EMMC_DLL_INC 8
+#define DWCMSHC_EMMC_DLL_BYPASS BIT(24)
#define DWCMSHC_EMMC_DLL_DLYENA BIT(27)
#define DLL_TXCLK_TAPNUM_DEFAULT 0x10
#define DLL_TXCLK_TAPNUM_90_DEGREES 0xA
@@ -60,6 +61,7 @@
#define DLL_RXCLK_NO_INVERTER 1
#define DLL_RXCLK_INVERTER 0
#define DLL_CMDOUT_TAPNUM_90_DEGREES 0x8
+#define DLL_RXCLK_ORI_GATE BIT(31)
#define DLL_CMDOUT_TAPNUM_FROM_SW BIT(24)
#define DLL_CMDOUT_SRC_CLK_NEG BIT(28)
#define DLL_CMDOUT_EN_SRC_CLK_NEG BIT(29)
@@ -234,9 +236,12 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
sdhci_writel(host, extra, reg);
if (clock <= 52000000) {
- /* Disable DLL and reset both of sample and drive clock */
- sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CTRL);
- sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_RXCLK);
+ /*
+ * Disable DLL and reset both of sample and drive clock.
+ * The bypass bit and start bit need to be set if DLL is not locked.
+ */
+ sdhci_writel(host, DWCMSHC_EMMC_DLL_BYPASS | DWCMSHC_EMMC_DLL_START, DWCMSHC_EMMC_DLL_CTRL);
+ sdhci_writel(host, DLL_RXCLK_ORI_GATE, DWCMSHC_EMMC_DLL_RXCLK);
sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
sdhci_writel(host, 0, DECMSHC_EMMC_DLL_CMDOUT);
/*
@@ -279,7 +284,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
}
extra = 0x1 << 16 | /* tune clock stop en */
- 0x2 << 17 | /* pre-change delay */
+ 0x3 << 17 | /* pre-change delay */
0x3 << 19; /* post-change delay */
sdhci_writel(host, extra, dwc_priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 1/3] mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip platform
@ 2023-02-02 0:35 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
For Rockchip platform, DLL bypass bit and start bit need to be set if
DLL is not locked. And adjust pre-change delay to 0x3 for better signal
test result.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/sdhci-of-dwcmshc.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 4933867..46b1ce7 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -48,6 +48,7 @@
#define DWCMSHC_EMMC_DLL_RXCLK_SRCSEL 29
#define DWCMSHC_EMMC_DLL_START_POINT 16
#define DWCMSHC_EMMC_DLL_INC 8
+#define DWCMSHC_EMMC_DLL_BYPASS BIT(24)
#define DWCMSHC_EMMC_DLL_DLYENA BIT(27)
#define DLL_TXCLK_TAPNUM_DEFAULT 0x10
#define DLL_TXCLK_TAPNUM_90_DEGREES 0xA
@@ -60,6 +61,7 @@
#define DLL_RXCLK_NO_INVERTER 1
#define DLL_RXCLK_INVERTER 0
#define DLL_CMDOUT_TAPNUM_90_DEGREES 0x8
+#define DLL_RXCLK_ORI_GATE BIT(31)
#define DLL_CMDOUT_TAPNUM_FROM_SW BIT(24)
#define DLL_CMDOUT_SRC_CLK_NEG BIT(28)
#define DLL_CMDOUT_EN_SRC_CLK_NEG BIT(29)
@@ -234,9 +236,12 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
sdhci_writel(host, extra, reg);
if (clock <= 52000000) {
- /* Disable DLL and reset both of sample and drive clock */
- sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CTRL);
- sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_RXCLK);
+ /*
+ * Disable DLL and reset both of sample and drive clock.
+ * The bypass bit and start bit need to be set if DLL is not locked.
+ */
+ sdhci_writel(host, DWCMSHC_EMMC_DLL_BYPASS | DWCMSHC_EMMC_DLL_START, DWCMSHC_EMMC_DLL_CTRL);
+ sdhci_writel(host, DLL_RXCLK_ORI_GATE, DWCMSHC_EMMC_DLL_RXCLK);
sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
sdhci_writel(host, 0, DECMSHC_EMMC_DLL_CMDOUT);
/*
@@ -279,7 +284,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
}
extra = 0x1 << 16 | /* tune clock stop en */
- 0x2 << 17 | /* pre-change delay */
+ 0x3 << 17 | /* pre-change delay */
0x3 << 19; /* post-change delay */
sdhci_writel(host, extra, dwc_priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
--
2.7.4
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-02 0:35 ` Shawn Lin
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
This patch adds runtime PM support.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 46b1ce7..fc917ed 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/sizes.h>
@@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
if (err)
goto err_setup_host;
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_put_autosuspend(&pdev->dev);
+
return 0;
err_setup_host:
@@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
if (rk_priv)
clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
rk_priv->rockchip_clks);
+
+ pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
sdhci_pltfm_free(pdev);
return 0;
@@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ u16 data;
+ int ret;
+
+ ret = sdhci_runtime_suspend_host(host);
+ if (ret)
+ return ret;
+
+ data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ data &= ~SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+ return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ u16 data;
+
+ data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ data |= SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+ return sdhci_runtime_resume_host(host, 0);
+}
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
+ dwcmshc_resume)
+ SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+ dwcmshc_runtime_resume, NULL)
+};
static struct platform_driver sdhci_dwcmshc_driver = {
.driver = {
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-02 0:35 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
This patch adds runtime PM support.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 46b1ce7..fc917ed 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/sizes.h>
@@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
if (err)
goto err_setup_host;
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_put_autosuspend(&pdev->dev);
+
return 0;
err_setup_host:
@@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
if (rk_priv)
clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
rk_priv->rockchip_clks);
+
+ pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
sdhci_pltfm_free(pdev);
return 0;
@@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ u16 data;
+ int ret;
+
+ ret = sdhci_runtime_suspend_host(host);
+ if (ret)
+ return ret;
+
+ data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ data &= ~SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+ return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ u16 data;
+
+ data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ data |= SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+ return sdhci_runtime_resume_host(host, 0);
+}
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
+ dwcmshc_resume)
+ SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+ dwcmshc_runtime_resume, NULL)
+};
static struct platform_driver sdhci_dwcmshc_driver = {
.driver = {
--
2.7.4
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] mmc: sdhci-of-dwcmshc: Add host software queue support
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-02 0:35 ` Shawn Lin
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- fix Kconfig error
drivers/mmc/host/Kconfig | 1 +
drivers/mmc/host/sdhci-of-dwcmshc.c | 29 ++++++++++++++++++++++++++++-
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index e96b302..e060bab 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -233,6 +233,7 @@ config MMC_SDHCI_OF_DWCMSHC
depends on MMC_SDHCI_PLTFM
depends on OF
depends on COMMON_CLK
+ select MMC_HSQ
help
This selects Synopsys DesignWare Cores Mobile Storage Controller
support.
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index fc917ed..e90fa69 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -20,6 +20,7 @@
#include <linux/sizes.h>
#include "sdhci-pltfm.h"
+#include "mmc_hsq.h"
#define SDHCI_DWCMSHC_ARG2_STUFF GENMASK(31, 16)
@@ -331,6 +332,14 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
sdhci_reset(host, mask);
}
+static void sdhci_dwcmshc_request_done(struct sdhci_host *host, struct mmc_request *mrq)
+{
+ if (mmc_hsq_finalize_request(host->mmc, mrq))
+ return;
+
+ mmc_request_done(host->mmc, mrq);
+}
+
static const struct sdhci_ops sdhci_dwcmshc_ops = {
.set_clock = sdhci_set_clock,
.set_bus_width = sdhci_set_bus_width,
@@ -347,6 +356,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
.get_max_clock = sdhci_pltfm_clk_get_max_clock,
.reset = rk35xx_sdhci_reset,
.adma_write_desc = dwcmshc_adma_write_desc,
+ .request_done = sdhci_dwcmshc_request_done,
};
static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
@@ -462,6 +472,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
struct dwcmshc_priv *priv;
struct rk35xx_priv *rk_priv = NULL;
const struct sdhci_pltfm_data *pltfm_data;
+ struct mmc_hsq *hsq;
int err;
u32 extra;
@@ -515,6 +526,16 @@ static int dwcmshc_probe(struct platform_device *pdev)
host->mmc_host_ops.request = dwcmshc_request;
host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
+ hsq = devm_kzalloc(&pdev->dev, sizeof(*hsq), GFP_KERNEL);
+ if (!hsq) {
+ err = -ENOMEM;
+ goto err_clk;
+ }
+
+ err = mmc_hsq_init(hsq, host->mmc);
+ if (err)
+ goto err_clk;
+
if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
if (!rk_priv) {
@@ -607,6 +628,8 @@ static int dwcmshc_suspend(struct device *dev)
struct rk35xx_priv *rk_priv = priv->priv;
int ret;
+ mmc_hsq_suspend(host->mmc);
+
ret = sdhci_suspend_host(host);
if (ret)
return ret;
@@ -647,7 +670,11 @@ static int dwcmshc_resume(struct device *dev)
return ret;
}
- return sdhci_resume_host(host);
+ ret = sdhci_resume_host(host);
+ if (ret)
+ return ret;
+
+ return mmc_hsq_resume(host->mmc);
}
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] mmc: sdhci-of-dwcmshc: Add host software queue support
@ 2023-02-02 0:35 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-02 0:35 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Shawn Lin
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- fix Kconfig error
drivers/mmc/host/Kconfig | 1 +
drivers/mmc/host/sdhci-of-dwcmshc.c | 29 ++++++++++++++++++++++++++++-
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index e96b302..e060bab 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -233,6 +233,7 @@ config MMC_SDHCI_OF_DWCMSHC
depends on MMC_SDHCI_PLTFM
depends on OF
depends on COMMON_CLK
+ select MMC_HSQ
help
This selects Synopsys DesignWare Cores Mobile Storage Controller
support.
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index fc917ed..e90fa69 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -20,6 +20,7 @@
#include <linux/sizes.h>
#include "sdhci-pltfm.h"
+#include "mmc_hsq.h"
#define SDHCI_DWCMSHC_ARG2_STUFF GENMASK(31, 16)
@@ -331,6 +332,14 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
sdhci_reset(host, mask);
}
+static void sdhci_dwcmshc_request_done(struct sdhci_host *host, struct mmc_request *mrq)
+{
+ if (mmc_hsq_finalize_request(host->mmc, mrq))
+ return;
+
+ mmc_request_done(host->mmc, mrq);
+}
+
static const struct sdhci_ops sdhci_dwcmshc_ops = {
.set_clock = sdhci_set_clock,
.set_bus_width = sdhci_set_bus_width,
@@ -347,6 +356,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
.get_max_clock = sdhci_pltfm_clk_get_max_clock,
.reset = rk35xx_sdhci_reset,
.adma_write_desc = dwcmshc_adma_write_desc,
+ .request_done = sdhci_dwcmshc_request_done,
};
static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
@@ -462,6 +472,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
struct dwcmshc_priv *priv;
struct rk35xx_priv *rk_priv = NULL;
const struct sdhci_pltfm_data *pltfm_data;
+ struct mmc_hsq *hsq;
int err;
u32 extra;
@@ -515,6 +526,16 @@ static int dwcmshc_probe(struct platform_device *pdev)
host->mmc_host_ops.request = dwcmshc_request;
host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
+ hsq = devm_kzalloc(&pdev->dev, sizeof(*hsq), GFP_KERNEL);
+ if (!hsq) {
+ err = -ENOMEM;
+ goto err_clk;
+ }
+
+ err = mmc_hsq_init(hsq, host->mmc);
+ if (err)
+ goto err_clk;
+
if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
if (!rk_priv) {
@@ -607,6 +628,8 @@ static int dwcmshc_suspend(struct device *dev)
struct rk35xx_priv *rk_priv = priv->priv;
int ret;
+ mmc_hsq_suspend(host->mmc);
+
ret = sdhci_suspend_host(host);
if (ret)
return ret;
@@ -647,7 +670,11 @@ static int dwcmshc_resume(struct device *dev)
return ret;
}
- return sdhci_resume_host(host);
+ ret = sdhci_resume_host(host);
+ if (ret)
+ return ret;
+
+ return mmc_hsq_resume(host->mmc);
}
#endif
--
2.7.4
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-13 23:45 ` Ulf Hansson
-1 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-13 23:45 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> This patch adds runtime PM support.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 46b1ce7..fc917ed 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/reset.h>
> #include <linux/sizes.h>
>
> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> if (err)
> goto err_setup_host;
>
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_put_autosuspend(&pdev->dev);
> +
> return 0;
>
> err_setup_host:
> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> if (rk_priv)
> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> rk_priv->rockchip_clks);
> +
> + pm_runtime_get_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> sdhci_pltfm_free(pdev);
>
> return 0;
> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> }
> #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + u16 data;
> + int ret;
> +
> + ret = sdhci_runtime_suspend_host(host);
> + if (ret)
> + return ret;
> +
> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> + data &= ~SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> + return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + u16 data;
> +
> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> + data |= SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> + return sdhci_runtime_resume_host(host, 0);
> +}
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> + dwcmshc_resume)
I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
As sdhci_suspend_host() will write to internal registers of the IP
block, it's recommended to make sure the device's runtime resumed
before doing that call. So that needs to be added along with $subject
patch.
There is also another option that may better for you, which is to use
the pm_runtime_force_suspend|resume() helpers. There should be plenty
of references to look at, but don't hesitate to ask around that, if
you need some more help to get that working.
> + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> + dwcmshc_runtime_resume, NULL)
> +};
>
> static struct platform_driver sdhci_dwcmshc_driver = {
> .driver = {
Kind regards
Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-13 23:45 ` Ulf Hansson
0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-13 23:45 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> This patch adds runtime PM support.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 46b1ce7..fc917ed 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/reset.h>
> #include <linux/sizes.h>
>
> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> if (err)
> goto err_setup_host;
>
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_put_autosuspend(&pdev->dev);
> +
> return 0;
>
> err_setup_host:
> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> if (rk_priv)
> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> rk_priv->rockchip_clks);
> +
> + pm_runtime_get_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> sdhci_pltfm_free(pdev);
>
> return 0;
> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> }
> #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + u16 data;
> + int ret;
> +
> + ret = sdhci_runtime_suspend_host(host);
> + if (ret)
> + return ret;
> +
> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> + data &= ~SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> + return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + u16 data;
> +
> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> + data |= SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> + return sdhci_runtime_resume_host(host, 0);
> +}
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> + dwcmshc_resume)
I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
As sdhci_suspend_host() will write to internal registers of the IP
block, it's recommended to make sure the device's runtime resumed
before doing that call. So that needs to be added along with $subject
patch.
There is also another option that may better for you, which is to use
the pm_runtime_force_suspend|resume() helpers. There should be plenty
of references to look at, but don't hesitate to ask around that, if
you need some more help to get that working.
> + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> + dwcmshc_runtime_resume, NULL)
> +};
>
> static struct platform_driver sdhci_dwcmshc_driver = {
> .driver = {
Kind regards
Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip platform
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-13 23:48 ` Ulf Hansson
-1 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-13 23:48 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> For Rockchip platform, DLL bypass bit and start bit need to be set if
> DLL is not locked. And adjust pre-change delay to 0x3 for better signal
> test result.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Applied for next, thanks!
Kind regards
Uffe
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/sdhci-of-dwcmshc.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 4933867..46b1ce7 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -48,6 +48,7 @@
> #define DWCMSHC_EMMC_DLL_RXCLK_SRCSEL 29
> #define DWCMSHC_EMMC_DLL_START_POINT 16
> #define DWCMSHC_EMMC_DLL_INC 8
> +#define DWCMSHC_EMMC_DLL_BYPASS BIT(24)
> #define DWCMSHC_EMMC_DLL_DLYENA BIT(27)
> #define DLL_TXCLK_TAPNUM_DEFAULT 0x10
> #define DLL_TXCLK_TAPNUM_90_DEGREES 0xA
> @@ -60,6 +61,7 @@
> #define DLL_RXCLK_NO_INVERTER 1
> #define DLL_RXCLK_INVERTER 0
> #define DLL_CMDOUT_TAPNUM_90_DEGREES 0x8
> +#define DLL_RXCLK_ORI_GATE BIT(31)
> #define DLL_CMDOUT_TAPNUM_FROM_SW BIT(24)
> #define DLL_CMDOUT_SRC_CLK_NEG BIT(28)
> #define DLL_CMDOUT_EN_SRC_CLK_NEG BIT(29)
> @@ -234,9 +236,12 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> sdhci_writel(host, extra, reg);
>
> if (clock <= 52000000) {
> - /* Disable DLL and reset both of sample and drive clock */
> - sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CTRL);
> - sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_RXCLK);
> + /*
> + * Disable DLL and reset both of sample and drive clock.
> + * The bypass bit and start bit need to be set if DLL is not locked.
> + */
> + sdhci_writel(host, DWCMSHC_EMMC_DLL_BYPASS | DWCMSHC_EMMC_DLL_START, DWCMSHC_EMMC_DLL_CTRL);
> + sdhci_writel(host, DLL_RXCLK_ORI_GATE, DWCMSHC_EMMC_DLL_RXCLK);
> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> sdhci_writel(host, 0, DECMSHC_EMMC_DLL_CMDOUT);
> /*
> @@ -279,7 +284,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> }
>
> extra = 0x1 << 16 | /* tune clock stop en */
> - 0x2 << 17 | /* pre-change delay */
> + 0x3 << 17 | /* pre-change delay */
> 0x3 << 19; /* post-change delay */
> sdhci_writel(host, extra, dwc_priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip platform
@ 2023-02-13 23:48 ` Ulf Hansson
0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-13 23:48 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> For Rockchip platform, DLL bypass bit and start bit need to be set if
> DLL is not locked. And adjust pre-change delay to 0x3 for better signal
> test result.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Applied for next, thanks!
Kind regards
Uffe
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/sdhci-of-dwcmshc.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 4933867..46b1ce7 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -48,6 +48,7 @@
> #define DWCMSHC_EMMC_DLL_RXCLK_SRCSEL 29
> #define DWCMSHC_EMMC_DLL_START_POINT 16
> #define DWCMSHC_EMMC_DLL_INC 8
> +#define DWCMSHC_EMMC_DLL_BYPASS BIT(24)
> #define DWCMSHC_EMMC_DLL_DLYENA BIT(27)
> #define DLL_TXCLK_TAPNUM_DEFAULT 0x10
> #define DLL_TXCLK_TAPNUM_90_DEGREES 0xA
> @@ -60,6 +61,7 @@
> #define DLL_RXCLK_NO_INVERTER 1
> #define DLL_RXCLK_INVERTER 0
> #define DLL_CMDOUT_TAPNUM_90_DEGREES 0x8
> +#define DLL_RXCLK_ORI_GATE BIT(31)
> #define DLL_CMDOUT_TAPNUM_FROM_SW BIT(24)
> #define DLL_CMDOUT_SRC_CLK_NEG BIT(28)
> #define DLL_CMDOUT_EN_SRC_CLK_NEG BIT(29)
> @@ -234,9 +236,12 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> sdhci_writel(host, extra, reg);
>
> if (clock <= 52000000) {
> - /* Disable DLL and reset both of sample and drive clock */
> - sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CTRL);
> - sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_RXCLK);
> + /*
> + * Disable DLL and reset both of sample and drive clock.
> + * The bypass bit and start bit need to be set if DLL is not locked.
> + */
> + sdhci_writel(host, DWCMSHC_EMMC_DLL_BYPASS | DWCMSHC_EMMC_DLL_START, DWCMSHC_EMMC_DLL_CTRL);
> + sdhci_writel(host, DLL_RXCLK_ORI_GATE, DWCMSHC_EMMC_DLL_RXCLK);
> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> sdhci_writel(host, 0, DECMSHC_EMMC_DLL_CMDOUT);
> /*
> @@ -279,7 +284,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
> }
>
> extra = 0x1 << 16 | /* tune clock stop en */
> - 0x2 << 17 | /* pre-change delay */
> + 0x3 << 17 | /* pre-change delay */
> 0x3 << 19; /* post-change delay */
> sdhci_writel(host, extra, dwc_priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
>
> --
> 2.7.4
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-13 23:45 ` Ulf Hansson
@ 2023-02-14 3:36 ` Shawn Lin
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-14 3:36 UTC (permalink / raw)
To: Ulf Hansson; +Cc: shawn.lin, linux-mmc, Adrian Hunter, linux-rockchip
Hi Ulf,
On 2023/2/14 7:45, Ulf Hansson wrote:
> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> This patch adds runtime PM support.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 46b1ce7..fc917ed 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -15,6 +15,7 @@
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/reset.h>
>> #include <linux/sizes.h>
>>
>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>> if (err)
>> goto err_setup_host;
>>
>> + pm_runtime_get_noresume(&pdev->dev);
>> + pm_runtime_set_active(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_runtime_put_autosuspend(&pdev->dev);
>> +
>> return 0;
>>
>> err_setup_host:
>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>> if (rk_priv)
>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> rk_priv->rockchip_clks);
>> +
>> + pm_runtime_get_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_put_noidle(&pdev->dev);
>> +
>> sdhci_pltfm_free(pdev);
>>
>> return 0;
>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>> }
>> #endif
>>
>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>> +#ifdef CONFIG_PM
>> +static int dwcmshc_runtime_suspend(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + u16 data;
>> + int ret;
>> +
>> + ret = sdhci_runtime_suspend_host(host);
>> + if (ret)
>> + return ret;
>> +
>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> + data &= ~SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> + return 0;
>> +}
>> +
>> +static int dwcmshc_runtime_resume(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + u16 data;
>> +
>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> + data |= SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> + return sdhci_runtime_resume_host(host, 0);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops dwcmshc_pmops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>> + dwcmshc_resume)
>
> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> As sdhci_suspend_host() will write to internal registers of the IP
> block, it's recommended to make sure the device's runtime resumed
> before doing that call. So that needs to be added along with $subject
> patch.
>
Yep, let me add a check here.
> There is also another option that may better for you, which is to use
> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> of references to look at, but don't hesitate to ask around that, if
> you need some more help to get that working.
If I understand correctly, pm_runtime_force_suspend|resume() helpers
would use runtime PM ops for suspend/resume routine. In this case, the
main issue is the handling of clock is quite different. In suspend we
need to gate all clocks but in rpm case, it shouldn't.
>
>> + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
>> + dwcmshc_runtime_resume, NULL)
>> +};
>>
>> static struct platform_driver sdhci_dwcmshc_driver = {
>> .driver = {
>
> Kind regards
> Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-14 3:36 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-14 3:36 UTC (permalink / raw)
To: Ulf Hansson; +Cc: shawn.lin, linux-mmc, Adrian Hunter, linux-rockchip
Hi Ulf,
On 2023/2/14 7:45, Ulf Hansson wrote:
> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> This patch adds runtime PM support.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 46b1ce7..fc917ed 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -15,6 +15,7 @@
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/reset.h>
>> #include <linux/sizes.h>
>>
>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>> if (err)
>> goto err_setup_host;
>>
>> + pm_runtime_get_noresume(&pdev->dev);
>> + pm_runtime_set_active(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_runtime_put_autosuspend(&pdev->dev);
>> +
>> return 0;
>>
>> err_setup_host:
>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>> if (rk_priv)
>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> rk_priv->rockchip_clks);
>> +
>> + pm_runtime_get_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> + pm_runtime_put_noidle(&pdev->dev);
>> +
>> sdhci_pltfm_free(pdev);
>>
>> return 0;
>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>> }
>> #endif
>>
>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>> +#ifdef CONFIG_PM
>> +static int dwcmshc_runtime_suspend(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + u16 data;
>> + int ret;
>> +
>> + ret = sdhci_runtime_suspend_host(host);
>> + if (ret)
>> + return ret;
>> +
>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> + data &= ~SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> + return 0;
>> +}
>> +
>> +static int dwcmshc_runtime_resume(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + u16 data;
>> +
>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> + data |= SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> + return sdhci_runtime_resume_host(host, 0);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops dwcmshc_pmops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>> + dwcmshc_resume)
>
> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> As sdhci_suspend_host() will write to internal registers of the IP
> block, it's recommended to make sure the device's runtime resumed
> before doing that call. So that needs to be added along with $subject
> patch.
>
Yep, let me add a check here.
> There is also another option that may better for you, which is to use
> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> of references to look at, but don't hesitate to ask around that, if
> you need some more help to get that working.
If I understand correctly, pm_runtime_force_suspend|resume() helpers
would use runtime PM ops for suspend/resume routine. In this case, the
main issue is the handling of clock is quite different. In suspend we
need to gate all clocks but in rpm case, it shouldn't.
>
>> + SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
>> + dwcmshc_runtime_resume, NULL)
>> +};
>>
>> static struct platform_driver sdhci_dwcmshc_driver = {
>> .driver = {
>
> Kind regards
> Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-14 3:36 ` Shawn Lin
@ 2023-02-14 10:41 ` Ulf Hansson
-1 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-14 10:41 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf,
>
> On 2023/2/14 7:45, Ulf Hansson wrote:
> > On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> This patch adds runtime PM support.
> >>
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> ---
> >>
> >> Changes in v2: None
> >>
> >> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> index 46b1ce7..fc917ed 100644
> >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> @@ -15,6 +15,7 @@
> >> #include <linux/module.h>
> >> #include <linux/of.h>
> >> #include <linux/of_device.h>
> >> +#include <linux/pm_runtime.h>
> >> #include <linux/reset.h>
> >> #include <linux/sizes.h>
> >>
> >> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >> if (err)
> >> goto err_setup_host;
> >>
> >> + pm_runtime_get_noresume(&pdev->dev);
> >> + pm_runtime_set_active(&pdev->dev);
> >> + pm_runtime_enable(&pdev->dev);
> >> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >> + pm_runtime_use_autosuspend(&pdev->dev);
> >> + pm_runtime_put_autosuspend(&pdev->dev);
> >> +
> >> return 0;
> >>
> >> err_setup_host:
> >> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >> if (rk_priv)
> >> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >> rk_priv->rockchip_clks);
> >> +
> >> + pm_runtime_get_sync(&pdev->dev);
> >> + pm_runtime_disable(&pdev->dev);
> >> + pm_runtime_put_noidle(&pdev->dev);
> >> +
> >> sdhci_pltfm_free(pdev);
> >>
> >> return 0;
> >> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >> }
> >> #endif
> >>
> >> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >> +#ifdef CONFIG_PM
> >> +static int dwcmshc_runtime_suspend(struct device *dev)
> >> +{
> >> + struct sdhci_host *host = dev_get_drvdata(dev);
> >> + u16 data;
> >> + int ret;
> >> +
> >> + ret = sdhci_runtime_suspend_host(host);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> + data &= ~SDHCI_CLOCK_CARD_EN;
> >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int dwcmshc_runtime_resume(struct device *dev)
> >> +{
> >> + struct sdhci_host *host = dev_get_drvdata(dev);
> >> + u16 data;
> >> +
> >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> + data |= SDHCI_CLOCK_CARD_EN;
> >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> + return sdhci_runtime_resume_host(host, 0);
> >> +}
> >> +#endif
> >> +
> >> +static const struct dev_pm_ops dwcmshc_pmops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >> + dwcmshc_resume)
> >
> > I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> > As sdhci_suspend_host() will write to internal registers of the IP
> > block, it's recommended to make sure the device's runtime resumed
> > before doing that call. So that needs to be added along with $subject
> > patch.
> >
>
> Yep, let me add a check here.
>
> > There is also another option that may better for you, which is to use
> > the pm_runtime_force_suspend|resume() helpers. There should be plenty
> > of references to look at, but don't hesitate to ask around that, if
> > you need some more help to get that working.
>
> If I understand correctly, pm_runtime_force_suspend|resume() helpers
> would use runtime PM ops for suspend/resume routine. In this case, the
> main issue is the handling of clock is quite different. In suspend we
> need to gate all clocks but in rpm case, it shouldn't.
I see, but let me then ask, what's the point of keeping the clocks
ungated at runtime suspend?
That sounds like wasting energy to me - but maybe there are good reasons for it?
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-14 10:41 ` Ulf Hansson
0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-14 10:41 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf,
>
> On 2023/2/14 7:45, Ulf Hansson wrote:
> > On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> This patch adds runtime PM support.
> >>
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> ---
> >>
> >> Changes in v2: None
> >>
> >> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> index 46b1ce7..fc917ed 100644
> >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> @@ -15,6 +15,7 @@
> >> #include <linux/module.h>
> >> #include <linux/of.h>
> >> #include <linux/of_device.h>
> >> +#include <linux/pm_runtime.h>
> >> #include <linux/reset.h>
> >> #include <linux/sizes.h>
> >>
> >> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >> if (err)
> >> goto err_setup_host;
> >>
> >> + pm_runtime_get_noresume(&pdev->dev);
> >> + pm_runtime_set_active(&pdev->dev);
> >> + pm_runtime_enable(&pdev->dev);
> >> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >> + pm_runtime_use_autosuspend(&pdev->dev);
> >> + pm_runtime_put_autosuspend(&pdev->dev);
> >> +
> >> return 0;
> >>
> >> err_setup_host:
> >> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >> if (rk_priv)
> >> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >> rk_priv->rockchip_clks);
> >> +
> >> + pm_runtime_get_sync(&pdev->dev);
> >> + pm_runtime_disable(&pdev->dev);
> >> + pm_runtime_put_noidle(&pdev->dev);
> >> +
> >> sdhci_pltfm_free(pdev);
> >>
> >> return 0;
> >> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >> }
> >> #endif
> >>
> >> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >> +#ifdef CONFIG_PM
> >> +static int dwcmshc_runtime_suspend(struct device *dev)
> >> +{
> >> + struct sdhci_host *host = dev_get_drvdata(dev);
> >> + u16 data;
> >> + int ret;
> >> +
> >> + ret = sdhci_runtime_suspend_host(host);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> + data &= ~SDHCI_CLOCK_CARD_EN;
> >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int dwcmshc_runtime_resume(struct device *dev)
> >> +{
> >> + struct sdhci_host *host = dev_get_drvdata(dev);
> >> + u16 data;
> >> +
> >> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> + data |= SDHCI_CLOCK_CARD_EN;
> >> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> + return sdhci_runtime_resume_host(host, 0);
> >> +}
> >> +#endif
> >> +
> >> +static const struct dev_pm_ops dwcmshc_pmops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >> + dwcmshc_resume)
> >
> > I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> > As sdhci_suspend_host() will write to internal registers of the IP
> > block, it's recommended to make sure the device's runtime resumed
> > before doing that call. So that needs to be added along with $subject
> > patch.
> >
>
> Yep, let me add a check here.
>
> > There is also another option that may better for you, which is to use
> > the pm_runtime_force_suspend|resume() helpers. There should be plenty
> > of references to look at, but don't hesitate to ask around that, if
> > you need some more help to get that working.
>
> If I understand correctly, pm_runtime_force_suspend|resume() helpers
> would use runtime PM ops for suspend/resume routine. In this case, the
> main issue is the handling of clock is quite different. In suspend we
> need to gate all clocks but in rpm case, it shouldn't.
I see, but let me then ask, what's the point of keeping the clocks
ungated at runtime suspend?
That sounds like wasting energy to me - but maybe there are good reasons for it?
[...]
Kind regards
Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] mmc: sdhci-of-dwcmshc: Add host software queue support
2023-02-02 0:35 ` Shawn Lin
@ 2023-02-14 11:31 ` Ulf Hansson
-1 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-14 11:31 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
Please elaborate a bit to share some more information to the commit message.
Perhaps you have some performance numbers too, that would be nice to
share in the commit message.
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - fix Kconfig error
>
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-dwcmshc.c | 29 ++++++++++++++++++++++++++++-
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index e96b302..e060bab 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -233,6 +233,7 @@ config MMC_SDHCI_OF_DWCMSHC
> depends on MMC_SDHCI_PLTFM
> depends on OF
> depends on COMMON_CLK
> + select MMC_HSQ
> help
> This selects Synopsys DesignWare Cores Mobile Storage Controller
> support.
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index fc917ed..e90fa69 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -20,6 +20,7 @@
> #include <linux/sizes.h>
>
> #include "sdhci-pltfm.h"
> +#include "mmc_hsq.h"
>
> #define SDHCI_DWCMSHC_ARG2_STUFF GENMASK(31, 16)
>
> @@ -331,6 +332,14 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> sdhci_reset(host, mask);
> }
>
> +static void sdhci_dwcmshc_request_done(struct sdhci_host *host, struct mmc_request *mrq)
> +{
> + if (mmc_hsq_finalize_request(host->mmc, mrq))
> + return;
> +
> + mmc_request_done(host->mmc, mrq);
> +}
> +
> static const struct sdhci_ops sdhci_dwcmshc_ops = {
> .set_clock = sdhci_set_clock,
> .set_bus_width = sdhci_set_bus_width,
> @@ -347,6 +356,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> .reset = rk35xx_sdhci_reset,
> .adma_write_desc = dwcmshc_adma_write_desc,
> + .request_done = sdhci_dwcmshc_request_done,
> };
>
> static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
> @@ -462,6 +472,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
> struct dwcmshc_priv *priv;
> struct rk35xx_priv *rk_priv = NULL;
> const struct sdhci_pltfm_data *pltfm_data;
> + struct mmc_hsq *hsq;
> int err;
> u32 extra;
>
> @@ -515,6 +526,16 @@ static int dwcmshc_probe(struct platform_device *pdev)
> host->mmc_host_ops.request = dwcmshc_request;
> host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
>
> + hsq = devm_kzalloc(&pdev->dev, sizeof(*hsq), GFP_KERNEL);
> + if (!hsq) {
> + err = -ENOMEM;
> + goto err_clk;
> + }
> +
> + err = mmc_hsq_init(hsq, host->mmc);
> + if (err)
> + goto err_clk;
> +
> if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
> rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
> if (!rk_priv) {
> @@ -607,6 +628,8 @@ static int dwcmshc_suspend(struct device *dev)
> struct rk35xx_priv *rk_priv = priv->priv;
> int ret;
>
> + mmc_hsq_suspend(host->mmc);
> +
> ret = sdhci_suspend_host(host);
> if (ret)
> return ret;
> @@ -647,7 +670,11 @@ static int dwcmshc_resume(struct device *dev)
> return ret;
> }
>
> - return sdhci_resume_host(host);
> + ret = sdhci_resume_host(host);
> + if (ret)
> + return ret;
> +
> + return mmc_hsq_resume(host->mmc);
> }
> #endif
If I understand correctly, you need to also inform sdhci and the mmc
core, whether you support atomic request management or not. For
sdhci-sprd, the following part below, is done during ->probe() - and I
assume we need something similar for the sdhci-of-dwcmshc, right?
if (!mmc_card_is_removable(host->mmc))
host->mmc_host_ops.request_atomic = sdhci_sprd_request_atomic;
else
host->always_defer_done = true;
Kind regards
Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] mmc: sdhci-of-dwcmshc: Add host software queue support
@ 2023-02-14 11:31 ` Ulf Hansson
0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-14 11:31 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
Please elaborate a bit to share some more information to the commit message.
Perhaps you have some performance numbers too, that would be nice to
share in the commit message.
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - fix Kconfig error
>
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-dwcmshc.c | 29 ++++++++++++++++++++++++++++-
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index e96b302..e060bab 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -233,6 +233,7 @@ config MMC_SDHCI_OF_DWCMSHC
> depends on MMC_SDHCI_PLTFM
> depends on OF
> depends on COMMON_CLK
> + select MMC_HSQ
> help
> This selects Synopsys DesignWare Cores Mobile Storage Controller
> support.
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index fc917ed..e90fa69 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -20,6 +20,7 @@
> #include <linux/sizes.h>
>
> #include "sdhci-pltfm.h"
> +#include "mmc_hsq.h"
>
> #define SDHCI_DWCMSHC_ARG2_STUFF GENMASK(31, 16)
>
> @@ -331,6 +332,14 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> sdhci_reset(host, mask);
> }
>
> +static void sdhci_dwcmshc_request_done(struct sdhci_host *host, struct mmc_request *mrq)
> +{
> + if (mmc_hsq_finalize_request(host->mmc, mrq))
> + return;
> +
> + mmc_request_done(host->mmc, mrq);
> +}
> +
> static const struct sdhci_ops sdhci_dwcmshc_ops = {
> .set_clock = sdhci_set_clock,
> .set_bus_width = sdhci_set_bus_width,
> @@ -347,6 +356,7 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> .reset = rk35xx_sdhci_reset,
> .adma_write_desc = dwcmshc_adma_write_desc,
> + .request_done = sdhci_dwcmshc_request_done,
> };
>
> static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
> @@ -462,6 +472,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
> struct dwcmshc_priv *priv;
> struct rk35xx_priv *rk_priv = NULL;
> const struct sdhci_pltfm_data *pltfm_data;
> + struct mmc_hsq *hsq;
> int err;
> u32 extra;
>
> @@ -515,6 +526,16 @@ static int dwcmshc_probe(struct platform_device *pdev)
> host->mmc_host_ops.request = dwcmshc_request;
> host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
>
> + hsq = devm_kzalloc(&pdev->dev, sizeof(*hsq), GFP_KERNEL);
> + if (!hsq) {
> + err = -ENOMEM;
> + goto err_clk;
> + }
> +
> + err = mmc_hsq_init(hsq, host->mmc);
> + if (err)
> + goto err_clk;
> +
> if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
> rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
> if (!rk_priv) {
> @@ -607,6 +628,8 @@ static int dwcmshc_suspend(struct device *dev)
> struct rk35xx_priv *rk_priv = priv->priv;
> int ret;
>
> + mmc_hsq_suspend(host->mmc);
> +
> ret = sdhci_suspend_host(host);
> if (ret)
> return ret;
> @@ -647,7 +670,11 @@ static int dwcmshc_resume(struct device *dev)
> return ret;
> }
>
> - return sdhci_resume_host(host);
> + ret = sdhci_resume_host(host);
> + if (ret)
> + return ret;
> +
> + return mmc_hsq_resume(host->mmc);
> }
> #endif
If I understand correctly, you need to also inform sdhci and the mmc
core, whether you support atomic request management or not. For
sdhci-sprd, the following part below, is done during ->probe() - and I
assume we need something similar for the sdhci-of-dwcmshc, right?
if (!mmc_card_is_removable(host->mmc))
host->mmc_host_ops.request_atomic = sdhci_sprd_request_atomic;
else
host->always_defer_done = true;
Kind regards
Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-14 10:41 ` Ulf Hansson
@ 2023-02-15 0:50 ` Shawn Lin
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-15 0:50 UTC (permalink / raw)
To: Ulf Hansson; +Cc: shawn.lin, linux-mmc, Adrian Hunter, linux-rockchip
Hi Ulf
On 2023/2/14 18:41, Ulf Hansson wrote:
> On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Ulf,
>>
>> On 2023/2/14 7:45, Ulf Hansson wrote:
>>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> This patch adds runtime PM support.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> index 46b1ce7..fc917ed 100644
>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> @@ -15,6 +15,7 @@
>>>> #include <linux/module.h>
>>>> #include <linux/of.h>
>>>> #include <linux/of_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/reset.h>
>>>> #include <linux/sizes.h>
>>>>
>>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>> if (err)
>>>> goto err_setup_host;
>>>>
>>>> + pm_runtime_get_noresume(&pdev->dev);
>>>> + pm_runtime_set_active(&pdev->dev);
>>>> + pm_runtime_enable(&pdev->dev);
>>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>>>> + pm_runtime_use_autosuspend(&pdev->dev);
>>>> + pm_runtime_put_autosuspend(&pdev->dev);
>>>> +
>>>> return 0;
>>>>
>>>> err_setup_host:
>>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>>>> if (rk_priv)
>>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>>>> rk_priv->rockchip_clks);
>>>> +
>>>> + pm_runtime_get_sync(&pdev->dev);
>>>> + pm_runtime_disable(&pdev->dev);
>>>> + pm_runtime_put_noidle(&pdev->dev);
>>>> +
>>>> sdhci_pltfm_free(pdev);
>>>>
>>>> return 0;
>>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>>>> }
>>>> #endif
>>>>
>>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>>>> +#ifdef CONFIG_PM
>>>> +static int dwcmshc_runtime_suspend(struct device *dev)
>>>> +{
>>>> + struct sdhci_host *host = dev_get_drvdata(dev);
>>>> + u16 data;
>>>> + int ret;
>>>> +
>>>> + ret = sdhci_runtime_suspend_host(host);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> + data &= ~SDHCI_CLOCK_CARD_EN;
>>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int dwcmshc_runtime_resume(struct device *dev)
>>>> +{
>>>> + struct sdhci_host *host = dev_get_drvdata(dev);
>>>> + u16 data;
>>>> +
>>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> + data |= SDHCI_CLOCK_CARD_EN;
>>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> + return sdhci_runtime_resume_host(host, 0);
>>>> +}
>>>> +#endif
>>>> +
>>>> +static const struct dev_pm_ops dwcmshc_pmops = {
>>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>>>> + dwcmshc_resume)
>>>
>>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
>>> As sdhci_suspend_host() will write to internal registers of the IP
>>> block, it's recommended to make sure the device's runtime resumed
>>> before doing that call. So that needs to be added along with $subject
>>> patch.
>>>
>>
>> Yep, let me add a check here.
>>
>>> There is also another option that may better for you, which is to use
>>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
>>> of references to look at, but don't hesitate to ask around that, if
>>> you need some more help to get that working.
>>
>> If I understand correctly, pm_runtime_force_suspend|resume() helpers
>> would use runtime PM ops for suspend/resume routine. In this case, the
>> main issue is the handling of clock is quite different. In suspend we
>> need to gate all clocks but in rpm case, it shouldn't.
>
> I see, but let me then ask, what's the point of keeping the clocks
> ungated at runtime suspend?
>
> That sounds like wasting energy to me - but maybe there are good reasons for it?
The point to keep the clocks ungated at runtime suspend is that if they
are gated, the DLL would lost its locked state which causes retuning
every time. It's quite painful for performance. However, if we just gate
output clock to the device, the devices(basically I refer to eMMC) will
get into power-save status by itself. That helps us too keep balance
between power & performance during runtime. ：）
>
> [...]
>
> Kind regards
> Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-15 0:50 ` Shawn Lin
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Lin @ 2023-02-15 0:50 UTC (permalink / raw)
To: Ulf Hansson; +Cc: shawn.lin, linux-mmc, Adrian Hunter, linux-rockchip
Hi Ulf
On 2023/2/14 18:41, Ulf Hansson wrote:
> On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Ulf,
>>
>> On 2023/2/14 7:45, Ulf Hansson wrote:
>>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> This patch adds runtime PM support.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> index 46b1ce7..fc917ed 100644
>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> @@ -15,6 +15,7 @@
>>>> #include <linux/module.h>
>>>> #include <linux/of.h>
>>>> #include <linux/of_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/reset.h>
>>>> #include <linux/sizes.h>
>>>>
>>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>> if (err)
>>>> goto err_setup_host;
>>>>
>>>> + pm_runtime_get_noresume(&pdev->dev);
>>>> + pm_runtime_set_active(&pdev->dev);
>>>> + pm_runtime_enable(&pdev->dev);
>>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>>>> + pm_runtime_use_autosuspend(&pdev->dev);
>>>> + pm_runtime_put_autosuspend(&pdev->dev);
>>>> +
>>>> return 0;
>>>>
>>>> err_setup_host:
>>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>>>> if (rk_priv)
>>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>>>> rk_priv->rockchip_clks);
>>>> +
>>>> + pm_runtime_get_sync(&pdev->dev);
>>>> + pm_runtime_disable(&pdev->dev);
>>>> + pm_runtime_put_noidle(&pdev->dev);
>>>> +
>>>> sdhci_pltfm_free(pdev);
>>>>
>>>> return 0;
>>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>>>> }
>>>> #endif
>>>>
>>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>>>> +#ifdef CONFIG_PM
>>>> +static int dwcmshc_runtime_suspend(struct device *dev)
>>>> +{
>>>> + struct sdhci_host *host = dev_get_drvdata(dev);
>>>> + u16 data;
>>>> + int ret;
>>>> +
>>>> + ret = sdhci_runtime_suspend_host(host);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> + data &= ~SDHCI_CLOCK_CARD_EN;
>>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int dwcmshc_runtime_resume(struct device *dev)
>>>> +{
>>>> + struct sdhci_host *host = dev_get_drvdata(dev);
>>>> + u16 data;
>>>> +
>>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> + data |= SDHCI_CLOCK_CARD_EN;
>>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> + return sdhci_runtime_resume_host(host, 0);
>>>> +}
>>>> +#endif
>>>> +
>>>> +static const struct dev_pm_ops dwcmshc_pmops = {
>>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>>>> + dwcmshc_resume)
>>>
>>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
>>> As sdhci_suspend_host() will write to internal registers of the IP
>>> block, it's recommended to make sure the device's runtime resumed
>>> before doing that call. So that needs to be added along with $subject
>>> patch.
>>>
>>
>> Yep, let me add a check here.
>>
>>> There is also another option that may better for you, which is to use
>>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
>>> of references to look at, but don't hesitate to ask around that, if
>>> you need some more help to get that working.
>>
>> If I understand correctly, pm_runtime_force_suspend|resume() helpers
>> would use runtime PM ops for suspend/resume routine. In this case, the
>> main issue is the handling of clock is quite different. In suspend we
>> need to gate all clocks but in rpm case, it shouldn't.
>
> I see, but let me then ask, what's the point of keeping the clocks
> ungated at runtime suspend?
>
> That sounds like wasting energy to me - but maybe there are good reasons for it?
The point to keep the clocks ungated at runtime suspend is that if they
are gated, the DLL would lost its locked state which causes retuning
every time. It's quite painful for performance. However, if we just gate
output clock to the device, the devices(basically I refer to eMMC) will
get into power-save status by itself. That helps us too keep balance
between power & performance during runtime. ：）
>
> [...]
>
> Kind regards
> Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
2023-02-15 0:50 ` Shawn Lin
@ 2023-02-28 11:24 ` Ulf Hansson
-1 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-28 11:24 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Rafael J. Wysocki
+ Rafael
On Wed, 15 Feb 2023 at 01:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf
>
> On 2023/2/14 18:41, Ulf Hansson wrote:
> > On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> Hi Ulf,
> >>
> >> On 2023/2/14 7:45, Ulf Hansson wrote:
> >>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>>>
> >>>> This patch adds runtime PM support.
> >>>>
> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>> ---
> >>>>
> >>>> Changes in v2: None
> >>>>
> >>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 50 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> index 46b1ce7..fc917ed 100644
> >>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> @@ -15,6 +15,7 @@
> >>>> #include <linux/module.h>
> >>>> #include <linux/of.h>
> >>>> #include <linux/of_device.h>
> >>>> +#include <linux/pm_runtime.h>
> >>>> #include <linux/reset.h>
> >>>> #include <linux/sizes.h>
> >>>>
> >>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>> if (err)
> >>>> goto err_setup_host;
> >>>>
> >>>> + pm_runtime_get_noresume(&pdev->dev);
> >>>> + pm_runtime_set_active(&pdev->dev);
> >>>> + pm_runtime_enable(&pdev->dev);
> >>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >>>> + pm_runtime_use_autosuspend(&pdev->dev);
> >>>> + pm_runtime_put_autosuspend(&pdev->dev);
> >>>> +
> >>>> return 0;
> >>>>
> >>>> err_setup_host:
> >>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >>>> if (rk_priv)
> >>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >>>> rk_priv->rockchip_clks);
> >>>> +
> >>>> + pm_runtime_get_sync(&pdev->dev);
> >>>> + pm_runtime_disable(&pdev->dev);
> >>>> + pm_runtime_put_noidle(&pdev->dev);
> >>>> +
> >>>> sdhci_pltfm_free(pdev);
> >>>>
> >>>> return 0;
> >>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >>>> }
> >>>> #endif
> >>>>
> >>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >>>> +#ifdef CONFIG_PM
> >>>> +static int dwcmshc_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> + struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> + u16 data;
> >>>> + int ret;
> >>>> +
> >>>> + ret = sdhci_runtime_suspend_host(host);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> + data &= ~SDHCI_CLOCK_CARD_EN;
> >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int dwcmshc_runtime_resume(struct device *dev)
> >>>> +{
> >>>> + struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> + u16 data;
> >>>> +
> >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> + data |= SDHCI_CLOCK_CARD_EN;
> >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> + return sdhci_runtime_resume_host(host, 0);
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> +static const struct dev_pm_ops dwcmshc_pmops = {
> >>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >>>> + dwcmshc_resume)
> >>>
> >>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> >>> As sdhci_suspend_host() will write to internal registers of the IP
> >>> block, it's recommended to make sure the device's runtime resumed
> >>> before doing that call. So that needs to be added along with $subject
> >>> patch.
> >>>
> >>
> >> Yep, let me add a check here.
> >>
> >>> There is also another option that may better for you, which is to use
> >>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> >>> of references to look at, but don't hesitate to ask around that, if
> >>> you need some more help to get that working.
> >>
> >> If I understand correctly, pm_runtime_force_suspend|resume() helpers
> >> would use runtime PM ops for suspend/resume routine. In this case, the
> >> main issue is the handling of clock is quite different. In suspend we
> >> need to gate all clocks but in rpm case, it shouldn't.
> >
> > I see, but let me then ask, what's the point of keeping the clocks
> > ungated at runtime suspend?
> >
> > That sounds like wasting energy to me - but maybe there are good reasons for it?
>
> The point to keep the clocks ungated at runtime suspend is that if they
> are gated, the DLL would lost its locked state which causes retuning
> every time. It's quite painful for performance. However, if we just gate
> output clock to the device, the devices(basically I refer to eMMC) will
> get into power-save status by itself. That helps us too keep balance
> between power & performance during runtime. ：）
I see, thanks for clarifying! In principle your approach should work
fine, but it depends a bit on the platform/soc too.
I assume there could also be a PM domain attached to the mmc host
device, right? If such a PM domain gets powered off, wouldn't you need
to run a retuning sequence anyway?
FYI, I have heard about similar problems for devices before - and it's
been discussed too. In principle, it sounds to me that we have devices
that would benefit from using *multiple* idle states, rather than just
the one we have for runtime suspend and the one for system wide
suspend.
Kind regards
Uffe
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support
@ 2023-02-28 11:24 ` Ulf Hansson
0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2023-02-28 11:24 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-mmc, Adrian Hunter, linux-rockchip, Rafael J. Wysocki
+ Rafael
On Wed, 15 Feb 2023 at 01:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf
>
> On 2023/2/14 18:41, Ulf Hansson wrote:
> > On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> Hi Ulf,
> >>
> >> On 2023/2/14 7:45, Ulf Hansson wrote:
> >>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>>>
> >>>> This patch adds runtime PM support.
> >>>>
> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>> ---
> >>>>
> >>>> Changes in v2: None
> >>>>
> >>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 50 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> index 46b1ce7..fc917ed 100644
> >>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> @@ -15,6 +15,7 @@
> >>>> #include <linux/module.h>
> >>>> #include <linux/of.h>
> >>>> #include <linux/of_device.h>
> >>>> +#include <linux/pm_runtime.h>
> >>>> #include <linux/reset.h>
> >>>> #include <linux/sizes.h>
> >>>>
> >>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>> if (err)
> >>>> goto err_setup_host;
> >>>>
> >>>> + pm_runtime_get_noresume(&pdev->dev);
> >>>> + pm_runtime_set_active(&pdev->dev);
> >>>> + pm_runtime_enable(&pdev->dev);
> >>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >>>> + pm_runtime_use_autosuspend(&pdev->dev);
> >>>> + pm_runtime_put_autosuspend(&pdev->dev);
> >>>> +
> >>>> return 0;
> >>>>
> >>>> err_setup_host:
> >>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >>>> if (rk_priv)
> >>>> clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >>>> rk_priv->rockchip_clks);
> >>>> +
> >>>> + pm_runtime_get_sync(&pdev->dev);
> >>>> + pm_runtime_disable(&pdev->dev);
> >>>> + pm_runtime_put_noidle(&pdev->dev);
> >>>> +
> >>>> sdhci_pltfm_free(pdev);
> >>>>
> >>>> return 0;
> >>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >>>> }
> >>>> #endif
> >>>>
> >>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >>>> +#ifdef CONFIG_PM
> >>>> +static int dwcmshc_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> + struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> + u16 data;
> >>>> + int ret;
> >>>> +
> >>>> + ret = sdhci_runtime_suspend_host(host);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> + data &= ~SDHCI_CLOCK_CARD_EN;
> >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int dwcmshc_runtime_resume(struct device *dev)
> >>>> +{
> >>>> + struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> + u16 data;
> >>>> +
> >>>> + data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> + data |= SDHCI_CLOCK_CARD_EN;
> >>>> + sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> + return sdhci_runtime_resume_host(host, 0);
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> +static const struct dev_pm_ops dwcmshc_pmops = {
> >>>> + SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >>>> + dwcmshc_resume)
> >>>
> >>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> >>> As sdhci_suspend_host() will write to internal registers of the IP
> >>> block, it's recommended to make sure the device's runtime resumed
> >>> before doing that call. So that needs to be added along with $subject
> >>> patch.
> >>>
> >>
> >> Yep, let me add a check here.
> >>
> >>> There is also another option that may better for you, which is to use
> >>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> >>> of references to look at, but don't hesitate to ask around that, if
> >>> you need some more help to get that working.
> >>
> >> If I understand correctly, pm_runtime_force_suspend|resume() helpers
> >> would use runtime PM ops for suspend/resume routine. In this case, the
> >> main issue is the handling of clock is quite different. In suspend we
> >> need to gate all clocks but in rpm case, it shouldn't.
> >
> > I see, but let me then ask, what's the point of keeping the clocks
> > ungated at runtime suspend?
> >
> > That sounds like wasting energy to me - but maybe there are good reasons for it?
>
> The point to keep the clocks ungated at runtime suspend is that if they
> are gated, the DLL would lost its locked state which causes retuning
> every time. It's quite painful for performance. However, if we just gate
> output clock to the device, the devices(basically I refer to eMMC) will
> get into power-save status by itself. That helps us too keep balance
> between power & performance during runtime. ：）
I see, thanks for clarifying! In principle your approach should work
fine, but it depends a bit on the platform/soc too.
I assume there could also be a PM domain attached to the mmc host
device, right? If such a PM domain gets powered off, wouldn't you need
to run a retuning sequence anyway?
FYI, I have heard about similar problems for devices before - and it's
been discussed too. In principle, it sounds to me that we have devices
that would benefit from using *multiple* idle states, rather than just
the one we have for runtime suspend and the one for system wide
suspend.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-02-28 11:27 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-02 0:35 [PATCH v3 0/3] Some features and fix for sdhci-of-dwcmshc Shawn Lin
2023-02-02 0:35 ` Shawn Lin
2023-02-02 0:35 ` [PATCH v3 1/3] mmc: sdhci-of-dwcmshc: Update DLL and pre-change delay for rockchip platform Shawn Lin
2023-02-02 0:35 ` Shawn Lin
2023-02-13 23:48 ` Ulf Hansson
2023-02-13 23:48 ` Ulf Hansson
2023-02-02 0:35 ` [PATCH v3 2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support Shawn Lin
2023-02-02 0:35 ` Shawn Lin
2023-02-13 23:45 ` Ulf Hansson
2023-02-13 23:45 ` Ulf Hansson
2023-02-14 3:36 ` Shawn Lin
2023-02-14 3:36 ` Shawn Lin
2023-02-14 10:41 ` Ulf Hansson
2023-02-14 10:41 ` Ulf Hansson
2023-02-15 0:50 ` Shawn Lin
2023-02-15 0:50 ` Shawn Lin
2023-02-28 11:24 ` Ulf Hansson
2023-02-28 11:24 ` Ulf Hansson
2023-02-02 0:35 ` [PATCH v3 3/3] mmc: sdhci-of-dwcmshc: Add host software queue support Shawn Lin
2023-02-02 0:35 ` Shawn Lin
2023-02-14 11:31 ` Ulf Hansson
2023-02-14 11:31 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.