[PATCH 2/6] ARM: OMAP3/4: iommu: adapt to runtime pm

[Felipe Contreras]

Hi,

On Tue, Oct 16, 2012 at 3:29 AM, Omar Ramirez Luna <omar.luna at linaro.org> wrote:

>> After your patch, even if I don't use the camera, or the DSP, the
>> iommu devices will be enabled, and will be consuming energy *all the
>> time*. Which I don't think is what we want.
>
> Wrong, the iommu device will be enabled by pm_runtime_get_sync once
> you decide to attach with iommu_attach_device, if you do not use
> camera or the dsp, you won't turn ON the iommus.

I see, somehow I conflated the two functions.

> On probe this patch does pm_runtime_enable, however this doesn't mean
> the device is turned ON or resumed or kept ON all the time.

In fact it's the other way around; pm_runtime_enable turns off the
power (if it's ON).

>>>>> @@ -1009,7 +1001,8 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev)
>>>>>         release_mem_region(res->start, resource_size(res));
>>>>>         iounmap(obj->regbase);
>>>>>
>>>>> -       clk_put(obj->clk);
>>>>> +       pm_runtime_disable(obj->dev);
>>>>
>>>> This will turn on the device unnecessarily, wasting power, and there's
>>>> no need for that, kfree will take care of that without resuming.
>>>
>>> Left aside the aesthetics of having balanced calls, the device will be
>>> enabled if there was a pending resume to be executed, otherwise it
>>> won't, kfree won't increment the disable_depth counter and I don't
>>> think that freeing the pointer is enough reason to ignore
>>> pm_runtime_disable.
>>
>> You are doing __pm_runtime_disable(dev, true), kfree will do
>> __pm_runtime_disable(dev, false), which is what we want. Both will
>> decrement the disable_depth.
>
> I'm quite confused here, could you please point me to the kfree snip
> that does __pm_runtime_disable(dev, false)?

Sorry, not kfree, but the device removal process:

device_del
 device_pm_remove
  pm_runtime_remove
   __pm_runtime_disable <- HERE
 bus_remove_device
  device_release_driver
   __device_release_driver
    .remove => platform_drv_remove
     .remove => omap_iommu_remove

Actually, it seems the pm is disabled _before_ omap_iommu_remove is
even called, so it's a no-op.

>> But at least you agree that there's a chance that the device will be waken up.
>
> Of course, if there is a pending resume to be executed, it must honor
> that resume request and then turn off the device before removing the
> iommu, IMHO.

Who will turn off the device afterwards?

>>>> Also, I still think that something like this is needed:
>>> ...
>>>> +static struct clk cam_fck = {
>>>> +       .name           = "cam_fck",
>>>> +       .ops            = &clkops_omap2_iclk_dflt,
>>>> +       .parent         = &l3_ick,
>>>> +       .enable_reg     = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN),
>>>
>>> a cam_fck name to enable the ick?
>>
>> Yeap, according to the TRM. Take a look at 12.3 Camera ISP Integration
>> Fig 12-50.
>
> What I meant is that, you are using the CM_ICLKEN to enable a clock
> named "cam_fck" which has l3_ick as a parent. And that is not
> consistent with what that register is meant to do, which is:
>
> 4.14.1.10 CAM_CM Registers
>
> CM_ICKLEN_CAM
> 0x0: CAM_L3_ICK and CAM_L4_ICLK are disabled
> 0x1: CAM_L3_ICK and CAM_L4_ICLK are enabled
>
> So, I'm complaining about the name "cam_fck", for an interface clock
> with parent l3_ick. However I don't know why on section 12.3 they
> refer to CAM_FCK to a l3_ick child clock.

Because it's also used as a functional clock.  Anyway, I don't care
much about the name of the clock, what is clear is that there's a link
missing to the l3_ick.

Cheers.

-- 
Felipe Contreras