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 BEDCB45D7E; Fri, 22 Nov 2024 21:44:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 456CE40291; Fri, 22 Nov 2024 21:44:45 +0100 (CET) Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by mails.dpdk.org (Postfix) with ESMTP id 403024028C for ; Fri, 22 Nov 2024 21:44:43 +0100 (CET) Received: by mail-pf1-f173.google.com with SMTP id d2e1a72fcca58-71e681bc315so1852743b3a.0 for ; Fri, 22 Nov 2024 12:44:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1732308282; x=1732913082; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=mYAGlujPF+wkdul9IAfTtKvKeXZoSfk+Zelwa/UCivU=; b=fbqSZIzb2KvX5OXXCc6TeVLmaLJ5Hf8cYQ3IgtQNY0cHTQgJHF5TduA44SXUjGibuP dpm+BYftF/srQFUih2ozhISr7s9AcvwihsFos5TUqIT1g9xPUeD3ur9GPHjPpJ383go+ roGtwBPDXQizGYSmnwZlJFSIFflKd1KxchHCpM3b/wjJT3MLAJYqx5wfmxLR4+b85PHz JhGYsNSNHYDaqAXhhJk8neNlCfOApJGqkw86dlLtclBbCCe7Drlj3lfEKmA+akO4gVUG NLzAgeffyOUhU4H+Em1qXUSK9ypJwMcwAhZxEhMpY9shGsg/C2B/34kD9B3Hy4jo0f7w ddIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732308282; x=1732913082; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=mYAGlujPF+wkdul9IAfTtKvKeXZoSfk+Zelwa/UCivU=; b=LJ6mmyNzhFfLo1b3gbykTC4WP3shhFpuE/Xm/QsgZSQTMTY2C4KrSL90iox6zbRB1o mFHQGwgFCQbZ90syhv7GkBT5gxj1q3/fgflt9pZeNB7Iia6MbGsOStPwZdh1mCWjrQGa IL4YIwb/e1mCVCRDbyp4nHHMnDHBiRlI7PHjaq0RFNsyA3t2guoEkph7u1oqBfbOlhwP ra1XV7kSRKadjP9dn5INMVW84u819JtTM6WVIrVqqAHhigVftJACOXhKJEaq0RiQHKaq u56JTSEHLGdnOigQ8B2zvX1IjEo6keRSCwa/Tk5jmMHfWzUJRM7RnyZOJx6FReya0bNj nswg== X-Gm-Message-State: AOJu0Yw79UffXcOtihD+xypJZvEdvSpu0XWv7T+H0uJADHPBmnjt1xcp ou5lCWyXHh3vhn2SPuGk6c94W6vkdv30eUkkpUlIK5xyXrk6cAXIpDjgK1qIp8c= X-Gm-Gg: ASbGncsd50gY2l+MScdHY+ZPVRf37W+sWeBFrFPkuTj/pHUXWyH9snCyUFuRz7201Cw YWlfkK6iXXf8fDCaVvj8L3A4zkqkfsD4MZUKRNosv1zViWOb+cof4Z8pVr4hWnHalBoUiwB/JTH tA8Qck0NtYFjnZ9i1y7nbVbeI/MAUY//ipW38RhSrGXUnYJoEswMU0zOslQP/IS3vBPU3BQ1K9z LgkBnXu/gvWgj0MlYJlTMokWmAWBzYxFZ6/xE0tnodN2xHUu3/XNGeA5ZWh2O30E4doiHOB1fdj Sym8aBaj+5SR7HLZ2OD8f2YeH1c= X-Google-Smtp-Source: AGHT+IGT6usIttgKmqhIj7YJ+23p2dWgtA/Cgp7unkeuSTFOznLvA2QmDmQ0CdYR8TfnlkHiteZv2A== X-Received: by 2002:a17:902:d483:b0:20c:d71d:69c5 with SMTP id d9443c01a7336-2129fe088abmr66110605ad.4.1732308282313; Fri, 22 Nov 2024 12:44:42 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2129dc219d8sm20464465ad.241.2024.11.22.12.44.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Nov 2024 12:44:42 -0800 (PST) Date: Fri, 22 Nov 2024 12:44:40 -0800 From: Stephen Hemminger To: liwencheng Cc: dev@dpdk.org Subject: Re: [PATCH v2 1/3] net/macb: add new driver Message-ID: <20241122124440.281cac24@hermes.local> In-Reply-To: <1730796099-1235390-1-git-send-email-liwencheng@phytium.com.cn> References: <1730455640-1084345-1-git-send-email-liwencheng@phytium.com.cn> <1730796099-1235390-1-git-send-email-liwencheng@phytium.com.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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 Tue, 5 Nov 2024 08:41:38 +0000 liwencheng wrote: > + > +int genphy_read_status(struct phy_device *phydev) > +{ > + struct macb *bp = phydev->bp; > + uint16_t bmcr, bmsr, ctrl1000 = 0, stat1000 = 0; > + uint32_t advertising, lp_advertising; > + uint32_t nego; > + uint16_t phyad = phydev->phyad; > + > + /* Do a fake read */ > + bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR); > + > + bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR); > + bmcr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMCR); > + > + if (bmcr & BMCR_ANENABLE) { > + ctrl1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_CTRL1000); > + stat1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_STAT1000); > + > + advertising = ADVERTISED_Autoneg; > + advertising |= genphy_get_an(bp, phyad, GENERIC_PHY_ADVERISE); > + advertising |= genphy_ctrl1000_to_ethtool_adv_t(ctrl1000); > + > + if (bmsr & BMSR_ANEGCOMPLETE) { > + lp_advertising = genphy_get_an(bp, phyad, GENERIC_PHY_LPA); > + lp_advertising |= genphy_stat1000_to_ethtool_lpa_t(stat1000); > + } else { > + lp_advertising = 0; > + } > + > + nego = advertising & lp_advertising; > + if (nego & (ADVERTISED_1000baseT_Full | ADVERTISED_1000baseT_Half)) { > + phydev->speed = SPEED_1000; > + phydev->duplex = !!(nego & ADVERTISED_1000baseT_Full); > + } else if (nego & > + (ADVERTISED_100baseT_Full | ADVERTISED_100baseT_Half)) { > + phydev->speed = SPEED_100; > + phydev->duplex = !!(nego & ADVERTISED_100baseT_Full); > + } else { > + phydev->speed = SPEED_10; > + phydev->duplex = !!(nego & ADVERTISED_10baseT_Full); > + } > + } else { > + phydev->speed = ((bmcr & BMCR_SPEED1000 && (bmcr & BMCR_SPEED100) == 0) > + ? SPEED_1000 > + : ((bmcr & BMCR_SPEED100) ? SPEED_100 : SPEED_10)); > + phydev->duplex = (bmcr & BMCR_FULLDPLX) ? DUPLEX_FULL : DUPLEX_HALF; > + } > + > + return 0; > +} Always returns 0 can be void function? > +int macb_usxgmii_pcs_resume(struct phy_device *phydev) > +{ > + u32 config; > + struct macb *bp = phydev->bp; > + > + config = gem_readl(bp, USX_CONTROL); > + > + /* enable signal */ > + config &= ~(GEM_BIT(RX_SYNC_RESET)); > + config |= GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN); > + gem_writel(bp, USX_CONTROL, config); > + > + return 0; > +} Always returns 0 can be void function? > +int macb_usxgmii_pcs_suspend(struct phy_device *phydev) > +{ > + uint32_t config; > + struct macb *bp = phydev->bp; > + > + config = gem_readl(bp, USX_CONTROL); > + config |= GEM_BIT(RX_SYNC_RESET); > + /* disable signal */ > + config &= ~(GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN)); > + gem_writel(bp, USX_CONTROL, config); > + rte_delay_ms(1); > + return 0; > +} Always returns 0 should be void? > + > +int macb_usxgmii_pcs_check_for_link(struct phy_device *phydev) > +{ > + int value; > + int link; > + struct macb *bp = phydev->bp; > + value = gem_readl(bp, USX_STATUS); > + link = GEM_BFEXT(BLOCK_LOCK, value); > + return link; > +} The driver is sloppy in using int where unsigned value is possible. You lose precision doing that and are prone to sign extension bugs. Since gem_readl() is wrapper around macb_reg_readl() and that returns u32; this function should be returning u32 and value should be u32 The temporary variable value is not needed. > +int macb_gbe_pcs_check_for_link(struct phy_device *phydev) > +{ > + int value; > + int link; > + struct macb *bp = phydev->bp; > + > + value = macb_readl(bp, NSR); > + link = MACB_BFEXT(NSR_LINK, value); > + return link; > +}