[PATCH 2/2] net: phy: dp83867: Add SGMII mode type switching

[PATCH 2/2] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

This patch adds ability to switch beetween two PHY SGMII modes.
Some hardware, for example, FPGA IP designs may use 6-wire mode
which enables differential SGMII clock to MAC.

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
 drivers/net/phy/dp83867.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 1f1ecee0ee2f..3efb33e7523f 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -37,6 +37,7 @@
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)
 
@@ -61,6 +62,9 @@
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
 
+/* SGMIICTL bits */
+#define DP83867_SGMII_TYPE		BIT(14)
+
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)
 
@@ -109,6 +113,7 @@ struct dp83867_private {
 	bool rxctrl_strap_quirk;
 	bool set_clk_output;
 	u32 clk_output_sel;
+	bool sgmii_type;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -197,6 +202,8 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 					"ti,dp83867-rxctrl-strap-quirk");
 
+	dp83867->sgmii_type = of_property_read_bool(of_node, "ti,sgmii-type");
+
 	/* Existing behavior was to use default pin strapping delay in rgmii
 	 * mode, but rgmii should have meant no delay.  Warn existing users.
 	 */
@@ -389,6 +396,14 @@ static int dp83867_config_init(struct phy_device *phydev)
 
 		if (ret)
 			return ret;
+
+		/* SGMII type is set to 4-wire mode by default */
+		if (dp83867->sgmii_type) {
+			/* Switch-on 6-wire mode */
+			val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
+			val |= DP83867_SGMII_TYPE;
+			phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
+		}
 	}
 
 	/* Enable Interrupt output INT_OE in CFG3 register */
-- 
2.16.4

[PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

Add documentation of ti,sgmii-type which can be used to select
SGMII mode type (4 or 6-wire).

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
 Documentation/devicetree/bindings/net/ti,dp83867.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index db6aa3f2215b..18e7fd52897f 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -37,6 +37,7 @@ Optional property:
 			      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.
+	- ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or 6-wire).
 
 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
       exclusive. When both properties are present ti,max-output-impedance
-- 
2.16.4

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Trent Piepho]

On Thu, 2019-09-05 at 19:26 +0300, Vitaly Gaiduk wrote:
> Add documentation of ti,sgmii-type which can be used to select
> SGMII mode type (4 or 6-wire).
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83867.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> index db6aa3f2215b..18e7fd52897f 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> @@ -37,6 +37,7 @@ Optional property:
>  			      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.
> +	- ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or 6-wire).

Really should explain what kind of value it is and what the values
mean.  I.e., should this be ti,sgimii-type = <4> to select 4 wire?

Maybe a boolean, "sgmii-clock", to indicate the presence of sgmii rx
clock lines, would make more sense?

I also wonder if phy-mode = "sgmii-clk" or "sgmii-6wire", vs the
existing phy-mode = "sgmii", might also be a better way to describe
this instead of a new property.

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Andrew Lunn]

On Thu, Sep 05, 2019 at 07:26:00PM +0300, Vitaly Gaiduk wrote:
> Add documentation of ti,sgmii-type which can be used to select
> SGMII mode type (4 or 6-wire).

Hi Vitaly

Is 4 vs 6-wire a generic SGMII property? Or is it proprietary to TI?

I did a quick search and i could not find any other PHYs supporting
it.

	Andrew

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

Hi, Andrew.

I'm not familiar with generic PHY HW archs but suppose that it is 
proprietary to TI.

I'v never seen such feature so moved it in TI dts field.

Vitaly.

06.09.2019 22:29, Andrew Lunn wrote:
> On Thu, Sep 05, 2019 at 07:26:00PM +0300, Vitaly Gaiduk wrote:
>> Add documentation of ti,sgmii-type which can be used to select
>> SGMII mode type (4 or 6-wire).
> Hi Vitaly
>
> Is 4 vs 6-wire a generic SGMII property? Or is it proprietary to TI?
>
> I did a quick search and i could not find any other PHYs supporting
> it.
>
> 	Andrew

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[David Miller]

From: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
Date: Thu,  5 Sep 2019 19:26:00 +0300

> +	- ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or 6-wire).

You need to document this more sufficiently as per Andrew's feedback.

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Andrew Lunn]

On Thu, Sep 05, 2019 at 07:26:00PM +0300, Vitaly Gaiduk wrote:
> Add documentation of ti,sgmii-type which can be used to select
> SGMII mode type (4 or 6-wire).
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83867.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> index db6aa3f2215b..18e7fd52897f 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> @@ -37,6 +37,7 @@ Optional property:
>  			      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.
> +	- ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or 6-wire).

Hi Vitaly

You probably want to make this a Boolean. I don't think SGMII type is
a good idea. This is about enabling the receive clock to be passed to
the MAC. So how about ti,sgmii-ref-clock-output-enable.

    Andrew

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Florian Fainelli]

On 9/6/2019 1:45 PM, Vitaly Gaiduk wrote:
> Hi, Andrew.
> 
> I'm not familiar with generic PHY HW archs but suppose that it is
> proprietary to TI.
> 
> I'v never seen such feature so moved it in TI dts field.

My search engine results seem to indicate that this is indeed TI
specific only and this is not something that exists with other vendors
apparently. The "ti," prefix is therefore appropriate.
-- 
Florian

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Andrew Lunn]

On Sun, Sep 08, 2019 at 01:47:19AM +0300, Vitaly Gaiduk wrote:
> Hi, Andrew.<div>I’m ready to do this property with such name but is it good practice to do such long names? :)</div><div>Also, Trent Piepho wrote about sgmii-clk and merged all ideas we have “ti,sgmii-ref-clk”.</div><div>It’s better, isn’t it?</div><div>Vitaly.</div><div><div><br />07.09.2019, 18:39, "Andrew Lunn" &lt;andrew@lunn.ch&gt;:<br /><blockquote><p>On Thu, Sep 05, 2019 at 07:26:00PM +0300, Vitaly Gaiduk wrote:<br /></p><blockquote class="b4fd5cf2ec92bc68cb898700bb81355fwmi-quote"> Add documentation of ti,sgmii-type which can be used to select<br /> SGMII mode type (4 or 6-wire).<br /><br /> Signed-off-by: Vitaly Gaiduk &lt;<a href="mailto:vitaly.gaiduk@cloudbear.ru">vitaly.gaiduk@cloudbear.ru</a>&gt;<br /> ---<br />  Documentation/devicetree/bindings/net/ti,dp83867.txt | 1 +<br />  1 file changed, 1 insertion(+)<br /><br /> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt<br /> index db6aa3f2215b..18e7fd52897f 100644<br /> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt<br /> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt<br /> @@ -37,6 +37,7 @@ Optional property:<br />                                for applicable values.  The CLK_OUT pin can also<br />                                be disabled by this property.  When omitted, the<br />                                PHY's default will be left as is.<br /> +	- ti,sgmii-type - This denotes the fact which SGMII mode is used (4 or 6-wire).<br /></blockquote><p><br />Hi Vitaly<br /><br />You probably want to make this a Boolean. I don't think SGMII type is<br />a good idea. This is about enabling the receive clock to be passed to<br />the MAC. So how about ti,sgmii-ref-clock-output-enable.<br /><br />    Andrew<br /></p></blockquote></div></div>

Hi Vitaly

Please reconfigure your mail client to not obfuscate with HTML.

The length should be O.K. For a PHY node, it should not be too deeply
indented, unless it happens to be part of an Ethernet switch.

	  Andrew

[PATCH v2 2/2] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

This patch adds ability to switch beetween two PHY SGMII modes.
Some hardware, for example, FPGA IP designs may use 6-wire mode
which enables differential SGMII clock to MAC.

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
Changes in v2:
- changed variable sgmii_type name to sgmii_ref_clk_en

 drivers/net/phy/dp83867.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 1f1ecee..cd6260e 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -37,6 +37,7 @@
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)

@@ -61,6 +62,9 @@
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)

+/* SGMIICTL bits */
+#define DP83867_SGMII_TYPE		BIT(14)
+
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)

@@ -109,6 +113,7 @@ struct dp83867_private {
 	bool rxctrl_strap_quirk;
 	bool set_clk_output;
 	u32 clk_output_sel;
+	bool sgmii_ref_clk_en;
 };

 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 					"ti,dp83867-rxctrl-strap-quirk");

+	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
+					"ti,sgmii-ref-clock-output-enable");
+
 	/* Existing behavior was to use default pin strapping delay in rgmii
 	 * mode, but rgmii should have meant no delay.  Warn existing users.
 	 */
@@ -389,6 +397,14 @@ static int dp83867_config_init(struct phy_device *phydev)

 		if (ret)
 			return ret;
+
+		/* SGMII type is set to 4-wire mode by default */
+		if (dp83867->sgmii_ref_clk_en) {
+			/* Switch on 6-wire mode */
+			val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
+			val |= DP83867_SGMII_TYPE;
+			phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
+		}
 	}

 	/* Enable Interrupt output INT_OE in CFG3 register */
--
2.7.4

[PATCH v2 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

Add documentation of ti,sgmii-ref-clock-output-enable
which can be used to select SGMII mode type (4 or 6-wire).

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
Changes in v2:
- renamed ti,sgmii-type to ti,sgmii-ref-clock-output-enable
  and extended description

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

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index db6aa3f..c98c682 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -37,6 +37,10 @@ Optional property:
 			      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.
+	- ti,sgmii-ref-clock-output-enable - This denotes the fact which
+				    SGMII configuration is used (4 or 6-wire modes).
+				    Some MACs work with differential SGMII clock.
+				    See data manual for details.

 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
       exclusive. When both properties are present ti,max-output-impedance
--
2.7.4

Re: [PATCH 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

I've done required changes. Sorry for HTML in previous mail I sent it 
from mobile app which has not disabling HTML.

Vitaly.

Re: [PATCH v2 2/2] net: phy: dp83867: Add SGMII mode type switching

[Florian Fainelli]

On 9/9/2019 4:02 AM, Vitaly Gaiduk wrote:
> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> ---
> Changes in v2:
> - changed variable sgmii_type name to sgmii_ref_clk_en
> 
>  drivers/net/phy/dp83867.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 1f1ecee..cd6260e 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -37,6 +37,7 @@
>  #define DP83867_STRAP_STS2	0x006f
>  #define DP83867_RGMIIDCTL	0x0086
>  #define DP83867_IO_MUX_CFG	0x0170
> +#define DP83867_SGMIICTL	0x00D3
>  #define DP83867_10M_SGMII_CFG   0x016F
>  #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)
> 
> @@ -61,6 +62,9 @@
>  #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
>  #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
> 
> +/* SGMIICTL bits */
> +#define DP83867_SGMII_TYPE		BIT(14)
> +
>  /* STRAP_STS1 bits */
>  #define DP83867_STRAP_STS1_RESERVED		BIT(11)
> 
> @@ -109,6 +113,7 @@ struct dp83867_private {
>  	bool rxctrl_strap_quirk;
>  	bool set_clk_output;
>  	u32 clk_output_sel;
> +	bool sgmii_ref_clk_en;
>  };
> 
>  static int dp83867_ack_interrupt(struct phy_device *phydev)
> @@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
>  	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
>  					"ti,dp83867-rxctrl-strap-quirk");
> 
> +	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
> +					"ti,sgmii-ref-clock-output-enable");
> +
>  	/* Existing behavior was to use default pin strapping delay in rgmii
>  	 * mode, but rgmii should have meant no delay.  Warn existing users.
>  	 */
> @@ -389,6 +397,14 @@ static int dp83867_config_init(struct phy_device *phydev)
> 
>  		if (ret)
>  			return ret;
> +
> +		/* SGMII type is set to 4-wire mode by default */
> +		if (dp83867->sgmii_ref_clk_en) {
> +			/* Switch on 6-wire mode */
> +			val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
> +			val |= DP83867_SGMII_TYPE;
> +			phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
> +		}

Is there a case where the value could be retained across a power
on/reset cycle and you would want to make sure you do write the intended
"wire mode" here? What I am suggesting is just changing this into a:

	val =  phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
	if (dp83867->sgmii_ref_clk_en)
		val |= DP83867_SGMII_TYPE;
	else
		val &= ~DP83867_SGMII_TYPE;
	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
	
Other than that, LGTM
-- 
Florian

[PATCH v3] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

This patch adds ability to switch beetween two PHY SGMII modes.
Some hardware, for example, FPGA IP designs may use 6-wire mode
which enables differential SGMII clock to MAC.

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
Changes in v3:
- Fixed retaining DP83867_SGMII_TYPE bit

 drivers/net/phy/dp83867.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 1f1ecee..37fceaf 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -37,6 +37,7 @@
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)

@@ -61,6 +62,9 @@
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)

+/* SGMIICTL bits */
+#define DP83867_SGMII_TYPE		BIT(14)
+
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)

@@ -109,6 +113,7 @@ struct dp83867_private {
 	bool rxctrl_strap_quirk;
 	bool set_clk_output;
 	u32 clk_output_sel;
+	bool sgmii_ref_clk_en;
 };

 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 					"ti,dp83867-rxctrl-strap-quirk");

+	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
+					"ti,sgmii-ref-clock-output-enable");
+
 	/* Existing behavior was to use default pin strapping delay in rgmii
 	 * mode, but rgmii should have meant no delay.  Warn existing users.
 	 */
@@ -389,6 +397,17 @@ static int dp83867_config_init(struct phy_device *phydev)

 		if (ret)
 			return ret;
+
+		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
+		/* SGMII type is set to 4-wire mode by default.
+		 * If we place appropriate property in dts (see above)
+		 * switch on 6-wire mode.
+		 */
+		if (dp83867->sgmii_ref_clk_en)
+			val |= DP83867_SGMII_TYPE;
+		else
+			val &= ~DP83867_SGMII_TYPE;
+		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
 	}

 	/* Enable Interrupt output INT_OE in CFG3 register */
--
2.7.4

Re: [PATCH v3] net: phy: dp83867: Add SGMII mode type switching

[Florian Fainelli]

On 9/9/19 9:02 AM, Vitaly Gaiduk wrote:
> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>

Thanks for addressing my comments, please re-post the entire patch
series per:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst#n134

Thanks!
-- 
Florian

[PATCH v3 2/2] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

This patch adds ability to switch beetween two PHY SGMII modes.
Some hardware, for example, FPGA IP designs may use 6-wire mode
which enables differential SGMII clock to MAC.

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
Changes in v3:
- Fixed retaining DP83867_SGMII_TYPE bit

 drivers/net/phy/dp83867.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 1f1ecee..37fceaf 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -37,6 +37,7 @@
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)

@@ -61,6 +62,9 @@
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)

+/* SGMIICTL bits */
+#define DP83867_SGMII_TYPE		BIT(14)
+
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)

@@ -109,6 +113,7 @@ struct dp83867_private {
 	bool rxctrl_strap_quirk;
 	bool set_clk_output;
 	u32 clk_output_sel;
+	bool sgmii_ref_clk_en;
 };

 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 					"ti,dp83867-rxctrl-strap-quirk");

+	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
+					"ti,sgmii-ref-clock-output-enable");
+
 	/* Existing behavior was to use default pin strapping delay in rgmii
 	 * mode, but rgmii should have meant no delay.  Warn existing users.
 	 */
@@ -389,6 +397,17 @@ static int dp83867_config_init(struct phy_device *phydev)

 		if (ret)
 			return ret;
+
+		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
+		/* SGMII type is set to 4-wire mode by default.
+		 * If we place appropriate property in dts (see above)
+		 * switch on 6-wire mode.
+		 */
+		if (dp83867->sgmii_ref_clk_en)
+			val |= DP83867_SGMII_TYPE;
+		else
+			val &= ~DP83867_SGMII_TYPE;
+		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
 	}

 	/* Enable Interrupt output INT_OE in CFG3 register */
--
2.7.4

[PATCH v3 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

Add documentation of ti,sgmii-ref-clock-output-enable
which can be used to select SGMII mode type (4 or 6-wire).

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
 Documentation/devicetree/bindings/net/ti,dp83867.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index db6aa3f..c98c682 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -37,6 +37,10 @@ Optional property:
 			      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.
+	- ti,sgmii-ref-clock-output-enable - This denotes the fact which
+				    SGMII configuration is used (4 or 6-wire modes).
+				    Some MACs work with differential SGMII clock.
+				    See data manual for details.
 
 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
       exclusive. When both properties are present ti,max-output-impedance
-- 
2.7.4

Re: [PATCH v3 2/2] net: phy: dp83867: Add SGMII mode type switching

[Florian Fainelli]

On 9/9/19 9:52 AM, Vitaly Gaiduk wrote:
> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Florian Fainelli]

On 9/9/19 9:52 AM, Vitaly Gaiduk wrote:
> Add documentation of ti,sgmii-ref-clock-output-enable
> which can be used to select SGMII mode type (4 or 6-wire).
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83867.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> index db6aa3f..c98c682 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
> @@ -37,6 +37,10 @@ Optional property:
>  			      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.
> +	- ti,sgmii-ref-clock-output-enable - This denotes the fact which
> +				    SGMII configuration is used (4 or 6-wire modes).
> +				    Some MACs work with differential SGMII clock.
> +				    See data manual for details.

The wording is a bit odd here, I would just omit "the fact" to make the
sentence more readable. With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v4 2/2] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

This patch adds ability to switch beetween two PHY SGMII modes.
Some hardware, for example, FPGA IP designs may use 6-wire mode
which enables differential SGMII clock to MAC.

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
 drivers/net/phy/dp83867.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 1f1ecee..37fceaf 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -37,6 +37,7 @@
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)
 
@@ -61,6 +62,9 @@
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
 
+/* SGMIICTL bits */
+#define DP83867_SGMII_TYPE		BIT(14)
+
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)
 
@@ -109,6 +113,7 @@ struct dp83867_private {
 	bool rxctrl_strap_quirk;
 	bool set_clk_output;
 	u32 clk_output_sel;
+	bool sgmii_ref_clk_en;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -197,6 +202,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 					"ti,dp83867-rxctrl-strap-quirk");
 
+	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
+					"ti,sgmii-ref-clock-output-enable");
+
 	/* Existing behavior was to use default pin strapping delay in rgmii
 	 * mode, but rgmii should have meant no delay.  Warn existing users.
 	 */
@@ -389,6 +397,17 @@ static int dp83867_config_init(struct phy_device *phydev)
 
 		if (ret)
 			return ret;
+
+		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
+		/* SGMII type is set to 4-wire mode by default.
+		 * If we place appropriate property in dts (see above)
+		 * switch on 6-wire mode.
+		 */
+		if (dp83867->sgmii_ref_clk_en)
+			val |= DP83867_SGMII_TYPE;
+		else
+			val &= ~DP83867_SGMII_TYPE;
+		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);
 	}
 
 	/* Enable Interrupt output INT_OE in CFG3 register */
-- 
2.7.4

[PATCH v4 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[Vitaly Gaiduk]

Add documentation of ti,sgmii-ref-clock-output-enable
which can be used to select SGMII mode type (4 or 6-wire).

Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
---
Changes in v4:
- Fixed the wording of property

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

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index db6aa3f..388ff48 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -37,6 +37,10 @@ Optional property:
 			      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.
+	- ti,sgmii-ref-clock-output-enable - This denotes which
+				    SGMII configuration is used (4 or 6-wire modes).
+				    Some MACs work with differential SGMII clock.
+				    See data manual for details.

 Note: ti,min-output-impedance and ti,max-output-impedance are mutually
       exclusive. When both properties are present ti,max-output-impedance
--
2.7.4

Re: [PATCH v4 2/2] net: phy: dp83867: Add SGMII mode type switching

[Trent Piepho]

On Mon, 2019-09-09 at 20:19 +0300, Vitaly Gaiduk wrote:
> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> +
> +		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL);
> +		/* SGMII type is set to 4-wire mode by default.
> +		 * If we place appropriate property in dts (see above)
> +		 * switch on 6-wire mode.
> +		 */
> +		if (dp83867->sgmii_ref_clk_en)
> +			val |= DP83867_SGMII_TYPE;
> +		else
> +			val &= ~DP83867_SGMII_TYPE;
> +		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val);

Should use phy_modify_mmd().

Re: [PATCH v4 1/2] net: phy: dp83867: Add documentation for SGMII mode type

[David Miller]

From: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
Date: Mon,  9 Sep 2019 20:19:25 +0300

> Add documentation of ti,sgmii-ref-clock-output-enable
> which can be used to select SGMII mode type (4 or 6-wire).
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>

Applied.

Re: [PATCH v4 2/2] net: phy: dp83867: Add SGMII mode type switching

[David Miller]

From: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
Date: Mon,  9 Sep 2019 20:19:24 +0300

> This patch adds ability to switch beetween two PHY SGMII modes.
> Some hardware, for example, FPGA IP designs may use 6-wire mode
> which enables differential SGMII clock to MAC.
> 
> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>

Applied.

Re: [PATCH v4 2/2] net: phy: dp83867: Add SGMII mode type switching

[Vitaly Gaiduk]

Hello, David.

Should I patch commit as Trent Piepho suggested? He wrote about using 
phy_modify_mmd() instead.

Vitaly.

On 12.09.2019 1:37, David Miller wrote:
> From: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> Date: Mon,  9 Sep 2019 20:19:24 +0300
>
>> This patch adds ability to switch beetween two PHY SGMII modes.
>> Some hardware, for example, FPGA IP designs may use 6-wire mode
>> which enables differential SGMII clock to MAC.
>>
>> Signed-off-by: Vitaly Gaiduk <vitaly.gaiduk@cloudbear.ru>
> Applied.