[PATCHv1 0/3] Add Reset controller to Ethernet PHY

[PATCHv1 0/3] Add Reset controller to Ethernet PHY

[Anand Moon]

It is being observed some time the Ethernet interface
will not send / recive any packet after reboot.

Earlier I had submitted Ethernet reset ID patch
but it did not resolve it issue much, Adding new 
reset controller of the Ethernet PHY for Amlogic SoC
could help resolve the issue.

Thanks
-Anand

Anand Moon (3):
  arm64: dts: amlogic: add missing ethernet reset ID
  ARM: dts: meson: Use new reset id for reset controller
  net: stmmac: dwmac-meson8b: Add reset controller for ethernet phy

 arch/arm/boot/dts/meson8b.dtsi                |  2 +-
 arch/arm/boot/dts/meson8m2.dtsi               |  2 +-
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  2 ++
 .../boot/dts/amlogic/meson-g12-common.dtsi    |  2 ++
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |  3 +++
 .../ethernet/stmicro/stmmac/dwmac-meson8b.c   | 20 +++++++++++++++++++
 6 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.32.0

[PATCHv1 1/3] arm64: dts: amlogic: add missing ethernet reset ID

[Anand Moon]

Add reset external reset of the ethernet mac controller,
used new reset id for reset controller as it conflict
with the core reset id.

Fixes: f3362f0c1817 ("arm64: dts: amlogic: add missing ethernet reset ID")

Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi        | 2 ++
 arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 2 ++
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi         | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 3f5254eeb47b..da3bf9f7c1c6 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -280,6 +280,8 @@ ethmac: ethernet@ff3f0000 {
 				      "timing-adjustment";
 			rx-fifo-depth = <4096>;
 			tx-fifo-depth = <2048>;
+			resets = <&reset RESET_ETHERNET>;
+			reset-names = "ethreset";
 			power-domains = <&pwrc PWRC_AXG_ETHERNET_MEM_ID>;
 			status = "disabled";
 		};
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 00c6f53290d4..c174ed50705f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -230,6 +230,8 @@ ethmac: ethernet@ff3f0000 {
 				      "timing-adjustment";
 			rx-fifo-depth = <4096>;
 			tx-fifo-depth = <2048>;
+			resets = <&reset RESET_ETHERNET>;
+			reset-names = "ethreset";
 			status = "disabled";
 
 			mdio0: mdio {
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 6b457b2c30a4..717fa3134882 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -13,6 +13,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/power/meson-gxbb-power.h>
+#include <dt-bindings/reset/amlogic,meson-gxbb-reset.h>
 #include <dt-bindings/thermal/thermal.h>
 
 / {
@@ -582,6 +583,8 @@ ethmac: ethernet@c9410000 {
 			interrupt-names = "macirq";
 			rx-fifo-depth = <4096>;
 			tx-fifo-depth = <2048>;
+			resets = <&reset RESET_ETHERNET>;
+			reset-names = "ethreset";
 			power-domains = <&pwrc PWRC_GXBB_ETHERNET_MEM_ID>;
 			status = "disabled";
 		};
-- 
2.32.0

Re: [PATCHv1 1/3] arm64: dts: amlogic: add missing ethernet reset ID

[Philipp Zabel]

Hi Anand,

On Fri, 2021-07-30 at 01:40 +0530, Anand Moon wrote:
> Add reset external reset of the ethernet mac controller,
> used new reset id for reset controller as it conflict
> with the core reset id.
> 
> Fixes: f3362f0c1817 ("arm64: dts: amlogic: add missing ethernet reset ID")
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-axg.dtsi        | 2 ++
>  arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 2 ++
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi         | 3 +++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
> index 3f5254eeb47b..da3bf9f7c1c6 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
> @@ -280,6 +280,8 @@ ethmac: ethernet@ff3f0000 {
>  				      "timing-adjustment";
>  			rx-fifo-depth = <4096>;
>  			tx-fifo-depth = <2048>;
> +			resets = <&reset RESET_ETHERNET>;
> +			reset-names = "ethreset";

This is missing binding documentation. Also, is this reset name taken
from the documentation? Otherwise, it would probably be better to call
it "phy" for a PHY reset.

regards
Philipp

[PATCHv1 2/3] ARM: dts: meson: Use new reset id for reset controller

[Anand Moon]

Used new reset id for reset controller as it conflict
with the core reset id.

Fixes: b96446541d83 ("ARM: dts: meson8b: extend ethernet controller description")

Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/meson8b.dtsi  | 2 +-
 arch/arm/boot/dts/meson8m2.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index c02b03cbcdf4..cb3a579d09ef 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -511,7 +511,7 @@ &ethmac {
 	tx-fifo-depth = <2048>;
 
 	resets = <&reset RESET_ETHERNET>;
-	reset-names = "stmmaceth";
+	reset-names = "ethreset";
 
 	power-domains = <&pwrc PWRC_MESON8_ETHERNET_MEM_ID>;
 };
diff --git a/arch/arm/boot/dts/meson8m2.dtsi b/arch/arm/boot/dts/meson8m2.dtsi
index 6725dd9fd825..cfaf60c4ba5f 100644
--- a/arch/arm/boot/dts/meson8m2.dtsi
+++ b/arch/arm/boot/dts/meson8m2.dtsi
@@ -34,7 +34,7 @@ &ethmac {
 		 <&clkc CLKID_FCLK_DIV2>;
 	clock-names = "stmmaceth", "clkin0", "clkin1", "timing-adjustment";
 	resets = <&reset RESET_ETHERNET>;
-	reset-names = "stmmaceth";
+	reset-names = "ethreset";
 };
 
 &pinctrl_aobus {
-- 
2.32.0

Re: [PATCHv1 2/3] ARM: dts: meson: Use new reset id for reset controller

[Philipp Zabel]

On Fri, 2021-07-30 at 01:40 +0530, Anand Moon wrote:
> Used new reset id for reset controller as it conflict
> with the core reset id.
> 
> Fixes: b96446541d83 ("ARM: dts: meson8b: extend ethernet controller description")
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/meson8b.dtsi  | 2 +-
>  arch/arm/boot/dts/meson8m2.dtsi | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> index c02b03cbcdf4..cb3a579d09ef 100644
> --- a/arch/arm/boot/dts/meson8b.dtsi
> +++ b/arch/arm/boot/dts/meson8b.dtsi
> @@ -511,7 +511,7 @@ &ethmac {
>  	tx-fifo-depth = <2048>;
>  
>  	resets = <&reset RESET_ETHERNET>;
> -	reset-names = "stmmaceth";
> +	reset-names = "ethreset";

This looks like an incompatible change. Is the "stmmaceth" reset not
used? It is documented as "MAC reset signal" in [1]. So a PHY reset
should be separate from this.

[1] Documentation/devicetree/bindings/net/snps,dwmac.yaml

regards
Philipp

Re: [PATCHv1 2/3] ARM: dts: meson: Use new reset id for reset controller

[Anand Moon]

Hi Philipp,

Thanks for your review comments.

On Fri, 30 Jul 2021 at 15:16, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Fri, 2021-07-30 at 01:40 +0530, Anand Moon wrote:
> > Used new reset id for reset controller as it conflict
> > with the core reset id.
> >
> > Fixes: b96446541d83 ("ARM: dts: meson8b: extend ethernet controller description")
> >
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  arch/arm/boot/dts/meson8b.dtsi  | 2 +-
> >  arch/arm/boot/dts/meson8m2.dtsi | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> > index c02b03cbcdf4..cb3a579d09ef 100644
> > --- a/arch/arm/boot/dts/meson8b.dtsi
> > +++ b/arch/arm/boot/dts/meson8b.dtsi
> > @@ -511,7 +511,7 @@ &ethmac {
> >       tx-fifo-depth = <2048>;
> >
> >       resets = <&reset RESET_ETHERNET>;
> > -     reset-names = "stmmaceth";
> > +     reset-names = "ethreset";
>
> This looks like an incompatible change. Is the "stmmaceth" reset not
> used? It is documented as "MAC reset signal" in [1]. So a PHY reset
> should be separate from this.
>
> [1] Documentation/devicetree/bindings/net/snps,dwmac.yaml
>
From the above device tree binding is been used below.
  reset-names:
    const: stmmaceth

While testing new reset driver changes I was getting conflict with
reset id, see the below links
hence I opted for a new reset-names = "ethreset".

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/stmicro/stmmac/stmmac.h?h=v5.14-rc3#n12
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c?h=v5.14-rc3#n598

> regards
> Philipp

Thanks

-Anand

[PATCHv1 3/3] net: stmmac: dwmac-meson8b: Add reset controller for ethernet phy

[Anand Moon]

Add reset controller for Ethernet phy reset on every boot for
Amlogic SoC.

Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 .../ethernet/stmicro/stmmac/dwmac-meson8b.c   | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index c7a6588d9398..8b3b5e8c2a8a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -17,6 +17,7 @@
 #include <linux/of_net.h>
 #include <linux/mfd/syscon.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/stmmac.h>
 
 #include "stmmac_platform.h"
@@ -95,6 +96,7 @@ struct meson8b_dwmac {
 	u32				tx_delay_ns;
 	u32				rx_delay_ps;
 	struct clk			*timing_adj_clk;
+	struct reset_control		*eth_reset;
 };
 
 struct meson8b_dwmac_clk_configs {
@@ -384,6 +386,17 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 	meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TX_AND_PHY_REF_CLK,
 				PRG_ETH0_TX_AND_PHY_REF_CLK);
 
+	/* Make sure the Ethernet PHY is properly reseted, as U-Boot may leave
+	 * it at deasserted state, and thus it may fail to reset EMAC.
+	 *
+	 * This assumes the driver has exclusive access to the EPHY reset.
+	 */
+	ret = reset_control_reset(dwmac->eth_reset);
+	if (ret) {
+		dev_err(dwmac->dev, "Cannot reset internal PHY\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -465,6 +478,13 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 		goto err_remove_config_dt;
 	}
 
+	dwmac->eth_reset = devm_reset_control_get_exclusive(dwmac->dev, "ethreset");
+	if (IS_ERR_OR_NULL(dwmac->eth_reset)) {
+		dev_err(dwmac->dev, "Failed to get Ethernet reset\n");
+		ret = PTR_ERR(dwmac->eth_reset);
+		goto err_remove_config_dt;
+	}
+
 	ret = meson8b_init_rgmii_delays(dwmac);
 	if (ret)
 		goto err_remove_config_dt;
-- 
2.32.0

Re: [PATCHv1 3/3] net: stmmac: dwmac-meson8b: Add reset controller for ethernet phy

[Andrew Lunn]

> @@ -465,6 +478,13 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
>  		goto err_remove_config_dt;
>  	}
>  
> +	dwmac->eth_reset = devm_reset_control_get_exclusive(dwmac->dev, "ethreset");
> +	if (IS_ERR_OR_NULL(dwmac->eth_reset)) {
> +		dev_err(dwmac->dev, "Failed to get Ethernet reset\n");
> +		ret = PTR_ERR(dwmac->eth_reset);
> +		goto err_remove_config_dt;
> +	}
> +

Hi Anand

Since this is a new property, you need to handle it not being in the
DT blob. You probably need to use
devm_reset_control_get_optinal_exclusive()

	Andrew

Re: [PATCHv1 3/3] net: stmmac: dwmac-meson8b: Add reset controller for ethernet phy

[Martin Blumenstingl]

Hi Anand,

On Thu, Jul 29, 2021 at 10:11 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> Add reset controller for Ethernet phy reset on every boot for
> Amlogic SoC.
I think this description does not match what's going on inside the SoC:
- for all SoCs earlier than GXL the PHY is external so the reset for
the PHY is a GPIO
- the reset line you are passing in the .dts belongs to the Ethernet
controller on SoCs earlier than GXL
- I *believe* that the rset line which you're passing in the .dts
belongs to the Ethernet controller AND the built-in MDIO mux on GXL
and newer, see also [0]
- from how the PRG_ETH registers work I doubt that these are connected
to a reset line (as they're managing mostly delays and protocol - so I
don't see what would be reset). This is speculation though.


Best regards,
Martin


[0] https://lore.kernel.org/linux-amlogic/553e127c-9839-d15b-d435-c01f18c7be48@gmail.com/

Re: [PATCHv1 0/3] Add Reset controller to Ethernet PHY

[Martin Blumenstingl]

Hi Anand,

On Thu, Jul 29, 2021 at 10:11 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> It is being observed some time the Ethernet interface
> will not send / recive any packet after reboot.
>
> Earlier I had submitted Ethernet reset ID patch
> but it did not resolve it issue much, Adding new
> reset controller of the Ethernet PHY for Amlogic SoC
> could help resolve the issue.
nowhere in this series you are addressing the issue from [0]
Some more comments in the individual patch


Best regards,
Martin


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/amlogic?id=19f6fe976a61f9afc289b062b7ef67f99b72e7b9