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
