Received: by 2002:a05:6a10:d5a5:0:0:0:0 with SMTP id gn37csp227565pxb; Wed, 6 Oct 2021 03:48:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzfe77gXM/ek6RAf/5Op0fS95hGw4jhi5+qGzbi4OiZZpwBUhMtx+RdJb2QzEGd1iXxpGs6 X-Received: by 2002:a17:906:71d4:: with SMTP id i20mr31346800ejk.390.1633517326564; Wed, 06 Oct 2021 03:48:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1633517326; cv=none; d=google.com; s=arc-20160816; b=ltKfhp6OaBVgY5txWGK8hZLfn8l1Kadbp0VUaZ+apRG2ewgqsAOlld1lCUR2cHCMH/ UAGfwBf6/AaHGJjHT65RNTg8cu4Kh3vWhDHFyI9Dnt/yAwQaDDOCb2EfvJkE0DIvvGeY cw/PASR8sBavTa5GCzeW43QKcS0i1e1z4r86NsEzf5zmW8dptI1lT/Sxr/cgbTViTfBm H//yk5Zl8Kl11mkIwrOGyvLeuXJwvlaC9OGvWwdBEPVABYVigb1Gv8wNcWmk7zEPU55w ZHNL4dCwiXDmzYEj21p9WH+Zg6Yf5WvqHx45RMMco4qfV+9N1GqV07HRzjEU8FrHha74 ylqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=WAZ6VYVxXpp+/4weV4Gw1QKhKbSTkxuMKBLfGMMXi34=; b=unLigt6s2iA6ImTiloRrIeeJ77Io56GOS8Rw8XxJCE2tM75c+07xGJXFTesLOoEQJU 7MVUUf8jxWXEGPxSX6uYa5ih5JVXduJRUC1/m1Qga6XGOMOQDi50WbxMwCpMSiUPJunj DJeiz5TdU4VF6o75eLKflMx+ljY6P+jKksxpDdfmzLmDOxS7gB8T8FzwU70jfkQhhjaP VCNPHJEXSqVMDdxnTOAsEQIwiSglsHozXV9/7NP+obdWlKZxZKLx15juFkLBEupQUHcp sPuA9LfqvnTokDZBYmwcar8zSSXbwgRtJsCwvY15w95Hmw6af8q5znGuRz3epj6MEn15 KPxA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=RW9U4DrK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id y10si249147ejm.92.2021.10.06.03.48.20; Wed, 06 Oct 2021 03:48:46 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=RW9U4DrK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237836AbhJFKsY (ORCPT + 99 others); Wed, 6 Oct 2021 06:48:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41752 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237931AbhJFKsX (ORCPT ); Wed, 6 Oct 2021 06:48:23 -0400 Received: from mail-ua1-x92c.google.com (mail-ua1-x92c.google.com [IPv6:2607:f8b0:4864:20::92c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 59C95C061755 for ; Wed, 6 Oct 2021 03:46:31 -0700 (PDT) Received: by mail-ua1-x92c.google.com with SMTP id e7so1425424ual.11 for ; Wed, 06 Oct 2021 03:46:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=WAZ6VYVxXpp+/4weV4Gw1QKhKbSTkxuMKBLfGMMXi34=; b=RW9U4DrKauxl7RiYBSIz5WrXXIfcuHG+K0ocrRpKxF4zt5xL9u4Fg9K6qYhR9lBPUl y93WKKhFCAtxSPKBKYCjA4QJ+8FwXc/5sku7+kPilJLHB2VdACALVbJaOlRA89g7O7DU Ik1VVeEJ4TJgHGexGQuKQSL+001+Wq5E7FzeRwC0K5S7UU8WQoM187ddsNnLUvQ1xA0R C684SbtTBtrE4+uvIvzyL09hXp/luX8lpE8xIKgenBW874pWNGtFq5b9x5LhNP000LvE yHp1CV3ozWmUotvTQbqGaUmaacMrD4brCiA2pHu/4d0FkCUHrgYmDY29/2zgPC2K3l2j nbeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=WAZ6VYVxXpp+/4weV4Gw1QKhKbSTkxuMKBLfGMMXi34=; b=K6C42V4Dh5/OsU+EV4N3ma7raIfqCF8kJvOdTvd8iDFAQECGHdk6K5F8VdJLz3zaXG NzYPgEdXJX5jpC5PM8w4JlUII7ZzlQVHb7FuxGa9eZPpViUrYTQ6ipm6FOLfQ7+/P8sc Qv4jgarUG3zAz0j47v0GXx5S3qIMjnzSZZKQbVdniqlqqqEZsD1FheWY9onRm179dmH9 ksuYuIl9jkVqUn9WgEoLYSD/FRMF2FLW3dKa+VfNxKkD2b9quKfOVoPZusoTrCE9oCkY TPU3iaTOKYj4vfE9AVcRsAOT0+KWniyB/kHfCBZdEBxYDFSthFWlyAL7Xipof9oS/LoF ZK7Q== X-Gm-Message-State: AOAM533HhNgfTid/zxWTe4Q3Kesn9HquEt5xGgUkImjLXwmnDj20DXKs qEDNCa25AYMCpCK5fEnvmZGBCsLbNI0/Nyp0QW744Q== X-Received: by 2002:ab0:3303:: with SMTP id r3mr17099371uao.17.1633517190130; Wed, 06 Oct 2021 03:46:30 -0700 (PDT) MIME-Version: 1.0 References: <20210914155607.14122-1-semen.protsenko@linaro.org> <20210914155607.14122-2-semen.protsenko@linaro.org> <6ef3e9a3-77e7-48b7-cbcd-c13db50d0cd9@canonical.com> In-Reply-To: <6ef3e9a3-77e7-48b7-cbcd-c13db50d0cd9@canonical.com> From: Sam Protsenko Date: Wed, 6 Oct 2021 13:46:18 +0300 Message-ID: Subject: Re: [PATCH 1/6] clk: samsung: Enable bus clock on init To: Krzysztof Kozlowski Cc: Sylwester Nawrocki , =?UTF-8?Q?Pawe=C5=82_Chmiel?= , Chanwoo Choi , Tomasz Figa , Rob Herring , Stephen Boyd , Michael Turquette , Ryu Euiyoul , Tom Gall , Sumit Semwal , John Stultz , Amit Pundir , devicetree , linux-arm Mailing List , linux-clk , Linux Kernel Mailing List , Linux Samsung SOC Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski wrote: > > On 14/09/2021 17:56, Sam Protsenko wrote: > > By default if bus clock has no users its "enable count" value is 0. It > > might be actually running if it's already enabled in bootloader, but > > then in some cases it can be disabled by mistake. For example, such case > > was observed when dw_mci_probe() enabled bus clock, then failed to do > > something and disabled that bus clock on error path. After that even > > attempt to read the 'clk_summary' file in DebugFS freezed forever, as > > CMU bus clock ended up being disabled and it wasn't possible to access > > CMU registers anymore. > > > > To avoid such cases, CMU driver must increment the ref count for that > > bus clock by running clk_prepare_enable(). There is already existing > > '.clk_name' field in struct samsung_cmu_info, exactly for that reason. > > It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add > > support for runtime PM"). But the clock is actually enabled only in > > Exynos5433 clock driver. Let's mimic what is done there in generic > > samsung_cmu_register_one() function, so other drivers can benefit from > > that `.clk_name' field. As was described above, it might be helpful not > > only for PM reasons, but also to prevent possible erroneous clock gating > > on error paths. > > > > Another way to workaround that issue would be to use CLOCK_IS_CRITICAL > > flag for corresponding gate clocks. But that might be not very good > > design decision, as we might still want to disable that bus clock, e.g. > > on PM suspend. > > > > Signed-off-by: Sam Protsenko > > --- > > drivers/clk/samsung/clk.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c > > index 1949ae7851b2..da65149fa502 100644 > > --- a/drivers/clk/samsung/clk.c > > +++ b/drivers/clk/samsung/clk.c > > @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one( > > > > ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids); > > > > + /* Keep bus clock running, so it's possible to access CMU registers */ > > + if (cmu->clk_name) { > > + struct clk *bus_clk; > > + > > + bus_clk = __clk_lookup(cmu->clk_name); > > + if (bus_clk) { > > + clk_prepare_enable(bus_clk); > > + } else { > > + pr_err("%s: could not find bus clock %s\n", __func__, > > + cmu->clk_name); > > + } > > + } > > + > > Solving this problem in generic way makes sense but your solution is > insufficient. You skipped suspend/resume paths and in such case you > should remove the Exynos5433-specific code. > Keeping core bus clocks always running seems like a separate independent feature to me (not related to suspend/resume). It's mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add support for runtime PM") this way: "Also for each CMU there is one special parent clock, which has to be enabled all the time when any access to CMU registers is being done." Why do you think suspend/resume paths have to be implemented along with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not implemented on my board yet and I can't test it. If you are suggesting moving all stuff from exynos5433_cmu_probe() into samsung_cmu_register_one(), it would take passing platform_device there, and implementing all PM related operations. I guess it's not a super easy task, as it would require converting clk-exynos7 to platform_driver for instance, and re-testing everything on exynos5433 and exynos7 boards (which I don't have). What do you say if I pull that code to clk-exynos850.c instead for v2? Refactoring (merging stuff from exynos5433_cmu_probe() into samsung_cmu_register_one() ) can be done later, when I add PM ops into clk-exynos850. > Best regards, > Krzysztof