Re: [PATCH net-next v1 4/6] r8169: add support for RTL8116af

[Andrew Lunn]

> +static bool rtl_is_8116af(struct rtl8169_private *tp)
> +{
> +	return tp->mac_version == RTL_GIGA_MAC_VER_52 &&
> +		(r8168_mac_ocp_read(tp, 0xdc00) & 0x0078) == 0x0030 &&
> +		(r8168_mac_ocp_read(tp, 0xd006) & 0x00ff) == 0x0000;

Do we know what these magic numbers mean?

>  static enum rtl_dash_type rtl_get_dash_type(struct rtl8169_private *tp)
>  {
>  	switch (tp->mac_version) {
> @@ -2397,7 +2431,7 @@ static int rtl8169_set_link_ksettings(struct net_device *ndev,
>  	int duplex = cmd->base.duplex;
>  	int speed = cmd->base.speed;
>  
> -	if (!tp->sfp_mode)
> +	if (tp->sfp_mode != RTL_SFP_8127_ATF)
>  		return phylink_ethtool_ksettings_set(tp->phylink, cmd);

Is this even needed? phylink should be able to handle sfp and copper
in the same way.

> @@ -2509,9 +2543,10 @@ void r8169_apply_firmware(struct rtl8169_private *tp)
>  		tp->ocp_base = OCP_STD_PHY_BASE;
>  
>  		/* PHY soft reset may still be in progress */
> -		phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
> -				      !(val & BMCR_RESET),
> -				      50000, 600000, true);
> +		if (tp->phydev)
> +			phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
> +					      !(val & BMCR_RESET),
> +					      50000, 600000, true);

Maybe this all needs to move into the PHY driver?

> -	rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
> -	if (rg_saw_cnt > 0) {
> -		u16 sw_cnt_1ms_ini;
> +	if (tp->phydev) {
> +		rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
> +		if (rg_saw_cnt > 0) {
> +			u16 sw_cnt_1ms_ini;
>  
> -		sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> -		r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> +			sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> +			r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> +		}

Can this move into the PHY driver?

> @@ -5017,9 +5054,11 @@ static void rtl8169_up(struct rtl8169_private *tp)
>  		rtl8168_driver_start(tp);
>  
>  	pci_set_master(tp->pci_dev);
> -	phy_init_hw(tp->phydev);
> -	phy_resume(tp->phydev);
> -	rtl8169_init_phy(tp);
> +	if (tp->phydev) {
> +		phy_init_hw(tp->phydev);
> +		phy_resume(tp->phydev);
> +		rtl8169_init_phy(tp);
> +	}

Why is this needed?

	Andrew