[PATCH 0/3] spi: spi-message transformation framework

[PATCH 0/3] 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 of max transfer sizes.

Note that for DMA alignment there probably exist multiple variations
of requirements for different types of HW.

The patch here implements the variation:
* a single entry in the scatter-gather list has to have a length of word
  size unless it is the last entry. Otherwise the SPI-HW FIFOs need to
  get cleared and the pending transfer size reset - this is the
  requirement of the SPI-HW implementation of the bcm2835.
  So the method will split the transfer into up to 2 or 3 transfers in
  such a way that last transfer starts on a word-aligned address and has
  maximum size. the first 1 or 2 transfers will be of only a few bytes
  in size (1,2 or 3 bytes in the case of bcm2835)

But it also can get used to aggregate multiple small spi_transfers into
a single transfer to reduce the number of transfers that need to get
executed. This is preferable as each transfer adds some  overhead -
especially with regards to DMA mapping.

Right now the "correct" methods have to be implemented inside the
spi_master.prepare_message method in the correcto order.
When it becomes clearer what other drivers may require, then we can
move the default implementation also into spi-core.

The patch-set essentially is based arround spi resource management
on a spi_message basis, which was inspired by dev_res.
This could also get used to move spi_unmap_buf calls into this framework
code and there are probably a few other use-cases as well.

The whole patch set has been tested on a raspberry pi with:
* spi_loopback_test (see below for the statistics)
* 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")

With these all my tests succeeded.

Here the spi-statistics after running spi_loopback-test without any
other spi activity directly after reboot:

root@raspcm:/sys/class/spi_master/spi32766/statistics# head *
==> bytes <==
15698948
==> bytes_rx <==
11085044
==> bytes_tx <==
13272208
==> errors <==
0
==> messages <==
752
==> spi_async <==
0
==> spi_sync <==
752
==> spi_sync_immediate <==
752
==> timedout <==
0
==> transfer_bytes_histo_0-1 <==
518
==> transfer_bytes_histo_1024-2047 <==
25
==> transfer_bytes_histo_128-255 <==
88
==> transfer_bytes_histo_16-31 <==
92
==> transfer_bytes_histo_16384-32767 <==
114
==> transfer_bytes_histo_2048-4095 <==
63
==> transfer_bytes_histo_2-3 <==
244
==> transfer_bytes_histo_256-511 <==
25
==> transfer_bytes_histo_32-63 <==
88
==> transfer_bytes_histo_32768-65535 <==
350
==> transfer_bytes_histo_4096-8191 <==
25
==> transfer_bytes_histo_4-7 <==
0
==> transfer_bytes_histo_512-1023 <==
63
==> transfer_bytes_histo_64-127 <==
151
==> transfer_bytes_histo_65536+ <==
0
==> transfer_bytes_histo_8-15 <==
0
==> transfer_bytes_histo_8192-16383 <==
0
==> transfers <==
1846
==> transfers_split_maxsize <==
160
==> transfers_split_unaligned <==
366

Martin Sperl (3):
  spi: added spi_resource management
  spi: add initial set of spi_transfer transformation methods
  spi: bcm2835: moved to the spi_transfer transformation to avoid HW
    restrictions

 drivers/spi/spi-bcm2835.c |   53 ++---
 drivers/spi/spi.c         |  504 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h   |   63 ++++++
 3 files changed, 588 insertions(+), 32 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 1/3] spi: 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 requzirements 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       |  112 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |   36 +++++++++++++++
 2 files changed, 148 insertions(+)

Note that this patch requires the spi_loopback_test [patch 1/2] that
separates the spi_message initialization from memset into
spi_message_init_no_memset.

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2b0a8ec..eecbbe1 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,116 @@ struct spi_master *spi_busnum_to_master(u16 bus_num)
 }
 EXPORT_SYMBOL_GPL(spi_busnum_to_master);

+/*-------------------------------------------------------------------------*/
+
+/* Core methods for SPI resource management */
+
+static struct spi_res *__spi_res_alloc(struct spi_device *spi,
+				       spi_res_release_t release,
+				       size_t size,
+				       gfp_t gfp)
+{
+	size_t tot_size = sizeof(struct spi_res) + size;
+	struct spi_res *sres;
+
+	/* This may get enhanced to allocate from a memory pool of the
+	 * @spi_device or @spi_master to avoid repeated allocations.
+	 */
+	sres = kzalloc(tot_size, gfp);
+	if (unlikely(!sres))
+		return NULL;
+
+	INIT_LIST_HEAD(&sres->entry);
+	sres->release = release;
+
+	return sres;
+}
+
+/**
+ * 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
+ */
+void *spi_res_alloc(struct spi_device *spi,
+		    spi_res_release_t release,
+		    size_t size, gfp_t gfp)
+{
+	struct spi_res *sres;
+
+	sres = __spi_res_alloc(spi, release, size, gfp);
+	if (unlikely(!sres))
+		return NULL;
+
+	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);
+
+static void __spi_res_add(struct spi_message *msg, struct spi_res *sres)
+{
+	WARN_ON(!list_empty(&sres->entry));
+	list_add_tail(&sres->entry, &msg->resources);
+}
+
+/**
+ * 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);
+
+	__spi_res_add(message, sres);
+}
+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 2/3] spi: add initial set of spi_transfer transformation methods

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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

added the following spi_transformation methods:
* spi_split_transfers_first_page_len_not_aligned -
  this splits spi_transfers that are not aligned on rx_buf/tx_buf
  this will create 2 or 3 transfers, where the first 1/2 transfers are
  just there to allow the last transfer to be fully aligned. These first
  transfers will have a length less than alignment.
* spi_split_transfers_maxsize
  this splits the spi_transfer into multiple independent spi_transfers
  all of which will be of size max_size or smaller.

To start these shall get used by the individual drivers in prepare_message,
but some may get moved into spi-core with the correct parametrization in
spi_master.

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

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index eecbbe1..7576131 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -145,6 +145,9 @@ 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");
+SPI_STATISTICS_SHOW(transfers_split_unaligned, "%lu");
+
 static struct attribute *spi_dev_attrs[] = {
 	&dev_attr_modalias.attr,
 	NULL,
@@ -182,6 +185,8 @@ 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,
+	&dev_attr_spi_device_transfers_split_unaligned.attr,
 	NULL,
 };
 
@@ -224,6 +229,8 @@ 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,
+	&dev_attr_spi_master_transfers_split_unaligned.attr,
 	NULL,
 };
 
@@ -2103,6 +2110,391 @@ void spi_res_release(struct spi_master *master,
 	}
 }
 EXPORT_SYMBOL_GPL(spi_res_release);
+/*-------------------------------------------------------------------------*/
+
+/* Core methods for spi_message alterations */
+
+/* the spi_resource structure used */
+struct spi_res_replaced_transfers {
+	spi_res_release_t release;
+	struct list_head replaced_transfers;
+	int inserted;
+	struct spi_transfer xfers[];
+};
+
+static void __spi_replace_transfers_release(struct spi_master *master,
+					    struct spi_message *msg,
+					    void *res)
+{
+	struct spi_res_replaced_transfers *srt = res;
+	int i;
+
+	/* call extra callback */
+	if (srt->release)
+		srt->release(master, msg, res);
+
+	/* insert transfers back into the message ahead of xfers[0] */
+	list_splice(&srt->replaced_transfers, &srt->xfers[0].transfer_list);
+
+	/* remove the formerly inserted entries */
+	for (i = 0; i < srt->inserted; i++)
+		list_del(&srt->xfers[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: the spi_transfer we want to replace
+ * @remove: number of transfers to remove
+ * @insert: the number of transfers we want to insert
+ * @release: extra release code necessary in some circumstances
+ *
+ * Returns: pointer to array of newly inserted transfers,
+ *          NULL in case of errors
+ */
+struct spi_transfer *spi_replace_transfers(struct spi_message *msg,
+					   struct spi_transfer *xfer,
+					   int remove, int insert,
+					   spi_res_release_t release)
+{
+	int i;
+	size_t size = insert * sizeof(struct spi_transfer)
+		      + sizeof(struct spi_res_replaced_transfers);
+	struct spi_res_replaced_transfers *srt;
+	struct spi_transfer *next;
+
+	if (unlikely(insert < 1))
+		return NULL;
+
+	/* allocate the structure */
+	srt = spi_res_alloc(msg->spi, __spi_replace_transfers_release,
+			    size, 0);
+	if (unlikely(!srt))
+		return NULL;
+
+	/* the release code to use before running the generic release */
+	srt->release = release;
+
+	/* create copy of the given xfer with identical settings */
+	srt->inserted = insert;
+	for (i = 0; i < insert ; i++) {
+		/* copy all settings */
+		memcpy(&srt->xfers[i], xfer, sizeof(*xfer));
+
+		/* for anything but the last transfer, clear some settings */
+		if (i < insert - 1) {
+			srt->xfers[i].cs_change = 0;
+			srt->xfers[i].delay_usecs = 0;
+		}
+
+		/* add before the xfer to remove itself */
+		list_add_tail(&srt->xfers[i].transfer_list,
+			      &xfer->transfer_list);
+	}
+
+	/* remove the requested number of transfers */
+	for (i = 0; i < remove; i++, xfer = next) {
+		/* check for error case - we want to remove list_head...*/
+		if (&xfer->transfer_list == &msg->transfers) {
+			dev_err(&msg->spi->dev,
+				"requested to remove more spi_transfers than are available\n");
+			spi_res_free(srt);
+			return NULL;
+		}
+		/* get the next xfer for later */
+		next = list_next_entry(xfer, transfer_list);
+
+		/* and remove the current transfer from the list of transfers */
+		list_del_init(&xfer->transfer_list);
+
+		/* and add it to the list in spi_res_replaced_transfers */
+		INIT_LIST_HEAD(&srt->replaced_transfers);
+		list_add(&xfer->transfer_list, &srt->replaced_transfers);
+	}
+
+	/* and register it */
+	spi_res_add(msg, srt);
+
+	/* return the head of the list */
+	return &srt->xfers[0];
+}
+EXPORT_SYMBOL_GPL(spi_replace_transfers);
+
+/* core spi_transfer transformation */
+
+static void __spi_split_transfers_fixup_transfer_addr_and_len(
+	struct spi_transfer *xfer, int count, int shift)
+{
+	int i;
+
+	/* fix up transfer length */
+	xfer[0].len = shift;
+
+	/* shift all the addresses arround */
+	for (i = 1; i < count; i++) {
+		xfer[i].len -= shift;
+		xfer[i].tx_buf += xfer[i].tx_buf ? shift : 0;
+		xfer[i].rx_buf += xfer[i].rx_buf ? shift : 0;
+		xfer[i].tx_dma += xfer[i].tx_dma ? shift : 0;
+		xfer[i].rx_dma += xfer[i].rx_dma ? shift : 0;
+	}
+}
+
+static int __spi_split_transfers_first_page_len_not_aligned(
+	struct spi_master *master,
+	struct spi_message *message,
+	struct spi_transfer *xfer,
+	size_t alignment_mask)
+{
+	int count;
+	struct spi_transfer *xfers;
+	const char *tx_start, *rx_start; /* the rx/tx_buf address */
+	const char *tx_end, *rx_end; /* the last byte of the transfer */
+	size_t tx_start_page, rx_start_page; /* the "page address" for start */
+	size_t tx_end_page, rx_end_page; /* the "page address" for end */
+	size_t tx_start_align, rx_start_align; /* alignment of buf address */
+
+	/* calculate the necessary values */
+	tx_start = xfer->tx_buf;
+	tx_start_page = (size_t)tx_start & (PAGE_SIZE - 1);
+	tx_start_align = ((size_t)tx_start & alignment_mask);
+	tx_end = tx_start ? &tx_start[xfer->len - 1] : NULL;
+	tx_end_page = (size_t)tx_end & (PAGE_SIZE - 1);
+
+	rx_start = xfer->rx_buf;
+	rx_start_page = (size_t)rx_start & (PAGE_SIZE - 1);
+	rx_start_align = ((size_t)rx_start & alignment_mask);
+	rx_end = rx_start ? &tx_start[xfer->len - 1] : NULL;
+	rx_end_page = (size_t)rx_end & (PAGE_SIZE - 1);
+
+	/* if we do not cross a page for either rx or tx,
+	 * then there is nothing to do...
+	 */
+	if ((tx_start_page == tx_end_page) &&
+	    (rx_start_page == rx_end_page))
+		return 0;
+
+	/* calculate how many transfers we need to replace the current */
+	count = 1;
+	if (rx_start_align)
+		count++;
+	if ((tx_start_align) &&
+	    (tx_start_align != rx_start_align) &&
+	    (tx_start != rx_start))
+		count++;
+
+	/* send a one-time warning */
+	dev_warn_once(&message->spi->dev,
+		      "unaligned spi_transfers produced by spi_device driver - please fix driver\n");
+
+	/* create replacement */
+	xfers = spi_replace_transfers(message, xfer, 1, count, NULL);
+	if (!xfers)
+		return -ENOMEM;
+
+	/* now we fix up the transfer pointer and transfer len */
+	if (count == 2) {
+		__spi_split_transfers_fixup_transfer_addr_and_len(
+			xfers, 2, max(tx_start_align, rx_start_align));
+	} else {
+		if (tx_start_align < rx_start_align) {
+			__spi_split_transfers_fixup_transfer_addr_and_len(
+				&xfers[0], 3,
+				tx_start_align);
+			__spi_split_transfers_fixup_transfer_addr_and_len(
+				&xfers[1], 2,
+				rx_start_align - tx_start_align);
+		} else {
+			__spi_split_transfers_fixup_transfer_addr_and_len(
+				&xfers[0], 3,
+				rx_start_align);
+			__spi_split_transfers_fixup_transfer_addr_and_len(
+				&xfers[1], 2,
+				tx_start_align - rx_start_align);
+		}
+	}
+
+	/* increment statistics counters */
+	SPI_STATISTICS_INCREMENT_FIELD(&master->statistics,
+				       transfers_split_unaligned);
+	SPI_STATISTICS_INCREMENT_FIELD(&message->spi->statistics,
+				       transfers_split_unaligned);
+
+	return 0;
+}
+
+/**
+ * spi_split_tranfers_first_page_len_not_aligned - split spi transfers into
+ *                                                 two transfers on the
+ *                                                 first page boundary, when
+ *                                                 the start address is not
+ *                                                 aligned
+ * @master:       the @spi_master for this transfer
+ * @message:      the @spi_message to transform
+ * @when_can_dma: true if we only should execute when we can use dma
+ * @alignment:    the requested alignment
+ * @min_size:     the minimum transfer when to apply this
+ *
+ * Return: status of transformation
+ *
+ * This implements one of the ways that (dma-enabled) HW may be constrained
+ * in this case the HW is able to do unaligned transfers, but a DMA-transfer
+ * is always of size align from a register perspective (even if it only
+ * writes < align bytes back to ram).
+ * As there is no guarantee that the next logical page is actually adjectant
+ * for the dma perspective, we need to split the first page of a transfer
+ * into separate transfers (1 or 2 depending on alignment of rx_buf and
+ * tx_buf respectively).
+ * this does not make sure that the individual transfers that are
+ * added are address, aligned, but it makes sure that each of those
+ * new transfers.len is < alignment.
+ *
+ */
+int spi_split_transfers_first_page_len_not_aligned(
+	struct spi_master *master,
+	struct spi_message *message,
+	bool when_can_dma,
+	size_t alignment)
+{
+	struct spi_device *spi = message->spi;
+	struct spi_transfer *_xfer, *xfer = NULL;
+	size_t alignment_mask = alignment - 1;
+	int ret;
+
+	/* if when_can_dma is set, but can_dma is unset,
+	 * something major is wrong...
+	 */
+	if (when_can_dma && (!master->can_dma)) {
+		dev_err(&master->dev,
+			"configured without can_dma, but requests can_dma to be set calling first_page_len_not_aligned\n");
+		return -EINVAL;
+	}
+
+	/* iterate over the transfer_list in a safe manner
+	 * there is a posibility of replacement and we need to make sure
+	 * we run over all the entries
+	 */
+	xfer = list_prepare_entry(xfer, &message->transfers, transfer_list);
+	list_for_each_entry_safe_continue(xfer, _xfer,
+					  &message->transfers,
+					  transfer_list) {
+		if (when_can_dma && (!master->can_dma(master, spi, xfer)))
+			continue;
+		if (((size_t)xfer->rx_buf & alignment_mask) ||
+		    ((size_t)xfer->tx_buf & alignment_mask)) {
+			ret = __spi_split_transfers_first_page_len_not_aligned(
+				master, message, xfer, alignment_mask);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_split_transfers_first_page_len_not_aligned);
+
+int __spi_split_transfer_maxsize(struct spi_master *master,
+				 struct spi_message *msg,
+				 struct spi_transfer *xfer,
+				 size_t maxsize)
+{
+	int count = DIV_ROUND_UP(xfer->len, maxsize);
+	size_t offset = 0;
+	int i;
+	struct spi_transfer *xfers;
+
+	/* create replacement */
+	xfers = spi_replace_transfers(msg, xfer, 1, count, NULL);
+	if (!xfers)
+		return -ENOMEM;
+
+	/* now handle each of those newly inserted xfers */
+	for (i = 0; i < count; i++) {
+		/* the first transfer just needs the length modified,
+		 * so do not change the pointers
+		 */
+		if (i) {
+			/* update rx_buf if it is not tx_buf */
+			if (xfers[i].rx_buf &&
+			    (xfers[i].tx_buf != xfers[i].rx_buf)) {
+				/* modify the rx_buf */
+				xfers[i].rx_buf += offset;
+				/* this is actually not used with any
+				 * scatter_lists, but it is still there,
+				 * so we have to support it
+				 */
+				if (xfers[i].rx_dma)
+					xfers[i].rx_dma += offset;
+			}
+			/* update tx_buf */
+			if (xfers[i].tx_buf) {
+				/* modify the rx_buf */
+				xfers[i].tx_buf += offset;
+				/* this is actually not used with any
+				 * scatter_lists, but it is still there,
+				 * so we have to support it...
+				 */
+				if (xfers[i].tx_dma)
+					xfers[i].tx_dma += offset;
+			}
+		}
+		/* modify length */
+		if (i < count - 1) /* for all but the last transfer */
+			xfers[i].len = maxsize;
+		else /* the last one is most likley not max_size */
+			xfers[i].len -= offset;
+
+		/* increment offset */
+		offset += maxsize;
+	}
+
+	/* 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, *xfer = NULL;
+	int ret;
+
+	/* iterate over the transfer_list in a safe manner
+	 * there is a posibility of replacement and we need to make sure
+	 * we run over all the entries
+	 */
+	xfer = list_prepare_entry(xfer, &msg->transfers, transfer_list);
+	list_for_each_entry_safe_continue(xfer, _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);
 
 /*-------------------------------------------------------------------------*/
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 7e74e0e..8075c93 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -53,6 +53,11 @@ extern struct bus_type spi_bus_type;
  *
  * @transfer_bytes_histo:
  *                 transfer bytes histogramm
+ *
+ * @transfers_split_maxsize:
+ *                 number of transfers split because of len exceeds maxsize
+ * @transfers_split_unaligned:
+ *                 number of transfers split because of unaligned transfers
  */
 struct spi_statistics {
 	spinlock_t		lock; /* lock for the whole structure */
@@ -72,6 +77,10 @@ struct spi_statistics {
 
 #define SPI_STATISTICS_HISTO_SIZE 17
 	unsigned long transfer_bytes_histo[SPI_STATISTICS_HISTO_SIZE];
+
+	unsigned long           transfers_split_maxsize;
+	unsigned long           transfers_split_unaligned;
+
 };
 
 void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
@@ -607,6 +616,24 @@ extern void spi_res_free(void *res);
 extern void spi_res_release(struct spi_master *master,
 			    struct spi_message *message);
 
+/* SPI transfer modifications using spi_res */
+
+extern struct spi_transfer *spi_replace_transfers(
+	struct spi_message *msg,
+	struct spi_transfer *xfer,
+	int remove, int insert,
+	spi_res_release_t release);
+
+extern int spi_split_transfers_first_page_len_not_aligned(
+	struct spi_master *master,
+	struct spi_message *message,
+	bool when_can_dma,
+	size_t alignment);
+
+extern int spi_split_transfers_maxsize(struct spi_master *master,
+				       struct spi_message *msg,
+				       size_t maxsize);
+
 /*---------------------------------------------------------------------------*/
 
 /*
-- 
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 3/3] spi: bcm2835: moved to the spi_transfer transformation to avoid HW restrictions

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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

Moved to the new spi_transfer transformation methods to work arround
the HW restrictions in place arround DMA and alignment or max_length.

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

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index cf04960..67516f7 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -365,38 +365,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))
-		return true;
-
-	/* otherwise we only allow transfers within the same page
-	 * to avoid wasting time on dma_mapping when it is not practical
-	 */
-	if (((size_t)tfr->tx_buf & (PAGE_SIZE - 1)) + tfr->len > PAGE_SIZE) {
-		dev_warn_once(&spi->dev,
-			      "Unaligned spi tx-transfer bridging page\n");
-		return false;
-	}
-	if (((size_t)tfr->rx_buf & (PAGE_SIZE - 1)) + tfr->len > PAGE_SIZE) {
-		dev_warn_once(&spi->dev,
-			      "Unaligned spi rx-transfer bridging page\n");
-		return false;
-	}
-
 	/* return OK */
 	return true;
 }
@@ -598,8 +566,28 @@ static int bcm2835_spi_prepare_message(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+	int ret;
+
+	/* apply some spi_transfer transformations */
+
+	/* align transfers on words length - avoiding
+	 * that the scatter/gather list produces transfers
+	 * that are not a multiple of 4
+	 */
+	ret = spi_split_transfers_first_page_len_not_aligned(
+		master, msg, true, 4);
+	if (ret)
+		return ret;
+
+	/* limit the max transfer size to 15 * PAGE_SIZE
+	 * the bcm2835 HW limit is actually 65535, but to avoid
+	 * another set of alignment issues we limit ourselves
+	 */
+	ret = spi_split_transfers_maxsize(master, msg, 15 * PAGE_SIZE);
+	if (ret)
+		return ret;

+	/* setting up clock and data polarity prior to asserting GPIO-cs */
 	cs &= ~(BCM2835_SPI_CS_CPOL | BCM2835_SPI_CS_CPHA);

 	if (spi->mode & SPI_CPOL)
--
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 1/3] spi: added spi_resource management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Mon, Nov 30, 2015 at 01:04:52PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:

> +static struct spi_res *__spi_res_alloc(struct spi_device *spi,
> +				       spi_res_release_t release,
> +				       size_t size,
> +				       gfp_t gfp)

This has exactly one (tiny) user.  Why is it a separate function?

> +	sres = kzalloc(tot_size, gfp);
> +	if (unlikely(!sres))
> +		return NULL;

Don't use likely() or unlikely() annotations unless the code is *really*
performance critical, just let the optimiser get on with things.  The
annotations most likely cost more time in reading the code than they'll
ever save.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: add initial set of spi_transfer transformation methods

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> added the following spi_transformation methods:
> * spi_split_transfers_first_page_len_not_aligned -
>   this splits spi_transfers that are not aligned on rx_buf/tx_buf
>   this will create 2 or 3 transfers, where the first 1/2 transfers are
>   just there to allow the last transfer to be fully aligned. These first
>   transfers will have a length less than alignment.
> * spi_split_transfers_maxsize
>   this splits the spi_transfer into multiple independent spi_transfers
>   all of which will be of size max_size or smaller.

Please split this up for review, as I'm fairly sure has been discussed
before smaller, more focused patches with clear changelogs describing
what they are suppose to do are much easier to review.  Start by adding
the framework changes, then add one transformation at once.  This is a
very big patch full of fiddly, unexplained code.

> To start these shall get used by the individual drivers in prepare_message,
> but some may get moved into spi-core with the correct parametrization in
> spi_master.

As discussed I'm not a big fan of this approach.

> +/* the spi_resource structure used */
> +struct spi_res_replaced_transfers {
> +	spi_res_release_t release;
> +	struct list_head replaced_transfers;
> +	int inserted;
> +	struct spi_transfer xfers[];
> +};

This quite clearly isn't a struct spi_resource, nor does it contain
one...

> +static void __spi_split_transfers_fixup_transfer_addr_and_len(
> +	struct spi_transfer *xfer, int count, int shift)
> +{
> +	int i;
> +
> +	/* fix up transfer length */
> +	xfer[0].len = shift;
> +
> +	/* shift all the addresses arround */
> +	for (i = 1; i < count; i++) {
> +		xfer[i].len -= shift;
> +		xfer[i].tx_buf += xfer[i].tx_buf ? shift : 0;
> +		xfer[i].rx_buf += xfer[i].rx_buf ? shift : 0;
> +		xfer[i].tx_dma += xfer[i].tx_dma ? shift : 0;
> +		xfer[i].rx_dma += xfer[i].rx_dma ? shift : 0;
> +	}
> +}

It's really not clear what this is supposed to do.  Among other things
"shift" appears to be misnamed and anything using tx_dma and rx_dma is
worrying.

I did read a bit further but like I said at the top this is a lot of
complicated code that's not split up enough to be clear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: add initial set of spi_transfer transformation methods

[Martin Sperl]

> On 01.12.2015, at 22:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> 
>> added the following spi_transformation methods:
>> * spi_split_transfers_first_page_len_not_aligned -
>>  this splits spi_transfers that are not aligned on rx_buf/tx_buf
>>  this will create 2 or 3 transfers, where the first 1/2 transfers are
>>  just there to allow the last transfer to be fully aligned. These first
>>  transfers will have a length less than alignment.
>> * spi_split_transfers_maxsize
>>  this splits the spi_transfer into multiple independent spi_transfers
>>  all of which will be of size max_size or smaller.
> 
> Please split this up for review, as I'm fairly sure has been discussed
> before smaller, more focused patches with clear changelogs describing
> what they are suppose to do are much easier to review.  Start by adding
> the framework changes, then add one transformation at once.  This is a
> very big patch full of fiddly, unexplained code.

I can split this in two or 3, as I already have a prototype that allows for
merging multiple transfers into one transfer by copying data arround.

> 
>> To start these shall get used by the individual drivers in prepare_message,
>> but some may get moved into spi-core with the correct parametrization in
>> spi_master.
> 
> As discussed I'm not a big fan of this approach.
It is just a first step.

As far as I can see different hw may require different policies.

If there is a common code to handle all the variations then we can
Implement that, but I have no idea what that could be.

That is why I took this prepare message approach for now as it
allows most control from the driver perspective (and is not 
breaking existing drivers)

As said: this can get moved to the core when we have found the
correct parametrization for driver.
Parameters that come to mind are:
* dma_alignment
* max_transfersize (for which there exists a patch already)

> 
>> +/* the spi_resource structure used */
>> +struct spi_res_replaced_transfers {
>> +    spi_res_release_t release;
>> +    struct list_head replaced_transfers;
>> +    int inserted;
>> +    struct spi_transfer xfers[];
>> +};
> 
> This quite clearly isn't a struct spi_resource, nor does it contain
> one...
It is done the same way as devm_kalloc uses struct devres:
the magic devres struct is hidden by the devres framework. 
Only the payload data pointer is returned, which is what this
structure describes.

You want it as a more explicit member instead?

> 
>> +static void __spi_split_transfers_fixup_transfer_addr_and_len(
>> +    struct spi_transfer *xfer, int count, int shift)
>> +{
>> +    int i;
>> +
>> +    /* fix up transfer length */
>> +    xfer[0].len = shift;
>> +
>> +    /* shift all the addresses arround */
>> +    for (i = 1; i < count; i++) {
>> +        xfer[i].len -= shift;
>> +        xfer[i].tx_buf += xfer[i].tx_buf ? shift : 0;
>> +        xfer[i].rx_buf += xfer[i].rx_buf ? shift : 0;
>> +        xfer[i].tx_dma += xfer[i].tx_dma ? shift : 0;
>> +        xfer[i].rx_dma += xfer[i].rx_dma ? shift : 0;
>> +    }
>> +}
> 
> It's really not clear what this is supposed to do.  Among other things
> "shift" appears to be misnamed and anything using tx_dma and rx_dma is
> worrying.
Essentially this will handle count spi-transfers that have all identical data
(Except for transfer_list) initially - especially Len and tx/rx buf.
The first of those transfers is set its length to shift 
(no need to modify rx/tx_buf)
The others are reduced by that length and the pointers are shifted up,
So that they are now starting at the address after the first transfer and
Transfer the correct number of bytes.

Rx/tx_dma is handled because it is there and it's address needs also to be
accurately Shifted by that offset. I know that it is a depreciated interface,
but as it is still there it needs to be supported...


--
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 1/3] spi: added spi_resource management

[Martin Sperl]

> On 01.12.2015, at 22:04, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> On Mon, Nov 30, 2015 at 01:04:52PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>> 
>> +static struct spi_res *__spi_res_alloc(struct spi_device *spi,
>> +                       spi_res_release_t release,
>> +                       size_t size,
>> +                       gfp_t gfp)
> 
> This has exactly one (tiny) user.  Why is it a separate function?
Readability, mimicking devres code and the option of adding
object caching/reuse here later...
There is a much higher likelihood that spi_resources will be
allocated and then freed several times per second, so this
can save cpu cycles and avoid locks...

> 
>> +    sres = kzalloc(tot_size, gfp);
>> +    if (unlikely(!sres))
>> +        return NULL;
> 
> Don't use likely() or unlikely() annotations unless the code is *really*
> performance critical, just let the optimiser get on with things.  The
> annotations most likely cost more time in reading the code than they'll
> ever save.
Same code structure was used with devres, so I copied it.
--
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 1/3] spi: added spi_resource management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Wed, Dec 02, 2015 at 08:30:51AM +0100, Martin Sperl wrote:

> > This has exactly one (tiny) user.  Why is it a separate function?

> Readability, mimicking devres code and the option of adding
> object caching/reuse here later...

This is *not* helping readability.

> There is a much higher likelihood that spi_resources will be
> allocated and then freed several times per second, so this
> can save cpu cycles and avoid locks...

In what way does splitting this over two functions helping improve
performance?  If there were multiple users or something perhaps but
there don't appear to be.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] spi: added spi_resource management

[Martin Sperl]

On 02.12.2015 13:29, Mark Brown wrote:
> On Wed, Dec 02, 2015 at 08:30:51AM +0100, Martin Sperl wrote:
>
>>> This has exactly one (tiny) user.  Why is it a separate function?
>
>> Readability, mimicking devres code and the option of adding
>> object caching/reuse here later...
>
> This is *not* helping readability.
Originally it contained a bit more code to avoid repeated allocations,
which was cut out for the initial version of the patch.

>> There is a much higher likelihood that spi_resources will be
>> allocated and then freed several times per second, so this
>> can save cpu cycles and avoid locks...
>
> In what way does splitting this over two functions helping improve
> performance?  If there were multiple users or something perhaps but
> there don't appear to be.
>

It does not help performance per se, but it allows adding more
code in that location that would allow to reuse allocated objects.

As of now I will merge those 2 functions.

The bigger question (based on your comments to Patch 2/3) is:

Do you want to follow the devres approach (i.e: hiding
"struct spi_res" after allocation and returning "void *"
to the data-payload only in spi_res_alloc)?

Or do you prefer to have "struct spi_res" as an explicit member of
a structure (i.e. in Patch 2/3 "struct spi_res_replaced_transfers")?

I want to get this settled before submitting newer versions of the
other patches that address other concerns, because any changes to
spi_res design directly impact these other patches.

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 1/3] spi: added spi_resource management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Wed, Dec 02, 2015 at 02:04:24PM +0100, Martin Sperl wrote:

> The bigger question (based on your comments to Patch 2/3) is:

I haven't even looked at your reply ot that yet.

> Do you want to follow the devres approach (i.e: hiding
> "struct spi_res" after allocation and returning "void *"
> to the data-payload only in spi_res_alloc)?

> Or do you prefer to have "struct spi_res" as an explicit member of
> a structure (i.e. in Patch 2/3 "struct spi_res_replaced_transfers")?

I wasn't aware that was an issue?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] spi: added spi_resource management

[Martin Sperl]

> On 02.12.2015, at 14:32, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Wed, Dec 02, 2015 at 02:04:24PM +0100, Martin Sperl wrote:
> 
>> The bigger question (based on your comments to Patch 2/3) is:
> 
> I haven't even looked at your reply ot that yet.
> 
>> Do you want to follow the devres approach (i.e: hiding
>> "struct spi_res" after allocation and returning "void *"
>> to the data-payload only in spi_res_alloc)?
> 
>> Or do you prefer to have "struct spi_res" as an explicit member of
>> a structure (i.e. in Patch 2/3 "struct spi_res_replaced_transfers")?
> 
> I wasn't aware that was an issue?

Here the section from your reply to the other patch I am referring to:
> On 01.12.2015, at 22:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 

>> 
>> +/* the spi_resource structure used */
>> +struct spi_res_replaced_transfers {
>> +	spi_res_release_t release;
>> +	struct list_head replaced_transfers;
>> +	int inserted;
>> +	struct spi_transfer xfers[];
>> +};
> 
> This quite clearly isn't a struct spi_resource, nor does it contain
> one…

As said: it could also get written as:
+/* the spi_resource structure used */
+struct spi_res_replaced_transfers {
+	struct spi_res resource;
+	spi_res_release_t release;
+	struct list_head replaced_transfers;
+	int inserted;
+	struct spi_transfer xfers[];
+};

But we loose the ability to just allocate memory the way we use
devm_kmalloc.

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 2/3] spi: add initial set of spi_transfer transformation methods

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Wed, Dec 02, 2015 at 08:25:08AM +0100, Martin Sperl wrote:
> > On 01.12.2015, at 22:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:

Please use normal formatting for e-mails with blank lines between
paragraphs, it makes things much easier to read.

> >> +/* the spi_resource structure used */
> >> +struct spi_res_replaced_transfers {
> >> +    spi_res_release_t release;
> >> +    struct list_head replaced_transfers;
> >> +    int inserted;
> >> +    struct spi_transfer xfers[];
> >> +};

> > This quite clearly isn't a struct spi_resource, nor does it contain
> > one...

> It is done the same way as devm_kalloc uses struct devres:
> the magic devres struct is hidden by the devres framework. 
> Only the payload data pointer is returned, which is what this
> structure describes.

> You want it as a more explicit member instead?

If it's not actually a resource and is really just a structure that
happens to be managed with a resource then describe it as such, I'm
complaining about the confusing naming here.

> >> +static void __spi_split_transfers_fixup_transfer_addr_and_len(
> >> +    struct spi_transfer *xfer, int count, int shift)
> >> +{
> >> +    int i;

> >> +    /* fix up transfer length */
> >> +    xfer[0].len = shift;

> > It's really not clear what this is supposed to do.  Among other things
> > "shift" appears to be misnamed and anything using tx_dma and rx_dma is
> > worrying.

> Essentially this will handle count spi-transfers that have all identical data
> (Except for transfer_list) initially - especially Len and tx/rx buf.
> The first of those transfers is set its length to shift 
> (no need to modify rx/tx_buf)
> The others are reduced by that length and the pointers are shifted up,
> So that they are now starting at the address after the first transfer and
> Transfer the correct number of bytes.

I'm sorry but the above is still very opaque - why would we have some
list of transfers that all have identical data?  I *think* the intention
here is to do some realignment and that shift is really an offset to
move things by?

> Rx/tx_dma is handled because it is there and it's address needs also to be
> accurately Shifted by that offset. I know that it is a depreciated interface,
> but as it is still there it needs to be supported...

It's really quite broken already for most users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: add initial set of spi_transfer transformation methods

[Martin Sperl]

> On 03.12.2015, at 01:36, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Wed, Dec 02, 2015 at 08:25:08AM +0100, Martin Sperl wrote:
>>>> On 01.12.2015, at 22:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>>> On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> 
> Please use normal formatting for e-mails with blank lines between
> paragraphs, it makes things much easier to read.
> 
>>>> +/* the spi_resource structure used */
>>>> +struct spi_res_replaced_transfers {
>>>> +    spi_res_release_t release;
>>>> +    struct list_head replaced_transfers;
>>>> +    int inserted;
>>>> +    struct spi_transfer xfers[];
>>>> +};
> 
>>> This quite clearly isn't a struct spi_resource, nor does it contain
>>> one...
> 
>> It is done the same way as devm_kalloc uses struct devres:
>> the magic devres struct is hidden by the devres framework. 
>> Only the payload data pointer is returned, which is what this
>> structure describes.
> 
>> You want it as a more explicit member instead?
> 
> If it's not actually a resource and is really just a structure that
> happens to be managed with a resource then describe it as such, I'm
> complaining about the confusing naming here.

Ok understood now - I feared you meant you disliked the whole approach. 

> 
>>>> +static void __spi_split_transfers_fixup_transfer_addr_and_len(
>>>> +    struct spi_transfer *xfer, int count, int shift)
>>>> +{
>>>> +    int i;
> 
>>>> +    /* fix up transfer length */
>>>> +    xfer[0].len = shift;
> 
>>> It's really not clear what this is supposed to do.  Among other things
>>> "shift" appears to be misnamed and anything using tx_dma and rx_dma is
>>> worrying.
> 
>> Essentially this will handle count spi-transfers that have all identical data
>> (Except for transfer_list) initially - especially Len and tx/rx buf.
>> The first of those transfers is set its length to shift 
>> (no need to modify rx/tx_buf)
>> The others are reduced by that length and the pointers are shifted up,
>> So that they are now starting at the address after the first transfer and
>> Transfer the correct number of bytes.
> 
> I'm sorry but the above is still very opaque - why would we have some
> list of transfers that all have identical data?  I *think* the intention
> here is to do some realignment and that shift is really an offset to
> move things by?

Yes, so probably the naming is wrong.
As for identical data: the spi_transfer_replace method also copies the
first transfer to to be replaced to all the newly created transfers
Before replacing/splicing them into spi_message.transfers.

Anyway: there is something probably not working for the alignment
code when rx/tx_buf are mis-aligned by different offsets (say 1 and 2)
even though I ran all those extensive transfer tests on bcm2835 and
they run fine there.

> 
>> Rx/tx_dma is handled because it is there and it's address needs also to be
>> accurately Shifted by that offset. I know that it is a depreciated interface,
>> but as it is still there it needs to be supported...
> 
> It's really quite broken already for most users.
If it is broken, then why not remove those members?
Or set those pointers to null in spi_verify (or at least croak about it
being depreciated)

With the current situation the code needs to handle those transfers
correctly - especially if we want to enable it by default in spi-core
for all spi-masters eventually.

One last thing:
Where do you prefer to run those transfer transformations?
In the master-loop (before/after prepare message)?
During spi_verify (or after it)?

The advantage of spi-verify would be that we potentially run those
transformations in a different thread than message-pump allowing
to make better use of multiple CPUs. (This assumes that spi-async
is used or multiple drivers submit to spi_master concurrently,
so that queueing occurs and the message-pump thread is woken).

Actually the same argument of potential concurrency would also 
support the move of scatter/gather list generation into the same
place (which is essentially just another transformation)

I will rewrite those patches separating them out a bit more
and resubmitting them still based on the dev-res approach,
But renaming those structures.--
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 2/3] spi: add initial set of spi_transfer transformation methods

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On Thu, Dec 03, 2015 at 07:22:10AM +0100, Martin Sperl wrote:
> > On 03.12.2015, at 01:36, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> >> Rx/tx_dma is handled because it is there and it's address needs also to be
> >> accurately Shifted by that offset. I know that it is a depreciated interface,
> >> but as it is still there it needs to be supported...

> > It's really quite broken already for most users.

> If it is broken, then why not remove those members?

There are still a couple of users and someone needs to go through and
fix them.  I don't know which if any systems they run on, it's possible
that they manage to work as a result of the DMA mappings not getting in
the way the systems they're being used on (if they're being used at all)
or the controller drivers they're used with being equally aged.

> One last thing:
> Where do you prefer to run those transfer transformations?
> In the master-loop (before/after prepare message)?
> During spi_verify (or after it)?

Remember that we can't do any allocations in _verify() as it can run in
atomic context so while we want to check if we can take the message then
we are most likely going to be unable to implement transformations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] spi: add initial set of spi_transfer transformation methods

[Martin Sperl]

> On 03.12.2015, at 15:34, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Thu, Dec 03, 2015 at 07:22:10AM +0100, Martin Sperl wrote:
>>> On 03.12.2015, at 01:36, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>>>> Rx/tx_dma is handled because it is there and it's address needs also to be
>>>> accurately Shifted by that offset. I know that it is a depreciated interface,
>>>> but as it is still there it needs to be supported...
> 
>>> It's really quite broken already for most users.
> 
>> If it is broken, then why not remove those members?
> 
> There are still a couple of users and someone needs to go through and
> fix them.  I don't know which if any systems they run on, it's possible
> that they manage to work as a result of the DMA mappings not getting in
> the way the systems they're being used on (if they're being used at all)
> or the controller drivers they're used with being equally aged.

So if we make use of this “optimization” globally we will need to support it,
otherwise we break it.

That was one of the reasons why I am advocating (for now) that we
make it an option that has to get enabled in the individual drivers.

But we could minimize the coding by providing a “default” implementation
that could get assigned to spi_master.prepare_message.

So an spi_master would only need to assign that, which makes it clear that
this has been tested with this specific HW (which is my biggest concern).

We could also create a separate spi_master.transform_message method, which
the drivers assign to the default or custom implementation and which
(eventually) may get assigned a default if NULL and dma_alignment
as well as (the future) max_transfer_size is set.

The advantage of this would be that we could (at a later stage) move the
invocation translation method to a different location - say: queuing it
in a different worker thread for translation when there are more than one
spi_message in the queue (if it is submitted from idle we would execute in
place at somewhere in __spi_pump_messages).

Just exploring some options...

Would it make any sense to separate it out as described, so that we can do
something like this later?

>> One last thing:
>> Where do you prefer to run those transfer transformations?
>> In the master-loop (before/after prepare message)?
>> During spi_verify (or after it)?
> 
> Remember that we can't do any allocations in _verify() as it can run in
> atomic context so while we want to check if we can take the message then
> we are most likely going to be unable to implement transformations.
I came to realize that in the meantime as well - it was just an idea
how we could defer such things to a second CPU...--
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 2/3] spi: add initial set of spi_transfer transformation methods

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On Thu, Dec 03, 2015 at 04:33:52PM +0100, Martin Sperl wrote:
> > On 03.12.2015, at 15:34, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > There are still a couple of users and someone needs to go through and
> > fix them.  I don't know which if any systems they run on, it's possible
> > that they manage to work as a result of the DMA mappings not getting in
> > the way the systems they're being used on (if they're being used at all)
> > or the controller drivers they're used with being equally aged.

> So if we make use of this “optimization” globally we will need to support it,
> otherwise we break it.

I'm relatively OK with breaking these users TBH, I'm not convinced any
of them are actually being run and it's probably easier to fix by fixing
the users when they're engaged.  Also if we do this in the message queue
thread then we're OK anyway since anything that uses these will old
enough to be using a custom message queue anyway so the risk is less
than it might appear.

> > Remember that we can't do any allocations in _verify() as it can run in
> > atomic context so while we want to check if we can take the message then
> > we are most likely going to be unable to implement transformations.

> I came to realize that in the meantime as well - it was just an idea
> how we could defer such things to a second CPU...

We could potentially try doing this in both call sites - we already have
special casing for spi_sync().  It's more fiddly but it could be done as
a future step.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]