On Mon, Jul 8, 2024 at 10:34 PM Marek Vasut <marex@xxxxxxx> 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@xxxxxxx> 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 great part shortage of 2021 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.
adam
>
