[PATCH] clk: vc5: Add suspend/resume support

[PATCH] clk: vc5: Add suspend/resume support

[Marek Vasut]

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

Re: [PATCH] clk: vc5: Add suspend/resume support

[Stephen Boyd]

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,

Re: [PATCH] clk: vc5: Add suspend/resume support

[Marek Vasut]

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

Re: [PATCH] clk: vc5: Add suspend/resume support

[Laurent Pinchart]

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

Re: [PATCH] clk: vc5: Add suspend/resume support

[Marek Vasut]

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

Re: [PATCH] clk: vc5: Add suspend/resume support

[Laurent Pinchart]

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

Re: [PATCH] clk: vc5: Add suspend/resume support

[Marek Vasut]

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