Hi Ulf, > -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx] > Sent: 2017年12月19日 20:40 > To: Xinming Hu <huxm@xxxxxxxxxxx> > Cc: Linux MMC <linux-mmc@xxxxxxxxxxxxxxx>; Kalle Valo > <kvalo@xxxxxxxxxxxxxxxx>; Arend van Spriel > <arend.vanspriel@xxxxxxxxxxxx>; Franky Lin <franky.lin@xxxxxxxxxxxx>; > Hante Meuleman <hante.meuleman@xxxxxxxxxxxx>; Chi-Hsien Lin > <chi-hsien.lin@xxxxxxxxxxx>; Wright Feng <wright.feng@xxxxxxxxxxx>; > Zhiyuan Yang <yangzy@xxxxxxxxxxx>; Tim Song <songtao@xxxxxxxxxxx>; > Cathy Luo <cluo@xxxxxxxxxxx>; James Cao <jcao@xxxxxxxxxxx>; Ganapathi > Bhat <gbhat@xxxxxxxxxxx>; Xu (Shanghai) Wang <xwang4@xxxxxxxxxxx>; > Bob Tan <tanbo@xxxxxxxxxxx>; Amitkumar Karwar <akarwar@xxxxxxxxxxx> > 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx> > > --- > > 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 ��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
