[PATCH 1/5] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay

[PATCH 1/5] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay

[Trent Piepho]

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>
---
 Documentation/devicetree/bindings/net/ti,dp83867.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index 9ef9338aaee1..e97f54aeac77 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -11,6 +11,12 @@ 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.
+
 Optional property:
 	- ti,min-output-impedance - MAC Interface Impedance control to set
 				    the programmable output impedance to
-- 
2.14.5

[PATCH 2/5] dt-bindings: phy: dp83867: Add documentation for disabling clock output

[Trent Piepho]

The clock output is generally only used for testing and development and
not used to daisy-chain PHYs.  It's just a source of RF noise afterward.

Add a mux value for "off".  I've added it as another enumeration to the
output property.  In the actual PHY, the mux and the output enable are
independently controllable.  However, it doesn't seem useful to be able
to describe the mux setting when the output is disabled.

Document that PHY's default setting will be left as is if the property
is omitted.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 Documentation/devicetree/bindings/net/ti,dp83867.txt | 6 ++++--
 include/dt-bindings/net/ti-dp83867.h                 | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index e97f54aeac77..95ecdc335e63 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -31,8 +31,10 @@ Optional property:
 				    software needs to take when this pin is
 				    strapped in these modes. See data manual
 				    for details.
-	- ti,clk-output-sel - Muxing option for CLK_OUT pin - see dt-bindings/net/ti-dp83867.h
-				    for applicable values.
+	- ti,clk-output-sel - Muxing option for CLK_OUT pin.  See dt-bindings/net/ti-dp83867.h
+			      for applicable values.  The CLK_OUT pin can also
+			      be disabled by this property.  When omitted, the
+			      PHY's default will be left as is.
 
 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
       exclusive. When both properties are present ti,max-output-impedance
diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h
index 7b1656427cbe..192b79439eb7 100644
--- a/include/dt-bindings/net/ti-dp83867.h
+++ b/include/dt-bindings/net/ti-dp83867.h
@@ -56,4 +56,6 @@
 #define DP83867_CLK_O_SEL_CHN_C_TCLK		0xA
 #define DP83867_CLK_O_SEL_CHN_D_TCLK		0xB
 #define DP83867_CLK_O_SEL_REF_CLK		0xC
+/* Special flag to indicate clock should be off */
+#define DP83867_CLK_O_SEL_OFF			0xFFFFFFFF
 #endif
-- 
2.14.5

[PATCH 3/5] net: phy: dp83867: Add ability to disable output clock

[Trent Piepho]

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>
---
 drivers/net/phy/dp83867.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index fd35131a0c39..420729cd6025 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -68,6 +68,7 @@
 
 #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
 #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
+#define DP83867_IO_MUX_CFG_CLK_O_DISABLE	BIT(6)
 #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
 #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
 
@@ -87,7 +88,8 @@ struct dp83867_private {
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
-	int clk_output_sel;
+	bool set_clk_output;
+	u32 clk_output_sel;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -154,11 +156,16 @@ static int dp83867_of_init(struct phy_device *phydev)
 	/* Optional configuration */
 	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
 				   &dp83867->clk_output_sel);
-	if (ret || dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK)
-		/* Keep the default value if ti,clk-output-sel is not set
-		 * or too high
-		 */
-		dp83867->clk_output_sel = DP83867_CLK_O_SEL_REF_CLK;
+	/* If not set, keep default */
+	if (!ret) {
+		dp83867->set_clk_output = true;
+		if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
+		    dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
+			phydev_err(phydev, "ti,clk-output-sel value %u out of range\n",
+				   dp83867->clk_output_sel);
+			return -EINVAL;
+		}
+	}
 
 	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
 		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
@@ -288,11 +295,20 @@ static int dp83867_config_init(struct phy_device *phydev)
 		dp83867_config_port_mirroring(phydev);
 
 	/* Clock output selection if muxing property is set */
-	if (dp83867->clk_output_sel != DP83867_CLK_O_SEL_REF_CLK)
+	if (dp83867->set_clk_output) {
+		u16 mask = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
+
+		if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) {
+			val = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
+		} else {
+			mask |= DP83867_IO_MUX_CFG_CLK_O_SEL_MASK;
+			val = dp83867->clk_output_sel <<
+			      DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT;
+		}
+
 		phy_modify_mmd(phydev, DP83867_DEVADDR, DP83867_IO_MUX_CFG,
-			       DP83867_IO_MUX_CFG_CLK_O_SEL_MASK,
-			       dp83867->clk_output_sel <<
-			       DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT);
+			       mask, val);
+	}
 
 	return 0;
 }
-- 
2.14.5

[PATCH 4/5] net: phy: dp83867: Disable tx/rx delay when not configured

[Trent Piepho]

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.

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>
---
 drivers/net/phy/dp83867.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 420729cd6025..a46cc9427fb3 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -256,10 +256,16 @@ static int dp83867_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
+	/* If rgmii mode with no internal delay is selected, we do NOT use
+	 * aligned mode as one might expect.  Instead we use the PHY's default
+	 * based on pin strapping.  And the "mode 0" default is to *use*
+	 * internal delay with a value of 7 (2.00 ns).
+	 */
 	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) &&
 	    (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
 		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL);
 
+		val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN);
 		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 			val |= (DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN);
 
-- 
2.14.5

[PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Trent Piepho]

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>
---
 drivers/net/phy/dp83867.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index a46cc9427fb3..edd9e27425e8 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -82,9 +82,9 @@ enum {
 };
 
 struct dp83867_private {
-	int rx_id_delay;
-	int tx_id_delay;
-	int fifo_depth;
+	u32 rx_id_delay;
+	u32 tx_id_delay;
+	u32 fifo_depth;
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
-- 
2.14.5

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Heiner Kallweit]

On 10.05.2019 23:46, 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.
> 
In patch 3 you added a check for DT properties being out of range.
I think this would be good also for the three properties here.
The delay values are only 4 bits wide, so you might also consider
to switch to u8 or u16.

Please note that net-next is closed currently. Please resubmit the
patches once it's open again, and please annotate them properly
with net-next.

> 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>
> ---
>  drivers/net/phy/dp83867.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index a46cc9427fb3..edd9e27425e8 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -82,9 +82,9 @@ enum {
>  };
>  
>  struct dp83867_private {
> -	int rx_id_delay;
> -	int tx_id_delay;
> -	int fifo_depth;
> +	u32 rx_id_delay;
> +	u32 tx_id_delay;
> +	u32 fifo_depth;
>  	int io_impedance;
>  	int port_mirroring;
>  	bool rxctrl_strap_quirk;
> 

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Heiner Kallweit]

On 11.05.2019 12:41, Heiner Kallweit wrote:
> On 10.05.2019 23:46, 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.
>>
> In patch 3 you added a check for DT properties being out of range.
> I think this would be good also for the three properties here.
> The delay values are only 4 bits wide, so you might also consider
> to switch to u8 or u16.
> 
I briefly looked over the rest of the driver. What is plain wrong
is to allocate memory for the private data structure in the
config_init callback. This has to be done in the probe callback.
An example is marvell_probe(). As you seem to work on this driver,
can you provide a patch for this?

> Please note that net-next is closed currently. Please resubmit the
> patches once it's open again, and please annotate them properly
> with net-next.
> 
>> 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>
>> ---
>>  drivers/net/phy/dp83867.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index a46cc9427fb3..edd9e27425e8 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -82,9 +82,9 @@ enum {
>>  };
>>  
>>  struct dp83867_private {
>> -	int rx_id_delay;
>> -	int tx_id_delay;
>> -	int fifo_depth;
>> +	u32 rx_id_delay;
>> +	u32 tx_id_delay;
>> +	u32 fifo_depth;
>>  	int io_impedance;
>>  	int port_mirroring;
>>  	bool rxctrl_strap_quirk;
>>
> 

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Trent Piepho]

On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote:
> On 11.05.2019 12:41, Heiner Kallweit wrote:
> > On 10.05.2019 23:46, 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.
> > > 
> > 
> > In patch 3 you added a check for DT properties being out of range.
> > I think this would be good also for the three properties here.
> > The delay values are only 4 bits wide, so you might also consider
> > to switch to u8 or u16.
> > 
> 
> I briefly looked over the rest of the driver. What is plain wrong
> is to allocate memory for the private data structure in the
> config_init callback. This has to be done in the probe callback.
> An example is marvell_probe(). As you seem to work on this driver,
> can you provide a patch for this?

It only allocates the data once, so it is not a memory leak.  But yes,
totally wrong place to do it.  I can fix this.  It also does not set
the signal line impedance from DT value unless unless also configuring
clock skew, even though they are orthogonal concepts.  And fails to
verify anything read from the DT.

Perhaps you could tell me if the approach I've taken in patch 3, 
"Add ability to disable output clock", and patch 4, "Disable tx/rx
delay when not configured", are considered acceptable?  I can conceive
of arguments for alternate approaches.  I would like to add support for
 these into u-boot too, but typically u-boot follows the kernel DT
bindings, so I want to finalize the kernel DT semantics before sending
patches to u-boot.

> > Please note that net-next is closed currently. Please resubmit the
> > patches once it's open again, and please annotate them properly
> > with net-next.

Sorry, didn't know about this policy.  Been years since I've submitted
net patches.  Is there a description somewhere of how this is done? 
Googling net-next wasn't helpful.  I gather new patches are only
allowed when the kernel merge window is open?  And they can't be queued
on patchwork or a topic branch until this happens?

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Heiner Kallweit]

On 13.05.2019 21:58, Trent Piepho wrote:
> On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote:
>> On 11.05.2019 12:41, Heiner Kallweit wrote:
>>> On 10.05.2019 23:46, 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.
>>>>
>>>
>>> In patch 3 you added a check for DT properties being out of range.
>>> I think this would be good also for the three properties here.
>>> The delay values are only 4 bits wide, so you might also consider
>>> to switch to u8 or u16.
>>>
>>
>> I briefly looked over the rest of the driver. What is plain wrong
>> is to allocate memory for the private data structure in the
>> config_init callback. This has to be done in the probe callback.
>> An example is marvell_probe(). As you seem to work on this driver,
>> can you provide a patch for this?
> 
> It only allocates the data once, so it is not a memory leak.  But yes,
> totally wrong place to do it.  I can fix this.  It also does not set
> the signal line impedance from DT value unless unless also configuring
> clock skew, even though they are orthogonal concepts.  And fails to
> verify anything read from the DT.
> 
> Perhaps you could tell me if the approach I've taken in patch 3, 
> "Add ability to disable output clock", and patch 4, "Disable tx/rx
> delay when not configured", are considered acceptable?  I can conceive
> of arguments for alternate approaches.  I would like to add support for
>  these into u-boot too, but typically u-boot follows the kernel DT
> bindings, so I want to finalize the kernel DT semantics before sending
> patches to u-boot.
> 
I lack experience with these TI PHY's. Maybe Andrew or Florian can advise.

>>> Please note that net-next is closed currently. Please resubmit the
>>> patches once it's open again, and please annotate them properly
>>> with net-next.
> 
> Sorry, didn't know about this policy.  Been years since I've submitted
> net patches.  Is there a description somewhere of how this is done? 
> Googling net-next wasn't helpful.  I gather new patches are only
> allowed when the kernel merge window is open?  And they can't be queued
> on patchwork or a topic branch until this happens?
> 
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

And the easy way to check whether net-next is open:
http://vger.kernel.org/~davem/net-next.html

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Andrew Lunn]

> > Perhaps you could tell me if the approach I've taken in patch 3, 
> > "Add ability to disable output clock", and patch 4, "Disable tx/rx
> > delay when not configured", are considered acceptable?  I can conceive
> > of arguments for alternate approaches.  I would like to add support for
> >  these into u-boot too, but typically u-boot follows the kernel DT
> > bindings, so I want to finalize the kernel DT semantics before sending
> > patches to u-boot.
> > 
> I lack experience with these TI PHY's. Maybe Andrew or Florian can advise.

Hi Trent

I already deleted the patches. For patch 3:

+ 	  if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
+	         dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
+		 	phydev_err(phydev, "ti,clk-output-sel value %u out of range\n",
+				   dp83867->clk_output_sel);
+			return -EINVAL;
+													       }

This last bit looks odd. If it is not OFF, it is invalid?

Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
be careful changing its meaning. But if nobody is actually using it...

Patch 4:

This is harder. Ideally we want to fix this. At some point, somebody
is going to want 'rgmii' to actually mean 'rgmii', because that is
what their hardware needs.

Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
delay? And add a comment about setting the correct thing in device
tree?  Hopefully we will then get patches correcting DT blobs. And if
we later do need to fix 'rgmii', we will break less board.

> >>> Please note that net-next is closed currently. Please resubmit the
> >>> patches once it's open again, and please annotate them properly
> >>> with net-next.
> > 
> > Sorry, didn't know about this policy.  Been years since I've submitted
> > net patches.  Is there a description somewhere of how this is done? 
> > Googling net-next wasn't helpful.  I gather new patches are only
> > allowed when the kernel merge window is open?  And they can't be queued
> > on patchwork or a topic branch until this happens?

You can post patches while it is closed for review, but add "RFC" in
the subject so it is clear you just want comments. You will still need
to resubmit once it opens up again.

    Andrew

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Trent Piepho]

On Mon, 2019-05-13 at 22:46 +0200, Andrew Lunn wrote:
> > > Perhaps you could tell me if the approach I've taken in patch 3, 
> > > "Add ability to disable output clock", and patch 4, "Disable tx/rx
> > > delay when not configured", are considered acceptable?  I can conceive
> > > of arguments for alternate approaches.  I would like to add support for
> > >  these into u-boot too, but typically u-boot follows the kernel DT
> > > bindings, so I want to finalize the kernel DT semantics before sending
> > > patches to u-boot.
> > > 
> > 
> > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise.
> 
> Hi Trent
> 
> I already deleted the patches. For patch 3:
> 
> + 	  if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
> +	         dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
> +		 	phydev_err(phydev, "ti,clk-output-sel value %u out of range\n",
> +				   dp83867->clk_output_sel);
> +			return -EINVAL;
> +		      }
> 
> This last bit looks odd. If it is not OFF, it is invalid?

The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and
also DP83867_CLK_O_SEL_OFF.  Thus invalid values are those greater than
DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF.

> 
> Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> be careful changing its meaning. But if nobody is actually using it...

Nope.  I doubt this will affect anyone.  They'd need to strap the phy
to get a different configuration, and the explicitly add a property,
which isn't in the example DTS files, to change the configuration to
something they didn't want, and then depend on a driver bug ignoring
the erroneous setting they added.

> Patch 4:
> 
> This is harder. Ideally we want to fix this. At some point, somebody
> is going to want 'rgmii' to actually mean 'rgmii', because that is
> what their hardware needs.
> 
> Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> delay? And add a comment about setting the correct thing in device
> tree?  Hopefully we will then get patches correcting DT blobs. And if
> we later do need to fix 'rgmii', we will break less board.

Yes I can do this.  Should it warn on any use of "rgmii"?  If so, how
would someone make the warning go away if they actually want rgmii mode
with no delay?

I suspect hsdk.dts is an example of an in-tree broken board that uses
"rgmii" would it should have used "rgmii-id".  Unfortunately, phy dts
nodes don't usually provide a compatible that identifies the phy, using
instead run-time auto-detection based on PHY id registers, so there's
no way to tell from the dts files what boards use this phy unless they
also specify one of the phy specific properties.  Which is how I found
hsdk.dts (and no other boards).

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Andrew Lunn]

> > Hi Trent
> > 
> > I already deleted the patches. For patch 3:
> > 
> > + 	  if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
> > +	         dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
> > +		 	phydev_err(phydev, "ti,clk-output-sel value %u out of range\n",
> > +				   dp83867->clk_output_sel);
> > +			return -EINVAL;
> > +		      }
> > 
> > This last bit looks odd. If it is not OFF, it is invalid?
> 
> The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and
> also DP83867_CLK_O_SEL_OFF.  Thus invalid values are those greater than
> DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF.

Hi Trent
 
O.K.

> > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> > be careful changing its meaning. But if nobody is actually using it...
> 
> Nope.  I doubt this will affect anyone.  They'd need to strap the phy
> to get a different configuration, and the explicitly add a property,
> which isn't in the example DTS files, to change the configuration to
> something they didn't want, and then depend on a driver bug ignoring
> the erroneous setting they added.

O.K, then this patch is O.K. Does the binding documentation need
updating?
 
> > Patch 4:
> > 
> > This is harder. Ideally we want to fix this. At some point, somebody
> > is going to want 'rgmii' to actually mean 'rgmii', because that is
> > what their hardware needs.
> > 
> > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> > delay? And add a comment about setting the correct thing in device
> > tree?  Hopefully we will then get patches correcting DT blobs. And if
> > we later do need to fix 'rgmii', we will break less board.
> 
> Yes I can do this.  Should it warn on any use of "rgmii"?

No, i would only warn when there is a delay configured by
strapping. If you want the PHY to be left alone, you should use
PHY_INTERFACE_MODE_NA, which should be the default if there is no
phy-mode property. If DT actually asked for "rgmii", it either means
it is wrong and rgmii-id should be used to match the strapping, or
both the strapping and the DT is wrong and somebody really does want
"rgmii".

> If so, how would someone make the warning go away if they actually
> want rgmii mode with no delay?

We take the warning out, and implement "rgmii" correctly, and let
boards break which have broken DT. We have done this before, but
without a period of time with a warning.

> I suspect hsdk.dts is an example of an in-tree broken board that uses
> "rgmii" would it should have used "rgmii-id".

O.K, so when you submit the patch Cc: Alexey Brodkin <abrodkin@synopsys.com>

     Andrew

Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Trent Piepho]

On Mon, 2019-05-13 at 23:43 +0200, Andrew Lunn wrote:
> > > 
> > > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> > > be careful changing its meaning. But if nobody is actually using it...
> > 
> > Nope.  I doubt this will affect anyone.  They'd need to strap the phy
> > to get a different configuration, and the explicitly add a property,
> > which isn't in the example DTS files, to change the configuration to
> > something they didn't want, and then depend on a driver bug ignoring
> > the erroneous setting they added.
> 
> O.K, then this patch is O.K. Does the binding documentation need
> updating?

Device tree binding patch was split out of the commit and was patch 2
of the series, https://patchwork.ozlabs.org/patch/1098349/

> > > Patch 4:
> > > 
> > > This is harder. Ideally we want to fix this. At some point, somebody
> > > is going to want 'rgmii' to actually mean 'rgmii', because that is
> > > what their hardware needs.
> > > 
> > > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> > > delay? And add a comment about setting the correct thing in device
> > > tree?  Hopefully we will then get patches correcting DT blobs. And if
> > > we later do need to fix 'rgmii', we will break less board.
> > 
> > Yes I can do this.  Should it warn on any use of "rgmii"?
> 
> No, i would only warn when there is a delay configured by
> strapping. If you want the PHY to be left alone, you should use
> PHY_INTERFACE_MODE_NA, which should be the default if there is no
> phy-mode property. If DT actually asked for "rgmii", it either means
> it is wrong and rgmii-id should be used to match the strapping, or
> both the strapping and the DT is wrong and somebody really does want
> "rgmii".

Ok, seems reasonable.  I've put in a phydev_warn() when the interface
is 'rgmii' and the strapping is set to have a delay.  I'm checking the
strapping config register for this, rather than the current phy
configuration of delay values.  The previous behavior of 'rgmii' mode
was "keep strapping default", rather than "keep current phy
configuration", which isn't exactly the same thing.

Here is the message:

phydev_warn(phydev,
            "PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
            "Should be 'rgmii-id' to use internal delays\n");