Re: [PATCH 4.6 69/81] gpio: zynq: initialize clock even without CONFIG_PM

Re: [PATCH 4.6 69/81] gpio: zynq: initialize clock even without CONFIG_PM

[Helmut Grohne]

On Fri, Jul 08, 2016 at 10:23:07AM +0200, Shubhrajyoti Datta wrote:
>  A small issue
> 
> Now it will be off by 1 ie we enable twice in probe.
> 
> solution is that either there should be get_sync or clk_enable but not both.

As far as I understand the inner workings of pm_runtime_get_sync this is
not a problem.

> > +       ret = clk_prepare_enable(gpio->clk);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> > +               return ret;
> > +       }
> 
> This will now enable the first time.
> >
> > +       pm_runtime_set_active(&pdev->dev);
> the the pm core that it is enabled .

This function sets pdev->dev.power.runtime_status to RPM_ACTIVE (via
__update_runtime_status).

> >         pm_runtime_enable(&pdev->dev);
> >         ret = pm_runtime_get_sync(&pdev->dev);
> 
> this will enable the second time.

The bulk of the work is done by rpm_resume. Close to the top of the
function the runtime_status field is compared to RPM_ACTIVE and if so,
the resume operation is skipped.

Is there any mistake in that analysis?

Helmut

Re: [PATCH 4.6 69/81] gpio: zynq: initialize clock even without CONFIG_PM

[Shubhrajyoti Datta]

On Thu, Jul 21, 2016 at 4:27 PM, Helmut Grohne <h.grohne@intenta.de> wrote:
> On Fri, Jul 08, 2016 at 10:23:07AM +0200, Shubhrajyoti Datta wrote:
>>  A small issue
>>
>> Now it will be off by 1 ie we enable twice in probe.
>>
>> solution is that either there should be get_sync or clk_enable but not both.
>
> As far as I understand the inner workings of pm_runtime_get_sync this is
> not a problem.

the issue is not with the runtime.
>
>> > +       ret = clk_prepare_enable(gpio->clk);
>> > +       if (ret) {
>> > +               dev_err(&pdev->dev, "Unable to enable clock.\n");
>> > +               return ret;
>> > +       }
>>
>> This will now enable the first time.
>> >
>> > +       pm_runtime_set_active(&pdev->dev);
>> the the pm core that it is enabled .
>
> This function sets pdev->dev.power.runtime_status to RPM_ACTIVE (via
> __update_runtime_status).
>
>> >         pm_runtime_enable(&pdev->dev);
>> >         ret = pm_runtime_get_sync(&pdev->dev);
>>
>> this will enable the second time.
>
> The bulk of the work is done by rpm_resume. Close to the top of the
> function the runtime_status field is compared to RPM_ACTIVE and if so,
> the resume operation is skipped.
>
> Is there any mistake in that analysis?

I feel that if you can print the clk enable count in at boot things
could be easy
to understand.

>
> Helmut

Re: [PATCH 4.6 69/81] gpio: zynq: initialize clock even without CONFIG_PM

[Helmut Grohne]

On Tue, Aug 02, 2016 at 10:59:05AM +0200, Shubhrajyoti Datta wrote:
> I feel that if you can print the clk enable count in at boot things
> could be easy
> to understand.

I verifed this via two ways now (holds both for CONFIG_PM=y and
CONFIG_PM=n):

 * zynq_gpio_runtime_resume is not called (as expected given my
   analysis). I simply added a printk to check that.
 * After boot, /sys/kernel/debug/clk/gpio_aper/clk_enable_count also
   confirms my analysis by yielding "1".

Thus I conclude that there there is no bug. Neither in theory nor in
practise.

Helmut