[PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Vikas Singh]

Auto-probe of c45 devices with extended scanning in xgmac_mdio works
well but fails to update device "fwnode" while registering PHYs on
MDIO bus.
This patch is based on https://www.spinics.net/lists/netdev/msg662173.html

This change will update the "fwnode" while PHYs get registered and allow
lookup for registered PHYs on MDIO bus from other drivers while probing.

Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
---
 drivers/net/phy/mdio_bus.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 46b3370..7275eff 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -447,8 +447,25 @@ static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
 	struct device *dev = &mdiodev->dev;
 	struct device_node *child;
 
-	if (dev->of_node || !bus->dev.of_node)
+	if (dev->of_node || !bus->dev.of_node) {
+		/* Checking for acpi node, before returning */
+		struct fwnode_handle *fwnode;
+
+		/* Set the device firmware node to look for child nodes */
+		bus->dev.fwnode = bus->parent->fwnode;
+
+		device_for_each_child_node(&bus->dev, fwnode) {
+			int addr;
+
+			if (fwnode_property_read_u32(fwnode, "reg", &addr))
+				continue;
+			if (addr == mdiodev->addr) {
+				dev->fwnode = fwnode;
+				break;
+			}
+		}
 		return;
+	}
 
 	for_each_available_child_of_node(bus->dev.of_node, child) {
 		int addr;
-- 
2.7.4

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Andrew Lunn]

On Tue, Jul 28, 2020 at 05:41:38PM +0530, Vikas Singh wrote:
> Auto-probe of c45 devices with extended scanning in xgmac_mdio works
> well but fails to update device "fwnode" while registering PHYs on
> MDIO bus.
> This patch is based on https://www.spinics.net/lists/netdev/msg662173.html
> 
> This change will update the "fwnode" while PHYs get registered and allow
> lookup for registered PHYs on MDIO bus from other drivers while probing.
> 
> Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
> ---
>  drivers/net/phy/mdio_bus.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 46b3370..7275eff 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -447,8 +447,25 @@ static void of_mdiobus_link_mdiodev(struct mii_bus *bus,

Why would a function called of_mdiobus_link_mdiodev() be poking
around trying to find ACPI properties?

       Andrew

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Florian Fainelli]

On 7/31/2020 9:53 PM, Vikas Singh wrote:
> Hi Andrew,
>  
> As i have already mentioned that this patch is based on
> https://www.spinics.net/lists/netdev/msg662173.html,
> <https://www.spinics.net/lists/netdev/msg662173.html>
> 
> When MDIO bus gets registered itself along with devices on it , the
> function mdiobus_register() inside of_mdiobus_register(), brings
> up all the PHYs on the mdio bus and attach them to the bus with the help
> of_mdiobus_link_mdiodev() inside mdiobus_scan() .
> Additionally it has been discussed with the maintainers that the
> mdiobus_register() function should be capable of handling both ACPI &
> DTB stuff
> without any change to existing implementation.
> Now of_mdiobus_link_mdiodev() inside mdiobus_scan() see if the
> auto-probed phy has a corresponding child in the bus node, and set the
> "of_node" pointer in DT case.
> But lacks to set the "fwnode" pointer in ACPI case which is resulting in
> mdiobus_register() failure as an end result theoretically.
> 
> Now this patch set (changes) will attempt to fill this gap and
> generalise the mdiobus_register() implementation for both ACPI & DT with
> no duplicacy or redundancy.

Please reply in plain text and do not top-post, thank you.
-- 
Florian

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Russell King - ARM Linux admin]

On Sat, Aug 01, 2020 at 10:23:38AM +0530, Vikas Singh wrote:
> Hi Andrew,
> 
> As i have already mentioned that this patch is based on
> https://www.spinics.net/lists/netdev/msg662173.html,
> <https://www.spinics.net/lists/netdev/msg662173.html>
> 
> When MDIO bus gets registered itself along with devices on it , the
> function mdiobus_register() inside of_mdiobus_register(), brings
> up all the PHYs on the mdio bus and attach them to the bus with the help
> of_mdiobus_link_mdiodev() inside mdiobus_scan() .
> Additionally it has been discussed with the maintainers that the
> mdiobus_register() function should be capable of handling both ACPI & DTB
> stuff
> without any change to existing implementation.
> Now of_mdiobus_link_mdiodev() inside mdiobus_scan() see if the auto-probed
> phy has a corresponding child in the bus node, and set the "of_node"
> pointer in DT case.
> But lacks to set the "fwnode" pointer in ACPI case which is resulting in
> mdiobus_register() failure as an end result theoretically.
> 
> Now this patch set (changes) will attempt to fill this gap and generalise
> the mdiobus_register() implementation for both ACPI & DT with no duplicacy
> or redundancy.

Please do not top-post.

What Andrew is asking is why _should_ a DT specific function (which
starts with of_, meaning "open firmware") have anything to do with
ACPI.  The function you refer to (of_mdiobus_link_mdiodev()) is only
built when CONFIG_OF_MDIO is enabled, which is again, a DT specific
thing.

So, why should a DT specific function care about ACPI?

We're not asking about the fine details, we're asking about the high
level idea that DT functions should know about ACPI.

The assignment in of_mdiobus_link_mdiodev() to dev->fwnode is not
itself about ACPI, it's about enabling drivers that wish to access
DT properties through the fwnode property APIs can do so.

IMHO, the right way to go about this is to implement it as a non-DT
function.  Given that it is a static function, Andrew may find it
acceptable if you also renamed of_mdiobus_link_mdiodev() as
mdiobus_link_mdiodev() and moved it out of the #ifdef.

+               bus->dev.fwnode = bus->parent->fwnode;

That should be done elsewhere, not here.  of_mdiobus_register() already
ensures that this is appropriately set, and if it isn't, maybe there's
a bug elsewhere.

Lastly, note that you don't need two loops, one for ACPI and one for
DT (it's a shame there isn't a device_for_each_available_child_node()):

	int addr;

	if (dev->fwnode && !bus->dev.fwnode)
		return;

	device_for_each_child_node(&bus->dev, fwnode) {
		if (!fwnode_device_is_available(fwnode))
			continue;

		if (is_of_node(fwnode))
			addr = of_mdio_parse_addr(dev, to_of_node(fwnode));
		else if (fwnode_property_read_u32(fwnode, "reg", &addr))
			continue;

		if (addr == mdiodev->addr) {
			dev->of_node = to_of_node(fwnode);
			dev->fwnode = fwnode;
			return;
		}
	}

which, I think, will behave identically to the existing implementation
when called in a DT setup, but should also add what you want.

So, maybe with the above, moving it out from under the ifdef, and
renaming it _may_ be acceptable.  This is just a suggestion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Vikas Singh]

On Sat, Aug 1, 2020 at 3:08 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Sat, Aug 01, 2020 at 10:23:38AM +0530, Vikas Singh wrote:
> > Hi Andrew,
> >
> > As i have already mentioned that this patch is based on
> > https://www.spinics.net/lists/netdev/msg662173.html,
> > <https://www.spinics.net/lists/netdev/msg662173.html>
> >
> > When MDIO bus gets registered itself along with devices on it , the
> > function mdiobus_register() inside of_mdiobus_register(), brings
> > up all the PHYs on the mdio bus and attach them to the bus with the help
> > of_mdiobus_link_mdiodev() inside mdiobus_scan() .
> > Additionally it has been discussed with the maintainers that the
> > mdiobus_register() function should be capable of handling both ACPI & DTB
> > stuff
> > without any change to existing implementation.
> > Now of_mdiobus_link_mdiodev() inside mdiobus_scan() see if the auto-probed
> > phy has a corresponding child in the bus node, and set the "of_node"
> > pointer in DT case.
> > But lacks to set the "fwnode" pointer in ACPI case which is resulting in
> > mdiobus_register() failure as an end result theoretically.
> >
> > Now this patch set (changes) will attempt to fill this gap and generalise
> > the mdiobus_register() implementation for both ACPI & DT with no duplicacy
> > or redundancy.
>
> Please do not top-post.
>
> What Andrew is asking is why _should_ a DT specific function (which
> starts with of_, meaning "open firmware") have anything to do with
> ACPI.  The function you refer to (of_mdiobus_link_mdiodev()) is only
> built when CONFIG_OF_MDIO is enabled, which is again, a DT specific
> thing.
>
> So, why should a DT specific function care about ACPI?
>
> We're not asking about the fine details, we're asking about the high
> level idea that DT functions should know about ACPI.
>
> The assignment in of_mdiobus_link_mdiodev() to dev->fwnode is not
> itself about ACPI, it's about enabling drivers that wish to access
> DT properties through the fwnode property APIs can do so.
>
> IMHO, the right way to go about this is to implement it as a non-DT
> function.  Given that it is a static function, Andrew may find it
> acceptable if you also renamed of_mdiobus_link_mdiodev() as
> mdiobus_link_mdiodev() and moved it out of the #ifdef.
>
> +               bus->dev.fwnode = bus->parent->fwnode;
>
> That should be done elsewhere, not here.  of_mdiobus_register() already
> ensures that this is appropriately set, and if it isn't, maybe there's
> a bug elsewhere.
>
> Lastly, note that you don't need two loops, one for ACPI and one for
> DT (it's a shame there isn't a device_for_each_available_child_node()):
>
>         int addr;
>
>         if (dev->fwnode && !bus->dev.fwnode)
>                 return;
>
>         device_for_each_child_node(&bus->dev, fwnode) {
>                 if (!fwnode_device_is_available(fwnode))
>                         continue;
>
>                 if (is_of_node(fwnode))
>                         addr = of_mdio_parse_addr(dev, to_of_node(fwnode));
>                 else if (fwnode_property_read_u32(fwnode, "reg", &addr))
>                         continue;
>
>                 if (addr == mdiodev->addr) {
>                         dev->of_node = to_of_node(fwnode);
>                         dev->fwnode = fwnode;
>                         return;
>                 }
>         }
>
> which, I think, will behave identically to the existing implementation
> when called in a DT setup, but should also add what you want.
>
> So, maybe with the above, moving it out from under the ifdef, and
> renaming it _may_ be acceptable.  This is just a suggestion.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Hi Russell,

Apologies for the late response, I was stuck with other critical stuff !!

Thank you so much. Agreed. I will do the suggested changes and send a
V2 patch shortly.

Thnx !!

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[David Miller]

From: Vikas Singh <vikas.singh@puresoftware.com>
Date: Wed, 22 Jul 2020 16:52:14 +0530

> *Disclaimer* -The information transmitted is intended solely for the 
> individual
> or entity to which it is addressed and may contain confidential 
> and/or
> privileged material. Any review, re-transmission, dissemination or 
> other use of
> or taking action in reliance upon this information by persons 
> or entities other
> than the intended recipient is prohibited. If you have 
> received this email in
> error please contact the sender and delete the 
> material from any computer. In
> such instances you are further prohibited 
> from reproducing, disclosing,
> distributing or taking any action in reliance 
> on it.As a recipient of this email,
> you are responsible for screening its 
> contents and the contents of any
> attachments for the presence of viruses. 
> No liability is accepted for any
> damages caused by any virus transmitted by 
> this email.

Postings with such disclaimers will be ignored.

Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Andrew Lunn]

> *Disclaimer* -The information transmitted is intended solely for the
> individual or entity to which it is addressed and may contain
> confidential and/or privileged material. Any review,
> re-transmission, dissemination or other use of or taking action in
> reliance upon this information by persons or entities other than the
> intended recipient is prohibited. If you have received this email in
> error please contact the sender and delete the material from any
> computer.

I.m going to partially ignore this request to delete your email...

This patch is dependent on

Calvin Johnson  [net-next PATCH v7 0/6]  ACPI support for dpaa2 MAC driver.

which is stuck in limbo waiting for an ACPI maintainer to ACK/NACK it.
I've also had off list suggestions it should be NACKed. If it does get
NACKed, it makes this patch pointless.

	Andrew

[PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Vikas Singh]

Auto-probe of c45 devices with extended scanning in xgmac_mdio works
well but fails to update device "fwnode" while registering PHYs on
MDIO bus.
This patch is based on https://www.spinics.net/lists/netdev/msg662173.html

This change will update the "fwnode" while PHYs get registered and allow
lookup for registered PHYs on MDIO bus from other drivers while probing.

Signed-off-by: Vikas Singh <vikas.singh@puresoftware.com>
---
 drivers/net/phy/mdio_bus.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 46b3370..7275eff 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -447,8 +447,25 @@ static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
 	struct device *dev = &mdiodev->dev;
 	struct device_node *child;
 
-	if (dev->of_node || !bus->dev.of_node)
+	if (dev->of_node || !bus->dev.of_node) {
+		/* Checking for acpi node, before returning */
+		struct fwnode_handle *fwnode;
+
+		/* Set the device firmware node to look for child nodes */
+		bus->dev.fwnode = bus->parent->fwnode;
+
+		device_for_each_child_node(&bus->dev, fwnode) {
+			int addr;
+
+			if (fwnode_property_read_u32(fwnode, "reg", &addr))
+				continue;
+			if (addr == mdiodev->addr) {
+				dev->fwnode = fwnode;
+				break;
+			}
+		}
 		return;
+	}
 
 	for_each_available_child_of_node(bus->dev.of_node, child) {
 		int addr;
-- 
2.7.4


-- 




*Disclaimer* -The information transmitted is intended solely for the 
individual
or entity to which it is addressed and may contain confidential 
and/or
privileged material. Any review, re-transmission, dissemination or 
other use of
or taking action in reliance upon this information by persons 
or entities other
than the intended recipient is prohibited. If you have 
received this email in
error please contact the sender and delete the 
material from any computer. In
such instances you are further prohibited 
from reproducing, disclosing,
distributing or taking any action in reliance 
on it.As a recipient of this email,
you are responsible for screening its 
contents and the contents of any
attachments for the presence of viruses. 
No liability is accepted for any
damages caused by any virus transmitted by 
this email.