| Message ID | 20210914151717.12232-1-stefan.wahren@i2se.com |
|---|---|
| Headers | show |
| Series | add Vertexcom MSE102x support | expand |
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)); > +}
> +#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
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
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
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