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.

>
>> > +       /*
>> > +        * 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.

>
>> > +
>> > +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.

Thanks,
Dan