>-----Original Message-----
>From: Simon Horman <[email protected]>
>Sent: Friday, May 31, 2024 4:54 PM
>To: Kwapulinski, Piotr <[email protected]>
>Cc: [email protected]; [email protected]; Keller, Jacob E
><[email protected]>; Wegrzyn, Stefan <[email protected]>;
>Jagielski, Jedrzej <[email protected]>; Glaza, Jan
><[email protected]>
>Subject: Re: [PATCH iwl-next v7 3/7] ixgbe: Add link management support for
>E610 device
>
>On Mon, May 27, 2024 at 05:10:19PM +0200, Piotr Kwapulinski wrote:
>> Add low level link management support for E610 device. Link management
>> operations are handled via the Admin Command Interface. Add the
>> following link management operations:
>> - get link capabilities
>> - set up link
>> - get media type
>> - get link status, link status events
>> - link power management
>>
>> 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 Glaza <[email protected]>
>> Signed-off-by: Piotr Kwapulinski <[email protected]>
>
>Hi Pitor, all,
>
>Some more minor feedback from my side.
>
>...
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>
>...
>
>> +/**
>> + * ixgbe_is_media_cage_present - check if media cage is present
>> + * @hw: pointer to the HW struct
>> + *
>> + * Identify presence of media cage using the ACI command (0x06E0).
>> + *
>> + * Return: true if media cage is present, else false. If no cage,
>> +then
>> + * media type is backplane or BASE-T.
>> + */
>> +static bool ixgbe_is_media_cage_present(struct ixgbe_hw *hw) {
>> + struct ixgbe_aci_cmd_get_link_topo *cmd;
>> + struct ixgbe_aci_desc desc;
>> +
>> + cmd = &desc.params.get_link_topo;
>> +
>> + ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_get_link_topo);
>> +
>> + cmd->addr.topo_params.node_type_ctx =
>> + FIELD_PREP(IXGBE_ACI_LINK_TOPO_NODE_CTX_M,
>> + IXGBE_ACI_LINK_TOPO_NODE_CTX_PORT);
>> +
>> + /* Set node type. */
>> + cmd->addr.topo_params.node_type_ctx |=
>> + (IXGBE_ACI_LINK_TOPO_NODE_TYPE_M &
>> + IXGBE_ACI_LINK_TOPO_NODE_TYPE_CAGE);
>
>nit: Can this also use FIELD_PREP?
Hello Simon
Will try to apply
>
>> +
>> + /* Node type cage can be used to determine if cage is present. If AQC
>> + * returns error (ENOENT), then no cage present. If no cage present then
>> + * connection type is backplane or BASE-T.
>> + */
>> + return ixgbe_aci_get_netlist_node(hw, cmd, NULL, NULL); }
>
>...
>
>> +/**
>> + * ixgbe_get_link_status - get status of the HW network link
>> + * @hw: pointer to the HW struct
>> + * @link_up: pointer to bool (true/false = linkup/linkdown)
>> + *
>> + * Variable link_up is true if link is up, false if link is down.
>> + * The variable link_up is invalid if status is non zero. As a
>> + * result of this call, link status reporting becomes enabled
>> + *
>> + * Return: the exit code of the operation.
>> + */
>> +int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up) {
>> + int err = 0;
>> +
>> + if (!hw || !link_up)
>> + return -EINVAL;
>> +
>> + if (hw->link.get_link_info) {
>> + err = ixgbe_update_link_info(hw);
>> + if (err)
>> + return err;
>> + }
>> +
>> + *link_up = hw->link.link_info.link_info & IXGBE_ACI_LINK_UP;
>> +
>> + return err;
>
>nit: If the function used "return 0" here,
> then there would be no need to initialise err to 0
> and the scope of err could be reduced to the if clause where
> ixgbe_update_link_info() is called.
Will do
>
>> +}
>
>...
>
>> +/**
>> + * ixgbe_fc_autoneg_e610 - Configure flow control
>> + * @hw: pointer to hardware structure
>> + *
>> + * Configure Flow Control.
>> + */
>> +void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw) {
>> + int err;
>> +
>> + /* Get current link err.
>> + * Current FC mode will be stored in the hw context.
>> + */
>> + err = ixgbe_aci_get_link_info(hw, false, NULL);
>> + if (err)
>> + goto out;
>> +
>> + /* Check if the link is up */
>> + if (!(hw->link.link_info.link_info & IXGBE_ACI_LINK_UP)) {
>> + err = -EIO;
>> + goto out;
>> + }
>> +
>> + /* Check if auto-negotiation has completed */
>> + if (!(hw->link.link_info.an_info & IXGBE_ACI_AN_COMPLETED)) {
>> + err = -EIO;
>> + goto out;
>> + }
>> +
>> +out:
>> + if (!err) {
>> + hw->fc.fc_was_autonegged = true;
>> + } else {
>> + hw->fc.fc_was_autonegged = false;
>> + hw->fc.current_mode = hw->fc.requested_mode;
>> + }
>> +}
>
>As out is only jumped to from error paths, and err is not returned, perhaps
>this is a bit nicer (completely untested!):
Will do
>
>void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw) {
> int err;
>
> /* Get current link err.
> * Current FC mode will be stored in the hw context.
> */
> err = ixgbe_aci_get_link_info(hw, false, NULL);
> if (err)
> goto no_autoneg;
>
> /* Check if the link is up */
> if (!(hw->link.link_info.link_info & IXGBE_ACI_LINK_UP))
> goto no_autoneg;
>
> /* Check if auto-negotiation has completed */
> if (!(hw->link.link_info.an_info & IXGBE_ACI_AN_COMPLETED))
> goto no_autoneg;
>
> hw->fc.fc_was_autonegged = true;
> return;
>
>no_autoneg:
> hw->fc.fc_was_autonegged = false;
> hw->fc.current_mode = hw->fc.requested_mode; }
>
>> +
>> +/**
>> + * ixgbe_disable_rx_e610 - Disable RX unit
>> + * @hw: pointer to hardware structure
>> + *
>> + * Disable RX DMA unit on E610 with use of ACI command (0x000C).
>> + *
>> + * Return: the exit code of the operation.
>> + */
>> +void ixgbe_disable_rx_e610(struct ixgbe_hw *hw) {
>> + u32 rxctrl = IXGBE_READ_REG(hw, IXGBE_RXCTRL);
>> +
>> + if (rxctrl & IXGBE_RXCTRL_RXEN) {
>
>FWIIW, this could be less indented using something like:
>
> if (!(rxctrl & IXGBE_RXCTRL_RXEN))
> return;
Will do
>
> ...
>
>> + u32 pfdtxgswc;
>> + int err;
>> +
>> + pfdtxgswc = IXGBE_READ_REG(hw, IXGBE_PFDTXGSWC);
>> + if (pfdtxgswc & IXGBE_PFDTXGSWC_VT_LBEN) {
>> + pfdtxgswc &= ~IXGBE_PFDTXGSWC_VT_LBEN;
>> + IXGBE_WRITE_REG(hw, IXGBE_PFDTXGSWC, pfdtxgswc);
>> + hw->mac.set_lben = true;
>> + } else {
>> + hw->mac.set_lben = false;
>> + }
>> +
>> + err = ixgbe_aci_disable_rxen(hw);
>> +
>> + /* If we fail - disable RX using register write */
>> + if (err) {
>> + rxctrl = IXGBE_READ_REG(hw, IXGBE_RXCTRL);
>> + if (rxctrl & IXGBE_RXCTRL_RXEN) {
>> + rxctrl &= ~IXGBE_RXCTRL_RXEN;
>> + IXGBE_WRITE_REG(hw, IXGBE_RXCTRL, rxctrl);
>> + }
>> + }
>> + }
>> +}
>
>...
>
>> +/**
>> + * ixgbe_setup_phy_link_e610 - Sets up firmware-controlled PHYs
>> + * @hw: pointer to hardware structure
>> + *
>> + * Set the parameters for the firmware-controlled PHYs.
>> + *
>> + * Return: the exit code of the operation.
>> + */
>> +int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw) {
>> + struct ixgbe_aci_cmd_get_phy_caps_data pcaps;
>> + struct ixgbe_aci_cmd_set_phy_cfg_data pcfg;
>> + int err;
>> +
>> + err = ixgbe_aci_get_link_info(hw, false, NULL);
>> + if (err)
>> + goto err;
>> +
>> + /* Set PHY Configuration only if media is available */
>> + if (!(hw->link.link_info.link_info & IXGBE_ACI_MEDIA_AVAILABLE)) {
>> + err = ixgbe_aci_set_link_restart_an(hw, false);
>> + if (err)
>> + goto err;
>> +
>> + return 0;
>> + }
>> +
>> + err = ixgbe_aci_get_phy_caps(hw, false, IXGBE_ACI_REPORT_DFLT_CFG,
>> + &pcaps);
>> + if (err)
>> + goto err;
>
>nit: The goto label only returns err.
> So IMHO it' slightly nicer to just do that here.
> And return 0 right at the end of the function.
Will do
Thank you for feedback
>
>> +
>> + ixgbe_copy_phy_caps_to_cfg(&pcaps, &pcfg);
>> +
>> + pcfg.caps |= IXGBE_ACI_PHY_ENA_LINK;
>> + pcfg.caps |= IXGBE_ACI_PHY_ENA_AUTO_LINK_UPDT;
>> +
>> + /* Set default PHY types for a given speed */
>> + pcfg.phy_type_low = 0;
>> + pcfg.phy_type_high = 0;
>> +
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_10_FULL) {
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10BASE_T;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10M_SGMII;
>> + }
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_100_FULL) {
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_100BASE_TX;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_100M_SGMII;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_100M_USXGMII;
>> + }
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_1GB_FULL) {
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_T;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_SX;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_LX;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_KX;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1G_SGMII;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_1G_USXGMII;
>> + }
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_2_5GB_FULL) {
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_T;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_X;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_KX;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_2500M_SGMII;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_2500M_USXGMII;
>> + }
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_5GB_FULL) {
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_5GBASE_T;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_5GBASE_KR;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_5G_USXGMII;
>> + }
>> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_10GB_FULL) {
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_T;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_DA;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_SR;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_LR;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_KR_CR1;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_AOC_ACC;
>> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_C2C;
>> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10G_USXGMII;
>> + }
>> +
>> + /* Mask the set values to avoid requesting unsupported link types */
>> + pcfg.phy_type_low &= pcaps.phy_type_low;
>> + pcfg.phy_type_high &= pcaps.phy_type_high;
>> +
>> + err = ixgbe_aci_set_phy_cfg(hw, &pcfg);
>> + if (err)
>> + goto err;
>> +
>> + /* Request link restart - without putting it down */
>> + err = ixgbe_aci_set_link_restart_an(hw, true);
>> + if (err)
>> + goto err;
>> +
>> +err:
>> + return err;
>> +}
>
>...
>
