Date:   Sat, 28 Dec 2019 14:17:23 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>
Cc:     Alexandru Marginean <alexandru.marginean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, netdev <netdev@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v3 net-next 05/11] net: phy: Add a property for
 controlling in-band auto-negotiation

On Fri, 27 Dec 2019 at 23:36, Vladimir Oltean <olteanv@...il.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> This patch aims to solve a simple problem: for SerDes interfaces, the
> MAC has a PCS layer which serializes the data before passing it to the
> link partner (typically a PHY with its own PCS, but not necessarily, it
> can also be a remote PHY).
>
> Some of these SerDes protocols have a configuration word that is
> transmitted in-band, to establish the link parameters. Some SerDes
> implementations strictly require this handshake (in-band AN) to complete
> before passing data, while on others it may be bypassed.
>
> From a user perspective, the top-most entity of this in-band AN
> relationship is the MAC PCS, driven by an Ethernet driver.
> When the MAC PCS operates as PHY-less, there is little that can be done
> from our side to prevent a settings mismatch, since the link partner may
> be remote and outside our control.
> When the MAC PCS is connected to a PHY driven by the PHY library, that
> is when we want to ensure the in-band AN settings are in sync and can be
> fulfilled by both parties.
>
> This setting is ternary and requested by the MAC PCS driver. For
> compatibility with existing code, we introduce a IN_BAND_AN_DEFAULT
> state equal to 0, which means that the MAC driver, caller of the PHY
> library, has no opinion about in-band AN settings. It is assumed that
> somebody else has taken care of this setting and nothing should be done.
>
> When the PHYLIB caller requests an explicit setting (IN_BAND_AN_ENABLED
> or IN_BAND_AN_DISABLED), the PHY driver must veto this operation in its
> .config_init callback if it can't do as requested. As mentioned, this is
> to avoid mismatches, which will be taken to result in failure to pass
> data between MAC and PHY.
>
> As for the caller of PHYLIB, it shouldn't hardcode any particular value
> for phydev->in_band_autoneg, but rather take this information from a
> board description file such as device tree. This gives the caller a
> chance to veto the setting as well, if it doesn't support it, and it
> leaves the choice at the level of individual MAC-PHY instances instead
> of drivers. A more-or-less standard device tree binding that can be used
> to gather this information is:
>         managed = "in-band-status";
> which PHYLINK already uses.
>
> Make PHYLINK the first user of this scheme, by parsing the DT binding
> mentioned above and passing it to the PHY library.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Changes in v3:
> - Patch is new.
>
>  drivers/net/phy/phylink.c |  2 ++
>  include/linux/phy.h       | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 73a01ea5fc55..af574d5a8426 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -878,6 +878,8 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>         if (!phy_dev)
>                 return -ENODEV;
>
> +       phy_dev->in_band_autoneg = (pl->cfg_link_an_mode == MLO_AN_INBAND);
> +

Oops, I sent an intermediary version of this patch, from when
in_band_autoneg was binary and not ternary.
So this should read:
        if (pl->cfg_link_an_mode == MLO_AN_INBAND)
                phy_dev->in_band_autoneg = IN_BAND_AN_ENABLED;
        else
                phy_dev->in_band_autoneg = IN_BAND_AN_DISABLED;

Will correct when I accumulate enough feedback for v4.

>         ret = phy_attach_direct(pl->netdev, phy_dev, flags,
>                                 pl->link_interface);
>         if (ret)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 30e599c454db..090e4ba303e2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -318,6 +318,22 @@ enum phy_state {
>         PHY_NOLINK,
>  };
>
> +/* Settings for system-side PHY auto-negotiation:
> + * - IN_BAND_AN_DEFAULT: the PHY is left in the default configuration, be it
> + *   out-of-reset, preconfigured by boot firmware, etc. In-band AN can be
> + *   disabled, enabled or even unsupported when this setting is on.
> + * - IN_BAND_AN_ENABLED: the PHY must enable system-side auto-negotiation with
> + *   the attached MAC, or veto this setting if it can't.
> + * - IN_BAND_AN_DISABLED: the PHY must disable or bypass system-side
> + *   auto-negotiation with the attached MAC (and force the link settings if
> + *   applicable), or veto this setting if it can't.
> + */
> +enum phy_in_band_autoneg {
> +       IN_BAND_AN_DEFAULT = 0,
> +       IN_BAND_AN_ENABLED,
> +       IN_BAND_AN_DISABLED,
> +};
> +
>  /**
>   * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
>   * @devices_in_package: Bit vector of devices present.
> @@ -388,6 +404,8 @@ struct phy_device {
>         /* Interrupts are enabled */
>         unsigned interrupts:1;
>
> +       enum phy_in_band_autoneg in_band_autoneg;
> +
>         enum phy_state state;
>
>         u32 dev_flags;
> --
> 2.17.1
>

Thanks,
-Vladimir

Powered by blists - more mailing lists