[PATCH V2 0/5] spi: spi-message transformation framework

[PATCH V2 0/5] spi: spi-message transformation framework

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This patchset implements a spi-message transformation framework in
SPI-core, that allows drivers to transfor individual spi messages
into something that can more easily get handled by the HW.

This would typically be used for HW which has some limitations on
DMA alignment or max transfer sizes.

This patchset implements at this very moment only splitting
transfers longer than 60k in size into multiple transfers.

Future patches based on this basic patchset:
* split misalligned transfers (there are probably multiple
  variations necessary for different HW)
* merge multiple small transfers into a single transfer

The patch-set essentially is based arround spi resource
management on a spi_message basis, which was inspired by
devres.

This framework could also get used to handle spi_unmap_buf
calls with the framework.

Right now the drivers themselves have to implement
spi_master.translate_message, but as soon as the "typical"
HW-variations become apparent this may be reduced to assign
one of a set of "standard" translation methods to
spi_master.translate_message instead, but this may require
some additional parametrization of the HW characteristics.

The whole patch set has been tested successfully on a RPI with:
* spi_loopback_test
* enc28j60
* fb_st7735r - framebuffer playing BigBuckBunny
* mmc-spi with an out of tree patch to work arround the mmc_spi
  internal dma mapping issues, that inhibits the driver from working
  correctly - this got introduced with commit 0589342c27944e50
   ("of: set dma_mask to point to coherent_dma_mask")

Martin Sperl (5):
  spi: core: added spi_resource management
  spi: core: add spi_replace_transfers method
  spi: core: add spi_split_transfers_maxsize
  spi: core add spi_master.translate_message
  spi: bcm2835: make use of the spi_translate_message methods

 drivers/spi/spi-bcm2835.c |   31 +++--
 drivers/spi/spi.c         |  339 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h   |   92 ++++++++++++
 3 files changed, 448 insertions(+), 14 deletions(-)

--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 1/5] spi: core: added spi_resource management

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

SPI resource management framework used while processing a spi_message
via the spi-core.

The basic idea is taken from dev_res, but as the allocation may happen
fairly frequently, some provisioning (in the form of an unused spi_device
pointer argument to spi_res_alloc) has been made so that at a later stage
we may implement reuse objects allocated earlier avoiding the repeated
allocation by keeping a cache of objects that we can reuse.

This framework can get used for:
* rewriting spi_messages
  * to fullfill alignment requirements of the spi_master HW
    there are at least 2 variants of this:
    * a dma transfer does not have to be aligned, but it is always
      a multiple of 4/8/16, which poses issues at the end of the first page
      where the length of the first DMA transfer would not be a multiple of
    * a dma transfer ALWAYS has to be aligned on spi_transfer.rx/tx_buf
* to fullfill transfer length requirements
  (e.g: transfers need to be less than 64k)
* consolidate spi_messages with multiple transfers into a single transfer
  when the total transfer length is below a threshold.
* reimplement spi_unmap_buf without explicitly needing to check for it.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |   93 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |   36 ++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2b0a8ec..fb39d23 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1007,6 +1007,8 @@ out:
 	if (msg->status && master->handle_err)
 		master->handle_err(master, msg);

+	spi_res_release(master, msg);
+
 	spi_finalize_current_message(master);

 	return ret;
@@ -1991,6 +1993,97 @@ struct spi_master *spi_busnum_to_master(u16 bus_num)
 }
 EXPORT_SYMBOL_GPL(spi_busnum_to_master);

+/*-------------------------------------------------------------------------*/
+
+/* Core methods for SPI resource management */
+
+/**
+ * spi_res_alloc - allocate a spi resource that is life-cycle managed
+ *                 during the processing of a spi_message while using
+ *                 spi_transfer_one
+ * @spi:     the spi device for which we allocate memory
+ * @release: the release code to execute for this resource
+ * @size:    size to alloc and return
+ * @gfp:     GFP allocation flags
+ *
+ * Return: the pointer to the allocated data
+ *
+ * This may get enhanced in the future to allocate from a memory pool
+ * of the @spi_device or @spi_master to avoid repeated allocations.
+ */
+void *spi_res_alloc(struct spi_device *spi,
+		    spi_res_release_t release,
+		    size_t size, gfp_t gfp)
+{
+	struct spi_res *sres;
+	size_t tot_size = sizeof(struct spi_res) + size;
+
+	sres = kzalloc(tot_size, gfp);
+	if (!sres)
+		return NULL;
+
+	INIT_LIST_HEAD(&sres->entry);
+	sres->release = release;
+
+	return sres->data;
+}
+EXPORT_SYMBOL_GPL(spi_res_alloc);
+
+/**
+ * spi_res_free - free an spi resource
+ * @res: pointer to the custom data of a resource
+ *
+ */
+void spi_res_free(void *res)
+{
+	struct spi_res *sres;
+
+	if (res) {
+		sres = container_of(res, struct spi_res, data);
+
+		WARN_ON(!list_empty(&sres->entry));
+		kfree(sres);
+	}
+}
+EXPORT_SYMBOL_GPL(spi_res_free);
+
+/**
+ * spi_res_add - add a spi_res to the spi_message
+ * @message: the spi message
+ * @res:     the spi_resource
+ */
+void spi_res_add(struct spi_message *message, void *res)
+{
+	struct spi_res *sres = container_of(res, struct spi_res, data);
+
+	WARN_ON(!list_empty(&sres->entry));
+	list_add_tail(&sres->entry, &message->resources);
+}
+EXPORT_SYMBOL_GPL(spi_res_add);
+
+/**
+ * spi_res_release - release all spi resources for this message
+ * @master:  the @spi_master
+ * @message: the @spi_message
+ */
+void spi_res_release(struct spi_master *master,
+		     struct spi_message *message)
+{
+	struct spi_res *res;
+
+	while (!list_empty(&message->resources)) {
+		res = list_last_entry(&message->resources,
+				      struct spi_res, entry);
+
+		if (res->release)
+			res->release(master, message, res->data);
+
+		list_del(&res->entry);
+
+		kfree(res);
+	}
+}
+EXPORT_SYMBOL_GPL(spi_res_release);

 /*-------------------------------------------------------------------------*/

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4c54d47..7e74e0e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -576,6 +576,37 @@ extern void spi_unregister_master(struct spi_master *master);

 extern struct spi_master *spi_busnum_to_master(u16 busnum);

+/*
+ * SPI resource management while processing a SPI message
+ */
+
+/**
+ * struct spi_res - spi resource management structure
+ * @entry:   list entry
+ * @release: release code called prior to freeing this resource
+ * @data:    extra data allocated for the specific use-case
+ *
+ * this is based on ideas from devres, but focused on life-cycle
+ * management during spi_message processing
+ */
+typedef void (*spi_res_release_t)(struct spi_master *master,
+				  struct spi_message *msg,
+				  void *res);
+struct spi_res {
+	struct list_head        entry;
+	spi_res_release_t       release;
+	unsigned long long      data[]; /* guarantee ull alignment */
+};
+
+extern void *spi_res_alloc(struct spi_device *spi,
+			   spi_res_release_t release,
+			   size_t size, gfp_t gfp);
+extern void spi_res_add(struct spi_message *message, void *res);
+extern void spi_res_free(void *res);
+
+extern void spi_res_release(struct spi_master *master,
+			    struct spi_message *message);
+
 /*---------------------------------------------------------------------------*/

 /*
@@ -714,6 +745,7 @@ struct spi_transfer {
  * @status: zero for success, else negative errno
  * @queue: for use by whichever driver currently owns the message
  * @state: for use by whichever driver currently owns the message
+ * @resources: for resource management when the spi message is processed
  *
  * A @spi_message is used to execute an atomic sequence of data transfers,
  * each represented by a struct spi_transfer.  The sequence is "atomic"
@@ -760,11 +792,15 @@ struct spi_message {
 	 */
 	struct list_head	queue;
 	void			*state;
+
+	/* list of spi_res reources when the spi message is processed */
+	struct list_head        resources;
 };

 static inline void spi_message_init_no_memset(struct spi_message *m)
 {
 	INIT_LIST_HEAD(&m->transfers);
+	INIT_LIST_HEAD(&m->resources);
 }

 static inline void spi_message_init(struct spi_message *m)
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 2/5] spi: core: add spi_replace_transfers method

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Add the spi_replace_transfers method that can get used
to replace some spi_transfers from a spi_message with other
transfers.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |  130 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |   44 ++++++++++++++++
 2 files changed, 174 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fb39d23..97c77b3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2087,6 +2087,136 @@ EXPORT_SYMBOL_GPL(spi_res_release);

 /*-------------------------------------------------------------------------*/

+/* Core methods for spi_message alterations */
+
+static void __spi_replace_transfers_release(struct spi_master *master,
+					    struct spi_message *msg,
+					    void *res)
+{
+	struct spi_replaced_transfers *srt = res;
+	int i;
+
+	/* call extra callback if requested */
+	if (srt->release)
+		srt->release(master, msg, res);
+
+	/* insert replaced transfers back into the message */
+	list_splice(&srt->replaced_transfers, srt->replaced_after);
+
+	/* remove the formerly inserted entries */
+	for (i = 0; i < srt->inserted; i++)
+		list_del(&srt->inserted_transfers[i].transfer_list);
+}
+
+/**
+ * spi_replace_transfers - replace transfers with several transfers
+ *                         and register change with spi_message.resources
+ * @msg:           the spi_message we work upon
+ * @xfer_first:    the first spi_transfer we want to replace
+ * @remove:        number of transfers to remove
+ * @insert:        the number of transfers we want to insert instead
+ * @release:       extra release code necessary in some circumstances
+ * @extradatasize: extra data to allocate (with alignment guarantees
+ *                 of struct @spi_transfer)
+ *
+ * Returns: pointer to @spi_replaced_transfers,
+ *          NULL in case of errors
+ */
+struct spi_replaced_transfers *spi_replace_transfers(
+	struct spi_message *msg,
+	struct spi_transfer *xfer_first,
+	size_t remove,
+	size_t insert,
+	spi_replaced_release_t release,
+	size_t extradatasize)
+{
+	struct spi_replaced_transfers *srt;
+	int i;
+
+	/* allocate the structure using spi_res */
+	srt = spi_res_alloc(msg->spi, __spi_replace_transfers_release,
+			    insert * sizeof(struct spi_transfer)
+			    + sizeof(struct spi_replaced_transfers)
+			    + extradatasize,
+			    0);
+	if (!srt)
+		return NULL;
+
+	/* the release code to invoke before running the generic release */
+	srt->release = release;
+
+	/* assign extradata */
+	if (extradatasize)
+		srt->extradata = &srt->inserted_transfers[srt->inserted];
+
+	/* init the replaced_transfers list */
+	INIT_LIST_HEAD(&srt->replaced_transfers);
+
+	/* assign the list_entry after which we should reinsert
+	 * the @replaced_transfers - it may be spi_message.messages!
+	 */
+	srt->replaced_after = xfer_first->transfer_list.prev;
+
+	/* remove the requested number of transfers */
+	for (i = 0; i < remove; i++) {
+		/* if the entry after replaced_after it is msg->transfers
+		 * then we have been requested to remove more transfers
+		 * than are in the list
+		 */
+		if (srt->replaced_after->next == &msg->transfers) {
+			dev_err(&msg->spi->dev,
+				"requested to remove more spi_transfers than are available\n");
+			/* insert replaced transfers back into the message */
+			list_splice(&srt->replaced_transfers,
+				    srt->replaced_after);
+
+			/* free the spi_replace_transfer structure */
+			spi_res_free(srt);
+
+			/* and return with an error */
+			return NULL;
+		}
+
+		/* remove the entry after replaced_after from list of
+		 * transfers and add it to list of replaced_transfers
+		 */
+		list_move_tail(srt->replaced_after->next,
+			       &srt->replaced_transfers);
+	}
+
+	/* create copy of the given xfer with identical settings
+	 * based on the first transfer to get removed
+	 * note that we run in reverse to keep the array order
+	 * match the order in transfer_list
+	 */
+	srt->inserted = insert;
+	for (i = insert - 1; i >= 0 ; i--) {
+		/* copy all spi_transfer data */
+		memcpy(&srt->inserted_transfers[i], xfer_first,
+		       sizeof(*xfer_first));
+
+		/* for all but the last inserted_transfer,
+		 * clear some settings
+		 */
+		if (i < insert - 1) {
+			srt->inserted_transfers[i].cs_change = 0;
+			srt->inserted_transfers[i].delay_usecs = 0;
+		}
+
+		/* add after srt->replaced_after */
+		list_add(&srt->inserted_transfers[i].transfer_list,
+			 srt->replaced_after);
+	}
+
+	/* and register it with spi_res/spi_message */
+	spi_res_add(msg, srt);
+
+	return srt;
+}
+EXPORT_SYMBOL_GPL(spi_replace_transfers);
+
+/*-------------------------------------------------------------------------*/
+
 /* Core methods for SPI master protocol drivers.  Some of the
  * other core methods are currently defined as inline functions.
  */
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 7e74e0e..35b5f17 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -875,6 +875,50 @@ extern int spi_async_locked(struct spi_device *spi,

 /*---------------------------------------------------------------------------*/

+/* SPI transfer replacement methods which make use of spi_res */
+
+/**
+ * struct spi_replaced_transfers - structure describing the spi_transfer
+ *                                 replacements that have occurred
+ *                                 so that they can get reverted
+ * @release:            some extra release code to get executed prior to
+ *                      relasing this structure
+ * @extradata:          pointer to some extra data if requested or NULL
+ * @replaced_transfers: transfers that have been replaced and which need
+ *                      to get restored
+ * @replaced_after:     the transfer after which the @replaced_transfers
+ *                      are to get re-inserted
+ * @inserted:           number of transfers inserted
+ * @inserted_transfers: array of spi_transfers of array-size @inserted,
+ *                      that have been replacing replaced_transfers
+ *
+ * note: that @extradata will point to @inserted_transfers[@inserted]
+ * if some extra allocation is requested, so alignment will be the same
+ * as for spi_transfers
+ */
+struct spi_replaced_transfers;
+typedef void (*spi_replaced_release_t)(struct spi_master *master,
+				       struct spi_message *msg,
+				       struct spi_replaced_transfers *res);
+struct spi_replaced_transfers {
+	spi_replaced_release_t release;
+	void *extradata;
+	struct list_head replaced_transfers;
+	struct list_head *replaced_after;
+	size_t inserted;
+	struct spi_transfer inserted_transfers[];
+};
+
+extern struct spi_replaced_transfers *spi_replace_transfers(
+	struct spi_message *msg,
+	struct spi_transfer *xfer_first,
+	size_t remove,
+	size_t insert,
+	spi_replaced_release_t release,
+	size_t extradatasize);
+
+/*---------------------------------------------------------------------------*/
+
 /* All these synchronous SPI transfer routines are utilities layered
  * over the core async transfer primitive.  Here, "synchronous" means
  * they will sleep uninterruptibly until the async transfer completes.
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 3/5] spi: core: add spi_split_transfers_maxsize

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Add spi_split_transfers_maxsize method that splits
spi_transfers transparently into multiple transfers
that are below the given max-size.

This makes use of the spi_res framework via
spi_replace_transfers to allocate/free the extra
transfers as well as reverting back the changes applied
while processing the spi_message.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |  101 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |    8 ++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 97c77b3..507dff0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -145,6 +145,8 @@ SPI_STATISTICS_TRANSFER_BYTES_HISTO(14, "16384-32767");
 SPI_STATISTICS_TRANSFER_BYTES_HISTO(15, "32768-65535");
 SPI_STATISTICS_TRANSFER_BYTES_HISTO(16, "65536+");

+SPI_STATISTICS_SHOW(transfers_split_maxsize, "%lu");
+
 static struct attribute *spi_dev_attrs[] = {
 	&dev_attr_modalias.attr,
 	NULL,
@@ -182,6 +184,7 @@ static struct attribute *spi_device_statistics_attrs[] = {
 	&dev_attr_spi_device_transfer_bytes_histo14.attr,
 	&dev_attr_spi_device_transfer_bytes_histo15.attr,
 	&dev_attr_spi_device_transfer_bytes_histo16.attr,
+	&dev_attr_spi_device_transfers_split_maxsize.attr,
 	NULL,
 };

@@ -224,6 +227,7 @@ static struct attribute *spi_master_statistics_attrs[] = {
 	&dev_attr_spi_master_transfer_bytes_histo14.attr,
 	&dev_attr_spi_master_transfer_bytes_histo15.attr,
 	&dev_attr_spi_master_transfer_bytes_histo16.attr,
+	&dev_attr_spi_master_transfers_split_maxsize.attr,
 	NULL,
 };

@@ -2215,6 +2219,103 @@ struct spi_replaced_transfers *spi_replace_transfers(
 }
 EXPORT_SYMBOL_GPL(spi_replace_transfers);

+int __spi_split_transfer_maxsize(struct spi_master *master,
+				 struct spi_message *msg,
+				 struct spi_transfer **xferp,
+				 size_t maxsize)
+{
+	struct spi_transfer *xfer = *xferp, *xfers;
+	struct spi_replaced_transfers *srt;
+	size_t offset;
+	int count, i;
+
+	/* calculate how many we have to replace */
+	count = DIV_ROUND_UP(xfer->len, maxsize);
+
+	/* create replacement */
+	srt = spi_replace_transfers(msg, xfer, 1, count, NULL, 0);
+	if (!srt)
+		return -ENOMEM;
+	xfers = srt->inserted_transfers;
+
+	/* now handle each of those newly inserted spi_transfers
+	 * note that the replacements spi_transfers all are preset
+	 * to the same values as *xferp, so tx_buf, rx_buf and len
+	 * are all identical (as well as most others)
+	 * so we just have to fix up len and the pointers.
+	 *
+	 * this also includes support for the depreciated
+	 * spi_message.is_dma_mapped interface
+	 */
+
+	/* the first transfer just needs the length modified, so we
+	 * run it outside the loop
+	 */
+	xfers[0].len = min(maxsize, xfer[0].len);
+
+	/* all the others need rx_buf/tx_buf also set */
+	for (i = 1, offset = maxsize; i < count; offset += maxsize, i++) {
+		/* update rx_buf, tx_buf and dma */
+		if (xfers[i].rx_buf)
+			xfers[i].rx_buf += offset;
+		if (xfers[i].rx_dma)
+			xfers[i].rx_dma += offset;
+		if (xfers[i].tx_buf)
+			xfers[i].tx_buf += offset;
+		if (xfers[i].tx_dma)
+			xfers[i].tx_dma += offset;
+
+		/* update length */
+		xfers[i].len = min(maxsize, xfers[i].len - offset);
+	}
+
+	/* we set up xferp to the last entry we have inserted,
+	 * so that we skip those already split transfers
+	 */
+	*xferp = &xfers[count - 1];
+
+	/* increment statistics counters */
+	SPI_STATISTICS_INCREMENT_FIELD(&master->statistics,
+				       transfers_split_maxsize);
+	SPI_STATISTICS_INCREMENT_FIELD(&msg->spi->statistics,
+				       transfers_split_maxsize);
+
+	return 0;
+}
+
+/**
+ * spi_split_tranfers_maxsize - split spi transfers into multiple transfers
+ *                              when an individual transfer exceeds a
+ *                              certain size
+ * @master:    the @spi_master for this transfer
+ * @message:   the @spi_message to transform
+ * @max_size:  the maximum when to apply this
+ *
+ * Return: status of transformation
+ */
+int spi_split_transfers_maxsize(struct spi_master *master,
+				struct spi_message *msg,
+				size_t maxsize)
+{
+	struct spi_transfer *xfer;
+	int ret;
+
+	/* iterate over the transfer_list,
+	 * but note that xfer is advanced to the last transfer inserted
+	 */
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (xfer->len > maxsize) {
+			ret = __spi_split_transfer_maxsize(
+				master, msg, &xfer, maxsize);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_split_transfers_maxsize);
+
 /*-------------------------------------------------------------------------*/

 /* Core methods for SPI master protocol drivers.  Some of the
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 35b5f17..f5c9296 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -72,6 +72,8 @@ struct spi_statistics {

 #define SPI_STATISTICS_HISTO_SIZE 17
 	unsigned long transfer_bytes_histo[SPI_STATISTICS_HISTO_SIZE];
+
+	unsigned long transfers_split_maxsize;
 };

 void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
@@ -917,6 +919,12 @@ extern struct spi_replaced_transfers *spi_replace_transfers(
 	spi_replaced_release_t release,
 	size_t extradatasize);

+/* SPI transfer transformation methods */
+
+extern int spi_split_transfers_maxsize(struct spi_master *master,
+				       struct spi_message *msg,
+				       size_t maxsize);
+
 /*---------------------------------------------------------------------------*/

 /* All these synchronous SPI transfer routines are utilities layered
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 4/5] spi: core add spi_master.translate_message

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Add translate_message to spi_master structure to allow
translations of spi_messages to happen independently of
prepare messages.

This "translations" are supposed to be using spi_res to handle the
reverse transformation (see spi_replace_transfers), so no explicit
code is necessary to revert any changes.

dma_mapping of spi_messages could also become part of this processing.

The long-term view is that this translation as well as the
dma_mapping of the messages may happen in a separate thread
when we are queueing those spi_messages - this would allows
for better use of cpu resources as well as reduction of latency
in the case of queuing.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi.c       |   15 +++++++++++++++
 include/linux/spi/spi.h |    4 ++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 507dff0..40fb4cf 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1143,6 +1143,21 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread)

 	trace_spi_message_start(master->cur_msg);

+	/* under some circumstances (i.e: when queuing a message) this
+	 * could get moved to a separate thread to reduce latencies
+	 * this could also handle the mapping of spi_transfers
+	 */
+	if (master->translate_message) {
+		ret = master->translate_message(master, master->cur_msg);
+		if (ret) {
+			dev_err(&master->dev,
+				"failed to translate message: %d\n", ret);
+			master->cur_msg->status = ret;
+			spi_finalize_current_message(master);
+			return;
+		}
+	}
+
 	if (master->prepare_message) {
 		ret = master->prepare_message(master, master->cur_msg);
 		if (ret) {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index f5c9296..55e5864 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -363,6 +363,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @handle_err: the subsystem calls the driver to handle an error that occurs
  *		in the generic implementation of transfer_one_message().
  * @unprepare_message: undo any work done by prepare_message().
+ * @translate_message: apply some message translations to optimize for the
+ *                     spi_master
  * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS
  *	number. Any individual value may be -ENOENT for CS lines that
  *	are not GPIOs (driven by the SPI controller itself).
@@ -509,6 +511,8 @@ struct spi_master {
 			       struct spi_message *message);
 	int (*unprepare_message)(struct spi_master *master,
 				 struct spi_message *message);
+	int (*translate_message)(struct spi_master *master,
+				 struct spi_message *message);

 	/*
 	 * These hooks are for drivers that use a generic implementation
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 5/5] spi: bcm2835: split long transfers

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Start to use spi_translate_message method when running in
DMA mode and (for now) split spi_transfers with len > 60kB
transparently into multiple transfers using the core method
spi_split_transfers_maxsize.

Eventually the bcm2835_translate_message may get moved into
SPI-core as shared code (when we find the common denominators
for different HW)

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index cf04960..03c889d 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -353,6 +353,14 @@ static int bcm2835_spi_transfer_one_dma(struct spi_master *master,
 	return 1;
 }

+static int bcm2835_translate_message(struct spi_master *master,
+				     struct spi_message *message)
+{
+	/* translate the message */
+	return spi_split_transfers_maxsize(master, message,
+					   master->max_dma_len);
+}
+
 static bool bcm2835_spi_can_dma(struct spi_master *master,
 				struct spi_device *spi,
 				struct spi_transfer *tfr)
@@ -365,19 +373,6 @@ static bool bcm2835_spi_can_dma(struct spi_master *master,
 	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
 		return false;

-	/* BCM2835_SPI_DLEN has defined a max transfer size as
-	 * 16 bit, so max is 65535
-	 * we can revisit this by using an alternative transfer
-	 * method - ideally this would get done without any more
-	 * interaction...
-	 */
-	if (tfr->len > 65535) {
-		dev_warn_once(&spi->dev,
-			      "transfer size of %d too big for dma-transfer\n",
-			      tfr->len);
-		return false;
-	}
-
 	/* if we run rx/tx_buf with word aligned addresses then we are OK */
 	if ((((size_t)tfr->rx_buf & 3) == 0) &&
 	    (((size_t)tfr->tx_buf & 3) == 0))
@@ -461,7 +456,15 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)

 	/* all went well, so set can_dma */
 	master->can_dma = bcm2835_spi_can_dma;
-	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
+
+	/* also set up transform message */
+	master->translate_message = bcm2835_translate_message;
+
+	/* the max_dma_size limited by BCM2835_SPI_DLEN is actually 65535,
+	 * but for al practical purposes we use 15 pages (60k)
+	 */
+	master->max_dma_len = 15 * PAGE_SIZE;
+	master->dma_alignment = 4; /* word alignment is recommended */
 	/* need to do TX AND RX DMA, so we need dummy buffers */
 	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;

--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 0/5] spi: spi-message transformation framework

[Martin Sperl]

On 07.12.2015 16:21, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> This patchset implements a spi-message transformation framework in
> SPI-core, that allows drivers to transfor individual spi messages
> into something that can more easily get handled by the HW.
>
> This would typically be used for HW which has some limitations on
> DMA alignment or max transfer sizes.
>
> This patchset implements at this very moment only splitting
> transfers longer than 60k in size into multiple transfers.
>
> Future patches based on this basic patchset:
> * split misalligned transfers (there are probably multiple
>    variations necessary for different HW)
> * merge multiple small transfers into a single transfer
>
> The patch-set essentially is based arround spi resource
> management on a spi_message basis, which was inspired by
> devres.
>
> This framework could also get used to handle spi_unmap_buf
> calls with the framework.
>
> Right now the drivers themselves have to implement
> spi_master.translate_message, but as soon as the "typical"
> HW-variations become apparent this may be reduced to assign
> one of a set of "standard" translation methods to
> spi_master.translate_message instead, but this may require
> some additional parametrization of the HW characteristics.
>
> The whole patch set has been tested successfully on a RPI with:
> * spi_loopback_test
> * enc28j60
> * fb_st7735r - framebuffer playing BigBuckBunny
> * mmc-spi with an out of tree patch to work arround the mmc_spi
>    internal dma mapping issues, that inhibits the driver from working
>    correctly - this got introduced with commit 0589342c27944e50
>     ("of: set dma_mask to point to coherent_dma_mask")
>
> Martin Sperl (5):
>    spi: core: added spi_resource management
>    spi: core: add spi_replace_transfers method
>    spi: core: add spi_split_transfers_maxsize
>    spi: core add spi_master.translate_message
>    spi: bcm2835: make use of the spi_translate_message methods
>
>   drivers/spi/spi-bcm2835.c |   31 +++--
>   drivers/spi/spi.c         |  339 +++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/spi/spi.h   |   92 ++++++++++++
>   3 files changed, 448 insertions(+), 14 deletions(-)

Please note that Patch 1/5 (spi: core: added spi_resource management)
requires that method spi_message_init_no_memset exists(see patch-set
for spi-loopback-test) - this note did not make it into the description
of the second version of this patchset.

In the meantime I have implemented 2 more transformations:
* alignment of buffers (first spi_transfer.tx/rx_buf buffer
   remains unaligned, but has spi_transfer.len < alignment,
   second spi_transfer is fully alligned for rx/tx_buf with
   copying of tx_buf to make it alligned in the case where
   rx_buf and tx_buf are misaligned to different offsets)
* merging multiple tiny transfers

One code issue showed up (but that impacted only the new use-cases)
and there was a missing kerneldoc for one of the spi_statistics
members - these are 2 separate patches.

Should I send these separately or do you want a big patchset that
contains all of them?

Here the some of the statistics when running my spi-loopback-test:
==> /sys/class/spi_master/spi32766/statistics/transfers
1588
==> /sys/class/spi_master/spi32766/statistics/transfers_merged
102
==> /sys/class/spi_master/spi32766/statistics/transfers_split_maxsize
160
==> /sys/class/spi_master/spi32766/statistics/transfers_split_unaligned
372

Based on the above I am starting to think of readying a default
implementation for translation that drivers may use:

int spi_translate_message_size_align_merge(
         struct spi_master *master, struct spi_message *message)
{
         int ret;

         /* translate the message */

         /* fix alignment of transfers by splitting rx_buf/tx_buf
          * (and worsted case copying tx_buf)
          */
         ret = spi_split_transfers_unaligned(master, message,
                                             master->min_dma_len,
                                             master->dma_alignment);
         if (ret)
                 return ret;

         /* limit transfer length */
         if (master->max_dma_len) {
                 ret = spi_split_transfers_maxsize(master, message,
                                                   master->max_dma_len);
                 if (ret)
                         return ret;
         }

         /* merge spi_transfers up to a full page */
         ret = spi_merge_transfers(master, message, 2, PAGE_SIZE);

         return ret;
}

The spi_master driver would need to set these:
	master->translate_message =
		spi_translate_message_size_align_merge;
	master->min_dma_len = BCM2835_SPI_DMA_MIN_LENGTH;
	master->max_dma_len = 15 * PAGE_SIZE;
	master->dma_alignment = 4;

As seen above we would need to define a min_dma_len that can get used
as a parameter to align transfers (if a driver is doing PIO for short
transfers, then it makes no sense aligning them in the first place).

This value could also get used in a default implementation of can_dma.

Note that we could also expose min_dma_size via sysfs.

Re: [PATCH V2 1/5] spi: core: added spi_resource management

[Andy Shevchenko]

On Mon, Dec 7, 2015 at 5:21 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> SPI resource management framework used while processing a spi_message
> via the spi-core.
>
> The basic idea is taken from dev_res, but as the allocation may happen
> fairly frequently, some provisioning (in the form of an unused spi_device
> pointer argument to spi_res_alloc) has been made so that at a later stage
> we may implement reuse objects allocated earlier avoiding the repeated
> allocation by keeping a cache of objects that we can reuse.
>
> This framework can get used for:
> * rewriting spi_messages
>   * to fullfill alignment requirements of the spi_master HW
>     there are at least 2 variants of this:
>     * a dma transfer does not have to be aligned, but it is always
>       a multiple of 4/8/16, which poses issues at the end of the first page
>       where the length of the first DMA transfer would not be a multiple of
>     * a dma transfer ALWAYS has to be aligned on spi_transfer.rx/tx_buf
> * to fullfill transfer length requirements
>   (e.g: transfers need to be less than 64k)
> * consolidate spi_messages with multiple transfers into a single transfer
>   when the total transfer length is below a threshold.
> * reimplement spi_unmap_buf without explicitly needing to check for it.

I have no thoughts (*) about the whole idea, but below some comments
regarding implementation.

(*) [Yet] For a fist glance looked a bit complicated.

>
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> ---
>  drivers/spi/spi.c       |   93 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |   36 ++++++++++++++++++
>  2 files changed, 129 insertions(+)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 2b0a8ec..fb39d23 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1007,6 +1007,8 @@ out:
>         if (msg->status && master->handle_err)
>                 master->handle_err(master, msg);
>
> +       spi_res_release(master, msg);
> +

Why it's in this patch and not later?

>         spi_finalize_current_message(master);
>
>         return ret;
> @@ -1991,6 +1993,97 @@ struct spi_master *spi_busnum_to_master(u16 bus_num)
>  }
>  EXPORT_SYMBOL_GPL(spi_busnum_to_master);
>
> +/*-------------------------------------------------------------------------*/
> +
> +/* Core methods for SPI resource management */
> +
> +/**
> + * spi_res_alloc - allocate a spi resource that is life-cycle managed
> + *                 during the processing of a spi_message while using
> + *                 spi_transfer_one
> + * @spi:     the spi device for which we allocate memory
> + * @release: the release code to execute for this resource
> + * @size:    size to alloc and return
> + * @gfp:     GFP allocation flags
> + *
> + * Return: the pointer to the allocated data
> + *
> + * This may get enhanced in the future to allocate from a memory pool
> + * of the @spi_device or @spi_master to avoid repeated allocations.
> + */
> +void *spi_res_alloc(struct spi_device *spi,
> +                   spi_res_release_t release,
> +                   size_t size, gfp_t gfp)
> +{
> +       struct spi_res *sres;

> +       size_t tot_size = sizeof(struct spi_res) + size;

Looks like you create a variable for one use. And I'm pretty sure the
80 character limit is not a case here.

> +
> +       sres = kzalloc(tot_size, gfp);
> +       if (!sres)
> +               return NULL;
> +
> +       INIT_LIST_HEAD(&sres->entry);
> +       sres->release = release;
> +
> +       return sres->data;
> +}
> +EXPORT_SYMBOL_GPL(spi_res_alloc);
> +
> +/**
> + * spi_res_free - free an spi resource
> + * @res: pointer to the custom data of a resource
> + *
> + */
> +void spi_res_free(void *res)
> +{
> +       struct spi_res *sres;
> +
> +       if (res) {

if (!res)
 return;
?

> +               sres = container_of(res, struct spi_res, data);

This might be at the definition block even if res == NULL.

> +
> +               WARN_ON(!list_empty(&sres->entry));
> +               kfree(sres);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(spi_res_free);
> +
> +/**
> + * spi_res_add - add a spi_res to the spi_message
> + * @message: the spi message
> + * @res:     the spi_resource
> + */
> +void spi_res_add(struct spi_message *message, void *res)
> +{
> +       struct spi_res *sres = container_of(res, struct spi_res, data);
> +
> +       WARN_ON(!list_empty(&sres->entry));
> +       list_add_tail(&sres->entry, &message->resources);
> +}
> +EXPORT_SYMBOL_GPL(spi_res_add);
> +
> +/**
> + * spi_res_release - release all spi resources for this message
> + * @master:  the @spi_master
> + * @message: the @spi_message
> + */
> +void spi_res_release(struct spi_master *master,
> +                    struct spi_message *message)
> +{
> +       struct spi_res *res;
> +
> +       while (!list_empty(&message->resources)) {
> +               res = list_last_entry(&message->resources,
> +                                     struct spi_res, entry);

Isn't the
list_for_each_entry_safe_reverse() ?

> +
> +               if (res->release)
> +                       res->release(master, message, res->data);
> +
> +               list_del(&res->entry);
> +
> +               kfree(res);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(spi_res_release);
>
>  /*-------------------------------------------------------------------------*/
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4c54d47..7e74e0e 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -576,6 +576,37 @@ extern void spi_unregister_master(struct spi_master *master);
>
>  extern struct spi_master *spi_busnum_to_master(u16 busnum);
>
> +/*
> + * SPI resource management while processing a SPI message
> + */
> +
> +/**
> + * struct spi_res - spi resource management structure
> + * @entry:   list entry
> + * @release: release code called prior to freeing this resource
> + * @data:    extra data allocated for the specific use-case
> + *
> + * this is based on ideas from devres, but focused on life-cycle
> + * management during spi_message processing
> + */
> +typedef void (*spi_res_release_t)(struct spi_master *master,
> +                                 struct spi_message *msg,
> +                                 void *res);
> +struct spi_res {
> +       struct list_head        entry;
> +       spi_res_release_t       release;
> +       unsigned long long      data[]; /* guarantee ull alignment */

Isn't better to use __aligned(8)?
And why not simple void *?

> +};


> +
> +extern void *spi_res_alloc(struct spi_device *spi,
> +                          spi_res_release_t release,
> +                          size_t size, gfp_t gfp);
> +extern void spi_res_add(struct spi_message *message, void *res);
> +extern void spi_res_free(void *res);
> +
> +extern void spi_res_release(struct spi_master *master,
> +                           struct spi_message *message);
> +
>  /*---------------------------------------------------------------------------*/
>
>  /*
> @@ -714,6 +745,7 @@ struct spi_transfer {
>   * @status: zero for success, else negative errno
>   * @queue: for use by whichever driver currently owns the message
>   * @state: for use by whichever driver currently owns the message
> + * @resources: for resource management when the spi message is processed
>   *
>   * A @spi_message is used to execute an atomic sequence of data transfers,
>   * each represented by a struct spi_transfer.  The sequence is "atomic"
> @@ -760,11 +792,15 @@ struct spi_message {
>          */
>         struct list_head        queue;
>         void                    *state;
> +
> +       /* list of spi_res reources when the spi message is processed */
> +       struct list_head        resources;
>  };
>
>  static inline void spi_message_init_no_memset(struct spi_message *m)
>  {
>         INIT_LIST_HEAD(&m->transfers);
> +       INIT_LIST_HEAD(&m->resources);
>  }
>
>  static inline void spi_message_init(struct spi_message *m)
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/5] spi: core: add spi_replace_transfers method

[Andy Shevchenko]

On Mon, Dec 7, 2015 at 5:21 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> Add the spi_replace_transfers method that can get used
> to replace some spi_transfers from a spi_message with other
> transfers.

Few more implementation comments.

>
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> ---
>  drivers/spi/spi.c       |  130 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |   44 ++++++++++++++++
>  2 files changed, 174 insertions(+)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index fb39d23..97c77b3 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2087,6 +2087,136 @@ EXPORT_SYMBOL_GPL(spi_res_release);
>
>  /*-------------------------------------------------------------------------*/
>
> +/* Core methods for spi_message alterations */
> +
> +static void __spi_replace_transfers_release(struct spi_master *master,
> +                                           struct spi_message *msg,
> +                                           void *res)
> +{
> +       struct spi_replaced_transfers *srt = res;

srt sounds magic
rxfer might be better?

> +       int i;

unsigned int i; or size_t to be in align with inserted field.

> +
> +       /* call extra callback if requested */
> +       if (srt->release)
> +               srt->release(master, msg, res);
> +
> +       /* insert replaced transfers back into the message */
> +       list_splice(&srt->replaced_transfers, srt->replaced_after);
> +
> +       /* remove the formerly inserted entries */
> +       for (i = 0; i < srt->inserted; i++)
> +               list_del(&srt->inserted_transfers[i].transfer_list);
> +}
> +
> +/**
> + * spi_replace_transfers - replace transfers with several transfers
> + *                         and register change with spi_message.resources
> + * @msg:           the spi_message we work upon
> + * @xfer_first:    the first spi_transfer we want to replace
> + * @remove:        number of transfers to remove
> + * @insert:        the number of transfers we want to insert instead
> + * @release:       extra release code necessary in some circumstances
> + * @extradatasize: extra data to allocate (with alignment guarantees
> + *                 of struct @spi_transfer)
> + *
> + * Returns: pointer to @spi_replaced_transfers,
> + *          NULL in case of errors
> + */
> +struct spi_replaced_transfers *spi_replace_transfers(
> +       struct spi_message *msg,
> +       struct spi_transfer *xfer_first,
> +       size_t remove,
> +       size_t insert,
> +       spi_replaced_release_t release,
> +       size_t extradatasize)
> +{
> +       struct spi_replaced_transfers *srt;
> +       int i;

Ditto.

> +
> +       /* allocate the structure using spi_res */
> +       srt = spi_res_alloc(msg->spi, __spi_replace_transfers_release,
> +                           insert * sizeof(struct spi_transfer)
> +                           + sizeof(struct spi_replaced_transfers)
> +                           + extradatasize,
> +                           0);
> +       if (!srt)
> +               return NULL;

ERR_PTR for this API seems better to have.

> +
> +       /* the release code to invoke before running the generic release */
> +       srt->release = release;
> +
> +       /* assign extradata */
> +       if (extradatasize)
> +               srt->extradata = &srt->inserted_transfers[srt->inserted];
> +
> +       /* init the replaced_transfers list */
> +       INIT_LIST_HEAD(&srt->replaced_transfers);
> +
> +       /* assign the list_entry after which we should reinsert
> +        * the @replaced_transfers - it may be spi_message.messages!
> +        */
> +       srt->replaced_after = xfer_first->transfer_list.prev;
> +
> +       /* remove the requested number of transfers */
> +       for (i = 0; i < remove; i++) {
> +               /* if the entry after replaced_after it is msg->transfers
> +                * then we have been requested to remove more transfers
> +                * than are in the list
> +                */
> +               if (srt->replaced_after->next == &msg->transfers) {
> +                       dev_err(&msg->spi->dev,
> +                               "requested to remove more spi_transfers than are available\n");
> +                       /* insert replaced transfers back into the message */
> +                       list_splice(&srt->replaced_transfers,
> +                                   srt->replaced_after);
> +
> +                       /* free the spi_replace_transfer structure */
> +                       spi_res_free(srt);
> +
> +                       /* and return with an error */
> +                       return NULL;
> +               }
> +
> +               /* remove the entry after replaced_after from list of
> +                * transfers and add it to list of replaced_transfers
> +                */
> +               list_move_tail(srt->replaced_after->next,
> +                              &srt->replaced_transfers);
> +       }
> +
> +       /* create copy of the given xfer with identical settings
> +        * based on the first transfer to get removed
> +        * note that we run in reverse to keep the array order
> +        * match the order in transfer_list
> +        */
> +       srt->inserted = insert;
> +       for (i = insert - 1; i >= 0 ; i--) {

This is classical pattern for

while (--insert >= 0) {
}

> +               /* copy all spi_transfer data */
> +               memcpy(&srt->inserted_transfers[i], xfer_first,
> +                      sizeof(*xfer_first));
> +
> +               /* for all but the last inserted_transfer,
> +                * clear some settings
> +                */
> +               if (i < insert - 1) {

…and
if (insert) {
}


> +                       srt->inserted_transfers[i].cs_change = 0;
> +                       srt->inserted_transfers[i].delay_usecs = 0;
> +               }
> +
> +               /* add after srt->replaced_after */
> +               list_add(&srt->inserted_transfers[i].transfer_list,
> +                        srt->replaced_after);
> +       }
> +
> +       /* and register it with spi_res/spi_message */
> +       spi_res_add(msg, srt);
> +
> +       return srt;
> +}
> +EXPORT_SYMBOL_GPL(spi_replace_transfers);
> +
> +/*-------------------------------------------------------------------------*/
> +
>  /* Core methods for SPI master protocol drivers.  Some of the
>   * other core methods are currently defined as inline functions.
>   */
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 7e74e0e..35b5f17 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -875,6 +875,50 @@ extern int spi_async_locked(struct spi_device *spi,
>
>  /*---------------------------------------------------------------------------*/
>
> +/* SPI transfer replacement methods which make use of spi_res */
> +
> +/**
> + * struct spi_replaced_transfers - structure describing the spi_transfer
> + *                                 replacements that have occurred
> + *                                 so that they can get reverted
> + * @release:            some extra release code to get executed prior to
> + *                      relasing this structure
> + * @extradata:          pointer to some extra data if requested or NULL
> + * @replaced_transfers: transfers that have been replaced and which need
> + *                      to get restored
> + * @replaced_after:     the transfer after which the @replaced_transfers
> + *                      are to get re-inserted
> + * @inserted:           number of transfers inserted
> + * @inserted_transfers: array of spi_transfers of array-size @inserted,
> + *                      that have been replacing replaced_transfers
> + *
> + * note: that @extradata will point to @inserted_transfers[@inserted]
> + * if some extra allocation is requested, so alignment will be the same
> + * as for spi_transfers
> + */
> +struct spi_replaced_transfers;
> +typedef void (*spi_replaced_release_t)(struct spi_master *master,
> +                                      struct spi_message *msg,
> +                                      struct spi_replaced_transfers *res);
> +struct spi_replaced_transfers {
> +       spi_replaced_release_t release;
> +       void *extradata;
> +       struct list_head replaced_transfers;
> +       struct list_head *replaced_after;
> +       size_t inserted;
> +       struct spi_transfer inserted_transfers[];
> +};
> +
> +extern struct spi_replaced_transfers *spi_replace_transfers(
> +       struct spi_message *msg,
> +       struct spi_transfer *xfer_first,
> +       size_t remove,
> +       size_t insert,
> +       spi_replaced_release_t release,
> +       size_t extradatasize);
> +
> +/*---------------------------------------------------------------------------*/
> +
>  /* All these synchronous SPI transfer routines are utilities layered
>   * over the core async transfer primitive.  Here, "synchronous" means
>   * they will sleep uninterruptibly until the async transfer completes.
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/5] spi: core: added spi_resource management

[Martin Sperl]

> On 11.12.2015, at 15:30, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Mon, Dec 7, 2015 at 5:21 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> 
>> SPI resource management framework used while processing a spi_message
>> 
> 
> I have no thoughts (*) about the whole idea, but below some comments
> regarding implementation.
> 
> (*) [Yet] For a fist glance looked a bit complicated.
Look at all the patch-sets - by now everything (besides dma_mapping) is
implemented and should come in the next version.

It then includes also the spi-loopback-test framework which I use heavily
to test the patches for functional regressions.

> 
>> 
>> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> ---
>> drivers/spi/spi.c       |   93 +++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/spi/spi.h |   36 ++++++++++++++++++
>> 2 files changed, 129 insertions(+)
>> 
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 2b0a8ec..fb39d23 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -1007,6 +1007,8 @@ out:
>>        if (msg->status && master->handle_err)
>>                master->handle_err(master, msg);
>> 
>> +       spi_res_release(master, msg);
>> +
> 
> Why it's in this patch and not later?
Because it needs to get released when we finish the spi_message processing code.
Also it is the only place were we talk directly about spi_res and releasing it.

> 
>>        spi_finalize_current_message(master);
>> 
>>        return ret;
>> @@ -1991,6 +1993,97 @@ struct spi_master *spi_busnum_to_master(u16 bus_num)
>> }
>> EXPORT_SYMBOL_GPL(spi_busnum_to_master);
>> 
>> +/*-------------------------------------------------------------------------*/
>> +
>> +/* Core methods for SPI resource management */
>> +
>> +/**
>> + * spi_res_alloc - allocate a spi resource that is life-cycle managed
>> + *                 during the processing of a spi_message while using
>> + *                 spi_transfer_one
>> + * @spi:     the spi device for which we allocate memory
>> + * @release: the release code to execute for this resource
>> + * @size:    size to alloc and return
>> + * @gfp:     GFP allocation flags
>> + *
>> + * Return: the pointer to the allocated data
>> + *
>> + * This may get enhanced in the future to allocate from a memory pool
>> + * of the @spi_device or @spi_master to avoid repeated allocations.
>> + */
>> +void *spi_res_alloc(struct spi_device *spi,
>> +                   spi_res_release_t release,
>> +                   size_t size, gfp_t gfp)
>> +{
>> +       struct spi_res *sres;
> 
>> +       size_t tot_size = sizeof(struct spi_res) + size;
> 
> Looks like you create a variable for one use. And I'm pretty sure the
> 80 character limit is not a case here.
fixed in next version of the patchset.

> 
>> +
>> +       sres = kzalloc(tot_size, gfp);
>> +       if (!sres)
>> +               return NULL;
>> +
>> +       INIT_LIST_HEAD(&sres->entry);
>> +       sres->release = release;
>> +
>> +       return sres->data;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_res_alloc);
>> +
>> +/**
>> + * spi_res_free - free an spi resource
>> + * @res: pointer to the custom data of a resource
>> + *
>> + */
>> +void spi_res_free(void *res)
>> +{
>> +       struct spi_res *sres;
>> +
>> +       if (res) {
> 
> if (!res)
> return;
> ?
matter of taste I guess - will fix it in the next version.
(I also guess it was taken from the template dev_res)

> 
>> +               sres = container_of(res, struct spi_res, data);
> 
> This might be at the definition block even if res == NULL.

I was skeptical about making this assumption - fixed.

>> +struct spi_res {
>> +       struct list_head        entry;
>> +       spi_res_release_t       release;
>> +       unsigned long long      data[]; /* guarantee ull alignment */
> 
> Isn't better to use __aligned(8)?
> And why not simple void *?
devres does it that way, so I kept it that way...

Thanks,
	Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/5] spi: core: add spi_replace_transfers method

[Martin Sperl]

> On 11.12.2015, at 15:37, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Mon, Dec 7, 2015 at 5:21 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> +/* Core methods for spi_message alterations */
>> +
>> +static void __spi_replace_transfers_release(struct spi_master *master,
>> +                                           struct spi_message *msg,
>> +                                           void *res)
>> +{
>> +       struct spi_replaced_transfers *srt = res;
> 
> srt sounds magic
> rxfer might be better?
srt = Spi_Replaced_Transfer - if you like rxfer better I can replace it.

> 
>> +       int i;
> 
> unsigned int i; or size_t to be in align with inserted field.
will do.

>> +       int i;
> 
> Ditto.
> 
>> +
>> +       /* allocate the structure using spi_res */
>> +       srt = spi_res_alloc(msg->spi, __spi_replace_transfers_release,
>> +                           insert * sizeof(struct spi_transfer)
>> +                           + sizeof(struct spi_replaced_transfers)
>> +                           + extradatasize,
>> +                           0);
>> +       if (!srt)
>> +               return NULL;
> 
> ERR_PTR for this API seems better to have.
Can do that.

>> +       srt->inserted = insert;
>> +       for (i = insert - 1; i >= 0 ; i--) {
> 
> This is classical pattern for
> 
> while (--insert >= 0) {
> }
did it slightly differently as size_t is always >= 0 - 
the main reason for using int here...

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html