Messages in this thread | | | Date | Mon, 17 Jun 2019 17:01:45 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH 2/6] net: macb: add support for sgmii MAC-PHY interface |
| |
> @@ -159,6 +160,9 @@ > #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_PCS_STATUS 0x0204 /* PCS Status */ > +#define GEM_PCS_AN_LP_BASE 0x0214 /* PCS AN LP BASE*/
It looks like there are some space vs tab issues here and else where.
> static int gem_phylink_mac_link_state(struct phylink_config *pl_config, > struct phylink_link_state *state) > { > + u32 status; > struct net_device *netdev = to_net_dev(pl_config->dev); > struct macb *bp = netdev_priv(netdev);
Reverse christmas tree please, here and everywhere you add new variables.
> > - state->speed = bp->speed; > - state->duplex = bp->duplex; > - state->link = bp->link; > + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { > + status = gem_readl(bp, PCS_STATUS); > + state->an_complete = GEM_BFEXT(PCS_STATUS_AN_DONE, status); > + status = gem_readl(bp, PCS_AN_LP_BASE); > + switch (GEM_BFEXT(PCS_AN_LP_BASE_SPEED, status)) { > + case 0: > + state->speed = 10; > + break; > + case 1: > + state->speed = 100; > + break; > + case 2: > + state->speed = 1000; > + break; > + default: > + break;
It would be nice to use SPEED_10, SPEED_100, etc.
> @@ -494,17 +551,23 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode, > reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD)); > if (macb_is_gem(bp)) > reg &= ~GEM_BIT(GBE); > - > if (state->duplex) > reg |= MACB_BIT(FD); > - if (state->speed == SPEED_100) > - reg |= MACB_BIT(SPD); > - if (state->speed == SPEED_1000 && > - bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) > - reg |= GEM_BIT(GBE); > - > macb_or_gem_writel(bp, NCFGR, reg); > > + if (state->speed == SPEED_2500) { > + gem_writel(bp, NCFGR, GEM_BIT(GBE) | > + gem_readl(bp, NCFGR)); > + gem_writel(bp, NCR, GEM_BIT(TWO_PT_FIVE_GIG) | > + gem_readl(bp, NCR)); > + } else if (state->speed == SPEED_1000) { > + gem_writel(bp, NCFGR, GEM_BIT(GBE) | > + gem_readl(bp, NCFGR)); > + } else if (state->speed == SPEED_100) { > + macb_writel(bp, NCFGR, MACB_BIT(SPD) | > + macb_readl(bp, NCFGR)); > + }
Maybe a switch statement?
> @@ -4232,11 +4327,37 @@ static int macb_probe(struct platform_device *pdev) > } > > err = of_get_phy_mode(np);
The following code would be more readable if you replaced err with phy_mode, or interface.
> - if (err < 0) > + if (err < 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 (err == PHY_INTERFACE_MODE_SGMII || > + err == PHY_INTERFACE_MODE_1000BASEX || > + err == PHY_INTERFACE_MODE_2500BASEX) { > + if (!(bp->caps & MACB_CAPS_PCS)) > + interface_supported = 0; > + } else if (err == PHY_INTERFACE_MODE_GMII || > + err == PHY_INTERFACE_MODE_RGMII) { > + if (!macb_is_gem(bp)) > + interface_supported = 0; > + } else if (err != PHY_INTERFACE_MODE_RMII && > + err != 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(err)); > + goto err_out_free_netdev; > + } > + > bp->phy_interface = err; > + } else { > + bp->phy_interface = err; > + }
Andrew
| |