[PATCH net-next v2 0/4] Documentation: net: phy: Improve documentation

[PATCH net-next v2 0/4] Documentation: net: phy: Improve documentation

[Florian Fainelli]

Hi all,

This patch series addresses discussions and feedback that was recently received
on the mailing-list in the area of: flow control/pause frames, interpretation of
phy_interface_t and finally add some links to useful standards documents.

Changes in v2:

- clarify a few things in the RGMII section, add a paragraph about common issues
  with RGMII delay mismatches

Florian Fainelli (4):
  Documentation: net: phy: remove description of function pointers
  Documentation: net: phy: Add a paragraph about pause frames/flow
    control
  Documentation: net: phy: Add blurb about RGMII
  Documentation: net: phy: Add links to several standards documents

 Documentation/networking/phy.txt | 139 +++++++++++++++++++++++++++++----------
 1 file changed, 104 insertions(+), 35 deletions(-)

-- 
2.9.3

[PATCH net-next v2 1/4] Documentation: net: phy: remove description of function pointers

[Florian Fainelli]

Remove the function pointers documentation which duplicates information
found in include/linux/phy.h. Maintaining documentation about two
different locations just does not work, but the code is less likely to
be outdated.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/networking/phy.txt | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 7ab9404a8412..4b25c0f24201 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -251,39 +251,8 @@ Writing a PHY driver
  PHY_BASIC_FEATURES, but you can look in include/mii.h for other
  features.
 
- Each driver consists of a number of function pointers:
-
-   soft_reset: perform a PHY software reset
-   config_init: configures PHY into a sane state after a reset.
-     For instance, a Davicom PHY requires descrambling disabled.
-   probe: Allocate phy->priv, optionally refuse to bind.
-   PHY may not have been reset or had fixups run yet.
-   suspend/resume: power management
-   config_aneg: Changes the speed/duplex/negotiation settings
-   aneg_done: Determines the auto-negotiation result
-   read_status: Reads the current speed/duplex/negotiation settings
-   ack_interrupt: Clear a pending interrupt
-   did_interrupt: Checks if the PHY generated an interrupt
-   config_intr: Enable or disable interrupts
-   remove: Does any driver take-down
-   ts_info: Queries about the HW timestamping status
-   match_phy_device: used for Clause 45 capable PHYs to match devices
-   in package and ensure they are compatible
-   hwtstamp: Set the PHY HW timestamping configuration
-   rxtstamp: Requests a receive timestamp at the PHY level for a 'skb'
-   txtsamp: Requests a transmit timestamp at the PHY level for a 'skb'
-   set_wol: Enable Wake-on-LAN at the PHY level
-   get_wol: Get the Wake-on-LAN status at the PHY level
-   link_change_notify: called to inform the core is about to change the
-   link state, can be used to work around bogus PHY between state changes
-   read_mmd_indirect: Read PHY MMD indirect register
-   write_mmd_indirect: Write PHY MMD indirect register
-   module_info: Get the size and type of an EEPROM contained in an plug-in
-   module
-   module_eeprom: Get EEPROM information of a plug-in module
-   get_sset_count: Get number of strings sets that get_strings will count
-   get_strings: Get strings from requested objects (statistics)
-   get_stats: Get the extended statistics from the PHY device
+ Each driver consists of a number of function pointers, documented
+ in include/linux/phy.h under the phy_driver structure.
 
  Of these, only config_aneg and read_status are required to be
  assigned by the driver code.  The rest are optional.  Also, it is
-- 
2.9.3

[PATCH net-next v2 2/4] Documentation: net: phy: Add a paragraph about pause frames/flow control

[Florian Fainelli]

Describe that the Ethernet MAC controller is ultimately responsible for
dealing with proper pause frames/flow control advertisement and
enabling, and that it is therefore allowed to have it change
phydev->supported/advertising with SUPPORTED_Pause and
SUPPORTED_AsymPause.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/networking/phy.txt | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 4b25c0f24201..9a42a9414cea 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -127,8 +127,9 @@ Letting the PHY Abstraction Layer do Everything
  values pruned from them which don't make sense for your controller (a 10/100
  controller may be connected to a gigabit capable PHY, so you would need to
  mask off SUPPORTED_1000baseT*).  See include/linux/ethtool.h for definitions
- for these bitfields. Note that you should not SET any bits, or the PHY may
- get put into an unsupported state.
+ for these bitfields. Note that you should not SET any bits, except the
+ SUPPORTED_Pause and SUPPORTED_AsymPause bits (see below), or the PHY may get
+ put into an unsupported state.
 
  Lastly, once the controller is ready to handle network traffic, you call
  phy_start(phydev).  This tells the PAL that you are ready, and configures the
@@ -139,6 +140,19 @@ Letting the PHY Abstraction Layer do Everything
  When you want to disconnect from the network (even if just briefly), you call
  phy_stop(phydev).
 
+Pause frames / flow control
+
+ The PHY does not participate directly in flow control/pause frames except by
+ making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
+ MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
+ controller supports such a thing. Since flow control/pause frames generation
+ involves the Ethernet MAC driver, it is recommended that this driver takes care
+ of properly indicating advertisement and support for such features by setting
+ the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
+ either before or after phy_connect() and/or as a result of implementing the
+ ethtool::set_pauseparam feature.
+
+
 Keeping Close Tabs on the PAL
 
  It is possible that the PAL's built-in state machine needs a little help to
-- 
2.9.3

[PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Florian Fainelli]

RGMII is a recurring source of pain for people with Gigabit Ethernet
hardware since it may require PHY driver and MAC driver level
configuration hints. Document what are the expectations from PHYLIB and
what options exist.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/networking/phy.txt | 76 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 9a42a9414cea..7a0cb1212b9e 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -65,6 +65,82 @@ The MDIO bus
  drivers/net/ethernet/freescale/fsl_pq_mdio.c and an associated DTS file
  for one of the users. (e.g. "git grep fsl,.*-mdio arch/powerpc/boot/dts/")
 
+(RG)MII/electrical interface considerations
+
+ The Reduced Gigabit Medium Independent Interface (RGMII) is a 12 pins
+ electrical signal interface using a synchronous 125Mhz clock signal and several
+ data lines. Due to this design decision, a 1.5ns to 2ns delay must be added
+ between the clock line (RXC or TXC) and the data lines to let the PHY (clock
+ sink) have enough setup and hold times to sample the data lines correctly. The
+ PHY library offers different types of PHY_INTERFACE_MODE_RGMII* values to let
+ the PHY driver and optionaly the MAC driver implement the required delay. The
+ values of phy_interface_t must be understood from the perspective of the PHY
+ device itself, leading to the following:
+
+ * PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
+   internal delay by itself, it assumes that either the Ethernet MAC (if capable
+   or the PCB traces) insert the correct 1.5-2ns delay
+
+ * PHY_INTERFACE_MODE_RGMII_TXID: the PHY should be inserting an internal delay
+   for the transmit data lines (TXD[3:0]) processed by the PHY device
+
+ * PHY_INTERFACE_MODE_RGMII_RXID: the PHY should be inserting an internal delay
+   for the receive data lines (RXD[3:0]) processed by the PHY device
+
+ * PHY_INTERFACE_MODE_RGMII_ID: the PHY should be inserting internal delays for
+   both transmit AND receive data lines from/to the PHY device
+
+ Whenever it is possible, it is preferrable to utilize the PHY side RGMII delay
+ for several reasons:
+
+ * PHY devices may offer sub-nanosecond granularity in how they allow a
+   receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
+   precision may be required to account for differences in PCB trace lengths
+
+ * PHY devices are typically qualified for a large range of applications
+   (industrial, medical, automotive...), and they provide a constant and
+   reliable delay across temperature/pressure/voltage ranges
+
+ * PHY device drivers in PHYLIB being reusable by nature, being able to
+   configure correctly a specified delay enables more designs with similar delay
+   requirements to be operate correctly
+
+ For cases where the PHY is not capable of providing this delay, but the
+ Ethernet MAC driver is capable of doing it, the correct phy_interface_t value
+ should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
+ configured correctly in order to provide the required transmit and/or receive
+ side delay from the perspective of the PHY device. Conversely, if the Ethernet
+ MAC driver looks at the phy_interface_t value, for any other mode but
+ PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
+ disabled.
+
+ In case neither the Ethernet MAC, nor the PHY are capable of providing the
+ required delays, as defined per the RGMII standard, several options may be
+ available:
+
+ * Some SoCs may offer a pin pad/mux/controller capable of configuring a given
+   set of pins' drive strength, delays and voltage, and it may be a suitable
+   option to insert the expected 2ns RGMII delay
+
+ * Modifying the PCB design to include a fixed delay (e.g: using a specifically
+   designed serpentine), which may not require software configuration at all
+
+Common problems with RGMII delay mismatch
+
+ When there is a RGMII delay mismatch between the Ethernet MAC and the PHY, this
+ will most likely result in the clock and data line sampling to capture unstable
+ signals, typical symptoms include:
+
+ * Transmission/reception partially works, and there is frequent or occasional
+   packet loss observed
+
+ * Ethernet MAC may report some, or all packets ingressing with a FCS/CRC error,
+   or just discard them all
+
+ * Switching to lower speeds such as 10/100Mbits/sec makes the problem go away
+   (since there is enough setup/hold time in that case)
+
+
 Connecting to a PHY
 
  Sometime during startup, the network driver needs to establish a connection
-- 
2.9.3

[PATCH net-next v2 4/4] Documentation: net: phy: Add links to several standards documents

[Florian Fainelli]

Add links to the IEEE 802.3-2008 document, and the RGMII v1.3 and v2.0
revisions of the standard.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/networking/phy.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 7a0cb1212b9e..6734cf1f64d4 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -406,3 +406,13 @@ Board Fixups
  The stubs set one of the two matching criteria, and set the other one to
  match anything.
 
+Standards
+
+ IEEE Standard 802.3: CSMA/CD Access Method and Physical Layer Specifications, Section Two:
+ http://standards.ieee.org/getieee802/download/802.3-2008_section2.pdf
+
+ RGMII v1.3:
+ http://web.archive.org/web/20160303212629/http://www.hp.com/rnd/pdfs/RGMIIv1_3.pdf
+
+ RGMII v2.0:
+ http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf
-- 
2.9.3

Re: [PATCH net-next v2 0/4] Documentation: net: phy: Improve documentation

[Martin Blumenstingl]

On Sun, Nov 27, 2016 at 7:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Hi all,
>
> This patch series addresses discussions and feedback that was recently received
> on the mailing-list in the area of: flow control/pause frames, interpretation of
> phy_interface_t and finally add some links to useful standards documents.
>
> Changes in v2:
>
> - clarify a few things in the RGMII section, add a paragraph about common issues
>   with RGMII delay mismatches
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks a lot Florian, this will definitely help others in the future!

> Florian Fainelli (4):
>   Documentation: net: phy: remove description of function pointers
>   Documentation: net: phy: Add a paragraph about pause frames/flow
>     control
>   Documentation: net: phy: Add blurb about RGMII
>   Documentation: net: phy: Add links to several standards documents
>
>  Documentation/networking/phy.txt | 139 +++++++++++++++++++++++++++++----------
>  1 file changed, 104 insertions(+), 35 deletions(-)
>
> --
> 2.9.3
>

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Timur Tabi]

Just some grammatical corrections.  You might want to run a spellchecker 
on all the patches.

Florian Fainelli wrote:
> + The Reduced Gigabit Medium Independent Interface (RGMII) is a 12 pins

"is a 12-pin"

> + electrical signal interface using a synchronous 125Mhz clock signal and several
> + data lines. Due to this design decision, a 1.5ns to 2ns delay must be added
> + between the clock line (RXC or TXC) and the data lines to let the PHY (clock
> + sink) have enough setup and hold times to sample the data lines correctly. The
> + PHY library offers different types of PHY_INTERFACE_MODE_RGMII* values to let
> + the PHY driver and optionaly the MAC driver implement the required delay. The

"driver, and optionally the MAC driver, implement"

> + values of phy_interface_t must be understood from the perspective of the PHY
> + device itself, leading to the following:
> +
> + * PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
> +   internal delay by itself, it assumes that either the Ethernet MAC (if capable
> +   or the PCB traces) insert the correct 1.5-2ns delay
> +
> + * PHY_INTERFACE_MODE_RGMII_TXID: the PHY should be inserting an internal delay

"should insert"


> +   for the transmit data lines (TXD[3:0]) processed by the PHY device
> +
> + * PHY_INTERFACE_MODE_RGMII_RXID: the PHY should be inserting an internal delay

"should insert"


> +   for the receive data lines (RXD[3:0]) processed by the PHY device
> +
> + * PHY_INTERFACE_MODE_RGMII_ID: the PHY should be inserting internal delays for

"should insert"

> +   both transmit AND receive data lines from/to the PHY device
> +
> + Whenever it is possible, it is preferrable to utilize the PHY side RGMII delay
> + for several reasons:

"Whenever possible, use the PHY side RGMII delay for these reasons:"

> + * PHY devices may offer sub-nanosecond granularity in how they allow a
> +   receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
> +   precision may be required to account for differences in PCB trace lengths
> +
> + * PHY devices are typically qualified for a large range of applications
> +   (industrial, medical, automotive...), and they provide a constant and
> +   reliable delay across temperature/pressure/voltage ranges
> +
> + * PHY device drivers in PHYLIB being reusable by nature, being able to
> +   configure correctly a specified delay enables more designs with similar delay
> +   requirements to be operate correctly

Ok, this one I don't know how to fix.  I'm not really sure what you're 
trying to say.

> +
> + For cases where the PHY is not capable of providing this delay, but the
> + Ethernet MAC driver is capable of doing it, the correct phy_interface_t value

"doing so,"

> + should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
> + configured correctly in order to provide the required transmit and/or receive
> + side delay from the perspective of the PHY device. Conversely, if the Ethernet
> + MAC driver looks at the phy_interface_t value, for any other mode but
> + PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
> + disabled.
> +
> + In case neither the Ethernet MAC, nor the PHY are capable of providing the
> + required delays, as defined per the RGMII standard, several options may be
> + available:
> +
> + * Some SoCs may offer a pin pad/mux/controller capable of configuring a given
> +   set of pins' drive strength, delays and voltage, and it may be a suitable

"strength, delays, and voltage; and"

> +   option to insert the expected 2ns RGMII delay
> +
> + * Modifying the PCB design to include a fixed delay (e.g: using a specifically
> +   designed serpentine), which may not require software configuration at all

period after "all".

> +
> +Common problems with RGMII delay mismatch
> +
> + When there is a RGMII delay mismatch between the Ethernet MAC and the PHY, this
> + will most likely result in the clock and data line sampling to capture unstable

I'm not sure what "sampling to capture unstable" is supposed to mean.

> + signals, typical symptoms include:
> +
> + * Transmission/reception partially works, and there is frequent or occasional
> +   packet loss observed
> +
> + * Ethernet MAC may report some, or all packets ingressing with a FCS/CRC error,

No comma after "some".

> +   or just discard them all
> +
> + * Switching to lower speeds such as 10/100Mbits/sec makes the problem go away
> +   (since there is enough setup/hold time in that case)

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Florian Fainelli]

Le 27/11/2016 à 14:24, Timur Tabi a écrit :
>> + * PHY device drivers in PHYLIB being reusable by nature, being able to
>> +   configure correctly a specified delay enables more designs with
>> similar delay
>> +   requirements to be operate correctly
> 
> Ok, this one I don't know how to fix.  I'm not really sure what you're
> trying to say.

What I am trying to say is that once a PHY driver properly configures a
delay that you have specified, there is no reason why this is not
applicable to other platforms using this same PHY driver.

>> +
>> +Common problems with RGMII delay mismatch
>> +
>> + When there is a RGMII delay mismatch between the Ethernet MAC and
>> the PHY, this
>> + will most likely result in the clock and data line sampling to
>> capture unstable
> 
> I'm not sure what "sampling to capture unstable" is supposed to mean.

When the PHY devices takes a "snapshot" of the state of the data lines,
after a clock edge, if the delay is improperly configured, these data
lines are going to still be floating, or show some kind of
capacitance/inductance effect, so the logical level which is going to be
read may be incorrect.
-- 
Florian

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Sebastian Frias]

On 27/11/16 19:44, Florian Fainelli wrote:
> RGMII is a recurring source of pain for people with Gigabit Ethernet
> hardware since it may require PHY driver and MAC driver level
> configuration hints. Document what are the expectations from PHYLIB and
> what options exist.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  Documentation/networking/phy.txt | 76 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
> index 9a42a9414cea..7a0cb1212b9e 100644
> --- a/Documentation/networking/phy.txt
> +++ b/Documentation/networking/phy.txt
> @@ -65,6 +65,82 @@ The MDIO bus
>   drivers/net/ethernet/freescale/fsl_pq_mdio.c and an associated DTS file
>   for one of the users. (e.g. "git grep fsl,.*-mdio arch/powerpc/boot/dts/")
>  
> +(RG)MII/electrical interface considerations
> +
> + The Reduced Gigabit Medium Independent Interface (RGMII) is a 12 pins
> + electrical signal interface using a synchronous 125Mhz clock signal and several
> + data lines. Due to this design decision, a 1.5ns to 2ns delay must be added
> + between the clock line (RXC or TXC) and the data lines to let the PHY (clock
> + sink) have enough setup and hold times to sample the data lines correctly. The
> + PHY library offers different types of PHY_INTERFACE_MODE_RGMII* values to let
> + the PHY driver and optionaly the MAC driver implement the required delay. The
> + values of phy_interface_t must be understood from the perspective of the PHY
> + device itself, leading to the following:
> +
> + * PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
> +   internal delay by itself, it assumes that either the Ethernet MAC (if capable
> +   or the PCB traces) insert the correct 1.5-2ns delay

For what is worth, the Atheros at803x driver comes with RX delay enabled as per HW
reset.
I understand that enforcing this documentation as is would result in changing the
driver to disable RX delay, but it could break existing DTs, so I don't know if that
path will be pursued.

Would explicit "versioning" the DT nodes be something worth exploring? So far it
seems the versioning is implicit: driver probably check the presence of some property
and conclude that it has to behave in a way or another.

> +
> + * PHY_INTERFACE_MODE_RGMII_TXID: the PHY should be inserting an internal delay
> +   for the transmit data lines (TXD[3:0]) processed by the PHY device
> +
> + * PHY_INTERFACE_MODE_RGMII_RXID: the PHY should be inserting an internal delay
> +   for the receive data lines (RXD[3:0]) processed by the PHY device
> +
> + * PHY_INTERFACE_MODE_RGMII_ID: the PHY should be inserting internal delays for
> +   both transmit AND receive data lines from/to the PHY device
> +
> + Whenever it is possible, it is preferrable to utilize the PHY side RGMII delay
> + for several reasons:
> +
> + * PHY devices may offer sub-nanosecond granularity in how they allow a
> +   receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
> +   precision may be required to account for differences in PCB trace lengths
> +
> + * PHY devices are typically qualified for a large range of applications
> +   (industrial, medical, automotive...), and they provide a constant and
> +   reliable delay across temperature/pressure/voltage ranges
> +
> + * PHY device drivers in PHYLIB being reusable by nature, being able to
> +   configure correctly a specified delay enables more designs with similar delay
> +   requirements to be operate correctly
> +
> + For cases where the PHY is not capable of providing this delay, but the
> + Ethernet MAC driver is capable of doing it, the correct phy_interface_t value
> + should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
> + configured correctly in order to provide the required transmit and/or receive
> + side delay from the perspective of the PHY device. 

While this clarifies the current situation very well, wouldn't the proposed approach
require that a property such as "txid-delay-ns" on the PHY's DT node to be duplicated
for the MAC?

>Conversely, if the Ethernet
> + MAC driver looks at the phy_interface_t value, for any other mode but
> + PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
> + disabled.
> +
> + In case neither the Ethernet MAC, nor the PHY are capable of providing the
> + required delays, as defined per the RGMII standard, several options may be
> + available:
> +
> + * Some SoCs may offer a pin pad/mux/controller capable of configuring a given
> +   set of pins' drive strength, delays and voltage, and it may be a suitable
> +   option to insert the expected 2ns RGMII delay
> +
> + * Modifying the PCB design to include a fixed delay (e.g: using a specifically
> +   designed serpentine), which may not require software configuration at all
> +
> +Common problems with RGMII delay mismatch
> +
> + When there is a RGMII delay mismatch between the Ethernet MAC and the PHY, this
> + will most likely result in the clock and data line sampling to capture unstable
> + signals, typical symptoms include:
> +
> + * Transmission/reception partially works, and there is frequent or occasional
> +   packet loss observed
> +
> + * Ethernet MAC may report some, or all packets ingressing with a FCS/CRC error,
> +   or just discard them all
> +
> + * Switching to lower speeds such as 10/100Mbits/sec makes the problem go away
> +   (since there is enough setup/hold time in that case)
> +
> +
>  Connecting to a PHY
>  
>   Sometime during startup, the network driver needs to establish a connection
> 

Re: [PATCH net-next v2 2/4] Documentation: net: phy: Add a paragraph about pause frames/flow control

[Sebastian Frias]

On 27/11/16 19:44, Florian Fainelli wrote:
> Describe that the Ethernet MAC controller is ultimately responsible for
> dealing with proper pause frames/flow control advertisement and
> enabling, and that it is therefore allowed to have it change
> phydev->supported/advertising with SUPPORTED_Pause and
> SUPPORTED_AsymPause.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  Documentation/networking/phy.txt | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
> index 4b25c0f24201..9a42a9414cea 100644
> --- a/Documentation/networking/phy.txt
> +++ b/Documentation/networking/phy.txt
> @@ -127,8 +127,9 @@ Letting the PHY Abstraction Layer do Everything
>   values pruned from them which don't make sense for your controller (a 10/100
>   controller may be connected to a gigabit capable PHY, so you would need to
>   mask off SUPPORTED_1000baseT*).  See include/linux/ethtool.h for definitions
> - for these bitfields. Note that you should not SET any bits, or the PHY may
> - get put into an unsupported state.
> + for these bitfields. Note that you should not SET any bits, except the
> + SUPPORTED_Pause and SUPPORTED_AsymPause bits (see below), or the PHY may get
> + put into an unsupported state.
>  
>   Lastly, once the controller is ready to handle network traffic, you call
>   phy_start(phydev).  This tells the PAL that you are ready, and configures the
> @@ -139,6 +140,19 @@ Letting the PHY Abstraction Layer do Everything
>   When you want to disconnect from the network (even if just briefly), you call
>   phy_stop(phydev).
>  
> +Pause frames / flow control
> +
> + The PHY does not participate directly in flow control/pause frames except by
> + making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
> + MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
> + controller supports such a thing. Since flow control/pause frames generation
> + involves the Ethernet MAC driver, it is recommended that this driver takes care
> + of properly indicating advertisement and support for such features by setting
> + the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
> + either before or after phy_connect() 

If the bits are set after phy_connect(), how does the PHY framework knows there's
an update to the bits? Should some call be made?

and/or as a result of implementing the
> + ethtool::set_pauseparam feature.
> +
> +
>  Keeping Close Tabs on the PAL
>  
>   It is possible that the PAL's built-in state machine needs a little help to
> 

Re: [PATCH net-next v2 2/4] Documentation: net: phy: Add a paragraph about pause frames/flow control

[Florian Fainelli]

On 11/28/2016 02:38 AM, Sebastian Frias wrote:
> On 27/11/16 19:44, Florian Fainelli wrote:
>> Describe that the Ethernet MAC controller is ultimately responsible for
>> dealing with proper pause frames/flow control advertisement and
>> enabling, and that it is therefore allowed to have it change
>> phydev->supported/advertising with SUPPORTED_Pause and
>> SUPPORTED_AsymPause.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  Documentation/networking/phy.txt | 18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
>> index 4b25c0f24201..9a42a9414cea 100644
>> --- a/Documentation/networking/phy.txt
>> +++ b/Documentation/networking/phy.txt
>> @@ -127,8 +127,9 @@ Letting the PHY Abstraction Layer do Everything
>>   values pruned from them which don't make sense for your controller (a 10/100
>>   controller may be connected to a gigabit capable PHY, so you would need to
>>   mask off SUPPORTED_1000baseT*).  See include/linux/ethtool.h for definitions
>> - for these bitfields. Note that you should not SET any bits, or the PHY may
>> - get put into an unsupported state.
>> + for these bitfields. Note that you should not SET any bits, except the
>> + SUPPORTED_Pause and SUPPORTED_AsymPause bits (see below), or the PHY may get
>> + put into an unsupported state.
>>  
>>   Lastly, once the controller is ready to handle network traffic, you call
>>   phy_start(phydev).  This tells the PAL that you are ready, and configures the
>> @@ -139,6 +140,19 @@ Letting the PHY Abstraction Layer do Everything
>>   When you want to disconnect from the network (even if just briefly), you call
>>   phy_stop(phydev).
>>  
>> +Pause frames / flow control
>> +
>> + The PHY does not participate directly in flow control/pause frames except by
>> + making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
>> + MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
>> + controller supports such a thing. Since flow control/pause frames generation
>> + involves the Ethernet MAC driver, it is recommended that this driver takes care
>> + of properly indicating advertisement and support for such features by setting
>> + the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
>> + either before or after phy_connect() 
> 
> If the bits are set after phy_connect(), how does the PHY framework knows there's
> an update to the bits? Should some call be made?

You would most likely either call phy_start() to start the PHY state
machine (again) or have to re-negotiate the link with e.g:
genphy_restart_aneg().
-- 
Florian

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Florian Fainelli]

On 11/28/2016 02:34 AM, Sebastian Frias wrote:
> On 27/11/16 19:44, Florian Fainelli wrote:
>> RGMII is a recurring source of pain for people with Gigabit Ethernet
>> hardware since it may require PHY driver and MAC driver level
>> configuration hints. Document what are the expectations from PHYLIB and
>> what options exist.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  Documentation/networking/phy.txt | 76 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
>> index 9a42a9414cea..7a0cb1212b9e 100644
>> --- a/Documentation/networking/phy.txt
>> +++ b/Documentation/networking/phy.txt
>> @@ -65,6 +65,82 @@ The MDIO bus
>>   drivers/net/ethernet/freescale/fsl_pq_mdio.c and an associated DTS file
>>   for one of the users. (e.g. "git grep fsl,.*-mdio arch/powerpc/boot/dts/")
>>  
>> +(RG)MII/electrical interface considerations
>> +
>> + The Reduced Gigabit Medium Independent Interface (RGMII) is a 12 pins
>> + electrical signal interface using a synchronous 125Mhz clock signal and several
>> + data lines. Due to this design decision, a 1.5ns to 2ns delay must be added
>> + between the clock line (RXC or TXC) and the data lines to let the PHY (clock
>> + sink) have enough setup and hold times to sample the data lines correctly. The
>> + PHY library offers different types of PHY_INTERFACE_MODE_RGMII* values to let
>> + the PHY driver and optionaly the MAC driver implement the required delay. The
>> + values of phy_interface_t must be understood from the perspective of the PHY
>> + device itself, leading to the following:
>> +
>> + * PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
>> +   internal delay by itself, it assumes that either the Ethernet MAC (if capable
>> +   or the PCB traces) insert the correct 1.5-2ns delay
> 
> For what is worth, the Atheros at803x driver comes with RX delay enabled as per HW
> reset.

Always, or is this a behavior possibly defined via a stra-pin that can
be changed?

> I understand that enforcing this documentation as is would result in changing the
> driver to disable RX delay, but it could break existing DTs, so I don't know if that
> path will be pursued.

Which is why the documentation update proposed indicates preferred vs.
mandatory.

> 
> Would explicit "versioning" the DT nodes be something worth exploring? So far it
> seems the versioning is implicit: driver probably check the presence of some property
> and conclude that it has to behave in a way or another.

I would not go that route, can of worms.

> 
>> +
>> + * PHY_INTERFACE_MODE_RGMII_TXID: the PHY should be inserting an internal delay
>> +   for the transmit data lines (TXD[3:0]) processed by the PHY device
>> +
>> + * PHY_INTERFACE_MODE_RGMII_RXID: the PHY should be inserting an internal delay
>> +   for the receive data lines (RXD[3:0]) processed by the PHY device
>> +
>> + * PHY_INTERFACE_MODE_RGMII_ID: the PHY should be inserting internal delays for
>> +   both transmit AND receive data lines from/to the PHY device
>> +
>> + Whenever it is possible, it is preferrable to utilize the PHY side RGMII delay
>> + for several reasons:
>> +
>> + * PHY devices may offer sub-nanosecond granularity in how they allow a
>> +   receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
>> +   precision may be required to account for differences in PCB trace lengths
>> +
>> + * PHY devices are typically qualified for a large range of applications
>> +   (industrial, medical, automotive...), and they provide a constant and
>> +   reliable delay across temperature/pressure/voltage ranges
>> +
>> + * PHY device drivers in PHYLIB being reusable by nature, being able to
>> +   configure correctly a specified delay enables more designs with similar delay
>> +   requirements to be operate correctly
>> +
>> + For cases where the PHY is not capable of providing this delay, but the
>> + Ethernet MAC driver is capable of doing it, the correct phy_interface_t value
>> + should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
>> + configured correctly in order to provide the required transmit and/or receive
>> + side delay from the perspective of the PHY device. 
> 
> While this clarifies the current situation very well, wouldn't the proposed approach
> require that a property such as "txid-delay-ns" on the PHY's DT node to be duplicated
> for the MAC?

The property name can be identical and represent essentially the same
things, but as as described, if the delay is implemented by the PHY,
then the MAC should disable it, conversely if the MAC needs to implement
it, the PHY should not contain any delay property. If both are found,
because the DTS is miswritten, then, the drivers should ignore/error out.

Or are you thinking about the case you described earlier with delays
that are neither 0 or 2, but e.g: 1 and 3 and you still want to have the
end-result be somewhere around 2ns?
-- 
Florian

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Mason]

On 28/11/2016 18:43, Florian Fainelli wrote:

> On 11/28/2016 02:34 AM, Sebastian Frias wrote:
>
>> For what is worth, the Atheros at803x driver comes with RX delay enabled
>> as per HW reset.
> 
> Always, or is this a behavior possibly defined via a stra-pin that can
> be changed?

Here's the data sheet:

  http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf

4.2.25 rgmii rx clock delay control
Offset: 0x00
bit 15: Control bit for rgmii interface rx clock delay:
1 = rgmii rx clock delay enable
0 = rgmii rx clock delay disable
HW Rst. 1
SW Rst. 1

As far as I can tell, rx delay is enabled by default, always.

The "Features" list mentions
"RGMII timing modes support internal delay and external delay on Rx path"
(Not sure about the internal vs external distinction.)

Table 3-6. RGMII AC Characteristics — no Internal Delay
Table 3-7. RGMII AC Characteristics — with internal delay added (Default)

Delay is 2 ns apparently.

There's also
4.2.27 Hib ctrl and rgmii gtx clock delay register
Offset: 0x0B

bits 6:5 Gtx_dly_val
Select the delay of gtx_clk.
00:0.25ns
01:1.3ns
10:2.4ns
11:3.4ns

I don't know what that is used for.
Maybe this is the external vs internal delay?

Regards.

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Florian Fainelli]

On 11/28/2016 11:15 AM, Mason wrote:
> On 28/11/2016 18:43, Florian Fainelli wrote:
> 
>> On 11/28/2016 02:34 AM, Sebastian Frias wrote:
>>
>>> For what is worth, the Atheros at803x driver comes with RX delay enabled
>>> as per HW reset.
>>
>> Always, or is this a behavior possibly defined via a stra-pin that can
>> be changed?
> 
> Here's the data sheet:
> 
>   http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
> 
> 4.2.25 rgmii rx clock delay control
> Offset: 0x00
> bit 15: Control bit for rgmii interface rx clock delay:
> 1 = rgmii rx clock delay enable
> 0 = rgmii rx clock delay disable
> HW Rst. 1
> SW Rst. 1
> 
> As far as I can tell, rx delay is enabled by default, always.
> 
> The "Features" list mentions
> "RGMII timing modes support internal delay and external delay on Rx path"
> (Not sure about the internal vs external distinction.)
> 
> Table 3-6. RGMII AC Characteristics — no Internal Delay
> Table 3-7. RGMII AC Characteristics — with internal delay added (Default)
> 
> Delay is 2 ns apparently.
> 
> There's also
> 4.2.27 Hib ctrl and rgmii gtx clock delay register
> Offset: 0x0B
> 
> bits 6:5 Gtx_dly_val
> Select the delay of gtx_clk.
> 00:0.25ns
> 01:1.3ns
> 10:2.4ns
> 11:3.4ns
> 
> I don't know what that is used for.
> Maybe this is the external vs internal delay?

First, this has little to do with the initial patch series being
discussed now, and second, this still looks like an internal delay
programming, just that it applies to the received transmit clock from
the MAC, that's how I read it though.
-- 
Florian

RE: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[David Laight]

From: Florian Fainelli
> Sent: 27 November 2016 23:03
> Le 27/11/2016  14:24, Timur Tabi a crit :
> >> + * PHY device drivers in PHYLIB being reusable by nature, being able to
> >> +   configure correctly a specified delay enables more designs with
> >> similar delay
> >> +   requirements to be operate correctly
> >
> > Ok, this one I don't know how to fix.  I'm not really sure what you're
> > trying to say.
> 
> What I am trying to say is that once a PHY driver properly configures a
> delay that you have specified, there is no reason why this is not
> applicable to other platforms using this same PHY driver.

As has been stated earlier it can depend on the track lengths on the
board itself.
(Although 1ns is about 1 foot - so track delays of that length are unlikely.)

> >> +Common problems with RGMII delay mismatch
> >> +
> >> + When there is a RGMII delay mismatch between the Ethernet MAC and
> >> the PHY, this
> >> + will most likely result in the clock and data line sampling to
> >> capture unstable
> >
> > I'm not sure what "sampling to capture unstable" is supposed to mean.
> 
> When the PHY devices takes a "snapshot" of the state of the data lines,
> after a clock edge, if the delay is improperly configured, these data
> lines are going to still be floating, or show some kind of
> capacitance/inductance effect, so the logical level which is going to be
> read may be incorrect.

No, the problem is that the data lines are being changed at much the same time
as the clock.
Quite possibly on both the rising and falling edges of the clock.

The actual latching of the data requires the data to be stable for the 'setup'
and 'hold' times of the latch (ie before and after the clock edge).
If the data and clock change at the same time it will be indeterminate whether
the old or new data is latched (the latch output might even oscillate).
The delay is there to ensure that the data isn't changing at the same time as
it is sampled.

At lower speed I suspect that the data only changes on one clock edge and is
sampled on the other.
(FWIW the latest DDR has an additional change in the data half way between
the clock edges!)

	David

Re: [PATCH net-next v2 2/4] Documentation: net: phy: Add a paragraph about pause frames/flow control

[Sebastian Frias]

On 28/11/16 18:33, Florian Fainelli wrote:
> On 11/28/2016 02:38 AM, Sebastian Frias wrote:
>> On 27/11/16 19:44, Florian Fainelli wrote:
>>> Describe that the Ethernet MAC controller is ultimately responsible for
>>> dealing with proper pause frames/flow control advertisement and
>>> enabling, and that it is therefore allowed to have it change
>>> phydev->supported/advertising with SUPPORTED_Pause and
>>> SUPPORTED_AsymPause.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  Documentation/networking/phy.txt | 18 ++++++++++++++++--
>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
>>> index 4b25c0f24201..9a42a9414cea 100644
>>> --- a/Documentation/networking/phy.txt
>>> +++ b/Documentation/networking/phy.txt
>>> @@ -127,8 +127,9 @@ Letting the PHY Abstraction Layer do Everything
>>>   values pruned from them which don't make sense for your controller (a 10/100
>>>   controller may be connected to a gigabit capable PHY, so you would need to
>>>   mask off SUPPORTED_1000baseT*).  See include/linux/ethtool.h for definitions
>>> - for these bitfields. Note that you should not SET any bits, or the PHY may
>>> - get put into an unsupported state.
>>> + for these bitfields. Note that you should not SET any bits, except the
>>> + SUPPORTED_Pause and SUPPORTED_AsymPause bits (see below), or the PHY may get
>>> + put into an unsupported state.
>>>  
>>>   Lastly, once the controller is ready to handle network traffic, you call
>>>   phy_start(phydev).  This tells the PAL that you are ready, and configures the
>>> @@ -139,6 +140,19 @@ Letting the PHY Abstraction Layer do Everything
>>>   When you want to disconnect from the network (even if just briefly), you call
>>>   phy_stop(phydev).
>>>  
>>> +Pause frames / flow control
>>> +
>>> + The PHY does not participate directly in flow control/pause frames except by
>>> + making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
>>> + MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
>>> + controller supports such a thing. Since flow control/pause frames generation
>>> + involves the Ethernet MAC driver, it is recommended that this driver takes care
>>> + of properly indicating advertisement and support for such features by setting
>>> + the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
>>> + either before or after phy_connect() 
>>
>> If the bits are set after phy_connect(), how does the PHY framework knows there's
>> an update to the bits? Should some call be made?
> 
> You would most likely either call phy_start() to start the PHY state
> machine (again) or have to re-negotiate the link with e.g:
> genphy_restart_aneg().
> 

Thanks, I think that would be worth adding to the documentation, right?

Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII

[Måns Rullgård]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Florian Fainelli
>> Sent: 27 November 2016 23:03
>> Le 27/11/2016  14:24, Timur Tabi a crit :
>> >> + * PHY device drivers in PHYLIB being reusable by nature, being able to
>> >> +   configure correctly a specified delay enables more designs with
>> >> similar delay
>> >> +   requirements to be operate correctly
>> >
>> > Ok, this one I don't know how to fix.  I'm not really sure what you're
>> > trying to say.
>> 
>> What I am trying to say is that once a PHY driver properly configures a
>> delay that you have specified, there is no reason why this is not
>> applicable to other platforms using this same PHY driver.
>
> As has been stated earlier it can depend on the track lengths on the
> board itself.
> (Although 1ns is about 1 foot - so track delays of that length are unlikely.)

There could be a delay element.

-- 
Måns Rullgård