[PATCH] (re-send) spi-imx: improve timing of bursts

[PATCH] (re-send) spi-imx: improve timing of bursts

[Daniel Sobe]

Hi,

The patch below addresses 2 improvements:

1) Burstsize of a transfer adjusted up to the maximum burstsize of the  hardware --> no unwanted CS toggling between data words
2) Variable trigger level allows to increase priority for data transfer,  enables continuous transfers up to the burstsize. --> remove gaps  every 64 words

The original behaviour of the driver is untouched. The additional features can be enabled via sysfs.

With file "bigburst" the driver is instructed to use the maximum  possible burst length, depending on the length of the transfer it  received. Due to the hardware of the iMX6 this only makes sense if a  transfer has its bits per word set to 32.

With file "txthres" the transmit FIFO level is set (in the range of 1 to  64), all other values disable the functionality. An interrupt will be  triggered if the specified FIFO level (or lower) is reached, re-filling  the FIFO up to the max resp. the remaining data of the transfer. If disabled, the original behaviour (refilling  when FIFO is empty) is active. The functionality creates a higher  interrupt load but allows continuous transfers up to the selected  burstsize.

I'd like to receive feedback on the patch, improving it and making it  suitable for a possible inclusion into the kernel. Unfortunately, I  could only test the patch on a very old kernel. The patch itself applies  against a recent kernel (3.14-rc6) and I checked that it compiles without warnings.

Please let me also know if there's something formally wrong with this patch.

Signed-off-by: Daniel Sobe <daniel.sobe-3arQi8VN3Tc@public.gmane.org>

--- linux-original/drivers/spi/spi-imx.c    2014-03-17 17:12:35.691791218 +0100
+++ linux-stable/drivers/spi/spi-imx.c    2014-03-17 17:09:44.451795597 +0100
@@ -51,12 +51,14 @@
 /* generic defines to abstract from the different register layouts */
 #define MXC_INT_RR    (1 << 0) /* Receive data ready interrupt */
 #define MXC_INT_TE    (1 << 1) /* Transmit FIFO empty interrupt */
+#define MXC_INT_THE     (1 << 2) /* Transmit FIFO threshold interrupt */
 
 struct spi_imx_config {
     unsigned int speed_hz;
     unsigned int bpw;
     unsigned int mode;
     u8 cs;
+    unsigned long burstlen;
 };
 
 enum spi_imx_devtype {
@@ -154,6 +156,10 @@
 static int mxc_clkdivs[] = {0, 3, 4, 6, 8, 12, 16, 24, 32, 48, 64, 96, 128, 192,
     256, 384, 512, 768, 1024};
 
+// (sysfs-)tunable attributes
+static int allow_big_bursts = 0; // if set to 1 this enables 32-bit big bursts
+static int tx_fifo_thres = -1;   // if positive, use TX data request interrupt when applicable
+
 /* MX21, MX27 */
 static unsigned int spi_imx_clkdiv_1(unsigned int fin,
         unsigned int fspi, unsigned int max)
@@ -200,8 +206,13 @@
 
 #define MX51_ECSPI_INT        0x10
 #define MX51_ECSPI_INT_TEEN        (1 <<  0)
+#define MX51_ECSPI_INT_TDREN        (1 <<  1)
 #define MX51_ECSPI_INT_RREN        (1 <<  3)
 
+#define MX51_ECSPI_DMA      0x14
+#define MX51_ECSPI_DMA_TX_THRES_OFFSET  0
+#define MX51_ECSPI_DMA_TX_THRES_MASK    (0x3F)
+
 #define MX51_ECSPI_STAT        0x18
 #define MX51_ECSPI_STAT_RR        (1 <<  3)
 
@@ -250,6 +261,12 @@
     if (enable & MXC_INT_TE)
         val |= MX51_ECSPI_INT_TEEN;
 
+    // enable Tx threshold interrupt for faster reaction when transmitting long messages
+    if (enable & MXC_INT_THE) {
+        val |= MX51_ECSPI_INT_TDREN;
+        writel((unsigned int) tx_fifo_thres, spi_imx->base + MX51_ECSPI_DMA);
+    }
+
     if (enable & MXC_INT_RR)
         val |= MX51_ECSPI_INT_RREN;
 
@@ -270,6 +287,7 @@
 {
     u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0;
     u32 clk = config->speed_hz, delay;
+    unsigned long burstlen;
 
     /*
      * The hardware seems to have a race condition when changing modes. The
@@ -286,6 +304,27 @@
     /* set chip select to use */
     ctrl |= MX51_ECSPI_CTRL_CS(config->cs);
 
+    // if possible and requested, increase burstlength
+    // over whole message in 32 bit mode
+    // (which is the only mode where HW supports this)
+    // all other modes still use burstlength == bpw
+    if ((config->bpw == 32) && (allow_big_bursts == 1)) {
+        if (config->burstlen <= 4096) {
+            burstlen = config->burstlen;
+        } else {
+            // burst longer than burst register width -- need to scale appropriately
+            // so that the hardware will always terminate the burst appropriately
+            burstlen = 16;
+            do {
+                burstlen = burstlen << 1;
+            } while ((config->burstlen % (burstlen << 1)) == 0);
+        }
+    } else {
+      burstlen = config->bpw;
+    }
+
+    ctrl |= (burstlen - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
+
     ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
 
     cfg |= MX51_ECSPI_CONFIG_SBBCTRL(config->cs);
@@ -666,6 +705,10 @@
         spi_imx->txfifo++;
     }
 
+    // always reconfigure IRQ source (for the TX data request IRQ to work properly)
+    spi_imx->devtype_data->intctrl(spi_imx,
+        ((spi_imx->txfifo == spi_imx_get_fifosize(spi_imx)) && (tx_fifo_thres != -1)) ? MXC_INT_THE : MXC_INT_TE);
+
     spi_imx->devtype_data->trigger(spi_imx);
 }
 
@@ -708,6 +751,7 @@
     config.speed_hz  = t ? t->speed_hz : spi->max_speed_hz;
     config.mode = spi->mode;
     config.cs = spi->chip_select;
+    config.burstlen = t ? (t->len * 8) : config.bpw;
 
     if (!config.speed_hz)
         config.speed_hz = spi->max_speed_hz;
@@ -745,8 +789,6 @@
 
     spi_imx_push(spi_imx);
 
-    spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
-
     wait_for_completion(&spi_imx->xfer_done);
 
     return transfer->len;
@@ -801,6 +843,63 @@
     return 0;
 }
 
+static ssize_t spidrv_setbigburst(struct device *dev,
+                                  struct device_attribute *attr,
+                                  const char *buf, size_t size)
+{
+    unsigned long tmp;
+    tmp = simple_strtoul(buf, NULL, 10);
+    if (tmp == 1) {
+        allow_big_bursts = 1;
+        } else {
+        allow_big_bursts = 0;
+    }
+    return size;
+}
+
+static ssize_t spidrv_getbigburst(struct device *dev,
+                                  struct device_attribute *attr,
+                                  char *buf)
+{
+    sprintf (buf, "%d\n", allow_big_bursts);
+    return (ssize_t)(strlen (buf) + 1);
+}
+
+static ssize_t spidrv_settxthres(struct device *dev,
+                                 struct device_attribute *attr,
+                                 const char *buf, size_t size)
+{
+    unsigned long tmp;
+    tmp = simple_strtoul(buf, NULL, 10);
+    if ((tmp > 0) && (tmp < 65)) {
+        tx_fifo_thres = tmp;
+    } else {
+        tx_fifo_thres = -1;
+    }
+    return size;
+}
+
+static ssize_t spidrv_gettxthres(struct device *dev,
+                                 struct device_attribute *attr,
+                                 char *buf)
+{
+    sprintf (buf, "%d\n", tx_fifo_thres);
+    return (ssize_t)(strlen (buf) + 1);
+}
+
+static DEVICE_ATTR(bigburst, 0666, spidrv_getbigburst, spidrv_setbigburst);
+static DEVICE_ATTR(txthres,  0666, spidrv_gettxthres , spidrv_settxthres);
+
+static struct attribute *spidrv_attributes[] = {
+    &dev_attr_bigburst.attr,
+    &dev_attr_txthres.attr,
+    NULL
+};
+
+static const struct attribute_group spidrv_group = {
+    .attrs = spidrv_attributes,
+};
+
 static int spi_imx_probe(struct platform_device *pdev)
 {
     struct device_node *np = pdev->dev.of_node;
@@ -840,6 +939,10 @@
     spi_imx = spi_master_get_devdata(master);
     spi_imx->bitbang.master = master;
 
+    if (sysfs_create_group(&pdev->dev.kobj, &spidrv_group)) {
+        return -ENOMEM;
+    }
+
     for (i = 0; i < master->num_chipselect; i++) {
         int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
         if (!gpio_is_valid(cs_gpio) && mxc_platform_info)


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] (re-send) spi-imx: improve timing of bursts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

On Fri, Mar 28, 2014 at 11:05:49AM +0000, Daniel Sobe wrote:
> 
> Hi,
> 
> The patch below addresses 2 improvements:

Please follow the patch submission process covered in SubmittingPatches
- in particular you need to send patches to the relevant developers as
well as the mailing lists, you need to format your changelogs in the
standard Linux style (including word wrapping within paragraphs) and you
need to follow the kernel coding style.

If you only send your patch to the list there's a good chance it'll be
missed.

> 1) Burstsize of a transfer adjusted up to the maximum burstsize of the  hardware --> no unwanted CS toggling between data words
> 2) Variable trigger level allows to increase priority for data transfer,  enables continuous transfers up to the burstsize. --> remove gaps  every 64 words

> The original behaviour of the driver is untouched. The additional features can be enabled via sysfs.

Why would you ever want /CS toggling between words - such behaviour is
just plain buggy?

> +// (sysfs-)tunable attributes
> +static int allow_big_bursts = 0; // if set to 1 this enables 32-bit big bursts
> +static int tx_fifo_thres = -1;   // if positive, use TX data request interrupt when applicable

These would need to be per device rather than global.  It's also not
clear to me that we don't want to enable some level of FIFO threashold
usage by default, perhaps aiming for something just slightly below the
full FIFO so that we minimise the number of extra interrupts but
increase throughput on lightly loaded systems - I'm not even sure that
making this behaviour tunable is needed.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] (re-send) spi-imx: improve timing of bursts

[Daniel Sobe]

Hi Mark,

thanks for your reply and your comments. I will make sure to follow your suggestions next time I try to submit a patch to the mailing list. It doesn't make sense to try again with this patch, because the project in which context it was created is already closed.

Regards,

Daniel

Daniel Sobe
Engineer, BL Car
NXP Semiconductors Germany GmbH

Am Waldschlösschen 1, Dresden, Germany
Tel: +49 351 32023 504
E-Mail: daniel.sobe-3arQi8VN3Tc@public.gmane.org, www.nxp.com
Geschäftsführung: Ruediger Stroh (Vors.), Frans Scheper, Kurt Sievers, Erik Just-Wartiainen / Aufsichtsratsvorsitzender: Gernot Fiedler / Sitz: Hamburg / Registergericht: Hamburg HRB 84 865

The information contained in this message is confidential and may be legally privileged. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, dissemination, or reproduction is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender by return e-mail and destroy all copies of the original message.

Unless otherwise recorded in a written agreement, all sales transactions by NXP Semiconductors are subject to our general terms and conditions of commercial sale. These are published at: www.nxp.com/profile/terms/index.html

________________________________________
From: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Sent: Saturday, August 16, 2014 5:13 PM
To: Daniel Sobe
Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] (re-send) spi-imx: improve timing of bursts

On Fri, Mar 28, 2014 at 11:05:49AM +0000, Daniel Sobe wrote:
>
> Hi,
>
> The patch below addresses 2 improvements:

Please follow the patch submission process covered in SubmittingPatches
- in particular you need to send patches to the relevant developers as
well as the mailing lists, you need to format your changelogs in the
standard Linux style (including word wrapping within paragraphs) and you
need to follow the kernel coding style.

If you only send your patch to the list there's a good chance it'll be
missed.

> 1) Burstsize of a transfer adjusted up to the maximum burstsize of the  hardware --> no unwanted CS toggling between data words
> 2) Variable trigger level allows to increase priority for data transfer,  enables continuous transfers up to the burstsize. --> remove gaps  every 64 words

> The original behaviour of the driver is untouched. The additional features can be enabled via sysfs.

Why would you ever want /CS toggling between words - such behaviour is
just plain buggy?

> +// (sysfs-)tunable attributes
> +static int allow_big_bursts = 0; // if set to 1 this enables 32-bit big bursts
> +static int tx_fifo_thres = -1;   // if positive, use TX data request interrupt when applicable

These would need to be per device rather than global.  It's also not
clear to me that we don't want to enable some level of FIFO threashold
usage by default, perhaps aiming for something just slightly below the
full FIFO so that we minimise the number of extra interrupts but
increase throughput on lightly loaded systems - I'm not even sure that
making this behaviour tunable is needed.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html