[2/3] spi: sh-msiof: Convert to spi core auto_runtime_pm framework

Commit Message

Geert Uytterhoeven March 11, 2014, 9:59 a.m. UTC

From: Geert Uytterhoeven <[e-mail]>

Signed-off-by: Geert Uytterhoeven <[e-mail]>
Cc: Magnus Damm <[e-mail]>
---
 drivers/spi/spi-sh-msiof.c |   25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

Comments

Mark Brown March 11, 2014, 10:45 a.m. UTC | #1

On Tue, Mar 11, 2014 at 10:59:11AM +0100, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <[e-mail]>

Applied, thanks.

Laurent Pinchart March 11, 2014, 3:55 p.m. UTC | #2

Hi Geert,

Thank you for the patch.

Does this require drivers/sh/pm_runtime.c to be compiled in ?

On Tuesday 11 March 2014 10:59:11 Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <[e-mail]>
> 
> Signed-off-by: Geert Uytterhoeven <[e-mail]>
> Cc: Magnus Damm <[e-mail]>
> ---
>  drivers/spi/spi-sh-msiof.c |   25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 739eb2f12ecc..e850d03e7190 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -448,9 +448,6 @@ static int sh_msiof_prepare_message(struct spi_master
> *master, struct sh_msiof_spi_priv *p = spi_master_get_devdata(master);
>  	const struct spi_device *spi = msg->spi;
> 
> -	pm_runtime_get_sync(&p->pdev->dev);
> -	clk_enable(p->clk);
> -
>  	/* Configure pins before asserting CS */
>  	sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
>  				  !!(spi->mode & SPI_CPHA),
> @@ -460,16 +457,6 @@ static int sh_msiof_prepare_message(struct spi_master
> *master, return 0;
>  }
> 
> -static int sh_msiof_unprepare_message(struct spi_master *master,
> -				      struct spi_message *msg)
> -{
> -	struct sh_msiof_spi_priv *p = spi_master_get_devdata(master);
> -
> -	clk_disable(p->clk);
> -	pm_runtime_put(&p->pdev->dev);
> -	return 0;
> -}
> -
>  static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
>  				  void (*tx_fifo)(struct sh_msiof_spi_priv *,
>  						  const void *, int, int),
> @@ -742,12 +729,6 @@ static int sh_msiof_spi_probe(struct platform_device
> *pdev) goto err1;
>  	}
> 
> -	ret = clk_prepare(p->clk);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "unable to prepare clock\n");
> -		goto err1;
> -	}
> -
>  	p->pdev = pdev;
>  	pm_runtime_enable(&pdev->dev);
> 
> @@ -768,8 +749,8 @@ static int sh_msiof_spi_probe(struct platform_device
> *pdev) master->num_chipselect = p->info->num_chipselect;
>  	master->setup = sh_msiof_spi_setup;
>  	master->prepare_message = sh_msiof_prepare_message;
> -	master->unprepare_message = sh_msiof_unprepare_message;
>  	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32);
> +	master->auto_runtime_pm = true;
>  	master->transfer_one = sh_msiof_transfer_one;
> 
>  	ret = devm_spi_register_master(&pdev->dev, master);
> @@ -782,7 +763,6 @@ static int sh_msiof_spi_probe(struct platform_device
> *pdev)
> 
>   err2:
>  	pm_runtime_disable(&pdev->dev);
> -	clk_unprepare(p->clk);
>   err1:
>  	spi_master_put(master);
>  	return ret;
> @@ -790,10 +770,7 @@ static int sh_msiof_spi_probe(struct platform_device
> *pdev)
> 
>  static int sh_msiof_spi_remove(struct platform_device *pdev)
>  {
> -	struct sh_msiof_spi_priv *p = platform_get_drvdata(pdev);
> -
>  	pm_runtime_disable(&pdev->dev);
> -	clk_unprepare(p->clk);
>  	return 0;
>  }

Geert Uytterhoeven March 11, 2014, 4:23 p.m. UTC | #3

Hi Laurent,

On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart
<[e-mail]> wrote:
> Does this require drivers/sh/pm_runtime.c to be compiled in ?

Let's check...

My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
My koelsch-reference kernel hasn't.

However, under the -reference kernel many MSTP clocks (incl. MSIOF)
seem to be enabled all the time, while under -legacy they are enabled and
disabled on demand.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart March 11, 2014, 4:32 p.m. UTC | #4

On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
> Hi Laurent,
> 
> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
> 
> Let's check...
> 
> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
> My koelsch-reference kernel hasn't.
> 
> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
> seem to be enabled all the time, while under -legacy they are enabled and
> disabled on demand.

Is PM_RUNTIME enabled in both cases ?


There's something fishy in there that we should try to fix without too further 
much delay. Ben Dooks has pointed out the problem a couple of months ago, but 
the discussion on the mailing list just died.

Geert Uytterhoeven March 11, 2014, 5:44 p.m. UTC | #5

Hi Laurent,

On Tue, Mar 11, 2014 at 5:32 PM, Laurent Pinchart
<[e-mail]> wrote:
> On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
>> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
>> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
>>
>> Let's check...
>>
>> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
>> My koelsch-reference kernel hasn't.
>>
>> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
>> seem to be enabled all the time, while under -legacy they are enabled and
>> disabled on demand.
>
> Is PM_RUNTIME enabled in both cases ?

Yes it is.

> There's something fishy in there that we should try to fix without too further
> much delay. Ben Dooks has pointed out the problem a couple of months ago, but
> the discussion on the mailing list just died.

Indeed. I'll look into it next week, unless someone beats me to it.

Note that I dropped Ben's patch to include drivers/sh, as I can't boot into
userspace with it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart March 11, 2014, 5:57 p.m. UTC | #6

Hi Geert,

On Tuesday 11 March 2014 18:44:21 Geert Uytterhoeven wrote:
> On Tue, Mar 11, 2014 at 5:32 PM, Laurent Pinchart wrote:
> > On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
> >> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
> >> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
> >> 
> >> Let's check...
> >> 
> >> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
> >> My koelsch-reference kernel hasn't.
> >> 
> >> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
> >> seem to be enabled all the time, while under -legacy they are enabled and
> >> disabled on demand.
> > 
> > Is PM_RUNTIME enabled in both cases ?
> 
> Yes it is.
> 
> > There's something fishy in there that we should try to fix without too
> > further much delay. Ben Dooks has pointed out the problem a couple of
> > months ago, but the discussion on the mailing list just died.
> 
> Indeed. I'll look into it next week, unless someone beats me to it.
> 
> Note that I dropped Ben's patch to include drivers/sh, as I can't boot into
> userspace with it.

The patch also had the issue of not being compatible with multiplatform 
kernels, as it resulted in Renesas-specific code being executed for all 
platforms.

If you can find the old mail thread you can just reply to it (CC'ing me) and 
I'll make sure to refresh my memory and explain my concerns again.

Magnus Damm March 12, 2014, 1:23 a.m. UTC | #7

On Wed, Mar 12, 2014 at 1:32 AM, Laurent Pinchart
<[e-mail]> wrote:
> On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
>> Hi Laurent,
>>
>> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
>> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
>>
>> Let's check...
>>
>> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
>> My koelsch-reference kernel hasn't.
>>
>> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
>> seem to be enabled all the time, while under -legacy they are enabled and
>> disabled on demand.
>
> Is PM_RUNTIME enabled in both cases ?
>
>
> There's something fishy in there that we should try to fix without too further
> much delay. Ben Dooks has pointed out the problem a couple of months ago, but
> the discussion on the mailing list just died.

Yes, Runtime PM is not working as expected in the multiplatform case,
that is true. I propose that we keep Runtime PM disabled in the
Kconfig for R-Car Gen2 for now to keep things simple.

From my side I see it as two separate solutions. The short term fix is
to temporarily work around drivers that depend on Runtime PM for clock
control, I propose enabling selected clocks statically using the
function introduced by this series:

[PATCH 00/03] ARM: shmobile: Break out and extend clock workaround
http://www.spinics.net/lists/arm-kernel/msg310278.html

The long term fix I'm not sure sure about, but I trust Geert to figure
it out. =)

Regardless, rushing to fix this "correctly" seems as useful to me as
dead line driven DT development....

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart March 12, 2014, 11:26 a.m. UTC | #8

Hi Magnus,

On Wednesday 12 March 2014 10:23:48 Magnus Damm wrote:
> On Wed, Mar 12, 2014 at 1:32 AM, Laurent Pinchart wrote:
> > On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
> >> Hi Laurent,
> >> 
> >> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
> >> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
> >> 
> >> Let's check...
> >> 
> >> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
> >> My koelsch-reference kernel hasn't.
> >> 
> >> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
> >> seem to be enabled all the time, while under -legacy they are enabled and
> >> disabled on demand.
> > 
> > Is PM_RUNTIME enabled in both cases ?
> > 
> > There's something fishy in there that we should try to fix without too
> > further much delay. Ben Dooks has pointed out the problem a couple of
> > months ago, but the discussion on the mailing list just died.
> 
> Yes, Runtime PM is not working as expected in the multiplatform case,
> that is true. I propose that we keep Runtime PM disabled in the
> Kconfig for R-Car Gen2 for now to keep things simple.

Isn't it ? I thought it was only broken with regard to clocks, but I might be 
missing something.

> From my side I see it as two separate solutions. The short term fix is
> to temporarily work around drivers that depend on Runtime PM for clock
> control, I propose enabling selected clocks statically using the
> function introduced by this series:
> 
> [PATCH 00/03] ARM: shmobile: Break out and extend clock workaround
> http://www.spinics.net/lists/arm-kernel/msg310278.html
> 
> The long term fix I'm not sure sure about, but I trust Geert to figure
> it out. =)
> 
> Regardless, rushing to fix this "correctly" seems as useful to me as
> dead line driven DT development....

Magnus Damm March 12, 2014, 11:28 a.m. UTC | #9

Hi Laurent,

On Wed, Mar 12, 2014 at 8:26 PM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> On Wednesday 12 March 2014 10:23:48 Magnus Damm wrote:
>> On Wed, Mar 12, 2014 at 1:32 AM, Laurent Pinchart wrote:
>> > On Tuesday 11 March 2014 17:23:37 Geert Uytterhoeven wrote:
>> >> Hi Laurent,
>> >>
>> >> On Tue, Mar 11, 2014 at 4:55 PM, Laurent Pinchart wrote:
>> >> > Does this require drivers/sh/pm_runtime.c to be compiled in ?
>> >>
>> >> Let's check...
>> >>
>> >> My koelsch-legacy kernel has drivers/sh/pm_runtime.c compiled in.
>> >> My koelsch-reference kernel hasn't.
>> >>
>> >> However, under the -reference kernel many MSTP clocks (incl. MSIOF)
>> >> seem to be enabled all the time, while under -legacy they are enabled and
>> >> disabled on demand.
>> >
>> > Is PM_RUNTIME enabled in both cases ?
>> >
>> > There's something fishy in there that we should try to fix without too
>> > further much delay. Ben Dooks has pointed out the problem a couple of
>> > months ago, but the discussion on the mailing list just died.
>>
>> Yes, Runtime PM is not working as expected in the multiplatform case,
>> that is true. I propose that we keep Runtime PM disabled in the
>> Kconfig for R-Car Gen2 for now to keep things simple.
>
> Isn't it ? I thought it was only broken with regard to clocks, but I might be
> missing something.

Perhaps the correct way to put it is that we have not yet figured out
how to enable Runtime PM in a way that is suitable for multiplatform.
Which means that clocks are behaving differently in the legacy case
and in the multiplatform case.

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 739eb2f12ecc..e850d03e7190 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -448,9 +448,6 @@  static int sh_msiof_prepare_message(struct spi_master *master,
 	struct sh_msiof_spi_priv *p = spi_master_get_devdata(master);
 	const struct spi_device *spi = msg->spi;
 
-	pm_runtime_get_sync(&p->pdev->dev);
-	clk_enable(p->clk);
-
 	/* Configure pins before asserting CS */
 	sh_msiof_spi_set_pin_regs(p, !!(spi->mode & SPI_CPOL),
 				  !!(spi->mode & SPI_CPHA),
@@ -460,16 +457,6 @@  static int sh_msiof_prepare_message(struct spi_master *master,
 	return 0;
 }
 
-static int sh_msiof_unprepare_message(struct spi_master *master,
-				      struct spi_message *msg)
-{
-	struct sh_msiof_spi_priv *p = spi_master_get_devdata(master);
-
-	clk_disable(p->clk);
-	pm_runtime_put(&p->pdev->dev);
-	return 0;
-}
-
 static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p,
 				  void (*tx_fifo)(struct sh_msiof_spi_priv *,
 						  const void *, int, int),
@@ -742,12 +729,6 @@  static int sh_msiof_spi_probe(struct platform_device *pdev)
 		goto err1;
 	}
 
-	ret = clk_prepare(p->clk);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "unable to prepare clock\n");
-		goto err1;
-	}
-
 	p->pdev = pdev;
 	pm_runtime_enable(&pdev->dev);
 
@@ -768,8 +749,8 @@  static int sh_msiof_spi_probe(struct platform_device *pdev)
 	master->num_chipselect = p->info->num_chipselect;
 	master->setup = sh_msiof_spi_setup;
 	master->prepare_message = sh_msiof_prepare_message;
-	master->unprepare_message = sh_msiof_unprepare_message;
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32);
+	master->auto_runtime_pm = true;
 	master->transfer_one = sh_msiof_transfer_one;
 
 	ret = devm_spi_register_master(&pdev->dev, master);
@@ -782,7 +763,6 @@  static int sh_msiof_spi_probe(struct platform_device *pdev)
 
  err2:
 	pm_runtime_disable(&pdev->dev);
-	clk_unprepare(p->clk);
  err1:
 	spi_master_put(master);
 	return ret;
@@ -790,10 +770,7 @@  static int sh_msiof_spi_probe(struct platform_device *pdev)
 
 static int sh_msiof_spi_remove(struct platform_device *pdev)
 {
-	struct sh_msiof_spi_priv *p = platform_get_drvdata(pdev);
-
 	pm_runtime_disable(&pdev->dev);
-	clk_unprepare(p->clk);
 	return 0;
 }