Re: [PATCH net-next 14/15] net: dsa: mt7530: introduce driver for MT7988 built-in switch

From: Arınç ÜNAL
Date: Fri Mar 31 2023 - 08:07:50 EST

On 31.03.2023 13:16, Daniel Golle wrote:
On Fri, Mar 31, 2023 at 08:50:28AM +0300, Arınç ÜNAL wrote:
On 30.03.2023 18:23, Daniel Golle wrote:
Add driver for the built-in Gigabit Ethernet switch which can be found
in the MediaTek MT7988 SoC.

The switch shares most of its design with MT7530 and MT7531, but has
it's registers mapped into the SoCs register space rather than being
connected externally or internally via MDIO.

Introduce a new platform driver to support that.

Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
---
MAINTAINERS | 2 +
drivers/net/dsa/Kconfig | 12 ++++
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/mt7530-mmio.c | 101 ++++++++++++++++++++++++++++++++++
drivers/net/dsa/mt7530.c | 86 ++++++++++++++++++++++++++++-
drivers/net/dsa/mt7530.h | 12 ++--
6 files changed, 206 insertions(+), 8 deletions(-)
create mode 100644 drivers/net/dsa/mt7530-mmio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 14924aed15ca7..674673dbdfd8b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13174,9 +13174,11 @@ MEDIATEK SWITCH DRIVER
M: Sean Wang <sean.wang@xxxxxxxxxxxx>
M: Landen Chao <Landen.Chao@xxxxxxxxxxxx>
M: DENG Qingfang <dqfext@xxxxxxxxx>
+M: Daniel Golle <daniel@xxxxxxxxxxxxxx>
L: netdev@xxxxxxxxxxxxxxx
S: Maintained
F: drivers/net/dsa/mt7530-mdio.c
+F: drivers/net/dsa/mt7530-mmio.c
F: drivers/net/dsa/mt7530.*
F: net/dsa/tag_mtk.c
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index c2551b13324c2..de4d86e37973f 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -52,6 +52,18 @@ config NET_DSA_MT7530
Multi-chip module MT7530 in MT7621AT, MT7621DAT, MT7621ST and
MT7623AI SoCs is supported as well.
+config NET_DSA_MT7988
+ tristate "MediaTek MT7988 built-in Ethernet switch support"
+ select NET_DSA_MT7530_COMMON
+ depends on HAS_IOMEM
+ help
+ This enables support for the built-in Ethernet switch found
+ in the MediaTek MT7988 SoC.
+ The switch is a similar design as MT7531, however, unlike
+ other MT7530 and MT7531 the switch registers are directly
+ mapped into the SoCs register space rather than being accessible
+ via MDIO.
+
config NET_DSA_MV88E6060
tristate "Marvell 88E6060 ethernet switch chip support"
select NET_DSA_TAG_TRAILER
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 71250d7dd41af..103a33e20de4b 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -8,6 +8,7 @@ endif
obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
obj-$(CONFIG_NET_DSA_MT7530_COMMON) += mt7530.o
obj-$(CONFIG_NET_DSA_MT7530) += mt7530-mdio.o
+obj-$(CONFIG_NET_DSA_MT7988) += mt7530-mmio.o

I'm not fond of this way. Wouldn't it be better if we split the mdio and
mmio drivers to separate modules and kept switch hardware support on
mt7530.c?

You mean this in terms of Kconfig symbols?
Because the way you describe is basically what I'm doing here:
* mt7530.c is the shared/common switch hardware driver
* mt7530-mdio.c contains the MDIO accessors and MDIO device drivers for
MT7530, MT7531, MT7621, MT7623, ...
* mt7530-mmio.c contains the platform device driver for in-SoC switches
which are accessed via MMIO, ie. MT7988 (and yes, this could be
extended to also support MT7620A/N).

Ok great.


In early drafts I also named the Kconfig symbols
CONFIG_NET_DSA_MT7530 for mt7530.c (ie. the common part)
CONFIG_NET_DSA_MT7530_MDIO for the MDIO driver
CONFIG_NET_DSA_MT7530_MMIO for the MMIO driver

However, as existing kernel configurations expect CONFIG_NET_DSA_MT7530 to
select the MDIO driver, I decided it would be better to hide the symbol of
the common part and have CONFIG_NET_DSA_MT7530 select the MDIO driver like
it was before.

You can "imply NET_DSA_MT7530_MDIO" from NET_DSA_MT7530 so the MDIO driver is also enabled when NET_DSA_MT7530 is selected. For example, on Realtek, both MDIO and SMI drivers are enabled by default when either of the main drivers are selected.

config NET_DSA_MT7530
tristate "MediaTek MT7530 and MT7531 Ethernet switch support"
select NET_DSA_TAG_MTK
select MEDIATEK_GE_PHY
select PCS_MTK_LYNXI
imply NET_DSA_MT7530_MDIO
imply NET_DSA_MT7530_MMIO


Hence I decided to go with
CONFIG_NET_DSA_MT7530 selects the MDIO driver, just like before
CONFIG_NET_DSA_MT7988 selects the new MMIO driver
CONFIG_NET_DSA_MT7530_COMMON is hidden, selected by both of the above


The mmio driver could be useful in the future for the MT7530 on the MT7620
SoCs or generally new hardware that would use MMIO to be controlled.


Sure, it would be a bit confusing once we add support for MT7620A/N (if
that ever happens...), then CONFIG_NET_DSA_MT7988 would need to be
selected to support this ancient MIPS SoC...

If you are planning to work on support for MT7620A/N feel free to suggest
a better way to name the Kconfig symbols.

I don't plan to but Luiz may. Onto my suggestions.

Firstly, all of the functions on the mt7530-mmio driver should be changed from mt7988_* to mt7530_mmio_*. Same goes for the mt7530-mdio driver too as some of the functions don't start with mt7530_mdio_*. The MDIO and MMIO drivers are supposed to be used for the switches the MT7530 DSA driver supports. The mt7530_ prefix is derived from that. The mmio_ or mdio_ prefix is derived from, well, the driver itself.

You're calling the .name of the MMIO driver, mt7988-switch; the MDIO driver mt7530. That doesn't make sense. They should be mt7530-mmio and mt7530-mdio (or mediatek-mmio and mediatek-mdio).

What I'm going to say next depends on how generic the MMIO and MDIO drivers are so that they can be used on all MediaTek architecture switches. Let's say, a new MediaTek switch is introduced. It seems likely that either the MMIO or MDIO driver will be used to control the switch. Maybe the driver for this new switch won't be under mt7530.c, like on Realtek, but that doesn't change the outcome.

You know the MMIO and MDIO drivers better than I do, so if this makes sense to you, I'd rather call the MDIO and MMIO drivers MediaTek MDIO and MediaTek MMIO, and change the code accordingly. E.g. mt7988_* to mediatek_mmio_*, the filename from mt7530-mmio.c to mediatek-mmio.c, kernel config option from NET_DSA_MT7530_MMIO to NET_DSA_MEDIATEK_MMIO. This is currently the case with the Realtek MDIO and SMI drivers.

If not, call it MediaTek MT7530 MMIO and MediaTek MT7530 MDIO.


Luiz did this for the Realtek switches that use MDIO and SMI to be
controlled.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/realtek/Kconfig

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/realtek/Makefile

Are you suggesting to split-off a device-specific driver which would
then select the access-method driver (MDIO vs. MMIO) and the
common/shared driver? To me this looks like overkill for MT7530, given
that the designs of all MT7530 are pretty similar, ie. same tag format
and also otherwise very similar.

No, and agreed. I just wanted to show the Realtek MDIO and SMI drivers' kconfig as an example.



Thank you for reviewing!

Great work so far!

Arınç