Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

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

On 04/24/2017 02:04 AM, Roger Quadros wrote:
> On 24/04/17 02:35, Andrew Lunn wrote:
>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>> new file mode 100644
>>>> index 0000000..4ffbbac
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>> @@ -0,0 +1,33 @@
>>>> +Common MDIO bus properties.
>>>> +
>>>> +These are generic properties that can apply to any MDIO bus.
>>>> +
>>>> +Optional properties:
>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>> +  of the PHYs on that MDIO bus.
>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>> +
>>>> +A list of child nodes, one per device on the bus is expected. These
>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>> +
>>>> +Example :
>>>> +This example shows these optional properties, plus other properties
>>>> +required for the TI Davinci MDIO driver.
>>>> +
>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>> +		compatible = "ti,davinci_mdio";
>>>> +		reg = <0x5c030000 0x1000>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>
>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>> node rather than of the MDIO controller node (which might have a reset on
>>> its own)?
>>>> +
>>>> +		ethphy0: ethernet-phy@1 {
>>>> +			reg = <1>;
>>>> +		};
>>>> +
>>>> +		ethphy1: ethernet-phy@3 {
>>>> +			reg = <3>;
>>>> +		};
>>
>> Hi Lars-Peter
>>
>> We discussed this when the first proposal was made. There are two
>> cases, to consider.
>>
>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>> example, two PHYs.
>>
>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>> say, the reset line should probably be considered a PHY property, not
>> an MDIO property. However, it can be messy, since in order to probe
>> the MDIO bus, you probably need to take the PHY out of reset.
>>
>> Anyway, this patch addresses the first case, so should be accepted. If
>> anybody wants to address the second case, they are free to do so.
> 
> Thanks for the explanation Andrew.
> 
> For the second case, even if the RESET GPIO property is specified
> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
> else the PHY might not be probed at all.
> 
> Whether we need additional code to just to make the DT look prettier is
> questionable and if required can come as a separate patch.

Well, it's not about prettier vs. uglier, it's about correct vs.
incorrect. The binding document you propose here is correct for a single
reset line controlling all PHYs, and that's why such a reset line needs
to be placed at the MDIO controller level, because it's a property of
such a node.

If you need to support individual reset lines per-PHY, then there should
be some kind of amendment to the Ethernet PHY Device Tree binding
document which specifies optional reset-gpio properties for these nodes.

Until that happens, I think your v4 is good to go.
-- 
Florian