From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shawn Lin Subject: =?UTF-8?Q?Re=3a_=5bPATCH=5d_mmc=3a_dw=5fmmc-rockchip=3a_Using_180_s?= =?UTF-8?B?YW1wbGUgcGhhc2UgaWYgYWxsIHBoYXNlcyB3b3Jr44CQ6K+35rOo5oSP77yM6YKu?= =?UTF-8?B?5Lu255SxbGludXgtbW1jLW93bmVyQHZnZXIua2VybmVsLm9yZ+S7o+WPkeOAkQ==?= Date: Thu, 5 Sep 2019 08:38:08 +0800 Message-ID: References: <1567564030-83224-1-git-send-email-shawn.lin@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Doug Anderson Cc: Ulf Hansson , Heiko Stuebner , shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org, Linux MMC List , Jaehoon Chung , "open list:ARM/Rockchip SoC..." List-Id: linux-mmc@vger.kernel.org On 2019/9/4 23:44, Doug Anderson wrote: > Hi, > > On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin wrote: >> >> default_sample_phase is used to make sure the cards are enumurated >> properly and will be set to 0 if not assigned. However, the sample >> phase should depends on the tuned phase if running higher clock rate. >> If all phases work but default_sample_phase isn't assigned, driver >> set sample phase to 0 for this case, which isn't the best choice, >> because we always expect to set phase to the middle of window. To >> solve the following continually issues we have seen in the test, we >> need set phase to the more stable one, 180, if all phases work. >> >> mmcblk1: error -84 transferring data, sector 1735064, nr 8, cmd >> response 0x900, card status 0xb00 >> mmcblk1: retrying using single block read >> dwmmc_rockchip ff0f0000.dwmmc: All phases work, using default phase 0. >> mmcblk1: retrying because a re-tune was needed >> >> ..... >> >> mmcblk1: error -84 transferring data, sector 1728672, nr 248, cmd >> response 0x900, card status 0xb00 >> mmcblk1: retrying using single block read >> dwmmc_rockchip ff0f0000.dwmmc: All phases work, using default phase 0. >> >> Signed-off-by: Shawn Lin >> --- >> >> drivers/mmc/host/dw_mmc-rockchip.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c >> index d4d0213..9ef9723 100644 >> --- a/drivers/mmc/host/dw_mmc-rockchip.c >> +++ b/drivers/mmc/host/dw_mmc-rockchip.c >> @@ -209,9 +209,8 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) >> } >> >> if (ranges[0].start == 0 && ranges[0].end == priv->num_phases - 1) { >> - clk_set_phase(priv->sample_clk, priv->default_sample_phase); >> - dev_info(host->dev, "All phases work, using default phase %d.", >> - priv->default_sample_phase); >> + clk_set_phase(priv->sample_clk, 180); >> + dev_info(host->dev, "All phases work, using phase 180."); > > This violates what is documented in the device tree bindings, which says: > Thanks for catching this. > * rockchip,default-sample-phase: The default phase to set ciu-sample at > probing, low speeds or in case where all phases work at tuning time. > If not specified 0 deg will be used. > > Specifically: > * You don't use "default-sample-phase" at all when all phases pass. > * You don't default to 0 if not specified. > > Personally I think we could change the bindings to say: "If not > specified 180 deg will be used" and then change the code to do the That sounds sane to me. I will respin v2 to fix the bingdings and change the code. > same. If we wanted to keep the "stable ABI" that is device tree we > could also just do the "180 default" for new SoCs and then manually > add the device tree fragment to all old dts files. > > > -Doug > >