[PATCH mmc-next v3 0/3] solve SDHCI DWC MSHC 128MB DMA boundary limitation

[PATCH mmc-next v3 0/3] solve SDHCI DWC MSHC 128MB DMA boundary limitation

[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.

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(-)

-- 
2.18.0

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

[Jisheng Zhang]

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 <Jisheng.Zhang@synaptics.com>
---
 drivers/mmc/host/sdhci.c | 31 +++++++++++++++++++++++--------
 drivers/mmc/host/sdhci.h |  6 ++++++
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 14dd4a49e03b..50c846d99182 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -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;
 		}
 
 		/*
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d55fd7033e93..0aad0ee8f63b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -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);
-- 
2.18.0

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

[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 <Jisheng.Zhang@synaptics.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 1b7cd144fb01..e890fc8f5284 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -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

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

[Jisheng Zhang]

On Mon, 30 Jul 2018 02:56:20 +0000 Matthew Leon <matthewleon@linux.com> wrote:

> Hey Jisheng,

Hi,

> 

In LKML, we'd better not top post.

> 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

> 
> Sincerely,
> Matthew Leon
> 
> On Sun, Jul 29, 2018 at 10:46 PM, Jisheng Zhang <Jisheng.Zhang@synaptics.com
> > 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 <Jisheng.Zhang@synaptics.com>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 1b7cd144fb01..e890fc8f5284 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -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
> >
> >  

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

[Jisheng Zhang]

On Mon, 30 Jul 2018 03:11:59 +0000
Matthew Leon <matthewleon@linux.com> wrote:

>  >> Hey Jisheng,  
> 
> >Hi,  
> 
> >>  
> 
> >In LKML, we'd better not top post.  
> 
> Noted. My apologies.
> 
> >> 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  
> 
> 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?

Re: [PATCH mmc-next v3 0/3] solve SDHCI DWC MSHC 128MB DMA boundary limitation

[Jisheng Zhang]

Hi Adrian, Ulf,

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

Thanks in advance,
Jisheng

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(-)
> 

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

[Adrian Hunter]

On 30/07/18 05:45, Jisheng Zhang wrote:
> 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 <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/mmc/host/sdhci.c | 31 +++++++++++++++++++++++--------
>  drivers/mmc/host/sdhci.h |  6 ++++++
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 14dd4a49e03b..50c846d99182 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -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);

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

> +
> +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,

> +					  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,

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

> +							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;
>  		}
>  
>  		/*
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d55fd7033e93..0aad0ee8f63b 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -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);
>