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

[Md Danish Anwar <a0501179@ti.com>]

Hi Andrew,

On 08/02/23 18:26, Andrew Lunn wrote:
>>>>>> +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.
> 

Sure, I'll do that. In the list of all phy modes described in [1], I can only
see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
phy-mode that requires enabling/disabling TX internal delays? Please let me
know if any other phy-mode also needs this. I will add check for that as well.

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

Yes I understand that board can have any phy-mode in it's DTS. We need to be
able to handle all different phy modes.

>> *) 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.
> 

So there could be four different RGMII phy modes as described in [1]. Below is
the handling mechanism for different phy modes.

1)    # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But
in our SoC TX internal delay is always enabled. So to handle this, we'll change
the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY
will enable RX internal delay only.

2)    # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do
nothing in the driver and just pass the same mode to phy, so that PHY will
enable RX internal delay only.

3)    # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid

For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC
should not add an TX delay in this case. But in our SoC TX internal delay is
always enabled. So this scenario can not be handled. We will return -EINVAL in
this case.


4)    # RX and TX delays are added by the MAC when required
      - rgmii

For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX
internal delay is always enabled so no need to add TX delay. For RX I am not
sure what should we do as there is no provision of adding RX delay in MAC
currently. Should we ask PHY to add RX delay?

Andrew, Roger, Can you please comment on this.

Apart from Case 4, below code change will be able to handle all other cases.

	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
		emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
		return -EINVAL;

Please let me know if I am missing any other phy modes.

[1] Documentation/devicetree/bindings/net/ethernet-controller.yaml

> 	Andrew

-- 
Thanks and Regards,
Danish.