* [PATCH] net: cpsw: remove unnecessary mdiobus_scan() @ 2018-02-16 7:39 Sascha Hauer 2018-02-17 9:18 ` Andreas Schmidt 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2018-02-16 7:39 UTC (permalink / raw) To: Barebox List; +Cc: Andreas Schmidt No need to call mdiobus_scan() manually. it is called from phy_device_connect() already in cpsw_open() which does all the magic to connect a network device with its phy. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/cpsw.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c index d11ca33f70..54ee5f327a 100644 --- a/drivers/net/cpsw.c +++ b/drivers/net/cpsw.c @@ -913,25 +913,13 @@ static int cpsw_slave_setup(struct cpsw_slave *slave, int slave_num, struct eth_device *edev = &slave->edev; struct device_d *dev = &slave->dev; int ret; - struct phy_device *phy; - - phy = mdiobus_scan(&priv->miibus, priv->slaves[slave_num].phy_id); - if (IS_ERR(phy)) { - ret = PTR_ERR(phy); - goto err_out; - } - - phy->dev.device_node = priv->slaves[slave_num].dev.device_node; - ret = phy_register_device(phy); - if (ret) - goto err_out; sprintf(dev->name, "cpsw-slave"); dev->id = slave->slave_num; dev->parent = priv->dev; ret = register_device(dev); if (ret) - goto err_register_dev; + return ret; dev_dbg(&slave->dev, "* %s\n", __func__); @@ -956,11 +944,9 @@ static int cpsw_slave_setup(struct cpsw_slave *slave, int slave_num, return 0; -err_register_dev: - phy_unregister_device(phy); err_register_edev: unregister_device(dev); -err_out: + slave->slave_num = -1; return ret; -- 2.15.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan() 2018-02-16 7:39 [PATCH] net: cpsw: remove unnecessary mdiobus_scan() Sascha Hauer @ 2018-02-17 9:18 ` Andreas Schmidt 2018-02-19 6:54 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Andreas Schmidt @ 2018-02-17 9:18 UTC (permalink / raw) To: barebox [-- Attachment #1.1.1.1: Type: text/plain, Size: 1246 bytes --] Hi Sascha, On 16.02.2018 08:39, Sascha Hauer wrote: > No need to call mdiobus_scan() manually. it is called from > phy_device_connect() already in cpsw_open() which does all > the magic to connect a network device with its phy. You added call of mdiobus_scan to check if the slave has valid phy, I guess. Or I misunderstood your commit: b2568de82d62c18fd5dc8affb0e4dc050403c498 net: cpsw: ignore error on slave setup ? I guess it should work in follow: If slave has a valid phy (determined by mdiobus_scan), slave will be continue to register (call eth_register), if not, cpsw_clave_setup function will exit with an error and next slave will be try to register. In case of a real phy (not fixed-link), I guess, it make sense to check is phy work and exists. If we not check phy, cpsw register eth device with maybe invalid phy. (maybe never added in oftree) Later, while calling cpsw_open, it maybe will be failed, because phy not exists. IHMO, it is a complex way checking if phy exists and is valid, but I know no easier way to do that. If I'm wrong and this part not needed anymore, this would make my patch ([PATCH] net: cpsw: fix probe with fixed-link) shorter and easier :) [...] Regards, Andreas [-- Attachment #1.1.1.2: 0xBEA6DEA0.asc --] [-- Type: application/pgp-keys, Size: 3133 bytes --] [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 801 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan() 2018-02-17 9:18 ` Andreas Schmidt @ 2018-02-19 6:54 ` Sascha Hauer 2018-02-19 11:08 ` list 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2018-02-19 6:54 UTC (permalink / raw) To: Andreas Schmidt; +Cc: barebox On Sat, Feb 17, 2018 at 10:18:16AM +0100, Andreas Schmidt wrote: > Hi Sascha, > > > On 16.02.2018 08:39, Sascha Hauer wrote: > > No need to call mdiobus_scan() manually. it is called from > > phy_device_connect() already in cpsw_open() which does all > > the magic to connect a network device with its phy. > You added call of mdiobus_scan to check if the slave has valid phy, I guess. > Or I misunderstood your commit: b2568de82d62c18fd5dc8affb0e4dc050403c498 > net: cpsw: ignore error on slave setup ? > > I guess it should work in follow: > If slave has a valid phy (determined by mdiobus_scan), slave will be > continue to register (call eth_register), > if not, cpsw_clave_setup function will exit with an error and next slave > will be try to register. You're right. I have overlooked the case that we do not want to register ethernet devices for slaves which do not have a valid phy. In this case, how about the following variant? Sascha -----------------------------8<------------------------------------ From 44e7a94624f920188f55ca3fb45e9b74ba10e0c9 Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Mon, 19 Feb 2018 07:50:56 +0100 Subject: [PATCH] net: cpsw: Call phy_device_connect() earlier We only want to register a slave when a valid phy is available. Instead of manually calling mdiobus_scan() and phy_register_device() we can let this do from phy_device_connect() which also works for fixed phys. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/cpsw.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c index d11ca33f70..70e2721dd0 100644 --- a/drivers/net/cpsw.c +++ b/drivers/net/cpsw.c @@ -761,11 +761,6 @@ static int cpsw_open(struct eth_device *edev) dev_dbg(&slave->dev, "* %s\n", __func__); - ret = phy_device_connect(edev, &priv->miibus, slave->phy_id, - cpsw_adjust_link, 0, slave->phy_if); - if (ret) - return ret; - /* soft reset the controller and initialize priv */ soft_reset(priv, &priv->regs->soft_reset); @@ -913,16 +908,9 @@ static int cpsw_slave_setup(struct cpsw_slave *slave, int slave_num, struct eth_device *edev = &slave->edev; struct device_d *dev = &slave->dev; int ret; - struct phy_device *phy; - phy = mdiobus_scan(&priv->miibus, priv->slaves[slave_num].phy_id); - if (IS_ERR(phy)) { - ret = PTR_ERR(phy); - goto err_out; - } - - phy->dev.device_node = priv->slaves[slave_num].dev.device_node; - ret = phy_register_device(phy); + ret = phy_device_connect(edev, &priv->miibus, slave->phy_id, + cpsw_adjust_link, 0, slave->phy_if); if (ret) goto err_out; @@ -957,7 +945,7 @@ static int cpsw_slave_setup(struct cpsw_slave *slave, int slave_num, return 0; err_register_dev: - phy_unregister_device(phy); + phy_unregister_device(edev->phydev); err_register_edev: unregister_device(dev); err_out: -- 2.15.1 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan() 2018-02-19 6:54 ` Sascha Hauer @ 2018-02-19 11:08 ` list 2018-02-22 7:17 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: list @ 2018-02-19 11:08 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On 2018-02-19 07:54, Sascha Hauer wrote: > On Sat, Feb 17, 2018 at 10:18:16AM +0100, Andreas Schmidt wrote: [...] >> On 16.02.2018 08:39, Sascha Hauer wrote: >> > No need to call mdiobus_scan() manually. it is called from >> > phy_device_connect() already in cpsw_open() which does all >> > the magic to connect a network device with its phy. >> You added call of mdiobus_scan to check if the slave has valid phy, I >> guess. >> Or I misunderstood your commit: >> b2568de82d62c18fd5dc8affb0e4dc050403c498 >> net: cpsw: ignore error on slave setup ? >> >> I guess it should work in follow: >> If slave has a valid phy (determined by mdiobus_scan), slave will be >> continue to register (call eth_register), >> if not, cpsw_clave_setup function will exit with an error and next >> slave >> will be try to register. > > You're right. I have overlooked the case that we do not want to > register > ethernet devices for slaves which do not have a valid phy. > > In this case, how about the following variant? Yes, it's better. I tested it with fixed-link and with real phy and it works. Except a little bug, see comments below. Thanks! > > > Sascha > > -----------------------------8<------------------------------------ > > From 44e7a94624f920188f55ca3fb45e9b74ba10e0c9 Mon Sep 17 00:00:00 2001 > From: Sascha Hauer <s.hauer@pengutronix.de> > Date: Mon, 19 Feb 2018 07:50:56 +0100 > Subject: [PATCH] net: cpsw: Call phy_device_connect() earlier > > We only want to register a slave when a valid phy is available. > Instead of manually calling mdiobus_scan() and phy_register_device() > we can let this do from phy_device_connect() which also works for > fixed phys. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/net/cpsw.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c > index d11ca33f70..70e2721dd0 100644 > --- a/drivers/net/cpsw.c > +++ b/drivers/net/cpsw.c > @@ -761,11 +761,6 @@ static int cpsw_open(struct eth_device *edev) > > dev_dbg(&slave->dev, "* %s\n", __func__); > > - ret = phy_device_connect(edev, &priv->miibus, slave->phy_id, > - cpsw_adjust_link, 0, slave->phy_if); > - if (ret) > - return ret; > - > /* soft reset the controller and initialize priv */ > soft_reset(priv, &priv->regs->soft_reset); > > @@ -913,16 +908,9 @@ static int cpsw_slave_setup(struct cpsw_slave > *slave, int slave_num, > struct eth_device *edev = &slave->edev; > struct device_d *dev = &slave->dev; > int ret; > - struct phy_device *phy; > > - phy = mdiobus_scan(&priv->miibus, priv->slaves[slave_num].phy_id); > - if (IS_ERR(phy)) { > - ret = PTR_ERR(phy); > - goto err_out; > - } > - > - phy->dev.device_node = priv->slaves[slave_num].dev.device_node; > - ret = phy_register_device(phy); Add "edev->parent = dev;" before calling phy_device_connect. It needed to find device_node (see phy.c line about 333 in of_mdio_find_phy function). (Remove instruction "edev->parent = dev;" below in cpsw_slave_setup to avoid to has it twice in the function). > + ret = phy_device_connect(edev, &priv->miibus, slave->phy_id, > + cpsw_adjust_link, 0, slave->phy_if); > if (ret) > goto err_out; > > @@ -957,7 +945,7 @@ static int cpsw_slave_setup(struct cpsw_slave > *slave, int slave_num, > return 0; > > err_register_dev: > - phy_unregister_device(phy); > + phy_unregister_device(edev->phydev); > err_register_edev: > unregister_device(dev); > err_out: > -- > 2.15.1 Regards, Andreas _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan() 2018-02-19 11:08 ` list @ 2018-02-22 7:17 ` Sascha Hauer 0 siblings, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2018-02-22 7:17 UTC (permalink / raw) To: list; +Cc: barebox On Mon, Feb 19, 2018 at 12:08:34PM +0100, list@schmidt-andreas.de wrote: > On 2018-02-19 07:54, Sascha Hauer wrote: > > On Sat, Feb 17, 2018 at 10:18:16AM +0100, Andreas Schmidt wrote: > [...] > > > On 16.02.2018 08:39, Sascha Hauer wrote: > > > > No need to call mdiobus_scan() manually. it is called from > > > > phy_device_connect() already in cpsw_open() which does all > > > > the magic to connect a network device with its phy. > > > You added call of mdiobus_scan to check if the slave has valid phy, > > > I guess. > > > Or I misunderstood your commit: > > > b2568de82d62c18fd5dc8affb0e4dc050403c498 > > > net: cpsw: ignore error on slave setup ? > > > > > > I guess it should work in follow: > > > If slave has a valid phy (determined by mdiobus_scan), slave will be > > > continue to register (call eth_register), > > > if not, cpsw_clave_setup function will exit with an error and next > > > slave > > > will be try to register. > > > > You're right. I have overlooked the case that we do not want to register > > ethernet devices for slaves which do not have a valid phy. > > > > In this case, how about the following variant? > Yes, it's better. I tested it with fixed-link and with real phy and it > works. > Except a little bug, see comments below. Ok, applied with your fixup added. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-22 7:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-16 7:39 [PATCH] net: cpsw: remove unnecessary mdiobus_scan() Sascha Hauer 2018-02-17 9:18 ` Andreas Schmidt 2018-02-19 6:54 ` Sascha Hauer 2018-02-19 11:08 ` list 2018-02-22 7:17 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox