From patchwork Fri Oct 12 07:48:33 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Contreras X-Patchwork-Id: 1585741 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 4F78DDFF71 for ; Fri, 12 Oct 2012 07:48:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932699Ab2JLHsf (ORCPT ); Fri, 12 Oct 2012 03:48:35 -0400 Received: from mail-oa0-f46.google.com ([209.85.219.46]:58255 "EHLO mail-oa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932482Ab2JLHse (ORCPT ); Fri, 12 Oct 2012 03:48:34 -0400 Received: by mail-oa0-f46.google.com with SMTP id h16so2624888oag.19 for ; Fri, 12 Oct 2012 00:48:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=jaAzEHfMVaKjtMHIWvQerDa1RyTtfEz6enP7TZQGqmQ=; b=d+4s5i9F3oi5RPsFsVZ7Mpv/VyHDLyonNYJpEla8ZO+VZxchE7OXX9lSRWiV2Tu3hG 2L126P6hBi8dcCHk+srJA9wOtKIhSlq9ATAXUPCZMGtasnZPMTd3XhPwGh6qxZcUITXm LxvfhQLGeCJFTkRQy3rnv6xR9Idkn/UDIFoYq31Bi/qZEEGa4o0hsS0QH0Iz6yZyjgCP LRdR7oyYp9IL721n87qP8nqbWIaXg9ERn9LLeDN595caS8/1HXAT3HVOj7a8Uz0RnIZO zA+8HZelqQdNnlPXVXnZd4q35i5hHe1gZtxy5xp/HhFWjpmLyPwCtpc1g3xK460FKqUR 0MNA== MIME-Version: 1.0 Received: by 10.182.10.6 with SMTP id e6mr2857227obb.16.1350028113769; Fri, 12 Oct 2012 00:48:33 -0700 (PDT) Received: by 10.60.58.137 with HTTP; Fri, 12 Oct 2012 00:48:33 -0700 (PDT) In-Reply-To: <1350003977-32744-5-git-send-email-omar.luna@linaro.org> References: <1350003977-32744-1-git-send-email-omar.luna@linaro.org> <1350003977-32744-5-git-send-email-omar.luna@linaro.org> Date: Fri, 12 Oct 2012 09:48:33 +0200 Message-ID: Subject: Re: [PATCH 2/6] ARM: OMAP3/4: iommu: adapt to runtime pm From: Felipe Contreras To: Omar Ramirez Luna Cc: Tony Lindgren , Joerg Roedel , Russell King , Benoit Cousson , Ohad Ben-Cohen , Paul Walmsley , Laurent Pinchart , devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org, iommu@lists.linux-foundation.org Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org On Fri, Oct 12, 2012 at 3:06 AM, Omar Ramirez Luna 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. > @@ -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. > @@ -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. > @@ -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: 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. --- 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),