| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <405696cb-5987-0e56-87f8-5a1443eadc19@wanyeetech.com>
Date: Thu, 10 Jun 2021 16:00:00 +0800
From: Zhou Yanjie <zhouyanjie@...yeetech.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: davem@...emloft.net, kuba@...nel.org, robh+dt@...nel.org,
peppe.cavallaro@...com, alexandre.torgue@...s.st.com,
joabreu@...opsys.com, mcoquelin.stm32@...il.com,
linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, dongsheng.qiu@...enic.com,
aric.pzqi@...enic.com, rick.tyliu@...enic.com,
sihui.liu@...enic.com, jun.jiang@...enic.com,
sernia.zhou@...mail.com, paul@...pouillou.net
Subject: Re: [PATCH v2 2/2] net: stmmac: Add Ingenic SoCs MAC support.
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
Powered by blists - more mailing lists