Re: regression caused by: "amlogic: mmc: meson-gx: add signal resampling"

[Jerome Brunet <jbrunet@baylibre.com>]

On Fri, 2019-01-18 at 11:05 +0100, Andreas Fenkart wrote:
> From 7ea0b63ddfc841251cbcc33c8ed0151e52e372f2 Mon Sep 17 00:00:00 2001
> From: Andreas Fenkart <afenkart@gmail.com>
> Date: Thu, 17 Jan 2019 15:39:52 +0100
> Subject: [PATCH] mmc: meson-gx: enable signal re-sampling together with
> tuning
> 
> card detection fails on "BeeLink Mini M8 SII" if enabled too early
> mmc1: error -110 whilst initialising MMC card
> 
> Fixes: 71645e65729f ("mmc: meson-gx: add signal resampling")
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> ---

Andreas,

Your patch gets corrupted by your mailer which makes it quite annoying to
apply.
I would suggest using git send-email

Plus, when sending patch it is better if the actual subject of the mail allows
patchwork to pick it up.
Since it is 2nd version, of your fix, the subject should start with
[PATCH v2].
This helps maintainers (and patchwork) track things

Jerome

>  drivers/mmc/host/meson-gx-mmc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-
> mmc.c
> index c2690c1a50ff..dba499009d0c 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -738,6 +738,11 @@ static int meson_mmc_clk_phase_tuning(struct
> mmc_host *mmc, u32 opcode,
>  static int meson_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>   struct meson_host *host = mmc_priv(mmc);
> + int adj = 0;
> +
> + /* enable signal resampling w/o delay */
> + adj = ADJUST_ADJ_EN;
> + writel(adj, host->regs + host->data->adjust);
> 
>   return meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);
>  }
> @@ -768,6 +773,9 @@ static void meson_mmc_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
>   if (!IS_ERR(mmc->supply.vmmc))
>   mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
> 
> + /* disable signal resampling */
> + writel(0, host->regs + host->data->adjust);
> +
>   /* Reset rx phase */
>   clk_set_phase(host->rx_clk, 0);
> 
> @@ -1166,7 +1174,7 @@ static int meson_mmc_get_cd(struct mmc_host *mmc)
> 
>  static void meson_mmc_cfg_init(struct meson_host *host)
>  {
> - u32 cfg = 0, adj = 0;
> + u32 cfg = 0;
> 
>   cfg |= FIELD_PREP(CFG_RESP_TIMEOUT_MASK,
>     ilog2(SD_EMMC_CFG_RESP_TIMEOUT));
> @@ -1177,10 +1185,6 @@ static void meson_mmc_cfg_init(struct meson_host
> *host)
>   cfg |= CFG_ERR_ABORT;
> 
>   writel(cfg, host->regs + SD_EMMC_CFG);
> -
> - /* enable signal resampling w/o delay */
> - adj = ADJUST_ADJ_EN;
> - writel(adj, host->regs + host->data->adjust);
>  }
> 
>  static int meson_mmc_card_busy(struct mmc_host *mmc)
> --
> 2.20.1
> 
> Am Do., 17. Jan. 2019 um 16:08 Uhr schrieb Jerome Brunet <
> jbrunet@baylibre.com>:
> > On Thu, 2019-01-17 at 15:47 +0100, Andreas Fenkart wrote:
> > > From: Andreas Fenkart <afenkart@gmail.com>
> > > Date: Thu, 17 Jan 2019 15:39:52 +0100
> > > Subject: [PATCH] mmc: meson-gx: enable signal re-sampling together with
> > > tuning
> > > 
> > > card detection fails on some p212 derived boards if enabled too early
> > 
> > Please clearly mention what board you are using, this is too vague.
> > 
> > > mmc1: error -110 whilst initialising MMC card
> > > 
> > 
> > missing the 'Fixes' tag here
> > 
> > > Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> > > ---
> > >   drivers/mmc/host/meson-gx-mmc.c | 16 ++++++++++------
> > >   1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-
> > > gx-
> > > mmc.c
> > > index c2690c1a50ff..b65ec4bea980 100644
> > > --- a/drivers/mmc/host/meson-gx-mmc.c
> > > +++ b/drivers/mmc/host/meson-gx-mmc.c
> > > @@ -709,7 +709,8 @@ static int meson_mmc_find_tuning_point(unsigned long
> > > *test)
> > >   static int meson_mmc_clk_phase_tuning(struct mmc_host *mmc, u32
> > > opcode,
> > >          struct clk *clk)
> > >   {
> > > - int point, ret;
> > > + struct meson_host *host = mmc_priv(mmc);
> > > + int point, ret, adj = 0;
> > >    DECLARE_BITMAP(test, CLK_PHASE_POINT_NUM);
> > > 
> > >    dev_dbg(mmc_dev(mmc), "%s phase/delay tunning...\n",
> > > @@ -729,6 +730,10 @@ static int meson_mmc_clk_phase_tuning(struct
> > > mmc_host *mmc, u32 opcode,
> > >    if (point < 0)
> > >    return point; /* tuning failed */
> > > 
> > > + /* enable signal resampling w/o delay */
> > > + adj = ADJUST_ADJ_EN;
> > > + writel(adj, host->regs + host->data->adjust);
> > > +
> > 
> > That's really what I meant.
> > 
> > Here, you are enabling the signal resampling after the tuning.
> > Several boards won't tune without signal resampling.
> > 
> > This should be done at the very beginning of the function at least.
> > I would prefer if it was done in meson_mmc_execute_tuning() before calling
> > meson_mmc_clk_phase_tuning()
> > 
> > Signal resampling should not be dealt with in the unrelated phase tuning
> > function
> > 
> > >    clk_set_phase(clk, point * CLK_PHASE_STEP);
> > >    dev_dbg(mmc_dev(mmc), "success with phase: %d\n",
> > >    clk_get_phase(clk));
> > > @@ -768,6 +773,9 @@ static void meson_mmc_set_ios(struct mmc_host
> > > *mmc, struct mmc_ios *ios)
> > >    if (!IS_ERR(mmc->supply.vmmc))
> > >    mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
> > > 
> > > + /* disable signal resampling w/o delay */
> > 
> > nitpick : 'disable signal resampling' is enough.
> > when disabled, the delay does not matter.
> > 
> > > + writel(0, host->regs + host->data->adjust);
> > > +
> > >    /* Reset rx phase */
> > >    clk_set_phase(host->rx_clk, 0);
> > > 
> > > @@ -1166,7 +1174,7 @@ static int meson_mmc_get_cd(struct mmc_host *mmc)
> > > 
> > >   static void meson_mmc_cfg_init(struct meson_host *host)
> > >   {
> > > - u32 cfg = 0, adj = 0;
> > > + u32 cfg = 0;
> > > 
> > >    cfg |= FIELD_PREP(CFG_RESP_TIMEOUT_MASK,
> > >      ilog2(SD_EMMC_CFG_RESP_TIMEOUT));
> > > @@ -1177,10 +1185,6 @@ static void meson_mmc_cfg_init(struct meson_host
> > > *host)
> > >    cfg |= CFG_ERR_ABORT;
> > > 
> > >    writel(cfg, host->regs + SD_EMMC_CFG);
> > > -
> > > - /* enable signal resampling w/o delay */
> > > - adj = ADJUST_ADJ_EN;
> > > - writel(adj, host->regs + host->data->adjust);
> > >   }
> > > 
> > >   static int meson_mmc_card_busy(struct mmc_host *mmc)



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic