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

[Md Danish Anwar]

On 09/02/23 18:28, Roger Quadros wrote:
> 
> 
> 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.
> 

Yes, I confirmed with the Hardware folks, the forced TX delay issue is still
there. It's not fixed as of now.

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

Noted.

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

Yes, My bad. "rgmii-rxid" is not possible. I'll return -EINVAL here.

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

Yes Noted, TX delay will be forced and phy-mode "rgmii" will be passed to 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.
>  

Sure, Noted.

So just to summarize,

For "rgmii-id" phy mode, pass "rgmii-rxid" to phy.
For "rgmii-rxid", return error.
For "rgmii-txid", TX delay is forced, pass "rgmii" to PHY.
For "rgmii", return error.


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

-- 
Thanks and Regards,
Danish.