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:
>> @@ -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?
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.
>> @@ -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.
>> @@ -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.
> 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?
Cheers,
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
