diff mbox

[04/37] atl1c: remove VPD register

Message ID 1334276102-15866-5-git-send-email-xiong@qca.qualcomm.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Huang, Xiong April 13, 2012, 12:14 a.m. UTC 
VPD register is only used for L1(devid=PCI_DEVICE_ID_ATTANSIC_L1) to
access external NV-memory.
l1c & later chip doesn't use it any more.

Signed-off-by: xiong <xiong@qca.qualcomm.com>
Tested-by: Liu David <dwliu@qca.qualcomm.com>
---
 drivers/net/ethernet/atheros/atl1c/atl1c_ethtool.c |    1 -
 drivers/net/ethernet/atheros/atl1c/atl1c_hw.h      |   11 -----------
 2 files changed, 0 insertions(+), 12 deletions(-)

Comments

David Miller April 14, 2012, 12:45 a.m. UTC | #1 
From: xiong <xiong@qca.qualcomm.com>
Date: Fri, 13 Apr 2012 08:14:29 +0800

> VPD register is only used for L1(devid=PCI_DEVICE_ID_ATTANSIC_L1) to
> access external NV-memory.
> l1c & later chip doesn't use it any more.
> 
> Signed-off-by: xiong <xiong@qca.qualcomm.com>
> Tested-by: Liu David <dwliu@qca.qualcomm.com>

You just broke ethtool register dumps with this change.

Now, all the initial registers are reported offset by one entry, yet
the last two are still reported in their original spots.

This layout is exposed to userspace, and interpreted by tools, and you
cannot change it.

If this register always reports some value, you should just keep it
there in the dumps.

I think you guys are way too aggressively removing things from the
driver.

You are also posting way too many patches at one time.

If you post so many patches at once, if one of the early patches
have a problem (as already is the case here) it means all the
rest of your patches will not apply without offsets or rejects
and therefore will all need to be redone and resubmitted.

Posting too many patches at once also creates a large burdon for
those who will choose to help review your changes.

Send a small, reasonable, number of patches at one time.  Say,
for example, 10 or 15 maximum.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Huang, Xiong April 14, 2012, 9:12 a.m. UTC | #2 
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, April 14, 2012 8:46
> To: Huang, Xiong
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; qca-linux-team; nic-
> devel
> Subject: Re: [PATCH 04/37] atl1c: remove VPD register
> 
> From: xiong <xiong@qca.qualcomm.com>
> Date: Fri, 13 Apr 2012 08:14:29 +0800
> 
> > VPD register is only used for L1(devid=PCI_DEVICE_ID_ATTANSIC_L1) to
> > access external NV-memory.
> > l1c & later chip doesn't use it any more.
> >
> > Signed-off-by: xiong <xiong@qca.qualcomm.com>
> > Tested-by: Liu David <dwliu@qca.qualcomm.com>
> 
> You just broke ethtool register dumps with this change.
> 
> Now, all the initial registers are reported offset by one entry, yet the last two are
> still reported in their original spots.
> 
> This layout is exposed to userspace, and interpreted by tools, and you cannot
> change it.
> 
> If this register always reports some value, you should just keep it there in the
> dumps.
>
The VPD register doesn't report anything :(, just a dummy register now.
we don't have any special tools in userspace to explain the dumped registers.
Actually the purpose of dumping these registers via ethtool is just for debug.


> I think you guys are way too aggressively removing things from the driver.
> 
> You are also posting way too many patches at one time.
> 
> If you post so many patches at once, if one of the early patches have a problem
> (as already is the case here) it means all the rest of your patches will not apply
> without offsets or rejects and therefore will all need to be redone and
> resubmitted.
> 
> Posting too many patches at once also creates a large burdon for those who will
> choose to help review your changes.
> 
> Send a small, reasonable, number of patches at one time.  Say, for example, 10
> or 15 maximum.

Completely agree.


Thanks
Xiong


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings April 14, 2012, 2:22 p.m. UTC | #3 
On Sat, 2012-04-14 at 09:12 +0000, Huang, Xiong wrote:
> 
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Saturday, April 14, 2012 8:46
> > To: Huang, Xiong
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; qca-linux-team; nic-
> > devel
> > Subject: Re: [PATCH 04/37] atl1c: remove VPD register
> > 
> > From: xiong <xiong@qca.qualcomm.com>
> > Date: Fri, 13 Apr 2012 08:14:29 +0800
> > 
> > > VPD register is only used for L1(devid=PCI_DEVICE_ID_ATTANSIC_L1) to
> > > access external NV-memory.
> > > l1c & later chip doesn't use it any more.
> > >
> > > Signed-off-by: xiong <xiong@qca.qualcomm.com>
> > > Tested-by: Liu David <dwliu@qca.qualcomm.com>
> > 
> > You just broke ethtool register dumps with this change.
> > 
> > Now, all the initial registers are reported offset by one entry, yet the last two are
> > still reported in their original spots.
> > 
> > This layout is exposed to userspace, and interpreted by tools, and you cannot
> > change it.
> > 
> > If this register always reports some value, you should just keep it there in the
> > dumps.
> >
> The VPD register doesn't report anything :(, just a dummy register now.
> we don't have any special tools in userspace to explain the dumped registers.
> Actually the purpose of dumping these registers via ethtool is just for debug.
[...]

Of course, the whole purpose of the operation is for debugging.  But you
should bump the dump version number (currently 0) every time you change
the offsets of registers in the dump.

Ben.
David Miller April 14, 2012, 6:24 p.m. UTC | #4 
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 14 Apr 2012 15:22:49 +0100

> On Sat, 2012-04-14 at 09:12 +0000, Huang, Xiong wrote:
>> 
>> > -----Original Message-----
>> > From: David Miller [mailto:davem@davemloft.net]
>> > Sent: Saturday, April 14, 2012 8:46
>> > To: Huang, Xiong
>> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; qca-linux-team; nic-
>> > devel
>> > Subject: Re: [PATCH 04/37] atl1c: remove VPD register
>> > 
>> > From: xiong <xiong@qca.qualcomm.com>
>> > Date: Fri, 13 Apr 2012 08:14:29 +0800
>> > 
>> > > VPD register is only used for L1(devid=PCI_DEVICE_ID_ATTANSIC_L1) to
>> > > access external NV-memory.
>> > > l1c & later chip doesn't use it any more.
>> > >
>> > > Signed-off-by: xiong <xiong@qca.qualcomm.com>
>> > > Tested-by: Liu David <dwliu@qca.qualcomm.com>
>> > 
>> > You just broke ethtool register dumps with this change.
>> > 
>> > Now, all the initial registers are reported offset by one entry, yet the last two are
>> > still reported in their original spots.
>> > 
>> > This layout is exposed to userspace, and interpreted by tools, and you cannot
>> > change it.
>> > 
>> > If this register always reports some value, you should just keep it there in the
>> > dumps.
>> >
>> The VPD register doesn't report anything :(, just a dummy register now.
>> we don't have any special tools in userspace to explain the dumped registers.
>> Actually the purpose of dumping these registers via ethtool is just for debug.
> [...]
> 
> Of course, the whole purpose of the operation is for debugging.  But you
> should bump the dump version number (currently 0) every time you change
> the offsets of registers in the dump.

And also update the size, and not leave any holes between the actually
used register slots, etc.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_ethtool.c b/drivers/net/ethernet/atheros/atl1c/atl1c_ethtool.c
index 0a9326a..ce63615 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_ethtool.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_ethtool.c
@@ -142,7 +142,6 @@  static void atl1c_get_regs(struct net_device *netdev,
 	memset(p, 0, AT_REGS_LEN);
 
 	regs->version = 0;
-	AT_READ_REG(hw, REG_VPD_CAP, 		  p++);
 	AT_READ_REG(hw, REG_PM_CTRL, 		  p++);
 	AT_READ_REG(hw, REG_MAC_HALF_DUPLX_CTRL,  p++);
 	AT_READ_REG(hw, REG_TWSI_CTRL, 		  p++);
diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_hw.h b/drivers/net/ethernet/atheros/atl1c/atl1c_hw.h
index c22d698..f5c7473 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_hw.h
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_hw.h
@@ -59,17 +59,6 @@  int atl1c_phy_power_saving(struct atl1c_hw *hw);
 #define LINK_CTRL_L1_EN			0x02
 #define LINK_CTRL_EXT_SYNC		0x80
 
-#define REG_VPD_CAP			0x6C
-#define VPD_CAP_ID_MASK                 0xff
-#define VPD_CAP_ID_SHIFT                0
-#define VPD_CAP_NEXT_PTR_MASK           0xFF
-#define VPD_CAP_NEXT_PTR_SHIFT          8
-#define VPD_CAP_VPD_ADDR_MASK           0x7FFF
-#define VPD_CAP_VPD_ADDR_SHIFT          16
-#define VPD_CAP_VPD_FLAG                0x80000000
-
-#define REG_VPD_DATA                	0x70
-
 #define REG_PCIE_UC_SEVERITY		0x10C
 #define PCIE_UC_SERVRITY_TRN		0x00000001
 #define PCIE_UC_SERVRITY_DLP		0x00000010