| Message ID | 20230710211313.567761-1-dinguyen@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] arm64: dts: socfpga: change the reset-name of "stmmaceth-ocp" to "ahb" | expand |
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.
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
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
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>;
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(-)