Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

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

On 12/15/2013 11:16 AM, Dinh Nguyen wrote:
> Hi Zhangfei,
>
>>> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>>>            dev_dbg(host->dev, "ciu clock not available\n");
>>>            host->bus_hz = host->pdata->bus_hz;
>>>        } else {
>>> +        /* If the CIU clk is available, we check for SDR and DDR
>>> +         * timing phase shift settings. But not all platforms
>>> +         * may have populated these settings, the driver will not fail
>>> +         * if these settings are not specified.
>>> +         */
>>> +        if (host->pdata->sdr_timing) {
>>> +            host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
>>> +            if (IS_ERR(host->sdr_clk))
>>> +                dev_dbg(host->dev, "sdr_mmc clock not available\n");
>>> +            else
>>> +                clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
>>> +        }
>>> +
>>> +        if (host->pdata->ddr_timing) {
>>> +            host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
>>> +            if (IS_ERR(host->ddr_clk))
>>> +                dev_dbg(host->dev, "ddr_mmc clock not available\n");
>>> +            else
>>> +                clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
>>> +        }
>>> +
>>
>> Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed,
>> fixed as each controller, or different with different board.
> Yes, they are fixed for each controller per SoC.
>> In case they are fixed, or fixed in each controller, and only required
>> to be set in probe only once, maybe they can be hide in
>> clk_mmc_ops.prepare
>> And the clock can be used as ciu_clock, or parent of ciu_clock, which
>> is called in dw_mmc.c, maybe these code can be ignored.
>>
> Please see v5 of this patch: https://patchwork.kernel.org/patch/3311121/
> The issue with V5 was that there is no way for the dw_mmc driver to pass
> the clock phase settings
> in the clk_mmc_ops.prepare function.

Personally, I think v5 is simpler :)

Could these fixed para be used as internal para when register.
Like
socfpga_gate_clk_init
rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
         compatible = "altr,socfpga-gate-clk";
	clocks = <&mainclk>;
	div-reg = <0x64 0 2>;
	clk-gate = <0x60 1>;
Could we also define mmc-ciu-clock with para clk-init.

Thanks