[PATCH] net: marvell: mvgbe: Set PHY page 0 before phy_connect

[Stefan Roese]

Hi Tony,

On 4/12/22 22:18, Tony Dinh wrote:
> For most Kirkwood boards, the PHY page is already set to page 0
> (in register 22) before phy_connect is invoked. But some board like
> the Zyxel NSA310S (which uses the network chip MV88E1318S), the PHY page
> is not set to page 0. There seems to be some bad data remained in
> register 22 when the uclass MVGBE about to invoke phy_connect().
> 
> This patch enables the uclass MVGBE to always set the PHY page to 0
> before phy_connect.
> 
> For reference, please see this discussion:
> [RFC PATCH v2] arm: kirkwood: nsa310s: Use Marvell uclass mvgbe
> and PHY driver for DM Ethernet.
> https://lists.denx.de/pipermail/u-boot/2022-April/480946.html

I don't think that this is the correct approach. We should not set
the *Marvell* PHY page address in this MAC driver IMHO. This driver
should not have any PHY specific changes. This should be done in the
PHY driver instead. Some PHY driver or board specific code seems to be
responsible for setting a non-zero PHY page and not resetting it back
to zero. A better solution would be to locate this code path and add
this page reset to 0 there.

What do you think?

Thanks,
Stefan

> This patch has been tested with the following Kirkwood boards:
> 
> NSA310S (88F6702, network chip MV88E1318S)
> Sheevaplug (88F6281, network chip MV88E1318)
> Pogo V4 (88F6192, network chip 88E1116R)
> GF Home(88F6281, network chip 88E1116R)
> Dreamplug (88F6281, network chip MV88E1318)
> Dell Kace M300 (88F6282, network chip MV88E1318) - out of tree u-boot
> 
> Signed-off-by: Tony Dinh <mibodhi at gmail.com>
> ---
> 
>   drivers/net/mvgbe.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
> index 954bf86121..d2db1e584a 100644
> --- a/drivers/net/mvgbe.c
> +++ b/drivers/net/mvgbe.c
> @@ -43,6 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   #define MV_PHY_ADR_REQUEST 0xee
>   #define MVGBE_SMI_REG (((struct mvgbe_registers *)MVGBE0_BASE)->smi)
> +#define MVGBE_PGADR_REG	22
>   
>   #if defined(CONFIG_PHYLIB) || defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
>   static int smi_wait_ready(struct mvgbe_device *dmvgbe)
> @@ -745,6 +746,9 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev,
>   	miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST,
>   		     phyid);
>   
> +	/* Make sure the selected PHY page is 0 before connecting */
> +	miiphy_write(dev->name, phyid, MVGBE_PGADR_REG, 0);
> +
>   	phydev = phy_connect(bus, phyid, dev, phy_interface);
>   	if (!phydev) {
>   		printf("phy_connect failed\n");

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de