Date:   Thu, 20 May 2021 17:54:00 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        linux-spi@...r.kernel.org, Guenter Roeck <linux@...ck-us.net>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller
 with a limited max transfer size

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists