RE: [PATCH net-next v1 1/5] net: phy: realtek: add support for dummy phy

[Javen]

>Hi,
>
>On 6/3/26 11:13, Javen wrote:
>>> Hi,
>>>
>>> On 6/3/26 10:00, Javen wrote:
>>>>> On 6/3/26 08:59, javen wrote:
>>>>>> From: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>>>>
>>>>>> Add support for rtl8116af dummy phy driver, match phy id and read
>>>>>> link speed from MII_BMCR.
>>>>>>
>>>>>> Signed-off-by: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>>>
>>>>> Can you elaborate more on why this is needed ?
>>>>>
>>>>> The cover says :
>>>>> "
>>>>> In this mode, the driver
>>>>> needs a dummy PHY ID so that phylib can attach to a dummy Realtek
>>>>> PHY driver, while selected standard PHY registers are handled
>>>>> through the SerDes register.
>>>>> "
>>>>>
>>>>> Why can't you use the SerDes registers for your PHY driver ? The
>>>>> phrase above suggests that the "SerDes registers" have the typical
>>>>> C22/45 layout. There are PHYs and PCSs out there that aren't
>>>>> accessed through regular MDIO with regmap being used to translate
>>>>> the mdio accesses done by phylib into the actual register access method
>used.
>>>>>
>>>>> Maxime
>>>>>
>>>>
>>>> Thanks for your review.
>>>> Maybe I shouldn't call it a "dummy" PHY. It is actually a real,
>>>> dedicated PHY
>>> driver for the RTL8116af operating in Fiber/SerDes mode. The reason
>>> we cannot use the standard generic PHY driver is that hardware does
>>> not correctly populate the standard Gigabit Status Register
>>> (MII_STAT1000, Reg 0x0a).
>>>> Here is a snippet of our MDIO read trace during link up:
>>>> r8169 0000:85:00.1: MDIO read: tp->ocp_base=0xa400, reg=0x0a,
>>>> value=0x0000
>>>> r8169 0000:85:00.1: MDIO read: tp->ocp_base=0xa400, reg=0x05,
>>>> value=0x41a0 Reg 0x0a returns 0x0000, the phylib generic status
>>>> parser fails
>>> to resolve the 1000Mbps link, causing the driver to incorrectly fall
>>> back to a forced 10Mbps, even though the physical link is actually UP at
>1000Mbps.
>>> Therefore, we need to register a custom PHY driver to provide a
>>> specific .read_status callback that parses MII_BMCR instead of
>>> relying on MII_STAT1000.
>>>
>>> This behaviour of not getting proper information through the standard
>>> register happens on multiple different PHYs, so having a dedicated
>>> driver is the way to go indeed.
>>>
>>> What does this PHY do exactly, does it connect to an SFP cage as the
>>> name seems to imply ? If so, what kind of mode can it support ?
>>> 1000BaseX/100BaseX, maybe SGMII as well ?
>>>
>>> What I don't quite understand then is why you have the need to return
>>> a custom PHY ID that matches that "dummy" driver, in the next patch.
>>>
>>> If this is a regular PHY with C22 registers, what's the original
>>> content of
>>> PHYSID1/2 ? M
>>>
>>> Can you explain what the hardware looks like exactly ? What are the
>>> different components involved, etc ?
>>>
>>> Maxime
>>
>> It supports 1000BaseX mode.
>
>Ok, but the driver for the "dummy" phy seems to be able to report SPEED_100,
>which you can't achieve with 1000BaseX. Either this is a stray check in the PHY
>driver, or there's more to it than 1000BaseX :)
>
>Also, if this is 1000BaseX maybe you need to use the C37 helpers to read the
>link status :
>
>https://elixir.bootlin.com/linux/v7.1-
>rc5/source/drivers/net/phy/phy_device.c#L2607
>
>> The datapath in Fiber mode is strictly and simply: MAC -> PCS -> SerDes.
>MAC accesses the PCS registers via MAC OCP channel. Although it is accessed
>via MAC OCP, the register definitions at those addresses are exactly the same
>as the IEEE 802.3 Clause 22 PHY standard registers.
>> Reading Reg 0x02 and 0x03 via this MAC OCP interface simply returns
>0x00000000.
>> [ 1671.299837] r8169 0000:85:00.1: 8116af read: reg=0x02, value=0x0000
>> [ 1671.299906] r8169 0000:85:00.1: 8116af read: reg=0x03, value=0x0000
>> So we need a dummy phy id to attach phy driver.
>
>So this PHY actually doesn't exist ? what you have is a PCS that you access over
>MDIO, right ?
>
>It seems it all boils down to how the realtek MAC driver deals with PHYs and
>PCSs and SFP, that is without using phylink or the SFP infrastructure at all :(
>
>Can you check for the C37 thing and the SPEED_100 reporting ?
>
>Maxime
>
You are exactly right. The PHY doesn't actually exist; what we have is an internal PCS.  I will try using the C37 helpers (genphy_c37_read_status()) to read the link status and test how it behaves with our hardware. I will get back to you with the results soon. And check SPEED_100 too.
I actually tried to migrate it to phylink before. However, the current driver codebase is deeply coupled with tp->phydev in many places. Decoupling it may introduce bugs for numerous existing devices. Therefore, I want to try to expose the internal PCS as a dummy PHY device.
Anyway, thanks for your review.

BRs,
Javen