Message-ID: <CAK+owoid1woDTiCxcGiEmdvKNHJeCnaKBjPEHyNrtHt_hKqi9g@mail.gmail.com>
Date: Thu, 15 May 2025 00:29:48 +0200
From: Stefano Radaelli <stefano.radaelli21@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Xu Liang <lxu@...linear.com>
Subject: Re: [PATCH v2] net: phy: mxl-8611: add support for MaxLinear
 MxL86110/MxL86111 PHY

Hi Andrew,

Thanks again for your detailed review and suggestions,
I really appreciate the time you took to go through the patch.

After reviewing your feedback, I realized that most of the more complex issues
are specific to the MXL86111. The MXL86110 side, on the other hand, seems much
closer to being ready once I address the points you raised.

Would you be open to me submitting a revised version of the driver
with support for the
MXL86110 only, for now? This would allow me to properly support the
PHYs I'm currently
using on Variscite boards, while taking the necessary time to rework
and structure the MXL86111
support for a future submission.

Please let me know if that sounds reasonable.

Thanks again,

Stefano


Il giorno mer 14 mag 2025 alle ore 20:49 Andrew Lunn <andrew@...n.ch>
ha scritto:
>
> On Wed, May 14, 2025 at 07:35:06PM +0200, Stefano Radaelli wrote:
> > The MaxLinear MxL86110 is a low power Ethernet PHY transceiver IC
> > compliant with the IEEE 802.3 Ethernet standard. It offers a
> > cost-optimized solution suitable for routers, switches, and home
> > gateways, supporting 1000, 100, and 10 Mbit/s data rates over
> > CAT5e or higher twisted pair copper cable.
> >
> > The driver for this PHY is based on initial code provided by MaxLinear
> > (MaxLinear Driver V1.0.0).
> >
> > Supported features:
> > ----------------------------------------+----------+----------+
> > Feature                                | MxL86110 | MxL86111 |
> > ----------------------------------------+----------+----------+
> > General Device Initialization          |    x     |    x     |
> > Wake on LAN (WoL)                      |    x     |    x     |
> > LED Configuration                      |    x     |    x     |
> > RGMII Interface Configuration          |    x     |    x     |
> > SyncE Clock Output Config              |    x     |    x     |
> > Dual Media Mode (Fiber/Copper)         |    -     |    x     |
> > ----------------------------------------+----------+----------+
> >
> > This driver was tested on a Variscite i.MX93-VAR-SOM and
> > a i.MX8MP-VAR-SOM. LEDs configuration were tested using
> > /sys/class/leds interface
> >
> > Changes since v1:
> > - Rewrote the driver following the PHY subsystem documentation
> >   and the style of existing PHY drivers
> > - Removed all vendor-specific code and unnecessary workarounds
> > - Reimplemented LED control using the generic /sys/class/leds interface
>
> Please start a new thread for each new version.
>
> > +static int mxlphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> > +{
> > +     struct net_device *netdev;
> > +     int page_to_restore;
> > +     const u8 *mac;
> > +     int ret = 0;
> > +
> > +     if (wol->wolopts & WAKE_MAGIC) {
> > +             netdev = phydev->attached_dev;
> > +             if (!netdev)
> > +                     return -ENODEV;
> > +
> > +             page_to_restore = phy_select_page(phydev, MXL86110_DEFAULT_PAGE);
> > +             if (page_to_restore < 0)
> > +                     goto error;
> > +
> > +             /* Configure the MAC address of the WOL magic packet */
> > +             mac = (const u8 *)netdev->dev_addr;
> > +             ret = mxlphy_write_extended_reg(phydev, MXL86110_WOL_MAC_ADDR_HIGH_EXTD_REG,
> > +                                             ((mac[0] << 8) | mac[1]));
>
> ...
>
> > +error:
> > +     return phy_restore_page(phydev, page_to_restore, ret);
> > +}
> > +
> > +/**
> > + * mxl8611x_enable_leds_activity_blink() - Enable activity blink on all PHY LEDs
> > + * @phydev: pointer to the PHY device structure
> > + *
> > + * Configure all PHY LEDs to blink on traffic activity regardless of their
> > + * ON or OFF state. This behavior allows each LED to serve as a pure activity
> > + * indicator, independently of its use as a link status indicator.
> > + *
> > + * By default, each LED blinks only when it is also in the ON state. This function
> > + * modifies the appropriate registers (LABx fields) to enable blinking even
> > + * when the LEDs are OFF, to allow the LED to be used as a traffic indicator
> > + * without requiring it to also serve as a link status LED.
> > + *
> > + * NOTE: Any further LED customization can be performed via the
> > + * /sys/class/led interface; the functions led_hw_is_supported, led_hw_control_get, and
> > + * led_hw_control_set are used to support this mechanism.
> > + *
> > + * Return: 0 on success or a negative error code.
> > + */
> > +static int mxl8611x_enable_led_activity_blink(struct phy_device *phydev)
> > +{
> > +     u16 val;
> > +     int ret, index;
> > +
> > +     for (index = 0; index < MXL8611x_MAX_LEDS; index++) {
> > +             val = mxlphy_read_extended_reg(phydev, MXL8611X_LED0_CFG_REG + index);
> > +             if (val < 0)
> > +                     return val;
> > +
> > +             val |= MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND;
> > +
> > +             ret = mxlphy_write_extended_reg(phydev, MXL8611X_LED0_CFG_REG + index, val);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> mxlphy_set_wol() swaps to the default page before making use of
> mxlphy_write_extended_reg(). Here you don't. What are the rules?
>
> > +/**
> > + * mxl8611x_broadcast_cfg() - applies broadcast configuration
> > + * @phydev: pointer to the phy_device
> > + *
> > + * configures the broadcast setting for the PHY based on the device tree
> > + * if the "mxl-8611x,broadcast-enabled" property is present the PHY broadcasts
> > + * address 0 on the MDIO bus. This feature enables PHY to always respond to MDIO access
> > + * returns 0 or negative errno code
> > + */
> > +static int mxl8611x_broadcast_cfg(struct phy_device *phydev)
> > +{
> > +     int ret = 0;
> > +     struct device_node *node;
> > +     u32 val;
> > +
> > +     if (!phydev) {
> > +             pr_err("%s, Invalid phy_device pointer\n", __func__);
> > +             return -EINVAL;
> > +     }
>
> How would that happen?
>
> We avoid defensive code like this, you should spend the time to
> understand all the ways you can get to here, and avoid passing a NULL
> pointer.
>
> > +
> > +     node = phydev->mdio.dev.of_node;
> > +     if (!node) {
> > +             phydev_err(phydev, "%s, Invalid device tree node\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     val = mxlphy_read_extended_reg(phydev, MXL8611X_EXT_RGMII_MDIO_CFG);
> > +
> > +     if (of_property_read_bool(node, "mxl-8611x,broadcast-enabled"))
> > +             val |= MXL8611X_EXT_RGMII_MDIO_CFG_EPA0_MASK;
> > +     else
> > +             val &= ~MXL8611X_EXT_RGMII_MDIO_CFG_EPA0_MASK;
>
> In my previous review i said drop this. Just hard code broadcast
> disabled.
>
> > +/**
> > + * mxl86110_config_init() - initialize the PHY
> > + * @phydev: pointer to the phy_device
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxl86110_config_init(struct phy_device *phydev)
> > +{
> > +     int page_to_restore, ret = 0;
> > +     unsigned int val = 0;
> > +
> > +     page_to_restore = phy_select_page(phydev, MXL86110_DEFAULT_PAGE);
> > +     if (page_to_restore < 0)
> > +             goto err_restore_page;
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val = MXL8611X_EXT_RGMII_CFG1_RX_DELAY_DEFAULT;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_TXID:
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val = MXL8611X_EXT_RGMII_CFG1_TX_1G_DELAY_DEFAULT |
> > +                             MXL8611X_EXT_RGMII_CFG1_TX_10MB_100MB_DEFAULT;
> > +             val |= MXL8611X_EXT_RGMII_CFG1_RX_DELAY_DEFAULT;
> > +             break;
>
> This looks wrong. The four RGMII modes should require different
> values. Also, if delays are added, they should be 2ns. Don't use
> _DEFAULT, use a name to indicate it is 2ns.
>
> > +struct mxl86111_priv {
> > +     /* dual_media_advertising used for Dual Media mode (MXL86111_EXT_SMI_SDS_PHY_AUTO) */
> > +     __ETHTOOL_DECLARE_LINK_MODE_MASK(dual_media_advertising);
> > +
> > +     /* MXL86111_MODE_FIBER / MXL86111_MODE_UTP / MXL86111_MODE_AUTO*/
> > +     u8 reg_page_mode;
> > +     u8 strap_mode; /* 8 working modes  */
> > +     /* current reg page of mxl86111 phy:
> > +      * MXL86111_EXT_SMI_SDS_PHYUTP_SPACE
> > +      * MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE
> > +      * MXL86111_EXT_SMI_SDS_PHY_AUTO
> > +      */
> > +     u8 reg_page;
> > +};
> > +
> > +/**
> > + * mxl86111_read_page() - read reg page
> > + * @phydev: pointer to the phy_device
> > + *
> > + * returns current reg space of mxl86111 (MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE/
> > + * MXL86111_EXT_SMI_SDS_PHYUTP_SPACE) or negative errno code
> > + */
> > +static int mxl86111_read_page(struct phy_device *phydev)
> > +{
> > +     int old_page;
> > +
> > +     old_page = mxlphy_locked_read_extended_reg(phydev, MXL86111_EXT_SMI_SDS_PHY_REG);
> > +     if (old_page < 0)
> > +             return old_page;
> > +
> > +     if ((old_page & MXL86111_EXT_SMI_SDS_PHYSPACE_MASK) == MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE)
> > +             return MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE;
> > +
> > +     return MXL86111_EXT_SMI_SDS_PHYUTP_SPACE;
> > +};
> > +
> > +/**
> > + * mxl86111_write_page() - Set reg page
> > + * @phydev: pointer to the phy_device
> > + * @page: The reg page to set
> > + * (MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE/MXL86111_EXT_SMI_SDS_PHYUTP_SPACE)
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxl86111_write_page(struct phy_device *phydev, int page)
> > +{
> > +     int mask = MXL86111_EXT_SMI_SDS_PHYSPACE_MASK;
> > +     int set;
> > +
> > +     if ((page & MXL86111_EXT_SMI_SDS_PHYSPACE_MASK) == MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE)
> > +             set = MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE;
> > +     else
> > +             set = MXL86111_EXT_SMI_SDS_PHYUTP_SPACE;
> > +
> > +     return mxlphy_modify_extended_reg(phydev, MXL86111_EXT_SMI_SDS_PHY_REG, mask, set);
> > +};
>
> We need some sort of overview of all these different functions
> accessing pages, and how they interact with each other.
>
> > +
> > +/**
> > + * mxl86111_modify_bmcr_paged - modify bits of the PHY's BMCR register of a given page
> > + * @phydev: pointer to the phy_device
> > + * @page: The reg page to operate
> > + * (MXL86111_EXT_SMI_SDS_PHYFIBER_SPACE/MXL86111_EXT_SMI_SDS_PHYUTP_SPACE)
> > + * @mask: bit mask of bits to clear
> > + * @set: bit mask of bits to set
> > + *
> > + * NOTE: new register value = (old register value & ~mask) | set.
> > + * MxL86111 has 2 register spaces (utp/fiber) and 3 modes (utp/fiber/auto).
> > + * Each space has its MII_BMCR.
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxl86111_modify_bmcr_paged(struct phy_device *phydev, int page,
> > +                                   u16 mask, u16 set)
> > +{
> > +     int bmcr_timeout = BMCR_RESET_TIMEOUT;
> > +     int page_to_restore;
> > +     int ret = 0;
> > +
> > +     page_to_restore = phy_select_page(phydev, page & MXL86111_EXT_SMI_SDS_PHYSPACE_MASK);
> > +     if (page_to_restore < 0)
> > +             goto err_restore_page;
> > +
> > +     ret = __phy_modify(phydev, MII_BMCR, mask, set);
> > +     if (ret < 0)
> > +             goto err_restore_page;
> > +
> > +     /* In case of BMCR_RESET, check until reset bit is cleared */
> > +     if (set == BMCR_RESET) {
> > +             while (bmcr_timeout--) {
> > +                     usleep_range(1000, 1050);
> > +                     ret = __phy_read(phydev, MII_BMCR);
> > +                     if (ret < 0)
> > +                             goto err_restore_page;
> > +
> > +                     if (!(ret & BMCR_RESET))
> > +                             return phy_restore_page(phydev, page_to_restore, 0);
> > +             }
> > +             phydev_warn(phydev, "%s, BMCR reset not completed until timeout", __func__);
>
> Loops like this after often buggy. Please take a look at
> phy_poll_reset().
>
> Also, is there really two BMCR registers? Does resetting one not also
> perform a reset on the other?
>
> > +/**
> > + * mxl86111_set_fiber_features() -  setup fiber mode features.
> > + * @phydev: pointer to the phy_device
> > + * @dst: a pointer to store fiber mode features
> > + */
> > +static void mxl86111_set_fiber_features(struct phy_device *phydev,
> > +                                     unsigned long *dst)
> > +{
> > +     linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT, dst);
> > +     linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, dst);
> > +     linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, dst);
> > +     linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, dst);
>
> Normally you can read these from the Extended Status register. Is that
> register broken?
>
> > +}
> > +
> > +/**
> > + * mxlphy_utp_read_abilities - read PHY abilities from Clause 22 registers
> > + * @phydev: pointer to the phy_device
> > + *
> > + * NOTE: Read the PHY abilities and set phydev->supported.
> > + * The caller must have taken the MDIO bus lock.
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxlphy_utp_read_abilities(struct phy_device *phydev)
> > +{
>
> This appears to be mostly a copy of genphy_read_abilities(). Please
> try to use that helper. Sometimes you need to call it, and then fixup
> whatever is broken. It might also be you need to call it twice, with
> to different pages selected. The .get_features call is made very
> early, before the PHY is published, so you don't need to worry about
> something changing the page.
>
> > +static int mxl86111_probe(struct phy_device *phydev)
> > +{
> > +     struct device *dev = &phydev->mdio.dev;
> > +     struct mxl86111_priv *priv;
> > +     int chip_config;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     phydev->priv = priv;
> > +
> > +     // TM: Debugging of pinstrap mode
> > +     ret = mxlphy_locked_modify_extended_reg(phydev, MXL86111_EXT_CHIP_CFG_REG,
> > +                                             MXL86111_EXT_CHIP_CFG_MODE_SEL_MASK,
> > +                                             MXL86111_EXT_CHIP_CFG_MODE_UTP_TO_RGMII);
> > +     if (ret < 0)
> > +             return ret;
> > +     // TM: Debugging of pinstrap mode
> > +     ret = mxlphy_locked_modify_extended_reg(phydev, MXL86111_EXT_CHIP_CFG_REG,
> > +                                             MXL86111_EXT_CHIP_CFG_SW_RST_N_MODE, 0);
>
> What are these two modifies doing?
>
> > +/**
> > + * mxlphy_check_and_restart_aneg - Enable and restart auto-negotiation
> > + * @phydev: pointer to the phy_device
> > + * @restart: bool if aneg restart is requested
> > + *
> > + * NOTE: The caller must have taken the MDIO bus lock.
> > + *
> > + * identical to genphy_check_and_restart_aneg, but phy_read without mdio lock
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxlphy_check_and_restart_aneg(struct phy_device *phydev, bool restart)
> > +{
>
> This is genphy_check_and_restart_aneg().
>
> > +/**
> > + * mxlphy_config_advert - sanitize and advertise auto-negotiation parameters
> > + * @phydev: pointer to the phy_device
> > + *
> > + * NOTE: Writes MII_ADVERTISE with the appropriate values,
> > + * Returns < 0 on error, 0 if the PHY's advertisement hasn't changed,
> > + * and > 0 if it has changed.
> > + *
> > + * identical to genphy_config_advert, but phy_read without mdio lock
> > + *
> > + * NOTE: The caller must have taken the MDIO bus lock.
> > + *
> > + * returns 0 or negative errno code
> > + */
> > +static int mxlphy_config_advert(struct phy_device *phydev)
> > +{
>
> > +     int err, bmsr, changed = 0;
> > +     u32 adv;
> > +
> > +     /* Only allow advertising what this PHY supports */
> > +     linkmode_and(phydev->advertising, phydev->advertising,
> > +                  phydev->supported);
>
> The core should of already done that.
>
> > +
> > +     adv = linkmode_adv_to_mii_adv_t(phydev->advertising);
> > +
> > +     /* Setup standard advertisement */
> > +     err = __phy_modify_changed(phydev, MII_ADVERTISE,
> > +                                ADVERTISE_ALL | ADVERTISE_100BASE4 |
> > +                                ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM,
> > +                                adv);
> > +     if (err < 0)
> > +             return err;
> > +     if (err > 0)
> > +             changed = 1;
>
> And this looks to be genphy_config_advert(). Please look at how you
> can use these helpers. Does the marvell driver also have
> reimplementations of all these functions?
>
>         Andrew

Powered by blists - more mailing lists