Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 23.07.2020 21:59, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/21/20 10:13 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 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..b25c64b45148 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,10 +740,20 @@ 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 (of_phy_is_fixed_link(np))
>> return mdiobus_register(bp->mii_bus);
>
> Does not this need changing as well? Consider the use case of having
> your MACB Ethernet node have a fixed-link property to describe how it
> connects to a switch, and your MACB MDIO controller, expressed as a
> sub-node, describing the MDIO attached switch it connects to.
Just asking, does this worth having/changing it as long as there is no in
kernel board/configuration that could benefit off?
Thank you,
Claudiu Beznea
>
>>
>> + /* 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;
>> + }
>> +
>> /* 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
>>
>
>
> --
> Florian
>
Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 7/23/20 3:08 PM, codrin.ciubota...@microchip.com wrote:
> On 23.07.2020 21:59, Florian Fainelli wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 7/21/20 10:13 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 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..b25c64b45148 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -740,10 +740,20 @@ 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 (of_phy_is_fixed_link(np))
>>>return mdiobus_register(bp->mii_bus);
>>
>> Does not this need changing as well? Consider the use case of having
>> your MACB Ethernet node have a fixed-link property to describe how it
>> connects to a switch, and your MACB MDIO controller, expressed as a
>> sub-node, describing the MDIO attached switch it connects to.
>
> Right, this is what I was discussing with Claudiu on the other thread. I
> am thinking to just move the look for mdio before checking for
> fixed-link. This will probe the MDIO devices and simple mdiobus_register
> will be called only if the mdio node is missing.
Found it after I had sent this email. What you propose sounds
reasonable, looking forward to v3.
>
> Thank you for your review(s)!
Of course.
--
Florian
Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 23.07.2020 21:59, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/21/20 10:13 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 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..b25c64b45148 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,10 +740,20 @@ 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 (of_phy_is_fixed_link(np))
>>return mdiobus_register(bp->mii_bus);
>
> Does not this need changing as well? Consider the use case of having
> your MACB Ethernet node have a fixed-link property to describe how it
> connects to a switch, and your MACB MDIO controller, expressed as a
> sub-node, describing the MDIO attached switch it connects to.
Right, this is what I was discussing with Claudiu on the other thread. I
am thinking to just move the look for mdio before checking for
fixed-link. This will probe the MDIO devices and simple mdiobus_register
will be called only if the mdio node is missing.
Thank you for your review(s)!
Best regards,
Codrin
>
>>
>> + /* 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;
>> + }
>> +
>>/* 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
>>
>
>
> --
> Florian
>
Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 7/21/20 10:13 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 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..b25c64b45148 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -740,10 +740,20 @@ 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 (of_phy_is_fixed_link(np))
> return mdiobus_register(bp->mii_bus);
Does not this need changing as well? Consider the use case of having
your MACB Ethernet node have a fixed-link property to describe how it
connects to a switch, and your MACB MDIO controller, expressed as a
sub-node, describing the MDIO attached switch it connects to.
>
> + /* 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;
> + }
> +
> /* 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
>
--
Florian
[PATCH net-next v2 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 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..b25c64b45148 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -740,10 +740,20 @@ 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 (of_phy_is_fixed_link(np))
return mdiobus_register(bp->mii_bus);
+ /* 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;
+ }
+
/* 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
--
2.25.1
