linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
@ 2016-11-04 15:02 Sebastian Frias
  2016-11-04 15:11 ` Andrew Lunn
  2016-11-04 15:18 ` Måns Rullgård
  0 siblings, 2 replies; 11+ messages in thread
From: Sebastian Frias @ 2016-11-04 15:02 UTC (permalink / raw)
  To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn

The delay can be applied at PHY or MAC level, but since
PHY drivers will apply the delay at PHY level when using
one of the "internal delay" declinations of RGMII mode
(like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
at MAC level causes issues.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index b59aa35..d2855c9 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:
-- 
1.7.11.2

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:02 [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level Sebastian Frias
@ 2016-11-04 15:11 ` Andrew Lunn
  2016-11-04 15:27   ` Måns Rullgård
  2016-11-04 15:29   ` Sebastian Frias
  2016-11-04 15:18 ` Måns Rullgård
  1 sibling, 2 replies; 11+ messages in thread
From: Andrew Lunn @ 2016-11-04 15:11 UTC (permalink / raw)
  To: Sebastian Frias
  Cc: Måns Rullgård, David S. Miller, netdev, LKML, Mason

On Fri, Nov 04, 2016 at 04:02:24PM +0100, Sebastian Frias wrote:
> The delay can be applied at PHY or MAC level, but since
> PHY drivers will apply the delay at PHY level when using
> one of the "internal delay" declinations of RGMII mode
> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
> at MAC level causes issues.
> 
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index b59aa35..d2855c9 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>  		break;
>  
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +		pad_mode = PAD_MODE_RGMII;
>  		break;

How many boards use this Ethernet driver? How many boards are your
potentially breaking, because they need this delay?

I guess it is a small number, because doesn't it require the PHY is
also broken, not adding a delay when it should?

     Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:02 [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level Sebastian Frias
  2016-11-04 15:11 ` Andrew Lunn
@ 2016-11-04 15:18 ` Måns Rullgård
  2016-11-04 15:36   ` Sebastian Frias
  1 sibling, 1 reply; 11+ messages in thread
From: Måns Rullgård @ 2016-11-04 15:18 UTC (permalink / raw)
  To: Sebastian Frias; +Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn

Sebastian Frias <sf84@laposte.net> writes:

> The delay can be applied at PHY or MAC level, but since
> PHY drivers will apply the delay at PHY level when using
> one of the "internal delay" declinations of RGMII mode
> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
> at MAC level causes issues.

The Broadcom GENET driver does the same thing.

> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index b59aa35..d2855c9 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>  		break;
>
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +		pad_mode = PAD_MODE_RGMII;
>  		break;
>
>  	default:
> -- 
> 1.7.11.2

If this change is correct (and I'm not convinced it is), that case
should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID
added as well.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:11 ` Andrew Lunn
@ 2016-11-04 15:27   ` Måns Rullgård
  2016-11-04 15:29   ` Sebastian Frias
  1 sibling, 0 replies; 11+ messages in thread
From: Måns Rullgård @ 2016-11-04 15:27 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Sebastian Frias, David S. Miller, netdev, LKML, Mason

Andrew Lunn <andrew@lunn.ch> writes:

> On Fri, Nov 04, 2016 at 04:02:24PM +0100, Sebastian Frias wrote:
>> The delay can be applied at PHY or MAC level, but since
>> PHY drivers will apply the delay at PHY level when using
>> one of the "internal delay" declinations of RGMII mode
>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
>> at MAC level causes issues.

If this is correct, most of the PHY drivers are broken.

>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index b59aa35..d2855c9 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>>  		break;
>>  
>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +		pad_mode = PAD_MODE_RGMII;
>>  		break;
>
> How many boards use this Ethernet driver? How many boards are your
> potentially breaking, because they need this delay?
>
> I guess it is a small number, because doesn't it require the PHY is
> also broken, not adding a delay when it should?

What if the PHY doesn't have that option?

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:11 ` Andrew Lunn
  2016-11-04 15:27   ` Måns Rullgård
@ 2016-11-04 15:29   ` Sebastian Frias
  1 sibling, 0 replies; 11+ messages in thread
From: Sebastian Frias @ 2016-11-04 15:29 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Måns Rullgård, David S. Miller, netdev, LKML, Mason

Hi Andrew,

On 11/04/2016 04:11 PM, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 04:02:24PM +0100, Sebastian Frias wrote:
>> The delay can be applied at PHY or MAC level, but since
>> PHY drivers will apply the delay at PHY level when using
>> one of the "internal delay" declinations of RGMII mode
>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
>> at MAC level causes issues.
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index b59aa35..d2855c9 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>>  		break;
>>  
>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +		pad_mode = PAD_MODE_RGMII;
>>  		break;
> 
> How many boards use this Ethernet driver? How many boards are your
> potentially breaking, because they need this delay?
> 

This part is specific to the Tango architecture, as noted by the
function name "nb8800_tangox_init".

Also the register used here is Sigma-specific (i.e.: not related to the
Aurora VLSI MAC, "au-nb8800")

The thing is that without this patch if we set
phy-connection-type="rgmii-txid" on the DT, then both, the PHY and the
MAC, will apply the delay.

Best regards,

Sebastian

> I guess it is a small number, because doesn't it require the PHY is
> also broken, not adding a delay when it should?
> 
>      Andrew
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:18 ` Måns Rullgård
@ 2016-11-04 15:36   ` Sebastian Frias
  2016-11-04 16:28     ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Frias @ 2016-11-04 15:36 UTC (permalink / raw)
  To: Måns Rullgård; +Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn

Hi Måns,

On 11/04/2016 04:18 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
> 
>> The delay can be applied at PHY or MAC level, but since
>> PHY drivers will apply the delay at PHY level when using
>> one of the "internal delay" declinations of RGMII mode
>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
>> at MAC level causes issues.
> 
> The Broadcom GENET driver does the same thing.
> 

Well, I don't know who uses that driver, or why they did it that way.

However, with the current code and DT bindings, if one requires
the delay, phy-connection-type="rgmii-txid" must be set.

But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
will apply the delay.

I think a better way of dealing with this is that both, PHY and MAC
drivers exchange information so that the delay is applied only once.

I can see how to do that in another patch set.

>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index b59aa35..d2855c9 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>>  		break;
>>
>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +		pad_mode = PAD_MODE_RGMII;
>>  		break;
>>
>>  	default:
>> -- 
>> 1.7.11.2
> 
> If this change is correct (and I'm not convinced it is), that case
> should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID
> added as well.
> 

I can do a single patch.

The reason I made two patches was that it was clear what this patch
does, i.e.: do not apply the delay at MAC level, and what the subsequent
patch does, i.e.: handle all RGMII declinations.

Best regards,

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 15:36   ` Sebastian Frias
@ 2016-11-04 16:28     ` Florian Fainelli
  2016-11-04 16:49       ` Måns Rullgård
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2016-11-04 16:28 UTC (permalink / raw)
  To: Sebastian Frias, Måns Rullgård
  Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn



On 11/04/2016 08:36 AM, Sebastian Frias wrote:
> Hi Måns,
> 
> On 11/04/2016 04:18 PM, Måns Rullgård wrote:
>> Sebastian Frias <sf84@laposte.net> writes:
>>
>>> The delay can be applied at PHY or MAC level, but since
>>> PHY drivers will apply the delay at PHY level when using
>>> one of the "internal delay" declinations of RGMII mode
>>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
>>> at MAC level causes issues.
>>
>> The Broadcom GENET driver does the same thing.
>>
> 
> Well, I don't know who uses that driver, or why they did it that way.

I do use this driver and it works for me (tm), although I tested mostly
with Broadcom PHYs and Ethernet switches, rarely with third party PHYs,
but had that too, but all of that is in tree though,
drivers/net/phy/broadcom.com, drivers/net/dsa/b53/ so feel free to
"audit" that part of the code too.

The configuration of the GENET port multiplexer requires us to specify
how we want to align the clock and data, if we don't do that, and the
PHY is also not agreeing with how its own delays should be configured,
mayhem ensues, ranging from occasional transmit success, to high rates
of CRC/FCS errors in best cases.

I did verify that the settings were correct using a scope FWIW.

> 
> However, with the current code and DT bindings, if one requires
> the delay, phy-connection-type="rgmii-txid" must be set.

Yes, and we would set it correctly for our Broadcom reference boards
using this driver.

> 
> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
> will apply the delay.
> 
> I think a better way of dealing with this is that both, PHY and MAC
> drivers exchange information so that the delay is applied only once.

Exchange what information? The PHY device interface (phydev->interface)
conveys the needed information for both entities.

> 
> I can see how to do that in another patch set.
> 
>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>> ---
>>>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>> index b59aa35..d2855c9 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>  		break;
>>>
>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>> +		pad_mode = PAD_MODE_RGMII;
>>>  		break;
>>>
>>>  	default:
>>> -- 
>>> 1.7.11.2
>>
>> If this change is correct (and I'm not convinced it is), that case
>> should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID
>> added as well.
>>
> 
> I can do a single patch.
> 
> The reason I made two patches was that it was clear what this patch
> does, i.e.: do not apply the delay at MAC level, and what the subsequent
> patch does, i.e.: handle all RGMII declinations.
> 
> Best regards,
> 
> Sebastian
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 16:28     ` Florian Fainelli
@ 2016-11-04 16:49       ` Måns Rullgård
  2016-11-09 13:02         ` Sebastian Frias
  0 siblings, 1 reply; 11+ messages in thread
From: Måns Rullgård @ 2016-11-04 16:49 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Sebastian Frias, David S. Miller, netdev, LKML, Mason, Andrew Lunn

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 11/04/2016 08:36 AM, Sebastian Frias wrote:
>> Hi Måns,
>> 
>> On 11/04/2016 04:18 PM, Måns Rullgård wrote:
>>> Sebastian Frias <sf84@laposte.net> writes:
>>>
>>>> The delay can be applied at PHY or MAC level, but since
>>>> PHY drivers will apply the delay at PHY level when using
>>>> one of the "internal delay" declinations of RGMII mode
>>>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
>>>> at MAC level causes issues.
>>>
>>> The Broadcom GENET driver does the same thing.
>>>
>> 
>> Well, I don't know who uses that driver, or why they did it that way.
>
> I do use this driver and it works for me (tm), although I tested mostly
> with Broadcom PHYs and Ethernet switches, rarely with third party PHYs,
> but had that too, but all of that is in tree though,
> drivers/net/phy/broadcom.com, drivers/net/dsa/b53/ so feel free to
> "audit" that part of the code too.
>
> The configuration of the GENET port multiplexer requires us to specify
> how we want to align the clock and data, if we don't do that, and the
> PHY is also not agreeing with how its own delays should be configured,
> mayhem ensues, ranging from occasional transmit success, to high rates
> of CRC/FCS errors in best cases.
>
> I did verify that the settings were correct using a scope FWIW.
>
>> 
>> However, with the current code and DT bindings, if one requires
>> the delay, phy-connection-type="rgmii-txid" must be set.
>
> Yes, and we would set it correctly for our Broadcom reference boards
> using this driver.
>
>> 
>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
>> will apply the delay.
>> 
>> I think a better way of dealing with this is that both, PHY and MAC
>> drivers exchange information so that the delay is applied only once.
>
> Exchange what information? The PHY device interface (phydev->interface)
> conveys the needed information for both entities.

There doesn't seem to be any consensus among the drivers regarding where
the delay should be applied.  Since only a few drivers, MAC or PHY, act
on this property, most combinations still work by chance.  It is common
for boards to set the delay at the PHY using external config pins so no
software setup is required (although I have one Sigma based board that
gets this wrong).  I suspect if drivers/net/ethernet/broadcom/genet were
used with one of the four PHY drivers that also set the delay based on
this DT property, things would go wrong.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-04 16:49       ` Måns Rullgård
@ 2016-11-09 13:02         ` Sebastian Frias
  2016-11-09 17:03           ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Frias @ 2016-11-09 13:02 UTC (permalink / raw)
  To: Måns Rullgård, Florian Fainelli
  Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn

On 11/04/2016 05:49 PM, Måns Rullgård wrote:
>>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
>>> will apply the delay.
>>>
>>> I think a better way of dealing with this is that both, PHY and MAC
>>> drivers exchange information so that the delay is applied only once.
>>
>> Exchange what information? The PHY device interface (phydev->interface)
>> conveys the needed information for both entities.
> 
> There doesn't seem to be any consensus among the drivers regarding where
> the delay should be applied.  Since only a few drivers, MAC or PHY, act
> on this property, most combinations still work by chance.  It is common
> for boards to set the delay at the PHY using external config pins so no
> software setup is required (although I have one Sigma based board that
> gets this wrong).  I suspect if drivers/net/ethernet/broadcom/genet were
> used with one of the four PHY drivers that also set the delay based on
> this DT property, things would go wrong.
> 

Exactly, what about a patch like (I can make a formal submission, even
merge it with the patch discussed in this thread, consider this a RFC):

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index fba2699..4217ff4 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		pad_mode = PAD_MODE_RGMII;
+
+		if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0)
+			pad_mode |= PAD_MODE_GTX_CLK_DELAY;
+
 		break;
 
 	default:
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2e0c759..5eddb04 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev)
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	.features		= PHY_GBIT_FEATURES,
-	.flags			= PHY_HAS_INTERRUPT,
+	.flags			= PHY_HAS_INTERRUPT |
+				  PHY_SUPPORTS_RXID |
+				  PHY_SUPPORTS_TXID,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e7e1fd3..0f0b17e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -61,6 +61,8 @@
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
 #define PHY_IS_INTERNAL		0x00000004
+#define PHY_SUPPORTS_RXID       0x00000008
+#define PHY_SUPPORTS_TXID       0x00000010
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-09 13:02         ` Sebastian Frias
@ 2016-11-09 17:03           ` Florian Fainelli
  2016-11-14 13:22             ` Sebastian Frias
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2016-11-09 17:03 UTC (permalink / raw)
  To: Sebastian Frias, Måns Rullgård
  Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn

On 11/09/2016 05:02 AM, Sebastian Frias wrote:
> On 11/04/2016 05:49 PM, Måns Rullgård wrote:
>>>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
>>>> will apply the delay.
>>>>
>>>> I think a better way of dealing with this is that both, PHY and MAC
>>>> drivers exchange information so that the delay is applied only once.
>>>
>>> Exchange what information? The PHY device interface (phydev->interface)
>>> conveys the needed information for both entities.
>>
>> There doesn't seem to be any consensus among the drivers regarding where
>> the delay should be applied.  Since only a few drivers, MAC or PHY, act
>> on this property, most combinations still work by chance.  It is common
>> for boards to set the delay at the PHY using external config pins so no
>> software setup is required (although I have one Sigma based board that
>> gets this wrong).  I suspect if drivers/net/ethernet/broadcom/genet were
>> used with one of the four PHY drivers that also set the delay based on
>> this DT property, things would go wrong.
>>
> 
> Exactly, what about a patch like (I can make a formal submission, even
> merge it with the patch discussed in this thread, consider this a RFC):

I really don't see a point in doing this when we can just clarify what
phydev->interface does and already have the knowledge that we need
without introducing additional flags in the phy driver.

> 
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index fba2699..4217ff4 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev)
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>  		pad_mode = PAD_MODE_RGMII;
> +
> +		if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0)
> +			pad_mode |= PAD_MODE_GTX_CLK_DELAY;
> +
>  		break;
>  
>  	default:
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 2e0c759..5eddb04 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev)
>  	.suspend		= at803x_suspend,
>  	.resume			= at803x_resume,
>  	.features		= PHY_GBIT_FEATURES,
> -	.flags			= PHY_HAS_INTERRUPT,
> +	.flags			= PHY_HAS_INTERRUPT |
> +				  PHY_SUPPORTS_RXID |
> +				  PHY_SUPPORTS_TXID,
>  	.config_aneg		= genphy_config_aneg,
>  	.read_status		= genphy_read_status,
>  	.ack_interrupt		= at803x_ack_interrupt,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index e7e1fd3..0f0b17e 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -61,6 +61,8 @@
>  #define PHY_HAS_INTERRUPT	0x00000001
>  #define PHY_HAS_MAGICANEG	0x00000002
>  #define PHY_IS_INTERNAL		0x00000004
> +#define PHY_SUPPORTS_RXID       0x00000008
> +#define PHY_SUPPORTS_TXID       0x00000010
>  #define MDIO_DEVICE_IS_PHY	0x80000000
>  
>  /* Interface Mode definitions */
> 


-- 
Florian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
  2016-11-09 17:03           ` Florian Fainelli
@ 2016-11-14 13:22             ` Sebastian Frias
  0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Frias @ 2016-11-14 13:22 UTC (permalink / raw)
  To: Florian Fainelli, Måns Rullgård
  Cc: David S. Miller, netdev, LKML, Mason, Andrew Lunn

On 11/09/2016 06:03 PM, Florian Fainelli wrote:
> On 11/09/2016 05:02 AM, Sebastian Frias wrote:
>> On 11/04/2016 05:49 PM, Måns Rullgård wrote:
>>>>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers
>>>>> will apply the delay.
>>>>>
>>>>> I think a better way of dealing with this is that both, PHY and MAC
>>>>> drivers exchange information so that the delay is applied only once.
>>>>
>>>> Exchange what information? The PHY device interface (phydev->interface)
>>>> conveys the needed information for both entities.
>>>
>>> There doesn't seem to be any consensus among the drivers regarding where
>>> the delay should be applied.  Since only a few drivers, MAC or PHY, act
>>> on this property, most combinations still work by chance.  It is common
>>> for boards to set the delay at the PHY using external config pins so no
>>> software setup is required (although I have one Sigma based board that
>>> gets this wrong).  I suspect if drivers/net/ethernet/broadcom/genet were
>>> used with one of the four PHY drivers that also set the delay based on
>>> this DT property, things would go wrong.
>>>
>>
>> Exactly, what about a patch like (I can make a formal submission, even
>> merge it with the patch discussed in this thread, consider this a RFC):
> 
> I really don't see a point in doing this when we can just clarify what
> phydev->interface does and already have the knowledge that we need
> without introducing additional flags in the phy driver.
> 

Ok, so who can clarify what "phydev->interface" does, especially in the
context of this discussion?

What happens when a TX delay must be applied and:
  - both the PHY and the MAC support the delay
  - only the PHY supports the delay
  - only the MAC supports the delay

Best regards,

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-11-14 13:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-04 15:02 [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level Sebastian Frias
2016-11-04 15:11 ` Andrew Lunn
2016-11-04 15:27   ` Måns Rullgård
2016-11-04 15:29   ` Sebastian Frias
2016-11-04 15:18 ` Måns Rullgård
2016-11-04 15:36   ` Sebastian Frias
2016-11-04 16:28     ` Florian Fainelli
2016-11-04 16:49       ` Måns Rullgård
2016-11-09 13:02         ` Sebastian Frias
2016-11-09 17:03           ` Florian Fainelli
2016-11-14 13:22             ` Sebastian Frias

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).