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

Hi,

On Tue, Oct 16, 2012 at 3:29 AM, Omar Ramirez Luna [off-list ref] wrote:

quoted

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).

quoted

quoted

quoted

quoted

@@ -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.

quoted

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?

quoted

quoted

quoted

Also, I still think that something like this is needed:

...

quoted

+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