* [PATCH 1/3] spi: bcm2835: add driver stats to debugfs
@ 2019-02-24 16:23 kernel
2019-02-24 16:23 ` [PATCH 2/3] spi: bcm2835: avoid 64 bit arithmetic and allow to configure polling limit kernel
2019-02-24 16:23 ` [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma kernel
0 siblings, 2 replies; 7+ messages in thread
From: kernel @ 2019-02-24 16:23 UTC (permalink / raw)
To: Mark Brown, Eric Anholt, Stefan Wahren, 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, interrupt and dma) used to debugfs.
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
drivers/spi/spi-bcm2835.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index c2912c1e09c5..0ccc6a2733c6 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -22,6 +22,7 @@
#include <linux/clk.h>
#include <linux/completion.h>
+#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
@@ -111,8 +112,60 @@ struct bcm2835_spi {
int rx_prologue;
unsigned int tx_spillover;
unsigned int dma_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;
+ u64 count_transfer_dma;
+
+ struct dentry *debugfs_dir;
+#else
+#define BCM2835_AUX_INCR(field)
+#endif /* CONFIG_DEBUG_FS */
};
+#if defined(CONFIG_DEBUG_FS)
+static void bcm2835_debugfs_create(struct bcm2835_spi *bs,
+ const char *dname)
+{
+ char name[64];
+ struct dentry *dir;
+
+ /* get full name */
+ snprintf(name, sizeof(name), "spi-bcm2835-%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);
+ debugfs_create_u64("count_transfer_dma", 0444, dir,
+ &bs->count_transfer_dma);
+}
+
+static void bcm2835_debugfs_remove(struct bcm2835_spi *bs)
+{
+ debugfs_remove_recursive(bs->debugfs_dir);
+ bs->debugfs_dir = NULL;
+}
+#else
+static void bcm2835_debugfs_create(struct bcm2835_spi *bs)
+{
+}
+
+static void bcm2835_debugfs_remove(struct bcm2835_spi *bs)
+{
+}
+#endif /* CONFIG_DEBUG_FS */
+
static inline u32 bcm2835_rd(struct bcm2835_spi *bs, unsigned reg)
{
return readl(bs->regs + reg);
@@ -316,6 +369,9 @@ static int bcm2835_spi_transfer_one_irq(struct spi_master *master,
{
struct bcm2835_spi *bs = spi_master_get_devdata(master);
+ /* update statistics */
+ BCM2835_AUX_INCR(bs->count_transfer_irq);
+
/*
* Enable HW block, but with interrupts still disabled.
* Otherwise the empty TX FIFO would immediately trigger an interrupt.
@@ -574,6 +630,9 @@ static int bcm2835_spi_transfer_one_dma(struct spi_master *master,
struct bcm2835_spi *bs = spi_master_get_devdata(master);
int ret;
+ /* update statistics */
+ BCM2835_AUX_INCR(bs->count_transfer_dma);
+
/*
* Transfer first few bytes without DMA if length of first TX or RX
* sglist entry is not a multiple of 4 bytes (hardware limitation).
@@ -731,6 +790,9 @@ static int bcm2835_spi_transfer_one_poll(struct spi_master *master,
struct bcm2835_spi *bs = spi_master_get_devdata(master);
unsigned long timeout;
+ /* update statistics */
+ BCM2835_AUX_INCR(bs->count_transfer_polling);
+
/* enable HW block without interrupts */
bcm2835_wr(bs, BCM2835_SPI_CS, cs | BCM2835_SPI_CS_TA);
@@ -759,6 +821,8 @@ static int bcm2835_spi_transfer_one_poll(struct spi_master *master,
"timeout period reached: jiffies: %lu remaining tx/rx: %d/%d - falling back to interrupt mode\n",
jiffies - timeout,
bs->tx_len, bs->rx_len);
+ /* update statistics */
+ BCM2835_AUX_INCR(bs->count_transfer_irq_after_poll);
/* fall back to interrupt mode */
return bcm2835_spi_transfer_one_irq(master, spi,
tfr, cs, false);
@@ -991,6 +1055,8 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
goto out_clk_disable;
}
+ bcm2835_debugfs_create(bs, dev_name(&pdev->dev));
+
return 0;
out_clk_disable:
@@ -1005,6 +1071,8 @@ static int bcm2835_spi_remove(struct platform_device *pdev)
struct spi_master *master = platform_get_drvdata(pdev);
struct bcm2835_spi *bs = spi_master_get_devdata(master);
+ bcm2835_debugfs_remove(bs);
+
/* Clear FIFOs, and disable the HW block */
bcm2835_wr(bs, BCM2835_SPI_CS,
BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
--
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] 7+ messages in thread* [PATCH 2/3] spi: bcm2835: avoid 64 bit arithmetic and allow to configure polling limit
2019-02-24 16:23 [PATCH 1/3] spi: bcm2835: add driver stats to debugfs kernel
@ 2019-02-24 16:23 ` kernel
2019-02-24 16:23 ` [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma kernel
1 sibling, 0 replies; 7+ messages in thread
From: kernel @ 2019-02-24 16:23 UTC (permalink / raw)
To: Mark Brown, Eric Anholt, Stefan Wahren, linux-spi,
linux-rpi-kernel, linux-arm-kernel
Cc: Martin Sperl
From: Martin Sperl <kernel@martin.sperl.org>
In analogy to commit d704afffe65c ("spi: bcm2835aux: Avoid 64-bit
arithmetic in xfer len calc") avoid 64 bit calculations and make
the polling interval configurable as a module parameter.
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
Note this patch requires the effective_speed_hz patch to be included
---
drivers/spi/spi-bcm2835.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 0ccc6a2733c6..dcf922ca2603 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -38,6 +38,12 @@
#include <linux/of_irq.h>
#include <linux/spi/spi.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 offsets */
#define BCM2835_SPI_CS 0x00
#define BCM2835_SPI_FIFO 0x04
@@ -74,8 +80,6 @@
#define BCM2835_SPI_FIFO_SIZE 64
#define BCM2835_SPI_FIFO_SIZE_3_4 48
-#define BCM2835_SPI_POLLING_LIMIT_US 30
-#define BCM2835_SPI_POLLING_JIFFIES 2
#define BCM2835_SPI_DMA_MIN_LENGTH 96
#define BCM2835_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \
| SPI_NO_CS | SPI_3WIRE)
@@ -784,8 +788,7 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
static int bcm2835_spi_transfer_one_poll(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr,
- u32 cs,
- unsigned long long xfer_time_us)
+ u32 cs)
{
struct bcm2835_spi *bs = spi_master_get_devdata(master);
unsigned long timeout;
@@ -802,8 +805,8 @@ static int bcm2835_spi_transfer_one_poll(struct spi_master *master,
*/
bcm2835_wr_fifo_blind(bs, BCM2835_SPI_FIFO_SIZE);
- /* set the timeout */
- timeout = jiffies + BCM2835_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) {
@@ -840,8 +843,7 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
struct spi_transfer *tfr)
{
struct bcm2835_spi *bs = spi_master_get_devdata(master);
- unsigned long spi_hz, clk_hz, cdiv;
- unsigned long long xfer_time_us;
+ unsigned long spi_hz, clk_hz, cdiv, hz_per_byte, byte_limit;
u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
/* set clock */
@@ -882,16 +884,18 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
bs->tx_len = tfr->len;
bs->rx_len = tfr->len;
- /* calculate the estimated time in us the transfer runs */
- xfer_time_us = (unsigned long long)tfr->len
- * 9 /* clocks/byte - SPI-HW waits 1 clock after each byte */
- * 1000000;
- do_div(xfer_time_us, tfr->effective_speed_hz);
+ /* Calculate the estimated time in us the transfer runs. Note that
+ * there is 1 idle clocks cycles after each byte transferredo - so
+ * 9 cycles/byte. This is used to find the number
+ * of Hz per byte per polling limit. E.g., we can transfer 1 byte in
+ * 30 us per 300,000 Hz of bus clock.
+ */
+ 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;
/* for short requests run polling*/
- if (xfer_time_us <= BCM2835_SPI_POLLING_LIMIT_US)
- return bcm2835_spi_transfer_one_poll(master, spi, tfr,
- cs, xfer_time_us);
+ if (tfr->len < byte_limit)
+ return bcm2835_spi_transfer_one_poll(master, spi, tfr, cs);
/* run in dma mode if conditions are right */
if (master->can_dma && bcm2835_spi_can_dma(master, spi, tfr))
--
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] 7+ messages in thread* [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma
2019-02-24 16:23 [PATCH 1/3] spi: bcm2835: add driver stats to debugfs kernel
2019-02-24 16:23 ` [PATCH 2/3] spi: bcm2835: avoid 64 bit arithmetic and allow to configure polling limit kernel
@ 2019-02-24 16:23 ` kernel
2019-02-24 19:10 ` Stefan Wahren
[not found] ` <20190322123610.ndwxoivsprc643f5@wunner.de>
1 sibling, 2 replies; 7+ messages in thread
From: kernel @ 2019-02-24 16:23 UTC (permalink / raw)
To: Mark Brown, Eric Anholt, Stefan Wahren, linux-spi,
linux-rpi-kernel, linux-arm-kernel
Cc: Martin Sperl
From: Martin Sperl <kernel@martin.sperl.org>
Allow setting the length of the transfer at which dma is used by
setting a module parameter.
Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
drivers/spi/spi-bcm2835.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index dcf922ca2603..ef4b3468a97d 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -31,6 +31,7 @@
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/moduleparam.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
@@ -44,6 +45,39 @@ module_param(polling_limit_us, uint, 0664);
MODULE_PARM_DESC(polling_limit_us,
"time in us to run a transfer in polling mode\n");
+/* define dma min number of bytes to use in dma mode with value validation */
+static int dma_min_bytes_limit_set(const char *val,
+ const struct kernel_param *kp)
+{
+ unsigned int v;
+
+ if (kstrtouint(val, 10, &v))
+ return -EINVAL;
+ /* value needs to be a multiple of 4 */
+ if (v % 4) {
+ pr_err("dma_min_bytes_limit needs to be a multiple of 4\n");
+ return -EINVAL;
+ }
+ /* value needs to be at least 6 - so actually 8 - rational below */
+ if (v < 6) {
+ pr_err("dma_min_bytes_limit needs to be at least 8\n");
+ return -EINVAL;
+ }
+
+ return param_set_uint(val, kp);
+}
+
+static const struct kernel_param_ops dma_min_bytes_limit_ops = {
+ .set = dma_min_bytes_limit_set,
+ .get = param_get_int,
+};
+
+unsigned int dma_min_bytes_limit = 96;
+module_param_cb(dma_min_bytes_limit, &dma_min_bytes_limit_ops,
+ &dma_min_bytes_limit, 0664);
+MODULE_PARM_DESC(dma_min_bytes_limit,
+ "minimum number of bytes to run a transfer in dma mode\n");
+
/* SPI register offsets */
#define BCM2835_SPI_CS 0x00
#define BCM2835_SPI_FIFO 0x04
@@ -80,7 +114,6 @@ MODULE_PARM_DESC(polling_limit_us,
#define BCM2835_SPI_FIFO_SIZE 64
#define BCM2835_SPI_FIFO_SIZE_3_4 48
-#define BCM2835_SPI_DMA_MIN_LENGTH 96
#define BCM2835_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \
| SPI_NO_CS | SPI_3WIRE)
@@ -447,7 +480,8 @@ static int bcm2835_spi_transfer_one_irq(struct spi_master *master,
* if the length of the first is *exactly* 1.
*
* At most 6 bytes are written and at most 3 bytes read. Do we know the
- * transfer has this many bytes? Yes, see BCM2835_SPI_DMA_MIN_LENGTH.
+ * transfer has this many bytes? Yes, see validation in
+ * dma_min_bytes_limit_set.
*
* The FIFO is normally accessed with 8-bit width by the CPU and 32-bit width
* by the DMA engine. Toggling the DMA Enable flag in the CS register switches
@@ -690,7 +724,7 @@ static bool bcm2835_spi_can_dma(struct spi_master *master,
struct spi_transfer *tfr)
{
/* we start DMA efforts only on bigger transfers */
- if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
+ if (tfr->len < dma_min_bytes_limit)
return false;
/* BCM2835_SPI_DLEN has defined a max transfer size as
--
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] 7+ messages in thread* Re: [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma
2019-02-24 16:23 ` [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma kernel
@ 2019-02-24 19:10 ` Stefan Wahren
2019-03-24 8:52 ` kernel
[not found] ` <20190322123610.ndwxoivsprc643f5@wunner.de>
1 sibling, 1 reply; 7+ messages in thread
From: Stefan Wahren @ 2019-02-24 19:10 UTC (permalink / raw)
To: kernel
Cc: Eric Anholt, Mark Brown, linux-rpi-kernel, linux-arm-kernel,
linux-spi
Hi Martin,
> kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben:
>
>
> From: Martin Sperl <kernel@martin.sperl.org>
>
> Allow setting the length of the transfer at which dma is used by
> setting a module parameter.
please provide the motivation of this change.
Regards
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] 7+ messages in thread* Re: [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma
2019-02-24 19:10 ` Stefan Wahren
@ 2019-03-24 8:52 ` kernel
[not found] ` <20190324101552.brc7ojrfrsgyr77d@wunner.de>
0 siblings, 1 reply; 7+ messages in thread
From: kernel @ 2019-03-24 8:52 UTC (permalink / raw)
To: Stefan Wahren
Cc: Eric Anholt, Mark Brown, linux-rpi-kernel, linux-arm-kernel,
linux-spi
> On 24.02.2019, at 20:10, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Martin,
>
>> kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben:
>>
>>
>> From: Martin Sperl <kernel@martin.sperl.org>
>>
>> Allow setting the length of the transfer at which dma is used by
>> setting a module parameter.
>
> please provide the motivation of this change.
As we provide control over the selection of polling vs. interrupt mode
we should - for consistency - also provide control over selection of
DMA mode.
DMA mapping is quite expensive and with higher SPI clock speeds it
may be more economic CPU wise to run in polling mode instead of DMA
mode.
Also DMA mode has one specific difference to Polling mode:
there is no idle clock cycle between bytes transferred.
This may have negative impact when transferring lots of bytes to
some mcus without SPI buffers at the fastest possible clock speed,
where it helps when there is a gap after each byte.
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] 7+ messages in thread
[parent not found: <20190322123610.ndwxoivsprc643f5@wunner.de>]
* Re: [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma
[not found] ` <20190322123610.ndwxoivsprc643f5@wunner.de>
@ 2019-03-24 8:58 ` kernel
0 siblings, 0 replies; 7+ messages in thread
From: kernel @ 2019-03-24 8:58 UTC (permalink / raw)
To: Lukas Wunner
Cc: Stefan Wahren, linux-spi, Eric Anholt, Mark Brown,
linux-rpi-kernel, linux-arm-kernel
> On 22.03.2019, at 13:36, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Sun, Feb 24, 2019 at 04:23:11PM +0000, kernel@martin.sperl.org wrote:
>> +/* define dma min number of bytes to use in dma mode with value validation */
>> +static int dma_min_bytes_limit_set(const char *val,
>> + const struct kernel_param *kp)
>> +{
>> + unsigned int v;
>> +
>> + if (kstrtouint(val, 10, &v))
>> + return -EINVAL;
>> + /* value needs to be a multiple of 4 */
>> + if (v % 4) {
>> + pr_err("dma_min_bytes_limit needs to be a multiple of 4\n");
>> + return -EINVAL;
>> + }
>
> Transfers don't need to be a multiple of 4 to be eligible for DMA,
> so this check can be dropped.
I definitely did not want to write a custom module argument parser
but if i remember correctly there is one limitation on the transmission path
where you would hit some inefficiencies in the DMA code when you run
transfers that are not a multiple of 4 - especially for short transfers.
That is why it was implemented.
But I can leave this precaution out.
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] 7+ messages in thread
end of thread, other threads:[~2019-03-24 11:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-24 16:23 [PATCH 1/3] spi: bcm2835: add driver stats to debugfs kernel
2019-02-24 16:23 ` [PATCH 2/3] spi: bcm2835: avoid 64 bit arithmetic and allow to configure polling limit kernel
2019-02-24 16:23 ` [PATCH 3/3] spi: bcm2835: add module parameter to configure minimum length for dma kernel
2019-02-24 19:10 ` Stefan Wahren
2019-03-24 8:52 ` kernel
[not found] ` <20190324101552.brc7ojrfrsgyr77d@wunner.de>
2019-03-24 11:23 ` kernel
[not found] ` <20190322123610.ndwxoivsprc643f5@wunner.de>
2019-03-24 8:58 ` kernel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox