Re: [PATCH 2/2] dt-bindings: rockchip-dw-mshc: add rockchip,default-drv-phase

[Shawn Lin <shawn.lin@rock-chips.com>]

在 2016/5/10 0:31, Doug Anderson 写道:
> Hi,
>
> On Mon, May 9, 2016 at 4:12 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>> 1. Specifying a single number for this property in terms of "degrees"
>>> is probably not right.  The whole point of setting the "drive phase"
>>> is to meet hold times, which are specified in the spec in terms of ns
>>> in the spec and also specified differently for different SD/MMC speed
>>> modes.  Note also that "phase" translates to very different delays (in
>>> terms of ns) depending on the clock rate:
>>>
>>> At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of 625
>>> ns
>>> At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a
>>> delay of 10 ns.
>>> At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a
>>> delay of 5 ns.
>>> At 200 MHz, period is period is 5 ns, so a 90 degree phase offset
>>> represents a delay of 1.25 ns.
>>
>>
>> yes, if we use degrees only(0/90/180/270), the timing is always right.
>> But considering the delay number, we need to do some crazy calculation
>> in the set_ios callback.
>
> Great, so let's limit it to 0/90/180/270 for the drive phase offset.
> We don't need lots of precision for the drive phase offset (right?)
> and accuracy is more important.

yes.

>
>
>>> 2. As I understand it, the value needed for the drive phase is not
>>> board specific unless you've got super crazy layout on a board (where
>>> the clock line takes a very different path than everything else).
>>> It's also not even terribly SoC-specific unless you've got some very
>>> strange incarnation of dw_mmc that has very different internal delays
>>> than everyone else.  Said another way, until we see an instance of an
>>> SoC/board that really needs to do things special I'd say that we
>>> should just implement this all in code (no device tree bindings).
>>>
>>
>> I'm prone to think it should be Soc specific if making sure the layout
>> for data lines is in equal length.
>
> Sure, it can be SoC specific.  ...though at the moment, I'd bet that
> you can come up with a single rule for the drive phase offset that
> will work for every Rockchip SoC produced so far, especially if you
> are using only 0/90/180/270.  I'd imagine that they all have similar
> enough internal delays.

For a specific Soc, it's the basic rule to make sure the internal delays
is the same(nearly the same) for all of the lines.

>
>
>>> 3. If this property was actually board specific and actually needed to
>>> be tuned board-by-board, you'd have a bug because your new device tree
>>> bindings are not backward compatible and you'd probably be breaking
>>> old boards.  Specifically you're changing the definition of what
>>> happens when "rockchip,default-drv-phase" is not specified.  Old
>>> behavior was to leave the value that was setup by the firmware (or
>>> perhaps the hardware default if the firmware didn't touch this).
>>
>>
>> drv_phase is for all the data lines instead of tuning the lines
>> one-by-one. So this patch can't save the terrible board layout.
>> But I agree that it will break the compatibility backward if firware
>> touch this value.
>>
>>>
>>> ---
>>>
>>> OK, so what should we do?
>>>
>>> We could certainly do lots of crazy math to come up with the ideal
>>> hold time for all different speed modes and all different types of
>>> cards.  With my reading of the Designware Databook this would mean
>>> that somewhere we'd want to specify which delay method we're using
>>> (phase shift vs. delay line) and how long all the delays timings all
>>> are on your particular SoC.  That all sounds quite difficult, though.
>>
>>
>> delay line is diff from chip to chip, soc-to-soc, board-to-board. For
>> sample-phase we have tuning process and re-tune, but not for drv-phase.
>> So We bascially should avoid to use it for drv-phase. Another
>> consideration is the temperature drift of delay line.
>>
>> Maybe we should do some tricky limitation on clk-mmc-phase to only
>> support fixed degrees?
>
> As per above, let's not use delay line for drive delay.  On all
> Rockchip SoCs that I have seen it's possible to make 0/90/180/270 very
> accurately.  That means no board-to-board differences.  Current
> mainline Linux kernel source code will always make 0/90/180/270 using
> phase offset (not delay line).

yes, 0/90/180/270 is very accurate and mainline kernel use phase offset
for these four phase.

>
> For sampling we use tuning and using the delay elements makes some
> sense (since 90 degrees is not quite accurate enough to fully tune
> UHS).  ...but for driving where the only requirement is hold times we
> don't need delay elements.

Right. We care hold time here.

>
>
>>> Probably you could just add a simple function that looked at the clock
>>> and speed mode and always chose an offset of 90 or 180 degrees.  At
>>> least on Rockchip devices you can be certain that you can make 90 and
>>> 180 degrees using phase shifts and thus the timings should be
>>> consistent.  By default you could just always choose 180.  The
>>> Designware databook has some examples where it picked 90 degrees
>>> (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC
>>> expert to know if there is some benefit to choosing 90.  Would we
>>> violate any specs if we just chose 180 degrees all the time everywhere
>>> on all Rockchip devices?
>>
>>
>> It needs more waveform test to see how things going. But most of
>> rockchip platforms in the pass years didn't touch drv-phase stuff not
>> only in kernel but also in firmware, then we still cannot see the
>> violation against the spec when using defalut reset value, namely 180, for
>> drv-phase.
>
> Right, most Rockchip platforms simply don't touch this and it works
> OK.  ...but I don't think it defaults to 180.  Grepping through on my
> veyron (rk3288) device shows
>
>        sdio1_drv - 90
>        sdio0_drv - 90
>        sdmmc_drv - 90
>        emmc_drv -180
>
> ...and, as we've seen, these values appear to be 270 on some other SoCs.
>

Have your code touched them? I check the TRM and find it should be 180
always.

Also for rk3036/3368/3399/3228.... the reset vaule is 180...

>
> The claim from the Designware Databook says that SDR104 and SDR12, and
> identification mode need 180 degrees to work properly.  It would be
> interesting to hook up rk3288 to a signal analyzer and see hold times
> are OK or if we need to move up to 180 degrees for those modes.
>
> Note that the Designware Databook assumes "Delay_O" of 1.4ns.  If
> yours is shorter then maybe 90 degrees is OK for those modes?
>

maybe. But I think 180(downside) is the better.

>
> Also: I still haven't heard whether there is any downside to using 180
> degrees for modes that only require 90 degrees.  Does it cause
> problems to just always use 180 degrees?  If not, we could possibly
> use 180 degrees everywhere and just hardcode it?

 From tons of test on rockchip products which always use 180, I didn't
see failure. Hardcoding it doesn't look so decent maybe... but anyway
it works.

>
>
> ...but if 180 is ideal everywhere, can you answer:
>
> * Why does dw_mmc manual spend so much time talking about it?  It
> could just say "set to 180 degrees".

I'm sure all the rockchip Socs defaultly use 180 for drv_phase if I got
the right TRMs these years.

Anyway let me check it with my ASIC team and I will let you know
the result.


Thanks.

>
> * Why do Rockchip SoCs default to values that are not 180 degrees?
>
> -Doug
>
>
>


-- 
Best Regards
Shawn Lin