Re: [PATCH v1] net: dsa: realtek: rtl8365mb: use mdio passthrough to access PHYs

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On Fri, Jun 02, 2023 at 07:02:31PM +0200, Christian Lamparter wrote:
> when bringing up the PHYs on a Netgear WNDAP660, I observed that
> none of the PHYs are getting enumerated and the rtl8365mb fails
> to load.
> 
> | realtek-mdio [...] lan1 (unini...): validation of gmii with support \
> |   0...,0.,..6280 and advertisement 0...,0...,6280 failed: -EINVAL
> | realtek-mdio [...] lan1 (uninit...): failed to connect to PHY: -EINVAL
> | realtek-mdio [...] lan1 (uninit...): error -22 setting up PHY for
> |   tree 0, switch 0, port 0
> 
> with phytool, all registers just returned "0000".
> 
> Now, the same behavior was present with the swconfig version of
> rtl8637b.c and in the device's uboot the "mii" register access
> utility also reports bogus values.

Not really relevant...

> 
> The Netgear WNDAP660 might be somewhat special, since the RTL8363SB
> uses exclusive MDC/MDIO-access (instead of SMI). (And the RTL8363SB
> is not part of the supported list of this driver).

We had other MDIO switches with support added, so I don't think it's unique.

> 
> Since this was all hopeless, I dug up some datasheet when searching
> for solutions:
> "10/100M & 10/100/1000M Switch Controller Programming Guide".
> It had an interesting passage that pointed to a magical
> MDC_MDIO_OPERATION define which resulted in different slave PHY
> access for the MDIO than it was implemented for SMI.

Got a reference? I do not see MDC_MDIO_OPERATION in your patch.

> 
> With this implemented, the RTL8363SB PHYs came to live:
> 
> | [...]: found an RTL8363SB-CG switch
> | [...]: missing child interrupt-controller node
> | [...]: no interrupt support
> | [...]: configuring for fixed/rgmii link mode
> | [...] lan1 (uninit...): PHY [dsa-0.0:01] driver [Generic PHY] (irq=POLL)
> | [...] lan2 (uninit...): PHY [dsa-0.0:02] driver [Generic PHY] (irq=POLL)
> | device eth0 entered promiscuous mode
> | DSA: tree 0 setup
> | realtek-mdio 4ef600c00.ethernet:00: Link is Up - 1Gbps/Full - [...]
> 
> | # phytool lan1/2
> | ieee-phy: id:0x001cc980 <--- this is correct!!
> |
> |  ieee-phy: reg:BMCR(0x00) val:0x1140
> |     flags:          -reset -loopback +aneg-enable -power-down
> |		      -isolate -aneg-restart -collision-test
> |     speed:          1000-full
> |
> |  ieee-phy: reg:BMSR(0x01) val:0x7969
> |     capabilities:   -100-b4 +100-f +100-h +10-f +10-h -100-t2-f
> |		      -100-t2-h
> |      flags:         +ext-status +aneg-complete -remote-fault
> |		      +aneg-capable -link -jabber +ext-register
> 
> the port statistics are working too and the exported LED triggers.
> But so far I can't get any traffic to pass.

This info is also not entirely relevant in a commit message, but thanks for
clarifying.

> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> Any good hints or comments? Is the RTL8363SB an odd one here and
> everybody else can just use SMI?

Luiz implemented MDIO support presumably because he could not use SMI. I think
at the very least, he or somebody else should test your patch on all existing
MDIO-wired chips supported by the driver. Since this could cause regressions. If
that is not possible but you would still like to support your new switch, maybe
we need both implementations available.

Regarding your patch, I do not fully understand it. I think you could perhaps
explain it a little more clearly. Basically I see that you are provisioning the
GPHY_OCP_MSB_0_REG a little bit differently, and then executing a register read
at some particular offset. Contrast this with the current indirect access
command register followed by polling. I think yours is a better approach, since
it is more direct, but only if it works and is well documented.

Perhaps you can give a pointer to which logic in the vendor driver you followed
in order to achieve this more direct register access without polling. This will
help me review it :)

But since you still haven't got data through your switch, I am a bit reluctant
to approve this kind of change. I would prefer to see a full series adding the
support, so that this kind of change/quirk is justfieid. Otherwise it is just
introducing potential regressions with no real benefit. I hope you understand.

> 
> So far, I'm just reusing the existing jam tables. rtl8367b.c jam
> tables ones don't help with getting "traffic". There are also the
> phy_read in realtek_ops, but it doesn't look like realtek-mdio.c
> is using those? So I left them as is.

Just fyi, the jam table is a bit of a fudge and might not be correct for all
switches. It's basically a blob lifted from the vendor driver. It should
probably be revisited and open-coded so that a human can read it. But I don't
mean to imply that this is why you don't get data through your switch.

And yes, realtek_ops::phy_{read,write} are only used by the SMI interface.

Kind regards,
Alvin

> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 78 +++++++++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 41ea3b5a42b1..6c00e6dcb193 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -825,15 +825,85 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum,
>  	return 0;
>  }
>  
> +static int rtl8365mb_mdio_phy_read(struct realtek_priv *priv, int phy, int regnum)
> +{
> +	unsigned int val, addr;
> +	int ret;
> +
> +	if (phy > RTL8365MB_PHYADDRMAX)
> +		return -EINVAL;
> +
> +	if (regnum > RTL8365MB_PHYREGMAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->map_lock);
> +	ret = regmap_update_bits(priv->map_nolock, RTL8365MB_GPHY_OCP_MSB_0_REG,
> +				 RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK, /* 0xA40 */
> +				 FIELD_PREP(RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK,
> +					    FIELD_GET(RTL8365MB_PHY_OCP_ADDR_PREFIX_MASK,
> +						      RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE)));
> +	if (ret) {
> +		mutex_unlock(&priv->map_lock);
> +		return ret;
> +	}
> +
> +	addr = RTL8365MB_PHY_BASE |
> +	       FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy) |
> +	       regnum;
> +	ret = regmap_read(priv->map_nolock, addr, &val);
> +	mutex_unlock(&priv->map_lock);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(priv->dev, "read PHY%d register 0x%02x, val <- %04x\n",
> +		phy, regnum, val);
> +
> +	return val & 0xFFFF;
> +}
> +
> +static int rtl8365mb_mdio_phy_write(struct realtek_priv *priv, int phy, int regnum,
> +				    u16 val)
> +{
> +	unsigned int addr;
> +	int ret;
> +
> +	if (phy > RTL8365MB_PHYADDRMAX)
> +		return -EINVAL;
> +
> +	if (regnum > RTL8365MB_PHYREGMAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->map_lock);
> +	ret = regmap_update_bits(priv->map_nolock, RTL8365MB_GPHY_OCP_MSB_0_REG,
> +				 RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK,
> +				 FIELD_PREP(RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK,
> +					    FIELD_GET(RTL8365MB_PHY_OCP_ADDR_PREFIX_MASK,
> +						      RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE)));
> +	if (ret) {
> +		mutex_unlock(&priv->map_lock);
> +		return ret;
> +	}
> +
> +	addr = RTL8365MB_PHY_BASE |
> +	       FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy) |
> +	       regnum;
> +	ret = regmap_write(priv->map_nolock, addr, val);
> +	mutex_unlock(&priv->map_lock);
> +
> +	dev_dbg(priv->dev, "write (%d) PHY%d register 0x%02x val -> %04x\n",
> +		ret, phy, regnum, val);
> +
> +	return ret;
> +}
> +
>  static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
>  {
> -	return rtl8365mb_phy_read(ds->priv, phy, regnum);
> +	return rtl8365mb_mdio_phy_read(ds->priv, phy, regnum);
>  }
>  
> -static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
> -				   u16 val)
> +static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
>  {
> -	return rtl8365mb_phy_write(ds->priv, phy, regnum, val);
> +	return rtl8365mb_mdio_phy_write(ds->priv, phy, regnum, val);
>  }
>  
>  static const struct rtl8365mb_extint *
> -- 
> 2.40.1
>