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

[fixed-term.Oleksij.Rempel <fixed-term.Oleksij.Rempel@de.bosch.com>]

[-- 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 --]