[net-next,v2,1/8] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay
diff mbox series

Message ID 20190522184255.16323-1-tpiepho@impinj.com
State Not Applicable
Headers show
Series
  • [net-next,v2,1/8] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 6 warnings, 14 lines checked"

Commit Message

Trent Piepho May 22, 2019, 6:43 p.m. UTC
Add a note to make it more clear how the driver behaves when "rgmii" vs
"rgmii-id", "rgmii-idrx", or "rgmii-idtx" interface modes are selected.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---

Notes:
    Changes from v1:
      Clarify behavior may change to enforce no delay in "rgmii" mode

 Documentation/devicetree/bindings/net/ti,dp83867.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Andrew Lunn May 22, 2019, 6:51 p.m. UTC | #1
On Wed, May 22, 2019 at 06:43:22PM +0000, Trent Piepho wrote:
> Generally, the output clock pin is only used for testing and only serves
> as a source of RF noise after this.  It could be used to daisy-chain
> PHYs, but this is uncommon.  Since the PHY can disable the output, make
> doing so an option.  I do this by adding another enumeration to the
> allowed values of ti,clk-output-sel.
> 
> The code was not using the value DP83867_CLK_O_SEL_REF_CLK as one might
> expect: to select the REF_CLK as the output.  Rather it meant "keep
> clock output setting as is", which, depending on PHY strapping, might
> not be outputting REF_CLK.
> 
> Change this so DP83867_CLK_O_SEL_REF_CLK means enable REF_CLK output.
> Omitting the property will leave the setting as is (which was the
> previous behavior in this case).
> 
> Out of range values were silently converted into
> DP83867_CLK_O_SEL_REF_CLK.  Change this so they generate an error.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 6:54 p.m. UTC | #2
On Wed, May 22, 2019 at 06:43:23PM +0000, Trent Piepho wrote:
> The code was assuming the reset default of the delay control register
> was to have delay disabled.  This is what the datasheet shows as the
> register's initial value.  However, that's not actually true: the
> default is controlled by the PHY's pin strapping.
> 
> If the interface mode is selected as RX or TX delay only, insure the
> other direction's delay is disabled.
> 
> If the interface mode is just "rgmii", with neither TX or RX internal
> delay, one might expect that the driver should disable both delays.  But
> this is not what the driver does.  It leaves the setting at the PHY's
> strapping's default.  And that default, for no pins with strapping
> resistors, is to have delay enabled and 2.00 ns.
> 
> Rather than change this behavior, I've kept it the same and documented
> it.  No delay will most likely not work and will break ethernet on any
> board using "rgmii" mode.  If the board is strapped to have a delay and
> is configured to use "rgmii" mode a warning is generated that "rgmii-id"
> should have been used.
> 
> Also validate the delay values and fail if they are not in range.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 6:59 p.m. UTC | #3
On Wed, May 22, 2019 at 06:43:24PM +0000, Trent Piepho wrote:
> The variables used to store u32 DT properties were signed ints.  This
> doesn't work properly if the value of the property were to overflow.
> Use unsigned variables so this doesn't happen.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 7:03 p.m. UTC | #4
On Wed, May 22, 2019 at 06:43:25PM +0000, Trent Piepho wrote:
> The driver would only set the IO impedance value when RGMII internal
> delays were enabled.  There is no reason for this.  Move the IO
> impedance block out of the RGMII delay block.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 7:05 p.m. UTC | #5
On Wed, May 22, 2019 at 06:43:26PM +0000, Trent Piepho wrote:
> Insure property is in valid range and fail when reading DT if it is not.
> Also add error message for existing failure if required property is not
> present.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 7:09 p.m. UTC | #6
On Wed, May 22, 2019 at 06:43:27PM +0000, Trent Piepho wrote:
> This was being done in config the first time the phy was configured.
> Should be in the probe method.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 22, 2019, 7:10 p.m. UTC | #7
>On Wed, May 22, 2019 at 06:43:19PM +0000, Trent Piepho wrote:
> Add a note to make it more clear how the driver behaves when "rgmii" vs
> "rgmii-id", "rgmii-idrx", or "rgmii-idtx" interface modes are selected.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
David Miller May 23, 2019, 12:43 a.m. UTC | #8
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:19 +0000

> Add a note to make it more clear how the driver behaves when "rgmii" vs
> "rgmii-id", "rgmii-idrx", or "rgmii-idtx" interface modes are selected.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #9
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:22 +0000

> Generally, the output clock pin is only used for testing and only serves
> as a source of RF noise after this.  It could be used to daisy-chain
> PHYs, but this is uncommon.  Since the PHY can disable the output, make
> doing so an option.  I do this by adding another enumeration to the
> allowed values of ti,clk-output-sel.
> 
> The code was not using the value DP83867_CLK_O_SEL_REF_CLK as one might
> expect: to select the REF_CLK as the output.  Rather it meant "keep
> clock output setting as is", which, depending on PHY strapping, might
> not be outputting REF_CLK.
> 
> Change this so DP83867_CLK_O_SEL_REF_CLK means enable REF_CLK output.
> Omitting the property will leave the setting as is (which was the
> previous behavior in this case).
> 
> Out of range values were silently converted into
> DP83867_CLK_O_SEL_REF_CLK.  Change this so they generate an error.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #10
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:23 +0000

> The code was assuming the reset default of the delay control register
> was to have delay disabled.  This is what the datasheet shows as the
> register's initial value.  However, that's not actually true: the
> default is controlled by the PHY's pin strapping.
> 
> If the interface mode is selected as RX or TX delay only, insure the
> other direction's delay is disabled.
> 
> If the interface mode is just "rgmii", with neither TX or RX internal
> delay, one might expect that the driver should disable both delays.  But
> this is not what the driver does.  It leaves the setting at the PHY's
> strapping's default.  And that default, for no pins with strapping
> resistors, is to have delay enabled and 2.00 ns.
> 
> Rather than change this behavior, I've kept it the same and documented
> it.  No delay will most likely not work and will break ethernet on any
> board using "rgmii" mode.  If the board is strapped to have a delay and
> is configured to use "rgmii" mode a warning is generated that "rgmii-id"
> should have been used.
> 
> Also validate the delay values and fail if they are not in range.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #11
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:24 +0000

> The variables used to store u32 DT properties were signed ints.  This
> doesn't work properly if the value of the property were to overflow.
> Use unsigned variables so this doesn't happen.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #12
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:25 +0000

> The driver would only set the IO impedance value when RGMII internal
> delays were enabled.  There is no reason for this.  Move the IO
> impedance block out of the RGMII delay block.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #13
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:26 +0000

> Insure property is in valid range and fail when reading DT if it is not.
> Also add error message for existing failure if required property is not
> present.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.
David Miller May 23, 2019, 12:44 a.m. UTC | #14
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 22 May 2019 18:43:27 +0000

> This was being done in config the first time the phy was configured.
> Should be in the probe method.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>

Applied.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index 9ef9338aaee1..99b8681bde49 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -11,6 +11,14 @@  Required properties:
 	- ti,fifo-depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
 		for applicable values
 
+Note: If the interface type is PHY_INTERFACE_MODE_RGMII the TX/RX clock delays
+      will be left at their default values, as set by the PHY's pin strapping.
+      The default strapping will use a delay of 2.00 ns.  Thus
+      PHY_INTERFACE_MODE_RGMII, by default, does not behave as RGMII with no
+      internal delay, but as PHY_INTERFACE_MODE_RGMII_ID.  The device tree
+      should use "rgmii-id" if internal delays are desired as this may be
+      changed in future to cause "rgmii" mode to disable delays.
+
 Optional property:
 	- ti,min-output-impedance - MAC Interface Impedance control to set
 				    the programmable output impedance to