On Tue, 5 Nov 2024 08:41:38 +0000
liwencheng <liwench...@phytium.com.cn> wrote:
> +
> +int genphy_read_status(struct phy_device *phydev)
> +{
> + struct macb *bp = phydev->bp;
> + uint16_t bmcr, bmsr, ctrl1000 = 0, stat1000 = 0;
> + uint32_t advertising, lp_advertising;
> + uint32_t nego;
> + uint16_t phyad = phydev->phyad;
> +
> + /* Do a fake read */
> + bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR);
> +
> + bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR);
> + bmcr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMCR);
> +
> + if (bmcr & BMCR_ANENABLE) {
> + ctrl1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_CTRL1000);
> + stat1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_STAT1000);
> +
> + advertising = ADVERTISED_Autoneg;
> + advertising |= genphy_get_an(bp, phyad, GENERIC_PHY_ADVERISE);
> + advertising |= genphy_ctrl1000_to_ethtool_adv_t(ctrl1000);
> +
> + if (bmsr & BMSR_ANEGCOMPLETE) {
> + lp_advertising = genphy_get_an(bp, phyad,
> GENERIC_PHY_LPA);
> + lp_advertising |=
> genphy_stat1000_to_ethtool_lpa_t(stat1000);
> + } else {
> + lp_advertising = 0;
> + }
> +
> + nego = advertising & lp_advertising;
> + if (nego & (ADVERTISED_1000baseT_Full |
> ADVERTISED_1000baseT_Half)) {
> + phydev->speed = SPEED_1000;
> + phydev->duplex = !!(nego & ADVERTISED_1000baseT_Full);
> + } else if (nego &
> + (ADVERTISED_100baseT_Full |
> ADVERTISED_100baseT_Half)) {
> + phydev->speed = SPEED_100;
> + phydev->duplex = !!(nego & ADVERTISED_100baseT_Full);
> + } else {
> + phydev->speed = SPEED_10;
> + phydev->duplex = !!(nego & ADVERTISED_10baseT_Full);
> + }
> + } else {
> + phydev->speed = ((bmcr & BMCR_SPEED1000 && (bmcr &
> BMCR_SPEED100) == 0)
> + ? SPEED_1000
> + : ((bmcr & BMCR_SPEED100) ?
> SPEED_100 : SPEED_10));
> + phydev->duplex = (bmcr & BMCR_FULLDPLX) ? DUPLEX_FULL :
> DUPLEX_HALF;
> + }
> +
> + return 0;
> +}
Always returns 0 can be void function?
> +int macb_usxgmii_pcs_resume(struct phy_device *phydev)
> +{
> + u32 config;
> + struct macb *bp = phydev->bp;
> +
> + config = gem_readl(bp, USX_CONTROL);
> +
> + /* enable signal */
> + config &= ~(GEM_BIT(RX_SYNC_RESET));
> + config |= GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN);
> + gem_writel(bp, USX_CONTROL, config);
> +
> + return 0;
> +}
Always returns 0 can be void function?
> +int macb_usxgmii_pcs_suspend(struct phy_device *phydev)
> +{
> + uint32_t config;
> + struct macb *bp = phydev->bp;
> +
> + config = gem_readl(bp, USX_CONTROL);
> + config |= GEM_BIT(RX_SYNC_RESET);
> + /* disable signal */
> + config &= ~(GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN));
> + gem_writel(bp, USX_CONTROL, config);
> + rte_delay_ms(1);
> + return 0;
> +}
Always returns 0 should be void?
> +
> +int macb_usxgmii_pcs_check_for_link(struct phy_device *phydev)
> +{
> + int value;
> + int link;
> + struct macb *bp = phydev->bp;
> + value = gem_readl(bp, USX_STATUS);
> + link = GEM_BFEXT(BLOCK_LOCK, value);
> + return link;
> +}
The driver is sloppy in using int where unsigned value is possible.
You lose precision doing that and are prone to sign extension bugs.
Since gem_readl() is wrapper around macb_reg_readl() and that returns u32;
this function should be returning u32 and value should be u32
The temporary variable value is not needed.
> +int macb_gbe_pcs_check_for_link(struct phy_device *phydev)
> +{
> + int value;
> + int link;
> + struct macb *bp = phydev->bp;
> +
> + value = macb_readl(bp, NSR);
> + link = MACB_BFEXT(NSR_LINK, value);
> + return link;
> +}
