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

Amarendra Reddy amar.lavanuru at gmail.com
Tue Jan 22 07:41:52 CET 2013


Hi Joonyoung,
Thanks for the comments.
Please find my response below.

Thanks & Regards
Amarendra

On 22 January 2013 10:53, Joonyoung Shim <jy0922.shim at samsung.com> wrote:

>  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 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)".
>>
>>
> But patch v5 doesn't use exynos_dwmmc_init(NULL) and it uses
> exynos_dwmci_add_port directly in board file.
>
> The initial thought was to use
a) exynos_dwmmc_init(const void *blob) for FDT  and
b) exynos_dwmmc_init(NULL) for non-FDT

But there were review comments from Simon, stating that
"exynos_dwmmc_add_port() should go in the board file, since without an FDT
the driver can't know what ports to init".
Only the board file knows the port numbers.
Hence exynos_dwmmc_init(NULL) is not used in non-FDT case.

Please find below comments from Simon, regarding the same
***********************************************************************
>Note that in the absence of an FDT it is supposed to be the board file
>which knows which MMC ports are active.
> {
>      #ifdef CONFIG_OF_CONTROL
>
>             /* Read data from FDT */
>
>             exynos_dwmmc_add_port(index, bus_width, ...)

>This code should go in the mmc driver. One of the ideas behind FDT is
>that the drivers can figure out by themselves what ports to set up.
>Also only the driver knows about its particular fields.
>
>      #else
>
>             exynos_dwmmc_add_port(0,8...)
>
>             exynos_dwmmc_add_port(2,4...)

>This code should go in the board file, since without an FDT the driver
>can't know what ports to init.
**********************************************************************


> Why we need blob argument in exynos_dwmmc_init? Already spi_init() just
> uses gd->fdt_blob directly of drivers/spi/exynos_spi.c
>
Yes, in case of spi_init(), there is no explicit mention (hard code) of
port numbers, bus_width etc.
But for dwmmc, in case of non-FDT, we need to hard code port number and
bus_width and this is done in board file.


> 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.
>
> Yes, this is a good approach.
Also in near future the non-FDT part may be removed, retaining only the FDT
part.
Please comment whether to add this new approach by using 'struct
exynos_dwmmc_config', I can add in next patch.


> 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<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<http://lists.denx.de/mailman/listinfo/u-boot>
>>
>
>


More information about the U-Boot mailing list