[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ba5b8b48-64b7-417d-a000-2e82e242c399@lunn.ch>
Date: Thu, 1 Aug 2024 02:42:37 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: yisen.zhuang@...wei.com, salil.mehta@...wei.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
horms@...nel.org, shenjian15@...wei.com, wangpeiyang1@...wei.com,
liuyonglong@...wei.com, sudongming1@...wei.com,
xujunsheng@...wei.com, shiyongbang@...wei.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 03/10] net: hibmcge: Add mdio and hardware
configuration supported in this module
> +int hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex)
> +{
> + if (speed != HBG_PORT_MODE_SGMII_10M &&
> + speed != HBG_PORT_MODE_SGMII_100M &&
> + speed != HBG_PORT_MODE_SGMII_1000M)
> + return -EOPNOTSUPP;
> +
> + if (duplex != DUPLEX_FULL && duplex != DUPLEX_HALF)
> + return -EOPNOTSUPP;
> +
> + if (speed == HBG_PORT_MODE_SGMII_1000M && duplex == DUPLEX_HALF)
> + return -EOPNOTSUPP;
If you tell phylib you don't support 1G/Half, this will not happen.
> +/* sgmii autoneg always enable */
> +int hbg_hw_sgmii_autoneg(struct hbg_priv *priv)
> +{
> + wait_time = 0;
> + do {
> + msleep(HBG_HW_AUTONEG_TIMEOUT_STEP);
> + wait_time += HBG_HW_AUTONEG_TIMEOUT_STEP;
> +
> + an_state.bits = hbg_reg_read(priv, HBG_REG_AN_NEG_STATE_ADDR);
> + if (an_state.an_done)
> + break;
> + } while (wait_time < HBG_HW_AUTONEG_TIMEOUT_MS);
include/linux/iopoll.h
> +static const u32 hbg_mode_ability[] = {
> + ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_Autoneg_BIT,
> + ETHTOOL_LINK_MODE_TP_BIT,
> +};
> +
> +static int hbg_mac_init(struct hbg_priv *priv)
> +{
> + struct hbg_mac *mac = &priv->mac;
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(hbg_mode_ability); i++)
> + linkmode_set_bit(hbg_mode_ability[i], mac->supported);
Humm, odd. Where is this leading...
> +#define HBG_MDIO_FREQUENCE_2_5M 0x1
I assume it supports other frequencies. You might want to implement
the DT property 'clock-frequency'. Many modern PHY will work faster
than 2.5Mhz.
> +static int hbg_phy_connect(struct hbg_priv *priv)
> +{
> + struct phy_device *phydev = priv->mac.phydev;
> + struct hbg_mac *mac = &priv->mac;
> + int ret;
> +
> + linkmode_copy(phydev->supported, mac->supported);
> + linkmode_copy(phydev->advertising, mac->supported);
And here it is. Why? Do you see any other MAC driver doing this?
What you probably want is:
phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
which is what other MAC drivers do.
> +
> + phy_connect_direct(priv->netdev, mac->phydev, hbg_phy_adjust_link,
> + PHY_INTERFACE_MODE_SGMII);
> + ret = devm_add_action_or_reset(&priv->pdev->dev,
> + hbg_phy_disconnect, mac->phydev);
> + if (ret)
> + return ret;
> +
> + phy_attached_info(phydev);
> + return 0;
> +}
> +
> +/* include phy link and mac link */
> +u32 hbg_get_link_status(struct hbg_priv *priv)
> +{
> + struct phy_device *phydev = priv->mac.phydev;
> + int ret;
> +
> + if (!phydev)
> + return HBG_LINK_DOWN;
> +
> + phy_read_status(phydev);
> + if ((phydev->state != PHY_UP && phydev->state != PHY_RUNNING) ||
> + !phydev->link)
> + return HBG_LINK_DOWN;
> +
> + ret = hbg_hw_sgmii_autoneg(priv);
> + if (ret)
> + return HBG_LINK_DOWN;
> +
> + return HBG_LINK_UP;
> +}
There should not be any need for this. So why do you have it?
Andrew
Powered by blists - more mailing lists