diff mbox series

[v8,11/21] clk: tegra: clk-dfll: Add suspend and resume support

Message ID 1565308020-31952-12-git-send-email-skomatineni@nvidia.com
State Superseded
Headers show
Series SC7 entry and exit support for Tegra210 | expand

Commit Message

Sowjanya Komatineni Aug. 8, 2019, 11:46 p.m. UTC
This patch implements DFLL suspend and resume operation.

During system suspend entry, CPU clock will switch CPU to safe
clock source of PLLP and disables DFLL clock output.

DFLL driver suspend confirms DFLL disable state and errors out on
being active.

DFLL is re-initialized during the DFLL driver resume as it goes
through complete reset during suspend entry.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
 drivers/clk/tegra/clk-dfll.h               |  2 ++
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
 3 files changed, 59 insertions(+)

Comments

Dmitry Osipenko Aug. 9, 2019, 12:23 p.m. UTC | #1
09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch implements DFLL suspend and resume operation.
> 
> During system suspend entry, CPU clock will switch CPU to safe
> clock source of PLLP and disables DFLL clock output.
> 
> DFLL driver suspend confirms DFLL disable state and errors out on
> being active.
> 
> DFLL is re-initialized during the DFLL driver resume as it goes
> through complete reset during suspend entry.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>  drivers/clk/tegra/clk-dfll.h               |  2 ++
>  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index f8688c2ddf1a..eb298a5d7be9 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>  	td->last_unrounded_rate = 0;
>  
>  	pm_runtime_enable(td->dev);
> +	pm_runtime_irq_safe(td->dev);
>  	pm_runtime_get_sync(td->dev);
>  
>  	dfll_set_mode(td, DFLL_DISABLED);
> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>  	return ret;
>  }
>  
> +/**
> + * tegra_dfll_suspend - check DFLL is disabled
> + * @dev: DFLL device *
> + *
> + * DFLL clock should be disabled by the CPUFreq driver. So, make
> + * sure it is disabled and disable all clocks needed by the DFLL.
> + */
> +int tegra_dfll_suspend(struct device *dev)
> +{
> +	struct tegra_dfll *td = dev_get_drvdata(dev);
> +
> +	if (dfll_is_running(td)) {
> +		dev_err(td->dev, "dfll is enabled while shouldn't be\n");
> +		return -EBUSY;
> +	}
> +
> +	reset_control_assert(td->dvco_rst);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tegra_dfll_suspend);
> +
> +/**
> + * tegra_dfll_resume - reinitialize DFLL on resume
> + * @dev: DFLL instance
> + *
> + * DFLL is disabled and reset during suspend and resume.
> + * So, reinitialize the DFLL IP block back for use.
> + * DFLL clock is enabled later in closed loop mode by CPUFreq
> + * driver before switching its clock source to DFLL output.
> + */
> +int tegra_dfll_resume(struct device *dev)
> +{
> +	struct tegra_dfll *td = dev_get_drvdata(dev);
> +
> +	reset_control_deassert(td->dvco_rst);

This doesn't look right because I assume that DFLL resetting is
synchronous and thus clk should be enabled in order for reset to
propagate inside hardware.

> +	pm_runtime_get_sync(td->dev);

Hence it will be better to remove the above reset_control_deassert() and
add here:

	reset_control_reset(td->dvco_rst);

> +	dfll_set_mode(td, DFLL_DISABLED);
> +	dfll_set_default_params(td);
> +
> +	if (td->soc->init_clock_trimmers)
> +		td->soc->init_clock_trimmers();
> +
> +	dfll_set_open_loop_config(td);
> +
> +	dfll_init_out_if(td);
> +
> +	pm_runtime_put_sync(td->dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tegra_dfll_resume);
> +
>  /*
>   * DT data fetch
>   */
> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
> index 1b14ebe7268b..fb209eb5f365 100644
> --- a/drivers/clk/tegra/clk-dfll.h
> +++ b/drivers/clk/tegra/clk-dfll.h
> @@ -42,5 +42,7 @@ int tegra_dfll_register(struct platform_device *pdev,
>  struct tegra_dfll_soc_data *tegra_dfll_unregister(struct platform_device *pdev);
>  int tegra_dfll_runtime_suspend(struct device *dev);
>  int tegra_dfll_runtime_resume(struct device *dev);
> +int tegra_dfll_suspend(struct device *dev);
> +int tegra_dfll_resume(struct device *dev);
>  
>  #endif /* __DRIVERS_CLK_TEGRA_CLK_DFLL_H */
> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> index e84b6d52cbbd..2ac2679d696d 100644
> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> @@ -631,6 +631,7 @@ static int tegra124_dfll_fcpu_remove(struct platform_device *pdev)
>  static const struct dev_pm_ops tegra124_dfll_pm_ops = {
>  	SET_RUNTIME_PM_OPS(tegra_dfll_runtime_suspend,
>  			   tegra_dfll_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_dfll_suspend, tegra_dfll_resume)
>  };
>  
>  static struct platform_driver tegra124_dfll_fcpu_driver = {
>
Sowjanya Komatineni Aug. 9, 2019, 4:39 p.m. UTC | #2
On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>> This patch implements DFLL suspend and resume operation.
>>
>> During system suspend entry, CPU clock will switch CPU to safe
>> clock source of PLLP and disables DFLL clock output.
>>
>> DFLL driver suspend confirms DFLL disable state and errors out on
>> being active.
>>
>> DFLL is re-initialized during the DFLL driver resume as it goes
>> through complete reset during suspend entry.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>>   drivers/clk/tegra/clk-dfll.h               |  2 ++
>>   drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>>   3 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>> index f8688c2ddf1a..eb298a5d7be9 100644
>> --- a/drivers/clk/tegra/clk-dfll.c
>> +++ b/drivers/clk/tegra/clk-dfll.c
>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>   	td->last_unrounded_rate = 0;
>>   
>>   	pm_runtime_enable(td->dev);
>> +	pm_runtime_irq_safe(td->dev);
>>   	pm_runtime_get_sync(td->dev);
>>   
>>   	dfll_set_mode(td, DFLL_DISABLED);
>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>   	return ret;
>>   }
>>   
>> +/**
>> + * tegra_dfll_suspend - check DFLL is disabled
>> + * @dev: DFLL device *
>> + *
>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>> + * sure it is disabled and disable all clocks needed by the DFLL.
>> + */
>> +int tegra_dfll_suspend(struct device *dev)
>> +{
>> +	struct tegra_dfll *td = dev_get_drvdata(dev);
>> +
>> +	if (dfll_is_running(td)) {
>> +		dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	reset_control_assert(td->dvco_rst);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>> +
>> +/**
>> + * tegra_dfll_resume - reinitialize DFLL on resume
>> + * @dev: DFLL instance
>> + *
>> + * DFLL is disabled and reset during suspend and resume.
>> + * So, reinitialize the DFLL IP block back for use.
>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>> + * driver before switching its clock source to DFLL output.
>> + */
>> +int tegra_dfll_resume(struct device *dev)
>> +{
>> +	struct tegra_dfll *td = dev_get_drvdata(dev);
>> +
>> +	reset_control_deassert(td->dvco_rst);
> This doesn't look right because I assume that DFLL resetting is
> synchronous and thus clk should be enabled in order for reset to
> propagate inside hardware.
>
>> +	pm_runtime_get_sync(td->dev);
> Hence it will be better to remove the above reset_control_deassert() and
> add here:
>
> 	reset_control_reset(td->dvco_rst);

By the time dfll resume happens, dfll controller clock will already be 
enabled.

so doing reset de-assert before pm_runtime seems ok.

>> +	dfll_set_mode(td, DFLL_DISABLED);
>> +	dfll_set_default_params(td);
>> +
>> +	if (td->soc->init_clock_trimmers)
>> +		td->soc->init_clock_trimmers();
>> +
>> +	dfll_set_open_loop_config(td);
>> +
>> +	dfll_init_out_if(td);
>> +
>> +	pm_runtime_put_sync(td->dev);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(tegra_dfll_resume);
>> +
>>   /*
>>    * DT data fetch
>>    */
>> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
>> index 1b14ebe7268b..fb209eb5f365 100644
>> --- a/drivers/clk/tegra/clk-dfll.h
>> +++ b/drivers/clk/tegra/clk-dfll.h
>> @@ -42,5 +42,7 @@ int tegra_dfll_register(struct platform_device *pdev,
>>   struct tegra_dfll_soc_data *tegra_dfll_unregister(struct platform_device *pdev);
>>   int tegra_dfll_runtime_suspend(struct device *dev);
>>   int tegra_dfll_runtime_resume(struct device *dev);
>> +int tegra_dfll_suspend(struct device *dev);
>> +int tegra_dfll_resume(struct device *dev);
>>   
>>   #endif /* __DRIVERS_CLK_TEGRA_CLK_DFLL_H */
>> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> index e84b6d52cbbd..2ac2679d696d 100644
>> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
>> @@ -631,6 +631,7 @@ static int tegra124_dfll_fcpu_remove(struct platform_device *pdev)
>>   static const struct dev_pm_ops tegra124_dfll_pm_ops = {
>>   	SET_RUNTIME_PM_OPS(tegra_dfll_runtime_suspend,
>>   			   tegra_dfll_runtime_resume, NULL)
>> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_dfll_suspend, tegra_dfll_resume)
>>   };
>>   
>>   static struct platform_driver tegra124_dfll_fcpu_driver = {
>>
Dmitry Osipenko Aug. 9, 2019, 6 p.m. UTC | #3
09.08.2019 19:39, Sowjanya Komatineni пишет:
> 
> On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>> This patch implements DFLL suspend and resume operation.
>>>
>>> During system suspend entry, CPU clock will switch CPU to safe
>>> clock source of PLLP and disables DFLL clock output.
>>>
>>> DFLL driver suspend confirms DFLL disable state and errors out on
>>> being active.
>>>
>>> DFLL is re-initialized during the DFLL driver resume as it goes
>>> through complete reset during suspend entry.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>   drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>>>   drivers/clk/tegra/clk-dfll.h               |  2 ++
>>>   drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>>>   3 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>>> index f8688c2ddf1a..eb298a5d7be9 100644
>>> --- a/drivers/clk/tegra/clk-dfll.c
>>> +++ b/drivers/clk/tegra/clk-dfll.c
>>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>>       td->last_unrounded_rate = 0;
>>>         pm_runtime_enable(td->dev);
>>> +    pm_runtime_irq_safe(td->dev);
>>>       pm_runtime_get_sync(td->dev);
>>>         dfll_set_mode(td, DFLL_DISABLED);
>>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>>       return ret;
>>>   }
>>>   +/**
>>> + * tegra_dfll_suspend - check DFLL is disabled
>>> + * @dev: DFLL device *
>>> + *
>>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>>> + * sure it is disabled and disable all clocks needed by the DFLL.
>>> + */
>>> +int tegra_dfll_suspend(struct device *dev)
>>> +{
>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>> +
>>> +    if (dfll_is_running(td)) {
>>> +        dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>>> +        return -EBUSY;
>>> +    }
>>> +
>>> +    reset_control_assert(td->dvco_rst);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>>> +
>>> +/**
>>> + * tegra_dfll_resume - reinitialize DFLL on resume
>>> + * @dev: DFLL instance
>>> + *
>>> + * DFLL is disabled and reset during suspend and resume.
>>> + * So, reinitialize the DFLL IP block back for use.
>>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>>> + * driver before switching its clock source to DFLL output.
>>> + */
>>> +int tegra_dfll_resume(struct device *dev)
>>> +{
>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>> +
>>> +    reset_control_deassert(td->dvco_rst);
>> This doesn't look right because I assume that DFLL resetting is
>> synchronous and thus clk should be enabled in order for reset to
>> propagate inside hardware.
>>
>>> +    pm_runtime_get_sync(td->dev);
>> Hence it will be better to remove the above reset_control_deassert() and
>> add here:
>>
>>     reset_control_reset(td->dvco_rst);
> 
> By the time dfll resume happens, dfll controller clock will already be enabled.
> 
> so doing reset de-assert before pm_runtime seems ok.

I don't see what enables the DFLL clock because it should be enabled by the CPUFreq driver
on resume from suspend and resume happens after resuming of the DFLL driver.
Sowjanya Komatineni Aug. 9, 2019, 6:33 p.m. UTC | #4
On 8/9/19 11:00 AM, Dmitry Osipenko wrote:
> 09.08.2019 19:39, Sowjanya Komatineni пишет:
>> On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
>>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>>> This patch implements DFLL suspend and resume operation.
>>>>
>>>> During system suspend entry, CPU clock will switch CPU to safe
>>>> clock source of PLLP and disables DFLL clock output.
>>>>
>>>> DFLL driver suspend confirms DFLL disable state and errors out on
>>>> being active.
>>>>
>>>> DFLL is re-initialized during the DFLL driver resume as it goes
>>>> through complete reset during suspend entry.
>>>>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>    drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>>>>    drivers/clk/tegra/clk-dfll.h               |  2 ++
>>>>    drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>>>>    3 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>>>> index f8688c2ddf1a..eb298a5d7be9 100644
>>>> --- a/drivers/clk/tegra/clk-dfll.c
>>>> +++ b/drivers/clk/tegra/clk-dfll.c
>>>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>>>        td->last_unrounded_rate = 0;
>>>>          pm_runtime_enable(td->dev);
>>>> +    pm_runtime_irq_safe(td->dev);
>>>>        pm_runtime_get_sync(td->dev);
>>>>          dfll_set_mode(td, DFLL_DISABLED);
>>>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>>>        return ret;
>>>>    }
>>>>    +/**
>>>> + * tegra_dfll_suspend - check DFLL is disabled
>>>> + * @dev: DFLL device *
>>>> + *
>>>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>>>> + * sure it is disabled and disable all clocks needed by the DFLL.
>>>> + */
>>>> +int tegra_dfll_suspend(struct device *dev)
>>>> +{
>>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>>> +
>>>> +    if (dfll_is_running(td)) {
>>>> +        dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>>>> +        return -EBUSY;
>>>> +    }
>>>> +
>>>> +    reset_control_assert(td->dvco_rst);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>>>> +
>>>> +/**
>>>> + * tegra_dfll_resume - reinitialize DFLL on resume
>>>> + * @dev: DFLL instance
>>>> + *
>>>> + * DFLL is disabled and reset during suspend and resume.
>>>> + * So, reinitialize the DFLL IP block back for use.
>>>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>>>> + * driver before switching its clock source to DFLL output.
>>>> + */
>>>> +int tegra_dfll_resume(struct device *dev)
>>>> +{
>>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>>> +
>>>> +    reset_control_deassert(td->dvco_rst);
>>> This doesn't look right because I assume that DFLL resetting is
>>> synchronous and thus clk should be enabled in order for reset to
>>> propagate inside hardware.
>>>
>>>> +    pm_runtime_get_sync(td->dev);
>>> Hence it will be better to remove the above reset_control_deassert() and
>>> add here:
>>>
>>>      reset_control_reset(td->dvco_rst);
>> By the time dfll resume happens, dfll controller clock will already be enabled.
>>
>> so doing reset de-assert before pm_runtime seems ok.
> I don't see what enables the DFLL clock because it should be enabled by the CPUFreq driver
> on resume from suspend and resume happens after resuming of the DFLL driver.

dvco_rst is part of peripheral clocks and all peripheral clocks are 
restored by clk-tegra210 driver which happens before dfll driver resume.

So dfll rst thru part of peripheral clock enable is set prior to dfll 
reset deassertion
Dmitry Osipenko Aug. 9, 2019, 6:52 p.m. UTC | #5
09.08.2019 21:33, Sowjanya Komatineni пишет:
> 
> On 8/9/19 11:00 AM, Dmitry Osipenko wrote:
>> 09.08.2019 19:39, Sowjanya Komatineni пишет:
>>> On 8/9/19 5:23 AM, Dmitry Osipenko wrote:
>>>> 09.08.2019 2:46, Sowjanya Komatineni пишет:
>>>>> This patch implements DFLL suspend and resume operation.
>>>>>
>>>>> During system suspend entry, CPU clock will switch CPU to safe
>>>>> clock source of PLLP and disables DFLL clock output.
>>>>>
>>>>> DFLL driver suspend confirms DFLL disable state and errors out on
>>>>> being active.
>>>>>
>>>>> DFLL is re-initialized during the DFLL driver resume as it goes
>>>>> through complete reset during suspend entry.
>>>>>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>>    drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>>>>>    drivers/clk/tegra/clk-dfll.h               |  2 ++
>>>>>    drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>>>>>    3 files changed, 59 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
>>>>> index f8688c2ddf1a..eb298a5d7be9 100644
>>>>> --- a/drivers/clk/tegra/clk-dfll.c
>>>>> +++ b/drivers/clk/tegra/clk-dfll.c
>>>>> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>>>>>        td->last_unrounded_rate = 0;
>>>>>          pm_runtime_enable(td->dev);
>>>>> +    pm_runtime_irq_safe(td->dev);
>>>>>        pm_runtime_get_sync(td->dev);
>>>>>          dfll_set_mode(td, DFLL_DISABLED);
>>>>> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>>>>>        return ret;
>>>>>    }
>>>>>    +/**
>>>>> + * tegra_dfll_suspend - check DFLL is disabled
>>>>> + * @dev: DFLL device *
>>>>> + *
>>>>> + * DFLL clock should be disabled by the CPUFreq driver. So, make
>>>>> + * sure it is disabled and disable all clocks needed by the DFLL.
>>>>> + */
>>>>> +int tegra_dfll_suspend(struct device *dev)
>>>>> +{
>>>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>>>> +
>>>>> +    if (dfll_is_running(td)) {
>>>>> +        dev_err(td->dev, "dfll is enabled while shouldn't be\n");
>>>>> +        return -EBUSY;
>>>>> +    }
>>>>> +
>>>>> +    reset_control_assert(td->dvco_rst);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(tegra_dfll_suspend);
>>>>> +
>>>>> +/**
>>>>> + * tegra_dfll_resume - reinitialize DFLL on resume
>>>>> + * @dev: DFLL instance
>>>>> + *
>>>>> + * DFLL is disabled and reset during suspend and resume.
>>>>> + * So, reinitialize the DFLL IP block back for use.
>>>>> + * DFLL clock is enabled later in closed loop mode by CPUFreq
>>>>> + * driver before switching its clock source to DFLL output.
>>>>> + */
>>>>> +int tegra_dfll_resume(struct device *dev)
>>>>> +{
>>>>> +    struct tegra_dfll *td = dev_get_drvdata(dev);
>>>>> +
>>>>> +    reset_control_deassert(td->dvco_rst);
>>>> This doesn't look right because I assume that DFLL resetting is
>>>> synchronous and thus clk should be enabled in order for reset to
>>>> propagate inside hardware.
>>>>
>>>>> +    pm_runtime_get_sync(td->dev);
>>>> Hence it will be better to remove the above reset_control_deassert() and
>>>> add here:
>>>>
>>>>      reset_control_reset(td->dvco_rst);
>>> By the time dfll resume happens, dfll controller clock will already be enabled.
>>>
>>> so doing reset de-assert before pm_runtime seems ok.
>> I don't see what enables the DFLL clock because it should be enabled by the CPUFreq driver
>> on resume from suspend and resume happens after resuming of the DFLL driver.
> 
> dvco_rst is part of peripheral clocks and all peripheral clocks are restored by clk-tegra210
> driver which happens before dfll driver resume.
> 
> So dfll rst thru part of peripheral clock enable is set prior to dfll reset deassertion

Ah, so that is DVCO resetting and not DFLL, which are different blocks. Looks correct then.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Thierry Reding Aug. 12, 2019, 10:01 a.m. UTC | #6
On Thu, Aug 08, 2019 at 04:46:50PM -0700, Sowjanya Komatineni wrote:
> This patch implements DFLL suspend and resume operation.
> 
> During system suspend entry, CPU clock will switch CPU to safe
> clock source of PLLP and disables DFLL clock output.
> 
> DFLL driver suspend confirms DFLL disable state and errors out on
> being active.
> 
> DFLL is re-initialized during the DFLL driver resume as it goes
> through complete reset during suspend entry.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/clk/tegra/clk-dfll.c               | 56 ++++++++++++++++++++++++++++++
>  drivers/clk/tegra/clk-dfll.h               |  2 ++
>  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  1 +
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index f8688c2ddf1a..eb298a5d7be9 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1487,6 +1487,7 @@ static int dfll_init(struct tegra_dfll *td)
>  	td->last_unrounded_rate = 0;
>  
>  	pm_runtime_enable(td->dev);
> +	pm_runtime_irq_safe(td->dev);
>  	pm_runtime_get_sync(td->dev);
>  
>  	dfll_set_mode(td, DFLL_DISABLED);
> @@ -1513,6 +1514,61 @@ static int dfll_init(struct tegra_dfll *td)
>  	return ret;
>  }
>  
> +/**
> + * tegra_dfll_suspend - check DFLL is disabled
> + * @dev: DFLL device *
> + *
> + * DFLL clock should be disabled by the CPUFreq driver. So, make
> + * sure it is disabled and disable all clocks needed by the DFLL.
> + */
> +int tegra_dfll_suspend(struct device *dev)
> +{
> +	struct tegra_dfll *td = dev_get_drvdata(dev);
> +
> +	if (dfll_is_running(td)) {
> +		dev_err(td->dev, "dfll is enabled while shouldn't be\n");

Minor nit: "DFLL" in the error message, just like you have in the
kerneldoc comment above. Perhaps also make the error message a little
more specific. "while shouldn't be" makes the user guess what that
means. Perhaps better to say something like:

	"DFLL still enabled while suspending\n"

or perhaps even add a hint as to what could be the culprit:

	"DFLL still enabled while suspending, possibly a cpufreq driverbug\n"

The latter is somewhat long and the former is enough because the
kerneldoc comment already explains that cpufreq might be the reason for
this.

With a more specific error message, this is:

Acked-by: Thierry Reding <treding@nvidia.com>
diff mbox series

Patch

diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
index f8688c2ddf1a..eb298a5d7be9 100644
--- a/drivers/clk/tegra/clk-dfll.c
+++ b/drivers/clk/tegra/clk-dfll.c
@@ -1487,6 +1487,7 @@  static int dfll_init(struct tegra_dfll *td)
 	td->last_unrounded_rate = 0;
 
 	pm_runtime_enable(td->dev);
+	pm_runtime_irq_safe(td->dev);
 	pm_runtime_get_sync(td->dev);
 
 	dfll_set_mode(td, DFLL_DISABLED);
@@ -1513,6 +1514,61 @@  static int dfll_init(struct tegra_dfll *td)
 	return ret;
 }
 
+/**
+ * tegra_dfll_suspend - check DFLL is disabled
+ * @dev: DFLL device *
+ *
+ * DFLL clock should be disabled by the CPUFreq driver. So, make
+ * sure it is disabled and disable all clocks needed by the DFLL.
+ */
+int tegra_dfll_suspend(struct device *dev)
+{
+	struct tegra_dfll *td = dev_get_drvdata(dev);
+
+	if (dfll_is_running(td)) {
+		dev_err(td->dev, "dfll is enabled while shouldn't be\n");
+		return -EBUSY;
+	}
+
+	reset_control_assert(td->dvco_rst);
+
+	return 0;
+}
+EXPORT_SYMBOL(tegra_dfll_suspend);
+
+/**
+ * tegra_dfll_resume - reinitialize DFLL on resume
+ * @dev: DFLL instance
+ *
+ * DFLL is disabled and reset during suspend and resume.
+ * So, reinitialize the DFLL IP block back for use.
+ * DFLL clock is enabled later in closed loop mode by CPUFreq
+ * driver before switching its clock source to DFLL output.
+ */
+int tegra_dfll_resume(struct device *dev)
+{
+	struct tegra_dfll *td = dev_get_drvdata(dev);
+
+	reset_control_deassert(td->dvco_rst);
+
+	pm_runtime_get_sync(td->dev);
+
+	dfll_set_mode(td, DFLL_DISABLED);
+	dfll_set_default_params(td);
+
+	if (td->soc->init_clock_trimmers)
+		td->soc->init_clock_trimmers();
+
+	dfll_set_open_loop_config(td);
+
+	dfll_init_out_if(td);
+
+	pm_runtime_put_sync(td->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(tegra_dfll_resume);
+
 /*
  * DT data fetch
  */
diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
index 1b14ebe7268b..fb209eb5f365 100644
--- a/drivers/clk/tegra/clk-dfll.h
+++ b/drivers/clk/tegra/clk-dfll.h
@@ -42,5 +42,7 @@  int tegra_dfll_register(struct platform_device *pdev,
 struct tegra_dfll_soc_data *tegra_dfll_unregister(struct platform_device *pdev);
 int tegra_dfll_runtime_suspend(struct device *dev);
 int tegra_dfll_runtime_resume(struct device *dev);
+int tegra_dfll_suspend(struct device *dev);
+int tegra_dfll_resume(struct device *dev);
 
 #endif /* __DRIVERS_CLK_TEGRA_CLK_DFLL_H */
diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
index e84b6d52cbbd..2ac2679d696d 100644
--- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
+++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
@@ -631,6 +631,7 @@  static int tegra124_dfll_fcpu_remove(struct platform_device *pdev)
 static const struct dev_pm_ops tegra124_dfll_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_dfll_runtime_suspend,
 			   tegra_dfll_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_dfll_suspend, tegra_dfll_resume)
 };
 
 static struct platform_driver tegra124_dfll_fcpu_driver = {