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