Re: [PATCHv4] mmc: dw_mmc: Enable the hold reg for certain speed modes

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

Dear, Dinh

On 01/08/2014 11:12 PM, Dinh Nguyen wrote:
> 
> On 1/7/14 6:37 PM, Jaehoon Chung wrote:
>> Hi, Dinh.
>>
>> Sorry for replying too late.
>>
>> ..[snip]..
>>>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>>>>>> +
>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>>>>>> +
>>>>>>>>> +    pdata->cclk_in_drv = 1;
>>>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>>>>>> +        pdata->cclk_in_drv = 0;
>>>>>>>>> +
>>>>>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>>>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>>>>>> If they are really commonly used, how about change name and define in
>>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>>>>>> I had submitted a patch to make this a common binding before:
>>>>>>>
>>>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>>>>>
>>>>>>> I think the ultimate conclusion to that thread was that its perfectly
>>>>>>> acceptable to re-use bindings from other
>>>>>>> platforms.
>>>>>>>
>>>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>>>>>> If this is the conclusion before, then just ignore this noise.
>>>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
>>>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
>>>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
>>>> Then do you suggest I go forward with an attempt to add a new generic
>>>> "snps,dw-mshc-sdr-timing"
>>>> binding?
>>> Ping Jaehoon?
>>>
>>> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
>>> "snps,dw-mshc-ddr-timing" bindings then?
>> Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
>> If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
> I went through the databook and I think that the timing values are
> mentioned throughout the spec. It
> just does not explicitly mentions ddr/sdr timing, but it is mentioned as
> cclk_in_drv(aka drvsel), and
> cclk_in_sample(aka smpsel). Even the macro in dw_mmc-exynos,
> SDMMC_CLKSEL_CCLK_DRIVE() and
> SDMMC_CLKSEL_CCLK_SAMPLE() is stating this.
> 
> So I do not believe that "samsung, dw-mshc-sdr-timing" and
> "samsung,dw-mshc-ddr-timing" are not
> exclusive to only the exynos platform. Just the fact that the SOCFPGA
> platform can re-use the same
> "samsung,dw-mshc-sdr-timing" property proves that this is not just an
> exynos specific value.

i didn't see the register that the sdr/ddr timing is used at Synopsys DoC file.
Even if SOCFPGA platform can reuse the samsung property, it's not general property, isn't?
Under SoC.. cclk_in_drv can be implemented differently.
We have just known that sdr/ddr timing is used at exynos/socfpga board.

I just found clk_drv/smpl_phase_ctrl at UHS register, I think this isn't ddr/sdr timing.

>> But i didn't see sdr/ddr timing in synopsys DoC.
>> I know you want to control the hold-reg bit. 
>> But this approach is not good.
> 
> The spec has a table on when to use the hold-reg bit. The conditions for
> using the hold-reg bit is
> only dependent the hold-reg hw implementation and the value of
> cclk_in_drv. The value of cclk_in_drv
> is specified by the 2nd parameter of "samsung,dw-mshc-sdr-timing".

Right, the spec is mentioned when hold-reg bit could be used.
But that's not that ddr/sdr timing is general value.

Best Regards,
Jaehoon Chung
> 
> So I don't understand why you think this approach is "not good"?
> 
> Thanks,
> Dinh
>> Rather, how about using the callback function for exynos specific value.
>> Then other SoC can also use it.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> Dinh
>>>> Dinh
>>>>> If i missed something, then let me know, plz.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>> Thanks
>>>>>>
>>>>
>>>
>>>
>>>
> 
>