[PATCH] phy: add the IC+ IP1001 driver

[PATCH] phy: add the IC+ IP1001 driver

[Peppe CAVALLARO]

This patch adds the IC+ IP1001 (Gigabit Ethernet Transceiver) driver.
I've had to add an additional delay (2ns) to adjust RX clock phase at
GMII/ RGMII interface (according to the PHY data-sheet). This helps to
have the RGMII working on some ST platforms.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/phy/Kconfig  |    2 +-
 drivers/net/phy/icplus.c |   59 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cb3d13e..35fda5a 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -64,7 +64,7 @@ config BCM63XX_PHY
 config ICPLUS_PHY
 	tristate "Drivers for ICPlus PHYs"
 	---help---
-	  Currently supports the IP175C PHY.
+	  Currently supports the IP175C and IP1001 PHYs.
 
 config REALTEK_PHY
 	tristate "Drivers for Realtek PHYs"
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index c1d2d25..9a09e24 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -30,7 +30,7 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 
-MODULE_DESCRIPTION("ICPlus IP175C PHY driver");
+MODULE_DESCRIPTION("ICPlus IP175C/IC1001 PHY drivers");
 MODULE_AUTHOR("Michael Barkowski");
 MODULE_LICENSE("GPL");
 
@@ -89,6 +89,33 @@ static int ip175c_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int ip1001_config_init(struct phy_device *phydev)
+{
+	int err, value;
+
+	/* Software Reset PHY */
+	value = phy_read(phydev, MII_BMCR);
+	value |= BMCR_RESET;
+	err = phy_write(phydev, MII_BMCR, value);
+	if (err < 0)
+		return err;
+
+	do {
+		value = phy_read(phydev, MII_BMCR);
+	} while (value & BMCR_RESET);
+
+	/* Additional delay (2ns) used to adjust RX clock phase
+	 * at GMII/ RGMII interface */
+	value = phy_read(phydev, 16);
+	value |= 0x3;
+
+	err = phy_write(phydev, 16, value);
+	if (err < 0)
+		return err;
+
+	return err;
+}
+
 static int ip175c_read_status(struct phy_device *phydev)
 {
 	if (phydev->addr == 4) /* WAN port */
@@ -121,21 +148,43 @@ static struct phy_driver ip175c_driver = {
 	.driver		= { .owner = THIS_MODULE,},
 };
 
-static int __init ip175c_init(void)
+static struct phy_driver ip1001_driver = {
+	.phy_id		= 0x02430d90,
+	.name		= "ICPlus IP1001",
+	.phy_id_mask	= 0x0ffffff0,
+	.features	= PHY_GBIT_FEATURES | SUPPORTED_Pause |
+			  SUPPORTED_Asym_Pause,
+	.config_init	= &ip1001_config_init,
+	.config_aneg	= &genphy_config_aneg,
+	.read_status	= &genphy_read_status,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+	.driver		= { .owner = THIS_MODULE,},
+};
+
+static int __init icplus_init(void)
 {
+	int ret = 0;
+
+	ret = phy_driver_register(&ip1001_driver);
+	if (ret < 0)
+		return -ENODEV;
+
 	return phy_driver_register(&ip175c_driver);
 }
 
-static void __exit ip175c_exit(void)
+static void __exit icplus_exit(void)
 {
+	phy_driver_unregister(&ip1001_driver);
 	phy_driver_unregister(&ip175c_driver);
 }
 
-module_init(ip175c_init);
-module_exit(ip175c_exit);
+module_init(icplus_init);
+module_exit(icplus_exit);
 
 static struct mdio_device_id __maybe_unused icplus_tbl[] = {
 	{ 0x02430d80, 0x0ffffff0 },
+	{ 0x02430d90, 0x0ffffff0 },
 	{ }
 };
 
-- 
1.5.5.6

Re: [PATCH] phy: add the IC+ IP1001 driver

[David Miller]

From: Peppe CAVALLARO <peppe.cavallaro@st.com>
Date: Thu, 9 Dec 2010 10:05:13 +0100

> This patch adds the IC+ IP1001 (Gigabit Ethernet Transceiver) driver.
> I've had to add an additional delay (2ns) to adjust RX clock phase at
> GMII/ RGMII interface (according to the PHY data-sheet). This helps to
> have the RGMII working on some ST platforms.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

You forgot to update the icplus_tbl module device table with a
new 0x02430d90 entry.

Please fix this and resubmit.

Re: [PATCH] phy: add the IC+ IP1001 driver

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 10 Dec 2010 15:47:41 -0800 (PST)

> From: Peppe CAVALLARO <peppe.cavallaro@st.com>
> Date: Thu, 9 Dec 2010 10:05:13 +0100
> 
>> This patch adds the IC+ IP1001 (Gigabit Ethernet Transceiver) driver.
>> I've had to add an additional delay (2ns) to adjust RX clock phase at
>> GMII/ RGMII interface (according to the PHY data-sheet). This helps to
>> have the RGMII working on some ST platforms.
>> 
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> 
> You forgot to update the icplus_tbl module device table with a
> new 0x02430d90 entry.

Ignore this, I can't read! :-)

I'll apply this, thanks a lot.

Re: [PATCH] phy: add the IC+ IP1001 driver

[Andy Fleming]

On Thu, Dec 9, 2010 at 3:05 AM, Peppe CAVALLARO <peppe.cavallaro@st.com> wrote:
> This patch adds the IC+ IP1001 (Gigabit Ethernet Transceiver) driver.
> I've had to add an additional delay (2ns) to adjust RX clock phase at
> GMII/ RGMII interface (according to the PHY data-sheet). This helps to
> have the RGMII working on some ST platforms.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/net/phy/Kconfig  |    2 +-
>  drivers/net/phy/icplus.c |   59 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cb3d13e..35fda5a 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -64,7 +64,7 @@ config BCM63XX_PHY
>  config ICPLUS_PHY
>        tristate "Drivers for ICPlus PHYs"
>        ---help---
> -         Currently supports the IP175C PHY.
> +         Currently supports the IP175C and IP1001 PHYs.
>
>  config REALTEK_PHY
>        tristate "Drivers for Realtek PHYs"
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index c1d2d25..9a09e24 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -30,7 +30,7 @@
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
>
> -MODULE_DESCRIPTION("ICPlus IP175C PHY driver");
> +MODULE_DESCRIPTION("ICPlus IP175C/IC1001 PHY drivers");
>  MODULE_AUTHOR("Michael Barkowski");
>  MODULE_LICENSE("GPL");
>
> @@ -89,6 +89,33 @@ static int ip175c_config_init(struct phy_device *phydev)
>        return 0;
>  }
>
> +static int ip1001_config_init(struct phy_device *phydev)
> +{
> +       int err, value;
> +
> +       /* Software Reset PHY */
> +       value = phy_read(phydev, MII_BMCR);
> +       value |= BMCR_RESET;
> +       err = phy_write(phydev, MII_BMCR, value);
> +       if (err < 0)
> +               return err;
> +
> +       do {
> +               value = phy_read(phydev, MII_BMCR);
> +       } while (value & BMCR_RESET);
> +
> +       /* Additional delay (2ns) used to adjust RX clock phase
> +        * at GMII/ RGMII interface */
> +       value = phy_read(phydev, 16);
> +       value |= 0x3;


Two things:
1) Is this generic for all instances of this PHY, or only for the
particular board you're using?  Frequently these sorts of clock
adjustments are done to deal with skew on the lines between the PHY
and the NIC.  If this is a board-specific change, consider using a
board fixup or passing a flag from the NIC.

2) Can we have names for register 16 and value 0x3?

Andy

Re: [PATCH] phy: add the IC+ IP1001 driver

[Peppe CAVALLARO]

On 12/11/2010 12:48 AM, Andy Fleming wrote:
>
> On Thu, Dec 9, 2010 at 3:05 AM, Peppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
> > This patch adds the IC+ IP1001 (Gigabit Ethernet Transceiver) driver.
> > I've had to add an additional delay (2ns) to adjust RX clock phase at
> > GMII/ RGMII interface (according to the PHY data-sheet). This helps to
> > have the RGMII working on some ST platforms.
> >
> > Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> > ---
> >  drivers/net/phy/Kconfig  |    2 +-
> >  drivers/net/phy/icplus.c |   59
> ++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index cb3d13e..35fda5a 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -64,7 +64,7 @@ config BCM63XX_PHY
> >  config ICPLUS_PHY
> >        tristate "Drivers for ICPlus PHYs"
> >        ---help---
> > -         Currently supports the IP175C PHY.
> > +         Currently supports the IP175C and IP1001 PHYs.
> >
> >  config REALTEK_PHY
> >        tristate "Drivers for Realtek PHYs"
> > diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> > index c1d2d25..9a09e24 100644
> > --- a/drivers/net/phy/icplus.c
> > +++ b/drivers/net/phy/icplus.c
> > @@ -30,7 +30,7 @@
> >  #include <asm/irq.h>
> >  #include <asm/uaccess.h>
> >
> > -MODULE_DESCRIPTION("ICPlus IP175C PHY driver");
> > +MODULE_DESCRIPTION("ICPlus IP175C/IC1001 PHY drivers");
> >  MODULE_AUTHOR("Michael Barkowski");
> >  MODULE_LICENSE("GPL");
> >
> > @@ -89,6 +89,33 @@ static int ip175c_config_init(struct phy_device
> *phydev)
> >        return 0;
> >  }
> >
> > +static int ip1001_config_init(struct phy_device *phydev)
> > +{
> > +       int err, value;
> > +
> > +       /* Software Reset PHY */
> > +       value = phy_read(phydev, MII_BMCR);
> > +       value |= BMCR_RESET;
> > +       err = phy_write(phydev, MII_BMCR, value);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       do {
> > +               value = phy_read(phydev, MII_BMCR);
> > +       } while (value & BMCR_RESET);
> > +
> > +       /* Additional delay (2ns) used to adjust RX clock phase
> > +        * at GMII/ RGMII interface */
> > +       value = phy_read(phydev, 16);
> > +       value |= 0x3;
>
>
> Two things:
> 1) Is this generic for all instances of this PHY, or only for the
> particular board you're using?  Frequently these sorts of clock
> adjustments are done to deal with skew on the lines between the PHY
> and the NIC.  If this is a board-specific change, consider using a
> board fixup or passing a flag from the NIC.
>

After a call with the Hw validation,it seems that we
should only add this for the RGMII mode (not necessary for
GMII mode). From the data-sheet I cannot catch this detail.
>From the IC1001 data-sheer IIUC, it's to add input delay to
the GTX_CLK (2ns for GMII/RGMII and 4ns for MII).

I've tested, with this delay, both GMII and RGMII modes
without any issues.

Hmm, I could re-create the patch and only add the IP1001.
Timing adjustment, through R16 setting, could be in another patch.
Let me know.

> 2) Can we have names for register 16 and value 0x3?
>

Yes, we can.

Regards,
Peppe

>
> Andy
>