[PATCH 1/2] mmc: dw_mmc-rockchip: add default drv phase support

[PATCH 1/2] mmc: dw_mmc-rockchip: add default drv phase support

[Shawn Lin]

This patch introduces default drv phase exposed to dts for
users to set drv phase. If default-drv-phase is not assigned,
let's set it to 180 degrees.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/mmc/host/dw_mmc-rockchip.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c20b81..482eff1 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -24,6 +24,7 @@ struct dw_mci_rockchip_priv_data {
 	struct clk		*drv_clk;
 	struct clk		*sample_clk;
 	int			default_sample_phase;
+	int			default_drv_phase;
 };
 
 static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
@@ -66,6 +67,9 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 	/* Make sure we use phases which we can enumerate with */
 	if (!IS_ERR(priv->sample_clk))
 		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+
+	if (!IS_ERR(priv->drv_clk))
+		clk_set_phase(priv->drv_clk, priv->default_drv_phase);
 }
 
 #define NUM_PHASES			360
@@ -203,6 +207,10 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
 					&priv->default_sample_phase))
 		priv->default_sample_phase = 0;
 
+	if (of_property_read_u32(np, "rockchip,default-drv-phase",
+					&priv->default_drv_phase))
+		priv->default_drv_phase = 180;
+
 	priv->drv_clk = devm_clk_get(host->dev, "ciu-drive");
 	if (IS_ERR(priv->drv_clk))
 		dev_dbg(host->dev, "ciu_drv not available\n");
-- 
2.3.7

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

[Shawn Lin]

rockchip,default-drv-phase is used to set the default drv degrees.
drv phases will decide the write timing of mmc controller.

Signed-off-by: Shawn Lin <shawn.lin@rock-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.
+
 Example:
 
 	rkdwmmc0@12200000 {
-- 
2.3.7

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

[Doug Anderson]

Shawn,

On Fri, May 6, 2016 at 2:41 AM, Shawn Lin <shawn.lin@rock-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@rock-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.


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



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

---

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.

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?



-Doug

Re: [PATCH 1/2] mmc: dw_mmc-rockchip: add default drv phase support

[Doug Anderson]

Hi,

On Fri, May 6, 2016 at 2:40 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> This patch introduces default drv phase exposed to dts for
> users to set drv phase. If default-drv-phase is not assigned,
> let's set it to 180 degrees.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>  drivers/mmc/host/dw_mmc-rockchip.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Replying to this patch in case someone isn't reading the bindings
patch.  See my reply there that it's probably wrong to specify it this
way in the device tree.  Let's continue the discussion there.

-Doug

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

[Shawn Lin]

On 2016/5/7 1:26, Doug Anderson wrote:
> Shawn,
>
> On Fri, May 6, 2016 at 2:41 AM, Shawn Lin <shawn.lin@rock-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@rock-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

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

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


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


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

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.


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


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?


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?


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

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

-Doug

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

[Shawn Lin]

在 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

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@rock-chips.com> wrote:
> 在 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.

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

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

[Shawn Lin]

On 2016/5/10 23:57, Doug Anderson wrote:
> (again, but not HTML)
>

------8<------------------

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

Okay, I should not have too much confidence on my TRM maybe :).

Again, We SHOULD refer to the Mobile Storage Host section (Variable
Delay/Clock Generation) instead of CRU section, otherwise even you will
see inconsistent decription of mmc_clock->shift.



> As far as whether code touches these values:
>

---------8<-----------------

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

NAK my previous comments here. Downside is better for SRD, but won't
work for DDR mode. When running in DDR mode, we should use 90 instead.

So let me elaborate a bit more here.
For DDR mode, one single clk cycle should sending two data bits outside
to the devices. We need a hold time for both. If 180 is used, the first
bit occurs around the downside area, which won't be sampled by devices
on the upside.  So on the upside, the devices will see a zero bit if you
actually send a one-bit, which makes the devices  generate CRC finally.


For this above, 180 for all SDR mode is ok, but 90 should be deployed
for DDR mode. So simply checking the timing to hardcode it should be
fine.

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



-- 
Best Regards
Shawn Lin

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

[Doug Anderson]

Hi,

On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>> maybe. But I think 180(downside) is the better.
>
>
> NAK my previous comments here. Downside is better for SRD, but won't
> work for DDR mode. When running in DDR mode, we should use 90 instead.
>
> So let me elaborate a bit more here.
> For DDR mode, one single clk cycle should sending two data bits outside
> to the devices. We need a hold time for both. If 180 is used, the first
> bit occurs around the downside area, which won't be sampled by devices
> on the upside.  So on the upside, the devices will see a zero bit if you
> actually send a one-bit, which makes the devices  generate CRC finally.
>
>
> For this above, 180 for all SDR mode is ok, but 90 should be deployed
> for DDR mode. So simply checking the timing to hardcode it should be
> fine.

OK, I sent out a patch for 180 always.  I can send v2 to use 90 for
DDR modes tomorrow.  ...or feel free to post that yourself if you
want.

We want 90 for all DDR modes?  So MMC_TIMING_UHS_DDR50,
MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400
in dw_mmc on Rockchip).

-Doug

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

[Shawn Lin]

On 2016/5/11 11:50, Doug Anderson wrote:
> Hi,
>
> On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>> maybe. But I think 180(downside) is the better.
>>
>>
>> NAK my previous comments here. Downside is better for SRD, but won't
>> work for DDR mode. When running in DDR mode, we should use 90 instead.
>>
>> So let me elaborate a bit more here.
>> For DDR mode, one single clk cycle should sending two data bits outside
>> to the devices. We need a hold time for both. If 180 is used, the first
>> bit occurs around the downside area, which won't be sampled by devices
>> on the upside.  So on the upside, the devices will see a zero bit if you
>> actually send a one-bit, which makes the devices  generate CRC finally.
>>
>>
>> For this above, 180 for all SDR mode is ok, but 90 should be deployed
>> for DDR mode. So simply checking the timing to hardcode it should be
>> fine.
>
> OK, I sent out a patch for 180 always.  I can send v2 to use 90 for
> DDR modes tomorrow.  ...or feel free to post that yourself if you
> want.
>
> We want 90 for all DDR modes?  So MMC_TIMING_UHS_DDR50,
> MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400
> in dw_mmc on Rockchip).

Right.

>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards
Shawn Lin

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

[Doug Anderson]

Shawn,

On Wed, May 11, 2016 at 1:25 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2016/5/11 11:50, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com>
>> wrote:
>>>>>
>>>>> maybe. But I think 180(downside) is the better.
>>>
>>>
>>>
>>> NAK my previous comments here. Downside is better for SRD, but won't
>>> work for DDR mode. When running in DDR mode, we should use 90 instead.
>>>
>>> So let me elaborate a bit more here.
>>> For DDR mode, one single clk cycle should sending two data bits outside
>>> to the devices. We need a hold time for both. If 180 is used, the first
>>> bit occurs around the downside area, which won't be sampled by devices
>>> on the upside.  So on the upside, the devices will see a zero bit if you
>>> actually send a one-bit, which makes the devices  generate CRC finally.
>>>
>>>
>>> For this above, 180 for all SDR mode is ok, but 90 should be deployed
>>> for DDR mode. So simply checking the timing to hardcode it should be
>>> fine.
>>
>>
>> OK, I sent out a patch for 180 always.  I can send v2 to use 90 for
>> DDR modes tomorrow.  ...or feel free to post that yourself if you
>> want.
>>
>> We want 90 for all DDR modes?  So MMC_TIMING_UHS_DDR50,
>> MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400
>> in dw_mmc on Rockchip).
>
>
> Right.

OK, so I dug into this more.  I don't think it's actually quite that
simple.  The Designware databook explicitly states that 'MMC DDR
8-bit' should use 180, not 90.  ...but that's probably explained by
the fact that "cclk_in" for MMC-DDR 8-bit is double what you'd expect.
You can see that in code:

if (ios->bus_width == MMC_BUS_WIDTH_8 &&
    ios->timing == MMC_TIMING_MMC_DDR52)
      cclkin = 2 * ios->clock * RK3288_CLKGEN_DIV;

---

tl;dr of all the below:

* For SD: use 180 for SDR104 to get margin, use 90 elsewhere.
* For mmc, use 180 for DDR52 to meet timings, use 180 for HS200 to get
margin, use 90 elsewhere


The nice thing about this is, at least for rk3288, things don't change
much from today where we use 90 degrees for most SD cards.

---

For new patch, see: <https://patchwork.kernel.org/patch/9075141/>

---

Details:

So assuming measurements I saw from an EE are correct, measured
"delay_o_ns" is ~ .5 ns for rk3399.  With that, we can do some math.
Please correct any mistakes.  Math is hard.  Note that on all Rockchip
devices I've seen pins are limited to 150 MHz and when we choose 150
MHz we end up at 148.5 MHz.  Calculations aren't too different if we
use 150 for that case.


MMC_TIMING_MMC_DDR52:
  min hold time = 2.5 ns
  min data delay = 2.5 ns + .5 ns = 3 ns
  with 100 MHz clock, 90 degree: 2.5 ns
  with 100 MHz clock, 180 degree: 5.0 ns

  ...need 180 degree

--

SD DDR50:
  min hold time = .8 ns
  min data delay = .8 ns + .5 ns = 1.3 ns
  with 50 MHz clock, 90 degree: 5 ns
  with 50 MHz clock, 180 degree: 10 ns

  ...90 degree is good and 180 is massive overkill.  Use 90.

--

SD SDR104 (limited to 148.5 MHz on existing Rockchip parts):
  min hold time = .8 ns
  min data delay = .8 ns + .5 ns = 1.3 ns
  with 148.5 MHz clock, 90 degree: 1.68 ns
  with 148.5 MHz clock, 180 degree: 3.37 ns

  ...so 90 degrees would probably work OK, but 180 degrees gives more margin.
  ...probably explains why existing rk3288 devices w/ 90 degree work OK.

  if we had 200 MHz clock, 90 degree: 1.25 ns
  if we had 200 MHz clock, 180 degree: 2.50 ns

--

SD SDR50:
  min hold time = .8 ns
  min data delay = .8 ns + .5 ns = 1.3 ns
  with 100 MHz clock, 90 degree: 2.5 ns
  with 100 MHz clock, 180 degree: 5.0 ns

  ...90 degree is great w/ plenty of margin

--

SD SDR25:
  min hold time = 2 ns
  min data delay = 2 ns + .5 ns = 2.5 ns
  with 50 MHz clock, 90 degree: 5 ns
  with 50 MHz clock, 180 degree: 10 ns

 ...90 degree is good and 180 is massive overkill.  Use 90.

--

SD SDR12 (databook shows cclk_in as 50 MHz here we'll get 25 MHz):
  min hold time = 5 ns
  min data delay = 5 ns + .5 ns = 5.5 ns
  with 25 MHz clock, 90 degree: 10 ns
  with 25 MHz clock, 180 degree: 20 ns

  ...90 degree is good (databook suggests 180 for this mode due to cclk_in = 50)

--

ID Mode
  min hold time = 5 ns
  min data delay = 5 ns + .5 ns = 5.5 ns
  with 400 kHz clock, 90 degree: 625 ns
  with 400 kHz clock, 180 degree: 1250 ns

  ...90 degree is good  (databook suggests 180 for this mode due to
cclk_in = 50)

--

MMC High speed:
  min hold time = 2.5 ns
  min data delay = 2.5 ns + .5 ns = 3 ns
  with 50 MHz clock, 90 degree: 5 ns
  with 50 MHz clock, 180 degree: 10 ns

  ...90 degree is good

--

HS200:
  min hold time (tIH from JEDEC spec):  .8 ns
  ...math all works out the same as SDR104.

--

HS400:
  we don't support it anyway

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

[Shawn Lin]

在 2016/5/12 5:42, Doug Anderson 写道:
> Shawn,
>
> On Wed, May 11, 2016 at 1:25 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> On 2016/5/11 11:50, Doug Anderson wrote:
>>>
>>> Hi,
>>>
>>> On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com>
>>> wrote:
>>>>>>
>>>>>> maybe. But I think 180(downside) is the better.
>>>>
>>>>
>>>>
>>>> NAK my previous comments here. Downside is better for SRD, but won't
>>>> work for DDR mode. When running in DDR mode, we should use 90 instead.
>>>>
>>>> So let me elaborate a bit more here.
>>>> For DDR mode, one single clk cycle should sending two data bits outside
>>>> to the devices. We need a hold time for both. If 180 is used, the first
>>>> bit occurs around the downside area, which won't be sampled by devices
>>>> on the upside.  So on the upside, the devices will see a zero bit if you
>>>> actually send a one-bit, which makes the devices  generate CRC finally.
>>>>
>>>>
>>>> For this above, 180 for all SDR mode is ok, but 90 should be deployed
>>>> for DDR mode. So simply checking the timing to hardcode it should be
>>>> fine.
>>>
>>>
>>> OK, I sent out a patch for 180 always.  I can send v2 to use 90 for
>>> DDR modes tomorrow.  ...or feel free to post that yourself if you
>>> want.
>>>
>>> We want 90 for all DDR modes?  So MMC_TIMING_UHS_DDR50,
>>> MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400
>>> in dw_mmc on Rockchip).
>>
>>
>> Right.
>
> OK, so I dug into this more.  I don't think it's actually quite that
> simple.  The Designware databook explicitly states that 'MMC DDR
> 8-bit' should use 180, not 90.  ...but that's probably explained by
> the fact that "cclk_in" for MMC-DDR 8-bit is double what you'd expect.
> You can see that in code:
>
> if (ios->bus_width == MMC_BUS_WIDTH_8 &&
>     ios->timing == MMC_TIMING_MMC_DDR52)
>       cclkin = 2 * ios->clock * RK3288_CLKGEN_DIV;

Yes, this requirment is from dw_mmc databook(v270a), table 10-5.

But you can see that we didn't handle MMC_TIMING_UHS_SDR12.
As recommended, MMC_TIMING_UHS_SDR12 mode need to 50M here to make
the internal devider to be "1".
Because Figure 10-3 (Clock Structure for Host Controller) assumes Soc 
only supplies 50M/100M/200M, these three frequency selection, to 
cclk_in. So SDR12 should be 50M/2 = 25M. But for Rockchip Socs, we have 
RK3288_CLKGEN_DIV before cclk_in that means we don't follow this table.

For DDR52-8bit, it's a mandatory requirment to use 100M as cclk_in.

>
> ---
>
> tl;dr of all the below:
>
> * For SD: use 180 for SDR104 to get margin, use 90 elsewhere.
> * For mmc, use 180 for DDR52 to meet timings, use 180 for HS200 to get
> margin, use 90 elsewhere

yes, see the comments blow.

>
>
> The nice thing about this is, at least for rk3288, things don't change
> much from today where we use 90 degrees for most SD cards.
>
> ---
>
> For new patch, see: <https://patchwork.kernel.org/patch/9075141/>
>
> ---
>
> Details:
>
> So assuming measurements I saw from an EE are correct, measured
> "delay_o_ns" is ~ .5 ns for rk3399.  With that, we can do some math.

Soc specific. But it's a trivial margin.

> Please correct any mistakes.  Math is hard.  Note that on all Rockchip
> devices I've seen pins are limited to 150 MHz and when we choose 150
> MHz we end up at 148.5 MHz.  Calculations aren't too different if we
> use 150 for that case.
>
>
> MMC_TIMING_MMC_DDR52:
>   min hold time = 2.5 ns
>   min data delay = 2.5 ns + .5 ns = 3 ns
>   with 100 MHz clock, 90 degree: 2.5 ns
>   with 100 MHz clock, 180 degree: 5.0 ns
>
>   ...need 180 degree
>

When running in DDR52-8bit, 2 cycle of cclk_in is used
for a single data bit internally. When shifting cclk_in_drv to 180,
we can see:

cclk_in    ____      ____      ____      ____      ___
	__|    |____|    |____|    |____|    |____|

cclk_in_drv-180
                 ____      ____      ____      ____      ___
	     __|    |____|    |____|    |____|    |____|

cclk_in_for_DDR52-8bit
            _________           _____
         __|         |_________|



So cclk_in_drv shifts 180 from cclk_in, which means it's just in the
90 degrees of cclk_in_for_DDR52-8bit. That is what I'd expect that when
running in DDR mode, the cclk_in_drv should be in the 90 degree of
*Actual Internal CLK*, not cclk_in always.

But fot SD DDR50-4bit, it just use cclk_in internlly, so 90 should
be okay.

So we were not in the same page of what the ref-clk is. But I think now
we are in.

Agree on all the calculation below.

> --
>
> SD DDR50:
>   min hold time = .8 ns
>   min data delay = .8 ns + .5 ns = 1.3 ns
>   with 50 MHz clock, 90 degree: 5 ns
>   with 50 MHz clock, 180 degree: 10 ns
>
>   ...90 degree is good and 180 is massive overkill.  Use 90.
>
> --
>
> SD SDR104 (limited to 148.5 MHz on existing Rockchip parts):
>   min hold time = .8 ns
>   min data delay = .8 ns + .5 ns = 1.3 ns
>   with 148.5 MHz clock, 90 degree: 1.68 ns
>   with 148.5 MHz clock, 180 degree: 3.37 ns
>
>   ...so 90 degrees would probably work OK, but 180 degrees gives more margin.
>   ...probably explains why existing rk3288 devices w/ 90 degree work OK.
>
>   if we had 200 MHz clock, 90 degree: 1.25 ns
>   if we had 200 MHz clock, 180 degree: 2.50 ns
>
> --
>
> SD SDR50:
>   min hold time = .8 ns
>   min data delay = .8 ns + .5 ns = 1.3 ns
>   with 100 MHz clock, 90 degree: 2.5 ns
>   with 100 MHz clock, 180 degree: 5.0 ns
>
>   ...90 degree is great w/ plenty of margin
>
> --
>
> SD SDR25:
>   min hold time = 2 ns
>   min data delay = 2 ns + .5 ns = 2.5 ns
>   with 50 MHz clock, 90 degree: 5 ns
>   with 50 MHz clock, 180 degree: 10 ns
>
>  ...90 degree is good and 180 is massive overkill.  Use 90.
>
> --
>
> SD SDR12 (databook shows cclk_in as 50 MHz here we'll get 25 MHz):
>   min hold time = 5 ns
>   min data delay = 5 ns + .5 ns = 5.5 ns
>   with 25 MHz clock, 90 degree: 10 ns
>   with 25 MHz clock, 180 degree: 20 ns
>
>   ...90 degree is good (databook suggests 180 for this mode due to cclk_in = 50)
>
> --
>
> ID Mode
>   min hold time = 5 ns
>   min data delay = 5 ns + .5 ns = 5.5 ns
>   with 400 kHz clock, 90 degree: 625 ns
>   with 400 kHz clock, 180 degree: 1250 ns
>
>   ...90 degree is good  (databook suggests 180 for this mode due to
> cclk_in = 50)
>
> --
>
> MMC High speed:
>   min hold time = 2.5 ns
>   min data delay = 2.5 ns + .5 ns = 3 ns
>   with 50 MHz clock, 90 degree: 5 ns
>   with 50 MHz clock, 180 degree: 10 ns
>
>   ...90 degree is good
>
> --
>
> HS200:
>   min hold time (tIH from JEDEC spec):  .8 ns
>   ...math all works out the same as SDR104.
>
> --
>
> HS400:
>   we don't support it anyway
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>