On Monday 18 February 2008, Andrea Paterniani wrote:
> This patch fixes 2 bugs:
> > flush() function revised.
> > End of transfers management revised.
But what bugs were fixed?? "Revised" is useless to anyone trying
to understand changes in a driver.
> > Some cosmetics changes.
Again, describe these...
>
> Signed-off-by: Andrea Paterniani <[EMAIL PROTECTED]>
> ---
>
> diff -uprN -X linux-2.6.25-rc2/Documentation/dontdiff
> linux-2.6.25-rc2/drivers/spi/spi_imx.c
> linux-2.6.25-rc2-spi_imx/drivers/spi/spi_imx.c
> --- linux-2.6.25-rc2/drivers/spi/spi_imx.c 2008-02-18 15:44:24.000000000
> +0100
> +++ linux-2.6.25-rc2-spi_imx/drivers/spi/spi_imx.c 2008-02-18
> 15:44:24.000000000 +0100
> @@ -270,19 +270,26 @@ struct chip_data {
>
> static void pump_messages(struct work_struct *work);
>
> -static int flush(struct driver_data *drv_data)
> +static void flush(struct driver_data *drv_data)
> {
> - unsigned long limit = loops_per_jiffy << 1;
> - void __iomem *regs = drv_data->regs;
> - volatile u32 d;
> + void __iomem *regs = drv_data->regs;
Something's odd with your patch generation; that "regs = ..." line
didn't change at all.
> + u32 control;
>
> dev_dbg(&drv_data->pdev->dev, "flush\n");
> +
> + /* Wait for end of transaction */
> do {
> - while (readl(regs + SPI_INT_STATUS) & SPI_STATUS_RR)
> - d = readl(regs + SPI_RXDATA);
> - } while ((readl(regs + SPI_CONTROL) & SPI_CONTROL_XCH) && limit--);
No endless looping, good (although done badly, since the
timeout should be fixed and not dependent on whatever 2/HZ
happens to be) ...
> + control = readl(regs + SPI_CONTROL);
> + } while (control & SPI_CONTROL_XCH);
... replaced by endless looping?? Bad!! Please restore
a limit mechanism. (Fixing it would be good.)
> +
> + /* Release chip select if requested, transfer delays are
> + handled in pump_transfers */
> + if (drv_data->cs_change)
> + drv_data->cs_control(SPI_CS_DEASSERT);
>
> - return limit;
> + /* Disable SPI to flush FIFOs */
> + writel(control & ~SPI_CONTROL_SPIEN, regs + SPI_CONTROL);
> + writel(control, regs + SPI_CONTROL);
I'm not following. If you're going to flush FIFOs, that
needs to be done before deasserting chipselect. But on the
other hand, once the transaction has ended, why would a FIFO
possibly be non-empty? If the idea is to discard RX data,
that should show up in the comments... and maybe even as a
better function name. (You're changing all callers anyway,
to have no return value, so fixing the name is easy.)
> }
>
> static void restore_state(struct driver_data *drv_data)
> @@ -570,6 +577,7 @@ static void giveback(struct spi_message
> writel(0, regs + SPI_INT_STATUS);
> writel(0, regs + SPI_DMA);
>
> + /* Unconditioned deselct */
> drv_data->cs_control(SPI_CS_DEASSERT);
>
> message->state = NULL;
> @@ -592,13 +600,10 @@ static void dma_err_handler(int channel,
> /* Disable both rx and tx dma channels */
> imx_dma_disable(drv_data->rx_channel);
> imx_dma_disable(drv_data->tx_channel);
> -
> - if (flush(drv_data) == 0)
> - dev_err(&drv_data->pdev->dev,
> - "dma_err_handler - flush failed\n");
> -
> unmap_dma_buffers(drv_data);
>
> + flush(drv_data);
> +
> msg->state = ERROR_STATE;
> tasklet_schedule(&drv_data->pump_transfers);
> }
> @@ -612,8 +617,7 @@ static void dma_tx_handler(int channel,
> imx_dma_disable(channel);
>
> /* Now waits for TX FIFO empty */
> - writel(readl(drv_data->regs + SPI_INT_STATUS) | SPI_INTEN_TE,
> - drv_data->regs + SPI_INT_STATUS);
> + writel(SPI_INTEN_TE, drv_data->regs + SPI_INT_STATUS);
That doesn't exactly "wait" for anything does it? Comment needs fixing...
> }
>
> static irqreturn_t dma_transfer(struct driver_data *drv_data)
> @@ -621,19 +625,17 @@ static irqreturn_t dma_transfer(struct d
> u32 status;
> struct spi_message *msg = drv_data->cur_msg;
> void __iomem *regs = drv_data->regs;
> - unsigned long limit;
>
> status = readl(regs + SPI_INT_STATUS);
>
> - if ((status & SPI_INTEN_RO) && (status & SPI_STATUS_RO)) {
> + if ((status & (SPI_INTEN_RO | SPI_STATUS_RO)) == (SPI_INTEN_RO |
> SPI_STATUS_RO)) {
> writel(status & ~SPI_INTEN, regs + SPI_INT_STATUS);
>
> + imx_dma_disable(drv_data->tx_channel);
> imx_dma_disable(drv_data->rx_channel);
> unmap_dma_buffers(drv_data);
>
> - if (flush(drv_data) == 0)
> - dev_err(&drv_data->pdev->dev,
> - "dma_transfer - flush failed\n");
> + flush(drv_data);
>
> dev_warn(&drv_data->pdev->dev,
> "dma_transfer - fifo overun\n");
> @@ -649,20 +651,16 @@ static irqreturn_t dma_transfer(struct d
>
> if (drv_data->rx) {
> /* Wait end of transfer before read trailing data */
> - limit = loops_per_jiffy << 1;
> - while ((readl(regs + SPI_CONTROL) & SPI_CONTROL_XCH) &&
> - limit--);
> -
> - if (limit == 0)
> - dev_err(&drv_data->pdev->dev,
> - "dma_transfer - end of tx failed\n");
> - else
> - dev_dbg(&drv_data->pdev->dev,
> - "dma_transfer - end of tx\n");
> + while (readl(regs + SPI_CONTROL) & SPI_CONTROL_XCH);
Again ... there *SHOULD* be a limit on such looping.
It shouldn't be 2/HZ, though having it depend on the
clock rate used to talk to the device may make sense.
It's also good style to have cpu_relax() inside such
loops.
>
> imx_dma_disable(drv_data->rx_channel);
> unmap_dma_buffers(drv_data);
>
> + /* Release chip select if requested, transfer delays are
> + handled in pump_transfers() */
> + if (drv_data->cs_change)
> + drv_data->cs_control(SPI_CS_DEASSERT);
> +
> /* Calculate number of trailing data and read them */
> dev_dbg(&drv_data->pdev->dev,
> "dma_transfer - test = 0x%08X\n",
> @@ -676,19 +674,12 @@ static irqreturn_t dma_transfer(struct d
> /* Write only transfer */
> unmap_dma_buffers(drv_data);
>
> - if (flush(drv_data) == 0)
> - dev_err(&drv_data->pdev->dev,
> - "dma_transfer - flush failed\n");
> + flush(drv_data);
> }
>
> /* End of transfer, update total byte transfered */
> msg->actual_length += drv_data->len;
>
> - /* Release chip select if requested, transfer delays are
> - handled in pump_transfers() */
> - if (drv_data->cs_change)
> - drv_data->cs_control(SPI_CS_DEASSERT);
> -
> /* Move to next transfer */
> msg->state = next_transfer(drv_data);
>
> @@ -711,44 +702,43 @@ static irqreturn_t interrupt_wronly_tran
>
> status = readl(regs + SPI_INT_STATUS);
>
> - while (status & SPI_STATUS_TH) {
> + if (status & SPI_INTEN_TE) {
> + /* TXFIFO Empty Interrupt on the last transfered word */
> + writel(status & ~SPI_INTEN, regs + SPI_INT_STATUS);
> dev_dbg(&drv_data->pdev->dev,
> - "interrupt_wronly_transfer - status = 0x%08X\n",
> status);
> + "interrupt_wronly_transfer - end of tx\n");
>
> - /* Pump data */
> - if (write(drv_data)) {
> - writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
> - regs + SPI_INT_STATUS);
> + flush(drv_data);
>
> - dev_dbg(&drv_data->pdev->dev,
> - "interrupt_wronly_transfer - end of tx\n");
> + /* Update total byte transfered */
> + msg->actual_length += drv_data->len;
>
> - if (flush(drv_data) == 0)
> - dev_err(&drv_data->pdev->dev,
> - "interrupt_wronly_transfer - "
> - "flush failed\n");
> + /* Move to next transfer */
> + msg->state = next_transfer(drv_data);
>
> - /* End of transfer, update total byte transfered */
> - msg->actual_length += drv_data->len;
> + /* Schedule transfer tasklet */
> + tasklet_schedule(&drv_data->pump_transfers);
>
> - /* Release chip select if requested, transfer delays are
> - handled in pump_transfers */
> - if (drv_data->cs_change)
> - drv_data->cs_control(SPI_CS_DEASSERT);
> + return IRQ_HANDLED;
> + } else {
> + while (status & SPI_STATUS_TH) {
> + dev_dbg(&drv_data->pdev->dev,
> + "interrupt_wronly_transfer - status = 0x%08X\n",
> + status);
>
> - /* Move to next transfer */
> - msg->state = next_transfer(drv_data);
> + /* Pump data */
> + if (write(drv_data)) {
> + /* End of TXFIFO writes,
> + now wait until TXFIFO is empty */
> + writel(SPI_INTEN_TE, regs + SPI_INT_STATUS);
> + return IRQ_HANDLED;
> + }
>
> - /* Schedule transfer tasklet */
> - tasklet_schedule(&drv_data->pump_transfers);
> + status = readl(regs + SPI_INT_STATUS);
>
> - return IRQ_HANDLED;
> + /* We did something */
> + handled = IRQ_HANDLED;
> }
> -
> - status = readl(regs + SPI_INT_STATUS);
> -
> - /* We did something */
> - handled = IRQ_HANDLED;
> }
>
> return handled;
> @@ -758,45 +748,31 @@ static irqreturn_t interrupt_transfer(st
> {
> struct spi_message *msg = drv_data->cur_msg;
> void __iomem *regs = drv_data->regs;
> - u32 status;
> + u32 status, control;
> irqreturn_t handled = IRQ_NONE;
> unsigned long limit;
>
> status = readl(regs + SPI_INT_STATUS);
>
> - while (status & (SPI_STATUS_TH | SPI_STATUS_RO)) {
> + if (status & SPI_INTEN_TE) {
> + /* TXFIFO Empty Interrupt on the last transfered word */
> + writel(status & ~SPI_INTEN, regs + SPI_INT_STATUS);
> dev_dbg(&drv_data->pdev->dev,
> - "interrupt_transfer - status = 0x%08X\n", status);
> -
> - if (status & SPI_STATUS_RO) {
> - writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
> - regs + SPI_INT_STATUS);
> -
> - dev_warn(&drv_data->pdev->dev,
> - "interrupt_transfer - fifo overun\n"
> - " data not yet written = %d\n"
> - " data not yet read = %d\n",
> - data_to_write(drv_data),
> - data_to_read(drv_data));
> -
> - if (flush(drv_data) == 0)
> - dev_err(&drv_data->pdev->dev,
> - "interrupt_transfer - flush failed\n");
> -
> - msg->state = ERROR_STATE;
> - tasklet_schedule(&drv_data->pump_transfers);
> + "interrupt_transfer - end of tx\n");
>
> - return IRQ_HANDLED;
> - }
> -
> - /* Pump data */
> - read(drv_data);
> - if (write(drv_data)) {
> - writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
> - regs + SPI_INT_STATUS);
> + if (msg->state == ERROR_STATE) {
> + /* RXFIFO overrun was detected and message aborted */
> + flush(drv_data);
> + } else {
> + /* Wait for end of transaction */
> + do {
> + control = readl(regs + SPI_CONTROL);
> + } while (control & SPI_CONTROL_XCH);
>
> - dev_dbg(&drv_data->pdev->dev,
> - "interrupt_transfer - end of tx\n");
> + /* Release chip select if requested, transfer delays are
> + handled in pump_transfers */
> + if (drv_data->cs_change)
> + drv_data->cs_control(SPI_CS_DEASSERT);
>
> /* Read trailing bytes */
> limit = loops_per_jiffy << 1;
> @@ -810,27 +786,54 @@ static irqreturn_t interrupt_transfer(st
> dev_dbg(&drv_data->pdev->dev,
> "interrupt_transfer - end of rx\n");
>
> - /* End of transfer, update total byte transfered */
> + /* Update total byte transfered */
> msg->actual_length += drv_data->len;
>
> - /* Release chip select if requested, transfer delays are
> - handled in pump_transfers */
> - if (drv_data->cs_change)
> - drv_data->cs_control(SPI_CS_DEASSERT);
> -
> /* Move to next transfer */
> msg->state = next_transfer(drv_data);
> + }
>
> - /* Schedule transfer tasklet */
> - tasklet_schedule(&drv_data->pump_transfers);
> + /* Schedule transfer tasklet */
> + tasklet_schedule(&drv_data->pump_transfers);
>
> - return IRQ_HANDLED;
> - }
> + return IRQ_HANDLED;
> + } else {
> + while (status & (SPI_STATUS_TH | SPI_STATUS_RO)) {
> + dev_dbg(&drv_data->pdev->dev,
> + "interrupt_transfer - status = 0x%08X\n",
> + status);
> +
> + if (status & SPI_STATUS_RO) {
> + /* RXFIFO overrun, abort message end wait
> + until TXFIFO is empty */
> + writel(SPI_INTEN_TE, regs + SPI_INT_STATUS);
> +
> + dev_warn(&drv_data->pdev->dev,
> + "interrupt_transfer - fifo overun\n"
> + " data not yet written = %d\n"
> + " data not yet read = %d\n",
> + data_to_write(drv_data),
> + data_to_read(drv_data));
> +
> + msg->state = ERROR_STATE;
> +
> + return IRQ_HANDLED;
> + }
>
> - status = readl(regs + SPI_INT_STATUS);
> + /* Pump data */
> + read(drv_data);
> + if (write(drv_data)) {
> + /* End of TXFIFO writes,
> + now wait until TXFIFO is empty */
> + writel(SPI_INTEN_TE, regs + SPI_INT_STATUS);
> + return IRQ_HANDLED;
> + }
>
> - /* We did something */
> - handled = IRQ_HANDLED;
> + status = readl(regs + SPI_INT_STATUS);
> +
> + /* We did something */
> + handled = IRQ_HANDLED;
> + }
> }
>
> return handled;
>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
