[PATCH v6 1/4] mmc: API for accessing host supported maximum segment count and size

[PATCH v6 1/4] mmc: API for accessing host supported maximum segment count and size

[Xinming Hu]

sdio device drivers need be able to get the host supported max_segs
and max_seg_size, so that they know the buffer size to allocate while
utilizing the scatter/gather DMA buffer list.

This patch provides API for this purpose.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: v2 was submitted with minor improvement like replacing BUG_ON() with WARN_ON()
v3: Addressed below review comments from Ulf Hansson
    a) In v3, patch has been split into two separate patches.
    b) Patch 1/2 introduces an API to fetch max_seg_size and max_segs
    c) Replaced WARN_ON() with proper error code when sg_ptr->length is invalid
    d) Instead of duplicating the code in mmc_io_rw_extended(), extra bool parameter
       has been added to this function and used it in new APIs for SG.
v4: Removed WARN_ON() calls in newly added APIs. It's gets called in probe handler.
    Caller already takes care of it(Shawn Lin).
v5: Rebased on latest code base.
v6: Split driver caller to separate patch.
---
 drivers/mmc/core/sdio_io.c    | 21 +++++++++++++++++++++
 include/linux/mmc/sdio_func.h |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index d40744b..4806521 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -725,3 +725,24 @@ int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sdio_set_host_pm_flags);
+
+/**
+ *	sdio_get_host_max_seg_size - get host maximum segment size
+ *	@func: SDIO function attached to host
+ */
+unsigned int sdio_get_host_max_seg_size(struct sdio_func *func)
+{
+	return func->card->host->max_seg_size;
+}
+EXPORT_SYMBOL_GPL(sdio_get_host_max_seg_size);
+
+/**
+ *	sdio_get_host_max_seg_count - get host maximum segment count
+ *	@func: SDIO function attached to host
+ */
+unsigned short sdio_get_host_max_seg_count(struct sdio_func *func)
+{
+	return func->card->host->max_segs;
+}
+EXPORT_SYMBOL_GPL(sdio_get_host_max_seg_count);
+
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 97ca105..72d4de4 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -159,4 +159,7 @@ extern void sdio_f0_writeb(struct sdio_func *func, unsigned char b,
 extern mmc_pm_flag_t sdio_get_host_pm_caps(struct sdio_func *func);
 extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags);
 
+unsigned short sdio_get_host_max_seg_count(struct sdio_func *func);
+unsigned int sdio_get_host_max_seg_size(struct sdio_func *func);
+
 #endif /* LINUX_MMC_SDIO_FUNC_H */
-- 
1.9.1

[PATCH v6 2/4] ath6kl: change to use mmc api for accessing host supported maximum segment count and size

[Xinming Hu]

Using mmc standard api to get the host sg capability.

Signed-off-by: Xinming Hu <huxm@marvell.com>
---
v6: separate driver patch from patch 1/4.
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 2195b1b..3217698 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -734,17 +734,18 @@ static int ath6kl_sdio_enable_scatter(struct ath6kl *ar)
 	struct htc_target *target = ar->htc_target;
 	int ret = 0;
 	bool virt_scat = false;
+	unsigned short max_segs = 0;
 
 	if (ar_sdio->scatter_enabled)
 		return 0;
 
 	ar_sdio->scatter_enabled = true;
 
+	max_segs = sdio_get_host_max_seg_count(ar_sdio->func);
 	/* check if host supports scatter and it meets our requirements */
-	if (ar_sdio->func->card->host->max_segs < MAX_SCATTER_ENTRIES_PER_REQ) {
+	if (max_segs < MAX_SCATTER_ENTRIES_PER_REQ) {
 		ath6kl_err("host only supports scatter of :%d entries, need: %d\n",
-			   ar_sdio->func->card->host->max_segs,
-			   MAX_SCATTER_ENTRIES_PER_REQ);
+			   max_segs, MAX_SCATTER_ENTRIES_PER_REQ);
 		virt_scat = true;
 	}
 
-- 
1.9.1

Re: [PATCH v6 2/4] ath6kl: change to use mmc api for accessing host supported maximum segment count and size

[Kalle Valo]

Xinming Hu <huxm@marvell.com> writes:

> Using mmc standard api to get the host sg capability.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> ---
> v6: separate driver patch from patch 1/4.
> ---
>  drivers/net/wireless/ath/ath6kl/sdio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

I assume this will go via some other tree and I should not take this.
So:

Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

-- 
Kalle Valo

[PATCH v6 3/4] brcmfmac: change to use mmc api for accessing host supported maximum segment count and size

[Xinming Hu]

Using mmc standard api to get the host sg capability.

Signed-off-by: Xinming Hu <huxm@marvell.com>
---
v6: separate driver patch from patch 1/4.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index cd58732..d0e08d0 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -886,19 +886,21 @@ void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
 {
 	struct sdio_func *func;
 	struct mmc_host *host;
+	unsigned short max_segs = 0;
 	uint max_blocks;
 	uint nents;
 	int err;
 
 	func = sdiodev->func[2];
 	host = func->card->host;
-	sdiodev->sg_support = host->max_segs > 1;
+
+	max_segs = sdio_get_host_max_seg_count(func);
+	sdiodev->sg_support = max_segs > 1;
 	max_blocks = min_t(uint, host->max_blk_count, 511u);
 	sdiodev->max_request_size = min_t(uint, host->max_req_size,
 					  max_blocks * func->cur_blksize);
-	sdiodev->max_segment_count = min_t(uint, host->max_segs,
-					   SG_MAX_SINGLE_ALLOC);
-	sdiodev->max_segment_size = host->max_seg_size;
+	sdiodev->max_segment_count = min_t(uint, SG_MAX_SINGLE_ALLOC, max_segs);
+	sdiodev->max_segment_size = sdio_get_host_max_seg_size(func);
 
 	if (!sdiodev->sg_support)
 		return;
-- 
1.9.1

[PATCH v6 4/4] mmc: sdio support external scatter gather list

[Xinming Hu]

Currently sdio device drivers need to prepare a big continuous
physical memory for large data transfer. mmc stack will construct
scatter/gather buffer list to make use of ADMA2.

According to the sdio host controller specification version 4.00
chapter 1.13.2, sdio device drivers have the ability to construct
scatter/gather DMA buffer list. In this way, they do not need to allocate
big memory.

This patch refactors current sdio command 53 wrapper mmc_io_rw_extended,
so that it can accept external scatter/gather buffer list from its caller,
such as the sdio device driver. This patch is very useful on some embedded
systems where large memory allocation fails sometimes. It gives 10 to 15%
better throughput performance compared to a case in which small single
data transfers are done.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v4: same as v3/v2/v1
v5: rebased on latest codebase
v6: same as v5
---
 drivers/mmc/core/sdio_io.c    | 22 ++++++++++++++++--
 drivers/mmc/core/sdio_ops.c   | 53 +++++++++++++++++++++++++++++++++++++++----
 drivers/mmc/core/sdio_ops.h   |  3 ++-
 include/linux/mmc/sdio_func.h |  6 +++++
 4 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 4806521..6046511 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
 			size = blocks * func->cur_blksize;
 
 			ret = mmc_io_rw_extended(func->card, write,
-				func->num, addr, incr_addr, buf,
+				func->num, addr, incr_addr, false, buf,
 				blocks, func->cur_blksize);
 			if (ret)
 				return ret;
@@ -345,7 +345,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
 
 		/* Indicate byte mode by setting "blocks" = 0 */
 		ret = mmc_io_rw_extended(func->card, write, func->num, addr,
-			 incr_addr, buf, 0, size);
+			 incr_addr, false, buf, 0, size);
 		if (ret)
 			return ret;
 
@@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned int addr, void *src,
 }
 EXPORT_SYMBOL_GPL(sdio_writesb);
 
+int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
+		       struct sg_table *dst)
+{
+	return mmc_io_rw_extended(func->card, 0, func->num,
+				  addr, 0, true,
+				  dst, 0, func->cur_blksize);
+}
+EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh);
+
+int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
+			struct sg_table *src)
+{
+	return mmc_io_rw_extended(func->card, 1, func->num,
+				  addr, 0, true,
+				  src, 0, func->cur_blksize);
+}
+EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh);
+
 /**
  *	sdio_readw - read a 16 bit integer from a SDIO function
  *	@func: SDIO function to access
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index abaaba3..2310a2a 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
 	return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
 }
 
+/**
+ *     mmc_io_rw_extended - wrapper for sdio command 53 read/write operation
+ *     @card: destination device for read/write
+ *     @write: zero indcate read, non-zero indicate write.
+ *     @fn: SDIO function to access
+ *     @addr: address of (single byte) FIFO
+ *     @incr_addr: set to 1 indicate read or write multiple bytes
+		of data to/from an IO register address that
+		increment by 1 after each operation.
+ *     @sg_enhance: if set true, the caller of this function will
+		prepare scatter gather dma buffer list.
+ *     @buf: buffer that contains the data to write. if sg_enhance
+		is set, it point to SG dma buffer list.
+ *     @blocks: number of blocks of data transfer.
+		if set zero, indicate byte mode
+		if set non-zero, indicate block mode
+		if sg_enhance is set, this parameter will not be used.
+ *     @blksz: block size for block mode data transfer.
+ *
+ */
 int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
-	unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz)
+		       unsigned int addr, int incr_addr, bool sg_enhance,
+		       void *buf, unsigned int blocks, unsigned int blksz)
 {
 	struct mmc_request mrq = {};
 	struct mmc_command cmd = {};
 	struct mmc_data data = {};
 	struct scatterlist sg, *sg_ptr;
 	struct sg_table sgtable;
+	struct sg_table *sgtable_external;
 	unsigned int nents, left_size, i;
 	unsigned int seg_size = card->host->max_seg_size;
+	unsigned int sg_blocks = 0;
 
 	WARN_ON(blksz == 0);
 
@@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	cmd.arg |= fn << 28;
 	cmd.arg |= incr_addr ? 0x04000000 : 0x00000000;
 	cmd.arg |= addr << 9;
+	cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
+
+	data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
+	data.blksz = blksz;
+
+	if (sg_enhance) {
+		sgtable_external = buf;
+
+		for_each_sg(sgtable_external->sgl, sg_ptr,
+			    sgtable_external->nents, i) {
+			if (sg_ptr->length > card->host->max_seg_size)
+				return -EINVAL;
+			sg_blocks += DIV_ROUND_UP(sg_ptr->length, blksz);
+		}
+
+		cmd.arg |= 0x08000000 | sg_blocks;
+		data.blocks = sg_blocks;
+		data.sg = sgtable_external->sgl;
+		data.sg_len = sgtable_external->nents;
+		goto mmc_data_req;
+	}
+
 	if (blocks == 0)
 		cmd.arg |= (blksz == 512) ? 0 : blksz;	/* byte mode */
 	else
 		cmd.arg |= 0x08000000 | blocks;		/* block mode */
-	cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
 
-	data.blksz = blksz;
 	/* Code in host drivers/fwk assumes that "blocks" always is >=1 */
 	data.blocks = blocks ? blocks : 1;
-	data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
 
 	left_size = data.blksz * data.blocks;
 	nents = DIV_ROUND_UP(left_size, seg_size);
@@ -172,11 +214,12 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 		sg_init_one(&sg, buf, left_size);
 	}
 
+mmc_data_req:
 	mmc_set_data_timeout(&data, card);
 
 	mmc_wait_for_req(card->host, &mrq);
 
-	if (nents > 1)
+	if (!sg_enhance && nents > 1)
 		sg_free_table(&sgtable);
 
 	if (cmd.error)
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index 96945ca..2d75100 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -23,7 +23,8 @@
 int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
 	unsigned addr, u8 in, u8* out);
 int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
-	unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
+		       unsigned int addr, int incr_addr, bool sg_enhance,
+		       void *buf, unsigned int blocks, unsigned int blksz);
 int sdio_reset(struct mmc_host *host);
 unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz);
 void sdio_irq_work(struct work_struct *work);
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 72d4de4..d36ba47 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -14,6 +14,7 @@
 
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/scatterlist.h>
 
 #include <linux/mmc/pm.h>
 
@@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func *func, void *dst,
 extern int sdio_readsb(struct sdio_func *func, void *dst,
 	unsigned int addr, int count);
 
+int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
+		       struct sg_table *dst);
+int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
+			struct sg_table *src);
+
 extern void sdio_writeb(struct sdio_func *func, u8 b,
 	unsigned int addr, int *err_ret);
 extern void sdio_writew(struct sdio_func *func, u16 b,
-- 
1.9.1

Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list

[Ulf Hansson]

On 19 December 2017 at 13:06, Xinming Hu <huxm@marvell.com> wrote:
> Currently sdio device drivers need to prepare a big continuous
> physical memory for large data transfer. mmc stack will construct
> scatter/gather buffer list to make use of ADMA2.
>
> According to the sdio host controller specification version 4.00
> chapter 1.13.2, sdio device drivers have the ability to construct
> scatter/gather DMA buffer list. In this way, they do not need to allocate
> big memory.
>
> This patch refactors current sdio command 53 wrapper mmc_io_rw_extended,
> so that it can accept external scatter/gather buffer list from its caller,
> such as the sdio device driver. This patch is very useful on some embedded
> systems where large memory allocation fails sometimes. It gives 10 to 15%
> better throughput performance compared to a case in which small single
> data transfers are done.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v4: same as v3/v2/v1
> v5: rebased on latest codebase
> v6: same as v5
> ---
>  drivers/mmc/core/sdio_io.c    | 22 ++++++++++++++++--
>  drivers/mmc/core/sdio_ops.c   | 53 +++++++++++++++++++++++++++++++++++++++----
>  drivers/mmc/core/sdio_ops.h   |  3 ++-
>  include/linux/mmc/sdio_func.h |  6 +++++
>  4 files changed, 76 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index 4806521..6046511 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>                         size = blocks * func->cur_blksize;
>
>                         ret = mmc_io_rw_extended(func->card, write,
> -                               func->num, addr, incr_addr, buf,
> +                               func->num, addr, incr_addr, false, buf,
>                                 blocks, func->cur_blksize);
>                         if (ret)
>                                 return ret;
> @@ -345,7 +345,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>
>                 /* Indicate byte mode by setting "blocks" = 0 */
>                 ret = mmc_io_rw_extended(func->card, write, func->num, addr,
> -                        incr_addr, buf, 0, size);
> +                        incr_addr, false, buf, 0, size);
>                 if (ret)
>                         return ret;
>
> @@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned int addr, void *src,
>  }
>  EXPORT_SYMBOL_GPL(sdio_writesb);
>
> +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,

Could you please rename this to sdio_readsb_sg() instead?

> +                      struct sg_table *dst)

I think what Christoph suggested and I agree with, is to use a "struct
scatterlist *sg" instead of "struct sg_table *dst" as the
in-parameter.

The similar applies for the write API, of course.

> +{
> +       return mmc_io_rw_extended(func->card, 0, func->num,
> +                                 addr, 0, true,
> +                                 dst, 0, func->cur_blksize);
> +}
> +EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh);
> +
> +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,

Could you please rename this to sdio_writesb_sg() instead?

> +                       struct sg_table *src)
> +{
> +       return mmc_io_rw_extended(func->card, 1, func->num,
> +                                 addr, 0, true,
> +                                 src, 0, func->cur_blksize);
> +}
> +EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh);
> +
>  /**
>   *     sdio_readw - read a 16 bit integer from a SDIO function
>   *     @func: SDIO function to access
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index abaaba3..2310a2a 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
>         return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
>  }
>
> +/**
> + *     mmc_io_rw_extended - wrapper for sdio command 53 read/write operation
> + *     @card: destination device for read/write
> + *     @write: zero indcate read, non-zero indicate write.
> + *     @fn: SDIO function to access
> + *     @addr: address of (single byte) FIFO
> + *     @incr_addr: set to 1 indicate read or write multiple bytes
> +               of data to/from an IO register address that
> +               increment by 1 after each operation.
> + *     @sg_enhance: if set true, the caller of this function will
> +               prepare scatter gather dma buffer list.
> + *     @buf: buffer that contains the data to write. if sg_enhance
> +               is set, it point to SG dma buffer list.
> + *     @blocks: number of blocks of data transfer.
> +               if set zero, indicate byte mode
> +               if set non-zero, indicate block mode
> +               if sg_enhance is set, this parameter will not be used.
> + *     @blksz: block size for block mode data transfer.
> + *
> + */

The descriptive function header isn't required here, because this is
an internal function for the mmc core.

However, if you really want this, I suggest you make it a separate patch.

>  int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> -       unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz)
> +                      unsigned int addr, int incr_addr, bool sg_enhance,

Instead of letting this take a new bool variable, let's instead
provide it with a  "struct scatterlist *sg". In cases when it isn't
used, just provide NULL.

Depending on the end result, we may even consider to make a separate
function dealing with the scatterlist case, perhaps via sharing some
common functionality from mmc_io_rw_extended() instead.

> +                      void *buf, unsigned int blocks, unsigned int blksz)
>  {
>         struct mmc_request mrq = {};
>         struct mmc_command cmd = {};
>         struct mmc_data data = {};
>         struct scatterlist sg, *sg_ptr;
>         struct sg_table sgtable;
> +       struct sg_table *sgtable_external;
>         unsigned int nents, left_size, i;
>         unsigned int seg_size = card->host->max_seg_size;
> +       unsigned int sg_blocks = 0;
>
>         WARN_ON(blksz == 0);
>
> @@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>         cmd.arg |= fn << 28;
>         cmd.arg |= incr_addr ? 0x04000000 : 0x00000000;
>         cmd.arg |= addr << 9;
> +       cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
> +
> +       data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> +       data.blksz = blksz;
> +
> +       if (sg_enhance) {
> +               sgtable_external = buf;
> +
> +               for_each_sg(sgtable_external->sgl, sg_ptr,
> +                           sgtable_external->nents, i) {
> +                       if (sg_ptr->length > card->host->max_seg_size)
> +                               return -EINVAL;
> +                       sg_blocks += DIV_ROUND_UP(sg_ptr->length, blksz);

This looks wrong. For each iteration the number of sg_blocks will be
increased, with the round up method.

This may lead to that the data.blocks could get a in-correct value
(bigger) when assigned below, right?

> +               }
> +
> +               cmd.arg |= 0x08000000 | sg_blocks;

This isn't going to work for byte mode transfers. Seems like that
should be possible too when using a pre-allocated scatterlist, right!?

> +               data.blocks = sg_blocks;
> +               data.sg = sgtable_external->sgl;
> +               data.sg_len = sgtable_external->nents;
> +               goto mmc_data_req;
> +       }
> +
>         if (blocks == 0)
>                 cmd.arg |= (blksz == 512) ? 0 : blksz;  /* byte mode */
>         else
>                 cmd.arg |= 0x08000000 | blocks;         /* block mode */
> -       cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
>
> -       data.blksz = blksz;
>         /* Code in host drivers/fwk assumes that "blocks" always is >=1 */
>         data.blocks = blocks ? blocks : 1;
> -       data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
>
>         left_size = data.blksz * data.blocks;
>         nents = DIV_ROUND_UP(left_size, seg_size);
> @@ -172,11 +214,12 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>                 sg_init_one(&sg, buf, left_size);
>         }
>
> +mmc_data_req:
>         mmc_set_data_timeout(&data, card);
>
>         mmc_wait_for_req(card->host, &mrq);
>
> -       if (nents > 1)
> +       if (!sg_enhance && nents > 1)
>                 sg_free_table(&sgtable);
>
>         if (cmd.error)
> diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
> index 96945ca..2d75100 100644
> --- a/drivers/mmc/core/sdio_ops.h
> +++ b/drivers/mmc/core/sdio_ops.h
> @@ -23,7 +23,8 @@
>  int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
>         unsigned addr, u8 in, u8* out);
>  int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> -       unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
> +                      unsigned int addr, int incr_addr, bool sg_enhance,
> +                      void *buf, unsigned int blocks, unsigned int blksz);
>  int sdio_reset(struct mmc_host *host);
>  unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz);
>  void sdio_irq_work(struct work_struct *work);
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index 72d4de4..d36ba47 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -14,6 +14,7 @@
>
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/scatterlist.h>
>
>  #include <linux/mmc/pm.h>
>
> @@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func *func, void *dst,
>  extern int sdio_readsb(struct sdio_func *func, void *dst,
>         unsigned int addr, int count);
>
> +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
> +                      struct sg_table *dst);
> +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
> +                       struct sg_table *src);
> +
>  extern void sdio_writeb(struct sdio_func *func, u8 b,
>         unsigned int addr, int *err_ret);
>  extern void sdio_writew(struct sdio_func *func, u16 b,
> --
> 1.9.1
>

Kind regards
Uffe

RE: [EXT] Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list

[Xinming Hu]

Hi Ulf,

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: 2017年12月19日 20:40
> To: Xinming Hu <huxm@marvell.com>
> Cc: Linux MMC <linux-mmc@vger.kernel.org>; Kalle Valo
> <kvalo@qca.qualcomm.com>; Arend van Spriel
> <arend.vanspriel@broadcom.com>; Franky Lin <franky.lin@broadcom.com>;
> Hante Meuleman <hante.meuleman@broadcom.com>; Chi-Hsien Lin
> <chi-hsien.lin@cypress.com>; Wright Feng <wright.feng@cypress.com>;
> Zhiyuan Yang <yangzy@marvell.com>; Tim Song <songtao@marvell.com>;
> Cathy Luo <cluo@marvell.com>; James Cao <jcao@marvell.com>; Ganapathi
> Bhat <gbhat@marvell.com>; Xu (Shanghai) Wang <xwang4@marvell.com>;
> Bob Tan <tanbo@marvell.com>; Amitkumar Karwar <akarwar@marvell.com>
> Subject: [EXT] Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 19 December 2017 at 13:06, Xinming Hu <huxm@marvell.com> wrote:
> > Currently sdio device drivers need to prepare a big continuous
> > physical memory for large data transfer. mmc stack will construct
> > scatter/gather buffer list to make use of ADMA2.
> >
> > According to the sdio host controller specification version 4.00
> > chapter 1.13.2, sdio device drivers have the ability to construct
> > scatter/gather DMA buffer list. In this way, they do not need to
> > allocate big memory.
> >
> > This patch refactors current sdio command 53 wrapper
> > mmc_io_rw_extended, so that it can accept external scatter/gather
> > buffer list from its caller, such as the sdio device driver. This
> > patch is very useful on some embedded systems where large memory
> > allocation fails sometimes. It gives 10 to 15% better throughput
> > performance compared to a case in which small single data transfers are
> done.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> > v4: same as v3/v2/v1
> > v5: rebased on latest codebase
> > v6: same as v5
> > ---
> >  drivers/mmc/core/sdio_io.c    | 22 ++++++++++++++++--
> >  drivers/mmc/core/sdio_ops.c   | 53
> +++++++++++++++++++++++++++++++++++++++----
> >  drivers/mmc/core/sdio_ops.h   |  3 ++-
> >  include/linux/mmc/sdio_func.h |  6 +++++
> >  4 files changed, 76 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> > index 4806521..6046511 100644
> > --- a/drivers/mmc/core/sdio_io.c
> > +++ b/drivers/mmc/core/sdio_io.c
> > @@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func
> *func, int write,
> >                         size = blocks * func->cur_blksize;
> >
> >                         ret = mmc_io_rw_extended(func->card, write,
> > -                               func->num, addr, incr_addr, buf,
> > +                               func->num, addr, incr_addr, false,
> > + buf,
> >                                 blocks, func->cur_blksize);
> >                         if (ret)
> >                                 return ret; @@ -345,7 +345,7 @@
> static
> > int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
> >
> >                 /* Indicate byte mode by setting "blocks" = 0 */
> >                 ret = mmc_io_rw_extended(func->card, write,
> func->num, addr,
> > -                        incr_addr, buf, 0, size);
> > +                        incr_addr, false, buf, 0, size);
> >                 if (ret)
> >                         return ret;
> >
> > @@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned
> > int addr, void *src,  }  EXPORT_SYMBOL_GPL(sdio_writesb);
> >
> > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
> 
> Could you please rename this to sdio_readsb_sg() instead?

Ok, sure.
> 
> > +                      struct sg_table *dst)
> 
> I think what Christoph suggested and I agree with, is to use a "struct
> scatterlist *sg" instead of "struct sg_table *dst" as the in-parameter.
> 
> The similar applies for the write API, of course.

Oh, I am not completely understand this point, could you share more information?
If using scatter_list, we need travel each node using sg_next()
Sg_table with nents could be able to use for_each_sg in the API, looks better here.

I guess, maybe scatter_list is a more generic data structure to be encouraged to use, right ?
If so, I will be fine.

> 
> > +{
> > +       return mmc_io_rw_extended(func->card, 0, func->num,
> > +                                 addr, 0, true,
> > +                                 dst, 0, func->cur_blksize); }
> > +EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh);
> > +
> > +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
> 
> Could you please rename this to sdio_writesb_sg() instead?

Ok.

> 
> > +                       struct sg_table *src) {
> > +       return mmc_io_rw_extended(func->card, 1, func->num,
> > +                                 addr, 0, true,
> > +                                 src, 0, func->cur_blksize); }
> > +EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh);
> > +
> >  /**
> >   *     sdio_readw - read a 16 bit integer from a SDIO function
> >   *     @func: SDIO function to access
> > diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> > index abaaba3..2310a2a 100644
> > --- a/drivers/mmc/core/sdio_ops.c
> > +++ b/drivers/mmc/core/sdio_ops.c
> > @@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int
> write, unsigned fn,
> >         return mmc_io_rw_direct_host(card->host, write, fn, addr, in,
> > out);  }
> >
> > +/**
> > + *     mmc_io_rw_extended - wrapper for sdio command 53 read/write
> operation
> > + *     @card: destination device for read/write
> > + *     @write: zero indcate read, non-zero indicate write.
> > + *     @fn: SDIO function to access
> > + *     @addr: address of (single byte) FIFO
> > + *     @incr_addr: set to 1 indicate read or write multiple bytes
> > +               of data to/from an IO register address that
> > +               increment by 1 after each operation.
> > + *     @sg_enhance: if set true, the caller of this function will
> > +               prepare scatter gather dma buffer list.
> > + *     @buf: buffer that contains the data to write. if sg_enhance
> > +               is set, it point to SG dma buffer list.
> > + *     @blocks: number of blocks of data transfer.
> > +               if set zero, indicate byte mode
> > +               if set non-zero, indicate block mode
> > +               if sg_enhance is set, this parameter will not be used.
> > + *     @blksz: block size for block mode data transfer.
> > + *
> > + */
> 
> The descriptive function header isn't required here, because this is an internal
> function for the mmc core.
> 
> However, if you really want this, I suggest you make it a separate patch.
> 
Ok.

> >  int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> > -       unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned
> blksz)
> > +                      unsigned int addr, int incr_addr, bool
> > + sg_enhance,
> 
> Instead of letting this take a new bool variable, let's instead provide it with a
> "struct scatterlist *sg". In cases when it isn't used, just provide NULL.
> 
Okay, still need choose between sg_table and scatter_list


> Depending on the end result, we may even consider to make a separate
> function dealing with the scatterlist case, perhaps via sharing some common
> functionality from mmc_io_rw_extended() instead.
> 

Oh, I have thought of the new API, and finally resue the old one, since less changes compared with the common part.
I will be fine if you really want a new API.

> > +                      void *buf, unsigned int blocks, unsigned int
> > + blksz)
> >  {
> >         struct mmc_request mrq = {};
> >         struct mmc_command cmd = {};
> >         struct mmc_data data = {};
> >         struct scatterlist sg, *sg_ptr;
> >         struct sg_table sgtable;
> > +       struct sg_table *sgtable_external;
> >         unsigned int nents, left_size, i;
> >         unsigned int seg_size = card->host->max_seg_size;
> > +       unsigned int sg_blocks = 0;
> >
> >         WARN_ON(blksz == 0);
> >
> > @@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card,
> int write, unsigned fn,
> >         cmd.arg |= fn << 28;
> >         cmd.arg |= incr_addr ? 0x04000000 : 0x00000000;
> >         cmd.arg |= addr << 9;
> > +       cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 |
> MMC_CMD_ADTC;
> > +
> > +       data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> > +       data.blksz = blksz;
> > +
> > +       if (sg_enhance) {
> > +               sgtable_external = buf;
> > +
> > +               for_each_sg(sgtable_external->sgl, sg_ptr,
> > +                           sgtable_external->nents, i) {
> > +                       if (sg_ptr->length > card->host->max_seg_size)
> > +                               return -EINVAL;
> > +                       sg_blocks += DIV_ROUND_UP(sg_ptr->length,
> > + blksz);
> 
> This looks wrong. For each iteration the number of sg_blocks will be increased,
> with the round up method.
> 
> This may lead to that the data.blocks could get a in-correct value
> (bigger) when assigned below, right?
> 

Per my understand, caller driver should keep each sg_ptr->length multiple of block size.
So divide should be fine for most place.
However if used wrong, sg_ptr->length is not multiple of block size, remainder memory part will be
Lost if using divide.

Please correct me if I have any misunderstanding here.


> > +               }
> > +
> > +               cmd.arg |= 0x08000000 | sg_blocks;
> 
> This isn't going to work for byte mode transfers. Seems like that should be
> possible too when using a pre-allocated scatterlist, right!?
> 

Aha, good suggestions.
Yes, I think it is possible, as there is actually a scatterlist generated for byte mode transfer in current API.
But it seems not the proper use case.
Do we really expect driver caller use sg in byte mode, it looks strange to me.

Tough in this way, the API looks more generic(with useless byte mode sg).


> > +               data.blocks = sg_blocks;
> > +               data.sg = sgtable_external->sgl;
> > +               data.sg_len = sgtable_external->nents;
> > +               goto mmc_data_req;
> > +       }
> > +
> >         if (blocks == 0)
> >                 cmd.arg |= (blksz == 512) ? 0 : blksz;  /* byte mode */
> >         else
> >                 cmd.arg |= 0x08000000 | blocks;         /* block
> mode */
> > -       cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 |
> MMC_CMD_ADTC;
> >
> > -       data.blksz = blksz;
> >         /* Code in host drivers/fwk assumes that "blocks" always is >=1 */
> >         data.blocks = blocks ? blocks : 1;
> > -       data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> >
> >         left_size = data.blksz * data.blocks;
> >         nents = DIV_ROUND_UP(left_size, seg_size); @@ -172,11
> +214,12
> > @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned
> fn,
> >                 sg_init_one(&sg, buf, left_size);
> >         }
> >
> > +mmc_data_req:
> >         mmc_set_data_timeout(&data, card);
> >
> >         mmc_wait_for_req(card->host, &mrq);
> >
> > -       if (nents > 1)
> > +       if (!sg_enhance && nents > 1)
> >                 sg_free_table(&sgtable);
> >
> >         if (cmd.error)
> > diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
> > index 96945ca..2d75100 100644
> > --- a/drivers/mmc/core/sdio_ops.h
> > +++ b/drivers/mmc/core/sdio_ops.h
> > @@ -23,7 +23,8 @@
> >  int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
> >         unsigned addr, u8 in, u8* out);  int mmc_io_rw_extended(struct
> > mmc_card *card, int write, unsigned fn,
> > -       unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned
> blksz);
> > +                      unsigned int addr, int incr_addr, bool
> sg_enhance,
> > +                      void *buf, unsigned int blocks, unsigned int
> > + blksz);
> >  int sdio_reset(struct mmc_host *host);  unsigned int
> > mmc_align_data_size(struct mmc_card *card, unsigned int sz);  void
> > sdio_irq_work(struct work_struct *work); diff --git
> > a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h index
> > 72d4de4..d36ba47 100644
> > --- a/include/linux/mmc/sdio_func.h
> > +++ b/include/linux/mmc/sdio_func.h
> > @@ -14,6 +14,7 @@
> >
> >  #include <linux/device.h>
> >  #include <linux/mod_devicetable.h>
> > +#include <linux/scatterlist.h>
> >
> >  #include <linux/mmc/pm.h>
> >
> > @@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func
> > *func, void *dst,  extern int sdio_readsb(struct sdio_func *func, void *dst,
> >         unsigned int addr, int count);
> >
> > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
> > +                      struct sg_table *dst); int
> > +sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
> > +                       struct sg_table *src);
> > +
> >  extern void sdio_writeb(struct sdio_func *func, u8 b,
> >         unsigned int addr, int *err_ret);  extern void
> > sdio_writew(struct sdio_func *func, u16 b,
> > --
> > 1.9.1
> >
> 
> Kind regards
> Uffe