[PATCH net-next v3 0/3] net: stmmac: Allow driver-specific AXI configuration

[PATCH net-next v3 0/3] net: stmmac: Allow driver-specific AXI configuration

[Thierry Reding]

AXI bus configuration can, in most cases, be derived from the compatible
string, so instead of relying exclusively on device tree for the AXI bus
configuration, create a method for device drivers to pass along a known-
good configuration.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- add comments to help explain override logic
- add missing kerneldoc for new parameter
- Link to v2: https://lore.kernel.org/r/20240202-stmmac-axi-config-v2-0-64eab2bab17b@nvidia.com

Changes in v2:
- fix bisectability issue between patches 1 and 2
- Link to v1: https://lore.kernel.org/r/20240201-stmmac-axi-config-v1-0-822e97b2d26e@nvidia.com

---
Thierry Reding (3):
      net: stmmac: Pass resources to DT parsing code
      net: stmmac: Allow drivers to provide a default AXI configuration
      net: stmmac: Configure AXI on Tegra234 MGBE

 .../net/ethernet/stmicro/stmmac/dwmac-anarion.c    |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-generic.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-ingenic.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-ipq806x.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-loongson1.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c    |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-starfive.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c  | 11 ++-
 .../net/ethernet/stmicro/stmmac/dwmac-visconti.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 78 ++++++++++++++--------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |  3 +-
 25 files changed, 87 insertions(+), 49 deletions(-)
---
base-commit: 35a4fdde2466b9d90af297f249436a270ef9d30e
change-id: 20240201-stmmac-axi-config-77ea05ea9eff

Best regards,
-- 
Thierry Reding <treding@nvidia.com>

[PATCH net-next v3 1/3] net: stmmac: Pass resources to DT parsing code

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

We already pass the stmmac_resources structure to other initialization
functions since it allows us to exchange more data between the driver
and those functions. Passing this structure to the DT parsing code makes
it easier to extend what we pass around in the future.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- fix bisectability
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-anarion.c   |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-generic.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c       |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c   |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c   |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c        |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c       |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 19 +++++++++++--------
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h |  3 ++-
 24 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-anarion.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-anarion.c
index 643ee6d8d4dd..f37d9912380b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-anarion.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-anarion.c
@@ -115,7 +115,7 @@ static int anarion_dwmac_probe(struct platform_device *pdev)
 	if (IS_ERR(gmac))
 		return PTR_ERR(gmac);
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index ec924c6c76c6..36ef2ed9fd2d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -435,7 +435,7 @@ static int dwc_eth_dwmac_probe(struct platform_device *pdev)
 	if (IS_ERR(stmmac_res.addr))
 		return PTR_ERR(stmmac_res.addr);
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-generic.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-generic.c
index 598eff926815..98a00084807b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-generic.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-generic.c
@@ -27,7 +27,7 @@ static int dwmac_generic_probe(struct platform_device *pdev)
 		return ret;
 
 	if (pdev->dev.of_node) {
-		plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+		plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 		if (IS_ERR(plat_dat)) {
 			dev_err(&pdev->dev, "dt configuration failed\n");
 			return PTR_ERR(plat_dat);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 6b65420e11b5..d72b9d8ac464 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -331,7 +331,7 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 	if (!dwmac)
 		return -ENOMEM;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c
index 19c93b998fb3..de2f5d7e95b2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c
@@ -241,7 +241,7 @@ static int ingenic_mac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index d68f0c4e7835..79582983c060 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -84,7 +84,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat)) {
 		dev_err(&pdev->dev, "dt configuration failed\n");
 		return PTR_ERR(plat_dat);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
index 281687d7083b..311410509012 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
@@ -384,7 +384,7 @@ static int ipq806x_gmac_probe(struct platform_device *pdev)
 	if (val)
 		return val;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
index 3e86810717d3..1ede4f409937 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
@@ -169,7 +169,7 @@ static int ls1x_dwmac_probe(struct platform_device *pdev)
 	if (!dwmac)
 		return -ENOMEM;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
 				     "dt configuration failed\n");
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c
index 4c810d8f5bea..004bf4d14fdb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-lpc18xx.c
@@ -37,7 +37,7 @@ static int lpc18xx_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
index 2a9132d6d743..763ec218ff2c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
@@ -656,7 +656,7 @@ static int mediatek_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
index a16bfa9089ea..5bd936ef2796 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
@@ -52,7 +52,7 @@ static int meson6_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index b23944aa344e..88db2907640d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -400,7 +400,7 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2691a250a5a7..7f4cbea4042f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -735,7 +735,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret,
 				     "Failed to get platform resources\n");
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat)) {
 		return dev_err_probe(dev, PTR_ERR(plat_dat),
 				     "dt configuration failed\n");
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 382e8de1255d..4297a6919c8a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1824,7 +1824,7 @@ static int rk_gmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 68f85e4605cb..e827e01b4398 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -400,7 +400,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 4e1076faee0c..a0f15fbb8993 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -122,7 +122,7 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, err,
 				     "failed to get resources\n");
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
 				     "dt configuration failed\n");
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index 4445cddc4cbe..5b2cde45dc3e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -273,7 +273,7 @@ static int sti_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index c92dfc4ecf57..af131b0bf589 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -366,7 +366,7 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index b21d99faa2d0..8f47f2e904d3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -1223,7 +1223,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return -EINVAL;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
index 2653a9f0958c..f78e244b9d81 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -108,7 +108,7 @@ static int sun7i_gmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
index 362f85136c3e..bab57d1675df 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
@@ -284,7 +284,7 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto disable_clks;
 
-	plat = devm_stmmac_probe_config_dt(pdev, res.mac);
+	plat = devm_stmmac_probe_config_dt(pdev, &res);
 	if (IS_ERR(plat)) {
 		err = PTR_ERR(plat);
 		goto disable_clks;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
index a5a5cfa989c6..f99d147b3424 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
@@ -220,7 +220,7 @@ static int visconti_eth_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	plat_dat = devm_stmmac_probe_config_dt(pdev, &stmmac_res);
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 54797edc9b38..4e2eb54306f9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -424,13 +424,14 @@ static void stmmac_remove_config_dt(struct platform_device *pdev,
 /**
  * stmmac_probe_config_dt - parse device-tree driver parameters
  * @pdev: platform_device structure
- * @mac: MAC address to use
+ * @res: driver-specific parameters
  * Description:
  * this function is to read the driver parameters from device-tree and
  * set some private fields that will be used by the main at runtime.
  */
 static struct plat_stmmacenet_data *
-stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
+stmmac_probe_config_dt(struct platform_device *pdev,
+		       struct stmmac_resources *res)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct plat_stmmacenet_data *plat;
@@ -443,12 +444,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	if (!plat)
 		return ERR_PTR(-ENOMEM);
 
-	rc = of_get_mac_address(np, mac);
+	rc = of_get_mac_address(np, res->mac);
 	if (rc) {
 		if (rc == -EPROBE_DEFER)
 			return ERR_PTR(rc);
 
-		eth_zero_addr(mac);
+		eth_zero_addr(res->mac);
 	}
 
 	phy_mode = device_get_phy_mode(&pdev->dev);
@@ -677,17 +678,18 @@ static void devm_stmmac_remove_config_dt(void *data)
 /**
  * devm_stmmac_probe_config_dt
  * @pdev: platform_device structure
- * @mac: MAC address to use
+ * @res: driver-specific parameters
  * Description: Devres variant of stmmac_probe_config_dt(). Does not require
  * the user to call stmmac_remove_config_dt() at driver detach.
  */
 struct plat_stmmacenet_data *
-devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
+devm_stmmac_probe_config_dt(struct platform_device *pdev,
+			    struct stmmac_resources *res)
 {
 	struct plat_stmmacenet_data *plat;
 	int ret;
 
-	plat = stmmac_probe_config_dt(pdev, mac);
+	plat = stmmac_probe_config_dt(pdev, res);
 	if (IS_ERR(plat))
 		return plat;
 
@@ -700,7 +702,8 @@ devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 }
 #else
 struct plat_stmmacenet_data *
-devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
+devm_stmmac_probe_config_dt(struct platform_device *pdev,
+			    struct stmmac_resources *res)
 {
 	return ERR_PTR(-EINVAL);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
index bb6fc7e59aed..daf3bbb503a3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
@@ -12,7 +12,8 @@
 #include "stmmac.h"
 
 struct plat_stmmacenet_data *
-devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac);
+devm_stmmac_probe_config_dt(struct platform_device *pdev,
+			    struct stmmac_resources *res);
 
 int stmmac_get_platform_resources(struct platform_device *pdev,
 				  struct stmmac_resources *stmmac_res);

-- 
2.43.2

[PATCH net-next v3 2/3] net: stmmac: Allow drivers to provide a default AXI configuration

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In many cases the AXI configuration can be derived from the compatible
string, so there's no need to add the configuration to DT. Allow drivers
to pass in the default AXI configuration so they can be properly set up
without extra data in DT.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- add comments to help explain override logic
- add missing kerneldoc for new parameter

Changes in v2:
- fix bisectability
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 59 +++++++++++++++-------
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index dddcaa9220cc..573c5d99b4d6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -36,6 +36,8 @@ struct stmmac_resources {
 	int sfty_ue_irq;
 	int rx_irq[MTL_MAX_RX_QUEUES];
 	int tx_irq[MTL_MAX_TX_QUEUES];
+
+	const struct stmmac_axi *axi;
 };
 
 enum stmmac_txbuf_type {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 4e2eb54306f9..583f78ae9bb0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -87,38 +87,61 @@ static int dwmac1000_validate_ucast_entries(struct device *dev,
 /**
  * stmmac_axi_setup - parse DT parameters for programming the AXI register
  * @pdev: platform device
+ * @res: driver-specific parameters
  * Description:
- * if required, from device-tree the AXI internal register can be tuned
- * by using platform parameters.
+ * Use driver-specific defaults for the AXI internal registers if available,
+ * or parse them from device tree, if present. Driver-specific defaults can
+ * be overridden by the values from device tree.
  */
-static struct stmmac_axi *stmmac_axi_setup(struct platform_device *pdev)
+static struct stmmac_axi *stmmac_axi_setup(struct platform_device *pdev,
+					   struct stmmac_resources *res)
 {
 	struct device_node *np;
 	struct stmmac_axi *axi;
 
+	/*
+	 * Exit early if the driver hasn't provided any defaults and the
+	 * device tree doesn't specify values for the AXI configuration.
+	 */
 	np = of_parse_phandle(pdev->dev.of_node, "snps,axi-config", 0);
-	if (!np)
+	if (!np && !res->axi)
 		return NULL;
 
 	axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
 	if (!axi) {
-		of_node_put(np);
+		if (np)
+			of_node_put(np);
+
 		return ERR_PTR(-ENOMEM);
 	}
 
-	axi->axi_lpi_en = of_property_read_bool(np, "snps,lpi_en");
-	axi->axi_xit_frm = of_property_read_bool(np, "snps,xit_frm");
-	axi->axi_kbbe = of_property_read_bool(np, "snps,kbbe");
-	axi->axi_fb = of_property_read_bool(np, "snps,fb");
-	axi->axi_mb = of_property_read_bool(np, "snps,mb");
-	axi->axi_rb =  of_property_read_bool(np, "snps,rb");
+	/* copy defaults provided by the driver */
+	if (res->axi)
+		*axi = *res->axi;
+
+	/* override defaults with data from DT */
+	if (np) {
+		axi->axi_lpi_en = of_property_read_bool(np, "snps,lpi_en");
+		axi->axi_xit_frm = of_property_read_bool(np, "snps,xit_frm");
+		axi->axi_kbbe = of_property_read_bool(np, "snps,kbbe");
+		axi->axi_fb = of_property_read_bool(np, "snps,fb");
+		axi->axi_mb = of_property_read_bool(np, "snps,mb");
+		axi->axi_rb =  of_property_read_bool(np, "snps,rb");
+
+		if (of_property_read_u32(np, "snps,wr_osr_lmt", &axi->axi_wr_osr_lmt)) {
+			if (!res->axi)
+				axi->axi_wr_osr_lmt = 1;
+		}
 
-	if (of_property_read_u32(np, "snps,wr_osr_lmt", &axi->axi_wr_osr_lmt))
-		axi->axi_wr_osr_lmt = 1;
-	if (of_property_read_u32(np, "snps,rd_osr_lmt", &axi->axi_rd_osr_lmt))
-		axi->axi_rd_osr_lmt = 1;
-	of_property_read_u32_array(np, "snps,blen", axi->axi_blen, AXI_BLEN);
-	of_node_put(np);
+		if (of_property_read_u32(np, "snps,rd_osr_lmt", &axi->axi_rd_osr_lmt)) {
+			if (!res->axi)
+				axi->axi_rd_osr_lmt = 1;
+		}
+
+		of_property_read_u32_array(np, "snps,blen", axi->axi_blen, AXI_BLEN);
+
+		of_node_put(np);
+	}
 
 	return axi;
 }
@@ -606,7 +629,7 @@ stmmac_probe_config_dt(struct platform_device *pdev,
 
 	of_property_read_u32(np, "snps,ps-speed", &plat->mac_port_sel_speed);
 
-	plat->axi = stmmac_axi_setup(pdev);
+	plat->axi = stmmac_axi_setup(pdev, res);
 
 	rc = stmmac_mtl_setup(pdev, plat);
 	if (rc) {

-- 
2.43.2

[PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234 MGBE

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Allow the device to use bursts and increase the maximum number of
outstanding requests to improve performance. Measurements show an
increase in throughput of around 5x on a 1 Gbps link.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
index bab57d1675df..b6bfa48f279d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
@@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 }
 
+static const struct stmmac_axi tegra234_mgbe_axi = {
+	.axi_wr_osr_lmt = 63,
+	.axi_rd_osr_lmt = 63,
+	.axi_blen = { 256, },
+};
+
 static int tegra_mgbe_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat;
@@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto disable_clks;
 
+	/* setup default AXI configuration */
+	res.axi = &tegra234_mgbe_axi;
+
 	plat = devm_stmmac_probe_config_dt(pdev, &res);
 	if (IS_ERR(plat)) {
 		err = PTR_ERR(plat);

-- 
2.43.2

Re: [PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234 MGBE

[Serge Semin]

On Mon, Feb 19, 2024 at 05:46:06PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Allow the device to use bursts and increase the maximum number of
> outstanding requests to improve performance. Measurements show an
> increase in throughput of around 5x on a 1 Gbps link.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> index bab57d1675df..b6bfa48f279d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> @@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
>  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>  }
>  
> +static const struct stmmac_axi tegra234_mgbe_axi = {
> +	.axi_wr_osr_lmt = 63,
> +	.axi_rd_osr_lmt = 63,
> +	.axi_blen = { 256, },
> +};
> +
>  static int tegra_mgbe_probe(struct platform_device *pdev)
>  {
>  	struct plat_stmmacenet_data *plat;
> @@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto disable_clks;
>  
> +	/* setup default AXI configuration */
> +	res.axi = &tegra234_mgbe_axi;
> +
>  	plat = devm_stmmac_probe_config_dt(pdev, &res);
>  	if (IS_ERR(plat)) {
>  		err = PTR_ERR(plat);

Let's get back to the v2 discussion:

On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> The entire series can be converted to just a few lines of change:
>     plat = devm_stmmac_probe_config_dt(pdev, res.mac);
>     if (IS_ERR(plat)) {
>             err = PTR_ERR(plat);
>             goto disable_clks;
>     }
> +
> +   if (IS_ERR_OR_NULL(plat->axi)) {
> +           plat->axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> +           if (!plat->axi) {
> +                   ret = -ENOMEM;
> +                   goto disable_clks;
> +           }
> +   } /* else memset plat->axi with zeros if you wish */
> +
> +   plat->axi->axi_wr_osr_lmt = 63;
> +   plat->axi->axi_rd_osr_lmt = 63;
> +   plat->axi->axi_blen[0] = 256;
>  
>     plat->has_xgmac = 1;
>     plat->flags |= STMMAC_FLAG_TSO_EN;
>     plat->pmt = 1;
>
> Please don't overcomplicate the already overcomplicated driver with a
> functionality which can be reached by the default one. In this case
> the easiest way is to let the generic code work and then
> override/replace/fix/etc the retrieved values. Thus there won't be
> need in adding the redundant functionality and keep the generic
> DT-platform code a bit simpler to read.

You responded with:

On Tue, Feb 13, 2024 at 04:51:34PM +0100, Thierry Reding wrote:
> I'm not sure I understand how this is overcomplicating things. The code
> is pretty much unchanged, except that the AXI configuration can now have
> driver-specified defaults before the DT is parsed. Perhaps I need to add
> comments to make that a bit clearer?
> 
> While your version is certainly simpler it has the drawback that it no
> longer allows the platform defaults to be overridden in device tree. I
> would prefer if the defaults can be derived from the compatible string
> but if need be for those defaults to still be overridable from device
> tree.

Currently available functionality is easier to read and understand: by
default the data is retrieved from the DT, if no AXI DT-node found you
can allocate/create your own AXI-configs, if there is AXI DT-node you
can fix it up in whatever way your wish. Thus the default behavior is
straightforward. You on the contrary suggest to add an additional
field to the resources structure which would need to be merged in with
the data retrieved from DT. It makes the stmmac_axi_setup() method and
the entire logic more complex and thus harder to comprehend. The
driver is already overwhelmed with flags and private/platform data
fixing the code here and there (see plat_stmmacenet_data, it's a
madness). So please justify in more details why do you need one more
complexity added instead of:
1. overriding the AXI-configs retrieved from DT,
2. updating DT on your platform
?

-Serge(y)

> 
> -- 
> 2.43.2
> 

Re: [PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234 MGBE

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5973 bytes --]

On Mon Feb 19, 2024 at 7:32 PM CET, Serge Semin wrote:
> On Mon, Feb 19, 2024 at 05:46:06PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Allow the device to use bursts and increase the maximum number of
> > outstanding requests to improve performance. Measurements show an
> > increase in throughput of around 5x on a 1 Gbps link.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > index bab57d1675df..b6bfa48f279d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > @@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
> >  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> >  }
> >  
> > +static const struct stmmac_axi tegra234_mgbe_axi = {
> > +	.axi_wr_osr_lmt = 63,
> > +	.axi_rd_osr_lmt = 63,
> > +	.axi_blen = { 256, },
> > +};
> > +
> >  static int tegra_mgbe_probe(struct platform_device *pdev)
> >  {
> >  	struct plat_stmmacenet_data *plat;
> > @@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
> >  	if (err < 0)
> >  		goto disable_clks;
> >  
> > +	/* setup default AXI configuration */
> > +	res.axi = &tegra234_mgbe_axi;
> > +
> >  	plat = devm_stmmac_probe_config_dt(pdev, &res);
> >  	if (IS_ERR(plat)) {
> >  		err = PTR_ERR(plat);
>
> Let's get back to the v2 discussion:
>
> On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> > The entire series can be converted to just a few lines of change:
> >     plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> >     if (IS_ERR(plat)) {
> >             err = PTR_ERR(plat);
> >             goto disable_clks;
> >     }
> > +
> > +   if (IS_ERR_OR_NULL(plat->axi)) {
> > +           plat->axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> > +           if (!plat->axi) {
> > +                   ret = -ENOMEM;
> > +                   goto disable_clks;
> > +           }
> > +   } /* else memset plat->axi with zeros if you wish */
> > +
> > +   plat->axi->axi_wr_osr_lmt = 63;
> > +   plat->axi->axi_rd_osr_lmt = 63;
> > +   plat->axi->axi_blen[0] = 256;
> >  
> >     plat->has_xgmac = 1;
> >     plat->flags |= STMMAC_FLAG_TSO_EN;
> >     plat->pmt = 1;
> >
> > Please don't overcomplicate the already overcomplicated driver with a
> > functionality which can be reached by the default one. In this case
> > the easiest way is to let the generic code work and then
> > override/replace/fix/etc the retrieved values. Thus there won't be
> > need in adding the redundant functionality and keep the generic
> > DT-platform code a bit simpler to read.
>
> You responded with:
>
> On Tue, Feb 13, 2024 at 04:51:34PM +0100, Thierry Reding wrote:
> > I'm not sure I understand how this is overcomplicating things. The code
> > is pretty much unchanged, except that the AXI configuration can now have
> > driver-specified defaults before the DT is parsed. Perhaps I need to add
> > comments to make that a bit clearer?
> > 
> > While your version is certainly simpler it has the drawback that it no
> > longer allows the platform defaults to be overridden in device tree. I
> > would prefer if the defaults can be derived from the compatible string
> > but if need be for those defaults to still be overridable from device
> > tree.
>
> Currently available functionality is easier to read and understand: by
> default the data is retrieved from the DT, if no AXI DT-node found you
> can allocate/create your own AXI-configs, if there is AXI DT-node you
> can fix it up in whatever way your wish. Thus the default behavior is
> straightforward. You on the contrary suggest to add an additional
> field to the resources structure which would need to be merged in with
> the data retrieved from DT. It makes the stmmac_axi_setup() method and
> the entire logic more complex and thus harder to comprehend.

I suppose that's subjective. Being able to let the driver provide
defaults that can then be overridden by values from DT doesn't seem like
a very exotic (or complicated) feature to me. We do that elsewhere all
the time. Do the comments that I added in this version not sufficiently
explain what's going on?

> The driver is already overwhelmed with flags and private/platform data
> fixing the code here and there (see plat_stmmacenet_data, it's a
> madness). So please justify in more details why do you need one more
> complexity added instead of:
> 1. overriding the AXI-configs retrieved from DT,

Again, overriding the AXI configs read from DT doesn't keep the current
default behaviour of DT being the final authority. That's a policy that
should remain intact. This patch (series) is about allowing the driver
to override the AXI defaults with something that's sensible based on
the compatible string. The current defaults, for example, cause the GBE
on Tegra devices to run at around 100 Mbps even on a 1 Gbps link.

> 2. updating DT on your platform

That's one possibility and was in fact the first variant I used, but it
has a few drawbacks. For example, it means that I need to create the AXI
node just to make the device functional, but if possible it's better to
derive all necessary information from the compatible string. Having this
in a separate AXI configuration node is duplicating information that's
already implied by the compatible string.

Also, on Tegra we have a few instances of this device that are all
configured the same way. Since the AXI configuration node is supposed to
be a child of the Ethernet controller node, we end up having to
duplicate even more information.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234 MGBE

[Serge Semin]

On Tue, Feb 20, 2024 at 03:28:39PM +0100, Thierry Reding wrote:
> On Mon Feb 19, 2024 at 7:32 PM CET, Serge Semin wrote:
> > On Mon, Feb 19, 2024 at 05:46:06PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Allow the device to use bursts and increase the maximum number of
> > > outstanding requests to improve performance. Measurements show an
> > > increase in throughput of around 5x on a 1 Gbps link.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > index bab57d1675df..b6bfa48f279d 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > @@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
> > >  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > >  }
> > >  
> > > +static const struct stmmac_axi tegra234_mgbe_axi = {
> > > +	.axi_wr_osr_lmt = 63,
> > > +	.axi_rd_osr_lmt = 63,
> > > +	.axi_blen = { 256, },
> > > +};
> > > +
> > >  static int tegra_mgbe_probe(struct platform_device *pdev)
> > >  {
> > >  	struct plat_stmmacenet_data *plat;
> > > @@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
> > >  	if (err < 0)
> > >  		goto disable_clks;
> > >  
> > > +	/* setup default AXI configuration */
> > > +	res.axi = &tegra234_mgbe_axi;
> > > +
> > >  	plat = devm_stmmac_probe_config_dt(pdev, &res);
> > >  	if (IS_ERR(plat)) {
> > >  		err = PTR_ERR(plat);
> >
> > Let's get back to the v2 discussion:
> >
> > On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> > > The entire series can be converted to just a few lines of change:
> > >     plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> > >     if (IS_ERR(plat)) {
> > >             err = PTR_ERR(plat);
> > >             goto disable_clks;
> > >     }
> > > +
> > > +   if (IS_ERR_OR_NULL(plat->axi)) {
> > > +           plat->axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> > > +           if (!plat->axi) {
> > > +                   ret = -ENOMEM;
> > > +                   goto disable_clks;
> > > +           }
> > > +   } /* else memset plat->axi with zeros if you wish */
> > > +
> > > +   plat->axi->axi_wr_osr_lmt = 63;
> > > +   plat->axi->axi_rd_osr_lmt = 63;
> > > +   plat->axi->axi_blen[0] = 256;
> > >  
> > >     plat->has_xgmac = 1;
> > >     plat->flags |= STMMAC_FLAG_TSO_EN;
> > >     plat->pmt = 1;
> > >
> > > Please don't overcomplicate the already overcomplicated driver with a
> > > functionality which can be reached by the default one. In this case
> > > the easiest way is to let the generic code work and then
> > > override/replace/fix/etc the retrieved values. Thus there won't be
> > > need in adding the redundant functionality and keep the generic
> > > DT-platform code a bit simpler to read.
> >
> > You responded with:
> >
> > On Tue, Feb 13, 2024 at 04:51:34PM +0100, Thierry Reding wrote:
> > > I'm not sure I understand how this is overcomplicating things. The code
> > > is pretty much unchanged, except that the AXI configuration can now have
> > > driver-specified defaults before the DT is parsed. Perhaps I need to add
> > > comments to make that a bit clearer?
> > > 
> > > While your version is certainly simpler it has the drawback that it no
> > > longer allows the platform defaults to be overridden in device tree. I
> > > would prefer if the defaults can be derived from the compatible string
> > > but if need be for those defaults to still be overridable from device
> > > tree.
> >
> > Currently available functionality is easier to read and understand: by
> > default the data is retrieved from the DT, if no AXI DT-node found you
> > can allocate/create your own AXI-configs, if there is AXI DT-node you
> > can fix it up in whatever way your wish. Thus the default behavior is
> > straightforward. You on the contrary suggest to add an additional
> > field to the resources structure which would need to be merged in with
> > the data retrieved from DT. It makes the stmmac_axi_setup() method and
> > the entire logic more complex and thus harder to comprehend.
> 
> I suppose that's subjective. Being able to let the driver provide
> defaults that can then be overridden by values from DT doesn't seem like
> a very exotic (or complicated) feature to me. We do that elsewhere all
> the time. Do the comments that I added in this version not sufficiently
> explain what's going on?

I have perfectly understood what was going on since v1. My concern is
the implementation. Here is the way the platform-specific setup
currently works.

There are two structures: stmmac_resources and plat_stmmacenet_data.
The former one contains the generic platform resources like CSRs
mapping, IRQs and MAC-address. The later one mainly has the DW
MAC-specific settings. Yes, plat_stmmacenet_data has been evolved to
an ugly monster with many redundant flags (fixing code and data here
and there in the driver) and with some generic platform resources
(which should have been added to the stmmac_resources structure in the
first place, like clocks and resets). But still it's purpose is
more-or-less defined. Both of these structures can be filled in with
data either directly by the glue drivers or by calling the
ready-to-use DW MAC platform data getters (stmmac_probe_config_dt()
and stmmac_get_platform_resources()). Most importantly is that
currently these structures have independent init semantics: no common
data, no fields used in both contexts. There are tons of problematic
places or questionable implementations in the driver, but at least
this one is more-or-less defined: coherency is minimal, logic is
linear.

You suggest to break that logic by introducing a new stmmac_resources
field which doesn't represent a generic resource data, but which
purpose is to tweak the AXI-settings in the plat_stmmacenet_data
structure. The pointer won't be even ever initialized in the
stmmac_get_platform_resources() method because it's done in the
stmmac_probe_config_dt() function. Based on all of that the change you
suggest look more like a fixup of the problem with your particular
device/platform.

Let's assume you patches are accepted. In sometime an another
developer comes with a need to pre-define say MTL Tx/Rx queue configs,
then another one with DMA configs, MDIO-bus settings, and so on. Thus
the stmmac_resources structure will eventually turn in a set of the
tweaks and the plat_stmmacenet_data pre-defines. That's how the
plat_stmmacenet_data structure has turned in what it is now. This
doesn't look like a right path to take again.

> 
> > The driver is already overwhelmed with flags and private/platform data
> > fixing the code here and there (see plat_stmmacenet_data, it's a
> > madness). So please justify in more details why do you need one more
> > complexity added instead of:
> > 1. overriding the AXI-configs retrieved from DT,
> 
> Again, overriding the AXI configs read from DT doesn't keep the current
> default behaviour of DT being the final authority. That's a policy that
> should remain intact. This patch (series) is about allowing the driver
> to override the AXI defaults with something that's sensible based on
> the compatible string. The current defaults, for example, cause the GBE
> on Tegra devices to run at around 100 Mbps even on a 1 Gbps link.
> 
> > 2. updating DT on your platform
> 

> That's one possibility and was in fact the first variant I used, but it
> has a few drawbacks. For example, it means that I need to create the AXI
> node just to make the device functional, but if possible it's better to
> derive all necessary information from the compatible string. Having this
> in a separate AXI configuration node is duplicating information that's
> already implied by the compatible string.
> 
> Also, on Tegra we have a few instances of this device that are all
> configured the same way. Since the AXI configuration node is supposed to
> be a child of the Ethernet controller node, we end up having to
> duplicate even more information.

None of that sounds like big problems. The default behavior doesn't
make your devices not-working. Yes, the performance is poor, but they
still work. Regarding the AXI-config DT-nodes it's not a problem at
all. A lot of the DW *MAC instances currently have the AXI-config
DT-subnodes. It's absolutely fine to have them setting up the same
configs.

Once again having the pre-defined configs is fine. All I am worried
about is the implementation you suggest especially in using the
stmmac_resources structure to tweak up the plat_stmmacenet_data data.
So the easiest solutions in your case are:
1. Initialize the plat_stmmacenet_data->axi pointer if no AXI
DT-subnode was detected by the stmmac_probe_config_dt() method, after
the method is called. This will provide almost the same semantics as
you suggest. The only difference is that it would work not on the
property level but on the node-level one. (Note the implementation
suggested by you doesn't provide the AXI-configs pre-definition for
the boolean properties. So it doesn't provide a complete AXI-config
default pre-definition.)
2. Add the proper AXI-config DT-subnodes to the respective device tree
sources.

If despite of all my reasoning you still insist on having a
pre-defined setting pattern, then we'll need to come up with some
better solution. On the top of my mind it might be for example a
pre-definition of the entire plat_stmmacenet_data structure instance
or using the software nodes.

-Serge(y)

> 
> Thierry

Re: [PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234 MGBE

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11408 bytes --]

On Fri Feb 23, 2024 at 11:24 PM CET, Serge Semin wrote:
> On Tue, Feb 20, 2024 at 03:28:39PM +0100, Thierry Reding wrote:
> > On Mon Feb 19, 2024 at 7:32 PM CET, Serge Semin wrote:
> > > On Mon, Feb 19, 2024 at 05:46:06PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Allow the device to use bursts and increase the maximum number of
> > > > outstanding requests to improve performance. Measurements show an
> > > > increase in throughput of around 5x on a 1 Gbps link.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > > index bab57d1675df..b6bfa48f279d 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> > > > @@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
> > > >  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> > > >  }
> > > >  
> > > > +static const struct stmmac_axi tegra234_mgbe_axi = {
> > > > +	.axi_wr_osr_lmt = 63,
> > > > +	.axi_rd_osr_lmt = 63,
> > > > +	.axi_blen = { 256, },
> > > > +};
> > > > +
> > > >  static int tegra_mgbe_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct plat_stmmacenet_data *plat;
> > > > @@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
> > > >  	if (err < 0)
> > > >  		goto disable_clks;
> > > >  
> > > > +	/* setup default AXI configuration */
> > > > +	res.axi = &tegra234_mgbe_axi;
> > > > +
> > > >  	plat = devm_stmmac_probe_config_dt(pdev, &res);
> > > >  	if (IS_ERR(plat)) {
> > > >  		err = PTR_ERR(plat);
> > >
> > > Let's get back to the v2 discussion:
> > >
> > > On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> > > > The entire series can be converted to just a few lines of change:
> > > >     plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> > > >     if (IS_ERR(plat)) {
> > > >             err = PTR_ERR(plat);
> > > >             goto disable_clks;
> > > >     }
> > > > +
> > > > +   if (IS_ERR_OR_NULL(plat->axi)) {
> > > > +           plat->axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> > > > +           if (!plat->axi) {
> > > > +                   ret = -ENOMEM;
> > > > +                   goto disable_clks;
> > > > +           }
> > > > +   } /* else memset plat->axi with zeros if you wish */
> > > > +
> > > > +   plat->axi->axi_wr_osr_lmt = 63;
> > > > +   plat->axi->axi_rd_osr_lmt = 63;
> > > > +   plat->axi->axi_blen[0] = 256;
> > > >  
> > > >     plat->has_xgmac = 1;
> > > >     plat->flags |= STMMAC_FLAG_TSO_EN;
> > > >     plat->pmt = 1;
> > > >
> > > > Please don't overcomplicate the already overcomplicated driver with a
> > > > functionality which can be reached by the default one. In this case
> > > > the easiest way is to let the generic code work and then
> > > > override/replace/fix/etc the retrieved values. Thus there won't be
> > > > need in adding the redundant functionality and keep the generic
> > > > DT-platform code a bit simpler to read.
> > >
> > > You responded with:
> > >
> > > On Tue, Feb 13, 2024 at 04:51:34PM +0100, Thierry Reding wrote:
> > > > I'm not sure I understand how this is overcomplicating things. The code
> > > > is pretty much unchanged, except that the AXI configuration can now have
> > > > driver-specified defaults before the DT is parsed. Perhaps I need to add
> > > > comments to make that a bit clearer?
> > > > 
> > > > While your version is certainly simpler it has the drawback that it no
> > > > longer allows the platform defaults to be overridden in device tree. I
> > > > would prefer if the defaults can be derived from the compatible string
> > > > but if need be for those defaults to still be overridable from device
> > > > tree.
> > >
> > > Currently available functionality is easier to read and understand: by
> > > default the data is retrieved from the DT, if no AXI DT-node found you
> > > can allocate/create your own AXI-configs, if there is AXI DT-node you
> > > can fix it up in whatever way your wish. Thus the default behavior is
> > > straightforward. You on the contrary suggest to add an additional
> > > field to the resources structure which would need to be merged in with
> > > the data retrieved from DT. It makes the stmmac_axi_setup() method and
> > > the entire logic more complex and thus harder to comprehend.
> > 
> > I suppose that's subjective. Being able to let the driver provide
> > defaults that can then be overridden by values from DT doesn't seem like
> > a very exotic (or complicated) feature to me. We do that elsewhere all
> > the time. Do the comments that I added in this version not sufficiently
> > explain what's going on?
>
> I have perfectly understood what was going on since v1. My concern is
> the implementation. Here is the way the platform-specific setup
> currently works.
>
> There are two structures: stmmac_resources and plat_stmmacenet_data.
> The former one contains the generic platform resources like CSRs
> mapping, IRQs and MAC-address. The later one mainly has the DW
> MAC-specific settings. Yes, plat_stmmacenet_data has been evolved to
> an ugly monster with many redundant flags (fixing code and data here
> and there in the driver) and with some generic platform resources
> (which should have been added to the stmmac_resources structure in the
> first place, like clocks and resets). But still it's purpose is
> more-or-less defined. Both of these structures can be filled in with
> data either directly by the glue drivers or by calling the
> ready-to-use DW MAC platform data getters (stmmac_probe_config_dt()
> and stmmac_get_platform_resources()). Most importantly is that
> currently these structures have independent init semantics: no common
> data, no fields used in both contexts. There are tons of problematic
> places or questionable implementations in the driver, but at least
> this one is more-or-less defined: coherency is minimal, logic is
> linear.
>
> You suggest to break that logic by introducing a new stmmac_resources
> field which doesn't represent a generic resource data, but which
> purpose is to tweak the AXI-settings in the plat_stmmacenet_data
> structure. The pointer won't be even ever initialized in the
> stmmac_get_platform_resources() method because it's done in the
> stmmac_probe_config_dt() function. Based on all of that the change you
> suggest look more like a fixup of the problem with your particular
> device/platform.
>
> Let's assume you patches are accepted. In sometime an another
> developer comes with a need to pre-define say MTL Tx/Rx queue configs,
> then another one with DMA configs, MDIO-bus settings, and so on. Thus
> the stmmac_resources structure will eventually turn in a set of the
> tweaks and the plat_stmmacenet_data pre-defines. That's how the
> plat_stmmacenet_data structure has turned in what it is now. This
> doesn't look like a right path to take again.
>
> > 
> > > The driver is already overwhelmed with flags and private/platform data
> > > fixing the code here and there (see plat_stmmacenet_data, it's a
> > > madness). So please justify in more details why do you need one more
> > > complexity added instead of:
> > > 1. overriding the AXI-configs retrieved from DT,
> > 
> > Again, overriding the AXI configs read from DT doesn't keep the current
> > default behaviour of DT being the final authority. That's a policy that
> > should remain intact. This patch (series) is about allowing the driver
> > to override the AXI defaults with something that's sensible based on
> > the compatible string. The current defaults, for example, cause the GBE
> > on Tegra devices to run at around 100 Mbps even on a 1 Gbps link.
> > 
> > > 2. updating DT on your platform
> > 
>
> > That's one possibility and was in fact the first variant I used, but it
> > has a few drawbacks. For example, it means that I need to create the AXI
> > node just to make the device functional, but if possible it's better to
> > derive all necessary information from the compatible string. Having this
> > in a separate AXI configuration node is duplicating information that's
> > already implied by the compatible string.
> > 
> > Also, on Tegra we have a few instances of this device that are all
> > configured the same way. Since the AXI configuration node is supposed to
> > be a child of the Ethernet controller node, we end up having to
> > duplicate even more information.
>
> None of that sounds like big problems. The default behavior doesn't
> make your devices not-working. Yes, the performance is poor, but they
> still work. Regarding the AXI-config DT-nodes it's not a problem at
> all. A lot of the DW *MAC instances currently have the AXI-config
> DT-subnodes. It's absolutely fine to have them setting up the same
> configs.

But that's precisely my issue. In my opinion the default behavior for a
device should be to be able to run at optimal performance without having
to resort to extra quirks.

All of the AXI configuration parameters can be derived from the DT
compatible string, so it shouldn't be necessary to duplicate that
information in an additional AXI configuration node.

> Once again having the pre-defined configs is fine. All I am worried
> about is the implementation you suggest especially in using the
> stmmac_resources structure to tweak up the plat_stmmacenet_data data.
> So the easiest solutions in your case are:
> 1. Initialize the plat_stmmacenet_data->axi pointer if no AXI
> DT-subnode was detected by the stmmac_probe_config_dt() method, after
> the method is called. This will provide almost the same semantics as
> you suggest. The only difference is that it would work not on the
> property level but on the node-level one. (Note the implementation
> suggested by you doesn't provide the AXI-configs pre-definition for
> the boolean properties. So it doesn't provide a complete AXI-config
> default pre-definition.)

Quite frankly that would not be useful at all. If there's no way to
override defaults individually this becomes very brittle. If somebody
wanted to override just a single parameter, they would need to reproduce
the entire AXI configuration node, using an exact copy of the defaults
for the parameters that they don't want to change.

> 2. Add the proper AXI-config DT-subnodes to the respective device tree
> sources.
>
> If despite of all my reasoning you still insist on having a
> pre-defined setting pattern, then we'll need to come up with some
> better solution. On the top of my mind it might be for example a
> pre-definition of the entire plat_stmmacenet_data structure instance
> or using the software nodes.

That sounds very impractical. We'd need to make sure to deep-copy the
pre-definition structure and such, then deal with reference counting of
the clocks, resets and whatever else.

I'll drop this series and stick with AXI configuration nodes like
everyone else.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]