dtc2278: ->io_32bit setting don't have to be identical for master/slave

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 6 messages Dec 9, 2007 – Dec 10, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-12-09 21:20 UTC #0 lore raw

PATCH 3/20 dtc2278: ->io_32bit setting don't have to be identical for master/slave

drive->io_32bit is used per PIO transfer so there is no reason to require it to be identical for master/slave.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/ide.c            |    4 ----
 drivers/ide/legacy/dtc2278.c |    7 +------
 2 files changed, 1 insertion(+), 10 deletions(-)

drivers/ide/ide.c +0 -4

Index: b/drivers/ide/ide.c
===================================================================
@@ -795,10 +795,6 @@ int set_io_32bit(ide_drive_t *drive, int
 		return -EBUSY;
 
 	drive->io_32bit = arg;
-#ifdef CONFIG_BLK_DEV_DTC2278
-	if (HWIF(drive)->chipset == ide_dtc2278)
-		HWIF(drive)->drives[!drive->select.b.unit].io_32bit = arg;
-#endif /* CONFIG_BLK_DEV_DTC2278 */
 
 	spin_unlock_irq(&ide_lock);

drivers/ide/legacy/dtc2278.c +1 -6

Index: b/drivers/ide/legacy/dtc2278.c
===================================================================
@@ -34,8 +34,7 @@
  * filesystem  corrupted with -u1, but under heavy disk load only :-)
  *
  * This card is now forced to use the "serialize" feature,
- * and irq-unmasking is disallowed.  If io_32bit is enabled,
- * it must be done for BOTH drives on each interface.
+ * and irq-unmasking is disallowed.
  *
  * This code was written for the DTC2278E, but might work with any of these:
  *

@@ -87,11 +86,7 @@ static void dtc2278_set_pio_mode(ide_dri
 		   Winbond but does anyone actually care */
 	}
 
-	/*
-	 * 32bit I/O has to be enabled for *both* drives at the same time.
-	 */
 	drive->io_32bit = 1;
-	HWIF(drive)->drives[!drive->select.b.unit].io_32bit = 1;
 }
 
 static int __init dtc2278_probe(void)

Alan Cox

2007-12-09 22:32 UTC #1 lore raw

Quoted 2 lines

drive->io_32bit is used per PIO transfer so there is no reason to require it to be identical for master/slave.

Sure about that ?

The driver has always gone to great pains to ensure that occurs and that suggests the author had a good reason for doing it. Have you tested this with an actual DTC2278 card ?

If not - NAK. You risk corruptors for no material benefit.

Bartlomiej Zolnierkiewicz

author

2007-12-09 23:06 UTC #2 lore raw

Quoted 10 lines · Alan Cox

Quoted 2 lines
drive->io_32bit is used per PIO transfer so there is no reason to require it to be identical for master/slave.
Sure about that ?
The driver has always gone to great pains to ensure that occurs and that suggests the author had a good reason for doing it. Have you tested this with an actual DTC2278 card ?
If not - NAK. You risk corruptors for no material benefit.

There is absolutely no hardware setup done by IDE subsystem or dtc2278 host driver when ->io_32bit setting is changed and since ->io_32bit setting is only ever checked per drive / PIO transfer so this is currently purely a software limitation (moreover HDIO_DRIVE_CMD and HDIO_DRIVE_TASKFILE ioctls force 16-bit I/O anyway).

[ I also haven't noticed anything related to this requirement in Winbond
W83759A datasheet (though I have only quickly skimmed through it)... ]

Bart

Alan Cox

2007-12-09 23:13 UTC #3 lore raw

Quoted 2 lines

There is absolutely no hardware setup done by IDE subsystem or dtc2278 host driver when ->io_32bit setting is changed and since ->io_32bit setting

So ?

The original author went to great trouble to specifically enforce the behaviour in question and probably did so for a reason. Would it not be far safer to fix it by keeping that enforcement and fixing where it is lost than removing it - especially if you cannot test the hardware ?

Quoted 6 lines

is only ever checked per drive / PIO transfer so this is currently purely a software limitation (moreover HDIO_DRIVE_CMD and HDIO_DRIVE_TASKFILE ioctls force 16-bit I/O anyway).
[ I also haven't noticed anything related to this requirement in Winbond
W83759A datasheet (though I have only quickly skimmed through it)... ]

Nothing in the sheet AFAIK but the DTC is a close relative not the actual winbond

Bartlomiej Zolnierkiewicz

author

2007-12-09 23:43 UTC #4 lore raw

Quoted 7 lines · Alan Cox

Quoted 2 lines
There is absolutely no hardware setup done by IDE subsystem or dtc2278 host driver when ->io_32bit setting is changed and since ->io_32bit setting
So ?
The original author went to great trouble to specifically enforce the behaviour in question and probably did so for a reason. Would it not be

Probably so but please keep in mind the state of IDE subsystem back then...

Quoted 2 lines

far safer to fix it by keeping that enforcement and fixing where it is lost than removing it - especially if you cannot test the hardware ?

What about just enforcing ->io_32bit for both drives in ->init_hwif method and disallowing any futher changes with 'hwif->no_io_32bit = 1'?

Thanks,
Bart

Alan Cox

2007-12-10 00:07 UTC #5 lore raw

Quoted 9 lines · On Mon, 10 Dec 2007 00:52:28 +0100 Bartlomiej Zolnierkiewicz…

Quoted 7 lines · Alan Cox
Quoted 2 lines
There is absolutely no hardware setup done by IDE subsystem or dtc2278 host driver when ->io_32bit setting is changed and since ->io_32bit setting
So ?
The original author went to great trouble to specifically enforce the behaviour in question and probably did so for a reason. Would it not be

Quoted 8 lines

Probably so but please keep in mind the state of IDE subsystem back then...
Quoted 2 lines
far safer to fix it by keeping that enforcement and fixing where it is lost than removing it - especially if you cannot test the hardware ?
What about just enforcing ->io_32bit for both drives in ->init_hwif method and disallowing any futher changes with 'hwif->no_io_32bit = 1'?

Sounds fine to me.

Alan