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

[Jaehoon Chung <jh80.chung@samsung.com>]

On 01/11/2013 12:33 AM, Simon Glass wrote:
> Hi Amar,
> 
> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@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@samsung.com>
>> Signed-off-by: Amar <amarendra.xt@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?
> 
> 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?
> 
>>  {
>>         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?
> 
>>
>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> 
> Same with this I think
Sorry..What means? Also need not?
> 
>> +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...
>> +
>> +       /* set the clock divisor for mmc */
>> +       set_mmc_clk(index, div);
>> +
>>         host->name = EXYNOS_NAME;
>>         host->ioaddr = (void *)regbase;
>>         host->buswidth = bus_width;
>> +#ifdef CONFIG_OF_CONTROL
>> +       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
>> +                               DWMCI_SET_DRV_CLK(timing[1]) |
>> +                               DWMCI_SET_DIV_RATIO(timing[2]));
> 
> timing should be a parameter, not a global. For the non-FDT case
> perhaps you can accept NULL, meaning default? Then:
> 
> if (timing)
>    do the code above
> else
>    do the code below
> 
>> +#else
>> +       if (0 == index)
>> +               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
>> +       if (2 == index)
>> +               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
>> +#endif
>>         host->clksel = exynos_dwmci_clksel;
>>         host->dev_index = index;
>> -
>> -       add_dwmci(host, 52000000, 400000);
>> +       host->mmc_clk = exynos_dwmci_get_clk;
>> +       /* Add the mmc chennel to be registered with mmc core */
>> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
> 
> Is error checking needed here?
> 
>>
>>         return 0;
>>  }
>>
>> +#ifdef CONFIG_OF_CONTROL
>> +unsigned int exynos_dwmmc_init(const void *blob)
>> +{
>> +       u32 base;
>> +       int index, bus_width;
>> +       int node_list[DWMMC_MAX_CH_NUM];
>> +       int err = 0;
>> +       int dev_id, flag;
>> +       int count, i;
>> +
>> +       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
>> +                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
>> +                               DWMMC_MAX_CH_NUM);
>> +
>> +       for (i = 0; i < count; i++) {
>> +               int node = node_list[i];
>> +
>> +               if (node <= 0)
>> +                       continue;
>> +
>> +               /* Extract device id for each mmc channel */
>> +               dev_id = pinmux_decode_periph_id(blob, node);
>> +
>> +               /* Get the bus width from the device node */
>> +               bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
>> +               if (bus_width < 0) {
> 
> <= 0? The function will return 0 if there is no property.
> 
>> +                       debug("DWMMC: Can't get bus-width\n");
>> +                       return -1;
>> +               }
>> +               if (8 == bus_width)
>> +                       flag = PINMUX_FLAG_8BIT_MODE;
>> +               else
>> +                       flag = PINMUX_FLAG_NONE;
>> +
>> +               /* config pinmux for each mmc channel */
>> +               err = exynos_pinmux_config(dev_id, flag);
>> +               if (err) {
>> +                       debug("DWMMC not configured\n");
>> +                       return err;
>> +               }
>> +
>> +               index = dev_id - PERIPH_ID_SDMMC0;
>> +
>> +               /* Get the base address from the device node */
>> +               base = fdtdec_get_addr(blob, node, "reg");
>> +               if (!base) {
>> +                       debug("DWMMC: Can't get base address\n");
>> +                       return -1;
>> +               }
>> +               /* Extract the timing info from the node */
>> +               err = fdtdec_get_int_array(blob, node, "samsung,timing",
>> +                                       timing, 3);
>> +               if (err) {
>> +                       debug("Can't get sdr-timings for divider\n");
>> +                       return -1;
>> +               }
>> +               /* Initialise each mmc channel */
>> +               err =  exynos_dwmci_init(base, bus_width, index);
>> +               if (err) {
>> +                       debug("Can't do dwmci init\n");
>> +                       return -1;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +#endif
>> diff --git a/include/dwmmc.h b/include/dwmmc.h
>> index c8b1d40..4a42849 100644
>> --- a/include/dwmmc.h
>> +++ b/include/dwmmc.h
>> @@ -123,6 +123,9 @@
>>  #define MSIZE(x)               ((x) << 28)
>>  #define RX_WMARK(x)            ((x) << 16)
>>  #define TX_WMARK(x)            (x)
>> +#define RX_WMARK_SHIFT         16
>> +#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
>> +
> 
> Remove this extra line?
> 
>>
>>  #define DWMCI_IDMAC_OWN                (1 << 31)
>>  #define DWMCI_IDMAC_CH         (1 << 4)
>> @@ -144,6 +147,7 @@ struct dwmci_host {
>>         unsigned int bus_hz;
>>         int dev_index;
>>         int buswidth;
>> +       u32 clksel_val;
>>         u32 fifoth_val;
>>         struct mmc *mmc;
>>
>> --
>> 1.8.0
>>
> Regards,
> Simon
>