| Message ID | 20240209091100.5341-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: fec: Refactor: #define magic constants | expand |
On 09.02.2024 10:11:01, Csókás Bence wrote: > Add defines for bits of ECR, RCR control registers, TX watermark etc. > > Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> > --- > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++-------- > 1 file changed, 33 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 63707e065141..a16220eff9b3 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep, > > static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2}; > > -/* Pause frame feild and FIFO threshold */ > -#define FEC_ENET_FCE (1 << 5) > #define FEC_ENET_RSEM_V 0x84 > #define FEC_ENET_RSFL_V 16 > #define FEC_ENET_RAEM_V 0x8 > @@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > #define PKT_MINBUF_SIZE 64 > > /* FEC receive acceleration */ > -#define FEC_RACC_IPDIS (1 << 1) > -#define FEC_RACC_PRODIS (1 << 2) > +#define FEC_RACC_IPDIS BIT(1) > +#define FEC_RACC_PRODIS BIT(2) > #define FEC_RACC_SHIFT16 BIT(7) > #define FEC_RACC_OPTIONS (FEC_RACC_IPDIS | FEC_RACC_PRODIS) > > @@ -273,8 +271,23 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > #define FEC_MMFR_TA (2 << 16) > #define FEC_MMFR_DATA(v) (v & 0xffff) > /* FEC ECR bits definition */ > -#define FEC_ECR_MAGICEN (1 << 2) > -#define FEC_ECR_SLEEP (1 << 3) > +#define FEC_ECR_RESET BIT(0) > +#define FEC_ECR_ETHEREN BIT(1) > +#define FEC_ECR_MAGICEN BIT(2) > +#define FEC_ECR_SLEEP BIT(3) > +#define FEC_ECR_EN1588 BIT(4) > +#define FEC_ECR_BYTESWP BIT(8) > +/* FEC RCR bits definition */ > +#define FEC_RCR_LOOP BIT(0) > +#define FEC_RCR_HALFDPX BIT(1) > +#define FEC_RCR_MII BIT(2) > +#define FEC_RCR_PROMISC BIT(3) > +#define FEC_RCR_BC_REJ BIT(4) > +#define FEC_RCR_FLOWCTL BIT(5) > +#define FEC_RCR_RMII BIT(8) > +#define FEC_RCR_10BASET BIT(9) > +/* TX WMARK bits */ > +#define FEC_TXWMRK_STRFWD BIT(8) > > #define FEC_MII_TIMEOUT 30000 /* us */ > > @@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev) > fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > rcntl |= (1 << 6); > else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) > - rcntl |= (1 << 8); > + rcntl |= FEC_RCR_RMII; > else > - rcntl &= ~(1 << 8); > + rcntl &= ~FEC_RCR_RMII; > > /* 1G, 100M or 10M */ > if (ndev->phydev) { > if (ndev->phydev->speed == SPEED_1000) > ecntl |= (1 << 5); > else if (ndev->phydev->speed == SPEED_100) > - rcntl &= ~(1 << 9); > + rcntl &= ~FEC_RCR_10BASET; > else > - rcntl |= (1 << 9); > + rcntl |= FEC_RCR_10BASET; > } > } else { > #ifdef FEC_MIIGSK_ENR > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) > if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || > ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && > ndev->phydev && ndev->phydev->pause)) { > - rcntl |= FEC_ENET_FCE; > + rcntl |= FEC_RCR_FLOWCTL; > > /* set FIFO threshold parameter to reduce overrun */ > writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM); > @@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev) > /* OPD */ > writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD); > } else { > - rcntl &= ~FEC_ENET_FCE; > + rcntl &= ~FEC_RCR_FLOWCTL; > } > #endif /* !defined(CONFIG_M5272) */ > > @@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev) > > if (fep->quirks & FEC_QUIRK_ENET_MAC) { > /* enable ENET endian swap */ > - ecntl |= (1 << 8); > + ecntl |= FEC_ECR_BYTESWP; > /* enable ENET store and forward mode */ > - writel(1 << 8, fep->hwp + FEC_X_WMRK); > + writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK); > } > > if (fep->bufdesc_ex) > - ecntl |= (1 << 4); > + ecntl |= FEC_ECR_EN1588; > > if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT && > fep->rgmii_txc_dly) > @@ -1312,7 +1325,8 @@ static void > fec_stop(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); > + u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII; > + u32 ecntl = 0; This is an unrelated change. > u32 val; > > /* We cannot expect a graceful transmit stop without link !!! */ > @@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev) > /* We have to keep ENET enabled to have MII interrupt stay working */ > if (fep->quirks & FEC_QUIRK_ENET_MAC && > !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - writel(2, fep->hwp + FEC_ECNTRL); > + ecntl |= FEC_ECR_ETHEREN; This is an unrelated change. > writel(rmii_mode, fep->hwp + FEC_R_CNTRL); > } > + > + writel(ecntl, fep->hwp + FEC_ECNTRL); This is an unrelated change. Marc
On 2/9/24 12:11, Csókás Bence wrote: > Add defines for bits of ECR, RCR control registers, TX watermark etc. > > Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> Please add net-next prefix > --- > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++-------- > 1 file changed, 33 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 63707e065141..a16220eff9b3 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep, > > static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2}; > > -/* Pause frame feild and FIFO threshold */ > -#define FEC_ENET_FCE (1 << 5) > #define FEC_ENET_RSEM_V 0x84 > #define FEC_ENET_RSFL_V 16 > #define FEC_ENET_RAEM_V 0x8 > @@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > #define PKT_MINBUF_SIZE 64 > > /* FEC receive acceleration */ > -#define FEC_RACC_IPDIS (1 << 1) > -#define FEC_RACC_PRODIS (1 << 2) > +#define FEC_RACC_IPDIS BIT(1) > +#define FEC_RACC_PRODIS BIT(2) > #define FEC_RACC_SHIFT16 BIT(7) > #define FEC_RACC_OPTIONS (FEC_RACC_IPDIS | FEC_RACC_PRODIS) > > @@ -273,8 +271,23 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > #define FEC_MMFR_TA (2 << 16) > #define FEC_MMFR_DATA(v) (v & 0xffff) > /* FEC ECR bits definition */ > -#define FEC_ECR_MAGICEN (1 << 2) > -#define FEC_ECR_SLEEP (1 << 3) > +#define FEC_ECR_RESET BIT(0) > +#define FEC_ECR_ETHEREN BIT(1) > +#define FEC_ECR_MAGICEN BIT(2) > +#define FEC_ECR_SLEEP BIT(3) > +#define FEC_ECR_EN1588 BIT(4) > +#define FEC_ECR_BYTESWP BIT(8) > +/* FEC RCR bits definition */ > +#define FEC_RCR_LOOP BIT(0) > +#define FEC_RCR_HALFDPX BIT(1) > +#define FEC_RCR_MII BIT(2) > +#define FEC_RCR_PROMISC BIT(3) > +#define FEC_RCR_BC_REJ BIT(4) > +#define FEC_RCR_FLOWCTL BIT(5) > +#define FEC_RCR_RMII BIT(8) > +#define FEC_RCR_10BASET BIT(9) > +/* TX WMARK bits */ > +#define FEC_TXWMRK_STRFWD BIT(8) > > #define FEC_MII_TIMEOUT 30000 /* us */ > > @@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev) > fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) > rcntl |= (1 << 6); > else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) > - rcntl |= (1 << 8); > + rcntl |= FEC_RCR_RMII; > else > - rcntl &= ~(1 << 8); > + rcntl &= ~FEC_RCR_RMII; > > /* 1G, 100M or 10M */ > if (ndev->phydev) { > if (ndev->phydev->speed == SPEED_1000) > ecntl |= (1 << 5); > else if (ndev->phydev->speed == SPEED_100) > - rcntl &= ~(1 << 9); > + rcntl &= ~FEC_RCR_10BASET; > else > - rcntl |= (1 << 9); > + rcntl |= FEC_RCR_10BASET; > } > } else { > #ifdef FEC_MIIGSK_ENR > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) > if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || > ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && > ndev->phydev && ndev->phydev->pause)) { > - rcntl |= FEC_ENET_FCE; > + rcntl |= FEC_RCR_FLOWCTL; > > /* set FIFO threshold parameter to reduce overrun */ > writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM); > @@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev) > /* OPD */ > writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD); > } else { > - rcntl &= ~FEC_ENET_FCE; > + rcntl &= ~FEC_RCR_FLOWCTL; > } > #endif /* !defined(CONFIG_M5272) */ > > @@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev) > > if (fep->quirks & FEC_QUIRK_ENET_MAC) { > /* enable ENET endian swap */ > - ecntl |= (1 << 8); > + ecntl |= FEC_ECR_BYTESWP; > /* enable ENET store and forward mode */ > - writel(1 << 8, fep->hwp + FEC_X_WMRK); > + writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK); > } > > if (fep->bufdesc_ex) > - ecntl |= (1 << 4); > + ecntl |= FEC_ECR_EN1588; > > if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT && > fep->rgmii_txc_dly) > @@ -1312,7 +1325,8 @@ static void > fec_stop(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); > + u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII; > + u32 ecntl = 0; > u32 val; > > /* We cannot expect a graceful transmit stop without link !!! */ > @@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev) > /* We have to keep ENET enabled to have MII interrupt stay working */ > if (fep->quirks & FEC_QUIRK_ENET_MAC && > !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - writel(2, fep->hwp + FEC_ECNTRL); > + ecntl |= FEC_ECR_ETHEREN; > writel(rmii_mode, fep->hwp + FEC_R_CNTRL); > } > + > + writel(ecntl, fep->hwp + FEC_ECNTRL); > } > >
> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) > if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || > ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && > ndev->phydev && ndev->phydev->pause)) { > - rcntl |= FEC_ENET_FCE; > + rcntl |= FEC_RCR_FLOWCTL; This immediately stood out to me while looking at the diff. Its not obvious why this is correct. Looking back, i see you removed FEC_ENET_FCE, not renamed it. Ideally, you want lots of small patches which are obviously correct. This change is not obvious, there is no explanation in the commit message etc. Please keep this patch about straight, obvious, replacement of bit shifts with macros. Do all other changes in additional patches. It is much easier to review then, both by you before you post, and us when it hits the list. Andrew
Hi! 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta: >> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) >> if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || >> ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && >> ndev->phydev && ndev->phydev->pause)) { >> - rcntl |= FEC_ENET_FCE; >> + rcntl |= FEC_RCR_FLOWCTL; > > This immediately stood out to me while looking at the diff. Its not > obvious why this is correct. Looking back, i see you removed > FEC_ENET_FCE, not renamed it. What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it obvious that it represents a bit in RCR (or `rcntl` as it is called on this line). How is that not "renaming" it? > Ideally, you want lots of small patches which are obviously correct. > This change is not obvious, there is no explanation in the commit > message etc. > > Please keep this patch about straight, obvious, replacement of bit > shifts with macros. So, how should I break it up then? One patch for the ECR bits, one for RCR, one for TX watermark register, one for RACC? Or one commit introducing the constants, and another replacing usages with these? > Do all other changes in additional patches. It is much easier to > review then, both by you before you post, and us when it hits the > list. > > Andrew > Thanks, Bence
On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote: > Hi! > > 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta: > > > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) > > > if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || > > > ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && > > > ndev->phydev && ndev->phydev->pause)) { > > > - rcntl |= FEC_ENET_FCE; > > > + rcntl |= FEC_RCR_FLOWCTL; > > > > This immediately stood out to me while looking at the diff. Its not > > obvious why this is correct. Looking back, i see you removed > > FEC_ENET_FCE, not renamed it. > > What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it > obvious that it represents a bit in RCR (or `rcntl` as it is called on this > line). How is that not "renaming" it? Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was it wrong before? Is this actually a fix? Is it correct now, or is this a cut/paste typo? Looking at the rest of the patch there is no obvious answer. As i said, you deleted FEC_ENET_FCE, but there is no explanation why. So what i'm asking for is obviously correct patches. You can add the #defines, and replace (1 << X) with one of the new macros, and it should be obvious. However, the change above is not obviously correct, so some explanation is required. And it is easier to do that in a patch dedicated to this change, with a good explanation. Andrew
2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta: > On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote: >> Hi! >> >> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta: >>>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) >>>> if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || >>>> ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && >>>> ndev->phydev && ndev->phydev->pause)) { >>>> - rcntl |= FEC_ENET_FCE; >>>> + rcntl |= FEC_RCR_FLOWCTL; >>> >>> This immediately stood out to me while looking at the diff. Its not >>> obvious why this is correct. Looking back, i see you removed >>> FEC_ENET_FCE, not renamed it. >> >> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it >> obvious that it represents a bit in RCR (or `rcntl` as it is called on this >> line). How is that not "renaming" it? > > Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was > it wrong before? Is this actually a fix? Is it correct now, or is this > a cut/paste typo? Looking at the rest of the patch there is no obvious > answer. As i said, you deleted FEC_ENET_FCE, but there is no > explanation why. The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control Enable) bit (1 << 5) of the RCR (Receive Control Register). I added FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a much more descriptive name (in my opinion, at least). > So what i'm asking for is obviously correct patches. You can add the > #defines, and replace (1 << X) with one of the new macros, and it > should be obvious. I replaced `#define FEC_ENET_FCE (1 << 5)` with `#define FEC_RCR_FLOWCTL BIT(5)`. I thought that was "obviously correct", but I can break the patch up more, if it helps readability. > However, the change above is not obviously correct, so some > explanation is required. And it is easier to do that in a patch > dedicated to this change, with a good explanation. So, a separate patch just for removing FEC_ENET_FCE and replacing all usages with FEC_RCR_FLOWCTL? And the rest can stay as-is? > > Andrew > Bence
On Mon, Feb 12, 2024 at 04:11:10PM +0100, Csókás Bence wrote: > > > 2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta: > > On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote: > > > Hi! > > > > > > 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta: > > > > > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) > > > > > if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || > > > > > ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && > > > > > ndev->phydev && ndev->phydev->pause)) { > > > > > - rcntl |= FEC_ENET_FCE; > > > > > + rcntl |= FEC_RCR_FLOWCTL; > > > > > > > > This immediately stood out to me while looking at the diff. Its not > > > > obvious why this is correct. Looking back, i see you removed > > > > FEC_ENET_FCE, not renamed it. > > > > > > What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it > > > obvious that it represents a bit in RCR (or `rcntl` as it is called on this > > > line). How is that not "renaming" it? > > > > Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was > > it wrong before? Is this actually a fix? Is it correct now, or is this > > a cut/paste typo? Looking at the rest of the patch there is no obvious > > answer. As i said, you deleted FEC_ENET_FCE, but there is no > > explanation why. > > The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control > Enable) bit (1 << 5) of the RCR (Receive Control Register). I added > FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a > much more descriptive name (in my opinion, at least). Some form of that would be good in the commit message. It explains the 'Why?' of the change. > So, a separate patch just for removing FEC_ENET_FCE and replacing all usages > with FEC_RCR_FLOWCTL? And the rest can stay as-is? A few others made review comments as well. It could be addressing those comments also requires more small patches. Sometimes you can avoid review comments by thinking, what are reviewers going to ask, and putting the answer to those questions in the commit message. This might all seam like a lot of hassle now, but it will help getting your future patchsets merged if you follow this advice. Andrew
2024. 02. 12. 17:03 keltezéssel, Andrew Lunn írta: > On Mon, Feb 12, 2024 at 04:11:10PM +0100, Csókás Bence wrote: >> >> >> 2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta: >>> On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote: >>>> Hi! >>>> >>>> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta: >>>>>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) >>>>>> if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || >>>>>> ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && >>>>>> ndev->phydev && ndev->phydev->pause)) { >>>>>> - rcntl |= FEC_ENET_FCE; >>>>>> + rcntl |= FEC_RCR_FLOWCTL; >>>>> >>>>> This immediately stood out to me while looking at the diff. Its not >>>>> obvious why this is correct. Looking back, i see you removed >>>>> FEC_ENET_FCE, not renamed it. >>>> >>>> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it >>>> obvious that it represents a bit in RCR (or `rcntl` as it is called on this >>>> line). How is that not "renaming" it? >>> >>> Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was >>> it wrong before? Is this actually a fix? Is it correct now, or is this >>> a cut/paste typo? Looking at the rest of the patch there is no obvious >>> answer. As i said, you deleted FEC_ENET_FCE, but there is no >>> explanation why. >> >> The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control >> Enable) bit (1 << 5) of the RCR (Receive Control Register). I added >> FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a >> much more descriptive name (in my opinion, at least). > > Some form of that would be good in the commit message. It explains the > 'Why?' of the change. Ok. I split that change into a separate patch with a quick summary of this, in v4 of this patch. Hopefully it is more clear now. >> So, a separate patch just for removing FEC_ENET_FCE and replacing all usages >> with FEC_RCR_FLOWCTL? And the rest can stay as-is? > > A few others made review comments as well. It could be addressing > those comments also requires more small patches. Sometimes you can > avoid review comments by thinking, what are reviewers going to ask, > and putting the answer to those questions in the commit message. This > might all seam like a lot of hassle now, but it will help getting your > future patchsets merged if you follow this advice. > > Andrew I believe I addressed all other comments already, but do enlighten me if that is not the case: * removed the "fix FEC_ECR_EN1588 being cleared on link-down" half of the patch (v2) - your feedback * removed `u32 ecntl` from `fec_stop()` (v3) - requested by Marc * added net-next subject-prefix (v3) - suggested by Denis * factored out the removal of FEC_ENET_FCE (v4) - your feedback Did I miss something? Bence
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 63707e065141..a16220eff9b3 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep, static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2}; -/* Pause frame feild and FIFO threshold */ -#define FEC_ENET_FCE (1 << 5) #define FEC_ENET_RSEM_V 0x84 #define FEC_ENET_RSFL_V 16 #define FEC_ENET_RAEM_V 0x8 @@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); #define PKT_MINBUF_SIZE 64 /* FEC receive acceleration */ -#define FEC_RACC_IPDIS (1 << 1) -#define FEC_RACC_PRODIS (1 << 2) +#define FEC_RACC_IPDIS BIT(1) +#define FEC_RACC_PRODIS BIT(2) #define FEC_RACC_SHIFT16 BIT(7) #define FEC_RACC_OPTIONS (FEC_RACC_IPDIS | FEC_RACC_PRODIS) @@ -273,8 +271,23 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); #define FEC_MMFR_TA (2 << 16) #define FEC_MMFR_DATA(v) (v & 0xffff) /* FEC ECR bits definition */ -#define FEC_ECR_MAGICEN (1 << 2) -#define FEC_ECR_SLEEP (1 << 3) +#define FEC_ECR_RESET BIT(0) +#define FEC_ECR_ETHEREN BIT(1) +#define FEC_ECR_MAGICEN BIT(2) +#define FEC_ECR_SLEEP BIT(3) +#define FEC_ECR_EN1588 BIT(4) +#define FEC_ECR_BYTESWP BIT(8) +/* FEC RCR bits definition */ +#define FEC_RCR_LOOP BIT(0) +#define FEC_RCR_HALFDPX BIT(1) +#define FEC_RCR_MII BIT(2) +#define FEC_RCR_PROMISC BIT(3) +#define FEC_RCR_BC_REJ BIT(4) +#define FEC_RCR_FLOWCTL BIT(5) +#define FEC_RCR_RMII BIT(8) +#define FEC_RCR_10BASET BIT(9) +/* TX WMARK bits */ +#define FEC_TXWMRK_STRFWD BIT(8) #define FEC_MII_TIMEOUT 30000 /* us */ @@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev) fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) rcntl |= (1 << 6); else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) - rcntl |= (1 << 8); + rcntl |= FEC_RCR_RMII; else - rcntl &= ~(1 << 8); + rcntl &= ~FEC_RCR_RMII; /* 1G, 100M or 10M */ if (ndev->phydev) { if (ndev->phydev->speed == SPEED_1000) ecntl |= (1 << 5); else if (ndev->phydev->speed == SPEED_100) - rcntl &= ~(1 << 9); + rcntl &= ~FEC_RCR_10BASET; else - rcntl |= (1 << 9); + rcntl |= FEC_RCR_10BASET; } } else { #ifdef FEC_MIIGSK_ENR @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev) if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) || ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) && ndev->phydev && ndev->phydev->pause)) { - rcntl |= FEC_ENET_FCE; + rcntl |= FEC_RCR_FLOWCTL; /* set FIFO threshold parameter to reduce overrun */ writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM); @@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev) /* OPD */ writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD); } else { - rcntl &= ~FEC_ENET_FCE; + rcntl &= ~FEC_RCR_FLOWCTL; } #endif /* !defined(CONFIG_M5272) */ @@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev) if (fep->quirks & FEC_QUIRK_ENET_MAC) { /* enable ENET endian swap */ - ecntl |= (1 << 8); + ecntl |= FEC_ECR_BYTESWP; /* enable ENET store and forward mode */ - writel(1 << 8, fep->hwp + FEC_X_WMRK); + writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK); } if (fep->bufdesc_ex) - ecntl |= (1 << 4); + ecntl |= FEC_ECR_EN1588; if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT && fep->rgmii_txc_dly) @@ -1312,7 +1325,8 @@ static void fec_stop(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); - u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); + u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII; + u32 ecntl = 0; u32 val; /* We cannot expect a graceful transmit stop without link !!! */ @@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev) /* We have to keep ENET enabled to have MII interrupt stay working */ if (fep->quirks & FEC_QUIRK_ENET_MAC && !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { - writel(2, fep->hwp + FEC_ECNTRL); + ecntl |= FEC_ECR_ETHEREN; writel(rmii_mode, fep->hwp + FEC_R_CNTRL); } + + writel(ecntl, fep->hwp + FEC_ECNTRL); }
Add defines for bits of ECR, RCR control registers, TX watermark etc. Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++-------- 1 file changed, 33 insertions(+), 17 deletions(-)