| Message ID | 20210903212758.22038-1-mahesh.r.vaidya@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] spi: dw: Enable Autosuspend delay for SPI DesignWare | expand |
On Sat, Sep 04, 2021 at 05:27:58AM +0800, mahesh.r.vaidya@intel.com wrote: > From: Mahesh R Vaidya <mahesh.r.vaidya@intel.com> > > Enable and set Autosuspend delay for SPI DesignWare driver. > The number 1000 ms for the autosuspend delay was picked a bit > arbitrarily, so if someone has measurements showing a better > value we could easily change this. > > Signed-off-by: Mahesh R Vaidya <mahesh.r.vaidya@intel.com> > --- > drivers/spi/spi-dw-mmio.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > index 3379720cfcb8..8b588ce9c16b 100644 > --- a/drivers/spi/spi-dw-mmio.c > +++ b/drivers/spi/spi-dw-mmio.c > @@ -33,6 +33,8 @@ struct dw_spi_mmio { > struct reset_control *rstc; > }; > > +#define SPI_DW_DEFAULT_AUTOSUSP_VAL 1000 > + > #define MSCC_CPU_SYSTEM_CTRL_GENERAL_CTRL 0x24 > #define OCELOT_IF_SI_OWNER_OFFSET 4 > #define JAGUAR2_IF_SI_OWNER_OFFSET 6 > @@ -309,6 +311,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) > goto out; > } > > + /* Set initial autosuspend default delay value and enable */ > + pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_DW_DEFAULT_AUTOSUSP_VAL); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); Seems reasonable, but doesn't it need to be reverted in the remove callback? Like it's done in the spi-imx.c driver for instance by calling the pm_runtime_dont_use_autosuspend() method. > pm_runtime_enable(&pdev->dev); > > ret = dw_spi_add_host(&pdev->dev, dws); > @@ -319,6 +325,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) > return 0; > > out: > + pm_runtime_put_noidle(&pdev->dev); This doesn't seem like related to the subject, thus needs to be submitted as a separate patch with fixes tag if it fixes some another problem. Additionally it isn't balanced with the pm_runtime_get*() method invocation. Am I missing something? -Sergey > pm_runtime_disable(&pdev->dev); > clk_disable_unprepare(dwsmmio->pclk); > out_clk: > -- > 2.17.1 >
> -----Original Message----- > From: Serge Semin <fancer.lancer@gmail.com> > Sent: Sunday, September 5, 2021 8:49 PM > To: Vaidya, Mahesh R <mahesh.r.vaidya@intel.com> > Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>; broonie@kernel.org; > linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > mgross@linux.intel.com; Pan, Kris <kris.pan@intel.com>; Zhou, Furong > <furong.zhou@intel.com>; Sangannavar, Mallikarjunappa > <mallikarjunappa.sangannavar@intel.com>; Raja Subramanian, Lakshmi Bai > <lakshmi.bai.raja.subramanian@intel.com> > Subject: Re: [PATCH v1] spi: dw: Enable Autosuspend delay for SPI > DesignWare > > On Sat, Sep 04, 2021 at 05:27:58AM +0800, mahesh.r.vaidya@intel.com > wrote: > > From: Mahesh R Vaidya <mahesh.r.vaidya@intel.com> > > > > Enable and set Autosuspend delay for SPI DesignWare driver. > > The number 1000 ms for the autosuspend delay was picked a bit > > arbitrarily, so if someone has measurements showing a better value we > > could easily change this. > > > > Signed-off-by: Mahesh R Vaidya <mahesh.r.vaidya@intel.com> > > --- > > drivers/spi/spi-dw-mmio.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > > index 3379720cfcb8..8b588ce9c16b 100644 > > --- a/drivers/spi/spi-dw-mmio.c > > +++ b/drivers/spi/spi-dw-mmio.c > > @@ -33,6 +33,8 @@ struct dw_spi_mmio { > > struct reset_control *rstc; > > }; > > > > +#define SPI_DW_DEFAULT_AUTOSUSP_VAL 1000 > > + > > #define MSCC_CPU_SYSTEM_CTRL_GENERAL_CTRL 0x24 > > #define OCELOT_IF_SI_OWNER_OFFSET 4 > > #define JAGUAR2_IF_SI_OWNER_OFFSET 6 > > @@ -309,6 +311,10 @@ static int dw_spi_mmio_probe(struct > platform_device *pdev) > > goto out; > > } > > > > > + /* Set initial autosuspend default delay value and enable */ > > + pm_runtime_set_autosuspend_delay(&pdev->dev, > SPI_DW_DEFAULT_AUTOSUSP_VAL); > > + pm_runtime_use_autosuspend(&pdev->dev); > > + pm_runtime_set_active(&pdev->dev); > > Seems reasonable, but doesn't it need to be reverted in the remove callback? > Like it's done in the spi-imx.c driver for instance by calling the > pm_runtime_dont_use_autosuspend() method. > I initially referred to the below two commits which enable the autosuspend feature for spi-rockchip and spi-sun6i, hence had followed the same. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=940f3bbf3dacd6e31e482a10e64c96e69b00dded https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ae0f18bef30d0e76dd62be46c59b24c75f175092 But, yes, I Agree with you, right way would be to handle the reverted calls in the remove part of the code. I will modify the change accordingly in the next patch. > > pm_runtime_enable(&pdev->dev); > > > > ret = dw_spi_add_host(&pdev->dev, dws); @@ -319,6 +325,7 @@ > static > > int dw_spi_mmio_probe(struct platform_device *pdev) > > return 0; > > > > out: > > > + pm_runtime_put_noidle(&pdev->dev); > > This doesn't seem like related to the subject, thus needs to be submitted as a > separate patch with fixes tag if it fixes some another problem. Additionally it > isn't balanced with the pm_runtime_get*() method invocation. Am I missing > something? > > -Sergey > I checked this further and pm_runtime_put_noidle() seems to be redundant in my use case. Hence, I will remove this in the next patch. Regards, Mahesh > > pm_runtime_disable(&pdev->dev); > > clk_disable_unprepare(dwsmmio->pclk); > > out_clk: > > -- > > 2.17.1 > >
diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c index 3379720cfcb8..8b588ce9c16b 100644 --- a/drivers/spi/spi-dw-mmio.c +++ b/drivers/spi/spi-dw-mmio.c @@ -33,6 +33,8 @@ struct dw_spi_mmio { struct reset_control *rstc; }; +#define SPI_DW_DEFAULT_AUTOSUSP_VAL 1000 + #define MSCC_CPU_SYSTEM_CTRL_GENERAL_CTRL 0x24 #define OCELOT_IF_SI_OWNER_OFFSET 4 #define JAGUAR2_IF_SI_OWNER_OFFSET 6 @@ -309,6 +311,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) goto out; } + /* Set initial autosuspend default delay value and enable */ + pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_DW_DEFAULT_AUTOSUSP_VAL); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); ret = dw_spi_add_host(&pdev->dev, dws); @@ -319,6 +325,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) return 0; out: + pm_runtime_put_noidle(&pdev->dev); pm_runtime_disable(&pdev->dev); clk_disable_unprepare(dwsmmio->pclk); out_clk: