[PATCH/RFT] mmc: meson-gx: include tx phase in the tuning process

It has been reported that some platforms (odroid-c2) may require
a different tx phase setting to operate at high speed.

To improve the situation, this patch includes tx phase in the tuning
process.

Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

---

Hi Heiner,

Would you mind trying this change with your setup and letting us know
if it helps ?

It seems like a good idea to add an initial Rx tuning before doing the
actual tuning but, honestly, I don't know if it is really necessary.

Regards

Jerome


 drivers/mmc/host/meson-gx-mmc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.13.5

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

Am 18.09.2017 um 15:44 schrieb Jerome Brunet:
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c

> index c885c2d4b904..0254d8bfd536 100644

> --- a/drivers/mmc/host/meson-gx-mmc.c

> +++ b/drivers/mmc/host/meson-gx-mmc.c

> @@ -717,6 +717,22 @@ static int meson_mmc_clk_phase_tuning(struct mmc_host *mmc, u32 opcode,

>  static int meson_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)

>  {

>  	struct meson_host *host = mmc_priv(mmc);

> +	int ret;

> +

> +	/*

> +	 * If this is the initial tuning, try to get a sane Rx starting

> +	 * phase before doing the actual tuning.

> +	 */

> +	if (!mmc->doing_retune) {

> +		ret = meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);

> +

> +		if (ret)

> +			return ret;

> +	}

> +

> +	ret = meson_mmc_clk_phase_tuning(mmc, opcode, host->tx_clk);

> +	if (ret)

> +		return ret;

>  

>  	return meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);

>  }

> -- 2.13.5

Unfortunately this still doesn't fix the issue here.
Tuning rx and tx clk sequentially assumes both are independent, what they
IMHO are not. So meybe we have to check all combinations of rx/tx clk phase.

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

On Mon, 2017-09-18 at 21:11 +0200, Heiner Kallweit wrote:
> Am 18.09.2017 um 15:44 schrieb Jerome Brunet:

> > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-

> > mmc.c

> > index c885c2d4b904..0254d8bfd536 100644

> > --- a/drivers/mmc/host/meson-gx-mmc.c

> > +++ b/drivers/mmc/host/meson-gx-mmc.c

> > @@ -717,6 +717,22 @@ static int meson_mmc_clk_phase_tuning(struct mmc_host

> > *mmc, u32 opcode,

> >  static int meson_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)

> >  {

> >  	struct meson_host *host = mmc_priv(mmc);

> > +	int ret;

> > +

> > +	/*

> > +	 * If this is the initial tuning, try to get a sane Rx starting

> > +	 * phase before doing the actual tuning.

> > +	 */

> > +	if (!mmc->doing_retune) {

> > +		ret = meson_mmc_clk_phase_tuning(mmc, opcode, host-

> > >rx_clk);

> > +

> > +		if (ret)

> > +			return ret;

> > +	}

> > +

> > +	ret = meson_mmc_clk_phase_tuning(mmc, opcode, host->tx_clk);

> > +	if (ret)

> > +		return ret;

> >  

> >  	return meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);

> >  }

> > -- 2.13.5

> 

> Unfortunately this still doesn't fix the issue here.

> Tuning rx and tx clk sequentially assumes both are independent, what they

> IMHO are not. So meybe we have to check all combinations of rx/tx clk phase.


Interesting, I would be curious to know what tuning value you ended with,
compared to the "hard-coded' working value you have set.

You can get that fairly easily now, using CCF in debugfs, in
<debugfs>/clk/clk_summary, the different phase are reported

What makes you think that tx and rx phase depends on one another ?
I have done a lot of tests on all the phase different settings while working on
this series and could see that:

1) For a fixed Tx and Core phase, Rx phase tuning tends to give a constant
result
2) For a fixed Tx, Rx phase tuning tends to rotate with the core phase
3) For a fixed Core phase, Rx phase tuning tends to remain constant for any
values of Tx phase

From what I understand of the HW, this would make sense:
* Tx phase would be the phase at which the data are sent compared to the core
clock
* Rx phase would be the phase at which the data are sampled compared to the core
clock

This would make me conclude that both Tx and Rx phases depends on the core phase
but are independent of one another. Of course, if you have evidence showing
otherwise, I'm happy to reconsider.
ATM, I don't see the added value of testing all the combination.

Another thing to consider is that, with the current driver, we set the Tx and Rx
with a precision of 30 degrees -> 12 possible phase settings.

* 2 sequential tuning => 24 test
* all combination of 2 phases => 144 test

Also, remember that this tuning is as much based on the working tuning point as
it is on the failing ones. I looks for the center of the tuning window, so
failing tests are very valuable. Looking for all the combination, you would have
you look for this "center" in 2D ... not impossible, but complex and annoying. 
  

> 


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

Am 19.09.2017 um 13:08 schrieb Jerome Brunet:
> On Mon, 2017-09-18 at 21:11 +0200, Heiner Kallweit wrote:

>> Am 18.09.2017 um 15:44 schrieb Jerome Brunet:

>>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-

>>> mmc.c

>>> index c885c2d4b904..0254d8bfd536 100644

>>> --- a/drivers/mmc/host/meson-gx-mmc.c

>>> +++ b/drivers/mmc/host/meson-gx-mmc.c

>>> @@ -717,6 +717,22 @@ static int meson_mmc_clk_phase_tuning(struct mmc_host

>>> *mmc, u32 opcode,

>>>  static int meson_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)

>>>  {

>>>  	struct meson_host *host = mmc_priv(mmc);

>>> +	int ret;

>>> +

>>> +	/*

>>> +	 * If this is the initial tuning, try to get a sane Rx starting

>>> +	 * phase before doing the actual tuning.

>>> +	 */

>>> +	if (!mmc->doing_retune) {

>>> +		ret = meson_mmc_clk_phase_tuning(mmc, opcode, host-

>>>> rx_clk);

>>> +

>>> +		if (ret)

>>> +			return ret;

>>> +	}

>>> +

>>> +	ret = meson_mmc_clk_phase_tuning(mmc, opcode, host->tx_clk);

>>> +	if (ret)

>>> +		return ret;

>>>  

>>>  	return meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);

>>>  }

>>> -- 2.13.5

>>

>> Unfortunately this still doesn't fix the issue here.

>> Tuning rx and tx clk sequentially assumes both are independent, what they

>> IMHO are not. So meybe we have to check all combinations of rx/tx clk phase.

> 

> Interesting, I would be curious to know what tuning value you ended with,

> compared to the "hard-coded' working value you have set.

> 

> You can get that fairly easily now, using CCF in debugfs, in

> <debugfs>/clk/clk_summary, the different phase are reported

> 

This gives me:

core phase: 180
rx phase: 0
tx phase: 270

And I end up with a corrupted root file system.

> What makes you think that tx and rx phase depends on one another ?

> I have done a lot of tests on all the phase different settings while working on

> this series and could see that:

> 

> 1) For a fixed Tx and Core phase, Rx phase tuning tends to give a constant

> result

> 2) For a fixed Tx, Rx phase tuning tends to rotate with the core phase

> 3) For a fixed Core phase, Rx phase tuning tends to remain constant for any

> values of Tx phase

> 

>>From what I understand of the HW, this would make sense:

> * Tx phase would be the phase at which the data are sent compared to the core

> clock

> * Rx phase would be the phase at which the data are sampled compared to the core

> clock

> 

> This would make me conclude that both Tx and Rx phases depends on the core phase

> but are independent of one another. Of course, if you have evidence showing

> otherwise, I'm happy to reconsider.

> ATM, I don't see the added value of testing all the combination.

> 

> Another thing to consider is that, with the current driver, we set the Tx and Rx

> with a precision of 30 degrees -> 12 possible phase settings.

> 

> * 2 sequential tuning => 24 test

> * all combination of 2 phases => 144 test

> 

> Also, remember that this tuning is as much based on the working tuning point as

> it is on the failing ones. I looks for the center of the tuning window, so

> failing tests are very valuable. Looking for all the combination, you would have

> you look for this "center" in 2D ... not impossible, but complex and annoying. 

>   

> 

>>

> 

> 


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

On Tue, 2017-09-19 at 20:54 +0200, Heiner Kallweit wrote:
> > > Unfortunately this still doesn't fix the issue here.

> > > Tuning rx and tx clk sequentially assumes both are independent, what they

> > > IMHO are not. So meybe we have to check all combinations of rx/tx clk

> > > phase.

> > 

> > Interesting, I would be curious to know what tuning value you ended with,

> > compared to the "hard-coded' working value you have set.

> > 

> > You can get that fairly easily now, using CCF in debugfs, in

> > <debugfs>/clk/clk_summary, the different phase are reported

> > 

> 

> This gives me:

> 

> core phase: 180

> rx phase: 0

> tx phase: 270

> 

> And I end up with a corrupted root file system.


You should have had tuning on both the Rx and Tx phase and yet, you end up with
the default values ... that's strange

I should be able to get my hands on 16GB emmc module for this platform soon.
Let's see ...
--
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

On Wed, 2017-09-27 at 18:24 +0200, Jerome Brunet wrote:
> On Tue, 2017-09-19 at 20:54 +0200, Heiner Kallweit wrote:

> > > > Unfortunately this still doesn't fix the issue here.

> > > > Tuning rx and tx clk sequentially assumes both are independent, what

> > > > they

> > > > IMHO are not. So meybe we have to check all combinations of rx/tx clk

> > > > phase.

> > > 

> > > Interesting, I would be curious to know what tuning value you ended with,

> > > compared to the "hard-coded' working value you have set.

> > > 

> > > You can get that fairly easily now, using CCF in debugfs, in

> > > <debugfs>/clk/clk_summary, the different phase are reported

> > > 

> > 

> > This gives me:

> > 

> > core phase: 180

> > rx phase: 0

> > tx phase: 270

> > 

> > And I end up with a corrupted root file system.

> 

> You should have had tuning on both the Rx and Tx phase and yet, you end up

> with

> the default values ... that's strange

> 

> I should be able to get my hands on 16GB emmc module for this platform soon.

> Let's see ...



So, I did some tests on the odroidc2 with the 16GB emmc module.

1) With Mainline + DTS patches (Waiting in Olof late branch) + the patch
proposed here:

mmc0: new HS200 MMC card at address 0001
mmcblk0: mmc0:0001 SDW16G 14.7 GiB
mmcblk0boot0: mmc0:0001 SDW16G partition 1 4.00 MiB
mmcblk0boot1: mmc0:0001 SDW16G partition 2 4.00 MiB
mmcblk0rpmb: mmc0:0001 SDW16G partition 3 4.00 MiB
mmcblk0: p1 p2 p3 p4

and clk_summary

d0074000.mmc#mux     1            1  1000000000          0 0
 d0074000.mmc#div    1            1   200000000          0 0
  d0074000.mmc#core  1            1   200000000          0 180
  d0074000.mmc#rx    0            0   200000000          0 120
  d0074000.mmc#tx    0            0   200000000          0 0

Note the tx phase tuning ended-up with the value you expected. I've done read
tests with this and there was no issue.

This made no sense with the value you reported but I thought maybe you did your
test in hs400 ... so I tested and it seems to be the case. In this mode, the
tuning value got reset.

This is because I mistakenly place the phase reset in POWER_ON instead of
POWER_UP.
The phase get reset every time the set_ios() is called, which is not what we
want.

With a few fix applied here, this is what I get:

# cat /sys/kernel/debug/mmc0/ios       
clock:          200000000 Hz           
actual clock:   166666667 Hz           
vdd:            21 (3.3 ~ 3.4 V)       
bus mode:       2 (push-pull)          
chip select:    0 (don't care)         
power mode:     2 (on)                 
bus width:      3 (8 bits)             
timing spec:    10 (mmc HS400)         
signal voltage: 1 (1.80 V)             
driver type:    0 (driver type B)      

clk_summary:
d0074000.mmc#mux            1            1  1000000000          0 0
 d0074000.mmc#div           1            1   333333334          0 0
  d0074000.mmc#core         1            1   333333334          0 180
  d0074000.mmc#rx           0            0   333333334          0 120
  d0074000.mmc#tx           0            0   333333334          0 0

No errors reported while doing read test using dd. Read throughput appears to be
around 220MB/s with this module.

I have posted a series with the fixes here:
https://lkml.kernel.org/r/20171002122743.32334-1-jbrunet@baylibre.com

Jerome.


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