[U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC

Amarendra Reddy amar.lavanuru at gmail.com
Fri Jan 11 14:06:24 CET 2013


Hi Simon / Jaehoon,

Thanks for review comments.
Please find the responses below.

Thanks & Regards
Amarendra Reddy

On 11 January 2013 11:14, Simon Glass <sjg at chromium.org> wrote:

> Hi Jaehoon,
>
> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung at samsung.com>
> wrote:
> > On 01/11/2013 12:33 AM, Simon Glass wrote:
> >> Hi Amar,
> >>
> >> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt at samsung.com> wrote:
> >>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
> >>> from the device tree and initialising DWMMC channels as per data
> >>> obtained from the node.
> >>>
> >>> Changes from V1:
> >>>         1)Updated code to have same signature for the function
> >>>         exynos_dwmci_init() for both FDT and non-FDT versions.
> >>>         2)Updated code to pass device_id parameter to the function
> >>>         exynos5_mmc_set_clk_div() instead of index.
> >>>         3)Updated code to decode the value of "samsung,width" from FDT.
> >>>         4)Channel index is computed instead of getting from FDT.
> >>>
> >>> Changes from V2:
> >>>         1)Updation of commit message and resubmition of proper patch
> set.
> >>>
> >>> Changes from V3:
> >>>         1)Replaced the new function exynos5_mmc_set_clk_div() with the
> >>>         existing function set_mmc_clk(). set_mmc_clk() will do the
> purpose.
> >>>         2)Computation of FSYS block clock divisor (pre-ratio) is added.
> >>>
> >>> Signed-off-by: Vivek Gautam <gautam.vivek at samsung.com>
> >>> Signed-off-by: Amar <amarendra.xt at samsung.com>
> >>> ---
> >>>  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
> >>>  drivers/mmc/exynos_dw_mmc.c              | 129
> +++++++++++++++++++++++++++++--
> >>>  include/dwmmc.h                          |   4 +
> >>>  3 files changed, 130 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> index 8acdf9b..40dcc7b 100644
> >>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> @@ -29,8 +29,12 @@
> >>>
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob);
> >>> +#else
> >>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> >>
> >> Why unsigned?
>
Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int
bus_width)" with
int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
clksel).
 Regarding the parameter *'clksel':*
i) "timing" value shall be passed in case of FDT, to be written into CLKSEL
register.
ii) NULL will be passed in case of non-FDT.


>  >>
> >> I'm really not that keen on functions which change their signature
> >> based on an #ifdef. Can we perhaps have
> >>
> >> int exynos_dwmmc_init(const void *blob);
> >>
> >> which will pass NULL when there is no FDT, and
> >>
> >> int exynos_dwmmc_add_port(int index, int bus_width)
> >>
> >> for use by non-FDT boards?
>

Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and
int exynos_dwmmc_init(const void *blob) for FDT.
And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width,
u32 clksel)".


>  >>
> >>>  {
> >>>         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
> >>>         return exynos_dwmci_init(base, bus_width, index);
> >>>  }
> >>> +#endif
> >>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> >>> index 72a31b7..d7ca7d0 100644
> >>> --- a/drivers/mmc/exynos_dw_mmc.c
> >>> +++ b/drivers/mmc/exynos_dw_mmc.c
> >>> @@ -19,39 +19,154 @@
> >>>   */
> >>>
> >>>  #include <common.h>
> >>> -#include <malloc.h>
> >>>  #include <dwmmc.h>
> >>> +#include <fdtdec.h>
> >>> +#include <libfdt.h>
> >>> +#include <malloc.h>
> >>>  #include <asm/arch/dwmmc.h>
> >>>  #include <asm/arch/clk.h>
> >>> +#include <asm/arch/pinmux.h>
> >>> +
> >>> +#define        DWMMC_MAX_CH_NUM                4
> >>> +#define        DWMMC_MAX_FREQ                  52000000
> >>> +#define        DWMMC_MIN_FREQ                  400000
> >>> +#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
> >>> +#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
> >>> +#define        ONE_MEGA_HZ                     1000000
> >>> +#define        SCALED_VAL_FOUR_HUNDRED         400
> >>
> >> I don't think you need these last two - you can just write the number
> >> in the code
> > Why didn't add into the dwmmc.h?
>

Ok, will just write the number in the code.

>  >>
> >>>
> >>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >>
> >> Same with this I think
> > Sorry..What means? Also need not?
>
> Yes I mean that you probably don't need this - just put the string in the
> code.
>
Ok.

>
> >>
> >>> +u32 timing[3];
> >>>
> >>> +/*
> >>> + * Function used as callback function to initialise the
> >>> + * CLKSEL register for every mmc channel.
> >>> + */
> >>>  static void exynos_dwmci_clksel(struct dwmci_host *host)
> >>>  {
> >>> -       u32 val;
> >>> -       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> >>> -               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |
> DWMCI_SET_DIV_RATIO(0);
> >>> +       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
> >>> +}
> >>>
> >>> -       dwmci_writel(host, DWMCI_CLKSEL, val);
> >>> +unsigned int exynos_dwmci_get_clk(int dev_index)
> >>> +{
> >>> +       return get_mmc_clk(dev_index);
> >>>  }
> >>>
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> >>>  {
> >>>         struct dwmci_host *host = NULL;
> >>> +       unsigned int clock, div;
> >>>         host = malloc(sizeof(struct dwmci_host));
> >>>         if (!host) {
> >>>                 printf("dwmci_host malloc fail!\n");
> >>>                 return 1;
> >>>         }
> >>>
> >>> +       /*
> >>> +        * The max operating freq of FSYS block is 400MHz.
> >>> +        * Scale down the 400MHz to number 400.
> >>> +        * Scale down the MPLL clock by dividing MPLL_CLK with
> ONE_MEGA_HZ.
> >>> +        * Arrive at the divisor value taking 400 as the reference.
> >>> +        */
> >>> +
> >>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
> >>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
> >>> +
> >>> +       /* Arrive at the divisor value. */
> >>> +       for (div = 0; div <= 0xf; div++) {
> >>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
> >>> +                       break;
> >>> +       }
> >>
> >> What if you don't find the right divisor?
> > i want to use like this.
> >
> > sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
> > div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
> > mmc_set_clk(index, div);
> >
> > Then we can set to div value at clock register.
> > It didn't need to add this code...
>
> OK, sounds good.
>
> Ok, shall implement as suggested by Jaehoon.


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


More information about the U-Boot mailing list