All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/9] spi: bcm2835aux: bug fixes and improvements
@ 2019-03-24 17:49 ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Set of patches improving the spi-bcm2835aux driver and fixing
a data read corruption bug.

The main motivation is a rare data corruption fix that is mostly
observed in polling mode first reported by Hubert Denkmair.

So this patchset first implements a means to control the parameters
of when polling mode is used via module parameters and exports
the corresponding statistics.

As stated in original patch the driver does not support native CS.
But when cs-gpios is not configured in the dt (so a buggy dt) it is
still working with a lot of limitations, but the driver does not report
this fact.

So this patchset adds reporting and allows for a single native CS
(with limited functionality) to continue working with a buggy DT.
One question here remains: do we need to legacy support DTs
that are not following specs in the first place?

Then there is the real fix for the data-corruption which is split
into 3 parts: some code cleanup with code reuse, removing "dangerous"
fifo read (possibly introducing fifo data corruption) and safe fifo read.

Also we remove some dead code.

---
Changelog:
  V1 -> V2: reordering commits as per request of Stefan Wahren
            to help backporting the "essential" bugfixes go in first
	    remove the dependency on a different patchset focused on
	    making cs_change delay configurable

Martin Sperl (9):
  spi: bcm2835aux: unifying code between polling and interrupt driven
    code
  spi: bcm2835aux: remove dangerous uncontrolled read of fifo
  spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
  spi: bcm2835aux: remove dead code
  spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios
  spi: bcm2835aux: warn in dmesg that native cs is not really supported
  spi: bcm2835aux: setup gpio-cs to output and correct level during
    setup
  spi: bcm2835aux: add driver stats to debugfs
  spi: bcm2835aux: make the polling duration limits configurable

 drivers/spi/spi-bcm2835aux.c | 207 +++++++++++++++++++++++++++++++------------
 1 file changed, 150 insertions(+), 57 deletions(-)

--
2.11.0

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH V2 0/9] spi: bcm2835aux: bug fixes and improvements
@ 2019-03-24 17:49 ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Set of patches improving the spi-bcm2835aux driver and fixing
a data read corruption bug.

The main motivation is a rare data corruption fix that is mostly
observed in polling mode first reported by Hubert Denkmair.

So this patchset first implements a means to control the parameters
of when polling mode is used via module parameters and exports
the corresponding statistics.

As stated in original patch the driver does not support native CS.
But when cs-gpios is not configured in the dt (so a buggy dt) it is
still working with a lot of limitations, but the driver does not report
this fact.

So this patchset adds reporting and allows for a single native CS
(with limited functionality) to continue working with a buggy DT.
One question here remains: do we need to legacy support DTs
that are not following specs in the first place?

Then there is the real fix for the data-corruption which is split
into 3 parts: some code cleanup with code reuse, removing "dangerous"
fifo read (possibly introducing fifo data corruption) and safe fifo read.

Also we remove some dead code.

---
Changelog:
  V1 -> V2: reordering commits as per request of Stefan Wahren
            to help backporting the "essential" bugfixes go in first
	    remove the dependency on a different patchset focused on
	    making cs_change delay configurable

Martin Sperl (9):
  spi: bcm2835aux: unifying code between polling and interrupt driven
    code
  spi: bcm2835aux: remove dangerous uncontrolled read of fifo
  spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
  spi: bcm2835aux: remove dead code
  spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios
  spi: bcm2835aux: warn in dmesg that native cs is not really supported
  spi: bcm2835aux: setup gpio-cs to output and correct level during
    setup
  spi: bcm2835aux: add driver stats to debugfs
  spi: bcm2835aux: make the polling duration limits configurable

 drivers/spi/spi-bcm2835aux.c | 207 +++++++++++++++++++++++++++++++------------
 1 file changed, 150 insertions(+), 57 deletions(-)

--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH V2 1/9] spi: bcm2835aux: unifying code between polling and interrupt driven code
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Sharing more code between polling and interrupt-driven mode.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 51 ++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index f7e054848ca5..41366d0a16af 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -178,23 +178,13 @@ static void bcm2835aux_spi_reset_hw(struct bcm2835aux_spi *bs)
 		      BCM2835_AUX_SPI_CNTL0_CLEARFIFO);
 }

-static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
+static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 {
-	struct spi_master *master = dev_id;
-	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
-	irqreturn_t ret = IRQ_NONE;
-
-	/* IRQ may be shared, so return if our interrupts are disabled */
-	if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
-	      (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
-		return ret;
-
 	/* check if we have data to read */
 	while (bs->rx_len &&
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
 		bcm2835aux_rd_fifo(bs);
-		ret = IRQ_HANDLED;
 	}

 	/* check if we have data to write */
@@ -203,7 +193,6 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
 		bcm2835aux_wr_fifo(bs);
-		ret = IRQ_HANDLED;
 	}

 	/* and check if we have reached "done" */
@@ -211,8 +200,21 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_BUSY))) {
 		bcm2835aux_rd_fifo(bs);
-		ret = IRQ_HANDLED;
 	}
+}
+
+static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
+{
+	struct spi_master *master = dev_id;
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	/* IRQ may be shared, so return if our interrupts are disabled */
+	if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
+	      (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
+		return IRQ_NONE;
+
+	/* do common fifo handling */
+	bcm2835aux_spi_transfer_helper(bs);

 	if (!bs->tx_len) {
 		/* disable tx fifo empty interrupt */
@@ -226,8 +228,7 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 		complete(&master->xfer_completion);
 	}

-	/* and return */
-	return ret;
+	return IRQ_HANDLED;
 }

 static int __bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
@@ -273,7 +274,6 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
 	unsigned long timeout;
-	u32 stat;

 	/* configure spi */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
@@ -284,24 +284,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,

 	/* loop until finished the transfer */
 	while (bs->rx_len) {
-		/* read status */
-		stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT);
-
-		/* fill in tx fifo with remaining data */
-		if ((bs->tx_len) && (!(stat & BCM2835_AUX_SPI_STAT_TX_FULL))) {
-			bcm2835aux_wr_fifo(bs);
-			continue;
-		}

-		/* read data from fifo for both cases */
-		if (!(stat & BCM2835_AUX_SPI_STAT_RX_EMPTY)) {
-			bcm2835aux_rd_fifo(bs);
-			continue;
-		}
-		if (!(stat & BCM2835_AUX_SPI_STAT_BUSY)) {
-			bcm2835aux_rd_fifo(bs);
-			continue;
-		}
+		/* do common fifo handling */
+		bcm2835aux_spi_transfer_helper(bs);

 		/* there is still data pending to read check the timeout */
 		if (bs->rx_len && time_after(jiffies, timeout)) {
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 1/9] spi: bcm2835aux: unifying code between polling and interrupt driven code
@ 2019-03-24 17:49     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Sharing more code between polling and interrupt-driven mode.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 51 ++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index f7e054848ca5..41366d0a16af 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -178,23 +178,13 @@ static void bcm2835aux_spi_reset_hw(struct bcm2835aux_spi *bs)
 		      BCM2835_AUX_SPI_CNTL0_CLEARFIFO);
 }

-static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
+static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 {
-	struct spi_master *master = dev_id;
-	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
-	irqreturn_t ret = IRQ_NONE;
-
-	/* IRQ may be shared, so return if our interrupts are disabled */
-	if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
-	      (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
-		return ret;
-
 	/* check if we have data to read */
 	while (bs->rx_len &&
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
 		bcm2835aux_rd_fifo(bs);
-		ret = IRQ_HANDLED;
 	}

 	/* check if we have data to write */
@@ -203,7 +193,6 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
 		bcm2835aux_wr_fifo(bs);
-		ret = IRQ_HANDLED;
 	}

 	/* and check if we have reached "done" */
@@ -211,8 +200,21 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
 		  BCM2835_AUX_SPI_STAT_BUSY))) {
 		bcm2835aux_rd_fifo(bs);
-		ret = IRQ_HANDLED;
 	}
+}
+
+static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
+{
+	struct spi_master *master = dev_id;
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	/* IRQ may be shared, so return if our interrupts are disabled */
+	if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) &
+	      (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE)))
+		return IRQ_NONE;
+
+	/* do common fifo handling */
+	bcm2835aux_spi_transfer_helper(bs);

 	if (!bs->tx_len) {
 		/* disable tx fifo empty interrupt */
@@ -226,8 +228,7 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 		complete(&master->xfer_completion);
 	}

-	/* and return */
-	return ret;
+	return IRQ_HANDLED;
 }

 static int __bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
@@ -273,7 +274,6 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
 	unsigned long timeout;
-	u32 stat;

 	/* configure spi */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
@@ -284,24 +284,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,

 	/* loop until finished the transfer */
 	while (bs->rx_len) {
-		/* read status */
-		stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT);
-
-		/* fill in tx fifo with remaining data */
-		if ((bs->tx_len) && (!(stat & BCM2835_AUX_SPI_STAT_TX_FULL))) {
-			bcm2835aux_wr_fifo(bs);
-			continue;
-		}

-		/* read data from fifo for both cases */
-		if (!(stat & BCM2835_AUX_SPI_STAT_RX_EMPTY)) {
-			bcm2835aux_rd_fifo(bs);
-			continue;
-		}
-		if (!(stat & BCM2835_AUX_SPI_STAT_BUSY)) {
-			bcm2835aux_rd_fifo(bs);
-			continue;
-		}
+		/* do common fifo handling */
+		bcm2835aux_spi_transfer_helper(bs);

 		/* there is still data pending to read check the timeout */
 		if (bs->rx_len && time_after(jiffies, timeout)) {
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 2/9] spi: bcm2835aux: remove dangerous uncontrolled read of fifo
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

This read of the fifo is a potential candidate for a race condition
as the spi transfer is not necessarily finished and so can lead to
an early read of the fifo that still misses data.

So it has been removed.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Suggested-by: Hubert Denkmair <h.denkmair-4pT7WzY1KUiELgA04lAiVw@public.gmane.org>
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 41366d0a16af..0838dbda57c7 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -194,13 +194,6 @@ static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
 		bcm2835aux_wr_fifo(bs);
 	}
-
-	/* and check if we have reached "done" */
-	while (bs->rx_len &&
-	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
-		  BCM2835_AUX_SPI_STAT_BUSY))) {
-		bcm2835aux_rd_fifo(bs);
-	}
 }

 static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 2/9] spi: bcm2835aux: remove dangerous uncontrolled read of fifo
@ 2019-03-24 17:49     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

This read of the fifo is a potential candidate for a race condition
as the spi transfer is not necessarily finished and so can lead to
an early read of the fifo that still misses data.

So it has been removed.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Suggested-by: Hubert Denkmair <h.denkmair@intence.de>
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 41366d0a16af..0838dbda57c7 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -194,13 +194,6 @@ static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
 		bcm2835aux_wr_fifo(bs);
 	}
-
-	/* and check if we have reached "done" */
-	while (bs->rx_len &&
-	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
-		  BCM2835_AUX_SPI_STAT_BUSY))) {
-		bcm2835aux_rd_fifo(bs);
-	}
 }

 static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 3/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49   ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

On long running tests with a mcp2517fd can controller it showed that
on rare occations the data read shows corruptions for longer spi transfers.

Example of a 22 byte transfer:

expected (as captured on logic analyzer):
FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 85 86 87 88 89 8a 8b

read by the driver:
FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 88 89 8a 00 00 8b 9b

To fix this use BCM2835_AUX_SPI_STAT_RX_LVL to determine when we may
read data from the fifo reliably without any corruption.

Surprisingly the only values ever empirically read in
BCM2835_AUX_SPI_STAT_RX_LVL are 0x00, 0x10, 0x20 and 0x30.
So whenever the mask is not 0 we can read from the fifo in a safe manner.

The patch has now been tested intensively and we are no longer
able to reproduce the "RX" issue any longer.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Reported-by: Hubert Denkmair <h.denkmair@intence.de>
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 0838dbda57c7..d9e62f717a45 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -180,12 +180,12 @@ static void bcm2835aux_spi_reset_hw(struct bcm2835aux_spi *bs)

 static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 {
+	u32 stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT);
+
 	/* check if we have data to read */
-	while (bs->rx_len &&
-	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
-		  BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
+	for (; bs->rx_len && (stat & BCM2835_AUX_SPI_STAT_RX_LVL);
+	     stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT))
 		bcm2835aux_rd_fifo(bs);
-	}

 	/* check if we have data to write */
 	while (bs->tx_len &&
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 3/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
@ 2019-03-24 17:49   ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

On long running tests with a mcp2517fd can controller it showed that
on rare occations the data read shows corruptions for longer spi transfers.

Example of a 22 byte transfer:

expected (as captured on logic analyzer):
FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 85 86 87 88 89 8a 8b

read by the driver:
FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 88 89 8a 00 00 8b 9b

To fix this use BCM2835_AUX_SPI_STAT_RX_LVL to determine when we may
read data from the fifo reliably without any corruption.

Surprisingly the only values ever empirically read in
BCM2835_AUX_SPI_STAT_RX_LVL are 0x00, 0x10, 0x20 and 0x30.
So whenever the mask is not 0 we can read from the fifo in a safe manner.

The patch has now been tested intensively and we are no longer
able to reproduce the "RX" issue any longer.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Reported-by: Hubert Denkmair <h.denkmair@intence.de>
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 0838dbda57c7..d9e62f717a45 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -180,12 +180,12 @@ static void bcm2835aux_spi_reset_hw(struct bcm2835aux_spi *bs)

 static void bcm2835aux_spi_transfer_helper(struct bcm2835aux_spi *bs)
 {
+	u32 stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT);
+
 	/* check if we have data to read */
-	while (bs->rx_len &&
-	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
-		  BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
+	for (; bs->rx_len && (stat & BCM2835_AUX_SPI_STAT_RX_LVL);
+	     stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT))
 		bcm2835aux_rd_fifo(bs);
-	}

 	/* check if we have data to write */
 	while (bs->tx_len &&
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 4/9] spi: bcm2835aux: remove dead code
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Remove dead code that never can get reached, as we limit count to
a max of 3.

Suggested-by: Hubert Denkmair <h.denkmair-4pT7WzY1KUiELgA04lAiVw@public.gmane.org>
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d9e62f717a45..890aca4873a7 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -123,9 +123,6 @@ static inline void bcm2835aux_rd_fifo(struct bcm2835aux_spi *bs)
 	data = bcm2835aux_rd(bs, BCM2835_AUX_SPI_IO);
 	if (bs->rx_buf) {
 		switch (count) {
-		case 4:
-			*bs->rx_buf++ = (data >> 24) & 0xff;
-			/* fallthrough */
 		case 3:
 			*bs->rx_buf++ = (data >> 16) & 0xff;
 			/* fallthrough */
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 4/9] spi: bcm2835aux: remove dead code
@ 2019-03-24 17:49     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Remove dead code that never can get reached, as we limit count to
a max of 3.

Suggested-by: Hubert Denkmair <h.denkmair@intence.de>
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d9e62f717a45..890aca4873a7 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -123,9 +123,6 @@ static inline void bcm2835aux_rd_fifo(struct bcm2835aux_spi *bs)
 	data = bcm2835aux_rd(bs, BCM2835_AUX_SPI_IO);
 	if (bs->rx_buf) {
 		switch (count) {
-		case 4:
-			*bs->rx_buf++ = (data >> 24) & 0xff;
-			/* fallthrough */
 		case 3:
 			*bs->rx_buf++ = (data >> 16) & 0xff;
 			/* fallthrough */
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 5/9] spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

The original driver by default defines num_chipselects as -1.
This actually allicates an array of 65535 entries in
of_spi_register_master.

There is a side-effect for buggy device trees that (contrary to
dt-binding documentation) have no cs-gpio defined.

This mode was never supported by the driver due to limitations
of native cs and additional code complexity and is explicitly
not stated to be implemented.

To keep backwards compatibility with such buggy DTs we limit
the number of chip_selects to 1, as for all practical purposes
it is only ever realistic to use a single chip select in
native cs mode without negative side-effects.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 890aca4873a7..c8acde017b6a 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -413,7 +413,18 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, master);
 	master->mode_bits = (SPI_CPOL | SPI_CS_HIGH | SPI_NO_CS);
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
-	master->num_chipselect = -1;
+	/* even though the driver never officially supported native CS
+	 * allow a single native CS for legacy DT support purposes when
+	 * no cs-gpio is configured.
+	 * Known limitations for native cs are:
+	 * * multiple chip-selects: cs0-cs2 are all simultaniously asserted
+	 *     whenever there is a transfer -  this even includes SPI_NO_CS
+	 * * SPI_CS_HIGH: is ignores - cs are always asserted low
+	 * * cs_change: cs is deasserted after each spi_transfer
+	 * * cs_delay_usec: cs is always deasserted one SCK cycle after
+	 *     a spi_transfer
+	 */
+	master->num_chipselect = 1;
 	master->transfer_one = bcm2835aux_spi_transfer_one;
 	master->handle_err = bcm2835aux_spi_handle_err;
 	master->prepare_message = bcm2835aux_spi_prepare_message;
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 5/9] spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios
@ 2019-03-24 17:49     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

The original driver by default defines num_chipselects as -1.
This actually allicates an array of 65535 entries in
of_spi_register_master.

There is a side-effect for buggy device trees that (contrary to
dt-binding documentation) have no cs-gpio defined.

This mode was never supported by the driver due to limitations
of native cs and additional code complexity and is explicitly
not stated to be implemented.

To keep backwards compatibility with such buggy DTs we limit
the number of chip_selects to 1, as for all practical purposes
it is only ever realistic to use a single chip select in
native cs mode without negative side-effects.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 890aca4873a7..c8acde017b6a 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -413,7 +413,18 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, master);
 	master->mode_bits = (SPI_CPOL | SPI_CS_HIGH | SPI_NO_CS);
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
-	master->num_chipselect = -1;
+	/* even though the driver never officially supported native CS
+	 * allow a single native CS for legacy DT support purposes when
+	 * no cs-gpio is configured.
+	 * Known limitations for native cs are:
+	 * * multiple chip-selects: cs0-cs2 are all simultaniously asserted
+	 *     whenever there is a transfer -  this even includes SPI_NO_CS
+	 * * SPI_CS_HIGH: is ignores - cs are always asserted low
+	 * * cs_change: cs is deasserted after each spi_transfer
+	 * * cs_delay_usec: cs is always deasserted one SCK cycle after
+	 *     a spi_transfer
+	 */
+	master->num_chipselect = 1;
 	master->transfer_one = bcm2835aux_spi_transfer_one;
 	master->handle_err = bcm2835aux_spi_handle_err;
 	master->prepare_message = bcm2835aux_spi_prepare_message;
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 6/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:49   ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

>From personal bad experience (even as the author of the original driver)
it shows that native-cs is "somewhat" supported by the spi bus driver
when using a buggy device tree.

So make sure that the driver is warning in dmesg about this fact
that we are running in a not supported mode that may have surprizing
limitations.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
---
 drivers/spi/spi-bcm2835aux.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index c8acde017b6a..6af07dec5fdc 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -396,6 +396,38 @@ static void bcm2835aux_spi_handle_err(struct spi_master *master,
 	bcm2835aux_spi_reset_hw(bs);
 }

+static int bcm2835aux_spi_setup(struct spi_device *spi)
+{
+	int ret;
+
+	if (spi->mode & SPI_NO_CS)
+		return 0;
+	/* sanity check for native cs */
+	if (!gpio_is_valid(spi->cs_gpio)) {
+		/* for dt-backwards compatibility: only support native on CS0
+		 * known things not supported with broken native CS:
+		 * * multiple chip-selects: cs0-cs2 are all
+		 *     simultaniously asserted whenever there is a transfer
+		 *     this even includes SPI_NO_CS
+		 * * SPI_CS_HIGH: cs are always asserted low
+		 * * cs_change: cs is deasserted after each spi_transfer
+		 * * cs_delay_usec: cs is always deasserted one SCK cycle
+		 *     after the last transfer
+		 * probably more...
+		 */
+		if (spi->chip_select == 0) {
+			dev_err(&spi->dev,
+				"native CS is not supported but may work for some use-cases for cs = 0 -  please define a valid value cs-gpios in DT for complete feature set\n");
+			return 0;
+		}
+		dev_err(&spi->dev,
+			"native CS is not supported for cs > 0 -  please define a valid value cs-gpios in DT to enable multiple cs\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int bcm2835aux_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -425,6 +457,7 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 	 *     a spi_transfer
 	 */
 	master->num_chipselect = 1;
+	master->setup = bcm2835aux_spi_setup;
 	master->transfer_one = bcm2835aux_spi_transfer_one;
 	master->handle_err = bcm2835aux_spi_handle_err;
 	master->prepare_message = bcm2835aux_spi_prepare_message;
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 6/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported
@ 2019-03-24 17:49   ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:49 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

From personal bad experience (even as the author of the original driver)
it shows that native-cs is "somewhat" supported by the spi bus driver
when using a buggy device tree.

So make sure that the driver is warning in dmesg about this fact
that we are running in a not supported mode that may have surprizing
limitations.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
---
 drivers/spi/spi-bcm2835aux.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index c8acde017b6a..6af07dec5fdc 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -396,6 +396,38 @@ static void bcm2835aux_spi_handle_err(struct spi_master *master,
 	bcm2835aux_spi_reset_hw(bs);
 }

+static int bcm2835aux_spi_setup(struct spi_device *spi)
+{
+	int ret;
+
+	if (spi->mode & SPI_NO_CS)
+		return 0;
+	/* sanity check for native cs */
+	if (!gpio_is_valid(spi->cs_gpio)) {
+		/* for dt-backwards compatibility: only support native on CS0
+		 * known things not supported with broken native CS:
+		 * * multiple chip-selects: cs0-cs2 are all
+		 *     simultaniously asserted whenever there is a transfer
+		 *     this even includes SPI_NO_CS
+		 * * SPI_CS_HIGH: cs are always asserted low
+		 * * cs_change: cs is deasserted after each spi_transfer
+		 * * cs_delay_usec: cs is always deasserted one SCK cycle
+		 *     after the last transfer
+		 * probably more...
+		 */
+		if (spi->chip_select == 0) {
+			dev_err(&spi->dev,
+				"native CS is not supported but may work for some use-cases for cs = 0 -  please define a valid value cs-gpios in DT for complete feature set\n");
+			return 0;
+		}
+		dev_err(&spi->dev,
+			"native CS is not supported for cs > 0 -  please define a valid value cs-gpios in DT to enable multiple cs\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int bcm2835aux_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -425,6 +457,7 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 	 *     a spi_transfer
 	 */
 	master->num_chipselect = 1;
+	master->setup = bcm2835aux_spi_setup;
 	master->transfer_one = bcm2835aux_spi_transfer_one;
 	master->handle_err = bcm2835aux_spi_handle_err;
 	master->prepare_message = bcm2835aux_spi_prepare_message;
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 7/9] spi: bcm2835aux: setup gpio-cs to output and correct level during setup
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:50     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Setup gpio-cs to the correct levels during setup and also make the
gpio definitely an output GPIO.

This is transparently fixing some badly configured DTs in the process
where cs-gpio is set but the gpios are still configured with native cs.

It also makes 100% sure that the initial CS levels are as expected -
especially on systems with devices on a bus with mixed CS_HIGH/CS_LOW
settings.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 6af07dec5fdc..dd0446968da6 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -425,7 +425,17 @@ static int bcm2835aux_spi_setup(struct spi_device *spi)
 		return -EINVAL;
 	}

-	return 0;
+	/* with gpio-cs set the GPIO to the correct level
+	 * and as output (in case the dt has the gpio not configured
+	 * as output but native cs)
+	 */
+	ret = gpio_direction_output(spi->cs_gpio,
+				    (spi->mode & SPI_CS_HIGH) ? 0 : 1);
+	if (ret)
+		dev_err(&spi->dev, "could not set gpio %i as output: %i\n",
+			spi->cs_gpio, ret);
+
+	return ret;
 }

 static int bcm2835aux_spi_probe(struct platform_device *pdev)
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 7/9] spi: bcm2835aux: setup gpio-cs to output and correct level during setup
@ 2019-03-24 17:50     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Setup gpio-cs to the correct levels during setup and also make the
gpio definitely an output GPIO.

This is transparently fixing some badly configured DTs in the process
where cs-gpio is set but the gpios are still configured with native cs.

It also makes 100% sure that the initial CS levels are as expected -
especially on systems with devices on a bus with mixed CS_HIGH/CS_LOW
settings.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 6af07dec5fdc..dd0446968da6 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -425,7 +425,17 @@ static int bcm2835aux_spi_setup(struct spi_device *spi)
 		return -EINVAL;
 	}

-	return 0;
+	/* with gpio-cs set the GPIO to the correct level
+	 * and as output (in case the dt has the gpio not configured
+	 * as output but native cs)
+	 */
+	ret = gpio_direction_output(spi->cs_gpio,
+				    (spi->mode & SPI_CS_HIGH) ? 0 : 1);
+	if (ret)
+		dev_err(&spi->dev, "could not set gpio %i as output: %i\n",
+			spi->cs_gpio, ret);
+
+	return ret;
 }

 static int bcm2835aux_spi_probe(struct platform_device *pdev)
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:50     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

To estimate efficiency add statistics on transfer types
(polling and interrupt) used to debugfs.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index dd0446968da6..d2b58060b333 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -21,6 +21,7 @@

 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -102,8 +103,57 @@ struct bcm2835aux_spi {
 	int tx_len;
 	int rx_len;
 	int pending;
+
+#if defined(CONFIG_DEBUG_FS)
+#define BCM2835_AUX_INCR(field) ((field)++)
+	u64 count_transfer_polling;
+	u64 count_transfer_irq;
+	u64 count_transfer_irq_after_poll;
+
+	struct dentry *debugfs_dir;
+#else
+#define BCM2835_AUX_INCR(field)
+#endif /* CONFIG_DEBUG_FS */
 };

+#if defined(CONFIG_DEBUG_FS)
+static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs,
+				      const char *dname)
+{
+	char name[64];
+	struct dentry *dir;
+
+	/* get full name */
+	snprintf(name, sizeof(name), "spi-bcm2835aux-%s", dname);
+
+	/* the base directory */
+	dir = debugfs_create_dir(name, NULL);
+	bs->debugfs_dir = dir;
+
+	/* the counters */
+	debugfs_create_u64("count_transfer_polling", 0444, dir,
+			   &bs->count_transfer_polling);
+	debugfs_create_u64("count_transfer_irq", 0444, dir,
+			   &bs->count_transfer_irq);
+	debugfs_create_u64("count_transfer_irq_after_poll", 0444, dir,
+			   &bs->count_transfer_irq_after_poll);
+}
+
+static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs)
+{
+	debugfs_remove_recursive(bs->debugfs_dir);
+	bs->debugfs_dir = NULL;
+}
+#else
+static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs)
+{
+}
+
+static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs)
+{
+}
+#endif /* CONFIG_DEBUG_FS */
+
 static inline u32 bcm2835aux_rd(struct bcm2835aux_spi *bs, unsigned reg)
 {
 	return readl(bs->regs + reg);
@@ -242,6 +292,9 @@ static int bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);

+	/* update statistics */
+	BCM2835_AUX_INCR(bs->count_transfer_irq);
+
 	/* fill in registers and fifos before enabling interrupts */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
@@ -265,6 +318,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
 	unsigned long timeout;

+	/* update statistics */
+	BCM2835_AUX_INCR(bs->count_transfer_polling);
+
 	/* configure spi */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
@@ -285,6 +341,7 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 					    jiffies - timeout,
 					    bs->tx_len, bs->rx_len);
 			/* forward to interrupt handler */
+			BCM2835_AUX_INCR(bs->count_transfer_irq_after_poll);
 			return __bcm2835aux_spi_transfer_one_irq(master,
 							       spi, tfr);
 		}
@@ -531,6 +588,8 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 		goto out_clk_disable;
 	}

+	bcm2835aux_debugfs_create(bs, dev_name(&pdev->dev));
+
 	return 0;

 out_clk_disable:
@@ -545,6 +604,8 @@ static int bcm2835aux_spi_remove(struct platform_device *pdev)
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);

+	bcm2835aux_debugfs_remove(bs);
+
 	bcm2835aux_spi_reset_hw(bs);

 	/* disable the HW block by releasing the clock */
--
2.11.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
@ 2019-03-24 17:50     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

To estimate efficiency add statistics on transfer types
(polling and interrupt) used to debugfs.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835aux.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index dd0446968da6..d2b58060b333 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -21,6 +21,7 @@

 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -102,8 +103,57 @@ struct bcm2835aux_spi {
 	int tx_len;
 	int rx_len;
 	int pending;
+
+#if defined(CONFIG_DEBUG_FS)
+#define BCM2835_AUX_INCR(field) ((field)++)
+	u64 count_transfer_polling;
+	u64 count_transfer_irq;
+	u64 count_transfer_irq_after_poll;
+
+	struct dentry *debugfs_dir;
+#else
+#define BCM2835_AUX_INCR(field)
+#endif /* CONFIG_DEBUG_FS */
 };

+#if defined(CONFIG_DEBUG_FS)
+static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs,
+				      const char *dname)
+{
+	char name[64];
+	struct dentry *dir;
+
+	/* get full name */
+	snprintf(name, sizeof(name), "spi-bcm2835aux-%s", dname);
+
+	/* the base directory */
+	dir = debugfs_create_dir(name, NULL);
+	bs->debugfs_dir = dir;
+
+	/* the counters */
+	debugfs_create_u64("count_transfer_polling", 0444, dir,
+			   &bs->count_transfer_polling);
+	debugfs_create_u64("count_transfer_irq", 0444, dir,
+			   &bs->count_transfer_irq);
+	debugfs_create_u64("count_transfer_irq_after_poll", 0444, dir,
+			   &bs->count_transfer_irq_after_poll);
+}
+
+static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs)
+{
+	debugfs_remove_recursive(bs->debugfs_dir);
+	bs->debugfs_dir = NULL;
+}
+#else
+static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs)
+{
+}
+
+static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs)
+{
+}
+#endif /* CONFIG_DEBUG_FS */
+
 static inline u32 bcm2835aux_rd(struct bcm2835aux_spi *bs, unsigned reg)
 {
 	return readl(bs->regs + reg);
@@ -242,6 +292,9 @@ static int bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);

+	/* update statistics */
+	BCM2835_AUX_INCR(bs->count_transfer_irq);
+
 	/* fill in registers and fifos before enabling interrupts */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
@@ -265,6 +318,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
 	unsigned long timeout;

+	/* update statistics */
+	BCM2835_AUX_INCR(bs->count_transfer_polling);
+
 	/* configure spi */
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
@@ -285,6 +341,7 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 					    jiffies - timeout,
 					    bs->tx_len, bs->rx_len);
 			/* forward to interrupt handler */
+			BCM2835_AUX_INCR(bs->count_transfer_irq_after_poll);
 			return __bcm2835aux_spi_transfer_one_irq(master,
 							       spi, tfr);
 		}
@@ -531,6 +588,8 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 		goto out_clk_disable;
 	}

+	bcm2835aux_debugfs_create(bs, dev_name(&pdev->dev));
+
 	return 0;

 out_clk_disable:
@@ -545,6 +604,8 @@ static int bcm2835aux_spi_remove(struct platform_device *pdev)
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);

+	bcm2835aux_debugfs_remove(bs);
+
 	bcm2835aux_spi_reset_hw(bs);

 	/* disable the HW block by releasing the clock */
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
  2019-03-24 17:49 ` kernel
@ 2019-03-24 17:50     ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel-TqfNSX0MhmxHKSADF0wUEw @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

From: Martin Sperl <kernel@martin.sperl.org>

Under some circumstances the default 30 us polling limit is not optimal
and may lead to long delays because we are waiting on an interrupt.
with this patch we have the possibility to influence this policy.

So make this limit (in us) configurable via a module parameters
(but also modifyable via /sys/modules/...)

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

---
Changelog:
  V1 -> V2: remove the dependency on a different patchset focused on
	    making cs_change delay configurable

---
 drivers/spi/spi-bcm2835aux.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d2b58060b333..df065108122b 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -37,6 +37,12 @@
 #include <linux/spi/spi.h>
 #include <linux/spinlock.h>

+/* define polling limits */
+unsigned int polling_limit_us = 30;
+module_param(polling_limit_us, uint, 0664);
+MODULE_PARM_DESC(polling_limit_us,
+		 "time in us to run a transfer in polling mode\n");
+
 /*
  * spi register defines
  *
@@ -89,10 +95,6 @@
 #define BCM2835_AUX_SPI_STAT_BUSY	0x00000040
 #define BCM2835_AUX_SPI_STAT_BITCOUNT	0x0000003F

-/* timeout values */
-#define BCM2835_AUX_SPI_POLLING_LIMIT_US	30
-#define BCM2835_AUX_SPI_POLLING_JIFFIES		2
-
 struct bcm2835aux_spi {
 	void __iomem *regs;
 	struct clk *clk;
@@ -325,8 +327,8 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

-	/* set the timeout */
-	timeout = jiffies + BCM2835_AUX_SPI_POLLING_JIFFIES;
+	/* set the timeout to at least 2 jiffies */
+	timeout = jiffies + 2 + HZ * polling_limit_us / 1000000;

 	/* loop until finished the transfer */
 	while (bs->rx_len) {
@@ -356,8 +358,8 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 				       struct spi_transfer *tfr)
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
-	unsigned long spi_hz, clk_hz, speed;
-	unsigned long spi_used_hz;
+	unsigned long spi_hz, clk_hz, speed, spi_used_hz;
+	unsigned long hz_per_byte, byte_limit;

 	/* calculate the registers to handle
 	 *
@@ -401,14 +403,15 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	 * of Hz per byte per polling limit.  E.g., we can transfer 1 byte in
 	 * 30 µs per 300,000 Hz of bus clock.
 	 */
-#define HZ_PER_BYTE ((9 * 1000000) / BCM2835_AUX_SPI_POLLING_LIMIT_US)
+	hz_per_byte = polling_limit_us ? (9 * 1000000) / polling_limit_us : 0;
+	byte_limit = hz_per_byte ? spi_used_hz / hz_per_byte : 1;
+
 	/* run in polling mode for short transfers */
-	if (tfr->len < spi_used_hz / HZ_PER_BYTE)
+	if (tfr->len < byte_limit)
 		return bcm2835aux_spi_transfer_one_poll(master, spi, tfr);

 	/* run in interrupt mode for all others */
 	return bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
-#undef HZ_PER_BYTE
 }

 static int bcm2835aux_spi_prepare_message(struct spi_master *master,
--
2.11.0

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
@ 2019-03-24 17:50     ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-24 17:50 UTC (permalink / raw)
  To: Mark Brown, Eric Anholt, Stefan Wahren, Hubert Denkmair,
	linux-spi, linux-rpi-kernel, linux-arm-kernel
  Cc: Martin Sperl

From: Martin Sperl <kernel@martin.sperl.org>

Under some circumstances the default 30 us polling limit is not optimal
and may lead to long delays because we are waiting on an interrupt.
with this patch we have the possibility to influence this policy.

So make this limit (in us) configurable via a module parameters
(but also modifyable via /sys/modules/...)

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

---
Changelog:
  V1 -> V2: remove the dependency on a different patchset focused on
	    making cs_change delay configurable

---
 drivers/spi/spi-bcm2835aux.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d2b58060b333..df065108122b 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -37,6 +37,12 @@
 #include <linux/spi/spi.h>
 #include <linux/spinlock.h>

+/* define polling limits */
+unsigned int polling_limit_us = 30;
+module_param(polling_limit_us, uint, 0664);
+MODULE_PARM_DESC(polling_limit_us,
+		 "time in us to run a transfer in polling mode\n");
+
 /*
  * spi register defines
  *
@@ -89,10 +95,6 @@
 #define BCM2835_AUX_SPI_STAT_BUSY	0x00000040
 #define BCM2835_AUX_SPI_STAT_BITCOUNT	0x0000003F

-/* timeout values */
-#define BCM2835_AUX_SPI_POLLING_LIMIT_US	30
-#define BCM2835_AUX_SPI_POLLING_JIFFIES		2
-
 struct bcm2835aux_spi {
 	void __iomem *regs;
 	struct clk *clk;
@@ -325,8 +327,8 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

-	/* set the timeout */
-	timeout = jiffies + BCM2835_AUX_SPI_POLLING_JIFFIES;
+	/* set the timeout to at least 2 jiffies */
+	timeout = jiffies + 2 + HZ * polling_limit_us / 1000000;

 	/* loop until finished the transfer */
 	while (bs->rx_len) {
@@ -356,8 +358,8 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 				       struct spi_transfer *tfr)
 {
 	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
-	unsigned long spi_hz, clk_hz, speed;
-	unsigned long spi_used_hz;
+	unsigned long spi_hz, clk_hz, speed, spi_used_hz;
+	unsigned long hz_per_byte, byte_limit;

 	/* calculate the registers to handle
 	 *
@@ -401,14 +403,15 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	 * of Hz per byte per polling limit.  E.g., we can transfer 1 byte in
 	 * 30 µs per 300,000 Hz of bus clock.
 	 */
-#define HZ_PER_BYTE ((9 * 1000000) / BCM2835_AUX_SPI_POLLING_LIMIT_US)
+	hz_per_byte = polling_limit_us ? (9 * 1000000) / polling_limit_us : 0;
+	byte_limit = hz_per_byte ? spi_used_hz / hz_per_byte : 1;
+
 	/* run in polling mode for short transfers */
-	if (tfr->len < spi_used_hz / HZ_PER_BYTE)
+	if (tfr->len < byte_limit)
 		return bcm2835aux_spi_transfer_one_poll(master, spi, tfr);

 	/* run in interrupt mode for all others */
 	return bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
-#undef HZ_PER_BYTE
 }

 static int bcm2835aux_spi_prepare_message(struct spi_master *master,
--
2.11.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-24 17:50     ` kernel
@ 2019-03-25  9:28       ` Stefan Wahren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:28 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> To estimate efficiency add statistics on transfer types
> (polling and interrupt) used to debugfs.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index dd0446968da6..d2b58060b333 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -21,6 +21,7 @@
>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> @@ -102,8 +103,57 @@ struct bcm2835aux_spi {
>  	int tx_len;
>  	int rx_len;
>  	int pending;
> +
> +#if defined(CONFIG_DEBUG_FS)
> +#define BCM2835_AUX_INCR(field) ((field)++)
> +	u64 count_transfer_polling;
> +	u64 count_transfer_irq;
> +	u64 count_transfer_irq_after_poll;
> +
> +	struct dentry *debugfs_dir;
> +#else
> +#define BCM2835_AUX_INCR(field)
> +#endif /* CONFIG_DEBUG_FS */
>  };
>
is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
@ 2019-03-25  9:28       ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:28 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> To estimate efficiency add statistics on transfer types
> (polling and interrupt) used to debugfs.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index dd0446968da6..d2b58060b333 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -21,6 +21,7 @@
>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> @@ -102,8 +103,57 @@ struct bcm2835aux_spi {
>  	int tx_len;
>  	int rx_len;
>  	int pending;
> +
> +#if defined(CONFIG_DEBUG_FS)
> +#define BCM2835_AUX_INCR(field) ((field)++)
> +	u64 count_transfer_polling;
> +	u64 count_transfer_irq;
> +	u64 count_transfer_irq_after_poll;
> +
> +	struct dentry *debugfs_dir;
> +#else
> +#define BCM2835_AUX_INCR(field)
> +#endif /* CONFIG_DEBUG_FS */
>  };
>
is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
  2019-03-24 17:50     ` kernel
@ 2019-03-25  9:44       ` Stefan Wahren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:44 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> Under some circumstances the default 30 us polling limit is not optimal
> and may lead to long delays because we are waiting on an interrupt.
> with this patch we have the possibility to influence this policy.
>
> So make this limit (in us) configurable via a module parameters
> (but also modifyable via /sys/modules/...)
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>
> ---
> Changelog:
>   V1 -> V2: remove the dependency on a different patchset focused on
> 	    making cs_change delay configurable
>
> ---
>  drivers/spi/spi-bcm2835aux.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2b58060b333..df065108122b 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -37,6 +37,12 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spinlock.h>
>
> +/* define polling limits */
> +unsigned int polling_limit_us = 30;
> +module_param(polling_limit_us, uint, 0664);
> +MODULE_PARM_DESC(polling_limit_us,
> +		 "time in us to run a transfer in polling mode\n");
> +
could you please document the case polling_limit_us = 0 ?

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
@ 2019-03-25  9:44       ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:44 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> Under some circumstances the default 30 us polling limit is not optimal
> and may lead to long delays because we are waiting on an interrupt.
> with this patch we have the possibility to influence this policy.
>
> So make this limit (in us) configurable via a module parameters
> (but also modifyable via /sys/modules/...)
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>
> ---
> Changelog:
>   V1 -> V2: remove the dependency on a different patchset focused on
> 	    making cs_change delay configurable
>
> ---
>  drivers/spi/spi-bcm2835aux.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2b58060b333..df065108122b 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -37,6 +37,12 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spinlock.h>
>
> +/* define polling limits */
> +unsigned int polling_limit_us = 30;
> +module_param(polling_limit_us, uint, 0664);
> +MODULE_PARM_DESC(polling_limit_us,
> +		 "time in us to run a transfer in polling mode\n");
> +
could you please document the case polling_limit_us = 0 ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
  2019-03-25  9:44       ` Stefan Wahren
@ 2019-03-25  9:49         ` kernel
  -1 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-25  9:49 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel


> On 25.03.2019, at 10:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> Hi Martin,
> 
> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> +/* define polling limits */
>> +unsigned int polling_limit_us = 30;
>> +module_param(polling_limit_us, uint, 0664);
>> +MODULE_PARM_DESC(polling_limit_us,
>> +		 "time in us to run a transfer in polling mode\n");
>> +
> could you please document the case polling_limit_us = 0 ?

Means no polling under any circumstances - I can add it as a comment in V3.

martin

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
@ 2019-03-25  9:49         ` kernel
  0 siblings, 0 replies; 36+ messages in thread
From: kernel @ 2019-03-25  9:49 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel


> On 25.03.2019, at 10:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> Hi Martin,
> 
> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> +/* define polling limits */
>> +unsigned int polling_limit_us = 30;
>> +module_param(polling_limit_us, uint, 0664);
>> +MODULE_PARM_DESC(polling_limit_us,
>> +		 "time in us to run a transfer in polling mode\n");
>> +
> could you please document the case polling_limit_us = 0 ?

Means no polling under any circumstances - I can add it as a comment in V3.

martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-25  9:28       ` Stefan Wahren
  (?)
@ 2019-03-25  9:52       ` kernel
  2019-03-25 10:06         ` Stefan Wahren
  -1 siblings, 1 reply; 36+ messages in thread
From: kernel @ 2019-03-25  9:52 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel


> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> Hi Martin,
> 
> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>> +
>> +#if defined(CONFIG_DEBUG_FS)
>> +#define BCM2835_AUX_INCR(field) ((field)++)
>> +	u64 count_transfer_polling;
>> +	u64 count_transfer_irq;
>> +	u64 count_transfer_irq_after_poll;
>> +
>> +	struct dentry *debugfs_dir;
>> +#else
>> +#define BCM2835_AUX_INCR(field)
>> +#endif /* CONFIG_DEBUG_FS */
>> };
>> 
> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?
> 

The idea was not to add those stats at all when debugfs is disabled.
Other groups (e.g net/can) prefer it this way…

I do not care either way - your preference...

Martin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 3/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
  2019-03-24 17:49   ` kernel
@ 2019-03-25  9:52     ` Stefan Wahren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:52 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

here is my suggestion for a subject:

spi: bcm2835aux: fix corruptions for longer spi transfers

Am 24.03.19 um 18:49 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> On long running tests with a mcp2517fd can controller it showed that
> on rare occations the data read shows corruptions for longer spi transfers.
>
> Example of a 22 byte transfer:
>
> expected (as captured on logic analyzer):
> FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 85 86 87 88 89 8a 8b
>
> read by the driver:
> FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 88 89 8a 00 00 8b 9b
>
> To fix this use BCM2835_AUX_SPI_STAT_RX_LVL to determine when we may
> read data from the fifo reliably without any corruption.
>
> Surprisingly the only values ever empirically read in
> BCM2835_AUX_SPI_STAT_RX_LVL are 0x00, 0x10, 0x20 and 0x30.
> So whenever the mask is not 0 we can read from the fifo in a safe manner.
>
> The patch has now been tested intensively and we are no longer
> able to reproduce the "RX" issue any longer.
>
> Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Please use a proper fixes tages in all of your patches. Doesn't
checkpatch.pl complain about it?

Thanks Stefan

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 3/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL
@ 2019-03-25  9:52     ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:52 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

here is my suggestion for a subject:

spi: bcm2835aux: fix corruptions for longer spi transfers

Am 24.03.19 um 18:49 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> On long running tests with a mcp2517fd can controller it showed that
> on rare occations the data read shows corruptions for longer spi transfers.
>
> Example of a 22 byte transfer:
>
> expected (as captured on logic analyzer):
> FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 85 86 87 88 89 8a 8b
>
> read by the driver:
> FF FF 78 00 00 00 08 06 00 00 91 20 77 56 84 88 89 8a 00 00 8b 9b
>
> To fix this use BCM2835_AUX_SPI_STAT_RX_LVL to determine when we may
> read data from the fifo reliably without any corruption.
>
> Surprisingly the only values ever empirically read in
> BCM2835_AUX_SPI_STAT_RX_LVL are 0x00, 0x10, 0x20 and 0x30.
> So whenever the mask is not 0 we can read from the fifo in a safe manner.
>
> The patch has now been tested intensively and we are no longer
> able to reproduce the "RX" issue any longer.
>
> Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e

Please use a proper fixes tages in all of your patches. Doesn't
checkpatch.pl complain about it?

Thanks Stefan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
  2019-03-25  9:49         ` kernel
@ 2019-03-25  9:59           ` Stefan Wahren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:59 UTC (permalink / raw)
  To: kernel
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel

Am 25.03.19 um 10:49 schrieb kernel@martin.sperl.org:
>> On 25.03.2019, at 10:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Martin,
>>
>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>
>>> +/* define polling limits */
>>> +unsigned int polling_limit_us = 30;
>>> +module_param(polling_limit_us, uint, 0664);
>>> +MODULE_PARM_DESC(polling_limit_us,
>>> +		 "time in us to run a transfer in polling mode\n");
>>> +
>> could you please document the case polling_limit_us = 0 ?
> Means no polling under any circumstances - I can add it as a comment in V3.
I meant the parameter description, because users shouldn't be forced to
look at the source code.
>
> martin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable
@ 2019-03-25  9:59           ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25  9:59 UTC (permalink / raw)
  To: kernel
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel

Am 25.03.19 um 10:49 schrieb kernel@martin.sperl.org:
>> On 25.03.2019, at 10:44, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Martin,
>>
>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>
>>> +/* define polling limits */
>>> +unsigned int polling_limit_us = 30;
>>> +module_param(polling_limit_us, uint, 0664);
>>> +MODULE_PARM_DESC(polling_limit_us,
>>> +		 "time in us to run a transfer in polling mode\n");
>>> +
>> could you please document the case polling_limit_us = 0 ?
> Means no polling under any circumstances - I can add it as a comment in V3.
I meant the parameter description, because users shouldn't be forced to
look at the source code.
>
> martin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-25  9:52       ` kernel
@ 2019-03-25 10:06         ` Stefan Wahren
  2019-03-25 10:11           ` kernel
  0 siblings, 1 reply; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25 10:06 UTC (permalink / raw)
  To: kernel
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel

Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org:
>> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Martin,
>>
>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>>> +
>>> +#if defined(CONFIG_DEBUG_FS)
>>> +#define BCM2835_AUX_INCR(field) ((field)++)
>>> +	u64 count_transfer_polling;
>>> +	u64 count_transfer_irq;
>>> +	u64 count_transfer_irq_after_poll;
>>> +
>>> +	struct dentry *debugfs_dir;
>>> +#else
>>> +#define BCM2835_AUX_INCR(field)
>>> +#endif /* CONFIG_DEBUG_FS */
>>> };
>>>
>> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?
>>
> The idea was not to add those stats at all when debugfs is disabled.
> Other groups (e.g net/can) prefer it this way…

Yes, this is always a balance between size reduction and code readability.

My biggest concern was about BCM2835_AUX_INCR


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-25 10:06         ` Stefan Wahren
@ 2019-03-25 10:11           ` kernel
  2019-03-25 10:45             ` Stefan Wahren
  0 siblings, 1 reply; 36+ messages in thread
From: kernel @ 2019-03-25 10:11 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel


> On 25.03.2019, at 11:06, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org:
>>> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>> 
>>> Hi Martin,
>>> 
>>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>>>> +
>>>> +#if defined(CONFIG_DEBUG_FS)
>>>> +#define BCM2835_AUX_INCR(field) ((field)++)
>>>> +	u64 count_transfer_polling;
>>>> +	u64 count_transfer_irq;
>>>> +	u64 count_transfer_irq_after_poll;
>>>> +
>>>> +	struct dentry *debugfs_dir;
>>>> +#else
>>>> +#define BCM2835_AUX_INCR(field)
>>>> +#endif /* CONFIG_DEBUG_FS */
>>>> };
>>>> 
>>> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?
>>> 
>> The idea was not to add those stats at all when debugfs is disabled.
>> Other groups (e.g net/can) prefer it this way…
> 
> Yes, this is always a balance between size reduction and code readability.
> 
> My biggest concern was about BCM2835_AUX_INCR
> 
just tell me which way you prefer it - anyway: disabling DEBUG is quite hard
to do in the first place (too many components select it automatically)
so I am always wondering why I need to hop thtu those loops.

If you agree I will remove those specific ifdefs and macros.

Martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 6/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported
  2019-03-24 17:49   ` kernel
@ 2019-03-25 10:28     ` Stefan Wahren
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25 10:28 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:49 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> From personal bad experience (even as the author of the original driver)
> it shows that native-cs is "somewhat" supported by the spi bus driver
> when using a buggy device tree.
>
> So make sure that the driver is warning in dmesg about this fact
> that we are running in a not supported mode that may have surprizing
> limitations.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
please use a proper fixes tag.
> ---
>  drivers/spi/spi-bcm2835aux.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index c8acde017b6a..6af07dec5fdc 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -396,6 +396,38 @@ static void bcm2835aux_spi_handle_err(struct spi_master *master,
>  	bcm2835aux_spi_reset_hw(bs);
>  }
>
> +static int bcm2835aux_spi_setup(struct spi_device *spi)
> +{
> +	int ret;
> +
> +	if (spi->mode & SPI_NO_CS)
> +		return 0;
> +	/* sanity check for native cs */
> +	if (!gpio_is_valid(spi->cs_gpio)) {
> +		/* for dt-backwards compatibility: only support native on CS0
> +		 * known things not supported with broken native CS:
> +		 * * multiple chip-selects: cs0-cs2 are all
> +		 *     simultaniously asserted whenever there is a transfer
> +		 *     this even includes SPI_NO_CS
> +		 * * SPI_CS_HIGH: cs are always asserted low
> +		 * * cs_change: cs is deasserted after each spi_transfer
> +		 * * cs_delay_usec: cs is always deasserted one SCK cycle
> +		 *     after the last transfer
> +		 * probably more...
> +		 */
> +		if (spi->chip_select == 0) {
> +			dev_err(&spi->dev,
> +				"native CS is not supported but may work for some use-cases for cs = 0 -  please define a valid value cs-gpios in DT for complete feature set\n");
> +			return 0;

These messages are rather too long, please keep it in the ~ 80 char
limit (in source code and dmesg).
Or make it two lines ...

Since we return with 0, i prefer to use dev_warn instead of dev_err.

Thanks

> +		}
> +		dev_err(&spi->dev,
> +			"native CS is not supported for cs > 0 -  please define a valid value cs-gpios in DT to enable multiple cs\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 6/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported
@ 2019-03-25 10:28     ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25 10:28 UTC (permalink / raw)
  To: kernel, Mark Brown, Eric Anholt, Hubert Denkmair, linux-spi,
	linux-rpi-kernel, linux-arm-kernel

Hi Martin,

Am 24.03.19 um 18:49 schrieb kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> From personal bad experience (even as the author of the original driver)
> it shows that native-cs is "somewhat" supported by the spi bus driver
> when using a buggy device tree.
>
> So make sure that the driver is warning in dmesg about this fact
> that we are running in a not supported mode that may have surprizing
> limitations.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
please use a proper fixes tag.
> ---
>  drivers/spi/spi-bcm2835aux.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index c8acde017b6a..6af07dec5fdc 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -396,6 +396,38 @@ static void bcm2835aux_spi_handle_err(struct spi_master *master,
>  	bcm2835aux_spi_reset_hw(bs);
>  }
>
> +static int bcm2835aux_spi_setup(struct spi_device *spi)
> +{
> +	int ret;
> +
> +	if (spi->mode & SPI_NO_CS)
> +		return 0;
> +	/* sanity check for native cs */
> +	if (!gpio_is_valid(spi->cs_gpio)) {
> +		/* for dt-backwards compatibility: only support native on CS0
> +		 * known things not supported with broken native CS:
> +		 * * multiple chip-selects: cs0-cs2 are all
> +		 *     simultaniously asserted whenever there is a transfer
> +		 *     this even includes SPI_NO_CS
> +		 * * SPI_CS_HIGH: cs are always asserted low
> +		 * * cs_change: cs is deasserted after each spi_transfer
> +		 * * cs_delay_usec: cs is always deasserted one SCK cycle
> +		 *     after the last transfer
> +		 * probably more...
> +		 */
> +		if (spi->chip_select == 0) {
> +			dev_err(&spi->dev,
> +				"native CS is not supported but may work for some use-cases for cs = 0 -  please define a valid value cs-gpios in DT for complete feature set\n");
> +			return 0;

These messages are rather too long, please keep it in the ~ 80 char
limit (in source code and dmesg).
Or make it two lines ...

Since we return with 0, i prefer to use dev_warn instead of dev_err.

Thanks

> +		}
> +		dev_err(&spi->dev,
> +			"native CS is not supported for cs > 0 -  please define a valid value cs-gpios in DT to enable multiple cs\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs
  2019-03-25 10:11           ` kernel
@ 2019-03-25 10:45             ` Stefan Wahren
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Wahren @ 2019-03-25 10:45 UTC (permalink / raw)
  To: kernel
  Cc: linux-spi, Eric Anholt, Mark Brown, linux-rpi-kernel,
	Hubert Denkmair, linux-arm-kernel

Am 25.03.19 um 11:11 schrieb kernel@martin.sperl.org:
>> On 25.03.2019, at 11:06, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org:
>>>> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>>>
>>>> Hi Martin,
>>>>
>>>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org:
>>>>> +
>>>>> +#if defined(CONFIG_DEBUG_FS)
>>>>> +#define BCM2835_AUX_INCR(field) ((field)++)
>>>>> +	u64 count_transfer_polling;
>>>>> +	u64 count_transfer_irq;
>>>>> +	u64 count_transfer_irq_after_poll;
>>>>> +
>>>>> +	struct dentry *debugfs_dir;
>>>>> +#else
>>>>> +#define BCM2835_AUX_INCR(field)
>>>>> +#endif /* CONFIG_DEBUG_FS */
>>>>> };
>>>>>
>>>> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?
>>>>
>>> The idea was not to add those stats at all when debugfs is disabled.
>>> Other groups (e.g net/can) prefer it this way…
>> Yes, this is always a balance between size reduction and code readability.
>>
>> My biggest concern was about BCM2835_AUX_INCR
>>
> just tell me which way you prefer it - anyway: disabling DEBUG is quite hard
> to do in the first place (too many components select it automatically)
> so I am always wondering why I need to hop thtu those loops.
>
> If you agree I will remove those specific ifdefs and macros.
Please remove it for the quoted part above

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 36+ messages in thread
end of thread, other threads:[~2019-03-25 10:45 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-24 17:49 [PATCH V2 0/9] spi: bcm2835aux: bug fixes and improvements kernel
2019-03-24 17:49 ` kernel
2019-03-24 17:49 ` [PATCH V2 3/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL kernel
2019-03-24 17:49   ` kernel
2019-03-25  9:52   ` Stefan Wahren
2019-03-25  9:52     ` Stefan Wahren
2019-03-24 17:49 ` [PATCH V2 6/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported kernel
2019-03-24 17:49   ` kernel
2019-03-25 10:28   ` Stefan Wahren
2019-03-25 10:28     ` Stefan Wahren
     [not found] ` <20190324175002.28969-1-kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
2019-03-24 17:49   ` [PATCH V2 1/9] spi: bcm2835aux: unifying code between polling and interrupt driven code kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:49     ` kernel
2019-03-24 17:49   ` [PATCH V2 2/9] spi: bcm2835aux: remove dangerous uncontrolled read of fifo kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:49     ` kernel
2019-03-24 17:49   ` [PATCH V2 4/9] spi: bcm2835aux: remove dead code kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:49     ` kernel
2019-03-24 17:49   ` [PATCH V2 5/9] spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:49     ` kernel
2019-03-24 17:50   ` [PATCH V2 7/9] spi: bcm2835aux: setup gpio-cs to output and correct level during setup kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:50     ` kernel
2019-03-24 17:50   ` [PATCH V2 8/9] spi: bcm2835aux: add driver stats to debugfs kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:50     ` kernel
2019-03-25  9:28     ` Stefan Wahren
2019-03-25  9:28       ` Stefan Wahren
2019-03-25  9:52       ` kernel
2019-03-25 10:06         ` Stefan Wahren
2019-03-25 10:11           ` kernel
2019-03-25 10:45             ` Stefan Wahren
2019-03-24 17:50   ` [PATCH V2 9/9] spi: bcm2835aux: make the polling duration limits configurable kernel-TqfNSX0MhmxHKSADF0wUEw
2019-03-24 17:50     ` kernel
2019-03-25  9:44     ` Stefan Wahren
2019-03-25  9:44       ` Stefan Wahren
2019-03-25  9:49       ` kernel
2019-03-25  9:49         ` kernel
2019-03-25  9:59         ` Stefan Wahren
2019-03-25  9:59           ` Stefan Wahren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.