| Message ID | 20210520135031.2969183-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size | expand |
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.
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.
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?
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/
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.
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)