[PATCH 1/6] spi: add flow controll support

[PATCH 1/6] spi: add flow controll support

[Oleksij Rempel]

Different HW implement different variants of SPI based flow control (FC).
To flexible FC implementation a spited it to fallowing common parts:
Flow control: Request Sequence
Master CS   |-------2\_____________________|
Slave  FC   |-----1\_______________________|
DATA        |-----------3\_________________|

Flow control: Ready Sequence
Master CS   |-----1\_______________________|
Slave  FC   |--------2\____________________|
DATA        |-----------3\_________________|

Flow control: ACK End of Data
Master CS   |______________________/2------|
Slave  FC   |________________________/3----|
DATA        |__________________/1----------|

Flow control: Pause
Master CS   |_______________________/------|
Slave  FC   |_______1/-----\3______/-------|
DATA        |________2/------\4___/--------|

Flow control: Ready signal on MISO
Master CS   |-----1\_______________________|
MISO/DATA   |------2\____3/----------------|
CONV START  |       ^                      |
DATA READY  |             ^                |

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |   7 +
 drivers/spi/spi.c                                 | 191 +++++++++++++++++++++-
 include/linux/spi/spi.h                           |  68 +++++++-
 3 files changed, 261 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index bbaa857..dfefc86 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -61,6 +61,13 @@ contain the following properties.
                       used for MOSI. Defaults to 1 if not present.
 - spi-rx-bus-width - (optional) The bus width(number of data wires) that
                       used for MISO. Defaults to 1 if not present.
+- spi-fc-ready      - (optional) flow control line used for ready signal.
+- spi-fc-stop-ack   - (optional) flow control line used to ACK end of transfer.
+- spi-fc-pause      - (optional) flow control line used for to pause transfer
+                      at any time.
+- spi-fc-request    - (optional) flow control line used for request signal.
+- spi-fc-miso-ready - (optional) MISO used for flow control ready signal.
+- fc-gpio           - (optional) gpio for flow control.
 
 Some SPI controllers and devices support Dual and Quad SPI transfer mode.
 It allows data in the SPI system to be transferred in 2 wires(DUAL) or 4 wires(QUAD).
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 47eff80..1140665 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -37,6 +37,8 @@
 #include <linux/kthread.h>
 #include <linux/ioport.h>
 #include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/spi.h>
@@ -396,6 +398,156 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv)
 EXPORT_SYMBOL_GPL(__spi_register_driver);
 
 /*-------------------------------------------------------------------------*/
+/*
+ * SPI flow control
+ */
+static int spi_fc_wait_miso(struct spi_device *spi)
+{
+	struct spi_master *master = spi->master;
+	int count = 100; /* 10 * 100 = 10 msec */
+
+	if (!master->get_miso_level)
+		return count;
+
+	while (count > 0) {
+		count--;
+		if (master->get_miso_level(spi))
+			break;
+
+		usleep_range(100, 200);
+	}
+
+	return count;
+}
+
+
+static int spi_fc_equal_cs(struct spi_device *spi)
+{
+	int fc_enabled;
+
+	fc_enabled = gpiod_get_value(spi->fc_gpio);
+	return (spi->cs_enabled == fc_enabled);
+}
+
+int spi_fc_wait_rq(struct spi_device *spi, u32 fc_state)
+{
+	unsigned long timeout = msecs_to_jiffies(10);
+	int ret = 1;
+
+	if (!(spi->mode & fc_state))
+		return ret;
+
+	if (spi->mode & SPI_FC_MISO_READY)
+		return spi_fc_wait_miso(spi);
+
+	if (atomic_read(&spi->active_rq)) {
+		if (!spi_fc_equal_cs(spi))
+			dev_warn(&spi->dev, "Got request, but device is not ready\n");
+		atomic_set(&spi->active_rq, 0);
+		return ret;
+	}
+
+	if (spi->fc_irq >= 0) {
+		ret = wait_for_completion_io_timeout(&spi->fc_complete,
+						     timeout);
+	} else {
+		int count = 100; /* 10 * 100 = 10-20 msec */
+
+		while (count > 0) {
+			count--;
+			if (spi_fc_equal_cs(spi))
+				break;
+
+			usleep_range(100, 200);
+		}
+		ret = count;
+	}
+
+	if (!ret)
+		dev_warn(&spi->dev, "FC timeout: requested state: 0x%x\n", fc_state);
+
+	return ret;
+}
+
+static irqreturn_t spi_fc_rq(int irq, void *dev_id)
+{
+	struct spi_device *spi = (struct spi_device *)dev_id;
+	int fc_enabled;
+
+	fc_enabled = gpiod_get_value(spi->fc_gpio);
+
+	if (spi->mode | SPI_FC_REQUEST &&
+			!spi->cs_enabled && fc_enabled) {
+		atomic_set(&spi->active_rq, 1);
+		if (spi->request_cb)
+			spi->request_cb(spi);
+	} else if (spi->mode | SPI_FC_STOP_ACK &&
+			!spi->cs_enabled && !fc_enabled) {
+		complete(&spi->fc_complete);
+	} else if (spi->mode | SPI_FC_READY &&
+			spi->cs_enabled && fc_enabled) {
+		complete(&spi->fc_complete);
+	} else {
+		dev_warn(&spi->dev, "Wrong FC State. CS:%i, FC:%i, Mode:0x%x\n",
+			 spi->cs_enabled, fc_enabled, spi->mode);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * spi_fc_probe - Configure Flow Control for called slave device.
+ * @spi: spi_device to register.
+ *
+ * Companion function to spi_setup.  Devices which support Flow Control
+ * functionality need to call this function to be able to use it.
+ *
+ * Return: 0 on success; negative errno on failure.
+ */
+int spi_fc_probe(struct spi_device *spi)
+{
+	struct device_node *np = spi->dev.of_node;
+	int ret;
+
+	if (!np)
+		return 0;
+
+	if (!(spi->mode & SPI_FC_MASK) || spi->mode & SPI_FC_HW_ONLY)
+		return 0;
+
+	spi->fc_gpio = devm_gpiod_get(&spi->dev, "fc", GPIOD_IN);
+	if (IS_ERR(spi->fc_gpio)) {
+		ret = PTR_ERR(spi->fc_gpio);
+		dev_err(&spi->dev, "Failed to request FC GPIO: %d\n", ret);
+		return ret;
+	}
+
+	init_completion(&spi->fc_complete);
+
+	spi->fc_irq = gpiod_to_irq(spi->fc_gpio);
+	if (spi->fc_irq < 0) {
+		/*
+		 * This will dramtically affect transfer speed,
+		 * so it is not recommended, but possible use case.
+		 */
+		dev_warn(&spi->dev, "gpio-fc, filed to get irq for configured pin, use slow polling instead\n");
+	} else {
+		snprintf(spi->fc_irq_name, sizeof(spi->fc_irq_name), "spi-fc-%s",
+			 dev_name(&spi->dev));
+		ret = devm_request_irq(&spi->dev, spi->fc_irq, spi_fc_rq,
+				       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				       spi->fc_irq_name, spi);
+		if (ret) {
+			dev_err(&spi->dev, "Failed to request FC IRQ\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_fc_probe);
+
+/*-------------------------------------------------------------------------*/
 
 /* SPI devices should normally not be created by SPI device drivers; that
  * would make them board-specific.  Similarly with SPI master drivers.
@@ -687,6 +839,9 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n)
 
 static void spi_set_cs(struct spi_device *spi, bool enable)
 {
+	spi->cs_enabled = enable;
+	reinit_completion(&spi->fc_complete);
+
 	if (spi->mode & SPI_CS_HIGH)
 		enable = !enable;
 
@@ -941,9 +1096,15 @@ static int spi_transfer_one_message(struct spi_master *master,
 	unsigned long ms = 1;
 	struct spi_statistics *statm = &master->statistics;
 	struct spi_statistics *stats = &msg->spi->statistics;
+	struct spi_device *spi = msg->spi;
 
 	spi_set_cs(msg->spi, true);
 
+	if (!spi_fc_wait_rq(spi, SPI_FC_READY | SPI_FC_MISO_READY)) {
+		ret = -EREMOTEIO;
+		goto out;
+	}
+
 	SPI_STATISTICS_INCREMENT_FIELD(statm, messages);
 	SPI_STATISTICS_INCREMENT_FIELD(stats, messages);
 
@@ -1006,8 +1167,20 @@ static int spi_transfer_one_message(struct spi_master *master,
 				keep_cs = true;
 			} else {
 				spi_set_cs(msg->spi, false);
-				udelay(10);
+
+				if (!spi_fc_wait_rq(spi, SPI_FC_STOP_ACK)) {
+					ret = -EREMOTEIO;
+					break;
+				}
+
+				if (!(spi->mode & SPI_FC_STOP_ACK))
+					udelay(10);
 				spi_set_cs(msg->spi, true);
+
+				if (!spi_fc_wait_rq(spi, SPI_FC_READY)) {
+					ret = -EREMOTEIO;
+					break;
+				}
 			}
 		}
 
@@ -1015,8 +1188,12 @@ static int spi_transfer_one_message(struct spi_master *master,
 	}
 
 out:
-	if (ret != 0 || !keep_cs)
+
+	if (ret != 0 || !keep_cs) {
 		spi_set_cs(msg->spi, false);
+		if (ret != -EREMOTEIO && !spi_fc_wait_rq(spi, SPI_FC_STOP_ACK))
+			ret = -EREMOTEIO;
+	}
 
 	if (msg->status == -EINPROGRESS)
 		msg->status = ret;
@@ -1483,6 +1660,16 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 		spi->mode |= SPI_3WIRE;
 	if (of_find_property(nc, "spi-lsb-first", NULL))
 		spi->mode |= SPI_LSB_FIRST;
+	if (of_find_property(nc, "spi-fc-ready", NULL))
+		spi->mode |= SPI_FC_READY;
+	if (of_find_property(nc, "spi-fc-stop-ack", NULL))
+		spi->mode |= SPI_FC_STOP_ACK;
+	if (of_find_property(nc, "spi-fc-pause", NULL))
+		spi->mode |= SPI_FC_PAUSE;
+	if (of_find_property(nc, "spi-fc-request", NULL))
+		spi->mode |= SPI_FC_REQUEST;
+	if (of_find_property(nc, "spi-fc-miso-ready", NULL))
+		spi->mode |= SPI_FC_MISO_READY;
 
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 53be3a4..07803f8 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -136,7 +136,7 @@ struct spi_device {
 	u32			max_speed_hz;
 	u8			chip_select;
 	u8			bits_per_word;
-	u16			mode;
+	u32			mode;
 #define	SPI_CPHA	0x01			/* clock phase */
 #define	SPI_CPOL	0x02			/* clock polarity */
 #define	SPI_MODE_0	(0|0)			/* (original MicroWire) */
@@ -148,16 +148,65 @@ struct spi_device {
 #define	SPI_3WIRE	0x10			/* SI/SO signals shared */
 #define	SPI_LOOP	0x20			/* loopback mode */
 #define	SPI_NO_CS	0x40			/* 1 dev/bus, no chipselect */
-#define	SPI_READY	0x80			/* slave pulls low to pause */
+/*
+ * Flow control: Ready Sequence (SPI_FC_READY)
+ * Master CS   |-----1\_______________________|
+ * Slave  FC   |--------2\____________________|
+ * DATA        |-----------3\_________________|
+ * 1. Chips Select set to active by Master.
+ * 2. Flow Control set to active by Slave.
+ * 3. Master starting Data transmission.
+ */
+#define	SPI_FC_READY	0x80
 #define	SPI_TX_DUAL	0x100			/* transmit with 2 wires */
 #define	SPI_TX_QUAD	0x200			/* transmit with 4 wires */
 #define	SPI_RX_DUAL	0x400			/* receive with 2 wires */
 #define	SPI_RX_QUAD	0x800			/* receive with 4 wires */
+/*
+ * Flow control: Pause (SPI_FC_PAUSE)
+ * Master CS   |_______________________/------|
+ * Slave  FC   |_______1/-----\3______/-------|
+ * DATA        |________2/------\4_____/------|
+ */
+#define SPI_FC_PAUSE		0x1000
+/*
+ * Flow control: ACK End of Data (SPI_FC_STOP_ACK)
+ * Master CS   |______________________/2------|
+ * Slave  FC   |________________________/3----|
+ * DATA        |__________________/1----------|
+ */
+#define SPI_FC_STOP_ACK		0x2000
+/*
+ * Flow control: Request Sequence (SPI_FC_REQUEST)
+ * Master CS   |-------2\_____________________|
+ * Slave  FC   |-----1\_______________________|
+ * DATA        |-----------3\_________________|
+ */
+#define SPI_FC_REQUEST		0x4000
+/* If complete FC is done by HW or controller driver, set this flag */
+#define SPI_FC_HW_ONLY		0x8000
+/*
+ * Flow control: Ready signal on MISO (SPI_FC_MISO_READY)
+ * Master CS   |-----1\_______________________|
+ * MISO/DATA   |------2\____3/----------------|
+ * CONV START  |       ^                      |
+ * DATA READY  |             ^                |
+ * When CS get asserted (1), MISO/DATA becomes dominant low (2)
+ * and the ADC conversion starts (within 100ns of (1)).
+ * When MISO/DATA goes dominant high (3) then the conversion has finished
+ * and the transfer may start.
+ * Example: MAX187/189 SPI-ADC.
+ */
+#define SPI_FC_MISO_READY	0x10000
+#define SPI_FC_MASK	(SPI_FC_READY | SPI_FC_PAUSE | \
+			 SPI_FC_STOP_ACK | SPI_FC_REQUEST)
+
 	int			irq;
 	void			*controller_state;
 	void			*controller_data;
 	char			modalias[SPI_NAME_SIZE];
 	int			cs_gpio;	/* chip select gpio */
+	int			cs_enabled;
 
 	/* the statistics */
 	struct spi_statistics	statistics;
@@ -171,6 +220,13 @@ struct spi_device {
 	 *  - chipselect delays
 	 *  - ...
 	 */
+
+	void (*request_cb)(struct spi_device *spi);
+	atomic_t		active_rq;
+	struct completion	fc_complete;
+	struct gpio_desc	*fc_gpio;	/* flow control */
+	int			fc_irq;
+	char			fc_irq_name[32];
 };
 
 static inline struct spi_device *to_spi_device(struct device *dev)
@@ -282,7 +338,6 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 #define module_spi_driver(__spi_driver) \
 	module_driver(__spi_driver, spi_register_driver, \
 			spi_unregister_driver)
-
 /**
  * struct spi_master - interface to SPI master controller
  * @dev: device interface to this driver
@@ -537,6 +592,10 @@ struct spi_master {
 	/* dummy data for full duplex devices */
 	void			*dummy_rx;
 	void			*dummy_tx;
+
+	int	(*wait_for_rq)(struct spi_device *spi);
+	/* some devices use MISO for flow control, see SPI_FC_MISO_READY */
+	int	(*get_miso_level)(struct spi_device *spi);
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
@@ -1146,4 +1205,7 @@ spi_transfer_is_last(struct spi_master *master, struct spi_transfer *xfer)
 	return list_is_last(&xfer->transfer_list, &master->cur_msg->transfers);
 }
 
+int spi_fc_wait_rq(struct spi_device *spi, u32 s5w_state);
+int spi_fc_probe(struct spi_device *spi);
+
 #endif /* __LINUX_SPI_H */
-- 
1.9.1

--
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/6] spi: add add flow control test driver

[Oleksij Rempel]

This testdriver can be used to test flow control
functionality by using some gpio pins to emulate
slave device.

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 drivers/spi/Kconfig       |  12 ++
 drivers/spi/Makefile      |   1 +
 drivers/spi/spi_fc_test.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/spi/spi_fc_test.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7706416..fb96295 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -681,6 +681,18 @@ config SPI_DW_MMIO
 #
 comment "SPI Protocol Masters"
 
+config SPI_FC_TEST
+        tristate "Test for SPI flow control functionality"
+        depends on SPI
+        default n
+        help
+          This option enables test module for flow control SPI
+	  extensions. For testing use debugfs interface with count
+	  of packet wich should be used for testing. For example:
+	  echo 100000 > /sys/kernel/debug/spi_fc_test/spi0/test_fc_request
+
+          If unsure, say N.
+
 config SPI_SPIDEV
 	tristate "User mode SPI device driver support"
 	help
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8991ffc..282224a 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -37,6 +37,7 @@ spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
 obj-$(CONFIG_SPI_EFM32)			+= spi-efm32.o
 obj-$(CONFIG_SPI_EP93XX)		+= spi-ep93xx.o
 obj-$(CONFIG_SPI_FALCON)		+= spi-falcon.o
+obj-$(CONFIG_SPI_FC_TEST)		+= spi_fc_test.o
 obj-$(CONFIG_SPI_FSL_CPM)		+= spi-fsl-cpm.o
 obj-$(CONFIG_SPI_FSL_DSPI)		+= spi-fsl-dspi.o
 obj-$(CONFIG_SPI_FSL_LIB)		+= spi-fsl-lib.o
diff --git a/drivers/spi/spi_fc_test.c b/drivers/spi/spi_fc_test.c
new file mode 100644
index 0000000..65bff99
--- /dev/null
+++ b/drivers/spi/spi_fc_test.c
@@ -0,0 +1,273 @@
+/*
+ * Copyright (C) Robert Bosch Car Multimedia GmbH
+ * Copyright (C) Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/proc_fs.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#define DRIVER_NAME "spi_fc_test"
+
+static struct dentry *debugfs_root;
+/*
+ * Some registers must be read back to modify.
+ * To save time we cache them here in memory
+ */
+struct spi_fc_test_priv {
+	struct mutex		lock;	/* protect from simultaneous accesses */
+	u8			port_config;
+	struct spi_device	*spi;
+	struct gpio_desc	*test_cs_in;
+	struct gpio_desc	*test_fc_out;
+	struct dentry		*debugfs;
+	struct completion	fc_complete;
+	atomic_t		active_rq;
+};
+
+static int spi_fc_test_set(struct spi_fc_test_priv *priv)
+{
+	u8 buf;
+
+	buf = 0xaa;
+	return spi_write(priv->spi, &buf, sizeof(buf));
+}
+
+static void spi_fc_test_request_cb(struct spi_device *spi)
+{
+	struct spi_fc_test_priv *priv = spi_get_drvdata(spi);
+
+	complete(&priv->fc_complete);
+}
+
+static ssize_t spi_fc_fops_request_write(struct file *file, const char
+		__user *user_buf, size_t count, loff_t *data)
+{
+	struct spi_fc_test_priv *priv = file_inode(file)->i_private ?:
+			PDE_DATA(file_inode(file));
+	unsigned long timeout = msecs_to_jiffies(10);
+	u32 rounds;
+	int ret;
+
+	if (kstrtouint_from_user(user_buf, count, 0, &rounds))
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	while (rounds > 0) {
+		atomic_set(&priv->active_rq, 1);
+		reinit_completion(&priv->fc_complete);
+		gpiod_set_value(priv->test_fc_out, true);
+		ret = wait_for_completion_io_timeout(&priv->fc_complete,
+						     timeout);
+		if (!ret) {
+			dev_err(&priv->spi->dev, "Request timeout\n");
+			goto exit;
+		}
+		ret = spi_fc_test_set(priv);
+		if (ret < 0) {
+			dev_err(&priv->spi->dev, "SPI transfer error\n");
+			goto exit;
+		}
+		rounds--;
+	}
+exit:
+	gpiod_set_value(priv->test_fc_out, false);
+	mutex_unlock(&priv->lock);
+	return count;
+}
+
+const struct file_operations spi_fc_fops_request = {
+	.owner      = THIS_MODULE,
+	.write      = spi_fc_fops_request_write,
+};
+
+static ssize_t spi_fc_fops_ready_write(struct file *file, const char
+		__user *user_buf, size_t count, loff_t *data)
+{
+	struct spi_fc_test_priv *priv = file_inode(file)->i_private ?:
+			PDE_DATA(file_inode(file));
+	u32 rounds;
+	int ret;
+
+	if (kstrtouint_from_user(user_buf, count, 0, &rounds))
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	while (rounds > 0) {
+		ret = spi_fc_test_set(priv);
+		if (ret < 0) {
+			mutex_unlock(&priv->lock);
+			return ret;
+		}
+		rounds--;
+	}
+	mutex_unlock(&priv->lock);
+
+	return count;
+}
+
+const struct file_operations spi_fc_fops_ready = {
+	.owner      = THIS_MODULE,
+	.write      = spi_fc_fops_ready_write,
+};
+
+static irqreturn_t spi_fc_test_isr(int irq, void *dev_id)
+{
+	struct spi_fc_test_priv *priv = (struct spi_fc_test_priv *)dev_id;
+	int val;
+
+	if (atomic_read(&priv->active_rq)) {
+		atomic_set(&priv->active_rq, 0);
+		return IRQ_HANDLED;
+	}
+
+	val = gpiod_get_value(priv->test_cs_in);
+	gpiod_set_value(priv->test_fc_out, val);
+
+	return IRQ_HANDLED;
+}
+
+static int spi_fc_test_probe(struct spi_device *spi)
+{
+	struct spi_fc_test_priv *priv;
+	struct dentry *de;
+	int ret, cs_irq;
+
+	spi->bits_per_word = 8;
+
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	priv = devm_kzalloc(&spi->dev, sizeof(struct spi_fc_test_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->lock);
+	init_completion(&priv->fc_complete);
+
+	spi_set_drvdata(spi, priv);
+
+	priv->spi = spi;
+	spi->master->rt = 1;
+	spi->request_cb = spi_fc_test_request_cb;
+
+	ret = spi_fc_probe(spi);
+	if (ret) {
+		dev_err(&spi->dev, "filed to probe FC\n");
+		return ret;
+	}
+
+	priv->test_fc_out = devm_gpiod_get(&spi->dev, "test-fc-out",
+					   GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->test_fc_out)) {
+		ret = PTR_ERR(priv->test_fc_out);
+		dev_err(&spi->dev, "failed to request FC GPIO: %d\n", ret);
+		return ret;
+	}
+
+	priv->test_cs_in = devm_gpiod_get(&spi->dev, "test-cs-in", GPIOD_IN);
+	if (IS_ERR(priv->test_cs_in)) {
+		ret = PTR_ERR(priv->test_cs_in);
+		dev_err(&spi->dev, "failed to request CS GPIO: %d\n", ret);
+		return ret;
+	}
+
+	cs_irq = gpiod_to_irq(priv->test_cs_in);
+	if (cs_irq < 0) {
+		dev_err(&spi->dev, "failed to reques irq for GPIO\n");
+		return -ENODEV;
+	}
+
+	ret = devm_request_irq(&spi->dev, cs_irq, spi_fc_test_isr,
+			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			       "test_cs_in", priv);
+	if (ret) {
+		dev_err(&spi->dev, "failed to request IRQ\n");
+		return ret;
+	}
+
+	priv->debugfs = debugfs_create_dir(dev_name(&spi->dev), debugfs_root);
+	de = debugfs_create_file_size("test_fc_ready", S_IRUGO,
+				      priv->debugfs, priv, &spi_fc_fops_ready,
+				      sizeof(u32));
+	if (IS_ERR_OR_NULL(de)) {
+		dev_err(&spi->dev, "failed to create test_fc_ready\n");
+		return -ENODEV;
+	}
+
+	de = debugfs_create_file_size("test_fc_request", S_IRUGO,
+				      priv->debugfs, priv, &spi_fc_fops_request,
+				      sizeof(u32));
+	if (IS_ERR_OR_NULL(de)) {
+		dev_err(&spi->dev, "failed to create test_fc_request\n");
+		return -ENODEV;
+	}
+
+	return ret;
+}
+
+static int spi_fc_test_remove(struct spi_device *spi)
+{
+	struct spi_fc_test_priv *priv;
+
+	priv = spi_get_drvdata(spi);
+	if (!priv)
+		return -ENODEV;
+
+	mutex_destroy(&priv->lock);
+
+	return 0;
+}
+
+static const struct of_device_id spi_fc_test_gpio_match[] = {
+	{
+		.compatible = "spi-fc-test",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, spi_fc_test_gpio_match);
+
+static struct spi_driver spi_fc_test_driver = {
+	.driver = {
+		.name		= DRIVER_NAME,
+		.of_match_table = of_match_ptr(spi_fc_test_gpio_match),
+	},
+	.probe		= spi_fc_test_probe,
+	.remove		= spi_fc_test_remove,
+};
+
+static int __init spi_fc_test_init(void)
+{
+	debugfs_root = debugfs_create_dir(DRIVER_NAME, NULL);
+	if (IS_ERR_OR_NULL(debugfs_root)) {
+		pr_err("%s: Filed to create debufs entry.\n", DRIVER_NAME);
+		return -ENOMEM;
+	}
+
+	return spi_register_driver(&spi_fc_test_driver);
+}
+subsys_initcall(spi_fc_test_init);
+
+static void __exit spi_fc_test_exit(void)
+{
+	debugfs_remove_recursive(debugfs_root);
+	spi_unregister_driver(&spi_fc_test_driver);
+}
+module_exit(spi_fc_test_exit);
+
+MODULE_AUTHOR("Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>");
+MODULE_LICENSE("GPL");
+
-- 
1.9.1

--
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/6] DT: add documentation for spi-fc-test driver

[Oleksij Rempel]

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 .../devicetree/bindings/spi/spi-fc-test.txt        | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-fc-test.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-fc-test.txt b/Documentation/devicetree/bindings/spi/spi-fc-test.txt
new file mode 100644
index 0000000..c1f14e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-fc-test.txt
@@ -0,0 +1,30 @@
+SPI-FC-TEST is a test driver to simulate slave
+flow control from spi master.
+
+Required properties:
+ - compatible: should be set to "spi-fc-test"
+ - fc-gpio: spi device gpio input for flow control line.
+ - test-cs-in-gpio: chip select gpio input to emulate slave device
+ - test-fc-out-gpio: request gpio output to emulate slave device
+Fallowing required properties are provided by spi framework.
+See spi-bus.txt for more information.
+ - spi-fc-ready
+ - spi-fc-stop-ack
+ - spi-fc-request
+
+Example:
+test-spi@0 {
+	status = "okay";
+	reg = <0>;
+	compatible = "spi-fc-test";
+	spi-max-frequency = <100000>;
+	spi-fc-ready;
+	spi-fc-request;
+	spi-fc-stop-ack;
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi_test_in_pins_bananapi>, <&spi_test_out_pins_bananapi>;
+	fc-gpio = <&pio 7 20 GPIO_ACTIVE_LOW>; /* request line, in IO-4, (PH20) */
+	test-cs-in-gpio = <&pio 7 1 GPIO_ACTIVE_LOW>; /* RXD0, (PH1)  */
+	test-rq-out-gpio = <&pio 7 21 GPIO_ACTIVE_LOW>; /* IO-5, (PH21) */
+};
+
-- 
1.9.1

--
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 3/6] DT: add documentation for spi-fc-test driver

[Mark Brown]

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

On Tue, Mar 01, 2016 at 03:43:17PM +0100, Oleksij Rempel wrote:

>  .../devicetree/bindings/spi/spi-fc-test.txt        | 30 ++++++++++++++++++++++

We should not be supporting DT bindings for a test device that doesn't
represent physical hardware, this is an invitation to abuse by people
writing bad DT bindings for actual hardware.

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

Re: [PATCH 3/6] DT: add documentation for spi-fc-test driver

[fixed-term.Oleksij.Rempel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 02.03.2016 03:17, Mark Brown wrote:
> On Tue, Mar 01, 2016 at 03:43:17PM +0100, Oleksij Rempel wrote:
> 
>> .../devicetree/bindings/spi/spi-fc-test.txt        | 30
>> ++++++++++++++++++++++
> 
> We should not be supporting DT bindings for a test device that
> doesn't represent physical hardware, this is an invitation to abuse
> by people writing bad DT bindings for actual hardware.
> 

Ok, i'll try to reimplement it with module parameters.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iF4EAREIAAYFAlbWhwAACgkQHwImuRkmbWlOOwEAgarzmDA119XNTrZOKDB4OtJW
J8j0WM80sbRwK9DvNQkA/2lYbFeQgS2hhcirvxB6vMr6/5C77sm0evfv/UAZrDqA
=d2kY
-----END PGP SIGNATURE-----

[PATCH 4/6] spi: davinci: set new SPI_FC_* flags

[Oleksij Rempel]

to indicate flow control support.

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 drivers/spi/spi-davinci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index fddb7a3..8728df9 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -351,8 +351,8 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
 	 *
 	 * Version 2 hardware supports an optional handshaking signal,
 	 * so it can support two more modes:
-	 *  - 5 pin SPI variant is standard SPI plus SPI_READY
-	 *  - 4 pin with enable is (SPI_READY | SPI_NO_CS)
+	 *  - 5 pin SPI variant is standard SPI plus SPI_FC_READY
+	 *  - 4 pin with enable is (SPI_FC_READY | SPI_NO_CS)
 	 */
 
 	if (dspi->version == SPI_VERSION_2) {
@@ -374,7 +374,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
 						& SPIDELAY_T2CDELAY_MASK;
 		}
 
-		if (spi->mode & SPI_READY) {
+		if (spi->mode & SPI_FC_READY) {
 			spifmt |= SPIFMT_WAITENA_MASK;
 			delay |= (spicfg->t2edelay << SPIDELAY_T2EDELAY_SHIFT)
 						& SPIDELAY_T2EDELAY_MASK;
@@ -452,7 +452,7 @@ static int davinci_spi_setup(struct spi_device *spi)
 			set_io_bits(dspi->base + SPIPC0, 1 << spi->chip_select);
 	}
 
-	if (spi->mode & SPI_READY)
+	if (spi->mode & SPI_FC_READY)
 		set_io_bits(dspi->base + SPIPC0, SPIPC0_SPIENA_MASK);
 
 	if (spi->mode & SPI_LOOP)
@@ -1021,7 +1021,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 
 	dspi->bitbang.flags = SPI_NO_CS | SPI_LSB_FIRST | SPI_LOOP;
 	if (dspi->version == SPI_VERSION_2)
-		dspi->bitbang.flags |= SPI_READY;
+		dspi->bitbang.flags |= SPI_FC_HW_ONLY | SPI_FC_READY | SPI_FC_PAUSE;
 
 	if (pdev->dev.of_node) {
 		int i;
-- 
1.9.1

--
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 4/6] spi: davinci: set new SPI_FC_* flags

[Mark Brown]

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

On Tue, Mar 01, 2016 at 03:43:18PM +0100, Oleksij Rempel wrote:

> -		if (spi->mode & SPI_READY) {
> +		if (spi->mode & SPI_FC_READY) {

This isn't setting new flags, this is replacing old flags with new ones
which suggests that the patch renaming the flag needs to rename the flag
in all the drivers...

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

Re: [PATCH 4/6] spi: davinci: set new SPI_FC_* flags

[fixed-term.Oleksij.Rempel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 02.03.2016 03:16, Mark Brown wrote:
> On Tue, Mar 01, 2016 at 03:43:18PM +0100, Oleksij Rempel wrote:
> 
>> -		if (spi->mode & SPI_READY) { +		if (spi->mode & SPI_FC_READY)
>> {
> 
> This isn't setting new flags, this is replacing old flags with new
> ones which suggests that the patch renaming the flag needs to
> rename the flag in all the drivers...
> 

Only this driver was using this flag SPI_READY. So i assume, i will
need only to reword patch comment. Right?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iF4EAREIAAYFAlbWh1QACgkQHwImuRkmbWkcDQEAiRMfYYl+PtT4TLSiilvlcD36
B4j8s+YFxKRHpX8BzgwA/2UXedfndYz2pPeZM+F0pwgNhXiFYKZkzjVfso86SQl6
=2QpT
-----END PGP SIGNATURE-----

Re: [PATCH 4/6] spi: davinci: set new SPI_FC_* flags

[Mark Brown]

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

On Wed, Mar 02, 2016 at 07:25:24AM +0100, fixed-term.Oleksij.Rempel wrote:
> On 02.03.2016 03:16, Mark Brown wrote:
> > On Tue, Mar 01, 2016 at 03:43:18PM +0100, Oleksij Rempel wrote:

> >> -		if (spi->mode & SPI_READY) { +		if (spi->mode & SPI_FC_READY)
> >> {

> > This isn't setting new flags, this is replacing old flags with new
> > ones which suggests that the patch renaming the flag needs to
> > rename the flag in all the drivers...

> Only this driver was using this flag SPI_READY. So i assume, i will
> need only to reword patch comment. Right?

No, if this is a rename you need to do it as a rename.

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

Re: [PATCH 4/6] spi: davinci: set new SPI_FC_* flags

[fixed-term.Oleksij.Rempel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Beside, this probably should be done by main patch. In other case this
will brake compilation - ware bad for bisecting.

On 02.03.2016 03:16, Mark Brown wrote:
> On Tue, Mar 01, 2016 at 03:43:18PM +0100, Oleksij Rempel wrote:
> 
>> -		if (spi->mode & SPI_READY) { +		if (spi->mode & SPI_FC_READY)
>> {
> 
> This isn't setting new flags, this is replacing old flags with new
> ones which suggests that the patch renaming the flag needs to
> rename the flag in all the drivers...
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iF4EAREIAAYFAlbWjusACgkQHwImuRkmbWmj6QD/VWneJFfDJb4cq6e4mpM3AjER
wA5p8k5RMwBr3/SaETcA/iPDHn1jstR1tgN2lQV4tRBmAVBz8U5cyKefJlmuvwu1
=8TBT
-----END PGP SIGNATURE-----

[PATCH 5/6] spi: sun4i: set new SPI_FC_* flags

[Oleksij Rempel]

to indicate flow control support.

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 drivers/spi/spi-sun4i.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 1ddd9e2..2443728 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -390,7 +390,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	master->set_cs = sun4i_spi_set_cs;
 	master->transfer_one = sun4i_spi_transfer_one;
 	master->num_chipselect = 4;
-	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST |
+			    SPI_FC_REQUEST | SPI_FC_READY | SPI_FC_STOP_ACK;
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->dev.of_node = pdev->dev.of_node;
 	master->auto_runtime_pm = true;
-- 
1.9.1

--
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 6/6] spi: bitbang: set new SPI_FC_* flags

[Oleksij Rempel]

to indicate flow control support

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 drivers/spi/spi-bitbang.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index 3aa9e6e..5b17295 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -363,7 +363,9 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 	mutex_init(&bitbang->lock);
 
 	if (!master->mode_bits)
-		master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
+		master->mode_bits = SPI_CPOL | SPI_CPHA |
+				    SPI_FC_READY | SPI_FC_STOP_ACK |
+				    SPI_FC_REQUEST | bitbang->flags;
 
 	if (master->transfer || master->transfer_one_message)
 		return -EINVAL;
-- 
1.9.1

--
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/6] spi: add flow controll support

[Mark Brown]

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

On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel wrote:
> Different HW implement different variants of SPI based flow control (FC).
> To flexible FC implementation a spited it to fallowing common parts:

Please don't bury new patches in reply to the middle of some other
thread, it makes it really hard to follow what's going on.

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

Re: [PATCH 1/6] spi: add flow controll support

[Mark Brown]

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

On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel wrote:

At a high level I think this needs splitting up quite a bit - this is
defining a new DT binding, a new core interface for controlling flow
control and a bitbanging flow control implementation.  At least those
three bits should probably be separate patches, it may make sense to
split up the bitbanging implementation some more too.

> Different HW implement different variants of SPI based flow control (FC).
> To flexible FC implementation a spited it to fallowing common parts:
> Flow control: Request Sequence
> Master CS   |-------2\_____________________|
> Slave  FC   |-----1\_______________________|
> DATA        |-----------3\_________________|

I don't know what these pictures mean since you don't say what "slave
FC" is - I'm assuming you mean a separate pin but you need to specify
that.  It really looks like this would be clearer with words rather than
ASCII art.

In the case above this looks like a normal interrupt from a device, I'm
not clear what is different here or why this is in the core?

> Flow control: ACK End of Data
> Master CS   |______________________/2------|
> Slave  FC   |________________________/3----|
> DATA        |__________________/1----------|

Why would anything care about this case and why is it part of the SPI
protocol rather than something that the device driver would do?

> +	if (atomic_read(&spi->active_rq)) {
> +		if (!spi_fc_equal_cs(spi))
> +			dev_warn(&spi->dev, "Got request, but device is not ready\n");
> +		atomic_set(&spi->active_rq, 0);
> +		return ret;
> +	}

Atomic operations are very hard to use safely, you need to really spell
out what you think this is doing from a synchronization point of view
and why it's safe.

> +	if (spi->mode | SPI_FC_REQUEST &&
> +			!spi->cs_enabled && fc_enabled) {
> +		atomic_set(&spi->active_rq, 1);
> +		if (spi->request_cb)
> +			spi->request_cb(spi);

This logic is all too complicated for me to follow.  Why are we oring
things with spi->mode?

> +	struct device_node *np = spi->dev.of_node;
> +	int ret;
> +
> +	if (!np)
> +		return 0;

We can't support things only for DT, we need a way for things to be used
on other systemms.

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

Re: [PATCH 1/6] spi: add flow controll support

[fixed-term.Oleksij.Rempel]

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



On 02.03.2016 03:46, Mark Brown wrote:
> On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel wrote:
> 
> At a high level I think this needs splitting up quite a bit - this is
> defining a new DT binding, a new core interface for controlling flow
> control and a bitbanging flow control implementation.  At least those
> three bits should probably be separate patches, it may make sense to
> split up the bitbanging implementation some more too.

ok

>> Different HW implement different variants of SPI based flow control (FC).
>> To flexible FC implementation a spited it to fallowing common parts:
>> Flow control: Request Sequence
>> Master CS   |-------2\_____________________|
>> Slave  FC   |-----1\_______________________|
>> DATA        |-----------3\_________________|
> 
> I don't know what these pictures mean since you don't say what "slave
> FC" is - I'm assuming you mean a separate pin but you need to specify
> that.  It really looks like this would be clearer with words rather than
> ASCII art.

Ok, Slave FC means a flow control line controlled by Slave - MISO

> 
> In the case above this looks like a normal interrupt from a device, I'm
> not clear what is different here or why this is in the core?

At least our HW use tree different signals for flow control. One of it
is Request Signal.

>> Flow control: ACK End of Data
>> Master CS   |______________________/2------|
>> Slave  FC   |________________________/3----|
>> DATA        |__________________/1----------|
> 
> Why would anything care about this case and why is it part of the SPI
> protocol rather than something that the device driver would do?

This case covers real bugs and in our case we need to detect if master
or slave stalls under one second. If communication problem is detected,
some of recovery scenarios is initiated. Like i said, this protocol is
used for automotive industry.
Please see two attached screen shots of Master and Slave initiated
transfers.

>> +	if (atomic_read(&spi->active_rq)) {
>> +		if (!spi_fc_equal_cs(spi))
>> +			dev_warn(&spi->dev, "Got request, but device is not ready\n");
>> +		atomic_set(&spi->active_rq, 0);
>> +		return ret;
>> +	}
> 
> Atomic operations are very hard to use safely, you need to really spell
> out what you think this is doing from a synchronization point of view
> and why it's safe.

I'm trying to detect if we are on Master or Slave initiated transfer,
which was detected by spi_fc_rq. Normally there should be no other
interrupts on FC line and active_rq should not be changed until this
transfer was processed. The "if (!spi_fc_equal_cs(spi))" check is in
case my assumption is wrong or there is some HW issue. Do you have other
suggestions?

> 
>> +	if (spi->mode | SPI_FC_REQUEST &&
>> +			!spi->cs_enabled && fc_enabled) {
>> +		atomic_set(&spi->active_rq, 1);
>> +		if (spi->request_cb)
>> +			spi->request_cb(spi);
> 
> This logic is all too complicated for me to follow.  Why are we oring
> things with spi->mode?

I'm trying to checking here if fallowing case is actually expected by
slave device. Other suggestions?

>> +	struct device_node *np = spi->dev.of_node;
>> +	int ret;
>> +
>> +	if (!np)
>> +		return 0;
> 
> We can't support things only for DT, we need a way for things to be used
> on other systemms.

I have nothing against it, but i even do not have other HW to test if my
assumptions are correct. Would you accept untested code? :)

This code was tested on Banana Pi with spi-sun4i and spi-gpio drivers.

[-- Attachment #2: spi_fc_ready.jpg --]
[-- Type: image/jpeg, Size: 32734 bytes --]

[-- Attachment #3: spi_fc_request.jpg --]
[-- Type: image/jpeg, Size: 32604 bytes --]

Re: [PATCH 1/6] spi: add flow controll support

[Mark Brown]

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

On Wed, Mar 02, 2016 at 07:48:55AM +0100, fixed-term.Oleksij.Rempel wrote:
> On 02.03.2016 03:46, Mark Brown wrote:
> > On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel wrote:

> > In the case above this looks like a normal interrupt from a device, I'm
> > not clear what is different here or why this is in the core?

> At least our HW use tree different signals for flow control. One of it
> is Request Signal.

That's still an interrupt, just on a different line to the main
interrupt.

> >> Flow control: ACK End of Data
> >> Master CS   |______________________/2------|
> >> Slave  FC   |________________________/3----|
> >> DATA        |__________________/1----------|

> > Why would anything care about this case and why is it part of the SPI
> > protocol rather than something that the device driver would do?

> This case covers real bugs and in our case we need to detect if master
> or slave stalls under one second. If communication problem is detected,
> some of recovery scenarios is initiated. Like i said, this protocol is
> used for automotive industry.

That doesn't answer the second part of my question.  This doesn't seem
to be something that's anything to do with SPI, it's just your device
signalling things out of band.

> Please see two attached screen shots of Master and Slave initiated
> transfers.

Those tell me nothing.

> > Atomic operations are very hard to use safely, you need to really spell
> > out what you think this is doing from a synchronization point of view
> > and why it's safe.

> I'm trying to detect if we are on Master or Slave initiated transfer,
> which was detected by spi_fc_rq. Normally there should be no other
> interrupts on FC line and active_rq should not be changed until this
> transfer was processed. The "if (!spi_fc_equal_cs(spi))" check is in
> case my assumption is wrong or there is some HW issue. Do you have other
> suggestions?

Among other things it really needs to be crystal clear how this update
gets propagated to all CPUs in the system with no race conditions.

> >> +	if (spi->mode | SPI_FC_REQUEST &&
> >> +			!spi->cs_enabled && fc_enabled) {
> >> +		atomic_set(&spi->active_rq, 1);
> >> +		if (spi->request_cb)
> >> +			spi->request_cb(spi);

> > This logic is all too complicated for me to follow.  Why are we oring
> > things with spi->mode?

> I'm trying to checking here if fallowing case is actually expected by
> slave device. Other suggestions?

In what way are we checking what?  The code is incomprehensible.

Again, you're ignoring my specific question about the or.

> > We can't support things only for DT, we need a way for things to be used
> > on other systemms.

> I have nothing against it, but i even do not have other HW to test if my
> assumptions are correct. Would you accept untested code? :)

In general the way we do DT code is that the DT parses into some data
structure and then we work with that data structure.  This means that
everyone's code path is tested.

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

Re: [PATCH 1/6] spi: add flow controll support

[fixed-term.Oleksij.Rempel]

On 02.03.2016 11:55, Mark Brown wrote:
> On Wed, Mar 02, 2016 at 07:48:55AM +0100, fixed-term.Oleksij.Rempel
> wrote:
>> On 02.03.2016 03:46, Mark Brown wrote:
>>> On Tue, Mar 01, 2016 at 03:43:15PM +0100, Oleksij Rempel
>>> wrote:
> 
>>> In the case above this looks like a normal interrupt from a
>>> device, I'm not clear what is different here or why this is in
>>> the core?
> 
>> At least our HW use tree different signals for flow control. One
>> of it is Request Signal.
> 
> That's still an interrupt, just on a different line to the main 
> interrupt.
> 
>>>> Flow control: ACK End of Data Master CS
>>>> |______________________/2------| Slave  FC
>>>> |________________________/3----| DATA
>>>> |__________________/1----------|
> 
>>> Why would anything care about this case and why is it part of
>>> the SPI protocol rather than something that the device driver
>>> would do?
> 
>> This case covers real bugs and in our case we need to detect if
>> master or slave stalls under one second. If communication problem
>> is detected, some of recovery scenarios is initiated. Like i
>> said, this protocol is used for automotive industry.
> 
> That doesn't answer the second part of my question.  This doesn't
> seem to be something that's anything to do with SPI, it's just your
> device signalling things out of band.

Because there are more devices using this protocol, not only this
company. Because implementing ACK signal will cost a lot more code
then it is needed. Because timing for the driver do explode. Because
implementing it for two device on one bus it is horror just to satisfy
spi framework.
This is why we needed to reimplement almost complete spi stack insight
of our driver.
So, do you feel safe if you know that this shit is implemented in your
car? :)

>> Please see two attached screen shots of Master and Slave
>> initiated transfers.
> 
> Those tell me nothing.
> 
>>> Atomic operations are very hard to use safely, you need to
>>> really spell out what you think this is doing from a
>>> synchronization point of view and why it's safe.
> 
>> I'm trying to detect if we are on Master or Slave initiated
>> transfer, which was detected by spi_fc_rq. Normally there should
>> be no other interrupts on FC line and active_rq should not be
>> changed until this transfer was processed. The "if
>> (!spi_fc_equal_cs(spi))" check is in case my assumption is wrong
>> or there is some HW issue. Do you have other suggestions?
> 
> Among other things it really needs to be crystal clear how this
> update gets propagated to all CPUs in the system with no race
> conditions.
> 
>>>> +	if (spi->mode | SPI_FC_REQUEST && +			!spi->cs_enabled &&
>>>> fc_enabled) { +		atomic_set(&spi->active_rq, 1); +		if
>>>> (spi->request_cb) +			spi->request_cb(spi);
> 
>>> This logic is all too complicated for me to follow.  Why are we
>>> oring things with spi->mode?
> 
>> I'm trying to checking here if fallowing case is actually
>> expected by slave device. Other suggestions?
> 
> In what way are we checking what?  The code is incomprehensible.
> 
> Again, you're ignoring my specific question about the or.
> 
>>> We can't support things only for DT, we need a way for things
>>> to be used on other systemms.
> 
>> I have nothing against it, but i even do not have other HW to
>> test if my assumptions are correct. Would you accept untested
>> code? :)
> 
> In general the way we do DT code is that the DT parses into some
> data structure and then we work with that data structure.  This
> means that everyone's code path is tested.
> 

Re: [PATCH 1/6] spi: add flow controll support

[Martin Sperl]

On 01.03.2016 15:43, Oleksij Rempel wrote:
> Different HW implement different variants of SPI based flow control (FC).
> To flexible FC implementation a spited it to fallowing common parts:
> Flow control: Request Sequence
> Master CS   |-------2\_____________________|
> Slave  FC   |-----1\_______________________|
> DATA        |-----------3\_________________|
>
> Flow control: Ready Sequence
> Master CS   |-----1\_______________________|
> Slave  FC   |--------2\____________________|
> DATA        |-----------3\_________________|
>
> Flow control: ACK End of Data
> Master CS   |______________________/2------|
> Slave  FC   |________________________/3----|
> DATA        |__________________/1----------|
>
> Flow control: Pause
> Master CS   |_______________________/------|
> Slave  FC   |_______1/-----\3______/-------|
> DATA        |________2/------\4___/--------|
>
> Flow control: Ready signal on MISO
> Master CS   |-----1\_______________________|
> MISO/DATA   |------2\____3/----------------|
> CONV START  |       ^                      |
> DATA READY  |             ^                |

One alternative idea:

I guess all of the above could also get implemented
without a single framework change like this only in
the spi_device driver that requires this:

* spi_bus_lock
* spi_sync_lock (but with cs_change = 1 on the last transfer,
    so that cs does not get de-asserted - maybe a 0 byte transfer)
* check for your gpio to be of the "expected" level
    (maybe via interrupt or polling)
* spi_sync_lock (continue your transfer)
* spi_bus_unlock

This could also get wrapped in a generic spi_* method.

Maybe another alternative to this approach could be a generic
callback after having finished the processing a specific spi_transfer.
(not the specific version that you propose)

E.g:
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1001,6 +1001,9 @@ static int spi_transfer_one_message(struct 
spi_master *master,
                 if (msg->status != -EINPROGRESS)
                         goto out;

+               if (xfer->complete)
+                       xfer->complete(xfer->context);
+
                 if (xfer->delay_usecs)
                         udelay(xfer->delay_usecs);

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 520a23d..d2b53c4 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -753,6 +753,9 @@ struct spi_transfer {
         u16             delay_usecs;
         u32             speed_hz;

+       void                    (*complete)(void *context);
+       void                    *context;
+
         struct list_head transfer_list;
  };

Then all that is left is implementing that gpio logic as this
callback.

The complete_code could be just waiting for an GPIO-interrupt to wake up
the thread - so it could be as simple as:

void spi_transfer_complete_wait_for_completion(void *context)
{
     /* possibly enable interrupt */
     /* wait for interrupt to wake us up*/
     wait_for_completion(context);
     /* possibly disable interrupt */
     reinit_completion(context);
}

The creation of the GPIO interrupt and such could also be spi_* helper
methods which then could take some of the required info from the
device tree.

This way the whole thing would be orthogonal to the SPI_READY,
for which there is no spi_device driver besides spidev.
So there would be also no impact to out of tree/userland drivers.

Such an interface actually would allow for other (ab)uses -
e.g: read len, then read len bytes becomes easy:

void spi_transfer_complete_read_length_then_read(void *context)
{
    struct spi_transfer *xfer = context;
    struct spi_transfer *next =
         list_first_entry(list, typeof(*next), transfer_list);

    /* saturate on length given in original transfer */
    if (xfer->rx_buf && (xfer->len>0))
        /* this may require unmapping the dma-buffer */
        next->len = min_t(unsigned, len, ((char*)xfer->rx_buf)[0]);

     /* note that if we wanted the ability to abort a transfer,
       * then the complete method would need to return a status
       */
}

All that is required is that a spi_message is created with 2 transfers,
where the first has:
* len = 1
* complete = spi_transfer_complete_read_length_then_read;
* context = xfer[0]
and the second contains:
* len = 16;

Just some ideas of how it could be implemented in a more generic way
that would also allow other uses.

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/6] spi: add flow controll support

[fixed-term.Oleksij.Rempel]

On 02.03.2016 13:26, Martin Sperl wrote:
> 
> 
> On 01.03.2016 15:43, Oleksij Rempel wrote:
>> Different HW implement different variants of SPI based flow control (FC).
>> To flexible FC implementation a spited it to fallowing common parts:
>> Flow control: Request Sequence
>> Master CS   |-------2\_____________________|
>> Slave  FC   |-----1\_______________________|
>> DATA        |-----------3\_________________|
>>
>> Flow control: Ready Sequence
>> Master CS   |-----1\_______________________|
>> Slave  FC   |--------2\____________________|
>> DATA        |-----------3\_________________|
>>
>> Flow control: ACK End of Data
>> Master CS   |______________________/2------|
>> Slave  FC   |________________________/3----|
>> DATA        |__________________/1----------|
>>
>> Flow control: Pause
>> Master CS   |_______________________/------|
>> Slave  FC   |_______1/-----\3______/-------|
>> DATA        |________2/------\4___/--------|
>>
>> Flow control: Ready signal on MISO
>> Master CS   |-----1\_______________________|
>> MISO/DATA   |------2\____3/----------------|
>> CONV START  |       ^                      |
>> DATA READY  |             ^                |
> 
> One alternative idea:
> 
> I guess all of the above could also get implemented
> without a single framework change like this only in
> the spi_device driver that requires this:
> 
> * spi_bus_lock
> * spi_sync_lock (but with cs_change = 1 on the last transfer,
>    so that cs does not get de-asserted - maybe a 0 byte transfer)
> * check for your gpio to be of the "expected" level
>    (maybe via interrupt or polling)
> * spi_sync_lock (continue your transfer)
> * spi_bus_unlock
> 
> This could also get wrapped in a generic spi_* method.
> 
> Maybe another alternative to this approach could be a generic
> callback after having finished the processing a specific spi_transfer.
> (not the specific version that you propose)

Thank you, it sounds match more cleaner then my variant. I'll try it.

> E.g:
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1001,6 +1001,9 @@ static int spi_transfer_one_message(struct
> spi_master *master,
>                 if (msg->status != -EINPROGRESS)
>                         goto out;
> 
> +               if (xfer->complete)
> +                       xfer->complete(xfer->context);
> +
>                 if (xfer->delay_usecs)
>                         udelay(xfer->delay_usecs);
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 520a23d..d2b53c4 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -753,6 +753,9 @@ struct spi_transfer {
>         u16             delay_usecs;
>         u32             speed_hz;
> 
> +       void                    (*complete)(void *context);
> +       void                    *context;
> +
>         struct list_head transfer_list;
>  };
> 
> Then all that is left is implementing that gpio logic as this
> callback.
> 
> The complete_code could be just waiting for an GPIO-interrupt to wake up
> the thread - so it could be as simple as:
> 
> void spi_transfer_complete_wait_for_completion(void *context)
> {
>     /* possibly enable interrupt */
>     /* wait for interrupt to wake us up*/
>     wait_for_completion(context);
>     /* possibly disable interrupt */
>     reinit_completion(context);
> }
> 
> The creation of the GPIO interrupt and such could also be spi_* helper
> methods which then could take some of the required info from the
> device tree.
> 
> This way the whole thing would be orthogonal to the SPI_READY,
> for which there is no spi_device driver besides spidev.
> So there would be also no impact to out of tree/userland drivers.
> 
> Such an interface actually would allow for other (ab)uses -
> e.g: read len, then read len bytes becomes easy:
> 
> void spi_transfer_complete_read_length_then_read(void *context)
> {
>    struct spi_transfer *xfer = context;
>    struct spi_transfer *next =
>         list_first_entry(list, typeof(*next), transfer_list);
> 
>    /* saturate on length given in original transfer */
>    if (xfer->rx_buf && (xfer->len>0))
>        /* this may require unmapping the dma-buffer */
>        next->len = min_t(unsigned, len, ((char*)xfer->rx_buf)[0]);
> 
>     /* note that if we wanted the ability to abort a transfer,
>       * then the complete method would need to return a status
>       */
> }
> 
> All that is required is that a spi_message is created with 2 transfers,
> where the first has:
> * len = 1
> * complete = spi_transfer_complete_read_length_then_read;
> * context = xfer[0]
> and the second contains:
> * len = 16;
> 
> Just some ideas of how it could be implemented in a more generic way
> that would also allow other uses.
> 
> Martin