[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Dan Murphy]

Not all devices use the same internal delay or fifo depth.
Add the ability to set the internal delay for rx or tx and the
fifo depth via the config file.  If the value is not set in the
config file then set the delay to the default.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
 include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++
 2 files changed, 98 insertions(+), 8 deletions(-)
 create mode 100644 include/dt-bindings/net/ti-dp83867.h

diff --git a/drivers/net/phy/ti.c b/drivers/net/phy/ti.c
index c3912d5..6da0523 100644
--- a/drivers/net/phy/ti.c
+++ b/drivers/net/phy/ti.c
@@ -6,6 +6,9 @@
  */
 #include <common.h>
 #include <phy.h>
+#include <linux/compat.h>
+
+#include <dt-bindings/net/ti-dp83867.h>
 
 /* TI DP83867 */
 #define DP83867_DEVADDR		0x1f
@@ -57,6 +60,17 @@
 #define MII_MMD_CTRL_INCR_RDWT	0x8000 /* post increment on reads & writes */
 #define MII_MMD_CTRL_INCR_ON_WT	0xC000 /* post increment on writes only */
 
+/* User setting - can be taken from DTS */
+#define DEFAULT_RX_ID_DELAY	8
+#define DEFAULT_TX_ID_DELAY	0xa
+#define DEFAULT_FIFO_DEPTH	1
+
+struct dp83867_private {
+	int rx_id_delay;
+	int tx_id_delay;
+	int fifo_depth;
+};
+
 /**
  * phy_read_mmd_indirect - reads data from the MMD registers
  * @phydev: The PHY device bus
@@ -134,16 +148,53 @@ static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
 		phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
 }
 
-/* User setting - can be taken from DTS */
-#define RX_ID_DELAY	8
-#define TX_ID_DELAY	0xa
-#define FIFO_DEPTH	1
+/**
+ * dp83867_data_init - Convenience function for setting PHY specific data
+ * @phydev: the phy_device struct
+ */
+static int dp83867_data_init(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867 = phydev->priv;
+
+#ifdef CONFIG_RGMII_RX_ID
+	dp83867->rx_id_delay = CONFIG_RGMII_RX_ID;
+#else
+	dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY;
+#endif
+
+#ifdef CONFIG_RGMII_TX_ID
+	dp83867->tx_id_delay = CONFIG_RGMII_TX_ID;
+#else
+	dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY;
+#endif
+
+#ifdef CONFIG_FIFO_DEPTH
+	dp83867->fifo_depth = CONFIG_FIFO_DEPTH;
+#else
+	dp83867->fifo_depth = DEFAULT_FIFO_DEPTH;
+#endif
+	return 0;
+}
 
 static int dp83867_config(struct phy_device *phydev)
 {
+	struct dp83867_private *dp83867;
 	unsigned int val, delay;
 	int ret;
 
+	if (!phydev->priv) {
+		dp83867 = kzalloc(sizeof(*dp83867), GFP_KERNEL);
+		if (!dp83867)
+			return -ENOMEM;
+
+		phydev->priv = dp83867;
+		ret = dp83867_data_init(phydev);
+		if (ret)
+			goto dp83867_write_error;
+	} else {
+		dp83867 = (struct dp83867_private *)phydev->priv;
+	}
+
 	/* Restart the PHY.  */
 	val = phy_read(phydev, MDIO_DEVAD_NONE, DP83867_CTRL);
 	phy_write(phydev, MDIO_DEVAD_NONE, DP83867_CTRL,
@@ -152,9 +203,9 @@ static int dp83867_config(struct phy_device *phydev)
 	if (phy_interface_is_rgmii(phydev)) {
 		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
 			(DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) |
-			(FIFO_DEPTH << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
+			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
 		if (ret)
-			return ret;
+			goto dp83867_write_error;
 	}
 
 	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) &&
@@ -175,8 +226,8 @@ static int dp83867_config(struct phy_device *phydev)
 		phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
 				       DP83867_DEVADDR, phydev->addr, val);
 
-		delay = (RX_ID_DELAY |
-			 (TX_ID_DELAY << DP83867_RGMII_TX_CLK_DELAY_SHIFT));
+		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);
@@ -184,6 +235,10 @@ static int dp83867_config(struct phy_device *phydev)
 
 	genphy_config_aneg(phydev);
 	return 0;
+
+dp83867_write_error:
+	free(dp83867);
+	return ret;
 }
 
 static struct phy_driver DP83867_driver = {
diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h
new file mode 100644
index 0000000..5c592fb
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83867.h
@@ -0,0 +1,35 @@
+/*
+ * TI DP83867 PHY drivers
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ *
+ */
+
+#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
-- 
2.8.0.rc3

[U-Boot] [RFC PATCH 2/2] board: ti: DRA7: Add in RGMII parameters for rev c

[Dan Murphy]

Add in the rx/tx delay as well as the fifo depth
for the dra7 evm rev c board.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 include/configs/dra7xx_evm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/configs/dra7xx_evm.h b/include/configs/dra7xx_evm.h
index 7734e8d..0b20b6d 100644
--- a/include/configs/dra7xx_evm.h
+++ b/include/configs/dra7xx_evm.h
@@ -157,6 +157,10 @@
 #define CONFIG_PHYLIB
 #define CONFIG_PHY_TI
 
+#define CONFIG_RGMII_RX_ID	DP83867_RGMIIDCTL_2_00_NS
+#define CONFIG_RGMII_TX_ID	DP83867_RGMIIDCTL_1_NS
+#define CONFIG_FIFO_DEPTH	DP83867_PHYCR_FIFO_DEPTH_8_B_NIB
+
 /* SPI */
 #undef	CONFIG_OMAP3_SPI
 #define CONFIG_CMD_SF
-- 
2.8.0.rc3

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Dan Murphy]

On 03/31/2016 07:42 AM, Dan Murphy wrote:
> Not all devices use the same internal delay or fifo depth.
> Add the ability to set the internal delay for rx or tx and the
> fifo depth via the config file.  If the value is not set in the
> config file then set the delay to the default.

This patch also aligns the uboot driver with the kernel driver.
And when DTS data is added the data_init api can be used
to populate the DTS data to the data structure.

>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
>  include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++
>  2 files changed, 98 insertions(+), 8 deletions(-)
>  create mode 100644 include/dt-bindings/net/ti-dp83867.h
>
> diff --git a/drivers/net/phy/ti.c b/drivers/net/phy/ti.c
> index c3912d5..6da0523 100644
> --- a/drivers/net/phy/ti.c
> +++ b/drivers/net/phy/ti.c
> @@ -6,6 +6,9 @@
>   */
>  #include <common.h>
>  #include <phy.h>
> +#include <linux/compat.h>
> +
> +#include <dt-bindings/net/ti-dp83867.h>
>  
>  /* TI DP83867 */
>  #define DP83867_DEVADDR		0x1f
> @@ -57,6 +60,17 @@
>  #define MII_MMD_CTRL_INCR_RDWT	0x8000 /* post increment on reads & writes */
>  #define MII_MMD_CTRL_INCR_ON_WT	0xC000 /* post increment on writes only */
>  
> +/* User setting - can be taken from DTS */
> +#define DEFAULT_RX_ID_DELAY	8
> +#define DEFAULT_TX_ID_DELAY	0xa
> +#define DEFAULT_FIFO_DEPTH	1
> +
> +struct dp83867_private {
> +	int rx_id_delay;
> +	int tx_id_delay;
> +	int fifo_depth;
> +};
> +
>  /**
>   * phy_read_mmd_indirect - reads data from the MMD registers
>   * @phydev: The PHY device bus
> @@ -134,16 +148,53 @@ static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
>  		phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
>  }
>  
> -/* User setting - can be taken from DTS */
> -#define RX_ID_DELAY	8
> -#define TX_ID_DELAY	0xa
> -#define FIFO_DEPTH	1

Probably should use the #defines here instead of magic numbers.

> +/**
> + * dp83867_data_init - Convenience function for setting PHY specific data
> + * @phydev: the phy_device struct
> + */
> +static int dp83867_data_init(struct phy_device *phydev)
> +{
> +	struct dp83867_private *dp83867 = phydev->priv;
> +
> +#ifdef CONFIG_RGMII_RX_ID
> +	dp83867->rx_id_delay = CONFIG_RGMII_RX_ID;
> +#else
> +	dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY;
> +#endif
> +
> +#ifdef CONFIG_RGMII_TX_ID
> +	dp83867->tx_id_delay = CONFIG_RGMII_TX_ID;
> +#else
> +	dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY;
> +#endif
> +
> +#ifdef CONFIG_FIFO_DEPTH
> +	dp83867->fifo_depth = CONFIG_FIFO_DEPTH;
> +#else
> +	dp83867->fifo_depth = DEFAULT_FIFO_DEPTH;
> +#endif
> +	return 0;
> +}
>  
>  static int dp83867_config(struct phy_device *phydev)
>  {
> +	struct dp83867_private *dp83867;
>  	unsigned int val, delay;
>  	int ret;
>  
> +	if (!phydev->priv) {
> +		dp83867 = kzalloc(sizeof(*dp83867), GFP_KERNEL);
> +		if (!dp83867)
> +			return -ENOMEM;
> +
> +		phydev->priv = dp83867;
> +		ret = dp83867_data_init(phydev);
> +		if (ret)
> +			goto dp83867_write_error;
> +	} else {
> +		dp83867 = (struct dp83867_private *)phydev->priv;
> +	}
> +
>  	/* Restart the PHY.  */
>  	val = phy_read(phydev, MDIO_DEVAD_NONE, DP83867_CTRL);
>  	phy_write(phydev, MDIO_DEVAD_NONE, DP83867_CTRL,
> @@ -152,9 +203,9 @@ static int dp83867_config(struct phy_device *phydev)
>  	if (phy_interface_is_rgmii(phydev)) {
>  		ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL,
>  			(DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER) |
> -			(FIFO_DEPTH << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
> +			(dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT));
>  		if (ret)
> -			return ret;
> +			goto dp83867_write_error;
>  	}
>  
>  	if ((phydev->interface >= PHY_INTERFACE_MODE_RGMII_ID) &&
> @@ -175,8 +226,8 @@ static int dp83867_config(struct phy_device *phydev)
>  		phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
>  				       DP83867_DEVADDR, phydev->addr, val);
>  
> -		delay = (RX_ID_DELAY |
> -			 (TX_ID_DELAY << DP83867_RGMII_TX_CLK_DELAY_SHIFT));
> +		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);
> @@ -184,6 +235,10 @@ static int dp83867_config(struct phy_device *phydev)
>  
>  	genphy_config_aneg(phydev);
>  	return 0;
> +
> +dp83867_write_error:
> +	free(dp83867);
> +	return ret;
>  }
>  
>  static struct phy_driver DP83867_driver = {
> diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h
> new file mode 100644
> index 0000000..5c592fb
> --- /dev/null
> +++ b/include/dt-bindings/net/ti-dp83867.h
> @@ -0,0 +1,35 @@
> +/*
> + * TI DP83867 PHY drivers
> + *
> + * SPDX-License-Identifier:	GPL-2.0
> + *
> + */
> +
> +#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


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

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Tom Rini]

On Thu, Mar 31, 2016 at 07:42:39AM -0500, Dan Murphy wrote:

> Not all devices use the same internal delay or fifo depth.
> Add the ability to set the internal delay for rx or tx and the
> fifo depth via the config file.  If the value is not set in the
> config file then set the delay to the default.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
>  include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++

I don't think this is taking things down the right path.  If it's a DT
binding, it comes from the device tree (which is fine and good!) but
that means the binding needs to meet the usual reviews and not just come
in via U-Boot like this.  We really don't want to add a DT binding that
gets values from the config.h file.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160331/09e84263/attachment.sig>

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Dan Murphy]

Tom

On 03/31/2016 09:11 AM, Tom Rini wrote:
> On Thu, Mar 31, 2016 at 07:42:39AM -0500, Dan Murphy wrote:
>
>> Not all devices use the same internal delay or fifo depth.
>> Add the ability to set the internal delay for rx or tx and the
>> fifo depth via the config file.  If the value is not set in the
>> config file then set the delay to the default.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>  drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
>>  include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++
> I don't think this is taking things down the right path.  If it's a DT
> binding, it comes from the device tree (which is fine and good!) but
> that means the binding needs to meet the usual reviews and not just come
> in via U-Boot like this.  We really don't want to add a DT binding that
> gets values from the config.h file.
>

This binding file I created that is already part of the Mainline kernel.
I just brought it in to use #defines and once the drivers are ported to use
DT then the bindings will already be available.

And the config.h should be getting its values from the dt-binding.

Dan

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

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Tom Rini]

On Thu, Mar 31, 2016 at 10:27:31AM -0500, Dan Murphy wrote:
> Tom
> 
> On 03/31/2016 09:11 AM, Tom Rini wrote:
> > On Thu, Mar 31, 2016 at 07:42:39AM -0500, Dan Murphy wrote:
> >
> >> Not all devices use the same internal delay or fifo depth.
> >> Add the ability to set the internal delay for rx or tx and the
> >> fifo depth via the config file.  If the value is not set in the
> >> config file then set the delay to the default.
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >> ---
> >>  drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
> >>  include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++
> > I don't think this is taking things down the right path.  If it's a DT
> > binding, it comes from the device tree (which is fine and good!) but
> > that means the binding needs to meet the usual reviews and not just come
> > in via U-Boot like this.  We really don't want to add a DT binding that
> > gets values from the config.h file.
> >
> 
> This binding file I created that is already part of the Mainline kernel.
> I just brought it in to use #defines and once the drivers are ported to use
> DT then the bindings will already be available.
> 
> And the config.h should be getting its values from the dt-binding.

OK.  Then lets bring in the whole binding as its own patch.  And
Mugunthan made cpsw do DM_ETH I see drivers/net/phy/micrel.c talks
DM_ETH and DT so lets get this PHY driver updated and then get this
additional bit from the DT.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160331/9ec41100/attachment.sig>

[U-Boot] [RFC PATCH 1/2] net: phy: ti: Allow the driver to be more configurable

[Dan Murphy]

Tom

On 03/31/2016 10:42 AM, Tom Rini wrote:
> On Thu, Mar 31, 2016 at 10:27:31AM -0500, Dan Murphy wrote:
>> Tom
>>
>> On 03/31/2016 09:11 AM, Tom Rini wrote:
>>> On Thu, Mar 31, 2016 at 07:42:39AM -0500, Dan Murphy wrote:
>>>
>>>> Not all devices use the same internal delay or fifo depth.
>>>> Add the ability to set the internal delay for rx or tx and the
>>>> fifo depth via the config file.  If the value is not set in the
>>>> config file then set the delay to the default.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>  drivers/net/phy/ti.c                 | 71 ++++++++++++++++++++++++++++++++----
>>>>  include/dt-bindings/net/ti-dp83867.h | 35 ++++++++++++++++++
>>> I don't think this is taking things down the right path.  If it's a DT
>>> binding, it comes from the device tree (which is fine and good!) but
>>> that means the binding needs to meet the usual reviews and not just come
>>> in via U-Boot like this.  We really don't want to add a DT binding that
>>> gets values from the config.h file.
>>>
>> This binding file I created that is already part of the Mainline kernel.
>> I just brought it in to use #defines and once the drivers are ported to use
>> DT then the bindings will already be available.
>>
>> And the config.h should be getting its values from the dt-binding.
> OK.  Then lets bring in the whole binding as its own patch.  And
> Mugunthan made cpsw do DM_ETH I see drivers/net/phy/micrel.c talks
> DM_ETH and DT so lets get this PHY driver updated and then get this
> additional bit from the DT.  Thanks!
>
Thanks for the DT pointer I will look at it.  What about platforms that do not use DT yet?
Well maybe the more appropriate question is are there any platforms that still use the config file?

Dan

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