[U-Boot] [PATCH 2/2] net: phy: Add gmiitorgmii converter support
Joe Hershberger
joe.hershberger at ni.com
Thu Jan 24 17:24:19 UTC 2019
On Thu, Jan 24, 2019 at 2:07 AM Michal Simek <michal.simek at xilinx.com> wrote:
>
> 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 at 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 at xilinx.com>
> >> Signed-off-by: Michal Simek <michal.simek at 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?
They are managed by 2 digital lines on EMIO
>
> >
> >> 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
That's very unfortunate. Is it not something you can have added to the IP?
> >
> >> 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.
A number of our boards have more than one, but they are not yet ported
to modern U-Boot.
> 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 think the inconsistency is a large part of why we can't have a
generic MDIO or ENET PHY driver class enumeration in U-Boot.
> 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.
Sure.
> >> + 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
We have exactly this example on one of our Zynq 7000 boards, and I
expect to be able to describe that.
> are not supported by u-boot. At least I didn't see that code for that
> some months ago. Or is it supported now?
I have not yet tried it to know what is lacking to support it.
> >
> >> + 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.
OK, so the IP configuration and the DTS just need to match.
> >
> >> + 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>;
> + };
> +};
> +
That looks reasonable.
> We can extend that binding in the kernel if it is not clear.
> Just let me know.
It would be good to have the binding include the relationship to the
gem as well.
> >
> >> + 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list