Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Atsushi Nemoto wrote:

>>>+		if (drive != &hwif->drives[i] &&
>>>+		    (hwif->drives[i].dev_flags & IDE_DFLAG_PRESENT))
>>>+			safe_speed = min(safe_speed,
>>>+					 (int)hwif->drives[i].current_speed);

>>    You shouldn't clamp the command PIO mode timings like this, and shouldn't 
>>do it at all when DMA mode is set. Call ide_get_best_pio_mode(255, 4) to get 
>>the mate drive's fastest PIO mode which should be a clamping value.

>>>+	/* Command Transfer Mode Select */
>>>+	switch (safe_speed) {
>>>+	case XFER_UDMA_5:
>>>+	case XFER_UDMA_4:
>>>+	case XFER_UDMA_3:
>>>+	case XFER_UDMA_2:
>>>+	case XFER_UDMA_1:
>>>+	case XFER_UDMA_0:
>>>+	case XFER_MW_DMA_2:

>>    You shouldn't change the command PIO mode when DMA mode is selected.

> But the "Command Transfer Mode Select" bits affects access timings on
> setting task registers for DMA command.

    So what? PIO and DMA are different protocols on IDE bus, so they shouldn't 
affect each other. The IDE core will always tune the best PIO mode for you, so 
the optimal command timings will be set.

>  Hmm... do you mean I should not do it _here_?

    You should only change command PIO timings only when PIO mode is changed.

>>>+	case XFER_MW_DMA_1:
>>>+	case XFER_MW_DMA_0:
>>>+	case XFER_PIO_4:

>>    MWDMA0/1 timings don't match PIO4, they are [much] slower.

> Oh thanks.  I will fix it.

    Just do not change PIO mode when selecitng DMA mode at all.

>>>+		hwif->select_data =
>>>+			(hwif->select_data & ~0xffff0000) | (value << 16);

>>    Why not just 0x0000ffff?

>>>+	else
>>>+		hwif->select_data = (hwif->select_data & ~0x0000ffff) | value;

>>    Why not just 0xffff0000?

> Indeed.

    Acltually, this is somewhat wrong WRT the programming the command PIO 
timings in the bits 8..10: they should be set to the same value (matching to 
the last "safest" PIO mode set) for both drives, so you should only "switch" 
bits 4 thru 7 of this register.

MBR, Sergei