Messages in this thread | | | From | Vadym Kochan <> | Subject | Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX325x (AC3x) | Date | Thu, 27 Feb 2020 21:32:00 +0000 |
| |
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: > >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@plvision.eu> > >Signed-off-by: Andrii Savka <andrii.savka@plvision.eu> > >Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu> > >Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu> > >Signed-off-by: Serhiy Pshyk <serhiy.pshyk@plvision.eu> > >Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu> > >Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu> > >--- > > 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. > 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 ?
> > >+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); > > [...]
Thank you for the comments and suggestions!
Regards, Vadym Kochan
| |