Message-ID: <dc1ab1b9-347e-5d6f-670b-03057a98d2dd@gmail.com>
Date:   Fri, 20 Jan 2023 11:52:07 +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:22, Heiner Kallweit wrote:
> 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.
>>
> 

I tested the patch with a slight modification. Maybe the PHY picks up other
settings also on power-up/soft-reset only. Therefore I moved setting
ETH_PHY_CNTL2 to before powering up the PHY. Do you think that's needed/better?

With this patch the new PHY ID is effective early enough and the right
PHY driver is loaded also w/o overriding the PHY ID with a compatible.

Based on which version of the patch you prefer, are you going to submit it?
Else I can do it too, just let me know.


diff --git a/drivers/net/mdio/mdio-mux-meson-g12a.c b/drivers/net/mdio/mdio-mux-meson-g12a.c
index 9d21fdf85..7882dcce2 100644
--- a/drivers/net/mdio/mdio-mux-meson-g12a.c
+++ b/drivers/net/mdio/mdio-mux-meson-g12a.c
@@ -6,6 +6,7 @@
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -148,6 +149,7 @@ static const struct clk_ops g12a_ephy_pll_ops = {
 
 static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv)
 {
+	u32 val;
 	int ret;
 
 	/* Enable the phy clock */
@@ -159,17 +161,19 @@ static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv)
 
 	/* Initialize ephy control */
 	writel(EPHY_G12A_ID, priv->regs + ETH_PHY_CNTL0);
-	writel(FIELD_PREP(PHY_CNTL1_ST_MODE, 3) |
-	       FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) |
-	       FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) |
-	       PHY_CNTL1_CLK_EN |
-	       PHY_CNTL1_CLKFREQ |
-	       PHY_CNTL1_PHY_ENB,
-	       priv->regs + ETH_PHY_CNTL1);
 	writel(PHY_CNTL2_USE_INTERNAL |
 	       PHY_CNTL2_SMI_SRC_MAC |
 	       PHY_CNTL2_RX_CLK_EPHY,
 	       priv->regs + ETH_PHY_CNTL2);
+	val = FIELD_PREP(PHY_CNTL1_ST_MODE, 3) |
+	      FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) |
+	      FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) |
+	      PHY_CNTL1_CLK_EN |
+	      PHY_CNTL1_CLKFREQ;
+	writel(val, priv->regs + ETH_PHY_CNTL1);
+	val |= PHY_CNTL1_PHY_ENB;
+	writel(val, priv->regs + ETH_PHY_CNTL1);
+	mdelay(10);
 
 	return 0;
 }
-- 
2.39.0

Powered by blists - more mailing lists