From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.9 required=3.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,SPF_PASS,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D7313C5CFE7 for ; Wed, 11 Jul 2018 11:11:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7880E20BF2 for ; Wed, 11 Jul 2018 11:11:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="h81iCb+D" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7880E20BF2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732764AbeGKLPC (ORCPT ); Wed, 11 Jul 2018 07:15:02 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:42369 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732738AbeGKLPB (ORCPT ); Wed, 11 Jul 2018 07:15:01 -0400 Received: by mail-oi0-f67.google.com with SMTP id n84-v6so48485577oib.9; Wed, 11 Jul 2018 04:11:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=EKGKHItDX/QI+YCgFQKgtqPERxRU5C+HIIXW7R94pi4=; b=h81iCb+Dek0/9gyQ7P7zmsW2bfKVI+lCXcQ+xgX8xx5nnxRUZjstW4he346V5HtsT4 fQbH5rjb5GU15a+GnH9sUClF2jC8gDS9Ja4w5ZE3Qj7lTByfkJJHQwiomScMcUCf70vT E5YBXDn11cptFA+aqtqabWQqP6+A7EecHNLVYmmXdMN1UmlKeT/uUf+YyNVA9P1JLB3B e/pJIMAC60S95qpgpECm0y7dzjOC/FLuN2lfXTJ2wMSCAakqqQ7hcRq6IOlvt/tMBfk3 mPPEJ6CpdTie24RqC8Hj4+4a2lwPv9A+pVYbEiNLc6f08FHb6TRbdtcGp7cOiXuD+/4n NqNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=EKGKHItDX/QI+YCgFQKgtqPERxRU5C+HIIXW7R94pi4=; b=dFB14+7rloF/COTfviOCEBvNBZMA4zFR8BFHW541UxALDPo5ucfRr+dAk6WujKrPD/ Aqx3pnb9D3FNPCZjbMfcb3DX36Z+FjMd6IoYVz6r1DFgLO4wtjzHv1Br7qLLZ5k/CWE1 IQUbv2a+hkFOgmTiXtlaSFsnZKRlF/cAsSoev8IMtvKxR61JksdlmwVCpcC1JGSWawH7 HToK/qHapMk/yDLE6duQndMRYugnvLX2RH4rZRsFn80WxLmYl9ia0OZ83CIw1sbN+38L 3lnpsL6WqHucvHGZ88RgAdMIHsvfxs7kC9h4TRP+HukVo4XfQUvwIVTVrfp0RkEcy2Dh endw== X-Gm-Message-State: APt69E0dZ+MU4Utl1hVBffpEsccxZBjqXKniyn+gx0N+VAqsTo5+GnED s98Q53h7PfA+/gVOu6h6xya+TjcdlNXhVjBh1mM= X-Google-Smtp-Source: AAOMgpf0Xe+NL5o5eMVQ3zHA4/0dqOM9JJxGwKXfqS8L0iJ45/SaEpHT+K6PpUBqHc0hx0tKS3Nx25R9uJjMWPuO2ys= X-Received: by 2002:aca:adc6:: with SMTP id w189-v6mr33127066oie.174.1531307471861; Wed, 11 Jul 2018 04:11:11 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:63d2:0:0:0:0:0 with HTTP; Wed, 11 Jul 2018 04:11:11 -0700 (PDT) In-Reply-To: References: <20180708173413.1965-1-vivek.gautam@codeaurora.org> <20180708173413.1965-2-vivek.gautam@codeaurora.org> <17407514.unFVTGoGrn@aspire.rjw.lan> From: "Rafael J. Wysocki" Date: Wed, 11 Jul 2018 13:11:11 +0200 X-Google-Sender-Auth: uhIbX5QXcL3HiJt5cFSs7-Rwsyo Message-ID: Subject: Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops To: Vivek Gautam Cc: "Rafael J. Wysocki" , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , "robh+dt" , Mark Rutland , Robin Murphy , Will Deacon , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , Alex Williamson , Rob Clark , Linux PM , freedreno , Stephen Boyd , Tomasz Figa , Sricharan R , Marek Szyprowski , Archit Taneja , linux-arm-msm , Jordan Crouse Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki wrote: >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>> From: Sricharan R >>> >>> The smmu needs to be functional only when the respective >>> master's using it are active. The device_link feature >>> helps to track such functional dependencies, so that the >>> iommu gets powered when the master device enables itself >>> using pm_runtime. So by adapting the smmu driver for >>> runtime pm, above said dependency can be addressed. >>> >>> This patch adds the pm runtime/sleep callbacks to the >>> driver and also the functions to parse the smmu clocks >>> from DT and enable them in resume/suspend. >>> >>> Signed-off-by: Sricharan R >>> Signed-off-by: Archit Taneja >>> [vivek: Clock rework to request bulk of clocks] >>> Signed-off-by: Vivek Gautam >>> Reviewed-by: Tomasz Figa >>> --- >>> >>> - No change since v11. >>> >>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 58 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index f7a96bcf94a6..a01d0dde21dd 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -48,6 +48,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> >>> @@ -205,6 +206,8 @@ struct arm_smmu_device { >>> u32 num_global_irqs; >>> u32 num_context_irqs; >>> unsigned int *irqs; >>> + struct clk_bulk_data *clks; >>> + int num_clks; >>> >>> u32 cavium_id_base; /* Specific to Cavium */ >>> >>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) >>> struct arm_smmu_match_data { >>> enum arm_smmu_arch_version version; >>> enum arm_smmu_implementation model; >>> + const char * const *clks; >>> + int num_clks; >>> }; >>> >>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \ >>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp } >>> +static const struct arm_smmu_match_data name = { .version = ver, .model = imp } >>> >>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU); >>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU); >>> @@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[] = { >>> }; >>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match); >>> >>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu, >>> + const char * const *clks) >>> +{ >>> + int i; >>> + >>> + if (smmu->num_clks < 1) >>> + return; >>> + >>> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks, >>> + sizeof(*smmu->clks), GFP_KERNEL); >>> + if (!smmu->clks) >>> + return; >>> + >>> + for (i = 0; i < smmu->num_clks; i++) >>> + smmu->clks[i].id = clks[i]; >>> +} >>> + >>> #ifdef CONFIG_ACPI >>> static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu) >>> { >>> @@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev, >>> data = of_device_get_match_data(dev); >>> smmu->version = data->version; >>> smmu->model = data->model; >>> + smmu->num_clks = data->num_clks; >>> + >>> + arm_smmu_fill_clk_data(smmu, data->clks); >>> >>> parse_driver_options(smmu); >>> >>> @@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >>> smmu->irqs[i] = irq; >>> } >>> >>> + err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks); >>> + if (err) >>> + return err; >>> + >>> + err = clk_bulk_prepare(smmu->num_clks, smmu->clks); >>> + if (err) >>> + return err; >>> + >>> err = arm_smmu_device_cfg_probe(smmu); >>> if (err) >>> return err; >>> @@ -2181,6 +2214,9 @@ static int arm_smmu_device_remove(struct platform_device *pdev) >>> >>> /* Turn the thing off */ >>> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); >>> + >>> + clk_bulk_unprepare(smmu->num_clks, smmu->clks); >>> + >>> return 0; >>> } >>> >>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>> return 0; >>> } >>> >>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>> +{ >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> + >>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>> +} >>> + >>> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev) >>> +{ >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> + >>> + clk_bulk_disable(smmu->num_clks, smmu->clks); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct dev_pm_ops arm_smmu_pm_ops = { >>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >> >> This is suspicious. >> >> If you need a runtime suspend method, why do you think that it is not necessary >> to suspend the device during system-wide transitions? > > Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? > In that case the clocks have to be enabled in the resume path too. > > I remember Tomasz pointed to that we shouldn't need clock enable in resume > path [1]. > > [1] https://lkml.org/lkml/2018/3/15/60 Honestly, I just don't know. :-) It just looks odd the way it is done. I think the clock should be gated during system-wide suspend too, because the system can spend much more time in a sleep state than in the working state, on average. And note that you cannot rely on runtime PM to always do it for you, because it may be disabled at a client device or even blocked by user space via power/control in sysfs and that shouldn't matter for system-wide PM. Thanks, Rafael