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

On 02.03.2016 11:55, Mark Brown wrote:

On Wed, Mar 02, 2016 at 07:48:55AM +0100, fixed-term.Oleksij.Rempel
wrote:

quoted

On 02.03.2016 03:46, Mark Brown wrote:

quoted

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

quoted

quoted

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?

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

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?

quoted

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? :)

quoted

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

Those tell me nothing.

quoted

quoted

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.

quoted

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.

quoted

quoted

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);

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

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.