| Message ID | 1355743176-12305-5-git-send-email-amarendra.xt@samsung.com |
|---|---|
| State | New |
| Headers | show |
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? > --- 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. > > 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. Also please can you briefly comment non-trivial functions? > { > 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; > + else > + flag = PINMUX_FLAG_NONE; > This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node, "samsung,width") == 8. > + > + 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? > + > + /* 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? > > #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
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. > > > > > 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; > > + else > > + flag = 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 > >
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. > >> >>> >>> 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; >>> + else >>> + flag = 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 >
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; > >>> + else > >>> + flag = 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 > > > >
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 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 { 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; + else + flag = PINMUX_FLAG_NONE; + + 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; + } + + /* 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) + #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;
Signed-off-by: Amar <amarendra.xt@samsung.com> --- 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(-)