| Message ID | 1561281781-13479-1-git-send-email-pthombar@cadence.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: macb: cover letter | expand |
On Sun, Jun 23, 2019 at 10:23:01AM +0100, Parshuram Thombare wrote: > This patch add support for SGMII interface) and > 2.5Gbps MAC in Cadence ethernet controller driver. > > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> > --- > drivers/net/ethernet/cadence/macb.h | 54 ++++++++++++---- > drivers/net/ethernet/cadence/macb_main.c | 80 +++++++++++++++++++++--- > 2 files changed, 112 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index 8629d345af31..6d268283c318 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -77,6 +77,7 @@ > #define MACB_RBQPH 0x04D4 > > /* GEM register offsets. */ > +#define GEM_NCR 0x0000 /* Network Control */ > #define GEM_NCFGR 0x0004 /* Network Config */ > #define GEM_USRIO 0x000c /* User IO */ > #define GEM_DMACFG 0x0010 /* DMA Configuration */ > @@ -156,6 +157,7 @@ > #define GEM_PEFTN 0x01f4 /* PTP Peer Event Frame Tx Ns */ > #define GEM_PEFRSL 0x01f8 /* PTP Peer Event Frame Rx Sec Low */ > #define GEM_PEFRN 0x01fc /* PTP Peer Event Frame Rx Ns */ > +#define GEM_PCS_CTRL 0x0200 /* PCS Control */ > #define GEM_DCFG1 0x0280 /* Design Config 1 */ > #define GEM_DCFG2 0x0284 /* Design Config 2 */ > #define GEM_DCFG3 0x0288 /* Design Config 3 */ > @@ -271,6 +273,10 @@ > #define MACB_IRXFCS_OFFSET 19 > #define MACB_IRXFCS_SIZE 1 > > +/* GEM specific NCR bitfields. */ > +#define GEM_TWO_PT_FIVE_GIG_OFFSET 29 > +#define GEM_TWO_PT_FIVE_GIG_SIZE 1 > + > /* GEM specific NCFGR bitfields. */ > #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */ > #define GEM_GBE_SIZE 1 > @@ -323,6 +329,9 @@ > #define MACB_MDIO_SIZE 1 > #define MACB_IDLE_OFFSET 2 /* The PHY management logic is idle */ > #define MACB_IDLE_SIZE 1 > +#define MACB_DUPLEX_OFFSET 3 > +#define MACB_DUPLEX_SIZE 1 > + > > /* Bitfields in TSR */ > #define MACB_UBR_OFFSET 0 /* Used bit read */ > @@ -456,11 +465,17 @@ > #define MACB_REV_OFFSET 0 > #define MACB_REV_SIZE 16 > > +/* Bitfields in PCS_CONTROL. */ > +#define GEM_PCS_CTRL_RST_OFFSET 15 > +#define GEM_PCS_CTRL_RST_SIZE 1 > + > /* Bitfields in DCFG1. */ > #define GEM_IRQCOR_OFFSET 23 > #define GEM_IRQCOR_SIZE 1 > #define GEM_DBWDEF_OFFSET 25 > #define GEM_DBWDEF_SIZE 3 > +#define GEM_NO_PCS_OFFSET 0 > +#define GEM_NO_PCS_SIZE 1 > > /* Bitfields in DCFG2. */ > #define GEM_RX_PKT_BUFF_OFFSET 20 > @@ -633,19 +648,32 @@ > #define MACB_MAN_CODE 2 > > /* Capability mask bits */ > -#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001 > -#define MACB_CAPS_USRIO_HAS_CLKEN 0x00000002 > -#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII 0x00000004 > -#define MACB_CAPS_NO_GIGABIT_HALF 0x00000008 > -#define MACB_CAPS_USRIO_DISABLED 0x00000010 > -#define MACB_CAPS_JUMBO 0x00000020 > -#define MACB_CAPS_GEM_HAS_PTP 0x00000040 > -#define MACB_CAPS_BD_RD_PREFETCH 0x00000080 > -#define MACB_CAPS_NEEDS_RSTONUBR 0x00000100 > -#define MACB_CAPS_FIFO_MODE 0x10000000 > -#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000 > -#define MACB_CAPS_SG_DISABLED 0x40000000 > -#define MACB_CAPS_MACB_IS_GEM 0x80000000 > +#define MACB_CAPS_ISR_CLEAR_ON_WRITE BIT(0) > +#define MACB_CAPS_USRIO_HAS_CLKEN BIT(1) > +#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII BIT(2) > +#define MACB_CAPS_NO_GIGABIT_HALF BIT(3) > +#define MACB_CAPS_USRIO_DISABLED BIT(4) > +#define MACB_CAPS_JUMBO BIT(5) > +#define MACB_CAPS_GEM_HAS_PTP BIT(6) > +#define MACB_CAPS_BD_RD_PREFETCH BIT(7) > +#define MACB_CAPS_NEEDS_RSTONUBR BIT(8) > +#define MACB_CAPS_FIFO_MODE BIT(28) > +#define MACB_CAPS_GIGABIT_MODE_AVAILABLE BIT(29) > +#define MACB_CAPS_SG_DISABLED BIT(30) > +#define MACB_CAPS_MACB_IS_GEM BIT(31) > +#define MACB_CAPS_PCS BIT(24) > +#define MACB_CAPS_MACB_IS_GEM_GXL BIT(25) > + > +#define MACB_GEM7010_IDNUM 0x009 > +#define MACB_GEM7014_IDNU 0x107 > +#define MACB_GEM7014A_IDNUM 0x207 > +#define MACB_GEM7016_IDNUM 0x10a > +#define MACB_GEM7017_IDNUM 0x00a > +#define MACB_GEM7017A_IDNUM 0x20a > +#define MACB_GEM7020_IDNUM 0x003 > +#define MACB_GEM7021_IDNUM 0x00c > +#define MACB_GEM7021A_IDNUM 0x20c > +#define MACB_GEM7022_IDNUM 0x00b > > /* LSO settings */ > #define MACB_LSO_UFO_ENABLE 0x01 > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 1d6527a5313f..10d18b2cef31 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -445,15 +445,15 @@ static void gem_phylink_validate(struct phylink_config *pl_config, > __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > switch (state->interface) { > + case PHY_INTERFACE_MODE_NA: > + case PHY_INTERFACE_MODE_SGMII: > case PHY_INTERFACE_MODE_GMII: > case PHY_INTERFACE_MODE_RGMII: > if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { > phylink_set(mask, 1000baseT_Full); > phylink_set(mask, 1000baseX_Full); > - if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) { > + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) > phylink_set(mask, 1000baseT_Half); > - phylink_set(mask, 1000baseT_Half); > - } > } > /* fallthrough */ > case PHY_INTERFACE_MODE_MII: > @@ -469,7 +469,6 @@ static void gem_phylink_validate(struct phylink_config *pl_config, > > linkmode_and(supported, supported, mask); > linkmode_and(state->advertising, state->advertising, mask); > - > } > > static int gem_phylink_mac_link_state(struct phylink_config *pl_config, > @@ -483,19 +482,42 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode, > { > struct net_device *netdev = to_net_dev(pl_config->dev); > struct macb *bp = netdev_priv(netdev); > + bool change_interface = bp->phy_interface != state->interface; > unsigned long flags; > > spin_lock_irqsave(&bp->lock, flags); > > + if (change_interface) { > + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { > + gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) & > + ~GEM_BIT(PCSSEL) & > + gem_readl(bp, NCFGR)); > + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) & > + gem_readl(bp, NCR)); > + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | > + GEM_BIT(PCS_CTRL_RST)); > + } I still don't think this makes much sense, splitting the interface configuration between here and below. > + bp->phy_interface = state->interface; > + } > + > if (!phylink_autoneg_inband(mode) && > (bp->speed != state->speed || > - bp->duplex != state->duplex)) { > + bp->duplex != state->duplex || > + change_interface)) { > u32 reg; > > reg = macb_readl(bp, NCFGR); > reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD)); > if (macb_is_gem(bp)) > reg &= ~GEM_BIT(GBE); > + macb_or_gem_writel(bp, NCFGR, reg); > + > + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) > + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) | > + GEM_BIT(PCSSEL) | > + gem_readl(bp, NCFGR)); This will only be executed when we are not using inband mode, which basically means it's not possible to switch to SGMII in-band mode. > + > + reg = macb_readl(bp, NCFGR); > if (state->duplex) > reg |= MACB_BIT(FD); > > @@ -590,8 +612,8 @@ static int macb_mii_probe(struct net_device *dev) > } > > bp->link = 0; > - bp->speed = 0; > - bp->duplex = -1; > + bp->speed = SPEED_UNKNOWN; > + bp->duplex = DUPLEX_UNKNOWN; > > return ret; > } > @@ -3340,6 +3362,22 @@ static void macb_configure_caps(struct macb *bp, > dcfg = gem_readl(bp, DCFG1); > if (GEM_BFEXT(IRQCOR, dcfg) == 0) > bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE; > + if (GEM_BFEXT(NO_PCS, dcfg) == 0) > + bp->caps |= MACB_CAPS_PCS; > + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) { > + case MACB_GEM7016_IDNUM: > + case MACB_GEM7017_IDNUM: > + case MACB_GEM7017A_IDNUM: > + case MACB_GEM7020_IDNUM: > + case MACB_GEM7021_IDNUM: > + case MACB_GEM7021A_IDNUM: > + case MACB_GEM7022_IDNUM: > + bp->caps |= MACB_CAPS_USRIO_DISABLED; > + bp->caps |= MACB_CAPS_MACB_IS_GEM_GXL; > + break; > + default: > + break; > + } > dcfg = gem_readl(bp, DCFG2); > if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0) > bp->caps |= MACB_CAPS_FIFO_MODE; > @@ -4322,11 +4360,35 @@ static int macb_probe(struct platform_device *pdev) > } > > phy_mode = of_get_phy_mode(np); > - if (phy_mode < 0) > + if (phy_mode < 0) { > /* not found in DT, MII by default */ > bp->phy_interface = PHY_INTERFACE_MODE_MII; > - else > + } else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) { > + u32 interface_supported = 1; > + > + if (phy_mode == PHY_INTERFACE_MODE_SGMII) { > + if (!(bp->caps & MACB_CAPS_PCS)) > + interface_supported = 0; > + } else if (phy_mode == PHY_INTERFACE_MODE_GMII || > + phy_mode == PHY_INTERFACE_MODE_RGMII) { > + if (!macb_is_gem(bp)) > + interface_supported = 0; > + } else if (phy_mode != PHY_INTERFACE_MODE_RMII && > + phy_mode != PHY_INTERFACE_MODE_MII) { > + /* Add new mode before this */ > + interface_supported = 0; > + } > + > + if (!interface_supported) { > + netdev_err(dev, "Phy mode %s not supported", > + phy_modes(phy_mode)); > + goto err_out_free_netdev; > + } > + > bp->phy_interface = phy_mode; > + } else { > + bp->phy_interface = phy_mode; > + } If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config() is called with state->interface = PHY_INTERFACE_MODE_SGMII, then mac_config() won't configure the MAC for the interface type - is that intentional? > > /* IP specific init */ > err = init(pdev); > -- > 2.17.1 > >
>> + if (change_interface) { >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { >> + gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) & >> + ~GEM_BIT(PCSSEL) & >> + gem_readl(bp, NCFGR)); >> + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) & >> + gem_readl(bp, NCR)); >> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | >> + GEM_BIT(PCS_CTRL_RST)); >> + } >I still don't think this makes much sense, splitting the interface >configuration between here and below. Do you mean splitting mac_config in two *_configure functions ? This was done as per Andrew's suggestion to make code mode readable and easy to manage by splitting MAC configuration for different interfaces. >> + bp->phy_interface = state->interface; >> + } >> + >> if (!phylink_autoneg_inband(mode) && >> (bp->speed != state->speed || >> - bp->duplex != state->duplex)) { >> + bp->duplex != state->duplex || >> + change_interface)) { >> u32 reg; >> >> reg = macb_readl(bp, NCFGR); >> reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD)); >> if (macb_is_gem(bp)) >> reg &= ~GEM_BIT(GBE); >> + macb_or_gem_writel(bp, NCFGR, reg); >> + >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) >> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) | >> + GEM_BIT(PCSSEL) | >> + gem_readl(bp, NCFGR)); >This will only be executed when we are not using inband mode, which >basically means it's not possible to switch to SGMII in-band mode. SGMII is used in default PHY mode. And above code is to program MAC to select PCS and SGMII interface. >> + >> + if (!interface_supported) { >> + netdev_err(dev, "Phy mode %s not supported", >> + phy_modes(phy_mode)); >> + goto err_out_free_netdev; >> + } >> + >> bp->phy_interface = phy_mode; >> + } else { >> + bp->phy_interface = phy_mode; >> + } >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config() >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then >mac_config() won't configure the MAC for the interface type - is that >intentional? In mac_config configure MAC for non in-band mode, there is also check for speed, duplex changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and DUPLEX_UNKNOWN values so it is expected that for non in band mode state contains valid speed and duplex mode which are different from *_UNKNOWN values. Regards, Parshuram Thombare
On Mon, Jun 24, 2019 at 06:35:44AM +0000, Parshuram Raju Thombare wrote: > > >> + if (change_interface) { > >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { > >> + gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) & > >> + ~GEM_BIT(PCSSEL) & > >> + gem_readl(bp, NCFGR)); > >> + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) & > >> + gem_readl(bp, NCR)); > >> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | > >> + GEM_BIT(PCS_CTRL_RST)); > >> + } > >I still don't think this makes much sense, splitting the interface > >configuration between here and below. > Do you mean splitting mac_config in two *_configure functions ? > This was done as per Andrew's suggestion to make code mode readable > and easy to manage by splitting MAC configuration for different interfaces. No, I mean here you disable SGMII if we're switching away from SGMII mode.... (note, this means there is more to come for this sentence) > > >> + bp->phy_interface = state->interface; > >> + } > >> + > >> if (!phylink_autoneg_inband(mode) && > >> (bp->speed != state->speed || > >> - bp->duplex != state->duplex)) { > >> + bp->duplex != state->duplex || > >> + change_interface)) { > >> u32 reg; > >> > >> reg = macb_readl(bp, NCFGR); > >> reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD)); > >> if (macb_is_gem(bp)) > >> reg &= ~GEM_BIT(GBE); > >> + macb_or_gem_writel(bp, NCFGR, reg); > >> + > >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) > >> + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) | > >> + GEM_BIT(PCSSEL) | > >> + gem_readl(bp, NCFGR)); > >This will only be executed when we are not using inband mode, which > >basically means it's not possible to switch to SGMII in-band mode. > SGMII is used in default PHY mode. And above code is to program MAC to > select PCS and SGMII interface. ... and here you enable it for SGMII mode, but only for non-inband modes. For inband modes, you do not have any code that enables SGMII mode. Since the only inband mode you support is SGMII, this is not very good behaviour. Why not: if (change_interface) { u32 ncfgr; bp->phy_interface = state->interface; // We don't support 2.5G modes gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) & gem_readl(bp, NCR)); ncfgr = gem_readl(bp, NCFGR); if (state->interface == PHY_INTERFACE_MODE_SGMII) { // Enable SGMII mode and PCS gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL)); } else { // Disable SGMII mode and PCS gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL))); // Reset PCS gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | GEM_BIT(PCS_CTRL_RST)); } } if (!phylink_autoneg_inband(mode) && (bp->speed != state->speed || bp->duplex != state->duplex)) { ? > > >> + > >> + if (!interface_supported) { > >> + netdev_err(dev, "Phy mode %s not supported", > >> + phy_modes(phy_mode)); > >> + goto err_out_free_netdev; > >> + } > >> + > >> bp->phy_interface = phy_mode; > >> + } else { > >> + bp->phy_interface = phy_mode; > >> + } > >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config() > >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then > >mac_config() won't configure the MAC for the interface type - is that > >intentional? > > In mac_config configure MAC for non in-band mode, there is also check for speed, duplex > changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and DUPLEX_UNKNOWN > values so it is expected that for non in band mode state contains valid speed and duplex mode > which are different from *_UNKNOWN values. Sorry, this reply doesn't answer my question. I'm not asking about bp->speed and bp->duplex. I'm asking: 1) why you are initialising bp->phy_interface here 2) you to consider the impact that has on the mac_config() implementation you are proposing because I think it's buggy.
>> >I still don't think this makes much sense, splitting the interface >> > configuration between here and below. >> Do you mean splitting mac_config in two *_configure functions ? >> This was done as per Andrew's suggestion to make code mode readable >> and easy to manage by splitting MAC configuration for different interfaces. >No, I mean here you disable SGMII if we're switching away from SGMII >mode.... (note, this means there is more to come for this sentence) Sorry, I misunderstood your original question. I think disabling old interface and enabling new one can be done in single place. I will do this change. >> >This will only be executed when we are not using inband mode, which >> >basically means it's not possible to switch to SGMII in-band mode. >> SGMII is used in default PHY mode. And above code is to program MAC to >> select PCS and SGMII interface. >... and here you enable it for SGMII mode, but only for non-inband >modes. > >Why not: > if (change_interface) { > if (state->interface == PHY_INTERFACE_MODE_SGMII) { > // Enable SGMII mode and PCS > gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) | > GEM_BIT(PCSSEL)); > } else { > // Disable SGMII mode and PCS > gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN) > GEM_BIT(PCSSEL))); > // Reset PCS > gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) > GEM_BIT(PCS_CTRL_RST)); > } > } > if (!phylink_autoneg_inband(mode) && > (bp->speed != state->speed || bp->duplex != state->duplex)) { >? Ok >> >> + >> >> + if (!interface_supported) { >> >> + netdev_err(dev, "Phy mode %s not supported", >> >> + phy_modes(phy_mode)); >> >> + goto err_out_free_netdev; >> >> + } >> >> + >> >> bp->phy_interface = phy_mode; >> >> + } else { >> >> + bp->phy_interface = phy_mode; >> >> + } >> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and >> > mac_config() >> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then >> >mac_config() won't configure the MAC for the interface type - is that >> >intentional? >> In mac_config configure MAC for non in-band mode, there is also check for >> speed, duplex >> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN >> and DUPLEX_UNKNOWN >> values so it is expected that for non in band mode state contains valid speed >> and duplex mode >> which are different from *_UNKNOWN values. >Sorry, this reply doesn't answer my question. I'm not asking about >bp->speed and bp->duplex. I'm asking: >1) why you are initialising bp->phy_interface here >2) you to consider the impact that has on the mac_config() implementation > you are proposing > because I think it's buggy. bp->phy_interface is to store phy mode value from device tree. This is used later to know what phy interface user has selected for PHY-MAC. Same is used to configure MAC correctly and based on your suggestion code is added to handle PHY dynamically changing phy interface, in which case bp->phy_interface is also updated. Though it may not be what user want, if phy interface is totally decided by PHY and is anyway going to be different from what user has selected in DT, initializing it here doesn't make sense. But in case of PHY not changing phy_interface dynamically bp->phy_interface need to be initialized with value from DT. Regards, Parshuram Thombare
On Mon, Jun 24, 2019 at 10:14:41AM +0000, Parshuram Raju Thombare wrote: > >> >I still don't think this makes much sense, splitting the interface > >> > configuration between here and below. > >> Do you mean splitting mac_config in two *_configure functions ? > >> This was done as per Andrew's suggestion to make code mode readable > >> and easy to manage by splitting MAC configuration for different interfaces. > >No, I mean here you disable SGMII if we're switching away from SGMII > >mode.... (note, this means there is more to come for this sentence) > Sorry, I misunderstood your original question. I think disabling old interface > and enabling new one can be done in single place. I will do this change. > > >> >This will only be executed when we are not using inband mode, which > >> >basically means it's not possible to switch to SGMII in-band mode. > >> SGMII is used in default PHY mode. And above code is to program MAC to > >> select PCS and SGMII interface. > >... and here you enable it for SGMII mode, but only for non-inband > >modes. > > > >Why not: > > if (change_interface) { > > if (state->interface == PHY_INTERFACE_MODE_SGMII) { > > // Enable SGMII mode and PCS > > gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) | > > GEM_BIT(PCSSEL)); > > } else { > > // Disable SGMII mode and PCS > > gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN) > > GEM_BIT(PCSSEL))); > > // Reset PCS > > gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) > > GEM_BIT(PCS_CTRL_RST)); > > } > > } > > if (!phylink_autoneg_inband(mode) && > > (bp->speed != state->speed || bp->duplex != state->duplex)) { > >? > Ok > > >> >> + > >> >> + if (!interface_supported) { > >> >> + netdev_err(dev, "Phy mode %s not supported", > >> >> + phy_modes(phy_mode)); > >> >> + goto err_out_free_netdev; > >> >> + } > >> >> + > >> >> bp->phy_interface = phy_mode; > >> >> + } else { > >> >> + bp->phy_interface = phy_mode; > >> >> + } > >> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and > >> > mac_config() > >> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then > >> >mac_config() won't configure the MAC for the interface type - is that > >> >intentional? > >> In mac_config configure MAC for non in-band mode, there is also check for > >> speed, duplex > >> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN > >> and DUPLEX_UNKNOWN > >> values so it is expected that for non in band mode state contains valid speed > >> and duplex mode > >> which are different from *_UNKNOWN values. > > >Sorry, this reply doesn't answer my question. I'm not asking about > >bp->speed and bp->duplex. I'm asking: > >1) why you are initialising bp->phy_interface here > >2) you to consider the impact that has on the mac_config() implementation > > you are proposing > > because I think it's buggy. > bp->phy_interface is to store phy mode value from device tree. This is used later > to know what phy interface user has selected for PHY-MAC. Same is used > to configure MAC correctly and based on your suggestion code is > added to handle PHY dynamically changing phy interface, in which > case bp->phy_interface is also updated. Though it may not be what user want, > if phy interface is totally decided by PHY and is anyway going to be different from what user > has selected in DT, initializing it here doesn't make sense. > But in case of PHY not changing phy_interface dynamically bp->phy_interface need to be > initialized with value from DT. When phylink_start() is called, you will receive a mac_config() call to configure the MAC for the initial operating settings, which will include the current PHY interface mode. This will initially be whatever interface mode was passed in to phylink_create().
>> >Sorry, this reply doesn't answer my question. I'm not asking about >> >bp->speed and bp->duplex. I'm asking: >> >1) why you are initialising bp->phy_interface here >> >2) you to consider the impact that has on the mac_config() implementation >> > you are proposing >> > because I think it's buggy. >> bp->phy_interface is to store phy mode value from device tree. This is used >later >> to know what phy interface user has selected for PHY-MAC. Same is used >> to configure MAC correctly and based on your suggestion code is >> added to handle PHY dynamically changing phy interface, in which >> case bp->phy_interface is also updated. Though it may not be what user >> want, >> if phy interface is totally decided by PHY and is anyway going to be different >> from what user has selected in DT, initializing it here doesn't make sense. >> But in case of PHY not changing phy_interface dynamically bp- >>phy_interface need to be initialized with value from DT. >When phylink_start() is called, you will receive a mac_config() call to >configure the MAC for the initial operating settings, which will include >the current PHY interface mode. This will initially be whatever >interface mode was passed in to phylink_create(). Yes, and same bp->phy_interface is passed to phylink_create(). We need this to know what phy interface is selected by user. Regards, Parshuram Thombare
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 8629d345af31..6d268283c318 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -77,6 +77,7 @@ #define MACB_RBQPH 0x04D4 /* GEM register offsets. */ +#define GEM_NCR 0x0000 /* Network Control */ #define GEM_NCFGR 0x0004 /* Network Config */ #define GEM_USRIO 0x000c /* User IO */ #define GEM_DMACFG 0x0010 /* DMA Configuration */ @@ -156,6 +157,7 @@ #define GEM_PEFTN 0x01f4 /* PTP Peer Event Frame Tx Ns */ #define GEM_PEFRSL 0x01f8 /* PTP Peer Event Frame Rx Sec Low */ #define GEM_PEFRN 0x01fc /* PTP Peer Event Frame Rx Ns */ +#define GEM_PCS_CTRL 0x0200 /* PCS Control */ #define GEM_DCFG1 0x0280 /* Design Config 1 */ #define GEM_DCFG2 0x0284 /* Design Config 2 */ #define GEM_DCFG3 0x0288 /* Design Config 3 */ @@ -271,6 +273,10 @@ #define MACB_IRXFCS_OFFSET 19 #define MACB_IRXFCS_SIZE 1 +/* GEM specific NCR bitfields. */ +#define GEM_TWO_PT_FIVE_GIG_OFFSET 29 +#define GEM_TWO_PT_FIVE_GIG_SIZE 1 + /* GEM specific NCFGR bitfields. */ #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */ #define GEM_GBE_SIZE 1 @@ -323,6 +329,9 @@ #define MACB_MDIO_SIZE 1 #define MACB_IDLE_OFFSET 2 /* The PHY management logic is idle */ #define MACB_IDLE_SIZE 1 +#define MACB_DUPLEX_OFFSET 3 +#define MACB_DUPLEX_SIZE 1 + /* Bitfields in TSR */ #define MACB_UBR_OFFSET 0 /* Used bit read */ @@ -456,11 +465,17 @@ #define MACB_REV_OFFSET 0 #define MACB_REV_SIZE 16 +/* Bitfields in PCS_CONTROL. */ +#define GEM_PCS_CTRL_RST_OFFSET 15 +#define GEM_PCS_CTRL_RST_SIZE 1 + /* Bitfields in DCFG1. */ #define GEM_IRQCOR_OFFSET 23 #define GEM_IRQCOR_SIZE 1 #define GEM_DBWDEF_OFFSET 25 #define GEM_DBWDEF_SIZE 3 +#define GEM_NO_PCS_OFFSET 0 +#define GEM_NO_PCS_SIZE 1 /* Bitfields in DCFG2. */ #define GEM_RX_PKT_BUFF_OFFSET 20 @@ -633,19 +648,32 @@ #define MACB_MAN_CODE 2 /* Capability mask bits */ -#define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001 -#define MACB_CAPS_USRIO_HAS_CLKEN 0x00000002 -#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII 0x00000004 -#define MACB_CAPS_NO_GIGABIT_HALF 0x00000008 -#define MACB_CAPS_USRIO_DISABLED 0x00000010 -#define MACB_CAPS_JUMBO 0x00000020 -#define MACB_CAPS_GEM_HAS_PTP 0x00000040 -#define MACB_CAPS_BD_RD_PREFETCH 0x00000080 -#define MACB_CAPS_NEEDS_RSTONUBR 0x00000100 -#define MACB_CAPS_FIFO_MODE 0x10000000 -#define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000 -#define MACB_CAPS_SG_DISABLED 0x40000000 -#define MACB_CAPS_MACB_IS_GEM 0x80000000 +#define MACB_CAPS_ISR_CLEAR_ON_WRITE BIT(0) +#define MACB_CAPS_USRIO_HAS_CLKEN BIT(1) +#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII BIT(2) +#define MACB_CAPS_NO_GIGABIT_HALF BIT(3) +#define MACB_CAPS_USRIO_DISABLED BIT(4) +#define MACB_CAPS_JUMBO BIT(5) +#define MACB_CAPS_GEM_HAS_PTP BIT(6) +#define MACB_CAPS_BD_RD_PREFETCH BIT(7) +#define MACB_CAPS_NEEDS_RSTONUBR BIT(8) +#define MACB_CAPS_FIFO_MODE BIT(28) +#define MACB_CAPS_GIGABIT_MODE_AVAILABLE BIT(29) +#define MACB_CAPS_SG_DISABLED BIT(30) +#define MACB_CAPS_MACB_IS_GEM BIT(31) +#define MACB_CAPS_PCS BIT(24) +#define MACB_CAPS_MACB_IS_GEM_GXL BIT(25) + +#define MACB_GEM7010_IDNUM 0x009 +#define MACB_GEM7014_IDNU 0x107 +#define MACB_GEM7014A_IDNUM 0x207 +#define MACB_GEM7016_IDNUM 0x10a +#define MACB_GEM7017_IDNUM 0x00a +#define MACB_GEM7017A_IDNUM 0x20a +#define MACB_GEM7020_IDNUM 0x003 +#define MACB_GEM7021_IDNUM 0x00c +#define MACB_GEM7021A_IDNUM 0x20c +#define MACB_GEM7022_IDNUM 0x00b /* LSO settings */ #define MACB_LSO_UFO_ENABLE 0x01 diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 1d6527a5313f..10d18b2cef31 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -445,15 +445,15 @@ static void gem_phylink_validate(struct phylink_config *pl_config, __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; switch (state->interface) { + case PHY_INTERFACE_MODE_NA: + case PHY_INTERFACE_MODE_SGMII: case PHY_INTERFACE_MODE_GMII: case PHY_INTERFACE_MODE_RGMII: if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { phylink_set(mask, 1000baseT_Full); phylink_set(mask, 1000baseX_Full); - if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) { + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) phylink_set(mask, 1000baseT_Half); - phylink_set(mask, 1000baseT_Half); - } } /* fallthrough */ case PHY_INTERFACE_MODE_MII: @@ -469,7 +469,6 @@ static void gem_phylink_validate(struct phylink_config *pl_config, linkmode_and(supported, supported, mask); linkmode_and(state->advertising, state->advertising, mask); - } static int gem_phylink_mac_link_state(struct phylink_config *pl_config, @@ -483,19 +482,42 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode, { struct net_device *netdev = to_net_dev(pl_config->dev); struct macb *bp = netdev_priv(netdev); + bool change_interface = bp->phy_interface != state->interface; unsigned long flags; spin_lock_irqsave(&bp->lock, flags); + if (change_interface) { + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { + gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) & + ~GEM_BIT(PCSSEL) & + gem_readl(bp, NCFGR)); + gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) & + gem_readl(bp, NCR)); + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | + GEM_BIT(PCS_CTRL_RST)); + } + bp->phy_interface = state->interface; + } + if (!phylink_autoneg_inband(mode) && (bp->speed != state->speed || - bp->duplex != state->duplex)) { + bp->duplex != state->duplex || + change_interface)) { u32 reg; reg = macb_readl(bp, NCFGR); reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD)); if (macb_is_gem(bp)) reg &= ~GEM_BIT(GBE); + macb_or_gem_writel(bp, NCFGR, reg); + + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) + gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) | + GEM_BIT(PCSSEL) | + gem_readl(bp, NCFGR)); + + reg = macb_readl(bp, NCFGR); if (state->duplex) reg |= MACB_BIT(FD); @@ -590,8 +612,8 @@ static int macb_mii_probe(struct net_device *dev) } bp->link = 0; - bp->speed = 0; - bp->duplex = -1; + bp->speed = SPEED_UNKNOWN; + bp->duplex = DUPLEX_UNKNOWN; return ret; } @@ -3340,6 +3362,22 @@ static void macb_configure_caps(struct macb *bp, dcfg = gem_readl(bp, DCFG1); if (GEM_BFEXT(IRQCOR, dcfg) == 0) bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE; + if (GEM_BFEXT(NO_PCS, dcfg) == 0) + bp->caps |= MACB_CAPS_PCS; + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) { + case MACB_GEM7016_IDNUM: + case MACB_GEM7017_IDNUM: + case MACB_GEM7017A_IDNUM: + case MACB_GEM7020_IDNUM: + case MACB_GEM7021_IDNUM: + case MACB_GEM7021A_IDNUM: + case MACB_GEM7022_IDNUM: + bp->caps |= MACB_CAPS_USRIO_DISABLED; + bp->caps |= MACB_CAPS_MACB_IS_GEM_GXL; + break; + default: + break; + } dcfg = gem_readl(bp, DCFG2); if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0) bp->caps |= MACB_CAPS_FIFO_MODE; @@ -4322,11 +4360,35 @@ static int macb_probe(struct platform_device *pdev) } phy_mode = of_get_phy_mode(np); - if (phy_mode < 0) + if (phy_mode < 0) { /* not found in DT, MII by default */ bp->phy_interface = PHY_INTERFACE_MODE_MII; - else + } else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) { + u32 interface_supported = 1; + + if (phy_mode == PHY_INTERFACE_MODE_SGMII) { + if (!(bp->caps & MACB_CAPS_PCS)) + interface_supported = 0; + } else if (phy_mode == PHY_INTERFACE_MODE_GMII || + phy_mode == PHY_INTERFACE_MODE_RGMII) { + if (!macb_is_gem(bp)) + interface_supported = 0; + } else if (phy_mode != PHY_INTERFACE_MODE_RMII && + phy_mode != PHY_INTERFACE_MODE_MII) { + /* Add new mode before this */ + interface_supported = 0; + } + + if (!interface_supported) { + netdev_err(dev, "Phy mode %s not supported", + phy_modes(phy_mode)); + goto err_out_free_netdev; + } + bp->phy_interface = phy_mode; + } else { + bp->phy_interface = phy_mode; + } /* IP specific init */ err = init(pdev);
This patch add support for SGMII interface) and 2.5Gbps MAC in Cadence ethernet controller driver. Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/net/ethernet/cadence/macb.h | 54 ++++++++++++---- drivers/net/ethernet/cadence/macb_main.c | 80 +++++++++++++++++++++--- 2 files changed, 112 insertions(+), 22 deletions(-)