Mailing List Archive

[PATCH] net: add QCA alx Ethernet driver
From: Luis R. Rodriguez <mcgrof@frijolero.org>

The next patch adds the new QCA alx Ethernet driver that
supercedes the atl1c Ethernet driver. For details please
read the commit log of the patch. Given the size you can
download the patch from:

http://bombadil.infradead.org/~mcgrof/2012/02/28/add-alx-next-20120228.patch
sha1sum: 8a8f7b6f1cbe737e70ec3b3eda483a6925fd9bd6

Many thanks to our QCA Networking engineering team for their hard
work on getting this driver polished, well tested against atl1c,
and also to Joe and Hao-Ran for their patches.

If you'd like to quickly install this on any system even with older
kernels you can get this tarball which has the driver backported
for older kernels:

http://www.orbit-lab.org/kernel/compat-wireless/2012/02/compat-wireless-2012-02-28-p.tar.bz2
sha1sum: 7c083fc568900fbeefed021d009620830f866819

The "-p" postfix annotates that we have applied a patch in this tarball
from the linux-next-pending/ directory. For more details see:

http://wireless.kernel.org/en/users/Download/stable/#Additional_patches_to_stable_releases

To only compile and install the alx driver you can do:

./scripts/driver-select alx
make
sudo make install

The install will disable atl1c in preference for alx. To revert back
to atl1c you can simply do:

./scripts/alx-enable atl1c

This is part of today's release of compat-wireless with the following
code metrics:

compat-wireless code metrics

828230 - Total upstream lines of code being pulled
2492 - backport code changes
2137 - backport code additions
355 - backport code deletions
9015 - backport from compat module
11507 - total backport code
1.3893 - % of code consists of backport work
13428 - Code changes posted but not yet merged
13407 - Code additions posted but not yet merged
21 - Code deletions posted but not yet merged
1.6213 - % of code not yet merged

Base tree: linux-next.git
Base tree version: next-20120228
compat-wireless release: compat-wireless-2012-02-28-p

Luis R. Rodriguez (1):
alx: add new QCA ethernet driver which supercedes atl1c

MAINTAINERS | 11 +
drivers/net/ethernet/atheros/Kconfig | 42 +-
drivers/net/ethernet/atheros/Makefile | 1 +
drivers/net/ethernet/atheros/alx/Makefile | 3 +
drivers/net/ethernet/atheros/alx/alc_cb.c | 912 ++++++
drivers/net/ethernet/atheros/alx/alc_hw.c | 1087 +++++++
drivers/net/ethernet/atheros/alx/alc_hw.h | 1324 ++++++++
drivers/net/ethernet/atheros/alx/alf_cb.c | 1187 +++++++
drivers/net/ethernet/atheros/alx/alf_hw.c | 918 ++++++
drivers/net/ethernet/atheros/alx/alf_hw.h | 2098 +++++++++++++
drivers/net/ethernet/atheros/alx/alx.h | 670 ++++
drivers/net/ethernet/atheros/alx/alx_ethtool.c | 519 ++++
drivers/net/ethernet/atheros/alx/alx_hwcom.h | 187 ++
drivers/net/ethernet/atheros/alx/alx_main.c | 3899 ++++++++++++++++++++++++
drivers/net/ethernet/atheros/alx/alx_sw.h | 493 +++
15 files changed, 13350 insertions(+), 1 deletions(-)
create mode 100644 drivers/net/ethernet/atheros/alx/Makefile
create mode 100644 drivers/net/ethernet/atheros/alx/alc_cb.c
create mode 100644 drivers/net/ethernet/atheros/alx/alc_hw.c
create mode 100644 drivers/net/ethernet/atheros/alx/alc_hw.h
create mode 100644 drivers/net/ethernet/atheros/alx/alf_cb.c
create mode 100644 drivers/net/ethernet/atheros/alx/alf_hw.c
create mode 100644 drivers/net/ethernet/atheros/alx/alf_hw.h
create mode 100644 drivers/net/ethernet/atheros/alx/alx.h
create mode 100644 drivers/net/ethernet/atheros/alx/alx_ethtool.c
create mode 100644 drivers/net/ethernet/atheros/alx/alx_hwcom.h
create mode 100644 drivers/net/ethernet/atheros/alx/alx_main.c
create mode 100644 drivers/net/ethernet/atheros/alx/alx_sw.h

--
1.7.4.15.g7811d

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
Date: Tue, 28 Feb 2012 17:50:09 -0800

> The next patch adds the new QCA alx Ethernet driver that
> supercedes the atl1c Ethernet driver. For details please
> read the commit log of the patch. Given the size you can
> download the patch from:

What's wrong with making evolutionary fixes to atl1c instead
of replacing it wholesale?

This isn't the way to do it Luis.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Tue, Feb 28, 2012 at 5:53 PM, David Miller <davem@davemloft.net> wrote:
> From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
> Date: Tue, 28 Feb 2012 17:50:09 -0800
>
>> The next patch adds the new QCA alx Ethernet driver that
>> supercedes the atl1c Ethernet driver. For details please
>> read the commit log of the patch. Given the size you can
>> download the patch from:
>
> What's wrong with making evolutionary fixes to atl1c instead
> of replacing it wholesale?
>
> This isn't the way to do it Luis.

I *vehemently* tried to change direction of this driver development to
do exactly what you stated when this driver was first posted last year
in October of 2011 [0]. I could not get commitment on that route
mainly due to the large delta and lack of resources to address this. I
knew this would still be a hard sale but what I proposed was a
comprise -- consider a replacement *if* we can get QCA engineers
tasked to work upstream and committed to fixing *all* issues reported,
*and* also properly tested and compared drivers.

The other option we had reviewed here was to get this into staging but
the garbage driver code is all resolved now. Its your call. This is
the best I could get commitment and resources on.

[0] http://markmail.org/message/7dk7ltdcdlx5ksu6

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
Date: Tue, 28 Feb 2012 18:12:31 -0800

> I *vehemently* tried to change direction of this driver development to
> do exactly what you stated when this driver was first posted last year
> in October of 2011 [0]. I could not get commitment on that route
> mainly due to the large delta and lack of resources to address this.

Oh well, that's unfortunate. Broadcom gave us a similar song and
dance 10 years or so ago when they tried to do the same thing.

We saw what happened then, and I know what Qualcomm will do in the
end. It's just a matter of time for how long it will take Qualcomm
to relent and do the right thing and properly maintain the atl1c
driver.

The good news is that I'm incredibly patient.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
Hi David
The code related to hardware of atl1c is quite diff with that in our hand, every time we take much time to find which part should be
revised for one bug/issue found on other platform/OSes. the new driver make it easy to fix and support new chipset as well.

We reviewed all patches that applied to atl1c and added it to the alx during coding. And did tests for both new and old drivers
You could find the result here: http://www.linuxfoundation.org/collaborate/workgroups/networking/alx


Best Regards
-Xiong

-----Original Message-----
> From: mcgrof@gmail.com [mailto:mcgrof@gmail.com] On Behalf Of Luis R.
> Rodriguez
> Sent: Wednesday, February 29, 2012 10:13
> To: David Miller
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; qca-linux-team; nic-
> devel; Giori, Kathy; chris.snook@gmail.com; Olivari, Mathieu; Huntsman, Bryan
> Subject: Re: [PATCH] net: add QCA alx Ethernet driver
>
> On Tue, Feb 28, 2012 at 5:53 PM, David Miller <davem@davemloft.net> wrote:
> > From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
> > Date: Tue, 28 Feb 2012 17:50:09 -0800
> >
> >> The next patch adds the new QCA alx Ethernet driver that supercedes
> >> the atl1c Ethernet driver. For details please read the commit log of
> >> the patch. Given the size you can download the patch from:
> >
> > What's wrong with making evolutionary fixes to atl1c instead of
> > replacing it wholesale?
> >
> > This isn't the way to do it Luis.
>
> I *vehemently* tried to change direction of this driver development to do
> exactly what you stated when this driver was first posted last year in October of
> 2011 [0]. I could not get commitment on that route mainly due to the large delta
> and lack of resources to address this. I knew this would still be a hard sale but
> what I proposed was a comprise -- consider a replacement *if* we can get QCA
> engineers tasked to work upstream and committed to fixing *all* issues
> reported,
> *and* also properly tested and compared drivers.
>
> The other option we had reviewed here was to get this into staging but the
> garbage driver code is all resolved now. Its your call. This is the best I could get
> commitment and resources on.
>
> [0] http://markmail.org/message/7dk7ltdcdlx5ksu6
>
> Luis
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü¨}©ž²Æ zÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ߢf”ù^jÇ«y§m…á@A«a¶Úÿ 0¶ìh®å’i
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
From: "Huang, Xiong" <xiong@qca.qualcomm.com>
Date: Wed, 29 Feb 2012 02:19:05 +0000

> The code related to hardware of atl1c is quite diff with that
> in our hand, every time we take much time to find which part should
> be revised for one bug/issue found on other platform/OSes. the new
> driver make it easy to fix and support new chipset as well.

Broadcom gave the same excuses. Save yourself some typing.

I already know what you are going to say and claim are the issues.

I've heard it all before, and none of them justify replacing the
existing driver wholesale. We simply don't operate this way.

Because you guys didn't upstream your driver properly back when
the atl1c hardware first came out and Linux needed a driver,
someone else did.

And we're not removing their driver just because now you guys just
want to come in and take over simply because you're not willing to
collaborate properly and contribute to the existing code like everyone
else on the planet does.

There is no arguing against this, there is no justification, and it
simply will not happen. I'm sorry you put some must effort into this
but you should have been smart and listened to Luis last year when he
tried to guide you in the right direction.

Listening to Luis would have saved you a lot of wasted engineering.

Maybe you will be smart and listen to him next time.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
Hi David

We understand your concern. To support the new chipset, do you think it's reasonable to upstream it as a new driver, not a replacement ?

Thanks
Xiong

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, February 29, 2012 10:29
> To: Huang, Xiong
> Cc: Rodriguez, Luis; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> qca-linux-team; nic-devel; Giori, Kathy; chris.snook@gmail.com; Olivari,
> Mathieu; Huntsman, Bryan
> Subject: Re: [PATCH] net: add QCA alx Ethernet driver
>
> From: "Huang, Xiong" <xiong@qca.qualcomm.com>
> Date: Wed, 29 Feb 2012 02:19:05 +0000
>
> > The code related to hardware of atl1c is quite diff with that in
> > our hand, every time we take much time to find which part should be
> > revised for one bug/issue found on other platform/OSes. the new driver
> > make it easy to fix and support new chipset as well.
>
> Broadcom gave the same excuses. Save yourself some typing.
>
> I already know what you are going to say and claim are the issues.
>
> I've heard it all before, and none of them justify replacing the existing driver
> wholesale. We simply don't operate this way.
>
> Because you guys didn't upstream your driver properly back when the atl1c
> hardware first came out and Linux needed a driver, someone else did.
>
> And we're not removing their driver just because now you guys just want to
> come in and take over simply because you're not willing to collaborate properly
> and contribute to the existing code like everyone else on the planet does.
>
> There is no arguing against this, there is no justification, and it simply will not
> happen. I'm sorry you put some must effort into this but you should have been
> smart and listened to Luis last year when he tried to guide you in the right
> direction.
>
> Listening to Luis would have saved you a lot of wasted engineering.
>
> Maybe you will be smart and listen to him next time.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Tue, 28 Feb 2012 17:50:09 -0800
"Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:

> From: Luis R. Rodriguez <mcgrof@frijolero.org>
>
> The next patch adds the new QCA alx Ethernet driver that
> supercedes the atl1c Ethernet driver. For details please
> read the commit log of the patch. Given the size you can
> download the patch from:
>
> http://bombadil.infradead.org/~mcgrof/2012/02/28/add-alx-next-20120228.patch
> sha1sum: 8a8f7b6f1cbe737e70ec3b3eda483a6925fd9bd6

Evolution is better. The new driver has lots of new callbacks to handle
the fact it is dealing with two different chipsets. Not only that your callbacks
are built at runtime which leads to security concerns.

There is a reason the two Marvell based drivers (skge and sky2) are
different drivers. Having to do extra per-chip callbacks is a clear sign
the driver should be split in two.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
From: "Huang, Xiong" <xiong@qca.qualcomm.com>
Date: Wed, 29 Feb 2012 03:11:14 +0000

> We understand your concern. To support the new chipset, do you
> think it's reasonable to upstream it as a new driver, not a
> replacement ?

It depends upon how similar the chips are.

To be honest tg3, as one example, supports quite a large array of
different pieces of hardware that use the same logical core.

So that would be my litmus test about how different a chip needs
to be to deserve an entirely new driver.

I strongly suggest you try to get atl1c working properly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
Even though this whole thing has been rejected,
here are some possible trivial logging corrections.
---
drivers/net/ethernet/atheros/alx/alf_cb.c | 2 +-
drivers/net/ethernet/atheros/alx/alx_main.c | 63 ++++++++++++---------------
drivers/net/ethernet/atheros/alx/alx_sw.h | 4 +-
3 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/alf_cb.c b/drivers/net/ethernet/atheros/alx/alf_cb.c
index d267760..9e823f6 100644
--- a/drivers/net/ethernet/atheros/alx/alf_cb.c
+++ b/drivers/net/ethernet/atheros/alx/alf_cb.c
@@ -274,7 +274,7 @@ static int alf_check_phy_link(struct alx_hw *hw, u32 *speed, bool *link_up)
if (giga & L1F_GIGA_PSSR_DPLX)
*speed = ALX_LINK_SPEED_1GB_FULL;
else
- alx_hw_err(hw, "1000M half is invalid");
+ alx_hw_err(hw, "1000M half is invalid\n");
break;
case L1F_GIGA_PSSR_100MBS:
if (giga & L1F_GIGA_PSSR_DPLX)
diff --git a/drivers/net/ethernet/atheros/alx/alx_main.c b/drivers/net/ethernet/atheros/alx/alx_main.c
index a51c608..a599269 100644
--- a/drivers/net/ethernet/atheros/alx/alx_main.c
+++ b/drivers/net/ethernet/atheros/alx/alx_main.c
@@ -19,8 +19,7 @@

char alx_drv_name[] = "alx";
static const char alx_drv_description[] =
- "Qualcomm Atheros(R) "
- "AR813x/AR815x/AR816x PCI-E Ethernet Network Driver";
+ "Qualcomm Atheros(R) AR813x/AR815x/AR816x PCI-E Ethernet Network Driver";

/* alx_pci_tbl - PCI Device ID Table
*
@@ -142,22 +141,19 @@ static int alx_validate_mac_addr(u8 *mac_addr)
{
int retval = 0;

- if (mac_addr[0] & 0x01) {
- printk(KERN_DEBUG "MAC address is multicast\n");
+ if (is_broadcast_ether_addr(mac_addr)) {
+ pr_debug("MAC address is broadcast\n");
retval = -EADDRNOTAVAIL;
- } else if (mac_addr[0] == 0xff && mac_addr[1] == 0xff) {
- printk(KERN_DEBUG "MAC address is broadcast\n");
+ } else if (is_multicast_ether_addr(mac_addr)) {
+ pr_debug("MAC address is multicast\n");
retval = -EADDRNOTAVAIL;
- } else if (mac_addr[0] == 0 && mac_addr[1] == 0 &&
- mac_addr[2] == 0 && mac_addr[3] == 0 &&
- mac_addr[4] == 0 && mac_addr[5] == 0) {
- printk(KERN_DEBUG "MAC address is all zeros\n");
+ } else if (is_zero_ether_addr(mac_addr)) {
+ pr_debug("MAC address is all zeros\n");
retval = -EADDRNOTAVAIL;
}
return retval;
}

-
/*
* alx_set_mac_type - Sets MAC type
*/
@@ -459,7 +455,7 @@ static bool alx_get_rrdesc(struct alx_rx_queue *rxque,

if (likely(srrd->genr.nor != 1)) {
/* TODO support mul rfd*/
- printk(KERN_EMERG "Multi rfd not support yet!\n");
+ pr_emerg("Multi rfd not support yet!\n");
}

srrd->genr.update = 0;
@@ -1068,7 +1064,7 @@ static void alx_set_msix_flags(struct alx_msix_param *msix,
SET_MSIX_FLAG(RX7);
break;
default:
- printk(KERN_ERR "alx_set_msix_flags: rx error.");
+ pr_err("%s: rx error\n", __func__);
break;
}
} else if (type == alx_msix_type_tx) {
@@ -1086,7 +1082,7 @@ static void alx_set_msix_flags(struct alx_msix_param *msix,
SET_MSIX_FLAG(TX3);
break;
default:
- printk(KERN_ERR "alx_set_msix_flags: tx error.");
+ pr_err("%s: tx error\n", __func__);
break;
}
} else if (type == alx_msix_type_other) {
@@ -1104,7 +1100,7 @@ static void alx_set_msix_flags(struct alx_msix_param *msix,
SET_MSIX_FLAG(PHY);
break;
default:
- printk(KERN_ERR "alx_set_msix_flags: other error.");
+ pr_err("%s: other error\n", __func__);
break;
}
}
@@ -1742,11 +1738,11 @@ static int alx_alloc_all_rtx_queue(struct alx_adapter *adpt)
return 0;

err_alloc_rx_queue:
- alx_err(adpt, "goto err_alloc_rx_queue");
+ alx_err(adpt, "goto err_alloc_rx_queue\n");
for (que_idx = 0; que_idx < adpt->num_rxques; que_idx++)
kfree(adpt->rx_queue[que_idx]);
err_alloc_tx_queue:
- alx_err(adpt, "goto err_alloc_tx_queue");
+ alx_err(adpt, "goto err_alloc_tx_queue\n");
for (que_idx = 0; que_idx < adpt->num_txques; que_idx++)
kfree(adpt->tx_queue[que_idx]);
return -ENOMEM;
@@ -2976,17 +2972,17 @@ static void alx_link_task_routine(struct alx_adapter *adpt)
if (netif_carrier_ok(netdev))
return;

- link_desc = (hw->link_speed == ALX_LINK_SPEED_1GB_FULL) ?
+ link_desc = hw->link_speed == ALX_LINK_SPEED_1GB_FULL) ?
"1 Gbps Duplex Full" :
- (hw->link_speed == ALX_LINK_SPEED_100_FULL ?
- "100 Mbps Duplex Full" :
- (hw->link_speed == ALX_LINK_SPEED_100_HALF ?
- "100 Mbps Duplex Half" :
- (hw->link_speed == ALX_LINK_SPEED_10_FULL ?
- "10 Mbps Duplex Full" :
- (hw->link_speed == ALX_LINK_SPEED_10_HALF ?
- "10 Mbps Duplex HALF" :
- "unknown speed"))));
+ hw->link_speed == ALX_LINK_SPEED_100_FULL ?
+ "100 Mbps Duplex Full" :
+ hw->link_speed == ALX_LINK_SPEED_100_HALF ?
+ "100 Mbps Duplex Half" :
+ hw->link_speed == ALX_LINK_SPEED_10_FULL ?
+ "10 Mbps Duplex Full" :
+ hw->link_speed == ALX_LINK_SPEED_10_HALF ?
+ "10 Mbps Duplex HALF" :
+ "unknown speed");
netif_info(adpt, timer, adpt->netdev,
"NIC Link is Up %s\n", link_desc);

@@ -3171,8 +3167,7 @@ do_csum:
cso = skb_checksum_start_offset(skb);

if (unlikely(cso & 0x1)) {
- dev_err(&pdev->dev, "pay load offset should not be an "
- "event number\n");
+ dev_err(&pdev->dev, "pay load offset should not be an event number\n");
return -1;
} else {
css = cso + skb->csum_offset;
@@ -3642,16 +3637,14 @@ static int __devinit alx_init(struct pci_dev *pdev,
alx_set_register_info_special(adpt);

netif_dbg(adpt, probe, adpt->netdev,
- "num_msix_noque_intrs = %d, num_msix_rxque_intrs = %d, "
- "num_msix_txque_intrs = %d\n",
+ "num_msix_noque_intrs = %d, num_msix_rxque_intrs = %d, num_msix_txque_intrs = %d\n",
adpt->num_msix_noques, adpt->num_msix_rxques,
adpt->num_msix_txques);
netif_dbg(adpt, probe, adpt->netdev, "num_msix_all_intrs = %d\n",
adpt->num_msix_intrs);

netif_dbg(adpt, probe, adpt->netdev,
- "RX Queue Count = %u, HRX Queue Count = %u, "
- "SRX Queue Count = %u, TX Queue Count = %u\n",
+ "RX Queue Count = %u, HRX Queue Count = %u, SRX Queue Count = %u, TX Queue Count = %u\n",
adpt->num_rxques, adpt->num_hw_rxques, adpt->num_sw_rxques,
adpt->num_txques);

@@ -3719,7 +3712,7 @@ static int __devinit alx_init(struct pci_dev *pdev,
CHK_ADPT_FLAG(0, SRSS_EN) ? "Enable" : "Disable");
}

- printk(KERN_INFO "alx: Atheros Gigabit Network Connection\n");
+ pr_info("Atheros Gigabit Network Connection\n");
cards_found++;
return 0;

@@ -3882,7 +3875,7 @@ static int __init alx_init_module(void)
{
int retval;

- printk(KERN_INFO "%s\n", alx_drv_description);
+ pr_info("%s\n", alx_drv_description);
retval = pci_register_driver(&alx_driver);

return retval;
diff --git a/drivers/net/ethernet/atheros/alx/alx_sw.h b/drivers/net/ethernet/atheros/alx/alx_sw.h
index 3daa392..9118da5 100644
--- a/drivers/net/ethernet/atheros/alx/alx_sw.h
+++ b/drivers/net/ethernet/atheros/alx/alx_sw.h
@@ -479,8 +479,8 @@ extern int alc_init_hw_callbacks(struct alx_hw *hw);
extern int alf_init_hw_callbacks(struct alx_hw *hw);

/* Logging message functions */
-void __printf(3, 4) alx_hw_printk(const char *level, const struct alx_hw *hw,
- const char *fmt, ...);
+void __printf(3, 4)
+alx_hw_printk(const char *level, const struct alx_hw *hw, const char *fmt, ...);

#define alx_hw_err(_hw, _format, ...) \
alx_hw_printk(KERN_ERR, _hw, _format, ##__VA_ARGS__)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
My another concern is the license type. Current atl1c use GPL v2, and got some contribution outside of QCA though the first version
was wrote by our Atheros guy. And that guy just copied the license from atl1e/atl1x that contains the words "Derived from Intel e1000".

If I update this driver and add new chip support on it, Our Company's legal might not permit me to do so, they prefer QCA ISC license.
Is it possible to change the license header to ISC while I applying patches to source code ?


Thanks
xiong

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, February 29, 2012 11:33
> To: Huang, Xiong
> Cc: Rodriguez, Luis; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> qca-linux-team; nic-devel; Giori, Kathy; chris.snook@gmail.com; Olivari,
> Mathieu; Huntsman, Bryan
> Subject: Re: [PATCH] net: add QCA alx Ethernet driver
>
> From: "Huang, Xiong" <xiong@qca.qualcomm.com>
> Date: Wed, 29 Feb 2012 03:11:14 +0000
>
> > We understand your concern. To support the new chipset, do you think
> > it's reasonable to upstream it as a new driver, not a replacement ?
>
> It depends upon how similar the chips are.
>
> To be honest tg3, as one example, supports quite a large array of different
> pieces of hardware that use the same logical core.
>
> So that would be my litmus test about how different a chip needs to be to
> deserve an entirely new driver.
>
> I strongly suggest you try to get atl1c working properly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Tue, Feb 28, 2012 at 11:30 PM, Huang, Xiong <xiong@qca.qualcomm.com> wrote:
> My another concern is the license type. Current atl1c use GPL v2, and got some contribution outside of QCA though the first version
> was wrote by our Atheros guy. And that guy just copied the license from atl1e/atl1x that contains the words "Derived from Intel e1000".
>
> If I update this driver and add new chip support on it, Our Company's legal might not permit me to do so, they prefer QCA ISC license.
> Is it possible to change the license header to ISC while I applying patches to source code ?

No.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
Stephen Hemminger <shemminger@vyatta.com> :
[...]
> Evolution is better. The new driver has lots of new callbacks to handle
> the fact it is dealing with two different chipsets. Not only that your callbacks
> are built at runtime which leads to security concerns.
>
> There is a reason the two Marvell based drivers (skge and sky2) are
> different drivers. Having to do extra per-chip callbacks is a clear sign
> the driver should be split in two.

(arguable)

Whatever QCA chooses, here is a collection of things QCA's incoming patches
could avoid.

> diff --git a/drivers/net/ethernet/atheros/Kconfig b/drivers/net/ethernet/atheros/Kconfig
> index 1ed886d..a1cfc98 100644
> --- a/drivers/net/ethernet/atheros/Kconfig
> +++ b/drivers/net/ethernet/atheros/Kconfig
[...]
> +#include <linux/pci_regs.h>
> +#include <linux/mii.h>
> +
> +#include "alc_hw.h"
> +
> +
> +/* NIC */
> +static int alc_identify_nic(struct alx_hw *hw)
> +{
> + return 0;
> +}
> +
> +
> +/* PHY */
> +static int alc_read_phy_reg(struct alx_hw *hw, u16 reg_addr, u16 *phy_data)
> +{
> + unsigned long flags;
^^

(applies several times through the driver)

> + int retval = 0;
> +
> + spin_lock_irqsave(&hw->mdio_lock, flags);
> +
> + if (l1c_read_phy(hw, false, ALX_MDIO_DEV_TYPE_NORM, false, reg_addr,
> + phy_data)) {

The return status style is gross. It could (should ?) be:

rc = l1c_read_phy(hw, false, ALX_MDIO_DEV_TYPE_NORM, false, reg, data);
if (rc < 0)


[...]
> +static int alc_setup_phy_link(struct alx_hw *hw, u32 speed, bool autoneg,
> + bool fc)
> +{
> + u8 link_cap = 0;
> + int retval = 0;
> +
> + alx_hw_info(hw, "speed = 0x%x, autoneg = %d\n", speed, autoneg);
> + if (speed & ALX_LINK_SPEED_1GB_FULL)
> + link_cap |= LX_LC_1000F;
> +
> + if (speed & ALX_LINK_SPEED_100_FULL)
> + link_cap |= LX_LC_100F;
> +
> + if (speed & ALX_LINK_SPEED_100_HALF)
> + link_cap |= LX_LC_100H;
> +
> + if (speed & ALX_LINK_SPEED_10_FULL)
> + link_cap |= LX_LC_10F;
> +
> + if (speed & ALX_LINK_SPEED_10_HALF)
> + link_cap |= LX_LC_10H;

This ought to be a loop but one of LX_LC_... or ALX_LINK_SPEED_... probably
deserves to be removed.

[...]
> +static int alc_setup_phy_link_speed(struct alx_hw *hw, u32 speed,
> + bool autoneg, bool fc)
> +{
> + /*
> + * Clear autoneg_advertised and set new values based on input link
> + * speed.
> + */
> + hw->autoneg_advertised = 0;
> +
> + if (speed & ALX_LINK_SPEED_1GB_FULL)
> + hw->autoneg_advertised |= ALX_LINK_SPEED_1GB_FULL;
> +
> + if (speed & ALX_LINK_SPEED_100_FULL)
> + hw->autoneg_advertised |= ALX_LINK_SPEED_100_FULL;
> +
> + if (speed & ALX_LINK_SPEED_100_HALF)
> + hw->autoneg_advertised |= ALX_LINK_SPEED_100_HALF;
> +
> + if (speed & ALX_LINK_SPEED_10_FULL)
> + hw->autoneg_advertised |= ALX_LINK_SPEED_10_FULL;
> +
> + if (speed & ALX_LINK_SPEED_10_HALF)
> + hw->autoneg_advertised |= ALX_LINK_SPEED_10_HALF;
> +
> + return alc_setup_phy_link(hw, hw->autoneg_advertised,
> + autoneg, fc);

It should fit within a single line.

> +}
> +
> +
> +static int alc_check_phy_link(struct alx_hw *hw, u32 *speed, bool *link_up)
> +{
> + u16 bmsr, giga;
> + int retval;
> +
> + alc_read_phy_reg(hw, MII_BMSR, &bmsr);
> + retval = alc_read_phy_reg(hw, MII_BMSR, &bmsr);
> + if (retval)
> + return retval;
> +
> + if (!(bmsr & BMSR_LSTATUS)) {
> + *link_up = false;
> + *speed = ALX_LINK_SPEED_UNKNOWN;
> + return 0;

It could be 'return rc^Wretval' too.

[...]
> +static int alc_config_mac(struct alx_hw *hw, u16 rxbuf_sz, u16 rx_qnum,
> + u16 rxring_sz, u16 tx_qnum, u16 txring_sz)
> +{
> + u8 *addr;
> +
> + u32 txmem_hi, txmem_lo[4];
> +
> + u32 rxmem_hi, rfdmem_lo, rrdmem_lo;
> +
> + u16 smb_timer, mtu_with_eth, int_mod;
> + bool hash_legacy;
> +
> + int i;

Why are they so many empty lines ?

> + int retval = 0;
> +
> + addr = hw->mac_addr;
> +
> + txmem_hi = ALX_DMA_ADDR_HI(hw->tpdma[0]);
> + for (i = 0; i < tx_qnum; i++)
> + txmem_lo[i] = ALX_DMA_ADDR_LO(hw->tpdma[i]);
> +
> +
> + rxmem_hi = ALX_DMA_ADDR_HI(hw->rfdma[0]);
> + rfdmem_lo = ALX_DMA_ADDR_LO(hw->rfdma[0]);
> + rrdmem_lo = ALX_DMA_ADDR_LO(hw->rrdma[0]);
> +
> +
> + smb_timer = (u16)hw->smb_timer;
> + mtu_with_eth = hw->mtu + ALX_ETH_LENGTH_OF_HEADER;
> + int_mod = hw->imt;
> +
> + hash_legacy = true;
> +
> + if (l1c_init_mac(hw, addr, txmem_hi, txmem_lo, tx_qnum, txring_sz,
> + rxmem_hi, rfdmem_lo, rrdmem_lo, rxring_sz, rxbuf_sz,
> + smb_timer, mtu_with_eth, int_mod, hash_legacy)) {

This should use some real struct.


> + alx_hw_err(hw, "error when config mac\n");
> + retval = -EINVAL;
> + }
> +
> + return retval;
> +}
> +
> +
> +/**
> + * alc_get_mac_addr
> + * @hw: pointer to hardware structure
> + **/

Useless.

[...]
> +static int alc_config_mac_ctrl(struct alx_hw *hw)
> +{
> + u32 mac;
> +
> + alx_mem_r32(hw, L1C_MAC_CTRL, &mac);
> +
> + /* enable/disable VLAN tag insert,strip */
> + if (CHK_HW_FLAG(VLANSTRIP_EN))
> + mac |= L1C_MAC_CTRL_VLANSTRIP;
> + else
> + mac &= ~L1C_MAC_CTRL_VLANSTRIP;
> +
> + if (CHK_HW_FLAG(PROMISC_EN))
> + mac |= L1C_MAC_CTRL_PROMISC_EN;
> + else
> + mac &= ~L1C_MAC_CTRL_PROMISC_EN;
> +
> + if (CHK_HW_FLAG(MULTIALL_EN))
> + mac |= L1C_MAC_CTRL_MULTIALL_EN;
> + else
> + mac &= ~L1C_MAC_CTRL_MULTIALL_EN;
> +
> + if (CHK_HW_FLAG(LOOPBACK_EN))
> + mac |= L1C_MAC_CTRL_LPBACK_EN;
> + else
> + mac &= ~L1C_MAC_CTRL_LPBACK_EN;

Code duplication. Could use loops.

[...]
> +/* RAR, Multicast, VLAN */
> +static int alc_set_mac_addr(struct alx_hw *hw, u8 *addr)
> +{
> + u32 sta;
> +
> + /*
> + * for example: 00-0B-6A-F6-00-DC
> + * 0<-->6AF600DC, 1<-->000B.
> + */
> +
> + /* low dword */
> + sta = (((u32)addr[2]) << 24) | (((u32)addr[3]) << 16) |
> + (((u32)addr[4]) << 8) | (((u32)addr[5])) ;

Useless casts.

[...]
> +static int alc_config_tx(struct alx_hw *hw)
> +{
> + return 0;
> +}

al{fc}_config_tx always return 0.

So do al{fc}_config_mac_ctrl, _set_mac_addr, _get_ethtool_regs and
surely a few others.

[...]
> +int alc_init_hw_callbacks(struct alx_hw *hw)
> +{
> + /* NIC */
> + hw->cbs.identify_nic = &alc_identify_nic;
> + /* MAC*/
> + hw->cbs.reset_mac = &alc_reset_mac;
> + hw->cbs.start_mac = &alc_start_mac;
> + hw->cbs.stop_mac = &alc_stop_mac;
> + hw->cbs.config_mac = &alc_config_mac;
> + hw->cbs.get_mac_addr = &alc_get_mac_addr;
> + hw->cbs.set_mac_addr = &alc_set_mac_addr;
> + hw->cbs.set_mc_addr = &alc_set_mc_addr;
> + hw->cbs.clear_mc_addr = &alc_clear_mc_addr;
> +
> + /* PHY */
> + hw->cbs.init_phy = &alc_init_phy;
> + hw->cbs.reset_phy = &alc_reset_phy;
> + hw->cbs.read_phy_reg = &alc_read_phy_reg;
> + hw->cbs.write_phy_reg = &alc_write_phy_reg;
> + hw->cbs.check_phy_link = &alc_check_phy_link;
> + hw->cbs.setup_phy_link = &alc_setup_phy_link;
> + hw->cbs.setup_phy_link_speed = &alc_setup_phy_link_speed;
> +
> + /* Interrupt */
> + hw->cbs.ack_phy_intr = &alc_ack_phy_intr;
> + hw->cbs.enable_legacy_intr = &alc_enable_legacy_intr;
> + hw->cbs.disable_legacy_intr = &alc_disable_legacy_intr;
> +
> + /* Configure */
> + hw->cbs.config_tx = &alc_config_tx;
> + hw->cbs.config_fc = &alc_config_fc;
> + hw->cbs.config_aspm = &alc_config_aspm;
> + hw->cbs.config_wol = &alc_config_wol;
> + hw->cbs.config_mac_ctrl = &alc_config_mac_ctrl;
> + hw->cbs.config_pow_save = &alc_config_pow_save;
> + hw->cbs.reset_pcie = &alc_reset_pcie;
> +
> + /* NVRam */
> + hw->cbs.check_nvram = &alc_check_nvram;
> + hw->cbs.read_nvram = &alc_read_nvram;
> + hw->cbs.write_nvram = &alc_write_nvram;
> +
> + /* Others */
> + hw->cbs.get_ethtool_regs = alc_get_ethtool_regs;

Two alc nics, twice the size ? Mildly efficient.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alc_hw.c b/drivers/net/ethernet/atheros/alx/alc_hw.c
> new file mode 100644
> index 0000000..b0eb72c
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alc_hw.c
[...]
> +u16 l1c_reset_phy(struct alx_hw *hw, bool pws_en, bool az_en, bool ptp_en)
> +{
> + u32 val;
> + u16 i, phy_val;
> +
> + ptp_en = ptp_en;
> +
> + /* reset PHY core */
> + alx_mem_r32(hw, L1C_PHY_CTRL, &val);
> + val &= ~(L1C_PHY_CTRL_DSPRST_OUT | L1C_PHY_CTRL_IDDQ |
> + L1C_PHY_CTRL_GATE_25M | L1C_PHY_CTRL_POWER_DOWN |
> + L1C_PHY_CTRL_CLS);
> + val |= L1C_PHY_CTRL_RST_ANALOG;
> +
> + if (pws_en)
> + val |= (L1C_PHY_CTRL_HIB_PULSE | L1C_PHY_CTRL_HIB_EN);
> + else
> + val &= ~(L1C_PHY_CTRL_HIB_PULSE | L1C_PHY_CTRL_HIB_EN);
> +
> + alx_mem_w32(hw, L1C_PHY_CTRL, val);
> + udelay(10); /* 5us is enough */
> + alx_mem_w32(hw, L1C_PHY_CTRL, val | L1C_PHY_CTRL_DSPRST_OUT);
> +
> + /* delay 800us */
> + for (i = 0; i < L1C_PHY_CTRL_DSPRST_TO; i++)
> + udelay(10);
> +
> + /* switch clock */
> + if (hw->pci_devid == L2CB_DEV_ID) {
> + l1c_read_phydbg(hw, true, L1C_MIIDBG_CFGLPSPD, &phy_val);
> + /* clear bit13 */
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_CFGLPSPD,
> + phy_val & ~L1C_CFGLPSPD_RSTCNT_CLK125SW);
> + }
> +
> + /* fix tx-half-amp issue */
> + if (hw->pci_devid == L2CB_DEV_ID || hw->pci_devid == L2CB2_DEV_ID) {
> + l1c_read_phydbg(hw, true, L1C_MIIDBG_CABLE1TH_DET, &phy_val);
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_CABLE1TH_DET,
> + phy_val | L1C_CABLE1TH_DET_EN); /* set bit15 */
> + }
> +
> + if (pws_en) {
> + /* clear bit[3] of debugport 3B to 0,
> + * lower voltage to save power */
> + if (hw->pci_devid == L2CB_DEV_ID ||
> + hw->pci_devid == L2CB2_DEV_ID) {
> + l1c_read_phydbg(hw, true, L1C_MIIDBG_VOLT_CTRL,
> + &phy_val);
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_VOLT_CTRL,
> + phy_val & ~L1C_VOLT_CTRL_SWLOWEST);
> + }
> + /* power saving config */
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_LEGCYPS,
> + (hw->pci_devid == L1D_DEV_ID ||
> + hw->pci_devid == L1D2_DEV_ID) ?
> + L1D_LEGCYPS_DEF : L1C_LEGCYPS_DEF);
> + /* hib */
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_SYSMODCTRL,
> + L1C_SYSMODCTRL_IECHOADJ_DEF);
> + } else {
> + /*dis powersaving */
> + l1c_read_phydbg(hw, true, L1C_MIIDBG_LEGCYPS, &phy_val);
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_LEGCYPS,
> + phy_val & ~L1C_LEGCYPS_EN);
> + /* disable hibernate */
> + l1c_read_phydbg(hw, true, L1C_MIIDBG_HIBNEG, &phy_val);
> + l1c_write_phydbg(hw, true, L1C_MIIDBG_HIBNEG,
> + phy_val & ~L1C_HIBNEG_PSHIB_EN);
> + }
> +
> + /* az is only for l2cbv2 / l1dv1 /l1dv2 */
> + if (hw->pci_devid == L1D_DEV_ID ||
> + hw->pci_devid == L1D2_DEV_ID ||
> + hw->pci_devid == L2CB2_DEV_ID) {

If it's chipset specific, why not take it elsewhere...

> + if (az_en) {
> + switch (hw->pci_devid) {
> + case L2CB2_DEV_ID:
[...]
> + case L1D2_DEV_ID:
> + l1c_write_phydbg(hw, true,
> + L1C_MIIDBG_AZ_ANADECT,
> + L1C_AZ_ANADECT_DEF);
> + phy_val = hw->long_cable ? L1C_CLDCTRL3_L1D :
> + (L1C_CLDCTRL3_L1D &
> + ~L1C_CLDCTRL3_BP_CABLE1TH_DET_GT);
> + l1c_write_phy(hw, true, L1C_MIIEXT_PCS, true,
> + L1C_MIIEXT_CLDCTRL3, phy_val);
> + l1c_write_phy(hw, true, L1C_MIIEXT_PCS, true,
> + L1C_MIIEXT_AZCTRL,
> + L1C_AZCTRL_L1D);
> + l1c_write_phy(hw, true, L1C_MIIEXT_PCS, true,
> + L1C_MIIEXT_AZCTRL2,
> + L1C_AZCTRL2_L1D2);
> + l1c_write_phy(hw, true, L1C_MIIEXT_PCS, true,
> + L1C_MIIEXT_AZCTRL6,
> + L1C_AZCTRL6_L1D2);

... and shift this stuff back to the left.

[...]
> +u16 l1c_reset_pcie(struct alx_hw *hw, bool l0s_en, bool l1_en)
> +{
> + u32 val;
> + u16 val16;
> + u16 ret;
> +
> + /* Workaround for PCI problem when BIOS sets MMRBC incorrectly. */
> + alx_cfg_r16(hw, PCI_COMMAND, &val16);
> + if ((val16 & (PCI_COMMAND_IO |
> + PCI_COMMAND_MEMORY |
> + PCI_COMMAND_MASTER)) == 0 ||
> + (val16 & PCI_COMMAND_INTX_DISABLE) != 0) {
> + val16 = (u16)((val16 | (PCI_COMMAND_IO |
> + PCI_COMMAND_MEMORY |
> + PCI_COMMAND_MASTER))
> + & ~PCI_COMMAND_INTX_DISABLE);
> + alx_cfg_w16(hw, PCI_COMMAND, val16);
> + }

u16 cmd;

#define ALX_PCI_CMD (PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER)

alx_cfg_r16(hw, PCI_COMMAND, &cmd);
if (!(cmd & ALX_PCI_CMD) || (cmd & PCI_COMMAND_INTX_DISABLE)) {
cmd = (cmd | ALX_PCI_CMD) & ~PCI_COMMAND_INTX_DISABLE;
alx_cfg_w16(hw, PCI_COMMAND, cmd);
}

[...]
> +u16 l1c_enable_mac(struct alx_hw *hw, bool en, u16 en_ctrl)
> +{
> + u32 rxq, txq, mac, val;
> + u16 i;
> +
> + alx_mem_r32(hw, L1C_RXQ0, &rxq);
> + alx_mem_r32(hw, L1C_TXQ0, &txq);
> + alx_mem_r32(hw, L1C_MAC_CTRL, &mac);
> +
> + if (en) { /* enable */
> + alx_mem_w32(hw, L1C_RXQ0, rxq | L1C_RXQ0_EN);
> + alx_mem_w32(hw, L1C_TXQ0, txq | L1C_TXQ0_EN);
> + if ((en_ctrl & LX_MACSPEED_1000) != 0) {
> + FIELD_SETL(mac, L1C_MAC_CTRL_SPEED,
> + L1C_MAC_CTRL_SPEED_1000);
> + } else {
> + FIELD_SETL(mac, L1C_MAC_CTRL_SPEED,
> + L1C_MAC_CTRL_SPEED_10_100);
> + }

speed = (en_ctrl & LX_MACSPEED_1000) ?
L1C_MAC_CTRL_SPEED_1000 : L1C_MAC_CTRL_SPEED_10_100;

FIELD_SETL(mac, L1C_MAC_CTRL_SPEED(speed);
> +
> + test_set_or_clear(mac, en_ctrl, LX_MACDUPLEX_FULL,
> + L1C_MAC_CTRL_FULLD);
> +
> + /* rx filter */
> + test_set_or_clear(mac, en_ctrl, LX_FLT_PROMISC,
> + L1C_MAC_CTRL_PROMISC_EN);
> + test_set_or_clear(mac, en_ctrl, LX_FLT_MULTI_ALL,
> + L1C_MAC_CTRL_MULTIALL_EN);
> + test_set_or_clear(mac, en_ctrl, LX_FLT_BROADCAST,
> + L1C_MAC_CTRL_BRD_EN);
> + test_set_or_clear(mac, en_ctrl, LX_FLT_DIRECT,
> + L1C_MAC_CTRL_RX_EN);
> + test_set_or_clear(mac, en_ctrl, LX_FC_TXEN,
> + L1C_MAC_CTRL_TXFC_EN);
> + test_set_or_clear(mac, en_ctrl, LX_FC_RXEN,
> + L1C_MAC_CTRL_RXFC_EN);
> + test_set_or_clear(mac, en_ctrl, LX_VLAN_STRIP,
> + L1C_MAC_CTRL_VLANSTRIP);
> + test_set_or_clear(mac, en_ctrl, LX_LOOPBACK,
> + L1C_MAC_CTRL_LPBACK_EN);
> + test_set_or_clear(mac, en_ctrl, LX_SINGLE_PAUSE,
> + L1C_MAC_CTRL_SPAUSE_EN);
> + test_set_or_clear(mac, en_ctrl, LX_ADD_FCS,
> + (L1C_MAC_CTRL_PCRCE | L1C_MAC_CTRL_CRCE));

Loop.

(test_set_or_clear is a macro => code bloat)

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h
> new file mode 100644
> index 0000000..6482bee
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx.h
[...]
> +#define ALX_VLAN_TO_TAG(_vlan, _tag) \
> + do { \
> + _tag = ((((_vlan) >> 8) & 0xFF) | (((_vlan) & 0xFF) << 8)); \
> + } while (0)
> +
> +#define ALX_TAG_TO_VLAN(_tag, _vlan) \
> + do { \
> + _vlan = ((((_tag) >> 8) & 0xFF) | (((_tag) & 0xFF) << 8)) ; \
> + } while (0)


It should be a real inline function.

> +
> +/* Coalescing Message Block */
> +struct coals_msg_block {
> + int test;
> +};
> +
> +
> +#define BAR_0 0
> +
> +#define ALX_DEF_RX_BUF_SIZE 1536
> +#define ALX_MAX_JUMBO_PKT_SIZE (9*1024)
> +#define ALX_MAX_TSO_PKT_SIZE (7*1024)
> +
> +#define ALX_MAX_ETH_FRAME_SIZE ALX_MAX_JUMBO_PKT_SIZE
> +#define ALX_MIN_ETH_FRAME_SIZE 68
> +
> +
> +#define ALX_MAX_RX_QUEUES 8
> +#define ALX_MAX_TX_QUEUES 4
> +#define ALX_MAX_HANDLED_INTRS 5
> +
> +#define ALX_WATCHDOG_TIME (5 * HZ)
> +
> +struct alx_cmb {
> + char name[IFNAMSIZ + 9];
> + void *cmb;
> + dma_addr_t dma;
> +};
> +struct alx_smb {
> + char name[IFNAMSIZ + 9];
> + void *smb;
> + dma_addr_t dma;
> +};

What are these 'name' fields for ?

> +
> +
> +/*
> + * RRD : definition
> + */
> +
> +/* general parameter format of rrd */
> +struct alx_rrdes_general {
> + u32 xsum:16;
> + u32 nor:4; /* number of RFD */
> + u32 si:12; /* start index of rfd-ring */

Bitfields are mildly welcome to say the least.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx_ethtool.c b/drivers/net/ethernet/atheros/alx/alx_ethtool.c
> new file mode 100644
> index 0000000..c044133
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_ethtool.c
[...]
> +#ifdef ETHTOOL_OPS_COMPAT
> +#include "alx_compat_ethtool.c"
> +#endif

This should go.

> +
> +
> +static int alx_get_settings(struct net_device *netdev,
> + struct ethtool_cmd *ecmd)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + struct alx_hw *hw = &adpt->hw;
> + u32 link_speed = hw->link_speed;
> + bool link_up = hw->link_up;
> +
> + ecmd->supported = (SUPPORTED_10baseT_Half |
> + SUPPORTED_10baseT_Full |
> + SUPPORTED_100baseT_Half |
> + SUPPORTED_100baseT_Full |
> + SUPPORTED_Autoneg |
> + SUPPORTED_TP);
> + if (CHK_HW_FLAG(GIGA_CAP))
> + ecmd->supported |= SUPPORTED_1000baseT_Full;
> +
> + ecmd->advertising = ADVERTISED_TP;
> +
> + ecmd->advertising |= ADVERTISED_Autoneg;
> + ecmd->advertising |= hw->autoneg_advertised;
> +
> + ecmd->port = PORT_TP;
> + ecmd->phy_address = 0;
> + ecmd->autoneg = AUTONEG_ENABLE;
> + ecmd->transceiver = XCVR_INTERNAL;
> +
> + if (!in_interrupt()) {

Dead branch. :o(

[...]
> +static int alx_set_settings(struct net_device *netdev,
> + struct ethtool_cmd *ecmd)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + struct alx_hw *hw = &adpt->hw;
> + u32 advertised, old;
> + int error = 0;
> +
> + while (CHK_ADPT_FLAG(1, STATE_RESETTING))
> + msleep(20);

An upper bound would not hurt.

[...]
> +static void alx_set_msglevel(struct net_device *netdev, u32 data)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + adpt->msg_enable = data;

Missing empty line.

> +}
> +
> +
> +static int alx_get_regs_len(struct net_device *netdev)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + struct alx_hw *hw = &adpt->hw;
> + return hw->hwreg_sz * sizeof(32);

Missing empty line.

[...]
> +static void alx_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> +
> + strlcpy(drvinfo->driver, alx_drv_name, sizeof(drvinfo->driver));
> + strlcpy(drvinfo->fw_version, "alx", 32);

No random stuff in fw_version.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx_hwcom.h b/drivers/net/ethernet/atheros/alx/alx_hwcom.h
> new file mode 100644
> index 0000000..d3bd2f1
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_hwcom.h
[...]
> +#define ASHFT31(_x) ((_x) << 31)
> +#define ASHFT30(_x) ((_x) << 30)
> +#define ASHFT29(_x) ((_x) << 29)
> +#define ASHFT28(_x) ((_x) << 28)
> +#define ASHFT27(_x) ((_x) << 27)
> +#define ASHFT26(_x) ((_x) << 26)
> +#define ASHFT25(_x) ((_x) << 25)
> +#define ASHFT24(_x) ((_x) << 24)
> +#define ASHFT23(_x) ((_x) << 23)
> +#define ASHFT22(_x) ((_x) << 22)
> +#define ASHFT21(_x) ((_x) << 21)
> +#define ASHFT20(_x) ((_x) << 20)
> +#define ASHFT19(_x) ((_x) << 19)
> +#define ASHFT18(_x) ((_x) << 18)
> +#define ASHFT17(_x) ((_x) << 17)
> +#define ASHFT16(_x) ((_x) << 16)
> +#define ASHFT15(_x) ((_x) << 15)
> +#define ASHFT14(_x) ((_x) << 14)
> +#define ASHFT13(_x) ((_x) << 13)
> +#define ASHFT12(_x) ((_x) << 12)
> +#define ASHFT11(_x) ((_x) << 11)
> +#define ASHFT10(_x) ((_x) << 10)
> +#define ASHFT9(_x) ((_x) << 9)
> +#define ASHFT8(_x) ((_x) << 8)
> +#define ASHFT7(_x) ((_x) << 7)
> +#define ASHFT6(_x) ((_x) << 6)
> +#define ASHFT5(_x) ((_x) << 5)
> +#define ASHFT4(_x) ((_x) << 4)
> +#define ASHFT3(_x) ((_x) << 3)
> +#define ASHFT2(_x) ((_x) << 2)
> +#define ASHFT1(_x) ((_x) << 1)
> +#define ASHFT0(_x) ((_x) << 0)

It does not save much.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx_main.c b/drivers/net/ethernet/atheros/alx/alx_main.c
> new file mode 100644
> index 0000000..a51c608
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_main.c
[...]
> +int alx_cfg_r16(const struct alx_hw *hw, int reg, u16 *pval)
> +{
> + if (!(hw && hw->adpt && hw->adpt->pdev))
> + return -EINVAL;
> + return pci_read_config_word(hw->adpt->pdev, reg, pval);
> +}
> +
> +
> +int alx_cfg_w16(const struct alx_hw *hw, int reg, u16 val)
> +{
> + if (!(hw && hw->adpt && hw->adpt->pdev))
> + return -EINVAL;
> + return pci_write_config_word(hw->adpt->pdev, reg, val);
> +}

"We don't know the context this code runs from."

[...]
> +static void alx_mem_r16(const struct alx_hw *hw, int reg, u16 *val)
> +{
> + if (unlikely(!hw->link_up))
> + readl(hw->hw_addr + reg);
> + *(u16 *)val = readw(hw->hw_addr + reg);

Useless cast.

[...]
> +void alx_hw_printk(const char *level, const struct alx_hw *hw,
> + const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> +
> + if (hw && hw->adpt && hw->adpt->netdev)
> + __netdev_printk(level, hw->adpt->netdev, &vaf);
> + else
> + printk("%salx_hw: %pV", level, &vaf);
> +
> + va_end(args);
> +}

Designing a new logging facility smells like being unable to figure
the current context.

And the printk does not even use KERN_... :o(

> +
> +
> +/*
> + * alx_validate_mac_addr - Validate MAC address
> + */
> +static int alx_validate_mac_addr(u8 *mac_addr)
> +{
> + int retval = 0;
> +
> + if (mac_addr[0] & 0x01) {
> + printk(KERN_DEBUG "MAC address is multicast\n");
> + retval = -EADDRNOTAVAIL;
> + } else if (mac_addr[0] == 0xff && mac_addr[1] == 0xff) {
> + printk(KERN_DEBUG "MAC address is broadcast\n");
> + retval = -EADDRNOTAVAIL;
> + } else if (mac_addr[0] == 0 && mac_addr[1] == 0 &&
> + mac_addr[2] == 0 && mac_addr[3] == 0 &&
> + mac_addr[4] == 0 && mac_addr[5] == 0) {
> + printk(KERN_DEBUG "MAC address is all zeros\n");
> + retval = -EADDRNOTAVAIL;
> + }
> + return retval;
> +}

Bloat. It should use is_valid_ether_addr.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx_sw.h b/drivers/net/ethernet/atheros/alx/alx_sw.h
> new file mode 100644
> index 0000000..3daa392
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_sw.h
[...]
> +/* mac address length */
> +#define ALX_ETH_LENGTH_OF_ADDRESS 6

ETH_ALEN

> +#define ALX_ETH_LENGTH_OF_HEADER ETH_HLEN

:o(

[...]
> +struct alx_hw {
> + struct alx_adapter *adpt;
> + struct alx_hw_callbacks cbs;
> + u8 __iomem *hw_addr; /* inner register address */
> + u16 pci_venid;
> + u16 pci_devid;
> + u16 pci_sub_devid;
> + u16 pci_sub_venid;
> + u8 pci_revid;

/me feels like reading drivers/net/wireless/rtlwifi

The pci_... values are available through the pci_dev struct. They do not
need to be duplicated (especially those that are not used).

> +
> + bool long_cable;
> + bool aps_en;
> + bool hi_txperf;
> + bool msi_lnkpatch;
> + u32 dma_chnl;
> + u32 hwreg_sz;
> + u32 eeprom_sz;
> +
> + /* PHY parameter */
> + u32 phy_id;
> + u32 autoneg_advertised;
> + u32 link_speed;
> + bool link_up;
> + spinlock_t mdio_lock;
> +
> + /* MAC parameter */
> + enum alx_mac_type mac_type;
> + u8 mac_addr[ALX_ETH_LENGTH_OF_ADDRESS];
> + u8 mac_perm_addr[ALX_ETH_LENGTH_OF_ADDRESS];
> +
> + u32 mtu;

What is wrong with net_device.mtu ?

> + u16 rxstat_reg;
> + u16 rxstat_sz;
> + u16 txstat_reg;
> + u16 txstat_sz;

struct alx_something {
u16 reg;
u16 size;
} rxstat, txstat;

> +
> + u16 tx_prod_reg[4];
> + u16 tx_cons_reg[4];
> + u16 rx_prod_reg[2];
> + u16 rx_cons_reg[2];
> + u64 tpdma[4];
> + u64 rfdma[2];
> + u64 rrdma[2];

Should be dma_addr_t

[...]
> +/* definitions for flags */
> +
> +#define CHK_FLAG_ARRAY(_st, _idx, _type, _flag) \
> + ((_st)->flags[_idx] & (ALX_##_type##_FLAG_##_idx##_##_flag))
> +#define CHK_FLAG(_st, _type, _flag) \
> + ((_st)->flags & (ALX_##_type##_FLAG_##_flag))
> +
> +#define SET_FLAG_ARRAY(_st, _idx, _type, _flag) \
> + ((_st)->flags[_idx] |= (ALX_##_type##_FLAG_##_idx##_##_flag))
> +#define SET_FLAG(_st, _type, _flag) \
> + ((_st)->flags |= (ALX_##_type##_FLAG_##_flag))
> +
> +#define CLI_FLAG_ARRAY(_st, _idx, _type, _flag) \
> + ((_st)->flags[_idx] &= ~(ALX_##_type##_FLAG_##_idx##_##_flag))
> +#define CLI_FLAG(_st, _type, _flag) \
> + ((_st)->flags &= ~(ALX_##_type##_FLAG_##_flag))

These macros do not help navigating through the ALX_HW_FLAG_ defines.

--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Fri, 2012-03-02 at 00:40 +0100, Francois Romieu wrote:
> Stephen Hemminger <shemminger@vyatta.com> :
> [...]
> > Evolution is better. The new driver has lots of new callbacks to handle
> > the fact it is dealing with two different chipsets. Not only that your callbacks
> > are built at runtime which leads to security concerns.
> >
> > There is a reason the two Marvell based drivers (skge and sky2) are
> > different drivers. Having to do extra per-chip callbacks is a clear sign
> > the driver should be split in two.
>
> (arguable)
[]
> > +void alx_hw_printk(const char *level, const struct alx_hw *hw,
> > + const char *fmt, ...)
> > +{
> > + struct va_format vaf;
> > + va_list args;
> > +
> > + va_start(args, fmt);
> > + vaf.fmt = fmt;
> > + vaf.va = &args;
> > +
> > + if (hw && hw->adpt && hw->adpt->netdev)
> > + __netdev_printk(level, hw->adpt->netdev, &vaf);
> > + else
> > + printk("%salx_hw: %pV", level, &vaf);
> > +
> > + va_end(args);
> > +}
>
> Designing a new logging facility smells like being unable to figure
> the current context.
>
> And the printk does not even use KERN_... :o(

Sure it does.

level is the first argument and is emitted as %s

> > +/*
> > + * alx_validate_mac_addr - Validate MAC address
> > + */
> > +static int alx_validate_mac_addr(u8 *mac_addr)
> > +{
> > + int retval = 0;
> > +
> > + if (mac_addr[0] & 0x01) {
> > + printk(KERN_DEBUG "MAC address is multicast\n");
> > + retval = -EADDRNOTAVAIL;
> > + } else if (mac_addr[0] == 0xff && mac_addr[1] == 0xff) {
> > + printk(KERN_DEBUG "MAC address is broadcast\n");
> > + retval = -EADDRNOTAVAIL;
> > + } else if (mac_addr[0] == 0 && mac_addr[1] == 0 &&
> > + mac_addr[2] == 0 && mac_addr[3] == 0 &&
> > + mac_addr[4] == 0 && mac_addr[5] == 0) {
> > + printk(KERN_DEBUG "MAC address is all zeros\n");
> > + retval = -EADDRNOTAVAIL;
> > + }
> > + return retval;
> > +}
>
> Bloat. It should use is_valid_ether_addr.

I sent patches already.
It's also out of order, testing multicast then broadcast
instead of the reverse.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
> From: "Huang, Xiong" <xiong@qca.qualcomm.com>
> Date: Wed, 29 Feb 2012 03:11:14 +0000
>
>> We understand your concern.  To support the new chipset, do you
>> think it's reasonable to upstream it as a new driver, not a
>> replacement ?
>
> It depends upon how similar the chips are.

OK.

> To be honest tg3, as one example, supports quite a large array of
> different pieces of hardware that use the same logical core.

At certain point it becomes a pain in the ass to support older
chipsets, and simply easier to leave the older driver to rot.

> So that would be my litmus test about how different a chip needs
> to be to deserve an entirely new driver.

Understood.

> I strongly suggest you try to get atl1c working properly.

This is what I have recommended since the alx driver was rejected, and
our team has been working on the atl1c driver now but we need to get
the legal approval to get contributions out to atl1c (I know this is
silly, but hey just letting you know).

Anyway, in the meantime another topic has creeped up, and that is to
share with BSD and also help kill proprietary drivers [0]. Me and
Adrian intend on sharing our thoughts on how we intend on doing this
at LF collab but was in hopes we can use alx as a test case. We've
gone back to the drawing board for another simple driver to test our
work against but... we come back to alx.

Would it be worthwhile to consider alx upstream only for the newer
chipsets (regardless of the litmus test, which I do agree with on
technical grounds) in consideration for helping pave the way on
killing proprietary drivers?

[0] https://events.linuxfoundation.org/events/collaboration-summit/rodriguez-chadd

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
Luis R. Rodriguez <rodrigue@qca.qualcomm.com> :
> On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
[...]
> > To be honest tg3, as one example, supports quite a large array of
> > different pieces of hardware that use the same logical core.
>
> At certain point it becomes a pain in the ass to support older
> chipsets, and simply easier to leave the older driver to rot.

I would avoid saying such things while trying to sell a plan for a bright
future of drivers maintained and supported by $BIGCORP. :o)

[...]
> Would it be worthwhile to consider alx upstream only for the newer
> chipsets (regardless of the litmus test, which I do agree with on
> technical grounds) in consideration for helping pave the way on
> killing proprietary drivers?

What is the situation regarding the availability for public use of
programming manuals and errata on older chipsets ?

I am fine with Qualcomm being completely uninterested in maintaining
code for old chipsets and dedicating manpower or $$$ on it, be it now,
tomorrow or after a new arrival of management execs. However it sends
a bad message if code for new chipsets comes in while users have to
maintain their pile of poo in the dark.

The hardware stays for years. It's one of the engineering problems of
the day. Killing proprietary drivers is an interesting goal but it is
far, far away.

Qualcomm should imvho meet davem's remarks with more short termed
deliverables.

--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Thu, Mar 22, 2012 at 2:27 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Luis R. Rodriguez <rodrigue@qca.qualcomm.com> :
>> On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
> [...]
>> > To be honest tg3, as one example, supports quite a large array of
>> > different pieces of hardware that use the same logical core.
>>
>> At certain point it becomes a pain in the ass to support older
>> chipsets, and simply easier to leave the older driver to rot.
>
> I would avoid saying such things while trying to sell a plan for a bright
> future of drivers maintained and supported by $BIGCORP. :o)

My statement was more from a resource dedication point of view, but
yes, I agree with your concern. I want to clarify that Atheros !=
Qualcomm. Atheros is now known as Qualcomm Atheros and what we do is
different than Qualcomm. Atheros proactively works upstream as a high
priority. Addressing legacy chipsets support is IMHO a definite
requirement when any $BIGCORP wants to work on addressing support
upstream in the Linux kernel (or any kernel).

> [...]
>> Would it be worthwhile to consider alx upstream only for the newer
>> chipsets (regardless of the litmus test, which I do agree with on
>> technical grounds) in consideration for helping pave the way on
>> killing proprietary drivers?
>
> What is the situation regarding the availability for public use of
> programming manuals and errata on older chipsets ?

Atheros has provided documentation for at least 802.11 chipsets for
older and even newer chipsets to interested active community
developers, to the extent we have also even released GPLv2 open
firmware through ar9170.fw [0] which resulted in fork of that firmware
carl9170.fw [1] and new shiny driver carl9170 [2].

The way we work with the community on providing documentation has been
on a case by case basis and documentation has also been evaluated as
such. Over the last year we have been formalizing this process a bit
further and trying to categorize documentation moving forward so that
it is easier for engineering groups for different types of
technologies to work with the community and enable the community as
best as possible. We now have a program called, the QCA Developer
Program, not documented anywhere but in practice already used widely
by QCA, which has for instance, already enabled quite a bit of DFS
code upstream for ath9k, and a few developers are enabled for other
things, one of which we hope is to enable development of open firmware
for ath9k_htc [3] to set the precedent of success for it even further.

So -- for Ethernet I see it not different, lets leave history behind
and focus on enabling developers as best as possible. If there is
documentation available internally we should revise it, categorize it
and see to it that we enable developers as best as possible, in
whatever way we can.

> I am fine with Qualcomm being completely uninterested in maintaining
> code for old chipsets and dedicating manpower or $$$ on it, be it now,
> tomorrow or after a new arrival of management execs. However it sends
> a bad message if code for new chipsets comes in while users have to
> maintain their pile of poo in the dark.

Agreed 100%

> The hardware stays for years. It's one of the engineering problems of
> the day.

Sure.

> Killing proprietary drivers is an interesting goal but it is
> far, far away.

This is why progress isn't made. Talk is cheap. Lets just get it done.

> Qualcomm should imvho meet davem's remarks with more short termed
> deliverables.

I don't disagree based on technical grounds, what I stated was more as
a review in consideration of goals of sharing code. I did review the
possibility of sticking to atl1c to help with the evolutionary changes
there instead and while I believe that is the right technical
approach, legally from a sharing point of view, the atl1c is
derivative works of the Intel e1000 driver and the e1000 driver has
changes even before git days of the kernel. Relicensing atl1c to ISC
license, which is another option, therefore extremely difficult, but
one option I did review.

Either way, I am happy we follow the general direction given, I just
wanted to make one last point on using alx based on sharing objectives
and as a good simple technical use case of trying to help kill
proprietary drivers for good. I'm fine with alx not being the pivotal
point for us but given Ethernet's simplicity it seems ideal to
consider given the objectives and I felt compelled to address that one
last point given the small set of other options we have.

[0] http://wireless.kernel.org/en/users/Drivers/ar9170.fw
[1] http://wireless.kernel.org/en/users/Drivers/carl9170.fw
[2] http://wireless.kernel.org/en/users/Drivers/carl9170
[3] http://wireless.kernel.org/en/developers/GSoC/2012/ath9k_htc_open_firmware

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Wed, Mar 21, 2012 at 6:28 PM, Luis R. Rodriguez
<rodrigue@qca.qualcomm.com> wrote:
> On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
>> From: "Huang, Xiong" <xiong@qca.qualcomm.com>
>> Date: Wed, 29 Feb 2012 03:11:14 +0000
>>
>>> We understand your concern.  To support the new chipset, do you
>>> think it's reasonable to upstream it as a new driver, not a
>>> replacement ?
>>
>> It depends upon how similar the chips are.
>
> OK.
>
>> To be honest tg3, as one example, supports quite a large array of
>> different pieces of hardware that use the same logical core.
>
> At certain point it becomes a pain in the ass to support older
> chipsets, and simply easier to leave the older driver to rot.
>
>> So that would be my litmus test about how different a chip needs
>> to be to deserve an entirely new driver.
>
> Understood.
>
>> I strongly suggest you try to get atl1c working properly.
>
> This is what I have recommended since the alx driver was rejected, and
> our team has been working on the atl1c driver now but we need to get
> the legal approval to get contributions out to atl1c (I know this is
> silly, but hey just letting you know).
>
> Anyway, in the meantime another topic has creeped up, and that is to
> share with BSD and also help kill proprietary drivers [0]. Me and
> Adrian intend on sharing our thoughts on how we intend on doing this
> at LF collab but was in hopes we can use alx as a test case. We've
> gone back to the drawing board for another simple driver to test our
> work against but... we come back to alx.
>
> Would it be worthwhile to consider alx upstream only for the newer
> chipsets (regardless of the litmus test, which I do agree with on
> technical grounds) in consideration for helping pave the way on
> killing proprietary drivers?
>
> [0] https://events.linuxfoundation.org/events/collaboration-summit/rodriguez-chadd

David, please us know if you think the above reason is worthy for
consideration of alx upstream, if at least for the newer chipsets. Our
engineers are ready to work on either approach, and we have a approval
take either route now. The sharing benefits however would make this a
great case to work on for the above mentioned project.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Fri, Mar 30, 2012 at 11:16 AM, Dave Taht <dave.taht@gmail.com> wrote:
>
>
> On Wed, Feb 29, 2012 at 1:50 AM, Luis R. Rodriguez
> <rodrigue@qca.qualcomm.com> wrote:
>>
>> From: Luis R. Rodriguez <mcgrof@frijolero.org>
>>
>> The next patch adds the new QCA alx Ethernet driver that
>> supercedes the atl1c Ethernet driver. For details please
>> read the commit log of the patch. Given the size you can
>> download the patch from:
>
>
>
>>
>>
>>
>> http://bombadil.infradead.org/~mcgrof/2012/02/28/add-alx-next-20120228.patch
>> sha1sum: 8a8f7b6f1cbe737e70ec3b3eda483a6925fd9bd6
>>
>
> The shiny graph was rather impressive vs a vs the old driver.
>
> https://www.linuxfoundation.org/sites/main/files/alx-iperf.jpg
>
> A quick grep showed no BQL support. :(

Good point, we'll add that to our TODO list.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
Date: Fri, 30 Mar 2012 10:10:55 -0700

> On Wed, Mar 21, 2012 at 6:28 PM, Luis R. Rodriguez
> <rodrigue@qca.qualcomm.com> wrote:
>> On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
>> Would it be worthwhile to consider alx upstream only for the newer
>> chipsets (regardless of the litmus test, which I do agree with on
>> technical grounds) in consideration for helping pave the way on
>> killing proprietary drivers?
...
> David, please us know if you think the above reason is worthy for
> consideration of alx upstream, if at least for the newer chipsets.

Submitting alx that only supports the newer chipsets is fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add QCA alx Ethernet driver [ In reply to ]
On Fri, Mar 30, 2012 at 04:45:54PM -0400, David Miller wrote:
> From: "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com>
> Date: Fri, 30 Mar 2012 10:10:55 -0700
>
> > On Wed, Mar 21, 2012 at 6:28 PM, Luis R. Rodriguez
> > <rodrigue@qca.qualcomm.com> wrote:
> >> On Tue, Feb 28, 2012 at 7:32 PM, David Miller <davem@davemloft.net> wrote:
> >> Would it be worthwhile to consider alx upstream only for the newer
> >> chipsets (regardless of the litmus test, which I do agree with on
> >> technical grounds) in consideration for helping pave the way on
> >> killing proprietary drivers?
> ...
> > David, please us know if you think the above reason is worthy for
> > consideration of alx upstream, if at least for the newer chipsets.
>
> Submitting alx that only supports the newer chipsets is fine.

We will work on this and resubmit, the delta for the other chipsets
can also be addressed within atl1c, in addition to working on getting
documentation out to help enable further.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/