Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Mikael Pettersson]

On Thu, 8 Feb 2007 00:00:32 +0300, Sergei Shtylyov wrote:
>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
>its author really thought that he could achieve that wrtiting to BMIDE status
>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
>coding style and whitespace changes while at it...
>
>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
>along with some more fixing, so I'm putting it off...
>
>Warning: this has been compile-tested only.

Worked fine on my SPARC Ultra5.

/Mikael

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Bartlomiej Zolnierkiewicz]

On Thursday 08 February 2007 09:58, Mikael Pettersson wrote:
> On Thu, 8 Feb 2007 00:00:32 +0300, Sergei Shtylyov wrote:
> >Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
> >its author really thought that he could achieve that wrtiting to BMIDE status
> >registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
> >do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
> >also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
> >coding style and whitespace changes while at it...
> >
> >Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
> >along with some more fixing, so I'm putting it off...
> >
> >Warning: this has been compile-tested only.
> 
> Worked fine on my SPARC Ultra5.

thanks, applied

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Mikael Pettersson]

On Thu, 8 Feb 2007 09:58:45 +0100 (MET), Mikael Pettersson wrote:
>On Thu, 8 Feb 2007 00:00:32 +0300, Sergei Shtylyov wrote:
>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
>>its author really thought that he could achieve that wrtiting to BMIDE status
>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
>>coding style and whitespace changes while at it...
>>
>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
>>along with some more fixing, so I'm putting it off...
>>
>>Warning: this has been compile-tested only.
>
>Worked fine on my SPARC Ultra5.

Correction: I was only looking for absence of errors when testing
this patch. However, later I found that this patch (version 1.42
of cmd64x.c) disabled DMA on my CMD646, dropping performance to
1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).

Here's the relevant kernel messages from before this patch:

ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
CMD646: IDE controller at PCI slot 0000:01:03.0
CMD646: chipset revision 3
CMD646: chipset revision 0x03, MultiWord DMA Force Limited
CMD646: 100% native mode on irq 14
    ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
    ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
Probing IDE interface ide0...
hda: ST320420A, ATA DISK drive
ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
Probing IDE interface ide1...
hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
hda: max request size: 128KiB
hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
hda: cache flushes not supported
 hda: hda1 hda2 hda3 hda4 hda5

With the patch the kernel messages are the same, except for the
3rd last line which becomes:

hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63

i.e., the (U)DMA indicator is gone.

Please revert this until the regression is fixed.

/Mikael

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Mikael Pettersson wrote:
> On Thu, 8 Feb 2007 09:58:45 +0100 (MET), Mikael Pettersson wrote:
> 
>>On Thu, 8 Feb 2007 00:00:32 +0300, Sergei Shtylyov wrote:
>>
>>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
>>>its author really thought that he could achieve that wrtiting to BMIDE status
>>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
>>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
>>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
>>>coding style and whitespace changes while at it...

>>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
>>>along with some more fixing, so I'm putting it off...

>>>Warning: this has been compile-tested only.

>>Worked fine on my SPARC Ultra5.

> Correction: I was only looking for absence of errors when testing
> this patch. However, later I found that this patch (version 1.42
> of cmd64x.c) disabled DMA on my CMD646, dropping performance to
> 1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).

    That was expected behavior.

> Here's the relevant kernel messages from before this patch:

> ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
> CMD646: IDE controller at PCI slot 0000:01:03.0
> CMD646: chipset revision 3
> CMD646: chipset revision 0x03, MultiWord DMA Force Limited

    The driver never supported UltraDMA on this revision, as indicated by that 
message.

> CMD646: 100% native mode on irq 14
>     ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
>     ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
> Probing IDE interface ide0...
> hda: ST320420A, ATA DISK drive
> ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
> Probing IDE interface ide1...
> hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
> ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
> hda: max request size: 128KiB
> hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
> hda: cache flushes not supported
>  hda: hda1 hda2 hda3 hda4 hda5

> With the patch the kernel messages are the same, except for the
> 3rd last line which becomes:

> hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63

> i.e., the (U)DMA indicator is gone.

> Please revert this until the regression is fixed.

    The intent of the patch was exactly to *remove* broken DMA support until 
it's fixed (which requires more work).  It only worked by chance -- because 
MWDMA2 timings are the same as of PIO4.  Have patience please.

> /Mikael

WBR, Sergei

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

>    The intent of the patch was exactly to *remove* broken DMA support 
> until it's fixed (which requires more work).  It only worked by chance 
> -- because MWDMA2 timings are the same as of PIO4.  Have patience please.

    You may also consider switching to the libata driver -- Alan said he fixed 
DMA support there after I frisrt reported the breakage (however, I' still not 
seeing it fixed in the mainline)...

>> /Mikael

WBR, Sergei

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Bartlomiej Zolnierkiewicz]

On Friday 16 February 2007 14:39, Sergei Shtylyov wrote:
> Hello.
> 
> Sergei Shtylyov wrote:
> 
> >    The intent of the patch was exactly to *remove* broken DMA support 
> > until it's fixed (which requires more work).  It only worked by chance 
> > -- because MWDMA2 timings are the same as of PIO4.  Have patience please.
> 
>     You may also consider switching to the libata driver -- Alan said he fixed 
> DMA support there after I frisrt reported the breakage (however, I' still not 
> seeing it fixed in the mainline)...

I've just checked 2.6.20-mm1 (contains libata tree) and it is not fixed.

Bart

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>   The intent of the patch was exactly to *remove* broken DMA support 
>>>until it's fixed (which requires more work).  It only worked by chance 
>>>-- because MWDMA2 timings are the same as of PIO4.  Have patience please.

>>    You may also consider switching to the libata driver -- Alan said he fixed 
>>DMA support there after I frisrt reported the breakage (however, I' still not 
>>seeing it fixed in the mainline)...

> I've just checked 2.6.20-mm1 (contains libata tree) and it is not fixed.

    I was saying exatly that. So, that's a question to Alan. :-)

> Bart

WBR, Sergei

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 16 February 2007 14:11, Sergei Shtylyov wrote:
> Mikael Pettersson wrote:
> > On Thu, 8 Feb 2007 09:58:45 +0100 (MET), Mikael Pettersson wrote:
> > 
> >>On Thu, 8 Feb 2007 00:00:32 +0300, Sergei Shtylyov wrote:
> >>
> >>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
> >>>its author really thought that he could achieve that wrtiting to BMIDE status
> >>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
> >>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
> >>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
> >>>coding style and whitespace changes while at it...
> 
> >>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
> >>>along with some more fixing, so I'm putting it off...
> 
> >>>Warning: this has been compile-tested only.
> 
> >>Worked fine on my SPARC Ultra5.
> 
> > Correction: I was only looking for absence of errors when testing
> > this patch. However, later I found that this patch (version 1.42
> > of cmd64x.c) disabled DMA on my CMD646, dropping performance to
> > 1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).
> 
>     That was expected behavior.
> 
> > Here's the relevant kernel messages from before this patch:
> 
> > ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
> > CMD646: IDE controller at PCI slot 0000:01:03.0
> > CMD646: chipset revision 3
> > CMD646: chipset revision 0x03, MultiWord DMA Force Limited
> 
>     The driver never supported UltraDMA on this revision, as indicated by that 
> message.
> 
> > CMD646: 100% native mode on irq 14
> >     ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
> >     ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
> > Probing IDE interface ide0...
> > hda: ST320420A, ATA DISK drive
> > ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
> > Probing IDE interface ide1...
> > hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
> > ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
> > hda: max request size: 128KiB
> > hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
> > hda: cache flushes not supported
> >  hda: hda1 hda2 hda3 hda4 hda5
> 
> > With the patch the kernel messages are the same, except for the
> > 3rd last line which becomes:
> 
> > hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63
> 
> > i.e., the (U)DMA indicator is gone.
> 
> > Please revert this until the regression is fixed.
> 
>     The intent of the patch was exactly to *remove* broken DMA support until 
> it's fixed (which requires more work).  It only worked by chance -- because 
> MWDMA2 timings are the same as of PIO4.  Have patience please.

It only worked by chance but it _worked_, especially for the usual case,
MWDMA2/PIO4 == all newer drives (despite writing 0x10 reserved bit of
BMIDESR0/1 for the master devices).  I think I'll remove the patch for now.

To fix SWDMA/MWDMA properly isn't it enough to just call cmd64x_tune_pio()
from cmd64x_tune_chipset() to tune the corresponding PIO mode?

Bart

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>Mikael Pettersson wrote:

>>>>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
>>>>>its author really thought that he could achieve that wrtiting to BMIDE status
>>>>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
>>>>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
>>>>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
>>>>>coding style and whitespace changes while at it...

>>>>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
>>>>>along with some more fixing, so I'm putting it off...

>>>>>Warning: this has been compile-tested only.

>>>>Worked fine on my SPARC Ultra5.

>>>Correction: I was only looking for absence of errors when testing
>>>this patch. However, later I found that this patch (version 1.42
>>>of cmd64x.c) disabled DMA on my CMD646, dropping performance to
>>>1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).

>>    That was expected behavior.

>>>Here's the relevant kernel messages from before this patch:

>>>ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
>>>CMD646: IDE controller at PCI slot 0000:01:03.0
>>>CMD646: chipset revision 3
>>>CMD646: chipset revision 0x03, MultiWord DMA Force Limited

>>    The driver never supported UltraDMA on this revision, as indicated by that 
>>message.

>>>CMD646: 100% native mode on irq 14
>>>    ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
>>>    ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
>>>Probing IDE interface ide0...
>>>hda: ST320420A, ATA DISK drive
>>>ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
>>>Probing IDE interface ide1...
>>>hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
>>>ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
>>>hda: max request size: 128KiB
>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
>>>hda: cache flushes not supported
>>> hda: hda1 hda2 hda3 hda4 hda5

>>>With the patch the kernel messages are the same, except for the
>>>3rd last line which becomes:

>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63

>>>i.e., the (U)DMA indicator is gone.

>>>Please revert this until the regression is fixed.

>>    The intent of the patch was exactly to *remove* broken DMA support until 
>>it's fixed (which requires more work).  It only worked by chance -- because 
>>MWDMA2 timings are the same as of PIO4.  Have patience please.

> It only worked by chance but it _worked_, especially for the usual case,
> MWDMA2/PIO4 == all newer drives (despite writing 0x10 reserved bit of
> BMIDESR0/1 for the master devices).  I think I'll remove the patch for now.

    Then I will protest. :-) This was *not* fixable all at once. And removing 
it just because some users happen to have the *known buggy* (and so reduced to 
MWDMA only) chips is a wrong thing to do.

> To fix SWDMA/MWDMA properly isn't it enough to just call cmd64x_tune_pio()
> from cmd64x_tune_chipset() to tune the corresponding PIO mode?

    No, that wouldn't be a clean fix, just a kind of workaround -- it will 
also change the address setup times (which is not quite desirable).  I can 
compose such quickie in five minutes though.

> Bart

WBR, Sergei

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Bartlomiej Zolnierkiewicz]

On Friday 16 February 2007 16:00, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>Mikael Pettersson wrote:
> 
> >>>>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
> >>>>>its author really thought that he could achieve that wrtiting to BMIDE status
> >>>>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
> >>>>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
> >>>>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
> >>>>>coding style and whitespace changes while at it...
> 
> >>>>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
> >>>>>along with some more fixing, so I'm putting it off...
> 
> >>>>>Warning: this has been compile-tested only.
> 
> >>>>Worked fine on my SPARC Ultra5.
> 
> >>>Correction: I was only looking for absence of errors when testing
> >>>this patch. However, later I found that this patch (version 1.42
> >>>of cmd64x.c) disabled DMA on my CMD646, dropping performance to
> >>>1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).
> 
> >>    That was expected behavior.
> 
> >>>Here's the relevant kernel messages from before this patch:
> 
> >>>ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
> >>>CMD646: IDE controller at PCI slot 0000:01:03.0
> >>>CMD646: chipset revision 3
> >>>CMD646: chipset revision 0x03, MultiWord DMA Force Limited
> 
> >>    The driver never supported UltraDMA on this revision, as indicated by that 
> >>message.
> 
> >>>CMD646: 100% native mode on irq 14
> >>>    ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
> >>>    ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
> >>>Probing IDE interface ide0...
> >>>hda: ST320420A, ATA DISK drive
> >>>ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
> >>>Probing IDE interface ide1...
> >>>hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
> >>>ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
> >>>hda: max request size: 128KiB
> >>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
> >>>hda: cache flushes not supported
> >>> hda: hda1 hda2 hda3 hda4 hda5
> 
> >>>With the patch the kernel messages are the same, except for the
> >>>3rd last line which becomes:
> 
> >>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63
> 
> >>>i.e., the (U)DMA indicator is gone.
> 
> >>>Please revert this until the regression is fixed.
> 
> >>    The intent of the patch was exactly to *remove* broken DMA support until 
> >>it's fixed (which requires more work).  It only worked by chance -- because 
> >>MWDMA2 timings are the same as of PIO4.  Have patience please.
> 
> > It only worked by chance but it _worked_, especially for the usual case,
> > MWDMA2/PIO4 == all newer drives (despite writing 0x10 reserved bit of
> > BMIDESR0/1 for the master devices).  I think I'll remove the patch for now.
> 
>     Then I will protest. :-) This was *not* fixable all at once. And removing 
> it just because some users happen to have the *known buggy* (and so reduced to 
> MWDMA only) chips is a wrong thing to do.

OK, I'm not removing it but it needs (rather urgent) fixing.

> > To fix SWDMA/MWDMA properly isn't it enough to just call cmd64x_tune_pio()
> > from cmd64x_tune_chipset() to tune the corresponding PIO mode?
> 
>     No, that wouldn't be a clean fix, just a kind of workaround -- it will 
> also change the address setup times (which is not quite desirable).  I can 

I see, how about:

* splitting off setup timing programming from program_drive_counts()
  into separate function (setup timing is programmed into separate register)

* pushing ide_get_best_pio_mode() call to higher layers
  [ cmd64x_tune_driver() only, it is not needed in cmd64x_tune_chipset() ]

* calling new function setting setup timing from cmd64x_tune_drive()
  and cmd64x_tune_chipset() (here for PIO modes only)

* calling cmd64x_tune_pio() fro SWDMA/MWDMA

Would the above assembly into the proper SWDMA/MWDMA fix?

> compose such quickie in five minutes though.

I can wait some time for the proper fix but if you see that
it won't happen soon (week?) please send me this workaround.

Thanks,
Bart

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>>>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
>>>>>>>its author really thought that he could achieve that wrtiting to BMIDE status
>>>>>>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
>>>>>>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
>>>>>>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
>>>>>>>coding style and whitespace changes while at it...
>>
>>>>>>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
>>>>>>>along with some more fixing, so I'm putting it off...
>>
>>>>>>>Warning: this has been compile-tested only.
>>
>>>>>>Worked fine on my SPARC Ultra5.
>>
>>>>>Correction: I was only looking for absence of errors when testing
>>>>>this patch. However, later I found that this patch (version 1.42
>>>>>of cmd64x.c) disabled DMA on my CMD646, dropping performance to
>>>>>1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).
>>
>>>>   That was expected behavior.
>>
>>>>>Here's the relevant kernel messages from before this patch:
>>
>>>>>ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
>>>>>CMD646: IDE controller at PCI slot 0000:01:03.0
>>>>>CMD646: chipset revision 3
>>>>>CMD646: chipset revision 0x03, MultiWord DMA Force Limited
>>
>>>>   The driver never supported UltraDMA on this revision, as indicated by that 
>>>>message.
>>
>>>>>CMD646: 100% native mode on irq 14
>>>>>   ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
>>>>>   ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
>>>>>Probing IDE interface ide0...
>>>>>hda: ST320420A, ATA DISK drive
>>>>>ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
>>>>>Probing IDE interface ide1...
>>>>>hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
>>>>>ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
>>>>>hda: max request size: 128KiB
>>>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
>>>>>hda: cache flushes not supported
>>>>>hda: hda1 hda2 hda3 hda4 hda5
>>
>>>>>With the patch the kernel messages are the same, except for the
>>>>>3rd last line which becomes:
>>
>>>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63
>>
>>>>>i.e., the (U)DMA indicator is gone.
>>
>>>>>Please revert this until the regression is fixed.
>>
>>>>   The intent of the patch was exactly to *remove* broken DMA support until 
>>>>it's fixed (which requires more work).  It only worked by chance -- because 
>>>>MWDMA2 timings are the same as of PIO4.  Have patience please.
>>
>>>It only worked by chance but it _worked_, especially for the usual case,
>>>MWDMA2/PIO4 == all newer drives (despite writing 0x10 reserved bit of
>>>BMIDESR0/1 for the master devices).  I think I'll remove the patch for now.

>>    Then I will protest. :-) This was *not* fixable all at once. And removing 
>>it just because some users happen to have the *known buggy* (and so reduced to 
>>MWDMA only) chips is a wrong thing to do.

    PCI0643 would also suffer, to be precise.

> OK, I'm not removing it but it needs (rather urgent) fixing.

>>>To fix SWDMA/MWDMA properly isn't it enough to just call cmd64x_tune_pio()
>>>from cmd64x_tune_chipset() to tune the corresponding PIO mode?

>>    No, that wouldn't be a clean fix, just a kind of workaround -- it will 
>>also change the address setup times (which is not quite desirable).  I can 

> I see, how about:

> * splitting off setup timing programming from program_drive_counts()
>   into separate function (setup timing is programmed into separate register)

    Yes, it's on my todo list. But consider the amount of cleanup work I had 
to do (and have in drafts still) for 2.6.21-rc1 -- if it really gets there 
:-).  And also, consider that currently I have a plenty of internal bugs to 
fix besides IDE (and they're not as obvious as IDE ones).

> * pushing ide_get_best_pio_mode() call to higher layers
>   [ cmd64x_tune_driver() only, it is not needed in cmd64x_tune_chipset() ]

    Ehm, is it really there?  I thought I left it in cmd64x_tune_pio() where 
it's on its right place...

> * calling new function setting setup timing from cmd64x_tune_drive()
>   and cmd64x_tune_chipset() (here for PIO modes only)

   I ceratainly don't want to put this into cmd64x_tune_chipset() -- this 
should be handled in cmd64x_tune_pio() still.

> * calling cmd64x_tune_pio() fro SWDMA/MWDMA

    Erm? Putting time-to-cycle converion into program_drive_counts() sound 
like a better plan.

> Would the above assembly into the proper SWDMA/MWDMA fix?

    Not all items did make sense to me, sorry. :-)

>>compose such quickie in five minutes though.

> I can wait some time for the proper fix but if you see that
> it won't happen soon (week?) please send me this workaround.

    I think it will happen in a week.

> Thanks,
> Bart

WBR, Sergei

Re: [PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Bartlomiej Zolnierkiewicz]

On Friday 16 February 2007 16:34, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>>>>Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
> >>>>>>>its author really thought that he could achieve that wrtiting to BMIDE status
> >>>>>>>registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
> >>>>>>>do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
> >>>>>>>also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
> >>>>>>>coding style and whitespace changes while at it...
> >>
> >>>>>>>Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
> >>>>>>>along with some more fixing, so I'm putting it off...
> >>
> >>>>>>>Warning: this has been compile-tested only.
> >>
> >>>>>>Worked fine on my SPARC Ultra5.
> >>
> >>>>>Correction: I was only looking for absence of errors when testing
> >>>>>this patch. However, later I found that this patch (version 1.42
> >>>>>of cmd64x.c) disabled DMA on my CMD646, dropping performance to
> >>>>>1/4th (from about 13MB/s to about 3.5MB/s according to hdparm -Tt).
> >>
> >>>>   That was expected behavior.
> >>
> >>>>>Here's the relevant kernel messages from before this patch:
> >>
> >>>>>ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
> >>>>>CMD646: IDE controller at PCI slot 0000:01:03.0
> >>>>>CMD646: chipset revision 3
> >>>>>CMD646: chipset revision 0x03, MultiWord DMA Force Limited
> >>
> >>>>   The driver never supported UltraDMA on this revision, as indicated by that 
> >>>>message.
> >>
> >>>>>CMD646: 100% native mode on irq 14
> >>>>>   ide0: BM-DMA at 0x1fe02c00020-0x1fe02c00027, BIOS settings: hda:pio, hdb:pio
> >>>>>   ide1: BM-DMA at 0x1fe02c00028-0x1fe02c0002f, BIOS settings: hdc:pio, hdd:pio
> >>>>>Probing IDE interface ide0...
> >>>>>hda: ST320420A, ATA DISK drive
> >>>>>ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14
> >>>>>Probing IDE interface ide1...
> >>>>>hdc: CRD-8483B, ATAPI CD/DVD-ROM drive
> >>>>>ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (shared with ide0)
> >>>>>hda: max request size: 128KiB
> >>>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63, (U)DMA
> >>>>>hda: cache flushes not supported
> >>>>>hda: hda1 hda2 hda3 hda4 hda5
> >>
> >>>>>With the patch the kernel messages are the same, except for the
> >>>>>3rd last line which becomes:
> >>
> >>>>>hda: 39851760 sectors (20404 MB) w/2048KiB Cache, CHS=39535/16/63
> >>
> >>>>>i.e., the (U)DMA indicator is gone.
> >>
> >>>>>Please revert this until the regression is fixed.
> >>
> >>>>   The intent of the patch was exactly to *remove* broken DMA support until 
> >>>>it's fixed (which requires more work).  It only worked by chance -- because 
> >>>>MWDMA2 timings are the same as of PIO4.  Have patience please.
> >>
> >>>It only worked by chance but it _worked_, especially for the usual case,
> >>>MWDMA2/PIO4 == all newer drives (despite writing 0x10 reserved bit of
> >>>BMIDESR0/1 for the master devices).  I think I'll remove the patch for now.
> 
> >>    Then I will protest. :-) This was *not* fixable all at once. And removing 
> >>it just because some users happen to have the *known buggy* (and so reduced to 
> >>MWDMA only) chips is a wrong thing to do.
> 
>     PCI0643 would also suffer, to be precise.
> 
> > OK, I'm not removing it but it needs (rather urgent) fixing.
> 
> >>>To fix SWDMA/MWDMA properly isn't it enough to just call cmd64x_tune_pio()
> >>>from cmd64x_tune_chipset() to tune the corresponding PIO mode?
> 
> >>    No, that wouldn't be a clean fix, just a kind of workaround -- it will 
> >>also change the address setup times (which is not quite desirable).  I can 
> 
> > I see, how about:
> 
> > * splitting off setup timing programming from program_drive_counts()
> >   into separate function (setup timing is programmed into separate register)
> 
>     Yes, it's on my todo list. But consider the amount of cleanup work I had 
> to do (and have in drafts still) for 2.6.21-rc1 -- if it really gets there 
> :-).  And also, consider that currently I have a plenty of internal bugs to 
> fix besides IDE (and they're not as obvious as IDE ones).
> 
> > * pushing ide_get_best_pio_mode() call to higher layers
> >   [ cmd64x_tune_driver() only, it is not needed in cmd64x_tune_chipset() ]
> 
>     Ehm, is it really there?  I thought I left it in cmd64x_tune_pio() where 
> it's on its right place...

Yes, it is still there - I meant pushing it up to avoid programming setup
time when programming timings for DMA modes.

> > * calling new function setting setup timing from cmd64x_tune_drive()
> >   and cmd64x_tune_chipset() (here for PIO modes only)
> 
>    I ceratainly don't want to put this into cmd64x_tune_chipset() -- this 
> should be handled in cmd64x_tune_pio() still.
> 
> > * calling cmd64x_tune_pio() fro SWDMA/MWDMA
> 
>     Erm? Putting time-to-cycle converion into program_drive_counts() sound 
> like a better plan.

agreed

> > Would the above assembly into the proper SWDMA/MWDMA fix?
> 
>     Not all items did make sense to me, sorry. :-)
> 
> >>compose such quickie in five minutes though.
> 
> > I can wait some time for the proper fix but if you see that
> > it won't happen soon (week?) please send me this workaround.
> 
>     I think it will happen in a week.

OK.

Thanks,
Bart

[PATCH] (pata-2.6 fix queue) cmd64x: remove broken SW/MW DMA support

[Sergei Shtylyov]

Remove the bogus code pretending to set SW/MW DMA timings -- I wonder whether
its author really thought that he could achieve that wrtiting to BMIDE status
registers?  Stop fiddling with the DMA capable bits in the speedproc() -- they
do not enable DMA, and are properly dealt with by the dma_host_{on,off} methods;
also, get rid of the duplicate reads/writes of UDIDETCRx registers, and do some
coding style and whitespace changes while at it...

Unfortunately, fixing the SW/MW DMA support would requre a major driver rewrite
along with some more fixing, so I'm putting it off...

Warning: this has been compile-tested only.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

 drivers/ide/pci/cmd64x.c |   80 +++++++++++++++++++++--------------------------
 1 files changed, 36 insertions(+), 44 deletions(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ linux-2.6/drivers/ide/pci/cmd64x.c
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.42	Feb 6, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because
@@ -381,61 +381,55 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static int cmd64x_tune_chipset (ide_drive_t *drive, u8 xferspeed)
+static int cmd64x_tune_chipset (ide_drive_t *drive, u8 speed)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
+	u8 unit			= drive->dn & 0x01;
+	u8 regU = 0, pciU	= hwif->channel ? UDIDETCR1 : UDIDETCR0;
 
-	u8 unit			= (drive->select.b.unit & 0x01);
-	u8 regU = 0, pciU	= (hwif->channel) ? UDIDETCR1 : UDIDETCR0;
-	u8 regD = 0, pciD	= (hwif->channel) ? BMIDESR1 : BMIDESR0;
-
-	u8 speed	= ide_rate_filter(cmd64x_ratemask(drive), xferspeed);
+	speed = ide_rate_filter(cmd64x_ratemask(drive), speed);
 
 	if (speed >= XFER_SW_DMA_0) {
-		(void) pci_read_config_byte(dev, pciD, &regD);
-		(void) pci_read_config_byte(dev, pciU, &regU);
-		regD &= ~(unit ? 0x40 : 0x20);
+		(void) pci_read_config_byte (dev, pciU, &regU);
 		regU &= ~(unit ? 0xCA : 0x35);
-		(void) pci_write_config_byte(dev, pciD, regD);
-		(void) pci_write_config_byte(dev, pciU, regU);
-		(void) pci_read_config_byte(dev, pciD, &regD);
-		(void) pci_read_config_byte(dev, pciU, &regU);
 	}
 
 	switch(speed) {
-		case XFER_UDMA_5:	regU |= (unit ? 0x0A : 0x05); break;
-		case XFER_UDMA_4:	regU |= (unit ? 0x4A : 0x15); break;
-		case XFER_UDMA_3:	regU |= (unit ? 0x8A : 0x25); break;
-		case XFER_UDMA_2:	regU |= (unit ? 0x42 : 0x11); break;
-		case XFER_UDMA_1:	regU |= (unit ? 0x82 : 0x21); break;
-		case XFER_UDMA_0:	regU |= (unit ? 0xC2 : 0x31); break;
-		case XFER_MW_DMA_2:	regD |= (unit ? 0x40 : 0x10); break;
-		case XFER_MW_DMA_1:	regD |= (unit ? 0x80 : 0x20); break;
-		case XFER_MW_DMA_0:	regD |= (unit ? 0xC0 : 0x30); break;
-		case XFER_SW_DMA_2:	regD |= (unit ? 0x40 : 0x10); break;
-		case XFER_SW_DMA_1:	regD |= (unit ? 0x80 : 0x20); break;
-		case XFER_SW_DMA_0:	regD |= (unit ? 0xC0 : 0x30); break;
-		case XFER_PIO_5:
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:
-			(void) cmd64x_tune_pio(drive, speed - XFER_PIO_0);
-			break;
-
-		default:
-			return 1;
+	case XFER_UDMA_5:
+		regU |= unit ? 0x0A : 0x05;
+		break;
+	case XFER_UDMA_4:
+		regU |= unit ? 0x4A : 0x15;
+		break;
+	case XFER_UDMA_3:
+		regU |= unit ? 0x8A : 0x25;
+		break;
+	case XFER_UDMA_2:
+		regU |= unit ? 0x42 : 0x11;
+		break;
+	case XFER_UDMA_1:
+		regU |= unit ? 0x82 : 0x21;
+		break;
+	case XFER_UDMA_0:
+		regU |= unit ? 0xC2 : 0x31;
+		break;
+	case XFER_PIO_5:
+	case XFER_PIO_4:
+	case XFER_PIO_3:
+	case XFER_PIO_2:
+	case XFER_PIO_1:
+	case XFER_PIO_0:
+		(void) cmd64x_tune_pio(drive, speed - XFER_PIO_0);
+		break;
+	default:
+		return 1;
 	}
 
-	if (speed >= XFER_SW_DMA_0) {
+	if (speed >= XFER_SW_DMA_0)
 		(void) pci_write_config_byte(dev, pciU, regU);
-		regD |= (unit ? 0x40 : 0x20);
-		(void) pci_write_config_byte(dev, pciD, regD);
-	}
 
-	return (ide_config_drive_speed(drive, speed));
+	return ide_config_drive_speed(drive, speed);
 }
 
 static int config_chipset_for_dma (ide_drive_t *drive)
@@ -684,8 +678,6 @@ static void __devinit init_hwif_cmd64x(i
 	hwif->atapi_dma = 1;
 
 	hwif->ultra_mask = 0x3f;
-	hwif->mwdma_mask = 0x07;
-	hwif->swdma_mask = 0x07;
 
 	if (dev->device == PCI_DEVICE_ID_CMD_643)
 		hwif->ultra_mask = 0x80;