> +static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data) > { > - int err, reg; > + int reg; > > - /* Disable BroadR-Reach function. */ > reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL); > - reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN; > - err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, > - reg); > - if (err < 0) bcm_phy_read_exp() could fail. So you should keep the test. Also, the caller of this function does look at the return value. > +/** > + * bcm5481x_read_abilities - read PHY abilities from LRESR or Clause 22 > + * (BMSR) registers, based on whether the PHY is in BroadR-Reach or IEEE mode > + * @phydev: target phy_device struct > + * > + * Description: Reads the PHY's abilities and populates > + * phydev->supported accordingly. The register to read the abilities from is > + * determined by current brr mode setting of the PHY. > + * Note that the LRE and IEEE sets of abilities are disjunct. > + * > + * Returns: 0 on success, < 0 on failure > + */ > +static int bcm5481x_read_abilities(struct phy_device *phydev) > +{ > + int i, val, err; > + u8 brr_mode; > + > + for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++) > + linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported); > + > + err = bcm5481x_get_brrmode(phydev, &brr_mode); > +static int bcm5481x_set_brrmode(struct phy_device *phydev, bool on) > +{ > + int reg; > + int err; > + u16 val; > + > + reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL); > + > + if (on) > + reg |= BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN; > + else > + reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN; > + > +static int bcm54811_config_init(struct phy_device *phydev) > +{ > + struct device_node *np = phydev->mdio.dev.of_node; > + bool brr = false; > + int err, reg; > + > err = bcm54xx_config_init(phydev); > > /* Enable CLK125 MUX on LED4 if ref clock is enabled. */ > @@ -576,29 +687,80 @@ static int bcm54811_config_init(struct phy_device *phydev) > return err; > } > > - return err; > + /* Configure BroadR-Reach function. */ > + brr = of_property_read_bool(np, "brr-mode"); > + > + /* With BCM54811, BroadR-Reach implies no autoneg */ > + if (brr) > + phydev->autoneg = 0; > + > + return bcm5481x_set_brrmode(phydev, brr); > } The ordering seems a bit strange here. phy_probe() will call phydrv->get_features. At this point, the PHY is in whatever mode it resets to, or maybe what it is strapped to. phydev->supported could thus be set to standard IEEE modes, despite the board design is actually for BroadR-Reach. Sometime later, when the MAC is connected to the PHY config_init() is called. At that point, you poke around in DT and find how the PHY is connected to the cable. At that point, you set the PHY mode, and change phydev->supported to reflect reality. I really think that reading DT should be done much earlier, maybe in the driver probe function, or maybe get_features. get_features should always return the correct values from the board. > +static int bcm5481_config_aneg(struct phy_device *phydev) > +{ > + u8 brr_mode; > + int ret; > + > + ret = bcm5481x_get_brrmode(phydev, &brr_mode); Rather than read it from the hardware every single time, could you store the DT value in bcm54xx_phy_priv ? > +/* Read LDS Link Partner Ability in BroadR-Reach mode */ > +static int bcm_read_lpa(struct phy_device *phydev) This function seems to be missing an lds or lre prefix. > +static int bcm_read_status_fixed(struct phy_device *phydev) and here. Please make sure the naming is consistent. Anything which only accesses lre or lds registers should make that clear in its name. > +static int bcm54811_read_status(struct phy_device *phydev) > +{ > + u8 brr_mode; > + int err; > + > + err = bcm5481x_get_brrmode(phydev, &brr_mode); > + > + if (err) > + return err; > + > + if (brr_mode) { > + /* Get the status in BroadRReach mode just like > + * genphy_read_status does in normal mode > + */ > + > + int err, old_link = phydev->link; > + > + /* Update the link, but return if there was an error */ > + > + err = lre_update_link(phydev); > + if (err) > + return err; > + > + /* why bother the PHY if nothing can have changed */ > + if (phydev->autoneg == > + AUTONEG_ENABLE && old_link && phydev->link) > + return 0; > + > + phydev->speed = SPEED_UNKNOWN; > + phydev->duplex = DUPLEX_UNKNOWN; > + phydev->pause = 0; > + phydev->asym_pause = 0; > + > + err = bcm_read_master_slave(phydev); > + if (err < 0) > + return err; > + > + /* Read LDS Link Partner Ability */ > + err = bcm_read_lpa(phydev); > + if (err < 0) > + return err; > + > + if (phydev->autoneg == > + AUTONEG_ENABLE && phydev->autoneg_complete) { > + phy_resolve_aneg_linkmode(phydev); > + } else if (phydev->autoneg == AUTONEG_DISABLE) { > + err = bcm_read_status_fixed(phydev); > + if (err < 0) > + return err; > + } This would probably look better if you pulled this code out into a helper bcm54811_lre_read_status(). Andrew --- pw-bot: cr