Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? Yes. For other buses, use foo_writel(), etc. Can this please be documented then? Never heard this before... You have come late to the party. WHat do you mean here? Could you please explain? This has been the case for many, many years. No, it was never documented AFAICS. And there is no point in a massive rename to pci_writel(), either. That would be really inconvenient, sure. It's also inconvenient that all the nice short names are PCI-only. Segher - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Segher Boessenkool wrote: Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? Yes. For other buses, use foo_writel(), etc. Can this please be documented then? Never heard this before... You have come late to the party. WHat do you mean here? Could you please explain? This has been the case for many, many years. No, it was never documented AFAICS. A de facto standard does not need to be documented, to be a de facto standard. A lot of Linux standards are often based on emails from Linus buried halfway down a thread. A decision gets made, and people follow. And there is no point in a massive rename to pci_writel(), either. That would be really inconvenient, sure. It's also inconvenient that all the nice short names are PCI-only. Only to you, a decided minority of developers. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote: On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and outsl, so remove the conditional use of insl_ns and outsl_ns. The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would byte-swap on big-endian archs such as powerpc.) diff --git a/drivers/net/3c509.c b/drivers/net/3c509.c index cbdae54..add6381 100644 --- a/drivers/net/3c509.c +++ b/drivers/net/3c509.c @@ -879,11 +879,7 @@ #endif outw(skb-len, ioaddr + TX_FIFO); outw(0x00, ioaddr + TX_FIFO); /* ... and the packet rounded to a doubleword. */ -#ifdef __powerpc__ - outsl_ns(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#else outsl(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#endif Dohh, a hack like this to work around some possbile byte-swapping bug should never have been done in the first place :-( However, I presume someone added the __powerpc__ define here because they picked up a 3c509 at a garage sale, stuck it in a powerpc, found out it didn't work due to a byte-swapping bug, and then patched it as above. I'm disturbed that somehow outsl_ns() became identical to outsl() at some point, presumably breaking this patch. --linas - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, Sep 19, 2006 at 08:52:18PM +0200, Matt Sealey wrote: [...] Linas Vepstas wrote: On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote: On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and outsl, so remove the conditional use of insl_ns and outsl_ns. The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would Never mind. Silly me don't know my history. asm-powerpc/io.h clearly states * The *_ns versions below don't do byte-swapping. * Neither do the standard versions now, these are just here * for older code. Seems that the byteswapping machanism was changed a while ago, and no longer handled in this way any more. --linas - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
However, I presume someone added the __powerpc__ define here because they picked up a 3c509 at a garage sale, stuck it in a powerpc, found out it didn't work due to a byte-swapping bug, and then patched it as above. I'm disturbed that somehow outsl_ns() became identical to outsl() at some point, presumably breaking this patch. The problem is that somebody had the bright idea to implement ppc outsl as byteswapping a lng time ago. This was totally bogus and got fixed, but it looks like this driver holds a remain of that period. Ben. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
On Tue, 2006-09-19 at 20:52 +0200, Matt Sealey wrote:
Some northbridges and PCI bridges have clever byteswapping in
hardware, maybe this is just an effect of that. In theory depending on
the host bridge, you should pass in big endian data and have it swap or
not swap, not pick that way in the driver, UNLESS your driver expects
bigendian data, in which case on a bigendian platform you can tell it to
write without swapping. Voila, two functions.
It's generally considered pretty bad for a northbridge to try to muck
around with byte order. There are fairly well defined rules to plug a
little endian bus (PCI, ISA, ...) on a big endian machine.
The trick that some people didn't get a while ago is that while
accessors like inw/inl shall return a byteswapped data, string
operations like in insw/insl who are copying from a fifo basically to
memory (and opposite write versions) shall -not- byteswap since the data
isn't interpreted. it's a byte stream. It doesn't have any endian
semantic associated to it until it's actually read back from memory in
which case the appropriate endian swap (if any) has to be used depending
on the endianness and size of a given field read/written.
Since some people didn't get it, in the early days, some BE
architectures like ppc had versions of insw/insl that did byteswap,
which was wrong. The bits in this driver are remains from that era.
Note that to aggravate the problem, it still happens that HW engineers
try to be smart when hooking a 16 bits or 32 bits FIFO to a BE machine
and byteswap it in hardware. This is of course totally bogus but did
happen with IDE controllers typically (I think atari or amiga has one of
these, the Tivo is like that too, and a bunch of embedded things). The
net result is that you have to pump the data fifo using a byteswapping
accessor and you cannot use DMA unless you DMA controller can re-swap
the other way around But lots of HW people still don't get it :)
However the existance of these PCI bridges these days? I haven't seen
one in years, and when I have nobody has ever enabled the magic swappy
thing as it's unreliable and can't always tell how you present the data.
One wishes that there was a ntoh and hton style macro in standard use
for PCI access.. hang on though that jsut wouldn't work would it.
Nah. We have the basic rule that readl/writel are little endian. PowerPC
additionally provides arch specific low level in_{be,le}32 type
accessors with explicit endianness. Or you can also use
cpu_to_le32/le32_to_cpu kind of macros to convert between native and
explicit endianness.
Ben.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
Linas Vepstas writes: The rest of this patch might indeed be correct, but the above comment bothers me. The ns versions of routines are supposed to be non-byte-swapped versions of the insl/outsl routines (which would byte-swap on big-endian archs such as powerpc.) If it were true that in/outsw and in/outsl were actually used to transfer arrays of 16-bit data items or 32-bit data items to/from an I/O device, I would agree with you, but they aren't. They are universally used to transfer arrays of bytes, with the optimization of doing so 2 or 4 bytes at a time. That is why in/outsw and in/outsl don't (and shouldn't) do byte swapping. Paul. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Nah. We have the basic rule that readl/writel are little endian.
PowerPC
additionally provides arch specific low level in_{be,le}32 type
accessors with explicit endianness. Or you can also use
cpu_to_le32/le32_to_cpu kind of macros to convert between native and
explicit endianness.
Sure, PCI busses are little-endian. But is readX()/writeX() for PCI
only? I sure hope not.
It would make a lot more sense if readX()/writeX() used the endianness
of the bus they are performed on. PowerPC byteswaps are cheap -- for
16- and 32-bit accesses. They're quite bad for 64-bit though; it would
be a pity to end up doing two of those for a 64-bit big-endian I/O
access
(one on the access itself, one to convert the data back to CPU order).
This would happily solve the problem of the various variations of
byte-swapping bus bridges, too (natural swap, 32-bit swap, 64-bit
swap,
perhaps others that I thankfully have never seen or cannot remember).
Now you can say, use readl_be() or something similar, but that's a)
ugly,
b) error-prone, c) exponential interface explosion, d) ugly.
Segher
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
On Wed, 2006-09-20 at 02:21 +0200, Segher Boessenkool wrote:
Nah. We have the basic rule that readl/writel are little endian.
PowerPC
additionally provides arch specific low level in_{be,le}32 type
accessors with explicit endianness. Or you can also use
cpu_to_le32/le32_to_cpu kind of macros to convert between native and
explicit endianness.
Sure, PCI busses are little-endian. But is readX()/writeX() for PCI
only? I sure hope not.
It's defined for PCI and possibly ISA memory. You can use it for other
things if you whish to, but other things are arch specific in any
case.
It would make a lot more sense if readX()/writeX() used the endianness
of the bus they are performed on.
No way ! Again, it's evil if such a simple thing start doing different
things depending on random external factors.
Different bus - different accessor.
We defined on PowerPC that readl was fine for anything that comes out of
ioremap and is little endian, but that's also why you have the explicit
{in,out}_{le,be}{16,32}. That's what you should use in fact with non-PCI
busses unless you know you are LE.
PowerPC byteswaps are cheap -- for 16- and 32-bit accesses. They're quite
bad for 64-bit though; it would
be a pity to end up doing two of those for a 64-bit big-endian I/O
access
(one on the access itself, one to convert the data back to CPU order).
This would happily solve the problem of the various variations of
byte-swapping bus bridges, too (natural swap, 32-bit swap, 64-bit
swap,
perhaps others that I thankfully have never seen or cannot remember).
Now you can say, use readl_be() or something similar, but that's a)
ugly,
b) error-prone, c) exponential interface explosion, d) ugly.
I'd rather has an interface explosion than having black endian magic
happening inside of the accessors.
Ben.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Segher Boessenkool wrote: Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? Yes. For other buses, use foo_writel(), etc. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? I sure hope not. It's defined for PCI and possibly ISA memory. You can use it for other things if you whish to, but other things are arch specific in any case. Huh? You're saying that only PCI and ISA are standardised busses? It would make a lot more sense if readX()/writeX() used the endianness of the bus they are performed on. No way ! Again, it's evil if such a simple thing start doing different things depending on random external factors. That's your opinion, yes. I'm saying it's *not* doing different things: in both cases it just does the correct-endian access. Also it doesn't depend on random external factors -- they're not random factors, and not external either: it only depends on the bus the access is done on. Different bus - different accessor. Then please rename readX()/writeX() to pci_readX()/pci_writeX(). Now you can say, use readl_be() or something similar, but that's a) ugly, b) error-prone, c) exponential interface explosion, d) ugly. I'd rather has an interface explosion than having black endian magic happening inside of the accessors. Any comments on a), b) and d) as well? Segher - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? Yes. For other buses, use foo_writel(), etc. Can this please be documented then? Never heard this before... Segher - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Hi, I am alarmed and embarassed that sloppy comments on my part has turned onto a long conversation. On Wed, Sep 20, 2006 at 02:58:39AM +0200, Segher Boessenkool wrote: Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? I sure hope not. It's defined for PCI and possibly ISA memory. You can use it for other things if you whish to, but other things are arch specific in any case. Huh? You're saying that only PCI and ISA are standardised busses? Well, I'm having trouble thinking of other busses that have as strong a sense of the address-data style I/O as PCI. Busses like scsi and ide are primarily command-data or data-data in style. Only the address-data style busses need readl/writel-style routines. I can't prove, but suspect that the adress-data style of access is why PCI is wired up close to the CPU. What other bsses are there that are direct-attached to the CPU? I can't think of much ... The sbus on sparc ... hypertransport from AMD ... but hypertransport is more or less invisible to the kernel. ... some recent attempts to supplant the system bus with infiniband, but I get the impression that this will be strangely engineered, and semi-invisible to the kernel as well. The actual infiniband protocols are ipv6-like+rdma and so fall into a data-data programming style. Different bus - different accessor. Then please rename readX()/writeX() to pci_readX()/pci_writeX(). Well, I don't get the impression that there will be othre busses for which this is an issue the way it is on pci. --linas - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Segher Boessenkool wrote: Sure, PCI busses are little-endian. But is readX()/writeX() for PCI only? Yes. For other buses, use foo_writel(), etc. Can this please be documented then? Never heard this before... You have come late to the party. This has been the case for many, many years. And there is no point in a massive rename to pci_writel(), either. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove powerpc specific parts of 3c509 driver
Well, I'm having trouble thinking of other busses that have as strong a sense of the address-data style I/O as PCI. Busses like scsi and ide are primarily command-data or data-data in style. Only the address-data style busses need readl/writel-style routines. SBUS, JBUS, VMEbus, NuBus, RapidIO, eBus, various SoC busses, to name a few. There are many, and most are big-endian. Segher - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Remove powerpc specific parts of 3c509 driver
On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and outsl, so remove the conditional use of insl_ns and outsl_ns. Signed-off-by: Stephen Rothwell [EMAIL PROTECTED] --- drivers/net/3c509.c |9 - 1 files changed, 0 insertions(+), 9 deletions(-) This is in anticipation of removing the insl_ns and outsl_ns definitions which are powerpc sepcific patches. -- Cheers, Stephen Rothwell[EMAIL PROTECTED] diff --git a/drivers/net/3c509.c b/drivers/net/3c509.c index cbdae54..add6381 100644 --- a/drivers/net/3c509.c +++ b/drivers/net/3c509.c @@ -879,11 +879,7 @@ #endif outw(skb-len, ioaddr + TX_FIFO); outw(0x00, ioaddr + TX_FIFO); /* ... and the packet rounded to a doubleword. */ -#ifdef __powerpc__ - outsl_ns(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#else outsl(ioaddr + TX_FIFO, skb-data, (skb-len + 3) 2); -#endif dev-trans_start = jiffies; if (inw(ioaddr + TX_FREE) 1536) @@ -1103,13 +1099,8 @@ el3_rx(struct net_device *dev) skb_reserve(skb, 2); /* Align IP on 16 byte */ /* 'skb-data' points to the start of sk_buff data area. */ -#ifdef __powerpc__ - insl_ns(ioaddr+RX_FIFO, skb_put(skb,pkt_len), - (pkt_len + 3) 2); -#else insl(ioaddr + RX_FIFO, skb_put(skb,pkt_len), (pkt_len + 3) 2); -#endif outw(RxDiscard, ioaddr + EL3_CMD); /* Pop top Rx packet. */ skb-protocol = eth_type_trans(skb,dev); -- 1.4.2.1 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
