solve SDHCI DWC MSHC 128MB DMA boundary limitation

PATCH Superseded Jisheng Zhang 3 participants 9 messages Jul 30, 2018 – Aug 15, 2018 1 message not in the archive

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Jisheng Zhang

author

2018-07-30 02:42 UTC #0 lore raw

v3 cover solve SDHCI DWC MSHC 128MB DMA boundary limitation

When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.

patch1 adds adma_table_num to struct sdhci_host so that driver can control the ADMA table number.
patch2 introduces adma_write_desc() hook to struct sdhci_ops so that driver can override it.
patch3 finally solves the 128MB boundary limitation.

since v2:
- make use of "likely" to check (!len || BOUNDARY_OK(addr, len))
- explictly include <linux/sizes.h> for SZ_128M

since v1:
- fix BOUNDARY_OK macro if addr+len is aligned to 128MB
- use DIV_ROUND_UP to cal extra desc num
- fix !len for dwcmshc_adma_write_desc()

Jisheng Zhang (3):
mmc: sdhci: add adma_table_num member to struct sdhci_host
mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops
mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 ++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c            | 48 +++++++++++++++++++----------
 drivers/mmc/host/sdhci.h            |  8 +++++
 3 files changed, 83 insertions(+), 16 deletions(-)

Signature · 1 line

2.18.0

Jisheng Zhang

author

2018-07-30 02:45 UTC #1 lore raw

PATCH v3 2/3 mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops

Add this hook so that it can be overridden with driver specific implementations. We also rename the original sdhci_adma_write_desc() to _sdhci_adma_write_desc() and export it, so that it could be reused by driver's specific implementations.

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/mmc/host/sdhci.c | 31 +++++++++++++++++++++++--------
 drivers/mmc/host/sdhci.h |  6 ++++++
 2 files changed, 29 insertions(+), 8 deletions(-)

drivers/mmc/host/sdhci.c +23 -8

index 14dd4a49e03b..50c846d99182 100644
@@ -554,8 +554,8 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)
 	local_irq_restore(*flags);
 }
 
-static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
-				  dma_addr_t addr, int len, unsigned cmd)
+unsigned int _sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
+				    dma_addr_t addr, int len, unsigned int cmd)
 {
 	struct sdhci_adma2_64_desc *dma_desc = desc;

@@ -566,6 +566,19 @@ static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
 
 	if (host->flags & SDHCI_USE_64_BIT_DMA)
 		dma_desc->addr_hi = cpu_to_le32((u64)addr >> 32);
+
+	return host->desc_sz;
+}
+EXPORT_SYMBOL_GPL(_sdhci_adma_write_desc);
+
+static unsigned int sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
+					  dma_addr_t addr, int len,
+					  unsigned int cmd)
+{
+	if (host->ops->adma_write_desc)
+		return host->ops->adma_write_desc(host, desc, addr, len, cmd);
+
+	return _sdhci_adma_write_desc(host, desc, addr, len, cmd);
 }
 
 static void sdhci_adma_mark_end(void *desc)

@@ -585,6 +598,7 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 	void *desc, *align;
 	char *buffer;
 	int len, offset, i;
+	unsigned int desc_sz;
 
 	/*
 	 * The spec does not specify endianness of descriptor table.

@@ -618,15 +632,16 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 			}
 
 			/* tran, valid */
-			sdhci_adma_write_desc(host, desc, align_addr, offset,
-					      ADMA2_TRAN_VALID);
+			desc_sz = sdhci_adma_write_desc(host, desc,
+							align_addr, offset,
+							ADMA2_TRAN_VALID);
 
 			BUG_ON(offset > 65536);
 
 			align += SDHCI_ADMA2_ALIGN;
 			align_addr += SDHCI_ADMA2_ALIGN;
 
-			desc += host->desc_sz;
+			desc += desc_sz;
 
 			addr += offset;
 			len -= offset;

@@ -636,9 +651,9 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 
 		if (len) {
 			/* tran, valid */
-			sdhci_adma_write_desc(host, desc, addr, len,
-					      ADMA2_TRAN_VALID);
-			desc += host->desc_sz;
+			desc_sz = sdhci_adma_write_desc(host, desc, addr, len,
+							ADMA2_TRAN_VALID);
+			desc += desc_sz;
 		}
 
 		/*

drivers/mmc/host/sdhci.h +6 -0

index d55fd7033e93..0aad0ee8f63b 100644
@@ -605,6 +605,9 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void    (*card_event)(struct sdhci_host *host);
 	void	(*voltage_switch)(struct sdhci_host *host);
+	unsigned int	(*adma_write_desc)(struct sdhci_host *host, void *desc,
+					   dma_addr_t addr, int len,
+					   unsigned int cmd);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

@@ -735,6 +738,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
 int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 				      struct mmc_ios *ios);
 void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable);
+unsigned int _sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
+				    dma_addr_t addr, int len,
+				    unsigned int cmd);
 
 #ifdef CONFIG_PM
 int sdhci_suspend_host(struct sdhci_host *host);

Signature · 1 line

2.18.0

Adrian Hunter

maintainer

2018-08-15 14:07 UTC #2 lore raw

Re: [PATCH mmc-next v3 2/3] mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops

Quoted 27 lines · Jisheng Zhang

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/mmc/host/sdhci.c | 31 +++++++++++++++++++++++--------
 drivers/mmc/host/sdhci.h |  6 ++++++
 2 files changed, 29 insertions(+), 8 deletions(-)

drivers/mmc/host/sdhci.c +2 -2

index 14dd4a49e03b..50c846d99182 100644
@@ -554,8 +554,8 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)
 	local_irq_restore(*flags);
 }
 
-static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
-				  dma_addr_t addr, int len, unsigned cmd)
+unsigned int _sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
+				    dma_addr_t addr, int len, unsigned int cmd)
 {
 	struct sdhci_adma2_64_desc *dma_desc = desc;

drivers/mmc/host/sdhci.c static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc, · line 572

Quoted 7 lines

drivers/mmc/host/sdhci.c +4 -0

@@ -566,6 +566,19 @@ static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
 
 	if (host->flags & SDHCI_USE_64_BIT_DMA)
 		dma_desc->addr_hi = cpu_to_le32((u64)addr >> 32);
+
+	return host->desc_sz;
+}
+EXPORT_SYMBOL_GPL(_sdhci_adma_write_desc);

Let's make the exported name without the _ i.e.sdhci_adma_write_desc

drivers/mmc/host/sdhci.c static void sdhci_adma_write_desc(struct sdhci_host *host, void *desc, · line 574

Quoted 2 lines

drivers/mmc/host/sdhci.c +2 -0

+
+static unsigned int sdhci_adma_write_desc(struct sdhci_host *host, void *desc,

Let's inline this and use __ i.e.

static inline void __sdhci_adma_write_desc(struct sdhci_host *host,

Quoted 19 lines

drivers/mmc/host/sdhci.c +8 -0

+					  dma_addr_t addr, int len,
+					  unsigned int cmd)
+{
+	if (host->ops->adma_write_desc)
+		return host->ops->adma_write_desc(host, desc, addr, len, cmd);
+
+	return _sdhci_adma_write_desc(host, desc, addr, len, cmd);
 }
 
 static void sdhci_adma_mark_end(void *desc)

@@ -585,6 +598,7 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 	void *desc, *align;
 	char *buffer;
 	int len, offset, i;
+	unsigned int desc_sz;
 
 	/*
 	 * The spec does not specify endianness of descriptor table.

drivers/mmc/host/sdhci.c static void sdhci_adma_table_pre(struct sdhci_host *host, · line 635

Quoted 6 lines

drivers/mmc/host/sdhci.c +1 -2

@@ -618,15 +632,16 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 			}
 
 			/* tran, valid */
-			sdhci_adma_write_desc(host, desc, align_addr, offset,
-					      ADMA2_TRAN_VALID);
+			desc_sz = sdhci_adma_write_desc(host, desc,

Instead of returning desc_sz, might just as well pass &desc and have the function set its new value.

Quoted 51 lines

drivers/mmc/host/sdhci.c +6 -4

+							align_addr, offset,
+							ADMA2_TRAN_VALID);
 
 			BUG_ON(offset > 65536);
 
 			align += SDHCI_ADMA2_ALIGN;
 			align_addr += SDHCI_ADMA2_ALIGN;
 
-			desc += host->desc_sz;
+			desc += desc_sz;
 
 			addr += offset;
 			len -= offset;

@@ -636,9 +651,9 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
 
 		if (len) {
 			/* tran, valid */
-			sdhci_adma_write_desc(host, desc, addr, len,
-					      ADMA2_TRAN_VALID);
-			desc += host->desc_sz;
+			desc_sz = sdhci_adma_write_desc(host, desc, addr, len,
+							ADMA2_TRAN_VALID);
+			desc += desc_sz;
 		}
 
 		/*

drivers/mmc/host/sdhci.h +6 -0

index d55fd7033e93..0aad0ee8f63b 100644
@@ -605,6 +605,9 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void    (*card_event)(struct sdhci_host *host);
 	void	(*voltage_switch)(struct sdhci_host *host);
+	unsigned int	(*adma_write_desc)(struct sdhci_host *host, void *desc,
+					   dma_addr_t addr, int len,
+					   unsigned int cmd);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

@@ -735,6 +738,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
 int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 				      struct mmc_ios *ios);
 void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable);
+unsigned int _sdhci_adma_write_desc(struct sdhci_host *host, void *desc,
+				    dma_addr_t addr, int len,
+				    unsigned int cmd);
 
 #ifdef CONFIG_PM
 int sdhci_suspend_host(struct sdhci_host *host);

Jisheng Zhang

author

2018-07-30 02:46 UTC #3 lore raw

PATCH v3 3/3 mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

In reply to Jisheng Zhang

When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

drivers/mmc/host/sdhci-of-dwcmshc.c +43 -0

index 1b7cd144fb01..e890fc8f5284 100644
@@ -8,21 +8,52 @@
  */
 
 #include <linux/clk.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/sizes.h>
 
 #include "sdhci-pltfm.h"
 
+#define BOUNDARY_OK(addr, len) \
+	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
+
 struct dwcmshc_priv {
 	struct clk	*bus_clk;
 };
 
+/*
+ * if DMA addr spans 128MB boundary, we split the DMA transfer into two
+ * so that the DMA transfer doesn't exceed the boundary.
+ */
+static unsigned int dwcmshc_adma_write_desc(struct sdhci_host *host,
+					    void *desc, dma_addr_t addr,
+					    int len, unsigned int cmd)
+{
+	int tmplen, offset;
+
+	if (likely(!len || BOUNDARY_OK(addr, len)))
+		return _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+	offset = addr & (SZ_128M - 1);
+	tmplen = SZ_128M - offset;
+	_sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
+
+	addr += tmplen;
+	len -= tmplen;
+	desc += host->desc_sz;
+	_sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+	return host->desc_sz * 2;
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
 	.set_clock		= sdhci_set_clock,
 	.set_bus_width		= sdhci_set_bus_width,
 	.set_uhs_signaling	= sdhci_set_uhs_signaling,
 	.get_max_clock		= sdhci_pltfm_clk_get_max_clock,
 	.reset			= sdhci_reset,
+	.adma_write_desc	= dwcmshc_adma_write_desc,
 };
 
 static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {

@@ -36,12 +67,24 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	struct sdhci_host *host;
 	struct dwcmshc_priv *priv;
 	int err;
+	u32 extra;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_dwcmshc_pdata,
 				sizeof(struct dwcmshc_priv));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
+	/*
+	 * The DMA descriptor table number is calculated as the maximum
+	 * number of segments times 2, to allow for an alignment
+	 * descriptor for each segment, plus 1 for a nop end descriptor,
+	 * plus extra number for cross 128M boundary handling.
+	 */
+	extra = DIV_ROUND_UP(totalram_pages, SZ_128M / PAGE_SIZE);
+	if (extra > SDHCI_MAX_SEGS)
+		extra = SDHCI_MAX_SEGS;
+	host->adma_table_num = SDHCI_MAX_SEGS * 2 + 1 + extra;
+
 	pltfm_host = sdhci_priv(host);
 	priv = sdhci_pltfm_priv(pltfm_host);

Signature · 1 line

2.18.0

Matthew Leon

2018-07-30 02:56 UTC #4 lore raw

Re: [PATCH mmc-next v3 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

Hey Jisheng,

Shouldn't we be splitting until all DMA blocks are less than 128M boundary? I am a noob, but I think we should be prepared for boundaries that when split in two, will still be greater than 128M. Feel free to disagree but please explain why I may be wrong. Thank-you.

Sincerely,
Matthew Leon

On Sun, Jul 29, 2018 at 10:46 PM, Jisheng Zhang <[email protected]

Quoted 97 lines

wrote:

When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

drivers/mmc/host/sdhci-of-dwcmshc.c +43 -1

index 1b7cd144fb01..e890fc8f5284 100644
@@ -8,21 +8,52 @@
  */
 
 #include <linux/clk.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/sizes.h>
 
 #include "sdhci-pltfm.h"
 
+#define BOUNDARY_OK(addr, len) \
+       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
+
 struct dwcmshc_priv {
        struct clk      *bus_clk;
 };
 
+/*
+ * if DMA addr spans 128MB boundary, we split the DMA transfer into two
+ * so that the DMA transfer doesn't exceed the boundary.
+ */
+static unsigned int dwcmshc_adma_write_desc(struct sdhci_host *host,
+                                           void *desc, dma_addr_t addr,
+                                           int len, unsigned int cmd)
+{
+       int tmplen, offset;
+
+       if (likely(!len || BOUNDARY_OK(addr, len)))
+               return _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+       offset = addr & (SZ_128M - 1);
+       tmplen = SZ_128M - offset;
+       _sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
+
+       addr += tmplen;
+       len -= tmplen;
+       desc += host->desc_sz;
+       _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+       return host->desc_sz * 2;
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
        .set_clock              = sdhci_set_clock,
        .set_bus_width          = sdhci_set_bus_width,
        .set_uhs_signaling      = sdhci_set_uhs_signaling,
        .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
        .reset                  = sdhci_reset,
+       .adma_write_desc        = dwcmshc_adma_write_desc,
 };
 
 static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {

@@ -36,12 +67,24 @@ static int dwcmshc_probe(struct platform_device *pdev)
        struct sdhci_host *host;
        struct dwcmshc_priv *priv;
        int err;
+       u32 extra;
 
        host = sdhci_pltfm_init(pdev, &sdhci_dwcmshc_pdata,
                                sizeof(struct dwcmshc_priv));
        if (IS_ERR(host))
                return PTR_ERR(host);
 
+       /*
+        * The DMA descriptor table number is calculated as the maximum
+        * number of segments times 2, to allow for an alignment
+        * descriptor for each segment, plus 1 for a nop end descriptor,
+        * plus extra number for cross 128M boundary handling.
+        */
+       extra = DIV_ROUND_UP(totalram_pages, SZ_128M / PAGE_SIZE);
+       if (extra > SDHCI_MAX_SEGS)
+               extra = SDHCI_MAX_SEGS;
+       host->adma_table_num = SDHCI_MAX_SEGS * 2 + 1 + extra;
+
        pltfm_host = sdhci_priv(host);
        priv = sdhci_pltfm_priv(pltfm_host);
 
--

2.18.0

Jisheng Zhang

author

2018-07-30 02:59 UTC #5 lore raw

Re: [PATCH mmc-next v3 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

Quoted 1 line · On Mon, 30 Jul 2018 02:56:20 +0000 Matthew Leon <[email protected]> wrote:

Hey Jisheng,

Hi,

Quoted 1 line

In LKML, we'd better not top post.

Quoted 4 lines

Shouldn't we be splitting until all DMA blocks are less than 128M boundary? I am a noob, but I think we should be prepared for boundaries that when split in two, will still be greater than 128M. Feel free to disagree but please explain why I may be wrong. Thank-you.

the limitation is "DMA addr can't span 128MB boundary" rather than "must be less than 128MB", they are different.

And the max transfer size of one DMA desc is 64KB.

thanks

Quoted 102 lines

Sincerely,
Matthew Leon

On Sun, Jul 29, 2018 at 10:46 PM, Jisheng Zhang <[email protected]

Quoted 97 lines

wrote:

When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

drivers/mmc/host/sdhci-of-dwcmshc.c +43 -1

index 1b7cd144fb01..e890fc8f5284 100644
@@ -8,21 +8,52 @@
  */
 
 #include <linux/clk.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/sizes.h>
 
 #include "sdhci-pltfm.h"
 
+#define BOUNDARY_OK(addr, len) \
+       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
+
 struct dwcmshc_priv {
        struct clk      *bus_clk;
 };
 
+/*
+ * if DMA addr spans 128MB boundary, we split the DMA transfer into two
+ * so that the DMA transfer doesn't exceed the boundary.
+ */
+static unsigned int dwcmshc_adma_write_desc(struct sdhci_host *host,
+                                           void *desc, dma_addr_t addr,
+                                           int len, unsigned int cmd)
+{
+       int tmplen, offset;
+
+       if (likely(!len || BOUNDARY_OK(addr, len)))
+               return _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+       offset = addr & (SZ_128M - 1);
+       tmplen = SZ_128M - offset;
+       _sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
+
+       addr += tmplen;
+       len -= tmplen;
+       desc += host->desc_sz;
+       _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+       return host->desc_sz * 2;
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
        .set_clock              = sdhci_set_clock,
        .set_bus_width          = sdhci_set_bus_width,
        .set_uhs_signaling      = sdhci_set_uhs_signaling,
        .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
        .reset                  = sdhci_reset,
+       .adma_write_desc        = dwcmshc_adma_write_desc,
 };
 
 static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {

@@ -36,12 +67,24 @@ static int dwcmshc_probe(struct platform_device *pdev)
        struct sdhci_host *host;
        struct dwcmshc_priv *priv;
        int err;
+       u32 extra;
 
        host = sdhci_pltfm_init(pdev, &sdhci_dwcmshc_pdata,
                                sizeof(struct dwcmshc_priv));
        if (IS_ERR(host))
                return PTR_ERR(host);
 
+       /*
+        * The DMA descriptor table number is calculated as the maximum
+        * number of segments times 2, to allow for an alignment
+        * descriptor for each segment, plus 1 for a nop end descriptor,
+        * plus extra number for cross 128M boundary handling.
+        */
+       extra = DIV_ROUND_UP(totalram_pages, SZ_128M / PAGE_SIZE);
+       if (extra > SDHCI_MAX_SEGS)
+               extra = SDHCI_MAX_SEGS;
+       host->adma_table_num = SDHCI_MAX_SEGS * 2 + 1 + extra;
+
        pltfm_host = sdhci_priv(host);
        priv = sdhci_pltfm_priv(pltfm_host);
 
--

2.18.0

A message that is not in the archive (sent off-list or not yet received): 01010164e92bcc1a-c3ac69eb-f6e5-4420-ac87-de1a0ffd0ac5-000000@us-west-2.amazonses.com

Jisheng Zhang

author

2018-07-30 03:11 UTC #7 lore raw

Re: [PATCH mmc-next v3 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

Quoted 19 lines · On Mon, 30 Jul 2018 03:11:59 +0000 Matthew Leon <[email protected]> wrote:

Quoted 7 lines
Quoted 1 line
Hey Jisheng,
Hi,
Quoted 1 line
In LKML, we'd better not top post.
Noted. My apologies.
Quoted 1 line
Quoted 1 line
Shouldn't we be splitting until all DMA blocks are less than 128M
boundary?
Quoted 6 lines
Quoted 3 lines
I am a noob, but I think we should be prepared for boundaries that when split in two, will still be greater than 128M. Feel free to disagree but please explain why I may be wrong. Thank-you.
the limitation is "DMA addr can't span 128MB boundary" rather than "must be less than 128MB", they are different.

Quoted 8 lines

Quoted 3 lines
And the max transfer size of one DMA desc is 64KB.
thanks
I have misspoken. What if the DMA transfer size is 1024M? If we split in two, then we have 2 transfers, each of which span 512M. So wouldn't we need to split again to have 4 transfers, each of which span 128M?

the max transfer size of each desc is 64KB, how could it be 1024MB?

Matthew Leon

2018-07-30 03:11 UTC #8 lore raw

Re: [PATCH mmc-next v3 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

In reply to Jisheng Zhang

Quoted 7 lines

Quoted 1 line
Hey Jisheng,
Hi,
Quoted 1 line
In LKML, we'd better not top post.

Noted. My apologies.

Quoted 1 line

Quoted 1 line
Shouldn't we be splitting until all DMA blocks are less than 128M

boundary?

Quoted 10 lines

Quoted 3 lines
I am a noob, but I think we should be prepared for boundaries that when split in two, will still be greater than 128M. Feel free to disagree but please explain why I may be wrong. Thank-you.
the limitation is "DMA addr can't span 128MB boundary" rather than "must be less than 128MB", they are different.
And the max transfer size of one DMA desc is 64KB.
thanks

I have misspoken. What if the DMA transfer size is 1024M? If we split in two, then we have 2 transfers, each of which span 512M. So wouldn't we need to split again to have 4 transfers, each of which span 128M?

Sincerely,
Matthew Leon

On Sun, Jul 29, 2018 at 10:59 PM, Jisheng Zhang <[email protected]

Quoted 137 lines

wrote:
Quoted 1 line · On Mon, 30 Jul 2018 02:56:20 +0000 Matthew Leon <[email protected]> wrote:
Hey Jisheng,
Hi,
Quoted 1 line
In LKML, we'd better not top post.
Quoted 1 line
Shouldn't we be splitting until all DMA blocks are less than 128M
boundary?
Quoted 3 lines
I am a noob, but I think we should be prepared for boundaries that when split in two, will still be greater than 128M. Feel free to disagree but please explain why I may be wrong. Thank-you.
the limitation is "DMA addr can't span 128MB boundary" rather than "must be less than 128MB", they are different.
And the max transfer size of one DMA desc is 64KB.
thanks
Quoted 5 lines
Sincerely,
Matthew Leon
On Sun, Jul 29, 2018 at 10:46 PM, Jisheng Zhang <
[email protected]
Quoted 8 lines
Quoted 8 lines
wrote:
When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.
Signed-off-by Jisheng Zhang <[email protected]>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43
+++++++++++++++++++++++++++++
Quoted 25 lines
Quoted 25 lines
 1 file changed, 43 insertions(+)
drivers/mmc/host/sdhci-of-dwcmshc.c +5 -0
index 1b7cd144fb01..e890fc8f5284 100644
@@ -8,21 +8,52 @@
  */
 
 #include <linux/clk.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/sizes.h>
 
 #include "sdhci-pltfm.h"
 
+#define BOUNDARY_OK(addr, len) \
+       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
+
 struct dwcmshc_priv {
        struct clk      *bus_clk;
 };
 
Quoted 2 lines
drivers/mmc/host/sdhci-of-dwcmshc.c line 26
Quoted 2 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +2 -0
+/*
+ * if DMA addr spans 128MB boundary, we split the DMA transfer into
two
Quoted 4 lines
drivers/mmc/host/sdhci-of-dwcmshc.c
Quoted 4 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +4 -0
+ * so that the DMA transfer doesn't exceed the boundary.
+ */
+static unsigned int dwcmshc_adma_write_desc(struct sdhci_host *host,
+                                           void *desc, dma_addr_t
addr,
Quoted 6 lines
drivers/mmc/host/sdhci-of-dwcmshc.c
Quoted 6 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +6 -0
+                                           int len, unsigned int cmd)
+{
+       int tmplen, offset;
+
+       if (likely(!len || BOUNDARY_OK(addr, len)))
+               return _sdhci_adma_write_desc(host, desc, addr, len,
cmd);
Quoted 22 lines
Quoted 22 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +14 -0
+
+       offset = addr & (SZ_128M - 1);
+       tmplen = SZ_128M - offset;
+       _sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
+
+       addr += tmplen;
+       len -= tmplen;
+       desc += host->desc_sz;
+       _sdhci_adma_write_desc(host, desc, addr, len, cmd);
+
+       return host->desc_sz * 2;
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
        .set_clock              = sdhci_set_clock,
        .set_bus_width          = sdhci_set_bus_width,
        .set_uhs_signaling      = sdhci_set_uhs_signaling,
        .get_max_clock          = sdhci_pltfm_clk_get_max_clock,
        .reset                  = sdhci_reset,
+       .adma_write_desc        = dwcmshc_adma_write_desc,
 };
 
Quoted 2 lines
drivers/mmc/host/sdhci-of-dwcmshc.c static int dwcmshc_probe(struct platform_device
Quoted 2 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +0 -0
 static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
@@ -36,12 +67,24 @@ static int dwcmshc_probe(struct platform_device
*pdev)
Quoted 10 lines
Quoted 10 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +1 -0
        struct sdhci_host *host;
        struct dwcmshc_priv *priv;
        int err;
+       u32 extra;
 
        host = sdhci_pltfm_init(pdev, &sdhci_dwcmshc_pdata,
                                sizeof(struct dwcmshc_priv));
        if (IS_ERR(host))
                return PTR_ERR(host);
 
Quoted 4 lines
drivers/mmc/host/sdhci-of-dwcmshc.c static int dwcmshc_probe(struct platform_device *pdev)
Quoted 4 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +4 -0
+       /*
+        * The DMA descriptor table number is calculated as the maximum
+        * number of segments times 2, to allow for an alignment
+        * descriptor for each segment, plus 1 for a nop end
descriptor,
Quoted 14 lines
Quoted 14 lines
drivers/mmc/host/sdhci-of-dwcmshc.c +7 -1
+        * plus extra number for cross 128M boundary handling.
+        */
+       extra = DIV_ROUND_UP(totalram_pages, SZ_128M / PAGE_SIZE);
+       if (extra > SDHCI_MAX_SEGS)
+               extra = SDHCI_MAX_SEGS;
+       host->adma_table_num = SDHCI_MAX_SEGS * 2 + 1 + extra;
+
        pltfm_host = sdhci_priv(host);
        priv = sdhci_pltfm_priv(pltfm_host);
 
--
2.18.0

Jisheng Zhang

author

2018-08-09 02:27 UTC #9 lore raw

In reply to Jisheng Zhang

Hi Adrian, Ulf,

could you please review these patches? Any comments are welcome.

Thanks in advance,
Jisheng

Quoted 28 lines · On Mon, 30 Jul 2018 10:42:28 +0800 Jisheng Zhang wrote:

When using DMA, if the DMA addr spans 128MB boundary, we have to split the DMA transfer into two so that each one doesn't exceed the boundary.
patch1 adds adma_table_num to struct sdhci_host so that driver can control the ADMA table number.
patch2 introduces adma_write_desc() hook to struct sdhci_ops so that driver can override it.
patch3 finally solves the 128MB boundary limitation.
since v2:
- make use of "likely" to check (!len || BOUNDARY_OK(addr, len))
- explictly include <linux/sizes.h> for SZ_128M
since v1:
- fix BOUNDARY_OK macro if addr+len is aligned to 128MB
- use DIV_ROUND_UP to cal extra desc num
- fix !len for dwcmshc_adma_write_desc()
Jisheng Zhang (3):
mmc: sdhci: add adma_table_num member to struct sdhci_host
mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops
mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 ++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c            | 48 +++++++++++++++++++----------
 drivers/mmc/host/sdhci.h            |  8 +++++
 3 files changed, 83 insertions(+), 16 deletions(-)