[PATCH net v2] net: stmmac: thead: Enable TX clock before MAC initialization

[PATCH net v2] net: stmmac: thead: Enable TX clock before MAC initialization

[Yao Zi]

The clk_tx_i clock must be supplied to the MAC for successful
initialization. On TH1520 SoC, the clock is provided by an internal
divider configured through GMAC_PLLCLK_DIV register when using RGMII
interface. However, currently we don't setup the divider before
initialization of the MAC, resulting in DMA reset failures if the
bootloader/firmware doesn't enable the divider,

[    7.839601] thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
[    7.938338] thead-dwmac ffe7060000.ethernet eth0: PHY [stmmac-0:02] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
[    8.160746] thead-dwmac ffe7060000.ethernet eth0: Failed to reset the dma
[    8.170118] thead-dwmac ffe7060000.ethernet eth0: stmmac_hw_setup: DMA engine initialization failed
[    8.179384] thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Hw setup failed

Let's simply write GMAC_PLLCLK_DIV_EN to GMAC_PLLCLK_DIV to enable the
divider before MAC initialization. The exact rate doesn't affect MAC's
initialization according to my test. It's set to the speed required by
RGMII when the linkspeed is 1Gbps and could be reclocked later after
link is up if necessary.

Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
Signed-off-by: Yao Zi <ziyao@disroot.org>
---

Note that the DMA reset failures cannot be reproduced with the vendor
U-Boot, which always enables the divider, regardless whether the port is
used[1].

As this scheme (enables the divider first and reclock it later) requires
access to the APB glue registers, the patch depends on v3 of series
"Fix broken link with TH1520 GMAC when linkspeed changes"[2] to ensure
the APB bus clock is ungated.

[1]: https://github.com/revyos/thead-u-boot/blob/93ff49d9f5bbe7942f727ab93311346173506d27/board/thead/light-c910/light.c#L581-L582
[2]: https://lore.kernel.org/netdev/20250808093655.48074-2-ziyao@disroot.org/

Changed from v1
- Initialize the divisor to a well-known value (producing the clock rate
  required by RGMII link at 1Gbps)
- Write zero to GMAC_PLLCLK_DIV before writing the configuration, as
  required by the TRM
- Link to v1: https://lore.kernel.org/netdev/20250801094507.54011-1-ziyao@disroot.org/

 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
index f2946bea0bc2..50c1920bde6a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
@@ -152,7 +152,7 @@ static int thead_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
 static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
 {
 	struct thead_dwmac *dwmac = plat->bsp_priv;
-	u32 reg;
+	u32 reg, div;
 
 	switch (plat->mac_interface) {
 	case PHY_INTERFACE_MODE_MII:
@@ -164,6 +164,13 @@ static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		/* use pll */
+		div = clk_get_rate(plat->stmmac_clk) / rgmii_clock(SPEED_1000);
+		reg = FIELD_PREP(GMAC_PLLCLK_DIV_EN, 1) |
+		      FIELD_PREP(GMAC_PLLCLK_DIV_NUM, div),
+
+		writel(0, dwmac->apb_base + GMAC_PLLCLK_DIV);
+		writel(reg, dwmac->apb_base + GMAC_PLLCLK_DIV);
+
 		writel(GMAC_GTXCLK_SEL_PLL, dwmac->apb_base + GMAC_GTXCLK_SEL);
 		reg = GMAC_TX_CLK_EN | GMAC_TX_CLK_N_EN | GMAC_TX_CLK_OUT_EN |
 		      GMAC_RX_CLK_EN | GMAC_RX_CLK_N_EN;
-- 
2.50.1

Re: [PATCH net v2] net: stmmac: thead: Enable TX clock before MAC initialization

[Simon Horman]

On Fri, Aug 08, 2025 at 10:34:48AM +0000, Yao Zi wrote:
> The clk_tx_i clock must be supplied to the MAC for successful
> initialization. On TH1520 SoC, the clock is provided by an internal
> divider configured through GMAC_PLLCLK_DIV register when using RGMII
> interface. However, currently we don't setup the divider before
> initialization of the MAC, resulting in DMA reset failures if the
> bootloader/firmware doesn't enable the divider,
> 
> [    7.839601] thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
> [    7.938338] thead-dwmac ffe7060000.ethernet eth0: PHY [stmmac-0:02] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
> [    8.160746] thead-dwmac ffe7060000.ethernet eth0: Failed to reset the dma
> [    8.170118] thead-dwmac ffe7060000.ethernet eth0: stmmac_hw_setup: DMA engine initialization failed
> [    8.179384] thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Hw setup failed
> 
> Let's simply write GMAC_PLLCLK_DIV_EN to GMAC_PLLCLK_DIV to enable the
> divider before MAC initialization. The exact rate doesn't affect MAC's
> initialization according to my test. It's set to the speed required by
> RGMII when the linkspeed is 1Gbps and could be reclocked later after
> link is up if necessary.
> 
> Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> 
> Note that the DMA reset failures cannot be reproduced with the vendor
> U-Boot, which always enables the divider, regardless whether the port is
> used[1].
> 
> As this scheme (enables the divider first and reclock it later) requires
> access to the APB glue registers, the patch depends on v3 of series
> "Fix broken link with TH1520 GMAC when linkspeed changes"[2] to ensure
> the APB bus clock is ungated.
> 
> [1]: https://github.com/revyos/thead-u-boot/blob/93ff49d9f5bbe7942f727ab93311346173506d27/board/thead/light-c910/light.c#L581-L582
> [2]: https://lore.kernel.org/netdev/20250808093655.48074-2-ziyao@disroot.org/
> 
> Changed from v1
> - Initialize the divisor to a well-known value (producing the clock rate
>   required by RGMII link at 1Gbps)
> - Write zero to GMAC_PLLCLK_DIV before writing the configuration, as
>   required by the TRM

FWIIW, I think it would be worth adding something about writing zero
to the commit message.

> - Link to v1: https://lore.kernel.org/netdev/20250801094507.54011-1-ziyao@disroot.org/
> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

...

Re: [PATCH net v2] net: stmmac: thead: Enable TX clock before MAC initialization

[Simon Horman]

On Fri, Aug 08, 2025 at 10:34:48AM +0000, Yao Zi wrote:

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> index f2946bea0bc2..50c1920bde6a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> @@ -152,7 +152,7 @@ static int thead_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
>  static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
>  {
>  	struct thead_dwmac *dwmac = plat->bsp_priv;
> -	u32 reg;
> +	u32 reg, div;
>  
>  	switch (plat->mac_interface) {
>  	case PHY_INTERFACE_MODE_MII:
> @@ -164,6 +164,13 @@ static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>  		/* use pll */
> +		div = clk_get_rate(plat->stmmac_clk) / rgmii_clock(SPEED_1000);
> +		reg = FIELD_PREP(GMAC_PLLCLK_DIV_EN, 1) |
> +		      FIELD_PREP(GMAC_PLLCLK_DIV_NUM, div),

Sorry for not noticing this before sending my previous email.

Although the code above is correct. I think it would be clearer
to use ';' rather than ',' at the end of the line above. Perhaps ','
is a typo.(',' is next to ';' on my keyboard at least).

Flagged by Clang 20.1.8 with -Wcomma

> +
> +		writel(0, dwmac->apb_base + GMAC_PLLCLK_DIV);
> +		writel(reg, dwmac->apb_base + GMAC_PLLCLK_DIV);
> +
>  		writel(GMAC_GTXCLK_SEL_PLL, dwmac->apb_base + GMAC_GTXCLK_SEL);
>  		reg = GMAC_TX_CLK_EN | GMAC_TX_CLK_N_EN | GMAC_TX_CLK_OUT_EN |
>  		      GMAC_RX_CLK_EN | GMAC_RX_CLK_N_EN;

-- 
pw-bot: changes-requested.

Re: [PATCH net v2] net: stmmac: thead: Enable TX clock before MAC initialization

[Yao Zi]

On Fri, Aug 08, 2025 at 02:39:00PM +0100, Simon Horman wrote:
> On Fri, Aug 08, 2025 at 10:34:48AM +0000, Yao Zi wrote:
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > index f2946bea0bc2..50c1920bde6a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > @@ -152,7 +152,7 @@ static int thead_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
> >  static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
> >  {
> >  	struct thead_dwmac *dwmac = plat->bsp_priv;
> > -	u32 reg;
> > +	u32 reg, div;
> >  
> >  	switch (plat->mac_interface) {
> >  	case PHY_INTERFACE_MODE_MII:
> > @@ -164,6 +164,13 @@ static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
> >  	case PHY_INTERFACE_MODE_RGMII_RXID:
> >  	case PHY_INTERFACE_MODE_RGMII_TXID:
> >  		/* use pll */
> > +		div = clk_get_rate(plat->stmmac_clk) / rgmii_clock(SPEED_1000);
> > +		reg = FIELD_PREP(GMAC_PLLCLK_DIV_EN, 1) |
> > +		      FIELD_PREP(GMAC_PLLCLK_DIV_NUM, div),
> 
> Sorry for not noticing this before sending my previous email.
> 
> Although the code above is correct. I think it would be clearer
> to use ';' rather than ',' at the end of the line above. Perhaps ','
> is a typo.(',' is next to ';' on my keyboard at least).

This is actually a typo that occasionally passes the compilation... yeah
thanks for catching it.

I'll add a comment about the requirement that PLLCLK_DIV_EN must be set
to zero before changing the rate, and fix this typo in v3 as well.

> Flagged by Clang 20.1.8 with -Wcomma

Best regards,
Yao Zi

> > +
> > +		writel(0, dwmac->apb_base + GMAC_PLLCLK_DIV);
> > +		writel(reg, dwmac->apb_base + GMAC_PLLCLK_DIV);
> > +
> >  		writel(GMAC_GTXCLK_SEL_PLL, dwmac->apb_base + GMAC_GTXCLK_SEL);
> >  		reg = GMAC_TX_CLK_EN | GMAC_TX_CLK_N_EN | GMAC_TX_CLK_OUT_EN |
> >  		      GMAC_RX_CLK_EN | GMAC_RX_CLK_N_EN;
> 
> -- 
> pw-bot: changes-requested.