Hi Jiri,

On Wed, Feb 26, 2020 at 04:54:23PM +0100, Jiri Pirko wrote:

Tue, Feb 25, 2020 at 05:30:54PM CET, vadym.kochan@plvision.eu wrote:

quoted

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 <redacted>
Signed-off-by: Andrii Savka <redacted>
Signed-off-by: Oleksandr Mazur <redacted>
Signed-off-by: Serhiy Boiko <redacted>
Signed-off-by: Serhiy Pshyk <redacted>
Signed-off-by: Taras Chornyi <redacted>
Signed-off-by: Volodymyr Mytnyk <redacted>
---
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.

quoted

+#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.

So you suggested to use prestera_ as a prefix, I dont see a problem
with that, but why not mvsw_pr_ ? So it has the vendor, device name parts
together as a key. Also it is necessary to apply prefix for the static
names ?

quoted

+static const char mvsw_driver_name[] = "mvsw_switchdev";

Why is this different from kind?

Also, don't mention "switchdev" anywhere.

quoted

+static const char mvsw_driver_version[] = PRESTERA_DRV_VER;

[...]

quoted

+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.

quoted

+}
+
+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.

quoted

+}

[...]

quoted

+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.

	
	

quoted

+}
+
+module_init(mvsw_pr_module_init);
+module_exit(mvsw_pr_module_exit);
+
+MODULE_AUTHOR("Marvell Semi.");

Does not look so :)

quoted

+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Marvell Prestera switch driver");
+MODULE_VERSION(PRESTERA_DRV_VER);

[...]

Thank you for the comments and suggestions!

Regards,
Vadym Kochan