au1xxx-ide: use ide_tune_dma()

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 9 messages Aug 4, 2007 – Aug 11, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-08-04 20:16 UTC #0 lore raw

PATCH 3/11 au1xxx-ide: use ide_tune_dma()

* Add ->mdma_filter to ide_hwif_t and use it in ide_get_mode_mask().

* Remove needless setting of drive->using_dma from auide_dma_check().

* Split off auide_mdma_filter() from auide_dma_check().

* Use ide_tune_dma() in auide_dma_check(), this fixes following issues:
- device's DMA capability bit not being checked
- device not being checked against generic DMA blacklist
- transfer mode not being set on device/host

* Add PIO autotune fallback to auide_dma_check().

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

---
 drivers/ide/ide-dma.c         |    3 +++
 drivers/ide/ide.c             |    1 +
 drivers/ide/mips/au1xxx-ide.c |   36 ++++++++++++++++++++++--------------
 include/linux/ide.h           |    1 +
 4 files changed, 27 insertions(+), 14 deletions(-)

drivers/ide/ide-dma.c +3 -0

Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -677,6 +677,9 @@ static unsigned int ide_get_mode_mask(id
 	case XFER_MW_DMA_0:
 		if (id->field_valid & 2)
 			mask = id->dma_mword & hwif->mwdma_mask;
+
+		if (hwif->mdma_filter)
+			mask &= hwif->mdma_filter(drive);
 		break;
 	case XFER_SW_DMA_0:
 		if (id->field_valid & 2) {

drivers/ide/ide.c +1 -0

Index: b/drivers/ide/ide.c
===================================================================
@@ -398,6 +398,7 @@ static void ide_hwif_restore(ide_hwif_t 
 
 	hwif->set_pio_mode		= tmp_hwif->set_pio_mode;
 	hwif->set_dma_mode		= tmp_hwif->set_dma_mode;
+	hwif->mdma_filter		= tmp_hwif->mdma_filter;
 	hwif->udma_filter		= tmp_hwif->udma_filter;
 	hwif->selectproc		= tmp_hwif->selectproc;
 	hwif->reset_poll		= tmp_hwif->reset_poll;

drivers/ide/mips/au1xxx-ide.c +22 ~~-14~~

Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
@@ -351,11 +351,18 @@ static int auide_dma_setup(ide_drive_t *
 	return 0;
 }
 
-static int auide_dma_check(ide_drive_t *drive)
+static u8 auide_mdma_filter(ide_drive_t *drive)
 {
-	u8 speed = ide_max_dma_mode(drive);
+	/*
+	 * FIXME: ->white_list and ->black_list are based on completely bogus
+	 * ->ide_dma_check implementation which didn't set neither the host
+	 * controller timings nor the device for the desired transfer mode.
+	 *
+	 * They should be either removed or 0x00 MWDMA mask should be
+	 * returned for devices on the ->black_list.
+	 */
 
-	if( dbdma_init_done == 0 ){
+	if (dbdma_init_done == 0) {
 		auide_hwif.white_list = ide_in_drive_list(drive->id,
 							  dma_white_list);
 		auide_hwif.black_list = ide_in_drive_list(drive->id,

@@ -366,21 +373,20 @@ static int auide_dma_check(ide_drive_t *
 	}
 
 	/* Is the drive in our DMA black list? */
-
-	if ( auide_hwif.black_list ) {
-		drive->using_dma = 0;
-
-		/* Borrowed the warning message from ide-dma.c */
-
+	if (auide_hwif.black_list)
 		printk(KERN_WARNING "%s: Disabling DMA for %s (blacklisted)\n",
-		       drive->name, drive->id->model);	       
-	}
-	else
-		drive->using_dma = 1;
+				    drive->name, drive->id->model);
 
-	if (drive->autodma && (speed & XFER_MODE) != XFER_PIO)
+	return drive->hwif->mwdma_mask;
+}
+
+static int auide_dma_check(ide_drive_t *drive)
+{
+	if (ide_tune_dma(drive))
 		return 0;
 
+	ide_set_max_pio(drive);
+
 	return -1;
 }

@@ -692,6 +698,8 @@ static int au_ide_probe(struct device *d
 	hwif->dma_off_quietly		= &auide_dma_off_quietly;
 	hwif->dma_timeout		= &auide_dma_timeout;
 
+	hwif->mdma_filter		= &auide_mdma_filter;
+
 	hwif->ide_dma_check             = &auide_dma_check;
 	hwif->dma_exec_cmd              = &auide_dma_exec_cmd;
 	hwif->dma_start                 = &auide_dma_start;

include/linux/ide.h +1 -0

Index: b/include/linux/ide.h
===================================================================
@@ -723,6 +723,7 @@ typedef struct hwif_s {
 	/* driver soft-power interface */
 	int	(*busproc)(ide_drive_t *, int);
 #endif
+	u8 (*mdma_filter)(ide_drive_t *);
 	u8 (*udma_filter)(ide_drive_t *);
 
 	void (*ata_input_data)(ide_drive_t *, void *, u32);

Sergei Shtylyov

2007-08-06 18:12 UTC #1 lore raw

Bartlomiej Zolnierkiewicz wrote:

Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?

Quoted 1 line

* Add ->mdma_filter to ide_hwif_t and use it in ide_get_mode_mask().

Hm, why not mwdma_filter()? That "mdma" word has unneeded connotation. ;-)

Quoted 23 lines

* Remove needless setting of drive->using_dma from auide_dma_check().
* Split off auide_mdma_filter() from auide_dma_check().
* Use ide_tune_dma() in auide_dma_check(), this fixes following issues:
- device's DMA capability bit not being checked
- device not being checked against generic DMA blacklist
- transfer mode not being set on device/host
* Add PIO autotune fallback to auide_dma_check().
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
drivers/ide/mips/au1xxx-ide.c +1 -1
@@ -351,11 +351,18 @@ static int auide_dma_setup(ide_drive_t *
 	return 0;
 }
 
-static int auide_dma_check(ide_drive_t *drive)
+static u8 auide_mdma_filter(ide_drive_t *drive)

drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 363

Quoted 10 lines

drivers/ide/mips/au1xxx-ide.c +8 -1

 {
-	u8 speed = ide_max_dma_mode(drive);
+	/*
+	 * FIXME: ->white_list and ->black_list are based on completely bogus
+	 * ->ide_dma_check implementation which didn't set neither the host
+	 * controller timings nor the device for the desired transfer mode.
+	 *
+	 * They should be either removed or 0x00 MWDMA mask should be
+	 * returned for devices on the ->black_list.
+	 */

I don't get it -- why then introduce a method that does nothing?

drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 365

Quoted 2 lines

drivers/ide/mips/au1xxx-ide.c +1 -1

-	if( dbdma_init_done == 0 ){
+	if (dbdma_init_done == 0) {

I wonder what this code is doing here at all...

drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 368

Quoted 3 lines

drivers/ide/mips/au1xxx-ide.c +0 -0

 		auide_hwif.white_list = ide_in_drive_list(drive->id,
 							  dma_white_list);
 		auide_hwif.black_list = ide_in_drive_list(drive->id,

Why the results of the drive list lockup gets tied to auide_hwif? :-O

Quoted 7 lines

drivers/ide/mips/au1xxx-ide.c +0 -3

@@ -366,21 +373,20 @@ static int auide_dma_check(ide_drive_t *
 	}
 
 	/* Is the drive in our DMA black list? */
-
-	if ( auide_hwif.black_list ) {
-		drive->using_dma = 0;

drivers/ide/mips/au1xxx-ide.c static int auide_dma_check(ide_drive_t * · line 378

Quoted 10 lines

drivers/ide/mips/au1xxx-ide.c +2 -7

-
-		/* Borrowed the warning message from ide-dma.c */
-
+	if (auide_hwif.black_list)
 		printk(KERN_WARNING "%s: Disabling DMA for %s (blacklisted)\n",
-		       drive->name, drive->id->model);	       
-	}
-	else
-		drive->using_dma = 1;
+				    drive->name, drive->id->model);

*Disabling* it, really? :-O

drivers/ide/mips/au1xxx-ide.c static int auide_dma_check(ide_drive_t * · line 391

Quoted 13 lines

drivers/ide/mips/au1xxx-ide.c +8 -1

-	if (drive->autodma && (speed & XFER_MODE) != XFER_PIO)
+	return drive->hwif->mwdma_mask;
+}
+
+static int auide_dma_check(ide_drive_t *drive)
+{
+	if (ide_tune_dma(drive))
 		return 0;
 
+	ide_set_max_pio(drive);
+
 	return -1;
 }

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-08-08 22:58 UTC #2 lore raw

Quoted 4 lines · Sergei Shtylyov

Bartlomiej Zolnierkiewicz wrote:
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?

Not really but if needed I will extract MWDMA filter part and push it sooner.

Quoted 3 lines

Quoted 1 line
* Add ->mdma_filter to ide_hwif_t and use it in ide_get_mode_mask().
Hm, why not mwdma_filter()? That "mdma" word has unneeded connotation. ;-)

Ha! As predicted:

->mdma_filter name would make people more ecstatic about the code

;)

Quoted 21 lines

Quoted 21 lines
* Remove needless setting of drive->using_dma from auide_dma_check().
* Split off auide_mdma_filter() from auide_dma_check().
* Use ide_tune_dma() in auide_dma_check(), this fixes following issues:
- device's DMA capability bit not being checked
- device not being checked against generic DMA blacklist
- transfer mode not being set on device/host
* Add PIO autotune fallback to auide_dma_check().
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
drivers/ide/mips/au1xxx-ide.c +0 -0
@@ -351,11 +351,18 @@ static int auide_dma_setup(ide_drive_t *
 	return 0;
 }
 

Quoted 14 lines

drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 363

Quoted 12 lines

drivers/ide/mips/au1xxx-ide.c +9 -2

-static int auide_dma_check(ide_drive_t *drive)
+static u8 auide_mdma_filter(ide_drive_t *drive)
 {
-	u8 speed = ide_max_dma_mode(drive);
+	/*
+	 * FIXME: ->white_list and ->black_list are based on completely bogus
+	 * ->ide_dma_check implementation which didn't set neither the host
+	 * controller timings nor the device for the desired transfer mode.
+	 *
+	 * They should be either removed or 0x00 MWDMA mask should be
+	 * returned for devices on the ->black_list.
+	 */

I don't get it -- why then introduce a method that does nothing?

It does something as you've noticed yourself:

Quoted 10 lines

drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 365
Quoted 2 lines
drivers/ide/mips/au1xxx-ide.c +1 -1
-	if( dbdma_init_done == 0 ){
+	if (dbdma_init_done == 0) {
I wonder what this code is doing here at all...
drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 368
Quoted 3 lines
drivers/ide/mips/au1xxx-ide.c +0 -0
 		auide_hwif.white_list = ide_in_drive_list(drive->id,
 							  dma_white_list);
 		auide_hwif.black_list = ide_in_drive_list(drive->id,
Why the results of the drive list lockup gets tied to auide_hwif? :-O

To use results in auide_ddma_init()... (which needs fixing of course).

The more interesting questions are: WTF is "safe MWDMA" mode (tsize == 1, devwidth == 16) and whether ->white/black_list is really needed.

I planned to cc: AU1XXX platform maintainers on this patch but to my surprise MAINTAINERS lacks AU1XXX entry.

Bart

Sergei Shtylyov

2007-08-10 19:09 UTC #3 lore raw

Quoted 4 lines · Bartlomiej Zolnierkiewicz

Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?
Not really but if needed I will extract MWDMA filter part and push it sooner.

Erm, may I just merge it to my patch (mentioning you of course)?

Quoted 9 lines

Quoted 3 lines
Quoted 1 line
* Add ->mdma_filter to ide_hwif_t and use it in ide_get_mode_mask().
Hm, why not mwdma_filter()? That "mdma" word has unneeded connotation. ;-)
Ha! As predicted:
->mdma_filter name would make people more ecstatic about the code
;)

Nice one. 8-)

Quoted 36 lines

Quoted 21 lines
Quoted 21 lines
* Remove needless setting of drive->using_dma from auide_dma_check().
* Split off auide_mdma_filter() from auide_dma_check().
* Use ide_tune_dma() in auide_dma_check(), this fixes following issues:
- device's DMA capability bit not being checked
- device not being checked against generic DMA blacklist
- transfer mode not being set on device/host
* Add PIO autotune fallback to auide_dma_check().
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
drivers/ide/mips/au1xxx-ide.c +0 -0
@@ -351,11 +351,18 @@ static int auide_dma_setup(ide_drive_t *
 	return 0;
 }
 
Quoted 14 lines
drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 363
Quoted 12 lines
drivers/ide/mips/au1xxx-ide.c +9 -2
-static int auide_dma_check(ide_drive_t *drive)
+static u8 auide_mdma_filter(ide_drive_t *drive)
 {
-	u8 speed = ide_max_dma_mode(drive);
+	/*
+	 * FIXME: ->white_list and ->black_list are based on completely bogus
+	 * ->ide_dma_check implementation which didn't set neither the host
+	 * controller timings nor the device for the desired transfer mode.
+	 *
+	 * They should be either removed or 0x00 MWDMA mask should be
+	 * returned for devices on the ->black_list.
+	 */
I don't get it -- why then introduce a method that does nothing?

Quoted 1 line

It does something as you've noticed yourself:

Yeah, I saw that it does something that shouldn't be done there. :-)
The more is the reason to move that method into my recent hpt366 filter patch.

Quoted 10 lines

Quoted 10 lines
drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 365
Quoted 2 lines
drivers/ide/mips/au1xxx-ide.c +1 -1
-	if( dbdma_init_done == 0 ){
+	if (dbdma_init_done == 0) {
I wonder what this code is doing here at all...
drivers/ide/mips/au1xxx-ide.c static int auide_dma_setup(ide_drive_t * · line 368
Quoted 3 lines
drivers/ide/mips/au1xxx-ide.c +0 -0
 		auide_hwif.white_list = ide_in_drive_list(drive->id,
 							  dma_white_list);
 		auide_hwif.black_list = ide_in_drive_list(drive->id,
Why the results of the drive list lockup gets tied to auide_hwif? :-O

Yet I was sure I'd typed "lookup". Probably a freudian slip. :-)

Quoted 4 lines

To use results in auide_ddma_init()... (which needs fixing of course).
The more interesting questions are: WTF is "safe MWDMA" mode (tsize == 1, devwidth == 16) and whether ->white/black_list is really needed.

No time to look into the manuals right now...

Quoted 2 lines

I planned to cc: AU1XXX platform maintainers on this patch but to my surprise MAINTAINERS lacks AU1XXX entry.

It's been solt out to Raza Microelectronics last year and those guys never
sent a single patch to linux-mips...

Quoted 1 line

Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-08-10 21:58 UTC #4 lore raw

Quoted 8 lines · Sergei Shtylyov

Quoted 4 lines · Bartlomiej Zolnierkiewicz
Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?
Not really but if needed I will extract MWDMA filter part and push it sooner.
Erm, may I just merge it to my patch (mentioning you of course)?

Sure.

Quoted 5 lines

Quoted 2 lines
I planned to cc: AU1XXX platform maintainers on this patch but to my surprise MAINTAINERS lacks AU1XXX entry.
It's been solt out to Raza Microelectronics last year and those guys never
sent a single patch to linux-mips...

Thanks for the info.

Bart

Sergei Shtylyov

2007-08-11 16:58 UTC #5 lore raw

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?
Not really but if needed I will extract MWDMA filter part and push it sooner.
Erm, may I just merge it to my patch (mentioning you of course)?
Sure.

I guess you want to keep the method's name? :-)

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
I planned to cc: AU1XXX platform maintainers on this patch but to my surprise MAINTAINERS lacks AU1XXX entry.
It's been solt out to Raza Microelectronics last year and those guys never
sent a single patch to linux-mips...

Unfortunately, I don't have direct access to DBAu1200 board (probably
could get a remote though) and ealier Alchemy SoCs (which we have here) don't have the IDE support.

Quoted 3 lines

Thanks for the info.
Bart

MBR, Sergei

Sergei Shtylyov

2007-08-11 16:58 UTC #6 lore raw

In reply to Bartlomiej Zolnierkiewicz

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?
Not really but if needed I will extract MWDMA filter part and push it sooner.
Erm, may I just merge it to my patch (mentioning you of course)?
Sure.

I guess you want to keep the method's name? :-)

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I planned to cc: AU1XXX platform maintainers on this patch but to my surprise MAINTAINERS lacks AU1XXX entry.
It's been solt out to Raza Microelectronics last year and those guys never
sent a single patch to linux-mips...
Thanks for the info.

Unfortunately, I don't have direct access to DBAu1200 board (probably
could get a remote though) and ealier Alchemy SoCs (which we have here) don't have the IDE support.

Quoted 1 line

Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-08-11 17:11 UTC #7 lore raw

Quoted 5 lines · you

Quoted 2 lines · Bartlomiej Zolnierkiewicz
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Not really but if needed I will extract MWDMA filter part and push it sooner.
Erm, may I just merge it to my patch (mentioning you of course)?
Sure.
I guess you want to keep the method's name? :-)

Not really, it may be ->mwdma_filter (to keep consistency with ->mwmda_mask).

Bart

Sergei Shtylyov

2007-08-11 18:50 UTC #8 lore raw

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Good, that's what I lacked for hpt366.c! Were you planning to push it to
Linus soon?
Not really but if needed I will extract MWDMA filter part and push it sooner.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Erm, may I just merge it to my patch (mentioning you of course)?
Sure.
I guess you want to keep the method's name? :-)
Not really, it may be ->mwdma_filter (to keep consistency with ->mwmda_mask).

Well, we have asimmetric udma_filter and ultra_mask, so the ecstatic name
seems to be in the same vein. :-)

Quoted 1 line

Bart

MBR, Sergei