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 1FCC5C35FF3 for ; Mon, 17 Mar 2025 09:01:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type: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=bfRlKgxGL6xoBQwjzEteEvARnfiax3+DCGaa/aKYtl4=; b=CAPxAUEvnwJi1Bt/DQ2t4h/x5u SQX/h1SAXXXJNbV29eYYrxFjyizO34gX44W5Ixv8PmGZkOquVas+RcVO95wpiwOMpYNmylnvdcQGA iYvrMSErpEF9aN+TkO0Xd5b669BP6xlYPAU9qcsPpD4d8JBV47Oscc7SoX5fS/Cvame3BpafXOZfL 3E8SYuqIIHB7E11a13t2KG2B4q2tIiL/kQHghIxKuheyRrzZue/I/3OnnqgKLY3GztfG9lpOQOusP 8lq+E8JkBfw5IWF9bDsxmpJHausIzuJv7mNE8e5Y6diYG1T95ns+M2US4B6MNXOQa+uYNtOF5FpI/ eMXBMvkQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tu6LK-00000001qJd-1tDR; Mon, 17 Mar 2025 09:01:06 +0000 Received: from relay7-d.mail.gandi.net ([2001:4b98:dc4:8::227]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tu6EN-00000001p6R-0IzB for linux-arm-kernel@lists.infradead.org; Mon, 17 Mar 2025 08:53:56 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id F1BDB4322A; Mon, 17 Mar 2025 08:53:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1742201628; 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=bfRlKgxGL6xoBQwjzEteEvARnfiax3+DCGaa/aKYtl4=; b=YCLf6vjHy3sJd0CsYWO7OZuY4buzPPO5wZbJqQSWcK7t3GABHlfmRQJoZSWTOQ1Zo4nDFe NYa2spNHSoIbU9TSvlftuLGzRft0Je0neMbDbkCoecYvgT5g6mH1wVxwlbapeL7YmAXb4w MPNFZvAlD2FeYq76sJzV7+BFCkfKb1yqKttrnJSjuG4qrM9sahsSXdS8RrkqN62uL7q48x TxDbk5IbPoyVSdcqcmsgvDb/k+67UwIemiT+/XoKd9yB7GGn/8MClLzMnsyRqTuRGZOkOn +950TlKYZFaKSVhb0if6ZyGFfvW6CJmN999djpQRpdUWooKL2llQ04EhzHQ0fw== Date: Mon, 17 Mar 2025 09:53:33 +0100 From: Maxime Chevallier To: Jacky Chou Cc: , , , , , , , , , , , , , , , , Subject: Re: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600 Message-ID: <20250317095229.6f8754dd@fedora.home> In-Reply-To: <20250317025922.1526937-5-jacky_chou@aspeedtech.com> References: <20250317025922.1526937-1-jacky_chou@aspeedtech.com> <20250317025922.1526937-5-jacky_chou@aspeedtech.com> Organization: Bootlin X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-GND-State: clean X-GND-Score: -100 X-GND-Cause: gggruggvucftvghtrhhoucdtuddrgeefvddrtddtgddufeeltdekucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuifetpfffkfdpucggtfgfnhhsuhgsshgtrhhisggvnecuuegrihhlohhuthemuceftddunecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjughrpeffhffvvefukfgjfhhoofggtgfgsehtjeertdertddvnecuhfhrohhmpeforgigihhmvgcuvehhvghvrghllhhivghruceomhgrgihimhgvrdgthhgvvhgrlhhlihgvrhessghoohhtlhhinhdrtghomheqnecuggftrfgrthhtvghrnhepvdeihfelueevvdekueeghfefiefgheeigeduveeifedvueeuleevudfhledulefgnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecukfhppedvrgdtudemtggsudelmeekugegheemgeeltddtmeeiheeikeemvdelsgdumeelvghfheemvgektgejnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehinhgvthepvdgrtddumegtsgduleemkegugeehmeegledttdemieehieekmedvlegsudemlegvfhehmegvkegtjedphhgvlhhopehfvgguohhrrgdrhhhomhgvpdhmrghilhhfrhhomhepmhgrgihimhgvrdgthhgvvhgrlhhlihgvrhessghoohhtlhhinhdrtghomhdpnhgspghrtghpthhtohepudekpdhrtghpthhtohepjhgrtghkhigptghhohhusegrshhpvggvughtvggthhdrtghomhdprhgtphhtthhopegrnhgurhgvfidonhgvthguvghvsehluhhnnhdrtghhp dhrtghpthhtohepuggrvhgvmhesuggrvhgvmhhlohhfthdrnhgvthdprhgtphhtthhopegvughumhgriigvthesghhoohhglhgvrdgtohhmpdhrtghpthhtohepkhhusggrsehkvghrnhgvlhdrohhrghdprhgtphhtthhopehprggsvghnihesrhgvughhrghtrdgtohhmpdhrtghpthhtoheprhhosghhsehkvghrnhgvlhdrohhrghdprhgtphhtthhopehkrhiikhdoughtsehkvghrnhgvlhdrohhrgh X-GND-Sasl: maxime.chevallier@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250317_015355_264755_56D93F60 X-CRM114-Status: GOOD ( 25.95 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On Mon, 17 Mar 2025 10:59:22 +0800 Jacky Chou wrote: > Use rx-internal-delay-ps and tx-internal-delay-ps > properties to configue the RGMII delay into corresponding register of > scu. Currently, the ftgmac100 driver only configures on AST2600 and will > be by pass the other platforms. > The details are in faraday,ftgmac100.yaml. > > Signed-off-by: Jacky Chou > --- > drivers/net/ethernet/faraday/ftgmac100.c | 88 ++++++++++++++++++++++++ > drivers/net/ethernet/faraday/ftgmac100.h | 12 ++++ > 2 files changed, 100 insertions(+) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index 17ec35e75a65..ea2061488cba 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -27,6 +27,9 @@ > #include > #include > #include > +#include > +#include > +#include > > #include "ftgmac100.h" > > @@ -1812,6 +1815,88 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name) > return ret; > } > > +static void ftgmac100_set_internal_delay(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct net_device *netdev; > + struct ftgmac100 *priv; > + struct regmap *scu; > + u32 rgmii_tx_delay, rgmii_rx_delay; > + u32 dly_reg, tx_dly_mask, rx_dly_mask; > + int tx, rx; Please use the reverse christmas tree notation, sorting declarations by descending line length > + netdev = platform_get_drvdata(pdev); > + priv = netdev_priv(netdev); > + > + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay); > + rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay); > + > + if (of_device_is_compatible(np, "aspeed,ast2600-mac")) { > + /* According to mac base address to get mac index */ > + switch (priv->res->start) { > + case 0x1e660000: > + dly_reg = AST2600_MAC12_CLK_DLY; > + tx_dly_mask = AST2600_MAC1_TX_DLY; > + rx_dly_mask = AST2600_MAC1_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e680000: > + dly_reg = AST2600_MAC12_CLK_DLY; > + tx_dly_mask = AST2600_MAC2_TX_DLY; > + rx_dly_mask = AST2600_MAC2_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e670000: > + dly_reg = AST2600_MAC34_CLK_DLY; > + tx_dly_mask = AST2600_MAC3_TX_DLY; > + rx_dly_mask = AST2600_MAC3_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e690000: > + dly_reg = AST2600_MAC34_CLK_DLY; > + tx_dly_mask = AST2600_MAC4_TX_DLY; > + rx_dly_mask = AST2600_MAC4_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, rgmii_rx_delay); > + break; > + default: > + dev_warn(&pdev->dev, "Invalid mac base address"); > + return; There has to be a better way that directly looking up the base address. Maybe you need an extra DT property ? > + } > + } else { > + return; > + } > + > + scu = syscon_regmap_lookup_by_phandle(np, "scu"); > + if (IS_ERR(scu)) { > + dev_warn(&pdev->dev, "failed to map scu base"); > + return; > + } > + > + if (!tx) { > + /* Use tx-internal-delay-ps as index to configure tx delay > + * into scu register. > + */ So this goes completely against the naming of the property. It has the -ps suffix, so you would expect to have picoseconds values passed, and not an arbiraty index. Take a look at other drivers, you should accept picseconds values from these properties, then compute the relevant index in the driver. That index should be something internal to your driver. An example here : https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c#L51 > + if (rgmii_tx_delay > 64) > + dev_warn(&pdev->dev, "Get invalid tx delay value"); > + else > + regmap_update_bits(scu, dly_reg, tx_dly_mask, rgmii_tx_delay); > + } > + > + if (!rx) { > + /* Use rx-internal-delay-ps as index to configure rx delay > + * into scu register. > + */ > + if (rgmii_tx_delay > 64) > + dev_warn(&pdev->dev, "Get invalid rx delay value"); > + else > + regmap_update_bits(scu, dly_reg, rx_dly_mask, rgmii_rx_delay); > + } > +} > + > static int ftgmac100_probe(struct platform_device *pdev) > { > struct resource *res; > @@ -1977,6 +2062,9 @@ static int ftgmac100_probe(struct platform_device *pdev) > if (of_device_is_compatible(np, "aspeed,ast2600-mac")) > iowrite32(FTGMAC100_TM_DEFAULT, > priv->base + FTGMAC100_OFFSET_TM); > + > + /* Configure RGMII delay if there are the corresponding properties */ > + ftgmac100_set_internal_delay(pdev); > } > > /* Default ring sizes */ > diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h > index 4968f6f0bdbc..d464d287502c 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.h > +++ b/drivers/net/ethernet/faraday/ftgmac100.h > @@ -271,4 +271,16 @@ struct ftgmac100_rxdes { > #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR (1 << 26) > #define FTGMAC100_RXDES1_IP_CHKSUM_ERR (1 << 27) > > +/* Aspeed SCU */ > +#define AST2600_MAC12_CLK_DLY 0x340 > +#define AST2600_MAC1_TX_DLY GENMASK(5, 0) > +#define AST2600_MAC1_RX_DLY GENMASK(17, 12) > +#define AST2600_MAC2_TX_DLY GENMASK(11, 6) > +#define AST2600_MAC2_RX_DLY GENMASK(23, 18) > +#define AST2600_MAC34_CLK_DLY 0x350 > +#define AST2600_MAC3_TX_DLY AST2600_MAC1_TX_DLY > +#define AST2600_MAC3_RX_DLY AST2600_MAC1_RX_DLY > +#define AST2600_MAC4_TX_DLY AST2600_MAC2_TX_DLY > +#define AST2600_MAC4_RX_DLY AST2600_MAC2_RX_DLY > + > #endif /* __FTGMAC100_H */ Maxime