Recently removed io accessors

Recently removed io accessors

[Peter Korsgaard]

Hi,

Commit 661f1cdb8b3e3c2c44e97df122c1d5643c054ce8 ([POWERPC] remove
unused asm routines) removed the _insl / _outsl routines for 32bit
copies with byteswap.

I've been working on adding ppc support to drivers/net/smc911x.c, and
I was unfortunately using those routines (The ethernet controller can
be configured for big endian mode, but the contents of the internal
packet buffers is still little endian, so you need to byteswap in the
tx/rx routines).

Any chance of getting them back or should I implement a (slower) loop
myself before submitting the patch?

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Benjamin Herrenschmidt]

On Thu, 2006-10-12 at 17:44 +0200, Peter Korsgaard wrote:
> Hi,
> 
> Commit 661f1cdb8b3e3c2c44e97df122c1d5643c054ce8 ([POWERPC] remove
> unused asm routines) removed the _insl / _outsl routines for 32bit
> copies with byteswap.
> 
> I've been working on adding ppc support to drivers/net/smc911x.c, and
> I was unfortunately using those routines (The ethernet controller can
> be configured for big endian mode, but the contents of the internal
> packet buffers is still little endian, so you need to byteswap in the
> tx/rx routines).
> 
> Any chance of getting them back or should I implement a (slower) loop
> myself before submitting the patch?

Well, a "packet buffer" should have no endian. When streaming in our out
a fifo, you basically stream bytes that happen to come out 2 at a time.
So unless somebody wired the hardware backward, you should do no
swapping when using the fifo.

Ben.

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "BH" == Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

Hi,

>> Any chance of getting them back or should I implement a (slower)
>> loop myself before submitting the patch?

BH> Well, a "packet buffer" should have no endian. When streaming in
BH> our out a fifo, you basically stream bytes that happen to come out
BH> 2 at a time.  So unless somebody wired the hardware backward, you
BH> should do no swapping when using the fifo.

I agree in principle, but the issue gets complicated by the fact that
the chip works in chunks of 32bit, but the 911{5..7} chips only have a
16bit interface to lower costs, and that the chip has a builtin endian
swap feature (which works for all direct registers, but apparently not
for the packet buffers).

The memory controller automatically translates a 32bit access to two
16 bit accesses.

You could be right that the hw people screwed something up, but that
doesn't help much once there's units in the field :/

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Benjamin Herrenschmidt]

On Fri, 2006-10-13 at 08:56 +0200, Peter Korsgaard wrote:
> >>>>> "BH" == Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> Hi,
> 
> >> Any chance of getting them back or should I implement a (slower)
> >> loop myself before submitting the patch?
> 
> BH> Well, a "packet buffer" should have no endian. When streaming in
> BH> our out a fifo, you basically stream bytes that happen to come out
> BH> 2 at a time.  So unless somebody wired the hardware backward, you
> BH> should do no swapping when using the fifo.
> 
> I agree in principle, but the issue gets complicated by the fact that
> the chip works in chunks of 32bit, but the 911{5..7} chips only have a
> 16bit interface to lower costs, and that the chip has a builtin endian
> swap feature (which works for all direct registers, but apparently not
> for the packet buffers).
> 
> The memory controller automatically translates a 32bit access to two
> 16 bit accesses.

Well, that is not necessarily broken :)

> You could be right that the hw people screwed something up, but that
> doesn't help much once there's units in the field :/

Well, I still don't see it. It all depends how the HW has been wired I
suppose but you should not need byteswap regardless of the 32 bits being
broken in packs of 2x16 bits or whatever... If the chip has a HW
byteswap for registers and not for the packet buffer, that makes it even
more clear that such swapping should not be necessary.

Unless the chip has been wired backward on the processor bus (in which
case, btw, DMA will not work either unless you have one of those
magically swapping dma controllers)..

So I still claim that you should not need them and if you do, then the
chip has probably been incorrect wired to your CPU bus. In which case,
you can either grab an old copy of the functions and put them in your
driver for your platform or add a cpu_to_leXX() loop to byteswap the
data in/out, but it's probably not the right thing to do in the generic
driver since it would be a problem specific to your board.

Unless I'm missing something ... It would be useful to have more details
about your setup.

Cheers.
Ben.

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "BH" == Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

Hi,

BH> Well, I still don't see it. It all depends how the HW has been
BH> wired I suppose but you should not need byteswap regardless of the
BH> 32 bits being broken in packs of 2x16 bits or whatever... If the
BH> chip has a HW byteswap for registers and not for the packet
BH> buffer, that makes it even more clear that such swapping should
BH> not be necessary.

Ok, a bit of details:

We're using the smsc9117
(http://www.smsc.com/main/datasheets/9117.pdf) connected over a 16bit
EMC bus (together with a spansion 29PL127 flash) to a Xilinx V4FX FPGA
with a 405 core in it.

LSB of the processor is connected to LSB of of the 9117 (and same for
the flash) and so on.

Reading the byte test register of the 9117 (supposed to contain
0x87654321) gives:

Endian register set to little endian (default at powerup):
RedBoot> x -b 0x8e000064 -2 -l 4
8E000064: 4321 8765
RedBoot> x -b 0x8e000064 -4 -l 4
8E000064: 43218765

And with it set to big endian:
RedBoot> x -b 0x8e000064 -2 -l 4
8E000064: 8765 4321
RedBoot> x -b 0x8e000064 -4 -l 4
8E000064: 87654321

So with this setup I need to enable the big endian mode to read
registers without byteswapping and use byteswapping for the packet
fifos.

If on the other hand the hw people had inverted the 2 byte lanes
(connected b0..b7 on the CPU to b8..b15, and b8..15 to b0..b7) I would
need to use byteswapping on the normal register accesses and no byte
swapping on the packet fifos.

They didn't unfortunately :/

BH> Unless the chip has been wired backward on the processor bus (in
BH> which case, btw, DMA will not work either unless you have one of
BH> those magically swapping dma controllers)..

True. We are not using DMA though.

BH> So I still claim that you should not need them and if you do, then
BH> the chip has probably been incorrect wired to your CPU bus. In
BH> which case, you can either grab an old copy of the functions and
BH> put them in your driver for your platform or add a cpu_to_leXX()
BH> loop to byteswap the data in/out, but it's probably not the right
BH> thing to do in the generic driver since it would be a problem
BH> specific to your board.

I could add another #ifdef CONFIG_<my board> to the smc911x.h to
select the right I/O accessors, but sticking the implementation of
_insl/_outsl in my platform file isn't that nice - Couldn't we put
them back in misc.S?

BH> Unless I'm missing something ... It would be useful to have more
BH> details about your setup.

I hope this makes it a bit more clear.

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Benjamin Herrenschmidt]

> Endian register set to little endian (default at powerup):
> RedBoot> x -b 0x8e000064 -2 -l 4
> 8E000064: 4321 8765
> RedBoot> x -b 0x8e000064 -4 -l 4
> 8E000064: 43218765
> 
> And with it set to big endian:
> RedBoot> x -b 0x8e000064 -2 -l 4
> 8E000064: 8765 4321
> RedBoot> x -b 0x8e000064 -4 -l 4
> 8E000064: 87654321
> 
> So with this setup I need to enable the big endian mode to read
> registers without byteswapping and use byteswapping for the packet
> fifos.

That's what bothers me... usually, if the registers are little endian
and you need a byteswap to read them, then you don't need a byteswap to
pump the fifo. For example, things like IDE, sound cards, wireless chips
etc... have LE registers (we use byteswap to access them) and we are
fine not swapping on the fifo read/writes.

I'll read the chip spec and try to figure out what can be done. Maybe an
option is to use the per-page little endian flag available on 4xx parts,
I think you may have that in your xilinx, and thus have automatic
byteswapping. We don't support that bit but it shouldn't be too hard to
add it so that you can pass it to __ioremap. But again, I'm surprised
that's necessary.

Ben.

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "BH" == Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

Hi,

BH> That's what bothers me... usually, if the registers are little
BH> endian and you need a byteswap to read them, then you don't need a
BH> byteswap to pump the fifo. For example, things like IDE, sound
BH> cards, wireless chips etc... have LE registers (we use byteswap to
BH> access them) and we are fine not swapping on the fifo read/writes.

Exactly. As I wrote in my previous mail: If the hw people had swapped
the 2 byte lanes I would need to byteswap on the normal register
accesses, but not on the FIFO. That would have been the preferable
setup.

Unfortunately they didn't so I need to enable big endian mode and NOT
swap on normal register access and swap on access to the FIFO.

The question is what to do about it. Adding another special case to
smc911x.h for my board is not a big deal, but I would like to get the
_insl / _outsl implementations back in misc.S instead of adding them
to my platform file.

BH> I'll read the chip spec and try to figure out what can be
BH> done. Maybe an option is to use the per-page little endian flag
BH> available on 4xx parts, I think you may have that in your xilinx,
BH> and thus have automatic byteswapping. We don't support that bit
BH> but it shouldn't be too hard to add it so that you can pass it to
BH> __ioremap. But again, I'm surprised that's necessary.

Sorry, but that sounds overkill to me. Everything works fine as long
as I don't byteswap on normal registers and use something like _insl /
_outsl for the FIFOs.

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Benjamin Herrenschmidt]

> Exactly. As I wrote in my previous mail: If the hw people had swapped
> the 2 byte lanes I would need to byteswap on the normal register
> accesses, but not on the FIFO. That would have been the preferable
> setup.
> 
> Unfortunately they didn't so I need to enable big endian mode and NOT
> swap on normal register access and swap on access to the FIFO.

That's where it bothers me... you need to enable BE register mode...

On normal HW, you don't do that... you get LE registers and everybody is
happy with that and the fifo just works. Unless what you really do is
enable BE register mode (and it looks like BE) or something around those
lines.

Have you actually verified that it works if you endian swap the whole
buffer ? Maybe it's just some headers added by the chip that need
swapping in which case it's all fine, just add the proper leXX_to_cpu()
to the driver when reading those...

> The question is what to do about it. Adding another special case to
> smc911x.h for my board is not a big deal, but I would like to get the
> _insl / _outsl implementations back in misc.S instead of adding them
> to my platform file.

Maybe with a different name but that's possible. I just wnat to be 150%
sure that this is what you need and the problem isn't lurking somewhere
else ;)

> BH> I'll read the chip spec and try to figure out what can be
> BH> done. Maybe an option is to use the per-page little endian flag
> BH> available on 4xx parts, I think you may have that in your xilinx,
> BH> and thus have automatic byteswapping. We don't support that bit
> BH> but it shouldn't be too hard to add it so that you can pass it to
> BH> __ioremap. But again, I'm surprised that's necessary.
> 
> Sorry, but that sounds overkill to me. Everything works fine as long
> as I don't byteswap on normal registers and use something like _insl /
> _outsl for the FIFOs.

Overkill ? well, not that much.. you get free byteswap from the hardware
with just a bit set in the PTEs :) Just a matter of making sure the TLB
miss handlers does proerly forward that bit from the linux PTE to the
TLB.

Ben.

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "BH" == Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

Hi,

>> Unfortunately they didn't so I need to enable big endian mode and
>> NOT swap on normal register access and swap on access to the FIFO.

BH> That's where it bothers me... you need to enable BE register
BH> mode...

Yes, because the hw people didn't swap the byte lanes. To go back to
my previous mail:

Endian register set to little endian (default at powerup):
RedBoot> x -b 0x8e000064 -2 -l 4
8E000064: 4321 8765
RedBoot> x -b 0x8e000064 -4 -l 4
8E000064: 43218765

With the bytelanes swapped the 32bit read would return 21436587 which
is simply the little endian form of what we need.

But because they didn't swap the lanes I need to enable the BE mode
and not do bswap.

BH> On normal HW, you don't do that... you get LE registers and
BH> everybody is happy with that and the fifo just works. Unless what
BH> you really do is enable BE register mode (and it looks like BE) or
BH> something around those lines.

Sorry, I cannot parse that. I AM enabling BE mode.

Again:
- Bytelanes directly connected (as on our boards): You have to enable BE
  mode in the chip, NOT swap normal register accesses and SWAP accesses
  to/from the FIFOs.

- Bytelanes swapped: You should NOT enable BE mode, SWAP normal
  register accesses and NOT swap accesses from/to the FIFOs.

BH> Have you actually verified that it works if you endian swap the
BH> whole buffer ? Maybe it's just some headers added by the chip that
BH> need swapping in which case it's all fine, just add the proper
BH> leXX_to_cpu() to the driver when reading those...

Yes, we've be using it since January (I worked together with Dustin
before smc911x.c got submitted to mainline) - We need to swap all data
to/from the FIFOs.

>> The question is what to do about it. Adding another special case to
>> smc911x.h for my board is not a big deal, but I would like to get
>> the _insl / _outsl implementations back in misc.S instead of adding
>> them to my platform file.

BH> Maybe with a different name but that's possible. I just wnat to be
BH> 150% sure that this is what you need and the problem isn't lurking
BH> somewhere else ;)

Not as far as I can see.

BH> Overkill ? well, not that much.. you get free byteswap from the
BH> hardware with just a bit set in the PTEs :) Just a matter of
BH> making sure the TLB miss handlers does proerly forward that bit
BH> from the linux PTE to the TLB.

It's still a lot more work than simply doing a s/_outsl_ns/_outsl/.
Isn't the stwbrx as fast as a regular stw?

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Paul Mackerras]

Peter Korsgaard writes:

> We're using the smsc9117
> (http://www.smsc.com/main/datasheets/9117.pdf) connected over a 16bit
> EMC bus (together with a spansion 29PL127 flash) to a Xilinx V4FX FPGA
> with a 405 core in it.
> 
> LSB of the processor is connected to LSB of of the 9117 (and same for
> the flash) and so on.
> 
> Reading the byte test register of the 9117 (supposed to contain
> 0x87654321) gives:
> 
> Endian register set to little endian (default at powerup):
> RedBoot> x -b 0x8e000064 -2 -l 4
> 8E000064: 4321 8765
> RedBoot> x -b 0x8e000064 -4 -l 4
> 8E000064: 43218765

Which is neither big-endian nor little-endian, but something more like
vax-endian (or pdp11-endian), but not exactly that either - vax-endian
would be 65872143.  What a mess.

> And with it set to big endian:
> RedBoot> x -b 0x8e000064 -2 -l 4
> 8E000064: 8765 4321
> RedBoot> x -b 0x8e000064 -4 -l 4
> 8E000064: 87654321

Clearly the "big endian" bit on the chip doesn't actually switch
between little-endian and big-endian at all, but just acts to invert
the 0x2 bit of the address...

So the two hardware bogosities taken together leave you with only one
combination that is anything like usable.  I suggest that you code up
a loop of readw (or equivalent) in the driver with a big comment
explaining why you need to do it that way.

Paul.

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "PM" == Paul Mackerras <paulus@samba.org> writes:

Hi,

>> Endian register set to little endian (default at powerup):
RedBoot> x -b 0x8e000064 -2 -l 4
>> 8E000064: 4321 8765
RedBoot> x -b 0x8e000064 -4 -l 4
>> 8E000064: 43218765

PM> Which is neither big-endian nor little-endian, but something more
PM> like vax-endian (or pdp11-endian), but not exactly that either -
PM> vax-endian would be 65872143.  What a mess.

Yes :/

PM> Clearly the "big endian" bit on the chip doesn't actually switch
PM> between little-endian and big-endian at all, but just acts to
PM> invert the 0x2 bit of the address...

Exactly.

PM> So the two hardware bogosities taken together leave you with only
PM> one combination that is anything like usable.  I suggest that you
PM> code up a loop of readw (or equivalent) in the driver with a big
PM> comment explaining why you need to do it that way.

It doesn't have to be readw (16bit accesses). Enabling the BE mode and
using _outsl/_insl (32bit access with byteswap) works fine.

Let me repeat my question:
Could I get _outsl/_insl re-added to kernel/misc.S or do I have to
reimplement it somewhere else (platform file or smc911x.h)?

-- 
Bye, Peter Korsgaard

Re: Recently removed io accessors

[Linas Vepstas]

On Sat, Oct 14, 2006 at 01:57:03PM +0200, Peter Korsgaard wrote:
> PM> code up a loop of readw (or equivalent) in the driver with a big
> PM> comment explaining why you need to do it that way.
> 
> It doesn't have to be readw (16bit accesses). Enabling the BE mode and
> using _outsl/_insl (32bit access with byteswap) works fine.

I think Paul was trrying to say that it might be easier just to 
16-bit words. Are you trying to use 32-bit access due to performance
concerns?

--linas

Re: Recently removed io accessors

[Peter Korsgaard]

>>>>> "LV" == Linas Vepstas <linas@austin.ibm.com> writes:

Hi,

>> It doesn't have to be readw (16bit accesses). Enabling the BE mode
>> and using _outsl/_insl (32bit access with byteswap) works fine.

LV> I think Paul was trrying to say that it might be easier just to
LV> 16-bit words. Are you trying to use 32-bit access due to
LV> performance concerns?

Yes, this is the hot path of the driver as I don't have DMA hardware
(copy to/from packet buffer).

Another reason is that the same driver is used for the entire 911x
series, some which have an 16bit interface and others with full 32bit.

-- 
Bye, Peter Korsgaard