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

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

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.

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

>>>* 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-)

>>>* 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 <bzolnier@gmail.com>

>>>Index: b/drivers/ide/mips/au1xxx-ide.c
>>>===================================================================
>>>--- a/drivers/ide/mips/au1xxx-ide.c
>>>+++ 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.
>>>+	 */

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

> 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.

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

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

>>> 		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. :-)

> 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...

> 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...

> Bart

MBR, Sergei