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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1jsfg5wmpi.fsf@starbuckisacylon.baylibre.com>
Date:   Fri, 20 Jan 2023 10:55:29 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Paolo Abeni <pabeni@...hat.com>
Cc:     Neil Armstrong <neil.armstrong@...aro.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@...ts.infradead.org>
Subject: Re: [PATCH net-next] net: phy: meson-gxl: support more
 G12A-internal PHY versions


On Tue 17 Jan 2023 at 15:51, Heiner Kallweit <hkallweit1@...il.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,