enable the system pm and runtime pm for mxs-dma and refine the gpmi
runtime pm code
Han Xu (6):
dmaengine: mxs: change the way to register probe function
dmaengine: mxs: add the remove function
dmaengine: mxs: add the power management functions
dmaengine: mxs: switch from dma_coherent to dma_pool
mtd: rawnand: gpmi: refine the runtime pm ops
mtd: rawnand: gpmi: set the pinctrl state for suspend/reusme
drivers/dma/mxs-dma.c | 155 ++++++++++++++++++---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 60 ++++----
2 files changed, 167 insertions(+), 48 deletions(-)
--
2.17.1
several changes for runtime code in gpmi-nand driver
- Always invoke runtime get/put in same function to balance the usage
counter.
- leverage the runtime pm for system pm, move acquire dma to runtime pm
to acquire dma only when needed.
- add pm_runtime_dont_use_autosuspend in err path. If driver failed to
probe before runtime pm timeout, such as NAND not mounted in socket,
runtime suspend won't be called without the change.
Signed-off-by: Han Xu <[email protected]>
---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 56 +++++++++++-----------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index fcc7325f2a10..73644c96fa9b 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -183,7 +183,6 @@ static int gpmi_init(struct gpmi_nand_data *this)
*/
writel(BM_GPMI_CTRL1_DECOUPLE_CS, r->gpmi_regs + HW_GPMI_CTRL1_SET);
- return 0;
err_out:
pm_runtime_mark_last_busy(this->dev);
pm_runtime_put_autosuspend(this->dev);
@@ -556,7 +555,6 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
/* Set *all* chip selects to use layout 0. */
writel(0, r->bch_regs + HW_BCH_LAYOUTSELECT);
- ret = 0;
err_out:
pm_runtime_mark_last_busy(this->dev);
pm_runtime_put_autosuspend(this->dev);
@@ -1213,10 +1211,6 @@ static int acquire_resources(struct gpmi_nand_data *this)
if (ret)
goto exit_regs;
- ret = acquire_dma_channels(this);
- if (ret)
- goto exit_regs;
-
ret = gpmi_get_clks(this);
if (ret)
goto exit_clock;
@@ -2656,15 +2650,9 @@ static int gpmi_nand_probe(struct platform_device *pdev)
if (ret)
goto exit_acquire_resources;
- ret = __gpmi_enable_clk(this, true);
- if (ret)
- goto exit_nfc_init;
-
+ pm_runtime_enable(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
pm_runtime_use_autosuspend(&pdev->dev);
- pm_runtime_set_active(&pdev->dev);
- pm_runtime_enable(&pdev->dev);
- pm_runtime_get_sync(&pdev->dev);
ret = gpmi_init(this);
if (ret)
@@ -2674,15 +2662,12 @@ static int gpmi_nand_probe(struct platform_device *pdev)
if (ret)
goto exit_nfc_init;
- pm_runtime_mark_last_busy(&pdev->dev);
- pm_runtime_put_autosuspend(&pdev->dev);
-
dev_info(this->dev, "driver registered.\n");
return 0;
exit_nfc_init:
- pm_runtime_put(&pdev->dev);
+ pm_runtime_dont_use_autosuspend(&pdev->dev);
pm_runtime_disable(&pdev->dev);
release_resources(this);
exit_acquire_resources:
@@ -2694,7 +2679,6 @@ static int gpmi_nand_remove(struct platform_device *pdev)
{
struct gpmi_nand_data *this = platform_get_drvdata(pdev);
- pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
nand_release(&this->nand);
@@ -2706,10 +2690,11 @@ static int gpmi_nand_remove(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int gpmi_pm_suspend(struct device *dev)
{
- struct gpmi_nand_data *this = dev_get_drvdata(dev);
+ int ret;
- release_dma_channels(this);
- return 0;
+ ret = pm_runtime_force_suspend(dev);
+
+ return ret;
}
static int gpmi_pm_resume(struct device *dev)
@@ -2717,9 +2702,11 @@ static int gpmi_pm_resume(struct device *dev)
struct gpmi_nand_data *this = dev_get_drvdata(dev);
int ret;
- ret = acquire_dma_channels(this);
- if (ret < 0)
+ ret = pm_runtime_force_resume(dev);
+ if (ret) {
+ dev_err(this->dev, "Error in resume %d\n", ret);
return ret;
+ }
/* re-init the GPMI registers */
ret = gpmi_init(this);
@@ -2743,18 +2730,33 @@ static int gpmi_pm_resume(struct device *dev)
}
#endif /* CONFIG_PM_SLEEP */
-static int __maybe_unused gpmi_runtime_suspend(struct device *dev)
+#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
+#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
+
+static int gpmi_runtime_suspend(struct device *dev)
{
struct gpmi_nand_data *this = dev_get_drvdata(dev);
- return __gpmi_enable_clk(this, false);
+ gpmi_disable_clk(this);
+ release_dma_channels(this);
+
+ return 0;
}
-static int __maybe_unused gpmi_runtime_resume(struct device *dev)
+static int gpmi_runtime_resume(struct device *dev)
{
struct gpmi_nand_data *this = dev_get_drvdata(dev);
+ int ret;
- return __gpmi_enable_clk(this, true);
+ ret = gpmi_enable_clk(this);
+ if (ret)
+ return ret;
+
+ ret = acquire_dma_channels(this);
+ if (ret < 0)
+ return ret;
+
+ return 0;
}
static const struct dev_pm_ops gpmi_pm_ops = {
--
2.17.1
set the correct pinctrl state in system pm suspend/resume ops
Signed-off-by: Han Xu <[email protected]>
---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 73644c96fa9b..de1e3dbb2eb1 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -15,6 +15,7 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/dma/mxs-dma.h>
#include "gpmi-nand.h"
#include "gpmi-regs.h"
@@ -2692,6 +2693,7 @@ static int gpmi_pm_suspend(struct device *dev)
{
int ret;
+ pinctrl_pm_select_sleep_state(dev);
ret = pm_runtime_force_suspend(dev);
return ret;
@@ -2708,6 +2710,8 @@ static int gpmi_pm_resume(struct device *dev)
return ret;
}
+ pinctrl_pm_select_default_state(dev);
+
/* re-init the GPMI registers */
ret = gpmi_init(this);
if (ret) {
--
2.17.1
create one dma_pool dedicate for mxs-dma to avoid the
"alloc_contig_range: [xxx, xxx) PFNs busy" warning message during
frequently alloc/free resource ops in runtime pm.
Signed-off-by: Han Xu <[email protected]>
---
drivers/dma/mxs-dma.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 251492c5ea58..dfee41ae1981 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -26,6 +26,7 @@
#include <linux/list.h>
#include <linux/dma/mxs-dma.h>
#include <linux/pm_runtime.h>
+#include <linux/dmapool.h>
#include <asm/irq.h>
@@ -121,6 +122,7 @@ struct mxs_dma_chan {
enum dma_status status;
unsigned int flags;
bool reset;
+ struct dma_pool *ccw_pool;
#define MXS_DMA_SG_LOOP (1 << 0)
#define MXS_DMA_USE_SEMAPHORE (1 << 1)
};
@@ -422,9 +424,10 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
struct device *dev = &mxs_dma->pdev->dev;
int ret;
- mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
- CCW_BLOCK_SIZE,
- &mxs_chan->ccw_phys, GFP_KERNEL);
+ mxs_chan->ccw = dma_pool_zalloc(mxs_chan->ccw_pool,
+ GFP_ATOMIC,
+ &mxs_chan->ccw_phys);
+
if (!mxs_chan->ccw) {
ret = -ENOMEM;
goto err_alloc;
@@ -454,8 +457,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
err_clk:
free_irq(mxs_chan->chan_irq, mxs_dma);
err_irq:
- dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
- mxs_chan->ccw, mxs_chan->ccw_phys);
+ dma_pool_free(mxs_chan->ccw_pool, mxs_chan->ccw,
+ mxs_chan->ccw_phys);
err_alloc:
return ret;
}
@@ -470,8 +473,8 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan)
free_irq(mxs_chan->chan_irq, mxs_dma);
- dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
- mxs_chan->ccw, mxs_chan->ccw_phys);
+ dma_pool_free(mxs_chan->ccw_pool, mxs_chan->ccw,
+ mxs_chan->ccw_phys);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
@@ -796,6 +799,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
const struct mxs_dma_type *dma_type;
struct mxs_dma_engine *mxs_dma;
struct resource *iores;
+ struct dma_pool *ccw_pool;
int ret, i;
mxs_dma = devm_kzalloc(&pdev->dev, sizeof(*mxs_dma), GFP_KERNEL);
@@ -843,7 +847,6 @@ static int mxs_dma_probe(struct platform_device *pdev)
tasklet_init(&mxs_chan->tasklet, mxs_dma_tasklet,
(unsigned long) mxs_chan);
-
/* Add the channel to mxs_chan list */
list_add_tail(&mxs_chan->chan.device_node,
&mxs_dma->dma_device.channels);
@@ -858,6 +861,17 @@ static int mxs_dma_probe(struct platform_device *pdev)
mxs_dma->dma_device.dev = &pdev->dev;
+ /* create the dma pool */
+ ccw_pool = dma_pool_create("ccw_pool",
+ mxs_dma->dma_device.dev,
+ CCW_BLOCK_SIZE, 32, 0);
+
+ for (i = 0; i < MXS_DMA_CHANNELS; i++) {
+ struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
+
+ mxs_chan->ccw_pool = ccw_pool;
+ }
+
/* mxs_dma gets 65535 bytes maximum sg size */
mxs_dma->dma_device.dev->dma_parms = &mxs_dma->dma_parms;
dma_set_max_seg_size(mxs_dma->dma_device.dev, MAX_XFER_BYTES);
@@ -899,11 +913,13 @@ static int mxs_dma_remove(struct platform_device *pdev)
int i;
dma_async_device_unregister(&mxs_dma->dma_device);
+ dma_pool_destroy(mxs_dma->mxs_chans[0].ccw_pool);
for (i = 0; i < MXS_DMA_CHANNELS; i++) {
struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
tasklet_kill(&mxs_chan->tasklet);
+ mxs_chan->ccw_pool = NULL;
}
return 0;
--
2.17.1
add the remove function for mxs-dma
Signed-off-by: Han Xu <[email protected]>
---
drivers/dma/mxs-dma.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 9deaaf4fc58f..b458f06f9067 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -863,6 +863,22 @@ static int mxs_dma_probe(struct platform_device *pdev)
return 0;
}
+static int mxs_dma_remove(struct platform_device *pdev)
+{
+ struct mxs_dma_engine *mxs_dma = platform_get_drvdata(pdev);
+ int i;
+
+ dma_async_device_unregister(&mxs_dma->dma_device);
+
+ for (i = 0; i < MXS_DMA_CHANNELS; i++) {
+ struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
+
+ tasklet_kill(&mxs_chan->tasklet);
+ }
+
+ return 0;
+}
+
static struct platform_driver mxs_dma_driver = {
.driver = {
.name = "mxs-dma",
@@ -870,6 +886,7 @@ static struct platform_driver mxs_dma_driver = {
},
.id_table = mxs_dma_ids,
.probe = mxs_dma_probe,
+ .remove = mxs_dma_remove,
};
module_platform_driver(mxs_dma_driver);
--
2.17.1
change the way to register probe function for mxs-dma
Signed-off-by: Han Xu <[email protected]>
---
drivers/dma/mxs-dma.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 3039bba0e4d5..9deaaf4fc58f 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -760,7 +760,7 @@ static struct dma_chan *mxs_dma_xlate(struct of_phandle_args *dma_spec,
ofdma->of_node);
}
-static int __init mxs_dma_probe(struct platform_device *pdev)
+static int mxs_dma_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
const struct platform_device_id *id_entry;
@@ -869,10 +869,7 @@ static struct platform_driver mxs_dma_driver = {
.of_match_table = mxs_dma_dt_ids,
},
.id_table = mxs_dma_ids,
+ .probe = mxs_dma_probe,
};
-static int __init mxs_dma_module_init(void)
-{
- return platform_driver_probe(&mxs_dma_driver, mxs_dma_probe);
-}
-subsys_initcall(mxs_dma_module_init);
+module_platform_driver(mxs_dma_driver);
--
2.17.1
add the power management functions and leverage the runtime pm for
system suspend/resume
Signed-off-by: Han Xu <[email protected]>
---
drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 90 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index b458f06f9067..251492c5ea58 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -25,6 +25,7 @@
#include <linux/of_dma.h>
#include <linux/list.h>
#include <linux/dma/mxs-dma.h>
+#include <linux/pm_runtime.h>
#include <asm/irq.h>
@@ -39,6 +40,8 @@
#define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
#define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
+#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
+
#define HW_APBHX_CTRL0 0x000
#define BM_APBH_CTRL0_APB_BURST8_EN (1 << 29)
#define BM_APBH_CTRL0_APB_BURST_EN (1 << 28)
@@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
{
struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+ struct device *dev = &mxs_dma->pdev->dev;
int ret;
mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
@@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
if (ret)
goto err_irq;
- ret = clk_prepare_enable(mxs_dma->clk);
- if (ret)
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to enable clock\n");
goto err_clk;
+ }
mxs_dma_reset_chan(chan);
@@ -458,6 +464,7 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan)
{
struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+ struct device *dev = &mxs_dma->pdev->dev;
mxs_dma_disable_chan(chan);
@@ -466,7 +473,9 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan)
dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
mxs_chan->ccw, mxs_chan->ccw_phys);
- clk_disable_unprepare(mxs_dma->clk);
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
}
/*
@@ -689,14 +698,32 @@ static enum dma_status mxs_dma_tx_status(struct dma_chan *chan,
return mxs_chan->status;
}
-static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
+static int mxs_dma_init_rpm(struct mxs_dma_engine *mxs_dma)
{
+ struct device *dev = &mxs_dma->pdev->dev;
+
+ pm_runtime_enable(dev);
+ pm_runtime_set_autosuspend_delay(dev, MXS_DMA_RPM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev);
+
+ return 0;
+}
+
+static int mxs_dma_init(struct mxs_dma_engine *mxs_dma)
+{
+ struct device *dev = &mxs_dma->pdev->dev;
int ret;
- ret = clk_prepare_enable(mxs_dma->clk);
+ ret = mxs_dma_init_rpm(mxs_dma);
if (ret)
return ret;
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to enable clock\n");
+ return ret;
+ }
+
ret = stmp_reset_block(mxs_dma->base);
if (ret)
goto err_out;
@@ -714,7 +741,8 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
mxs_dma->base + HW_APBHX_CTRL1 + STMP_OFFSET_REG_SET);
err_out:
- clk_disable_unprepare(mxs_dma->clk);
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
return ret;
}
@@ -821,11 +849,13 @@ static int mxs_dma_probe(struct platform_device *pdev)
&mxs_dma->dma_device.channels);
}
+ platform_set_drvdata(pdev, mxs_dma);
+ mxs_dma->pdev = pdev;
+
ret = mxs_dma_init(mxs_dma);
if (ret)
return ret;
- mxs_dma->pdev = pdev;
mxs_dma->dma_device.dev = &pdev->dev;
/* mxs_dma gets 65535 bytes maximum sg size */
@@ -879,9 +909,62 @@ static int mxs_dma_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+static int mxs_dma_pm_suspend(struct device *dev)
+{
+ int ret;
+
+ ret = pm_runtime_force_suspend(dev);
+
+ return ret;
+}
+
+static int mxs_dma_pm_resume(struct device *dev)
+{
+ struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+ int ret;
+
+ ret = mxs_dma_init(mxs_dma);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+#endif
+
+int mxs_dma_runtime_suspend(struct device *dev)
+{
+ struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(mxs_dma->clk);
+
+ return 0;
+}
+
+int mxs_dma_runtime_resume(struct device *dev)
+{
+ struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(mxs_dma->clk);
+ if (ret) {
+ dev_err(&mxs_dma->pdev->dev, "failed to enable the clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops mxs_dma_pm_ops = {
+ SET_RUNTIME_PM_OPS(mxs_dma_runtime_suspend,
+ mxs_dma_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(mxs_dma_pm_suspend, mxs_dma_pm_resume)
+};
+
static struct platform_driver mxs_dma_driver = {
.driver = {
.name = "mxs-dma",
+ .pm = &mxs_dma_pm_ops,
.of_match_table = mxs_dma_dt_ids,
},
.id_table = mxs_dma_ids,
--
2.17.1
On Tue, Jan 14, 2020 at 6:48 PM Han Xu <[email protected]> wrote:
>
> change the way to register probe function for mxs-dma
Please provide the reasoning for such change in the commit log.
On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> add the power management functions and leverage the runtime pm for
> system suspend/resume
>
> Signed-off-by: Han Xu <[email protected]>
> ---
> drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 90 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index b458f06f9067..251492c5ea58 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -25,6 +25,7 @@
> #include <linux/of_dma.h>
> #include <linux/list.h>
> #include <linux/dma/mxs-dma.h>
> +#include <linux/pm_runtime.h>
>
> #include <asm/irq.h>
>
> @@ -39,6 +40,8 @@
> #define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
> #define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
>
> +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> +
> #define HW_APBHX_CTRL0 0x000
> #define BM_APBH_CTRL0_APB_BURST8_EN (1 << 29)
> #define BM_APBH_CTRL0_APB_BURST_EN (1 << 28)
> @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> {
> struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> + struct device *dev = &mxs_dma->pdev->dev;
> int ret;
>
> mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> if (ret)
> goto err_irq;
>
> - ret = clk_prepare_enable(mxs_dma->clk);
> - if (ret)
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "Failed to enable clock\n");
> goto err_clk;
From looking at other DMA drivers I know we are in good company here,
but I think this is wrong. Doing pm_runtime_get_sync() in
alloc_chan_resources() and going to autosuspend in free_chan_resources()
effectively disables runtime_pm as clients normally acquire their
channels during driver probe and release them only in driver remove.
In the next patch you release the DMA channels in the GPMI nand drivers
runtime_suspend hook just to somehow trigger the runtime_suspend of the
DMA driver.
What you should do instead is to make sure the hook runtime_pm to the
DMA drivers activity phases, like for example the pl330 driver does.
Then you wouldn't have to care about manually putting the DMA driver into
suspend from the GPMI NAND driver.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Jan 15, 2020 at 05:44:01AM +0800, Han Xu wrote:
> create one dma_pool dedicate for mxs-dma to avoid the
> "alloc_contig_range: [xxx, xxx) PFNs busy" warning message during
> frequently alloc/free resource ops in runtime pm.
>
> Signed-off-by: Han Xu <[email protected]>
> ---
> drivers/dma/mxs-dma.c | 32 ++++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 251492c5ea58..dfee41ae1981 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -26,6 +26,7 @@
> #include <linux/list.h>
> #include <linux/dma/mxs-dma.h>
> #include <linux/pm_runtime.h>
> +#include <linux/dmapool.h>
>
> #include <asm/irq.h>
>
> @@ -121,6 +122,7 @@ struct mxs_dma_chan {
> enum dma_status status;
> unsigned int flags;
> bool reset;
> + struct dma_pool *ccw_pool;
> #define MXS_DMA_SG_LOOP (1 << 0)
> #define MXS_DMA_USE_SEMAPHORE (1 << 1)
> };
> @@ -422,9 +424,10 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> struct device *dev = &mxs_dma->pdev->dev;
> int ret;
>
> - mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> - CCW_BLOCK_SIZE,
> - &mxs_chan->ccw_phys, GFP_KERNEL);
> + mxs_chan->ccw = dma_pool_zalloc(mxs_chan->ccw_pool,
> + GFP_ATOMIC,
> + &mxs_chan->ccw_phys);
Why GFP_ATOMIC?
> +
> if (!mxs_chan->ccw) {
> ret = -ENOMEM;
> goto err_alloc;
> @@ -454,8 +457,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> err_clk:
> free_irq(mxs_chan->chan_irq, mxs_dma);
> err_irq:
> - dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
> - mxs_chan->ccw, mxs_chan->ccw_phys);
> + dma_pool_free(mxs_chan->ccw_pool, mxs_chan->ccw,
> + mxs_chan->ccw_phys);
> err_alloc:
> return ret;
> }
> @@ -470,8 +473,8 @@ static void mxs_dma_free_chan_resources(struct dma_chan *chan)
>
> free_irq(mxs_chan->chan_irq, mxs_dma);
>
> - dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
> - mxs_chan->ccw, mxs_chan->ccw_phys);
> + dma_pool_free(mxs_chan->ccw_pool, mxs_chan->ccw,
> + mxs_chan->ccw_phys);
>
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> @@ -796,6 +799,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
> const struct mxs_dma_type *dma_type;
> struct mxs_dma_engine *mxs_dma;
> struct resource *iores;
> + struct dma_pool *ccw_pool;
> int ret, i;
>
> mxs_dma = devm_kzalloc(&pdev->dev, sizeof(*mxs_dma), GFP_KERNEL);
> @@ -843,7 +847,6 @@ static int mxs_dma_probe(struct platform_device *pdev)
> tasklet_init(&mxs_chan->tasklet, mxs_dma_tasklet,
> (unsigned long) mxs_chan);
>
> -
> /* Add the channel to mxs_chan list */
> list_add_tail(&mxs_chan->chan.device_node,
> &mxs_dma->dma_device.channels);
> @@ -858,6 +861,17 @@ static int mxs_dma_probe(struct platform_device *pdev)
>
> mxs_dma->dma_device.dev = &pdev->dev;
>
> + /* create the dma pool */
> + ccw_pool = dma_pool_create("ccw_pool",
> + mxs_dma->dma_device.dev,
> + CCW_BLOCK_SIZE, 32, 0);
> +
> + for (i = 0; i < MXS_DMA_CHANNELS; i++) {
> + struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
> +
> + mxs_chan->ccw_pool = ccw_pool;
> + }
ccw_pool is the same for every channel, it should be a member of
struct mxs_dma_engine and not of struct mcs_dma_chan.
> +
> /* mxs_dma gets 65535 bytes maximum sg size */
> mxs_dma->dma_device.dev->dma_parms = &mxs_dma->dma_parms;
> dma_set_max_seg_size(mxs_dma->dma_device.dev, MAX_XFER_BYTES);
> @@ -899,11 +913,13 @@ static int mxs_dma_remove(struct platform_device *pdev)
> int i;
>
> dma_async_device_unregister(&mxs_dma->dma_device);
> + dma_pool_destroy(mxs_dma->mxs_chans[0].ccw_pool);
It doesn't seem to make a big difference, but I would do this after
killing the tasklets, not before. Otherwise we would have to prove that
no tasklet is still accessing the dma_pool.
>
> for (i = 0; i < MXS_DMA_CHANNELS; i++) {
> struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
>
> tasklet_kill(&mxs_chan->tasklet);
> + mxs_chan->ccw_pool = NULL;
There's no point in resetting this to NULL, mxs_chan is never going to
be touched again.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Jan 15, 2020 at 05:44:02AM +0800, Han Xu wrote:
> several changes for runtime code in gpmi-nand driver
>
> - Always invoke runtime get/put in same function to balance the usage
> counter.
>
> - leverage the runtime pm for system pm, move acquire dma to runtime pm
> to acquire dma only when needed.
>
> - add pm_runtime_dont_use_autosuspend in err path. If driver failed to
> probe before runtime pm timeout, such as NAND not mounted in socket,
> runtime suspend won't be called without the change.
Using a bullet list in a commit message is often a sign that the patch
should be split into multiple patches...
>
> Signed-off-by: Han Xu <[email protected]>
> ---
> drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 56 +++++++++++-----------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index fcc7325f2a10..73644c96fa9b 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -183,7 +183,6 @@ static int gpmi_init(struct gpmi_nand_data *this)
> */
> writel(BM_GPMI_CTRL1_DECOUPLE_CS, r->gpmi_regs + HW_GPMI_CTRL1_SET);
>
> - return 0;
> err_out:
> pm_runtime_mark_last_busy(this->dev);
> pm_runtime_put_autosuspend(this->dev);
> @@ -556,7 +555,6 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
> /* Set *all* chip selects to use layout 0. */
> writel(0, r->bch_regs + HW_BCH_LAYOUTSELECT);
>
> - ret = 0;
> err_out:
> pm_runtime_mark_last_busy(this->dev);
> pm_runtime_put_autosuspend(this->dev);
While I agree that this "ret = 0" is unnecessary because 'ret' holds the
successful return value of the last function called, I still think it's
nice to make it explicit that this is the success path of this function.
If you disagree please at least make this a separate patch.
> @@ -1213,10 +1211,6 @@ static int acquire_resources(struct gpmi_nand_data *this)
> if (ret)
> goto exit_regs;
>
> - ret = acquire_dma_channels(this);
> - if (ret)
> - goto exit_regs;
> -
> ret = gpmi_get_clks(this);
> if (ret)
> goto exit_clock;
> @@ -2656,15 +2650,9 @@ static int gpmi_nand_probe(struct platform_device *pdev)
> if (ret)
> goto exit_acquire_resources;
>
> - ret = __gpmi_enable_clk(this, true);
> - if (ret)
> - goto exit_nfc_init;
> -
> + pm_runtime_enable(&pdev->dev);
> pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
> pm_runtime_use_autosuspend(&pdev->dev);
> - pm_runtime_set_active(&pdev->dev);
> - pm_runtime_enable(&pdev->dev);
> - pm_runtime_get_sync(&pdev->dev);
>
> ret = gpmi_init(this);
> if (ret)
> @@ -2674,15 +2662,12 @@ static int gpmi_nand_probe(struct platform_device *pdev)
> if (ret)
> goto exit_nfc_init;
>
> - pm_runtime_mark_last_busy(&pdev->dev);
> - pm_runtime_put_autosuspend(&pdev->dev);
> -
> dev_info(this->dev, "driver registered.\n");
>
> return 0;
>
> exit_nfc_init:
> - pm_runtime_put(&pdev->dev);
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> release_resources(this);
> exit_acquire_resources:
> @@ -2694,7 +2679,6 @@ static int gpmi_nand_remove(struct platform_device *pdev)
> {
> struct gpmi_nand_data *this = platform_get_drvdata(pdev);
>
> - pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> nand_release(&this->nand);
> @@ -2706,10 +2690,11 @@ static int gpmi_nand_remove(struct platform_device *pdev)
> #ifdef CONFIG_PM_SLEEP
> static int gpmi_pm_suspend(struct device *dev)
> {
> - struct gpmi_nand_data *this = dev_get_drvdata(dev);
> + int ret;
>
> - release_dma_channels(this);
> - return 0;
> + ret = pm_runtime_force_suspend(dev);
> +
> + return ret;
> }
>
> static int gpmi_pm_resume(struct device *dev)
> @@ -2717,9 +2702,11 @@ static int gpmi_pm_resume(struct device *dev)
> struct gpmi_nand_data *this = dev_get_drvdata(dev);
> int ret;
>
> - ret = acquire_dma_channels(this);
> - if (ret < 0)
> + ret = pm_runtime_force_resume(dev);
> + if (ret) {
> + dev_err(this->dev, "Error in resume %d\n", ret);
> return ret;
> + }
>
> /* re-init the GPMI registers */
> ret = gpmi_init(this);
> @@ -2743,18 +2730,33 @@ static int gpmi_pm_resume(struct device *dev)
> }
> #endif /* CONFIG_PM_SLEEP */
>
> -static int __maybe_unused gpmi_runtime_suspend(struct device *dev)
> +#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
> +#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
These defines do not add any value.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Jan 15, 2020 at 2:03 AM Sascha Hauer <[email protected]> wrote:
>
> On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> > add the power management functions and leverage the runtime pm for
> > system suspend/resume
> >
> > Signed-off-by: Han Xu <[email protected]>
> > ---
> > drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 90 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > index b458f06f9067..251492c5ea58 100644
> > --- a/drivers/dma/mxs-dma.c
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -25,6 +25,7 @@
> > #include <linux/of_dma.h>
> > #include <linux/list.h>
> > #include <linux/dma/mxs-dma.h>
> > +#include <linux/pm_runtime.h>
> >
> > #include <asm/irq.h>
> >
> > @@ -39,6 +40,8 @@
> > #define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
> > #define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
> >
> > +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> > +
> > #define HW_APBHX_CTRL0 0x000
> > #define BM_APBH_CTRL0_APB_BURST8_EN (1 << 29)
> > #define BM_APBH_CTRL0_APB_BURST_EN (1 << 28)
> > @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > {
> > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > + struct device *dev = &mxs_dma->pdev->dev;
> > int ret;
> >
> > mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> > @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > if (ret)
> > goto err_irq;
> >
> > - ret = clk_prepare_enable(mxs_dma->clk);
> > - if (ret)
> > + ret = pm_runtime_get_sync(dev);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to enable clock\n");
> > goto err_clk;
>
> From looking at other DMA drivers I know we are in good company here,
> but I think this is wrong. Doing pm_runtime_get_sync() in
> alloc_chan_resources() and going to autosuspend in free_chan_resources()
> effectively disables runtime_pm as clients normally acquire their
> channels during driver probe and release them only in driver remove.
Thanks for the comments.
That's why I moved acquire_dma_resource from the probe to
runtime_resume in the gpmi driver, this change won't disable the
runtime_pm function and the incremental counter always balanced.
>
> In the next patch you release the DMA channels in the GPMI nand drivers
> runtime_suspend hook just to somehow trigger the runtime_suspend of the
> DMA driver.
>
> What you should do instead is to make sure the hook runtime_pm to the
> DMA drivers activity phases, like for example the pl330 driver does.
> Then you wouldn't have to care about manually putting the DMA driver into
> suspend from the GPMI NAND driver.
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Sincerely,
Han XU
On Thu, Jan 16, 2020 at 10:36:33AM -0600, Han Xu wrote:
> On Wed, Jan 15, 2020 at 2:03 AM Sascha Hauer <[email protected]> wrote:
> >
> > On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> > > add the power management functions and leverage the runtime pm for
> > > system suspend/resume
> > >
> > > Signed-off-by: Han Xu <[email protected]>
> > > ---
> > > drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 90 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > > index b458f06f9067..251492c5ea58 100644
> > > --- a/drivers/dma/mxs-dma.c
> > > +++ b/drivers/dma/mxs-dma.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/of_dma.h>
> > > #include <linux/list.h>
> > > #include <linux/dma/mxs-dma.h>
> > > +#include <linux/pm_runtime.h>
> > >
> > > #include <asm/irq.h>
> > >
> > > @@ -39,6 +40,8 @@
> > > #define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
> > > #define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
> > >
> > > +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> > > +
> > > #define HW_APBHX_CTRL0 0x000
> > > #define BM_APBH_CTRL0_APB_BURST8_EN (1 << 29)
> > > #define BM_APBH_CTRL0_APB_BURST_EN (1 << 28)
> > > @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > > {
> > > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > > struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > > + struct device *dev = &mxs_dma->pdev->dev;
> > > int ret;
> > >
> > > mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> > > @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > > if (ret)
> > > goto err_irq;
> > >
> > > - ret = clk_prepare_enable(mxs_dma->clk);
> > > - if (ret)
> > > + ret = pm_runtime_get_sync(dev);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to enable clock\n");
> > > goto err_clk;
> >
> > From looking at other DMA drivers I know we are in good company here,
> > but I think this is wrong. Doing pm_runtime_get_sync() in
> > alloc_chan_resources() and going to autosuspend in free_chan_resources()
> > effectively disables runtime_pm as clients normally acquire their
> > channels during driver probe and release them only in driver remove.
>
> Thanks for the comments.
> That's why I moved acquire_dma_resource from the probe to
> runtime_resume in the gpmi driver, this change won't disable the
> runtime_pm function and the incremental counter always balanced.
Yes, that's what I've written a few lines further down:
>
> >
> > In the next patch you release the DMA channels in the GPMI nand drivers
> > runtime_suspend hook just to somehow trigger the runtime_suspend of the
> > DMA driver.
And I consider doing this a crude hack. Here is what I suggested doing
instead:
> >
> > What you should do instead is to make sure the hook runtime_pm to the
> > DMA drivers activity phases, like for example the pl330 driver does.
> > Then you wouldn't have to care about manually putting the DMA driver into
> > suspend from the GPMI NAND driver.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Han Xu <[email protected]> writes:
> set the correct pinctrl state in system pm suspend/resume ops
>
> Signed-off-by: Han Xu <[email protected]>
> ---
> drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 73644c96fa9b..de1e3dbb2eb1 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -15,6 +15,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> #include <linux/dma/mxs-dma.h>
> #include "gpmi-nand.h"
> #include "gpmi-regs.h"
> @@ -2692,6 +2693,7 @@ static int gpmi_pm_suspend(struct device *dev)
> {
> int ret;
>
> + pinctrl_pm_select_sleep_state(dev);
> ret = pm_runtime_force_suspend(dev);
>
> return ret;
> @@ -2708,6 +2710,8 @@ static int gpmi_pm_resume(struct device *dev)
> return ret;
> }
>
> + pinctrl_pm_select_default_state(dev);
> +
> /* re-init the GPMI registers */
> ret = gpmi_init(this);
> if (ret) {
Acked-by: Esben Haabendal <[email protected]>