IDE janitoring comments

IDE janitoring comments

[Benjamin Herrenschmidt]

A few notes taken while adapting the ide-pmac (MMIO) code
to the new layer in -ac:

 - Do we really want to keep all those _P versions around ?
A quick grep showed _only_ by the non-portable x86 specific
recovery timer stuff that taps ISA timers (well, I think ports
0x40 and 0x43 and an ISA timer). I would strongly suggest to
get rid of those functions from the hwif, and use directly
the appropriate PIO functon i that timer routine. Also make
sure that timer stuff don't get compiled on anything but
legacy harware where we know those ports mean something.

 - In ide-iops, the insw, insl, outsw, outsl functions are
broken for big endian. They should not do byteswap on these,
however, implemeting them with a loop of IN/OUT_BYTE/WORD
will cause byteswapped access on archs like PPC.
The problem is that the macros IN/OUT_BYTE/WORD don't define
non-swapping equivalents that would allow us to correctly
implement the "s" versions.

After much thinking about the above, I came to the conslusion
we probably want to just kill all the IN_BYTE, OUT_BYTE, etc...
macros and use directly inb/outb/etc... in the default PIO
ops defined in ide-iops.c. We would then use the normal arch
provided insw/insl/outsw/outsl functions here as well.

I already added a set of ops for MMIO in there using the normal
readb/writeb/... and for the "multiple" versions, I did loops
using the __raw versions (no byteswap) and a manual io_barrier().

I grep'ed in 2.4, the only archs that use custom IN/OUT_BYTE
macros for IDE are m68k and cris. I'm pretty sure m68k is wrong
and should do like PPC :) In anyway, both should be fixed the
new 'clean' way which is to define a set of access functions
to be stuffed in the HWIF structure. The goal of ide-iops is
to provide defaults for PIO (and MMIO soon as those can be made
fairly generic too).

Also, getting rid of the _P version would make things a lot
easier as well here too.

Any comments ?

Ben.

Re: IDE janitoring comments

[Alan Cox]

On Sat, 2002-08-24 at 16:15, Benjamin Herrenschmidt wrote:
>  - Do we really want to keep all those _P versions around ?
> A quick grep showed _only_ by the non-portable x86 specific
> recovery timer stuff that taps ISA timers (well, I think ports
> 0x40 and 0x43 and an ISA timer). I would strongly suggest to

I'd like to keep them around for the moment. They should be using
udelay() but thats a general issue with _p inb/outb etc.

> After much thinking about the above, I came to the conslusion
> we probably want to just kill all the IN_BYTE, OUT_BYTE, etc.

Agreed entirely


> Also, getting rid of the _P version would make things a lot
> easier as well here too.

What currently uses the _P versions ?

Re: IDE janitoring comments

[Andre Hedrick]

Yep, just be careful of how to decouple the hwif->iops from procfs for pci
and the general lameness of x86 centric issues.

On 24 Aug 2002, Alan Cox wrote:

> On Sat, 2002-08-24 at 16:15, Benjamin Herrenschmidt wrote:
> >  - Do we really want to keep all those _P versions around ?
> > A quick grep showed _only_ by the non-portable x86 specific
> > recovery timer stuff that taps ISA timers (well, I think ports
> > 0x40 and 0x43 and an ISA timer). I would strongly suggest to
> 
> I'd like to keep them around for the moment. They should be using
> udelay() but thats a general issue with _p inb/outb etc.
> 
> > After much thinking about the above, I came to the conslusion
> > we probably want to just kill all the IN_BYTE, OUT_BYTE, etc.
> 
> Agreed entirely
> 
> 
> > Also, getting rid of the _P version would make things a lot
> > easier as well here too.
> 
> What currently uses the _P versions ?
> 

Andre Hedrick
LAD Storage Consulting Group

Re: IDE janitoring comments

[Benjamin Herrenschmidt]

>>  - Do we really want to keep all those _P versions around ?
>> A quick grep showed _only_ by the non-portable x86 specific
>> recovery timer stuff that taps ISA timers (well, I think ports
>> 0x40 and 0x43 and an ISA timer). I would strongly suggest to
>
>I'd like to keep them around for the moment. They should be using
>udelay() but thats a general issue with _p inb/outb etc.

I don't think we need them at all in hwif (see below)

>> After much thinking about the above, I came to the conslusion
>> we probably want to just kill all the IN_BYTE, OUT_BYTE, etc.
>
>Agreed entirely
>
>
>> Also, getting rid of the _P version would make things a lot
>> easier as well here too.
>
>What currently uses the _P versions ?

Ok, I looked and found out that those are used by 

 - some weird stuff in ide.c tapping IO ports at 0x40 and 0x43 that
I assume is a timer. (Can someone make sure that code don't get used
on anything but legacy x86 ?).

 - a couple of interface drivers like cmd640 tapping the PCI config
IO stuffs for probing.

In all cases, I firmly beleive those are way outside of the domain
of application of the hwif-> abstraction. Those are code that knows
it's tapping legacy stuff on a IO port, and can/should directly use
the in/out_p function. I don't think those have anything to do in
hwif. All that should be in hwif is what is needed by the generic
code to tap the IDE registers themselves.

Do you agree ? (I'd love to get rid of them :)

If you agree, I'll send you a patch tomorrow along with the fixing
of ide-pmac that will do

 - Remove the _P versions from hwif->iops
 - slightly change ide-iops to define both sets of iops, one set
   providing PIO ops using directly in/outx & int/outsx, and one
   set providing MMIO ops using directly read/writex

Anybody that need different routines for the generic IDE code to
tap the taskfile (or eventually DMA) registers (typically cris
arch) will provide it's own set of routines to hwif. I can probably
fix cris (though I don't know anything about that arch, but the
code seem obvious enough), and i'll rely on you to fix m86k :)


Ben.

Re: IDE janitoring comments

[Andre Hedrick]

I have part of cris fixed

On Sun, 25 Aug 2002, Benjamin Herrenschmidt wrote:

> >>  - Do we really want to keep all those _P versions around ?
> >> A quick grep showed _only_ by the non-portable x86 specific
> >> recovery timer stuff that taps ISA timers (well, I think ports
> >> 0x40 and 0x43 and an ISA timer). I would strongly suggest to
> >
> >I'd like to keep them around for the moment. They should be using
> >udelay() but thats a general issue with _p inb/outb etc.
> 
> I don't think we need them at all in hwif (see below)
> 
> >> After much thinking about the above, I came to the conslusion
> >> we probably want to just kill all the IN_BYTE, OUT_BYTE, etc.
> >
> >Agreed entirely
> >
> >
> >> Also, getting rid of the _P version would make things a lot
> >> easier as well here too.
> >
> >What currently uses the _P versions ?
> 
> Ok, I looked and found out that those are used by 
> 
>  - some weird stuff in ide.c tapping IO ports at 0x40 and 0x43 that
> I assume is a timer. (Can someone make sure that code don't get used
> on anything but legacy x86 ?).
> 
>  - a couple of interface drivers like cmd640 tapping the PCI config
> IO stuffs for probing.
> 
> In all cases, I firmly beleive those are way outside of the domain
> of application of the hwif-> abstraction. Those are code that knows
> it's tapping legacy stuff on a IO port, and can/should directly use
> the in/out_p function. I don't think those have anything to do in
> hwif. All that should be in hwif is what is needed by the generic
> code to tap the IDE registers themselves.
> 
> Do you agree ? (I'd love to get rid of them :)
> 
> If you agree, I'll send you a patch tomorrow along with the fixing
> of ide-pmac that will do
> 
>  - Remove the _P versions from hwif->iops
>  - slightly change ide-iops to define both sets of iops, one set
>    providing PIO ops using directly in/outx & int/outsx, and one
>    set providing MMIO ops using directly read/writex
> 
> Anybody that need different routines for the generic IDE code to
> tap the taskfile (or eventually DMA) registers (typically cris
> arch) will provide it's own set of routines to hwif. I can probably
> fix cris (though I don't know anything about that arch, but the
> code seem obvious enough), and i'll rely on you to fix m86k :)
> 
> 
> Ben.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Andre Hedrick
LAD Storage Consulting Group

IDE janitoring comments

[Benjamin Herrenschmidt]

A few notes taken while adapting the ide-pmac (MMIO) code
to the new layer in -ac:

 - Do we really want to keep all those _P versions around ?
A quick grep showed _only_ by the non-portable x86 specific
recovery timer stuff that taps ISA timers (well, I think ports
0x40 and 0x43 and an ISA timer). I would strongly suggest to
get rid of those functions from the hwif, and use directly
the appropriate PIO functon i that timer routine. Also make
sure that timer stuff don't get compiled on anything but
legacy harware where we know those ports mean something.

 - In ide-iops, the insw, insl, outsw, outsl functions are
broken for big endian. They should not do byteswap on these,
however, implemeting them with a loop of IN/OUT_BYTE/WORD
will cause byteswapped access on archs like PPC.
The problem is that the macros IN/OUT_BYTE/WORD don't define
non-swapping equivalents that would allow us to correctly
implement the "s" versions.

After much thinking about the above, I came to the conslusion
we probably want to just kill all the IN_BYTE, OUT_BYTE, etc...
macros and use directly inb/outb/etc... in the default PIO
ops defined in ide-iops.c. We would then use the normal arch
provided insw/insl/outsw/outsl functions here as well.

I already added a set of ops for MMIO in there using the normal
readb/writeb/... and for the "multiple" versions, I did loops
using the __raw versions (no byteswap) and a manual io_barrier().

I grep'ed in 2.4, the only archs that use custom IN/OUT_BYTE
macros for IDE are m68k and cris. I'm pretty sure m68k is wrong
and should do like PPC :) In anyway, both should be fixed the
new 'clean' way which is to define a set of access functions
to be stuffed in the HWIF structure. The goal of ide-iops is
to provide defaults for PIO (and MMIO soon as those can be made
fairly generic too).

Also, getting rid of the _P version would make things a lot
easier as well here too.

Any comments ?

Ben.

Re: IDE janitoring comments

[Richard Zidlicky]

On Sat, Aug 24, 2002 at 05:09:16PM +0200, Benjamin Herrenschmidt wrote:

>  - In ide-iops, the insw, insl, outsw, outsl functions are
> broken for big endian. They should not do byteswap on these,
> however, implemeting them with a loop of IN/OUT_BYTE/WORD
> will cause byteswapped access on archs like PPC.
> The problem is that the macros IN/OUT_BYTE/WORD don't define
> non-swapping equivalents that would allow us to correctly
> implement the "s" versions. 

we have one special problem on m68k, on some machines the IDE
bus is byteswapped (unrelated to cpu endianness). For historical 
and performance reasons data to the HD is by default read and 
written in this "wrong" order (thus the bswap/swapdata option)
and special fixup code is used in ide_fix_driveid (see 
M68K_IDE_SWAPW). However data returned by IDE_DRIVE_CMD is not 
treated in any way, so that eg WIN_SMART data end up in the 
wrong order on those machines and this is something I would 
like to fix properly.
I figure I would define ata_*_{control,data} to handle special
data resp raw HD data and modify ide_handler_parser to return
specialised interrupt handlers or set some additional flag.

Any thoughts?

Richard

Re: IDE janitoring comments

[Benjamin Herrenschmidt]

>we have one special problem on m68k, on some machines the IDE
>bus is byteswapped (unrelated to cpu endianness). For historical 
>and performance reasons data to the HD is by default read and 
>written in this "wrong" order (thus the bswap/swapdata option)
>and special fixup code is used in ide_fix_driveid (see 
>M68K_IDE_SWAPW). However data returned by IDE_DRIVE_CMD is not 
>treated in any way, so that eg WIN_SMART data end up in the 
>wrong order on those machines and this is something I would 
>like to fix properly.
>I figure I would define ata_*_{control,data} to handle special
>data resp raw HD data and modify ide_handler_parser to return
>specialised interrupt handlers or set some additional flag.
>
>Any thoughts?

You just need to provide your own ide-ops doing the right
thing, I don't think you need to touch ata_*_data if you
properly implement "s" ops {in,out}s{b,w,l} for your hwif.

Ben.

Re: IDE janitoring comments

[Andre Hedrick]

Poke in your own special ide-ops function pointers.
This should have been allowed on a per chipset/channel bases.

Did I miss something?

On Tue, 24 Sep 2002, Richard Zidlicky wrote:

> On Sat, Aug 24, 2002 at 05:09:16PM +0200, Benjamin Herrenschmidt wrote:
> 
> >  - In ide-iops, the insw, insl, outsw, outsl functions are
> > broken for big endian. They should not do byteswap on these,
> > however, implemeting them with a loop of IN/OUT_BYTE/WORD
> > will cause byteswapped access on archs like PPC.
> > The problem is that the macros IN/OUT_BYTE/WORD don't define
> > non-swapping equivalents that would allow us to correctly
> > implement the "s" versions. 
> 
> we have one special problem on m68k, on some machines the IDE
> bus is byteswapped (unrelated to cpu endianness). For historical 
> and performance reasons data to the HD is by default read and 
> written in this "wrong" order (thus the bswap/swapdata option)
> and special fixup code is used in ide_fix_driveid (see 
> M68K_IDE_SWAPW). However data returned by IDE_DRIVE_CMD is not 
> treated in any way, so that eg WIN_SMART data end up in the 
> wrong order on those machines and this is something I would 
> like to fix properly.
> I figure I would define ata_*_{control,data} to handle special
> data resp raw HD data and modify ide_handler_parser to return
> specialised interrupt handlers or set some additional flag.
> 
> Any thoughts?
> 
> Richard
> 

Andre Hedrick
LAD Storage Consulting Group

Re: IDE janitoring comments

[Richard Zidlicky]

On Mon, Sep 23, 2002 at 05:28:03PM -0700, Andre Hedrick wrote:
> 
> Poke in your own special ide-ops function pointers.
> This should have been allowed on a per chipset/channel bases.

I need different transfer functions depending on whether drive
control data(like IDENT,SMART) or HD sectors are to be transfered. 
Control data requires byteswapping to correct bus-byteorder
whereas sector r/w has to be raw for compatibility.

So that will require 2 additional iops pointers and some change
in ide_handler_parser or ide_cmd_type_parser to select the
appropriate version depending on the drive command.

Richard

Re: IDE janitoring comments

[Benjamin Herrenschmidt]

>I need different transfer functions depending on whether drive
>control data(like IDENT,SMART) or HD sectors are to be transfered. 
>Control data requires byteswapping to correct bus-byteorder
>whereas sector r/w has to be raw for compatibility.
>
>So that will require 2 additional iops pointers and some change
>in ide_handler_parser or ide_cmd_type_parser to select the
>appropriate version depending on the drive command.

No, it doesn't. There are already separate iops for control
and datas, typically {in,out}{b,w,l} are for control (though
only "b" versions are really useful and {in,out}s{b,w,l} are
for datas.

There are cases where ide_{input,output}_data my try to
"re-invent" the "s" functions with a loop of non-s ones,
but you shouldn't have to care about that case. It might
actually work for you because of your weird wiring, but it's
definitely broken for other BE archs, and so drive->slow
shouldn't be set on anything but x86.

Actually, the whole set of iops could probably be shrunk
down to just {in,out}b for control and {in,out}s{w,l} for
datas. Though we probably want, ultimately, to change that
to some different (higher level ?) kind of abstraction.

Ben.

Re: IDE janitoring comments

[Benjamin Herrenschmidt]

>>I need different transfer functions depending on whether drive
>>control data(like IDENT,SMART) or HD sectors are to be transfered. 
>>Control data requires byteswapping to correct bus-byteorder
>>whereas sector r/w has to be raw for compatibility.
>>
>>So that will require 2 additional iops pointers and some change
>>in ide_handler_parser or ide_cmd_type_parser to select the
>>appropriate version depending on the drive command.
>
>No, it doesn't. There are already separate iops for control
>and datas, typically {in,out}{b,w,l} are for control (though
>only "b" versions are really useful and {in,out}s{b,w,l} are
>for datas.

Oops, sorry, I mis-read you

Well, if you have proper iops that swap for normal datas, then
you can use the "normal" fixup routines for control datas
like ident, the same we use on PPC or other BE archs.

The problem is typiucally the same for everybody here: the
swapping of datas themselves must be done so that you get an
exact image of the datas in memory, then you need additional
fixup to "interpret" some of these (ident, smart, ...)

Ben.

Re: IDE janitoring comments

[Andre Hedrick]

Switch the transport on the fly for the channel based on the OPCODE.

Easy to say harder to do.

Andre Hedrick
LAD Storage Consulting Group

On Tue, 24 Sep 2002, Richard Zidlicky wrote:

> On Mon, Sep 23, 2002 at 05:28:03PM -0700, Andre Hedrick wrote:
> > 
> > Poke in your own special ide-ops function pointers.
> > This should have been allowed on a per chipset/channel bases.
> 
> I need different transfer functions depending on whether drive
> control data(like IDENT,SMART) or HD sectors are to be transfered. 
> Control data requires byteswapping to correct bus-byteorder
> whereas sector r/w has to be raw for compatibility.
> 
> So that will require 2 additional iops pointers and some change
> in ide_handler_parser or ide_cmd_type_parser to select the
> appropriate version depending on the drive command.
> 
> Richard
>