From: Zhou Yanjie <zhouyanjie@wanyeetech.com> To: Andrew Lunn <andrew@lunn.ch> 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. Date: Thu, 10 Jun 2021 16:00:00 +0800 [thread overview] Message-ID: <405696cb-5987-0e56-87f8-5a1443eadc19@wanyeetech.com> (raw) In-Reply-To: <YMGEutCet7fP1NZ9@lunn.ch> Hi Andrew, On 2021/6/10 上午11:19, Andrew Lunn wrote: >> +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. Got it, thanks! > >> +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. The manual does not tell how much ps a unit is. I am confirming with Ingenic, but there is no reply at the moment. Can we follow Rockchip's approach? According to the description in "rockchip-dwmac.yaml" and the related code in "dwmac-rk.c", it seems that their delay parameter seems to be the value used by the hardware directly instead of ps. > 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. Sure, I'll change it in the next version. > 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. Sure, maybe I should write a new independent document for Ingenic instead of just making corresponding changes in "snps, dwmac.yaml" > > 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. It seems that this method is better, I will adopt it in v3. >> + /* 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. Sure. > >> + >> + 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. Sure. Thanks and best regards! > > Andrew
WARNING: multiple messages have this Message-ID (diff)
From: Zhou Yanjie <zhouyanjie@wanyeetech.com> To: Andrew Lunn <andrew@lunn.ch> 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. Date: Thu, 10 Jun 2021 16:00:00 +0800 [thread overview] Message-ID: <405696cb-5987-0e56-87f8-5a1443eadc19@wanyeetech.com> (raw) In-Reply-To: <YMGEutCet7fP1NZ9@lunn.ch> Hi Andrew, On 2021/6/10 上午11:19, Andrew Lunn wrote: >> +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. Got it, thanks! > >> +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. The manual does not tell how much ps a unit is. I am confirming with Ingenic, but there is no reply at the moment. Can we follow Rockchip's approach? According to the description in "rockchip-dwmac.yaml" and the related code in "dwmac-rk.c", it seems that their delay parameter seems to be the value used by the hardware directly instead of ps. > 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. Sure, I'll change it in the next version. > 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. Sure, maybe I should write a new independent document for Ingenic instead of just making corresponding changes in "snps, dwmac.yaml" > > 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. It seems that this method is better, I will adopt it in v3. >> + /* 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. Sure. > >> + >> + 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. Sure. Thanks and best regards! > > Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-10 8:01 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-09 17:35 [PATCH v2 0/2] Add Ingenic SoCs MAC support 周琰杰 (Zhou Yanjie) 2021-06-09 17:35 ` 周琰杰 (Zhou Yanjie) 2021-06-09 17:35 ` [PATCH v2 1/2] dt-bindings: dwmac: Add bindings for new Ingenic SoCs 周琰杰 (Zhou Yanjie) 2021-06-09 17:35 ` 周琰杰 (Zhou Yanjie) 2021-06-09 17:35 ` [PATCH v2 2/2] net: stmmac: Add Ingenic SoCs MAC support 周琰杰 (Zhou Yanjie) 2021-06-09 17:35 ` 周琰杰 (Zhou Yanjie) 2021-06-10 3:19 ` Andrew Lunn 2021-06-10 3:19 ` Andrew Lunn 2021-06-10 8:00 ` Zhou Yanjie [this message] 2021-06-10 8:00 ` Zhou Yanjie 2021-06-10 12:15 ` Andrew Lunn 2021-06-10 12:15 ` Andrew Lunn [not found] ` <346f64d9-6949-b506-258f-4cfa7eb22784@wanyeetech.com> 2021-06-10 14:42 ` Andrew Lunn 2021-06-10 14:42 ` Andrew Lunn 2021-06-13 8:26 ` 周琰杰 2021-06-13 8:26 ` 周琰杰 [not found] ` <12f35415-532e-5514-bc97-683fb9655091@wanyeetech.com> 2021-06-10 14:57 ` Andrew Lunn 2021-06-10 14:57 ` Andrew Lunn 2021-06-13 8:34 ` 周琰杰 2021-06-13 8:34 ` 周琰杰 2021-06-13 16:31 ` Andrew Lunn 2021-06-13 16:31 ` Andrew Lunn 2021-06-14 15:57 ` 周琰杰 2021-06-14 15:57 ` 周琰杰
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=405696cb-5987-0e56-87f8-5a1443eadc19@wanyeetech.com \ --to=zhouyanjie@wanyeetech.com \ --cc=alexandre.torgue@foss.st.com \ --cc=andrew@lunn.ch \ --cc=aric.pzqi@ingenic.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=dongsheng.qiu@ingenic.com \ --cc=joabreu@synopsys.com \ --cc=jun.jiang@ingenic.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=paul@crapouillou.net \ --cc=peppe.cavallaro@st.com \ --cc=rick.tyliu@ingenic.com \ --cc=robh+dt@kernel.org \ --cc=sernia.zhou@foxmail.com \ --cc=sihui.liu@ingenic.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.