[2/4] spi: gpio: Fix reading for 3WIRE

Message ID 20180903215035.17265-3-linus.walleij@linaro.org
State New
Headers show
Series
  • SPI 3WIRE fixes and highz turnaround
Related show

Commit Message

Linus Walleij Sept. 3, 2018, 9:50 p.m.
If he GPIO bitbanged host is registered using just one line,
MISO, naturally the DT parser will flag the host as
SPI_MASTER_NO_RX.

This makes the GPIO SPI driver assign word transfer functions
that enforce the SPI master flags SPI_MASTER_NO_RX (or
SPI_MASTER_NO_TX) to the flags on each call down to the
inlined bitbang functions such as bitbang_txrx_be_cpha0().

In the 3WIRE case, enforcing this flag is wrong, because the
master can then do both TX and RX (albeit not at the same time)
using the same line, by just switching the direction of the
line and keep clocking in bits.

Augment spi_gpio_spec_txrx_word_mode[0123] to account for
this.

Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/spi/spi-gpio.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Mark Brown Sept. 4, 2018, 5:40 p.m. | #1
On Mon, Sep 03, 2018 at 11:50:33PM +0200, Linus Walleij wrote:
> If he GPIO bitbanged host is registered using just one line,

> MISO, naturally the DT parser will flag the host as

> SPI_MASTER_NO_RX.

> 

> This makes the GPIO SPI driver assign word transfer functions

> that enforce the SPI master flags SPI_MASTER_NO_RX (or

> SPI_MASTER_NO_TX) to the flags on each call down to the

> inlined bitbang functions such as bitbang_txrx_be_cpha0().


No, this is at completely the wrong abstraction level and is not going
to help with maintainability at all.  If devices that can recieve data
are being marked as not having that capability that's obviously not
desirable and is the thing that should be fixed, trying to contort
around it in individual drivers is just going to lead to fragility later
on if someone writes other code looks at the flag and trusts it to be
accurate.  We should instead fix the code that sets the flags to take
account of three wire support.
Linus Walleij Sept. 4, 2018, 9:27 p.m. | #2
On Tue, Sep 4, 2018 at 7:40 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 03, 2018 at 11:50:33PM +0200, Linus Walleij wrote:

> > If he GPIO bitbanged host is registered using just one line,

> > MISO, naturally the DT parser will flag the host as

> > SPI_MASTER_NO_RX.

> >

> > This makes the GPIO SPI driver assign word transfer functions

> > that enforce the SPI master flags SPI_MASTER_NO_RX (or

> > SPI_MASTER_NO_TX) to the flags on each call down to the

> > inlined bitbang functions such as bitbang_txrx_be_cpha0().

>

> No, this is at completely the wrong abstraction level and is not going

> to help with maintainability at all.  If devices that can recieve data

> are being marked as not having that capability that's obviously not

> desirable and is the thing that should be fixed, trying to contort

> around it in individual drivers is just going to lead to fragility later

> on if someone writes other code looks at the flag and trusts it to be

> accurate.  We should instead fix the code that sets the flags to take

> account of three wire support.


It's a bit convoluted... :/

The basic brokenness with the current 3WIRE support is that is
doesn't work with just 3 wires, you have to have both MOSI
and MISO for the code to work (as explained by Lorenzo in
another message, his testbed has 4 wires so he didn't see it).

So the code can't even do what it is
named after. At least no bitbang driver can, because the
spi-bitbang-txrx.h have clauses like this:

if ((flags & SPI_MASTER_NO_TX) == 0)
(...)
if ((flags & SPI_MASTER_NO_RX) == 0)
(...)

Where it actively avoids writing or sampling the line unless there
is TX/RX lines (MOSI/MISO).

I will try to provide a proper fix.

Yours,
Linus Walleij
Mark Brown Sept. 5, 2018, 9:41 a.m. | #3
On Tue, Sep 04, 2018 at 11:49:49PM +0200, Linus Walleij wrote:

> So remains to add two new permutations:

> bitbang_txrx_be_3wire_cpha[0|1]() and double the

> code in spi-bitbang-txrx.h. Not good for maintenance.


> So damned if I do, damned if I don't.


> But I guess I go for the latter approach?


Yes, I think so - there's obviously a lot of overhead from bitbanging
this stuff so it's worth it to try to minimize it.
Lorenzo Bianconi Sept. 6, 2018, 9:05 a.m. | #4
> On Tue, Sep 04, 2018 at 11:49:49PM +0200, Linus Walleij wrote:

> 

> > So remains to add two new permutations:

> > bitbang_txrx_be_3wire_cpha[0|1]() and double the

> > code in spi-bitbang-txrx.h. Not good for maintenance.

> 

> > So damned if I do, damned if I don't.

> 

> > But I guess I go for the latter approach?

> 

> Yes, I think so - there's obviously a lot of overhead from bitbanging

> this stuff so it's worth it to try to minimize it.


Hi Linus and Mark,

maybe here we can move flags definition in spi_bitbang_bufs routine, e.g:

-	return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t, 0);
+
+	if (spi->master->flags & (SPI_MASTER_NO_TX | SPI_MASTER_NO_RX))
+		return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t,
+				     spi->master->flags);
+	else
+		return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t, 0);

in this way we can unify spi_gpio_txrx_word_mode{0-3}/spi_gpio_spec_txrx_word_mode{0-3} definitions
in spi-gpio.c and other drivers in the future can use this optimization. What
do you think? Are there any side affects with that approach?

Regards,
Lorenzo
Lorenzo Bianconi Sept. 6, 2018, 1:19 p.m. | #5
On Sep 06, Linus Walleij wrote:
> On Thu, Sep 6, 2018 at 11:05 AM Lorenzo Bianconi

> <lorenzo.bianconi@redhat.com> wrote:

> > > On Tue, Sep 04, 2018 at 11:49:49PM +0200, Linus Walleij wrote:

> > >

> > > > So remains to add two new permutations:

> > > > bitbang_txrx_be_3wire_cpha[0|1]() and double the

> > > > code in spi-bitbang-txrx.h. Not good for maintenance.

> > >

> > > > So damned if I do, damned if I don't.

> > >

> > > > But I guess I go for the latter approach?

> > >

> > > Yes, I think so - there's obviously a lot of overhead from bitbanging

> > > this stuff so it's worth it to try to minimize it.

> >

> > Hi Linus and Mark,

> >

> > maybe here we can move flags definition in spi_bitbang_bufs routine, e.g:

> >

> > -       return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t, 0);

> > +

> > +       if (spi->master->flags & (SPI_MASTER_NO_TX | SPI_MASTER_NO_RX))

> > +               return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t,

> > +                                    spi->master->flags);

> > +       else

> > +               return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t, 0);

> >

> > in this way we can unify spi_gpio_txrx_word_mode{0-3}/spi_gpio_spec_txrx_word_mode{0-3} definitions

> > in spi-gpio.c and other drivers in the future can use this optimization. What

> > do you think? Are there any side affects with that approach?

> 

> I think it's not working because real half-duplex adapters, those

> who *REALLY* cannot do RX or TX will attempt to do RX and TX

> anyways if we do that. Not that I know if there are any.


please correct me if I am wrong but for half-duplex adapters we
will have SPI_MASTER_NO_TX or SPI_MASTER_NO_RX set in master flags,
so we will pass these flags to the master driver. Moreover at the moment
txrx_word[] is set just in (except spi-gpio):
- spi-ath79.c
- spi-butterfly.c
- spi-sh-sci.c
- spi-lm70llp.c

where flags was always set to 0 before commit 304d34360b09 and where
SPI_MASTER_NO_{TX|RX} flags are not actually used.
Anyway maybe there is a better solution :)

Regards,
Lorenzo


> 

> AT lunch I've finally understood, maybe, what needs to be done

> so I' working on some better solution.

> 

> Yours,

> Linus Walleij

Patch

diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 9f4882f82c3c..6bd692304b92 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -183,33 +183,42 @@  static u32 spi_gpio_txrx_word_mode3(struct spi_device *spi,
  * speed in the generic case (when both MISO and MOSI lines are
  * available), as optimiser will remove the checks when argument is
  * constant.
+ *
+ * A special kludge is needed for 3WIRE SPI, as this mode can use
+ * the same line for RX and TX and should not enforce the host
+ * flag - we will just switch MISO from output to input mode when
+ * needed.
  */
 
 static u32 spi_gpio_spec_txrx_word_mode0(struct spi_device *spi,
 		unsigned nsecs, u32 word, u8 bits, unsigned flags)
 {
-	flags = spi->master->flags;
+	if (!(spi->mode & SPI_3WIRE))
+		flags = spi->master->flags;
 	return bitbang_txrx_be_cpha0(spi, nsecs, 0, flags, word, bits);
 }
 
 static u32 spi_gpio_spec_txrx_word_mode1(struct spi_device *spi,
 		unsigned nsecs, u32 word, u8 bits, unsigned flags)
 {
-	flags = spi->master->flags;
+	if (!(spi->mode & SPI_3WIRE))
+		flags = spi->master->flags;
 	return bitbang_txrx_be_cpha1(spi, nsecs, 0, flags, word, bits);
 }
 
 static u32 spi_gpio_spec_txrx_word_mode2(struct spi_device *spi,
 		unsigned nsecs, u32 word, u8 bits, unsigned flags)
 {
-	flags = spi->master->flags;
+	if (!(spi->mode & SPI_3WIRE))
+		flags = spi->master->flags;
 	return bitbang_txrx_be_cpha0(spi, nsecs, 1, flags, word, bits);
 }
 
 static u32 spi_gpio_spec_txrx_word_mode3(struct spi_device *spi,
 		unsigned nsecs, u32 word, u8 bits, unsigned flags)
 {
-	flags = spi->master->flags;
+	if (!(spi->mode & SPI_3WIRE))
+		flags = spi->master->flags;
 	return bitbang_txrx_be_cpha1(spi, nsecs, 1, flags, word, bits);
 }