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

[Doug Anderson]

(again, but not HTML)

On Tue, May 10, 2016 at 3:19 AM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
> å 2016/5/10 0:31, Doug Anderson åé:
>>
>> Hi,
>>
>> On Mon, May 9, 2016 at 4:12 AM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>> 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.

Right.  I was saying that not only are all lines within a given SoC
similar in terms of delay, but I was suggesting that perhaps the
internal delay for rk3188, rk3288, rk3228, ... is probably fairly
similar.  That's only a guess, though.


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

I have less faith than you in the TRM.  The TRM is full of minor
errors and is often wrong about the default state of things.  IMHO the
only true way to find out is to boot up some SoCs and check.

As far as whether code touches these values:

* I'm 100% certain that the kernel on rk3288 I tested doesn't touch.

* I'm 100% certain that the kernel on rk3399 I tested doesn't touch.

* I'm 95% certain that the BIOS (coreboot/depthcharge) on rk3288 I
tested doesn't touch.

* I'm 95% certain that the BIOS (coreboot/depthcharge) on rk3399 I
tested doesn't touch.  Note that the TRM on rk3399 does say 180
degrees is the default, but I believe you have also verified that it
comes up as 270.  Have you found any reason for this?


Also note that I look at V1.1 of the rk3288 TRM in the CRU section and I see:

* CRU_SDMMC_CON0: drive degree defaults to 0x1
* CRU_SDIO0_CON0: drive degree defaults to 0x1
* CRU_SDIO1_CON0: drive degree defaults to 0x1
* CRU_EMMC_CON0: drive degree defaults to 0x1

That actually means that they default to 90.  Of course you can also
see that the TRM is probably flawed because it claims that all the
bits here are "WO" (write only).  They are clearly not.

The TRM is also internally inconsistent.  When I then go into the
Mobile Storage Host section (Variable Delay/Clock Generation), I see
that it does in fact claim that the default 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.

I'm OK with using 180 always as long as SD cards continue to work OK.
Best would be if someone could actually run a protocol analyzer for
all the different speed modes.


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

Hardcoding in this case is much better than putting in a device tree
binding.  I'm nearly certain that having a device tree binding that
doesn't account for different clock rates / speed modes is not a good
idea, and device tree bindings are supposed to be "set in stone".
Let's just hardcode it for now.

I can post up a CL.


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

You probably have the right TRMs.  The TRMs are just not accurate.  ;)