From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CEB0CC433F5 for ; Thu, 31 Mar 2022 17:50:07 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C62198423B; Thu, 31 Mar 2022 19:49:21 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gateworks-com.20210112.gappssmtp.com header.i=@gateworks-com.20210112.gappssmtp.com header.b="ELcHB3/u"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6EAA584239; Thu, 31 Mar 2022 19:49:19 +0200 (CEST) Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E5EBB8426F for ; Thu, 31 Mar 2022 19:49:08 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=tharvey@gateworks.com Received: by mail-pf1-x434.google.com with SMTP id y10so270721pfa.7 for ; Thu, 31 Mar 2022 10:49:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qzHvXzqM/0e9Alx4A3i1ySNjqqFo9FlVGKKxXkMRos0=; b=ELcHB3/u484aScrkw3OQu4I1IpCAgv2XcxoE1l/rSUB901NFQU+l2HF34wJz6LUd1v I8WBFKeqTTZWfR4LXFywUjwoqxbMzaTkQA75QqPsY9PDL45d2BsmpB33vfZXWGoiGier fz0Iru4kQyWK9f+xxo0AT2wP9UKHjqeoZcNfROqDU/sdHI78ZZ33OhZbiydGHgyOT4QN 44R1yvBfX2dk6AoExjJsygxXWOKYD9R4jZ2DspHRdLJcF/XiYymZR1rH17fB9ecKLoL6 uyv0lg747peKPqJpzYOYwATLZ81+z8HW1x72eSV9JOwRdtcXrHA3qWU3wA6l6Nl6LpmT Jrjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qzHvXzqM/0e9Alx4A3i1ySNjqqFo9FlVGKKxXkMRos0=; b=BVNMZuAYINYdgqBc0i4wk7FQIHLdhSePkW8SDk+JK1VL0OphweJm/1ndyrqvc7C1Ms QQIT+oUuyESNvecjeOCeaDAob1MiBKmL2yL1M4vhWLFOTzAE6qUNSXJJU6+M0BjKbn6J NSv861FTKtTCLuIex77QdHPqb7T3pZI5QlcXXdeL0DkAyzgAgOq0jy+IcAMENxxE5fAH 5lI1EKh7ycP6ci+mN9enTWmQ3fyXmH/N5jxA19oCbyl4oiJYy8DmbGYHxzycZl7H2Sw8 25a8Ocq9ohYsin9+Bxin2FudlAmi9xjFsu+2XmWBjiOC5gJx9G2P7KRkZ3PT+UKC7kkP UFDw== X-Gm-Message-State: AOAM53104VuzpXzy0kftpvxUaLc6bBVwLegxC7MYBipWeSEabeLS3HyO 91kcT7cvm4gCJ1vHkniYs2/wpwICSf2iSxto3JPiKA== X-Google-Smtp-Source: ABdhPJw10O/XI7SDMDJR4/UIbV5gxanEIRJ/DnnSDJ6OlxQpiBekPzBjkuokg1MHgLBQCQ4p34MfZHSllfAh2ImtUg8= X-Received: by 2002:a05:6a00:14ca:b0:4fb:5d3e:5f77 with SMTP id w10-20020a056a0014ca00b004fb5d3e5f77mr6751921pfu.34.1648748947255; Thu, 31 Mar 2022 10:49:07 -0700 (PDT) MIME-Version: 1.0 References: <20220329225240.1416-1-tharvey@gateworks.com> <20220329225240.1416-5-tharvey@gateworks.com> <20220331170131.lbl6scb4xj4kl5lv@skbuf> In-Reply-To: <20220331170131.lbl6scb4xj4kl5lv@skbuf> From: Tim Harvey Date: Thu, 31 Mar 2022 10:48:55 -0700 Message-ID: Subject: Re: [PATCH 4/6] net: fec: add support for DM_MDIO To: Vladimir Oltean Cc: Joe Hershberger , Ramon Fried , "u-boot@lists.denx.de" , Stefano Babic , Fabio Estevam , dl-uboot-imx Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Thu, Mar 31, 2022 at 10:01 AM Vladimir Oltean wrote: > > On Tue, Mar 29, 2022 at 03:52:38PM -0700, Tim Harvey wrote: > > Add support for DM_MDIO by registering a UCLASS_MDIO driver and > > attempting to use it. This is necessary if wanting to use a DSA > > driver for example hanging off of the FEC MAC. > > > > Care is taken to fallback to non DM_MDIO as several boards define > > DM_MDIO without having the proper device-tree configuration necessary > > such as an mdio subnode, a phy-mode prop, and either a valid phy-handle > > prop or fixed-phy subnode. > > > > Signed-off-by: Tim Harvey > > --- > > drivers/net/fec_mxc.c | 113 ++++++++++++++++++++++++++++++++++++++---- > > drivers/net/fec_mxc.h | 1 + > > 2 files changed, 104 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > > index e8ebef09032a..374ef9d67d5e 100644 > > --- a/drivers/net/fec_mxc.c > > +++ b/drivers/net/fec_mxc.c > > @@ -30,6 +30,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > > > #include "fec_mxc.h" > > #include > > @@ -1025,6 +1027,80 @@ struct mii_dev *fec_get_miibus(ulong base_addr, int dev_id) > > return bus; > > } > > > > +#ifdef CONFIG_DM_MDIO > > +struct dm_fec_mdio_priv { > > + struct ethernet_regs *regs; > > +}; > > + > > +static int dm_fec_mdio_read(struct udevice *dev, int addr, int devad, int reg) > > +{ > > + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); > > + > > + return fec_mdio_read(priv->regs, addr, reg); > > +} > > + > > +static int dm_fec_mdio_write(struct udevice *dev, int addr, int devad, int reg, u16 data) > > +{ > > + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); > > + > > + return fec_mdio_write(priv->regs, addr, reg, data); > > +} > > + > > +static const struct mdio_ops dm_fec_mdio_ops = { > > + .read = dm_fec_mdio_read, > > + .write = dm_fec_mdio_write, > > +}; > > + > > +static int dm_fec_mdio_probe(struct udevice *dev) > > +{ > > + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); > > + > > + priv->regs = (struct ethernet_regs *)ofnode_get_addr(dev_ofnode(dev->parent)); > > + > > + return 0; > > +} > > + > > +U_BOOT_DRIVER(fec_mdio) = { > > + .name = "fec_mdio", > > + .id = UCLASS_MDIO, > > + .probe = dm_fec_mdio_probe, > > + .ops = &dm_fec_mdio_ops, > > + .priv_auto = sizeof(struct dm_fec_mdio_priv), > > +}; > > + > > +static int dm_fec_bind_mdio(struct udevice *dev) > > +{ > > + struct udevice *mdiodev; > > + const char *name; > > + ofnode mdio; > > + int ret; > > + > > + /* for a UCLASS_MDIO driver we need to bind and probe manually > > + * for an internal MDIO bus that has no dt compatible of its own > > + */ > > + ofnode_for_each_subnode(mdio, dev_ofnode(dev)) { > > + name = ofnode_get_name(mdio); > > + > > + if (strcmp(name, "mdio")) > > + continue; > > + > > + ret = device_bind_driver_to_node(dev, "fec_mdio", > > + name, mdio, &mdiodev); > > + if (ret) > > + return ret; > > + > > + /* need to probe it as there is no compatible to do so */ > > + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdiodev); > > + if (!ret) > > + return 0; > > + > > + break; > > + } > > + > > + return ret; > > +} > > +#endif > > + > > On which branch does this apply? The context above fecmxc_read_rom_hwaddr() > is different in the branches I've checked: > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276 > https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276 > Sorry, I should have specified in my cover letter that this is on top of next which removes the non dm-eth support from the driver which is quite the cleanup. Also, after more testing I believe the dm-mdio driver code 'above' is correct but the business of trying to use it and fallback 'below' is wrong and needs work. It doesn't break current users of fec_mxc from what I can tell but it also doesn't properly connect the dm_mdio driver. > > static int fecmxc_read_rom_hwaddr(struct udevice *dev) > > { > > struct fec_priv *priv = dev_get_priv(dev); > > @@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev) > > > > static int fec_phy_init(struct fec_priv *priv, struct udevice *dev) > > { > > - struct phy_device *phydev; > > + struct phy_device *phydev = NULL; > > int addr; > > > > addr = device_get_phy_addr(priv, dev); > > @@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev) > > addr = CONFIG_FEC_MXC_PHYADDR; > > #endif > > > > - phydev = phy_connect(priv->bus, addr, dev, priv->interface); > > +#ifdef CONFIG_DM_MDIO > > + if (priv->dm_mdio) > > + phydev = dm_eth_phy_connect(dev); > > +#endif > > + if (!phydev) > > + phydev = phy_connect(priv->bus, addr, dev, priv->interface); > > if (!phydev) > > return -ENODEV; > > > > @@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev) > > > > priv->dev_id = dev_seq(dev); > > > > +#ifdef CONFIG_DM_MDIO > > + ret = dm_fec_bind_mdio(dev); > > + if (!ret) { > > + ret = fec_phy_init(priv, dev); > > + if (!ret) > > + priv->dm_mdio = true; > > + } > > +#endif > > #ifdef CONFIG_DM_ETH_PHY > > bus = eth_phy_get_mdio_bus(dev); > > #endif > > > > - if (!bus) { > > + if (!bus && !priv->dm_mdio) { > > dm_mii_bus = false; > > #ifdef CONFIG_FEC_MXC_MDIO_BASE > > bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE, > > @@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev) > > bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev)); > > #endif > > } > > - if (!bus) { > > + if (!bus && !priv->dm_mdio) { > > ret = -ENOMEM; > > goto err_mii; > > } > > @@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev) > > break; > > } > > > > - ret = fec_phy_init(priv, dev); > > - if (ret) > > - goto err_phy; > > + if (!priv->dm_mdio) { > > + ret = fec_phy_init(priv, dev); > > + if (ret) > > + goto err_phy; > > + } > > > > return 0; > > > > err_phy: > > - if (!dm_mii_bus) { > > + if (!dm_mii_bus && !priv->dm_mdio) { > > mdio_unregister(bus); > > free(bus); > > } > > @@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev) > > > > free(priv->phydev); > > fec_free_descs(priv); > > - mdio_unregister(priv->bus); > > - mdio_free(priv->bus); > > + if (priv->bus) { > > + mdio_unregister(priv->bus); > > + mdio_free(priv->bus); > > + } > > > > #ifdef CONFIG_DM_REGULATOR > > if (priv->phy_supply) > > diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h > > index 48faa33d66ec..c297880ccc54 100644 > > --- a/drivers/net/fec_mxc.h > > +++ b/drivers/net/fec_mxc.h > > @@ -248,6 +248,7 @@ struct fec_priv { > > uint8_t *tdb_ptr; > > int dev_id; > > struct mii_dev *bus; > > + bool dm_mdio; > > #ifdef CONFIG_PHYLIB > > struct phy_device *phydev; > > ofnode phy_of_node; > > -- > > 2.17.1 > > What I'm trying to accomplish here vs just simply using dm_fec_bind_mdio(dev) and dm_eth_phy_connect(dev) is to provide a fallback for the many users of fec_mxc that do not have a dt that works with dm-mdio yet still have defconfig's that enable DM_MDIO. Most of the users would not currently have an mdio subnode in the fec node, nor have the required phy-mode 'and' phy-handle prop or fixed-link subnode which would cause the dm_eth_phy_connect to fail. Best Regards, Tim