[dpdk-dev] [PATCH v1 1/3] drivers/net/i40e: Add ethdev functions
Ananyev, Konstantin
konstantin.ananyev at intel.com
Wed Feb 24 11:43:50 CET 2016
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Wednesday, February 24, 2016 10:32 AM
> To: Zhang, Helin; Xie, Huawei
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] drivers/net/i40e: Add ethdev functions
>
> Comments inline.
>
> ..Remy
>
> On 23/02/2016 02:06, Zhang, Helin wrote:
> >
> >> +static inline int
> >> +i40e_read_regs(struct i40e_hw *hw, const struct reg_info *reg,
> >> + uint32_t *reg_buf)
> >> +{
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < reg->count; i++)
> >> + reg_buf[i] = I40E_READ_REG(hw,
> >> + reg->base_addr + i * reg->stride);
> >> + return reg->count;
> >> +}
> > From FVL5, some registers should be read by AQ commands, otherwise
> it may fail to
> > read without any warning.
> > Please see my patches of which registers should be read by AQ commands.
> > Please check i40e_osdep.h from below link. Thanks!
> > http://www.dpdk.org/dev/patchwork/patch/10654/
>
> Ok - will change for v2.
>
> I noticed that other patches in the same patchset expose extra registers
> - are these new or were they simply not exposed previously?
>
>
> >> + /* Only support doing full dump */
> >> + if (regs->offset != 0 && 0)
> > '&& 0' means it will never be false, right?
> > Anything wrong here?
>
> Oops - some dead code that slipped through.. :)
>
>
> >> + return -ENOTSUP;
> > A message before this return to tell the uers what happened would be
> better.
>
> Will add these into v2.
>
>
> >> +static int i40e_get_eeprom_length(__rte_unused struct rte_eth_dev *dev)
> > Why needs __rte_unused?
>
> Good point - surprised the compiler did not complain about them, as they
> are not supposed to be there..
>
>
> >> +static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
> >> + struct ether_addr *mac_addr)
> >> +{
> >> + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >>> dev_private);
> >> +
> >> + /* Flags: 0x3 updates port address */
> >> + i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes,
> >> NULL); }
> > Checks are needed before writing the MAC address.
>
> Will look into this.
>
>
> >> +struct reg_info {
> >> + uint32_t base_addr;
> >> + uint32_t count;
> >> + uint32_t stride;
> >> + const char *name;
> >> +} reg_info;
> > I think array definition shouldn't be added into a header file,
> otherwise any .c source
> > file which includes that header file will define that.
>
> Since it is quite a large table I think this approach, which is also
> used in ixgbe, is the lesser of evils. i40e_ethdev.c itself is already
> pretty big, and would prefer to avoid giving a driver-specific table
> non-static visibility until it actually has to be used from other
> compilation units.
Why not to have a separate .h file, specially for registers table definition?
More information about the dev
mailing list