From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1DEE2C04AA5 for ; Thu, 25 Aug 2022 12:35:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+/bMzZ0CaiMRX7B9dL/WIedMZ3e9U5R2yvJv/dSJ19M=; b=Viwm2pHP1xy7Av etZ+0TH2nSZRmvoPEY1en9jTvIL6evJIutTgK61VnwpRsF3IQVr8gqo/HS+eUKFJKsIv9Jx5UT8Dc uOoeqqXs26YnaUtDkHCaZ/2XPg2Y+h+AsVR4T4SzVk0UkPf51YXZgAa+GpC3Gc2iMMyyPkF57EuWl kRSNBuUpJ8JNE7JHSnrYzw694wNleM83DgoFPuwaidW1d4W6kcKevP8SSabt5oF9QV5T/Q4QVSZdH FUOp2eUH5XE1J+w/rivoCPjpZMJyVZKfP3R8++iCcUs3x/hPDSTgl9gpTSj0Meydp81JiG8hefOR4 nCpS4A4If0ED5dhw+HUg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oRC3o-00DjJo-1u; Thu, 25 Aug 2022 12:34:12 +0000 Received: from relay3-d.mail.gandi.net ([217.70.183.195]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oRC3j-00Dj9z-MP for linux-arm-kernel@lists.infradead.org; Thu, 25 Aug 2022 12:34:10 +0000 Received: (Authenticated sender: maxime.chevallier@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 2F66B60009; Thu, 25 Aug 2022 12:33:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1661430841; 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=a0zpixNx2moc4UmuE0XK44Om71XCEqAKsnZH44MUnsU=; b=OhIh7lw6Td3ts9utYrzkiL4bYZjhRvcOeJNk3S8Ki5tQ2/rZBcpIJU3eoxfOS5RnmbGddn M1XDe2idXfywORCA8CCQN7rIG2SDALhpxa9xrxBva0YU9KrIYhsrLJSLBWWjgSkEQB4H+h zOBROxxttSprCA1bwoHDIz9tLf0tkxvlrRumjWZbthHVzRG7HhvVJqp3WQBP4q4EkMaxVu /7X7od38ylOeBVapY9PrrCyJMPhMulMnVxIYJEhLLXd5gIHpQoCfqPHQwrO3AAIHYvi4CO 8zc/ie+9owpRn4Xf1FmifUV8Lx7NmF6H3FbauxTmZWa14csHmoeG7JgnjhVamg== Date: Thu, 25 Aug 2022 14:33:48 +0200 From: Maxime Chevallier To: "Russell King (Oracle)" Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn , Jakub Kicinski , Eric Dumazet , Paolo Abeni , Florian Fainelli , Heiner Kallweit , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH net-next 2/2] net: altera: tse: convert to phylink Message-ID: <20220825143348.662279d6@pc-10.home> In-Reply-To: References: <20220823140517.3091239-1-maxime.chevallier@bootlin.com> <20220823140517.3091239-3-maxime.chevallier@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220825_053408_345263_3A0FD462 X-CRM114-Status: GOOD ( 29.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello Russell, On Tue, 23 Aug 2022 18:59:05 +0100 "Russell King (Oracle)" wrote: > On Tue, Aug 23, 2022 at 04:05:17PM +0200, Maxime Chevallier wrote: > > This commit converts the Altera Triple Speed Ethernet Controller to > > phylink. This controller supports MII, GMII and RGMII with its MAC, > > and SGMII + 1000BaseX through a small embedded PCS. > > > > The PCS itself has a register set very similar to what is found in a > > typical 802.3 ethernet PHY, but this register set memory-mapped > > instead of lying on an mdio bus. > > > > Signed-off-by: Maxime Chevallier > > This needs some work. Looks like it, thanks for the review. From what you said, and after some more testing and digging, it looks like the TSE PCS can be used as a standalone IP that can be plugged to other macs by putting it in the PL part of some socfpga platforms. Given this and your review, I think I'll resubmit this with a proper PCS driver for the TSE PCS, if that makes sense. Thanks for the pointers, Maxime > > +static void alt_tse_mac_link_state(struct phylink_config *config, > > + struct phylink_link_state > > *state) +{ > > + struct net_device *ndev = to_net_dev(config->dev); > > + struct altera_tse_private *priv = netdev_priv(ndev); > > + > > + u16 bmsr, lpa; > > + > > + bmsr = sgmii_pcs_read(priv, MII_BMSR); > > + lpa = sgmii_pcs_read(priv, MII_LPA); > > + > > + phylink_mii_c22_pcs_decode_state(state, bmsr, lpa); > > +} > > + > > +static void alt_tse_mac_an_restart(struct phylink_config *config) > > +{ > > + struct net_device *ndev = to_net_dev(config->dev); > > + struct altera_tse_private *priv = netdev_priv(ndev); > > + u16 bmcr; > > + > > + bmcr = sgmii_pcs_read(priv, MII_BMCR); > > + bmcr |= BMCR_ANRESTART; > > + sgmii_pcs_write(priv, MII_BMCR, bmcr); > > +} > > + > > +static void alt_tse_pcs_config(struct net_device *ndev, > > + const struct phylink_link_state > > *state) +{ > > + struct altera_tse_private *priv = netdev_priv(ndev); > > + u32 ctrl, if_mode; > > + > > + if (state->interface != PHY_INTERFACE_MODE_SGMII && > > + state->interface != PHY_INTERFACE_MODE_1000BASEX) > > + return; > > + > > + ctrl = sgmii_pcs_read(priv, MII_BMCR); > > + if_mode = sgmii_pcs_read(priv, SGMII_PCS_IF_MODE); > > + > > + /* Set link timer to 1.6ms, as per the MegaCore Function > > User Guide */ > > + sgmii_pcs_write(priv, SGMII_PCS_LINK_TIMER_0, 0x0D40); > > + sgmii_pcs_write(priv, SGMII_PCS_LINK_TIMER_1, 0x03); > > + > > + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > > + if_mode |= PCS_IF_MODE_USE_SGMII_AN | > > PCS_IF_MODE_SGMII_ENA; > > + } else if (state->interface == > > PHY_INTERFACE_MODE_1000BASEX) { > > + if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN | > > PCS_IF_MODE_SGMII_ENA); > > + if_mode |= PCS_IF_MODE_SGMI_SPEED_1000; > > + } > > + > > + ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE); > > + > > + sgmii_pcs_write(priv, MII_BMCR, ctrl); > > + sgmii_pcs_write(priv, SGMII_PCS_IF_MODE, if_mode); > > + > > + sgmii_pcs_reset(priv); > > +} > > These look like they can be plugged directly into the phylink_pcs > support - please use that in preference to bolting it ino the MAC > ops - as every other ethernet driver (with the exception of > mtk_eth_soc) does today. > > > + > > +static void alt_tse_mac_config(struct phylink_config *config, > > unsigned int mode, > > + const struct phylink_link_state > > *state) +{ > > + struct net_device *ndev = to_net_dev(config->dev); > > + struct altera_tse_private *priv = netdev_priv(ndev); > > + u32 ctrl; > > + > > + ctrl = csrrd32(priv->mac_dev, tse_csroffs(command_config)); > > + ctrl &= ~(MAC_CMDCFG_ENA_10 | MAC_CMDCFG_ETH_SPEED | > > MAC_CMDCFG_HD_ENA); + > > + if (state->duplex == DUPLEX_HALF) > > + ctrl |= MAC_CMDCFG_HD_ENA; > > Using state->duplex in mac_config has always been a problem, it's not > well defined, and there are paths through phylink where state->duplex > does not reflect the state of the link when this function is called. > This is why it's always been clearly documented that this shall not be > used in mac_config. > > > + > > + if (state->speed == SPEED_1000) > > + ctrl |= MAC_CMDCFG_ETH_SPEED; > > + else if (state->speed == SPEED_10) > > + ctrl |= MAC_CMDCFG_ENA_10; > > Using state->speed brings with it the same problems as state->duplex. > > Please instead use mac_link_up() (and pcs_link_up()) - which are the > only callbacks from phylink that you can be sure give you the > speed, duplex and pause settings for the link. > > Thanks. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel