[PATCH] net: ethtool: fix spelling mistake: "tubale" -> "tunable"

[Michael Heimpold]

Signed-off-by: Michael Heimpold 
---
 include/uapi/linux/ethtool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b56084f..7363f18e65a5 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -226,7 +226,7 @@ enum tunable_id {
ETHTOOL_TX_COPYBREAK,
ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
/*
-* Add your fresh new tubale attribute above and remember to update
+* Add your fresh new tunable attribute above and remember to update
 * tunable_strings[] in net/core/ethtool.c
 */
__ETHTOOL_TUNABLE_COUNT,
-- 
2.17.1

[PATCH] net: ethtool: fix spelling mistake: "tubale" -> "tunable"

[Michael Heimpold]

Signed-off-by: Michael Heimpold 
---
 include/uapi/linux/ethtool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b56084f..7363f18e65a5 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -226,7 +226,7 @@ enum tunable_id {
ETHTOOL_TX_COPYBREAK,
ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
/*
-* Add your fresh new tubale attribute above and remember to update
+* Add your fresh new tunable attribute above and remember to update
 * tunable_strings[] in net/core/ethtool.c
 */
__ETHTOOL_TUNABLE_COUNT,
-- 
2.17.1

Re: [PATCH v5 15/17] dt-bindings: qca7000: append UART interface to binding

[Michael Heimpold]

Hi,

Zitat von Jakub Kicinski <kubak...@wp.pl>:


On Thu, 11 May 2017 21:12:22 +0200, Michael Heimpold wrote:

Am Mittwoch, 10. Mai 2017, 10:53:26 CEST schrieb Stefan Wahren:
> This merges the serdev binding for the QCA7000 UART driver (Ethernet over
> UART) into the existing document.
>
> Signed-off-by: Stefan Wahren <stefan.wah...@i2se.com>
> ---
>  .../devicetree/bindings/net/qca-qca7000.txt| 32
> ++ 1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt index
> a37f656..08364c3 100644
> --- a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> +++ b/Documentation/devicetree/bindings/net/qca-qca7000.txt
> @@ -54,3 +54,35 @@ ssp2: spi@80014000 {
>local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>};
>  };
> +
> +(b) Ethernet over UART
> +
> +In order to use the QCA7000 as UART slave it must be defined as  
a child of

> a +UART master in the device tree. It is possible to preconfigure the UART
> +settings of the QCA7000 firmware, but it's not possible to change them
> during +runtime.
> +
> +Required properties:
> +- compatible: Should be "qca,qca7000-uart"

I already discussed this with Stefan off-list a little bit, but I would like
to bring this to a broader audience: I'm not sure whether the compatible
should contain the "-uart" suffix, because the hardware chip is the  
very same

QCA7000 chip which can also be used with SPI protocol.
The only difference is the loaded firmware within the chip which can either
speak SPI or UART protocol (but not both at the same time - due to shared
pins). So the hardware design decides which interface type is used.

At the moment, this patch series adds a dedicated driver for the UART
protocol, in parallel to the existing SPI driver. So a different compatible
string is needed here to match against the new driver.

An alternative approach would be to re-use the existing compatible string
"qca,qca7000" for both, the SPI and UART protocol, because a "smarter"
(combined) driver would detect which protocol to use. For example the driver
could check for spi-cpha and/or spi-cpol which are required for SPI  
protocol:

if these exists the driver could assume that SPI must be used, if both are
missing then UART protocol should be used.
(This way it would not be necessary to check whether the node is a child of
a SPI or UART master node - but maybe this is even easier - I don't know)

Or in shorter words: my concern is that while "qca7000-uart" describes the
hardware, it's too closely coupled to the driver implementation. Having
some feedback of the experts would be nice :-)


I'm no expert, but devices which can do both I2C and SPI are quite
common, and they usually have the same compatible string for both
buses.


do you have an example driver at hand? I only found GPIO mcp23s08 driver,
which can handle both I2C and SPI chips, but there are different compatible
strings used to distinguish several chip models.

Regards,
Michael

Re: [PATCH v5 15/17] dt-bindings: qca7000: append UART interface to binding

[Michael Heimpold]

Hi,

Zitat von Jakub Kicinski :


On Thu, 11 May 2017 21:12:22 +0200, Michael Heimpold wrote:

Am Mittwoch, 10. Mai 2017, 10:53:26 CEST schrieb Stefan Wahren:
> This merges the serdev binding for the QCA7000 UART driver (Ethernet over
> UART) into the existing document.
>
> Signed-off-by: Stefan Wahren 
> ---
>  .../devicetree/bindings/net/qca-qca7000.txt| 32
> ++ 1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt index
> a37f656..08364c3 100644
> --- a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> +++ b/Documentation/devicetree/bindings/net/qca-qca7000.txt
> @@ -54,3 +54,35 @@ ssp2: spi@80014000 {
>local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>};
>  };
> +
> +(b) Ethernet over UART
> +
> +In order to use the QCA7000 as UART slave it must be defined as  
a child of

> a +UART master in the device tree. It is possible to preconfigure the UART
> +settings of the QCA7000 firmware, but it's not possible to change them
> during +runtime.
> +
> +Required properties:
> +- compatible: Should be "qca,qca7000-uart"

I already discussed this with Stefan off-list a little bit, but I would like
to bring this to a broader audience: I'm not sure whether the compatible
should contain the "-uart" suffix, because the hardware chip is the  
very same

QCA7000 chip which can also be used with SPI protocol.
The only difference is the loaded firmware within the chip which can either
speak SPI or UART protocol (but not both at the same time - due to shared
pins). So the hardware design decides which interface type is used.

At the moment, this patch series adds a dedicated driver for the UART
protocol, in parallel to the existing SPI driver. So a different compatible
string is needed here to match against the new driver.

An alternative approach would be to re-use the existing compatible string
"qca,qca7000" for both, the SPI and UART protocol, because a "smarter"
(combined) driver would detect which protocol to use. For example the driver
could check for spi-cpha and/or spi-cpol which are required for SPI  
protocol:

if these exists the driver could assume that SPI must be used, if both are
missing then UART protocol should be used.
(This way it would not be necessary to check whether the node is a child of
a SPI or UART master node - but maybe this is even easier - I don't know)

Or in shorter words: my concern is that while "qca7000-uart" describes the
hardware, it's too closely coupled to the driver implementation. Having
some feedback of the experts would be nice :-)


I'm no expert, but devices which can do both I2C and SPI are quite
common, and they usually have the same compatible string for both
buses.


do you have an example driver at hand? I only found GPIO mcp23s08 driver,
which can handle both I2C and SPI chips, but there are different compatible
strings used to distinguish several chip models.

Regards,
Michael

Re: [PATCH v5 13/17] dt-bindings: qca7000: rename binding

[Michael Heimpold]

Hi,

Am Mittwoch, 10. Mai 2017, 10:53:24 CEST schrieb Stefan Wahren:
> Before we can merge the QCA7000 UART binding the document needs to be
> renamed.
> 
> Signed-off-by: Stefan Wahren 
> ---
>  .../devicetree/bindings/net/{qca-qca7000-spi.txt => qca-qca7000.txt}  |
> 0 1 file changed, 0 insertions(+), 0 deletions(-)
>  rename Documentation/devicetree/bindings/net/{qca-qca7000-spi.txt =>
> qca-qca7000.txt} (100%)
> 
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000-spi.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt similarity index
> 100%
> rename from Documentation/devicetree/bindings/net/qca-qca7000-spi.txt
> rename to Documentation/devicetree/bindings/net/qca-qca7000.txt

I was told here [1], that it is prefered when the filename matchs the
compatible string, so using "qca,qca7000.txt" should be better.

Regards,
Michael


[1]
https://www.spinics.net/lists/devicetree/msg124088.html

Re: [PATCH v5 13/17] dt-bindings: qca7000: rename binding

[Michael Heimpold]

Hi,

Am Mittwoch, 10. Mai 2017, 10:53:24 CEST schrieb Stefan Wahren:
> Before we can merge the QCA7000 UART binding the document needs to be
> renamed.
> 
> Signed-off-by: Stefan Wahren 
> ---
>  .../devicetree/bindings/net/{qca-qca7000-spi.txt => qca-qca7000.txt}  |
> 0 1 file changed, 0 insertions(+), 0 deletions(-)
>  rename Documentation/devicetree/bindings/net/{qca-qca7000-spi.txt =>
> qca-qca7000.txt} (100%)
> 
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000-spi.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt similarity index
> 100%
> rename from Documentation/devicetree/bindings/net/qca-qca7000-spi.txt
> rename to Documentation/devicetree/bindings/net/qca-qca7000.txt

I was told here [1], that it is prefered when the filename matchs the
compatible string, so using "qca,qca7000.txt" should be better.

Regards,
Michael


[1]
https://www.spinics.net/lists/devicetree/msg124088.html

Re: [PATCH v5 15/17] dt-bindings: qca7000: append UART interface to binding

[Michael Heimpold]

Hi,

Am Mittwoch, 10. Mai 2017, 10:53:26 CEST schrieb Stefan Wahren:
> This merges the serdev binding for the QCA7000 UART driver (Ethernet over
> UART) into the existing document.
> 
> Signed-off-by: Stefan Wahren 
> ---
>  .../devicetree/bindings/net/qca-qca7000.txt| 32
> ++ 1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt index
> a37f656..08364c3 100644
> --- a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> +++ b/Documentation/devicetree/bindings/net/qca-qca7000.txt
> @@ -54,3 +54,35 @@ ssp2: spi@80014000 {
>   local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>   };
>  };
> +
> +(b) Ethernet over UART
> +
> +In order to use the QCA7000 as UART slave it must be defined as a child of
> a +UART master in the device tree. It is possible to preconfigure the UART
> +settings of the QCA7000 firmware, but it's not possible to change them
> during +runtime.
> +
> +Required properties:
> +- compatible: Should be "qca,qca7000-uart"

I already discussed this with Stefan off-list a little bit, but I would like
to bring this to a broader audience: I'm not sure whether the compatible 
should contain the "-uart" suffix, because the hardware chip is the very same
QCA7000 chip which can also be used with SPI protocol.
The only difference is the loaded firmware within the chip which can either
speak SPI or UART protocol (but not both at the same time - due to shared 
pins). So the hardware design decides which interface type is used.

At the moment, this patch series adds a dedicated driver for the UART 
protocol, in parallel to the existing SPI driver. So a different compatible
string is needed here to match against the new driver.

An alternative approach would be to re-use the existing compatible string
"qca,qca7000" for both, the SPI and UART protocol, because a "smarter" 
(combined) driver would detect which protocol to use. For example the driver 
could check for spi-cpha and/or spi-cpol which are required for SPI protocol: 
if these exists the driver could assume that SPI must be used, if both are 
missing then UART protocol should be used.
(This way it would not be necessary to check whether the node is a child of
a SPI or UART master node - but maybe this is even easier - I don't know)

Or in shorter words: my concern is that while "qca7000-uart" describes the 
hardware, it's too closely coupled to the driver implementation. Having
some feedback of the experts would be nice :-)

Thanks,
Michael

> +
> +Optional properties:
> +- local-mac-address : see ./ethernet.txt
> +- current-speed : current baud rate of QCA7000 which defaults to 115200
> +   if absent, see also ../serial/slave-device.txt
> +
> +UART Example:
> +
> +/* Freescale i.MX28 UART */
> +auart0: serial@8006a000 {
> + compatible = "fsl,imx28-auart", "fsl,imx23-auart";
> + reg = <0x8006a000 0x2000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_2pins_a>;
> + status = "okay";
> +
> + qca7000: ethernet {
> + compatible = "qca,qca7000-uart";
> + local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
> + current-speed = <38400>;
> + };
> +};

Re: [PATCH v5 15/17] dt-bindings: qca7000: append UART interface to binding

[Michael Heimpold]

Hi,

Am Mittwoch, 10. Mai 2017, 10:53:26 CEST schrieb Stefan Wahren:
> This merges the serdev binding for the QCA7000 UART driver (Ethernet over
> UART) into the existing document.
> 
> Signed-off-by: Stefan Wahren 
> ---
>  .../devicetree/bindings/net/qca-qca7000.txt| 32
> ++ 1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> b/Documentation/devicetree/bindings/net/qca-qca7000.txt index
> a37f656..08364c3 100644
> --- a/Documentation/devicetree/bindings/net/qca-qca7000.txt
> +++ b/Documentation/devicetree/bindings/net/qca-qca7000.txt
> @@ -54,3 +54,35 @@ ssp2: spi@80014000 {
>   local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>   };
>  };
> +
> +(b) Ethernet over UART
> +
> +In order to use the QCA7000 as UART slave it must be defined as a child of
> a +UART master in the device tree. It is possible to preconfigure the UART
> +settings of the QCA7000 firmware, but it's not possible to change them
> during +runtime.
> +
> +Required properties:
> +- compatible: Should be "qca,qca7000-uart"

I already discussed this with Stefan off-list a little bit, but I would like
to bring this to a broader audience: I'm not sure whether the compatible 
should contain the "-uart" suffix, because the hardware chip is the very same
QCA7000 chip which can also be used with SPI protocol.
The only difference is the loaded firmware within the chip which can either
speak SPI or UART protocol (but not both at the same time - due to shared 
pins). So the hardware design decides which interface type is used.

At the moment, this patch series adds a dedicated driver for the UART 
protocol, in parallel to the existing SPI driver. So a different compatible
string is needed here to match against the new driver.

An alternative approach would be to re-use the existing compatible string
"qca,qca7000" for both, the SPI and UART protocol, because a "smarter" 
(combined) driver would detect which protocol to use. For example the driver 
could check for spi-cpha and/or spi-cpol which are required for SPI protocol: 
if these exists the driver could assume that SPI must be used, if both are 
missing then UART protocol should be used.
(This way it would not be necessary to check whether the node is a child of
a SPI or UART master node - but maybe this is even easier - I don't know)

Or in shorter words: my concern is that while "qca7000-uart" describes the 
hardware, it's too closely coupled to the driver implementation. Having
some feedback of the experts would be nice :-)

Thanks,
Michael

> +
> +Optional properties:
> +- local-mac-address : see ./ethernet.txt
> +- current-speed : current baud rate of QCA7000 which defaults to 115200
> +   if absent, see also ../serial/slave-device.txt
> +
> +UART Example:
> +
> +/* Freescale i.MX28 UART */
> +auart0: serial@8006a000 {
> + compatible = "fsl,imx28-auart", "fsl,imx23-auart";
> + reg = <0x8006a000 0x2000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_2pins_a>;
> + status = "okay";
> +
> + qca7000: ethernet {
> + compatible = "qca,qca7000-uart";
> + local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
> + current-speed = <38400>;
> + };
> +};

Re: [PATCH] net: fec: fix enet_out clock handling

[Michael Heimpold]

Hi,

Am 27.11.2015 um 14:39 schrieb Lothar Waßmann:

When ENET_OUT is being used as reference clock for an external PHY,
the clock must not be disabled while the PHY is active. Otherwise the
PHY may lose its internal state and require a reset to become
functional again.

A symptom for this bug is a network interface that constantly toggles
between UP and DOWN state:
fec 800f.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f.ethernet eth0: Link is Down
fec 800f.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f.ethernet eth0: Link is Down
[...]


I would add a sentence about the solution, e.g. moving ENET_OUT handling to 
driver probe etc.


Signed-off-by: Lothar Waßmann 
---
  drivers/net/ethernet/freescale/fec_main.c | 34 +--
  1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index d2328fc..d9df4c5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1857,11 +1857,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ahb);
if (ret)
return ret;
-   if (fep->clk_enet_out) {
-   ret = clk_prepare_enable(fep->clk_enet_out);
-   if (ret)
-   goto failed_clk_enet_out;
-   }
if (fep->clk_ptp) {
mutex_lock(>ptp_clk_mutex);
ret = clk_prepare_enable(fep->clk_ptp);
@@ -1873,35 +1868,26 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
}
mutex_unlock(>ptp_clk_mutex);
}
-   if (fep->clk_ref) {
-   ret = clk_prepare_enable(fep->clk_ref);
-   if (ret)
-   goto failed_clk_ref;
-   }
+   ret = clk_prepare_enable(fep->clk_ref);
+   if (ret)
+   goto failed_clk_ref;


This change seems unrelated to the problem. At least, I can leave this part out
and the toggle still disappear after apply the remaining parts.
However, I've only my Duckbill (iMX28) around to test with.


} else {
clk_disable_unprepare(fep->clk_ahb);
-   if (fep->clk_enet_out)
-   clk_disable_unprepare(fep->clk_enet_out);
if (fep->clk_ptp) {
mutex_lock(>ptp_clk_mutex);
clk_disable_unprepare(fep->clk_ptp);
fep->ptp_clk_on = false;
mutex_unlock(>ptp_clk_mutex);
}
-   if (fep->clk_ref)
-   clk_disable_unprepare(fep->clk_ref);
+   clk_disable_unprepare(fep->clk_ref);


Same as above, might be unrelated.


}
  
  	return 0;
  
  failed_clk_ref:

-   if (fep->clk_ref)
-   clk_disable_unprepare(fep->clk_ref);
+   clk_disable_unprepare(fep->clk_ref);

dito


  failed_clk_ptp:
-   if (fep->clk_enet_out)
-   clk_disable_unprepare(fep->clk_enet_out);
-failed_clk_enet_out:
-   clk_disable_unprepare(fep->clk_ahb);
+   clk_disable_unprepare(fep->clk_ahb);
  
  	return ret;

  }
@@ -3425,6 +3411,10 @@ fec_probe(struct platform_device *pdev)
if (ret)
goto failed_clk;
  
+	ret = clk_prepare_enable(fep->clk_enet_out);

+   if (ret)
+   goto failed_clk_enet_out;
+

As enet_out is optional, shouldn't this block be guarded by
if (fep->clk_enet_out)... ?


ret = clk_prepare_enable(fep->clk_ipg);
if (ret)
goto failed_clk_ipg;
@@ -3509,6 +3499,8 @@ failed_init:
if (fep->reg_phy)
regulator_disable(fep->reg_phy);
  failed_regulator:
+   clk_disable_unprepare(fep->clk_enet_out);

here too?

+failed_clk_enet_out:
clk_disable_unprepare(fep->clk_ipg);
  failed_clk_ipg:
fec_enet_clk_enable(ndev, false);
@@ -3531,6 +3523,8 @@ fec_drv_remove(struct platform_device *pdev)
fec_ptp_stop(pdev);
unregister_netdev(ndev);
fec_enet_mii_remove(fep);
+   fec_enet_clk_enable(ndev, false);
+   clk_disable_unprepare(fep->clk_enet_out);


and here too?


if (fep->reg_phy)
    regulator_disable(fep->reg_phy);
of_node_put(fep->phy_node);


Mit freundlichen Grüßen / Kind regards
Michael Heimpold
--
Software Engineer

I2SE GmbH   Tel: +49 (0) 341 355667-00
Friedrich-Ebert-Str. 61 Fax: +49 (0) 341 355667-02
04109 Leipzig
Germany
Web: http://www.i2se.com/   Mail: i...@i2se.com
V

Re: [PATCH] net: fec: fix enet_out clock handling

[Michael Heimpold]

Hi,

Am 27.11.2015 um 14:39 schrieb Lothar Waßmann:

When ENET_OUT is being used as reference clock for an external PHY,
the clock must not be disabled while the PHY is active. Otherwise the
PHY may lose its internal state and require a reset to become
functional again.

A symptom for this bug is a network interface that constantly toggles
between UP and DOWN state:
fec 800f.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f.ethernet eth0: Link is Down
fec 800f.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f.ethernet eth0: Link is Down
[...]


I would add a sentence about the solution, e.g. moving ENET_OUT handling to 
driver probe etc.


Signed-off-by: Lothar Waßmann <l...@karo-electronics.de>
---
  drivers/net/ethernet/freescale/fec_main.c | 34 +--
  1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index d2328fc..d9df4c5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1857,11 +1857,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ahb);
if (ret)
return ret;
-   if (fep->clk_enet_out) {
-   ret = clk_prepare_enable(fep->clk_enet_out);
-   if (ret)
-   goto failed_clk_enet_out;
-   }
if (fep->clk_ptp) {
mutex_lock(>ptp_clk_mutex);
ret = clk_prepare_enable(fep->clk_ptp);
@@ -1873,35 +1868,26 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
}
mutex_unlock(>ptp_clk_mutex);
}
-   if (fep->clk_ref) {
-   ret = clk_prepare_enable(fep->clk_ref);
-   if (ret)
-   goto failed_clk_ref;
-   }
+   ret = clk_prepare_enable(fep->clk_ref);
+   if (ret)
+   goto failed_clk_ref;


This change seems unrelated to the problem. At least, I can leave this part out
and the toggle still disappear after apply the remaining parts.
However, I've only my Duckbill (iMX28) around to test with.


} else {
clk_disable_unprepare(fep->clk_ahb);
-   if (fep->clk_enet_out)
-   clk_disable_unprepare(fep->clk_enet_out);
if (fep->clk_ptp) {
mutex_lock(>ptp_clk_mutex);
clk_disable_unprepare(fep->clk_ptp);
fep->ptp_clk_on = false;
mutex_unlock(>ptp_clk_mutex);
}
-   if (fep->clk_ref)
-   clk_disable_unprepare(fep->clk_ref);
+   clk_disable_unprepare(fep->clk_ref);


Same as above, might be unrelated.


}
  
  	return 0;
  
  failed_clk_ref:

-   if (fep->clk_ref)
-   clk_disable_unprepare(fep->clk_ref);
+   clk_disable_unprepare(fep->clk_ref);

dito


  failed_clk_ptp:
-   if (fep->clk_enet_out)
-   clk_disable_unprepare(fep->clk_enet_out);
-failed_clk_enet_out:
-   clk_disable_unprepare(fep->clk_ahb);
+   clk_disable_unprepare(fep->clk_ahb);
  
  	return ret;

  }
@@ -3425,6 +3411,10 @@ fec_probe(struct platform_device *pdev)
if (ret)
goto failed_clk;
  
+	ret = clk_prepare_enable(fep->clk_enet_out);

+   if (ret)
+   goto failed_clk_enet_out;
+

As enet_out is optional, shouldn't this block be guarded by
if (fep->clk_enet_out)... ?


ret = clk_prepare_enable(fep->clk_ipg);
if (ret)
goto failed_clk_ipg;
@@ -3509,6 +3499,8 @@ failed_init:
if (fep->reg_phy)
regulator_disable(fep->reg_phy);
  failed_regulator:
+   clk_disable_unprepare(fep->clk_enet_out);

here too?

+failed_clk_enet_out:
clk_disable_unprepare(fep->clk_ipg);
  failed_clk_ipg:
fec_enet_clk_enable(ndev, false);
@@ -3531,6 +3523,8 @@ fec_drv_remove(struct platform_device *pdev)
fec_ptp_stop(pdev);
unregister_netdev(ndev);
fec_enet_mii_remove(fep);
+   fec_enet_clk_enable(ndev, false);
+   clk_disable_unprepare(fep->clk_enet_out);


and here too?


if (fep->reg_phy)
    regulator_disable(fep->reg_phy);
of_node_put(fep->phy_node);


Mit freundlichen Grüßen / Kind regards
Michael Heimpold
--
Software Engineer

I2SE GmbH   Tel: +49 (0) 341 355667-00
Friedrich-Ebert-Str. 61 Fax: +49 (0) 341 355667-02
04109 Leipzig
Germany
Web: http://www.i2se.c

Re: [PATCH v2] net: phy: workaround for buggy cable detection by LAN8700 after cable plugging

[Michael Heimpold]

Hi Igor,

Am Freitag, 14. August 2015, 11:03:04 schrieb Igor Plyatov:
> Dear Michael,
> 
> > Hi Igor,
> >
> > Am Donnerstag, 13. August 2015, 22:18:34 schrieben Sie:
> >
> > > * Due to HW bug, LAN8700 sometimes does not detect presence of 
> > energy in the
> >
> > > Ethernet cable in Energy Detect Power-Down mode (e.g while EDPWRDOWN 
> > bit is
> >
> > > set, the ENERGYON bit does not asserted sometimes). This is a common 
> > bug of
> >
> > > LAN87xx family of PHY chips.
> >
> > Is there any offical errata sheet for this PHY family? How do you 
> > know, that this is a
> >
> > common HW bug?
> >
> 
> The LAN8700, LAN8710, LAN8720 is a product of the SMSC company. 
> Microchip acquired SMSC in August 2012.
> 
> The LAN8700 is a legacy product for Microchip and they will not update 
> anything about it. So, even if Microchip know about HW bug, then there 
> is no chance to have Errata sheet or any new documents about LAN8700.

Long time ago, I worked on a custom device with a PHY of the same family.
Errata sheet existed but was only available by signing a NDA. So I simply
wondered whether this changed since SMSC is now Microchip or if they keep
it still so covered...

> 
> I think same history is for LAN8710/LAN8720 even if they are not marked 
> as legacy. They are SMSC products.
> 
> The workarounds for same issue in LAN8710/LAN8720 was committed by:
>   * Marek Vasut  as b629820d18fa65cc598390e4b9712fd5f83ee693.
>   * Patrick Trantham  as 
> 4223dbffed9f89596177ff2b256ef3258b20fa46.
> 
> > Me too, I think that this family has some problems with this mode, 
> > however, without
> >
> > hard evidence, I would put it softer.
> >
> 
> I have discovered this bug by just monitoring of data to/from MDIO 
> registers of LAN8700.
> And HW issue is proven on 100 % by rare absence of ENERGYON bit when 
> cable is plugged in.
> Sometimes, it is required to make 2-20 tests to catch this issue.
> 
> The configuration of CPU pins, responsible for the MDIO interface, was 
> checked carefully by oscilloscope and they are fine (no spikes, no 
> garbage, good shape of edges).
> 
> > > * The lan87xx_read_status() was improved to acquire ENERGYON bit. 
> > Its previous
> >
> > > algorythm still not reliable on 100 % and sometimes skip cable plugging.
> >
> > >
> >
> > > Signed-off-by: Igor Plyatov 
> >
> > > ---
> >
> > > drivers/net/phy/smsc.c | 15 ---
> >
> > > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > >
> >
> > > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> >
> > > index c0f6479..8559ff1 100644
> >
> > > --- a/drivers/net/phy/smsc.c
> >
> > > +++ b/drivers/net/phy/smsc.c
> >
> > > @@ -104,6 +104,7 @@ static int lan911x_config_init(struct phy_device 
> > *phydev)
> >
> > > static int lan87xx_read_status(struct phy_device *phydev)
> >
> > > {
> >
> > > int err = genphy_read_status(phydev);
> >
> > > + int i;
> >
> > >
> >
> > > if (!phydev->link) {
> >
> > > /* Disable EDPD to wake up PHY */
> >
> > > @@ -116,8 +117,16 @@ static int lan87xx_read_status(struct 
> > phy_device *phydev)
> >
> > > if (rc < 0)
> >
> > > return rc;
> >
> > >
> >
> > > - /* Sleep 64 ms to allow ~5 link test pulses to be sent */
> >
> > > - msleep(64);
> >
> > > + /* Wait max 640 ms to detect energy */
> >
> > Why 640ms and not e.g. 650ms?
> >
> > I'm no PHY expert, but this looks like an ugly workaround.
> >
> 
> Such a value was adopted after many trial and probes. It allows to 
> detect cable plugging on 100 %.
> Ugly or not, but it works and reliable.
> 
> > Maybe it would be better to avoid this power saving mode at all, when 
> > it is not
> >
> > reliable, but this are just my 2cts. :-)
> >
> 
> Power saving mode allow to save around 220 mW of energy consumed from 
> power supply, when Ethernet cable is not plugged in.
> This is a good value for embedded devices.
> Better to keep power save mode on.

Ok, I was not aware, that this is so much.

> 
> > Anyway, I guess you should also update the explanation on top of the 
> > function to reflect
> >
> > your new approach.
> >
> 
> I propose following comment for the lan87xx_read_status():
> /*
>   * The LAN87xx suffers from rare absence of the ENERGYON-bit when 
> Ethernet cable
>   * plugs in while LAN87xx is in Energy Detect Power-Down mode. This 
> leads to
>   * unstable detection of plugging in Ethernet cable.
>   * This workaround disables Energy Detect Power-Down mode and waiting for
>   * response on link pulses to detect presence of plugged Ethernet cable.
>   * The Energy Detect Power-Down mode enabled again in the end of 
> procedure to
>   * save approximately 220 mW of power if cable is unplugged.
>   */

Nice. Only one nitpick: ... _is_ enabled again...

> 
> > > + for (i = 0; i < 64; i++) {
> >
> > > + /* Sleep to allow link test pulses to be sent */
> >
> > > + msleep(10);
> >
> > > + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> >
> > > + if (rc < 0)
> >
> > > + return rc;
> >
> > > + if (rc & MII_LAN83C185_ENERGYON)
> >
> > 

Re: [PATCH v2] net: phy: workaround for buggy cable detection by LAN8700 after cable plugging

[Michael Heimpold]

Hi Igor,

Am Freitag, 14. August 2015, 11:03:04 schrieb Igor Plyatov:
 Dear Michael,
 
  Hi Igor,
 
  Am Donnerstag, 13. August 2015, 22:18:34 schrieben Sie:
 
   * Due to HW bug, LAN8700 sometimes does not detect presence of 
  energy in the
 
   Ethernet cable in Energy Detect Power-Down mode (e.g while EDPWRDOWN 
  bit is
 
   set, the ENERGYON bit does not asserted sometimes). This is a common 
  bug of
 
   LAN87xx family of PHY chips.
 
  Is there any offical errata sheet for this PHY family? How do you 
  know, that this is a
 
  common HW bug?
 
 
 The LAN8700, LAN8710, LAN8720 is a product of the SMSC company. 
 Microchip acquired SMSC in August 2012.
 
 The LAN8700 is a legacy product for Microchip and they will not update 
 anything about it. So, even if Microchip know about HW bug, then there 
 is no chance to have Errata sheet or any new documents about LAN8700.

Long time ago, I worked on a custom device with a PHY of the same family.
Errata sheet existed but was only available by signing a NDA. So I simply
wondered whether this changed since SMSC is now Microchip or if they keep
it still so covered...

 
 I think same history is for LAN8710/LAN8720 even if they are not marked 
 as legacy. They are SMSC products.
 
 The workarounds for same issue in LAN8710/LAN8720 was committed by:
   * Marek Vasut ma...@denx.de as b629820d18fa65cc598390e4b9712fd5f83ee693.
   * Patrick Trantham patrick.trant...@fuel7.com as 
 4223dbffed9f89596177ff2b256ef3258b20fa46.
 
  Me too, I think that this family has some problems with this mode, 
  however, without
 
  hard evidence, I would put it softer.
 
 
 I have discovered this bug by just monitoring of data to/from MDIO 
 registers of LAN8700.
 And HW issue is proven on 100 % by rare absence of ENERGYON bit when 
 cable is plugged in.
 Sometimes, it is required to make 2-20 tests to catch this issue.
 
 The configuration of CPU pins, responsible for the MDIO interface, was 
 checked carefully by oscilloscope and they are fine (no spikes, no 
 garbage, good shape of edges).
 
   * The lan87xx_read_status() was improved to acquire ENERGYON bit. 
  Its previous
 
   algorythm still not reliable on 100 % and sometimes skip cable plugging.
 
  
 
   Signed-off-by: Igor Plyatov plya...@gmail.com
 
   ---
 
   drivers/net/phy/smsc.c | 15 ---
 
   1 file changed, 12 insertions(+), 3 deletions(-)
 
  
 
   diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
 
   index c0f6479..8559ff1 100644
 
   --- a/drivers/net/phy/smsc.c
 
   +++ b/drivers/net/phy/smsc.c
 
   @@ -104,6 +104,7 @@ static int lan911x_config_init(struct phy_device 
  *phydev)
 
   static int lan87xx_read_status(struct phy_device *phydev)
 
   {
 
   int err = genphy_read_status(phydev);
 
   + int i;
 
  
 
   if (!phydev-link) {
 
   /* Disable EDPD to wake up PHY */
 
   @@ -116,8 +117,16 @@ static int lan87xx_read_status(struct 
  phy_device *phydev)
 
   if (rc  0)
 
   return rc;
 
  
 
   - /* Sleep 64 ms to allow ~5 link test pulses to be sent */
 
   - msleep(64);
 
   + /* Wait max 640 ms to detect energy */
 
  Why 640ms and not e.g. 650ms?
 
  I'm no PHY expert, but this looks like an ugly workaround.
 
 
 Such a value was adopted after many trial and probes. It allows to 
 detect cable plugging on 100 %.
 Ugly or not, but it works and reliable.
 
  Maybe it would be better to avoid this power saving mode at all, when 
  it is not
 
  reliable, but this are just my 2cts. :-)
 
 
 Power saving mode allow to save around 220 mW of energy consumed from 
 power supply, when Ethernet cable is not plugged in.
 This is a good value for embedded devices.
 Better to keep power save mode on.

Ok, I was not aware, that this is so much.

 
  Anyway, I guess you should also update the explanation on top of the 
  function to reflect
 
  your new approach.
 
 
 I propose following comment for the lan87xx_read_status():
 /*
   * The LAN87xx suffers from rare absence of the ENERGYON-bit when 
 Ethernet cable
   * plugs in while LAN87xx is in Energy Detect Power-Down mode. This 
 leads to
   * unstable detection of plugging in Ethernet cable.
   * This workaround disables Energy Detect Power-Down mode and waiting for
   * response on link pulses to detect presence of plugged Ethernet cable.
   * The Energy Detect Power-Down mode enabled again in the end of 
 procedure to
   * save approximately 220 mW of power if cable is unplugged.
   */

Nice. Only one nitpick: ... _is_ enabled again...

 
   + for (i = 0; i  64; i++) {
 
   + /* Sleep to allow link test pulses to be sent */
 
   + msleep(10);
 
   + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 
   + if (rc  0)
 
   + return rc;
 
   + if (rc  MII_LAN83C185_ENERGYON)
 
   + break;
 
   + };
 
  
 
   /* Re-enable EDPD */
 
   rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 
   @@ -191,7 +200,7 @@ static struct phy_driver smsc_phy_driver[] = {
 
  
 
   /* basic functions */
 
   .config_aneg = genphy_config_aneg,
 
   -