Re: [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type

[Florian Fainelli <f.fainelli@gmail.com>]

On 14/07/15 10:13, Stas Sergeev wrote:
> 
> Currently the PHY management type is selected by the MAC driver arbitrary.
> The decision is based on the presence of the "fixed-link" node and on a
> will of the driver's authors.
> This caused a regression recently, when mvneta driver suddenly started
> to use the in-band status for auto-negotiation on fixed links.
> It appears the auto-negotiation may not work when expected by the MAC driver.
> Sebastien Rannou explains:
> << Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's
> a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with
> inband status) is connected to the switch through QSGMII, and in this context
> we are on the media side of the PHY. >>
> https://lkml.org/lkml/2015/7/10/206
> 
> This patch introduces the new string property 'managed' that allows
> the user to set the management type explicitly.
> The supported values are:
> "auto" - default. Uses either MDIO or nothing, depending on the presence
> of the fixed-link node
> "mdio" - use MDIO
> "in-band-status" - use in-band status
> "none" - use fixed-link description

I thought we agreed in the last thread that "mdio" was implied whenever
a proper PHY phandle to a device sitting on MDIO bus is used and that
"auto" did not make much sense unless we were also describing how to do
this auto-negotiation completely?

The way I see it, the only thing that is needed at this point is an
"in-band-status" property which you rightfully placed at the Ethernet
MAC DT node level, this is fine, however, I disagree with how the values
are enforced, see below:

> 
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> 
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Pawel Moll <pawel.moll@arm.com>
> CC: Mark Rutland <mark.rutland@arm.com>
> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
> CC: Kumar Gala <galak@codeaurora.org>
> CC: Florian Fainelli <f.fainelli@gmail.com>
> CC: Grant Likely <grant.likely@linaro.org>
> CC: devicetree@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: netdev@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/net/ethernet.txt |  5 ++++
>  drivers/of/of_mdio.c                               | 28 ++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> index 3fc3605..23743e9 100644
> --- a/Documentation/devicetree/bindings/net/ethernet.txt
> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> @@ -19,7 +19,12 @@ The following properties are common to the Ethernet controllers:
>  - phy: the same as "phy-handle" property, not recommended for new bindings.
>  - phy-device: the same as "phy-handle" property, not recommended for new
>    bindings.
> +- managed: string, specifies the PHY management type. Supported values are:
> +  "auto", "mdio", "in-band-status", "none". "auto" is the default, and it
> +  sets the management type to either "mdio" or "none", depending on the
> +  presence of the "fixed-link" child node.

As mentioned below, "mdio" is redundant with finding a "phy-handle", and
"auto" is not specific enough to be useful to a consumer of this
information.

> 
>  Child nodes of the Ethernet controller are typically the individual PHY devices
>  connected via the MDIO bus (sometimes the MDIO bus controller is separate).
>  They are described in the phy.txt file in this same directory.
> +For non-MDIO PHY management see fixed-link.txt.
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 1bd4305..a460812 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -262,7 +262,16 @@ EXPORT_SYMBOL(of_phy_attach);
>  bool of_phy_is_fixed_link(struct device_node *np)
>  {
>  	struct device_node *dn;
> -	int len;
> +	int len, m_err;
> +	const char *managed;
> +
> +	m_err = of_property_read_string(np, "managed", &managed);
> +	if (m_err == 0) {
> +		if (strcmp(managed, "none") == 0)
> +			return true;
> +		if (strcmp(managed, "mdio") == 0)
> +			return false;
> +	}

managed = "mdio" on a fixed link is by definition not something remotely
correct, there should be a proper PHY driver for this, and therefore a
different representation: a PHY phandle to a PHY node on a MDIO bus. I
do not think enforcing this has been incorrect provides much value,
since you ought to write correct DT in the first place.

> 
>  	/* New binding */
>  	dn = of_get_child_by_name(np, "fixed-link");
> @@ -271,6 +280,9 @@ bool of_phy_is_fixed_link(struct device_node *np)
>  		return true;
>  	}
> 
> +	if (m_err == 0 && strcmp(managed, "auto") != 0)
> +		return true;
> +
>  	/* Old binding */
>  	if (of_get_property(np, "fixed-link", &len) &&
>  	    len == (5 * sizeof(__be32)))
> @@ -285,8 +297,20 @@ int of_phy_register_fixed_link(struct device_node *np)
>  	struct fixed_phy_status status = {};
>  	struct device_node *fixed_link_node;
>  	const __be32 *fixed_link_prop;
> -	int len;
> +	int len, err;
>  	struct phy_device *phy;
> +	const char *managed;
> +
> +	err = of_property_read_string(np, "managed", &managed);
> +	if (err == 0) {
> +		if (strcmp(managed, "mdio") == 0)
> +			return -EINVAL;
> +		if (strcmp(managed, "in-band-status") == 0) {
> +			status.link = 0;
> +			phy = fixed_phy_register(PHY_POLL, &status, np);
> +			return IS_ERR(phy) ? PTR_ERR(phy) : 0;
> +		}

And that is the only hunk of this patch that is really needed and useful
to solving the problem here.
-- 
Florian