| Message ID | 20200724105033.2124881-4-codrin.ciubotariu@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add an MDIO sub-node under MACB | expand |
On 7/24/20 3:50 AM, Codrin Ciubotariu wrote: > The MACB embeds an MDIO bus controller. For this reason, the PHY nodes > were represented as sub-nodes in the MACB node. Generally, the > Ethernet controller is different than the MDIO controller, so the PHYs > are probed by a separate MDIO driver. Since adding the PHY nodes directly > under the ETH node became deprecated, we adjust the MACB driver to look > for an MDIO node and register the subnode MDIO devices. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > Changes in v3: > - moved the check for the mdio node at the beginnging of > macb_mdiobus_register(). This way, the mdio devices will be probed even > if macb is a fixed-link > > Changes in v2: > - readded newline removed by mistake; > > drivers/net/ethernet/cadence/macb_main.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 89fe7af5e408..cb0b3637651c 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -740,6 +740,16 @@ static int macb_mii_probe(struct net_device *dev) > static int macb_mdiobus_register(struct macb *bp) > { > struct device_node *child, *np = bp->pdev->dev.of_node; > + struct device_node *mdio_node; > + int ret; > + > + /* if an MDIO node is present, it should contain the PHY nodes */ > + mdio_node = of_get_child_by_name(np, "mdio"); > + if (mdio_node) { > + ret = of_mdiobus_register(bp->mii_bus, mdio_node); > + of_node_put(mdio_node); > + return ret; > + } This does take care of registering the MDIO bus controller when present as a sub-node, however if you also plan on making use of fixed-link, we will have already returned. > > if (of_phy_is_fixed_link(np)) > return mdiobus_register(bp->mii_bus); > Really not sure what this is achieving, because we start off assuming that we have an OF driven configuration, but later on we register the MDIO bus with mdiobus_register() (and not of_mdiobus_register()), so no scanning of the MDIO bus will happen. How does the driver currently support being provided a fixed-link property? Should not we at least have this pattern: */ if (of_phy_is_fixed_link(dn)) { ret = of_phy_register_fixed_link(dn); if (ret) return ret; priv->phy_dn = dn; } It does not look like you are breaking anything here, because it does not look like this works at all.
On 24.07.2020 20:40, Florian Fainelli wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 7/24/20 3:50 AM, Codrin Ciubotariu wrote: >> The MACB embeds an MDIO bus controller. For this reason, the PHY nodes >> were represented as sub-nodes in the MACB node. Generally, the >> Ethernet controller is different than the MDIO controller, so the PHYs >> are probed by a separate MDIO driver. Since adding the PHY nodes directly >> under the ETH node became deprecated, we adjust the MACB driver to look >> for an MDIO node and register the subnode MDIO devices. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> >> Changes in v3: >> - moved the check for the mdio node at the beginnging of >> macb_mdiobus_register(). This way, the mdio devices will be probed even >> if macb is a fixed-link >> >> Changes in v2: >> - readded newline removed by mistake; >> >> drivers/net/ethernet/cadence/macb_main.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 89fe7af5e408..cb0b3637651c 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -740,6 +740,16 @@ static int macb_mii_probe(struct net_device *dev) >> static int macb_mdiobus_register(struct macb *bp) >> { >> struct device_node *child, *np = bp->pdev->dev.of_node; >> + struct device_node *mdio_node; >> + int ret; >> + >> + /* if an MDIO node is present, it should contain the PHY nodes */ >> + mdio_node = of_get_child_by_name(np, "mdio"); >> + if (mdio_node) { >> + ret = of_mdiobus_register(bp->mii_bus, mdio_node); >> + of_node_put(mdio_node); >> + return ret; >> + } > > This does take care of registering the MDIO bus controller when present > as a sub-node, however if you also plan on making use of fixed-link, we > will have already returned. > >> >> if (of_phy_is_fixed_link(np)) >> return mdiobus_register(bp->mii_bus); >> > > Really not sure what this is achieving, because we start off assuming > that we have an OF driven configuration, but later on we register the > MDIO bus with mdiobus_register() (and not of_mdiobus_register()), so no > scanning of the MDIO bus will happen. I agree that returning mdiobus_register() here makes no sense since there are no other PHY devices to scan for and probe. I can replace this with NULL. The reason for fixed-link check here is to skip the following loop: for_each_available_child_of_node(np, child) if (of_mdiobus_child_is_phy(child)) { of_node_put(child); return of_mdiobus_register(bp->mii_bus, np); } of_mdiobus_child_is_phy() returns true when it finds the fixed-link node, because it has no compatible. > > How does the driver currently support being provided a fixed-link > property? Should not we at least have this pattern: > > */ > if (of_phy_is_fixed_link(dn)) { > ret = of_phy_register_fixed_link(dn); > if (ret) > return ret; > > priv->phy_dn = dn; > } > > It does not look like you are breaking anything here, because it does > not look like this works at all. From what I understand, the new phylink(_create) handles the fixed-link case, so there is not reason to explicitly register the fixed-link. Best regards, Codrin > -- > Florian >
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 89fe7af5e408..cb0b3637651c 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -740,6 +740,16 @@ static int macb_mii_probe(struct net_device *dev) static int macb_mdiobus_register(struct macb *bp) { struct device_node *child, *np = bp->pdev->dev.of_node; + struct device_node *mdio_node; + int ret; + + /* if an MDIO node is present, it should contain the PHY nodes */ + mdio_node = of_get_child_by_name(np, "mdio"); + if (mdio_node) { + ret = of_mdiobus_register(bp->mii_bus, mdio_node); + of_node_put(mdio_node); + return ret; + } if (of_phy_is_fixed_link(np)) return mdiobus_register(bp->mii_bus);
The MACB embeds an MDIO bus controller. For this reason, the PHY nodes were represented as sub-nodes in the MACB node. Generally, the Ethernet controller is different than the MDIO controller, so the PHYs are probed by a separate MDIO driver. Since adding the PHY nodes directly under the ETH node became deprecated, we adjust the MACB driver to look for an MDIO node and register the subnode MDIO devices. Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- Changes in v3: - moved the check for the mdio node at the beginnging of macb_mdiobus_register(). This way, the mdio devices will be probed even if macb is a fixed-link Changes in v2: - readded newline removed by mistake; drivers/net/ethernet/cadence/macb_main.c | 10 ++++++++++ 1 file changed, 10 insertions(+)