* [PATCH RFC 0/3] add Vertexcom MSE102x support
@ 2021-09-14 15:17 Stefan Wahren
2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Stefan Wahren @ 2021-09-14 15:17 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski, Rob Herring
Cc: Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree,
Stefan Wahren
This patch series adds support for the Vertexcom MSE102x Homeplug GreenPHY
chips [1]. They can be connected either via RGMII, RMII or SPI to a host CPU.
These patches handles only the last one, with an Ethernet over SPI protocol
driver.
The code has been tested only on Raspberry Pi boards, but should work
on other platforms.
Any comments about the code are welcome.
[1] - http://www.vertexcom.com/p_homeplug_plc_en.html
Stefan Wahren (3):
dt-bindings: add vendor Vertexcom
dt-bindings: net: add Vertexcom MSE102x support
net: vertexcom: Add MSE102x SPI support
.../bindings/net/vertexcom-mse102x.yaml | 71 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
drivers/net/ethernet/Kconfig | 1 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/vertexcom/Kconfig | 25 +
drivers/net/ethernet/vertexcom/Makefile | 6 +
drivers/net/ethernet/vertexcom/mse102x.c | 803 ++++++++++++++++++
7 files changed, 909 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
create mode 100644 drivers/net/ethernet/vertexcom/Kconfig
create mode 100644 drivers/net/ethernet/vertexcom/Makefile
create mode 100644 drivers/net/ethernet/vertexcom/mse102x.c
--
2.17.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom 2021-09-14 15:17 [PATCH RFC 0/3] add Vertexcom MSE102x support Stefan Wahren @ 2021-09-14 15:17 ` Stefan Wahren 2021-09-21 20:55 ` Rob Herring 2021-09-14 15:17 ` [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support Stefan Wahren 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren 2 siblings, 1 reply; 14+ messages in thread From: Stefan Wahren @ 2021-09-14 15:17 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, Rob Herring Cc: Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree, Stefan Wahren Add vendor prefix for Vertexcom Technologies, Inc [1]. [1] - http://www.vertexcom.com/ Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index a867f7102c35..0610e4323724 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1250,6 +1250,8 @@ patternProperties: description: Variscite Ltd. "^vdl,.*": description: Van der Laan b.v. + "^vertexcom,.*": + description: Vertexcom Technologies, Inc. "^via,.*": description: VIA Technologies, Inc. "^videostrong,.*": -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom 2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren @ 2021-09-21 20:55 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-09-21 20:55 UTC (permalink / raw) To: Stefan Wahren Cc: Jakub Kicinski, Michael Heimpold, linux-kernel, devicetree, netdev, David S. Miller, Rob Herring, jimmy.shen On Tue, 14 Sep 2021 17:17:15 +0200, Stefan Wahren wrote: > Add vendor prefix for Vertexcom Technologies, Inc [1]. > > [1] - http://www.vertexcom.com/ > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support 2021-09-14 15:17 [PATCH RFC 0/3] add Vertexcom MSE102x support Stefan Wahren 2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren @ 2021-09-14 15:17 ` Stefan Wahren 2021-09-21 21:09 ` Rob Herring 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren 2 siblings, 1 reply; 14+ messages in thread From: Stefan Wahren @ 2021-09-14 15:17 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, Rob Herring Cc: Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree, Stefan Wahren Add devicetree binding for the Vertexcom MSE102x Homeplug GreenPHY chip as SPI device. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- .../bindings/net/vertexcom-mse102x.yaml | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml diff --git a/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml new file mode 100644 index 000000000000..d1a4159a8449 --- /dev/null +++ b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/net/vertexcom-mse102x.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: The Vertexcom MSE102x (SPI) Device Tree Bindings + +maintainers: + - Stefan Wahren <stefan.wahren@in-tech.com> + +description: + Vertexcom's MSE102x are a family of HomePlug GreenPHY chips. + They can be connected either via RGMII, RMII or SPI to a host CPU. + + In order to use a MSE102x chip as SPI device, it must be defined as + a child of an SPI master device in the device tree. + + More information can be found at + http://www.vertexcom.com/doc/MSE1022%20Product%20Brief.pdf + +allOf: + - $ref: ethernet-controller.yaml# + +properties: + compatible: + enum: + - vertexcom,mse1021 + - vertexcom,mse1022 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + spi-cpha: true + + spi-cpol: true + + spi-max-frequency: + minimum: 6000000 + maximum: 7142857 + +required: + - compatible + - reg + - interrupts + - spi-cpha + - spi-cpol + - spi-max-frequency + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + spi0 { + #address-cells = <1>; + #size-cells = <0>; + + ethernet@0 { + compatible = "vertexcom,mse1021"; + reg = <0>; + interrupt-parent = <&gpio>; + interrupts = <23 IRQ_TYPE_EDGE_RISING>; + spi-cpha; + spi-cpol; + spi-max-frequency = <7142857>; + }; + }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support 2021-09-14 15:17 ` [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support Stefan Wahren @ 2021-09-21 21:09 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-09-21 21:09 UTC (permalink / raw) To: Stefan Wahren Cc: devicetree, Rob Herring, Michael Heimpold, Jakub Kicinski, linux-kernel, jimmy.shen, David S. Miller, netdev On Tue, 14 Sep 2021 17:17:16 +0200, Stefan Wahren wrote: > Add devicetree binding for the Vertexcom MSE102x Homeplug GreenPHY chip > as SPI device. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > .../bindings/net/vertexcom-mse102x.yaml | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-14 15:17 [PATCH RFC 0/3] add Vertexcom MSE102x support Stefan Wahren 2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren 2021-09-14 15:17 ` [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support Stefan Wahren @ 2021-09-14 15:17 ` Stefan Wahren 2021-09-15 2:55 ` Jakub Kicinski ` (2 more replies) 2 siblings, 3 replies; 14+ messages in thread From: Stefan Wahren @ 2021-09-14 15:17 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, Rob Herring Cc: Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree, Stefan Wahren This implements an SPI protocol driver for Vertexcom MSE102x Homeplug GreenPHY chip. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/net/ethernet/Kconfig | 1 + drivers/net/ethernet/Makefile | 1 + drivers/net/ethernet/vertexcom/Kconfig | 25 + drivers/net/ethernet/vertexcom/Makefile | 6 + drivers/net/ethernet/vertexcom/mse102x.c | 803 +++++++++++++++++++++++ 5 files changed, 836 insertions(+) create mode 100644 drivers/net/ethernet/vertexcom/Kconfig create mode 100644 drivers/net/ethernet/vertexcom/Makefile create mode 100644 drivers/net/ethernet/vertexcom/mse102x.c diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig index d796684ec9ca..373f3ec28b7a 100644 --- a/drivers/net/ethernet/Kconfig +++ b/drivers/net/ethernet/Kconfig @@ -180,6 +180,7 @@ source "drivers/net/ethernet/tehuti/Kconfig" source "drivers/net/ethernet/ti/Kconfig" source "drivers/net/ethernet/toshiba/Kconfig" source "drivers/net/ethernet/tundra/Kconfig" +source "drivers/net/ethernet/vertexcom/Kconfig" source "drivers/net/ethernet/via/Kconfig" source "drivers/net/ethernet/wiznet/Kconfig" source "drivers/net/ethernet/xilinx/Kconfig" diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile index aaa5078cd7d1..2ef412a88296 100644 --- a/drivers/net/ethernet/Makefile +++ b/drivers/net/ethernet/Makefile @@ -91,6 +91,7 @@ obj-$(CONFIG_NET_VENDOR_TEHUTI) += tehuti/ obj-$(CONFIG_NET_VENDOR_TI) += ti/ obj-$(CONFIG_NET_VENDOR_TOSHIBA) += toshiba/ obj-$(CONFIG_NET_VENDOR_TUNDRA) += tundra/ +obj-$(CONFIG_NET_VENDOR_VERTEXCOM) += vertexcom/ obj-$(CONFIG_NET_VENDOR_VIA) += via/ obj-$(CONFIG_NET_VENDOR_WIZNET) += wiznet/ obj-$(CONFIG_NET_VENDOR_XILINX) += xilinx/ diff --git a/drivers/net/ethernet/vertexcom/Kconfig b/drivers/net/ethernet/vertexcom/Kconfig new file mode 100644 index 000000000000..4184a635fe01 --- /dev/null +++ b/drivers/net/ethernet/vertexcom/Kconfig @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Vertexcom network device configuration +# + +config NET_VENDOR_VERTEXCOM + bool "Vertexcom devices" + default y + help + If you have a network (Ethernet) card belonging to this class, say Y. + + Note that the answer to this question doesn't directly affect the + kernel: saying N will just cause the configurator to skip all + the questions about Vertexcom cards. If you say Y, you will be asked + for your specific card in the following questions. + +if NET_VENDOR_VERTEXCOM + +config MSE102X + tristate "Vertexcom MSE102x SPI" + depends on SPI + help + SPI driver for Vertexcom MSE102x SPI attached network chip. + +endif # NET_VENDOR_VERTEXCOM diff --git a/drivers/net/ethernet/vertexcom/Makefile b/drivers/net/ethernet/vertexcom/Makefile new file mode 100644 index 000000000000..f8b12e312637 --- /dev/null +++ b/drivers/net/ethernet/vertexcom/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for the Vertexcom network device drivers. +# + +obj-$(CONFIG_MSE102X) += mse102x.o diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c new file mode 100644 index 000000000000..a580ccac3782 --- /dev/null +++ b/drivers/net/ethernet/vertexcom/mse102x.c @@ -0,0 +1,803 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (C) 2021 in-tech smart charging GmbH + * + * driver is based on micrel/ks8851_spi.c + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/netdevice.h> +#include <linux/etherdevice.h> +#include <linux/ethtool.h> +#include <linux/cache.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> + +#include <linux/spi/spi.h> +#include <linux/of_net.h> + +#define MSG_DEFAULT (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK | \ + NETIF_MSG_TIMER) + +#define DRV_NAME "mse102x" + +#define DET_CMD 0x0001 +#define DET_SOF 0x0002 +#define DET_DFT 0x55AA + +#define CMD_SHIFT 12 +#define CMD_RTS (0x1 << CMD_SHIFT) +#define CMD_CTR (0x2 << CMD_SHIFT) + +#define CMD_MASK GENMASK(15, CMD_SHIFT) +#define LEN_MASK GENMASK(CMD_SHIFT - 1, 0) + +#define DET_CMD_LEN 4 +#define DET_SOF_LEN 2 +#define DET_DFT_LEN 2 + +#define MIN_FREQ_HZ 6000000 +#define MAX_FREQ_HZ 7142857 + +struct mse102x_stats { + u64 xfer_err; + u64 invalid_cmd; + u64 invalid_ctr; + u64 invalid_dft; + u64 invalid_len; + u64 invalid_rts; + u64 invalid_sof; + u64 tx_timeout; +}; + +static const char mse102x_gstrings_stats[][ETH_GSTRING_LEN] = { + "SPI transfer errors", + "Invalid command", + "Invalid CTR", + "Invalid DFT", + "Invalid frame length", + "Invalid RTS", + "Invalid SOF", + "TX timeout", +}; + +struct mse102x_net { + struct net_device *ndev; + + u8 rxd[8]; + u8 txd[8]; + + u32 msg_enable ____cacheline_aligned; + + struct sk_buff_head txq; + struct mse102x_stats stats; +}; + +struct mse102x_net_spi { + struct mse102x_net mse102x; + struct mutex lock; /* Protect SPI frame transfer */ + struct work_struct tx_work; + struct spi_device *spidev; + struct spi_message spi_msg; + struct spi_transfer spi_xfer; + +#ifdef CONFIG_DEBUG_FS + struct dentry *device_root; +#endif +}; + +#define to_mse102x_spi(mse) container_of((mse), struct mse102x_net_spi, mse102x) + +static int msg_enable; +module_param_named(message, msg_enable, int, 0); +MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)"); + +#ifdef CONFIG_DEBUG_FS + +static int mse102x_info_show(struct seq_file *s, void *what) +{ + struct mse102x_net_spi *mses = s->private; + + seq_printf(s, "TX ring size : %u\n", + skb_queue_len(&mses->mse102x.txq)); + + seq_printf(s, "IRQ : %d\n", + mses->spidev->irq); + + seq_printf(s, "SPI effective speed : %lu\n", + (unsigned long)mses->spi_xfer.effective_speed_hz); + seq_printf(s, "SPI mode : %x\n", + mses->spidev->mode); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(mse102x_info); + +void mse102x_init_device_debugfs(struct mse102x_net_spi *mses) +{ + mses->device_root = debugfs_create_dir(dev_name(&mses->mse102x.ndev->dev), + NULL); + + debugfs_create_file("info", S_IFREG | 0444, mses->device_root, mses, + &mse102x_info_fops); +} + +void mse102x_remove_device_debugfs(struct mse102x_net_spi *mses) +{ + debugfs_remove_recursive(mses->device_root); +} + +#else /* CONFIG_DEBUG_FS */ + +void mse102x_init_device_debugfs(struct mse102x_net_spi *mses) +{ +} + +void mse102x_remove_device_debugfs(struct mse102x_net_spi *mses) +{ +} + +#endif + +/* SPI register read/write calls. + * + * All these calls issue SPI transactions to access the chip's registers. They + * all require that the necessary lock is held to prevent accesses when the + * chip is busy transferring packet data. + */ + +static void mse102x_tx_cmd_spi(struct mse102x_net *mse, u16 cmd) +{ + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + struct spi_transfer *xfer = &mses->spi_xfer; + struct spi_message *msg = &mses->spi_msg; + __be16 txb[2]; + int ret; + + txb[0] = cpu_to_be16(DET_CMD); + txb[1] = cpu_to_be16(cmd); + + xfer->tx_buf = txb; + xfer->rx_buf = NULL; + xfer->len = DET_CMD_LEN; + + ret = spi_sync(mses->spidev, msg); + if (ret < 0) { + netdev_err(mse->ndev, "%s: spi_sync() failed: %d\n", + __func__, ret); + mse->stats.xfer_err++; + } +} + +static int mse102x_rx_cmd_spi(struct mse102x_net *mse, u8 *rxb) +{ + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + struct spi_transfer *xfer = &mses->spi_xfer; + struct spi_message *msg = &mses->spi_msg; + __be16 *txb = (__be16 *)mse->txd; + __be16 *cmd = (__be16 *)mse->rxd; + u8 *trx = mse->rxd; + int ret; + + txb[0] = 0; + txb[1] = 0; + + xfer->tx_buf = txb; + xfer->rx_buf = trx; + xfer->len = DET_CMD_LEN; + + ret = spi_sync(mses->spidev, msg); + if (ret < 0) { + netdev_err(mse->ndev, "%s: spi_sync() failed: %d\n", + __func__, ret); + mse->stats.xfer_err++; + } else if (*cmd != cpu_to_be16(DET_CMD)) { + net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n", + __func__, *cmd); + mse->stats.invalid_cmd++; + ret = -EIO; + } else { + memcpy(rxb, trx + 2, 2); + } + + return ret; +} + +static inline void mse102x_push_header(struct sk_buff *skb) +{ + __be16 *header = skb_push(skb, DET_SOF_LEN); + + *header = cpu_to_be16(DET_SOF); +} + +static inline void mse102x_put_footer(struct sk_buff *skb) +{ + __be16 *footer = skb_put(skb, DET_DFT_LEN); + + *footer = cpu_to_be16(DET_DFT); +} + +static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp, + unsigned int pad) +{ + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + struct spi_transfer *xfer = &mses->spi_xfer; + struct spi_message *msg = &mses->spi_msg; + struct sk_buff *tskb; + int ret; + + netif_dbg(mse, tx_queued, mse->ndev, "%s: skb %p, %d@%p\n", + __func__, txp, txp->len, txp->data); + + if ((skb_headroom(txp) < DET_SOF_LEN) || + (skb_tailroom(txp) < DET_DFT_LEN + pad)) { + tskb = skb_copy_expand(txp, DET_SOF_LEN, DET_DFT_LEN + pad, + GFP_KERNEL); + if (!tskb) + return -ENOMEM; + + dev_kfree_skb(txp); + txp = tskb; + } + + mse102x_push_header(txp); + + if (pad) + skb_put_zero(txp, pad); + + mse102x_put_footer(txp); + + xfer->tx_buf = txp->data; + xfer->rx_buf = NULL; + xfer->len = txp->len; + + ret = spi_sync(mses->spidev, msg); + if (ret < 0) { + netdev_err(mse->ndev, "%s: spi_sync() failed: %d\n", + __func__, ret); + mse->stats.xfer_err++; + } + + return ret; +} + +static int mse102x_rx_frame_spi(struct mse102x_net *mse, u8 *buff, + unsigned int frame_len) +{ + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + struct spi_transfer *xfer = &mses->spi_xfer; + struct spi_message *msg = &mses->spi_msg; + __be16 *sof = (__be16 *)buff; + __be16 *dft = (__be16 *)(buff + DET_SOF_LEN + frame_len); + int ret; + + xfer->rx_buf = buff; + xfer->tx_buf = NULL; + xfer->len = DET_SOF_LEN + frame_len + DET_DFT_LEN; + + ret = spi_sync(mses->spidev, msg); + if (ret < 0) { + netdev_err(mse->ndev, "%s: spi_sync() failed: %d\n", + __func__, ret); + mse->stats.xfer_err++; + } else if (*sof != cpu_to_be16(DET_SOF)) { + netdev_dbg(mse->ndev, "%s: SPI start of frame is invalid (0x%04x)\n", + __func__, *sof); + mse->stats.invalid_sof++; + ret = -EIO; + } else if (*dft != cpu_to_be16(DET_DFT)) { + netdev_dbg(mse->ndev, "%s: SPI frame tail is invalid (0x%04x)\n", + __func__, *dft); + mse->stats.invalid_dft++; + ret = -EIO; + } + + return ret; +} + +static void mse102x_dump_packet(const char *msg, int len, const char *data) +{ + printk(KERN_DEBUG ": %s - packet len:%d\n", msg, len); + print_hex_dump(KERN_DEBUG, "pk data: ", DUMP_PREFIX_OFFSET, 16, 1, + data, len, true); +} + +static void mse102x_rx_pkt_spi(struct mse102x_net *mse) +{ + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + struct sk_buff *skb; + unsigned int rxalign; + unsigned int rxlen; + __be16 rx = 0; + u16 cmd_resp; + u8 *rxpkt; + int ret; + + mutex_lock(&mses->lock); + + mse102x_tx_cmd_spi(mse, CMD_CTR); + ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx); + cmd_resp = be16_to_cpu(rx); + + if (ret || ((cmd_resp & CMD_MASK) != CMD_RTS)) { + usleep_range(50, 100); + + mse102x_tx_cmd_spi(mse, CMD_CTR); + ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx); + cmd_resp = be16_to_cpu(rx); + if (ret) { + goto unlock_spi; + } else if ((cmd_resp & CMD_MASK) != CMD_RTS) { + net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n", + __func__, cmd_resp); + mse->stats.invalid_rts++; + goto unlock_spi; + } else { + net_dbg_ratelimited("%s: Unexpected response to first CMD\n", + __func__); + } + } + + rxlen = cmd_resp & LEN_MASK; + if (!rxlen) { + net_dbg_ratelimited("%s: No frame length defined\n", __func__); + mse->stats.invalid_len++; + goto unlock_spi; + } + + rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4); + skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign); + if (!skb) + goto unlock_spi; + + /* 2 bytes Start of frame (before ethernet header) + * 2 bytes Data frame tail (after ethernet frame) + * They are copied, but ignored. + */ + rxpkt = skb_put(skb, rxlen) - DET_SOF_LEN; + if (mse102x_rx_frame_spi(mse, rxpkt, rxlen)) { + mse->ndev->stats.rx_errors++; + dev_kfree_skb(skb); + goto unlock_spi; + } + + if (netif_msg_pktdata(mse)) + mse102x_dump_packet(__func__, skb->len, skb->data); + + skb->protocol = eth_type_trans(skb, mse->ndev); + netif_rx_ni(skb); + + mse->ndev->stats.rx_packets++; + mse->ndev->stats.rx_bytes += rxlen; + +unlock_spi: + mutex_unlock(&mses->lock); +} + +static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff *txb, + unsigned long work_timeout) +{ + unsigned int pad = 0; + __be16 rx = 0; + u16 cmd_resp; + int ret; + bool first = true; + + if (txb->len < 60) + pad = 60 - txb->len; + + while (1) { + /* It's not predictable how long / many retries it takes to + * send at least one packet, so TX timeouts are possible. + * That's the reason why the netdev watchdog is not used here. + */ + if (time_after(jiffies, work_timeout)) + return -ETIMEDOUT; + + mse102x_tx_cmd_spi(mse, CMD_RTS | (txb->len + pad)); + ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx); + cmd_resp = be16_to_cpu(rx); + + if (!ret) { + /* ready to send frame ? */ + if (cmd_resp == CMD_CTR) + break; + + net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n", + __func__, cmd_resp); + mse->stats.invalid_ctr++; + } + + if (first) { + /* throttle at first issue */ + netif_stop_queue(mse->ndev); + /* fast retry */ + usleep_range(50, 100); + first = false; + } else { + msleep(20); + } + }; + + ret = mse102x_tx_frame_spi(mse, txb, pad); + if (ret) { + net_dbg_ratelimited("%s: Failed to send (%d), drop frame\n", + __func__, ret); + } + + return ret; +} + +#define TX_QUEUE_MAX 10 + +static void mse102x_tx_work(struct work_struct *work) +{ + /* Make sure timeout is sufficient to transfer TX_QUEUE_MAX frames */ + unsigned long work_timeout = jiffies + msecs_to_jiffies(1000); + struct mse102x_net_spi *mses; + struct mse102x_net *mse; + struct sk_buff *txb; + bool done = false; + int ret = 0; + + mses = container_of(work, struct mse102x_net_spi, tx_work); + mse = &mses->mse102x; + + while (!done) { + mutex_lock(&mses->lock); + + txb = skb_dequeue(&mse->txq); + if (!txb) { + done = true; + goto unlock_spi; + } + + ret = mse102x_tx_pkt_spi(mse, txb, work_timeout); + if (ret) { + mse->ndev->stats.tx_dropped++; + } else { + mse->ndev->stats.tx_bytes += txb->len; + mse->ndev->stats.tx_packets++; + } + + dev_kfree_skb(txb); + +unlock_spi: + mutex_unlock(&mses->lock); + } + + if (ret == -ETIMEDOUT) { + if (netif_msg_timer(mse)) + netdev_err(mse->ndev, "tx work timeout\n"); + + mse->stats.tx_timeout++; + } + + netif_wake_queue(mse->ndev); +} + +static netdev_tx_t mse102x_start_xmit_spi(struct sk_buff *skb, + struct net_device *ndev) +{ + struct mse102x_net *mse = netdev_priv(ndev); + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + netdev_tx_t ret = NETDEV_TX_OK; + + netif_dbg(mse, tx_queued, ndev, + "%s: skb %p, %d@%p\n", __func__, skb, skb->len, skb->data); + + if (skb_queue_len(&mse->txq) >= TX_QUEUE_MAX) { + netif_stop_queue(ndev); + ret = NETDEV_TX_BUSY; + } else { + skb_queue_tail(&mse->txq, skb); + } + + schedule_work(&mses->tx_work); + + return ret; +} + +static void mse102x_init_mac(struct mse102x_net *mse, struct device_node *np) +{ + struct net_device *ndev = mse->ndev; + int ret = of_get_mac_address(np, ndev->dev_addr); + + if (ret) { + eth_hw_addr_random(ndev); + netdev_err(ndev, "Using random MAC address: %pM\n", + ndev->dev_addr); + } +} + +/* Assumption: this is called for every incoming packet */ +static irqreturn_t mse102x_irq(int irq, void *_mse) +{ + struct mse102x_net *mse = _mse; + + mse102x_rx_pkt_spi(mse); + + return IRQ_HANDLED; +} + +static int mse102x_net_open(struct net_device *ndev) +{ + struct mse102x_net *mse = netdev_priv(ndev); + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + int ret; + + ret = request_threaded_irq(ndev->irq, NULL, mse102x_irq, IRQF_ONESHOT, + ndev->name, mse); + if (ret < 0) { + netdev_err(ndev, "Failed to get irq: %d\n", ret); + return ret; + } + + /* lock the card, even if we may not actually be doing anything + * else at the moment + */ + mutex_lock(&mses->lock); + + netif_dbg(mse, ifup, ndev, "opening\n"); + + netif_start_queue(ndev); + + netif_carrier_on(ndev); + + netif_dbg(mse, ifup, ndev, "network device up\n"); + + mutex_unlock(&mses->lock); + + return 0; +} + +static int mse102x_net_stop(struct net_device *ndev) +{ + struct mse102x_net *mse = netdev_priv(ndev); + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + + netif_info(mse, ifdown, ndev, "shutting down\n"); + + netif_stop_queue(ndev); + + /* stop any outstanding work */ + flush_work(&mses->tx_work); + + /* ensure any queued tx buffers are dumped */ + while (!skb_queue_empty(&mse->txq)) { + struct sk_buff *txb = skb_dequeue(&mse->txq); + + netif_dbg(mse, ifdown, ndev, + "%s: freeing txb %p\n", __func__, txb); + + dev_kfree_skb(txb); + } + + free_irq(ndev->irq, mse); + + return 0; +} + +static const struct net_device_ops mse102x_netdev_ops = { + .ndo_open = mse102x_net_open, + .ndo_stop = mse102x_net_stop, + .ndo_start_xmit = mse102x_start_xmit_spi, + .ndo_set_mac_address = eth_mac_addr, + .ndo_validate_addr = eth_validate_addr, +}; + +/* ethtool support */ + +static void mse102x_get_drvinfo(struct net_device *ndev, + struct ethtool_drvinfo *di) +{ + strscpy(di->driver, DRV_NAME, sizeof(di->driver)); + strscpy(di->version, "1.00", sizeof(di->version)); + strscpy(di->bus_info, dev_name(ndev->dev.parent), sizeof(di->bus_info)); +} + +static u32 mse102x_get_msglevel(struct net_device *ndev) +{ + struct mse102x_net *mse = netdev_priv(ndev); + + return mse->msg_enable; +} + +static void mse102x_set_msglevel(struct net_device *ndev, u32 to) +{ + struct mse102x_net *mse = netdev_priv(ndev); + + mse->msg_enable = to; +} + +static void mse102x_get_ethtool_stats(struct net_device *ndev, + struct ethtool_stats *estats, u64 *data) +{ + struct mse102x_net *mse = netdev_priv(ndev); + struct mse102x_stats *st = &mse->stats; + + memcpy(data, st, ARRAY_SIZE(mse102x_gstrings_stats) * sizeof(u64)); +} + +static void mse102x_get_strings(struct net_device *ndev, u32 stringset, u8 *buf) +{ + switch (stringset) { + case ETH_SS_STATS: + memcpy(buf, &mse102x_gstrings_stats, + sizeof(mse102x_gstrings_stats)); + break; + default: + WARN_ON(1); + break; + } +} + +static int mse102x_get_sset_count(struct net_device *ndev, int sset) +{ + switch (sset) { + case ETH_SS_STATS: + return ARRAY_SIZE(mse102x_gstrings_stats); + default: + return -EINVAL; + } +} + +static const struct ethtool_ops mse102x_ethtool_ops = { + .get_drvinfo = mse102x_get_drvinfo, + .get_link = ethtool_op_get_link, + .get_msglevel = mse102x_get_msglevel, + .set_msglevel = mse102x_set_msglevel, + .get_ethtool_stats = mse102x_get_ethtool_stats, + .get_strings = mse102x_get_strings, + .get_sset_count = mse102x_get_sset_count, +}; + +/* driver bus management functions */ + +#ifdef CONFIG_PM_SLEEP + +static int mse102x_suspend(struct device *dev) +{ + struct mse102x_net *mse = dev_get_drvdata(dev); + struct net_device *ndev = mse->ndev; + + if (netif_running(ndev)) { + netif_device_detach(ndev); + mse102x_net_stop(ndev); + } + + return 0; +} + +static int mse102x_resume(struct device *dev) +{ + struct mse102x_net *mse = dev_get_drvdata(dev); + struct net_device *ndev = mse->ndev; + + if (netif_running(ndev)) { + mse102x_net_open(ndev); + netif_device_attach(ndev); + } + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(mse102x_pm_ops, mse102x_suspend, mse102x_resume); + +static int mse102x_probe_spi(struct spi_device *spi) +{ + struct device *dev = &spi->dev; + struct mse102x_net_spi *mses; + struct net_device *ndev; + struct mse102x_net *mse; + int ret; + + spi->bits_per_word = 8; + spi->mode |= SPI_MODE_3; + /* enforce minimum speed to ensure device functionality */ + spi->master->min_speed_hz = MIN_FREQ_HZ; + + if (!spi->max_speed_hz) + spi->max_speed_hz = MAX_FREQ_HZ; + + if (spi->max_speed_hz < MIN_FREQ_HZ || + spi->max_speed_hz > MAX_FREQ_HZ) { + dev_err(&spi->dev, "SPI max frequency out of range (min: %u, max: %u)\n", + MIN_FREQ_HZ, MAX_FREQ_HZ); + return -EINVAL; + } + + ret = spi_setup(spi); + if (ret < 0) { + dev_err(&spi->dev, "Unable to setup SPI device: %d\n", ret); + return ret; + } + + ndev = devm_alloc_etherdev(dev, sizeof(struct mse102x_net_spi)); + if (!ndev) + return -ENOMEM; + + ndev->priv_flags &= ~IFF_TX_SKB_SHARING; + ndev->tx_queue_len = 100; + + mse = netdev_priv(ndev); + mses = to_mse102x_spi(mse); + + mses->spidev = spi; + mutex_init(&mses->lock); + INIT_WORK(&mses->tx_work, mse102x_tx_work); + + /* initialise pre-made spi transfer messages */ + spi_message_init(&mses->spi_msg); + spi_message_add_tail(&mses->spi_xfer, &mses->spi_msg); + + ndev->irq = spi->irq; + mse->ndev = ndev; + + /* set the default message enable */ + mse->msg_enable = netif_msg_init(msg_enable, MSG_DEFAULT); + + skb_queue_head_init(&mse->txq); + + SET_NETDEV_DEV(ndev, dev); + + dev_set_drvdata(dev, mse); + + netif_carrier_off(mse->ndev); + ndev->if_port = IF_PORT_10BASET; + ndev->netdev_ops = &mse102x_netdev_ops; + ndev->ethtool_ops = &mse102x_ethtool_ops; + + mse102x_init_mac(mse, dev->of_node); + + ret = register_netdev(ndev); + if (ret) { + dev_err(dev, "failed to register network device: %d\n", ret); + return ret; + } + + mse102x_init_device_debugfs(mses); + + return 0; +} + +static int mse102x_remove_spi(struct spi_device *spi) +{ + struct mse102x_net *mse = dev_get_drvdata(&spi->dev); + struct mse102x_net_spi *mses = to_mse102x_spi(mse); + + if (netif_msg_drv(mse)) + dev_info(&spi->dev, "remove\n"); + + mse102x_remove_device_debugfs(mses); + unregister_netdev(mse->ndev); + + return 0; +} + +static const struct of_device_id mse102x_match_table[] = { + { .compatible = "vertexcom,mse1021" }, + { .compatible = "vertexcom,mse1022" }, + { } +}; +MODULE_DEVICE_TABLE(of, mse102x_match_table); + +static struct spi_driver mse102x_driver = { + .driver = { + .name = DRV_NAME, + .of_match_table = mse102x_match_table, + .pm = &mse102x_pm_ops, + }, + .probe = mse102x_probe_spi, + .remove = mse102x_remove_spi, +}; +module_spi_driver(mse102x_driver); + +MODULE_DESCRIPTION("MSE102x Network driver"); +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@in-tech.com>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("spi:" DRV_NAME); -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren @ 2021-09-15 2:55 ` Jakub Kicinski 2021-09-15 21:17 ` Andrew Lunn 2021-09-16 4:57 ` kernel test robot 2 siblings, 0 replies; 14+ messages in thread From: Jakub Kicinski @ 2021-09-15 2:55 UTC (permalink / raw) To: Stefan Wahren Cc: David S. Miller, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree On Tue, 14 Sep 2021 17:17:17 +0200 Stefan Wahren wrote: > This implements an SPI protocol driver for Vertexcom MSE102x > Homeplug GreenPHY chip. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > + rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4); > + skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign); > + if (!skb) > + goto unlock_spi; > + > + /* 2 bytes Start of frame (before ethernet header) > + * 2 bytes Data frame tail (after ethernet frame) > + * They are copied, but ignored. > + */ > + rxpkt = skb_put(skb, rxlen) - DET_SOF_LEN; This assumes there is SOF_LEN headroom, but you never reserved that headroom, and SOF_LEN is added to the frame len.. one of those is not necessary? > + if (mse102x_rx_frame_spi(mse, rxpkt, rxlen)) { > + mse->ndev->stats.rx_errors++; > + dev_kfree_skb(skb); > + goto unlock_spi; > + } > + > + if (netif_msg_pktdata(mse)) > + mse102x_dump_packet(__func__, skb->len, skb->data); > + > + skb->protocol = eth_type_trans(skb, mse->ndev); > + netif_rx_ni(skb); > + > + mse->ndev->stats.rx_packets++; > + mse->ndev->stats.rx_bytes += rxlen; > + > +unlock_spi: > + mutex_unlock(&mses->lock); > +} > + > +static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff *txb, > + unsigned long work_timeout) > +{ > + unsigned int pad = 0; > + __be16 rx = 0; > + u16 cmd_resp; > + int ret; > + bool first = true; > + > + if (txb->len < 60) > + pad = 60 - txb->len; > + > + while (1) { > + /* It's not predictable how long / many retries it takes to > + * send at least one packet, so TX timeouts are possible. > + * That's the reason why the netdev watchdog is not used here. > + */ > + if (time_after(jiffies, work_timeout)) > + return -ETIMEDOUT; > + > + mse102x_tx_cmd_spi(mse, CMD_RTS | (txb->len + pad)); > + ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx); > + cmd_resp = be16_to_cpu(rx); > + > + if (!ret) { > + /* ready to send frame ? */ > + if (cmd_resp == CMD_CTR) > + break; > + > + net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n", > + __func__, cmd_resp); > + mse->stats.invalid_ctr++; > + } > + > + if (first) { > + /* throttle at first issue */ > + netif_stop_queue(mse->ndev); > + /* fast retry */ > + usleep_range(50, 100); > + first = false; > + } else { > + msleep(20); > + } > + }; > + > + ret = mse102x_tx_frame_spi(mse, txb, pad); > + if (ret) { > + net_dbg_ratelimited("%s: Failed to send (%d), drop frame\n", > + __func__, ret); > + } No need for brackets. > + return ret; > +} > + > +#define TX_QUEUE_MAX 10 > + > +static void mse102x_tx_work(struct work_struct *work) > +{ > + /* Make sure timeout is sufficient to transfer TX_QUEUE_MAX frames */ > + unsigned long work_timeout = jiffies + msecs_to_jiffies(1000); Sure this is safe? what if the system is under heavy load and the worker thread just gets scheduled out for the best part of the second? > + struct mse102x_net_spi *mses; > + struct mse102x_net *mse; > + struct sk_buff *txb; > + bool done = false; > + int ret = 0; > + > + mses = container_of(work, struct mse102x_net_spi, tx_work); > + mse = &mses->mse102x; > + > + while (!done) { > + mutex_lock(&mses->lock); I think you can take the lock just around the mse102x_tx_pkt_spi(). > + txb = skb_dequeue(&mse->txq); > + if (!txb) { > + done = true; > + goto unlock_spi; > + } > + > + ret = mse102x_tx_pkt_spi(mse, txb, work_timeout); > + if (ret) { > + mse->ndev->stats.tx_dropped++; > + } else { > + mse->ndev->stats.tx_bytes += txb->len; > + mse->ndev->stats.tx_packets++; > + } > + > + dev_kfree_skb(txb); > + > +unlock_spi: > + mutex_unlock(&mses->lock); > + } > + > + if (ret == -ETIMEDOUT) { > + if (netif_msg_timer(mse)) > + netdev_err(mse->ndev, "tx work timeout\n"); > + > + mse->stats.tx_timeout++; > + } > + > + netif_wake_queue(mse->ndev); > +} > + > +static netdev_tx_t mse102x_start_xmit_spi(struct sk_buff *skb, > + struct net_device *ndev) > +{ > + struct mse102x_net *mse = netdev_priv(ndev); > + struct mse102x_net_spi *mses = to_mse102x_spi(mse); > + netdev_tx_t ret = NETDEV_TX_OK; > + > + netif_dbg(mse, tx_queued, ndev, > + "%s: skb %p, %d@%p\n", __func__, skb, skb->len, skb->data); > + > + if (skb_queue_len(&mse->txq) >= TX_QUEUE_MAX) { > + netif_stop_queue(ndev); > + ret = NETDEV_TX_BUSY; It's best practice to stop the queue in advance if you know you won't be able to send the next packet, rather than return BUSY and force the qdisc to requeue the frame. > + } else { > + skb_queue_tail(&mse->txq, skb); > + } > + > + schedule_work(&mses->tx_work); > + > + return ret; > +} > +static int mse102x_net_open(struct net_device *ndev) > +{ > + struct mse102x_net *mse = netdev_priv(ndev); > + struct mse102x_net_spi *mses = to_mse102x_spi(mse); > + int ret; > + > + ret = request_threaded_irq(ndev->irq, NULL, mse102x_irq, IRQF_ONESHOT, > + ndev->name, mse); > + if (ret < 0) { > + netdev_err(ndev, "Failed to get irq: %d\n", ret); > + return ret; > + } > + > + /* lock the card, even if we may not actually be doing anything > + * else at the moment > + */ > + mutex_lock(&mses->lock); What is this lock protecting? > + netif_dbg(mse, ifup, ndev, "opening\n"); > + > + netif_start_queue(ndev); > + > + netif_carrier_on(ndev); > + > + netif_dbg(mse, ifup, ndev, "network device up\n"); > + > + mutex_unlock(&mses->lock); > + > + return 0; > +} > + > +static int mse102x_net_stop(struct net_device *ndev) > +{ > + struct mse102x_net *mse = netdev_priv(ndev); > + struct mse102x_net_spi *mses = to_mse102x_spi(mse); > + > + netif_info(mse, ifdown, ndev, "shutting down\n"); > + > + netif_stop_queue(ndev); > + > + /* stop any outstanding work */ > + flush_work(&mses->tx_work); The work can restart the queue. > + /* ensure any queued tx buffers are dumped */ > + while (!skb_queue_empty(&mse->txq)) { > + struct sk_buff *txb = skb_dequeue(&mse->txq); > + > + netif_dbg(mse, ifdown, ndev, > + "%s: freeing txb %p\n", __func__, txb); > + > + dev_kfree_skb(txb); > + } skb_queue_purge(), maybe? > + free_irq(ndev->irq, mse); > + > + return 0; > +} > +static void mse102x_get_drvinfo(struct net_device *ndev, > + struct ethtool_drvinfo *di) > +{ > + strscpy(di->driver, DRV_NAME, sizeof(di->driver)); > + strscpy(di->version, "1.00", sizeof(di->version)); Please drop the driver version, we depend on the kernel version these days (and that's provided by ethtool core by default). > + strscpy(di->bus_info, dev_name(ndev->dev.parent), sizeof(di->bus_info)); > +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren 2021-09-15 2:55 ` Jakub Kicinski @ 2021-09-15 21:17 ` Andrew Lunn 2021-09-16 11:12 ` Stefan Wahren 2021-09-16 11:26 ` Michael Heimpold 2021-09-16 4:57 ` kernel test robot 2 siblings, 2 replies; 14+ messages in thread From: Andrew Lunn @ 2021-09-15 21:17 UTC (permalink / raw) To: Stefan Wahren Cc: David S. Miller, Jakub Kicinski, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/netdevice.h> > +#include <linux/etherdevice.h> > +#include <linux/ethtool.h> > +#include <linux/cache.h> > +#include <linux/debugfs.h> > +#include <linux/seq_file.h> > + > +#include <linux/spi/spi.h> > +#include <linux/of_net.h> > + > +#define MSG_DEFAULT (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK | \ > + NETIF_MSG_TIMER) > + > +#define DRV_NAME "mse102x" > + > +#define DET_CMD 0x0001 > +#define DET_SOF 0x0002 > +#define DET_DFT 0x55AA > + > +#define CMD_SHIFT 12 > +#define CMD_RTS (0x1 << CMD_SHIFT) > +#define CMD_CTR (0x2 << CMD_SHIFT) > + > +#define CMD_MASK GENMASK(15, CMD_SHIFT) > +#define LEN_MASK GENMASK(CMD_SHIFT - 1, 0) > + > +#define DET_CMD_LEN 4 > +#define DET_SOF_LEN 2 > +#define DET_DFT_LEN 2 Looks like these tabs should be spaces? > +static int msg_enable; > +module_param_named(message, msg_enable, int, 0); > +MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)"); I know a lot of drivers do this, but module parameters are not liked. There is a well used ethtool setting for this, msglvl, which should be used instead. Which in fact, you have support for. > +static void mse102x_init_mac(struct mse102x_net *mse, struct device_node *np) > +{ > + struct net_device *ndev = mse->ndev; > + int ret = of_get_mac_address(np, ndev->dev_addr); > + > + if (ret) { > + eth_hw_addr_random(ndev); > + netdev_err(ndev, "Using random MAC address: %pM\n", > + ndev->dev_addr); > + } > +} No need to tell the hardware? Does it work in promiscuous mode by default? > +static int mse102x_net_stop(struct net_device *ndev) > +{ > + struct mse102x_net *mse = netdev_priv(ndev); > + struct mse102x_net_spi *mses = to_mse102x_spi(mse); > + > + netif_info(mse, ifdown, ndev, "shutting down\n"); > + > + netif_stop_queue(ndev); > + > + /* stop any outstanding work */ > + flush_work(&mses->tx_work); > + > + /* ensure any queued tx buffers are dumped */ > + while (!skb_queue_empty(&mse->txq)) { > + struct sk_buff *txb = skb_dequeue(&mse->txq); > + > + netif_dbg(mse, ifdown, ndev, > + "%s: freeing txb %p\n", __func__, txb); > + > + dev_kfree_skb(txb); > + } > + > + free_irq(ndev->irq, mse); > + > + return 0; Maybe a netif_carrier_off() in there, to be symmetric with open? > +/* ethtool support */ > + > +static void mse102x_get_drvinfo(struct net_device *ndev, > + struct ethtool_drvinfo *di) > +{ > + strscpy(di->driver, DRV_NAME, sizeof(di->driver)); > + strscpy(di->version, "1.00", sizeof(di->version)); > + strscpy(di->bus_info, dev_name(ndev->dev.parent), sizeof(di->bus_info)); > +} Version is pretty pointless. We suggest you don't use it. The ethtool core will then fill it with the kernel version, > +static int mse102x_probe_spi(struct spi_device *spi) > +{ ... > + netif_carrier_off(mse->ndev); > + ndev->if_port = IF_PORT_10BASET; That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? > + ndev->netdev_ops = &mse102x_netdev_ops; > + ndev->ethtool_ops = &mse102x_ethtool_ops; > + > + mse102x_init_mac(mse, dev->of_node); > + > + ret = register_netdev(ndev); > + if (ret) { > + dev_err(dev, "failed to register network device: %d\n", ret); > + return ret; > + } > + > + mse102x_init_device_debugfs(mses); > + > + return 0; > +} > +static const struct of_device_id mse102x_match_table[] = { > + { .compatible = "vertexcom,mse1021" }, > + { .compatible = "vertexcom,mse1022" }, Is there an ID register you can read to determine what device you actually have? If so, i suggest you verify the correct compatible is used. Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-15 21:17 ` Andrew Lunn @ 2021-09-16 11:12 ` Stefan Wahren 2021-09-16 12:35 ` Andrew Lunn 2021-09-16 11:26 ` Michael Heimpold 1 sibling, 1 reply; 14+ messages in thread From: Stefan Wahren @ 2021-09-16 11:12 UTC (permalink / raw) To: Andrew Lunn Cc: David S. Miller, Jakub Kicinski, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree Hi Andrew, thanks for your review. Am 15.09.21 um 23:17 schrieb Andrew Lunn: >> +static void mse102x_init_mac(struct mse102x_net *mse, struct device_node *np) >> +{ >> + struct net_device *ndev = mse->ndev; >> + int ret = of_get_mac_address(np, ndev->dev_addr); >> + >> + if (ret) { >> + eth_hw_addr_random(ndev); >> + netdev_err(ndev, "Using random MAC address: %pM\n", >> + ndev->dev_addr); >> + } >> +} > No need to tell the hardware? Does it work in promiscuous mode by > default? Yes and yes > >> + netif_carrier_off(mse->ndev); >> + ndev->if_port = IF_PORT_10BASET; > That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? There is already a driver (qca_spi, qcauart) for a similiar Homeplug device (QCA7000), which also uses IF_PORT_10BASET. Should i change this too or leave it because of resulting changes to userspace? > >> +static const struct of_device_id mse102x_match_table[] = { >> + { .compatible = "vertexcom,mse1021" }, >> + { .compatible = "vertexcom,mse1022" }, > Is there an ID register you can read to determine what device you > actually have? If so, i suggest you verify the correct compatible is > used. AFAIK the device doesn't have any kind of ID register. @Jimmy Please correct me, if i'm wrong. Best regards > > Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-16 11:12 ` Stefan Wahren @ 2021-09-16 12:35 ` Andrew Lunn 2021-09-21 21:07 ` Rob Herring 0 siblings, 1 reply; 14+ messages in thread From: Andrew Lunn @ 2021-09-16 12:35 UTC (permalink / raw) To: Stefan Wahren Cc: David S. Miller, Jakub Kicinski, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree > >> + netif_carrier_off(mse->ndev); > >> + ndev->if_port = IF_PORT_10BASET; > > That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? > There is already a driver (qca_spi, qcauart) for a similiar Homeplug > device (QCA7000), which also uses IF_PORT_10BASET. Should i change this > too or leave it because of resulting changes to userspace? Technically, it would be an ABI change. But ifmap seems pretty loosely defined. See man 7 netdevice: SIOCGIFMAP, SIOCSIFMAP Get or set the interface's hardware parameters using ifr_map. Setting the parameters is a privileged operation. struct ifmap { unsigned long mem_start; unsigned long mem_end; unsigned short base_addr; unsigned char irq; unsigned char dma; unsigned char port; }; The interpretation of the ifmap structure depends on the device driver and the architecture. The if_port value ends up in port. And i've no idea where it is actually available in user space. iproute2 does not use it, nor ethtool. So, i would say, submit a separate patch for the other drivers, and we will see if anybody notices. > >> +static const struct of_device_id mse102x_match_table[] = { > >> + { .compatible = "vertexcom,mse1021" }, > >> + { .compatible = "vertexcom,mse1022" }, > > Is there an ID register you can read to determine what device you > > actually have? If so, i suggest you verify the correct compatible is > > used. > > AFAIK the device doesn't have any kind of ID register. Then i would suggest changing the compatible to "vertexcom,mse102x". If you cannot verify it, and it makes no actual difference, then 50% of the boards will use the wrong one. Which means you can then later not actually make use of it to enable features specific to a compatible string. Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-16 12:35 ` Andrew Lunn @ 2021-09-21 21:07 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-09-21 21:07 UTC (permalink / raw) To: Andrew Lunn Cc: Stefan Wahren, David S. Miller, Jakub Kicinski, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree On Thu, Sep 16, 2021 at 02:35:41PM +0200, Andrew Lunn wrote: > > >> + netif_carrier_off(mse->ndev); > > >> + ndev->if_port = IF_PORT_10BASET; > > > That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? > > > > There is already a driver (qca_spi, qcauart) for a similiar Homeplug > > device (QCA7000), which also uses IF_PORT_10BASET. Should i change this > > too or leave it because of resulting changes to userspace? > > Technically, it would be an ABI change. But ifmap seems pretty loosely > defined. See man 7 netdevice: > > SIOCGIFMAP, SIOCSIFMAP > Get or set the interface's hardware parameters using ifr_map. > Setting the parameters is a privileged operation. > > struct ifmap { > unsigned long mem_start; > unsigned long mem_end; > unsigned short base_addr; > unsigned char irq; > unsigned char dma; > unsigned char port; > }; > > The interpretation of the ifmap structure depends on the device driver > and the architecture. > > The if_port value ends up in port. And i've no idea where it is > actually available in user space. iproute2 does not use it, nor > ethtool. So, i would say, submit a separate patch for the other > drivers, and we will see if anybody notices. > > > >> +static const struct of_device_id mse102x_match_table[] = { > > >> + { .compatible = "vertexcom,mse1021" }, > > >> + { .compatible = "vertexcom,mse1022" }, > > > Is there an ID register you can read to determine what device you > > > actually have? If so, i suggest you verify the correct compatible is > > > used. > > > > AFAIK the device doesn't have any kind of ID register. > > Then i would suggest changing the compatible to "vertexcom,mse102x". Don't use wildcards in compatible strings. > > If you cannot verify it, and it makes no actual difference, then 50% > of the boards will use the wrong one. Which means you can then later > not actually make use of it to enable features specific to a > compatible string. A wildcard doesn't help with this. If some boards are wrong, not any way we can fix that short of a DT update. If you can update the DT, then you can correct the string then. Rob ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-15 21:17 ` Andrew Lunn 2021-09-16 11:12 ` Stefan Wahren @ 2021-09-16 11:26 ` Michael Heimpold 2021-09-16 12:46 ` Andrew Lunn 1 sibling, 1 reply; 14+ messages in thread From: Michael Heimpold @ 2021-09-16 11:26 UTC (permalink / raw) To: Andrew Lunn Cc: Stefan Wahren, David S. Miller, Jakub Kicinski, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree [-- Attachment #1: Type: text/plain, Size: 493 bytes --] Hi Andrew, Zitat von Andrew Lunn <andrew@lunn.ch>: >> +static int mse102x_probe_spi(struct spi_device *spi) >> +{ > > ... > >> + netif_carrier_off(mse->ndev); >> + ndev->if_port = IF_PORT_10BASET; > > That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? Would a simple IF_PORT_HOMEPLUG be sufficient, or should it be more precise as for Ethernet (10BASET, 100BASET...), e.g. IF_PORT_HOMEPLUG_10 IF_PORT_HOMEPLUG_AV IF_PORT_HOMEPLUG_AV2 IF_PORT_HOMEPLUG_GREENPHY Thanks, Michael [-- Attachment #2: Öffentlicher PGP-Schlüssel --] [-- Type: application/pgp-keys, Size: 3153 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-16 11:26 ` Michael Heimpold @ 2021-09-16 12:46 ` Andrew Lunn 0 siblings, 0 replies; 14+ messages in thread From: Andrew Lunn @ 2021-09-16 12:46 UTC (permalink / raw) To: Michael Heimpold Cc: Stefan Wahren, David S. Miller, Jakub Kicinski, Rob Herring, Michael Heimpold, jimmy.shen, netdev, linux-kernel, devicetree On Thu, Sep 16, 2021 at 11:26:18AM +0000, Michael Heimpold wrote: > Hi Andrew, > > Zitat von Andrew Lunn <andrew@lunn.ch>: > > > > +static int mse102x_probe_spi(struct spi_device *spi) > > > +{ > > > > ... > > > > > + netif_carrier_off(mse->ndev); > > > + ndev->if_port = IF_PORT_10BASET; > > > > That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ? > > Would a simple IF_PORT_HOMEPLUG be sufficient, or should it be > more precise as for Ethernet (10BASET, 100BASET...), e.g. > IF_PORT_HOMEPLUG_10 > IF_PORT_HOMEPLUG_AV > IF_PORT_HOMEPLUG_AV2 > IF_PORT_HOMEPLUG_GREENPHY It is an interesting question. I think the first thing to find out is, what in userspace actually uses this. If it is a deprecated tool, i would not spend the energy. Probably a better interface is ethtool get_link_ksettings, and set_link_ksettings. $ /sbin/ethtool enp3s0 Settings for enp3s0: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full You can set supported ports to HomePlug, and supported link modes to 10, AV, AV2, GREENPHY etc. Is there a negotiation mechanism where different homeplug devices can find out what they have in common and select a mode? That would be very similar to Ethernet autoneg, so you can make use of the other fields ethtool provides to show this information, etc. Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren 2021-09-15 2:55 ` Jakub Kicinski 2021-09-15 21:17 ` Andrew Lunn @ 2021-09-16 4:57 ` kernel test robot 2 siblings, 0 replies; 14+ messages in thread From: kernel test robot @ 2021-09-16 4:57 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2541 bytes --] Hi Stefan, [FYI, it's a private test report for your RFC patch.] [auto build test ERROR on robh/for-next] [also build test ERROR on linus/master v5.15-rc1 next-20210915] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Stefan-Wahren/add-Vertexcom-MSE102x-support/20210914-232404 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: x86_64-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/09f1b772a438e7953db653d74e869c6981dd8d45 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Stefan-Wahren/add-Vertexcom-MSE102x-support/20210914-232404 git checkout 09f1b772a438e7953db653d74e869c6981dd8d45 # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/net/ethernet/vertexcom/mse102x.c:119:6: error: no previous prototype for 'mse102x_init_device_debugfs' [-Werror=missing-prototypes] 119 | void mse102x_init_device_debugfs(struct mse102x_net_spi *mses) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/net/ethernet/vertexcom/mse102x.c:128:6: error: no previous prototype for 'mse102x_remove_device_debugfs' [-Werror=missing-prototypes] 128 | void mse102x_remove_device_debugfs(struct mse102x_net_spi *mses) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors vim +/mse102x_init_device_debugfs +119 drivers/net/ethernet/vertexcom/mse102x.c 118 > 119 void mse102x_init_device_debugfs(struct mse102x_net_spi *mses) 120 { 121 mses->device_root = debugfs_create_dir(dev_name(&mses->mse102x.ndev->dev), 122 NULL); 123 124 debugfs_create_file("info", S_IFREG | 0444, mses->device_root, mses, 125 &mse102x_info_fops); 126 } 127 > 128 void mse102x_remove_device_debugfs(struct mse102x_net_spi *mses) 129 { 130 debugfs_remove_recursive(mses->device_root); 131 } 132 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 66522 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-09-21 21:09 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 15:17 [PATCH RFC 0/3] add Vertexcom MSE102x support Stefan Wahren 2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren 2021-09-21 20:55 ` Rob Herring 2021-09-14 15:17 ` [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support Stefan Wahren 2021-09-21 21:09 ` Rob Herring 2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren 2021-09-15 2:55 ` Jakub Kicinski 2021-09-15 21:17 ` Andrew Lunn 2021-09-16 11:12 ` Stefan Wahren 2021-09-16 12:35 ` Andrew Lunn 2021-09-21 21:07 ` Rob Herring 2021-09-16 11:26 ` Michael Heimpold 2021-09-16 12:46 ` Andrew Lunn 2021-09-16 4:57 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.