Re: [PATCH 2/2] arm64: dts: renesas: Drop ethernet-phy-ieee802.3-c22 from PHY compatible string on all RZ boards

[Marek Vasut <marex@denx.de>]

On 7/10/24 1:37 AM, Adam Ford wrote:
> On Mon, Jul 8, 2024 at 10:34 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/8/24 9:09 AM, Geert Uytterhoeven wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Fri, Jul 5, 2024 at 11:50 PM Marek Vasut <marex@denx.de> wrote:
>>>> On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
>>>>>>> What about moving the PHYs inside an mdio subnode, and removing the
>>>>>>> compatible properties instead? That would protect against different
>>>>>>> board revisions using different PHYs or PHY revisions.
>>>>>>>
>>>>>>> According to Niklas[1], using an mdio subnode cancels the original
>>>>>>> reason (failure to identify the PHY in reset state after unbind/rebind
>>>>>>> or kexec) for adding the compatible values[2].
>>>>>>
>>>>>> My understanding is that the compatible string is necessary if the PHY
>>>>>> needs clock/reset sequencing of any kind. Without the compatible string,
>>>>>> it is not possible to select the correct PHY driver which would handle
>>>>>> that sequencing according to the PHY requirements. This board here does
>>>>>> use reset-gpio property in the PHY node (it is not visible in this diff
>>>>>> context), so I believe a compatible string should be present here.
>>>>>
>>>>> With the introduction of an mdio subnode, the reset-gpios would move
>>>>> from the PHY node to the mio subnode, cfr. commit b4944dc7b7935a02
>>>>> ("arm64: dts: renesas: white-hawk: ethernet: Describe AVB1 and AVB2")
>>>>> in linux-next.
>>>>
>>>> That's a different use case, that commit uses generic
>>>> "ethernet-phy-ieee802.3-c45" compatible string and the PHY type is
>>>> determined by reading out the PHY ID registers after the reset is released.
>>>>
>>>> This here uses specific compatible string, so the kernel can determine
>>>> the PHY ID from the DT before the reset is released .
>>>
>>> I am suggesting removing the specific compatible string here, too,
>>> introducing an mdio subnode, so the kernel can read it from the PHY
>>> ID registers after the reset is released?
>>
>> I wrote this to Niklas already, so let me expand on it:
>>
>> My understanding of reset GPIO in the MDIO node is that it is used in
>> case there might be multiple PHYs with shared reset GPIO on the same
>> MDIO bus. Like on the NXP iMX28 .
>>
>> In this case, the reset is connected to this single PHY, so the reset
>> line connection is a property of the PHY and should be described in the
>> PHY node.
>>
>> You could argue that in this case, because there is only one PHY and
>> only one reset line, it fits both categories, PHY reset and MDIO reset.
>>
>> And then, there is the future-proofing aspect.
>>
>> If the compatible string is retained, then if in the future there is
>> some problem discovered related to the reset of this PHY, the PHY driver
>> can match on the compatible string and apply a fix up. But it prevents
>> future PHY replacement (which is unlikely in my opinion).
>>
>> If the compatible string is removed and the reset is moved to MDIO node,
>> then replacement of the PHY in the future is likely possible (assuming
>> it does not have any special reset timing requirements), but if there is
>> a problem related to the reset of the current PHY model, the PHY driver
>> cannot fix it up because there is no compatible to match on.
>>
>> I think that about sums the pros and cons up, right ?
>>
>> I also think there is no good solution here, only two bad ones, with
>> different issues each.
>>
>>>>>> What would happen if this board got a revision with another PHY with
>>>>>> different PHY reset sequencing requirements ? The MDIO node level reset
>>>>>> handling might no longer be viable.
>>>>>
>>>>> True. However, please consider these two cases, both assuming
>>>>> reset-gpios is in the MDIO node:
>>>>>
>>>>>      1. The PHY node has a compatible value, and a different PHY is
>>>>>         mounted: the new PHY will not work, as the wrong PHY driver
>>>>>         is used.
>>>>
>>>> What is the likelihood of such PHY exchange happening with these three
>>>> specific boards ? I think close to none, as that would require a board
>>>> redesign to swap in a different PHY.
>>>
>>> I don't know about the likelihood for these boards.
>>> It did happen before on other boards, e.g. commit a0d23b8645b2d577
>>> ("arm64: dts: renesas: beacon-renesom: Update Ethernet PHY ID").
>>
>> I had that happen too. The solution there was to upstream the newer PHY
>> ID and apply backward compatibility DTO that rewrote the PHY ID for the
>> few older boards. The DTO application decision was done in U-Boot scripting.
>>
>> It was not possible to auto-detect the PHY after deasserting its reset
>> in my case, I had to determine whether to apply DTO or not based on
>> strap resistors on the board.
> 
> Beacon had to swap the phy due to the

Oh ...

> great part shortage of 2021

... I'll borrow this quote.

> and
> having the hard-code ID's prevented backwards compatibility.  For the
> Beacon downstream kernel, we removed the PHY ID and kept the generic
> 'ethernet-phy-ieee802.3-c22' because it could auto-detect what we
> needed and both PHY's appear to come out of reset and register
> properly.  I don't know if we could make a generic phy-ieee802.3-c22
> handle the reset, wait a moment and then try to auto-detect, but it
> would be nice to not have to jump through hoops if/when people need to
> change PHY's.

The reset handling and clock handling is the main issue here. Maybe we 
should cross that bridge when we come to it ? I.e. when someone needs to 
replace a PHY, let's deal with the requirement then ?