Re: [PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Marc Gonzalez <marc_gonzalez@sigmadesigns.com>]

On 21/07/2017 15:47, Måns Rullgård wrote:

> Marc Gonzalez wrote:
> 
>> On 21/07/2017 15:04, Måns Rullgård wrote:
>>
>>> Marc Gonzalez wrote:
>>>
>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>> there are 4 RGMII modes to handle:
>>>>
>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>> "rgmii-id" (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-rxid" (RGMII with internal RX delay provided by the PHY,
>>>> 	the MAC should not add an RX delay in this case)
>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>> 	the MAC should not add an TX delay in this case)
>>>>
>>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>> index ded041dbafe7..f3ed320eb4ad 100644
>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>>  		break;
>>>>
>>>>  	case PHY_INTERFACE_MODE_RGMII:
>>>> -		pad_mode = PAD_MODE_RGMII;
>>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>  		break;
>>>>
>>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>> +		pad_mode = PAD_MODE_RGMII;
>>>>  		break;
>>>>
>>>>  	default:
>>>
>>> I still don't like this.  Having both the MAC and PHY drivers react to
>>> the phy-connection-type property is bound to cause trouble somewhere.
>>>
>>> The only way out of the current mess is to define new properties for
>>> both MAC and PHY that override the existing ones if present.
>>
>> Do you mean defining 4 new bindings and their corresponding
>> phy_interface_t enum values? For example:
>>
>> "rgmii-v2"
>> "rgmii-id-v2"
>> "rgmii-rxid-v2"
>> "rgmii-txid-v2"
>>
>> 	PHY_INTERFACE_MODE_RGMII_V2,
>> 	PHY_INTERFACE_MODE_RGMII_ID_V2,
>> 	PHY_INTERFACE_MODE_RGMII_RXID_V2,
>> 	PHY_INTERFACE_MODE_RGMII_TXID_V2,
>>
>> And then handling these new enums in the at803x and nb8800 drivers?
> 
> It has already been suggested to add new properties specifying desired
> delays in picoseconds.  If present on the MAC node, the MAC driver
> should attempt to provide the delay, and if present on the PHY node, the
> PHY driver is responsible.

Sorry, I had already forgotten about Florian's suggestion:
> If you introduced PHY and/or MAC specific properties to configure the
> delays in the appropriate unit of time (say ps), you could use a
> non-compliant 'phy-mode' just to satisfy the driver/PHY library and
> still override the delays you need.

So we would need two properties (RX and TX).
"rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"

but it's not clear to me how the MAC probe function communicates
the arguments to the phy driver. Looks like we would need to add
two fields to struct phy_device, and maybe define a new phy-mode
to instruct the PHY driver to look for the two fields.

I don't have time to work on that for now, but I do need to
fix the nb8800 driver now. Can we apply the following patch
in the interim?

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..e94159507847 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
                break;
 
        case PHY_INTERFACE_MODE_RGMII:
-               pad_mode = PAD_MODE_RGMII;
-               break;
-
+       case PHY_INTERFACE_MODE_RGMII_ID:
+       case PHY_INTERFACE_MODE_RGMII_RXID:
        case PHY_INTERFACE_MODE_RGMII_TXID:
-               pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+               pad_mode = PAD_MODE_RGMII;
                break;
 
        default:

Regards.