[PATCH v1] Fixed bug where clock frequency could be set wrong

[PATCH v1] Fixed bug where clock frequency could be set wrong

[Yong Mao]

yong mao (1):
  mmc: mediatek: Fixed bug where clock frequency could be set wrong

 drivers/mmc/host/mtk-sd.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
1.8.1.1.dirty

[PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

From: yong mao <yong.mao@mediatek.com>

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 <yong.mao@mediatek.com>
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)
 	u32 mode;
 	u32 flags;
 	u32 div;
+	u32 max_div;
 	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);
 	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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Daniel Kurtz]

On Fri, Feb 24, 2017 at 5:22 PM, Yong Mao <yong.mao@mediatek.com> wrote:
>
> From: yong mao <yong.mao@mediatek.com>
>
> 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 <yong.mao@mediatek.com>
> 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)
>         u32 mode;
>         u32 flags;
>         u32 div;
> +       u32 max_div;

There's really no need for this variable.  Just use 0xff below.

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

>         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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

From: 	Yong Mao <yong.mao@mediatek.com>
To: 	Daniel Kurtz <djkurtz@chromium.org>
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 <yong.mao@mediatek.com>
wrote:
> >
> > From: yong mao <yong.mao@mediatek.com>
> >
> > 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 <yong.mao@mediatek.com>
> > 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)
> >         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.

> 
> >         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.


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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Daniel Kurtz]

On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao <yong.mao@mediatek.com> wrote:
> From:   Yong Mao <yong.mao@mediatek.com>
> To:     Daniel Kurtz <djkurtz@chromium.org>
> 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 <yong.mao@mediatek.com>
> wrote:
>> >
>> > From: yong mao <yong.mao@mediatek.com>
>> >
>> > 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 <yong.mao@mediatek.com>
>> > 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)
>> >         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

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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:
> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao <yong.mao@mediatek.com> wrote:
> > From:   Yong Mao <yong.mao@mediatek.com>
> > To:     Daniel Kurtz <djkurtz@chromium.org>
> > 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 <yong.mao@mediatek.com>
> > wrote:
> >> >
> >> > From: yong mao <yong.mao@mediatek.com>
> >> >
> >> > 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 <yong.mao@mediatek.com>
> >> > 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)
> >> >         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?

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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Daniel Kurtz]

On Wed, Mar 1, 2017 at 5:56 PM, Yong Mao <yong.mao@mediatek.com> wrote:
> On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:
>> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao <yong.mao@mediatek.com> wrote:
>> > From:   Yong Mao <yong.mao@mediatek.com>
>> > To:     Daniel Kurtz <djkurtz@chromium.org>
>> > 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 <yong.mao@mediatek.com>
>> > wrote:
>> >> >
>> >> > From: yong mao <yong.mao@mediatek.com>
>> >> >
>> >> > 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 <yong.mao@mediatek.com>
>> >> > 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)
>> >> >         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 */


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

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

On Thu, 2017-03-02 at 20:20 +0800, Daniel Kurtz wrote:
> On Wed, Mar 1, 2017 at 5:56 PM, Yong Mao <yong.mao@mediatek.com> wrote:
> > On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:
> >> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao <yong.mao@mediatek.com> wrote:
> >> > From:   Yong Mao <yong.mao@mediatek.com>
> >> > To:     Daniel Kurtz <djkurtz@chromium.org>
> >> > 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 <yong.mao@mediatek.com>
> >> > wrote:
> >> >> >
> >> >> > From: yong mao <yong.mao@mediatek.com>
> >> >> >
> >> >> > 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 <yong.mao@mediatek.com>
> >> >> > 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)
> >> >> >         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
> >> >
> >> >
> >> >
> >
> >