> >>>> +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
