On Fri, Oct 12, 2012 at 7:46 PM, Omar Ramirez Luna <omar.luna@xxxxxxxxxx> wrote:
> On 12 October 2012 02:48, Felipe Contreras <felipe.contreras@xxxxxxxxx> wrote:
>> I already made most of these comments, but here they go again.
>
> I replied to all, but here it goes again:
Mostly, but not all :)
>>> @@ -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.
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.
I'm not saying I have a solution, I'm simply saying that's what's
going to happen if I'm correct.
> 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?
>>> @@ -288,7 +285,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>>> if (!obj || !obj->nr_tlb_entries || !e)
>>> return -EINVAL;
>>>
>>> - clk_enable(obj->clk);
>>> + pm_runtime_get_sync(obj->dev);
>>>
>>> iotlb_lock_get(obj, &l);
>>> if (l.base == obj->nr_tlb_entries) {
>>> @@ -318,7 +315,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>>>
>>> cr = iotlb_alloc_cr(obj, e);
>>> if (IS_ERR(cr)) {
>>> - clk_disable(obj->clk);
>>> + pm_runtime_put_sync(obj->dev);
>>> return PTR_ERR(cr);
>>> }
>>
>> If I'm correct, the above pm_runtime_get/put are redundant, because
>> the count can't possibly reach 0 because of the reason I explained
>> before.
>>
>> The same for all the cases below.
>
> You can access this paths through debugfs, some of them doesn't work
> if the module is not enabled first, but in future if you just want to
> idle the iommu withouth freeing, these are needed to debug.
>
> BTW, the next patch in the series: ARM: OMAP: iommu: pm runtime save
> and restore context, let's you do a pm_runtime_[enable|put] through
> save/restore ctx functions, which is just for compatibility on how isp
> code uses the save and restore code.
All right, it has been some time since I've touched pm code, so if you say so.
>>> @@ -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.
But at least you agree that there's a chance that the device will be waken up.
>> 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.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
