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

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

Commit Message

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

Comments

Brian Norris May 12, 2016, 11:10 p.m. UTC | #1
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,
Shawn Lin May 12, 2016, 11:47 p.m. UTC | #2
? 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,
>
>
>
Doug Anderson May 13, 2016, 4:36 a.m. UTC | #3
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.
Shawn Lin May 13, 2016, 7:46 a.m. UTC | #4
? 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.



>
>
>
Doug Anderson May 13, 2016, 4:38 p.m. UTC | #5
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
diff mbox

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,