[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver
Dan Murphy
dmurphy at ti.com
Tue Apr 21 14:04:15 CEST 2020
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
More information about the U-Boot
mailing list