[U-Boot] [PATCH 0/8] net: phy: dp83867: ti: sync with linux kernel and

hi
The intention of this series is to sync TI DP83867 driver with mainline Linux kernel and ensure that proper PHY configuration in "rgmii-rxid" mode, which is done in the Patch 8.
It also some code refactoring and optimization: patches 2,7.
Grygorii Strashko (8): net: phy: ti: rename ti.c to dp83867.c net: phy: dp83867: move static initialization to .probe() dt-bindings: phy: dp83867: Add documentation for disabling clock output net: phy: dp83867: Add ability to disable output clock net: phy: dp83867: rework delay rgmii delay handling net: phy: dp83867: io impedance is not dependent on RGMII delay net: phy: dp83867: refactor rgmii configuration arm: dts: k3-am654-base-board-u-boot: change cpsw2g interface mode to rgmii-rxid
arch/arm/dts/k3-am654-base-board-u-boot.dtsi | 3 +- doc/device-tree-bindings/net/ti,dp83867.txt | 6 +- drivers/net/phy/Makefile | 2 +- drivers/net/phy/{ti.c => dp83867.c} | 243 ++++++++++++------- include/dt-bindings/net/ti-dp83867.h | 3 +- 5 files changed, 166 insertions(+), 91 deletions(-) rename drivers/net/phy/{ti.c => dp83867.c} (63%)

The driver ti.c is actually driver for TI DP83867x PHYs, so rename it accordingly.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/Makefile | 2 +- drivers/net/phy/{ti.c => dp83867.c} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename drivers/net/phy/{ti.c => dp83867.c} (100%)
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 76b6197009..78955c57a8 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -25,7 +25,7 @@ obj-$(CONFIG_PHY_NATSEMI) += natsemi.o obj-$(CONFIG_PHY_REALTEK) += realtek.o obj-$(CONFIG_PHY_SMSC) += smsc.o obj-$(CONFIG_PHY_TERANETICS) += teranetics.o -obj-$(CONFIG_PHY_TI) += ti.o +obj-$(CONFIG_PHY_TI) += dp83867.o obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o obj-$(CONFIG_PHY_VITESSE) += vitesse.o diff --git a/drivers/net/phy/ti.c b/drivers/net/phy/dp83867.c similarity index 100% rename from drivers/net/phy/ti.c rename to drivers/net/phy/dp83867.c

On Mon, Nov 18, 2019 at 3:12 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
The driver ti.c is actually driver for TI DP83867x PHYs, so rename it accordingly.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

Move static, one-time initialization to .probe() callback.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/dp83867.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 7509936465..8dc2163342 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -204,18 +204,11 @@ static int dp83867_config(struct phy_device *phydev) unsigned int val, delay, cfg2; int ret, bs;
- if (!phydev->priv) { - dp83867 = kzalloc(sizeof(*dp83867), GFP_KERNEL); - if (!dp83867) - return -ENOMEM; + dp83867 = (struct dp83867_private *)phydev->priv;
- phydev->priv = dp83867; - ret = dp83867_of_init(phydev); - if (ret) - goto err_out; - } else { - dp83867 = (struct dp83867_private *)phydev->priv; - } + ret = dp83867_of_init(phydev); + if (ret) + return ret;
/* Restart the PHY. */ val = phy_read(phydev, MDIO_DEVAD_NONE, DP83867_CTRL); @@ -324,15 +317,27 @@ static int dp83867_config(struct phy_device *phydev) return 0;
err_out: - kfree(dp83867); return ret; }
+static int dp83867_probe(struct phy_device *phydev) +{ + struct dp83867_private *dp83867; + + dp83867 = kzalloc(sizeof(*dp83867), GFP_KERNEL); + if (!dp83867) + return -ENOMEM; + + phydev->priv = dp83867; + return 0; +} + static struct phy_driver DP83867_driver = { .name = "TI DP83867", .uid = 0x2000a231, .mask = 0xfffffff0, .features = PHY_GBIT_FEATURES, + .probe = dp83867_probe, .config = &dp83867_config, .startup = &genphy_startup, .shutdown = &genphy_shutdown,

On Mon, Nov 18, 2019 at 3:08 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Move static, one-time initialization to .probe() callback.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

Based on commit 980066e6d964 ("dt-bindings: phy: dp83867: Add documentation for disabling clock output") of mainline linux kernel.
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.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- doc/device-tree-bindings/net/ti,dp83867.txt | 6 ++++-- include/dt-bindings/net/ti-dp83867.h | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/doc/device-tree-bindings/net/ti,dp83867.txt b/doc/device-tree-bindings/net/ti,dp83867.txt index 034146f5f8..268220964a 100644 --- a/doc/device-tree-bindings/net/ti,dp83867.txt +++ b/doc/device-tree-bindings/net/ti,dp83867.txt @@ -12,8 +12,10 @@ Required properties: compensate for the board being designed with the lanes swapped. - enet-phy-no-lane-swap - Indicates that PHY will disable swap of the TX/RX lanes. - - ti,clk-output-sel - Clock output select - 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.
Default child nodes are standard Ethernet PHY device nodes as described in doc/devicetree/bindings/net/ethernet.txt diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h index 85d08f6974..cde5aa7e27 100644 --- a/include/dt-bindings/net/ti-dp83867.h +++ b/include/dt-bindings/net/ti-dp83867.h @@ -45,5 +45,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

On Mon, Nov 18, 2019 at 3:17 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Based on commit 980066e6d964 ("dt-bindings: phy: dp83867: Add documentation for disabling clock output") of mainline linux kernel.
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.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

Based on commit 13c83cf8af0d ("net: phy: dp83867: Add ability to disable output clock") of mainline linux kernel.
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.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/dp83867.c | 53 ++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 8dc2163342..cd3c1c596a 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -83,6 +83,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_SHIFT 8 #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK \ GENMASK(0x1f, DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT) @@ -103,6 +104,7 @@ struct dp83867_private { int io_impedance; bool rxctrl_strap_quirk; int port_mirroring; + bool set_clk_output; unsigned int clk_output_sel; };
@@ -134,16 +136,28 @@ static int dp83867_of_init(struct phy_device *phydev) { struct dp83867_private *dp83867 = phydev->priv; ofnode node; - u16 val; + int ret;
node = phy_get_ofnode(phydev); if (!ofnode_valid(node)) return -EINVAL;
- /* Keep the default value if ti,clk-output-sel is not set */ - dp83867->clk_output_sel = - ofnode_read_u32_default(node, "ti,clk-output-sel", - DP83867_CLK_O_SEL_REF_CLK); + /* Optional configuration */ + ret = ofnode_read_u32(node, "ti,clk-output-sel", + &dp83867->clk_output_sel); + /* If not set, keep default */ + if (!ret) { + dp83867->set_clk_output = true; + /* Valid values are 0 to DP83867_CLK_O_SEL_REF_CLK or + * DP83867_CLK_O_SEL_OFF. + */ + if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK && + dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) { + pr_debug("ti,clk-output-sel value %u out of range\n", + dp83867->clk_output_sel); + return -EINVAL; + } + }
if (ofnode_read_bool(node, "ti,max-output-impedance")) dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX; @@ -170,18 +184,6 @@ static int dp83867_of_init(struct phy_device *phydev) if (ofnode_read_bool(node, "enet-phy-lane-no-swap")) dp83867->port_mirroring = DP83867_PORT_MIRRORING_DIS;
- - /* Clock output selection if muxing property is set */ - if (dp83867->clk_output_sel != DP83867_CLK_O_SEL_REF_CLK) { - val = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG); - val &= ~DP83867_IO_MUX_CFG_CLK_O_SEL_MASK; - val |= (dp83867->clk_output_sel << - DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT); - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG, val); - } - return 0; } #else @@ -313,6 +315,23 @@ static int dp83867_config(struct phy_device *phydev) if (dp83867->port_mirroring != DP83867_PORT_MIRRORING_KEEP) dp83867_config_port_mirroring(phydev);
+ /* Clock output selection if muxing property is set */ + if (dp83867->set_clk_output) { + val = phy_read_mmd(phydev, DP83867_DEVADDR, + DP83867_IO_MUX_CFG); + + if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) { + val |= DP83867_IO_MUX_CFG_CLK_O_DISABLE; + } else { + val &= ~(DP83867_IO_MUX_CFG_CLK_O_SEL_MASK | + DP83867_IO_MUX_CFG_CLK_O_DISABLE); + val |= dp83867->clk_output_sel << + DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT; + } + phy_write_mmd(phydev, DP83867_DEVADDR, + DP83867_IO_MUX_CFG, val); + } + genphy_config_aneg(phydev); return 0;

On Mon, Nov 18, 2019 at 3:22 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Based on commit 13c83cf8af0d ("net: phy: dp83867: Add ability to disable output clock") of mainline linux kernel.
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.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Nit below, but
Acked-by: Joe Hershberger joe.hershberger@ni.com
drivers/net/phy/dp83867.c | 53 ++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 8dc2163342..cd3c1c596a 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -83,6 +83,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_SHIFT 8 #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK \ GENMASK(0x1f, DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT) @@ -103,6 +104,7 @@ struct dp83867_private { int io_impedance; bool rxctrl_strap_quirk; int port_mirroring;
bool set_clk_output; unsigned int clk_output_sel;
};
@@ -134,16 +136,28 @@ static int dp83867_of_init(struct phy_device *phydev) { struct dp83867_private *dp83867 = phydev->priv; ofnode node;
u16 val;
int ret; node = phy_get_ofnode(phydev); if (!ofnode_valid(node)) return -EINVAL;
/* Keep the default value if ti,clk-output-sel is not set */
dp83867->clk_output_sel =
ofnode_read_u32_default(node, "ti,clk-output-sel",
DP83867_CLK_O_SEL_REF_CLK);
/* Optional configuration */
ret = ofnode_read_u32(node, "ti,clk-output-sel",
&dp83867->clk_output_sel);
/* If not set, keep default */
if (!ret) {
dp83867->set_clk_output = true;
/* Valid values are 0 to DP83867_CLK_O_SEL_REF_CLK or
Use the correct multi-line comment format. [1]
[1] - http://www.denx.de/wiki/U-Boot/CodingStyle
* DP83867_CLK_O_SEL_OFF.
*/
if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
pr_debug("ti,clk-output-sel value %u out of range\n",
dp83867->clk_output_sel);
return -EINVAL;
}
} if (ofnode_read_bool(node, "ti,max-output-impedance")) dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
@@ -170,18 +184,6 @@ static int dp83867_of_init(struct phy_device *phydev) if (ofnode_read_bool(node, "enet-phy-lane-no-swap")) dp83867->port_mirroring = DP83867_PORT_MIRRORING_DIS;
/* Clock output selection if muxing property is set */
if (dp83867->clk_output_sel != DP83867_CLK_O_SEL_REF_CLK) {
val = phy_read_mmd(phydev, DP83867_DEVADDR,
DP83867_IO_MUX_CFG);
val &= ~DP83867_IO_MUX_CFG_CLK_O_SEL_MASK;
val |= (dp83867->clk_output_sel <<
DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT);
phy_write_mmd(phydev, DP83867_DEVADDR,
DP83867_IO_MUX_CFG, val);
}
return 0;
} #else @@ -313,6 +315,23 @@ static int dp83867_config(struct phy_device *phydev) if (dp83867->port_mirroring != DP83867_PORT_MIRRORING_KEEP) dp83867_config_port_mirroring(phydev);
/* Clock output selection if muxing property is set */
if (dp83867->set_clk_output) {
val = phy_read_mmd(phydev, DP83867_DEVADDR,
DP83867_IO_MUX_CFG);
if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) {
val |= DP83867_IO_MUX_CFG_CLK_O_DISABLE;
} else {
val &= ~(DP83867_IO_MUX_CFG_CLK_O_SEL_MASK |
DP83867_IO_MUX_CFG_CLK_O_DISABLE);
val |= dp83867->clk_output_sel <<
DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT;
}
phy_write_mmd(phydev, DP83867_DEVADDR,
DP83867_IO_MUX_CFG, val);
}
genphy_config_aneg(phydev); return 0;
-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

Based on commit c11669a2757e ("net: phy: dp83867: Rework delay rgmii delay handling") of mainline linux kernel.
The current code is 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.
This patch: - insures the other direction's delay is disabled If the interface mode is selected as RX or TX delay only - validates the delay values and fail if they are not in range - checks if the board is strapped to have a delay and is configured to use "rgmii" mode and warning is generated that "rgmii-id" should have been used.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/dp83867.c | 76 ++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index cd3c1c596a..1721f6892b 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -25,6 +25,7 @@ #define DP83867_CFG4 0x0031 #define DP83867_RGMIICTL 0x0032 #define DP83867_STRAP_STS1 0x006E +#define DP83867_STRAP_STS2 0x006f #define DP83867_RGMIIDCTL 0x0086 #define DP83867_IO_MUX_CFG 0x0170
@@ -52,6 +53,13 @@ /* STRAP_STS1 bits */ #define DP83867_STRAP_STS1_RESERVED BIT(11)
+/* STRAP_STS2 bits */ +#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK GENMASK(6, 4) +#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT 4 +#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK GENMASK(2, 0) +#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT 0 +#define DP83867_STRAP_STS2_CLK_SKEW_NONE BIT(2) + /* PHY CTRL bits */ #define DP83867_PHYCR_FIFO_DEPTH_SHIFT 14 #define DP83867_PHYCR_RESERVED_MASK BIT(11) @@ -63,7 +71,9 @@ #define DP83867_PHYCTRL_TXFIFO_SHIFT 14
/* RGMIIDCTL bits */ +#define DP83867_RGMII_TX_CLK_DELAY_MAX 0xf #define DP83867_RGMII_TX_CLK_DELAY_SHIFT 4 +#define DP83867_RGMII_RX_CLK_DELAY_MAX 0xf
/* CFG2 bits */ #define MII_DP83867_CFG2_SPEEDOPT_10EN 0x0040 @@ -74,8 +84,6 @@ #define MII_DP83867_CFG2_MASK 0x003F
/* User setting - can be taken from DTS */ -#define DEFAULT_RX_ID_DELAY DP83867_RGMIIDCTL_2_25_NS -#define DEFAULT_TX_ID_DELAY DP83867_RGMIIDCTL_2_75_NS #define DEFAULT_FIFO_DEPTH DP83867_PHYCR_FIFO_DEPTH_4_B_NIB
/* IO_MUX_CFG bits */ @@ -98,8 +106,8 @@ enum { };
struct dp83867_private { - int rx_id_delay; - int tx_id_delay; + u32 rx_id_delay; + u32 tx_id_delay; int fifo_depth; int io_impedance; bool rxctrl_strap_quirk; @@ -168,13 +176,55 @@ static int dp83867_of_init(struct phy_device *phydev)
if (ofnode_read_bool(node, "ti,dp83867-rxctrl-strap-quirk")) dp83867->rxctrl_strap_quirk = true; - dp83867->rx_id_delay = ofnode_read_u32_default(node, - "ti,rx-internal-delay", - DEFAULT_RX_ID_DELAY);
- dp83867->tx_id_delay = ofnode_read_u32_default(node, - "ti,tx-internal-delay", - DEFAULT_TX_ID_DELAY); + /* Existing behavior was to use default pin strapping delay in rgmii + * mode, but rgmii should have meant no delay. Warn existing users. + */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { + u16 val = phy_read_mmd(phydev, DP83867_DEVADDR, + DP83867_STRAP_STS2); + u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >> + DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT; + u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >> + DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT; + + if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE || + rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE) + pr_warn("PHY has delays via pin strapping, but phy-mode = 'rgmii'\n" + "Should be 'rgmii-id' to use internal delays\n"); + } + + /* RX delay *must* be specified if internal delay of RX is used. */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { + ret = ofnode_read_u32(node, "ti,rx-internal-delay", + &dp83867->rx_id_delay); + if (ret) { + pr_debug("ti,rx-internal-delay must be specified\n"); + return ret; + } + if (dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) { + pr_debug("ti,rx-internal-delay value of %u out of range\n", + dp83867->rx_id_delay); + return -EINVAL; + } + } + + /* TX delay *must* be specified if internal delay of RX is used. */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + ret = ofnode_read_u32(node, "ti,tx-internal-delay", + &dp83867->tx_id_delay); + if (ret) { + debug("ti,tx-internal-delay must be specified\n"); + return ret; + } + if (dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) { + pr_debug("ti,tx-internal-delay value of %u out of range\n", + dp83867->tx_id_delay); + return -EINVAL; + } + }
dp83867->fifo_depth = ofnode_read_u32_default(node, "ti,fifo-depth", DEFAULT_FIFO_DEPTH); @@ -191,8 +241,8 @@ static int dp83867_of_init(struct phy_device *phydev) { struct dp83867_private *dp83867 = phydev->priv;
- dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY; - dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY; + dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_25_NS; + dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_75_NS; dp83867->fifo_depth = DEFAULT_FIFO_DEPTH; dp83867->io_impedance = -EINVAL;
@@ -281,6 +331,8 @@ static int dp83867_config(struct phy_device *phydev) 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);

On Mon, Nov 18, 2019 at 3:26 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Based on commit c11669a2757e ("net: phy: dp83867: Rework delay rgmii delay handling") of mainline linux kernel.
The current code is 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.
This patch:
- insures the other direction's delay is disabled If the interface mode is
selected as RX or TX delay only
- validates the delay values and fail if they are not in range
- checks if the board is strapped to have a delay and is configured to use
"rgmii" mode and warning is generated that "rgmii-id" should have been used.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Nit below...
Acked-by: Joe Hershberger joe.hershberger@ni.com
drivers/net/phy/dp83867.c | 76 ++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index cd3c1c596a..1721f6892b 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -25,6 +25,7 @@ #define DP83867_CFG4 0x0031 #define DP83867_RGMIICTL 0x0032 #define DP83867_STRAP_STS1 0x006E +#define DP83867_STRAP_STS2 0x006f #define DP83867_RGMIIDCTL 0x0086 #define DP83867_IO_MUX_CFG 0x0170
@@ -52,6 +53,13 @@ /* STRAP_STS1 bits */ #define DP83867_STRAP_STS1_RESERVED BIT(11)
+/* STRAP_STS2 bits */ +#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK GENMASK(6, 4) +#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT 4 +#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK GENMASK(2, 0) +#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT 0 +#define DP83867_STRAP_STS2_CLK_SKEW_NONE BIT(2)
/* PHY CTRL bits */ #define DP83867_PHYCR_FIFO_DEPTH_SHIFT 14 #define DP83867_PHYCR_RESERVED_MASK BIT(11) @@ -63,7 +71,9 @@ #define DP83867_PHYCTRL_TXFIFO_SHIFT 14
/* RGMIIDCTL bits */ +#define DP83867_RGMII_TX_CLK_DELAY_MAX 0xf #define DP83867_RGMII_TX_CLK_DELAY_SHIFT 4 +#define DP83867_RGMII_RX_CLK_DELAY_MAX 0xf
/* CFG2 bits */ #define MII_DP83867_CFG2_SPEEDOPT_10EN 0x0040 @@ -74,8 +84,6 @@ #define MII_DP83867_CFG2_MASK 0x003F
/* User setting - can be taken from DTS */ -#define DEFAULT_RX_ID_DELAY DP83867_RGMIIDCTL_2_25_NS -#define DEFAULT_TX_ID_DELAY DP83867_RGMIIDCTL_2_75_NS #define DEFAULT_FIFO_DEPTH DP83867_PHYCR_FIFO_DEPTH_4_B_NIB
/* IO_MUX_CFG bits */ @@ -98,8 +106,8 @@ enum { };
struct dp83867_private {
int rx_id_delay;
int tx_id_delay;
u32 rx_id_delay;
u32 tx_id_delay; int fifo_depth; int io_impedance; bool rxctrl_strap_quirk;
@@ -168,13 +176,55 @@ static int dp83867_of_init(struct phy_device *phydev)
if (ofnode_read_bool(node, "ti,dp83867-rxctrl-strap-quirk")) dp83867->rxctrl_strap_quirk = true;
dp83867->rx_id_delay = ofnode_read_u32_default(node,
"ti,rx-internal-delay",
DEFAULT_RX_ID_DELAY);
dp83867->tx_id_delay = ofnode_read_u32_default(node,
"ti,tx-internal-delay",
DEFAULT_TX_ID_DELAY);
/* Existing behavior was to use default pin strapping delay in rgmii
Is this copied out of Linux verbatim? If not, please change the comment block format.
* mode, but rgmii should have meant no delay. Warn existing users.
*/
if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
u16 val = phy_read_mmd(phydev, DP83867_DEVADDR,
DP83867_STRAP_STS2);
u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >>
DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT;
u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >>
DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT;
if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE ||
rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE)
pr_warn("PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
"Should be 'rgmii-id' to use internal delays\n");
}
/* RX delay *must* be specified if internal delay of RX is used. */
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
ret = ofnode_read_u32(node, "ti,rx-internal-delay",
&dp83867->rx_id_delay);
if (ret) {
pr_debug("ti,rx-internal-delay must be specified\n");
return ret;
}
if (dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) {
pr_debug("ti,rx-internal-delay value of %u out of range\n",
dp83867->rx_id_delay);
return -EINVAL;
}
}
/* TX delay *must* be specified if internal delay of RX is used. */
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
ret = ofnode_read_u32(node, "ti,tx-internal-delay",
&dp83867->tx_id_delay);
if (ret) {
debug("ti,tx-internal-delay must be specified\n");
return ret;
}
if (dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) {
pr_debug("ti,tx-internal-delay value of %u out of range\n",
dp83867->tx_id_delay);
return -EINVAL;
}
} dp83867->fifo_depth = ofnode_read_u32_default(node, "ti,fifo-depth", DEFAULT_FIFO_DEPTH);
@@ -191,8 +241,8 @@ static int dp83867_of_init(struct phy_device *phydev) { struct dp83867_private *dp83867 = phydev->priv;
dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY;
dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY;
dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_25_NS;
dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_75_NS; dp83867->fifo_depth = DEFAULT_FIFO_DEPTH; dp83867->io_impedance = -EINVAL;
@@ -281,6 +331,8 @@ static int dp83867_config(struct phy_device *phydev) 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.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot

Based on commit 27708eb5481b ("net: phy: dp83867: IO impedance is not dependent on RGMII delay") of mainline linux kernel.
The driver would only set the IO impedance value when RGMII internal delays were enabled. There is no reason for this. Move the IO impedance block out of the RGMII delay block.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/dp83867.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 1721f6892b..f9bb925646 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -351,17 +351,17 @@ static int dp83867_config(struct phy_device *phydev)
phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIIDCTL, delay); + }
- if (dp83867->io_impedance >= 0) { - val = phy_read_mmd(phydev, - DP83867_DEVADDR, - DP83867_IO_MUX_CFG); - val &= ~DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; - val |= dp83867->io_impedance & - DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG, val); - } + if (dp83867->io_impedance >= 0) { + val = phy_read_mmd(phydev, + DP83867_DEVADDR, + DP83867_IO_MUX_CFG); + val &= ~DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; + val |= dp83867->io_impedance & + DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; + phy_write_mmd(phydev, DP83867_DEVADDR, + DP83867_IO_MUX_CFG, val); }
if (dp83867->port_mirroring != DP83867_PORT_MIRRORING_KEEP)

On Mon, Nov 18, 2019 at 3:31 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Based on commit 27708eb5481b ("net: phy: dp83867: IO impedance is not dependent on RGMII delay") of mainline linux kernel.
The driver would only set the IO impedance value when RGMII internal delays were enabled. There is no reason for this. Move the IO impedance block out of the RGMII delay block.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

Refactor SGMII configuration to group all settings together and reduce number of MDIO transactions.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- drivers/net/phy/dp83867.c | 75 +++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index f9bb925646..a43793cd42 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -62,9 +62,9 @@
/* PHY CTRL bits */ #define DP83867_PHYCR_FIFO_DEPTH_SHIFT 14 +#define DP83867_PHYCR_FIFO_DEPTH_MASK GENMASK(15, 14) #define DP83867_PHYCR_RESERVED_MASK BIT(11) #define DP83867_MDI_CROSSOVER 5 -#define DP83867_MDI_CROSSOVER_AUTO 2 #define DP83867_MDI_CROSSOVER_MDIX 2 #define DP83867_PHYCTRL_SGMIIEN 0x0800 #define DP83867_PHYCTRL_RXFIFO_SHIFT 12 @@ -277,11 +277,11 @@ static int dp83867_config(struct phy_device *phydev) }
if (phy_interface_is_rgmii(phydev)) { - ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL, - (DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) | - (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT)); - if (ret) + val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL); + if (val < 0) goto err_out; + val &= ~DP83867_PHYCR_FIFO_DEPTH_MASK; + val |= (dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT);
/* The code below checks if "port mirroring" N/A MODE4 has been * enabled during power on bootstrap. @@ -293,16 +293,39 @@ static int dp83867_config(struct phy_device *phydev) * register's bit 11 (marked as RESERVED). */
- bs = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_STRAP_STS1); - val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL); - if (bs & DP83867_STRAP_STS1_RESERVED) { + bs = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_STRAP_STS1); + if (bs & DP83867_STRAP_STS1_RESERVED) val &= ~DP83867_PHYCR_RESERVED_MASK; - phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL, - val); - }
- } else if (phy_interface_is_sgmii(phydev)) { + ret = phy_write(phydev, MDIO_DEVAD_NONE, + MII_DP83867_PHYCTRL, val); + + 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); + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) + val |= DP83867_RGMII_TX_CLK_DELAY_EN; + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) + val |= DP83867_RGMII_RX_CLK_DELAY_EN; + + phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL, val); + + delay = (dp83867->rx_id_delay | + (dp83867->tx_id_delay << + DP83867_RGMII_TX_CLK_DELAY_SHIFT)); + + phy_write_mmd(phydev, DP83867_DEVADDR, + DP83867_RGMIIDCTL, delay); + } + + if (phy_interface_is_sgmii(phydev)) { phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, (BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000));
@@ -327,32 +350,6 @@ static int dp83867_config(struct phy_device *phydev) phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_BISCR, 0x0); }
- if (phy_interface_is_rgmii(phydev)) { - 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); - - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) - val |= DP83867_RGMII_TX_CLK_DELAY_EN; - - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) - val |= DP83867_RGMII_RX_CLK_DELAY_EN; - - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_RGMIICTL, val); - - delay = (dp83867->rx_id_delay | - (dp83867->tx_id_delay << DP83867_RGMII_TX_CLK_DELAY_SHIFT)); - - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_RGMIIDCTL, delay); - } - if (dp83867->io_impedance >= 0) { val = phy_read_mmd(phydev, DP83867_DEVADDR,

On Mon, Nov 18, 2019 at 3:35 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
Refactor SGMII configuration to group all settings together and reduce number of MDIO transactions.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

The AM654 SoC doesn't allow to disabling RGMII TX internal delay in CPSW2G MAC. Hence, change CPSW2G interface mode to "rgmii-rxid" - RGMII with internal RX delay provided by the PHY, the MAC will add an TX delay in this case.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com --- arch/arm/dts/k3-am654-base-board-u-boot.dtsi | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi index 8589f76d23..bea80c5d00 100644 --- a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi +++ b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi @@ -336,13 +336,12 @@ reg = <0>; /* TODO: phy reset: TCA9555RTWR(i2c:0x21)[p04].GPIO_MCU_RGMII_RSTN */ ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>; - ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>; ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>; }; };
&cpsw_port1 { - phy-mode = "rgmii-id"; + phy-mode = "rgmii-rxid"; phy-handle = <&phy0>; };

On Mon, Nov 18, 2019 at 3:40 PM Grygorii Strashko grygorii.strashko@ti.com wrote:
The AM654 SoC doesn't allow to disabling RGMII TX internal delay in CPSW2G MAC. Hence, change CPSW2G interface mode to "rgmii-rxid" - RGMII with internal RX delay provided by the PHY, the MAC will add an TX delay in this case.
Signed-off-by: Grygorii Strashko grygorii.strashko@ti.com
Acked-by: Joe Hershberger joe.hershberger@ni.com

Hi,
On 18. 11. 19 22:04, Grygorii Strashko wrote:
hi
The intention of this series is to sync TI DP83867 driver with mainline Linux kernel and ensure that proper PHY configuration in "rgmii-rxid" mode, which is done in the Patch 8.
It also some code refactoring and optimization: patches 2,7.
Grygorii Strashko (8): net: phy: ti: rename ti.c to dp83867.c net: phy: dp83867: move static initialization to .probe() dt-bindings: phy: dp83867: Add documentation for disabling clock output net: phy: dp83867: Add ability to disable output clock net: phy: dp83867: rework delay rgmii delay handling net: phy: dp83867: io impedance is not dependent on RGMII delay net: phy: dp83867: refactor rgmii configuration arm: dts: k3-am654-base-board-u-boot: change cpsw2g interface mode to rgmii-rxid
arch/arm/dts/k3-am654-base-board-u-boot.dtsi | 3 +- doc/device-tree-bindings/net/ti,dp83867.txt | 6 +- drivers/net/phy/Makefile | 2 +- drivers/net/phy/{ti.c => dp83867.c} | 243 ++++++++++++------- include/dt-bindings/net/ti-dp83867.h | 3 +- 5 files changed, 166 insertions(+), 91 deletions(-) rename drivers/net/phy/{ti.c => dp83867.c} (63%)
Nice to see this series. I haven't developed the code but we have also added 6-wire mode to this phy. Also some other things there.
Anyway here is the link to Xilinx changes if you want to take a look. https://github.com/Xilinx/u-boot-xlnx/blob/master/drivers/net/phy/ti.c
Thanks, Michal
participants (3)
-
Grygorii Strashko
-
Joe Hershberger
-
Michal Simek