[PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[Eliezer Tamir]

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

Add bnx2x to Kconfig and Makefile



[-- Attachment #2: 0001-add-bnx2x-to-Kconfig-and-Makefile.txt --]
[-- Type: text/plain, Size: 1425 bytes --]

From 94bf385550356534e6dd41ce61879608a63ed972 Mon Sep 17 00:00:00 2001
From: Eliezer Tamir <eliezert@broadcom.com>
Date: Mon, 8 Oct 2007 11:48:10 +0200
Subject: [PATCH 1/8] add bnx2x to Kconfig and Makefile

Signed-off-by: Eliezer Tamir <eliezert@broadcom.com>
---
 drivers/net/Kconfig  |    9 +++++++++
 drivers/net/Makefile |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 9c635a2..1d44a5e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2630,6 +2630,15 @@ config TEHUTI
 	help
 	  Tehuti Networks 10G Ethernet NIC
 
+config BNX2X
+	tristate "Broadcom NetXtremeII XGb support"
+	depends on PCI
+	help
+	  This driver supports Broadcom NetXtremeII 10 gigabit Ethernet cards.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called bnx2x.  This is recommended.
+
 endif # NETDEV_10000
 
 source "drivers/net/tokenring/Kconfig"
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index d2e0f35..2f11a2c 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_STNIC) += stnic.o 8390.o
 obj-$(CONFIG_FEALNX) += fealnx.o
 obj-$(CONFIG_TIGON3) += tg3.o
 obj-$(CONFIG_BNX2) += bnx2.o
+obj-$(CONFIG_BNX2X) += bnx2x.o
 spidernet-y += spider_net.o spider_net_ethtool.o
 obj-$(CONFIG_SPIDER_NET) += spidernet.o sungem_phy.o
 obj-$(CONFIG_GELIC_NET) += ps3_gelic.o
-- 
1.4.2

Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[David Miller]

From: "Eliezer Tamir" <eliezert@broadcom.com>
Date: Mon, 08 Oct 2007 18:15:17 +0200

> Add bnx2x to Kconfig and Makefile

In a patch submission, the tree should build on every step along the
way in applying your patches, with any given configuration.

Here, the user (or something automated like "make allmodconfig") can
select the new config option, but because the driver source hasn't
been added, the compile will fail.

The idea is that if your patch set really is composed of logically
seperate changes, you submit them one logical compilable piece at a
time, and if the first few patches are ok, they could go right in
whilst we work out issues in later patches.  But that's not how
a new driver is, it is logically one change only.

You go on next to add a foo.c file, and then a foo.h file.
This makes things even more difficult to review.

Really, for a new driver that doesn't make any generic kernel code
changes, just submit the whole thing in one shot.  It's the only
reasonable way.

Thank you.

Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[Eliezer Tamir]

David Miller wrote:
> From: "Eliezer Tamir" <eliezert@broadcom.com>
> Date: Mon, 08 Oct 2007 18:15:17 +0200
> 
>> Add bnx2x to Kconfig and Makefile
> 
> In a patch submission, the tree should build on every step along the
> way in applying your patches, with any given configuration.
> 
> Here, the user (or something automated like "make allmodconfig") can
> select the new config option, but because the driver source hasn't
> been added, the compile will fail.
> 
> The idea is that if your patch set really is composed of logically
> seperate changes, you submit them one logical compilable piece at a
> time, and if the first few patches are ok, they could go right in
> whilst we work out issues in later patches.  But that's not how
> a new driver is, it is logically one change only.
> 
> You go on next to add a foo.c file, and then a foo.h file.
> This makes things even more difficult to review.
> 
> Really, for a new driver that doesn't make any generic kernel code
> changes, just submit the whole thing in one shot.  It's the only
> reasonable way.
> 
> Thank you.
> 

Due to the size of the patch I can not post it to the list.

Here is an FTP link.
ftp://Net_sys_anon@ftp1.broadcom.com/bnx2x-0.40.10-net-2.6.24-one.patch.txt

Or if you prefer, I can post it gzipped.

Thanks
Eliezer

Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[David Miller]

From: "Eliezer Tamir" <eliezert@broadcom.com>
Date: Tue, 09 Oct 2007 06:13:23 +0200

> Due to the size of the patch I can not post it to the list.

Understood

> Here is an FTP link.
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2x-0.40.10-net-2.6.24-one.patch.txt
> 
> Or if you prefer, I can post it gzipped.

I took a look at what takes up so much space and it's the firmware and
this register init stuff.

The firmware is unavoidable, but wrt. the register init tables there
appears to be a ton of superfluous information in that header file.

It seem extreme overkill and I would guess it exists purely to
simplify your in-house chip validation.  I would really wish you
wouldn't do this as these tables take up unswappable kernel memory.

No other driver goes about initializing the chip registers using
tables like this.  Many of them are sequences of nothing but zeros or
the same value repeated over and over!  The non-zero values that are
specific are "magic constants" with no macros to describe up the
meaning of the bits being set in these registers.

Huge functions like bnx2x_idle_chk() that just validate register
values with hundreds of lines of C code are not appropriate for
submission into a Linux kernel network driver.  Again, this looks
like code that assists you with in-house chip validation.

I'm open to putting self tests into the driver, but this adds so much
bloat it really takes things beyond reasonable.

Please remove this self-test code and compress the register
initialization information, preferrably into foo_hw_init() C functions
as is done traditionally in drivers.  It will take up less space,
remove the magic values, and make the hardware easier to understand
for other developers.

This should get the driver under the posting limit of 400K, which to
be honest no driver's should be larger than.  Unfortunately that
firmware file is 350K so you won't have much left to work with :-)

What we could do is post the driver without the firmware, so that
people can review the actual C-code by just replying to this posting
and it won't go over the posting size limit.

Thanks!

Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[Eliezer Tamir]

On Mon, 2007-10-08 at 21:29 -0700, David Miller wrote:
> From: "Eliezer Tamir" <eliezert@broadcom.com>
> Date: Tue, 09 Oct 2007 06:13:23 +0200
> 
> > Due to the size of the patch I can not post it to the list.
> 
> Understood
> 
> > Here is an FTP link.
> > ftp://Net_sys_anon@ftp1.broadcom.com/bnx2x-0.40.10-net-2.6.24-one.patch.txt
> > 
> > Or if you prefer, I can post it gzipped.
> 
> I took a look at what takes up so much space and it's the firmware and
> this register init stuff.
> 
> The firmware is unavoidable, but wrt. the register init tables there
> appears to be a ton of superfluous information in that header file.

First, I must admit that looking over the file again I noticed an error
on my part in generating the file, all the data that initializes FPGA or
EMULATION should not be there. fixed. 
(This reduced about half of the lines but only a third in size.)


> It seem extreme overkill and I would guess it exists purely to
> simplify your in-house chip validation.  I would really wish you
> wouldn't do this as these tables take up unswappable kernel memory.
> 
> No other driver goes about initializing the chip registers using
> tables like this.  Many of them are sequences of nothing but zeros or
> the same value repeated over and over!  The non-zero values that are
> specific are "magic constants" with no macros to describe up the
> meaning of the bits being set in these registers.

Almost all of the zero-filled tables will be removed.
The rest of the registers do need to be initialized.

I agree that the number of registers that needs to be initialized is
huge, but that is caused by the way the hardware was designed.

The values for the initialization come from several sources:
Some are derived from HW code (the XML files used to derive the verilog
code),
Others (along with much of the machine generated .h files) are generated
at microcode build time, adding a microcode routine will cause the init
values to change, using a new variable can cause an .h file to change.
In the last group which is very small, are registers that are controlled
by the driver.

The values in this file really are machine generated, they really are
not meant to be modified directly by editing the file.

The registers that are under the driver's control are in the main .c
and .h files.


> 
> Huge functions like bnx2x_idle_chk() that just validate register
> values with hundreds of lines of C code are not appropriate for
> submission into a Linux kernel network driver.  Again, this looks
> like code that assists you with in-house chip validation.

The idle check code is not a manufacturing test, it is meant to help
debug the driver and microcode.
If the driver sends an invalid command to one of the CPUs which then
chokes on it, this will tell you which one of them died and the general
whereabouts of the problem. (ingress CPU X is stuck because output queue
Y is full)

I agree that it is not a clean implementation.
I will remove it and it will be re-written. 


> I'm open to putting self tests into the driver, but this adds so much
> bloat it really takes things beyond reasonable.
> 
> Please remove this self-test code and compress the register
> initialization information, preferrably into foo_hw_init() C functions
> as is done traditionally in drivers.  It will take up less space,
> remove the magic values, and make the hardware easier to understand
> for other developers.

> This should get the driver under the posting limit of 400K, which to
> be honest no driver's should be larger than.  Unfortunately that
> firmware file is 350K so you won't have much left to work with :-)
> 
> What we could do is post the driver without the firmware, so that
> people can review the actual C-code by just replying to this posting
> and it won't go over the posting size limit.

( Michael has showed me the trick of how to post with evolution, so I
hope that the mangled patch problem is behind us and I think that I can
now post everything without a problem, Hallelujah!)
> 
> Thanks!
> 

Re: [PATCH 1/8][BNX2X] resubmit as attachments: add bnx2x to Kconfig and Makefile

[David Miller]

From: "Eliezer Tamir" <eliezert@broadcom.com>
Date: Tue, 09 Oct 2007 18:20:01 +0200

> Almost all of the zero-filled tables will be removed.
> The rest of the registers do need to be initialized.
> 
> I agree that the number of registers that needs to be initialized is
> huge, but that is caused by the way the hardware was designed.
> 
> The values for the initialization come from several sources:
> Some are derived from HW code (the XML files used to derive the verilog
> code),
> Others (along with much of the machine generated .h files) are generated
> at microcode build time, adding a microcode routine will cause the init
> values to change, using a new variable can cause an .h file to change.
> In the last group which is very small, are registers that are controlled
> by the driver.
> 
> The values in this file really are machine generated, they really are
> not meant to be modified directly by editing the file.
> 
> The registers that are under the driver's control are in the main .c
> and .h files.
...
> The idle check code is not a manufacturing test, it is meant to help
> debug the driver and microcode.
> If the driver sends an invalid command to one of the CPUs which then
> chokes on it, this will tell you which one of them died and the general
> whereabouts of the problem. (ingress CPU X is stuck because output queue
> Y is full)
 ...
> ( Michael has showed me the trick of how to post with evolution, so I
> hope that the mangled patch problem is behind us and I think that I can
> now post everything without a problem, Hallelujah!)

Thanks for the explanations, I look forward to your next
submission.