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

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 01/11/2013 10:06 PM, Amarendra Reddy wrote:
> 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@chromium.org> wrote:
>
>> Hi Jaehoon,
>>
>> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung@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@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?
> 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)".
>

But patch v5 doesn't use exynos_dwmmc_init(NULL) and it uses 
exynos_dwmci_add_port directly in board file.

Why we need blob argument in exynos_dwmmc_init? Already spi_init() just 
uses gd->fdt_blob directly of drivers/spi/exynos_spi.c
I think exynos_dwmmc_init function needs a struct argument for int index 
and int bus_width such follows.

struct exynos_dwmmc_config {
     int index;
     int bus_width;
};

exynos_dwmmc_init(struct exynos_dwmmc_config *config)
{
...
}

If config is NULL and gd->fdt_blob isn't NULL, we can get index and 
bus_width from blob.

Thanks.


>>   >>
>>>>>   {
>>>>>          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
>>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot