[RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[Kedareswara rao Appana]

This patch series does the following
---> Add support for gmii2rgmii converter.
---> Add support for gmii2rgmii converter in the macb driver.

Earlier sent one RFC patch https://patchwork.kernel.org/patch/9186615/ 
which includes converter related stuff also in macb driver.
This patch series fixes this issue.

Kedareswara rao Appana (2):
  net: ethernet: xilinx: Add gmii2rgmii converter support
  net: macb: Add gmii2rgmii phy converter support

 drivers/net/ethernet/cadence/macb.c             |   21 ++++++-
 drivers/net/ethernet/cadence/macb.h             |    3 +
 drivers/net/ethernet/xilinx/Kconfig             |    7 ++
 drivers/net/ethernet/xilinx/Makefile            |    1 +
 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c |   76 +++++++++++++++++++++++
 include/linux/xilinx_gmii2rgmii.h               |   24 +++++++
 6 files changed, 131 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
 create mode 100644 include/linux/xilinx_gmii2rgmii.h

[RFC PATCH 1/2] net: ethernet: xilinx: Add gmii2rgmii converter support

[Kedareswara rao Appana]

This patch adds support for gmii2rgmii 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.
MDIO interface is used to set operating speed of Ethernet MAC

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
 drivers/net/ethernet/xilinx/Kconfig             |    7 ++
 drivers/net/ethernet/xilinx/Makefile            |    1 +
 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c |   76 +++++++++++++++++++++++
 include/linux/xilinx_gmii2rgmii.h               |   24 +++++++
 4 files changed, 108 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
 create mode 100644 include/linux/xilinx_gmii2rgmii.h

diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
index 4f5c024..d7df70a 100644
--- a/drivers/net/ethernet/xilinx/Kconfig
+++ b/drivers/net/ethernet/xilinx/Kconfig
@@ -39,4 +39,11 @@ config XILINX_LL_TEMAC
 	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
 	  core used in Xilinx Spartan and Virtex FPGAs
 
+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 # NET_VENDOR_XILINX
diff --git a/drivers/net/ethernet/xilinx/Makefile b/drivers/net/ethernet/xilinx/Makefile
index 214205e..bca0da0 100644
--- a/drivers/net/ethernet/xilinx/Makefile
+++ b/drivers/net/ethernet/xilinx/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_XILINX_LL_TEMAC) += ll_temac.o
 obj-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
 xilinx_emac-objs := xilinx_axienet_main.o xilinx_axienet_mdio.o
 obj-$(CONFIG_XILINX_AXI_EMAC) += xilinx_emac.o
+obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
diff --git a/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
new file mode 100644
index 0000000..ca9f1ad
--- /dev/null
+++ b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
@@ -0,0 +1,76 @@
+/* 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/types.h>
+#include <linux/netdevice.h>
+#include <linux/mii.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_mdio.h>
+#include <linux/xilinx_gmii2rgmii.h>
+
+static void xgmii2rgmii_fix_mac_speed(void *priv, unsigned int speed)
+{
+	struct gmii2rgmii *xphy = (struct xphy *)priv;
+	struct phy_device *gmii2rgmii_phydev = xphy->gmii2rgmii_phy_dev;
+	u16 gmii2rgmii_reg = 0;
+
+	switch (speed) {
+	case 1000:
+		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED1000;
+		break;
+	case 100:
+		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED100;
+		break;
+	default:
+		return;
+	}
+
+	xphy->mdio_write(xphy->mii_bus, gmii2rgmii_phydev->mdio.addr,
+			 XILINX_GMII2RGMII_REG_NUM,
+			 gmii2rgmii_reg);
+}
+
+int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
+{
+	struct device_node *phy_node;
+	struct phy_device *phydev;
+	struct device_node *np = (struct device_node *)xphy->platform_data;
+
+	phy_node = of_parse_phandle(np, "gmii2rgmii-phy-handle", 0);
+	if (phy_node) {
+		phydev = of_phy_attach(xphy->dev, phy_node, 0, 0);
+		if (!phydev) {
+			netdev_err(xphy->dev,
+				   "%s: no gmii to rgmii converter found\n",
+				   xphy->dev->name);
+			return -1;
+		}
+		xphy->gmii2rgmii_phy_dev = phydev;
+	}
+	xphy->fix_mac_speed = xgmii2rgmii_fix_mac_speed;
+
+	return 0;
+}
+EXPORT_SYMBOL(gmii2rgmii_phyprobe);
+
+MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/xilinx_gmii2rgmii.h b/include/linux/xilinx_gmii2rgmii.h
new file mode 100644
index 0000000..64e1659
--- /dev/null
+++ b/include/linux/xilinx_gmii2rgmii.h
@@ -0,0 +1,24 @@
+#ifndef _GMII2RGMII_H
+#define _GMII2RGMII_H
+
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/mii.h>
+
+#define XILINX_GMII2RGMII_FULLDPLX		BMCR_FULLDPLX
+#define XILINX_GMII2RGMII_SPEED1000		BMCR_SPEED1000
+#define XILINX_GMII2RGMII_SPEED100		BMCR_SPEED100
+#define XILINX_GMII2RGMII_REG_NUM			0x10
+
+struct gmii2rgmii {
+	struct net_device	*dev;
+	struct mii_bus		*mii_bus;
+	struct phy_device	*gmii2rgmii_phy_dev;
+	void			*platform_data;
+	void (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
+			   int val);
+	void (*fix_mac_speed)(void *priv, unsigned int speed);
+};
+
+extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
+#endif
-- 
1.7.1

Re: [RFC PATCH 1/2] net: ethernet: xilinx: Add gmii2rgmii converter support

[Florian Fainelli]

Le 30/06/2016 23:20, Kedareswara rao Appana a écrit :
> This patch adds support for gmii2rgmii 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.
> MDIO interface is used to set operating speed of Ethernet MAC
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
>  drivers/net/ethernet/xilinx/Kconfig             |    7 ++
>  drivers/net/ethernet/xilinx/Makefile            |    1 +
>  drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c |   76 +++++++++++++++++++++++
>  include/linux/xilinx_gmii2rgmii.h               |   24 +++++++
>  4 files changed, 108 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
>  create mode 100644 include/linux/xilinx_gmii2rgmii.h
> 
> diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
> index 4f5c024..d7df70a 100644
> --- a/drivers/net/ethernet/xilinx/Kconfig
> +++ b/drivers/net/ethernet/xilinx/Kconfig
> @@ -39,4 +39,11 @@ config XILINX_LL_TEMAC
>  	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
>  	  core used in Xilinx Spartan and Virtex FPGAs
>  
> +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 # NET_VENDOR_XILINX
> diff --git a/drivers/net/ethernet/xilinx/Makefile b/drivers/net/ethernet/xilinx/Makefile
> index 214205e..bca0da0 100644
> --- a/drivers/net/ethernet/xilinx/Makefile
> +++ b/drivers/net/ethernet/xilinx/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_XILINX_LL_TEMAC) += ll_temac.o
>  obj-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
>  xilinx_emac-objs := xilinx_axienet_main.o xilinx_axienet_mdio.o
>  obj-$(CONFIG_XILINX_AXI_EMAC) += xilinx_emac.o
> +obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> diff --git a/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
> new file mode 100644
> index 0000000..ca9f1ad
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
> @@ -0,0 +1,76 @@
> +/* 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/types.h>
> +#include <linux/netdevice.h>
> +#include <linux/mii.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_mdio.h>
> +#include <linux/xilinx_gmii2rgmii.h>
> +
> +static void xgmii2rgmii_fix_mac_speed(void *priv, unsigned int speed)
> +{
> +	struct gmii2rgmii *xphy = (struct xphy *)priv;

Why not pass struct xphy pointer directly?

> +	struct phy_device *gmii2rgmii_phydev = xphy->gmii2rgmii_phy_dev;
> +	u16 gmii2rgmii_reg = 0;
> +
> +	switch (speed) {
> +	case 1000:
> +		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED1000;
> +		break;
> +	case 100:
> +		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED100;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	xphy->mdio_write(xphy->mii_bus, gmii2rgmii_phydev->mdio.addr,
> +			 XILINX_GMII2RGMII_REG_NUM,
> +			 gmii2rgmii_reg);
> +}
> +
> +int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
> +{
> +	struct device_node *phy_node;
> +	struct phy_device *phydev;
> +	struct device_node *np = (struct device_node *)xphy->platform_data;
> +
> +	phy_node = of_parse_phandle(np, "gmii2rgmii-phy-handle", 0);

Is that property documented in a binding document?

> +	if (phy_node) {

Should not there be an else clause which does not assign
xphy->fix_mac_speed in case this property lookup fails?

> +		phydev = of_phy_attach(xphy->dev, phy_node, 0, 0);
> +		if (!phydev) {
> +			netdev_err(xphy->dev,
> +				   "%s: no gmii to rgmii converter found\n",
> +				   xphy->dev->name);
> +			return -1;
> +		}
> +		xphy->gmii2rgmii_phy_dev = phydev;
> +	}
> +	xphy->fix_mac_speed = xgmii2rgmii_fix_mac_speed;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(gmii2rgmii_phyprobe);
> +
> +MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/xilinx_gmii2rgmii.h b/include/linux/xilinx_gmii2rgmii.h
> new file mode 100644
> index 0000000..64e1659
> --- /dev/null
> +++ b/include/linux/xilinx_gmii2rgmii.h
> @@ -0,0 +1,24 @@
> +#ifndef _GMII2RGMII_H
> +#define _GMII2RGMII_H
> +
> +#include <linux/of.h>
> +#include <linux/phy.h>
> +#include <linux/mii.h>
> +
> +#define XILINX_GMII2RGMII_FULLDPLX		BMCR_FULLDPLX
> +#define XILINX_GMII2RGMII_SPEED1000		BMCR_SPEED1000
> +#define XILINX_GMII2RGMII_SPEED100		BMCR_SPEED100
> +#define XILINX_GMII2RGMII_REG_NUM			0x10

So the register semantics are fairly standard but not the register
location, have you considered writing a small PHY driver for this block?

> +
> +struct gmii2rgmii {
> +	struct net_device	*dev;
> +	struct mii_bus		*mii_bus;
> +	struct phy_device	*gmii2rgmii_phy_dev;
> +	void			*platform_data;
> +	void (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
> +			   int val);
> +	void (*fix_mac_speed)(void *priv, unsigned int speed);
> +};
> +
> +extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
> +#endif
> 


-- 
Florian

RE: [RFC PATCH 1/2] net: ethernet: xilinx: Add gmii2rgmii converter support

[Appana Durga Kedareswara Rao]

Hi Florian,

	Thanks for the review.

> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/mii.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/xilinx_gmii2rgmii.h>
> > +
> > +static void xgmii2rgmii_fix_mac_speed(void *priv, unsigned int speed)
> > +{
> > +	struct gmii2rgmii *xphy = (struct xphy *)priv;
> 
> Why not pass struct xphy pointer directly?

Ok will fix in v2...

> 
> > +	struct phy_device *gmii2rgmii_phydev = xphy->gmii2rgmii_phy_dev;
> > +	u16 gmii2rgmii_reg = 0;
> > +
> > +	switch (speed) {
> > +	case 1000:
> > +		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED1000;
> > +		break;
> > +	case 100:
> > +		gmii2rgmii_reg |= XILINX_GMII2RGMII_SPEED100;
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	xphy->mdio_write(xphy->mii_bus, gmii2rgmii_phydev->mdio.addr,
> > +			 XILINX_GMII2RGMII_REG_NUM,
> > +			 gmii2rgmii_reg);
> > +}
> > +
> > +int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) {
> > +	struct device_node *phy_node;
> > +	struct phy_device *phydev;
> > +	struct device_node *np = (struct device_node *)xphy->platform_data;
> > +
> > +	phy_node = of_parse_phandle(np, "gmii2rgmii-phy-handle", 0);
> 
> Is that property documented in a binding document?

Will document. Will fix in v2...

> 
> > +	if (phy_node) {
> 
> Should not there be an else clause which does not assign
> xphy->fix_mac_speed in case this property lookup fails?

Will fix in v2...

> 
> > +		phydev = of_phy_attach(xphy->dev, phy_node, 0, 0);
> > +		if (!phydev) {
> > +			netdev_err(xphy->dev,
> > +				   "%s: no gmii to rgmii converter found\n",
> > +				   xphy->dev->name);
> > +			return -1;
> > +		}
> > +		xphy->gmii2rgmii_phy_dev = phydev;
> > +	}
> > +	xphy->fix_mac_speed = xgmii2rgmii_fix_mac_speed;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(gmii2rgmii_phyprobe);
> > +
> > +MODULE_DESCRIPTION("Xilinx GMII2RGMII converter driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/xilinx_gmii2rgmii.h
> > b/include/linux/xilinx_gmii2rgmii.h
> > new file mode 100644
> > index 0000000..64e1659
> > --- /dev/null
> > +++ b/include/linux/xilinx_gmii2rgmii.h
> > @@ -0,0 +1,24 @@
> > +#ifndef _GMII2RGMII_H
> > +#define _GMII2RGMII_H
> > +
> > +#include <linux/of.h>
> > +#include <linux/phy.h>
> > +#include <linux/mii.h>
> > +
> > +#define XILINX_GMII2RGMII_FULLDPLX		BMCR_FULLDPLX
> > +#define XILINX_GMII2RGMII_SPEED1000		BMCR_SPEED1000
> > +#define XILINX_GMII2RGMII_SPEED100		BMCR_SPEED100
> > +#define XILINX_GMII2RGMII_REG_NUM			0x10
> 
> So the register semantics are fairly standard but not the register location, have
> you considered writing a small PHY driver for this block?

I tried but this PHY doesn't have any vendor / Device ID's
This converter won't suit to PHY framework as we need to programmed the
PHY Control register with the external phy negotiated speed as explained in the other
Mail thread...

Regards,
Kedar. 

[RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Kedareswara rao Appana]

This patch adds support for gmii2rgmii phy converter
in the macb driver.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
 drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
 drivers/net/ethernet/cadence/macb.h |    3 +++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index cb07d95..de0ad71 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	return 0;
 }
 
+static inline void macb_hw_fix_mac_speed(struct macb *bp,
+					 struct phy_device *phydev)
+{
+	if (likely(bp->converter_phy.fix_mac_speed))
+		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
+						phydev->speed);
+}
+
 /**
  * macb_set_tx_clk() - Set a clock to a new frequency
  * @clk		Pointer to the clock to change
@@ -329,6 +337,7 @@ static void macb_handle_link_change(struct net_device *dev)
 				reg |= GEM_BIT(GBE);
 
 			macb_or_gem_writel(bp, NCFGR, reg);
+			macb_hw_fix_mac_speed(bp, phydev);
 
 			bp->speed = phydev->speed;
 			bp->duplex = phydev->duplex;
@@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device *pdev)
 			struct clk **, struct clk **)
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *np1, *np11;
 	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
@@ -3011,6 +3020,16 @@ static int macb_probe(struct platform_device *pdev)
 		goto err_out_free_netdev;
 
 	phydev = bp->phy_dev;
+	np1 = of_get_next_child(np, NULL);
+	for_each_child_of_node(np1, np11) {
+		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
+			bp->converter_phy.dev = dev;
+			bp->converter_phy.mii_bus = bp->mii_bus;
+			bp->converter_phy.mdio_write = macb_mdio_write;
+			bp->converter_phy.platform_data = bp->pdev->dev.of_node;
+			gmii2rgmii_phyprobe(&bp->converter_phy);
+		}
+	}
 
 	netif_carrier_off(dev);
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8a13824..735bce2 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,8 @@
 #ifndef _MACB_H
 #define _MACB_H
 
+#include <linux/xilinx_gmii2rgmii.h>
+
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
 #define MACB_MAX_QUEUES 8
@@ -846,6 +848,7 @@ struct macb {
 	unsigned int		jumbo_max_len;
 
 	u32			wol;
+	struct	gmii2rgmii	converter_phy;
 };
 
 static inline bool macb_is_gem(struct macb *bp)
-- 
1.7.1

Re: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Nicolas Ferre]

Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
> This patch adds support for gmii2rgmii phy converter
> in the macb driver.

Okay, I'd like more explanation here.
Hints & key words:
- dt property
- mdio bus
- mac speed


> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
>  drivers/net/ethernet/cadence/macb.h |    3 +++
>  2 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index cb07d95..de0ad71 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  	return 0;
>  }
>  
> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
> +					 struct phy_device *phydev)
> +{
> +	if (likely(bp->converter_phy.fix_mac_speed))

What is the purpose of this branch bias? The code isn't in some hot
path, so I suspect that its not needed.

> +		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
> +						phydev->speed);
> +}
> +
>  /**
>   * macb_set_tx_clk() - Set a clock to a new frequency
>   * @clk		Pointer to the clock to change
> @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct net_device *dev)
>  				reg |= GEM_BIT(GBE);
>  
>  			macb_or_gem_writel(bp, NCFGR, reg);
> +			macb_hw_fix_mac_speed(bp, phydev);
>  
>  			bp->speed = phydev->speed;
>  			bp->duplex = phydev->duplex;
> @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device *pdev)
>  			struct clk **, struct clk **)
>  					      = macb_clk_init;
>  	int (*init)(struct platform_device *) = macb_init;
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np = pdev->dev.of_node, *np1, *np11;

Nitpicking:
Be more explicit on variable names. Simple name for the iterator is
okay, the other is better if changed.
I also like to see variable on separated lines.

>  	struct device_node *phy_node;
>  	const struct macb_config *macb_config = NULL;
>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> @@ -3011,6 +3020,16 @@ static int macb_probe(struct platform_device *pdev)
>  		goto err_out_free_netdev;
>  
>  	phydev = bp->phy_dev;
> +	np1 = of_get_next_child(np, NULL);
> +	for_each_child_of_node(np1, np11) {
> +		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {

This would definitively need documentation and at least link in the macb
binding to show how this emulated phy connect to the mdio bus...

I'm not able to judge if the node arrangement is okay: I let Florian
tell his view on this...

> +			bp->converter_phy.dev = dev;
> +			bp->converter_phy.mii_bus = bp->mii_bus;
> +			bp->converter_phy.mdio_write = macb_mdio_write;
> +			bp->converter_phy.platform_data = bp->pdev->dev.of_node;
> +			gmii2rgmii_phyprobe(&bp->converter_phy);
> +		}
> +	}
>  
>  	netif_carrier_off(dev);
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 8a13824..735bce2 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,8 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#include <linux/xilinx_gmii2rgmii.h>

No, put it in the macb.c.

> +
>  #define MACB_GREGS_NBR 16
>  #define MACB_GREGS_VERSION 2
>  #define MACB_MAX_QUEUES 8
> @@ -846,6 +848,7 @@ struct macb {
>  	unsigned int		jumbo_max_len;
>  
>  	u32			wol;
> +	struct	gmii2rgmii	converter_phy;
>  };
>  
>  static inline bool macb_is_gem(struct macb *bp)


If Florian and phy guys are okay with the approach, I'm fine with this
patch, once corrected.

Thanks, bye,
-- 
Nicolas Ferre

RE: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Appana Durga Kedareswara Rao]

Hi Nicolas Ferre,
	
	Thanks for the quick review...

> 
> Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
> > This patch adds support for gmii2rgmii phy converter in the macb
> > driver.
> 
> Okay, I'd like more explanation here.
> Hints & key words:
> - dt property
> - mdio bus
> - mac speed

Sure will fix in the next version...

> 
> 
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> >  drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
> >  drivers/net/ethernet/cadence/macb.h |    3 +++
> >  2 files changed, 23 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.c
> > b/drivers/net/ethernet/cadence/macb.c
> > index cb07d95..de0ad71 100644
> > --- a/drivers/net/ethernet/cadence/macb.c
> > +++ b/drivers/net/ethernet/cadence/macb.c
> > @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
> mii_id, int regnum,
> >  	return 0;
> >  }
> >
> > +static inline void macb_hw_fix_mac_speed(struct macb *bp,
> > +					 struct phy_device *phydev)
> > +{
> > +	if (likely(bp->converter_phy.fix_mac_speed))
> 
> What is the purpose of this branch bias? The code isn't in some hot path, so I
> suspect that its not needed.

If we won't put this check driver will crash with NULL pointer dereference for the below cases
---> Converter driver is disabled
---> Converter driver is enabled but the converter probe is not called from the macb driver.

> 
> > +		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
> > +						phydev->speed);
> > +}
> > +
> >  /**
> >   * macb_set_tx_clk() - Set a clock to a new frequency
> >   * @clk		Pointer to the clock to change
> > @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct
> net_device *dev)
> >  				reg |= GEM_BIT(GBE);
> >
> >  			macb_or_gem_writel(bp, NCFGR, reg);
> > +			macb_hw_fix_mac_speed(bp, phydev);
> >
> >  			bp->speed = phydev->speed;
> >  			bp->duplex = phydev->duplex;
> > @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device
> *pdev)
> >  			struct clk **, struct clk **)
> >  					      = macb_clk_init;
> >  	int (*init)(struct platform_device *) = macb_init;
> > -	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *np = pdev->dev.of_node, *np1, *np11;
> 
> Nitpicking:
> Be more explicit on variable names. Simple name for the iterator is okay, the
> other is better if changed.
> I also like to see variable on separated lines.

Ok sure will fix in the next version...

> 
> >  	struct device_node *phy_node;
> >  	const struct macb_config *macb_config = NULL;
> >  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16
> > @@ static int macb_probe(struct platform_device *pdev)
> >  		goto err_out_free_netdev;
> >
> >  	phydev = bp->phy_dev;
> > +	np1 = of_get_next_child(np, NULL);
> > +	for_each_child_of_node(np1, np11) {
> > +		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
> 
> This would definitively need documentation and at least link in the macb binding
> to show how this emulated phy connect to the mdio bus...
> 
> I'm not able to judge if the node arrangement is okay: I let Florian tell his view
> on this...

Ok will document in the macb binding doc.

> 
> > +			bp->converter_phy.dev = dev;
> > +			bp->converter_phy.mii_bus = bp->mii_bus;
> > +			bp->converter_phy.mdio_write = macb_mdio_write;
> > +			bp->converter_phy.platform_data = bp->pdev-
> >dev.of_node;
> > +			gmii2rgmii_phyprobe(&bp->converter_phy);
> > +		}
> > +	}
> >
> >  	netif_carrier_off(dev);
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.h
> > b/drivers/net/ethernet/cadence/macb.h
> > index 8a13824..735bce2 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
> > @@ -10,6 +10,8 @@
> >  #ifndef _MACB_H
> >  #define _MACB_H
> >
> > +#include <linux/xilinx_gmii2rgmii.h>
> 
> No, put it in the macb.c.

Ok will fix in v2...

> 
> > +
> >  #define MACB_GREGS_NBR 16
> >  #define MACB_GREGS_VERSION 2
> >  #define MACB_MAX_QUEUES 8
> > @@ -846,6 +848,7 @@ struct macb {
> >  	unsigned int		jumbo_max_len;
> >
> >  	u32			wol;
> > +	struct	gmii2rgmii	converter_phy;
> >  };
> >
> >  static inline bool macb_is_gem(struct macb *bp)
> 
> 
> If Florian and phy guys are okay with the approach, I'm fine with this patch, once
> corrected.

Ok thanks...

Regards,
Kedar.

> 
> Thanks, bye,
> --
> Nicolas Ferre

Re: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Nicolas Ferre]

Le 01/07/2016 11:02, Appana Durga Kedareswara Rao a écrit :
> Hi Nicolas Ferre,
> 	
> 	Thanks for the quick review...
> 
>>
>> Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
>>> This patch adds support for gmii2rgmii phy converter in the macb
>>> driver.
>>
>> Okay, I'd like more explanation here.
>> Hints & key words:
>> - dt property
>> - mdio bus
>> - mac speed
> 
> Sure will fix in the next version...
> 
>>
>>
>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>>> ---
>>>  drivers/net/ethernet/cadence/macb.c |   21 ++++++++++++++++++++-
>>>  drivers/net/ethernet/cadence/macb.h |    3 +++
>>>  2 files changed, 23 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.c
>>> b/drivers/net/ethernet/cadence/macb.c
>>> index cb07d95..de0ad71 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
>> mii_id, int regnum,
>>>  	return 0;
>>>  }
>>>
>>> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
>>> +					 struct phy_device *phydev)
>>> +{
>>> +	if (likely(bp->converter_phy.fix_mac_speed))
>>
>> What is the purpose of this branch bias? The code isn't in some hot path, so I
>> suspect that its not needed.
> 
> If we won't put this check driver will crash with NULL pointer dereference for the below cases

I know that...

> ---> Converter driver is disabled
> ---> Converter driver is enabled but the converter probe is not called from the macb driver.

I didn't make myself clear: It's not the branch itself that I'm talking
about it's the branch profiling directive "likely()" that seems not
necessary.

>>> +		bp->converter_phy.fix_mac_speed(&bp->converter_phy,
>>> +						phydev->speed);
>>> +}
>>> +
>>>  /**
>>>   * macb_set_tx_clk() - Set a clock to a new frequency
>>>   * @clk		Pointer to the clock to change
>>> @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct
>> net_device *dev)
>>>  				reg |= GEM_BIT(GBE);
>>>
>>>  			macb_or_gem_writel(bp, NCFGR, reg);
>>> +			macb_hw_fix_mac_speed(bp, phydev);
>>>
>>>  			bp->speed = phydev->speed;
>>>  			bp->duplex = phydev->duplex;
>>> @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device
>> *pdev)
>>>  			struct clk **, struct clk **)
>>>  					      = macb_clk_init;
>>>  	int (*init)(struct platform_device *) = macb_init;
>>> -	struct device_node *np = pdev->dev.of_node;
>>> +	struct device_node *np = pdev->dev.of_node, *np1, *np11;
>>
>> Nitpicking:
>> Be more explicit on variable names. Simple name for the iterator is okay, the
>> other is better if changed.
>> I also like to see variable on separated lines.
> 
> Ok sure will fix in the next version...
> 
>>
>>>  	struct device_node *phy_node;
>>>  	const struct macb_config *macb_config = NULL;
>>>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16
>>> @@ static int macb_probe(struct platform_device *pdev)
>>>  		goto err_out_free_netdev;
>>>
>>>  	phydev = bp->phy_dev;
>>> +	np1 = of_get_next_child(np, NULL);
>>> +	for_each_child_of_node(np1, np11) {
>>> +		if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
>>
>> This would definitively need documentation and at least link in the macb binding
>> to show how this emulated phy connect to the mdio bus...
>>
>> I'm not able to judge if the node arrangement is okay: I let Florian tell his view
>> on this...
> 
> Ok will document in the macb binding doc.
> 
>>
>>> +			bp->converter_phy.dev = dev;
>>> +			bp->converter_phy.mii_bus = bp->mii_bus;
>>> +			bp->converter_phy.mdio_write = macb_mdio_write;
>>> +			bp->converter_phy.platform_data = bp->pdev-
>>> dev.of_node;
>>> +			gmii2rgmii_phyprobe(&bp->converter_phy);
>>> +		}
>>> +	}
>>>
>>>  	netif_carrier_off(dev);
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.h
>>> b/drivers/net/ethernet/cadence/macb.h
>>> index 8a13824..735bce2 100644
>>> --- a/drivers/net/ethernet/cadence/macb.h
>>> +++ b/drivers/net/ethernet/cadence/macb.h
>>> @@ -10,6 +10,8 @@
>>>  #ifndef _MACB_H
>>>  #define _MACB_H
>>>
>>> +#include <linux/xilinx_gmii2rgmii.h>
>>
>> No, put it in the macb.c.
> 
> Ok will fix in v2...
> 
>>
>>> +
>>>  #define MACB_GREGS_NBR 16
>>>  #define MACB_GREGS_VERSION 2
>>>  #define MACB_MAX_QUEUES 8
>>> @@ -846,6 +848,7 @@ struct macb {
>>>  	unsigned int		jumbo_max_len;
>>>
>>>  	u32			wol;
>>> +	struct	gmii2rgmii	converter_phy;
>>>  };
>>>
>>>  static inline bool macb_is_gem(struct macb *bp)
>>
>>
>> If Florian and phy guys are okay with the approach, I'm fine with this patch, once
>> corrected.
> 
> Ok thanks...
> 
> Regards,
> Kedar.
> 
>>
>> Thanks, bye,
>> --
>> Nicolas Ferre
> 


-- 
Nicolas Ferre

RE: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Appana Durga Kedareswara Rao]

Hi,

> >>> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
> >>> +					 struct phy_device *phydev)
> >>> +{
> >>> +	if (likely(bp->converter_phy.fix_mac_speed))
> >>
> >> What is the purpose of this branch bias? The code isn't in some hot
> >> path, so I suspect that its not needed.
> >
> > If we won't put this check driver will crash with NULL pointer
> > dereference for the below cases
> 
> I know that...
> 
> > ---> Converter driver is disabled
> > ---> Converter driver is enabled but the converter probe is not called from the
> macb driver.
> 
> I didn't make myself clear: It's not the branch itself that I'm talking about it's the
> branch profiling directive "likely()" that seems not necessary.

Ok will remove in the next version...

Regards,
Kedar.

Re: [RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[Florian Fainelli]

Le 30/06/2016 23:20, Kedareswara rao Appana a écrit :
> This patch series does the following
> ---> Add support for gmii2rgmii converter.
> ---> Add support for gmii2rgmii converter in the macb driver.
> 
> Earlier sent one RFC patch https://patchwork.kernel.org/patch/9186615/ 
> which includes converter related stuff also in macb driver.
> This patch series fixes this issue.

This still seems very adhoc and not completely explained. Can you
clarify how the gmmi2rgmii converter gets used?

Is the expectation that a MACB Ethernet adapter will be connected to a
RGMII PHY like this:

MACB <=> GMII2RGMII <=> RGMII PHY
MACB MDIO <===========> RGMII_PHY

and still have the ability to control both the PHY device's MDIO
registers and the GMII2RGMII converter and we need to make sure both
have matching settings, or is it something like this:

MACB <=> GMII2RGMII <=> RGMII PHY
MACB MDIO unconnected

and we lose the ability to query the PHY via MDIO?

As Nicolas pointed out, providing a binding documentation update may
help reviewers understand what is being accomplished here.

Thanks!

> 
> Kedareswara rao Appana (2):
>   net: ethernet: xilinx: Add gmii2rgmii converter support
>   net: macb: Add gmii2rgmii phy converter support
> 
>  drivers/net/ethernet/cadence/macb.c             |   21 ++++++-
>  drivers/net/ethernet/cadence/macb.h             |    3 +
>  drivers/net/ethernet/xilinx/Kconfig             |    7 ++
>  drivers/net/ethernet/xilinx/Makefile            |    1 +
>  drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c |   76 +++++++++++++++++++++++
>  include/linux/xilinx_gmii2rgmii.h               |   24 +++++++
>  6 files changed, 131 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
>  create mode 100644 include/linux/xilinx_gmii2rgmii.h
> 


-- 
Florian

RE: [RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[Appana Durga Kedareswara Rao]

Hi Florian,

	Thanks for the review...

> Le 30/06/2016 23:20, Kedareswara rao Appana a écrit :
> > This patch series does the following
> > ---> Add support for gmii2rgmii converter.
> > ---> Add support for gmii2rgmii converter in the macb driver.
> >
> > Earlier sent one RFC patch https://patchwork.kernel.org/patch/9186615/
> > which includes converter related stuff also in macb driver.
> > This patch series fixes this issue.
> 
> This still seems very adhoc and not completely explained. Can you clarify how
> the gmmi2rgmii converter gets used?

Sorry I should have been explained it clearly in the patch.
Will fix it in the v2.

This converter works like below

MACB <==> GMII2RGMII <==> RGMII_PHY

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

Using MACB 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. 

MDIO interface is used to set operating speed (line speed) 

The converter has only one register (0x10) that we need to program to set the operating speed.
The composition of this register is similar to the IEEE standard 802.3 MDIO control register 0x0.

Please let me know if you still need not clear with how this converter works.

IP data sheet is available here @ http://www.xilinx.com/support/documentation/ip_documentation/gmii_to_rgmii/v3_0/pg160-gmii-to-rgmii.pdf 

		   
> 
> Is the expectation that a MACB Ethernet adapter will be connected to a RGMII
> PHY like this:
> 
> MACB <=> GMII2RGMII <=> RGMII PHY
> MACB MDIO <===========> RGMII_PHY
> 
> and still have the ability to control both the PHY device's MDIO registers and the
> GMII2RGMII converter and we need to make sure both have matching settings,
> or is it something like this:
> 
> MACB <=> GMII2RGMII <=> RGMII PHY
> MACB MDIO unconnected
> 
> and we lose the ability to query the PHY via MDIO?
> 
> As Nicolas pointed out, providing a binding documentation update may help
> reviewers understand what is being accomplished here.

Sure will fix in v2.

Regards,
Kedar.

Re: [RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[Andrew Lunn]

On Fri, Jul 01, 2016 at 11:50:10AM +0530, Kedareswara rao Appana wrote:
> This patch series does the following
> ---> Add support for gmii2rgmii converter.

How generic is this gmii2rgmii IP block? Could it be used with any
GMII and RGMII device?

Should it be placed in drivers/net/phy, so making it easier to reuse?

Also, Russell Kings phylink might be a more natural structure for
this. It is hard to know when those patches will land, but it might be
worth looking at.

       Andrew

RE: [RFC PATCH 0/2] net: ethernet: Add support for gmii2rgmii converter

[Appana Durga Kedareswara Rao]

Hi Andrew,

> On Fri, Jul 01, 2016 at 11:50:10AM +0530, Kedareswara rao Appana wrote:
> > This patch series does the following
> > ---> Add support for gmii2rgmii converter.
> 
> How generic is this gmii2rgmii IP block? Could it be used with any GMII and
> RGMII device?

This converter does GMII2RGMII conversion.
This can be used with any MAC which has shared MDIO with external PHY
And this Converter. This Converter IP is validated for MACB.
But it should work with any MAC which has shared MDIO bus (I mean single MDIO multiple PHYS)...

This converter works like below

MACB <==> GMII2RGMII <==> RGMII_PHY

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

Using MACB 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. 

MDIO interface is used to set operating speed (line speed) 

The converter has only one register (0x10) that we need to program to set the operating speed.
The composition of this register is similar to the IEEE standard 802.3 MDIO control register 0x0.

Please let me know if you still not clear about how this converter works.

> 
> Should it be placed in drivers/net/phy, so making it easier to reuse?

Ok will move it drivers/net/phy folder in the next version...

> 
> Also, Russell Kings phylink might be a more natural structure for this. It is hard to
> know when those patches will land, but it might be worth looking at.

Ok sure will take a look at this series once posted...

Regards,
Kedar.

> 
>        Andrew