Re: [PATCH net-next v2 01/16] net: txgbe: Store PCI info

[Andrew Lunn <andrew@lunn.ch>]

> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_dummy.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2015 - 2022 Beijing WangXun Technology Co., Ltd. */
> +
> +#ifndef _TXGBE_DUMMY_H_
> +#define _TXGBE_DUMMY_H_
> +
> +#ifdef TUP
> +#elif defined(__GNUC__)
> +  #define TUP(x) x##_unused __always_unused
> +#elif defined(__LCLINT__)
> +  #define TUP(x) x /*@unused@*/
> +#else
> +  #define TUP(x) x
> +#endif /*TUP*/
> +#define TUP0 TUP(p0)
> +#define TUP1 TUP(p1)
> +#define TUP2 TUP(p2)
> +#define TUP3 TUP(p3)
> +#define TUP4 TUP(p4)

Please avoid macro magic, unless really needed.

> +
> +/* struct txgbe_mac_operations */
> +static void txgbe_bus_set_lan_id_dummy(struct txgbe_hw *TUP0)
> +{
> +}

Seems pointless, so just remove this function, and then you don't need
the macro magic.

> +
> +static void txgbe_init_ops_dummy(struct txgbe_hw *hw)
> +{
> +	struct txgbe_mac_info *mac = &hw->mac;
> +
> +	/* MAC */
> +	mac->ops.set_lan_id = txgbe_bus_set_lan_id_dummy;

Just set it to NULL. A user can first check if its NULL, and not call
it, or return -EOPNOTSUPP etc.


> +/* cmd_addr is used for some special command:
> + * 1. to be sector address, when implemented erase sector command
> + * 2. to be flash address when implemented read, write flash address
> + */
> +int txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr)
> +{
> +	u32 cmd_val = 0, val = 0;
> +
> +	cmd_val = (cmd << SPI_CLK_CMD_OFFSET) |
> +		  (SPI_CLK_DIV << SPI_CLK_DIV_OFFSET) | cmd_addr;
> +	wr32(hw, SPI_H_CMD_REG_ADDR, cmd_val);
> +
> +	return read_poll_timeout(rd32, val, (val & 0x1), 10, SPI_TIME_OUT_VALUE,
> +				 false, hw, SPI_H_STA_REG_ADDR);
> +}
> +

> +int txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr, u32 *data)
> +{
> +	int ret = 0;
> +
> +	ret = txgbe_fmgr_cmd_op(hw, SPI_CMD_READ_DWORD, addr);
> +	if (ret == -ETIMEDOUT)
> +		return ret;

Are you absolutely sure it will never return any other error code?
The pattern in the kernel is

> +	if (ret)
> +		return ret;

or

> +	if (ret < 0)
> +		return ret;

> +int txgbe_check_flash_load(struct txgbe_hw *hw, u32 check_bit)
> +{
> +	u32 i = 0, reg = 0;
> +	int err = 0;
> +
> +	/* if there's flash existing */
> +	if (!(rd32(hw, TXGBE_SPI_STATUS) &
> +	      TXGBE_SPI_STATUS_FLASH_BYPASS)) {
> +		/* wait hw load flash done */
> +		for (i = 0; i < TXGBE_MAX_FLASH_LOAD_POLL_TIME; i++) {
> +			reg = rd32(hw, TXGBE_SPI_ILDR_STATUS);
> +			if (!(reg & check_bit)) {
> +				/* done */
> +				break;
> +			}
> +			msleep(200);
> +		}

This is what iopoll.h is for. Any sort of loop waiting for something
to happen should use one of the helpers in there.

> +		if (i == TXGBE_MAX_FLASH_LOAD_POLL_TIME) {
> +			err = -ETIMEDOUT;
> +			txgbe_info(hw, "Check flash load timeout.\n");

Either dev_err() if this is fatal, or dev_dbg() if it is
not. dev_info() is not used very often, maybe just in your probe
function when you have found your hardware.

> +static int txgbe_sw_init(struct txgbe_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct txgbe_hw *hw = &adapter->hw;
> +	u32 ssid = 0;
> +	int err = 0;
> +
> +	/* PCI config space info */
> +	hw->vendor_id = pdev->vendor;
> +	hw->device_id = pdev->device;
> +	hw->revision_id = pdev->revision;
> +	hw->oem_svid = pdev->subsystem_vendor;
> +	hw->oem_ssid = pdev->subsystem_device;
> +
> +	if (hw->oem_svid == PCI_VENDOR_ID_WANGXUN) {
> +		hw->subsystem_vendor_id = pdev->subsystem_vendor;
> +		hw->subsystem_device_id = pdev->subsystem_device;
> +	} else {
> +		err = txgbe_flash_read_dword(hw, 0xfffdc, &ssid);
> +		if (err == -ETIMEDOUT) {
> +			netif_err(adapter, probe, adapter->netdev,
> +				  "read of internal subsystem device id failed\n");
> +			return -ENODEV;

Don't turn an -ETIMEDOUT into an -ENODEV.  In general, never convert
from one error code to another, unless you have a really good reason,
and if you do, comment why. -ETIMEDOUT is useful information, it helps
narrow down the problem.

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> index b2e329f50bae..3b1dd104373b 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> @@ -4,7 +4,9 @@
>  #ifndef _TXGBE_TYPE_H_
>  #define _TXGBE_TYPE_H_
>  
> +#include <linux/pci.h>
>  #include <linux/types.h>
> +#include <linux/iopoll.h>
>  #include <linux/netdevice.h>

These should be in the .c file. Avoid them in header files, since you
will include more headers than actually needed, slowing down the
build. A lot of time is spent in the build processing useless
headers. Please don't make it worse.

>  
>  /************ txgbe_register.h ************/
> @@ -54,4 +56,153 @@
>  /* Revision ID */
>  #define TXGBE_SP_MPW  1
>  
> +/**************** Global Registers ****************************/
> +/* chip control Registers */
> +#define TXGBE_MIS_RST                   0x1000C
> +#define TXGBE_MIS_PWR                   0x10000
> +#define TXGBE_MIS_CTL                   0x10004
> +#define TXGBE_MIS_PF_SM                 0x10008
> +#define TXGBE_MIS_PRB_CTL               0x10010
> +#define TXGBE_MIS_ST                    0x10028
> +#define TXGBE_MIS_SWSM                  0x1002C
> +#define TXGBE_MIS_RST_ST                0x10030
> +
> +#define TXGBE_MIS_RST_SW_RST            0x00000001U
> +#define TXGBE_MIS_RST_LAN0_RST          0x00000002U
> +#define TXGBE_MIS_RST_LAN1_RST          0x00000004U
> +#define TXGBE_MIS_RST_LAN0_CHG_ETH_MODE 0x20000000U
> +#define TXGBE_MIS_RST_LAN1_CHG_ETH_MODE 0x40000000U
> +#define TXGBE_MIS_RST_GLOBAL_RST        0x80000000U

Generally that would be BIT(0), BIT(1), BIT(2), BIT(29), BIT(30),
BIT(31).  I find this easier to read, especially when it is bit(21),
somewhere in the middle.

Please consider changing all the #defines that are for bits.

       Andrew