| Message ID | 1595938298-13190-1-git-send-email-vikas.singh@puresoftware.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe | expand |
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
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.
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.
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 !!
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;
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(-)