* [PATCH v2 1/2] mtd: nand: raw: gpmi: switch to SYSTEM_SLEEP_PM_OPS
@ 2024-10-02 15:39 Han Xu
2024-10-02 15:39 ` [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling Han Xu
0 siblings, 1 reply; 9+ messages in thread
From: Han Xu @ 2024-10-02 15:39 UTC (permalink / raw)
To: Han Xu, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra
Cc: imx, linux-mtd, linux-kernel
Replace the SET_SYSTEM_SLEEP_PM_OPS with modern SYSTEM_SLEEP_PM_OPS
alternatives.
Signed-off-by: Han Xu <han.xu@nxp.com>
---
Changes in v2:
- Add the missing pm_ptr() macro
---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index e1b515304e3c..a7d79679248d 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -2811,7 +2811,6 @@ static void gpmi_nand_remove(struct platform_device *pdev)
release_resources(this);
}
-#ifdef CONFIG_PM_SLEEP
static int gpmi_pm_suspend(struct device *dev)
{
struct gpmi_nand_data *this = dev_get_drvdata(dev);
@@ -2849,7 +2848,6 @@ static int gpmi_pm_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static int __maybe_unused gpmi_runtime_suspend(struct device *dev)
{
@@ -2866,14 +2864,14 @@ static int __maybe_unused gpmi_runtime_resume(struct device *dev)
}
static const struct dev_pm_ops gpmi_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(gpmi_pm_suspend, gpmi_pm_resume)
- SET_RUNTIME_PM_OPS(gpmi_runtime_suspend, gpmi_runtime_resume, NULL)
+ SYSTEM_SLEEP_PM_OPS(gpmi_pm_suspend, gpmi_pm_resume)
+ RUNTIME_PM_OPS(gpmi_runtime_suspend, gpmi_runtime_resume, NULL)
};
static struct platform_driver gpmi_nand_driver = {
.driver = {
.name = "gpmi-nand",
- .pm = &gpmi_pm_ops,
+ .pm = pm_ptr(&gpmi_pm_ops),
.of_match_table = gpmi_nand_id_table,
},
.probe = gpmi_nand_probe,
--
2.34.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-02 15:39 [PATCH v2 1/2] mtd: nand: raw: gpmi: switch to SYSTEM_SLEEP_PM_OPS Han Xu
@ 2024-10-02 15:39 ` Han Xu
2024-10-02 16:23 ` Frank Li
2024-10-03 7:38 ` Miquel Raynal
0 siblings, 2 replies; 9+ messages in thread
From: Han Xu @ 2024-10-02 15:39 UTC (permalink / raw)
To: Han Xu, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra
Cc: imx, linux-mtd, linux-kernel
The commit refactors the power management handling in the gpmi nand
driver. It removes redundant pm_runtime calls in the probe function,
handles the pad control in suspend and resume, and moves the calls to
acquire and release DMA channels to the runtime suspend and resume
functions.
Signed-off-by: Han Xu <han.xu@nxp.com>
---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 66 ++++++++++++----------
1 file changed, 37 insertions(+), 29 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index a7d79679248d..76e4b89e2451 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/platform_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"
@@ -737,9 +738,8 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
if (ret)
return ret;
- ret = pm_runtime_get_sync(this->dev);
+ ret = pm_runtime_resume_and_get(this->dev);
if (ret < 0) {
- pm_runtime_put_autosuspend(this->dev);
return ret;
}
@@ -1301,10 +1301,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;
@@ -2761,15 +2757,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_acquire_resources;
-
+ 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)
@@ -2779,15 +2769,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:
@@ -2801,22 +2788,23 @@ static void gpmi_nand_remove(struct platform_device *pdev)
struct nand_chip *chip = &this->nand;
int ret;
- pm_runtime_put_sync(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
-
ret = mtd_device_unregister(nand_to_mtd(chip));
WARN_ON(ret);
nand_cleanup(chip);
gpmi_free_dma_buffer(this);
release_resources(this);
+ pm_runtime_dont_use_autosuspend(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
}
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;
+ pinctrl_pm_select_sleep_state(dev);
+ ret = pm_runtime_force_suspend(dev);
+
+ return ret;
}
static int gpmi_pm_resume(struct device *dev)
@@ -2824,9 +2812,13 @@ 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;
+ }
+
+ pinctrl_pm_select_default_state(dev);
/* re-init the GPMI registers */
ret = gpmi_init(this);
@@ -2849,18 +2841,34 @@ static int gpmi_pm_resume(struct device *dev)
return 0;
}
-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;
+
+ ret = gpmi_enable_clk(this);
+ if (ret)
+ return ret;
+
+ ret = acquire_dma_channels(this);
+ if (ret < 0)
+ return ret;
+
+ return 0;
- return __gpmi_enable_clk(this, true);
}
static const struct dev_pm_ops gpmi_pm_ops = {
--
2.34.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-02 15:39 ` [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling Han Xu
@ 2024-10-02 16:23 ` Frank Li
2024-10-03 7:38 ` Miquel Raynal
1 sibling, 0 replies; 9+ messages in thread
From: Frank Li @ 2024-10-02 16:23 UTC (permalink / raw)
To: Han Xu
Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, imx,
linux-mtd, linux-kernel
On Wed, Oct 02, 2024 at 10:39:44AM -0500, Han Xu wrote:
> The commit refactors the power management handling in the gpmi nand
> driver. It removes redundant pm_runtime calls in the probe function,
> handles the pad control in suspend and resume, and moves the calls to
> acquire and release DMA channels to the runtime suspend and resume
> functions.
Use imperative mode
Refactor the power management. Remove redundate pm_runtime calls and pad
control in the probe function. Move acquire and release DMA channels to
the runtime suspend and resume because dma engine use DMA channel used/free
to do power saving.
>
> Signed-off-by: Han Xu <han.xu@nxp.com>
> ---
> drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 66 ++++++++++++----------
> 1 file changed, 37 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index a7d79679248d..76e4b89e2451 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/platform_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
order by alphabet
> #include <linux/dma/mxs-dma.h>
> #include "gpmi-nand.h"
> #include "gpmi-regs.h"
> @@ -737,9 +738,8 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
> if (ret)
> return ret;
>
> - ret = pm_runtime_get_sync(this->dev);
> + ret = pm_runtime_resume_and_get(this->dev);
> if (ret < 0) {
> - pm_runtime_put_autosuspend(this->dev);
> return ret;
> }
>
> @@ -1301,10 +1301,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;
> @@ -2761,15 +2757,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_acquire_resources;
> -
> + 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)
> @@ -2779,15 +2769,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:
> @@ -2801,22 +2788,23 @@ static void gpmi_nand_remove(struct platform_device *pdev)
> struct nand_chip *chip = &this->nand;
> int ret;
>
> - pm_runtime_put_sync(&pdev->dev);
> - pm_runtime_disable(&pdev->dev);
> -
> ret = mtd_device_unregister(nand_to_mtd(chip));
> WARN_ON(ret);
> nand_cleanup(chip);
> gpmi_free_dma_buffer(this);
> release_resources(this);
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> }
>
> 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;
> + pinctrl_pm_select_sleep_state(dev);
> + ret = pm_runtime_force_suspend(dev);
> +
> + return ret;
needn't define varible ret
return pm_runtime_force_suspend(dev);
> }
>
> static int gpmi_pm_resume(struct device *dev)
> @@ -2824,9 +2812,13 @@ 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;
> + }
> +
> + pinctrl_pm_select_default_state(dev);
>
> /* re-init the GPMI registers */
> ret = gpmi_init(this);
> @@ -2849,18 +2841,34 @@ static int gpmi_pm_resume(struct device *dev)
> return 0;
> }
>
> -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)
remove __maybe should below previous patch.
> {
> 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;
> +
> + ret = gpmi_enable_clk(this);
> + if (ret)
> + return ret;
> +
> + ret = acquire_dma_channels(this);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
>
> - return __gpmi_enable_clk(this, true);
It will be simple if
return acquire_dma_channels(this);
> }
>
> static const struct dev_pm_ops gpmi_pm_ops = {
> --
> 2.34.1
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-02 15:39 ` [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling Han Xu
2024-10-02 16:23 ` Frank Li
@ 2024-10-03 7:38 ` Miquel Raynal
2024-10-03 15:05 ` Han Xu
1 sibling, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2024-10-03 7:38 UTC (permalink / raw)
To: Han Xu
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
Hi Han,
han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
> The commit refactors the power management handling in the gpmi nand
> driver. It removes redundant pm_runtime calls in the probe function,
> handles the pad control in suspend and resume, and moves the calls to
> acquire and release DMA channels to the runtime suspend and resume
> functions.
May I know the motivation to acquire and release the DMA channels
during suspend? In general it seems like a different change which I'd
prefer to see in its own commit with a justification. The rest looks
ok otherwise.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-03 7:38 ` Miquel Raynal
@ 2024-10-03 15:05 ` Han Xu
2024-10-03 15:28 ` Miquel Raynal
0 siblings, 1 reply; 9+ messages in thread
From: Han Xu @ 2024-10-03 15:05 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
On 24/10/03 09:38AM, Miquel Raynal wrote:
> Hi Han,
>
> han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
>
> > The commit refactors the power management handling in the gpmi nand
> > driver. It removes redundant pm_runtime calls in the probe function,
> > handles the pad control in suspend and resume, and moves the calls to
> > acquire and release DMA channels to the runtime suspend and resume
> > functions.
>
> May I know the motivation to acquire and release the DMA channels
> during suspend? In general it seems like a different change which I'd
> prefer to see in its own commit with a justification. The rest looks
> ok otherwise.
Hi Miquel,
Thanks for your comments. IMHO there is no much logic changes indeed, just move
the dma channel acquire and release from system pm to the runtime pm, releasing
the unused resources as early as possible. If you think it's necessary I will
split the patch into two parts.
>
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-03 15:05 ` Han Xu
@ 2024-10-03 15:28 ` Miquel Raynal
2024-10-03 20:05 ` Han Xu
0 siblings, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2024-10-03 15:28 UTC (permalink / raw)
To: Han Xu
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
Hi Han,
han.xu@nxp.com wrote on Thu, 3 Oct 2024 10:05:16 -0500:
> On 24/10/03 09:38AM, Miquel Raynal wrote:
> > Hi Han,
> >
> > han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
> >
> > > The commit refactors the power management handling in the gpmi nand
> > > driver. It removes redundant pm_runtime calls in the probe function,
> > > handles the pad control in suspend and resume, and moves the calls to
> > > acquire and release DMA channels to the runtime suspend and resume
> > > functions.
> >
> > May I know the motivation to acquire and release the DMA channels
> > during suspend? In general it seems like a different change which I'd
> > prefer to see in its own commit with a justification. The rest looks
> > ok otherwise.
>
> Hi Miquel,
>
> Thanks for your comments. IMHO there is no much logic changes indeed, just move
> the dma channel acquire and release from system pm to the runtime pm, releasing
> the unused resources as early as possible. If you think it's necessary I will
> split the patch into two parts.
Actually I don't understand why these channels are released and
acquired again. Does it make sense to do that in the (runtime)
suspend/resume path? I'd be in favor of avoiding this extra
configuration which as a first sight does not seem required here.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-03 15:28 ` Miquel Raynal
@ 2024-10-03 20:05 ` Han Xu
2024-10-04 7:21 ` Miquel Raynal
0 siblings, 1 reply; 9+ messages in thread
From: Han Xu @ 2024-10-03 20:05 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
On 24/10/03 05:28PM, Miquel Raynal wrote:
> Hi Han,
>
> han.xu@nxp.com wrote on Thu, 3 Oct 2024 10:05:16 -0500:
>
> > On 24/10/03 09:38AM, Miquel Raynal wrote:
> > > Hi Han,
> > >
> > > han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
> > >
> > > > The commit refactors the power management handling in the gpmi nand
> > > > driver. It removes redundant pm_runtime calls in the probe function,
> > > > handles the pad control in suspend and resume, and moves the calls to
> > > > acquire and release DMA channels to the runtime suspend and resume
> > > > functions.
> > >
> > > May I know the motivation to acquire and release the DMA channels
> > > during suspend? In general it seems like a different change which I'd
> > > prefer to see in its own commit with a justification. The rest looks
> > > ok otherwise.
> >
> > Hi Miquel,
> >
> > Thanks for your comments. IMHO there is no much logic changes indeed, just move
> > the dma channel acquire and release from system pm to the runtime pm, releasing
> > the unused resources as early as possible. If you think it's necessary I will
> > split the patch into two parts.
>
> Actually I don't understand why these channels are released and
> acquired again. Does it make sense to do that in the (runtime)
> suspend/resume path? I'd be in favor of avoiding this extra
> configuration which as a first sight does not seem required here.
Our local mxs-dma driver (will upstream the changes later) implemented the runtime
resume/suspend in channel alloc/release functions, so I did this in the gpmi
nand driver suspend/resume path.
>
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-03 20:05 ` Han Xu
@ 2024-10-04 7:21 ` Miquel Raynal
2024-10-07 9:52 ` Miquel Raynal
0 siblings, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2024-10-04 7:21 UTC (permalink / raw)
To: Han Xu
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
Hi Han,
han.xu@nxp.com wrote on Thu, 3 Oct 2024 15:05:08 -0500:
> On 24/10/03 05:28PM, Miquel Raynal wrote:
> > Hi Han,
> >
> > han.xu@nxp.com wrote on Thu, 3 Oct 2024 10:05:16 -0500:
> >
> > > On 24/10/03 09:38AM, Miquel Raynal wrote:
> > > > Hi Han,
> > > >
> > > > han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
> > > >
> > > > > The commit refactors the power management handling in the gpmi nand
> > > > > driver. It removes redundant pm_runtime calls in the probe function,
> > > > > handles the pad control in suspend and resume, and moves the calls to
> > > > > acquire and release DMA channels to the runtime suspend and resume
> > > > > functions.
> > > >
> > > > May I know the motivation to acquire and release the DMA channels
> > > > during suspend? In general it seems like a different change which I'd
> > > > prefer to see in its own commit with a justification. The rest looks
> > > > ok otherwise.
> > >
> > > Hi Miquel,
> > >
> > > Thanks for your comments. IMHO there is no much logic changes indeed, just move
> > > the dma channel acquire and release from system pm to the runtime pm, releasing
> > > the unused resources as early as possible. If you think it's necessary I will
> > > split the patch into two parts.
> >
> > Actually I don't understand why these channels are released and
> > acquired again. Does it make sense to do that in the (runtime)
> > suspend/resume path? I'd be in favor of avoiding this extra
> > configuration which as a first sight does not seem required here.
>
> Our local mxs-dma driver (will upstream the changes later) implemented the runtime
> resume/suspend in channel alloc/release functions, so I did this in the gpmi
> nand driver suspend/resume path.
I think it no longer makes sense. RPM takes care of the
suppliers, at least since fw_devlink=rpm.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling
2024-10-04 7:21 ` Miquel Raynal
@ 2024-10-07 9:52 ` Miquel Raynal
0 siblings, 0 replies; 9+ messages in thread
From: Miquel Raynal @ 2024-10-07 9:52 UTC (permalink / raw)
To: Han Xu
Cc: Richard Weinberger, Vignesh Raghavendra, imx, linux-mtd,
linux-kernel
Hi Miquel,
miquel.raynal@bootlin.com wrote on Fri, 4 Oct 2024 09:21:44 +0200:
> Hi Han,
>
> han.xu@nxp.com wrote on Thu, 3 Oct 2024 15:05:08 -0500:
>
> > On 24/10/03 05:28PM, Miquel Raynal wrote:
> > > Hi Han,
> > >
> > > han.xu@nxp.com wrote on Thu, 3 Oct 2024 10:05:16 -0500:
> > >
> > > > On 24/10/03 09:38AM, Miquel Raynal wrote:
> > > > > Hi Han,
> > > > >
> > > > > han.xu@nxp.com wrote on Wed, 2 Oct 2024 10:39:44 -0500:
> > > > >
> > > > > > The commit refactors the power management handling in the gpmi nand
> > > > > > driver. It removes redundant pm_runtime calls in the probe function,
> > > > > > handles the pad control in suspend and resume, and moves the calls to
> > > > > > acquire and release DMA channels to the runtime suspend and resume
> > > > > > functions.
> > > > >
> > > > > May I know the motivation to acquire and release the DMA channels
> > > > > during suspend? In general it seems like a different change which I'd
> > > > > prefer to see in its own commit with a justification. The rest looks
> > > > > ok otherwise.
> > > >
> > > > Hi Miquel,
> > > >
> > > > Thanks for your comments. IMHO there is no much logic changes indeed, just move
> > > > the dma channel acquire and release from system pm to the runtime pm, releasing
> > > > the unused resources as early as possible. If you think it's necessary I will
> > > > split the patch into two parts.
> > >
> > > Actually I don't understand why these channels are released and
> > > acquired again. Does it make sense to do that in the (runtime)
> > > suspend/resume path? I'd be in favor of avoiding this extra
> > > configuration which as a first sight does not seem required here.
> >
> > Our local mxs-dma driver (will upstream the changes later) implemented the runtime
> > resume/suspend in channel alloc/release functions, so I did this in the gpmi
> > nand driver suspend/resume path.
>
> I think it no longer makes sense. RPM takes care of the
> suppliers, at least since fw_devlink=rpm.
Just to be clear, you can send v3 addressing Frank's comment and just
keep the DMA channels handling as it is, but please work on removing
this hack of dropping/allocating DMA channels in suspend/resume path, it
is really not worth the trouble and purely useless with any recent
kernel.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-07 9:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-02 15:39 [PATCH v2 1/2] mtd: nand: raw: gpmi: switch to SYSTEM_SLEEP_PM_OPS Han Xu
2024-10-02 15:39 ` [PATCH v2 2/2] mtd: nand: raw: gpmi: improve power management handling Han Xu
2024-10-02 16:23 ` Frank Li
2024-10-03 7:38 ` Miquel Raynal
2024-10-03 15:05 ` Han Xu
2024-10-03 15:28 ` Miquel Raynal
2024-10-03 20:05 ` Han Xu
2024-10-04 7:21 ` Miquel Raynal
2024-10-07 9:52 ` Miquel Raynal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).