Message-ID: <20180301221228.GD343@lunn.ch>
Date:   Thu, 1 Mar 2018 23:12:28 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Bryan Whitehead <Bryan.Whitehead@...rochip.com>
Cc:     davem@...emloft.net, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 1/2] lan743x: Add main source files for new
 lan743x driver

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

Powered by blists - more mailing lists