Re: [PATCH 2/6] ARM: OMAP3/4: iommu: adapt to runtime pm
From: Felipe Contreras <hidden>
Date: 2012-10-12 07:48:33
Also in:
linux-arm-kernel, linux-iommu, linux-omap
On Fri, Oct 12, 2012 at 3:06 AM, Omar Ramirez Luna [off-list ref] wrote:
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations and sysconfig handling. Dues to reset sequence, pm_runtime_put_sync must be used, to avoid possible operations with the module under reset.
I already made most of these comments, but here they go again.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
dev_info(&pdev->dev, "%s removed\n", obj->name);
kfree(obj);
return 0;Also, I still think that something like this is needed:
--- a/arch/arm/mach-omap2/clock3xxx_data.c
+++ b/arch/arm/mach-omap2/clock3xxx_data.c@@ -2222,8 +2222,17 @@ static struct clk cam_mclk = { .recalc = &followparent_recalc, }; +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), + .enable_bit = OMAP3430_EN_CAM_SHIFT, + .clkdm_name = "cam_clkdm", + .recalc = &followparent_recalc, +}; + static struct clk cam_ick = { - /* Handles both L3 and L4 clocks */ .name = "cam_ick", .ops = &clkops_omap2_iclk_dflt, .parent = &l4_ick,
@@ -3394,6 +3403,7 @@ static struct omap_clk omap3xxx_clks[] = { CLK("omapdss_dss", "ick", &dss_ick_3430es2, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK(NULL, "dss_ick", &dss_ick_3430es2, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK(NULL, "cam_mclk", &cam_mclk, CK_34XX | CK_36XX), + CLK(NULL, "cam_fck", &cam_fck, CK_34XX | CK_36XX), CLK(NULL, "cam_ick", &cam_ick, CK_34XX | CK_36XX), CLK(NULL, "csi2_96m_fck", &csi2_96m_fck, CK_34XX | CK_36XX), CLK(NULL, "usbhost_120m_fck", &usbhost_120m_fck,
CK_3430ES2PLUS | CK_AM35XX | CK_36XX), Cheers. -- Felipe Contreras