Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 02:29 PM, Mason wrote:
> On 19/07/2017 21:30, Florian Fainelli wrote:
>> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>>> Hi
>>>
>>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
 The current code supports enabling RGMII RX and TX clock delays.
 The unstated assumption is that these settings are disabled by
 default at reset, which is not the case.

 RX clock delay is enabled at reset. And TX clock delay "survives"
 across SW resets. Thus, if the bootloader enables TX clock delay,
 it will remain enabled at reset in Linux.

 Provide disable functions to configure the RGMII clock delays
 exactly as specified in the fwspec.

 Signed-off-by: Marc Gonzalez 
 ---
   drivers/net/phy/at803x.c | 32 
   1 file changed, 24 insertions(+), 8 deletions(-)
>>> This patch breaks am335x-evm networking.
>>>
>>> To restore it I've had to apply below diff:
>>> diff --git a/arch/arm/boot/dts/am335x-evm.dts 
>>> b/arch/arm/boot/dts/am335x-evm.dts
>>> index 200d6ab..9578bdf 100644
>>> --- a/arch/arm/boot/dts/am335x-evm.dts
>>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>>> @@ -724,12 +724,12 @@
>>>  
>>>  _emac0 {
>>> phy_id = <_mdio>, <0>;
>>> -   phy-mode = "rgmii-txid";
>>> +   phy-mode = "rgmii-id";
>>>  };
>>>  
>>>  _emac1 {
>>> phy_id = <_mdio>, <1>;
>>> -   phy-mode = "rgmii-txid";
>>> +   phy-mode = "rgmii-id";
>>>  };
>>>  
>>>   {
>>>
>>> Sry, can't comment here to much - not E-PHY expert.
>>
>> It's useful feedback, since we had poorly defined "phy-mode" semantics
>> for too long, this is totally expected, Marc this is exactly why Mans is
>> suggesting additional MAC-specific properties to define delays.
> 
> In the current situation, it is impossible to configure
> the at803x to disable RX clock delay or TX clock delay
> (in case the boot loader enabled it).
> 
> Are you saying that, because no one has had a problem
> so far, it is not possible to fix it now, as it would
> break boards like am335x-evm.dts which didn't request
> RX clock delay, but got one anyway?

First it means that your patch as-is broke Grygorii's board, and you
need to at least integrate his patch if you plan on having your own
patch accepted. This will fix am335x-evm.dts, but we have no visibility
into the other DTSes out there that may be using an at803x PHY. If you u
break something you need to fix it, and touching how PHY delays are

> 
> Does that mean we cannot support boards using AR8035
> that need the RX and TX clock delays disabled?

No, that is not what that means, it means that you cannot change how an
existing PHY driver with active and existing deployments is interpreting
the phy_interface_t value in a way that it breaks people setups, which
your patch just did. Yes this makes it non-conforming to the revised
definition of "phy-mode", but it is just how it is, people did not know
any better before.

See below for what you could do.

> 
> I'm not sure how the MAC-specific properties can save
> the day?

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

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Mason]

On 19/07/2017 21:30, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez 
>>> ---
>>>   drivers/net/phy/at803x.c | 32 
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts 
>> b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>  
>>  _emac0 {
>> phy_id = <_mdio>, <0>;
>> -   phy-mode = "rgmii-txid";
>> +   phy-mode = "rgmii-id";
>>  };
>>  
>>  _emac1 {
>> phy_id = <_mdio>, <1>;
>> -   phy-mode = "rgmii-txid";
>> +   phy-mode = "rgmii-id";
>>  };
>>  
>>   {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.

In the current situation, it is impossible to configure
the at803x to disable RX clock delay or TX clock delay
(in case the boot loader enabled it).

Are you saying that, because no one has had a problem
so far, it is not possible to fix it now, as it would
break boards like am335x-evm.dts which didn't request
RX clock delay, but got one anyway?

Does that mean we cannot support boards using AR8035
that need the RX and TX clock delays disabled?

I'm not sure how the MAC-specific properties can save
the day?

Regards.

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

On 07/19/2017 02:30 PM, Florian Fainelli wrote:

On 07/19/2017 12:24 PM, Grygorii Strashko wrote:

Hi

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.

Signed-off-by: Marc Gonzalez 
---
   drivers/net/phy/at803x.c | 32 
   1 file changed, 24 insertions(+), 8 deletions(-)

This patch breaks am335x-evm networking.

To restore it I've had to apply below diff:
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 200d6ab..9578bdf 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -724,12 +724,12 @@
  
  _emac0 {

 phy_id = <_mdio>, <0>;
-   phy-mode = "rgmii-txid";
+   phy-mode = "rgmii-id";
  };
  
  _emac1 {

 phy_id = <_mdio>, <1>;
-   phy-mode = "rgmii-txid";
+   phy-mode = "rgmii-id";
  };
  
   {


Sry, can't comment here to much - not E-PHY expert.


It's useful feedback, since we had poorly defined "phy-mode" semantics
for too long, this is totally expected, Marc this is exactly why Mans is
suggesting additional MAC-specific properties to define delays.



Yeah. original commit is pretty old and description is not very useful

commit 6d75afe2916adf9e9de6862275cdf89b9b7e4d0e
Author: Mugunthan V N 
Date:   Mon Jun 3 20:10:11 2013 +

ARM: dts: AM33XX: Add phy-mode to CPSW node


--
regards,
-grygorii

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
> Hi
> 
> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>> The current code supports enabling RGMII RX and TX clock delays.
>> The unstated assumption is that these settings are disabled by
>> default at reset, which is not the case.
>>
>> RX clock delay is enabled at reset. And TX clock delay "survives"
>> across SW resets. Thus, if the bootloader enables TX clock delay,
>> it will remain enabled at reset in Linux.
>>
>> Provide disable functions to configure the RGMII clock delays
>> exactly as specified in the fwspec.
>>
>> Signed-off-by: Marc Gonzalez 
>> ---
>>   drivers/net/phy/at803x.c | 32 
>>   1 file changed, 24 insertions(+), 8 deletions(-)
> This patch breaks am335x-evm networking.
> 
> To restore it I've had to apply below diff:
> diff --git a/arch/arm/boot/dts/am335x-evm.dts 
> b/arch/arm/boot/dts/am335x-evm.dts
> index 200d6ab..9578bdf 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -724,12 +724,12 @@
>  
>  _emac0 {
> phy_id = <_mdio>, <0>;
> -   phy-mode = "rgmii-txid";
> +   phy-mode = "rgmii-id";
>  };
>  
>  _emac1 {
> phy_id = <_mdio>, <1>;
> -   phy-mode = "rgmii-txid";
> +   phy-mode = "rgmii-id";
>  };
>  
>   {
> 
> Sry, can't comment here to much - not E-PHY expert.

It's useful feedback, since we had poorly defined "phy-mode" semantics
for too long, this is totally expected, Marc this is exactly why Mans is
suggesting additional MAC-specific properties to define delays.
-- 
Florian

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

Hi

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
> The current code supports enabling RGMII RX and TX clock delays.
> The unstated assumption is that these settings are disabled by
> default at reset, which is not the case.
> 
> RX clock delay is enabled at reset. And TX clock delay "survives"
> across SW resets. Thus, if the bootloader enables TX clock delay,
> it will remain enabled at reset in Linux.
> 
> Provide disable functions to configure the RGMII clock delays
> exactly as specified in the fwspec.
> 
> Signed-off-by: Marc Gonzalez 
> ---
>   drivers/net/phy/at803x.c | 32 
>   1 file changed, 24 insertions(+), 8 deletions(-)
This patch breaks am335x-evm networking.

To restore it I've had to apply below diff:
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 200d6ab..9578bdf 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -724,12 +724,12 @@
 
 _emac0 {
phy_id = <_mdio>, <0>;
-   phy-mode = "rgmii-txid";
+   phy-mode = "rgmii-id";
 };
 
 _emac1 {
phy_id = <_mdio>, <1>;
-   phy-mode = "rgmii-txid";
+   phy-mode = "rgmii-id";
 };
 
  {

Sry, can't comment here to much - not E-PHY expert.

-- 
regards,
-grygorii

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Timur Tabi]

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.


I only use SGMII mode, and I tested and can confirm that this patch does 
not break SGMII, so:


Acked-by: Timur Tabi 

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Marc Gonzalez]

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.

Signed-off-by: Marc Gonzalez 
---
 drivers/net/phy/at803x.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..9c96e2cb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -117,12 +117,24 @@ static inline int at803x_enable_rx_delay(struct 
phy_device *phydev)
AT803X_DEBUG_RX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+{
+   return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+   AT803X_DEBUG_RX_CLK_DLY_EN, 0);
+}
+
 static inline int at803x_enable_tx_delay(struct phy_device *phydev)
 {
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
AT803X_DEBUG_TX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+{
+   return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
+   AT803X_DEBUG_TX_CLK_DLY_EN, 0);
+}
+
 /* save relevant PHY registers to private copy */
 static void at803x_context_save(struct phy_device *phydev,
struct at803x_context *context)
@@ -284,18 +296,22 @@ static int at803x_config_init(struct phy_device *phydev)
return ret;
 
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
-   phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+   phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
ret = at803x_enable_rx_delay(phydev);
-   if (ret < 0)
-   return ret;
-   }
+   else
+   ret = at803x_disable_rx_delay(phydev);
+
+   if (ret < 0)
+   return ret;
 
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-   phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+   phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
ret = at803x_enable_tx_delay(phydev);
-   if (ret < 0)
-   return ret;
-   }
+   else
+   ret = at803x_disable_tx_delay(phydev);
+
+   if (ret < 0)
+   return ret;
 
return 0;
 }
-- 
2.11.0