linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: vc5: Add suspend/resume support
@ 2018-12-04 18:27 Marek Vasut
  2018-12-04 20:52 ` Stephen Boyd
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2018-12-04 18:27 UTC (permalink / raw)
  To: linux-clk
  Cc: Marek Vasut, Alexey Firago, Laurent Pinchart, Michael Turquette,
	Stephen Boyd, linux-renesas-soc

Add simple suspend/resume handlers to the driver to restore the chip
configuration after resume. It is possible that the chip was configured
with non-default values before suspend-resume cycle and that the chip
is powered down during this cycle, so the configuration could get lost.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Alexey Firago <alexey_firago@mentor.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/clk/clk-versaclock5.c | 36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index decffb3826ec..ac90fb36af1a 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int vc5_suspend(struct device *dev)
+{
+	struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regcache_sync(vc5->regmap);
+	if (ret != 0) {
+		dev_err(dev, "Failed to save register map: %d\n", ret);
+		return ret;
+	}
+	regcache_cache_only(vc5->regmap, true);
+	regcache_mark_dirty(vc5->regmap);
+
+	return 0;
+}
+
+static int vc5_resume(struct device *dev)
+{
+	struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
+	int ret;
+
+	regcache_cache_only(vc5->regmap, false);
+	ret = regcache_sync(vc5->regmap);
+	if (ret != 0) {
+		dev_err(dev, "Failed to restore register map: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
 static const struct vc5_chip_info idt_5p49v5923_info = {
 	.model = IDT_VC5_5P49V5923,
 	.clk_fod_cnt = 2,
@@ -961,9 +994,12 @@ static const struct of_device_id clk_vc5_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, clk_vc5_of_match);
 
+static SIMPLE_DEV_PM_OPS(vc5_pm_ops, vc5_suspend, vc5_resume);
+
 static struct i2c_driver vc5_driver = {
 	.driver = {
 		.name = "vc5",
+		.pm	= &vc5_pm_ops,
 		.of_match_table = clk_vc5_of_match,
 	},
 	.probe		= vc5_probe,
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-04 18:27 [PATCH] clk: vc5: Add suspend/resume support Marek Vasut
@ 2018-12-04 20:52 ` Stephen Boyd
  2018-12-04 23:48   ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2018-12-04 20:52 UTC (permalink / raw)
  To: Marek Vasut, linux-clk
  Cc: Marek Vasut, Alexey Firago, Laurent Pinchart, Michael Turquette,
	Stephen Boyd, linux-renesas-soc

Quoting Marek Vasut (2018-12-04 10:27:21)
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index decffb3826ec..ac90fb36af1a 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
>         return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int vc5_suspend(struct device *dev)

Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP

> +{
> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = regcache_sync(vc5->regmap);
> +       if (ret != 0) {
> +               dev_err(dev, "Failed to save register map: %d\n", ret);
> +               return ret;

Do we need to block suspend if we can't save the register map away? Or
can we just throw up our hands and not restore on resume?

> +       }
> +       regcache_cache_only(vc5->regmap, true);
> +       regcache_mark_dirty(vc5->regmap);
> +
> +       return 0;
> +}
> +
> +static int vc5_resume(struct device *dev)
> +{
> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> +       int ret;
> +
> +       regcache_cache_only(vc5->regmap, false);
> +       ret = regcache_sync(vc5->regmap);
> +       if (ret != 0) {
> +               dev_err(dev, "Failed to restore register map: %d\n", ret);
> +               return ret;
> +       }

Simplify to

	if (ret)
		dev_err()
	retun ret;

> +
> +       return 0;
> +}
> +#endif
> +
>  static const struct vc5_chip_info idt_5p49v5923_info = {
>         .model = IDT_VC5_5P49V5923,
>         .clk_fod_cnt = 2,

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-04 20:52 ` Stephen Boyd
@ 2018-12-04 23:48   ` Marek Vasut
  2018-12-05  5:21     ` Laurent Pinchart
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2018-12-04 23:48 UTC (permalink / raw)
  To: Stephen Boyd, linux-clk
  Cc: Marek Vasut, Alexey Firago, Laurent Pinchart, Michael Turquette,
	Stephen Boyd, linux-renesas-soc

On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> Quoting Marek Vasut (2018-12-04 10:27:21)
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index decffb3826ec..ac90fb36af1a 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
>>         return 0;
>>  }
>>  
>> +#ifdef CONFIG_PM_SLEEP
>> +static int vc5_suspend(struct device *dev)
> 
> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> 
>> +{
>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
>> +       int ret;
>> +
>> +       ret = regcache_sync(vc5->regmap);
>> +       if (ret != 0) {
>> +               dev_err(dev, "Failed to save register map: %d\n", ret);
>> +               return ret;
> 
> Do we need to block suspend if we can't save the register map away? Or
> can we just throw up our hands and not restore on resume?

Some hardware will fail on resume, so I'd say -- yes ?

The rest is fixed.

>> +       }
>> +       regcache_cache_only(vc5->regmap, true);
>> +       regcache_mark_dirty(vc5->regmap);
>> +
>> +       return 0;
>> +}
>> +
>> +static int vc5_resume(struct device *dev)
>> +{
>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
>> +       int ret;
>> +
>> +       regcache_cache_only(vc5->regmap, false);
>> +       ret = regcache_sync(vc5->regmap);
>> +       if (ret != 0) {
>> +               dev_err(dev, "Failed to restore register map: %d\n", ret);
>> +               return ret;
>> +       }
> 
> Simplify to
> 
> 	if (ret)
> 		dev_err()
> 	retun ret;
> 
>> +
>> +       return 0;
>> +}
>> +#endif
>> +
>>  static const struct vc5_chip_info idt_5p49v5923_info = {
>>         .model = IDT_VC5_5P49V5923,
>>         .clk_fod_cnt = 2,


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-04 23:48   ` Marek Vasut
@ 2018-12-05  5:21     ` Laurent Pinchart
  2018-12-05 12:29       ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-05  5:21 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Stephen Boyd, linux-clk, Marek Vasut, Alexey Firago,
	Michael Turquette, Stephen Boyd, linux-renesas-soc

Hi Marek,

On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> > Quoting Marek Vasut (2018-12-04 10:27:21)
> > 
> >> diff --git a/drivers/clk/clk-versaclock5.c
> >> b/drivers/clk/clk-versaclock5.c
> >> index decffb3826ec..ac90fb36af1a 100644
> >> --- a/drivers/clk/clk-versaclock5.c
> >> +++ b/drivers/clk/clk-versaclock5.c
> >> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
> >> 
> >>         return 0;
> >>  
> >>  }
> >> 
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int vc5_suspend(struct device *dev)
> > 
> > Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> > 
> >> +{
> >> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> +       int ret;
> >> +
> >> +       ret = regcache_sync(vc5->regmap);
> >> +       if (ret != 0) {
> >> +               dev_err(dev, "Failed to save register map: %d\n", ret);
> >> +               return ret;
> > 
> > Do we need to block suspend if we can't save the register map away? Or
> > can we just throw up our hands and not restore on resume?
> 
> Some hardware will fail on resume, so I'd say -- yes ?

But why do you need to sync on suspend in the first place ? What could cause 
the map to be dirty at this stage, and require syncing before suspend, that 
couldn't work with the sync be delayed to resume time ?

> The rest is fixed.
> 
> >> +       }
> >> +       regcache_cache_only(vc5->regmap, true);
> >> +       regcache_mark_dirty(vc5->regmap);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int vc5_resume(struct device *dev)
> >> +{
> >> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> +       int ret;
> >> +
> >> +       regcache_cache_only(vc5->regmap, false);
> >> +       ret = regcache_sync(vc5->regmap);
> >> +       if (ret != 0) {
> >> +               dev_err(dev, "Failed to restore register map: %d\n",
> >> ret);
> >> +               return ret;
> >> +       }
> > 
> > Simplify to
> > 
> > 	if (ret)
> > 	
> > 		dev_err()
> > 	
> > 	retun ret;
> > 	
> >> +
> >> +       return 0;
> >> +}
> >> +#endif
> >> +
> >> 
> >>  static const struct vc5_chip_info idt_5p49v5923_info = {
> >>  
> >>         .model = IDT_VC5_5P49V5923,
> >>         .clk_fod_cnt = 2,

-- 
Regards,

Laurent Pinchart




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-05  5:21     ` Laurent Pinchart
@ 2018-12-05 12:29       ` Marek Vasut
  2018-12-05 13:54         ` Laurent Pinchart
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2018-12-05 12:29 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Stephen Boyd, linux-clk, Marek Vasut, Alexey Firago,
	Michael Turquette, Stephen Boyd, linux-renesas-soc

On 12/05/2018 06:21 AM, Laurent Pinchart wrote:
> Hi Marek,
> 
> On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
>> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
>>> Quoting Marek Vasut (2018-12-04 10:27:21)
>>>
>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>> b/drivers/clk/clk-versaclock5.c
>>>> index decffb3826ec..ac90fb36af1a 100644
>>>> --- a/drivers/clk/clk-versaclock5.c
>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
>>>>
>>>>         return 0;
>>>>  
>>>>  }
>>>>
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int vc5_suspend(struct device *dev)
>>>
>>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
>>>
>>>> +{
>>>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
>>>> +       int ret;
>>>> +
>>>> +       ret = regcache_sync(vc5->regmap);
>>>> +       if (ret != 0) {
>>>> +               dev_err(dev, "Failed to save register map: %d\n", ret);
>>>> +               return ret;
>>>
>>> Do we need to block suspend if we can't save the register map away? Or
>>> can we just throw up our hands and not restore on resume?
>>
>> Some hardware will fail on resume, so I'd say -- yes ?
> 
> But why do you need to sync on suspend in the first place ? What could cause 
> the map to be dirty at this stage, and require syncing before suspend, that 
> couldn't work with the sync be delayed to resume time ?

Possibly a configuration coming from eg. bootloader time , or some other
configuration not done by Linux.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-05 12:29       ` Marek Vasut
@ 2018-12-05 13:54         ` Laurent Pinchart
  2018-12-05 14:21           ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-05 13:54 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Stephen Boyd, linux-clk, Marek Vasut, Alexey Firago,
	Michael Turquette, Stephen Boyd, linux-renesas-soc

Hi Marek,

On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote:
> On 12/05/2018 06:21 AM, Laurent Pinchart wrote:
> > On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
> >> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> >>> Quoting Marek Vasut (2018-12-04 10:27:21)
> >>> 
> >>>> diff --git a/drivers/clk/clk-versaclock5.c
> >>>> b/drivers/clk/clk-versaclock5.c
> >>>> index decffb3826ec..ac90fb36af1a 100644
> >>>> --- a/drivers/clk/clk-versaclock5.c
> >>>> +++ b/drivers/clk/clk-versaclock5.c
> >>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
> >>>>         return 0;
> >>>>  }
> >>>> 
> >>>> +#ifdef CONFIG_PM_SLEEP
> >>>> +static int vc5_suspend(struct device *dev)
> >>> 
> >>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> >>> 
> >>>> +{
> >>>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = regcache_sync(vc5->regmap);
> >>>> +       if (ret != 0) {
> >>>> +               dev_err(dev, "Failed to save register map: %d\n", ret);
> >>>> +               return ret;
> >>> 
> >>> Do we need to block suspend if we can't save the register map away? Or
> >>> can we just throw up our hands and not restore on resume?
> >> 
> >> Some hardware will fail on resume, so I'd say -- yes ?
> > 
> > But why do you need to sync on suspend in the first place ? What could
> > cause the map to be dirty at this stage, and require syncing before
> > suspend, that couldn't work with the sync be delayed to resume time ?
> 
> Possibly a configuration coming from eg. bootloader time , or some other
> configuration not done by Linux.

I still don't get it. As far as I know, regcache_sync() will write the content 
of the regmap to the hardware, not the other way around. You call it at resume 
time, so the hardware should be programmed properly, regardless of what the 
boot loader or the firmware does when resuming.

Could you please explain why this is needed at suspend time ?

-- 
Regards,

Laurent Pinchart




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Add suspend/resume support
  2018-12-05 13:54         ` Laurent Pinchart
@ 2018-12-05 14:21           ` Marek Vasut
  0 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2018-12-05 14:21 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Stephen Boyd, linux-clk, Marek Vasut, Alexey Firago,
	Michael Turquette, Stephen Boyd, linux-renesas-soc

On 12/05/2018 02:54 PM, Laurent Pinchart wrote:
> Hi Marek,

Hi,

> On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote:
>> On 12/05/2018 06:21 AM, Laurent Pinchart wrote:
>>> On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
>>>> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
>>>>> Quoting Marek Vasut (2018-12-04 10:27:21)
>>>>>
>>>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>>>> b/drivers/clk/clk-versaclock5.c
>>>>>> index decffb3826ec..ac90fb36af1a 100644
>>>>>> --- a/drivers/clk/clk-versaclock5.c
>>>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>>> +static int vc5_suspend(struct device *dev)
>>>>>
>>>>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
>>>>>
>>>>>> +{
>>>>>> +       struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       ret = regcache_sync(vc5->regmap);
>>>>>> +       if (ret != 0) {
>>>>>> +               dev_err(dev, "Failed to save register map: %d\n", ret);
>>>>>> +               return ret;
>>>>>
>>>>> Do we need to block suspend if we can't save the register map away? Or
>>>>> can we just throw up our hands and not restore on resume?
>>>>
>>>> Some hardware will fail on resume, so I'd say -- yes ?
>>>
>>> But why do you need to sync on suspend in the first place ? What could
>>> cause the map to be dirty at this stage, and require syncing before
>>> suspend, that couldn't work with the sync be delayed to resume time ?
>>
>> Possibly a configuration coming from eg. bootloader time , or some other
>> configuration not done by Linux.
> 
> I still don't get it. As far as I know, regcache_sync() will write the content 
> of the regmap to the hardware, not the other way around. You call it at resume 
> time, so the hardware should be programmed properly, regardless of what the 
> boot loader or the firmware does when resuming.
> 
> Could you please explain why this is needed at suspend time ?

It is not, can be dropped.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-12-05 14:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-04 18:27 [PATCH] clk: vc5: Add suspend/resume support Marek Vasut
2018-12-04 20:52 ` Stephen Boyd
2018-12-04 23:48   ` Marek Vasut
2018-12-05  5:21     ` Laurent Pinchart
2018-12-05 12:29       ` Marek Vasut
2018-12-05 13:54         ` Laurent Pinchart
2018-12-05 14:21           ` Marek Vasut

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).