diff mbox series

[4/7] spi: bcm2835: Drop unused code for native Chip Select

Message ID a24869503ed4e867b11c66c8615a4d5cddb3b2b5.1541659680.git.lukas@wunner.de (mailing list archive)
State Accepted
Commit 5c09e42f59313774bc494b652f3177ee347786d9
Headers show
Series Raspberry Pi spi0 improvements | expand

Commit Message

Lukas Wunner Nov. 8, 2018, 7:06 a.m. UTC
Commit a30a555d7435 ("spi: bcm2835: transform native-cs to gpio-cs on
first spi_setup") disabled the use of hardware-controlled native Chip
Select in favour of software-controlled GPIO Chip Select but left code
to support the former untouched.  Remove it to simplify the driver and
ease the addition of new features and further optimizations.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Mathias Duckeck <m.duckeck@kunbus.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi-bcm2835.c | 96 ++++++---------------------------------
 1 file changed, 13 insertions(+), 83 deletions(-)

Comments

Martin Sperl Nov. 10, 2018, 9:07 a.m. UTC | #1
> On 08.11.2018, at 08:06, Lukas Wunner <lukas@wunner.de> wrote:
> 
> Commit a30a555d7435 ("spi: bcm2835: transform native-cs to gpio-cs on
> first spi_setup") disabled the use of hardware-controlled native Chip
> Select in favour of software-controlled GPIO Chip Select but left code
> to support the former untouched.  Remove it to simplify the driver and
> ease the addition of new features and further optimizations.

I believe downstream is still using native CS instead of GPIO by default.
(Not sure if this really still applies), so they would still be using
this code-fragment.

IIRC the code was als kept for DT-backwards compatibility when the GPIO
function is set to ALT0 - but I may be wrong there...

Martin
Lukas Wunner Nov. 10, 2018, 12:04 p.m. UTC | #2
[cc += Phil Elwell]

On Sat, Nov 10, 2018 at 10:07:38AM +0100, kernel@martin.sperl.org wrote:
> > On 08.11.2018, at 08:06, Lukas Wunner <lukas@wunner.de> wrote:
> > Commit a30a555d7435 ("spi: bcm2835: transform native-cs to gpio-cs on
> > first spi_setup") disabled the use of hardware-controlled native Chip
> > Select in favour of software-controlled GPIO Chip Select but left code
> > to support the former untouched.  Remove it to simplify the driver and
> > ease the addition of new features and further optimizations.
> 
> I believe downstream is still using native CS instead of GPIO by default.
> (Not sure if this really still applies), so they would still be using
> this code-fragment.

Back in 2016 someone reported issues caused by software-controlled CS
with an RFID reader.  Phil Elwell therefore disabled the conversion
from native CS to software-controlled CS as a temporary measure:

   "The quick fix is to push a downstream patch that restores the
    ability to use hardware CE, followed by an overlay that makes
    use of that ability. Later we could try to understand why the
    software CE doesn't work and fix it if possible."

    https://github.com/raspberrypi/linux/issues/1547#issuecomment-230043141
    https://github.com/raspberrypi/linux/commit/b6b83b7b1ef4
    https://github.com/raspberrypi/linux/commit/98eed4009d63

The root cause turned out to be a bug in user space, in the SPI-Py
library to be specific:

    https://github.com/lthiery/SPI-Py/issues/17

After the fix to the SPI-Py library was accepted, the temporary
changes in the kernel and DT should arguably have been reverted,
but unfortunately that never happened.  The commits are still
part of the rpi-4.19.y branch.

However I don't think it is upstream's responsibility to accommodate
to downstream keeping obsolete code changes around.


> IIRC the code was als kept for DT-backwards compatibility when the GPIO
> function is set to ALT0 - but I may be wrong there...

AFAICS, the pin's function is set to alt0 by *default*.

I think there is another issue though with the conversion from native CS
to GPIO CS in bcm2835_spi_setup():  If pin 7/8 as well as pin 35/36 is
configured to alt0, *both* pins should be toggled.  But that issue is
unrelated to the present series.

Thanks,

Lukas
Mark Brown Nov. 14, 2018, 12:24 a.m. UTC | #3
On Sat, Nov 10, 2018 at 01:04:44PM +0100, Lukas Wunner wrote:

> However I don't think it is upstream's responsibility to accommodate
> to downstream keeping obsolete code changes around.

Right, if there's important changes then the expecation is that they'll
be upstreamed.  However in this case it sounds like the rebase required
might actually be a good prompt to clean up some obsolete stuff in the
downstream tree.
diff mbox series

Patch

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 774161bbcb2e..b2febd8e806e 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -172,28 +172,16 @@  static int bcm2835_spi_transfer_one_irq(struct spi_master *master,
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 
-	/* fill in fifo if we have gpio-cs
-	 * note that there have been rare events where the native-CS
-	 * flapped for <1us which may change the behaviour
-	 * with gpio-cs this does not happen, so it is implemented
-	 * only for this case
-	 */
-	if (gpio_is_valid(spi->cs_gpio)) {
-		/* enable HW block, but without interrupts enabled
-		 * this would triggern an immediate interrupt
-		 */
-		bcm2835_wr(bs, BCM2835_SPI_CS,
-			   cs | BCM2835_SPI_CS_TA);
-		/* fill in tx fifo as much as possible */
-		bcm2835_wr_fifo(bs);
-	}
-
 	/*
-	 * Enable the HW block. This will immediately trigger a DONE (TX
-	 * empty) interrupt, upon which we will fill the TX FIFO with the
-	 * first TX bytes. Pre-filling the TX FIFO here to avoid the
-	 * interrupt doesn't work:-(
+	 * Enable HW block, but with interrupts still disabled.
+	 * Otherwise the empty TX FIFO would immediately trigger an interrupt.
 	 */
+	bcm2835_wr(bs, BCM2835_SPI_CS, cs | BCM2835_SPI_CS_TA);
+
+	/* fill TX FIFO as much as possible */
+	bcm2835_wr_fifo(bs);
+
+	/* enable interrupts */
 	cs |= BCM2835_SPI_CS_INTR | BCM2835_SPI_CS_INTD | BCM2835_SPI_CS_TA;
 	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
 
@@ -356,10 +344,6 @@  static bool bcm2835_spi_can_dma(struct spi_master *master,
 				struct spi_device *spi,
 				struct spi_transfer *tfr)
 {
-	/* only run for gpio_cs */
-	if (!gpio_is_valid(spi->cs_gpio))
-		return false;
-
 	/* we start DMA efforts only on bigger transfers */
 	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
 		return false;
@@ -559,12 +543,12 @@  static int bcm2835_spi_transfer_one(struct spi_master *master,
 	else
 		cs &= ~BCM2835_SPI_CS_REN;
 
-	/* for gpio_cs set dummy CS so that no HW-CS get changed
-	 * we can not run this in bcm2835_spi_set_cs, as it does
-	 * not get called for cs_gpio cases, so we need to do it here
+	/*
+	 * The driver always uses software-controlled GPIO Chip Select.
+	 * Set the hardware-controlled native Chip Select to an invalid
+	 * value to prevent it from interfering.
 	 */
-	if (gpio_is_valid(spi->cs_gpio) || (spi->mode & SPI_NO_CS))
-		cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
+	cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
 
 	/* set transmit buffers and length */
 	bs->tx_buf = tfr->tx_buf;
@@ -624,59 +608,6 @@  static void bcm2835_spi_handle_err(struct spi_master *master,
 	bcm2835_spi_reset_hw(master);
 }
 
-static void bcm2835_spi_set_cs(struct spi_device *spi, bool gpio_level)
-{
-	/*
-	 * we can assume that we are "native" as per spi_set_cs
-	 *   calling us ONLY when cs_gpio is not set
-	 * we can also assume that we are CS < 3 as per bcm2835_spi_setup
-	 *   we would not get called because of error handling there.
-	 * the level passed is the electrical level not enabled/disabled
-	 *   so it has to get translated back to enable/disable
-	 *   see spi_set_cs in spi.c for the implementation
-	 */
-
-	struct spi_master *master = spi->master;
-	struct bcm2835_spi *bs = spi_master_get_devdata(master);
-	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
-	bool enable;
-
-	/* calculate the enable flag from the passed gpio_level */
-	enable = (spi->mode & SPI_CS_HIGH) ? gpio_level : !gpio_level;
-
-	/* set flags for "reverse" polarity in the registers */
-	if (spi->mode & SPI_CS_HIGH) {
-		/* set the correct CS-bits */
-		cs |= BCM2835_SPI_CS_CSPOL;
-		cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
-	} else {
-		/* clean the CS-bits */
-		cs &= ~BCM2835_SPI_CS_CSPOL;
-		cs &= ~(BCM2835_SPI_CS_CSPOL0 << spi->chip_select);
-	}
-
-	/* select the correct chip_select depending on disabled/enabled */
-	if (enable) {
-		/* set cs correctly */
-		if (spi->mode & SPI_NO_CS) {
-			/* use the "undefined" chip-select */
-			cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
-		} else {
-			/* set the chip select */
-			cs &= ~(BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01);
-			cs |= spi->chip_select;
-		}
-	} else {
-		/* disable CSPOL which puts HW-CS into deselected state */
-		cs &= ~BCM2835_SPI_CS_CSPOL;
-		/* use the "undefined" chip-select as precaution */
-		cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
-	}
-
-	/* finally set the calculated flags in SPI_CS */
-	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
-}
-
 static int chip_match_name(struct gpio_chip *chip, void *data)
 {
 	return !strcmp(chip->label, data);
@@ -748,7 +679,6 @@  static int bcm2835_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;
 	master->setup = bcm2835_spi_setup;
-	master->set_cs = bcm2835_spi_set_cs;
 	master->transfer_one = bcm2835_spi_transfer_one;
 	master->handle_err = bcm2835_spi_handle_err;
 	master->prepare_message = bcm2835_spi_prepare_message;