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

2013-01-21 Thread Joonyoung Shim

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 s...@chromium.org wrote:


Hi Jaehoon,

On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung jh80.ch...@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...@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.vi...@samsung.com
Signed-off-by: Amar amarendra...@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() + (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 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
+
+#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 

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

2013-01-21 Thread Amarendra Reddy
Hi Joonyoung,
Thanks for the comments.
Please find my response below.

Thanks  Regards
Amarendra

On 22 January 2013 10:53, Joonyoung Shim jy0922.s...@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 s...@chromium.org wrote:

 Hi Jaehoon,

 On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung jh80.ch...@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...@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.vi...@samsung.com
 Signed-off-by: Amar amarendra...@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 

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

2013-01-11 Thread Amarendra Reddy
Hi Simon / Jaehoon,

Thanks for review comments.
Please find the responses below.

Thanks  Regards
Amarendra Reddy

On 11 January 2013 11:14, Simon Glass s...@chromium.org wrote:

 Hi Jaehoon,

 On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung jh80.ch...@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...@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.vi...@samsung.com
  Signed-off-by: Amar amarendra...@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() + (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 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
  +
  +#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, 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 

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

2013-01-10 Thread Simon Glass
Hi Amar,

On Fri, Jan 4, 2013 at 1:34 AM, Amar amarendra...@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.vi...@samsung.com
 Signed-off-by: Amar amarendra...@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() + (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 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
 +
 +#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]) |
 +   

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

2013-01-10 Thread Jaehoon Chung
On 01/11/2013 12:33 AM, Simon Glass wrote:
 Hi Amar,
 
 On Fri, Jan 4, 2013 at 1:34 AM, Amar amarendra...@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.vi...@samsung.com
 Signed-off-by: Amar amarendra...@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() + (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 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
 +
 +#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 get the FSYS1 clock value
div = DIV_ROUND_UP(sclk, freq); = freq is request clock value.

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

2013-01-10 Thread Simon Glass
Hi Jaehoon,

On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung jh80.ch...@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...@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.vi...@samsung.com
 Signed-off-by: Amar amarendra...@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() + (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 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
 +
 +#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 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 

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

2013-01-04 Thread Amar
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.vi...@samsung.com
Signed-off-by: Amar amarendra...@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)
 {
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 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
+
+#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