| Message ID | 1357292050-12137-5-git-send-email-amarendra.xt@samsung.com |
|---|---|
| State | New |
| Headers | show |
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() 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.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? 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() + (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 > > 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_CLK(timing[1]) | > + DWMCI_SET_DIV_RATIO(timing[2])); timing should be a parameter, not a global. For the non-FDT case perhaps you can accept NULL, meaning default? Then: if (timing) do the code above else do the code below > +#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, 52000000, 400000); > + 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); Is error checking needed here? > > 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; > + 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; > + > + /* Extract device id for each mmc channel */ > + dev_id = pinmux_decode_periph_id(blob, node); > + > + /* Get the bus width from the device node */ > + bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0); > + if (bus_width < 0) { <= 0? The function will return 0 if there is no property. > + debug("DWMMC: Can't get bus-width\n"); > + return -1; > + } > + if (8 == bus_width) > + flag = PINMUX_FLAG_8BIT_MODE; > + else > + flag = PINMUX_FLAG_NONE; > + > + /* config pinmux for each mmc channel */ > + err = exynos_pinmux_config(dev_id, flag); > + if (err) { > + debug("DWMMC not configured\n"); > + return err; > + } > + > + index = dev_id - PERIPH_ID_SDMMC0; > + > + /* 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; > + } > + /* Extract the timing info from the node */ > + err = fdtdec_get_int_array(blob, node, "samsung,timing", > + timing, 3); > + if (err) { > + debug("Can't get sdr-timings for divider\n"); > + return -1; > + } > + /* Initialise each mmc channel */ > + err = exynos_dwmci_init(base, bus_width, index); > + 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) > + Remove this extra line? > > #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.8.0 > Regards, Simon
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() 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.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? > > 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() + (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? > >> >> 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. mmc_set_clk(index, div); Then we can set to div value at clock register. It didn't need to add this code... >> + >> + /* 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])); > > timing should be a parameter, not a global. For the non-FDT case > perhaps you can accept NULL, meaning default? Then: > > if (timing) > do the code above > else > do the code below > >> +#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, 52000000, 400000); >> + 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); > > Is error checking needed here? > >> >> 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; >> + 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; >> + >> + /* Extract device id for each mmc channel */ >> + dev_id = pinmux_decode_periph_id(blob, node); >> + >> + /* Get the bus width from the device node */ >> + bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0); >> + if (bus_width < 0) { > > <= 0? The function will return 0 if there is no property. > >> + debug("DWMMC: Can't get bus-width\n"); >> + return -1; >> + } >> + if (8 == bus_width) >> + flag = PINMUX_FLAG_8BIT_MODE; >> + else >> + flag = PINMUX_FLAG_NONE; >> + >> + /* config pinmux for each mmc channel */ >> + err = exynos_pinmux_config(dev_id, flag); >> + if (err) { >> + debug("DWMMC not configured\n"); >> + return err; >> + } >> + >> + index = dev_id - PERIPH_ID_SDMMC0; >> + >> + /* 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; >> + } >> + /* Extract the timing info from the node */ >> + err = fdtdec_get_int_array(blob, node, "samsung,timing", >> + timing, 3); >> + if (err) { >> + debug("Can't get sdr-timings for divider\n"); >> + return -1; >> + } >> + /* Initialise each mmc channel */ >> + err = exynos_dwmci_init(base, bus_width, index); >> + 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) >> + > > Remove this extra line? > >> >> #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.8.0 >> > Regards, > Simon >
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() 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.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? >> >> 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() + (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? >> >>> >>> 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 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. Regards, Simon
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() 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.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)". > >> > >>> { > >>> 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 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. > > 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-boot >
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() 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.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. 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() + (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 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. >>> 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-boot >> > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
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() 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.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 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. >>>> 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-boot<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<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..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() + (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 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, 52000000, 400000); + 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_NUM]; + int err = 0; + int dev_id, flag; + 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; + + /* Extract device id for each mmc channel */ + dev_id = pinmux_decode_periph_id(blob, node); + + /* Get the bus width from the device node */ + bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0); + if (bus_width < 0) { + debug("DWMMC: Can't get bus-width\n"); + return -1; + } + if (8 == bus_width) + flag = PINMUX_FLAG_8BIT_MODE; + else + flag = PINMUX_FLAG_NONE; + + /* config pinmux for each mmc channel */ + err = exynos_pinmux_config(dev_id, flag); + if (err) { + debug("DWMMC not configured\n"); + return err; + } + + index = dev_id - PERIPH_ID_SDMMC0; + + /* 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; + } + /* Extract the timing info from the node */ + err = fdtdec_get_int_array(blob, node, "samsung,timing", + timing, 3); + if (err) { + debug("Can't get sdr-timings for divider\n"); + return -1; + } + /* Initialise each mmc channel */ + err = exynos_dwmci_init(base, bus_width, index); + 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;
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.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(-)