Hi Felipe, On 12 October 2012 16:25, Felipe Contreras <felipe.contre...@gmail.com> wrote: >>>> @@ -142,11 +142,10 @@ static int iommu_enable(struct omap_iommu *obj) >>>> } >>>> } >>>> >>>> - clk_enable(obj->clk); >>>> + pm_runtime_get_sync(obj->dev); >>>> >>>> err = arch_iommu->enable(obj); >>>> >>>> - clk_disable(obj->clk); >>> >>> The device will never go to sleep, until iommu_disable is called. >>> clk_enable -> pm_runtime_get_sync, clk_disable pm_runtime_put. >> >> Which is what you want... why would you want your iommu to be disabled >> if the client of that iommu could request a translation? > > That's the whole point of *dynamic* pm; _when_ the client wants to > request a translation, _then_ the device is waken up, which is what I > believe the code currently does.
No it doesn't, current code is working because the processor and the iommu share the same clock, so enabling the processor is implicitly guaranteeing that the iommu will be enabled. IMHO, there shouldn't be such assumption that you can control both with the same clock. So, once the remote processor is enabled, any "dynamic pm" from iommu with current code has no effect because the clock was already enabled for the processor. > 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. 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. > I'm not saying I have a solution, I'm simply saying that's what's > going to happen if I'm correct. Ok, but that is not what happens here. >> Remember that these iommus, sit along of other processors not on the >> main processor side. So, this code should enable it for the other >> processor to use, and there is no point where the processor can say >> "I'm not using it, shut it down" or "I'm using it, turn it on" in the >> middle of execution, other than suspend/resume and if supported, >> autosuspend. > > I understand, but perhaps there should be? Autosuspend is a feature missing and should handle the scenario where the remote processor can sleep dynamically, this scenario should turn off the iommu and the remote processor itself when there is no workload but it depends on the remote processor activity not the iommu activity. >>>> @@ -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)? > 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. >>> 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. Cheers, Omar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html