| Message ID | 20180903215035.17265-3-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | SPI 3WIRE fixes and highz turnaround | expand |
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.
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
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.
> 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
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
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); }
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