Re: [PATCH] net: phy: at803x: disable delay only for RGMII mode

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/14/19 7:06 AM, Niklas Cassel wrote:
> On Thu, Feb 14, 2019 at 03:22:28PM +0200, Peter Ujfalusi wrote:
>> Hi Niklas,
>>
>> On 14/02/2019 14.39, Niklas Cassel wrote:
>>>>> So, I've rebased your old patch, see attachment.
>>>>> I suggest that Peter test it on am335x-evm.
>>>>
>>>> with the patch + s/rgmii-txid/rgmii-id in the am335x-evmsk.dts ethernet
>>>> is working.
>>>> I don't have am335x-evm to test, but it has the same PHY as evmsk.
>>>>
>>>
>>> Florian's concern was that this PHY driver looked at "phy-mode" from the
>>> perspective of the MAC rather than the PHY.
>>> However, if s/rgmii-txid/rgmii-id is the correct fix for am335x-evm,
>>> then this means that this PHY driver was just broken.
>>>
>>> If the driver had misinterpreted the perspective, then the correct
>>> fix for am335x-evm would have been s/rgmii-txid/rgmii-rxid.
>>
>> Not sure if I got this right, but:
>> rgmii-id/txid/rxid is the delay mode between PHY and MAC, right?
>> on the PHY node it is from the PHY perspective, right?
> 
> Yes, from the PHY perspective.
> 
> (According to Florian, IIUC, some old PHY drivers were implemented before
> it was decided that it is from PHY perspective, rather than from MAC
> perspective.)
> 
>>
>> The errata I have mentioned for am335x say:
>> "The reset state of RGMII1_IDMODE (bit 4) and RGMII2_IDMODE (bit 5) in
>> the GMII_SEL register enables internal delay mode on the transmit clock
>> of the respective RGMII port. The AM335x device does not support
>> internal delay mode, so RGMII1_IDMODE and RGMII2_IDMODE must be set to 1b."
>>
>> If the delay mode on the transmit clock is not working on the am335x,
>> then this translate that the rxid needs to be enabled on the PHY side?
> 
> IIUC what Florian explained, then either MAC or PHY needs to add delays,
> so if the PHY only adds delay on e.g. TX, then the MAC needs to add delay
> on RX.
> 
> However, in your case, the errata says that your MAC is not capable of
> adding a delay on TX, therefore the PHY needs to add a delay on TX.

So that mandates specifying either 'rgmii-id' or 'rgmii-txid'.

> 
>>
>> But then why it worked when only the txid was enabled and rxid was not
>> on the PHY side, and why it works if both txid and rxid is enabled?
> 
> Because the PHY driver was broken, so the PHY driver always enabled
> delays on both TX and RX.
> 
> This is how the driver looked before Vinod's change:
> 
>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>                         phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>                 ret = at803x_enable_rx_delay(phydev);
>                 if (ret < 0)
>                         return ret;
>         }
>  
>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>                         phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>                 ret = at803x_enable_tx_delay(phydev);
>                 if (ret < 0)
>                         return ret;
>         }
> 
> 
> Yet, the initial value for this PHY is that both TX and RX delay is
> enabled, and since this driver never disabled TX/RX delays,
> the TX and RX delays were always enabled, no matter what phy-mode
> you specified.

Fixing all PHY drivers is probably too much, but since we seem to have
traction and people assigned to QCOM working on this at803x PHY driver,
then let's fix it correctly, I agree with that. So that means:

- disable both RX and TX delay by default
- if RGMII_RXID or RGMII_ID is specified: turn on RX delay
- if RGMII_TXID or RGMII_ID is specified: turn on TX delay

so basically the patch above, with an initial delay disabling for both
RX and TX. Then we fix all DTSes to be correct, at least those in tree
and we provide appropriate Fixes: tag so things get backported
automatically.

How does that sound?
-- 
Florian