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

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

On 07/21/2017 07:24 AM, Måns Rullgård wrote:
> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> 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.
> 
> There's no need for the drivers to communicate.  The location of the
> properties in the device tree determines which driver should deal with
> it.

Exactly. I really like how the PHY delay properties are defined in
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt because they
are quite generic and for a MAC counterpart those defined in
Documentation/devicetree/bindings/net/dwmac-sun8i.txt and
Documentation/devicetree/bindings/net/meson-dwmac.txt are also good
examples.

> 
>> 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:
> 
> Simply stop reacting to the delay aspect of the phy-connection-type
> property?  Yes, I'm fine with that.
> 


-- 
Florian