2018-08-23 15:59:14

by Jisheng Zhang

[permalink] [raw]
Subject: [PATCH v4 0/3] 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 v3:
- s/adma_table_num/adma_table_cnt
- add comment to adma_table_cnt
- make the exported function name without the _
- let sdhci_adma_write_desc() accept &desc param and set the new desc
value

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_cnt 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 | 39 +++++++++++++++++++++
drivers/mmc/host/sdhci.c | 54 +++++++++++++++++------------
drivers/mmc/host/sdhci.h | 7 ++++
3 files changed, 78 insertions(+), 22 deletions(-)

--
2.18.0



2018-08-23 15:59:57

by Jisheng Zhang

[permalink] [raw]
Subject: [PATCH v4 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 let the original sdhci_adma_write_desc()
accept &desc so that the function can set its new value. Then export
the function so that it could be reused by driver's specific
implementations.

Signed-off-by: Jisheng Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 37 +++++++++++++++++++++++--------------
drivers/mmc/host/sdhci.h | 4 ++++
2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 52ccf4644384..eb21d2db7f05 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -554,10 +554,10 @@ 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)
+void 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;
+ struct sdhci_adma2_64_desc *dma_desc = *desc;

/* 32-bit and 64-bit descriptors have these members in same position */
dma_desc->cmd = cpu_to_le16(cmd);
@@ -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);
+
+ *desc += host->desc_sz;
+}
+EXPORT_SYMBOL_GPL(sdhci_adma_write_desc);
+
+static inline void __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)
+ host->ops->adma_write_desc(host, desc, addr, len, cmd);
+
+ sdhci_adma_write_desc(host, desc, addr, len, cmd);
}

static void sdhci_adma_mark_end(void *desc)
@@ -618,28 +631,24 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
}

/* tran, valid */
- sdhci_adma_write_desc(host, desc, align_addr, offset,
- ADMA2_TRAN_VALID);
+ __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;
-
addr += offset;
len -= offset;
}

BUG_ON(len > 65536);

- if (len) {
- /* tran, valid */
- sdhci_adma_write_desc(host, desc, addr, len,
- ADMA2_TRAN_VALID);
- desc += host->desc_sz;
- }
+ /* tran, valid */
+ if (len)
+ __sdhci_adma_write_desc(host, &desc, addr, len,
+ ADMA2_TRAN_VALID);

/*
* If this triggers then we have a calculation bug
@@ -656,7 +665,7 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
}
} else {
/* Add a terminating entry - nop, end, valid */
- sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
+ __sdhci_adma_write_desc(host, &desc, 0, 0, ADMA2_NOP_END_VALID);
}
}

diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 25bddd21de31..2115416f973a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -606,6 +606,8 @@ 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);
+ void (*adma_write_desc)(struct sdhci_host *host, void **desc,
+ dma_addr_t addr, int len, unsigned int cmd);
};

#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
@@ -736,6 +738,8 @@ 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);
+void 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);
--
2.18.0


2018-08-23 16:00:05

by Jisheng Zhang

[permalink] [raw]
Subject: [PATCH v4 3/3] mmc: sdhci-of-dwcmshc: solve 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.

Signed-off-by: Jisheng Zhang <[email protected]>
---
drivers/mmc/host/sdhci-of-dwcmshc.c | 39 +++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 1b7cd144fb01..df0a3aeabe19 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -8,21 +8,48 @@
*/

#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 void 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)))
+ 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;
+ sdhci_adma_write_desc(host, desc, addr, len, cmd);
+}
+
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 +63,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 table descriptor count 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_cnt = SDHCI_MAX_SEGS * 2 + 1 + extra;
+
pltfm_host = sdhci_priv(host);
priv = sdhci_pltfm_priv(pltfm_host);

--
2.18.0


2018-08-23 16:00:47

by Jisheng Zhang

[permalink] [raw]
Subject: [PATCH v4 1/3] mmc: sdhci: add adma_table_cnt member to struct sdhci_host

This patch adds adma_table_cnt member to struct sdhci_host to give more
flexibility to drivers to control the ADMA table count.

Default value of adma_table_cnt is set to (SDHCI_MAX_SEGS * 2 + 1).

Signed-off-by: Jisheng Zhang <[email protected]>
---
drivers/mmc/host/sdhci.c | 17 +++++++++--------
drivers/mmc/host/sdhci.h | 3 +++
2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1b3fbd9bd5c5..52ccf4644384 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3322,6 +3322,13 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,

host->sdma_boundary = SDHCI_DEFAULT_BOUNDARY_ARG;

+ /*
+ * The DMA table descriptor count 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.
+ */
+ host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1;
+
return host;
}

@@ -3567,18 +3574,12 @@ int sdhci_setup_host(struct sdhci_host *host)
dma_addr_t dma;
void *buf;

- /*
- * The DMA descriptor table size 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,
- * all multipled by the descriptor size.
- */
if (host->flags & SDHCI_USE_64_BIT_DMA) {
- host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
+ host->adma_table_sz = host->adma_table_cnt *
SDHCI_ADMA2_64_DESC_SZ;
host->desc_sz = SDHCI_ADMA2_64_DESC_SZ;
} else {
- host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
+ host->adma_table_sz = host->adma_table_cnt *
SDHCI_ADMA2_32_DESC_SZ;
host->desc_sz = SDHCI_ADMA2_32_DESC_SZ;
}
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index f0bd36ce3817..25bddd21de31 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -563,6 +563,9 @@ struct sdhci_host {
/* Host SDMA buffer boundary. */
u32 sdma_boundary;

+ /* Host ADMA table count */
+ u32 adma_table_cnt;
+
u64 data_timeout;

unsigned long private[0] ____cacheline_aligned;
--
2.18.0


2018-08-23 16:13:12

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] mmc: sdhci: add adma_table_cnt member to struct sdhci_host

On 23/08/18 13:07, Jisheng Zhang wrote:
> This patch adds adma_table_cnt member to struct sdhci_host to give more
> flexibility to drivers to control the ADMA table count.
>
> Default value of adma_table_cnt is set to (SDHCI_MAX_SEGS * 2 + 1).
>
> Signed-off-by: Jisheng Zhang <[email protected]>

Acked-by: Adrian Hunter <[email protected]>

> ---
> drivers/mmc/host/sdhci.c | 17 +++++++++--------
> drivers/mmc/host/sdhci.h | 3 +++
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1b3fbd9bd5c5..52ccf4644384 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3322,6 +3322,13 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
>
> host->sdma_boundary = SDHCI_DEFAULT_BOUNDARY_ARG;
>
> + /*
> + * The DMA table descriptor count 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.
> + */
> + host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1;
> +
> return host;
> }
>
> @@ -3567,18 +3574,12 @@ int sdhci_setup_host(struct sdhci_host *host)
> dma_addr_t dma;
> void *buf;
>
> - /*
> - * The DMA descriptor table size 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,
> - * all multipled by the descriptor size.
> - */
> if (host->flags & SDHCI_USE_64_BIT_DMA) {
> - host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
> + host->adma_table_sz = host->adma_table_cnt *
> SDHCI_ADMA2_64_DESC_SZ;
> host->desc_sz = SDHCI_ADMA2_64_DESC_SZ;
> } else {
> - host->adma_table_sz = (SDHCI_MAX_SEGS * 2 + 1) *
> + host->adma_table_sz = host->adma_table_cnt *
> SDHCI_ADMA2_32_DESC_SZ;
> host->desc_sz = SDHCI_ADMA2_32_DESC_SZ;
> }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index f0bd36ce3817..25bddd21de31 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -563,6 +563,9 @@ struct sdhci_host {
> /* Host SDMA buffer boundary. */
> u32 sdma_boundary;
>
> + /* Host ADMA table count */
> + u32 adma_table_cnt;
> +
> u64 data_timeout;
>
> unsigned long private[0] ____cacheline_aligned;
>


2018-08-23 16:14:53

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH v4 2/3] mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops

On 23/08/18 13:08, Jisheng Zhang wrote:
> Add this hook so that it can be overridden with driver specific
> implementations. We also let the original sdhci_adma_write_desc()
> accept &desc so that the function can set its new value. Then export
> the function so that it could be reused by driver's specific
> implementations.
>
> Signed-off-by: Jisheng Zhang <[email protected]>

Acked-by: Adrian Hunter <[email protected]>

> ---
> drivers/mmc/host/sdhci.c | 37 +++++++++++++++++++++++--------------
> drivers/mmc/host/sdhci.h | 4 ++++
> 2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 52ccf4644384..eb21d2db7f05 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -554,10 +554,10 @@ 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)
> +void 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;
> + struct sdhci_adma2_64_desc *dma_desc = *desc;
>
> /* 32-bit and 64-bit descriptors have these members in same position */
> dma_desc->cmd = cpu_to_le16(cmd);
> @@ -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);
> +
> + *desc += host->desc_sz;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_adma_write_desc);
> +
> +static inline void __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)
> + host->ops->adma_write_desc(host, desc, addr, len, cmd);
> +
> + sdhci_adma_write_desc(host, desc, addr, len, cmd);
> }
>
> static void sdhci_adma_mark_end(void *desc)
> @@ -618,28 +631,24 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
> }
>
> /* tran, valid */
> - sdhci_adma_write_desc(host, desc, align_addr, offset,
> - ADMA2_TRAN_VALID);
> + __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;
> -
> addr += offset;
> len -= offset;
> }
>
> BUG_ON(len > 65536);
>
> - if (len) {
> - /* tran, valid */
> - sdhci_adma_write_desc(host, desc, addr, len,
> - ADMA2_TRAN_VALID);
> - desc += host->desc_sz;
> - }
> + /* tran, valid */
> + if (len)
> + __sdhci_adma_write_desc(host, &desc, addr, len,
> + ADMA2_TRAN_VALID);
>
> /*
> * If this triggers then we have a calculation bug
> @@ -656,7 +665,7 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,
> }
> } else {
> /* Add a terminating entry - nop, end, valid */
> - sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
> + __sdhci_adma_write_desc(host, &desc, 0, 0, ADMA2_NOP_END_VALID);
> }
> }
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 25bddd21de31..2115416f973a 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -606,6 +606,8 @@ 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);
> + void (*adma_write_desc)(struct sdhci_host *host, void **desc,
> + dma_addr_t addr, int len, unsigned int cmd);
> };
>
> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> @@ -736,6 +738,8 @@ 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);
> +void 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);
>


2018-08-23 16:17:30

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

On 23/08/18 13:09, 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.
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> drivers/mmc/host/sdhci-of-dwcmshc.c | 39 +++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 1b7cd144fb01..df0a3aeabe19 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -8,21 +8,48 @@
> */
>
> #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 void 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)))
> + sdhci_adma_write_desc(host, desc, addr, len, cmd);

Doesn't this need a return? i.e.

return;
}

> +
> + offset = addr & (SZ_128M - 1);
> + tmplen = SZ_128M - offset;
> + sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
> +
> + addr += tmplen;
> + len -= tmplen;
> + sdhci_adma_write_desc(host, desc, addr, len, cmd);
> +}
> +
> 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 +63,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 table descriptor count 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);

You are assuming here that totalram_pages gives you the highest physical
page number. Is that true?

> + if (extra > SDHCI_MAX_SEGS)
> + extra = SDHCI_MAX_SEGS;
> + host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1 + extra;

That should be:

host->adma_table_cnt += extra;

> +
> pltfm_host = sdhci_priv(host);
> priv = sdhci_pltfm_priv(pltfm_host);
>
>


2018-08-27 07:44:49

by Jisheng Zhang

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] mmc: sdhci-of-dwcmshc: solve 128MB DMA boundary limitation

On Thu, 23 Aug 2018 14:51:26 +0300 Adrian Hunter wrote:

> On 23/08/18 13:09, 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.
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > drivers/mmc/host/sdhci-of-dwcmshc.c | 39 +++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 1b7cd144fb01..df0a3aeabe19 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -8,21 +8,48 @@
> > */
> >
> > #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 void 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)))
> > + sdhci_adma_write_desc(host, desc, addr, len, cmd);
>
> Doesn't this need a return? i.e.
>
> return;

oops, I made a mistake here. Pre v4, there's a return...
Thanks so much.


> }
>
> > +
> > + offset = addr & (SZ_128M - 1);
> > + tmplen = SZ_128M - offset;
> > + sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
> > +
> > + addr += tmplen;
> > + len -= tmplen;
> > + sdhci_adma_write_desc(host, desc, addr, len, cmd);
> > +}
> > +
> > 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 +63,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 table descriptor count 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);
>
> You are assuming here that totalram_pages gives you the highest physical
> page number. Is that true?

Per my understanding, it's true. What I need is the MemTotal/PAGE_SIZE where
Memtotal is from /proc/meminfo. After reading the code, I found this is
totalram_pages as shown in si_meminfo()

>
> > + if (extra > SDHCI_MAX_SEGS)
> > + extra = SDHCI_MAX_SEGS;
> > + host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1 + extra;
>
> That should be:
>
> host->adma_table_cnt += extra;

yep, will do in v5

>
> > +
> > pltfm_host = sdhci_priv(host);
> > priv = sdhci_pltfm_priv(pltfm_host);
> >
> >
>