linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [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).