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: <1jo7qtwm1z.fsf@starbuckisacylon.baylibre.com>
Date:   Fri, 20 Jan 2023 11:01:56 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     Russell King - ARM Linux <linux@...linux.org.uk>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Kevin Hilman <khilman@...libre.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 Thu 19 Jan 2023 at 23:42, Heiner Kallweit <hkallweit1@...il.com> wrote:

> On 15.01.2023 21:38, Heiner Kallweit wrote:
>> On 15.01.2023 19:43, Neil Armstrong wrote:
>>> Hi Heiner,
>>>
>>> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit :
>>>> On 15.01.2023 17:57, Andrew Lunn wrote:
>>>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote:
>>>>>> On my SC2-based system the genphy driver was used because the PHY
>>>>>> identifies as 0x01803300. It works normal with the meson g12a
>>>>>> driver after this change.
>>>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions.
>>>>>
>>>>> Hi Heiner
>>>>>
>>>>> Are there any datasheets for these devices? Anything which documents
>>>>> the lower nibble really is a revision?
>>>>>
>>>>> I'm just trying to avoid future problems where we find it is actually
>>>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we
>>>>> break devices using 0x01803302 which we had no idea exists, but got
>>>>> covered by this change.
>>>>>
>>>> The SC2 platform inherited a lot from G12A, therefore it's plausible
>>>> that it's the same PHY. Also the vendor driver for SC2 gives a hint
>>>> as it has the following compatible for the PHY:
>>>>
>>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22";
>>>>
>>>> But you're right, I can't say for sure as I don't have the datasheets.
>>>
>>> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers,
>>> please see:
>>> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36
>>>
>>> So you should either add support for the PHY mux in SC2 or check
>>> what is in the ETH_PHY_CNTL0 register.
>>>
>> Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the
>> end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180.
>> But still the PHY reports 3300.
>> Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300
>> as PHY ID.
>> 
>> For u-boot I found the following:
>> 
>> https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c
>> 
>> static struct phy_driver amlogic_internal_driver = {
>> 	.name = "Meson GXL Internal PHY",
>> 	.uid = 0x01803300,
>> 	.mask = 0xfffffff0,
>> 	.features = PHY_BASIC_FEATURES,
>> 	.config = &meson_phy_config,
>> 	.startup = &meson_aml_startup,
>> 	.shutdown = &genphy_shutdown,
>> };
>> 
>> So it's the same PHY ID I'm seeing in Linux.
>> 
>> My best guess is that the following is the case:
>> 
>> The PHY compatible string in DT is the following in all cases:
>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22";
>> 
>> Therefore id 0180/3301 is used even if the PHY reports something else.
>> Means it doesn't matter which value you write to ETH_PHY_CNTL0.
>> 
>> I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22"
>> and this resulted in the actual PHY ID being used.
>> You could change the compatible in dts the same way for any g12a system
>> and I assume you would get 0180/3300 too.
>> 
>> Remaining question is why the value in ETH_PHY_CNTL0 is ignored.
>> 
>
> I think I found what's going on. The PHY ID written to SoC register
> ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before
> it reports the new PHY ID. Would be good to have a comment in the
> g12a mdio mux code mentioning this constraint.
>
> I see no easy way to trigger a soft reset early enough. Therefore it's indeed
> the simplest option to specify the new PHY ID in the compatible.

This is because (I guess) the PHY only picks ups the ID from the glue
when it powers up. After that the values are ignored.

Remember the PHY is a "bought" IP, the glue/mux provides the input
settings required by the PHY provider.

Best would be to trigger an HW reset of PHY from glue after setting the
register ETH_PHY_CNTL0.

Maybe this patch could help : ?
https://gitlab.com/jbrunet/linux/-/commit/ccbb07b0c9eb2de26818eb4f8aa1fd0e5b31e6db.patch

I tried this when we debugged the connectivity issue on the g12 earlier
this spring. I did not send it because the problem was found to be in
stmmac.