Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>, Paolo Abeni <pabeni@redhat.com>,
	Jerome Brunet <jbrunet@baylibre.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	David Miller <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"open list:ARM/Amlogic Meson..."
	<linux-amlogic@lists.infradead.org>
Subject: Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions
Date: Tue, 17 Jan 2023 15:51:14 +0100	[thread overview]
Message-ID: <1f841ad8-d2f9-cf39-da65-5c90fddb3cee@gmail.com> (raw)
In-Reply-To: <Y8ai6+oaaP0KwkAY@lunn.ch>

On 17.01.2023 14:30, Andrew Lunn wrote:
>>> The PHY compatible string in DT is the following in all cases:
>>> compatible = "ethernet-phy-id0180.3301"
> 
> This form of compatible has two purposes.
> 
> 1) You cannot read the PHY ID register during MDIO bus enumeration,
> generally because you need to turn on GPIOs, clocks, regulators etc,
> which the MDIO/PHY core does not know how to do.
> 
> 2) The PHY has bad values in its ID registers, typically because the
> manufactures messed up.
> 
> If you have a compatible like this, the ID registers are totally
> ignored by Linux, and the ID is used to find the driver and tell the
> driver exactly which of the multiple devices it supports it should
> assume the device is.
> 
> So you should use this from of compatible with care. You can easily
> end up thinking you have a different PHY to what you actually have,
> which could then result in wrong erratas being applied etc, or even
> the wrong driver being used.
> 

Right. I checked and this compatible was added with
280c17df8fbf ("arm64: dts: meson: g12a: add mdio multiplexer").

compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22";

The commit message doesn't explain why overriding the PHY ID
is needed. Maybe Jerome as author can shed some light on it.

At least on my system it's not needed (after setting the PHY ID
in the PHY driver to the actual value).

Would be interesting to know whether PHY is still detected
and driver loaded with this patch on g12 systems.
If the genphy driver should be used then the actual PHY ID
would be interesting (look for attribute phy_id in sysfs).

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 585dd70f6..8af48aff0 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -1695,8 +1695,7 @@ int_mdio: mdio@1 {
 					#size-cells = <0>;
 
 					internal_ephy: ethernet_phy@8 {
-						compatible = "ethernet-phy-id0180.3301",
-							     "ethernet-phy-ieee802.3-c22";
+						compatible = "ethernet-phy-ieee802.3-c22";
 						interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
 						reg = <8>;
 						max-speed = <100>;
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index c49062ad7..0fd76d49a 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -262,7 +262,7 @@ static struct phy_driver meson_gxl_phy[] = {
 		.suspend        = genphy_suspend,
 		.resume         = genphy_resume,
 	}, {
-		PHY_ID_MATCH_EXACT(0x01803301),
+		PHY_ID_MATCH_EXACT(0x01803300),
 		.name		= "Meson G12A Internal PHY",
 		/* PHY_BASIC_FEATURES */
 		.flags		= PHY_IS_INTERNAL,
-- 
2.39.0

>     Andrew


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
