Re: [PATCH][pata-2.6 tree] pdc202xx_old: rewrite mode programmingcode

From: Sergei Shtylyov
Date: Mon Mar 05 2007 - 15:51:35 EST


Hello.

Bartlomiej Zolnierkiewicz wrote:

[PATCH] pdc202xx_old: rewrite mode programming code

This patch is based on the documentation (I would like to thank Promise
for it) and also partially on the older vendor driver.

Rewrite mode programming code:

* fix XFER_MW_DMA0 timings (they were overclocked, use the official ones)

official == same as in the docs and vendor driver :-)

Erm, those look a bit doubtful...

I believe that they are correct - please see explanations below.

Yeah, sorry about that. Only SWDMA timings are suspicious.

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
[...]
@@ -107,52 +70,23 @@ static int pdc202xx_tune_chipset (ide_dr
u8 drive_pci = 0x60 + (drive->dn << 2);
u8 speed = ide_rate_filter(drive, xferspeed);

- u32 drive_conf;
- u8 AP, BP, CP, DP;
+ u32 drive_conf = 0;
+ u8 AP = 0, BP = 0, CP = 0;
u8 TA = 0, TB = 0, TC = 0;

- if (drive->media != ide_disk &&
- drive->media != ide_cdrom && speed < XFER_SW_DMA_0)
- return -1;
+ /*
+ * TODO: do this once per channel
+ */
+ if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
+ pdc_old_disable_66MHz_clock(hwif);

pci_read_config_dword(dev, drive_pci, &drive_conf);

This function never uses it as u32 entity, I wonder why read it? Just to hush a warning? :-)

It is used for debugging purposes by PDC202XX_DEBUG_DRIVE_INFO
(it prints old/new content of drive configuration registers).

Ah, indeed. That printk() didn't look obvious... :-)

I think that I'll cover it by #if PDC202XX_DEBUG_DRIVE_INFO to make
the aforementioned fact clear and to optimize non-debug case a bit...

switch(speed) {
- case XFER_UDMA_6: speed = XFER_UDMA_5;
case XFER_UDMA_5:
case XFER_UDMA_4: TB = 0x20; TC = 0x01; break;

The same clocks for UDMA4/5... I wonder if PDC20265/7 indeed supported UDMA5 (as I'm not seeing any extra clock switching for this mode)?

Probably chipset snoops WIN_SETFEATURES (w/ SETFEATURES_XFER subcommand)
and sets the appropriate timings internally. It might be possible to drop
the timing setup completely for UDMA modes but the vendor driver actually
does it so I left it alone for now.

Actually, their BIOSes also do it and even do some fixups to the table with timings (I forgot on which condition), sooo.. snooping doesn't seem likely (although possible, judging on the UDMA/MWDMA issue).

case XFER_UDMA_2: TB = 0x20; TC = 0x01; break;
@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
case XFER_UDMA_0:
case XFER_MW_DMA_2: TB = 0x60; TC = 0x03; break;
case XFER_MW_DMA_1: TB = 0x60; TC = 0x04; break;
- case XFER_MW_DMA_0:
+ case XFER_MW_DMA_0: TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles (MWDMA1/2 settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

The calculations are done in a different way so we get the correct timings:

7 cycles (== 210 ns) are used for active time
16 cycles (== 480 ns) are used for cycle time

Ah, indeed, I've erred in MWDMA1/2 calculations. This makes sense then.

These timings are the maximum possible ones using MB[2:0] and MC[3:0]
(please refer to the comments in the code to see how MB/MC map to TB/TC).

Yeah, I've taken that into account of course.

case XFER_SW_DMA_2: TB = 0x60; TC = 0x05; break;

Well, this don't look right to me -- we need longer active time (given that my hypothesis is true)

MB[2:0] and MC[3:0] are for MWDMA/UDMA timings only
(it is impossible to set SWDMA0/1 timings using them).

That only strenghtens my belief that SWDMA was never intended to be working on these chips, and should just be dropped (or only SWDMA2 supported).

I suppose that PA[3:0] and PB[4:0] (PIO timings) should be used for SWDMA.

This seems doubtful bit of course you can never predict what the broken- minded chip designers could come up with... :-)

case XFER_SW_DMA_1: TB = 0x80; TC = 0x06; break;

This looks more fitting for SWDMA1 -- however, the recovery time seems to be overly long. It certainly doesn't look like SWDMA1 unless the active/recover times are not in clock cycles (should be 8 cycles, not 4 or 6).

case XFER_SW_DMA_0: TB = 0xC0; TC = 0x0B; break;

Same here -- should be 16 cycles both for active and recovery...

Fixing SWDMA was not a goal of my changes (my patch is already quite
overloaded) but I would happily welcome the incremental patch doing it.

[ I'm also aware that it may difficult without docs so it still on my
personal TODO if nobody beats my to it earlier. ]

I'd need to find time and get to the real hardware as a mimumum (which I haven't been able to do in the past months... :-)

Thanks,
Bart

MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/