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@xxxxxxxxxxxx> wrote:
> From:   Yong Mao <yong.mao@xxxxxxxxxxxx>
> To:     Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> wrote:
>> >
>> > From: yong mao <yong.mao@xxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxx>
>> > Signed-off-by: Chaotian Jing <chaotian.jing@xxxxxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxxxxxxxxxxx
>> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
>
>