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:
signature.asc
Description: PGP signature
