On Wed, 4 Sep 2019 at 17:44, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Tue, Sep 3, 2019 at 7:28 PM Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> >
> > 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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip
