Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1634851 > unrolled thread

[PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

Started byBenoît Thébaudeau <benoit@wsystem.com>
First post2017-05-03 12:20 +0200
Last post2017-05-04 11:40 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR Benoît Thébaudeau <benoit@wsystem.com> - 2017-05-03 12:20 +0200
    [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround Benoît Thébaudeau <benoit@wsystem.com> - 2017-05-03 12:20 +0200
    [PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values Benoît Thébaudeau <benoit@wsystem.com> - 2017-05-03 12:20 +0200
    Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR Arnd Bergmann <arnd@arndb.de> - 2017-05-04 10:50 +0200
      Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR Benoît Thébaudeau <benoit@wsystem.com> - 2017-05-04 11:10 +0200
        Re: [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR Arnd Bergmann <arnd@arndb.de> - 2017-05-04 11:40 +0200

#1634851 — [PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR

FromBenoît Thébaudeau <benoit@wsystem.com>
Date2017-05-03 12:20 +0200
Subject[PATCH 1/4] mmc: sdhci-esdhc: Add SDHCI_QUIRK_32BIT_DMA_ADDR
Message-ID<tCZy1-3Rz-3@gated-at.bofh.it>
The eSDHC can only DMA from 32-bit-aligned addresses.

This fixes the following test cases of mmc_test:
  11:	Badly aligned write
  12:	Badly aligned read
  13:	Badly aligned multi-block write
  14:	Badly aligned multi-block read

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index c4bbd74..e7893f2 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -19,6 +19,7 @@
  */
 
 #define ESDHC_DEFAULT_QUIRKS	(SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
+				SDHCI_QUIRK_32BIT_DMA_ADDR | \
 				SDHCI_QUIRK_NO_BUSY_IRQ | \
 				SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
 				SDHCI_QUIRK_PIO_NEEDS_DELAY | \
-- 
2.7.4

[toc] | [next] | [standalone]

#1634853 — [PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround

FromBenoît Thébaudeau <benoit@wsystem.com>
Date2017-05-03 12:20 +0200
Subject[PATCH 4/4] mmc: sdhci-esdhc-imx: Remove the ENGcm07207 workaround
Message-ID<tCZy1-3Rz-9@gated-at.bofh.it>
In reply to#1634851
The SDHCI_QUIRK_NO_MULTIBLOCK quirk was used as a workaround for the
ENGcm07207 erratum. However, it caused excruciatingly slow SD transfers
(300 kB/s on average), and this erratum actually does not imply that
multiple-block transfers are not supported, so this was overkill.

The suggested workaround for this erratum is to set SYSCTL.RSTA, but the
simple DAT line software reset (which resets the DMA circuit among
others) triggered by sdhci_finish_data() in case of errors seems to be
sufficient. Indeed, generating errors in a controlled manner on i.MX25
using the FEVT register right in the middle of read data transfers
without this quirk shows that nothing is written to the buffer by the
eSDHC past CMD12, and no extra Auto CMD12 is sent with AC12EN set, so
the data transfers on AHB are properly aborted. For write data
transfers, neither extra data nor extra Auto CMD12 is sent, as expected.
Moreover, after intensive stress tests on i.MX25, removing
SDHCI_QUIRK_NO_MULTIBLOCK seems to be safe.

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index fa60d13..868a51f 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -115,11 +115,6 @@
  */
 #define ESDHC_FLAG_MULTIBLK_NO_INT	BIT(1)
 /*
- * The flag enables the workaround for ESDHC errata ENGcm07207 which
- * affects i.MX25 and i.MX35.
- */
-#define ESDHC_FLAG_ENGCM07207		BIT(2)
-/*
  * The flag tells that the ESDHC controller is an USDHC block that is
  * integrated on the i.MX6 series.
  */
@@ -149,11 +144,11 @@ struct esdhc_soc_data {
 };
 
 static struct esdhc_soc_data esdhc_imx25_data = {
-	.flags = ESDHC_FLAG_ENGCM07207,
+	.flags = ESDHC_FLAG_ERR004536,
 };
 
 static struct esdhc_soc_data esdhc_imx35_data = {
-	.flags = ESDHC_FLAG_ENGCM07207,
+	.flags = ESDHC_FLAG_ERR004536,
 };
 
 static struct esdhc_soc_data esdhc_imx51_data = {
@@ -286,7 +281,7 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 		 * ADMA2 capability of esdhc, but this bit is messed up on
 		 * some SOCs (e.g. on MX25, MX35 this bit is set, but they
 		 * don't actually support ADMA2). So set the BROKEN_ADMA
-		 * uirk on MX25/35 platforms.
+		 * quirk on MX25/35 platforms.
 		 */
 
 		if (val & SDHCI_CAN_DO_ADMA1) {
@@ -1278,11 +1273,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (IS_ERR(imx_data->pins_default))
 		dev_warn(mmc_dev(host->mmc), "could not get default state\n");
 
-	if (imx_data->socdata->flags & ESDHC_FLAG_ENGCM07207)
-		/* Fix errata ENGcm07207 present on i.MX25 and i.MX35 */
-		host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
-			| SDHCI_QUIRK_BROKEN_ADMA;
-
 	if (esdhc_is_usdhc(imx_data)) {
 		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
 		host->mmc->caps |= MMC_CAP_1_8V_DDR;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1634854 — [PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values

FromBenoît Thébaudeau <benoit@wsystem.com>
Date2017-05-03 12:20 +0200
Subject[PATCH 3/4] mmc: sdhci-esdhc-imx: Allow all supported prescaler values
Message-ID<tCZy1-3Rz-19@gated-at.bofh.it>
In reply to#1634851
On i.MX, SYSCTL.SDCLKFS may always be set to 0 in order to make the SD
clock frequency prescaler divide by 1 in SDR mode, even with the eSDHC.
The previous minimum prescaler value of 2 in SDR mode with the eSDHC was
a code remnant from PowerPC, which actually has this limitation on
earlier revisions.

In DDR mode, the prescaler can divide by up to 512.

The maximum SD clock frequency in High Speed mode is 50 MHz. On i.MX25,
this change makes it possible to get 48 MHz from the USB PLL
(240 MHz / 5 / 1) instead of only 40 MHz from the USB PLL
(240 MHz / 3 / 2) or 33.25 MHz from the AHB clock (133 MHz / 2 / 2).

Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 4ee82e1..fa60d13 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -674,7 +674,8 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	unsigned int host_clock = pltfm_host->clock;
-	int pre_div = 2;
+	int ddr_pre_div = imx_data->is_ddr ? 2 : 1;
+	int pre_div = 1;
 	int div = 1;
 	u32 temp, val;
 
@@ -689,28 +690,23 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 		return;
 	}
 
-	if (esdhc_is_usdhc(imx_data) && !imx_data->is_ddr)
-		pre_div = 1;
-
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
 	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
 		| ESDHC_CLOCK_MASK);
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
 
-	while (host_clock / pre_div / 16 > clock && pre_div < 256)
+	while (host_clock / (16 * pre_div * ddr_pre_div) > clock &&
+			pre_div < 256)
 		pre_div *= 2;
 
-	while (host_clock / pre_div / div > clock && div < 16)
+	while (host_clock / (div * pre_div * ddr_pre_div) > clock && div < 16)
 		div++;
 
-	host->mmc->actual_clock = host_clock / pre_div / div;
+	host->mmc->actual_clock = host_clock / (div * pre_div * ddr_pre_div);
 	dev_dbg(mmc_dev(host->mmc), "desired SD clock: %d, actual: %d\n",
 		clock, host->mmc->actual_clock);
 
-	if (imx_data->is_ddr)
-		pre_div >>= 2;
-	else
-		pre_div >>= 1;
+	pre_div >>= 1;
 	div--;
 
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1635479

FromArnd Bergmann <arnd@arndb.de>
Date2017-05-04 10:50 +0200
Message-ID<tDkCt-12n-1@gated-at.bofh.it>
In reply to#1634851
On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> The eSDHC can only DMA from 32-bit-aligned addresses.
>
> This fixes the following test cases of mmc_test:
>   11:   Badly aligned write
>   12:   Badly aligned read
>   13:   Badly aligned multi-block write
>   14:   Badly aligned multi-block read
>
> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>

Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
was for devices that cannot address high memory above 0xffffffff, rather than
requiring a specific alignment.

If this is indeed an address range problem rather than an alignment problem,
are you sure it is the SD controller that is wrong here, rather than having a
64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
latter case, the dma-ranges property in the parent bus should limit
the addressing, not the device.

      Arnd

[toc] | [prev] | [next] | [standalone]

#1635513

FromBenoît Thébaudeau <benoit@wsystem.com>
Date2017-05-04 11:10 +0200
Message-ID<tDkVR-1oS-45@gated-at.bofh.it>
In reply to#1635479
On 04/05/2017 10:47, Arnd Bergmann wrote:
> On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
>> The eSDHC can only DMA from 32-bit-aligned addresses.
>>
>> This fixes the following test cases of mmc_test:
>>   11:   Badly aligned write
>>   12:   Badly aligned read
>>   13:   Badly aligned multi-block write
>>   14:   Badly aligned multi-block read
>>
>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
> 
> Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
> was for devices that cannot address high memory above 0xffffffff, rather than
> requiring a specific alignment.
> 
> If this is indeed an address range problem rather than an alignment problem,
> are you sure it is the SD controller that is wrong here, rather than having a
> 64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
> latter case, the dma-ranges property in the parent bus should limit
> the addressing, not the device.

No, this is the right description. This quirk really is about alignment, and not
about address range. See:

drivers/mmc/host/sdhci.h:
>---
/* Controller can only DMA from 32-bit aligned addresses */
#define SDHCI_QUIRK_32BIT_DMA_ADDR			(1<<7)
<---

drivers/mmc/host/sdhci.c @ sdhci_prepare_data():
>---
		offset_mask = 0;
[...]
			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
				offset_mask = 3;
[...]
				if (sg->offset & offset_mask) {
					DBG("Reverting to PIO because of bad alignment\n");
					host->flags &= ~SDHCI_REQ_USE_DMA;
					break;
				}
<---

Benoît

[toc] | [prev] | [next] | [standalone]

#1635596

FromArnd Bergmann <arnd@arndb.de>
Date2017-05-04 11:40 +0200
Message-ID<tDloR-1BT-9@gated-at.bofh.it>
In reply to#1635513
On Thu, May 4, 2017 at 11:00 AM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
> On 04/05/2017 10:47, Arnd Bergmann wrote:
>> On Wed, May 3, 2017 at 12:05 PM, Benoît Thébaudeau <benoit@wsystem.com> wrote:
>>> The eSDHC can only DMA from 32-bit-aligned addresses.
>>>
>>> This fixes the following test cases of mmc_test:
>>>   11:   Badly aligned write
>>>   12:   Badly aligned read
>>>   13:   Badly aligned multi-block write
>>>   14:   Badly aligned multi-block read
>>>
>>> Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com>
>>
>> Is this the right description? I thought that SDHCI_QUIRK_32BIT_DMA_ADDR
>> was for devices that cannot address high memory above 0xffffffff, rather than
>> requiring a specific alignment.
>>
>> If this is indeed an address range problem rather than an alignment problem,
>> are you sure it is the SD controller that is wrong here, rather than having a
>> 64-bit DMA capable  SDHCI connected to a 32-bit parent bus? In the
>> latter case, the dma-ranges property in the parent bus should limit
>> the addressing, not the device.
>
> No, this is the right description. This quirk really is about alignment, and not
> about address range. See:
>
> drivers/mmc/host/sdhci.h:
>>---
> /* Controller can only DMA from 32-bit aligned addresses */
> #define SDHCI_QUIRK_32BIT_DMA_ADDR                      (1<<7)
> <---
>
> drivers/mmc/host/sdhci.c @ sdhci_prepare_data():
>>---
>                 offset_mask = 0;
> [...]
>                         if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
>                                 offset_mask = 3;

Ok, thanks for the clarification. I guess I should have checked this myself
first.

       Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web