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

[Dinh Nguyen <dinguyen@altera.com>]

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.


> 
> >> 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.


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?

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
> >>>>>>
> >>>>
> >>>
> >>>
> >>>
> > 
> > 
> 
>