Re: [RFC PATCH net-next 03/10] net: hibmcge: Add mdio and hardware configuration supported in this module

[Jijie Shao <shaojijie@huawei.com>]

Thanks for reviewing

on 2024/8/1 8:42, Andrew Lunn wrote:
>> +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.

ok, I will delete it in V2

>
>> +/* 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

Thanks for the guide. These macros will simplify the code.

>
>> +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.

it's a good idea,

>
>> +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.

Yeah， using phy_remove_link_mode, the hbg_mode_ability[] and linkmode_set_bit
of the previous code can be removed together.

Thank you.

>
>> +
>> +	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?

I'll move this to another patch where it's more appropriate.

>
> 	Andrew

Thanks, Jijie