[PATCH 3/4] spi: orion: Prepare space for per-child options

[PATCH 3/4] spi: orion: Prepare space for per-child options

[Jan Kundrát]

Aggregating all options for a particular child underneath a common
struct looks cleaner compared to having a separate array for each
per-child option.

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
This is just a precursor for the next one in series ("spi: orion: Software control for inter-word delays").
---
 drivers/spi/spi-orion.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index 56e419a7ed78..1eccc2287079 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -90,6 +90,10 @@ struct orion_direct_acc {
 	u32			size;
 };
 
+struct orion_child_options {
+	struct orion_direct_acc direct_access;
+};
+
 struct orion_spi {
 	struct spi_master	*master;
 	void __iomem		*base;
@@ -98,7 +102,7 @@ struct orion_spi {
 	const struct orion_spi_dev *devdata;
 	int			unused_hw_gpio;
 
-	struct orion_direct_acc	direct_access[ORION_NUM_CHIPSELECTS];
+	struct orion_child_options	child[ORION_NUM_CHIPSELECTS];
 };
 
 static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg)
@@ -436,7 +440,7 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 	 * Use SPI direct write mode if base address is available. Otherwise
 	 * fall back to PIO mode for this transfer.
 	 */
-	if ((orion_spi->direct_access[cs].vaddr) && (xfer->tx_buf) &&
+	if ((orion_spi->child[cs].direct_access.vaddr) && (xfer->tx_buf) &&
 	    (word_len == 8)) {
 		unsigned int cnt = count / 4;
 		unsigned int rem = count % 4;
@@ -445,12 +449,12 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 		 * Send the TX-data to the SPI device via the direct
 		 * mapped address window
 		 */
-		iowrite32_rep(orion_spi->direct_access[cs].vaddr,
+		iowrite32_rep(orion_spi->child[cs].direct_access.vaddr,
 			      xfer->tx_buf, cnt);
 		if (rem) {
 			u32 *buf = (u32 *)xfer->tx_buf;
 
-			iowrite8_rep(orion_spi->direct_access[cs].vaddr,
+			iowrite8_rep(orion_spi->child[cs].direct_access.vaddr,
 				     &buf[cnt], rem);
 		}
 
@@ -707,14 +711,14 @@ static int orion_spi_probe(struct platform_device *pdev)
 		 * This needs to get extended for the direct SPI-NOR / SPI-NAND
 		 * support, once this gets implemented.
 		 */
-		spi->direct_access[cs].vaddr = devm_ioremap(&pdev->dev,
+		spi->child[cs].direct_access.vaddr = devm_ioremap(&pdev->dev,
 							    r->start,
 							    PAGE_SIZE);
-		if (!spi->direct_access[cs].vaddr) {
+		if (!spi->child[cs].direct_access.vaddr) {
 			status = -ENOMEM;
 			goto out_rel_axi_clk;
 		}
-		spi->direct_access[cs].size = PAGE_SIZE;
+		spi->child[cs].direct_access.size = PAGE_SIZE;
 
 		dev_info(&pdev->dev, "CS%d configured for direct access\n", cs);
 	}
-- 
2.14.3


--
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

[PATCH 4/4] spi: orion: Software control for inter-word delays

[Jan Kundrát]

Add a SW-controlled timer for inserting delays between individual words
as transmitted over the SPI bus. The DT property name is loosely
modelled after a similar, but HW-based feature in spi-davinci.c (commit
365a7bb32e09) -- hence the DT property name.

My HW sucks. One of the less-serious troubles is that it requires a 3us
delay between all SPI words. It also requires "big" transfers, easily
around 40kB, which is 20k of 16-bit words. It's also a specialised,
proprietary thing with no need for an in-kernel driver, so I'm using
spidev to access it.

There's a limit in spidev's SPI_IOC_MESSAGE and ioctl architecture in
general which means that I can stash at most 512 individual
spi_ioc_transfer into one ioctl. When I needed to transfer small
pockets, that was enough -- I could simply build a series of hundreds of
spi_ioc_trnasfers, two bytes each, with a proper delay_usecs to persuade
the SPI core to implement these delays for me. However, this won't work
if I need to send more than 1kB of data that way.

It seems that there's nothing generic in Linux to implement this
feature. The TI's spi-davinci.c can do something liek this in HW. People
at various forums apparently want to do something similar on Zynq and on
RPis, perhaps in SW. I wasn't able to find any ready-made patches,
though. My SoC (88F68xx, Marvell Armada A38x) apparently cannot do this
natively, so this patch simply adds a call to udelay which does the
trick for me.

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-orion.txt | 20 ++++++++++++++++++++
 drivers/spi/spi-orion.c                             | 19 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
index 8434a65fc12a..ed35cba1bc33 100644
--- a/Documentation/devicetree/bindings/spi/spi-orion.txt
+++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
@@ -29,6 +29,10 @@ Optional properties:
 		used, the name must be "core", and "axi" (the latter
 		is only for Armada 7K/8K).
 
+Optional properties of child nodes (SPI slave devices):
+- linux,spi-wdelay : If present and non-zero, specifies a delay in
+		     microseconds between words transferred over the SPI bus.
+
 
 Example:
        spi@10600 {
@@ -77,3 +81,19 @@ are used in the default indirect (PIO) mode):
 For further information on the MBus bindings, please see the MBus
 DT documentation:
 Documentation/devicetree/bindings/bus/mvebu-mbus.txt
+
+Example of a per-child inter-word delay:
+
+	spi0: spi@10600 {
+		/* ... */
+
+		some_slave_device@2 {
+			reg = <2>;
+			compatible = "something";
+			/* ... */
+
+			/* Wait 3 microseconds between all words within all
+			SPI transactions */
+			linux,spi-wdelay = /bits/ 16 <3>;
+		};
+	};
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index 1eccc2287079..1a9475857808 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -92,6 +92,7 @@ struct orion_direct_acc {
 
 struct orion_child_options {
 	struct orion_direct_acc direct_access;
+	u16			word_delay;
 };
 
 struct orion_spi {
@@ -469,6 +470,8 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 			if (orion_spi_write_read_8bit(spi, &tx, &rx) < 0)
 				goto out;
 			count--;
+			if (orion_spi->child[cs].word_delay)
+				udelay(orion_spi->child[cs].word_delay);
 		} while (count);
 	} else if (word_len == 16) {
 		const u16 *tx = xfer->tx_buf;
@@ -477,6 +480,8 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 		do {
 			if (orion_spi_write_read_16bit(spi, &tx, &rx) < 0)
 				goto out;
+			if (orion_spi->child[cs].word_delay)
+				udelay(orion_spi->child[cs].word_delay);
 			count -= 2;
 		} while (count);
 	}
@@ -681,7 +686,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 		goto out_rel_axi_clk;
 	}
 
-	/* Scan all SPI devices of this controller for direct mapped devices */
+	/* Scan all SPI devices of this controller for direct mapped devices and word delay */
 	for_each_available_child_of_node(pdev->dev.of_node, np) {
 		u32 cs;
 
@@ -694,6 +699,12 @@ static int orion_spi_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		spi->child[cs].word_delay = 0;
+		if (!of_property_read_u16(np, "linux,spi-wdelay",
+					&spi->child[cs].word_delay))
+			dev_info(&pdev->dev, "%pOF: %dus delay between words\n",
+					np, spi->child[cs].word_delay);
+
 		/*
 		 * Check if an address is configured for this SPI device. If
 		 * not, the MBus mapping via the 'ranges' property in the 'soc'
@@ -705,6 +716,12 @@ static int orion_spi_probe(struct platform_device *pdev)
 		if (status)
 			continue;
 
+		if (spi->child[cs].word_delay) {
+			dev_warn(&pdev->dev,
+				"%pOF linux,spi-wdelay takes preference over a direct-mode", np);
+			continue;
+		}
+
 		/*
 		 * Only map one page for direct access. This is enough for the
 		 * simple TX transfer which only writes to the first word.
-- 
2.14.3


--
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

[PATCH 1/4] spi: orion: Make the error message greppable

[Jan Kundrát]

Commit 544248623b95 introduced a new user-visible string which was
however split into two chunks. Thanks to Mark Brown for noticing.

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
 drivers/spi/spi-orion.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index b341235d2947..ca52300baeb1 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -762,8 +762,8 @@ static int orion_spi_probe(struct platform_device *pdev)
 			}
 			if (spi->unused_hw_gpio == -1) {
 				dev_info(&pdev->dev,
-					"Selected unused HW CS#%d "
-					"for any GPIO CSes\n", i);
+					"Selected unused HW CS#%d for any GPIO CSes\n",
+					i);
 				spi->unused_hw_gpio = i;
 			}
 		}
-- 
2.14.3


--
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

[PATCH 2/4] spi: orion: Respect per-transfer bits_per_word settings

[Jan Kundrát]

It's a bug to look at the device's bits_per_word because each transfer
can override these settings. In fact, e.g., spidev cannot specify
"global" bits_per_word and instead relies on passing that through every
ioctl.

This code already did the right thing in orion_spi_setup_transfer, but
then failed to access the HW in an appropriate way here in
orion_spi_write_read.

Tested by 16-bit trnasfers via spidev on an Armada 388 (Solidrun
Clearfog).

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
 drivers/spi/spi-orion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index ca52300baeb1..56e419a7ed78 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -427,7 +427,7 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
 	struct orion_spi *orion_spi;
 	int cs = spi->chip_select;
 
-	word_len = spi->bits_per_word;
+	word_len = xfer->bits_per_word ? xfer->bits_per_word : spi->bits_per_word;
 	count = xfer->len;
 
 	orion_spi = spi_master_get_devdata(spi->master);
-- 
2.14.3


--
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 4/4] spi: orion: Software control for inter-word delays

[Mark Brown]

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

On Sat, Feb 10, 2018 at 01:25:32PM +0100, Jan Kundrát wrote:

> Add a SW-controlled timer for inserting delays between individual words
> as transmitted over the SPI bus. The DT property name is loosely
> modelled after a similar, but HW-based feature in spi-davinci.c (commit
> 365a7bb32e09) -- hence the DT property name.

I think this is a sensible and reasonable thing to want to do however I
think we should move this from a DT property to being something in the
transfer structure which drivers then implement.  That way if a device
has an inter-word delay requirement it can go in the individual device
driver so it always gets applied.  Does that make sense to you?

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

Re: [PATCH 4/4] spi: orion: Software control for inter-word delays

[Jan Kundrát]

On čtvrtek 15. února 2018 16:19:07 CET, Mark Brown wrote:
> I think this is a sensible and reasonable thing to want to do however I
> think we should move this from a DT property to being something in the
> transfer structure which drivers then implement.  That way if a device
> has an inter-word delay requirement it can go in the individual device
> driver so it always gets applied.  Does that make sense to you?

Yep, that indeed makes sense (Damn, my easy attempt didn't make it :).)

How should the interface look like? Is spi_controller->mode_bits a good 
place for controllers to advertise this feature (SPI_WORD_DELAY, perhaps?) 
as supported, with spi-orion being the only implementation now? If we allow 
each spi_transfer to override this value, is there a common place in the 
SPI core which somehow validates a spi_transfer against each controller's 
capabilities? I see a code like that (bad_bits, ugly_bits) in spi_setup 
which looks like something to be called per-device, not per-transfer.

My most important use case is, however, the userspace-facing spidev, and 
its ioctl complicates stuff a bit. The relevant struct has a 16bit padding 
now (used to be 32bit prior to 
https://patchwork.kernel.org/patch/3715391/). Is it OK to use this padding 
for this feature? Or should I perhaps eat just 8bits and limit the delays 
to an arbitrary value of 255us? Or should I not care about that now and let 
somebody else come up with another "bigger" ioctl when they need that 
space?

With kind regards,
Jan
--
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 4/4] spi: orion: Software control for inter-word delays

[Mark Brown]

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

On Thu, Feb 15, 2018 at 04:45:55PM +0100, Jan Kundrát wrote:

> How should the interface look like? Is spi_controller->mode_bits a good
> place for controllers to advertise this feature (SPI_WORD_DELAY, perhaps?)
> as supported, with spi-orion being the only implementation now? If we allow

Yup.

> each spi_transfer to override this value, is there a common place in the SPI
> core which somehow validates a spi_transfer against each controller's
> capabilities? I see a code like that (bad_bits, ugly_bits) in spi_setup
> which looks like something to be called per-device, not per-transfer.

__spi_validate().

> My most important use case is, however, the userspace-facing spidev, and its
> ioctl complicates stuff a bit. The relevant struct has a 16bit padding now
> (used to be 32bit prior to https://patchwork.kernel.org/patch/3715391/). Is
> it OK to use this padding for this feature? Or should I perhaps eat just
> 8bits and limit the delays to an arbitrary value of 255us? Or should I not
> care about that now and let somebody else come up with another "bigger"
> ioctl when they need that space?

Ugh.  Using the whole padding is probably OK.

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

Re: [PATCH 2/4] spi: orion: Respect per-transfer bits_per_word settings

[Trent Piepho]

On Thu, 2018-02-15 at 15:19 +0100, Jan Kundrát wrote:
> It's a bug to look at the device's bits_per_word because each transfer
> can override these settings. In fact, e.g., spidev cannot specify
> "global" bits_per_word and instead relies on passing that through every
> ioctl.
> 
> @@ -427,7 +427,7 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
>  	struct orion_spi *orion_spi;
>  	int cs = spi->chip_select;
>  
> -	word_len = spi->bits_per_word;
> +	word_len = xfer->bits_per_word ? xfer->bits_per_word : spi->bits_per_word;
>  	count = xfer->len;

__spi_validate() already does this.  You can just look at xfer-
>bits_per_word and it will have already been set  to spi->bits_per_word 
if necessary.  This should hold for all the xfer fields.  They are
validated and will incorporate the spi slave's and/or spi master's
default values as necessary.

Re: [PATCH 4/4] spi: orion: Software control for inter-word delays

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1200 bytes --]

On Thu, 2018-02-15 at 15:49 +0000, Mark Brown wrote:
> On Thu, Feb 15, 2018 at 04:45:55PM +0100, Jan Kundrát wrote:
> 
> > My most important use case is, however, the userspace-facing spidev, and its
> > ioctl complicates stuff a bit. The relevant struct has a 16bit padding now
> > (used to be 32bit prior to https://patchwork.kernel.org/patch/3715391/). Is
> > it OK to use this padding for this feature? Or should I perhaps eat just
> > 8bits and limit the delays to an arbitrary value of 255us? Or should I not
> > care about that now and let somebody else come up with another "bigger"
> > ioctl when they need that space?
> 
> Ugh.  Using the whole padding is probably OK.

There could be a flag that switches delay_us from being a post xfer
delay to being a post word delay. Then no need field needs to be added.
 If someone wants both a post word and post xfer delay in the same
xfer, then they'll just need to split it, which should be possible
since it must be longer than one word to need a delay after each word.

Not that great either.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH 4/4] spi: orion: Software control for inter-word delays

[Mark Brown]

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

On Thu, Feb 15, 2018 at 05:18:08PM +0000, Trent Piepho wrote:
> On Thu, 2018-02-15 at 15:49 +0000, Mark Brown wrote:

> > Ugh.  Using the whole padding is probably OK.

> There could be a flag that switches delay_us from being a post xfer
> delay to being a post word delay. Then no need field needs to be added.
>  If someone wants both a post word and post xfer delay in the same
> xfer, then they'll just need to split it, which should be possible
> since it must be longer than one word to need a delay after each word.

> Not that great either.

Yeah, that'd work too but isn't a model of elegance either.  I wonder if
just making a new ioctl() might not be better...  not ideal either but
probably cleaner.

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

Re: Applied "spi: orion: Respect per-transfer bits_per_word settings" to the spi tree

[Mark Brown]

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

On Thu, Feb 15, 2018 at 05:27:06PM +0000, Mark Brown wrote:
> The patch
> 
>    spi: orion: Respect per-transfer bits_per_word settings
> 
> has been applied to the spi tree at
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

Sorry, this was applied in error (as Trent identified) - it got done
later along with some other stuff as I fat fingered some scripting,
dropped again.

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