On 7/10/24 1:37 AM, Adam Ford wrote:
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
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 ?
