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

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

quoted

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.

quoted

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.

quoted

+	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?

quoted

+	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?

quoted

+	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.