From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752794AbeDECJE (ORCPT ); Wed, 4 Apr 2018 22:09:04 -0400 Received: from mail-pl0-f66.google.com ([209.85.160.66]:43174 "EHLO mail-pl0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752688AbeDECJC (ORCPT ); Wed, 4 Apr 2018 22:09:02 -0400 X-Google-Smtp-Source: AIpwx48hvbX7QF6ON5bAfNZVxNAfamuNeK2F/d8Qqonn0VT5DXfmsvmxcqx3Q6aaRXDmbSM9J2oQuA== Subject: Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor To: Shawn Lin , Ryan Grachek Cc: Jaehoon Chung , Ulf Hansson , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, liwei213@huawei.com, Suzhuangluan References: <20180329182423.21201-1-ryan@edited.us> From: zhangfei Message-ID: <00fe2c61-1cb8-d548-2057-9e70ec18df67@linaro.org> Date: Thu, 5 Apr 2018 10:08:57 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org + Hisilicon colleague On 2018年04月05日 08:51, Shawn Lin wrote: > [+ Zhangfei Gao who added support for hi6220] > > On 2018/4/4 23:31, Ryan Grachek wrote: >> On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin > > wrote: >> >>     On 2018/3/30 2:24, oscardagrach wrote: >> >>     Need at least one line commit body. >> >>         Signed-off-by: oscardagrach > > >>         --- >>            drivers/mmc/host/dw_mmc-k3.c | 10 ++++++++-- >>            1 file changed, 8 insertions(+), 2 deletions(-) >> >>         diff --git a/drivers/mmc/host/dw_mmc-k3.c >>         b/drivers/mmc/host/dw_mmc-k3.c >>         index 89cdb3d533bb..efc546cb4db8 100644 >>         --- a/drivers/mmc/host/dw_mmc-k3.c >>         +++ b/drivers/mmc/host/dw_mmc-k3.c >>         @@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct >>         dw_mci *host, struct mmc_ios *ios) >>                  int ret; >>                  unsigned int clock; >>            -     clock = (ios->clock <= 25000000) ? 25000000 : >> ios->clock; >>         - >>         +       /* CLKDIV must be 1 for DDR52/8-bit mode */ >>         +       if (ios->bus_width == MMC_BUS_WIDTH_8 && >>         +               ios->timing == MMC_TIMING_MMC_DDR52) { >>         +               mci_writel(host, CLKDIV, 0x1); >>         +               clock = ios->clock; >>         +       } else { >>         +               clock = (ios->clock <= 25000000) ? 25000000 : >>         ios->clock; >>         +       } >> >> >>     I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the >> following >>     change is more sensible? >> >>     if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing == >>     MMC_TIMING_MMC_DDR52) >>              clock = ios->clock * 2; >>     else >>              clock = (ios->clock <= 25000000) ? 25000000 : ios->clock; >> >> >>     The reason is ios->clock is 52MHz and you could claim 104MHz from >> the >>     clock provider and let dw_mmc core take care of the divder to be 1. >>     Otherwise, you just force it to be DDR52/8-bit with a clk rate of >> 26MHz. >> >> >>                  ret = clk_set_rate(host->biu_clk, clock); >>                  if (ret) >>                          dev_warn(host->dev, "failed to set rate >>         %uHz\n", clock); >> >> >> > > For future wise, please use plain mode mail, but not HTML format. > >> Your feedback is correct. I see the Rockchip dwmmc driver has a similar >> implementation. After applying your suggested changes, however, my board >> reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 104000000Hz" >> during intialization of eMMC. In addition, I do not see CLKDIV being >> set to 1. clk_set_rate fails and I wonder if this is out-of-scope of >> the driver. >> >> If I set CLKDIV where I did prior, with your changes, the device fails >> to set the clock and falls back to 52 MHz (26 MHz) and works fine, but >> again, CLKDIV is reported as 0 (even though it is 1.) One thing of >> interest to note is when I manually set the clock by doing: >> (echo 104000000 > /sys/kernel/debug/mmc0/clock) the device reports back >> 'mmc_host mmc0: Bus speed (slot 0) = 198400000Hz (slot req 104000000Hz, >>   actual 99200000HZ div = 1)' which works reliably and clk_set_rate does >> not report any error. >> > > When looking closely into the code, at least dw_mci_hi6220_set_ios > goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk. > "b" stands for bus, and "c" stands for card IMHO, however bus_hz > describs the clock to the card, provided by controller. Does the > following patch help? > > > diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c > index 89cdb3d..9e78cf2 100644 > --- a/drivers/mmc/host/dw_mmc-k3.c > +++ b/drivers/mmc/host/dw_mmc-k3.c > @@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci > *host, struct mmc_ios *ios) >         int ret; >         unsigned int clock; > > -       clock = (ios->clock <= 25000000) ? 25000000 : ios->clock; > +       if (ios->bus_width == MMC_BUS_WIDTH_8 && > +           ios->timing == MMC_TIMING_MMC_DDR52) > +               clock = ios->clock * 2; > +       else > +               clock = (ios->clock <= 25000000) ? 25000000 : ios->clock; > > -       ret = clk_set_rate(host->biu_clk, clock); > +       ret = clk_set_rate(host->ciu_clk, clock); >         if (ret) >                 dev_warn(host->dev, "failed to set rate %uHz\n", clock); > > -       host->bus_hz = clk_get_rate(host->biu_clk); > +       clock = clk_get_rate(host->ciu_clk); > +       if (clock != host->bus_hz) { > +               host->bus_hz = clock; > +               host->current_speed = 0; > +       } >  } > > >> I am not sure where to begin debugging these clock issues and welcome >> any feedback. >