pata_it821x: sync with IDE it821x driver

* (partially) fix DMA in RAID mode

Code intended to check DMA status was checking DMA command register. Moreover firmware seems to "forget" to set DMA capable bit for the slave device (at least in RAID mode but without ITE RAID volumes) so check device ID for DMA capable bit when deciding whether to use DMA and remove DMA status check completely.

Thanks to Pavol Šimo for the bugreport and testing the initial fix.

This change unfortunately still doesn't fix DMA in RAID mode (which works fine with IDE it821x) but Alan is working on the missing pieces (pata_it821x vs libata EH issues).

Hello,

Quoted 8 lines · Bartlomiej Zolnierkiewicz

* (partially) fix DMA in RAID mode

Code intended to check DMA status was checking DMA command register. Moreover firmware seems to "forget" to set DMA capable bit for the slave device (at least in RAID mode but without ITE RAID volumes) so check device ID for DMA capable bit when deciding whether to use DMA and remove DMA status check completely.

Quoted 1 line

Thanks to Pavol Šimo for the bugreport and testing the initial fix.

Ah... This is the mysterious mwdma configuration in smart mode, right?

Thanks for fixing this.

Quoted 3 lines

This change unfortunately still doesn't fix DMA in RAID mode (which works fine with IDE it821x) but Alan is working on the missing pieces (pata_it821x vs libata EH issues).

This is the lbal/nsect SRST problem, right?

drivers/ata/pata_it821x.c static void it821x_passthru_set_piomode( · line 269

Quoted 13 lines

drivers/ata/pata_it821x.c +7 -1

@@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
 	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
 
 	struct it821x_dev *itdev = ap->private_data;
+	struct ata_device *pair = ata_dev_pair(adev);
 	int unit = adev->devno;
-	int mode_wanted = adev->pio_mode - XFER_PIO_0;
+	int mode_wanted = adev->pio_mode;
+
+	if (pair && adev->pio_mode > pair->pio_mode)
+		mode_wanted = pair->pio_mode;
+
+	mode_wanted -= XFER_PIO_0;

I think this is better done by mode_filter callback which is guaranteed to be called before any actual mode configuration is performed and in device order (master then slave).

Quoted 16 lines

drivers/ata/pata_it821x.c static void it821x_passthru_set_piomode( · line 269

Quoted 13 lines

drivers/ata/pata_it821x.c +7 -1

@@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
 	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
 
 	struct it821x_dev *itdev = ap->private_data;
+	struct ata_device *pair = ata_dev_pair(adev);
 	int unit = adev->devno;
-	int mode_wanted = adev->pio_mode - XFER_PIO_0;
+	int mode_wanted = adev->pio_mode;
+
+	if (pair && adev->pio_mode > pair->pio_mode)
+		mode_wanted = pair->pio_mode;
+
+	mode_wanted -= XFER_PIO_0;

I think this is better done by mode_filter callback which is guaranteed to be called before any actual mode configuration is performed and in device order (master then slave).

I was thinking about using ->mode_filter but since all other PATA host drivers are doing PIO filtering in ->set_piomode methods and also since it seemed that using ->mode_filter method would result in slightly more complex code I opted for coherency / simplicity. However if you (or somebody else) want to make a follow-up change to this driver (and preferably other PATA host drivers ie. pata_sil680) to use ->mode_filter I'm also fine with that.

Hmmm... indeed. Alan, is there any reason we do that in ->set_piomode not ->mode_filter? We end up with mismatching configuration between the controller and the higher speed drive.

drivers/ata/pata_it821x.c static void it821x_passthru_set_piomode( · line 269

Quoted 13 lines

drivers/ata/pata_it821x.c +7 -1

@@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
 	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
 
 	struct it821x_dev *itdev = ap->private_data;
+	struct ata_device *pair = ata_dev_pair(adev);
 	int unit = adev->devno;
-	int mode_wanted = adev->pio_mode - XFER_PIO_0;
+	int mode_wanted = adev->pio_mode;
+
+	if (pair && adev->pio_mode > pair->pio_mode)
+		mode_wanted = pair->pio_mode;
+
+	mode_wanted -= XFER_PIO_0;

NAK this too

Please see it821x_program.

it821x_passthru_set_piomode computes the required mode and the preferred clock for the mode
it821x_clock_strategy the computes the best clock to use it821x_program the programs the device

Whenever we issue a command it821x_passthru_dev_select then does a timing mode switch and loads the PIO timing before doing the dev_select. The same is also done when doing a qc_issue_prot so each qc_issue will have the right timings loaded for the device.

So if you've got a problem case your diagnosis isn't one that makes a lot of sense, and the fix is clearly wrong but I've not seen most of the discussion between you and whoever you worked with to judge why.

One obvious possibility is that we are not currently doing timing clipping for the address setup and merge for the 8bit timings. However the docs I have strongly imply that the IT821x PIO/MWDMA timing register is for data cycles only.

The other would be that there is some kind of pattern of interaction between the ATA midlayer and the driver causing the wrong timings to get loaded at some point.

Your "fix" would happen to work for both of these cases, but is wrong for both of them.

If the register is switching all the timings then the fix is to switch the PIO timing loaded to the lowest of the two initially (in _program) and flip it back to the correct value at the start of the data transfer sequence. We can then flip it back next qc_issue or indeed at the end of the data xfer

If we've got an interaction problem then we need proper traces with LIBATA debug turned right up so we can fix the actual case where the switching occurs wrongly.

Quoted 1 line

NAK this too

OK, I'm able to understand the meaning of "NAK" [1] but "this too"?

PIO fix was directly ported from my it821x.c patch. I now see that thanks to it821x_passthru_dev_select() it is not really needeed in pata_it821x.c.