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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c443bf0-03ac-cc1c-3776-a36287801e96@gmail.com>
Date:   Tue, 25 Apr 2017 09:31:01 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Lars-Peter Clausen <lars@...afoo.de>,
        Roger Quadros <rogerq@...com>, Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, tony@...mide.com, nsekhar@...com,
        jsarha@...com, netdev@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

On 04/25/2017 09:22 AM, Lars-Peter Clausen wrote:
> On 04/24/2017 11: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@...c030000 {
>>>>> +		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.
>>>
> 
> But the DT binding documentation says something else "List of one or more
> GPIOs that control the RESET lines of the PHYs on that MDIO bus".

I agree, it should be defined more strictly as:

"One GPIO that controls the reset line of *all* PHYs populated on that
MDIO bus"

If there are separate lines, these automatically become properties of
the PHY nodes.

> 
>>> 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.
> 
> I think we all know that that's not going to happen. Once there is a working
> kludge there is no incentive to do a proper implementation anymore.
> 
> 
>> 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.
> 
> I'm not arguing with that, just that the hardware description should be
> truthful to the hardware topology and not to the software topology, i.e. the
> implementation details of the Linux kernel in this case. Reset GPIOs are not
> the only resource that is connected to the PHY that needs to be enabled
> before they can be enumerated. E.g. clocks and regulators fall into the same
> realm. And while you might argue that with a on-SoC phy controller node
> there wont be any conflicts in regard to the reset-gpios property, this not
> so very true for the clocks property.

Agreed, but with the exception of the unfortunate choice of words here
(single vs. multiple) there is not a really a divergence in how the
shared reset line is represented compared to other similar control
busses, is there?

> 
> And MDIO is not really special in this regard, other discoverable buses
> (like USB, SDIO, ULPI) have the very same issue. Having a standardized
> binding approach where the resources are declared as part as the child child
> is preferable in my opinion.
> 
>>
>> Whether we need additional code to just to make the DT look prettier is
>> questionable and if required can come as a separate patch.
> 
> Unfortunately not, once it is merged it can't be changed anymore.

There are no in tree users yet, so let's get the different things fixed
right now.
-- 
Florian