| Message ID | 1391349172-27668-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sunday, February 02, 2014 at 02:52:52 PM, Mark Brown wrote: Hi, thanks for preparing this patch! I have just a few very minor nitpicks, ignore if you please. [...] > +static int spi_map_buf(struct spi_master *master, struct device *dev, > + struct sg_table *sgt, void *buf, size_t len, > + enum dma_data_direction dir) > +{ > + const bool vmalloced_buf = is_vmalloc_addr(buf); > + const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len; You might want to rename this to "sg_chunk_max_size" or something, "desc_len" doesn't make much sense here. The variable describes the maximum size of one single scatterlist element. > + const int sgs = DIV_ROUND_UP(len, desc_len); Looking at this, the variables could generally use a more meaningful name. I think it'd be clearer to call this "num_sg_chunks" or so ? > + struct page *vm_page; > + void *sg_buf; > + size_t min; > + int i, ret; > + > + ret = sg_alloc_table(sgt, sgs, GFP_KERNEL); > + if (ret != 0) > + return ret; > + > + for (i = 0; i < sgs; i++) { > + min = min_t(size_t, len, desc_len); > + > + if (vmalloced_buf) { > + vm_page = vmalloc_to_page(buf); Just curious, but shouldn't we check if buf != NULL right at the begining of this function? [...] > +static void spi_unmap_buf(struct spi_master *master, struct device *dev, > + struct sg_table *sgt, enum dma_data_direction dir) > +{ > + if (sgt->orig_nents) { I don't want to nag, but why not use if (!sgt->...) return; ? This would cut down one level of indent. > + dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir); > + sg_free_table(sgt); > + } > +} > + [...] -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 05, 2014 at 07:30:57AM +0100, Marek Vasut wrote: > On Sunday, February 02, 2014 at 02:52:52 PM, Mark Brown wrote: > > +static int spi_map_buf(struct spi_master *master, struct device *dev, > > + struct sg_table *sgt, void *buf, size_t len, > > + enum dma_data_direction dir) > > +{ > > + const bool vmalloced_buf = is_vmalloc_addr(buf); > > + const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len; > You might want to rename this to "sg_chunk_max_size" or something, "desc_len" > doesn't make much sense here. The variable describes the maximum size of one > single scatterlist element. A scatterlist entry is pretty much an abstract descriptor though. I seem to remember looking at the name and thinking it was good that it was something less easily applicable to the length of the table but it doesn't make much odds. > > + const int sgs = DIV_ROUND_UP(len, desc_len); > Looking at this, the variables could generally use a more meaningful name. I > think it'd be clearer to call this "num_sg_chunks" or so ? You do know where I lifted most of these variable names from, right? :P Looking at the code again everything seems idiomatic with the naming of the fields inside the sg_table - I probably would apply a patch to rename but I wouldn't write one. > > + min = min_t(size_t, len, desc_len); > > + > > + if (vmalloced_buf) { > > + vm_page = vmalloc_to_page(buf); > Just curious, but shouldn't we check if buf != NULL right at the begining of > this function? No need, the check is outside the function along with the check that the controller is OK with DMAing on this transfer and so on. > > +static void spi_unmap_buf(struct spi_master *master, struct device *dev, > > + struct sg_table *sgt, enum dma_data_direction dir) > > +{ > > + if (sgt->orig_nents) { > I don't want to nag, but why not use if (!sgt->...) return; ? This would cut > down one level of indent. I was looking at some stuff which might add a bit more in here if it's not just the core doing mappings. Not sure that's sensible though so it might never materialise.
On Wednesday, February 05, 2014 at 01:00:02 PM, Mark Brown wrote: > On Wed, Feb 05, 2014 at 07:30:57AM +0100, Marek Vasut wrote: > > On Sunday, February 02, 2014 at 02:52:52 PM, Mark Brown wrote: > > > +static int spi_map_buf(struct spi_master *master, struct device *dev, > > > + struct sg_table *sgt, void *buf, size_t len, > > > + enum dma_data_direction dir) > > > +{ > > > + const bool vmalloced_buf = is_vmalloc_addr(buf); > > > + const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len; > > > > You might want to rename this to "sg_chunk_max_size" or something, > > "desc_len" doesn't make much sense here. The variable describes the > > maximum size of one single scatterlist element. > > A scatterlist entry is pretty much an abstract descriptor though. I > seem to remember looking at the name and thinking it was good that it > was something less easily applicable to the length of the table but it > doesn't make much odds. It's the length of the entry, but I see your point. > > > + const int sgs = DIV_ROUND_UP(len, desc_len); > > > > Looking at this, the variables could generally use a more meaningful > > name. I think it'd be clearer to call this "num_sg_chunks" or so ? > > You do know where I lifted most of these variable names from, right? :P Yeah, I don't like looking at my old code, it's always so frustrating ;-) Still, it's a good source for learning how to NOT do things again :) > Looking at the code again everything seems idiomatic with the naming of > the fields inside the sg_table - I probably would apply a patch to > rename but I wouldn't write one. OK, makes sense. > > > + min = min_t(size_t, len, desc_len); > > > + > > > + if (vmalloced_buf) { > > > + vm_page = vmalloc_to_page(buf); > > > > Just curious, but shouldn't we check if buf != NULL right at the begining > > of this function? > > No need, the check is outside the function along with the check that the > controller is OK with DMAing on this transfer and so on. OK, thank you for checking this. > > > +static void spi_unmap_buf(struct spi_master *master, struct device > > > *dev, + struct sg_table *sgt, enum dma_data_direction dir) > > > +{ > > > + if (sgt->orig_nents) { > > > > I don't want to nag, but why not use if (!sgt->...) return; ? This would > > cut down one level of indent. > > I was looking at some stuff which might add a bit more in here if it's > not just the core doing mappings. Not sure that's sensible though so it > might never materialise. OK. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index b9ba7a3e7741..4490e8c499c0 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -470,7 +470,7 @@ static void s3c64xx_spi_unmap_mssg(struct s3c64xx_spi_driver_data *sdd, } static void prepare_dma(struct s3c64xx_spi_dma_data *dma, - unsigned len, dma_addr_t buf) + struct sg_table *sgt) { struct s3c64xx_spi_driver_data *sdd; struct dma_slave_config config; @@ -496,8 +496,8 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, dmaengine_slave_config(dma->ch, &config); } - desc = dmaengine_prep_slave_single(dma->ch, buf, len, - dma->direction, DMA_PREP_INTERRUPT); + desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, + dma->direction, DMA_PREP_INTERRUPT); desc->callback = s3c64xx_spi_dmacb; desc->callback_param = dma; @@ -616,7 +616,11 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd, chcfg |= S3C64XX_SPI_CH_TXCH_ON; if (dma_mode) { modecfg |= S3C64XX_SPI_MODE_TXDMA_ON; +#ifndef CONFIG_S3C_DMA + prepare_dma(&sdd->tx_dma, &xfer->tx_sg); +#else prepare_dma(&sdd->tx_dma, xfer->len, xfer->tx_dma); +#endif } else { switch (sdd->cur_bpw) { case 32: @@ -648,7 +652,11 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd, writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff) | S3C64XX_SPI_PACKET_CNT_EN, regs + S3C64XX_SPI_PACKET_CNT); +#ifndef CONFIG_S3C_DMA + prepare_dma(&sdd->rx_dma, &xfer->rx_sg); +#else prepare_dma(&sdd->rx_dma, xfer->len, xfer->rx_dma); +#endif } } diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 1826a50c2aaf..fb93d2d69182 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -582,13 +582,70 @@ static void spi_set_cs(struct spi_device *spi, bool enable) spi->master->set_cs(spi, !enable); } +static int spi_map_buf(struct spi_master *master, struct device *dev, + struct sg_table *sgt, void *buf, size_t len, + enum dma_data_direction dir) +{ + const bool vmalloced_buf = is_vmalloc_addr(buf); + const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len; + const int sgs = DIV_ROUND_UP(len, desc_len); + struct page *vm_page; + void *sg_buf; + size_t min; + int i, ret; + + ret = sg_alloc_table(sgt, sgs, GFP_KERNEL); + if (ret != 0) + return ret; + + for (i = 0; i < sgs; i++) { + min = min_t(size_t, len, desc_len); + + if (vmalloced_buf) { + vm_page = vmalloc_to_page(buf); + if (!vm_page) { + sg_free_table(sgt); + return -ENOMEM; + } + sg_buf = page_address(vm_page) + + ((size_t)buf & ~PAGE_MASK); + } else { + sg_buf = buf; + } + + sg_set_buf(&sgt->sgl[i], sg_buf, min); + + buf += min; + len -= min; + } + + ret = dma_map_sg(dev, sgt->sgl, sgt->nents, dir); + if (ret < 0) { + sg_free_table(sgt); + return ret; + } + + sgt->nents = ret; + + return 0; +} + +static void spi_unmap_buf(struct spi_master *master, struct device *dev, + struct sg_table *sgt, enum dma_data_direction dir) +{ + if (sgt->orig_nents) { + dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir); + sg_free_table(sgt); + } +} + static int spi_map_msg(struct spi_master *master, struct spi_message *msg) { - struct device *dev = master->dev.parent; struct device *tx_dev, *rx_dev; struct spi_transfer *xfer; void *tmp; size_t max_tx, max_rx; + int ret; if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) { max_tx = 0; @@ -631,7 +688,7 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg) } } - if (msg->is_dma_mapped || !master->can_dma) + if (!master->can_dma) return 0; tx_dev = &master->dma_tx->dev->device; @@ -642,25 +699,21 @@ static int spi_map_msg(struct spi_master *master, struct spi_message *msg) continue; if (xfer->tx_buf != NULL) { - xfer->tx_dma = dma_map_single(tx_dev, - (void *)xfer->tx_buf, - xfer->len, - DMA_TO_DEVICE); - if (dma_mapping_error(dev, xfer->tx_dma)) { - dev_err(dev, "dma_map_single Tx failed\n"); - return -ENOMEM; - } + ret = spi_map_buf(master, tx_dev, &xfer->tx_sg, + (void *)xfer->tx_buf, xfer->len, + DMA_TO_DEVICE); + if (ret != 0) + return ret; } if (xfer->rx_buf != NULL) { - xfer->rx_dma = dma_map_single(rx_dev, - xfer->rx_buf, xfer->len, - DMA_FROM_DEVICE); - if (dma_mapping_error(dev, xfer->rx_dma)) { - dev_err(dev, "dma_map_single Rx failed\n"); - dma_unmap_single(tx_dev, xfer->tx_dma, - xfer->len, DMA_TO_DEVICE); - return -ENOMEM; + ret = spi_map_buf(master, rx_dev, &xfer->rx_sg, + xfer->rx_buf, xfer->len, + DMA_FROM_DEVICE); + if (ret != 0) { + spi_unmap_buf(master, tx_dev, &xfer->tx_sg, + DMA_TO_DEVICE); + return ret; } } } @@ -675,7 +728,7 @@ static int spi_unmap_msg(struct spi_master *master, struct spi_message *msg) struct spi_transfer *xfer; struct device *tx_dev, *rx_dev; - if (!master->cur_msg_mapped || msg->is_dma_mapped || !master->can_dma) + if (!master->cur_msg_mapped || !master->can_dma) return 0; tx_dev = &master->dma_tx->dev->device; @@ -685,12 +738,8 @@ static int spi_unmap_msg(struct spi_master *master, struct spi_message *msg) if (!master->can_dma(master, msg->spi, xfer)) continue; - if (xfer->rx_buf) - dma_unmap_single(rx_dev, xfer->rx_dma, xfer->len, - DMA_FROM_DEVICE); - if (xfer->tx_buf) - dma_unmap_single(tx_dev, xfer->tx_dma, xfer->len, - DMA_TO_DEVICE); + spi_unmap_buf(master, rx_dev, &xfer->rx_sg, DMA_FROM_DEVICE); + spi_unmap_buf(master, tx_dev, &xfer->tx_sg, DMA_TO_DEVICE); } return 0; @@ -1509,6 +1558,8 @@ int spi_register_master(struct spi_master *master) mutex_init(&master->bus_lock_mutex); master->bus_lock_flag = 0; init_completion(&master->xfer_completion); + if (!master->max_dma_len) + master->max_dma_len = INT_MAX; /* register the device, then userspace will see it. * registration fails if the bus ID is in use. diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index c4c68093cd7c..36c86ef51ff3 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/kthread.h> #include <linux/completion.h> +#include <linux/scatterlist.h> struct dma_chan; @@ -268,6 +269,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * @auto_runtime_pm: the core should ensure a runtime PM reference is held * while the hardware is prepared, using the parent * device for the spidev + * @max_dma_len: Maximum length of a DMA transfer for the device. * @prepare_transfer_hardware: a message will soon arrive from the queue * so the subsystem requests the driver to prepare the transfer hardware * by issuing this call @@ -424,6 +426,7 @@ struct spi_master { bool cur_msg_prepared; bool cur_msg_mapped; struct completion xfer_completion; + size_t max_dma_len; int (*prepare_transfer_hardware)(struct spi_master *master); int (*transfer_one_message)(struct spi_master *master, @@ -536,6 +539,8 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); * (optionally) changing the chipselect status, then starting * the next transfer or completing this @spi_message. * @transfer_list: transfers are sequenced through @spi_message.transfers + * @tx_sg: Scatterlist for transmit, currently not for client use + * @rx_sg: Scatterlist for receive, currently not for client use * * SPI transfers always write the same number of bytes as they read. * Protocol drivers should always provide @rx_buf and/or @tx_buf. @@ -603,6 +608,8 @@ struct spi_transfer { dma_addr_t tx_dma; dma_addr_t rx_dma; + struct sg_table tx_sg; + struct sg_table rx_sg; unsigned cs_change:1; unsigned tx_nbits:3;