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

[RFT net-next v4 0/5] dwmac-meson8b: 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)

Due to the fact that the public S805/S905/S912 datasheets all seem to
be outdated regarding the description of the PRG_ETH0 (also called
PRG_ETHERNET_ADDR0) register Linus Lüssing offered to help testing with
an oscilloscope and an Odroid-C1. I would like to say HUGE thanks to him
at this point as he spent hours figuring out the effects of the bits
that are (though to be) relevant to get Ethernet working on the
Odroid-C1.
We tested three scenarios, all based on version 3 of this series:
1) MPLL2 at ~500MHz, m250_div set to 1, bit 10 enabled
this resulted in a clock rate twice as high as expected at the RGMII TX
clock pin (250MHz instead of 125MHz for Gbit connections and 50MHz
instead of 25MHz for 100Mbit/s connections). it did not change the
rate at the XTAL_IN pin of PHY (which stayed consistenly at 25MHz)
2) MPLL2 at ~250MHz, m250_div set to 1, bit 10 disabled
the oscilloscope shows "no clock" for the RGMII TX clock pin at it's
highest resolution (and random rates at lower resolutions). XTAL_IN is
still at 25MHz
3) MPLL2 at ~250MHz, m250_div set to 1, bit 10 enabled
this resulted in a 125MHz signal at the RGMII TX clock pin for Gbit
speeds and 25MHz for 100Mbit/s - both values are as expected. The rate
on the XTAL_IN pin was at 25MHz
-> boot-logs (with the PRG_ETH0 register value) and screenshots from the
readings of the oscilloscope can be found at:
https://metameute.de/~tux/linux/amlogic/odroidc1/ethernet/

Version 4 of this series is based on the results from Linus Lüssing's
help with the oscilloscope and Odroid-C1.
Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
only partially test this. @Emiliano: Could you please give this version
a try and let me know about the results (preferably with a "Tested-by"
if it works)?
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 (according to your last thest a TX
  delay of 4ns is required to make it work properly)

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 v3 at [3]:
- renamed the function PATCH #1 from meson8b_init_rgmii_clk to
  meson8b_init_rgmii_tx_clk since we now know what the register bits
  mean
- rewrote PATCH #3 because bit 10 is a gate clock and it seems that
  there is an internal fixed divide-by-2 clock. see the patch
  description for a detailed explanation
- updated the description of PATCH #4 and #5 as the clock we're trying
  to fix is the "RGMII TX" clock (old version stated that this is the
  "RGMII clock" or "PHY reference clock"). also updated the numbers in
  the description now that we have the clock hierarchy right (at least
  we hope so)

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)

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


[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
[3] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005899.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 TX clock on Meson8b
  net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

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

-- 
2.15.1

[RFT net-next v4 0/5] dwmac-meson8b: 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)

Due to the fact that the public S805/S905/S912 datasheets all seem to
be outdated regarding the description of the PRG_ETH0 (also called
PRG_ETHERNET_ADDR0) register Linus L?ssing offered to help testing with
an oscilloscope and an Odroid-C1. I would like to say HUGE thanks to him
at this point as he spent hours figuring out the effects of the bits
that are (though to be) relevant to get Ethernet working on the
Odroid-C1.
We tested three scenarios, all based on version 3 of this series:
1) MPLL2 at ~500MHz, m250_div set to 1, bit 10 enabled
this resulted in a clock rate twice as high as expected at the RGMII TX
clock pin (250MHz instead of 125MHz for Gbit connections and 50MHz
instead of 25MHz for 100Mbit/s connections). it did not change the
rate at the XTAL_IN pin of PHY (which stayed consistenly at 25MHz)
2) MPLL2 at ~250MHz, m250_div set to 1, bit 10 disabled
the oscilloscope shows "no clock" for the RGMII TX clock pin at it's
highest resolution (and random rates at lower resolutions). XTAL_IN is
still at 25MHz
3) MPLL2 at ~250MHz, m250_div set to 1, bit 10 enabled
this resulted in a 125MHz signal at the RGMII TX clock pin for Gbit
speeds and 25MHz for 100Mbit/s - both values are as expected. The rate
on the XTAL_IN pin was at 25MHz
-> boot-logs (with the PRG_ETH0 register value) and screenshots from the
readings of the oscilloscope can be found at:
https://metameute.de/~tux/linux/amlogic/odroidc1/ethernet/

Version 4 of this series is based on the results from Linus L?ssing's
help with the oscilloscope and Odroid-C1.
Unfortunately I don't have any Meson8b boards with RGMII PHY so I could
only partially test this. @Emiliano: Could you please give this version
a try and let me know about the results (preferably with a "Tested-by"
if it works)?
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 (according to your last thest a TX
  delay of 4ns is required to make it work properly)

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 v3 at [3]:
- renamed the function PATCH #1 from meson8b_init_rgmii_clk to
  meson8b_init_rgmii_tx_clk since we now know what the register bits
  mean
- rewrote PATCH #3 because bit 10 is a gate clock and it seems that
  there is an internal fixed divide-by-2 clock. see the patch
  description for a detailed explanation
- updated the description of PATCH #4 and #5 as the clock we're trying
  to fix is the "RGMII TX" clock (old version stated that this is the
  "RGMII clock" or "PHY reference clock"). also updated the numbers in
  the description now that we have the clock hierarchy right (at least
  we hope so)

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)

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


[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
[3] http://lists.infradead.org/pipermail/linux-amlogic/2017-December/005899.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 TX clock on Meson8b
  net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock

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

-- 
2.15.1

[RFT net-next v4 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_tx_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..c6f87e9c4ccb 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_tx_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_tx_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 v4 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_tx_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..c6f87e9c4ccb 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_tx_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_tx_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 v4 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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 v4 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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 v4 3/5] net: stmmac: dwmac-meson8b: fix internal RGMII clock configuration

[Martin Blumenstingl]

Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
- bit 4 is a mux to choose between two parent clocks. according to the
  public S805 datasheet the only supported parent clock is MPLL2 (this
  was not verified using the oscilloscope).
  The public S805/S905 datasheet claims that this bit is reserved.
- bits 9:7 control a one-based divider (register value 1 means "divide
  by 1", etc.) for the input clock. we call this clock the "m250_div"
  clock because it's value is always supposed to be (close to) 250MHz
  (see below for an explanation).
  The description in the public S805/S905 datasheet is a bit cryptic,
  but it comes down to "input clock = 250MHz * value" (which could also
  be expressed as "250MHz = input clock / value")
- there seems to be an internal fixed divide-by-2 clock which takes the
  output from the m250_div and divides it by 2. This is not unusual on
  Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
  divide-by-2 clock.
  This is not documented in the public S805/S905 datasheet
- bit 10 controls a gate clock which enables or disables the RGMII TX
  clock (which is an output on the MAC/SoC and an input in the PHY). we
  call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
  TX clock output is close to 0
  The description for this bit in the public S805/S905 datasheet is
  "Generate 25MHz clock for PHY". Based on these tests it's believed
  that this is wrong, and should probably read "Generate the 125MHz
  RGMII TX clock for the PHY"
- the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
  output (automatically) depending on the line speed (RGMII specifies
  that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
  25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
  100Mbit/s were tested with an oscilloscope). Due to the requirement
  that this clock always has to be set to 125MHz and due to the fixed
  divide-by-2 parent clock this means that m250_div will always end up
  with a rate of (close to) 250MHz.
- bits 6:5 are the TX delay, which is also named "clock phase" in some
  of Amlogic's older GPL kernel sources.

The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
Tests with the oscilloscope have shown that this is routed to a crystal
right next to the RTL8211F PHY. The same seems to be true on the Khadas
VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
other side of the PCB there.

This updates the clocks in the dwmac-meson8b driver by replacing the
"m25_div" with the "rgmii_tx_en" clock and additionally introducing a
fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
Now we also need to set a frequency of 125MHz on the RGMII clock
(opposed to the 25MHz we set before, with that non-existing
divide-by-5-or-10 divider).

Special thanks go to Linus Lüssing for testing the various bits and
checking the results with an oscilloscope on his Odroid-C1!

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    | 79 +++++++++++++---------
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 670f344f7168..e9fec9e0425c 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_RGMII_TX_CLK_EN	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_div2;
+	struct clk		*fixed_div2_clk;
+
+	struct clk_gate		rgmii_tx_en;
+	struct clk		*rgmii_tx_en_clk;
 
 	u32			tx_delay_ns;
 };
@@ -88,11 +89,6 @@ static int meson8b_init_rgmii_tx_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,40 @@ static int meson8b_init_rgmii_tx_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_div2 */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div2",
 				   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_div2.mult = 1;
+	dwmac->fixed_div2.div = 2;
+	dwmac->fixed_div2.hw.init = &init;
+
+	dwmac->fixed_div2_clk = devm_clk_register(dev, &dwmac->fixed_div2.hw);
+	if (WARN_ON(IS_ERR(dwmac->fixed_div2_clk)))
+		return PTR_ERR(dwmac->fixed_div2_clk);
+
+	/* create the rgmii_tx_en */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#rgmii_tx_en",
+				   dev_name(dev));
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	clk_div_parents[0] = __clk_get_name(dwmac->fixed_div2_clk);
+	init.parent_names = clk_div_parents;
+	init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+	dwmac->rgmii_tx_en.reg = dwmac->regs + PRG_ETH0;
+	dwmac->rgmii_tx_en.bit_idx = PRG_ETH0_RGMII_TX_CLK_EN;
+	dwmac->rgmii_tx_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->rgmii_tx_en_clk = devm_clk_register(dev,
+						   &dwmac->rgmii_tx_en.hw);
+	if (WARN_ON(IS_ERR(dwmac->rgmii_tx_en_clk)))
+		return PTR_ERR(dwmac->rgmii_tx_en_clk);
 
 	return 0;
 }
@@ -200,18 +211,22 @@ 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);
+		/* Configure the 125MHz RGMII TX clock, the IP block changes
+		 * the output automatically (= without us having to configure
+		 * a register) based on the line-speed (125MHz for Gbit speeds,
+		 * 25MHz for 100Mbit/s and 2.5MHz for 10Mbit/s).
+		 */
+		ret = clk_set_rate(dwmac->rgmii_tx_en_clk, 125 * 1000 * 1000);
 		if (ret) {
-			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
+			dev_err(&dwmac->pdev->dev,
+				"failed to set RGMII TX 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_prepare_enable(dwmac->rgmii_tx_en_clk);
 		if (ret) {
-			clk_disable_unprepare(dwmac->m25_div_clk);
-
-			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
+			dev_err(&dwmac->pdev->dev,
+				"failed to enable the RGMII TX clock\n");
 			return ret;
 		}
 		break;
@@ -305,7 +320,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->rgmii_tx_en_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -317,7 +332,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->rgmii_tx_en_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

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

[Martin Blumenstingl]

Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
- bit 4 is a mux to choose between two parent clocks. according to the
  public S805 datasheet the only supported parent clock is MPLL2 (this
  was not verified using the oscilloscope).
  The public S805/S905 datasheet claims that this bit is reserved.
- bits 9:7 control a one-based divider (register value 1 means "divide
  by 1", etc.) for the input clock. we call this clock the "m250_div"
  clock because it's value is always supposed to be (close to) 250MHz
  (see below for an explanation).
  The description in the public S805/S905 datasheet is a bit cryptic,
  but it comes down to "input clock = 250MHz * value" (which could also
  be expressed as "250MHz = input clock / value")
- there seems to be an internal fixed divide-by-2 clock which takes the
  output from the m250_div and divides it by 2. This is not unusual on
  Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
  divide-by-2 clock.
  This is not documented in the public S805/S905 datasheet
- bit 10 controls a gate clock which enables or disables the RGMII TX
  clock (which is an output on the MAC/SoC and an input in the PHY). we
  call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
  TX clock output is close to 0
  The description for this bit in the public S805/S905 datasheet is
  "Generate 25MHz clock for PHY". Based on these tests it's believed
  that this is wrong, and should probably read "Generate the 125MHz
  RGMII TX clock for the PHY"
- the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
  output (automatically) depending on the line speed (RGMII specifies
  that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
  25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
  100Mbit/s were tested with an oscilloscope). Due to the requirement
  that this clock always has to be set to 125MHz and due to the fixed
  divide-by-2 parent clock this means that m250_div will always end up
  with a rate of (close to) 250MHz.
- bits 6:5 are the TX delay, which is also named "clock phase" in some
  of Amlogic's older GPL kernel sources.

The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
Tests with the oscilloscope have shown that this is routed to a crystal
right next to the RTL8211F PHY. The same seems to be true on the Khadas
VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
other side of the PCB there.

This updates the clocks in the dwmac-meson8b driver by replacing the
"m25_div" with the "rgmii_tx_en" clock and additionally introducing a
fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
Now we also need to set a frequency of 125MHz on the RGMII clock
(opposed to the 25MHz we set before, with that non-existing
divide-by-5-or-10 divider).

Special thanks go to Linus L?ssing for testing the various bits and
checking the results with an oscilloscope on his Odroid-C1!

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    | 79 +++++++++++++---------
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 670f344f7168..e9fec9e0425c 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_RGMII_TX_CLK_EN	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_div2;
+	struct clk		*fixed_div2_clk;
+
+	struct clk_gate		rgmii_tx_en;
+	struct clk		*rgmii_tx_en_clk;
 
 	u32			tx_delay_ns;
 };
@@ -88,11 +89,6 @@ static int meson8b_init_rgmii_tx_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,40 @@ static int meson8b_init_rgmii_tx_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_div2 */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div2",
 				   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_div2.mult = 1;
+	dwmac->fixed_div2.div = 2;
+	dwmac->fixed_div2.hw.init = &init;
+
+	dwmac->fixed_div2_clk = devm_clk_register(dev, &dwmac->fixed_div2.hw);
+	if (WARN_ON(IS_ERR(dwmac->fixed_div2_clk)))
+		return PTR_ERR(dwmac->fixed_div2_clk);
+
+	/* create the rgmii_tx_en */
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#rgmii_tx_en",
+				   dev_name(dev));
+	init.ops = &clk_gate_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	clk_div_parents[0] = __clk_get_name(dwmac->fixed_div2_clk);
+	init.parent_names = clk_div_parents;
+	init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+	dwmac->rgmii_tx_en.reg = dwmac->regs + PRG_ETH0;
+	dwmac->rgmii_tx_en.bit_idx = PRG_ETH0_RGMII_TX_CLK_EN;
+	dwmac->rgmii_tx_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->rgmii_tx_en_clk = devm_clk_register(dev,
+						   &dwmac->rgmii_tx_en.hw);
+	if (WARN_ON(IS_ERR(dwmac->rgmii_tx_en_clk)))
+		return PTR_ERR(dwmac->rgmii_tx_en_clk);
 
 	return 0;
 }
@@ -200,18 +211,22 @@ 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);
+		/* Configure the 125MHz RGMII TX clock, the IP block changes
+		 * the output automatically (= without us having to configure
+		 * a register) based on the line-speed (125MHz for Gbit speeds,
+		 * 25MHz for 100Mbit/s and 2.5MHz for 10Mbit/s).
+		 */
+		ret = clk_set_rate(dwmac->rgmii_tx_en_clk, 125 * 1000 * 1000);
 		if (ret) {
-			dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
+			dev_err(&dwmac->pdev->dev,
+				"failed to set RGMII TX 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_prepare_enable(dwmac->rgmii_tx_en_clk);
 		if (ret) {
-			clk_disable_unprepare(dwmac->m25_div_clk);
-
-			dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
+			dev_err(&dwmac->pdev->dev,
+				"failed to enable the RGMII TX clock\n");
 			return ret;
 		}
 		break;
@@ -305,7 +320,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->rgmii_tx_en_clk);
 err_remove_config_dt:
 	stmmac_remove_config_dt(pdev, plat_dat);
 
@@ -317,7 +332,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->rgmii_tx_en_clk);
 
 	return stmmac_pltfr_remove(pdev);
 }
-- 
2.15.1

[RFT net-next v4 4/5] net: stmmac: dwmac-meson8b: fix setting the RGMII TX 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 (but not exactly) 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) 125MHz RGMII TX clock (on Gbit speeds,
the IP block internally divides that down to 25MHz on 100Mbit/s
connections and 2.5MHz on 10Mbit/s connections - we don't need any
special configuration for that).

Round the divider to the closest value to prevent this issue on Meson8b.
This means we'll now end up with a clock rate for the RGMII TX clock of
125001197Hz (= 125MHz plus 1197Hz), which is close-enough to 125MHz.
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 125MHz).

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 e9fec9e0425c..e30ad05d6a6e 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_tx_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 v4 4/5] net: stmmac: dwmac-meson8b: fix setting the RGMII TX 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 (but not exactly) 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) 125MHz RGMII TX clock (on Gbit speeds,
the IP block internally divides that down to 25MHz on 100Mbit/s
connections and 2.5MHz on 10Mbit/s connections - we don't need any
special configuration for that).

Round the divider to the closest value to prevent this issue on Meson8b.
This means we'll now end up with a clock rate for the RGMII TX clock of
125001197Hz (= 125MHz plus 1197Hz), which is close-enough to 125MHz.
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 125MHz).

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 e9fec9e0425c..e30ad05d6a6e 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_tx_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 v4 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 125MHz using the m250_div clock. Currently the common clock
framework chooses a m250_div of 2 - with the internal fixed
"divide by 10" this results in a RGMII TX clock of 125001197Hz (120Hz
above the requested 125MHz).

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 2 (which is an internal,
fixed divider for the RGMII TX clock) gives us an RGMII TX clock of
124999850Hz (which is only 150Hz off the requested 125MHz, compared to
1197Hz based on the MPLL2 rate set by u-boot and the Amlogic GPL kernel
sources).

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, so it's already a multiple of 250MHz and
125MHz).

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 e30ad05d6a6e..b64a5351c665 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_tx_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 v4 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 125MHz using the m250_div clock. Currently the common clock
framework chooses a m250_div of 2 - with the internal fixed
"divide by 10" this results in a RGMII TX clock of 125001197Hz (120Hz
above the requested 125MHz).

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 2 (which is an internal,
fixed divider for the RGMII TX clock) gives us an RGMII TX clock of
124999850Hz (which is only 150Hz off the requested 125MHz, compared to
1197Hz based on the MPLL2 rate set by u-boot and the Amlogic GPL kernel
sources).

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, so it's already a multiple of 250MHz and
125MHz).

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 e30ad05d6a6e..b64a5351c665 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_tx_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 v4 0/5] dwmac-meson8b: clock fixes for Meson8b

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 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 TX clock on Meson8b
>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 124 +++++++++++----------
>  1 file changed, 68 insertions(+), 56 deletions(-)

Ethernet works with this series applied (and tweaked DT patches from Emiliano)

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 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 TX clock on Meson8b
>   net: stmmac: dwmac-meson8b: propagate rate changes to the parent clock
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 124 +++++++++++----------
>  1 file changed, 68 insertions(+), 56 deletions(-)

Ethernet works with this series applied (and tweaked DT patches from Emiliano)

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 

[...]

> @@ -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;
> -	}
> -

I would set the rate first then enable. Less chances of glitches and no need to
call clk_disable_unprepare if it fails

>  	/* 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_tx_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);

if you use
devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
			 YOUR-CLK)

after enabling the clock, your can discard these conditional hunks.

>  
>  	return stmmac_pltfr_remove(pdev);
>  }

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 

[...]

> @@ -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;
> -	}
> -

I would set the rate first then enable. Less chances of glitches and no need to
call clk_disable_unprepare if it fails

>  	/* 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_tx_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);

if you use
devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
?			 YOUR-CLK)

after enabling the clock, your can discard these conditional hunks.

>  
>  	return stmmac_pltfr_remove(pdev);
>  }

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 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.

CCF copies the name from the init_data to its own structures, so the init_data
are useless after the register.

Here you'd allocate memory for each string which will remain until the driver
unload. It's not much, but still, it is wasted memory.

> 
> 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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);

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> 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.

CCF copies the name from the init_data to its own structures, so the init_data
are useless after the register.

Here you'd allocate memory for each string which will remain until the driver
unload. It's not much, but still, it is wasted memory.

> 
> 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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);

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
> - bit 4 is a mux to choose between two parent clocks. according to the
>   public S805 datasheet the only supported parent clock is MPLL2 (this
>   was not verified using the oscilloscope).
>   The public S805/S905 datasheet claims that this bit is reserved.
> - bits 9:7 control a one-based divider (register value 1 means "divide
>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>   clock because it's value is always supposed to be (close to) 250MHz
>   (see below for an explanation).
>   The description in the public S805/S905 datasheet is a bit cryptic,
>   but it comes down to "input clock = 250MHz * value" (which could also
>   be expressed as "250MHz = input clock / value")
> - there seems to be an internal fixed divide-by-2 clock which takes the
>   output from the m250_div and divides it by 2. This is not unusual on
>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>   divide-by-2 clock.
>   This is not documented in the public S805/S905 datasheet
> - bit 10 controls a gate clock which enables or disables the RGMII TX
>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>   TX clock output is close to 0
>   The description for this bit in the public S805/S905 datasheet is
>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>   that this is wrong, and should probably read "Generate the 125MHz
>   RGMII TX clock for the PHY"
> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>   output (automatically) depending on the line speed (RGMII specifies
>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>   that this clock always has to be set to 125MHz and due to the fixed
>   divide-by-2 parent clock this means that m250_div will always end up
>   with a rate of (close to) 250MHz.
> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>   of Amlogic's older GPL kernel sources.
> 
> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
> Tests with the oscilloscope have shown that this is routed to a crystal
> right next to the RTL8211F PHY. The same seems to be true on the Khadas
> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
> other side of the PCB there.
> 
> This updates the clocks in the dwmac-meson8b driver by replacing the
> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
> Now we also need to set a frequency of 125MHz on the RGMII clock
> (opposed to the 25MHz we set before, with that non-existing
> divide-by-5-or-10 divider).
> 
> Special thanks go to Linus Lüssing for testing the various bits and
> checking the results with an oscilloscope on his Odroid-C1!
> 
> 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>

Looks Good
Acked-by: Jerome Brunet <jbrunet@baylibre.com>

Not related to this particular change but we still need to re-factor the clock
registration of this driver. We carry a lot of useless pointers in our private
data. I guess this is something for another day.

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
> - bit 4 is a mux to choose between two parent clocks. according to the
>   public S805 datasheet the only supported parent clock is MPLL2 (this
>   was not verified using the oscilloscope).
>   The public S805/S905 datasheet claims that this bit is reserved.
> - bits 9:7 control a one-based divider (register value 1 means "divide
>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>   clock because it's value is always supposed to be (close to) 250MHz
>   (see below for an explanation).
>   The description in the public S805/S905 datasheet is a bit cryptic,
>   but it comes down to "input clock = 250MHz * value" (which could also
>   be expressed as "250MHz = input clock / value")
> - there seems to be an internal fixed divide-by-2 clock which takes the
>   output from the m250_div and divides it by 2. This is not unusual on
>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>   divide-by-2 clock.
>   This is not documented in the public S805/S905 datasheet
> - bit 10 controls a gate clock which enables or disables the RGMII TX
>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>   TX clock output is close to 0
>   The description for this bit in the public S805/S905 datasheet is
>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>   that this is wrong, and should probably read "Generate the 125MHz
>   RGMII TX clock for the PHY"
> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>   output (automatically) depending on the line speed (RGMII specifies
>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>   that this clock always has to be set to 125MHz and due to the fixed
>   divide-by-2 parent clock this means that m250_div will always end up
>   with a rate of (close to) 250MHz.
> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>   of Amlogic's older GPL kernel sources.
> 
> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
> Tests with the oscilloscope have shown that this is routed to a crystal
> right next to the RTL8211F PHY. The same seems to be true on the Khadas
> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
> other side of the PCB there.
> 
> This updates the clocks in the dwmac-meson8b driver by replacing the
> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
> Now we also need to set a frequency of 125MHz on the RGMII clock
> (opposed to the 25MHz we set before, with that non-existing
> divide-by-5-or-10 divider).
> 
> Special thanks go to Linus L?ssing for testing the various bits and
> checking the results with an oscilloscope on his Odroid-C1!
> 
> 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>

Looks Good
Acked-by: Jerome Brunet <jbrunet@baylibre.com>

Not related to this particular change but we still need to re-factor the clock
registration of this driver. We carry a lot of useless pointers in our private
data. I guess this is something for another day.

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> On Meson8b the only valid input clock is MPLL2. The bootloader
> configures that to run at 500002394Hz which cannot be divided evenly
> down to 125MHz using the m250_div clock. Currently the common clock
> framework chooses a m250_div of 2 - with the internal fixed
> "divide by 10" this results in a RGMII TX clock of 125001197Hz (120Hz
> above the requested 125MHz).
> 
> 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 2 (which is an internal,
> fixed divider for the RGMII TX clock) gives us an RGMII TX clock of
> 124999850Hz (which is only 150Hz off the requested 125MHz, compared to
> 1197Hz based on the MPLL2 rate set by u-boot and the Amlogic GPL kernel
> sources).
> 
> 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, so it's already a multiple of 250MHz and
> 125MHz).
> 
> 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>

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

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

[Jerome Brunet]

On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
> On Meson8b the only valid input clock is MPLL2. The bootloader
> configures that to run at 500002394Hz which cannot be divided evenly
> down to 125MHz using the m250_div clock. Currently the common clock
> framework chooses a m250_div of 2 - with the internal fixed
> "divide by 10" this results in a RGMII TX clock of 125001197Hz (120Hz
> above the requested 125MHz).
> 
> 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 2 (which is an internal,
> fixed divider for the RGMII TX clock) gives us an RGMII TX clock of
> 124999850Hz (which is only 150Hz off the requested 125MHz, compared to
> 1197Hz based on the MPLL2 rate set by u-boot and the Amlogic GPL kernel
> sources).
> 
> 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, so it's already a multiple of 250MHz and
> 125MHz).
> 
> 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>

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:46 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>> 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.
>
> CCF copies the name from the init_data to its own structures, so the init_data
> are useless after the register.
>
> Here you'd allocate memory for each string which will remain until the driver
> unload. It's not much, but still, it is wasted memory.
good catch, thank you!
maybe I should drop this patch for now and clean up the clock
registration in another series - I can try to get rid of the
"unneeded" members in struct meson8b_dwmac in a new patch. what do you
think?

>>
>> 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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);
>

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:46 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>> 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.
>
> CCF copies the name from the init_data to its own structures, so the init_data
> are useless after the register.
>
> Here you'd allocate memory for each string which will remain until the driver
> unload. It's not much, but still, it is wasted memory.
good catch, thank you!
maybe I should drop this patch for now and clean up the clock
registration in another series - I can try to get rid of the
"unneeded" members in struct meson8b_dwmac in a new patch. what do you
think?

>>
>> 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 c6f87e9c4ccb..670f344f7168 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_tx_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_tx_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_tx_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_tx_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);
>

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:46 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>>
>
> [...]
>
>> @@ -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;
>> -     }
>> -
>
> I would set the rate first then enable. Less chances of glitches and no need to
> call clk_disable_unprepare if it fails
I did swap the calls in PATCH #3 of this series
with this patch I wanted to make sure that all of the current logic is
only executed in RGMII mode

>>       /* 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_tx_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);
>
> if you use
> devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
>                         YOUR-CLK)
>
> after enabling the clock, your can discard these conditional hunks.
noted, I'll also make this part of the clock cleanup series

>>
>>       return stmmac_pltfr_remove(pdev);
>>  }
>

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:46 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>>
>
> [...]
>
>> @@ -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;
>> -     }
>> -
>
> I would set the rate first then enable. Less chances of glitches and no need to
> call clk_disable_unprepare if it fails
I did swap the calls in PATCH #3 of this series
with this patch I wanted to make sure that all of the current logic is
only executed in RGMII mode

>>       /* 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_tx_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);
>
> if you use
> devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
> ?                        YOUR-CLK)
>
> after enabling the clock, your can discard these conditional hunks.
noted, I'll also make this part of the clock cleanup series

>>
>>       return stmmac_pltfr_remove(pdev);
>>  }
>

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:49 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
>> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
>> - bit 4 is a mux to choose between two parent clocks. according to the
>>   public S805 datasheet the only supported parent clock is MPLL2 (this
>>   was not verified using the oscilloscope).
>>   The public S805/S905 datasheet claims that this bit is reserved.
>> - bits 9:7 control a one-based divider (register value 1 means "divide
>>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>>   clock because it's value is always supposed to be (close to) 250MHz
>>   (see below for an explanation).
>>   The description in the public S805/S905 datasheet is a bit cryptic,
>>   but it comes down to "input clock = 250MHz * value" (which could also
>>   be expressed as "250MHz = input clock / value")
>> - there seems to be an internal fixed divide-by-2 clock which takes the
>>   output from the m250_div and divides it by 2. This is not unusual on
>>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>>   divide-by-2 clock.
>>   This is not documented in the public S805/S905 datasheet
>> - bit 10 controls a gate clock which enables or disables the RGMII TX
>>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>>   TX clock output is close to 0
>>   The description for this bit in the public S805/S905 datasheet is
>>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>>   that this is wrong, and should probably read "Generate the 125MHz
>>   RGMII TX clock for the PHY"
>> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>>   output (automatically) depending on the line speed (RGMII specifies
>>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>>   that this clock always has to be set to 125MHz and due to the fixed
>>   divide-by-2 parent clock this means that m250_div will always end up
>>   with a rate of (close to) 250MHz.
>> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>>   of Amlogic's older GPL kernel sources.
>>
>> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
>> Tests with the oscilloscope have shown that this is routed to a crystal
>> right next to the RTL8211F PHY. The same seems to be true on the Khadas
>> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
>> other side of the PCB there.
>>
>> This updates the clocks in the dwmac-meson8b driver by replacing the
>> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
>> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
>> Now we also need to set a frequency of 125MHz on the RGMII clock
>> (opposed to the 25MHz we set before, with that non-existing
>> divide-by-5-or-10 divider).
>>
>> Special thanks go to Linus Lüssing for testing the various bits and
>> checking the results with an oscilloscope on his Odroid-C1!
>>
>> 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>
>
> Looks Good
> Acked-by: Jerome Brunet <jbrunet@baylibre.com>
thank you!

> Not related to this particular change but we still need to re-factor the clock
> registration of this driver. We carry a lot of useless pointers in our private
> data. I guess this is something for another day.
can you share your thoughts how to do this?
I can devm_kzalloc the memory for struct clk_mux, clk_divider and
clk_fixed_factor in the function which registers these clocks. but I
cannot declare them on the stack, because the clk-* implementations
still need it during runtime
this would leave us with only the struct clk instances in meson8_dwmac


Regards
Martin

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

[Martin Blumenstingl]

Hi Jerome,

On Mon, Jan 15, 2018 at 12:49 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
>> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
>> - bit 4 is a mux to choose between two parent clocks. according to the
>>   public S805 datasheet the only supported parent clock is MPLL2 (this
>>   was not verified using the oscilloscope).
>>   The public S805/S905 datasheet claims that this bit is reserved.
>> - bits 9:7 control a one-based divider (register value 1 means "divide
>>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>>   clock because it's value is always supposed to be (close to) 250MHz
>>   (see below for an explanation).
>>   The description in the public S805/S905 datasheet is a bit cryptic,
>>   but it comes down to "input clock = 250MHz * value" (which could also
>>   be expressed as "250MHz = input clock / value")
>> - there seems to be an internal fixed divide-by-2 clock which takes the
>>   output from the m250_div and divides it by 2. This is not unusual on
>>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>>   divide-by-2 clock.
>>   This is not documented in the public S805/S905 datasheet
>> - bit 10 controls a gate clock which enables or disables the RGMII TX
>>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>>   TX clock output is close to 0
>>   The description for this bit in the public S805/S905 datasheet is
>>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>>   that this is wrong, and should probably read "Generate the 125MHz
>>   RGMII TX clock for the PHY"
>> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>>   output (automatically) depending on the line speed (RGMII specifies
>>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>>   that this clock always has to be set to 125MHz and due to the fixed
>>   divide-by-2 parent clock this means that m250_div will always end up
>>   with a rate of (close to) 250MHz.
>> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>>   of Amlogic's older GPL kernel sources.
>>
>> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
>> Tests with the oscilloscope have shown that this is routed to a crystal
>> right next to the RTL8211F PHY. The same seems to be true on the Khadas
>> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
>> other side of the PCB there.
>>
>> This updates the clocks in the dwmac-meson8b driver by replacing the
>> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
>> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
>> Now we also need to set a frequency of 125MHz on the RGMII clock
>> (opposed to the 25MHz we set before, with that non-existing
>> divide-by-5-or-10 divider).
>>
>> Special thanks go to Linus L?ssing for testing the various bits and
>> checking the results with an oscilloscope on his Odroid-C1!
>>
>> 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>
>
> Looks Good
> Acked-by: Jerome Brunet <jbrunet@baylibre.com>
thank you!

> Not related to this particular change but we still need to re-factor the clock
> registration of this driver. We carry a lot of useless pointers in our private
> data. I guess this is something for another day.
can you share your thoughts how to do this?
I can devm_kzalloc the memory for struct clk_mux, clk_divider and
clk_fixed_factor in the function which registers these clocks. but I
cannot declare them on the stack, because the clk-* implementations
still need it during runtime
this would leave us with only the struct clk instances in meson8_dwmac


Regards
Martin

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

[Jerome Brunet]

On Mon, 2018-01-15 at 13:02 +0100, Martin Blumenstingl wrote:
> > Here you'd allocate memory for each string which will remain until the driver
> > unload. It's not much, but still, it is wasted memory.
> 
> good catch, thank you!
> maybe I should drop this patch for now and clean up the clock
> registration in another series - I can try to get rid of the
> "unneeded" members in struct meson8b_dwmac in a new patch. what do you
> think?

I think it would be wise, yes.

Cheers
Jerome

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

[Jerome Brunet]

On Mon, 2018-01-15 at 13:02 +0100, Martin Blumenstingl wrote:
> > Here you'd allocate memory for each string which will remain until the driver
> > unload. It's not much, but still, it is wasted memory.
> 
> good catch, thank you!
> maybe I should drop this patch for now and clean up the clock
> registration in another series - I can try to get rid of the
> "unneeded" members in struct meson8b_dwmac in a new patch. what do you
> think?

I think it would be wise, yes.

Cheers
Jerome

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

[Jerome Brunet]

On Mon, 2018-01-15 at 13:08 +0100, Martin Blumenstingl wrote:
> can you share your thoughts how to do this?
> I can devm_kzalloc the memory for struct clk_mux, clk_divider and
> clk_fixed_factor in the function which registers these clocks. but I
> cannot declare them on the stack, because the clk-* implementations
> still need it during runtime

You can declare the init_data on the stack, CCF makes a copy of what it needs.
For clk_mux, clk_gate and friends, yes, use devm functions is the way to go

For the struct *clk, you can also use devm. Just keep a reference on the leaf
clock to be able to call set_rate() and prepare_enable(). You don't need the
rest.

> this would leave us with only the struct clk instances in meson8_dwmac

You may have a look at drivers/mmc/host/meson-gx-mmc.c. The clock scheme of this
IP is actually very clock the ethernet one.

Cheers

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

[Jerome Brunet]

On Mon, 2018-01-15 at 13:08 +0100, Martin Blumenstingl wrote:
> can you share your thoughts how to do this?
> I can devm_kzalloc the memory for struct clk_mux, clk_divider and
> clk_fixed_factor in the function which registers these clocks. but I
> cannot declare them on the stack, because the clk-* implementations
> still need it during runtime

You can declare the init_data on the stack, CCF makes a copy of what it needs.
For clk_mux, clk_gate and friends, yes, use devm functions is the way to go

For the struct *clk, you can also use devm. Just keep a reference on the leaf
clock to be able to call set_rate() and prepare_enable(). You don't need the
rest.

> this would leave us with only the struct clk instances in meson8_dwmac

You may have a look at drivers/mmc/host/meson-gx-mmc.c. The clock scheme of this
IP is actually very clock the ethernet one.

Cheers

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

[Martin Blumenstingl]

On Sun, Jan 14, 2018 at 10:48 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
> - bit 4 is a mux to choose between two parent clocks. according to the
>   public S805 datasheet the only supported parent clock is MPLL2 (this
>   was not verified using the oscilloscope).
>   The public S805/S905 datasheet claims that this bit is reserved.
> - bits 9:7 control a one-based divider (register value 1 means "divide
>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>   clock because it's value is always supposed to be (close to) 250MHz
>   (see below for an explanation).
>   The description in the public S805/S905 datasheet is a bit cryptic,
>   but it comes down to "input clock = 250MHz * value" (which could also
>   be expressed as "250MHz = input clock / value")
> - there seems to be an internal fixed divide-by-2 clock which takes the
>   output from the m250_div and divides it by 2. This is not unusual on
>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>   divide-by-2 clock.
>   This is not documented in the public S805/S905 datasheet
> - bit 10 controls a gate clock which enables or disables the RGMII TX
>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>   TX clock output is close to 0
>   The description for this bit in the public S805/S905 datasheet is
>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>   that this is wrong, and should probably read "Generate the 125MHz
>   RGMII TX clock for the PHY"
> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>   output (automatically) depending on the line speed (RGMII specifies
>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>   that this clock always has to be set to 125MHz and due to the fixed
>   divide-by-2 parent clock this means that m250_div will always end up
>   with a rate of (close to) 250MHz.
> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>   of Amlogic's older GPL kernel sources.
>
> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
> Tests with the oscilloscope have shown that this is routed to a crystal
> right next to the RTL8211F PHY. The same seems to be true on the Khadas
> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
> other side of the PCB there.
>
> This updates the clocks in the dwmac-meson8b driver by replacing the
> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
> Now we also need to set a frequency of 125MHz on the RGMII clock
> (opposed to the 25MHz we set before, with that non-existing
> divide-by-5-or-10 divider).
>
> Special thanks go to Linus Lüssing for testing the various bits and
> checking the results with an oscilloscope on his Odroid-C1!
>
> 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>
NACK-ed by: Yixun Lan <yixun.lan@amlogic.com>

as it breaks the AXG SoCs (maybe not even directly related to this
patch, but we're currently not sure if m250_mux is defined correctly)
see: [0]

> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 79 +++++++++++++---------
>  1 file changed, 47 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> index 670f344f7168..e9fec9e0425c 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_RGMII_TX_CLK_EN       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_div2;
> +       struct clk              *fixed_div2_clk;
> +
> +       struct clk_gate         rgmii_tx_en;
> +       struct clk              *rgmii_tx_en_clk;
>
>         u32                     tx_delay_ns;
>  };
> @@ -88,11 +89,6 @@ static int meson8b_init_rgmii_tx_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,40 @@ static int meson8b_init_rgmii_tx_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_div2 */
> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div2",
>                                    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_div2.mult = 1;
> +       dwmac->fixed_div2.div = 2;
> +       dwmac->fixed_div2.hw.init = &init;
> +
> +       dwmac->fixed_div2_clk = devm_clk_register(dev, &dwmac->fixed_div2.hw);
> +       if (WARN_ON(IS_ERR(dwmac->fixed_div2_clk)))
> +               return PTR_ERR(dwmac->fixed_div2_clk);
> +
> +       /* create the rgmii_tx_en */
> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#rgmii_tx_en",
> +                                  dev_name(dev));
> +       init.ops = &clk_gate_ops;
> +       init.flags = CLK_SET_RATE_PARENT;
> +       clk_div_parents[0] = __clk_get_name(dwmac->fixed_div2_clk);
> +       init.parent_names = clk_div_parents;
> +       init.num_parents = ARRAY_SIZE(clk_div_parents);
> +
> +       dwmac->rgmii_tx_en.reg = dwmac->regs + PRG_ETH0;
> +       dwmac->rgmii_tx_en.bit_idx = PRG_ETH0_RGMII_TX_CLK_EN;
> +       dwmac->rgmii_tx_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->rgmii_tx_en_clk = devm_clk_register(dev,
> +                                                  &dwmac->rgmii_tx_en.hw);
> +       if (WARN_ON(IS_ERR(dwmac->rgmii_tx_en_clk)))
> +               return PTR_ERR(dwmac->rgmii_tx_en_clk);
>
>         return 0;
>  }
> @@ -200,18 +211,22 @@ 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);
> +               /* Configure the 125MHz RGMII TX clock, the IP block changes
> +                * the output automatically (= without us having to configure
> +                * a register) based on the line-speed (125MHz for Gbit speeds,
> +                * 25MHz for 100Mbit/s and 2.5MHz for 10Mbit/s).
> +                */
> +               ret = clk_set_rate(dwmac->rgmii_tx_en_clk, 125 * 1000 * 1000);
>                 if (ret) {
> -                       dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
> +                       dev_err(&dwmac->pdev->dev,
> +                               "failed to set RGMII TX 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_prepare_enable(dwmac->rgmii_tx_en_clk);
>                 if (ret) {
> -                       clk_disable_unprepare(dwmac->m25_div_clk);
> -
> -                       dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
> +                       dev_err(&dwmac->pdev->dev,
> +                               "failed to enable the RGMII TX clock\n");
>                         return ret;
>                 }
>                 break;
> @@ -305,7 +320,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->rgmii_tx_en_clk);
>  err_remove_config_dt:
>         stmmac_remove_config_dt(pdev, plat_dat);
>
> @@ -317,7 +332,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->rgmii_tx_en_clk);
>
>         return stmmac_pltfr_remove(pdev);
>  }
> --
> 2.15.1
>

[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006153.html

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

[Martin Blumenstingl]

On Tue, Jan 16, 2018 at 12:20 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Sun, Jan 14, 2018 at 10:48 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
>> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
>> - bit 4 is a mux to choose between two parent clocks. according to the
>>   public S805 datasheet the only supported parent clock is MPLL2 (this
>>   was not verified using the oscilloscope).
>>   The public S805/S905 datasheet claims that this bit is reserved.
>> - bits 9:7 control a one-based divider (register value 1 means "divide
>>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>>   clock because it's value is always supposed to be (close to) 250MHz
>>   (see below for an explanation).
>>   The description in the public S805/S905 datasheet is a bit cryptic,
>>   but it comes down to "input clock = 250MHz * value" (which could also
>>   be expressed as "250MHz = input clock / value")
>> - there seems to be an internal fixed divide-by-2 clock which takes the
>>   output from the m250_div and divides it by 2. This is not unusual on
>>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>>   divide-by-2 clock.
>>   This is not documented in the public S805/S905 datasheet
>> - bit 10 controls a gate clock which enables or disables the RGMII TX
>>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>>   TX clock output is close to 0
>>   The description for this bit in the public S805/S905 datasheet is
>>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>>   that this is wrong, and should probably read "Generate the 125MHz
>>   RGMII TX clock for the PHY"
>> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>>   output (automatically) depending on the line speed (RGMII specifies
>>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>>   that this clock always has to be set to 125MHz and due to the fixed
>>   divide-by-2 parent clock this means that m250_div will always end up
>>   with a rate of (close to) 250MHz.
>> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>>   of Amlogic's older GPL kernel sources.
>>
>> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
>> Tests with the oscilloscope have shown that this is routed to a crystal
>> right next to the RTL8211F PHY. The same seems to be true on the Khadas
>> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
>> other side of the PCB there.
>>
>> This updates the clocks in the dwmac-meson8b driver by replacing the
>> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
>> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
>> Now we also need to set a frequency of 125MHz on the RGMII clock
>> (opposed to the 25MHz we set before, with that non-existing
>> divide-by-5-or-10 divider).
>>
>> Special thanks go to Linus Lüssing for testing the various bits and
>> checking the results with an oscilloscope on his Odroid-C1!
>>
>> 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>
> NACK-ed by: Yixun Lan <yixun.lan@amlogic.com>
>
> as it breaks the AXG SoCs (maybe not even directly related to this
> patch, but we're currently not sure if m250_mux is defined correctly)
> see: [0]
for the record (as this series is now merged): it turned out that the
breakage (of this series) on the AXG SoC is a side-effect of at least
two problems in the clock controller driver - these should be fixed
with: [0]

>> ---
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 79 +++++++++++++---------
>>  1 file changed, 47 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> index 670f344f7168..e9fec9e0425c 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_RGMII_TX_CLK_EN       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_div2;
>> +       struct clk              *fixed_div2_clk;
>> +
>> +       struct clk_gate         rgmii_tx_en;
>> +       struct clk              *rgmii_tx_en_clk;
>>
>>         u32                     tx_delay_ns;
>>  };
>> @@ -88,11 +89,6 @@ static int meson8b_init_rgmii_tx_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,40 @@ static int meson8b_init_rgmii_tx_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_div2 */
>> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div2",
>>                                    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_div2.mult = 1;
>> +       dwmac->fixed_div2.div = 2;
>> +       dwmac->fixed_div2.hw.init = &init;
>> +
>> +       dwmac->fixed_div2_clk = devm_clk_register(dev, &dwmac->fixed_div2.hw);
>> +       if (WARN_ON(IS_ERR(dwmac->fixed_div2_clk)))
>> +               return PTR_ERR(dwmac->fixed_div2_clk);
>> +
>> +       /* create the rgmii_tx_en */
>> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#rgmii_tx_en",
>> +                                  dev_name(dev));
>> +       init.ops = &clk_gate_ops;
>> +       init.flags = CLK_SET_RATE_PARENT;
>> +       clk_div_parents[0] = __clk_get_name(dwmac->fixed_div2_clk);
>> +       init.parent_names = clk_div_parents;
>> +       init.num_parents = ARRAY_SIZE(clk_div_parents);
>> +
>> +       dwmac->rgmii_tx_en.reg = dwmac->regs + PRG_ETH0;
>> +       dwmac->rgmii_tx_en.bit_idx = PRG_ETH0_RGMII_TX_CLK_EN;
>> +       dwmac->rgmii_tx_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->rgmii_tx_en_clk = devm_clk_register(dev,
>> +                                                  &dwmac->rgmii_tx_en.hw);
>> +       if (WARN_ON(IS_ERR(dwmac->rgmii_tx_en_clk)))
>> +               return PTR_ERR(dwmac->rgmii_tx_en_clk);
>>
>>         return 0;
>>  }
>> @@ -200,18 +211,22 @@ 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);
>> +               /* Configure the 125MHz RGMII TX clock, the IP block changes
>> +                * the output automatically (= without us having to configure
>> +                * a register) based on the line-speed (125MHz for Gbit speeds,
>> +                * 25MHz for 100Mbit/s and 2.5MHz for 10Mbit/s).
>> +                */
>> +               ret = clk_set_rate(dwmac->rgmii_tx_en_clk, 125 * 1000 * 1000);
>>                 if (ret) {
>> -                       dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
>> +                       dev_err(&dwmac->pdev->dev,
>> +                               "failed to set RGMII TX 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_prepare_enable(dwmac->rgmii_tx_en_clk);
>>                 if (ret) {
>> -                       clk_disable_unprepare(dwmac->m25_div_clk);
>> -
>> -                       dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
>> +                       dev_err(&dwmac->pdev->dev,
>> +                               "failed to enable the RGMII TX clock\n");
>>                         return ret;
>>                 }
>>                 break;
>> @@ -305,7 +320,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->rgmii_tx_en_clk);
>>  err_remove_config_dt:
>>         stmmac_remove_config_dt(pdev, plat_dat);
>>
>> @@ -317,7 +332,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->rgmii_tx_en_clk);
>>
>>         return stmmac_pltfr_remove(pdev);
>>  }
>> --
>> 2.15.1
>>
>
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006153.html


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006204.html

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

[Martin Blumenstingl]

On Tue, Jan 16, 2018 at 12:20 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Sun, Jan 14, 2018 at 10:48 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> Tests (using an oscilloscope and an Odroid-C1 board with a RTL8211F
>> RGMII PHY) have shown that the PRG_ETH0 register behaves as follows:
>> - bit 4 is a mux to choose between two parent clocks. according to the
>>   public S805 datasheet the only supported parent clock is MPLL2 (this
>>   was not verified using the oscilloscope).
>>   The public S805/S905 datasheet claims that this bit is reserved.
>> - bits 9:7 control a one-based divider (register value 1 means "divide
>>   by 1", etc.) for the input clock. we call this clock the "m250_div"
>>   clock because it's value is always supposed to be (close to) 250MHz
>>   (see below for an explanation).
>>   The description in the public S805/S905 datasheet is a bit cryptic,
>>   but it comes down to "input clock = 250MHz * value" (which could also
>>   be expressed as "250MHz = input clock / value")
>> - there seems to be an internal fixed divide-by-2 clock which takes the
>>   output from the m250_div and divides it by 2. This is not unusual on
>>   Amlogic SoCs, since the SDIO (MMC) driver also uses an internal fixed
>>   divide-by-2 clock.
>>   This is not documented in the public S805/S905 datasheet
>> - bit 10 controls a gate clock which enables or disables the RGMII TX
>>   clock (which is an output on the MAC/SoC and an input in the PHY). we
>>   call this the "rgmii_tx_en" clock. if this bit is set to "0" the RGMII
>>   TX clock output is close to 0
>>   The description for this bit in the public S805/S905 datasheet is
>>   "Generate 25MHz clock for PHY". Based on these tests it's believed
>>   that this is wrong, and should probably read "Generate the 125MHz
>>   RGMII TX clock for the PHY"
>> - the RGMII TX clock has to be set to 125MHz - the IP block adjusts the
>>   output (automatically) depending on the line speed (RGMII specifies
>>   that Gbit connections use a 125MHz clock, 100Mbit/s connections use a
>>   25MHz clock and 10Mbit/s connections use a 2.5MHz clock. only Gbit and
>>   100Mbit/s were tested with an oscilloscope). Due to the requirement
>>   that this clock always has to be set to 125MHz and due to the fixed
>>   divide-by-2 parent clock this means that m250_div will always end up
>>   with a rate of (close to) 250MHz.
>> - bits 6:5 are the TX delay, which is also named "clock phase" in some
>>   of Amlogic's older GPL kernel sources.
>>
>> The PHY also has an XTAL_IN pin where a 25MHz clock has to be provided.
>> Tests with the oscilloscope have shown that this is routed to a crystal
>> right next to the RTL8211F PHY. The same seems to be true on the Khadas
>> VIM2 (which uses a GXM SoC) board - however the 25MHz crystal is on the
>> other side of the PCB there.
>>
>> This updates the clocks in the dwmac-meson8b driver by replacing the
>> "m25_div" with the "rgmii_tx_en" clock and additionally introducing a
>> fixed divide-by-2 clock between "m250_div" and "rgmii_tx_en".
>> Now we also need to set a frequency of 125MHz on the RGMII clock
>> (opposed to the 25MHz we set before, with that non-existing
>> divide-by-5-or-10 divider).
>>
>> Special thanks go to Linus L?ssing for testing the various bits and
>> checking the results with an oscilloscope on his Odroid-C1!
>>
>> 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>
> NACK-ed by: Yixun Lan <yixun.lan@amlogic.com>
>
> as it breaks the AXG SoCs (maybe not even directly related to this
> patch, but we're currently not sure if m250_mux is defined correctly)
> see: [0]
for the record (as this series is now merged): it turned out that the
breakage (of this series) on the AXG SoC is a side-effect of at least
two problems in the clock controller driver - these should be fixed
with: [0]

>> ---
>>  .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    | 79 +++++++++++++---------
>>  1 file changed, 47 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>> index 670f344f7168..e9fec9e0425c 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_RGMII_TX_CLK_EN       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_div2;
>> +       struct clk              *fixed_div2_clk;
>> +
>> +       struct clk_gate         rgmii_tx_en;
>> +       struct clk              *rgmii_tx_en_clk;
>>
>>         u32                     tx_delay_ns;
>>  };
>> @@ -88,11 +89,6 @@ static int meson8b_init_rgmii_tx_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,40 @@ static int meson8b_init_rgmii_tx_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_div2 */
>> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#fixed_div2",
>>                                    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_div2.mult = 1;
>> +       dwmac->fixed_div2.div = 2;
>> +       dwmac->fixed_div2.hw.init = &init;
>> +
>> +       dwmac->fixed_div2_clk = devm_clk_register(dev, &dwmac->fixed_div2.hw);
>> +       if (WARN_ON(IS_ERR(dwmac->fixed_div2_clk)))
>> +               return PTR_ERR(dwmac->fixed_div2_clk);
>> +
>> +       /* create the rgmii_tx_en */
>> +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#rgmii_tx_en",
>> +                                  dev_name(dev));
>> +       init.ops = &clk_gate_ops;
>> +       init.flags = CLK_SET_RATE_PARENT;
>> +       clk_div_parents[0] = __clk_get_name(dwmac->fixed_div2_clk);
>> +       init.parent_names = clk_div_parents;
>> +       init.num_parents = ARRAY_SIZE(clk_div_parents);
>> +
>> +       dwmac->rgmii_tx_en.reg = dwmac->regs + PRG_ETH0;
>> +       dwmac->rgmii_tx_en.bit_idx = PRG_ETH0_RGMII_TX_CLK_EN;
>> +       dwmac->rgmii_tx_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->rgmii_tx_en_clk = devm_clk_register(dev,
>> +                                                  &dwmac->rgmii_tx_en.hw);
>> +       if (WARN_ON(IS_ERR(dwmac->rgmii_tx_en_clk)))
>> +               return PTR_ERR(dwmac->rgmii_tx_en_clk);
>>
>>         return 0;
>>  }
>> @@ -200,18 +211,22 @@ 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);
>> +               /* Configure the 125MHz RGMII TX clock, the IP block changes
>> +                * the output automatically (= without us having to configure
>> +                * a register) based on the line-speed (125MHz for Gbit speeds,
>> +                * 25MHz for 100Mbit/s and 2.5MHz for 10Mbit/s).
>> +                */
>> +               ret = clk_set_rate(dwmac->rgmii_tx_en_clk, 125 * 1000 * 1000);
>>                 if (ret) {
>> -                       dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
>> +                       dev_err(&dwmac->pdev->dev,
>> +                               "failed to set RGMII TX 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_prepare_enable(dwmac->rgmii_tx_en_clk);
>>                 if (ret) {
>> -                       clk_disable_unprepare(dwmac->m25_div_clk);
>> -
>> -                       dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
>> +                       dev_err(&dwmac->pdev->dev,
>> +                               "failed to enable the RGMII TX clock\n");
>>                         return ret;
>>                 }
>>                 break;
>> @@ -305,7 +320,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->rgmii_tx_en_clk);
>>  err_remove_config_dt:
>>         stmmac_remove_config_dt(pdev, plat_dat);
>>
>> @@ -317,7 +332,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->rgmii_tx_en_clk);
>>
>>         return stmmac_pltfr_remove(pdev);
>>  }
>> --
>> 2.15.1
>>
>
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006153.html


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006204.html