Re: [net,PATCH] net: phy: dp83869: fix STRAP_OPMODE bitmask

[Marek Vasut <marek.vasut@mailbox.org>]

On 10/27/25 12:48 AM, Andrew Lunn wrote:
> On Sat, Oct 25, 2025 at 05:09:17AM +0200, Marek Vasut wrote:
>> On 10/24/25 2:24 AM, Andrew Lunn wrote:
>>
>> Hello Andrew,
>>
>>> On Fri, Oct 24, 2025 at 12:39:45AM +0200, Marek Vasut wrote:
>>>> From: Thanh Quan <thanh.quan.xn@renesas.com>
>>>>
>>>> According to the TI DP83869HM datasheet Revision D (June 2025), section
>>>> 7.6.1.41 STRAP_STS Register, the STRAP_OPMODE bitmask is bit [11:9].
>>>> Fix this.
>>>
>>> It is a good idea to state in the commit message what the bad
>>> behaviour is which the patch fixes. Somebody looking through the
>>> patches can then decide if they need to cherry-pick the patch into
>>> their dead vendor tree, etc.
>>>
>>> Please add to the commit message what issue you where seeing which
>>> made you create this patch.
>>
>> In short, on the hardware I use, the interface to the PHY is SGMII, but the
>> driver is confused into thinking it is RGMII based on the STRAP_STS register
>> content, and misconfigures the PHY for RGMII.
>>
>> I plan to extend the commit message this way. I tried to cover all the bases
>> there, so people can decide whether the are affected or not. Is this
>> understandable or is it maybe too much ?
>>
>> "
>> net: phy: dp83869: fix STRAP_OPMODE bitmask
>>
>> According to the TI DP83869HM datasheet Revision D (June 2025), section
>> 7.6.1.41 STRAP_STS Register, the STRAP_OPMODE bitmask is bit [11:9].
>> Fix this.
>>
>> In case the PHY is auto-detected via PHY ID registers, or not described
>> in DT, or, in case the PHY is described in DT but the optional DT property
>> "ti,op-mode" is not present, then the driver reads out the PHY functional
>> mode (RGMII, SGMII, ...) from hardware straps.
>>
>> Currently, all upstream users of this PHY specify both DT compatible string
>> "ethernet-phy-id2000.a0f1" and ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>
>> property, therefore it seems no upstream users are affected by this bug.
>>
>> The driver currently interprets bits [2:0] of STRAP_STS register as PHY
>> functional mode. Those bits are controlled by ANEG_DIS, ANEGSEL_0 straps
>> and an always-zero reserved bit. Systems that use RGMII-to-Copper functional
>> mode are unlikely to disable auto-negotiation via ANEG_DIS strap, or change
>> auto-negotiation behavior via ANEGSEL_0 strap. Therefore, even with this bug
>> in place, the STRAP_STS register content is likely going to be interpreted
>> by the driver as RGMII-to-Copper mode.
>>
>> However, for a system with PHY functional mode strapping set to other mode
>> than RGMII-to-Copper, the driver is likely to misinterpret the strapping
>> as RGMII-to-Copper and misconfigure the PHY.
>>
>> For example, on a system with SGMII-to-Copper strapping, the STRAP_STS
>> register reads as 0x0c20, but the PHY ends up being configured for
>> incompatible RGMII-to-Copper mode.
>> "
> 
> This is good. It nice to have lots of details, cause and effect, even
> if its a silly topo bug.
> 
> Please add my Reviewed-by to the next version.
Will do. Thank you for your help !