Message-ID: <MWHPR11MB169364EFBC8FE61E0772A25BEF1C9@MWHPR11MB1693.namprd11.prod.outlook.com>
Date:   Fri, 9 Dec 2022 18:00:47 +0000
From:   <Jerry.Ray@...rochip.com>
To:     <olteanv@...il.com>
CC:     <andrew@...n.ch>, <f.fainelli@...il.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux@...linux.org.uk>
Subject: RE: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

Hi Vladimir,

> We discourage code movement which also changes the code in question.
> Code movement should be just that, movement. If you don't like this
> check and want to replace it with dsa_port_is_cpu(), do so in a
> preparatory patch.
> 

Understood.  Will do.


> As a reader, I find my intelligence insulted by self-evident comments such as this.
> 
> Especially in contrast with the writes below to the MII_BMCR of the
> Virtual PHY, which would certainly deserve a bit more of an explanation,
> yet there is none there.
> 

I struggle with the lack of comments I see in the kernel codebase. While
experts can look at the source code and understand it, I find I spend a
good deal of time chasing down macros - following data structures - and
reverse engineering an understanding of the purpose of something that could
have been explained in the maintained source.  In-line comments target the
unfamiliar reader as there are a lot of us out here and far too few experts.
But I do see your point and I'll try to do a better job on this.


> You actually had something good going in the previous patch. The action
> of disabling Turbo MII mode seems invariant of the negotiated link
> settings. So it would be better to do it just once, during switch setup.
> It would be good if you could add one more preparatory patch which does
> just that. Assuming, of course, that the reordering in register writes
> between LAN9303_VIRT_SPECIAL_CTRL and MII_BMCR will not cause a problem,
> and that the link still works.

I'll rework the patch series to break this change out.

Regards,
Jerry.

-----Original Message-----
From: Vladimir Oltean <olteanv@...il.com> 
Sent: Thursday, December 8, 2022 11:21 AM
To: Jerry Ray - C33025 <Jerry.Ray@...rochip.com>
Cc: Andrew Lunn <andrew@...n.ch>; Florian Fainelli <f.fainelli@...il.com>; David S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; netdev@...r.kernel.org; linux-kernel@...r.kernel.org; linux@...linux.org.uk
Subject: Re: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Wed, Dec 07, 2022 at 05:28:28PM -0600, Jerry Ray wrote:
> This patch replaces the adjust_link api with the phylink apis to provide
> equivalent functionality.
>
> The functionality from the adjust_link is moved to the phylink_mac_link_up
> api.  The code being removed only affected the cpu port.
>
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_up
>
> Signed-off-by: Jerry Ray <jerry.ray@...rochip.com>
> ---
> v3-> v4:
>   - Reworked the implementation to preserve the adjust_link functionality
>     by including it in the phylink_mac_link_up api.
> v2-> v3:
>   Added back in disabling Turbo Mode on the CPU MII interface.
>   Removed the unnecessary clearing of the phy supported interfaces.
> ---
>  drivers/net/dsa/lan9303-core.c | 123 +++++++++++++++++++++++----------
>  1 file changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index d9f7b554a423..a800448c9433 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1047,42 +1047,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>       return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>
> -static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> -                             struct phy_device *phydev)
> -{
> -     struct lan9303 *chip = ds->priv;
> -     int ctl;
> -
> -     if (!phy_is_pseudo_fixed_link(phydev))
> -             return;

We discourage code movement which also changes the code in question.
Code movement should be just that, movement. If you don't like this
check and want to replace it with dsa_port_is_cpu(), do so in a
preparatory patch.

> -
> -     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -
> -     ctl &= ~BMCR_ANENABLE;
> -
> -     if (phydev->speed == SPEED_100)
> -             ctl |= BMCR_SPEED100;
> -     else if (phydev->speed == SPEED_10)
> -             ctl &= ~BMCR_SPEED100;
> -     else
> -             dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
> -
> -     if (phydev->duplex == DUPLEX_FULL)
> -             ctl |= BMCR_FULLDPLX;
> -     else
> -             ctl &= ~BMCR_FULLDPLX;
> -
> -     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> -
> -     if (port == chip->phy_addr_base) {
> -             /* Virtual Phy: Remove Turbo 200Mbit mode */
> -             lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> -
> -             ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> -             regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> -     }
> -}
> -
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
>                              struct phy_device *phy)
>  {
> @@ -1279,13 +1243,98 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
>       return 0;
>  }
>
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> +                                  struct phylink_config *config)
> +{
> +     struct lan9303 *chip = ds->priv;
> +
> +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> +                                MAC_SYM_PAUSE;
> +
> +     if (dsa_is_cpu_port(ds, port)) {
> +             __set_bit(PHY_INTERFACE_MODE_RMII,
> +                       config->supported_interfaces);
> +             __set_bit(PHY_INTERFACE_MODE_MII,
> +                       config->supported_interfaces);
> +     } else {
> +             __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +                       config->supported_interfaces);
> +             /* Compatibility for phylib's default interface type when the
> +              * phy-mode property is absent
> +              */
> +             __set_bit(PHY_INTERFACE_MODE_GMII,
> +                       config->supported_interfaces);
> +     }
> +
> +     /* This driver does not make use of the speed, duplex, pause or the
> +      * advertisement in its mac_config, so it is safe to mark this driver
> +      * as non-legacy.
> +      */
> +     config->legacy_pre_march2020 = false;
> +}
> +
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +                                     unsigned int mode,
> +                                     phy_interface_t interface,
> +                                     struct phy_device *phydev, int speed,
> +                                     int duplex, bool tx_pause,
> +                                     bool rx_pause)
> +{
> +     struct lan9303 *chip = ds->priv;
> +     u32 ctl;
> +     int ret;
> +
> +     dev_dbg(chip->dev, "%s(%d) entered - %s.", __func__, port,
> +             phy_modes(interface));
> +
> +     /* if this is not the cpu port, then simply return. */

As a reader, I find my intelligence insulted by self-evident comments such as this.

Especially in contrast with the writes below to the MII_BMCR of the
Virtual PHY, which would certainly deserve a bit more of an explanation,
yet there is none there.

> +     if (!dsa_port_is_cpu(dsa_to_port(ds, port)))
> +             return;
> +
> +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +     ctl &= ~BMCR_ANENABLE;
> +
> +     if (speed == SPEED_100)
> +             ctl |= BMCR_SPEED100;
> +     else if (speed == SPEED_10)
> +             ctl &= ~BMCR_SPEED100;
> +     else
> +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> +
> +     if (duplex == DUPLEX_FULL)
> +             ctl |= BMCR_FULLDPLX;
> +     else
> +             ctl &= ~BMCR_FULLDPLX;
> +
> +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +
> +     if (port == chip->phy_addr_base) {
> +             /* Virtual Phy: Remove Turbo 200Mbit mode */
> +             ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +                                &ctl);
> +             if (ret)
> +                     return;
> +
> +             /* Clear the TURBO Mode bit if it was set. */
> +             if (ctl & LAN9303_VIRT_SPECIAL_TURBO) {
> +                     ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> +                     regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +                                  ctl);
> +             }
> +     }

You actually had something good going in the previous patch. The action
of disabling Turbo MII mode seems invariant of the negotiated link
settings. So it would be better to do it just once, during switch setup.
It would be good if you could add one more preparatory patch which does
just that. Assuming, of course, that the reordering in register writes
between LAN9303_VIRT_SPECIAL_CTRL and MII_BMCR will not cause a problem,
and that the link still works.

> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>       .get_tag_protocol       = lan9303_get_tag_protocol,
>       .setup                  = lan9303_setup,
>       .get_strings            = lan9303_get_strings,
>       .phy_read               = lan9303_phy_read,
>       .phy_write              = lan9303_phy_write,
> -     .adjust_link            = lan9303_adjust_link,
> +     .phylink_get_caps       = lan9303_phylink_get_caps,
> +     .phylink_mac_link_up    = lan9303_phylink_mac_link_up,
>       .get_ethtool_stats      = lan9303_get_ethtool_stats,
>       .get_sset_count         = lan9303_get_sset_count,
>       .port_enable            = lan9303_port_enable,
> --
> 2.17.1
>

Powered by blists - more mailing lists