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

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

[Christian Lamparter]

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.

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).

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.

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.

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?

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.
---
 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

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

[Alvin Šipraga]

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
>

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

[Christian Lamparter]

On 6/4/23 14:03, Alvin Šipraga wrote:
> 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...

Oh, maybe I should be blunt here. This is the first time that I
got proper mii values for this RTL8363. This is revevant, because
in u-boot the vendor (Netgear) usually takes care of the "mii" tool.

But this wasn't the case here, I'm not sure if this RTL8363SB is
an odd-ball or not. This patch was meant for discussion, if the
discussion is fruitful, I fully expect to do a v2..v3 with the
information that was gathered during review.

>>
>> 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.

Oh, I overwrote the current rtl8365mb_dsa_phy_write and
rtl8365mb_mdio_phy_read to match what's I found in ASUS WRT's codebase.
 From what I gathered, this mdc-mdio was tested/developped with an RT-AC88U.
So I think you all are no stranger to the ASUSWRT Merlin Project @
https://www.asuswrt-merlin.net/. Thankfully they have a link to their
github and provide the realtek source code which includes the phy
access routines in:

https://github.com/RMerl/asuswrt-merlin.ng/blob/master/release/src-rt-6.x.4708/linux/linux-2.6.36/drivers/char/rtl8365mb/rtl8367c_asicdrv_phy.c

as rtl8367c_setAsicPHYReg and rtl8367c_getAsicPHYReg. (look in line 21 for
the #if defined(MDC_MDIO_OPERATION) until the #else 188). These two are
what I implemented in rtl8365mb_mdio_phy_write and rtl8365mb_mdio_phy_read.

>>
>> 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.

True :) and it was a pain to format. Still I'm hoping to get confirmation
about 0x001cc980-ish PHYID. This is something all switches should produce and
this should be repoduceable by others (with slightly different IDs).
The source for the phytool: https://github.com/wkz/phytool )

>> 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.

Yes, I'm fully expecting these comments.

> 
> 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.

There are sadly the only useful comment in the vendor driver is "Default OCP Address"
with the associated 0x29 "Magic value". This 0x29 becomes 0xA400 if you put it through
the FIELD_PREP... And funnily enough, that exactly matches the existing
"RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE" which is used by the current accessors (but they
do much much more).

It looks like the person that wrote the rtl8365mb_phy_ocp_read and
rtl8365mb_phy_ocp_write() could answer in detail what's behind this.
I'm sure this would be on a datasheet, but sadly I don't have one for the switch.

> 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.

Oh, this is no longer the case. I have it sort of working now. The "no traffic"
issue was "fixed" by the second patch
"net: dsa: realtek: rtl8365mb: add missing case for digital interface 0".

Though, It's not 100% finished. "Normal data" works, but it seems the switch
doesn't like that I have multiple other VLANs on the same network (including
non-vlan traffic). I see a constant stream of "non-realtek ethertype ..."
message from tag_rtl8_4.c. These include 802.1Q (0x8100) EtherTypes,
IPv4 (0x0800) and IPv6 (86DD)). Though, I'm optimistic that this can be solved.

>>
>> 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.

Yes, the provided jam_tables are enough to get the switch "online".
Thing is, I tried openwrt's rtl8367b.c tables too. But didn't notice any
difference. (The WNDAP660 isn't capable of "saturating" the 1 Gbit/s.
So, it's unlikely that I can trigger any performance related bugs by
simply flooding it.

Regards,
Christian

> 
>> ---
>>   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
>>

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

[Alvin Šipraga]

On Sun, Jun 04, 2023 at 04:42:58PM +0200, Christian Lamparter wrote:
> On 6/4/23 14:03, Alvin Šipraga wrote:
> > 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...
> 
> Oh, maybe I should be blunt here. This is the first time that I
> got proper mii values for this RTL8363. This is revevant, because
> in u-boot the vendor (Netgear) usually takes care of the "mii" tool.
> 
> But this wasn't the case here, I'm not sure if this RTL8363SB is
> an odd-ball or not. This patch was meant for discussion, if the
> discussion is fruitful, I fully expect to do a v2..v3 with the
> information that was gathered during review.

No problem, but I think it's not that useful in the final commit message. Just
my 2c...

> 
> > > 
> > > 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.
> 
> Oh, I overwrote the current rtl8365mb_dsa_phy_write and
> rtl8365mb_mdio_phy_read to match what's I found in ASUS WRT's codebase.
> From what I gathered, this mdc-mdio was tested/developped with an RT-AC88U.
> So I think you all are no stranger to the ASUSWRT Merlin Project @
> https://www.asuswrt-merlin.net/. Thankfully they have a link to their
> github and provide the realtek source code which includes the phy
> access routines in:
> 
> https://github.com/RMerl/asuswrt-merlin.ng/blob/master/release/src-rt-6.x.4708/linux/linux-2.6.36/drivers/char/rtl8365mb/rtl8367c_asicdrv_phy.c
> 
> as rtl8367c_setAsicPHYReg and rtl8367c_getAsicPHYReg. (look in line 21 for
> the #if defined(MDC_MDIO_OPERATION) until the #else 188). These two are
> what I implemented in rtl8365mb_mdio_phy_write and rtl8365mb_mdio_phy_read.

Hmm, so the sources I think you should follow are here (also GPL'ed):

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files/drivers/net/phy/rtk/rtl8367c/rtl8367c_asicdrv_phy.c;h=fb4db113a9870a3aaa785b8765fa5a341a407c3b;hb=HEAD

... and I guess you want to just implement an alternative
rtl8365mb_phy_ocp_{read,write} based on rtl8367c_{get,set}AsicPHYOCPReg when
MDC_MDIO_OPERATION is defined. As I see it, there are a few differences. Could
you try with this approach and let me know if it works? This will be more
consistent as the driver will remain based on the same set of sources.

> 
> > > 
> > > 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.
> 
> True :) and it was a pain to format. Still I'm hoping to get confirmation
> about 0x001cc980-ish PHYID. This is something all switches should produce and
> this should be repoduceable by others (with slightly different IDs).
> The source for the phytool: https://github.com/wkz/phytool )

0x001cc942 is what my (SMI-connected) RTL8365MB-VC-CG reports:

| # phytool swp0/0
| ieee-phy: id:0x001cc942
| 
|    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:0x79c9
|       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

Since your switch reports something else, you will want to update
drivers/net/phy/realtek.c if you want to have the chip deliver
interrupts. Otherwise you end up with the Generic PHY driver polling for PHY
status.

> 
> > > 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.
> 
> Yes, I'm fully expecting these comments.

Cool, let's wait for him to reply/test as well.

> 
> > 
> > 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.
> 
> There are sadly the only useful comment in the vendor driver is "Default OCP Address"
> with the associated 0x29 "Magic value". This 0x29 becomes 0xA400 if you put it through
> the FIELD_PREP... And funnily enough, that exactly matches the existing
> "RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE" which is used by the current accessors (but they
> do much much more).

Please compare with the link I sent above and tell me what you think.

> 
> It looks like the person that wrote the rtl8365mb_phy_ocp_read and
> rtl8365mb_phy_ocp_write() could answer in detail what's behind this.
> I'm sure this would be on a datasheet, but sadly I don't have one for the switch.

That would be me, but I just followed what was done in the vendor sources... The
registers are a bit of a mess and I don't know what OCP stands for, so I am not
much better informed than anybody else. I would just like to keep things more or
less consistent, i.e. make the code look something like:

  phy_read() {
    /* prep the OCP_MSB_0_REG */
  
    if (mdio)
      /* just read from the suitable register */
    else
      /* do the INDIRECT_ACCESS_*_REG write/poll/read dance */
  
    return val;
  }

... somewhat matching the two different access methods in the vendor sources I
linked.

> 
> > 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.
> 
> Oh, this is no longer the case. I have it sort of working now. The "no traffic"
> issue was "fixed" by the second patch
> "net: dsa: realtek: rtl8365mb: add missing case for digital interface 0".
> 
> Though, It's not 100% finished. "Normal data" works, but it seems the switch
> doesn't like that I have multiple other VLANs on the same network (including
> non-vlan traffic). I see a constant stream of "non-realtek ethertype ..."
> message from tag_rtl8_4.c. These include 802.1Q (0x8100) EtherTypes,
> IPv4 (0x0800) and IPv6 (86DD)). Though, I'm optimistic that this can be solved.

Could be your MAC doing something weird, but I'm not sure. You could try using
the trailing tag instead of the leading tag. Luiz or Arınç may also be able to
help you.

Kind regards,
Alvin

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

[Luiz Angelo Daros de Luca]

Hi, Christian

> 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.
>
> 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).
>
> 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.
>
> 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.
>
> 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?

It seems that the indirect read using the register protocol does not
work with your device. Yes, I could have used the memory mapped under
the 0x2000 base to read those indirect registers, but I chose to keep
the SMI-way as it was working even for an MDIO device. Using the same
code path also helped uncover some issues with SMI. I didn't test
writing to those registers, but reading from them does work with my
RTL8367S.

Perhaps in your device generation, there is something different in the
indirect read protocol used by SMI but optional (or non-existing) for
MDIO devices. It might be worth trying to check if the SMI protocol
used by other old drivers would work with your device and how they
differ from the current rtl8365mb.c. If not, another RTL8363SB
connected through SMI might also break with rtl8365mb.c. I believe
that keeping a single code path independently the interface is
beneficial for the driver.

If we do need to have different phy_read/write methods, especially if
RTL8363SB SMI needs a different protocol, I think we could convert the
dsa_switch_ops into a dynamic structure and let the interface driver
(realtek-smi/mdio) select the methods.

The reason we have two ops for SMI and MDIO is not really related to
differences between them. It's only because realtek-smi implemented a
custom slave-mii, while realtek-mdio used the generic one that dsa
creates. I wrote a series of patches that eliminate that difference
and clean up some code. It was submitted last year but didn't progress
any further. I rebased it on net-next:

https://github.com/luizluca/linux/tree/realtek_deprecate_slave_mdio

Alvin, maybe we should revisit the series before introducing more complexity.

> So far, I'm just reusing the existing jam tables. rtl8367b.c jam
> tables ones don't help with getting "traffic".

I did some decoding while I was debugging an issue during realtek-mdio
implementation, but I stopped and never submitted it (and I probably
lost it). There were a lot of register declarations, getters and
setters, converting a dozen lines into hundreds. Alvin, is that level
of verbosity acceptable? We could start by declaring the register
names. We need to read the vendor code to understand the context of
each register, as the docs we find online (search for "realtek
unmanaged switch programming guide") don't provide detailed
information about the registers. Even the vendor code sometimes uses
magic number shortcuts instead of calling its own functions.

Regarding the non-realtek packets, it might be the switch trapping
some special cases to the CPU port. Check if the IPv6 case, for
example, is not actually a multicast request.
RTL8365MB_CPU_INSERT_TO_ALL should handle that job, but your switch
might handle things differently.

Regards,