[PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[Sascha Hauer]

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

Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[Andreas Schmidt]

[-- 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

Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[Sascha Hauer]

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

Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[list]

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

Re: [PATCH] net: cpsw: remove unnecessary mdiobus_scan()

[Sascha Hauer]

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