Hi Jaehoon, Thanks for your comments. Please find the response below.
Thanks & Regards Amarendra
On 21 December 2012 14:48, Jaehoon Chung jh80.chung@samsung.com wrote:
On 12/21/2012 02:16 PM, Amarendra Reddy wrote:
Hi Simon,
Thanks for your review comments. Please find the responses below.
Thanks & Regards Amarendra Reddy
On 20 December 2012 07:53, Simon Glass sjg@chromium.org wrote:
Hi Amar,
On Mon, Dec 17, 2012 at 3:19 AM, Amar amarendra.xt@samsung.com wrote:
Signed-off-by: Amar amarendra.xt@samsung.com
Good to see this patch! Please can you add a short commit message? Oops.. sorry. I will put the commit msg.
arch/arm/include/asm/arch-exynos/dwmmc.h | 4 +
drivers/mmc/exynos_dw_mmc.c | 117 ++++++++++++++++++++++++++++-- include/dwmmc.h | 4 + 3 files changed, 119 insertions(+), 6 deletions(-)
diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h b/arch/arm/include/asm/arch-exynos/dwmmc.h index 8acdf9b..92352df 100644 --- a/arch/arm/include/asm/arch-exynos/dwmmc.h +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h @@ -27,6 +27,9 @@ #define DWMCI_SET_DRV_CLK(x) ((x) << 16) #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
+#ifdef CONFIG_OF_CONTROL +unsigned int exynos_dwmmc_init(const void *blob); +#else int exynos_dwmci_init(u32 regbase, int bus_width, int index);
static inline unsigned int exynos_dwmmc_init(int index, int bus_width) @@ -34,3 +37,4 @@ static inline unsigned int exynos_dwmmc_init(int
index,
int bus_width) 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..3b3e3e5 100644 --- a/drivers/mmc/exynos_dw_mmc.c +++ b/drivers/mmc/exynos_dw_mmc.c @@ -19,23 +19,38 @@ */
#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
What do these last two values mean? I think clock setting should be
done in
clock.c, not here.
In case of non-dt support, these two values are written into respective
clock selection register-CLKSEL, to select the clock for MMC-channel0 and MMC-channel2. Will move clock setting into clock.c and the #defines will be moved into include/asm/arch/clk.h.
I didn't want to move into clk.h. Why do you move there? It is related with CLKSEL register into DesignWare IP.
Ok, now I understand that all register settings corresponding to
DesignWare IP should be present in exynos_dw_mmc.c / dw_mmc.c. Fine, I will not move CLKSEL register setting into clock.c / clk.h.
static char *EXYNOS_NAME = "EXYNOS DWMMC";
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);}
+#ifdef CONFIG_OF_CONTROL +static int exynos_dwmci_init(u32 regbase, int bus_width,
int index, u32 *timing)+#else int exynos_dwmci_init(u32 regbase, int bus_width, int index) +#endif
I'm really not keen on having the same function with different
signatures.
My first question is whether we need to support operation without CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an FDT blob as a parameter, and a separate add_port function which can be called by non-FDT-enabled board files. The init routine will call the add_port function for each port it finds in the FDT.
I think we need support operation without CONFIG_OF_CONTROL atleast
untill
the entire FDT is formed. Hence I will implement it as per your suggestion.
Also please can you briefly comment non-trivial functions? OK.
{ struct dwmci_host *host = NULL; host = malloc(sizeof(struct dwmci_host)); @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width,
int
index) return 1; }
/* set the clock divisor - clk_div_fsys for mmc */if (exynos5_mmc_set_clk_div(index)) {debug("mmc clock div set failed\n");return -1;}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;
host->mmc_clk = exynos_dwmci_get_clk;
add_dwmci(host, 52000000, 400000);
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_NUM];int err = 0;int dev_id, flag;u32 timing[3];int count, i;count = fdtdec_find_aliases_for_id(blob, "dwmmc",COMPAT_SAMSUNG_EXYNOS5_DWMMC,node_list,
DWMMC_MAX_CH_NUM);for (i = 0; i < count; i++) {int node = node_list[i];if (node <= 0)continue;/* config pinmux for each mmc channel */dev_id = pinmux_decode_periph_id(blob, node);if (dev_id == PERIPH_ID_SDMMC0)flag = PINMUX_FLAG_8BIT_MODE;elseflag = PINMUX_FLAG_NONE;This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node, "samsung,width") == 8.
OK.
err = exynos_pinmux_config(dev_id, flag);if (err) {debug("DWMMC not configured\n");return err;}/* Get the base address from the device node */base = fdtdec_get_addr(blob, node, "reg");if (!base) {debug("DWMMC: Can't get base address\n");return -1;}/* Get the channel index from the device node */index = fdtdec_get_int(blob, node, "index", 0);if (index < 0) {debug("DWMMC: Can't get channel index\n");return -1;}What is this needed for? Doesn't the reg address tell you everything you need here? Ok will change accordingly.
/* Get the bus width from the device node */bus_width = fdtdec_get_int(blob, node, "bus-width", 0);if (bus_width < 0) {debug("DWMMC: Can't get bus-width\n");return -1;}
err = fdtdec_get_int_array(blob, node, "timing",timing, 3);if (err) {debug("Can't get sdr-timings for divider\n");return -1;}err = exynos_dwmci_init(base, bus_width,index, timing);if (err) {debug("Can't do dwmci init\n");return -1;}}return 0;+} +#endif diff --git a/include/dwmmc.h b/include/dwmmc.h index c8b1d40..4a42849 100644 --- a/include/dwmmc.h +++ b/include/dwmmc.h @@ -123,6 +123,9 @@ #define MSIZE(x) ((x) << 28) #define RX_WMARK(x) ((x) << 16) #define TX_WMARK(x) (x) +#define RX_WMARK_SHIFT 16 +#define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)
Extra blank line here? Will remove it.
#define DWMCI_IDMAC_OWN (1 << 31) #define DWMCI_IDMAC_CH (1 << 4) @@ -144,6 +147,7 @@ struct dwmci_host { unsigned int bus_hz; int dev_index; int buswidth;
u32 clksel_val; u32 fifoth_val; struct mmc *mmc;-- 1.7.0.4
Regards, Simon
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot