Date:   Wed, 14 Feb 2018 02:11:32 +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 v1 net-next 1/3] lan743x: Add main source file for new
 lan743x driver

On Tue, Feb 13, 2018 at 06:26:48PM -0500, Bryan Whitehead wrote:
> Add main source files for new lan743x driver
> 
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@...rochip.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 2964 +++++++++++++++++++++++++
>  drivers/net/ethernet/microchip/lan743x_main.h | 1331 +++++++++++
>  2 files changed, 4295 insertions(+)
>  create mode 100644 drivers/net/ethernet/microchip/lan743x_main.c
>  create mode 100644 drivers/net/ethernet/microchip/lan743x_main.h
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> new file mode 100644
> index 0000000..6cfb439
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -0,0 +1,2964 @@
> +/*
> + * Copyright (C) 2018 Microchip Technology
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "lan743x_main.h"
> +
> +#define LAN743X_COMPONENT_FLAG_PCI          BIT(0)
> +#define LAN743X_COMPONENT_FLAG_CSR          BIT(1)
> +#define LAN743X_COMPONENT_FLAG_INTR         BIT(2)
> +#define LAN743X_COMPONENT_FLAG_DP           BIT(3)
> +#define LAN743X_COMPONENT_FLAG_MAC          BIT(4)
> +#define LAN743X_COMPONENT_FLAG_PHY          BIT(5)
> +#define LAN743X_COMPONENT_FLAG_RFE          BIT(6)
> +#define LAN743X_COMPONENT_FLAG_FCT          BIT(7)
> +#define LAN743X_COMPONENT_FLAG_TX(channel)  BIT(16 + (channel))
> +#define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
> +
> +#define LAN743X_INIT_FLAG_NETDEV_REGISTERED	BIT(24)
> +#define LAN743X_INIT_FLAG_MDIOBUS_ALLOCATED	BIT(25)
> +#define LAN743X_INIT_FLAG_MDIOBUS_REGISTERED	BIT(26)

Hi Bryan

It seems odd to have some defines in lan743x_main.h and some inline.
It is probably better to have them all in one place.

> +static int lan743x_csr_light_reset(struct lan743x_adapter *adapter)
> +{
> +	unsigned long timeout;
> +	u32 data;
> +
> +	data = lan743x_csr_read(adapter, HW_CFG);
> +	data |= HW_CFG_LRST_;
> +	lan743x_csr_write(adapter, HW_CFG, data);
> +	timeout = jiffies + (10 * HZ);
> +	do {
> +		if (time_after(jiffies, timeout))
> +			return -EIO;
> +		msleep(100);
> +		data = lan743x_csr_read(adapter, HW_CFG);
> +	} while (data & HW_CFG_LRST_);

You could probably use readx_poll_timeout() here.

> +
> +	return 0;
> +}
> +
> +static int lan743x_csr_wait_for_bit(struct lan743x_adapter *adapter,
> +				    int offset, u32 bit_mask,
> +				    int target_value, int usleep_min,
> +				    int usleep_max, int count)
> +{
> +	int timeout = count;
> +	int current_value;
> +	int ret = -EIO;
> +
> +	while (timeout) {
> +		current_value = (lan743x_csr_read(adapter, offset) & bit_mask)
> +				? 1 : 0;
> +		if (target_value == current_value) {
> +			ret = 0;
> +			break;
> +		}
> +		usleep_range(usleep_min, usleep_max);
> +		timeout--;
> +	}

And here as well.

> +	return ret;
> +}
> +
> +static int lan743x_csr_init(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_csr *csr = &adapter->csr;
> +	resource_size_t bar_start, bar_length;
> +	int result;
> +
> +	bar_start = pci_resource_start(adapter->pci.pdev, 0);
> +	bar_length = pci_resource_len(adapter->pci.pdev, 0);
> +	csr->csr_address = ioremap(bar_start, bar_length);

devm_ioremap() will make your cleanup code simpler.

> +	if (!csr->csr_address) {
> +		result = -ENOMEM;
> +		goto clean_up;
> +	}
> +
> +	csr->id_rev = lan743x_csr_read(adapter, ID_REV);
> +	csr->fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
> +	netif_info(adapter, probe, adapter->netdev,
> +		   "ID_REV = 0x%08X, FPGA_REV = %d.%d\n",
> +		   csr->id_rev,	(csr->fpga_rev) & 0x000000FF,
> +		   ((csr->fpga_rev) >> 8) & 0x000000FF);
> +	if ((csr->id_rev & 0xFFF00000) != 0x74300000) {
> +		result = -ENODEV;
> +		goto clean_up;
> +	}

You have:

define ID_REV_CHIP_ID_MASK_                 (0xFFFF0000)
#define ID_REV_CHIP_ID_7430_                 (0x7430)
#define ID_REV_CHIP_REV_MASK_                        (0x0000FFFF)
#define ID_REV_CHIP_REV_A0_                  (0x00000000)
#define ID_REV_CHIP_REV_B0_                  (0x00000010)

#define FPGA_REV				(0x04)	    
#define FPGA_REV_MINOR_MASK_			(0x0000FF00)
#define FPGA_REV_MAJOR_MASK_			(0x000000FF)

Why not use them.

> +	csr->flags = LAN743X_CSR_FLAG_SUPPORTS_INTR_AUTO_SET_CLR;
> +	switch (csr->id_rev & ID_REV_CHIP_REV_MASK_) {
> +	case ID_REV_CHIP_REV_A0_:
> +		csr->flags |= LAN743X_CSR_FLAG_IS_A0;
> +		csr->flags &= ~LAN743X_CSR_FLAG_SUPPORTS_INTR_AUTO_SET_CLR;
> +		break;
> +	case ID_REV_CHIP_REV_B0_:
> +		csr->flags |= LAN743X_CSR_FLAG_IS_B0;
> +		break;
> +	}
> +
> +	result = lan743x_csr_light_reset(adapter);
> +	if (result)
> +		goto clean_up;
> +	return 0;
> +clean_up:
> +	if (csr->csr_address)
> +		iounmap(csr->csr_address);
> +	return result;
> +}

> +static int lan743x_intr_register_isr(struct lan743x_adapter *adapter,
> +				     int vector_index, u32 flags,
> +				     u32 int_mask,
> +				     lan743x_vector_handler handler,
> +				     void *context)
> +{
> +	struct lan743x_vector *vector = &adapter->intr.vector_list
> +					[vector_index];
> +	int ret;
> +
> +	vector->adapter = adapter;
> +	vector->flags = flags;
> +	vector->vector_index = vector_index;
> +	vector->int_mask = int_mask;
> +	vector->handler = handler;
> +	vector->context = context;
> +	ret = request_irq(vector->irq,
> +			  lan743x_intr_entry_isr,
> +			  (flags & LAN743X_VECTOR_FLAG_IRQ_SHARED) ?
> +			  IRQF_SHARED : 0,
> +			  DRIVER_NAME,
> +			  vector);

devm_request_irq()?

> +static int lan743x_intr_open(struct lan743x_adapter *adapter)
> +{
> +#if LAN743X_TRY_MSIX
> +	struct msix_entry msix_entries[LAN743X_MAX_VECTOR_COUNT];
> +#endif
> +	struct lan743x_intr *intr = &adapter->intr;
> +	u32 int_vec_en_auto_clr = 0;
> +	u32 int_vec_map0 = 0;
> +	u32 int_vec_map1 = 0;
> +	int ret = -ENODEV;
> +	int index = 0;
> +	u32 flags = 0;
> +
> +	intr->number_of_vectors = 0;
> +#if LAN743X_TRY_MSIX

It is better to use

        if (IS_ENABLED(CONFIG_LAN743X_TRY_MSIX)) {

The compiler will then detect problems in the code, even when it is
disabled, and still optimize it out. In general, we don't like #ifdefs.

> +	memset(&msix_entries[0], 0,
> +	       sizeof(struct msix_entry) * LAN743X_MAX_VECTOR_COUNT);
> +	for (index = 0; index < LAN743X_MAX_VECTOR_COUNT; index++)
> +		msix_entries[index].entry = index;
> +	ret = pci_enable_msix_range(adapter->pci.pdev,
> +				    msix_entries, 1,
> +				    1 + LAN743X_USED_TX_CHANNELS +
> +				    LAN743X_USED_RX_CHANNELS);
> +	if (ret > 0) {
> +		intr->flags |= INTR_FLAG_MSIX_ENABLED;
> +		intr->number_of_vectors = ret;
> +		intr->using_vectors = true;
> +		for (index = 0; index < intr->number_of_vectors; index++)
> +			intr->vector_list[index].irq = msix_entries
> +						       [index].vector;
> +		netif_info(adapter, ifup, adapter->netdev,
> +			   "using MSIX interrupts, number of vectors = %d\n",
> +			   intr->number_of_vectors);
> +	}
> +#endif
> +#if LAN743X_TRY_MSI
> +	if (!intr->number_of_vectors) {
> +		if (!(adapter->csr.flags & LAN743X_CSR_FLAG_IS_A0)) {
> +			if (!pci_enable_msi(adapter->pci.pdev)) {
> +				intr->flags |= INTR_FLAG_MSI_ENABLED;
> +				intr->number_of_vectors = 1;
> +				intr->using_vectors = true;
> +				intr->vector_list[0].irq =
> +					adapter->pci.pdev->irq;
> +				netif_info(adapter, ifup, adapter->netdev,
> +					   "using MSI interrupts, number of vectors = %d\n",
> +					   intr->number_of_vectors);
> +			}
> +		}
> +	}
> +#endif

> +static int lan743x_mac_mii_wait_till_not_busy(struct lan743x_adapter *adapter)
> +{
> +	unsigned long start_time = jiffies;
> +	u32 data;
> +
> +	do {
> +		data = lan743x_csr_read(adapter, MAC_MII_ACC);
> +		if (!(data & MAC_MII_ACC_MII_BUSY_))
> +			return 0;
> +	} while (!time_after(jiffies, start_time + HZ));

Another possible use of readx_poll_timeout.

> +static void lan743x_mac_set_address(struct lan743x_adapter *adapter,
> +				    u8 *addr)
> +{
> +	u32 addr_lo, addr_hi;
> +
> +	addr_lo = addr[0] |
> +		addr[1] << 8 |
> +		addr[2] << 16 |
> +		addr[3] << 24;
> +	addr_hi = addr[4] |
> +		addr[5] << 8;
> +	lan743x_csr_write(adapter, MAC_RX_ADDRL, addr_lo);
> +	lan743x_csr_write(adapter, MAC_RX_ADDRH, addr_hi);
> +	ether_addr_copy(adapter->mac.mac_address, addr);
> +	netif_info(adapter, drv, adapter->netdev,
> +		   "MAC address set to %02X:%02X:%02X:%02X:%02X:%02X\n",

%pM ?

> +		   addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
> +}
> +
> +static int lan743x_mac_init(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_mac *mac = &adapter->mac;
> +	bool mac_address_valid = true;
> +	struct net_device *netdev;
> +	u32 mac_addr_hi = 0;
> +	u32 mac_addr_lo = 0;
> +	u32 data;
> +	int ret;
> +
> +	netdev = adapter->netdev;
> +	lan743x_csr_write(adapter, MAC_CR, MAC_CR_RST_);
> +	ret = lan743x_csr_wait_for_bit(adapter, MAC_CR, MAC_CR_RST_,
> +				       0, 1000, 20000, 100);
> +	if (ret)
> +		return ret;
> +
> +	/* setup auto duplex, and speed detection */
> +	data = lan743x_csr_read(adapter, MAC_CR);
> +	data |= MAC_CR_ADD_ | MAC_CR_ASD_;
> +	data |= MAC_CR_CNTR_RST_;
> +	lan743x_csr_write(adapter, MAC_CR, data);
> +	mac_addr_hi = lan743x_csr_read(adapter, MAC_RX_ADDRH);
> +	mac_addr_lo = lan743x_csr_read(adapter, MAC_RX_ADDRL);
> +	mac->mac_address[0] = mac_addr_lo & 0xFF;
> +	mac->mac_address[1] = (mac_addr_lo >> 8) & 0xFF;
> +	mac->mac_address[2] = (mac_addr_lo >> 16) & 0xFF;
> +	mac->mac_address[3] = (mac_addr_lo >> 24) & 0xFF;
> +	mac->mac_address[4] = mac_addr_hi & 0xFF;
> +	mac->mac_address[5] = (mac_addr_hi >> 8) & 0xFF;
> +	if (((mac_addr_hi & 0x0000FFFF) == 0x0000FFFF) &&
> +	    mac_addr_lo == 0xFFFFFFFF) {
> +		mac_address_valid = false;
> +	} else if (!is_valid_ether_addr(mac->mac_address)) {
> +		mac_address_valid = false;
> +	}
> +	if (!mac_address_valid)
> +		random_ether_addr(mac->mac_address);
> +	lan743x_mac_set_address(adapter, mac->mac_address);
> +	netif_info(adapter, probe, adapter->netdev,
> +		   "MAC Address = %02X:%02X:%02X:%02X:%02X:%02X\n",
> +		   mac->mac_address[0], mac->mac_address[1],
> +		   mac->mac_address[2], mac->mac_address[3],
> +		   mac->mac_address[4], mac->mac_address[5]);

lan743x_mac_set_address() prints it. Why print it again?

> +	ether_addr_copy(netdev->dev_addr, mac->mac_address);
> +	return 0;
> +}
> +

> +
> +/* PHY */
> +#define PHY_FLAG_OPENED     BIT(0)
> +#define PHY_FLAG_ATTACHED   BIT(1)
> +static int lan743x_phy_reset(struct lan743x_adapter *adapter)
> +{
> +	unsigned long timeout;
> +	u32 data;
> +
> +	data = lan743x_csr_read(adapter, PMT_CTL);
> +	data |= PMT_CTL_ETH_PHY_RST_;
> +	lan743x_csr_write(adapter, PMT_CTL, data);
> +	timeout = jiffies + HZ;
> +	do {
> +		if (time_after(jiffies, timeout))
> +			return -EIO;
> +		msleep(50);
> +		data = lan743x_csr_read(adapter, PMT_CTL);
> +	} while ((data & PMT_CTL_ETH_PHY_RST_) || !(data & PMT_CTL_READY_));

readx_poll_timeout()

> +	return 0;
> +}
> +

> +static void lan743x_phy_link_status_change(struct net_device *netdev)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	struct phy_device *phydev = netdev->phydev;
> +
> +	if (phydev) {
> +		if (phydev->state == PHY_RUNNING) {
> +			struct ethtool_link_ksettings ksettings;
> +			struct lan743x_phy *phy = NULL;
> +			int remote_advertisement = 0;
> +			int local_advertisement = 0;
> +
> +			phy = &adapter->phy;
> +			memset(&ksettings, 0, sizeof(ksettings));
> +			phy_ethtool_get_link_ksettings(netdev, &ksettings);
> +			local_advertisement = phy_read(phydev, MII_ADVERTISE);
> +			if (local_advertisement < 0)
> +				goto done;

A bit unusual to use a goto here, since this is not error handling, or
dealing with a mutex. Maybe just return;

> +			remote_advertisement = phy_read(phydev, MII_LPA);
> +			if (remote_advertisement < 0)
> +				goto done;
> +			netif_info(adapter, link, adapter->netdev,
> +				   "link UP: speed: %u duplex: %d anadv: 0x%04x anlpa: 0x%04x\n",
> +				   ksettings.base.speed, ksettings.base.duplex,
> +				   local_advertisement, remote_advertisement);

phy_print_status()

> +			lan743x_phy_update_flowcontrol(adapter,
> +						       ksettings.base.duplex,
> +						       local_advertisement,
> +						       remote_advertisement);
> +		} else if (phydev->state == PHY_NOLINK) {
> +			netif_info(adapter, link, adapter->netdev,
> +				   "link DOWN\n");

Here too.

> +		}
> +	}
> +done:
> +	return;
> +}
> +

> +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) {
> +		ret = -EIO;
> +		goto clean_up;
> +	}
> +	ret = phy_connect_direct(netdev, phydev,
> +				 lan743x_phy_link_status_change,
> +				 PHY_INTERFACE_MODE_GMII);
> +	if (ret) {
> +		ret = -EIO;

Why change the error code phy_connect_direct() returned?

> +		goto clean_up;
> +	}
> +	phy->flags |= PHY_FLAG_ATTACHED;
> +
> +	/* 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);
> +	phy->flags |= PHY_FLAG_OPENED;
> +	return 0;
> +clean_up:
> +	lan743x_phy_close(adapter);
> +	return ret;
> +}
> +

> +static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> +{
> +	int ret;
> +
> +	adapter->mdiobus = mdiobus_alloc();

devm_mdiobus_alloc() ?

> +	if (!(adapter->mdiobus)) {
> +		ret = -ENOMEM;
> +		goto clean_up;
> +	}
> +	adapter->init_flags |= LAN743X_INIT_FLAG_MDIOBUS_ALLOCATED;
> +	adapter->mdiobus->priv = (void *)adapter;
> +	adapter->mdiobus->read = lan743x_mdiobus_read;
> +	adapter->mdiobus->write = lan743x_mdiobus_write;
> +	adapter->mdiobus->name = "lan743x-mdiobus";
> +	snprintf(adapter->mdiobus->id, MII_BUS_ID_SIZE,
> +		 "pci-%s", pci_name(adapter->pci.pdev));
> +
> +	/* set to internal PHY id */
> +	adapter->mdiobus->phy_mask = ~(int)BIT(1);

phy_mask is a u32.

> +
> +	/* register mdiobus */
> +	ret = mdiobus_register(adapter->mdiobus);
> +	if (ret < 0)
> +		goto clean_up;
> +	adapter->init_flags |= LAN743X_INIT_FLAG_MDIOBUS_REGISTERED;
> +	return 0;
> +clean_up:
> +	if (adapter->mdiobus)
> +		mdiobus_free(adapter->mdiobus);
> +	adapter->init_flags &= ~(LAN743X_INIT_FLAG_MDIOBUS_REGISTERED |
> +				 LAN743X_INIT_FLAG_MDIOBUS_ALLOCATED);
> +	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 = alloc_etherdev(sizeof(struct lan743x_adapter));

devm_alloc_etherdev() ?

> +	if (!netdev)
> +		goto clean_up;
> +	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 clean_up;
> +	adapter->init_flags |= LAN743X_COMPONENT_FLAG_PCI;
> +	ret = lan743x_csr_init(adapter);
> +	if (ret)
> +		goto clean_up;
> +	adapter->init_flags |= LAN743X_COMPONENT_FLAG_CSR;
> +	ret = lan743x_hardware_init(adapter, pdev);
> +	if (ret)
> +		goto clean_up;
> +	ret = lan743x_mdiobus_init(adapter);
> +	if (ret)
> +		goto clean_up;
> +	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;
> +	ret = register_netdev(adapter->netdev);
> +	if (ret < 0)
> +		goto clean_up;
> +	adapter->init_flags |= LAN743X_INIT_FLAG_NETDEV_REGISTERED;
> +	netif_info(adapter, probe, adapter->netdev, "Probe succeeded\n");
> +	return 0;
> +
> +clean_up:
> +	if (adapter) {
> +		netif_warn(adapter, probe, adapter->netdev,
> +			   "Incomplete initialization, performing clean up\n");
> +		lan743x_full_cleanup(adapter);
> +	}
> +	return ret;
> +}

> +++ b/drivers/net/ethernet/microchip/lan743x_main.h

> +u32 lan743x_csr_read(struct lan743x_adapter *adapter, int offset);
> +void lan743x_csr_write(struct lan743x_adapter *adapter, int offset, u32 data);

Why are these needed?

> +
> +/* INTERRUPTS */
> +typedef void(*lan743x_vector_handler)(void *context, u32 int_sts, u32 flags);

And this.

    Andrew 

Powered by blists - more mailing lists