Hi Joonyoung, Thanks for the comments. Please find my response below.
Thanks & Regards Amarendra
On 22 January 2013 10:53, Joonyoung Shim jy0922.shim@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@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() withthe existing function set_mmc_clk(). set_mmc_clk() will do the
purpose.
2)Computation of FSYS block clock divisor (pre-ratio) isadded.
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.
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 withONE_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@lists.denx.de http://lists.denx.de/mailman/**listinfo/u-boothttp://lists.denx.de/mailman/listinfo/u-boot
______________________________**_________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/**listinfo/u-boothttp://lists.denx.de/mailman/listinfo/u-boot