[PATCH v2 10/12] spi: add driver for J-Core SPI controller

[PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
My previous post of the patch series accidentally omitted omitted
Cc'ing of subsystem maintainers for the necessary clocksource,
irqchip, and spi drivers. Please ack if this looks ok because I want
to get it merged as part of the arch/sh pull request for 4.7.

 drivers/spi/Kconfig     |   4 +
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-jcore.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 drivers/spi/spi-jcore.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9d8c84b..5bd2ccf 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -285,6 +285,10 @@ config SPI_IMX
 	  This enables using the Freescale i.MX SPI controllers in master
 	  mode.
 
+config SPI_JCORE
+	tristate "J-Core SPI Master"
+	depends on OF
+
 config SPI_LM70_LLP
 	tristate "Parallel port adapter for LM70 eval board (DEVELOPMENT)"
 	depends on PARPORT
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index fbb255c..6a34124 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_FSL_SPI)		+= spi-fsl-spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi-gpio.o
 obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
+obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
diff --git a/drivers/spi/spi-jcore.c b/drivers/spi/spi-jcore.c
new file mode 100644
index 0000000..552304c
--- /dev/null
+++ b/drivers/spi/spi-jcore.c
@@ -0,0 +1,266 @@
+/*
+ * J-Core SPI controller driver
+ *
+ * Copyright (C) 2012-2016 SEI Inc.
+ *
+ * Current version by Rich Felker
+ * Based loosely on initial version by Oleksandr G Zhadan
+ *
+ */
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+
+#define USE_MESSAGE_MODE 1
+
+#define DRV_NAME "jcore_spi"
+
+#define MAX_SPI_SPEED			12500000	/* 12.5 MHz */
+
+#define CTRL_REG		0x0
+#define DATA_REG		0x4
+
+#define SPI_NOCHIP_CS	0
+#define SPI_FLASH_CS	1
+#define SPI_CONF_CS	2
+#define SPI_SD_CS	2
+#define SPI_CODEC_CS	3
+
+#define JCORE_SPI_CTRL_ACS		0x01
+#define JCORE_SPI_CTRL_XMIT		0x02
+#define JCORE_SPI_STAT_BUSY		0x02
+#define JCORE_SPI_CTRL_CCS		0x04
+#define JCORE_SPI_CTRL_LOOP		0x08
+#define JCORE_SPI_CTRL_DCS		0x10
+
+#define JCORE_SPI_WAIT_RDY_MAX_LOOP	2000000	/* in usec */
+
+struct jcore_spi {
+	struct spi_master *master;
+	void __iomem *base;
+	volatile unsigned int ctrlReg;
+	unsigned int csReg;
+	unsigned int speedReg;
+	unsigned int speed_hz;
+};
+
+static void jcore_spi_wait_till_ready(struct jcore_spi *hw, int timeout)
+{
+	while (timeout--) {
+		hw->ctrlReg = readl(hw->base + CTRL_REG);
+		if (!(hw->ctrlReg & JCORE_SPI_STAT_BUSY))
+			return;
+		cpu_relax();
+	}
+	pr_err("%s: Timeout..\n", __func__);
+}
+
+static void jcore_spi_program(struct jcore_spi *hw)
+{
+	jcore_spi_wait_till_ready(hw, JCORE_SPI_WAIT_RDY_MAX_LOOP);
+	writel(hw->csReg | hw->speedReg, hw->base + CTRL_REG);	
+}
+
+static void jcore_spi_chipsel(struct spi_device *spi, bool value)
+{
+	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
+
+	pr_debug("%s: CS=%d\n", __func__, value);
+
+	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
+		^ (!value << 2*spi->chip_select);
+
+	jcore_spi_program(hw);
+}
+
+static void jcore_spi_baudrate(struct jcore_spi *hw, int speed)
+{
+	if (speed == hw->speed_hz) return;
+	hw->speed_hz = speed;
+	hw->speedReg = ((MAX_SPI_SPEED / speed) - 1) << 27;
+	jcore_spi_program(hw);
+	pr_debug("%s: speed=%d pre=0x%x\n", __func__, speed, hw->speedReg);
+}
+
+static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
+{
+	struct jcore_spi *hw = spi_master_get_devdata(master);
+
+	void *ctrl_reg = hw->base + CTRL_REG;
+	void *data_reg = hw->base + DATA_REG;
+	int timeout;
+	int xmit;
+	int status;
+
+	/* data buffers */
+	const unsigned char *tx;
+	unsigned char *rx;
+	int len;
+	int count;
+
+	jcore_spi_baudrate(hw, t->speed_hz);
+
+	xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
+	tx = t->tx_buf;
+	rx = t->rx_buf;
+	len = t->len;
+
+	for (count = 0; count < len; count++) {
+		timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
+		do status = readl(ctrl_reg);
+		while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
+		if (!timeout) break;
+
+		writel(tx ? *tx++ : 0, data_reg);
+		writel(xmit, ctrl_reg);
+
+		timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
+		do status = readl(ctrl_reg);
+		while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
+		if (!timeout) break;
+
+		if (rx) *rx++ = readl(data_reg);
+	}
+
+#if !USE_MESSAGE_MODE
+	spi_finalize_current_transfer(master);
+#endif
+
+	return count<len ? -EREMOTEIO : 0;
+}
+
+#if USE_MESSAGE_MODE
+static int jcore_spi_transfer_one_message(struct spi_master *master,
+					struct spi_message *msg)
+{
+	struct spi_device *spi = msg->spi;
+	struct spi_transfer *xfer;
+	bool keep_cs = false;
+	int ret = 0;
+
+	jcore_spi_chipsel(spi, false);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		ret = jcore_spi_txrx(master, spi, xfer);
+		if (ret) break;
+		if (xfer->delay_usecs)
+			udelay(xfer->delay_usecs);
+		if (xfer->cs_change) {
+			if (list_is_last(&xfer->transfer_list,
+					 &msg->transfers)) {
+				keep_cs = true;
+			} else {
+				jcore_spi_chipsel(spi, true);
+				udelay(10);
+				jcore_spi_chipsel(spi, false);
+			}
+		}
+		msg->actual_length += xfer->len;
+	}
+
+	if (!keep_cs)
+		jcore_spi_chipsel(spi, true);
+
+	msg->status = ret;
+
+	spi_finalize_current_message(master);
+
+	return ret;
+}
+#endif
+
+static int jcore_spi_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct jcore_spi *hw;
+	struct spi_master *master;
+	struct resource *res;
+	int err = -ENODEV;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct jcore_spi));
+	if (!master)
+		return err;
+
+	/* setup the master state. */
+	master->num_chipselect = 3;
+	master->mode_bits = SPI_MODE_3;
+#if USE_MESSAGE_MODE
+	master->transfer_one_message = jcore_spi_transfer_one_message;
+#else
+	master->transfer_one = jcore_spi_txrx;
+#endif
+	master->set_cs = jcore_spi_chipsel;
+	master->dev.of_node = node;
+
+	hw = spi_master_get_devdata(master);
+	hw->master = master;
+	platform_set_drvdata(pdev, hw);
+
+	/* find and map our resources */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		goto exit_busy;
+	if (!devm_request_mem_region
+	    (&pdev->dev, res->start, resource_size(res), pdev->name))
+		goto exit_busy;
+	hw->base =
+	    devm_ioremap_nocache(&pdev->dev, res->start, resource_size(res));
+	if (!hw->base)
+		goto exit_busy;
+
+	jcore_spi_baudrate(hw, 400000);
+
+	pdev->dev.dma_mask = 0;
+	/* register our spi controller */
+	err = spi_register_master(master);
+	if (err)
+		goto exit;
+	dev_info(&pdev->dev, "base %p, noirq\n", hw->base);
+
+	return 0;
+
+exit_busy:
+	err = -EBUSY;
+exit:
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(master);
+	return err;
+}
+
+static int jcore_spi_remove(struct platform_device *dev)
+{
+	struct jcore_spi *hw = platform_get_drvdata(dev);
+	struct spi_master *master = hw->master;
+
+	platform_set_drvdata(dev, NULL);
+	spi_master_put(master);
+	return 0;
+}
+
+static const struct of_device_id jcore_spi_of_match[] = {
+	{ .compatible = "jcore,spi2" },
+	{},
+};
+
+static struct platform_driver jcore_spi_driver = {
+	.probe = jcore_spi_probe,
+	.remove = jcore_spi_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.pm = NULL,
+		.of_match_table = jcore_spi_of_match,
+	},
+};
+
+module_platform_driver(jcore_spi_driver);
+
+MODULE_DESCRIPTION("J-Core SPI driver");
+MODULE_AUTHOR("Rich Felker <dalias@libc.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.8.1

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> --- /dev/null
> +++ b/drivers/spi/spi-jcore.c

> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> +{
> +       struct jcore_spi *hw = spi_master_get_devdata(master);
> +
> +       void *ctrl_reg = hw->base + CTRL_REG;
> +       void *data_reg = hw->base + DATA_REG;
> +       int timeout;

unsigned int

> +       int xmit;

u32

> +       int status;

u32

> +
> +       /* data buffers */
> +       const unsigned char *tx;
> +       unsigned char *rx;
> +       int len;

unsigned int

> +       int count;

unsigned int

> +
> +       jcore_spi_baudrate(hw, t->speed_hz);
> +
> +       xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
> +       tx = t->tx_buf;
> +       rx = t->rx_buf;
> +       len = t->len;
> +
> +       for (count = 0; count < len; count++) {
> +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> +               do status = readl(ctrl_reg);
> +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);

do {
        ...
} while (...)

> +               if (!timeout) break;

if (...)
        ...

> +
> +               writel(tx ? *tx++ : 0, data_reg);

You can remove the check for tx if you set the SPI_MASTER_MUST_TX
flag in spi_master.flags.

> +               writel(xmit, ctrl_reg);
> +
> +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> +               do status = readl(ctrl_reg);
> +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);

do {
        ...
} while (...)

> +               if (!timeout) break;


if (...)
        ...

> +
> +               if (rx) *rx++ = readl(data_reg);


if (...)
        ...

> +       }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Fri, May 20, 2016 at 10:15:08AM +0200, Geert Uytterhoeven wrote:
> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> > --- /dev/null
> > +++ b/drivers/spi/spi-jcore.c
> 
> > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> > +{
> > +       struct jcore_spi *hw = spi_master_get_devdata(master);
> > +
> > +       void *ctrl_reg = hw->base + CTRL_REG;
> > +       void *data_reg = hw->base + DATA_REG;
> > +       int timeout;
> 
> unsigned int

All of these have value ranges where the type is irrelevant, but I'm
happy to change it to whatever types you prefer.

> > +       jcore_spi_baudrate(hw, t->speed_hz);
> > +
> > +       xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
> > +       tx = t->tx_buf;
> > +       rx = t->rx_buf;
> > +       len = t->len;
> > +
> > +       for (count = 0; count < len; count++) {
> > +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> > +               do status = readl(ctrl_reg);
> > +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
> 
> do {
>         ...
> } while (...)
> 
> > +               if (!timeout) break;
> 
> if (...)
>         ...

OK. (for this and other instances)

> > +
> > +               writel(tx ? *tx++ : 0, data_reg);
> 
> You can remove the check for tx if you set the SPI_MASTER_MUST_TX
> flag in spi_master.flags.

I don't want to do that, because the new version of the hardware
that's going to support DMA can only do unidirectional DMA, and thus
we need to be able to see if either tx or rx is null.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

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

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.

This is *extremely* late for a first posting of a driver for v4.7 (you
missed the list as well as the maintainers).

> +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> +{
> +	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> +
> +	pr_debug("%s: CS=%d\n", __func__, value);

dev_dbg()

> +
> +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> +		^ (!value << 2*spi->chip_select);

Why the xor here and not an or?  The coding style is also weird, a mix
of extra spaces around the () and missing ones around *.  I'm finding
the intent of the code confusing here.

> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)

Coding style, please keep lines under 80 columns unless there's a good
reason.

> +#if !USE_MESSAGE_MODE
> +	spi_finalize_current_transfer(master);
> +#endif

I'm not sure what the if is about but it doesn't belong upstream, you
shouldn't be open coding bits of the framework.

> +	/* register our spi controller */
> +	err = spi_register_master(master);

devm_

> +static int jcore_spi_remove(struct platform_device *dev)
> +{
> +	struct jcore_spi *hw = platform_get_drvdata(dev);
> +	struct spi_master *master = hw->master;
> +
> +	platform_set_drvdata(dev, NULL);
> +	spi_master_put(master);
> +	return 0;
> +}

This can be removed entirely.

> +static const struct of_device_id jcore_spi_of_match[] = {
> +	{ .compatible = "jcore,spi2" },
> +	{},
> +};

This is adding a DT binding with no binding document.  All new DT
bindings need to be documented.

> +		.owner = THIS_MODULE,
> +		.pm = NULL,

No need to set either of these.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:
> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> > My previous post of the patch series accidentally omitted omitted
> > Cc'ing of subsystem maintainers for the necessary clocksource,
> > irqchip, and spi drivers. Please ack if this looks ok because I want
> > to get it merged as part of the arch/sh pull request for 4.7.
> 
> This is *extremely* late for a first posting of a driver for v4.7 (you
> missed the list as well as the maintainers).

I'm sorry for the mix-up. The kernel workflow is still fairly new to
me and I've been fighting with git format-patch/send-email and
get_maintainer.pl to get big patch series prepared the way I want and
sent to the right people/lists. I think I've got it right now though.

> > +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> > +{
> > +	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> > +
> > +	pr_debug("%s: CS=%d\n", __func__, value);
> 
> dev_dbg()

OK.

> > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > +		^ (!value << 2*spi->chip_select);
> 
> Why the xor here and not an or?  The coding style is also weird, a mix
> of extra spaces around the () and missing ones around *.  I'm finding
> the intent of the code confusing here.

The intent is to set all chipselect bits (the 3 macro values) high
except possibly spi->chip_select. The xor is to turn off a bit, not
turn it on. &~ would also have worked; would that be more idiomatic?

Since the individual CS bits and their names in the hardware aren't
actually relevant to the driver, I'm replacing them with a single:

#define JCORE_SPI_CTRL_CS_BITS          0x15

so I can just write:

hw->csReg = JCORE_SPI_CTRL_CS_BITS ^ (!value << 2*spi->chip_select);

Does that look better, or should I opt for &~?

> > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> 
> Coding style, please keep lines under 80 columns unless there's a good
> reason.

OK.

> > +#if !USE_MESSAGE_MODE
> > +	spi_finalize_current_transfer(master);
> > +#endif
> 
> I'm not sure what the if is about but it doesn't belong upstream, you
> shouldn't be open coding bits of the framework.

I can explain the motivation and see what you think is best to do. I'd
like to be able to provide just the transfer_one operation, and use
the generic transfer_one_message. However, at 50 MHz cpu clock, the
stats collection and other overhead in spi.c's generic
spi_transfer_one_message takes up so much time between the end of one
SD card transfer and the beginning of the next that the overall
transfer rate is something like 15-20% higher with my version.

Another consideration is that DMA support is being added to the
hardware right now, and I think we're going to want to be able to
queue up whole messages for DMA rather than just individual transfers;
in that case, spi_transfer_one_message is probably not suitable
anyway. So we'll probably have to end up having our own
transfer_one_message function anyway.

If that's not actually needed, a possible alternative for fixing the
performance problem might be adding a Kconfig option to turn off all
the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I
could work on that instead (or in addition), and I wouldn't consider
it critical to get in for this merge window.

> > +	/* register our spi controller */
> > +	err = spi_register_master(master);
> 
> devm_
> 
> > +static int jcore_spi_remove(struct platform_device *dev)
> > +{
> > +	struct jcore_spi *hw = platform_get_drvdata(dev);
> > +	struct spi_master *master = hw->master;
> > +
> > +	platform_set_drvdata(dev, NULL);
> > +	spi_master_put(master);
> > +	return 0;
> > +}
> 
> This can be removed entirely.

OK. Does using the devm register function cause removal to be handled
generically, or is there another reason it's not needed?

> > +static const struct of_device_id jcore_spi_of_match[] = {
> > +	{ .compatible = "jcore,spi2" },
> > +	{},
> > +};
> 
> This is adding a DT binding with no binding document.  All new DT
> bindings need to be documented.

The DT binding was in patch 05/12. Should linux-spi have been added to
the Cc list for it? get_maintainer.pl didn't include it.

> > +		.owner = THIS_MODULE,
> > +		.pm = NULL,
> 
> No need to set either of these.

OK.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

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

On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote:
> On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:

> > This is *extremely* late for a first posting of a driver for v4.7 (you
> > missed the list as well as the maintainers).

> I'm sorry for the mix-up. The kernel workflow is still fairly new to
> me and I've been fighting with git format-patch/send-email and
> get_maintainer.pl to get big patch series prepared the way I want and
> sent to the right people/lists. I think I've got it right now though.

One question here is why this is even part of a series - it's adding a
new controller driver which wouldn't normally have any sort of direct
build or other dependency on anything else or have other things
depending on it.  Unless there are dependencies it is generally best to
send separate changes separately as far as possible so that there is no
need to worry about issues with one part of the series slowing down
other parts of the series.

If it does make sense to send a series you need to communicate what's
going on with dependencies with all the maintainers, normally at least
the cover letter if not the entire series should go to everyone.

You should never rely on get_maintainers, you need to think about what
it's telling you.  It both misses people and generates false positives
(especially when it looks at git history).  It's useful to look at since
it normally gets you most of the way there but 

> > > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > > +		^ (!value << 2*spi->chip_select);

> > Why the xor here and not an or?  The coding style is also weird, a mix
> > of extra spaces around the () and missing ones around *.  I'm finding
> > the intent of the code confusing here.

> The intent is to set all chipselect bits (the 3 macro values) high
> except possibly spi->chip_select. The xor is to turn off a bit, not
> turn it on. &~ would also have worked; would that be more idiomatic?

No, the reader has to be able to tell what the code is doing.  If this
made sense the thing to do would be to write out the logic operations
explicitly to make it clear that every step is deliberate.  However in
this case it sounds like the code is just plain buggy, though - the
driver is being asked to set a specific chip select to a specific value
but instead of just doing that it is also trying to also change some
other settings.  Don't do that, just have the function do what it was
asked.  If the calling code has problems it'll need fixing anyway and if
some feature you hadn't anticipated ends up meaning the combination of
operations makes sense then things will just work.

> > > +#if !USE_MESSAGE_MODE
> > > +	spi_finalize_current_transfer(master);
> > > +#endif

> > I'm not sure what the if is about but it doesn't belong upstream, you
> > shouldn't be open coding bits of the framework.

> I can explain the motivation and see what you think is best to do. I'd
> like to be able to provide just the transfer_one operation, and use
> the generic transfer_one_message. However, at 50 MHz cpu clock, the
> stats collection and other overhead in spi.c's generic
> spi_transfer_one_message takes up so much time between the end of one
> SD card transfer and the beginning of the next that the overall
> transfer rate is something like 15-20% higher with my version.

No, this is just not a good approach.  If the generic code isn't working
for you improve the generic code, don't just copy it and open code the
bits you like.  The reason Linux has all these great framework is that
people collaborate to make the generic code as good and fully featured
as they can rather than open coding everything in drivers.  Doing things
in drivers results in lots of code duplication which increases the cost
of maintaining things and requires that everyone waste time copying code
in order to keep the feature set consistent between drivers.

Drivers should do things specific to a given piece of hardware, anything
in the generic code should be dealt with in the generic code.

> Another consideration is that DMA support is being added to the
> hardware right now, and I think we're going to want to be able to
> queue up whole messages for DMA rather than just individual transfers;
> in that case, spi_transfer_one_message is probably not suitable
> anyway. So we'll probably have to end up having our own
> transfer_one_message function anyway.

This is again a simple generic optimisation which would be best
implemented in generic code - it's not just this device which would
benefit from the ability to coalesce compatible transfers.  There is no
reason for individual drivers to even think about such an optimisation,
the core should just be handling them a transfer with a coalesced DMA
transfer in it.  We're not doing it at present because someone needs to
take the time to write the code that figures out if adjacent transfers
are compatible and merges them if they are.

> If that's not actually needed, a possible alternative for fixing the
> performance problem might be adding a Kconfig option to turn off all
> the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I
> could work on that instead (or in addition), and I wouldn't consider
> it critical to get in for this merge window.

This driver is *not* going in during this merge window, sorry.  You need
to get code into maintainer trees before the merge window opens but this
was only sent to maintainers after the merge window was already open.
This isn't the end of the world, there will be another kernel release in
a few months.

> > > +	platform_set_drvdata(dev, NULL);
> > > +	spi_master_put(master);
> > > +	return 0;
> > > +}

> > This can be removed entirely.

> OK. Does using the devm register function cause removal to be handled
> generically, or is there another reason it's not needed?

Yes.

> > > +static const struct of_device_id jcore_spi_of_match[] = {
> > > +	{ .compatible = "jcore,spi2" },
> > > +	{},
> > > +};

> > This is adding a DT binding with no binding document.  All new DT
> > bindings need to be documented.

> The DT binding was in patch 05/12. Should linux-spi have been added to
> the Cc list for it? get_maintainer.pl didn't include it.

Yes, the documentation and the code need to be reviewed together.  It's
hard to tell if the code implements the bindings without seeing both.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote:
> On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote:
> > On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:
> 
> > > This is *extremely* late for a first posting of a driver for v4.7 (you
> > > missed the list as well as the maintainers).
> 
> > I'm sorry for the mix-up. The kernel workflow is still fairly new to
> > me and I've been fighting with git format-patch/send-email and
> > get_maintainer.pl to get big patch series prepared the way I want and
> > sent to the right people/lists. I think I've got it right now though.
> 
> One question here is why this is even part of a series - it's adding a
> new controller driver which wouldn't normally have any sort of direct
> build or other dependency on anything else or have other things
> depending on it.  Unless there are dependencies it is generally best to
> send separate changes separately as far as possible so that there is no
> need to worry about issues with one part of the series slowing down
> other parts of the series.

I grouped the patches as a series because they make up support for a
complete SoC. While some of the peripheral drivers may well be useful
for non-J2 systems in the future (the cores are all open source, BSD
licensed), there's no short-term benefit to having these drivers
without the main J2 support.

In terms of hard dependencies, the mimas_v2 dts file depends on the
jcore,spi binding, but nothing depends on the driver. Still it's
important to have for the sake of actually making use of it all.

> > > > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > > > +		^ (!value << 2*spi->chip_select);
> 
> > > Why the xor here and not an or?  The coding style is also weird, a mix
> > > of extra spaces around the () and missing ones around *.  I'm finding
> > > the intent of the code confusing here.
> 
> > The intent is to set all chipselect bits (the 3 macro values) high
> > except possibly spi->chip_select. The xor is to turn off a bit, not
> > turn it on. &~ would also have worked; would that be more idiomatic?
> 
> No, the reader has to be able to tell what the code is doing.  If this
> made sense the thing to do would be to write out the logic operations
> explicitly to make it clear that every step is deliberate.  However in
> this case it sounds like the code is just plain buggy, though - the
> driver is being asked to set a specific chip select to a specific value
> but instead of just doing that it is also trying to also change some
> other settings.

It may be redundant, if the general SPI framework handles mutual
exclusion of chipselects, but it's not buggy. Only a single chipselect
can be active (low) at once; with multiple chips selected very bad
things will happen. I don't see any documentation of the high-level
SPI framework in Linux and what (if anything) it does to ensure that
all other chipselects are disabled before enabling one, which I why I
wrote the code so that the other chipselects are explicitly disabled.

> Don't do that, just have the function do what it was
> asked.  If the calling code has problems it'll need fixing anyway and if
> some feature you hadn't anticipated ends up meaning the combination of
> operations makes sense then things will just work.

Setting just a single bit seems more complicated and error-prone to
me; explicit effort has to be made to ensure that the hardware state
remains in sync with the driver's view of it. As written now, the
whole register (with all the cs/speed/etc. bits) is just written every
time.

> > > > +#if !USE_MESSAGE_MODE
> > > > +	spi_finalize_current_transfer(master);
> > > > +#endif
> 
> > > I'm not sure what the if is about but it doesn't belong upstream, you
> > > shouldn't be open coding bits of the framework.
> 
> > I can explain the motivation and see what you think is best to do. I'd
> > like to be able to provide just the transfer_one operation, and use
> > the generic transfer_one_message. However, at 50 MHz cpu clock, the
> > stats collection and other overhead in spi.c's generic
> > spi_transfer_one_message takes up so much time between the end of one
> > SD card transfer and the beginning of the next that the overall
> > transfer rate is something like 15-20% higher with my version.
> 
> No, this is just not a good approach.  If the generic code isn't working
> for you improve the generic code, don't just copy it and open code the
> bits you like.  The reason Linux has all these great framework is that
> people collaborate to make the generic code as good and fully featured
> as they can rather than open coding everything in drivers.  Doing things
> in drivers results in lots of code duplication which increases the cost
> of maintaining things and requires that everyone waste time copying code
> in order to keep the feature set consistent between drivers.

I totally agree with this principle, and I'm fine with dropping the
offending code. I may maintain it out-of-tree as a performance patch
for a while, but DMA and other improvements will hopefully make it
unnecessary.

> > Another consideration is that DMA support is being added to the
> > hardware right now, and I think we're going to want to be able to
> > queue up whole messages for DMA rather than just individual transfers;
> > in that case, spi_transfer_one_message is probably not suitable
> > anyway. So we'll probably have to end up having our own
> > transfer_one_message function anyway.
> 
> This is again a simple generic optimisation which would be best
> implemented in generic code - it's not just this device which would
> benefit from the ability to coalesce compatible transfers.  There is no
> reason for individual drivers to even think about such an optimisation,
> the core should just be handling them a transfer with a coalesced DMA
> transfer in it.  We're not doing it at present because someone needs to
> take the time to write the code that figures out if adjacent transfers
> are compatible and merges them if they are.

OK, then let's go with that approach. As long as there's a viable way
forward for doing it in a way that benefits all devices/drivers, I
prefer it anyway. I just didn't want to hit a situation where I later
propose that and a mainainer (you or someone else) tells me it's not
possible or not necessary.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

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

On Mon, May 23, 2016 at 04:29:38PM -0400, Rich Felker wrote:
> On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote:

> > One question here is why this is even part of a series - it's adding a
> > new controller driver which wouldn't normally have any sort of direct
> > build or other dependency on anything else or have other things
> > depending on it.  Unless there are dependencies it is generally best to
> > send separate changes separately as far as possible so that there is no
> > need to worry about issues with one part of the series slowing down
> > other parts of the series.

> I grouped the patches as a series because they make up support for a
> complete SoC. While some of the peripheral drivers may well be useful
> for non-J2 systems in the future (the cores are all open source, BSD
> licensed), there's no short-term benefit to having these drivers
> without the main J2 support.

That's what -next is for, merging everyone's trees together to give
something to test.  Practically speaking most maintainence is done at
the build level, it's how we do all the other SoCs so that people can
see what's going on at the subsystem level.

> > No, the reader has to be able to tell what the code is doing.  If this
> > made sense the thing to do would be to write out the logic operations
> > explicitly to make it clear that every step is deliberate.  However in
> > this case it sounds like the code is just plain buggy, though - the
> > driver is being asked to set a specific chip select to a specific value
> > but instead of just doing that it is also trying to also change some
> > other settings.

> It may be redundant, if the general SPI framework handles mutual
> exclusion of chipselects, but it's not buggy. Only a single chipselect
> can be active (low) at once; with multiple chips selected very bad
> things will happen. I don't see any documentation of the high-level
> SPI framework in Linux and what (if anything) it does to ensure that
> all other chipselects are disabled before enabling one, which I why I
> wrote the code so that the other chipselects are explicitly disabled.

There is no such guarantee because there are applications where it
makes sense to write to multiple chips simultaneously - this is one
common way of doing simultaneous updates over multiple audio audio
CODECs to ensure synchronization for example.  It's also just not
something that it makes any sense to worry about at the indivudal driver
level.

The generic code is responsible for ensuring that things work well,
writing bodges that silently try to work around the generic code is
always a recipie for fragility, especially if that code is hard to
understand.  Either the driver was making unwarranted assumptions that
break use cases it didn't think of or we end up having to find and fix
issues multiple times due to duplication.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]