Re: net: dsa: Realtek switch drivers

[Chris Packham]

On 15/06/24 09:36, Luiz Angelo Daros de Luca wrote:
> Hello Chris and Linus,
>
> > > I'm starting to look at some L2/L3 switches with Realtek silicon. I see
> > > in the upstream kernel there are dsa drivers for a couple of simple L2
> > > switches. While openwrt has support for a lot of the more advanced
> > > silicon. I'm just wondering if there is a particular reason no-one has
> > > attempted to upstream support for these switches?
> > It began with the RTL8366RB ("RTL8366 revision B") which I think is
> > equivalent to RTL8366S as well, but have not been able to test.
> >
> > Then Luiz and Alvin jumped in and fixed up the RTL8365MB family.
> >
> > So the support is pretty much what is stated in the DT bindings
> > in Documentation/devicetree/bindings/net/dsa/realtek.yaml:
> >
> > properties:
> >    compatible:
> >      enum:
> >        - realtek,rtl8365mb
> >        - realtek,rtl8366rb
> >      description: |
> >        realtek,rtl8365mb:
> >          Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB,
> >          RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S,
> >          RTL8367SB, RTL8370MB, RTL8310SR
> >        realtek,rtl8366rb:
> >          Use with models RTL8366RB, RTL8366S
> >
> > It may look like just RTL8365 and RTL8366 on the surface but the sub-version
> > is detected at runtime.
> >
> > > If I were to start
> > > grabbing drivers from openwrt and trying to get them landed would that
> > > be a problem?
> > I think the base is there, when I started with RTL8366RB it was pretty
> > uphill but the kernel DSA experts (Vladimir & Andrew especially) are super
> > helpful so eventually we have arrived at something that works reasonably.
> >
> > The RTL8356MB-family driver is more advanced and has a lot more features,
> > notably it supports all known RTL8367 variants.
> I played with RTL8367R. It mostly works with rtl8365mb driver but I
> wasn't able to enable the CPU tagging. Although
>
> > The upstream OpenWrt in target/linux/generic/files/drivers/net/phy
> > has the following drivers for the old switchdev:
> > -rw-r--r--. 1 linus linus 25382 Jun  7 21:44 rtl8306.c
> > -rw-r--r--. 1 linus linus 40268 Jun  7 21:44 rtl8366rb.c
> > -rw-r--r--. 1 linus linus 33681 Jun  7 21:44 rtl8366s.c
> > -rw-r--r--. 1 linus linus 36324 Jun  7 21:44 rtl8366_smi.c
> > -rw-r--r--. 1 linus linus  4838 Jun  7 21:44 rtl8366_smi.h
> > -rw-r--r--. 1 linus linus 58021 Jun 12 18:50 rtl8367b.c
> > -rw-r--r--. 1 linus linus 59612 Jun 12 18:50 rtl8367.c
> >
> > As far as I can tell we cover all but RTL8306 with the current in-tree
> > drivers, the only reason these are still in OpenWrt would be that some
> > boards are not migrated to DSA.
> These drivers you listed are mostly found in old or low spec devices.
> There is little incentive to invest too much time to migrate them. For
> rtl8365mb, it still lacks support for vlan and forwarding offload. So,
> the swconfig driver still makes sense.
> There is also a performance problem with checksum offloading. These
> switches are used with non-realtek SoC, which might lead to:
>
> "Checksum offload should work with category 1 and 2 taggers when the
> DSA conduit driver declares NETIF_F_HW_CSUM in vlan_features and looks
> at csum_start and csum_offset. For those cases, DSA will shift the
> checksum start and offset by the tag size. If the DSA conduit driver
> still uses the legacy NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM in
> vlan_features, the offload might only work if the offload hardware
> already expects that specific tag (perhaps due to matching vendors).
> DSA user ports inherit those flags from the conduit, and it is up to
> the driver to correctly fall back to software checksum when the IP
> header is not where the hardware expects. If that check is
> ineffective, the packets might go to the network without a proper
> checksum (the checksum field will have the pseudo IP header sum). For
> category 3, when the offload hardware does not already expect the
> switch tag in use, the checksum must be calculated before any tag is
> inserted (i.e. inside the tagger). Otherwise, the DSA conduit would
> include the tail tag in the (software or hardware) checksum
> calculation. Then, when the tag gets stripped by the switch during
> transmission, it will leave an incorrect IP checksum in place."
> See: https://docs.kernel.org/networking/dsa/dsa.html
>
> > But maybe I missed something?
> I guess Chris is talking about the realtek target that uses Realtek
> SoC (target/linux/realtek/files-5.15/). That is a completely different
> beast. Although it might share some (or a lot) logic with current
> upstream drivers, it is way more complex. It might require a
> multi-function device driver. Anyway, the current realtek SoC/target
> drivers need some love, like using regmap, implement functions using
> an abstraction layer (and not if model a inside the code), get rid of
> all magic numbers and replace them with meaningful macros, create a
> proper tagger (and not translate a generic one just before forwarding
> it). In OpenWrt, a code that gets things done might be acceptable but
> the upstream kernel requires something more maintainable. So, if you
> want to upstream those drivers, you can start by improving them in the
> openwrt.

So now got access to the Realtek docs and I've been pouring over them 
and the openwrt code (I'm avoiding looking at the Realtek SDK for now, 
just to make sure I don't submit something I don't have the rights to).

If someone were to look at the block diagram in the brief datasheet 
they'd probably come away with the impression that it very much fits the 
DSA model. There's a SoC portion with the CPU, peripherals and a "NIC". 
That NIC is connected to the CPU MAC in the switch block. All seems like 
a pretty standard DSA type design and that's what the openwrt code 
implements a ethernet/rtl838x_eth.c driver for the Ethernet NIC and a 
dsa/rtl83xx driver for the DSA switch.

But when you start digging into the detail you find that the registers 
for the NIC are scattered through the address space for the switch. Same 
for the MDIO related registers. There is a more detailed block diagram 
in the CPU and Peripherals datasheet that shows the NIC and switch as 
part of the same IP block. The openwrt implementation does things that I 
think would be frowned upon upstream like calling from the Ethernet 
driver into the switch driver to access registers.

This leads me to conclude that what Realtek call the "NIC" is actually 
just the DMA interface for packets sent from or trapped to the CPU. 
Rather than trying to make this fit the DSA model I should be looking at 
using switchdev directly (I can still probably leverage a lot of code 
from the openwrt drivers because the switch tables are the same either way).

What would I be loosing if I don't use the DSA infrastructure? I got 
kind of hung up at the point where it really wanted a CPU port and I 
just couldn't provide a nice discrete NIC.