On Sat, Mar 29, 2014 at 04:09:10PM +0100, Gerhard Sittig wrote: > The core implements the loop over the transfers of a message > (spi_transfer_one_message() routine). The logic is > - unconditionally assert CS at the start of a message > - invert(?) CS between transfers if the transfer's cs_change > property is set > - deassert CS at the end of the message _if_ transmission was > successful _and_ the caller did not ask to keep CS asserted > (when the last transfer's cs_change is set) > Is my interpretation of the implementation correct? Then I'd > expect "undocumented features" in the current implementation. That's what it's intended to do, yes (and what a bunch of drivers were doing). > Does it mean that with cs_change set, subsequent transfers will > run with CS deasserted? I thought this property would "pulse" > the CS, i.e. release and re-assert the signal. The spi.h kernel > doc suggests so: "(i) If the transfer isn't the last one in the > message, this flag is used to make the chipselect briefly go > inactive in the middle of the message." An approach to adjust > the core's implementation might be to move the set_cs(true) into > the transfer loop, and to set_cs(false) between transfers if > cs_change is set. Or do the set_cs(false) set_cs(true) sequence > in the if() arm depending on how the cost might be perceived. Right, that's what the documentation says and what the bitbang driver does so we should probably update the code to reflect that. > And I was not aware of the effect on the message's end, that one > could leave CS asserted. The spi.h comment reads "(ii) When the > transfer is the last one in the message, the chip may stay > selected until the next transfer.", which fits the > implementation. Then it continues "... this is just a > performance hint; starting a message to another device deselects > this one.", which might assume a specific hardware implementation > and need not apply to all setups in general. I assume that the > documentation is more permissive or suggests more than the > subsystem can enforce. Well, the thing is that for hardware that can't control /CS from software so anything using it between transfers can't rely on it happening at all. Within a transfer you can always paper this over since there's full visibility of what's going to happen but that's not possible over transfers. > And I would like to ask whether determining how CS needs to be > set at the end of a transfer can be done before the transfer_one > callback gets invoked. In the specific MPC512x PSC case, the > appropriate control is done _during_ transmission. The injection > of the MPC512x_PSC_FIFO_EOF txcmd code before feeding more data > into the TX FIFO will make CS go inactive as the data is drained > (gets sent over the wire). So the set_cs() after the transfer's > completion may be late, won't work and/or will disturb other > communication in the case of PSC internal SS lines. Other > hardware may have similar constraints. Those who need not handle > CS during transmission already can ignore the information during > transfer_one(), and use the regular set_cs() invocation. It could be I guess, though to be honest I've never been convinced that supporting set_cs on the final transfer is a sane idea in the first place - my guess is that it's actually going to cost more on average to implement it (what with remembering to deassert if another device is selected and so on) than just unconditionally disabling /CS. > Is it appropriate for the SPI subsystem to modify the passed in > transfer descriptions? In that case the transfer's cs_change It does already. > I might have prepared and sent patches, but wanted to verify that > the issues are present, and how best to address them. Feel free > to tell me if this discussion should have its own thread, to not > end up in the review comments of a patch (although it's RFT). Another thread would've been helpful - I was reading it thinking it was going to explain the poor performance!
Attachment:
signature.asc
Description: Digital signature
