| Message ID | 1567564030-83224-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: dw_mmc-rockchip: Using 180 sample phase if all phases work | expand |
On Wed, 4 Sep 2019 at 04:28, Shawn Lin <shawn.lin@rock-chips.com> 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 <shawn.lin@rock-chips.com> Is this ready to be tagged for stable, or think its better to get it tested a while and then send a backport to Greg etc instead? In any case, applied for next, thanks! Kind regards Uffe > --- > > 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."); > goto free; > } > > -- > 1.9.1 > > >
On 2019/9/4 14:32, Ulf Hansson wrote: > On Wed, 4 Sep 2019 at 04:28, Shawn Lin <shawn.lin@rock-chips.com> 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 <shawn.lin@rock-chips.com> > > Is this ready to be tagged for stable, or think its better to get it > tested a while and then send a backport to Greg etc instead? > Thanks, Ulf. I would prefer to get it tested in -next and will send backport patches to the revelent stable kernels. > In any case, applied for next, thanks! > > Kind regards > Uffe > > >> --- >> >> 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."); >> goto free; >> } >> >> -- >> 1.9.1 >> >> >> > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > >
Hi, On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin <shawn.lin@rock-chips.com> 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 <shawn.lin@rock-chips.com> > --- > > 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: * 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 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
On 2019/9/4 23:44, Doug Anderson wrote: > Hi, > > On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin <shawn.lin@rock-chips.com> 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 <shawn.lin@rock-chips.com> >> --- >> >> 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 > >
On Wed, 4 Sep 2019 at 17:44, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin <shawn.lin@rock-chips.com> 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 <shawn.lin@rock-chips.com> > > --- > > > > 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: > > * 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 > 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. > Thanks for spotting this! Let me drop the patch and wait for a new version from Shawn. Kind regards Uffe
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."); goto free; }
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 <shawn.lin@rock-chips.com> --- drivers/mmc/host/dw_mmc-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)