Messages in this thread | | | From | Parshuram Raju Thombare <> | Subject | RE: [PATCH v2 2/5] net: macb: add support for sgmii MAC-PHY interface | Date | Wed, 19 Jun 2019 11:23:01 +0000 |
| |
>From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > >On Wed, Jun 19, 2019 at 09:40:46AM +0100, Parshuram Thombare wrote: > >> This patch add support for SGMII interface) and > >> 2.5Gbps MAC in Cadence ethernet controller driver.
>> switch (state->interface) { > >> + case PHY_INTERFACE_MODE_SGMII: > >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) > >> + phylink_set(mask, 2500baseT_Full); > > > >This doesn't look correct to me. SGMII as defined by Cisco only >supports 1G, 100M and 10M speeds, not 2.5G.
Cadence MAC support 2.5G SGMII by using higher clock frequency. Even > >Even so, SGMII is not limited to just base-T - PHYs are free to offer >base-X to SGMII conversion too.
Ok, I will make change to allow 1000BASE-X and 2500BASE-X In SGMII mode.
>> + case PHY_INTERFACE_MODE_2500BASEX: > >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) > >> + phylink_set(mask, 2500baseX_Full); > >> + /* fallthrough */ > >> + case PHY_INTERFACE_MODE_1000BASEX: > >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) > >> + phylink_set(mask, 1000baseX_Full); > >> + break; > > > >Please see how other drivers which use phylink deal with the validate() >format, and please read the phylink documentation: > > * Note that the PHY may be able to transform from one connection > * technology to another, so, eg, don't clear 1000BaseX just > * because the MAC is unable to BaseX mode. This is more about > * clearing unsupported speeds and duplex settings. >
There are some configs used in this driver which limits MAC speed. Above checks just to make sure this use case does not break.
>> + state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp, >NSR)); >> + state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR)); >> + } else if (bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) { >> + state->speed = SPEED_2500; >> + state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp, >NSR)); >> + state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR)); >> + } else if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX) { >> + state->speed = SPEED_1000; >> + state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp, >NSR)); >> + state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR)); >> + } > > > >So if the phy_interface type is not one listed, we leave state alone? >That doesn't seem good. It looks like you should at least simply set >state->duplex and state->link according to the NSR register content, >and always derive the speed.
Ok, I will make that change
> >It would also be good to set state->lp_advertising if you have access >to that so ethtool can report the link partner's abilities. Current >Marvell drivers that use phylink don't do that because that information >is not available from the hardware. > Link partner ability information is available only for SGMII, I will add change to populate state->lp_ advertising for SGMII.
>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII || >> + bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX || >> + bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) { >> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | >> + GEM_BIT(PCS_CTRL_RESTART_AN)); >> + } >This will only be called for 802.3z link modes, so you don't need these >checks.
Ok, I will remove these checks.
>> @@ -506,18 +563,26 @@ static void gem_mac_config(struct phylink_config >*pl_config, unsigned int mode, >> switch (state->speed) { >> + case SPEED_2500: >> + gem_writel(bp, NCFGR, GEM_BIT(GBE) | >> + gem_readl(bp, NCFGR)); >> } >> - macb_or_gem_writel(bp, NCFGR, reg); >> >> bp->speed = state->speed; >> bp->duplex = state->duplex; > > > >This is not going to work for 802.3z nor SGMII properly when in-band >negotiation is used. We don't know ahead of time what the speed and >duplex will be. Please see existing drivers for examples showing >how mac_config() should be implemented (there's good reason why its >laid out as it is in those drivers.) > Ok, Here I will configure MAC only for FIXED and PHY mode.
> >> @@ -555,6 +620,7 @@ static void gem_mac_link_down(struct >phylink_config *pl_config, > >> static const struct phylink_mac_ops gem_phylink_ops = { > >> .validate = gem_phylink_validate, > >> .mac_link_state = gem_phylink_mac_link_state, > >> + .mac_an_restart = gem_mac_an_restart, > >> .mac_config = gem_mac_config, > >> .mac_link_up = gem_mac_link_up, > >> .mac_link_down = gem_mac_link_down, > >> @@ -2248,7 +2314,9 @@ static void macb_init_hw(struct macb *bp) > >> macb_set_hwaddr(bp); > >> > >> config = macb_mdc_clk_div(bp); > >> - if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) > >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII || > >> + bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX || > >> + bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) > >> config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL); > > > >Configuration of the phy interface mode should be done in mac_config() >as previously mentioned, some PHYs can change their link mode at run >time. Hotplugging SFPs can change the link mode between SGMII and >base-X too. Ok
>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII || > >> + bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX || > >> + bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) { > >> + //Enable PCS AN > >> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | > >> + GEM_BIT(PCS_CTRL_EN_AN)); > >> + //Reset PCS block > >> + gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) | > >> + GEM_BIT(PCS_CTRL_RST)); > >> + } > >> + > > > >Should be in mac_config. > Ok
> >> - if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) > >> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII || > >> + bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX || > >> + bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) > >> val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL); > > > >Should be in mac_config. > Ok
Regards, Parshuram Thombare
| |