| Message ID | 20230710211313.567761-2-dinguyen@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| 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 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
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?
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/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)
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(-)