Re: [PATCH net-next v1 2/6] r8169: add support for phylink

[Maxime Chevallier]

On 6/8/26 09:40, Javen wrote:
>>
>> Hi,
>>
>> On 6/5/26 12:39, javen wrote:
>>> From: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>
>>> Transfer old framework to phylink. Phylink can support fiber mode card
>>> which can not get link status or link speed from standard phy registers.
>>
>> This is a good start, but you need to go deeper than that. Looking at the end
>> result, you still access tp->phydev a lot in this driver :
>>
>> Looking at r8169_mdio_register() for example :
>>
>> -> don't configure the PHY eee support with phy_support_eee() and
>>    phy_disable_eee_mode(), let phylink to that for you
>>
>> All over the driver, there's still a lot of manual control of the PHY with phylib,
>> look at the calls for phy_init_hw(), phy_resume(), and so on, you need to
>> assume that with phylink you may not have a PHY.
>>
>> The problem then is that there are places in the code where PHY registers are
>> directly accessed :
>>
>> static void rtl8169_init_phy(struct rtl8169_private *tp) { [...]
>>
>>        if (tp->mac_version == RTL_GIGA_MAC_VER_05 &&
>>            tp->pci_dev->subsystem_vendor == PCI_VENDOR_ID_GIGABYTE &&
>>            tp->pci_dev->subsystem_device == 0xe000)
>>                phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b); [...]
>>        genphy_soft_reset(tp->phydev);
>> }
>>
>> In the end, you shouldn't even need to use tp->phydev at all.
>>
>> It's hard to know what this is all about, but it seems like something a PHY
>> driver should have to do, not a MAC driver :(
>>
>> Also, I'd merge the next commit with this one to have one single commit doing
>> the phylink conversion.
>>
>> Maxime
>>
> Hi Maxime,
> 
> I have one question about the existing link-change interrupt model.
> 
> For most r8169 chips, the PHY exists and the current driver uses the MAC LinkChg interrupt to notify phylib:
>         if (status & LinkChg)
>                 phy_mac_interrupt(tp->phydev);

This is the first time there's an attempt to port a driver that
typically deals with an embedded PHY to phylink, so we need to
sort this out.

The main issue is that this assumes the phy device is an integrated
PHY. But what if the PHY isn't integrated and can deal itself with the
interrupt ? or there's just no PHY ?

I think we could imagine adding a phylink helper that would call
phy_mac_interrupt(pl->phydev), however we need to make sure that
this is only done when the MAC is handling the interrupt on behalf
of the integrated PHY, and not any PHY.

> 
> After converting the driver to phylink, my understanding is that the MAC driver should avoid keeping a private tp->phydev pointer. The PHY can still be found during setup and passed to phylink_connect_phy(), but then I am not sure how the MAC-routed PHY interrupt should be handled.
> 
> Would switching the internal PHY to PHY_POLL be acceptable , or would you prefer to keep the existing interrupt-driven behaviour? If the latter, what would be the preferred way to notify phylib without storing tp->phydev in the driver?

I think we need to address this instead of falling to POLL :)

> 
> I also have a related question about the existing firmware handling and sw_cnt_1ms_ini logic. Some of this code touches MAC MMIO registers, so it does not seem suitable to move it entirely into the Realtek PHY driver. The current code uses tp->phydev in this area.

I'll continue looking at this deeper in the upcoming days :)

Maxime






> 
> What would be the preferred phylink-compatible way to handle this? Should the driver still keep the tp->phydev, or is there another recommended approach?
> 
> Link: https://lore.kernel.org/netdev/b54b5f3a6703498fbc17e64548550ba0@xxxxxxxxxxxxxx/
> 
> Thanks,
> BRs,
> Javen
> 
>