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