[PATCH 1/2] arm64: dts: socfpga: change the reset-name of "stmmaceth-ocp" to "ahb"

[PATCH 1/2] arm64: dts: socfpga: change the reset-name of "stmmaceth-ocp" to "ahb"

[Dinh Nguyen]

The "stmmaceth-ocp" reset line on the SoCFPGA stmmac ethernet driver is
the same as the "abh" reset on a standard stmmac ethernet.

commit ("843f603762a5 dt-bindings: net: snps,dwmac: Add 'ahb'
reset/reset-name") documented the second reset signal as 'ahb' instead
of 'stmmaceth-ocp'. Change the reset-names of the SoCFPGA DWMAC driver to
'ahb'.

This also fixes the dtbs_check warning:
ethernet@ff802000: reset-names:1: 'ahb' was expected

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi | 6 +++---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 6 +++---
 arch/arm64/boot/dts/intel/socfpga_agilex.dtsi        | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi
index 72c55e5187ca..f36063c57c7f 100644
--- a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi
@@ -440,7 +440,7 @@ gmac0: ethernet@ff800000 {
 			clocks = <&l4_mp_clk>, <&peri_emac_ptp_clk>;
 			clock-names = "stmmaceth", "ptp_ref";
 			resets = <&rst EMAC0_RESET>, <&rst EMAC0_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			snps,axi-config = <&socfpga_axi_setup>;
 			status = "disabled";
 		};
@@ -460,7 +460,7 @@ gmac1: ethernet@ff802000 {
 			clocks = <&l4_mp_clk>, <&peri_emac_ptp_clk>;
 			clock-names = "stmmaceth", "ptp_ref";
 			resets = <&rst EMAC1_RESET>, <&rst EMAC1_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			snps,axi-config = <&socfpga_axi_setup>;
 			status = "disabled";
 		};
@@ -480,7 +480,7 @@ gmac2: ethernet@ff804000 {
 			clocks = <&l4_mp_clk>, <&peri_emac_ptp_clk>;
 			clock-names = "stmmaceth", "ptp_ref";
 			resets = <&rst EMAC2_RESET>, <&rst EMAC2_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			snps,axi-config = <&socfpga_axi_setup>;
 			status = "disabled";
 		};
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 1c846f13539c..439497ab967d 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -153,7 +153,7 @@ gmac0: ethernet@ff800000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC0_RESET>, <&rst EMAC0_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			clocks = <&clkmgr STRATIX10_EMAC0_CLK>, <&clkmgr STRATIX10_EMAC_PTP_CLK>;
 			clock-names = "stmmaceth", "ptp_ref";
 			tx-fifo-depth = <16384>;
@@ -171,7 +171,7 @@ gmac1: ethernet@ff802000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC1_RESET>, <&rst EMAC1_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			clocks = <&clkmgr STRATIX10_EMAC1_CLK>, <&clkmgr STRATIX10_EMAC_PTP_CLK>;
 			clock-names = "stmmaceth", "ptp_ref";
 			tx-fifo-depth = <16384>;
@@ -189,7 +189,7 @@ gmac2: ethernet@ff804000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC2_RESET>, <&rst EMAC2_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			clocks = <&clkmgr STRATIX10_EMAC2_CLK>, <&clkmgr STRATIX10_EMAC_PTP_CLK>;
 			clock-names = "stmmaceth", "ptp_ref";
 			tx-fifo-depth = <16384>;
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
index fc047aef4911..d3adb6a130ae 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -158,7 +158,7 @@ gmac0: ethernet@ff800000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC0_RESET>, <&rst EMAC0_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			tx-fifo-depth = <16384>;
 			rx-fifo-depth = <16384>;
 			snps,multicast-filter-bins = <256>;
@@ -176,7 +176,7 @@ gmac1: ethernet@ff802000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC1_RESET>, <&rst EMAC1_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			tx-fifo-depth = <16384>;
 			rx-fifo-depth = <16384>;
 			snps,multicast-filter-bins = <256>;
@@ -194,7 +194,7 @@ gmac2: ethernet@ff804000 {
 			interrupt-names = "macirq";
 			mac-address = [00 00 00 00 00 00];
 			resets = <&rst EMAC2_RESET>, <&rst EMAC2_OCP_RESET>;
-			reset-names = "stmmaceth", "stmmaceth-ocp";
+			reset-names = "stmmaceth", "ahb";
 			tx-fifo-depth = <16384>;
 			rx-fifo-depth = <16384>;
 			snps,multicast-filter-bins = <256>;
-- 
2.25.1

[PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Dinh Nguyen]

The "stmmaceth-ocp" reset line of stmmac controller on the SoCFPGA
platform is essentially the "ahb" reset on the standard stmmac controller.

Because of commit ("e67f325e9cd6 net: stmmac: explicitly deassert
GMAC_AHB_RESET") adds the support for getting the 'ahb' reset, the
SoCFPGA dwmac driver no longer need to get the stmmaceth-ocp reset.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 20 ++++++-------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6267bcb60206..a4b8b86129f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -52,7 +52,7 @@ struct socfpga_dwmac {
 	struct	device *dev;
 	struct regmap *sys_mgr_base_addr;
 	struct reset_control *stmmac_rst;
-	struct reset_control *stmmac_ocp_rst;
+	struct reset_control *stmmac_ahb_rst;
 	void __iomem *splitter_base;
 	void __iomem *tse_pcs_base;
 	void __iomem *sgmii_adapter_base;
@@ -290,7 +290,7 @@ static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
 		val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
 
 	/* Assert reset to the enet controller before changing the phy mode */
-	reset_control_assert(dwmac->stmmac_ocp_rst);
+	reset_control_assert(dwmac->stmmac_ahb_rst);
 	reset_control_assert(dwmac->stmmac_rst);
 
 	regmap_read(sys_mgr_base_addr, reg_offset, &ctrl);
@@ -319,7 +319,7 @@ static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
 	/* Deassert reset for the phy configuration to be sampled by
 	 * the enet controller, and operation to start in requested mode
 	 */
-	reset_control_deassert(dwmac->stmmac_ocp_rst);
+	reset_control_deassert(dwmac->stmmac_ahb_rst);
 	reset_control_deassert(dwmac->stmmac_rst);
 	if (phymode == PHY_INTERFACE_MODE_SGMII)
 		socfpga_sgmii_config(dwmac, true);
@@ -346,7 +346,7 @@ static int socfpga_gen10_set_phy_mode(struct socfpga_dwmac *dwmac)
 		val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
 
 	/* Assert reset to the enet controller before changing the phy mode */
-	reset_control_assert(dwmac->stmmac_ocp_rst);
+	reset_control_assert(dwmac->stmmac_ahb_rst);
 	reset_control_assert(dwmac->stmmac_rst);
 
 	regmap_read(sys_mgr_base_addr, reg_offset, &ctrl);
@@ -372,7 +372,7 @@ static int socfpga_gen10_set_phy_mode(struct socfpga_dwmac *dwmac)
 	/* Deassert reset for the phy configuration to be sampled by
 	 * the enet controller, and operation to start in requested mode
 	 */
-	reset_control_deassert(dwmac->stmmac_ocp_rst);
+	reset_control_deassert(dwmac->stmmac_ahb_rst);
 	reset_control_deassert(dwmac->stmmac_rst);
 	if (phymode == PHY_INTERFACE_MODE_SGMII)
 		socfpga_sgmii_config(dwmac, true);
@@ -410,15 +410,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 		goto err_remove_config_dt;
 	}
 
-	dwmac->stmmac_ocp_rst = devm_reset_control_get_optional(dev, "stmmaceth-ocp");
-	if (IS_ERR(dwmac->stmmac_ocp_rst)) {
-		ret = PTR_ERR(dwmac->stmmac_ocp_rst);
-		dev_err(dev, "error getting reset control of ocp %d\n", ret);
-		goto err_remove_config_dt;
-	}
-
-	reset_control_deassert(dwmac->stmmac_ocp_rst);
-
 	ret = socfpga_dwmac_parse_data(dwmac, dev);
 	if (ret) {
 		dev_err(dev, "Unable to parse OF data\n");
@@ -441,6 +432,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	 * the driver later.
 	 */
 	dwmac->stmmac_rst = stpriv->plat->stmmac_rst;
+	dwmac->stmmac_ahb_rst = stpriv->plat->stmmac_ahb_rst;
 
 	ret = ops->set_phy_mode(dwmac);
 	if (ret)
-- 
2.25.1

Re: [PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Jakub Kicinski]

On Mon, 10 Jul 2023 16:13:13 -0500 Dinh Nguyen wrote:
> -	dwmac->stmmac_ocp_rst = devm_reset_control_get_optional(dev, "stmmaceth-ocp");
> -	if (IS_ERR(dwmac->stmmac_ocp_rst)) {
> -		ret = PTR_ERR(dwmac->stmmac_ocp_rst);
> -		dev_err(dev, "error getting reset control of ocp %d\n", ret);
> -		goto err_remove_config_dt;
> -	}
> -
> -	reset_control_deassert(dwmac->stmmac_ocp_rst);

Noob question, perhaps - what's the best practice for incompatible
device tree changes? Updating the in-tree definitions is good enough?
Seems like we could quite easily continue to support "stmmaceth-ocp"
but no point complicating the code if not required.

Re: [PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Krzysztof Kozlowski]

On 13/07/2023 02:08, Jakub Kicinski wrote:
> On Mon, 10 Jul 2023 16:13:13 -0500 Dinh Nguyen wrote:
>> -	dwmac->stmmac_ocp_rst = devm_reset_control_get_optional(dev, "stmmaceth-ocp");
>> -	if (IS_ERR(dwmac->stmmac_ocp_rst)) {
>> -		ret = PTR_ERR(dwmac->stmmac_ocp_rst);
>> -		dev_err(dev, "error getting reset control of ocp %d\n", ret);
>> -		goto err_remove_config_dt;
>> -	}
>> -
>> -	reset_control_deassert(dwmac->stmmac_ocp_rst);
> 
> Noob question, perhaps - what's the best practice for incompatible
> device tree changes?

They are an ABI break.

> Updating the in-tree definitions is good enough?

No, because this is an ABI so we expect:
1. old DTS
2. out-of-tree DTS
to work properly with new kernel (not broken by a change).

However for ABI breaks with scope limited to only one given platform, it
is the platform's maintainer choice to allow or not allow ABI breaks.
What we, Devicetree maintainers expect, is to mention and provide
rationale for the ABI break in the commit msg.

> Seems like we could quite easily continue to support "stmmaceth-ocp"
> but no point complicating the code if not required.

Best regards,
Krzysztof

Re: [PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Paolo Abeni]

On Thu, 2023-07-13 at 10:24 +0200, Krzysztof Kozlowski wrote:
> On 13/07/2023 02:08, Jakub Kicinski wrote:
> > On Mon, 10 Jul 2023 16:13:13 -0500 Dinh Nguyen wrote:
> > > -	dwmac->stmmac_ocp_rst = devm_reset_control_get_optional(dev, "stmmaceth-ocp");
> > > -	if (IS_ERR(dwmac->stmmac_ocp_rst)) {
> > > -		ret = PTR_ERR(dwmac->stmmac_ocp_rst);
> > > -		dev_err(dev, "error getting reset control of ocp %d\n", ret);
> > > -		goto err_remove_config_dt;
> > > -	}
> > > -
> > > -	reset_control_deassert(dwmac->stmmac_ocp_rst);
> > 
> > Noob question, perhaps - what's the best practice for incompatible
> > device tree changes?
> 
> They are an ABI break.
> 
> > Updating the in-tree definitions is good enough?
> 
> No, because this is an ABI so we expect:
> 1. old DTS
> 2. out-of-tree DTS
> to work properly with new kernel (not broken by a change).
> 
> However for ABI breaks with scope limited to only one given platform, it
> is the platform's maintainer choice to allow or not allow ABI breaks.
> What we, Devicetree maintainers expect, is to mention and provide
> rationale for the ABI break in the commit msg.

@Dinh: you should at least update the commit message to provide such
rationale, or possibly even better, drop this 2nd patch on next
submission.

Thanks,

Paolo

Re: [PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Jakub Kicinski]

On Thu, 13 Jul 2023 14:39:57 +0200 Paolo Abeni wrote:
> > However for ABI breaks with scope limited to only one given platform, it
> > is the platform's maintainer choice to allow or not allow ABI breaks.
> > What we, Devicetree maintainers expect, is to mention and provide
> > rationale for the ABI break in the commit msg.  
> 
> @Dinh: you should at least update the commit message to provide such
> rationale, or possibly even better, drop this 2nd patch on next
> submission.

Or support both bindings, because the reset looks optional. So maybe 
instead of deleting the use of "stmmaceth-ocp", only go down that path
if stpriv->plat->stmmac_ahb_rst is NULL?

Re: [PATCH 2/2] net: dwmac_socfpga: use the standard "ahb" reset

[Dinh Nguyen]

On 7/13/23 11:51, Jakub Kicinski wrote:
> On Thu, 13 Jul 2023 14:39:57 +0200 Paolo Abeni wrote:
>>> However for ABI breaks with scope limited to only one given platform, it
>>> is the platform's maintainer choice to allow or not allow ABI breaks.
>>> What we, Devicetree maintainers expect, is to mention and provide
>>> rationale for the ABI break in the commit msg.
>>
>> @Dinh: you should at least update the commit message to provide such
>> rationale, or possibly even better, drop this 2nd patch on next
>> submission.
> 
> Or support both bindings, because the reset looks optional. So maybe
> instead of deleting the use of "stmmaceth-ocp", only go down that path
> if stpriv->plat->stmmac_ahb_rst is NULL?

I think in a way, it's already supporting both reset lines. The main 
dwmac-platform is looking for "ahb" and the socfpga-dwmac is looking for 
"stmmaceth-ocp".

So I'll just drop this patch.

Thanks for all the review.

Dinh