On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:

On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao [off-list ref] wrote:

quoted

From:   Yong Mao <redacted>
To:     Daniel Kurtz <redacted>
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:

quoted

On Fri, Feb 24, 2017 at 5:22 PM, Yong Mao [off-list ref]

wrote:

quoted

quoted

From: yong mao <redacted>

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 <redacted>
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
 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)

quoted

quoted

        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.

quoted

quoted

quoted

        u32 sclk;

        if (!hz) {

@@ -590,8 +591,18 @@ static void msdc_set_mclk(struct msdc_host

*host, unsigned char timing, u32 hz)

quoted

quoted

                        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,

quoted

quoted

-                       (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?

quoted

quoted

quoted

        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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek