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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dinh,

On 01/10/2014 01:15 AM, Dinh Nguyen wrote:
> On Thu, 2014-01-09 at 12:41 +0900, Jaehoon Chung wrote:
>> 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?
> 
> I think the Rockchip platform can also use it, but it hasn't been
> clearly documented yet.
> 
>> Under SoC.. cclk_in_drv can be implemented differently.
> 
> Yes, agreed. Clearly the cclk_in_drv and cclk_in_sample methods to shift
> the phase of the CIU clock is implemented differently. The exynos'
> implementation is not getting touched at all.
> 
>> 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 let's take a look at what are the possible values ddr/sdr timings
> can take? From Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:
> 
> "Valid values for SDR and DDR CIU clock timing for Exynos5250:
>       - valid value for tx phase shift and rx phase shift is 0 to 7."
> 
> For socfpga, 0 = 45 degrees shift, 1 = 90 degrees shift,...[n++] and 7 =
> 315 degrees shift. If my guess is correct, it should also be the same
> for exynos.

Right, socfpga is used with similar timing value.
But All SoC should not be same with exynos.

> 
> 
>>
>>>> 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.
> 
> Do you agree that the 2nd value of sdr/ddr timing is representing the
> cclk_in_drv? Looking at the code dw_mmc-exynos.c, this is indeed what
> the 2nd value in sdr/ddr timing is representing.

Agreed it. did you think that cclk_in_drv and DDR/SDR timing value is same?
Right..it's same meaning. Using Hold_REG is affected with presence of sdr/ddr timing.

> 
> 
> So we can come up with a more generic DTS binding that the generic
> dw_mmc driver can use to set the hold-reg, but that binding still needs
> to pass in the cclk_in_drv somehow, as cclk_in_drv is the only external
> variable that is needed to determine when to set hold-reg.
> 
> And if sdr/ddr timing is already representing cclk_in_drv, doesn't it
> make sense to re-use that binding?

I didn't know exactly which soc is used with sdr/ddr timing.
As socfpga and exynos is used, it's not become the general property.

Well, I understood your opinion. So i will consider more about this.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Dinh
>>
>> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux