| Message ID | 1463076197-15900-2-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
? 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, > > >
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.
? 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. > > >
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
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,
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(-)