Re: [PATCH v2 16/24] net: phy: Add Sanechips ZX5201 PHY support

[Andrew Lunn]

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Sanechips ZX5201 Ethernet PHY
> + *
> + * The ZX5201 answers on two MDIO addresses: the copper PHY on its own
> + * address and its MAC-side SerDes interface on the next one.

What do you get when you read registers 2 and 3 of the SERDES? Is
there an ID in them?

If the mdio bus is scanned, and there is an ID, phylib will try to
probe a PHY driver on the SERDES.

> +static int zx5201_sds_read(struct phy_device *phydev, u32 regnum)
> +{
> +	return mdiobus_read(phydev->mdio.bus, phydev->mdio.addr + 1, regnum);
> +}

I would suggest adding a probe, and validate phydev->mdio.addr is not
31. ENODEV, or EINVAL, to indicate the setup is invalid.

> +static int zx5201_config_init(struct phy_device *phydev)
> +{
> +	static const u16 led_regs[] = { 0xb409, 0xb407, 0xb406, 0xb408 };

> +	/* LED control registers, all cleared as the vendor firmware does */
> +	for (i = 0; i < ARRAY_SIZE(led_regs); i++) {
> +		ret = zx5201_ext_write(phydev, led_regs[i], 0);
> +		if (ret)
> +			return ret;

It would be nice to reverse engineer what these bits do. And then add
proper LED controls. However, that can be later.

Ah, now i'm confused:

https://www.sanechips.com.cn/en/ProductEn/WiredEn/ethernetConnectivityEn/index.html

This says it is an RGMII PHY. Why does it have a SERDES? That suggests
SGMII?

We need to understand this, because if it is an RGMII PHY, we need to
address the RGMII delays, which that page says it can controller.

	Andrew