Message ID | 1485960215-5148-1-git-send-email-lukma@denx.de |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
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!
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
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?
> 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
> 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
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
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
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
> 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
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
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; }
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(+)