Hi Daniel,
On Fri, 2014-02-07 at 16:34 +0000, dsneddon@xxxxxxxxxxxxxx wrote:
> > From: "Ivan T. Ivanov" <iivanov@xxxxxxxxxx>
<snip>
> > +
> > +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.
> > + /*
> > + * 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.
> > +
> > +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.
> > + /*
> > + * 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>
> > +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.
> > + 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
> > + 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
