Re: [PATCH v2 2/2] net: stmmac: Add Ingenic SoCs MAC support.

[Zhou Yanjie]

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