Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Andrew Lunn <andrew@lunn.ch>]

> >>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
> >>>> +				    struct device_node *eth_np,
> >>>> +				    phy_interface_t phy_if)
> >>>> +{
> >>>
> >>> ...
> >>>
> >>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
> >>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
> >>>> +
> >>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
> > 
> > This is only applicable to some devices so you need to restrict this only
> > to those devices.
> > 
> 
> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
> think there is any need for any device related restriction. Once support for
> other devices are enabled for upstream, we can modify this accordingly.

The problem is, this is a board property, not a SoC property. What if
somebody designs a board with extra long clock lines in order to add
the delay?

> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
> 5-624) for AM65 Silicon Revision 2.0.
> 
> Below is the description in Table 5-624
> 
> BIT	    : 24
> Field	    : RGMII0_ID_MODE
> Type	    : R/W
> Reset	    : 0h
> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
> 	      0h - Internal transmit delay is enabled
> 	      1h - Reserved
> 
> The TX internal delay is always enabled and couldn't be disabled as 1h is
> reserved. So hardware support for disabling TX internal delay is not there.

So if somebody passes a phy-mode which requires it disabled, you need
to return -EINVAL, to indicate the hardware cannot actually do it.

> As, TX internal delay is always there, there is no need to enable it in MAC or
> PHY. So no need of API prueth_config_rgmiidelay().
> 
> My approach to handle delay would be as below.
> 
> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.

As i said this depends on the board, not the SoC. In theory, you could
design a board with an extra long RX clock line, and then use phy-mode
rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay.

> *) Let TX internal delay enabled in Hardware.
> *) Let PHY configure RX internal delay.
> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
> Internal delay is always enabled.
> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
> API, add below if condition.
> 
> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID

You should handle all cases where a TX delay is requested, not just
ID.

	Andrew