| Message ID | 20240207123610.16337-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [resubmit] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down | expand |
On Wed, Feb 07, 2024 at 01:36:11PM +0100, Csókás Bence wrote: > FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which > makes all 1588 functionality shut down on link-down. However, some > functionality needs to be retained (e.g. PPS) even without link. This is the second version of the patch, so the subject should say v2 within the [PATCH ]. Is this fixing a regression, or did it never work correctly? > Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> > --- > drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 63707e065141..652251e48ad4 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -273,8 +273,11 @@ 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) There was a request to keep the indentation the same. So the BIT() need moving right. > > #define FEC_MII_TIMEOUT 30000 /* us */ > > @@ -1213,7 +1216,7 @@ fec_restart(struct net_device *ndev) > } > > if (fep->bufdesc_ex) > - ecntl |= (1 << 4); > + ecntl |= FEC_ECR_EN1588; Please could you split this into two patches. The first patch introduced the new #defines, and uses them in the existing code. That should be obviously correct. And a second patch adding the new functionality, with a good commit message explaining the change, particularly the Why? > > if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT && > fep->rgmii_txc_dly) > @@ -1314,6 +1317,7 @@ 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 val; > + u32 ecntl = 0; Reverse Christmas tree, as pointed out in your first version of the patch. Thanks Andrew --- pw-bot: cr
2024. 02. 07. 15:44 keltezéssel, Andrew Lunn írta: > On Wed, Feb 07, 2024 at 01:36:11PM +0100, Csókás Bence wrote: >> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which >> makes all 1588 functionality shut down on link-down. However, some >> functionality needs to be retained (e.g. PPS) even without link. > > This is the second version of the patch, so the subject should say v2 > within the [PATCH ]. This is the same as the other, just added the rationale text to the message body. > Is this fixing a regression, or did it never work correctly? PTP on this family of Ethernet controllers is utterly broken. See commit f79959220fa5fbda939592bf91c7a9ea90419040 (reverted). I plan on re-submitting that entire patch, if i can figure out the locking problems that have arisen. > Please could you split this into two patches. The first patch > introduced the new #defines, and uses them in the existing code. That > should be obviously correct. And a second patch adding the new > functionality, with a good commit message explaining the change, > particularly the Why? Sure. > There was a request to keep the indentation the same. So the BIT() > need moving right. > Reverse Christmas tree, as pointed out in your first version of the > patch. Ah, sorry, I missed those. Also can do. > Thanks > Andrew > > --- > pw-bot: cr > Bence
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 63707e065141..652251e48ad4 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -273,8 +273,11 @@ 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_MII_TIMEOUT 30000 /* us */ @@ -1213,7 +1216,7 @@ fec_restart(struct net_device *ndev) } if (fep->bufdesc_ex) - ecntl |= (1 << 4); + ecntl |= FEC_ECR_EN1588; if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT && fep->rgmii_txc_dly) @@ -1314,6 +1317,7 @@ 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 val; + u32 ecntl = 0; /* We cannot expect a graceful transmit stop without link !!! */ if (fep->link) { @@ -1342,12 +1346,17 @@ fec_stop(struct net_device *ndev) writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); + if (fep->bufdesc_ex) + ecntl |= FEC_ECR_EN1588; + /* 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); }
FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which makes all 1588 functionality shut down on link-down. However, some functionality needs to be retained (e.g. PPS) even without link. Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)