[PATCH v6 0/3] net: phy: Add xilinx gmiitorgmii converter support

[PATCH v6 0/3] net: phy: Add xilinx gmiitorgmii converter support

[Kedareswara rao Appana]

The Gigabit Media Independent Interface (GMII) to Reduced Gigabit Media
Independent Interface (RGMII) core provides the RGMII between RGMII-compliant
Ethernet physical media devices (PHY) and the Gigabit Ethernet controller.
This core can be used in all three modes of operation(10/100/1000 Mb/s).
The Management Data Input/Output (MDIO) interface is used to configure the
Speed of operation. This core can switch dynamically between the three
Different speed modes by configuring the conveter register through mdio write.

The conveter sits b/w the MAC and external phy like below

MACB <==> GMII2RGMII <==> RGMII_PHY

        MDIO    <========> GMII2RGMII
MCAB <=======>
                <========> RGMII

Using MAC MDIO bus we can access both the converter and the external PHY.
We need to program the line speed of the converter during run time based
On the external phy negotiated speed.

This patch series does the below
---> Add mask for Control register 10Mbps speed.
---> Add support for xilinx gmiitorgmii converter.

Kedareswara rao Appana (3):
  net: Add mask for Control register 10Mbps speed
  Documentation: DT: net: Add Xilinx gmiitorgmii converter device tree
    binding documentation
  net: phy: Add gmiitorgmii converter support

 .../devicetree/bindings/net/xilinx_gmii2rgmii.txt  |  35 +++++++
 drivers/net/phy/Kconfig                            |   7 ++
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/xilinx_gmii2rgmii.c                | 109 +++++++++++++++++++++
 include/uapi/linux/mii.h                           |   1 +
 5 files changed, 153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt
 create mode 100644 drivers/net/phy/xilinx_gmii2rgmii.c

-- 
2.1.2

[PATCH v6 1/3] net: Add mask for Control register 10Mbps speed

[Kedareswara rao Appana]

This patch adds mask for the Control register
10Mbps speed.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v6:
- None.
Changes for v5:
- New patch.

 include/uapi/linux/mii.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 237fac4..15d8510 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -48,6 +48,7 @@
 #define BMCR_SPEED100		0x2000	/* Select 100Mbps              */
 #define BMCR_LOOPBACK		0x4000	/* TXD loopback bits           */
 #define BMCR_RESET		0x8000	/* Reset to default state      */
+#define BMCR_SPEED10		0x0000	/* Select 10Mbps               */
 
 /* Basic mode status register. */
 #define BMSR_ERCAP		0x0001	/* Ext-reg capability          */
-- 
2.1.2

[PATCH v6 2/3] Documentation: DT: net: Add Xilinx gmiitorgmii converter device tree binding documentation

[Kedareswara rao Appana]

Device-tree binding documentation for xilinx gmiitorgmii converter.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v6:
---> Removed mdio description as suggested by Florian.
Changes for v5:
---> Fixed Indentation in the example as suggested by Michal.
Changes for v4:
--> Modified compatible as suggested by Rob.
--> Removed underscores from the converter node name as suggested by Rob.
Changes for v3:
--> None.
Changes for v2:
--> New patch

 .../devicetree/bindings/net/xilinx_gmii2rgmii.txt  | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt

diff --git a/Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt b/Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt
new file mode 100644
index 0000000..038dda4
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt
@@ -0,0 +1,35 @@
+XILINX GMIITORGMII Converter Driver Device Tree Bindings
+--------------------------------------------------------
+
+The Gigabit Media Independent Interface (GMII) to Reduced Gigabit Media
+Independent Interface (RGMII) core provides the RGMII between RGMII-compliant
+Ethernet physical media devices (PHY) and the Gigabit Ethernet controller.
+This core can be used in all three modes of operation(10/100/1000 Mb/s).
+The Management Data Input/Output (MDIO) interface is used to configure the
+Speed of operation. This core can switch dynamically between the three
+Different speed modes by configuring the conveter register through mdio write.
+
+This converter sits between the ethernet MAC and the external phy.
+MAC <==> GMII2RGMII <==> RGMII_PHY
+
+For more details about mdio please refer phy.txt file in the same directory.
+
+Required properties:
+- compatible	: Should be "xlnx,gmii-to-rgmii-1.0"
+- reg		: The ID number for the phy, usually a small integer
+- phy-handle	: Should point to the external phy device.
+		  See ethernet.txt file in the same directory.
+
+Example:
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		phy: ethernet-phy@0 {
+			......
+		};
+		gmiitorgmii: gmiitorgmii@8 {
+			compatible = "xlnx,gmii-to-rgmii-1.0";
+			reg = <8>;
+			phy-handle = <&phy>;
+		};
+	};
-- 
2.1.2

[PATCH v6 3/3] net: phy: Add gmiitorgmii converter support

[Kedareswara rao Appana]

This patch adds support for gmiitorgmii converter.

The GMII to RGMII IP core provides the Reduced Gigabit Media
Independent Interface (RGMII) between Ethernet physical media
Devices and the Gigabit Ethernet controller. This core can
Switch dynamically between the three different speed modes of
Operation by configuring the converter register through mdio write.

MDIO interface is used to set operating speed of Ethernet MAC.

This converter sits between the MAC and the external phy
MAC <==> GMII2RGMII <==> RGMII_PHY

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Thanks a lot Andrew for your inputs.
Changes for v6:
--> Don't force phy to default enabled as suggested by Florian.
--> Fix Mask value as suggested by Florian.
--> Used mdio_module_driver as suggested by Florian.
--> Remove switch case and used if-else conditions for phy speed
    checking as suggested by Florian.
Changes for v5:
--> Fixed return values in the probe as suggested by punnaiah.
--> Added a mask for the converter speed as suggested by punnaiah.
Changes for v4:
--> Updated phydev speed for all 3 speeds as suggested by zhuyj.
Changes for v3:
--> Updated the driver as suggested by Andrew.
Changes for v2:
--> Passed struct xphy pointer directly to the fix_mac_speed
API as suggested by the Florian.
--> Added checks for the phy-node fail case as suggested
by the Florian

 drivers/net/phy/Kconfig             |   7 +++
 drivers/net/phy/Makefile            |   1 +
 drivers/net/phy/xilinx_gmii2rgmii.c | 109 ++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 drivers/net/phy/xilinx_gmii2rgmii.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1b534ea..d66133bf 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -312,6 +312,13 @@ config MICROSEMI_PHY
     ---help---
       Currently supports the VSC8531 and VSC8541 PHYs
 
+config XILINX_GMII2RGMII
+       tristate "Xilinx GMII2RGMII converter driver"
+       ---help---
+         This driver support xilinx GMII to RGMII IP core it provides
+         the Reduced Gigabit Media Independent Interface(RGMII) between
+         Ethernet physical media devices and the Gigabit Ethernet controller.
+
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a713bd4..73d65ce 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -50,3 +50,4 @@ obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
 obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
 obj-$(CONFIG_MDIO_XGENE)	+= mdio-xgene.o
+obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
new file mode 100644
index 0000000..8e980ad
--- /dev/null
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -0,0 +1,109 @@
+/* Xilinx GMII2RGMII Converter driver
+ *
+ * Copyright (C) 2016 Xilinx, Inc.
+ *
+ * Author: Kedareswara rao Appana <appanad@xilinx.com>
+ *
+ * Description:
+ * This driver is developed for Xilinx GMII2RGMII Converter
+ *
+ * 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, or
+ * (at your option) any later version.
+ *
+ * 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/module.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/of_mdio.h>
+
+#define XILINX_GMII2RGMII_REG		0x10
+#define XILINX_GMII2RGMII_SPEED_MASK	(BMCR_SPEED1000 | BMCR_SPEED100)
+
+struct gmii2rgmii {
+	struct phy_device *phy_dev;
+	struct phy_driver *phy_drv;
+	struct phy_driver conv_phy_drv;
+	int addr;
+};
+
+static int xgmiitorgmii_read_status(struct phy_device *phydev)
+{
+	struct gmii2rgmii *priv = phydev->priv;
+	u16 val = 0;
+
+	priv->phy_drv->read_status(phydev);
+
+	val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);
+	val &= XILINX_GMII2RGMII_SPEED_MASK;
+
+	if (phydev->speed == SPEED_1000)
+		val |= BMCR_SPEED1000;
+	else if (phydev->speed == SPEED_100)
+		val |= BMCR_SPEED100;
+	else
+		val |= BMCR_SPEED10;
+
+	mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val);
+
+	return 0;
+}
+
+int xgmiitorgmii_probe(struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct device_node *np = dev->of_node, *phy_node;
+	struct gmii2rgmii *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phy_node = of_parse_phandle(np, "phy-handle", 0);
+	if (IS_ERR(phy_node)) {
+		dev_err(dev, "Couldn't parse phy-handle\n");
+		return -ENODEV;
+	}
+
+	priv->phy_dev = of_phy_find_device(phy_node);
+	if (!priv->phy_dev) {
+		dev_info(dev, "Couldn't find phydev\n");
+		return -EPROBE_DEFER;
+	}
+
+	priv->addr = mdiodev->addr;
+	priv->phy_drv = priv->phy_dev->drv;
+	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
+	       sizeof(struct phy_driver));
+	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
+	priv->phy_dev->priv = priv;
+	priv->phy_dev->drv = &priv->conv_phy_drv;
+
+	return 0;
+}
+
+static const struct of_device_id xgmiitorgmii_of_match[] = {
+	{ .compatible = "xlnx,gmii-to-rgmii-1.0" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
+
+static struct mdio_driver xgmiitorgmii_driver = {
+	.probe	= xgmiitorgmii_probe,
+	.mdiodrv.driver = {
+		.name = "xgmiitorgmii",
+		.of_match_table = xgmiitorgmii_of_match,
+	},
+};
+
+mdio_module_driver(xgmiitorgmii_driver);
+
+MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
+MODULE_LICENSE("GPL");
-- 
2.1.2

Re: [PATCH v6 2/3] Documentation: DT: net: Add Xilinx gmiitorgmii converter device tree binding documentation

[Rob Herring]

On Wed, Aug 10, 2016 at 11:20:07AM +0530, Kedareswara rao Appana wrote:
> Device-tree binding documentation for xilinx gmiitorgmii converter.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v6:
> ---> Removed mdio description as suggested by Florian.
> Changes for v5:
> ---> Fixed Indentation in the example as suggested by Michal.
> Changes for v4:
> --> Modified compatible as suggested by Rob.
> --> Removed underscores from the converter node name as suggested by Rob.
> Changes for v3:
> --> None.
> Changes for v2:
> --> New patch
> 
>  .../devicetree/bindings/net/xilinx_gmii2rgmii.txt  | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 0/3] net: phy: Add xilinx gmiitorgmii converter support

[David Miller]

From: Kedareswara rao Appana <appana.durga.rao@xilinx.com>
Date: Wed, 10 Aug 2016 11:20:05 +0530

> The Gigabit Media Independent Interface (GMII) to Reduced Gigabit Media
> Independent Interface (RGMII) core provides the RGMII between RGMII-compliant
> Ethernet physical media devices (PHY) and the Gigabit Ethernet controller.
> This core can be used in all three modes of operation(10/100/1000 Mb/s).
> The Management Data Input/Output (MDIO) interface is used to configure the
> Speed of operation. This core can switch dynamically between the three
> Different speed modes by configuring the conveter register through mdio write.
> 
> The conveter sits b/w the MAC and external phy like below
> 
> MACB <==> GMII2RGMII <==> RGMII_PHY
> 
>         MDIO    <========> GMII2RGMII
> MCAB <=======>
>                 <========> RGMII
> 
> Using MAC MDIO bus we can access both the converter and the external PHY.
> We need to program the line speed of the converter during run time based
> On the external phy negotiated speed.
> 
> This patch series does the below
> ---> Add mask for Control register 10Mbps speed.
> ---> Add support for xilinx gmiitorgmii converter.

Series applied, thanks.

Re: [PATCH v6 3/3] net: phy: Add gmiitorgmii converter support

[Andrew Lunn]

> +static int xgmiitorgmii_read_status(struct phy_device *phydev)
> +{
> +	struct gmii2rgmii *priv = phydev->priv;
> +	u16 val = 0;
> +
> +	priv->phy_drv->read_status(phydev);

This can return an error, in which case phydev->speed should not be
trusted.

I've not thought locking all the way through yet. I don't think you
need a lock here, but i need to think about it.

> +
> +	val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);

You should check for an error here.

> +	val &= XILINX_GMII2RGMII_SPEED_MASK;
> +
> +	if (phydev->speed == SPEED_1000)
> +		val |= BMCR_SPEED1000;
> +	else if (phydev->speed == SPEED_100)
> +		val |= BMCR_SPEED100;
> +	else
> +		val |= BMCR_SPEED10;

What happens if for example the PHY is an aquantia and has negotiated
SPEED_2500? Some Marvell PHYs can also do odd speeds, like 200Mbps.
You probably want to return an error, rather than silently have things
go wrong.

> +
> +	mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val);

This can also return an error.

> +	return 0;
> +}
> +
> +int xgmiitorgmii_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct device_node *np = dev->of_node, *phy_node;
> +	struct gmii2rgmii *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phy_node = of_parse_phandle(np, "phy-handle", 0);
> +	if (IS_ERR(phy_node)) {
> +		dev_err(dev, "Couldn't parse phy-handle\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->phy_dev = of_phy_find_device(phy_node);
> +	if (!priv->phy_dev) {
> +		dev_info(dev, "Couldn't find phydev\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	priv->addr = mdiodev->addr;
> +	priv->phy_drv = priv->phy_dev->drv;
> +	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
> +	       sizeof(struct phy_driver));
> +	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
> +	priv->phy_dev->priv = priv;
> +	priv->phy_dev->drv = &priv->conv_phy_drv;

So from this point onward, the phy driver depends on the memory
allocated by this driver. If this driver goes away, freeing its
memory, the next call to read_status() is going to have a problem.

Also, i think this assignment should take the phy lock, just to be
safe.

There also needs to be some thought into what happens if the phy
driver is unloaded. Should this driver take a reference on the phy
driver to prevent that?

       Andrew

RE: [PATCH v6 3/3] net: phy: Add gmiitorgmii converter support

[Appana Durga Kedareswara Rao]

Hi Andrew,

	Thanks for the review...

> 
> > +static int xgmiitorgmii_read_status(struct phy_device *phydev) {
> > +	struct gmii2rgmii *priv = phydev->priv;
> > +	u16 val = 0;
> > +
> > +	priv->phy_drv->read_status(phydev);
> 
> This can return an error, in which case phydev->speed should not be trusted.

Will fix...

> 
> I've not thought locking all the way through yet. I don't think you need a lock
> here, but i need to think about it.

Ok...

> 
> > +
> > +	val = mdiobus_read(phydev->mdio.bus, priv->addr,
> > +XILINX_GMII2RGMII_REG);
> 
> You should check for an error here.
> 
> > +	val &= XILINX_GMII2RGMII_SPEED_MASK;
> > +
> > +	if (phydev->speed == SPEED_1000)
> > +		val |= BMCR_SPEED1000;
> > +	else if (phydev->speed == SPEED_100)
> > +		val |= BMCR_SPEED100;
> > +	else
> > +		val |= BMCR_SPEED10;
> 
> What happens if for example the PHY is an aquantia and has negotiated
> SPEED_2500? Some Marvell PHYs can also do odd speeds, like 200Mbps.
> You probably want to return an error, rather than silently have things go wrong.

Will fix...

> 
> > +
> > +	mdiobus_write(phydev->mdio.bus, priv->addr,
> XILINX_GMII2RGMII_REG,
> > +val);
> 
> This can also return an error.

Will fix...

> 
> > +	return 0;
> > +}
> > +
> > +int xgmiitorgmii_probe(struct mdio_device *mdiodev) {
> > +	struct device *dev = &mdiodev->dev;
> > +	struct device_node *np = dev->of_node, *phy_node;
> > +	struct gmii2rgmii *priv;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	phy_node = of_parse_phandle(np, "phy-handle", 0);
> > +	if (IS_ERR(phy_node)) {
> > +		dev_err(dev, "Couldn't parse phy-handle\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	priv->phy_dev = of_phy_find_device(phy_node);
> > +	if (!priv->phy_dev) {
> > +		dev_info(dev, "Couldn't find phydev\n");
> > +		return -EPROBE_DEFER;
> > +	}
> > +
> > +	priv->addr = mdiodev->addr;
> > +	priv->phy_drv = priv->phy_dev->drv;
> > +	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
> > +	       sizeof(struct phy_driver));
> > +	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
> > +	priv->phy_dev->priv = priv;
> > +	priv->phy_dev->drv = &priv->conv_phy_drv;
> 
> So from this point onward, the phy driver depends on the memory allocated by
> this driver. If this driver goes away, freeing its memory, the next call to
> read_status() is going to have a problem.
> 
> Also, i think this assignment should take the phy lock, just to be safe.

Ok will fix....

> 
> There also needs to be some thought into what happens if the phy driver is
> unloaded. Should this driver take a reference on the phy driver to prevent that?

Ok will increment the reference count of the external phy device...


Regards,
Kedar.

> 
>        Andrew