powerpc, fs_enet: scanning PHY after Linux is up

powerpc, fs_enet: scanning PHY after Linux is up

[Heiko Schocher]

Hello all,

we have on the mgcoge arch/powerpc/boot/dts/mgcoge.dts 3 fs_enet
devices. The first is accessible on boot, and so get correct
probed and works fine. For the other two fs_enet devices the PHYs
are on startup in reset, and gets later, through userapplikations,
out of reset ... so, on bootup, this 2 fs_enet devices could
not detect the PHY in drivers/of/of_mdio.c of_mdiobus_register(),
and if we want to use them later, we get for example:

-bash-3.2# ifconfig eth2 172.31.31.33
net eth2: Could not attach to PHY
SIOCSIFFLAGS: No such device

So the problem is, that we cannot rescan the PHYs, if they are
accessible. Also we could not load the fs_enet driver as a module,
because the first port is used fix.

So, first question which comes in my mind, is:

Is detecting the phy in drivers/of/of_mdio.c of_mdiobus_register()
the right place, or should it not better be done, when really
using the port?

But we found another way to solve this issue:

After the PHYs are out of reset, we just have to rescan the PHYs
with (for example PHY with addr 1)

err = mdiobus_scan(bus, 1);

and

of_find_node_by_path("/soc@f0000000/cpm@119c0/mdio@10d40/ethernet-phy@1");
of_node_get(np);
dev_archdata_set_node(&err->dev.archdata, np);

but thats just a hack ...

So, the question is, is there a possibility to solve this problem?

If there is no standard option, what would be with adding a
"scan_phy" file in

/proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
(or better destination?)

which with we could rescan a PHY with
"echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/scan_phy"
(so there is no need for using of_find_node_by_path(), as we should
 have the associated device node here, and can step through the child
 nodes with "for_each_child_of_node(np, child)" and check if reg == addr)

or shouldn;t be at least, if the phy couldn;t be found when opening
the port, retrigger a scanning, if the phy now is accessible?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Grant Likely]

On Mon, Oct 4, 2010 at 1:32 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello all,
>
> we have on the mgcoge arch/powerpc/boot/dts/mgcoge.dts 3 fs_enet
> devices. The first is accessible on boot, and so get correct
> probed and works fine. For the other two fs_enet devices the PHYs
> are on startup in reset, and gets later, through userapplikations,
> out of reset ... so, on bootup, this 2 fs_enet devices could
> not detect the PHY in drivers/of/of_mdio.c of_mdiobus_register(),
> and if we want to use them later, we get for example:
>
> -bash-3.2# ifconfig eth2 172.31.31.33
> net eth2: Could not attach to PHY
> SIOCSIFFLAGS: No such device
>
> So the problem is, that we cannot rescan the PHYs, if they are
> accessible. Also we could not load the fs_enet driver as a module,
> because the first port is used fix.
>
> So, first question which comes in my mind, is:
>
> Is detecting the phy in drivers/of/of_mdio.c of_mdiobus_register()
> the right place, or should it not better be done, when really
> using the port?
>
> But we found another way to solve this issue:
>
> After the PHYs are out of reset, we just have to rescan the PHYs
> with (for example PHY with addr 1)
>
> err =3D mdiobus_scan(bus, 1);
>
> and
>
> of_find_node_by_path("/soc@f0000000/cpm@119c0/mdio@10d40/ethernet-phy@1")=
;
> of_node_get(np);
> dev_archdata_set_node(&err->dev.archdata, np);
>
> but thats just a hack ...

Yeah, that's a hack.  It really needs to be done via the of_mdio
mechanisms so that dt <--> phy_device linkages remain consistent.

> So, the question is, is there a possibility to solve this problem?
>
> If there is no standard option, what would be with adding a
> "scan_phy" file in
>
> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
> (or better destination?)
>
> which with we could rescan a PHY with
> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/scan_=
phy"
> (so there is no need for using of_find_node_by_path(), as we should
> =A0have the associated device node here, and can step through the child
> =A0nodes with "for_each_child_of_node(np, child)" and check if reg =3D=3D=
 addr)
>
> or shouldn;t be at least, if the phy couldn;t be found when opening
> the port, retrigger a scanning, if the phy now is accessible?

One option would be to still register a phy_device for each phy
described in the device tree, but defer binding a driver to each phy
that doesn't respond.  Then at of_phy_find_device() time, if it
matches with a phy_device that isn't bound to a driver yet, then
re-trigger the binding operation.  At which point the phy id can be
probed and the correct driver can be chosen.  If binding succeeds,
then return the phy_device handle.  If not, then fail as it currently
does.

g.

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Heiko Schocher]

Hello Grant,

Thanks for your answer!

Grant Likely wrote:
> On Mon, Oct 4, 2010 at 1:32 AM, Heiko Schocher <hs@denx.de> wrote:
>> Hello all,
>>
>> we have on the mgcoge arch/powerpc/boot/dts/mgcoge.dts 3 fs_enet
>> devices. The first is accessible on boot, and so get correct
>> probed and works fine. For the other two fs_enet devices the PHYs
>> are on startup in reset, and gets later, through userapplikations,
>> out of reset ... so, on bootup, this 2 fs_enet devices could
>> not detect the PHY in drivers/of/of_mdio.c of_mdiobus_register(),
>> and if we want to use them later, we get for example:
>>
>> -bash-3.2# ifconfig eth2 172.31.31.33
>> net eth2: Could not attach to PHY
>> SIOCSIFFLAGS: No such device
>>
>> So the problem is, that we cannot rescan the PHYs, if they are
>> accessible. Also we could not load the fs_enet driver as a module,
>> because the first port is used fix.
>>
>> So, first question which comes in my mind, is:
>>
>> Is detecting the phy in drivers/of/of_mdio.c of_mdiobus_register()
>> the right place, or should it not better be done, when really
>> using the port?
>>
>> But we found another way to solve this issue:
>>
>> After the PHYs are out of reset, we just have to rescan the PHYs
>> with (for example PHY with addr 1)
>>
>> err = mdiobus_scan(bus, 1);
>>
>> and
>>
>> of_find_node_by_path("/soc@f0000000/cpm@119c0/mdio@10d40/ethernet-phy@1");
>> of_node_get(np);
>> dev_archdata_set_node(&err->dev.archdata, np);
>>
>> but thats just a hack ...
> 
> Yeah, that's a hack.  It really needs to be done via the of_mdio
> mechanisms so that dt <--> phy_device linkages remain consistent.

Yep, I know, thats the reason why I ask ;-)

>> So, the question is, is there a possibility to solve this problem?
>>
>> If there is no standard option, what would be with adding a
>> "scan_phy" file in
>>
>> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
>> (or better destination?)
>>
>> which with we could rescan a PHY with
>> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/scan_phy"
>> (so there is no need for using of_find_node_by_path(), as we should
>>  have the associated device node here, and can step through the child
>>  nodes with "for_each_child_of_node(np, child)" and check if reg == addr)
>>
>> or shouldn;t be at least, if the phy couldn;t be found when opening
>> the port, retrigger a scanning, if the phy now is accessible?
> 
> One option would be to still register a phy_device for each phy
> described in the device tree, but defer binding a driver to each phy
> that doesn't respond.  Then at of_phy_find_device() time, if it

Maybe I din;t get the trick, but the problem is, that
you can;t register a phy_device in drivers/of/of_mdio.c
of_mdiobus_register(), if the phy didn;t respond with the
phy_id ... and of_phy_find_device() is not (yet) used in fs_enet

> matches with a phy_device that isn't bound to a driver yet, then
> re-trigger the binding operation.  At which point the phy id can be
> probed and the correct driver can be chosen.  If binding succeeds,
> then return the phy_device handle.  If not, then fail as it currently
> does.

Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
to look if there is one?

Something like that (not tested):

in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
called from fs_enet_open():

Do first:
phydev =  of_phy_find_device(fep->fpi->phy_node);

Look if there is a driver (phy_dev->drv == NULL ?)

If not, call new function
of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
see below patch for it.

If this succeeds, all is OK, and we can use this phy,
else ethernet not work.

!!just no idea, how to get mii_bus pointer ...

here the patch for the new function of_mdiobus_register_phy():

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index b474833..7afbb0b 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -21,6 +21,51 @@
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");

+int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child)
+{
+	struct phy_device *phy;
+	const __be32 *addr;
+	int len;
+	int rc;
+
+	/* A PHY must have a reg property in the range [0-31] */
+	addr = of_get_property(child, "reg", &len);
+	if (!addr || len < sizeof(*addr) || *addr >= 32 || *addr < 0) {
+		dev_err(&mdio->dev, "%s has invalid PHY address\n",
+			child->full_name);
+		return -1;
+	}
+
+	if (mdio->irq) {
+		mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
+		if (!mdio->irq[*addr])
+			mdio->irq[*addr] = PHY_POLL;
+	}
+
+	phy = get_phy_device(mdio, be32_to_cpup(addr));
+	if (!phy || IS_ERR(phy)) {
+		dev_err(&mdio->dev, "error probing PHY at address %i\n",
+			*addr);
+		return -2;
+	}
+	phy_scan_fixups(phy);
+	/* Associate the OF node with the device structure so it
+	 * can be looked up later */
+	of_node_get(child);
+	dev_archdata_set_node(&phy->dev.archdata, child);
+
+	/* All data is now stored in the phy struct; register it */
+	rc = phy_device_register(phy);
+	if (rc) {
+		phy_device_free(phy);
+		of_node_put(child);
+		return -3;
+	}
+
+	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
+		child->name, *addr);
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -31,7 +76,6 @@ MODULE_LICENSE("GPL");
  */
 int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
-	struct phy_device *phy;
 	struct device_node *child;
 	int rc, i;

@@ -51,46 +95,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)

 	/* Loop over the child nodes and register a phy_device for each one */
 	for_each_child_of_node(np, child) {
-		const __be32 *addr;
-		int len;
-
-		/* A PHY must have a reg property in the range [0-31] */
-		addr = of_get_property(child, "reg", &len);
-		if (!addr || len < sizeof(*addr) || *addr >= 32 || *addr < 0) {
-			dev_err(&mdio->dev, "%s has invalid PHY address\n",
-				child->full_name);
-			continue;
-		}
-
-		if (mdio->irq) {
-			mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
-			if (!mdio->irq[*addr])
-				mdio->irq[*addr] = PHY_POLL;
-		}
-
-		phy = get_phy_device(mdio, be32_to_cpup(addr));
-		if (!phy || IS_ERR(phy)) {
-			dev_err(&mdio->dev, "error probing PHY at address %i\n",
-				*addr);
-			continue;
-		}
-		phy_scan_fixups(phy);
-
-		/* Associate the OF node with the device structure so it
-		 * can be looked up later */
-		of_node_get(child);
-		dev_archdata_set_node(&phy->dev.archdata, child);
-
-		/* All data is now stored in the phy struct; register it */
-		rc = phy_device_register(phy);
-		if (rc) {
-			phy_device_free(phy);
-			of_node_put(child);
-			continue;
-		}
-
-		dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
-			child->name, *addr);
+		of_mdiobus_register_phy(mdio, child);
 	}

 	return 0;

With this change, it would work on boot as actual (phy_device_register()
will fail for the PHYs who don;t work when booting).

Later, when opening the ethernet device, fs_init_phy, will look, if
we have a valid phy with driver, if not we try to register it again.
If this is successfull, we can use the device, if not we will fail,
as now ... what do you think?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Grant Likely]

On Wed, Oct 6, 2010 at 3:53 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Grant,
>
> Thanks for your answer!
>
> Grant Likely wrote:
>> On Mon, Oct 4, 2010 at 1:32 AM, Heiko Schocher <hs@denx.de> wrote:
>>> Hello all,
>>>
>>> we have on the mgcoge arch/powerpc/boot/dts/mgcoge.dts 3 fs_enet
>>> devices. The first is accessible on boot, and so get correct
>>> probed and works fine. For the other two fs_enet devices the PHYs
>>> are on startup in reset, and gets later, through userapplikations,
>>> out of reset ... so, on bootup, this 2 fs_enet devices could
>>> not detect the PHY in drivers/of/of_mdio.c of_mdiobus_register(),
>>> and if we want to use them later, we get for example:
>>>
>>> -bash-3.2# ifconfig eth2 172.31.31.33
>>> net eth2: Could not attach to PHY
>>> SIOCSIFFLAGS: No such device
>>>
>>> So the problem is, that we cannot rescan the PHYs, if they are
>>> accessible. Also we could not load the fs_enet driver as a module,
>>> because the first port is used fix.
>>>
>>> So, first question which comes in my mind, is:
>>>
>>> Is detecting the phy in drivers/of/of_mdio.c of_mdiobus_register()
>>> the right place, or should it not better be done, when really
>>> using the port?
>>>
>>> But we found another way to solve this issue:
>>>
>>> After the PHYs are out of reset, we just have to rescan the PHYs
>>> with (for example PHY with addr 1)
>>>
>>> err =3D mdiobus_scan(bus, 1);
>>>
>>> and
>>>
>>> of_find_node_by_path("/soc@f0000000/cpm@119c0/mdio@10d40/ethernet-phy@1=
");
>>> of_node_get(np);
>>> dev_archdata_set_node(&err->dev.archdata, np);
>>>
>>> but thats just a hack ...
>>
>> Yeah, that's a hack. =A0It really needs to be done via the of_mdio
>> mechanisms so that dt <--> phy_device linkages remain consistent.
>
> Yep, I know, thats the reason why I ask ;-)
>
>>> So, the question is, is there a possibility to solve this problem?
>>>
>>> If there is no standard option, what would be with adding a
>>> "scan_phy" file in
>>>
>>> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
>>> (or better destination?)
>>>
>>> which with we could rescan a PHY with
>>> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/sca=
n_phy"
>>> (so there is no need for using of_find_node_by_path(), as we should
>>> =A0have the associated device node here, and can step through the child
>>> =A0nodes with "for_each_child_of_node(np, child)" and check if reg =3D=
=3D addr)
>>>
>>> or shouldn;t be at least, if the phy couldn;t be found when opening
>>> the port, retrigger a scanning, if the phy now is accessible?
>>
>> One option would be to still register a phy_device for each phy
>> described in the device tree, but defer binding a driver to each phy
>> that doesn't respond. =A0Then at of_phy_find_device() time, if it
>
> Maybe I din;t get the trick, but the problem is, that
> you can;t register a phy_device in drivers/of/of_mdio.c
> of_mdiobus_register(), if the phy didn;t respond with the
> phy_id ... and of_phy_find_device() is not (yet) used in fs_enet

I'm suggesting modifying the phy layer so that it is possible to
register a phy_device that doesn't (yet) respond.

>> matches with a phy_device that isn't bound to a driver yet, then
>> re-trigger the binding operation. =A0At which point the phy id can be
>> probed and the correct driver can be chosen. =A0If binding succeeds,
>> then return the phy_device handle. =A0If not, then fail as it currently
>> does.
>
> Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
> to look if there is one?
>
> Something like that (not tested):
>
> in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
> called from fs_enet_open():
>
> Do first:
> phydev =3D =A0of_phy_find_device(fep->fpi->phy_node);
>
> Look if there is a driver (phy_dev->drv =3D=3D NULL ?)
>
> If not, call new function
> of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
> see below patch for it.
>
> If this succeeds, all is OK, and we can use this phy,
> else ethernet not work.

I don't like this approach because it muddies the concept of which
device is actually responsible for managing the phys on the bus.  Is
it managed by the mdio bus device or the Ethernet device?  It also has
a potential race condition.  Whereas triggering a late driver bind
will be safe.

Alternately, I'd also be okay with a common method to trigger a
reprobe of a particular phy from userspace, but I fear that would be a
significantly more complex solution.

>
> !!just no idea, how to get mii_bus pointer ...

You'd have to get the parent of the phy node, and then loop over all
the registered mdio busses looking for a bus that uses that node.

>
> here the patch for the new function of_mdiobus_register_phy():
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index b474833..7afbb0b 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -21,6 +21,51 @@
> =A0MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> =A0MODULE_LICENSE("GPL");
>
> +int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *ch=
ild)
> +{
> + =A0 =A0 =A0 struct phy_device *phy;
> + =A0 =A0 =A0 const __be32 *addr;
> + =A0 =A0 =A0 int len;
> + =A0 =A0 =A0 int rc;
> +
> + =A0 =A0 =A0 /* A PHY must have a reg property in the range [0-31] */
> + =A0 =A0 =A0 addr =3D of_get_property(child, "reg", &len);
> + =A0 =A0 =A0 if (!addr || len < sizeof(*addr) || *addr >=3D 32 || *addr =
< 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&mdio->dev, "%s has invalid PHY add=
ress\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 child->full_name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 if (mdio->irq) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mdio->irq[*addr] =3D irq_of_parse_and_map(c=
hild, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!mdio->irq[*addr])
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mdio->irq[*addr] =3D PHY_PO=
LL;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 phy =3D get_phy_device(mdio, be32_to_cpup(addr));
> + =A0 =A0 =A0 if (!phy || IS_ERR(phy)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&mdio->dev, "error probing PHY at a=
ddress %i\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *addr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -2;
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 phy_scan_fixups(phy);
> + =A0 =A0 =A0 /* Associate the OF node with the device structure so it
> + =A0 =A0 =A0 =A0* can be looked up later */
> + =A0 =A0 =A0 of_node_get(child);
> + =A0 =A0 =A0 dev_archdata_set_node(&phy->dev.archdata, child);
> +
> + =A0 =A0 =A0 /* All data is now stored in the phy struct; register it */
> + =A0 =A0 =A0 rc =3D phy_device_register(phy);
> + =A0 =A0 =A0 if (rc) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy_device_free(phy);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(child);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -3;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 child->name, *addr);
> +}
> +
> =A0/**
> =A0* of_mdiobus_register - Register mii_bus and create PHYs from the devi=
ce tree
> =A0* @mdio: pointer to mii_bus structure
> @@ -31,7 +76,6 @@ MODULE_LICENSE("GPL");
> =A0*/
> =A0int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> =A0{
> - =A0 =A0 =A0 struct phy_device *phy;
> =A0 =A0 =A0 =A0struct device_node *child;
> =A0 =A0 =A0 =A0int rc, i;
>
> @@ -51,46 +95,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct d=
evice_node *np)
>
> =A0 =A0 =A0 =A0/* Loop over the child nodes and register a phy_device for=
 each one */
> =A0 =A0 =A0 =A0for_each_child_of_node(np, child) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 const __be32 *addr;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 int len;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* A PHY must have a reg property in the ra=
nge [0-31] */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 addr =3D of_get_property(child, "reg", &len=
);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!addr || len < sizeof(*addr) || *addr >=
=3D 32 || *addr < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&mdio->dev, "%s has=
 invalid PHY address\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 child->full=
_name);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (mdio->irq) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mdio->irq[*addr] =3D irq_of=
_parse_and_map(child, 0);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!mdio->irq[*addr])
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mdio->irq[*=
addr] =3D PHY_POLL;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy =3D get_phy_device(mdio, be32_to_cpup(a=
ddr));
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!phy || IS_ERR(phy)) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&mdio->dev, "error =
probing PHY at address %i\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *addr);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy_scan_fixups(phy);
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Associate the OF node with the device st=
ructure so it
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* can be looked up later */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_get(child);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_archdata_set_node(&phy->dev.archdata, c=
hild);
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* All data is now stored in the phy struct=
; register it */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D phy_device_register(phy);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rc) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy_device_free(phy);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(child);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&mdio->dev, "registered phy %s at a=
ddress %i\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 child->name, *addr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_mdiobus_register_phy(mdio, child);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0return 0;
>
> With this change, it would work on boot as actual (phy_device_register()
> will fail for the PHYs who don;t work when booting).
>
> Later, when opening the ethernet device, fs_init_phy, will look, if
> we have a valid phy with driver, if not we try to register it again.
> If this is successfull, we can use the device, if not we will fail,
> as now ... what do you think?

As mentioned above, it has the potential for some nasty confusion
about where exactly phy devices get registered, not to mention an
unlikely but potential race condition between the mdio bus and the
ethernet device trying to register the same phy since the MDIO bus
gets registered first, and then the bus loops over the phy nodes while
the bus is 'live'.

g.

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Grant Likely]

On Wed, Oct 6, 2010 at 7:50 AM, Holger brunck <holger.brunck@keymile.com> w=
rote:
> Hello Heiko,
>
> On 10/06/2010 11:53 AM, Heiko Schocher wrote:
>>>> So, the question is, is there a possibility to solve this problem?
>>>>
>>>> If there is no standard option, what would be with adding a
>>>> "scan_phy" file in
>>>>
>>>> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
>>>> (or better destination?)
>>>>
>>>> which with we could rescan a PHY with
>>>> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/sc=
an_phy"
>>>> (so there is no need for using of_find_node_by_path(), as we should
>>>> =A0have the associated device node here, and can step through the chil=
d
>>>> =A0nodes with "for_each_child_of_node(np, child)" and check if reg =3D=
=3D addr)
>>>>
>>>> or shouldn;t be at least, if the phy couldn;t be found when opening
>>>> the port, retrigger a scanning, if the phy now is accessible?
>>>
>>> One option would be to still register a phy_device for each phy
>>> described in the device tree, but defer binding a driver to each phy
>>> that doesn't respond. =A0Then at of_phy_find_device() time, if it
>>
>> Maybe I din;t get the trick, but the problem is, that
>> you can;t register a phy_device in drivers/of/of_mdio.c
>> of_mdiobus_register(), if the phy didn;t respond with the
>> phy_id ... and of_phy_find_device() is not (yet) used in fs_enet
>>
>>> matches with a phy_device that isn't bound to a driver yet, then
>>> re-trigger the binding operation. =A0At which point the phy id can be
>>> probed and the correct driver can be chosen. =A0If binding succeeds,
>>> then return the phy_device handle. =A0If not, then fail as it currently
>>> does.
>>
>> Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
>> to look if there is one?
>>
>> Something like that (not tested):
>>
>> in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
>> called from fs_enet_open():
>>
>> Do first:
>> phydev =3D =A0of_phy_find_device(fep->fpi->phy_node);
>>
>> Look if there is a driver (phy_dev->drv =3D=3D NULL ?)
>>
>> If not, call new function
>> of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
>> see below patch for it.
>>
>> If this succeeds, all is OK, and we can use this phy,
>> else ethernet not work.
>>
>> !!just no idea, how to get mii_bus pointer ...
>>
>
> in my understanding it should be posssible to get this pointer via the pa=
rent of
> the device_node you got via the private data of the fs_enet:
> fep->fpi->phy_node->parent should point you to the device_node for the md=
io_bus.

Yes, this will give you the mdio bus node pointer.

> In the next step you should be able to get the pointer of of_device for t=
he
> mdio_bus:
> ofdevice* ofdev =3D to_of_device(fep->fpi->phy_node->parent);

of_device is just an alias for platform_device now, and not all mdio
busses will be instantiated by a platform device.  This method won't
always work.  What is really needed is the pointer to the mii_bus
structure.  That can be obtained by looping over the members of the
mdio_bus_class and comparing the mii_bus->device.parent->of_node to
the parent node from above.

g.

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Holger brunck]

Hi Grant,

On 10/06/2010 06:52 PM, Grant Likely wrote:
> On Wed, Oct 6, 2010 at 3:53 AM, Heiko Schocher <hs@denx.de> wrote:
>>>> So, the question is, is there a possibility to solve this problem?
>>>>
>>>> If there is no standard option, what would be with adding a
>>>> "scan_phy" file in
>>>>
>>>> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
>>>> (or better destination?)
>>>>
>>>> which with we could rescan a PHY with
>>>> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/scan_phy"
>>>> (so there is no need for using of_find_node_by_path(), as we should
>>>>  have the associated device node here, and can step through the child
>>>>  nodes with "for_each_child_of_node(np, child)" and check if reg == addr)
>>>>
>>>> or shouldn;t be at least, if the phy couldn;t be found when opening
>>>> the port, retrigger a scanning, if the phy now is accessible?
>>>
>>> One option would be to still register a phy_device for each phy
>>> described in the device tree, but defer binding a driver to each phy
>>> that doesn't respond.  Then at of_phy_find_device() time, if it
>>
>> Maybe I din;t get the trick, but the problem is, that
>> you can;t register a phy_device in drivers/of/of_mdio.c
>> of_mdiobus_register(), if the phy didn;t respond with the
>> phy_id ... and of_phy_find_device() is not (yet) used in fs_enet
> 
> I'm suggesting modifying the phy layer so that it is possible to
> register a phy_device that doesn't (yet) respond.
> 

yes this sounds reasonable.

>>> matches with a phy_device that isn't bound to a driver yet, then
>>> re-trigger the binding operation.  At which point the phy id can be
>>> probed and the correct driver can be chosen.  If binding succeeds,
>>> then return the phy_device handle.  If not, then fail as it currently
>>> does.
>>
>> Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
>> to look if there is one?
>>
>> Something like that (not tested):
>>
>> in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
>> called from fs_enet_open():
>>
>> Do first:
>> phydev =  of_phy_find_device(fep->fpi->phy_node);
>>
>> Look if there is a driver (phy_dev->drv == NULL ?)
>>
>> If not, call new function
>> of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
>> see below patch for it.
>>
>> If this succeeds, all is OK, and we can use this phy,
>> else ethernet not work.
> 
> I don't like this approach because it muddies the concept of which
> device is actually responsible for managing the phys on the bus.  Is
> it managed by the mdio bus device or the Ethernet device?  It also has
> a potential race condition.  Whereas triggering a late driver bind
> will be safe.
> 
> Alternately, I'd also be okay with a common method to trigger a
> reprobe of a particular phy from userspace, but I fear that would be a
> significantly more complex solution.
> 
>>
>> !!just no idea, how to get mii_bus pointer ...
> 
> You'd have to get the parent of the phy node, and then loop over all
> the registered mdio busses looking for a bus that uses that node.
> 

you say that you don't like the approach to probe the phy again in fs_enet_open,
but currently I don't understand what would be the alternate trigger point to
rescan the mdio bus?

I made a first patch to enhance the phy_device structure and rescan the mdio bus
at time of fs_enet_open (because I didn't see a better trigger point). The
advantage is that we got the mii_bus pointer and the phy addr stored in the
already created phy device structure and is therefore easy to use. See the patch
below for this modifications. Whats currently missing in the patch is to set the
phy_id if the phy was scanned later after phy_device creation. For the mgcoge
board it seems to solve our problem, but maybe I miss something important.

Best regards
Holger Brunck

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index ec2f503..6bc117f 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -775,7 +774,8 @@ static int fs_enet_open(struct net_device *dev)
 {
        struct fs_enet_private *fep = netdev_priv(dev);
        int r;
-       int err;
+       int err = 0;
+       u32 phy_id = 0;

        /* to initialize the fep->cur_rx,... */
        /* not doing this, will cause a crash in fs_enet_rx_napi */
@@ -795,13 +795,23 @@ static int fs_enet_open(struct net_device *dev)
                return -EINVAL;
        }

-       err = fs_init_phy(dev);
-       if (err) {
+       if (fep->phydev == NULL)
+               err = fs_init_phy(dev);
+
+       if (!err && (fep->phydev->available == false))
+               r = get_phy_id(fep->phydev->bus, fep->phydev->addr, &phy_id);
+
+       if (err || (phy_id == 0xffffffff)) {
                free_irq(fep->interrupt, dev);
                if (fep->fpi->use_napi)
                        napi_disable(&fep->napi);
-               return err;
+               if (err)
+                       return err;
+               else
+                       return -EINVAL;
        }
+       else
+               fep->phydev->available = true;
        phy_start(fep->phydev);

        netif_start_queue(dev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index adbc0fd..1f443cb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -173,6 +173,10 @@ struct phy_device* phy_device_create(struct mii_bus *bus,
int addr, int phy_id)
        dev->dev.bus = &mdio_bus_type;
        dev->irq = bus->irq != NULL ? bus->irq[addr] : PHY_POLL;
        dev_set_name(&dev->dev, PHY_ID_FMT, bus->id, addr);
+       if (phy_id == 0xffffffff)
+               dev->available = false;
+       else
+               dev->available = true;

        dev->state = PHY_DOWN;

@@ -232,13 +236,11 @@ struct phy_device * get_phy_device(struct mii_bus *bus,
int addr)
        int r;

        r = get_phy_id(bus, addr, &phy_id);
-       if (r)
-               return ERR_PTR(r);

        /* If the phy_id is mostly Fs, there is no device there */
-       if ((phy_id & 0x1fffffff) == 0x1fffffff)
-               return NULL;
-
+       if (((phy_id & 0x1fffffff) == 0x1fffffff) || r)
+               phy_id = 0xffffffff;
+       /* create phy even if the phy is currently not available */
        dev = phy_device_create(bus, addr, phy_id);

        return dev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6a7eb40..12dc3e4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -303,6 +303,9 @@ struct phy_device {

        int link_timeout;

+       /* Flag to support delayed availability */
+       bool available;
+
        /*
         * Interrupt number for this PHY
         * -1 means no interrupt

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Grant Likely]

On Fri, Oct 08, 2010 at 10:50:50AM +0200, Holger brunck wrote:
> Hi Grant,
> 
> On 10/06/2010 06:52 PM, Grant Likely wrote:
> > On Wed, Oct 6, 2010 at 3:53 AM, Heiko Schocher <hs@denx.de> wrote:
> >>>> So, the question is, is there a possibility to solve this problem?
> >>>>
> >>>> If there is no standard option, what would be with adding a
> >>>> "scan_phy" file in
> >>>>
> >>>> /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40
> >>>> (or better destination?)
> >>>>
> >>>> which with we could rescan a PHY with
> >>>> "echo addr > /proc/device-tree/soc\@f0000000/cpm\@119c0/mdio\@10d40/scan_phy"
> >>>> (so there is no need for using of_find_node_by_path(), as we should
> >>>>  have the associated device node here, and can step through the child
> >>>>  nodes with "for_each_child_of_node(np, child)" and check if reg == addr)
> >>>>
> >>>> or shouldn;t be at least, if the phy couldn;t be found when opening
> >>>> the port, retrigger a scanning, if the phy now is accessible?
> >>>
> >>> One option would be to still register a phy_device for each phy
> >>> described in the device tree, but defer binding a driver to each phy
> >>> that doesn't respond.  Then at of_phy_find_device() time, if it
> >>
> >> Maybe I din;t get the trick, but the problem is, that
> >> you can;t register a phy_device in drivers/of/of_mdio.c
> >> of_mdiobus_register(), if the phy didn;t respond with the
> >> phy_id ... and of_phy_find_device() is not (yet) used in fs_enet
> > 
> > I'm suggesting modifying the phy layer so that it is possible to
> > register a phy_device that doesn't (yet) respond.
> > 
> 
> yes this sounds reasonable.
> 
> >>> matches with a phy_device that isn't bound to a driver yet, then
> >>> re-trigger the binding operation.  At which point the phy id can be
> >>> probed and the correct driver can be chosen.  If binding succeeds,
> >>> then return the phy_device handle.  If not, then fail as it currently
> >>> does.
> >>
> >> Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
> >> to look if there is one?
> >>
> >> Something like that (not tested):
> >>
> >> in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
> >> called from fs_enet_open():
> >>
> >> Do first:
> >> phydev =  of_phy_find_device(fep->fpi->phy_node);
> >>
> >> Look if there is a driver (phy_dev->drv == NULL ?)
> >>
> >> If not, call new function
> >> of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
> >> see below patch for it.
> >>
> >> If this succeeds, all is OK, and we can use this phy,
> >> else ethernet not work.
> > 
> > I don't like this approach because it muddies the concept of which
> > device is actually responsible for managing the phys on the bus.  Is
> > it managed by the mdio bus device or the Ethernet device?  It also has
> > a potential race condition.  Whereas triggering a late driver bind
> > will be safe.
> > 
> > Alternately, I'd also be okay with a common method to trigger a
> > reprobe of a particular phy from userspace, but I fear that would be a
> > significantly more complex solution.
> > 
> >>
> >> !!just no idea, how to get mii_bus pointer ...
> > 
> > You'd have to get the parent of the phy node, and then loop over all
> > the registered mdio busses looking for a bus that uses that node.
> > 
> 
> you say that you don't like the approach to probe the phy again in fs_enet_open,
> but currently I don't understand what would be the alternate trigger point to
> rescan the mdio bus?

Same trigger point, but different operation.  At fs_enet_open time,
instead of registering the phy_device, the phy layer could sanity
check the already registered phy_device, and refuse to connect to it
if the phy isn't responding.  If it is responding, then it could
re-attempt binding a phy_driver to it (although I just realized that
this has other problems, such as correct module loading.  See below)

> I made a first patch to enhance the phy_device structure and rescan the mdio bus
> at time of fs_enet_open (because I didn't see a better trigger point). The
> advantage is that we got the mii_bus pointer and the phy addr stored in the
> already created phy device structure and is therefore easy to use. See the patch
> below for this modifications. Whats currently missing in the patch is to set the
> phy_id if the phy was scanned later after phy_device creation. For the mgcoge
> board it seems to solve our problem, but maybe I miss something important.
> 
> Best regards
> Holger Brunck
> 
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index ec2f503..6bc117f 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -775,7 +774,8 @@ static int fs_enet_open(struct net_device *dev)
>  {
>         struct fs_enet_private *fep = netdev_priv(dev);
>         int r;
> -       int err;
> +       int err = 0;
> +       u32 phy_id = 0;
> 
>         /* to initialize the fep->cur_rx,... */
>         /* not doing this, will cause a crash in fs_enet_rx_napi */
> @@ -795,13 +795,23 @@ static int fs_enet_open(struct net_device *dev)
>                 return -EINVAL;
>         }
> 
> -       err = fs_init_phy(dev);
> -       if (err) {
> +       if (fep->phydev == NULL)
> +               err = fs_init_phy(dev);
> +
> +       if (!err && (fep->phydev->available == false))
> +               r = get_phy_id(fep->phydev->bus, fep->phydev->addr, &phy_id);
> +
> +       if (err || (phy_id == 0xffffffff)) {
>                 free_irq(fep->interrupt, dev);
>                 if (fep->fpi->use_napi)
>                         napi_disable(&fep->napi);
> -               return err;
> +               if (err)
> +                       return err;
> +               else
> +                       return -EINVAL;
>         }
> +       else
> +               fep->phydev->available = true;
>         phy_start(fep->phydev);
> 
>         netif_start_queue(dev);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index adbc0fd..1f443cb 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -173,6 +173,10 @@ struct phy_device* phy_device_create(struct mii_bus *bus,
> int addr, int phy_id)
>         dev->dev.bus = &mdio_bus_type;
>         dev->irq = bus->irq != NULL ? bus->irq[addr] : PHY_POLL;
>         dev_set_name(&dev->dev, PHY_ID_FMT, bus->id, addr);
> +       if (phy_id == 0xffffffff)
> +               dev->available = false;
> +       else
> +               dev->available = true;

This flag shouldn't be necessary.  Just check whether or not
phy_device->phy_id is sane at phy_attach_direct() time.  If it is
mostly f's, then don't attach.

> 
>         dev->state = PHY_DOWN;
> 
> @@ -232,13 +236,11 @@ struct phy_device * get_phy_device(struct mii_bus *bus,
> int addr)
>         int r;
> 
>         r = get_phy_id(bus, addr, &phy_id);
> -       if (r)
> -               return ERR_PTR(r);
> 
>         /* If the phy_id is mostly Fs, there is no device there */
> -       if ((phy_id & 0x1fffffff) == 0x1fffffff)
> -               return NULL;
> -
> +       if (((phy_id & 0x1fffffff) == 0x1fffffff) || r)
> +               phy_id = 0xffffffff;
> +       /* create phy even if the phy is currently not available */
>         dev = phy_device_create(bus, addr, phy_id);

Cannot do it this way because many phylib users probe the bus for phys
instead of the explicit creation used with the device tree.  There
needs to be a method to explicitly skip this test when creating a phy;
possibly by having the device tree code call phy_device_create()
directly.

Hmmm.... I see another problem.  Deferred probing of the phy will
potentially cause problems with module loading.  If the binding is
deferred to phy connect time; then the phy driver may not have time to
get loaded before the phy layer decides there is no driver and binds
it to the generic one.  Blech.

Okay, so it seems like a method of explicitly triggering a phy_device
rebind from userspace is necessary.  This could be done with a
per-phy_device sysfs file I suppose.  Just an empty file that when
read triggers a re-read of the phy id registers, and retries binding a
driver, including the request_module call in phy_device_create().

> 
>         return dev;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 6a7eb40..12dc3e4 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -303,6 +303,9 @@ struct phy_device {
> 
>         int link_timeout;
> 
> +       /* Flag to support delayed availability */
> +       bool available;
> +
>         /*
>          * Interrupt number for this PHY
>          * -1 means no interrupt
> 

Re: powerpc, fs_enet: scanning PHY after Linux is up

[Holger brunck]

Hi Grant,

On 10/08/2010 07:06 PM, Grant Likely wrote:
> On Fri, Oct 08, 2010 at 10:50:50AM +0200, Holger brunck wrote:
>>> On Wed, Oct 6, 2010 at 3:53 AM, Heiko Schocher <hs@denx.de> wrote:
>>
>>>> Wouldn;t it be good, just if we need a PHY (on calling fs_enet_open)
>>>> to look if there is one?
>>>>
>>>> Something like that (not tested):
>>>>
>>>> in drivers/net/fs_enet/fs_enet-main.c in fs_init_phy()
>>>> called from fs_enet_open():
>>>>
>>>> Do first:
>>>> phydev =  of_phy_find_device(fep->fpi->phy_node);
>>>>
>>>> Look if there is a driver (phy_dev->drv == NULL ?)
>>>>
>>>> If not, call new function
>>>> of_mdiobus_register_phy(mii_bus, fep->fpi->phy_node)
>>>> see below patch for it.
>>>>
>>>> If this succeeds, all is OK, and we can use this phy,
>>>> else ethernet not work.
>>>
>>> I don't like this approach because it muddies the concept of which
>>> device is actually responsible for managing the phys on the bus.  Is
>>> it managed by the mdio bus device or the Ethernet device?  It also has
>>> a potential race condition.  Whereas triggering a late driver bind
>>> will be safe.
>>>
>>> Alternately, I'd also be okay with a common method to trigger a
>>> reprobe of a particular phy from userspace, but I fear that would be a
>>> significantly more complex solution.
>>>
>>>>
>>>> !!just no idea, how to get mii_bus pointer ...
>>>
>>> You'd have to get the parent of the phy node, and then loop over all
>>> the registered mdio busses looking for a bus that uses that node.
>>>
>>
>> you say that you don't like the approach to probe the phy again in fs_enet_open,
>> but currently I don't understand what would be the alternate trigger point to
>> rescan the mdio bus?
> 
> Same trigger point, but different operation.  At fs_enet_open time,
> instead of registering the phy_device, the phy layer could sanity
> check the already registered phy_device, and refuse to connect to it
> if the phy isn't responding.  If it is responding, then it could
> re-attempt binding a phy_driver to it (although I just realized that
> this has other problems, such as correct module loading.  See below)
> 

ok.

>> I made a first patch to enhance the phy_device structure and rescan the mdio bus
>> at time of fs_enet_open (because I didn't see a better trigger point). The
>> advantage is that we got the mii_bus pointer and the phy addr stored in the
>> already created phy device structure and is therefore easy to use. See the patch
>> below for this modifications. Whats currently missing in the patch is to set the
>> phy_id if the phy was scanned later after phy_device creation. For the mgcoge
>> board it seems to solve our problem, but maybe I miss something important.
>>
>> Best regards
>> Holger Brunck
>>
>> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
>> index ec2f503..6bc117f 100644
>> --- a/drivers/net/fs_enet/fs_enet-main.c
>> +++ b/drivers/net/fs_enet/fs_enet-main.c
>> @@ -775,7 +774,8 @@ static int fs_enet_open(struct net_device *dev)
>>  {
>>         struct fs_enet_private *fep = netdev_priv(dev);
>>         int r;
>> -       int err;
>> +       int err = 0;
>> +       u32 phy_id = 0;
>>
>>         /* to initialize the fep->cur_rx,... */
>>         /* not doing this, will cause a crash in fs_enet_rx_napi */
>> @@ -795,13 +795,23 @@ static int fs_enet_open(struct net_device *dev)
>>                 return -EINVAL;
>>         }
>>
>> -       err = fs_init_phy(dev);
>> -       if (err) {
>> +       if (fep->phydev == NULL)
>> +               err = fs_init_phy(dev);
>> +
>> +       if (!err && (fep->phydev->available == false))
>> +               r = get_phy_id(fep->phydev->bus, fep->phydev->addr, &phy_id);
>> +
>> +       if (err || (phy_id == 0xffffffff)) {
>>                 free_irq(fep->interrupt, dev);
>>                 if (fep->fpi->use_napi)
>>                         napi_disable(&fep->napi);
>> -               return err;
>> +               if (err)
>> +                       return err;
>> +               else
>> +                       return -EINVAL;
>>         }
>> +       else
>> +               fep->phydev->available = true;
>>         phy_start(fep->phydev);
>>
>>         netif_start_queue(dev);
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index adbc0fd..1f443cb 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -173,6 +173,10 @@ struct phy_device* phy_device_create(struct mii_bus *bus,
>> int addr, int phy_id)
>>         dev->dev.bus = &mdio_bus_type;
>>         dev->irq = bus->irq != NULL ? bus->irq[addr] : PHY_POLL;
>>         dev_set_name(&dev->dev, PHY_ID_FMT, bus->id, addr);
>> +       if (phy_id == 0xffffffff)
>> +               dev->available = false;
>> +       else
>> +               dev->available = true;
> 
> This flag shouldn't be necessary.  Just check whether or not
> phy_device->phy_id is sane at phy_attach_direct() time.  If it is
> mostly f's, then don't attach.
>

Yes, indeed it is unneeded. Thanks for pointing out.

>>
>>         dev->state = PHY_DOWN;
>>
>> @@ -232,13 +236,11 @@ struct phy_device * get_phy_device(struct mii_bus *bus,
>> int addr)
>>         int r;
>>
>>         r = get_phy_id(bus, addr, &phy_id);
>> -       if (r)
>> -               return ERR_PTR(r);
>>
>>         /* If the phy_id is mostly Fs, there is no device there */
>> -       if ((phy_id & 0x1fffffff) == 0x1fffffff)
>> -               return NULL;
>> -
>> +       if (((phy_id & 0x1fffffff) == 0x1fffffff) || r)
>> +               phy_id = 0xffffffff;
>> +       /* create phy even if the phy is currently not available */
>>         dev = phy_device_create(bus, addr, phy_id);
> 
> Cannot do it this way because many phylib users probe the bus for phys
> instead of the explicit creation used with the device tree.  There
> needs to be a method to explicitly skip this test when creating a phy;
> possibly by having the device tree code call phy_device_create()
> directly.
> 

Ah ok, every phy_device_create() call from of_mdiobus_register should skip this
test, because if a phy is described in the dts it is present (sooner or later)
and if phy_device_create is called from somewhere else I do this test as usual.
I adapted my patch accordingly.

> Hmmm.... I see another problem.  Deferred probing of the phy will
> potentially cause problems with module loading.  If the binding is
> deferred to phy connect time; then the phy driver may not have time to
> get loaded before the phy layer decides there is no driver and binds
> it to the generic one.  Blech.
> 
> Okay, so it seems like a method of explicitly triggering a phy_device
> rebind from userspace is necessary.  This could be done with a
> per-phy_device sysfs file I suppose.  Just an empty file that when
> read triggers a re-read of the phy id registers, and retries binding a
> driver, including the request_module call in phy_device_create().
> 

Okay I suspected that there is not an easy solution for our problem. Another
solution comes in my mind. If we defer the call to fs_enet_probe at startup. So
enhance the dts entry with something like an hotplug indication and then trigger
via an sysfs entry the call to fs_enet_probe if the phy is up... Other hotplug
devices should have similar problems...

However for our mgcoge board we can live with the fact that we can't load/unload
the driver dynamically. So I think we will go with this modified "out of tree"
patch for our board. Thanks for your support.

Best regards
Holger Brunck