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

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 17 Jan 2023 at 15:51, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> 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.

Guilty ... I'm afraid git a far better memory than I do.

There is no reason for this compatible. It works without it (as
explained on other threads)

It was a mistake to add it in the first place. Probably a stupid
copy/paste. It can (and should) be removed.

>
> 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,