Re: [PATCH net-next 2/6] net: fec: register a fixed phy using fixed_phy_register_100fd if needed

[Heiner Kallweit <hkallweit1@gmail.com>]

On 11/3/2025 4:15 AM, Greg Ungerer wrote:
> Hi Heiner,
> 
> On 31/10/25 07:42, Heiner Kallweit wrote:
>> In case of coldfire/5272 a fixed phy is used, which so far is created
>> by platform code, using fixed_phy_add(). This function has a number of
>> problems, therefore create a potentially needed fixed phy here, using
>> fixed_phy_register_100fd.
>>
>> Note 1: This includes a small functional change, as coldfire/5272
>> created a fixed phy in half-duplex mode. Likely this was by mistake,
>> because the fec MAC is 100FD-capable, and connection is to a switch.
>>
>> Note 2: Usage of phy_find_next() makes use of the fact that dev_id can
>> only be 0 or 1.
>>
>> Due to lack of hardware, this is compile-tested only.
> 
> I did not get net-next and try this all out, but it doesn't compile when
> applied to 6.18-rc4.
> 
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>   drivers/net/ethernet/freescale/Kconfig    |  1 +
>>   drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------
>>   2 files changed, 27 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
>> index bbef47c34..e2a591cf9 100644
>> --- a/drivers/net/ethernet/freescale/Kconfig
>> +++ b/drivers/net/ethernet/freescale/Kconfig
>> @@ -28,6 +28,7 @@ config FEC
>>       depends on PTP_1588_CLOCK_OPTIONAL
>>       select CRC32
>>       select PHYLIB
>> +    select FIXED_PHY if M5272
> 
> Does it make sense to limit this to 5272 only here?
> I get that this most closely keeps functionality same as before.
> 
On DT-based systems FIXED_PHY is selected by OF_MDIO.
For non-DT systems I limited this to M5272, because it provides the
same functionality as before, and reduces object code size a little
on other platforms due to using fixed_phy function stubs.

> Regards
> Greg
> 
> 
> 
>>       select PAGE_POOL
>>       imply PAGE_POOL_STATS
>>       imply NET_SELFTESTS
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index c60ed8bac..0b71e4c15 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -52,6 +52,7 @@
>>   #include <linux/of_net.h>
>>   #include <linux/phy.h>
>>   #include <linux/pinctrl/consumer.h>
>> +#include <linux/phy_fixed.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/prefetch.h>
>> @@ -2476,11 +2477,8 @@ static int fec_enet_parse_rgmii_delay(struct fec_enet_private *fep,
>>   static int fec_enet_mii_probe(struct net_device *ndev)
>>   {
>>       struct fec_enet_private *fep = netdev_priv(ndev);
>> -    struct phy_device *phy_dev = NULL;
>> -    char mdio_bus_id[MII_BUS_ID_SIZE];
>> -    char phy_name[MII_BUS_ID_SIZE + 3];
>> -    int phy_id;
>> -    int dev_id = fep->dev_id;
>> +    struct phy_device *phy_dev;
>> +    int ret;
>>         if (fep->phy_node) {
>>           phy_dev = of_phy_connect(ndev, fep->phy_node,
>> @@ -2492,30 +2490,28 @@ static int fec_enet_mii_probe(struct net_device *ndev)
>>           }
>>       } else {
>>           /* check for attached phy */
>> -        for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {
>> -            if (!mdiobus_is_registered_device(fep->mii_bus, phy_id))
>> -                continue;
>> -            if (dev_id--)
>> -                continue;
>> -            strscpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
>> -            break;
>> -        }
>> +        phy_dev = phy_find_first(fep->mii_bus);
>> +        if (fep->dev_id && phy_dev)
>> +            phy_dev = phy_find_next(fep->mii_bus, phy_dev);
>>   -        if (phy_id >= PHY_MAX_ADDR) {
>> +        if (!phy_dev) {
>>               netdev_info(ndev, "no PHY, assuming direct connection to switch\n");
>> -            strscpy(mdio_bus_id, "fixed-0", MII_BUS_ID_SIZE);
>> -            phy_id = 0;
>> +            phy_dev = fixed_phy_register_100fd();
>> +            if (IS_ERR(phy_dev)) {
>> +                netdev_err(ndev, "could not register fixed PHY\n");
>> +                return PTR_ERR(phy_dev);
>> +            }
>>           }
>>   -        snprintf(phy_name, sizeof(phy_name),
>> -             PHY_ID_FMT, mdio_bus_id, phy_id);
>> -        phy_dev = phy_connect(ndev, phy_name, &fec_enet_adjust_link,
>> -                      fep->phy_interface);
>> -    }
>> +        ret = phy_connect_direct(ndev, phy_dev, &fec_enet_adjust_link,
>> +                     fep->phy_interface);
>> +        if (ret) {
>> +            if (phy_is_pseudo_fixed_link(phy_dev))
>> +                fixed_phy_unregister(phy_dev);
>> +            netdev_err(ndev, "could not attach to PHY\n");
>> +            return ret;
>> +        }
>>   -    if (IS_ERR(phy_dev)) {
>> -        netdev_err(ndev, "could not attach to PHY\n");
>> -        return PTR_ERR(phy_dev);
>>       }
>>         /* mask with MAC supported features */
>> @@ -3622,8 +3618,9 @@ static int
>>   fec_enet_close(struct net_device *ndev)
>>   {
>>       struct fec_enet_private *fep = netdev_priv(ndev);
>> +    struct phy_device *phy_dev = ndev->phydev;
>>   -    phy_stop(ndev->phydev);
>> +    phy_stop(phy_dev);
>>         if (netif_device_present(ndev)) {
>>           napi_disable(&fep->napi);
>> @@ -3631,7 +3628,10 @@ fec_enet_close(struct net_device *ndev)
>>           fec_stop(ndev);
>>       }
>>   -    phy_disconnect(ndev->phydev);
>> +    phy_disconnect(phy_dev);
>> +
>> +    if (!fep->phy_node && phy_is_pseudo_fixed_link(phy_dev))
>> +        fixed_phy_unregister(phy_dev);
>>         if (fep->quirks & FEC_QUIRK_ERR006687)
>>           imx6q_cpuidle_fec_irqs_unused();
>