[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Dave,

please do not apply this series until it got a Tested-by from Emiliano.


Hi Emiliano,

you reported [0] that you couldn't get dwmac-meson8b to work on your
Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
I think I was able to find a fix: it consists of two patches (which you
find in this series)

Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
only partially test this (I could only check if the clocks were
calculated correctly when using a dummy 500002394Hz input clock instead
of MPLL2).

Could you please give this series a try and let me know about the
results?
You obviously still need your two "ARM: dts: meson8b" patches which
- add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
- enable Ethernet on the Odroid-C1

When testing on Meson8b this also needs a fix for the MPLL clock driver:
"clk: meson: mpll: use 64-bit maths in params_from_rate", see:
https://patchwork.kernel.org/patch/10131677/


I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
fine (so let's hope that this also fixes your Meson8b issue :)).


changes since v1 at [1]:
- changed the subject of the cover-letter to indicate that this is all
  about the RGMII clock
- added PATCH #1 which ensures that we don't unnecessarily change the
  parent clocks in RMII mode (and also makes the code easier to
  understand)
- changed subject of PATCH #2 (formerly PATCH #1) to state that this
  is about the RGMII clock
- added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
- replaced PATCH #3 (formerly PATCH #2) with one that sets
  CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
  on Meson8b correctly

changes since v2 at [2]:
- added PATCH #2 to make the following patch easier
- Emiliano reported that there's currently another bug in the
  dwmac-meson8b driver which prevents it from working with RGMII PHYs on
  Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
  (instead of a divide by 5 or divide by 10 clock divider). This has not
  been visible on GXBB and later due to the input clock which always led
  to a selection of "divide by 10" (which is done internally in the IP
  block, but the bit actually means "enable RGMII clock output").
  PATCH #3 was added to address this issue.
- the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
  updated and the patch itself rebased because the m25_div clock was
  removed with the new PATCH #3 (so some of the statements were not
  valid anymore)


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html


Martin Blumenstingl (5):
  net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
  net: stmmac: dwmac-meson8b: simplify generating the clock names
  net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
  net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
  net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
 1 file changed, 63 insertions(+), 56 deletions(-)

-- 
2.15.1

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Dave,

please do not apply this series until it got a Tested-by from Emiliano.


Hi Emiliano,

you reported [0] that you couldn't get dwmac-meson8b to work on your
Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
I think I was able to find a fix: it consists of two patches (which you
find in this series)

Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
only partially test this (I could only check if the clocks were
calculated correctly when using a dummy 500002394Hz input clock instead
of MPLL2).

Could you please give this series a try and let me know about the
results?
You obviously still need your two "ARM: dts: meson8b" patches which
- add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
- enable Ethernet on the Odroid-C1

When testing on Meson8b this also needs a fix for the MPLL clock driver:
"clk: meson: mpll: use 64-bit maths in params_from_rate", see:
https://patchwork.kernel.org/patch/10131677/


I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
fine (so let's hope that this also fixes your Meson8b issue :)).


changes since v1 at [1]:
- changed the subject of the cover-letter to indicate that this is all
  about the RGMII clock
- added PATCH #1 which ensures that we don't unnecessarily change the
  parent clocks in RMII mode (and also makes the code easier to
  understand)
- changed subject of PATCH #2 (formerly PATCH #1) to state that this
  is about the RGMII clock
- added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
- replaced PATCH #3 (formerly PATCH #2) with one that sets
  CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
  on Meson8b correctly

changes since v2 at [2]:
- added PATCH #2 to make the following patch easier
- Emiliano reported that there's currently another bug in the
  dwmac-meson8b driver which prevents it from working with RGMII PHYs on
  Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
  (instead of a divide by 5 or divide by 10 clock divider). This has not
  been visible on GXBB and later due to the input clock which always led
  to a selection of "divide by 10" (which is done internally in the IP
  block, but the bit actually means "enable RGMII clock output").
  PATCH #3 was added to address this issue.
- the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
  updated and the patch itself rebased because the m25_div clock was
  removed with the new PATCH #3 (so some of the statements were not
  valid anymore)


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html


Martin Blumenstingl (5):
  net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
  net: stmmac: dwmac-meson8b: simplify generating the clock names
  net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
  net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
  net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
 1 file changed, 63 insertions(+), 56 deletions(-)

-- 
2.15.1

[RFT net-next v3 1/5] net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode

[Martin Blumenstingl]

Neither the m25_div_clk nor the m250_div_clk or m250_mux_clk are used in
RMII mode. The m25_div_clk output is routed to the RGMII PHY's "RGMII
clock".
This means that we don't need to configure the clocks in RMII mode. The
driver however did this - with no effect since the clocks are not routed
to the PHY in RMII mode.

While here also rename meson8b_init_clk to meson8b_init_rgmii_clk to
make it easier to understand the code.

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 46 ++++++++++------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 4404650b32c5..e1d5907e481c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -81,7 +81,7 @@ static void meson8b_dwmac_mask_bits(struct meson8b_dwmac *dwmac, u32 reg,
 	writel(data, dwmac->regs + reg);
 }
 
-static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
+static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 {
 	struct clk_init_data init;
 	int i, ret;
@@ -176,7 +176,6 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
 static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 {
 	int ret;
-	unsigned long clk_rate;
 	u8 tx_dly_val = 0;
 
 	switch (dwmac->phy_mode) {
@@ -191,9 +190,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 
 	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		/* Generate a 25MHz clock for the PHY */
-		clk_rate = 25 * 1000 * 1000;
-
 		/* enable RGMII mode */
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE,
 					PRG_ETH0_RGMII_MODE);
@@ -204,12 +200,24 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
 					tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
+
+		ret = clk_prepare_enable(dwmac->m25_div_clk);
+		if (ret) {
+			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
+			return ret;
+		}
+
+		/* Generate the 25MHz RGMII clock for the PHY */
+		ret = clk_set_rate(dwmac->m25_div_clk, 25 * 1000 * 1000);
+		if (ret) {
+			clk_disable_unprepare(dwmac->m25_div_clk);
+
+			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
+			return ret;
+		}
 		break;
 
 	case PHY_INTERFACE_MODE_RMII:
-		/* Use the rate of the mux clock for the internal RMII PHY */
-		clk_rate = clk_get_rate(dwmac->m250_mux_clk);
-
 		/* disable RGMII mode -> enables RMII mode */
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE,
 					0);
@@ -231,20 +239,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 		return -EINVAL;
 	}
 
-	ret = clk_prepare_enable(dwmac->m25_div_clk);
-	if (ret) {
-		dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
-		return ret;
-	}
-
-	ret = clk_set_rate(dwmac->m25_div_clk, clk_rate);
-	if (ret) {
-		clk_disable_unprepare(dwmac->m25_div_clk);
-
-		dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
-		return ret;
-	}
-
 	/* enable TX_CLK and PHY_REF_CLK generator */
 	meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TX_AND_PHY_REF_CLK,
 				PRG_ETH0_TX_AND_PHY_REF_CLK);
@@ -294,7 +288,7 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 				 &dwmac->tx_delay_ns))
 		dwmac->tx_delay_ns = 2;
 
-	ret = meson8b_init_clk(dwmac);
+	ret = meson8b_init_rgmii_clk(dwmac);
 	if (ret)
 		goto err_remove_config_dt;
 
@@ -311,7 +305,8 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 	return 0;
 
 err_clk_disable:
-	clk_disable_unprepare(dwmac->m25_div_clk);
+	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
+		clk_disable_unprepare(dwmac->m25_div_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -322,7 +317,8 @@ static int meson8b_dwmac_remove(struct platform_device *pdev)
 {
 	struct meson8b_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
 
-	clk_disable_unprepare(dwmac->m25_div_clk);
+	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
+		clk_disable_unprepare(dwmac->m25_div_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

[RFT net-next v3 1/5] net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode

[Martin Blumenstingl]

Neither the m25_div_clk nor the m250_div_clk or m250_mux_clk are used in
RMII mode. The m25_div_clk output is routed to the RGMII PHY's "RGMII
clock".
This means that we don't need to configure the clocks in RMII mode. The
driver however did this - with no effect since the clocks are not routed
to the PHY in RMII mode.

While here also rename meson8b_init_clk to meson8b_init_rgmii_clk to
make it easier to understand the code.

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 46 ++++++++++------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 4404650b32c5..e1d5907e481c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -81,7 +81,7 @@ static void meson8b_dwmac_mask_bits(struct meson8b_dwmac *dwmac, u32 reg,
 	writel(data, dwmac->regs + reg);
 }
 
-static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
+static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 {
 	struct clk_init_data init;
 	int i, ret;
@@ -176,7 +176,6 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac)
 static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 {
 	int ret;
-	unsigned long clk_rate;
 	u8 tx_dly_val = 0;
 
 	switch (dwmac->phy_mode) {
@@ -191,9 +190,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 
 	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		/* Generate a 25MHz clock for the PHY */
-		clk_rate = 25 * 1000 * 1000;
-
 		/* enable RGMII mode */
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE,
 					PRG_ETH0_RGMII_MODE);
@@ -204,12 +200,24 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
 					tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
+
+		ret = clk_prepare_enable(dwmac->m25_div_clk);
+		if (ret) {
+			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
+			return ret;
+		}
+
+		/* Generate the 25MHz RGMII clock for the PHY */
+		ret = clk_set_rate(dwmac->m25_div_clk, 25 * 1000 * 1000);
+		if (ret) {
+			clk_disable_unprepare(dwmac->m25_div_clk);
+
+			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
+			return ret;
+		}
 		break;
 
 	case PHY_INTERFACE_MODE_RMII:
-		/* Use the rate of the mux clock for the internal RMII PHY */
-		clk_rate = clk_get_rate(dwmac->m250_mux_clk);
-
 		/* disable RGMII mode -> enables RMII mode */
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE,
 					0);
@@ -231,20 +239,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 		return -EINVAL;
 	}
 
-	ret = clk_prepare_enable(dwmac->m25_div_clk);
-	if (ret) {
-		dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
-		return ret;
-	}
-
-	ret = clk_set_rate(dwmac->m25_div_clk, clk_rate);
-	if (ret) {
-		clk_disable_unprepare(dwmac->m25_div_clk);
-
-		dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
-		return ret;
-	}
-
 	/* enable TX_CLK and PHY_REF_CLK generator */
 	meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TX_AND_PHY_REF_CLK,
 				PRG_ETH0_TX_AND_PHY_REF_CLK);
@@ -294,7 +288,7 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 				 &dwmac->tx_delay_ns))
 		dwmac->tx_delay_ns = 2;
 
-	ret = meson8b_init_clk(dwmac);
+	ret = meson8b_init_rgmii_clk(dwmac);
 	if (ret)
 		goto err_remove_config_dt;
 
@@ -311,7 +305,8 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 	return 0;
 
 err_clk_disable:
-	clk_disable_unprepare(dwmac->m25_div_clk);
+	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
+		clk_disable_unprepare(dwmac->m25_div_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -322,7 +317,8 @@ static int meson8b_dwmac_remove(struct platform_device *pdev)
 {
 	struct meson8b_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
 
-	clk_disable_unprepare(dwmac->m25_div_clk);
+	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
+		clk_disable_unprepare(dwmac->m25_div_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

[RFT net-next v3 2/5] net: stmmac: dwmac-meson8b: simplify generating the clock names

[Martin Blumenstingl]

Instead of using a custom buffer, snprintf() and devm_kstrdup() we can
simplify this by using devm_kasprintf().
No functional changes - this just makes the code shorter.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index e1d5907e481c..1c14210df465 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -86,7 +86,6 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	struct clk_init_data init;
 	int i, ret;
 	struct device *dev = &dwmac->pdev->dev;
-	char clk_name[32];
 	const char *clk_div_parents[1];
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
 	static const struct clk_div_table clk_25m_div_table[] = {
@@ -113,8 +112,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	}
 
 	/* create the m250_mux */
-	snprintf(clk_name, sizeof(clk_name), "%s#m250_sel", dev_name(dev));
-	init.name = clk_name;
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_sel",
+				   dev_name(dev));
 	init.ops = &clk_mux_ops;
 	init.flags = 0;
 	init.parent_names = mux_parent_names;
@@ -132,8 +131,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 		return PTR_ERR(dwmac->m250_mux_clk);
 
 	/* create the m250_div */
-	snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
-	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_div",
+				   dev_name(dev));
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
@@ -151,8 +150,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 		return PTR_ERR(dwmac->m250_div_clk);
 
 	/* create the m25_div */
-	snprintf(clk_name, sizeof(clk_name), "%s#m25_div", dev_name(dev));
-	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_div",
+				   dev_name(dev));
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk);
-- 
2.15.1

[RFT net-next v3 2/5] net: stmmac: dwmac-meson8b: simplify generating the clock names

[Martin Blumenstingl]

Instead of using a custom buffer, snprintf() and devm_kstrdup() we can
simplify this by using devm_kasprintf().
No functional changes - this just makes the code shorter.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index e1d5907e481c..1c14210df465 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -86,7 +86,6 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	struct clk_init_data init;
 	int i, ret;
 	struct device *dev = &dwmac->pdev->dev;
-	char clk_name[32];
 	const char *clk_div_parents[1];
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
 	static const struct clk_div_table clk_25m_div_table[] = {
@@ -113,8 +112,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	}
 
 	/* create the m250_mux */
-	snprintf(clk_name, sizeof(clk_name), "%s#m250_sel", dev_name(dev));
-	init.name = clk_name;
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_sel",
+				   dev_name(dev));
 	init.ops = &clk_mux_ops;
 	init.flags = 0;
 	init.parent_names = mux_parent_names;
@@ -132,8 +131,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 		return PTR_ERR(dwmac->m250_mux_clk);
 
 	/* create the m250_div */
-	snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
-	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_div",
+				   dev_name(dev));
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
@@ -151,8 +150,8 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 		return PTR_ERR(dwmac->m250_div_clk);
 
 	/* create the m25_div */
-	snprintf(clk_name, sizeof(clk_name), "%s#m25_div", dev_name(dev));
-	init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_div",
+				   dev_name(dev));
 	init.ops = &clk_divider_ops;
 	init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk);
-- 
2.15.1

[RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Martin Blumenstingl]

While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
discovered that the m25_div is not actually a divider but rather a gate.
This matches with the datasheet which describes bit 10 as "Generate
25MHz clock for PHY". Back when the driver was written it was assumed
that this was a divider (which could divide by 5 or 10) because other
clock bits in the datasheet were documented incorrectly.

Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
Odroid-C1 (using a Meson8b SoC) does not work.
On GXBB and newer SoCs (where the driver was initially tested with RGMII
PHYs) this is not a problem because the input clock is running at 1GHz.
The m250_div clock's biggest possible divider is 7 (3-bit divider, with
value 0 being reserved). Thus we end up with a m250_div of 4 and a
"m25_div" of 10 (= register value 1).

Instead it turns out that the Ethernet IP block seems to have a fixed
"divide by 10" clock internally. This means that bit 10 is a gate clock
which enables the RGMII clock output.

This replaces the "m25_div" clock with a clock gate called "m25_en"
which ensures that we can set this bit to 1 whenever we enable RGMII
mode. This however means that we are now missing a "divide by 10" after
the m250_div (and before our new m25_en), otherwise the common clock
framework thinks that the rate of the m25_en clock is 10-times higher
than it is in the actual hardware. That is solved by adding a
fixed-factor clock which divides the m250_div output by 10.

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 1c14210df465..7199e8c08536 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -40,9 +40,7 @@
 #define PRG_ETH0_CLK_M250_DIV_SHIFT	7
 #define PRG_ETH0_CLK_M250_DIV_WIDTH	3
 
-/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
-#define PRG_ETH0_CLK_M25_DIV_SHIFT	10
-#define PRG_ETH0_CLK_M25_DIV_WIDTH	1
+#define PRG_ETH0_GENERATE_25M_PHY_CLOCK	10
 
 #define PRG_ETH0_INVERTED_RMII_CLK	BIT(11)
 #define PRG_ETH0_TX_AND_PHY_REF_CLK	BIT(12)
@@ -63,8 +61,11 @@ struct meson8b_dwmac {
 	struct clk_divider	m250_div;
 	struct clk		*m250_div_clk;
 
-	struct clk_divider	m25_div;
-	struct clk		*m25_div_clk;
+	struct clk_fixed_factor	fixed_div10;
+	struct clk		*fixed_div10_clk;
+
+	struct clk_gate		m25_en;
+	struct clk		*m25_en_clk;
 
 	u32			tx_delay_ns;
 };
@@ -88,11 +89,6 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	struct device *dev = &dwmac->pdev->dev;
 	const char *clk_div_parents[1];
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
-	static const struct clk_div_table clk_25m_div_table[] = {
-		{ .val = 0, .div = 5 },
-		{ .val = 1, .div = 10 },
-		{ /* sentinel */ },
-	};
 
 	/* get the mux parents from DT */
 	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
@@ -149,25 +145,39 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	if (WARN_ON(IS_ERR(dwmac->m250_div_clk)))
 		return PTR_ERR(dwmac->m250_div_clk);
 
-	/* create the m25_div */
-	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_div",
+	/* create the fixed_div10 */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div10",
 				   dev_name(dev));
-	init.ops = &clk_divider_ops;
-	init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+	init.ops = &clk_fixed_factor_ops;
+	init.flags = CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk);
 	init.parent_names = clk_div_parents;
 	init.num_parents = ARRAY_SIZE(clk_div_parents);
 
-	dwmac->m25_div.reg = dwmac->regs + PRG_ETH0;
-	dwmac->m25_div.shift = PRG_ETH0_CLK_M25_DIV_SHIFT;
-	dwmac->m25_div.width = PRG_ETH0_CLK_M25_DIV_WIDTH;
-	dwmac->m25_div.table = clk_25m_div_table;
-	dwmac->m25_div.hw.init = &init;
-	dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->fixed_div10.mult = 1;
+	dwmac->fixed_div10.div = 10;
+	dwmac->fixed_div10.hw.init = &init;
+
+	dwmac->fixed_div10_clk = devm_clk_register(dev, &dwmac->fixed_div10.hw);
+	if (WARN_ON(IS_ERR(dwmac->fixed_div10_clk)))
+		return PTR_ERR(dwmac->fixed_div10_clk);
+
+	/* create the m25_en */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_en",
+				   dev_name(dev));
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	clk_div_parents[0] = __clk_get_name(dwmac->fixed_div10_clk);
+	init.parent_names = clk_div_parents;
+	init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+	dwmac->m25_en.reg = dwmac->regs + PRG_ETH0;
+	dwmac->m25_en.bit_idx = PRG_ETH0_GENERATE_25M_PHY_CLOCK;
+	dwmac->m25_en.hw.init = &init;
 
-	dwmac->m25_div_clk = devm_clk_register(dev, &dwmac->m25_div.hw);
-	if (WARN_ON(IS_ERR(dwmac->m25_div_clk)))
-		return PTR_ERR(dwmac->m25_div_clk);
+	dwmac->m25_en_clk = devm_clk_register(dev, &dwmac->m25_en.hw);
+	if (WARN_ON(IS_ERR(dwmac->m25_en_clk)))
+		return PTR_ERR(dwmac->m25_en_clk);
 
 	return 0;
 }
@@ -200,16 +210,16 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
 					tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
 
-		ret = clk_prepare_enable(dwmac->m25_div_clk);
+		ret = clk_prepare_enable(dwmac->m25_en_clk);
 		if (ret) {
 			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
 			return ret;
 		}
 
 		/* Generate the 25MHz RGMII clock for the PHY */
-		ret = clk_set_rate(dwmac->m25_div_clk, 25 * 1000 * 1000);
+		ret = clk_set_rate(dwmac->m25_en_clk, 25 * 1000 * 1000);
 		if (ret) {
-			clk_disable_unprepare(dwmac->m25_div_clk);
+			clk_disable_unprepare(dwmac->m25_en_clk);
 
 			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
 			return ret;
@@ -305,7 +315,7 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 
 err_clk_disable:
 	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
-		clk_disable_unprepare(dwmac->m25_div_clk);
+		clk_disable_unprepare(dwmac->m25_en_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -317,7 +327,7 @@ static int meson8b_dwmac_remove(struct platform_device *pdev)
 	struct meson8b_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
 
 	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
-		clk_disable_unprepare(dwmac->m25_div_clk);
+		clk_disable_unprepare(dwmac->m25_en_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

[RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Martin Blumenstingl]

While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
discovered that the m25_div is not actually a divider but rather a gate.
This matches with the datasheet which describes bit 10 as "Generate
25MHz clock for PHY". Back when the driver was written it was assumed
that this was a divider (which could divide by 5 or 10) because other
clock bits in the datasheet were documented incorrectly.

Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
Odroid-C1 (using a Meson8b SoC) does not work.
On GXBB and newer SoCs (where the driver was initially tested with RGMII
PHYs) this is not a problem because the input clock is running at 1GHz.
The m250_div clock's biggest possible divider is 7 (3-bit divider, with
value 0 being reserved). Thus we end up with a m250_div of 4 and a
"m25_div" of 10 (= register value 1).

Instead it turns out that the Ethernet IP block seems to have a fixed
"divide by 10" clock internally. This means that bit 10 is a gate clock
which enables the RGMII clock output.

This replaces the "m25_div" clock with a clock gate called "m25_en"
which ensures that we can set this bit to 1 whenever we enable RGMII
mode. This however means that we are now missing a "divide by 10" after
the m250_div (and before our new m25_en), otherwise the common clock
framework thinks that the rate of the m25_en clock is 10-times higher
than it is in the actual hardware. That is solved by adding a
fixed-factor clock which divides the m250_div output by 10.

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 1c14210df465..7199e8c08536 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -40,9 +40,7 @@
 #define PRG_ETH0_CLK_M250_DIV_SHIFT	7
 #define PRG_ETH0_CLK_M250_DIV_WIDTH	3
 
-/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
-#define PRG_ETH0_CLK_M25_DIV_SHIFT	10
-#define PRG_ETH0_CLK_M25_DIV_WIDTH	1
+#define PRG_ETH0_GENERATE_25M_PHY_CLOCK	10
 
 #define PRG_ETH0_INVERTED_RMII_CLK	BIT(11)
 #define PRG_ETH0_TX_AND_PHY_REF_CLK	BIT(12)
@@ -63,8 +61,11 @@ struct meson8b_dwmac {
 	struct clk_divider	m250_div;
 	struct clk		*m250_div_clk;
 
-	struct clk_divider	m25_div;
-	struct clk		*m25_div_clk;
+	struct clk_fixed_factor	fixed_div10;
+	struct clk		*fixed_div10_clk;
+
+	struct clk_gate		m25_en;
+	struct clk		*m25_en_clk;
 
 	u32			tx_delay_ns;
 };
@@ -88,11 +89,6 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	struct device *dev = &dwmac->pdev->dev;
 	const char *clk_div_parents[1];
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
-	static const struct clk_div_table clk_25m_div_table[] = {
-		{ .val = 0, .div = 5 },
-		{ .val = 1, .div = 10 },
-		{ /* sentinel */ },
-	};
 
 	/* get the mux parents from DT */
 	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
@@ -149,25 +145,39 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	if (WARN_ON(IS_ERR(dwmac->m250_div_clk)))
 		return PTR_ERR(dwmac->m250_div_clk);
 
-	/* create the m25_div */
-	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_div",
+	/* create the fixed_div10 */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div10",
 				   dev_name(dev));
-	init.ops = &clk_divider_ops;
-	init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+	init.ops = &clk_fixed_factor_ops;
+	init.flags = CLK_SET_RATE_PARENT;
 	clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk);
 	init.parent_names = clk_div_parents;
 	init.num_parents = ARRAY_SIZE(clk_div_parents);
 
-	dwmac->m25_div.reg = dwmac->regs + PRG_ETH0;
-	dwmac->m25_div.shift = PRG_ETH0_CLK_M25_DIV_SHIFT;
-	dwmac->m25_div.width = PRG_ETH0_CLK_M25_DIV_WIDTH;
-	dwmac->m25_div.table = clk_25m_div_table;
-	dwmac->m25_div.hw.init = &init;
-	dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->fixed_div10.mult = 1;
+	dwmac->fixed_div10.div = 10;
+	dwmac->fixed_div10.hw.init = &init;
+
+	dwmac->fixed_div10_clk = devm_clk_register(dev, &dwmac->fixed_div10.hw);
+	if (WARN_ON(IS_ERR(dwmac->fixed_div10_clk)))
+		return PTR_ERR(dwmac->fixed_div10_clk);
+
+	/* create the m25_en */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m25_en",
+				   dev_name(dev));
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	clk_div_parents[0] = __clk_get_name(dwmac->fixed_div10_clk);
+	init.parent_names = clk_div_parents;
+	init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+	dwmac->m25_en.reg = dwmac->regs + PRG_ETH0;
+	dwmac->m25_en.bit_idx = PRG_ETH0_GENERATE_25M_PHY_CLOCK;
+	dwmac->m25_en.hw.init = &init;
 
-	dwmac->m25_div_clk = devm_clk_register(dev, &dwmac->m25_div.hw);
-	if (WARN_ON(IS_ERR(dwmac->m25_div_clk)))
-		return PTR_ERR(dwmac->m25_div_clk);
+	dwmac->m25_en_clk = devm_clk_register(dev, &dwmac->m25_en.hw);
+	if (WARN_ON(IS_ERR(dwmac->m25_en_clk)))
+		return PTR_ERR(dwmac->m25_en_clk);
 
 	return 0;
 }
@@ -200,16 +210,16 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
 		meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
 					tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
 
-		ret = clk_prepare_enable(dwmac->m25_div_clk);
+		ret = clk_prepare_enable(dwmac->m25_en_clk);
 		if (ret) {
 			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
 			return ret;
 		}
 
 		/* Generate the 25MHz RGMII clock for the PHY */
-		ret = clk_set_rate(dwmac->m25_div_clk, 25 * 1000 * 1000);
+		ret = clk_set_rate(dwmac->m25_en_clk, 25 * 1000 * 1000);
 		if (ret) {
-			clk_disable_unprepare(dwmac->m25_div_clk);
+			clk_disable_unprepare(dwmac->m25_en_clk);
 
 			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
 			return ret;
@@ -305,7 +315,7 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 
 err_clk_disable:
 	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
-		clk_disable_unprepare(dwmac->m25_div_clk);
+		clk_disable_unprepare(dwmac->m25_en_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -317,7 +327,7 @@ static int meson8b_dwmac_remove(struct platform_device *pdev)
 	struct meson8b_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
 
 	if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
-		clk_disable_unprepare(dwmac->m25_div_clk);
+		clk_disable_unprepare(dwmac->m25_en_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

[RFT net-next v3 4/5] net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b

[Martin Blumenstingl]

Meson8b only supports MPLL2 as clock input. The rate of the MPLL2 clock
set by Odroid-C1's u-boot is close to 500MHz. The exact rate is
500002394Hz, which is calculated in drivers/clk/meson/clk-mpll.c
using the following formula:
DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, (SDM_DEN * n2) + sdm)
Odroid-C1's u-boot configures MPLL2 with the following values:
- SDM_DEN = 16384
- SDM = 1638
- N2 = 5

The 250MHz clock (m250_div) inside dwmac-meson8b driver is derived from
the MPLL2 clock. Due to MPLL2 running slightly faster than 500MHz the
common clock framework chooses a divider which is too big to generate
the 250MHz clock (a divider of 2 would be needed, but this is rounded up
to a divider of 3). This breaks the RTL8211F RGMII PHY on Odroid-C1
because it requires a (close to) 25MHz clock.

Round the divider to the closest value to prevent this issue on Meson8b.
This means we'll now end up with a clock rate of 25000120Hz (= 25MHz
plus 120Hz).
This has no effect on the Meson GX SoCs since there fclk_div2 is used as
input clock, which has a rate of 1000MHz (and thus is divisible cleanly
to 250MHz and 25MHz).

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 7199e8c08536..d06106417063 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -139,7 +139,9 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	dwmac->m250_div.shift = PRG_ETH0_CLK_M250_DIV_SHIFT;
 	dwmac->m250_div.width = PRG_ETH0_CLK_M250_DIV_WIDTH;
 	dwmac->m250_div.hw.init = &init;
-	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED |
+				CLK_DIVIDER_ALLOW_ZERO |
+				CLK_DIVIDER_ROUND_CLOSEST;
 
 	dwmac->m250_div_clk = devm_clk_register(dev, &dwmac->m250_div.hw);
 	if (WARN_ON(IS_ERR(dwmac->m250_div_clk)))
-- 
2.15.1

[RFT net-next v3 4/5] net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b

[Martin Blumenstingl]

Meson8b only supports MPLL2 as clock input. The rate of the MPLL2 clock
set by Odroid-C1's u-boot is close to 500MHz. The exact rate is
500002394Hz, which is calculated in drivers/clk/meson/clk-mpll.c
using the following formula:
DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, (SDM_DEN * n2) + sdm)
Odroid-C1's u-boot configures MPLL2 with the following values:
- SDM_DEN = 16384
- SDM = 1638
- N2 = 5

The 250MHz clock (m250_div) inside dwmac-meson8b driver is derived from
the MPLL2 clock. Due to MPLL2 running slightly faster than 500MHz the
common clock framework chooses a divider which is too big to generate
the 250MHz clock (a divider of 2 would be needed, but this is rounded up
to a divider of 3). This breaks the RTL8211F RGMII PHY on Odroid-C1
because it requires a (close to) 25MHz clock.

Round the divider to the closest value to prevent this issue on Meson8b.
This means we'll now end up with a clock rate of 25000120Hz (= 25MHz
plus 120Hz).
This has no effect on the Meson GX SoCs since there fclk_div2 is used as
input clock, which has a rate of 1000MHz (and thus is divisible cleanly
to 250MHz and 25MHz).

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 7199e8c08536..d06106417063 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -139,7 +139,9 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	dwmac->m250_div.shift = PRG_ETH0_CLK_M250_DIV_SHIFT;
 	dwmac->m250_div.width = PRG_ETH0_CLK_M250_DIV_WIDTH;
 	dwmac->m250_div.hw.init = &init;
-	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO;
+	dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED |
+				CLK_DIVIDER_ALLOW_ZERO |
+				CLK_DIVIDER_ROUND_CLOSEST;
 
 	dwmac->m250_div_clk = devm_clk_register(dev, &dwmac->m250_div.hw);
 	if (WARN_ON(IS_ERR(dwmac->m250_div_clk)))
-- 
2.15.1

[RFT net-next v3 5/5] net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

[Martin Blumenstingl]

On Meson8b the only valid input clock is MPLL2. The bootloader
configures that to run at 500002394Hz which cannot be divided evenly
down to 25MHz using the m250_div and m25_div clocks. Currently the
common clock framework chooses a m250_div of 2 - with the internal fixed
"divide by 10" this results in a RGMII clock of 25000120Hz (120Hz above
the requested 25MHz).

Letting the common clock framework propagate the rate changes up to the
parent of m250_mux allows us to get the best possible clock rate. With
this patch the common clock framework calculates a rate of
very-close-to-250MHz (249999701Hz to be exact) for the MPLL2 clock
(which is the mux input). Dividing that by 1 (using m250_div) along with
the internal fixed divide-by-10 gives us a RGMII clock of 24999970Hz
(which is only 30Hz off the requested 25MHz, compared to 120Hz from
u-boot and the vendor driver).

SoCs from the Meson GX series are not affected by this change because
the input clock is FCLK_DIV2 whose rate cannot be changed (which is fine
since it's running at 1GHz, thus it's a multiple of 250MHz).

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index d06106417063..9c3cdfef414a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -111,7 +111,7 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_sel",
 				   dev_name(dev));
 	init.ops = &clk_mux_ops;
-	init.flags = 0;
+	init.flags = CLK_SET_RATE_PARENT;
 	init.parent_names = mux_parent_names;
 	init.num_parents = MUX_CLK_NUM_PARENTS;
 
-- 
2.15.1

[RFT net-next v3 5/5] net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

[Martin Blumenstingl]

On Meson8b the only valid input clock is MPLL2. The bootloader
configures that to run at 500002394Hz which cannot be divided evenly
down to 25MHz using the m250_div and m25_div clocks. Currently the
common clock framework chooses a m250_div of 2 - with the internal fixed
"divide by 10" this results in a RGMII clock of 25000120Hz (120Hz above
the requested 25MHz).

Letting the common clock framework propagate the rate changes up to the
parent of m250_mux allows us to get the best possible clock rate. With
this patch the common clock framework calculates a rate of
very-close-to-250MHz (249999701Hz to be exact) for the MPLL2 clock
(which is the mux input). Dividing that by 1 (using m250_div) along with
the internal fixed divide-by-10 gives us a RGMII clock of 24999970Hz
(which is only 30Hz off the requested 25MHz, compared to 120Hz from
u-boot and the vendor driver).

SoCs from the Meson GX series are not affected by this change because
the input clock is FCLK_DIV2 whose rate cannot be changed (which is fine
since it's running at 1GHz, thus it's a multiple of 250MHz).

Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index d06106417063..9c3cdfef414a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -111,7 +111,7 @@ static int meson8b_init_rgmii_clk(struct meson8b_dwmac *dwmac)
 	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#m250_sel",
 				   dev_name(dev));
 	init.ops = &clk_mux_ops;
-	init.flags = 0;
+	init.flags = CLK_SET_RATE_PARENT;
 	init.parent_names = mux_parent_names;
 	init.num_parents = MUX_CLK_NUM_PARENTS;
 
-- 
2.15.1

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

Hi Martin, Hi Dave,

On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> Hi Dave,
> 
> please do not apply this series until it got a Tested-by from Emiliano.
> 
> 
> Hi Emiliano,
> 
> you reported [0] that you couldn't get dwmac-meson8b to work on your
> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> I think I was able to find a fix: it consists of two patches (which you
> find in this series)
> 
> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> only partially test this (I could only check if the clocks were
> calculated correctly when using a dummy 500002394Hz input clock instead
> of MPLL2).
> 
> Could you please give this series a try and let me know about the
> results?
> You obviously still need your two "ARM: dts: meson8b" patches which
> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> - enable Ethernet on the Odroid-C1
> 
> When testing on Meson8b this also needs a fix for the MPLL clock driver:
> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> https://patchwork.kernel.org/patch/10131677/
> 
> 
> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> fine (so let's hope that this also fixes your Meson8b issue :)).
> 
> 
> changes since v1 at [1]:
> - changed the subject of the cover-letter to indicate that this is all
>   about the RGMII clock
> - added PATCH #1 which ensures that we don't unnecessarily change the
>   parent clocks in RMII mode (and also makes the code easier to
>   understand)
> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>   is about the RGMII clock
> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>   on Meson8b correctly
> 
> changes since v2 at [2]:
> - added PATCH #2 to make the following patch easier
> - Emiliano reported that there's currently another bug in the
>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>   been visible on GXBB and later due to the input clock which always led
>   to a selection of "divide by 10" (which is done internally in the IP
>   block, but the bit actually means "enable RGMII clock output").
>   PATCH #3 was added to address this issue.
> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>   updated and the patch itself rebased because the m25_div clock was
>   removed with the new PATCH #3 (so some of the statements were not
>   valid anymore)
>

Here is the clk_summary relative to ethernet on Odroid-C1+
with this new series applied:

xtal				    1            1    24000000          0 0
 sys_pll			    0            0  1200000000          0 0
  cpu_clk			    0            0  1200000000          0 0
 vid_pll			    0            0   732000000          0 0
 fixed_pll			    2            2  2550000000          0 0
  mpll2				    1            1   249999701          0 0
   c9410000.ethernet#m250_sel       1            1   249999701          0 0
    c9410000.ethernet#m250_div	    1            1   249999701          0 0
     c9410000.ethernet#fixed_div10  1            1    24999970          0 0
      c9410000.ethernet#m25_en	    1            1    24999970          0 0

The ethernet prg0 register is set to 0x74A1 which should be correct with
respect to the information contained in the S805 SoC manual.
Actually, the ethernet is not yet fully functional.
Trying to ping the board, I can see ARP request from host to board using
tcpdump. However, the host can't see any response.

Following the U-Boot value for prg0 register, which is 0x7d21, I also
tried to set bit 11. As expected, this did not have any influence.
Another thing that we should check is the "Ethernet Memory PD" (see S805
manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
normal operation. However, those bits are already cleared by U-Boot.

Thank you for the support.

Best regards,

Emiliano

> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> 
> 
> Martin Blumenstingl (5):
>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>  1 file changed, 63 insertions(+), 56 deletions(-)
> 
> -- 
> 2.15.1
> 

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

Hi Martin, Hi Dave,

On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> Hi Dave,
> 
> please do not apply this series until it got a Tested-by from Emiliano.
> 
> 
> Hi Emiliano,
> 
> you reported [0] that you couldn't get dwmac-meson8b to work on your
> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> I think I was able to find a fix: it consists of two patches (which you
> find in this series)
> 
> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> only partially test this (I could only check if the clocks were
> calculated correctly when using a dummy 500002394Hz input clock instead
> of MPLL2).
> 
> Could you please give this series a try and let me know about the
> results?
> You obviously still need your two "ARM: dts: meson8b" patches which
> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> - enable Ethernet on the Odroid-C1
> 
> When testing on Meson8b this also needs a fix for the MPLL clock driver:
> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> https://patchwork.kernel.org/patch/10131677/
> 
> 
> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> fine (so let's hope that this also fixes your Meson8b issue :)).
> 
> 
> changes since v1 at [1]:
> - changed the subject of the cover-letter to indicate that this is all
>   about the RGMII clock
> - added PATCH #1 which ensures that we don't unnecessarily change the
>   parent clocks in RMII mode (and also makes the code easier to
>   understand)
> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>   is about the RGMII clock
> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>   on Meson8b correctly
> 
> changes since v2 at [2]:
> - added PATCH #2 to make the following patch easier
> - Emiliano reported that there's currently another bug in the
>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>   been visible on GXBB and later due to the input clock which always led
>   to a selection of "divide by 10" (which is done internally in the IP
>   block, but the bit actually means "enable RGMII clock output").
>   PATCH #3 was added to address this issue.
> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>   updated and the patch itself rebased because the m25_div clock was
>   removed with the new PATCH #3 (so some of the statements were not
>   valid anymore)
>

Here is the clk_summary relative to ethernet on Odroid-C1+
with this new series applied:

xtal				    1            1    24000000          0 0
 sys_pll			    0            0  1200000000          0 0
  cpu_clk			    0            0  1200000000          0 0
 vid_pll			    0            0   732000000          0 0
 fixed_pll			    2            2  2550000000          0 0
  mpll2				    1            1   249999701          0 0
   c9410000.ethernet#m250_sel       1            1   249999701          0 0
    c9410000.ethernet#m250_div	    1            1   249999701          0 0
     c9410000.ethernet#fixed_div10  1            1    24999970          0 0
      c9410000.ethernet#m25_en	    1            1    24999970          0 0

The ethernet prg0 register is set to 0x74A1 which should be correct with
respect to the information contained in the S805 SoC manual.
Actually, the ethernet is not yet fully functional.
Trying to ping the board, I can see ARP request from host to board using
tcpdump. However, the host can't see any response.

Following the U-Boot value for prg0 register, which is 0x7d21, I also
tried to set bit 11. As expected, this did not have any influence.
Another thing that we should check is the "Ethernet Memory PD" (see S805
manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
normal operation. However, those bits are already cleared by U-Boot.

Thank you for the support.

Best regards,

Emiliano

> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> 
> 
> Martin Blumenstingl (5):
>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>  1 file changed, 63 insertions(+), 56 deletions(-)
> 
> -- 
> 2.15.1
> 

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Emiliano,

On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> Hi Martin, Hi Dave,
>
> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>> Hi Dave,
>>
>> please do not apply this series until it got a Tested-by from Emiliano.
>>
>>
>> Hi Emiliano,
>>
>> you reported [0] that you couldn't get dwmac-meson8b to work on your
>> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>> I think I was able to find a fix: it consists of two patches (which you
>> find in this series)
>>
>> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>> only partially test this (I could only check if the clocks were
>> calculated correctly when using a dummy 500002394Hz input clock instead
>> of MPLL2).
>>
>> Could you please give this series a try and let me know about the
>> results?
>> You obviously still need your two "ARM: dts: meson8b" patches which
>> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>> - enable Ethernet on the Odroid-C1
>>
>> When testing on Meson8b this also needs a fix for the MPLL clock driver:
>> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>> https://patchwork.kernel.org/patch/10131677/
>>
>>
>> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>> fine (so let's hope that this also fixes your Meson8b issue :)).
>>
>>
>> changes since v1 at [1]:
>> - changed the subject of the cover-letter to indicate that this is all
>>   about the RGMII clock
>> - added PATCH #1 which ensures that we don't unnecessarily change the
>>   parent clocks in RMII mode (and also makes the code easier to
>>   understand)
>> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>>   is about the RGMII clock
>> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>>   on Meson8b correctly
>>
>> changes since v2 at [2]:
>> - added PATCH #2 to make the following patch easier
>> - Emiliano reported that there's currently another bug in the
>>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>>   been visible on GXBB and later due to the input clock which always led
>>   to a selection of "divide by 10" (which is done internally in the IP
>>   block, but the bit actually means "enable RGMII clock output").
>>   PATCH #3 was added to address this issue.
>> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>>   updated and the patch itself rebased because the m25_div clock was
>>   removed with the new PATCH #3 (so some of the statements were not
>>   valid anymore)
>>
>
> Here is the clk_summary relative to ethernet on Odroid-C1+
> with this new series applied:
>
> xtal                                1            1    24000000          0 0
>  sys_pll                            0            0  1200000000          0 0
>   cpu_clk                           0            0  1200000000          0 0
>  vid_pll                            0            0   732000000          0 0
>  fixed_pll                          2            2  2550000000          0 0
>   mpll2                             1            1   249999701          0 0
>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>     c9410000.ethernet#m250_div      1            1   249999701          0 0
>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>       c9410000.ethernet#m25_en      1            1    24999970          0 0
>
> The ethernet prg0 register is set to 0x74A1 which should be correct with
> respect to the information contained in the S805 SoC manual.
> Actually, the ethernet is not yet fully functional.
> Trying to ping the board, I can see ARP request from host to board using
> tcpdump. However, the host can't see any response.
great - we're getting closer!

> Following the U-Boot value for prg0 register, which is 0x7d21, I also
> tried to set bit 11. As expected, this did not have any influence.
it *may* be something outside the PRG_ETH0 register than
to confirm that: could you temporarily revert the last patch from this
series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
should be identical to what u-boot sets (apart from bit 11, but that
is only relevant in RMII mode according to the datasheet)

> Another thing that we should check is the "Ethernet Memory PD" (see S805
> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> normal operation. However, those bits are already cleared by U-Boot.
if the peripheral registers itself are configured correctly it's
typically one of these issues:
- gate clock not being enabled (can you confirm that you hav the
"stmmaceth" with CLKID_ETH in the ethmac node?)
- incorrect pinmux settings (as a hack I would remove all ethernet
pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
before booting the mainline kernel you'll need to use Ethernet from
within u-boot once)
- incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
meson8b-odroidc1.dts, but the driver should auto-select that value if
it's missing)
- IP block being in some undefined state which can be brought back
into a working state by adding the reset line (RESET_ETHERNET)
- Ethernet PHY being in some undefined state  can be brought back into
a working state by adding the reset line (GPIOH_4, see
meson-gxbb-odroidc2.dts how to use that)
- I have not seen that the power-domains ("Ethernet Memory PD") were a
problem yet, but you already checked that

maybe you can share your current .dts patch and a boot-log so others
can have a look as well?

> Thank you for the support.
thank you for your patience as well, most people would have given up by now

> Best regards,
>
> Emiliano
>
>>
>> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>>
>>
>> Martin Blumenstingl (5):
>>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>>
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>>  1 file changed, 63 insertions(+), 56 deletions(-)
>>
>> --
>> 2.15.1
>>


Regards
Martin

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Emiliano,

On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> Hi Martin, Hi Dave,
>
> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>> Hi Dave,
>>
>> please do not apply this series until it got a Tested-by from Emiliano.
>>
>>
>> Hi Emiliano,
>>
>> you reported [0] that you couldn't get dwmac-meson8b to work on your
>> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>> I think I was able to find a fix: it consists of two patches (which you
>> find in this series)
>>
>> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>> only partially test this (I could only check if the clocks were
>> calculated correctly when using a dummy 500002394Hz input clock instead
>> of MPLL2).
>>
>> Could you please give this series a try and let me know about the
>> results?
>> You obviously still need your two "ARM: dts: meson8b" patches which
>> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>> - enable Ethernet on the Odroid-C1
>>
>> When testing on Meson8b this also needs a fix for the MPLL clock driver:
>> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>> https://patchwork.kernel.org/patch/10131677/
>>
>>
>> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>> fine (so let's hope that this also fixes your Meson8b issue :)).
>>
>>
>> changes since v1 at [1]:
>> - changed the subject of the cover-letter to indicate that this is all
>>   about the RGMII clock
>> - added PATCH #1 which ensures that we don't unnecessarily change the
>>   parent clocks in RMII mode (and also makes the code easier to
>>   understand)
>> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>>   is about the RGMII clock
>> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>>   on Meson8b correctly
>>
>> changes since v2 at [2]:
>> - added PATCH #2 to make the following patch easier
>> - Emiliano reported that there's currently another bug in the
>>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>>   been visible on GXBB and later due to the input clock which always led
>>   to a selection of "divide by 10" (which is done internally in the IP
>>   block, but the bit actually means "enable RGMII clock output").
>>   PATCH #3 was added to address this issue.
>> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>>   updated and the patch itself rebased because the m25_div clock was
>>   removed with the new PATCH #3 (so some of the statements were not
>>   valid anymore)
>>
>
> Here is the clk_summary relative to ethernet on Odroid-C1+
> with this new series applied:
>
> xtal                                1            1    24000000          0 0
>  sys_pll                            0            0  1200000000          0 0
>   cpu_clk                           0            0  1200000000          0 0
>  vid_pll                            0            0   732000000          0 0
>  fixed_pll                          2            2  2550000000          0 0
>   mpll2                             1            1   249999701          0 0
>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>     c9410000.ethernet#m250_div      1            1   249999701          0 0
>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>       c9410000.ethernet#m25_en      1            1    24999970          0 0
>
> The ethernet prg0 register is set to 0x74A1 which should be correct with
> respect to the information contained in the S805 SoC manual.
> Actually, the ethernet is not yet fully functional.
> Trying to ping the board, I can see ARP request from host to board using
> tcpdump. However, the host can't see any response.
great - we're getting closer!

> Following the U-Boot value for prg0 register, which is 0x7d21, I also
> tried to set bit 11. As expected, this did not have any influence.
it *may* be something outside the PRG_ETH0 register than
to confirm that: could you temporarily revert the last patch from this
series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
should be identical to what u-boot sets (apart from bit 11, but that
is only relevant in RMII mode according to the datasheet)

> Another thing that we should check is the "Ethernet Memory PD" (see S805
> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> normal operation. However, those bits are already cleared by U-Boot.
if the peripheral registers itself are configured correctly it's
typically one of these issues:
- gate clock not being enabled (can you confirm that you hav the
"stmmaceth" with CLKID_ETH in the ethmac node?)
- incorrect pinmux settings (as a hack I would remove all ethernet
pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
before booting the mainline kernel you'll need to use Ethernet from
within u-boot once)
- incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
meson8b-odroidc1.dts, but the driver should auto-select that value if
it's missing)
- IP block being in some undefined state which can be brought back
into a working state by adding the reset line (RESET_ETHERNET)
- Ethernet PHY being in some undefined state  can be brought back into
a working state by adding the reset line (GPIOH_4, see
meson-gxbb-odroidc2.dts how to use that)
- I have not seen that the power-domains ("Ethernet Memory PD") were a
problem yet, but you already checked that

maybe you can share your current .dts patch and a boot-log so others
can have a look as well?

> Thank you for the support.
thank you for your patience as well, most people would have given up by now

> Best regards,
>
> Emiliano
>
>>
>> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>>
>>
>> Martin Blumenstingl (5):
>>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>>
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>>  1 file changed, 63 insertions(+), 56 deletions(-)
>>
>> --
>> 2.15.1
>>


Regards
Martin

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

On Fri, Dec 29, 2017 at 8:48 AM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Emiliano,
>
> On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
>> Hi Martin, Hi Dave,
>>
>> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>>> Hi Dave,
>>>
>>> please do not apply this series until it got a Tested-by from Emiliano.
>>>
>>>
>>> Hi Emiliano,
>>>
>>> you reported [0] that you couldn't get dwmac-meson8b to work on your
>>> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>>> I think I was able to find a fix: it consists of two patches (which you
>>> find in this series)
>>>
>>> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>>> only partially test this (I could only check if the clocks were
>>> calculated correctly when using a dummy 500002394Hz input clock instead
>>> of MPLL2).
>>>
>>> Could you please give this series a try and let me know about the
>>> results?
>>> You obviously still need your two "ARM: dts: meson8b" patches which
>>> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>>> - enable Ethernet on the Odroid-C1
>>>
>>> When testing on Meson8b this also needs a fix for the MPLL clock driver:
>>> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>>> https://patchwork.kernel.org/patch/10131677/
>>>
>>>
>>> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>>> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>>> fine (so let's hope that this also fixes your Meson8b issue :)).
>>>
>>>
>>> changes since v1 at [1]:
>>> - changed the subject of the cover-letter to indicate that this is all
>>>   about the RGMII clock
>>> - added PATCH #1 which ensures that we don't unnecessarily change the
>>>   parent clocks in RMII mode (and also makes the code easier to
>>>   understand)
>>> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>>>   is about the RGMII clock
>>> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>>> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>>>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>>>   on Meson8b correctly
>>>
>>> changes since v2 at [2]:
>>> - added PATCH #2 to make the following patch easier
>>> - Emiliano reported that there's currently another bug in the
>>>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>>>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>>>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>>>   been visible on GXBB and later due to the input clock which always led
>>>   to a selection of "divide by 10" (which is done internally in the IP
>>>   block, but the bit actually means "enable RGMII clock output").
>>>   PATCH #3 was added to address this issue.
>>> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>>>   updated and the patch itself rebased because the m25_div clock was
>>>   removed with the new PATCH #3 (so some of the statements were not
>>>   valid anymore)
>>>
>>
>> Here is the clk_summary relative to ethernet on Odroid-C1+
>> with this new series applied:
>>
>> xtal                                1            1    24000000          0 0
>>  sys_pll                            0            0  1200000000          0 0
>>   cpu_clk                           0            0  1200000000          0 0
>>  vid_pll                            0            0   732000000          0 0
>>  fixed_pll                          2            2  2550000000          0 0
>>   mpll2                             1            1   249999701          0 0
>>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>>     c9410000.ethernet#m250_div      1            1   249999701          0 0
>>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>>       c9410000.ethernet#m25_en      1            1    24999970          0 0
>>
>> The ethernet prg0 register is set to 0x74A1 which should be correct with
>> respect to the information contained in the S805 SoC manual.
>> Actually, the ethernet is not yet fully functional.
>> Trying to ping the board, I can see ARP request from host to board using
>> tcpdump. However, the host can't see any response.
> great - we're getting closer!
>
>> Following the U-Boot value for prg0 register, which is 0x7d21, I also
>> tried to set bit 11. As expected, this did not have any influence.
> it *may* be something outside the PRG_ETH0 register than
> to confirm that: could you temporarily revert the last patch from this
> series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
> parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
> should be identical to what u-boot sets (apart from bit 11, but that
> is only relevant in RMII mode according to the datasheet)
>
>> Another thing that we should check is the "Ethernet Memory PD" (see S805
>> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
>> normal operation. However, those bits are already cleared by U-Boot.
> if the peripheral registers itself are configured correctly it's
> typically one of these issues:
> - gate clock not being enabled (can you confirm that you hav the
> "stmmaceth" with CLKID_ETH in the ethmac node?)
> - incorrect pinmux settings (as a hack I would remove all ethernet
> pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
> before booting the mainline kernel you'll need to use Ethernet from
> within u-boot once)
> - incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
> meson8b-odroidc1.dts, but the driver should auto-select that value if
> it's missing)
> - IP block being in some undefined state which can be brought back
> into a working state by adding the reset line (RESET_ETHERNET)
> - Ethernet PHY being in some undefined state  can be brought back into
> a working state by adding the reset line (GPIOH_4, see
> meson-gxbb-odroidc2.dts how to use that)
> - I have not seen that the power-domains ("Ethernet Memory PD") were a
> problem yet, but you already checked that
and I forgot the "eee-broken-1000t;" property on the PHY! have a look
at meson-gxbb-odroidc2.dts - I would assume that the ethmac node in
your meson8b-odroidc1.dts looks almost identical (no interrupt
configuration inside the PHY node, different reset-GPIO)

>
> maybe you can share your current .dts patch and a boot-log so others
> can have a look as well?
>
>> Thank you for the support.
> thank you for your patience as well, most people would have given up by now
>
>> Best regards,
>>
>> Emiliano
>>
>>>
>>> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>>> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>>> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>>>
>>>
>>> Martin Blumenstingl (5):
>>>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>>>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>>>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>>>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>>>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>>>
>>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>>>  1 file changed, 63 insertions(+), 56 deletions(-)
>>>
>>> --
>>> 2.15.1
>>>
>
>
> Regards
> Martin

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

On Fri, Dec 29, 2017 at 8:48 AM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Emiliano,
>
> On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
>> Hi Martin, Hi Dave,
>>
>> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>>> Hi Dave,
>>>
>>> please do not apply this series until it got a Tested-by from Emiliano.
>>>
>>>
>>> Hi Emiliano,
>>>
>>> you reported [0] that you couldn't get dwmac-meson8b to work on your
>>> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>>> I think I was able to find a fix: it consists of two patches (which you
>>> find in this series)
>>>
>>> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>>> only partially test this (I could only check if the clocks were
>>> calculated correctly when using a dummy 500002394Hz input clock instead
>>> of MPLL2).
>>>
>>> Could you please give this series a try and let me know about the
>>> results?
>>> You obviously still need your two "ARM: dts: meson8b" patches which
>>> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>>> - enable Ethernet on the Odroid-C1
>>>
>>> When testing on Meson8b this also needs a fix for the MPLL clock driver:
>>> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>>> https://patchwork.kernel.org/patch/10131677/
>>>
>>>
>>> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>>> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>>> fine (so let's hope that this also fixes your Meson8b issue :)).
>>>
>>>
>>> changes since v1 at [1]:
>>> - changed the subject of the cover-letter to indicate that this is all
>>>   about the RGMII clock
>>> - added PATCH #1 which ensures that we don't unnecessarily change the
>>>   parent clocks in RMII mode (and also makes the code easier to
>>>   understand)
>>> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>>>   is about the RGMII clock
>>> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>>> - replaced PATCH #3 (formerly PATCH #2) with one that sets
>>>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>>>   on Meson8b correctly
>>>
>>> changes since v2 at [2]:
>>> - added PATCH #2 to make the following patch easier
>>> - Emiliano reported that there's currently another bug in the
>>>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>>>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>>>   (instead of a divide by 5 or divide by 10 clock divider). This has not
>>>   been visible on GXBB and later due to the input clock which always led
>>>   to a selection of "divide by 10" (which is done internally in the IP
>>>   block, but the bit actually means "enable RGMII clock output").
>>>   PATCH #3 was added to address this issue.
>>> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>>>   updated and the patch itself rebased because the m25_div clock was
>>>   removed with the new PATCH #3 (so some of the statements were not
>>>   valid anymore)
>>>
>>
>> Here is the clk_summary relative to ethernet on Odroid-C1+
>> with this new series applied:
>>
>> xtal                                1            1    24000000          0 0
>>  sys_pll                            0            0  1200000000          0 0
>>   cpu_clk                           0            0  1200000000          0 0
>>  vid_pll                            0            0   732000000          0 0
>>  fixed_pll                          2            2  2550000000          0 0
>>   mpll2                             1            1   249999701          0 0
>>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>>     c9410000.ethernet#m250_div      1            1   249999701          0 0
>>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>>       c9410000.ethernet#m25_en      1            1    24999970          0 0
>>
>> The ethernet prg0 register is set to 0x74A1 which should be correct with
>> respect to the information contained in the S805 SoC manual.
>> Actually, the ethernet is not yet fully functional.
>> Trying to ping the board, I can see ARP request from host to board using
>> tcpdump. However, the host can't see any response.
> great - we're getting closer!
>
>> Following the U-Boot value for prg0 register, which is 0x7d21, I also
>> tried to set bit 11. As expected, this did not have any influence.
> it *may* be something outside the PRG_ETH0 register than
> to confirm that: could you temporarily revert the last patch from this
> series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
> parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
> should be identical to what u-boot sets (apart from bit 11, but that
> is only relevant in RMII mode according to the datasheet)
>
>> Another thing that we should check is the "Ethernet Memory PD" (see S805
>> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
>> normal operation. However, those bits are already cleared by U-Boot.
> if the peripheral registers itself are configured correctly it's
> typically one of these issues:
> - gate clock not being enabled (can you confirm that you hav the
> "stmmaceth" with CLKID_ETH in the ethmac node?)
> - incorrect pinmux settings (as a hack I would remove all ethernet
> pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
> before booting the mainline kernel you'll need to use Ethernet from
> within u-boot once)
> - incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
> meson8b-odroidc1.dts, but the driver should auto-select that value if
> it's missing)
> - IP block being in some undefined state which can be brought back
> into a working state by adding the reset line (RESET_ETHERNET)
> - Ethernet PHY being in some undefined state  can be brought back into
> a working state by adding the reset line (GPIOH_4, see
> meson-gxbb-odroidc2.dts how to use that)
> - I have not seen that the power-domains ("Ethernet Memory PD") were a
> problem yet, but you already checked that
and I forgot the "eee-broken-1000t;" property on the PHY! have a look
at meson-gxbb-odroidc2.dts - I would assume that the ethmac node in
your meson8b-odroidc1.dts looks almost identical (no interrupt
configuration inside the PHY node, different reset-GPIO)

>
> maybe you can share your current .dts patch and a boot-log so others
> can have a look as well?
>
>> Thank you for the support.
> thank you for your patience as well, most people would have given up by now
>
>> Best regards,
>>
>> Emiliano
>>
>>>
>>> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>>> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>>> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>>>
>>>
>>> Martin Blumenstingl (5):
>>>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>>>   net: stmmac: dwmac-meson8b: simplify generating the clock names
>>>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>>>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>>>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>>>
>>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>>>  1 file changed, 63 insertions(+), 56 deletions(-)
>>>
>>> --
>>> 2.15.1
>>>
>
>
> Regards
> Martin

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

[-- Attachment #1: Type: text/plain, Size: 8645 bytes --]

Hi Martin,

On Fri, Dec 29, 2017 at 08:48:54AM +0100, Martin Blumenstingl wrote:
> Hi Emiliano,
> 
> On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > Hi Martin, Hi Dave,
> >
> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> >> Hi Dave,
> >>
> >> please do not apply this series until it got a Tested-by from Emiliano.
> >>
> >>
> >> Hi Emiliano,
> >>
> >> you reported [0] that you couldn't get dwmac-meson8b to work on your
> >> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> >> I think I was able to find a fix: it consists of two patches (which you
> >> find in this series)
> >>
> >> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> >> only partially test this (I could only check if the clocks were
> >> calculated correctly when using a dummy 500002394Hz input clock instead
> >> of MPLL2).
> >>
> >> Could you please give this series a try and let me know about the
> >> results?
> >> You obviously still need your two "ARM: dts: meson8b" patches which
> >> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> >> - enable Ethernet on the Odroid-C1
> >>
> >> When testing on Meson8b this also needs a fix for the MPLL clock driver:
> >> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> >> https://patchwork.kernel.org/patch/10131677/
> >>
> >>
> >> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> >> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> >> fine (so let's hope that this also fixes your Meson8b issue :)).
> >>
> >>
> >> changes since v1 at [1]:
> >> - changed the subject of the cover-letter to indicate that this is all
> >>   about the RGMII clock
> >> - added PATCH #1 which ensures that we don't unnecessarily change the
> >>   parent clocks in RMII mode (and also makes the code easier to
> >>   understand)
> >> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> >>   is about the RGMII clock
> >> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> >> - replaced PATCH #3 (formerly PATCH #2) with one that sets
> >>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> >>   on Meson8b correctly
> >>
> >> changes since v2 at [2]:
> >> - added PATCH #2 to make the following patch easier
> >> - Emiliano reported that there's currently another bug in the
> >>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> >>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> >>   (instead of a divide by 5 or divide by 10 clock divider). This has not
> >>   been visible on GXBB and later due to the input clock which always led
> >>   to a selection of "divide by 10" (which is done internally in the IP
> >>   block, but the bit actually means "enable RGMII clock output").
> >>   PATCH #3 was added to address this issue.
> >> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> >>   updated and the patch itself rebased because the m25_div clock was
> >>   removed with the new PATCH #3 (so some of the statements were not
> >>   valid anymore)
> >>
> >
> > Here is the clk_summary relative to ethernet on Odroid-C1+
> > with this new series applied:
> >
> > xtal                                1            1    24000000          0 0
> >  sys_pll                            0            0  1200000000          0 0
> >   cpu_clk                           0            0  1200000000          0 0
> >  vid_pll                            0            0   732000000          0 0
> >  fixed_pll                          2            2  2550000000          0 0
> >   mpll2                             1            1   249999701          0 0
> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
> >     c9410000.ethernet#m250_div      1            1   249999701          0 0
> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
> >       c9410000.ethernet#m25_en      1            1    24999970          0 0
> >
> > The ethernet prg0 register is set to 0x74A1 which should be correct with
> > respect to the information contained in the S805 SoC manual.
> > Actually, the ethernet is not yet fully functional.
> > Trying to ping the board, I can see ARP request from host to board using
> > tcpdump. However, the host can't see any response.
> great - we're getting closer!
> 
> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
> > tried to set bit 11. As expected, this did not have any influence.
> it *may* be something outside the PRG_ETH0 register than
> to confirm that: could you temporarily revert the last patch from this
> series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
> parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
> should be identical to what u-boot sets (apart from bit 11, but that
> is only relevant in RMII mode according to the datasheet)
>

Here is the clk_summary after your suggestion:

xtal                                 1            1    24000000          0 0
 sys_pll                             0            0  1200000000          0 0
  cpu_clk                            0            0  1200000000          0 0
 vid_pll                             0            0   732000000          0 0
 fixed_pll                           2            2  2550000000          0 0
  mpll2                              1            1   500002394          0 0
   c9410000.ethernet#m250_sel        1            1   500002394          0 0
    c9410000.ethernet#m250_div	     1            1   166667465          0 0
     c9410000.ethernet#fixed_div10   1            1    16666746          0 0
      c9410000.ethernet#m25_en       1            1    16666746          0 0

which seems worse. Pinging the board, I still see ARP request/reply via
tcpdump. The host, however, can't see any.

> > Another thing that we should check is the "Ethernet Memory PD" (see S805
> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> > normal operation. However, those bits are already cleared by U-Boot.
> if the peripheral registers itself are configured correctly it's
> typically one of these issues:
> - gate clock not being enabled (can you confirm that you hav the
> "stmmaceth" with CLKID_ETH in the ethmac node?)

- yes, I have;

> - incorrect pinmux settings (as a hack I would remove all ethernet
> pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
> before booting the mainline kernel you'll need to use Ethernet from
> within u-boot once)

- tried; nothing changed;

> - incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
> meson8b-odroidc1.dts, but the driver should auto-select that value if
> it's missing)

- defined as in odroid-c2 dts;

> - IP block being in some undefined state which can be brought back
> into a working state by adding the reset line (RESET_ETHERNET)

- I have it;

> - Ethernet PHY being in some undefined state  can be brought back into
> a working state by adding the reset line (GPIOH_4, see
> meson-gxbb-odroidc2.dts how to use that)

- have it.

> - I have not seen that the power-domains ("Ethernet Memory PD") were a
> problem yet, but you already checked that
> 
> maybe you can share your current .dts patch and a boot-log so others
> can have a look as well?
>

OK, no problem.

> > Thank you for the support.
> thank you for your patience as well, most people would have given up by now
> 
> > Best regards,
> >
> > Emiliano
> >
> >>
> >> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> >> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> >> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> >>
> >>
> >> Martin Blumenstingl (5):
> >>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> >>   net: stmmac: dwmac-meson8b: simplify generating the clock names
> >>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> >>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> >>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> >>
> >>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> >>  1 file changed, 63 insertions(+), 56 deletions(-)
> >>
> >> --
> >> 2.15.1
> >>
> 
> 
> Regards
> Martin
> 

Regards,

Emiliano

> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

[-- Attachment #2: meson8b.dtsi --]
[-- Type: text/plain, Size: 7728 bytes --]

/*
 * Copyright 2015 Endless Mobile, Inc.
 * Author: Carlo Caione <carlo@endlessm.com>
 *
 * This file is dual-licensed: you can use it either under the terms
 * of the GPL or the X11 license, at your option. Note that this dual
 * licensing only applies to this file, and not this project as a
 * whole.
 *
 *  a) This library is free software; you can redistribute it and/or
 *     modify it under the terms of the GNU General Public License as
 *     published by the Free Software Foundation; either version 2 of the
 *     License, or (at your option) any later version.
 *
 *     This library is distributed in the hope that it will be useful,
 *     but WITHOUT ANY WARRANTY; without even the implied warranty of
 *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *     GNU General Public License for more details.
 *
 *     You should have received a copy of the GNU General Public License
 *     along with this program. If not, see <http://www.gnu.org/licenses/>.
 *
 * Or, alternatively,
 *
 *  b) Permission is hereby granted, free of charge, to any person
 *     obtaining a copy of this software and associated documentation
 *     files (the "Software"), to deal in the Software without
 *     restriction, including without limitation the rights to use,
 *     copy, modify, merge, publish, distribute, sublicense, and/or
 *     sell copies of the Software, and to permit persons to whom the
 *     Software is furnished to do so, subject to the following
 *     conditions:
 *
 *     The above copyright notice and this permission notice shall be
 *     included in all copies or substantial portions of the Software.
 *
 *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
 *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
 *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
 *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 *     OTHER DEALINGS IN THE SOFTWARE.
 */

#include <dt-bindings/clock/meson8b-clkc.h>
#include <dt-bindings/gpio/meson8b-gpio.h>
#include <dt-bindings/reset/amlogic,meson8b-reset.h>
#include <dt-bindings/reset/amlogic,meson8b-clkc-reset.h>
#include "meson.dtsi"

/ {
	cpus {
		#address-cells = <1>;
		#size-cells = <0>;

		cpu@200 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x200>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU0_SOFT_RESET>;
		};

		cpu@201 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x201>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU1_SOFT_RESET>;
		};

		cpu@202 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x202>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU2_SOFT_RESET>;
		};

		cpu@203 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x203>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU3_SOFT_RESET>;
		};
	};

	reserved-memory {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		/* 2 MiB reserved for Hardware ROM Firmware? */
		hwrom@0 {
			reg = <0x0 0x200000>;
			no-map;
		};
	};

	scu@c4300000 {
		compatible = "arm,cortex-a5-scu";
		reg = <0xc4300000 0x100>;
	};
}; /* end of / */

&aobus {
	pmu: pmu@e0 {
		compatible = "amlogic,meson8b-pmu", "syscon";
		reg = <0xe0 0x18>;
	};

	pinctrl_aobus: pinctrl@84 {
		compatible = "amlogic,meson8b-aobus-pinctrl";
		reg = <0x84 0xc>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		gpio_ao: ao-bank@14 {
			reg = <0x14 0x4>,
				<0x2c 0x4>,
				<0x24 0x8>;
			reg-names = "mux", "pull", "gpio";
			gpio-controller;
			#gpio-cells = <2>;
			gpio-ranges = <&pinctrl_aobus 0 0 16>;
		};

		uart_ao_a_pins: uart_ao_a {
			mux {
				groups = "uart_tx_ao_a", "uart_rx_ao_a";
				function = "uart_ao";
			};
		};
	};
};

&cbus {
	clkc: clock-controller@4000 {
		#clock-cells = <1>;
		#reset-cells = <1>;
		compatible = "amlogic,meson8b-clkc";
		reg = <0x8000 0x4>, <0x4000 0x460>;
	};

	reset: reset-controller@4404 {
		compatible = "amlogic,meson8b-reset";
		reg = <0x4404 0x20>;
		#reset-cells = <1>;
	};

	analog_top: analog-top@81a8 {
		compatible = "amlogic,meson8b-analog-top", "syscon";
		reg = <0x81a8 0x14>;
	};

	pwm_ef: pwm@86c0 {
		compatible = "amlogic,meson8b-pwm";
		reg = <0x86c0 0x10>;
		#pwm-cells = <3>;
		status = "disabled";
	};

	pinctrl_cbus: pinctrl@9880 {
		compatible = "amlogic,meson8b-cbus-pinctrl";
		reg = <0x9880 0x10>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		gpio: banks@80b0 {
			reg = <0x80b0 0x28>,
				<0x80e8 0x18>,
				<0x8120 0x18>,
				<0x8030 0x38>;
			reg-names = "mux", "pull", "pull-enable", "gpio";
			gpio-controller;
			#gpio-cells = <2>;
			gpio-ranges = <&pinctrl_cbus 0 0 130>;
		};

		eth_rgmii_pins: eth-rgmii {
			mux {
				groups = "eth_tx_clk",
					 "eth_tx_en",
					 "eth_txd1_0",
					 "eth_txd1_1",
					 "eth_txd0_0",
					 "eth_txd0_1",
					 "eth_rx_clk",
					 "eth_rx_dv",
					 "eth_rxd1",
					 "eth_rxd0",
					 "eth_mdio_en",
					 "eth_mdc",
					 "eth_ref_clk",
					 "eth_txd2",
					 "eth_txd3";
				function = "ethernet";
			};
		};
	};
};

&ahb_sram {
	smp-sram@1ff80 {
		compatible = "amlogic,meson8b-smp-sram";
		reg = <0x1ff80 0x8>;
	};
};


&efuse {
	compatible = "amlogic,meson8b-efuse";
	clocks = <&clkc CLKID_EFUSE>;
	clock-names = "core";
};

&ethmac {
	compatible = "amlogic,meson8b-dwmac", "snps,dwmac-3.70a", "snps,dwmac";

	reg = <0xc9410000 0x10000
	       0xc1108140 0x4>;

	clocks = <&clkc CLKID_ETH>,
		 <&clkc CLKID_MPLL2>,
		 <&clkc CLKID_MPLL2>;
	clock-names = "stmmaceth", "clkin0", "clkin1";

	resets = <&reset RESET_ETHERNET>;
	reset-names = "stmmaceth";
};

&gpio_intc {
	compatible = "amlogic,meson-gpio-intc",
		     "amlogic,meson8b-gpio-intc";
	status = "okay";
};

&hwrng {
	compatible = "amlogic,meson8b-rng", "amlogic,meson-rng";
	clocks = <&clkc CLKID_RNG0>;
	clock-names = "core";
};

&L2 {
	arm,data-latency = <3 3 3>;
	arm,tag-latency = <2 2 2>;
	arm,filter-ranges = <0x100000 0xc0000000>;
};

&pwm_ab {
	compatible = "amlogic,meson8b-pwm";
};

&pwm_cd {
	compatible = "amlogic,meson8b-pwm";
};

&saradc {
	compatible = "amlogic,meson8b-saradc", "amlogic,meson-saradc";
	clocks = <&clkc CLKID_XTAL>,
		<&clkc CLKID_SAR_ADC>,
		<&clkc CLKID_SANA>;
	clock-names = "clkin", "core", "sana";
};

&sdio {
	compatible = "amlogic,meson8b-sdio", "amlogic,meson-mx-sdio";
	clocks = <&clkc CLKID_SDIO>, <&clkc CLKID_CLK81>;
	clock-names = "core", "clkin";
};

&uart_AO {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_A {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_B {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_C {
	clocks = <&clkc CLKID_CLK81>;
};

&usb0 {
	compatible = "amlogic,meson8b-usb", "snps,dwc2";
	clocks = <&clkc CLKID_USB0_DDR_BRIDGE>;
	clock-names = "otg";
};

&usb1 {
	compatible = "amlogic,meson8b-usb", "snps,dwc2";
	clocks = <&clkc CLKID_USB1_DDR_BRIDGE>;
	clock-names = "otg";
};

&usb0_phy {
	compatible = "amlogic,meson8b-usb2-phy", "amlogic,meson-mx-usb2-phy";
	clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB0>;
	clock-names = "usb_general", "usb";
	resets = <&reset RESET_USB_OTG>;
};

&usb1_phy {
	compatible = "amlogic,meson8b-usb2-phy", "amlogic,meson-mx-usb2-phy";
	clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB1>;
	clock-names = "usb_general", "usb";
	resets = <&reset RESET_USB_OTG>;
};

&wdt {
	compatible = "amlogic,meson8b-wdt";
};

[-- Attachment #3: meson8b-odroidc1.dts --]
[-- Type: text/plain, Size: 3850 bytes --]

/*
 * Copyright 2015 Endless Mobile, Inc.
 * Author: Carlo Caione <carlo@endlessm.com>
 *
 * This file is dual-licensed: you can use it either under the terms
 * of the GPL or the X11 license, at your option. Note that this dual
 * licensing only applies to this file, and not this project as a
 * whole.
 *
 *  a) This library is free software; you can redistribute it and/or
 *     modify it under the terms of the GNU General Public License as
 *     published by the Free Software Foundation; either version 2 of the
 *     License, or (at your option) any later version.
 *
 *     This library is distributed in the hope that it will be useful,
 *     but WITHOUT ANY WARRANTY; without even the implied warranty of
 *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *     GNU General Public License for more details.
 *
 *     You should have received a copy of the GNU General Public License
 *     along with this program. If not, see <http://www.gnu.org/licenses/>.
 *
 * Or, alternatively,
 *
 *  b) Permission is hereby granted, free of charge, to any person
 *     obtaining a copy of this software and associated documentation
 *     files (the "Software"), to deal in the Software without
 *     restriction, including without limitation the rights to use,
 *     copy, modify, merge, publish, distribute, sublicense, and/or
 *     sell copies of the Software, and to permit persons to whom the
 *     Software is furnished to do so, subject to the following
 *     conditions:
 *
 *     The above copyright notice and this permission notice shall be
 *     included in all copies or substantial portions of the Software.
 *
 *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
 *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
 *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
 *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 *     OTHER DEALINGS IN THE SOFTWARE.
 */

/dts-v1/;
#include "meson8b.dtsi"
#include <dt-bindings/gpio/gpio.h>

/ {
	model = "Hardkernel ODROID-C1";
	compatible = "hardkernel,odroid-c1", "amlogic,meson8b";

	aliases {
		serial0 = &uart_AO;
	};

	memory {
		reg = <0x40000000 0x40000000>;
	};

	leds {
		compatible = "gpio-leds";
		blue {
			label = "c1:blue:alive";
			gpios = <&gpio_ao GPIOAO_13 GPIO_ACTIVE_LOW>;
			linux,default-trigger = "heartbeat";
			default-state = "off";
		};
	};
};

&uart_AO {
	status = "okay";
	pinctrl-0 = <&uart_ao_a_pins>;
	pinctrl-names = "default";
};

&gpio_ao {
	/*
	 * WARNING: The USB Hub on the Odroid-C1/C1+ needs a reset signal
	 * to be turned high in order to be detected by the USB Controller.
	 * This signal should be handled by a USB specific power sequence
	 * in order to reset the Hub when USB bus is powered down.
	 */
	usb-hub {
		gpio-hog;
		gpios = <GPIOAO_4 GPIO_ACTIVE_HIGH>;
		output-high;
		line-name = "usb-hub-reset";
	};
};

&usb1_phy {
	status = "okay";
};

&usb1 {
	status = "okay";
};

&ethmac {
	status = "okay";

	snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>;
	snps,reset-active-low;
	snps,reset-delays-us = <0 10000 30000>;

	pinctrl-0 = <&eth_rgmii_pins>;
	pinctrl-names = "default";

	phy-mode = "rgmii";
	phy-handle = <&eth_phy>;
	amlogic,tx-delay-ns = <2>;

	mdio {
		compatible = "snps,dwmac-mdio";
		#address-cells = <1>;
		#size-cells = <0>;

		eth_phy: ethernet-phy@0 {
			/* ethernet PHY RTL8211F */
			compatible = "ethernet-phy-id001c.c916",
				     "ethernet-phy-ieee802.3-c22";
			reg = <0>;
			max-speed = <1000>;
			eee-broken-1000t;
			interrupt-parent = <&gpio_intc>;
			interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
		};
	};
};

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

Hi Martin,

On Fri, Dec 29, 2017 at 08:48:54AM +0100, Martin Blumenstingl wrote:
> Hi Emiliano,
> 
> On Fri, Dec 29, 2017 at 2:31 AM, Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > Hi Martin, Hi Dave,
> >
> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> >> Hi Dave,
> >>
> >> please do not apply this series until it got a Tested-by from Emiliano.
> >>
> >>
> >> Hi Emiliano,
> >>
> >> you reported [0] that you couldn't get dwmac-meson8b to work on your
> >> Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> >> I think I was able to find a fix: it consists of two patches (which you
> >> find in this series)
> >>
> >> Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> >> only partially test this (I could only check if the clocks were
> >> calculated correctly when using a dummy 500002394Hz input clock instead
> >> of MPLL2).
> >>
> >> Could you please give this series a try and let me know about the
> >> results?
> >> You obviously still need your two "ARM: dts: meson8b" patches which
> >> - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> >> - enable Ethernet on the Odroid-C1
> >>
> >> When testing on Meson8b this also needs a fix for the MPLL clock driver:
> >> "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> >> https://patchwork.kernel.org/patch/10131677/
> >>
> >>
> >> I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> >> and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> >> fine (so let's hope that this also fixes your Meson8b issue :)).
> >>
> >>
> >> changes since v1 at [1]:
> >> - changed the subject of the cover-letter to indicate that this is all
> >>   about the RGMII clock
> >> - added PATCH #1 which ensures that we don't unnecessarily change the
> >>   parent clocks in RMII mode (and also makes the code easier to
> >>   understand)
> >> - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> >>   is about the RGMII clock
> >> - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> >> - replaced PATCH #3 (formerly PATCH #2) with one that sets
> >>   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> >>   on Meson8b correctly
> >>
> >> changes since v2 at [2]:
> >> - added PATCH #2 to make the following patch easier
> >> - Emiliano reported that there's currently another bug in the
> >>   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> >>   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> >>   (instead of a divide by 5 or divide by 10 clock divider). This has not
> >>   been visible on GXBB and later due to the input clock which always led
> >>   to a selection of "divide by 10" (which is done internally in the IP
> >>   block, but the bit actually means "enable RGMII clock output").
> >>   PATCH #3 was added to address this issue.
> >> - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> >>   updated and the patch itself rebased because the m25_div clock was
> >>   removed with the new PATCH #3 (so some of the statements were not
> >>   valid anymore)
> >>
> >
> > Here is the clk_summary relative to ethernet on Odroid-C1+
> > with this new series applied:
> >
> > xtal                                1            1    24000000          0 0
> >  sys_pll                            0            0  1200000000          0 0
> >   cpu_clk                           0            0  1200000000          0 0
> >  vid_pll                            0            0   732000000          0 0
> >  fixed_pll                          2            2  2550000000          0 0
> >   mpll2                             1            1   249999701          0 0
> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
> >     c9410000.ethernet#m250_div      1            1   249999701          0 0
> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
> >       c9410000.ethernet#m25_en      1            1    24999970          0 0
> >
> > The ethernet prg0 register is set to 0x74A1 which should be correct with
> > respect to the information contained in the S805 SoC manual.
> > Actually, the ethernet is not yet fully functional.
> > Trying to ping the board, I can see ARP request from host to board using
> > tcpdump. However, the host can't see any response.
> great - we're getting closer!
> 
> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
> > tried to set bit 11. As expected, this did not have any influence.
> it *may* be something outside the PRG_ETH0 register than
> to confirm that: could you temporarily revert the last patch from this
> series ("net: stmmac: dwmac-meson8b: propagate rate changes to the
> parent clock")? this way MPLL2 will stay at ~500MHz and PRG_ETH0
> should be identical to what u-boot sets (apart from bit 11, but that
> is only relevant in RMII mode according to the datasheet)
>

Here is the clk_summary after your suggestion:

xtal                                 1            1    24000000          0 0
 sys_pll                             0            0  1200000000          0 0
  cpu_clk                            0            0  1200000000          0 0
 vid_pll                             0            0   732000000          0 0
 fixed_pll                           2            2  2550000000          0 0
  mpll2                              1            1   500002394          0 0
   c9410000.ethernet#m250_sel        1            1   500002394          0 0
    c9410000.ethernet#m250_div	     1            1   166667465          0 0
     c9410000.ethernet#fixed_div10   1            1    16666746          0 0
      c9410000.ethernet#m25_en       1            1    16666746          0 0

which seems worse. Pinging the board, I still see ARP request/reply via
tcpdump. The host, however, can't see any.

> > Another thing that we should check is the "Ethernet Memory PD" (see S805
> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> > normal operation. However, those bits are already cleared by U-Boot.
> if the peripheral registers itself are configured correctly it's
> typically one of these issues:
> - gate clock not being enabled (can you confirm that you hav the
> "stmmaceth" with CLKID_ETH in the ethmac node?)

- yes, I have;

> - incorrect pinmux settings (as a hack I would remove all ethernet
> pinctrl properties/nodes from meson8b.dtsi and meson8b-odroidc1.dts.
> before booting the mainline kernel you'll need to use Ethernet from
> within u-boot once)

- tried; nothing changed;

> - incorrect TX delay (amlogic,tx-delay-ns = <2> should be defined in
> meson8b-odroidc1.dts, but the driver should auto-select that value if
> it's missing)

- defined as in odroid-c2 dts;

> - IP block being in some undefined state which can be brought back
> into a working state by adding the reset line (RESET_ETHERNET)

- I have it;

> - Ethernet PHY being in some undefined state  can be brought back into
> a working state by adding the reset line (GPIOH_4, see
> meson-gxbb-odroidc2.dts how to use that)

- have it.

> - I have not seen that the power-domains ("Ethernet Memory PD") were a
> problem yet, but you already checked that
> 
> maybe you can share your current .dts patch and a boot-log so others
> can have a look as well?
>

OK, no problem.

> > Thank you for the support.
> thank you for your patience as well, most people would have given up by now
> 
> > Best regards,
> >
> > Emiliano
> >
> >>
> >> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> >> [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> >> [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> >>
> >>
> >> Martin Blumenstingl (5):
> >>   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> >>   net: stmmac: dwmac-meson8b: simplify generating the clock names
> >>   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> >>   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> >>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> >>
> >>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> >>  1 file changed, 63 insertions(+), 56 deletions(-)
> >>
> >> --
> >> 2.15.1
> >>
> 
> 
> Regards
> Martin
> 

Regards,

Emiliano

> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
-------------- next part --------------
/*
 * Copyright 2015 Endless Mobile, Inc.
 * Author: Carlo Caione <carlo@endlessm.com>
 *
 * This file is dual-licensed: you can use it either under the terms
 * of the GPL or the X11 license, at your option. Note that this dual
 * licensing only applies to this file, and not this project as a
 * whole.
 *
 *  a) This library is free software; you can redistribute it and/or
 *     modify it under the terms of the GNU General Public License as
 *     published by the Free Software Foundation; either version 2 of the
 *     License, or (at your option) any later version.
 *
 *     This library is distributed in the hope that it will be useful,
 *     but WITHOUT ANY WARRANTY; without even the implied warranty of
 *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *     GNU General Public License for more details.
 *
 *     You should have received a copy of the GNU General Public License
 *     along with this program. If not, see <http://www.gnu.org/licenses/>.
 *
 * Or, alternatively,
 *
 *  b) Permission is hereby granted, free of charge, to any person
 *     obtaining a copy of this software and associated documentation
 *     files (the "Software"), to deal in the Software without
 *     restriction, including without limitation the rights to use,
 *     copy, modify, merge, publish, distribute, sublicense, and/or
 *     sell copies of the Software, and to permit persons to whom the
 *     Software is furnished to do so, subject to the following
 *     conditions:
 *
 *     The above copyright notice and this permission notice shall be
 *     included in all copies or substantial portions of the Software.
 *
 *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
 *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
 *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
 *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 *     OTHER DEALINGS IN THE SOFTWARE.
 */

#include <dt-bindings/clock/meson8b-clkc.h>
#include <dt-bindings/gpio/meson8b-gpio.h>
#include <dt-bindings/reset/amlogic,meson8b-reset.h>
#include <dt-bindings/reset/amlogic,meson8b-clkc-reset.h>
#include "meson.dtsi"

/ {
	cpus {
		#address-cells = <1>;
		#size-cells = <0>;

		cpu at 200 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x200>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU0_SOFT_RESET>;
		};

		cpu at 201 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x201>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU1_SOFT_RESET>;
		};

		cpu at 202 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x202>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU2_SOFT_RESET>;
		};

		cpu at 203 {
			device_type = "cpu";
			compatible = "arm,cortex-a5";
			next-level-cache = <&L2>;
			reg = <0x203>;
			enable-method = "amlogic,meson8b-smp";
			resets = <&clkc CLKC_RESET_CPU3_SOFT_RESET>;
		};
	};

	reserved-memory {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		/* 2 MiB reserved for Hardware ROM Firmware? */
		hwrom at 0 {
			reg = <0x0 0x200000>;
			no-map;
		};
	};

	scu at c4300000 {
		compatible = "arm,cortex-a5-scu";
		reg = <0xc4300000 0x100>;
	};
}; /* end of / */

&aobus {
	pmu: pmu at e0 {
		compatible = "amlogic,meson8b-pmu", "syscon";
		reg = <0xe0 0x18>;
	};

	pinctrl_aobus: pinctrl at 84 {
		compatible = "amlogic,meson8b-aobus-pinctrl";
		reg = <0x84 0xc>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		gpio_ao: ao-bank at 14 {
			reg = <0x14 0x4>,
				<0x2c 0x4>,
				<0x24 0x8>;
			reg-names = "mux", "pull", "gpio";
			gpio-controller;
			#gpio-cells = <2>;
			gpio-ranges = <&pinctrl_aobus 0 0 16>;
		};

		uart_ao_a_pins: uart_ao_a {
			mux {
				groups = "uart_tx_ao_a", "uart_rx_ao_a";
				function = "uart_ao";
			};
		};
	};
};

&cbus {
	clkc: clock-controller at 4000 {
		#clock-cells = <1>;
		#reset-cells = <1>;
		compatible = "amlogic,meson8b-clkc";
		reg = <0x8000 0x4>, <0x4000 0x460>;
	};

	reset: reset-controller at 4404 {
		compatible = "amlogic,meson8b-reset";
		reg = <0x4404 0x20>;
		#reset-cells = <1>;
	};

	analog_top: analog-top at 81a8 {
		compatible = "amlogic,meson8b-analog-top", "syscon";
		reg = <0x81a8 0x14>;
	};

	pwm_ef: pwm at 86c0 {
		compatible = "amlogic,meson8b-pwm";
		reg = <0x86c0 0x10>;
		#pwm-cells = <3>;
		status = "disabled";
	};

	pinctrl_cbus: pinctrl at 9880 {
		compatible = "amlogic,meson8b-cbus-pinctrl";
		reg = <0x9880 0x10>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		gpio: banks at 80b0 {
			reg = <0x80b0 0x28>,
				<0x80e8 0x18>,
				<0x8120 0x18>,
				<0x8030 0x38>;
			reg-names = "mux", "pull", "pull-enable", "gpio";
			gpio-controller;
			#gpio-cells = <2>;
			gpio-ranges = <&pinctrl_cbus 0 0 130>;
		};

		eth_rgmii_pins: eth-rgmii {
			mux {
				groups = "eth_tx_clk",
					 "eth_tx_en",
					 "eth_txd1_0",
					 "eth_txd1_1",
					 "eth_txd0_0",
					 "eth_txd0_1",
					 "eth_rx_clk",
					 "eth_rx_dv",
					 "eth_rxd1",
					 "eth_rxd0",
					 "eth_mdio_en",
					 "eth_mdc",
					 "eth_ref_clk",
					 "eth_txd2",
					 "eth_txd3";
				function = "ethernet";
			};
		};
	};
};

&ahb_sram {
	smp-sram at 1ff80 {
		compatible = "amlogic,meson8b-smp-sram";
		reg = <0x1ff80 0x8>;
	};
};


&efuse {
	compatible = "amlogic,meson8b-efuse";
	clocks = <&clkc CLKID_EFUSE>;
	clock-names = "core";
};

&ethmac {
	compatible = "amlogic,meson8b-dwmac", "snps,dwmac-3.70a", "snps,dwmac";

	reg = <0xc9410000 0x10000
	       0xc1108140 0x4>;

	clocks = <&clkc CLKID_ETH>,
		 <&clkc CLKID_MPLL2>,
		 <&clkc CLKID_MPLL2>;
	clock-names = "stmmaceth", "clkin0", "clkin1";

	resets = <&reset RESET_ETHERNET>;
	reset-names = "stmmaceth";
};

&gpio_intc {
	compatible = "amlogic,meson-gpio-intc",
		     "amlogic,meson8b-gpio-intc";
	status = "okay";
};

&hwrng {
	compatible = "amlogic,meson8b-rng", "amlogic,meson-rng";
	clocks = <&clkc CLKID_RNG0>;
	clock-names = "core";
};

&L2 {
	arm,data-latency = <3 3 3>;
	arm,tag-latency = <2 2 2>;
	arm,filter-ranges = <0x100000 0xc0000000>;
};

&pwm_ab {
	compatible = "amlogic,meson8b-pwm";
};

&pwm_cd {
	compatible = "amlogic,meson8b-pwm";
};

&saradc {
	compatible = "amlogic,meson8b-saradc", "amlogic,meson-saradc";
	clocks = <&clkc CLKID_XTAL>,
		<&clkc CLKID_SAR_ADC>,
		<&clkc CLKID_SANA>;
	clock-names = "clkin", "core", "sana";
};

&sdio {
	compatible = "amlogic,meson8b-sdio", "amlogic,meson-mx-sdio";
	clocks = <&clkc CLKID_SDIO>, <&clkc CLKID_CLK81>;
	clock-names = "core", "clkin";
};

&uart_AO {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_A {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_B {
	clocks = <&clkc CLKID_CLK81>;
};

&uart_C {
	clocks = <&clkc CLKID_CLK81>;
};

&usb0 {
	compatible = "amlogic,meson8b-usb", "snps,dwc2";
	clocks = <&clkc CLKID_USB0_DDR_BRIDGE>;
	clock-names = "otg";
};

&usb1 {
	compatible = "amlogic,meson8b-usb", "snps,dwc2";
	clocks = <&clkc CLKID_USB1_DDR_BRIDGE>;
	clock-names = "otg";
};

&usb0_phy {
	compatible = "amlogic,meson8b-usb2-phy", "amlogic,meson-mx-usb2-phy";
	clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB0>;
	clock-names = "usb_general", "usb";
	resets = <&reset RESET_USB_OTG>;
};

&usb1_phy {
	compatible = "amlogic,meson8b-usb2-phy", "amlogic,meson-mx-usb2-phy";
	clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB1>;
	clock-names = "usb_general", "usb";
	resets = <&reset RESET_USB_OTG>;
};

&wdt {
	compatible = "amlogic,meson8b-wdt";
};
-------------- next part --------------
/*
 * Copyright 2015 Endless Mobile, Inc.
 * Author: Carlo Caione <carlo@endlessm.com>
 *
 * This file is dual-licensed: you can use it either under the terms
 * of the GPL or the X11 license, at your option. Note that this dual
 * licensing only applies to this file, and not this project as a
 * whole.
 *
 *  a) This library is free software; you can redistribute it and/or
 *     modify it under the terms of the GNU General Public License as
 *     published by the Free Software Foundation; either version 2 of the
 *     License, or (at your option) any later version.
 *
 *     This library is distributed in the hope that it will be useful,
 *     but WITHOUT ANY WARRANTY; without even the implied warranty of
 *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *     GNU General Public License for more details.
 *
 *     You should have received a copy of the GNU General Public License
 *     along with this program. If not, see <http://www.gnu.org/licenses/>.
 *
 * Or, alternatively,
 *
 *  b) Permission is hereby granted, free of charge, to any person
 *     obtaining a copy of this software and associated documentation
 *     files (the "Software"), to deal in the Software without
 *     restriction, including without limitation the rights to use,
 *     copy, modify, merge, publish, distribute, sublicense, and/or
 *     sell copies of the Software, and to permit persons to whom the
 *     Software is furnished to do so, subject to the following
 *     conditions:
 *
 *     The above copyright notice and this permission notice shall be
 *     included in all copies or substantial portions of the Software.
 *
 *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
 *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
 *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
 *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 *     OTHER DEALINGS IN THE SOFTWARE.
 */

/dts-v1/;
#include "meson8b.dtsi"
#include <dt-bindings/gpio/gpio.h>

/ {
	model = "Hardkernel ODROID-C1";
	compatible = "hardkernel,odroid-c1", "amlogic,meson8b";

	aliases {
		serial0 = &uart_AO;
	};

	memory {
		reg = <0x40000000 0x40000000>;
	};

	leds {
		compatible = "gpio-leds";
		blue {
			label = "c1:blue:alive";
			gpios = <&gpio_ao GPIOAO_13 GPIO_ACTIVE_LOW>;
			linux,default-trigger = "heartbeat";
			default-state = "off";
		};
	};
};

&uart_AO {
	status = "okay";
	pinctrl-0 = <&uart_ao_a_pins>;
	pinctrl-names = "default";
};

&gpio_ao {
	/*
	 * WARNING: The USB Hub on the Odroid-C1/C1+ needs a reset signal
	 * to be turned high in order to be detected by the USB Controller.
	 * This signal should be handled by a USB specific power sequence
	 * in order to reset the Hub when USB bus is powered down.
	 */
	usb-hub {
		gpio-hog;
		gpios = <GPIOAO_4 GPIO_ACTIVE_HIGH>;
		output-high;
		line-name = "usb-hub-reset";
	};
};

&usb1_phy {
	status = "okay";
};

&usb1 {
	status = "okay";
};

&ethmac {
	status = "okay";

	snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>;
	snps,reset-active-low;
	snps,reset-delays-us = <0 10000 30000>;

	pinctrl-0 = <&eth_rgmii_pins>;
	pinctrl-names = "default";

	phy-mode = "rgmii";
	phy-handle = <&eth_phy>;
	amlogic,tx-delay-ns = <2>;

	mdio {
		compatible = "snps,dwmac-mdio";
		#address-cells = <1>;
		#size-cells = <0>;

		eth_phy: ethernet-phy at 0 {
			/* ethernet PHY RTL8211F */
			compatible = "ethernet-phy-id001c.c916",
				     "ethernet-phy-ieee802.3-c22";
			reg = <0>;
			max-speed = <1000>;
			eee-broken-1000t;
			interrupt-parent = <&gpio_intc>;
			interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
		};
	};
};

Re: [RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Jerome Brunet]

On Thu, 2017-12-28 at 23:21 +0100, Martin Blumenstingl wrote:
> While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
> discovered that the m25_div is not actually a divider but rather a gate.
> This matches with the datasheet which describes bit 10 as "Generate
> 25MHz clock for PHY". Back when the driver was written it was assumed
> that this was a divider (which could divide by 5 or 10) because other
> clock bits in the datasheet were documented incorrectly.

Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
RGMII, before being divided by 10 to produce the 25MHz of div25

IOW, maybe we need this intermediate rate.
It would not be surprising, 1GBps usually requires a 125MHz clock somewhere. 

This is still doable:
* Keep the divider
* drop CLK_SET_RATE_PARENT on div25
* call set_rate on div250 with 250MHz then on div25 with 25Mhz


> 
> Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
> Odroid-C1 (using a Meson8b SoC) does not work.
> On GXBB and newer SoCs (where the driver was initially tested with RGMII
> PHYs) this is not a problem because the input clock is running at 1GHz.
> The m250_div clock's biggest possible divider is 7 (3-bit divider, with
> value 0 being reserved). Thus we end up with a m250_div of 4 and a
> "m25_div" of 10 (= register value 1).
> 
> Instead it turns out that the Ethernet IP block seems to have a fixed
> "divide by 10" clock internally. This means that bit 10 is a gate clock
> which enables the RGMII clock output.
> 
> This replaces the "m25_div" clock with a clock gate called "m25_en"
> which ensures that we can set this bit to 1 whenever we enable RGMII
> mode. This however means that we are now missing a "divide by 10" after
> the m250_div (and before our new m25_en), otherwise the common clock
> framework thinks that the rate of the m25_en clock is 10-times higher
> than it is in the actual hardware. That is solved by adding a
> fixed-factor clock which divides the m250_div output by 10.
> 
> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> index 1c14210df465..7199e8c08536 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> @@ -40,9 +40,7 @@
>  #define PRG_ETH0_CLK_M250_DIV_SHIFT    7
>  #define PRG_ETH0_CLK_M250_DIV_WIDTH    3
>  
> -/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
> -#define PRG_ETH0_CLK_M25_DIV_SHIFT     10
> -#define PRG_ETH0_CLK_M25_DIV_WIDTH     1
> +#define PRG_ETH0_GENERATE_25M_PHY_CLOCK        10
>  
>  #define PRG_ETH0_INVERTED_RMII_CLK     BIT(11)
>  #define PRG_ETH0_TX_AND_PHY_REF_CLK    BIT(12)
> @@ -63,8 +61,11 @@ struct meson8b_dwmac {
>         struct clk_divider      m250_div;
>         struct clk              *m250_div_clk;
>  
> -       struct clk_divider      m25_div;
> -       struct clk              *m25_div_clk;
> +       struct clk_fixed_factor fixed_div10;
> +       struct clk              *fixed_div10_clk;
> +
> +       struct clk_gate         m25_en;
> +       struct clk              *m25_en_clk;

Maybe it could be the topic of another series but we don't need to keep all
those structures around, thanks to devm

all clk_divider, fixed_factor, gate and mux can go away
You only need to keep  the'struct clk*' you are going to use later on

at the moment it would be m25_en_clk only.

>  
>         u32                     tx_delay_ns;

[RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Jerome Brunet]

On Thu, 2017-12-28 at 23:21 +0100, Martin Blumenstingl wrote:
> While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
> discovered that the m25_div is not actually a divider but rather a gate.
> This matches with the datasheet which describes bit 10 as "Generate
> 25MHz clock for PHY". Back when the driver was written it was assumed
> that this was a divider (which could divide by 5 or 10) because other
> clock bits in the datasheet were documented incorrectly.

Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
RGMII, before being divided by 10 to produce the 25MHz of div25

IOW, maybe we need this intermediate rate.
It would not be surprising, 1GBps usually requires a 125MHz clock somewhere. 

This is still doable:
* Keep the divider
* drop CLK_SET_RATE_PARENT on div25
* call set_rate on div250 with 250MHz then on div25 with 25Mhz


> 
> Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
> Odroid-C1 (using a Meson8b SoC) does not work.
> On GXBB and newer SoCs (where the driver was initially tested with RGMII
> PHYs) this is not a problem because the input clock is running at 1GHz.
> The m250_div clock's biggest possible divider is 7 (3-bit divider, with
> value 0 being reserved). Thus we end up with a m250_div of 4 and a
> "m25_div" of 10 (= register value 1).
> 
> Instead it turns out that the Ethernet IP block seems to have a fixed
> "divide by 10" clock internally. This means that bit 10 is a gate clock
> which enables the RGMII clock output.
> 
> This replaces the "m25_div" clock with a clock gate called "m25_en"
> which ensures that we can set this bit to 1 whenever we enable RGMII
> mode. This however means that we are now missing a "divide by 10" after
> the m250_div (and before our new m25_en), otherwise the common clock
> framework thinks that the rate of the m25_en clock is 10-times higher
> than it is in the actual hardware. That is solved by adding a
> fixed-factor clock which divides the m250_div output by 10.
> 
> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> index 1c14210df465..7199e8c08536 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> @@ -40,9 +40,7 @@
>  #define PRG_ETH0_CLK_M250_DIV_SHIFT    7
>  #define PRG_ETH0_CLK_M250_DIV_WIDTH    3
>  
> -/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
> -#define PRG_ETH0_CLK_M25_DIV_SHIFT     10
> -#define PRG_ETH0_CLK_M25_DIV_WIDTH     1
> +#define PRG_ETH0_GENERATE_25M_PHY_CLOCK        10
>  
>  #define PRG_ETH0_INVERTED_RMII_CLK     BIT(11)
>  #define PRG_ETH0_TX_AND_PHY_REF_CLK    BIT(12)
> @@ -63,8 +61,11 @@ struct meson8b_dwmac {
>         struct clk_divider      m250_div;
>         struct clk              *m250_div_clk;
>  
> -       struct clk_divider      m25_div;
> -       struct clk              *m25_div_clk;
> +       struct clk_fixed_factor fixed_div10;
> +       struct clk              *fixed_div10_clk;
> +
> +       struct clk_gate         m25_en;
> +       struct clk              *m25_en_clk;

Maybe it could be the topic of another series but we don't need to keep all
those structures around, thanks to devm

all clk_divider, fixed_factor, gate and mux can go away
You only need to keep  the'struct clk*' you are going to use later on

at the moment it would be m25_en_clk only.

>  
>         u32                     tx_delay_ns;

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Jerome Brunet]

On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
> Hi Martin, Hi Dave,
> 
> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> > Hi Dave,
> > 
> > please do not apply this series until it got a Tested-by from Emiliano.
> > 
> > 
> > Hi Emiliano,
> > 
> > you reported [0] that you couldn't get dwmac-meson8b to work on your
> > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> > I think I was able to find a fix: it consists of two patches (which you
> > find in this series)
> > 
> > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> > only partially test this (I could only check if the clocks were
> > calculated correctly when using a dummy 500002394Hz input clock instead
> > of MPLL2).
> > 
> > Could you please give this series a try and let me know about the
> > results?
> > You obviously still need your two "ARM: dts: meson8b" patches which
> > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> > - enable Ethernet on the Odroid-C1
> > 
> > When testing on Meson8b this also needs a fix for the MPLL clock driver:
> > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> > https://patchwork.kernel.org/patch/10131677/
> > 
> > 
> > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> > fine (so let's hope that this also fixes your Meson8b issue :)).
> > 
> > 
> > changes since v1 at [1]:
> > - changed the subject of the cover-letter to indicate that this is all
> >   about the RGMII clock
> > - added PATCH #1 which ensures that we don't unnecessarily change the
> >   parent clocks in RMII mode (and also makes the code easier to
> >   understand)
> > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> >   is about the RGMII clock
> > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> > - replaced PATCH #3 (formerly PATCH #2) with one that sets
> >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> >   on Meson8b correctly
> > 
> > changes since v2 at [2]:
> > - added PATCH #2 to make the following patch easier
> > - Emiliano reported that there's currently another bug in the
> >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> >   (instead of a divide by 5 or divide by 10 clock divider). This has not
> >   been visible on GXBB and later due to the input clock which always led
> >   to a selection of "divide by 10" (which is done internally in the IP
> >   block, but the bit actually means "enable RGMII clock output").
> >   PATCH #3 was added to address this issue.
> > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> >   updated and the patch itself rebased because the m25_div clock was
> >   removed with the new PATCH #3 (so some of the statements were not
> >   valid anymore)
> > 
> 
> Here is the clk_summary relative to ethernet on Odroid-C1+
> with this new series applied:
> 
> xtal				    1            1    24000000          0 0
>  sys_pll			    0            0  1200000000          0 0
>   cpu_clk			    0            0  1200000000          0 0
>  vid_pll			    0            0   732000000          0 0
>  fixed_pll			    2            2  2550000000          0 0
>   mpll2				    1            1   249999701          0 0
>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>     c9410000.ethernet#m250_div	    1            1   249999701          0 0
>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>       c9410000.ethernet#m25_en	    1            1    24999970          0 0
> 
> The ethernet prg0 register is set to 0x74A1 which should be correct with
> respect to the information contained in the S805 SoC manual.
> Actually, the ethernet is not yet fully functional.
> Trying to ping the board, I can see ARP request from host to board using
> tcpdump. However, the host can't see any response.

If the rx path is ok-ish, I suppose the clock setting applied is good.
Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?

> 
> Following the U-Boot value for prg0 register, which is 0x7d21, I also
> tried to set bit 11. As expected, this did not have any influence.
> Another thing that we should check is the "Ethernet Memory PD" (see S805
> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> normal operation. However, those bits are already cleared by U-Boot.
> 
> Thank you for the support.
> 
> Best regards,
> 
> Emiliano
> 
> > 
> > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> > 
> > 
> > Martin Blumenstingl (5):
> >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> >   net: stmmac: dwmac-meson8b: simplify generating the clock names
> >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> > 
> >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> >  1 file changed, 63 insertions(+), 56 deletions(-)
> > 
> > -- 
> > 2.15.1
> > 

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Jerome Brunet]

On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
> Hi Martin, Hi Dave,
> 
> On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> > Hi Dave,
> > 
> > please do not apply this series until it got a Tested-by from Emiliano.
> > 
> > 
> > Hi Emiliano,
> > 
> > you reported [0] that you couldn't get dwmac-meson8b to work on your
> > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> > I think I was able to find a fix: it consists of two patches (which you
> > find in this series)
> > 
> > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> > only partially test this (I could only check if the clocks were
> > calculated correctly when using a dummy 500002394Hz input clock instead
> > of MPLL2).
> > 
> > Could you please give this series a try and let me know about the
> > results?
> > You obviously still need your two "ARM: dts: meson8b" patches which
> > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> > - enable Ethernet on the Odroid-C1
> > 
> > When testing on Meson8b this also needs a fix for the MPLL clock driver:
> > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> > https://patchwork.kernel.org/patch/10131677/
> > 
> > 
> > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> > fine (so let's hope that this also fixes your Meson8b issue :)).
> > 
> > 
> > changes since v1 at [1]:
> > - changed the subject of the cover-letter to indicate that this is all
> >   about the RGMII clock
> > - added PATCH #1 which ensures that we don't unnecessarily change the
> >   parent clocks in RMII mode (and also makes the code easier to
> >   understand)
> > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> >   is about the RGMII clock
> > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> > - replaced PATCH #3 (formerly PATCH #2) with one that sets
> >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> >   on Meson8b correctly
> > 
> > changes since v2 at [2]:
> > - added PATCH #2 to make the following patch easier
> > - Emiliano reported that there's currently another bug in the
> >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> >   (instead of a divide by 5 or divide by 10 clock divider). This has not
> >   been visible on GXBB and later due to the input clock which always led
> >   to a selection of "divide by 10" (which is done internally in the IP
> >   block, but the bit actually means "enable RGMII clock output").
> >   PATCH #3 was added to address this issue.
> > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> >   updated and the patch itself rebased because the m25_div clock was
> >   removed with the new PATCH #3 (so some of the statements were not
> >   valid anymore)
> > 
> 
> Here is the clk_summary relative to ethernet on Odroid-C1+
> with this new series applied:
> 
> xtal				    1            1    24000000          0 0
>  sys_pll			    0            0  1200000000          0 0
>   cpu_clk			    0            0  1200000000          0 0
>  vid_pll			    0            0   732000000          0 0
>  fixed_pll			    2            2  2550000000          0 0
>   mpll2				    1            1   249999701          0 0
>    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>     c9410000.ethernet#m250_div	    1            1   249999701          0 0
>      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>       c9410000.ethernet#m25_en	    1            1    24999970          0 0
> 
> The ethernet prg0 register is set to 0x74A1 which should be correct with
> respect to the information contained in the S805 SoC manual.
> Actually, the ethernet is not yet fully functional.
> Trying to ping the board, I can see ARP request from host to board using
> tcpdump. However, the host can't see any response.

If the rx path is ok-ish, I suppose the clock setting applied is good.
Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?

> 
> Following the U-Boot value for prg0 register, which is 0x7d21, I also
> tried to set bit 11. As expected, this did not have any influence.
> Another thing that we should check is the "Ethernet Memory PD" (see S805
> manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> normal operation. However, those bits are already cleared by U-Boot.
> 
> Thank you for the support.
> 
> Best regards,
> 
> Emiliano
> 
> > 
> > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> > 
> > 
> > Martin Blumenstingl (5):
> >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> >   net: stmmac: dwmac-meson8b: simplify generating the clock names
> >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> > 
> >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> >  1 file changed, 63 insertions(+), 56 deletions(-)
> > 
> > -- 
> > 2.15.1
> > 

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

Hi Jerome, Hi Martin,

On Fri, Dec 29, 2017 at 07:04:23PM +0100, Jerome Brunet wrote:
> On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
> > Hi Martin, Hi Dave,
> > 
> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> > > Hi Dave,
> > > 
> > > please do not apply this series until it got a Tested-by from Emiliano.
> > > 
> > > 
> > > Hi Emiliano,
> > > 
> > > you reported [0] that you couldn't get dwmac-meson8b to work on your
> > > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> > > I think I was able to find a fix: it consists of two patches (which you
> > > find in this series)
> > > 
> > > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> > > only partially test this (I could only check if the clocks were
> > > calculated correctly when using a dummy 500002394Hz input clock instead
> > > of MPLL2).
> > > 
> > > Could you please give this series a try and let me know about the
> > > results?
> > > You obviously still need your two "ARM: dts: meson8b" patches which
> > > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> > > - enable Ethernet on the Odroid-C1
> > > 
> > > When testing on Meson8b this also needs a fix for the MPLL clock driver:
> > > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> > > https://patchwork.kernel.org/patch/10131677/
> > > 
> > > 
> > > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> > > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> > > fine (so let's hope that this also fixes your Meson8b issue :)).
> > > 
> > > 
> > > changes since v1 at [1]:
> > > - changed the subject of the cover-letter to indicate that this is all
> > >   about the RGMII clock
> > > - added PATCH #1 which ensures that we don't unnecessarily change the
> > >   parent clocks in RMII mode (and also makes the code easier to
> > >   understand)
> > > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> > >   is about the RGMII clock
> > > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> > > - replaced PATCH #3 (formerly PATCH #2) with one that sets
> > >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> > >   on Meson8b correctly
> > > 
> > > changes since v2 at [2]:
> > > - added PATCH #2 to make the following patch easier
> > > - Emiliano reported that there's currently another bug in the
> > >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> > >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> > >   (instead of a divide by 5 or divide by 10 clock divider). This has not
> > >   been visible on GXBB and later due to the input clock which always led
> > >   to a selection of "divide by 10" (which is done internally in the IP
> > >   block, but the bit actually means "enable RGMII clock output").
> > >   PATCH #3 was added to address this issue.
> > > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> > >   updated and the patch itself rebased because the m25_div clock was
> > >   removed with the new PATCH #3 (so some of the statements were not
> > >   valid anymore)
> > > 
> > 
> > Here is the clk_summary relative to ethernet on Odroid-C1+
> > with this new series applied:
> > 
> > xtal				    1            1    24000000          0 0
> >  sys_pll			    0            0  1200000000          0 0
> >   cpu_clk			    0            0  1200000000          0 0
> >  vid_pll			    0            0   732000000          0 0
> >  fixed_pll			    2            2  2550000000          0 0
> >   mpll2				    1            1   249999701          0 0
> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
> >     c9410000.ethernet#m250_div	    1            1   249999701          0 0
> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
> >       c9410000.ethernet#m25_en	    1            1    24999970          0 0
> > 
> > The ethernet prg0 register is set to 0x74A1 which should be correct with
> > respect to the information contained in the S805 SoC manual.
> > Actually, the ethernet is not yet fully functional.
> > Trying to ping the board, I can see ARP request from host to board using
> > tcpdump. However, the host can't see any response.
> 
> If the rx path is ok-ish, I suppose the clock setting applied is good.
> Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?
>

Thanks for the suggestion. Finally the ethernet works correctly using 4
ns as tx-delay.
The clock summary is the same reported above. The prg0 ethernet register
value is 0x74c1 as expected.

I would like to thanks Martin for the support!
As soon as this patch series [v3] will be submitted, I'll submit my patch for
the device tree.
Let me know if you have any question.

Thanks again,

Emiliano

> > 
> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
> > tried to set bit 11. As expected, this did not have any influence.
> > Another thing that we should check is the "Ethernet Memory PD" (see S805
> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> > normal operation. However, those bits are already cleared by U-Boot.
> > 
> > Thank you for the support.
> > 
> > Best regards,
> > 
> > Emiliano
> > 
> > > 
> > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> > > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> > > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> > > 
> > > 
> > > Martin Blumenstingl (5):
> > >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> > >   net: stmmac: dwmac-meson8b: simplify generating the clock names
> > >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> > >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> > >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> > > 
> > >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> > >  1 file changed, 63 insertions(+), 56 deletions(-)
> > > 
> > > -- 
> > > 2.15.1
> > > 
> 

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Emiliano Ingrassia]

Hi Jerome, Hi Martin,

On Fri, Dec 29, 2017 at 07:04:23PM +0100, Jerome Brunet wrote:
> On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
> > Hi Martin, Hi Dave,
> > 
> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
> > > Hi Dave,
> > > 
> > > please do not apply this series until it got a Tested-by from Emiliano.
> > > 
> > > 
> > > Hi Emiliano,
> > > 
> > > you reported [0] that you couldn't get dwmac-meson8b to work on your
> > > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
> > > I think I was able to find a fix: it consists of two patches (which you
> > > find in this series)
> > > 
> > > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
> > > only partially test this (I could only check if the clocks were
> > > calculated correctly when using a dummy 500002394Hz input clock instead
> > > of MPLL2).
> > > 
> > > Could you please give this series a try and let me know about the
> > > results?
> > > You obviously still need your two "ARM: dts: meson8b" patches which
> > > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
> > > - enable Ethernet on the Odroid-C1
> > > 
> > > When testing on Meson8b this also needs a fix for the MPLL clock driver:
> > > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
> > > https://patchwork.kernel.org/patch/10131677/
> > > 
> > > 
> > > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
> > > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
> > > fine (so let's hope that this also fixes your Meson8b issue :)).
> > > 
> > > 
> > > changes since v1 at [1]:
> > > - changed the subject of the cover-letter to indicate that this is all
> > >   about the RGMII clock
> > > - added PATCH #1 which ensures that we don't unnecessarily change the
> > >   parent clocks in RMII mode (and also makes the code easier to
> > >   understand)
> > > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
> > >   is about the RGMII clock
> > > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
> > > - replaced PATCH #3 (formerly PATCH #2) with one that sets
> > >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
> > >   on Meson8b correctly
> > > 
> > > changes since v2 at [2]:
> > > - added PATCH #2 to make the following patch easier
> > > - Emiliano reported that there's currently another bug in the
> > >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
> > >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
> > >   (instead of a divide by 5 or divide by 10 clock divider). This has not
> > >   been visible on GXBB and later due to the input clock which always led
> > >   to a selection of "divide by 10" (which is done internally in the IP
> > >   block, but the bit actually means "enable RGMII clock output").
> > >   PATCH #3 was added to address this issue.
> > > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
> > >   updated and the patch itself rebased because the m25_div clock was
> > >   removed with the new PATCH #3 (so some of the statements were not
> > >   valid anymore)
> > > 
> > 
> > Here is the clk_summary relative to ethernet on Odroid-C1+
> > with this new series applied:
> > 
> > xtal				    1            1    24000000          0 0
> >  sys_pll			    0            0  1200000000          0 0
> >   cpu_clk			    0            0  1200000000          0 0
> >  vid_pll			    0            0   732000000          0 0
> >  fixed_pll			    2            2  2550000000          0 0
> >   mpll2				    1            1   249999701          0 0
> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
> >     c9410000.ethernet#m250_div	    1            1   249999701          0 0
> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
> >       c9410000.ethernet#m25_en	    1            1    24999970          0 0
> > 
> > The ethernet prg0 register is set to 0x74A1 which should be correct with
> > respect to the information contained in the S805 SoC manual.
> > Actually, the ethernet is not yet fully functional.
> > Trying to ping the board, I can see ARP request from host to board using
> > tcpdump. However, the host can't see any response.
> 
> If the rx path is ok-ish, I suppose the clock setting applied is good.
> Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?
>

Thanks for the suggestion. Finally the ethernet works correctly using 4
ns as tx-delay.
The clock summary is the same reported above. The prg0 ethernet register
value is 0x74c1 as expected.

I would like to thanks Martin for the support!
As soon as this patch series [v3] will be submitted, I'll submit my patch for
the device tree.
Let me know if you have any question.

Thanks again,

Emiliano

> > 
> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
> > tried to set bit 11. As expected, this did not have any influence.
> > Another thing that we should check is the "Ethernet Memory PD" (see S805
> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
> > normal operation. However, those bits are already cleared by U-Boot.
> > 
> > Thank you for the support.
> > 
> > Best regards,
> > 
> > Emiliano
> > 
> > > 
> > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
> > > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
> > > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
> > > 
> > > 
> > > Martin Blumenstingl (5):
> > >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
> > >   net: stmmac: dwmac-meson8b: simplify generating the clock names
> > >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
> > >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
> > >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> > > 
> > >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
> > >  1 file changed, 63 insertions(+), 56 deletions(-)
> > > 
> > > -- 
> > > 2.15.1
> > > 
> 

Re: [RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Martin Blumenstingl]

Hi Jerome,

On Fri, Dec 29, 2017 at 6:57 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Thu, 2017-12-28 at 23:21 +0100, Martin Blumenstingl wrote:
>> While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
>> discovered that the m25_div is not actually a divider but rather a gate.
>> This matches with the datasheet which describes bit 10 as "Generate
>> 25MHz clock for PHY". Back when the driver was written it was assumed
>> that this was a divider (which could divide by 5 or 10) because other
>> clock bits in the datasheet were documented incorrectly.
>
> Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
> Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
> RGMII, before being divided by 10 to produce the 25MHz of div25
>
> IOW, maybe we need this intermediate rate.
I am starting to believe that you (Emiliano and Jerome) are both right
does anyone of you have access to a scope so we can measure the actual
clock output?

> It would not be surprising, 1GBps usually requires a 125MHz clock somewhere.
this could mean that two clocks are derived from m250_div (names are
not final obviously):
- phy_ref_clk (25MHz or 50MHz)
- rgmii_tx_clk (fixed divide by 2, 125MHz)

> This is still doable:
> * Keep the divider
> * drop CLK_SET_RATE_PARENT on div25
> * call set_rate on div250 with 250MHz then on div25 with 25Mhz
yep, I will try this next
this would also be work with the assumption that the rgmii_tx_clk is
derived from m250_div

>
>>
>> Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
>> Odroid-C1 (using a Meson8b SoC) does not work.
>> On GXBB and newer SoCs (where the driver was initially tested with RGMII
>> PHYs) this is not a problem because the input clock is running at 1GHz.
>> The m250_div clock's biggest possible divider is 7 (3-bit divider, with
>> value 0 being reserved). Thus we end up with a m250_div of 4 and a
>> "m25_div" of 10 (= register value 1).
>>
>> Instead it turns out that the Ethernet IP block seems to have a fixed
>> "divide by 10" clock internally. This means that bit 10 is a gate clock
>> which enables the RGMII clock output.
>>
>> This replaces the "m25_div" clock with a clock gate called "m25_en"
>> which ensures that we can set this bit to 1 whenever we enable RGMII
>> mode. This however means that we are now missing a "divide by 10" after
>> the m250_div (and before our new m25_en), otherwise the common clock
>> framework thinks that the rate of the m25_en clock is 10-times higher
>> than it is in the actual hardware. That is solved by adding a
>> fixed-factor clock which divides the m250_div output by 10.
>>
>> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
>> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
>>  1 file changed, 38 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> index 1c14210df465..7199e8c08536 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> @@ -40,9 +40,7 @@
>>  #define PRG_ETH0_CLK_M250_DIV_SHIFT    7
>>  #define PRG_ETH0_CLK_M250_DIV_WIDTH    3
>>
>> -/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
>> -#define PRG_ETH0_CLK_M25_DIV_SHIFT     10
>> -#define PRG_ETH0_CLK_M25_DIV_WIDTH     1
>> +#define PRG_ETH0_GENERATE_25M_PHY_CLOCK        10
>>
>>  #define PRG_ETH0_INVERTED_RMII_CLK     BIT(11)
>>  #define PRG_ETH0_TX_AND_PHY_REF_CLK    BIT(12)
>> @@ -63,8 +61,11 @@ struct meson8b_dwmac {
>>         struct clk_divider      m250_div;
>>         struct clk              *m250_div_clk;
>>
>> -       struct clk_divider      m25_div;
>> -       struct clk              *m25_div_clk;
>> +       struct clk_fixed_factor fixed_div10;
>> +       struct clk              *fixed_div10_clk;
>> +
>> +       struct clk_gate         m25_en;
>> +       struct clk              *m25_en_clk;
>
> Maybe it could be the topic of another series but we don't need to keep all
> those structures around, thanks to devm
>
> all clk_divider, fixed_factor, gate and mux can go away
> You only need to keep  the'struct clk*' you are going to use later on
>
> at the moment it would be m25_en_clk only.
let's get the whole thing to work first, then I will have another look
at the struct members (it already annoyed me too)


PS: on another note: the title of this series and most patches is
wrong as I just discovered:
the 25MHz clock is *NOT* the RGMII clock, but it's the "PHY reference
clock". Hardkernel calls it "ETH_PHY_REF_CLK_25MOUT" in their
Odroid-C1 schematics [4] on page 23, which is the only actual
description I could find for this pin (on the Khadas VIM2 schematics
for example there's a 25MHz clock seemingly coming out of nowhere)

I used three publicly available datasheets for reference:
1) TI DP83822 (MII/RMII/RGMII): [0]
page: 5
pin: XI
description for MII and RGMII: Reference clock 25-MHz ±50
ppm-tolerance crystal or oscillator input. The device supports either
an external crystal resonator connected across pins XI and XO, or an
external CMOS-level oscillator connected to pin XI only
description for RMII: RMII reference clock: Reference clock 50-MHz ±50
ppm-tolerance CMOS-level oscillator in RMII Slave mode. Reference
clock 25-MHz ±50 ppm-tolerance crystal or oscillator in RMII Master
mode.

2) micrel DP83822 (RGMII): [1]
page: 13
pin: XI
description: Crystal / Oscillator / External Clock Input
25MHz ±50ppm tolerance

3) Realtek RTL8211E (RGMII, should be close to the RTL8211F PHY on our
Amlogic boards): [2]
page: 17
pin: CKXTAL1
description: 25/50MHz Crystal Input

this shows that Ethernet PHYs "typically" support 25MHz and 50MHz as
"reference clock input"
it also supports Emiliano's and Jerome's suggestion that m250_div
should run at 250MHz and m25_div might act as a divide-by-5 or
divide-by-10 bit.


Regards
Martin


[0] http://www.ti.com/lit/ds/symlink/dp83822h.pdf
[1] https://datasheet.lcsc.com/szlcsc/KSZ9031RNXCA_C58758.pdf
[2] https://www.pine64.pro/download/documents/realtek-10-100-1000-ethernet.pdf
[3] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20160226.pdf

[RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Martin Blumenstingl]

Hi Jerome,

On Fri, Dec 29, 2017 at 6:57 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Thu, 2017-12-28 at 23:21 +0100, Martin Blumenstingl wrote:
>> While testing the dwmac-meson8b with an RGMII PHY on Meson8b we
>> discovered that the m25_div is not actually a divider but rather a gate.
>> This matches with the datasheet which describes bit 10 as "Generate
>> 25MHz clock for PHY". Back when the driver was written it was assumed
>> that this was a divider (which could divide by 5 or 10) because other
>> clock bits in the datasheet were documented incorrectly.
>
> Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
> Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
> RGMII, before being divided by 10 to produce the 25MHz of div25
>
> IOW, maybe we need this intermediate rate.
I am starting to believe that you (Emiliano and Jerome) are both right
does anyone of you have access to a scope so we can measure the actual
clock output?

> It would not be surprising, 1GBps usually requires a 125MHz clock somewhere.
this could mean that two clocks are derived from m250_div (names are
not final obviously):
- phy_ref_clk (25MHz or 50MHz)
- rgmii_tx_clk (fixed divide by 2, 125MHz)

> This is still doable:
> * Keep the divider
> * drop CLK_SET_RATE_PARENT on div25
> * call set_rate on div250 with 250MHz then on div25 with 25Mhz
yep, I will try this next
this would also be work with the assumption that the rgmii_tx_clk is
derived from m250_div

>
>>
>> Tests have shown that without bit 10 set the RTL8211F RGMII PHY on
>> Odroid-C1 (using a Meson8b SoC) does not work.
>> On GXBB and newer SoCs (where the driver was initially tested with RGMII
>> PHYs) this is not a problem because the input clock is running at 1GHz.
>> The m250_div clock's biggest possible divider is 7 (3-bit divider, with
>> value 0 being reserved). Thus we end up with a m250_div of 4 and a
>> "m25_div" of 10 (= register value 1).
>>
>> Instead it turns out that the Ethernet IP block seems to have a fixed
>> "divide by 10" clock internally. This means that bit 10 is a gate clock
>> which enables the RGMII clock output.
>>
>> This replaces the "m25_div" clock with a clock gate called "m25_en"
>> which ensures that we can set this bit to 1 whenever we enable RGMII
>> mode. This however means that we are now missing a "divide by 10" after
>> the m250_div (and before our new m25_en), otherwise the common clock
>> framework thinks that the rate of the m25_en clock is 10-times higher
>> than it is in the actual hardware. That is solved by adding a
>> fixed-factor clock which divides the m250_div output by 10.
>>
>> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC")
>> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 66 +++++++++++++---------
>>  1 file changed, 38 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> index 1c14210df465..7199e8c08536 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> @@ -40,9 +40,7 @@
>>  #define PRG_ETH0_CLK_M250_DIV_SHIFT    7
>>  #define PRG_ETH0_CLK_M250_DIV_WIDTH    3
>>
>> -/* divides the result of m25_sel by either 5 (bit unset) or 10 (bit set) */
>> -#define PRG_ETH0_CLK_M25_DIV_SHIFT     10
>> -#define PRG_ETH0_CLK_M25_DIV_WIDTH     1
>> +#define PRG_ETH0_GENERATE_25M_PHY_CLOCK        10
>>
>>  #define PRG_ETH0_INVERTED_RMII_CLK     BIT(11)
>>  #define PRG_ETH0_TX_AND_PHY_REF_CLK    BIT(12)
>> @@ -63,8 +61,11 @@ struct meson8b_dwmac {
>>         struct clk_divider      m250_div;
>>         struct clk              *m250_div_clk;
>>
>> -       struct clk_divider      m25_div;
>> -       struct clk              *m25_div_clk;
>> +       struct clk_fixed_factor fixed_div10;
>> +       struct clk              *fixed_div10_clk;
>> +
>> +       struct clk_gate         m25_en;
>> +       struct clk              *m25_en_clk;
>
> Maybe it could be the topic of another series but we don't need to keep all
> those structures around, thanks to devm
>
> all clk_divider, fixed_factor, gate and mux can go away
> You only need to keep  the'struct clk*' you are going to use later on
>
> at the moment it would be m25_en_clk only.
let's get the whole thing to work first, then I will have another look
at the struct members (it already annoyed me too)


PS: on another note: the title of this series and most patches is
wrong as I just discovered:
the 25MHz clock is *NOT* the RGMII clock, but it's the "PHY reference
clock". Hardkernel calls it "ETH_PHY_REF_CLK_25MOUT" in their
Odroid-C1 schematics [4] on page 23, which is the only actual
description I could find for this pin (on the Khadas VIM2 schematics
for example there's a 25MHz clock seemingly coming out of nowhere)

I used three publicly available datasheets for reference:
1) TI DP83822 (MII/RMII/RGMII): [0]
page: 5
pin: XI
description for MII and RGMII: Reference clock 25-MHz ?50
ppm-tolerance crystal or oscillator input. The device supports either
an external crystal resonator connected across pins XI and XO, or an
external CMOS-level oscillator connected to pin XI only
description for RMII: RMII reference clock: Reference clock 50-MHz ?50
ppm-tolerance CMOS-level oscillator in RMII Slave mode. Reference
clock 25-MHz ?50 ppm-tolerance crystal or oscillator in RMII Master
mode.

2) micrel DP83822 (RGMII): [1]
page: 13
pin: XI
description: Crystal / Oscillator / External Clock Input
25MHz ?50ppm tolerance

3) Realtek RTL8211E (RGMII, should be close to the RTL8211F PHY on our
Amlogic boards): [2]
page: 17
pin: CKXTAL1
description: 25/50MHz Crystal Input

this shows that Ethernet PHYs "typically" support 25MHz and 50MHz as
"reference clock input"
it also supports Emiliano's and Jerome's suggestion that m250_div
should run at 250MHz and m25_div might act as a divide-by-5 or
divide-by-10 bit.


Regards
Martin


[0] http://www.ti.com/lit/ds/symlink/dp83822h.pdf
[1] https://datasheet.lcsc.com/szlcsc/KSZ9031RNXCA_C58758.pdf
[2] https://www.pine64.pro/download/documents/realtek-10-100-1000-ethernet.pdf
[3] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20160226.pdf

Re: [RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Emiliano,

On Sat, Dec 30, 2017 at 12:00 AM, Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> Hi Jerome, Hi Martin,
>
> On Fri, Dec 29, 2017 at 07:04:23PM +0100, Jerome Brunet wrote:
>> On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
>> > Hi Martin, Hi Dave,
>> >
>> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>> > > Hi Dave,
>> > >
>> > > please do not apply this series until it got a Tested-by from Emiliano.
>> > >
>> > >
>> > > Hi Emiliano,
>> > >
>> > > you reported [0] that you couldn't get dwmac-meson8b to work on your
>> > > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>> > > I think I was able to find a fix: it consists of two patches (which you
>> > > find in this series)
>> > >
>> > > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>> > > only partially test this (I could only check if the clocks were
>> > > calculated correctly when using a dummy 500002394Hz input clock instead
>> > > of MPLL2).
>> > >
>> > > Could you please give this series a try and let me know about the
>> > > results?
>> > > You obviously still need your two "ARM: dts: meson8b" patches which
>> > > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>> > > - enable Ethernet on the Odroid-C1
>> > >
>> > > When testing on Meson8b this also needs a fix for the MPLL clock driver:
>> > > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>> > > https://patchwork.kernel.org/patch/10131677/
>> > >
>> > >
>> > > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>> > > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>> > > fine (so let's hope that this also fixes your Meson8b issue :)).
>> > >
>> > >
>> > > changes since v1 at [1]:
>> > > - changed the subject of the cover-letter to indicate that this is all
>> > >   about the RGMII clock
>> > > - added PATCH #1 which ensures that we don't unnecessarily change the
>> > >   parent clocks in RMII mode (and also makes the code easier to
>> > >   understand)
>> > > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>> > >   is about the RGMII clock
>> > > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>> > > - replaced PATCH #3 (formerly PATCH #2) with one that sets
>> > >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>> > >   on Meson8b correctly
>> > >
>> > > changes since v2 at [2]:
>> > > - added PATCH #2 to make the following patch easier
>> > > - Emiliano reported that there's currently another bug in the
>> > >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>> > >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>> > >   (instead of a divide by 5 or divide by 10 clock divider). This has not
>> > >   been visible on GXBB and later due to the input clock which always led
>> > >   to a selection of "divide by 10" (which is done internally in the IP
>> > >   block, but the bit actually means "enable RGMII clock output").
>> > >   PATCH #3 was added to address this issue.
>> > > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>> > >   updated and the patch itself rebased because the m25_div clock was
>> > >   removed with the new PATCH #3 (so some of the statements were not
>> > >   valid anymore)
>> > >
>> >
>> > Here is the clk_summary relative to ethernet on Odroid-C1+
>> > with this new series applied:
>> >
>> > xtal                                    1            1    24000000          0 0
>> >  sys_pll                        0            0  1200000000          0 0
>> >   cpu_clk                       0            0  1200000000          0 0
>> >  vid_pll                        0            0   732000000          0 0
>> >  fixed_pll                      2            2  2550000000          0 0
>> >   mpll2                                 1            1   249999701          0 0
>> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>> >     c9410000.ethernet#m250_div          1            1   249999701          0 0
>> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>> >       c9410000.ethernet#m25_en          1            1    24999970          0 0
>> >
>> > The ethernet prg0 register is set to 0x74A1 which should be correct with
>> > respect to the information contained in the S805 SoC manual.
>> > Actually, the ethernet is not yet fully functional.
>> > Trying to ping the board, I can see ARP request from host to board using
>> > tcpdump. However, the host can't see any response.
>>
>> If the rx path is ok-ish, I suppose the clock setting applied is good.
>> Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?
>>
>
> Thanks for the suggestion. Finally the ethernet works correctly using 4
> ns as tx-delay.
> The clock summary is the same reported above. The prg0 ethernet register
> value is 0x74c1 as expected.
this is awesome!

I think I also know the reason why you need a 4ns TX delay: because
with the vendor kernel it's the same!
the TX delay in PRG_ETHERNET0 is set to 2ns
however, the RTL8211F PHY can also generate a 2ns TX delay - this can
be enabled by pin-strapping (by pulling a pin low/high - there's no
public documentation on that). if this is also enabled then the two
delays sum up
the mainline RTL8211F PHY driver however disables the TX delay in the
PHY when not using phy-mode "rgmii-txid"

> I would like to thanks Martin for the support!
> As soon as this patch series [v3] will be submitted, I'll submit my patch for
> the device tree.
I would like to finish the discussion with Jerome on patch #3 - then I
will send a final version

if anyone has an oscilloscope that is able to measure up to ~50MHz
then I'd be happy if someone would volunteer to test... :)
(I ordered some parts to do that myself, but I'm not sure when this will arrive)

on any Amlogic SoC with RTL8211F Ethernet PHY this can be measured on
pin 36 of the PHY (Odroid-C1's datasheet describes pin 36 as
"XTAL_IN", see page 12 of the schematics: [0]):
- test 1: this series with bit 10 set -> should output 25MHz
- test 2: this series, but manually unset bit 10 after the
clk_set_rate() call -> if bit 10 is a gate then the clock signal is
off, if it's a divider then there should be a 50MHz signal

> Let me know if you have any question.
>
> Thanks again,
thank you for not giving up :)

> Emiliano
>
>> >
>> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
>> > tried to set bit 11. As expected, this did not have any influence.
>> > Another thing that we should check is the "Ethernet Memory PD" (see S805
>> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
>> > normal operation. However, those bits are already cleared by U-Boot.
>> >
>> > Thank you for the support.
>> >
>> > Best regards,
>> >
>> > Emiliano
>> >
>> > >
>> > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>> > > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>> > > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>> > >
>> > >
>> > > Martin Blumenstingl (5):
>> > >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>> > >   net: stmmac: dwmac-meson8b: simplify generating the clock names
>> > >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>> > >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>> > >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>> > >
>> > >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>> > >  1 file changed, 63 insertions(+), 56 deletions(-)
>> > >
>> > > --
>> > > 2.15.1
>> > >
>>


Regards
Martin


[0] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20160226.pdf

[RFT net-next v3 0/5] dwmac-meson8b: RGMII clock fixes for Meson8b

[Martin Blumenstingl]

Hi Emiliano,

On Sat, Dec 30, 2017 at 12:00 AM, Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> Hi Jerome, Hi Martin,
>
> On Fri, Dec 29, 2017 at 07:04:23PM +0100, Jerome Brunet wrote:
>> On Fri, 2017-12-29 at 02:31 +0100, Emiliano Ingrassia wrote:
>> > Hi Martin, Hi Dave,
>> >
>> > On Thu, Dec 28, 2017 at 11:21:23PM +0100, Martin Blumenstingl wrote:
>> > > Hi Dave,
>> > >
>> > > please do not apply this series until it got a Tested-by from Emiliano.
>> > >
>> > >
>> > > Hi Emiliano,
>> > >
>> > > you reported [0] that you couldn't get dwmac-meson8b to work on your
>> > > Odroid-C1. With your findings (register dumps, clk_summary output, etc.)
>> > > I think I was able to find a fix: it consists of two patches (which you
>> > > find in this series)
>> > >
>> > > Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
>> > > only partially test this (I could only check if the clocks were
>> > > calculated correctly when using a dummy 500002394Hz input clock instead
>> > > of MPLL2).
>> > >
>> > > Could you please give this series a try and let me know about the
>> > > results?
>> > > You obviously still need your two "ARM: dts: meson8b" patches which
>> > > - add the amlogic,meson8b-dwmac" compatible to meson8b.dtsi
>> > > - enable Ethernet on the Odroid-C1
>> > >
>> > > When testing on Meson8b this also needs a fix for the MPLL clock driver:
>> > > "clk: meson: mpll: use 64-bit maths in params_from_rate", see:
>> > > https://patchwork.kernel.org/patch/10131677/
>> > >
>> > >
>> > > I have tested this myself on a Khadas VIM (GXL SoC, internal RMII PHY)
>> > > and a Khadas VIM2 (GXM SoC, external RGMII PHY). Both are still working
>> > > fine (so let's hope that this also fixes your Meson8b issue :)).
>> > >
>> > >
>> > > changes since v1 at [1]:
>> > > - changed the subject of the cover-letter to indicate that this is all
>> > >   about the RGMII clock
>> > > - added PATCH #1 which ensures that we don't unnecessarily change the
>> > >   parent clocks in RMII mode (and also makes the code easier to
>> > >   understand)
>> > > - changed subject of PATCH #2 (formerly PATCH #1) to state that this
>> > >   is about the RGMII clock
>> > > - added Jerome's Reviewed-by to PATCH #2 (formerly PATCH #1)
>> > > - replaced PATCH #3 (formerly PATCH #2) with one that sets
>> > >   CLK_SET_RATE_PARENT on the mux and thus re-configures the MPLL2 clock
>> > >   on Meson8b correctly
>> > >
>> > > changes since v2 at [2]:
>> > > - added PATCH #2 to make the following patch easier
>> > > - Emiliano reported that there's currently another bug in the
>> > >   dwmac-meson8b driver which prevents it from working with RGMII PHYs on
>> > >   Meson8b: bit 10 of the PRG_ETH0 register is configures a clock gate
>> > >   (instead of a divide by 5 or divide by 10 clock divider). This has not
>> > >   been visible on GXBB and later due to the input clock which always led
>> > >   to a selection of "divide by 10" (which is done internally in the IP
>> > >   block, but the bit actually means "enable RGMII clock output").
>> > >   PATCH #3 was added to address this issue.
>> > > - the commit message of PATCH #4 and #5 (formerly PATCH #2 and #3) were
>> > >   updated and the patch itself rebased because the m25_div clock was
>> > >   removed with the new PATCH #3 (so some of the statements were not
>> > >   valid anymore)
>> > >
>> >
>> > Here is the clk_summary relative to ethernet on Odroid-C1+
>> > with this new series applied:
>> >
>> > xtal                                    1            1    24000000          0 0
>> >  sys_pll                        0            0  1200000000          0 0
>> >   cpu_clk                       0            0  1200000000          0 0
>> >  vid_pll                        0            0   732000000          0 0
>> >  fixed_pll                      2            2  2550000000          0 0
>> >   mpll2                                 1            1   249999701          0 0
>> >    c9410000.ethernet#m250_sel       1            1   249999701          0 0
>> >     c9410000.ethernet#m250_div          1            1   249999701          0 0
>> >      c9410000.ethernet#fixed_div10  1            1    24999970          0 0
>> >       c9410000.ethernet#m25_en          1            1    24999970          0 0
>> >
>> > The ethernet prg0 register is set to 0x74A1 which should be correct with
>> > respect to the information contained in the S805 SoC manual.
>> > Actually, the ethernet is not yet fully functional.
>> > Trying to ping the board, I can see ARP request from host to board using
>> > tcpdump. However, the host can't see any response.
>>
>> If the rx path is ok-ish, I suppose the clock setting applied is good.
>> Maybe you could try to play with the tx delay (BIT 5-6 of the register) ?
>>
>
> Thanks for the suggestion. Finally the ethernet works correctly using 4
> ns as tx-delay.
> The clock summary is the same reported above. The prg0 ethernet register
> value is 0x74c1 as expected.
this is awesome!

I think I also know the reason why you need a 4ns TX delay: because
with the vendor kernel it's the same!
the TX delay in PRG_ETHERNET0 is set to 2ns
however, the RTL8211F PHY can also generate a 2ns TX delay - this can
be enabled by pin-strapping (by pulling a pin low/high - there's no
public documentation on that). if this is also enabled then the two
delays sum up
the mainline RTL8211F PHY driver however disables the TX delay in the
PHY when not using phy-mode "rgmii-txid"

> I would like to thanks Martin for the support!
> As soon as this patch series [v3] will be submitted, I'll submit my patch for
> the device tree.
I would like to finish the discussion with Jerome on patch #3 - then I
will send a final version

if anyone has an oscilloscope that is able to measure up to ~50MHz
then I'd be happy if someone would volunteer to test... :)
(I ordered some parts to do that myself, but I'm not sure when this will arrive)

on any Amlogic SoC with RTL8211F Ethernet PHY this can be measured on
pin 36 of the PHY (Odroid-C1's datasheet describes pin 36 as
"XTAL_IN", see page 12 of the schematics: [0]):
- test 1: this series with bit 10 set -> should output 25MHz
- test 2: this series, but manually unset bit 10 after the
clk_set_rate() call -> if bit 10 is a gate then the clock signal is
off, if it's a divider then there should be a 50MHz signal

> Let me know if you have any question.
>
> Thanks again,
thank you for not giving up :)

> Emiliano
>
>> >
>> > Following the U-Boot value for prg0 register, which is 0x7d21, I also
>> > tried to set bit 11. As expected, this did not have any influence.
>> > Another thing that we should check is the "Ethernet Memory PD" (see S805
>> > manual - sec. 5.4) register which bits 3-2 enable/disable ethernet
>> > normal operation. However, those bits are already cleared by U-Boot.
>> >
>> > Thank you for the support.
>> >
>> > Best regards,
>> >
>> > Emiliano
>> >
>> > >
>> > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005596.html
>> > > [1] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005848.html
>> > > [2] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005861.html
>> > >
>> > >
>> > > Martin Blumenstingl (5):
>> > >   net: stmmac: dwmac-meson8b: only configure the clocks in RGMII mode
>> > >   net: stmmac: dwmac-meson8b: simplify generating the clock names
>> > >   net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration
>> > >   net: stmmac: dwmac-meson8b: fix setting the RGMII clock on Meson8b
>> > >   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
>> > >
>> > >  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 119 +++++++++++----------
>> > >  1 file changed, 63 insertions(+), 56 deletions(-)
>> > >
>> > > --
>> > > 2.15.1
>> > >
>>


Regards
Martin


[0] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20160226.pdf

Re: [RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Jerome Brunet]

On Sat, 2017-12-30 at 00:40 +0100, Martin Blumenstingl wrote:
> > Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
> > Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
> > RGMII, before being divided by 10 to produce the 25MHz of div25
> > 
> > IOW, maybe we need this intermediate rate.
> 
> I am starting to believe that you (Emiliano and Jerome) are both right
> does anyone of you have access to a scope so we can measure the actual
> clock output?

I wanted to check this out on Gx but the 25M output is not any of the boards I
have (p200, OC2, S400). I should be able to look at the related SoC pad on the
p200 but, I don't know how to enable the GPIOCLK_1 Function 1 in the pinmux
configuration

> 
> > It would not be surprising, 1GBps usually requires a 125MHz clock somewhere.
> 
> this could mean that two clocks are derived from m250_div (names are
> not final obviously):
> - phy_ref_clk (25MHz or 50MHz)
> - rgmii_tx_clk (fixed divide by 2, 125MHz)

Probably yes.

What we consider in the code as div250 divider is actually described in snip of
doc we have as:
-----
bit 10 : Generate 25MHz clock for PHY
bit 9-7: RMII & RGMII mode:
- 001: clock source is 250MHz
- 010: clock source is 500MHz.
...
-----

1) It kind of shows that the minimum input frequency could be 250M indeed.
2) It is these unclear whether bit 10 is a gate or a divider ... ATM, I can't
check that myself
3) Looks like properly setting up div250 should also be done for RMII.

> 
> > This is still doable:
> > * Keep the divider
> > * drop CLK_SET_RATE_PARENT on div25
> > * call set_rate on div250 with 250MHz then on div25 with 25Mhz
> 
> yep, I will try this next
> this would also be work with the assumption that the rgmii_tx_clk is
> derived from m250_div

[RFT net-next v3 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Jerome Brunet]

On Sat, 2017-12-30 at 00:40 +0100, Martin Blumenstingl wrote:
> > Maybe this bit 10 is indeed a 5/10 divider, as amlogic claims it is. Maybe, as
> > Emiliano suggested, the output rate of div250 actually needs to be 250Mhz in
> > RGMII, before being divided by 10 to produce the 25MHz of div25
> > 
> > IOW, maybe we need this intermediate rate.
> 
> I am starting to believe that you (Emiliano and Jerome) are both right
> does anyone of you have access to a scope so we can measure the actual
> clock output?

I wanted to check this out on Gx but the 25M output is not any of the boards I
have (p200, OC2, S400). I should be able to look at the related SoC pad on the
p200 but, I don't know how to enable the GPIOCLK_1 Function 1 in the pinmux
configuration

> 
> > It would not be surprising, 1GBps usually requires a 125MHz clock somewhere.
> 
> this could mean that two clocks are derived from m250_div (names are
> not final obviously):
> - phy_ref_clk (25MHz or 50MHz)
> - rgmii_tx_clk (fixed divide by 2, 125MHz)

Probably yes.

What we consider in the code as div250 divider is actually described in snip of
doc we have as:
-----
bit 10 : Generate 25MHz clock for PHY
bit 9-7: RMII & RGMII mode:
- 001: clock source is 250MHz
- 010: clock source is 500MHz.
...
-----

1) It kind of shows that the minimum input frequency could be 250M indeed.
2) It is these unclear whether bit 10 is a gate or a divider ... ATM, I can't
check that myself
3) Looks like properly setting up div250 should also be done for RMII.

> 
> > This is still doable:
> > * Keep the divider
> > * drop CLK_SET_RATE_PARENT on div25
> > * call set_rate on div250 with 250MHz then on div25 with 25Mhz
> 
> yep, I will try this next
> this would also be work with the assumption that the rgmii_tx_clk is
> derived from m250_div