| Message ID | 20241002153944.69733-2-han.xu@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] mtd: nand: raw: gpmi: switch to SYSTEM_SLEEP_PM_OPS | expand |
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 >
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
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
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
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
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
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
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 = {
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(-)