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

[Jerome Brunet <jbrunet@baylibre.com>]

On Fri 20 Jan 2023 at 11:52, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> 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@gmail.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?

Something in between.
The first write to CNTL1 powers off the PHY if it was on.
I think I'll change CNTL2 while the PHY is down and then power it back up

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

Thanks for testing. I will submit it soon.

>
>
> 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;
>  }