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

From: Andrew Lunn <andrew@lunn.ch>
To: Jerome Brunet <jbrunet@baylibre.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	linux-amlogic@lists.infradead.org,
	Kevin Hilman <khilman@baylibre.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Da Xue <da@lessconfused.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support
Date: Thu, 19 Jan 2023 18:17:57 +0100	[thread overview]
Message-ID: <Y8l7Rc9Vde9J45ij@lunn.ch> (raw)
In-Reply-To: <1jmt6eye1m.fsf@starbuckisacylon.baylibre.com>

> >> +
> >> +	/* 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.

Please document where these values have come from. In the future we
might need to point a finger when it all goes horribly wrong.

> 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

Some Marvell Ethernet switches with integrated PHYs have IDs with the
vendor part set to Marvell, but the lower part is 0. The date sheet
even says this is deliberate, you need to look at some other register
in the switches address space to determine what the part is. That
works O.K in the vendor crap monolithic driver, but not for Linux
which separates the drivers up. So we have to intercept the reads and
fill in the lower part. And we have no real knowledge if the PHYs are
all the same, or there are differences. So we put in the switch ID,
and the PHY driver then has an entry per switch. That gives us some
future wiggle room if we find the PHYs are actually different.

Is there any indication in the datasheets that the PHY is the exact
same one as in the g12? Are we really safe to reuse this value between
different SoCs?

I actually find it an odd feature. Does the datasheet say anything
about Why you can set the ID in software? The ID describes the
hardware, and software configuration should not be able to change the
hardware in any meaningful way.

> >> +	/* 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. 

O.K, please reword it to indicate power up, not link up.

     Andrew