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

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.

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@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