Marvell MV6436xx ethernet driver patch

Marvell MV6436xx ethernet driver patch

[Nicolas DET]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

  Amiga............: SimpleMail   http://simplemail.sourceforge.net/
  Unix.............: Metamail     ftp://ftp.bellcore.com/nsb/
  Windows/Macintosh: Eudora       http://www.qualcomm.com/

General info about MIME can be found at:

http://www.cis.ohio-state.edu/hypertext/faq/usenet/mail/mime-faq/top.html

[-- Attachment #2: Type: text/plain, Size: 858 bytes --]

Hello,

You can find enclosed a patch for the Marvell MV643xx ethernet driver.

It's also there:
http://arrakin.homedns.org/~nicolas/mv643xx_eth.tar.gz (tarball)
http://arrakin.homedns.org/~nicolas/mv643xx_eth.diff.bz2 

The diff is against the kernel 2.6.13 (kernel.org).

The main changes (AFAIR):
* Workaround for the TCP/UDP hw checksum
* Use hardware for statistics
* Define and use SRAM (for pegasos II archp/ppc/chrp_pegasos_eth.c)
* Able to use max burst size from/to DDR (serious transfer boost)
* Option can be selected through the menu (drivers/net/Kconfig)
* ...

some testing...

By the way, I noticed that page_address() sometimes returns NULL when using
highmem (with a lot of mem).

You are welcome to review this patch. Some parts (especially the TX bug
workaroud) will be appreciated IMO.

Regards,
-- 
Nicolas DET
MorphOS & Linux developer

[-- Attachment #3: mv643xx_eth.diff.bz2 --]
[-- Type: application/octet-stream, Size: 14098 bytes --]

Re: Marvell MV6436xx ethernet driver patch

[Christoph Hellwig]

On Tue, Aug 30, 2005 at 08:07:33PM +0100, Nicolas DET wrote:
> By the way, I noticed that page_address() sometimes returns NULL when using
> highmem (with a lot of mem).

You can only use page_address() on kernel-mapped memory.  For
non-GFP_KERNEL allocation you need to use kmap/kmap_atomic to map it
into kernel-virtual memory space.

Re: Marvell MV6436xx ethernet driver patch

[Dale Farnsworth]

On Tue, Aug 30, 2005 at 08:16:12PM +0100, Nicolas DET wrote:
> You can find enclosed a patch for the Marvell MV643xx ethernet driver.
> 
> It's also there:
> http://arrakin.homedns.org/~nicolas/mv643xx_eth.tar.gz (tarball)
> http://arrakin.homedns.org/~nicolas/mv643xx_eth.diff.bz2 
> 
> The diff is against the kernel 2.6.13 (kernel.org).

Thank you for working on this driver, Nicolas.

> The main changes (AFAIR):
> * Workaround for the TCP/UDP hw checksum

I implemented something similar a while back, but found it too ugly to
keep around.  Fortunately, there is already a much simpler workaround for
the hw checksum bug in linux-2.6.git.  See
	http://oss.sgi.com/archives/netdev/2005-08/msg00124.html

> * Use hardware for statistics

This is good.  We need to remove the code it replaces rather than
have it partially ifdeffed out.  Can you do that and split this part
out as a separate patch and submit to netdev@vger.kernel.org ?

> * Define and use SRAM (for pegasos II archp/ppc/chrp_pegasos_eth.c)

Good.  This is pegasos-specific and it would be good to split this
patch out from the mv643xx driver bits.  Descriptors in SRAM is a big win.  

> * Able to use max burst size from/to DDR (serious transfer boost)

This is a good idea.  I suspect that most of the gain is from
turning off snooping and flushing/invalidating the cache explicitly.
Implementation-wise, I'd rather we not manipulate the MV643XX_ETH_BAR_?
registers directly in the driver.  Today that is done in platform
setup code.  This has promise but needs to be reworked.

> * Option can be selected through the menu (drivers/net/Kconfig)

Do these really need to be user configurable?

The following shouldn't be user options, IMHO:
MV643XX_CHECKSUM_OFFLOAD_TX -- on
MV643XX_CHECKSUM_OFFLOAD_TX_WORKAROUND -- on
MV64XXX_HWSTATS -- on
MV643XX_COAL -- on
MV64XXX_USESRAM -- on/off depending on platform
MV64XXX_MAXBURST -- on/off depending on platform

I'm not as sure about MV643XX_NAPI, but I think it should always be on
for GigE drivers.  Is it worth being able to turn off NAPI?

Some general comments:
Several Kconfig hunks seem unrelated to MV643XX
Several lines in the patch have trailing whitespace.
C++ style comments (//) aren't acceptable.
Why the addition of EXTRA_BYTES to the definition of RX_SKB_SIZE?  Did
you see problems without it?

-Dale Farnsworth

Re: Marvell MV6436xx ethernet driver patch

[Andrew Morton]

Nicolas DET <nd@bplan-gmbh.de> wrote:
>
> You can find enclosed a patch for the Marvell MV643xx ethernet driver.
> 
>  It's also there:
>  http://arrakin.homedns.org/~nicolas/mv643xx_eth.tar.gz (tarball)
>  http://arrakin.homedns.org/~nicolas/mv643xx_eth.diff.bz2 
> 
>  The diff is against the kernel 2.6.13 (kernel.org).
> 
>  The main changes (AFAIR):
>  * Workaround for the TCP/UDP hw checksum
>  * Use hardware for statistics
>  * Define and use SRAM (for pegasos II archp/ppc/chrp_pegasos_eth.c)
>  * Able to use max burst size from/to DDR (serious transfer boost)
>  * Option can be selected through the menu (drivers/net/Kconfig)
>  * ...
> 
>  some testing...
> 
>  By the way, I noticed that page_address() sometimes returns NULL when using
>  highmem (with a lot of mem).
> 
>  You are welcome to review this patch. Some parts (especially the TX bug
>  workaroud) will be appreciated IMO.

- Does strange things to drivers/net/Kconfig, like removing the SKGE entry(?)

- Generates rather a lot of rejects agains post-2.6.13 changes.  You'll
  need to rediff this against current Linus tree or, preferably, against
  the next -mm kernel, please.

- Please cc netdev@vger.kernel.org next time.

Re: Marvell MV6436xx ethernet driver patch

[Nicolas DET]

Hello Dale,

On 31/08/2005, you wrote:
> On Tue, Aug 30, 2005 at 08:16:12PM +0100, Nicolas DET wrote:
>> You can find enclosed a patch for the Marvell MV643xx ethernet driver.
>> 
>> It's also there:
>> http://arrakin.homedns.org/~nicolas/mv643xx_eth.tar.gz (tarball)
>> http://arrakin.homedns.org/~nicolas/mv643xx_eth.diff.bz2 
>> 
>> The diff is against the kernel 2.6.13 (kernel.org).

> Thank you for working on this driver, Nicolas.

It was very fun and interresting :-)
I wish I could spend more time, however I won't be able to this week ;-/.

>> The main changes (AFAIR):
>> * Workaround for the TCP/UDP hw checksum

> I implemented something similar a while back, but found it too ugly to
> keep around.  Fortunately, there is already a much simpler workaround for
> the hw checksum bug in linux-2.6.git.  See
>     http://oss.sgi.com/archives/netdev/2005-08/msg00124.html

Ok.

>> * Define and use SRAM (for pegasos II archp/ppc/chrp_pegasos_eth.c)

> Good.  This is pegasos-specific and it would be good to split this
> patch out from the mv643xx driver bits.  Descriptors in SRAM is a big
> win.   
>> * Able to use max burst size from/to DDR (serious transfer boost)

> This is a good idea.  I suspect that most of the gain is from
> turning off snooping and flushing/invalidating the cache explicitly.
> Implementation-wise, I'd rather we not manipulate the MV643XX_ETH_BAR_?
> registers directly in the driver.  Today that is done in platform
> setup code.  This has promise but needs to be reworked.

Yeah, the point was to have no snooping for this part of the chip.
The descriptors in SRAM, and the data in DDR. This give a serious boost.

I noticed MV643xx memory performances are really higher when turning
off snoop (not only for ethernet).

Well, I confess manipulating such thing here, is not totaly smart.
However I don't really know where to put them.
Maybe, somewhere in arch/ppc ?

Because, at some pooint the driver will need to have this modified in order
to reall work correctly.

For example, if you use a module with that option (it will disable
snooping) and then 'rmmod & modprobe' a new module without it will not work
(no snooping as the new module expect!).

Conclusion: yes, touching ETH_BAR isn't really well here, but where could
we move it ?

>> * Option can be selected through the menu (drivers/net/Kconfig)

> Do these really need to be user configurable?

> The following shouldn't be user options, IMHO:
> MV643XX_CHECKSUM_OFFLOAD_TX -- on
> MV643XX_CHECKSUM_OFFLOAD_TX_WORKAROUND -- on
> MV64XXX_HWSTATS -- on
> MV643XX_COAL -- on
> MV64XXX_USESRAM -- on/off depending on platform
> MV64XXX_MAXBURST -- on/off depending on platform

Ok. It can easily go back into the .h

> I'm not as sure about MV643XX_NAPI, but I think it should always be on
> for GigE drivers.  Is it worth being able to turn off NAPI?

I'm really not sure. I also though NAPI should always be turned on.
Nevertheless, I noticed the CPU usage is really highy with NAPI on than
off. Maybe, NAPI doesn't like too much interrupt coalescing.

> Some general comments:
> Several Kconfig hunks seem unrelated to MV643XX
> Several lines in the patch have trailing whitespace.
> C++ style comments (//) aren't acceptable.
> Why the addition of EXTRA_BYTES to the definition of RX_SKB_SIZE?  Did
> you see problems without it?

Thanks a lot for this comments!
I confess eassily,, I'm not use to submit patch and to the kernel code
rules.
So, you advices are welcome.

Do you want me to clean the patch ? or something ?
I'm not sure I could, I'll be away from thursday to the next friday at
least.

Regards
-- 
Nicolas DET
MorphOS & Linux developer

Re: Marvell MV6436xx ethernet driver patch

[Sven Luther]

On Wed, Aug 31, 2005 at 07:55:49AM +0100, Nicolas DET wrote:
> Hello Dale,
> > This is a good idea.  I suspect that most of the gain is from
> > turning off snooping and flushing/invalidating the cache explicitly.
> > Implementation-wise, I'd rather we not manipulate the MV643XX_ETH_BAR_?
> > registers directly in the driver.  Today that is done in platform
> > setup code.  This has promise but needs to be reworked.
> 
> Conclusion: yes, touching ETH_BAR isn't really well here, but where could
> we move it ?

What about : arch/ppc/plateform/chrp_pegasos_eth.c ?

Friendly,

Sven Luther

Re: Marvell MV6436xx ethernet driver patch

[Mark A. Greer]

On Wed, Aug 31, 2005 at 07:55:49AM +0100, Nicolas DET wrote:
> > This is a good idea.  I suspect that most of the gain is from
> > turning off snooping and flushing/invalidating the cache explicitly.
> > Implementation-wise, I'd rather we not manipulate the MV643XX_ETH_BAR_?
> > registers directly in the driver.  Today that is done in platform
> > setup code.  This has promise but needs to be reworked.
> 
> Yeah, the point was to have no snooping for this part of the chip.
> The descriptors in SRAM, and the data in DDR. This give a serious boost.
> 
> I noticed MV643xx memory performances are really higher when turning
> off snoop (not only for ethernet).
> 
> Well, I confess manipulating such thing here, is not totaly smart.
> However I don't really know where to put them.
> Maybe, somewhere in arch/ppc ?
> 
> Because, at some pooint the driver will need to have this modified in order
> to reall work correctly.
> 
> For example, if you use a module with that option (it will disable
> snooping) and then 'rmmod & modprobe' a new module without it will not work
> (no snooping as the new module expect!).
> 
> Conclusion: yes, touching ETH_BAR isn't really well here, but where could
> we move it ?

The enet->mem BARs are configured in
arch/ppc/syslib/mv64xc60.c:mv64360_config_io2mem_windows().

You can choose how the BAR reg is configured via the mv64x60_setup_info's
'enet_options' member.  The setup_info is passed in from your platform file
via mv64x60_init() at setup_arch() time.  There are examples arch/ppc/platforms.

The enet->sram window can be set up any way you want it in the platform
file as well.  Again, there are examples in arch/ppc/platforms.

IMHO, the platform file should be where things like that belong.

Mark

Re: Marvell MV6436xx ethernet driver patch

[Mark A. Greer]

On Wed, Aug 31, 2005 at 09:04:17AM -0700, Mark A. Greer wrote:
> The enet->mem BARs are configured in
> arch/ppc/syslib/mv64xc60.c:mv64360_config_io2mem_windows().

Doh, that should be arch/ppc/syslib/mv64x60.c:...

> IMHO, the platform file should be where things like that belong.

Should be, "IMHO, the platform file *is* where things like that belong."

Mark

Re: Marvell MV6436xx ethernet driver patch

[Sven Luther]

On Wed, Aug 31, 2005 at 09:04:17AM -0700, Mark A. Greer wrote:
> On Wed, Aug 31, 2005 at 07:55:49AM +0100, Nicolas DET wrote:
> > > This is a good idea.  I suspect that most of the gain is from
> > > turning off snooping and flushing/invalidating the cache explicitly.
> > > Implementation-wise, I'd rather we not manipulate the MV643XX_ETH_BAR_?
> > > registers directly in the driver.  Today that is done in platform
> > > setup code.  This has promise but needs to be reworked.
> > 
> > Yeah, the point was to have no snooping for this part of the chip.
> > The descriptors in SRAM, and the data in DDR. This give a serious boost.
> > 
> > I noticed MV643xx memory performances are really higher when turning
> > off snoop (not only for ethernet).
> > 
> > Well, I confess manipulating such thing here, is not totaly smart.
> > However I don't really know where to put them.
> > Maybe, somewhere in arch/ppc ?
> > 
> > Because, at some pooint the driver will need to have this modified in order
> > to reall work correctly.
> > 
> > For example, if you use a module with that option (it will disable
> > snooping) and then 'rmmod & modprobe' a new module without it will not work
> > (no snooping as the new module expect!).
> > 
> > Conclusion: yes, touching ETH_BAR isn't really well here, but where could
> > we move it ?
> 
> The enet->mem BARs are configured in
> arch/ppc/syslib/mv64xc60.c:mv64360_config_io2mem_windows().

Which is not used in the pegasos code path, as we are chrp though.

Friendly,

Sven Luther

Re: Marvell MV6436xx ethernet driver patch

[Mark A. Greer]

On Wed, Aug 31, 2005 at 06:33:13PM +0200, Sven Luther wrote:

> Which is not used in the pegasos code path, as we are chrp though.

I wondered that after I sent the email.  So, yep, chrp_pegasos_eth.c
seem reasonable to me.

Mark

Re: Marvell MV6436xx ethernet driver patch

[Nicolas DET]

Hello !

I was wondering what is the status of the mv eth driver?

> The main changes (AFAIR):
> * Workaround for the TCP/UDP hw checksum
Should be included in my opinion. It gives a serious boost on TX.

> * Use hardware for statistics
Dunno if it should be commited. I don't think you would gain more than 0.1%
;-)

> * Define and use SRAM (for pegasos II archp/ppc/chrp_pegasos_eth.c)
Should be include IMO. It only applies for the Pegasos II

> * Able to use max burst size from/to DDR (serious transfer boost)
see notes below.

> * Option can be selected through the menu (drivers/net/Kconfig)
Should be removed/ignored as discussed previously

> * ...

> some testing...

> By the way, I noticed that page_address() sometimes returns NULL when
> using highmem (with a lot of mem).

Implementating MAXBURST...
Using kmap/kunmap instead of page_address (which is the right way do to, as
far as I understood), is a bit more complex.
Indeed, kunmap() can't be called from an interrupt.
However, this would have been very simple to kunmap() the page from there.

It would be possible to put every pages to kunmap into
a list (list_add() would be inside the interrupt) and to kunmap() the whole
list at another moment.

This would probably mean to extend pkt_info/eth_tx_desc.
also, I wonder what would happen if a page is:

- kmap() into xmit
- then put in the list into the interuupt
- kmap again into xmit
- and kunmap() afterwards...

Is there any other way to get the bus address of a page ?
The first thing, if we want to keep 'max burst size' should be to disable
it
if high mem is enable. At least, it would be stable.

Bye
-- 
Nicolas DET
MorphOS & Linux developer

Re: Marvell MV6436xx ethernet driver patch

[Christoph Hellwig]

On Tue, Sep 13, 2005 at 11:35:31AM +0100, Nicolas DET wrote:
> Using kmap/kunmap instead of page_address (which is the right way do to, as
> far as I understood), is a bit more complex.
> Indeed, kunmap() can't be called from an interrupt.

kmap_atomic/kunmap_atomic can.

anywya, I think the patch discussion is rather offtopic here.  Network
driver patches are discussed and reviewed at netdev@oss.sgi.com

Re: Marvell MV6436xx ethernet driver patch

[Dale Farnsworth]

On Tue, Sep 13, 2005 at 10:35:39AM +0000, Christoph Hellwig wrote:
> anywya, I think the patch discussion is rather offtopic here.  Network
> driver patches are discussed and reviewed at netdev@oss.sgi.com

The current address is netdev@vger.kernel.org

I responded privately to Nicolas' message (before I realized it was posted
here).  Still, I think it worth mentioning that a workaround for the
mv643xx_eth TCP/UDP hw checksum issue is already in 2.6.14-rc1.

-Dale