>From: Russell King - ARM Linux admin <li...@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
