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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
