[PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The static config of the sja1105 switch is a long stream of bytes which
is programmed to the hardware in chunks (portions with the chip select
continuously asserted) of max 256 bytes each.

Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
bridge, cannot keep the chip select asserted for that long.
The spi_max_transfer_size() and spi_max_message_size() functions are how
the controller can impose its hardware limitations upon the SPI
peripheral driver.

The sja1105 sends its static config to the SPI master as a huge
scatter/gather spi_message - commit 08839c06e96f ("net: dsa: sja1105:
Switch to scatter/gather API for SPI") contains a description of that.
That spi_message contains the following list of spi_transfers:

                |                         |      cs_change
 spi_transfer # |       Contents          | (deassert chip select)
 ---------------|-------------------------|-----------------------
       1        |   SPI message header 1  |         no
       2        |  Static config chunk 1  |         yes
       3        |   SPI message header 2  |         no
       4        |  Static config chunk 2  |         yes
      ...       |           ...           |         ...

Since what the SPI master does not support is keeping the CS asserted
for more than, say, 200 bytes, we must limit the summed length of the
spi_transfers with cs_change deasserted (1+2, 3+4 etc) lower than 200.

This is a bit fuzzy, but I think the proper way to handle this is to
just look at the spi_max_transfer_size() reported by the master, and
adapt to that. We can disregard the spi_max_message_size() limit since I
suppose that assumes no cs_change - but then again, spi_max_transfer_size
is itself capped by spi_max_message_size, which makes sense.

Regression-tested on a switch connected to a controller with no
limitations (spi-fsl-dspi) as well as with one with caps for both
max_transfer_size and max_message_size (spi-sc18is602).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_spi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index f7a1514f81e8..ac766def45c8 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -46,18 +46,22 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 			sja1105_spi_rw_mode_t rw, u64 reg_addr, u8 *buf,
 			size_t len, struct ptp_system_timestamp *ptp_sts)
 {
+	struct spi_device *spi = priv->spidev;
 	struct sja1105_chunk chunk = {
-		.len = min_t(size_t, len, SJA1105_SIZE_SPI_MSG_MAXLEN),
 		.reg_addr = reg_addr,
 		.buf = buf,
 	};
-	struct spi_device *spi = priv->spidev;
 	struct spi_transfer *xfers;
+	size_t xfer_len;
 	int num_chunks;
 	int rc, i = 0;
 	u8 *hdr_bufs;
 
-	num_chunks = DIV_ROUND_UP(len, SJA1105_SIZE_SPI_MSG_MAXLEN);
+	xfer_len = min_t(size_t, SJA1105_SIZE_SPI_MSG_MAXLEN,
+			 spi_max_transfer_size(spi) - SJA1105_SIZE_SPI_MSG_HEADER);
+
+	num_chunks = DIV_ROUND_UP(len, xfer_len);
+	chunk.len = min(len, xfer_len);
 
 	/* One transfer for each message header, one for each message
 	 * payload (chunk).
@@ -127,7 +131,7 @@ static int sja1105_xfer(const struct sja1105_private *priv,
 		chunk.buf += chunk.len;
 		chunk.reg_addr += chunk.len / 4;
 		chunk.len = min_t(size_t, (ptrdiff_t)(buf + len - chunk.buf),
-				  SJA1105_SIZE_SPI_MSG_MAXLEN);
+				  xfer_len);
 
 		/* De-assert the chip select after each chunk. */
 		if (chunk.len)
-- 
2.25.1

Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Mark Brown]

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

On Thu, May 20, 2021 at 04:50:31PM +0300, Vladimir Oltean wrote:

> Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> bridge, cannot keep the chip select asserted for that long.
> The spi_max_transfer_size() and spi_max_message_size() functions are how
> the controller can impose its hardware limitations upon the SPI
> peripheral driver.

You should respect both, frankly I don't see any advantage to using
cs_change for something like this - just do a bunch of async SPI
transfers and you'll get the same effect in terms of being able to keep
the queue for the controller primed with more robust support since it's
not stressing edge cases.  cs_change is more for doing things that are
just very non-standard.

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

Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Vladimir Oltean]

Hi Mark,

On Thu, May 20, 2021 at 02:56:15PM +0100, Mark Brown wrote:
> On Thu, May 20, 2021 at 04:50:31PM +0300, Vladimir Oltean wrote:
> 
> > Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> > bridge, cannot keep the chip select asserted for that long.
> > The spi_max_transfer_size() and spi_max_message_size() functions are how
> > the controller can impose its hardware limitations upon the SPI
> > peripheral driver.
> 
> You should respect both, frankly I don't see any advantage to using
> cs_change for something like this - just do a bunch of async SPI
> transfers and you'll get the same effect in terms of being able to keep
> the queue for the controller primed with more robust support since it's
> not stressing edge cases.  cs_change is more for doing things that are
> just very non-standard.

Sorry, I don't really understand your comment: in which way would it be
more robust for my use case to use spi_async()?

The cs_change logic was already there prior to this patch, I am just
reiterating how it works. Given the way in which it works (which I think
is correct), the most natural way to limit the buffer length is to look
for the max transfer len.

Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Mark Brown]

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

On Thu, May 20, 2021 at 05:06:09PM +0300, Vladimir Oltean wrote:
> On Thu, May 20, 2021 at 02:56:15PM +0100, Mark Brown wrote:
> > On Thu, May 20, 2021 at 04:50:31PM +0300, Vladimir Oltean wrote:

> > > Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> > > bridge, cannot keep the chip select asserted for that long.
> > > The spi_max_transfer_size() and spi_max_message_size() functions are how
> > > the controller can impose its hardware limitations upon the SPI
> > > peripheral driver.

> > You should respect both, frankly I don't see any advantage to using
> > cs_change for something like this - just do a bunch of async SPI
> > transfers and you'll get the same effect in terms of being able to keep
> > the queue for the controller primed with more robust support since it's
> > not stressing edge cases.  cs_change is more for doing things that are
> > just very non-standard.

> Sorry, I don't really understand your comment: in which way would it be
> more robust for my use case to use spi_async()?

Your description sounds like the driver is just stitching a bunch of
messages together into a single big message with lots of cs_changes with
the goal of improving performance which is really not using the API at
all idiomatically.  That's at best asking for trouble (it'll certainly
work with fewer controllers), it may even be less performant as you're
less likely to get the benefit of framework enhancements.

> The cs_change logic was already there prior to this patch, I am just
> reiterating how it works. Given the way in which it works (which I think

It seems like you could avoid this issue and most likely other future
issues by making the way the driver uses the API more normal.

> is correct), the most natural way to limit the buffer length is to look
> for the max transfer len.

No, you really do need to pay attention to both - what makes you think
it is safe to just ignore one of them?

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

Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Vladimir Oltean]

On Thu, May 20, 2021 at 03:29:47PM +0100, Mark Brown wrote:
> On Thu, May 20, 2021 at 05:06:09PM +0300, Vladimir Oltean wrote:
> > On Thu, May 20, 2021 at 02:56:15PM +0100, Mark Brown wrote:
> > > On Thu, May 20, 2021 at 04:50:31PM +0300, Vladimir Oltean wrote:
> 
> > > > Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> > > > bridge, cannot keep the chip select asserted for that long.
> > > > The spi_max_transfer_size() and spi_max_message_size() functions are how
> > > > the controller can impose its hardware limitations upon the SPI
> > > > peripheral driver.
> 
> > > You should respect both, frankly I don't see any advantage to using
> > > cs_change for something like this - just do a bunch of async SPI
> > > transfers and you'll get the same effect in terms of being able to keep
> > > the queue for the controller primed with more robust support since it's
> > > not stressing edge cases.  cs_change is more for doing things that are
> > > just very non-standard.
> 
> > Sorry, I don't really understand your comment: in which way would it be
> > more robust for my use case to use spi_async()?
> 
> Your description sounds like the driver is just stitching a bunch of
> messages together into a single big message with lots of cs_changes with
> the goal of improving performance which is really not using the API at
> all idiomatically.  That's at best asking for trouble (it'll certainly
> work with fewer controllers), it may even be less performant as you're
> less likely to get the benefit of framework enhancements.

Stitching a bunch of s/messages/transfers/, but yes, that is more or
less correct. I think cs_change is pretty well handled with the SPI
drivers I've had the pleasure so far. The spi-sc18is602.c driver has had
no changes in this area since its introduction in 2012 and it worked out
of the box (well, except for the maximum buffer length limit, which I
was expecting even before I had my hands on the hardware, since it is
explained here:
https://www.kernel.org/doc/html/latest/spi/spi-sc18is602.html).
SPI controllers that don't treat cs_change properly can always be
patched, although there is a sizable user base for this feature at the
moment from what I can see, so the semantics are pretty clear to me (and
the sja1105 is in line with them).

> > The cs_change logic was already there prior to this patch, I am just
> > reiterating how it works. Given the way in which it works (which I think
> 
> It seems like you could avoid this issue and most likely other future
> issues by making the way the driver uses the API more normal.

Does this piece of advice mean "don't use cs_change"? Why does it exist
then? I'm confused. Is it because the max_*_size properties are not well
defined in its presence? Isn't that a problem with the self-consistency
of the SPI API then?

> > is correct), the most natural way to limit the buffer length is to look
> > for the max transfer len.
> 
> No, you really do need to pay attention to both - what makes you think
> it is safe to just ignore one of them?

I think the sja1105 is safe to just ignore the maximum message length
because "it knows what it's doing" (famous last words). The only real
question is "what does .max_message_size count when its containing
spi_transfers have cs_change set?", and while I can perfectly understand
why you'd like to avoid that question, I think my interpretation is the
sane one (it just counts the pieces with continuous CS), and I don't see
the problems that this interpretation can cause down the line.

If you want to, I can just resend the spi-sc18is602 patch without
master->max_message_size implemented, and voila, I'm not ignoring it any
longer :)
https://patchwork.kernel.org/project/spi-devel-general/patch/20210520131238.2903024-3-olteanv@gmail.com/

Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

[Mark Brown]

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

On Thu, May 20, 2021 at 05:54:29PM +0300, Vladimir Oltean wrote:
> On Thu, May 20, 2021 at 03:29:47PM +0100, Mark Brown wrote:

> > Your description sounds like the driver is just stitching a bunch of
> > messages together into a single big message with lots of cs_changes with

> Stitching a bunch of s/messages/transfers/, but yes, that is more or
> less correct. I think cs_change is pretty well handled with the SPI

No, if you're just doing plain transfers that's just a perfectly normal
SPI message (eg, a message consisting of a write only transfer followed
by a read only one for a register read) - the purpose of the cs_change
operations seems to be to glue what should normally be a series of
messages into something that the API sees as a single big message.

> SPI controllers that don't treat cs_change properly can always be
> patched, although there is a sizable user base for this feature at the
> moment from what I can see, so the semantics are pretty clear to me (and
> the sja1105 is in line with them).

It's not always possible to convince controllers to implement cs_change,
sometimes the hardware just isn't flexible enough to do anything other
than assert chip select during a single operation and you can't even put
the chip selects into GPIO mode to do it by hand as there's no pinmuxing.

> > > The cs_change logic was already there prior to this patch, I am just
> > > reiterating how it works. Given the way in which it works (which I think

> > It seems like you could avoid this issue and most likely other future
> > issues by making the way the driver uses the API more normal.

> Does this piece of advice mean "don't use cs_change"? Why does it exist

Yes.

> then? I'm confused. Is it because the max_*_size properties are not well
> defined in its presence? Isn't that a problem with the self-consistency
> of the SPI API then?

cs_change is mainly there mainly for devices which require things like
leaving CS asserted outside of messages, often because the length of the
message on the bus is not known at the beginning of the the message (eg,
the device sends a header back including the length so the physical
message gets split into two Linux level messages with cs_change used to
hold chip select asserted between them).  There's also some things like
zero length transfers that bounce CS which some more esoteric hardware
requires.

> > > is correct), the most natural way to limit the buffer length is to look
> > > for the max transfer len.

> > No, you really do need to pay attention to both - what makes you think
> > it is safe to just ignore one of them?

> I think the sja1105 is safe to just ignore the maximum message length
> because "it knows what it's doing" (famous last words). The only real

No, the maximum message length is a *controller* limitation - if the
controller is unable to handle a message larger than a given size then
it's just going to be unable to handle it.

> question is "what does .max_message_size count when its containing
> spi_transfers have cs_change set?", and while I can perfectly understand
> why you'd like to avoid that question, I think my interpretation is the

max_message_size always means the same thing, it's the maximum length
that can be passed safely in a single spi_message.  Similarly for the
maximum transfer, it's the maximum length that can be passed safely in a
single spi_transfer.  We do have code in the core which will split
transfers up, though the rewriting adds overhead so it's best avoided in
performance sensitive cases.

It would be possible for either the driver (or better the core, as with
transfer splitting this isn't device specific) to try to pattern match
and pick apart *some* usages, though not all and mostly not the ones
that match the intended use of the feature so I'm not convinced it's
worthwhile.  For usages like sending a stream of messages which can be
directly expressed in the API it's going to be much better to have code
that just directly sends a stream of messages, it's a much more common
pattern so is more likely to benefit from optimisations and less likely
to get sent down slow paths.

> sane one (it just counts the pieces with continuous CS), and I don't see
> the problems that this interpretation can cause down the line.

If the device is able to support offloading the chip select changes to
the hardware as part of the data stream (some work by basically sending
streams of command packets to the hardware, and some can do fun stuff
as the DMA controller can chain operations together and is perfectly
capable of writing to the SPI controller registers) or is queuing all
the data transfers in a message as a single big DMA then it may run into
limits with those.

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