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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


OK.

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


OK.


+	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)
+{
+}

ata_noop_qc_prep


OK.


+/**
+ * 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) {

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

ata_sff_std_ports ? (at least for the 8bit case)


OK.

Thanks,

David Daney




[Index of Archives]     [Linux MIPS Home]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Linux]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux