Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
Hi Joonyoung,
Thanks for the comments.
Please find my response below.
Thanks & Regards
Amarendra
On 22 January 2013 10:53, Joonyoung Shim 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 wrote:
>>
>> Hi Jaehoon,
>>>
>>> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung
>>> wrote:
>>>
On 01/11/2013 12:33 AM, Simon Glass wrote:
> Hi Amar,
>
> On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
>> Signed-off-by: Amar
>> ---
>> 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 w
Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
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 wrote:
Hi Jaehoon,
On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung
wrote:
On 01/11/2013 12:33 AM, Simon Glass wrote:
Hi Amar,
On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
Signed-off-by: Amar
---
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() + (0x1 * 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
-#include
#include
+#include
+#include
+#include
#include
#include
+#include
+
+#defineDWMMC_MAX_CH_NUM4
+#defineDWMMC_MAX_FREQ 5200
+#defineDWMMC_MIN_FREQ 40
+#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
+#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
+#defineONE_MEGA_HZ 100
+#defineSCALED_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_R
Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
Hi Simon / Jaehoon,
Thanks for review comments.
Please find the responses below.
Thanks & Regards
Amarendra Reddy
On 11 January 2013 11:14, Simon Glass wrote:
> Hi Jaehoon,
>
> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung
> wrote:
> > On 01/11/2013 12:33 AM, Simon Glass wrote:
> >> Hi Amar,
> >>
> >> On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
> >>> Signed-off-by: Amar
> >>> ---
> >>> 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() + (0x1 * 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
> >>> -#include
> >>> #include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> #include
> >>> #include
> >>> +#include
> >>> +
> >>> +#defineDWMMC_MAX_CH_NUM4
> >>> +#defineDWMMC_MAX_FREQ 5200
> >>> +#defineDWMMC_MIN_FREQ 40
> >>> +#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
> >>> +#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
> >>> +#defineONE_MEGA_HZ 100
> >>> +#defineSCALED_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
Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
Hi Jaehoon,
On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung wrote:
> On 01/11/2013 12:33 AM, Simon Glass wrote:
>> Hi Amar,
>>
>> On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
>>> Signed-off-by: Amar
>>> ---
>>> 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() + (0x1 * 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
>>> -#include
>>> #include
>>> +#include
>>> +#include
>>> +#include
>>> #include
>>> #include
>>> +#include
>>> +
>>> +#defineDWMMC_MAX_CH_NUM4
>>> +#defineDWMMC_MAX_FREQ 5200
>>> +#defineDWMMC_MIN_FREQ 40
>>> +#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
>>> +#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
>>> +#defineONE_MEGA_HZ 100
>>> +#defineSCALED_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?
Yes I mean that you probably don't need this - just put the string in the code.
>>
>>> +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 val
Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
On 01/11/2013 12:33 AM, Simon Glass wrote:
> Hi Amar,
>
> On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
>> Signed-off-by: Amar
>> ---
>> 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() + (0x1 * 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
>> -#include
>> #include
>> +#include
>> +#include
>> +#include
>> #include
>> #include
>> +#include
>> +
>> +#defineDWMMC_MAX_CH_NUM4
>> +#defineDWMMC_MAX_FREQ 5200
>> +#defineDWMMC_MIN_FREQ 40
>> +#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
>> +#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
>> +#defineONE_MEGA_HZ 100
>> +#defineSCALED_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 g
Re: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
Hi Amar,
On Fri, Jan 4, 2013 at 1:34 AM, Amar 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
> Signed-off-by: Amar
> ---
> 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() + (0x1 * 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
> -#include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> +#include
> +
> +#defineDWMMC_MAX_CH_NUM4
> +#defineDWMMC_MAX_FREQ 5200
> +#defineDWMMC_MIN_FREQ 40
> +#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
> +#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
> +#defineONE_MEGA_HZ 100
> +#defineSCALED_VAL_FOUR_HUNDRED 400
I don't think you need these last two - you can just write the number
in the code
>
> static char *EXYNOS_NAME = "EXYNOS DWMMC";
Same with this I think
> +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?
> +
> + /* 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
[U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC
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
Signed-off-by: Amar
---
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)
{
unsigned int base = samsung_get_base_mmc() + (0x1 * 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
-#include
#include
+#include
+#include
+#include
#include
#include
+#include
+
+#defineDWMMC_MAX_CH_NUM4
+#defineDWMMC_MAX_FREQ 5200
+#defineDWMMC_MIN_FREQ 40
+#defineDWMMC_MMC0_CLKSEL_VAL 0x03030001
+#defineDWMMC_MMC2_CLKSEL_VAL 0x03020001
+#defineONE_MEGA_HZ 100
+#defineSCALED_VAL_FOUR_HUNDRED 400
static char *EXYNOS_NAME = "EXYNOS DWMMC";
+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;
+ }
+
+ /* 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]));
+#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, 5200, 40);
+ 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);
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
