Re: [PATCH] M68k IDE updates

Re: [PATCH] M68k IDE updates

[Richard Zidlicky]

> Date: 22 Apr 2003 15:49:15 +0100
> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> To: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Linus Torvalds <torvalds@transmeta.com>, Paul Mackerras <paulus@samba.org>,
>      Linux Kernel Development <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] M68k IDE updates
> 
> On Llu, 2003-04-21 at 17:55, Geert Uytterhoeven wrote:
> > However, there's also a routine that involves more magic:
> > taskfile_lib_get_identify(). While trying to understand that one, I found more
> > commands that should call the (possible byteswapping) hwif->ata_input_id()
> > operations, like SMART commands. So first we need a clearer differentiation
> > between commands that transfer on-platter data, or other drive data.
> > 
> > Any comments from the IDE experts?
> 
> Only one, stop abusing the IDE layer and do your byte swapping via a loopback/md 
> or similar piece of code.

It seems that Geert´ idea would fit neatly into the current IDE 
system. Endianness of on disk data and drive control data are
clearly different things. A while ago Andre suggested to switch
the transport based on opcode to make it work, it might be even
more straightforward to set some flag when the handler is selected
or take a distinct handler altogether (ide_cmd_type_parser or
ide_handler_parser).

Otoh trying to solve that with loopback would mean new kernels 
wouldn´t even see the partition table of old installed harddisks
on some machines. Fixing that would require an initial ramdisk
that does setup the loopback device according to kernel version,
reading the partition table from a loop device and magically make
it appear compatible to the old devices.

Richard

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Mer, 2003-04-23 at 12:27, Richard Zidlicky wrote:
> It seems that Geert´ idea would fit neatly into the current IDE 
> system. Endianness of on disk data and drive control data are
> clearly different things. A while ago Andre suggested to switch
> the transport based on opcode to make it work, it might be even
> more straightforward to set some flag when the handler is selected
> or take a distinct handler altogether (ide_cmd_type_parser or
> ide_handler_parser).

Thats over complicating stuff I think.

> Otoh trying to solve that with loopback would mean new kernels 
> wouldn´t even see the partition table of old installed harddisks
> on some machines. 

Which is a real pain. I think its the right 2.5 answer. I'm not happy
about breaking that (even if its only for the m68k userbase in 2.4)
either.

I don't think command parsing is the right place. Turn your IDE layer
"right endian" and most stuff begins to look a lot saner already. 
The "fixing" needs to be happening at the top of the IDE layer not
in the driver itself. For 2.5 that ought to be loopback or similar
for 2.4 it makes sense I think to effectively implement an endian
switcher without loopback for compatibility.

Re: [PATCH] M68k IDE updates

[John Bradford]

> 
> On Mer, 2003-04-23 at 12:27, Richard Zidlicky wrote:
> > It seems that Geert=C2=B4 idea would fit neatly into the current IDE=20
> > system. Endianness of on disk data and drive control data are
> > clearly different things. A while ago Andre suggested to switch
> > the transport based on opcode to make it work, it might be even
> > more straightforward to set some flag when the handler is selected
> > or take a distinct handler altogether (ide_cmd_type_parser or
> > ide_handler_parser).
> 
> Thats over complicating stuff I think.
> 
> > Otoh trying to solve that with loopback would mean new kernels=20
> > wouldn=C2=B4t even see the partition table of old installed harddisks
> > on some machines.=20
> 
> Which is a real pain. I think its the right 2.5 answer. I'm not happy
> about breaking that (even if its only for the m68k userbase in 2.4)
> either.
> 
> I don't think command parsing is the right place. Turn your IDE layer
> "right endian" and most stuff begins to look a lot saner already.=20
> The "fixing" needs to be happening at the top of the IDE layer not
> in the driver itself. For 2.5 that ought to be loopback or similar
> for 2.4 it makes sense I think to effectively implement an endian
> switcher without loopback for compatibility.

Moving byte swapping out of the IDE layer also means that dumping the
whole disk to a file will then give you non-byte swapped data, which
could then be written back to a disk on another machine without a
byte-swapped IDE interface.

It will also allow you to exchange tar archives on raw hard disk
devices, and have them readable on non-byte swapped IDE interfaces
:-).

John.

Re: [PATCH] M68k IDE updates

[Richard Zidlicky]

On Wed, Apr 23, 2003 at 09:19:07PM +0100, John Bradford wrote:
 
> Moving byte swapping out of the IDE layer also means that dumping the
> whole disk to a file will then give you non-byte swapped data, which
> could then be written back to a disk on another machine without a
> byte-swapped IDE interface.
> 
> It will also allow you to exchange tar archives on raw hard disk
> devices, and have them readable on non-byte swapped IDE interfaces
> :-).

Linux is much better and will do all of that work perfectly since 
ages, just use hdX=swapdata on one of the machines. Trivially it 
is also possible to mount the partitions on either architecture.

The issue is the distinction between drive control data (like SMART
and ident) and on disk data - currently the ide layer makes no clear
distinction between those varieties and thus in some cases the 
byteswapping must be done (or undone) in userspace.

There are some reasons why I am hesitant to move it out of IDE:

- IDE knows best what is control or on disk data and
  having separate ide-iops for them would be trivial
- partitioning loop devices smells like plenty of fun
- performance, for compatibility with current kernels
  we would byteswap in IDE and undo it in the loop layer
- don´t like to rely on utterly complicated boot ramdisks

Richard

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Thu, 24 Apr 2003, Richard Zidlicky wrote:
> On Wed, Apr 23, 2003 at 09:19:07PM +0100, John Bradford wrote:
> > Moving byte swapping out of the IDE layer also means that dumping the
> > whole disk to a file will then give you non-byte swapped data, which
> > could then be written back to a disk on another machine without a
> > byte-swapped IDE interface.
> > 
> > It will also allow you to exchange tar archives on raw hard disk
> > devices, and have them readable on non-byte swapped IDE interfaces
> > :-).
> 
> Linux is much better and will do all of that work perfectly since 
> ages, just use hdX=swapdata on one of the machines. Trivially it 
> is also possible to mount the partitions on either architecture.

Note that the swapping works for PIO only, IIRC. For DMA mode, you're limited
to the loop solution.

> The issue is the distinction between drive control data (like SMART
> and ident) and on disk data - currently the ide layer makes no clear
> distinction between those varieties and thus in some cases the 
> byteswapping must be done (or undone) in userspace.
> 
> There are some reasons why I am hesitant to move it out of IDE:
> 
> - IDE knows best what is control or on disk data and
>   having separate ide-iops for them would be trivial
> - partitioning loop devices smells like plenty of fun
> - performance, for compatibility with current kernels
>   we would byteswap in IDE and undo it in the loop layer
> - donŽt like to rely on utterly complicated boot ramdisks

After some more thinking, Alan's suggestion (always doing the swapping) isn't
that bad. Except for the loop layer on old slow machines, which I'd like to
avoid.

If we always swap, we only have to un-swap when reading/writing platter data
from native disks. No more swapping has to be done in ide_fix_driveid(), apart
from the obvious conversion from little to big endian of the driveid structure
itself, which we cannot avoid.

Since both Atari and Q40/Q60 use PIO only, this affects ata_{in,out}put_data()
only. It's quite easy to add a swap flag to ide_drive_t (configurable through
hdX=swapdata), that is checked in ata_{in,out}put_data().  To improve
performance, we wouldn't swap twice, but just call the new routines
hwif->{IN,OUT}S[WL]_NOSWAP.

Note that this would be the inverse logic of the current scheme, which swaps
conditionally in atapi_{in,out}put_bytes().

All of this can be protected by #ifdef CONFIG_IDE_BYTESWAPPED_HWIF. Influence
on generic code is limited to ata_{in,out}put_data() and the new routines
hwif->{IN,OUT}S[WL]_NOSWAP.

Is this OK?

For reference, I think this is the list of machines with a byteswapped IDE
interface:
  - Atari Falcon
  - Q40/Q60
  - Tivo

Any others?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Richard Zidlicky]

On Thu, Apr 24, 2003 at 01:26:12PM +0200, Geert Uytterhoeven wrote:

> 
> After some more thinking, Alan's suggestion (always doing the swapping) isn't
> that bad. Except for the loop layer on old slow machines, which I'd like to
> avoid.

convincing by simplicity, oversimplification also may have drawbacks.
 
> If we always swap, we only have to un-swap when reading/writing platter data
> from native disks. No more swapping has to be done in ide_fix_driveid(), apart
> from the obvious conversion from little to big endian of the driveid structure
> itself, which we cannot avoid.

yes. Smartdata and everything would be correct, except for the disk
contents.

> Since both Atari and Q40/Q60 use PIO only, this affects ata_{in,out}put_data()
> only. It's quite easy to add a swap flag to ide_drive_t (configurable through
> hdX=swapdata), that is checked in ata_{in,out}put_data().  To improve
> performance, we wouldn't swap twice, but just call the new routines
> hwif->{IN,OUT}S[WL]_NOSWAP.

contradicts previous paragraph? Still wrong smartdata etc unless
you mean to set the flag per request depending on the type of command
- which would be quite easy afaics. 

> All of this can be protected by #ifdef CONFIG_IDE_BYTESWAPPED_HWIF. Influence
> on generic code is limited to ata_{in,out}put_data() and the new routines
> hwif->{IN,OUT}S[WL]_NOSWAP.
> 
> Is this OK?

so to sum up, your idea is 
- cleanup and correct current solution for IDE_BYTESWAPPED_HWIF machines
- make all others use the loop layer

looks fine.

Richard

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Thu, 24 Apr 2003, Richard Zidlicky wrote:
> On Thu, Apr 24, 2003 at 01:26:12PM +0200, Geert Uytterhoeven wrote:
> > Since both Atari and Q40/Q60 use PIO only, this affects ata_{in,out}put_data()
> > only. It's quite easy to add a swap flag to ide_drive_t (configurable through
> > hdX=swapdata), that is checked in ata_{in,out}put_data().  To improve
> > performance, we wouldn't swap twice, but just call the new routines
> > hwif->{IN,OUT}S[WL]_NOSWAP.
> 
> contradicts previous paragraph? Still wrong smartdata etc unless
> you mean to set the flag per request depending on the type of command
> - which would be quite easy afaics. 

Oops, you're right.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

RE: [PATCH] M68k IDE updates

[Mudama, Eric]

-----Original Message-----
> Geert Uytterhoeven wrote:
>
> However, there's also a routine that involves more magic:
> taskfile_lib_get_identify(). While trying to understand that one, I found
more
> commands that should call the (possible byteswapping) hwif->ata_input_id()
> operations, like SMART commands. So first we need a clearer
differentiation
> between commands that transfer on-platter data, or other drive data.
> 
> Any comments from the IDE experts?
> 
> Gr{oetje,eeting}s,
> 
>						Geert


I'm hardly an expert, but all the specs can be found at http://www.t13.org

"typical" commands that involve platter data as of ATA-7 are:

COMMAND NAME			hex	decimal
===========================	===	=======

standard:

READ SECTOR(S)			20	32
READ SECTOR(S) w/ retry		21	33
WRITE SECTOR(S)			30	48
WRITE SECTOR(S) w/ retry	31	49
WRITE VERIFY			3c	60
READ MULTIPLE			c4	196
WRITE MULTIPLE			c5	197
READ DMA				c8	200
READ DMA w/ retry			c9	201
WRITE DMA				ca	202
WRITE DMA w/ retry		cb	203

48-bit feature set:

READ SECTOR(S) EXT		24	36
READ DMA EXT			25	37
READ MULTIPLE EXT			29	41
WRITE SECTOR(S) EXT		34	52
WRITE DMA EXT			35	53
WRITE MULTIPLE EXT		39	57

queued feature set:

READ DMA QUEUED EXT		26	38
WRITE DMA QUEUED EXT		36	54
SERVICE				a2	162
READ DMA QUEUED			c7	199
WRITE DMA QUEUED			cc	204

ata-7 fua feature set:

WRITE DMA FUA EXT			3d	61
WRITE DMA QUEUED FUA EXT	3e	62
WRITE MULTIPLE FUA EXT		ce	206

stream feature set:

READ STREAM DMA			2a	42
READ STREAM				2b	43
WRITE STREAM DMA			3a	58
WRITE STREAM			3b	59


Additionally I think the WRITE BUFFER / READ BUFFER pair are supposed to
work in the same format as a single sector write, however, they never go to
the media. (And of course byte-swapping doesn't matter, since if you issue a
read buffer, you must have immediately prior issued a write buffer command)

--eric

[PATCH] M68k IDE updates

[Geert Uytterhoeven]

M68k IDE updates: Add m68k-isms to the generic ide_fix_driveid()

--- linux-2.5.x/drivers/ide/ide-iops.c	Mon Sep 16 09:49:17 2002
+++ linux-m68k-2.5.x/drivers/ide/ide-iops.c	Wed Oct  2 23:01:40 2002
@@ -360,6 +360,23 @@
 	int i;
 	u16 *stringcast;
 
+#ifdef __mc68000__
+	if (!MACH_IS_AMIGA && !MACH_IS_MAC && !MACH_IS_Q40 && !MACH_IS_ATARI)
+		return;
+
+#ifdef M68K_IDE_SWAPW
+	if (M68K_IDE_SWAPW) {	/* fix bus byteorder first */
+		u_char *p = (u_char *)id;
+		u_char t;
+		for (i = 0; i < 512; i += 2) {
+			t = p[i];
+			p[i] = p[i+1];
+			p[i+1] = t;
+		}
+	}
+#endif
+#endif /* __mc68000__ */
+
 	id->config         = __le16_to_cpu(id->config);
 	id->cyls           = __le16_to_cpu(id->cyls);
 	id->reserved2      = __le16_to_cpu(id->reserved2);

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Sul, 2003-04-13 at 14:06, Geert Uytterhoeven wrote:
> +#ifdef M68K_IDE_SWAPW
> +	if (M68K_IDE_SWAPW) {	/* fix bus byteorder first */
> +		u_char *p = (u_char *)id;
> +		u_char t;
> +		for (i = 0; i < 512; i += 2) {
> +			t = p[i];
> +			p[i] = p[i+1];
> +			p[i+1] = t;
> +		}
> +	}
> +#endif

This looks the wrong place to fix this problem Geert. The PPC 
folks have the same issues with byte order on busses but you
won't see ifdefs in the core IDE code for it.

Fix your __ide_mm_insw/ide_mm_outsw macros and the rest happens
automatically.

Linus, until better justification of why this has to be here
can you not apply this change.

Alan

Re: [PATCH] M68k IDE updates

[Paul Mackerras]

Alan Cox writes:

> This looks the wrong place to fix this problem Geert. The PPC 
> folks have the same issues with byte order on busses but you
> won't see ifdefs in the core IDE code for it.
> 
> Fix your __ide_mm_insw/ide_mm_outsw macros and the rest happens
> automatically.

As I understand it, on some platforms (including some PPC platforms,
but not powermacs) one needs to byteswap drive ID data but not the
normal sector data.  Or vice versa.  Whether drive ID data needs
byte-swapping comes down to how the drive is attached to the bus.  The
conventions used by other systems that we need to interoperate with
(e.g. other OSes, or just older kernels) determine whether normal
sector data needs byte-swapping or not.

Since __ide_mm_insw doesn't get told whether it is transferring normal
sector data or drive ID data, it can't necessarily do the right thing
in both situations.

It's very possible that there are some PPC platforms for which IDE is
borken right now - I strongly suspect this would be the case for the
Tivo at least, and probably several other embedded PPC platforms.

Paul.

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Mon, 14 Apr 2003, Paul Mackerras wrote:
> Alan Cox writes:
> > This looks the wrong place to fix this problem Geert. The PPC 
> > folks have the same issues with byte order on busses but you
> > won't see ifdefs in the core IDE code for it.
> > 
> > Fix your __ide_mm_insw/ide_mm_outsw macros and the rest happens
> > automatically.
> 
> As I understand it, on some platforms (including some PPC platforms,
> but not powermacs) one needs to byteswap drive ID data but not the
> normal sector data.  Or vice versa.  Whether drive ID data needs
> byte-swapping comes down to how the drive is attached to the bus.  The
> conventions used by other systems that we need to interoperate with
> (e.g. other OSes, or just older kernels) determine whether normal
> sector data needs byte-swapping or not.
> 
> Since __ide_mm_insw doesn't get told whether it is transferring normal
> sector data or drive ID data, it can't necessarily do the right thing
> in both situations.

Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
on-disk data to be that way, for compatibility with e.g. TOS.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Benjamin Herrenschmidt]

On Mon, 2003-04-14 at 10:39, Geert Uytterhoeven wrote:

> Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> on-disk data to be that way, for compatibility with e.g. TOS.

Some designers need to be shot...

What about optionally making fix_drive_id a platoform hook
(like it was, but with a reasonable default) to avoid clobbering
the common code with those #ifdefs ?

Ben.

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On 14 Apr 2003, Benjamin Herrenschmidt wrote:
> On Mon, 2003-04-14 at 10:39, Geert Uytterhoeven wrote:
> 
> > Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> > on-disk data to be that way, for compatibility with e.g. TOS.
> 
> Some designers need to be shot...
> 
> What about optionally making fix_drive_id a platoform hook
> (like it was, but with a reasonable default) to avoid clobbering
> the common code with those #ifdefs ?

Yes, I already suggested that in my IDE patch for 2.4.x. But I was in a hurry,
since I wanted to get m68k IDE working in 2.4.21.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-14 at 10:24, Geert Uytterhoeven wrote:
> > What about optionally making fix_drive_id a platoform hook
> > (like it was, but with a reasonable default) to avoid clobbering
> > the common code with those #ifdefs ?
> 
> Yes, I already suggested that in my IDE patch for 2.4.x. But I was in a hurry,
> since I wanted to get m68k IDE working in 2.4.21.

The base kernel is not an appropriate place for hurrying. Lets fix this properly
even if it takes a bit of time

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-14 at 09:39, Geert Uytterhoeven wrote:
> Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> on-disk data to be that way, for compatibility with e.g. TOS.

That is a higher level problem. You need a byteswap mode for the loop device
it seems. Then you can read atari disks on any box..

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On 14 Apr 2003, Alan Cox wrote:
> On Llu, 2003-04-14 at 09:39, Geert Uytterhoeven wrote:
> > Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> > on-disk data to be that way, for compatibility with e.g. TOS.
> 
> That is a higher level problem. You need a byteswap mode for the loop device
> it seems. Then you can read atari disks on any box..

You're talking about a different problem.

We want to read Atari disks on Ataris, too.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-14 at 14:44, Geert Uytterhoeven wrote:
> On 14 Apr 2003, Alan Cox wrote:
> > On Llu, 2003-04-14 at 09:39, Geert Uytterhoeven wrote:
> > > Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> > > on-disk data to be that way, for compatibility with e.g. TOS.
> > 
> > That is a higher level problem. You need a byteswap mode for the loop device
> > it seems. Then you can read atari disks on any box..
> 
> You're talking about a different problem.
> 
> We want to read Atari disks on Ataris, too.

You can initrd a loop over ide root on the Atari. I can see reasons (performance)
you don't want to do that, and I'm quite happy for someone to split the mmio ops
for data/control to clean that up.

Re: [PATCH] M68k IDE updates

[Jamie Lokier]

Geert Uytterhoeven wrote:
> > Since __ide_mm_insw doesn't get told whether it is transferring normal
> > sector data or drive ID data, it can't necessarily do the right thing
> > in both situations.
> 
> Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> on-disk data to be that way, for compatibility with e.g. TOS.

Isn't that best solved in the TOS filesystem code?

That way, Ataris running Linux can read ext2 disks from other systems
properly, and other systems can read TOS disks written by Ataris
properly.

Or did I miss something?

-- Jamie

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Tue, 15 Apr 2003, Jamie Lokier wrote:
> Geert Uytterhoeven wrote:
> > > Since __ide_mm_insw doesn't get told whether it is transferring normal
> > > sector data or drive ID data, it can't necessarily do the right thing
> > > in both situations.
> > 
> > Indeed. Ataris and Q40/Q60s have byteswapped IDE busses, but they expect
> > on-disk data to be that way, for compatibility with e.g. TOS.
> 
> Isn't that best solved in the TOS filesystem code?

There's also a partition table to read. BTW, Atari uses MS-DOS style
partitioning.

> That way, Ataris running Linux can read ext2 disks from other systems
> properly, and other systems can read TOS disks written by Ataris
> properly.

That's why there's also an option to swap all diskdata at the IDE level, so you
can take your Atari disks to a PC and vice versa.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Jörn Engel]

On Tue, 15 April 2003 10:11:37 +0200, Geert Uytterhoeven wrote:
> 
> BTW, Atari uses MS-DOS style partitioning.

Interesting. Then how do you explain this (2.5.67)
config MSDOS_PARTITION
	bool "PC BIOS (MSDOS partition tables) support" if PARTITION_ADVANCED
	default y if !PARTITION_ADVANCED && !AMIGA && !ATARI && !MAC && !SGI_IP22 && !ARM && !SGI_IP27

or this (2.4.20)
   if [ "$CONFIG_AMIGA" != "y" -a "$CONFIG_ATARI" != "y" -a \
        "$CONFIG_MAC" != "y" -a "$CONFIG_SGI_IP22" != "y" -a \
        "$CONFIG_SGI_IP27" != "y" ]; then
      define_bool CONFIG_MSDOS_PARTITION y
   fi

In both cases, CONFIG_MSDOS_PARTITION is always y, *except* for Atari
and some others. According to your comment above, that should be
changed, shouldn't it?

Jörn

-- 
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Tue, 15 Apr 2003, [iso-8859-1] Jörn Engel wrote:
> On Tue, 15 April 2003 10:11:37 +0200, Geert Uytterhoeven wrote:
> > BTW, Atari uses MS-DOS style partitioning.
> 
> Interesting. Then how do you explain this (2.5.67)
> config MSDOS_PARTITION
> 	bool "PC BIOS (MSDOS partition tables) support" if PARTITION_ADVANCED
> 	default y if !PARTITION_ADVANCED && !AMIGA && !ATARI && !MAC && !SGI_IP22 && !ARM && !SGI_IP27
> 
> or this (2.4.20)
>    if [ "$CONFIG_AMIGA" != "y" -a "$CONFIG_ATARI" != "y" -a \
>         "$CONFIG_MAC" != "y" -a "$CONFIG_SGI_IP22" != "y" -a \
>         "$CONFIG_SGI_IP27" != "y" ]; then
>       define_bool CONFIG_MSDOS_PARTITION y
>    fi
> 
> In both cases, CONFIG_MSDOS_PARTITION is always y, *except* for Atari
> and some others. According to your comment above, that should be
> changed, shouldn't it?

Bummer, better check the sources first (I never had an Atari). Atari uses its
own partitioning scheme.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-14 at 00:43, Paul Mackerras wrote:
> Since __ide_mm_insw doesn't get told whether it is transferring normal
> sector data or drive ID data, it can't necessarily do the right thing
> in both situations.

That may be the right change to make.

> It's very possible that there are some PPC platforms for which IDE is
> borken right now - I strongly suspect this would be the case for the
> Tivo at least, and probably several other embedded PPC platforms.

For the older tivo that would be sad, for the newer tivo where they
digitally sign the binary and don't provide the signatures -- too bad
they are violating the license clarifications on the IDE code if they
use it.

Alan

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-14 at 00:43, Paul Mackerras wrote:
> Since __ide_mm_insw doesn't get told whether it is transferring normal
> sector data or drive ID data, it can't necessarily do the right thing
> in both situations.

That may be the right change to make.

> It's very possible that there are some PPC platforms for which IDE is
> borken right now - I strongly suspect this would be the case for the
> Tivo at least, and probably several other embedded PPC platforms.

For the older tivo that would be sad, for the newer tivo where they
digitally sign the binary and don't provide the signatures -- too bad
they are violating the license clarifications on the IDE code if they
use it.

Alan

Re: [PATCH] M68k IDE updates

[Linus Torvalds]

On Mon, 14 Apr 2003, Paul Mackerras wrote:
> 
> Since __ide_mm_insw doesn't get told whether it is transferring normal
> sector data or drive ID data, it can't necessarily do the right thing
> in both situations.

Can we please then just separate the two functions out into "fetch sector
data" and "fetch drive ID"? And NOT playing with another frigging broken
passed-down flag that people get wrong and isn't obvious what it does
anyway? It's a lot easier to do

	/* On sane architectures, data and ID are accessed the same */
	#define ide_fetch_sector_data(...) __ide_fetch_data(..)
	#define ide_fetch_id_data(...) __ide_fetch_data(..)

than it is to carry a flag around and having to remember to get it right 
in every place this is used.

It's more efficient too, but the _clarity_ and lack of dynamic flags is a 
hell of a lot more important.

And stupid architectures that may have to re-implement (and possible
duplicate) the ID fetch code only have themselves to blame. Although it 
might easily be as simple as

	/*
	 * The PCI bus is wired up the wrong way, we need to byteswap
	 * the ID results after they come back
	 */
	static inline xxx ide_fetch_id_data(...)
	{
		__ide_fetch_data(..)
		bswap_id_data(..)
	}

and please keep this in some m68k-specific file instead of forcing 
_everybody_ to know about the braindamage.

		Linus

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Mon, 14 Apr 2003, Linus Torvalds wrote:
> On Mon, 14 Apr 2003, Paul Mackerras wrote:
> > Since __ide_mm_insw doesn't get told whether it is transferring normal
> > sector data or drive ID data, it can't necessarily do the right thing
> > in both situations.
> 
> Can we please then just separate the two functions out into "fetch sector
> data" and "fetch drive ID"? And NOT playing with another frigging broken
> passed-down flag that people get wrong and isn't obvious what it does
> anyway? It's a lot easier to do
> 
> 	/* On sane architectures, data and ID are accessed the same */
> 	#define ide_fetch_sector_data(...) __ide_fetch_data(..)
> 	#define ide_fetch_id_data(...) __ide_fetch_data(..)
> 
> than it is to carry a flag around and having to remember to get it right 
> in every place this is used.
> 
> It's more efficient too, but the _clarity_ and lack of dynamic flags is a 
> hell of a lot more important.
> 
> And stupid architectures that may have to re-implement (and possible
> duplicate) the ID fetch code only have themselves to blame. Although it 
> might easily be as simple as
> 
> 	/*
> 	 * The PCI bus is wired up the wrong way, we need to byteswap
> 	 * the ID results after they come back
> 	 */
> 	static inline xxx ide_fetch_id_data(...)
> 	{
> 		__ide_fetch_data(..)
> 		bswap_id_data(..)
> 	}
> 
> and please keep this in some m68k-specific file instead of forcing 
> _everybody_ to know about the braindamage.

I think the least-intrusive solution is something like this:

--- linux-2.5/drivers/ide/ide-iops.c.orig	Mon Apr 14 21:43:30 2003
+++ linux-2.5/drivers/ide/ide-iops.c	Mon Apr 14 21:44:53 2003
@@ -423,8 +423,7 @@
  */
 void ide_fix_driveid (struct hd_driveid *id)
 {
-#ifndef __LITTLE_ENDIAN
-# ifdef __BIG_ENDIAN
+    if (ide_driveid_needs_swapping(id)) {
 	int i;
 	u16 *stringcast;
 
@@ -512,10 +511,7 @@
 	for (i = 0; i < 48; i++)
 		id->words206_254[i] = __le16_to_cpu(id->words206_254[i]);
 	id->integrity_word  = __le16_to_cpu(id->integrity_word);
-# else
-#  error "Please fix <asm/byteorder.h>"
-# endif
-#endif
+    }
 }
 
 EXPORT_SYMBOL(ide_fix_driveid);

Where ide_driveid_needs_swapping() is #define'd to return 0 (never swap), 1
(always swap), or whatever architecture-specific logic you need.

We can even have defaults in <linux/ide.h>

    #ifndef ide_driveid_needs_swapping
    # ifdef __LITTLE_ENDIAN
    #  define ide_driveid_needs_swapping(id)	0
    # else
    #  ifdef __BIG_ENDIAN
    #   define ide_driveid_needs_swapping(id)	1
    #  else
    #   error "Please fix <asm/byteorder.h>"
    #  endif
    # endif
    #endif

Sounds sufficiently sane?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Paul Mackerras]

Geert Uytterhoeven writes:

> I think the least-intrusive solution is something like this:
> 
> --- linux-2.5/drivers/ide/ide-iops.c.orig	Mon Apr 14 21:43:30 2003
> +++ linux-2.5/drivers/ide/ide-iops.c	Mon Apr 14 21:44:53 2003
> @@ -423,8 +423,7 @@
>   */
>  void ide_fix_driveid (struct hd_driveid *id)
>  {
> -#ifndef __LITTLE_ENDIAN
> -# ifdef __BIG_ENDIAN
> +    if (ide_driveid_needs_swapping(id)) {

I really think that whether the driveid needs swapping should be
regarded as a property of the interface, not of the system as a whole.

I like the idea of adding a "read in driveid" function pointer to the
ide_hwif_t structure.  Most systems would set that to the same as the
INSW function pointer.  For those systems where the hardware designer
suffered a momentary dizzy spell we can set it to point to a function
that does the necessary byte-swapping.

Paul.

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Tue, 15 Apr 2003, Paul Mackerras wrote:
> Geert Uytterhoeven writes:
> > I think the least-intrusive solution is something like this:
> > 
> > --- linux-2.5/drivers/ide/ide-iops.c.orig	Mon Apr 14 21:43:30 2003
> > +++ linux-2.5/drivers/ide/ide-iops.c	Mon Apr 14 21:44:53 2003
> > @@ -423,8 +423,7 @@
> >   */
> >  void ide_fix_driveid (struct hd_driveid *id)
> >  {
> > -#ifndef __LITTLE_ENDIAN
> > -# ifdef __BIG_ENDIAN
> > +    if (ide_driveid_needs_swapping(id)) {
> 
> I really think that whether the driveid needs swapping should be
> regarded as a property of the interface, not of the system as a whole.
> 
> I like the idea of adding a "read in driveid" function pointer to the
> ide_hwif_t structure.  Most systems would set that to the same as the
> INSW function pointer.  For those systems where the hardware designer
> suffered a momentary dizzy spell we can set it to point to a function
> that does the necessary byte-swapping.

That sounds OK to me.

But I'd like to have the actual swapping code in a common source or header
file, else we fall back to the old approach, where all platforms that needed it
implemented there own driveid swapping code, which had to be kept in sync when
more reserved fields in the driveid got an actual meaning.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Mon, 14 Apr 2003, Linus Torvalds wrote:
> On Mon, 14 Apr 2003, Paul Mackerras wrote:
> > Since __ide_mm_insw doesn't get told whether it is transferring normal
> > sector data or drive ID data, it can't necessarily do the right thing
> > in both situations.
> 
> Can we please then just separate the two functions out into "fetch sector
> data" and "fetch drive ID"? And NOT playing with another frigging broken
> passed-down flag that people get wrong and isn't obvious what it does
> anyway? It's a lot easier to do
> 
> 	/* On sane architectures, data and ID are accessed the same */
> 	#define ide_fetch_sector_data(...) __ide_fetch_data(..)
> 	#define ide_fetch_id_data(...) __ide_fetch_data(..)
> 
> than it is to carry a flag around and having to remember to get it right 
> in every place this is used.

OK, I took a closer look at the IDE identification innards.

It's quite easy to call hwif->ata_input_id() instead of hwif->ata_input_data()
for reading the drive ID. Then hwif->ata_input_id() can take care of the
byteswapping for hardwired byteswapped IDE busses.

However, there's also a routine that involves more magic:
taskfile_lib_get_identify(). While trying to understand that one, I found more
commands that should call the (possible byteswapping) hwif->ata_input_id()
operations, like SMART commands. So first we need a clearer differentiation
between commands that transfer on-platter data, or other drive data.

Any comments from the IDE experts?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] M68k IDE updates

[Alan Cox]

On Llu, 2003-04-21 at 17:55, Geert Uytterhoeven wrote:
> However, there's also a routine that involves more magic:
> taskfile_lib_get_identify(). While trying to understand that one, I found more
> commands that should call the (possible byteswapping) hwif->ata_input_id()
> operations, like SMART commands. So first we need a clearer differentiation
> between commands that transfer on-platter data, or other drive data.
> 
> Any comments from the IDE experts?

Only one, stop abusing the IDE layer and do your byte swapping via a loopback/md 
or similar piece of code.