Messages in this thread | | | Subject | Re: [PATCH v2 2/2] net: stmmac: Add Ingenic SoCs MAC support. | From | Zhou Yanjie <> | Date | Thu, 10 Jun 2021 16:00:00 +0800 |
| |
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
| |