diff mbox series

[RFC,3/3] net: phy: dp83869: Port the kernel driver

Message ID 20200420185310.6630-3-dmurphy@ti.com
State New
Headers show
Series [RFC,1/3] net: phy: Add a generic phy file for TI generic PHYs | expand

Commit Message

Dan Murphy April 20, 2020, 6:53 p.m. UTC
Port the DP83869 kernel driver.

Signed-off-by: Dan Murphy <dmurphy at ti.com>
---
 drivers/net/phy/Kconfig              |   4 +
 drivers/net/phy/Makefile             |   1 +
 drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
 drivers/net/phy/ti_phy_init.c        |   4 +
 drivers/net/phy/ti_phy_init.h        |   1 +
 include/dt-bindings/net/ti-dp83869.h |  42 +++
 6 files changed, 492 insertions(+)
 create mode 100644 drivers/net/phy/dp83869.c
 create mode 100644 include/dt-bindings/net/ti-dp83869.h

Comments

Michal Simek April 21, 2020, 8:23 a.m. UTC | #1
On 20. 04. 20 20:53, Dan Murphy wrote:
> Port the DP83869 kernel driver.
> 
> Signed-off-by: Dan Murphy <dmurphy at ti.com>
> ---
>  drivers/net/phy/Kconfig              |   4 +
>  drivers/net/phy/Makefile             |   1 +
>  drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
>  drivers/net/phy/ti_phy_init.c        |   4 +
>  drivers/net/phy/ti_phy_init.h        |   1 +
>  include/dt-bindings/net/ti-dp83869.h |  42 +++
>  6 files changed, 492 insertions(+)
>  create mode 100644 drivers/net/phy/dp83869.c
>  create mode 100644 include/dt-bindings/net/ti-dp83869.h
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index e366f10afc59..5f14bdba0a3b 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -252,6 +252,10 @@ config PHY_DP83867
>  	select PHY_TI
>  	bool "Texas Instruments Ethernet DP83867 PHY support"
>  
> +config PHY_DP83869
> +	select PHY_TI
> +	bool "Texas Instruments Ethernet DP83869 PHY support"

isn't this a little bit short? I expect checkpatch should be reporting
issue with it.


> +
>  config PHY_VITESSE
>  	bool "Vitesse Ethernet PHYs support"
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 9c6d31718c00..f8797319154f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>  obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>  obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>  obj-$(CONFIG_PHY_DP83867) += dp83867.o
> +obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c
> new file mode 100644
> index 000000000000..1eaaea20b37a
> --- /dev/null
> +++ b/drivers/net/phy/dp83869.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for the Texas Instruments DP83869 PHY
> + * Copyright (C) 2019 Texas Instruments Inc.

2020?

> + */
> +
> +#include <common.h>
> +#include <phy.h>
> +#include <dm/devres.h>
> +#include <linux/compat.h>
> +#include <malloc.h>
> +
> +#include <dm.h>
> +
> +#include <dt-bindings/net/ti-dp83869.h>
> +
> +#include "ti_phy_init.h"
> +
> +#define DP83869_PHY_ID		0x2000a0f1
> +#define DP83869_DEVADDR		0x1f
> +
> +#define MII_DP83869_PHYCTRL	0x10
> +#define MII_DP83869_MICR	0x12
> +#define MII_DP83869_ISR		0x13
> +#define DP83869_CTRL		0x1f
> +#define DP83869_CFG4		0x1e
> +
> +/* Extended Registers */
> +#define DP83869_GEN_CFG3        0x0031
> +#define DP83869_RGMIICTL	0x0032
> +#define DP83869_STRAP_STS1	0x006e
> +#define DP83869_RGMIIDCTL	0x0086
> +#define DP83869_IO_MUX_CFG	0x0170
> +#define DP83869_OP_MODE		0x01df
> +#define DP83869_FX_CTRL		0x0c00
> +
> +#define DP83869_SW_RESET	BIT(15)
> +#define DP83869_SW_RESTART	BIT(14)
> +
> +/* MICR Interrupt bits */
> +#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
> +#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
> +#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
> +#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
> +
> +#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
> +					 BMCR_FULLDPLX | \
> +					 BMCR_SPEED1000)
> +
> +#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
> +					 ADVERTISE_1000XHALF | \
> +					 ADVERTISE_1000XFULL | \
> +					 ADVERTISE_1000XPAUSE | \
> +					 ADVERTISE_1000XPSE_ASYM)
> +
> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
> +#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
> +
> +/* CFG1 bits */
> +#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
> +				 ADVERTISE_1000FULL | \
> +				 CTL1000_AS_MASTER)
> +
> +/* RGMIICTL bits */
> +#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
> +#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
> +
> +/* STRAP_STS1 bits */
> +#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
> +#define DP83869_STRAP_STS1_RESERVED		BIT(11)
> +#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
> +
> +/* PHYCTRL bits */
> +#define DP83869_RX_FIFO_SHIFT	12
> +#define DP83869_TX_FIFO_SHIFT	14
> +
> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
> +#define DP83869_PHY_CTRL_DEFAULT	0x48
> +#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
> +#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
> +
> +/* RGMIIDCTL bits */
> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
> +
> +/* IO_MUX_CFG bits */
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
> +
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
> +
> +/* CFG3 bits */
> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
> +
> +/* CFG4 bits */
> +#define DP83869_INT_OE	BIT(7)
> +
> +/* OP MODE */
> +#define DP83869_OP_MODE_MII			BIT(5)
> +#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
> +
> +enum {
> +	DP83869_PORT_MIRRORING_KEEP,
> +	DP83869_PORT_MIRRORING_EN,
> +	DP83869_PORT_MIRRORING_DIS,
> +};

We have met with this in the kernel. Greg was asking us to write exact
value to all enum entries.


> +
> +struct dp83869_private {
> +	int tx_fifo_depth;
> +	int rx_fifo_depth;
> +	int io_impedance;
> +	int port_mirroring;
> +	bool rxctrl_strap_quirk;
> +	int clk_output_sel;
> +	int mode;
> +};

I would prefer kernel-doc format to make reading easier.
Also tx_fifo_depth doesn't look like signed type. I would make them
unsigned.


> +
> +static int dp83869_config_port_mirroring(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +
> +	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
> +		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +					DP83869_GEN_CFG3,
> +					DP83869_CFG3_PORT_MIRROR_EN);
> +	else

you can remove this else.

> +		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
> +					  DP83869_GEN_CFG3,
> +					  DP83869_CFG3_PORT_MIRROR_EN);
> +}
> +
> +static int dp83869_set_strapped_mode(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	u16 val;
> +
> +	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
> +	if (val < 0)
> +		return val;
> +
> +	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF_MDIO

Isn't there a way to remove these?

if (IS_ENABLED(CONFIG_OF_MDIO))
...

> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;
> +	int ret;
> +
> +	if (!of_node)
> +		return -ENODEV;

didn't go deep to this but can this happen?

> +
> +	dp83869->io_impedance = -EINVAL;

I would prefer to group it together. It means move this before dt reading.

> +
> +	/* Optional configuration */
> +	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
> +				   &dp83869->clk_output_sel);
> +	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
> +		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
> +
> +	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
> +	if (ret == 0) {

!ret

> +		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
> +		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
> +			return -EINVAL;
> +	} else {
> +		ret = dp83869_set_strapped_mode(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
> +	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
> +
> +	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
> +		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
> +	} else {
> +		/* If the lane swap is not in the DT then check the straps */
> +		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
> +		if (ret < 0)
> +			return ret;

newline here please.

> +		if (ret & DP83869_STRAP_MIRROR_ENABLED)
> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
> +		else
> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
> +	}
> +
> +	if (of_property_read_u32(of_node, "rx-fifo-depth",
> +				 &dp83869->rx_fifo_depth))
> +		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
> +
> +	if (of_property_read_u32(of_node, "tx-fifo-depth",
> +				 &dp83869->tx_fifo_depth))
> +		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
> +
> +	return ret;
> +}
> +#else
> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	return dp83869_set_strapped_mode(phydev);
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
> +static int dp83869_configure_rgmii(struct phy_device *phydev,
> +				   struct dp83869_private *dp83869)
> +{
> +	int ret = 0, val;
> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
> +		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
> +		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (dp83869->io_impedance >= 0)
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +				       DP83869_IO_MUX_CFG,
> +				       dp83869->io_impedance &
> +				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
> +	return ret;
> +}
> +
> +static int dp83869_config_aneg(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	int err;
> +
> +	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
> +		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
> +				MII_DP83869_FIBER_ADVERTISE);
> +		if (err)
> +			return err;
> +
> +		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
> +				DP83869_SW_RESTART);
> +		if (err)
> +			return err;
> +
> +		return genphy_restart_aneg(phydev);
> +	} else {

you can remove this else.

> +		return genphy_config_aneg(phydev);
> +	}
> +}
> +
> +static int dp83869_configure_mode(struct phy_device *phydev,
> +				  struct dp83869_private *dp83869)
> +{
> +	int phy_ctrl_val;
> +	int ret;
> +
> +	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
> +	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
> +		return -EINVAL;
> +
> +	/* Below init sequence for each operational mode is defined in
> +	 * section 9.4.8 of the datasheet.
> +	 */
> +	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
> +			    dp83869->mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
> +			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
> +			DP83869_PHY_CTRL_DEFAULT);

() are useless.

> +
> +	switch (dp83869->mode) {
> +	case DP83869_RGMII_COPPER_ETHERNET:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
> +				DP83869_CFG1_DEFAULT);
> +		if (ret)
> +			return ret;
> +
> +		ret = dp83869_configure_rgmii(phydev, dp83869);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_RGMII_SGMII_BRIDGE:
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
> +				     DP83869_SGMII_RGMII_BRIDGE,
> +				     DP83869_SGMII_RGMII_BRIDGE);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +

nit: remove this newline - it is not consistent.

> +		break;
> +	case DP83869_1000M_MEDIA_CONVERT:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_100M_MEDIA_CONVERT:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +		break;
> +	case DP83869_SGMII_COPPER_ETHERNET:
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
> +				phy_ctrl_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
> +		if (ret)
> +			return ret;
> +

ditto.

> +		break;
> +	case DP83869_RGMII_100_BASE:
> +	case DP83869_RGMII_1000_BASE:

Does it mean that there is not need to anything to configure?
Comment about it would be good.


> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int dp83869_phy_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	udelay(200);

comment about this delay would be necessary. Just explain where that
200us is coming from.


> +
> +	return 0;
> +}
> +
> +static int dp83869_config_init(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	int ret;
> +
> +	ret = dp83869_phy_reset(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = dp83869_configure_mode(phydev, dp83869);
> +	if (ret)
> +		return ret;
> +
> +	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
> +		dp83869_config_port_mirroring(phydev);
> +
> +	/* Clock output selection if muxing property is set */
> +	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
> +				       DP83869_IO_MUX_CFG,
> +				       dp83869->clk_output_sel <<
> +				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
> +
> +	dp83869_config_aneg(phydev);


You are not checking return value from this function.




> +
> +	return ret;
> +}
> +
> +static int dp83869_probe(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869;
> +	int ret;
> +
> +	dp83869 = kzalloc(sizeof(*dp83869), GFP_KERNEL);
> +	if (!dp83869)
> +		return -ENOMEM;
> +
> +	phydev->priv = dp83869;
> +
> +	ret = dp83869_of_init(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return dp83869_config_init(phydev);
> +}
> +
> +static struct phy_driver dp83869_driver = {
> +	.name = "TI DP83869",
> +	.uid = DP83869_PHY_ID,
> +	.mask = 0xfffffff0,
> +	.features = PHY_GBIT_FEATURES,
> +	.probe = dp83869_probe,
> +	.config = &dp83869_config_init,
> +	.startup = &genphy_startup,
> +	.shutdown = &genphy_shutdown,
> +};
> +
> +int phy_dp83869_init(void)
> +{
> +	phy_register(&dp83869_driver);
> +	return 0;
> +}
> diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
> index 11c4c166b2f5..cb261b86b05d 100644
> --- a/drivers/net/phy/ti_phy_init.c
> +++ b/drivers/net/phy/ti_phy_init.c
> @@ -92,6 +92,10 @@ int phy_ti_init(void)
>  	phy_dp83867_init();
>  #endif
>  
> +#ifdef CONFIG_PHY_DP83869
> +	phy_dp83869_init();
> +#endif
> +
>  #ifdef CONFIG_PHY_TI_GENERIC
>  	phy_register(&dp83822_driver);
>  	phy_register(&dp83825s_driver);
> diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
> index 309da2aacccb..f94ff6291a3f 100644
> --- a/drivers/net/phy/ti_phy_init.h
> +++ b/drivers/net/phy/ti_phy_init.h
> @@ -12,5 +12,6 @@
>  #define _TI_GEN_PHY_H
>  
>  int phy_dp83867_init(void);
> +int phy_dp83869_init(void);
>  
>  #endif /* _TI_GEN_PHY_H */
> diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
> new file mode 100644
> index 000000000000..218b1a64e975
> --- /dev/null
> +++ b/include/dt-bindings/net/ti-dp83869.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Device Tree constants for the Texas Instruments DP83869 PHY
> + *
> + * Author: Dan Murphy <dmurphy at ti.com>
> + *
> + * Copyright:   (C) 2019 Texas Instruments, Inc.
> + */
> +
> +#ifndef _DT_BINDINGS_TI_DP83869_H
> +#define _DT_BINDINGS_TI_DP83869_H
> +
> +/* PHY CTRL bits */
> +#define DP83869_PHYCR_FIFO_DEPTH_3_B_NIB	0x00
> +#define DP83869_PHYCR_FIFO_DEPTH_4_B_NIB	0x01
> +#define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
> +#define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
> +
> +/* IO_MUX_CFG - Clock output selection */
> +#define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
> +#define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
> +#define DP83869_CLK_O_SEL_CHN_C_RCLK		0x2
> +#define DP83869_CLK_O_SEL_CHN_D_RCLK		0x3
> +#define DP83869_CLK_O_SEL_CHN_A_RCLK_DIV5	0x4
> +#define DP83869_CLK_O_SEL_CHN_B_RCLK_DIV5	0x5
> +#define DP83869_CLK_O_SEL_CHN_C_RCLK_DIV5	0x6
> +#define DP83869_CLK_O_SEL_CHN_D_RCLK_DIV5	0x7
> +#define DP83869_CLK_O_SEL_CHN_A_TCLK		0x8
> +#define DP83869_CLK_O_SEL_CHN_B_TCLK		0x9
> +#define DP83869_CLK_O_SEL_CHN_C_TCLK		0xa
> +#define DP83869_CLK_O_SEL_CHN_D_TCLK		0xb
> +#define DP83869_CLK_O_SEL_REF_CLK		0xc
> +
> +#define DP83869_RGMII_COPPER_ETHERNET		0x00
> +#define DP83869_RGMII_1000_BASE			0x01
> +#define DP83869_RGMII_100_BASE			0x02
> +#define DP83869_RGMII_SGMII_BRIDGE		0x03
> +#define DP83869_1000M_MEDIA_CONVERT		0x04
> +#define DP83869_100M_MEDIA_CONVERT		0x05
> +#define DP83869_SGMII_COPPER_ETHERNET		0x06
> +
> +#endif
> 

Thanks,
Michal
Dan Murphy April 21, 2020, 12:04 p.m. UTC | #2
Michal

On 4/21/20 3:23 AM, Michal Simek wrote:
> On 20. 04. 20 20:53, Dan Murphy wrote:
>> Port the DP83869 kernel driver.
>>
>> Signed-off-by: Dan Murphy <dmurphy at ti.com>
>> ---
>>   drivers/net/phy/Kconfig              |   4 +
>>   drivers/net/phy/Makefile             |   1 +
>>   drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++
>>   drivers/net/phy/ti_phy_init.c        |   4 +
>>   drivers/net/phy/ti_phy_init.h        |   1 +
>>   include/dt-bindings/net/ti-dp83869.h |  42 +++
>>   6 files changed, 492 insertions(+)
>>   create mode 100644 drivers/net/phy/dp83869.c
>>   create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index e366f10afc59..5f14bdba0a3b 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>   	select PHY_TI
>>   	bool "Texas Instruments Ethernet DP83867 PHY support"
>>   
>> +config PHY_DP83869
>> +	select PHY_TI
>> +	bool "Texas Instruments Ethernet DP83869 PHY support"
> isn't this a little bit short? I expect checkpatch should be reporting
> issue with it.

Yes but I was following other config flags in this file.


>
>
>> +
>>   config PHY_VITESSE
>>   	bool "Vitesse Ethernet PHYs support"
>>   
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 9c6d31718c00..f8797319154f 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>   obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>   obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>   obj-$(CONFIG_PHY_DP83867) += dp83867.o
>> +obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c
>> new file mode 100644
>> index 000000000000..1eaaea20b37a
>> --- /dev/null
>> +++ b/drivers/net/phy/dp83869.c
>> @@ -0,0 +1,440 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Driver for the Texas Instruments DP83869 PHY
>> + * Copyright (C) 2019 Texas Instruments Inc.
> 2020?

This driver was developed in 2019 and added to the 5.4 kernel so not 
sure we should update the copyright when side porting.

At the very least I will need to add 2019-20

>
>> + */
>> +
>> +#include <common.h>
>> +#include <phy.h>
>> +#include <dm/devres.h>
>> +#include <linux/compat.h>
>> +#include <malloc.h>
>> +
>> +#include <dm.h>
>> +
>> +#include <dt-bindings/net/ti-dp83869.h>
>> +
>> +#include "ti_phy_init.h"
>> +
>> +#define DP83869_PHY_ID		0x2000a0f1
>> +#define DP83869_DEVADDR		0x1f
>> +
>> +#define MII_DP83869_PHYCTRL	0x10
>> +#define MII_DP83869_MICR	0x12
>> +#define MII_DP83869_ISR		0x13
>> +#define DP83869_CTRL		0x1f
>> +#define DP83869_CFG4		0x1e
>> +
>> +/* Extended Registers */
>> +#define DP83869_GEN_CFG3        0x0031
>> +#define DP83869_RGMIICTL	0x0032
>> +#define DP83869_STRAP_STS1	0x006e
>> +#define DP83869_RGMIIDCTL	0x0086
>> +#define DP83869_IO_MUX_CFG	0x0170
>> +#define DP83869_OP_MODE		0x01df
>> +#define DP83869_FX_CTRL		0x0c00
>> +
>> +#define DP83869_SW_RESET	BIT(15)
>> +#define DP83869_SW_RESTART	BIT(14)
>> +
>> +/* MICR Interrupt bits */
>> +#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
>> +#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
>> +#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
>> +
>> +#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
>> +					 BMCR_FULLDPLX | \
>> +					 BMCR_SPEED1000)
>> +
>> +#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
>> +					 ADVERTISE_1000XHALF | \
>> +					 ADVERTISE_1000XFULL | \
>> +					 ADVERTISE_1000XPAUSE | \
>> +					 ADVERTISE_1000XPSE_ASYM)
>> +
>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>> +#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
>> +
>> +/* CFG1 bits */
>> +#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
>> +				 ADVERTISE_1000FULL | \
>> +				 CTL1000_AS_MASTER)
>> +
>> +/* RGMIICTL bits */
>> +#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
>> +#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
>> +
>> +/* STRAP_STS1 bits */
>> +#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
>> +#define DP83869_STRAP_STS1_RESERVED		BIT(11)
>> +#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
>> +
>> +/* PHYCTRL bits */
>> +#define DP83869_RX_FIFO_SHIFT	12
>> +#define DP83869_TX_FIFO_SHIFT	14
>> +
>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>> +#define DP83869_PHY_CTRL_DEFAULT	0x48
>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
>> +#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
>> +
>> +/* RGMIIDCTL bits */
>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
>> +
>> +/* IO_MUX_CFG bits */
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
>> +
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>> +
>> +/* CFG3 bits */
>> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
>> +
>> +/* CFG4 bits */
>> +#define DP83869_INT_OE	BIT(7)
>> +
>> +/* OP MODE */
>> +#define DP83869_OP_MODE_MII			BIT(5)
>> +#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
>> +
>> +enum {
>> +	DP83869_PORT_MIRRORING_KEEP,
>> +	DP83869_PORT_MIRRORING_EN,
>> +	DP83869_PORT_MIRRORING_DIS,
>> +};
> We have met with this in the kernel. Greg was asking us to write exact
> value to all enum entries.
>
Hmm. Can you give me a reference?? I am not doubting you but I would 
like to read that guidance.

That reference will help with an debate I am having in the kernel.


>> +
>> +struct dp83869_private {
>> +	int tx_fifo_depth;
>> +	int rx_fifo_depth;
>> +	int io_impedance;
>> +	int port_mirroring;
>> +	bool rxctrl_strap_quirk;
>> +	int clk_output_sel;
>> +	int mode;
>> +};
> I would prefer kernel-doc format to make reading easier.
> Also tx_fifo_depth doesn't look like signed type. I would make them
> unsigned.
>
>
>> +
>> +static int dp83869_config_port_mirroring(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +
>> +	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
>> +		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +					DP83869_GEN_CFG3,
>> +					DP83869_CFG3_PORT_MIRROR_EN);
>> +	else
> you can remove this else.

Ack


>
>> +		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
>> +					  DP83869_GEN_CFG3,
>> +					  DP83869_CFG3_PORT_MIRROR_EN);
>> +}
>> +
>> +static int dp83869_set_strapped_mode(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	u16 val;
>> +
>> +	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF_MDIO
> Isn't there a way to remove these?
>
> if (IS_ENABLED(CONFIG_OF_MDIO))
> ...
I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change 
this
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
>> +	int ret;
>> +
>> +	if (!of_node)
>> +		return -ENODEV;
> didn't go deep to this but can this happen?

Does every device in the uBoot tree use the DT or do some still use 
board files?

>
>> +
>> +	dp83869->io_impedance = -EINVAL;
> I would prefer to group it together. It means move this before dt reading.
>
>> +
>> +	/* Optional configuration */
>> +	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
>> +				   &dp83869->clk_output_sel);
>> +	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
>> +		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
>> +
>> +	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
>> +	if (ret == 0) {
> !ret
Ack
>
>> +		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
>> +		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
>> +			return -EINVAL;
>> +	} else {
>> +		ret = dp83869_set_strapped_mode(phydev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
>> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
>> +	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
>> +		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
>> +
>> +	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
>> +		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>> +	} else {
>> +		/* If the lane swap is not in the DT then check the straps */
>> +		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>> +		if (ret < 0)
>> +			return ret;
> newline here please.
Ack
>
>> +		if (ret & DP83869_STRAP_MIRROR_ENABLED)
>> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>> +		else
>> +			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
>> +	}
>> +
>> +	if (of_property_read_u32(of_node, "rx-fifo-depth",
>> +				 &dp83869->rx_fifo_depth))
>> +		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>> +
>> +	if (of_property_read_u32(of_node, "tx-fifo-depth",
>> +				 &dp83869->tx_fifo_depth))
>> +		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>> +
>> +	return ret;
>> +}
>> +#else
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	return dp83869_set_strapped_mode(phydev);
>> +}
>> +#endif /* CONFIG_OF_MDIO */
>> +
>> +static int dp83869_configure_rgmii(struct phy_device *phydev,
>> +				   struct dp83869_private *dp83869)
>> +{
>> +	int ret = 0, val;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
>> +		if (val < 0)
>> +			return val;
>> +
>> +		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
>> +		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
>> +		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (dp83869->io_impedance >= 0)
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +				       DP83869_IO_MUX_CFG,
>> +				       dp83869->io_impedance &
>> +				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
>> +	return ret;
>> +}
>> +
>> +static int dp83869_config_aneg(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	int err;
>> +
>> +	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
>> +		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
>> +				MII_DP83869_FIBER_ADVERTISE);
>> +		if (err)
>> +			return err;
>> +
>> +		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
>> +				DP83869_SW_RESTART);
>> +		if (err)
>> +			return err;
>> +
>> +		return genphy_restart_aneg(phydev);
>> +	} else {
> you can remove this else.
Ack
>
>> +		return genphy_config_aneg(phydev);
>> +	}
>> +}
>> +
>> +static int dp83869_configure_mode(struct phy_device *phydev,
>> +				  struct dp83869_private *dp83869)
>> +{
>> +	int phy_ctrl_val;
>> +	int ret;
>> +
>> +	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
>> +	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
>> +		return -EINVAL;
>> +
>> +	/* Below init sequence for each operational mode is defined in
>> +	 * section 9.4.8 of the datasheet.
>> +	 */
>> +	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
>> +			    dp83869->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
>> +			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
>> +			DP83869_PHY_CTRL_DEFAULT);
> () are useless.
Ack
>
>> +
>> +	switch (dp83869->mode) {
>> +	case DP83869_RGMII_COPPER_ETHERNET:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
>> +				DP83869_CFG1_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = dp83869_configure_rgmii(phydev, dp83869);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_RGMII_SGMII_BRIDGE:
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
>> +				     DP83869_SGMII_RGMII_BRIDGE,
>> +				     DP83869_SGMII_RGMII_BRIDGE);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
> nit: remove this newline - it is not consistent.
Ack
>
>> +		break;
>> +	case DP83869_1000M_MEDIA_CONVERT:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_100M_MEDIA_CONVERT:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case DP83869_SGMII_COPPER_ETHERNET:
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
>> +				phy_ctrl_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
>> +				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
>> +		if (ret)
>> +			return ret;
>> +
> ditto.
Ack
>
>> +		break;
>> +	case DP83869_RGMII_100_BASE:
>> +	case DP83869_RGMII_1000_BASE:
> Does it mean that there is not need to anything to configure?
> Comment about it would be good.

I will add a comment as one of these is actually a fiber connection 
which I have not figured out how uBoot handles fiber connections yet.


>
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int dp83869_phy_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	udelay(200);
> comment about this delay would be necessary. Just explain where that
> 200us is coming from.

Ack.


>
>> +
>> +	return 0;
>> +}
>> +
>> +static int dp83869_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	int ret;
>> +
>> +	ret = dp83869_phy_reset(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dp83869_configure_mode(phydev, dp83869);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
>> +		dp83869_config_port_mirroring(phydev);
>> +
>> +	/* Clock output selection if muxing property is set */
>> +	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
>> +		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
>> +				       DP83869_IO_MUX_CFG,
>> +				       dp83869->clk_output_sel <<
>> +				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
>> +
>> +	dp83869_config_aneg(phydev);
>
> You are not checking return value from this function.

Ack


Dan
Michal Simek April 21, 2020, 12:39 p.m. UTC | #3
On 21. 04. 20 14:04, Dan Murphy wrote:
> Michal
> 
> On 4/21/20 3:23 AM, Michal Simek wrote:
>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>> Port the DP83869 kernel driver.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy at ti.com>
>>> ---
>>> ? drivers/net/phy/Kconfig????????????? |?? 4 +
>>> ? drivers/net/phy/Makefile???????????? |?? 1 +
>>> ? drivers/net/phy/dp83869.c??????????? | 440 +++++++++++++++++++++++++++
>>> ? drivers/net/phy/ti_phy_init.c??????? |?? 4 +
>>> ? drivers/net/phy/ti_phy_init.h??????? |?? 1 +
>>> ? include/dt-bindings/net/ti-dp83869.h |? 42 +++
>>> ? 6 files changed, 492 insertions(+)
>>> ? create mode 100644 drivers/net/phy/dp83869.c
>>> ? create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>>
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index e366f10afc59..5f14bdba0a3b 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>> ????? select PHY_TI
>>> ????? bool "Texas Instruments Ethernet DP83867 PHY support"
>>> ? +config PHY_DP83869
>>> +??? select PHY_TI
>>> +??? bool "Texas Instruments Ethernet DP83869 PHY support"
>> isn't this a little bit short? I expect checkpatch should be reporting
>> issue with it.
> 
> Yes but I was following other config flags in this file.

Just no reason to follow bad examples. :-)



>>
>>
>>> +
>>> ? config PHY_VITESSE
>>> ????? bool "Vitesse Ethernet PHYs support"
>>> ? diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>>> index 9c6d31718c00..f8797319154f 100644
>>> --- a/drivers/net/phy/Makefile
>>> +++ b/drivers/net/phy/Makefile
>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>> ? obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>> ? obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>> ? obj-$(CONFIG_PHY_DP83867) += dp83867.o
>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c
>>> new file mode 100644
>>> index 000000000000..1eaaea20b37a
>>> --- /dev/null
>>> +++ b/drivers/net/phy/dp83869.c
>>> @@ -0,0 +1,440 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Driver for the Texas Instruments DP83869 PHY
>>> + * Copyright (C) 2019 Texas Instruments Inc.
>> 2020?
> 
> This driver was developed in 2019 and added to the 5.4 kernel so not
> sure we should update the copyright when side porting.
> 
> At the very least I will need to add 2019-20

yup.

> 
>>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <phy.h>
>>> +#include <dm/devres.h>
>>> +#include <linux/compat.h>
>>> +#include <malloc.h>
>>> +
>>> +#include <dm.h>
>>> +
>>> +#include <dt-bindings/net/ti-dp83869.h>
>>> +
>>> +#include "ti_phy_init.h"
>>> +
>>> +#define DP83869_PHY_ID??????? 0x2000a0f1
>>> +#define DP83869_DEVADDR??????? 0x1f
>>> +
>>> +#define MII_DP83869_PHYCTRL??? 0x10
>>> +#define MII_DP83869_MICR??? 0x12
>>> +#define MII_DP83869_ISR??????? 0x13
>>> +#define DP83869_CTRL??????? 0x1f
>>> +#define DP83869_CFG4??????? 0x1e
>>> +
>>> +/* Extended Registers */
>>> +#define DP83869_GEN_CFG3??????? 0x0031
>>> +#define DP83869_RGMIICTL??? 0x0032
>>> +#define DP83869_STRAP_STS1??? 0x006e
>>> +#define DP83869_RGMIIDCTL??? 0x0086
>>> +#define DP83869_IO_MUX_CFG??? 0x0170
>>> +#define DP83869_OP_MODE??????? 0x01df
>>> +#define DP83869_FX_CTRL??????? 0x0c00
>>> +
>>> +#define DP83869_SW_RESET??? BIT(15)
>>> +#define DP83869_SW_RESTART??? BIT(14)
>>> +
>>> +/* MICR Interrupt bits */
>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN??????? BIT(15)
>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN??? BIT(14)
>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN??? BIT(13)
>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN??? BIT(12)
>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN??? BIT(11)
>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN??? BIT(10)
>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN??? BIT(8)
>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN??? BIT(4)
>>> +#define MII_DP83869_MICR_WOL_INT_EN??????? BIT(3)
>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN??? BIT(2)
>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN??? BIT(1)
>>> +#define MII_DP83869_MICR_JABBER_INT_EN??????? BIT(0)
>>> +
>>> +#define MII_DP83869_BMCR_DEFAULT??? (BMCR_ANENABLE | \
>>> +???????????????????? BMCR_FULLDPLX | \
>>> +???????????????????? BMCR_SPEED1000)
>>> +
>>> +#define MII_DP83869_FIBER_ADVERTISE??? (ADVERTISE_CSMA | \
>>> +???????????????????? ADVERTISE_1000XHALF | \
>>> +???????????????????? ADVERTISE_1000XFULL | \
>>> +???????????????????? ADVERTISE_1000XPAUSE | \
>>> +???????????????????? ADVERTISE_1000XPSE_ASYM)
>>> +
>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>>> +#define DP83869_FX_CTRL_DEFAULT??? MII_DP83869_BMCR_DEFAULT
>>> +
>>> +/* CFG1 bits */
>>> +#define DP83869_CFG1_DEFAULT??? (ADVERTISE_1000HALF | \
>>> +???????????????? ADVERTISE_1000FULL | \
>>> +???????????????? CTL1000_AS_MASTER)
>>> +
>>> +/* RGMIICTL bits */
>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN??????? BIT(1)
>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN??????? BIT(0)
>>> +
>>> +/* STRAP_STS1 bits */
>>> +#define DP83869_STRAP_OP_MODE_MASK??????? GENMASK(2, 0)
>>> +#define DP83869_STRAP_STS1_RESERVED??????? BIT(11)
>>> +#define DP83869_STRAP_MIRROR_ENABLED??????? BIT(12)
>>> +
>>> +/* PHYCTRL bits */
>>> +#define DP83869_RX_FIFO_SHIFT??? 12
>>> +#define DP83869_TX_FIFO_SHIFT??? 14
>>> +
>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>>> +#define DP83869_PHY_CTRL_DEFAULT??? 0x48
>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK??? GENMASK(15, 12)
>>> +#define DP83869_PHYCR_RESERVED_MASK??? BIT(11)
>>> +
>>> +/* RGMIIDCTL bits */
>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT??? 4
>>> +
>>> +/* IO_MUX_CFG bits */
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL??? 0x1f
>>> +
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX??? 0x0
>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN??? 0x1f
>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK??? (0x1f << 8)
>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT??? 8
>>> +
>>> +/* CFG3 bits */
>>> +#define DP83869_CFG3_PORT_MIRROR_EN????????????? BIT(0)
>>> +
>>> +/* CFG4 bits */
>>> +#define DP83869_INT_OE??? BIT(7)
>>> +
>>> +/* OP MODE */
>>> +#define DP83869_OP_MODE_MII??????????? BIT(5)
>>> +#define DP83869_SGMII_RGMII_BRIDGE??????? BIT(6)
>>> +
>>> +enum {
>>> +??? DP83869_PORT_MIRRORING_KEEP,
>>> +??? DP83869_PORT_MIRRORING_EN,
>>> +??? DP83869_PORT_MIRRORING_DIS,
>>> +};
>> We have met with this in the kernel. Greg was asking us to write exact
>> value to all enum entries.
>>
> Hmm. Can you give me a reference?? I am not doubting you but I would
> like to read that guidance.
> 
> That reference will help with an debate I am having in the kernel.

Take a look at this thread.
https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094 at kroah.com/

 <snip>

>>> +
>>> +#ifdef CONFIG_OF_MDIO
>> Isn't there a way to remove these?
>>
>> if (IS_ENABLED(CONFIG_OF_MDIO))
>> ...
> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change
> this

There are a lot of places which should be update/done better.


>>> +static int dp83869_of_init(struct phy_device *phydev)
>>> +{
>>> +??? struct dp83869_private *dp83869 = phydev->priv;
>>> +??? struct device *dev = &phydev->mdio.dev;
>>> +??? struct device_node *of_node = dev->of_node;
>>> +??? int ret;
>>> +
>>> +??? if (!of_node)
>>> +??????? return -ENODEV;
>> didn't go deep to this but can this happen?
> 
> Does every device in the uBoot tree use the DT or do some still use
> board files?

IIRC ethernet phys are not based on driver model that's why devices are
not created for it and I am not quite sure if platdata are supported.

I think question is what way you use. If you use just OF_MDIO/DM_ETH
then Kconfig should have dependencies. And if someone else want to run
it without it (which is IMHO unlikely) then they can update the driver self.

>>
>>> +
>>> +??? dp83869->io_impedance = -EINVAL;
>> I would prefer to group it together. It means move this before dt
>> reading.
>>

No reaction on this line that's why just commenting it that you spot it.

Thanks,
Michal
Dan Murphy April 21, 2020, 2:27 p.m. UTC | #4
Michal

On 4/21/20 7:39 AM, Michal Simek wrote:
> On 21. 04. 20 14:04, Dan Murphy wrote:
>> Michal
>>
>> On 4/21/20 3:23 AM, Michal Simek wrote:
>>> On 20. 04. 20 20:53, Dan Murphy wrote:
>>>> Port the DP83869 kernel driver.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy at ti.com>
>>>> ---
>>>>  ? drivers/net/phy/Kconfig????????????? |?? 4 +
>>>>  ? drivers/net/phy/Makefile???????????? |?? 1 +
>>>>  ? drivers/net/phy/dp83869.c??????????? | 440 +++++++++++++++++++++++++++
>>>>  ? drivers/net/phy/ti_phy_init.c??????? |?? 4 +
>>>>  ? drivers/net/phy/ti_phy_init.h??????? |?? 1 +
>>>>  ? include/dt-bindings/net/ti-dp83869.h |? 42 +++
>>>>  ? 6 files changed, 492 insertions(+)
>>>>  ? create mode 100644 drivers/net/phy/dp83869.c
>>>>  ? create mode 100644 include/dt-bindings/net/ti-dp83869.h
>>>>
>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>>> index e366f10afc59..5f14bdba0a3b 100644
>>>> --- a/drivers/net/phy/Kconfig
>>>> +++ b/drivers/net/phy/Kconfig
>>>> @@ -252,6 +252,10 @@ config PHY_DP83867
>>>>  ????? select PHY_TI
>>>>  ????? bool "Texas Instruments Ethernet DP83867 PHY support"
>>>>  ? +config PHY_DP83869
>>>> +??? select PHY_TI
>>>> +??? bool "Texas Instruments Ethernet DP83869 PHY support"
>>> isn't this a little bit short? I expect checkpatch should be reporting
>>> issue with it.
>> Yes but I was following other config flags in this file.
> Just no reason to follow bad examples. :-)

True.

I will update the examples for the TI PHYs.


>
>
>
>>>
>>>> +
>>>>  ? config PHY_VITESSE
>>>>  ????? bool "Vitesse Ethernet PHYs support"
>>>>  ? diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>>>> index 9c6d31718c00..f8797319154f 100644
>>>> --- a/drivers/net/phy/Makefile
>>>> +++ b/drivers/net/phy/Makefile
>>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>>>  ? obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>>>  ? obj-$(CONFIG_PHY_TI) += ti_phy_init.o
>>>>  ? obj-$(CONFIG_PHY_DP83867) += dp83867.o
>>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c
>>>> new file mode 100644
>>>> index 000000000000..1eaaea20b37a
>>>> --- /dev/null
>>>> +++ b/drivers/net/phy/dp83869.c
>>>> @@ -0,0 +1,440 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/* Driver for the Texas Instruments DP83869 PHY
>>>> + * Copyright (C) 2019 Texas Instruments Inc.
>>> 2020?
>> This driver was developed in 2019 and added to the 5.4 kernel so not
>> sure we should update the copyright when side porting.
>>
>> At the very least I will need to add 2019-20
> yup.


Ack

>
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <phy.h>
>>>> +#include <dm/devres.h>
>>>> +#include <linux/compat.h>
>>>> +#include <malloc.h>
>>>> +
>>>> +#include <dm.h>
>>>> +
>>>> +#include <dt-bindings/net/ti-dp83869.h>
>>>> +
>>>> +#include "ti_phy_init.h"
>>>> +
>>>> +#define DP83869_PHY_ID??????? 0x2000a0f1
>>>> +#define DP83869_DEVADDR??????? 0x1f
>>>> +
>>>> +#define MII_DP83869_PHYCTRL??? 0x10
>>>> +#define MII_DP83869_MICR??? 0x12
>>>> +#define MII_DP83869_ISR??????? 0x13
>>>> +#define DP83869_CTRL??????? 0x1f
>>>> +#define DP83869_CFG4??????? 0x1e
>>>> +
>>>> +/* Extended Registers */
>>>> +#define DP83869_GEN_CFG3??????? 0x0031
>>>> +#define DP83869_RGMIICTL??? 0x0032
>>>> +#define DP83869_STRAP_STS1??? 0x006e
>>>> +#define DP83869_RGMIIDCTL??? 0x0086
>>>> +#define DP83869_IO_MUX_CFG??? 0x0170
>>>> +#define DP83869_OP_MODE??????? 0x01df
>>>> +#define DP83869_FX_CTRL??????? 0x0c00
>>>> +
>>>> +#define DP83869_SW_RESET??? BIT(15)
>>>> +#define DP83869_SW_RESTART??? BIT(14)
>>>> +
>>>> +/* MICR Interrupt bits */
>>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN??????? BIT(15)
>>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN??? BIT(14)
>>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN??? BIT(13)
>>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN??? BIT(12)
>>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN??? BIT(11)
>>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN??? BIT(10)
>>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN??? BIT(8)
>>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN??? BIT(4)
>>>> +#define MII_DP83869_MICR_WOL_INT_EN??????? BIT(3)
>>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN??? BIT(2)
>>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN??? BIT(1)
>>>> +#define MII_DP83869_MICR_JABBER_INT_EN??????? BIT(0)
>>>> +
>>>> +#define MII_DP83869_BMCR_DEFAULT??? (BMCR_ANENABLE | \
>>>> +???????????????????? BMCR_FULLDPLX | \
>>>> +???????????????????? BMCR_SPEED1000)
>>>> +
>>>> +#define MII_DP83869_FIBER_ADVERTISE??? (ADVERTISE_CSMA | \
>>>> +???????????????????? ADVERTISE_1000XHALF | \
>>>> +???????????????????? ADVERTISE_1000XFULL | \
>>>> +???????????????????? ADVERTISE_1000XPAUSE | \
>>>> +???????????????????? ADVERTISE_1000XPSE_ASYM)
>>>> +
>>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */
>>>> +#define DP83869_FX_CTRL_DEFAULT??? MII_DP83869_BMCR_DEFAULT
>>>> +
>>>> +/* CFG1 bits */
>>>> +#define DP83869_CFG1_DEFAULT??? (ADVERTISE_1000HALF | \
>>>> +???????????????? ADVERTISE_1000FULL | \
>>>> +???????????????? CTL1000_AS_MASTER)
>>>> +
>>>> +/* RGMIICTL bits */
>>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN??????? BIT(1)
>>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN??????? BIT(0)
>>>> +
>>>> +/* STRAP_STS1 bits */
>>>> +#define DP83869_STRAP_OP_MODE_MASK??????? GENMASK(2, 0)
>>>> +#define DP83869_STRAP_STS1_RESERVED??????? BIT(11)
>>>> +#define DP83869_STRAP_MIRROR_ENABLED??????? BIT(12)
>>>> +
>>>> +/* PHYCTRL bits */
>>>> +#define DP83869_RX_FIFO_SHIFT??? 12
>>>> +#define DP83869_TX_FIFO_SHIFT??? 14
>>>> +
>>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */
>>>> +#define DP83869_PHY_CTRL_DEFAULT??? 0x48
>>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK??? GENMASK(15, 12)
>>>> +#define DP83869_PHYCR_RESERVED_MASK??? BIT(11)
>>>> +
>>>> +/* RGMIIDCTL bits */
>>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT??? 4
>>>> +
>>>> +/* IO_MUX_CFG bits */
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL??? 0x1f
>>>> +
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX??? 0x0
>>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN??? 0x1f
>>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK??? (0x1f << 8)
>>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT??? 8
>>>> +
>>>> +/* CFG3 bits */
>>>> +#define DP83869_CFG3_PORT_MIRROR_EN????????????? BIT(0)
>>>> +
>>>> +/* CFG4 bits */
>>>> +#define DP83869_INT_OE??? BIT(7)
>>>> +
>>>> +/* OP MODE */
>>>> +#define DP83869_OP_MODE_MII??????????? BIT(5)
>>>> +#define DP83869_SGMII_RGMII_BRIDGE??????? BIT(6)
>>>> +
>>>> +enum {
>>>> +??? DP83869_PORT_MIRRORING_KEEP,
>>>> +??? DP83869_PORT_MIRRORING_EN,
>>>> +??? DP83869_PORT_MIRRORING_DIS,
>>>> +};
>>> We have met with this in the kernel. Greg was asking us to write exact
>>> value to all enum entries.
>>>
>> Hmm. Can you give me a reference?? I am not doubting you but I would
>> like to read that guidance.
>>
>> That reference will help with an debate I am having in the kernel.
> Take a look at this thread.
> https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094 at kroah.com/
Thank you
>   <snip>
>
>>>> +
>>>> +#ifdef CONFIG_OF_MDIO
>>> Isn't there a way to remove these?
>>>
>>> if (IS_ENABLED(CONFIG_OF_MDIO))
>>> ...
>> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change
>> this
> There are a lot of places which should be update/done better.
Are you inferring that this is not correct either?
>
>
>>>> +static int dp83869_of_init(struct phy_device *phydev)
>>>> +{
>>>> +??? struct dp83869_private *dp83869 = phydev->priv;
>>>> +??? struct device *dev = &phydev->mdio.dev;
>>>> +??? struct device_node *of_node = dev->of_node;
>>>> +??? int ret;
>>>> +
>>>> +??? if (!of_node)
>>>> +??????? return -ENODEV;
>>> didn't go deep to this but can this happen?
>> Does every device in the uBoot tree use the DT or do some still use
>> board files?
> IIRC ethernet phys are not based on driver model that's why devices are
> not created for it and I am not quite sure if platdata are supported.
>
> I think question is what way you use. If you use just OF_MDIO/DM_ETH
> then Kconfig should have dependencies. And if someone else want to run
> it without it (which is IMHO unlikely) then they can update the driver self.

Well technically some phys like this one may not need DT at all if 
strapped in hardware.


>>>> +
>>>> +??? dp83869->io_impedance = -EINVAL;
>>> I would prefer to group it together. It means move this before dt
>>> reading.
>>>
> No reaction on this line that's why just commenting it that you spot it.

I had to look at it in detail.? Not sure why this was set there. This 
should be removed

Dan


> Thanks,
> Michal
>
>
>
Christian Gmeiner April 27, 2021, 7:01 a.m. UTC | #5
Hi Dan

I am interested to see the mainline support for the DP83869 phy. Is
there any progress on this patch or are there any blocker?

Am Di., 21. Apr. 2020 um 16:35 Uhr schrieb Dan Murphy <dmurphy@ti.com>:
>

> Michal

>

> On 4/21/20 7:39 AM, Michal Simek wrote:

> > On 21. 04. 20 14:04, Dan Murphy wrote:

> >> Michal

> >>

> >> On 4/21/20 3:23 AM, Michal Simek wrote:

> >>> On 20. 04. 20 20:53, Dan Murphy wrote:

> >>>> Port the DP83869 kernel driver.

> >>>>

> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> >>>> ---

> >>>>    drivers/net/phy/Kconfig              |   4 +

> >>>>    drivers/net/phy/Makefile             |   1 +

> >>>>    drivers/net/phy/dp83869.c            | 440 +++++++++++++++++++++++++++

> >>>>    drivers/net/phy/ti_phy_init.c        |   4 +

> >>>>    drivers/net/phy/ti_phy_init.h        |   1 +

> >>>>    include/dt-bindings/net/ti-dp83869.h |  42 +++

> >>>>    6 files changed, 492 insertions(+)

> >>>>    create mode 100644 drivers/net/phy/dp83869.c

> >>>>    create mode 100644 include/dt-bindings/net/ti-dp83869.h

> >>>>

> >>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig

> >>>> index e366f10afc59..5f14bdba0a3b 100644

> >>>> --- a/drivers/net/phy/Kconfig

> >>>> +++ b/drivers/net/phy/Kconfig

> >>>> @@ -252,6 +252,10 @@ config PHY_DP83867

> >>>>        select PHY_TI

> >>>>        bool "Texas Instruments Ethernet DP83867 PHY support"

> >>>>    +config PHY_DP83869

> >>>> +    select PHY_TI

> >>>> +    bool "Texas Instruments Ethernet DP83869 PHY support"

> >>> isn't this a little bit short? I expect checkpatch should be reporting

> >>> issue with it.

> >> Yes but I was following other config flags in this file.

> > Just no reason to follow bad examples. :-)

>

> True.

>

> I will update the examples for the TI PHYs.

>

>

> >

> >

> >

> >>>

> >>>> +

> >>>>    config PHY_VITESSE

> >>>>        bool "Vitesse Ethernet PHYs support"

> >>>>    diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile

> >>>> index 9c6d31718c00..f8797319154f 100644

> >>>> --- a/drivers/net/phy/Makefile

> >>>> +++ b/drivers/net/phy/Makefile

> >>>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o

> >>>>    obj-$(CONFIG_PHY_TERANETICS) += teranetics.o

> >>>>    obj-$(CONFIG_PHY_TI) += ti_phy_init.o

> >>>>    obj-$(CONFIG_PHY_DP83867) += dp83867.o

> >>>> +obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c

> >>>> new file mode 100644

> >>>> index 000000000000..1eaaea20b37a

> >>>> --- /dev/null

> >>>> +++ b/drivers/net/phy/dp83869.c

> >>>> @@ -0,0 +1,440 @@

> >>>> +// SPDX-License-Identifier: GPL-2.0

> >>>> +/* Driver for the Texas Instruments DP83869 PHY

> >>>> + * Copyright (C) 2019 Texas Instruments Inc.

> >>> 2020?

> >> This driver was developed in 2019 and added to the 5.4 kernel so not

> >> sure we should update the copyright when side porting.

> >>

> >> At the very least I will need to add 2019-20

> > yup.

>

>

> Ack

>

> >

> >>>> + */

> >>>> +

> >>>> +#include <common.h>

> >>>> +#include <phy.h>

> >>>> +#include <dm/devres.h>

> >>>> +#include <linux/compat.h>

> >>>> +#include <malloc.h>

> >>>> +

> >>>> +#include <dm.h>

> >>>> +

> >>>> +#include <dt-bindings/net/ti-dp83869.h>

> >>>> +

> >>>> +#include "ti_phy_init.h"

> >>>> +

> >>>> +#define DP83869_PHY_ID        0x2000a0f1

> >>>> +#define DP83869_DEVADDR        0x1f

> >>>> +

> >>>> +#define MII_DP83869_PHYCTRL    0x10

> >>>> +#define MII_DP83869_MICR    0x12

> >>>> +#define MII_DP83869_ISR        0x13

> >>>> +#define DP83869_CTRL        0x1f

> >>>> +#define DP83869_CFG4        0x1e

> >>>> +

> >>>> +/* Extended Registers */

> >>>> +#define DP83869_GEN_CFG3        0x0031

> >>>> +#define DP83869_RGMIICTL    0x0032

> >>>> +#define DP83869_STRAP_STS1    0x006e

> >>>> +#define DP83869_RGMIIDCTL    0x0086

> >>>> +#define DP83869_IO_MUX_CFG    0x0170

> >>>> +#define DP83869_OP_MODE        0x01df

> >>>> +#define DP83869_FX_CTRL        0x0c00

> >>>> +

> >>>> +#define DP83869_SW_RESET    BIT(15)

> >>>> +#define DP83869_SW_RESTART    BIT(14)

> >>>> +

> >>>> +/* MICR Interrupt bits */

> >>>> +#define MII_DP83869_MICR_AN_ERR_INT_EN        BIT(15)

> >>>> +#define MII_DP83869_MICR_SPEED_CHNG_INT_EN    BIT(14)

> >>>> +#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN    BIT(13)

> >>>> +#define MII_DP83869_MICR_PAGE_RXD_INT_EN    BIT(12)

> >>>> +#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN    BIT(11)

> >>>> +#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN    BIT(10)

> >>>> +#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN    BIT(8)

> >>>> +#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN    BIT(4)

> >>>> +#define MII_DP83869_MICR_WOL_INT_EN        BIT(3)

> >>>> +#define MII_DP83869_MICR_XGMII_ERR_INT_EN    BIT(2)

> >>>> +#define MII_DP83869_MICR_POL_CHNG_INT_EN    BIT(1)

> >>>> +#define MII_DP83869_MICR_JABBER_INT_EN        BIT(0)

> >>>> +

> >>>> +#define MII_DP83869_BMCR_DEFAULT    (BMCR_ANENABLE | \

> >>>> +                     BMCR_FULLDPLX | \

> >>>> +                     BMCR_SPEED1000)

> >>>> +

> >>>> +#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISE_CSMA | \

> >>>> +                     ADVERTISE_1000XHALF | \

> >>>> +                     ADVERTISE_1000XFULL | \

> >>>> +                     ADVERTISE_1000XPAUSE | \

> >>>> +                     ADVERTISE_1000XPSE_ASYM)

> >>>> +

> >>>> +/* This is the same bit mask as the BMCR so re-use the BMCR default */

> >>>> +#define DP83869_FX_CTRL_DEFAULT    MII_DP83869_BMCR_DEFAULT

> >>>> +

> >>>> +/* CFG1 bits */

> >>>> +#define DP83869_CFG1_DEFAULT    (ADVERTISE_1000HALF | \

> >>>> +                 ADVERTISE_1000FULL | \

> >>>> +                 CTL1000_AS_MASTER)

> >>>> +

> >>>> +/* RGMIICTL bits */

> >>>> +#define DP83869_RGMII_TX_CLK_DELAY_EN        BIT(1)

> >>>> +#define DP83869_RGMII_RX_CLK_DELAY_EN        BIT(0)

> >>>> +

> >>>> +/* STRAP_STS1 bits */

> >>>> +#define DP83869_STRAP_OP_MODE_MASK        GENMASK(2, 0)

> >>>> +#define DP83869_STRAP_STS1_RESERVED        BIT(11)

> >>>> +#define DP83869_STRAP_MIRROR_ENABLED        BIT(12)

> >>>> +

> >>>> +/* PHYCTRL bits */

> >>>> +#define DP83869_RX_FIFO_SHIFT    12

> >>>> +#define DP83869_TX_FIFO_SHIFT    14

> >>>> +

> >>>> +/* PHY_CTRL lower bytes 0x48 are declared as reserved */

> >>>> +#define DP83869_PHY_CTRL_DEFAULT    0x48

> >>>> +#define DP83869_PHYCR_FIFO_DEPTH_MASK    GENMASK(15, 12)

> >>>> +#define DP83869_PHYCR_RESERVED_MASK    BIT(11)

> >>>> +

> >>>> +/* RGMIIDCTL bits */

> >>>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT    4

> >>>> +

> >>>> +/* IO_MUX_CFG bits */

> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL    0x1f

> >>>> +

> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX    0x0

> >>>> +#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN    0x1f

> >>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)

> >>>> +#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT    8

> >>>> +

> >>>> +/* CFG3 bits */

> >>>> +#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)

> >>>> +

> >>>> +/* CFG4 bits */

> >>>> +#define DP83869_INT_OE    BIT(7)

> >>>> +

> >>>> +/* OP MODE */

> >>>> +#define DP83869_OP_MODE_MII            BIT(5)

> >>>> +#define DP83869_SGMII_RGMII_BRIDGE        BIT(6)

> >>>> +

> >>>> +enum {

> >>>> +    DP83869_PORT_MIRRORING_KEEP,

> >>>> +    DP83869_PORT_MIRRORING_EN,

> >>>> +    DP83869_PORT_MIRRORING_DIS,

> >>>> +};

> >>> We have met with this in the kernel. Greg was asking us to write exact

> >>> value to all enum entries.

> >>>

> >> Hmm. Can you give me a reference?  I am not doubting you but I would

> >> like to read that guidance.

> >>

> >> That reference will help with an debate I am having in the kernel.

> > Take a look at this thread.

> > https://lore.kernel.org/linux-arm-kernel/20200318125003.GA2727094@kroah.com/

> Thank you

> >   <snip>

> >

> >>>> +

> >>>> +#ifdef CONFIG_OF_MDIO

> >>> Isn't there a way to remove these?

> >>>

> >>> if (IS_ENABLED(CONFIG_OF_MDIO))

> >>> ...

> >> I looked at the 83867 it uses #if defined(CONFIG_DM_ETH) so I can change

> >> this

> > There are a lot of places which should be update/done better.

> Are you inferring that this is not correct either?

> >

> >

> >>>> +static int dp83869_of_init(struct phy_device *phydev)

> >>>> +{

> >>>> +    struct dp83869_private *dp83869 = phydev->priv;

> >>>> +    struct device *dev = &phydev->mdio.dev;

> >>>> +    struct device_node *of_node = dev->of_node;

> >>>> +    int ret;

> >>>> +

> >>>> +    if (!of_node)

> >>>> +        return -ENODEV;

> >>> didn't go deep to this but can this happen?

> >> Does every device in the uBoot tree use the DT or do some still use

> >> board files?

> > IIRC ethernet phys are not based on driver model that's why devices are

> > not created for it and I am not quite sure if platdata are supported.

> >

> > I think question is what way you use. If you use just OF_MDIO/DM_ETH

> > then Kconfig should have dependencies. And if someone else want to run

> > it without it (which is IMHO unlikely) then they can update the driver self.

>

> Well technically some phys like this one may not need DT at all if

> strapped in hardware.

>

>

> >>>> +

> >>>> +    dp83869->io_impedance = -EINVAL;

> >>> I would prefer to group it together. It means move this before dt

> >>> reading.

> >>>

> > No reaction on this line that's why just commenting it that you spot it.

>

> I had to look at it in detail.  Not sure why this was set there. This

> should be removed

>

> Dan

>

>

> > Thanks,

> > Michal

> >

> >

> >




-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index e366f10afc59..5f14bdba0a3b 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -252,6 +252,10 @@  config PHY_DP83867
 	select PHY_TI
 	bool "Texas Instruments Ethernet DP83867 PHY support"
 
+config PHY_DP83869
+	select PHY_TI
+	bool "Texas Instruments Ethernet DP83869 PHY support"
+
 config PHY_VITESSE
 	bool "Vitesse Ethernet PHYs support"
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 9c6d31718c00..f8797319154f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -27,6 +27,7 @@  obj-$(CONFIG_PHY_SMSC) += smsc.o
 obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
 obj-$(CONFIG_PHY_TI) += ti_phy_init.o
 obj-$(CONFIG_PHY_DP83867) += dp83867.o
+obj-$(CONFIG_PHY_DP83869) += dp83869.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/dp83869.c b/drivers/net/phy/dp83869.c
new file mode 100644
index 000000000000..1eaaea20b37a
--- /dev/null
+++ b/drivers/net/phy/dp83869.c
@@ -0,0 +1,440 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for the Texas Instruments DP83869 PHY
+ * Copyright (C) 2019 Texas Instruments Inc.
+ */
+
+#include <common.h>
+#include <phy.h>
+#include <dm/devres.h>
+#include <linux/compat.h>
+#include <malloc.h>
+
+#include <dm.h>
+
+#include <dt-bindings/net/ti-dp83869.h>
+
+#include "ti_phy_init.h"
+
+#define DP83869_PHY_ID		0x2000a0f1
+#define DP83869_DEVADDR		0x1f
+
+#define MII_DP83869_PHYCTRL	0x10
+#define MII_DP83869_MICR	0x12
+#define MII_DP83869_ISR		0x13
+#define DP83869_CTRL		0x1f
+#define DP83869_CFG4		0x1e
+
+/* Extended Registers */
+#define DP83869_GEN_CFG3        0x0031
+#define DP83869_RGMIICTL	0x0032
+#define DP83869_STRAP_STS1	0x006e
+#define DP83869_RGMIIDCTL	0x0086
+#define DP83869_IO_MUX_CFG	0x0170
+#define DP83869_OP_MODE		0x01df
+#define DP83869_FX_CTRL		0x0c00
+
+#define DP83869_SW_RESET	BIT(15)
+#define DP83869_SW_RESTART	BIT(14)
+
+/* MICR Interrupt bits */
+#define MII_DP83869_MICR_AN_ERR_INT_EN		BIT(15)
+#define MII_DP83869_MICR_SPEED_CHNG_INT_EN	BIT(14)
+#define MII_DP83869_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
+#define MII_DP83869_MICR_PAGE_RXD_INT_EN	BIT(12)
+#define MII_DP83869_MICR_AUTONEG_COMP_INT_EN	BIT(11)
+#define MII_DP83869_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
+#define MII_DP83869_MICR_FALSE_CARRIER_INT_EN	BIT(8)
+#define MII_DP83869_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
+#define MII_DP83869_MICR_WOL_INT_EN		BIT(3)
+#define MII_DP83869_MICR_XGMII_ERR_INT_EN	BIT(2)
+#define MII_DP83869_MICR_POL_CHNG_INT_EN	BIT(1)
+#define MII_DP83869_MICR_JABBER_INT_EN		BIT(0)
+
+#define MII_DP83869_BMCR_DEFAULT	(BMCR_ANENABLE | \
+					 BMCR_FULLDPLX | \
+					 BMCR_SPEED1000)
+
+#define MII_DP83869_FIBER_ADVERTISE	(ADVERTISE_CSMA | \
+					 ADVERTISE_1000XHALF | \
+					 ADVERTISE_1000XFULL | \
+					 ADVERTISE_1000XPAUSE | \
+					 ADVERTISE_1000XPSE_ASYM)
+
+/* This is the same bit mask as the BMCR so re-use the BMCR default */
+#define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
+
+/* CFG1 bits */
+#define DP83869_CFG1_DEFAULT	(ADVERTISE_1000HALF | \
+				 ADVERTISE_1000FULL | \
+				 CTL1000_AS_MASTER)
+
+/* RGMIICTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
+#define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
+
+/* STRAP_STS1 bits */
+#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
+#define DP83869_STRAP_STS1_RESERVED		BIT(11)
+#define DP83869_STRAP_MIRROR_ENABLED		BIT(12)
+
+/* PHYCTRL bits */
+#define DP83869_RX_FIFO_SHIFT	12
+#define DP83869_TX_FIFO_SHIFT	14
+
+/* PHY_CTRL lower bytes 0x48 are declared as reserved */
+#define DP83869_PHY_CTRL_DEFAULT	0x48
+#define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
+#define DP83869_PHYCR_RESERVED_MASK	BIT(11)
+
+/* RGMIIDCTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
+
+/* IO_MUX_CFG bits */
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
+
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
+#define DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
+#define DP83869_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
+#define DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
+
+/* CFG3 bits */
+#define DP83869_CFG3_PORT_MIRROR_EN              BIT(0)
+
+/* CFG4 bits */
+#define DP83869_INT_OE	BIT(7)
+
+/* OP MODE */
+#define DP83869_OP_MODE_MII			BIT(5)
+#define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
+
+enum {
+	DP83869_PORT_MIRRORING_KEEP,
+	DP83869_PORT_MIRRORING_EN,
+	DP83869_PORT_MIRRORING_DIS,
+};
+
+struct dp83869_private {
+	int tx_fifo_depth;
+	int rx_fifo_depth;
+	int io_impedance;
+	int port_mirroring;
+	bool rxctrl_strap_quirk;
+	int clk_output_sel;
+	int mode;
+};
+
+static int dp83869_config_port_mirroring(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+
+	if (dp83869->port_mirroring == DP83869_PORT_MIRRORING_EN)
+		return phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+					DP83869_GEN_CFG3,
+					DP83869_CFG3_PORT_MIRROR_EN);
+	else
+		return phy_clear_bits_mmd(phydev, DP83869_DEVADDR,
+					  DP83869_GEN_CFG3,
+					  DP83869_CFG3_PORT_MIRROR_EN);
+}
+
+static int dp83869_set_strapped_mode(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	u16 val;
+
+	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+	if (val < 0)
+		return val;
+
+	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
+
+	return 0;
+}
+
+#ifdef CONFIG_OF_MDIO
+static int dp83869_of_init(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	int ret;
+
+	if (!of_node)
+		return -ENODEV;
+
+	dp83869->io_impedance = -EINVAL;
+
+	/* Optional configuration */
+	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
+				   &dp83869->clk_output_sel);
+	if (ret || dp83869->clk_output_sel > DP83869_CLK_O_SEL_REF_CLK)
+		dp83869->clk_output_sel = DP83869_CLK_O_SEL_REF_CLK;
+
+	ret = of_property_read_u32(of_node, "ti,op-mode", &dp83869->mode);
+	if (ret == 0) {
+		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
+		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
+			return -EINVAL;
+	} else {
+		ret = dp83869_set_strapped_mode(phydev);
+		if (ret)
+			return ret;
+	}
+
+	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
+		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MAX;
+	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
+		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
+
+	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
+		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
+	} else {
+		/* If the lane swap is not in the DT then check the straps */
+		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+		if (ret < 0)
+			return ret;
+		if (ret & DP83869_STRAP_MIRROR_ENABLED)
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
+		else
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
+	}
+
+	if (of_property_read_u32(of_node, "rx-fifo-depth",
+				 &dp83869->rx_fifo_depth))
+		dp83869->rx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
+
+	if (of_property_read_u32(of_node, "tx-fifo-depth",
+				 &dp83869->tx_fifo_depth))
+		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
+
+	return ret;
+}
+#else
+static int dp83869_of_init(struct phy_device *phydev)
+{
+	return dp83869_set_strapped_mode(phydev);
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int dp83869_configure_rgmii(struct phy_device *phydev,
+				   struct dp83869_private *dp83869)
+{
+	int ret = 0, val;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		val = phy_read(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL);
+		if (val < 0)
+			return val;
+
+		val &= ~DP83869_PHYCR_FIFO_DEPTH_MASK;
+		val |= (dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT);
+		val |= (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT);
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL, val);
+		if (ret)
+			return ret;
+	}
+
+	if (dp83869->io_impedance >= 0)
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+				       DP83869_IO_MUX_CFG,
+				       dp83869->io_impedance &
+				       DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
+	return ret;
+}
+
+static int dp83869_config_aneg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int err;
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		err = phy_write(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE,
+				MII_DP83869_FIBER_ADVERTISE);
+		if (err)
+			return err;
+
+		err = phy_write(phydev, MDIO_DEVAD_NONE, DP83869_CTRL,
+				DP83869_SW_RESTART);
+		if (err)
+			return err;
+
+		return genphy_restart_aneg(phydev);
+	} else {
+		return genphy_config_aneg(phydev);
+	}
+}
+
+static int dp83869_configure_mode(struct phy_device *phydev,
+				  struct dp83869_private *dp83869)
+{
+	int phy_ctrl_val;
+	int ret;
+
+	if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
+	    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
+		return -EINVAL;
+
+	/* Below init sequence for each operational mode is defined in
+	 * section 9.4.8 of the datasheet.
+	 */
+	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
+			    dp83869->mode);
+	if (ret)
+		return ret;
+
+	ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, MII_DP83869_BMCR_DEFAULT);
+	if (ret)
+		return ret;
+
+	phy_ctrl_val = (dp83869->rx_fifo_depth << DP83869_RX_FIFO_SHIFT |
+			dp83869->tx_fifo_depth << DP83869_TX_FIFO_SHIFT |
+			DP83869_PHY_CTRL_DEFAULT);
+
+	switch (dp83869->mode) {
+	case DP83869_RGMII_COPPER_ETHERNET:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000,
+				DP83869_CFG1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = dp83869_configure_rgmii(phydev, dp83869);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_RGMII_SGMII_BRIDGE:
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
+				     DP83869_SGMII_RGMII_BRIDGE,
+				     DP83869_SGMII_RGMII_BRIDGE);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+
+		break;
+	case DP83869_1000M_MEDIA_CONVERT:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_100M_MEDIA_CONVERT:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+		break;
+	case DP83869_SGMII_COPPER_ETHERNET:
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83869_PHYCTRL,
+				phy_ctrl_val);
+		if (ret)
+			return ret;
+
+		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, DP83869_CFG1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR,
+				    DP83869_FX_CTRL, DP83869_FX_CTRL_DEFAULT);
+		if (ret)
+			return ret;
+
+		break;
+	case DP83869_RGMII_100_BASE:
+	case DP83869_RGMII_1000_BASE:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int dp83869_phy_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write(phydev, MDIO_DEVAD_NONE,DP83869_CTRL, DP83869_SW_RESET);
+	if (ret < 0)
+		return ret;
+
+	udelay(200);
+
+	return 0;
+}
+
+static int dp83869_config_init(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int ret;
+
+	ret = dp83869_phy_reset(phydev);
+	if (ret)
+		return ret;
+
+	ret = dp83869_configure_mode(phydev, dp83869);
+	if (ret)
+		return ret;
+
+	if (dp83869->port_mirroring != DP83869_PORT_MIRRORING_KEEP)
+		dp83869_config_port_mirroring(phydev);
+
+	/* Clock output selection if muxing property is set */
+	if (dp83869->clk_output_sel != DP83869_CLK_O_SEL_REF_CLK)
+		ret = phy_set_bits_mmd(phydev, DP83869_DEVADDR,
+				       DP83869_IO_MUX_CFG,
+				       dp83869->clk_output_sel <<
+				       DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
+
+	dp83869_config_aneg(phydev);
+
+	return ret;
+}
+
+static int dp83869_probe(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869;
+	int ret;
+
+	dp83869 = kzalloc(sizeof(*dp83869), GFP_KERNEL);
+	if (!dp83869)
+		return -ENOMEM;
+
+	phydev->priv = dp83869;
+
+	ret = dp83869_of_init(phydev);
+	if (ret)
+		return ret;
+
+	return dp83869_config_init(phydev);
+}
+
+static struct phy_driver dp83869_driver = {
+	.name = "TI DP83869",
+	.uid = DP83869_PHY_ID,
+	.mask = 0xfffffff0,
+	.features = PHY_GBIT_FEATURES,
+	.probe = dp83869_probe,
+	.config = &dp83869_config_init,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
+int phy_dp83869_init(void)
+{
+	phy_register(&dp83869_driver);
+	return 0;
+}
diff --git a/drivers/net/phy/ti_phy_init.c b/drivers/net/phy/ti_phy_init.c
index 11c4c166b2f5..cb261b86b05d 100644
--- a/drivers/net/phy/ti_phy_init.c
+++ b/drivers/net/phy/ti_phy_init.c
@@ -92,6 +92,10 @@  int phy_ti_init(void)
 	phy_dp83867_init();
 #endif
 
+#ifdef CONFIG_PHY_DP83869
+	phy_dp83869_init();
+#endif
+
 #ifdef CONFIG_PHY_TI_GENERIC
 	phy_register(&dp83822_driver);
 	phy_register(&dp83825s_driver);
diff --git a/drivers/net/phy/ti_phy_init.h b/drivers/net/phy/ti_phy_init.h
index 309da2aacccb..f94ff6291a3f 100644
--- a/drivers/net/phy/ti_phy_init.h
+++ b/drivers/net/phy/ti_phy_init.h
@@ -12,5 +12,6 @@ 
 #define _TI_GEN_PHY_H
 
 int phy_dp83867_init(void);
+int phy_dp83869_init(void);
 
 #endif /* _TI_GEN_PHY_H */
diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
new file mode 100644
index 000000000000..218b1a64e975
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83869.h
@@ -0,0 +1,42 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Device Tree constants for the Texas Instruments DP83869 PHY
+ *
+ * Author: Dan Murphy <dmurphy at ti.com>
+ *
+ * Copyright:   (C) 2019 Texas Instruments, Inc.
+ */
+
+#ifndef _DT_BINDINGS_TI_DP83869_H
+#define _DT_BINDINGS_TI_DP83869_H
+
+/* PHY CTRL bits */
+#define DP83869_PHYCR_FIFO_DEPTH_3_B_NIB	0x00
+#define DP83869_PHYCR_FIFO_DEPTH_4_B_NIB	0x01
+#define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
+#define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
+
+/* IO_MUX_CFG - Clock output selection */
+#define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
+#define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
+#define DP83869_CLK_O_SEL_CHN_C_RCLK		0x2
+#define DP83869_CLK_O_SEL_CHN_D_RCLK		0x3
+#define DP83869_CLK_O_SEL_CHN_A_RCLK_DIV5	0x4
+#define DP83869_CLK_O_SEL_CHN_B_RCLK_DIV5	0x5
+#define DP83869_CLK_O_SEL_CHN_C_RCLK_DIV5	0x6
+#define DP83869_CLK_O_SEL_CHN_D_RCLK_DIV5	0x7
+#define DP83869_CLK_O_SEL_CHN_A_TCLK		0x8
+#define DP83869_CLK_O_SEL_CHN_B_TCLK		0x9
+#define DP83869_CLK_O_SEL_CHN_C_TCLK		0xa
+#define DP83869_CLK_O_SEL_CHN_D_TCLK		0xb
+#define DP83869_CLK_O_SEL_REF_CLK		0xc
+
+#define DP83869_RGMII_COPPER_ETHERNET		0x00
+#define DP83869_RGMII_1000_BASE			0x01
+#define DP83869_RGMII_100_BASE			0x02
+#define DP83869_RGMII_SGMII_BRIDGE		0x03
+#define DP83869_1000M_MEDIA_CONVERT		0x04
+#define DP83869_100M_MEDIA_CONVERT		0x05
+#define DP83869_SGMII_COPPER_ETHERNET		0x06
+
+#endif