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

[Roger Quadros <rogerq@kernel.org>]

On 09/02/2023 12:29, Md Danish Anwar wrote:
> 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

OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions.
But it looks like it hasn't been fixed yet.


> the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY
> will enable RX internal delay only.

OK.

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

But the MAC is forcing TX-delay right? So this case can't be implemented.
you have to return error.

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

As you didn't return error for 1st case "rgmii-id" even though TX delay was requested
for PHY but you added it in the MAC I see no reason to return error here.

You just do the delay in MAC and pass "rgmii" to the PHY.

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

I don't think it will work so you can error out in this case.
 
> 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
> 

cheers,
-roger