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 <dmur...@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 <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. :-)
>
> 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.ga2727...@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
