Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751941AbdCDG5E (ORCPT ); Sat, 4 Mar 2017 01:57:04 -0500 Received: from mailgw02.mediatek.com ([218.249.47.111]:57567 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1750853AbdCDG5C (ORCPT ); Sat, 4 Mar 2017 01:57:02 -0500 Message-ID: <1488609882.29409.1.camel@mhfsdcap03> Subject: Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong From: Yong Mao To: Daniel Kurtz CC: Ulf Hansson , srv_heupstream , Linus Walleij , , "linux-kernel@vger.kernel.org" , "moderated list:ARM/Mediatek SoC support" , "linux-arm-kernel@lists.infradead.org" , Chunfeng Yun , Eddie Huang , Chaotian Jing Date: Sat, 4 Mar 2017 14:44:42 +0800 In-Reply-To: References: <1487924527-22584-1-git-send-email-yong.mao@mediatek.com> <1487924527-22584-2-git-send-email-yong.mao@mediatek.com> <1487929124.22858.10.camel@mhfsdcap03> <1488362160.27826.27.camel@mhfsdcap03> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6137 Lines: 157 On Thu, 2017-03-02 at 20:20 +0800, Daniel Kurtz wrote: > On Wed, Mar 1, 2017 at 5:56 PM, Yong Mao wrote: > > On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote: > >> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao wrote: > >> > From: Yong Mao > >> > To: Daniel Kurtz > >> > Subject: Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency > >> > could be set wrong > >> > Date: Fri, 24 Feb 2017 17:33:37 +0800 > >> > > >> > > >> > On Fri, 2017-02-24 at 17:52 +0900, Daniel Kurtz wrote: > >> >> On Fri, Feb 24, 2017 at 5:22 PM, Yong Mao > >> > wrote: > >> >> > > >> >> > From: yong mao > >> >> > > >> >> > This patch can fix two issues: > >> >> > > >> >> > Issue 1: > >> >> > The maximum value of clock divider is 0xff. > >> >> > Because the type of div is u32, div may be larger than max_div. > >> >> > In this case, we should use max_div to set the clock frequency. > >> >> > > >> >> > Issue 2: > >> >> > In previous code, we can not set the correct clock frequency when > >> >> > div equals 0xff. > >> >> > > >> >> > Signed-off-by: Yong Mao > >> >> > Signed-off-by: Chaotian Jing > >> >> > --- > >> >> > drivers/mmc/host/mtk-sd.c | 13 ++++++++++++- > >> >> > 1 file changed, 12 insertions(+), 1 deletion(-) > >> >> > > >> >> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > >> >> > index 07f3236..3174445 100644 > >> >> > --- a/drivers/mmc/host/mtk-sd.c > >> >> > +++ b/drivers/mmc/host/mtk-sd.c > >> >> > @@ -540,6 +540,7 @@ static void msdc_set_mclk(struct msdc_host > >> > *host, unsigned char timing, u32 hz) > >> >> > u32 mode; > >> >> > u32 flags; > >> >> > u32 div; > >> >> > + u32 max_div; > >> >> > >> >> There's really no need for this variable. Just use 0xff below. > >> > For all of our IC, max_div is not a constant. > >> > We will upstream another patch which max_div will get the different > >> > value depending on the IC. > >> > Therefore, we keep the max_div as a variable here. > >> > >> Please add the variable in the patch that uses it as a variable. > >> > >> > > >> >> > >> >> > u32 sclk; > >> >> > > >> >> > if (!hz) { > >> >> > @@ -590,8 +591,18 @@ static void msdc_set_mclk(struct msdc_host > >> > *host, unsigned char timing, u32 hz) > >> >> > sclk = (host->src_clk_freq >> 2) / div; > >> >> > } > >> >> > } > >> >> > + > >> >> > + /** > >> >> > + * The maximum value of div is 0xff. > >> >> > + * Check if the div is larger than max_div. > >> >> > + */ > >> >> > + max_div = 0xff; > >> >> > + if (div > max_div) { > >> >> > + div = max_div; > >> >> > + sclk = (host->src_clk_freq >> 2) / div; > >> >> > + } > >> >> > sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD | > >> > MSDC_CFG_CKDIV, > >> >> > - (mode << 8) | (div % 0xff)); > >> >> > + (mode << 8) | div); > >> >> > >> >> Hmm, I don't know much about this sub-system, but should we even be > >> >> allowing requests to set a frequency that we can't actually achieve > >> >> with the divider? > >> >> > >> > > >> > No. We can not get a frequency that we can't actually achieve with the > >> > divider. This patch is to solve this kind of issue. > >> > >> Sorry, I am trying to understand why we need this patch. > >> > >> AFAICT, it looks like sometimes msdc_set_mclk() is being called with > >> hz that cannot be generated by your hardware. In particular, > >> sometimes the original code computes "div > 255". > >> To work around this problem, this patch just caps the divider value to > >> 255, which is the maximum divider provided by the hardware. However, > >> presumably this means that in this case we won't actually be > >> generating the requested hz value. > >> > >> So, can you please explain in what exact scenario this patch is > >> required, and justify why it is ok to generate a clock other than the > >> requested in this case? > >> > >> -Dan > >> > > > > This issue is hidden deeply. It is a boundary related issue. > > Let me take the real value to explain how this issue occurs. > > > > 1. mmc->f_min = host->src_clk_freq / (4 * 255); > > mmc->f_min = 400000000 / (4 * 255) = 392156; > > 2. mmc core tries to initialize emmc by using 400000hz. > > If the first try failed, it will retry by using max(300000hz, > > mmc->f_min)= 392156hz > > 3. msdc_set_mclk will be invoked by mmc core to set the clock. > > and then following code will be executed. > > div = (host->src_clk_freq + ((hz << 2) - 1)) / (hz << 2); > > (div = (400000000 + (392156 << 2) - 1)) / (392156 << 2) = 256) > > > > Why do we use (host->src_clk_freq + ((hz << 2) - 1))? > > ==> Because if we can not get a proper clock frequency, we should set a > > clock frequency which is less than proper clock frequency. > > > > 4. In this IC, it only use 8 bits to indicate the value of clock > > divider. Therefore, 256 is overflow, it is equal 0 here. > > And then we will get a wrong 100000000Hz clock frequency. > > > > Can you understand how this issue occurs now? > > Thanks for the excellent explanation. Things are a lot clearer now. > > I think this can be solved even easier by computing f_min the same way: > > mmc->f_min = DIV_ROUND_UP(400000000, 4 * 255); /* = 392157 */ > That is really a easier way to solve this issue. I will submit a new version as soon as possible. Thanks. > > > > >> > > >> > > >> >> > sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_CKPDN); > >> >> > while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB)) > >> >> > cpu_relax(); > >> >> > -- > >> >> > 1.7.9.5 > >> >> > > >> >> > > >> >> > _______________________________________________ > >> >> > Linux-mediatek mailing list > >> >> > Linux-mediatek@lists.infradead.org > >> >> > http://lists.infradead.org/mailman/listinfo/linux-mediatek > >> > > >> > > >> > > > > >