netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] net: phy: dp83867: Add TI dp83867 phy
@ 2015-06-02 14:34 Dan Murphy
  2015-06-04  2:41 ` David Miller
  2015-06-04  2:47 ` Florian Fainelli
  0 siblings, 2 replies; 5+ messages in thread
From: Dan Murphy @ 2015-06-02 14:34 UTC (permalink / raw)
  To: netdev; +Cc: f.fainelli, Dan Murphy

Add support for the TI dp83867 Gigabit ethernet phy
device.

The DP83867 is a robust, low power, fully featured
Physical Layer transceiver with integrated PMD
sublayers to support 10BASE-T, 100BASE-TX and
1000BASE-T Ethernet protocols.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---

v2 - Add device tree support and other minor changes.  Added change for rgmii helper
http://patchwork.ozlabs.org/patch/476446/

 .../devicetree/bindings/net/ti,dp83867.txt         |  19 ++
 drivers/net/phy/Kconfig                            |   6 +-
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/dp83867.c                          | 239 +++++++++++++++++++++
 include/dt-bindings/net/ti-dp83867.h               |  45 ++++
 5 files changed, 309 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83867.txt
 create mode 100644 drivers/net/phy/dp83867.c
 create mode 100644 include/dt-bindings/net/ti-dp83867.h

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt b/Documentation/devicetree/bindings/net/ti,dp83867.txt
new file mode 100644
index 0000000..46bb67a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
@@ -0,0 +1,19 @@
+* Texas Instruments - dp83867 Giga bit ethernet phy
+
+Required properties:
+	- reg - The ID number for the phy, usually a small integer
+	- ti,rx_int_delay - RGMII Recieve Clock Delay - see dt-bindings/net/ti-dp83867.h
+		for applicable values
+	- ti,tx_int_delay - RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83867.h
+		for applicable values
+	- ti,fifo_depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
+		for applicable values
+
+Example:
+
+	ethernet-phy@0 {
+		reg = <0>;
+		ti,rx_int_delay = <DP83867_RGMIIDCTL_2_25_NS>;
+		ti,tx_int_delay = <DP83867_RGMIIDCTL_2_75_NS>;
+		ti,fifo_depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+	};
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 70641d2..939f06e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -119,6 +119,11 @@ config MICREL_PHY
 	---help---
 	  Supports the KSZ9021, VSC8201, KS8001 PHYs.
 
+config DP83867_PHY
+	tristate "Drivers for Texas Instruments DP83867 Gigabit PHY"
+	---help---
+	  Currently supports the DP83867 PHY.
+
 config FIXED_PHY
 	tristate "Driver for MDIO Bus/PHY emulation with fixed speed/link PHYs"
 	depends on PHYLIB
@@ -212,7 +217,6 @@ config MDIO_BCM_UNIMAC
 	  This hardware can be found in the Broadcom GENET Ethernet MAC
 	  controllers as well as some Broadcom Ethernet switches such as the
 	  Starfighter 2 switches.
-
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 501ea76..58e6131 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_DP83640_PHY)	+= dp83640.o
+obj-$(CONFIG_DP83867_PHY)	+= dp83867.o
 obj-$(CONFIG_STE10XP)		+= ste10Xp.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
new file mode 100644
index 0000000..ef0b4eb
--- /dev/null
+++ b/drivers/net/phy/dp83867.c
@@ -0,0 +1,239 @@
+/*
+ * Driver for the Texas Instruments DP83867 PHY
+ *
+ * Copyright (C) 2015 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+
+#include <dt-bindings/net/ti-dp83867.h>
+
+#define DP83867_PHY_ID		0x2000a231
+#define DP83867_DEVADDR		0x1f
+
+#define MII_DP83867_PHYCTRL	0x10
+#define MII_DP83867_MICR	0x12
+#define MII_DP83867_ISR		0x13
+#define DP83867_CTRL		0x1f
+
+/* Extended Registers */
+#define DP83867_RGMIICTL	0x0032
+#define DP83867_RGMIIDCTL	0x0086
+
+#define DP83867_SW_RESET	BIT(15)
+#define DP83867_SW_RESTART	BIT(14)
+
+/* MICR Interrupt bits */
+#define MII_DP83867_MICR_AN_ERR_INT_EN		BIT(15)
+#define MII_DP83867_MICR_SPEED_CHNG_INT_EN	BIT(14)
+#define MII_DP83867_MICR_DUP_MODE_CHNG_INT_EN	BIT(13)
+#define MII_DP83867_MICR_PAGE_RXD_INT_EN	BIT(12)
+#define MII_DP83867_MICR_AUTONEG_COMP_INT_EN	BIT(11)
+#define MII_DP83867_MICR_LINK_STS_CHNG_INT_EN	BIT(10)
+#define MII_DP83867_MICR_FALSE_CARRIER_INT_EN	BIT(8)
+#define MII_DP83867_MICR_SLEEP_MODE_CHNG_INT_EN	BIT(4)
+#define MII_DP83867_MICR_WOL_INT_EN		BIT(3)
+#define MII_DP83867_MICR_XGMII_ERR_INT_EN	BIT(2)
+#define MII_DP83867_MICR_POL_CHNG_INT_EN	BIT(1)
+#define MII_DP83867_MICR_JABBER_INT_EN		BIT(0)
+
+/* RGMIICTL bits */
+#define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
+#define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
+
+/* PHY CTRL bits */
+#define DP83867_PHYCR_FIFO_DEPTH_SHIFT		14
+
+/* RGMIIDCTL bits */
+#define DP83867_RGMII_TX_CLK_DELAY_SHIFT	4
+
+struct dp83867_private {
+	int rx_id_delay;
+	int tx_id_delay;
+	int fifo_depth;
+};
+
+static int dp83867_ack_interrupt(struct phy_device *phydev)
+{
+	int err = phy_read(phydev, MII_DP83867_ISR);
+
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int dp83867_config_intr(struct phy_device *phydev)
+{
+	int micr_status;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		micr_status = phy_read(phydev, MII_DP83867_MICR);
+		if (micr_status < 0)
+			return micr_status;
+
+		micr_status |=
+			(MII_DP83867_MICR_AN_ERR_INT_EN |
+			MII_DP83867_MICR_SPEED_CHNG_INT_EN |
+			MII_DP83867_MICR_DUP_MODE_CHNG_INT_EN |
+			MII_DP83867_MICR_SLEEP_MODE_CHNG_INT_EN);
+
+		return phy_write(phydev, MII_DP83867_MICR, micr_status);
+	}
+
+	micr_status = 0x0;
+	return phy_write(phydev, MII_DP83867_MICR, micr_status);
+}
+
+#ifdef CONFIG_OF_MDIO
+static int dp83867_of_init(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867 = phydev->priv;
+	struct device *dev = &phydev->dev;
+	struct device_node *of_node = dev->of_node;
+	int ret;
+
+	if (!of_node && dev->parent->of_node)
+		of_node = dev->parent->of_node;
+
+	if (!phydev->dev.of_node)
+		return -ENODEV;
+
+	ret = of_property_read_u32(of_node, "ti,rx_int_delay",
+				   &dp83867->rx_id_delay);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(of_node, "ti,tx_int_delay",
+				   &dp83867->tx_id_delay);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(of_node, "ti,fifo_depth",
+				   &dp83867->fifo_depth);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+#else
+static int dp83867_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int dp83867_config_init(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867;
+	int ret;
+	u16 val, delay;
+
+	if (!phydev->priv) {
+		dp83867 = devm_kzalloc(&phydev->dev, sizeof(*dp83867),
+				       GFP_KERNEL);
+		if (!dp83867)
+			return -ENOMEM;
+
+		phydev->priv = dp83867;
+		ret = dp83867_of_init(phydev);
+		if (ret)
+			return ret;
+	} else {
+		dp83867 = (struct dp83867_private *)phydev->priv;
+	}
+
+	if (phy_interface_is_rgmii(phydev)) {
+		ret = phy_write(phydev, MII_DP83867_PHYCTRL,
+			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
+		if (ret)
+			return ret;
+	}
+
+	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) ||
+	    (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
+		val = phy_read_mmd_indirect(phydev, DP83867_RGMIICTL,
+					    DP83867_DEVADDR, phydev->addr);
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+			val |= (DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN);
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			val |= DP83867_RGMII_TX_CLK_DELAY_EN;
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			val |= DP83867_RGMII_RX_CLK_DELAY_EN;
+
+		phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
+				       DP83867_DEVADDR, phydev->addr, val);
+
+		delay = (dp83867->rx_id_delay |
+			(dp83867->tx_id_delay << DP83867_RGMII_TX_CLK_DELAY_SHIFT));
+
+		phy_write_mmd_indirect(phydev, DP83867_RGMIIDCTL,
+				       DP83867_DEVADDR, phydev->addr, delay);
+	}
+
+	return 0;
+}
+
+static int dp83867_phy_reset(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
+	if (err < 0)
+		return err;
+
+	return dp83867_config_init(phydev);
+}
+
+static struct phy_driver dp83867_driver[] = {
+	{
+		.phy_id		= DP83867_PHY_ID,
+		.phy_id_mask	= 0xfffffff0,
+		.name		= "TI DP83867",
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+
+		.config_init	= dp83867_config_init,
+		.soft_reset	= dp83867_phy_reset,
+
+		/* IRQ related */
+		.ack_interrupt	= dp83867_ack_interrupt,
+		.config_intr	= dp83867_config_intr,
+
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+
+		.driver		= {.owner = THIS_MODULE,}
+	},
+};
+module_phy_driver(dp83867_driver);
+
+static struct mdio_device_id __maybe_unused dp83867_tbl[] = {
+	{ DP83867_PHY_ID, 0xfffffff0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, dp83867_tbl);
+
+MODULE_DESCRIPTION("Texas Instruments DP83867 PHY driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com");
+MODULE_LICENSE("GPL");
diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h
new file mode 100644
index 0000000..172744a
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83867.h
@@ -0,0 +1,45 @@
+/*
+ * Device Tree constants for the Texas Instruments DP83867 PHY
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * Copyright:   (C) 2015 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef _DT_BINDINGS_TI_DP83867_H
+#define _DT_BINDINGS_TI_DP83867_H
+
+/* PHY CTRL bits */
+#define DP83867_PHYCR_FIFO_DEPTH_3_B_NIB	0x00
+#define DP83867_PHYCR_FIFO_DEPTH_4_B_NIB	0x01
+#define DP83867_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
+#define DP83867_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
+
+/* RGMIIDCTL internal delay for rx and tx */
+#define	DP83867_RGMIIDCTL_250_PS	0x0
+#define	DP83867_RGMIIDCTL_500_PS	0x1
+#define	DP83867_RGMIIDCTL_750_PS	0x2
+#define	DP83867_RGMIIDCTL_1_NS		0x3
+#define	DP83867_RGMIIDCTL_1_25_NS	0x4
+#define	DP83867_RGMIIDCTL_1_50_NS	0x5
+#define	DP83867_RGMIIDCTL_1_75_NS	0x6
+#define	DP83867_RGMIIDCTL_2_00_NS	0x7
+#define	DP83867_RGMIIDCTL_2_25_NS	0x8
+#define	DP83867_RGMIIDCTL_2_50_NS	0x9
+#define	DP83867_RGMIIDCTL_2_75_NS	0xa
+#define	DP83867_RGMIIDCTL_3_00_NS	0xb
+#define	DP83867_RGMIIDCTL_3_25_NS	0xc
+#define	DP83867_RGMIIDCTL_3_50_NS	0xd
+#define	DP83867_RGMIIDCTL_3_75_NS	0xe
+#define	DP83867_RGMIIDCTL_4_00_NS	0xf
+
+#endif
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy
  2015-06-02 14:34 [PATCH v2] net: phy: dp83867: Add TI dp83867 phy Dan Murphy
@ 2015-06-04  2:41 ` David Miller
  2015-06-04  2:47 ` Florian Fainelli
  1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2015-06-04  2:41 UTC (permalink / raw)
  To: dmurphy; +Cc: netdev, f.fainelli

From: Dan Murphy <dmurphy@ti.com>
Date: Tue, 2 Jun 2015 09:34:37 -0500

> Add support for the TI dp83867 Gigabit ethernet phy
> device.
> 
> The DP83867 is a robust, low power, fully featured
> Physical Layer transceiver with integrated PMD
> sublayers to support 10BASE-T, 100BASE-TX and
> 1000BASE-T Ethernet protocols.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Applied to net-next, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy
  2015-06-02 14:34 [PATCH v2] net: phy: dp83867: Add TI dp83867 phy Dan Murphy
  2015-06-04  2:41 ` David Miller
@ 2015-06-04  2:47 ` Florian Fainelli
  2015-06-08 14:04   ` Dan Murphy
  1 sibling, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2015-06-04  2:47 UTC (permalink / raw)
  To: Dan Murphy, netdev, David Miller

Le 06/02/15 07:34, Dan Murphy a écrit :
> Add support for the TI dp83867 Gigabit ethernet phy
> device.
> 
> The DP83867 is a robust, low power, fully featured
> Physical Layer transceiver with integrated PMD
> sublayers to support 10BASE-T, 100BASE-TX and
> 1000BASE-T Ethernet protocols.

Sorry for the late feedback, since this is a new driver, things outline
below can still be submitted as incremental fixes.

[snip]

> +Required properties:
> +	- reg - The ID number for the phy, usually a small integer
> +	- ti,rx_int_delay - RGMII Recieve Clock Delay - see dt-bindings/net/ti-dp83867.h
> +		for applicable values
> +	- ti,tx_int_delay - RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83867.h
> +		for applicable values
> +	- ti,fifo_depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
> +		for applicable values

We typically use "-" to separate words in DT properties, not "_". I am
not sure about the required nature of these 3 proprietary/specific
properties, cannot there be good reset defaults from the HW in any case?

You might want to add a reference to net/phy.txt for the remaiming DT
properties.

[snip]

> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		ret = phy_write(phydev, MII_DP83867_PHYCTRL,
> +			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) ||
> +	    (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {

This one has not been converted to use the phy_interface_is_rgmii()
helper, but in fact, once you do that, you could probably just add an
early check for this condition, return, and reduce the indentation for
the normal case/RGMII, and eliminate two redundant condition checks.

Thanks!
-- 
Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy
  2015-06-04  2:47 ` Florian Fainelli
@ 2015-06-08 14:04   ` Dan Murphy
  2015-06-08 18:03     ` Florian Fainelli
  0 siblings, 1 reply; 5+ messages in thread
From: Dan Murphy @ 2015-06-08 14:04 UTC (permalink / raw)
  To: Florian Fainelli, netdev, David Miller

Florian

Thanks for the re-review

On 06/03/2015 09:47 PM, Florian Fainelli wrote:
> Le 06/02/15 07:34, Dan Murphy a écrit :
>> Add support for the TI dp83867 Gigabit ethernet phy
>> device.
>>
>> The DP83867 is a robust, low power, fully featured
>> Physical Layer transceiver with integrated PMD
>> sublayers to support 10BASE-T, 100BASE-TX and
>> 1000BASE-T Ethernet protocols.
> Sorry for the late feedback, since this is a new driver, things outline
> below can still be submitted as incremental fixes.
>
> [snip]
>
>> +Required properties:
>> +	- reg - The ID number for the phy, usually a small integer
>> +	- ti,rx_int_delay - RGMII Recieve Clock Delay - see dt-bindings/net/ti-dp83867.h
>> +		for applicable values
>> +	- ti,tx_int_delay - RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83867.h
>> +		for applicable values
>> +	- ti,fifo_depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
>> +		for applicable values
> We typically use "-" to separate words in DT properties, not "_". I am
> not sure about the required nature of these 3 proprietary/specific
> properties, cannot there be good reset defaults from the HW in any case?

Yes you are correct I will modify as an incremental fix.

The hardware is defaulted to an internal delay of 2 ns.  This value needs to be adjusted
per product based on trace length.  I would anticipate the DT properties to grow a little as the
part gets used more.

> You might want to add a reference to net/phy.txt for the remaiming DT
> properties.
>
> [snip]
>
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		ret = phy_write(phydev, MII_DP83867_PHYCTRL,
>> +			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) ||
>> +	    (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
> This one has not been converted to use the phy_interface_is_rgmii()
> helper, but in fact, once you do that, you could probably just add an
> early check for this condition, return, and reduce the indentation for
> the normal case/RGMII, and eliminate two redundant condition checks.

This check is different then the generic RGMII check.  I am checking to see if the interface has the internal
delay flag set.  The RGMII check is to broad.  Maybe I can add a helper for this
if it makes sense thoughts?  Maybe a phy_interface_has_rgmii_int_delay API just to enter this check.

Dan

> Thanks!


-- 
------------------
Dan Murphy

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: dp83867: Add TI dp83867 phy
  2015-06-08 14:04   ` Dan Murphy
@ 2015-06-08 18:03     ` Florian Fainelli
  0 siblings, 0 replies; 5+ messages in thread
From: Florian Fainelli @ 2015-06-08 18:03 UTC (permalink / raw)
  To: Dan Murphy, netdev, David Miller

On 08/06/15 07:04, Dan Murphy wrote:
> Florian
> 
> Thanks for the re-review
> 
> On 06/03/2015 09:47 PM, Florian Fainelli wrote:
>> Le 06/02/15 07:34, Dan Murphy a écrit :
>>> Add support for the TI dp83867 Gigabit ethernet phy
>>> device.
>>>
>>> The DP83867 is a robust, low power, fully featured
>>> Physical Layer transceiver with integrated PMD
>>> sublayers to support 10BASE-T, 100BASE-TX and
>>> 1000BASE-T Ethernet protocols.
>> Sorry for the late feedback, since this is a new driver, things outline
>> below can still be submitted as incremental fixes.
>>
>> [snip]
>>
>>> +Required properties:
>>> +	- reg - The ID number for the phy, usually a small integer
>>> +	- ti,rx_int_delay - RGMII Recieve Clock Delay - see dt-bindings/net/ti-dp83867.h
>>> +		for applicable values
>>> +	- ti,tx_int_delay - RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83867.h
>>> +		for applicable values
>>> +	- ti,fifo_depth - Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h
>>> +		for applicable values
>> We typically use "-" to separate words in DT properties, not "_". I am
>> not sure about the required nature of these 3 proprietary/specific
>> properties, cannot there be good reset defaults from the HW in any case?
> 
> Yes you are correct I will modify as an incremental fix.
> 
> The hardware is defaulted to an internal delay of 2 ns.  This value needs to be adjusted
> per product based on trace length.  I would anticipate the DT properties to grow a little as the
> part gets used more.

Ok, that makes sense then.

> 
>> You might want to add a reference to net/phy.txt for the remaiming DT
>> properties.
>>
>> [snip]
>>
>>> +
>>> +	if (phy_interface_is_rgmii(phydev)) {
>>> +		ret = phy_write(phydev, MII_DP83867_PHYCTRL,
>>> +			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) ||
>>> +	    (phydev->interface <= PHY_INTERFACE_MODE_RGMII_RXID)) {
>> This one has not been converted to use the phy_interface_is_rgmii()
>> helper, but in fact, once you do that, you could probably just add an
>> early check for this condition, return, and reduce the indentation for
>> the normal case/RGMII, and eliminate two redundant condition checks.
> 
> This check is different then the generic RGMII check.  I am checking to see if the interface has the internal
> delay flag set.  The RGMII check is to broad.  Maybe I can add a helper for this
> if it makes sense thoughts?  Maybe a phy_interface_has_rgmii_int_delay API just to enter this check.

Oh, I misread the range check. I do not think this deserves a helper
function for now, since that seems to be pretty unique and localized to
your driver for now, but if we have more than one users, factoring
things out would definitively make sense then.

Thanks.
-- 
Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-06-08 18:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-02 14:34 [PATCH v2] net: phy: dp83867: Add TI dp83867 phy Dan Murphy
2015-06-04  2:41 ` David Miller
2015-06-04  2:47 ` Florian Fainelli
2015-06-08 14:04   ` Dan Murphy
2015-06-08 18:03     ` Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).