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.whiteh...@microchip.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
