[U-Boot] [PATCH V3 5/9] EXYNOS5: DWMMC: API to set mmc clock divisor

[Amarendra Reddy]

Hi Jaehoon

Thanks for the comments.
Please find my response below.

Than

On 2 January 2013 09:01, Jaehoon Chung <jh80.chung at samsung.com> wrote:

> Hi Amar,
>
> On 12/31/2012 07:58 PM, Amar wrote:
> > This API computes the divisor value based on MPLL clock and
> > writes it into the FSYS1 register.
> >
> > Changes from V1:
> >       1)Updated the function exynos5_mmc_set_clk_div() to receive
> >       'device_i'd as input parameter instead of 'index'.
> >
> > Changes from V2:
> >       1)Updation of commit message and resubmition of proper patch set.
> >
> > Signed-off-by: Amar <amarendra.xt at samsung.com>
> > ---
> >  arch/arm/cpu/armv7/exynos/clock.c      | 38
> ++++++++++++++++++++++++++++++++--
> >  arch/arm/include/asm/arch-exynos/clk.h |  4 ++++
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv7/exynos/clock.c
> b/arch/arm/cpu/armv7/exynos/clock.c
> > index 973b84e..cd42689 100644
> > --- a/arch/arm/cpu/armv7/exynos/clock.c
> > +++ b/arch/arm/cpu/armv7/exynos/clock.c
> > @@ -490,7 +490,7 @@ static unsigned long exynos4_get_mmc_clk(int
> dev_index)
> >               (struct exynos4_clock *)samsung_get_base_clock();
> >       unsigned long uclk, sclk;
> >       unsigned int sel, ratio, pre_ratio;
> > -     int shift;
> > +     int shift = 0;
> >
> >       sel = readl(&clk->src_fsys);
> >       sel = (sel >> (dev_index << 2)) & 0xf;
> > @@ -539,7 +539,7 @@ static unsigned long exynos5_get_mmc_clk(int
> dev_index)
> >               (struct exynos5_clock *)samsung_get_base_clock();
> >       unsigned long uclk, sclk;
> >       unsigned int sel, ratio, pre_ratio;
> > -     int shift;
> > +     int shift = 0;
> >
> >       sel = readl(&clk->src_fsys);
> >       sel = (sel >> (dev_index << 2)) & 0xf;
> > @@ -659,6 +659,40 @@ static void exynos5_set_mmc_clk(int dev_index,
> unsigned int div)
> >       writel(val, addr);
> >  }
> >
> > +/* exynos5: set the mmc clock div ratio in fsys1 */
> > +int exynos5_mmc_set_clk_div(int dev_id)
> Why return the int type?
>
Yes, not required to return. I will update.

> > +{
> > +     struct exynos5_clock *clk =
> > +             (struct exynos5_clock *)samsung_get_base_clock();
> > +     unsigned int addr;
> > +     unsigned int clock;
> > +     unsigned int tmp;
> > +     unsigned int i;
> Can't use unsigned int addr, clock....? We can save the line.
>
Ok.

> > +
> > +     /* get mpll clock */
> > +     clock = get_pll_clk(MPLL) / 1000000;
> > +
> > +     /*
> > +      * CLK_DIV_FSYS1
> > +      * MMC0_PRE_RATIO [15:8], MMC0_RATIO [3:0]
> > +      * CLK_DIV_FSYS2
> > +      * MMC2_PRE_RATIO [15:8], MMC2_RATIO [3:0]
> > +      */
> > +     if (dev_id <= PERIPH_ID_SDMMC1)
> > +             addr = (unsigned int)&clk->div_fsys1;
> > +     else
> > +             addr = (unsigned int)&clk->div_fsys2;
> > +
> > +     tmp = readl(addr) & ~FSYS1_MMC0_DIV_MASK;
> FSYS1_MMC0_DIV_MASK? then case of MMC2?
> It is wrong macro name..You also used the clk->div_fsys2..
>
Yes, will change the macro name.

> > +     for (i = 0; i <= 0xf; i++) {
> > +             if ((clock / (i + 1)) <= 400) {
> > +                     writel(tmp | i << 0, addr);
> > +                     break;
> > +             }
> > +     }
> > +     return 0;
> > +}
> I didn't understand this function(exynos5_mmc_set_clk_div?).
> What purpose? I think good that proper to use the "div" as argument.
> void exynos5_mmc_set_clk(int dev_id, unsigned int div).


For fixing dwmmc driver, I referred to working code from
"chromiumos/src/third_party/u-boot".
In chromium uboot, during mhci init, the function "clock_set_mshci()" is
called which is in "arch/arm/cpu/armv7/exynos5/clock.c"

Our function "*exynos5_mmc_set_clk_div()"* is same as "*clock_set_mshci()".*

Now coming to 'div' as argument .... we have the below scenarios / questions
1-> What is the value of 'div' to be passed from calling function ?
2-> The value of 'div' needs to be computed in calling function.
3-> As per my understanding, 'div' value depends on values of MPLL clock
and FSYS1/2.
4-> *Question:* Is it OK to put the piece of code which accesses MPLL,
FSYS1, FSYS2 in drivers/mmc/exynos_dw_mmc.c. ?
5-> If we compute 'div' value in "drivers/mmc/exynos_dw_mmc.c", then
there will be duplication of code (Read of FSYS1/2).

Please comment on the above.

> +
>  /* get_lcd_clk: return lcd clock frequency */
>  static unsigned long exynos4_get_lcd_clk(void)
>  {
> diff --git a/arch/arm/include/asm/arch-exynos/clk.h
b/arch/arm/include/asm/arch-exynos/clk.h
> index 1935b0b..2fd7c3e 100644
> --- a/arch/arm/include/asm/arch-exynos/clk.h
> +++ b/arch/arm/include/asm/arch-exynos/clk.h
> @@ -29,6 +29,9 @@
>  #define VPLL 4
>  #define BPLL 5
>
> +#define FSYS1_MMC0_DIV_MASK  0xff0f
> +#define FSYS1_MMC0_DIV_VAL   0x0701
> +
>  unsigned long get_pll_clk(int pllreg);
>  unsigned long get_arm_clk(void);
>  unsigned long get_i2c_clk(void);
> @@ -36,6 +39,7 @@ unsigned long get_pwm_clk(void);
>  unsigned long get_uart_clk(int dev_index);
>  unsigned long get_mmc_clk(int dev_index);
>  void set_mmc_clk(int dev_index, unsigned int div);
> +int exynos5_mmc_set_clk_div(int dev_index);
>  unsigned long get_lcd_clk(void);
>  void set_lcd_clk(void);
>  void set_mipi_clk(void);
>

 _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>