[PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Lukasz Majewski]

This patch adds support for enabling or disabling the port mirroring
feature of the DP83867 TI's PHY device.

One use case is when bootstrap configuration enables this feature (because
of e.g. LED wiring) so then one needs to disable it in software
(u-boot/Linux).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 .../devicetree/bindings/net/ti,dp83867.txt         |  4 +++
 drivers/net/phy/dp83867.c                          | 40 ++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
index afe9630..d1be241 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -18,6 +18,10 @@ Optional property:
 	- ti,max-output-impedance - MAC Interface Impedance control to set
 				    the programmable output impedance to
 				    maximum value (70 ohms).
+	- ti,port-mirroring - Set port mirroring. It is possible to overwrite -
+			      enable (by setting <1>) or disable
+			      (by setting <0>) the port mirroring feature set
+			      by bootstrap initial reading.
 
 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/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index ca1b462..98948bd 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -32,6 +32,7 @@
 #define DP83867_CFG3		0x1e
 
 /* Extended Registers */
+#define DP83867_CFG4            0x0031
 #define DP83867_RGMIICTL	0x0032
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
@@ -70,11 +71,20 @@
 #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
 #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
 
+/* CFG4 bits */
+#define DP83867_CFG4_PORT_MIRROR_EN              BIT(0)
+
+enum {
+	DP83867_PORT_MIRROING_EN = 1,
+	DP83867_PORT_MIRROING_DIS,
+};
+
 struct dp83867_private {
 	int rx_id_delay;
 	int tx_id_delay;
 	int fifo_depth;
 	int io_impedance;
+	int port_mirroring;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -111,6 +121,24 @@ static int dp83867_config_intr(struct phy_device *phydev)
 	return phy_write(phydev, MII_DP83867_MICR, micr_status);
 }
 
+static int dp83867_config_port_mirroring(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867 =
+		(struct dp83867_private *)phydev->priv;
+	u16 val;
+
+	val = phy_read_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR);
+
+	if (dp83867->port_mirroring == DP83867_PORT_MIRROING_EN)
+		val |= DP83867_CFG4_PORT_MIRROR_EN;
+	else
+		val &= ~DP83867_CFG4_PORT_MIRROR_EN;
+
+	phy_write_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR, val);
+
+	return 0;
+}
+
 #ifdef CONFIG_OF_MDIO
 static int dp83867_of_init(struct phy_device *phydev)
 {
@@ -118,6 +146,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
 	int ret;
+	u32 v;
 
 	if (!of_node)
 		return -ENODEV;
@@ -144,6 +173,14 @@ static int dp83867_of_init(struct phy_device *phydev)
 	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID))
 		return ret;
 
+	ret = of_property_read_u32(of_node, "ti,port-mirroring", &v);
+	if (!ret) {
+		if (v)
+			dp83867->port_mirroring = DP83867_PORT_MIRROING_EN;
+		else
+			dp83867->port_mirroring = DP83867_PORT_MIRROING_DIS;
+	}
+
 	return of_property_read_u32(of_node, "ti,fifo-depth",
 				   &dp83867->fifo_depth);
 }
@@ -228,6 +265,9 @@ static int dp83867_config_init(struct phy_device *phydev)
 		phy_write(phydev, DP83867_CFG3, val);
 	}
 
+	if (dp83867->port_mirroring)
+		dp83867_config_port_mirroring(phydev);
+
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Andrew Lunn]

On Wed, Feb 01, 2017 at 03:43:35PM +0100, Lukasz Majewski wrote:
> This patch adds support for enabling or disabling the port mirroring
> feature of the DP83867 TI's PHY device.
> 
> One use case is when bootstrap configuration enables this feature (because
> of e.g. LED wiring) so then one needs to disable it in software
> (u-boot/Linux).

Hi Lukasz

How does this differ from "enet-phy-lane-swap"?

Thanks
    Andrew

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Florian Fainelli]

On 02/01/2017 09:16 AM, Andrew Lunn wrote:
> On Wed, Feb 01, 2017 at 03:43:35PM +0100, Lukasz Majewski wrote:
>> This patch adds support for enabling or disabling the port mirroring
>> feature of the DP83867 TI's PHY device.
>>
>> One use case is when bootstrap configuration enables this feature (because
>> of e.g. LED wiring) so then one needs to disable it in software
>> (u-boot/Linux).
> 
> Hi Lukasz
> 
> How does this differ from "enet-phy-lane-swap"?

Same here, I am confused about what port mirroring could be meaning here
and if we can find a better way to describe what is being added. Thanks!
-- 
Florian

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Lukasz Majewski]

Dear All,

Thanks for prompt reply.

> On 02/01/2017 09:16 AM, Andrew Lunn wrote:
> > On Wed, Feb 01, 2017 at 03:43:35PM +0100, Lukasz Majewski wrote:
> >> This patch adds support for enabling or disabling the port
> >> mirroring feature of the DP83867 TI's PHY device.
> >>
> >> One use case is when bootstrap configuration enables this feature
> >> (because of e.g. LED wiring) so then one needs to disable it in
> >> software (u-boot/Linux).
> > 
> > Hi Lukasz
> > 
> > How does this differ from "enet-phy-lane-swap"?
> 
> Same here, I am confused about what port mirroring could be meaning
> here

The "net-phy-lane-swap" when defined indicates that the "lane swap"
needs to be enabled. This is a simple bool variable. In my case it
would mean: "please enable port mirroring -> write 1 to CFG4 register's
PORT_MIRROR_EN field"

My use case is unfortunately different:

- Due to HW design - during the bootstrap PHY phase - the PHY enables
  "port mirroring", which is incorrect. 

Then, in SW I do need to explicitly disable port mirroring (write 0 to
PORT_MIRROR_EN field in CFG4 register). 


> and if we can find a better way to describe what is being added.
> Thanks!

We would need a tri-state device tree properly:

1. Not defined - do nothing
2. Defined as 0 -> explicitly disable port mirroring
3. Defined as 1 -> explicitly enable port mirriring

The "net-phy-lane-swap" only fulfills points 1 and 3 above.

In my use case I do need point 2.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Florian Fainelli]

On 02/01/2017 01:05 PM, Lukasz Majewski wrote:
> Dear All,
> 
> Thanks for prompt reply.
> 
>> On 02/01/2017 09:16 AM, Andrew Lunn wrote:
>>> On Wed, Feb 01, 2017 at 03:43:35PM +0100, Lukasz Majewski wrote:
>>>> This patch adds support for enabling or disabling the port
>>>> mirroring feature of the DP83867 TI's PHY device.
>>>>
>>>> One use case is when bootstrap configuration enables this feature
>>>> (because of e.g. LED wiring) so then one needs to disable it in
>>>> software (u-boot/Linux).
>>>
>>> Hi Lukasz
>>>
>>> How does this differ from "enet-phy-lane-swap"?
>>
>> Same here, I am confused about what port mirroring could be meaning
>> here
> 
> The "net-phy-lane-swap" when defined indicates that the "lane swap"
> needs to be enabled. This is a simple bool variable. In my case it
> would mean: "please enable port mirroring -> write 1 to CFG4 register's
> PORT_MIRROR_EN field"
> 
> My use case is unfortunately different:
> 
> - Due to HW design - during the bootstrap PHY phase - the PHY enables
>   "port mirroring", which is incorrect. 
> 
> Then, in SW I do need to explicitly disable port mirroring (write 0 to
> PORT_MIRROR_EN field in CFG4 register). 

You did not really explain whether port mirroring meant the same thing
as swapping the PHY lanes or not, but based on your paragraph later on,
it does sound like this is the case.

What a poorly chosen name though... in Ethernet world, port mirroring
means the ability to capture traffic from a vector of ports and copying
it verbatim (or sampled) towards a capture port, aka the mirror port...

> 
> 
>> and if we can find a better way to describe what is being added.
>> Thanks!
> 
> We would need a tri-state device tree properly:
> 
> 1. Not defined - do nothing
> 2. Defined as 0 -> explicitly disable port mirroring
> 3. Defined as 1 -> explicitly enable port mirriring
> 
> The "net-phy-lane-swap" only fulfills points 1 and 3 above.
> 
> In my use case I do need point 2.

You can define another boolean property:

net-phy-lane-no-swap?

-- 
Florian

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Andrew Lunn]

> We would need a tri-state device tree properly:
> 
> 1. Not defined - do nothing
> 2. Defined as 0 -> explicitly disable port mirroring
> 3. Defined as 1 -> explicitly enable port mirriring
> 
> The "net-phy-lane-swap" only fulfills points 1 and 3 above.
> 
> In my use case I do need point 2.

Looking at the datasheet, PORT_MIRROR_EN defaults to 0. So it seems
reasonable to unconditionally set it to 0 when the PHY driver
loads. Any device which needs a value of 1 can set "net-phy-lane-swap"

       Andrew

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Andrew Lunn]

> What a poorly chosen name though... in Ethernet world, port mirroring
> means the ability to capture traffic from a vector of ports and copying
> it verbatim (or sampled) towards a capture port, aka the mirror port...

Ack. We should avoid "port mirroring" in what ever patch we decide upon.

     Andrew

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Lukasz Majewski]

Hi Andrew,

> > We would need a tri-state device tree properly:
> > 
> > 1. Not defined - do nothing
> > 2. Defined as 0 -> explicitly disable port mirroring
> > 3. Defined as 1 -> explicitly enable port mirriring
> > 
> > The "net-phy-lane-swap" only fulfills points 1 and 3 above.
> > 
> > In my use case I do need point 2.
> 
> Looking at the datasheet, PORT_MIRROR_EN defaults to 0. So it seems
> reasonable to unconditionally set it to 0 when the PHY driver
> loads. Any device which needs a value of 1 can set "net-phy-lane-swap"

Unconditionally setting this field to 0 (as we expect the reset default
setting) seems to me like a good solution. 

However, I was not sure if such approach is acceptable by the community.

> 
>        Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Andrew Lunn]

On Wed, Feb 01, 2017 at 11:13:23PM +0100, Lukasz Majewski wrote:
> Hi Andrew,
> 
> > > We would need a tri-state device tree properly:
> > > 
> > > 1. Not defined - do nothing
> > > 2. Defined as 0 -> explicitly disable port mirroring
> > > 3. Defined as 1 -> explicitly enable port mirriring
> > > 
> > > The "net-phy-lane-swap" only fulfills points 1 and 3 above.
> > > 
> > > In my use case I do need point 2.
> > 
> > Looking at the datasheet, PORT_MIRROR_EN defaults to 0. So it seems
> > reasonable to unconditionally set it to 0 when the PHY driver
> > loads. Any device which needs a value of 1 can set "net-phy-lane-swap"
> 
> Unconditionally setting this field to 0 (as we expect the reset default
> setting) seems to me like a good solution. 
> 
> However, I was not sure if such approach is acceptable by the community.

So the issue here is, are there boards with bootloaders which set this
"lane swap" bit, and rely on Linux not changing it, because the
hardware design has such a swap?

It seems the bootloader you are using does this. But in your case, it
does it wrongly. I'm guessing you have a vendor bootloader? And no
easy access to the sources? Otherwise you would of fixed the
bootloader. So can we assume there are vendor designed boards which
require the swap? Do they run a mainline kernel? Or only the vendor
kernel?

If we know of mainline boards which are going to break, we should not
do this.

If however we do decide to reset it to default value, i think it would
be good to implement net-phy-lane-swap as well, so giving people an
easier path towards mainline.

       Andrew

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Lukasz Majewski]

Hi Andrew,

> On Wed, Feb 01, 2017 at 11:13:23PM +0100, Lukasz Majewski wrote:
> > Hi Andrew,
> > 
> > > > We would need a tri-state device tree properly:
> > > > 
> > > > 1. Not defined - do nothing
> > > > 2. Defined as 0 -> explicitly disable port mirroring
> > > > 3. Defined as 1 -> explicitly enable port mirriring
> > > > 
> > > > The "net-phy-lane-swap" only fulfills points 1 and 3 above.
> > > > 
> > > > In my use case I do need point 2.
> > > 
> > > Looking at the datasheet, PORT_MIRROR_EN defaults to 0. So it
> > > seems reasonable to unconditionally set it to 0 when the PHY
> > > driver loads. Any device which needs a value of 1 can set
> > > "net-phy-lane-swap"
> > 
> > Unconditionally setting this field to 0 (as we expect the reset
> > default setting) seems to me like a good solution. 
> > 
> > However, I was not sure if such approach is acceptable by the
> > community.
> 
> So the issue here is, are there boards with bootloaders which set this
> "lane swap" bit,

The bootstrapping process in the PHY sets this bit. This is wrong since
the board lane layout is not "swapped"

The bootloader (u-boot) fixes this, since we need to support
networking (tftp, ping).


> and rely on Linux not changing it, 

When we boot Linux everything is OK, until the dp83867 driver comes
into play and performs reset (including register reset).

Then the "bootstrap", initial line swap is setup again (wrongly). So we
need a way in Linux to make things correct again.

> because the
> hardware design has such a swap?
> 
> It seems the bootloader you are using does this. 

The bootloader fixes things, but then in Linux the PHY driver
(dp83867.c) performs "RESET" which breaks networking again.

> But in your case, it
> does it wrongly. 

The bootloader does its job correctly.

> I'm guessing you have a vendor bootloader? And no
> easy access to the sources? 

Mainline u-boot (all vendor code will be upstreamed).

> Otherwise you would of fixed the
> bootloader. So can we assume there are vendor designed boards which
> require the swap? Do they run a mainline kernel? Or only the vendor
> kernel?

All stuff is going to run mainline kernel (LTS - v4.9 ?).

> 
> If we know of mainline boards which are going to break, we should not
> do this.
> 
> If however we do decide to reset it to default value, i think it would
> be good to implement net-phy-lane-swap as well, so giving people an
> easier path towards mainline.

I have thought a bit about that and I think that we should define
complementary "net-phy-lane-no-swap" as suggested by Florian. Then
affected boards could define it and use.

> 
>        Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Andrew Lunn]

> The bootstrapping process in the PHY sets this bit. This is wrong since
> the board lane layout is not "swapped"
 
Ah, you mean a strapping pin? Resistor to ground/VCC?
That is a different matter. It makes it a lot less likely to break
some existing board with such a change.

> I have thought a bit about that and I think that we should define
> complementary "net-phy-lane-no-swap" as suggested by Florian. Then
> affected boards could define it and use.

This is the most flexible solution. Yes, that is O.K. for me.

     Andrew

Re: [PATCH] net: phy: dp83867: Port mirroring support in the DP83867 TI's PHY driver

[Lukasz Majewski]

On Thu, 2 Feb 2017 14:11:12 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > The bootstrapping process in the PHY sets this bit. This is wrong
> > since the board lane layout is not "swapped"
>  
> Ah, you mean a strapping pin? Resistor to ground/VCC?

Yes, exactly.

> That is a different matter. It makes it a lot less likely to break
> some existing board with such a change.
> 
> > I have thought a bit about that and I think that we should define
> > complementary "net-phy-lane-no-swap" as suggested by Florian. Then
> > affected boards could define it and use.
> 
> This is the most flexible solution. Yes, that is O.K. for me.

Ok. thanks.

> 
>      Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de