Curently in 2.5 (afaik in -ac too), the ide-iops "s" routines used
to transfer datas in/out the data port are incorrect for big endian
machines. They are implemented with a loop of inw/outw which are
byteswapping, but a fifo transfer like that mustn't be swapped.
Here's a patch against current 2.5 (that should apply to 2.4 -ac too)
that fixes those with a #define for now. I have done some further
cleanup of the iops (removing the {IN,OUT}{BYTE,WORD,LONG} macros
and removing the "P" iops from the hwif structure) too, but Alan
didn't accept those yet.
I enclosed the patch as an attachement too in case the mailer screws
it up...
===== drivers/ide/ide-iops.c 1.1 vs edited =====
--- 1.1/drivers/ide/ide-iops.c Wed Sep 11 08:54:11 2002
+++ edited/drivers/ide/ide-iops.c Wed Sep 25 14:19:58 2002
@@ -54,12 +54,20 @@
static inline void ide_insw (u32 port, void *addr, u32 count)
{
+#ifdef __BIG_ENDIAN
+ insw(port, addr, count);
+#else
while (count--) { *(u16 *)addr = IN_WORD(port); addr += 2; }
+#endif
}
static inline void ide_insw_p (u32 port, void *addr, u32 count)
{
- while (count--) { *(u16 *)addr = IN_WORD_P(port); addr += 2; }
+#ifdef __BIG_ENDIAN
+ insw(port, addr, count);
+#else
+ while (count--) { *(u16 *)addr = IN_WORD(port); addr += 2; }
+#endif
}
static inline u32 ide_inl (u32 port)
@@ -106,12 +114,20 @@
static inline void ide_outsw (u32 port, void *addr, u32 count)
{
+#ifdef __BIG_ENDIAN
+ outsw(port, addr, count);
+#else
while (count--) { OUT_WORD(*(u16 *)addr, port); addr += 2; }
+#endif
}
static inline void ide_outsw_p (u32 port, void *addr, u32 count)
{
+#ifdef __BIG_ENDIAN
+ outsw(port, addr, count);
+#else
while (count--) { OUT_WORD_P(*(u16 *)addr, port); addr += 2; }
+#endif
}
static inline void ide_outl (u32 addr, u32 port)
> From: "Benjamin Herrenschmidt" <[email protected]>
> Date: Wed, 25 Sep 2002 14:32:23 +0200
> Curently in 2.5 (afaik in -ac too), the ide-iops "s" routines used
> to transfer datas in/out the data port are incorrect for big endian
> machines. They are implemented with a loop of inw/outw which are
> byteswapping, but a fifo transfer like that mustn't be swapped.
Dunno about ppc, but sparc works just fine as it is in 2.4.
When was the last time you examined include/asm-sparc/ide.h?
IDE uses ide_insw instead of plain insw specifically to
resolve this kind of issue, and you are trying to defeat
the mechanism designed to help you. I smell a fish here.
-- Pete
On Wed, 25 Sep 2002, Benjamin Herrenschmidt wrote:
> --- 1.1/drivers/ide/ide-iops.c Wed Sep 11 08:54:11 2002
> +++ edited/drivers/ide/ide-iops.c Wed Sep 25 14:19:58 2002
> @@ -54,12 +54,20 @@
>
> static inline void ide_insw (u32 port, void *addr, u32 count)
> {
> +#ifdef __BIG_ENDIAN
> + insw(port, addr, count);
> +#else
> while (count--) { *(u16 *)addr = IN_WORD(port); addr += 2; }
> +#endif
If insw is correct on big-endian, then it sure as hell should be correct
on little-endian. I don't understand why we wouldn't use insw on PC's,
since it's smaller and much more traditional.
Linus
From: Pete Zaitcev <[email protected]>
Date: Wed, 25 Sep 2002 14:19:46 -0400
IDE uses ide_insw instead of plain insw specifically to
resolve this kind of issue, and you are trying to defeat
the mechanism designed to help you. I smell a fish here.
They're trying to abstract out as much as possible in 2.5.x, maybe a
bit too much :-)
>
>On Wed, 25 Sep 2002, Benjamin Herrenschmidt wrote:
>> --- 1.1/drivers/ide/ide-iops.c Wed Sep 11 08:54:11 2002
>> +++ edited/drivers/ide/ide-iops.c Wed Sep 25 14:19:58 2002
>> @@ -54,12 +54,20 @@
>>
>> static inline void ide_insw (u32 port, void *addr, u32 count)
>> {
>> +#ifdef __BIG_ENDIAN
>> + insw(port, addr, count);
>> +#else
>> while (count--) { *(u16 *)addr = IN_WORD(port); addr += 2; }
>> +#endif
>
>If insw is correct on big-endian, then it sure as hell should be correct
>on little-endian. I don't understand why we wouldn't use insw on PC's,
>since it's smaller and much more traditional.
Well, i'm pretty sure it is, though I didn't want to post a patch
affecting what currently work, I prefer letting you or other
x86 addicts figure that out :)
In the case of ide_inswp though, there is a real problem as there
is no equivalent of the "p" functions on non-x86 anyway, and you
can't easily reproduce the "insw" semantics with {in,out}{w,l}
without adding useless double-byteswap on BE. But on the other
hand, the IDE layer doesn't use ide_inswp() callback, and the
case where "p" functions are used is currently broken on BE
as well (see ide_{input,output}_data when drive->slow is true).
I suggest that you get this patch in asap (eventually swithing
to insw unconditionally in ide_insw) so at least 2.5 works again
properly on BE, further cleanup of the iops is pending, I'm waiting
for Alan own experiments before I push again my own that remove
all "p" iops and all of the {IN,OUT}{BYTE,WORD,LONG} macros.
Ben.
>> Curently in 2.5 (afaik in -ac too), the ide-iops "s" routines used
>> to transfer datas in/out the data port are incorrect for big endian
>> machines. They are implemented with a loop of inw/outw which are
>> byteswapping, but a fifo transfer like that mustn't be swapped.
>
>Dunno about ppc, but sparc works just fine as it is in 2.4.
>When was the last time you examined include/asm-sparc/ide.h?
I'm talking about 2.4-ac with the new IDE code, not Marcelo
2.4.
>IDE uses ide_insw instead of plain insw specifically to
>resolve this kind of issue, and you are trying to defeat
>the mechanism designed to help you. I smell a fish here.
Again, I'm talking about the new layer. In this, the iops
functions are no longer macros defined by include/asm*/ide.h,
but are now function pointers inside the hwif structure
(so we can now deal with real MMIO or CPU register based IDE
without playing macro tricks in asm*/ide.h).
The problem resides in the default implementation of those
iops in the new ide-iops.c file, which currently re-implements
insw as a loop of IN_WORD, which usually translates to inw,
and thus would cause incorrect endianness.
My patch fixes that. Ultimately, we want to completely get
rid of the uppercase {IN,OUT}{BYTE,WORD,LONG} macros though.
The principle is that ide-iops.c will provide reasonable
"default" ops for PIO (and MMIO if my next patch gets in),
any "different" interface can provide it's own ops, and you
keep the ability to mix different kind of interfaces on the
same machine (like an embeded CPU-register based interface
_and_ a PCI based interface using different iops).
Ben.
> IDE uses ide_insw instead of plain insw specifically to
> resolve this kind of issue, and you are trying to defeat
> the mechanism designed to help you. I smell a fish here.
>
>They're trying to abstract out as much as possible in 2.5.x, maybe a
>bit too much :-)
Well, no, we are changing the abstraction but didn't quite yet
remove rests of the old one ;)
Ben.
>On Wed, 2002-09-25 at 13:32, Benjamin Herrenschmidt wrote:
>> I enclosed the patch as an attachement too in case the mailer screws
>> it up...
>
>Please do one thing. For the stuff that needs weird powerpcisms put all
>the seperate stuff in one block with its own copy of the static inlines
>so we dont have in ifdef in half the functions in the file
This is _NOT_ a weird PowerPC'ism !!!
It's just a matter of use the proper operation, that is "insw/outsw"
instead of trying to re-implement it with a loop of basic inw/outw,
which is wrong for _any_ BE arch (except maybe weird m68k'ism where
IDE is wired the wrong way around)
Ben.
On Wed, 2002-09-25 at 23:44, Benjamin Herrenschmidt wrote:
> properly on BE, further cleanup of the iops is pending, I'm waiting
> for Alan own experiments before I push again my own that remove
> all "p" iops and all of the {IN,OUT}{BYTE,WORD,LONG} macros.
Thats true in current -ac. I killed the _p crap. Nobody uses it, the
switching for handling it is bogus anyway. If anyone has such broken
code they can implement ide-iops-speak-slowly-after-the-tone.c
On Wed, 2002-09-25 at 13:32, Benjamin Herrenschmidt wrote:
> I enclosed the patch as an attachement too in case the mailer screws
> it up...
Please do one thing. For the stuff that needs weird powerpcisms put all
the seperate stuff in one block with its own copy of the static inlines
so we dont have in ifdef in half the functions in the file
>Thats true in current -ac. I killed the _p crap. Nobody uses it, the
>switching for handling it is bogus anyway. If anyone has such broken
>code they can implement ide-iops-speak-slowly-after-the-tone.c
Ok, now go one step further and remove the {IN,OUT}{BYTE,WORD,LONG}
macros and you'll end up with the stuff I had send you previously ;)
Regarding the MMIO ops, where indeed I had some #ifdef CONFIG_PPC
crap, that was because we lack proper abstraction of either an
MMIO version of insw/outsw, or of the barrier (see previous
discussion we had on this issue). It is by no mean yet another
PowerPC'ism ;)
Ben.
>> properly on BE, further cleanup of the iops is pending, I'm waiting
>> for Alan own experiments before I push again my own that remove
>> all "p" iops and all of the {IN,OUT}{BYTE,WORD,LONG} macros.
>
>Thats true in current -ac. I killed the _p crap. Nobody uses it, the
>switching for handling it is bogus anyway. If anyone has such broken
>code they can implement ide-iops-speak-slowly-after-the-tone.c
Ok, I finally went and looked at your tree for real and I see
the cleanup is actually there, good :)
So now, let's see how to get rid of those CONFIG_PPC, either by
having everybody define iobarrier_*() or having {read,write}s{b,w,l},
I just started a new thread on the list just about that.
Jens: can you look into merging -ac's iops change to 2.5 ? That
would fix it.
Ben.
On Thu, Sep 26, 2002 at 05:14:14PM +0200, Benjamin Herrenschmidt wrote:
> >On Wed, 2002-09-25 at 13:32, Benjamin Herrenschmidt wrote:
> >> I enclosed the patch as an attachement too in case the mailer screws
> >> it up...
> >
> >Please do one thing. For the stuff that needs weird powerpcisms put all
> >the seperate stuff in one block with its own copy of the static inlines
> >so we dont have in ifdef in half the functions in the file
>
> This is _NOT_ a weird PowerPC'ism !!!
>
> It's just a matter of use the proper operation, that is "insw/outsw"
> instead of trying to re-implement it with a loop of basic inw/outw,
> which is wrong for _any_ BE arch (except maybe weird m68k'ism where
> IDE is wired the wrong way around)
to put it a bit more precise, this is bus endianness, nothing to
do with arch endianness.
Richard
>to put it a bit more precise, this is bus endianness, nothing to
>do with arch endianness.
Well, right, though this is also the way a PCI bus is supposed
to be wired to a BE CPU, and is the most common way to wire a
bus with ISA-like chipsets (16 bits busses) on a BE core ;)
Ben.