Re: Fwd: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[dsneddon@codeaurora.org]

Hi Ivan,

>> >> > +static int spi_qup_set_state(struct spi_qup *controller, u32
>> state)
>> >> > +{
>> >> > +       unsigned long loop = 0;
>> >> > +       u32 cur_state;
>> >> > +
>> >> > +       cur_state = readl_relaxed(controller->base + QUP_STATE);
>> >> Make sure the state is valid before you read the current state.
>> >
>> > Why? Controller is always left in valid state (after probe and every
>> > transaction)? I know that CAF code contain this check, but now driver
>> > is little bit different. I have made some tests and controller is
>> > always in valid state in this point.
>>
>> The hardware programming guide we recommends doing this.  I'd have to
>> talk
>> to the hardware designers to know exactly the reason why.  I can follow
>> up
>> on this if you'd like.
>>
>
> Ok, thanks. I could add it back. Please, could you point me to place
> in driver where this could happen.
>
>
>> >
>> >> > +       /*
>> >> > +        * Per spec: for PAUSE_STATE to RESET_STATE, two writes
>> >> > +        * of (b10) are required
>> >> > +        */
>> >> > +       if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
>> >> > +           (state == QUP_STATE_RESET)) {
>> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> >> > QUP_STATE);
>> >> > +               writel_relaxed(QUP_STATE_CLEAR, controller->base +
>> >> > QUP_STATE);
>> >> > +       } else {
>> >> Make sure you don't transition from RESET to PAUSE.
>> >
>> > I don't see this to be handled in CAF code. Could you give me
>> > more details, please?
>> >
>> > Right now possible state transactions are:
>> >
>> > * if everything is fine:
>> >   RESET -> RUN -> PAUSE -> RUN -> RESET
>> > * in case of error:
>> >   RESET -> RUN -> RESET
>> >   RESET -> RUN -> PAUSE -> RESET
>> >
>> > Please correct me if I am wrong.
>>
>> According to the hardware documentation if the hardware is in the RESET
>> state and we try to transition to the PAUSE state the hardware behavior
>> is
>> "undefined", which usually means bad things will happen.  Admittedly, if
>> the driver always follows the "valid" rules (the ones you've listed
>> above)
>> it _should_ never happen.  However, it is _possible_ the hardware is in
>> the RESET state while the driver thinks it's in the RUN state and the
>> driver tries to put is in the PAUSE state.  In other words, I'd like to
>> err on the side of caution since the check doesn't really cost us
>> anything.
>
> Ok that is fine, but did you see where/how this could happen in
> the current implementation. If this could happen I will like to fix it.

I don't see a problem in the current implementation that would cause you
to get in the RESET state without knowing it.  It's just my paranoia
kicking in.

>
>>
>> >
>> >> > +
>> >> > +static void spi_qup_fifo_read(struct spi_qup *controller,
>> >> > +                             struct spi_transfer *xfer)
>> >> > +{
>> >> > +       u8 *rx_buf = xfer->rx_buf;
>> >> > +       u32 word, state;
>> >> > +       int idx, shift;
>> >> > +
>> >> > +       while (controller->rx_bytes < xfer->len) {
>> >> > +
>> >> > +               state = readl_relaxed(controller->base +
>> >> QUP_OPERATIONAL);
>> >> > +               if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
>> >> > +                       break;
>> >> > +
>> >> > +               word = readl_relaxed(controller->base +
>> >> QUP_INPUT_FIFO);
>> >> > +
>> >> > +               for (idx = 0; idx < controller->bytes_per_word &&
>> >> > +                    controller->rx_bytes < xfer->len; idx++,
>> >> > +                    controller->rx_bytes++) {
>> >> > +
>> >> > +                       if (!rx_buf)
>> >> > +                               continue;
>> >> If there is no rx_buf just set rx_bytes to xfer->len and skip the
>> loop
>> >> entirely.
>> >
>> > Well, FIFO buffer still should be drained, right?
>> > Anyway. I am looking for better way to handle this.
>> > Same applies for filling FIFO buffer.
>> >
>>
>> Yes.  Still read from the FIFO but skip the for loop since we're just
>> adding bytes_per_word to the total rx_byte count one iteration at a time
>> and not doing anything with the data.
>>
>
> My point was: If I made rx_bytes equal xfer->len, outer while loop will
> be terminated before FIFO was drained :-). I will see how to handle
> this better.

You're right!  Had to read that snippet again.  Though, I think we can
skip draining the FIFO by setting the NO_INPUT bit in the QUP_CONFIG
register (and the NO_OUTPUT for writes).