[Top] [All Lists]

Re: [PATCH] ide: New libata driver for OCTEON SOC Compact Flash interfac

To: Alan Cox <>
Subject: Re: [PATCH] ide: New libata driver for OCTEON SOC Compact Flash interface.
From: David Daney <>
Date: Fri, 21 Nov 2008 09:05:46 -0800
Cc:, linux-mips <>
In-reply-to: <>
Original-recipient: rfc822;
References: <> <>
User-agent: Thunderbird (X11/20080723)
Alan Cox wrote:
+ * Called to enable the use of DMA based on kernel command line.
+ */
+void octeon_cf_enable_dma(void)
+       use_cf_dma = 1;

Why not use the standard module parameter interface ?

I will do that.

+       /*
+        * PIO modes 0-4 all allow the device to deassert IORDY to slow down
+        * the host.
+        */

See ata_timing_compute() which also knows about master/slave timing,
PIO5/6 rules and timing adjustments as well as doing bus clock
quantisations and lengthenings for you.


+       use_iordy = 1;

This depends on the device as well and gets quite complicated. We have
ata_pio_need_iordy() to do the work for you.

+       t1 = (t1 * clocks_us) / 1000 / 2;
+       if (t1)
+               t1--;

Even if you wanted to do it this way you could just use arrays and lookup
tables as many other drivers do - ie

        pio = dev->pio_mode - XFER_PIO_0;
        t1 = data[pio];

The timing calculations are based on the CPU clock rate, It is difficult to encapsulate that in a table.

+       /*
+        * Odd lengths are not supported. We should always be a
+        * multiple of 512.
+        */
+       BUG_ON(buflen & 1);

If you get a request for an odd length you should write an extra word
containing the last byte and one other. See how the standard methods
handle this.


+       if (ocd->is16bit) {

Or you could have two methods and two transfer routines defined

Good idea.

+                       while (words--) {
+                               *(uint16_t *)buffer = ioread16(data_addr);
+                               buffer += sizeof(uint16_t);

By definition tht is 2. Do you have an ioread16_rep ?

It appears to be broken. One would expect ioread16 and ioread16_rep to do endian swapping in the same manner. On MIPS they do not. Perhaps it would be better to fix the problem at the source.

+ * Get ready for a dma operation.  We do nothing, as all DMA
+ * operations are taken care of in octeon_cf_bmdma_start.
+ */
+void octeon_cf_qc_prep(struct ata_queued_cmd *qc)



+ * Check if any queued commands have more DMAs, if so start the next
+ * transfer, else do standard handling.
+ */
+irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance)

A lot of these functions could be static it seems

An oversight on my part.  I will make them all static.

+static void octeon_cf_delayed_irq(unsigned long data)

What stops the following occuring

        ATA irq
                BUSY still set
                Queue tasklet

        Other irq on same line
        ATA busy clear
                Handle command

        Tasklet runs but command was sorted out

(or a reset of the ata controller in the gap)

Probably nothing.  I will try to sort it out.

+       base = cs0 + ocd->base_region_bias;
+       if (!ocd->is16bit) {

+               ap->ioaddr.cmd_addr  = base + ATA_REG_CMD;

ata_sff_std_ports ? (at least for the 8bit case)



David Daney

<Prev in Thread] Current Thread [Next in Thread>