[PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Jerome Brunet]

The purpose of this series is too improve reliability of the amlogic mmc
driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)

* The 3 first patches are just harmless clean ups.
* Patch 4 makes sure HS400 can't be enabled, we still have not been able
  to crack this modes.
* Patch 5 removes some clock glitches when switching to DDR modes
* Patch 6 and 7 changes the tuning method from Rx phase to signal
  resampling. It could have been done in a single patch but the unified
  diff was extremely ugly. The change has been split in two patches to
  ease review.

The last tuning update that went through was meant to improve the axg
support. Since then, it was reported to break some other boards, like the
s912 vim2.

Also with the current tuning method, it was impossible to find phase
settings which would work on all the SoCs, including the new ones.

After redoing all the tests from scratch, it appeared that Rx phase made
(strangely) almost no difference, especially on g12a and axg. However, it
showed that it is important to have a phase shift between the Core and Tx
clock, 180 works best.

I discussed the test results with Amlogic. They suggested to use 180/0 or
0/180 for the Core and Tx phase. For tuning, they suggested to use
signal resampling.

So far, so good ... here the platform and modes tested:

NanoPi-K2 (S905): SD UHS SDR50/DDR50, SDIO HS
Odroid-C2 (S905): SD UHS SDR50/DDR50, eMMC DDR52/HS200 (16GB module)
Khadas Vim (S905X): SD HS, SDIO HS, eMMC HS200
Libretech CC (S905X): SD HS, eMMC HS200
Khadas Vim2 (S912): SD HS, SDIO HS, eMMC HS200
S400 (A113D): SDIO UHS SDR104, eMMC DDR52/HS200
U200 (S905D2): SD HS, eMMC DDR52/HS200
SEI510 (S905X2): SD HS, eMMC DDR52/HS200

Changes since v1 [0]:
* Add missing writel in patch 5 (error when switching width)
* Change patch 3 commit description

[0]: https://lkml.kernel.org/r/20190417204355.469-1-jbrunet@baylibre.com

Jerome Brunet (7):
  mmc: meson-gx: remove open coded read with timeout
  mmc: meson-gx: ack only raised irq
  mmc: meson-gx: correct irq flag
  mmc: meson-gx: disable HS400
  mmc: meson-gx: avoid clock glitch when switching to DDR modes
  mmc: meson-gx: remove Rx phase tuning
  mmc: meson-gx: add signal resampling tuning

 drivers/mmc/host/meson-gx-mmc.c | 419 +++++++++-----------------------
 1 file changed, 114 insertions(+), 305 deletions(-)

-- 
2.20.1


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

[PATCH v2 1/7] mmc: meson-gx: remove open coded read with timeout

[Jerome Brunet]

There is already a function available to poll a register until a
condition is met. Let's use it instead of open coding it.

Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 2eba507790e4..2deeacc051b1 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/iopoll.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/ioport.h>
@@ -1100,7 +1101,6 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 
 static int meson_mmc_wait_desc_stop(struct meson_host *host)
 {
-	int loop;
 	u32 status;
 
 	/*
@@ -1110,20 +1110,10 @@ static int meson_mmc_wait_desc_stop(struct meson_host *host)
 	 * If we don't confirm the descriptor is stopped, it might raise new
 	 * IRQs after we have called mmc_request_done() which is bad.
 	 */
-	for (loop = 50; loop; loop--) {
-		status = readl(host->regs + SD_EMMC_STATUS);
-		if (status & (STATUS_BUSY | STATUS_DESC_BUSY))
-			udelay(100);
-		else
-			break;
-	}
 
-	if (status & (STATUS_BUSY | STATUS_DESC_BUSY)) {
-		dev_err(host->dev, "Timed out waiting for host to stop\n");
-		return -ETIMEDOUT;
-	}
-
-	return 0;
+	return readl_poll_timeout(host->regs + SD_EMMC_STATUS, status,
+				  !(status & (STATUS_BUSY | STATUS_DESC_BUSY)),
+				  100, 5000);
 }
 
 static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
-- 
2.20.1


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

[PATCH v2 2/7] mmc: meson-gx: ack only raised irq

[Jerome Brunet]

This is merely a clean up. It makes sense to only ack raised irqs
instead of acking everything all the time.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 2deeacc051b1..8b690ecde4c5 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -1082,9 +1082,6 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 	}
 
 out:
-	/* ack all enabled interrupts */
-	writel(irq_en, host->regs + SD_EMMC_STATUS);
-
 	if (cmd->error) {
 		/* Stop desc in case of errors */
 		u32 start = readl(host->regs + SD_EMMC_START);
@@ -1096,6 +1093,9 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 	if (ret == IRQ_HANDLED)
 		meson_mmc_request_done(host->mmc, cmd->mrq);
 
+	/* ack all raised interrupts */
+	writel(status, host->regs + SD_EMMC_STATUS);
+
 	return ret;
 }
 
-- 
2.20.1


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

Re: [PATCH v2 2/7] mmc: meson-gx: ack only raised irq

[Martin Blumenstingl]

On Tue, Apr 23, 2019 at 11:02 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> This is merely a clean up. It makes sense to only ack raised irqs
> instead of acking everything all the time.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
based on reading (and understanding) the code and a test on my Khadas
VIM this seems fine so:
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

[PATCH v2 3/7] mmc: meson-gx: correct irq flag

[Jerome Brunet]

There is no reason for another device to request the MMC irq. It should
only be used the MMC device, so remove IRQ_SHARED and replace by
IRQ_ONESHOT as we don't the irq to fire again until the irq thread is
done

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 8b690ecde4c5..3df50b53f834 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -1328,7 +1328,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	       host->regs + SD_EMMC_IRQ_EN);
 
 	ret = request_threaded_irq(host->irq, meson_mmc_irq,
-				   meson_mmc_irq_thread, IRQF_SHARED,
+				   meson_mmc_irq_thread, IRQF_ONESHOT,
 				   dev_name(&pdev->dev), host);
 	if (ret)
 		goto err_init_clk;
-- 
2.20.1


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

Re: [PATCH v2 3/7] mmc: meson-gx: correct irq flag

[Martin Blumenstingl]

On Tue, Apr 23, 2019 at 11:02 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> There is no reason for another device to request the MMC irq. It should
> only be used the MMC device, so remove IRQ_SHARED and replace by
> IRQ_ONESHOT as we don't the irq to fire again until the irq thread is
> done
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
I'm doing the same thing (for the same purpose) in the meson-mx-sdio driver:
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

[PATCH v2 4/7] mmc: meson-gx: disable HS400

[Jerome Brunet]

At the moment, all our attempts to enable HS400 on Amlogic chipsets have
been unsuccessful or unreliable. Until we can figure out how to enable this
mode safely and reliably, let's force it off.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 3df50b53f834..118f09da8dfb 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -823,10 +823,6 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (meson_mmc_timing_is_ddr(ios))
 		val |= CFG_DDR;
 
-	val &= ~CFG_CHK_DS;
-	if (ios->timing == MMC_TIMING_MMC_HS400)
-		val |= CFG_CHK_DS;
-
 	err = meson_mmc_clk_set(host, ios);
 	if (err)
 		dev_err(host->dev, "Failed to set clock: %d\n,", err);
@@ -1339,6 +1335,13 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	mmc->max_segs = SD_EMMC_DESC_BUF_LEN / sizeof(struct sd_emmc_desc);
 	mmc->max_seg_size = mmc->max_req_size;
 
+	/*
+	 * At the moment, we don't know how to reliably enable HS400.
+	 * From the different datasheets, it is not even clear if this mode
+	 * is officially supported by any of the SoCs
+	 */
+	mmc->caps2 &= ~MMC_CAP2_HS400;
+
 	/* data bounce buffer */
 	host->bounce_buf_size = mmc->max_req_size;
 	host->bounce_buf =
-- 
2.20.1


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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Martin Blumenstingl]

Hi Jerome,

On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> been unsuccessful or unreliable. Until we can figure out how to enable this
> mode safely and reliably, let's force it off.
last year I have seen issues with HS400 on my Khadas VIM2: [0]
have you tested all other patches from this series and HS400 is still
not working for you?

I'm curious because this patch is early in the series and all the
tuning fixes and improvements are after this patch.


Martin


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006251.html

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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Jerome Brunet]

On Sat, 2019-04-27 at 22:02 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> > been unsuccessful or unreliable. Until we can figure out how to enable this
> > mode safely and reliably, let's force it off.
> last year I have seen issues with HS400 on my Khadas VIM2: [0]
> have you tested all other patches from this series and HS400 is still
> not working for you?

Because HS400 was never really stable to begin with.
It was a mistake to enable it on the VIM2

> 
> I'm curious because this patch is early in the series and all the
> tuning fixes and improvements are after this patch.

There are several reasons why this new tuning won't solve the HS400 problem:
- Signal resampling tuning granularity gets worse when rate rises. The resampling 
is done using the input frequency. We can basically resample up to the value of 
internal divider.

In HS200 - Fin is 1GHz, Fout is 200MHz (1/5) so the resample range is [0, 4]
In HS400 - Fin should be fdiv5 (400MHZ) and in such case, no resampling is
           possible (internal div = 1)
           Even if we keep 1GHz, then out is 333MHz max giving a range of [0, 2]
           that's not enough to tune

Going further, tuning the Rx path does not make much sense in HS400 since we
should be using the data strobe signal to account for the round trip delay of
the clock and correctly sample Rx. AFAICT, If there is a tuning to be done for
HS400, it is most likely linked to the data strobe.

> 
> 
> Martin
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006251.html



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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Martin Blumenstingl]

Hi Jerome,

On Mon, Apr 29, 2019 at 10:29 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Sat, 2019-04-27 at 22:02 +0200, Martin Blumenstingl wrote:
> > Hi Jerome,
> >
> > On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> > > been unsuccessful or unreliable. Until we can figure out how to enable this
> > > mode safely and reliably, let's force it off.
> > last year I have seen issues with HS400 on my Khadas VIM2: [0]
> > have you tested all other patches from this series and HS400 is still
> > not working for you?
>
> Because HS400 was never really stable to begin with.
> It was a mistake to enable it on the VIM2
>
> >
> > I'm curious because this patch is early in the series and all the
> > tuning fixes and improvements are after this patch.
>
> There are several reasons why this new tuning won't solve the HS400 problem:
> - Signal resampling tuning granularity gets worse when rate rises. The resampling
> is done using the input frequency. We can basically resample up to the value of
> internal divider.
>
> In HS200 - Fin is 1GHz, Fout is 200MHz (1/5) so the resample range is [0, 4]
> In HS400 - Fin should be fdiv5 (400MHZ) and in such case, no resampling is
>            possible (internal div = 1)
>            Even if we keep 1GHz, then out is 333MHz max giving a range of [0, 2]
>            that's not enough to tune
this limitation would be great to have in the description of patch 7
from this series

> Going further, tuning the Rx path does not make much sense in HS400 since we
> should be using the data strobe signal to account for the round trip delay of
> the clock and correctly sample Rx. AFAICT, If there is a tuning to be done for
> HS400, it is most likely linked to the data strobe.
it would be great to have a better description as part of the commit
message - with that you can add my:
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

my proposal for an update patch description (apologies I have
incorrectly summarized your findings):
"
At the moment, all our attempts to enable HS400 on Amlogic chipsets have
been unsuccessful or unreliable:
- signal resampling without delay adjustments and phase tuning for the
RX and TX clocks (this caused CRC errors and hangs even without HS400
mode, for example on the Khadas VIM, Khadas VIM2 and libretech-cc
boards)
- signal resampling without delay adjustments and RX clock phase
tuning (some HS200 and HS400 eMMC chips were not recognized, for
example on the Khadas VIM and Khadas VIM2 boards)
- signal resampling tuning with delay adjustments only (works fine for
HS200 and UHS modes but doesn't fix HS400 eMMC chips, for example on
Khadas VIM2)

Further improvements for the HS400 mode are likely to be linked to the
data strobe signal.
Until we can figure out how to enable this mode safely and reliably,
let's force it off.
"

This whole series is a good step forward.
also thank you for this additional explanation!


Regards
Martin

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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Jerome Brunet]

On Mon, 2019-04-29 at 20:31 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Mon, Apr 29, 2019 at 10:29 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Sat, 2019-04-27 at 22:02 +0200, Martin Blumenstingl wrote:
> > > Hi Jerome,
> > > 
> > > On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > > At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> > > > been unsuccessful or unreliable. Until we can figure out how to enable this
> > > > mode safely and reliably, let's force it off.
> > > last year I have seen issues with HS400 on my Khadas VIM2: [0]
> > > have you tested all other patches from this series and HS400 is still
> > > not working for you?
> > 
> > Because HS400 was never really stable to begin with.
> > It was a mistake to enable it on the VIM2
> > 
> > > I'm curious because this patch is early in the series and all the
> > > tuning fixes and improvements are after this patch.
> > 
> > There are several reasons why this new tuning won't solve the HS400 problem:
> > - Signal resampling tuning granularity gets worse when rate rises. The resampling
> > is done using the input frequency. We can basically resample up to the value of
> > internal divider.
> > 
> > In HS200 - Fin is 1GHz, Fout is 200MHz (1/5) so the resample range is [0, 4]
> > In HS400 - Fin should be fdiv5 (400MHZ) and in such case, no resampling is
> >            possible (internal div = 1)
> >            Even if we keep 1GHz, then out is 333MHz max giving a range of [0, 2]
> >            that's not enough to tune
> this limitation would be great to have in the description of patch 7
> from this series

That's not really a limitation. I should probably not have mentioned as it it seems to
have made things even more unclear. I disabled HS400 before introducing the new tuning on
purpose. Any comment regarding hs400 does not belong in patch 7 IHMO. If you want
to add comment regarding hs400, I think it belongs here

> 
> > Going further, tuning the Rx path does not make much sense in HS400 since we
> > should be using the data strobe signal to account for the round trip delay of
> > the clock and correctly sample Rx. AFAICT, If there is a tuning to be done for
> > HS400, it is most likely linked to the data strobe.
> it would be great to have a better description as part of the commit
> message - with that you can add my:
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> my proposal for an update patch description (apologies I have
> incorrectly summarized your findings):
> "
> At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> been unsuccessful or unreliable:
> - signal resampling without delay adjustments and phase tuning for the
> RX and TX clocks (this caused CRC errors and hangs even without HS400
> mode, for example on the Khadas VIM, Khadas VIM2 and libretech-cc
> boards)
> - signal resampling without delay adjustments and RX clock phase
> tuning (some HS200 and HS400 eMMC chips were not recognized, for
> example on the Khadas VIM and Khadas VIM2 boards)
> - signal resampling tuning with delay adjustments only (works fine for
> HS200 and UHS modes but doesn't fix HS400 eMMC chips, for example on
> Khadas VIM2)
> 
> Further improvements for the HS400 mode are likely to be linked to the
> data strobe signal.
> Until we can figure out how to enable this mode safely and reliably,
> let's force it off.
> "

Thanks for effort but all this just maintain the blur around HS400 on amlogic.

Let me rephrase it:
Tuning (phase or resampling) is meant to compensate the clock round trip in UHS
and HS200 modes. In HS400, this should be taken care of by the data strobe.
But we have not been to enable this reliably enable this on amlogic chipset ...

... and I believe we are back to the original commit message.

That's my understanding of the hs400 problem.

> 
> This whole series is a good step forward.
> also thank you for this additional explanation!
> 
> 
> Regards
> Martin



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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Martin Blumenstingl]

Hi Jerome,

On Mon, Apr 29, 2019 at 8:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Mon, 2019-04-29 at 20:31 +0200, Martin Blumenstingl wrote:
> > Hi Jerome,
> >
> > On Mon, Apr 29, 2019 at 10:29 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > On Sat, 2019-04-27 at 22:02 +0200, Martin Blumenstingl wrote:
> > > > Hi Jerome,
> > > >
> > > > On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > > > At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> > > > > been unsuccessful or unreliable. Until we can figure out how to enable this
> > > > > mode safely and reliably, let's force it off.
> > > > last year I have seen issues with HS400 on my Khadas VIM2: [0]
> > > > have you tested all other patches from this series and HS400 is still
> > > > not working for you?
> > >
> > > Because HS400 was never really stable to begin with.
> > > It was a mistake to enable it on the VIM2
> > >
> > > > I'm curious because this patch is early in the series and all the
> > > > tuning fixes and improvements are after this patch.
> > >
> > > There are several reasons why this new tuning won't solve the HS400 problem:
> > > - Signal resampling tuning granularity gets worse when rate rises. The resampling
> > > is done using the input frequency. We can basically resample up to the value of
> > > internal divider.
> > >
> > > In HS200 - Fin is 1GHz, Fout is 200MHz (1/5) so the resample range is [0, 4]
> > > In HS400 - Fin should be fdiv5 (400MHZ) and in such case, no resampling is
> > >            possible (internal div = 1)
> > >            Even if we keep 1GHz, then out is 333MHz max giving a range of [0, 2]
> > >            that's not enough to tune
> > this limitation would be great to have in the description of patch 7
> > from this series
>
> That's not really a limitation. I should probably not have mentioned as it it seems to
> have made things even more unclear. I disabled HS400 before introducing the new tuning on
> purpose. Any comment regarding hs400 does not belong in patch 7 IHMO. If you want
> to add comment regarding hs400, I think it belongs here
>
> >
> > > Going further, tuning the Rx path does not make much sense in HS400 since we
> > > should be using the data strobe signal to account for the round trip delay of
> > > the clock and correctly sample Rx. AFAICT, If there is a tuning to be done for
> > > HS400, it is most likely linked to the data strobe.
> > it would be great to have a better description as part of the commit
> > message - with that you can add my:
> > Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >
> > my proposal for an update patch description (apologies I have
> > incorrectly summarized your findings):
> > "
> > At the moment, all our attempts to enable HS400 on Amlogic chipsets have
> > been unsuccessful or unreliable:
> > - signal resampling without delay adjustments and phase tuning for the
> > RX and TX clocks (this caused CRC errors and hangs even without HS400
> > mode, for example on the Khadas VIM, Khadas VIM2 and libretech-cc
> > boards)
> > - signal resampling without delay adjustments and RX clock phase
> > tuning (some HS200 and HS400 eMMC chips were not recognized, for
> > example on the Khadas VIM and Khadas VIM2 boards)
> > - signal resampling tuning with delay adjustments only (works fine for
> > HS200 and UHS modes but doesn't fix HS400 eMMC chips, for example on
> > Khadas VIM2)
> >
> > Further improvements for the HS400 mode are likely to be linked to the
> > data strobe signal.
> > Until we can figure out how to enable this mode safely and reliably,
> > let's force it off.
> > "
>
> Thanks for effort but all this just maintain the blur around HS400 on amlogic.
>
> Let me rephrase it:
> Tuning (phase or resampling) is meant to compensate the clock round trip in UHS
> and HS200 modes. In HS400, this should be taken care of by the data strobe.
> But we have not been to enable this reliably enable this on amlogic chipset ...
I wasn't aware of this: so far I assumed that we're not setting the
phase correctly, end of the story.
thank you again for taking your time to explain this!

> ... and I believe we are back to the original commit message.
no need to update the description just to explain how HS400 works in
general, so feel free to use my:
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Martin

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

Re: [PATCH v2 4/7] mmc: meson-gx: disable HS400

[Jerome Brunet]

On Tue, 2019-04-30 at 22:02 +0200, Martin Blumenstingl wrote:
> > Thanks for effort but all this just maintain the blur around HS400 on amlogic.
> > 
> > Let me rephrase it:
> > Tuning (phase or resampling) is meant to compensate the clock round trip in UHS
> > and HS200 modes. In HS400, this should be taken care of by the data strobe.
> > But we have not been to enable this reliably enable this on amlogic chipset ...
> I wasn't aware of this: so far I assumed that we're not setting the
> phase correctly, end of the story.
> thank you again for taking your time to explain this!

No worries. There is the MMC in general, I think I understand it a bit now but 
I might still be mistaken about some stuff. Then there is the HW we have and the
related black magic.

I doubt this is the last update in this driver ...

> 
> > ... and I believe we are back to the original commit message.
> no need to update the description just to explain how HS400 works in
> general, so feel free to use my:
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>



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

[PATCH v2 5/7] mmc: meson-gx: avoid clock glitch when switching to DDR modes

[Jerome Brunet]

Activating DDR in the Amlogic mmc controller, among other things, will
divide the output clock by 2. So by activating it with clock on, we are
creating a glitch on the output.

Instead, let's deal with DDR when the clock output is off, when setting
the clock.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 73 +++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 118f09da8dfb..0454021c9ff5 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -169,6 +169,7 @@ struct meson_host {
 	struct clk *rx_clk;
 	struct clk *tx_clk;
 	unsigned long req_rate;
+	bool ddr;
 
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pins_default;
@@ -384,16 +385,6 @@ static void meson_mmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 			     mmc_get_dma_dir(data));
 }
 
-static bool meson_mmc_timing_is_ddr(struct mmc_ios *ios)
-{
-	if (ios->timing == MMC_TIMING_MMC_DDR52 ||
-	    ios->timing == MMC_TIMING_UHS_DDR50 ||
-	    ios->timing == MMC_TIMING_MMC_HS400)
-		return true;
-
-	return false;
-}
-
 /*
  * Gating the clock on this controller is tricky.  It seems the mmc clock
  * is also used by the controller.  It may crash during some operation if the
@@ -430,36 +421,41 @@ static void meson_mmc_clk_ungate(struct meson_host *host)
 	writel(cfg, host->regs + SD_EMMC_CFG);
 }
 
-static int meson_mmc_clk_set(struct meson_host *host, struct mmc_ios *ios)
+static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate,
+			     bool ddr)
 {
 	struct mmc_host *mmc = host->mmc;
-	unsigned long rate = ios->clock;
 	int ret;
 	u32 cfg;
 
-	/* DDR modes require higher module clock */
-	if (meson_mmc_timing_is_ddr(ios))
-		rate <<= 1;
-
 	/* Same request - bail-out */
-	if (host->req_rate == rate)
+	if (host->ddr == ddr && host->req_rate == rate)
 		return 0;
 
 	/* stop clock */
 	meson_mmc_clk_gate(host);
 	host->req_rate = 0;
+	mmc->actual_clock = 0;
 
-	if (!rate) {
-		mmc->actual_clock = 0;
-		/* return with clock being stopped */
+	/* return with clock being stopped */
+	if (!rate)
 		return 0;
-	}
 
 	/* Stop the clock during rate change to avoid glitches */
 	cfg = readl(host->regs + SD_EMMC_CFG);
 	cfg |= CFG_STOP_CLOCK;
 	writel(cfg, host->regs + SD_EMMC_CFG);
 
+	if (ddr) {
+		/* DDR modes require higher module clock */
+		rate <<= 1;
+		cfg |= CFG_DDR;
+	} else {
+		cfg &= ~CFG_DDR;
+	}
+	writel(cfg, host->regs + SD_EMMC_CFG);
+	host->ddr = ddr;
+
 	ret = clk_set_rate(host->mmc_clk, rate);
 	if (ret) {
 		dev_err(host->dev, "Unable to set cfg_div_clk to %lu. ret=%d\n",
@@ -471,12 +467,14 @@ static int meson_mmc_clk_set(struct meson_host *host, struct mmc_ios *ios)
 	mmc->actual_clock = clk_get_rate(host->mmc_clk);
 
 	/* We should report the real output frequency of the controller */
-	if (meson_mmc_timing_is_ddr(ios))
+	if (ddr) {
+		host->req_rate >>= 1;
 		mmc->actual_clock >>= 1;
+	}
 
 	dev_dbg(host->dev, "clk rate: %u Hz\n", mmc->actual_clock);
-	if (ios->clock != mmc->actual_clock)
-		dev_dbg(host->dev, "requested rate was %u\n", ios->clock);
+	if (rate != mmc->actual_clock)
+		dev_dbg(host->dev, "requested rate was %lu\n", rate);
 
 	/* (re)start clock */
 	meson_mmc_clk_ungate(host);
@@ -750,6 +748,25 @@ static int meson_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	return meson_mmc_clk_phase_tuning(mmc, opcode, host->rx_clk);
 }
 
+static int meson_mmc_prepare_ios_clock(struct meson_host *host,
+				       struct mmc_ios *ios)
+{
+	bool ddr;
+
+	switch (ios->timing) {
+	case MMC_TIMING_MMC_DDR52:
+	case MMC_TIMING_UHS_DDR50:
+		ddr = true;
+		break;
+
+	default:
+		ddr = false;
+		break;
+	}
+
+	return meson_mmc_clk_set(host, ios->clock, ddr);
+}
+
 static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct meson_host *host = mmc_priv(mmc);
@@ -818,16 +835,12 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	val = readl(host->regs + SD_EMMC_CFG);
 	val &= ~CFG_BUS_WIDTH_MASK;
 	val |= FIELD_PREP(CFG_BUS_WIDTH_MASK, bus_width);
+	writel(val, host->regs + SD_EMMC_CFG);
 
-	val &= ~CFG_DDR;
-	if (meson_mmc_timing_is_ddr(ios))
-		val |= CFG_DDR;
-
-	err = meson_mmc_clk_set(host, ios);
+	err = meson_mmc_prepare_ios_clock(host, ios);
 	if (err)
 		dev_err(host->dev, "Failed to set clock: %d\n,", err);
 
-	writel(val, host->regs + SD_EMMC_CFG);
 	dev_dbg(host->dev, "SD_EMMC_CFG:  0x%08x\n", val);
 }
 
-- 
2.20.1


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

Re: [PATCH v2 5/7] mmc: meson-gx: avoid clock glitch when switching to DDR modes

[Martin Blumenstingl]

On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> Activating DDR in the Amlogic mmc controller, among other things, will
> divide the output clock by 2. So by activating it with clock on, we are
> creating a glitch on the output.
>
> Instead, let's deal with DDR when the clock output is off, when setting
> the clock.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
[boot-tested on Khadas VIM and no obvious regressions could be seen]
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

thank you for fixing this issue from v1!

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

[PATCH v2 6/7] mmc: meson-gx: remove Rx phase tuning

[Jerome Brunet]

This remove all the code related to phase settings. Using the Rx phase
for tuning has not been reliable. We had several issues over the past
months, on both v2 and v3 mmc chips After discussing the issue matter
with Amlogic, They suggested to set a phase shift of 180 between Core and
Tx and use signal resampling for the tuning.

Since we won't be playing with the phase anymore, let's remove all the
clock code related to it and set the appropriate value on init.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 290 ++------------------------------
 1 file changed, 13 insertions(+), 277 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 0454021c9ff5..acdc5520d02c 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -49,6 +49,8 @@
 #define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
 #define   CLK_TX_PHASE_MASK GENMASK(11, 10)
 #define   CLK_RX_PHASE_MASK GENMASK(13, 12)
+#define   CLK_PHASE_0 0
+#define   CLK_PHASE_180 2
 #define   CLK_V2_TX_DELAY_MASK GENMASK(19, 16)
 #define   CLK_V2_RX_DELAY_MASK GENMASK(23, 20)
 #define   CLK_V2_ALWAYS_ON BIT(24)
@@ -57,10 +59,6 @@
 #define   CLK_V3_RX_DELAY_MASK GENMASK(27, 22)
 #define   CLK_V3_ALWAYS_ON BIT(28)
 
-#define   CLK_DELAY_STEP_PS 200
-#define   CLK_PHASE_STEP 30
-#define   CLK_PHASE_POINT_NUM (360 / CLK_PHASE_STEP)
-
 #define   CLK_TX_DELAY_MASK(h)		(h->data->tx_delay_mask)
 #define   CLK_RX_DELAY_MASK(h)		(h->data->rx_delay_mask)
 #define   CLK_ALWAYS_ON(h)		(h->data->always_on)
@@ -165,9 +163,8 @@ struct meson_host {
 
 	void __iomem *regs;
 	struct clk *core_clk;
+	struct clk *mux_clk;
 	struct clk *mmc_clk;
-	struct clk *rx_clk;
-	struct clk *tx_clk;
 	unsigned long req_rate;
 	bool ddr;
 
@@ -209,90 +206,6 @@ struct meson_host {
 #define CMD_RESP_MASK GENMASK(31, 1)
 #define CMD_RESP_SRAM BIT(0)
 
-struct meson_mmc_phase {
-	struct clk_hw hw;
-	void __iomem *reg;
-	unsigned long phase_mask;
-	unsigned long delay_mask;
-	unsigned int delay_step_ps;
-};
-
-#define to_meson_mmc_phase(_hw) container_of(_hw, struct meson_mmc_phase, hw)
-
-static int meson_mmc_clk_get_phase(struct clk_hw *hw)
-{
-	struct meson_mmc_phase *mmc = to_meson_mmc_phase(hw);
-	unsigned int phase_num = 1 <<  hweight_long(mmc->phase_mask);
-	unsigned long period_ps, p, d;
-		int degrees;
-	u32 val;
-
-	val = readl(mmc->reg);
-	p = (val & mmc->phase_mask) >> __ffs(mmc->phase_mask);
-	degrees = p * 360 / phase_num;
-
-	if (mmc->delay_mask) {
-		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
-					 clk_get_rate(hw->clk));
-		d = (val & mmc->delay_mask) >> __ffs(mmc->delay_mask);
-		degrees += d * mmc->delay_step_ps * 360 / period_ps;
-		degrees %= 360;
-	}
-
-	return degrees;
-}
-
-static void meson_mmc_apply_phase_delay(struct meson_mmc_phase *mmc,
-					unsigned int phase,
-					unsigned int delay)
-{
-	u32 val;
-
-	val = readl(mmc->reg);
-	val &= ~mmc->phase_mask;
-	val |= phase << __ffs(mmc->phase_mask);
-
-	if (mmc->delay_mask) {
-		val &= ~mmc->delay_mask;
-		val |= delay << __ffs(mmc->delay_mask);
-	}
-
-	writel(val, mmc->reg);
-}
-
-static int meson_mmc_clk_set_phase(struct clk_hw *hw, int degrees)
-{
-	struct meson_mmc_phase *mmc = to_meson_mmc_phase(hw);
-	unsigned int phase_num = 1 <<  hweight_long(mmc->phase_mask);
-	unsigned long period_ps, d = 0, r;
-	uint64_t p;
-
-	p = degrees % 360;
-
-	if (!mmc->delay_mask) {
-		p = DIV_ROUND_CLOSEST_ULL(p, 360 / phase_num);
-	} else {
-		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
-					 clk_get_rate(hw->clk));
-
-		/* First compute the phase index (p), the remainder (r) is the
-		 * part we'll try to acheive using the delays (d).
-		 */
-		r = do_div(p, 360 / phase_num);
-		d = DIV_ROUND_CLOSEST(r * period_ps,
-				      360 * mmc->delay_step_ps);
-		d = min(d, mmc->delay_mask >> __ffs(mmc->delay_mask));
-	}
-
-	meson_mmc_apply_phase_delay(mmc, p, d);
-	return 0;
-}
-
-static const struct clk_ops meson_mmc_clk_phase_ops = {
-	.get_phase = meson_mmc_clk_get_phase,
-	.set_phase = meson_mmc_clk_set_phase,
-};
-
 static unsigned int meson_mmc_get_timeout_msecs(struct mmc_data *data)
 {
 	unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC;
@@ -492,8 +405,6 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	struct clk_init_data init;
 	struct clk_mux *mux;
 	struct clk_divider *div;
-	struct meson_mmc_phase *core, *tx, *rx;
-	struct clk *clk;
 	char clk_name[32];
 	int i, ret = 0;
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
@@ -501,9 +412,11 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	u32 clk_reg;
 
 	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
-	clk_reg = 0;
-	clk_reg |= CLK_ALWAYS_ON(host);
+	clk_reg = CLK_ALWAYS_ON(host);
 	clk_reg |= CLK_DIV_MASK;
+	clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
+	clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
+	clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
 	writel(clk_reg, host->regs + SD_EMMC_CLOCK);
 
 	/* get the mux parents */
@@ -539,9 +452,9 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	mux->mask = CLK_SRC_MASK >> mux->shift;
 	mux->hw.init = &init;
 
-	clk = devm_clk_register(host->dev, &mux->hw);
-	if (WARN_ON(IS_ERR(clk)))
-		return PTR_ERR(clk);
+	host->mux_clk = devm_clk_register(host->dev, &mux->hw);
+	if (WARN_ON(IS_ERR(host->mux_clk)))
+		return PTR_ERR(host->mux_clk);
 
 	/* create the divider */
 	div = devm_kzalloc(host->dev, sizeof(*div), GFP_KERNEL);
@@ -552,7 +465,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	init.name = clk_name;
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_SET_RATE_PARENT;
-	clk_parent[0] = __clk_get_name(clk);
+	clk_parent[0] = __clk_get_name(host->mux_clk);
 	init.parent_names = clk_parent;
 	init.num_parents = 1;
 
@@ -562,192 +475,19 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	div->hw.init = &init;
 	div->flags = CLK_DIVIDER_ONE_BASED;
 
-	clk = devm_clk_register(host->dev, &div->hw);
-	if (WARN_ON(IS_ERR(clk)))
-		return PTR_ERR(clk);
-
-	/* create the mmc core clock */
-	core = devm_kzalloc(host->dev, sizeof(*core), GFP_KERNEL);
-	if (!core)
-		return -ENOMEM;
-
-	snprintf(clk_name, sizeof(clk_name), "%s#core", dev_name(host->dev));
-	init.name = clk_name;
-	init.ops = &meson_mmc_clk_phase_ops;
-	init.flags = CLK_SET_RATE_PARENT;
-	clk_parent[0] = __clk_get_name(clk);
-	init.parent_names = clk_parent;
-	init.num_parents = 1;
-
-	core->reg = host->regs + SD_EMMC_CLOCK;
-	core->phase_mask = CLK_CORE_PHASE_MASK;
-	core->hw.init = &init;
-
-	host->mmc_clk = devm_clk_register(host->dev, &core->hw);
-	if (WARN_ON(PTR_ERR_OR_ZERO(host->mmc_clk)))
+	host->mmc_clk = devm_clk_register(host->dev, &div->hw);
+	if (WARN_ON(IS_ERR(host->mmc_clk)))
 		return PTR_ERR(host->mmc_clk);
 
-	/* create the mmc tx clock */
-	tx = devm_kzalloc(host->dev, sizeof(*tx), GFP_KERNEL);
-	if (!tx)
-		return -ENOMEM;
-
-	snprintf(clk_name, sizeof(clk_name), "%s#tx", dev_name(host->dev));
-	init.name = clk_name;
-	init.ops = &meson_mmc_clk_phase_ops;
-	init.flags = 0;
-	clk_parent[0] = __clk_get_name(host->mmc_clk);
-	init.parent_names = clk_parent;
-	init.num_parents = 1;
-
-	tx->reg = host->regs + SD_EMMC_CLOCK;
-	tx->phase_mask = CLK_TX_PHASE_MASK;
-	tx->delay_mask = CLK_TX_DELAY_MASK(host);
-	tx->delay_step_ps = CLK_DELAY_STEP_PS;
-	tx->hw.init = &init;
-
-	host->tx_clk = devm_clk_register(host->dev, &tx->hw);
-	if (WARN_ON(PTR_ERR_OR_ZERO(host->tx_clk)))
-		return PTR_ERR(host->tx_clk);
-
-	/* create the mmc rx clock */
-	rx = devm_kzalloc(host->dev, sizeof(*rx), GFP_KERNEL);
-	if (!rx)
-		return -ENOMEM;
-
-	snprintf(clk_name, sizeof(clk_name), "%s#rx", dev_name(host->dev));
-	init.name = clk_name;
-	init.ops = &meson_mmc_clk_phase_ops;
-	init.flags = 0;
-	clk_parent[0] = __clk_get_name(host->mmc_clk);
-	init.parent_names = clk_parent;
-	init.num_parents = 1;
-
-	rx->reg = host->regs + SD_EMMC_CLOCK;
-	rx->phase_mask = CLK_RX_PHASE_MASK;
-	rx->delay_mask = CLK_RX_DELAY_MASK(host);
-	rx->delay_step_ps = CLK_DELAY_STEP_PS;
-	rx->hw.init = &init;
-
-	host->rx_clk = devm_clk_register(host->dev, &rx->hw);
-	if (WARN_ON(PTR_ERR_OR_ZERO(host->rx_clk)))
-		return PTR_ERR(host->rx_clk);
-
 	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
 	host->mmc->f_min = clk_round_rate(host->mmc_clk, 400000);
 	ret = clk_set_rate(host->mmc_clk, host->mmc->f_min);
 	if (ret)
 		return ret;
 
-	clk_set_phase(host->mmc_clk, 180);
-	clk_set_phase(host->tx_clk, 0);
-	clk_set_phase(host->rx_clk, 0);
-
 	return clk_prepare_enable(host->mmc_clk);
 }
 
-static void meson_mmc_shift_map(unsigned long *map, unsigned long shift)
-{
-	DECLARE_BITMAP(left, CLK_PHASE_POINT_NUM);
-	DECLARE_BITMAP(right, CLK_PHASE_POINT_NUM);
-
-	/*
-	 * shift the bitmap right and reintroduce the dropped bits on the left
-	 * of the bitmap
-	 */
-	bitmap_shift_right(right, map, shift, CLK_PHASE_POINT_NUM);
-	bitmap_shift_left(left, map, CLK_PHASE_POINT_NUM - shift,
-			  CLK_PHASE_POINT_NUM);
-	bitmap_or(map, left, right, CLK_PHASE_POINT_NUM);
-}
-
-static void meson_mmc_find_next_region(unsigned long *map,
-				       unsigned long *start,
-				       unsigned long *stop)
-{
-	*start = find_next_bit(map, CLK_PHASE_POINT_NUM, *start);
-	*stop = find_next_zero_bit(map, CLK_PHASE_POINT_NUM, *start);
-}
-
-static int meson_mmc_find_tuning_point(unsigned long *test)
-{
-	unsigned long shift, stop, offset = 0, start = 0, size = 0;
-
-	/* Get the all good/all bad situation out the way */
-	if (bitmap_full(test, CLK_PHASE_POINT_NUM))
-		return 0; /* All points are good so point 0 will do */
-	else if (bitmap_empty(test, CLK_PHASE_POINT_NUM))
-		return -EIO; /* No successful tuning point */
-
-	/*
-	 * Now we know there is a least one region find. Make sure it does
-	 * not wrap by the shifting the bitmap if necessary
-	 */
-	shift = find_first_zero_bit(test, CLK_PHASE_POINT_NUM);
-	if (shift != 0)
-		meson_mmc_shift_map(test, shift);
-
-	while (start < CLK_PHASE_POINT_NUM) {
-		meson_mmc_find_next_region(test, &start, &stop);
-
-		if ((stop - start) > size) {
-			offset = start;
-			size = stop - start;
-		}
-
-		start = stop;
-	}
-
-	/* Get the center point of the region */
-	offset += (size / 2);
-
-	/* Shift the result back */
-	offset = (offset + shift) % CLK_PHASE_POINT_NUM;
-
-	return offset;
-}
-
-static int meson_mmc_clk_phase_tuning(struct mmc_host *mmc, u32 opcode,
-				      struct clk *clk)
-{
-	int point, ret;
-	DECLARE_BITMAP(test, CLK_PHASE_POINT_NUM);
-
-	dev_dbg(mmc_dev(mmc), "%s phase/delay tunning...\n",
-		__clk_get_name(clk));
-	bitmap_zero(test, CLK_PHASE_POINT_NUM);
-
-	/* Explore tuning points */
-	for (point = 0; point < CLK_PHASE_POINT_NUM; point++) {
-		clk_set_phase(clk, point * CLK_PHASE_STEP);
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (!ret)
-			set_bit(point, test);
-	}
-
-	/* Find the optimal tuning point and apply it */
-	point = meson_mmc_find_tuning_point(test);
-	if (point < 0)
-		return point; /* tuning failed */
-
-	clk_set_phase(clk, point * CLK_PHASE_STEP);
-	dev_dbg(mmc_dev(mmc), "success with phase: %d\n",
-		clk_get_phase(clk));
-	return 0;
-}
-
-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);
-}
-
 static int meson_mmc_prepare_ios_clock(struct meson_host *host,
 				       struct mmc_ios *ios)
 {
@@ -796,9 +536,6 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		/* disable signal resampling */
 		writel(0, host->regs + host->data->adjust);
 
-		/* Reset rx phase */
-		clk_set_phase(host->rx_clk, 0);
-
 		break;
 
 	case MMC_POWER_ON:
@@ -1226,7 +963,6 @@ static const struct mmc_host_ops meson_mmc_ops = {
 	.get_cd         = meson_mmc_get_cd,
 	.pre_req	= meson_mmc_pre_req,
 	.post_req	= meson_mmc_post_req,
-	.execute_tuning = meson_mmc_execute_tuning,
 	.card_busy	= meson_mmc_card_busy,
 	.start_signal_voltage_switch = meson_mmc_voltage_switch,
 };
-- 
2.20.1


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

Re: [PATCH v2 6/7] mmc: meson-gx: remove Rx phase tuning

[Martin Blumenstingl]

On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> This remove all the code related to phase settings. Using the Rx phase
> for tuning has not been reliable. We had several issues over the past
> months, on both v2 and v3 mmc chips After discussing the issue matter
> with Amlogic, They suggested to set a phase shift of 180 between Core and
> Tx and use signal resampling for the tuning.
>
> Since we won't be playing with the phase anymore, let's remove all the
> clock code related to it and set the appropriate value on init.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
[Khadas VIM now shows the HS200 eMMC]
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


BEFORE (no patches from this series applied):
# dmesg | grep mmc1
(no output)

AFTER (all 7 patches from this series applied):
# dmesg | grep mmc1
[    2.945155] mmc1: new HS200 MMC card at address 0001
[    2.957737] mmcblk1: mmc1:0001 AWPD3R 14.6 GiB
[    2.966278] mmcblk1boot0: mmc1:0001 AWPD3R partition 1 4.00 MiB
[    2.976114] mmcblk1boot1: mmc1:0001 AWPD3R partition 2 4.00 MiB
[    2.986354] mmcblk1rpmb: mmc1:0001 AWPD3R partition 3 4.00 MiB,
chardev (241:0)


Regards
Martin

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

[PATCH v2 7/7] mmc: meson-gx: add signal resampling tuning

[Jerome Brunet]

Use signal resampling tuning for the UHS and HS200 modes.
Instead of trying to get the *best* resampling setting with complex
window calculation, we just stop on the first working setting.

If the tuning setting later proves unstable, we will just continue the
tuning where we left it.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 73 +++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index acdc5520d02c..c5a8af4ca76b 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -488,6 +488,61 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	return clk_prepare_enable(host->mmc_clk);
 }
 
+static void meson_mmc_disable_resampling(struct meson_host *host)
+{
+	unsigned int val = readl(host->regs + host->data->adjust);
+
+	val &= ~ADJUST_ADJ_EN;
+	writel(val, host->regs + host->data->adjust);
+}
+
+static void meson_mmc_reset_resampling(struct meson_host *host)
+{
+	unsigned int val;
+
+	meson_mmc_disable_resampling(host);
+
+	val = readl(host->regs + host->data->adjust);
+	val &= ~ADJUST_ADJ_DELAY_MASK;
+	writel(val, host->regs + host->data->adjust);
+}
+
+static int meson_mmc_resampling_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct meson_host *host = mmc_priv(mmc);
+	unsigned int val, dly, max_dly, i;
+	int ret;
+
+	/* Resampling is done using the source clock */
+	max_dly = DIV_ROUND_UP(clk_get_rate(host->mux_clk),
+			       clk_get_rate(host->mmc_clk));
+
+	val = readl(host->regs + host->data->adjust);
+	val |= ADJUST_ADJ_EN;
+	writel(val, host->regs + host->data->adjust);
+
+	if (mmc->doing_retune)
+		dly = FIELD_GET(ADJUST_ADJ_DELAY_MASK, val) + 1;
+	else
+		dly = 0;
+
+	for (i = 0; i < max_dly; i++) {
+		val &= ~ADJUST_ADJ_DELAY_MASK;
+		val |= FIELD_PREP(ADJUST_ADJ_DELAY_MASK, (dly + i) % max_dly);
+		writel(val, host->regs + host->data->adjust);
+
+		ret = mmc_send_tuning(mmc, opcode, NULL);
+		if (!ret) {
+			dev_dbg(mmc_dev(mmc), "resampling delay: %u\n",
+				(dly + i) % max_dly);
+			return 0;
+		}
+	}
+
+	meson_mmc_reset_resampling(host);
+	return -EIO;
+}
+
 static int meson_mmc_prepare_ios_clock(struct meson_host *host,
 				       struct mmc_ios *ios)
 {
@@ -507,6 +562,19 @@ static int meson_mmc_prepare_ios_clock(struct meson_host *host,
 	return meson_mmc_clk_set(host, ios->clock, ddr);
 }
 
+static void meson_mmc_check_resampling(struct meson_host *host,
+				       struct mmc_ios *ios)
+{
+	switch (ios->timing) {
+	case MMC_TIMING_LEGACY:
+	case MMC_TIMING_MMC_HS:
+	case MMC_TIMING_SD_HS:
+	case MMC_TIMING_MMC_DDR52:
+		meson_mmc_disable_resampling(host);
+		break;
+	}
+}
+
 static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct meson_host *host = mmc_priv(mmc);
@@ -533,9 +601,6 @@ 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);
-
 		break;
 
 	case MMC_POWER_ON:
@@ -574,6 +639,7 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	val |= FIELD_PREP(CFG_BUS_WIDTH_MASK, bus_width);
 	writel(val, host->regs + SD_EMMC_CFG);
 
+	meson_mmc_check_resampling(host, ios);
 	err = meson_mmc_prepare_ios_clock(host, ios);
 	if (err)
 		dev_err(host->dev, "Failed to set clock: %d\n,", err);
@@ -963,6 +1029,7 @@ static const struct mmc_host_ops meson_mmc_ops = {
 	.get_cd         = meson_mmc_get_cd,
 	.pre_req	= meson_mmc_pre_req,
 	.post_req	= meson_mmc_post_req,
+	.execute_tuning = meson_mmc_resampling_tuning,
 	.card_busy	= meson_mmc_card_busy,
 	.start_signal_voltage_switch = meson_mmc_voltage_switch,
 };
-- 
2.20.1


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

Re: [PATCH v2 7/7] mmc: meson-gx: add signal resampling tuning

[Martin Blumenstingl]

On Tue, Apr 23, 2019 at 11:03 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> Use signal resampling tuning for the UHS and HS200 modes.
> Instead of trying to get the *best* resampling setting with complex
> window calculation, we just stop on the first working setting.
>
> If the tuning setting later proves unstable, we will just continue the
> tuning where we left it.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
[Khadas VIM now shows the HS200 eMMC]
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


BEFORE (no patches from this series applied):
# dmesg | grep mmc1
(no output)

AFTER (all 7 patches from this series applied):
# dmesg | grep mmc1
[    2.945155] mmc1: new HS200 MMC card at address 0001
[    2.957737] mmcblk1: mmc1:0001 AWPD3R 14.6 GiB
[    2.966278] mmcblk1boot0: mmc1:0001 AWPD3R partition 1 4.00 MiB
[    2.976114] mmcblk1boot1: mmc1:0001 AWPD3R partition 2 4.00 MiB
[    2.986354] mmcblk1rpmb: mmc1:0001 AWPD3R partition 3 4.00 MiB,
chardev (241:0)


Regards
Martin

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

Re: [PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Ulf Hansson]

On Tue, 23 Apr 2019 at 11:02, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> The purpose of this series is too improve reliability of the amlogic mmc
> driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)
>
> * The 3 first patches are just harmless clean ups.

Applied these first three, postponing the the rest until Martin are
happy with all of them. Thanks!

Kind regards
Uffe


> * Patch 4 makes sure HS400 can't be enabled, we still have not been able
>   to crack this modes.
> * Patch 5 removes some clock glitches when switching to DDR modes
> * Patch 6 and 7 changes the tuning method from Rx phase to signal
>   resampling. It could have been done in a single patch but the unified
>   diff was extremely ugly. The change has been split in two patches to
>   ease review.
>
> The last tuning update that went through was meant to improve the axg
> support. Since then, it was reported to break some other boards, like the
> s912 vim2.
>
> Also with the current tuning method, it was impossible to find phase
> settings which would work on all the SoCs, including the new ones.
>
> After redoing all the tests from scratch, it appeared that Rx phase made
> (strangely) almost no difference, especially on g12a and axg. However, it
> showed that it is important to have a phase shift between the Core and Tx
> clock, 180 works best.
>
> I discussed the test results with Amlogic. They suggested to use 180/0 or
> 0/180 for the Core and Tx phase. For tuning, they suggested to use
> signal resampling.
>
> So far, so good ... here the platform and modes tested:
>
> NanoPi-K2 (S905): SD UHS SDR50/DDR50, SDIO HS
> Odroid-C2 (S905): SD UHS SDR50/DDR50, eMMC DDR52/HS200 (16GB module)
> Khadas Vim (S905X): SD HS, SDIO HS, eMMC HS200
> Libretech CC (S905X): SD HS, eMMC HS200
> Khadas Vim2 (S912): SD HS, SDIO HS, eMMC HS200
> S400 (A113D): SDIO UHS SDR104, eMMC DDR52/HS200
> U200 (S905D2): SD HS, eMMC DDR52/HS200
> SEI510 (S905X2): SD HS, eMMC DDR52/HS200
>
> Changes since v1 [0]:
> * Add missing writel in patch 5 (error when switching width)
> * Change patch 3 commit description
>
> [0]: https://lkml.kernel.org/r/20190417204355.469-1-jbrunet@baylibre.com
>
> Jerome Brunet (7):
>   mmc: meson-gx: remove open coded read with timeout
>   mmc: meson-gx: ack only raised irq
>   mmc: meson-gx: correct irq flag
>   mmc: meson-gx: disable HS400
>   mmc: meson-gx: avoid clock glitch when switching to DDR modes
>   mmc: meson-gx: remove Rx phase tuning
>   mmc: meson-gx: add signal resampling tuning
>
>  drivers/mmc/host/meson-gx-mmc.c | 419 +++++++++-----------------------
>  1 file changed, 114 insertions(+), 305 deletions(-)
>
> --
> 2.20.1
>

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

Re: [PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Martin Blumenstingl]

Hi Ulf, Hi Kevin,

On Mon, Apr 29, 2019 at 12:45 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 23 Apr 2019 at 11:02, Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> > The purpose of this series is too improve reliability of the amlogic mmc
> > driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)
> >
> > * The 3 first patches are just harmless clean ups.
>
> Applied these first three, postponing the the rest until Martin are
> happy with all of them. Thanks!
thank you for taking the first three patches!
I am fine with everything except the patch description of patch 4 and
7 as noted here: [0]

Kevin, can you please also have a look at this series (if you didn't already)?
you reviewed earlier changes to the tuning mechanism in this driver.
it would be great to know that you're happy with these changes as well.


Regards
Martin


[0] http://lists.infradead.org/pipermail/linux-amlogic/2019-April/011488.html

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

Re: [PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Kevin Hilman]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hi Ulf, Hi Kevin,
>
> On Mon, Apr 29, 2019 at 12:45 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Tue, 23 Apr 2019 at 11:02, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> >
>> > The purpose of this series is too improve reliability of the amlogic mmc
>> > driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)
>> >
>> > * The 3 first patches are just harmless clean ups.
>>
>> Applied these first three, postponing the the rest until Martin are
>> happy with all of them. Thanks!
> thank you for taking the first three patches!
> I am fine with everything except the patch description of patch 4 and
> 7 as noted here: [0]
>
> Kevin, can you please also have a look at this series (if you didn't already)?
> you reviewed earlier changes to the tuning mechanism in this driver.
> it would be great to know that you're happy with these changes as well.

I've reviewed the series, and am happy with it as is, including the
changelogs as is.

Ulf, for the series, feel free to add:

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin

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

Re: [PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Martin Blumenstingl]

Hi Ulf,

On Mon, Apr 29, 2019 at 8:40 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Ulf, Hi Kevin,
>
> On Mon, Apr 29, 2019 at 12:45 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 23 Apr 2019 at 11:02, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > >
> > > The purpose of this series is too improve reliability of the amlogic mmc
> > > driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)
> > >
> > > * The 3 first patches are just harmless clean ups.
> >
> > Applied these first three, postponing the the rest until Martin are
> > happy with all of them. Thanks!
> thank you for taking the first three patches!
> I am fine with everything except the patch description of patch 4 and
> 7 as noted here: [0]
I did not understand how HS400 works. based on Jerome's latest
explanation I'm fine with the patches as they are


Martin

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

Re: [PATCH v2 0/7] mmc: meson-gx: clean up and tuning update

[Ulf Hansson]

On Mon, 29 Apr 2019 at 12:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 23 Apr 2019 at 11:02, Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> > The purpose of this series is too improve reliability of the amlogic mmc
> > driver on new (g12a) and old ones (axg, gxl, gxbb, etc...)
> >
> > * The 3 first patches are just harmless clean ups.
>
> Applied these first three, postponing the the rest until Martin are
> happy with all of them. Thanks!

Applied the rest of the series for next and by adding also Kevin's tags, thanks!

[...]

Kind regards
Uffe

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