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

[Luiz Angelo Daros de Luca <luizluca@gmail.com>]

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,