[U-Boot] [PATCH v3 1/4]drivers: mmc: dwmmc: enable support for DT

[Jaehoon Chung <jh80.chung@samsung.com>]

Hi, Minkyu.

On 04/08/2014 02:08 PM, Beomho Seo wrote:
> Thank you for your advice.
> 
> On 04/08/2014 11:53 AM, Minkyu Kang wrote:
>> Dear Beonho Seo,
>>
>> On 20/03/14 13:14, Beomho Seo wrote:
>>> This patch enables for device tree for dw-mmc driver.
>>> Driver have been fixed because it is used exynos5 and exynos4.
>>> Add, fdt compat id of exynos4 dwmmc.
>>>
>>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Tested-by: Piotr Wilczek <p.wilczek@samsung.com>
>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>> Cc: Piotr Wilczek <p.wilczek@samsung.com>
>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>> ---
>>> Changes for v3:
>>> - None.
>>>
>>>  drivers/mmc/exynos_dw_mmc.c |   21 +++++++++++++++++----
>>>  include/fdtdec.h            |    1 +
>>>  lib/fdtdec.c                |    1 +
>>>  3 files changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
>>> index de8cdcc..b9d41d4 100644
>>> --- a/drivers/mmc/exynos_dw_mmc.c
>>> +++ b/drivers/mmc/exynos_dw_mmc.c
>>> @@ -13,6 +13,7 @@
>>>  #include <asm/arch/dwmmc.h>
>>>  #include <asm/arch/clk.h>
>>>  #include <asm/arch/pinmux.h>
>>> +#include <asm/gpio.h>
>>>
>>>  #define	DWMMC_MAX_CH_NUM		4
>>>  #define	DWMMC_MAX_FREQ			52000000
>>> @@ -82,6 +83,7 @@ int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32 clksel)
>>>  	freq = 52000000;
>>>  	sclk = get_mmc_clk(index);
>>>  	div = DIV_ROUND_UP(sclk, freq);
>>> +	div -= 1;
>>
>> why?
>>
> 
> It need for set accurately required frequency.
> Above code prevent to set below the required frequency.
> For example above case,
> If sclk is 100 MHz and required frequency 50 MHz, Where div = 2.
> and then mmc clock set 33 MHz calculating.
> So It need div minus one.

It didn't consider how div value is set into CMU register.
SCLK = (MOUT/(PRE_RATIO + 1))/(RATIO + 1).
Finally, it didn't consider for plus one. Source clock is set to wrong value.

It needs to change other approach than using just "div -= 1".

If i understood something wrong, let me know, plz.

Best Regards,
Jaehoon Chung
> 
>>>  	/* set the clock divisor for mmc */
>>>  	set_mmc_clk(index, div);
>>>
>>> @@ -118,15 +120,21 @@ int exynos_dwmmc_init(const void *blob)
>>>  {
>>>  	int index, bus_width;
>>>  	int node_list[DWMMC_MAX_CH_NUM];
>>> -	int err = 0, dev_id, flag, count, i;
>>> +	int err = 0, dev_id, flag, count, i, compat_id;
>>>  	u32 clksel_val, base, timing[3];
>>>
>>> +#ifdef CONFIG_EXYNOS4
>>> +	compat_id = COMPAT_SAMSUNG_EXYNOS4_DWMMC;
>>> +#else
>>> +	compat_id = COMPAT_SAMSUNG_EXYNOS5_DWMMC;
>>> +#endif
>>
>> NAK.
>> please don't use ifdef.
>>
> 
> OK. I will change it.
> 
>>> +
>>>  	count = fdtdec_find_aliases_for_id(blob, "mmc",
>>> -				COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
>>> -				DWMMC_MAX_CH_NUM);
>>> +				compat_id, node_list, DWMMC_MAX_CH_NUM);
>>>
>>>  	for (i = 0; i < count; i++) {
>>>  		int node = node_list[i];
>>> +		struct fdt_gpio_state pwr_gpio;
>>>
>>>  		if (node <= 0)
>>>  			continue;
>>> @@ -145,6 +153,9 @@ int exynos_dwmmc_init(const void *blob)
>>>  		else
>>>  			flag = PINMUX_FLAG_NONE;
>>>
>>> +		fdtdec_decode_gpio(blob, node, "pwr-gpios", &pwr_gpio);
>>> +		if (fdt_gpio_isvalid(&pwr_gpio))
>>> +			gpio_direction_output(pwr_gpio.gpio, 1);
>>
>> please add blank line.
>>
> 
> OK. I will add blank line.
> 
>>>  		/* config pinmux for each mmc channel */
>>>  		err = exynos_pinmux_config(dev_id, flag);
>>>  		if (err) {
>>> @@ -152,7 +163,9 @@ int exynos_dwmmc_init(const void *blob)
>>>  			return err;
>>>  		}
>>>
>>> -		index = dev_id - PERIPH_ID_SDMMC0;
>>> +		index = fdtdec_get_int(blob, node, "index", dev_id);
>>> +		if (index == dev_id)
>>> +			index = dev_id - PERIPH_ID_SDMMC0;
>>
>> I can't understand why this routine is needed.
>> Could you please explain?
>>
> 
> SDMMC index(0, 1 ... 4) is need to get/set mmc clk.
> It is decided the difference between dev_id and PERIPH_ID_SDMMC0(=75).
> But If dev_id is PERIPH_ID_SDMMC4(=131), index is set inaccurately.
> So I add index property at device tree. and then parse it.
> If index property not exist, index is decided the difference between dev_id and PERIPH_ID_SDMMC0.
> 
>>>
>>>  		/* Get the base address from the device node */
>>>  		base = fdtdec_get_addr(blob, node, "reg");
>>> diff --git a/include/fdtdec.h b/include/fdtdec.h
>>> index 63027bd..15f50fe 100644
>>> --- a/include/fdtdec.h
>>> +++ b/include/fdtdec.h
>>> @@ -83,6 +83,7 @@ enum fdt_compat_id {
>>>  	COMPAT_SAMSUNG_EXYNOS5_DP,	/* Exynos Display port controller */
>>>  	COMPAT_SAMSUNG_EXYNOS5_DWMMC,	/* Exynos5 DWMMC controller */
>>>  	COMPAT_SAMSUNG_EXYNOS_MMC,	/* Exynos MMC controller */
>>> +	COMPAT_SAMSUNG_EXYNOS4_DWMMC,	/* Exynos4 DWMMC controller */
>>
>> Do we need to separate exynos4 dwmmc and exynos5 dwmmc?
>>
> 
> OK, I will change and test it.
> 
>> Jaehoon,
>> how you think?
>>
>>>  	COMPAT_SAMSUNG_EXYNOS_SERIAL,	/* Exynos UART */
>>>  	COMPAT_MAXIM_MAX77686_PMIC,	/* MAX77686 PMIC */
>>>  	COMPAT_GENERIC_SPI_FLASH,	/* Generic SPI Flash chip */
>>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>>> index be04598..79179cf 100644
>>> --- a/lib/fdtdec.c
>>> +++ b/lib/fdtdec.c
>>> @@ -56,6 +56,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
>>>  	COMPAT(SAMSUNG_EXYNOS5_DP, "samsung,exynos5-dp"),
>>>  	COMPAT(SAMSUNG_EXYNOS5_DWMMC, "samsung,exynos5250-dwmmc"),
>>>  	COMPAT(SAMSUNG_EXYNOS_MMC, "samsung,exynos-mmc"),
>>> +	COMPAT(SAMSUNG_EXYNOS4_DWMMC, "samsung,exynos4412-dwmmc"),
>>>  	COMPAT(SAMSUNG_EXYNOS_SERIAL, "samsung,exynos4210-uart"),
>>>  	COMPAT(MAXIM_MAX77686_PMIC, "maxim,max77686_pmic"),
>>>  	COMPAT(GENERIC_SPI_FLASH, "spi-flash"),
>>>
>>
>> Thanks,
>> Minkyu Kang.
>>
> 
>