From: "Kok, Auke" <auke-jan.h.kok@intel.com>
To: Joe Perches <joe@perches.com>
Cc: netdev <netdev@vger.kernel.org>,
e1000-devel@lists.sourceforge.net,
Auke Kok <auke-jan.h.kok@intel.com>,
Jeff Garzik <jgarzik@pobox.com>
Subject: Re: [PATCH] - e1000_ethtool.c - convert macros to functions
Date: Wed, 31 Oct 2007 14:30:09 -0700 [thread overview]
Message-ID: <4728F3E1.1040908@intel.com> (raw)
In-Reply-To: <1193865488.11020.11.camel@localhost>
Joe Perches wrote:
> 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:
> }
>
> -#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.
> }
>
> - 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
next prev parent reply other threads:[~2007-10-31 21:36 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-10-31 21:18 [PATCH] - e1000_ethtool.c - convert macros to functions Joe Perches
2007-10-31 21:30 ` Kok, Auke [this message]
2007-10-31 23:15 ` Joe Perches
2007-11-01 0:39 ` Kok, Auke
2007-11-01 0:34 ` Joe Perches
2007-11-01 3:29 ` [PATCH] - e1000e/ethtool.c " Joe Perches
2007-11-01 16:01 ` Kok, Auke
-- strict thread matches above, loose matches on Subject: below --
2007-11-01 21:16 [PATCH] - e1000_ethtool.c " Joe Perches
2007-11-01 21:29 ` Kok, Auke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4728F3E1.1040908@intel.com \
--to=auke-jan.h.kok@intel.com \
--cc=e1000-devel@lists.sourceforge.net \
--cc=jgarzik@pobox.com \
--cc=joe@perches.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).