Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet <jbrunet@baylibre.com>]

On Wed 18 Jan 2023 at 04:02, Andrew Lunn <andrew@lunn.ch> wrote:

>> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
>> +{
>> +	u32 val;
>> +
>> + 	/* Setup the internal phy */
>> +	val = (REG3_ENH |
>> +	       FIELD_PREP(REG3_CFGMODE, 0x7) |
>> +	       REG3_AUTOMDIX |
>> +	       FIELD_PREP(REG3_PHYADDR, 8) |
>> +	       REG3_LEDPOL |
>> +	       REG3_PHYMDI |
>> +	       REG3_CLKINEN |
>> +	       REG3_PHYIP);
>> +
>> +	writel_relaxed(REG4_PWRUPRSTSIG, priv->regs + ETH_REG4);
>> +	writel_relaxed(val, priv->regs + ETH_REG3);
>> +	mdelay(10);
>
> Probably the second _relaxed() should not be. You want it guaranteed
> to be written out before you do the mdelay().

Good point, I'll have a look

>
>> +
>> +	/* Set the internal phy id */
>> +	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
>> +		       priv->regs + ETH_REG2);
>
> So how does this play with what Heiner has been reporting recently?

What Heiner reported recently is related to the g12 family, not the gxl
which this driver address.

That being said, the g12 does things in a similar way - the glue
is just a bit different:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/mdio/mdio-mux-meson-g12a.c?h=v6.2-rc4#n165

> What is the reset default? Who determined this value?

It's the problem, the reset value is 0. That is why GXL does work with the
internal PHY if the bootloader has not initialized it before the kernel
comes up ... and there is no guarantee that it will.

The phy id value is arbitrary, same as the address. They match what AML
is using internally.

They have been kept to avoid making a mess if a vendor bootloader is
used with the mainline kernel, I guess.

I suppose any value could be used here, as long as it matches the value
in the PHY driver:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/meson-gxl.c?h=v6.2-rc4#n253

>
>> +	/* Enable the internal phy */
>> +	val |= REG3_PHYEN;
>> +	writel_relaxed(val, priv->regs + ETH_REG3);
>> +	writel_relaxed(0, priv->regs + ETH_REG4);
>> +
>> +	/* The phy needs a bit of time to come up */
>> +	mdelay(10);
>
> What do you mean by 'come up'? Not link up i assume. But maybe it will
> not respond to MDIO requests?

Yes this MDIO multiplexer is also the glue that provides power and
clocks to the internal PHY. Once the internal PHY is selected, it needs
a bit a of time before it is usuable. 

>
>     Andrew