Re: [PATCH] - e1000_ethtool.c - convert macros to functions

view
thread

!MAILaRCHIVE_VOTE_RePLACE

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

[view in full thread]

From: Kok, Auke <auke-jan.h.kok@...>

To: Joe Perches <joe@...>

Cc: netdev <netdev@...>, <e1000-devel@...>, Auke Kok <auke-jan.h.kok@...>, Jeff Garzik <jgarzik@...>

Subject: Re: [PATCH] - e1000_ethtool.c - convert macros to functions

Date: Wednesday, October 31, 2007 - 5:30 pm

Joe Perches wrote:
quoted text> Convert REG_PATTERN_TEST and REG_SET_AND_CHECK macros to functions
> Reduces x86 defconfig image by about 3k
> 
> compiled, untested (no hardware)
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> 
> New:
> 
> $ size vmlinux
>    text    data     bss     dec     hex filename
> 4792735  490626  606208 5889569  59de21 vmlinux
> 
> Current:
> 
> $ size vmlinux
>    text    data     bss     dec     hex filename
> 4795759  490626  606208 5892593  59e9f1 vmlinux
> 
> ---
> 
>  drivers/net/e1000/e1000_ethtool.c |  185 +++++++++++++++++++++++++------------
>  drivers/net/e1000/e1000_osdep.h   |   42 +++++----
>  2 files changed, 149 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
> index 667f18b..2627395 100644
> --- a/drivers/net/e1000/e1000_ethtool.c
> +++ b/drivers/net/e1000/e1000_ethtool.c
> @@ -728,37 +728,45 @@ err_setup:
>  	return err;
>  }
>  
> -#define REG_PATTERN_TEST(R, M, W)                                              \
> -{                                                                              \
> -	uint32_t pat, val;                                                     \
> -	const uint32_t test[] = 					       \
> -		{0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF};              \
> -	for (pat = 0; pat < ARRAY_SIZE(test); pat++) {           	       \
> -		E1000_WRITE_REG(&adapter->hw, R, (test[pat] & W));             \
> -		val = E1000_READ_REG(&adapter->hw, R);                         \
> -		if (val != (test[pat] & W & M)) {                              \
> -			DPRINTK(DRV, ERR, "pattern test reg %04X failed: got " \
> -			        "0x%08X expected 0x%08X\n",                    \
> -			        E1000_##R, val, (test[pat] & W & M));          \
> -			*data = (adapter->hw.mac_type < e1000_82543) ?         \
> -				E1000_82542_##R : E1000_##R;                   \
> -			return 1;                                              \
> -		}                                                              \
> -	}                                                                      \
> +static bool reg_pattern_test(struct e1000_adapter *adapter, uint64_t *data,
> +			     int reg, uint32_t mask, uint32_t write)
> +{
> +	static const uint32_t test[] =
> +		{0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF};
> +	uint8_t __iomem *address = adapter->hw.hw_addr + reg;
> +	uint32_t read;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(test); i++) {
> +		writel(write & test[i], address);
> +		read = readl(address);
> +		if (read != (write & test[i] & mask)) {
> +			DPRINTK(DRV, ERR, "pattern test reg %04X failed: "
> +				"got 0x%08X expected 0x%08X\n",
> +			        reg, read, (write & test[i] & mask));
> +			*data = reg;
> +			return true;
> +		}
> +	}
> +	return false;

that's not a bad idea, however see below:

quoted text>  }
>  
> -#define REG_SET_AND_CHECK(R, M, W)                                             \
> -{                                                                              \
> -	uint32_t val;                                                          \
> -	E1000_WRITE_REG(&adapter->hw, R, W & M);                               \
> -	val = E1000_READ_REG(&adapter->hw, R);                                 \
> -	if ((W & M) != (val & M)) {                                            \
> -		DPRINTK(DRV, ERR, "set/check reg %04X test failed: got 0x%08X "\
> -		        "expected 0x%08X\n", E1000_##R, (val & M), (W & M));   \
> -		*data = (adapter->hw.mac_type < e1000_82543) ?                 \
> -			E1000_82542_##R : E1000_##R;                           \
> -		return 1;                                                      \
> -	}                                                                      \
> +static bool reg_set_and_check(struct e1000_adapter *adapter, uint64_t *data,
> +			      int reg, uint32_t mask, uint32_t write)
> +{
> +	uint8_t __iomem *address = adapter->hw.hw_addr + reg;
> +	uint32_t read;
> +
> +	writel(write & mask, address);
> +	read = readl(address);
> +	if ((read & mask) != (write & mask)) {
> +		DPRINTK(DRV, ERR, "set/check reg %04X test failed: "
> +			"got 0x%08X expected 0x%08X\n",
> +			reg, (read & mask), (write & mask));
> +		*data = reg;
> +		return true;
> +	}
> +	return false;
>  }
>  
>  static int
> @@ -800,58 +808,115 @@ e1000_reg_test(struct e1000_adapter *adapter, uint64_t *data)
>  	E1000_WRITE_REG(&adapter->hw, STATUS, before);
>  
>  	if (adapter->hw.mac_type != e1000_ich8lan) {
> -		REG_PATTERN_TEST(FCAL, 0xFFFFFFFF, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(FCAH, 0x0000FFFF, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(FCT, 0x0000FFFF, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(VET, 0x0000FFFF, 0xFFFFFFFF);
> +		if (reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, FCAL),
> +				     0xFFFFFFFF, 0xFFFFFFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, FCAH),
> +				     0x0000FFFF, 0xFFFFFFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, FCT),
> +				     0x0000FFFF, 0xFFFFFFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, VET),
> +				     0x0000FFFF, 0xFFFFFFFF))
> +			return 1;

can't we keep the macro here (and just make it call the function instead of
expanding). the resulting code is much more lenghty and contains all these logic
traps that the previous code didn't have.

just have the macro expand to `if (reg_pattern_test(...)) return 1)` and you don't
need to change any of the calling lines.

quoted text>  	}
>  
> -	REG_PATTERN_TEST(RDTR, 0x0000FFFF, 0xFFFFFFFF);
> -	REG_PATTERN_TEST(RDBAH, 0xFFFFFFFF, 0xFFFFFFFF);
> -	REG_PATTERN_TEST(RDLEN, 0x000FFF80, 0x000FFFFF);
> -	REG_PATTERN_TEST(RDH, 0x0000FFFF, 0x0000FFFF);
> -	REG_PATTERN_TEST(RDT, 0x0000FFFF, 0x0000FFFF);
> -	REG_PATTERN_TEST(FCRTH, 0x0000FFF8, 0x0000FFF8);
> -	REG_PATTERN_TEST(FCTTV, 0x0000FFFF, 0x0000FFFF);
> -	REG_PATTERN_TEST(TIPG, 0x3FFFFFFF, 0x3FFFFFFF);
> -	REG_PATTERN_TEST(TDBAH, 0xFFFFFFFF, 0xFFFFFFFF);
> -	REG_PATTERN_TEST(TDLEN, 0x000FFF80, 0x000FFFFF);
> +	if (reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, RDTR),
> +			     0x0000FFFF, 0xFFFFFFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, RDBAH),
> +			     0xFFFFFFFF, 0xFFFFFFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, RDLEN),
> +			     0x000FFF80, 0x000FFFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, RDH),
> +			     0x0000FFFF, 0x0000FFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, RDT),
> +			     0x0000FFFF, 0x0000FFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, FCRTH),
> +			     0x0000FFF8, 0x0000FFF8) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, FCTTV),
> +			     0x0000FFFF, 0x0000FFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, TIPG),
> +			     0x3FFFFFFF, 0x3FFFFFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, TDBAH),
> +			     0xFFFFFFFF, 0xFFFFFFFF) ||
> +	    reg_pattern_test(adapter, data, E1000_REG(&adapter->hw, TDLEN),
> +			     0x000FFF80, 0x000FFFFF))
> +		return 1;
>  
> -	REG_SET_AND_CHECK(RCTL, 0xFFFFFFFF, 0x00000000);
> +	if (reg_set_and_check(adapter, data, E1000_REG(&adapter->hw, RCTL),
> +			      0xFFFFFFFF, 0x00000000))
> +		return 1;
>  
>  	before = (adapter->hw.mac_type == e1000_ich8lan ?
>  	          0x06C3B33E : 0x06DFB3FE);
> -	REG_SET_AND_CHECK(RCTL, before, 0x003FFFFB);
> -	REG_SET_AND_CHECK(TCTL, 0xFFFFFFFF, 0x00000000);
> +	if (reg_set_and_check(adapter, data, E1000_REG(&adapter->hw, RCTL),
> +			      before, 0x003FFFFB) ||
> +	    reg_set_and_check(adapter, data, E1000_REG(&adapter->hw, TCTL),
> +			      0xFFFFFFFF, 0x00000000))
> +		return 1;
>  
>  	if (adapter->hw.mac_type >= e1000_82543) {
>  
> -		REG_SET_AND_CHECK(RCTL, before, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(RDBAL, 0xFFFFFFF0, 0xFFFFFFFF);
> -		if (adapter->hw.mac_type != e1000_ich8lan)
> -			REG_PATTERN_TEST(TXCW, 0xC000FFFF, 0x0000FFFF);
> -		REG_PATTERN_TEST(TDBAL, 0xFFFFFFF0, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(TIDV, 0x0000FFFF, 0x0000FFFF);
> +		if (reg_set_and_check(adapter, data,
> +				      E1000_REG(&adapter->hw, RCTL),
> +				      before, 0xFFFFFFFF))
> +			return 1;
> +		if (reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, RDBAL),
> +				     0xFFFFFFF0, 0xFFFFFFFF))
> +			return 1;
> +		if (adapter->hw.mac_type != e1000_ich8lan) {
> +			if (reg_pattern_test(adapter, data,
> +					     E1000_REG(&adapter->hw, TXCW),
> +					     0xC000FFFF, 0x0000FFFF))
> +				return 1;
> +		}
> +		if (reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, TDBAL),
> +				     0xFFFFFFF0, 0xFFFFFFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, TIDV),
> +				     0x0000FFFF, 0x0000FFFF))
> +			return 1;
>  		value = (adapter->hw.mac_type == e1000_ich8lan ?
>  		         E1000_RAR_ENTRIES_ICH8LAN : E1000_RAR_ENTRIES);
>  		for (i = 0; i < value; i++) {
> -			REG_PATTERN_TEST(RA + (((i << 1) + 1) << 2), 0x8003FFFF,
> -			                 0xFFFFFFFF);
> +			if (reg_pattern_test(adapter, data,
> +					     E1000_REG(&adapter->hw, RA) +
> +					     (((i << 1) + 1) << 2),
> +					     0x8003FFFF, 0xFFFFFFFF))
> +				return 1;
>  		}
>  
>  	} else {
>  
> -		REG_SET_AND_CHECK(RCTL, 0xFFFFFFFF, 0x01FFFFFF);
> -		REG_PATTERN_TEST(RDBAL, 0xFFFFF000, 0xFFFFFFFF);
> -		REG_PATTERN_TEST(TXCW, 0x0000FFFF, 0x0000FFFF);
> -		REG_PATTERN_TEST(TDBAL, 0xFFFFF000, 0xFFFFFFFF);
> +		if (reg_set_and_check(adapter, data,
> +				      E1000_REG(&adapter->hw, RCTL),
> +				      0xFFFFFFFF, 0x01FFFFFF))
> +			return 1;
> +		if (reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, RDBAL),
> +				     0xFFFFF000, 0xFFFFFFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, TXCW),
> +				     0x0000FFFF, 0x0000FFFF) ||
> +		    reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, TDBAL),
> +				     0xFFFFF000, 0xFFFFFFFF))
> +			return 1;
>  
>  	}
>  
>  	value = (adapter->hw.mac_type == e1000_ich8lan ?
>  			E1000_MC_TBL_SIZE_ICH8LAN : E1000_MC_TBL_SIZE);
> -	for (i = 0; i < value; i++)
> -		REG_PATTERN_TEST(MTA + (i << 2), 0xFFFFFFFF, 0xFFFFFFFF);
> +	for (i = 0; i < value; i++) {
> +		if (reg_pattern_test(adapter, data,
> +				     E1000_REG(&adapter->hw, MTA) + (i << 2),
> +				     0xFFFFFFFF, 0xFFFFFFFF))
> +			return 1;
> +	}
>  
>  	*data = 0;
>  	return 0;
> diff --git a/drivers/net/e1000/e1000_osdep.h b/drivers/net/e1000/e1000_osdep.h
> index 10af742..7d70eb6 100644
> --- a/drivers/net/e1000/e1000_osdep.h
> +++ b/drivers/net/e1000/e1000_osdep.h
> @@ -61,24 +61,30 @@ typedef enum {
>  #define DEBUGOUT3 DEBUGOUT2
>  #define DEBUGOUT7 DEBUGOUT3
>  
> -
> -#define E1000_WRITE_REG(a, reg, value) ( \
> -    writel((value), ((a)->hw_addr + \
> -        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg))))
> -
> -#define E1000_READ_REG(a, reg) ( \
> -    readl((a)->hw_addr + \
> -        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg)))
> -
> -#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) ( \
> -    writel((value), ((a)->hw_addr + \
> -        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg) + \
> -        ((offset) << 2))))
> -
> -#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
> -    readl((a)->hw_addr + \
> -        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg) + \
> -        ((offset) << 2)))
> +#define E1000_REG(a, reg) \
> +	(((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg)
> +
> +#define E1000_WRITE_REG(a, reg, value)					\
> +	(writel((value), ((a)->hw_addr +				\
> +			  (((a)->mac_type >= e1000_82543)		\
> +			   ? E1000_##reg : E1000_82542_##reg))))
> +
> +#define E1000_READ_REG(a, reg)				\
> +	(readl((a)->hw_addr +				\
> +	       (((a)->mac_type >= e1000_82543)		\
> +		? E1000_##reg : E1000_82542_##reg)))
> +
> +#define E1000_WRITE_REG_ARRAY(a, reg, offset, value)	       \
> +	(writel((value), ((a)->hw_addr +		       \
> +			  (((a)->mac_type >= e1000_82543)      \
> +			   ? E1000_##reg : E1000_82542_##reg) +\
> +			  ((offset) << 2))))
> +
> +#define E1000_READ_REG_ARRAY(a, reg, offset)		\
> +	(readl((a)->hw_addr +				\
> +	       (((a)->mac_type >= e1000_82543)		\
> +		? E1000_##reg : E1000_82542_##reg) +	\
> +	       ((offset) << 2)))

did you have to change these macro's ?


also, I'm a bit inclined to prefer a patch for e1000e for now as we're about to
move the pci-express hardware over, but we can certainly merge something like this
in e1000 after the move as well.


Auke
-
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

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:

[PATCH] - e1000_ethtool.c - convert macros to functions, Joe Perches, (Wed Oct 31, 5:18 pm)

Re: [PATCH] - e1000_ethtool.c - convert macros to functions, Kok, Auke, (Wed Oct 31, 5:30 pm)

Re: [PATCH] - e1000_ethtool.c - convert macros to functions, Joe Perches, (Wed Oct 31, 8:34 pm)

[PATCH] - e1000e/ethtool.c - convert macros to functions, Joe Perches, (Wed Oct 31, 11:29 pm)

Re: [PATCH] - e1000e/ethtool.c - convert macros to functions, Kok, Auke, (Thu Nov 1, 12:01 pm)

Re: [PATCH] - e1000_ethtool.c - convert macros to functions, Joe Perches, (Wed Oct 31, 7:15 pm)

Re: [PATCH] - e1000_ethtool.c - convert macros to functions, Kok, Auke, (Wed Oct 31, 8:39 pm)

Navigation

Popular discussions


linux-kernel:
Bart Van Assche	Integration of SCST in the mainstream Linux kernel
Linus Torvalds	Linux 2.6.27-rc5
Jared Hulbert	[PATCH 00/10] AXFS: Advanced XIP filesystem
Linus Torvalds	Linux 2.6.27-rc8
git:

linux-netdev:
David Miller	[GIT]: Networking
Antonio Almeida	HTB accuracy for high speed
Gerrit Renker	[PATCH 27/37] dccp: Integration of dynamic feature activation - part 2 (server side)
David Miller	Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock().
openbsd-misc:

Colocation donated by:

Online users

Syndicate