Date:	Wed, 18 Sep 2013 10:21:11 +0100
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Lior Amsalem <alior@...vell.com>,
	Mark Rutland <mark.rutland@....com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Christian Gmeiner <christian.gmeiner@...il.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs

Hello,

2013/9/18 Grant Likely <grant.likely@...retlab.ca>:
> On Fri,  6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni@...e-electrons.com> wrote:
>> Some Ethernet MACs have a "fixed link", and are not connected to a
>> normal MDIO-managed PHY device. For those situations, a Device Tree
>> binding allows to describe a "fixed link" using a special PHY node.
>>
>> This patch adds:
>>
>>  * A documentation for the fixed PHY Device Tree binding.
>>
>>  * An of_phy_is_fixed_link() function that an Ethernet driver can call
>>    on its PHY phandle to find out whether it's a fixed link PHY or
>>    not. It should typically be used to know if
>>    of_phy_register_fixed_link() should be called.
>>
>>  * An of_phy_register_fixed_link() function that instantiates the
>>    fixed PHY into the PHY subsystem, so that when the driver calls
>>    of_phy_connect(), the PHY device associated to the OF node will be
>>    found.
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
>
> Hi Thomas,
>
> The implemenation in this series looks like it is in good shape, so I'll
> restrict my comments to be binding...
>
>> ---
>>  .../devicetree/bindings/net/fixed-link.txt         | 34 ++++++++++++++++++++++
>>  drivers/of/of_mdio.c                               | 24 +++++++++++++++
>>  include/linux/of_mdio.h                            | 15 ++++++++++
>>  3 files changed, 73 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
>> new file mode 100644
>> index 0000000..9f2a1a50
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
>> @@ -0,0 +1,34 @@
>> +Fixed link Device Tree binding
>> +------------------------------
>> +
>> +Some Ethernet MACs have a "fixed link", and are not connected to a
>> +normal MDIO-managed PHY device. For those situations, a Device Tree
>> +binding allows to describe a "fixed link".
>> +
>> +Such a fixed link situation is described by creating a PHY node as a
>> +sub-node of an Ethernet device, with the following properties:
>> +
>> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
>> +  fixed link PHY.
>> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted
>> +  values are 10, 100 and 1000
>> +* 'full-duplex' (boolean, optional), to indicate that full duplex is
>> +  used. When absent, half duplex is assumed.
>> +* 'pause' (boolean, optional), to indicate that pause should be
>> +  enabled.
>> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should
>> +  be enabled.
>
> I understand what you're trying to do here, but it causes a troublesome
> leakage of implementation detail into the binding, making the whole
> thing look very odd. This binding tries to make a fixed link look
> exactly like a real PHY even to the point of including a phandle to the
> phy. But having a phandle to a node which is *always* a direct child of
> the MAC node is redundant and a rather looney. Yes, doing it that way
> makes it easy for of_phy_find_device() to be transparent for fixed link,
> but that should *not* drive bindings, especially when that makes the
> binding really rather weird.

This is not exactly true in the sense that the "new" binding just
re-shuffles the properties representation into something that is
clearer and more extendible but there is not much difference in the
semantics.

>
> Second, this new binding doesn't provide anything over and above the
> existing fixed-link binding. It may not be pretty, but it is
> estabilshed.

In fact it does, the old one is obscure and not easily extendable
because we rely on an integer array to represent the various
properties, so at least this new one makes it easy to extend the
binding to support a possibly new fixed-link property. Being able to
deprecate a fundamentaly badly designed binding should still be a
prerogative, software is flexible and can deal with both with little
cost.

>
> That said, I do agree that the current Linux implementation is not good
> because it cannot handle a fixed-link property transparently. That's a
> deficiency in the Linux implementation and it should be fixed.
> of_phy_connect() currently requires the phy phandle to be passed in.
> Part of the reason it was done this way is that some drivers connect to
> multiple 'phys'. A soulition could be to make the phy handle optional.
> If it is empty then go looking for either a phy-device or fixed-link
> property. Otherwise use the provided node.

I do not quite follow you on this one, and I fear we might be leaking
some Linux probing heuristic into Device Tree bindings by implicitely
saying "not including a PHY phandle means connecting to a fixed-PHY
link." This would also dramatically change the current behavior for
most drivers where they might refuse probing if no corresponding PHY
device node is present and they are not designed to connect to a fixed
PHY one.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists