Date:	Mon, 9 May 2016 19:12:17 +0800
From:	Shawn Lin <shawn.lin@...k-chips.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	shawn.lin@...k-chips.com, Jaehoon Chung <jh80.chung@...sung.com>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Brian Norris <briannorris@...omium.org>,
	Heiko Stuebner <heiko@...ech.de>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 2/2] dt-bindings: rockchip-dw-mshc: add
 rockchip,default-drv-phase

On 2016/5/7 1:26, Doug Anderson wrote:
> Shawn,
>
> On Fri, May 6, 2016 at 2:41 AM, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>> rockchip,default-drv-phase is used to set the default drv degrees.
>> drv phases will decide the write timing of mmc controller.
>
> Device tree bindings should probably have been patch 1/2, then the
> code patch 2/2.
>
>
>> Signed-off-by: Shawn Lin <shawn.lin@...k-chips.com>
>> ---
>>
>>  Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt
>> index ea5614b..c48dba6 100644
>> --- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt
>> @@ -29,6 +29,9 @@ Optional Properties:
>>    probing, low speeds or in case where all phases work at tuning time.
>>    If not specified 0 deg will be used.
>>
>> +* rockchip,default-drv-phase: The default phase to set ciu_drv at probing
>> +  for host to write data to devices. If not specified 180 deg will be used.
>
> This is probably not right for a few reasons.
>
> 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.

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

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

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

>
>
>
> -Doug
>
>
>


-- 
Best Regards
Shawn Lin

Powered by blists - more mailing lists