[RFC PATCH 3/3] net: phy: dp83869: Port the kernel driver
Michal Simek
michal.simek at xilinx.com
Tue Apr 21 14:39:44 CEST 2020
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. :-)
>>
>>
>>> +
>>> 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.GA2727094@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
More information about the U-Boot
mailing list