[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Douglas Anderson]

This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
before mmc card initialization").

Though not totally obvious from the commit message nor from the source
code, that commit appears to be trying to reset the "_drv" MMC clocks to
90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
not touched).

The major problem here is that it doesn't properly reset things.  The
phase is a two bit field and the commit only touches one of the two
bits.  Thus the commit had the following affect:
- phase   0  => phase  90
- phase  90  => phase  90
- phase 180  => phase 270
- phase 270  => phase 270

Things get even weirder if you happen to have a bootloader that was
actually using delay elements (should be no reason to, but you never
know), since those are additional bits that weren't touched by the
original patch.

This is unlikely to be what we actually want.  Checking on rk3288-veyron
devices, I can see that the bootloader leaves these clocks as:
- emmc:  phase 180
- sdmmc: phase 90
- sdio0: phase 90

Thus on rk3288-veyron devices the commit we're reverting had the effect
of changing the eMMC clock to phase 270.  This probably explains the
scattered reports I've heard of eMMC devices not working on some veyron
devices when using the upstream kernel.

The original commit was presumably made because previously the kernel
didn't touch the "_drv" phase at all and relied on whatever value was
there when the kernel started.  If someone was using a bootloader that
touched the "_drv" phase then, indeed, we should have code in the kernel
to fix that.  ...and also, to get ideal timings, we should also have the
kernel change the phase depending on the speed mode.  In fact, that's
the subject of a recent patch I posted at
<https://patchwork.kernel.org/patch/9075141/>.

Ideally, we should take both the patch posted to dw_mmc and this
revert.  Since those will likely go through different trees, here I
describe behavior with the combos:

1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
   + other cases; might break someone with a strange bootloader that
   sets the phase to 0 or one that uses delay elements (pretty
   unpredicable what would happen in that case).
2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
   totally override the broken patch and fix everything.
3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
   any defaults from the clock code doesn't mattery.

Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index bc856f21f6b2..5b18265c2306 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
 #define ROCKCHIP_MMC_DEGREE_MASK 0x3
 #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
 #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
-#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
-#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
 
 #define PSECS_PER_SEC 1000000000000LL
 
@@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	mmc_clock->reg = reg;
 	mmc_clock->shift = shift;
 
-	/*
-	 * Assert init_state to soft reset the CLKGEN
-	 * for mmc tuning phase and degree
-	 */
-	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
-		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
-				     ROCKCHIP_MMC_INIT_STATE_RESET,
-				     mmc_clock->shift), mmc_clock->reg);
-
 	clk = clk_register(NULL, &mmc_clock->hw);
 	if (IS_ERR(clk))
 		kfree(mmc_clock);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Douglas Anderson]

This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
before mmc card initialization").

Though not totally obvious from the commit message nor from the source
code, that commit appears to be trying to reset the "_drv" MMC clocks to
90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
not touched).

The major problem here is that it doesn't properly reset things.  The
phase is a two bit field and the commit only touches one of the two
bits.  Thus the commit had the following affect:
- phase   0  => phase  90
- phase  90  => phase  90
- phase 180  => phase 270
- phase 270  => phase 270

Things get even weirder if you happen to have a bootloader that was
actually using delay elements (should be no reason to, but you never
know), since those are additional bits that weren't touched by the
original patch.

This is unlikely to be what we actually want.  Checking on rk3288-veyron
devices, I can see that the bootloader leaves these clocks as:
- emmc:  phase 180
- sdmmc: phase 90
- sdio0: phase 90

Thus on rk3288-veyron devices the commit we're reverting had the effect
of changing the eMMC clock to phase 270.  This probably explains the
scattered reports I've heard of eMMC devices not working on some veyron
devices when using the upstream kernel.

The original commit was presumably made because previously the kernel
didn't touch the "_drv" phase at all and relied on whatever value was
there when the kernel started.  If someone was using a bootloader that
touched the "_drv" phase then, indeed, we should have code in the kernel
to fix that.  ...and also, to get ideal timings, we should also have the
kernel change the phase depending on the speed mode.  In fact, that's
the subject of a recent patch I posted at
<https://patchwork.kernel.org/patch/9075141/>.

Ideally, we should take both the patch posted to dw_mmc and this
revert.  Since those will likely go through different trees, here I
describe behavior with the combos:

1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
   + other cases; might break someone with a strange bootloader that
   sets the phase to 0 or one that uses delay elements (pretty
   unpredicable what would happen in that case).
2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
   totally override the broken patch and fix everything.
3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
   any defaults from the clock code doesn't mattery.

Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index bc856f21f6b2..5b18265c2306 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
 #define ROCKCHIP_MMC_DEGREE_MASK 0x3
 #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
 #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
-#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
-#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
 
 #define PSECS_PER_SEC 1000000000000LL
 
@@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	mmc_clock->reg = reg;
 	mmc_clock->shift = shift;
 
-	/*
-	 * Assert init_state to soft reset the CLKGEN
-	 * for mmc tuning phase and degree
-	 */
-	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
-		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
-				     ROCKCHIP_MMC_INIT_STATE_RESET,
-				     mmc_clock->shift), mmc_clock->reg);
-
 	clk = clk_register(NULL, &mmc_clock->hw);
 	if (IS_ERR(clk))
 		kfree(mmc_clock);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Douglas Anderson]

Just like every other Rockhip device, the MMC "_sample" clocks should
have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
these values were set to 0 because of a typo.

Things _sorta_ would have worked with the incorrect sample phase shift
because of the register layout but wouldn't have been ideal and we would
have skipped lots of phases.  Also: we would never actually enabled the
fine delay unless we happened to have 128 or more delay elements.

This is expected behavior before this patch:
* Try to set:     0 degrees +   1 delay elements
  Actually get:   0 degrees +   0 delay elements
* Try to set:    90 degrees +   0 delay elements
  Actually get: 180 degrees +   0 delay elements
* Try to set:   180 degrees +   0 delay elements
  Actually get:   0 degrees +   0 delay elements
* Try to set:   270 degrees +   0 delay elements
  Actually get: 180 degrees +   0 delay elements
* Try to set:     0 degrees + 129 delay elements
  Actually get:   0 degrees +   2 delay elements
* Try to set:   180 degrees + 129 delay elements
  Actually get:   0 degrees +   3 delay elements
* Try to set:     0 degrees + 130 delay elements
  Actually get:   0 degrees +   4 delay elements

I verified that old code had a problem by turning on debug printouts and
seeing that the old code would report this for one SD card I had:
  Good phase range 347-101 (115 len)
  Good phase range 202-326 (125 len)

After my fix, it went down to one big good range for the same card.
This is more expected:
  Good phase range 189-1 (173 len)
  Good phase range 82-85 (4 len)
  Good phase range 166-168 (3 len)

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 291543f52caa..14ff3e109e1e 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(6), 1, GFLAGS),
 
 	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
-	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
+	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
 
 	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
-	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
+	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
 
 	/* pcie */
 	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Douglas Anderson]

Just like every other Rockhip device, the MMC "_sample" clocks should
have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
these values were set to 0 because of a typo.

Things _sorta_ would have worked with the incorrect sample phase shift
because of the register layout but wouldn't have been ideal and we would
have skipped lots of phases.  Also: we would never actually enabled the
fine delay unless we happened to have 128 or more delay elements.

This is expected behavior before this patch:
* Try to set:     0 degrees +   1 delay elements
  Actually get:   0 degrees +   0 delay elements
* Try to set:    90 degrees +   0 delay elements
  Actually get: 180 degrees +   0 delay elements
* Try to set:   180 degrees +   0 delay elements
  Actually get:   0 degrees +   0 delay elements
* Try to set:   270 degrees +   0 delay elements
  Actually get: 180 degrees +   0 delay elements
* Try to set:     0 degrees + 129 delay elements
  Actually get:   0 degrees +   2 delay elements
* Try to set:   180 degrees + 129 delay elements
  Actually get:   0 degrees +   3 delay elements
* Try to set:     0 degrees + 130 delay elements
  Actually get:   0 degrees +   4 delay elements

I verified that old code had a problem by turning on debug printouts and
seeing that the old code would report this for one SD card I had:
  Good phase range 347-101 (115 len)
  Good phase range 202-326 (125 len)

After my fix, it went down to one big good range for the same card.
This is more expected:
  Good phase range 189-1 (173 len)
  Good phase range 82-85 (4 len)
  Good phase range 166-168 (3 len)

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 291543f52caa..14ff3e109e1e 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(6), 1, GFLAGS),
 
 	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
-	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
+	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
 
 	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
-	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
+	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
 
 	/* pcie */
 	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Brian Norris]

On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
> Just like every other Rockhip device, the MMC "_sample" clocks should
> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
> these values were set to 0 because of a typo.

I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
itself, so it sometimes agrees with you and sometimes doesn't :)

On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
are {drv,sample}_degree. But on page 208 of the 1st book, those are put
at bits [1:0].

Perhaps we can get a straight answer from Rockchip though.

Brian

> Things _sorta_ would have worked with the incorrect sample phase shift
> because of the register layout but wouldn't have been ideal and we would
> have skipped lots of phases.  Also: we would never actually enabled the
> fine delay unless we happened to have 128 or more delay elements.
> 
> This is expected behavior before this patch:
> * Try to set:     0 degrees +   1 delay elements
>   Actually get:   0 degrees +   0 delay elements
> * Try to set:    90 degrees +   0 delay elements
>   Actually get: 180 degrees +   0 delay elements
> * Try to set:   180 degrees +   0 delay elements
>   Actually get:   0 degrees +   0 delay elements
> * Try to set:   270 degrees +   0 delay elements
>   Actually get: 180 degrees +   0 delay elements
> * Try to set:     0 degrees + 129 delay elements
>   Actually get:   0 degrees +   2 delay elements
> * Try to set:   180 degrees + 129 delay elements
>   Actually get:   0 degrees +   3 delay elements
> * Try to set:     0 degrees + 130 delay elements
>   Actually get:   0 degrees +   4 delay elements
> 
> I verified that old code had a problem by turning on debug printouts and
> seeing that the old code would report this for one SD card I had:
>   Good phase range 347-101 (115 len)
>   Good phase range 202-326 (125 len)
> 
> After my fix, it went down to one big good range for the same card.
> This is more expected:
>   Good phase range 189-1 (173 len)
>   Good phase range 82-85 (4 len)
>   Good phase range 166-168 (3 len)
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 291543f52caa..14ff3e109e1e 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>  			RK3399_CLKGATE_CON(6), 1, GFLAGS),
>  
>  	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
> -	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
> +	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
>  
>  	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
> -	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
> +	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
>  
>  	/* pcie */
>  	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Brian Norris]

On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
> Just like every other Rockhip device, the MMC "_sample" clocks should
> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
> these values were set to 0 because of a typo.

I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
itself, so it sometimes agrees with you and sometimes doesn't :)

On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
are {drv,sample}_degree. But on page 208 of the 1st book, those are put
at bits [1:0].

Perhaps we can get a straight answer from Rockchip though.

Brian

> Things _sorta_ would have worked with the incorrect sample phase shift
> because of the register layout but wouldn't have been ideal and we would
> have skipped lots of phases.  Also: we would never actually enabled the
> fine delay unless we happened to have 128 or more delay elements.
> 
> This is expected behavior before this patch:
> * Try to set:     0 degrees +   1 delay elements
>   Actually get:   0 degrees +   0 delay elements
> * Try to set:    90 degrees +   0 delay elements
>   Actually get: 180 degrees +   0 delay elements
> * Try to set:   180 degrees +   0 delay elements
>   Actually get:   0 degrees +   0 delay elements
> * Try to set:   270 degrees +   0 delay elements
>   Actually get: 180 degrees +   0 delay elements
> * Try to set:     0 degrees + 129 delay elements
>   Actually get:   0 degrees +   2 delay elements
> * Try to set:   180 degrees + 129 delay elements
>   Actually get:   0 degrees +   3 delay elements
> * Try to set:     0 degrees + 130 delay elements
>   Actually get:   0 degrees +   4 delay elements
> 
> I verified that old code had a problem by turning on debug printouts and
> seeing that the old code would report this for one SD card I had:
>   Good phase range 347-101 (115 len)
>   Good phase range 202-326 (125 len)
> 
> After my fix, it went down to one big good range for the same card.
> This is more expected:
>   Good phase range 189-1 (173 len)
>   Good phase range 82-85 (4 len)
>   Good phase range 166-168 (3 len)
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 291543f52caa..14ff3e109e1e 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>  			RK3399_CLKGATE_CON(6), 1, GFLAGS),
>  
>  	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
> -	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
> +	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
>  
>  	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
> -	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
> +	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
>  
>  	/* pcie */
>  	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Heiko Stuebner]

Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:

Not sure what the policy is for revert-subjects in the clock-tree, but I 
guess

clk: rockchip: Revert "reset init state before mmc card initialization"

might look nicer?

> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").

I've tested these two patches together with the dw_mmc one and everything 
that worked before still works (tuning on veyron-jerry) and everything that 
didn't work still doesn't work (tuning on firefly [likely a regulator issue]).

And re-checking the code against the TRM I now also see the mask that is to 
short (1 bit in the code vs. the actual 2 bits described in the manual)

[...]

> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.

I'm still trying to make up my mind on how to proceed - aka in which order 
patches should get picked up.

Going with the "if it isn't upstream it doesn't exist" mantra Rob wrote 
somewhere some days ago, taking this revert should not hurt anything, as all 
the non-veyron devices currently don't support any tuning at all.

So I'd think except for the issue Shawn was originally trying to fix (for 
some non-mainline board?) it should be possible to take this patch any time 
independently of the dw_mmc part.

If Stephen or Mike want to do this directly, this is
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card
> initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Heiko Stuebner]

Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:

Not sure what the policy is for revert-subjects in the clock-tree, but I 
guess

clk: rockchip: Revert "reset init state before mmc card initialization"

might look nicer?

> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").

I've tested these two patches together with the dw_mmc one and everything 
that worked before still works (tuning on veyron-jerry) and everything that 
didn't work still doesn't work (tuning on firefly [likely a regulator issue]).

And re-checking the code against the TRM I now also see the mask that is to 
short (1 bit in the code vs. the actual 2 bits described in the manual)

[...]

> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.

I'm still trying to make up my mind on how to proceed - aka in which order 
patches should get picked up.

Going with the "if it isn't upstream it doesn't exist" mantra Rob wrote 
somewhere some days ago, taking this revert should not hurt anything, as all 
the non-veyron devices currently don't support any tuning at all.

So I'd think except for the issue Shawn was originally trying to fix (for 
some non-mainline board?) it should be possible to take this patch any time 
independently of the dw_mmc part.

If Stephen or Mike want to do this directly, this is
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card
> initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Shawn Lin]

在 2016/5/13 7:10, Brian Norris 写道:
> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>> Just like every other Rockhip device, the MMC "_sample" clocks should
>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>> these values were set to 0 because of a typo.
>
> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
> itself, so it sometimes agrees with you and sometimes doesn't :)
>
> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
> at bits [1:0].
>


Please refer to Mobile Strorage Host Controller section for anything
about sdmmc/sdio. So shift should be 1.

Sometime I also get bothered to address it. Anyway, I will always keep
a eye on it from now on.....

> Perhaps we can get a straight answer from Rockchip though.
>
> Brian
>
>> Things _sorta_ would have worked with the incorrect sample phase shift
>> because of the register layout but wouldn't have been ideal and we would
>> have skipped lots of phases.  Also: we would never actually enabled the
>> fine delay unless we happened to have 128 or more delay elements.
>>
>> This is expected behavior before this patch:
>> * Try to set:     0 degrees +   1 delay elements
>>   Actually get:   0 degrees +   0 delay elements
>> * Try to set:    90 degrees +   0 delay elements
>>   Actually get: 180 degrees +   0 delay elements
>> * Try to set:   180 degrees +   0 delay elements
>>   Actually get:   0 degrees +   0 delay elements
>> * Try to set:   270 degrees +   0 delay elements
>>   Actually get: 180 degrees +   0 delay elements
>> * Try to set:     0 degrees + 129 delay elements
>>   Actually get:   0 degrees +   2 delay elements
>> * Try to set:   180 degrees + 129 delay elements
>>   Actually get:   0 degrees +   3 delay elements
>> * Try to set:     0 degrees + 130 delay elements
>>   Actually get:   0 degrees +   4 delay elements
>>
>> I verified that old code had a problem by turning on debug printouts and
>> seeing that the old code would report this for one SD card I had:
>>   Good phase range 347-101 (115 len)
>>   Good phase range 202-326 (125 len)
>>
>> After my fix, it went down to one big good range for the same card.
>> This is more expected:
>>   Good phase range 189-1 (173 len)
>>   Good phase range 82-85 (4 len)
>>   Good phase range 166-168 (3 len)
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>> index 291543f52caa..14ff3e109e1e 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>>  			RK3399_CLKGATE_CON(6), 1, GFLAGS),
>>
>>  	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
>> -	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
>> +	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
>>
>>  	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
>> -	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
>> +	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
>>
>>  	/* pcie */
>>  	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,
>
>
>


-- 
Best Regards
Shawn Lin

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Shawn Lin]

? 2016/5/13 7:10, Brian Norris ??:
> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>> Just like every other Rockhip device, the MMC "_sample" clocks should
>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>> these values were set to 0 because of a typo.
>
> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
> itself, so it sometimes agrees with you and sometimes doesn't :)
>
> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
> at bits [1:0].
>


Please refer to Mobile Strorage Host Controller section for anything
about sdmmc/sdio. So shift should be 1.

Sometime I also get bothered to address it. Anyway, I will always keep
a eye on it from now on.....

> Perhaps we can get a straight answer from Rockchip though.
>
> Brian
>
>> Things _sorta_ would have worked with the incorrect sample phase shift
>> because of the register layout but wouldn't have been ideal and we would
>> have skipped lots of phases.  Also: we would never actually enabled the
>> fine delay unless we happened to have 128 or more delay elements.
>>
>> This is expected behavior before this patch:
>> * Try to set:     0 degrees +   1 delay elements
>>   Actually get:   0 degrees +   0 delay elements
>> * Try to set:    90 degrees +   0 delay elements
>>   Actually get: 180 degrees +   0 delay elements
>> * Try to set:   180 degrees +   0 delay elements
>>   Actually get:   0 degrees +   0 delay elements
>> * Try to set:   270 degrees +   0 delay elements
>>   Actually get: 180 degrees +   0 delay elements
>> * Try to set:     0 degrees + 129 delay elements
>>   Actually get:   0 degrees +   2 delay elements
>> * Try to set:   180 degrees + 129 delay elements
>>   Actually get:   0 degrees +   3 delay elements
>> * Try to set:     0 degrees + 130 delay elements
>>   Actually get:   0 degrees +   4 delay elements
>>
>> I verified that old code had a problem by turning on debug printouts and
>> seeing that the old code would report this for one SD card I had:
>>   Good phase range 347-101 (115 len)
>>   Good phase range 202-326 (125 len)
>>
>> After my fix, it went down to one big good range for the same card.
>> This is more expected:
>>   Good phase range 189-1 (173 len)
>>   Good phase range 82-85 (4 len)
>>   Good phase range 166-168 (3 len)
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>> index 291543f52caa..14ff3e109e1e 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -895,10 +895,10 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>>  			RK3399_CLKGATE_CON(6), 1, GFLAGS),
>>
>>  	MMC(SCLK_SDMMC_DRV,     "sdmmc_drv",    "clk_sdmmc", RK3399_SDMMC_CON0, 1),
>> -	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 1),
>> +	MMC(SCLK_SDMMC_SAMPLE,  "sdmmc_sample", "clk_sdmmc", RK3399_SDMMC_CON1, 0),
>>
>>  	MMC(SCLK_SDIO_DRV,      "sdio_drv",    "clk_sdio",  RK3399_SDIO_CON0,  1),
>> -	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  1),
>> +	MMC(SCLK_SDIO_SAMPLE,   "sdio_sample", "clk_sdio",  RK3399_SDIO_CON1,  0),
>>
>>  	/* pcie */
>>  	COMPOSITE(SCLK_PCIE_PM, "clk_pcie_pm", mux_pll_src_cpll_gpll_npll_24m_p, 0,
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Shawn Lin]

On 2016/5/13 2:03, Douglas Anderson wrote:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
>
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).

Hrrr...my fault, anyway it should be reverted in case of new register
layout in the future.

Have you found the difference of sample shift between rk3399 and former
Socs?  Per Mobile Storage Host Controller section, it migrate from
bit[1:0] to bit[2:1]...

Acutally there are three register layouts about sdmmc/sdio tuning
stuff I found. Although just two types were upstreamed, it still
caused difficulty. With your patch for dw_mmc to explictly assign
dr_phase, we keep the phase policy better to be unstood and traced.

Again sorry for commit 7a03fe6f48!

>
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
>
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
>
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
>
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
>
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
>
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
>
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.
>
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
> index bc856f21f6b2..5b18265c2306 100644
> --- a/drivers/clk/rockchip/clk-mmc-phase.c
> +++ b/drivers/clk/rockchip/clk-mmc-phase.c
> @@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
>  #define ROCKCHIP_MMC_DEGREE_MASK 0x3
>  #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
>  #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
> -#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
> -#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
>
>  #define PSECS_PER_SEC 1000000000000LL
>
> @@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	mmc_clock->reg = reg;
>  	mmc_clock->shift = shift;
>
> -	/*
> -	 * Assert init_state to soft reset the CLKGEN
> -	 * for mmc tuning phase and degree
> -	 */
> -	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
> -		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     mmc_clock->shift), mmc_clock->reg);
> -
>  	clk = clk_register(NULL, &mmc_clock->hw);
>  	if (IS_ERR(clk))
>  		kfree(mmc_clock);
>


-- 
Best Regards
Shawn Lin

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Shawn Lin]

On 2016/5/13 2:03, Douglas Anderson wrote:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
>
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).

Hrrr...my fault, anyway it should be reverted in case of new register
layout in the future.

Have you found the difference of sample shift between rk3399 and former
Socs?  Per Mobile Storage Host Controller section, it migrate from
bit[1:0] to bit[2:1]...

Acutally there are three register layouts about sdmmc/sdio tuning
stuff I found. Although just two types were upstreamed, it still
caused difficulty. With your patch for dw_mmc to explictly assign
dr_phase, we keep the phase policy better to be unstood and traced.

Again sorry for commit 7a03fe6f48!

>
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
>
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
>
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
>
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
>
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
>
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
>
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.
>
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
> index bc856f21f6b2..5b18265c2306 100644
> --- a/drivers/clk/rockchip/clk-mmc-phase.c
> +++ b/drivers/clk/rockchip/clk-mmc-phase.c
> @@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
>  #define ROCKCHIP_MMC_DEGREE_MASK 0x3
>  #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
>  #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
> -#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
> -#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
>
>  #define PSECS_PER_SEC 1000000000000LL
>
> @@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	mmc_clock->reg = reg;
>  	mmc_clock->shift = shift;
>
> -	/*
> -	 * Assert init_state to soft reset the CLKGEN
> -	 * for mmc tuning phase and degree
> -	 */
> -	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
> -		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     mmc_clock->shift), mmc_clock->reg);
> -
>  	clk = clk_register(NULL, &mmc_clock->hw);
>  	if (IS_ERR(clk))
>  		kfree(mmc_clock);
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Doug Anderson]

Heiko,

On Thu, May 12, 2016 at 4:41 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:
>
> Not sure what the policy is for revert-subjects in the clock-tree, but I
> guess
>
> clk: rockchip: Revert "reset init state before mmc card initialization"
>
> might look nicer?

Sure.  If you want me to re-post with that, let me know.  Else feel
free to fix when applying.


>> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
>> before mmc card initialization").
>
> I've tested these two patches together with the dw_mmc one and everything
> that worked before still works (tuning on veyron-jerry) and everything that
> didn't work still doesn't work (tuning on firefly [likely a regulator issue]).
>
> And re-checking the code against the TRM I now also see the mask that is to
> short (1 bit in the code vs. the actual 2 bits described in the manual)
>
> [...]
>
>> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>>    + other cases; might break someone with a strange bootloader that
>>    sets the phase to 0 or one that uses delay elements (pretty
>>    unpredicable what would happen in that case).
>> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>>    totally override the broken patch and fix everything.
>> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>>    any defaults from the clock code doesn't mattery.
>
> I'm still trying to make up my mind on how to proceed - aka in which order
> patches should get picked up.
>
> Going with the "if it isn't upstream it doesn't exist" mantra Rob wrote
> somewhere some days ago, taking this revert should not hurt anything, as all
> the non-veyron devices currently don't support any tuning at all.

Note that this patch can still affect devices that don't use tuning.
MMC Hold times are important not just for high speed modes but even
for low speed modes.


> So I'd think except for the issue Shawn was originally trying to fix (for
> some non-mainline board?) it should be possible to take this patch any time
> independently of the dw_mmc part.

Yeah, just not sure.  Shawn: do you know which specific device you
were trying to fix with the original patch?  Is it OK to revert for
now until the dw_mmc patch lands?

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Doug Anderson]

Heiko,

On Thu, May 12, 2016 at 4:41 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:
>
> Not sure what the policy is for revert-subjects in the clock-tree, but I
> guess
>
> clk: rockchip: Revert "reset init state before mmc card initialization"
>
> might look nicer?

Sure.  If you want me to re-post with that, let me know.  Else feel
free to fix when applying.


>> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
>> before mmc card initialization").
>
> I've tested these two patches together with the dw_mmc one and everything
> that worked before still works (tuning on veyron-jerry) and everything that
> didn't work still doesn't work (tuning on firefly [likely a regulator issue]).
>
> And re-checking the code against the TRM I now also see the mask that is to
> short (1 bit in the code vs. the actual 2 bits described in the manual)
>
> [...]
>
>> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>>    + other cases; might break someone with a strange bootloader that
>>    sets the phase to 0 or one that uses delay elements (pretty
>>    unpredicable what would happen in that case).
>> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>>    totally override the broken patch and fix everything.
>> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>>    any defaults from the clock code doesn't mattery.
>
> I'm still trying to make up my mind on how to proceed - aka in which order
> patches should get picked up.
>
> Going with the "if it isn't upstream it doesn't exist" mantra Rob wrote
> somewhere some days ago, taking this revert should not hurt anything, as all
> the non-veyron devices currently don't support any tuning at all.

Note that this patch can still affect devices that don't use tuning.
MMC Hold times are important not just for high speed modes but even
for low speed modes.


> So I'd think except for the issue Shawn was originally trying to fix (for
> some non-mainline board?) it should be possible to take this patch any time
> independently of the dw_mmc part.

Yeah, just not sure.  Shawn: do you know which specific device you
were trying to fix with the original patch?  Is it OK to revert for
now until the dw_mmc patch lands?

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Doug Anderson]

Heiko,

On Thu, May 12, 2016 at 4:41 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:
>
> Not sure what the policy is for revert-subjects in the clock-tree, but I
> guess
>
> clk: rockchip: Revert "reset init state before mmc card initialization"
>
> might look nicer?

Sure.  If you want me to re-post with that, let me know.  Else feel
free to fix when applying.


>> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
>> before mmc card initialization").
>
> I've tested these two patches together with the dw_mmc one and everything
> that worked before still works (tuning on veyron-jerry) and everything that
> didn't work still doesn't work (tuning on firefly [likely a regulator issue]).
>
> And re-checking the code against the TRM I now also see the mask that is to
> short (1 bit in the code vs. the actual 2 bits described in the manual)
>
> [...]
>
>> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>>    + other cases; might break someone with a strange bootloader that
>>    sets the phase to 0 or one that uses delay elements (pretty
>>    unpredicable what would happen in that case).
>> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>>    totally override the broken patch and fix everything.
>> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>>    any defaults from the clock code doesn't mattery.
>
> I'm still trying to make up my mind on how to proceed - aka in which order
> patches should get picked up.
>
> Going with the "if it isn't upstream it doesn't exist" mantra Rob wrote
> somewhere some days ago, taking this revert should not hurt anything, as all
> the non-veyron devices currently don't support any tuning at all.

Note that this patch can still affect devices that don't use tuning.
MMC Hold times are important not just for high speed modes but even
for low speed modes.


> So I'd think except for the issue Shawn was originally trying to fix (for
> some non-mainline board?) it should be possible to take this patch any time
> independently of the dw_mmc part.

Yeah, just not sure.  Shawn: do you know which specific device you
were trying to fix with the original patch?  Is it OK to revert for
now until the dw_mmc patch lands?

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 在 2016/5/13 7:10, Brian Norris 写道:
>>
>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>
>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>> these values were set to 0 because of a typo.
>>
>>
>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>
>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>> at bits [1:0].
>>
>
>
> Please refer to Mobile Strorage Host Controller section for anything
> about sdmmc/sdio. So shift should be 1.
>
> Sometime I also get bothered to address it. Anyway, I will always keep
> a eye on it from now on.....

I still in general have mistrust for TRM docs for things like this.
Have you verified that this was an intentional change for rk3399, or
could it be a  typo?  Typically SoCs don't change this type of stuff
for no reason.

This should be possible to verify in one of two ways.  If the TRM has
a typo and things truly _do_ start at 0 instead of 1, then:

1. There will be roughly mirrors of valid ranges.
2. Things won't match up if we change tuning to use 180 course offsets
and the rest fine offsets.

It would be ideal if you could confirm with the chip guys, but if you
can't I'll try to do more tests tomorrow.

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote=
:
> =E5=9C=A8 2016/5/13 7:10, Brian Norris =E5=86=99=E9=81=93:
>>
>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>
>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumabl=
y
>>> these values were set to 0 because of a typo.
>>
>>
>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>
>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>> at bits [1:0].
>>
>
>
> Please refer to Mobile Strorage Host Controller section for anything
> about sdmmc/sdio. So shift should be 1.
>
> Sometime I also get bothered to address it. Anyway, I will always keep
> a eye on it from now on.....

I still in general have mistrust for TRM docs for things like this.
Have you verified that this was an intentional change for rk3399, or
could it be a  typo?  Typically SoCs don't change this type of stuff
for no reason.

This should be possible to verify in one of two ways.  If the TRM has
a typo and things truly _do_ start at 0 instead of 1, then:

1. There will be roughly mirrors of valid ranges.
2. Things won't match up if we change tuning to use 180 course offsets
and the rest fine offsets.

It would be ideal if you could confirm with the chip guys, but if you
can't I'll try to do more tests tomorrow.

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 在 2016/5/13 7:10, Brian Norris 写道:
>>
>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>
>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>> these values were set to 0 because of a typo.
>>
>>
>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>
>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>> at bits [1:0].
>>
>
>
> Please refer to Mobile Strorage Host Controller section for anything
> about sdmmc/sdio. So shift should be 1.
>
> Sometime I also get bothered to address it. Anyway, I will always keep
> a eye on it from now on.....

I still in general have mistrust for TRM docs for things like this.
Have you verified that this was an intentional change for rk3399, or
could it be a  typo?  Typically SoCs don't change this type of stuff
for no reason.

This should be possible to verify in one of two ways.  If the TRM has
a typo and things truly _do_ start at 0 instead of 1, then:

1. There will be roughly mirrors of valid ranges.
2. Things won't match up if we change tuning to use 180 course offsets
and the rest fine offsets.

It would be ideal if you could confirm with the chip guys, but if you
can't I'll try to do more tests tomorrow.

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> ? 2016/5/13 7:10, Brian Norris ??:
>>
>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>
>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>> these values were set to 0 because of a typo.
>>
>>
>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>
>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>> at bits [1:0].
>>
>
>
> Please refer to Mobile Strorage Host Controller section for anything
> about sdmmc/sdio. So shift should be 1.
>
> Sometime I also get bothered to address it. Anyway, I will always keep
> a eye on it from now on.....

I still in general have mistrust for TRM docs for things like this.
Have you verified that this was an intentional change for rk3399, or
could it be a  typo?  Typically SoCs don't change this type of stuff
for no reason.

This should be possible to verify in one of two ways.  If the TRM has
a typo and things truly _do_ start at 0 instead of 1, then:

1. There will be roughly mirrors of valid ranges.
2. Things won't match up if we change tuning to use 180 course offsets
and the rest fine offsets.

It would be ideal if you could confirm with the chip guys, but if you
can't I'll try to do more tests tomorrow.

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Shawn Lin]

在 2016/5/13 12:36, Doug Anderson 写道:
> Shawn,
>
> On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> 在 2016/5/13 7:10, Brian Norris 写道:
>>>
>>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>>
>>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>>> these values were set to 0 because of a typo.
>>>
>>>
>>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>>
>>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>>> at bits [1:0].
>>>
>>
>>
>> Please refer to Mobile Strorage Host Controller section for anything
>> about sdmmc/sdio. So shift should be 1.
>>
>> Sometime I also get bothered to address it. Anyway, I will always keep
>> a eye on it from now on.....
>
> I still in general have mistrust for TRM docs for things like this.
> Have you verified that this was an intentional change for rk3399, or
> could it be a  typo?  Typically SoCs don't change this type of stuff
> for no reason.
>

Typically it doesn't, but the reality is that {SDMMC,SDIO}_CON{0,}[2:1]
for drv/sample_degree both. Obviously they want to make drv stuff the
same layout as sampe stuff...

> This should be possible to verify in one of two ways.  If the TRM has
> a typo and things truly _do_ start at 0 instead of 1, then:
>
> 1. There will be roughly mirrors of valid ranges.
> 2. Things won't match up if we change tuning to use 180 course offsets
> and the rest fine offsets.
>
> It would be ideal if you could confirm with the chip guys, but if you

I have checked it before Xing upstreamed the code, but as your question 
on the TRM, I check it with the  chip guys again.

So the answer is that drv/sample stuff should refer to  Mobile Strorage
Host Controller section, and it will fit the future Socs from now on.


> can't I'll try to do more tests tomorrow.



>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Shawn Lin]

在 2016/5/13 12:36, Doug Anderson 写道:
> Shawn,
>
> On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> 在 2016/5/13 7:10, Brian Norris 写道:
>>>
>>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>>
>>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>>> these values were set to 0 because of a typo.
>>>
>>>
>>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>>
>>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>>> at bits [1:0].
>>>
>>
>>
>> Please refer to Mobile Strorage Host Controller section for anything
>> about sdmmc/sdio. So shift should be 1.
>>
>> Sometime I also get bothered to address it. Anyway, I will always keep
>> a eye on it from now on.....
>
> I still in general have mistrust for TRM docs for things like this.
> Have you verified that this was an intentional change for rk3399, or
> could it be a  typo?  Typically SoCs don't change this type of stuff
> for no reason.
>

Typically it doesn't, but the reality is that {SDMMC,SDIO}_CON{0,}[2:1]
for drv/sample_degree both. Obviously they want to make drv stuff the
same layout as sampe stuff...

> This should be possible to verify in one of two ways.  If the TRM has
> a typo and things truly _do_ start at 0 instead of 1, then:
>
> 1. There will be roughly mirrors of valid ranges.
> 2. Things won't match up if we change tuning to use 180 course offsets
> and the rest fine offsets.
>
> It would be ideal if you could confirm with the chip guys, but if you

I have checked it before Xing upstreamed the code, but as your question 
on the TRM, I check it with the  chip guys again.

So the answer is that drv/sample stuff should refer to  Mobile Strorage
Host Controller section, and it will fit the future Socs from now on.


> can't I'll try to do more tests tomorrow.



>
>
>


-- 
Best Regards
Shawn Lin

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Shawn Lin]

? 2016/5/13 12:36, Doug Anderson ??:
> Shawn,
>
> On Thu, May 12, 2016 at 4:47 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> ? 2016/5/13 7:10, Brian Norris ??:
>>>
>>> On Thu, May 12, 2016 at 11:03:17AM -0700, Doug Anderson wrote:
>>>>
>>>> Just like every other Rockhip device, the MMC "_sample" clocks should
>>>> have a shift of 0, not a shift of 1.  The rk3399 TRM agrees.  Presumably
>>>> these values were set to 0 because of a typo.
>>>
>>>
>>> I'll semi-disagree about the TRM: the TRM doesn't seem to agree with
>>> itself, so it sometimes agrees with you and sometimes doesn't :)
>>>
>>> On page 79 of the 2nd (?) book, it looks like {SDMMC,SDIO}_CON{0,}[2:1]
>>> are {drv,sample}_degree. But on page 208 of the 1st book, those are put
>>> at bits [1:0].
>>>
>>
>>
>> Please refer to Mobile Strorage Host Controller section for anything
>> about sdmmc/sdio. So shift should be 1.
>>
>> Sometime I also get bothered to address it. Anyway, I will always keep
>> a eye on it from now on.....
>
> I still in general have mistrust for TRM docs for things like this.
> Have you verified that this was an intentional change for rk3399, or
> could it be a  typo?  Typically SoCs don't change this type of stuff
> for no reason.
>

Typically it doesn't, but the reality is that {SDMMC,SDIO}_CON{0,}[2:1]
for drv/sample_degree both. Obviously they want to make drv stuff the
same layout as sampe stuff...

> This should be possible to verify in one of two ways.  If the TRM has
> a typo and things truly _do_ start at 0 instead of 1, then:
>
> 1. There will be roughly mirrors of valid ranges.
> 2. Things won't match up if we change tuning to use 180 course offsets
> and the rest fine offsets.
>
> It would be ideal if you could confirm with the chip guys, but if you

I have checked it before Xing upstreamed the code, but as your question 
on the TRM, I check it with the  chip guys again.

So the answer is that drv/sample stuff should refer to  Mobile Strorage
Host Controller section, and it will fit the future Socs from now on.


> can't I'll try to do more tests tomorrow.



>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Fri, May 13, 2016 at 12:46 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> This should be possible to verify in one of two ways.  If the TRM has
>> a typo and things truly _do_ start at 0 instead of 1, then:
>>
>> 1. There will be roughly mirrors of valid ranges.
>> 2. Things won't match up if we change tuning to use 180 course offsets
>> and the rest fine offsets.
>>
>> It would be ideal if you could confirm with the chip guys, but if you
>
>
> I have checked it before Xing upstreamed the code, but as your question on
> the TRM, I check it with the  chip guys again.
>
> So the answer is that drv/sample stuff should refer to  Mobile Strorage
> Host Controller section, and it will fit the future Socs from now on.

OK, awesome.  I also did more quick tests by forcing dw_mmc to give me
extra details about the tuning.  These tests agree with you.

Please consider this patch abandoned.  Note that the previous patch
(1/2) is still good as far as I know, so just this patch (2/2) should
be abandoned.

Thanks!  :)

-Doug

Re: [PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Fri, May 13, 2016 at 12:46 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> This should be possible to verify in one of two ways.  If the TRM has
>> a typo and things truly _do_ start at 0 instead of 1, then:
>>
>> 1. There will be roughly mirrors of valid ranges.
>> 2. Things won't match up if we change tuning to use 180 course offsets
>> and the rest fine offsets.
>>
>> It would be ideal if you could confirm with the chip guys, but if you
>
>
> I have checked it before Xing upstreamed the code, but as your question on
> the TRM, I check it with the  chip guys again.
>
> So the answer is that drv/sample stuff should refer to  Mobile Strorage
> Host Controller section, and it will fit the future Socs from now on.

OK, awesome.  I also did more quick tests by forcing dw_mmc to give me
extra details about the tuning.  These tests agree with you.

Please consider this patch abandoned.  Note that the previous patch
(1/2) is still good as far as I know, so just this patch (2/2) should
be abandoned.

Thanks!  :)

-Doug

[PATCH 2/2] clk: rockchip: fix the rk3399 sdmmc sample shift

[Doug Anderson]

Shawn,

On Fri, May 13, 2016 at 12:46 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> This should be possible to verify in one of two ways.  If the TRM has
>> a typo and things truly _do_ start at 0 instead of 1, then:
>>
>> 1. There will be roughly mirrors of valid ranges.
>> 2. Things won't match up if we change tuning to use 180 course offsets
>> and the rest fine offsets.
>>
>> It would be ideal if you could confirm with the chip guys, but if you
>
>
> I have checked it before Xing upstreamed the code, but as your question on
> the TRM, I check it with the  chip guys again.
>
> So the answer is that drv/sample stuff should refer to  Mobile Strorage
> Host Controller section, and it will fit the future Socs from now on.

OK, awesome.  I also did more quick tests by forcing dw_mmc to give me
extra details about the tuning.  These tests agree with you.

Please consider this patch abandoned.  Note that the previous patch
(1/2) is still good as far as I know, so just this patch (2/2) should
be abandoned.

Thanks!  :)

-Doug

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Heiko Stuebner]

Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
> 
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).
> 
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
> 
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
> 
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
> 
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
> 
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
> 
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
> 
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.
> 
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card
> initialization") Signed-off-by: Douglas Anderson <dianders@chromium.org>

I've tested this revert myself on rk3288-veyron-jerry and rk3288-firefly 
[everything still works] and have put it temporarily into my clk-fixes branch 
to hopefully get a report from kernelci for rk3288-rock2, but may drop it 
again if necessary.


Heiko

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Heiko Stuebner]

Am Donnerstag, 12. Mai 2016, 11:03:16 schrieb Douglas Anderson:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
> 
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).
> 
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
> 
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
> 
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
> 
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
> 
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
> 
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
> 
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>    + other cases; might break someone with a strange bootloader that
>    sets the phase to 0 or one that uses delay elements (pretty
>    unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>    totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>    any defaults from the clock code doesn't mattery.
> 
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card
> initialization") Signed-off-by: Douglas Anderson <dianders@chromium.org>

I've tested this revert myself on rk3288-veyron-jerry and rk3288-firefly 
[everything still works] and have put it temporarily into my clk-fixes branch 
to hopefully get a report from kernelci for rk3288-rock2, but may drop it 
again if necessary.


Heiko

Re: [PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Shawn Lin]

On 2016-5-13 2:03, Douglas Anderson wrote:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
>
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).
>
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
>
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
>
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
>
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
>
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
>
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
>
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>     + other cases; might break someone with a strange bootloader that
>     sets the phase to 0 or one that uses delay elements (pretty
>     unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>     totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>     any defaults from the clock code doesn't mattery.
>
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks for this fix.

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

> ---
>   drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
>   1 file changed, 11 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
> index bc856f21f6b2..5b18265c2306 100644
> --- a/drivers/clk/rockchip/clk-mmc-phase.c
> +++ b/drivers/clk/rockchip/clk-mmc-phase.c
> @@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
>   #define ROCKCHIP_MMC_DEGREE_MASK 0x3
>   #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
>   #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
> -#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
> -#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
>
>   #define PSECS_PER_SEC 1000000000000LL
>
> @@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>   	mmc_clock->reg = reg;
>   	mmc_clock->shift = shift;
>
> -	/*
> -	 * Assert init_state to soft reset the CLKGEN
> -	 * for mmc tuning phase and degree
> -	 */
> -	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
> -		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     mmc_clock->shift), mmc_clock->reg);
> -
>   	clk = clk_register(NULL, &mmc_clock->hw);
>   	if (IS_ERR(clk))
>   		kfree(mmc_clock);
>

[PATCH 1/2] Revert "clk: rockchip: reset init state before mmc card initialization"

[Shawn Lin]

On 2016-5-13 2:03, Douglas Anderson wrote:
> This reverts commit 7a03fe6f48f3 ("clk: rockchip: reset init state
> before mmc card initialization").
>
> Though not totally obvious from the commit message nor from the source
> code, that commit appears to be trying to reset the "_drv" MMC clocks to
> 90 degrees (note that the "_sample" MMC clocks have a shift of 0 so are
> not touched).
>
> The major problem here is that it doesn't properly reset things.  The
> phase is a two bit field and the commit only touches one of the two
> bits.  Thus the commit had the following affect:
> - phase   0  => phase  90
> - phase  90  => phase  90
> - phase 180  => phase 270
> - phase 270  => phase 270
>
> Things get even weirder if you happen to have a bootloader that was
> actually using delay elements (should be no reason to, but you never
> know), since those are additional bits that weren't touched by the
> original patch.
>
> This is unlikely to be what we actually want.  Checking on rk3288-veyron
> devices, I can see that the bootloader leaves these clocks as:
> - emmc:  phase 180
> - sdmmc: phase 90
> - sdio0: phase 90
>
> Thus on rk3288-veyron devices the commit we're reverting had the effect
> of changing the eMMC clock to phase 270.  This probably explains the
> scattered reports I've heard of eMMC devices not working on some veyron
> devices when using the upstream kernel.
>
> The original commit was presumably made because previously the kernel
> didn't touch the "_drv" phase at all and relied on whatever value was
> there when the kernel started.  If someone was using a bootloader that
> touched the "_drv" phase then, indeed, we should have code in the kernel
> to fix that.  ...and also, to get ideal timings, we should also have the
> kernel change the phase depending on the speed mode.  In fact, that's
> the subject of a recent patch I posted at
> <https://patchwork.kernel.org/patch/9075141/>.
>
> Ideally, we should take both the patch posted to dw_mmc and this
> revert.  Since those will likely go through different trees, here I
> describe behavior with the combos:
>
> 1. Just this revert: likely will fix rk3288-veyron eMMC on some devices
>     + other cases; might break someone with a strange bootloader that
>     sets the phase to 0 or one that uses delay elements (pretty
>     unpredicable what would happen in that case).
> 2. Just dw_mmc patch: fixes everyone.  Effectly the dw_mmc patch will
>     totally override the broken patch and fix everything.
> 3. Both patches: fixes everyone.  Once dw_mmc is initting properly then
>     any defaults from the clock code doesn't mattery.
>
> Fixes: 7a03fe6f48f3 ("clk: rockchip: reset init state before mmc card initialization")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks for this fix.

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

> ---
>   drivers/clk/rockchip/clk-mmc-phase.c | 11 -----------
>   1 file changed, 11 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
> index bc856f21f6b2..5b18265c2306 100644
> --- a/drivers/clk/rockchip/clk-mmc-phase.c
> +++ b/drivers/clk/rockchip/clk-mmc-phase.c
> @@ -41,8 +41,6 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
>   #define ROCKCHIP_MMC_DEGREE_MASK 0x3
>   #define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
>   #define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
> -#define ROCKCHIP_MMC_INIT_STATE_RESET 0x1
> -#define ROCKCHIP_MMC_INIT_STATE_SHIFT 1
>
>   #define PSECS_PER_SEC 1000000000000LL
>
> @@ -162,15 +160,6 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>   	mmc_clock->reg = reg;
>   	mmc_clock->shift = shift;
>
> -	/*
> -	 * Assert init_state to soft reset the CLKGEN
> -	 * for mmc tuning phase and degree
> -	 */
> -	if (mmc_clock->shift == ROCKCHIP_MMC_INIT_STATE_SHIFT)
> -		writel(HIWORD_UPDATE(ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     ROCKCHIP_MMC_INIT_STATE_RESET,
> -				     mmc_clock->shift), mmc_clock->reg);
> -
>   	clk = clk_register(NULL, &mmc_clock->hw);
>   	if (IS_ERR(clk))
>   		kfree(mmc_clock);
>