[PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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

Finally we remove some dead code.

Martin Sperl (9):
  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 specific stats to debugfs
  spi: bcm2835aux: make the polling duration limits configurable
  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

 drivers/spi/spi-bcm2835aux.c | 205 +++++++++++++++++++++++++++++++------------
 1 file changed, 149 insertions(+), 56 deletions(-)

--
2.11.0

[PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[kernel]

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

Finally we remove some dead code.

Martin Sperl (9):
  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 specific stats to debugfs
  spi: bcm2835aux: make the polling duration limits configurable
  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

 drivers/spi/spi-bcm2835aux.c | 205 +++++++++++++++++++++++++++++++------------
 1 file changed, 149 insertions(+), 56 deletions(-)

--
2.11.0

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

[PATCH 1/9] spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios

[kernel]

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 51ce76706073..2ece25250d86 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -437,7 +437,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

[PATCH 1/9] spi: bcm2835aux: fix driver to not allow 65535 (=-1) cs-gpios

[kernel]

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 51ce76706073..2ece25250d86 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -437,7 +437,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

[PATCH 2/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 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 2ece25250d86..4b6fdcb2cf4b 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -420,6 +420,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;
@@ -449,6 +481,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

[PATCH 2/9] spi: bcm2835aux: warn in dmesg that native cs is not really supported

[kernel]

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.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 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 2ece25250d86..4b6fdcb2cf4b 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -420,6 +420,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;
@@ -449,6 +481,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

[PATCH 3/9] spi: bcm2835aux: setup gpio-cs to output and correct level during setup

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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 4b6fdcb2cf4b..922fd798508e 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -449,7 +449,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

[PATCH 3/9] spi: bcm2835aux: setup gpio-cs to output and correct level during setup

[kernel]

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 4b6fdcb2cf4b..922fd798508e 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -449,7 +449,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

[PATCH 4/9] spi: bcm2835aux: add driver specific stats to debugfs

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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

To estimate efficiency add statistics on transfer types
(polling and interrupt) used via 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 922fd798508e..0a5f21eb3b99 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);
@@ -251,6 +301,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]);
@@ -275,6 +328,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	unsigned long timeout;
 	u32 stat;

+	/* 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]);
@@ -310,6 +366,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);
 		}
@@ -555,6 +612,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:
@@ -569,6 +628,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

[PATCH 4/9] spi: bcm2835aux: add driver specific stats to debugfs

[kernel]

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

To estimate efficiency add statistics on transfer types
(polling and interrupt) used via 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 922fd798508e..0a5f21eb3b99 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);
@@ -251,6 +301,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]);
@@ -275,6 +328,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 	unsigned long timeout;
 	u32 stat;

+	/* 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]);
@@ -310,6 +366,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);
 		}
@@ -555,6 +612,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:
@@ -569,6 +628,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

[PATCH 5/9] spi: bcm2835aux: make the polling duration limits configurable

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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 the interrupt
handler resulting in poor spi bus utilization.

With this patch we have the possibility to influence this policy
by making this limit configurable via a module parameters

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
Note that this patch relies on the effective_speed_hz patch for the
driver to be applied!

---
 drivers/spi/spi-bcm2835aux.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 0a5f21eb3b99..af668cb6b38b 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;
@@ -335,8 +337,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) {
@@ -381,7 +383,7 @@ 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_hz, clk_hz, speed, hz_per_byte, byte_limit;

 	/* calculate the registers to handle
 	 *
@@ -425,14 +427,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 ? tfr->effective_speed_hz / hz_per_byte : 1;
+
 	/* run in polling mode for short transfers */
-	if (tfr->len < tfr->effective_speed_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

[PATCH 5/9] spi: bcm2835aux: make the polling duration limits configurable

[kernel]

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 the interrupt
handler resulting in poor spi bus utilization.

With this patch we have the possibility to influence this policy
by making this limit configurable via a module parameters

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
Note that this patch relies on the effective_speed_hz patch for the
driver to be applied!

---
 drivers/spi/spi-bcm2835aux.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 0a5f21eb3b99..af668cb6b38b 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;
@@ -335,8 +337,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) {
@@ -381,7 +383,7 @@ 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_hz, clk_hz, speed, hz_per_byte, byte_limit;

 	/* calculate the registers to handle
 	 *
@@ -425,14 +427,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 ? tfr->effective_speed_hz / hz_per_byte : 1;
+
 	/* run in polling mode for short transfers */
-	if (tfr->len < tfr->effective_speed_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

[PATCH 6/9] spi: bcm2835aux: unifying code between polling and interrupt driven code

[kernel]

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

Sharing 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 af668cb6b38b..9552f5343982 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -230,23 +230,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 */
@@ -255,7 +245,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" */
@@ -263,8 +252,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 */
@@ -278,8 +280,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,
@@ -328,7 +329,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;

 	/* update statistics */
 	BCM2835_AUX_INCR(bs->count_transfer_polling);
@@ -342,24 +342,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

[PATCH 6/9] spi: bcm2835aux: unifying code between polling and interrupt driven code

[kernel]

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

Sharing 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 af668cb6b38b..9552f5343982 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -230,23 +230,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 */
@@ -255,7 +245,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" */
@@ -263,8 +252,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 */
@@ -278,8 +280,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,
@@ -328,7 +329,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;

 	/* update statistics */
 	BCM2835_AUX_INCR(bs->count_transfer_polling);
@@ -342,24 +342,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

[PATCH 7/9] spi: bcm2835aux: remove dangerous uncontrolled read of fifo

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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 - an additional loop will read the fifo
correctly.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
(Note: requires Patch 6)

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 9552f5343982..8821fc183ca0 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -246,13 +246,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

[PATCH 7/9] spi: bcm2835aux: remove dangerous uncontrolled read of fifo

[kernel]

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 - an additional loop will read the fifo
correctly.

Fixes: 1ea29b39f4c812ece2f936065a0a3d6fe44a263e
(Note: requires Patch 6)

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 9552f5343982..8821fc183ca0 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -246,13 +246,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

[PATCH 8/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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
(Note: requires Patch 6)

Reported-by: Hubert Denkmair <h.denkmair-4pT7WzY1KUiELgA04lAiVw@public.gmane.org>
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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 8821fc183ca0..8cd273d0fef2 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -232,12 +232,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

[PATCH 8/9] spi: bcm2835aux: use BCM2835_AUX_SPI_STAT_RX_LVL

[kernel]

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
(Note: requires Patch 6)

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 8821fc183ca0..8cd273d0fef2 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -232,12 +232,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

[PATCH 9/9] spi: bcm2835aux: remove dead code

[kernel-TqfNSX0MhmxHKSADF0wUEw]

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 8cd273d0fef2..109eba6308cc 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -175,9 +175,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

[PATCH 9/9] spi: bcm2835aux: remove dead code

[kernel]

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 8cd273d0fef2..109eba6308cc 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -175,9 +175,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

Re: [PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[Stefan Wahren]

Hi Martin,

> kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org hat am 24. Februar 2019 um 13:54 geschrieben:
> 
> 
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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
> 
> Finally we remove some dead code.
> 
> Martin Sperl (9):
>   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 specific stats to debugfs
>   spi: bcm2835aux: make the polling duration limits configurable
>   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
> 

i consider patch 7 and 8 as possible stable candidates. How about rearranging this series that these patches comes first and could be backported?

Re: [PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[Stefan Wahren]

Hi Martin,

> kernel@martin.sperl.org hat am 24. Februar 2019 um 13:54 geschrieben:
> 
> 
> 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
> 
> Finally we remove some dead code.
> 
> Martin Sperl (9):
>   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 specific stats to debugfs
>   spi: bcm2835aux: make the polling duration limits configurable
>   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
> 

i consider patch 7 and 8 as possible stable candidates. How about rearranging this series that these patches comes first and could be backported?

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

Re: [PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[kernel]

> On 24.02.2019, at 20:22, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> 
>> Finally we remove some dead code.
>> 
>> Martin Sperl (9):
>>  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 specific stats to debugfs
>>  spi: bcm2835aux: make the polling duration limits configurable
>>  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
>> 
> 
> i consider patch 7 and 8 as possible stable candidates. How about rearranging this series that these patches comes first and could be backported?

Well - as far as I know 6+7+8 can get applied independently 
(and you will need 6 as otherwise you would need to apply
at least 8 in 2 places).

The one observation that I have made is that when applying those
against 4.14 you needed some more patches that have gone in since
4.14 to apply cleanly.

Martin

Re: [PATCH 0/9] spi: bcm2835aux: bug fixes and improvements

[kernel]

> On 24.02.2019, at 20:22, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> 
>> Finally we remove some dead code.
>> 
>> Martin Sperl (9):
>>  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 specific stats to debugfs
>>  spi: bcm2835aux: make the polling duration limits configurable
>>  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
>> 
> 
> i consider patch 7 and 8 as possible stable candidates. How about rearranging this series that these patches comes first and could be backported?

Well - as far as I know 6+7+8 can get applied independently 
(and you will need 6 as otherwise you would need to apply
at least 8 in 2 places).

The one observation that I have made is that when applying those
against 4.14 you needed some more patches that have gone in since
4.14 to apply cleanly.

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