| Message ID | 1500065326-26182-1-git-send-email-mdf@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 07/14/2017 01:48 PM, Moritz Fischer wrote: > Add support for the National Instruments XGE 1/10G network device. > > It uses the EEPROM on the board via NVMEM. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > + > +static void nixge_handle_link_change(struct net_device *ndev) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + struct phy_device *phydev = ndev->phydev; > + unsigned long flags; > + int status_change = 0; > + > + spin_lock_irqsave(&priv->lock, flags); The adjust_link function is called with the PHY device mutex held so the spinlock here looks completely unnecessary. > + > + if (phydev->link != priv->link || phydev->speed != priv->speed || > + phydev->duplex != priv->duplex) { > + priv->link = phydev->link; > + priv->speed = phydev->speed; > + priv->duplex = phydev->duplex; > + status_change = 1; > + } > + > + spin_unlock_irqrestore(&priv->lock, flags); > + > + if (status_change) > + phy_print_status(phydev); It's fine to print what changed, but surely the hardware should also react to link changes, like change of duplex, speed, pause etc. > +} > + > +static void nixge_start_xmit_done(struct net_device *ndev) > +{ This should be done in a NAPI context (soft IRQ) as well, except that for TX you don't need to bind the reclaiming process against the NAPI budget. > + u32 size = 0; > + u32 packets = 0; > + struct nixge_priv *priv = netdev_priv(ndev); > + struct nixge_dma_bd *cur_p; > + unsigned int status = 0; > + > + cur_p = &priv->tx_bd_v[priv->tx_bd_ci]; > + status = cur_p->status; > + > + while (status & XAXIDMA_BD_STS_COMPLETE_MASK) { > + dma_unmap_single(ndev->dev.parent, cur_p->phys, > + (cur_p->cntrl & XAXIDMA_BD_CTRL_LENGTH_MASK), > + DMA_TO_DEVICE); Fragments are unmapped with dma_unmap_page(), how are you unmapping them at the moment? > + if (cur_p->app4) > + dev_kfree_skb_irq((struct sk_buff *)cur_p->app4); > + /*cur_p->phys = 0;*/ > + cur_p->app0 = 0; > + cur_p->app1 = 0; > + cur_p->app2 = 0; > + cur_p->app4 = 0; > + cur_p->status = 0; Is this really necessary? Your descriptor is in coherent memory which means that you are doing slow uncached/writethrough accesses to the memory that holds them. Can't you just set status to 0 for the HW to ignore this descriptor? > + > + size += status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK; > + packets++; > + > + ++priv->tx_bd_ci; > + priv->tx_bd_ci %= TX_BD_NUM; > + cur_p = &priv->tx_bd_v[priv->tx_bd_ci]; > + status = cur_p->status; > + } > + > + ndev->stats.tx_packets += packets; > + ndev->stats.tx_bytes += size; > + netif_wake_queue(ndev); You can only wake the queue if you were successful transmitting packets. > +} > + > +static inline int nixge_check_tx_bd_space(struct nixge_priv *priv, > + int num_frag) > +{ > + struct nixge_dma_bd *cur_p; > + > + cur_p = &priv->tx_bd_v[(priv->tx_bd_tail + num_frag) % TX_BD_NUM]; > + if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK) > + return NETDEV_TX_BUSY; You are not propagating this to the caller, so just return a boolean for this. > + return 0; > +} > + > +static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + u32 ii; > + u32 num_frag; > + skb_frag_t *frag; > + dma_addr_t tail_p; > + struct nixge_priv *priv = netdev_priv(ndev); > + struct nixge_dma_bd *cur_p; > + > + num_frag = skb_shinfo(skb)->nr_frags; > + cur_p = &priv->tx_bd_v[priv->tx_bd_tail]; > + > + if (nixge_check_tx_bd_space(priv, num_frag)) { > + if (!netif_queue_stopped(ndev)) > + netif_stop_queue(ndev); > + return NETDEV_TX_BUSY; NETDEV_TX_OK is what you should return since you properly asserted flow contro with netif_stop_queue(). > + } > + > + cur_p->cntrl = skb_headlen(skb) | XAXIDMA_BD_CTRL_TXSOF_MASK; > + cur_p->phys = dma_map_single(ndev->dev.parent, skb->data, > + skb_headlen(skb), DMA_TO_DEVICE); This needs to be checked with dma_mapping_error(). > + > + for (ii = 0; ii < num_frag; ii++) { > + ++priv->tx_bd_tail; > + priv->tx_bd_tail %= TX_BD_NUM; > + cur_p = &priv->tx_bd_v[priv->tx_bd_tail]; > + frag = &skb_shinfo(skb)->frags[ii]; > + cur_p->phys = dma_map_single(ndev->dev.parent, > + skb_frag_address(frag), > + skb_frag_size(frag), > + DMA_TO_DEVICE); Needs to be checked against dma_mapping_error() and you would have to unwind the whole SKB linear + fragments mappings and buffer descriptors. > + cur_p->cntrl = skb_frag_size(frag); > + } > + > + cur_p->cntrl |= XAXIDMA_BD_CTRL_TXEOF_MASK; > + cur_p->app4 = (unsigned long)skb; > + > + tail_p = priv->tx_bd_p + sizeof(*priv->tx_bd_v) * priv->tx_bd_tail; > + /* Start the transfer */ You might be able to check for (!skb->xmit_more || netif_queue_stopped()) here to only do the write when you know for sure there is nothing more coming. > + nixge_dma_write_reg(priv, XAXIDMA_TX_TDESC_OFFSET, tail_p); > + ++priv->tx_bd_tail; > + priv->tx_bd_tail %= TX_BD_NUM; > + > + return NETDEV_TX_OK; > +} > + > +static void nixge_recv(struct net_device *ndev) > +{ > + u32 length; > + u32 size = 0; > + u32 packets = 0; > + dma_addr_t tail_p = 0; > + struct nixge_priv *priv = netdev_priv(ndev); > + struct sk_buff *skb, *new_skb; > + struct nixge_dma_bd *cur_p; > + > + cur_p = &priv->rx_bd_v[priv->rx_bd_ci]; Please do this in a NAPI context and bound the reception to the NAPI budget. > + > + while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)) { > + tail_p = priv->rx_bd_p > + + sizeof(*priv->rx_bd_v) * priv->rx_bd_ci; > + skb = (struct sk_buff *)(cur_p->sw_id_offset); > + > + length = cur_p->status & 0x7fffff; You can't trust the HW to return a length that is correct, you need to check that length is smaller than or equal to priv->max_frm_size here, otherwise you will overflow your skb size. > + dma_unmap_single(ndev->dev.parent, cur_p->phys, > + priv->max_frm_size, > + DMA_FROM_DEVICE); > + > + skb_put(skb, length); > + > + skb->protocol = eth_type_trans(skb, ndev); > + skb_checksum_none_assert(skb); > + > + /* For now mark them as CHECKSUM_NONE since > + * we don't have offload capabilities > + */ > + skb->ip_summed = CHECKSUM_NONE; > + > + netif_rx(skb); napi_gro_receive() or netif_receive_skb() at the very least, but that needs a conversion to NAPI first. > + > + size += length; > + packets++; > + > + new_skb = netdev_alloc_skb_ip_align(ndev, priv->max_frm_size); > + if (!new_skb) > + return; > + > + cur_p->phys = dma_map_single(ndev->dev.parent, new_skb->data, > + priv->max_frm_size, > + DMA_FROM_DEVICE); You need to check for dma_maping_error() here. > + cur_p->cntrl = priv->max_frm_size; > + cur_p->status = 0; > + cur_p->sw_id_offset = (u32)new_skb; > + > + ++priv->rx_bd_ci; > + priv->rx_bd_ci %= RX_BD_NUM; > + cur_p = &priv->rx_bd_v[priv->rx_bd_ci]; > + } > + > + ndev->stats.rx_packets += packets; > + ndev->stats.rx_bytes += size; > + > + if (tail_p) > + nixge_dma_write_reg(priv, XAXIDMA_RX_TDESC_OFFSET, tail_p); > +} > +static int nixge_open(struct net_device *ndev) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + struct phy_device *phy; > + int ret; > + > + nixge_device_reset(ndev); > + > + phy = of_phy_connect(ndev, priv->phy_node, > + &nixge_handle_link_change, 0, priv->phy_mode); > + if (!phy) > + return -ENODEV; > + > + phy_start(phy); > + > + /* Enable tasklets for Axi DMA error handling */ > + tasklet_init(&priv->dma_err_tasklet, nixge_dma_err_handler, > + (unsigned long)priv); > + > + /* Enable interrupts for Axi DMA Tx */ > + ret = request_irq(priv->tx_irq, nixge_tx_irq, 0, ndev->name, ndev); > + if (ret) > + goto err_tx_irq; > + /* Enable interrupts for Axi DMA Rx */ > + ret = request_irq(priv->rx_irq, nixge_rx_irq, 0, ndev->name, ndev); > + if (ret) > + goto err_rx_irq; netif_start_queue() is missing, if your queues were stopped before (try several up/down/up/down sequences to check) then it would never transmit. > + > + return 0; > + > +err_rx_irq: > + free_irq(priv->tx_irq, ndev); > +err_tx_irq: > + tasklet_kill(&priv->dma_err_tasklet); > + netdev_err(ndev, "request_irq() failed\n"); You are not stopping nor disconnecting the PHY in case of error. > + return ret; > +} > + > +static int nixge_stop(struct net_device *ndev) > +{ > + u32 cr; > + struct nixge_priv *priv = netdev_priv(ndev); First thing is probably to stop the transmit queue(s) with netif_stop_queue() to avoid submitting new packets. > + > + cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET); > + nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET, > + cr & (~XAXIDMA_CR_RUNSTOP_MASK)); > + cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET); > + nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET, > + cr & (~XAXIDMA_CR_RUNSTOP_MASK)); > + > + tasklet_kill(&priv->dma_err_tasklet); > + > + free_irq(priv->tx_irq, ndev); > + free_irq(priv->rx_irq, ndev); > + > + nixge_dma_bd_release(ndev); > + > + if (ndev->phydev) { > + phy_stop(ndev->phydev); > + phy_disconnect(ndev->phydev); > + } > + > + return 0; > +} > + > + > +static void nixge_ethtools_get_drvinfo(struct net_device *ndev, > + struct ethtool_drvinfo *ed) > +{ > + strlcpy(ed->driver, "nixge", sizeof(ed->driver)); You might want to return the bus type as well (e.g: platform). > +} > + > +static int nixge_ethtools_get_coalesce(struct net_device *ndev, > + struct ethtool_coalesce *ecoalesce) > +{ > + u32 regval = 0; > + struct nixge_priv *priv = netdev_priv(ndev); Reverse christmas tree declarations. > + > + regval = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET); > + ecoalesce->rx_max_coalesced_frames = (regval & XAXIDMA_COALESCE_MASK) > + >> XAXIDMA_COALESCE_SHIFT; > + regval = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET); > + ecoalesce->tx_max_coalesced_frames = (regval & XAXIDMA_COALESCE_MASK) > + >> XAXIDMA_COALESCE_SHIFT; > + return 0; > +} > + > +static int nixge_ethtools_set_coalesce(struct net_device *ndev, > + struct ethtool_coalesce *ecoalesce) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + > + if (netif_running(ndev)) { > + netdev_err(ndev, > + "Please stop netif before applying configuration\n"); > + return -EFAULT; -EBUSY may be, or -EINVAL? You are supposed to be able to allow changing coalescing parameters while the interface is running. > + } > + err = nixge_ctrl_poll_timeout(priv, NIXGE_REG_MDIO_CTRL, status, > + !status, 10, 1000); > + if (err) { > + dev_err(priv->dev, "timeout setting read command"); > + return err; > + } > + > + status = nixge_ctrl_read_reg(priv, NIXGE_REG_MDIO_DATA); > + > + dev_dbg(priv->dev, "%s: phy_id = %x reg = %x got %x\n", __func__, > + phy_id, reg & 0xffff, status); mdiobus_read() already contains trace points that would return the same information. > + > + return status; > +} > + > +static int nixge_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val) > +{ > + struct nixge_priv *priv = bus->priv; > + u32 status, tmp; > + int err; > + u16 device; > + > + /* FIXME: Currently don't do writes */ > + if (reg & MII_ADDR_C45) > + return -EOPNOTSUPP; Then you might as well remove Clause 45 read support, because it's not going to be very useful if you can't do writes. I could see how this allows you to get e.g: a 10GB PHY working with little to no intervention. > + > + device = reg & 0x1f; > + > + tmp = NIXGE_MDIO_CLAUSE22 | NIXGE_MDIO_OP(MDIO_C22_WRITE) | > + NIXGE_MDIO_ADDR(phy_id) | NIXGE_MDIO_MMD(device); > + > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_DATA, val); > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_OP, tmp); > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_CTRL, 1); > + > + err = nixge_ctrl_poll_timeout(priv, NIXGE_REG_MDIO_CTRL, status, > + !status, 10, 1000); > + if (err) { > + dev_err(priv->dev, "timeout setting write command"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "%x %x <- %x\n", phy_id, reg, val); > + > + return 0; > +} > + > +static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np) > +{ > + struct mii_bus *bus; > + struct resource res; > + int err; > + > + bus = mdiobus_alloc(); > + if (!bus) > + return -ENOMEM; > + > + of_address_to_resource(np, 0, &res); You don't appear to be using this resource. > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev)); > + bus->priv = priv; > + bus->name = "nixge_mii_bus"; > + bus->read = nixge_mdio_read; > + bus->write = nixge_mdio_write; > + bus->parent = priv->dev; > + > + priv->mii_bus = bus; > + err = of_mdiobus_register(bus, np); > + if (err) > + goto err_register; > + > + dev_info(priv->dev, "MDIO bus registered\n"); This is redundant with what you can obtain from of_mdiobus_register() and a "... MDIO bus probed type of message. > + > + return 0; > + > +err_register: > + mdiobus_free(bus); > + return err; > +} > + > +static void *nixge_get_nvmem_address(struct device *dev) > +{ > + struct nvmem_cell *cell; > + size_t cell_size; > + char *mac; > + > + cell = nvmem_cell_get(dev, "address"); > + if (IS_ERR(cell)) > + return cell; > + > + mac = nvmem_cell_read(cell, &cell_size); > + nvmem_cell_put(cell); > + > + return mac; > +} I would if this could be a candidate for some kind of generic helper function that would retrieve the MAC address, food for thought. > + > +static int nixge_probe(struct platform_device *pdev) > +{ > + int err; > + struct nixge_priv *priv; > + struct net_device *ndev; > + struct resource *dmares; > + const char *mac_addr; > + > + ndev = alloc_etherdev(sizeof(*priv)); > + if (!ndev) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, ndev); > + SET_NETDEV_DEV(ndev, &pdev->dev); > + > + ndev->flags &= ~IFF_MULTICAST; /* clear multicast */ > + ndev->features = NETIF_F_SG; > + ndev->netdev_ops = &nixge_netdev_ops; > + ndev->ethtool_ops = &nixge_ethtool_ops; > + > + /* MTU range: 64 - 9000 */ > + ndev->min_mtu = 64; > + ndev->max_mtu = NIXGE_JUMBO_MTU; > + > + mac_addr = nixge_get_nvmem_address(&pdev->dev); > + if (mac_addr && is_valid_ether_addr(mac_addr)) > + ether_addr_copy(ndev->dev_addr, mac_addr); > + else > + eth_hw_addr_random(ndev); > + > + priv = netdev_priv(ndev); > + priv->ndev = ndev; > + priv->dev = &pdev->dev; > + priv->rxmem = NIXGE_DEFAULT_RX_MEM; > + > + dmares = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->dma_regs = devm_ioremap_resource(&pdev->dev, dmares); > + if (IS_ERR(priv->dma_regs)) { > + netdev_err(ndev, "failed to map dma regs\n"); > + return PTR_ERR(priv->dma_regs); > + } > + priv->ctrl_regs = priv->dma_regs + NIXGE_REG_CTRL_OFFSET; > + __nixge_hw_set_mac_address(ndev); > + > + priv->tx_irq = platform_get_irq_byname(pdev, "tx-irq"); > + if (priv->tx_irq < 0) { > + netdev_err(ndev, "no tx irq available"); > + return priv->tx_irq; > + } > + > + priv->rx_irq = platform_get_irq_byname(pdev, "rx-irq"); > + if (priv->rx_irq < 0) { > + netdev_err(ndev, "no rx irq available"); > + return priv->rx_irq; > + } > + > + priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD; > + priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD; > + > + spin_lock_init(&priv->lock); > + > + err = nixge_mdio_setup(priv, pdev->dev.of_node); > + if (err) { > + netdev_err(ndev, "error registering mdio bus"); > + goto free_netdev; > + } > + > + priv->phy_mode = of_get_phy_mode(pdev->dev.of_node); > + if (priv->phy_mode < 0) { > + netdev_err(ndev, "not find phy-mode\n"); "Could not find \"phy-mode\" property" maybe? > + err = -EINVAL; > + goto unregister_mdio; > + } > + > + priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0); > + if (!priv->phy_node) { > + netdev_err(ndev, "not find phy-handle\n"); Same here. > + err = -EINVAL; > + goto unregister_mdio; > + } > + > + err = register_netdev(priv->ndev); > + if (err) { > + netdev_err(ndev, "register_netdev() error (%i)\n", err); > + goto unregister_mdio; > + } > + > + return 0; > + > +unregister_mdio: > + mdiobus_unregister(priv->mii_bus); > + mdiobus_free(priv->mii_bus); > + > +free_netdev: > + free_netdev(ndev); > + > + return err; > +} > + > +static int nixge_remove(struct platform_device *pdev) > +{ > + struct net_device *ndev = platform_get_drvdata(pdev); > + struct nixge_priv *priv = netdev_priv(ndev); > + > + if (ndev->phydev) > + phy_disconnect(ndev->phydev); You should consider moving this to the ndo_stop() for mainly two reasons: - to be strictly symmetrical with your ndo_open() function which does the of_phy_connect() call - to leverage possible power savings by suspending the PHY when the interface is not used > + ndev->phydev = NULL; phy_disconnect() does NULLify dev->phydev already > + > + mdiobus_unregister(priv->mii_bus); > + mdiobus_free(priv->mii_bus); > + priv->mii_bus = NULL; This is not necessary, probe() and remove() won't be called with partially initialized private structure data. > + > + unregister_netdev(ndev); > + > + free_netdev(ndev); > + > + return 0; > +} > + > +/* Match table for of_platform binding */ > +static const struct of_device_id nixge_dt_ids[] = { > + { .compatible = "ni,xge-enet-2.00", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, nixge_dt_ids); > + > +static struct platform_driver nixge_driver = { > + .probe = nixge_probe, > + .remove = nixge_remove, > + .driver = { > + .name = "nixge", > + .of_match_table = of_match_ptr(nixge_dt_ids), > + }, > +}; > +module_platform_driver(nixge_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("National Instruments XGE Management MAC"); > +MODULE_AUTHOR("Moritz Fischer <mdf@kernel.org>"); >
> +++ b/drivers/net/ethernet/ni/Kconfig > @@ -0,0 +1,27 @@ > +# > +# National Instuments network device configuration > +# > + > +config NET_VENDOR_NI > + bool "National Instruments Devices" > + default y > + ---help--- > + If you have a network (Ethernet) device 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 National Instrument devices. > + If you say Y, you will be asked for your specific device in the > + following questions. > + > +if NET_VENDOR_NI > + > +config NI_XGE_MANAGEMENT_ENET > + tristate "National Instruments XGE management enet support" > + depends on ARCH_ZYNQ Consider also adding COMPILE_TEST, if possible. > +#define nixge_ctrl_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \ > + readl_poll_timeout((priv)->ctrl_regs + (addr), (val), cond, \ > + (sleep_us), (timeout_us)) Seems odd not having cond inside (), especially since cond could be a complex expression. > +static void nixge_handle_link_change(struct net_device *ndev) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + struct phy_device *phydev = ndev->phydev; > + unsigned long flags; > + int status_change = 0; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + if (phydev->link != priv->link || phydev->speed != priv->speed || > + phydev->duplex != priv->duplex) { > + priv->link = phydev->link; > + priv->speed = phydev->speed; > + priv->duplex = phydev->duplex; > + status_change = 1; > + } > + > + spin_unlock_irqrestore(&priv->lock, flags); > + > + if (status_change) > + phy_print_status(phydev); > +} As Florian pointed out, you don't make use of any of this information. So maybe don't bother, just have a return statement. > +static int nixge_stop(struct net_device *ndev) > +{ > + u32 cr; > + struct nixge_priv *priv = netdev_priv(ndev); > + > + cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET); > + nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET, > + cr & (~XAXIDMA_CR_RUNSTOP_MASK)); > + cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET); > + nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET, > + cr & (~XAXIDMA_CR_RUNSTOP_MASK)); > + > + tasklet_kill(&priv->dma_err_tasklet); > + > + free_irq(priv->tx_irq, ndev); > + free_irq(priv->rx_irq, ndev); > + > + nixge_dma_bd_release(ndev); > + > + if (ndev->phydev) { Do you need this condition? You bail out with ENODEV if of_phy_connect fails? > + phy_stop(ndev->phydev); > + phy_disconnect(ndev->phydev); > + } > + > + return 0; > +} > + > +static int nixge_change_mtu(struct net_device *ndev, int new_mtu) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + > + if (netif_running(ndev)) > + return -EBUSY; > + > + if ((new_mtu + VLAN_ETH_HLEN + > + NIXGE_TRL_SIZE) > priv->rxmem) > + return -EINVAL; > + > + ndev->mtu = new_mtu; > + > + return 0; > +} > + > +static s32 __nixge_hw_set_mac_address(struct net_device *ndev) > +{ > + struct nixge_priv *priv = netdev_priv(ndev); > + > + nixge_ctrl_write_reg(priv, NIXGE_REG_MAC_LSB, > + (ndev->dev_addr[2]) << 24 | > + (ndev->dev_addr[3] << 16) | > + (ndev->dev_addr[4] << 8) | > + (ndev->dev_addr[5] << 0)); > + > + nixge_ctrl_write_reg(priv, NIXGE_REG_MAC_MSB, > + (ndev->dev_addr[1] | (ndev->dev_addr[0] << 8))); > + > + return 0; > +} > + > +static int nixge_net_set_mac_address(struct net_device *ndev, void *p) > +{ > + int err; > + > + err = eth_mac_addr(ndev, p); > + if (!err) > + __nixge_hw_set_mac_address(ndev); > + > + return err; > +} Much better, thanks. > + > +static int nixge_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val) > +{ > + struct nixge_priv *priv = bus->priv; > + u32 status, tmp; > + int err; > + u16 device; > + > + /* FIXME: Currently don't do writes */ > + if (reg & MII_ADDR_C45) > + return -EOPNOTSUPP; > + > + device = reg & 0x1f; > + > + tmp = NIXGE_MDIO_CLAUSE22 | NIXGE_MDIO_OP(MDIO_C22_WRITE) | > + NIXGE_MDIO_ADDR(phy_id) | NIXGE_MDIO_MMD(device); > + > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_DATA, val); > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_OP, tmp); > + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_CTRL, 1); > + > + err = nixge_ctrl_poll_timeout(priv, NIXGE_REG_MDIO_CTRL, status, > + !status, 10, 1000); > + if (err) { > + dev_err(priv->dev, "timeout setting write command"); > + return -ETIMEDOUT; return err; > + } > + > + dev_dbg(priv->dev, "%x %x <- %x\n", phy_id, reg, val); > + > + return 0; > +} > + > +static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np) > +{ > + struct mii_bus *bus; > + struct resource res; > + int err; > + > + bus = mdiobus_alloc(); > + if (!bus) > + return -ENOMEM; > + > + of_address_to_resource(np, 0, &res); This can fail. Err, why are you actually doing it anyway? You don't make use of res, you don't ioremap() it, etc. > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev)); > + bus->priv = priv; > + bus->name = "nixge_mii_bus"; > + bus->read = nixge_mdio_read; > + bus->write = nixge_mdio_write; > + bus->parent = priv->dev; > + > + priv->mii_bus = bus; > + err = of_mdiobus_register(bus, np); > + if (err) > + goto err_register; > + > + dev_info(priv->dev, "MDIO bus registered\n"); > + > + return 0; > + > +err_register: > + mdiobus_free(bus); > + return err; > +} > + > +static int nixge_probe(struct platform_device *pdev) > +{ > + int err; > + struct nixge_priv *priv; > + struct net_device *ndev; > + struct resource *dmares; > + const char *mac_addr; > + > + ndev = alloc_etherdev(sizeof(*priv)); > + if (!ndev) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, ndev); > + SET_NETDEV_DEV(ndev, &pdev->dev); > + > + ndev->flags &= ~IFF_MULTICAST; /* clear multicast */ Could you explain this a bit better please. Does this imply that IPv6 neighbour discovery is not supported? That is a severe restriction. > +static int nixge_remove(struct platform_device *pdev) > +{ > + struct net_device *ndev = platform_get_drvdata(pdev); > + struct nixge_priv *priv = netdev_priv(ndev); > + > + if (ndev->phydev) > + phy_disconnect(ndev->phydev); nixge_stop() disconnects the phy. I don't think you need it twice. > + ndev->phydev = NULL; > + > + mdiobus_unregister(priv->mii_bus); > + mdiobus_free(priv->mii_bus); > + priv->mii_bus = NULL; > + > + unregister_netdev(ndev); > + > + free_netdev(ndev); > + > + return 0; Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 14, 2017 at 01:48:45PM -0700, Moritz Fischer wrote: > This adds bindings for the NI XGE 1G/10G network device. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > Documentation/devicetree/bindings/net/nixge.txt | 32 +++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/nixge.txt > > diff --git a/Documentation/devicetree/bindings/net/nixge.txt b/Documentation/devicetree/bindings/net/nixge.txt > new file mode 100644 > index 0000000..9fff5a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/nixge.txt > @@ -0,0 +1,32 @@ > +* NI XGE Ethernet controller > + > +Required properties: > +- compatible: Should be "ni,xge-enet-2.00" > +- reg: Address and length of the register set for the device > +- interrupts: Should contain tx and rx interrupt > +- interrupt-names: Should be "rx-irq" and "tx-irq" > +- phy-mode: See ethernet.txt file in the same directory. Hi Moritz phy-handle is now required. > +Examples (10G generic PHY): > + nixge0: ethernet@40000000 { > + compatible = "ni,xge-enet-2.00"; > + reg = <0x40000000 0x6000>; > + > + nvmem-cells = <ð1_addr>; > + nvmem-cell-names = "address"; > + > + interrupts = <0 29 4>, <0 30 4>; IRQ_TYPE_LEVEL_HIGH > + interrupt-names = "rx-irq", "tx-irq"; > + interrupt-parent = <&intc>; > + > + phy-mode = "xgmii"; > + phy-handle = <ðernet_phy1>; > + > + ethernet_phy1: ethernet-phy@4 { > + compatible = "ethernet-phy-ieee802.3-c45"; > + reg = <4>; > + devices = <0xa>; > + }; Since you don't fully implement c45, does this example actually work? And devices is not a standard phy property. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On Sat, Jul 15, 2017 at 08:37:45PM +0200, Andrew Lunn wrote: > On Fri, Jul 14, 2017 at 01:48:45PM -0700, Moritz Fischer wrote: > > This adds bindings for the NI XGE 1G/10G network device. > > > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > > --- > > Documentation/devicetree/bindings/net/nixge.txt | 32 +++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/nixge.txt > > > > diff --git a/Documentation/devicetree/bindings/net/nixge.txt b/Documentation/devicetree/bindings/net/nixge.txt > > new file mode 100644 > > index 0000000..9fff5a7 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/nixge.txt > > @@ -0,0 +1,32 @@ > > +* NI XGE Ethernet controller > > + > > +Required properties: > > +- compatible: Should be "ni,xge-enet-2.00" > > +- reg: Address and length of the register set for the device > > +- interrupts: Should contain tx and rx interrupt > > +- interrupt-names: Should be "rx-irq" and "tx-irq" > > +- phy-mode: See ethernet.txt file in the same directory. > > Hi Moritz > > phy-handle is now required. Good catch, thanks. > > > +Examples (10G generic PHY): > > + nixge0: ethernet@40000000 { > > + compatible = "ni,xge-enet-2.00"; > > + reg = <0x40000000 0x6000>; > > + > > + nvmem-cells = <ð1_addr>; > > + nvmem-cell-names = "address"; > > + > > + interrupts = <0 29 4>, <0 30 4>; > > IRQ_TYPE_LEVEL_HIGH Sure, will do. > > > + interrupt-names = "rx-irq", "tx-irq"; > > + interrupt-parent = <&intc>; > > + > > + phy-mode = "xgmii"; > > + phy-handle = <ðernet_phy1>; > > + > > + ethernet_phy1: ethernet-phy@4 { > > + compatible = "ethernet-phy-ieee802.3-c45"; > > + reg = <4>; > > + devices = <0xa>; > > + }; > > Since you don't fully implement c45, does this example actually work? Yeah, I've tested this continuously. But for v3 I anyways implmented c45 writes. > And devices is not a standard phy property. > Will fix. > Andrew Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > + ethernet_phy1: ethernet-phy@4 { > > > + compatible = "ethernet-phy-ieee802.3-c45"; > > > + reg = <4>; > > > + devices = <0xa>; > > > + }; > > > > Since you don't fully implement c45, does this example actually work? > > Yeah, I've tested this continuously. But for v3 I anyways implmented c45 > writes. Hi Moritz Just out of interest, what PHY are you using? Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 15, 2017 at 09:48:32PM +0200, Andrew Lunn wrote: > > > > + ethernet_phy1: ethernet-phy@4 { > > > > + compatible = "ethernet-phy-ieee802.3-c45"; > > > > + reg = <4>; > > > > + devices = <0xa>; > > > > + }; > > > > > > Since you don't fully implement c45, does this example actually work? > > > > Yeah, I've tested this continuously. But for v3 I anyways implmented c45 > > writes. > > Hi Moritz > > Just out of interest, what PHY are you using? Depending on whether the FPGA image is configured either: - Xilinx 10G PCS/PMA LogiCore IP (C45) - Xilinx LogiCORE IP Ethernet 1000Base-X PCS/PMA (C22) in between that and the DMA engine there's a bunch of custom stuff (will be open source once the product ships). Moritz
On Fri, Jul 14, 2017 at 01:48:45PM -0700, Moritz Fischer wrote: > This adds bindings for the NI XGE 1G/10G network device. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > Documentation/devicetree/bindings/net/nixge.txt | 32 +++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/nixge.txt > > diff --git a/Documentation/devicetree/bindings/net/nixge.txt b/Documentation/devicetree/bindings/net/nixge.txt > new file mode 100644 > index 0000000..9fff5a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/nixge.txt > @@ -0,0 +1,32 @@ > +* NI XGE Ethernet controller > + > +Required properties: > +- compatible: Should be "ni,xge-enet-2.00" > +- reg: Address and length of the register set for the device > +- interrupts: Should contain tx and rx interrupt > +- interrupt-names: Should be "rx-irq" and "tx-irq" The "-irq" part is redundant. > +- phy-mode: See ethernet.txt file in the same directory. > +- nvmem-cells: Phandle of nvmem cell containing the mac address s/mac/MAC/ > +- nvmem-cell-names: Should be "address" > + > +Examples (10G generic PHY): > + nixge0: ethernet@40000000 { > + compatible = "ni,xge-enet-2.00"; > + reg = <0x40000000 0x6000>; > + > + nvmem-cells = <ð1_addr>; > + nvmem-cell-names = "address"; > + > + interrupts = <0 29 4>, <0 30 4>; > + interrupt-names = "rx-irq", "tx-irq"; > + interrupt-parent = <&intc>; > + > + phy-mode = "xgmii"; > + phy-handle = <ðernet_phy1>; > + > + ethernet_phy1: ethernet-phy@4 { > + compatible = "ethernet-phy-ieee802.3-c45"; > + reg = <4>; > + devices = <0xa>; ?? > + }; > + }; > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/nixge.txt b/Documentation/devicetree/bindings/net/nixge.txt new file mode 100644 index 0000000..9fff5a7 --- /dev/null +++ b/Documentation/devicetree/bindings/net/nixge.txt @@ -0,0 +1,32 @@ +* NI XGE Ethernet controller + +Required properties: +- compatible: Should be "ni,xge-enet-2.00" +- reg: Address and length of the register set for the device +- interrupts: Should contain tx and rx interrupt +- interrupt-names: Should be "rx-irq" and "tx-irq" +- phy-mode: See ethernet.txt file in the same directory. +- nvmem-cells: Phandle of nvmem cell containing the mac address +- nvmem-cell-names: Should be "address" + +Examples (10G generic PHY): + nixge0: ethernet@40000000 { + compatible = "ni,xge-enet-2.00"; + reg = <0x40000000 0x6000>; + + nvmem-cells = <ð1_addr>; + nvmem-cell-names = "address"; + + interrupts = <0 29 4>, <0 30 4>; + interrupt-names = "rx-irq", "tx-irq"; + interrupt-parent = <&intc>; + + phy-mode = "xgmii"; + phy-handle = <ðernet_phy1>; + + ethernet_phy1: ethernet-phy@4 { + compatible = "ethernet-phy-ieee802.3-c45"; + reg = <4>; + devices = <0xa>; + }; + };
This adds bindings for the NI XGE 1G/10G network device. Signed-off-by: Moritz Fischer <mdf@kernel.org> --- Documentation/devicetree/bindings/net/nixge.txt | 32 +++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/nixge.txt