Hi Daniel,

On Fri, 2014-02-07 at 16:34 +0000, dsneddon-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote: 

quoted

From: "Ivan T. Ivanov" <redacted>

<snip>

quoted

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

quoted

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

quoted

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

quoted

+                       /*
+                        * The data format depends on bytes_per_word:
+                        *  4 bytes: 0x12345678
+                        *  2 bytes: 0x00001234
+                        *  1 byte : 0x00000012
+                        */
+                       shift = BITS_PER_BYTE;
+                       shift *= (controller->bytes_per_word - idx - 1);
+                       rx_buf[controller->rx_bytes] = word >> shift;
+               }
+       }
+}

<snip>

quoted

+static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
+{
+       struct spi_qup *controller = dev_id;
+       struct spi_transfer *xfer;
+       u32 opflags, qup_err, spi_err;
+
+       xfer = controller->xfer;
+
+       qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
+       spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
+       opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
+
+       writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
+       writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
+       writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
+
+       if (!xfer)
+               return IRQ_HANDLED;
+
+       if (qup_err) {
+               if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
+                       dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
+               if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
+                       dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
+               if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
+                       dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
+               if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
+                       dev_warn(controller->dev, "INPUT_OVER_RUN\n");
+
+               controller->error = -EIO;
+       }
+
+       if (spi_err) {
+               if (spi_err & SPI_ERROR_CLK_OVER_RUN)
+                       dev_warn(controller->dev, "CLK_OVER_RUN\n");
+               if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
+                       dev_warn(controller->dev, "CLK_UNDER_RUN\n");
+
+               controller->error = -EIO;
+       }
+
+       if (opflags & QUP_OP_IN_SERVICE_FLAG) {
+               writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+                      controller->base + QUP_OPERATIONAL);

Write is not necessary since already cleared above.

quoted

+               spi_qup_fifo_read(controller, xfer);
+       }
+
+       if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
+               writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
+                      controller->base + QUP_OPERATIONAL);

Write is not necessary since already cleared above.

True. I have forgot to remove these writes. Will fix.

Regards,
Ivan

quoted

+               spi_qup_fifo_write(controller, xfer);
+       }
+
+       if (controller->rx_bytes == xfer->len ||
+           controller->error)
+               complete(&controller->done);
+
+       return IRQ_HANDLED;
+}
+

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html