From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hershberger Date: Thu, 24 Jan 2019 17:24:19 +0000 Subject: [U-Boot] [PATCH 2/2] net: phy: Add gmiitorgmii converter support In-Reply-To: <600998c9-0fb1-b5b3-f599-be1f2dd178e3@xilinx.com> References: <1543299551-1965-1-git-send-email-siva.durga.paladugu@xilinx.com> <1543299551-1965-2-git-send-email-siva.durga.paladugu@xilinx.com> <600998c9-0fb1-b5b3-f599-be1f2dd178e3@xilinx.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Jan 24, 2019 at 2:07 AM Michal Simek wrote: > > On 23. 01. 19 19:20, Joe Hershberger wrote: > > On Tue, Nov 27, 2018 at 12:20 AM Siva Durga Prasad Paladugu > > 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 > >> Signed-off-by: Michal Simek > >> --- > > > > 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 > >> +#include > >> +#include > >> +#include > >> + > >> +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 > >> > >> #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