From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from vps0.lunn.ch ([185.16.172.187]:42444 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1162460AbeCAWMg (ORCPT ); Thu, 1 Mar 2018 17:12:36 -0500 Date: Thu, 1 Mar 2018 23:12:28 +0100 From: Andrew Lunn To: Bryan Whitehead Cc: davem@davemloft.net, UNGLinuxDriver@microchip.com, netdev@vger.kernel.org Subject: Re: [PATCH v3 net-next 1/2] lan743x: Add main source files for new lan743x driver Message-ID: <20180301221228.GD343@lunn.ch> References: <1519938009-10322-1-git-send-email-Bryan.Whitehead@microchip.com> <1519938009-10322-2-git-send-email-Bryan.Whitehead@microchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1519938009-10322-2-git-send-email-Bryan.Whitehead@microchip.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Mar 01, 2018 at 04:00:08PM -0500, Bryan Whitehead wrote: > +static int lan743x_phy_reset(struct lan743x_adapter *adapter) > +{ > + u32 data; > + > + data = lan743x_csr_read(adapter, PMT_CTL); > + data |= PMT_CTL_ETH_PHY_RST_; > + lan743x_csr_write(adapter, PMT_CTL, data); > + > + return readx_poll_timeout(LAN743X_CSR_READ_OP, PMT_CTL, data, > + (!(data & PMT_CTL_ETH_PHY_RST_) && > + (data & PMT_CTL_READY_)), > + 50000, 1000000); > +} Hi Bryan Could you explain this a bit more. What exactly is it resetting? Do we need to tell the phylib that the PHY has been reset and that it needs to re-program it? Or by phy do you mean a SERDES interface? > +static int lan743x_phy_open(struct lan743x_adapter *adapter) > +{ > + struct lan743x_phy *phy = &adapter->phy; > + struct phy_device *phydev; > + struct net_device *netdev; > + int ret = -EIO; > + u32 mii_adv; > + > + netdev = adapter->netdev; > + phydev = phy_find_first(adapter->mdiobus); > + if (!phydev) > + goto return_error; > + > + ret = phy_connect_direct(netdev, phydev, > + lan743x_phy_link_status_change, > + PHY_INTERFACE_MODE_GMII); > + if (ret) > + goto return_error; > + > + /* MAC doesn't support 1000T Half */ > + phydev->supported &= ~SUPPORTED_1000baseT_Half; > + > + /* support both flow controls */ > + phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX); > + phydev->advertising &= ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause); > + mii_adv = (u32)mii_advertise_flowctrl(phy->fc_request_control); > + phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv); > + phy->fc_autoneg = phydev->autoneg; > + > + /* PHY interrupt enabled here */ > + phy_start(phydev); > + phy_start_aneg(phydev); > + return 0; Are phy interrupts really enabled here? I could of missed it, but i don't see anywhere PHY interrupts are configured. This is either done via device tree, you set phydev->irq, or mdiobus->irq[X]. > +static int lan743x_tx_open(struct lan743x_tx *tx) > +{ > + struct lan743x_adapter *adapter = NULL; > + u32 data = 0; > + int ret; > + > + adapter = tx->adapter; > + ret = lan743x_tx_ring_init(tx); > + if (ret) > + goto return_error; You could just return here. You don't do anything useful at return_error: > + /* start dmac channel */ > + lan743x_csr_write(adapter, DMAC_CMD, > + DMAC_CMD_START_T_(tx->channel_number)); > + return 0; > + > +return_error: > + return ret; > +} > +/* lan743x_pcidev_probe - Device Initialization Routine > + * @pdev: PCI device information struct > + * @id: entry in lan743x_pci_tbl > + * > + * Returns 0 on success, negative on failure > + * > + * initializes an adapter identified by a pci_dev structure. > + * The OS initialization, configuring of the adapter private structure, > + * and a hardware reset occur. > + **/ > +static int lan743x_pcidev_probe(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + struct lan743x_adapter *adapter = NULL; > + struct net_device *netdev = NULL; > + int ret = -ENODEV; > + > + netdev = devm_alloc_etherdev(&pdev->dev, > + sizeof(struct lan743x_adapter)); > + if (!netdev) > + goto return_error; > + > + SET_NETDEV_DEV(netdev, &pdev->dev); > + pci_set_drvdata(pdev, netdev); > + adapter = netdev_priv(netdev); > + adapter->netdev = netdev; > + adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE | > + NETIF_MSG_LINK | NETIF_MSG_IFUP | > + NETIF_MSG_IFDOWN | NETIF_MSG_TX_QUEUED; > + netdev->max_mtu = LAN743X_MAX_FRAME_SIZE; > + > + ret = lan743x_pci_init(adapter, pdev); > + if (ret) > + goto return_error; > + > + ret = lan743x_csr_init(adapter); > + if (ret) > + goto cleanup_pci; > + > + ret = lan743x_hardware_init(adapter, pdev); > + if (ret) > + goto cleanup_pci; > + > + ret = lan743x_mdiobus_init(adapter); > + if (ret) > + goto cleanup_hardware; > + > + adapter->netdev->netdev_ops = &lan743x_netdev_ops; > + adapter->netdev->features = NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM; > + adapter->netdev->hw_features = adapter->netdev->features; > + > + /* carrier off reporting is important to ethtool even BEFORE open */ > + netif_carrier_off(netdev); > + > + ret = register_netdev(adapter->netdev); > + if (ret < 0) > + goto cleanup_mdiobus; > + return 0; > + > +cleanup_mdiobus: > + lan743x_mdiobus_cleanup(adapter); > + > +cleanup_hardware: > + lan743x_hardware_cleanup(adapter); > + > +cleanup_pci: > + lan743x_pci_cleanup(adapter); > + > +return_error: > + pr_warn("Initialization failed\n"); > + return ret; > +} This is much nicer without all the flags. Thanks. > +static struct pci_driver lan743x_pcidev_driver = { > + .name = DRIVER_NAME, > + .id_table = lan743x_pcidev_tbl, > + .probe = lan743x_pcidev_probe, > + .remove = lan743x_pcidev_remove, > + .shutdown = lan743x_pcidev_shutdown, > +}; > + > +static int __init lan743x_module_init(void) > +{ > + int result = -EINVAL; > + > + pr_info(DRIVER_DESC "\n"); > + result = pci_register_driver(&lan743x_pcidev_driver); > + if (result) > + pr_warn("pci_register_driver returned error code, %d\n", > + result); > + return result; > +} > + > +module_init(lan743x_module_init); > + > +static void __exit lan743x_module_exit(void) > +{ > + pci_unregister_driver(&lan743x_pcidev_driver); > +} > > +module_exit(lan743x_module_exit); You can replace this boilerplate code with module_pci_driver(). You don't do anything special here. Andrew