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

Message ID 1463076197-15900-1-git-send-email-dianders@chromium.org
State New
Headers show

Commit Message

Doug Anderson May 12, 2016, 6:03 p.m. UTC
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(-)

Comments

Heiko Stübner May 12, 2016, 11:41 p.m. UTC | #1
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>
Shawn Lin May 13, 2016, 12:19 a.m. UTC | #2
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);
>
Doug Anderson May 13, 2016, 2:11 a.m. UTC | #3
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?
Heiko Stübner May 17, 2016, 9:56 p.m. UTC | #4
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
Shawn Lin May 18, 2016, 7:25 a.m. UTC | #5
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
diff mbox

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