Re: [PATCH 1/7] net/txgbe: support OEM subsystem vendor ID

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 6/20/2022 8:55 AM, Jiawen Wu wrote:
> Add support for OEM subsystem vendor ID.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   doc/guides/rel_notes/release_22_07.rst |  4 +++
>   drivers/net/txgbe/base/txgbe_hw.c      | 37 ++++++++++++++++++++++++++
>   drivers/net/txgbe/base/txgbe_hw.h      |  2 ++
>   drivers/net/txgbe/txgbe_ethdev.c       | 12 +++++++++
>   4 files changed, 55 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
> index 6fc044edaa..96db85a707 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -167,6 +167,10 @@ New Features
>   
>     * Added support for yt8531s PHY.
>   
> +* **Updated Wangxun txgbe driver.**
> +
> +  * Added support for OEM subsystem vendor ID.
> +
>   * **Added Elliptic Curve Diffie-Hellman (ECDH) algorithm in cryptodev.**
>   
>     Added support for Elliptic Curve Diffie Hellman (ECDH) asymmetric
> diff --git a/drivers/net/txgbe/base/txgbe_hw.c b/drivers/net/txgbe/base/txgbe_hw.c
> index 6a045cba79..8acebf8b60 100644
> --- a/drivers/net/txgbe/base/txgbe_hw.c
> +++ b/drivers/net/txgbe/base/txgbe_hw.c
> @@ -2608,6 +2608,43 @@ s32 txgbe_prot_autoc_write_raptor(struct txgbe_hw *hw, bool locked, u64 autoc)
>   	return err;
>   }
>   
> +/* 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
> + */
> +u32 txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr)
> +{
> +	u32 cmd_val = 0;
> +	u32 i = 0;
> +

No need to initialize these variables.

> +	cmd_val = TXGBE_SPICMD_CMD(cmd) | TXGBE_SPICMD_CLK(3) | cmd_addr;
> +	wr32(hw, TXGBE_SPICMD, cmd_val);
> +
> +	for (i = 0; i < 10000; i++) {

May be good to have a macro for time, up to you
#defile TXGBE_100MS 100000
#define TXGBE_10US  10
for (delay = 0; delay < TXGBE_100MS; delay += TXGBE_10US) {
	...
	usec_delay(TXGBE_10US);
}

> +		if (rd32(hw, TXGBE_SPISTAT) & TXGBE_SPISTAT_OPDONE)
> +			break;
> +
> +		usec_delay(10);
> +	}
> +	if (i == 10000)
> +		return 1;
> +

May be good to comment function that it will return '1' on timeout 
(since some caller functions in the chain are checking explicit '0x1' 
return value.)

> +	return 0;
> +}
> +
> +u32 txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr)
> +{
> +	u32 status = 0;
> +

no initialization is required.

> +	status = txgbe_fmgr_cmd_op(hw, 1, addr);
> +	if (status) {
> +		DEBUGOUT("Read flash timeout.");
> +		return status;
> +	}
> +
> +	return rd32(hw, TXGBE_SPIDAT);
> +}
> +
>   /**
>    *  txgbe_init_ops_pf - Inits func ptrs and MAC type
>    *  @hw: pointer to hardware structure
> diff --git a/drivers/net/txgbe/base/txgbe_hw.h b/drivers/net/txgbe/base/txgbe_hw.h
> index fd2f7d784c..7031589f7c 100644
> --- a/drivers/net/txgbe/base/txgbe_hw.h
> +++ b/drivers/net/txgbe/base/txgbe_hw.h
> @@ -111,4 +111,6 @@ s32 txgbe_prot_autoc_read_raptor(struct txgbe_hw *hw, bool *locked, u64 *value);
>   s32 txgbe_prot_autoc_write_raptor(struct txgbe_hw *hw, bool locked, u64 value);
>   s32 txgbe_reinit_fdir_tables(struct txgbe_hw *hw);
>   bool txgbe_verify_lesm_fw_enabled_raptor(struct txgbe_hw *hw);
> +u32 txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr);
> +u32 txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr);
>   #endif /* _TXGBE_HW_H_ */
> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index f0994f028d..6fb91cdf07 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -552,6 +552,7 @@ eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
>   	const struct rte_memzone *mz;
>   	uint32_t ctrl_ext;
>   	uint16_t csum;
> +	u32 ssid = 0;

This is only used in 'else' block, you can move it to the context it is 
used, and again it seems not required to initialize.

>   	int err, i, ret;
>   
>   	PMD_INIT_FUNC_TRACE();
> @@ -594,6 +595,17 @@ eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
>   	/* Vendor and Device ID need to be set before init of shared code */
>   	hw->device_id = pci_dev->id.device_id;
>   	hw->vendor_id = pci_dev->id.vendor_id;
> +	if (pci_dev->id.subsystem_vendor_id == PCI_VENDOR_ID_WANGXUN) {
> +		hw->subsystem_device_id = pci_dev->id.subsystem_device_id;
> +	} else {
> +		ssid = txgbe_flash_read_dword(hw, 0xFFFDC);
> +		if (ssid == 0x1) {
> +			PMD_INIT_LOG(ERR,
> +				"Read of internal subsystem device id failed\n");
> +			return -ENODEV;
> +		}
> +		hw->subsystem_device_id = (u16)ssid >> 8 | (u16)ssid << 8;
> +	}
>   	hw->hw_addr = (void *)pci_dev->mem_resource[0].addr;
>   	hw->allow_unsupported_sfp = 1;
>