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: <eef8668d-a1f1-7b5d-1a15-ebf3a00d1e4b@gmail.com>
Date:   Fri, 20 Jan 2023 11:22:10 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Jerome Brunet <jbrunet@...libre.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 20.01.2023 11:01, Jerome Brunet wrote:
> 
> 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.
> 
I tested and a PHY soft reset is also sufficient to pick up the new PHY ID.
Supposedly everything executing the soft reset logic is sufficient:
power-up, soft reset, coming out of suspend/power-down


> 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
> 
Thanks for the hint, I'll look at it and test.

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