Re: [PATCH 2/3] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[zhangfei <zhangfei.gao@linaro.org>]

On 12/16/2013 03:29 PM, Seungwon Jeon wrote:
> On Mon, December 16, 2013, Zhangfei Gao wrote:
>> Dear Seungwon
>>
>> On 12/16/2013 11:50 AM, Seungwon Jeon wrote:
>>> On Sat, December 14, 2013, Zhangfei Gao wrote:
>>
>>>> +	/* SoC portion */
>>>> +	dwmmc_0: dwmmc0@fcd03000 {
>>>> +		compatible = "hisilicon,hi4511-dw-mshc";
>>>> +		reg = <0xfcd03000 0x1000>;
>>>> +		interrupts = <0 16 4>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +		clocks = <&mmc_clock HI3620_SD_CIUCLK>, <&clock HI3620_DDRC_PER_CLK>;
>>>> +		clock-names = "ciu", "biu";
>>>> +		clock-freq-table =
>>>> +		<25000000 0 50000000 25000000 50000000 100000000 0 50000000>;
>>> I think it could be solved with mmc->f_min and mmc->f_max(from clock-freq-min-max property)
>>> if there is no limitation per each speed mode. As seeing described table, it looks that.
>>
>> Have tried them before, but unfortunately, they are different.
>>
>> The controller can not generate clock itself, while depending on the
>> outside clock generator, which may require to change clock source in
>> differnt mode.
>>
>> The value we want is set the capacibility of the clock input and the max
>> clock freq where mmc works stable in that mode, which may be different
>> in different mode.
>>
>> clock-freq-min-max will set the value for set_ios.
>> For example, we use 25M as clock capability when init, which can not be
>> set as freq-min, and used as set_ios, where 400K should be used,
>> otherwise init definitely fail.
> Can you check 'f_init' within 'drivers/mmc/core/core.c' file?
> If you set 'f_min = 25000000' for init-sequence, mmc_set_ios() will pass that rate through 'ios.clock'
> Then, dw_mmc-k3 can do clk_set_rate() with 25MHz at dw_mci_k3_set_ios().

This means controller directly use 25M init emmc and sd card, it will fail.
We still need 400K init freq, while the input clock source is 25M,
divided by the controller.

> And other required speeds for each mode seem not specific compared with standard spec.
> Also clk_set_rate() would be possible with 'ios.clock' instead of specific table.
> I just referred your example's clock-freq-table above.
There also some controller limitation currently.
In some mode, the controller only works well in the limited freq.
For example UHS_SDR104_MAX_DTR 208000000 can not be used, only half may
be reached, at least currently

> 
>>
>>>> +static int dw_mci_k3_suspend(struct device *dev)
>>>> +{
>>>> +	struct dw_mci *host = dev_get_drvdata(dev);
>>>> +
>>>> +	if (!IS_ERR(host->ciu_clk))
>>>> +		clk_disable_unprepare(host->ciu_clk);
>>>> +
>>>> +	return dw_mci_suspend(host);
>>> If k3 needs clk_disable here, dw_mci_suspend()is expected to be called prior to
>> clk_disable_unprepare()?
>>> Of course, it's ok at present though.
>>>
>>
>> Sure, it can be switched, though dw_mci_suspend does nothing related
>> with clock now.
> Yes, but if some works with ciu_clk is added in dw_mci_suspend(), it will affect.
> 
Sorry for the misunderstood, I mean it make sense and will change.
Thanks the advice.