Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX325x (AC3x)
From: Jiri Pirko
Date: Wed Feb 26 2020 - 10:54:32 EST
Tue, Feb 25, 2020 at 05:30:54PM CET, vadym.kochan@xxxxxxxxxxx wrote:
>Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
>ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
>wireless SMB deployment.
>
>This driver implementation includes only L1 & basic L2 support.
>
>The core Prestera switching logic is implemented in prestera.c, there is
>an intermediate hw layer between core logic and firmware. It is
>implemented in prestera_hw.c, the purpose of it is to encapsulate hw
>related logic, in future there is a plan to support more devices with
>different HW related configurations.
>
>The following Switchdev features are supported:
>
> - VLAN-aware bridge offloading
> - VLAN-unaware bridge offloading
> - FDB offloading (learning, ageing)
> - Switchport configuration
>
>Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
>Signed-off-by: Andrii Savka <andrii.savka@xxxxxxxxxxx>
>Signed-off-by: Oleksandr Mazur <oleksandr.mazur@xxxxxxxxxxx>
>Signed-off-by: Serhiy Boiko <serhiy.boiko@xxxxxxxxxxx>
>Signed-off-by: Serhiy Pshyk <serhiy.pshyk@xxxxxxxxxxx>
>Signed-off-by: Taras Chornyi <taras.chornyi@xxxxxxxxxxx>
>Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@xxxxxxxxxxx>
>---
> drivers/net/ethernet/marvell/Kconfig | 1 +
> drivers/net/ethernet/marvell/Makefile | 1 +
> drivers/net/ethernet/marvell/prestera/Kconfig | 13 +
> .../net/ethernet/marvell/prestera/Makefile | 3 +
> .../net/ethernet/marvell/prestera/prestera.c | 1502 +++++++++++++++++
> .../net/ethernet/marvell/prestera/prestera.h | 244 +++
> .../marvell/prestera/prestera_drv_ver.h | 23 +
> .../ethernet/marvell/prestera/prestera_hw.c | 1094 ++++++++++++
> .../ethernet/marvell/prestera/prestera_hw.h | 159 ++
> .../marvell/prestera/prestera_switchdev.c | 1217 +++++++++++++
> 10 files changed, 4257 insertions(+)
> create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
> create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.c
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_drv_ver.h
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
> create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
>
>diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
>index 3d5caea096fb..74313d9e1fc0 100644
>--- a/drivers/net/ethernet/marvell/Kconfig
>+++ b/drivers/net/ethernet/marvell/Kconfig
>@@ -171,5 +171,6 @@ config SKY2_DEBUG
>
>
> source "drivers/net/ethernet/marvell/octeontx2/Kconfig"
>+source "drivers/net/ethernet/marvell/prestera/Kconfig"
>
> endif # NET_VENDOR_MARVELL
>diff --git a/drivers/net/ethernet/marvell/Makefile b/drivers/net/ethernet/marvell/Makefile
>index 89dea7284d5b..9f88fe822555 100644
>--- a/drivers/net/ethernet/marvell/Makefile
>+++ b/drivers/net/ethernet/marvell/Makefile
>@@ -12,3 +12,4 @@ obj-$(CONFIG_PXA168_ETH) += pxa168_eth.o
> obj-$(CONFIG_SKGE) += skge.o
> obj-$(CONFIG_SKY2) += sky2.o
> obj-y += octeontx2/
>+obj-y += prestera/
>diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig b/drivers/net/ethernet/marvell/prestera/Kconfig
>new file mode 100644
>index 000000000000..d0b416dcb677
>--- /dev/null
>+++ b/drivers/net/ethernet/marvell/prestera/Kconfig
>@@ -0,0 +1,13 @@
>+# SPDX-License-Identifier: GPL-2.0-only
>+#
>+# Marvell Prestera drivers configuration
>+#
>+
>+config PRESTERA
>+ tristate "Marvell Prestera Switch ASICs support"
>+ depends on NET_SWITCHDEV && VLAN_8021Q
>+ ---help---
>+ This driver supports Marvell Prestera Switch ASICs family.
>+
>+ To compile this driver as a module, choose M here: the
>+ module will be called prestera_sw.
>diff --git a/drivers/net/ethernet/marvell/prestera/Makefile b/drivers/net/ethernet/marvell/prestera/Makefile
>new file mode 100644
>index 000000000000..9446298fb7f4
>--- /dev/null
>+++ b/drivers/net/ethernet/marvell/prestera/Makefile
>@@ -0,0 +1,3 @@
>+# SPDX-License-Identifier: GPL-2.0
>+obj-$(CONFIG_PRESTERA) += prestera_sw.o
>+prestera_sw-objs := prestera.o prestera_hw.o prestera_switchdev.o
>diff --git a/drivers/net/ethernet/marvell/prestera/prestera.c b/drivers/net/ethernet/marvell/prestera/prestera.c
>new file mode 100644
>index 000000000000..12d0eb590bbb
>--- /dev/null
>+++ b/drivers/net/ethernet/marvell/prestera/prestera.c
>@@ -0,0 +1,1502 @@
>+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
>+ *
>+ * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
>+ *
>+ */
>+#include <linux/kernel.h>
>+#include <linux/module.h>
>+#include <linux/list.h>
>+#include <linux/netdevice.h>
>+#include <linux/netdev_features.h>
>+#include <linux/etherdevice.h>
>+#include <linux/ethtool.h>
>+#include <linux/jiffies.h>
>+#include <net/switchdev.h>
>+
>+#include "prestera.h"
>+#include "prestera_hw.h"
>+#include "prestera_drv_ver.h"
>+
>+#define MVSW_PR_MTU_DEFAULT 1536
>+
>+#define PORT_STATS_CACHE_TIMEOUT_MS (msecs_to_jiffies(1000))
>+#define PORT_STATS_CNT (sizeof(struct mvsw_pr_port_stats) / sizeof(u64))
Keep the prefix for all defines withing the file. "PORT_STATS_CNT"
looks way to generic on the first look.
>+#define PORT_STATS_IDX(name) \
>+ (offsetof(struct mvsw_pr_port_stats, name) / sizeof(u64))
>+#define PORT_STATS_FIELD(name) \
>+ [PORT_STATS_IDX(name)] = __stringify(name)
>+
>+static struct list_head switches_registered;
>+
>+static const char mvsw_driver_kind[] = "prestera_sw";
Please be consistent. Make your prefixes, name, filenames the same.
For example:
prestera_driver_kind[] = "prestera";
Applied to the whole code.
>+static const char mvsw_driver_name[] = "mvsw_switchdev";
Why is this different from kind?
Also, don't mention "switchdev" anywhere.
>+static const char mvsw_driver_version[] = PRESTERA_DRV_VER;
[...]
>+static void mvsw_pr_port_remote_cap_get(struct ethtool_link_ksettings *ecmd,
>+ struct mvsw_pr_port *port)
>+{
>+ u64 bitmap;
>+
>+ if (!mvsw_pr_hw_port_remote_cap_get(port, &bitmap)) {
>+ mvsw_modes_to_eth(ecmd->link_modes.lp_advertising,
>+ bitmap, 0, MVSW_PORT_TYPE_NONE);
>+ }
Don't use {} for single statement. checkpatch.pl should warn you about
this.
>+}
>+
>+static void mvsw_pr_port_duplex_get(struct ethtool_link_ksettings *ecmd,
>+ struct mvsw_pr_port *port)
>+{
>+ u8 duplex;
>+
>+ if (!mvsw_pr_hw_port_duplex_get(port, &duplex)) {
>+ ecmd->base.duplex = duplex == MVSW_PORT_DUPLEX_FULL ?
>+ DUPLEX_FULL : DUPLEX_HALF;
>+ } else {
>+ ecmd->base.duplex = DUPLEX_UNKNOWN;
>+ }
Same here.
>+}
[...]
>+static void __exit mvsw_pr_module_exit(void)
>+{
>+ destroy_workqueue(mvsw_pr_wq);
>+
>+ pr_info("Unloading Marvell Prestera Switch Driver\n");
No prints like this please.
>+}
>+
>+module_init(mvsw_pr_module_init);
>+module_exit(mvsw_pr_module_exit);
>+
>+MODULE_AUTHOR("Marvell Semi.");
Does not look so :)
>+MODULE_LICENSE("GPL");
>+MODULE_DESCRIPTION("Marvell Prestera switch driver");
>+MODULE_VERSION(PRESTERA_DRV_VER);
[...]