From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 31CE9A0560; Tue, 18 Oct 2022 14:54:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C99DA40395; Tue, 18 Oct 2022 14:54:15 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 076454021D for ; Tue, 18 Oct 2022 14:54:13 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666097653; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5cN/w6zeiU7SPMIdBsKqysUtc3oS1b5Ly9YEIWKBmDs=; b=CPhj7oTfVW53ODst1h6eMFtq030yUWDt4aTAlr8R3rRTww4GtZI/j7S0LXIibgMWfuo1L5 o+O6yedW0gIIeEPXnaf69il8kaDB+IxK4xT3Sq6GAvxHNPZ1xamqHdMkXLBi7feq41Z6pT GMxvrOqoiJhMHCmOQFrXDIOp4ZeN/jY= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-113-xv6HYC_jPGS3N70V258Ekg-1; Tue, 18 Oct 2022 08:54:12 -0400 X-MC-Unique: xv6HYC_jPGS3N70V258Ekg-1 Received: by mail-wr1-f72.google.com with SMTP id s30-20020adfa29e000000b002302b9671feso4599657wra.20 for ; Tue, 18 Oct 2022 05:54:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:references:cc:to:from :content-language:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5cN/w6zeiU7SPMIdBsKqysUtc3oS1b5Ly9YEIWKBmDs=; b=2p1/lZr5zlIGal/aULxjQEvS2hUcDwxGZsum74QIssBoMMxi2tdjQsPY369cq1n7mk 8KjJchwwLQ/RDO8Q3GCJkIDXOdI1bJSK6hpUhbKp37aF3YUD576sZ2BZN9zEUNoCNTvS FFxHpyxiRmznQYZglXVDyG4ei3vmoYWBihrWmi9CxxdSn2CSKLTI8LsvSCML4UoDDS0J ODNmgZY+W+YAq8hoFP/3hGG9i+zfy1MTpTpzUTqOhfFrS0kdtWUyQtei7mWh0qxPsiXr xNXhlXKs5FiEHtXTzHoDXUYdfwlUsivjS7oLDv62FoCwqydOZSFa0y9aHCikTsO7EC7C lf4w== X-Gm-Message-State: ACrzQf28xHOU7RhmC46dRh6H/KlsN+tEQFBEre4l0vvsY0qutdHJXtsr CKJk15SFD6Z/g6gyFPAuPLWOyAXm7FBB3RcfDhleCmOw5khAou+mCdH47tgFKwXa/nG8D963Qky FZnw= X-Received: by 2002:a05:6000:a11:b0:22e:3dab:537c with SMTP id co17-20020a0560000a1100b0022e3dab537cmr1877351wrb.638.1666097651283; Tue, 18 Oct 2022 05:54:11 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7dS3pngNWDHVZHGLqYLY5LkKLyMmteMmTTPOmcZtazCjaVXD8lav4SiV8A/BKHsUxywXH1IA== X-Received: by 2002:a05:6000:a11:b0:22e:3dab:537c with SMTP id co17-20020a0560000a1100b0022e3dab537cmr1877338wrb.638.1666097651090; Tue, 18 Oct 2022 05:54:11 -0700 (PDT) Received: from [192.168.0.36] ([78.19.110.164]) by smtp.gmail.com with ESMTPSA id l12-20020a05600c2ccc00b003c6f27d275dsm9893643wmc.33.2022.10.18.05.54.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Oct 2022 05:54:10 -0700 (PDT) Message-ID: <44a7bd84-ef82-1d69-b591-06c61f5e2ec7@redhat.com> Date: Tue, 18 Oct 2022 13:54:09 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 From: Kevin Traynor To: "Mah, Yock Gen" , IOTG DPDK Ref App , "Zhang, Qi Z" Cc: "dev@dpdk.org" References: <20220829081436.29945-1-iotg.dpdk.ref.app@intel.com> <20220831225139.78201-1-iotg.dpdk.ref.app@intel.com> <45165da1-25af-b735-f80d-a6b42592cf6c@redhat.com> <31c280bd-2d69-e302-8ec6-cbff21badd4a@redhat.com> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking In-Reply-To: <31c280bd-2d69-e302-8ec6-cbff21badd4a@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 12/10/2022 09:39, Kevin Traynor wrote: > On 12/10/2022 08:45, Mah, Yock Gen wrote: >> >> >> -----Original Message----- >> From: Kevin Traynor >> Sent: Tuesday, 11 October, 2022 7:05 PM >> To: IOTG DPDK Ref App ; Zhang, Qi Z >> Cc: dev@dpdk.org >> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking >> >> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote: >>> From: NSWE SWS DPDK Dev >>> >>> i225 devices have only one PHY vendor. There is unnecessary to check >>> _I_PHY_ID during the link establishment and auto-negotiation process, >>> the checking also caused devices like i225-IT failed. This patch is to >>> remove the mentioned unnecessary checking. >>> >>> Cc: stable@dpdk.org >>> Signed-off-by: NSWE SWS DPDK Dev >>> --- >>> drivers/net/igc/base/igc_i225.c | 15 ++------------- >>> drivers/net/igc/base/igc_phy.c | 6 ++---- >>> 2 files changed, 4 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/net/igc/base/igc_i225.c >>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 100644 >>> --- a/drivers/net/igc/base/igc_i225.c >>> +++ b/drivers/net/igc/base/igc_i225.c >>> @@ -173,19 +173,8 @@ static s32 igc_init_phy_params_i225(struct igc_hw *hw) >>> phy->ops.write_reg = igc_write_phy_reg_gpy; >>> >>> ret_val = igc_get_phy_id(hw); >>> - /* Verify phy id and set remaining function pointers */ >>> - switch (phy->id) { >>> - case I225_I_PHY_ID: >>> - case I226_LM_PHY_ID: >>> - phy->type = igc_phy_i225; >>> - phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225; >>> - phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225; >> >>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed. >> >>> - Why are they removed, were they not needed? >> >> >> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process. >> > > Right, that's clear about the vendor ID check. But it's not clear to me > why the the resulting code like this: > > phy->type = igc_phy_i225; > > and not like this: > > phy->type = igc_phy_i225; > phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225; > phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225; > > So it is using dummy null functions instead: > https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61 > > Do the device registers not need to be set anymore? > For main branch, it would be nice to have an answer to above. It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches. >>> - /* TODO - complete with GPY PHY information */ >>> - break; >>> - default: >>> - ret_val = -IGC_ERR_PHY; >>> - goto out; >>> - } >>> + phy->type = igc_phy_i225; >>> + >>> >>> out: >>> return ret_val; >>> diff --git a/drivers/net/igc/base/igc_phy.c >>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644 >>> --- a/drivers/net/igc/base/igc_phy.c >>> +++ b/drivers/net/igc/base/igc_phy.c >>> @@ -1474,8 +1474,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw) >>> return ret_val; >>> } >>> >>> - if ((phy->autoneg_mask & ADVERTISE_2500_FULL) && >>> - hw->phy.id == I225_I_PHY_ID) { >>> + if (phy->autoneg_mask & ADVERTISE_2500_FULL) { >>> /* Read the MULTI GBT AN Control Register - reg 7.32 */ >>> ret_val = phy->ops.read_reg(hw, (STANDARD_AN_REG_MASK << >>> MMD_DEVADDR_SHIFT) | >>> @@ -1615,8 +1614,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw) >>> ret_val = phy->ops.write_reg(hw, PHY_1000T_CTRL, >>> mii_1000t_ctrl_reg); >>> >>> - if ((phy->autoneg_mask & ADVERTISE_2500_FULL) && >>> - hw->phy.id == I225_I_PHY_ID) >>> + if (phy->autoneg_mask & ADVERTISE_2500_FULL) >>> ret_val = phy->ops.write_reg(hw, >>> (STANDARD_AN_REG_MASK << >>> MMD_DEVADDR_SHIFT) | >> >