[v2] ALSA: hda/tegra: enable clock during probe

If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
will not be ON. This could cause issue during probe, where hda init
setup is done. This patch enables clocks unconditionally during probe.

Along with above, follwoing changes are done.
  * enable runtime PM before exiting from probe work. This helps to avoid
    usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
  * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
  * runtime PM callbacks moved out of CONFIG_PM check

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

On 25/01/2019 11:06, Sameer Pujar wrote:
> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> will not be ON. This could cause issue during probe, where hda init
> setup is done. This patch enables clocks unconditionally during probe.
> 
> Along with above, follwoing changes are done.
>   * enable runtime PM before exiting from probe work. This helps to avoid
>     usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>   * runtime PM callbacks moved out of CONFIG_PM check
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index c8d18dc..ba6175f 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
>  	return rc;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static void hda_tegra_disable_clocks(struct hda_tegra *data)
>  {
>  	clk_disable_unprepare(data->hda2hdmi_clk);
> @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
>  	clk_disable_unprepare(data->hda_clk);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
>  /*
>   * power management
>   */
> @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  
> -#ifdef CONFIG_PM
>  static int hda_tegra_runtime_suspend(struct device *dev)
>  {
>  	struct snd_card *card = dev_get_drvdata(dev);
> @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
>  	int rc;
>  
>  	rc = hda_tegra_enable_clocks(hda);
> -	if (rc != 0)
> +	if (rc)
>  		return rc;
>  	if (chip && chip->running) {
>  		hda_tegra_init(hda);
> @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops hda_tegra_pm = {
>  	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
> @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(&pdev->dev, card);
>  
> -	pm_runtime_enable(hda->dev);
> -	if (!azx_has_pm_runtime(chip))
> -		pm_runtime_forbid(hda->dev);
> +	err = hda_tegra_enable_clocks(hda);
> +	if (err)
> +		goto out_free;
>  
>  	schedule_work(&hda->probe_work);
>  
> @@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work)
>  	struct platform_device *pdev = to_platform_device(hda->dev);
>  	int err;
>  
> -	pm_runtime_get_sync(hda->dev);
>  	err = hda_tegra_first_init(chip, pdev);
>  	if (err < 0)
>  		goto out_free;
> @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work)
>  	chip->running = 1;
>  	snd_hda_set_power_save(&chip->bus, power_save * 1000);
>  
> +	/* set device state as active */
> +	if (pm_runtime_set_active(hda->dev) < 0)
> +		goto out_free;
> +	/* enable runtime PM */
> +	pm_runtime_enable(hda->dev);
> +	if (!azx_has_pm_runtime(chip))
> +		pm_runtime_forbid(hda->dev);
> +
>   out_free:
> -	pm_runtime_put(hda->dev);
>  	return; /* no error return from async probe */
>  }
>  
> @@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev)
>  
>  	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
>  	pm_runtime_disable(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
> +		hda_tegra_runtime_suspend(&pdev->dev);
> +		pm_runtime_set_suspended(&pdev->dev);
> +	}

I think that we need to be consistent in the above with what is done in
the probe. If in the probe we call hda_tegra_enable_clocks(), then here
we should call hda_tegra_disable_clocks(). However, my preference is
still to all hda_tegra_runtime_resume() in probe and then leave the
above as-is. Let's see what everyone else thinks.

Cheers
Jon

On 1/25/2019 5:12 PM, Jon Hunter wrote:
> On 25/01/2019 11:06, Sameer Pujar wrote:
>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>> will not be ON. This could cause issue during probe, where hda init
>> setup is done. This patch enables clocks unconditionally during probe.
>>
>> Along with above, follwoing changes are done.
>>    * enable runtime PM before exiting from probe work. This helps to avoid
>>      usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>    * runtime PM callbacks moved out of CONFIG_PM check
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>   sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>> index c8d18dc..ba6175f 100644
>> --- a/sound/pci/hda/hda_tegra.c
>> +++ b/sound/pci/hda/hda_tegra.c
>> @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
>>   	return rc;
>>   }
>>   
>> -#ifdef CONFIG_PM_SLEEP
>>   static void hda_tegra_disable_clocks(struct hda_tegra *data)
>>   {
>>   	clk_disable_unprepare(data->hda2hdmi_clk);
>> @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
>>   	clk_disable_unprepare(data->hda_clk);
>>   }
>>   
>> +#ifdef CONFIG_PM_SLEEP
>>   /*
>>    * power management
>>    */
>> @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
>>   }
>>   #endif /* CONFIG_PM_SLEEP */
>>   
>> -#ifdef CONFIG_PM
>>   static int hda_tegra_runtime_suspend(struct device *dev)
>>   {
>>   	struct snd_card *card = dev_get_drvdata(dev);
>> @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
>>   	int rc;
>>   
>>   	rc = hda_tegra_enable_clocks(hda);
>> -	if (rc != 0)
>> +	if (rc)
>>   		return rc;
>>   	if (chip && chip->running) {
>>   		hda_tegra_init(hda);
>> @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
>>   
>>   	return 0;
>>   }
>> -#endif /* CONFIG_PM */
>>   
>>   static const struct dev_pm_ops hda_tegra_pm = {
>>   	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
>> @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>   
>>   	dev_set_drvdata(&pdev->dev, card);
>>   
>> -	pm_runtime_enable(hda->dev);
>> -	if (!azx_has_pm_runtime(chip))
>> -		pm_runtime_forbid(hda->dev);
>> +	err = hda_tegra_enable_clocks(hda);
>> +	if (err)
>> +		goto out_free;
>>   
>>   	schedule_work(&hda->probe_work);
>>   
>> @@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work)
>>   	struct platform_device *pdev = to_platform_device(hda->dev);
>>   	int err;
>>   
>> -	pm_runtime_get_sync(hda->dev);
>>   	err = hda_tegra_first_init(chip, pdev);
>>   	if (err < 0)
>>   		goto out_free;
>> @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work)
>>   	chip->running = 1;
>>   	snd_hda_set_power_save(&chip->bus, power_save * 1000);
>>   
>> +	/* set device state as active */
>> +	if (pm_runtime_set_active(hda->dev) < 0)
>> +		goto out_free;
>> +	/* enable runtime PM */
>> +	pm_runtime_enable(hda->dev);
>> +	if (!azx_has_pm_runtime(chip))
>> +		pm_runtime_forbid(hda->dev);
>> +
>>    out_free:
>> -	pm_runtime_put(hda->dev);
>>   	return; /* no error return from async probe */
>>   }
>>   
>> @@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev)
>>   
>>   	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
>>   	pm_runtime_disable(&pdev->dev);
>> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
>> +		hda_tegra_runtime_suspend(&pdev->dev);
>> +		pm_runtime_set_suspended(&pdev->dev);
>> +	}
> I think that we need to be consistent in the above with what is done in
> the probe. If in the probe we call hda_tegra_enable_clocks(), then here
> we should call hda_tegra_disable_clocks(). However, my preference is
> still to all hda_tegra_runtime_resume() in probe and then leave the
> above as-is. Let's see what everyone else thinks.
Though it is good to have the handling at a single place, it is slightly 
confusing to call
runtime_resume() in probe. When runtime PM is not yet enabled, why call 
something that is related
to it(not logically but intuitively)!

What we can possibly follow is,
1. Till probe() completes, handle things explicitly.
2. Before returning from successful probe, handle control to runtime PM.

Nevertheless, shall go with the majority of the opinion.

Thanks,
Sameer.

> Cheers
> Jon
>

On 25/01/2019 12:19, Sameer Pujar wrote:
> 
> On 1/25/2019 5:12 PM, Jon Hunter wrote:
>> On 25/01/2019 11:06, Sameer Pujar wrote:
>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>>> will not be ON. This could cause issue during probe, where hda init
>>> setup is done. This patch enables clocks unconditionally during probe.
>>>
>>> Along with above, follwoing changes are done.
>>>    * enable runtime PM before exiting from probe work. This helps to
>>> avoid
>>>      usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>>    * runtime PM callbacks moved out of CONFIG_PM check
>>>
>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>   sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
>>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>>> index c8d18dc..ba6175f 100644
>>> --- a/sound/pci/hda/hda_tegra.c
>>> +++ b/sound/pci/hda/hda_tegra.c
>>> @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct
>>> hda_tegra *data)
>>>       return rc;
>>>   }
>>>   -#ifdef CONFIG_PM_SLEEP
>>>   static void hda_tegra_disable_clocks(struct hda_tegra *data)
>>>   {
>>>       clk_disable_unprepare(data->hda2hdmi_clk);
>>> @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct
>>> hda_tegra *data)
>>>       clk_disable_unprepare(data->hda_clk);
>>>   }
>>>   +#ifdef CONFIG_PM_SLEEP
>>>   /*
>>>    * power management
>>>    */
>>> @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
>>>   }
>>>   #endif /* CONFIG_PM_SLEEP */
>>>   -#ifdef CONFIG_PM
>>>   static int hda_tegra_runtime_suspend(struct device *dev)
>>>   {
>>>       struct snd_card *card = dev_get_drvdata(dev);
>>> @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device
>>> *dev)
>>>       int rc;
>>>         rc = hda_tegra_enable_clocks(hda);
>>> -    if (rc != 0)
>>> +    if (rc)
>>>           return rc;
>>>       if (chip && chip->running) {
>>>           hda_tegra_init(hda);
>>> @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device
>>> *dev)
>>>         return 0;
>>>   }
>>> -#endif /* CONFIG_PM */
>>>     static const struct dev_pm_ops hda_tegra_pm = {
>>>       SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
>>> @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device
>>> *pdev)
>>>         dev_set_drvdata(&pdev->dev, card);
>>>   -    pm_runtime_enable(hda->dev);
>>> -    if (!azx_has_pm_runtime(chip))
>>> -        pm_runtime_forbid(hda->dev);
>>> +    err = hda_tegra_enable_clocks(hda);
>>> +    if (err)
>>> +        goto out_free;
>>>         schedule_work(&hda->probe_work);
>>>   @@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct
>>> work_struct *work)
>>>       struct platform_device *pdev = to_platform_device(hda->dev);
>>>       int err;
>>>   -    pm_runtime_get_sync(hda->dev);
>>>       err = hda_tegra_first_init(chip, pdev);
>>>       if (err < 0)
>>>           goto out_free;
>>> @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct
>>> work_struct *work)
>>>       chip->running = 1;
>>>       snd_hda_set_power_save(&chip->bus, power_save * 1000);
>>>   +    /* set device state as active */
>>> +    if (pm_runtime_set_active(hda->dev) < 0)
>>> +        goto out_free;
>>> +    /* enable runtime PM */
>>> +    pm_runtime_enable(hda->dev);
>>> +    if (!azx_has_pm_runtime(chip))
>>> +        pm_runtime_forbid(hda->dev);
>>> +
>>>    out_free:
>>> -    pm_runtime_put(hda->dev);
>>>       return; /* no error return from async probe */
>>>   }
>>>   @@ -603,6 +607,10 @@ static int hda_tegra_remove(struct
>>> platform_device *pdev)
>>>         ret = snd_card_free(dev_get_drvdata(&pdev->dev));
>>>       pm_runtime_disable(&pdev->dev);
>>> +    if (!pm_runtime_status_suspended(&pdev->dev)) {
>>> +        hda_tegra_runtime_suspend(&pdev->dev);
>>> +        pm_runtime_set_suspended(&pdev->dev);
>>> +    }
>> I think that we need to be consistent in the above with what is done in
>> the probe. If in the probe we call hda_tegra_enable_clocks(), then here
>> we should call hda_tegra_disable_clocks(). However, my preference is
>> still to all hda_tegra_runtime_resume() in probe and then leave the
>> above as-is. Let's see what everyone else thinks.
> Though it is good to have the handling at a single place, it is slightly
> confusing to call
> runtime_resume() in probe. When runtime PM is not yet enabled, why call
> something that is related
> to it(not logically but intuitively)!

I still don't think that it is confusing, the name of the function is
called hda_tegra_runtime_resume and does what is says on the tin (ie.
resumes the device at runtime).

Furthermore, I think it is completely logical that if pm-runtime is not
enabled, we manually call it. The benefit is that we have single
function for resuming the device for all circumstances.

Cheers
Jon

On 25/01/2019 11:06, Sameer Pujar wrote:
> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> will not be ON. This could cause issue during probe, where hda init
> setup is done. This patch enables clocks unconditionally during probe.
> 
> Along with above, follwoing changes are done.
>   * enable runtime PM before exiting from probe work. This helps to avoid
>     usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
>   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>   * runtime PM callbacks moved out of CONFIG_PM check
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index c8d18dc..ba6175f 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
>  	return rc;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static void hda_tegra_disable_clocks(struct hda_tegra *data)
>  {
>  	clk_disable_unprepare(data->hda2hdmi_clk);
> @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
>  	clk_disable_unprepare(data->hda_clk);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
>  /*
>   * power management
>   */
> @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  
> -#ifdef CONFIG_PM
>  static int hda_tegra_runtime_suspend(struct device *dev)
>  {
>  	struct snd_card *card = dev_get_drvdata(dev);
> @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
>  	int rc;
>  
>  	rc = hda_tegra_enable_clocks(hda);
> -	if (rc != 0)
> +	if (rc)
>  		return rc;
>  	if (chip && chip->running) {
>  		hda_tegra_init(hda);
> @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops hda_tegra_pm = {
>  	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
> @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(&pdev->dev, card);
>  
> -	pm_runtime_enable(hda->dev);
> -	if (!azx_has_pm_runtime(chip))
> -		pm_runtime_forbid(hda->dev);
> +	err = hda_tegra_enable_clocks(hda);
> +	if (err)
> +		goto out_free;

We also need to think about power-domains here. Enabling the clocks
might not be enough as the appropriate power-domain needs to be enabled.
For 64-bit Tegra runtime-pm will handle the power-domains (assuming they
are populated in device-tree). So I still think it is better we call
pm_runtime_get_sync() at some point rather than just replying on
enabling the clocks.

Cheers
Jon

On Wed, 30 Jan 2019 13:45:49 +0100,
Jon Hunter wrote:
> 
> 
> On 25/01/2019 11:06, Sameer Pujar wrote:
> > If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> > will not be ON. This could cause issue during probe, where hda init
> > setup is done. This patch enables clocks unconditionally during probe.
> > 
> > Along with above, follwoing changes are done.
> >   * enable runtime PM before exiting from probe work. This helps to avoid
> >     usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
> >   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
> >   * runtime PM callbacks moved out of CONFIG_PM check
> > 
> > Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> > Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> > Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> >  sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> > index c8d18dc..ba6175f 100644
> > --- a/sound/pci/hda/hda_tegra.c
> > +++ b/sound/pci/hda/hda_tegra.c
> > @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
> >  	return rc;
> >  }
> >  
> > -#ifdef CONFIG_PM_SLEEP
> >  static void hda_tegra_disable_clocks(struct hda_tegra *data)
> >  {
> >  	clk_disable_unprepare(data->hda2hdmi_clk);
> > @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
> >  	clk_disable_unprepare(data->hda_clk);
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> >  /*
> >   * power management
> >   */
> > @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
> >  }
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > -#ifdef CONFIG_PM
> >  static int hda_tegra_runtime_suspend(struct device *dev)
> >  {
> >  	struct snd_card *card = dev_get_drvdata(dev);
> > @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
> >  	int rc;
> >  
> >  	rc = hda_tegra_enable_clocks(hda);
> > -	if (rc != 0)
> > +	if (rc)
> >  		return rc;
> >  	if (chip && chip->running) {
> >  		hda_tegra_init(hda);
> > @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > -#endif /* CONFIG_PM */
> >  
> >  static const struct dev_pm_ops hda_tegra_pm = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
> > @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
> >  
> >  	dev_set_drvdata(&pdev->dev, card);
> >  
> > -	pm_runtime_enable(hda->dev);
> > -	if (!azx_has_pm_runtime(chip))
> > -		pm_runtime_forbid(hda->dev);
> > +	err = hda_tegra_enable_clocks(hda);
> > +	if (err)
> > +		goto out_free;
> 
> We also need to think about power-domains here. Enabling the clocks
> might not be enough as the appropriate power-domain needs to be enabled.
> For 64-bit Tegra runtime-pm will handle the power-domains (assuming they
> are populated in device-tree). So I still think it is better we call
> pm_runtime_get_sync() at some point rather than just replying on
> enabling the clocks.

If I understand correctly the code, the pm domain is already activated
at calling driver's probe callback.


thanks,

Takashi

On 1/30/2019 10:10 PM, Takashi Iwai wrote:
> On Wed, 30 Jan 2019 13:45:49 +0100,
> Jon Hunter wrote:
>>
>> On 25/01/2019 11:06, Sameer Pujar wrote:
>>> If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
>>> will not be ON. This could cause issue during probe, where hda init
>>> setup is done. This patch enables clocks unconditionally during probe.
>>>
>>> Along with above, follwoing changes are done.
>>>    * enable runtime PM before exiting from probe work. This helps to avoid
>>>      usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
>>>    * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
>>>    * runtime PM callbacks moved out of CONFIG_PM check
>>>
>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>> Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>   sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
>>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>>> index c8d18dc..ba6175f 100644
>>> --- a/sound/pci/hda/hda_tegra.c
>>> +++ b/sound/pci/hda/hda_tegra.c
>>> @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
>>>   	return rc;
>>>   }
>>>   
>>> -#ifdef CONFIG_PM_SLEEP
>>>   static void hda_tegra_disable_clocks(struct hda_tegra *data)
>>>   {
>>>   	clk_disable_unprepare(data->hda2hdmi_clk);
>>> @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
>>>   	clk_disable_unprepare(data->hda_clk);
>>>   }
>>>   
>>> +#ifdef CONFIG_PM_SLEEP
>>>   /*
>>>    * power management
>>>    */
>>> @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
>>>   }
>>>   #endif /* CONFIG_PM_SLEEP */
>>>   
>>> -#ifdef CONFIG_PM
>>>   static int hda_tegra_runtime_suspend(struct device *dev)
>>>   {
>>>   	struct snd_card *card = dev_get_drvdata(dev);
>>> @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
>>>   	int rc;
>>>   
>>>   	rc = hda_tegra_enable_clocks(hda);
>>> -	if (rc != 0)
>>> +	if (rc)
>>>   		return rc;
>>>   	if (chip && chip->running) {
>>>   		hda_tegra_init(hda);
>>> @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
>>>   
>>>   	return 0;
>>>   }
>>> -#endif /* CONFIG_PM */
>>>   
>>>   static const struct dev_pm_ops hda_tegra_pm = {
>>>   	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
>>> @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>>   
>>>   	dev_set_drvdata(&pdev->dev, card);
>>>   
>>> -	pm_runtime_enable(hda->dev);
>>> -	if (!azx_has_pm_runtime(chip))
>>> -		pm_runtime_forbid(hda->dev);
>>> +	err = hda_tegra_enable_clocks(hda);
>>> +	if (err)
>>> +		goto out_free;
>> We also need to think about power-domains here. Enabling the clocks
>> might not be enough as the appropriate power-domain needs to be enabled.
>> For 64-bit Tegra runtime-pm will handle the power-domains (assuming they
>> are populated in device-tree). So I still think it is better we call
>> pm_runtime_get_sync() at some point rather than just replying on
>> enabling the clocks.
> If I understand correctly the code, the pm domain is already activated
> at calling driver's probe callback.

If there are no further concerns, can we consider this for approval?

Thanks,
Sameer.

>
> thanks,
>
> Takashi

On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> On Wed, 30 Jan 2019 13:45:49 +0100,
> Jon Hunter wrote:
> > 
> > 
> > On 25/01/2019 11:06, Sameer Pujar wrote:
> > > If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> > > will not be ON. This could cause issue during probe, where hda init
> > > setup is done. This patch enables clocks unconditionally during probe.
> > > 
> > > Along with above, follwoing changes are done.
> > >   * enable runtime PM before exiting from probe work. This helps to avoid
> > >     usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
> > >   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
> > >   * runtime PM callbacks moved out of CONFIG_PM check
> > > 
> > > Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> > > Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> > > Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > > ---
> > >  sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
> > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> > > index c8d18dc..ba6175f 100644
> > > --- a/sound/pci/hda/hda_tegra.c
> > > +++ b/sound/pci/hda/hda_tegra.c
> > > @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
> > >  	return rc;
> > >  }
> > >  
> > > -#ifdef CONFIG_PM_SLEEP
> > >  static void hda_tegra_disable_clocks(struct hda_tegra *data)
> > >  {
> > >  	clk_disable_unprepare(data->hda2hdmi_clk);
> > > @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
> > >  	clk_disable_unprepare(data->hda_clk);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM_SLEEP
> > >  /*
> > >   * power management
> > >   */
> > > @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
> > >  }
> > >  #endif /* CONFIG_PM_SLEEP */
> > >  
> > > -#ifdef CONFIG_PM
> > >  static int hda_tegra_runtime_suspend(struct device *dev)
> > >  {
> > >  	struct snd_card *card = dev_get_drvdata(dev);
> > > @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
> > >  	int rc;
> > >  
> > >  	rc = hda_tegra_enable_clocks(hda);
> > > -	if (rc != 0)
> > > +	if (rc)
> > >  		return rc;
> > >  	if (chip && chip->running) {
> > >  		hda_tegra_init(hda);
> > > @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
> > >  
> > >  	return 0;
> > >  }
> > > -#endif /* CONFIG_PM */
> > >  
> > >  static const struct dev_pm_ops hda_tegra_pm = {
> > >  	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
> > > @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
> > >  
> > >  	dev_set_drvdata(&pdev->dev, card);
> > >  
> > > -	pm_runtime_enable(hda->dev);
> > > -	if (!azx_has_pm_runtime(chip))
> > > -		pm_runtime_forbid(hda->dev);
> > > +	err = hda_tegra_enable_clocks(hda);
> > > +	if (err)
> > > +		goto out_free;
> > 
> > We also need to think about power-domains here. Enabling the clocks
> > might not be enough as the appropriate power-domain needs to be enabled.
> > For 64-bit Tegra runtime-pm will handle the power-domains (assuming they
> > are populated in device-tree). So I still think it is better we call
> > pm_runtime_get_sync() at some point rather than just replying on
> > enabling the clocks.
> 
> If I understand correctly the code, the pm domain is already activated
> at calling driver's probe callback.

As far as I can tell, the domain will also be powered off again after
probe finished, unless the device grabs a runtime PM reference. This is
what happens via the dev->pm_domain->sync() call after successful probe
of a driver.

It seems to me like it's not a very well defined case what to do when a
device needs to be powered up but runtime PM is not enabled.

Adding Rafael and linux-pm, maybe they can provide some guidance on what
to do in these situations.

To summarize, what we're debating here is how to handle powering up a
device if the pm_runtime infrastructure doesn't take care of it. Jon's
proposal here was, and we use this elsewhere, to do something like this:

	pm_runtime_enable(dev);
	if (!pm_runtime_enabled(dev)) {
		err = foo_runtime_resume(dev);
		if (err < 0)
			goto fail;
	}

So basically when runtime PM is not available, we explicitly "resume"
the device to power it up.

It seems to me like that's a fairly common problem, so I'm wondering if
there's something that the runtime PM core could do to help with this.
Or perhaps there's already a way to achieve this that we're all
overlooking?

Rafael, any suggestions?

Thanks,
Thierry

On Thu, 31 Jan 2019 12:05:30 +0100,
Thierry Reding wrote:
> 
> On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > On Wed, 30 Jan 2019 13:45:49 +0100,
> > Jon Hunter wrote:
> > > 
> > > 
> > > On 25/01/2019 11:06, Sameer Pujar wrote:
> > > > If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks
> > > > will not be ON. This could cause issue during probe, where hda init
> > > > setup is done. This patch enables clocks unconditionally during probe.
> > > > 
> > > > Along with above, follwoing changes are done.
> > > >   * enable runtime PM before exiting from probe work. This helps to avoid
> > > >     usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
> > > >   * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
> > > >   * runtime PM callbacks moved out of CONFIG_PM check
> > > > 
> > > > Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> > > > Reviewed-by: Ravindra Lokhande <rlokhande@nvidia.com>
> > > > Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > > > ---
> > > >  sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++---------
> > > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> > > > index c8d18dc..ba6175f 100644
> > > > --- a/sound/pci/hda/hda_tegra.c
> > > > +++ b/sound/pci/hda/hda_tegra.c
> > > > @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data)
> > > >  	return rc;
> > > >  }
> > > >  
> > > > -#ifdef CONFIG_PM_SLEEP
> > > >  static void hda_tegra_disable_clocks(struct hda_tegra *data)
> > > >  {
> > > >  	clk_disable_unprepare(data->hda2hdmi_clk);
> > > > @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data)
> > > >  	clk_disable_unprepare(data->hda_clk);
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_PM_SLEEP
> > > >  /*
> > > >   * power management
> > > >   */
> > > > @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev)
> > > >  }
> > > >  #endif /* CONFIG_PM_SLEEP */
> > > >  
> > > > -#ifdef CONFIG_PM
> > > >  static int hda_tegra_runtime_suspend(struct device *dev)
> > > >  {
> > > >  	struct snd_card *card = dev_get_drvdata(dev);
> > > > @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev)
> > > >  	int rc;
> > > >  
> > > >  	rc = hda_tegra_enable_clocks(hda);
> > > > -	if (rc != 0)
> > > > +	if (rc)
> > > >  		return rc;
> > > >  	if (chip && chip->running) {
> > > >  		hda_tegra_init(hda);
> > > > @@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -#endif /* CONFIG_PM */
> > > >  
> > > >  static const struct dev_pm_ops hda_tegra_pm = {
> > > >  	SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume)
> > > > @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
> > > >  
> > > >  	dev_set_drvdata(&pdev->dev, card);
> > > >  
> > > > -	pm_runtime_enable(hda->dev);
> > > > -	if (!azx_has_pm_runtime(chip))
> > > > -		pm_runtime_forbid(hda->dev);
> > > > +	err = hda_tegra_enable_clocks(hda);
> > > > +	if (err)
> > > > +		goto out_free;
> > > 
> > > We also need to think about power-domains here. Enabling the clocks
> > > might not be enough as the appropriate power-domain needs to be enabled.
> > > For 64-bit Tegra runtime-pm will handle the power-domains (assuming they
> > > are populated in device-tree). So I still think it is better we call
> > > pm_runtime_get_sync() at some point rather than just replying on
> > > enabling the clocks.
> > 
> > If I understand correctly the code, the pm domain is already activated
> > at calling driver's probe callback.
> 
> As far as I can tell, the domain will also be powered off again after
> probe finished, unless the device grabs a runtime PM reference. This is
> what happens via the dev->pm_domain->sync() call after successful probe
> of a driver.

Ah, a good point.  This can be a problem with a probe work like this
case.

> It seems to me like it's not a very well defined case what to do when a
> device needs to be powered up but runtime PM is not enabled.
> 
> Adding Rafael and linux-pm, maybe they can provide some guidance on what
> to do in these situations.
> 
> To summarize, what we're debating here is how to handle powering up a
> device if the pm_runtime infrastructure doesn't take care of it. Jon's
> proposal here was, and we use this elsewhere, to do something like this:
> 
> 	pm_runtime_enable(dev);
> 	if (!pm_runtime_enabled(dev)) {
> 		err = foo_runtime_resume(dev);
> 		if (err < 0)
> 			goto fail;
> 	}
> 
> So basically when runtime PM is not available, we explicitly "resume"
> the device to power it up.
> 
> It seems to me like that's a fairly common problem, so I'm wondering if
> there's something that the runtime PM core could do to help with this.
> Or perhaps there's already a way to achieve this that we're all
> overlooking?
> 
> Rafael, any suggestions?

If any, a common helper would be appreciated, indeed.


thanks,

Takashi

On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 31 Jan 2019 12:05:30 +0100,
> Thierry Reding wrote:
> >
> > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:

[cut]

> > > If I understand correctly the code, the pm domain is already activated
> > > at calling driver's probe callback.
> >
> > As far as I can tell, the domain will also be powered off again after
> > probe finished, unless the device grabs a runtime PM reference. This is
> > what happens via the dev->pm_domain->sync() call after successful probe
> > of a driver.
>
> Ah, a good point.  This can be a problem with a probe work like this
> case.
>
> > It seems to me like it's not a very well defined case what to do when a
> > device needs to be powered up but runtime PM is not enabled.
> >
> > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > to do in these situations.
> >
> > To summarize, what we're debating here is how to handle powering up a
> > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > proposal here was, and we use this elsewhere, to do something like this:
> >
> >       pm_runtime_enable(dev);
> >       if (!pm_runtime_enabled(dev)) {
> >               err = foo_runtime_resume(dev);
> >               if (err < 0)
> >                       goto fail;
> >       }
> >
> > So basically when runtime PM is not available, we explicitly "resume"
> > the device to power it up.
> >
> > It seems to me like that's a fairly common problem, so I'm wondering if
> > there's something that the runtime PM core could do to help with this.
> > Or perhaps there's already a way to achieve this that we're all
> > overlooking?
> >
> > Rafael, any suggestions?
>
> If any, a common helper would be appreciated, indeed.

I'm not sure that I understand the problem correctly, so let me
restate it the way I understand it.

What we're talking about is a driver ->probe() callback.  Runtime PM
is disabled initially and the device is off.  It needs to be powered
up, but the way to do that depends on some configuration of the board
etc., so ideally

pm_runtime_enable(dev);
ret = pm_runtime_resume(dev);

should just work, but the question is what to do if runtime PM doesn't
work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
else?

On Thu, Jan 31, 2019 at 12:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Thu, 31 Jan 2019 12:05:30 +0100,
> > Thierry Reding wrote:
> > >
> > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
>
> [cut]
>
> > > > If I understand correctly the code, the pm domain is already activated
> > > > at calling driver's probe callback.
> > >
> > > As far as I can tell, the domain will also be powered off again after
> > > probe finished, unless the device grabs a runtime PM reference. This is
> > > what happens via the dev->pm_domain->sync() call after successful probe
> > > of a driver.
> >
> > Ah, a good point.  This can be a problem with a probe work like this
> > case.
> >
> > > It seems to me like it's not a very well defined case what to do when a
> > > device needs to be powered up but runtime PM is not enabled.
> > >
> > > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > > to do in these situations.
> > >
> > > To summarize, what we're debating here is how to handle powering up a
> > > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > > proposal here was, and we use this elsewhere, to do something like this:
> > >
> > >       pm_runtime_enable(dev);
> > >       if (!pm_runtime_enabled(dev)) {
> > >               err = foo_runtime_resume(dev);
> > >               if (err < 0)
> > >                       goto fail;
> > >       }
> > >
> > > So basically when runtime PM is not available, we explicitly "resume"
> > > the device to power it up.
> > >
> > > It seems to me like that's a fairly common problem, so I'm wondering if
> > > there's something that the runtime PM core could do to help with this.
> > > Or perhaps there's already a way to achieve this that we're all
> > > overlooking?
> > >
> > > Rafael, any suggestions?
> >
> > If any, a common helper would be appreciated, indeed.
>
> I'm not sure that I understand the problem correctly, so let me
> restate it the way I understand it.
>
> What we're talking about is a driver ->probe() callback.  Runtime PM
> is disabled initially and the device is off.  It needs to be powered
> up, but the way to do that depends on some configuration of the board
> etc., so ideally
>
> pm_runtime_enable(dev);
> ret = pm_runtime_resume(dev);
>
> should just work, but the question is what to do if runtime PM doesn't
> work as expected.  That is, CONFIG_PM_RUNTIME is unset?

Well, we got rid of CONFIG_PM_RUNTIME, so that would be CONFIG_PM unset rather.

>  Or something else?

On Thu, 31 Jan 2019 12:46:54 +0100,
Rafael J. Wysocki wrote:
> 
> On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Thu, 31 Jan 2019 12:05:30 +0100,
> > Thierry Reding wrote:
> > >
> > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> 
> [cut]
> 
> > > > If I understand correctly the code, the pm domain is already activated
> > > > at calling driver's probe callback.
> > >
> > > As far as I can tell, the domain will also be powered off again after
> > > probe finished, unless the device grabs a runtime PM reference. This is
> > > what happens via the dev->pm_domain->sync() call after successful probe
> > > of a driver.
> >
> > Ah, a good point.  This can be a problem with a probe work like this
> > case.
> >
> > > It seems to me like it's not a very well defined case what to do when a
> > > device needs to be powered up but runtime PM is not enabled.
> > >
> > > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > > to do in these situations.
> > >
> > > To summarize, what we're debating here is how to handle powering up a
> > > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > > proposal here was, and we use this elsewhere, to do something like this:
> > >
> > >       pm_runtime_enable(dev);
> > >       if (!pm_runtime_enabled(dev)) {
> > >               err = foo_runtime_resume(dev);
> > >               if (err < 0)
> > >                       goto fail;
> > >       }
> > >
> > > So basically when runtime PM is not available, we explicitly "resume"
> > > the device to power it up.
> > >
> > > It seems to me like that's a fairly common problem, so I'm wondering if
> > > there's something that the runtime PM core could do to help with this.
> > > Or perhaps there's already a way to achieve this that we're all
> > > overlooking?
> > >
> > > Rafael, any suggestions?
> >
> > If any, a common helper would be appreciated, indeed.
> 
> I'm not sure that I understand the problem correctly, so let me
> restate it the way I understand it.
> 
> What we're talking about is a driver ->probe() callback.  Runtime PM
> is disabled initially and the device is off.  It needs to be powered
> up, but the way to do that depends on some configuration of the board
> etc., so ideally
> 
> pm_runtime_enable(dev);
> ret = pm_runtime_resume(dev);
> 
> should just work, but the question is what to do if runtime PM doesn't
> work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> else?

Yes, the question is how to write the code for both with and without
CONFIG_PM (or CONFIG_PM_RUNTIME).

Right now, we have a code like below, pushing the initialization in an
async work and let the probe returning quickly.

	hda_tegra_probe() {
		....
		pm_runtime_enable();
		schedule_work();
		return;
	}

	hda_tegra_probe_work() {
		pm_runtime_get_sync();
		....
		pm_runtime_put_sync();
	}

Then it truned outhis code lacks of the clock initialization when
runtime PM isn't enabled.  Normally it's done via runtime resume

	hda_tegra_runtime_resume() {
		hda_tegra_enable_clocks();
		....
	}

And now the question is what is the standard idiom in such a case.

IMO, calling pm_runtime_resume() inside the probe function looks
weird, and my preference was to initialize the clocks explicitly, then
enable runtime PM.  But if using pm_runtime_resume() in the proc
should be seen as a standard procedure, I'm fine with that.


thanks,

Takashi

On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 31 Jan 2019 12:46:54 +0100,
> Rafael J. Wysocki wrote:
> >
> > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > Thierry Reding wrote:
> > > >
> > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> >
> > [cut]
> >
> > > > > If I understand correctly the code, the pm domain is already activated
> > > > > at calling driver's probe callback.
> > > >
> > > > As far as I can tell, the domain will also be powered off again after
> > > > probe finished, unless the device grabs a runtime PM reference. This is
> > > > what happens via the dev->pm_domain->sync() call after successful probe
> > > > of a driver.
> > >
> > > Ah, a good point.  This can be a problem with a probe work like this
> > > case.
> > >
> > > > It seems to me like it's not a very well defined case what to do when a
> > > > device needs to be powered up but runtime PM is not enabled.
> > > >
> > > > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > > > to do in these situations.
> > > >
> > > > To summarize, what we're debating here is how to handle powering up a
> > > > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > > > proposal here was, and we use this elsewhere, to do something like this:
> > > >
> > > >       pm_runtime_enable(dev);
> > > >       if (!pm_runtime_enabled(dev)) {
> > > >               err = foo_runtime_resume(dev);
> > > >               if (err < 0)
> > > >                       goto fail;
> > > >       }
> > > >
> > > > So basically when runtime PM is not available, we explicitly "resume"
> > > > the device to power it up.
> > > >
> > > > It seems to me like that's a fairly common problem, so I'm wondering if
> > > > there's something that the runtime PM core could do to help with this.
> > > > Or perhaps there's already a way to achieve this that we're all
> > > > overlooking?
> > > >
> > > > Rafael, any suggestions?
> > >
> > > If any, a common helper would be appreciated, indeed.
> >
> > I'm not sure that I understand the problem correctly, so let me
> > restate it the way I understand it.
> >
> > What we're talking about is a driver ->probe() callback.  Runtime PM
> > is disabled initially and the device is off.  It needs to be powered
> > up, but the way to do that depends on some configuration of the board
> > etc., so ideally
> >
> > pm_runtime_enable(dev);
> > ret = pm_runtime_resume(dev);
> >
> > should just work, but the question is what to do if runtime PM doesn't
> > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > else?
>
> Yes, the question is how to write the code for both with and without
> CONFIG_PM (or CONFIG_PM_RUNTIME).

This basically is about setup, because after that point all should
just work in both cases.

Personally, I would do

if (IS_ENABLED(CONFIG_PM)) {
  do setup based on pm-runtime
} else {
  do manual setup
}

> Right now, we have a code like below, pushing the initialization in an
> async work and let the probe returning quickly.
>
>         hda_tegra_probe() {
>                 ....

So why don't you do

if (!IS_ENABLED(CONFIG_PM)) {
  do manual clock setup
}

here?

>                 pm_runtime_enable();
>                 schedule_work();
>                 return;
>         }
>
>         hda_tegra_probe_work() {
>                 pm_runtime_get_sync();
>                 ....
>                 pm_runtime_put_sync();
>         }
>
> Then it truned outhis code lacks of the clock initialization when
> runtime PM isn't enabled.  Normally it's done via runtime resume
>
>         hda_tegra_runtime_resume() {
>                 hda_tegra_enable_clocks();
>                 ....
>         }
>
> And now the question is what is the standard idiom in such a case.
>
> IMO, calling pm_runtime_resume() inside the probe function looks
> weird, and my preference was to initialize the clocks explicitly, then
> enable runtime PM.  But if using pm_runtime_resume() in the proc
> should be seen as a standard procedure, I'm fine with that.

Well, people do pm_runtime_resume() in ->probe() too, but
pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give
you what you want anyway. :-)

Cheers,
Rafael

On 1/31/2019 5:40 PM, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
>> On Thu, 31 Jan 2019 12:46:54 +0100,
>> Rafael J. Wysocki wrote:
>>> On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
>>>> On Thu, 31 Jan 2019 12:05:30 +0100,
>>>> Thierry Reding wrote:
>>>>> On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
>>> [cut]
>>>
>>>>>> If I understand correctly the code, the pm domain is already activated
>>>>>> at calling driver's probe callback.
>>>>> As far as I can tell, the domain will also be powered off again after
>>>>> probe finished, unless the device grabs a runtime PM reference. This is
>>>>> what happens via the dev->pm_domain->sync() call after successful probe
>>>>> of a driver.
>>>> Ah, a good point.  This can be a problem with a probe work like this
>>>> case.

Are you suggesting, whether runtime PM is enabled/disabled, after 
successful probe the
domain would be powered off?
For CONFIG_PM enabled case, probe() can call get_sync() and put_sync() 
can be in probe_work.
How this needs to be handled for CONFG_PM disabled case? (just calling 
clock_enable() may
not be sufficient as per previous comments)

>>>>> It seems to me like it's not a very well defined case what to do when a
>>>>> device needs to be powered up but runtime PM is not enabled.
>>>>>
>>>>> Adding Rafael and linux-pm, maybe they can provide some guidance on what
>>>>> to do in these situations.
>>>>>
>>>>> To summarize, what we're debating here is how to handle powering up a
>>>>> device if the pm_runtime infrastructure doesn't take care of it. Jon's
>>>>> proposal here was, and we use this elsewhere, to do something like this:
>>>>>
>>>>>        pm_runtime_enable(dev);
>>>>>        if (!pm_runtime_enabled(dev)) {
>>>>>                err = foo_runtime_resume(dev);
>>>>>                if (err < 0)
>>>>>                        goto fail;
>>>>>        }
>>>>>
>>>>> So basically when runtime PM is not available, we explicitly "resume"
>>>>> the device to power it up.
>>>>>
>>>>> It seems to me like that's a fairly common problem, so I'm wondering if
>>>>> there's something that the runtime PM core could do to help with this.
>>>>> Or perhaps there's already a way to achieve this that we're all
>>>>> overlooking?
>>>>>
>>>>> Rafael, any suggestions?
>>>> If any, a common helper would be appreciated, indeed.
>>> I'm not sure that I understand the problem correctly, so let me
>>> restate it the way I understand it.
>>>
>>> What we're talking about is a driver ->probe() callback.  Runtime PM
>>> is disabled initially and the device is off.  It needs to be powered
>>> up, but the way to do that depends on some configuration of the board
>>> etc., so ideally
>>>
>>> pm_runtime_enable(dev);
>>> ret = pm_runtime_resume(dev);
>>>
>>> should just work, but the question is what to do if runtime PM doesn't
>>> work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
>>> else?
>> Yes, the question is how to write the code for both with and without
>> CONFIG_PM (or CONFIG_PM_RUNTIME).
> This basically is about setup, because after that point all should
> just work in both cases.
>
> Personally, I would do
>
> if (IS_ENABLED(CONFIG_PM)) {
>    do setup based on pm-runtime
> } else {
>    do manual setup
> }

do we really need config check here?
The debate was, whether to call hda_tegra_runtime_resume() or 
hda_tegra_enable_clocks() unconditionally here.
It would take care of both CONFIG_PM enabled/disabled cases. Then enable 
runtime PM.

>> Right now, we have a code like below, pushing the initialization in an
>> async work and let the probe returning quickly.
>>
>>          hda_tegra_probe() {
>>                  ....
> So why don't you do
>
> if (!IS_ENABLED(CONFIG_PM)) {
>    do manual clock setup
> }
>
> here?
>
>>                  pm_runtime_enable();
>>                  schedule_work();
>>                  return;
>>          }
>>
>>          hda_tegra_probe_work() {
>>                  pm_runtime_get_sync();
>>                  ....
>>                  pm_runtime_put_sync();
>>          }
>>
>> Then it truned outhis code lacks of the clock initialization when
>> runtime PM isn't enabled.  Normally it's done via runtime resume
>>
>>          hda_tegra_runtime_resume() {
>>                  hda_tegra_enable_clocks();
>>                  ....
>>          }
>>
>> And now the question is what is the standard idiom in such a case.
>>
>> IMO, calling pm_runtime_resume() inside the probe function looks
>> weird, and my preference was to initialize the clocks explicitly, then
>> enable runtime PM.  But if using pm_runtime_resume() in the proc
>> should be seen as a standard procedure, I'm fine with that.

I think reference here is, whether calling hda_tegra_runtime_resume() in 
probe() is
a standard procedure or not.

> Well, people do pm_runtime_resume() in ->probe() too, but
> pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give
> you what you want anyway. :-)
>
> Cheers,
> Rafael

On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Thu, 31 Jan 2019 12:46:54 +0100,
> > Rafael J. Wysocki wrote:
> > >
> > > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > >
> > > [cut]
> > >
> > > > > > If I understand correctly the code, the pm domain is already activated
> > > > > > at calling driver's probe callback.
> > > > >
> > > > > As far as I can tell, the domain will also be powered off again after
> > > > > probe finished, unless the device grabs a runtime PM reference. This is
> > > > > what happens via the dev->pm_domain->sync() call after successful probe
> > > > > of a driver.
> > > >
> > > > Ah, a good point.  This can be a problem with a probe work like this
> > > > case.
> > > >
> > > > > It seems to me like it's not a very well defined case what to do when a
> > > > > device needs to be powered up but runtime PM is not enabled.
> > > > >
> > > > > Adding Rafael and linux-pm, maybe they can provide some guidance on what
> > > > > to do in these situations.
> > > > >
> > > > > To summarize, what we're debating here is how to handle powering up a
> > > > > device if the pm_runtime infrastructure doesn't take care of it. Jon's
> > > > > proposal here was, and we use this elsewhere, to do something like this:
> > > > >
> > > > >       pm_runtime_enable(dev);
> > > > >       if (!pm_runtime_enabled(dev)) {
> > > > >               err = foo_runtime_resume(dev);
> > > > >               if (err < 0)
> > > > >                       goto fail;
> > > > >       }
> > > > >
> > > > > So basically when runtime PM is not available, we explicitly "resume"
> > > > > the device to power it up.
> > > > >
> > > > > It seems to me like that's a fairly common problem, so I'm wondering if
> > > > > there's something that the runtime PM core could do to help with this.
> > > > > Or perhaps there's already a way to achieve this that we're all
> > > > > overlooking?
> > > > >
> > > > > Rafael, any suggestions?
> > > >
> > > > If any, a common helper would be appreciated, indeed.
> > >
> > > I'm not sure that I understand the problem correctly, so let me
> > > restate it the way I understand it.
> > >
> > > What we're talking about is a driver ->probe() callback.  Runtime PM
> > > is disabled initially and the device is off.  It needs to be powered
> > > up, but the way to do that depends on some configuration of the board
> > > etc., so ideally
> > >
> > > pm_runtime_enable(dev);
> > > ret = pm_runtime_resume(dev);
> > >
> > > should just work, but the question is what to do if runtime PM doesn't
> > > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > > else?
> >
> > Yes, the question is how to write the code for both with and without
> > CONFIG_PM (or CONFIG_PM_RUNTIME).
> 
> This basically is about setup, because after that point all should
> just work in both cases.
> 
> Personally, I would do
> 
> if (IS_ENABLED(CONFIG_PM)) {
>   do setup based on pm-runtime
> } else {
>   do manual setup
> }
> 
> > Right now, we have a code like below, pushing the initialization in an
> > async work and let the probe returning quickly.
> >
> >         hda_tegra_probe() {
> >                 ....
> 
> So why don't you do
> 
> if (!IS_ENABLED(CONFIG_PM)) {
>   do manual clock setup
> }
> 
> here?

I think that's exactly what Jon and Sameer were proposing, although the
discussion started primarily because of the way it was done.

So basically the idea was to do:

	pm_runtime_enable()
	if (!pm_runtime_enabled())  /* basically !IS_ENABLED(CONFIG_PM) */
		hda_runtime_resume()

So we're not calling pm_runtime_resume() but rather the driver's
implementation of it. This is to avoid duplicating the code, which under
some circumstances can be fairly long. Duplicating is also error prone
because both instances may not always be in sync.

My understanding is that Takashi had reservations about using this kind
of construct because, well, frankly, it looks a little weird. We'd also
likely want to have a similar construct again in the ->remove() callback
to make sure we properly power off the device when it is no longer
needed. I'm just wondering if perhaps there should be a mechanism in the
core to take care of this, because this is basically something that we'd
need to do for every single driver.

For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
modified to do the above? This would be somewhat tricky because drivers
usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
that would result in an empty structure if !CONFIG_PM, but we could
probably work around that by adding a __SET_RUNTIME_PM_OPS that would
never be compiled out for this kind of case. Or such drivers could even
manually set .runtime_suspend and .runtime_resume to make sure they're
always populated.

Another way out of this would be to make sure we never run into the case
where runtime PM is disabled. If we always "select PM" on Tegra, then PM
should always be available. But is it guaranteed that runtime PM for the
devices is functional in that case? From a cursory look at the code it
would seem that way.

Thierry

On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
> 
> --Pk/CTwBz1VvfPIDp
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Thu, 31 Jan 2019 12:46:54 +0100,
> > > Rafael J. Wysocki wrote:
> > > >
> > > > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > >
> > > > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > > >
> > > > [cut]
> > > >
> > > > > > > If I understand correctly the code, the pm domain is already ac=
> tivated
> > > > > > > at calling driver's probe callback.
> > > > > >
> > > > > > As far as I can tell, the domain will also be powered off again a=
> fter
> > > > > > probe finished, unless the device grabs a runtime PM reference. T=
> his is
> > > > > > what happens via the dev->pm_domain->sync() call after successful=
>  probe
> > > > > > of a driver.
> > > > >
> > > > > Ah, a good point.  This can be a problem with a probe work like this
> > > > > case.
> > > > >
> > > > > > It seems to me like it's not a very well defined case what to do =
> when a
> > > > > > device needs to be powered up but runtime PM is not enabled.
> > > > > >
> > > > > > Adding Rafael and linux-pm, maybe they can provide some guidance =
> on what
> > > > > > to do in these situations.
> > > > > >
> > > > > > To summarize, what we're debating here is how to handle powering =
> up a
> > > > > > device if the pm_runtime infrastructure doesn't take care of it. =
> Jon's
> > > > > > proposal here was, and we use this elsewhere, to do something lik=
> e this:
> > > > > >
> > > > > >       pm_runtime_enable(dev);
> > > > > >       if (!pm_runtime_enabled(dev)) {
> > > > > >               err =3D foo_runtime_resume(dev);
> > > > > >               if (err < 0)
> > > > > >                       goto fail;
> > > > > >       }
> > > > > >
> > > > > > So basically when runtime PM is not available, we explicitly "res=
> ume"
> > > > > > the device to power it up.
> > > > > >
> > > > > > It seems to me like that's a fairly common problem, so I'm wonder=
> ing if
> > > > > > there's something that the runtime PM core could do to help with =
> this.
> > > > > > Or perhaps there's already a way to achieve this that we're all
> > > > > > overlooking?
> > > > > >
> > > > > > Rafael, any suggestions?
> > > > >
> > > > > If any, a common helper would be appreciated, indeed.
> > > >
> > > > I'm not sure that I understand the problem correctly, so let me
> > > > restate it the way I understand it.
> > > >
> > > > What we're talking about is a driver ->probe() callback.  Runtime PM
> > > > is disabled initially and the device is off.  It needs to be powered
> > > > up, but the way to do that depends on some configuration of the board
> > > > etc., so ideally
> > > >
> > > > pm_runtime_enable(dev);
> > > > ret =3D pm_runtime_resume(dev);
> > > >
> > > > should just work, but the question is what to do if runtime PM doesn't
> > > > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > > > else?
> > >
> > > Yes, the question is how to write the code for both with and without
> > > CONFIG_PM (or CONFIG_PM_RUNTIME).
> >=20
> > This basically is about setup, because after that point all should
> > just work in both cases.
> >=20
> > Personally, I would do
> >=20
> > if (IS_ENABLED(CONFIG_PM)) {
> >   do setup based on pm-runtime
> > } else {
> >   do manual setup
> > }
> >=20
> > > Right now, we have a code like below, pushing the initialization in an
> > > async work and let the probe returning quickly.
> > >
> > >         hda_tegra_probe() {
> > >                 ....
> >=20
> > So why don't you do
> >=20
> > if (!IS_ENABLED(CONFIG_PM)) {
> >   do manual clock setup
> > }
> >=20
> > here?
> 
> I think that's exactly what Jon and Sameer were proposing, although the
> discussion started primarily because of the way it was done.
> 
> So basically the idea was to do:
> 
> 	pm_runtime_enable()
> 	if (!pm_runtime_enabled())  /* basically !IS_ENABLED(CONFIG_PM) */

But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?

> 		hda_runtime_resume()
> 
> So we're not calling pm_runtime_resume() but rather the driver's
> implementation of it. This is to avoid duplicating the code, which under
> some circumstances can be fairly long. Duplicating is also error prone
> because both instances may not always be in sync.
> 
> My understanding is that Takashi had reservations about using this kind
> of construct because, well, frankly, it looks a little weird.

Yes, the way it was originally written above was weird, but is checking
IS_ENABLED(CONFIG_PM) directly really so weird?

> We'd also likely want to have a similar construct again in the ->remove()
> callback to make sure we properly power off the device when it is no longer
> needed.

Sure.  Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?

> I'm just wondering if perhaps there should be a mechanism in the
> core to take care of this,

How exactly?  How's the core going to know what to do when CONFIG_PM is
disabled?

> because this is basically something that we'd need to do for every single
> driver.

That is not true.  If the device is alwyas "on" to start with, you don't
need to do anything.  That's the case on many systems.

> For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
> modified to do the above?

But you'd need to pass a pointer to your hda_runtime_resume() to it at least
and how's that simpler than using a simple conditional directly?

> This would be somewhat tricky because drivers
> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> that would result in an empty structure if !CONFIG_PM, but we could
> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> never be compiled out for this kind of case. Or such drivers could even
> manually set .runtime_suspend and .runtime_resume to make sure they're
> always populated.
> 
> Another way out of this would be to make sure we never run into the case
> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> should always be available. But is it guaranteed that runtime PM for the
> devices is functional in that case? From a cursory look at the code it
> would seem that way.

If you select PM, then all of the requisite code should be there.

Alternatively, you can make the driver depend on PM.

Cheers,
Rafael

On 2/1/2019 4:54 AM, Rafael J. Wysocki wrote:
> On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
>> --Pk/CTwBz1VvfPIDp
>> Content-Type: text/plain; charset=us-ascii
>> Content-Disposition: inline
>> Content-Transfer-Encoding: quoted-printable
>>
>> On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
>>> On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
>>>> On Thu, 31 Jan 2019 12:46:54 +0100,
>>>> Rafael J. Wysocki wrote:
>>>>> On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
>>>>>> On Thu, 31 Jan 2019 12:05:30 +0100,
>>>>>> Thierry Reding wrote:
>>>>>>> On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
>>>>> [cut]
>>>>>
>>>>>>>> If I understand correctly the code, the pm domain is already ac=
>> tivated
>>>>>>>> at calling driver's probe callback.
>>>>>>> As far as I can tell, the domain will also be powered off again a=
>> fter
>>>>>>> probe finished, unless the device grabs a runtime PM reference. T=
>> his is
>>>>>>> what happens via the dev->pm_domain->sync() call after successful=
>>   probe
>>>>>>> of a driver.
>>>>>> Ah, a good point.  This can be a problem with a probe work like this
>>>>>> case.
>>>>>>
>>>>>>> It seems to me like it's not a very well defined case what to do =
>> when a
>>>>>>> device needs to be powered up but runtime PM is not enabled.
>>>>>>>
>>>>>>> Adding Rafael and linux-pm, maybe they can provide some guidance =
>> on what
>>>>>>> to do in these situations.
>>>>>>>
>>>>>>> To summarize, what we're debating here is how to handle powering =
>> up a
>>>>>>> device if the pm_runtime infrastructure doesn't take care of it. =
>> Jon's
>>>>>>> proposal here was, and we use this elsewhere, to do something lik=
>> e this:
>>>>>>>        pm_runtime_enable(dev);
>>>>>>>        if (!pm_runtime_enabled(dev)) {
>>>>>>>                err =3D foo_runtime_resume(dev);
>>>>>>>                if (err < 0)
>>>>>>>                        goto fail;
>>>>>>>        }
>>>>>>>
>>>>>>> So basically when runtime PM is not available, we explicitly "res=
>> ume"
>>>>>>> the device to power it up.
>>>>>>>
>>>>>>> It seems to me like that's a fairly common problem, so I'm wonder=
>> ing if
>>>>>>> there's something that the runtime PM core could do to help with =
>> this.
>>>>>>> Or perhaps there's already a way to achieve this that we're all
>>>>>>> overlooking?
>>>>>>>
>>>>>>> Rafael, any suggestions?
>>>>>> If any, a common helper would be appreciated, indeed.
>>>>> I'm not sure that I understand the problem correctly, so let me
>>>>> restate it the way I understand it.
>>>>>
>>>>> What we're talking about is a driver ->probe() callback.  Runtime PM
>>>>> is disabled initially and the device is off.  It needs to be powered
>>>>> up, but the way to do that depends on some configuration of the board
>>>>> etc., so ideally
>>>>>
>>>>> pm_runtime_enable(dev);
>>>>> ret =3D pm_runtime_resume(dev);
>>>>>
>>>>> should just work, but the question is what to do if runtime PM doesn't
>>>>> work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
>>>>> else?
>>>> Yes, the question is how to write the code for both with and without
>>>> CONFIG_PM (or CONFIG_PM_RUNTIME).
>>> =20
>>> This basically is about setup, because after that point all should
>>> just work in both cases.
>>> =20
>>> Personally, I would do
>>> =20
>>> if (IS_ENABLED(CONFIG_PM)) {
>>>    do setup based on pm-runtime
>>> } else {
>>>    do manual setup
>>> }
>>> =20
>>>> Right now, we have a code like below, pushing the initialization in an
>>>> async work and let the probe returning quickly.
>>>>
>>>>          hda_tegra_probe() {
>>>>                  ....
>>> =20
>>> So why don't you do
>>> =20
>>> if (!IS_ENABLED(CONFIG_PM)) {
>>>    do manual clock setup
>>> }
>>> =20
>>> here?
>> I think that's exactly what Jon and Sameer were proposing, although the
>> discussion started primarily because of the way it was done.
>>
>> So basically the idea was to do:
>>
>> 	pm_runtime_enable()
>> 	if (!pm_runtime_enabled())  /* basically !IS_ENABLED(CONFIG_PM) */
> But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
>
>> 		hda_runtime_resume()
>>
>> So we're not calling pm_runtime_resume() but rather the driver's
>> implementation of it. This is to avoid duplicating the code, which under
>> some circumstances can be fairly long. Duplicating is also error prone
>> because both instances may not always be in sync.
>>
>> My understanding is that Takashi had reservations about using this kind
>> of construct because, well, frankly, it looks a little weird.
> Yes, the way it was originally written above was weird, but is checking
> IS_ENABLED(CONFIG_PM) directly really so weird?
>
>> We'd also likely want to have a similar construct again in the ->remove()
>> callback to make sure we properly power off the device when it is no longer
>> needed.
> Sure.  Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
>
>> I'm just wondering if perhaps there should be a mechanism in the
>> core to take care of this,
> How exactly?  How's the core going to know what to do when CONFIG_PM is
> disabled?
>
>> because this is basically something that we'd need to do for every single
>> driver.
> That is not true.  If the device is alwyas "on" to start with, you don't
> need to do anything.  That's the case on many systems.
>
>> For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
>> modified to do the above?
> But you'd need to pass a pointer to your hda_runtime_resume() to it at least
> and how's that simpler than using a simple conditional directly?
>
>> This would be somewhat tricky because drivers
>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>> that would result in an empty structure if !CONFIG_PM, but we could
>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>> never be compiled out for this kind of case. Or such drivers could even
>> manually set .runtime_suspend and .runtime_resume to make sure they're
>> always populated.
>>
>> Another way out of this would be to make sure we never run into the case
>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>> should always be available. But is it guaranteed that runtime PM for the
>> devices is functional in that case? From a cursory look at the code it
>> would seem that way.
> If you select PM, then all of the requisite code should be there.
>
> Alternatively, you can make the driver depend on PM.
Objective is to have things working with or without CONFIG_PM enabled.
 From previous comments and discussions it appears that there is mixed 
response
for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() 
call. Need
to have consensus regarding the best practice to be followed, which 
would eventually
can be used in other drivers too.

Rafael is suggesting to use CONFIG_PM check to do manual setup or 
runtime PM setup in probe,
which would bring back the earlier above mentioned concern.

if (IS_ENABLED(CONFIG_PM)) {
do setup based on pm-runtime
} else {
     do manual setup
}
Both if/else might end up doing the same here.
Do we really need CONFIG_PM check here?

Instead does below proposal appear fine?

probe() {
     hda_tegra_enable_clock();
}

probe_work() {
     /* hda setup */
     . . .
     pm_runtime_set_active(); /* initial state as active */
     pm_runtime_enable();
     return;
}

remove() {
     pm_runtime_disable();
     if (!pm_runtime_status_suspended())
         hda_tegra_runtime_suspend(); /* takes care of both CONFIG_PM 
enable/disable case */
}

One of the other concern was, remove() and probe() do not appear to be 
in sync, because in probe() hda_tegra_enable_clock()
is called and in remove() there is hda_tegra_runtime_suspend() to 
effectively disable clock.
IMO, this should be ok since it can avoid duplication and proper comment 
can be added here for clarity.
Alternatively we can call hda_tegra_runtime_resume() in probe() 
unconditionally to avoid confusion.

Another point Thierry mentioned was, after successful probe() 
power-domain would be turned OFF. It seems Rafael had a different
view. I am little confused here. Kindly clarify if above proposal seems 
fine.

Thanks,
Sameer.

> Cheers,
> Rafael
>

On Fri, Feb 01, 2019 at 12:24:31AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
> > 
> > --Pk/CTwBz1VvfPIDp
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> > Content-Transfer-Encoding: quoted-printable
> > 
> > On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > On Thu, 31 Jan 2019 12:46:54 +0100,
> > > > Rafael J. Wysocki wrote:
> > > > >
> > > > > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > >
> > > > > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > > > >
> > > > > [cut]
> > > > >
> > > > > > > > If I understand correctly the code, the pm domain is already ac=
> > tivated
> > > > > > > > at calling driver's probe callback.
> > > > > > >
> > > > > > > As far as I can tell, the domain will also be powered off again a=
> > fter
> > > > > > > probe finished, unless the device grabs a runtime PM reference. T=
> > his is
> > > > > > > what happens via the dev->pm_domain->sync() call after successful=
> >  probe
> > > > > > > of a driver.
> > > > > >
> > > > > > Ah, a good point.  This can be a problem with a probe work like this
> > > > > > case.
> > > > > >
> > > > > > > It seems to me like it's not a very well defined case what to do =
> > when a
> > > > > > > device needs to be powered up but runtime PM is not enabled.
> > > > > > >
> > > > > > > Adding Rafael and linux-pm, maybe they can provide some guidance =
> > on what
> > > > > > > to do in these situations.
> > > > > > >
> > > > > > > To summarize, what we're debating here is how to handle powering =
> > up a
> > > > > > > device if the pm_runtime infrastructure doesn't take care of it. =
> > Jon's
> > > > > > > proposal here was, and we use this elsewhere, to do something lik=
> > e this:
> > > > > > >
> > > > > > >       pm_runtime_enable(dev);
> > > > > > >       if (!pm_runtime_enabled(dev)) {
> > > > > > >               err =3D foo_runtime_resume(dev);
> > > > > > >               if (err < 0)
> > > > > > >                       goto fail;
> > > > > > >       }
> > > > > > >
> > > > > > > So basically when runtime PM is not available, we explicitly "res=
> > ume"
> > > > > > > the device to power it up.
> > > > > > >
> > > > > > > It seems to me like that's a fairly common problem, so I'm wonder=
> > ing if
> > > > > > > there's something that the runtime PM core could do to help with =
> > this.
> > > > > > > Or perhaps there's already a way to achieve this that we're all
> > > > > > > overlooking?
> > > > > > >
> > > > > > > Rafael, any suggestions?
> > > > > >
> > > > > > If any, a common helper would be appreciated, indeed.
> > > > >
> > > > > I'm not sure that I understand the problem correctly, so let me
> > > > > restate it the way I understand it.
> > > > >
> > > > > What we're talking about is a driver ->probe() callback.  Runtime PM
> > > > > is disabled initially and the device is off.  It needs to be powered
> > > > > up, but the way to do that depends on some configuration of the board
> > > > > etc., so ideally
> > > > >
> > > > > pm_runtime_enable(dev);
> > > > > ret =3D pm_runtime_resume(dev);
> > > > >
> > > > > should just work, but the question is what to do if runtime PM doesn't
> > > > > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > > > > else?
> > > >
> > > > Yes, the question is how to write the code for both with and without
> > > > CONFIG_PM (or CONFIG_PM_RUNTIME).
> > >=20
> > > This basically is about setup, because after that point all should
> > > just work in both cases.
> > >=20
> > > Personally, I would do
> > >=20
> > > if (IS_ENABLED(CONFIG_PM)) {
> > >   do setup based on pm-runtime
> > > } else {
> > >   do manual setup
> > > }
> > >=20
> > > > Right now, we have a code like below, pushing the initialization in an
> > > > async work and let the probe returning quickly.
> > > >
> > > >         hda_tegra_probe() {
> > > >                 ....
> > >=20
> > > So why don't you do
> > >=20
> > > if (!IS_ENABLED(CONFIG_PM)) {
> > >   do manual clock setup
> > > }
> > >=20
> > > here?
> > 
> > I think that's exactly what Jon and Sameer were proposing, although the
> > discussion started primarily because of the way it was done.
> > 
> > So basically the idea was to do:
> > 
> > 	pm_runtime_enable()
> > 	if (!pm_runtime_enabled())  /* basically !IS_ENABLED(CONFIG_PM) */
> 
> But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?

I guess the intention here was to avoid the conditional. This may also
have been cargo-culted from a time when we didn't have IS_ENABLED, and
the ifdefery required would've made this a lot worse.

> > 		hda_runtime_resume()
> > 
> > So we're not calling pm_runtime_resume() but rather the driver's
> > implementation of it. This is to avoid duplicating the code, which under
> > some circumstances can be fairly long. Duplicating is also error prone
> > because both instances may not always be in sync.
> > 
> > My understanding is that Takashi had reservations about using this kind
> > of construct because, well, frankly, it looks a little weird.
> 
> Yes, the way it was originally written above was weird, but is checking
> IS_ENABLED(CONFIG_PM) directly really so weird?
> 
> > We'd also likely want to have a similar construct again in the ->remove()
> > callback to make sure we properly power off the device when it is no longer
> > needed.
> 
> Sure.  Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
> 
> > I'm just wondering if perhaps there should be a mechanism in the
> > core to take care of this,
> 
> How exactly?  How's the core going to know what to do when CONFIG_PM is
> disabled?
> 
> > because this is basically something that we'd need to do for every single
> > driver.
> 
> That is not true.  If the device is alwyas "on" to start with, you don't
> need to do anything.  That's the case on many systems.
> 
> > For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
> > modified to do the above?
> 
> But you'd need to pass a pointer to your hda_runtime_resume() to it at least
> and how's that simpler than using a simple conditional directly?

The idea was, as I was saying below, to reuse dev_pm_ops even if
!CONFIG_PM. So pm_runtime_enable() could be something like this:

	pm_runtime_enable(dev)
	{
		if (!CONFIG_PM)
			if (dev->pm_ops->resume)
				dev->pm_ops->resume(dev);

		...
	}

But that's admittedly somewhat of a stretch. This could of course be
made somewhat nicer by adding an explicit variant, say:

	pm_runtime_enable_foo(dev)
	{
		if (!CONFIG_PM && dev->pm_ops->resume)
			return dev->pm_ops->resume(dev);

		return 0;
	}

Maybe the fact that I couldn't come up with a good name is a good
indication that this is a bad idea...

> > This would be somewhat tricky because drivers
> > usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> > that would result in an empty structure if !CONFIG_PM, but we could
> > probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> > never be compiled out for this kind of case. Or such drivers could even
> > manually set .runtime_suspend and .runtime_resume to make sure they're
> > always populated.
> > 
> > Another way out of this would be to make sure we never run into the case
> > where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> > should always be available. But is it guaranteed that runtime PM for the
> > devices is functional in that case? From a cursory look at the code it
> > would seem that way.
> 
> If you select PM, then all of the requisite code should be there.

We do this on 64-bit ARM, but there had been some pushback when we had
proposed to do the same thing on 32-bit ARM. I think there were two
concerns:

	1) select PM would force the setting for all platforms on multi-
	   platforms builds

	2) prevents anyone from disabling PM for debugging purposes

1) no longer seems to be valid because Rockchip already selects PM
unconditionally. I'm not sure if 2) is valid anymore either. I haven't
run a build with !PM in a very long time and I wouldn't be surprised if
that was completely broken.

Maybe we need to try this again since a couple of years have elapsed and
runtime PM support on Tegra is much more mature at this point.

> Alternatively, you can make the driver depend on PM.

That's probably the easiest way out, but to be honest I think I'd prefer
to just enforce PM and keep things simple.

Jon, any objections?

Thierry

On Mon, Feb 04, 2019 at 01:46:20PM +0530, Sameer Pujar wrote:
> 
> On 2/1/2019 4:54 AM, Rafael J. Wysocki wrote:
> > On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
> > > --Pk/CTwBz1VvfPIDp
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > > Content-Transfer-Encoding: quoted-printable
> > > 
> > > On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > On Thu, 31 Jan 2019 12:46:54 +0100,
> > > > > Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > > > On Thu, 31 Jan 2019 12:05:30 +0100,
> > > > > > > Thierry Reding wrote:
> > > > > > > > On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
> > > > > > [cut]
> > > > > > 
> > > > > > > > > If I understand correctly the code, the pm domain is already ac=
> > > tivated
> > > > > > > > > at calling driver's probe callback.
> > > > > > > > As far as I can tell, the domain will also be powered off again a=
> > > fter
> > > > > > > > probe finished, unless the device grabs a runtime PM reference. T=
> > > his is
> > > > > > > > what happens via the dev->pm_domain->sync() call after successful=
> > >   probe
> > > > > > > > of a driver.
> > > > > > > Ah, a good point.  This can be a problem with a probe work like this
> > > > > > > case.
> > > > > > > 
> > > > > > > > It seems to me like it's not a very well defined case what to do =
> > > when a
> > > > > > > > device needs to be powered up but runtime PM is not enabled.
> > > > > > > > 
> > > > > > > > Adding Rafael and linux-pm, maybe they can provide some guidance =
> > > on what
> > > > > > > > to do in these situations.
> > > > > > > > 
> > > > > > > > To summarize, what we're debating here is how to handle powering =
> > > up a
> > > > > > > > device if the pm_runtime infrastructure doesn't take care of it. =
> > > Jon's
> > > > > > > > proposal here was, and we use this elsewhere, to do something lik=
> > > e this:
> > > > > > > >        pm_runtime_enable(dev);
> > > > > > > >        if (!pm_runtime_enabled(dev)) {
> > > > > > > >                err =3D foo_runtime_resume(dev);
> > > > > > > >                if (err < 0)
> > > > > > > >                        goto fail;
> > > > > > > >        }
> > > > > > > > 
> > > > > > > > So basically when runtime PM is not available, we explicitly "res=
> > > ume"
> > > > > > > > the device to power it up.
> > > > > > > > 
> > > > > > > > It seems to me like that's a fairly common problem, so I'm wonder=
> > > ing if
> > > > > > > > there's something that the runtime PM core could do to help with =
> > > this.
> > > > > > > > Or perhaps there's already a way to achieve this that we're all
> > > > > > > > overlooking?
> > > > > > > > 
> > > > > > > > Rafael, any suggestions?
> > > > > > > If any, a common helper would be appreciated, indeed.
> > > > > > I'm not sure that I understand the problem correctly, so let me
> > > > > > restate it the way I understand it.
> > > > > > 
> > > > > > What we're talking about is a driver ->probe() callback.  Runtime PM
> > > > > > is disabled initially and the device is off.  It needs to be powered
> > > > > > up, but the way to do that depends on some configuration of the board
> > > > > > etc., so ideally
> > > > > > 
> > > > > > pm_runtime_enable(dev);
> > > > > > ret =3D pm_runtime_resume(dev);
> > > > > > 
> > > > > > should just work, but the question is what to do if runtime PM doesn't
> > > > > > work as expected.  That is, CONFIG_PM_RUNTIME is unset?  Or something
> > > > > > else?
> > > > > Yes, the question is how to write the code for both with and without
> > > > > CONFIG_PM (or CONFIG_PM_RUNTIME).
> > > > =20
> > > > This basically is about setup, because after that point all should
> > > > just work in both cases.
> > > > =20
> > > > Personally, I would do
> > > > =20
> > > > if (IS_ENABLED(CONFIG_PM)) {
> > > >    do setup based on pm-runtime
> > > > } else {
> > > >    do manual setup
> > > > }
> > > > =20
> > > > > Right now, we have a code like below, pushing the initialization in an
> > > > > async work and let the probe returning quickly.
> > > > > 
> > > > >          hda_tegra_probe() {
> > > > >                  ....
> > > > =20
> > > > So why don't you do
> > > > =20
> > > > if (!IS_ENABLED(CONFIG_PM)) {
> > > >    do manual clock setup
> > > > }
> > > > =20
> > > > here?
> > > I think that's exactly what Jon and Sameer were proposing, although the
> > > discussion started primarily because of the way it was done.
> > > 
> > > So basically the idea was to do:
> > > 
> > > 	pm_runtime_enable()
> > > 	if (!pm_runtime_enabled())  /* basically !IS_ENABLED(CONFIG_PM) */
> > But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
> > 
> > > 		hda_runtime_resume()
> > > 
> > > So we're not calling pm_runtime_resume() but rather the driver's
> > > implementation of it. This is to avoid duplicating the code, which under
> > > some circumstances can be fairly long. Duplicating is also error prone
> > > because both instances may not always be in sync.
> > > 
> > > My understanding is that Takashi had reservations about using this kind
> > > of construct because, well, frankly, it looks a little weird.
> > Yes, the way it was originally written above was weird, but is checking
> > IS_ENABLED(CONFIG_PM) directly really so weird?
> > 
> > > We'd also likely want to have a similar construct again in the ->remove()
> > > callback to make sure we properly power off the device when it is no longer
> > > needed.
> > Sure.  Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
> > 
> > > I'm just wondering if perhaps there should be a mechanism in the
> > > core to take care of this,
> > How exactly?  How's the core going to know what to do when CONFIG_PM is
> > disabled?
> > 
> > > because this is basically something that we'd need to do for every single
> > > driver.
> > That is not true.  If the device is alwyas "on" to start with, you don't
> > need to do anything.  That's the case on many systems.
> > 
> > > For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be
> > > modified to do the above?
> > But you'd need to pass a pointer to your hda_runtime_resume() to it at least
> > and how's that simpler than using a simple conditional directly?
> > 
> > > This would be somewhat tricky because drivers
> > > usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> > > that would result in an empty structure if !CONFIG_PM, but we could
> > > probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> > > never be compiled out for this kind of case. Or such drivers could even
> > > manually set .runtime_suspend and .runtime_resume to make sure they're
> > > always populated.
> > > 
> > > Another way out of this would be to make sure we never run into the case
> > > where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> > > should always be available. But is it guaranteed that runtime PM for the
> > > devices is functional in that case? From a cursory look at the code it
> > > would seem that way.
> > If you select PM, then all of the requisite code should be there.
> > 
> > Alternatively, you can make the driver depend on PM.
> Objective is to have things working with or without CONFIG_PM enabled.
> From previous comments and discussions it appears that there is mixed
> response
> for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call.
> Need
> to have consensus regarding the best practice to be followed, which would
> eventually
> can be used in other drivers too.
> 
> Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM
> setup in probe,
> which would bring back the earlier above mentioned concern.
> 
> if (IS_ENABLED(CONFIG_PM)) {
> do setup based on pm-runtime
> } else {
>     do manual setup
> }
> Both if/else might end up doing the same here.
> Do we really need CONFIG_PM check here?
> 
> Instead does below proposal appear fine?
> 
> probe() {
>     hda_tegra_enable_clock();
> }
> 
> probe_work() {
>     /* hda setup */
>     . . .
>     pm_runtime_set_active(); /* initial state as active */
>     pm_runtime_enable();
>     return;
> }
> 
> remove() {
>     pm_runtime_disable();
>     if (!pm_runtime_status_suspended())
>         hda_tegra_runtime_suspend(); /* takes care of both CONFIG_PM
> enable/disable case */
> }
> 
> One of the other concern was, remove() and probe() do not appear to be in
> sync, because in probe() hda_tegra_enable_clock()
> is called and in remove() there is hda_tegra_runtime_suspend() to
> effectively disable clock.
> IMO, this should be ok since it can avoid duplication and proper comment can
> be added here for clarity.
> Alternatively we can call hda_tegra_runtime_resume() in probe()
> unconditionally to avoid confusion.
> 
> Another point Thierry mentioned was, after successful probe() power-domain
> would be turned OFF. It seems Rafael had a different
> view. I am little confused here. Kindly clarify if above proposal seems
> fine.

We're wasting an awful lot of time over the discussion of something that
I think is of questionable usefulness. I propose we do the below and can
forget about this once and for all.

Thierry

--- >8 ---
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index 7f3b83e0d324..51a8fa3566ef 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -10,6 +10,7 @@ menuconfig ARCH_TEGRA
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP
 	select PINCTRL
+	select PM
 	select PM_OPP
 	select ARCH_HAS_RESET_CONTROLLER
 	select RESET_CONTROLLER

On 04/02/2019 08:45, Thierry Reding wrote:

...

> The idea was, as I was saying below, to reuse dev_pm_ops even if
> !CONFIG_PM. So pm_runtime_enable() could be something like this:
> 
> 	pm_runtime_enable(dev)
> 	{
> 		if (!CONFIG_PM)
> 			if (dev->pm_ops->resume)
> 				dev->pm_ops->resume(dev);
> 
> 		...
> 	}
> 
> But that's admittedly somewhat of a stretch. This could of course be
> made somewhat nicer by adding an explicit variant, say:
> 
> 	pm_runtime_enable_foo(dev)
> 	{
> 		if (!CONFIG_PM && dev->pm_ops->resume)
> 			return dev->pm_ops->resume(dev);
> 
> 		return 0;
> 	}
> 
> Maybe the fact that I couldn't come up with a good name is a good
> indication that this is a bad idea...

How about some new APIs called ...

pm_runtime_enable_get()
pm_runtime_enable_get_sync()
pm_runtime_put_disable() (implies a put_sync)

... and in these APIs we add ...

pm_runtime_enable_get(dev)
{
	if (!CONFIG_PM && dev->pm_ops->resume)
		return dev->pm_ops->resume(dev);

	pm_runtime_enable(dev);
	return pm_runtime_get(dev);
}

>>> This would be somewhat tricky because drivers
>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>> that would result in an empty structure if !CONFIG_PM, but we could
>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>> never be compiled out for this kind of case. Or such drivers could even
>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>> always populated.
>>>
>>> Another way out of this would be to make sure we never run into the case
>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>> should always be available. But is it guaranteed that runtime PM for the
>>> devices is functional in that case? From a cursory look at the code it
>>> would seem that way.
>>
>> If you select PM, then all of the requisite code should be there.
> 
> We do this on 64-bit ARM, but there had been some pushback when we had
> proposed to do the same thing on 32-bit ARM. I think there were two
> concerns:
> 
> 	1) select PM would force the setting for all platforms on multi-
> 	   platforms builds
> 
> 	2) prevents anyone from disabling PM for debugging purposes
> 
> 1) no longer seems to be valid because Rockchip already selects PM
> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> run a build with !PM in a very long time and I wouldn't be surprised if
> that was completely broken.
> 
> Maybe we need to try this again since a couple of years have elapsed and
> runtime PM support on Tegra is much more mature at this point.
> 
>> Alternatively, you can make the driver depend on PM.
> 
> That's probably the easiest way out, but to be honest I think I'd prefer
> to just enforce PM and keep things simple.
> 
> Jon, any objections?

None, but seems overkill just for this case.

Cheers
Jon

On 04/02/2019 08:16, Sameer Pujar wrote:

...

> Objective is to have things working with or without CONFIG_PM enabled.
> From previous comments and discussions it appears that there is mixed
> response
> for calling hda_tegra_runtime_resume() or runtime PM APIs in probe()
> call. Need
> to have consensus regarding the best practice to be followed, which
> would eventually
> can be used in other drivers too.
> 
> Rafael is suggesting to use CONFIG_PM check to do manual setup or
> runtime PM setup in probe,
> which would bring back the earlier above mentioned concern.
> 
> if (IS_ENABLED(CONFIG_PM)) {
> do setup based on pm-runtime
> } else {
>     do manual setup
> }
> Both if/else might end up doing the same here.
> Do we really need CONFIG_PM check here?
> 
> Instead does below proposal appear fine?
> 
> probe() {
>     hda_tegra_enable_clock();
> }
> 
> probe_work() {
>     /* hda setup */
>     . . .
>     pm_runtime_set_active(); /* initial state as active */
>     pm_runtime_enable();
>     return;
> }

I believe that this still does not work, because if there is a
power-domain that needs to be turned on, this does not guarantee this.
So I think that you need to call pm_runtime_get ...

 probe() {
 	if (!IS_ENABLED(CONFIG_PM))
 		hda_tegra_enable_clock();
 }


 probe_work() {
    	/* hda setup */
    	. . .
	pm_runtime_enable();
	pm_runtime_get_sync();
	return;
 }

The alternative here could just be ...

 probe() {
	pm_runtime_enable();
 	if (!pm_runtime_enabled())
 		ret = hda_tegra_enable_clock();
	else
		ret = pm_runtime_get_sync();

	if (ret < 0) {
		...
	}
 }

Very similar to what I was saying to begin with but not call the
pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike.

Cheers
Jon

On Mon, 04 Feb 2019 11:04:50 +0100,
Jon Hunter wrote:
> 
> 
> On 04/02/2019 08:16, Sameer Pujar wrote:
> 
> ...
> 
> > Objective is to have things working with or without CONFIG_PM enabled.
> > From previous comments and discussions it appears that there is mixed
> > response
> > for calling hda_tegra_runtime_resume() or runtime PM APIs in probe()
> > call. Need
> > to have consensus regarding the best practice to be followed, which
> > would eventually
> > can be used in other drivers too.
> > 
> > Rafael is suggesting to use CONFIG_PM check to do manual setup or
> > runtime PM setup in probe,
> > which would bring back the earlier above mentioned concern.
> > 
> > if (IS_ENABLED(CONFIG_PM)) {
> > do setup based on pm-runtime
> > } else {
> >     do manual setup
> > }
> > Both if/else might end up doing the same here.
> > Do we really need CONFIG_PM check here?
> > 
> > Instead does below proposal appear fine?
> > 
> > probe() {
> >     hda_tegra_enable_clock();
> > }
> > 
> > probe_work() {
> >     /* hda setup */
> >     . . .
> >     pm_runtime_set_active(); /* initial state as active */
> >     pm_runtime_enable();
> >     return;
> > }
> 
> I believe that this still does not work, because if there is a
> power-domain that needs to be turned on, this does not guarantee this.
> So I think that you need to call pm_runtime_get ...
> 
>  probe() {
>  	if (!IS_ENABLED(CONFIG_PM))
>  		hda_tegra_enable_clock();
>  }
> 
> 
>  probe_work() {
>     	/* hda setup */
>     	. . .
> 	pm_runtime_enable();
> 	pm_runtime_get_sync();
> 	return;
>  }
> 
> The alternative here could just be ...
> 
>  probe() {
> 	pm_runtime_enable();
>  	if (!pm_runtime_enabled())
>  		ret = hda_tegra_enable_clock();
> 	else
> 		ret = pm_runtime_get_sync();
> 
> 	if (ret < 0) {
> 		...
> 	}
>  }
> 
> Very similar to what I was saying to begin with but not call the
> pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike.

Yes, exactly, what bothered me was really a nuance: calling
hda_tegra_runtime_resume() there makes the code misleading (or
confusing) as if the runtime PM were mandatory.

I hoped there could be some standard idiom for this expression, but
apparently there isn't any, so far...

Obviously the easiest option is to enforce the dependency on
CONFIG_PM.  Would there be any platform that needs to run without PM,
practically seen...?

But, now after lengthy discussions and the clarification of the
current situation, I have no strong opinion on this any longer.
So I leave the decision to you guys, and I'll apply it as-is.


thanks,

Takashi

On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> 
> 
> On 04/02/2019 08:45, Thierry Reding wrote:
> 
> ...
> 
> > The idea was, as I was saying below, to reuse dev_pm_ops even if
> > !CONFIG_PM. So pm_runtime_enable() could be something like this:
> > 
> > 	pm_runtime_enable(dev)
> > 	{
> > 		if (!CONFIG_PM)
> > 			if (dev->pm_ops->resume)
> > 				dev->pm_ops->resume(dev);
> > 
> > 		...
> > 	}
> > 
> > But that's admittedly somewhat of a stretch. This could of course be
> > made somewhat nicer by adding an explicit variant, say:
> > 
> > 	pm_runtime_enable_foo(dev)
> > 	{
> > 		if (!CONFIG_PM && dev->pm_ops->resume)
> > 			return dev->pm_ops->resume(dev);
> > 
> > 		return 0;
> > 	}
> > 
> > Maybe the fact that I couldn't come up with a good name is a good
> > indication that this is a bad idea...
> 
> How about some new APIs called ...
> 
> pm_runtime_enable_get()
> pm_runtime_enable_get_sync()
> pm_runtime_put_disable() (implies a put_sync)
> 
> ... and in these APIs we add ...
> 
> pm_runtime_enable_get(dev)
> {
> 	if (!CONFIG_PM && dev->pm_ops->resume)
> 		return dev->pm_ops->resume(dev);
> 
> 	pm_runtime_enable(dev);
> 	return pm_runtime_get(dev);
> }

Yeah, those sound sensible. I'm still a bit torn between this and just
enforcing PM. At least on the display side I think we already require PM
and with all the power domain work that you did we'd be much better off
if we could just get rid of the !PM workarounds.

> >>> This would be somewhat tricky because drivers
> >>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> >>> that would result in an empty structure if !CONFIG_PM, but we could
> >>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> >>> never be compiled out for this kind of case. Or such drivers could even
> >>> manually set .runtime_suspend and .runtime_resume to make sure they're
> >>> always populated.
> >>>
> >>> Another way out of this would be to make sure we never run into the case
> >>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> >>> should always be available. But is it guaranteed that runtime PM for the
> >>> devices is functional in that case? From a cursory look at the code it
> >>> would seem that way.
> >>
> >> If you select PM, then all of the requisite code should be there.
> > 
> > We do this on 64-bit ARM, but there had been some pushback when we had
> > proposed to do the same thing on 32-bit ARM. I think there were two
> > concerns:
> > 
> > 	1) select PM would force the setting for all platforms on multi-
> > 	   platforms builds
> > 
> > 	2) prevents anyone from disabling PM for debugging purposes
> > 
> > 1) no longer seems to be valid because Rockchip already selects PM
> > unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> > run a build with !PM in a very long time and I wouldn't be surprised if
> > that was completely broken.
> > 
> > Maybe we need to try this again since a couple of years have elapsed and
> > runtime PM support on Tegra is much more mature at this point.
> > 
> >> Alternatively, you can make the driver depend on PM.
> > 
> > That's probably the easiest way out, but to be honest I think I'd prefer
> > to just enforce PM and keep things simple.
> > 
> > Jon, any objections?
> 
> None, but seems overkill just for this case.

But that's precisely the point. It's not just about this case. We've
already got some drivers where we do a similar dance just to be able to
support the, let's admit it, exotic case where somebody turns off PM. I
think supporting !PM might have made sense at a point where we had no
support for power management at all. But I think we've come a long way
since then, and while we may still have some ways to go in some areas,
we do fairly decent runtime PM most of the time, to the point where I no
longer see any benefits in !PM.

Thierry

04.02.2019 14:05, Thierry Reding пишет:
> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
>>
>>
>> On 04/02/2019 08:45, Thierry Reding wrote:
>>
>> ...
>>
>>> The idea was, as I was saying below, to reuse dev_pm_ops even if
>>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
>>>
>>> 	pm_runtime_enable(dev)
>>> 	{
>>> 		if (!CONFIG_PM)
>>> 			if (dev->pm_ops->resume)
>>> 				dev->pm_ops->resume(dev);
>>>
>>> 		...
>>> 	}
>>>
>>> But that's admittedly somewhat of a stretch. This could of course be
>>> made somewhat nicer by adding an explicit variant, say:
>>>
>>> 	pm_runtime_enable_foo(dev)
>>> 	{
>>> 		if (!CONFIG_PM && dev->pm_ops->resume)
>>> 			return dev->pm_ops->resume(dev);
>>>
>>> 		return 0;
>>> 	}
>>>
>>> Maybe the fact that I couldn't come up with a good name is a good
>>> indication that this is a bad idea...
>>
>> How about some new APIs called ...
>>
>> pm_runtime_enable_get()
>> pm_runtime_enable_get_sync()
>> pm_runtime_put_disable() (implies a put_sync)
>>
>> ... and in these APIs we add ...
>>
>> pm_runtime_enable_get(dev)
>> {
>> 	if (!CONFIG_PM && dev->pm_ops->resume)
>> 		return dev->pm_ops->resume(dev);
>>
>> 	pm_runtime_enable(dev);
>> 	return pm_runtime_get(dev);
>> }
> 
> Yeah, those sound sensible. I'm still a bit torn between this and just
> enforcing PM. At least on the display side I think we already require PM
> and with all the power domain work that you did we'd be much better off
> if we could just get rid of the !PM workarounds.
> 
>>>>> This would be somewhat tricky because drivers
>>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>>>> that would result in an empty structure if !CONFIG_PM, but we could
>>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>>>> never be compiled out for this kind of case. Or such drivers could even
>>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>>>> always populated.
>>>>>
>>>>> Another way out of this would be to make sure we never run into the case
>>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>>>> should always be available. But is it guaranteed that runtime PM for the
>>>>> devices is functional in that case? From a cursory look at the code it
>>>>> would seem that way.
>>>>
>>>> If you select PM, then all of the requisite code should be there.
>>>
>>> We do this on 64-bit ARM, but there had been some pushback when we had
>>> proposed to do the same thing on 32-bit ARM. I think there were two
>>> concerns:
>>>
>>> 	1) select PM would force the setting for all platforms on multi-
>>> 	   platforms builds
>>>
>>> 	2) prevents anyone from disabling PM for debugging purposes
>>>
>>> 1) no longer seems to be valid because Rockchip already selects PM
>>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
>>> run a build with !PM in a very long time and I wouldn't be surprised if
>>> that was completely broken.
>>>
>>> Maybe we need to try this again since a couple of years have elapsed and
>>> runtime PM support on Tegra is much more mature at this point.
>>>
>>>> Alternatively, you can make the driver depend on PM.
>>>
>>> That's probably the easiest way out, but to be honest I think I'd prefer
>>> to just enforce PM and keep things simple.
>>>
>>> Jon, any objections?
>>
>> None, but seems overkill just for this case.
> 
> But that's precisely the point. It's not just about this case. We've
> already got some drivers where we do a similar dance just to be able to
> support the, let's admit it, exotic case where somebody turns off PM. I
> think supporting !PM might have made sense at a point where we had no
> support for power management at all. But I think we've come a long way
> since then, and while we may still have some ways to go in some areas,
> we do fairly decent runtime PM most of the time, to the point where I no
> longer see any benefits in !PM.

I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.

On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
> 04.02.2019 14:05, Thierry Reding пишет:
> > On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> >>
> >>
> >> On 04/02/2019 08:45, Thierry Reding wrote:
> >>
> >> ...
> >>
> >>> The idea was, as I was saying below, to reuse dev_pm_ops even if
> >>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
> >>>
> >>> 	pm_runtime_enable(dev)
> >>> 	{
> >>> 		if (!CONFIG_PM)
> >>> 			if (dev->pm_ops->resume)
> >>> 				dev->pm_ops->resume(dev);
> >>>
> >>> 		...
> >>> 	}
> >>>
> >>> But that's admittedly somewhat of a stretch. This could of course be
> >>> made somewhat nicer by adding an explicit variant, say:
> >>>
> >>> 	pm_runtime_enable_foo(dev)
> >>> 	{
> >>> 		if (!CONFIG_PM && dev->pm_ops->resume)
> >>> 			return dev->pm_ops->resume(dev);
> >>>
> >>> 		return 0;
> >>> 	}
> >>>
> >>> Maybe the fact that I couldn't come up with a good name is a good
> >>> indication that this is a bad idea...
> >>
> >> How about some new APIs called ...
> >>
> >> pm_runtime_enable_get()
> >> pm_runtime_enable_get_sync()
> >> pm_runtime_put_disable() (implies a put_sync)
> >>
> >> ... and in these APIs we add ...
> >>
> >> pm_runtime_enable_get(dev)
> >> {
> >> 	if (!CONFIG_PM && dev->pm_ops->resume)
> >> 		return dev->pm_ops->resume(dev);
> >>
> >> 	pm_runtime_enable(dev);
> >> 	return pm_runtime_get(dev);
> >> }
> > 
> > Yeah, those sound sensible. I'm still a bit torn between this and just
> > enforcing PM. At least on the display side I think we already require PM
> > and with all the power domain work that you did we'd be much better off
> > if we could just get rid of the !PM workarounds.
> > 
> >>>>> This would be somewhat tricky because drivers
> >>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> >>>>> that would result in an empty structure if !CONFIG_PM, but we could
> >>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> >>>>> never be compiled out for this kind of case. Or such drivers could even
> >>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
> >>>>> always populated.
> >>>>>
> >>>>> Another way out of this would be to make sure we never run into the case
> >>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> >>>>> should always be available. But is it guaranteed that runtime PM for the
> >>>>> devices is functional in that case? From a cursory look at the code it
> >>>>> would seem that way.
> >>>>
> >>>> If you select PM, then all of the requisite code should be there.
> >>>
> >>> We do this on 64-bit ARM, but there had been some pushback when we had
> >>> proposed to do the same thing on 32-bit ARM. I think there were two
> >>> concerns:
> >>>
> >>> 	1) select PM would force the setting for all platforms on multi-
> >>> 	   platforms builds
> >>>
> >>> 	2) prevents anyone from disabling PM for debugging purposes
> >>>
> >>> 1) no longer seems to be valid because Rockchip already selects PM
> >>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> >>> run a build with !PM in a very long time and I wouldn't be surprised if
> >>> that was completely broken.
> >>>
> >>> Maybe we need to try this again since a couple of years have elapsed and
> >>> runtime PM support on Tegra is much more mature at this point.
> >>>
> >>>> Alternatively, you can make the driver depend on PM.
> >>>
> >>> That's probably the easiest way out, but to be honest I think I'd prefer
> >>> to just enforce PM and keep things simple.
> >>>
> >>> Jon, any objections?
> >>
> >> None, but seems overkill just for this case.
> > 
> > But that's precisely the point. It's not just about this case. We've
> > already got some drivers where we do a similar dance just to be able to
> > support the, let's admit it, exotic case where somebody turns off PM. I
> > think supporting !PM might have made sense at a point where we had no
> > support for power management at all. But I think we've come a long way
> > since then, and while we may still have some ways to go in some areas,
> > we do fairly decent runtime PM most of the time, to the point where I no
> > longer see any benefits in !PM.
> 
> I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a
> useful debug feature, it can't just go away. 

What is it about disabling PM that you consider useful? I can understand
why you'd want that option if power management is broken, but as far as
I can tell, power management on Tegra is in pretty good state, and it's
more likely that !PM would actually be broken (though I haven't built a
configuration like that in a couple of years, so I'm speculating). We
already can't disable PM on 64-bit ARM, so I don't understand why 32-bit
ARM should be treated any differently.

Thierry

04.02.2019 17:00, Thierry Reding пишет:
> On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
>> 04.02.2019 14:05, Thierry Reding пишет:
>>> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
>>>>
>>>>
>>>> On 04/02/2019 08:45, Thierry Reding wrote:
>>>>
>>>> ...
>>>>
>>>>> The idea was, as I was saying below, to reuse dev_pm_ops even if
>>>>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
>>>>>
>>>>> 	pm_runtime_enable(dev)
>>>>> 	{
>>>>> 		if (!CONFIG_PM)
>>>>> 			if (dev->pm_ops->resume)
>>>>> 				dev->pm_ops->resume(dev);
>>>>>
>>>>> 		...
>>>>> 	}
>>>>>
>>>>> But that's admittedly somewhat of a stretch. This could of course be
>>>>> made somewhat nicer by adding an explicit variant, say:
>>>>>
>>>>> 	pm_runtime_enable_foo(dev)
>>>>> 	{
>>>>> 		if (!CONFIG_PM && dev->pm_ops->resume)
>>>>> 			return dev->pm_ops->resume(dev);
>>>>>
>>>>> 		return 0;
>>>>> 	}
>>>>>
>>>>> Maybe the fact that I couldn't come up with a good name is a good
>>>>> indication that this is a bad idea...
>>>>
>>>> How about some new APIs called ...
>>>>
>>>> pm_runtime_enable_get()
>>>> pm_runtime_enable_get_sync()
>>>> pm_runtime_put_disable() (implies a put_sync)
>>>>
>>>> ... and in these APIs we add ...
>>>>
>>>> pm_runtime_enable_get(dev)
>>>> {
>>>> 	if (!CONFIG_PM && dev->pm_ops->resume)
>>>> 		return dev->pm_ops->resume(dev);
>>>>
>>>> 	pm_runtime_enable(dev);
>>>> 	return pm_runtime_get(dev);
>>>> }
>>>
>>> Yeah, those sound sensible. I'm still a bit torn between this and just
>>> enforcing PM. At least on the display side I think we already require PM
>>> and with all the power domain work that you did we'd be much better off
>>> if we could just get rid of the !PM workarounds.
>>>
>>>>>>> This would be somewhat tricky because drivers
>>>>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>>>>>> that would result in an empty structure if !CONFIG_PM, but we could
>>>>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>>>>>> never be compiled out for this kind of case. Or such drivers could even
>>>>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>>>>>> always populated.
>>>>>>>
>>>>>>> Another way out of this would be to make sure we never run into the case
>>>>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>>>>>> should always be available. But is it guaranteed that runtime PM for the
>>>>>>> devices is functional in that case? From a cursory look at the code it
>>>>>>> would seem that way.
>>>>>>
>>>>>> If you select PM, then all of the requisite code should be there.
>>>>>
>>>>> We do this on 64-bit ARM, but there had been some pushback when we had
>>>>> proposed to do the same thing on 32-bit ARM. I think there were two
>>>>> concerns:
>>>>>
>>>>> 	1) select PM would force the setting for all platforms on multi-
>>>>> 	   platforms builds
>>>>>
>>>>> 	2) prevents anyone from disabling PM for debugging purposes
>>>>>
>>>>> 1) no longer seems to be valid because Rockchip already selects PM
>>>>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
>>>>> run a build with !PM in a very long time and I wouldn't be surprised if
>>>>> that was completely broken.
>>>>>
>>>>> Maybe we need to try this again since a couple of years have elapsed and
>>>>> runtime PM support on Tegra is much more mature at this point.
>>>>>
>>>>>> Alternatively, you can make the driver depend on PM.
>>>>>
>>>>> That's probably the easiest way out, but to be honest I think I'd prefer
>>>>> to just enforce PM and keep things simple.
>>>>>
>>>>> Jon, any objections?
>>>>
>>>> None, but seems overkill just for this case.
>>>
>>> But that's precisely the point. It's not just about this case. We've
>>> already got some drivers where we do a similar dance just to be able to
>>> support the, let's admit it, exotic case where somebody turns off PM. I
>>> think supporting !PM might have made sense at a point where we had no
>>> support for power management at all. But I think we've come a long way
>>> since then, and while we may still have some ways to go in some areas,
>>> we do fairly decent runtime PM most of the time, to the point where I no
>>> longer see any benefits in !PM.
>>
>> I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a
>> useful debug feature, it can't just go away. 
> 
> What is it about disabling PM that you consider useful? I can understand
> why you'd want that option if power management is broken, but as far as
> I can tell, power management on Tegra is in pretty good state, and it's
> more likely that !PM would actually be broken (though I haven't built a
> configuration like that in a couple of years, so I'm speculating). We
> already can't disable PM on 64-bit ARM, so I don't understand why 32-bit
> ARM should be treated any differently.

Yes, I want an option for debugging of a broken PM. It doesn't matter that there are no known PM-related issues on Tegra today, it could become a problem tomorrow. Probably a kernel boot parameter like pm_debug_always_on would suit even better here. 

Or maybe PM API already provides such debug options and I'm just not aware about them?

Do you know what's the exact reason for ARM64 to not support !PM?

On Mon, Feb 04, 2019 at 05:28:23PM +0300, Dmitry Osipenko wrote:
> 04.02.2019 17:00, Thierry Reding пишет:
> > On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
> >> 04.02.2019 14:05, Thierry Reding пишет:
> >>> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> >>>>
> >>>>
> >>>> On 04/02/2019 08:45, Thierry Reding wrote:
> >>>>
> >>>> ...
> >>>>
> >>>>> The idea was, as I was saying below, to reuse dev_pm_ops even if
> >>>>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
> >>>>>
> >>>>> 	pm_runtime_enable(dev)
> >>>>> 	{
> >>>>> 		if (!CONFIG_PM)
> >>>>> 			if (dev->pm_ops->resume)
> >>>>> 				dev->pm_ops->resume(dev);
> >>>>>
> >>>>> 		...
> >>>>> 	}
> >>>>>
> >>>>> But that's admittedly somewhat of a stretch. This could of course be
> >>>>> made somewhat nicer by adding an explicit variant, say:
> >>>>>
> >>>>> 	pm_runtime_enable_foo(dev)
> >>>>> 	{
> >>>>> 		if (!CONFIG_PM && dev->pm_ops->resume)
> >>>>> 			return dev->pm_ops->resume(dev);
> >>>>>
> >>>>> 		return 0;
> >>>>> 	}
> >>>>>
> >>>>> Maybe the fact that I couldn't come up with a good name is a good
> >>>>> indication that this is a bad idea...
> >>>>
> >>>> How about some new APIs called ...
> >>>>
> >>>> pm_runtime_enable_get()
> >>>> pm_runtime_enable_get_sync()
> >>>> pm_runtime_put_disable() (implies a put_sync)
> >>>>
> >>>> ... and in these APIs we add ...
> >>>>
> >>>> pm_runtime_enable_get(dev)
> >>>> {
> >>>> 	if (!CONFIG_PM && dev->pm_ops->resume)
> >>>> 		return dev->pm_ops->resume(dev);
> >>>>
> >>>> 	pm_runtime_enable(dev);
> >>>> 	return pm_runtime_get(dev);
> >>>> }
> >>>
> >>> Yeah, those sound sensible. I'm still a bit torn between this and just
> >>> enforcing PM. At least on the display side I think we already require PM
> >>> and with all the power domain work that you did we'd be much better off
> >>> if we could just get rid of the !PM workarounds.
> >>>
> >>>>>>> This would be somewhat tricky because drivers
> >>>>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> >>>>>>> that would result in an empty structure if !CONFIG_PM, but we could
> >>>>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> >>>>>>> never be compiled out for this kind of case. Or such drivers could even
> >>>>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
> >>>>>>> always populated.
> >>>>>>>
> >>>>>>> Another way out of this would be to make sure we never run into the case
> >>>>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> >>>>>>> should always be available. But is it guaranteed that runtime PM for the
> >>>>>>> devices is functional in that case? From a cursory look at the code it
> >>>>>>> would seem that way.
> >>>>>>
> >>>>>> If you select PM, then all of the requisite code should be there.
> >>>>>
> >>>>> We do this on 64-bit ARM, but there had been some pushback when we had
> >>>>> proposed to do the same thing on 32-bit ARM. I think there were two
> >>>>> concerns:
> >>>>>
> >>>>> 	1) select PM would force the setting for all platforms on multi-
> >>>>> 	   platforms builds
> >>>>>
> >>>>> 	2) prevents anyone from disabling PM for debugging purposes
> >>>>>
> >>>>> 1) no longer seems to be valid because Rockchip already selects PM
> >>>>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> >>>>> run a build with !PM in a very long time and I wouldn't be surprised if
> >>>>> that was completely broken.
> >>>>>
> >>>>> Maybe we need to try this again since a couple of years have elapsed and
> >>>>> runtime PM support on Tegra is much more mature at this point.
> >>>>>
> >>>>>> Alternatively, you can make the driver depend on PM.
> >>>>>
> >>>>> That's probably the easiest way out, but to be honest I think I'd prefer
> >>>>> to just enforce PM and keep things simple.
> >>>>>
> >>>>> Jon, any objections?
> >>>>
> >>>> None, but seems overkill just for this case.
> >>>
> >>> But that's precisely the point. It's not just about this case. We've
> >>> already got some drivers where we do a similar dance just to be able to
> >>> support the, let's admit it, exotic case where somebody turns off PM. I
> >>> think supporting !PM might have made sense at a point where we had no
> >>> support for power management at all. But I think we've come a long way
> >>> since then, and while we may still have some ways to go in some areas,
> >>> we do fairly decent runtime PM most of the time, to the point where I no
> >>> longer see any benefits in !PM.
> >>
> >> I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a
> >> useful debug feature, it can't just go away. 
> > 
> > What is it about disabling PM that you consider useful? I can understand
> > why you'd want that option if power management is broken, but as far as
> > I can tell, power management on Tegra is in pretty good state, and it's
> > more likely that !PM would actually be broken (though I haven't built a
> > configuration like that in a couple of years, so I'm speculating). We
> > already can't disable PM on 64-bit ARM, so I don't understand why 32-bit
> > ARM should be treated any differently.
> 
> Yes, I want an option for debugging of a broken PM. It doesn't matter
> that there are no known PM-related issues on Tegra today, it could
> become a problem tomorrow. Probably a kernel boot parameter like
> pm_debug_always_on would suit even better here. 
> 
> Or maybe PM API already provides such debug options and I'm just not
> aware about them?

I'm not sure I understand what you're trying to do. If you want to use
!PM as a way to debug broken PM, how would that help? Presumably if you
disable !PM then the problem would be gone? In other words, in order to
reproduce a PM related issue don't you have to enable PM first?

I'm not aware of an alternate mechanism to force PM to off once built
into the kernel.

> Do you know what's the exact reason for ARM64 to not support !PM?

There's no reason per se not to support PM. It's just that at the time
we made the decision that we didn't want to. I vaguely remember that we
tried to do the same thing for 32-bit ARM, but at the time we would've
been the only platform to select PM, and that would've meant enabling
PM for all subarchitectures and therefore forcing PM in multi-platform
builds. Today if you build multi-platform kernels, at least Rockchip
will also forcibly enable PM for everyone.

I think you may also have objected at the time for similar reasons. But
this was a long time ago (at least 2 to 3 years I would guess) and much
has changed since then. Power management has gotten a lot better on
Tegra, to the point where we actually need it in order to properly
function. For example, Tegra DRM is not going to work with !PM because
we rely on runtime PM while disabling and enabling display pipelines.

Thierry

04.02.2019 19:17, Thierry Reding пишет:
> On Mon, Feb 04, 2019 at 05:28:23PM +0300, Dmitry Osipenko wrote:
>> 04.02.2019 17:00, Thierry Reding пишет:
>>> On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
>>>> 04.02.2019 14:05, Thierry Reding пишет:
>>>>> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
>>>>>>
>>>>>>
>>>>>> On 04/02/2019 08:45, Thierry Reding wrote:
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>> The idea was, as I was saying below, to reuse dev_pm_ops even if
>>>>>>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
>>>>>>>
>>>>>>> 	pm_runtime_enable(dev)
>>>>>>> 	{
>>>>>>> 		if (!CONFIG_PM)
>>>>>>> 			if (dev->pm_ops->resume)
>>>>>>> 				dev->pm_ops->resume(dev);
>>>>>>>
>>>>>>> 		...
>>>>>>> 	}
>>>>>>>
>>>>>>> But that's admittedly somewhat of a stretch. This could of course be
>>>>>>> made somewhat nicer by adding an explicit variant, say:
>>>>>>>
>>>>>>> 	pm_runtime_enable_foo(dev)
>>>>>>> 	{
>>>>>>> 		if (!CONFIG_PM && dev->pm_ops->resume)
>>>>>>> 			return dev->pm_ops->resume(dev);
>>>>>>>
>>>>>>> 		return 0;
>>>>>>> 	}
>>>>>>>
>>>>>>> Maybe the fact that I couldn't come up with a good name is a good
>>>>>>> indication that this is a bad idea...
>>>>>>
>>>>>> How about some new APIs called ...
>>>>>>
>>>>>> pm_runtime_enable_get()
>>>>>> pm_runtime_enable_get_sync()
>>>>>> pm_runtime_put_disable() (implies a put_sync)
>>>>>>
>>>>>> ... and in these APIs we add ...
>>>>>>
>>>>>> pm_runtime_enable_get(dev)
>>>>>> {
>>>>>> 	if (!CONFIG_PM && dev->pm_ops->resume)
>>>>>> 		return dev->pm_ops->resume(dev);
>>>>>>
>>>>>> 	pm_runtime_enable(dev);
>>>>>> 	return pm_runtime_get(dev);
>>>>>> }
>>>>>
>>>>> Yeah, those sound sensible. I'm still a bit torn between this and just
>>>>> enforcing PM. At least on the display side I think we already require PM
>>>>> and with all the power domain work that you did we'd be much better off
>>>>> if we could just get rid of the !PM workarounds.
>>>>>
>>>>>>>>> This would be somewhat tricky because drivers
>>>>>>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
>>>>>>>>> that would result in an empty structure if !CONFIG_PM, but we could
>>>>>>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
>>>>>>>>> never be compiled out for this kind of case. Or such drivers could even
>>>>>>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
>>>>>>>>> always populated.
>>>>>>>>>
>>>>>>>>> Another way out of this would be to make sure we never run into the case
>>>>>>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
>>>>>>>>> should always be available. But is it guaranteed that runtime PM for the
>>>>>>>>> devices is functional in that case? From a cursory look at the code it
>>>>>>>>> would seem that way.
>>>>>>>>
>>>>>>>> If you select PM, then all of the requisite code should be there.
>>>>>>>
>>>>>>> We do this on 64-bit ARM, but there had been some pushback when we had
>>>>>>> proposed to do the same thing on 32-bit ARM. I think there were two
>>>>>>> concerns:
>>>>>>>
>>>>>>> 	1) select PM would force the setting for all platforms on multi-
>>>>>>> 	   platforms builds
>>>>>>>
>>>>>>> 	2) prevents anyone from disabling PM for debugging purposes
>>>>>>>
>>>>>>> 1) no longer seems to be valid because Rockchip already selects PM
>>>>>>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
>>>>>>> run a build with !PM in a very long time and I wouldn't be surprised if
>>>>>>> that was completely broken.
>>>>>>>
>>>>>>> Maybe we need to try this again since a couple of years have elapsed and
>>>>>>> runtime PM support on Tegra is much more mature at this point.
>>>>>>>
>>>>>>>> Alternatively, you can make the driver depend on PM.
>>>>>>>
>>>>>>> That's probably the easiest way out, but to be honest I think I'd prefer
>>>>>>> to just enforce PM and keep things simple.
>>>>>>>
>>>>>>> Jon, any objections?
>>>>>>
>>>>>> None, but seems overkill just for this case.
>>>>>
>>>>> But that's precisely the point. It's not just about this case. We've
>>>>> already got some drivers where we do a similar dance just to be able to
>>>>> support the, let's admit it, exotic case where somebody turns off PM. I
>>>>> think supporting !PM might have made sense at a point where we had no
>>>>> support for power management at all. But I think we've come a long way
>>>>> since then, and while we may still have some ways to go in some areas,
>>>>> we do fairly decent runtime PM most of the time, to the point where I no
>>>>> longer see any benefits in !PM.
>>>>
>>>> I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a
>>>> useful debug feature, it can't just go away. 
>>>
>>> What is it about disabling PM that you consider useful? I can understand
>>> why you'd want that option if power management is broken, but as far as
>>> I can tell, power management on Tegra is in pretty good state, and it's
>>> more likely that !PM would actually be broken (though I haven't built a
>>> configuration like that in a couple of years, so I'm speculating). We
>>> already can't disable PM on 64-bit ARM, so I don't understand why 32-bit
>>> ARM should be treated any differently.
>>
>> Yes, I want an option for debugging of a broken PM. It doesn't matter
>> that there are no known PM-related issues on Tegra today, it could
>> become a problem tomorrow. Probably a kernel boot parameter like
>> pm_debug_always_on would suit even better here. 
>>
>> Or maybe PM API already provides such debug options and I'm just not
>> aware about them?
> 
> I'm not sure I understand what you're trying to do. If you want to use
> !PM as a way to debug broken PM, how would that help? Presumably if you
> disable !PM then the problem would be gone? In other words, in order to
> reproduce a PM related issue don't you have to enable PM first?

Disabling PM globally is one of first diagnostics steps, at least it gives an idea about source of a problem.

> I'm not aware of an alternate mechanism to force PM to off once built
> into the kernel.

I think runtime PM could be disabled per-device. That's probably what tools like powertop use to tune power management.

>> Do you know what's the exact reason for ARM64 to not support !PM?
> 
> There's no reason per se not to support PM. It's just that at the time
> we made the decision that we didn't want to. I vaguely remember that we
> tried to do the same thing for 32-bit ARM, but at the time we would've
> been the only platform to select PM, and that would've meant enabling
> PM for all subarchitectures and therefore forcing PM in multi-platform
> builds. Today if you build multi-platform kernels, at least Rockchip
> will also forcibly enable PM for everyone.
> 
> I think you may also have objected at the time for similar reasons. But
> this was a long time ago (at least 2 to 3 years I would guess) and much
> has changed since then. Power management has gotten a lot better on
> Tegra, to the point where we actually need it in order to properly
> function. For example, Tegra DRM is not going to work with !PM because
> we rely on runtime PM while disabling and enabling display pipelines.

I also know that people are disabling PM for performance reasons, sometime it's more important to reduce system's response latency than to care about power savings. But that probably not very relevant to Tegra usages. Okay, I guess in general it should be fine to enforce PM requirement and improve debug-ability later on by as-needed basis.

On Monday, February 4, 2019 11:04:50 AM CET Jon Hunter wrote:
> 
> On 04/02/2019 08:16, Sameer Pujar wrote:
> 
> ...
> 
> > Objective is to have things working with or without CONFIG_PM enabled.
> > From previous comments and discussions it appears that there is mixed
> > response
> > for calling hda_tegra_runtime_resume() or runtime PM APIs in probe()
> > call. Need
> > to have consensus regarding the best practice to be followed, which
> > would eventually
> > can be used in other drivers too.
> > 
> > Rafael is suggesting to use CONFIG_PM check to do manual setup or
> > runtime PM setup in probe,
> > which would bring back the earlier above mentioned concern.
> > 
> > if (IS_ENABLED(CONFIG_PM)) {
> > do setup based on pm-runtime
> > } else {
> >     do manual setup
> > }
> > Both if/else might end up doing the same here.
> > Do we really need CONFIG_PM check here?
> > 
> > Instead does below proposal appear fine?
> > 
> > probe() {
> >     hda_tegra_enable_clock();
> > }
> > 
> > probe_work() {
> >     /* hda setup */
> >     . . .
> >     pm_runtime_set_active(); /* initial state as active */
> >     pm_runtime_enable();
> >     return;
> > }
> 
> I believe that this still does not work, because if there is a
> power-domain that needs to be turned on, this does not guarantee this.
> So I think that you need to call pm_runtime_get ...
> 
>  probe() {
>  	if (!IS_ENABLED(CONFIG_PM))
>  		hda_tegra_enable_clock();
>  }
> 

But alas, there are no PM domains with CONFIG_PM unset.

CONFIG_PM unset means that the *driver* has to know how to turn on the
device and that has to be sufficient.

Which basically is my point when I'm saying that this information is
not available to the core and it cannot do anything to handle this
case without the extra knowledge.

Thanks,
Rafael

On Monday, February 4, 2019 12:05:10 PM CET Thierry Reding wrote:
> 
> --FCuugMFkClbJLl1L
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> >=20
> >=20
> > On 04/02/2019 08:45, Thierry Reding wrote:
> >=20
> > ...
> >=20
> > > The idea was, as I was saying below, to reuse dev_pm_ops even if
> > > !CONFIG_PM. So pm_runtime_enable() could be something like this:
> > >=20
> > > 	pm_runtime_enable(dev)
> > > 	{
> > > 		if (!CONFIG_PM)
> > > 			if (dev->pm_ops->resume)
> > > 				dev->pm_ops->resume(dev);
> > >=20
> > > 		...
> > > 	}
> > >=20
> > > But that's admittedly somewhat of a stretch. This could of course be
> > > made somewhat nicer by adding an explicit variant, say:
> > >=20
> > > 	pm_runtime_enable_foo(dev)
> > > 	{
> > > 		if (!CONFIG_PM && dev->pm_ops->resume)
> > > 			return dev->pm_ops->resume(dev);
> > >=20
> > > 		return 0;
> > > 	}
> > >=20
> > > Maybe the fact that I couldn't come up with a good name is a good
> > > indication that this is a bad idea...
> >=20
> > How about some new APIs called ...
> >=20
> > pm_runtime_enable_get()
> > pm_runtime_enable_get_sync()
> > pm_runtime_put_disable() (implies a put_sync)
> >=20
> > ... and in these APIs we add ...
> >=20
> > pm_runtime_enable_get(dev)
> > {
> > 	if (!CONFIG_PM && dev->pm_ops->resume)
> > 		return dev->pm_ops->resume(dev);

No, we're not adding this to the core.

While in principle you could provide a set of pointers to the routines
you want to be called when CONFIG_PM is unset, IMO it is just cleaner
and more straightforward (and fewer lines of code for that matter) to add
a simple conditional to each ->probe() and ->remove().

[cut]

> > None, but seems overkill just for this case.
> 
> But that's precisely the point. It's not just about this case. We've
> already got some drivers where we do a similar dance just to be able to
> support the, let's admit it, exotic case where somebody turns off PM. I
> think supporting !PM might have made sense at a point where we had no
> support for power management at all. But I think we've come a long way
> since then, and while we may still have some ways to go in some areas,
> we do fairly decent runtime PM most of the time, to the point where I no
> longer see any benefits in !PM.

This IMO is an excellent point.

Trying to handle the !CONFIG_PM case only really makes sense if you know
what to do to turn the device on without relying on things like PM domains
etc which are not even available if CONFIG_PM is not set.

IOW, if any of the platforms your driver is expected to work with require
something like genpd to even make the device functional, I wouldn't bother.

Thanks,
Rafael