From: conley <[email protected]>
- sun4i-emac.h: add register related marcos
- sun4i-emac.c: replace magic number with marco
*** BLURB HERE ***
conley (2):
sun4i-emac.h: add register related marcos
sun4i-emac.c: replace magic number with macro
drivers/net/ethernet/allwinner/sun4i-emac.c | 26 ++++++++++-----------
drivers/net/ethernet/allwinner/sun4i-emac.h | 18 ++++++++++++++
2 files changed, 31 insertions(+), 13 deletions(-)
--
2.31.1
Le Mon, Jan 10, 2022 at 03:23:07PM +0800, [email protected] a ?crit :
> From: conley <[email protected]>
>
> - sun4i-emac.h: add register related marcos
> - sun4i-emac.c: replace magic number with marco
>
> *** BLURB HERE ***
>
> conley (2):
> sun4i-emac.h: add register related marcos
> sun4i-emac.c: replace magic number with macro
>
> drivers/net/ethernet/allwinner/sun4i-emac.c | 26 ++++++++++-----------
> drivers/net/ethernet/allwinner/sun4i-emac.h | 18 ++++++++++++++
> 2 files changed, 31 insertions(+), 13 deletions(-)
>
Hello
The from should be your real name.
You miss commit message.
The subject should be "net: allwinner: xxxx" or "net: ethernet: sun4i-emac: xxxx"
You did a typo marcos/macros
If you add a changelog, either put it in cover letter or below "---" in patch along git stats (changelog should not be part of commit message).
I think both patch should be merged.
Regards
This patch remove magic numbers in sun4i-emac.c and replace with macros
defined in sun4i-emac.h
Change since v1
---------------
- reformat
- merge commits
- add commit message
Signed-off-by: Conley Lee <[email protected]>
---
drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
2 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
index 849de4564709..98fd98feb439 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
@@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
/* set EMAC SPEED, depend on PHY */
reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
- reg_val &= ~(0x1 << 8);
+ reg_val &= ~EMAC_MAC_SUPP_100M;
if (db->speed == SPEED_100)
- reg_val |= 1 << 8;
+ reg_val |= EMAC_MAC_SUPP_100M;
writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
}
@@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
/* re enable interrupt */
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0x01 << 8);
+ reg_val |= EMAC_INT_CTL_RX_EN;
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
db->emacrx_completed_flag = 1;
@@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
/* initial EMAC */
/* flush RX FIFO */
reg_val = readl(db->membase + EMAC_RX_CTL_REG);
- reg_val |= 0x8;
+ reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
writel(reg_val, db->membase + EMAC_RX_CTL_REG);
udelay(1);
@@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
/* set MII clock */
reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
- reg_val &= (~(0xf << 2));
- reg_val |= (0xD << 2);
+ reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
+ reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
/* clear RX counter */
@@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
/* enable RX/TX0/RX Hlevel interrup */
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
spin_unlock_irqrestore(&db->lock, flags);
@@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
if (!rxcount) {
db->emacrx_completed_flag = 1;
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |=
+ (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
+ EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
/* had one stuck? */
@@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
writel(reg_val | EMAC_CTL_RX_EN,
db->membase + EMAC_CTL_REG);
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |=
+ (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
+ EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
db->emacrx_completed_flag = 1;
@@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
}
/* Transmit Interrupt check */
- if (int_status & (0x01 | 0x02))
+ if (int_status & EMAC_INT_STA_TX_COMPLETE)
emac_tx_done(dev, db, int_status);
- if (int_status & (0x04 | 0x08))
+ if (int_status & EMAC_INT_STA_TX_ABRT)
netdev_info(dev, " ab : %x\n", int_status);
/* Re-enable interrupt mask */
if (db->emacrx_completed_flag == 1) {
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
} else {
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
}
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
index 38c72d9ec600..90bd9ad77607 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.h
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
@@ -38,6 +38,7 @@
#define EMAC_RX_CTL_REG (0x3c)
#define EMAC_RX_CTL_AUTO_DRQ_EN (1 << 1)
#define EMAC_RX_CTL_DMA_EN (1 << 2)
+#define EMAC_RX_CTL_FLUSH_FIFO (1 << 3)
#define EMAC_RX_CTL_PASS_ALL_EN (1 << 4)
#define EMAC_RX_CTL_PASS_CTL_EN (1 << 5)
#define EMAC_RX_CTL_PASS_CRC_ERR_EN (1 << 6)
@@ -61,7 +62,21 @@
#define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
#define EMAC_RX_FBC_REG (0x50)
#define EMAC_INT_CTL_REG (0x54)
+#define EMAC_INT_CTL_RX_EN (1 << 8)
+#define EMAC_INT_CTL_TX0_EN (1)
+#define EMAC_INT_CTL_TX1_EN (1 << 1)
+#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
+#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
+#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
+#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
#define EMAC_INT_STA_REG (0x58)
+#define EMAC_INT_STA_TX0_COMPLETE (0x1)
+#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
+#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
+#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
+#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
+#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
+#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
#define EMAC_MAC_CTL0_REG (0x5c)
#define EMAC_MAC_CTL0_RX_FLOW_CTL_EN (1 << 2)
#define EMAC_MAC_CTL0_TX_FLOW_CTL_EN (1 << 3)
@@ -87,8 +102,11 @@
#define EMAC_MAC_CLRT_RM (0x0f)
#define EMAC_MAC_MAXF_REG (0x70)
#define EMAC_MAC_SUPP_REG (0x74)
+#define EMAC_MAC_SUPP_100M (0x1 << 8)
#define EMAC_MAC_TEST_REG (0x78)
#define EMAC_MAC_MCFG_REG (0x7c)
+#define EMAC_MAC_MCFG_MII_CLKD_MASK (0xff << 2)
+#define EMAC_MAC_MCFG_MII_CLKD_72 (0x0d << 2)
#define EMAC_MAC_A0_REG (0x98)
#define EMAC_MAC_A1_REG (0x9c)
#define EMAC_MAC_A2_REG (0xa0)
--
2.31.1
On Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee wrote:
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
>
> Change since v1
> ---------------
> - reformat
> - merge commits
> - add commit message
This changelog should be placed after "---" below your SOB.
Thanks
>
> Signed-off-by: Conley Lee <[email protected]>
> ---
> drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
> drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
> 2 files changed, 35 insertions(+), 13 deletions(-)
> @@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
> if (!rxcount) {
> db->emacrx_completed_flag = 1;
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0) | (0x01 << 8);
> + reg_val |=
> + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> + EMAC_INT_CTL_RX_EN);
Putting the first value on the next line is a bit odd. This would be
preferred:
+ reg_val |= (EMAC_INT_CTL_TX_EN |
+ EMAC_INT_CTL_TX_ABRT_EN |
+ EMAC_INT_CTL_RX_EN);
I also have to wonder why two | have become three? (0x01 << 8) is
clearly a single value. (0xf << 0) should either be a single macro, or
4 macros since 0xf is four bits. Without looking into the details, i
cannot say this is wrong, but it does look strange.
Andrew
Le Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee a ?crit :
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
>
> Change since v1
> ---------------
> - reformat
> - merge commits
> - add commit message
>
> Signed-off-by: Conley Lee <[email protected]>
> ---
> drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
> drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
> 2 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 849de4564709..98fd98feb439 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
>
> /* set EMAC SPEED, depend on PHY */
> reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
> - reg_val &= ~(0x1 << 8);
> + reg_val &= ~EMAC_MAC_SUPP_100M;
> if (db->speed == SPEED_100)
> - reg_val |= 1 << 8;
> + reg_val |= EMAC_MAC_SUPP_100M;
> writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
> }
>
> @@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
>
> /* re enable interrupt */
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0x01 << 8);
> + reg_val |= EMAC_INT_CTL_RX_EN;
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>
> db->emacrx_completed_flag = 1;
> @@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
> /* initial EMAC */
> /* flush RX FIFO */
> reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> - reg_val |= 0x8;
> + reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
> writel(reg_val, db->membase + EMAC_RX_CTL_REG);
> udelay(1);
>
> @@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
>
> /* set MII clock */
> reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
> - reg_val &= (~(0xf << 2));
> - reg_val |= (0xD << 2);
> + reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
> + reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
> writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
>
> /* clear RX counter */
> @@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
>
> /* enable RX/TX0/RX Hlevel interrup */
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0) | (0x01 << 8);
> + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>
> spin_unlock_irqrestore(&db->lock, flags);
> @@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
> if (!rxcount) {
> db->emacrx_completed_flag = 1;
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0) | (0x01 << 8);
> + reg_val |=
> + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> + EMAC_INT_CTL_RX_EN);
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>
> /* had one stuck? */
> @@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
> writel(reg_val | EMAC_CTL_RX_EN,
> db->membase + EMAC_CTL_REG);
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0) | (0x01 << 8);
> + reg_val |=
> + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> + EMAC_INT_CTL_RX_EN);
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
>
> db->emacrx_completed_flag = 1;
> @@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
> }
>
> /* Transmit Interrupt check */
> - if (int_status & (0x01 | 0x02))
> + if (int_status & EMAC_INT_STA_TX_COMPLETE)
> emac_tx_done(dev, db, int_status);
>
> - if (int_status & (0x04 | 0x08))
> + if (int_status & EMAC_INT_STA_TX_ABRT)
> netdev_info(dev, " ab : %x\n", int_status);
>
> /* Re-enable interrupt mask */
> if (db->emacrx_completed_flag == 1) {
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0) | (0x01 << 8);
> + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> } else {
> reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> - reg_val |= (0xf << 0);
> + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
> writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> }
>
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
> index 38c72d9ec600..90bd9ad77607 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.h
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
> @@ -38,6 +38,7 @@
> #define EMAC_RX_CTL_REG (0x3c)
> #define EMAC_RX_CTL_AUTO_DRQ_EN (1 << 1)
> #define EMAC_RX_CTL_DMA_EN (1 << 2)
> +#define EMAC_RX_CTL_FLUSH_FIFO (1 << 3)
> #define EMAC_RX_CTL_PASS_ALL_EN (1 << 4)
> #define EMAC_RX_CTL_PASS_CTL_EN (1 << 5)
> #define EMAC_RX_CTL_PASS_CRC_ERR_EN (1 << 6)
> @@ -61,7 +62,21 @@
> #define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
> #define EMAC_RX_FBC_REG (0x50)
> #define EMAC_INT_CTL_REG (0x54)
> +#define EMAC_INT_CTL_RX_EN (1 << 8)
> +#define EMAC_INT_CTL_TX0_EN (1)
> +#define EMAC_INT_CTL_TX1_EN (1 << 1)
> +#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
> +#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
> +#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
> +#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
> #define EMAC_INT_STA_REG (0x58)
> +#define EMAC_INT_STA_TX0_COMPLETE (0x1)
> +#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
> +#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
> +#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
> +#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
> +#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
> +#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
Hello
As proposed by checkpatch, I thing there are several place (like all EMAC_INT_STA) where you could use BIT(x) instead of (0xX << x)
Regards
On 01/10/22 at 02:31下午, Andrew Lunn wrote:
> Date: Mon, 10 Jan 2022 14:31:28 +0100
> From: Andrew Lunn <[email protected]>
> To: Conley Lee <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number
> with macro
>
> > @@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
> > if (!rxcount) {
> > db->emacrx_completed_flag = 1;
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0) | (0x01 << 8);
> > + reg_val |=
> > + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> > + EMAC_INT_CTL_RX_EN);
>
> Putting the first value on the next line is a bit odd. This would be
> preferred:
>
> + reg_val |= (EMAC_INT_CTL_TX_EN |
> + EMAC_INT_CTL_TX_ABRT_EN |
> + EMAC_INT_CTL_RX_EN);
>
> I also have to wonder why two | have become three? (0x01 << 8) is
> clearly a single value. (0xf << 0) should either be a single macro, or
> 4 macros since 0xf is four bits. Without looking into the details, i
> cannot say this is wrong, but it does look strange.
>
> Andrew
>
Thanks for your suggestion. The (0xf << 0) mask enable tx finish and tx abort
interrupts at hardware level. And the reason this mask has 4 bits is that
sun4i emac has 2 tx channels. I reduce it into two macros EMAC_INT_CTL_TX_EN
and EMAC_INT_CTL_TX_ABRT_EN, this may be more readable, since we always
enable both tx channels in the driver.
On 01/10/22 at 03:05下午, Leon Romanovsky wrote:
> Date: Mon, 10 Jan 2022 15:05:20 +0200
> From: Leon Romanovsky <[email protected]>
> To: Conley Lee <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number
> with macro
>
> On Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee wrote:
> > This patch remove magic numbers in sun4i-emac.c and replace with macros
> > defined in sun4i-emac.h
> >
> > Change since v1
> > ---------------
> > - reformat
> > - merge commits
> > - add commit message
>
> This changelog should be placed after "---" below your SOB.
>
> Thanks
>
> >
> > Signed-off-by: Conley Lee <[email protected]>
> > ---
> > drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
> > drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
> > 2 files changed, 35 insertions(+), 13 deletions(-)
I will reformat it and submit again, thanks ~
On Mon, 10 Jan 2022 14:56:35 +0100 Corentin Labbe wrote:
> > @@ -61,7 +62,21 @@
> > #define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
> > #define EMAC_RX_FBC_REG (0x50)
> > #define EMAC_INT_CTL_REG (0x54)
> > +#define EMAC_INT_CTL_RX_EN (1 << 8)
> > +#define EMAC_INT_CTL_TX0_EN (1)
> > +#define EMAC_INT_CTL_TX1_EN (1 << 1)
> > +#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
> > +#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
> > +#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
> > +#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
> > #define EMAC_INT_STA_REG (0x58)
> > +#define EMAC_INT_STA_TX0_COMPLETE (0x1)
> > +#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
> > +#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
> > +#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
> > +#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
> > +#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
> > +#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
>
> As proposed by checkpatch, I thing there are several place (like all
> EMAC_INT_STA) where you could use BIT(x) instead of (0xX << x)
That's not a hard requirement, if the driver already uses the shift you
can leave your code as is, some upstream developers actually prefer to
avoid the BIT() macro.
This patch remove magic numbers in sun4i-emac.c and replace with macros
defined in sun4i-emac.h
Signed-off-by: Conley Lee <[email protected]>
---
Change since v2.
- fix some code style issues
Change since v1.
- reformat
- merge commits
- add commit message
---
drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
2 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
index 849de4564709..74635a6fa8ca 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
@@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
/* set EMAC SPEED, depend on PHY */
reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
- reg_val &= ~(0x1 << 8);
+ reg_val &= ~EMAC_MAC_SUPP_100M;
if (db->speed == SPEED_100)
- reg_val |= 1 << 8;
+ reg_val |= EMAC_MAC_SUPP_100M;
writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
}
@@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
/* re enable interrupt */
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0x01 << 8);
+ reg_val |= EMAC_INT_CTL_RX_EN;
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
db->emacrx_completed_flag = 1;
@@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
/* initial EMAC */
/* flush RX FIFO */
reg_val = readl(db->membase + EMAC_RX_CTL_REG);
- reg_val |= 0x8;
+ reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
writel(reg_val, db->membase + EMAC_RX_CTL_REG);
udelay(1);
@@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
/* set MII clock */
reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
- reg_val &= (~(0xf << 2));
- reg_val |= (0xD << 2);
+ reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
+ reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
/* clear RX counter */
@@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
/* enable RX/TX0/RX Hlevel interrup */
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
spin_unlock_irqrestore(&db->lock, flags);
@@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
if (!rxcount) {
db->emacrx_completed_flag = 1;
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN |
+ EMAC_INT_CTL_TX_ABRT_EN |
+ EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
/* had one stuck? */
@@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
writel(reg_val | EMAC_CTL_RX_EN,
db->membase + EMAC_CTL_REG);
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN |
+ EMAC_INT_CTL_TX_ABRT_EN |
+ EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
db->emacrx_completed_flag = 1;
@@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
}
/* Transmit Interrupt check */
- if (int_status & (0x01 | 0x02))
+ if (int_status & EMAC_INT_STA_TX_COMPLETE)
emac_tx_done(dev, db, int_status);
- if (int_status & (0x04 | 0x08))
+ if (int_status & EMAC_INT_STA_TX_ABRT)
netdev_info(dev, " ab : %x\n", int_status);
/* Re-enable interrupt mask */
if (db->emacrx_completed_flag == 1) {
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0) | (0x01 << 8);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
} else {
reg_val = readl(db->membase + EMAC_INT_CTL_REG);
- reg_val |= (0xf << 0);
+ reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
writel(reg_val, db->membase + EMAC_INT_CTL_REG);
}
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
index 38c72d9ec600..90bd9ad77607 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.h
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
@@ -38,6 +38,7 @@
#define EMAC_RX_CTL_REG (0x3c)
#define EMAC_RX_CTL_AUTO_DRQ_EN (1 << 1)
#define EMAC_RX_CTL_DMA_EN (1 << 2)
+#define EMAC_RX_CTL_FLUSH_FIFO (1 << 3)
#define EMAC_RX_CTL_PASS_ALL_EN (1 << 4)
#define EMAC_RX_CTL_PASS_CTL_EN (1 << 5)
#define EMAC_RX_CTL_PASS_CRC_ERR_EN (1 << 6)
@@ -61,7 +62,21 @@
#define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
#define EMAC_RX_FBC_REG (0x50)
#define EMAC_INT_CTL_REG (0x54)
+#define EMAC_INT_CTL_RX_EN (1 << 8)
+#define EMAC_INT_CTL_TX0_EN (1)
+#define EMAC_INT_CTL_TX1_EN (1 << 1)
+#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
+#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
+#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
+#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
#define EMAC_INT_STA_REG (0x58)
+#define EMAC_INT_STA_TX0_COMPLETE (0x1)
+#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
+#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
+#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
+#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
+#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
+#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
#define EMAC_MAC_CTL0_REG (0x5c)
#define EMAC_MAC_CTL0_RX_FLOW_CTL_EN (1 << 2)
#define EMAC_MAC_CTL0_TX_FLOW_CTL_EN (1 << 3)
@@ -87,8 +102,11 @@
#define EMAC_MAC_CLRT_RM (0x0f)
#define EMAC_MAC_MAXF_REG (0x70)
#define EMAC_MAC_SUPP_REG (0x74)
+#define EMAC_MAC_SUPP_100M (0x1 << 8)
#define EMAC_MAC_TEST_REG (0x78)
#define EMAC_MAC_MCFG_REG (0x7c)
+#define EMAC_MAC_MCFG_MII_CLKD_MASK (0xff << 2)
+#define EMAC_MAC_MCFG_MII_CLKD_72 (0x0d << 2)
#define EMAC_MAC_A0_REG (0x98)
#define EMAC_MAC_A1_REG (0x9c)
#define EMAC_MAC_A2_REG (0xa0)
--
2.31.1
On 01/10/22 at 02:56下午, Corentin Labbe wrote:
> Date: Mon, 10 Jan 2022 14:56:35 +0100
> From: Corentin Labbe <[email protected]>
> To: Conley Lee <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v2] net: ethernet: sun4i-emac: replace magic number
> with macro
>
> Le Mon, Jan 10, 2022 at 07:35:49PM +0800, Conley Lee a écrit :
> > This patch remove magic numbers in sun4i-emac.c and replace with macros
> > defined in sun4i-emac.h
> >
> > Change since v1
> > ---------------
> > - reformat
> > - merge commits
> > - add commit message
> >
> > Signed-off-by: Conley Lee <[email protected]>
> > ---
> > drivers/net/ethernet/allwinner/sun4i-emac.c | 30 ++++++++++++---------
> > drivers/net/ethernet/allwinner/sun4i-emac.h | 18 +++++++++++++
> > 2 files changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> > index 849de4564709..98fd98feb439 100644
> > --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> > +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> > @@ -106,9 +106,9 @@ static void emac_update_speed(struct net_device *dev)
> >
> > /* set EMAC SPEED, depend on PHY */
> > reg_val = readl(db->membase + EMAC_MAC_SUPP_REG);
> > - reg_val &= ~(0x1 << 8);
> > + reg_val &= ~EMAC_MAC_SUPP_100M;
> > if (db->speed == SPEED_100)
> > - reg_val |= 1 << 8;
> > + reg_val |= EMAC_MAC_SUPP_100M;
> > writel(reg_val, db->membase + EMAC_MAC_SUPP_REG);
> > }
> >
> > @@ -264,7 +264,7 @@ static void emac_dma_done_callback(void *arg)
> >
> > /* re enable interrupt */
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0x01 << 8);
> > + reg_val |= EMAC_INT_CTL_RX_EN;
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> >
> > db->emacrx_completed_flag = 1;
> > @@ -429,7 +429,7 @@ static unsigned int emac_powerup(struct net_device *ndev)
> > /* initial EMAC */
> > /* flush RX FIFO */
> > reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> > - reg_val |= 0x8;
> > + reg_val |= EMAC_RX_CTL_FLUSH_FIFO;
> > writel(reg_val, db->membase + EMAC_RX_CTL_REG);
> > udelay(1);
> >
> > @@ -441,8 +441,8 @@ static unsigned int emac_powerup(struct net_device *ndev)
> >
> > /* set MII clock */
> > reg_val = readl(db->membase + EMAC_MAC_MCFG_REG);
> > - reg_val &= (~(0xf << 2));
> > - reg_val |= (0xD << 2);
> > + reg_val &= ~EMAC_MAC_MCFG_MII_CLKD_MASK;
> > + reg_val |= EMAC_MAC_MCFG_MII_CLKD_72;
> > writel(reg_val, db->membase + EMAC_MAC_MCFG_REG);
> >
> > /* clear RX counter */
> > @@ -506,7 +506,7 @@ static void emac_init_device(struct net_device *dev)
> >
> > /* enable RX/TX0/RX Hlevel interrup */
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0) | (0x01 << 8);
> > + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> >
> > spin_unlock_irqrestore(&db->lock, flags);
> > @@ -637,7 +637,9 @@ static void emac_rx(struct net_device *dev)
> > if (!rxcount) {
> > db->emacrx_completed_flag = 1;
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0) | (0x01 << 8);
> > + reg_val |=
> > + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> > + EMAC_INT_CTL_RX_EN);
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> >
> > /* had one stuck? */
> > @@ -669,7 +671,9 @@ static void emac_rx(struct net_device *dev)
> > writel(reg_val | EMAC_CTL_RX_EN,
> > db->membase + EMAC_CTL_REG);
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0) | (0x01 << 8);
> > + reg_val |=
> > + (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN |
> > + EMAC_INT_CTL_RX_EN);
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> >
> > db->emacrx_completed_flag = 1;
> > @@ -783,20 +787,20 @@ static irqreturn_t emac_interrupt(int irq, void *dev_id)
> > }
> >
> > /* Transmit Interrupt check */
> > - if (int_status & (0x01 | 0x02))
> > + if (int_status & EMAC_INT_STA_TX_COMPLETE)
> > emac_tx_done(dev, db, int_status);
> >
> > - if (int_status & (0x04 | 0x08))
> > + if (int_status & EMAC_INT_STA_TX_ABRT)
> > netdev_info(dev, " ab : %x\n", int_status);
> >
> > /* Re-enable interrupt mask */
> > if (db->emacrx_completed_flag == 1) {
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0) | (0x01 << 8);
> > + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN | EMAC_INT_CTL_RX_EN);
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> > } else {
> > reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> > - reg_val |= (0xf << 0);
> > + reg_val |= (EMAC_INT_CTL_TX_EN | EMAC_INT_CTL_TX_ABRT_EN);
> > writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> > }
> >
> > diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.h b/drivers/net/ethernet/allwinner/sun4i-emac.h
> > index 38c72d9ec600..90bd9ad77607 100644
> > --- a/drivers/net/ethernet/allwinner/sun4i-emac.h
> > +++ b/drivers/net/ethernet/allwinner/sun4i-emac.h
> > @@ -38,6 +38,7 @@
> > #define EMAC_RX_CTL_REG (0x3c)
> > #define EMAC_RX_CTL_AUTO_DRQ_EN (1 << 1)
> > #define EMAC_RX_CTL_DMA_EN (1 << 2)
> > +#define EMAC_RX_CTL_FLUSH_FIFO (1 << 3)
> > #define EMAC_RX_CTL_PASS_ALL_EN (1 << 4)
> > #define EMAC_RX_CTL_PASS_CTL_EN (1 << 5)
> > #define EMAC_RX_CTL_PASS_CRC_ERR_EN (1 << 6)
> > @@ -61,7 +62,21 @@
> > #define EMAC_RX_IO_DATA_STATUS_OK (1 << 7)
> > #define EMAC_RX_FBC_REG (0x50)
> > #define EMAC_INT_CTL_REG (0x54)
> > +#define EMAC_INT_CTL_RX_EN (1 << 8)
> > +#define EMAC_INT_CTL_TX0_EN (1)
> > +#define EMAC_INT_CTL_TX1_EN (1 << 1)
> > +#define EMAC_INT_CTL_TX_EN (EMAC_INT_CTL_TX0_EN | EMAC_INT_CTL_TX1_EN)
> > +#define EMAC_INT_CTL_TX0_ABRT_EN (0x1 << 2)
> > +#define EMAC_INT_CTL_TX1_ABRT_EN (0x1 << 3)
> > +#define EMAC_INT_CTL_TX_ABRT_EN (EMAC_INT_CTL_TX0_ABRT_EN | EMAC_INT_CTL_TX1_ABRT_EN)
> > #define EMAC_INT_STA_REG (0x58)
> > +#define EMAC_INT_STA_TX0_COMPLETE (0x1)
> > +#define EMAC_INT_STA_TX1_COMPLETE (0x1 << 1)
> > +#define EMAC_INT_STA_TX_COMPLETE (EMAC_INT_STA_TX0_COMPLETE | EMAC_INT_STA_TX1_COMPLETE)
> > +#define EMAC_INT_STA_TX0_ABRT (0x1 << 2)
> > +#define EMAC_INT_STA_TX1_ABRT (0x1 << 3)
> > +#define EMAC_INT_STA_TX_ABRT (EMAC_INT_STA_TX0_ABRT | EMAC_INT_STA_TX1_ABRT)
> > +#define EMAC_INT_STA_RX_COMPLETE (0x1 << 8)
>
> Hello
>
> As proposed by checkpatch, I thing there are several place (like all EMAC_INT_STA) where you could use BIT(x) instead of (0xX << x)
>
> Regards
Hi ~
Thanks for your suggestion. But I think it might be better to keep the code style consistent,
so I didn't change it in the new version of this patch.
Le Tue, Jan 11, 2022 at 11:05:53AM +0800, Conley Lee a ?crit :
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
>
> Signed-off-by: Conley Lee <[email protected]>
Hello
I sent a CI job witch next-20220107+yourpatch and saw no regression.
Tested-by: Corentin Labbe <[email protected]>
Tested-on: sun4i-a10-olinuxino-lime
Thanks!
Regards
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[email protected]>:
On Tue, 11 Jan 2022 11:05:53 +0800 you wrote:
> This patch remove magic numbers in sun4i-emac.c and replace with macros
> defined in sun4i-emac.h
>
> Signed-off-by: Conley Lee <[email protected]>
> ---
> Change since v2.
> - fix some code style issues
>
> [...]
Here is the summary with links:
- [v3] net: ethernet: sun4i-emac: replace magic number with macro
https://git.kernel.org/netdev/net/c/274c224062ff
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On 01/11/22 at 05:32下午, Corentin Labbe wrote:
> Date: Tue, 11 Jan 2022 17:32:11 +0100
> From: Corentin Labbe <[email protected]>
> To: Conley Lee <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v3] net: ethernet: sun4i-emac: replace magic number
> with macro
>
> Le Tue, Jan 11, 2022 at 11:05:53AM +0800, Conley Lee a écrit :
> > This patch remove magic numbers in sun4i-emac.c and replace with macros
> > defined in sun4i-emac.h
> >
> > Signed-off-by: Conley Lee <[email protected]>
>
> Hello
>
> I sent a CI job witch next-20220107+yourpatch and saw no regression.
>
> Tested-by: Corentin Labbe <[email protected]>
> Tested-on: sun4i-a10-olinuxino-lime
>
> Thanks!
> Regards
>
Could you please provide more information about it ? I test it on my
marsboard-a20 and everything work well.
Thanks