On 20. 04. 20 20:53, Dan Murphy wrote: > Port the DP83869 kernel driver. > > Signed-off-by: Dan Murphy <dmur...@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 <dmur...@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