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

[Javen]

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

0xdc00 is a package-detect field. 0xd006 is internal HW id. RTL8116AF shares the same RTL_GIGA_MAC_VER_52 mac_version with other variants. 

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

I will try to handle this.

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

This is after firmware application. And PHY_MDIO_CHG opcode switches the access callbacks between PHY and MAC accessors.
data == 0: phy_read/phy_write
data != 0: mac_mcu_read/mac_mcu_write
So the firmware may contain mixed PHY and MAC.

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

It reads a counter from PHY, but the calculated value is programmed into MAC OCP register via r8168_mac_ocp_modify, which accesses r8169 through tp->mmio_addr. So I think this can not be moved.

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

I will try to remove this.

Thanks for your review.

BRs,
Javen