Re: [RFC 0/2 net-next] net: dsa: MT7628 embedded switch initial support

[Vladimir Oltean <olteanv@gmail.com>]

Hi Joris,

On Sat, Feb 28, 2026 at 08:52:40PM +0200, Joris Vaisvila wrote:
> Hello,
> 
> This RFC adds initial support for the basic functionality needed to
> operate the MT7628 embedded switch.
> 
> The driver configures the switch to isolate each port and does not
> offload any bridge functions, so all of the forwarding is done in
> software. Bridge and VLAN offloading are possible but omitted to
> simplify this initial patch set.
> 
> The special tag currently overlaps some functionality with tag_8021q,
> but it enables precise TX handling and (more) precise RX handling for
> future VLAN offloading.
> 
> Tested on MT7628AN with 5 100mbps ports. All 5 ports were tested for
> correct forwarding with two devices.
> 
> # Request for comments:
> 
> - What would be an acceptable way to present the undocumented phy init
>   in mt7628_vendor_phys_init and switch_init?

Like Andrew said. Run cat /sys/bus/mdio_bus/devices/.../phy_id for your
probed embedded PHYs, and write a specific PHY driver for that ID in
drivers/net/phy/. With some luck, you may already be using a specific
PHY driver, case in which you might be able to work out what the things
in mt7628_vendor_phys_init() do, based on what doesn't work without them.

What undocumented bits in switch_init? These?

	/* undocumented init sequence from openwrt/uboot */
	regmap_write(esw->regmap, MT7628_ESW_REG_FCT0, 0xC8A07850);
	regmap_write(esw->regmap, MT7628_ESW_REG_SGC2, 0x00000000);

I ran a Google search for MT7628 and found:
https://vonger.cn/upload/MT7628_Full.pdf

It says:
FCT0 (Flow Control Threshold 0)
Bits 31:24 FC_RLS_TH   Flow Control Release Threshold
Bits 23:16 FC_SET_TH   Flow Control Set Threshold
Bits 15:8  DRO_RLS_TH  Drop Release Threshold
Bits 7:0   DROP_SET_TH Drop Set Threshold

SGC2 (Switch Global Control 2)
Bits 31    P6_RXFC_QUE_EN       Port 6 RX flow control on per egress queue
Bits 30    P6_TXFC_WL_EN        Port 6 TX flow controll by Switch WAN/LAN port
Bits 29:24 LAN_PMAP             Lan port bit map
Bits 23    SPECIAL_TAG_EN       Special Tag enable
Bits 22:16 TX_CPU_TPID_BIT_MAP  Transmit CPU TPID(810x) port bit map
Bits 12    P6_TXFC_QUE_EN       Port 6 per queue TX flow control
Bits 11    ARBITER_LAN_EN       Memory arbiter only for P0~P4 enable
Bits 10    CPU_TPID_EN          CPU TPID(81xx) enable
Bits 9     ARBITER_GPT_EN       Memory Arbiter only for P5 and P6
Bits 8     SLOT_4TO1            Memory Arbiter Ratio Selection
Bits 6:0   DOUBLE_TAG_EN        Insert double tag field

I don't really understand why you left them "undocumented"?

The PHY registers indeed seem to not be described in that PDF. But maybe
it's an embedded version of a similar discrete PHY for which you can find
documentation (elsewhere).

> - Should I aim to set all register default values for general switch
>   config or is relying on the documented switch reset state acceptable?

Too little information to answer the question. We need to know if the
documented out-of-reset state of the switch is reliable. You have more
time spent with the switch, so it's probably a question for you. You
assert two reset domains - rst_esw and rst_ephy. If that is enough to
restore the hardware to the documented states, then all is probably
fine. If not, you'd need to explain what is persistent across resets,
and who else may have touched it (bootloader, kexec etc).

If it was more of a general question - from network maintainers there is
no requirement that you redundantly write all registers with what is
supposed to be their out-of-reset value.

> - I am also looking for any and all feedback on the architecture and
>   coding style of this driver.

Oh, this is the fun part :)

I'll start in order with what I think is the most important. I would
love to know more about the tag_8021q and VLAN unaware implementation,
but that may be made irrelevant by something else, so I'll skip it for
now.

I found some references in the documentation to a block named "Switch
DMA (SDM)" starting with base address 10100800. Can you comment on what
purpose this block serves? At a superficial glance, it appears to allow
packet injection into the switch from the embedded MIPS CPU. I wonder,
in that case, whether packet I/O can be done without DSA tags through
those packet rings (case in which a pure switchdev driver might have
been more appropriate than DSA).

I looked at arch/mips/boot/dts/ralink/mt7628a.dtsi and found nothing
Ethernet-related. So it's a bit hard to comment from me whether this is
an instance of mtk_eth_soc.c or something, that is being used as a DSA
conduit. Case in which your architectural choice is fine.


Perhaps a bit more wording in the cover letter and/or commit messages
regarding the general switch block diagram would be nice, in terms that
Linux people already understand.


In terms of coding style there's a few hiccups. I would call out
incorrect alignment of arguments of multi-line functions (they should be
aligned with the open parenthesis, accounting for 8-character wide tabs).

Also, we practice an empty line between local variable declarations and
the beginning of code in a function body.

And the struct platform_driver mt7628_driver definition is indented with
spaces, not tabs, unlike the rest of the code.

And you don't need to put a comma after sentinel entries ("{},"). The
only reason we do commas after terminating array entries is to minimize
the git diff when adding new elements to the array.  But with sentinels,
we expect no follow-up by their very definition.

And we practice something called "reverse Christmas tree notation",
where local variable declarations in functions are sorted in the
decreasing order of their line length. No other subsystem except netdev
does that, but we're pretty crazy about it, so yeah, you're not going to
get us stop nagging you about it.

And we don't practice initializing variables with 0 (or NULL) as part of
their definition, when that value is going to be overwritten later in
the function with no reads in between. Examples: "ret", "val" in
mt7628_phy_read(), "ds", "esw" in mt7628_probe(), maybe more.

> Thank you for your time and feedback.
> 
> Joris Vaisvila (2):
>   net: dsa: initial MT7628 special tag driver
>   net: dsa: initial support for MT7628 embedded switch
> 
>  drivers/net/dsa/Kconfig  |   6 +
>  drivers/net/dsa/Makefile |   1 +
>  drivers/net/dsa/mt7628.c | 567 +++++++++++++++++++++++++++++++++++++++
>  include/net/dsa.h        |   2 +
>  net/dsa/Kconfig          |   6 +
>  net/dsa/Makefile         |   1 +
>  net/dsa/tag_mt7628.c     |  85 ++++++
>  7 files changed, 668 insertions(+)
>  create mode 100644 drivers/net/dsa/mt7628.c
>  create mode 100644 net/dsa/tag_mt7628.c
> 
> -- 
> 2.53.0
>