Re: [RESEND] spi/tegra114: Correct support for cs_change

From: Rhyland Klein
Date: Tue Sep 24 2013 - 12:33:59 EST


On 9/23/2013 7:08 PM, Trent Piepho wrote:
> On Mon, Sep 23, 2013 at 2:14 PM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
>>
>> That sounds broken. Normally, shouldn't CS assert before a transaction,
>> stay asserted during a transaction, then deassert after the transaction?
>> It shouldn't rise and fall very quickly in between parts of the transaction.
>
> That is normal, where a transaction is a spi_message made up of
> multiple spi_transfers. The cs_change bit for a transfer will insert
> a de-asserted pulse after a transfer or leave CS de-asserted after the
> last transfer.
>
>>>>> need to generate a falling-edge to trigger the beginning of a SPI
>>>>> transaction. Doing this write with the default value of SPI_COMMAND1
>>>>> causes a brief rise and fall of CS, giving us our falling-edge.
>
> I wonder, is the real problem that the spi layer allows CS to possibly
> remain asserted between transactions to the same device? Normally you
> would expect it to be de-asserted at the end of a spi_message, but I
> believe the Linux spi semantics are that it may or may not actually be
> de-asserted at that time. It only guarantees that is will be
> de-asserted before a message to a different device starts.
>
> I guess this is supposed to be an optimization. Some drivers, like
> gpio bit-banging, probably have a cost associated with any CS change.
> Usually many messages in a row are to the same device. Most devices
> don't care if CS pulses between messages. Thus not pulsing CS between
> each message is faster.
>
>>>
>>> Otherwise, this logic allows us to skip the spi of COMMAND1 which would
>>> normally be used to create the falling edge to start a new transaction,
>>> leaving the previous one open for more transfers.
>>
>> This sounds like something the SPI core should be managing. If a driver
>> is using the SPI bus to communicate with a device in a way that needs CS
>> left active while outside a transaction, it shouldn't be possible for
>> another driver to come along and communicate with another device before
>> the first transaction is all complete. The bus should be locked.
>> Allowing anything else could corrupt the protocol that required specific
>> CS states outside the transaction.
>
> If the transaction is one message, which can be multiple transfers and
> multiple CS pulses, then the spi core always does it atomically. The
> limitation is the driver can't get the result of the transaction until
> the entire transaction is finished.
>
> If a driver needs to get part of a transaction to complete the rest,
> e.g. read a 16-bit length from the device and then read that many
> bytes, it can still be done. It doesn't seem to be documented in
> spi-summary, but the way to do this is with spi_bus_(un)lock() and
> spi_(a)sync_locked() calls. The driver must lock the bus, used the
> _locked versions to issue spi_messages, then unlock when done. This
> should prevent another device on the bus from getting a messages, and
> thus CS pulses, in the middle of the transaction.

I suppose I can should reword my comment then on the code which checked:

+ if (tspi->cs_control) {
+ if (tspi->cs_control != spi)
+ tegra_spi_writel(tspi, command1, SPI_COMMAND1);

While this does do exactly what I said, ending a previous "on-going"
transaction in favor of a new one, this shouldn't be expected to be the
way for clients to guarantee that they have locked a bus. This is more
of a way internally to the Tegra SPI driver to clear its state.

-rhyland
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/