Message-ID: <DM6PR11MB46103DEF82B2CFAA97FB9145F3FF2@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Mon, 3 Jun 2024 13:05:13 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Keller, Jacob E"
	<jacob.e.keller@...el.com>, "Wegrzyn, Stefan" <stefan.wegrzyn@...el.com>,
	"Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>, "Glaza, Jan"
	<jan.glaza@...el.com>
Subject: RE: [PATCH iwl-next v7 3/7] ixgbe: Add link management support for
 E610 device

>-----Original Message-----
>From: Simon Horman <horms@...nel.org> 
>Sent: Friday, May 31, 2024 4:54 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Keller, Jacob E <jacob.e.keller@...el.com>; Wegrzyn, Stefan <stefan.wegrzyn@...el.com>; Jagielski, Jedrzej <jedrzej.jagielski@...el.com>; Glaza, Jan <jan.glaza@...el.com>
>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 <stefan.wegrzyn@...el.com>
>> Signed-off-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
>> Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Reviewed-by: Jan Glaza <jan.glaza@...el.com>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>
>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;
>> +}
>
>...
>

Powered by blists - more mailing lists