| Message ID | 20190522184255.16323-1-tpiepho@impinj.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [net-next,v2,1/8] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 6 warnings, 14 lines checked" |
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
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
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
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
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
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
>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
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.
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.
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.
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.
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.
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.
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.
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
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(+)