| Message ID | 20200330160136.23018-1-codrin.ciubotariu@microchip.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | net: mdio: of: Do not treat fixed-link as PHY | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 1 warnings, 26 lines checked" |
On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: > Some ethernet controllers, such as cadence's macb, have an embedded MDIO. > For this reason, the ethernet PHY nodes are not under an MDIO bus, but > directly under the ethernet node. Hi Codrin That is deprecated. It causes all sorts of problems putting PHY nodes in the MAC without a container. Please fix macb to look for an mdio node, and place your fixed link inside it. Andrew
On Mon, Mar 30, 2020 at 06:17:40PM +0200, Andrew Lunn wrote: > On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: > > Some ethernet controllers, such as cadence's macb, have an embedded MDIO. > > For this reason, the ethernet PHY nodes are not under an MDIO bus, but > > directly under the ethernet node. > > Hi Codrin > > That is deprecated. It causes all sorts of problems putting PHY nodes > in the MAC without a container. > > Please fix macb to look for an mdio node, and place your fixed link > inside it. Seems wrong. fixed links have never needed to be under a mdio node - see Documentation/devicetree/bindings/net/ethernet-controller.yaml fixed-link is a child of the MAC controller, not of a mdio node.
On Mon, Mar 30, 2020 at 05:21:30PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Mar 30, 2020 at 06:17:40PM +0200, Andrew Lunn wrote: > > On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: > > > Some ethernet controllers, such as cadence's macb, have an embedded MDIO. > > > For this reason, the ethernet PHY nodes are not under an MDIO bus, but > > > directly under the ethernet node. > > > > Hi Codrin > > > > That is deprecated. It causes all sorts of problems putting PHY nodes > > in the MAC without a container. > > > > Please fix macb to look for an mdio node, and place your fixed link > > inside it. > > Seems wrong. Hi Russell Gerr. You are right. > fixed links have never needed to be under a mdio node - see > Documentation/devicetree/bindings/net/ethernet-controller.yaml macb does crazy stuff. I will take another look. Andrew
On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: > Some ethernet controllers, such as cadence's macb, have an embedded MDIO. > For this reason, the ethernet PHY nodes are not under an MDIO bus, but > directly under the ethernet node. Since these drivers might use > of_mdiobus_child_is_phy(), we should fix this function by returning false > if a fixed-link is found. So i assume the problem occurs here: static int macb_mdiobus_register(struct macb *bp) { struct device_node *child, *np = bp->pdev->dev.of_node; /* Only create the PHY from the device tree if at least one PHY is * described. Otherwise scan the entire MDIO bus. We do this to support * old device tree that did not follow the best practices and did not * describe their network PHYs. */ for_each_available_child_of_node(np, child) if (of_mdiobus_child_is_phy(child)) { /* The loop increments the child refcount, * decrement it before returning. */ of_node_put(child); return of_mdiobus_register(bp->mii_bus, np); } return mdiobus_register(bp->mii_bus); } I think a better solution is for_each_available_child_of_node(np, child) + if (of_phy_is_fixed_link(child) + continue; if (of_mdiobus_child_is_phy(child)) { /* The loop increments the child refcount, * decrement it before returning. */ of_node_put(child); return of_mdiobus_register(bp->mii_bus, np); } return mdiobus_register(bp->mii_bus); } This problem is only an issue for macb, so keep the fix local to macb. Andrew
On 3/30/2020 9:30 AM, Andrew Lunn wrote: > On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: >> Some ethernet controllers, such as cadence's macb, have an embedded MDIO. >> For this reason, the ethernet PHY nodes are not under an MDIO bus, but >> directly under the ethernet node. Since these drivers might use >> of_mdiobus_child_is_phy(), we should fix this function by returning false >> if a fixed-link is found. > > So i assume the problem occurs here: > > static int macb_mdiobus_register(struct macb *bp) > { > struct device_node *child, *np = bp->pdev->dev.of_node; > > /* Only create the PHY from the device tree if at least one PHY is > * described. Otherwise scan the entire MDIO bus. We do this to support > * old device tree that did not follow the best practices and did not > * describe their network PHYs. > */ > for_each_available_child_of_node(np, child) > if (of_mdiobus_child_is_phy(child)) { > /* The loop increments the child refcount, > * decrement it before returning. > */ > of_node_put(child); > > return of_mdiobus_register(bp->mii_bus, np); > } > > return mdiobus_register(bp->mii_bus); > } > > I think a better solution is > > for_each_available_child_of_node(np, child) > + if (of_phy_is_fixed_link(child) > + continue; > if (of_mdiobus_child_is_phy(child)) { > /* The loop increments the child refcount, > * decrement it before returning. > */ > of_node_put(child); > > return of_mdiobus_register(bp->mii_bus, np); > } > > return mdiobus_register(bp->mii_bus); > } > > This problem is only an issue for macb, so keep the fix local to macb. Agree, there is no reason for of_mdiobus_child_is_phy() to be checking for a fixed-link. If you submit this formally: Acked-by: Florian Fainelli <f.fainelli@gmail.com>
On 30.03.2020 20:22, Florian Fainelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 3/30/2020 9:30 AM, Andrew Lunn wrote: >> On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote: >>> Some ethernet controllers, such as cadence's macb, have an embedded MDIO. >>> For this reason, the ethernet PHY nodes are not under an MDIO bus, but >>> directly under the ethernet node. Since these drivers might use >>> of_mdiobus_child_is_phy(), we should fix this function by returning false >>> if a fixed-link is found. >> >> So i assume the problem occurs here: >> >> static int macb_mdiobus_register(struct macb *bp) >> { >> struct device_node *child, *np = bp->pdev->dev.of_node; >> >> /* Only create the PHY from the device tree if at least one PHY is >> * described. Otherwise scan the entire MDIO bus. We do this to support >> * old device tree that did not follow the best practices and did not >> * describe their network PHYs. >> */ >> for_each_available_child_of_node(np, child) >> if (of_mdiobus_child_is_phy(child)) { >> /* The loop increments the child refcount, >> * decrement it before returning. >> */ >> of_node_put(child); >> >> return of_mdiobus_register(bp->mii_bus, np); >> } >> >> return mdiobus_register(bp->mii_bus); >> } >> >> I think a better solution is >> >> for_each_available_child_of_node(np, child) >> + if (of_phy_is_fixed_link(child) >> + continue; >> if (of_mdiobus_child_is_phy(child)) { >> /* The loop increments the child refcount, >> * decrement it before returning. >> */ >> of_node_put(child); >> >> return of_mdiobus_register(bp->mii_bus, np); >> } >> >> return mdiobus_register(bp->mii_bus); >> } >> >> This problem is only an issue for macb, so keep the fix local to macb. > > Agree, there is no reason for of_mdiobus_child_is_phy() to be checking > for a fixed-link. If you submit this formally: > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> Thanks guys. I thought there might be other controllers that have the PHY nodes inside the ethernet node. If not, I guess that of_mdiobus_child_is_phy() can be restricted to be called only if the passed device_node points to an MDIO node. Moving the fix to macb, I think that of_phy_is_fixed_link() needs a device_node to the ethernet node, since it also has to deal with the legacy case in which fixed-link is a property, so it would look like something like this: struct device_node *child, *np = bp->pdev->dev.of_node; + if (of_phy_is_fixed_link(np)) + return mdiobus_register(bp->mii_bus); + /* Only create the PHY from the device tree if at least one PHY is I will send another patch shortly. Thanks and best regards, Codrin
> Thanks guys. I thought there might be other controllers that have the > PHY nodes inside the ethernet node. Hi Codrin There are some still using this deprecated feature. But macb is the only one doing this odd looping over child nodes. It is this looping which is breaking things, not the use of the deprecated feature itself. Andrew
On 31.03.2020 15:59, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Thanks guys. I thought there might be other controllers that have the >> PHY nodes inside the ethernet node. > > Hi Codrin Hi Andrew > > There are some still using this deprecated feature. But macb is the > only one doing this odd looping over child nodes. It is this looping > which is breaking things, not the use of the deprecated feature > itself. Yes, its due to the fact that the MDIO node is missing. Should we have in mind to add an MDIO node under the macb node, where we could add the PHY nodes? The macb bindings don't seem to require the PHY nodes directly under the macb node, but the compatibility with the deprecated feature needs to be maintained. Best regards, Codrin
> Hi Andrew > > > > > There are some still using this deprecated feature. But macb is the > > only one doing this odd looping over child nodes. It is this looping > > which is breaking things, not the use of the deprecated feature > > itself. > > Yes, its due to the fact that the MDIO node is missing. Should we have > in mind to add an MDIO node under the macb node, where we could add the > PHY nodes? Yes, you can make the driver complient any time you want. But as you said, you need to keep with backwards compatibility. But net-next is closed now, for the merge window. So you probably want to wait two weeks before posting code. Andrew
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 9f982c0627a0..7cf8aad645a4 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -195,12 +195,17 @@ static const struct of_device_id whitelist_phys[] = { * o Compatible string of "ethernet-phy-ieee802.3-c22" * o In the white list above (and issue a warning) * o No compatibility string + * o Not a fixed-link * * A device which is not a phy is expected to have a compatible string * indicating what sort of device it is. */ bool of_mdiobus_child_is_phy(struct device_node *child) { + const struct of_device_id fixed_link[] = { + { .name = "fixed-link" }, + {} + }; u32 phy_id; if (of_get_phy_id(child, &phy_id) != -EINVAL) @@ -219,7 +224,8 @@ bool of_mdiobus_child_is_phy(struct device_node *child) return true; } - if (!of_find_property(child, "compatible", NULL)) + if (!of_find_property(child, "compatible", NULL) && + !of_match_node(fixed_link, child)) return true; return false;
Some ethernet controllers, such as cadence's macb, have an embedded MDIO. For this reason, the ethernet PHY nodes are not under an MDIO bus, but directly under the ethernet node. Since these drivers might use of_mdiobus_child_is_phy(), we should fix this function by returning false if a fixed-link is found. Fixes: 801a8ef54e8b ("of: phy: Only register a phy device for phys") Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- drivers/of/of_mdio.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)