Re: [PATCH net-next v3 3/7] net: macb: parse PHY nodes found under an MDIO node
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
>> ---
>>
>> 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
>
Re: [PATCH net-next v3 3/7] net: macb: parse PHY nodes found under an MDIO node
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
> ---
>
> 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.
--
Florian
[PATCH net-next v3 3/7] net: macb: parse PHY nodes found under an MDIO node
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
---
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;
+ }
if (of_phy_is_fixed_link(np))
return mdiobus_register(bp->mii_bus);
--
2.25.1
