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

2016-07-04 Thread Appana Durga Kedareswara Rao 
Hi Nicolas,


> >
> > #ifdef CONFIG_NET_VENDOR_XILINX
> 
> You may need to have:
> #if defined(CONFIG_NET_VENDOR_XILINX) &&
> defined(CONFIG_XILINX_GMII2RGMII)
> 
> >  extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy); #else extern
> > void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
> 
> No need for the line above...
> 
> > void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) { }
> 
> On one single line, like:
> 
> static inline void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) { }
> 
> > #endif
> > For me the changes are looking odd...
> 
> For me, it's okay...

Ok will fix as you suggested...

> 
> >
> > Other possible ways
> > 1)  Put a config check around phyprobe api in the macb driver.
> > #ifdef CONFIG_XILINX_GMII2RGMII
> >gmii2rgmii_phyprobe(>converter_phy);
> > #endif
> 
> Nope!
> 
> > 2) Select NET_VENDOR_XILINX in the macb Kconfig @ -22,6 +22,7 @@
> > config MACB
> > tristate "Cadence MACB/GEM support"
> > depends on HAS_DMA
> > select PHYLIB
> > +   select NET_VENDOR_XILINX
> 
> 
> 
> > Please let me know which one you prefer will fix that and will post v3...
> 
> First one with my changes is the best. But maybe wait for more feedback...

Ok sure will wait and will post next version with your suggestion in case of no 
comments...

Regards,
Kedar.

> 
> Bye,
> --
> Nicolas Ferre

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

2016-07-04 Thread Nicolas Ferre 
Le 04/07/2016 13:47, Appana Durga Kedareswara Rao a écrit :
> Hi Nicolas,
> 
>   Thanks for the review...
> 
>>> diff --git a/include/linux/xilinx_gmii2rgmii.h
>>> b/include/linux/xilinx_gmii2rgmii.h
>>> new file mode 100644
>>> index 000..b328ee7
>>> --- /dev/null
>>> +++ b/include/linux/xilinx_gmii2rgmii.h
>>> @@ -0,0 +1,24 @@
>>
>>
>> Here, header of the file seems needed.
> 
> Sure will fix in the next version...
> 
>>
>>> +#ifndef _GMII2RGMII_H
>>> +#define _GMII2RGMII_H
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define XILINX_GMII2RGMII_FULLDPLX BMCR_FULLDPLX
>>> +#define XILINX_GMII2RGMII_SPEED1000BMCR_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;
>>> +   int (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
>>> + u16 val);
>>> +   void (*fix_mac_speed)(struct gmii2rgmii *xphy, unsigned int speed);
>>> +};
>>> +
>>> +extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy); #endif
>>
>> I see a compilation issue here:
>>
>> You should provide a way to have this function even if the NET_VENDOR_XILINX
>> config option is not selected (test to compile with the sama5_defconfig and
>> you'll see).
> 
> Ok will fix in the next version...
> 
>>
>> What about making this function void in case of !XILINX?
> 
> This is one way to get rid of compilation error. Changes will be look like 
> below
> 
> #ifdef CONFIG_NET_VENDOR_XILINX

You may need to have:
#if defined(CONFIG_NET_VENDOR_XILINX) && defined(CONFIG_XILINX_GMII2RGMII)

>  extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
> #else
> extern void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);

No need for the line above...

> void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
> {
> }

On one single line, like:

static inline void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) { }

> #endif
> For me the changes are looking odd...

For me, it's okay...

> 
> Other possible ways 
>   1)  Put a config check around phyprobe api in the macb driver.
> #ifdef CONFIG_XILINX_GMII2RGMII
>gmii2rgmii_phyprobe(>converter_phy);
> #endif

Nope!

>   2) Select NET_VENDOR_XILINX in the macb Kconfig 
> @ -22,6 +22,7 @@ config MACB
> tristate "Cadence MACB/GEM support"
> depends on HAS_DMA
> select PHYLIB
> +   select NET_VENDOR_XILINX



> Please let me know which one you prefer will fix that and will post v3...

First one with my changes is the best. But maybe wait for more feedback...

Bye,
-- 
Nicolas Ferre

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

2016-07-04 Thread Appana Durga Kedareswara Rao 
Hi Nicolas,

Thanks for the review...

> > diff --git a/include/linux/xilinx_gmii2rgmii.h
> > b/include/linux/xilinx_gmii2rgmii.h
> > new file mode 100644
> > index 000..b328ee7
> > --- /dev/null
> > +++ b/include/linux/xilinx_gmii2rgmii.h
> > @@ -0,0 +1,24 @@
> 
> 
> Here, header of the file seems needed.

Sure will fix in the next version...

> 
> > +#ifndef _GMII2RGMII_H
> > +#define _GMII2RGMII_H
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define XILINX_GMII2RGMII_FULLDPLX BMCR_FULLDPLX
> > +#define XILINX_GMII2RGMII_SPEED1000BMCR_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;
> > +   int (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
> > + u16 val);
> > +   void (*fix_mac_speed)(struct gmii2rgmii *xphy, unsigned int speed);
> > +};
> > +
> > +extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy); #endif
> 
> I see a compilation issue here:
> 
> You should provide a way to have this function even if the NET_VENDOR_XILINX
> config option is not selected (test to compile with the sama5_defconfig and
> you'll see).

Ok will fix in the next version...

> 
> What about making this function void in case of !XILINX?

This is one way to get rid of compilation error. Changes will be look like below

#ifdef CONFIG_NET_VENDOR_XILINX
 extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
#else
extern void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
{
}
#endif
For me the changes are looking odd...

Other possible ways 
1)  Put a config check around phyprobe api in the macb driver.
#ifdef CONFIG_XILINX_GMII2RGMII
   gmii2rgmii_phyprobe(>converter_phy);
#endif
2) Select NET_VENDOR_XILINX in the macb Kconfig 
@ -22,6 +22,7 @@ config MACB
tristate "Cadence MACB/GEM support"
depends on HAS_DMA
select PHYLIB
+   select NET_VENDOR_XILINX

Please let me know which one you prefer will fix that and will post v3...

Regards,
Kedar.

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

2016-07-04 Thread Nicolas Ferre 
Le 04/07/2016 11:04, 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 by configuring the converter register through mdio write.
> 
> MDIO interface is used to set operating speed of Ethernet MAC.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> 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/ethernet/xilinx/Kconfig |  8 +++
>  drivers/net/ethernet/xilinx/Makefile|  1 +
>  drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c | 79 
> +
>  include/linux/xilinx_gmii2rgmii.h   | 24 
>  4 files changed, 112 insertions(+)
>  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..4b65174 100644
> --- a/drivers/net/ethernet/xilinx/Kconfig
> +++ b/drivers/net/ethernet/xilinx/Kconfig
> @@ -39,4 +39,12 @@ 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"
> + default y
> + ---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 000..de3bd89
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
> @@ -0,0 +1,79 @@
> +/* Xilinx GMII2RGMII Converter driver
> + *
> + * Copyright (C) 2016 Xilinx, Inc.
> + *
> + * Author: Kedareswara rao Appana 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static void xgmii2rgmii_fix_mac_speed(struct gmii2rgmii *xphy,
> +   unsigned int speed)
> +{
> + 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 =

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

2016-07-04 Thread 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 by configuring the converter register through mdio write.

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

Signed-off-by: Kedareswara rao Appana 
---
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/ethernet/xilinx/Kconfig |  8 +++
 drivers/net/ethernet/xilinx/Makefile|  1 +
 drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c | 79 +
 include/linux/xilinx_gmii2rgmii.h   | 24 
 4 files changed, 112 insertions(+)
 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..4b65174 100644
--- a/drivers/net/ethernet/xilinx/Kconfig
+++ b/drivers/net/ethernet/xilinx/Kconfig
@@ -39,4 +39,12 @@ 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"
+   default y
+   ---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 000..de3bd89
--- /dev/null
+++ b/drivers/net/ethernet/xilinx/xilinx_gmii2rgmii.c
@@ -0,0 +1,79 @@
+/* Xilinx GMII2RGMII Converter driver
+ *
+ * Copyright (C) 2016 Xilinx, Inc.
+ *
+ * Author: Kedareswara rao Appana 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static void xgmii2rgmii_fix_mac_speed(struct gmii2rgmii *xphy,
+ unsigned int speed)
+{
+   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;
+   } else {
+   xphy->gmii2rgmii_phy_dev = 0;
+   xphy->fix_mac_speed = 0;
+   }
+
+   return 0;
+}