From patchwork Wed Nov 16 04:42:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ritesh Harjani X-Patchwork-Id: 9430885 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E88C360469 for ; Wed, 16 Nov 2016 04:42:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DAF7828E0D for ; Wed, 16 Nov 2016 04:42:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CE2AF28E15; Wed, 16 Nov 2016 04:42:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4B81F28E1C for ; Wed, 16 Nov 2016 04:42:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932760AbcKPEmx (ORCPT ); Tue, 15 Nov 2016 23:42:53 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:53720 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932297AbcKPEmv (ORCPT ); Tue, 15 Nov 2016 23:42:51 -0500 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id A962E614C4; Wed, 16 Nov 2016 04:42:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1479271370; bh=99qHs10H0eYyylyoqmYXVV+u+ZAE9g6dxrWW/v/B27g=; h=Subject:To:References:Cc:From:Date:In-Reply-To:From; b=nGpjg25tJ6/617daYUQkP1n2MpiJ2DE5PxRLnUe8wHEIECuzpBTcksbiC1GwUibAu ThS3SmUp1Qo9jVmCcjO76fg2ynEg/zAtuK2YqxxvU/Ntg+36AoFalYyLyDgjYFTSPp dA2PcD0pf1/ReTo36SXtfUO5XjTVnhN7NiRk/09Q= Received: from [10.44.21.36] (unknown [202.46.23.62]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: riteshh@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 8F19A6158F; Wed, 16 Nov 2016 04:42:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1479271369; bh=99qHs10H0eYyylyoqmYXVV+u+ZAE9g6dxrWW/v/B27g=; h=Subject:To:References:Cc:From:Date:In-Reply-To:From; b=mDLbPbKU1Rrjv2lzMod06u8wFtjVGcQWzhWDjgYtrAfJnR6PWj6bn53OWfPGasS5c 5EV1Xg/xDdUfowMWG8qBqkOChTQD9Ba++71ooVqOIP/u46DWYyKCP8+4rh/F+s/5iA HR6PdWsRfbdTVWSsrjx7J9oO+s2sMSWOOPrWFds0= DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 8F19A6158F Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=riteshh@codeaurora.org Subject: Re: [PATCH v7 08/14] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm To: Stephen Boyd , adrian.hunter@intel.com, ulf.hansson@linaro.org References: <1479103248-9491-1-git-send-email-riteshh@codeaurora.org> <1479103248-9491-9-git-send-email-riteshh@codeaurora.org> <20161114193004.GI5177@codeaurora.org> <3c1a7c72-0ac1-8ed0-87fc-238331f0645b@codeaurora.org> Cc: linux-mmc@vger.kernel.org, shawn.lin@rock-chips.com, andy.gross@linaro.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, david.brown@linaro.org, linux-arm-msm@vger.kernel.org, georgi.djakov@linaro.org, alex.lemberg@sandisk.com, mateusz.nowak@intel.com, Yuliy.Izrailov@sandisk.com, asutoshd@codeaurora.org, kdorfman@codeaurora.org, david.griego@linaro.org, stummala@codeaurora.org, venkatg@codeaurora.org, rnayak@codeaurora.org, pramod.gurav@linaro.org From: Ritesh Harjani Message-ID: Date: Wed, 16 Nov 2016 10:12:40 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <3c1a7c72-0ac1-8ed0-87fc-238331f0645b@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On 11/15/2016 10:40 AM, Ritesh Harjani wrote: > Hi Stephen/Adrian, > > On 11/15/2016 1:07 AM, Stephen Boyd wrote: >> On 11/14, Ritesh Harjani wrote: >>> @@ -577,6 +578,90 @@ static unsigned int >>> sdhci_msm_get_min_clock(struct sdhci_host *host) >>> return SDHCI_MSM_MIN_CLOCK; >>> } >>> >>> +/** >>> + * __sdhci_msm_set_clock - sdhci_msm clock control. >>> + * >>> + * Description: >>> + * Implement MSM version of sdhci_set_clock. >>> + * This is required since MSM controller does not >>> + * use internal divider and instead directly control >>> + * the GCC clock as per HW recommendation. >>> + **/ >>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) >>> +{ >>> + u16 clk; >>> + unsigned long timeout; >>> + >>> + /* >>> + * Keep actual_clock as zero - >>> + * - since there is no divider used so no need of having >>> actual_clock. >>> + * - MSM controller uses SDCLK for data timeout calculation. If >>> + * actual_clock is zero, host->clock is taken for calculation. >>> + */ >>> + host->mmc->actual_clock = 0; >>> + >>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>> + >>> + if (clock == 0) >>> + return; >>> + >>> + /* >>> + * MSM controller do not use clock divider. >>> + * Thus read SDHCI_CLOCK_CONTROL and only enable >>> + * clock with no divider value programmed. >>> + */ >>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >>> + >>> + clk |= SDHCI_CLOCK_INT_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>> + >>> + /* Wait max 20 ms */ >>> + timeout = 20; >>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>> + & SDHCI_CLOCK_INT_STABLE)) { >>> + if (timeout == 0) { >>> + pr_err("%s: Internal clock never stabilised\n", >>> + mmc_hostname(host->mmc)); >>> + return; >>> + } >>> + timeout--; >>> + mdelay(1); >>> + } >>> + >>> + clk |= SDHCI_CLOCK_CARD_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >> >> This is almost a copy/paste of sdhci_set_clock(). Can we make >> sdhci_set_clock() call a __sdhci_set_clock() function that takes >> unsigned int clock, and also a flag indicating if we want to set >> the internal clock divider or not? Then we can call >> __sdhci_set_clock() from sdhci_set_clock() with (clock, true) as >> arguments and (clock, false). Actually what you may be referring here is some sort of quirks which is not entertained any more for sdhci driver. sdhci is tending towards becoming a library and hence such changes are restricted. But I think we may do below changes to avoid duplication of code which enables the sdhci internal clock and waits for internal clock to be stable. Adrian, could you please tell if this should be ok? Then we may be able to call for sdhci_set_clock_enable function from sdhci_msm_set_clock. void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > Adrian, > Could you please comment here ? > >> >>> +} >>> + >>> +/* sdhci_msm_set_clock - Called with (host->lock) spinlock held. */ >>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned >>> int clock) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>> + int rc; >>> + >>> + if (!clock) { >>> + msm_host->clk_rate = clock; >>> + goto out; >>> + } >>> + >>> + spin_unlock_irq(&host->lock); >>> + if (clock != msm_host->clk_rate) { >> >> Why do we need to check here? Can't we call clk_set_rate() >> Unconditionally? > Since it may so happen that above layers may call for ->set_clock > function with same requested clock more than once, hence we cache the > host->clock here. > Also, since requested clock (host->clock) can be say 400Mhz but the > actual pltfm supported clock would be say 384MHz. > > >> >>> + rc = clk_set_rate(msm_host->clk, clock); >>> + if (rc) { >>> + pr_err("%s: Failed to set clock at rate %u\n", >>> + mmc_hostname(host->mmc), clock); >>> + spin_lock_irq(&host->lock); >>> + goto out; >> >> Or replace the above two lines with goto err; > Ok, I will have another label out_lock instead of err. >> >>> + } >>> + msm_host->clk_rate = clock; >>> + pr_debug("%s: Setting clock at rate %lu\n", >>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk)); >>> + } >> >> And put an err label here. > will put the label here as out_lock; >> >>> + spin_lock_irq(&host->lock); >>> +out: >>> + __sdhci_msm_set_clock(host, clock); >>> +} >>> + >>> static const struct of_device_id sdhci_msm_dt_match[] = { >>> { .compatible = "qcom,sdhci-msm-v4" }, >>> {}, >> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 42ef3eb..28e605c 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1343,19 +1343,8 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, } EXPORT_SYMBOL_GPL(sdhci_calc_clk); -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) +void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk) { - u16 clk; - unsigned long timeout; - - host->mmc->actual_clock = 0; - - sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); - - if (clock == 0) - return; - - clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock); clk |= SDHCI_CLOCK_INT_EN; sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); @@ -1377,6 +1366,24 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) clk |= SDHCI_CLOCK_CARD_EN; sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); } +EXPORT_SYMBOL_GPL(sdhci_set_clock_enable); + +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) +{ + u16 clk; + unsigned long timeout; + + host->mmc->actual_clock = 0; + + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); + + if (clock == 0) + return; + + clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock); + + sdhci_set_clock_enable(host, clk); +} EXPORT_SYMBOL_GPL(sdhci_set_clock); static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 766df17..43382e1 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -681,6 +681,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host) u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, unsigned int *actual_clock); void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); +void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk); void sdhci_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd);