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



Reply via email to