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=-9.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 6A4FEC4727C for ; Thu, 1 Oct 2020 06:15:51 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C52572074B for ; Thu, 1 Oct 2020 06:15:50 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="AOnEaTlx"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="nsKh9Smy" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C52572074B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=UWVJQp29uEwEMINts90vPvjanarmuwY+fbaCsZDOVXQ=; b=AOnEaTlxrxqGbDD9/8Yj2+hLi gzBKz80IEBPlLEC5CGTKLxWd9qeZoAMoNAsnjSyA2NUnDaGxwrNDfQBbB3wIispgNAcn27YukpZ4T EvKxlJcxgJIFV1G5732k1kTn7iSRmuUUC4csQD4FyS1jpIoXWgbYa1qPXhr/XHD9x/+RojwIgGvXI dzmJW0XRWhSexFKLYTV/GazksjKhjA63LL2kRgv/TKpVjJ3GJ4+tRyILiSB8Cj0KFxWfVeF2lsldP 2izIXKFYJCgClIxlhQs0Wsi0tzuKpioSbrzHO80jLJd56Nkek4BhtpDFG90A8/Q4hSEgmgNzm2mNj 6nAkBdBiw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNrrB-00027g-BI; Thu, 01 Oct 2020 06:14:21 +0000 Received: from mail-vs1-xe41.google.com ([2607:f8b0:4864:20::e41]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNrr8-00025x-Ba for linux-arm-kernel@lists.infradead.org; Thu, 01 Oct 2020 06:14:19 +0000 Received: by mail-vs1-xe41.google.com with SMTP id y194so2075435vsc.4 for ; Wed, 30 Sep 2020 23:14:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VlK25AeCqL3jo7oZoKivx7ZKCZnGaTWNtqH24P3Rx58=; b=nsKh9SmyYr6dFqhw75fYDL7n5mG3UUdgFMIuakzzITavWvq6EHjqF1X9nWwIt2r+wX ojx5yjM71XwFhwhBRKWEodxX/dyhQeHrEI6fO/7tXmPZCrQuvRAeuMTU8UkiYbAfYgfV y257KdPrMIfZeX9eyDP+cp0PpHAtvE9Vqtfko= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VlK25AeCqL3jo7oZoKivx7ZKCZnGaTWNtqH24P3Rx58=; b=Bq9RVZptLA7JvG/erA5hrLi0JAqXALAIvVeApQU/hKukpsixHQlI1o3aPj8uONNnmb VScbcnjIs/WMYjW3lheH2pNn+uj4b7QLLRukIa1lJh86fX2wf9buelsynnhtmHXNk+pu zEPhYJJMIcfhv7bh27jjKfRQdCqerEZy2qUJypBW417CTCbPd0ChlXt1Ods3XKsRSVNX 9jFfhzgYLsO84lLzigyurDLEzGeiirB5+O72zsvZbai8NXz8WD+t7Bc1x3axbQAEnnCf LaUWTGcvbbuisBwGOPct6wnd2PkrYqkB8vBKw19buN4NmnzQETdu7UU1Fgcizqp46Jn0 29dQ== X-Gm-Message-State: AOAM530o86NqAufIO3iWWJ4soreuB6KgRY8LjBFw6JFpTAc86lYra4E8 cHN9h4ScEBwi++mMH8i1m4L9FEhBBvL2YaorXewvyg== X-Google-Smtp-Source: ABdhPJxLXQQGLRLzY6RFBL5DI0m1HQoOFsQi0w8LIWHFTIqUlUJYtSC0vKE7KVqUO73TNfZ8033bmhNJ5nUfTYZXppE= X-Received: by 2002:a67:be08:: with SMTP id x8mr4072437vsq.47.1601532853052; Wed, 30 Sep 2020 23:14:13 -0700 (PDT) MIME-Version: 1.0 References: <20200930083120.11971-1-wenbin.mei@mediatek.com> <20200930083120.11971-5-wenbin.mei@mediatek.com> In-Reply-To: <20200930083120.11971-5-wenbin.mei@mediatek.com> From: Nicolas Boichat Date: Thu, 1 Oct 2020 14:14:02 +0800 Message-ID: Subject: Re: [PATCH v3 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc To: Wenbin Mei X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201001_021418_405369_7E48DCA6 X-CRM114-Status: GOOD ( 27.11 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Devicetree List , Ulf Hansson , srv_heupstream , linux-mmc@vger.kernel.org, lkml , Rob Herring , "moderated list:ARM/Mediatek SoC support" , linux-arm Mailing List , Matthias Brugger , Chaotian Jing Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 30, 2020 at 4:31 PM Wenbin Mei wrote: > > MT8192 msdc is an independent sub system, we need control more bus > clocks for it. > Add support for the additional subsys clocks to allow it to be > configured appropriately. > > Signed-off-by: Wenbin Mei > --- > drivers/mmc/host/mtk-sd.c | 77 ++++++++++++++++++++++++++++++--------- > 1 file changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > index a704745e5882..9a1422955593 100644 > --- a/drivers/mmc/host/mtk-sd.c > +++ b/drivers/mmc/host/mtk-sd.c > @@ -425,6 +425,8 @@ struct msdc_host { > struct clk *h_clk; /* msdc h_clk */ > struct clk *bus_clk; /* bus clock which used to access register */ > struct clk *src_clk_cg; /* msdc source clock control gate */ > + struct clk *sys_clk_cg; /* msdc subsys clock control gate */ > + struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */ > u32 mclk; /* mmc subsystem clock frequency */ > u32 src_clk_freq; /* source clock frequency */ > unsigned char timing; > @@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks) > > static void msdc_gate_clock(struct msdc_host *host) > { > + clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks), > + host->bulk_clks); > clk_disable_unprepare(host->src_clk_cg); > clk_disable_unprepare(host->src_clk); > clk_disable_unprepare(host->bus_clk); > @@ -792,10 +796,17 @@ static void msdc_gate_clock(struct msdc_host *host) > > static void msdc_ungate_clock(struct msdc_host *host) > { > + int ret; > + > clk_prepare_enable(host->h_clk); > clk_prepare_enable(host->bus_clk); > clk_prepare_enable(host->src_clk); > clk_prepare_enable(host->src_clk_cg); > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks), > + host->bulk_clks); > + if (ret) > + dev_dbg(host->dev, "enable clks failed!\n"); dev_err looks a lot more appropriate. Also, don't you want to exit the function in that case, rather than going to the while loop below where you may get stuck? > + > while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB)) > cpu_relax(); > } > @@ -2366,6 +2377,52 @@ static void msdc_of_property_parse(struct platform_device *pdev, > host->cqhci = false; > } > > +static int msdc_of_clock_parse(struct platform_device *pdev, > + struct msdc_host *host) > +{ > + struct clk *clk; > + > + host->src_clk = devm_clk_get(&pdev->dev, "source"); > + if (IS_ERR(host->src_clk)) > + return PTR_ERR(host->src_clk); > + > + host->h_clk = devm_clk_get(&pdev->dev, "hclk"); > + if (IS_ERR(host->h_clk)) > + return PTR_ERR(host->h_clk); > + > + host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk"); > + if (IS_ERR(host->bus_clk)) > + host->bus_clk = NULL; Use devm_clk_get_optional instead (ditto for the next 2). > + > + /*source clock control gate is optional clock*/ > + host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg"); > + if (IS_ERR(host->src_clk_cg)) > + host->src_clk_cg = NULL; > + > + host->sys_clk_cg = devm_clk_get(&pdev->dev, "sys_cg"); > + if (IS_ERR(host->sys_clk_cg)) > + host->sys_clk_cg = NULL; > + else > + clk_prepare_enable(host->sys_clk_cg); This doesn't need to be in an else branch, calling clk_prepare_enable on a NULL clock is fine. However, is it expected that this clock is turned on forever after probe?! At the very least, the clock should be disabled in msdc_drv_remove, but, really, I think it should be enabled as needed, like the other clocks, in msdc_gate_clock? > + > + clk = devm_clk_get(&pdev->dev, "pclk_cg"); > + if (IS_ERR(clk)) > + clk = NULL; > + host->bulk_clks[0].clk = clk; > + > + clk = devm_clk_get(&pdev->dev, "axi_cg"); > + if (IS_ERR(clk)) > + clk = NULL; > + host->bulk_clks[1].clk = clk; > + > + clk = devm_clk_get(&pdev->dev, "ahb_cg"); > + if (IS_ERR(clk)) > + clk = NULL; > + host->bulk_clks[2].clk = clk; Use devm_clk_bulk_get_optional for these 3. > + > + return 0; > +} > + > static int msdc_drv_probe(struct platform_device *pdev) > { > struct mmc_host *mmc; > @@ -2405,25 +2462,9 @@ static int msdc_drv_probe(struct platform_device *pdev) > if (ret) > goto host_free; > > - host->src_clk = devm_clk_get(&pdev->dev, "source"); > - if (IS_ERR(host->src_clk)) { > - ret = PTR_ERR(host->src_clk); > - goto host_free; > - } > - > - host->h_clk = devm_clk_get(&pdev->dev, "hclk"); > - if (IS_ERR(host->h_clk)) { > - ret = PTR_ERR(host->h_clk); > + ret = msdc_of_clock_parse(pdev, host); > + if (ret) > goto host_free; > - } > - > - host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk"); > - if (IS_ERR(host->bus_clk)) > - host->bus_clk = NULL; > - /*source clock control gate is optional clock*/ > - host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg"); > - if (IS_ERR(host->src_clk_cg)) > - host->src_clk_cg = NULL; > > host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, > "hrst"); > -- > 2.18.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel