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

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

[Andreas Fenkart]

failed to initialize builtin emmc device:
"mmc1: error -110 whilst initialising MMC card"

amlogic/meson-gxl-s905x-p212.dtb
linux-5.0-rc2 / 1c7fc5cbc

I bisected the problem and after reverting the following patch, mmc is
found again:

commit 71645e65729f0c5aef8dccf961c97fa766835bd4 (tag: mmc_bad_resable)
Author: Jerome Brunet <jbrunet@baylibre.com>
Date:   Thu Dec 6 16:18:28 2018 +0100

    mmc: meson-gx: add signal resampling

    With some eMMC devices, there is still issues with the new phase
    settings. Enabling signal resampling seems to solve the problem
    for these.

    Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

/Andi

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

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

[Ulf Hansson]

+ Jerome, Kevin

On Wed, 16 Jan 2019 at 11:13, Andreas Fenkart <afenkart@gmail.com> wrote:
>
> failed to initialize builtin emmc device:
> "mmc1: error -110 whilst initialising MMC card"
>
> amlogic/meson-gxl-s905x-p212.dtb
> linux-5.0-rc2 / 1c7fc5cbc
>
> I bisected the problem and after reverting the following patch, mmc is
> found again:
>
> commit 71645e65729f0c5aef8dccf961c97fa766835bd4 (tag: mmc_bad_resable)
> Author: Jerome Brunet <jbrunet@baylibre.com>
> Date:   Thu Dec 6 16:18:28 2018 +0100
>
>     mmc: meson-gx: add signal resampling
>
>     With some eMMC devices, there is still issues with the new phase
>     settings. Enabling signal resampling seems to solve the problem
>     for these.
>
>     Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> /Andi

Andi, thanks for reporting and bisecting!

I have looped in Jerome and Kevin to see if they can have a look.
Let's give them a few days before we consider the revert.

Kind regards
Uffe

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

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

[Jerome Brunet]

On Wed, 2019-01-16 at 12:14 +0100, Ulf Hansson wrote:
> + Jerome, Kevin
> 
> On Wed, 16 Jan 2019 at 11:13, Andreas Fenkart <afenkart@gmail.com> wrote:
> > failed to initialize builtin emmc device:
> > "mmc1: error -110 whilst initialising MMC card"
> > 
> > amlogic/meson-gxl-s905x-p212.dtb
> > linux-5.0-rc2 / 1c7fc5cbc
> > 
> > I bisected the problem and after reverting the following patch, mmc is
> > found again:
> > 
> > commit 71645e65729f0c5aef8dccf961c97fa766835bd4 (tag: mmc_bad_resable)
> > Author: Jerome Brunet <jbrunet@baylibre.com>
> > Date:   Thu Dec 6 16:18:28 2018 +0100
> > 
> >     mmc: meson-gx: add signal resampling
> > 
> >     With some eMMC devices, there is still issues with the new phase
> >     settings. Enabling signal resampling seems to solve the problem
> >     for these.
> > 
> >     Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > 
> > /Andi
> 
> Andi, thanks for reporting and bisecting!
> 
> I have looped in Jerome and Kevin to see if they can have a look.
> Let's give them a few days before we consider the revert.

Hi Andi,

There an actual s905x p212 in kernelci which is not showing any issue, AFAIK.

https://kernelci.org/boot/meson-gxl-s905x-p212/

Have a look a this recent boot of mainline/arm64:
https://storage.kernelci.org/mainline/master/v5.0-rc2-36-gfe76fc6aaf53/arm64/defconfig/lab-baylibre-seattle/boot-meson-gxl-s905x-p212.txt

Has the p212 is ref design of the s905x, we often see the related DT used on
other device.

Is your board actually a p212 ?
If yes, could you tell us about any of the marking on PCB, such as the board
revision ?
If not, could you tell us more about your device ?

We proposing this series, I flagged that this patch and the other one (on
phase settings) needed broader testing. It could possibly cause regression for
tunning and high speed modes ... this typically show with error -5 (tunning
failed)

Here your eMMC timedout during init, so even before an attempt is made to tune
which is odd. Could you try to enable signal resampling only when going forhigh speed modes ? 

> 
> Kind regards
> Uffe



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

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

[Andreas Fenkart]

Hi Jerome
> There an actual s905x p212 in kernelci which is not showing any issue, AFAIK.
>
> https://kernelci.org/boot/meson-gxl-s905x-p212/
>
> Have a look a this recent boot of mainline/arm64:
> https://storage.kernelci.org/mainline/master/v5.0-rc2-36-gfe76fc6aaf53/arm64/defconfig/lab-baylibre-seattle/boot-meson-gxl-s905x-p212.txt

thanks, great way to compare with my local output messages

already wondered about this:
[    1.195173] meson_vid_pll_div_recalc_rate: Invalid config value for
vid_pll_div

> Has the p212 is ref design of the s905x, we often see the related DT used on
> other device.
>
> Is your board actually a p212 ?
> If yes, could you tell us about any of the marking on PCB, such as the board
> revision ?
> If not, could you tell us more about your device ?

it's a "BeeLink Mini M8 SII"

pasted the output here
https://pastebin.com/UCnR25mK

should I try with another dtb?

> We proposing this series, I flagged that this patch and the other one (on
> phase settings) needed broader testing. It could possibly cause regression for
> tunning and high speed modes ... this typically show with error -5 (tunning
> failed)
>
> Here your eMMC timedout during init, so even before an attempt is made to tune
> which is odd. Could you try to enable signal resampling only when going forhigh speed modes ?

huh, need to look into how to do that.

/Andi

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

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

[Neil Armstrong]

Hi Andreas,

On 17/01/2019 13:35, Andreas Fenkart wrote:
> Hi Jerome
>> There an actual s905x p212 in kernelci which is not showing any issue, AFAIK.
>>
>> https://kernelci.org/boot/meson-gxl-s905x-p212/
>>
>> Have a look a this recent boot of mainline/arm64:
>> https://storage.kernelci.org/mainline/master/v5.0-rc2-36-gfe76fc6aaf53/arm64/defconfig/lab-baylibre-seattle/boot-meson-gxl-s905x-p212.txt
> 
> thanks, great way to compare with my local output messages
> 
> already wondered about this:
> [    1.195173] meson_vid_pll_div_recalc_rate: Invalid config value for
> vid_pll_div

This is a warning I should put in dbg, the vid_pll config at reset is unknown and we can't calc a rate...

> 
>> Has the p212 is ref design of the s905x, we often see the related DT used on
>> other device.
>>
>> Is your board actually a p212 ?
>> If yes, could you tell us about any of the marking on PCB, such as the board
>> revision ?
>> If not, could you tell us more about your device ?
> 
> it's a "BeeLink Mini M8 SII"
> 
> pasted the output here
> https://pastebin.com/UCnR25mK
> 
> should I try with another dtb?

Devices derivative from P212 are not 100% same as the reference design, by using different components
and with maybe less good pcb routing, so high speed clocks & modes could not work at all.

You should add a dts for you device based on p212 with some changes in the mmc capabilities.

since we can't have access to the source changes for these devices, it's only based on guess..

Neil

> 
>> We proposing this series, I flagged that this patch and the other one (on
>> phase settings) needed broader testing. It could possibly cause regression for
>> tunning and high speed modes ... this typically show with error -5 (tunning
>> failed)
>>
>> Here your eMMC timedout during init, so even before an attempt is made to tune
>> which is odd. Could you try to enable signal resampling only when going forhigh speed modes ?
> 
> huh, need to look into how to do that.
> 
> /Andi
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 


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

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

[Andreas Fenkart]

Am Do., 17. Jan. 2019 um 13:35 Uhr schrieb Andreas Fenkart <afenkart@gmail.com>:
>
> Hi Jerome

> > Here your eMMC timedout during init, so even before an attempt is made to tune
> > which is odd. Could you try to enable signal resampling only when going forhigh speed modes ?
>

card is found with this patch:

--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -432,12 +432,17 @@ static int meson_mmc_clk_set(struct meson_host
*host, struct mmc_ios *ios)
        struct mmc_host *mmc = host->mmc;
        unsigned long rate = ios->clock;
        int ret;
-       u32 cfg;
+       u32 cfg = 0, adj = 0;

        /* DDR modes require higher module clock */
        if (meson_mmc_timing_is_ddr(ios))
                rate <<= 1;

        /* Same request - bail-out */
        if (host->req_rate == rate)
                return 0;
@@ -475,6 +480,13 @@ static int meson_mmc_clk_set(struct meson_host
*host, struct mmc_ios *ios)
        if (ios->clock != mmc->actual_clock)
                dev_dbg(host->dev, "requested rate was %u\n", ios->clock);

+       if (rate >= 200000000) {
+               printk("enable signal resampling");
+               /* enable signal resampling w/o delay */
+               adj = ADJUST_ADJ_EN;
+               writel(adj, host->regs + host->data->adjust);
+       }
+
        /* (re)start clock */
        meson_mmc_clk_ungate(host);

/Andi

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

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

[Jerome Brunet]

On Thu, 2019-01-17 at 14:56 +0100, Andreas Fenkart wrote:
> Am Do., 17. Jan. 2019 um 13:35 Uhr schrieb Andreas Fenkart <
> afenkart@gmail.com>:
> > Hi Jerome
> > > Here your eMMC timedout during init, so even before an attempt is made
> > > to tune
> > > which is odd. Could you try to enable signal resampling only when going
> > > forhigh speed modes ?
> 
> card is found with this patch:
> 
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -432,12 +432,17 @@ static int meson_mmc_clk_set(struct meson_host
> *host, struct mmc_ios *ios)
>         struct mmc_host *mmc = host->mmc;
>         unsigned long rate = ios->clock;
>         int ret;
> -       u32 cfg;
> +       u32 cfg = 0, adj = 0;
> 
>         /* DDR modes require higher module clock */
>         if (meson_mmc_timing_is_ddr(ios))
>                 rate <<= 1;
> 
>         /* Same request - bail-out */
>         if (host->req_rate == rate)
>                 return 0;
> @@ -475,6 +480,13 @@ static int meson_mmc_clk_set(struct meson_host
> *host, struct mmc_ios *ios)
>         if (ios->clock != mmc->actual_clock)
>                 dev_dbg(host->dev, "requested rate was %u\n", ios->clock);
> 
> +       if (rate >= 200000000) {
> +               printk("enable signal resampling");
> +               /* enable signal resampling w/o delay */
> +               adj = ADJUST_ADJ_EN;
> +               writel(adj, host->regs + host->data->adjust);
> +       }
> +

200Mhz is a bit arbitrary.
I would prefer if you enabled resampling when starting a tuning
And disable it on power up, in the set_ios() callaback.

>         /* (re)start clock */
>         meson_mmc_clk_ungate(host);
> 
> /Andi



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

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

[Andreas Fenkart]

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
mmc1: error -110 whilst initialising MMC card

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);
+
  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 */
+ 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 15:11 Uhr schrieb Jerome Brunet <jbrunet@baylibre.com>:
>
> On Thu, 2019-01-17 at 14:56 +0100, Andreas Fenkart wrote:
> > Am Do., 17. Jan. 2019 um 13:35 Uhr schrieb Andreas Fenkart <
> > afenkart@gmail.com>:
> > > Hi Jerome
> > > > Here your eMMC timedout during init, so even before an attempt is made
> > > > to tune
> > > > which is odd. Could you try to enable signal resampling only when going
> > > > forhigh speed modes ?
> >
> > card is found with this patch:
> >
> > --- a/drivers/mmc/host/meson-gx-mmc.c
> > +++ b/drivers/mmc/host/meson-gx-mmc.c
> > @@ -432,12 +432,17 @@ static int meson_mmc_clk_set(struct meson_host
> > *host, struct mmc_ios *ios)
> >         struct mmc_host *mmc = host->mmc;
> >         unsigned long rate = ios->clock;
> >         int ret;
> > -       u32 cfg;
> > +       u32 cfg = 0, adj = 0;
> >
> >         /* DDR modes require higher module clock */
> >         if (meson_mmc_timing_is_ddr(ios))
> >                 rate <<= 1;
> >
> >         /* Same request - bail-out */
> >         if (host->req_rate == rate)
> >                 return 0;
> > @@ -475,6 +480,13 @@ static int meson_mmc_clk_set(struct meson_host
> > *host, struct mmc_ios *ios)
> >         if (ios->clock != mmc->actual_clock)
> >                 dev_dbg(host->dev, "requested rate was %u\n", ios->clock);
> >
> > +       if (rate >= 200000000) {
> > +               printk("enable signal resampling");
> > +               /* enable signal resampling w/o delay */
> > +               adj = ADJUST_ADJ_EN;
> > +               writel(adj, host->regs + host->data->adjust);
> > +       }
> > +
>
> 200Mhz is a bit arbitrary.
> I would prefer if you enabled resampling when starting a tuning
> And disable it on power up, in the set_ios() callaback.
>
> >         /* (re)start clock */
> >         meson_mmc_clk_ungate(host);
> >
> > /Andi
>
>

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

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

[Jerome Brunet]

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

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

[Andreas Fenkart]

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>
---
 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

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

[Jerome Brunet]

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

[PATCH v2] mmc: meson-gx: enable signal re-sampling together with tuning

[Andreas Fenkart]

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>
---
 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


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

Re: [PATCH v2] mmc: meson-gx: enable signal re-sampling together with tuning

[Jerome Brunet]

On Fri, 2019-01-18 at 14:32 +0100, Andreas Fenkart wrote:
> 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>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

on the axg-s400 (eMMC and SDIO)
on lhe libretech-cc (eMMC and SD)
Tested-by: Jerome Brunet <jbrunet@baylibre.com>


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

Re: [PATCH v2] mmc: meson-gx: enable signal re-sampling together with tuning

[Ulf Hansson]

On Fri, 18 Jan 2019 at 14:32, Andreas Fenkart <afenkart@gmail.com> wrote:
>
> 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>

Applied for fixes, thanks!

Kind regards
Uffe


> ---
>  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
>

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