[U-Boot] [PATCH 2/2] net: phy: Add gmiitorgmii converter support

[Michal Simek <michal.simek@xilinx.com>]

On 23. 01. 19 19:20, Joe Hershberger wrote:
> On Tue, Nov 27, 2018 at 12:20 AM Siva Durga Prasad Paladugu
> <siva.durga.paladugu@xilinx.com> wrote:
>>
>> This patch adds support for gmiitorgmii converter.
>> This converter sits between the MAC and the external phy
>> MAC <==> GMII2RGMII <==> RGMII_PHY.
>> The ethernet driver probes this bridge and this bridge driver
>> probes real phy driver and invokes the real phy functionalities
>> as requested. This bridge just needs to be configured based on
>> real phy negotiated speed and duplex.
>>
>> Signed-off-by: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
> 
> FYI, NI doesn't use the Xilinx adapter, we have several internal
> adapters... GMII to RGMII, RMII, and I believe GMII (passing through
> EMIO).

ok. How do you manage them?


> 
>>  drivers/net/phy/Kconfig             |   7 +++
>>  drivers/net/phy/Makefile            |   1 +
>>  drivers/net/phy/phy.c               |  41 ++++++++++++++
>>  drivers/net/phy/xilinx_gmii2rgmii.c | 103 ++++++++++++++++++++++++++++++++++++
>>  include/phy.h                       |   6 +++
>>  5 files changed, 158 insertions(+)
>>  create mode 100644 drivers/net/phy/xilinx_gmii2rgmii.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 3dc0822..a68e167 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -217,6 +217,13 @@ config PHY_VITESSE
>>  config PHY_XILINX
>>         bool "Xilinx Ethernet PHYs support"
>>
>> +config PHY_XILINX_GMII2RGMII
>> +       bool "Xilinx GMII to RGMII Ethernet PHYs support"
>> +       help
>> +         This adds support for Xilinx GMII to RGMII IP core. This IP acts
>> +         as bridge between MAC connected over GMII and external phy that
>> +         is connected over RGMII interface.
>> +
>>  config PHY_FIXED
>>         bool "Fixed-Link PHY"
>>         depends on DM_ETH
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 555da83..76b6197 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_PHY_SMSC) += smsc.o
>>  obj-$(CONFIG_PHY_TERANETICS) += teranetics.o
>>  obj-$(CONFIG_PHY_TI) += ti.o
>>  obj-$(CONFIG_PHY_XILINX) += xilinx_phy.o
>> +obj-$(CONFIG_PHY_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>>  obj-$(CONFIG_PHY_VITESSE) += vitesse.o
>>  obj-$(CONFIG_PHY_MSCC) += mscc.o
>>  obj-$(CONFIG_PHY_FIXED) += fixed.o
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 3cb2785..d02c4d8 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -528,6 +528,9 @@ int phy_init(void)
>>  #ifdef CONFIG_PHY_FIXED
>>         phy_fixed_init();
>>  #endif
>> +#ifdef CONFIG_PHY_XILINX_GMII2RGMII
>> +       phy_xilinx_gmii2rgmii_init();
>> +#endif
> 
> I'm a bit surprised this needs to act as a unique type of device and
> not a normal phy driver? When the phy_id is read, what is returned?
> The real phy? Nothing? You change the speed by accessing MDIO?

It is connected on mdio bus but unfortunately there is no phy_id to read
to be able to use that.

https://www.xilinx.com/support/documentation/ip_documentation/gmii_to_rgmii/v4_0/pg160-gmii-to-rgmii.pdf
page 21


> 
>>         return 0;
>>  }
>>
>> @@ -875,6 +878,41 @@ void phy_connect_dev(struct phy_device *phydev, struct eth_device *dev)
>>         debug("%s connected to %s\n", dev->name, phydev->drv->name);
>>  }
>>
>> +#ifdef CONFIG_PHY_XILINX_GMII2RGMII
>> +#ifdef CONFIG_DM_ETH
>> +static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus,
>> +                                                struct udevice *dev,
>> +                                                phy_interface_t interface)
>> +#else
>> +static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus,
>> +                                                struct eth_device *dev,
>> +                                                phy_interface_t interface)
>> +#endif
>> +{
>> +       struct phy_device *phydev = NULL;
>> +       int sn = dev_of_offset(dev);
>> +       int off;
>> +
>> +       while (sn > 0) {
>> +               off = fdt_node_offset_by_compatible(gd->fdt_blob, sn,
>> +                                                   "xlnx,gmii-to-rgmii-1.0");
> 
> This seems to be searching under the eth dev, but at least in the
> "linux/Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt"
> docs, this is found under the mdio node. How is this supposed to
> handle more than one bridge?

I haven't seen design with more than one bridge. It is really just one
brigde which goes to regular phy.

Regarding mdio and DT binding. I haven't seen anywhere written if phys
should be inside mdio node or they should be just child in controller node.

I see some examples where mdio node needs to contain compatibility
string and there is separate driver for that.

In dt spec nothing is written about it and also I haven't seen yaml
checking from Rob.

The key point for example is to show how bridge needs description for
phy on the board.

>> +               if (off > 0) {
>> +                       phydev = phy_device_create(bus,
>> +                                                  off, PHY_GMII2RGMII_ID,
>> +                                                  interface);
>> +                       break;
>> +               }
>> +               if (off == -FDT_ERR_NOTFOUND)
>> +                       sn = fdt_first_subnode(gd->fdt_blob, sn);
>> +               else
>> +                       printf("%s: Error finding compat string:%d\n",
>> +                              __func__, off);
>> +       }
>> +
>> +       return phydev;
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_PHY_FIXED
>>  #ifdef CONFIG_DM_ETH
>>  static struct phy_device *phy_connect_fixed(struct mii_dev *bus,
>> @@ -920,6 +958,9 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
>>  #ifdef CONFIG_PHY_FIXED
>>         phydev = phy_connect_fixed(bus, dev, interface);
>>  #endif
>> +#ifdef CONFIG_PHY_XILINX_GMII2RGMII
>> +       phydev = phy_connect_gmii2rgmii(bus, dev, interface);
>> +#endif
>>
>>         if (!phydev)
>>                 phydev = phy_find_by_mask(bus, 1 << addr, interface);
>> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
>> new file mode 100644
>> index 0000000..aa4ce86
>> --- /dev/null
>> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
>> @@ -0,0 +1,103 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx GMII2RGMII phy driver
>> + *
>> + * Copyright (C) 2018 Xilinx, Inc.
>> + */
>> +
>> +#include <dm.h>
>> +#include <phy.h>
>> +#include <config.h>
>> +#include <common.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#define ZYNQ_GMII2RGMII_REG            0x10
>> +#define ZYNQ_GMII2RGMII_SPEED_MASK     (BMCR_SPEED1000 | BMCR_SPEED100)
>> +
>> +static int xilinxgmiitorgmii_config(struct phy_device *phydev)
>> +{
>> +       struct phy_device *ext_phydev = phydev->priv;
>> +
>> +       debug("%s\n", __func__);
>> +       if (ext_phydev->drv->config)
>> +               ext_phydev->drv->config(ext_phydev);
>> +
>> +       return 0;
>> +}
>> +
>> +static int xilinxgmiitorgmii_startup(struct phy_device *phydev)
>> +{
>> +       u16 val = 0;
>> +       struct phy_device *ext_phydev = phydev->priv;
>> +
>> +       debug("%s\n", __func__);
>> +       ext_phydev->dev = phydev->dev;
>> +       if (ext_phydev->drv->startup)
>> +               ext_phydev->drv->startup(ext_phydev);
>> +
>> +       val = phy_read(phydev, phydev->addr, ZYNQ_GMII2RGMII_REG);
> 
> This seems to imply that the bridge IP is actually on MDIO at a
> separate address from the real phy, but the same MDIO bus.

yes. They are both on the same bus and that's how it is designed. I was
looking in past if there is a code in u-boot for handling several phys
on the same bus and this is not in place.

That's why even cases like this
eth0 -> mdio -> phy_for_eth0, phy_for_eth1
eth1 without mdio bus

are not supported by u-boot. At least I didn't see that code for that
some months ago. Or is it supported now?


> 
>> +       val &= ~ZYNQ_GMII2RGMII_SPEED_MASK;
>> +
>> +       if (ext_phydev->speed == SPEED_1000)
>> +               val |= BMCR_SPEED1000;
>> +       else if (ext_phydev->speed == SPEED_100)
>> +               val |= BMCR_SPEED100;
>> +
>> +       phy_write(phydev, phydev->addr, ZYNQ_GMII2RGMII_REG, val |
>> +                 BMCR_FULLDPLX);
>> +
>> +       phydev->duplex = ext_phydev->duplex;
>> +       phydev->speed = ext_phydev->speed;
>> +       phydev->link = ext_phydev->link;
>> +
>> +       return 0;
>> +}
>> +
>> +static int xilinxgmiitorgmii_probe(struct phy_device *phydev)
>> +{
>> +       int ofnode = phydev->addr;
>> +       u32 phy_of_handle;
>> +       int ext_phyaddr = -1;
>> +
>> +       debug("%s\n", __func__);
>> +
>> +       /*
>> +        * Read the phy address again as the one we read in ethernet driver
>> +        * was overwritten for the purpose of storing the ofnode
>> +        */
>> +       phydev->addr = fdtdec_get_int(gd->fdt_blob, ofnode, "reg", -1);
> 
> Is this phyaddr made up? Or is this IP actually listening on a
> specific address on MDIO?

this IP is in xilinx catalog and you can setup whatever number you want.

> 
>> +       phy_of_handle = fdtdec_lookup_phandle(gd->fdt_blob, ofnode,
>> +                                             "phy-handle");
>> +       if (phy_of_handle > 0)
>> +               ext_phyaddr = fdtdec_get_int(gd->fdt_blob,
>> +                                            phy_of_handle,
>> +                                            "reg", -1);
> 
> I'd like to see a xilinx_gmii2rgmii.txt added to describe how the
> device tree is expected to be written. The one in Linux seems a bit
> incomplete, also. For instance, does the phy-handle in the ethmac node
> point to the bridge? Is it just missing? No mention.

I have sent you wiring for zc1275-revB

Here is that fragment. Phy in root points to bridge. Bridge points to
phy on the board.

+&gem1 {
+       status = "okay";
+       phy-mode = "gmii";
+       phy-handle = <&gmiitorgmii>;
+       phy: ethernet-phy at 0 {
+               reg = <0x0>;
+       };
+       gmiitorgmii: gmiitorgmii at 8 {
+               compatible = "xlnx,gmii-to-rgmii-1.0";
+               reg = <8>;
+               phy-handle = <&phy>;
+       };
+};
+

We can extend that binding in the kernel if it is not clear.
Just let me know.

> 
>> +       phydev->priv = phy_find_by_mask(phydev->bus,
>> +                                       1 << ext_phyaddr,
>> +                                       phydev->interface);
>> +
>> +       debug("%s, gmii2rgmmi:0x%x, extphy:0x%x\n", __func__, phydev->addr,
>> +             ext_phyaddr);
>> +
>> +       phydev->flags |= PHY_FLAG_BROKEN_RESET;
>> +
>> +       return 0;
>> +}
>> +
>> +static struct phy_driver gmii2rgmii_driver = {
>> +       .name = "XILINX GMII2RGMII",
>> +       .uid = PHY_GMII2RGMII_ID,
>> +       .mask = 0xffffffff,
>> +       .features = PHY_GBIT_FEATURES,
>> +       .probe = xilinxgmiitorgmii_probe,
>> +       .config = xilinxgmiitorgmii_config,
>> +       .startup = xilinxgmiitorgmii_startup,
>> +};
>> +
>> +int phy_xilinx_gmii2rgmii_init(void)
>> +{
>> +       phy_register(&gmii2rgmii_driver);
>> +
>> +       return 0;
>> +}
>> diff --git a/include/phy.h b/include/phy.h
>> index b86fdfb..0485701 100644
>> --- a/include/phy.h
>> +++ b/include/phy.h
>> @@ -17,6 +17,11 @@
>>  #include <phy_interface.h>
>>
>>  #define PHY_FIXED_ID           0xa5a55a5a
>> +/*
>> + * There is no actual id for this.
>> + * This is just a dummy id for gmii2rgmmi converter.
>> + */
>> +#define PHY_GMII2RGMII_ID      0x5a5a5a5a
>>
>>  #define PHY_MAX_ADDR 32
>>
>> @@ -240,6 +245,7 @@ int phy_vitesse_init(void);
>>  int phy_xilinx_init(void);
>>  int phy_mscc_init(void);
>>  int phy_fixed_init(void);
>> +int phy_xilinx_gmii2rgmii_init(void);
>>
>>  int board_phy_config(struct phy_device *phydev);
>>  int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot


Thanks,
Michal