From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joonyoung Shim Date: Tue, 22 Jan 2013 14:23:32 +0900 Subject: [U-Boot] [PATCH V4 4/9] EXYNOS5: DWMMC: Added FDT support for DWMMC In-Reply-To: References: <1357292050-12137-1-git-send-email-amarendra.xt@samsung.com> <1357292050-12137-5-git-send-email-amarendra.xt@samsung.com> <50EF9125.2000903@samsung.com> Message-ID: <50FE2254.5070809@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 wrote: > >> Hi Jaehoon, >> >> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung >> wrote: >>> On 01/11/2013 12:33 AM, Simon Glass wrote: >>>> Hi Amar, >>>> >>>> On Fri, Jan 4, 2013 at 1:34 AM, Amar 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 >>>>> Signed-off-by: Amar >>>>> --- >>>>> 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 >>>>> -#include >>>>> #include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> #include >>>>> #include >>>>> +#include >>>>> + >>>>> +#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 at lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot >> > > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot