>-----Original Message-----
>From: Simon Horman <[email protected]>
>Sent: Saturday, April 20, 2024 8:18 PM
>To: Kwapulinski, Piotr <[email protected]>
>Cc: [email protected]; [email protected]; Gomes, Vinicius
><[email protected]>; Wegrzyn, Stefan <[email protected]>;
>Jagielski, Jedrzej <[email protected]>; Sokolowski, Jan
><[email protected]>
>Subject: Re: [PATCH iwl-next v2 2/5] ixgbe: Add support for E610 device
>capabilities detection
>
>On Mon, Apr 15, 2024 at 12:34:32PM +0200, Piotr Kwapulinski wrote:
>> Add low level support for E610 device capabilities detection. The
>> capabilities are discovered via the Admin Command Interface. Discover
>> the following capabilities:
>> - function caps: vmdq, dcb, rss, rx/tx qs, msix, nvm, orom, reset
>> - device caps: vsi, fdir, 1588
>> - phy caps
>>
>> Co-developed-by: Stefan Wegrzyn <[email protected]>
>> Signed-off-by: Stefan Wegrzyn <[email protected]>
>> Co-developed-by: Jedrzej Jagielski <[email protected]>
>> Signed-off-by: Jedrzej Jagielski <[email protected]>
>> Reviewed-by: Jan Sokolowski <[email protected]>
>> Signed-off-by: Piotr Kwapulinski <[email protected]>
>
>Hi Pitor,
>
>A few minor nits from my side.
>No need to respin just because of these.
>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 517
>> ++++++++++++++++++ drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h |
>> 11 +
>> 2 files changed, 528 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>
>...
>
>> +/**
>> + * ixgbe_get_num_per_func - determine number of resources per PF
>> + * @hw: pointer to the HW structure
>> + * @max: value to be evenly split between each PF
>> + *
>> + * Determine the number of valid functions by going through the
>> +bitmap returned
>> + * from parsing capabilities and use this to calculate the number of
>> +resources
>> + * per PF based on the max value passed in.
>> + *
>> + * Return: the number of resources per PF or 0, if no PH are available.
>> + */
>> +static u32 ixgbe_get_num_per_func(struct ixgbe_hw *hw, u32 max) {
>> + const u32 IXGBE_CAPS_VALID_FUNCS_M = 0xFF;
>
>nit: Maybe this could simply be a #define?
Hello,
will do
>
>> + u8 funcs = hweight8(hw->dev_caps.common_cap.valid_functions &
>> + IXGBE_CAPS_VALID_FUNCS_M);
>
>nit: Please consider using reverse xmas tree order - longest line to shortest -
> for local variables in new Networking code
Will do
>
>> +
>> + return funcs ? (max / funcs) : 0;
>> +}
>
>...
>
>> +/**
>> + * ixgbe_aci_disable_rxen - disable RX
>> + * @hw: pointer to the HW struct
>> + *
>> + * Request a safe disable of Receive Enable using ACI command (0x000C).
>> + *
>> + * Return: the exit code of the operation.
>> + */
>> +int ixgbe_aci_disable_rxen(struct ixgbe_hw *hw) {
>> + struct ixgbe_aci_cmd_disable_rxen *cmd;
>> + struct ixgbe_aci_desc desc;
>> +
>> + cmd = &desc.params.disable_rxen;
>> +
>> + ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_disable_rxen);
>> +
>> + cmd->lport_num = (u8)hw->bus.func;
>
>nit: This cast seems unnecessary.
> AFAICT the type of hw->bus.func is u8.
Will do
>
>> +
>> + return ixgbe_aci_send_cmd(hw, &desc, NULL, 0); }
>
>...
Thank you for review
Piotr
