Re: 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Andrew Lunn]

On Tue, Mar 18, 2025 at 10:46:58AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for your reply.
> 
> > > +	u32 rgmii_tx_delay, rgmii_rx_delay;
> > > +	u32 dly_reg, tx_dly_mask, rx_dly_mask;
> > > +	int tx, rx;
> > > +
> > > +	netdev = platform_get_drvdata(pdev);
> > > +	priv = netdev_priv(netdev);
> > > +
> > > +	tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
> > > +	rx = of_property_read_u32(np, "rx-internal-delay-ps",
> > > +&rgmii_rx_delay);
> > 
> > > +	if (!tx) {
> > 
> > The documentation for of_property_read_u32() says:
> > 
> >  * Return: 0 on success, -EINVAL if the property does not exist,
> >  * -ENODATA if property does not have a value, and -EOVERFLOW if the
> >  * property data isn't large enough.
> > 
> > You need to handle EINVAL different to the other errors, which are real errors
> > and should fail the probe.
> > 
> > The commit message, and probably the binding needs to document what
> > happens when the properties are not in the DT blob. This needs to be part of
> > the bigger picture of how you are going to sort out the mess with existing .dts
> > files listing 'rgmii' when in fact they should be 'rgmii-id'.
> 
> Why can't the MAC add internal delay to RGMII? Is it necessary to add on PHY side?

The MAC could, but that is not the point. You need to explain how you
are going to solve the mess you have in DT, why all aspeed boards have
the wrong phy-mode. You need to fix that, and i will continue to NACK
new boards until the correct rgmii-id value can be used to indicate
there do not have extra long clock lines on the PCB.

> > > +		/* Use tx-internal-delay-ps as index to configure tx delay
> > > +		 * into scu register.
> > > +		 */
> > > +		if (rgmii_tx_delay > 64)
> > > +			dev_warn(&pdev->dev, "Get invalid tx delay value");
> > 
> > Return EINVAL and fail the probe.
> 
> Agreed.
> I just show warning here, because sometimes the RGMII delay value will configure at bootloader.

That is a different issue. If there is a value in DT, it must be
valid, fail the probe otherwise.

	Andrew