diff mbox

[v1,1/1] mmc: sdhci-esdhc-imx: Set maximum watermark levels for PIO access

Message ID 1523970912-20269-2-git-send-email-harish_kandiga@mentor.com (mailing list archive)
State New, archived
Headers show

Commit Message

Harish Jenny K N April 17, 2018, 1:15 p.m. UTC 
From: Andrew Gabbasov <andrew_gabbasov@mentor.com>

While performing R/W access in PIO mode, the common SDHCI driver checks
the buffer ready status once per whole block processing. That is, after
getting an appropriate interrupt, or checking an appropriate status bit,
the driver makes buffer accesses for the whole block size (e.g. 128 reads
for 512 bytes block). This is done in accordance with SD Host Controller
Specification.

At the same time, the Ultra Secured Digital Host Controller (uSDHC), used
in i.MX6 (and, probably, earlier i.MX series too), uses a separate
Watermark Levels register, controlling the amount of data or space
available when raising status bit or interrupt. For default watermark
setting of 16 words, the controller expects (and guarantees) no more
than 16 buffer accesses after raising buffer ready status bit and
generating an appropriate interrupt. If the driver tries to access the
whole block size, it will get incorrect data at the end, and a new
interrupt will appear later, when the driver already doesn't expect it.
This happens sometimes, more likely on low frequencies, e.g. when
reading EXT_CSD at MMC card initialization phase
(which makes that initialization fail).

Such behavior of i.MX uSDHC seems to be non-compliant
to SDHCI Specification, but this is the way it works now.

In order not to rewrite the SDHCI driver PIO mode access logic,
the IMX specific driver can just set the watermark level to default
block size (128 words or 512 bytes), so that the controller behavior
will be consistent to generic specification. This patch does this
for PIO mode accesses only, restoring default values for DMA accesses
to avoid any possible side effects from performance point of view.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Adrian Hunter April 26, 2018, 11:05 a.m. UTC | #1 
On 17/04/18 16:15, Harish Jenny K N wrote:
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> 
> While performing R/W access in PIO mode, the common SDHCI driver checks
> the buffer ready status once per whole block processing. That is, after
> getting an appropriate interrupt, or checking an appropriate status bit,
> the driver makes buffer accesses for the whole block size (e.g. 128 reads
> for 512 bytes block). This is done in accordance with SD Host Controller
> Specification.
> 
> At the same time, the Ultra Secured Digital Host Controller (uSDHC), used
> in i.MX6 (and, probably, earlier i.MX series too), uses a separate
> Watermark Levels register, controlling the amount of data or space
> available when raising status bit or interrupt. For default watermark
> setting of 16 words, the controller expects (and guarantees) no more
> than 16 buffer accesses after raising buffer ready status bit and
> generating an appropriate interrupt. If the driver tries to access the
> whole block size, it will get incorrect data at the end, and a new
> interrupt will appear later, when the driver already doesn't expect it.
> This happens sometimes, more likely on low frequencies, e.g. when
> reading EXT_CSD at MMC card initialization phase
> (which makes that initialization fail).
> 
> Such behavior of i.MX uSDHC seems to be non-compliant
> to SDHCI Specification, but this is the way it works now.
> 
> In order not to rewrite the SDHCI driver PIO mode access logic,
> the IMX specific driver can just set the watermark level to default
> block size (128 words or 512 bytes), so that the controller behavior
> will be consistent to generic specification. This patch does this
> for PIO mode accesses only, restoring default values for DMA accesses
> to avoid any possible side effects from performance point of view.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index cd2b5f6..d6aef70 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -41,6 +41,12 @@
>  #define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON	(1 << 8)
>  #define ESDHC_WTMK_LVL			0x44
>  #define  ESDHC_WTMK_DEFAULT_VAL		0x10401040
> +#define  ESDHC_WTMK_LVL_RD_WML_MASK	0x000000FF
> +#define  ESDHC_WTMK_LVL_RD_WML_SHIFT	0
> +#define  ESDHC_WTMK_LVL_WR_WML_MASK	0x00FF0000
> +#define  ESDHC_WTMK_LVL_WR_WML_SHIFT	16
> +#define  ESDHC_WTMK_LVL_WML_VAL_DEF	64
> +#define  ESDHC_WTMK_LVL_WML_VAL_MAX	128
>  #define ESDHC_MIX_CTRL			0x48
>  #define  ESDHC_MIX_CTRL_DDREN		(1 << 3)
>  #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
> @@ -516,6 +522,7 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>  		}
>  
>  		if (esdhc_is_usdhc(imx_data)) {
> +			u32 wml;
>  			u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL);
>  			/* Swap AC23 bit */
>  			if (val & SDHCI_TRNS_AUTO_CMD23) {
> @@ -524,6 +531,21 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>  			}
>  			m = val | (m & ~ESDHC_MIX_CTRL_SDHCI_MASK);
>  			writel(m, host->ioaddr + ESDHC_MIX_CTRL);
> +
> +			/* Set watermark levels for PIO access to maximum value
> +			 * (128 words) to accommodate full 512 bytes buffer.
> +			 * For DMA access restore the levels to default value.
> +			 */
> +			m = readl(host->ioaddr + ESDHC_WTMK_LVL);
> +			if (val & SDHCI_TRNS_DMA)
> +				wml = ESDHC_WTMK_LVL_WML_VAL_DEF;
> +			else
> +				wml = ESDHC_WTMK_LVL_WML_VAL_MAX;
> +			m &= ~(ESDHC_WTMK_LVL_RD_WML_MASK |
> +			       ESDHC_WTMK_LVL_WR_WML_MASK);
> +			m |= (wml << ESDHC_WTMK_LVL_RD_WML_SHIFT) |
> +			     (wml << ESDHC_WTMK_LVL_WR_WML_SHIFT);
> +			writel(m, host->ioaddr + ESDHC_WTMK_LVL);
>  		} else {
>  			/*
>  			 * Postpone this write, we must do it together with a
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson April 27, 2018, 12:05 p.m. UTC | #2 
On 17 April 2018 at 15:15, Harish Jenny K N <harish_kandiga@mentor.com> wrote:
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
>
> While performing R/W access in PIO mode, the common SDHCI driver checks
> the buffer ready status once per whole block processing. That is, after
> getting an appropriate interrupt, or checking an appropriate status bit,
> the driver makes buffer accesses for the whole block size (e.g. 128 reads
> for 512 bytes block). This is done in accordance with SD Host Controller
> Specification.
>
> At the same time, the Ultra Secured Digital Host Controller (uSDHC), used
> in i.MX6 (and, probably, earlier i.MX series too), uses a separate
> Watermark Levels register, controlling the amount of data or space
> available when raising status bit or interrupt. For default watermark
> setting of 16 words, the controller expects (and guarantees) no more
> than 16 buffer accesses after raising buffer ready status bit and
> generating an appropriate interrupt. If the driver tries to access the
> whole block size, it will get incorrect data at the end, and a new
> interrupt will appear later, when the driver already doesn't expect it.
> This happens sometimes, more likely on low frequencies, e.g. when
> reading EXT_CSD at MMC card initialization phase
> (which makes that initialization fail).
>
> Such behavior of i.MX uSDHC seems to be non-compliant
> to SDHCI Specification, but this is the way it works now.
>
> In order not to rewrite the SDHCI driver PIO mode access logic,
> the IMX specific driver can just set the watermark level to default
> block size (128 words or 512 bytes), so that the controller behavior
> will be consistent to generic specification. This patch does this
> for PIO mode accesses only, restoring default values for DMA accesses
> to avoid any possible side effects from performance point of view.
>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index cd2b5f6..d6aef70 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -41,6 +41,12 @@
>  #define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON        (1 << 8)
>  #define ESDHC_WTMK_LVL                 0x44
>  #define  ESDHC_WTMK_DEFAULT_VAL                0x10401040
> +#define  ESDHC_WTMK_LVL_RD_WML_MASK    0x000000FF
> +#define  ESDHC_WTMK_LVL_RD_WML_SHIFT   0
> +#define  ESDHC_WTMK_LVL_WR_WML_MASK    0x00FF0000
> +#define  ESDHC_WTMK_LVL_WR_WML_SHIFT   16
> +#define  ESDHC_WTMK_LVL_WML_VAL_DEF    64
> +#define  ESDHC_WTMK_LVL_WML_VAL_MAX    128
>  #define ESDHC_MIX_CTRL                 0x48
>  #define  ESDHC_MIX_CTRL_DDREN          (1 << 3)
>  #define  ESDHC_MIX_CTRL_AC23EN         (1 << 7)
> @@ -516,6 +522,7 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>                 }
>
>                 if (esdhc_is_usdhc(imx_data)) {
> +                       u32 wml;
>                         u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL);
>                         /* Swap AC23 bit */
>                         if (val & SDHCI_TRNS_AUTO_CMD23) {
> @@ -524,6 +531,21 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>                         }
>                         m = val | (m & ~ESDHC_MIX_CTRL_SDHCI_MASK);
>                         writel(m, host->ioaddr + ESDHC_MIX_CTRL);
> +
> +                       /* Set watermark levels for PIO access to maximum value
> +                        * (128 words) to accommodate full 512 bytes buffer.
> +                        * For DMA access restore the levels to default value.
> +                        */
> +                       m = readl(host->ioaddr + ESDHC_WTMK_LVL);
> +                       if (val & SDHCI_TRNS_DMA)
> +                               wml = ESDHC_WTMK_LVL_WML_VAL_DEF;
> +                       else
> +                               wml = ESDHC_WTMK_LVL_WML_VAL_MAX;
> +                       m &= ~(ESDHC_WTMK_LVL_RD_WML_MASK |
> +                              ESDHC_WTMK_LVL_WR_WML_MASK);
> +                       m |= (wml << ESDHC_WTMK_LVL_RD_WML_SHIFT) |
> +                            (wml << ESDHC_WTMK_LVL_WR_WML_SHIFT);
> +                       writel(m, host->ioaddr + ESDHC_WTMK_LVL);
>                 } else {
>                         /*
>                          * Postpone this write, we must do it together with a
> --
> 1.9.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index cd2b5f6..d6aef70 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -41,6 +41,12 @@ 
 #define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON	(1 << 8)
 #define ESDHC_WTMK_LVL			0x44
 #define  ESDHC_WTMK_DEFAULT_VAL		0x10401040
+#define  ESDHC_WTMK_LVL_RD_WML_MASK	0x000000FF
+#define  ESDHC_WTMK_LVL_RD_WML_SHIFT	0
+#define  ESDHC_WTMK_LVL_WR_WML_MASK	0x00FF0000
+#define  ESDHC_WTMK_LVL_WR_WML_SHIFT	16
+#define  ESDHC_WTMK_LVL_WML_VAL_DEF	64
+#define  ESDHC_WTMK_LVL_WML_VAL_MAX	128
 #define ESDHC_MIX_CTRL			0x48
 #define  ESDHC_MIX_CTRL_DDREN		(1 << 3)
 #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
@@ -516,6 +522,7 @@  static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 		}
 
 		if (esdhc_is_usdhc(imx_data)) {
+			u32 wml;
 			u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL);
 			/* Swap AC23 bit */
 			if (val & SDHCI_TRNS_AUTO_CMD23) {
@@ -524,6 +531,21 @@  static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 			}
 			m = val | (m & ~ESDHC_MIX_CTRL_SDHCI_MASK);
 			writel(m, host->ioaddr + ESDHC_MIX_CTRL);
+
+			/* Set watermark levels for PIO access to maximum value
+			 * (128 words) to accommodate full 512 bytes buffer.
+			 * For DMA access restore the levels to default value.
+			 */
+			m = readl(host->ioaddr + ESDHC_WTMK_LVL);
+			if (val & SDHCI_TRNS_DMA)
+				wml = ESDHC_WTMK_LVL_WML_VAL_DEF;
+			else
+				wml = ESDHC_WTMK_LVL_WML_VAL_MAX;
+			m &= ~(ESDHC_WTMK_LVL_RD_WML_MASK |
+			       ESDHC_WTMK_LVL_WR_WML_MASK);
+			m |= (wml << ESDHC_WTMK_LVL_RD_WML_SHIFT) |
+			     (wml << ESDHC_WTMK_LVL_WR_WML_SHIFT);
+			writel(m, host->ioaddr + ESDHC_WTMK_LVL);
 		} else {
 			/*
 			 * Postpone this write, we must do it together with a