[PATCH net v2 0/3] Fix broken link with TH1520 GMAC when linkspeed changes

[PATCH net v2 0/3] Fix broken link with TH1520 GMAC when linkspeed changes

[Yao Zi]

It's noted that on TH1520 SoC, the GMAC's link becomes broken after
the link speed is changed (for example, running ethtool -s eth0 speed
100 on the peer when negotiated to 1Gbps), but the GMAC could function
normally if the speed is brought back to the initial.

Just like many other SoCs utilizing STMMAC IP, we need to adjust the TX
clock supplying TH1520's GMAC through some SoC-specific glue registers
when linkspeed changes. But it's found that after the full kernel
startup, reading from them results in garbage and writing to them makes
no effect, which is the cause of broken link.

Further testing shows perisys-apb4-hclk must be ungated for normal
access to Th1520 GMAC APB glue registers, which is neither described in
dt-binding nor acquired by the driver.

This series expands the dt-binding of TH1520's GMAC to allow an extra
"APB glue registers interface clock", instructs the driver to acquire
and enable the clock, and finally supplies CLK_PERISYS_APB4_HCLK for
TH1520's GMACs in SoC devicetree.

Changed from v1
- Make apb clock essential in dt-binding
- Collect review tags
- Link to v1: https://lore.kernel.org/all/20250729093734.40132-1-ziyao@disroot.org/

Yao Zi (3):
  dt-bindings: net: thead,th1520-gmac: Describe APB interface clock
  net: stmmac: thead: Get and enable APB clock on initialization
  riscv: dts: thead: Add APB clocks for TH1520 GMACs

 .../devicetree/bindings/net/thead,th1520-gmac.yaml     |  6 ++++--
 arch/riscv/boot/dts/thead/th1520.dtsi                  | 10 ++++++----
 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c      |  6 ++++++
 3 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.50.1

[PATCH net v2 1/3] dt-bindings: net: thead,th1520-gmac: Describe APB interface clock

[Yao Zi]

Besides ones for GMAC core and peripheral registers, the TH1520 GMAC
requires one more clock for configuring APB glue registers. Describe
it in the binding.

Fixes: f920ce04c399 ("dt-bindings: net: Add T-HEAD dwmac support")
Signed-off-by: Yao Zi <ziyao@disroot.org>
Tested-by: Drew Fustini <fustini@kernel.org>
---
 .../devicetree/bindings/net/thead,th1520-gmac.yaml          | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
index 6d9de3303762..b3492a9aa4ef 100644
--- a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
+++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
@@ -62,11 +62,13 @@ properties:
     items:
       - description: GMAC main clock
       - description: Peripheral registers interface clock
+      - description: APB glue registers interface clock
 
   clock-names:
     items:
       - const: stmmaceth
       - const: pclk
+      - const: apb
 
   interrupts:
     items:
@@ -88,8 +90,8 @@ examples:
         compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
         reg = <0xe7070000 0x2000>, <0xec003000 0x1000>;
         reg-names = "dwmac", "apb";
-        clocks = <&clk 1>, <&clk 2>;
-        clock-names = "stmmaceth", "pclk";
+        clocks = <&clk 1>, <&clk 2>, <&clk 3>;
+        clock-names = "stmmaceth", "pclk", "apb";
         interrupts = <66>;
         interrupt-names = "macirq";
         phy-mode = "rgmii-id";
-- 
2.50.1

[PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Yao Zi]

It's necessary to adjust the MAC TX clock when the linkspeed changes,
but it's noted such adjustment always fails on TH1520 SoC, and reading
back from APB glue registers that control clock generation results in
garbage, causing broken link.

With some testing, it's found a clock must be ungated for access to APB
glue registers. Without any consumer, the clock is automatically
disabled during late kernel startup. Let's get and enable it if it's
described in devicetree.

Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
Signed-off-by: Yao Zi <ziyao@disroot.org>
Reviewed-by: Drew Fustini <fustini@kernel.org>
Tested-by: Drew Fustini <fustini@kernel.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
index c72ee759aae5..95096244a846 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
@@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
 	struct stmmac_resources stmmac_res;
 	struct plat_stmmacenet_data *plat;
 	struct thead_dwmac *dwmac;
+	struct clk *apb_clk;
 	void __iomem *apb;
 	int ret;
 
@@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
 				     "dt configuration failed\n");
 
+	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
+	if (IS_ERR(apb_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(apb_clk),
+				     "failed to get apb clock\n");
+
 	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
 	if (!dwmac)
 		return -ENOMEM;
-- 
2.50.1

[PATCH net v2 3/3] riscv: dts: thead: Add APB clocks for TH1520 GMACs

[Yao Zi]

Describe perisys-apb4-hclk as the APB clock for TH1520 SoC, which is
essential for accessing GMAC glue registers.

Fixes: 7e756671a664 ("riscv: dts: thead: Add TH1520 ethernet nodes")
Signed-off-by: Yao Zi <ziyao@disroot.org>
Reviewed-by: Drew Fustini <fustini@kernel.org>
Tested-by: Drew Fustini <fustini@kernel.org>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 42724bf7e90e..03f1d7319049 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -297,8 +297,9 @@ gmac1: ethernet@ffe7060000 {
 			reg-names = "dwmac", "apb";
 			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "macirq";
-			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC1>;
-			clock-names = "stmmaceth", "pclk";
+			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC1>,
+				 <&clk CLK_PERISYS_APB4_HCLK>;
+			clock-names = "stmmaceth", "pclk", "apb";
 			snps,pbl = <32>;
 			snps,fixed-burst;
 			snps,multicast-filter-bins = <64>;
@@ -319,8 +320,9 @@ gmac0: ethernet@ffe7070000 {
 			reg-names = "dwmac", "apb";
 			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "macirq";
-			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC0>;
-			clock-names = "stmmaceth", "pclk";
+			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC0>,
+				 <&clk CLK_PERISYS_APB4_HCLK>;
+			clock-names = "stmmaceth", "pclk", "apb";
 			snps,pbl = <32>;
 			snps,fixed-burst;
 			snps,multicast-filter-bins = <64>;
-- 
2.50.1

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Rob Herring]

On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> It's necessary to adjust the MAC TX clock when the linkspeed changes,
> but it's noted such adjustment always fails on TH1520 SoC, and reading
> back from APB glue registers that control clock generation results in
> garbage, causing broken link.
> 
> With some testing, it's found a clock must be ungated for access to APB
> glue registers. Without any consumer, the clock is automatically
> disabled during late kernel startup. Let's get and enable it if it's
> described in devicetree.
> 
> Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> Reviewed-by: Drew Fustini <fustini@kernel.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> index c72ee759aae5..95096244a846 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
>  	struct stmmac_resources stmmac_res;
>  	struct plat_stmmacenet_data *plat;
>  	struct thead_dwmac *dwmac;
> +	struct clk *apb_clk;
>  	void __iomem *apb;
>  	int ret;
>  
> @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
>  				     "dt configuration failed\n");
>  
> +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");

The description sounds like this should not be optional. The binding 
change also makes it not optional.

Rob

Re: [PATCH net v2 1/3] dt-bindings: net: thead,th1520-gmac: Describe APB interface clock

[Drew Fustini]

On Fri, Aug 01, 2025 at 09:12:38AM +0000, Yao Zi wrote:
> Besides ones for GMAC core and peripheral registers, the TH1520 GMAC
> requires one more clock for configuring APB glue registers. Describe
> it in the binding.
> 
> Fixes: f920ce04c399 ("dt-bindings: net: Add T-HEAD dwmac support")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> ---
>  .../devicetree/bindings/net/thead,th1520-gmac.yaml          | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> index 6d9de3303762..b3492a9aa4ef 100644
> --- a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> @@ -62,11 +62,13 @@ properties:
>      items:
>        - description: GMAC main clock
>        - description: Peripheral registers interface clock
> +      - description: APB glue registers interface clock
>  
>    clock-names:
>      items:
>        - const: stmmaceth
>        - const: pclk
> +      - const: apb
>  
>    interrupts:
>      items:
> @@ -88,8 +90,8 @@ examples:
>          compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
>          reg = <0xe7070000 0x2000>, <0xec003000 0x1000>;
>          reg-names = "dwmac", "apb";
> -        clocks = <&clk 1>, <&clk 2>;
> -        clock-names = "stmmaceth", "pclk";
> +        clocks = <&clk 1>, <&clk 2>, <&clk 3>;
> +        clock-names = "stmmaceth", "pclk", "apb";
>          interrupts = <66>;
>          interrupt-names = "macirq";
>          phy-mode = "rgmii-id";
> -- 
> 2.50.1
> 

Reviewed-by: Drew Fustini <fustini@kernel.org>

Thank you for making all 3 clocks required.

Drew

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Drew Fustini]

On Sun, Aug 03, 2025 at 12:02:06PM -0500, Rob Herring wrote:
> On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> > It's necessary to adjust the MAC TX clock when the linkspeed changes,
> > but it's noted such adjustment always fails on TH1520 SoC, and reading
> > back from APB glue registers that control clock generation results in
> > garbage, causing broken link.
> > 
> > With some testing, it's found a clock must be ungated for access to APB
> > glue registers. Without any consumer, the clock is automatically
> > disabled during late kernel startup. Let's get and enable it if it's
> > described in devicetree.
> > 
> > Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > Reviewed-by: Drew Fustini <fustini@kernel.org>
> > Tested-by: Drew Fustini <fustini@kernel.org>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > index c72ee759aae5..95096244a846 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> >  	struct stmmac_resources stmmac_res;
> >  	struct plat_stmmacenet_data *plat;
> >  	struct thead_dwmac *dwmac;
> > +	struct clk *apb_clk;
> >  	void __iomem *apb;
> >  	int ret;
> >  
> > @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> >  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> >  				     "dt configuration failed\n");
> >  
> > +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
> 
> The description sounds like this should not be optional. The binding 
> change also makes it not optional.

Good point, it should be devm_clk_get_enabled() otherwise the link speed
change bug will be possible. This series also changes the schema and dts
so I don't think compatibility is a problem.

Thanks,
Drew

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Yao Zi]

On Sun, Aug 03, 2025 at 12:02:06PM -0500, Rob Herring wrote:
> On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> > It's necessary to adjust the MAC TX clock when the linkspeed changes,
> > but it's noted such adjustment always fails on TH1520 SoC, and reading
> > back from APB glue registers that control clock generation results in
> > garbage, causing broken link.
> > 
> > With some testing, it's found a clock must be ungated for access to APB
> > glue registers. Without any consumer, the clock is automatically
> > disabled during late kernel startup. Let's get and enable it if it's
> > described in devicetree.
> > 
> > Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > Reviewed-by: Drew Fustini <fustini@kernel.org>
> > Tested-by: Drew Fustini <fustini@kernel.org>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > index c72ee759aae5..95096244a846 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> >  	struct stmmac_resources stmmac_res;
> >  	struct plat_stmmacenet_data *plat;
> >  	struct thead_dwmac *dwmac;
> > +	struct clk *apb_clk;
> >  	void __iomem *apb;
> >  	int ret;
> >  
> > @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> >  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> >  				     "dt configuration failed\n");
> >  
> > +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
> 
> The description sounds like this should not be optional. The binding 
> change also makes it not optional.

Yes, it shouldn't be. But using the non-optional API will cause the
driver fails to probe with the old (problematic) devicetree, IOW, it
breaks the ABI. Comparing to unusable ethernet, failing to adjust the
link speed sounds a minor point to me.

Maybe we could add a comment to explain why optional API is used, or
just use the non-optional one if such ABI breakages are acceptable --
for which I'd like to wait for more opinions.

> Rob
> 

Thanks,
Yao Zi

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Drew Fustini]

On Mon, Aug 04, 2025 at 05:12:26AM +0000, Yao Zi wrote:
> On Sun, Aug 03, 2025 at 12:02:06PM -0500, Rob Herring wrote:
> > On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> > > It's necessary to adjust the MAC TX clock when the linkspeed changes,
> > > but it's noted such adjustment always fails on TH1520 SoC, and reading
> > > back from APB glue registers that control clock generation results in
> > > garbage, causing broken link.
> > > 
> > > With some testing, it's found a clock must be ungated for access to APB
> > > glue registers. Without any consumer, the clock is automatically
> > > disabled during late kernel startup. Let's get and enable it if it's
> > > described in devicetree.
> > > 
> > > Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > Reviewed-by: Drew Fustini <fustini@kernel.org>
> > > Tested-by: Drew Fustini <fustini@kernel.org>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > index c72ee759aae5..95096244a846 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > >  	struct stmmac_resources stmmac_res;
> > >  	struct plat_stmmacenet_data *plat;
> > >  	struct thead_dwmac *dwmac;
> > > +	struct clk *apb_clk;
> > >  	void __iomem *apb;
> > >  	int ret;
> > >  
> > > @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > >  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> > >  				     "dt configuration failed\n");
> > >  
> > > +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
> > 
> > The description sounds like this should not be optional. The binding 
> > change also makes it not optional.
> 
> Yes, it shouldn't be. But using the non-optional API will cause the
> driver fails to probe with the old (problematic) devicetree, IOW, it
> breaks the ABI. Comparing to unusable ethernet, failing to adjust the
> link speed sounds a minor point to me.

I've just read Conor's comment in the v1 again: 

 Nah, introduce the warnings. If the clock is required for operation, it
 should be marked as such. You've made it optional in the driver, which
 is the important part (backwards compatible) and you've got the dts
 patch in the series.

Thus I think the argument I made in my reply to this thread is
incorrect and the driver should remain backwards compatible.

> Maybe we could add a comment to explain why optional API is used, or
> just use the non-optional one if such ABI breakages are acceptable --
> for which I'd like to wait for more opinions.

I think a comment in the code about why it uses the optional variant of
the function is a good idea.

Thanks,
Drew

Re: [PATCH net v2 1/3] dt-bindings: net: thead,th1520-gmac: Describe APB interface clock

[Krzysztof Kozlowski]

On Fri, Aug 01, 2025 at 09:12:38AM +0000, Yao Zi wrote:
> Besides ones for GMAC core and peripheral registers, the TH1520 GMAC
> requires one more clock for configuring APB glue registers. Describe
> it in the binding.
> 
> Fixes: f920ce04c399 ("dt-bindings: net: Add T-HEAD dwmac support")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> Tested-by: Drew Fustini <fustini@kernel.org>

You cannoy really test the binding, except part of build process and we
do not consider building something equal to testing.

> ---
>  .../devicetree/bindings/net/thead,th1520-gmac.yaml          | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH net v2 1/3] dt-bindings: net: thead,th1520-gmac: Describe APB interface clock

[Drew Fustini]

On Tue, Aug 05, 2025 at 08:39:59AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Aug 01, 2025 at 09:12:38AM +0000, Yao Zi wrote:
> > Besides ones for GMAC core and peripheral registers, the TH1520 GMAC
> > requires one more clock for configuring APB glue registers. Describe
> > it in the binding.
> > 
> > Fixes: f920ce04c399 ("dt-bindings: net: Add T-HEAD dwmac support")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > Tested-by: Drew Fustini <fustini@kernel.org>
> 
> You cannoy really test the binding, except part of build process and we
> do not consider building something equal to testing.

Good point. I've since provided my Reviewed-by: in this v2 thread so the
Tested-by: should be dropped if there or is another series or when
applying this v2.

Thanks,
Drew

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Drew Fustini]

On Mon, Aug 04, 2025 at 09:23:00AM -0700, Drew Fustini wrote:
> On Mon, Aug 04, 2025 at 05:12:26AM +0000, Yao Zi wrote:
> > On Sun, Aug 03, 2025 at 12:02:06PM -0500, Rob Herring wrote:
> > > On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> > > > It's necessary to adjust the MAC TX clock when the linkspeed changes,
> > > > but it's noted such adjustment always fails on TH1520 SoC, and reading
> > > > back from APB glue registers that control clock generation results in
> > > > garbage, causing broken link.
> > > > 
> > > > With some testing, it's found a clock must be ungated for access to APB
> > > > glue registers. Without any consumer, the clock is automatically
> > > > disabled during late kernel startup. Let's get and enable it if it's
> > > > described in devicetree.
> > > > 
> > > > Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> > > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > > Reviewed-by: Drew Fustini <fustini@kernel.org>
> > > > Tested-by: Drew Fustini <fustini@kernel.org>
> > > > ---
> > > >  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > index c72ee759aae5..95096244a846 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > > >  	struct stmmac_resources stmmac_res;
> > > >  	struct plat_stmmacenet_data *plat;
> > > >  	struct thead_dwmac *dwmac;
> > > > +	struct clk *apb_clk;
> > > >  	void __iomem *apb;
> > > >  	int ret;
> > > >  
> > > > @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > > >  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> > > >  				     "dt configuration failed\n");
> > > >  
> > > > +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
> > > 
> > > The description sounds like this should not be optional. The binding 
> > > change also makes it not optional.
> > 
> > Yes, it shouldn't be. But using the non-optional API will cause the
> > driver fails to probe with the old (problematic) devicetree, IOW, it
> > breaks the ABI. Comparing to unusable ethernet, failing to adjust the
> > link speed sounds a minor point to me.
> 
> I've just read Conor's comment in the v1 again: 
> 
>  Nah, introduce the warnings. If the clock is required for operation, it
>  should be marked as such. You've made it optional in the driver, which
>  is the important part (backwards compatible) and you've got the dts
>  patch in the series.
> 
> Thus I think the argument I made in my reply to this thread is
> incorrect and the driver should remain backwards compatible.
> 
> > Maybe we could add a comment to explain why optional API is used, or
> > just use the non-optional one if such ABI breakages are acceptable --
> > for which I'd like to wait for more opinions.
> 
> I think a comment in the code about why it uses the optional variant of
> the function is a good idea.

I was chatting on devicetree irc channel, and I think that it would be
good to add something like this to the commit message:

 For the purposes of backwards compatibility, the probe will not fail if
 'apb' is not found, but the link will break if the speed changes after
 probe.

Also, if devm_clk_get_optional_enabled("apb") fails, then I think it
would be a good idea to warn the user that changing the link speed will
not be supported.

Thanks,
Drew

Re: [PATCH net v2 2/3] net: stmmac: thead: Get and enable APB clock on initialization

[Yao Zi]

On Tue, Aug 05, 2025 at 11:05:31AM -0700, Drew Fustini wrote:
> On Mon, Aug 04, 2025 at 09:23:00AM -0700, Drew Fustini wrote:
> > On Mon, Aug 04, 2025 at 05:12:26AM +0000, Yao Zi wrote:
> > > On Sun, Aug 03, 2025 at 12:02:06PM -0500, Rob Herring wrote:
> > > > On Fri, Aug 01, 2025 at 09:12:39AM +0000, Yao Zi wrote:
> > > > > It's necessary to adjust the MAC TX clock when the linkspeed changes,
> > > > > but it's noted such adjustment always fails on TH1520 SoC, and reading
> > > > > back from APB glue registers that control clock generation results in
> > > > > garbage, causing broken link.
> > > > > 
> > > > > With some testing, it's found a clock must be ungated for access to APB
> > > > > glue registers. Without any consumer, the clock is automatically
> > > > > disabled during late kernel startup. Let's get and enable it if it's
> > > > > described in devicetree.
> > > > > 
> > > > > Fixes: 33a1a01e3afa ("net: stmmac: Add glue layer for T-HEAD TH1520 SoC")
> > > > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > > > Reviewed-by: Drew Fustini <fustini@kernel.org>
> > > > > Tested-by: Drew Fustini <fustini@kernel.org>
> > > > > ---
> > > > >  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > > index c72ee759aae5..95096244a846 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> > > > > @@ -211,6 +211,7 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > > > >  	struct stmmac_resources stmmac_res;
> > > > >  	struct plat_stmmacenet_data *plat;
> > > > >  	struct thead_dwmac *dwmac;
> > > > > +	struct clk *apb_clk;
> > > > >  	void __iomem *apb;
> > > > >  	int ret;
> > > > >  
> > > > > @@ -224,6 +225,11 @@ static int thead_dwmac_probe(struct platform_device *pdev)
> > > > >  		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> > > > >  				     "dt configuration failed\n");
> > > > >  
> > > > > +	apb_clk = devm_clk_get_optional_enabled(&pdev->dev, "apb");
> > > > 
> > > > The description sounds like this should not be optional. The binding 
> > > > change also makes it not optional.
> > > 
> > > Yes, it shouldn't be. But using the non-optional API will cause the
> > > driver fails to probe with the old (problematic) devicetree, IOW, it
> > > breaks the ABI. Comparing to unusable ethernet, failing to adjust the
> > > link speed sounds a minor point to me.
> > 
> > I've just read Conor's comment in the v1 again: 
> > 
> >  Nah, introduce the warnings. If the clock is required for operation, it
> >  should be marked as such. You've made it optional in the driver, which
> >  is the important part (backwards compatible) and you've got the dts
> >  patch in the series.
> > 
> > Thus I think the argument I made in my reply to this thread is
> > incorrect and the driver should remain backwards compatible.
> > 
> > > Maybe we could add a comment to explain why optional API is used, or
> > > just use the non-optional one if such ABI breakages are acceptable --
> > > for which I'd like to wait for more opinions.
> > 
> > I think a comment in the code about why it uses the optional variant of
> > the function is a good idea.
> 
> I was chatting on devicetree irc channel, and I think that it would be
> good to add something like this to the commit message:
> 
>  For the purposes of backwards compatibility, the probe will not fail if
>  'apb' is not found, but the link will break if the speed changes after
>  probe.
> 
> Also, if devm_clk_get_optional_enabled("apb") fails, then I think it
> would be a good idea to warn the user that changing the link speed will
> not be supported.

Both sound reasonable to me, and I'll add them in v3, thanks.

> Thanks,
> Drew

Best regards,
Yao Zi

Re: [PATCH net v2 1/3] dt-bindings: net: thead,th1520-gmac: Describe APB interface clock

[Yao Zi]

On Tue, Aug 05, 2025 at 09:59:06AM -0700, Drew Fustini wrote:
> On Tue, Aug 05, 2025 at 08:39:59AM +0200, Krzysztof Kozlowski wrote:
> > On Fri, Aug 01, 2025 at 09:12:38AM +0000, Yao Zi wrote:
> > > Besides ones for GMAC core and peripheral registers, the TH1520 GMAC
> > > requires one more clock for configuring APB glue registers. Describe
> > > it in the binding.
> > > 
> > > Fixes: f920ce04c399 ("dt-bindings: net: Add T-HEAD dwmac support")
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > Tested-by: Drew Fustini <fustini@kernel.org>
> > 
> > You cannoy really test the binding, except part of build process and we
> > do not consider building something equal to testing.
> 
> Good point. I've since provided my Reviewed-by: in this v2 thread so the
> Tested-by: should be dropped if there or is another series or when
> applying this v2.

I'll send v3 of this series soon, dropping the Tested-by and adding
Krzysztof's Acked-by tag. Thanks for the guidance!

> Thanks,
> Drew

Best regards,
Yao Zi