On 4/3/2019 12:47 PM, Rosen Xu wrote:
> Add Intel FPGA Acceleration NIC IPN3KE representor of PMD driver.
>
> Signed-off-by: Rosen Xu <rosen...@intel.com>
> Signed-off-by: Andy Pei <andy....@intel.com>
> Signed-off-by: Dan Wei <dan....@intel.com>
<...>
> +static void
> +ipn3ke_rpst_dev_stop(__rte_unused struct rte_eth_dev *dev)
> +{
'dev' is used in this function, can drop '__rte_unused'
> + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev);
> + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev);
> + uint32_t val;
> +
> + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
> + } else {
Why having empty 'if' condition and put everything into 'else', why not revert
the check in if?
Similar usage in many places.
> + /* Disable the TX path */
Fix indentation please.
> + val = 1;
I assume this is the value to write to say disable Tx path, I wonder if there is
a define MACRO for it? If not does it make sense to have one?
<...>
> +/*
> + * Reset PF device only to re-initialize resources in PMD layer
> + */
> +static int
> +ipn3ke_rpst_dev_reset(struct rte_eth_dev *dev)
> +{
> + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev);
> + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev);
> + uint32_t val;
> +
> + if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
> + } else {
> + /* Disable the TX path */
> + val = 1;
> + val &= IPN3KE_MAC_TX_PACKET_CONTROL_MASK;
> + (*hw->f_mac_write)(hw,
> + val,
> + IPN3KE_MAC_TX_PACKET_CONTROL,
> + rpst->port_id,
> + 0);
> +
> + /* Disable the RX path */
> + val = 1;
> + val &= IPN3KE_MAC_RX_TRANSFER_CONTROL_MASK;
> + (*hw->f_mac_write)(hw,
> + val,
> + IPN3KE_MAC_RX_TRANSFER_CONTROL,
> + rpst->port_id,
> + 0);
> + }
Same exact block repeated third time now, and there is one more copy as 'val =
0', what do you think converting this into function which gets enable/disable as
argument so that 'val' also can be detail of that function.
<...>
> +static int
> +ipn3ke_rpst_xstats_get(__rte_unused struct rte_eth_dev *dev,
> + __rte_unused struct rte_eth_xstat *xstats,
> + __rte_unused unsigned int n)
> +{
> + return 0;
> +}
> +static int ipn3ke_rpst_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> + __rte_unused struct rte_eth_xstat_name *xstats_names,
> + __rte_unused unsigned int limit)
> +{
> + return 0;
> +}
Can you please apply same syntax to this function as others:
static int
....
Also can you please put a empty line between previous and this function.
<...>
> +static int
> +ipn3ke_rpst_link_check(struct ipn3ke_rpst *rpst)
> +{
> + struct ipn3ke_hw *hw;
> + struct rte_rawdev *rawdev;
> + struct rte_eth_link link;
> + struct rte_eth_dev *pf;
> +
> + if (rpst == NULL)
> + return -1;
> +
> + hw = rpst->hw;
> +
> + memset(&link, 0, sizeof(link));
> +
> + link.link_duplex = ETH_LINK_FULL_DUPLEX;
> + link.link_autoneg = !(rpst->ethdev->data->dev_conf.link_speeds &
> + ETH_LINK_SPEED_FIXED);
> +
> + rawdev = hw->rawdev;
> + ipn3ke_update_link(rawdev, rpst->port_id, &link);
> +
> + if (!rpst->ori_linfo.link_status &&
> + link.link_status) {
Better to put one more tab here, to make it easy to pick the condition part from
the indendent code.
<...>
> +
> +struct eth_dev_ops ipn3ke_rpst_dev_ops = {
This can be "static const"
> + .dev_infos_get = ipn3ke_rpst_dev_infos_get,
> +
> + .dev_configure = ipn3ke_rpst_dev_configure,
> + .dev_start = ipn3ke_rpst_dev_start,
> + .dev_stop = ipn3ke_rpst_dev_stop,
> + .dev_close = ipn3ke_rpst_dev_close,
> + .dev_reset = ipn3ke_rpst_dev_reset,
> +
> + .stats_get = ipn3ke_rpst_stats_get,
> + .xstats_get = ipn3ke_rpst_xstats_get,
> + .xstats_get_names = ipn3ke_rpst_xstats_get_names,
> + .stats_reset = ipn3ke_rpst_stats_reset,
> + .xstats_reset = ipn3ke_rpst_stats_reset,
> +
> + .filter_ctrl = ipn3ke_afu_filter_ctrl,
> +
> + .rx_queue_start = ipn3ke_rpst_rx_queue_start,
> + .rx_queue_stop = ipn3ke_rpst_rx_queue_stop,
> + .tx_queue_start = ipn3ke_rpst_tx_queue_start,
> + .tx_queue_stop = ipn3ke_rpst_tx_queue_stop,
> + .rx_queue_setup = ipn3ke_rpst_rx_queue_setup,
> + .rx_queue_release = ipn3ke_rpst_rx_queue_release,
> + .tx_queue_setup = ipn3ke_rpst_tx_queue_setup,
> + .tx_queue_release = ipn3ke_rpst_tx_queue_release,
> +
> + .dev_set_link_up = ipn3ke_rpst_dev_set_link_up,
> + .dev_set_link_down = ipn3ke_rpst_dev_set_link_down,
> + .link_update = ipn3ke_rpst_link_update,
> +
> + .promiscuous_enable = ipn3ke_rpst_promiscuous_enable,
> + .promiscuous_disable = ipn3ke_rpst_promiscuous_disable,
> + .allmulticast_enable = ipn3ke_rpst_allmulticast_enable,
> + .allmulticast_disable = ipn3ke_rpst_allmulticast_disable,
> + .mac_addr_set = ipn3ke_rpst_mac_addr_set,
> + .mtu_set = ipn3ke_rpst_mtu_set,
> +
> + .tm_ops_get = NULL,
No need to set NULL ones.
<...>
> +int
> +ipn3ke_rpst_init(struct rte_eth_dev *ethdev, void *init_params)
> +{
> + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(ethdev);
> + struct ipn3ke_rpst *representor_param =
> + (struct ipn3ke_rpst *)init_params;
> +
> + if (representor_param->port_id >= representor_param->hw->port_num)
> + return -ENODEV;
> +
> + rpst->ethdev = ethdev;
> + rpst->switch_domain_id = representor_param->switch_domain_id;
> + rpst->port_id = representor_param->port_id;
> + rpst->hw = representor_param->hw;
> + rpst->i40e_pf_eth = NULL;
> + rpst->i40e_pf_eth_port_id = 0xFFFF;
> +
> + ethdev->data->mac_addrs = rte_zmalloc("ipn3ke", ETHER_ADDR_LEN, 0);
> + if (!ethdev->data->mac_addrs) {
> + IPN3KE_AFU_PMD_ERR("Failed to "
> + "allocated memory for storing mac address");
> + return -ENODEV;
> + }
> +
> + /** representor shares the same driver as it's PF device */
> + /**
> + * ethdev->device->driver = rpst->hw->eth_dev->device->driver;
> + */
Please remove commented code.
