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 X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69BB8C48BCD for ; Thu, 10 Jun 2021 03:23:57 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 350F561421 for ; Thu, 10 Jun 2021 03:23:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 350F561421 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lunn.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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:In-Reply-To:MIME-Version:References: 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=/yBYExOb0FAcCdCAQ9Sd+pffwjxnUVFRVLC+ZuXsLDQ=; b=qKFqqaufHQWuzc oywAr5B3uao15RF5Hl5Ic5eP/+IEnrWwa2LJ9U6vc+3WJ4q4Q8dShTkZw/OqW8MQKTGhRHw0Rx7U8 erT2whWQ39vgKWMVRev+wYu/PPBi3P2f9SSwpz5TYEnHOmiQ3fsiNTkiQQMU2NZyds0kt0tUhylP/ sA7yH2iSb9dg3zKrfiCPV6xSApxDmrGhl8IULmOsr8Sd1fbu+NSY6CqjCvULJMyr8NK23n7e6JULB qMRwArX/FrBtN3wyyAUOwr7V/UTpt/oSwfSKLGMhIr6mGVuxmBBYNgcbzkz0+e4x7Nx6921+yjvvI auPjNS3J0QD6aegSd3lw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lrBEd-00Gddb-9W; Thu, 10 Jun 2021 03:19:59 +0000 Received: from vps0.lunn.ch ([185.16.172.187]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lrBEZ-00GddH-I9 for linux-arm-kernel@lists.infradead.org; Thu, 10 Jun 2021 03:19:57 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=jWmr2xlRwn6JQaehy4GwAbYBW8L4D3ESDbeB+eyjFlI=; b=hhiJjKT6WeE4zxbXF0IMfq/cIp 4f29LBeN15gfX1AVIvAng+bg3qf9bFv4B9FvdCvxTnIDHot3hU6fdyvFxCB+L1skt3f3wTRAu4Y/K XQ44pUFutdRWIB7WN+v7catNdKHEgF0mCEaMjZLPh97X6hBkFGOR19LeXsxGtklpHdko=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1lrBE2-008axE-Ng; Thu, 10 Jun 2021 05:19:22 +0200 Date: Thu, 10 Jun 2021 05:19:22 +0200 From: Andrew Lunn To: =?utf-8?B?5ZGo55Cw5p2wIChaaG91IFlhbmppZSk=?= Cc: davem@davemloft.net, kuba@kernel.org, robh+dt@kernel.org, peppe.cavallaro@st.com, alexandre.torgue@foss.st.com, joabreu@synopsys.com, mcoquelin.stm32@gmail.com, linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, dongsheng.qiu@ingenic.com, aric.pzqi@ingenic.com, rick.tyliu@ingenic.com, sihui.liu@ingenic.com, jun.jiang@ingenic.com, sernia.zhou@foxmail.com, paul@crapouillou.net Subject: Re: [PATCH v2 2/2] net: stmmac: Add Ingenic SoCs MAC support. Message-ID: References: <1623260110-25842-1-git-send-email-zhouyanjie@wanyeetech.com> <1623260110-25842-3-git-send-email-zhouyanjie@wanyeetech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1623260110-25842-3-git-send-email-zhouyanjie@wanyeetech.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210609_201955_632355_5CB5E5AE X-CRM114-Status: GOOD ( 24.49 ) 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 > +static int jz4775_mac_set_mode(struct plat_stmmacenet_data *plat_dat) > +{ > + struct ingenic_mac *mac = plat_dat->bsp_priv; > + unsigned int val; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_ID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + case PHY_INTERFACE_MODE_RGMII_TXID: > + val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) | > + FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII); > + dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n"); > + break; So this does what DT writes expect. They put 'rgmii-id' as phy mode. The MAC does not add a delay. PHY_INTERFACE_MODE_RGMII_ID is passed to the PHY and it adds the delay. And frames flow to/from the PHY and users are happy. The majority of MAC drivers are like this. > +static int x2000_mac_set_mode(struct plat_stmmacenet_data *plat_dat) > +{ > + struct ingenic_mac *mac = plat_dat->bsp_priv; > + unsigned int val; Here we have a complete different story. > + case PHY_INTERFACE_MODE_RGMII: > + val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII); > + > + if (mac->tx_delay == 0) { > + val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN); > + } else { > + val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY); > + > + if (mac->tx_delay > MACPHYC_TX_DELAY_MAX) > + val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1); > + else > + val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1); > + } What are the units of tx_delay. The DT binding should be pS, and you need to convert from that to whatever the hardware is using. If mac->tx_delay is greater than MACPHYC_TX_DELAY_MAX, please return -EINVAL when parsing the binding. We want the DT writer to know they have requested something the hardware cannot do. So if the device tree contains 'rgmii' for PHY mode, you can use this for when you have long clock lines on your board adding the delay, and you just need to fine tune the delay, add a few pS. The PHY will also not add a delay, due to receiving PHY_INTERFACE_MODE_RGMII. > + > + if (mac->rx_delay == 0) { > + val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN); > + } else { > + val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY); > + > + if (mac->rx_delay > MACPHYC_RX_DELAY_MAX) > + val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1); > + else > + val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1); > + } > + > + dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n"); > + break; > + > + case PHY_INTERFACE_MODE_RGMII_ID: > + val = FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN) | > + FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN) | > + FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII); > + dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_ID\n"); > + break; So this one is pretty normal. The MAC does not add a delay, PHY_INTERFACE_MODE_RGMII_ID is passed to the PHY, and it adds the delay. The interface will likely work. > + > + case PHY_INTERFACE_MODE_RGMII_RXID: > + val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) | > + FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN); > + > + if (mac->tx_delay == 0) { > + val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN); > + } else { > + val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY); > + > + if (mac->tx_delay > MACPHYC_TX_DELAY_MAX) > + val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1); > + else > + val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1); > + } So here, the PHY is going to be passed PHY_INTERFACE_MODE_RGMII_RXID. The PHY will add a delay in the receive path. The MAC needs to add the delay in the transmit path. So tx_delay needs to be the full 2ns, not just a small fine tuning value, or the PCB is adding the delay. And you also cannot fine tune the RX delay, since rx_delay is ignored. > + > + dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_RXID\n"); > + break; > + > + case PHY_INTERFACE_MODE_RGMII_TXID: > + val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) | > + FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN); > + > + if (mac->rx_delay == 0) { > + val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN); > + } else { > + val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY); > + > + if (mac->rx_delay > MACPHYC_RX_DELAY_MAX) > + val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1); > + else > + val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1); > + } And here we have the opposite to PHY_INTERFACE_MODE_RGMII_RXID. So you need to clearly document in the device tree binding when rx_delay and tx_delay are used, and when they are ignored. You don't want to have DT writers having to look deep into the code to figure this out. Personally, i would simply this, in a big way. I see two options: 1) The MAC never adds a delay. The hardware is there, but simply don't use it, to keep thing simple, and the same as nearly every other MAC. 2) If the hardware can do small steps of delay, allow this delay, both RX and TX, to be configured in all four modes, in order to allow for fine tuning. Leave the PHY to insert the majority of the delay. > + /* Get MAC PHY control register */ > + mac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "mode-reg"); > + if (IS_ERR(mac->regmap)) { > + dev_err(&pdev->dev, "%s: failed to get syscon regmap\n", __func__); > + goto err_remove_config_dt; > + } Please document this in the device tree binding. > + > + ret = of_property_read_u32(pdev->dev.of_node, "rx-clk-delay", &mac->rx_delay); > + if (ret) > + mac->rx_delay = 0; > + > + ret = of_property_read_u32(pdev->dev.of_node, "tx-clk-delay", &mac->tx_delay); > + if (ret) > + mac->tx_delay = 0; Please take a look at dwmac-mediatek.c. It handles delays nicely. I would suggest that is the model to follow. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel