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

[Andrew Lunn]

> Patch is already in net-next. How can we get this fixed? Should I send a v5?

Hi Roger

See the followup patch Florian submitted. Send your Reviewed-by: if
you agree with it.

  Andrew

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

[Andrew Lunn]

> Patch is already in net-next. How can we get this fixed? Should I send a v5?

Hi Roger

See the followup patch Florian submitted. Send your Reviewed-by: if
you agree with it.

  Andrew

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

[Roger Quadros]

On 25/04/17 19:31, Florian Fainelli wrote:
> 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 000..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@0x5c03 {
>> +compatible = "ti,davinci_mdio";
>> +reg = <0x5c03 0x1000>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +reset-gpios = < 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"

Patch is already in net-next. How can we get this fixed? Should I send a v5?

> 
> 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 

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

[Roger Quadros]

On 25/04/17 19:31, Florian Fainelli wrote:
> 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 000..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@0x5c03 {
>> +compatible = "ti,davinci_mdio";
>> +reg = <0x5c03 0x1000>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +reset-gpios = < 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"

Patch is already in net-next. How can we get this fixed? Should I send a v5?

> 
> 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 

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

[Roger Quadros]

On 25/04/17 19:22, 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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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".
> 
>>> 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.
> 
> 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.

Good point. I agree now that if PHYs have individual RESET lines, they
should be part of the PHY node.
> 
>>
>> 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.
> 

cheers,
-roger

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

[Roger Quadros]

On 25/04/17 19:22, 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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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".
> 
>>> 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.
> 
> 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.

Good point. I agree now that if PHYs have individual RESET lines, they
should be part of the PHY node.
> 
>>
>> 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.
> 

cheers,
-roger

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

[Florian Fainelli]

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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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

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

[Florian Fainelli]

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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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

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

[Lars-Peter Clausen]

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 000..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@0x5c03 {
 +  compatible = "ti,davinci_mdio";
 +  reg = <0x5c03 0x1000>;
 +  #address-cells = <1>;
 +  #size-cells = <0>;
 +
 +  reset-gpios = < 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".

>> 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.

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.

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

[Lars-Peter Clausen]

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 000..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@0x5c03 {
 +  compatible = "ti,davinci_mdio";
 +  reg = <0x5c03 0x1000>;
 +  #address-cells = <1>;
 +  #size-cells = <0>;
 +
 +  reset-gpios = < 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".

>> 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.

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.

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

[David Miller]

From: Roger Quadros 
Date: Fri, 21 Apr 2017 16:15:38 +0300

> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros 

Applied, thanks.

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

[David Miller]

From: Roger Quadros 
Date: Fri, 21 Apr 2017 16:15:38 +0300

> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros 

Applied, thanks.

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

[Florian Fainelli]

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 000..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@0x5c03 {
 +  compatible = "ti,davinci_mdio";
 +  reg = <0x5c03 0x1000>;
 +  #address-cells = <1>;
 +  #size-cells = <0>;
 +
 +  reset-gpios = < 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

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

[Florian Fainelli]

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 000..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@0x5c03 {
 +  compatible = "ti,davinci_mdio";
 +  reg = <0x5c03 0x1000>;
 +  #address-cells = <1>;
 +  #size-cells = <0>;
 +
 +  reset-gpios = < 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

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

[Roger Quadros]

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 000..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@0x5c03 {
>>> +   compatible = "ti,davinci_mdio";
>>> +   reg = <0x5c03 0x1000>;
>>> +   #address-cells = <1>;
>>> +   #size-cells = <0>;
>>> +
>>> +   reset-gpios = < 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.

cheers,
-roger

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

[Roger Quadros]

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 000..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@0x5c03 {
>>> +   compatible = "ti,davinci_mdio";
>>> +   reg = <0x5c03 0x1000>;
>>> +   #address-cells = <1>;
>>> +   #size-cells = <0>;
>>> +
>>> +   reset-gpios = < 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.

cheers,
-roger

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

[Andrew Lunn]

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 000..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@0x5c03 {
> > +   compatible = "ti,davinci_mdio";
> > +   reg = <0x5c03 0x1000>;
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +
> > +   reset-gpios = < 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.

Andrew

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

[Andrew Lunn]

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 000..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@0x5c03 {
> > +   compatible = "ti,davinci_mdio";
> > +   reg = <0x5c03 0x1000>;
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +
> > +   reset-gpios = < 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.

Andrew

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

[Lars-Peter Clausen]

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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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>;
> + };

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

[Lars-Peter Clausen]

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 000..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@0x5c03 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c03 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = < 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>;
> + };