Re: [PATCH] mmc: dw_mmc-rockchip: Using 180 sample phase if all phases work

[Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Wed, 4 Sep 2019 at 17:44, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > ---
> >
> >  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